Skip to content

Commit e1f14ee

Browse files
authored
fix: enable JWT audience verification (#459)
Signed-off-by: Miguel Martinez Trivino <[email protected]>
1 parent 8ae1211 commit e1f14ee

File tree

10 files changed

+46
-12
lines changed

10 files changed

+46
-12
lines changed

app/controlplane/internal/biz/robotaccount.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func (uc *RobotAccountUseCase) Create(ctx context.Context, name string, orgID, w
8989
return nil, err
9090
}
9191

92-
jwt, err := b.GenerateJWT(orgID, workflowID, res.ID.String(), jwt.DefaultAudience)
92+
jwt, err := b.GenerateJWT(orgID, workflowID, res.ID.String())
9393
if err != nil {
9494
return nil, err
9595
}

app/controlplane/internal/jwt/common.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,5 +16,4 @@
1616
package jwt
1717

1818
const DefaultIssuer = "cp.chainloop"
19-
const DefaultAudience = "client.chainloop"
2019
const CASAudience = "artifact-cas.chainloop"

app/controlplane/internal/jwt/robotaccount/robotaccount.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@ import (
2323

2424
var SigningMethod = jwt.SigningMethodHS256
2525

26+
// This type of JWT is meant to be used by the attestations service
27+
const (
28+
Audience = "attestations.chainloop"
29+
// Previous audience, deprecated, we keep it to not to break compatibility
30+
DeprecatedAudience = "client.chainloop"
31+
)
32+
2633
type Builder struct {
2734
issuer string
2835
hmacSecret string
@@ -64,15 +71,15 @@ func NewBuilder(opts ...NewOpt) (*Builder, error) {
6471
}
6572

6673
// NOTE: It does not expire, it will get revoked instead
67-
func (ra *Builder) GenerateJWT(orgID, workflowID, keyID, audience string) (string, error) {
74+
func (ra *Builder) GenerateJWT(orgID, workflowID, keyID string) (string, error) {
6875
claims := CustomClaims{
6976
orgID,
7077
workflowID,
7178
jwt.RegisteredClaims{
7279
// Key identifier so we can check it's revocation status
7380
ID: keyID,
7481
Issuer: ra.issuer,
75-
Audience: jwt.ClaimStrings{audience},
82+
Audience: jwt.ClaimStrings{Audience},
7683
},
7784
}
7885

app/controlplane/internal/jwt/robotaccount/robotaccount_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func TestGenerateJWT(t *testing.T) {
7575
)
7676
require.NoError(t, err)
7777

78-
token, err := b.GenerateJWT("org-id", "workflow-id", "key-id", "my-audience")
78+
token, err := b.GenerateJWT("org-id", "workflow-id", "key-id")
7979
assert.NoError(t, err)
8080
assert.NotEmpty(t, token)
8181

@@ -91,6 +91,6 @@ func TestGenerateJWT(t *testing.T) {
9191
assert.Equal(t, "workflow-id", claims.WorkflowID)
9292
assert.Equal(t, "key-id", claims.ID)
9393
assert.Equal(t, "my-issuer", claims.Issuer)
94-
assert.Contains(t, claims.Audience, "my-audience")
94+
assert.Contains(t, claims.Audience, Audience)
9595
assert.Nil(t, claims.ExpiresAt)
9696
}

app/controlplane/internal/jwt/user/user.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"github.com/golang-jwt/jwt/v4"
2323
)
2424

25+
const Audience = "user-auth.chainloop"
26+
2527
type Builder struct {
2628
issuer string
2729
hmacSecret string
@@ -71,12 +73,12 @@ func NewBuilder(opts ...NewOpt) (*Builder, error) {
7173
return b, nil
7274
}
7375

74-
func (ra *Builder) GenerateJWT(userID, audience string) (string, error) {
76+
func (ra *Builder) GenerateJWT(userID string) (string, error) {
7577
claims := CustomClaims{
7678
userID,
7779
jwt.RegisteredClaims{
7880
Issuer: ra.issuer,
79-
Audience: jwt.ClaimStrings{audience},
81+
Audience: jwt.ClaimStrings{Audience},
8082
ExpiresAt: jwt.NewNumericDate(time.Now().Add(ra.expiration)),
8183
},
8284
}

app/controlplane/internal/jwt/user/user_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func TestGenerateJWT(t *testing.T) {
7777
)
7878
require.NoError(t, err)
7979

80-
token, err := b.GenerateJWT("user-id", "my-audience")
80+
token, err := b.GenerateJWT("user-id")
8181
assert.NoError(t, err)
8282
assert.NotEmpty(t, token)
8383

@@ -91,6 +91,6 @@ func TestGenerateJWT(t *testing.T) {
9191
assert.True(t, tokenInfo.Valid)
9292
assert.Equal(t, "user-id", claims.UserID)
9393
assert.Equal(t, "my-issuer", claims.Issuer)
94-
assert.Contains(t, claims.Audience, "my-audience")
94+
assert.Contains(t, claims.Audience, Audience)
9595
assert.WithinDuration(t, time.Now(), claims.ExpiresAt.Time, 10*time.Second)
9696
}

app/controlplane/internal/service/auth.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ func generateUserJWT(userID, passphrase string, expiration time.Duration) (strin
350350
return "", err
351351
}
352352

353-
return b.GenerateJWT(userID, jwt.DefaultAudience)
353+
return b.GenerateJWT(userID)
354354
}
355355

356356
func setOauthCookie(w http.ResponseWriter, name, value string) {

app/controlplane/internal/usercontext/robotaccount_middleware.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,12 @@ func WithCurrentRobotAccount(robotAccountUseCase *biz.RobotAccountUseCase, logge
6262
return nil, errors.New("error mapping the claims")
6363
}
6464

65+
// Do not accept tokens that are crafted for a different audience in this system
66+
// NOTE: we allow deprecated audience to not to break compatibility with previously issued robot-accounts
67+
if !claims.VerifyAudience(robotaccount.Audience, true) && !claims.VerifyAudience(robotaccount.DeprecatedAudience, true) {
68+
return nil, errors.New("unexpected token, invalid audience")
69+
}
70+
6571
// Extract account ID
6672
robotAccountID := claims.ID
6773
if robotAccountID == "" {

app/controlplane/internal/usercontext/userorg_middleware.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ func WithCurrentUserAndOrgMiddleware(userUseCase biz.UserOrgFinder, logger *log.
8686
return nil, errors.New("error mapping the claims")
8787
}
8888

89+
// Do not accept tokens that are crafted for a different audience in this system
90+
if !customClaims.VerifyAudience(user.Audience, true) {
91+
return nil, errors.New("unexpected token, invalid audience")
92+
}
93+
8994
userID := customClaims.UserID
9095
if userID == "" {
9196
return nil, errors.New("error retrieving the user information from the auth token")

app/controlplane/internal/usercontext/userorg_middleware_test.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
userjwtbuilder "github.com/chainloop-dev/chainloop/app/controlplane/internal/jwt/user"
2626
"github.com/go-kratos/kratos/v2/log"
2727
jwtmiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt"
28+
"github.com/golang-jwt/jwt/v4"
2829
"github.com/google/uuid"
2930
"github.com/stretchr/testify/assert"
3031
)
@@ -36,32 +37,43 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) {
3637
testCases := []struct {
3738
name string
3839
loggedIn bool
40+
audience string
3941
userExist bool
4042
orgExist bool
4143
wantErr bool
4244
}{
45+
{
46+
name: "invalid audience",
47+
loggedIn: true,
48+
audience: "another-aud",
49+
wantErr: true,
50+
},
4351
{
4452
name: "logged in, user and org exists",
4553
loggedIn: true,
54+
audience: userjwtbuilder.Audience,
4655
userExist: true,
4756
orgExist: true,
4857
wantErr: false,
4958
},
5059
{
5160
name: "logged in, user does not exist",
5261
loggedIn: true,
62+
audience: userjwtbuilder.Audience,
5363
userExist: false,
5464
wantErr: true,
5565
},
5666
{
5767
name: "logged in, org does not exist",
5868
loggedIn: true,
69+
audience: userjwtbuilder.Audience,
5970
userExist: true,
6071
wantErr: true,
6172
},
6273
{
6374
name: "not logged in",
6475
loggedIn: false,
76+
audience: userjwtbuilder.Audience,
6577
wantErr: true,
6678
},
6779
}
@@ -76,13 +88,16 @@ func TestWithCurrentUserAndOrgMiddleware(t *testing.T) {
7688
if tc.loggedIn {
7789
ctx = jwtmiddleware.NewContext(ctx, &userjwtbuilder.CustomClaims{
7890
UserID: wantUser.ID,
91+
RegisteredClaims: jwt.RegisteredClaims{
92+
Audience: jwt.ClaimStrings{tc.audience},
93+
},
7994
})
8095
}
8196

8297
if tc.userExist {
8398
usecase.On("FindByID", ctx, wantUser.ID).Return(wantUser, nil)
8499
} else if tc.loggedIn {
85-
usecase.On("FindByID", ctx, wantUser.ID).Return(nil, nil)
100+
usecase.On("FindByID", ctx, wantUser.ID).Maybe().Return(nil, nil)
86101
}
87102

88103
if tc.orgExist {

0 commit comments

Comments
 (0)