diff --git a/internal/webhook/ingress/validate_hostnames.go b/internal/webhook/ingress/validate_hostnames.go index b1205cfc..c6bf46b2 100644 --- a/internal/webhook/ingress/validate_hostnames.go +++ b/internal/webhook/ingress/validate_hostnames.go @@ -136,40 +136,32 @@ func (r *hostnames) validateHostnames(tenant capsulev1beta2.Tenant, hostnames se return nil } - var valid, matched bool - tenantHostnameSet := sets.New[string](tenant.Spec.IngressOptions.AllowedHostnames.Exact...) - var invalidHostnames []string - - if len(hostnames) > 0 { - if diff := hostnames.Difference(tenantHostnameSet); len(diff) > 0 { - invalidHostnames = append(invalidHostnames, diff.UnsortedList()...) - } - - if len(invalidHostnames) == 0 { - valid = true - } - } - - var notMatchingHostnames []string + // Only hostnames outside the exact allow-list still need to be checked. + notAllowedHostnames := hostnames.Difference(tenantHostnameSet).UnsortedList() //nolint:staticcheck - if allowedRegex := tenant.Spec.IngressOptions.AllowedHostnames.Regex; len(allowedRegex) > 0 { - for currentHostname := range hostnames { - matched, _ = regexp.MatchString(allowedRegex, currentHostname) - if !matched { - notMatchingHostnames = append(notMatchingHostnames, currentHostname) + if allowedRegex := tenant.Spec.IngressOptions.AllowedHostnames.Regex; len(allowedRegex) > 0 && len(notAllowedHostnames) > 0 { + var failedRegexHostnames []string + + // compile regex once. if compilation fails, the remaining hostnames are not allowed + re, err := regexp.Compile(allowedRegex) + if err != nil { + return caperrors.NewIngressHostnamesNotValid(notAllowedHostnames, *tenant.Spec.IngressOptions.AllowedHostnames) + } + + for _, currentHostname := range notAllowedHostnames { + if ok := re.MatchString(currentHostname); !ok { + failedRegexHostnames = append(failedRegexHostnames, currentHostname) } } - if len(notMatchingHostnames) == 0 { - matched = true - } + notAllowedHostnames = failedRegexHostnames } - if !valid && !matched { - return caperrors.NewIngressHostnamesNotValid(invalidHostnames, notMatchingHostnames, *tenant.Spec.IngressOptions.AllowedHostnames) + if len(notAllowedHostnames) > 0 { + return caperrors.NewIngressHostnamesNotValid(notAllowedHostnames, *tenant.Spec.IngressOptions.AllowedHostnames) } return nil diff --git a/internal/webhook/ingress/validate_hostnames_test.go b/internal/webhook/ingress/validate_hostnames_test.go new file mode 100644 index 00000000..419c86d1 --- /dev/null +++ b/internal/webhook/ingress/validate_hostnames_test.go @@ -0,0 +1,194 @@ +// Copyright 2020-2026 Project Capsule Authors +// SPDX-License-Identifier: Apache-2.0 + +package ingress + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/sets" + + capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2" + "github.com/projectcapsule/capsule/pkg/api" +) + +// tenantWithAllowedHostnames builds a minimal Tenant carrying the given exact +// allow-list and regex under spec.ingressOptions.allowedHostnames. +func tenantWithAllowedHostnames(exact []string, regex string) capsulev1beta2.Tenant { + return capsulev1beta2.Tenant{ + Spec: capsulev1beta2.TenantSpec{ + IngressOptions: capsulev1beta2.IngressOptions{ + AllowedHostnames: &api.AllowedListSpec{ + Exact: exact, + Regex: regex, + }, + }, + }, + } +} + +func TestValidateHostnames(t *testing.T) { + t.Parallel() + + const ( + appsRegex = `^[a-z0-9-]{3,40}\.apps\.example\.com$` + ) + + tests := []struct { + name string + // tenant carries the allowed hostnames configuration under test. + tenant capsulev1beta2.Tenant + // hostnames are the Ingress hostnames being validated. + hostnames []string + // wantErr is true when the hostnames must be denied. + wantErr bool + // wantDenied lists hostnames that must appear in the denial message. + wantDenied []string + // wantAbsent lists hostnames that must NOT appear as denied (e.g. valid + // via regex but outside the exact list). + wantAbsent []string + }{ + { + name: "no allowed hostnames configured allows everything", + tenant: capsulev1beta2.Tenant{}, + hostnames: []string{"anything.example.com"}, + wantErr: false, + }, + { + name: "empty hostname set is allowed", + tenant: tenantWithAllowedHostnames([]string{"a.example.com"}, ""), + hostnames: nil, + wantErr: false, + }, + { + name: "all hostnames in exact list are allowed", + tenant: tenantWithAllowedHostnames([]string{"a.example.com", "b.example.com"}, ""), + hostnames: []string{"a.example.com", "b.example.com"}, + wantErr: false, + }, + { + name: "hostname outside exact list without regex is denied", + tenant: tenantWithAllowedHostnames([]string{"a.example.com"}, ""), + hostnames: []string{"a.example.com", "c.example.com"}, + wantErr: true, + wantDenied: []string{"c.example.com"}, + }, + { + name: "hostnames matching regex are allowed", + tenant: tenantWithAllowedHostnames(nil, `.*\.clastix\.io`), + hostnames: []string{"foo.clastix.io", "bar.clastix.io"}, + wantErr: false, + }, + { + name: "mixed exact and regex hostnames are allowed", + tenant: tenantWithAllowedHostnames([]string{"a.example.com"}, `.*\.clastix\.io`), + hostnames: []string{"a.example.com", "foo.clastix.io"}, + wantErr: false, + }, + { + name: "hostname not matching regex is denied", + tenant: tenantWithAllowedHostnames(nil, `.*\.clastix\.io`), + hostnames: []string{"foo.example.com"}, + wantErr: true, + wantDenied: []string{"foo.example.com"}, + }, + { + name: "denies only the hostname that is neither in the exact list nor matches the regex", + tenant: tenantWithAllowedHostnames([]string{"allowed.example.com"}, appsRegex), + hostnames: []string{ + "allowed.example.com", // allowed via exact + "web.apps.example.com", // allowed via regex + "denied.example.com", // denied: neither exact nor regex + }, + wantErr: true, + wantDenied: []string{"denied.example.com"}, + wantAbsent: []string{"web.apps.example.com"}, + }, + { + name: "invalid regex denies hostnames outside the exact list", + tenant: tenantWithAllowedHostnames([]string{"a.example.com"}, "("), + hostnames: []string{"a.example.com", "b.example.com"}, + wantErr: true, + wantDenied: []string{"b.example.com"}, + }, + { + name: "invalid regex is ignored when every hostname is in the exact list", + tenant: tenantWithAllowedHostnames([]string{"a.example.com"}, "("), + hostnames: []string{"a.example.com"}, + wantErr: false, + }, + } + + h := &hostnames{} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := h.validateHostnames(tt.tenant, sets.New[string](tt.hostnames...)) + + if tt.wantErr && err == nil { + t.Fatalf("expected hostnames %v to be denied, got no error", tt.hostnames) + } + + if !tt.wantErr { + if err != nil { + t.Fatalf("expected hostnames %v to be allowed, got error: %v", tt.hostnames, err) + } + + return + } + + msg := err.Error() + + for _, denied := range tt.wantDenied { + if !strings.Contains(msg, denied) { + t.Errorf("expected denial message to mention %q, got: %s", denied, msg) + } + } + + for _, absent := range tt.wantAbsent { + if strings.Contains(msg, absent) { + t.Errorf("did not expect denial message to mention allowed hostname %q, got: %s", absent, msg) + } + } + }) + } +} + +// TestValidateHostnamesDeterministic guards against the historical +// non-deterministic bug: because the hostname set has a randomized iteration +// order, validation of the same input must always produce the same result. +func TestValidateHostnamesDeterministic(t *testing.T) { + t.Parallel() + + tenant := tenantWithAllowedHostnames( + []string{"allowed.example.com"}, + `^[a-z0-9-]{3,40}\.apps\.example\.com$`, + ) + + hostnameSet := sets.New[string]( + "allowed.example.com", + "web.apps.example.com", + "denied.example.com", + ) + + h := &hostnames{} + + for i := 0; i < 50; i++ { + err := h.validateHostnames(tenant, hostnameSet) + if err == nil { + t.Fatalf("iteration %d: expected denial, got no error", i) + } + + msg := err.Error() + if !strings.Contains(msg, "denied.example.com") { + t.Fatalf("iteration %d: expected message to mention denied.example.com, got: %s", i, msg) + } + + if strings.Contains(msg, "web.apps.example.com") { + t.Fatalf("iteration %d: message wrongly mentions regex-allowed hostname: %s", i, msg) + } + } +} diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index a713e8a7..e7a8c2f5 100644 --- a/pkg/api/errors/errors_test.go +++ b/pkg/api/errors/errors_test.go @@ -80,7 +80,7 @@ func TestErrorConstructors(t *testing.T) { {name: "ingress forbidden", err: apierrors.NewIngressClassForbidden("nginx", allowed), want: "Ingress Class nginx is forbidden"}, {name: "ingress collision", err: apierrors.NewIngressHostnameCollision("example.com"), want: "example.com is already used"}, {name: "empty ingress hostname", err: apierrors.NewEmptyIngressHostname(api.AllowedListSpec{Exact: []string{"example.com"}, Regex: ".*\\.example\\.com"}), want: "empty hostname is not allowed"}, - {name: "ingress hostnames invalid", err: apierrors.NewIngressHostnamesNotValid([]string{"bad_host"}, []string{"other.com"}, api.AllowedListSpec{Exact: []string{"example.com"}}), want: "Hostnames [bad_host] are not valid"}, + {name: "ingress hostnames invalid", err: apierrors.NewIngressHostnamesNotValid([]string{"bad_host"}, api.AllowedListSpec{Exact: []string{"example.com"}}), want: "Hostnames [bad_host] are not valid"}, {name: "ingress undefined", err: apierrors.NewIngressClassUndefined(allowed), want: "No Ingress Class is forbidden"}, {name: "ingress not valid", err: apierrors.NewIngressClassNotValid("nginx", allowed), want: "Ingress Class nginx is forbidden"}, {name: "namespace quota", err: apierrors.NewNamespaceQuotaExceededError(), want: "Cannot exceed Namespace quota"}, diff --git a/pkg/api/errors/ingress.go b/pkg/api/errors/ingress.go index 272cb221..7a20ae15 100644 --- a/pkg/api/errors/ingress.go +++ b/pkg/api/errors/ingress.go @@ -45,9 +45,8 @@ func (i IngressClassForbiddenError) Error() string { } type IngressHostnameNotValidError struct { - invalidHostnames []string - notMatchingHostnames []string - spec api.AllowedListSpec + invalidHostnames []string + spec api.AllowedListSpec } type IngressHostnameCollisionError struct { @@ -76,13 +75,13 @@ func (e EmptyIngressHostnameError) Error() string { return fmt.Sprintf("empty hostname is not allowed for the current Tenant%s", appendHostnameError(e.spec)) } -func NewIngressHostnamesNotValid(invalidHostnames []string, notMatchingHostnames []string, spec api.AllowedListSpec) error { - return &IngressHostnameNotValidError{invalidHostnames: invalidHostnames, notMatchingHostnames: notMatchingHostnames, spec: spec} +func NewIngressHostnamesNotValid(hostnames []string, spec api.AllowedListSpec) error { + return &IngressHostnameNotValidError{invalidHostnames: hostnames, spec: spec} } func (i IngressHostnameNotValidError) Error() string { - return fmt.Sprintf("Hostnames %s are not valid for the current Tenant. Hostnames %s not matching for the current Tenant%s", - i.invalidHostnames, i.notMatchingHostnames, appendHostnameError(i.spec)) + return fmt.Sprintf("Hostnames %v are not valid for the current Tenant%s", + i.invalidHostnames, appendHostnameError(i.spec)) } type IngressClassUndefinedError struct {