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

Switch to github.com/golang-jwt/jwt #1172

Merged
merged 4 commits into from Aug 19, 2021
Merged

Switch to github.com/golang-jwt/jwt #1172

merged 4 commits into from Aug 19, 2021

Conversation

vovinacci
Copy link
Contributor

Closes #1026

This PR switches to github.com/golang-jwt/jwt to address security vulnerability. More details in the issue and GHSA-w73w-5m7g-f7qc

@peterbourgon
Copy link
Member

peterbourgon commented Jul 27, 2021

@sagikazarmark Look good to you?

Just saw discussion here. Remind me the breaking change? Just that the types are different?

@sagikazarmark
Copy link
Contributor

After taking a more thorough look at the implementation, I'm not sure that there is a breaking change, but I suspect there might be.

Basically, those different types referenced in the public API are all interfaces and I assume the new package didn't break those overnight, so in theory the replaced types could be backwards compatible with the old library.

In practice, though, some of the types used internally (eg. standard claims struct) are not interfaces, so those could break.

A simple test to check if there is a breaking change would be (temporarily) changing back the packages in tests to the old module to see if it works with the new code.

I'm not particularly worried about a breaking change (the library is basically abandoned after all), but if there is a breaking change, it should be released in a minor version, not a patch. Go 1.17 is just around the corner and Chris wants to cut a new release after that (hoping that we can further reduce the amount of dependency downloads).

@peterbourgon
Copy link
Member

OK let's bench this until the next release.

@peterbourgon peterbourgon added this to the v0.12.0 milestone Jul 28, 2021
@vovinacci
Copy link
Contributor Author

@peterbourgon Do you have any preliminary release date in mind?

@AliLogic
Copy link

Any updates on this?

Copy link
Contributor

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Can you please upgrade the library to v4?

go.mod Outdated Show resolved Hide resolved
@vovinacci
Copy link
Contributor Author

@sagikazarmark Done

@shogo82148
Copy link

I recommend to upgrade to Go 1.17 pruned module graphs.

https://golang.org/doc/go1.17#go-command

go mod tidy -go=1.17

It enables some optimizations, such as Lazy module loading.

@eli-darkly
Copy link

@shogo82148 If that would require setting the minimum version to 1.17 in go.mod, I would very strongly prefer that they not make that change, at least not yet. Go 1.17 was just released and we can't all immediately update our projects to require it.

@sagikazarmark
Copy link
Contributor

I recommend to upgrade to Go 1.17 pruned module graphs.

This is already in progress on another branch. Not sure which will be merged first yet.

@peterbourgon peterbourgon merged commit fd3754b into go-kit:master Aug 19, 2021
@andig
Copy link

andig commented Aug 20, 2021

@shogo82148 If that would require setting the minimum version to 1.17 in go.mod, I would very strongly prefer that they not make that change, at least not yet. Go 1.17 was just released and we can't all immediately update our projects to require it.

Imho this does not raise the minimum version.

@vovinacci vovinacci deleted the migrate-from-jwt-go branch August 20, 2021 08:55
@eli-darkly
Copy link

@andig:

Imho this does not raise the minimum version.

Could you say more about why you think it wouldn't? My reading of the linked documentation is that pruned module graphs are only a thing in go.mod files that specify go 1.17.

@shogo82148
Copy link

pruned module graphs are just required directives.
So, Go 1.16 or earlier parses them as extra dependencies.
This has no negative effects, because these extra dependencies are also in go.sum and Go 1.16 loads whole dependencies in go.sum.

The go 1.17 directive enables new language features, and changes the go command behavior. https://golang.org/ref/mod#go-mod-file-go
However it doesn't break backward compatibility.

When Go 2.0 comes out in the future, Go 2.0 will not be able to import modules that contain the go 1.17 directive.

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.

High severity issue in github.com/dgrijalva/jwt-go
7 participants