Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Securty Update for CVEs in JWT lib #101

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Conversation

HaBaLeS
Copy link
Contributor

@HaBaLeS HaBaLeS commented Oct 23, 2021

Move form JWT: https://github.com/dgrijalva/jwt-go to Community maintained clone https://github.com/dgrijalva/jwt-go for CVE's reported by Dependabot

Move form JWT: https://github.com/dgrijalva/jwt-go to Community maintained clone https://github.com/dgrijalva/jwt-go for CVE's reported by Dependabot
@uberswe
Copy link

uberswe commented Oct 25, 2021

I don't think you intended to change go modules to your forked repo in this PR @HaBaLeS ? 9058f7c

@HaBaLeS
Copy link
Contributor Author

HaBaLeS commented Oct 25, 2021

I don't think you intended to change go modules to your forked repo in this PR @HaBaLeS ? 9058f7c

Ups ... yes you are right. Reminds myself why not to force-push :-/

@nklaassen
Copy link

Move form JWT: https://github.com/dgrijalva/jwt-go to Community maintained clone https://github.com/dgrijalva/jwt-go for CVE's reported by Dependabot

Typo here, I think you mean that this PR switches from https://github.com/dgrijalva/jwt-go to the community maintained https://github.com/golang-jwt/jwt. Would like to see this merged so that we can continue to use this library without forking

@oschwald
Copy link

It would be great to see this merged so that we don't have to switch to a fork.

@james-d-elliott
Copy link
Contributor

It would be great to see this merged so that we don't have to switch to a fork.

For this particular fix you wouldn't need to. A replace on the affected downstream library will do it. Just add this to your go.mod:

replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt/v4 v4.2.0

@oschwald
Copy link

@james-d-elliott, thanks for mentioning that. I hadn't looked closely enough at the PR to realize that golang-jwt was completely compatible with jwt-go.

@james-d-elliott
Copy link
Contributor

james-d-elliott commented Dec 15, 2021

Yep github.com/golang-jwt/jwt is the "official" community fork of the original and the original is officially unmtaintained. See dgrijalva/jwt-go#462 for more info. The main difference between v3 and v4 is that v3 didn't properly setup the module path (hence the +incompatible suffix); as well as the CVE fix.

Also just as an educated guess, since the JWT lib is only used for MDS, I don't believe the CVE actually affects this library directly.

@MasterKale
Copy link
Collaborator

MasterKale commented Dec 16, 2021

I just got write access to this repo so I can at least merge PRs. I am not in a position to actually review code beyond a PR like this (I'm just not knowledgeable in Golang) so I'll be relying on feedback from @nicksteele and other maintainers to gauge which PR's are suitable to merge.

That said this PR seems fine to me. I'm considering merging this and closing #95 since this PR updates to a more recent version of golang-jwt/jwt. Any objections?

@james-d-elliott
Copy link
Contributor

james-d-elliott commented Dec 16, 2021

Yeah that seems like the best course of action in my opinion. For reference I'd rate #93 as the most important fix outstanding by the way, and it's a really small change that Nick should be able to review relatively quickly.

The change in this PR updates a library for a CVE that actually doesn't affect this repos usage of the library, at least as far as I can tell. See here for the CVE description: https://nvd.nist.gov/vuln/detail/CVE-2020-26160

But your usage is actually for parsing MDS data, which I don't believe uses audience and would generally not need validation (of the audience) since you're validating the signature and only using the information for metadata, not authorization/access.

@davedoesdev davedoesdev mentioned this pull request Dec 16, 2021
@MasterKale MasterKale merged commit 092d5e0 into duo-labs:master Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants