Update JWT version (#391)

* Update JWT version * Tests * Migrate token validation with leeway to jwt v5 * Update common --------- Co-authored-by: Rajiv Senthilnathan <[email protected]>
codeready-toolchain · Jan 16, 2024 · 81de976 · 81de976
1 parent 38eb3fc
commit 81de976
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 60 deletions.
diff --git a/go.mod b/go.mod
@@ -5,7 +5,7 @@ go 1.20
 require (
 	github.com/aws/aws-sdk-go v1.44.100
 	github.com/codeready-toolchain/api v0.0.0-20240103194050-d5c7803671c1
-	github.com/codeready-toolchain/toolchain-common v0.0.0-20240103195541-637ca99d891b
+	github.com/codeready-toolchain/toolchain-common v0.0.0-20240115235114-36095a5f2acc
 	github.com/go-logr/logr v1.2.3
 	github.com/gofrs/uuid v4.2.0+incompatible
 	github.com/pkg/errors v0.9.1
@@ -24,7 +24,7 @@ require (
 	github.com/gin-contrib/gzip v0.0.6
 	github.com/gin-contrib/static v0.0.1
 	github.com/gin-gonic/gin v1.9.1
-	github.com/golang-jwt/jwt v3.2.2+incompatible
+	github.com/golang-jwt/jwt/v5 v5.2.0
 	github.com/kevinburke/twilio-go v0.0.0-20220922200631-8f3f155dfe1f
 	github.com/labstack/echo/v4 v4.10.2
 	github.com/labstack/gommon v0.4.0
@@ -55,6 +55,7 @@ require (
 	github.com/emicklei/go-restful/v3 v3.8.0 // indirect
 	github.com/evanphx/json-patch/v5 v5.6.0 // indirect
 	github.com/gabriel-vasile/mimetype v1.4.3 // indirect
+	github.com/golang-jwt/jwt v3.2.2+incompatible // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/google/go-github/v52 v52.0.0 // indirect
 	github.com/google/go-querystring v1.1.0 // indirect

diff --git a/go.sum b/go.sum
@@ -117,8 +117,8 @@ github.com/cncf/xds/go v0.0.0-20211001041855-01bcc9b48dfe/go.mod h1:eXthEFrGJvWH
 github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/codeready-toolchain/api v0.0.0-20240103194050-d5c7803671c1 h1:R+5BmQrz9hBfj6QFL+ojExD6CiZ5EuuVUbeb3pqxdds=
 github.com/codeready-toolchain/api v0.0.0-20240103194050-d5c7803671c1/go.mod h1:FO7kgXH1x1LqkF327D5a36u0WIrwjVCbeijPkzgwaZc=
-github.com/codeready-toolchain/toolchain-common v0.0.0-20240103195541-637ca99d891b h1:Is479bj8oC3/p/MDhtlBbadAh711YnKd6aBod9ef//k=
-github.com/codeready-toolchain/toolchain-common v0.0.0-20240103195541-637ca99d891b/go.mod h1:/5KdPDrtN6ksuReL0/IuqV9kLsumkVbt/EjZDBmb+Ig=
+github.com/codeready-toolchain/toolchain-common v0.0.0-20240115235114-36095a5f2acc h1:tiRzD0Xasn/o2aBfZttt/O9Dw5S/13dY3dtqJ5wvu5I=
+github.com/codeready-toolchain/toolchain-common v0.0.0-20240115235114-36095a5f2acc/go.mod h1:9YS35niqlaKOpGTKX9RoXhonHopTkwH9W4C9Bcg7ZjM=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
@@ -202,6 +202,8 @@ github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
 github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
 github.com/golang-jwt/jwt v3.2.2+incompatible h1:IfV12K8xAKAnZqdXVzCZ+TOjboZ2keLg81eXfW3O+oY=
 github.com/golang-jwt/jwt v3.2.2+incompatible/go.mod h1:8pz2t5EyA70fFQQSrl6XZXzqecmYZeUEB8OUGHkxJ+I=
+github.com/golang-jwt/jwt/v5 v5.2.0 h1:d/ix8ftRUorsN+5eMIlF4T6J8CAt9rch3My2winC1Jw=
+github.com/golang-jwt/jwt/v5 v5.2.0/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
 github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
 github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=

diff --git a/pkg/auth/keymanager_test.go b/pkg/auth/keymanager_test.go
@@ -17,7 +17,7 @@ import (
 	testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config"
 
 	"github.com/gofrs/uuid"
-	"github.com/golang-jwt/jwt"
+	"github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -236,7 +236,7 @@ func (s *TestKeyManagerSuite) TestKeyFetching() {
 					require.NotEqual(s.T(), tt.kid, kid)
 					return keyManager.Key(tt.kid)
 				})
-				assert.EqualError(s.T(), err, "crypto/rsa: verification error")
+				assert.EqualError(s.T(), err, "token signature is invalid: crypto/rsa: verification error")
 			})
 		}
 	})

