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

Replace JWT validation library #5592

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/server/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ require (
github.com/fatih/structtag v1.2.0 // indirect
github.com/gogo/googleapis v1.3.2 // indirect
github.com/gogo/status v1.1.0 // indirect
github.com/golang-jwt/jwt/v5 v5.2.0 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.3 // indirect
github.com/golang/snappy v0.0.4 // indirect
Expand Down
2 changes: 2 additions & 0 deletions cmd/server/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
github.com/gogo/status v1.1.0 h1:+eIkrewn5q6b30y+g/BJINVVdi2xH7je5MPJ3ZPK3JA=
github.com/gogo/status v1.1.0/go.mod h1:BFv9nrluPLmrS0EmGVvLaPNmRosr9KapBYd5/hpY1WM=
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/freetype v0.0.0-20170609003504-e2365dfdc4a0/go.mod h1:E/TSTwGwJL78qG/PmXZO1EjYhfJinVAhrmmHX6Z8B9k=
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=
Expand Down
1 change: 1 addition & 0 deletions common/archiver/gcloud/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ require (
github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a // indirect
github.com/gogo/googleapis v1.3.2 // indirect
github.com/gogo/status v1.1.0 // indirect
github.com/golang-jwt/jwt/v5 v5.2.0 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.3 // indirect
github.com/google/s2a-go v0.1.4 // indirect
Expand Down
2 changes: 2 additions & 0 deletions common/archiver/gcloud/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
github.com/gogo/status v1.1.0 h1:+eIkrewn5q6b30y+g/BJINVVdi2xH7je5MPJ3ZPK3JA=
github.com/gogo/status v1.1.0/go.mod h1:BFv9nrluPLmrS0EmGVvLaPNmRosr9KapBYd5/hpY1WM=
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-20200121045136-8c9f03a8e57e/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE=
Expand Down
15 changes: 8 additions & 7 deletions common/authorization/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ package authorization
import (
"testing"

"github.com/cristalhq/jwt/v3"
"github.com/golang-jwt/jwt/v5"
"github.com/stretchr/testify/suite"

"github.com/uber/cadence/common"
Expand Down Expand Up @@ -63,7 +63,7 @@ func cfgOAuth() config.Authorization {
OAuthAuthorizer: config.OAuthAuthorizer{
Enable: true,
JwtCredentials: config.JwtCredentials{
Algorithm: jwt.RS256.String(),
Algorithm: jwt.SigningMethodRS256.Name,
PublicKey: "../../config/credentials/keytest.pub",
},
MaxJwtTTL: 12345,
Expand All @@ -76,17 +76,18 @@ func (s *factorySuite) TestFactoryNoopAuthorizer() {

publicKey, _ := common.LoadRSAPublicKey(cfgOAuthVar.OAuthAuthorizer.JwtCredentials.PublicKey)

verifier, _ := jwt.NewVerifierRS(
jwt.Algorithm(cfgOAuthVar.OAuthAuthorizer.JwtCredentials.Algorithm),
publicKey,
)
var tests = []struct {
cfg config.Authorization
expected Authorizer
err error
}{
{cfgNoop(), &nopAuthority{}, nil},
{cfgOAuthVar, &oauthAuthority{authorizationCfg: cfgOAuthVar.OAuthAuthorizer, log: s.logger, verifier: verifier}, nil},
{cfgOAuthVar, &oauthAuthority{
authorizationCfg: cfgOAuthVar.OAuthAuthorizer,
log: s.logger,
publicKey: publicKey,
parser: jwt.NewParser(jwt.WithValidMethods([]string{cfgOAuthVar.OAuthAuthorizer.JwtCredentials.Algorithm}), jwt.WithIssuedAt()),
}, nil},
}

for _, test := range tests {
Expand Down
77 changes: 47 additions & 30 deletions common/authorization/oauthAuthorizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@ package authorization

import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"
"time"

"github.com/cristalhq/jwt/v3"
"github.com/golang-jwt/jwt/v5"
"go.uber.org/yarpc"

"github.com/uber/cadence/common"
Expand All @@ -38,20 +37,24 @@ import (
"github.com/uber/cadence/common/log/tag"
)

var _ jwt.Claims = (*JWTClaims)(nil)

type oauthAuthority struct {
authorizationCfg config.OAuthAuthorizer
domainCache cache.DomainCache
log log.Logger
verifier jwt.Verifier
parser *jwt.Parser
publicKey interface{}
}

// JWTClaims is a Cadence specific claim with embeded Claims defined https://datatracker.ietf.org/doc/html/rfc7519#section-4.1
type JWTClaims struct {
Sub string
jwt.RegisteredClaims

Name string
Groups string // separated by space
Admin bool
Iat int64
TTL int64
TTL int64 // TODO should be removed. ExpiresAt should be used
}

func (j JWTClaims) GetGroups() []string {
Expand All @@ -66,25 +69,25 @@ func NewOAuthAuthorizer(
log log.Logger,
domainCache cache.DomainCache,
) (Authorizer, error) {
publicKey, err := common.LoadRSAPublicKey(authorizationCfg.JwtCredentials.PublicKey)

key, err := common.LoadRSAPublicKey(authorizationCfg.JwtCredentials.PublicKey)
if err != nil {
return nil, fmt.Errorf("loading RSA public key: %w", err)
}

verifier, err := jwt.NewVerifierRS(
jwt.Algorithm(authorizationCfg.JwtCredentials.Algorithm),
publicKey,
)

if err != nil {
return nil, fmt.Errorf("creating JWT verifier: %w", err)
if authorizationCfg.JwtCredentials.Algorithm != jwt.SigningMethodRS256.Name {
return nil, fmt.Errorf("algorithm %q is not supported", authorizationCfg.JwtCredentials.Algorithm)
}

return &oauthAuthority{
authorizationCfg: authorizationCfg,
domainCache: domainCache,
log: log,
verifier: verifier,
parser: jwt.NewParser(
jwt.WithValidMethods([]string{authorizationCfg.JwtCredentials.Algorithm}),
jwt.WithIssuedAt(),
),
publicKey: key,
}, nil
}

Expand All @@ -101,49 +104,63 @@ func (a *oauthAuthority) Authorize(
return Result{Decision: DecisionDeny}, nil
}

claims, err := a.parseToken(token, a.verifier)
var claims JWTClaims

_, err := a.parser.ParseWithClaims(token, &claims, a.keyFunc)

if err != nil {
a.log.Debug("request is not authorized", tag.Error(err))
return Result{Decision: DecisionDeny}, nil
}

if err := a.validateTTL(claims); err != nil {
if err := a.validateTTL(&claims); err != nil {
a.log.Debug("request is not authorized", tag.Error(err))
return Result{Decision: DecisionDeny}, nil
}

if claims.Admin {
return Result{Decision: DecisionAllow}, nil
}

domain, err := a.domainCache.GetDomain(attributes.DomainName)
if err != nil {
return Result{Decision: DecisionDeny}, err
}

if err := validatePermission(claims, attributes, domain.GetInfo().Data); err != nil {
if err := validatePermission(&claims, attributes, domain.GetInfo().Data); err != nil {
a.log.Debug("request is not authorized", tag.Error(err))
return Result{Decision: DecisionDeny}, nil
}

return Result{Decision: DecisionAllow}, nil
}

func (a *oauthAuthority) parseToken(tokenStr string, verifier jwt.Verifier) (*JWTClaims, error) {
token, err := jwt.ParseAndVerifyString(tokenStr, verifier)
if err != nil {
return nil, fmt.Errorf("parse token: %w", err)
}
var claims JWTClaims
_ = json.Unmarshal(token.RawClaims(), &claims)
return &claims, nil
// keyFunc returns correct key to check signature
func (a *oauthAuthority) keyFunc(token *jwt.Token) (interface{}, error) {
// only local public key is supported currently
return a.publicKey, nil
}

func (a *oauthAuthority) validateTTL(claims *JWTClaims) error {
if claims.TTL > a.authorizationCfg.MaxJwtTTL {
return fmt.Errorf("token TTL: %d is larger than MaxTTL allowed: %d", claims.TTL, a.authorizationCfg.MaxJwtTTL)
// Fill ExpiresAt when TTL is passed
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little surprised this is necessary, it's not a problem, but I wouldj have throught that the JWT lib would do this natively

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our clients (java and go) are not setting ExpiresAt claim. Instead, they are providing TTL claim. This is to support backwards compatibility. Later on, clients needs to be updated to fill in ExpiresAt and we can delegate this check to JWT lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After re-reading your comment, I realized that it was not about TTL check, but for IssuedAt check. And yes, you are right, this is not needed as library will do this validation. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, sorry, yeah, i wasn't being super clear and to be honest, i get confused between the different values. I'll don't think it's a blocker, so stamping. Thank you for looking into it.

if claims.TTL > 0 {
claims.ExpiresAt = jwt.NewNumericDate(claims.IssuedAt.Time.Add(time.Second * time.Duration(claims.TTL)))
}
if claims.Iat+claims.TTL < time.Now().Unix() {
return errors.New("JWT has expired")

exp, err := claims.GetExpirationTime()

if err != nil || exp == nil {
return errors.New("ExpiresAt is not set")
}

timeLeft := exp.Unix() - time.Now().Unix()
Copy link
Contributor

Choose a reason for hiding this comment

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

my comment wasn't great, I actually meant this: I thought this would be something that the library would do natively

if timeLeft < 0 {
return errors.New("token is expired")
}

if timeLeft > a.authorizationCfg.MaxJwtTTL {
return fmt.Errorf("token TTL: %d is larger than MaxTTL allowed: %d", timeLeft, a.authorizationCfg.MaxJwtTTL)
}

return nil
}
66 changes: 58 additions & 8 deletions common/authorization/oauthAuthorizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ package authorization

import (
"fmt"
"strings"
"testing"
"time"

"github.com/cristalhq/jwt/v3"
"github.com/golang-jwt/jwt/v5"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/suite"
"go.uber.org/yarpc/api/encoding"
Expand Down Expand Up @@ -64,7 +67,7 @@ func (s *oauthSuite) SetupTest() {
s.cfg = config.OAuthAuthorizer{
Enable: true,
JwtCredentials: config.JwtCredentials{
Algorithm: jwt.RS256.String(),
Algorithm: jwt.SigningMethodRS256.Name,
PublicKey: "../../config/credentials/keytest.pub",
},
MaxJwtTTL: 300000001,
Expand Down Expand Up @@ -167,23 +170,23 @@ func (s *oauthSuite) TestGetDomainError() {
func (s *oauthSuite) TestIncorrectPublicKey() {
s.cfg.JwtCredentials.PublicKey = "incorrectPublicKey"
authorizer, err := NewOAuthAuthorizer(s.cfg, s.logger, s.domainCache)
s.Equal(authorizer, nil)
s.Equal(nil, authorizer)
s.EqualError(err, "loading RSA public key: invalid public key path incorrectPublicKey")
}

func (s *oauthSuite) TestIncorrectAlgorithm() {
s.cfg.JwtCredentials.Algorithm = "SHA256"
authorizer, err := NewOAuthAuthorizer(s.cfg, s.logger, s.domainCache)
s.Equal(authorizer, nil)
s.ErrorContains(err, "jwt: algorithm is not supported")
s.Equal(nil, authorizer)
s.ErrorContains(err, "algorithm \"SHA256\" is not supported")
}

func (s *oauthSuite) TestMaxTTLLargerInToken() {
s.cfg.MaxJwtTTL = 1
authorizer, err := NewOAuthAuthorizer(s.cfg, s.logger, s.domainCache)
s.NoError(err)
s.logger.On("Debug", "request is not authorized", mock.MatchedBy(func(t []tag.Tag) bool {
return fmt.Sprintf("%v", t[0].Field().Interface) == "token TTL: 300000000 is larger than MaxTTL allowed: 1"
return strings.HasPrefix(fmt.Sprintf("%v", t[0].Field().Interface), "token TTL:")
}))
result, _ := authorizer.Authorize(s.ctx, &s.att)
s.Equal(result.Decision, DecisionDeny)
Expand All @@ -199,7 +202,7 @@ func (s *oauthSuite) TestIncorrectToken() {
authorizer, err := NewOAuthAuthorizer(s.cfg, s.logger, s.domainCache)
s.NoError(err)
s.logger.On("Debug", "request is not authorized", mock.MatchedBy(func(t []tag.Tag) bool {
return fmt.Sprintf("%v", t[0].Field().Interface) == "parse token: jwt: token format is not valid"
return fmt.Sprintf("%v", t[0].Field().Interface) == "token is malformed: token contains an invalid number of segments"
}))
result, _ := authorizer.Authorize(ctx, &s.att)
s.Equal(result.Decision, DecisionDeny)
Expand All @@ -217,7 +220,7 @@ func (s *oauthSuite) TestIatExpiredToken() {
authorizer, err := NewOAuthAuthorizer(s.cfg, s.logger, s.domainCache)
s.NoError(err)
s.logger.On("Debug", "request is not authorized", mock.MatchedBy(func(t []tag.Tag) bool {
return fmt.Sprintf("%v", t[0].Field().Interface) == "JWT has expired"
return fmt.Sprintf("%v", t[0].Field().Interface) == "token is expired"
}))
result, _ := authorizer.Authorize(ctx, &s.att)
s.Equal(result.Decision, DecisionDeny)
Expand Down Expand Up @@ -248,3 +251,50 @@ func (s *oauthSuite) TestIncorrectPermission() {
s.NoError(err)
s.Equal(result.Decision, DecisionDeny)
}

func Test_oauthAuthority_validateTTL(t *testing.T) {

tests := []struct {
name string
claims *JWTClaims
ttlConfig int64
wantErr assert.ErrorAssertionFunc
}{
{
name: "Empty claims will fail TTL validation",
claims: &JWTClaims{},
wantErr: assert.Error,
},
{
name: "Claims with IAT and Claim TTL will pass",
claims: &JWTClaims{
TTL: 300,
RegisteredClaims: jwt.RegisteredClaims{
IssuedAt: jwt.NewNumericDate(time.Now()),
},
},
wantErr: assert.NoError,
ttlConfig: 500,
},

{
name: "Claims with IAT but without TTL or ExpiresAT will fail TTL validation",
claims: &JWTClaims{
RegisteredClaims: jwt.RegisteredClaims{
IssuedAt: jwt.NewNumericDate(time.Now().Add(-time.Minute)),
},
},
ttlConfig: 1,
wantErr: assert.Error,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
validator := &oauthAuthority{
authorizationCfg: config.OAuthAuthorizer{MaxJwtTTL: tt.ttlConfig},
}
tt.wantErr(t, validator.validateTTL(tt.claims), fmt.Sprintf("validateTTL(%v)", tt.claims))
})
}
}
8 changes: 4 additions & 4 deletions common/config/authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ package config
import (
"fmt"

"github.com/cristalhq/jwt/v3"
"github.com/golang-jwt/jwt/v5"
)

// Validate validates the persistence config
Expand All @@ -33,8 +33,8 @@ func (a *Authorization) Validate() error {
}

if a.OAuthAuthorizer.Enable {
if oauthError := a.validateOAuth(); oauthError != nil {
return oauthError
if err := a.validateOAuth(); err != nil {
return err
}
}

Expand All @@ -50,7 +50,7 @@ func (a *Authorization) validateOAuth() error {
if oauthConfig.JwtCredentials.PublicKey == "" {
return fmt.Errorf("[OAuthConfig] PublicKey can't be empty")
}
if oauthConfig.JwtCredentials.Algorithm != jwt.RS256.String() {
if oauthConfig.JwtCredentials.Algorithm != jwt.SigningMethodRS256.Name {
return fmt.Errorf("[OAuthConfig] The only supported Algorithm is RS256")
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion common/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ type (

OAuthAuthorizer struct {
Enable bool `yaml:"enable"`
// Credentials to verify/create the JWT
// Credentials to verify/create the JWT using public/private keys
JwtCredentials JwtCredentials `yaml:"jwtCredentials"`
// Max of TTL in the claim
MaxJwtTTL int64 `yaml:"maxJwtTTL"`
Expand Down
Loading