Implement object deep inspector (#8456)

cmd/nginx/flags.go

@@ -201,6 +201,8 @@ Takes the form "<host>:port". If not provided, no admission controller is starte
 		shutdownGracePeriod = flags.Int("shutdown-grace-period", 0, "Seconds to wait after receiving the shutdown signal, before stopping the nginx process.")
 
 		postShutdownGracePeriod = flags.Int("post-shutdown-grace-period", 10, "Seconds to wait after the nginx process has stopped before controller exits.")
+
+		deepInspector = flags.Bool("deep-inspect", true, "Enables ingress object security deep inspector")
 	)
 
 	flags.StringVar(&nginx.MaxmindMirror, "maxmind-mirror", "", `Maxmind mirror url (example: http://geoip.local/databases`)
@@ -321,6 +323,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 		UDPConfigMapName:           *udpConfigMapName,
 		DisableFullValidationTest:  *disableFullValidationTest,
 		DefaultSSLCertificate:      *defSSLCertificate,
+		DeepInspector:              *deepInspector,
 		PublishService:             *publishSvc,
 		PublishStatusAddress:       *publishStatusAddress,
 		UpdateStatusOnShutdown:     *updateStatusOnShutdown,

docs/user-guide/cli-arguments.md

@@ -12,6 +12,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment
 | `--apiserver-host`                 | Address of the Kubernetes API server. Takes the form "protocol://address:port". If not specified, it is assumed the program runs inside a Kubernetes cluster and local discovery is attempted. |
 | `--certificate-authority`          | Path to a cert file for the certificate authority. This certificate is used only when the flag --apiserver-host is specified. |
 | `--configmap`                      | Name of the ConfigMap containing custom global configurations for the controller. |
+| `--deep-inspect`                   | Enables ingress object security deep inspector. (default true) |
 | `--default-backend-service`        | Service used to serve HTTP requests not matching any known server name (catch-all). Takes the form "namespace/name". The controller configures NGINX to forward requests to the first port of this Service. |
 | `--default-server-port`            | Port to use for exposing the default server (catch-all). (default 8181) |
 | `--default-ssl-certificate`        | Secret containing a SSL certificate to be used by the default HTTPS server (catch-all). Takes the form "namespace/name". |

internal/ingress/controller/controller.go

