Skip to content

OCPBUGS-56908: validate identity provider names as annotation keys #3025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
agawai wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-56908: validate identity provider names as annotation keys #3025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 179 additions & 0 deletions config/v1/tests/oauths.config.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,182 @@ tests:
apiVersion: config.openshift.io/v1
kind: OAuth
spec: {}
- name: Should be able to create an identity provider with a valid annotation-key name

@everettraven everettraven Sep 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also want to test that the change to the validation successfully ratchets. The tests in

api/example/v1/tests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml

Lines 902 to 964 in 8127870

- name: Should allow changing other fields when a persisted value is no longer valid
initialCRDPatches:
- op: remove
path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/nonZeroDefault/minimum # Minimum is normally 8
initial: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 1
immutableField: foo
updated: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 1
immutableField: foo
optionalImmutableField: foo
expected: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 1
immutableField: foo
optionalImmutableField: foo
- name: Should allow updating a persisted value that is no longer valid to a valid value
initialCRDPatches:
- op: remove
path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/nonZeroDefault/minimum # Minimum is normally 8
initial: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 1
immutableField: foo
updated: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 8
immutableField: foo
expected: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 8
immutableField: foo
- name: Should not allow updating a persisted value that is no longer valid to a still invalid value
initialCRDPatches:
- op: remove
path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/nonZeroDefault/minimum # Minimum is normally 8
initial: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 1
immutableField: foo
updated: |
apiVersion: example.openshift.io/v1
kind: StableConfigType
spec:
nonZeroDefault: 2
immutableField: foo
expectedError: "spec.nonZeroDefault: Invalid value: 2: spec.nonZeroDefault in body should be greater than or equal to 8"
are a good example of how to do that ratcheting testing.

initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
expected: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
- name: Should be able to create an identity provider named AIF-Keycloak
initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: AIF-Keycloak
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
expected: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: AIF-Keycloak
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
- name: Should not be able to create an identity provider whose name contains spaces
initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
metadata:
name: cluster
spec:
identityProviders:
- name: Microsoft Entra ID
mappingMethod: claim
type: OpenID
openID:
clientID: example
clientSecret:
name: microsoft-entra-id-secret
issuer: https://login.microsoftonline.com/example/v2.0
expectedError: "identity provider name must consist of alphanumeric characters"
- name: Should not be able to create an identity provider named AIF - Keycloak
initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
metadata:
name: cluster
spec:
identityProviders:
- name: AIF - Keycloak
mappingMethod: claim
type: OpenID
openID:
clientID: example
clientSecret:
name: aif-keycloak-secret
issuer: https://keycloak.example.com/realms/example
expectedError: "identity provider name must consist of alphanumeric characters"
onUpdate:
- name: Should allow updating other fields of a valid identity provider
initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
updated: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret-updated
expected: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret-updated
- name: Should not be able to add a new identity provider whose name contains spaces
initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: htpasswd
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
updated: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: htpasswd
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
- name: AIF - Keycloak
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
expectedError: "identity provider name must consist of alphanumeric characters"
- name: Should allow renaming an identity provider to another valid annotation-key name
initial: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: MicrosoftEntraID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
updated: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
expected: |
apiVersion: config.openshift.io/v1
kind: OAuth
spec:
identityProviders:
- name: Microsoft-Entra-ID
mappingMethod: claim
type: HTPasswd
htpasswd:
fileData:
name: htpass-secret
10 changes: 10 additions & 0 deletions config/v1/types_oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ type OAuth struct {
type OAuthSpec struct {
// identityProviders is an ordered list of ways for a user to identify themselves.
// When this list is empty, no identities are provisioned for users.
//
// name is also used as the name-part of the Group annotation
// oauth.openshift.io/idp.<name> during OpenID group sync, so it must be a valid
// Kubernetes annotation name (alphanumeric, '-', '_', '.', at most 59 characters).
// Existing names that predate this restriction are grandfathered until changed.
//
// +kubebuilder:validation:XValidation:rule="self.all(idp, (idp.name.matches('^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$') && size(idp.name) <= 59) || (oldSelf.hasValue() && oldSelf.value().exists(old, old.name == idp.name)))",message="identity provider name must consist of alphanumeric characters, '-', '_' or '.', start and end with an alphanumeric character, and be at most 59 characters because it is used as a Kubernetes annotation key during group sync",optionalOldSelf=true
Comment on lines +42 to +47

@everettraven everettraven Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this being a validation on this field, we can probably just add equivalent validation on the actual Name field within the IdentityProvider type.

// +optional
// +listType=atomic
IdentityProviders []IdentityProvider `json:"identityProviders,omitempty"`
Expand Down Expand Up @@ -139,6 +146,9 @@ type IdentityProvider struct {
// - It MUST be unique and not shared by any other identity provider used
// - It MUST be a valid path segment: name cannot equal "." or ".." or contain "/" or "%" or ":"
// Ref: https://godoc.org/github.com/openshift/origin/pkg/user/apis/user/validation#ValidateIdentityProviderName
// - It MUST also be a valid Kubernetes annotation name-part (alphanumeric characters,
// '-', '_' or '.', starting and ending with an alphanumeric character, at most 59 characters)
// because oauth-server writes oauth.openshift.io/idp.<name> onto Group objects during group sync.
Name string `json:"name"`

// mappingMethod determines how identities from this provider are mapped to users
Expand Down
17 changes: 17 additions & 0 deletions config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_oauths.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ spec:
description: |-
identityProviders is an ordered list of ways for a user to identify themselves.
When this list is empty, no identities are provisioned for users.

name is also used as the name-part of the Group annotation
oauth.openshift.io/idp.<name> during OpenID group sync, so it must be a valid
Kubernetes annotation name (alphanumeric, '-', '_', '.', at most 59 characters).
Existing names that predate this restriction are grandfathered until changed.
items:
description: IdentityProvider provides identities for users authenticating
using credentials
Expand Down Expand Up @@ -418,6 +423,9 @@ spec:
- It MUST be unique and not shared by any other identity provider used
- It MUST be a valid path segment: name cannot equal "." or ".." or contain "/" or "%" or ":"
Ref: https://godoc.org/github.com/openshift/origin/pkg/user/apis/user/validation#ValidateIdentityProviderName
- It MUST also be a valid Kubernetes annotation name-part (alphanumeric characters,
'-', '_' or '.', starting and ending with an alphanumeric character, at most 59 characters)
because oauth-server writes oauth.openshift.io/idp.<name> onto Group objects during group sync.
type: string
openID:
description: openID enables user authentication using OpenID
Expand Down Expand Up @@ -597,6 +605,15 @@ spec:
type: object
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: identity provider name must consist of alphanumeric characters,
'-', '_' or '.', start and end with an alphanumeric character,
and be at most 59 characters because it is used as a Kubernetes
annotation key during group sync
optionalOldSelf: true
rule: self.all(idp, (idp.name.matches('^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$')
&& size(idp.name) <= 59) || (oldSelf.hasValue() && oldSelf.value().exists(old,
old.name == idp.name)))
templates:
description: templates allow you to customize pages like the login
page.
Expand Down
17 changes: 17 additions & 0 deletions ...ig/v1/zz_generated.featuregated-crd-manifests/oauths.config.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ spec:
description: |-
identityProviders is an ordered list of ways for a user to identify themselves.
When this list is empty, no identities are provisioned for users.

name is also used as the name-part of the Group annotation
oauth.openshift.io/idp.<name> during OpenID group sync, so it must be a valid
Kubernetes annotation name (alphanumeric, '-', '_', '.', at most 59 characters).
Existing names that predate this restriction are grandfathered until changed.
items:
description: IdentityProvider provides identities for users authenticating
using credentials
Expand Down Expand Up @@ -419,6 +424,9 @@ spec:
- It MUST be unique and not shared by any other identity provider used
- It MUST be a valid path segment: name cannot equal "." or ".." or contain "/" or "%" or ":"
Ref: https://godoc.org/github.com/openshift/origin/pkg/user/apis/user/validation#ValidateIdentityProviderName
- It MUST also be a valid Kubernetes annotation name-part (alphanumeric characters,
'-', '_' or '.', starting and ending with an alphanumeric character, at most 59 characters)
because oauth-server writes oauth.openshift.io/idp.<name> onto Group objects during group sync.
type: string
openID:
description: openID enables user authentication using OpenID
Expand Down Expand Up @@ -598,6 +606,15 @@ spec:
type: object
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: identity provider name must consist of alphanumeric characters,
'-', '_' or '.', start and end with an alphanumeric character,
and be at most 59 characters because it is used as a Kubernetes
annotation key during group sync
optionalOldSelf: true
rule: self.all(idp, (idp.name.matches('^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$')
&& size(idp.name) <= 59) || (oldSelf.hasValue() && oldSelf.value().exists(old,
old.name == idp.name)))
templates:
description: templates allow you to customize pages like the login
page.
Expand Down
4 changes: 2 additions & 2 deletions config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions payload-manifests/crds/0000_10_config-operator_01_oauths.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ spec:
description: |-
identityProviders is an ordered list of ways for a user to identify themselves.
When this list is empty, no identities are provisioned for users.

name is also used as the name-part of the Group annotation
oauth.openshift.io/idp.<name> during OpenID group sync, so it must be a valid
Kubernetes annotation name (alphanumeric, '-', '_', '.', at most 59 characters).
Existing names that predate this restriction are grandfathered until changed.
items:
description: IdentityProvider provides identities for users authenticating
using credentials
Expand Down Expand Up @@ -418,6 +423,9 @@ spec:
- It MUST be unique and not shared by any other identity provider used
- It MUST be a valid path segment: name cannot equal "." or ".." or contain "/" or "%" or ":"
Ref: https://godoc.org/github.com/openshift/origin/pkg/user/apis/user/validation#ValidateIdentityProviderName
- It MUST also be a valid Kubernetes annotation name-part (alphanumeric characters,
'-', '_' or '.', starting and ending with an alphanumeric character, at most 59 characters)
because oauth-server writes oauth.openshift.io/idp.<name> onto Group objects during group sync.
type: string
openID:
description: openID enables user authentication using OpenID
Expand Down Expand Up @@ -597,6 +605,15 @@ spec:
type: object
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: identity provider name must consist of alphanumeric characters,
'-', '_' or '.', start and end with an alphanumeric character,
and be at most 59 characters because it is used as a Kubernetes
annotation key during group sync
optionalOldSelf: true
rule: self.all(idp, (idp.name.matches('^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$')
&& size(idp.name) <= 59) || (oldSelf.hasValue() && oldSelf.value().exists(old,
old.name == idp.name)))
templates:
description: templates allow you to customize pages like the login
page.
Expand Down