Fix EC JWKS authentication support by wheresNasha · Pull Request #6106 · opensearch-project/security

wheresNasha · 2026-04-22T17:49:04Z

Description

This PR fixes EC-based JWKS authentication support as part of issue #6045.

Category : Bug fix
Why these changes are required?
EC-based JWT verification using JWKS was not handled correctly, leading to authentication failures for EC-signed tokens.
What is the old behavior before changes and new behavior after changes?
Old behavior:
EC-signed JWTs using JWKS could fail validation due to missing or incorrect key handling.
New behavior:
EC JWKS keys are now properly parsed and used for JWT verification, enabling successful authentication for EC-signed tokens.

Issues Resolved

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

Validated locally using EC-signed JWTs with JWKS
Tested token verification flow end-to-end
Confirmed backward compatibility with existing RSA-based flows

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
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.

codecov · 2026-04-27T17:09:45Z

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.99%. Comparing base (3c51d00) to head (4f6202d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
.../security/auth/http/jwt/keybyoidc/JwtVerifier.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6106      +/-   ##
==========================================
- Coverage   75.00%   74.99%   -0.02%     
==========================================
  Files         452      452              
  Lines       29106    29111       +5     
  Branches     4382     4384       +2     
==========================================
  Hits        21832    21832              
- Misses       5251     5252       +1     
- Partials     2023     2027       +4

Files with missing lines	Coverage Δ
.../security/auth/http/jwt/keybyoidc/JwtVerifier.java	`75.40% <60.00%> (-1.79%)`	⬇️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cwperks · 2026-05-01T18:15:44Z

@wheresNasha Can you please fix the Code Hygiene errors? Run ./gradlew spotlessApply and commit the changes.

cwperks · 2026-05-01T18:16:45Z

-        if (key.getClass() == OctetSequenceKey.class) {
+        if (key instanceof OctetSequenceKey) {
            result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toOctetSequenceKey().toSecretKey());
+        } else if (key instanceof RSAKey) {


Can we write tests to hit all of these branches?

I have already addded test coverage for the missing EC (ECKey) branch here.

I also looked into adding OCT (OctetSequenceKey) coverage, but the current JWKS authentication flow does not appear to successfully authenticate symmetric OCT keys in the existing test infrastructure (extractCredentials() returns null), so that test is currently failing.

RSA coverage is already present, so this PR focuses on covering the missing EC path.

Would you mind filing an issue for missing OctetSequenceKey support in test setup. We can track and merge as a separate issue.

Hello @DarshitChanpura ,
I have filled the issue: #6155

cwperks · 2026-05-01T18:18:04Z

+                    .createJWSVerifier(jwt.getHeader(), key.toECKey().toECPublicKey());
        } else {
-            result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toRSAKey().toRSAPublicKey());
+            throw new IllegalArgumentException("Unsupported JWK key type: " + key.getClass());


Not sure if it makes sense but should we keep the fallback logic in place?

Any idea what the entire universe of key types can be?

I leaned toward explicit key type handling here since failing closed felt safer than broadly falling back and potentially masking unsupported JWK types.
From what I understand, Nimbus JWK currently supports symmetric (oct), RSA, EC, and additional types like OKP.

If preserving backward compatibility with prior RSA fallback makes more sense here, I’m happy to adjust it.

github-actions · 2026-05-02T05:59:44Z

PR Reviewer Guide 🔍

(Review updated until commit `4f6202d`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Possible Issue The logic uses `instanceof` checks but does not handle the case where a key might be both an `OctetSequenceKey` and another type due to class hierarchy. If `OctetSequenceKey` is a subclass of another checked type (or vice versa), the first matching branch executes, potentially using the wrong conversion method. This can cause signature verification to fail or use an incorrect key type when the JWK implementation has overlapping type hierarchies. if (key instanceof OctetSequenceKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toOctetSequenceKey().toSecretKey()); } else if (key instanceof RSAKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toRSAKey().toRSAPublicKey()); } else if (key instanceof ECKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toECKey().toECPublicKey()); } else { throw new IllegalArgumentException("Unsupported JWK key type: " + key.getClass());

github-actions · 2026-05-02T06:00:00Z

PR Code Suggestions ✨

Latest suggestions up to 4f6202d

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use consistent exception type The `IllegalArgumentException` thrown for unsupported key types should be wrapped in a `BadCredentialsException` to maintain consistency with the method's exception handling pattern. This ensures proper error propagation and handling in the authentication flow. src/main/java/org/opensearch/security/auth/http/jwt/keybyoidc/JwtVerifier.java [107-115] if (key instanceof OctetSequenceKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toOctetSequenceKey().toSecretKey()); } else if (key instanceof RSAKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toRSAKey().toRSAPublicKey()); } else if (key instanceof ECKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toECKey().toECPublicKey()); } else { - throw new IllegalArgumentException("Unsupported JWK key type: " + key.getClass()); + throw new BadCredentialsException("Unsupported JWK key type: " + key.getClass()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an inconsistency in exception handling. The method signature declares `BadCredentialsException`, and using `IllegalArgumentException` for unsupported key types breaks this pattern. However, `IllegalArgumentException` is also a reasonable choice for invalid input types, making this a moderate improvement rather than a critical fix.	Medium

Previous suggestions

Suggestions up to commit dfc12e9

Category	Suggestion	Impact
General	Use consistent exception type The exception thrown for unsupported key types should be `BadCredentialsException` instead of `IllegalArgumentException` to maintain consistency with the method's existing error handling pattern and signature. This ensures proper authentication failure handling in the security context. src/main/java/org/opensearch/security/auth/http/jwt/keybyoidc/JwtVerifier.java [107-115] if (key instanceof OctetSequenceKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toOctetSequenceKey().toSecretKey()); } else if (key instanceof RSAKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toRSAKey().toRSAPublicKey()); } else if (key instanceof ECKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toECKey().toECPublicKey()); } else { - throw new IllegalArgumentException("Unsupported JWK key type: " + key.getClass()); + throw new BadCredentialsException("Unsupported JWK key type: " + key.getClass()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `BadCredentialsException` would be more consistent with the method's signature and existing error handling pattern (line 118 throws `BadCredentialsException`). This improves consistency in authentication failure handling, though the current `IllegalArgumentException` is not technically incorrect for an unsupported key type scenario.	Medium

Suggestions up to commit 5a130c0

Category	Suggestion	Impact
General	Use consistent exception type The exception thrown for unsupported key types should be `BadCredentialsException` instead of `IllegalArgumentException` to maintain consistency with the method's existing error handling pattern and signature. This ensures proper authentication failure handling in the security context. src/main/java/org/opensearch/security/auth/http/jwt/keybyoidc/JwtVerifier.java [107-115] if (key instanceof OctetSequenceKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toOctetSequenceKey().toSecretKey()); } else if (key instanceof RSAKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toRSAKey().toRSAPublicKey()); } else if (key instanceof ECKey) { result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toECKey().toECPublicKey()); } else { - throw new IllegalArgumentException("Unsupported JWK key type: " + key.getClass()); + throw new BadCredentialsException("Unsupported JWK key type: " + key.getClass()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `BadCredentialsException` would be more consistent with the method's signature and existing error handling pattern (line 118 throws `BadCredentialsException`). This improves consistency in authentication failure handling, though the current `IllegalArgumentException` is not technically incorrect for an unsupported key type scenario.	Medium

Suggestions up to commit 4143730

Category	Suggestion	Impact
Possible issue	Wrap unsupported key type in declared exception The `IllegalArgumentException` thrown for unsupported key types is not declared in the method signature and may propagate unexpectedly to callers. Since the method already throws `BadCredentialsException` and `JOSEException`, the unsupported key type should be wrapped in one of those to maintain consistent error handling. src/main/java/org/opensearch/security/auth/http/jwt/keybyoidc/JwtVerifier.java [113-115] } else { - throw new IllegalArgumentException("Unsupported JWK key type: " + key.getClass()); + throw new BadCredentialsException("Unsupported JWK key type: " + key.getClass()); } Suggestion importance[1-10]: 6 __ Why: The `IllegalArgumentException` is unchecked and not declared in the method signature, while `BadCredentialsException` is already declared and would provide more consistent error handling for callers. This is a valid improvement for API consistency.	Low
General	Add attribute count assertion in EC test The EC authentication test only checks the username but does not verify the number of attributes like the RSA/OCT test does. This may miss regressions in claims extraction for EC-signed tokens. Consider adding an assertion on `creds.getAttributes().size()` to ensure claims are properly parsed. src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/HTTPJwtKeyByJWKSAuthenticatorTest.java [70-71] Assert.assertNotNull(creds); assertThat(creds.getUsername(), is(TestJwts.MCCOY_SUBJECT)); +assertThat(creds.getAttributes().size(), is(4)); Suggestion importance[1-10]: 4 __ Why: Adding an assertion on `creds.getAttributes().size()` would improve test coverage for EC-signed tokens, but this is a minor test completeness improvement that doesn't affect production code correctness.	Low

wheresNasha · 2026-05-02T06:06:22Z

@cwperks Thanks for the helpful guidance!
I’ve run ./gradlew spotlessApply to fix the formatting issues and have addressed the review comments as well.

Please let me know your thoughts, and if everything looks good from your side, I can rebase the branch.
I’d also appreciate it if CI could be triggered
Thanks!

github-actions · 2026-05-18T17:21:02Z

Persistent review updated to latest commit 5a130c0

github-actions · 2026-05-19T18:15:58Z

Persistent review updated to latest commit dfc12e9

DarshitChanpura

thank you for adding this @wheresNasha ! Left a couple of comments. LGTM otherwise.

DarshitChanpura · 2026-05-19T18:17:56Z

    }

+    @Test
+    public void testJwksAuthenticationWithEC() throws Exception {


can we add a unhappy path test here as well?

Thanks for the review! I’ve added the unhappy path EC test and also fixed the formatting issues using spotlessApply.

I’ve now rebased the branch on latest main as well.
Can we merge this change please
Thanks!

DarshitChanpura · 2026-05-19T18:19:18Z

-        if (key.getClass() == OctetSequenceKey.class) {
+        if (key instanceof OctetSequenceKey) {
            result = new DefaultJWSVerifierFactory().createJWSVerifier(jwt.getHeader(), key.toOctetSequenceKey().toSecretKey());
+        } else if (key instanceof RSAKey) {


Would you mind filing an issue for missing OctetSequenceKey support in test setup. We can track and merge as a separate issue.

Signed-off-by: Sakshi Nasha <sakshinasha11@gmail.com>

github-actions · 2026-05-20T18:26:59Z

Persistent review updated to latest commit 4f6202d

wheresNasha requested review from DarshitChanpura, RyanL1997, cwperks, derek-ho, finnegancarroll, nibix, reta, shikharj05 and willyborankin as code owners April 22, 2026 17:49

cwperks reviewed May 1, 2026

View reviewed changes

wheresNasha force-pushed the jwt-ec-jwks-fix branch from cbae25a to 4143730 Compare May 2, 2026 05:58

cwperks previously approved these changes May 18, 2026

View reviewed changes

DarshitChanpura reviewed May 19, 2026

View reviewed changes

wheresNasha added 2 commits May 20, 2026 13:24

Fix EC JWKS authentication support

fc64b31

Signed-off-by: Sakshi Nasha <sakshinasha11@gmail.com>

Add EC JWT authentication negative test

4f6202d

Signed-off-by: Sakshi Nasha <sakshinasha11@gmail.com>

wheresNasha dismissed cwperks’s stale review via 4f6202d May 20, 2026 18:25

wheresNasha force-pushed the jwt-ec-jwks-fix branch from dfc12e9 to 4f6202d Compare May 20, 2026 18:25

cwperks approved these changes May 20, 2026

View reviewed changes

wheresNasha mentioned this pull request May 21, 2026

[FEATURE] Add OctetSequenceKey support in JWT test setup #6155

Open

Conversation

wheresNasha commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

codecov Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cwperks commented May 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheresNasha May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 4f6202d)

Uh oh!

github-actions Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

wheresNasha commented May 2, 2026

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wheresNasha commented Apr 22, 2026 •

edited

Loading

codecov Bot commented Apr 27, 2026 •

edited

Loading

wheresNasha May 2, 2026 •

edited

Loading

github-actions Bot commented May 2, 2026 •

edited

Loading

(Review updated until commit `4f6202d`)

github-actions Bot commented May 2, 2026 •

edited

Loading