From 0896b90b8be18da37b604e15de644032852cebb5 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Mon, 27 Jul 2026 12:26:02 +0545 Subject: [PATCH 1/3] 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) +} From df885cdedbc9e4f6f89e1fc30c6094f7c97f7702 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 30 Jul 2026 12:21:42 +0545 Subject: [PATCH 2/3] fix: log when enable-csrf is set without a csrf-key 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. --- internal/adc/translator/annotations/plugins/csrf.go | 8 +++++++- .../adc/translator/annotations/plugins/csrf_test.go | 10 ++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/internal/adc/translator/annotations/plugins/csrf.go b/internal/adc/translator/annotations/plugins/csrf.go index 1bee407c..811f4753 100644 --- a/internal/adc/translator/annotations/plugins/csrf.go +++ b/internal/adc/translator/annotations/plugins/csrf.go @@ -16,6 +16,8 @@ package plugins import ( + "fmt" + adctypes "github.com/apache/apisix-ingress-controller/api/adc" "github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations" ) @@ -39,7 +41,11 @@ func (c *csrf) Handle(e annotations.Extractor) (any, error) { key := e.GetStringAnnotation(annotations.AnnotationsCsrfKey) if key == "" { - return nil, nil + // csrf requested without a key: the webhook rejects this, but log a + // breadcrumb for the cases it cannot cover (webhook off, pre-upgrade + // Ingress). Parse skips only this plugin, the rest of the route stands. + return nil, fmt.Errorf("annotation %q is enabled but %q is missing or empty", + annotations.AnnotationsEnableCsrf, annotations.AnnotationsCsrfKey) } return &adctypes.CSRFConfig{ diff --git a/internal/adc/translator/annotations/plugins/csrf_test.go b/internal/adc/translator/annotations/plugins/csrf_test.go index 0f681703..e41f8cf5 100644 --- a/internal/adc/translator/annotations/plugins/csrf_test.go +++ b/internal/adc/translator/annotations/plugins/csrf_test.go @@ -43,10 +43,16 @@ func TestCSRFHandler(t *testing.T) { assert.Nil(t, err, "checking given error") assert.Nil(t, out, "checking given output") - // Test with enable-csrf true but no key + // Test with enable-csrf true but no key: errors so Parse logs a breadcrumb anno[annotations.AnnotationsEnableCsrf] = "true" delete(anno, annotations.AnnotationsCsrfKey) out, err = p.Handle(annotations.NewExtractor(anno)) - assert.Nil(t, err, "checking given error") + assert.Error(t, err, "expecting an error when csrf-key is missing") assert.Nil(t, out, "checking given output when key is missing") + + // Test with enable-csrf true but empty key: same behavior + anno[annotations.AnnotationsCsrfKey] = "" + out, err = p.Handle(annotations.NewExtractor(anno)) + assert.Error(t, err, "expecting an error when csrf-key is empty") + assert.Nil(t, out, "checking given output when key is empty") } From 167aed95241689f4ee33433baee3a4271fd43147 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 30 Jul 2026 12:48:05 +0545 Subject: [PATCH 3/3] refactor: run validateAnnotations before SSL conflict detection 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. --- internal/webhook/v1/ingress_webhook.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/webhook/v1/ingress_webhook.go b/internal/webhook/v1/ingress_webhook.go index 4c198bde..caf1beb4 100644 --- a/internal/webhook/v1/ingress_webhook.go +++ b/internal/webhook/v1/ingress_webhook.go @@ -77,16 +77,16 @@ func (v *IngressCustomValidator) ValidateCreate(ctx context.Context, obj runtime return nil, nil } + if err := validateAnnotations(ingress); err != nil { + return nil, err + } + detector := sslvalidator.NewConflictDetector(v.Client) conflicts := detector.DetectConflicts(ctx, ingress) if len(conflicts) > 0 { 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 } @@ -102,16 +102,16 @@ func (v *IngressCustomValidator) ValidateUpdate(ctx context.Context, oldObj, new return nil, nil } + if err := validateAnnotations(ingress); err != nil { + return nil, err + } + detector := sslvalidator.NewConflictDetector(v.Client) conflicts := detector.DetectConflicts(ctx, ingress) if len(conflicts) > 0 { 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 }