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

use github.com/golang-jwt/jwt #1916

Closed
wants to merge 1 commit into from
Closed

use github.com/golang-jwt/jwt #1916

wants to merge 1 commit into from

Conversation

Kielek
Copy link

@Kielek Kielek commented Jul 12, 2021

https://github.com/golang-jwt/jwt/releases/tag/v3.2.1
from 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 (#12). This fixes CVE-2020-26160

@aldas
Copy link
Contributor

aldas commented Jul 12, 2021

Because Echo has direct import from dgrijalva/jwt-go and we expose that our of our API with public fields/types we can not just replace that library. It would not be backwards compatible. This will change in V5

Some remarks - Echo middleware is not using aud field validation and therefore is not directly affected. VerifyAudience is optional method that dgrijalva/jwt-go does not call during ordinary token validation. Unless you have written custom check for it and even then you are affected only when you have made that check optional

CVE occurs for mapClaims type, method VerifyAudience when second argument req is set to false

if ok := claims.VerifyAudience("https://mysite.com", false)  // false makes check optional
// you code is only affected when it is  set to `false`

Please see what that vuln actually is:

#1663 (comment)
#1663 (comment)

Workarounds:

  1. Use replace directive in go.mod in your project
    replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt v3.2.1

  2. use JWTConfig.ParseTokenFunc to implement token parsing with library of your choosing.
    See https://echo.labstack.com/middleware/jwt/ for example.

import (
  "github.com/golang-jwt/jwt"
)

// ...
// ...

signingKey := []byte("secret")

config := middleware.JWTConfig{
  TokenLookup: "query:token",
  ParseTokenFunc: func(auth string, c echo.Context) (interface{}, error) {
    keyFunc := func(t *jwt.Token) (interface{}, error) {
      if t.Method.Alg() != "HS256" {
        return nil, fmt.Errorf("unexpected jwt signing method=%v", t.Header["alg"])
      }
      return signingKey, nil
    }

    // claims are of type `jwt.MapClaims` when token is created with `jwt.Parse`
    token, err := jwt.Parse(auth, keyFunc)
    if err != nil {
      return nil, err
    }
    if !token.Valid {
      return nil, errors.New("invalid token")
    }
    return token, nil
  },
}

e.Use(middleware.JWTWithConfig(config))

@cezary-butler
Copy link

@aldas you're right. This will be a breaking change.
Do you have a broader plan how to implement that?
Is there anyone working on this feature for v5?

I might take care of that if there is such need.

@aldas
Copy link
Contributor

aldas commented Jul 13, 2021

Current plan is to remove fields/methods related to github.com/dgrijalva/jwt-go. With ParseTokenFunc these are unnecessary. Library user has to provide ParseTokenFunc implementation by him/herself. This makes user responsible for token creation. It if is very unpopular then there will be function with implementation but this will not be set automatically as default. This will be still user that sets it (conscious decision).
Echo will provide example for that method under tests with github.com/golang-jwt/jwt.

I have done it already in separate branch but time will tell what solution actually lands in v5

@Kielek
Copy link
Author

Kielek commented Jul 14, 2021

@aldas, I know, that it echo is not affected, but it can be displayed in various security scans as vulnerable.
I am looking forward for the v5 release to mitigate this false positives scans.

@aldas aldas mentioned this pull request Jul 15, 2021
3 tasks
@lammel
Copy link
Contributor

lammel commented Jul 16, 2021

We might think about accepting the breaking change in a minor release due to the indirect vulnerability (we show up as vulnerable because we are using that version) and move to https://github.com/golang-jwt/jwt/releases/tag/v3.2.1

Although developers may need to adjust their code to golang-jwt/jwt this can be considered a good thing, as they are vulnerable too (or show up due to the dependency).

So with some documentation we could do a v4.5.0 with a security notice. Not 100% sure though...

@178inaba
Copy link
Contributor

@aldas
Hello.
I tried replace.

  1. Use replace directive in go.mod in your project
    replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt v3.2.1

(I also saw golang-jwt/jwt/MIGRATION_GUIDE.md.)
However, I get the following error.

$ go mod edit -replace github.com/dgrijalva/jwt-go=github.com/golang-jwt/[email protected]+incompatible
$ go mod tidy
go: finding module for package github.com/golang-jwt/jwt/test
go: finding module for package github.com/golang-jwt/jwt
go: finding module for package github.com/golang-jwt/jwt/request
go: found github.com/golang-jwt/jwt in github.com/golang-jwt/jwt v3.2.1+incompatible
go: found github.com/golang-jwt/jwt/request in github.com/golang-jwt/jwt v3.2.1+incompatible
go: found github.com/golang-jwt/jwt/test in github.com/golang-jwt/jwt v3.2.1+incompatible
go: github.com/golang-jwt/[email protected]+incompatible used for two different module paths (github.com/dgrijalva/jwt-go and github.com/golang-jwt/jwt)

Do you know how to resolve this error?

@Abdulsametileri
Copy link

Any updates?

@aldas
Copy link
Contributor

aldas commented Aug 2, 2021

done in #1946

@aldas aldas closed this Aug 2, 2021
@Kielek Kielek deleted the use-golang-jwt-jwt branch September 2, 2021 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants