Skip to content

Commit

Permalink
fix: override sub with federated_claims.user_id when dex is used (#20683
Browse files Browse the repository at this point in the history
)

Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>
  • Loading branch information
aali309 and agaudreault authored Jan 30, 2025
1 parent 40d86e7 commit 85c6d26
Show file tree
Hide file tree
Showing 13 changed files with 405 additions and 84 deletions.
21 changes: 13 additions & 8 deletions cmd/argocd/commands/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ import (
argocdclient "github.com/argoproj/argo-cd/v3/pkg/apiclient"
sessionpkg "github.com/argoproj/argo-cd/v3/pkg/apiclient/session"
settingspkg "github.com/argoproj/argo-cd/v3/pkg/apiclient/settings"
claimsutil "github.com/argoproj/argo-cd/v3/util/claims"
"github.com/argoproj/argo-cd/v3/util/cli"
"github.com/argoproj/argo-cd/v3/util/errors"
grpc_util "github.com/argoproj/argo-cd/v3/util/grpc"
"github.com/argoproj/argo-cd/v3/util/io"
jwtutil "github.com/argoproj/argo-cd/v3/util/jwt"
"github.com/argoproj/argo-cd/v3/util/localconfig"
oidcutil "github.com/argoproj/argo-cd/v3/util/oidc"
"github.com/argoproj/argo-cd/v3/util/rand"
Expand Down Expand Up @@ -143,7 +143,9 @@ argocd login cd.argoproj.io --core`,
claims := jwt.MapClaims{}
_, _, err := parser.ParseUnverified(tokenString, &claims)
errors.CheckError(err)
fmt.Printf("'%s' logged in successfully\n", userDisplayName(claims))
argoClaims, err := claimsutil.MapClaimsToArgoClaims(claims)
errors.CheckError(err)
fmt.Printf("'%s' logged in successfully\n", userDisplayName(argoClaims))
}

// login successful. Persist the config
Expand Down Expand Up @@ -190,14 +192,17 @@ argocd login cd.argoproj.io --core`,
return command
}

