Skip to content

CNTRLPLANE-311: adding auth config missing fields to API #2487

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
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

CNTRLPLANE-311: adding auth config missing fields to API #2487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9da6c1f
adding auth config missing fields
ShazaAldawamneh Sep 19, 2025
8e0ef94
adding auth config missing fields
ShazaAldawamneh Sep 23, 2025
1fa3639
fixing tests
ShazaAldawamneh Sep 26, 2025
e12527d
fixing tests
ShazaAldawamneh Sep 29, 2025
582db23
fixing tests
ShazaAldawamneh Oct 1, 2025
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
589 changes: 589 additions & 0 deletions config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml
View file
Open in desktop

Large diffs are not rendered by default.

150 changes: 130 additions & 20 deletions config/v1/types_authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
// +genclient
// +genclient:nonNamespaced
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings,rule="!has(self.spec.oidcProviders) || self.spec.oidcProviders.all(p, !has(p.oidcClients) || p.oidcClients.all(specC, self.status.oidcClients.exists(statusC, statusC.componentNamespace == specC.componentNamespace && statusC.componentName == specC.componentName) || (has(oldSelf.spec.oidcProviders) && oldSelf.spec.oidcProviders.exists(oldP, oldP.name == p.name && has(oldP.oidcClients) && oldP.oidcClients.exists(oldC, oldC.componentNamespace == specC.componentNamespace && oldC.componentName == specC.componentName)))))",message="all oidcClients in the oidcProviders must match their componentName and componentNamespace to either a previously configured oidcClient or they must exist in the status.oidcClients"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings;ExternalOIDCWithUpstreamParity,rule="!has(self.spec.oidcProviders) || self.spec.oidcProviders.all(p, !has(p.oidcClients) || p.oidcClients.all(specC, self.status.oidcClients.exists(statusC, statusC.componentNamespace == specC.componentNamespace && statusC.componentName == specC.componentName) || (has(oldSelf.spec.oidcProviders) && oldSelf.spec.oidcProviders.exists(oldP, oldP.name == p.name && has(oldP.oidcClients) && oldP.oidcClients.exists(oldC, oldC.componentNamespace == specC.componentNamespace && oldC.componentName == specC.componentName)))))",message="all oidcClients in the oidcProviders must match their componentName and componentNamespace to either a previously configured oidcClient or they must exist in the status.oidcClients"

// Authentication specifies cluster-wide settings for authentication (like OAuth and
// webhook token authenticators). The canonical name of an instance is `cluster`.
Expand Down Expand Up @@ -91,6 +91,7 @@ type AuthenticationSpec struct {
// +kubebuilder:validation:MaxItems=1
// +openshift:enable:FeatureGate=ExternalOIDC
// +openshift:enable:FeatureGate=ExternalOIDCWithUIDAndExtraClaimMappings
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
// +optional
OIDCProviders []OIDCProvider `json:"oidcProviders,omitempty"`
}
Expand Down Expand Up @@ -243,11 +244,22 @@ type OIDCProvider struct {
// +listType=atomic
// +optional
ClaimValidationRules []TokenClaimValidationRule `json:"claimValidationRules,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we are trying to achieve upstream parity, I noticed that the claim validation rules have some additional constraints we should be enforcing.

Specifically that there should not be duplicate entries for required claim validations and expression validations.

See usage of https://github.com/kubernetes/kubernetes/blob/ef95e1fd7e1248858b426f30ac23a3b28eb3d6c9/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go#L262-L263


// userValidationRules defines the set of rules used to validate claims in a user’s token.
// These rules determine whether a token subject is considered valid based on its claims.
// Each rule is evaluated independently.
// See the TokenUserValidationRule type for more information on rule structure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Users will not know what this type is.

// +listType=atomic
// +kubebuilder:validation:MaxItems=64
// +optional
Comment on lines +253 to +254
Copy link
Contributor

Choose a reason for hiding this comment

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

Include this information in the GoDoc please.

// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
UserValidationRules []TokenUserValidationRule `json:"userValidationRules,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What other constraints are there? Looking at the upstream validations, it looks like there is a uniqueness constraint on the CEL expression, we should enforce this as well.

}

// +kubebuilder:validation:MinLength=1
type TokenAudience string

// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="self.?discoveryURL.orValue(\"\").size() > 0 ? (self.issuerURL.size() == 0 || self.discoveryURL.find('^.+[^/]') != self.issuerURL.find('^.+[^/]')) : true",message="discoveryURL must be different from issuerURL"
type TokenIssuer struct {
// issuerURL is a required field that configures the URL used to issue tokens
// by the identity provider.
Expand Down Expand Up @@ -291,8 +303,46 @@ type TokenIssuer struct {
//
// +optional
CertificateAuthority ConfigMapNameReference `json:"issuerCertificateAuthority"`
// discoveryURL is an optional field that, if specified, overrides the default discovery endpoint
// used to retrieve OIDC configuration metadata. By default, the discovery URL is derived from `url`
// as "{url}/.well-known/openid-configuration".
//
// The discoveryURL must:
// - Be a valid absolute URL.
// - Use the HTTPS scheme.
// - Not contain query parameters, user info, or fragments.
// - Be different from the value of `url` (ignoring trailing slashes)
//
// +optional
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
// +kubebuilder:validation:XValidation:rule="self.size() > 0 ? isURL(self) : true",message="discoveryURL must be a valid URL"
// +kubebuilder:validation:XValidation:rule="self.size() > 0 ? (isURL(self) && url(self).getScheme() == 'https') : true",message="discoveryURL must be a valid https URL"
// +kubebuilder:validation:XValidation:rule="self.matches('^[^?]*$')",message="discoveryURL must not contain query parameters"
// +kubebuilder:validation:XValidation:rule="self.matches('^[^#]*$')",message="discoveryURL must not contain fragments"
// +kubebuilder:validation:XValidation:rule="self.matches('^[^@]*$')",message="discoveryURL must not contain user info"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this robust enough of a regular expression to effectively capture this?

As-is this seems like it would match URLs like https://en.wikipedia.org/wiki/@_(album) which doesn't contain any userinfo in the URL but does contain the @ symbol as part of the path and is technically a valid URL.

Copy link
Author

@ShazaAldawamneh ShazaAldawamneh Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@everettraven Honestly, I’m not sure what the best replacement would be. any suggestions ?

Copy link
Contributor

Choose a reason for hiding this comment

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

user info seems to be generally parsed from somestring:someotherstring@hostname, maybe you can do something like ^https:\/\/.+:.+@{1}.+\/.+?

That should ensure we are only attempting to match the general user info pattern between the https:// prefix and the {hostname}/ values and allow anything in the path.

Quickly testing on https://regexr.com/ shows that it would match on https://testuser:[email protected]/some/path/@value but not https://example.com/some/path/@value or https://example.com:7784/some/path/@value which is what I think we want.

It would be good to make sure we have a few tests exercising this behavior to make sure it isn't matching something that should be valid.

// +kubebuilder:validation:MaxLength=2048
DiscoveryURL *string `json:"discoveryURL,omitempty"`

// audienceMatchPolicy specifies how token audiences are matched.
// If omitted, the system applies a default policy.
// Valid values are:
// - "MatchAny": The token is accepted if any of its audiences match any of the configured audiences.
//
// +optional
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
AudienceMatchPolicy *AudienceMatchPolicy `json:"audienceMatchPolicy,omitempty"`
}

// AudienceMatchPolicyType is a set of valid values for Issuer.AudienceMatchPolicy.
//
// +kubebuilder:validation:Enum=MatchAny;""
Copy link
Contributor

Choose a reason for hiding this comment

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

What does setting audienceMatchPolicy to "" mean?

Copy link
Author

Choose a reason for hiding this comment

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

Setting audienceMatchPolicy to "" means the system will apply the default policy, which currently behaves like MatchAny.
I will include that in the comments of the field

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand, the audience match policy of "" on the Kubernetes Structured Authentication Configuration is only valid when a singular audience is present, so this essentially means "match only this audience".

MatchAny is required when specifying more than a singular audience but is also valid whenever a single audience is provided.

If MatchAny is always a valid value and is really the only option that exists today, why should we allow a user to configure this?

type AudienceMatchPolicy string

// Valid types for AudienceMatchPolicyType
const (
AudienceMatchPolicyMatchAny AudienceMatchPolicy = "MatchAny"
)

type TokenClaimMappings struct {
// username is a required field that configures how the username of a cluster identity
// should be constructed from the claims in a JWT token issued by the identity provider.
Expand Down Expand Up @@ -717,44 +767,54 @@ type PrefixedClaimMapping struct {
Prefix string `json:"prefix"`
}

// TokenValidationRuleType represents the different
// claim validation rule types that can be configured.
// +enum
// TokenValidationRuleType defines the type of token validation rule.
//
// +kubebuilder:validation:Enum=RequiredClaim;Expression
type TokenValidationRuleType string

const (
TokenValidationRuleTypeRequiredClaim = "RequiredClaim"
TokenValidationRuleRequiredClaim = "RequiredClaim"
TokenValidationRuleExpression = "Expression"
)

// TokenClaimValidationRule represents a validation rule based on token claims.
// If type is RequiredClaim, requiredClaim must be set.
// If type is Expression, expressionRule must be set.
//
// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'RequiredClaim' ? has(self.requiredClaim) : !has(self.requiredClaim)",message="requiredClaim must be set when type is 'RequiredClaim', and forbidden otherwise"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.type) && self.type == 'Expression' ? has(self.expressionRule) : !has(self.expressionRule)",message="expressionRule must be set when type is 'Expression', and forbidden otherwise"
type TokenClaimValidationRule struct {
// type is an optional field that configures the type of the validation rule.
//
// Allowed values are 'RequiredClaim' and omitted (not provided or an empty string).
// Allowed values are "RequiredClaim" and "Expression".
//
// When set to 'RequiredClaim', the Kubernetes API server will be configured
// to validate that the incoming JWT contains the required claim and that its
// value matches the required value.
//
// When set to 'RequiredClaim', the Kubernetes API server
// will be configured to validate that the incoming JWT
// contains the required claim and that its value matches
// the required value.
// When set to 'Expression', the Kubernetes API server will be configured
// to validate the incoming JWT against the configured CEL expression.
//
// Defaults to 'RequiredClaim'.
// Defaults to "RequiredClaim".
//
// +kubebuilder:validation:Enum={"RequiredClaim"}
// +kubebuilder:default="RequiredClaim"
Comment on lines -739 to 800
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not recall what we landed on here exactly, but I don't think it was ever possible for this to default without an error being returned from the CAO: https://github.com/openshift/cluster-authentication-operator/blob/cb20ecadf8f6a878cfc1803b29ae6a20c27f7601/pkg/controllers/externaloidc/externaloidc_controller.go#L425-L427

That same logic applies for HyperShift as well: https://github.com/openshift/hypershift/blob/0fe7be3e59f44eb701d2a492bdada22838ec8b1e/control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go#L290-L292

I think it is probably safe to remove the defaulting here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, it should be safe to make this field required if this configuration is present. It would have been an invalid configuration for this type to be configured without specifying anything previously (hence the defaulting behavior).

Copy link
Author

@ShazaAldawamneh ShazaAldawamneh Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this field

you mean making Type TokenValidationRuleType required ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, instead of optional with a default, lets make the type field required using the +required marker.

// +kubebuilder:validation:Enum=RequiredClaim;Expression
Type TokenValidationRuleType `json:"type"`

// requiredClaim is an optional field that configures the required claim
// and value that the Kubernetes API server will use to validate if an incoming
// JWT is valid for this identity provider.
//
// requiredClaim allows configuring a required claim name and its expected value.
// RequiredClaim is used when type is RequiredClaim.
// +optional
RequiredClaim *TokenRequiredClaim `json:"requiredClaim,omitempty"`

// expressionRule contains the configuration for the "Expression" type.
// Must be set if type == "Expression".
//
// +optional
ExpressionRule *TokenExpressionRule `json:"expressionRule,omitempty"`
}

type TokenRequiredClaim struct {
// claim is a required field that configures the name of the required claim.
// When taken from the JWT claims, claim must be a string value.
//
// claim must not be an empty string ("").
// claim is a name of a required claim. Only claims with string values are supported.
//
// +kubebuilder:validation:MinLength=1
// +required
Expand All @@ -771,3 +831,53 @@ type TokenRequiredClaim struct {
// +required
RequiredValue string `json:"requiredValue"`
}

type TokenExpressionRule struct {
// expression is a CEL expression evaluated against token claims.
// The expression must be a non-empty string and no longer than 4096 characters.
// This field is required.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=4096
// +required
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
Expression string `json:"expression,omitempty"`

// message allows configuring the human-readable message that is returned
// from the Kubernetes API server when a token fails validation based on
// the CEL expression defined in 'expression'. This field is optional.
Comment on lines +846 to +848
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is quite true. Reading https://kubernetes.io/docs/reference/access-authn-authz/authentication/#using-authentication-configuration it looks like this is the message that is logged by the Kubernetes API server.

It isn't included in anything returned to the caller.

//
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Comment on lines +851 to +852
Copy link
Contributor

Choose a reason for hiding this comment

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

Include all constraints in plain english in the GoDoc please. Users cannot see these markers.

// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
Message string `json:"message,omitempty"`
}

// TokenUserValidationRule provides a CEL-based rule used to validate a token subject.
// Each rule contains a CEL expression that is evaluated against the token’s claims.
type TokenUserValidationRule struct {
// expression is a CEL expression that must evaluate
// to true for the token to be accepted. The expression is evaluated against the token's
// user information (e.g., username, groups).
//
// If the expression evaluates to false, the token is rejected.
// See https://kubernetes.io/docs/reference/using-api/cel/ for CEL syntax.
// At least one rule must evaluate to true for the token to be considered valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a list, so we should move this to where the list is defined.

//
// This field must be non-empty and may not exceed 4096 characters.
//
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=4096
Copy link
Contributor

Choose a reason for hiding this comment

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

1024 please.

// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
Expression string `json:"expression,omitempty"`
// message is an optional, human-readable message returned by the API server when
// this validation rule fails. It can help clarify why a token was rejected.
//
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Comment on lines +878 to +880
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as the previous message field for token claim validations.

// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
Message string `json:"message,omitempty"`
}
Loading