-
Notifications
You must be signed in to change notification settings - Fork 334
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
upstream fix for security vulnerability from form3tech-oss/jwt-go fork #40
Conversation
/cc @dims @brackendawson This is just a quick cherry-pick; there were some conflicts though, so if someone with more knowledge could have a close look (or wants to carry this), let me know edit: conflicts were in a comment, and in the test (both easy to resolve), but more eyes won't hurt 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. Thanks for porting this over from the other fork.
Just some smaller things that should be corrected.
Fixes a security vulnerability where a jwt token could potentially be validated having invalid string characters. (cherry picked from commit a211650c6ae1cff6d7347d3e24070d65dcfb1122) form3tech-oss/jwt-go#14 Co-Authored-By: Giorgos Lampadakis <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
3479ee4
to
58c5ce9
Compare
@oxisto thanks for reviewing; I think I addressed your comments; PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I will tacke the test consolidation in a future issue, I need to rework the TestVerifyAud
method first.
LGTM ( non-binding of course! ) |
Based on a quick glance over distribution/distribution#3361, wasn't that issue addressed in #12 I don't see how this PR fixes that. My only concern is the implementation is a breaking change, we're modifying the behaviour of what happens when the field is set but is not the correct type. I do agree with this implementation though. |
Correct, #12 is to address CVE-2020-26160. This patch is unrelated, but was in the form3tech-oss/jwt-go, and marked as a security issue. I doubt a CVE has been requested for this one. (neither am I familiar enough with this to determine what the impact is and/or if it's indeed a security issue 😅). Also see my comment on distribution/distribution#3466 (comment) |
Basically, my goal here was trying to prevent users of the https://github.com/dgrijalva/jwt-go package having to decide between either using the "official" maintained fork and (potentially) miss out on a security fix, or an "unofficial fork" with a security fix. 🤷♂️ |
Ye that's fair. Thanks for submitting this PR. We'll call this out in the changelog release. |
Thanks everyone! If I find some time, I'll write up a ticket in the other fork, to see if maintainers of that fork are ok with abandoning the fork in favor of this one (possibly upstream other changes they have) |
This forwards the changes of form3tech-oss/jwt-go#14 to the upstream repository.
I am not the author of the original PR (nor do I know much about JWT), but thought I'd give this an attempt, following the discussion in Azure/go-autorest#642 (and issues linked from there).
Fixes a security vulnerability where a jwt token could potentially be validated having invalid string characters.
(cherry picked from commit a211650c6ae1cff6d7347d3e24070d65dcfb1122)
form3tech-oss/jwt-go#14