[FEATURE] usage of JWKS with JWT (w/o OpenID connect)

[sebastianmichalski]

Description

Extending auth with jwks_uri parameter to omit passing openid_connect_url with openid-configuration

• Category: feature
• Why these changes are required? To add possibility to define a JWKS endpoint for JWT-based authentication
• What is the old behavior before changes and new behavior after changes? Old behavior remains the same, new one omits the part with calling openid-configuration endpoint of IdP

Issues Resolved

#1858

Check List

• New functionality includes testing
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[stephen-crawford]

Hi @sebastianmichalski, thank you for taking the time to put together this PR. I left a couple nit-picky comments and then also would appreciate it if you could expand your test to include an 'unhappy' scenario. Thank you for all your work! Generally things look good :)

[cwperks]

Hi @sebastianmichalski , I have a question in general about this change. The issue description mentions the use of jwks_uri in the JWT backend, but this PR only has edits in HTTPJwtKeyByOpenIdConnectAuthenticator.

I believe the HTTPJwtAuthenticator is the JWT Authentication backend according to this line in DynamicConfigModel: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/DynamicConfigModel.java#L109

Will this PR solve for the issue described in #1858 ?

[nibix]

@cwperks You are mentioning a good point.

Effectively, OpenSearch has two JWT authenticator implementations:

• jwt in HTTPJwtAuthenticator
• openid in HTTPJwtKeyByOpenIdConnectAuthenticator

Even though one of these is called openid, it is not a full OpenId implementation - it only implements fetching of JWKS keys from the "well known" OIDC config endpoint. Otherwise, it is just a JWT authenticator based on a JWKS file linked in the well known OIDC endpoint.

Thus, the requirement from #1858 can be very easily implemented in HTTPJwtKeyByOpenIdConnectAuthenticator, as it already provides 95% of the infrastructure.

The other JWT authenticator, HTTPJwtAuthenticator, however does only support a single key and does not know anything about JWKS. Adding support for JWKS to that class would be actually a complete rewrite of that class.

Thus, for kicking off the conversation, we have implemented the new feature in HTTPJwtKeyByOpenIdConnectAuthenticator. It would now totally make sense to consolidate the two JWK implementations into one.

However, this needs some product mgmt decisions:

• How should this look like
• To which extent is backwards compat required
• How shall backwards compat be achieved

[cwperks]

Hey @nibix , off the top of my head I think there are a few differences between these 2 backends. Here's a list of the settings that are available to the HTTPJwtAuthenticator:

• signing_key - Signing key used for HMAC SHA512 signing of the JWT
• jwt_url_parameter - URL param corresponding to jwt opposed to using HTTP Authorization header to bear the token
• jwt_header - Used to change from the default Authorization header
• roles_key - Same as OIDC backend, key to extract the backend roles
• subject_key Same as OIDC backend, used to extract username
• required_audience - This is a string and designates the required aud claim in the token
• required_issuer - This is a string and designates the required iss claim in the token

[nibix]

@cwperks Thank you for the list!

However, I think that the following attributes are already supported by HTTPJwtKeyByOpenIdConnectAuthenticator (via AbstractHTTPJwtAuthenticator):

• jwt_url_parameter
• jwt_header
• roles_key
• subject_key
• jwt_clock_skew_tolerance_seconds

The attribute signing_key is indeed not supported, but I am not sure whether it would make sense to directly support this, as we get the keys in the case of HTTPJwtKeyByOpenIdConnectAuthenticator from the JWKS file.

This leaves for following attributes which we can add

• required_audience
• required_issuer

[DarshitChanpura]

The attribute signing_key is indeed not supported, but I am not sure whether it would make sense to directly support this, as we get the keys in the case of HTTPJwtKeyByOpenIdConnectAuthenticator from the JWKS file.

Hey @nibix, Can you elaborate a little on why would the param signing_key not be required?

[codecov-commenter]

Codecov Report

Merging #2708 (5f9cc0d) into main (a580dfc) will increase coverage by 0.06%.
The diff coverage is 90.47%.

❗ Current head 5f9cc0d differs from pull request most recent head 052836e. Consider uploading reports for the commit 052836e to get more accurate results

@@             Coverage Diff              @@
##               main    #2708      +/-   ##
============================================
+ Coverage     61.47%   61.54%   +0.06%     
- Complexity     3403     3407       +4     
============================================
  Files           266      266              
  Lines         18865    18881      +16     
  Branches       3302     3302              
============================================
+ Hits          11598    11620      +22     
+ Misses         5673     5663      -10     
- Partials       1594     1598       +4     

