Problem/Motivation

Simple OAuth requires you to create public.key and private.key files. The League/OAuth2 library it uses requires those to have permissions of 400, 440, 600, or 660. This can cause issues in rare situations where setting the files to one of those permissions isn't possible.

For example when using Pantheon, the module lets you generate the keys in the sites/default/files/private directory, and tries to set them to 600, but something on the host side is enforcing a different and non-compatible set of file permissions. If you know what you're doing, and are comfortable for whatever reason bypassing these permissions it would be nice if there was a setting to be able to do.

Proposed resolution

Add a service container configuration flag:

parameters:
  simple_oauth.config:
    bypass_key_permission_check_UNSAFE: true

This keeps the option relatively hidden, people will likely try and resolve it by doing the correct thing first and change file permissions. But, if that's not an option and they start digging into the code to see if there's a setting or something (like I did) they're will be. Woot!

Then, when it's set, anytime we use either the private or public key in a way that would cause \League\OAuth2\Server\CryptKey to trigger it's permissions check, instead create our own instance of \League\OAuth2\Server\CryptKey and set the "check permissions" constructor argument to false.

It looks like were we currently pass the path to the key file, you can also pass an instance of \League\OAuth2\Server\CryptKey instead.

CommentFileSizeAuthor
#15 3021054-15-bypass_permissions.patch3.96 KBkmonty
#13 3021054-13-bypass_permissions.patch3.95 KBkelvinwong
#10 3021054-10-bypass_permissions.patch4.3 KBe0ipso
#5 3021054-5-bypass_permissions.patch3.91 KBbradjones1
#2 3021054-2-bypass_permissions.patch5.87 KBeojthebrave

Issue fork simple_oauth-3021054

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

eojthebrave created an issue. See original summary.

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
StatusFileSize
new 3021054-2-bypass_permissions.patch5.87 KB

Assuming this is a reasonable idea ... here's a patch. Should we provide documentation for this in the README or elsewhere? Or just let people discover it on their own?

Anonymous’s picture

Anonymous (not verified) commented

This would be a nice improvement to make. Right now we're patching League/Oauth2 in order to mute these messages (we're on Pantheon, so in the same situation as the original reporter), but it would be nice if we could remove that patch and just let this module take care of it.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Active » Needs review

This issue definitely needs to be resolved. BTW, Laravel framework uses the same approach: https://github.com/laravel/passport/pull/454

bradjones1’s picture

bradjones1
Primary language English
commented
StatusFileSize
new 3021054-5-bypass_permissions.patch3.91 KB

This is a per-environment consideration (that is, it depends highly on the deployment context) and as such I think is a great use of the Settings API. This also keeps the diff smaller and maintains this as an expert setting that can be toggled in settings.php and not in the admin UI, which probably aids in security.

Alternative patch attached.

bradjones1’s picture

bradjones1
Primary language English
commented
Related issues: +#2915576: Key file permissions are not correct, should be 600 or 660
jmarcou’s picture

jmarcou commented
Status: Needs review » Reviewed & tested by the community

I reviewed and tested it.
It works properly and I think it can be now merged :)

I was having this issue using WSL and Docker Desktop for Windows, where OAuth Keys have -rwxr-xr-x (755) permissions by default and could not be set lower than -r-xr-xr-x (555). With the patch I can disable permissions check for my DEV env only and everything works fine.

briangonzalezmia’s picture

briangonzalezmia
Primary language English
Location Miami
commented

Also can confirm that this patch works. On pantheon as well and just ran into this bummer. Thanks for the help all!

jedgar1mx’s picture

jedgar1mx commented

I'm getting this error:
"Unable to create resource server. Make sure public and private keys are correctly configured."

However, I'm running version 4.3.0 so I can't apply the patch.

Was this already merged into 4.3.0?

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3021054-10-bypass_permissions.patch4.3 KB

