search

Drenso / symfony-oidc

This project contains the Symfony OIDC bundle, which is directly based on https://github.com/jumbojett/OpenID-Connect-PHP
Apache License 2.0
54 stars 32 forks source link

Azure token verification fails #60

Closed bresam closed 3 months ago

bresam commented 3 months ago

Hello everyone

I have discovered a problem with token verification with Azure. I don't know the reason yet and therefore I don't have a fix ready.

I have now built a workaround to move forward, like disable_nonce but for disable_token_verification. Token verification can be switched off optionally but is switched on by default.

Not validating the token is of course bad and not a real solution, but would you allow this config or not? Would you like this PR already or should I get back to you when/if I have a fix ready?

If any of you would like to have a look into that too, please feel free.

The error occurs on:

message = "Unable to verify JWT claims" file = "/var/www/app/vendor/drenso/symfony-oidc-bundle/src/OidcJwtHelper.php" line = 167

When OidcTokenType = ACCESS It fails on validating token signature

bobvandevijver commented 3 months ago

That error is not really descriptive, and I apologise for that. Can you try to install the dev-update-exception-texts version of this bundle? That should add more information to the message as to why the verification fails.

I am not a fan of adding the option to disable token verification fully (for security), so lets keep that out.

bresam commented 3 months ago

The bit more detailed message is: Unable to verify JWT claims - The token violates some mandatory constraints, details:

bobvandevijver commented 3 months ago

That would mean that the issuer as retrieved from the well known does not match with what is returned in the resulting JWT. That is an issue with your provider, not this bundle.

bresam commented 3 months ago

That may be possible, but it's Microsoft EntraID, the probably most used IdP. I will go into that a bit deeper today and try to figure out a bit more secure solution than disabling the whole token verification.

In case it's an unfixable issue with Microsoft EntraID, would you be fine with something like disable_issuer_validation to provide Microsoft as IdP?

bobvandevijver commented 3 months ago

No, as this is a configuration issue on your or your IdP side. And we already offer an option parse the well known information in case it is incorrect: https://github.com/Drenso/symfony-oidc?tab=readme-ov-file#parsing-well-known-information. You can use that to fix your issue, but you should first investigate what your well known endpoint is returning, and what is used in the token (use xdebug to find the value).

bresam commented 3 months ago

So you also don't want an integrated easy to configure solution for azure, even it's solved by a custom azure parser configurable on the client?

bobvandevijver commented 3 months ago

It shouldn't be easy to solve, as it is just wrong. The issuer must match with the information from the well-known, which is different to the nonce as that is optional according to the OIDC specification.

And if you are unable to solve the configuration issue, there is a way to override the well-known information if you are absolutely sure that is what you need.

tuxes3 commented 3 months ago

Just updated to ^3.0 and having the same error:

Authenticator failed. {"exception":"[object] (Drenso\\OidcBundle\\Security\\Exception\\OidcAuthenticationException(code: 0): Unable to verify JWT claims - The token violates some mandatory constraints, details:\n- The token was not issued by the given issuers at /var/www/vendor/drenso/symfony-oidc-bundle/src/OidcJwtHelper.php:177)\n[previous exception] [object] (Lcobucci\\JWT\\Validation\\RequiredConstraintsViolated(code: 0): The token violates some mandatory constraints, details:\n- The token was not issued by the given issuers at /var/www/vendor/lcobucci/jwt/src/Validation/RequiredConstraintsViolated.php:24)","authenticator":"Drenso\\OidcBundle\\Security\\OidcAuthenticator"} []

Switching back to version 2.13.1, and it works fine. The update should not break the working solution. There is no BC in the upgrade guide either.

bobvandevijver commented 3 months ago

Instead of only suggesting a downgrade to a version that is no longer actively maintained, it would be much more constructive to share what the actual token claims are (in both the id and access tokens) and what the well known indicates what it should be. Just shouting "it doesn't work" helps nobody.

tuxes3 commented 3 months ago

@bobvandevijver thanks for the information. I will debug this route and share any useful information.

tuxes3 commented 3 months ago

So I did some researching. In the OidcJwtHelper::verifyTokens method the $this->verifyToken method is called twice. On the first call both claims are valid: LooseValidAt and IssuedBy claim are correctly asserted.

It then goes into the second verifyToken where the claimed issuer somehow is null. (I do not understand this part of the code enough to judge if that is intended). Anyway, this then throws a ConstraintViolation which is not catched by catch (InvalidJwtTokenException).

@bobvandevijver Can you help me further how to continue on debugging?

bobvandevijver commented 3 months ago

Alright, thanks for that information. It seems your provider isn't adding the iss claim to the access token, and the implementations are not consistent.

Can you please check whether authentication succeeds with dev-update-access-token-validation?

bresam commented 3 months ago

