From 9b57ba2830f96cd0e293f94e38277436f5f3a560 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Mon, 27 Jun 2022 20:56:51 +0100 Subject: [PATCH] Validation for App Protect ingress annotations (#2793) * Add validation for App Protect ingress annotations Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- ...advanced-configuration-with-annotations.md | 11 +- internal/k8s/controller.go | 7 - internal/k8s/validation.go | 49 ++++ internal/k8s/validation_test.go | 259 ++++++++++++++++++ 4 files changed, 310 insertions(+), 16 deletions(-) diff --git a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md index c9d2e52bbf..9a33837269 100644 --- a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md +++ b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md @@ -81,16 +81,9 @@ Note how the events section includes a Warning event with the Rejected reason. **Note**: If you make an existing Ingress invalid, the Ingress Controller will reject it and remove the corresponding configuration from NGINX. -The following Ingress annotations currently have limited or no validation: +The following Ingress annotation currently has limited validation: -- `nginx.com/jwt-key`, -- `nginx.com/jwt-realm`, -- `nginx.com/jwt-token`, -- `nginx.com/jwt-login-url`, -- `appprotect.f5.com/app-protect-policy`, -- `appprotect.f5.com/app-protect-security-log`. - -Validation of these annotations will be addressed in the future. +- `nginx.com/jwt-token`. ## Summary of Annotations diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index df3d2793ba..0924f2b24d 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -2364,13 +2364,6 @@ func (lbc *LoadBalancerController) getAppProtectLogConfAndDst(ing *networking.In return apLogs, fmt.Errorf("Error Validating App Protect Destination and Config for Ingress %v: LogConf and LogDestination must have equal number of items", ing.Name) } - for _, logDst := range logDsts { - err := validation.ValidateAppProtectLogDestination(logDst) - if err != nil { - return apLogs, fmt.Errorf("Error Validating App Protect Destination Config for Ingress %v: %w", ing.Name, err) - } - } - for i, logConfNsN := range logConfNsNs { logConf, err := lbc.appProtectConfiguration.GetAppResource(appprotect.LogConfGVK.Kind, logConfNsN) if err != nil { diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 788dd7ff40..07ad7e766c 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/nginxinc/kubernetes-ingress/internal/configs" + ap_validation "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/validation" networking "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" @@ -54,6 +55,9 @@ const ( failTimeoutAnnotation = "nginx.org/fail-timeout" appProtectEnableAnnotation = "appprotect.f5.com/app-protect-enable" appProtectSecurityLogEnableAnnotation = "appprotect.f5.com/app-protect-security-log-enable" + appProtectPolicyAnnotation = "appprotect.f5.com/app-protect-policy" + appProtectSecurityLogAnnotation = "appprotect.f5.com/app-protect-security-log" + appProtectSecurityLogDestAnnotation = "appprotect.f5.com/app-protect-security-log-destination" appProtectDosProtectedAnnotation = "appprotectdos.f5.com/app-protect-dos-resource" internalRouteAnnotation = "nsm.nginx.com/internal-route" websocketServicesAnnotation = "nginx.org/websocket-services" @@ -257,14 +261,34 @@ var ( }, appProtectEnableAnnotation: { validateAppProtectOnlyAnnotation, + validatePlusOnlyAnnotation, validateRequiredAnnotation, validateBoolAnnotation, }, appProtectSecurityLogEnableAnnotation: { validateAppProtectOnlyAnnotation, + validatePlusOnlyAnnotation, validateRequiredAnnotation, validateBoolAnnotation, }, + appProtectPolicyAnnotation: { + validateAppProtectOnlyAnnotation, + validatePlusOnlyAnnotation, + validateRequiredAnnotation, + validateQualifiedName, + }, + appProtectSecurityLogAnnotation: { + validateAppProtectOnlyAnnotation, + validatePlusOnlyAnnotation, + validateRequiredAnnotation, + validateAppProtectSecurityLogAnnotation, + }, + appProtectSecurityLogDestAnnotation: { + validateAppProtectOnlyAnnotation, + validatePlusOnlyAnnotation, + validateRequiredAnnotation, + validateAppProtectSecurityLogDestAnnotation, + }, appProtectDosProtectedAnnotation: { validateAppProtectDosOnlyAnnotation, validatePlusOnlyAnnotation, @@ -710,6 +734,31 @@ func validateRewriteListAnnotation(context *annotationValidationContext) field.E return allErrs } +func validateAppProtectSecurityLogAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + logConf := strings.Split(context.value, ",") + for _, logConf := range logConf { + err := validation.IsQualifiedName(logConf) + if err != nil { + allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, "security log configuration resource name must be qualified name, e.g. namespace/name")) + } + } + return allErrs +} + +func validateAppProtectSecurityLogDestAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + logDsts := strings.Split(context.value, ",") + for _, logDst := range logDsts { + err := ap_validation.ValidateAppProtectLogDestination(logDst) + if err != nil { + errorMsg := fmt.Sprintf("Error Validating App Protect Log Destination Config: %v", err) + allErrs = append(allErrs, field.Invalid(context.fieldPath, context.value, errorMsg)) + } + } + return allErrs +} + func validateSnippetsAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index f9f61a590f..68d83d65ef 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -1882,6 +1882,20 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid appprotect.f5.com/app-protect-enable annotation", }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-enable": "true", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.appprotect.f5.com/app-protect-enable: Forbidden: annotation requires NGINX Plus`, + }, + msg: "invalid appprotect.f5.com/app-protect-enable annotation, requires NGINX Plus", + }, { annotations: map[string]string{ @@ -1923,6 +1937,250 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid appprotect.f5.com/app-protect-security-log-enable annotation", }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-enable": "true", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.appprotect.f5.com/app-protect-security-log-enable: Forbidden: annotation requires NGINX Plus`, + }, + msg: "invalid appprotect.f5.com/app-protect-security-log-enable annotation, requires NGINX Plus", + }, + + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-policy": "default/dataguard-alarm", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-policy annotation", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-policy": `default/dataguard\alarm`, + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-policy: Invalid value: \"default/dataguard\\\\alarm\": must be a qualified name", + }, msg: "invalid appprotect.f5.com/app-protect-policy annotation, not a qualified name", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-policy": "true", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-policy: Forbidden: annotation requires AppProtect", + }, + msg: "invalid appprotect.f5.com/app-protect-policy annotation, requires AppProtect", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-policy": "true", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-policy: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid appprotect.f5.com/app-protect-policy annotation, requires NGINX Plus", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-policy": "", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-policy: Required value", + }, + msg: "invalid appprotect.f5.com/app-protect-policy annotation, requires value", + }, + + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log": "default/logconf", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-security-log annotation", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log": `default/logconf,default/logconf2`, + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-security-log annotation, multiple values", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log": `default/logconf\`, + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log: Invalid value: \"default/logconf\\\\\": security log configuration resource name must be qualified name, e.g. namespace/name", + }, msg: "invalid appprotect.f5.com/app-protect-security-log annotation, not a qualified name", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log": "true", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log: Forbidden: annotation requires AppProtect", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log annotation, requires AppProtect", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log": "true", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log annotation, requires NGINX Plus", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log": "", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log: Required value", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log annotation, requires value", + }, + + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-destination": "syslog:server=localhost:514", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-security-log-destination annotation", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-destination": `syslog:server=localhost:514,syslog:server=syslog-svc.default:514`, + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-security-log-destination annotation, multiple values", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-destination": `syslog:server=localhost\:514`, + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log-destination: Invalid value: \"syslog:server=localhost\\\\:514\": Error Validating App Protect Log Destination Config: error parsing App Protect Log config: Destination must follow format: syslog:server=: or fqdn or stderr or absolute path to file Log Destination did not follow format", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log-destination, invalid value", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-destination": "true", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log-destination: Forbidden: annotation requires AppProtect", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log-destination annotation, requires AppProtect", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-destination": "true", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log-destination: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log-destination annotation, requires NGINX Plus", + }, + { + annotations: map[string]string{ + "appprotect.f5.com/app-protect-security-log-destination": "", + }, + specServices: map[string]bool{}, + isPlus: true, + appProtectEnabled: true, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.appprotect.f5.com/app-protect-security-log-destination: Required value", + }, + msg: "invalid appprotect.f5.com/app-protect-security-log-destination, requires value", + }, + { annotations: map[string]string{ "appprotectdos.f5.com/app-protect-dos-resource": "dos-resource-name", @@ -1989,6 +2247,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid appprotectdos.f5.com/app-protect-dos-enable annotation with incorrectly qualified identifier", }, + { annotations: map[string]string{ "nsm.nginx.com/internal-route": "true",