I re-rolled this and renamed the setting name to make it more alarming.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Version: 8.x-3.x-dev » 8.x-4.x-dev
bradjones1’s picture

bradjones1
Primary language English
commented 
+++ b/README.md
@@ -50,6 +50,13 @@ Along with your access token, an authentication token is created. It's called th
+In `settings.php`:
+```$settings['simple_oauth.skip_key_permissions_check_UNSAFE'] = TRUE;```

I'm not trying to draw this out but I'm wondering if we should actually use a service parameter? That seems to be the better/"right" way of handling what we have been using $settings for, moving forward? I'm also not feeling strongly about this since it's a common pattern in Drupal and this is not deprecated in D9.

I understand wanting to call this out as "unsafe" however I don't really think it's truly a safety issue? Mostly this is pertinent in container environments where the ownership of mounted secrets may not match the web user, but it's not really an unsafe condition. Again, not wanting to delay this for semantics but I also don't want to scare people unnecessarily.

kelvinwong’s picture

kelvinwong commented
StatusFileSize
new 3021054-13-bypass_permissions.patch3.95 KB

Updated patch for 8.x-4.x-dev

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

+1 RBTC for the patch in comment #13.

Personally, I like the flexibility that controlling this via `settings.php` offers, as it can be more easily set per-environment (without needing something like config split).

I agree with @bradjones1 that writing _UNSAFE is misleading. Personally, I'd rather have why this could be considered unsafe written out in the README more clearly without throwing that into the setting name without explanation. For that reason, I'm in favor of patch in #13.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Version: 8.x-4.x-dev » 5.x-dev
StatusFileSize
new 3021054-15-bypass_permissions.patch3.96 KB

Updated the patch so it doesn't trigger any phpcs errors. Also confirming this works against the 5.x branch.

bradjones1 opened merge request !1

bradjones1’s picture

bradjones1
Primary language English
commented

Re-rolled for 5.x

jedgar1mx’s picture

jedgar1mx commented

I'm still getting error even with #13
Uncaught PHP Exception LogicException: "Unable to create resource server. Make sure public and private keys are correctly configured." at /mnt/www/html/detroitmi/docroot/modules/contrib/simple_oauth/src/Server/ResourceServer.php line 76 request_id="v-3675e366-513f-11eb-a23e-2365d6452622"

areebmoin’s picture

areebmoin commented

Unable to patch using #13 on 5.x Drupal 9.1.4 and PHP 7.3 (Acquia Dev Desktop)

patch "-p1" --no-backup-if-mismatch -d "web/modules/contrib/simple_oauth" < "patches\3021054-15-bypass_permissions.patch"

patching file README.md
patching file src/Plugin/Oauth2GrantManager.php
Hunk #1 succeeded at 13 (offset 1 line).
Hunk #2 FAILED at 142.
1 out of 2 hunks FAILED -- saving rejects to file src/Plugin/Oauth2GrantManager.php.rej
patching file src/Server/ResourceServer.php
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 46.
2 out of 2 hunks FAILED -- saving rejects to file src/Server/ResourceServer.php.rej

bradjones1’s picture

bradjones1
Primary language English
commented

The re-rolled patch is in the merge request.

bradjones1’s picture

bradjones1
Primary language English
commented
tarasich’s picture

tarasich
Primary language Ukrainian
Location Kyiv
commented
Status: Needs review » Reviewed & tested by the community

Tested latest version, works as expected. Code looks good.
Thank you!

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Thanks for everyone's patience on this. I have committed it now. I know this was annoying.

  • e0ipso committed 24ba44f on 5.x authored by eojthebrave 
    Issue #3021054 by bradjones1, eojthebrave, e0ipso, kmonty, KelvinWong,...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alina.basarabeanu’s picture

alina.basarabeanu commented

This is very confusing as the pull request is still open but the code exists in the dev version.

guardiola86’s picture

guardiola86 commented

The latest merge request patch didn't work for me with version 5.2.3, so I'm using dev version with the latest commit for now.