with dev-update-access-token-validation it breaks like this:

image

bresam commented 3 months ago

Additional findings:

Regarding rfc: 3.1.3.8. Access Token Validation When using the Authorization Code Flow, if the ID Token contains an at_hash Claim, the Client MAY use it to validate the Access Token in the same manner as for the Implicit Flow, as defined in Section 3.2.2.9, but using the ID Token and Access Token returned from the Token Endpoint.

3.2.2.9. Access Token Validation To validate an Access Token issued from the Authorization Endpoint with an ID Token, the Client SHOULD do the following:

Hash the octets of the ASCII representation of the access_token with the hash algorithm specified in JWA [JWA] for the alg Header Parameter of the ID Token's JOSE Header. For instance, if the alg is RS256, the hash algorithm used is SHA-256. Take the left-most half of the hash and base64url-encode it. The value of at_hash in the ID Token MUST match the value produced in the previous step.

So the validation of access and id tokens should be different and a missmatch of iss and issuer on access token MUST not lead to an invalid token.

As i understand this in regard of this: 3.1.3.6. ID Token The contents of the ID Token are as described in Section 2. When using the Authorization Code Flow, these additional requirements for the following ID Token Claims apply:

at_hash OPTIONAL. Access Token hash value. Its value is the base64url encoding of the left-most half of the hash of the octets of the ASCII representation of the access_token value, where the hash algorithm used is the hash algorithm used in the alg Header Parameter of the ID Token's JOSE Header. For instance, if the alg is RS256, hash the access_token value with SHA-256, then take the left-most 128 bits and base64url-encode them. The at_hash value is a case-sensitive string.

The at_hash in id token is optional and if not existing on id token the access token is not validated.

@bobvandevijver Is this correct and do you see an easy fix?

bresam commented 3 months ago

And as i see in azure's well-known, at_hash would be available: "claims_supported": [ "sub", "iss", "cloud_instance_name", "cloud_instance_host_name", "cloud_graph_host_name", "msgraph_host", "aud", "exp", "iat", "auth_time", "acr", "nonce", "preferred_username", "name", "tid", "ver", "at_hash", "c_hash", "email" ],

But i think it's not claimed by the client on request

bresam commented 3 months ago

Just gone a bit deeper into code and created a pull request #61 which removes the wrong validation of access token. The correct validation of the access token is still missing.

tuxes3 commented 3 months ago

@bresam @bobvandevijver

I tested both of your solutions and having the same results:
Stacktrace with dev-update-access-token-validation

Lcobucci\JWT\Validation\NoConstraintsGiven:
No constraint given.

  at vendor/lcobucci/jwt/src/Validation/Validator.php:13
  at Lcobucci\JWT\Validation\Validator->assert(object(Plain))
     (vendor/drenso/symfony-oidc-bundle/src/OidcJwtHelper.php:197)
  at Drenso\OidcBundle\OidcJwtHelper->verifyToken('https://xxx.cloud/', 'https://xxx.cloud/.well-known/jwks.json', object(OidcTokenType), object(Plain), true, '<BEARER TOKEN>')
     (vendor/drenso/symfony-oidc-bundle/src/OidcJwtHelper.php:96)
  at Drenso\OidcBundle\OidcJwtHelper->verifyTokens('https://xxx.cloud/', 'https://xxx.cloud/.well-known/jwks.json', object(OidcTokens), true)
     (vendor/drenso/symfony-oidc-bundle/src/OidcClient.php:445)
  at Drenso\OidcBundle\OidcClient->verifyTokens(object(OidcTokens), true)
     (vendor/drenso/symfony-oidc-bundle/src/OidcClient.php:89)
  at Drenso\OidcBundle\OidcClient->authenticate(object(Request))
     (vendor/drenso/symfony-oidc-bundle/src/Security/OidcAuthenticator.php:63)
  at Drenso\OidcBundle\Security\OidcAuthenticator->authenticate(object(Request))
     (vendor/symfony/security-http/Authenticator/Debug/TraceableAuthenticator.php:70)
  at Symfony\Component\Security\Http\Authenticator\Debug\TraceableAuthenticator->authenticate(object(Request))
     (vendor/symfony/security-http/Authentication/AuthenticatorManager.php:176)
  at Symfony\Component\Security\Http\Authentication\AuthenticatorManager->executeAuthenticator(object(TraceableAuthenticator), object(Request))
     (vendor/symfony/security-http/Authentication/AuthenticatorManager.php:158)
  at Symfony\Component\Security\Http\Authentication\AuthenticatorManager->executeAuthenticators(array(object(TraceableAuthenticator)), object(Request))
     (vendor/symfony/security-http/Authentication/AuthenticatorManager.php:140)
  at Symfony\Component\Security\Http\Authentication\AuthenticatorManager->authenticateRequest(object(Request))
     (vendor/symfony/security-http/Firewall/AuthenticatorManagerListener.php:40)
  at Symfony\Component\Security\Http\Firewall\AuthenticatorManagerListener->authenticate(object(RequestEvent))
     (vendor/symfony/security-http/Authenticator/Debug/TraceableAuthenticatorManagerListener.php:68)
  at Symfony\Component\Security\Http\Authenticator\Debug\TraceableAuthenticatorManagerListener->authenticate(object(RequestEvent))
     (vendor/symfony/security-bundle/Debug/WrappedLazyListener.php:46)
  at Symfony\Bundle\SecurityBundle\Debug\WrappedLazyListener->authenticate(object(RequestEvent))
     (vendor/symfony/security-http/Firewall/AbstractListener.php:26)
  at Symfony\Component\Security\Http\Firewall\AbstractListener->__invoke(object(RequestEvent))
     (vendor/symfony/security-bundle/Debug/TraceableFirewallListener.php:83)
  at Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener->callListeners(object(RequestEvent), object(Generator))
     (vendor/symfony/security-http/Firewall.php:95)
  at Symfony\Component\Security\Http\Firewall->onKernelRequest(object(RequestEvent), 'kernel.request', object(TraceableEventDispatcher))
     (vendor/symfony/event-dispatcher/Debug/WrappedListener.php:116)
  at Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke(object(RequestEvent), 'kernel.request', object(TraceableEventDispatcher))
     (vendor/symfony/event-dispatcher/EventDispatcher.php:220)
  at Symfony\Component\EventDispatcher\EventDispatcher->callListeners(array(object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener)), 'kernel.request', object(RequestEvent))
     (vendor/symfony/event-dispatcher/EventDispatcher.php:56)
  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch(object(RequestEvent), 'kernel.request')
     (vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php:139)
  at Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch(object(RequestEvent), 'kernel.request')
     (vendor/symfony/http-kernel/HttpKernel.php:157)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:76)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:197)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (public/index.php:20)

With @bresam branch it works fine!

bobvandevijver commented 3 months ago

No constraint given.

This error has been fixed in the dev branch (it is what you get when you push something you haven't tested yet). If the issuer is indeed missing as mentioned by @tuxes3, you can try again and it should work now.

ACCESS Token iss is a different than in well-known

This is the real issue. The issuer must be equal to the one in the well known. OpenID is an extension of OAuth, and when JWT based access tokens are used, the issuer must be validated and match with the well known value: https://datatracker.ietf.org/doc/html/rfc9068#name-validating-jwt-access-token.

I have validated with KeyCloak and OpenConext, both set the same issuer on both token types.

So the question for @bresam is: why does your issuer change when both tokens are issued by the same issuer? Can you explain that?

The usage of JWT is not required, which is why this bundle checks whether it actually is a JWT, before it tries to validate.

The correct validation of the access token is still missing.

The at_hash is already validated when the claim has been provided.

https://github.com/Drenso/symfony-oidc/blob/e3f056ea7b19678d62fa792c56f9a252427905f7/src/OidcJwtHelper.php#L149

tuxes3 commented 3 months ago

I just tested it, and it works as intended! If you could merge and tag that, I will happily upgrade to 3.x

bresam commented 3 months ago

@bobvandevijver no i don't know why Microsoft uses another issuer for the access token. But please read the mentioned OIDC specs, there's nowhere described to validate the issuer on the Access Token. I think they are pretty clear how to validate the Access Token and Microsoft is still compliant with the OIDC specs. You are right, OIDC is based on OAuth, but for some reason there is a OIDC RFC which i would suggest to use when implementing OIDC.

I could imagine Microsoft uses separate servers for authentication and authorization which is possibly the reason for the different issuers but i don't know. Of course a small and simple infrastructure like a keycloak instance would never use or need to use different issuers. I could imagine that other cloud providers which are a bit bigger do it the same way, but i also don't know that.

Is there a reason why you don't want to be compliant with the OIDC specification? https://openid.net/specs/openid-connect-core-1_0.html

bobvandevijver commented 3 months ago

@bresam Please read the first line of the RFC and then read my previous comment again. OpenID is an layer on top of OAuth, and OAuth specifies that when a JWT is used as access token, its issuer must be validated.

bresam commented 3 months ago

I've read it, you too? Afterwards we can chat again. Fact is your solution does not work with Microsoft and the reason why is obvious when reading the oidc specs. so why we shouldn't talk about?

bobvandevijver commented 2 months ago

@tuxes3 https://github.com/Drenso/symfony-oidc/releases/tag/v3.1.0 contains the workaround you were looking for! Note that while it is actually out of spec for when a JWT access token is generated, we have decided to be lenient on missing claims for the sake of usability.