Skip to content

Commit 1384232

Browse files
kushsharmakushsharma
authored
fix: jwt generated via service users should authenticate (#860)
JWTs generated by Service Users were ignoring the user id during verification of the user. Instead of checking just real user, service users should be treated similar as well. Signed-off-by: Kush Sharma <[email protected]>
1 parent 67a34d7 commit 1384232

File tree

7 files changed

+195
-49
lines changed

7 files changed

+195
-49
lines changed

‎core/authenticate/mocks/service_user_service.go‎

Lines changed: 58 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎core/authenticate/service.go‎

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ type UserService interface {
6565
}
6666

6767
type ServiceUserService interface {
68+
Get(ctx context.Context, id string) (serviceuser.ServiceUser, error)
6869
GetByJWT(ctx context.Context, token string) (serviceuser.ServiceUser, error)
6970
GetBySecret(ctx context.Context, clientID, clientSecret string) (serviceuser.ServiceUser, error)
7071
}
@@ -677,8 +678,12 @@ func (s Service) applyOIDC(ctx context.Context, request RegistrationFinishReques
677678
}
678679

679680
// BuildToken creates an access token for the given subjectID
680-
func (s Service) BuildToken(ctx context.Context, subjectID string, metadata map[string]string) ([]byte, error) {
681-
return s.internalTokenService.Build(subjectID, metadata)
681+
func (s Service) BuildToken(ctx context.Context, principal Principal, metadata map[string]string) ([]byte, error) {
682+
metadata[token.SubTypeClaimsKey] = principal.Type
683+
if principal.Type == schema.UserPrincipal && s.config.Token.Claims.AddUserEmailClaim {
684+
metadata[token.SubEmailClaimsKey] = principal.User.Email
685+
}
686+
return s.internalTokenService.Build(principal.ID, metadata)
682687
}
683688

684689
// JWKs returns the public keys to verify the access token
@@ -775,27 +780,42 @@ func (s Service) GetPrincipal(ctx context.Context, assertions ...ClientAssertion
775780
return Principal{}, errors.ErrUnauthenticated
776781
}
777782
// check type of jwt
778-
if val, ok := insecureJWT.Get(token.GeneratedClaimKey); ok {
779-
if claimVal, ok := val.(string); ok && claimVal == token.GeneratedClaimValue {
780-
// extract user from token if present as its created by frontier
781-
userID, _, err := s.internalTokenService.Parse(ctx, []byte(userToken))
782-
if err == nil && utils.IsValidUUID(userID) {
783-
// userID is a valid uuid
784-
currentUser, err := s.userService.GetByID(ctx, userID)
785-
if err != nil {
786-
return Principal{}, err
787-
}
788-
return Principal{
789-
ID: currentUser.ID,
790-
Type: schema.UserPrincipal,
791-
User: &currentUser,
792-
}, nil
793-
}
783+
if genClaim, ok := insecureJWT.Get(token.GeneratedClaimKey); ok {
784+
// jwt generated by frontier using public key
785+
claimVal, ok := genClaim.(string)
786+
if !ok || claimVal != token.GeneratedClaimValue {
787+
return Principal{}, errors.ErrUnauthenticated
788+
}
789+
790+
// extract user from token if present as its created by frontier
791+
userID, claims, err := s.internalTokenService.Parse(ctx, []byte(userToken))
792+
if err != nil || !utils.IsValidUUID(userID) {
793+
s.log.Debug("failed to parse as internal token ", "err", err)
794+
return Principal{}, errors.ErrUnauthenticated
795+
}
796+
797+
// userID is a valid uuid
798+
if claims[token.SubTypeClaimsKey] == schema.ServiceUserPrincipal {
799+
currentUser, err := s.serviceUserService.Get(ctx, userID)
794800
if err != nil {
795-
s.log.Debug("failed to parse as internal token ", "err", err)
796-
return Principal{}, errors.ErrUnauthenticated
801+
return Principal{}, err
797802
}
803+
return Principal{
804+
ID: currentUser.ID,
805+
Type: schema.ServiceUserPrincipal,
806+
ServiceUser: &currentUser,
807+
}, nil
808+
}
809+
810+
currentUser, err := s.userService.GetByID(ctx, userID)
811+
if err != nil {
812+
return Principal{}, err
798813
}
814+
return Principal{
815+
ID: currentUser.ID,
816+
Type: schema.UserPrincipal,
817+
User: &currentUser,
818+
}, nil
799819
}
800820
}
801821

‎core/authenticate/token/service.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ const (
2121
GeneratedClaimKey = "gen"
2222
GeneratedClaimValue = "system"
2323
OrgIDsClaimKey = "org_ids"
24+
SubTypeClaimsKey = "sub_type"
25+
SubEmailClaimsKey = "email"
2426
)
2527

2628
type Service struct {
@@ -79,11 +81,11 @@ func (s Service) Parse(ctx context.Context, userToken []byte) (string, map[strin
7981
// verify token with jwks
8082
verifiedToken, err := jwt.Parse(userToken, jwt.WithKeySet(s.publicKeySet))
8183
if err != nil {
82-
return "", nil, fmt.Errorf("%s: %w", ErrInvalidToken.Error(), err)
84+
return "", nil, fmt.Errorf("%s: %w", err.Error(), ErrInvalidToken)
8385
}
8486
tokenClaims, err := verifiedToken.AsMap(ctx)
8587
if err != nil {
86-
return "", nil, fmt.Errorf("%s: %w", ErrInvalidToken.Error(), err)
88+
return "", nil, fmt.Errorf("%s: %w", err.Error(), ErrInvalidToken)
8789
}
8890
return verifiedToken.Subject(), tokenClaims, nil
8991
}

‎internal/api/v1beta1/authenticate.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import (
3838
type AuthnService interface {
3939
StartFlow(ctx context.Context, request authenticate.RegistrationStartRequest) (*authenticate.RegistrationStartResponse, error)
4040
FinishFlow(ctx context.Context, request authenticate.RegistrationFinishRequest) (*authenticate.RegistrationFinishResponse, error)
41-
BuildToken(ctx context.Context, principalID string, metadata map[string]string) ([]byte, error)
41+
BuildToken(ctx context.Context, principal authenticate.Principal, metadata map[string]string) ([]byte, error)
4242
JWKs(ctx context.Context) jwk.Set
4343
GetPrincipal(ctx context.Context, via ...authenticate.ClientAssertion) (authenticate.Principal, error)
4444
SupportedStrategies() []string
@@ -292,9 +292,6 @@ func (h Handler) getAccessToken(ctx context.Context, principal authenticate.Prin
292292
}
293293
customClaims[token.OrgIDsClaimKey] = strings.Join(orgIds, ",")
294294
}
295-
if principal.Type == schema.UserPrincipal && h.authConfig.Token.Claims.AddUserEmailClaim {
296-
customClaims["email"] = principal.User.Email
297-
}
298295

299296
// find selected project id
300297
if md, ok := metadata.FromIncomingContext(ctx); ok {
@@ -317,7 +314,7 @@ func (h Handler) getAccessToken(ctx context.Context, principal authenticate.Prin
317314
}
318315

319316
// build jwt for user context
320-
return h.authnService.BuildToken(ctx, principal.ID, customClaims)
317+
return h.authnService.BuildToken(ctx, principal, customClaims)
321318
}
322319

323320
func setRedirectHeaders(ctx context.Context, url string) error {

‎internal/api/v1beta1/mocks/authn_service.go‎

Lines changed: 16 additions & 16 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/api/v1beta1/user_test.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,10 @@ func TestGetCurrentUser(t *testing.T) {
450450
mockOrgService := new(mocks.OrganizationService)
451451
mockOrgService.EXPECT().ListByUser(mock.Anything, "user-id-1", organization.Filter{}).Return([]organization.Organization{}, nil)
452452
mockAuthnSrv.EXPECT().BuildToken(mock.Anything,
453-
"user-id-1", map[string]string{"orgs": ""}).Return(nil, token.ErrMissingRSADisableToken)
453+
authenticate.Principal{
454+
ID: "user-id-1",
455+
Type: schema.UserPrincipal,
456+
}, map[string]string{"orgs": ""}).Return(nil, token.ErrMissingRSADisableToken)
454457

455458
if tt.setup != nil {
456459
ctx = tt.setup(ctx, mockAuthnSrv, nil)

0 commit comments

Comments
 (0)