From e648deca3bc72e3f233ead213dd03d4229ff986a Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Wed, 12 Aug 2020 20:44:38 -0400 Subject: [PATCH] Fix validation of ingress rules with wildcard host Fix ingress validation so that it validates the rules of an ingress that specifies a wildcard host. Commit 60f4fbf4f25764dbd94b7f8146d927ddc684514d added an inopportune continue statement that caused this validation to be skipped. For backwards compatibility, this change restores validation for v1 of the api but still skips it on v1beta1. * pkg/apis/networking/validation/validation.go (IngressValidationOptions): Add AllowInvalidWildcardHostRule field to indicate that validation of rules should be skipped for ingresses that specify wildcard hosts. (ValidateIngressCreate): Set AllowInvalidWildcardHostRule to true if the request is using the v1beta1 API version. (ValidateIngressUpdate): Set AllowInvalidWildcardHostRule to true if the request or old ingress is using the v1beta1 API version. (validateIngressRules): Don't skip validation of the ingress rules unless the ingress has a wildcard host and AllowInvalidWildcardHostRule is true. (allowInvalidWildcardHostRule): New helper for ValidateIngressCreate and ValidateIngressUpdate. * pkg/apis/networking/validation/validation_test.go (TestValidateIngressCreate, TestValidateIngressUpdate): Add test cases to ensure that validation is performed on v1 objects and skipped on v1beta objects for backwards compatibility. (TestValidateIngressTLS): Specify PathType so that the test passes. Co-authored-by: jordan@liggitt.net --- pkg/apis/networking/validation/validation.go | 40 +++- .../networking/validation/validation_test.go | 206 +++++++++++++++++- 2 files changed, 237 insertions(+), 9 deletions(-) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index f3a5e82ad96..b053d279e64 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -201,6 +201,9 @@ var ValidateIngressName = apimachineryvalidation.NameIsDNSSubdomain type IngressValidationOptions struct { // AllowInvalidSecretName indicates whether spec.tls[*].secretName values that are not valid Secret names should be allowed AllowInvalidSecretName bool + + // AllowInvalidWildcardHostRule indicates whether invalid rule values are allowed in rules with wildcard hostnames + AllowInvalidWildcardHostRule bool } // ValidateIngress validates Ingresses on create and update. @@ -215,7 +218,8 @@ func ValidateIngressCreate(ingress *networking.Ingress, requestGV schema.GroupVe allErrs := field.ErrorList{} var opts IngressValidationOptions opts = IngressValidationOptions{ - AllowInvalidSecretName: allowInvalidSecretName(requestGV, nil), + AllowInvalidSecretName: allowInvalidSecretName(requestGV, nil), + AllowInvalidWildcardHostRule: allowInvalidWildcardHostRule(requestGV, nil), } allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...) annotationVal, annotationIsSet := ingress.Annotations[annotationIngressClass] @@ -231,7 +235,8 @@ func ValidateIngressUpdate(ingress, oldIngress *networking.Ingress, requestGV sc allErrs := apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta, field.NewPath("metadata")) var opts IngressValidationOptions opts = IngressValidationOptions{ - AllowInvalidSecretName: allowInvalidSecretName(requestGV, oldIngress), + AllowInvalidSecretName: allowInvalidSecretName(requestGV, oldIngress), + AllowInvalidWildcardHostRule: allowInvalidWildcardHostRule(requestGV, oldIngress), } allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...) @@ -313,6 +318,7 @@ func validateIngressRules(ingressRules []networking.IngressRule, fldPath *field. return append(allErrs, field.Required(fldPath, "")) } for i, ih := range ingressRules { + wildcardHost := false if len(ih.Host) > 0 { if isIP := (net.ParseIP(ih.Host) != nil); isIP { allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, "must be a DNS name, not an IP address")) @@ -323,13 +329,17 @@ func validateIngressRules(ingressRules []networking.IngressRule, fldPath *field. for _, msg := range validation.IsWildcardDNS1123Subdomain(ih.Host) { allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, msg)) } - continue - } - for _, msg := range validation.IsDNS1123Subdomain(ih.Host) { - allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, msg)) + wildcardHost = true + } else { + for _, msg := range validation.IsDNS1123Subdomain(ih.Host) { + allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, msg)) + } } } - allErrs = append(allErrs, validateIngressRuleValue(&ih.IngressRuleValue, fldPath.Index(0), opts, requestGV)...) + + if !wildcardHost || !opts.AllowInvalidWildcardHostRule { + allErrs = append(allErrs, validateIngressRuleValue(&ih.IngressRuleValue, fldPath.Index(i), opts, requestGV)...) + } } return allErrs } @@ -558,3 +568,19 @@ func validateTLSSecretName(name string) []string { } return apivalidation.ValidateSecretName(name, false) } + +func allowInvalidWildcardHostRule(gv schema.GroupVersion, oldIngress *networking.Ingress) bool { + if gv == networkingv1beta1.SchemeGroupVersion || gv == extensionsv1beta1.SchemeGroupVersion { + // backwards compatibility with released API versions that allowed invalid rules + return true + } + if oldIngress != nil { + for _, rule := range oldIngress.Spec.Rules { + if strings.Contains(rule.Host, "*") && len(validateIngressRuleValue(&rule.IngressRuleValue, nil, IngressValidationOptions{}, gv)) > 0 { + // backwards compatibility with existing invalid data + return true + } + } + } + return false +} diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index afbb0ccd349..9b4e5b5ddd4 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -1270,6 +1270,7 @@ func TestValidateIngressRuleValue(t *testing.T) { func TestValidateIngressCreate(t *testing.T) { implementationPathType := networking.PathTypeImplementationSpecific + exactPathType := networking.PathTypeExact serviceBackend := &networking.IngressServiceBackend{ Name: "defaultbackend", Port: networking.ServiceBackendPort{ @@ -1404,6 +1405,79 @@ func TestValidateIngressCreate(t *testing.T) { ingress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 1"}} }, }, + "v1: valid rules with wildcard host": { + groupVersion: &networkingv1.SchemeGroupVersion, + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + ingress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "/foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + }, + "v1: invalid rules with wildcard host": { + groupVersion: &networkingv1.SchemeGroupVersion, + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + ingress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + expectedErrs: field.ErrorList{field.Invalid(field.NewPath("spec").Child("rules").Index(0).Child("http").Child("paths").Index(0).Child("path"), "foo", `must be an absolute path`)}, + }, + "v1beta1: valid rules with wildcard host": { + groupVersion: &networkingv1beta1.SchemeGroupVersion, + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + ingress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "/foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + }, + "v1beta1: invalid rules with wildcard host": { + groupVersion: &networkingv1beta1.SchemeGroupVersion, + tweakIngress: func(ingress *networking.Ingress) { + ingress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + ingress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + }, } for name, testCase := range testCases { @@ -1430,6 +1504,7 @@ func TestValidateIngressCreate(t *testing.T) { func TestValidateIngressUpdate(t *testing.T) { implementationPathType := networking.PathTypeImplementationSpecific + exactPathType := networking.PathTypeExact serviceBackend := &networking.IngressServiceBackend{ Name: "defaultbackend", Port: networking.ServiceBackendPort{ @@ -1769,6 +1844,131 @@ func TestValidateIngressUpdate(t *testing.T) { newIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 2"}} }, }, + "v1: change valid rules with wildcard host -> invalid rules": { + gv: networkingv1.SchemeGroupVersion, + tweakIngresses: func(newIngress, oldIngress *networking.Ingress) { + oldIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + oldIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "/foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + newIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + newIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + expectedErrs: field.ErrorList{field.Invalid(field.NewPath("spec").Child("rules").Index(0).Child("http").Child("paths").Index(0).Child("path"), "foo", `must be an absolute path`)}, + }, + "v1: change invalid rules with wildcard host -> invalid rules": { + gv: networkingv1.SchemeGroupVersion, + tweakIngresses: func(newIngress, oldIngress *networking.Ingress) { + oldIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + oldIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + newIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + newIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "bar", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + }, + "v1beta1: change valid rules with wildcard host -> invalid rules": { + gv: networkingv1beta1.SchemeGroupVersion, + tweakIngresses: func(newIngress, oldIngress *networking.Ingress) { + oldIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + oldIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "/foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + newIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + newIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + }, + "v1beta1: change invalid rules with wildcard host -> invalid rules": { + gv: networkingv1beta1.SchemeGroupVersion, + tweakIngresses: func(newIngress, oldIngress *networking.Ingress) { + oldIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + oldIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "foo", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + newIngress.Spec.TLS = []networking.IngressTLS{{Hosts: []string{"*.bar.com"}}} + newIngress.Spec.Rules = []networking.IngressRule{{ + Host: "*.foo.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{{ + Path: "bar", + PathType: &exactPathType, + Backend: defaultBackend, + }}, + }, + }, + }} + }, + }, } for name, testCase := range testCases { @@ -1984,6 +2184,7 @@ func TestValidateIngressClassUpdate(t *testing.T) { } func TestValidateIngressTLS(t *testing.T) { + pathTypeImplementationSpecific := networking.PathTypeImplementationSpecific serviceBackend := &networking.IngressServiceBackend{ Name: "defaultbackend", Port: networking.ServiceBackendPort{ @@ -2008,8 +2209,9 @@ func TestValidateIngressTLS(t *testing.T) { HTTP: &networking.HTTPIngressRuleValue{ Paths: []networking.HTTPIngressPath{ { - Path: "/foo", - Backend: defaultBackend, + Path: "/foo", + PathType: &pathTypeImplementationSpecific, + Backend: defaultBackend, }, }, },