CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR#810
CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR#810ShazaAldawamneh wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds upstream-parity support for External OIDC by generating and wiring user validation rules and CEL-based claim validation into JWT issuance (guarded by a feature gate), propagates issuer DiscoveryURL, updates operator gating, and bumps two OpenShift module versions; tests expanded for upstream-parity scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant Controller
participant Issuer
participant JWTConfig as "API Server\nJWT Authenticator"
Operator->>Controller: Enable ExternalOIDC feature gates (including UpstreamParity)
Controller->>Issuer: Read issuer configuration (mappings, discoveryURL, rules)
Issuer-->>Controller: Return issuer config (includes DiscoveryURL if set)
Controller->>Controller: Validate claim rules (incl. CEL) and user validation rules
Controller->>JWTConfig: Generate JWT authenticator (Issuer, ClaimValidationRules, UserValidationRules)
JWTConfig-->>Controller: Store/apply JWT auth config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ShazaAldawamneh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall, the structure looks pretty good.
I have a handful of comments.
|
/retest-required |
|
/retest-required |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
82a054d to
4cb42b2
Compare
openshift-merge-robot
added
the
needs-rebase
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
4cb42b2 to
ffbc0c5
Compare
openshift-merge-robot
removed
the
needs-rebase
ShazaAldawamneh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A few minor things. Other than that, this LGTM
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/externaloidc/externaloidc_controller.go (1)
407-418:⚠️ Potential issue | 🟠 MajorEnforce UpstreamParity gate for CEL claim validation rules.
Line 407 ignores
featureGates, soTokenValidationRuleTypeCELrules can be emitted even whenFeatureGateExternalOIDCWithUpstreamParityis off. This bypasses feature gating for new fields.🛠️ Suggested fix
func generateClaimValidationRules(featureGates featuregates.FeatureGate, claimValidationRules ...configv1.TokenClaimValidationRule) ([]apiserverv1beta1.ClaimValidationRule, error) { out := []apiserverv1beta1.ClaimValidationRule{} errs := []error{} for _, claimValidationRule := range claimValidationRules { + if claimValidationRule.Type == configv1.TokenValidationRuleTypeCEL && + !featureGates.Enabled(features.FeatureGateExternalOIDCWithUpstreamParity) { + errs = append(errs, fmt.Errorf("claimValidationRule.type %q requires %s feature gate", configv1.TokenValidationRuleTypeCEL, features.FeatureGateExternalOIDCWithUpstreamParity)) + continue + } rule, err := generateClaimValidationRule(claimValidationRule) if err != nil { errs = append(errs, fmt.Errorf("generating claimValidationRule: %v", err)) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 407 - 418, The loop in generateClaimValidationRules ignores the featureGates parameter and may emit CEL-based rules regardless of the FeatureGateExternalOIDCWithUpstreamParity flag; update generateClaimValidationRules to check featureGates.Has(featuregates.FeatureGateExternalOIDCWithUpstreamParity) (or equivalent) before appending any rule whose type is TokenValidationRuleTypeCEL (as determined from generateClaimValidationRule or the returned rule.Type), and if the gate is off either skip that rule or return a descriptive error; reference generateClaimValidationRules, generateClaimValidationRule, TokenValidationRuleTypeCEL, and FeatureGateExternalOIDCWithUpstreamParity to locate and implement the gate check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 226-228: The code currently sets out.DiscoveryURL when
issuer.DiscoveryURL is non-empty; instead validate the URL scheme (parse
issuer.DiscoveryURL and ensure scheme == "https") before assigning to
out.DiscoveryURL and return the appropriate error for non-HTTPS or invalid URLs;
update the branch around issuer.DiscoveryURL and out.DiscoveryURL so it parses
the value (using net/url.Parse), checks u.Scheme == "https", and only then sets
out.DiscoveryURL = &issuer.DiscoveryURL, otherwise return the “invalid discovery
URL (http)” style error.
---
Outside diff comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 407-418: The loop in generateClaimValidationRules ignores the
featureGates parameter and may emit CEL-based rules regardless of the
FeatureGateExternalOIDCWithUpstreamParity flag; update
generateClaimValidationRules to check
featureGates.Has(featuregates.FeatureGateExternalOIDCWithUpstreamParity) (or
equivalent) before appending any rule whose type is TokenValidationRuleTypeCEL
(as determined from generateClaimValidationRule or the returned rule.Type), and
if the gate is off either skip that rule or return a descriptive error;
reference generateClaimValidationRules, generateClaimValidationRule,
TokenValidationRuleTypeCEL, and FeatureGateExternalOIDCWithUpstreamParity to
locate and implement the gate check.
|
/retest |
|
/retest-required |
|
/retest |
|
Looks like the failing non-required jobs are related to in-development tests. I'm not sure we need to be concerned about breaking them at the moment, especially because all of the other jobs are passing fine. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from this still piping through unused feature gate parameters, the changes LGTM.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/controllers/externaloidc/externaloidc_controller.go (1)
226-228:⚠️ Potential issue | 🟠 MajorValidate DiscoveryURL scheme before setting it.
Line 226 currently accepts any non-empty value; this allows insecure or malformed endpoints and conflicts with the “invalid discovery URL (http)” expectation.
🔒 Proposed fix
- if len(issuer.DiscoveryURL) > 0 { - out.DiscoveryURL = &issuer.DiscoveryURL - } + if len(issuer.DiscoveryURL) > 0 { + parsed, err := url.Parse(issuer.DiscoveryURL) + if err != nil || parsed.Scheme != "https" || parsed.Host == "" { + return apiserverv1beta1.Issuer{}, fmt.Errorf("discoveryURL must be a valid https URL: %q", issuer.DiscoveryURL) + } + out.DiscoveryURL = &issuer.DiscoveryURL + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 226 - 228, Validate the issuer.DiscoveryURL before assigning it to out.DiscoveryURL: parse issuer.DiscoveryURL (e.g., using url.Parse), ensure it is a well-formed URL and that its scheme is "https" (reject "http" or other schemes), and only set out.DiscoveryURL = &issuer.DiscoveryURL when the parse succeeds and scheme == "https"; if parsing fails or the scheme is not "https", return or surface an appropriate validation error instead of assigning the value (refer to issuer.DiscoveryURL and out.DiscoveryURL in externaloidc_controller.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 463-492: The generateUserValidationRule functions currently don't
validate CEL expressions; update generateUserValidationRule to compile the
rule.Expression using the authenticationcel.UserValidationCondition and the
compiler. Use the compiler.CompileUserExpression(...,
authenticationcel.UserValidationCondition) call (matching the claim validation
pattern) and return a descriptive error if compilation fails; keep
generateUserValidationRules behavior but propagate compilation errors so invalid
expressions are rejected at config time.
- Around line 441-454: The code is using
authenticationcel.ClaimMappingExpression (meant for claim mappings returning
AnyType) when validating claimValidationRule.CEL.Expression; change the
validation to use authenticationcel.ClaimValidationCondition (which returns
BoolType) instead and pass the value (not a pointer) from
claimValidationRule.CEL into validateCELExpression. Specifically, update the
call to validateCELExpression to construct an
authenticationcel.ClaimValidationCondition with Expression set from
claimValidationRule.CEL.Expression (and Message if needed), keep the existing
error handling, and leave out.Expression and out.Message assignments as they
are.
---
Duplicate comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 226-228: Validate the issuer.DiscoveryURL before assigning it to
out.DiscoveryURL: parse issuer.DiscoveryURL (e.g., using url.Parse), ensure it
is a well-formed URL and that its scheme is "https" (reject "http" or other
schemes), and only set out.DiscoveryURL = &issuer.DiscoveryURL when the parse
succeeds and scheme == "https"; if parsing fails or the scheme is not "https",
return or surface an appropriate validation error instead of assigning the value
(refer to issuer.DiscoveryURL and out.DiscoveryURL in
externaloidc_controller.go).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/controllers/externaloidc/externaloidc_controller.go
| case configv1.TokenValidationRuleTypeCEL: | ||
| if len(claimValidationRule.CEL.Expression) == 0 { | ||
| return apiserverv1beta1.ClaimValidationRule{}, fmt.Errorf("claimValidationRule.type is %s and expression is not set", configv1.TokenValidationRuleTypeCEL) | ||
| } | ||
|
|
||
| // validate CEL expression | ||
| if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{ | ||
| Expression: claimValidationRule.CEL.Expression, | ||
| }); err != nil { | ||
| return apiserverv1beta1.ClaimValidationRule{}, fmt.Errorf("invalid CEL expression: %v", err) | ||
| } | ||
|
|
||
| out.Expression = claimValidationRule.CEL.Expression | ||
| out.Message = claimValidationRule.CEL.Message |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" | grep -E "(externaloidc|authenticationcel)" | head -20Repository: openshift/cluster-authentication-operator
Length of output: 205
🏁 Script executed:
cat -n pkg/controllers/externaloidc/externaloidc_controller.go | sed -n '430,470p'Repository: openshift/cluster-authentication-operator
Length of output: 2269
🏁 Script executed:
rg "ClaimValidationRule" --type go -A 3 -B 1 | head -100Repository: openshift/cluster-authentication-operator
Length of output: 9821
🏁 Script executed:
rg "ClaimMappingExpression|ClaimValidationExpression" --type go | head -50Repository: openshift/cluster-authentication-operator
Length of output: 1277
🏁 Script executed:
rg "type.*CEL" pkg/ --type go | grep -i claimRepository: openshift/cluster-authentication-operator
Length of output: 298
🏁 Script executed:
rg "TokenValidationRuleTypeCEL|type.*CEL" --type go -B 5 -A 5 | head -80Repository: openshift/cluster-authentication-operator
Length of output: 7868
🏁 Script executed:
find . -path "*/vendor" -prune -o -type f -name "*.go" -exec grep -l "claimValidationRule.*CEL" {} \;Repository: openshift/cluster-authentication-operator
Length of output: 142
🏁 Script executed:
rg "case configv1.TokenValidationRuleTypeCEL" -A 20 --type goRepository: openshift/cluster-authentication-operator
Length of output: 2056
🏁 Script executed:
rg "type.*CEL\s*struct" --type go -A 10 | head -80Repository: openshift/cluster-authentication-operator
Length of output: 961
🏁 Script executed:
grep -r "CEL.*\*" pkg/controllers/externaloidc/ --include="*.go"Repository: openshift/cluster-authentication-operator
Length of output: 67
🏁 Script executed:
find . -path "*/vendor" -prune -o -type f -name "*.go" -print | xargs grep -l "TokenValidationRuleTypeCEL\|type.*CEL struct" | grep -v vendor | head -10Repository: openshift/cluster-authentication-operator
Length of output: 142
🏁 Script executed:
rg "type.*CEL\s" pkg/controllers/externaloidc/ --type go -B 5 -A 10Repository: openshift/cluster-authentication-operator
Length of output: 67
🏁 Script executed:
find . -path "*/vendor" -prune -o -type f -name "*.go" -exec grep -l "TokenValidationRule" {} \; | grep -v vendorRepository: openshift/cluster-authentication-operator
Length of output: 205
🏁 Script executed:
rg "configv1\." pkg/controllers/externaloidc/externaloidc_controller.go | head -20Repository: openshift/cluster-authentication-operator
Length of output: 2341
🏁 Script executed:
rg "import\s*\(" pkg/controllers/externaloidc/externaloidc_controller.go -A 30Repository: openshift/cluster-authentication-operator
Length of output: 1180
🏁 Script executed:
rg "type TokenClaimValidationRule" --type go -A 15Repository: openshift/cluster-authentication-operator
Length of output: 4624
🏁 Script executed:
find . -path "*/vendor" -prune -o -type f -name "*.go" -exec grep -l "TokenClaimValidationRule" {} \; | head -5Repository: openshift/cluster-authentication-operator
Length of output: 205
🏁 Script executed:
rg "type.*ClaimValidation.*CEL" --type go -A 10Repository: openshift/cluster-authentication-operator
Length of output: 1428
🏁 Script executed:
rg "ExpressionAccessor\|ClaimValidation.*Expression" vendor/k8s.io/apiserver/pkg/authentication/cel/ --type go -A 5Repository: openshift/cluster-authentication-operator
Length of output: 67
🏁 Script executed:
rg "type TokenClaimValidationRule struct" vendor/github.com/openshift/api/config/v1/ --type go -A 25Repository: openshift/cluster-authentication-operator
Length of output: 3071
🏁 Script executed:
rg "CEL.*\*TokenClaimValidationCELRule|CEL.*TokenClaimValidationCELRule" vendor/github.com/openshift/api/config/v1/ --type go -B 2 -A 2Repository: openshift/cluster-authentication-operator
Length of output: 1689
🏁 Script executed:
cat vendor/github.com/openshift/api/config/v1/types_authentication.go | grep -A 50 "type TokenClaimValidationRule struct"Repository: openshift/cluster-authentication-operator
Length of output: 2390
🏁 Script executed:
rg "validateCELExpression" pkg/controllers/externaloidc/ --type go -B 5 -A 10Repository: openshift/cluster-authentication-operator
Length of output: 6031
🏁 Script executed:
rg "ClaimValidationExpression|ClaimValidation" vendor/k8s.io/apiserver/pkg/authentication/cel/ --type go -A 5Repository: openshift/cluster-authentication-operator
Length of output: 2376
🏁 Script executed:
rg "compileClaimsCELExpression|ClaimValidation" vendor/k8s.io/apiserver/ --type go | head -30Repository: openshift/cluster-authentication-operator
Length of output: 4847
🏁 Script executed:
rg "ClaimValidationCondition\|ClaimMappingExpression" vendor/k8s.io/apiserver/pkg/authentication/cel/interface.go -A 10Repository: openshift/cluster-authentication-operator
Length of output: 67
🏁 Script executed:
cat vendor/k8s.io/apiserver/pkg/authentication/cel/interface.go | grep -A 15 "type ClaimMappingExpression\|type ClaimValidationCondition"Repository: openshift/cluster-authentication-operator
Length of output: 1126
🏁 Script executed:
rg "ClaimValidationCondition" vendor/k8s.io/apiserver/pkg/apis/apiserver/ --type go -B 3 -A 3 | head -50Repository: openshift/cluster-authentication-operator
Length of output: 818
🏁 Script executed:
rg "compileClaimsExpression.*ClaimValidation|ClaimValidationCondition" vendor/k8s.io/apiserver/ --type go | head -20Repository: openshift/cluster-authentication-operator
Length of output: 854
🏁 Script executed:
rg "CompileClaimsExpression" vendor/k8s.io/apiserver/pkg/authentication/cel/ --type go -A 5Repository: openshift/cluster-authentication-operator
Length of output: 1708
🏁 Script executed:
cat vendor/k8s.io/apiserver/pkg/authentication/cel/interface.go | head -100Repository: openshift/cluster-authentication-operator
Length of output: 3897
🏁 Script executed:
rg "type TokenClaimValidationCELRule struct" vendor/github.com/openshift/api/config/v1/ --type go -A 20Repository: openshift/cluster-authentication-operator
Length of output: 2527
Use ClaimValidationCondition instead of ClaimMappingExpression for claim validation rule expressions.
Line 447 uses authenticationcel.ClaimMappingExpression, which is for claim mappings and returns AnyType. Claim validation rules should use authenticationcel.ClaimValidationCondition, which returns BoolType and is the correct accessor for validating token claims (as used by upstream k8s in its validation code). The claimValidationRule.CEL field itself is safe to access directly—it's a value type, not a pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 441 -
454, The code is using authenticationcel.ClaimMappingExpression (meant for claim
mappings returning AnyType) when validating claimValidationRule.CEL.Expression;
change the validation to use authenticationcel.ClaimValidationCondition (which
returns BoolType) instead and pass the value (not a pointer) from
claimValidationRule.CEL into validateCELExpression. Specifically, update the
call to validateCELExpression to construct an
authenticationcel.ClaimValidationCondition with Expression set from
claimValidationRule.CEL.Expression (and Message if needed), keep the existing
error handling, and leave out.Expression and out.Message assignments as they
are.
There was a problem hiding this comment.
This is a good catch - we should do this.
There was a problem hiding this comment.
🧩 Analysis chain
[plan_code_changes]
- Replace ClaimMappingExpression with ClaimValidationCondition when validating CEL in claim validation rules.
- Keep error handling and out.Expression/out.Message assignments unchanged.
- Verify no other misuse of ClaimMappingExpression for validations.
@coderabbit-user Thanks, @everettraven — agreed. I’ve queued the change to use authenticationcel.ClaimValidationCondition for claim-validation CEL checks. The above minimal patch swaps the accessor and keeps behavior intact. If you’re good with this, I’ll proceed to land it in this PR.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show generateClaimValidationRule context =="
rg -nPU '(?s)func\s+generateClaimValidationRule\([^)]*\)\s*\(apiserverv1beta1\.ClaimValidationRule, error\)\s*{.*?^\}' pkg/controllers/externaloidc/externaloidc_controller.go -n -C3 || true
echo
echo "== Find any validateCELExpression calls using ClaimMappingExpression =="
rg -n "validateCELExpression\\s*\\(&authenticationcel\\.ClaimMappingExpression" -S || true
echo
echo "== Find any validateCELExpression calls using ClaimValidationCondition =="
rg -n "validateCELExpression\\s*\\(&authenticationcel\\.ClaimValidationCondition" -S || true
echo
echo "== Confirm vendor OpenShift API types for claim validation =="
rg -n "type\\s+TokenClaimValidationRule\\s+struct" vendor/github.com/openshift/api/config/v1/types_authentication.go -n -A20 || true
rg -n "type\\s+TokenClaimValidationCELRule\\s+struct" vendor/github.com/openshift/api/config/v1/types_authentication.go -n -A20 || true
echo
echo "== Confirm k8s authentication CEL accessors exist =="
rg -n "type\\s+ClaimMappingExpression\\s+struct|type\\s+ClaimValidationCondition\\s+struct" vendor/k8s.io/apiserver/pkg/authentication/cel/interface.go -n -A5 || true
Length of output: 2945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show context around line 348 (first ClaimMappingExpression usage) =="
cat -n pkg/controllers/externaloidc/externaloidc_controller.go | sed -n '340,360p'
Length of output: 1091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Verify generateExtraMapping also uses ClaimMappingExpression (expected/correct) =="
rg -n "func generateExtraMapping" pkg/controllers/externaloidc/externaloidc_controller.go -A30 | grep -E "(validateCELExpression|ClaimMapping)" || echo "No CEL validation in generateExtraMapping"
Length of output: 165
@everettraven Perfect! I've verified the fix is needed only for line 447 in generateClaimValidationRule(). The other CEL validation uses are correct:
- Line 348 (
generateUIDClaimMapping): UsesClaimMappingExpression✅ (extracts AnyType value) - Line 394 (
generateExtraMapping): UsesExtraMappingExpression✅ (for extra mappings) - Line 447 (
generateClaimValidationRule): UsesClaimMappingExpression❌ (should useClaimValidationConditionfor boolean validation)
Here's the precise fix:
File: pkg/controllers/externaloidc/externaloidc_controller.go
case configv1.TokenValidationRuleTypeCEL:
if len(claimValidationRule.CEL.Expression) == 0 {
return apiserverv1beta1.ClaimValidationRule{}, fmt.Errorf("claimValidationRule.type is %s and expression is not set", configv1.TokenValidationRuleTypeCEL)
}
// validate CEL expression
- if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{
+ if err := validateCELExpression(&authenticationcel.ClaimValidationCondition{
Expression: claimValidationRule.CEL.Expression,
}); err != nil {
return apiserverv1beta1.ClaimValidationRule{}, fmt.Errorf("invalid CEL expression: %v", err)
}
out.Expression = claimValidationRule.CEL.Expression
out.Message = claimValidationRule.CEL.MessageThis switches to the boolean-returning accessor that matches upstream k8s validation semantics. Ready to proceed!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| func generateUserValidationRule(rule configv1.TokenUserValidationRule) (apiserverv1beta1.UserValidationRule, error) { | ||
| if len(rule.Expression) == 0 { | ||
| return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("userValidationRule expression must be non-empty") | ||
| } | ||
|
|
||
| return apiserverv1beta1.UserValidationRule{ | ||
| Expression: rule.Expression, | ||
| Message: rule.Message, | ||
| }, nil | ||
| } | ||
|
|
||
| func generateUserValidationRules(rules []configv1.TokenUserValidationRule) ([]apiserverv1beta1.UserValidationRule, error) { | ||
| out := []apiserverv1beta1.UserValidationRule{} | ||
| errs := []error{} | ||
|
|
||
| for _, r := range rules { | ||
| uvr, err := generateUserValidationRule(r) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("generating userValidationRule: %v", err)) | ||
| continue | ||
| } | ||
| out = append(out, uvr) | ||
| } | ||
|
|
||
| if len(errs) > 0 { | ||
| return nil, errors.Join(errs...) | ||
| } | ||
|
|
||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
k8s.io/apiserver authenticationcel user validation rule CEL expression
💡 Result:
In apiserver.config.k8s.io AuthenticationConfiguration (JWT authenticator), user validation rules are CEL expressions in jwt[].userValidationRules[].expression that must evaluate to true for the final UserInfo to be accepted. The CEL environment provides:
user: anauthentication.k8s.io/v1UserInfo object (e.g.,user.username,user.uid,user.groups,user.extra). [1]
Example rules (reject reserved system: identities):
userValidationRules:
- expression: "!user.username.startsWith('system:')"
message: "username cannot used reserved system: prefix"
- expression: "user.groups.all(group, !group.startsWith('system:'))"
message: "groups cannot used reserved system: prefix"These are taken from the Kubernetes authentication docs as valid userValidationRules CEL expressions. [2]
Sources
[1] https://kubernetes.io/docs/reference/config-api/apiserver-config.v1/
[2] https://kubernetes.io/docs/reference/access-authn-authz/authentication/
🏁 Script executed:
# Search for CEL validation patterns in the codebase
rg "authenticationcel" -A 3 -B 3
# Also search for CEL validation usage
rg "CEL|cel" pkg/controllers/externaloidc/ -A 2 -B 2
# Check imports in the file
head -50 pkg/controllers/externaloidc/externaloidc_controller.go | grep -E "^import|authenticationcel"Repository: openshift/cluster-authentication-operator
Length of output: 30923
🏁 Script executed:
# Search for CEL compilation or validation functions
rg "Compile|ValidateRule" --type go -A 3
# Look for similar user validation in the codebase
rg "UserValidationRule" --type go -B 2 -A 2Repository: openshift/cluster-authentication-operator
Length of output: 50398
🏁 Script executed:
# Find the exact context where user validation rules are generated
grep -n "generateUserValidationRule" pkg/controllers/externaloidc/externaloidc_controller.go -B 5 -A 15
# Check how validateCELExpression is used in the context of user validation
grep -n "UserValidationRule\|generateUserValidationRule" pkg/controllers/externaloidc/externaloidc_controller.go -B 3 -A 3Repository: openshift/cluster-authentication-operator
Length of output: 3819
🏁 Script executed:
# Check what expression accessor type is expected for user validation
rg "UserValidationCondition|UserValidationRule" vendor/k8s.io/apiserver/pkg/authentication/cel/ -A 2 -B 2Repository: openshift/cluster-authentication-operator
Length of output: 2054
Validate user validation CEL expressions before accepting them.
User validation rule expressions are not validated at configuration time. Invalid CEL will only surface at runtime during token evaluation. Add CEL compilation validation using authenticationcel.UserValidationCondition and the CompileUserExpression method from the compiler, similar to the pattern used for claim validation rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 463 -
492, The generateUserValidationRule functions currently don't validate CEL
expressions; update generateUserValidationRule to compile the rule.Expression
using the authenticationcel.UserValidationCondition and the compiler. Use the
compiler.CompileUserExpression(..., authenticationcel.UserValidationCondition)
call (matching the claim validation pattern) and return a descriptive error if
compilation fails; keep generateUserValidationRules behavior but propagate
compilation errors so invalid expressions are rejected at config time.
There was a problem hiding this comment.
This is also a good catch
There was a problem hiding this comment.
@everettraven, thanks for confirming! You're right—this is a good catch by the bot. The current implementation only checks if the expression is non-empty but doesn't validate the CEL syntax at configuration time.
Looking at the code, generateUserValidationRule should follow the same validation pattern used for claim validation rules. The fix would involve:
- Creating an
authenticationcel.UserValidationConditionwith the expression - Calling
compiler.CompileUserExpression()to validate it - Returning a descriptive error if compilation fails
This would catch invalid CEL expressions early (at config generation time) rather than failing later during token evaluation.
Would you like me to implement this fix for you?
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
22caac5 to
70daaf2
Compare
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/controllers/externaloidc/externaloidc_controller.go (3)
463-471:⚠️ Potential issue | 🟠 Major
generateUserValidationRuleperforms no CEL expression validation.Only a non-empty check is present; invalid CEL will only surface at runtime during token evaluation. This was flagged in a previous review and remains unaddressed.
An important additional constraint: the existing
validateCELExpressionhelper (line 607) callsCompileClaimsExpression, which compiles in the JWT-claims CEL environment. User validation rules evaluate against theuser(authentication.k8s.io/v1 UserInfo) object, so a separate helper callingcompiler.CompileUserExpressionwithauthenticationcel.UserValidationConditionis required — reusingvalidateCELExpressiondirectly would produce incorrect compilation semantics.🛠️ Suggested approach
Add a dedicated helper and call it from
generateUserValidationRule:+func validateUserValidationCELExpression(expressionAccessor authenticationcel.ExpressionAccessor) error { + _, err := authenticationcel.NewDefaultCompiler().CompileUserExpression(expressionAccessor) + return err +}func generateUserValidationRule(rule configv1.TokenUserValidationRule) (apiserverv1beta1.UserValidationRule, error) { if len(rule.Expression) == 0 { return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("userValidationRule expression must be non-empty") } + if err := validateUserValidationCELExpression(&authenticationcel.UserValidationCondition{ + Expression: rule.Expression, + }); err != nil { + return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("invalid CEL expression: %v", err) + } return apiserverv1beta1.UserValidationRule{ Expression: rule.Expression, Message: rule.Message, }, nil }Run the following to confirm that
CompileUserExpressionandUserValidationConditionare available in the vendoredauthenticationcelpackage:#!/bin/bash # Description: Verify CompileUserExpression and UserValidationCondition exist in the vendored authenticationcel package. rg -n "CompileUserExpression\|UserValidationCondition" vendor/k8s.io/apiserver/pkg/authentication/cel/ --type go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 463 - 471, generateUserValidationRule currently only checks for non-empty Expression and doesn't validate the CEL against the UserInfo environment; add a new helper (e.g., validateUserCELExpression) that calls compiler.CompileUserExpression with authenticationcel.UserValidationCondition to compile the rule expression, and invoke this helper from generateUserValidationRule to return any compilation error instead of deferring failures to runtime; do not reuse validateCELExpression (it uses CompileClaimsExpression), and ensure generateUserValidationRule propagates the compile error and only returns the apiserverv1beta1.UserValidationRule when compilation succeeds.
447-451:⚠️ Potential issue | 🟠 Major
ClaimMappingExpressionis the wrong CEL accessor type for claim validation rules.
ClaimMappingExpressioncompiles the expression expectingAnyTypereturn (it is designed for claim-to-value mappings). A claim validation rule must evaluate to a boolean; the correct accessor isauthenticationcel.ClaimValidationCondition, which instructs the compiler to enforceBoolType. Using the mapping type here means a CEL expression returning a non-boolean (e.g., a string) will pass compile-time validation but fail at runtime token evaluation.🛠️ Suggested fix
- if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{ - Expression: claimValidationRule.CEL.Expression, - }); err != nil { + if err := validateCELExpression(&authenticationcel.ClaimValidationCondition{ + Expression: claimValidationRule.CEL.Expression, + }); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 447 - 451, The code uses authenticationcel.ClaimMappingExpression when calling validateCELExpression for claimValidationRule, which allows non-boolean results; change the CEL accessor to authenticationcel.ClaimValidationCondition so the compiler enforces BoolType. Concretely, update the validateCELExpression call that currently constructs &authenticationcel.ClaimMappingExpression{Expression: claimValidationRule.CEL.Expression} to instead construct &authenticationcel.ClaimValidationCondition{Expression: claimValidationRule.CEL.Expression} (leave validateCELExpression and claimValidationRule references unchanged).
226-228:⚠️ Potential issue | 🟠 Major
DiscoveryURLis propagated without HTTPS scheme validation.Any non-empty string passes — including
http://URLs — which allows insecure metadata endpoints to be accepted silently. This was flagged in a prior review and remains unresolved.🛠️ Suggested fix
if len(issuer.DiscoveryURL) > 0 { - out.DiscoveryURL = &issuer.DiscoveryURL + parsed, err := url.Parse(issuer.DiscoveryURL) + if err != nil || parsed.Scheme != "https" || parsed.Host == "" { + return apiserverv1beta1.Issuer{}, fmt.Errorf("discoveryURL must be a valid https URL: %q", issuer.DiscoveryURL) + } + out.DiscoveryURL = &issuer.DiscoveryURL }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 226 - 228, The DiscoveryURL is assigned without validating its scheme; update the logic around issuer.DiscoveryURL → out.DiscoveryURL in externaloidc_controller.go to only propagate non-empty values that use the HTTPS scheme (e.g., validate that issuer.DiscoveryURL starts with "https://" or parse with url.Parse and ensure Scheme == "https"); if the URL is not HTTPS, do not set out.DiscoveryURL and optionally surface/return an error or log a warning from the surrounding reconcile/validation function so insecure HTTP endpoints are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 463-471: generateUserValidationRule currently only checks for
non-empty Expression and doesn't validate the CEL against the UserInfo
environment; add a new helper (e.g., validateUserCELExpression) that calls
compiler.CompileUserExpression with authenticationcel.UserValidationCondition to
compile the rule expression, and invoke this helper from
generateUserValidationRule to return any compilation error instead of deferring
failures to runtime; do not reuse validateCELExpression (it uses
CompileClaimsExpression), and ensure generateUserValidationRule propagates the
compile error and only returns the apiserverv1beta1.UserValidationRule when
compilation succeeds.
- Around line 447-451: The code uses authenticationcel.ClaimMappingExpression
when calling validateCELExpression for claimValidationRule, which allows
non-boolean results; change the CEL accessor to
authenticationcel.ClaimValidationCondition so the compiler enforces BoolType.
Concretely, update the validateCELExpression call that currently constructs
&authenticationcel.ClaimMappingExpression{Expression:
claimValidationRule.CEL.Expression} to instead construct
&authenticationcel.ClaimValidationCondition{Expression:
claimValidationRule.CEL.Expression} (leave validateCELExpression and
claimValidationRule references unchanged).
- Around line 226-228: The DiscoveryURL is assigned without validating its
scheme; update the logic around issuer.DiscoveryURL → out.DiscoveryURL in
externaloidc_controller.go to only propagate non-empty values that use the HTTPS
scheme (e.g., validate that issuer.DiscoveryURL starts with "https://" or parse
with url.Parse and ensure Scheme == "https"); if the URL is not HTTPS, do not
set out.DiscoveryURL and optionally surface/return an error or log a warning
from the surrounding reconcile/validation function so insecure HTTP endpoints
are rejected.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (32)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.coderabbit.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha2/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.gopkg/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- pkg/operator/starter.go
- pkg/controllers/externaloidc/externaloidc_controller_test.go
|
@ShazaAldawamneh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{ | ||
| Expression: claimValidationRule.CEL.Expression, | ||
| }); err != nil { |
There was a problem hiding this comment.
The change coderabbit is suggesting:
| if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{ | |
| Expression: claimValidationRule.CEL.Expression, | |
| }); err != nil { | |
| if err := validateCELExpression(&authenticationcel.ClaimValidationCondition{ | |
| Expression: claimValidationRule.CEL.Expression, | |
| }); err != nil { |
| if len(rule.Expression) == 0 { | ||
| return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("userValidationRule expression must be non-empty") | ||
| } | ||
|
|
There was a problem hiding this comment.
Add CEL expression validation here as well. Should use the https://github.com/kubernetes/kubernetes/blob/33c064512696629e672e0e9f2350c7f875996d59/staging/src/k8s.io/apiserver/pkg/authentication/cel/interface.go#L134 expression accessor (UserValidationCondition)
4 participants
Summary by CodeRabbit