fix: fail loud when enable-csrf is set but csrf-key annotation is missing - #440
fix: fail loud when enable-csrf is set but csrf-key annotation is missing#440shreemaan-abhishek wants to merge 3 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:
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)
📝 WalkthroughWalkthroughIngress webhook create and update validation now reject enabled CSRF without a non-empty key. The CSRF translator plugin applies the same error behavior, with tests covering missing, empty, valid, and disabled-CSRF configurations. ChangesIngress CSRF validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-30T07:11:39Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.6.0
implementation:
contact:
- https://github.com/apache/apisix-ingress-controller/issues
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: partial
skippedTests:
- HTTPRouteHTTPSListener
- HTTPRouteInvalidBackendRefUnknownKind
- HTTPRouteInvalidCrossNamespaceBackendRef
- HTTPRouteInvalidNonExistentBackendRef
- HTTPRouteListenerHostnameMatching
- HTTPRouteMultipleGateways
- HTTPRouteNoBackendRefs
statistics:
Failed: 0
Passed: 30
Skipped: 7
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 12
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- BackendTLSPolicy
- BackendTLSPolicySANValidation
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRoute303RedirectStatusCode
- HTTPRoute307RedirectStatusCode
- HTTPRoute308RedirectStatusCode
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteCORS
- HTTPRouteNamedRouteRule
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
- HTTPRouteRetry
- HTTPRouteRetryBackendTimeout
- HTTPRouteRetryConnectionError
- ListenerSet
name: GATEWAY-HTTP
summary: Core tests partially succeeded with 7 test skips. Extended tests partially
succeeded with 1 test skips.
- core:
result: partial
skippedTests:
- GRPCRouteListenerHostnameMatching
statistics:
Failed: 0
Passed: 14
Skipped: 1
extended:
result: success
statistics:
Failed: 0
Passed: 1
Skipped: 0
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
unsupportedFeatures:
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- ListenerSet
name: GATEWAY-GRPC
summary: Core tests partially succeeded with 1 test skips. Extended tests succeeded.
- core:
result: partial
skippedTests:
- TLSRouteHostnameIntersection
- TLSRouteInvalidBackendRefNonexistent
- TLSRouteInvalidBackendRefUnknownKind
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 16
Skipped: 4
extended:
result: partial
skippedTests:
- TLSRouteTerminateSimpleSameNamespace
statistics:
Failed: 0
Passed: 3
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- TLSRouteModeTerminate
unsupportedFeatures:
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- ListenerSet
- TLSRouteModeMixed
name: GATEWAY-TLS
summary: Core tests partially succeeded with 4 test skips. Extended tests partially
succeeded with 1 test skips.
succeededProvisionalTests:
- GatewayOptionalAddressValue |
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-30T07:11:41Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.6.0
implementation:
contact:
- https://github.com/apache/apisix-ingress-controller/issues
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: partial
skippedTests:
- HTTPRouteHTTPSListener
- HTTPRouteInvalidBackendRefUnknownKind
- HTTPRouteInvalidCrossNamespaceBackendRef
- HTTPRouteInvalidNonExistentBackendRef
- HTTPRouteListenerHostnameMatching
- HTTPRouteMultipleGateways
- HTTPRouteNoBackendRefs
statistics:
Failed: 0
Passed: 30
Skipped: 7
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 12
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- BackendTLSPolicy
- BackendTLSPolicySANValidation
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRoute303RedirectStatusCode
- HTTPRoute307RedirectStatusCode
- HTTPRoute308RedirectStatusCode
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteCORS
- HTTPRouteNamedRouteRule
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
- HTTPRouteRetry
- HTTPRouteRetryBackendTimeout
- HTTPRouteRetryConnectionError
- ListenerSet
name: GATEWAY-HTTP
summary: Core tests partially succeeded with 7 test skips. Extended tests partially
succeeded with 1 test skips.
- core:
result: partial
skippedTests:
- GRPCRouteListenerHostnameMatching
statistics:
Failed: 0
Passed: 14
Skipped: 1
extended:
result: success
statistics:
Failed: 0
Passed: 1
Skipped: 0
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
unsupportedFeatures:
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- ListenerSet
name: GATEWAY-GRPC
summary: Core tests partially succeeded with 1 test skips. Extended tests succeeded.
- core:
result: partial
skippedTests:
- TLSRouteHostnameIntersection
- TLSRouteInvalidBackendRefNonexistent
- TLSRouteInvalidBackendRefUnknownKind
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 16
Skipped: 4
extended:
result: partial
skippedTests:
- TLSRouteTerminateSimpleSameNamespace
statistics:
Failed: 0
Passed: 3
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- TLSRouteModeTerminate
unsupportedFeatures:
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- ListenerSet
- TLSRouteModeMixed
name: GATEWAY-TLS
summary: Core tests partially succeeded with 4 test skips. Extended tests partially
succeeded with 1 test skips.
succeededProvisionalTests:
- GatewayOptionalAddressValue |
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-30T07:31:35Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.6.0
implementation:
contact:
- https://github.com/apache/apisix-ingress-controller/issues
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
- HTTPRouteInvalidBackendRefUnknownKind
- HTTPRouteMultipleGateways
- HTTPRouteNoBackendRefs
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 4
Passed: 32
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 12
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- BackendTLSPolicy
- BackendTLSPolicySANValidation
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRoute303RedirectStatusCode
- HTTPRoute307RedirectStatusCode
- HTTPRoute308RedirectStatusCode
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteCORS
- HTTPRouteNamedRouteRule
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
- HTTPRouteRetry
- HTTPRouteRetryBackendTimeout
- HTTPRouteRetryConnectionError
- ListenerSet
name: GATEWAY-HTTP
summary: Core tests failed with 4 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
failedTests:
- GatewayModifyListeners
result: failure
statistics:
Failed: 1
Passed: 14
Skipped: 0
extended:
result: success
statistics:
Failed: 0
Passed: 1
Skipped: 0
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
unsupportedFeatures:
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- ListenerSet
name: GATEWAY-GRPC
summary: Core tests failed with 1 test failures. Extended tests succeeded.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteHostnameIntersection
- TLSRouteInvalidBackendRefNonexistent
- TLSRouteInvalidBackendRefUnknownKind
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 5
Passed: 15
Skipped: 0
extended:
failedTests:
- TLSRouteTerminateSimpleSameNamespace
result: failure
statistics:
Failed: 1
Passed: 3
Skipped: 0
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- TLSRouteModeTerminate
unsupportedFeatures:
- GatewayBackendClientCertificate
- GatewayFrontendClientCertificateValidation
- GatewayFrontendClientCertificateValidationInsecureFallback
- GatewayHTTPListenerIsolation
- GatewayHTTPSListenerDetectMisdirectedRequests
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- ListenerSet
- TLSRouteModeMixed
name: GATEWAY-TLS
summary: Core tests failed with 5 test failures. Extended tests failed with 1 test
failures.
succeededProvisionalTests:
- GatewayOptionalAddressValue |
| if key == "" { | ||
| return nil, nil | ||
| // csrf requested but the key is missing: fail loud instead of | ||
| // silently dropping the plugin and programming the route unprotected. |
There was a problem hiding this comment.
This still fails open at the route level. plugins.Parse logs handler errors and continues, then returns a nil error, so this error never reaches reconciliation and the route is still programmed without the csrf plugin. The linked finding explicitly requires translation to fail rather than only adding a log entry. Please propagate the handler error from plugins.Parse and cover the parser/reconcile path so enable-csrf=true cannot reconcile successfully without a key.
There was a problem hiding this comment.
You are right, and I have pushed the full fix.
Confirming your diagnosis: the error was swallowed and never reached reconciliation. I traced it and there were three swallow points, not one:
plugins.Parse- logged the handler error,continue, returned a nil error.TranslateIngressAnnotations- logged the parser error and returned the config anyway.TranslateIngress- had no error to inspect, since the function above returned only a config.
One note on scope, because the diff is wider than the single change you asked for. Propagating from plugins.Parse alone would have made things worse: on error Parse returns nil for the entire plugin map, which also holds key-auth, basic-auth, ip-restriction and forward-auth. With points 2 and 3 still swallowing, the route would still have been programmed, now stripped of authentication and IP restrictions as well. A one-plugin gap would have become an all-plugins gap. So points 2 and 3 had to move together with point 1 for point 1 to be safe.
With all three propagating, TranslateIngress now returns the error, and the provider aborts at provider.go:154 before the sync task is built, so the route is never programmed and the failure surfaces on the reconcile.
Two things worth flagging for your review:
- Behavior change beyond csrf. Any annotation handler error now fails translation. That includes the
upstreamparser, so an invalidupstream-scheme/retry/timeout annotation now fails the Ingress instead of being silently ignored. I think this is correct and the same bug class (a typo'dhttpssilently fell back to plaintext http), but it will turn previously-"working" typo'd Ingresses red on upgrade. Happy to narrow it to plugin handlers only if you would rather not carry that. - I updated the existing
invalid schemecase inTestTranslateIngressAnnotationsto expect an error for that reason, and added parser-level cases covering both missing and emptycsrf-key.
Full unit suite passes. The same change is in the upstream PR (apache/apisix-ingress-controller#2813).
There was a problem hiding this comment.
Thanks, the error now reaches reconciliation, but the update path still fails open. If an existing Ingress has already programmed a route without CSRF and is then updated with enable-csrf: "true" but no key, Provider.Update returns on this error before calling UpdateConfig; it does not delete or replace the previous resources, so the old unprotected route remains in the ADC cache and data plane. The translator-only tests cannot catch this. Please reject the invalid update in admission or program a fail-closed replacement, and cover an update from an already-served route.
There was a problem hiding this comment.
@jarvis9443 you are right about the update path, thanks. Confirmed Provider.Update returns on the translation error before UpdateConfig, so the previously-programmed unprotected route survived in the ADC cache and data plane, and translator-only tests could not catch it.
I have moved enforcement to the Ingress admission webhook (ValidateCreate + ValidateUpdate): enable-csrf: true with no key is now rejected at apply time for updates as well as creates, so the already-served case is covered. The translation-failure change is reverted. Added a webhook test that exercises an update from a route with no csrf annotations into the invalid enable-csrf-without-key state.
Note the Ingress webhook is deployed with failurePolicy: Ignore, so this is a best-effort gate; hardening that bypass is out of scope here. Same change is in the upstream PR apache/apisix-ingress-controller#2813.
There was a problem hiding this comment.
Done in df885cd. csrf.Handle now returns an error when the key is missing, so plugins.Parse logs a breadcrumb for the cases the webhook cannot cover (webhook off, or a pre-upgrade Ingress). Parse skips only the csrf plugin, so the rest of the route is untouched. The webhook stays the enforcement point. Mirrors apache/apisix-ingress-controller#2813.
…hook FINDING-031: when enable-csrf is set but csrf-key is missing or empty, the csrf plugin was silently dropped and the route programmed without CSRF protection while the Ingress reconciled cleanly. Enforce this in the Ingress validating webhook (create and update) so the invalid combination is rejected at kubectl apply, loudly and synchronously, for both new and updated Ingresses. This replaces the earlier translation-failure approach, which: - did not cover the update path (an already-served unprotected route survived, since Provider.Update returns before UpdateConfig); - over-reached: any annotation error, e.g. a typo'd upstream-scheme, failed the whole Ingress including its TLS; - could not truly fail closed anyway, since dropping a route lets traffic fall through to any broader wildcard route. The webhook is deployed with failurePolicy: Ignore, so this is a best-effort gate; hardening that bypass is tracked separately.
2d1b3ba to
0896b90
Compare
| return nil, fmt.Errorf("%s", sslvalidator.FormatConflicts(conflicts)) | ||
| } | ||
|
|
||
| if err := validateAnnotations(ingress); err != nil { |
There was a problem hiding this comment.
[P1] This guard only runs when the admission webhook is enabled, but the shipped configuration defaults webhook.enable to false. In default deployments, ValidateCreate and ValidateUpdate never run and the translator still silently drops csrf, so both create and update remain unprotected. failurePolicy: Ignore is not the only bypass. Please enforce this invariant in a path that always runs, with fail-closed behavior for updates, or make admission mandatory for this feature.
The webhook is the enforcement point, but it cannot cover every case: webhook disabled, or an Ingress created before the upgrade that never gets re-admitted. Return an error from csrf.Handle so plugins.Parse emits a log line for those cases instead of dropping the plugin in total silence. Parse skips only the csrf plugin, so the rest of the route is unaffected.
validateAnnotations is a pure in-memory check, so run it before DetectConflicts, which does List calls and Secret reads. Reject an invalid annotation combination without the network round-trips.
3 participants
What this PR does
Fixes FINDING-031. When
enable-csrf: "true"is set on an Ingress butcsrf-keyis missing or empty, the csrf plugin was silently dropped: the route was programmed without CSRF protection and the Ingress reconciled cleanly, so an endpoint the operator believed was protected ran unprotected with no error surfaced.Approach (changed after review)
Enforce the rule in the Ingress validating admission webhook (
ingress_webhook.go), for bothValidateCreateandValidateUpdate.enable-csrf: truewith an empty/missingcsrf-keyis now a hard error, sokubectl applyis rejected loudly and synchronously, before the object is ever stored, for both new and updated Ingresses.This replaces the earlier translation-failure approach, per review feedback from @AlinsRan and @nic-6443. That approach was reverted because:
enable-csrfwith no key,Provider.Updatereturned on the translation error beforeUpdateConfig, so the old unprotected route stayed live in the data plane (raised by @nic-6443).{Service, SSL}together, so failing the whole translation on any annotation error meant a typo in a non-security annotation (e.g.upstream-scheme) took down the Ingress including its TLS (raised by @AlinsRan).The webhook can enforce severity properly (hard error vs warning) and blocks the bad intent at the source, which the translation layer structurally cannot.
Known limitation
The Ingress webhook is deployed with
failurePolicy: Ignore, so if the webhook is unavailable the invalid Ingress is admitted. This is a best-effort gate, not an unbreakable boundary; hardening that bypass is a separate concern (same class as the webhook-availability finding).Test
Added to
ingress_webhook_test.go:enable-csrfwithout a key, and with an empty key, are rejected on create.enable-csrfwith a key, and csrf-not-enabled, are allowed.Synced with apache/apisix-ingress-controller#2813.
Summary by CodeRabbit