Add JWT authenticator support for validating ID Tokens #13242
Conversation
| @@ -26,6 +26,7 @@ | |||
|
|
|||
| public class OIDCConfig | |||
| { | |||
| private final String DEFAULT_SCOPE = "name"; | |||
There was a problem hiding this comment.
The default claim to use for the identity is sub since name is not a standard OIDC claim
There was a problem hiding this comment.
name is a standard claim according to the OIDC spec.
There was a problem hiding this comment.
While it is a standard claim, it isn't mandatory. Some OIDC compliant auth servers don't include that claim in the ID token even when the profile scope is requested.
| @JsonCreator | ||
| public OIDCConfig( | ||
| @JsonProperty("clientID") String clientID, | ||
| @JsonProperty("clientSecret") PasswordProvider clientSecret, | ||
| @JsonProperty("discoveryURI") String discoveryURI | ||
| @JsonProperty("discoveryURI") String discoveryURI, | ||
| @JsonProperty("oidcClaim") String oidcClaim |
There was a problem hiding this comment.
Since this parameter is used to specify an arbitrary claim value as the Druid AuthenticationResult's identity, perhaps we can give it a name such as identityClaim
|
@a2l007 - is there more work to be done for this PR? |
@abhishekagarwal87 No, this is good to go. There will be some work on this extension in a future PR when we upgrade the pac4j library dependencies. I haven't upgraded them yet since the newer versions are not backward compatible with JDK 8. |
| servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult); | ||
| } else { | ||
| LOG.error( | ||
| "Authentication failed! Please ensure the validity of the ID token and contains the configured claim."); |
There was a problem hiding this comment.
| "Authentication failed! Please ensure the validity of the ID token and contains the configured claim."); | |
| "Authentication failed! Please ensure that ID token is valid and it contains the configured claim."); |
There was a problem hiding this comment.
should we not reject the request at this point? If an id token is present, any error should result in an error. isn't it?
There was a problem hiding this comment.
My initial thought was to not break the authentication chain in case the user has any other custom authenticators further down the chain. But i see that if the request already has an id token, they have signaled intent to use that to authenticate.
| { | ||
| String header = request.getHeader(HttpConstants.AUTHORIZATION_HEADER); | ||
| if (header == null || !header.startsWith(HttpConstants.BEARER_HEADER_PREFIX)) { | ||
| LOG.debug("Invalid prefix for Bearer token"); |
There was a problem hiding this comment.
it doesn't really mean that prefix is invalid. It could be that this is not the right scheme.
There was a problem hiding this comment.
Fixed the error message here.
| servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult); | ||
| } else { | ||
| LOG.error( | ||
| "Authentication failed! Please ensure the validity of the ID token and contains the configured claim."); |
There was a problem hiding this comment.
should we not reject the request at this point? If an id token is present, any error should result in an error. isn't it?
|
Sorry for the delay @abhishekagarwal87 |
| // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation | ||
| IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null); | ||
| if (claims != null) { | ||
| Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim())); |
There was a problem hiding this comment.
Did you mean to use Optional.ofNullable here instead of Optional.of? This will currently throw an NPE if the claims are null.
| if (claims != null) { | ||
| Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim())); | ||
|
|
||
| if (claim.isPresent()) { |
There was a problem hiding this comment.
this is always true since Optional.of will throw an NPE if claims.getStringClaim return null
| } else { | ||
| LOG.error( | ||
| "Authentication failed! Please ensure that the ID token is valid and it contains the configured claim."); | ||
| httpServletResponse.sendError(HttpServletResponse.SC_UNAUTHORIZED); | ||
| return; | ||
| } |
There was a problem hiding this comment.
it sounds like we are missing a test case that handles the invalid claims code path, since this will never be executed as it is.
Description
Expands the OIDC based auth in Druid by adding a JWT Authenticator that validates ID Tokens associated with a request. The existing pac4j authenticator works for authenticating web users while accessing the console, whereas this authenticator is for validating Druid API requests made by Direct clients. Services already supporting OIDC can attach their ID tokens to the Druid requests
under the
Authorizationrequest header.This PR has: