Handle verification of JWT tokens gracefully. #350
Conversation
nk1506
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike and
ebyhr
as code owners
| // TokenExpiredException - if the token has expired. | ||
| // InvalidClaimException - if a claim contained a different value than the expected one. | ||
| throw new NotAuthorizedException( | ||
| "Failed to verify the token with cause %s and reason %s", e.getCause(), e.getMessage()); |
There was a problem hiding this comment.
Will this message be reported to the client? Exposing too specific validation messages (like Token Expired, Invalid Claim) may not be desirable from the security perspective.
There was a problem hiding this comment.
I'd suggest logging the specific reason on the server side and returning a generic failure message to the client. WDYT?
If we want debugging aids, we could add the same fresh UUID to both messages for correlation.
There was a problem hiding this comment.
I think "Token expired" should be good others reasons are not required to throw. Also the other validations looks redundant to me.
if (decodedJWT.getExpiresAtAsInstant().isBefore(Instant.now())) {
throw new NotAuthorizedException("Token has expired");
}
Adding UUID is a good idea.
There was a problem hiding this comment.
Yeah, anything that can be derived from the public JWT data (e.g. expiry) is ok to mention in the public error message (to help with trouble-shooting).
There was a problem hiding this comment.
We shouldn't add another UUID. The traceId and spanId are already in the Logging MDC - https://github.com/collado-mike/polaris/blob/8cb6b44bf57dc597dab612d109d3eb534aef5715/polaris-service/src/main/java/org/apache/polaris/service/tracing/TracingFilter.java#L80-L82 .
I think a great idea would be to modify the IcebergExceptionMapper to include the traceId in the error message (or maybe we can subclass the ErrorResponse to have its own traceId field?). This would include the traceId for every error returned so we can always find the log statements that correspond to the error
There was a problem hiding this comment.
I've also recently introduced a PolarisExceptionMapper, we could use that
There was a problem hiding this comment.
I have done some modification of token verifying logic. this should do expiry and token status validation.
JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();
I have added tests for the same.
I might need help for adding the trace it. Do we have traceId set in thread local ? or I should just assign a randomId with exception and logs.
There was a problem hiding this comment.
Although other fix would have been adding JWTVerificationException at IcebergExceptionMapper as
case JWTVerificationException e -> Response.Status.UNAUTHORIZED.getStatusCode();
But this will start showing all the token validation failed reasons as response entity.
There was a problem hiding this comment.
@eric-maynard / @collado-mike , PTAL. let me know if there are any thoughts.
There was a problem hiding this comment.
+1 to adding the traceId to the error message
|
|
||
| } catch (JWTVerificationException e) { | ||
| LOGGER.error( | ||
| "Failed to verify the token with cause {} and message {}", e.getCause(), e.getMessage()); |
There was a problem hiding this comment.
suggestion: add the whole e object (instead of individual e.getCause() and e.getMessage()) to the log call to get its stack trace printed.
|
@collado-mike @eric-maynard , Please review . There was a flaky test failure with new test Class so I had to move tests to PolarisServiceImplIntegrationTest |
Description
Expired JWT token should return 401 instead of 5xx.
Fixes # (issue)
#304
Please delete options that are not relevant.
Checklist:
Please delete options that are not relevant.