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 backport] remove github.com/dgrijalva/jwt-go #3465

Closed

Conversation

brackendawson
Copy link
Contributor

@brackendawson brackendawson commented Jul 29, 2021

By updating github.com/Azure/go-autorest/autorest to v.0.11.19 (10e0b31633f168ce1a329dcbdd0ab9842e533fb5)

Backport of #3459

#3361

By updating github.com/Azure/go-autorest/autorest to v.0.11.19 (10e0b31633f168ce1a329dcbdd0ab9842e533fb5)

Backport of github.com/distribution#3459

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##           release/2.7    #3465   +/-   ##
============================================
  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...5906192. Read the comment docs.

@thaJeztah thaJeztah changed the title Remove github.com/dgrijalva/jwt-go [release/2.7 backport] remove github.com/dgrijalva/jwt-go Jul 29, 2021
@@ -8,9 +8,9 @@ github.com/bugsnag/bugsnag-go b1d153021fcd90ca3f080db36bec96dc690fb274
github.com/bugsnag/osext 0dd3f918b21bec95ace9dc86c7e70266cfc5c702
github.com/bugsnag/panicwrap e2c28503fcd0675329da73bf48b33404db873782
github.com/denverdino/aliyungo afedced274aa9a7fcdd47ac97018f0f8db4e5de2
github.com/dgrijalva/jwt-go a601269ab70c205d26370c16f7c81e9017c14e04
Copy link
Member

Choose a reason for hiding this comment

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

I see updating Azure/go-autorest brings a substantial number of code changes (for a patch release).

If the bug is in this library, could we instead update just this library to a version with the fix? I see maintains a fork with the fix (IIUC), and we can specify it with a custom location (the equivalent to replace in go.mod);

Suggested change
github.com/dgrijalva/jwt-go a601269ab70c205d26370c16f7c81e9017c14e04
github.com/dgrijalva/jwt-go a211650c6ae1cff6d7347d3e24070d65dcfb1122 https://github.com/form3tech-oss/jwt-go.git # v3.2.4

That would only bring the diff of the jwt-go package;
form3tech-oss/jwt-go@a601269...v3.2.4

Copy link
Member

Choose a reason for hiding this comment

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

gave it a quick attempt; #3466

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Arf... there's two forks now, and both being actively maintained? 😞

Copy link
Member

Choose a reason for hiding this comment

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

I updated #3466 with a commit (allowing the differences between those forks to be reviewed)

github.com/docker/go-metrics 399ea8c73916000c64c2c76e8da00ca82f8387ab
github.com/docker/libtrust fa567046d9b14f6aa788882a950d69651d230b21
github.com/form3tech-oss/jwt-go 9162a5abdbc046b7c8b03ee90052cee67e25caa7
Copy link
Member

Choose a reason for hiding this comment

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

This commit looks to be matching v3.2.2; form3tech-oss/jwt-go@9162a5a...v3.2.2, which is missing a security fix in v3.2.4; form3tech-oss/jwt-go@v3.2.2...v3.2.4 (see https://github.com/form3tech-oss/jwt-go/tree/v3.2.4)

@milosgajdos
Copy link
Member

If we want this fix in 2.7.x, should we just propagate #3138 i.e. update Azure autorest to ``v0.11.20` and be done with this? If we do do that, we should then close #3466 🤔

@thaJeztah
Copy link
Member

#3138 would bring in an even larger diff.

Screenshot 2021-09-05 at 20 46 26

The security fix is only a few line changes (which is why I opened #3466)

@milosgajdos
Copy link
Member

which is why I opened #3466

Oh, right. There have been a few related PRs flying around this issue that I've completely lost the track.
I say let's merge #3466. We probably don't need it per se, but if there is nothing that prevents us from having a peace of mind I see no problem with doing that 🤔

@milosgajdos
Copy link
Member

Let's close this one in favour of #3466

@thaJeztah
Copy link
Member

looks like github didn't auto-close this one for some reason. #3466 was merged, so let me close this one (thanks @brackendawson !)

@thaJeztah thaJeztah closed this Nov 23, 2021
@brackendawson brackendawson deleted the backport-3459 branch November 24, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/P1 Major item. Should definitely have it. security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants