Hi, we are trying to update to the version 3.0.0 and are running into an annoying issue. It seems that no matter what we set the permissions of our encryption keys to, our log ends up being filled with the following error/notice:

User notice: Key file {file name} permissions are not correct, should be 600 or 660

First, it seems that reading the key files requires execute permissions even though the error message says otherwise. We initially had the permissions as 777 and then the code on line 43 of Server\ResourceServer.php (realpath($public_key)) could at least find the file and return a proper file path. But if it's not 600 or 660 CryptKey.php from oauth2-server triggers those notices.

Second, if we change the permissions to 600 or 660 on those files, then the call to realpath($public_key) mentioned above returns false and you still get the same notices relating to permissions. I've tried all kinds of variations of who the owner of the key files is, such as www-data, user 1000, and root.

Is anyone else running into this?

The only mention I've seen of this issue is here in the oauth2-server repo: https://github.com/thephpleague/oauth2-server/issues/779

Some notes about our environment. We are using docker and the official drupal docker image and I am personally on a Windows 10 Home machine using docker toolbox.

CommentFileSizeAuthor
#15 2915576-oauth2-server-crypt-key-allow-444.patch707 bytesgdev2018
#12 2915576-crypt-key-allow-440-do-not-test.patch726 bytessime

Comments

scrumorg created an issue. See original summary.

thebigmacmurray’s picture

thebigmacmurray commented

Having the same issue after updating to ^3.0 to satisfy acquia/lightning update requirements.

bloke_zero’s picture

bloke_zero commented

Same issue here with lightning.

lucas.constantino’s picture

lucas.constantino commented

Same

lhguerra’s picture

lhguerra commented

Same here. I hacked the module to comment this verification so I can work, but I don't know how to address this issue for production.

I also find it a bit odd throwing an error inside the constructor.

greedesign’s picture

greedesign commented

same

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Can everyone confirm that they're getting the notice even after changing the permissions to the allowed values?

bloke_zero’s picture

bloke_zero commented

@e0ipso don't believe that is the root issue and that the creator of the upstream code has changed approach:

https://github.com/laravel/passport/pull/454 and here: https://github.com/thephpleague/oauth2-server/pull/776

There are a number of cases where setting the permissions correctly/automatically is difficult or impossible.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

@bloke_zero yeah, I'm aware of that. What I want to know if this is a problem here or in the upstream library. If the problem still exists with the expected permissions, we'll need to look into it. If not, then it should be fixed upstream.

  1. Do you see to notice after changing the correct permissions?
  2. What are the scenarios where you don't have the ability to set the permissions correctly that you are thinking?
bloke_zero’s picture

bloke_zero commented

@e0ipso - ok cool - the issue for me is running Lightning distribution and Vagrant - the error is unclear as it doesn't say where the file is. I did look into the issue a bit but ran out of time - I'll see what I can discover today.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Oh, I'm unfamiliar with the Lightning distribution. Probably worth moving this to their queue as well.

sime’s picture

sime
any/any
Location Melbourne
commented
StatusFileSize
new 2915576-crypt-key-allow-440-do-not-test.patch726 bytes

The attached patch (on `league/oauth2-server`) might help in the meantime, without removing the implied security. Seems like Acquia hosting sets my the key file in ../keys to be 440 which is more secure, so I just added that option.

Probably won't get committed but the PR is here https://github.com/thephpleague/oauth2-server/pull/839

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Thanks for the update @sime!

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Active » Closed (works as designed)

Thanks to @sime this will no longer be a problem in the future. His PR got merged upstream!

https://github.com/thephpleague/oauth2-server/pull/839

gdev2018’s picture

gdev2018 commented
StatusFileSize
new 2915576-oauth2-server-crypt-key-allow-444.patch707 bytes

Here are the patch fixes file permissions 444

"patches": {
"league/oauth2-server": {
"Key file permissions are not correct, should be 600 or 660": "patches/2915576-oauth2-server-crypt-key-allow-444.patch"
}
}