Implement annotation validation (#9673)

* Add validation to all annotations

* Add annotation validation for fcgi

* Fix reviews and fcgi e2e

* Add flag to disable cross namespace validation

* Add risk, flag for validation, tests

* Add missing formating

* Enable validation by default on tests

* Test validation flag

* remove ajp from list

* Finalize validation changes

* Add validations to CI

* Update helm docs

* Fix code review

* Use a better name for annotation risk

internal/ingress/controller/config/config.go

@@ -97,6 +97,17 @@ type Configuration struct {
 	// If disabled, only snippets added via ConfigMap are added to ingress.
 	AllowSnippetAnnotations bool `json:"allow-snippet-annotations"`
 
+	// AllowCrossNamespaceResources enables users to consume cross namespace resource on annotations
+	// Case disabled, attempts to use secrets or configmaps from a namespace different from Ingress will
+	// be denied
+	// This value will default to `false` on future releases
+	AllowCrossNamespaceResources bool `json:"allow-cross-namespace-resources"`
+
+	// AnnotationsRiskLevel represents the risk accepted on an annotation. If the risk is, for instance `Medium`, annotations
+	// with risk High and Critical will not be accepted.
+	// Default Risk is Critical by default, but this may be changed in future releases
+	AnnotationsRiskLevel string `json:"annotations-risk-level"`
+
 	// AnnotationValueWordBlocklist defines words that should not be part of an user annotation value
 	// (can be used to run arbitrary code or configs, for example) and that should be dropped.
 	// This list should be separated by "," character
@@ -708,7 +719,7 @@ type Configuration struct {
 
 	// DatadogSampleRate specifies sample rate for any traces created.
 	// Default: use a dynamic rate instead
-	DatadogSampleRate *float32 `json:"datadog-sample-rate",omitempty`
+	DatadogSampleRate *float32 `json:"datadog-sample-rate,omitempty"`
 
 	// MainSnippet adds custom configuration to the main section of the nginx configuration
 	MainSnippet string `json:"main-snippet"`
@@ -853,8 +864,10 @@ func NewDefault() Configuration {
 
 	cfg := Configuration{
 		AllowSnippetAnnotations:          true,
+		AllowCrossNamespaceResources:     true,
 		AllowBackendServerHeader:         false,
 		AnnotationValueWordBlocklist:     "",
+		AnnotationsRiskLevel:             "Critical",
 		AccessLogPath:                    "/var/log/nginx/access.log",
 		AccessLogParams:                  "",
 		EnableAccessLogForDefaultBackend: false,

internal/ingress/controller/controller.go

@@ -389,14 +389,19 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 			toCheck.ObjectMeta.Name == ing.ObjectMeta.Name
 	}
 	ings := store.FilterIngresses(allIngresses, filter)
+	parsed, err := annotations.NewAnnotationExtractor(n.store).Extract(ing)
+	if err != nil {
+		n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
+		return err
+	}
 	ings = append(ings, &ingress.Ingress{
 		Ingress:           *ing,
-		ParsedAnnotations: annotations.NewAnnotationExtractor(n.store).Extract(ing),
+		ParsedAnnotations: parsed,
 	})
 	startTest := time.Now().UnixNano() / 1000000
 	_, servers, pcfg := n.getConfiguration(ings)
 
-	err := checkOverlap(ing, allIngresses, servers)
+	err = checkOverlap(ing, allIngresses, servers)
 	if err != nil {
 		n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
 		return err
@@ -1509,7 +1514,7 @@ func locationApplyAnnotations(loc *ingress.Location, anns *annotations.Ingress)
 	loc.Rewrite = anns.Rewrite
 	loc.UpstreamVhost = anns.UpstreamVhost
 	loc.Denylist = anns.Denylist
-	loc.Whitelist = anns.Whitelist
+	loc.Allowlist = anns.Allowlist
 	loc.Denied = anns.Denied
 	loc.XForwardedPrefix = anns.XForwardedPrefix
 	loc.UsePortInRedirects = anns.UsePortInRedirects
@@ -1808,9 +1813,9 @@ func checkOverlap(ing *networking.Ingress, ingresses []*ingress.Ingress, servers
 			}
 
 			// path overlap. Check if one of the ingresses has a canary annotation
-			isCanaryEnabled, annotationErr := parser.GetBoolAnnotation("canary", ing)
+			isCanaryEnabled, annotationErr := parser.GetBoolAnnotation("canary", ing, canary.CanaryAnnotations.Annotations)
 			for _, existing := range existingIngresses {
-				isExistingCanaryEnabled, existingAnnotationErr := parser.GetBoolAnnotation("canary", existing)
+				isExistingCanaryEnabled, existingAnnotationErr := parser.GetBoolAnnotation("canary", existing, canary.CanaryAnnotations.Annotations)
 
 				if isCanaryEnabled && isExistingCanaryEnabled {
 					return fmt.Errorf(`host "%s" and path "%s" is already defined in ingress %s/%s`, rule.Host, path.Path, existing.Namespace, existing.Name)

internal/ingress/controller/controller_test.go

@@ -44,7 +44,7 @@ import (
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/canary"
-	"k8s.io/ingress-nginx/internal/ingress/annotations/ipwhitelist"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/ipallowlist"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/proxyssl"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/sessionaffinity"
@@ -73,6 +73,13 @@ func (fis fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration {
 	return fis.configuration
 }
 
+func (fis fakeIngressStore) GetSecurityConfiguration() defaults.SecurityConfiguration {
+	return defaults.SecurityConfiguration{
+		AnnotationsRiskLevel:         fis.configuration.AnnotationsRiskLevel,
+		AllowCrossNamespaceResources: fis.configuration.AllowCrossNamespaceResources,
+	}
+}
+
 func (fakeIngressStore) GetConfigMap(key string) (*corev1.ConfigMap, error) {
 	return nil, fmt.Errorf("test error")
 }
@@ -2418,7 +2425,7 @@ func TestGetBackendServers(t *testing.T) {
 						},
 					},
 					ParsedAnnotations: &annotations.Ingress{
-						Whitelist:            ipwhitelist.SourceRange{CIDR: []string{"10.0.0.0/24"}},
+						Allowlist:            ipallowlist.SourceRange{CIDR: []string{"10.0.0.0/24"}},
 						ServerSnippet:        "bla",
 						ConfigurationSnippet: "blo",
 					},
@@ -2439,7 +2446,7 @@ func TestGetBackendServers(t *testing.T) {
 					t.Errorf("config snippet should be empty, got '%s'", s.Locations[0].ConfigurationSnippet)
 				}
 
-				if len(s.Locations[0].Whitelist.CIDR) != 1 || s.Locations[0].Whitelist.CIDR[0] != "10.0.0.0/24" {
+				if len(s.Locations[0].Allowlist.CIDR) != 1 || s.Locations[0].Allowlist.CIDR[0] != "10.0.0.0/24" {
 					t.Errorf("allow list was incorrectly dropped, len should be 1 and contain 10.0.0.0/24")
 				}
 

internal/ingress/controller/store/store.go

@@ -69,6 +69,9 @@ type Storer interface {
 	// GetBackendConfiguration returns the nginx configuration stored in a configmap
 	GetBackendConfiguration() ngx_config.Configuration
 
+	// GetSecurityConfiguration returns the configuration options from Ingress
+	GetSecurityConfiguration() defaults.SecurityConfiguration
+
 	// GetConfigMap returns the ConfigMap matching key.
 	GetConfigMap(key string) (*corev1.ConfigMap, error)
 
@@ -882,9 +885,14 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
 
 	k8s.SetDefaultNGINXPathType(copyIng)
 
-	err := s.listers.IngressWithAnnotation.Update(&ingress.Ingress{
+	parsed, err := s.annotations.Extract(ing)
+	if err != nil {
+		klog.Error(err)
+		return
+	}
+	err = s.listers.IngressWithAnnotation.Update(&ingress.Ingress{
 		Ingress:           *copyIng,
-		ParsedAnnotations: s.annotations.Extract(ing),
+		ParsedAnnotations: parsed,
 	})
 	if err != nil {
 		klog.Error(err)
@@ -920,8 +928,10 @@ func (s *k8sStore) updateSecretIngressMap(ing *networkingv1.Ingress) {
 		"proxy-ssl-secret",
 		"secure-verify-ca-secret",
 	}
+
+	secConfig := s.GetSecurityConfiguration().AllowCrossNamespaceResources
 	for _, ann := range secretAnnotations {
-		secrKey, err := objectRefAnnotationNsKey(ann, ing)
+		secrKey, err := objectRefAnnotationNsKey(ann, ing, secConfig)
 		if err != nil && !errors.IsMissingAnnotations(err) {
 			klog.Errorf("error reading secret reference in annotation %q: %s", ann, err)
 			continue
@@ -937,8 +947,9 @@ func (s *k8sStore) updateSecretIngressMap(ing *networkingv1.Ingress) {
 
 // objectRefAnnotationNsKey returns an object reference formatted as a
 // 'namespace/name' key from the given annotation name.
-func objectRefAnnotationNsKey(ann string, ing *networkingv1.Ingress) (string, error) {
-	annValue, err := parser.GetStringAnnotation(ann, ing)
+func objectRefAnnotationNsKey(ann string, ing *networkingv1.Ingress, allowCrossNamespace bool) (string, error) {
+	// We pass nil fields, as this is an internal process and we don't need to validate it.
+	annValue, err := parser.GetStringAnnotation(ann, ing, nil)
 	if err != nil {
 		return "", err
 	}
@@ -951,6 +962,9 @@ func objectRefAnnotationNsKey(ann string, ing *networkingv1.Ingress) (string, er
 	if secrNs == "" {
 		return fmt.Sprintf("%v/%v", ing.Namespace, secrName), nil
 	}
+	if !allowCrossNamespace && secrNs != ing.Namespace {
+		return "", fmt.Errorf("cross namespace secret is not supported")
+	}
 	return annValue, nil
 }
 
@@ -1125,6 +1139,17 @@ func (s *k8sStore) GetBackendConfiguration() ngx_config.Configuration {
 	return s.backendConfig
 }
 
+func (s *k8sStore) GetSecurityConfiguration() defaults.SecurityConfiguration {
+	s.backendConfigMu.RLock()
+	defer s.backendConfigMu.RUnlock()
+
+	secConfig := defaults.SecurityConfiguration{
+		AllowCrossNamespaceResources: s.backendConfig.AllowCrossNamespaceResources,
+		AnnotationsRiskLevel:         s.backendConfig.AnnotationsRiskLevel,
+	}
+	return secConfig
+}
+
 func (s *k8sStore) setConfig(cmap *corev1.ConfigMap) {
 	s.backendConfigMu.Lock()
 	defer s.backendConfigMu.Unlock()

internal/ingress/controller/store/store_test.go

@@ -1414,18 +1414,31 @@ func TestUpdateSecretIngressMap(t *testing.T) {
 	t.Run("with annotation in namespace/name format", func(t *testing.T) {
 		ing := ingTpl.DeepCopy()
 		ing.ObjectMeta.SetAnnotations(map[string]string{
-			parser.GetAnnotationWithPrefix("auth-secret"): "otherns/auth",
+			parser.GetAnnotationWithPrefix("auth-secret"): "testns/auth",
 		})
 		if err := s.listers.Ingress.Update(ing); err != nil {
 			t.Errorf("error updating the Ingress: %v", err)
 		}
 		s.updateSecretIngressMap(ing)
 
-		if l := s.secretIngressMap.Len(); !(l == 1 && s.secretIngressMap.Has("otherns/auth")) {
+		if l := s.secretIngressMap.Len(); !(l == 1 && s.secretIngressMap.Has("testns/auth")) {
 			t.Errorf("Expected \"otherns/auth\" to be the only referenced Secret (got %d)", l)
 		}
 	})
 
+	t.Run("with annotation in namespace/name format should not be supported", func(t *testing.T) {
+		ing := ingTpl.DeepCopy()
+		ing.ObjectMeta.SetAnnotations(map[string]string{
+			parser.GetAnnotationWithPrefix("auth-secret"): "anotherns/auth",
+		})
+		s.listers.Ingress.Update(ing)
+		s.updateSecretIngressMap(ing)
+
+		if l := s.secretIngressMap.Len(); l != 0 {
+			t.Errorf("Expected \"otherns/auth\" to be denied as it contains a different namespace (got %d)", l)
+		}
+	})
+
 	t.Run("with annotation in invalid format", func(t *testing.T) {
 		ing := ingTpl.DeepCopy()
 		ing.ObjectMeta.SetAnnotations(map[string]string{