Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue#427 DefaultJWTTokenParser#parseClaims Exception Handling #429

Merged
merged 2 commits into from Mar 1, 2021

Conversation

SoniaZaldana
Copy link
Contributor

Fixes #427

When a invalid public key has been configured in the mp.jwt.verify.publickey.location, an UnresolvableKeyException should be reported. However, if this exception arises inside the call to the JOSE method, then the UnresolvableKeyException gets wrapped in a InvalidJwtException by the JOSE method, resulting in the wrong error code being sent in JWTHTTPAuthenticationMechanism.

@sberyozkin
Copy link
Contributor

sberyozkin commented Feb 25, 2021

@SoniaZaldana Thanks; FYI, testsuite/basic fails, please update.

I think it is a correct decision - this is the case when JWT may well be valid, the key is unresolvable, so indeed it is correct to throw this exception unwrapped - cause of InvalidJwtException should be a concrete JWT problem which caused it. Technically it is a breaking change, but putting it into a different surface, I'd qualify it as a bug fix, hence the backward compatibility concerns do not quite apply.
We can do 3.1.0 though, @darranl is it what you meant when you were referring to a possible 3.1.0 (instead of 3.0.1) ?

@SoniaZaldana
Copy link
Contributor Author

hi @sberyozkin I was taking a closer look at one of the test failures DefaultJwtParserTest#testParseWithConfiguredCertAndThumbprintMissing. In this test case, with the code that I added, it is reporting an UnresolvableKeyException when the JWS is missing the X5T or X5T#S256 header. I am wondering whether this would be considered a concrete JWT problem where an InvalidJWTException might be more appropriate due to the missing headers or whether we still consider this a potentially valid JWT where we were just unable to resolve the public key?

@sberyozkin
Copy link
Contributor

@SoniaZaldana Sorry I have missed this comment, I thought I checked it during the recharge day :-)

No, this is still an UnresolvedKeyException as that test is about a case where a thumbprint is acting as a kid for the X509 resolver to match it against the pre-calculated thumbprint from the x5cchain JWK property - the key would then come from the matched x5c. It is not a standard verification process as well - it currently depends on the smallrye-jwt specific property.

@SoniaZaldana
Copy link
Contributor Author

Thanks @sberyozkin! I updated the PR and all tests pass now.

@sberyozkin
Copy link
Contributor

@SoniaZaldana Nice work, thanks for your careful analysis of the failing tests as well

@sberyozkin sberyozkin self-requested a review March 1, 2021 15:24
@sberyozkin sberyozkin merged commit 1efa854 into smallrye:master Mar 1, 2021
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.

DefaultJwtTokenParser#parseClaims Exception Handling
2 participants