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

Change jwt library to golang-jwt/jwt #1946

Merged
merged 2 commits into from Aug 1, 2021
Merged

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Aug 1, 2021

Change to latest golang-jwt/jwt. In reaction to midigate amount of issues that are raised. See #1940

This just changes library imports and nothing more.

@aldas
Copy link
Contributor Author

aldas commented Aug 1, 2021

seems that 3.2.2 does not work with older versions. I'll add build tags for go1.15+ and reasoning is - if you care about security - which you do by wanting this change then you definitely want to use SUPPORTED version of Go (1.15+)

failure with <1.15

# github.com/golang-jwt/jwt
Error: ../pkg/mod/github.com/golang-jwt/jwt@v3.2.2+incompatible/ecdsa.go:135:4: r.FillBytes undefined (type *big.Int has no field or method FillBytes)
Error: ../pkg/mod/github.com/golang-jwt/jwt@v3.2.2+incompatible/ecdsa.go:136:4: s.FillBytes undefined (type *big.Int has no field or method FillBytes)

… github.com/golang-jwt/jwt` due former library being unmaintained and having security

issues.
NOTE: `golang-jwt/jwt` now only supports last 2 Go releases. So 1.15+
For detailed information please read labstack#1940
@aldas
Copy link
Contributor Author

aldas commented Aug 1, 2021

@lammel what do you think of it?

@lammel
Copy link
Contributor

lammel commented Aug 1, 2021

I think it is the best we can do. Thank you very much for doing a PR, I was not sure you like the approach.

Let me review the wording in the evening.

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

Please use the suggested wording for the breaking change, if you like it.
I think this is a required step to move away from the problematic vulnerability checks by automated tools.

@aldas
Copy link
Contributor Author

aldas commented Aug 1, 2021

I do not think this will be last time we need to deal with that library and security notices. New maintainers for golang-jwt/jwt will definitively ramp up their efforts and this will trickle down here eventually. at least patch releases will be easier to handle.

@modood
Copy link

modood commented Aug 4, 2021

@aldas @lammel
I inadvertently upgraded the version of echo from v4.4.0 to v4.5.0, but did not notice that the jwt library has been changed to golang-jwt/jwt. The jwt library used in other places in my program to parse claims is still dgrijalva/jwt-go, which will cause the type assertion to fail:

import (
	"github.com/dgrijalva/jwt-go"
)

	user, ok := c.Get("user").(*jwt.Token)
	if !ok {
		// enter here - -!
	}

Although I can solve this problem by modifying dgrijalva/jwt-go to golang-jwt/jwt, this implicit upgrade caused my program to report an error, which is really unacceptable.

@lammel
Copy link
Contributor

lammel commented Aug 4, 2021

Although I can solve this problem by modifying dgrijalva/jwt-go to golang-jwt/jwt, this implicit upgrade caused my program to report an error, which is really unacceptable.

@modood
We discussed the situation multiple times and took the decision very serious.

You are right the situation caused by the maintenance state of dgrijalva/jwt-go (which is practically unmaintained) is unacceptable. The result is numerous "security" bugs reported against echo (and other projects) which picked up in the last months.
This is the reason why we did not want to wait for v5 to be released and needed to accept the breaking change in a minor release, as it is taking away valuable developer time.

Either continue using v4.4.0 or do yourself a favour and step away from the vulnerable old jwt implementation by a simple string replace. Both will work fine for you.

@aldas
Copy link
Contributor Author

aldas commented Aug 4, 2021

Hi @modood I am sorry for your inconvience. We have been holding this change back since 2021 winter/early spring (I think) and hoped we can do the change in v5. My personal opinion about subject can be found in posts #1940 (We are not happy either). Now feedback that we get from community is issues raised agains that CVE etc and now when Github released (1-2 weeks ago) their Go security notices this problem started to get out of hands.

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.

None yet

3 participants