From 4478a6969c00d18c13d7821c949122d968fa90eb Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 7 Dec 2023 16:40:27 +0100 Subject: [PATCH 1/3] fix: enable JWT audience verification Signed-off-by: Miguel Martinez Trivino --- app/controlplane/internal/biz/robotaccount.go | 2 +- app/controlplane/internal/jwt/common.go | 1 - .../internal/jwt/robotaccount/robotaccount.go | 14 +++++++++++--- .../jwt/robotaccount/robotaccount_test.go | 4 ++-- app/controlplane/internal/jwt/user/user.go | 8 ++++++-- .../internal/jwt/user/user_test.go | 4 ++-- app/controlplane/internal/service/auth.go | 2 +- .../usercontext/robotaccount_middleware.go | 6 ++++++ .../internal/usercontext/userorg_middleware.go | 5 +++++ .../usercontext/userorg_middleware_test.go | 18 +++++++++++++++++- 10 files changed, 51 insertions(+), 13 deletions(-) diff --git a/app/controlplane/internal/biz/robotaccount.go b/app/controlplane/internal/biz/robotaccount.go index 81d3a2922..ba9900aef 100644 --- a/app/controlplane/internal/biz/robotaccount.go +++ b/app/controlplane/internal/biz/robotaccount.go @@ -89,7 +89,7 @@ func (uc *RobotAccountUseCase) Create(ctx context.Context, name string, orgID, w return nil, err } - jwt, err := b.GenerateJWT(orgID, workflowID, res.ID.String(), jwt.DefaultAudience) + jwt, err := b.GenerateJWT(orgID, workflowID, res.ID.String()) if err != nil { return nil, err } diff --git a/app/controlplane/internal/jwt/common.go b/app/controlplane/internal/jwt/common.go index be243e5c0..5f162cd0c 100644 --- a/app/controlplane/internal/jwt/common.go +++ b/app/controlplane/internal/jwt/common.go @@ -16,5 +16,4 @@ package jwt const DefaultIssuer = "cp.chainloop" -const DefaultAudience = "client.chainloop" const CASAudience = "artifact-cas.chainloop" diff --git a/app/controlplane/internal/jwt/robotaccount/robotaccount.go b/app/controlplane/internal/jwt/robotaccount/robotaccount.go index 49a82d372..501ca52c0 100644 --- a/app/controlplane/internal/jwt/robotaccount/robotaccount.go +++ b/app/controlplane/internal/jwt/robotaccount/robotaccount.go @@ -23,9 +23,17 @@ import ( var SigningMethod = jwt.SigningMethodHS256 +// This type of JWT is meant to be used by the attestations service +const ( + Audience = "attestations.chainloop" + // Previous audience, deprecated, we keep it to not to break compatibility + DeprecatedAudience = "client.chainloop" +) + type Builder struct { issuer string hmacSecret string + audience string } type NewOpt func(b *Builder) @@ -47,7 +55,7 @@ func WithKeySecret(hmacSecret string) NewOpt { // Currently we use a simple hmac encryption method meant to be continuously rotated // TODO: additional/alternative encryption method, i.e DSE asymetric, see CAS robot account for reference func NewBuilder(opts ...NewOpt) (*Builder, error) { - b := &Builder{} + b := &Builder{audience: Audience} for _, opt := range opts { opt(b) } @@ -64,7 +72,7 @@ func NewBuilder(opts ...NewOpt) (*Builder, error) { } // NOTE: It does not expire, it will get revoked instead -func (ra *Builder) GenerateJWT(orgID, workflowID, keyID, audience string) (string, error) { +func (ra *Builder) GenerateJWT(orgID, workflowID, keyID string) (string, error) { claims := CustomClaims{ orgID, workflowID, @@ -72,7 +80,7 @@ func (ra *Builder) GenerateJWT(orgID, workflowID, keyID, audience string) (strin // Key identifier so we can check it's revocation status ID: keyID, Issuer: ra.issuer, - Audience: jwt.ClaimStrings{audience}, + Audience: jwt.ClaimStrings{ra.audience}, }, } diff --git a/app/controlplane/internal/jwt/robotaccount/robotaccount_test.go b/app/controlplane/internal/jwt/robotaccount/robotaccount_test.go index 2a8d7f923..e66275fa6 100644 --- a/app/controlplane/internal/jwt/robotaccount/robotaccount_test.go +++ b/app/controlplane/internal/jwt/robotaccount/robotaccount_test.go @@ -75,7 +75,7 @@ func TestGenerateJWT(t *testing.T) { ) require.NoError(t, err) - token, err := b.GenerateJWT("org-id", "workflow-id", "key-id", "my-audience") + token, err := b.GenerateJWT("org-id", "workflow-id", "key-id") assert.NoError(t, err) assert.NotEmpty(t, token) @@ -91,6 +91,6 @@ func TestGenerateJWT(t *testing.T) { assert.Equal(t, "workflow-id", claims.WorkflowID) assert.Equal(t, "key-id", claims.ID) assert.Equal(t, "my-issuer", claims.Issuer) - assert.Contains(t, claims.Audience, "my-audience") + assert.Contains(t, claims.Audience, Audience) assert.Nil(t, claims.ExpiresAt) } diff --git a/app/controlplane/internal/jwt/user/user.go b/app/controlplane/internal/jwt/user/user.go index d2bd13f15..316507435 100644 --- a/app/controlplane/internal/jwt/user/user.go +++ b/app/controlplane/internal/jwt/user/user.go @@ -22,10 +22,13 @@ import ( "github.com/golang-jwt/jwt/v4" ) +const Audience = "user-auth.chainloop" + type Builder struct { issuer string hmacSecret string expiration time.Duration + audience string } type NewOpt func(b *Builder) @@ -54,6 +57,7 @@ var SigningMethod = jwt.SigningMethodHS256 func NewBuilder(opts ...NewOpt) (*Builder, error) { b := &Builder{ expiration: defaultExpiration, + audience: Audience, } for _, opt := range opts { @@ -71,12 +75,12 @@ func NewBuilder(opts ...NewOpt) (*Builder, error) { return b, nil } -func (ra *Builder) GenerateJWT(userID, audience string) (string, error) { +func (ra *Builder) GenerateJWT(userID string) (string, error) { claims := CustomClaims{ userID, jwt.RegisteredClaims{ Issuer: ra.issuer, - Audience: jwt.ClaimStrings{audience}, + Audience: jwt.ClaimStrings{ra.audience}, ExpiresAt: jwt.NewNumericDate(time.Now().Add(ra.expiration)), }, } diff --git a/app/controlplane/internal/jwt/user/user_test.go b/app/controlplane/internal/jwt/user/user_test.go index 9b19acc4e..154fe6132 100644 --- a/app/controlplane/internal/jwt/user/user_test.go +++ b/app/controlplane/internal/jwt/user/user_test.go @@ -77,7 +77,7 @@ func TestGenerateJWT(t *testing.T) { ) require.NoError(t, err) - token, err := b.GenerateJWT("user-id", "my-audience") + token, err := b.GenerateJWT("user-id") assert.NoError(t, err) assert.NotEmpty(t, token) @@ -91,6 +91,6 @@ func TestGenerateJWT(t *testing.T) { assert.True(t, tokenInfo.Valid) assert.Equal(t, "user-id", claims.UserID) assert.Equal(t, "my-issuer", claims.Issuer) - assert.Contains(t, claims.Audience, "my-audience") + assert.Contains(t, claims.Audience, Audience) assert.WithinDuration(t, time.Now(), claims.ExpiresAt.Time, 10*time.Second) } diff --git a/app/controlplane/internal/service/auth.go b/app/controlplane/internal/service/auth.go index b4c7f88a9..fd10f6f17 100644 --- a/app/controlplane/internal/service/auth.go +++ b/app/controlplane/internal/service/auth.go @@ -350,7 +350,7 @@ func generateUserJWT(userID, passphrase string, expiration time.Duration) (strin return "", err } - return b.GenerateJWT(userID, jwt.DefaultAudience) + return b.GenerateJWT(userID) } func setOauthCookie(w http.ResponseWriter, name, value string) { diff --git a/app/controlplane/internal/usercontext/robotaccount_middleware.go b/app/controlplane/internal/usercontext/robotaccount_middleware.go index 4576d0d78..b345bd6ed 100644 --- a/app/controlplane/internal/usercontext/robotaccount_middleware.go +++ b/app/controlplane/internal/usercontext/robotaccount_middleware.go @@ -62,6 +62,12 @@ func WithCurrentRobotAccount(robotAccountUseCase *biz.RobotAccountUseCase, logge return nil, errors.New("error mapping the claims") } + // Do not accept tokens that are crafted for a different audience in this system + // NOTE: we allow deprecated audience to not to break compatibility with previously issued robot-accounts + if !claims.VerifyAudience(robotaccount.Audience, true) && !claims.VerifyAudience(robotaccount.DeprecatedAudience, true) { + return nil, errors.New("unexpected token, invalid audience") + } + // Extract account ID robotAccountID := claims.ID if robotAccountID == "" { diff --git a/app/controlplane/internal/usercontext/userorg_middleware.go b/app/controlplane/internal/usercontext/userorg_middleware.go index ddb811abd..51cee7419 100644 --- a/app/controlplane/internal/usercontext/userorg_middleware.go +++ b/app/controlplane/internal/usercontext/userorg_middleware.go @@ -86,6 +86,11 @@ func WithCurrentUserAndOrgMiddleware(userUseCase biz.UserOrgFinder, logger *log. return nil, errors.New("error mapping the claims") } + // Do not accept tokens that are crafted for a different audience in this system + if !customClaims.VerifyAudience(user.Audience, true) { + return nil, errors.New("unexpected token, invalid audience") + } + userID := customClaims.UserID if userID == "" { return nil, errors.New("error retrieving the user information from the auth token") diff --git a/app/controlplane/internal/usercontext/userorg_middleware_test.go b/app/controlplane/internal/usercontext/userorg_middleware_test.go index 9c5f5d56f..92e508fda 100644 --- a/app/controlplane/internal/usercontext/userorg_middleware_test.go +++ b/app/controlplane/internal/usercontext/userorg_middleware_test.go @@ -22,9 +22,11 @@ import ( "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz" bizMocks "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz/mocks" + "github.com/chainloop-dev/chainloop/app/controlplane/internal/jwt/user" userjwtbuilder "github.com/chainloop-dev/chainloop/app/controlplane/internal/jwt/user" "github.com/go-kratos/kratos/v2/log" jwtmiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt" + "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "github.com/stretchr/testify/assert" ) @@ -36,13 +38,21 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) { testCases := []struct { name string loggedIn bool + audience string userExist bool orgExist bool wantErr bool }{ + { + name: "invalid audience", + loggedIn: true, + audience: "another-aud", + wantErr: true, + }, { name: "logged in, user and org exists", loggedIn: true, + audience: user.Audience, userExist: true, orgExist: true, wantErr: false, @@ -50,18 +60,21 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) { { name: "logged in, user does not exist", loggedIn: true, + audience: user.Audience, userExist: false, wantErr: true, }, { name: "logged in, org does not exist", loggedIn: true, + audience: user.Audience, userExist: true, wantErr: true, }, { name: "not logged in", loggedIn: false, + audience: user.Audience, wantErr: true, }, } @@ -76,13 +89,16 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) { if tc.loggedIn { ctx = jwtmiddleware.NewContext(ctx, &userjwtbuilder.CustomClaims{ UserID: wantUser.ID, + RegisteredClaims: jwt.RegisteredClaims{ + Audience: jwt.ClaimStrings{tc.audience}, + }, }) } if tc.userExist { usecase.On("FindByID", ctx, wantUser.ID).Return(wantUser, nil) } else if tc.loggedIn { - usecase.On("FindByID", ctx, wantUser.ID).Return(nil, nil) + usecase.On("FindByID", ctx, wantUser.ID).Maybe().Return(nil, nil) } if tc.orgExist { From 94280da3d42cbc8f7b13d5471ebef8e5eb22179d Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 7 Dec 2023 16:46:30 +0100 Subject: [PATCH 2/3] fix: enable JWT audience verification Signed-off-by: Miguel Martinez Trivino --- app/controlplane/internal/jwt/robotaccount/robotaccount.go | 5 ++--- app/controlplane/internal/jwt/user/user.go | 4 +--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/app/controlplane/internal/jwt/robotaccount/robotaccount.go b/app/controlplane/internal/jwt/robotaccount/robotaccount.go index 501ca52c0..44047270c 100644 --- a/app/controlplane/internal/jwt/robotaccount/robotaccount.go +++ b/app/controlplane/internal/jwt/robotaccount/robotaccount.go @@ -33,7 +33,6 @@ const ( type Builder struct { issuer string hmacSecret string - audience string } type NewOpt func(b *Builder) @@ -55,7 +54,7 @@ func WithKeySecret(hmacSecret string) NewOpt { // Currently we use a simple hmac encryption method meant to be continuously rotated // TODO: additional/alternative encryption method, i.e DSE asymetric, see CAS robot account for reference func NewBuilder(opts ...NewOpt) (*Builder, error) { - b := &Builder{audience: Audience} + b := &Builder{} for _, opt := range opts { opt(b) } @@ -80,7 +79,7 @@ func (ra *Builder) GenerateJWT(orgID, workflowID, keyID string) (string, error) // Key identifier so we can check it's revocation status ID: keyID, Issuer: ra.issuer, - Audience: jwt.ClaimStrings{ra.audience}, + Audience: jwt.ClaimStrings{Audience}, }, } diff --git a/app/controlplane/internal/jwt/user/user.go b/app/controlplane/internal/jwt/user/user.go index 316507435..5414ab432 100644 --- a/app/controlplane/internal/jwt/user/user.go +++ b/app/controlplane/internal/jwt/user/user.go @@ -28,7 +28,6 @@ type Builder struct { issuer string hmacSecret string expiration time.Duration - audience string } type NewOpt func(b *Builder) @@ -57,7 +56,6 @@ var SigningMethod = jwt.SigningMethodHS256 func NewBuilder(opts ...NewOpt) (*Builder, error) { b := &Builder{ expiration: defaultExpiration, - audience: Audience, } for _, opt := range opts { @@ -80,7 +78,7 @@ func (ra *Builder) GenerateJWT(userID string) (string, error) { userID, jwt.RegisteredClaims{ Issuer: ra.issuer, - Audience: jwt.ClaimStrings{ra.audience}, + Audience: jwt.ClaimStrings{Audience}, ExpiresAt: jwt.NewNumericDate(time.Now().Add(ra.expiration)), }, } From a6547051a818d688e3c8ea0c92834336b5e40251 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 7 Dec 2023 17:00:44 +0100 Subject: [PATCH 3/3] fix: enable JWT audience verification Signed-off-by: Miguel Martinez Trivino --- .../internal/usercontext/userorg_middleware_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/controlplane/internal/usercontext/userorg_middleware_test.go b/app/controlplane/internal/usercontext/userorg_middleware_test.go index 92e508fda..09765479f 100644 --- a/app/controlplane/internal/usercontext/userorg_middleware_test.go +++ b/app/controlplane/internal/usercontext/userorg_middleware_test.go @@ -22,7 +22,6 @@ import ( "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz" bizMocks "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz/mocks" - "github.com/chainloop-dev/chainloop/app/controlplane/internal/jwt/user" userjwtbuilder "github.com/chainloop-dev/chainloop/app/controlplane/internal/jwt/user" "github.com/go-kratos/kratos/v2/log" jwtmiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt" @@ -52,7 +51,7 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) { { name: "logged in, user and org exists", loggedIn: true, - audience: user.Audience, + audience: userjwtbuilder.Audience, userExist: true, orgExist: true, wantErr: false, @@ -60,21 +59,21 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) { { name: "logged in, user does not exist", loggedIn: true, - audience: user.Audience, + audience: userjwtbuilder.Audience, userExist: false, wantErr: true, }, { name: "logged in, org does not exist", loggedIn: true, - audience: user.Audience, + audience: userjwtbuilder.Audience, userExist: true, wantErr: true, }, { name: "not logged in", loggedIn: false, - audience: user.Audience, + audience: userjwtbuilder.Audience, wantErr: true, }, }