Impacted Files	Coverage Δ
...byoidc/HTTPJwtKeyByOpenIdConnectAuthenticator.java	90.00% <80.00%> (-4.12%)	⬇️
.../dlic/auth/http/jwt/keybyoidc/KeySetRetriever.java	77.41% <91.66%> (+1.22%)	⬆️
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java	60.67% <100.00%> (+5.38%)	⬆️

... and 4 files with indirect coverage changes

[nibix]

Hey @nibix, Can you elaborate a little on why would the param signing_key not be required?

The signing keys now come from the referenced JWKS.

[cwperks]

Thank you @sebastianmichalski . This will need to be accompanied with documentation to instruct users to use OIDC auth backend for simple JWT authentication with a JWKS config instead of using the JWT backend. It would be nice if this could be extended to the JWT backend as the issue describes, but this solution does solve the request from the issue filed.

[DarshitChanpura]

LGTM overall. Left some nit comments.

[DarshitChanpura]

LGTM. The comment about extracting the hard-coded values is not a blocker from merging.

[cwperks]

@sebastianmichalski Quick question: Is it possible to add an integration test to show required audience and required issuer working with OIDC backend?

[sebastianmichalski]

@sebastianmichalski Quick question: Is it possible to add an integration test to show required audience and required issuer working with OIDC backend?

Is this test not sufficient? https://github.com/opensearch-project/security/pull/2708/files#diff-7495fb84f47dfc4449f13b5a9eb8cc3ea42e5b5a58529ef133e373947918bb2fR64

[cwperks]

@sebastianmichalski How about a negative test? The backend is configured with required issuer, but the token does not contain the claim? Same for required audience? Let me know what you think.

[cwperks]

I'm trying to understand, based on the equivalent test in HTTPJwtAuthenticatorTest it looks like this should be returning null creds because if the audience claim in the token is different from the configured requiredAudience then it would not be considered a valid token. Since the creds are not null here, does that mean it would successfully authenticate?

[sebastianmichalski]

The reason that the one You found is providing null instead of AuthCredentials is that we are using different extractCredentials0() method (from HTTPJwtAuthenticator instead of AbstractHTTPJwtAuthenticator). The one more verbose is trying to parse claims and checks ie. audience, while the other one in the same time just takes claims from token without any checks there (or at least the checks agains audience/issuer):

jwtParser.parseClaimsJws(jwtToken) throws Exception which leads to return null; in line 205:

in the meantime jwt.getClaims(); is just returning the claims which are just validated only against signature, expiry and nbf.

[cwperks]

Is this testing to ensure that if the user presents a token that was signed with a verifiable signature, but contains a mismatching audience claim that the request is unauthorized?

If a user has in their authc: list:

openid_auth_domain:
  http_enabled: true
  transport_enabled: true
  order: 0
  http_authenticator:
    type: openid
    challenge: false
    config:
      subject_key: preferred_username
      roles_key: roles
      jwks_uri: <jwks_uri>
      required_issuer: keycloak
  authentication_backend:
    type: noop

and sends a JWT with an HTTP Request that was signed with the same, JWKS but with claims

{
  'sub: '<username>',
  'roles: '<role1>,<role2>',
  'iss: 'auth0',
 ...
}

would that request be rejected? I don't see any logic in this PR that would deny that request and I understand that we are just so happening to be adding support for required_issuer and required_audience here even though this PR is introducing support for jwks_uri.

Does there need to be any additional logic here to check if the issuer in the claims matches the required issuer and if the audience in the claims matches the required audience?

[sebastianmichalski]

The topic of this issue was to just provide jwks_uri to use it instead of changing signing key manually I guess, not to change current plugin behavior. Should we change this behavior within this issue?

[cwperks]

That's what I took @nibix comment to mean here. There is no need to add required_issuer and required_audience in this PR if they are config options for the OIDC backend, but do nothing. I'm also happy to accept this PR introducing the config options and fast-follow it to wire them up and make them useful.

[nibix]

@cwperks I have to apologize for the irritation here. Of course, having required_audience and required_issuer as config attributes needs the accompanying logic. We will add this logic ASAP.

[sebastianmichalski]

Sure, now it's doable when I know what to do with those params 😉

[cwperks]

@sebastianmichalski @nibix Would you be able to merge the latest from main into this branch? There is a fix for the jar hell issue recently merged into main.

[cwperks]

Can this exception text be updated to read Invalid audience?

[cwperks]

Should this read required_audience here?

[DarshitChanpura]

Closing in lieu of #2808. Thank you for your effort @sebastianmichalski!