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 from dgrijalva/jwt-go to form3tech-oss/jwt-go. #12580

Merged
merged 2 commits into from
Jan 16, 2021

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented Dec 24, 2020

dgrijalva/jwt-go has been abandoned and contains several serious
security issues. Most projects are now switching to the form3tech fork.

See https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMDGRIJALVAJWTGO-596515 for
info on the issues.

@dlorenc dlorenc force-pushed the update branch 2 times, most recently from 3bf3aed to cc589ef Compare December 24, 2020 16:02
@dlorenc
Copy link
Contributor Author

dlorenc commented Dec 28, 2020

Looks like this was a timeout, Is there anyway to rerun it?

@dlorenc
Copy link
Contributor Author

dlorenc commented Jan 6, 2021

Looks like this was a timeout, Is there anyway to rerun it?

Rebased, passing now!

dgrijalva/jwt-go has been abandoned and contains several serious
security issues. Most projects are now switching to the form3tech fork.

See https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMDGRIJALVAJWTGO-596515 for
info on the issues.

Signed-off-by: Dan Lorenc <[email protected]>
@sagikazarmark
Copy link

Most projects are now switching to the form3tech fork

Actually, most projects seem to prefer this package now: https://github.com/square/go-jose

But even that has its own issues: square/go-jose#342 (comment)

A third alternative I've seen mentioned is this one: https://github.com/lestrrat-go/jwx

@ptabor ptabor linked an issue Jan 15, 2021 that may be closed by this pull request
@ptabor
Copy link
Contributor

ptabor commented Jan 15, 2021

Thank you for the contribution.
I checked https://github.com/kubernetes/kubernetes/blob/master/go.mod for dependency k8s use and it has both:
https://github.com/square/go-jose & form3tech-oss/jwt-go.

Digging deeper square/go-jose wins in popularity:

cd kubernetes
git grep square/go-jose | grep "[.]go" | grep -v vendor | nl

     1	pkg/controller/serviceaccount/tokens_controller_test.go:	"gopkg.in/square/go-jose.v2/jwt"
     2	pkg/serviceaccount/claims.go:	"gopkg.in/square/go-jose.v2/jwt"
     3	pkg/serviceaccount/claims_test.go:	"gopkg.in/square/go-jose.v2/jwt"
     4	pkg/serviceaccount/jwt.go:	jose "gopkg.in/square/go-jose.v2"
     5	pkg/serviceaccount/jwt.go:	"gopkg.in/square/go-jose.v2/jwt"
     6	pkg/serviceaccount/jwt.go:// See https://github.com/square/go-jose/issues/169
     7	pkg/serviceaccount/jwt_test.go:	jose "gopkg.in/square/go-jose.v2"
     8	pkg/serviceaccount/legacy.go:	"gopkg.in/square/go-jose.v2/jwt"
     9	pkg/serviceaccount/openidmetadata.go:	jose "gopkg.in/square/go-jose.v2"
    10	pkg/serviceaccount/openidmetadata_test.go:	jose "gopkg.in/square/go-jose.v2"
    11	staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go:	jose "gopkg.in/square/go-jose.v2"
    12	staging/src/k8s.io/cluster-bootstrap/token/jws/jws.go:	jose "gopkg.in/square/go-jose.v2"
    13	test/images/agnhost/openidmetadata/openidmetadata.go:	"gopkg.in/square/go-jose.v2/jwt"
    14	test/integration/auth/svcaccttoken_test.go:	jose "gopkg.in/square/go-jose.v2"
    15	test/integration/auth/svcaccttoken_test.go:	"gopkg.in/square/go-jose.v2/jwt"

vs.

% git grep form3tech | grep "[.]go" 
vendor/github.com/Azure/go-autorest/autorest/adal/token.go:	"github.com/form3tech-oss/jwt-go"

@dlorenc Do you have expertise why one of them might be better than the other ?

@dlorenc
Copy link
Contributor Author

dlorenc commented Jan 16, 2021

I honestly can't tell :( The square one has some issues too as pointed out by @sagikazarmark. It doesn't seem to be maintained either, and has a different API from the one currently in use here.

The form3-tech one is more of a drop-in replacement with just the security patches applied. But again, whether that will be maintained is up the air.

@ptabor
Copy link
Contributor

ptabor commented Jan 16, 2021

That's compelling argument that this one does not require code-changes. Thank you.

@ptabor ptabor merged commit bb71f70 into etcd-io:master Jan 16, 2021
@sagikazarmark
Copy link

I agree this is a good fix for the short term, but I'd consider reviewing the available JWT libraries and replace the current one if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Consider to migrate to a different jwt library
3 participants