OAuth2 Auth provider incorrectly validating Access Tokens

[chrispatmore]

Questions

Do not use this issue tracker to ask questions, instead use one of these channels. Questions will likely be closed without notice.

Version

Version: 4.3.8

Context

Whilst trying to set up an OIDC client with discovery against a keycloak instance, I discovered that the access token which is in JWT form was not being decoded and added to the session as I expected it would be. The non decoded format is present on the session.

Steps to reproduce

1. Run a keycloak instance somewhere and create a realm and client and a user
2. Set up the vertex OIDC client with discovery
3. Have an endpoint that prints out the session
4. Run the program and log in and inspect the session

Extra

The reason the decoded form is not stored is because of

vertx-auth/vertx-auth-oauth2/src/main/java/io/vertx/ext/auth/oauth2/impl/OAuth2AuthProviderImpl.java

Line 542 in f0215a1

.put("accessToken", validToken(token, false));

Which calls validToken Which internally does

vertx-auth/vertx-auth-oauth2/src/main/java/io/vertx/ext/auth/oauth2/impl/OAuth2AuthProviderImpl.java

Line 643 in f0215a1

if (idToken || jwtOptions.getAudience() == null) {

With the comment

// https://openid.net/specs/openid-connect-core-1_0.html#  $3.1.3.7.
// The Client MUST validate that the aud (audience) Claim contains its client_id value registered at the Issuer
// identified by the iss (issuer) Claim as an audience. The aud (audience) Claim MAY contain an array with more
// than one element. The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience,
// or if it contains additional audiences not trusted by the Client.

This fails for the keycloak access token as the aud is not the client ID it is account

https://openid.net/specs/openid-connect-core-1_0.html# $3.1.3.7 refers to the section for ID Token Validation. In this case the token is not an ID token so should not be subject to the same validation logic. The validation logic for access tokens is stated in section 3.2.2.9

Therefore there are a couple of options / parts:

1. switch

   vertx-auth/vertx-auth-oauth2/src/main/java/io/vertx/ext/auth/oauth2/impl/OAuth2AuthProviderImpl.java

   Line 643 in f0215a1

   if (idToken || jwtOptions.getAudience() == null) {

   to idToken && jwtOptions.getAudience() == null instead of || so that this validation is skipped for access tokens, fixing this bug
2. Add in the rules from 3.2.2.9 for access tokens (possibly a new feature or extension)

[fposch]

See #495 (since version 4.2.0)

I have the same problem and this broke our attempts to upgrade to vertx4 with keycloak. We don't use specific audience values and so this requires us to either work around this in (Java) code or mitigate our Keycloak settings (using mappers, ...).

Would be great to at least optionally get the old behavior back or disable the check when using the handler, since this is is all internal API, I didn't find a feasible way to fix this with overrides, you'd have to intercept the token, remove audience and then pass it to the OAuth2 handlers...

[chrispatmore]

Would appreciate your input on this @pmlopes .

I appreciate that this issue can be resolved by adding the account audience to the jwtOptions, which is then used to check the Access Token JWT and it all works (for keycloak specifically).

However, this is somewhat inconvenient when allowing the user to choose their own provider. It is hard to know what access token we will see and if it is a JWT Access token we don't know what the audience in there will be. We could ask the user to provide, but that is not necessarily very simple for them to determine. Additionally the addition of this seems to have broken others. And there is nothing in the OIDC spec that says the access token should be subject to this validation .

I would appreciate it if we could either:

• allow this check to be disabled (for ATs)
• Only ever run this check for idTokens

[chrispatmore]

In fact on further review of

if (target != null && target.size() > 0) {
      if (idToken || jwtOptions.getAudience() == null) {
        // https://openid.net/specs/openid-connect-core-1_0.html#  $3.1.3.7.
        // The Client MUST validate that the aud (audience) Claim contains its client_id value registered at the Issuer
        // identified by the iss (issuer) Claim as an audience. The aud (audience) Claim MAY contain an array with more
        // than one element. The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience,
        // or if it contains additional audiences not trusted by the Client.
        if (!target.contains(config.getClientId())) {
          throw new IllegalStateException("Invalid JWT audience. expected: " + config.getClientId());
        }
      } else {
        if (Collections.disjoint(jwtOptions.getAudience(), target.getList())) {
          throw new IllegalStateException("Invalid JWT audience. expected: " + Json.encode(jwtOptions.getAudience()));
        }
      }
    }

It does not appear to conform to the rules:

• The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience - working
• Or if it contains additional audiences not trusted by the Client - not working

the else block instead of looking for audience values in the ID token that are not trusted by the client (assuming that would be use of jwtOptions.getAudience() here) is looking at any token (ID or AT) and checking that at least 1 value is common

Would something like

if (idToken && target != null && target.size() > 0) {
      if (!target.contains(config.getClientId())) {
        throw new IllegalStateException("Invalid JWT audience. expected: " + config.getClientId());
      }
      if (target.size() > 0) {
        List<String> trustedAudiences = Optional.ofNullable(jwtOptions.getAudience()).orElse(new ArrayList<>());
        trustedAudiences.add(config.getClientId());
        List<String> untrustedAudiences = target.stream()
          .filter(String.class::isInstance)
          .map(String.class::cast)
          .filter(additionalTarget -> !trustedAudiences.contains(additionalTarget))
          .collect(Collectors.toList());
        if (untrustedAudiences.size() > 0) {
          throw new IllegalStateException("Invalid JWT audience. untrusted values: " + String.join(",", untrustedAudiences));
        }
      }
    }

be more appropriate, this would satisfy both rules and only apply to IDTs not ATs

[fposch]

Sorry, any update here? I think @chrispatmore has a point and this definitely broke our existing vertx server when trying to upgrade. We would have to dig into the details of audience and implement that or hope and trust in the fact that Keycloak by default returns account as audience against every server we are authenticating against....

[vietj]

@pmlopes could you please take a look at this PR ?

[fposch]

@vietj Do you also have some input on the initial issue about Access Token audience validation?

Is this something you want to fix as suggested by @chrispatmore or will this behavior stick with 4.x? Would be good to know for our upgrade plans in our stack, we're currently stuck with end-of-live vertx 3.x and that's the only issue. Hardcoding account to work against keycloak is only my 2nd favorite approach.

Thanks!

[chrispatmore]

Hey @pmlopes , this is the last one I have open in the auth space, then hopefully I can leave you in peace haha, would appreciate your thoughts here

[fposch]

Hi @pmlopes,

Sorry for being such a pain.

Are there any plans to accept this as an issue and releasing a fix?

As I understand it, it is change of behavior and arguably clashes with the standard. It would be good to know before I have to go down the road with a workaround or potential changes in our auth handling if there is a chance getting the old behavior back in the foreseeable future.

Thanks and Merry Christmas,
Florian

[fposch]

In fact on further review of

if (target != null && target.size() > 0) {
      if (idToken || jwtOptions.getAudience() == null) {
        // https://openid.net/specs/openid-connect-core-1_0.html#  $3.1.3.7.
        // The Client MUST validate that the aud (audience) Claim contains its client_id value registered at the Issuer
        // identified by the iss (issuer) Claim as an audience. The aud (audience) Claim MAY contain an array with more
        // than one element. The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience,
        // or if it contains additional audiences not trusted by the Client.
        if (!target.contains(config.getClientId())) {
          throw new IllegalStateException("Invalid JWT audience. expected: " + config.getClientId());
        }
      } else {
        if (Collections.disjoint(jwtOptions.getAudience(), target.getList())) {
          throw new IllegalStateException("Invalid JWT audience. expected: " + Json.encode(jwtOptions.getAudience()));
        }
      }
    }

It does not appear to conform to the rules:

• The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience - working
• Or if it contains additional audiences not trusted by the Client - not working

the else block instead of looking for audience values in the ID token that are not trusted by the client (assuming that would be use of jwtOptions.getAudience() here) is looking at any token (ID or AT) and checking that at least 1 value is common

Would something like

if (idToken && target != null && target.size() > 0) {
      if (!target.contains(config.getClientId())) {
        throw new IllegalStateException("Invalid JWT audience. expected: " + config.getClientId());
      }
      if (target.size() > 0) {
        List<String> trustedAudiences = Optional.ofNullable(jwtOptions.getAudience()).orElse(new ArrayList<>());
        trustedAudiences.add(config.getClientId());
        List<String> untrustedAudiences = target.stream()
          .filter(String.class::isInstance)
          .map(String.class::cast)
          .filter(additionalTarget -> !trustedAudiences.contains(additionalTarget))
          .collect(Collectors.toList());
        if (untrustedAudiences.size() > 0) {
          throw new IllegalStateException("Invalid JWT audience. untrusted values: " + String.join(",", untrustedAudiences));
        }
      }
    }

be more appropriate, this would satisfy both rules and only apply to IDTs not ATs

Tested the above with our implementation and that indeed resolves the unintended verification of ATs in our case.

I'd be happy to raise a PR for 4.x if there are no concerns about the change in general, would also be great if you @chrispatmore could do this since I'm not set up as an Eclipse contributor, yet.

Change seems solid and carefully reading the spec, nothing suggests that audience verification should be applied to anything but ID tokens, so I see no fault in changing this.

Let me know what you think!

[fposch]

See PR: #673

[chrispatmore]

Thank you for raising that @fposch, glad the fix works for you, sorry I didn't get to it quicker

[fposch]

Thank you for raising that @fposch, glad the fix works for you, sorry I didn't get to it quicker

@chrispatmore Sorry I didn't see OAuth2Keycloak14IT because I was just naively running mvn:test for the whole module (which doesn't include the integration suite) - this has indeed broken some integration tests.

Do you want me to take a look?

Some of it looks like the test assumption could be changed but I'm not so sure about the rest.