diff --git a/pkg/api/authn.go b/pkg/api/authn.go index fed8fca1..1fd84ca9 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -602,7 +602,7 @@ func bearerAuthHandler(ctlr *Controller) mux.MiddlewareFunc { } // Try OIDC authentication first if configured - var username string + var identity string var groups []string @@ -611,21 +611,21 @@ func bearerAuthHandler(ctlr *Controller) mux.MiddlewareFunc { var authenticated bool - username, groups, authenticated, err = oidcAuthorizer.AuthenticateRequest(request.Context(), header) + identity, groups, authenticated, err = oidcAuthorizer.AuthenticateRequest(request.Context(), header) if err == nil && authenticated { // OIDC authentication succeeded - ctlr.Log.Debug().Str("username", username).Msg("the OIDC bearer authentication was successful") + ctlr.Log.Debug().Str("identity", identity).Msg("the OIDC bearer authentication was successful") // Set user context for authorization userAc := reqCtx.NewUserAccessControl() - userAc.SetUsername(username) + userAc.SetUsername(identity) userAc.AddGroups(groups) userAc.SaveOnRequest(request) // Update user groups in MetaDB if available if ctlr.MetaDB != nil { if err := ctlr.MetaDB.SetUserGroups(request.Context(), groups); err != nil { - ctlr.Log.Error().Err(err).Str("username", username).Msg("failed to update user profile") + ctlr.Log.Error().Err(err).Str("identity", identity).Msg("failed to update user profile") response.WriteHeader(http.StatusInternalServerError) return @@ -1027,10 +1027,10 @@ func getUsernamePasswordBasicAuth(request *http.Request) (string, string, error) return "", "", zerr.ErrParsingAuthHeader } - username := pair[0] + identity := pair[0] passphrase := pair[1] - return username, passphrase, nil + return identity, passphrase, nil } func GetGithubUserInfo(ctx context.Context, client *github.Client, log log.Logger) (string, []string, error) { @@ -1111,36 +1111,37 @@ const ( defaultGroupsClaim = "groups" ) -// getOpenIDClaimMapping resolves the username and groups claim names for a given provider. -// The third return value reports whether the username claim was explicitly configured -// (false means the default "email" claim is being used as a fallback). +// getOpenIDClaimMapping resolves which OIDC claims supply identity (see ClaimMapping.Username) +// and groups for a given provider. +// The third return value reports whether the identity username claim was explicitly configured +// (false means the default "email" claim is being used). func getOpenIDClaimMapping(authConfig *config.AuthConfig, providerName string) (string, string, bool) { - usernameClaim := defaultUsernameClaim + identityClaim := defaultUsernameClaim groupsClaim := defaultGroupsClaim - usernameConfigured := false + identityConfigured := false if authConfig == nil || authConfig.OpenID == nil || providerName == "" { - return usernameClaim, groupsClaim, usernameConfigured + return identityClaim, groupsClaim, identityConfigured } providerConfig, ok := authConfig.OpenID.Providers[providerName] if !ok || providerConfig.ClaimMapping == nil { - return usernameClaim, groupsClaim, usernameConfigured + return identityClaim, groupsClaim, identityConfigured } if providerConfig.ClaimMapping.Username != "" { - usernameClaim = providerConfig.ClaimMapping.Username - usernameConfigured = true + identityClaim = providerConfig.ClaimMapping.Username + identityConfigured = true } if providerConfig.ClaimMapping.Groups != "" { groupsClaim = providerConfig.ClaimMapping.Groups } - return usernameClaim, groupsClaim, usernameConfigured + return identityClaim, groupsClaim, identityConfigured } -func getOpenIDUsername(info *oidc.UserInfo, claimName string) string { +func getOpenIDIdentity(info *oidc.UserInfo, claimName string) string { if info == nil { return "" } @@ -1196,73 +1197,67 @@ func appendOpenIDGroups(groups []string, claims map[string]any, claimName string return groups, false } -// extractOpenIDIdentity resolves the username and groups for an OIDC callback +// extractOpenIDIdentity resolves identity and groups for an OIDC callback // based on the provider's configured claim mapping. It returns the resolved -// username, the deduplicated/sorted groups, and a boolean reporting whether -// the username could be resolved at all (false means callers should reject). +// identity string, the deduplicated/sorted groups, and a boolean reporting whether +// identity could be resolved at all (false means callers should reject). func extractOpenIDIdentity(logger log.Logger, authConfig *config.AuthConfig, providerName string, info *oidc.UserInfo, idTokenClaims map[string]any, ) (string, []string, bool) { - usernameClaim, groupsClaim, usernameConfigured := getOpenIDClaimMapping(authConfig, providerName) + identityClaim, groupsClaim, identityConfigured := getOpenIDClaimMapping(authConfig, providerName) - username := getOpenIDUsername(info, usernameClaim) + identity := getOpenIDIdentity(info, identityClaim) + fellBackToDefaultClaim := false - if username == "" && usernameConfigured { - configuredClaim := usernameClaim - usernameClaim = defaultUsernameClaim - usernameConfigured = false - username = getOpenIDUsername(info, usernameClaim) + if identity == "" && identityConfigured && identityClaim != defaultUsernameClaim { + fellBackToDefaultClaim = true + configuredClaim := identityClaim + identityClaim = defaultUsernameClaim + identity = getOpenIDIdentity(info, identityClaim) logger.Warn(). Str("provider", providerName). Str("claim", configuredClaim). - Msg("configured username claim missing or empty, falling back to email") + Msgf("configured username claim missing or empty, falling back to %q claim", defaultUsernameClaim) } - if username == "" { + if identity == "" { return "", nil, false } - if usernameConfigured { - logger.Debug(). - Str("provider", providerName). - Str("claim", usernameClaim). - Str("username", username). - Msg("extracted username from configured claim") - } else { - logger.Debug(). - Str("provider", providerName). - Str("username", username). - Msg("using email as username (fallback)") - } + logger.Debug(). + Str("provider", providerName). + Str("claim", identityClaim). + Str("identity", identity). + Bool("fellBackToDefaultClaim", fellBackToDefaultClaim). + Msg("extracted identity") - var ( - groups []string - groupsFound bool - ) + var groups []string if info != nil { - groups, groupsFound = appendOpenIDGroups(groups, info.Claims, groupsClaim) - if !groupsFound { - logger.Info().Msgf("failed to find any %q claim for user %s in UserInfo", groupsClaim, username) - } + groups, _ = appendOpenIDGroups(groups, info.Claims, groupsClaim) } if idTokenClaims != nil { - groups, groupsFound = appendOpenIDGroups(groups, idTokenClaims, groupsClaim) - if !groupsFound { - logger.Info().Msgf("failed to find any %q claim for user %s in IDTokenClaimsToken", groupsClaim, username) - } + groups, _ = appendOpenIDGroups(groups, idTokenClaims, groupsClaim) } slices.Sort(groups) groups = slices.Compact(groups) - return username, groups, true + if len(groups) == 0 { + logger.Debug(). + Str("provider", providerName). + Str("groupsClaim", groupsClaim). + Str("identity", identity). + Msg("no groups claim values found in UserInfo or ID token claims") + } + + return identity, groups, true } // OAuth2Callback is the callback logic where openid/oauth2 will redirect back to our app. -func OAuth2Callback(ctlr *Controller, w http.ResponseWriter, r *http.Request, state, email, provider string, +func OAuth2Callback(ctlr *Controller, w http.ResponseWriter, r *http.Request, state, identity, provider string, groups []string, ) (string, error) { stateCookie, _ := ctlr.CookieStore.Get(r, "statecookie") @@ -1283,24 +1278,24 @@ func OAuth2Callback(ctlr *Controller, w http.ResponseWriter, r *http.Request, st } userAc := reqCtx.NewUserAccessControl() - userAc.SetUsername(email) + userAc.SetUsername(identity) userAc.AddGroups(groups) userAc.SaveOnRequest(r) // if this line has been reached, then a new session should be created // if the `session` key is already on the cookie, it's not a valid one secure := ctlr.Config.UseSecureSession() - if err := saveUserLoggedSession(ctlr.CookieStore, w, r, email, provider, secure, ctlr.Log); err != nil { + if err := saveUserLoggedSession(ctlr.CookieStore, w, r, identity, provider, secure, ctlr.Log); err != nil { return "", err } if err := ctlr.MetaDB.SetUserGroups(r.Context(), groups); err != nil { - ctlr.Log.Error().Err(err).Str("identity", email).Msg("failed to update the user profile") + ctlr.Log.Error().Err(err).Str("identity", identity).Msg("failed to update the user profile") return "", err } - ctlr.Log.Info().Msgf("user profile set successfully for email %s", email) + ctlr.Log.Info().Str("identity", identity).Msg("user profile set successfully") // redirect to UI callbackUI, _ := stateCookie.Values["callback"].(string) @@ -1316,7 +1311,7 @@ func hashUUID(uuid string) string { } /* -GetAuthUserFromRequestSession returns identity +GetAuthUserFromRequestSession returns the authenticated user identifier and auth status if on the request's cookie session is a logged in user. */ func GetAuthUserFromRequestSession(cookieStore sessions.Store, request *http.Request, log log.Logger, @@ -1452,7 +1447,7 @@ func extractIdentityFromCertificate(cert *x509.Certificate, identityAttribute st } } -// extractMTLSIdentity extracts identity from certificate using configured soidentity attributes with fallback chain. +// extractMTLSIdentity extracts identity from certificate using configured identity attributes with fallback chain. func extractMTLSIdentity(cert *x509.Certificate, mtlsConfig *config.MTLSConfig) (string, error) { identityAttributes := []string{"CommonName"} // Default if mtlsConfig != nil && len(mtlsConfig.IdentityAttibutes) > 0 { diff --git a/pkg/api/authn_internal_test.go b/pkg/api/authn_internal_test.go index bed91740..7f131bd4 100644 --- a/pkg/api/authn_internal_test.go +++ b/pkg/api/authn_internal_test.go @@ -1,29 +1,32 @@ package api import ( + "slices" "testing" + "github.com/stretchr/testify/assert" "github.com/zitadel/oidc/v3/pkg/oidc" "zotregistry.dev/zot/v2/pkg/api/config" + "zotregistry.dev/zot/v2/pkg/log" ) func TestGetOpenIDClaimMapping(t *testing.T) { t.Parallel() tests := []struct { - name string - authConfig *config.AuthConfig - providerName string - expectedUsername string - expectedGroups string - expectedConfigured bool + name string + authConfig *config.AuthConfig + providerName string + expectedIdentityClaim string + expectedGroups string + expectedIdentityConfigured bool }{ { - name: "nil auth config uses defaults", - expectedUsername: defaultUsernameClaim, - expectedGroups: defaultGroupsClaim, - expectedConfigured: false, + name: "nil auth config uses defaults", + expectedIdentityClaim: defaultUsernameClaim, + expectedGroups: defaultGroupsClaim, + expectedIdentityConfigured: false, }, { name: "empty provider uses defaults", @@ -32,9 +35,9 @@ func TestGetOpenIDClaimMapping(t *testing.T) { Providers: map[string]config.OpenIDProviderConfig{}, }, }, - expectedUsername: defaultUsernameClaim, - expectedGroups: defaultGroupsClaim, - expectedConfigured: false, + expectedIdentityClaim: defaultUsernameClaim, + expectedGroups: defaultGroupsClaim, + expectedIdentityConfigured: false, }, { name: "missing provider uses defaults", @@ -43,10 +46,10 @@ func TestGetOpenIDClaimMapping(t *testing.T) { Providers: map[string]config.OpenIDProviderConfig{}, }, }, - providerName: "oidc", - expectedUsername: defaultUsernameClaim, - expectedGroups: defaultGroupsClaim, - expectedConfigured: false, + providerName: "oidc", + expectedIdentityClaim: defaultUsernameClaim, + expectedGroups: defaultGroupsClaim, + expectedIdentityConfigured: false, }, { name: "provider without claim mapping uses defaults", @@ -57,10 +60,10 @@ func TestGetOpenIDClaimMapping(t *testing.T) { }, }, }, - providerName: "oidc", - expectedUsername: defaultUsernameClaim, - expectedGroups: defaultGroupsClaim, - expectedConfigured: false, + providerName: "oidc", + expectedIdentityClaim: defaultUsernameClaim, + expectedGroups: defaultGroupsClaim, + expectedIdentityConfigured: false, }, { name: "custom username and groups claims", @@ -76,10 +79,10 @@ func TestGetOpenIDClaimMapping(t *testing.T) { }, }, }, - providerName: "oidc", - expectedUsername: "preferred_username", - expectedGroups: "roles", - expectedConfigured: true, + providerName: "oidc", + expectedIdentityClaim: "preferred_username", + expectedGroups: "roles", + expectedIdentityConfigured: true, }, { name: "custom groups keeps default username", @@ -94,10 +97,10 @@ func TestGetOpenIDClaimMapping(t *testing.T) { }, }, }, - providerName: "oidc", - expectedUsername: defaultUsernameClaim, - expectedGroups: "roles", - expectedConfigured: false, + providerName: "oidc", + expectedIdentityClaim: defaultUsernameClaim, + expectedGroups: "roles", + expectedIdentityConfigured: false, }, } @@ -107,23 +110,91 @@ func TestGetOpenIDClaimMapping(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() - usernameClaim, groupsClaim, usernameConfigured := getOpenIDClaimMapping(test.authConfig, test.providerName) - if usernameClaim != test.expectedUsername { - t.Fatalf("expected username claim %q, got %q", test.expectedUsername, usernameClaim) - } - - if groupsClaim != test.expectedGroups { - t.Fatalf("expected groups claim %q, got %q", test.expectedGroups, groupsClaim) - } - - if usernameConfigured != test.expectedConfigured { - t.Fatalf("expected usernameConfigured %t, got %t", test.expectedConfigured, usernameConfigured) - } + identityClaim, groupsClaim, identityConfigured := getOpenIDClaimMapping(test.authConfig, test.providerName) + assert.Equal(t, test.expectedIdentityClaim, identityClaim) + assert.Equal(t, test.expectedGroups, groupsClaim) + assert.Equal(t, test.expectedIdentityConfigured, identityConfigured) }) } + + logger := log.NewTestLogger() + + t.Run("extractOpenIDIdentity_fallbackToEmailWhenConfiguredNonDefaultClaimMissing", func(t *testing.T) { + t.Parallel() + + authConfig := &config.AuthConfig{ + OpenID: &config.OpenIDConfig{ + Providers: map[string]config.OpenIDProviderConfig{ + "oidc": { + ClaimMapping: &config.ClaimMapping{ + Username: "preferred_username", + }, + }, + }, + }, + } + + info := &oidc.UserInfo{ + UserInfoProfile: oidc.UserInfoProfile{ + PreferredUsername: "", + }, + UserInfoEmail: oidc.UserInfoEmail{Email: "user@example.com"}, + } + + identity, groups, ok := extractOpenIDIdentity(logger, authConfig, "oidc", info, nil) + assert.True(t, ok) + assert.Equal(t, "user@example.com", identity) + assert.Empty(t, groups) + }) + + t.Run("extractOpenIDIdentity_explicitDefaultClaimMissingRejects", func(t *testing.T) { + t.Parallel() + + authConfig := &config.AuthConfig{ + OpenID: &config.OpenIDConfig{ + Providers: map[string]config.OpenIDProviderConfig{ + "oidc": { + ClaimMapping: &config.ClaimMapping{ + Username: "email", + }, + }, + }, + }, + } + + info := &oidc.UserInfo{ + UserInfoEmail: oidc.UserInfoEmail{Email: ""}, + } + + identity, groups, ok := extractOpenIDIdentity(logger, authConfig, "oidc", info, nil) + assert.False(t, ok) + assert.Empty(t, identity) + assert.Nil(t, groups) + }) + + t.Run("extractOpenIDIdentity_mergesSortsDedupesGroupsFromUserInfoAndIDToken", func(t *testing.T) { + t.Parallel() + + info := &oidc.UserInfo{ + UserInfoEmail: oidc.UserInfoEmail{Email: "user@example.com"}, + Claims: map[string]any{ + "groups": []any{"b", "a", "", nil, "a"}, + }, + } + + idTokenClaims := map[string]any{ + "groups": []string{"c", "b"}, + } + + identity, groups, ok := extractOpenIDIdentity(logger, nil, "", info, idTokenClaims) + assert.True(t, ok) + assert.Equal(t, "user@example.com", identity) + expected := []string{"a", "b", "c"} + assert.True(t, slices.Equal(expected, groups)) + }) } -func TestGetOpenIDUsername(t *testing.T) { +func TestGetOpenIDIdentity(t *testing.T) { t.Parallel() info := &oidc.UserInfo{ @@ -161,10 +232,8 @@ func TestGetOpenIDUsername(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() - username := getOpenIDUsername(test.info, test.claim) - if username != test.expected { - t.Fatalf("expected username %q, got %q", test.expected, username) - } + identity := getOpenIDIdentity(test.info, test.claim) + assert.Equal(t, test.expected, identity) }) } } @@ -260,19 +329,8 @@ func TestAppendOpenIDGroups(t *testing.T) { t.Parallel() groups, found := appendOpenIDGroups(test.groups, test.claims, test.claim) - if found != test.expectedFound { - t.Fatalf("expected found %t, got %t", test.expectedFound, found) - } - - if len(groups) != len(test.expected) { - t.Fatalf("expected groups %v, got %v", test.expected, groups) - } - - for i := range test.expected { - if groups[i] != test.expected[i] { - t.Fatalf("expected groups %v, got %v", test.expected, groups) - } - } + assert.Equal(t, test.expectedFound, found) + assert.Equal(t, test.expected, groups) }) } } diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index adeb6e30..1ad7190d 100644 --- a/pkg/api/config/config.go +++ b/pkg/api/config/config.go @@ -315,8 +315,8 @@ type OpenIDProviderConfig struct { ClaimMapping *ClaimMapping `mapstructure:",omitempty"` } -// ClaimMapping specifies how OpenID claims are mapped to application fields. -// It allows customization of which claim is used as the username when authenticating users. +// ClaimMapping specifies how OpenID claims are mapped to Zot identities: +// which claim supplies the username and which claim supplies group membership. type ClaimMapping struct { // Username specifies which OpenID claim to use as the username for the authenticated user. // Acceptable values include "preferred_username", "email", "sub", "name", or any custom claim name. diff --git a/test/blackbox/openid_claim_mapping.bats b/test/blackbox/openid_claim_mapping.bats index 728e263e..49f27cc2 100755 --- a/test/blackbox/openid_claim_mapping.bats +++ b/test/blackbox/openid_claim_mapping.bats @@ -25,6 +25,17 @@ function verify_prerequisites { return 0 } +function assert_oidc_extract_identity_log() { + local log_file=${1:?} + local expect_fell_back=${2:?} + local expect_claim=${3:?} + + grep '"message":"extracted identity"' "${log_file}" \ + | grep "\"claim\":\"${expect_claim}\"" \ + | grep "\"fellBackToDefaultClaim\":${expect_fell_back}" \ + | grep -q . +} + IDP_PID="" function setup_file() { @@ -173,9 +184,8 @@ EOF [ "$status" -eq 0 ] echo "$output" - # Verify that the log contains the preferred_username claim being used - log_output ${BATS_FILE_TMPDIR}/zot-preferred-username.log | jq 'contains("extracted username from configured claim")' | grep true - ! log_output ${BATS_FILE_TMPDIR}/zot-preferred-username.log | grep -q "using email as username (fallback)" + # Verify structured extract identity debug line (preferred_username mapped) + assert_oidc_extract_identity_log ${BATS_FILE_TMPDIR}/zot-preferred-username.log false preferred_username } @test "test OIDC claim mapping with email" { @@ -249,9 +259,8 @@ EOF [ "$status" -eq 0 ] echo "$output" - # Verify that the log contains email claim being used - grep -q "extracted username from configured claim" ${BATS_FILE_TMPDIR}/zot-email.log - ! grep -q "using email as username (fallback)" ${BATS_FILE_TMPDIR}/zot-email.log + # Verify structured extract identity debug line (explicit email username claim) + assert_oidc_extract_identity_log ${BATS_FILE_TMPDIR}/zot-email.log false email } @test "test OIDC claim mapping with sub" { @@ -319,9 +328,8 @@ EOF [ "$status" -eq 0 ] echo "$output" - # Verify that the log contains sub claim being used - log_output ${BATS_FILE_TMPDIR}/zot-sub.log | jq 'contains("extracted username from configured claim")' | grep true - ! log_output ${BATS_FILE_TMPDIR}/zot-sub.log | grep -q "using email as username (fallback)" + # Verify structured extract identity debug line (sub mapped) + assert_oidc_extract_identity_log ${BATS_FILE_TMPDIR}/zot-sub.log false sub } @test "test OIDC with no claim mapping (default to email)" { @@ -394,6 +402,6 @@ EOF [ "$status" -eq 0 ] echo "$output" - # Verify that the log contains email claim being used (default behavior) - log_output ${ZOT_LOG_FILE} | jq 'contains("using email as username (fallback)")' | grep true + # Verify structured extract identity debug line (default email claim, no claimMapping) + assert_oidc_extract_identity_log ${ZOT_LOG_FILE} false email }