Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions internal/webhook/v1/ingress_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

return nil, err
}

warnings := v.collectReferenceWarnings(ctx, ingress)
return warnings, nil
}
Expand All @@ -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
Expand Down
45 changes: 45 additions & 0 deletions internal/webhook/v1/ingress_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Loading