CAMEL-23768: camel-keycloak - select the JWKS verification key by the token kid#24527
Merged
Conversation
Contributor
|
there is a merge conflict |
davsclaus
approved these changes
Jul 8, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Review: CAMEL-23768 — Select JWKS verification key by token kid
No blocking issues. Clean correctness fix for a real key-rotation problem.
What was checked
- Git history:
KeycloakPublicKeyResolverintroduced in CAMEL-22854 (c1ed776) by the same author. ThegetPublicKey(null)call has been there since inception — this fixes an original oversight, not reverting intentional behavior. - Correctness:
selectKey(kid)throwsIOExceptionwhen a kid is given but no key matches. Right security posture: never verify a token against an unrelated key during rotation. - Backward compatibility: When the token carries no
kid(null), the first-key fallback is preserved. Single-key JWKS deployments are unaffected. - Tests: Good coverage of the three key scenarios (matching kid, unknown kid fails closed, null kid fallback). Tests build their own JWKS from generated RSA keys — no container needed.
- AI attribution: Present in both the PR body and the commit trailer.
Minor observations (non-blocking)
getPublicKey()can callrefreshKeys()twice in quick succession (cache expiry + unknown kid). Harmless (idempotent, synchronized) but costs an extra HTTP round-trip in that edge case.parseJwkswidened fromprivateto package-visible for testability. Common pattern.- Broad
Exceptioncatch inextractKeyIdis pragmatic sinceJWSInputcan throw various exceptions from untrusted input, and a null return is safe becauseTokenVerifierrejects bad tokens downstream.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
… token kid KeycloakPublicKeyResolver ignored the JWT header kid and returned the first key in the JWKS, and KeycloakSecurityProcessor never passed the kid through (getPublicKey(null)). During key rotation (multiple keys present) this could pick the wrong key and reject an otherwise-valid token. The token is still cryptographically verified against a real key, so this is a correctness/availability matter, not a bypass. The processor now extracts the kid from the token header (KeycloakSecurityHelper .extractKeyId via Keycloak's JWSInput) and passes it to the resolver. getPublicKey forces a single refresh when the kid is unknown (to pick up rotated keys) and fails closed via a new selectKey helper when a kid is given but no key matches; the first-key fallback is kept only when the token carries no kid. Adds KeycloakPublicKeyResolverTest (matching kid / fail-closed on unknown kid / first-key fallback for null kid) and extractKeyId coverage in KeycloakSecurityHelperTest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Rebased onto Claude Code on behalf of Andrea Cosentino |
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CAMEL-23768.
Problem
KeycloakPublicKeyResolverignored the JWT headerkidand returned the first key in the JWKS, andKeycloakSecurityProcessornever passed the kid through (getPublicKey(null)). During key rotation (multiple keys present) this could pick the wrong key and reject an otherwise-valid token. The token is still cryptographically verified against a real key, so this is a correctness/availability matter, not a bypass.Change
KeycloakSecurityHelper.extractKeyId(token)— pulls thekidfrom the JWS header using Keycloak's ownJWSInput(existing dependency); returnsnullif absent/unparseable.KeycloakSecurityProcessor— extracts the kid and passes it toresolver.getPublicKey(kid).KeycloakPublicKeyResolver.getPublicKey— forces one refresh when the kid is unknown (to pick up rotated keys), then delegates to a new package-visibleselectKeythat fails closed (throws) when a kid is given but no key matches. The first-key fallback is kept only when the token carries no kid.Tests
KeycloakPublicKeyResolverTest— matching kid selects that key (not the first); unknown kid fails closed; null kid falls back to the first key (JWKS built from generated RSA keys, no container).KeycloakSecurityHelperTest—extractKeyIdreturns the header kid /nullfor a malformed token.mvn clean install -DskipTests, 1871 modules) green; no generated-file impact (plain Java).Compatibility / backport
Correctness fix,
fixVersions4.18.4 / 4.22.0 (camel-keycloak does not exist on the 4.14.x line). Default behaviour for a single-key JWKS is unchanged.Claude Code on behalf of Andrea Cosentino