Skip to content

Commit

Permalink
Validation for App Protect ingress annotations (#2793)
Browse files Browse the repository at this point in the history 
* Add validation for App Protect ingress annotations

Co-authored-by: Ciara Stacke <[email protected]>
  • Loading branch information
@haywoodsh @ciarams87
haywoodsh and ciarams87 authored Jun 27, 2022
1 parent 57d4b81 commit 9b57ba2
Show file tree
Hide file tree
Showing 4 changed files with 310 additions and 16 deletions.
11 changes: 2 additions & 9 deletions ...tent/configuration/ingress-resources/advanced-configuration-with-annotations.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 0 additions & 7 deletions internal/k8s/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
49 changes: 49 additions & 0 deletions internal/k8s/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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{}

Expand Down
259 changes: 259 additions & 0 deletions internal/k8s/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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=<ip-address | localhost>:<port> 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",
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 9b57ba2

Please sign in to comment.