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") } diff --git a/internal/webhook/v1/ingress_webhook.go b/internal/webhook/v1/ingress_webhook.go index a2f9f2d4..caf1beb4 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" @@ -76,6 +77,10 @@ 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 { @@ -97,6 +102,10 @@ 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 { @@ -107,6 +116,20 @@ func (v *IngressCustomValidator) ValidateUpdate(ctx context.Context, oldObj, new 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) +}