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

Remove github.com/dgrijalva/jwt-go for CVE-2020-26160. #15

Closed

Conversation

elliotcourant
Copy link

This removes the dependency on github.com/dgrijalva/jwt-go entirely in
order to prevent using it in the future. Due to the vulnerability of the
dependency in CVE-2020-26160 and the lack of active maintenance on the
library.

The GitHub overview of this CVE is available here: CVE-2020-26160

This does introduce a breaking change (though no more breaking than renaming the method anyway) by removing the dependency entirely. But will resolve any security notices that users of this library may be encountering circumstantially just by using the keyfunc library.

Maybe this would warrant a 0.7.1 or 0.8.0?

Curious what your thoughts on in general on removing this dependency @MicahParks

This removes the dependency on github.com/dgrijalva/jwt-go entirely in
order to prevent using it in the future. Due to the vulnerability of the
dependency in CVE-2020-26160 and the lack of active maintainence on the
library.
@MicahParks
Copy link
Owner

Thank you @elliotcourant for this PR. This project will not be dropping support for github.com/dgrijalva/jwt-go.

The biggest reason is because not everyone will be switching from github.com/dgrijalva/jwt-go to github.com/golang-jwt/jwt/v4. This may sound silly to some people, because they are fully compatible, but switching isn't always a technology problem. Some entities, like governments, are unintentionally resistant to software changes. Depending on the government office, the software developers may be responsible for pages of explanations and approvals per dependency, which acts as a deterrent to switching dependencies. It might not be worth it for some developers, considering that this CVE may not apply to their use case. I'd like this repository to be ready to ship for those developers and developers in other circumstances where they cannot switch to github.com/golang-jwt/jwt/v4.

It's been my intention to make this project compatible with all JWT use cases at the cost of a minimal project. As I see it, the biggest benefit to dropping support for github.com/dgrijalva/jwt-go would be the removal of the security notice. At that point, if github.com/form3tech-oss/jwt-go were removed, the project would have minimal dependencies.

I'm aware that people prefer minimal dependencies. A perfect example is this PR: gofiber/jwt#57

I believe I could get away with smaller changes if I would reference https://github.com/MicahParks/keyfunc directly. But that would bring additional unused legacy dependencies to this middleware and to the rest of the projects which will be using it, so I decided to copy files instead.

I've had plans for a while to make an upstream PR (with upstream being github.com/golang-jwt/jwt/v4). This would mean this project's keyfunc code would be in the github.com/golang-jwt/jwt/v4 and it would help fulfill this issue: golang-jwt/jwt#73. This wouldn't include any fork support, so it would have minimal dependencies. In fact, I started working on this PR yesterday! Check out https://github.com/MicahParks/jwt. All I have left to do is add more documentation. When the PR is submitted, I'd appreciate it if you'd voice your support with a thumbs up emoji or constructive review comment.

@MicahParks MicahParks closed this Sep 16, 2021
@MicahParks
Copy link
Owner

@elliotcourant check out golang-jwt/jwt#105 if you are interested.

@elliotcourant
Copy link
Author

Will do, thank you very much!

@MicahParks MicahParks mentioned this pull request Oct 8, 2021
@MicahParks
Copy link
Owner

@elliotcourant This ended up being merged into this repository. Please see this release: https://github.com/MicahParks/keyfunc/releases/tag/v0.9.0

@elliotcourant
Copy link
Author

Thank you very much for letting me know! I appreciate it

@MicahParks MicahParks mentioned this pull request Aug 28, 2022
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.

2 participants