Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,6 @@ tests:
message: "Empty expressions are invalid"
expectedError: "spec.oidcProviders[0].userValidationRules[0].expression: Invalid value: \"\": spec.oidcProviders[0].userValidationRules[0].expression in body should be at least 1 chars long"


- name: Invalid TokenUserValidationRule with expression only
initial: |
apiVersion: config.openshift.io/v1
Expand All @@ -456,3 +455,102 @@ tests:
userValidationRules:
- expression: "user.username.startsWith('admin')"
expectedError: "message: Required value"

- name: Can set username claim mapping using a CEL expression only
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
expression: "has(claims.upn) ? claims.upn : claims.oid"
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
expression: "has(claims.upn) ? claims.upn : claims.oid"

- name: Cannot set both claim and expression for username mapping
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
expression: "claims.sub"
expectedError: "claim must not be set when expression is provided"

- name: Can set groups mapping using a CEL expression
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
groups:
expression: "claims.roles.split(',')"
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
groups:
expression: "claims.roles.split(',')"

- name: Cannot set both claim and expression for groups mapping
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
groups:
claim: "roles"
expression: "claims.roles.split(',')"
expectedError: "claim must not be set when expression is provided"



40 changes: 37 additions & 3 deletions config/v1/types_authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,25 @@ type TokenClaimMappings struct {
}

// TokenClaimMapping allows specifying a JWT token claim to be used when mapping claims from an authentication token to cluster identities.
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="!(has(self.claim) && has(self.expression))",message="claim and expression cannot both be set"
type TokenClaimMapping struct {
// claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
//
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Claim string `json:"claim"`

// expression is an optional CEL expression used to derive
// group values from JWT claims.
//
// When specified, claim must not be set.
//
// +optional
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
Expression string `json:"expression,omitempty"`
Comment on lines 352 to +371
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the TokenClaimMapping type from the file
cat -n config/v1/types_authentication.go | sed -n '352,380p'

Repository: openshift/api

Length of output: 1903


🏁 Script executed:

# Also check if there are any similar types in the file to understand the pattern
rg -n "TokenClaimMapping|UsernameClaimMapping" config/v1/types_authentication.go | head -20

Repository: openshift/api

Length of output: 466


🏁 Script executed:

# Search for the ExternalOIDCWithUpstreamParity feature gate usage to understand context
rg -A 5 -B 5 "ExternalOIDCWithUpstreamParity" config/v1/types_authentication.go | head -50

Repository: openshift/api

Length of output: 3279


Expression is unreachable because Claim is required and the mutual-exclusion rule blocks it.

With Claim marked as +required (non-omitempty JSON tag), the validation rule !(has(self.claim) && has(self.expression)) becomes logically equivalent to !has(expression). This makes the Expression field unusable despite being optional under the feature gate.

Update Claim to optional and change the rule to a proper XOR condition to allow expression-based mappings:

Suggested fix
-// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="!(has(self.claim) && has(self.expression))",message="claim and expression cannot both be set"
+// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.claim) ? !has(self.expression) : has(self.expression)",message="precisely one of claim or expression must be set"
 type TokenClaimMapping struct {
 	// claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
 	//
-	// +required
+	// +optional
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=256
-	Claim string `json:"claim"`
+	Claim string `json:"claim,omitempty"`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@config/v1/types_authentication.go` around lines 352 - 371, The
TokenClaimMapping struct currently makes Claim required which, combined with the
mutual-exclusion rule, prevents Expression from ever being set; change Claim to
optional and make the XNOR rule into an XOR. Remove the +required annotation and
change the JSON tag for Claim to include omitempty (e.g.,
json:"claim,omitempty"), and update the openshift:validation rule to an XOR such
as rule="has(self.claim) != has(self.expression)" so exactly one of Claim or
Expression can be provided (keep Expression's existing omitempty and
feature-gate annotations).

}

// TokenClaimOrExpressionMapping allows specifying either a JWT token claim or CEL expression to be used when mapping claims from an authentication token to cluster identities.
Expand Down Expand Up @@ -590,15 +604,35 @@ type OIDCClientReference struct {

// +kubebuilder:validation:XValidation:rule="has(self.prefixPolicy) && self.prefixPolicy == 'Prefix' ? (has(self.prefix) && size(self.prefix.prefixString) > 0) : !has(self.prefix)",message="prefix must be set if prefixPolicy is 'Prefix', but must remain unset otherwise"
// +union
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="!(has(self.claim) && has(self.expression))",message="claim and expression cannot both be set"
type UsernameClaimMapping struct {
// claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
// claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
//
// Precisely one of claim or expression must be set if the
// ExternalOIDCWithUpstreamParity feature gate is enabled.
//
// claim must not be an empty string ("") and must not exceed 256 characters.
//
// +required
// +optional
// +kubebuilder:validation:MinLength:=1
// +kubebuilder:validation:MaxLength:=256
Claim string `json:"claim"`
// +kubebuilder:validation:XValidation:rule="has(self.claim)",message="claim must be set"
Claim string `json:"claim,omitempty"`

// expression is an optional CEL expression used to derive
// the username from JWT claims.
//
// CEL expressions have access to the token claims
// through a CEL variable, 'claims'.
//
// Precisely one of claim or expression must be set if the
// ExternalOIDCWithUpstreamParity feature gate is enabled.
//
// +optional
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
Expression string `json:"expression,omitempty"`
Comment on lines 605 to +635
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n config/v1/types_authentication.go | sed -n '600,650p'

Repository: openshift/api

Length of output: 2972


Remove the unconditional has(self.claim) rule from the Claim field.

The unconditional validation rule on line 619 (has(self.claim)) requires the Claim field to always be set, which blocks expression-only username mappings even when the ExternalOIDCWithUpstreamParity feature gate is enabled. The feature gate documentation (lines 611–612) states that precisely one of claim or expression must be set when the gate is enabled, but the unconditional field-level rule prevents this.

Replace the struct-level rule on line 607 to enforce exclusive-or semantics:

  • When feature gate is enabled: exactly one of claim or expression must be set
  • The unconditional field-level rule on line 619 must be removed
Fix
-// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="!(has(self.claim) && has(self.expression))",message="claim and expression cannot both be set"
+// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.claim) ? !has(self.expression) : has(self.expression)",message="precisely one of claim or expression must be set"
 type UsernameClaimMapping struct {
@@
-	// +kubebuilder:validation:XValidation:rule="has(self.claim)",message="claim must be set"
 	Claim string `json:"claim,omitempty"`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// +kubebuilder:validation:XValidation:rule="has(self.prefixPolicy) && self.prefixPolicy == 'Prefix' ? (has(self.prefix) && size(self.prefix.prefixString) > 0) : !has(self.prefix)",message="prefix must be set if prefixPolicy is 'Prefix', but must remain unset otherwise"
// +union
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="!(has(self.claim) && has(self.expression))",message="claim and expression cannot both be set"
type UsernameClaimMapping struct {
// claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
// claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
//
// Precisely one of claim or expression must be set if the
// ExternalOIDCWithUpstreamParity feature gate is enabled.
//
// claim must not be an empty string ("") and must not exceed 256 characters.
//
// +required
// +optional
// +kubebuilder:validation:MinLength:=1
// +kubebuilder:validation:MaxLength:=256
Claim string `json:"claim"`
// +kubebuilder:validation:XValidation:rule="has(self.claim)",message="claim must be set"
Claim string `json:"claim,omitempty"`
// expression is an optional CEL expression used to derive
// the username from JWT claims.
//
// CEL expressions have access to the token claims
// through a CEL variable, 'claims'.
//
// Precisely one of claim or expression must be set if the
// ExternalOIDCWithUpstreamParity feature gate is enabled.
//
// +optional
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
Expression string `json:"expression,omitempty"`
// +kubebuilder:validation:XValidation:rule="has(self.prefixPolicy) && self.prefixPolicy == 'Prefix' ? (has(self.prefix) && size(self.prefix.prefixString) > 0) : !has(self.prefix)",message="prefix must be set if prefixPolicy is 'Prefix', but must remain unset otherwise"
// +union
// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDCWithUpstreamParity,rule="has(self.claim) ? !has(self.expression) : has(self.expression)",message="precisely one of claim or expression must be set"
type UsernameClaimMapping struct {
// claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
//
// Precisely one of claim or expression must be set if the
// ExternalOIDCWithUpstreamParity feature gate is enabled.
//
// claim must not be an empty string ("") and must not exceed 256 characters.
//
// +optional
// +kubebuilder:validation:MinLength:=1
// +kubebuilder:validation:MaxLength:=256
Claim string `json:"claim,omitempty"`
// expression is an optional CEL expression used to derive
// the username from JWT claims.
//
// CEL expressions have access to the token claims
// through a CEL variable, 'claims'.
//
// Precisely one of claim or expression must be set if the
// ExternalOIDCWithUpstreamParity feature gate is enabled.
//
// +optional
// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
Expression string `json:"expression,omitempty"`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@config/v1/types_authentication.go` around lines 605 - 635, Remove the
unconditional field-level XValidation on the Claim field (delete the
+kubebuilder:validation:XValidation:rule="has(self.claim)" annotation) and
instead enforce exclusive-or at the struct level when the feature gate is
enabled: update the existing FeatureGateAwareXValidation (on type
UsernameClaimMapping) to a rule that requires exactly one of claim or expression
when ExternalOIDCWithUpstreamParity is enabled (e.g. rule="has(self.claim) !=
has(self.expression)" with an appropriate message). Keep the MinLength/MaxLength
tags on Claim and Expression but do not require Claim unconditionally.


// prefixPolicy is an optional field that configures how a prefix should be applied to the value of the JWT claim specified in the 'claim' field.
//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,17 @@ spec:
description: claim is a required field that configures
the JWT token claim whose value is assigned to the
cluster identity field associated with this mapping.
maxLength: 256
minLength: 1
type: string
expression:
description: |-
expression is an optional CEL expression used to derive
group values from JWT claims.

When specified, claim must not be set.
maxLength: 1024
minLength: 1
type: string
prefix:
description: |-
Expand All @@ -202,6 +213,9 @@ spec:
required:
- claim
type: object
x-kubernetes-validations:
- message: claim and expression cannot both be set
rule: '!(has(self.claim) && has(self.expression))'
uid:
description: |-
uid is an optional field for configuring the claim mapping used to construct the uid for the cluster identity.
Expand Down Expand Up @@ -252,12 +266,31 @@ spec:
properties:
claim:
description: |-
claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.

Precisely one of claim or expression must be set if the
ExternalOIDCWithUpstreamParity feature gate is enabled.

claim must not be an empty string ("") and must not exceed 256 characters.
maxLength: 256
minLength: 1
type: string
x-kubernetes-validations:
- message: claim must be set
rule: has(self.claim)
expression:
description: |-
expression is an optional CEL expression used to derive
the username from JWT claims.

CEL expressions have access to the token claims
through a CEL variable, 'claims'.

Precisely one of claim or expression must be set if the
ExternalOIDCWithUpstreamParity feature gate is enabled.
maxLength: 1024
minLength: 1
type: string
prefix:
description: |-
prefix configures the prefix that should be prepended to the value of the JWT claim.
Expand Down Expand Up @@ -301,10 +334,10 @@ spec:
- NoPrefix
- Prefix
type: string
required:
- claim
type: object
x-kubernetes-validations:
- message: claim and expression cannot both be set
rule: '!(has(self.claim) && has(self.expression))'
- message: prefix must be set if prefixPolicy is 'Prefix',
but must remain unset otherwise
rule: 'has(self.prefixPolicy) && self.prefixPolicy ==
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,8 @@ spec:
description: claim is a required field that configures
the JWT token claim whose value is assigned to the
cluster identity field associated with this mapping.
maxLength: 256
minLength: 1
type: string
prefix:
description: |-
Expand Down Expand Up @@ -252,12 +254,18 @@ spec:
properties:
claim:
description: |-
claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.

Precisely one of claim or expression must be set if the
ExternalOIDCWithUpstreamParity feature gate is enabled.

claim must not be an empty string ("") and must not exceed 256 characters.
maxLength: 256
minLength: 1
type: string
x-kubernetes-validations:
- message: claim must be set
rule: has(self.claim)
prefix:
description: |-
prefix configures the prefix that should be prepended to the value of the JWT claim.
Expand Down Expand Up @@ -301,8 +309,6 @@ spec:
- NoPrefix
- Prefix
type: string
required:
- claim
type: object
x-kubernetes-validations:
- message: prefix must be set if prefixPolicy is 'Prefix',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,17 @@ spec:
description: claim is a required field that configures
the JWT token claim whose value is assigned to the
cluster identity field associated with this mapping.
maxLength: 256
minLength: 1
type: string
expression:
description: |-
expression is an optional CEL expression used to derive
group values from JWT claims.

When specified, claim must not be set.
maxLength: 1024
minLength: 1
type: string
prefix:
description: |-
Expand All @@ -202,6 +213,9 @@ spec:
required:
- claim
type: object
x-kubernetes-validations:
- message: claim and expression cannot both be set
rule: '!(has(self.claim) && has(self.expression))'
uid:
description: |-
uid is an optional field for configuring the claim mapping used to construct the uid for the cluster identity.
Expand Down Expand Up @@ -252,12 +266,31 @@ spec:
properties:
claim:
description: |-
claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.

Precisely one of claim or expression must be set if the
ExternalOIDCWithUpstreamParity feature gate is enabled.

claim must not be an empty string ("") and must not exceed 256 characters.
maxLength: 256
minLength: 1
type: string
x-kubernetes-validations:
- message: claim must be set
rule: has(self.claim)
expression:
description: |-
expression is an optional CEL expression used to derive
the username from JWT claims.

CEL expressions have access to the token claims
through a CEL variable, 'claims'.

Precisely one of claim or expression must be set if the
ExternalOIDCWithUpstreamParity feature gate is enabled.
maxLength: 1024
minLength: 1
type: string
prefix:
description: |-
prefix configures the prefix that should be prepended to the value of the JWT claim.
Expand Down Expand Up @@ -301,10 +334,10 @@ spec:
- NoPrefix
- Prefix
type: string
required:
- claim
type: object
x-kubernetes-validations:
- message: claim and expression cannot both be set
rule: '!(has(self.claim) && has(self.expression))'
- message: prefix must be set if prefixPolicy is 'Prefix',
but must remain unset otherwise
rule: 'has(self.prefixPolicy) && self.prefixPolicy ==
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,8 @@ spec:
description: claim is a required field that configures
the JWT token claim whose value is assigned to the
cluster identity field associated with this mapping.
maxLength: 256
minLength: 1
type: string
prefix:
description: |-
Expand Down Expand Up @@ -252,12 +254,18 @@ spec:
properties:
claim:
description: |-
claim is a required field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.
claim is a optional field that configures the JWT token claim whose value is assigned to the cluster identity field associated with this mapping.

Precisely one of claim or expression must be set if the
ExternalOIDCWithUpstreamParity feature gate is enabled.

claim must not be an empty string ("") and must not exceed 256 characters.
maxLength: 256
minLength: 1
type: string
x-kubernetes-validations:
- message: claim must be set
rule: has(self.claim)
prefix:
description: |-
prefix configures the prefix that should be prepended to the value of the JWT claim.
Expand Down Expand Up @@ -301,8 +309,6 @@ spec:
- NoPrefix
- Prefix
type: string
required:
- claim
type: object
x-kubernetes-validations:
- message: prefix must be set if prefixPolicy is 'Prefix',
Expand Down
Loading