Skip to content

Feat: Modified Id Token Validation#221

Open
tanya732 wants to merge 1 commit intojava-mvc-v2from
id-token-validation-support
Open

Feat: Modified Id Token Validation#221
tanya732 wants to merge 1 commit intojava-mvc-v2from
id-token-validation-support

Conversation

@tanya732
Copy link
Copy Markdown
Contributor

@tanya732 tanya732 commented May 6, 2026

Summary

  • Replaces custom ID Token validation with auth0-java v3's public IdTokenVerifier and SignatureVerifier
  • Enforces mandatory signature verification for all flows (code, implicit, hybrid)
  • Supports both RS256 and HS256 based on the token's alg header: RS256 uses JwkProvider (auto-discovered or customer-provided), HS256 uses client secret
  • Fixes MCD (Multiple Custom Domains) signature validation, JwkProvider is now resolved per-domain via ConcurrentHashMap cache instead of a single fixed verifier
  • Removes 6 custom classes: IdTokenVerifier, SignatureVerifier, AlgorithmNameVerifier, AsymmetricSignatureVerifier, SymmetricSignatureVerifier, TokenValidationException

Security fix

Now all tokens are cryptographically verified.

Breaking changes

  • Tokens with invalid signatures in code flow will now be rejected
  • Internal package-private classes removed (no public API impact)

@tanya732 tanya732 requested a review from a team as a code owner May 6, 2026 09:54
@tanya732
Copy link
Copy Markdown
Contributor Author

tanya732 commented May 6, 2026

@copilot

Copy link
Copy Markdown

Copilot AI commented May 6, 2026

@copilot

It looks like your comment is just a mention without any specific request. Could you clarify what you'd like me to help with?

@tanya732
Copy link
Copy Markdown
Contributor Author

tanya732 commented May 6, 2026

@copilot

Please review the PR

Copy link
Copy Markdown

Copilot AI commented May 6, 2026

@copilot

Please review the PR

Here's my review of the PR:

Overall: ✅ Good change — reduces custom crypto code and enforces mandatory signature verification.


Strengths

  • Removes ~1,325 lines of hand-rolled cryptographic code and replaces it with the battle-tested auth0-java v3 library (IdTokenVerifier / SignatureVerifier)
  • Security improvement: Signature verification is now enforced for all flows, including code flow — previously AlgorithmNameVerifier in code flow only checked the algorithm name without verifying the signature
  • Thread-safe MCD support: ConcurrentHashMap<String, JwkProvider> correctly caches one UrlJwkProvider per domain and handles concurrent access without data races
  • Exception chain is correct: JwkExceptionPublicKeyProviderExceptionIdTokenValidationExceptionIdentityVerificationException
  • All existing tests pass and test changes correctly replace SignatureVerifier mock with JwkProvider mock

Issues / Observations

1. UrlJwkProvider constructed with potentially pre-formatted domain (minor risk)

// RequestProcessor.java:350
return jwkProviders.computeIfAbsent(domain, d -> new UrlJwkProvider(d));

domainProvider.getDomain(request) could return a full URL (e.g. https://example.auth0.com/) in MCD scenarios. UrlJwkProvider.urlForDomain() prepends https:// only if not already present, but still appends /.well-known/jwks.json literally. A trailing slash would produce a double-slash URL (https://example.auth0.com//.well-known/jwks.json). The constructIssuer helper handles this normalization — ideally getJwkProvider should use the same normalized form, or use the same helper.

2. alg header read from unverified JWT before signature check (by-design, low risk)

String algorithm = JWT.decode(idToken).getAlgorithm();

Reading alg from the unverified header is standard practice (all JWT libraries do it), but it means a token with alg: HS256 will be verified with the client secret. If Auth0 is configured for RS256 but receives an attacker-forged HS256 token, SignatureVerifier.forHS256(clientSecret) will reject it because the HMAC won't match. No practical attack surface, but worth documenting in the Javadoc that HS256 tokens are only expected when the Auth0 app is configured for HS256.

3. IdTokenVerifier instantiated on every verifyIdToken call (minor inefficiency)
The verifierBuilder.build() is called per-request. The builder/verifier is lightweight, so this is acceptable. However if nonce and authenticationMaxAge didn't vary per call, caching the verifier would be possible. Just worth noting.

4. No test for the auto-discovered UrlJwkProvider path
All tests inject mockJwkProvider. The computeIfAbsent(domain, d -> new UrlJwkProvider(d)) code path (when no JwkProvider is configured) has no test coverage. A unit test that omits .withJwkProvider() and verifies the processor still attempts RS256 verification would help.


Minor nits

  • The AuthenticationController.Builder Javadoc for withJwkProvider still says "required if using RS256 in Implicit Grant flows" — it should now say it's optional (auto-discovered if not provided)
  • Redundant originDomain = null assignment followed immediately by assignment on the next line (line 223–225 in RequestProcessor)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants