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

Enhance JWTParser #265

Merged
merged 1 commit into from
Jul 3, 2020
Merged

Enhance JWTParser #265

merged 1 commit into from
Jul 3, 2020

Conversation

sberyozkin
Copy link
Contributor

@sberyozkin sberyozkin commented Jun 23, 2020

Fixes #262

This PR is large but only because it ports some of the MP JWT 1.2 branch into it. Fundamentally it is all pretty simple, it is all about making it straight-forward to convert an encoded token string to JsonWebToken as we've had several discussions around a simple verification/decryption API.
For example, in Quarkus, we are talking about a fast way to create JWT as a cookie, and then verify/decrypt it on the way back, without using Jose4j/etc directly.
So this PR:

  • enhances JWTParser to verify or decrypt the token with all types of keys. If it is injected then the internal injected JWTAuthContextInfo will be reused and only the provided key parameters will be used to reset the specific properties. Or DefaultJWTParser can be safely used directly.
  • Get the decryption support from mpjwt12 branch (with a few improvements), this gives us a complete end to end support, where we can sign, encrypt, sign/encrypt with the builder API and correctly process with JWTParser. Roberto was suggesting to do it earlier on the master branch, I did not want the new mp.* properties, so for now I've added smallrye specific property which will be deprecated once MP JWT 1.2 is out...
  • Documentation and other minor improvements

This is the last PR I'd like to do before the next release

@sberyozkin
Copy link
Contributor Author

Note I've also deprecated a require issuer property - no such mp.jwt. property exists in the spec, and I think it is better be gone anyway, if we ever would want to let users not to verify issuer (I honestly see no reason for it) then we'd make mp.jwt.verify.issuer optional. Right now, setting mp.jwt.verify.issuer=NONE works anyway if someone would like to do it...

@sberyozkin
Copy link
Contributor Author

sberyozkin commented Jun 25, 2020

Hi All

I've been busy syncing the verification and decryption key resolvers together, it is not bad, and I expect going forward, there will be very minimum duplication, right now one deals with the private keys, the other one with the public ones, but most of the code will be shared in time.

@sberyozkin
Copy link
Contributor Author

I'd really like to have some progress on this issue, now it will unlikely make it to Quarkus 1.6.0.Final.
Let me summarize again:

  • the decryption code is backported to the master from the mpjwt12 branch in order to support new JWTParser methods.
  • the decryption works like this: if only one of the decryption key properties is set the the directly encrypted claims are expected, but if one of the verify key properties is also set then the decrypted content is expected to be a signed token which goes through the usual signature verification process. So it is really straightforward.

@MikeEdgar
Copy link
Member

@sberyozkin, I'm meaning to take a look at this. I have not been able to commit the time it deserves yet, hopefully in a day or so.

@sberyozkin
Copy link
Contributor Author

@MikeEdgar thanks.

Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

@sberyozkin - a few comments/observations:
(1) I think using the mp.* config name would be ideal if the behavior will not be changing between this and 1.2. Just my opinion.
(2) The two concrete key location resolver classes are very similar except for the configuration retrieval calls that it seems like more code could be moved to the abstract parent. Maybe add some abstract methods like getKeyContent and getKeyLocation to eliminate the specific calls to those methods for the decryption and public keys and move the instance Key field to the parent as well?
(3) The new DecryptionKeyLocationResolver is not covered by any tests. Just to make you aware. I think if something could be done to eliminate common code in (2), it would be less of a concern.

Otherwise, looks good. I'll wait for your feedback to the comments before approving.

@sberyozkin
Copy link
Contributor Author

sberyozkin commented Jul 2, 2020

@MikeEdgar Thanks for starting the review

(1) I think using the mp.* config name would be ideal if the behavior will not be changing between this and 1.2. Just my opinion.

I was concerned about reusing the mp.jwt namespace - in general we use smallrye.jwt for anything that is not in the spec. It is probably not a big deal, but it can be taken literally by some integrations' customers - why mp.jwt..., where is the 1.1.1 text about them, after talking to Darran @darranl (in some earlier issue discussions) I'm trying to be more precise about it...

(2) The two concrete key location resolver classes are very similar except for the configuration retrieval calls that it seems like more code could be moved to the abstract parent. Maybe add some abstract methods like getKeyContent and getKeyLocation to eliminate the specific calls to those methods for the decryption and public keys and move the instance Key field to the parent as well?

Yeah, I've tried to push as much as possible to the abstract class but I've found it difficult to abstract cleanly more code given that one calls typed public key extraction methods in KeyUtils, the other one - private key extraction methods. I think I can do more once I get the symmetric keys supported, I will be at least be able to push Key to the abstract level, etc

(3) The new DecryptionKeyLocationResolver is not covered by any tests. Just to make you aware. I think if something could be done to eliminate common code in (2), it would be less of a concern.

It is tested indirectly in DefaultJWTParserTest (the PEM key content), but I'll also add a JWK test, and copy the resolver test from the mpjwt12

Thanks

@sberyozkin
Copy link
Contributor Author

@MikeEdgar Hi Mike, so I've done some work around the key location verifiers and I've pushed most of the code down to the abstract class. Also added a unit test for the decryption key location verifier, and a test to check PublicJsonWebKey.getPrivateKey() works. (it should really be in Jose4j RsaJsonWebkey but it is a minor issue).
Fixed a few minor typos along the way which was caused by the duplication, so I'm much happier now about this code :-)
Cheers

Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

@sberyozkin - changes look good.

@sberyozkin
Copy link
Contributor Author

Hi Mike, thank you for finding the time to do the review - as always it helped :-)
Sorry, this PR is too large, and I was in a rush, but the code is a bit better now. I think the next version should be 2.2.0 just to highlight it.
Cheers

@sberyozkin sberyozkin added this to the 2.1.3 milestone Jul 3, 2020
@sberyozkin sberyozkin merged commit 319ec6b into smallrye:master Jul 3, 2020
@sberyozkin sberyozkin deleted the enhance_jwt_parser branch July 3, 2020 20:34
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.

Provide an API to verify/decrypt token directly in the service code
2 participants