func userDisplayName(claims jwt.MapClaims) string {
if email := jwtutil.StringField(claims, "email"); email != "" {
return email
func userDisplayName(claims *claimsutil.ArgoClaims) string {
if claims == nil {
return ""
}
if claims.Email != "" {
return claims.Email
}
if name := jwtutil.StringField(claims, "name"); name != "" {
return name
if claims.Name != "" {
return claims.Name
}
return jwtutil.StringField(claims, "sub")
return claims.GetUserIdentifier()
}

// oauth2Login opens a browser, runs a temporary HTTP server to delegate OAuth2 login flow and
Expand Down
27 changes: 24 additions & 3 deletions cmd/argocd/commands/login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import (
"os"
"testing"

claimsutil "github.com/argoproj/argo-cd/v3/util/claims"
utils "github.com/argoproj/argo-cd/v3/util/io"

"github.com/golang-jwt/jwt/v5"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func captureStdout(callback func()) (string, error) {
Expand All @@ -35,26 +37,45 @@ func captureStdout(callback func()) (string, error) {
}

func Test_userDisplayName_email(t *testing.T) {
claims := jwt.MapClaims{"iss": "qux", "sub": "foo", "email": "[email protected]", "groups": []string{"baz"}}
claims, err := claimsutil.MapClaimsToArgoClaims(jwt.MapClaims{"iss": "qux", "sub": "foo", "email": "[email protected]", "groups": []string{"baz"}})
require.NoError(t, err)
actualName := userDisplayName(claims)
expectedName := "[email protected]"
assert.Equal(t, expectedName, actualName)
}

func Test_userDisplayName_name(t *testing.T) {
claims := jwt.MapClaims{"iss": "qux", "sub": "foo", "name": "Firstname Lastname", "groups": []string{"baz"}}
claims, err := claimsutil.MapClaimsToArgoClaims(jwt.MapClaims{"iss": "qux", "sub": "foo", "name": "Firstname Lastname", "groups": []string{"baz"}})
require.NoError(t, err)
actualName := userDisplayName(claims)
expectedName := "Firstname Lastname"
assert.Equal(t, expectedName, actualName)
}

func Test_userDisplayName_sub(t *testing.T) {
claims := jwt.MapClaims{"iss": "qux", "sub": "foo", "groups": []string{"baz"}}
claims, err := claimsutil.MapClaimsToArgoClaims(jwt.MapClaims{"iss": "qux", "sub": "foo", "groups": []string{"baz"}})
require.NoError(t, err)
actualName := userDisplayName(claims)
expectedName := "foo"
assert.Equal(t, expectedName, actualName)
}

func Test_userDisplayName_federatedClaims(t *testing.T) {
claims, err := claimsutil.MapClaimsToArgoClaims(jwt.MapClaims{
"iss": "qux",
"sub": "foo",
"groups": []string{"baz"},
"federated_claims": map[string]any{
"connector_id": "dex",
"user_id": "ldap-123",
},
})
require.NoError(t, err)
actualName := userDisplayName(claims)
expectedName := "ldap-123"
assert.Equal(t, expectedName, actualName)
}

func Test_ssoAuthFlow_ssoLaunchBrowser_false(t *testing.T) {
out, _ := captureStdout(func() {
ssoAuthFlow("http://test-sso-browser-flow.com", false)
Expand Down
21 changes: 14 additions & 7 deletions cmd/argocd/commands/project_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
argocdclient "github.com/argoproj/argo-cd/v3/pkg/apiclient"
projectpkg "github.com/argoproj/argo-cd/v3/pkg/apiclient/project"
"github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
claimsutil "github.com/argoproj/argo-cd/v3/util/claims"
"github.com/argoproj/argo-cd/v3/util/errors"
"github.com/argoproj/argo-cd/v3/util/io"
"github.com/argoproj/argo-cd/v3/util/jwt"
Expand Down Expand Up @@ -191,7 +192,7 @@ func NewProjectRoleCreateCommand(clientOpts *argocdclient.ClientOptions) *cobra.
command := &cobra.Command{
Use: "create PROJECT ROLE-NAME",
Short: "Create a project role",
Example: templates.Examples(`
Example: templates.Examples(`
# Create a project role in the "my-project" project with the name "my-role".
argocd proj role create my-project my-role --description "My project role description"
`),
Expand Down Expand Up @@ -321,18 +322,24 @@ Create token succeeded for proj:test-project:test-role.
})
errors.CheckError(err)

token, err := jwtgo.Parse(tokenResponse.Token, nil)
if token == nil {
var claims jwtgo.MapClaims
_, _, err = jwtgo.NewParser().ParseUnverified(tokenResponse.Token, &claims)
if err != nil {
err = fmt.Errorf("received malformed token %w", err)
errors.CheckError(err)
return
}

claims := token.Claims.(jwtgo.MapClaims)
argoClaims, err := claimsutil.MapClaimsToArgoClaims(claims)
if err != nil {
errors.CheckError(fmt.Errorf("invalid argo claims: %w", err))
return
}

issuedAt, _ := jwt.IssuedAt(claims)
expiresAt := int64(jwt.Float64Field(claims, "exp"))
id := jwt.StringField(claims, "jti")
subject := jwt.StringField(claims, "sub")
id := argoClaims.ID
subject := argoClaims.GetUserIdentifier()

if !outputTokenOnly {
fmt.Printf("Create token succeeded for %s.\n", subject)
Expand Down Expand Up @@ -496,7 +503,7 @@ func NewProjectRoleListCommand(clientOpts *argocdclient.ClientOptions) *cobra.Co
command := &cobra.Command{
Use: "list PROJECT",
Short: "List all the roles in a project",
Example: templates.Examples(`
Example: templates.Examples(`
# This command will list all the roles in argocd-project in a default table format.
argocd proj role list PROJECT
Expand Down
11 changes: 7 additions & 4 deletions server/account/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,16 @@ func NewServer(sessionMgr *session.SessionManager, settingsMgr *settings.Setting

// UpdatePassword updates the password of the currently authenticated account or the account specified in the request.
func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRequest) (*account.UpdatePasswordResponse, error) {
issuer := session.Iss(ctx)
username := session.Sub(ctx)
updatedUsername := username
username := session.GetUserIdentifier(ctx)

updatedUsername := username
if q.Name != "" {
updatedUsername = q.Name
}

// check for permission is user is trying to change someone else's password
// assuming user is trying to update someone else if username is different or issuer is not Argo CD
issuer := session.Iss(ctx)
if updatedUsername != username || issuer != session.SessionManagerClaimsIssuer {
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, rbacpolicy.ActionUpdate, q.Name); err != nil {
return nil, fmt.Errorf("permission denied: %w", err)
Expand Down Expand Up @@ -168,8 +169,10 @@ func toApiAccount(name string, a settings.Account) *account.Account {
}

func (s *Server) ensureHasAccountPermission(ctx context.Context, action string, account string) error {
id := session.GetUserIdentifier(ctx)

// account has always has access to itself
if session.Sub(ctx) == account && session.Iss(ctx) == session.SessionManagerClaimsIssuer {
if id == account && session.Iss(ctx) == session.SessionManagerClaimsIssuer {
return nil
}
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, action, account); err != nil {
Expand Down
7 changes: 6 additions & 1 deletion server/rbacpolicy/rbacpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
applister "github.com/argoproj/argo-cd/v3/pkg/client/listers/application/v1alpha1"
claimsutil "github.com/argoproj/argo-cd/v3/util/claims"
jwtutil "github.com/argoproj/argo-cd/v3/util/jwt"
"github.com/argoproj/argo-cd/v3/util/rbac"
)
Expand Down Expand Up @@ -115,8 +116,12 @@ func (p *RBACPolicyEnforcer) EnforceClaims(claims jwt.Claims, rvals ...any) bool
if err != nil {
return false
}
argoClaims, err := claimsutil.MapClaimsToArgoClaims(mapClaims)
if err != nil {
return false
}

subject := jwtutil.StringField(mapClaims, "sub")
subject := argoClaims.GetUserIdentifier()
// Check if the request is for an application resource. We have special enforcement which takes
// into consideration the project's token and group bindings
var runtimePolicy string
Expand Down
5 changes: 5 additions & 0 deletions server/rbacpolicy/rbacpolicy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ func TestEnforceAllPolicies(t *testing.T) {
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "exec", "create", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "qwertyuiop", "federated_claims": map[string]any{"user_id": "bob"}}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "exec", "create", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "proj:my-proj:my-role", "iat": 1234}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))
Expand Down
34 changes: 20 additions & 14 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,10 @@ import (
"github.com/argoproj/argo-cd/v3/ui"
"github.com/argoproj/argo-cd/v3/util/assets"
cacheutil "github.com/argoproj/argo-cd/v3/util/cache"
claimsutil "github.com/argoproj/argo-cd/v3/util/claims"
"github.com/argoproj/argo-cd/v3/util/db"
dexutil "github.com/argoproj/argo-cd/v3/util/dex"

"github.com/argoproj/argo-cd/v3/util/env"
errorsutil "github.com/argoproj/argo-cd/v3/util/errors"
grpc_util "github.com/argoproj/argo-cd/v3/util/grpc"
Expand Down Expand Up @@ -1522,19 +1524,19 @@ func (server *ArgoCDServer) getClaims(ctx context.Context) (jwt.Claims, string,
return claims, "", status.Errorf(codes.Unauthenticated, "invalid session: %v", err)
}

// Some SSO implementations (Okta) require a call to
// the OIDC user info path to get attributes like groups
// we assume that everywhere in argocd jwt.MapClaims is used as type for interface jwt.Claims
// otherwise this would cause a panic
var groupClaims jwt.MapClaims
if groupClaims, ok = claims.(jwt.MapClaims); !ok {
if tmpClaims, ok := claims.(*jwt.MapClaims); ok {
groupClaims = *tmpClaims
}
mapClaims, err := jwtutil.MapClaims(claims)
if err != nil {
return claims, "", status.Errorf(codes.Internal, "invalid claims")
}
argoClaims, err := claimsutil.MapClaimsToArgoClaims(mapClaims)
if err != nil {
return claims, "", status.Errorf(codes.Internal, "invalid argo claims")
}
iss := jwtutil.StringField(groupClaims, "iss")

// Some SSO implementations (Okta) require a call to the OIDC user info path to get attributes like groups
iss := jwtutil.StringField(mapClaims, "iss")
if iss != util_session.SessionManagerClaimsIssuer && server.settings.UserInfoGroupsEnabled() && server.settings.UserInfoPath() != "" {
userInfo, unauthorized, err := server.ssoClientApp.GetUserInfo(groupClaims, server.settings.IssuerURL(), server.settings.UserInfoPath())
userInfo, unauthorized, err := server.ssoClientApp.GetUserInfo(mapClaims, server.settings.IssuerURL(), server.settings.UserInfoPath())
if unauthorized {
log.Errorf("error while quering userinfo endpoint: %v", err)
return claims, "", status.Errorf(codes.Unauthenticated, "invalid session")
Expand All @@ -1543,13 +1545,17 @@ func (server *ArgoCDServer) getClaims(ctx context.Context) (jwt.Claims, string,
log.Errorf("error fetching user info endpoint: %v", err)
return claims, "", status.Errorf(codes.Internal, "invalid userinfo response")
}
if groupClaims["sub"] != userInfo["sub"] {
userInfoClaims, err := claimsutil.MapClaimsToArgoClaims(userInfo)
if err != nil {
return claims, "", status.Errorf(codes.Internal, "invalid userinfo claims")
}
if argoClaims.Subject != userInfoClaims.Subject {
return claims, "", status.Error(codes.Unknown, "subject of claims from user info endpoint didn't match subject of idToken, see https://openid.net/specs/openid-connect-core-1_0.html#UserInfo")
}
groupClaims["groups"] = userInfo["groups"]
mapClaims["groups"] = userInfo["groups"]
}

return groupClaims, newToken, nil
return mapClaims, newToken, nil
}

// getToken extracts the token from gRPC metadata or cookie headers
Expand Down
4 changes: 2 additions & 2 deletions server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,7 @@ func TestAuthenticate_3rd_party_JWTs(t *testing.T) {
anonymousEnabled: false,
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
expectedErrorContains: common.TokenVerificationError,
expectedClaims: jwt.RegisteredClaims{Issuer: "sso"},
expectedClaims: jwt.MapClaims{"iss": "sso"},
},
{
test: "anonymous enabled, expired token, admin claim",
Expand Down Expand Up @@ -870,7 +870,7 @@ func TestAuthenticate_3rd_party_JWTs(t *testing.T) {
claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{common.ArgoCDClientAppID}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())},
useDex: true,
expectedErrorContains: common.TokenVerificationError,
expectedClaims: jwt.RegisteredClaims{Issuer: "sso"},
expectedClaims: jwt.MapClaims{"iss": "sso"},
},
{
test: "external OIDC: anonymous enabled, expired token, admin claim",
Expand Down
56 changes: 56 additions & 0 deletions util/claims/claims.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package claims

import (
"encoding/json"

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

// ArgoClaims defines the claims structure based on Dex's documented claims
type ArgoClaims struct {
jwt.RegisteredClaims
Email string `json:"email,omitempty"`
EmailVerified bool `json:"email_verified,omitempty"`
Name string `json:"name,omitempty"`
Groups []string `json:"groups,omitempty"`
// As per Dex docs, federated_claims has a specific structure
FederatedClaims *FederatedClaims `json:"federated_claims,omitempty"`
}

// FederatedClaims represents the structure documented by Dex
type FederatedClaims struct {
ConnectorID string `json:"connector_id"`
UserID string `json:"user_id"`
}

// MapClaimsToArgoClaims converts a jwt.MapClaims to a ArgoClaims
func MapClaimsToArgoClaims(claims jwt.MapClaims) (*ArgoClaims, error) {
if claims == nil {
return &ArgoClaims{}, nil
}

claimsBytes, err := json.Marshal(claims)
if err != nil {
return nil, err
}

var argoClaims ArgoClaims
err = json.Unmarshal(claimsBytes, &argoClaims)
if err != nil {
return nil, err
}
return &argoClaims, nil
}

// GetUserIdentifier returns a consistent user identifier, checking federated_claims.user_id when Dex is in use
func (c *ArgoClaims) GetUserIdentifier() string {
// Check federated claims first
if c.FederatedClaims != nil && c.FederatedClaims.UserID != "" {
return c.FederatedClaims.UserID
}
// Fallback to sub
if c.Subject != "" {
return c.Subject
}
return ""
}
Loading

0 comments on commit 85c6d26

Please sign in to comment.