fix: enforce ReferenceGrant on cross-namespace Consumer SecretRef#432
Open
shreemaan-abhishek wants to merge 2 commits into
Open
fix: enforce ReferenceGrant on cross-namespace Consumer SecretRef#432shreemaan-abhishek wants to merge 2 commits into
shreemaan-abhishek wants to merge 2 commits into
Conversation
A v1alpha1 Consumer credential secretRef carries an attacker-settable namespace. The reconciler honored it and read the Secret with the controller's cluster-wide RBAC, no ReferenceGrant check, binding a foreign namespace's auth material to the consumer identity (confused deputy). The route controllers already gate cross-namespace refs via checkReferenceGrant; apply the same gate in the Consumer reconciler and refuse the reference when no grant permits it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConsumer reconciliation now checks ChangesConsumer Secret Reference Authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-20T11:13:25Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips.
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded. |
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-20T11:12:42Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
result: partial
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 0
Passed: 32
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests partially succeeded with 1 test skips. Extended tests partially
succeeded with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-20T11:30:42Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- GatewayModifyListeners
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures.
- core:
failedTests:
- GatewayModifyListeners
result: failure
statistics:
Failed: 1
Passed: 11
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 1 test failures. |
![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
🤖 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 `@internal/webhook/v1/consumer_webhook_test.go`:
- Around line 167-172: Update the ValidateCreate test around
buildConsumerValidator so its fake client fails when Get requests
auth/jwt-secret, preventing the foreign Secret from being served before
ReferenceGrant validation. Preserve the existing error assertion containing “not
permitted by any ReferenceGrant”.
🪄 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: 86667401-87d4-4f82-9437-00651858b187
📒 Files selected for processing (1)
internal/webhook/v1/consumer_webhook_test.go
Comment on lines
+167
to
+172
| secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "jwt-secret", Namespace: "auth"}} | ||
| validator := buildConsumerValidator(t, secret) | ||
|
|
||
| _, err := validator.ValidateCreate(context.Background(), consumer) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "not permitted by any ReferenceGrant") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline internal/webhook/v1/consumer_webhook.go --match ConsumerCustomValidator --view expanded
rg -n -C4 'func \(v \*ConsumerCustomValidator\) collectWarnings|checker\.Secret|adcValidator\.Validate|processSpec' \
internal/webhook/v1/consumer_webhook.go internal/webhook/v1/reference/checker.goRepository: api7/api7-ingress-controller
Length of output: 4073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test around the cited lines
sed -n '1,260p' internal/webhook/v1/consumer_webhook_test.go
echo
echo '--- consumer_webhook.go (relevant slice) ---'
sed -n '115,150p' internal/webhook/v1/consumer_webhook.go
echo
echo '--- reference/checker.go (relevant slice) ---'
sed -n '1,240p' internal/webhook/v1/reference/checker.goRepository: api7/api7-ingress-controller
Length of output: 13339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,95p' internal/webhook/v1/consumer_webhook.go
echo
echo '--- adc admission validator references ---'
rg -n -C3 'type adcAdmissionValidator|func \(v \*adcAdmissionValidator\) Validate|ReferenceGrant|Check.*Grant|Secret\(' internal/webhook/v1 -g '*.go'Repository: api7/api7-ingress-controller
Length of output: 18985
Block the foreign Secret lookup (internal/webhook/v1/consumer_webhook_test.go:167-172)
Because ValidateCreate collects warnings first, the fake client can still serve auth/jwt-secret before the ReferenceGrant denial. Use a client that fails on Get for that Secret and keep the not permitted by any ReferenceGrant assertion.
🤖 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 `@internal/webhook/v1/consumer_webhook_test.go` around lines 167 - 172, Update
the ValidateCreate test around buildConsumerValidator so its fake client fails
when Get requests auth/jwt-secret, preventing the foreign Secret from being
served before ReferenceGrant validation. Preserve the existing error assertion
containing “not permitted by any ReferenceGrant”.
Source: Coding guidelines
…ment The Consumer admission validator shares processSpec via PrepareConsumerForValidation, so the ReferenceGrant gate now also fail-closes at admission. Update the two cross-namespace webhook tests to permit the reference via a ReferenceGrant, and add a test asserting a cross-namespace secretRef without a grant is denied at admission.
shreemaan-abhishek
force-pushed
the
fix/consumer-cross-ns-secretref-grant
branch
from
July 20, 2026 10:59
cea1094 to
5e59ede
Compare
nic-6443
approved these changes
Jul 23, 2026
| } | ||
| // A cross-namespace SecretRef needs a ReferenceGrant, same as routes. | ||
| secretNS := gatewayv1.Namespace(ns) | ||
| if permitted := checkReferenceGrant(ctx, |
Collaborator
There was a problem hiding this comment.
Verified this against the paired OSS PR (apache/apisix-ingress-controller#2805) — the two diffs are byte-identical apart from hunk offsets, so the sync side is fine.
The fix itself holds up: PrepareConsumerForValidation calls processSpec, so this one change point gates the admission webhook as well, which is why the webhook test passes without any webhook code change. Reverting just this hunk makes both the controller and webhook tests fail, so the tests do pin the invariant.
Worth a release note though: checkReferenceGrant returns false for a cross-namespace ref when enableReferenceGrant is off, and that flag comes from whether the ReferenceGrant CRD is installed (manager/run.go). On a cluster without that CRD the behaviour goes from "cross-namespace secretRef works" to "always refused" — right direction, and consistent with the route controllers, but existing users of cross-namespace Consumer secretRef will need both the CRD and a grant after upgrade.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes api7/rfcs#138 (Sec Review FINDING-004).
What this fixes
A
v1alpha1Consumercredential'ssecretRefcarries a caller-settablenamespace.ConsumerReconciler.processSpechonored it and read the Secret with the controller's cluster-widesecretsRBAC, with noReferenceGrantcheck, thenTranslateConsumerV1alpha1copied the entire foreign Secret's data into the ADC consumer credential and pushed it to the data plane.A user who can only
create consumersin namespace A could namenamespace-B/victim-secretand bind that namespace's auth material (key-auth keys, basic-auth passwords, JWT secrets, HMAC keys) to a Consumer identity they own. The caller never hadget secretsin namespace B; the controller does. Classic cross-namespace confused deputy.The route controllers already gate cross-namespace refs via
checkReferenceGrant; the Consumer path was the one auth pathway that skipped it. Thev2ApisixConsumerAPI is namespace-locked (LocalObjectReference), so onlyv1alpha1was affected.The fix
Enforce
checkReferenceGrantbefore dereferencing a cross-namespace ConsumersecretRef, matching the route controllers. Same-namespace refs are unaffected (the check short-circuits). Without a permitting grant the reconcile refuses the reference and surfaces the error on the Consumer status, so the foreign Secret is never loaded or bound.Tests
internal/controller/consumer_controller_test.gocovers cross-namespace denied (no grant), cross-namespace allowed (matching grant), and same-namespace allowed.Sync
Paired open-source fix: apache/apisix-ingress-controller#2805 (identical change). Per the two-PR sync rule, the sync is complete when both merge.
Summary by CodeRabbit
New Features
ReferenceGrantauthorization for cross-namespacesecretRefduring reconciliation and validation.ReferenceGrantexists, while same-namespace references continue to work unchanged.Tests
secretRefbehavior based on grant presence.