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

drop legacy features #20

Closed
szuecs opened this issue Oct 8, 2021 · 5 comments · Fixed by #22
Closed

drop legacy features #20

szuecs opened this issue Oct 8, 2021 · 5 comments · Fixed by #22

Comments

@szuecs
Copy link
Contributor

szuecs commented Oct 8, 2021

We get this alert because of a dependency in this project to a vulnerable library:


Overview
Security policy
Security advisories
Dependabot alerts 1
Code scanning alerts
github.com/dgrijalva/jwt-go
Open
GitHub opened this alert 23 hours ago
1 github.com/dgrijalva/jwt-go vulnerability found in go.sum 23 hours ago
Remediation

No patched version is available.
Details
CVE-2020-26160
high severity
Vulnerable versions: <= 3.2.0
Patched version: No fix

jwt-go allows attackers to bypass intended access restrictions in situations with []string{} for m["aud"] (which is allowed by the specification). Because the type assertion fails, "" is the value of aud. This is a security problem if the JWT token is presented to a service that lacks its own audience check. There is no patch available and users of jwt-go are advised to migrate to golang-jwt at version 3.2.1
This was referenced Oct 8, 2021
@MicahParks
Copy link
Owner

Thank you, @szuecs, for this issue and the corresponding PR.

This is something that's been mentioned to me before and there was an earlier PR to remove the fork support. See: #15 (comment)

I stand behind the reasoning in that comment's second paragraph and would like to add that anyone using keyfunc is not subject to CVE-2020-26160 unless they are also actively using github.com/dgrijalva/jwt-go and the aud claim.

However, if I were on the the other side of this conversation, I probably wouldn't find that explanation sufficient, as I enjoy a repository with minimal dependencies and no security warnings as well. I've even noticed a project or two that has refrained from importing keyfunc as a dependency and decided embed a modified version instead. See: gofiber/jwt#57

I've come around to thinking that there needs to be an option ready with minimal dependencies for the latest version of github.com/golang-jwt/jwt. It is my intention to merge the keyfunc code upstream, so that no external dependencies from github.com/golang-jwt/jwt are required, not even the project at this path. At that point, the project at this path could be maintained for legacy forks. Here's my upstream PR: golang-jwt/jwt#105

The upstream PR is ongoing, but nobody has touched it in about two weeks. If people are in favor of this, I would appreciate it if they would react on the upstream PR with a positive emoji 👍 to their show support. Or a negative one to show their opposition! 👎

I planned on moving the project at this path to v1.0.0 after that PR was accepted or rejected. I'm thinking it might be time to plan for that PR to be officially rejected.

Here's my plan:
I'm accepting your PR, then I'm going to remove the other fork support. (The F3T fork was also recently archived and now points to github.com/golang-jwt/jwt.) That'll make the only dependency github.com/golang-jwt/jwt/v4. I've made github.com/MicahParks/compatibility-keyfunc for those who are stuck using legacy forks.

@MicahParks
Copy link
Owner

MicahParks commented Oct 8, 2021

@szuecs Please see the v.0.9.0 release.

@szuecs
Copy link
Contributor Author

szuecs commented Oct 8, 2021

@MicahParks thanks!!

@szuecs
Copy link
Contributor Author

szuecs commented Oct 8, 2021

I deleted my repository, because I was not sure how you decide, so I worked on making it ok for our project, but now I just need to increase the version, thanks !

@MicahParks
Copy link
Owner

Glad to hear that 😄 github.com/dgrijalva/jwt-go had enough forks. Don't want to repeat that pattern with github.com/MicahParks/keyfunc.

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 a pull request may close this issue.

2 participants