Fix EC token signature verification, add verify against JWKS URL, and more#60
Open
ulidtko wants to merge 8 commits intoticarpi:masterfrom
Open
Fix EC token signature verification, add verify against JWKS URL, and more#60ulidtko wants to merge 8 commits intoticarpi:masterfrom
ulidtko wants to merge 8 commits intoticarpi:masterfrom
Conversation
Author
|
Yearly ping @ticarpi. EC token sigs are still broken, care to review? |
Author
|
Retested with RSA tokens, too. |
Author
|
Bump @ticarpi, any review comment? |
|
I find so annoying that an effort for the community has been ignored for so long. Just wanted to say that and express my sympathy |
The correct variable name is `sig`, but under try: it's referred to as `signature`. Normally that'd crash with NameError exception -- but here we have a catch-all except block misinterpreting that as wrong signature.
Keys can be/are of wildly different types, including different elliptic curves. IETF RFC7518 (JWA) section 3.4 table mandates this 3-row map: | JWT.alg | Hash, curve | ES256 | SHA256, P-256 | ES384 | SHA384, P-384 | ES512 | SHA512, P-521 The assert (unless disabled with -O) will clearly fail when JWT mismatches the pubkey, (as far as by curve choice).
For those JWK's which lack the kid attribute, the logic assigns one.
When parsing pubkey bundle (JWKS, a set of JWK), the previous logic
enables a clash, consider this JWK sequence:
* {"kid": "2", "kty":"EC", "use":"sig", ... }
* {"kty":"RS", "use":"sig", ... } -- this saves with kid=1
* {"kty":"RS", "use":"enc", ... } -- this *overwrites* kid=2
Author
|
Thanks @halfluke for the kind words 🙏 What's most annoying to me, is that other PRs #101 #108 do get reviewed & merged, and releases come out every so often. But radio silence here, no feedback whatsoever. Ping @ticarpi @rbrown256 @JJK96 anything I should change to land this fix?.. FWIW, rebased once again to resolve the merge confict. |
Contributor
|
This looks like a worthwhile merge to me. Thanks for your contribution. @ticarpi any chance of merging this in? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Greetings @ticarpi !
I'm unshelving a bunch of fixes to the tool, please consider merging.
Most important first:
I'm sure you know, there are Elliptic Curve based JWT algos, EC256 EC384 EC521. They were not getting sig-verified correctly. The bug may be hard to see in the diff, but it's pretty simple, I'll just echo the commit message:
With this fix, I got complete parity with https://jwt.io & other implementations.
I also added a straightforward "verify token against JWKS URL" mode, for UX speed reasons.
Some more bits and pieces too, I hope the rest will be obvious.
I tested the changes with real keys and tokens, mostly EC ones.
Feel free to question anything unclear in review; I'm hoping to get the PR landed, completely in the spirit of FOSS to make the tool ever a bit sharper 🚀
Best regards