From 8a97de791f620681985b09a4074ebc41366b2ba3 Mon Sep 17 00:00:00 2001 From: Akanksha Gawai <280726545+agawai@users.noreply.github.com> Date: Thu, 27 Aug 2026 00:42:24 +0530 Subject: [PATCH] OCPBUGS-56908: sanitize IdP names used in group sync annotations Kubernetes annotation keys cannot contain spaces. Identity provider names like "AIF - Keycloak" are otherwise legal, but writing oauth.openshift.io/idp. on Group objects fails API validation and blocks login when OpenID groups claims are enabled. Sanitize the IdP name used in the annotation key so group sync succeeds without changing the OAuth CR name. Signed-off-by: Akanksha Gawai <280726545+agawai@users.noreply.github.com> --- pkg/groupmapper/groupmapper.go | 70 +++++++++++++- pkg/groupmapper/groupmapper_test.go | 136 +++++++++++++++++++++++++++- 2 files changed, 196 insertions(+), 10 deletions(-) diff --git a/pkg/groupmapper/groupmapper.go b/pkg/groupmapper/groupmapper.go index 185de2be0..ddfa33eec 100644 --- a/pkg/groupmapper/groupmapper.go +++ b/pkg/groupmapper/groupmapper.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "slices" + "strings" "time" "k8s.io/apimachinery/pkg/api/errors" @@ -24,6 +25,10 @@ import ( const ( groupGeneratedKey = "oauth.openshift.io/generated" groupSyncedKeyFmt = "oauth.openshift.io/idp.%s" + + // Kubernetes annotation name parts are limited to 63 characters. The + // "idp." prefix of groupSyncedKeyFmt consumes 4 of those. + maxIDPAnnotationNameLen = 63 - len("idp.") ) var _ authapi.UserIdentityMapper = &UserGroupsMapper{} @@ -138,7 +143,7 @@ func (m *UserGroupsMapper) removeUserFromGroup(idpName, username, group string) } // don't perform any actions on the group if it hasn't been synced for this IdP - if updatedGroup.Annotations[fmt.Sprintf(groupSyncedKeyFmt, idpName)] != "synced" { + if updatedGroup.Annotations[idpAnnotationKey(idpName)] != "synced" { return nil } @@ -167,8 +172,8 @@ func (m *UserGroupsMapper) addUserToGroup(idpName, username, group string) error ObjectMeta: metav1.ObjectMeta{ Name: group, Annotations: map[string]string{ - fmt.Sprintf(groupSyncedKeyFmt, idpName): "synced", - groupGeneratedKey: "true", + idpAnnotationKey(idpName): "synced", + groupGeneratedKey: "true", }, }, Users: []string{username}, @@ -188,7 +193,7 @@ func (m *UserGroupsMapper) addUserToGroup(idpName, username, group string) error var onlyAddAnnotation bool for _, u := range updatedGroup.Users { if u == username { - if updatedGroup.Annotations[fmt.Sprintf(groupSyncedKeyFmt, idpName)] != "synced" { + if updatedGroup.Annotations[idpAnnotationKey(idpName)] != "synced" { onlyAddAnnotation = true break } @@ -200,7 +205,7 @@ func (m *UserGroupsMapper) addUserToGroup(idpName, username, group string) error if !onlyAddAnnotation { updatedGroupCopy.Users = append(updatedGroupCopy.Users, username) } - updatedGroupCopy.Annotations[fmt.Sprintf(groupSyncedKeyFmt, idpName)] = "synced" + updatedGroupCopy.Annotations[idpAnnotationKey(idpName)] = "synced" _, err = m.groupsClient.Update(context.TODO(), updatedGroupCopy, metav1.UpdateOptions{}) return err @@ -214,3 +219,58 @@ func groupsDiff(existing []*userv1.Group, required sets.String) (toRemove, toAdd return existingNames.Difference(required).UnsortedList(), required.Difference(existingNames).UnsortedList() } + +// idpAnnotationKey returns the Group annotation used to record that a group +// was synchronized from the named identity provider. +// +// The IdP name is sanitized because Kubernetes annotation keys cannot contain +// spaces or other characters that are otherwise legal in identity provider +// names. Login paths already tolerate those names (OCPBUGS-42772, OCPBUGS-44099); +// group sync must as well (OCPBUGS-56908). +func idpAnnotationKey(idpName string) string { + return fmt.Sprintf(groupSyncedKeyFmt, sanitizeIDPNameForAnnotation(idpName)) +} + +// sanitizeIDPNameForAnnotation rewrites an identity provider name so it is +// valid as the name part of a Kubernetes annotation key (alphanumeric, '-', +// '_', '.', must start and end with alphanumeric). Invalid characters are +// replaced with '-' rather than stripped so distinct names stay distinct +// ("AIF - Keycloak" vs "AIF-Keycloak"). +func sanitizeIDPNameForAnnotation(name string) string { + if name == "" { + return "unknown" + } + + var b strings.Builder + b.Grow(len(name)) + for _, r := range name { + if isAnnotationNameRune(r) { + b.WriteRune(r) + } else { + b.WriteByte('-') + } + } + + sanitized := strings.Trim(b.String(), "-_.") + if sanitized == "" { + return "unknown" + } + if len(sanitized) > maxIDPAnnotationNameLen { + sanitized = strings.TrimRight(sanitized[:maxIDPAnnotationNameLen], "-_.") + if sanitized == "" { + return "unknown" + } + } + return sanitized +} + +func isAnnotationNameRune(r rune) bool { + switch { + case r >= 'A' && r <= 'Z', r >= 'a' && r <= 'z', r >= '0' && r <= '9': + return true + case r == '-' || r == '_' || r == '.': + return true + default: + return false + } +} diff --git a/pkg/groupmapper/groupmapper_test.go b/pkg/groupmapper/groupmapper_test.go index 3043366a4..8df4bec9d 100644 --- a/pkg/groupmapper/groupmapper_test.go +++ b/pkg/groupmapper/groupmapper_test.go @@ -5,6 +5,7 @@ import ( "fmt" "reflect" "slices" + "strings" "sync" "testing" "time" @@ -13,10 +14,14 @@ import ( "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" + apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/watch" kuser "k8s.io/apiserver/pkg/authentication/user" k8stesting "k8s.io/client-go/testing" @@ -305,6 +310,127 @@ func TestUserGroupsMapper_removeUserFromGroup(t *testing.T) { } } +// prependGroupAnnotationValidation makes the fake client reject Groups whose +// annotation keys would be rejected by the real Kubernetes API. This is the +// validation that fails login when an IdP name contains spaces +// (OCPBUGS-56908). +func prependGroupAnnotationValidation(fake *fakeuserclient.Clientset) { + validate := func(obj runtime.Object) error { + group, ok := obj.(*userv1.Group) + if !ok { + return nil + } + if errs := apivalidation.ValidateAnnotations(group.Annotations, field.NewPath("metadata", "annotations")); len(errs) > 0 { + return apierrors.NewInvalid(schema.GroupKind{Group: userv1.GroupName, Kind: "Group"}, group.Name, errs) + } + return nil + } + fake.PrependReactor("create", "groups", func(action k8stesting.Action) (bool, runtime.Object, error) { + if err := validate(action.(k8stesting.CreateAction).GetObject()); err != nil { + return true, nil, err + } + return false, nil, nil + }) + fake.PrependReactor("update", "groups", func(action k8stesting.Action) (bool, runtime.Object, error) { + if err := validate(action.(k8stesting.UpdateAction).GetObject()); err != nil { + return true, nil, err + } + return false, nil, nil + }) +} + +func TestSanitizeIDPNameForAnnotation(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {name: "already valid", in: "AIF-Keycloak", want: "AIF-Keycloak"}, + {name: "customer name with spaces", in: "AIF - Keycloak", want: "AIF---Keycloak"}, + {name: "Microsoft Entra ID", in: "Microsoft Entra ID", want: "Microsoft-Entra-ID"}, + {name: "punctuation", in: "my idp #2?", want: "my-idp--2"}, + {name: "leading and trailing junk", in: " ??foo?? ", want: "foo"}, + {name: "only illegal characters", in: " ??? ", want: "unknown"}, + {name: "empty", in: "", want: "unknown"}, + {name: "dots and underscores kept", in: "my.idp_name", want: "my.idp_name"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, sanitizeIDPNameForAnnotation(tt.in)) + key := idpAnnotationKey(tt.in) + require.Emptyf(t, validation.IsQualifiedName(strings.ToLower(key)), "annotation key %q from IdP name %q is invalid", key, tt.in) + }) + } + + t.Run("long name is truncated to annotation limit", func(t *testing.T) { + in := strings.Repeat("a", 80) + got := sanitizeIDPNameForAnnotation(in) + require.Equal(t, maxIDPAnnotationNameLen, len(got)) + require.Empty(t, validation.IsQualifiedName(strings.ToLower(idpAnnotationKey(in)))) + }) +} + +func TestAddUserToGroup_IdPNameWithInvalidAnnotationChars(t *testing.T) { + const testGroupName = "AIF-DEV" + + tests := []struct { + name string + idpName string + }{ + { + name: "spaces around hyphen like customer AIF - Keycloak", + idpName: "AIF - Keycloak", + }, + { + name: "spaces in Microsoft Entra ID from original bug", + idpName: "Microsoft Entra ID", + }, + { + name: "punctuation from previously allowed IdP names", + idpName: "my idp #2?", + }, + { + name: "already valid name is unchanged", + idpName: "AIF-Keycloak", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeUserClient := fakeuserclient.NewSimpleClientset() + prependGroupAnnotationValidation(fakeUserClient) + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + + m := &UserGroupsMapper{ + groupsLister: userlisterv1.NewGroupLister(indexer), + groupsClient: fakeUserClient.UserV1().Groups(), + } + + err := m.addUserToGroup(tt.idpName, "user1", testGroupName) + require.NoError(t, err, "group sync must succeed for IdP name %q", tt.idpName) + + got, err := fakeUserClient.UserV1().Groups().Get(context.Background(), testGroupName, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, []string{"user1"}, []string(got.Users)) + require.Equal(t, "true", got.Annotations[groupGeneratedKey]) + + for key := range got.Annotations { + require.Emptyf(t, validation.IsQualifiedName(strings.ToLower(key)), "annotation key %q is not a valid Kubernetes qualified name", key) + } + + syncedKey := idpAnnotationKey(tt.idpName) + require.Equal(t, "synced", got.Annotations[syncedKey]) + + // A later login must still recognize this IdP's sync annotation so + // membership can be removed when the user leaves the group. + require.NoError(t, indexer.Add(got)) + require.NoError(t, m.removeUserFromGroup(tt.idpName, "user1", testGroupName)) + _, err = fakeUserClient.UserV1().Groups().Get(context.Background(), testGroupName, metav1.GetOptions{}) + require.True(t, apierrors.IsNotFound(err), "generated group should be deleted when last user is removed, got %v", err) + }) + } +} + func TestUserGroupsMapper_addUserToGroup(t *testing.T) { const testGroupName = "test-group" @@ -414,8 +540,8 @@ func createGroupWithUsers(groupname string, users ...string) *userv1.Group { ObjectMeta: metav1.ObjectMeta{ Name: groupname, Annotations: map[string]string{ - fmt.Sprintf(groupSyncedKeyFmt, testIDPName): "synced", - groupGeneratedKey: "true", + idpAnnotationKey(testIDPName): "synced", + groupGeneratedKey: "true", }, }, Users: users, @@ -428,7 +554,7 @@ func removeGeneratedKeyFromGroup(g *userv1.Group) *userv1.Group { } func removeSyncedKeyFromGroup(g *userv1.Group, idpName string) *userv1.Group { - delete(g.Annotations, fmt.Sprintf(groupSyncedKeyFmt, idpName)) + delete(g.Annotations, idpAnnotationKey(idpName)) return g } @@ -591,8 +717,8 @@ func TestAddUserToGroup_DoesNotMutateCachedObject(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: testGroupName, Annotations: map[string]string{ - fmt.Sprintf(groupSyncedKeyFmt, testIDPName): "synced", - groupGeneratedKey: "true", + idpAnnotationKey(testIDPName): "synced", + groupGeneratedKey: "true", }, }, Users: users,