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

[release/2.7] github.com/golang-jwt/jwt v3.2.2 #3466

Merged
merged 1 commit into from Nov 23, 2021

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 29, 2021

closes #3465

relates to #3361
relates to #3459 (main branch)
alternative to #3465
closes #3465

Using the https://github.com/form3tech-oss/jwt-go.git, which is actively maintained.

full diff: form3tech-oss/jwt-go@a601269...v3.2.4

to address CVE-2020-26160

full diff: golang-jwt/jwt@a601269...v3.2.2

3.2.1 release notes

  • Import Path Change: See MIGRATION_GUIDE.md for tips on updating your code
    Changed the import path from github.com/dgrijalva/jwt-go to github.com/golang-jwt/jwt
  • Fixed type confusion issue between string and []string in VerifyAudience.
    This fixes CVE-2020-26160

3.2.2 release notes

  • Starting from this release, we are adopting the policy to support the most 2
    recent versions of Go currently available. By the time of this release, this
    is Go 1.15 and 1.16.
  • Fixed a potential issue that could occur when the verification of exp, iat
    or nbf was not required and contained invalid contents, i.e. non-numeric/date.
    Thanks for @thaJeztah for making us aware of that and @giorgos-f3 for originally
    reporting it to the formtech fork.
  • Added support for EdDSA / ED25519.
  • Optimized allocations.

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2021

Codecov Report

Merging #3466 (2b076bf) into release/2.7 (18230b7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/2.7    #3466   +/-   ##
============================================
  Coverage        58.77%   58.77%           
============================================
  Files              102      102           
  Lines             7085     7085           
============================================
  Hits              4164     4164           
  Misses            2280     2280           
  Partials           641      641           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18230b7...2b076bf. Read the comment docs.

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 29, 2021

From #3465 (comment)

Not against using a replace if vndr can do that. Should we consider using the fork which the original repository now links to? https://github.com/golang-jwt/jwt

Looks like there's another fork as well (and that's the "official" replacement?)

@thaJeztah thaJeztah changed the title [release/2.7] vendor: github.com/dgrijalva|form3tech-oss/jwt-go v3.2.4 [release/2.7] github.com/golang-jwt/jwt v3.2.1 Jul 29, 2021
exp, ok := m["exp"]
if !ok {
return !req
}
switch expType := exp.(type) {
switch exp := m["exp"].(type) {
Copy link
Member Author

@thaJeztah thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the official repository does not have the security fix that the orm3tech-oss/jwt-go fork has; switching to github.com/golang-jwt/jwt removes these changes, which seem to be related to that security fix: form3tech-oss/jwt-go@v3.2.3...v3.2.4 (see form3tech-oss/jwt-go#14)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe golang-jwt/jwt v3.2.1 fixed CVE-2020-26160 with different code: golang-jwt/jwt@0f726ea

Copy link
Member Author

@thaJeztah thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equivalent to that one is in form3tech-oss/jwt-go@bb5e6d8 for the form3tech repository. Looks like v3.2.4 is fixing a different security issue?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case the fix for CVE-2020-26160 is in form3tech-oss/jwt.go v3.2.2 and both #3459 and #3465 do fix CVE-2020-26160.

Copy link
Member Author

@thaJeztah thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I suspect no CVE has been requested for the security fix in form3tech-oss/jwt-go v3.2.4. If it's verified to be a security issue, a CVE should probably be requested (both for form3tech-oss/jwt-go, and for github.com/golang-jwt/jwt / dgrijalva/jwt-go)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we merge those two PRs while we wait for the situation with this new vulerability to be resolved upstream or do you think the workaround in this PR is a better fix for right now?

Copy link
Member Author

@thaJeztah thaJeztah Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern with #3465 is that it's pulling in 5.6k lines of code that's unrelated to the security fix (and which would have to be reviewed), with the goal to pull in a security fix that's < 50 lines. And (worse?) switching to a non-official fork (which, based on the go.mod addition, is at least somewhat "dubious" on quality).

From that perspective, I'd rather take the official fork (ideally with golang-jwt/jwt#40, if that's confirmed to be an issue), as is done in this PR.

For the main branch, I don't think there's an direct urgency to get it merged, unless a v3.0.0 release is pending.

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 29, 2021

Looks like https://github.com/golang-jwt/jwt is the best option, and golang-jwt/jwt#40 is currently being reviewed, so I squashed the commits, and went for that fork (as it's the official replacement).

@thaJeztah thaJeztah changed the title [release/2.7] github.com/golang-jwt/jwt v3.2.1 [release/2.7] github.com/golang-jwt/jwt v3.2.2 Jul 30, 2021
@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 30, 2021

Updated to v3.2.2, which was just released

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 30, 2021

Looks like we need to update Golang as well;

/docker/distribution/vendor/github.com/dgrijalva/jwt-go/ecdsa.go:135:5: r.FillBytes undefined (type *big.Int has no field or method FillBytes

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 30, 2021

ok, works with Go 1.16, but should be moved to a separate PR

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 30, 2021

opened #3472

to address CVE-2020-26160

full diff: golang-jwt/jwt@a601269...v3.2.2

3.2.1 release notes
---------------------------------------

- Import Path Change: See MIGRATION_GUIDE.md for tips on updating your code
  Changed the import path from github.com/dgrijalva/jwt-go to github.com/golang-jwt/jwt
- Fixed type confusion issue between string and []string in VerifyAudience.
  This fixes CVE-2020-26160

3.2.2 release notes
---------------------------------------

- Starting from this release, we are adopting the policy to support the most 2
  recent versions of Go currently available. By the time of this release, this
  is Go 1.15 and 1.16.
- Fixed a potential issue that could occur when the verification of exp, iat
  or nbf was not required and contained invalid contents, i.e. non-numeric/date.
  Thanks for @thaJeztah for making us aware of that and @giorgos-f3 for originally
  reporting it to the formtech fork.
- Added support for EdDSA / ED25519.
- Optimized allocations.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 10, 2021

Rebased, because #3472 was merged

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 13, 2021

Thinking a bit more about this PR: does this project actually perform the validation of the JWT token? Because if not, then this patch may not be needed at all

@milosgajdos
Copy link
Member

milosgajdos commented Nov 17, 2021

@thaJeztah distribution has token auth package that does verification https://github.com/distribution/distribution/blob/release/2.7/registry/auth/token/token.go

@milosgajdos milosgajdos requested a review from wy65701436 Nov 17, 2021
@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 17, 2021

@thaJeztah distribution has token auth package that does verification

Ah, thanks for checking!

@milosgajdos milosgajdos requested review from joaodrp and waynr Nov 18, 2021
Copy link
Collaborator

@wy65701436 wy65701436 left a comment

lgtm

@milosgajdos milosgajdos merged commit afe8542 into distribution:release/2.7 Nov 23, 2021
2 checks passed
@thaJeztah thaJeztah deleted the 2.7_update_jwt branch Nov 23, 2021
@thaJeztah thaJeztah added this to the Registry/2.7.2 milestone Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants