Removed chmod from CryptKey and add toggle to disable checking

[yannickl88]

First of all, please do not touch my files... I do not think it's the responsibility of a package to set file permissions. It is the responsibility of your deployment pipeline or developers themselves.

While I appreciate the intent that was done in 2f8de3d. This is breaking for us in both our development and our production environments. And according to #760 for more people.

With this PR I would like to propose two things:

• First and foremost, remove any code that modifies files. However, do trigger a USER_NOTICE when not set to 600.
• Option to skip the check. This can be useful for DEV environments.

A bit of context for our case:

Our PROD setup is as follows:
We have 4 users: deploy, worker, scheduler and apache. All four are part of the deployment group. So our code is chowned as deploy:deployment. So while 600 might seems like a good assumption, it will prevent our apache user from reading the key, thus breaking the web requests.

Our DEV setup is a bit different:
We have our own users, yannickl88 in my case, and www-data for the web user. Files are chmodded 666 so both users can read everything. (we have a umask 0000 everywhere). While not the best setup, it's just development. In this case 660 is even not enough, but a warning would be justified. However, we would like to silence this in our DEV environment since Symfony converts the USER_NOTICE to an exception, making the lib unusable.

[yannickl88]

If okay, I would like to loosen the permissions to 660 instead of 600 to support cases like ours (and that of #760)

[alexbilbie]

I'm happy to merge this if you could please remove the disabling of the check.

[alexbilbie]

Actually ignore my comment about disabling the check.

Instead I'd be happy with an API that looks something like this:

$privateKey = new CryptKey('file://path/to/private.key', 'passphrase');
$privateKey->acknowledgeIncorrectKeyPermissions(); // this disables the check

With regards to the 600 permission I'm also happy to see 600 or 660 as a valid file permission.

[yannickl88]

I would like to, but that might be a bit of a bigger change than possible. The check is done in the constructor. So in order for that code to work the validation of the key needs to be moved.

This will change the behavior quite a bit, because checking so will either be done by another class or when one of the public methods is called, changing the place where the error might be triggered (if at all).

[alexbilbie]

Okay I will re-evaluate the acknowledgeIncorrectKeyPermissions.

Please can you address the comments I've made on the commit and I will merge this PR.

[yannickl88]

which comments? The one about removing the disabling of the check?

[alexbilbie]

@yannickl88 see my comments in line here https://github.com/thephpleague/oauth2-server/pull/776/files

[yannickl88]

I'm not seeing any comments, did you publish them?

[alexbilbie]

Please change this to use === false instead of !

[alexbilbie]

Please change parameter to $keyPermissionsCheck

[alexbilbie]

Please use the explicit form of if ($keyPermissionsCheck === true)

[alexbilbie]

Apparently not...

[yannickl88]

np! should be fixed now.

[valeryan]

So any information on how one would get laravel passport to use this new toggle for the permission check? I just ran a composer update and now my whole dev environment has come crashing down. I am using bash on windows and you cannot chmod files because it ignores it. microsoft/WSL#81

[yannickl88]

There is a third constructor argument for the keys. Setting this to false will disable the check.

The AuthorizationServer has 2 arguments for the public and private keys which can be both a string or a CryptKey. So the easiest thing to do is use the CryptKey and make sure to set the third argument to false.
You do the same for the ResourceServer.

How to do this with Laravel passport I do not know, we use Symfony. I recommend making a PR there or try to extend it yourself to use this behavior.

[cringerjs]

I don't know if this is because I use the built in PHP development server, but my files in windows always report 0666, and folder report 0777. If specific couldn't we have the package automatically disable checking/modification using a php_uname('s') check?

[valeryan]

@cringer bash on windows does not support chmod of files that are stored on the ntfs file system. see microsoft/WSL#81

[cringerjs]

Yeah I get that. My point is that the package is attempting to check and/or modify the permissions on the keys. Since the specified values will always be reported back from a windows system the package will always crash and burn.

While a 3rd constructor has been added to disable this check I was just wondering why it doesn't do a check based on the OS anyway. As a cross platform package, it would seem like it would be a better solution to ignore checking and modification completely on Windows.

[valeryan]

@cringer I haven't actually dug into how they are doing the permissions check, maybe it does check the OS but because you are running bash for windows the script execution is happening in a *nix environment.

[cringerjs]

@valeryan I don't really think they are checking. If the check was done through PHP it a check to phpuname('s') would return the Operating System of the server running the php. It doesn't matter if the development server was started using command prompt or git bash this will always be Windows. Specifically php_uname('s') would return Windows NT.

[vibrantBits]

For backwards and crossplatform compatibility $keyPermissionsCheck should be false by default.