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

cwperks · 2023-05-30T15:53:40Z

Re-creating #2708 with the jar hell fix merged into main and addressing 2 minor comments from the PR.

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.

Signed-off-by: Sebastian Michalski <shekerama@gmail.com>

Signed-off-by: Craig Perkins <cwperx@amazon.com>

stephen-crawford · 2023-05-30T16:00:49Z

...ain/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticator.java

-
-		KeySetRetriever keySetRetriever = new KeySetRetriever(settings.get("openid_connect_url"),
-				getSSLConfig(settings, configPath), settings.getAsBoolean("cache_jwks_endpoint", false));
+		var jwksUri = settings.get("jwks_uri");


I think we want strongly typed where possible.

Switched this to String

stephen-crawford · 2023-05-30T16:08:02Z

src/test/java/com/amazon/dlic/auth/http/jwt/keybyoidc/TestJwts.java

-    static final JwtToken MC_COY_EXPIRED = create(MCCOY_SUBJECT, TEST_AUDIENCE, ROLES_CLAIM, TEST_ROLES_STRING, JwtConstants.CLAIM_EXPIRY, 10);
+	static final JwtToken MC_COY = create(MCCOY_SUBJECT, TEST_AUDIENCE, TEST_ISSUER, ROLES_CLAIM, TEST_ROLES_STRING);
+
+	static final JwtToken MC_COY_2 = create(MCCOY_SUBJECT, TEST_AUDIENCE, TEST_ISSUER, ROLES_CLAIM, TEST_ROLES_STRING);


Is there a reason you make a second identical token? It does not look it is every compared to MC_COY so it seems like any tests using MC_COY_SIGNED_OCT_2 could just use MC_COY_SIGNED_OCT_1? Perhaps I am misreading things?

Looks like the intention was that it was signed with a larger octet key, but it was not being configured correctly down below. I updated it now to ensure its using TestJwk.OCT_2 as expected.

These Jwks are originally used for SelfRefreshingKeySetTest and are being re-purposed for these tests.

DarshitChanpura

Should we close the other PR then?

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-05-30T16:27:25Z

@DarshitChanpura Yes, the other PR should be closed unless there are any substantial changes requested in the review.

cwperks · 2023-05-30T17:50:31Z

Looking at the test failures

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-05-30T18:02:33Z

I just pushed a fix for the tests. There were 2 issues:

JWTVerifier was not doing a null/empty check before checking if required issuer or required audience settings matches. It would always try to compare against an unset value instead of checking if the setting was set before performing the validation.
A new claim was added to MCCOY_SUBJECT so the count of attributes needed to be incremented in most tests of SingleKeyHTTPJwtKeyByOpenIdConnectAuthenticatorTest.

DarshitChanpura · 2023-05-30T18:43:38Z

Looks like the tests are still failing😥.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-05-30T19:32:36Z

@DarshitChanpura I pushed another commit to fix tests. I created more TestJwts to make it more obvious what the usage of each TestJwt was by giving them a clearer name which resembles the test case they are used for.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

codecov · 2023-05-30T20:46:12Z

Codecov Report

Merging #2808 (8ca5c52) into main (9be79bd) will increase coverage by 0.12%.
The diff coverage is 91.42%.

@@             Coverage Diff              @@
##               main    #2808      +/-   ##
============================================
+ Coverage     61.48%   61.61%   +0.12%     
- Complexity     3401     3413      +12     
============================================
  Files           266      266              
  Lines         18865    18893      +28     
  Branches       3302     3305       +3     
============================================
+ Hits          11599    11640      +41     
+ Misses         5669     5656      -13     
  Partials       1597     1597

Impacted Files	Coverage Δ
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java	`58.42% <60.00%> (+3.13%)`	⬆️
...byoidc/HTTPJwtKeyByOpenIdConnectAuthenticator.java	`90.00% <80.00%> (-4.12%)`	⬇️
...azon/dlic/auth/http/jwt/keybyoidc/JwtVerifier.java	`88.67% <100.00%> (+2.63%)`	⬆️
.../dlic/auth/http/jwt/keybyoidc/KeySetRetriever.java	`78.94% <100.00%> (+2.75%)`	⬆️

... and 2 files with indirect coverage changes

* [FEATURE] usage of JWKS with JWT (w/o OpenID connect) Signed-off-by: Sebastian Michalski <shekerama@gmail.com> * Change issuer to audience Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch to OCT_2 and use String instead of var Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only check required issuer and required audience if set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test cases Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com> (cherry picked from commit 4b38671)

* [FEATURE] usage of JWKS with JWT (w/o OpenID connect) --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com> (cherry picked from commit 4b38671) Co-authored-by: Craig Perkins <cwperx@amazon.com>