@@ -41,6 +41,7 @@ import (
 	"k8s.io/ingress-nginx/internal/ingress/controller/ingressclass"
 	"k8s.io/ingress-nginx/internal/ingress/controller/store"
 	"k8s.io/ingress-nginx/internal/ingress/errors"
+	"k8s.io/ingress-nginx/internal/ingress/inspector"
 	"k8s.io/ingress-nginx/internal/ingress/metric/collectors"
 	"k8s.io/ingress-nginx/internal/k8s"
 	"k8s.io/ingress-nginx/internal/nginx"
@@ -123,6 +124,7 @@ type Configuration struct {
 
 	InternalLoggerAddress string
 	IsChroot              bool
+	DeepInspector         bool
 }
 
 // GetPublishService returns the Service used to set the load-balancer status of Ingresses.
@@ -237,7 +239,11 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 	if !ing.DeletionTimestamp.IsZero() {
 		return nil
 	}
-
+	if n.cfg.DeepInspector {
+		if err := inspector.DeepInspect(ing); err != nil {
+			return fmt.Errorf("invalid object: %w", err)
+		}
+	}
 	// Do not attempt to validate an ingress that's not meant to be controlled by the current instance of the controller.
 	if ingressClass, err := n.store.GetIngressClass(ing, n.cfg.IngressClassConfiguration); ingressClass == "" {
 		klog.Warningf("ignoring ingress %v in %v based on annotation %v: %v", ing.Name, ing.ObjectMeta.Namespace, ingressClass, err)

internal/ingress/controller/controller_test.go

@@ -2396,6 +2396,7 @@ func newNGINXController(t *testing.T) *NGINXController {
 		clientSet,
 		channels.NewRingChannel(10),
 		false,
+		true,
 		&ingressclass.IngressClassConfiguration{
 			Controller:      "k8s.io/ingress-nginx",
 			AnnotationValue: "nginx",
@@ -2460,6 +2461,7 @@ func newDynamicNginxController(t *testing.T, setConfigMap func(string) *v1.Confi
 		clientSet,
 		channels.NewRingChannel(10),
 		false,
+		true,
 		&ingressclass.IngressClassConfiguration{
 			Controller:      "k8s.io/ingress-nginx",
 			AnnotationValue: "nginx",

internal/ingress/controller/nginx.go

@@ -131,6 +131,7 @@ func NewNGINXController(config *Configuration, mc metric.Collector) *NGINXContro
 		config.Client,
 		n.updateCh,
 		config.DisableCatchAll,
+		config.DeepInspector,
 		config.IngressClassConfiguration)
 
 	n.syncQueue = task.NewTaskQueue(n.syncIngress)

internal/ingress/controller/store/store.go

@@ -42,6 +42,7 @@ import (
 	clientcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/ingress-nginx/internal/ingress/inspector"
 	"k8s.io/klog/v2"
 
 	"k8s.io/ingress-nginx/internal/file"
@@ -247,6 +248,7 @@ func New(
 	client clientset.Interface,
 	updateCh *channels.RingChannel,
 	disableCatchAll bool,
+	deepInspector bool,
 	icConfig *ingressclass.IngressClassConfiguration) Storer {
 
 	store := &k8sStore{
@@ -426,6 +428,12 @@ func New(
 
 			klog.InfoS("Found valid IngressClass", "ingress", klog.KObj(ing), "ingressclass", ic)
 
+			if deepInspector {
+				if err := inspector.DeepInspect(ing); err != nil {
+					klog.ErrorS(err, "received invalid ingress", "ingress", klog.KObj(ing))
+					return
+				}
+			}
 			if hasCatchAllIngressRule(ing.Spec) && disableCatchAll {
 				klog.InfoS("Ignoring add for catch-all ingress because of --disable-catch-all", "ingress", klog.KObj(ing))
 				return
@@ -482,6 +490,13 @@ func New(
 				return
 			}
 
+			if deepInspector {
+				if err := inspector.DeepInspect(curIng); err != nil {
+					klog.ErrorS(err, "received invalid ingress", "ingress", klog.KObj(curIng))
+					return
+				}
+			}
+
 			store.syncIngress(curIng)
 			store.updateSecretIngressMap(curIng)
 			store.syncSecrets(curIng)

internal/ingress/controller/store/store_test.go

@@ -124,6 +124,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -204,6 +205,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -307,6 +309,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -422,6 +425,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			ingressClassconfig)
 
 		storer.Run(stopCh)
@@ -551,6 +555,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			ingressClassconfig)
 
 		storer.Run(stopCh)
@@ -650,6 +655,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -743,6 +749,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -828,6 +835,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -923,6 +931,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -1046,6 +1055,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)
@@ -1166,6 +1176,7 @@ func TestStore(t *testing.T) {
 			clientSet,
 			updateCh,
 			false,
+			true,
 			DefaultClassConfig)
 
 		storer.Run(stopCh)

internal/ingress/inspector/ingress.go

@@ -0,0 +1,62 @@
+/*
+Copyright 2022 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 inspector
+
+import (
+	"fmt"
+
+	networking "k8s.io/api/networking/v1"
+)
+
+// InspectIngress is used to do the deep inspection of an ingress object, walking through all
+// of the spec fields and checking for matching strings and configurations that may represent
+// an attempt to escape configs
+func InspectIngress(ingress *networking.Ingress) error {
+	for _, rule := range ingress.Spec.Rules {
+		if rule.Host != "" {
+			if err := CheckRegex(rule.Host); err != nil {
+				return fmt.Errorf("invalid host in ingress %s/%s: %s", ingress.Namespace, ingress.Name, err)
+			}
+		}
+		if rule.HTTP != nil {
+			if err := inspectIngressRule(rule.HTTP); err != nil {
+				return fmt.Errorf("invalid rule in ingress %s/%s: %s", ingress.Namespace, ingress.Name, err)
+			}
+		}
+	}
+
+	for _, tls := range ingress.Spec.TLS {
+		if err := CheckRegex(tls.SecretName); err != nil {
+			return fmt.Errorf("invalid secret in ingress %s/%s: %s", ingress.Namespace, ingress.Name, err)
+		}
+		for _, host := range tls.Hosts {
+			if err := CheckRegex(host); err != nil {
+				return fmt.Errorf("invalid host in ingress tls config %s/%s: %s", ingress.Namespace, ingress.Name, err)
+			}
+		}
+	}
+	return nil
+}
+
+func inspectIngressRule(httprule *networking.HTTPIngressRuleValue) error {
+	for _, path := range httprule.Paths {
+		if err := CheckRegex(path.Path); err != nil {
+			return fmt.Errorf("invalid http path: %s", err)
+		}
+	}
+	return nil
+}

internal/ingress/inspector/ingress_test.go

@@ -0,0 +1,99 @@
+/*
+Copyright 2022 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 inspector
+
+import (
+	"testing"
+
+	networking "k8s.io/api/networking/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func makeSimpleIngress(hostname string, paths ...string) *networking.Ingress {
+
+	newIngress := networking.Ingress{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test1",
+			Namespace: "default",
+		},
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					Host: hostname,
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{
+							Paths: []networking.HTTPIngressPath{},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	prefix := networking.PathTypePrefix
+	for _, path := range paths {
+		newPath := networking.HTTPIngressPath{
+			Path:     path,
+			PathType: &prefix,
+		}
+		newIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths = append(newIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths, newPath)
+	}
+	return &newIngress
+}
+
+func TestInspectIngress(t *testing.T) {
+	tests := []struct {
+		name     string
+		hostname string
+		path     []string
+		wantErr  bool
+	}{
+		{
+			name:     "invalid-path-etc",
+			hostname: "invalid.etc.com",
+			path: []string{
+				"/var/run/secrets",
+				"/mypage",
+			},
+			wantErr: true,
+		},
+		{
+			name:     "invalid-path-etc",
+			hostname: "invalid.etc.com",
+			path: []string{
+				"/etc/nginx",
+			},
+			wantErr: true,
+		},
+		{
+			name:     "invalid-path-etc",
+			hostname: "invalid.etc.com",
+			path: []string{
+				"/mypage",
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ingress := makeSimpleIngress(tt.hostname, tt.path...)
+			if err := InspectIngress(ingress); (err != nil) != tt.wantErr {
+				t.Errorf("InspectIngress() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

internal/ingress/inspector/inspector.go

@@ -0,0 +1,38 @@
+/*
+Copyright 2022 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 inspector
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	networking "k8s.io/api/networking/v1"
+	"k8s.io/klog/v2"
+)
+
+// DeepInspect is the function called by admissionwebhook and store syncer to check
+// if an object contains invalid configurations that may represent a security risk,
+// and returning an error in this case
+func DeepInspect(obj interface{}) error {
+	switch obj.(type) {
+	case *networking.Ingress:
+		return InspectIngress(obj.(*networking.Ingress))
+	case *corev1.Service:
+		return InspectService(obj.(*corev1.Service))
+	default:
+		klog.Warningf("received invalid object to inspect: %T", obj)
+		return nil
+	}
+}

internal/ingress/inspector/rules.go

@@ -0,0 +1,53 @@
+/*
+Copyright 2022 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 inspector
+
+import (
+	"fmt"
+	"regexp"
+)
+
+var (
+	invalidAliasDirective = regexp.MustCompile(`\s*alias\s*.*;`)
+	invalidRootDirective  = regexp.MustCompile(`\s*root\s*.*;`)
+	invalidEtcDir         = regexp.MustCompile(`/etc/(passwd|shadow|group|nginx|ingress-controller)`)
+	invalidSecretsDir     = regexp.MustCompile(`/var/run/secrets`)
+	invalidByLuaDirective = regexp.MustCompile(`.*_by_lua.*`)
+
+	invalidRegex = []regexp.Regexp{}
+)
+
+func init() {
+	invalidRegex = []regexp.Regexp{
+		*invalidAliasDirective,
+		*invalidRootDirective,
+		*invalidEtcDir,
+		*invalidSecretsDir,
+		*invalidByLuaDirective,
+	}
+}
+
+// CheckRegex receives a value/configuration and validates if it matches with one of the
+// forbidden regexes.
+func CheckRegex(value string) error {
+	for _, regex := range invalidRegex {
+		if regex.MatchString(value) {
+			return fmt.Errorf("invalid value found: %s", value)
+		}
+	}
+	return nil
+}

internal/ingress/inspector/rules_test.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2022 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 inspector
+
+import "testing"
+
+func TestCheckRegex(t *testing.T) {
+
+	tests := []struct {
+		name    string
+		value   string
+		wantErr bool
+	}{
+		{
+			name:    "must refuse invalid root",
+			wantErr: true,
+			value:   "   root    blabla/lala ;",
+		},
+		{
+			name:    "must refuse invalid alias",
+			wantErr: true,
+			value:   "   alias    blabla/lala ;",
+		},
+		{
+			name:    "must refuse invalid attempt to call /etc",
+			wantErr: true,
+			value:   "location /etc/nginx/lalala",
+		},
+		{
+			name:    "must refuse invalid attempt to call k8s secret",
+			wantErr: true,
+			value:   "ssl_cert /var/run/secrets/kubernetes.io/lalala; xpto",
+		},
+		{
+			name:    "must refuse invalid attempt to call lua directives",
+			wantErr: true,
+			value:   "set_by_lua lala",
+		},
+		{
+			name:    "must pass with valid configuration",
+			wantErr: false,
+			value:   "/test/mypage1",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := CheckRegex(tt.value); (err != nil) != tt.wantErr {
+				t.Errorf("CheckRegex() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

internal/ingress/inspector/service.go

@@ -0,0 +1,26 @@
+/*
+Copyright 2022 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 inspector
+
+import (
+	corev1 "k8s.io/api/core/v1"
+)
+
+// InspectService will be used to inspect service objects for possible invalid configurations
+func InspectService(svc *corev1.Service) error {
+	return nil
+}

test/e2e/admission/admission.go

@@ -110,6 +110,23 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
 		assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress with the same host and path should not return an error using a canary annotation")
 	})
 
+	ginkgo.It("should block ingress with invalid path", func() {
+		host := "invalid-path"
+
+		firstIngress := framework.NewSingleIngress("valid-path", "/mypage", host, f.Namespace, framework.EchoService, 80, nil)
+		_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
+		assert.Nil(ginkgo.GinkgoT(), err, "creating ingress")
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, fmt.Sprintf("server_name %v", host))
+			})
+
+		secondIngress := framework.NewSingleIngress("second-ingress", "/etc/nginx", host, f.Namespace, framework.EchoService, 80, nil)
+		_, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), secondIngress, metav1.CreateOptions{})
+		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid path should return an error")
+	})
+
 	ginkgo.It("should return an error if there is an error validating the ingress definition", func() {
 		host := "admission-test"
 

test/e2e/ingress/deep_inspection.go

@@ -0,0 +1,67 @@
+/*
+Copyright 2022 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 ingress
+
+import (
+	"net/http"
+	"strings"
+
+	"github.com/onsi/ginkgo"
+	"k8s.io/ingress-nginx/test/e2e/framework"
+)
+
+var _ = framework.IngressNginxDescribe("[Ingress] DeepInspection", func() {
+	f := framework.NewDefaultFramework("deep-inspection")
+
+	ginkgo.BeforeEach(func() {
+		f.NewEchoDeployment()
+	})
+
+	ginkgo.It("should drop whole ingress if one path matches invalid regex", func() {
+		host := "inspection123.com"
+
+		ingInvalid := framework.NewSingleIngress("invalidregex", "/bla{alias /var/run/secrets/;}location ~* ^/abcd", host, f.Namespace, framework.EchoService, 80, nil)
+		f.EnsureIngress(ingInvalid)
+		ingValid := framework.NewSingleIngress("valid", "/xpto", host, f.Namespace, framework.EchoService, 80, nil)
+		f.EnsureIngress(ingValid)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, host) &&
+					strings.Contains(server, "location /xpto") &&
+					!strings.Contains(server, "location /bla")
+			})
+
+		f.HTTPTestClient().
+			GET("/xpto").
+			WithHeader("Host", host).
+			Expect().
+			Status(http.StatusOK)
+
+		f.HTTPTestClient().
+			GET("/bla").
+			WithHeader("Host", host).
+			Expect().
+			Status(http.StatusNotFound)
+
+		f.HTTPTestClient().
+			GET("/abcd/").
+			WithHeader("Host", host).
+			Expect().
+			Status(http.StatusNotFound)
+	})
+})