Skip to content

RANGER-5533:Verify JWT Issuer Claims if present#901

Open
ChinmayHegde24 wants to merge 1 commit intoapache:masterfrom
ChinmayHegde24:RANGER-5533
Open

RANGER-5533:Verify JWT Issuer Claims if present#901
ChinmayHegde24 wants to merge 1 commit intoapache:masterfrom
ChinmayHegde24:RANGER-5533

Conversation

@ChinmayHegde24
Copy link
Copy Markdown
Contributor

Support for validating the iss (issuer) claim in JWT has been added as part of this PR.

* @param jwtToken the JWT token from which the JWT issuer will be obtained
* @return true if an expected issuer is present, otherwise false
*/
protected boolean validateIssuer(final SignedJWT jwtToken) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a standalone validateIssuer() method, consider using Nimbus's DefaultJWTClaimsVerifier, which would consolidate issuer, audience, and expiration checks in one place:

JWTClaimsSet.Builder exactMatchBuilder = new JWTClaimsSet.Builder();
String               issuer            = config.getProperty(KEY_JWT_ISSUER);
if (StringUtils.isNotBlank(issuer)) {
    exactMatchBuilder.issuer(issuer);
}

Set<String> acceptedAudiences = null;
String      audiencesStr      = config.getProperty(KEY_JWT_AUDIENCES);
if (StringUtils.isNotBlank(audiencesStr)) {
    acceptedAudiences = new HashSet<>(Arrays.asList(audiencesStr.split(",")));
}

claimsVerifier = new DefaultJWTClaimsVerifier<>(acceptedAudiences, exactMatchBuilder.build(), null, null);
claimsVerifier.verify(jwtToken.getJWTClaimsSet(), null);

This would replace all three of validateExpiration(), validateAudiences(), and the proposed validateIssuer() with a single, well-tested library call. It also gets clock skew handling for free (the verifier has a configurable skew, defaulting to 60 seconds).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current validateIssuer method can accept multiple issuers, but if we use DefaultJWTClaimsVerifier acceptance of single-issuer limitation comes , is that okay?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-issuer support is a good follow-up (in another PR). For this PR, I suggest to scope it for single-issuer validation. Multi-issuer support would require issuer-specific config and processor selection, something like:

jwt.issuers=issuerA,issuerB
jwt.issuer.issuerA.iss=https://idp-a/...
jwt.issuer.issuerA.jwks-url=...
jwt.issuer.issuerA.audiences=...
jwt.issuer.issuerB.iss=https://idp-b/...
jwt.issuer.issuerB.jwks-url=...
jwt.issuer.issuerB.audiences=...

so it would be better handled in a separate PR.

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