oidc-policy: compile claim predicates as equality on scalar JWT claims#1914
oidc-policy: compile claim predicates as equality on scalar JWT claims#1914SAY-5 wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe reconciler now emits CEL predicates that use equality for scalar JWT claims and guarded membership for list claims; unit tests validate scalar and list claim evaluations. ChangesCEL predicate and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go`:
- Around line 303-314: The CEL predicate is built by interpolating claim key `k`
and value `v` directly (in the loop that appends to `authPatterns` in
oidcpolicy_reconciler.go), which breaks for non-identifier claim names and
unescaped characters; change the predicate to use bracket notation and
JSON-encode/escape both the claim key and claim value (e.g.,
auth.identity[<json-encoded-key>] == <json-encoded-value>) when constructing the
`authorinov1beta3.CelPredicate.Predicate` string so keys like `custom:role` and
values with quotes/backslashes are safe; locate the loop that appends to
`authPatterns` and replace the fmt.Sprintf interpolation with
JSON/string-quoting of `k` and `v` before building the predicate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92a9e90f-5450-437c-b606-38fe4bcf1063
📒 Files selected for processing (1)
cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go
| for k, v := range claims { | ||
| // Build an equality predicate against the JWT claim. The prior | ||
| // shape (`"v" in auth.identity.k`) required the claim to be a | ||
| // list/map and failed outright on scalar claims such as `sub`, | ||
| // `iss`, or `email_verified` with authorino reporting "no such | ||
| // overload", so the OIDCPolicy auth check never matched anything | ||
| // for configurations that named a scalar claim (#1625). | ||
| authPatterns = append(authPatterns, authorinov1beta3.PatternExpressionOrRef{ | ||
| CelPredicate: authorinov1beta3.CelPredicate{ | ||
| Predicate: fmt.Sprintf(`"%s" in auth.identity.%s`, v, k), | ||
| Predicate: fmt.Sprintf(`auth.identity.%s == "%s"`, k, v), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the actual code at the specified lines
cat -n cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go | sed -n '303,314p'Repository: Kuadrant/kuadrant-operator
Length of output: 783
🏁 Script executed:
# Check the imports in the file to see what's available
head -50 cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.goRepository: Kuadrant/kuadrant-operator
Length of output: 1593
🏁 Script executed:
# Search for tests related to OIDCPolicy claims
fd -e go -path '*oidc*test*' --type f | head -20Repository: Kuadrant/kuadrant-operator
Length of output: 238
🏁 Script executed:
# Look for similar CEL predicate construction patterns in the codebase
rg 'CelPredicate' --type go -B 2 -A 2Repository: Kuadrant/kuadrant-operator
Length of output: 2931
🏁 Script executed:
# Check if there are any existing escaping/validation utilities for CEL
rg 'celStringLiteral|CEL.*escape|CEL.*quote' --type goRepository: Kuadrant/kuadrant-operator
Length of output: 52
🏁 Script executed:
# Search for OIDCPolicy tests related to claims
fd -e go -path '*oidc*' | xargs rg -l 'GetClaims\|claims' | head -10Repository: Kuadrant/kuadrant-operator
Length of output: 238
🏁 Script executed:
# Look for tests that exercise claim matching
rg 'OIDCPolicy.*claims|claims.*auth.identity' --type go -B 3 -A 3Repository: Kuadrant/kuadrant-operator
Length of output: 52
🏁 Script executed:
# Check the PR/issue context - look for `#1625`
rg '1625|claim.*scalar|scalar.*claim' --type goRepository: Kuadrant/kuadrant-operator
Length of output: 340
🏁 Script executed:
# Verify Authorino version and CEL capabilities in go.mod
cat go.mod | grep -i authorinoRepository: Kuadrant/kuadrant-operator
Length of output: 156
🏁 Script executed:
# Search for examples of how auth.identity is accessed in Authorino contexts
rg 'auth\.identity\[' --type goRepository: Kuadrant/kuadrant-operator
Length of output: 52
🏁 Script executed:
# Find all Go files in oidc-policy extension
find cmd/extensions/oidc-policy -name '*.go' -type fRepository: Kuadrant/kuadrant-operator
Length of output: 427
🏁 Script executed:
# Look for test files related to oidc-policy
find cmd/extensions/oidc-policy -name '*test*' -type fRepository: Kuadrant/kuadrant-operator
Length of output: 134
🏁 Script executed:
# Search for buildMainAuthPolicy function to understand context better
rg 'buildMainAuthPolicy' -B 5 -A 20Repository: Kuadrant/kuadrant-operator
Length of output: 5820
🏁 Script executed:
# Look for how claims are retrieved and what type they are
rg 'GetClaims' -B 3 -A 10Repository: Kuadrant/kuadrant-operator
Length of output: 2935
🏁 Script executed:
# Check if there are any examples of map indexing in CEL expressions
rg '\[.*\].*==' --type-add 'go:*.go' -g 'cmd/extensions' | head -20Repository: Kuadrant/kuadrant-operator
Length of output: 218
🏁 Script executed:
# Check the OIDCPolicy test file
cat -n cmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types_test.goRepository: Kuadrant/kuadrant-operator
Length of output: 6831
🏁 Script executed:
# Look for any documentation or examples about auth.identity access
rg 'auth\.identity' --max-count 20Repository: Kuadrant/kuadrant-operator
Length of output: 47318
🏁 Script executed:
# Check if there's a vendor directory with Authorino source
find . -name 'authorino' -type d 2>/dev/null | head -5Repository: Kuadrant/kuadrant-operator
Length of output: 122
🏁 Script executed:
# Look for any CEL-related tests or documentation
fd -e md -name '*CEL*' -o -name '*cel*' 2>/dev/null | head -10Repository: Kuadrant/kuadrant-operator
Length of output: 52
🏁 Script executed:
# Search for how the issue `#1625` was described or resolved
rg '1625' -B 5 -A 5Repository: Kuadrant/kuadrant-operator
Length of output: 1462
🏁 Script executed:
# Search for how issuer URL is handled (line 319 in same function)
rg 'auth\.identity\.iss' -B 2 -A 2Repository: Kuadrant/kuadrant-operator
Length of output: 602
🏁 Script executed:
# Check if there are any integration tests that validate generated CEL
find . -path '*/test*' -name '*.go' -exec grep -l 'OIDCPolicy.*Predicate\|CelPredicate.*iss' {} \; | head -5Repository: Kuadrant/kuadrant-operator
Length of output: 52
🏁 Script executed:
# Look for any documentation about auth.identity direct bracket access
find . -name '*.md' -exec grep -l 'auth\.identity\[' {} \; | head -5Repository: Kuadrant/kuadrant-operator
Length of output: 52
Escape unescaped claim keys and values in CEL expressions at line 312.
Interpolating claim key k and value v directly into the CEL predicate allows special characters to break the expression. A claim value with quotes or backslashes can cause syntax errors, and non-identifier claim names (e.g., custom:role, urn:name) fail with dot notation. Use bracket notation with JSON-encoded string literals instead.
Proposed fix
+func celStringLiteral(value string) string {
+ encoded, _ := json.Marshal(value)
+ return string(encoded)
+}
+
func buildMainAuthPolicy(pol *v1alpha1.OIDCPolicy, igw *ingressGatewayInfo) (*kuadrantv1.AuthPolicy, error) {
authorizeURL, err := pol.GetAuthorizeURL(igw.GetURL())
if err != nil {
return nil, err
@@
authPatterns = append(authPatterns, authorinov1beta3.PatternExpressionOrRef{
CelPredicate: authorinov1beta3.CelPredicate{
- Predicate: fmt.Sprintf(`auth.identity.%s == "%s"`, k, v),
+ Predicate: fmt.Sprintf(`auth.identity[%s] == %s`, celStringLiteral(k), celStringLiteral(v)),
},
})Test with claim names containing non-identifier characters (e.g., custom:role) and claim values with quotes to ensure the generated predicates are syntactically valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go`
around lines 303 - 314, The CEL predicate is built by interpolating claim key
`k` and value `v` directly (in the loop that appends to `authPatterns` in
oidcpolicy_reconciler.go), which breaks for non-identifier claim names and
unescaped characters; change the predicate to use bracket notation and
JSON-encode/escape both the claim key and claim value (e.g.,
auth.identity[<json-encoded-key>] == <json-encoded-value>) when constructing the
`authorinov1beta3.CelPredicate.Predicate` string so keys like `custom:role` and
values with quotes/backslashes are safe; locate the loop that appends to
`authPatterns` and replace the fmt.Sprintf interpolation with
JSON/string-quoting of `k` and `v` before building the predicate.
9d843c7 to
459a12a
Compare
maksymvavilov
left a comment
There was a problem hiding this comment.
looks good. @didierofrivia for a sanity check
OIDCPolicy's claim -> CEL lowering emitted: fmt.Sprintf(`"%s" in auth.identity.%s`, v, k) which expands to e.g. `"true" in auth.identity.email_verified`. That shape only type-checks when the claim is a list or map value; on scalar claims (`sub`, `iss`, `email_verified`, most OIDC standard claims) authorino logs an authorization denial with `no such overload`, so any OIDCPolicy whose `auth.claims` named a scalar claim denied every request. Claims are declared as `map[string]string`, so the caller's intent is unambiguous: "claim X equals string V". Compile that directly as equality instead of set containment: fmt.Sprintf(`auth.identity.%s == "%s"`, k, v) This matches the reporter's expected predicate in Kuadrant#1625 and fixes the scalar-claim use case that was broken since the policy shipped. If/when list-valued claims need their own predicate style that is a separate API shape (`map[string][]string`, or a dedicated `containsClaims` field) — either will sit alongside this equality form, not replace it. Fixes Kuadrant#1625 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
459a12a to
3de1704
Compare
didierofrivia
left a comment
There was a problem hiding this comment.
Hi @SAY-5, thanks for the contribution! The fix should also work with list values in claims. Could you write some tests when the final fix is in place? thanks!
|
Also, about the Coderabbit complaining - while I do agree that this should be fixed, I'd rather see it in a separate issue #2036 |
…equality Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
|
Done in 7c1737c. The predicate now matches list-valued claims by membership while keeping equality for scalars: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go (1)
40-92: ⚡ Quick winExpand test coverage to include edge cases.
The current test cases verify basic scalar equality and list membership, but miss several edge cases that would improve confidence in the predicate logic:
- Missing claims: What happens when
identitydoesn't contain the claim key?- Empty lists: Does
"admin" in []evaluate correctly?- Non-string types: How does the predicate handle numeric or boolean claim values (not as strings)?
- Special characters in values: Test a value with quotes or backslashes to verify escaping (or expose the lack thereof before
#2036is addressed).These cases would help catch potential runtime failures in authorino's CEL evaluation.
📋 Proposed additional test cases
+ { + name: "missing claim returns false", + claim: "role", + value: "admin", + identity: map[string]interface{}{"email": "user@example.com"}, + want: false, + }, + { + name: "empty list returns false", + claim: "groups", + value: "admin", + identity: map[string]interface{}{"groups": []interface{}{}}, + want: false, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go` around lines 40 - 92, TestClaimPredicate lacks coverage for missing claims, empty lists, non-string typed claim values, and values with special characters; add subtests to the existing cases slice in TestClaimPredicate that call evalClaimPredicate with: (1) an identity missing the claim key to assert false, (2) a claim key present with an empty []interface{} and the sought value to assert false, (3) numeric and boolean claim values (e.g., 42 and true) while passing corresponding string/value expectations to assert correct behavior, and (4) a value containing quotes/backslashes to ensure it's evaluated consistently; update the cases array used by the t.Run loop so all new scenarios exercise evalClaimPredicate and assert the expected boolean outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go`:
- Around line 285-293: The predicate built by claimPredicate currently accesses
auth.identity.<k> directly; update claimPredicate to guard existence first by
prefixing the expression with has(auth.identity.<k>) (or an equivalent
has(auth.identity) && has(auth.identity.<k>)) so the equality check in
claimPredicate and the type/membership branch only run when the claim exists;
modify the fmt.Sprintf format string in claimPredicate to include that has(...)
guard around auth.identity.%s before the existing checks.
---
Nitpick comments:
In
`@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go`:
- Around line 40-92: TestClaimPredicate lacks coverage for missing claims, empty
lists, non-string typed claim values, and values with special characters; add
subtests to the existing cases slice in TestClaimPredicate that call
evalClaimPredicate with: (1) an identity missing the claim key to assert false,
(2) a claim key present with an empty []interface{} and the sought value to
assert false, (3) numeric and boolean claim values (e.g., 42 and true) while
passing corresponding string/value expectations to assert correct behavior, and
(4) a value containing quotes/backslashes to ensure it's evaluated consistently;
update the cases array used by the t.Run loop so all new scenarios exercise
evalClaimPredicate and assert the expected boolean outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca6fc3d2-adb8-459b-8586-398c9265c586
📒 Files selected for processing (2)
cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.gocmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go
| // claimPredicate builds the CEL predicate that matches a configured claim against | ||
| // the JWT identity. A scalar claim (the common case, e.g. email, sub, email_verified) | ||
| // is matched by equality; a list-valued claim (e.g. groups, roles) is matched by | ||
| // membership. The combined expression covers both because CEL `==` between mismatched | ||
| // types yields false rather than erroring, so the `in` branch only applies when the | ||
| // claim is actually a list. | ||
| func claimPredicate(k, v string) string { | ||
| return fmt.Sprintf(`auth.identity.%s == "%s" || (type(auth.identity.%s) == list && "%s" in auth.identity.%s)`, k, v, k, v, k) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for authorino CEL evaluation examples with missing identity fields.
# Look for tests or documentation showing behavior of auth.identity.<missing_key>
rg -nP --type=go -C5 'auth\.identity\.\w+.*missing|optional.*auth\.identity'Repository: Kuadrant/kuadrant-operator
Length of output: 52
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all CEL expressions using auth.identity.<...>
rg -n --type=go 'auth\.identity\.[A-Za-z0-9_]+'Repository: Kuadrant/kuadrant-operator
Length of output: 12460
🌐 Web query:
Authorino CEL auth.identity missing field dot notation returns false or error
💡 Result:
In Authorino (as in standard Common Expression Language), accessing a missing field using dot notation (e.g., auth.identity.missing_field) on a message type will typically result in a runtime error (no_such_field) rather than returning false [1][2]. If you need to safely check for the existence or value of a field that might be missing, you should use the has macro or explicit null/existence checks [1][2]. Key Technical Details: 1. Runtime Errors: According to the CEL specification, if an expression evaluates to a message and a field is not declared (or not present), the CEL runtime raises a no_such_field error [1]. 2. Safe Checking with has: You can use the has(e.f) macro to test for the presence of a field without triggering an error [1]. 3. Map Access: If the object being accessed is treated as a map (e.g., certain identity claims or headers), accessing a missing key may behave differently depending on the specific implementation, but it often returns null or results in an error [1][2]. 4. Best Practices: To avoid unexpected evaluation failures in your AuthConfig policies, always ensure that your expressions account for the potential absence of fields, especially when dealing with dynamic identity metadata [2]. For example, instead of using auth.identity.custom_claim directly, you can use: has(auth.identity.custom_claim) && auth.identity.custom_claim == 'expected_value' [1]
Citations:
- 1: https://github.com/google/cel-spec/blob/v0.7.0/doc/langdef.md
- 2: https://docs.kuadrant.io/1.4.x/kuadrant-operator/doc/cel/introduction/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect CEL identity handling implementation and any safety checks for missing fields.
rg -n --type=go "auth\.identity|no_such_field|has\(auth\.identity|has\(" internal/cel cmd/extensions/oidc-policy/internal/controller -S || true
# Look at the CEL transformer code that builds/uses expressions like auth.identity.<field>.
fd -a --type f internal/cel -t go | head -n 50Repository: Kuadrant/kuadrant-operator
Length of output: 1835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections of transformer tests and implementation (small, targeted reads).
# (We rely on the previously found transformer_test.go locations.)
sed -n '1,220p' internal/cel/transformer_test.go
# Find transformer implementation files
fd -a --type f "internal/cel" -t go | sed -n '1,120p'Repository: Kuadrant/kuadrant-operator
Length of output: 4875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find CEL transformer implementation
rg -n "func TransformCounterVariable|TransformCounterVariable\(" internal/cel -S
fd -a --type f internal/cel -t go || trueRepository: Kuadrant/kuadrant-operator
Length of output: 1614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the transformer and any identity/dot-notation rewrite logic
rg -n "auth\.identity\." internal/cel -S || true
rg -n "no_such_field|has\(" internal/cel -S || trueRepository: Kuadrant/kuadrant-operator
Length of output: 1096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate transformer source files and print them in targeted ranges
fd -e go -a internal/cel | head -n 50Repository: Kuadrant/kuadrant-operator
Length of output: 424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read likely relevant transformer file(s)
# (We use fd to list .go files first, then show the full file if small, else partial.)
files=$(fd -e go -a internal/cel)
echo "$files" | head -n 20Repository: Kuadrant/kuadrant-operator
Length of output: 424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the transformer implementation
sed -n '1,220p' internal/cel/transformer.go
sed -n '220,520p' internal/cel/transformer.go
# 2) Check how identity fields are accessed in repo predicates: look for has(auth.identity.*) / has(auth.identity)
rg -n --type=go 'has\(auth\.identity' .
rg -n --type=go 'type\(auth\.identity\.' .
rg -n --type=go "'[^']+' in auth\.identity" .
rg -n --type=go 'auth\.identity\.[A-Za-z0-9_]+' cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go internal/controller tests internal/cel -S || true
# 3) Inspect the claimPredicate and surrounding logic in the OIDC policy reconciler
sed -n '240,380p' cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.goRepository: Kuadrant/kuadrant-operator
Length of output: 19643
Add an existence guard for auth.identity.<claim> in claimPredicate
claimPredicate currently evaluates auth.identity.<k> twice in auth.identity.<k> == "<v>" and type(auth.identity.<k>) == list ... without checking the field exists. CEL/Authorino commonly requires has(...)-style guards for fields that may be absent (this repo already uses has(auth.identity) in CEL expressions), so the predicate should short-circuit with an has(auth.identity.<k>) (or equivalent) condition before doing the equality / list-membership checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go`
around lines 285 - 293, The predicate built by claimPredicate currently accesses
auth.identity.<k> directly; update claimPredicate to guard existence first by
prefixing the expression with has(auth.identity.<k>) (or an equivalent
has(auth.identity) && has(auth.identity.<k>)) so the equality check in
claimPredicate and the type/membership branch only run when the claim exists;
modify the fmt.Sprintf format string in claimPredicate to include that has(...)
guard around auth.identity.%s before the existing checks.
|
@didierofrivia gentle ping. List-valued claims are now matched by membership alongside scalar equality (7c1737c), and the unit test covers scalar match/mismatch, boolean-as-string, and list contains/missing. CI is green. Let me know if anything else is needed. |
| // types yields false rather than erroring, so the `in` branch only applies when the | ||
| // claim is actually a list. | ||
| func claimPredicate(k, v string) string { | ||
| return fmt.Sprintf(`auth.identity.%s == "%s" || (type(auth.identity.%s) == list && "%s" in auth.identity.%s)`, k, v, k, v, k) |
There was a problem hiding this comment.
ClaimPredicate function should check existence for claim.identity and claim.identity.%s before doing the checks
What
oidcpolicy_reconciler.golowers everyauth.claimsentry into a CEL predicate via:which expands to e.g.
"true" in auth.identity.email_verified. That shape only type-checks when the claim is a list/map value; on scalar claims (sub,iss,email_verified,azp,name, the majority of OIDC standard claims) authorino denies the request with:So any
OIDCPolicywhoseauth.claimsreferences a scalar claim silently denies every request. Per #1625.Fix
Claims are declared as
map[string]string, so the caller's intent is unambiguous: "claim X equals string V". Compile that directly as equality instead of set containment:auth.identity.email_verified == "true").inform never worked end-to-end against a scalar, so this isn't breaking a behaviour users could have been relying on, it's fixing the code path to match the declared API.Not in this PR
If/when list-valued claims (e.g.
groups,roles) need their own predicate style, that's a separate API shape, either wideningClaimstomap[string][]stringor adding a dedicatedcontainsClaimsfield. Either will sit alongside this equality form rather than replacing it.Fixes #1625
Summary by CodeRabbit
Bug Fixes
Tests