* [FEATURE] usage of JWKS with JWT (w/o OpenID connect) Signed-off-by: Sebastian Michalski <shekerama@gmail.com> * Change issuer to audience Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch to OCT_2 and use String instead of var Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only check required issuer and required audience if set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test cases Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com> (cherry picked from commit 4b38671) Co-authored-by: Craig Perkins <cwperx@amazon.com>

…ject#2808) * [FEATURE] usage of JWKS with JWT (w/o OpenID connect) Signed-off-by: Sebastian Michalski <shekerama@gmail.com> * Change issuer to audience Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch to OCT_2 and use String instead of var Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only check required issuer and required audience if set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test cases Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com>

…ject#2808) * [FEATURE] usage of JWKS with JWT (w/o OpenID connect) Signed-off-by: Sebastian Michalski <shekerama@gmail.com> * Change issuer to audience Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch to OCT_2 and use String instead of var Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only check required issuer and required audience if set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test cases Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

…ject#2808) * [FEATURE] usage of JWKS with JWT (w/o OpenID connect) Signed-off-by: Sebastian Michalski <shekerama@gmail.com> * Change issuer to audience Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch to OCT_2 and use String instead of var Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only check required issuer and required audience if set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test cases Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com>

…ject#2808) * [FEATURE] usage of JWKS with JWT (w/o OpenID connect) Signed-off-by: Sebastian Michalski <shekerama@gmail.com> * Change issuer to audience Signed-off-by: Craig Perkins <cwperx@amazon.com> * Switch to OCT_2 and use String instead of var Signed-off-by: Craig Perkins <cwperx@amazon.com> * Only check required issuer and required audience if set Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test cases Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: Sebastian Michalski <shekerama@gmail.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

sebastianmichalski and others added 3 commits May 29, 2023 09:46

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

052836e

Signed-off-by: Sebastian Michalski <shekerama@gmail.com>

Merge branch 'main' into feature/jwks_jwt

179105b

Change issuer to audience

2b656f9

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks requested review from DarshitChanpura, RyanL1997, cliu123, davidlago, peternied, reta and stephen-crawford as code owners May 30, 2023 15:53

cwperks added backport 2.x backport 2.8 Backport to 2.8 branch labels May 30, 2023

stephen-crawford reviewed May 30, 2023

View reviewed changes

DarshitChanpura reviewed May 30, 2023

View reviewed changes

Switch to OCT_2 and use String instead of var

988eca1

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Only check required issuer and required audience if set

4634797

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Fix test cases

e3b4d82

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Remove unused import

8ca5c52

Signed-off-by: Craig Perkins <cwperx@amazon.com>

RyanL1997 approved these changes May 30, 2023

View reviewed changes

DarshitChanpura approved these changes May 30, 2023

View reviewed changes

DarshitChanpura merged commit 4b38671 into opensearch-project:main May 30, 2023

opensearch-trigger-bot bot mentioned this pull request May 30, 2023

[Backport 2.x] [FEATURE] usage of JWKS with JWT (w/o OpenID connect) #2814

Merged

opensearch-trigger-bot bot mentioned this pull request May 30, 2023

[Backport 2.8] [FEATURE] usage of JWKS with JWT (w/o OpenID connect) #2815

Merged

DarshitChanpura mentioned this pull request May 30, 2023

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

Closed

2 tasks

This was referenced Jun 13, 2023

Sync up security/extensions feature branch with main on 06/13/2023 #2853

Closed

Sync up security/extensions feature branch with main on 06/13/2023 #2854

Merged

cwperks mentioned this pull request Oct 31, 2023

[BUG] OAUTH2.0 Access Token to access Opnesearch Cluster from an API #3363

Closed

Rishav9852Kumar mentioned this pull request Dec 17, 2024

[FEATURE] usage of JWKS with JWT (without using OpenID connect) #4974

Open

Rishav9852Kumar mentioned this pull request Aug 21, 2025

Usage of JWKS with JWT (without using OpenID connect) #5578

Merged

5 tasks

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

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

Uh oh!

Conversation

cwperks commented May 30, 2023

Description

Issues Resolved

Check List

Uh oh!

stephen-crawford May 30, 2023

Choose a reason for hiding this comment

Uh oh!

cwperks May 30, 2023

Choose a reason for hiding this comment

Uh oh!

stephen-crawford May 30, 2023

Choose a reason for hiding this comment

Uh oh!

cwperks May 30, 2023

Choose a reason for hiding this comment

Uh oh!

cwperks May 30, 2023

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

cwperks commented May 30, 2023

Uh oh!

cwperks commented May 30, 2023

Uh oh!

cwperks commented May 30, 2023

Uh oh!

DarshitChanpura commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwperks commented May 30, 2023

Uh oh!

codecov bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DarshitChanpura commented May 30, 2023 •

edited

Loading

codecov bot commented May 30, 2023 •

edited

Loading