diff --git a/pkg/auth/tokenparser.go b/pkg/auth/tokenparser.go
@@ -3,20 +3,12 @@ package auth
 import (
 	"errors"
 	"fmt"
-	"strconv"
 	"time"
 
-	"github.com/codeready-toolchain/registration-service/pkg/log"
-	"github.com/golang-jwt/jwt"
+	"github.com/golang-jwt/jwt/v5"
 )
 
-/****************************************************
-
-  This section is a temporary fix until formal leeway support is available in the next jwt-go release
-
- *****************************************************/
-
-const leeway = 5000
+const leeway = 5 * time.Second
 
 // TokenClaims represents access token claims
 type TokenClaims struct {
@@ -30,21 +22,7 @@ type TokenClaims struct {
 	OriginalSub       string `json:"original_sub"`
 	UserID            string `json:"user_id"`
 	AccountID         string `json:"account_id"`
-	jwt.StandardClaims
-}
-
-// Valid checks whether the token claims are valid
-func (c *TokenClaims) Valid() error {
-	c.StandardClaims.IssuedAt -= leeway
-	now := time.Now().Unix()
-	err := c.StandardClaims.Valid()
-	if err != nil {
-		log.Error(nil, err, "Token validation failed")
-		log.Infof(nil, "Current time: %s", strconv.FormatInt(now, 10))
-		log.Infof(nil, "Token IssuedAt time: %s", strconv.FormatInt(c.StandardClaims.IssuedAt, 10))
-	}
-	c.StandardClaims.IssuedAt += leeway
-	return err
+	jwt.RegisteredClaims
 }
 
 // TokenParser represents a parser for JWT tokens.
@@ -64,27 +42,32 @@ func NewTokenParser(keyManager *KeyManager) (*TokenParser, error) {
 
 // FromString parses a JWT, validates the signature and returns the claims struct.
 func (tp *TokenParser) FromString(jwtEncoded string) (*TokenClaims, error) {
-	token, err := jwt.ParseWithClaims(jwtEncoded, &TokenClaims{}, func(token *jwt.Token) (interface{}, error) {
-		// validate the alg is what we expect
-		if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok {
-			return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
-		}
+	token, err := jwt.ParseWithClaims(
+		jwtEncoded,
+		&TokenClaims{},
+		func(token *jwt.Token) (interface{}, error) {
+			// validate the alg is what we expect
+			if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok {
+				return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
+			}
 
-		kid := token.Header["kid"]
-		if kid == nil {
-			return nil, errors.New("no key id given in the token")
-		}
-		kidStr, ok := kid.(string)
-		if !ok {
-			return nil, errors.New("given key id has unknown type")
-		}
-		// get the public key for kid from keyManager
-		publicKey, err := tp.keyManager.Key(kidStr)
-		if err != nil {
-			return nil, err
-		}
-		return publicKey, nil
-	})
+			kid := token.Header["kid"]
+			if kid == nil {
+				return nil, errors.New("no key id given in the token")
+			}
+			kidStr, ok := kid.(string)
+			if !ok {
+				return nil, errors.New("given key id has unknown type")
+			}
+			// get the public key for kid from keyManager
+			publicKey, err := tp.keyManager.Key(kidStr)
+			if err != nil {
+				return nil, err
+			}
+			return publicKey, nil
+		},
+		jwt.WithLeeway(leeway),
+	)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/auth/tokenparser_test.go b/pkg/auth/tokenparser_test.go
@@ -5,7 +5,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/golang-jwt/jwt"
+	"github.com/golang-jwt/jwt/v5"
 
 	"github.com/codeready-toolchain/registration-service/pkg/auth"
 	"github.com/codeready-toolchain/registration-service/pkg/configuration"
@@ -134,7 +134,7 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		// validate token
 		_, err = tokenParser.FromString(jwt0string)
 		require.Error(s.T(), err)
-		require.EqualError(s.T(), err, "unexpected signing method: HS256")
+		require.EqualError(s.T(), err, "token is unverifiable: error while executing keyfunc: unexpected signing method: HS256")
 	})
 
 	s.Run("token signed by unknown key", func() {
@@ -156,7 +156,7 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		// validate token
 		_, err = tokenParser.FromString(jwtX)
 		require.Error(s.T(), err)
-		require.EqualError(s.T(), err, "unknown kid")
+		require.EqualError(s.T(), err, "token is unverifiable: error while executing keyfunc: unknown kid")
 	})
 
 	s.Run("no KID header in token", func() {
@@ -175,7 +175,7 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		// validate token
 		_, err = tokenParser.FromString(jwt0string)
 		require.Error(s.T(), err)
-		require.EqualError(s.T(), err, "no key id given in the token")
+		require.EqualError(s.T(), err, "token is unverifiable: error while executing keyfunc: no key id given in the token")
 	})
 
 	s.Run("missing claim: preferred_username", func() {
@@ -252,7 +252,7 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		// validate token
 		_, err = tokenParser.FromString(jwt0string)
 		require.Error(s.T(), err)
-		require.True(s.T(), strings.HasPrefix(err.Error(), "token is expired by "))
+		require.EqualError(s.T(), err, "token has invalid claims: token is expired")
 	})
 
 	s.Run("signature is good but token not valid yet", func() {
@@ -273,7 +273,27 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		// validate token
 		_, err = tokenParser.FromString(jwt0string)
 		require.Error(s.T(), err)
-		require.EqualError(s.T(), err, "token is not valid yet")
+		require.EqualError(s.T(), err, "token has invalid claims: token is not valid yet")
+	})
+
+	s.Run("signature is good and token expiration is within leeway", func() {
+		username0 := uuid.Must(uuid.NewV4()).String()
+		identity0 := &authsupport.Identity{
+			ID:       uuid.Must(uuid.NewV4()),
+			Username: username0,
+		}
+		email0 := identity0.Username + "@email.tld"
+		expTime := time.Now().Add(-1 * time.Second)
+		expClaim := authsupport.WithExpClaim(expTime)
+		// generate non-serialized token
+		jwt0 := tokengenerator.GenerateToken(*identity0, kid0, authsupport.WithEmailClaim(email0), expClaim)
+
+		// serialize
+		jwt0string, err := tokengenerator.SignToken(jwt0, kid0)
+		require.NoError(s.T(), err)
+		// validate token
+		_, err = tokenParser.FromString(jwt0string)
+		require.NoError(s.T(), err)
 	})
 
 	s.Run("token signed by known key but the signature is invalid", func() {
@@ -296,7 +316,7 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		// validate token
 		_, err = tokenParser.FromString(jwt0string)
 		require.Error(s.T(), err)
-		require.EqualError(s.T(), err, "crypto/rsa: verification error")
+		require.EqualError(s.T(), err, "token signature is invalid: crypto/rsa: verification error")
 	})
 
 	s.Run("parse valid token with original_sub claim", func() {
@@ -321,4 +341,39 @@ func (s *TestTokenParserSuite) TestTokenParser() {
 		require.Equal(s.T(), email0, claims.Email)
 		require.Equal(s.T(), "OriginalSubValue:1234-ABCD", claims.OriginalSub)
 	})
+
+	s.Run("parse valid token with aud claim", func() {
+		username0 := uuid.Must(uuid.NewV4()).String()
+		identity0 := &authsupport.Identity{
+			ID:       uuid.Must(uuid.NewV4()),
+			Username: username0,
+		}
+		email0 := identity0.Username + "@email.tld"
+
+		tests := map[string]struct {
+			aud []string
+		}{
+			"single string": {
+				aud: []string{"aud-claim-1"},
+			},
+			"multiple strings": {
+				aud: []string{"aud-claim-1", "aud-claim-2"},
+			},
+		}
+
+		for k, tc := range tests {
+			s.T().Run(k, func(t *testing.T) {
+				// generate non-serialized token
+				jwt0 := tokengenerator.GenerateToken(*identity0, kid0, authsupport.WithEmailClaim(email0), authsupport.WithAudClaim(tc.aud))
+
+				// serialize
+				jwt0string, err := tokengenerator.SignToken(jwt0, kid0)
+				require.NoError(s.T(), err)
+				// validate token
+				parsed, err := tokenParser.FromString(jwt0string)
+				require.NoError(s.T(), err)
+				require.Equal(s.T(), jwt.ClaimStrings(tc.aud), parsed.Audience)
+			})
+		}
+	})
 }
diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go
@@ -162,7 +162,7 @@ func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) {
 			require.NotNil(s.T(), resp)
 			defer resp.Body.Close()
 			assert.Equal(s.T(), http.StatusUnauthorized, resp.StatusCode)
-			s.assertResponseBody(resp, "invalid bearer token: unable to extract userID from token: token contains an invalid number of segments")
+			s.assertResponseBody(resp, "invalid bearer token: unable to extract userID from token: token is malformed: token contains an invalid number of segments")
 		})
 
 		s.Run("unauthorized if can't extract userID from a valid token", func() {
@@ -247,7 +247,7 @@ func (s *TestProxySuite) checkWebsocketsError() {
 			},
 			"not a jwt token": {
 				ProtocolHeaders: []string{"base64url.bearer.authorization.k8s.io.dG9rZW4,dummy"},
-				ExpectedError:   "invalid bearer token: unable to extract userID from token: token contains an invalid number of segments",
+				ExpectedError:   "invalid bearer token: unable to extract userID from token: token is malformed: token contains an invalid number of segments",
 			},
 			"invalid token is not base64 encoded": {
 				ProtocolHeaders: []string{"base64url.bearer.authorization.k8s.io.token,dummy"},