eclipse-vertx / vertx-auth

Apache License 2.0
166 stars 156 forks source link

Skip validation of opaque access tokens #609

Closed teodorakostova closed 1 year ago

teodorakostova commented 1 year ago

Motivation:

Add check for is_opaque_access_token flag in outh2 options, skip access validation if present and true. This fix resolves https://github.com/vert-x3/vertx-auth/issues/552

teodorakostova commented 1 year ago

Hi, pendula95, can you please check the new version of the fix?

pendula95 commented 1 year ago

@teodorakostova thank you for you effort. As I am not a maintainer of this repo I don't feel comfortable doing anything without @pmlopes opinion. From the code stand of view I would change couple of details in the PR regarding naming and small corrections in Tests also but....

In my opinion this is wrong way of handling the issue you are having as from what I am reading the issue you are having is a result of bad configuration and badly configured request (something to do with scopes) to Azure Graph API for Access Token. You are trying to use a token that is not issued for you app and this is why you are failing (Azure AD applies some custom logic in JWT header that is used internally by Graph API) as it thinks it is issuing token for internal usage, not usage by your app. I recommend that you read the posts that I have send previously and fix the issue without adding this feature. I have checked spring boot implementation and I don't see this feature there as option and what would you know people were also struggling with the same problem and found a way without changing the library. https://github.com/microsoft/azure-spring-boot/issues/857

On the other hand if @pmlopes agrees with you and sees this feature as a good thing I will gladly support you in merging this PR with few minor corrections.

narras-oss commented 1 year ago

@pmlopes @pendula95 We are not using Microsoft Graph APIs. We just configure Azure as a third party IDP and only care about the ID token that is issued. We actually don't use the Access token at all. Even though we are blocked by this issue due to Azure behavior, the fix we propose is applicable anytime we want to treat Access tokens as opaque strings and the IDP (Identity provider) chooses to use JWT format for access token without telling us what the signing cert is (Oauth2 spec does not need them to use the same signing cert that they use for ID token to be used to sign access token JWTs)

I would like to hear why the proposed changeset is harmful/not good to have if we ignore Azure behavior completely for the sake of this argument. Thank you, Sridevi

I do see that we already ignore missing kid when validating access token JWT based on a flag

        } catch (NoSuchKeyIdException e) {
          if (!skipMissingKeyNotify) {
narras-oss commented 1 year ago

@pmlopes Currently we have added this fix by extending class OAuth2AuthProviderImpl and duplicating some of the code in authenticate method in our project. This requires us to backport any changes made to this copy-pasted code in future released of this library. It would be great if we can extend the functionality to allow us to ignore JWT validation failures in access token based on a configuration flag. Appreciate your time if you can look at my comments above and the proposed changeset and provide your inputs

pmlopes commented 1 year ago

We have to deal with this in 2 phases, either rely on introspection (which many services do not support) or delegate to the userinfo endpoint which is usually used to validate opaque tokens, on success, we know the token is valid and the return can be used roughly as an id_token.

pmlopes commented 1 year ago

Let's address this chage on 4.4.0 as it likely introduces behavior changes.

teodorakostova commented 1 year ago

Hi, @pendula95 Can you please point me out to what should I do to retry the failed jobs in this PR. I don't see any option to rerun here https://github.com/eclipse-vertx/vertx-auth/actions/runs/4046178254/jobs/6960273324

pmlopes commented 1 year ago

Fixed in master. Thanks!