From 1587d189cbf27b3c2470cf1fe56e50afbde412b6 Mon Sep 17 00:00:00 2001 From: Mehdy Bohlool Date: Tue, 30 Oct 2018 11:57:29 -0700 Subject: [PATCH] Refactor webhookclientConfig validation of admission and audit registration --- .../validation/validation.go | 96 ++--------------- .../validation/validation_test.go | 2 +- .../validation/validation.go | 99 ++--------------- .../validation/validation_test.go | 2 +- .../apiserver/pkg/util/webhook/validation.go | 101 ++++++++++++++++++ 5 files changed, 123 insertions(+), 177 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/util/webhook/validation.go diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 9174bb0835a..4f0fe60b2f0 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -18,7 +18,6 @@ package validation import ( "fmt" - "net/url" "strings" genericvalidation "k8s.io/apimachinery/pkg/api/validation" @@ -26,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/util/webhook" "k8s.io/kubernetes/pkg/apis/admissionregistration" ) @@ -200,93 +200,15 @@ func validateWebhook(hook *admissionregistration.Webhook, fldPath *field.Path) f allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...) } - allErrors = append(allErrors, validateWebhookClientConfig(fldPath.Child("clientConfig"), &hook.ClientConfig)...) - - return allErrors -} - -func validateWebhookClientConfig(fldPath *field.Path, cc *admissionregistration.WebhookClientConfig) field.ErrorList { - var allErrors field.ErrorList - if (cc.URL == nil) == (cc.Service == nil) { - allErrors = append(allErrors, field.Required(fldPath.Child("url"), "exactly one of url or service is required")) + cc := hook.ClientConfig + switch { + case (cc.URL == nil) == (cc.Service == nil): + allErrors = append(allErrors, field.Required(fldPath.Child("clientConfig"), "exactly one of url or service is required")) + case cc.URL != nil: + allErrors = append(allErrors, webhook.ValidateWebhookURL(fldPath.Child("clientConfig").Child("url"), *cc.URL, true)...) + case cc.Service != nil: + allErrors = append(allErrors, webhook.ValidateWebhookService(fldPath.Child("clientConfig").Child("service"), cc.Service.Name, cc.Service.Namespace, cc.Service.Path)...) } - - if cc.URL != nil { - const form = "; desired format: https://host[/path]" - if u, err := url.Parse(*cc.URL); err != nil { - allErrors = append(allErrors, field.Required(fldPath.Child("url"), "url must be a valid URL: "+err.Error()+form)) - } else { - if u.Scheme != "https" { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Scheme, "'https' is the only allowed URL scheme"+form)) - } - if len(u.Host) == 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Host, "host must be provided"+form)) - } - if u.User != nil { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.User.String(), "user information is not permitted in the URL")) - } - if len(u.Fragment) != 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Fragment, "fragments are not permitted in the URL")) - } - if len(u.RawQuery) != 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.RawQuery, "query parameters are not permitted in the URL")) - } - } - } - - if cc.Service != nil { - allErrors = append(allErrors, validateWebhookService(fldPath.Child("service"), cc.Service)...) - } - return allErrors -} - -// note: this has copy/paste inheritance in auditregistration -func validateWebhookService(fldPath *field.Path, svc *admissionregistration.ServiceReference) field.ErrorList { - var allErrors field.ErrorList - - if len(svc.Name) == 0 { - allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required")) - } - - if len(svc.Namespace) == 0 { - allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required")) - } - - if svc.Path == nil { - return allErrors - } - - // TODO: replace below with url.Parse + verifying that host is empty? - - urlPath := *svc.Path - if urlPath == "/" || len(urlPath) == 0 { - return allErrors - } - if urlPath == "//" { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty")) - return allErrors - } - - if !strings.HasPrefix(urlPath, "/") { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'")) - } - - urlPathToCheck := urlPath[1:] - if strings.HasSuffix(urlPathToCheck, "/") { - urlPathToCheck = urlPathToCheck[:len(urlPathToCheck)-1] - } - steps := strings.Split(urlPathToCheck, "/") - for i, step := range steps { - if len(step) == 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i))) - continue - } - failures := validation.IsDNS1123Subdomain(step) - for _, failure := range failures { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure))) - } - } - return allErrors } diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index 2cf41fa5979..6eaa1804a66 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/pkg/apis/admissionregistration/validation/validation_test.go @@ -540,7 +540,7 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { }, }, }), - expectedError: `[0].clientConfig.url: Required value: exactly one of url or service is required`, + expectedError: `[0].clientConfig: Required value: exactly one of url or service is required`, }, { name: "blank URL", diff --git a/pkg/apis/auditregistration/validation/validation.go b/pkg/apis/auditregistration/validation/validation.go index 693a6574181..101b1fd6aa9 100644 --- a/pkg/apis/auditregistration/validation/validation.go +++ b/pkg/apis/auditregistration/validation/validation.go @@ -17,14 +17,12 @@ limitations under the License. package validation import ( - "fmt" - "net/url" "strings" genericvalidation "k8s.io/apimachinery/pkg/api/validation" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/util/webhook" "k8s.io/kubernetes/pkg/apis/auditregistration" ) @@ -49,7 +47,16 @@ func ValidateWebhook(w auditregistration.Webhook, fldPath *field.Path) field.Err if w.Throttle != nil { allErrs = append(allErrs, ValidateWebhookThrottleConfig(w.Throttle, fldPath.Child("throttle"))...) } - allErrs = append(allErrs, ValidateWebhookClientConfig(&w.ClientConfig, fldPath.Child("clientConfig"))...) + + cc := w.ClientConfig + switch { + case (cc.URL == nil) == (cc.Service == nil): + allErrs = append(allErrs, field.Required(fldPath.Child("clientConfig"), "exactly one of url or service is required")) + case cc.URL != nil: + allErrs = append(allErrs, webhook.ValidateWebhookURL(fldPath.Child("clientConfig").Child("url"), *cc.URL, false)...) + case cc.Service != nil: + allErrs = append(allErrs, webhook.ValidateWebhookService(fldPath.Child("clientConfig").Child("service"), cc.Service.Name, cc.Service.Namespace, cc.Service.Path)...) + } return allErrs } @@ -65,90 +72,6 @@ func ValidateWebhookThrottleConfig(c *auditregistration.WebhookThrottleConfig, f return allErrs } -// ValidateWebhookClientConfig validates the WebhookClientConfig -// note: this is largely copy/paste inheritance from admissionregistration with subtle changes -func ValidateWebhookClientConfig(cc *auditregistration.WebhookClientConfig, fldPath *field.Path) field.ErrorList { - var allErrors field.ErrorList - if (cc.URL == nil) == (cc.Service == nil) { - allErrors = append(allErrors, field.Required(fldPath.Child("url"), "exactly one of url or service is required")) - } - - if cc.URL != nil { - const form = "; desired format: https://host[/path]" - if u, err := url.Parse(*cc.URL); err != nil { - allErrors = append(allErrors, field.Required(fldPath.Child("url"), "url must be a valid URL: "+err.Error()+form)) - } else { - if len(u.Host) == 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Host, "host must be provided"+form)) - } - if u.User != nil { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.User.String(), "user information is not permitted in the URL")) - } - if len(u.Fragment) != 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.Fragment, "fragments are not permitted in the URL")) - } - if len(u.RawQuery) != 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("url"), u.RawQuery, "query parameters are not permitted in the URL")) - } - } - } - - if cc.Service != nil { - allErrors = append(allErrors, validateWebhookService(cc.Service, fldPath.Child("service"))...) - } - return allErrors -} - -// note: this is copy/paste inheritance from admissionregistration -func validateWebhookService(svc *auditregistration.ServiceReference, fldPath *field.Path) field.ErrorList { - var allErrors field.ErrorList - - if len(svc.Name) == 0 { - allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required")) - } - - if len(svc.Namespace) == 0 { - allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required")) - } - - if svc.Path == nil { - return allErrors - } - - // TODO: replace below with url.Parse + verifying that host is empty? - - urlPath := *svc.Path - if urlPath == "/" || len(urlPath) == 0 { - return allErrors - } - if urlPath == "//" { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty")) - return allErrors - } - - if !strings.HasPrefix(urlPath, "/") { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'")) - } - - urlPathToCheck := urlPath[1:] - if strings.HasSuffix(urlPathToCheck, "/") { - urlPathToCheck = urlPathToCheck[:len(urlPathToCheck)-1] - } - steps := strings.Split(urlPathToCheck, "/") - for i, step := range steps { - if len(step) == 0 { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i))) - continue - } - failures := validation.IsDNS1123Subdomain(step) - for _, failure := range failures { - allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure))) - } - } - - return allErrors -} - // ValidatePolicy validates the audit policy func ValidatePolicy(policy auditregistration.Policy, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList diff --git a/pkg/apis/auditregistration/validation/validation_test.go b/pkg/apis/auditregistration/validation/validation_test.go index 522fb246ce8..96c6a882c5d 100644 --- a/pkg/apis/auditregistration/validation/validation_test.go +++ b/pkg/apis/auditregistration/validation/validation_test.go @@ -159,7 +159,7 @@ func TestValidateWebhookConfiguration(t *testing.T) { URL: strPtr("example.com/k8s/webhook"), }, }, - expectedError: `webhook.clientConfig.url: Required value: exactly one of url or service is required`, + expectedError: `webhook.clientConfig: Required value: exactly one of url or service is required`, }, { name: "blank URL", diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/validation.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/validation.go new file mode 100644 index 00000000000..2ddb2c09ab7 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/util/webhook/validation.go @@ -0,0 +1,101 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhook + +import ( + "fmt" + "net/url" + "strings" + + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// ValidateWebhookURL validates webhook's URL. +func ValidateWebhookURL(fldPath *field.Path, URL string, forceHttps bool) field.ErrorList { + var allErrors field.ErrorList + const form = "; desired format: https://host[/path]" + if u, err := url.Parse(URL); err != nil { + allErrors = append(allErrors, field.Required(fldPath, "url must be a valid URL: "+err.Error()+form)) + } else { + if forceHttps && u.Scheme != "https" { + allErrors = append(allErrors, field.Invalid(fldPath, u.Scheme, "'https' is the only allowed URL scheme"+form)) + } + if len(u.Host) == 0 { + allErrors = append(allErrors, field.Invalid(fldPath, u.Host, "host must be provided"+form)) + } + if u.User != nil { + allErrors = append(allErrors, field.Invalid(fldPath, u.User.String(), "user information is not permitted in the URL")) + } + if len(u.Fragment) != 0 { + allErrors = append(allErrors, field.Invalid(fldPath, u.Fragment, "fragments are not permitted in the URL")) + } + if len(u.RawQuery) != 0 { + allErrors = append(allErrors, field.Invalid(fldPath, u.RawQuery, "query parameters are not permitted in the URL")) + } + } + return allErrors +} + +func ValidateWebhookService(fldPath *field.Path, namespace, name string, path *string) field.ErrorList { + var allErrors field.ErrorList + + if len(name) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("name"), "service name is required")) + } + + if len(namespace) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("namespace"), "service namespace is required")) + } + + if path == nil { + return allErrors + } + + // TODO: replace below with url.Parse + verifying that host is empty? + + urlPath := *path + if urlPath == "/" || len(urlPath) == 0 { + return allErrors + } + if urlPath == "//" { + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "segment[0] may not be empty")) + return allErrors + } + + if !strings.HasPrefix(urlPath, "/") { + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, "must start with a '/'")) + } + + urlPathToCheck := urlPath[1:] + if strings.HasSuffix(urlPathToCheck, "/") { + urlPathToCheck = urlPathToCheck[:len(urlPathToCheck)-1] + } + steps := strings.Split(urlPathToCheck, "/") + for i, step := range steps { + if len(step) == 0 { + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d] may not be empty", i))) + continue + } + failures := validation.IsDNS1123Subdomain(step) + for _, failure := range failures { + allErrors = append(allErrors, field.Invalid(fldPath.Child("path"), urlPath, fmt.Sprintf("segment[%d]: %v", i, failure))) + } + } + + return allErrors +}