[Token/JWT] Update to golang-jwt v5.2.0 (#19802)

* feat: update to golang-jwt v5.2.0 Signed-off-by: Antoine Jouve <[email protected]> * fix: module issues and robot claims Signed-off-by: Antoine Jouve <[email protected]> * fix: add missing time import Signed-off-by: Antoine Jouve <[email protected]> * feat: set jwt validation leeway to 60s Signed-off-by: Antoine Jouve <[email protected]> * fix: update leeways that were still set to 10s Signed-off-by: Antoine Jouve <[email protected]> * feat: update go.sum Signed-off-by: Antoine Jouve <[email protected]> * feat: add two leeway related test cases Signed-off-by: Antoine Jouve <[email protected]> * fix: correct jwt audience validation Signed-off-by: Antoine Jouve <[email protected]> * fix: gofmt v2_token.go Signed-off-by: Antoine Jouve <[email protected]> * feat: take into account review comments Signed-off-by: Antoine Jouve <[email protected]> * feat: use a common constant to store JWT leeway Signed-off-by: Antoine Jouve <[email protected]> --------- Signed-off-by: Antoine Jouve <[email protected]> Signed-off-by: Antoine Jouve <[email protected]> Co-authored-by: MinerYang <[email protected]>
goharbor · Feb 23, 2024 · 73c2884 · 73c2884
1 parent bca9b14
commit 73c2884
Show file tree

Hide file tree

Showing 13 changed files with 118 additions and 38 deletions.
diff --git a/src/common/const.go b/src/common/const.go
@@ -14,6 +14,8 @@
 
 package common
 
+import "time"
+
 type contextKey string
 
 // const variables
@@ -241,4 +243,7 @@ const (
 	BeegoMaxUploadSizeBytes = "beego_max_upload_size_bytes"
 	// DefaultBeegoMaxUploadSizeBytes sets default max upload size to 128GB
 	DefaultBeegoMaxUploadSizeBytes = 1 << 37
+
+	// Global Leeway used for token validation
+	JwtLeeway = 60 * time.Second
 )
diff --git a/src/core/service/token/authutils.go b/src/core/service/token/authutils.go
@@ -22,7 +22,7 @@ import (
 
 	"github.com/docker/distribution/registry/auth/token"
 	"github.com/docker/libtrust"
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/security"

diff --git a/src/core/service/token/token_test.go b/src/core/service/token/token_test.go
@@ -27,7 +27,7 @@ import (
 	"testing"
 
 	"github.com/docker/distribution/registry/auth/token"
-	jwt "github.com/golang-jwt/jwt/v4"
+	jwt "github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/goharbor/harbor/src/common/rbac"

diff --git a/src/go.mod b/src/go.mod
@@ -30,7 +30,7 @@ require (
 	github.com/go-redis/redis/v8 v8.11.4
 	github.com/gocarina/gocsv v0.0.0-20210516172204-ca9e8a8ddea8
 	github.com/gocraft/work v0.5.1
-	github.com/golang-jwt/jwt/v4 v4.5.0
+	github.com/golang-jwt/jwt/v5 v5.2.0
 	github.com/golang-migrate/migrate/v4 v4.16.2
 	github.com/gomodule/redigo v2.0.0+incompatible
 	github.com/google/uuid v1.3.1
@@ -109,6 +109,7 @@ require (
 	github.com/go-openapi/jsonpointer v0.20.0 // indirect
 	github.com/go-openapi/jsonreference v0.20.2 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/golang-jwt/jwt/v4 v4.4.2 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/go-querystring v1.1.0 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect

diff --git a/src/go.sum b/src/go.sum
@@ -228,8 +228,10 @@ github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69
 github.com/goji/httpauth v0.0.0-20160601135302-2da839ab0f4d/go.mod h1:nnjvkQ9ptGaCkuDUx6wNykzzlUixGxvkme+H/lnzb+A=
 github.com/golang-jwt/jwt/v4 v4.0.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg=
 github.com/golang-jwt/jwt/v4 v4.2.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg=
-github.com/golang-jwt/jwt/v4 v4.5.0 h1:7cYmW1XlMY7h7ii7UhUyChSgS5wUJEnm9uZVTGqOWzg=
-github.com/golang-jwt/jwt/v4 v4.5.0/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=
+github.com/golang-jwt/jwt/v4 v4.4.2 h1:rcc4lwaZgFMCZ5jxF9ABolDcIHdBytAFgqFPbSJQAYs=
+github.com/golang-jwt/jwt/v4 v4.4.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=
+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-migrate/migrate/v4 v4.16.2 h1:8coYbMKUyInrFk1lfGfRovTLAW7PhWp8qQDT2iKfuoA=
 github.com/golang-migrate/migrate/v4 v4.16.2/go.mod h1:pfcJX4nPHaVdc5nmdCikFBWtm+UBpiZjRNNsyBbp0/o=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=

diff --git a/src/pkg/token/claims/robot/robot.go b/src/pkg/token/claims/robot/robot.go
@@ -17,8 +17,9 @@ package robot
 import (
 	"errors"
 
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 
+	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/pkg/permission/types"
 )
 
@@ -45,8 +46,9 @@ func (rc Claim) Valid() error {
 	if rc.Access == nil {
 		return errors.New("the access info cannot be nil")
 	}
-	stdErr := rc.RegisteredClaims.Valid()
-	if stdErr != nil {
+	var v = jwt.NewValidator(jwt.WithLeeway(common.JwtLeeway))
+
+	if stdErr := v.Validate(rc.RegisteredClaims); stdErr != nil {
 		return stdErr
 	}
 	return nil

diff --git a/src/pkg/token/claims/v2/claims.go b/src/pkg/token/claims/v2/claims.go
@@ -15,11 +15,10 @@
 package v2
 
 import (
-	"crypto/subtle"
-	"fmt"
+	"github.com/goharbor/harbor/src/common"
 
 	"github.com/docker/distribution/registry/auth/token"
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 )
 
 func init() {
@@ -39,11 +38,10 @@ type Claims struct {
 
 // Valid checks if the issuer is harbor
 func (c *Claims) Valid() error {
-	if err := c.RegisteredClaims.Valid(); err != nil {
+	var v = jwt.NewValidator(jwt.WithLeeway(common.JwtLeeway), jwt.WithIssuer(Issuer))
+
+	if err := v.Validate(c.RegisteredClaims); err != nil {
 		return err
 	}
-	if subtle.ConstantTimeCompare([]byte(c.Issuer), []byte(Issuer)) == 0 {
-		return fmt.Errorf("invalid token issuer: %s", c.Issuer)
-	}
 	return nil
 }
diff --git a/src/pkg/token/claims/v2/claims_test.go b/src/pkg/token/claims/v2/claims_test.go
@@ -4,7 +4,7 @@ import (
 	"testing"
 
 	"github.com/docker/distribution/registry/auth/token"
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
 )
 

diff --git a/src/pkg/token/option_test.go b/src/pkg/token/option_test.go
@@ -3,7 +3,7 @@ package token
 import (
 	"testing"
 
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
 )
 

diff --git a/src/pkg/token/options.go b/src/pkg/token/options.go
@@ -19,7 +19,7 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 
 	"github.com/goharbor/harbor/src/lib/config"
 	"github.com/goharbor/harbor/src/lib/log"

diff --git a/src/pkg/token/token.go b/src/pkg/token/token.go
@@ -20,8 +20,9 @@ import (
 	"errors"
 	"fmt"
 
-	"github.com/golang-jwt/jwt/v4"
+	"github.com/golang-jwt/jwt/v5"
 
+	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/lib/log"
 )
 
@@ -34,8 +35,8 @@ type Token struct {
 
 // New ...
 func New(opt *Options, claims jwt.Claims) (*Token, error) {
-	err := claims.Valid()
-	if err != nil {
+	var v = jwt.NewValidator(jwt.WithLeeway(common.JwtLeeway))
+	if err := v.Validate(claims); err != nil {
 		return nil, err
 	}
 	return &Token{
@@ -65,10 +66,8 @@ func Parse(opt *Options, rawToken string, claims jwt.Claims) (*Token, error) {
 	if err != nil {
 		return nil, err
 	}
-	token, err := jwt.ParseWithClaims(rawToken, claims, func(token *jwt.Token) (interface{}, error) {
-		if token.Method.Alg() != opt.SignMethod.Alg() {
-			return nil, errors.New("invalid signing method")
-		}
+	var parser = jwt.NewParser(jwt.WithLeeway(common.JwtLeeway), jwt.WithValidMethods([]string{opt.SignMethod.Alg()}))
+	token, err := parser.ParseWithClaims(rawToken, claims, func(token *jwt.Token) (interface{}, error) {
 		switch k := key.(type) {
 		case *rsa.PrivateKey:
 			return &k.PublicKey, nil

diff --git a/src/pkg/token/token_test.go b/src/pkg/token/token_test.go
@@ -5,7 +5,7 @@ import (
 	"testing"
 	"time"
 
-	jwt "github.com/golang-jwt/jwt/v4"
+	jwt "github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/goharbor/harbor/src/lib/config"
@@ -92,6 +92,42 @@ func TestRaw(t *testing.T) {
 	assert.NotNil(t, rawTk)
 }
 
+func TestNewWithClockSkew(t *testing.T) {
+	rbacPolicy := &types.Policy{
+		Resource: "/project/library/repository",
+		Action:   "pull",
+	}
+	var policies []*types.Policy
+	policies = append(policies, rbacPolicy)
+
+	tokenID := int64(123)
+	projectID := int64(321)
+
+	expiresAt := time.Now().UTC().Add(-50 * time.Second)
+	robot := robot_claim.Claim{
+		TokenID:   tokenID,
+		ProjectID: projectID,
+		Access:    policies,
+		RegisteredClaims: jwt.RegisteredClaims{
+			ExpiresAt: jwt.NewNumericDate(expiresAt),
+		},
+	}
+	defaultOpt := DefaultTokenOptions()
+	if defaultOpt == nil {
+		assert.NotNil(t, defaultOpt)
+		return
+	}
+	token, err := New(defaultOpt, robot)
+	if err != nil {
+		assert.Nil(t, err)
+		return
+	}
+
+	rawTk, err := token.Raw()
+	assert.Nil(t, err)
+	assert.NotNil(t, rawTk)
+}
+
 func TestParseWithClaims(t *testing.T) {
 	rawTk := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJJRCI6MTIzLCJQcm9qZWN0SUQiOjAsIkFjY2VzcyI6W3siUmVzb3VyY2UiOiIvcHJvamVjdC9saWJyYXkvcmVwb3NpdG9yeSIsIkFjdGlvbiI6InB1bGwiLCJFZmZlY3QiOiIifV0sIlN0YW5kYXJkQ2xhaW1zIjp7ImV4cCI6MTU0ODE0MDIyOSwiaXNzIjoiaGFyYm9yLXRva2VuLWlzc3VlciJ9fQ.Jc3qSKN4SJVUzAvBvemVpRcSOZaHlu0Avqms04qzPm4ru9-r9IRIl3mnSkI6m9XkzLUeJ7Kiwyw63ghngnVKw_PupeclOGC6s3TK5Cfmo4h-lflecXjZWwyy-dtH_e7Us_ItS-R3nXDJtzSLEpsGHCcAj-1X2s93RB2qD8LNSylvYeDezVkTzqRzzfawPJheKKh9JTrz-3eUxCwQard9-xjlwvfUYULoHTn9npNAUq4-jqhipW4uE8HL-ym33AGF57la8U0RO11hmDM5K8-PiYknbqJ_oONeS3HBNym2pEFeGjtTv2co213wl4T5lemlg4SGolMBuJ03L7_beVZ0o-MKTkKDqDwJalb6_PM-7u3RbxC9IzJMiwZKIPnD3FvV10iPxUUQHaH8Jz5UZ2pFIhi_8BNnlBfT0JOPFVYATtLjHMczZelj2YvAeR1UHBzq3E0jPpjjwlqIFgaHCaN_KMwEvadTo_Fi2sEH4pNGP7M3yehU_72oLJQgF4paJarsmEoij6ZtPs6xekBz1fccVitq_8WNIz9aeCUdkUBRwI5QKw1RdW4ua-w74ld5MZStWJA8veyoLkEb_Q9eq2oAj5KWFjJbW5-ltiIfM8gxKflsrkWAidYGcEIYcuXr7UdqEKXxtPiWM0xb3B91ovYvO5402bn3f9-UGtlcestxNHA"
 	rClaims := &robot_claim.Claim{}
@@ -104,3 +140,47 @@ func TestParseWithClaims(t *testing.T) {
 	assert.Equal(t, int64(0), rClaims.ProjectID)
 	assert.Equal(t, "/project/libray/repository", rClaims.Access[0].Resource.String())
 }
+
+func TestParseWithClaimsWithClockSkew(t *testing.T) {
+	rbacPolicy := &types.Policy{
+		Resource: "/project/library/repository",
+		Action:   "push",
+	}
+	var policies []*types.Policy
+	policies = append(policies, rbacPolicy)
+
+	tokenID := int64(123)
+	projectID := int64(321)
+
+	now := time.Now().UTC()
+	expiresAt := jwt.NewNumericDate(now.Add(time.Duration(10) * 24 * time.Hour))
+	notBefore := jwt.NewNumericDate(now.Add(50 * time.Second))
+	issuedAt := jwt.NewNumericDate(now.Add(50 * time.Second))
+	robot := robot_claim.Claim{
+		TokenID:   tokenID,
+		ProjectID: projectID,
+		Access:    policies,
+		RegisteredClaims: jwt.RegisteredClaims{
+			ExpiresAt: expiresAt,
+			NotBefore: notBefore,
+			IssuedAt:  issuedAt,
+		},
+	}
+	defaultOpt := DefaultTokenOptions()
+	if defaultOpt == nil {
+		assert.NotNil(t, defaultOpt)
+		return
+	}
+	token, err := New(defaultOpt, robot)
+	if err != nil {
+		assert.Nil(t, err)
+		return
+	}
+	rawTk, err := token.Raw()
+	assert.Nil(t, err)
+	rClaims := &robot_claim.Claim{}
+	token, err = Parse(defaultOpt, rawTk, rClaims)
+	assert.Nil(t, err)
+	assert.Equal(t, token.Token.Claims.(*robot_claim.Claim).Access[0].Resource, types.Resource("/project/library/repository"))
+	assert.Equal(t, token.Token.Claims.(*robot_claim.Claim).Access[0].Action, types.Action("push"))
+}
diff --git a/src/server/middleware/security/v2_token.go b/src/server/middleware/security/v2_token.go
@@ -15,12 +15,14 @@
 package security
 
 import (
-	"fmt"
 	"net/http"
 	"strings"
 
+	"github.com/golang-jwt/jwt/v5"
+
 	registry_token "github.com/docker/distribution/registry/auth/token"
 
+	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/common/security"
 	"github.com/goharbor/harbor/src/common/security/v2token"
 	svc_token "github.com/goharbor/harbor/src/core/service/token"
@@ -34,16 +36,6 @@ type v2TokenClaims struct {
 	Access []*registry_token.ResourceActions `json:"access"`
 }
 
-func (vtc *v2TokenClaims) Valid() error {
-	if err := vtc.Claims.Valid(); err != nil {
-		return err
-	}
-	if !vtc.VerifyAudience(svc_token.Registry, true) {
-		return fmt.Errorf("invalid token audience: %s", vtc.Audience)
-	}
-	return nil
-}
-
 type v2Token struct{}
 
 func (vt *v2Token) Generate(req *http.Request) security.Context {
@@ -67,7 +59,8 @@ func (vt *v2Token) Generate(req *http.Request) security.Context {
 		logger.Warningf("failed to decode bearer token: %v", err)
 		return nil
 	}
-	if err := t.Claims.Valid(); err != nil {
+	var v = jwt.NewValidator(jwt.WithLeeway(common.JwtLeeway), jwt.WithAudience(svc_token.Registry))
+	if err := v.Validate(t.Claims); err != nil {
 		logger.Warningf("failed to decode bearer token: %v", err)
 		return nil
 	}