Implement nonce in OIDC

Thanks for opening this. Is there any chance you can provide a patch for this?

🙏

No worries, I am sure that if this affects many people we will get a patch eventually.

Thanks for opening the issue in the first place.

Per https://openid.net/specs/openid-connect-basic-1_0.html#RequestParameters this is optional:

"Use of the nonce is OPTIONAL when using the code flow."

So this appears to be a feature request; I also was only tangentially involved in the OIDC implementation issue so I'm not sure if this was considered and not implemented for any particular reason. It might also be unnecessary given other mitigating controls, I'd have to read the spec closer.

https://git.drupalcode.org/search?search=nonce&group_id=2&project_id=523...

Hello, i would like to point that nonce is optional for the openid client implementation (client libraries do not need to provide a nonce parameter) but as the documentation clearly says it MUST return a token when the nonce is present.

Client libraries which are openid compliant are not compatible because are providing the nonce token, like angular-auth-oidc-client.
Kind regards and many thanks for your module.

Ah! Thanks for pointing this out. You are right!

Is there any chance either of you (@kamalw or @Pandepoulus) can work on a patch for this feature? Otherwise there is a big chance this will not get implemented, as we are spread too thin already and don't have a use case for our projects.

Hello, i did spend some time reading the spec, but i am quite noob to all this auth flow. If i didn't missunderstand, the only thing required is to take the "nonce" parameter in the request and sign it as a claim in the signed token_id provided.

So i think it might be done just adding the token from the request at hook_simple_oauth_private_claims_alter.

If you think that solution is ok i could provide the patch.

Kind regards.

Thanks for volunteering to help. I think in general, you're correct - it just needs to be included in the response to help protect against replay attacks. However I don't think the hook is necessary, since the module itself is providing the implementation.

The nonce should be included as claim in the id token. (not the access token)

So use of hook_simple_oauth_private_claims_alter will not help.

The correct place is: web/modules/contrib/simple_oauth/src/Normalizer/UserClaimsNormalizer.php:88

So we have:
1. to store somewhere the nonce received at the request of: /authorize
2. to return the nonce as a new claim.

Attached a patch - fully working, but obviously need more testing.
Mind that in a lack of better idea, where to store the nonce, it stores it with drupal cache (expired in hour).

Let me know your thoughts,

1. +++ b/src/Controller/Oauth2AuthorizeController.php
   @@ -162,6 +162,10 @@ class Oauth2AuthorizeController extends ControllerBase {
   +          \Drupal::cache()
   

   Use DI

2. +++ b/src/Controller/Oauth2AuthorizeController.php
   @@ -162,6 +162,10 @@ class Oauth2AuthorizeController extends ControllerBase {
   +            ->set($this->currentUser->id() . ":oauth_nonce", $request->query->get('nonce'), \Drupal::time()->getRequestTime() + 3600);
   

   I don't think this qualifies as securely binding the nonce to the client and user-agent. See https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#s...

More specifically to the last point, I think storing the nonce on the session would be sufficient/better than caching it.

This cannot be stored in session, since the user is not logged in, in terms of session cookie when passing token.

I repeat. I'm conscious that cache is not the most appropriate place to store the nonce, and we are open to suggestions.

One would argue that the most logical place would be the token entity itself in the DB.
I'm willing to make some progress on this, but would need some guidance.

This cannot be stored in session, since the user is not logged in, in terms of session cookie when passing token.

Anonymous users may still have sessions. E.g., how Commerce handles shopping carts for anonymous customers.

Let me shed some more light how you guys (the developers of this module) built this:

Background: The user is at some mobile app, and want to interact with Drupal site, but are unable to read HTML, so the data flow rely on API (json format). In order to make requests as a logged-in user, the user must obtain a token instead of the cookie based authentication mechanism built-in the Drupal. But the process must kick in with web technologies, so there is two requests.

1. The user click some button on the native application, which opens a light-web-browser.
2. First request to drupal at: /oauth/authorize - return HTML, possibly requests the user to log in. This light-web-browser is fully support cookies. So if a cookie is present at this light-web-browser, that refer an active session to a logged-in user, then a redirect to the application URL takes place (can be anything specified at "redirect URI" field in the consumer settings), If not, the user get a NEW session cookie, logging-in, the cookie is persistent between requests at the light-web-browser. Once the redirection took place, the application can close this light-web-browser. (still it's up to the device to respect the expiry in the session cookie, meaning, if tomorrow the same light-web-browser opens up, the cookie will be there, sent along to all requests to this domain).
3. Once the application picked up the code from the redirect URL, then it ables to make NATIVELY the second request.
4. Second request to drupal at: /oauth/token - returns JSON, with an access token. At this point the "Cookie" header is not present at all, and has no relation to the previously mentioned light-web-browser.

The e-commerce solutions rely on the fact that calls to the cart page is still coming from a web browser.

In our scenario, the requests comes from mobile device that simply doesn't read/write cookies.

meaning that we have no session whatsoever between the two requests.

Having said the above, I would like to iterate, that to my opinion, if it's possible to change the schema of the token entity, to hold one more property: nonce.

I also think storing the nonce should be handled differently. Having said that, this patch has helped us with getting the nonce in the ID token.

But the current patch will not add the nonce when you are redirected to the authorization form to give permission for the client. The attached patch solves that.

I updated the patch for 6.0.x

As said before, there must be a better way to store the nonce

Updating the target version - the MR I just created is against 6.0.x instead of 6.1.x.

Ok, went through this with GPT Sol, came up with a better flow that is part of the authorization flow, and eliminates a race from the previous patch version.

I'm going to test this on a production instance, to use as an IDP for a Headscale (Tailscale) network, will report back if this works...

Ok, tests pass now, and cleaned up the phpstan issues...

Confirmed that this is working for us now, with Headscale as a consumer -- without this patch, you cannot authenticate against Drupal/Simple_OAuth. With it, it appears to work fine!