From 0896b90b8be18da37b604e15de644032852cebb5 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Mon, 27 Jul 2026 12:26:02 +0545 Subject: [PATCH] fix: reject enable-csrf without csrf-key in the Ingress admission webhook 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. --- internal/webhook/v1/ingress_webhook.go | 23 +++++++++++ internal/webhook/v1/ingress_webhook_test.go | 45 +++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/internal/webhook/v1/ingress_webhook.go b/internal/webhook/v1/ingress_webhook.go index a2f9f2d4..4c198bde 100644 --- a/internal/webhook/v1/ingress_webhook.go +++ b/internal/webhook/v1/ingress_webhook.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations" "github.com/apache/apisix-ingress-controller/internal/controller" "github.com/apache/apisix-ingress-controller/internal/webhook/v1/reference" sslvalidator "github.com/apache/apisix-ingress-controller/internal/webhook/v1/ssl" @@ -82,6 +83,10 @@ func (v *IngressCustomValidator) ValidateCreate(ctx context.Context, obj runtime return nil, fmt.Errorf("%s", sslvalidator.FormatConflicts(conflicts)) } + if err := validateAnnotations(ingress); err != nil { + return nil, err + } + warnings := v.collectReferenceWarnings(ctx, ingress) return warnings, nil } @@ -103,10 +108,28 @@ func (v *IngressCustomValidator) ValidateUpdate(ctx context.Context, oldObj, new return nil, fmt.Errorf("%s", sslvalidator.FormatConflicts(conflicts)) } + if err := validateAnnotations(ingress); err != nil { + return nil, err + } + warnings := v.collectReferenceWarnings(ctx, ingress) return warnings, nil } +// validateAnnotations rejects annotation combinations that would otherwise be +// silently dropped, leaving the route without a requested security plugin. +// enable-csrf with no csrf-key is refused here so kubectl apply fails loudly +// instead of programming a route the operator believes is protected. +func validateAnnotations(ingress *networkingv1.Ingress) error { + e := annotations.NewExtractor(ingress.Annotations) + if e.GetBoolAnnotation(annotations.AnnotationsEnableCsrf) && + e.GetStringAnnotation(annotations.AnnotationsCsrfKey) == "" { + return fmt.Errorf("annotation %q is enabled but %q is missing or empty", + annotations.AnnotationsEnableCsrf, annotations.AnnotationsCsrfKey) + } + return nil +} + // ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type Ingress. func (v *IngressCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { return nil, nil diff --git a/internal/webhook/v1/ingress_webhook_test.go b/internal/webhook/v1/ingress_webhook_test.go index d6bce33f..0a5481c4 100644 --- a/internal/webhook/v1/ingress_webhook_test.go +++ b/internal/webhook/v1/ingress_webhook_test.go @@ -113,3 +113,48 @@ func TestIngressCustomValidator_NoWarningsWhenReferencesExist(t *testing.T) { require.NoError(t, err) assert.Empty(t, warnings) } + +func csrfIngress(anno map[string]string) *networkingv1.Ingress { + return &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ingress", Namespace: "default", Annotations: anno}, + Spec: networkingv1.IngressSpec{}, + } +} + +func TestIngressCustomValidator_RejectsEnableCsrfWithoutKey(t *testing.T) { + validator := buildIngressValidator(t) + + // missing csrf-key + _, err := validator.ValidateCreate(context.Background(), csrfIngress(map[string]string{ + "k8s.apisix.apache.org/enable-csrf": "true", + })) + require.Error(t, err) + + // empty csrf-key + _, err = validator.ValidateCreate(context.Background(), csrfIngress(map[string]string{ + "k8s.apisix.apache.org/enable-csrf": "true", + "k8s.apisix.apache.org/csrf-key": "", + })) + require.Error(t, err) + + // the same invalid update must also be rejected, not only create + old := csrfIngress(nil) + _, err = validator.ValidateUpdate(context.Background(), old, csrfIngress(map[string]string{ + "k8s.apisix.apache.org/enable-csrf": "true", + })) + require.Error(t, err) +} + +func TestIngressCustomValidator_AllowsEnableCsrfWithKey(t *testing.T) { + validator := buildIngressValidator(t) + + _, err := validator.ValidateCreate(context.Background(), csrfIngress(map[string]string{ + "k8s.apisix.apache.org/enable-csrf": "true", + "k8s.apisix.apache.org/csrf-key": "my-secret-key", + })) + require.NoError(t, err) + + // csrf not enabled: key absence is fine + _, err = validator.ValidateCreate(context.Background(), csrfIngress(nil)) + require.NoError(t, err) +}