CHERRY-PICK of #7665 - Remove snippet (#7666)

* Add option to force enabling snippet directives (#7665)

Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>

* Add missing key when cherry-picking

charts/ingress-nginx/ci/daemonset-customconfig-values.yaml

@@ -1,5 +1,6 @@
 controller:
   kind: DaemonSet
+  enableSnippetDirectives: false
   admissionWebhooks:
     enabled: false
   service:

charts/ingress-nginx/ci/deployment-customconfig-values.yaml

@@ -1,6 +1,7 @@
 controller:
   config:
     use-proxy-protocol: "true"
+  enableSnippetDirectives: false
   admissionWebhooks:
     enabled: false
   service:

charts/ingress-nginx/templates/controller-configmap.yaml

@@ -10,6 +10,7 @@ metadata:
   name: {{ include "ingress-nginx.controller.fullname" . }}
   namespace: {{ .Release.Namespace }}
 data:
+  enable-snippet-directives: "{{ .Values.controller.enableSnippetDirectives }}"
 {{- if .Values.controller.addHeaders }}
   add-headers: {{ .Release.Namespace }}/{{ include "ingress-nginx.fullname" . }}-custom-add-headers
 {{- end }}

charts/ingress-nginx/values.yaml

@@ -61,6 +61,12 @@ controller:
   # Ingress status was blank because there is no Service exposing the NGINX Ingress controller in a configuration using the host network, the default --publish-service flag used in standard cloud setups does not apply
   reportNodeInternalIp: false
 
+  # This configuration defines if Ingress Controller should allow users to set
+  # their own *-snippet directives/annotations, otherwise this is forbidden / dropped
+  # when users add those annotations.
+  # Global snippets in ConfigMap are still respected
+  enableSnippetDirectives: true
+
   # Required for use with CNI based kubernetes installations (such as ones set up by kubeadm),
   # since CNI and hostport don't mix yet. Can be deprecated once https://github.com/kubernetes/kubernetes/issues/23920
   # is merged

docs/user-guide/nginx-configuration/configmap.md

@@ -46,6 +46,7 @@ The following table shows a configuration option's name, type, and the default v
 |[disable-access-log](#disable-access-log)|bool|false|
 |[disable-ipv6](#disable-ipv6)|bool|false|
 |[disable-ipv6-dns](#disable-ipv6-dns)|bool|false|
+|[enable-snippet-directives](#enable-snippet-directives)|bool|true|
 |[enable-underscores-in-headers](#enable-underscores-in-headers)|bool|false|
 |[enable-ocsp](#enable-ocsp)|bool|false|
 |[ignore-invalid-headers](#ignore-invalid-headers)|bool|true|
@@ -312,6 +313,12 @@ Disable listening on IPV6. _**default:**_ `false`; IPv6 listening is enabled
 
 Disable IPV6 for nginx DNS resolver. _**default:**_ `false`; IPv6 resolving enabled.
 
+## enable-snippet-directives
+
+Enables Ingress to parse and add *-snippet annotations/directives created by the user. _**default:**_ `true`;
+Obs.: We recommend enabling this option only if you TRUST users with permission to create Ingress objects, as this 
+may allow a user to add restricted configurations to the final nginx.conf file
+
 ## enable-underscores-in-headers
 
 Enables underscores in header names. _**default:**_ is disabled

internal/ingress/controller/config/config.go

@@ -93,6 +93,10 @@ const (
 type Configuration struct {
 	defaults.Backend `json:",squash"`
 
+	// EnableSnippetDirectives enable users to add their own snippets via ingress annotation.
+	// If disabled, only snippets added via ConfigMap are added to ingress.
+	EnableSnippetDirectives bool `json:"enable-snippet-directives"`
+
 	// Sets the name of the configmap that contains the headers to pass to the client
 	AddHeaders string `json:"add-headers,omitempty"`
 
@@ -757,6 +761,7 @@ func NewDefault() Configuration {
 	defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", "", append(defResponseHeaders, ""), "", "", "", []string{}, map[string]string{}}
 
 	cfg := Configuration{
+		EnableSnippetDirectives:          true,
 		AllowBackendServerHeader:         false,
 		AccessLogPath:                    "/var/log/nginx/access.log",
 		AccessLogParams:                  "",

internal/ingress/controller/controller.go

@@ -235,27 +235,28 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 		return fmt.Errorf("This deployment is trying to create a catch-all ingress while DisableCatchAll flag is set to true. Remove '.spec.backend' or set DisableCatchAll flag to false.")
 	}
 
-	if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
-		for key := range ing.ObjectMeta.GetAnnotations() {
+	cfg := n.store.GetBackendConfiguration()
+	cfg.Resolver = n.resolver
+
+	for key := range ing.ObjectMeta.GetAnnotations() {
+		if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
 			if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) {
 				return fmt.Errorf("This deployment has a custom annotation prefix defined. Use '%s' instead of '%s'", parser.AnnotationsPrefix, parser.DefaultAnnotationsPrefix)
 			}
 		}
-	}
 
-	k8s.SetDefaultNGINXPathType(ing)
-
-	cfg := n.store.GetBackendConfiguration()
-	cfg.Resolver = n.resolver
+		if !cfg.EnableSnippetDirectives && strings.HasSuffix(key, "-snippet") {
+			return fmt.Errorf("%s annotation cannot be used. Snippet directives are disabled by the Ingress administrator", key)
+		}
 
-	if len(cfg.GlobalRateLimitMemcachedHost) == 0 {
-		for key := range ing.ObjectMeta.GetAnnotations() {
-			if strings.HasPrefix(key, fmt.Sprintf("%s/%s", parser.AnnotationsPrefix, "global-rate-limit")) {
-				return fmt.Errorf("'global-rate-limit*' annotations require 'global-rate-limit-memcached-host' settings configured in the global configmap")
-			}
+		if len(cfg.GlobalRateLimitMemcachedHost) == 0 && strings.HasPrefix(key, fmt.Sprintf("%s/%s", parser.AnnotationsPrefix, "global-rate-limit")) {
+			return fmt.Errorf("'global-rate-limit*' annotations require 'global-rate-limit-memcached-host' settings configured in the global configmap")
 		}
+
 	}
 
+	k8s.SetDefaultNGINXPathType(ing)
+
 	allIngresses := n.store.ListIngresses()
 
 	filter := func(toCheck *ingress.Ingress) bool {
@@ -508,6 +509,30 @@ func (n *NGINXController) getConfiguration(ingresses []*ingress.Ingress) (sets.S
 	}
 }
 
+func dropSnippetDirectives(anns *annotations.Ingress, ingKey string) {
+	if anns != nil {
+		if anns.ConfigurationSnippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use configuration-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ConfigurationSnippet = ""
+		}
+		if anns.ServerSnippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use server-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ServerSnippet = ""
+		}
+
+		if anns.ModSecurity.Snippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use modsecurity-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ModSecurity.Snippet = ""
+		}
+
+		if anns.ExternalAuth.AuthSnippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use auth-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ExternalAuth.AuthSnippet = ""
+		}
+
+	}
+}
+
 // getBackendServers returns a list of Upstream and Server to be used by the
 // backend.  An upstream can be used in multiple servers if the namespace,
 // service name and port are the same.
@@ -522,6 +547,10 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
+
 		for _, rule := range ing.Spec.Rules {
 			host := rule.Host
 			if host == "" {
@@ -789,6 +818,11 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
 
 	for _, ing := range data {
 		anns := ing.ParsedAnnotations
+		ingKey := k8s.MetaNamespaceKey(ing)
+
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
 
 		var defBackend string
 		if ing.Spec.Backend != nil {
@@ -1069,6 +1103,10 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
+
 		// default upstream name
 		un := du.Name
 
@@ -1145,6 +1183,10 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
+
 		if anns.Canary.Enabled {
 			klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
 			continue

internal/ingress/controller/controller_test.go

@@ -42,6 +42,7 @@ import (
 	"k8s.io/ingress-nginx/internal/ingress"
 	"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/parser"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/proxyssl"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/sessionaffinity"
@@ -56,11 +57,12 @@ import (
 )
 
 type fakeIngressStore struct {
-	ingresses []*ingress.Ingress
+	ingresses     []*ingress.Ingress
+	configuration ngx_config.Configuration
 }
 
-func (fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration {
-	return ngx_config.Configuration{}
+func (fis fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration {
+	return fis.configuration
 }
 
 func (fakeIngressStore) GetConfigMap(key string) (*corev1.ConfigMap, error) {
@@ -246,6 +248,9 @@ func TestCheckIngress(t *testing.T) {
 		})
 
 		t.Run("When the default annotation prefix is used despite an override", func(t *testing.T) {
+			defer func() {
+				parser.AnnotationsPrefix = "nginx.ingress.kubernetes.io"
+			}()
 			parser.AnnotationsPrefix = "ingress.kubernetes.io"
 			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/backend-protocol"] = "GRPC"
 			nginx.command = testNginxTestCommand{
@@ -257,6 +262,23 @@ func TestCheckIngress(t *testing.T) {
 			}
 		})
 
+		t.Run("When snippets are disabled and user tries to use snippet annotation", func(t *testing.T) {
+			nginx.store = fakeIngressStore{
+				ingresses: []*ingress.Ingress{},
+				configuration: ngx_config.Configuration{
+					EnableSnippetDirectives: false,
+				},
+			}
+			nginx.command = testNginxTestCommand{
+				t:   t,
+				err: nil,
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/server-snippet"] = "bla"
+			if err := nginx.CheckIngress(ing); err == nil {
+				t.Errorf("with a snippet annotation, ingresses using the default should be rejected")
+			}
+		})
+
 		t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) {
 			backendBefore := ing.Spec.Backend
 			disableCatchAllBefore := nginx.cfg.DisableCatchAll
@@ -284,6 +306,9 @@ func TestCheckIngress(t *testing.T) {
 		})
 
 		t.Run("When the ingress is in a different namespace than the watched one", func(t *testing.T) {
+			defer func() {
+				nginx.cfg.Namespace = "test-namespace"
+			}()
 			nginx.command = testNginxTestCommand{
 				t:   t,
 				err: fmt.Errorf("test error"),
@@ -2075,6 +2100,83 @@ func TestGetBackendServers(t *testing.T) {
 				}
 			},
 		},
+		{
+			Ingresses: []*ingress.Ingress{
+				{
+					Ingress: networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "not-allowed-snippet",
+							Namespace: "default",
+							Annotations: map[string]string{
+								"nginx.ingress.kubernetes.io/server-snippet":         "bla",
+								"nginx.ingress.kubernetes.io/configuration-snippet":  "blo",
+								"nginx.ingress.kubernetes.io/whitelist-source-range": "10.0.0.0/24",
+							},
+						},
+						Spec: networking.IngressSpec{
+							Rules: []networking.IngressRule{
+								{
+									Host: "example.com",
+									IngressRuleValue: networking.IngressRuleValue{
+										HTTP: &networking.HTTPIngressRuleValue{
+											Paths: []networking.HTTPIngressPath{
+												{
+													Path:     "/path1",
+													PathType: &pathTypePrefix,
+													Backend: networking.IngressBackend{
+														ServiceName: "path1-svc",
+														ServicePort: intstr.IntOrString{
+															Type:   intstr.Int,
+															IntVal: 80,
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Whitelist:            ipwhitelist.SourceRange{CIDR: []string{"10.0.0.0/24"}},
+						ServerSnippet:        "bla",
+						ConfigurationSnippet: "blo",
+					},
+				},
+			},
+			Validate: func(ingresses []*ingress.Ingress, upstreams []*ingress.Backend, servers []*ingress.Server) {
+				if len(servers) != 2 {
+					t.Errorf("servers count should be 2, got %d", len(servers))
+					return
+				}
+				s := servers[1]
+
+				if s.ServerSnippet != "" {
+					t.Errorf("server snippet should be empty, got '%s'", s.ServerSnippet)
+				}
+
+				if s.Locations[0].ConfigurationSnippet != "" {
+					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" {
+					t.Errorf("allow list was incorrectly dropped, len should be 1 and contain 10.0.0.0/24")
+				}
+
+			},
+			SetConfigMap: func(ns string) *v1.ConfigMap {
+				return &v1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:     "config",
+						SelfLink: fmt.Sprintf("/api/v1/namespaces/%s/configmaps/config", ns),
+					},
+					Data: map[string]string{
+						"enable-snippet-directives": "false",
+					},
+				}
+			},
+		},
 	}
 
 	for _, testCase := range testCases {

test/e2e/annotations/modsecurity.go

@@ -282,7 +282,7 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 
 		f.WaitForNginxServer(host,
 			func(server string) bool {
-				return true
+				return strings.Contains(server, "SecRequestBodyAccess On")
 			})
 
 		f.HTTPTestClient().
@@ -292,4 +292,44 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 			Expect().
 			Status(http.StatusForbidden)
 	})
+
+	ginkgo.It("should enable modsecurity through the config map but ignore snippet as disabled by admin", func() {
+		host := "modsecurity.foo.com"
+		nameSpace := f.Namespace
+
+		snippet := `SecRequestBodyAccess On
+		SecAuditEngine RelevantOnly
+		SecAuditLogParts ABIJDEFHZ
+		SecAuditLog /dev/stdout
+		SecAuditLogType Serial
+		SecRule REQUEST_HEADERS:User-Agent \"block-ua\" \"log,deny,id:107,status:403,msg:\'UA blocked\'\"`
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
+		}
+
+		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
+		f.EnsureIngress(ing)
+
+		expectedComment := "SecRuleEngine On"
+
+		f.SetNginxConfigMapData(map[string]string{
+			"enable-modsecurity":           "true",
+			"enable-owasp-modsecurity-crs": "true",
+			"enable-snippet-directives":    "false",
+			"modsecurity-snippet":          expectedComment,
+		})
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return !strings.Contains(server, "block-ua")
+			})
+
+		f.HTTPTestClient().
+			GET("/").
+			WithHeader("Host", host).
+			WithHeader("User-Agent", "block-ua").
+			Expect().
+			Status(http.StatusOK)
+	})
 })