Jail/chroot nginx process inside controller container (kubernetes#8337)

* Initial work on chrooting nginx process

* More improvements in chroot

* Fix charts and some file locations

* Fix symlink on non chrooted container

* fix psp test

* Add e2e tests to chroot image

* Fix logger

* Add internal logger in controller

* Fix overlay for chrooted tests

* Fix tests

* fix boilerplates

* Fix unittest to point to the right pid

* Fix PR review

.github/workflows/ci.yaml

@@ -100,13 +100,14 @@ jobs:
           REGISTRY: ingress-controller
         run: |
           echo "building images..."
-          make clean-image build image
+          make clean-image build image image-chroot
           make -C test/e2e-image image
 
           echo "creating images cache..."
           docker save \
             nginx-ingress-controller:e2e \
             ingress-controller/controller:1.0.0-dev \
+            ingress-controller/controller-chroot:1.0.0-dev \
             | pigz > docker.tar.gz
 
       - name: cache
@@ -250,6 +251,65 @@ jobs:
           kind get kubeconfig > $HOME/.kube/kind-config-kind
           make kind-e2e-test
 
+  kubernetes-chroot:
+    name: Kubernetes chroot
+    runs-on: ubuntu-latest
+    needs:
+      - changes
+      - build
+    if: |
+      (needs.changes.outputs.go == 'true')
+
+    strategy:
+      matrix:
+        k8s: [v1.21.10, v1.22.7, v1.23.4]
+
+    steps:
+
+      - name: Checkout
+        uses: actions/checkout@v2
+
+      - name: cache
+        uses: actions/download-artifact@v2
+        with:
+          name: docker.tar.gz
+
+      - name: Create Kubernetes ${{ matrix.k8s }} cluster
+        id: kind
+        uses: engineerd/[email protected]
+        with:
+          version: v0.12.0
+          config: test/e2e/kind.yaml
+          image: kindest/node:${{ matrix.k8s }}
+
+      - uses: geekyeggo/delete-artifact@v1
+        with:
+          name: docker.tar.gz
+          failOnError: false
+
+      - name: Prepare cluster for testing
+        id: local-path
+        run: |
+          kubectl version
+          echo
+          echo "installing helm 3..."
+          curl -sSL https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash
+
+      - name: Load images from cache
+        run: |
+          echo "loading docker images..."
+          pigz -dc docker.tar.gz | docker load
+
+      - name: Run e2e tests
+        env:
+          KIND_CLUSTER_NAME: kind
+          SKIP_CLUSTER_CREATION: true
+          SKIP_IMAGE_CREATION: true
+          IS_CHROOT: true
+        run: |
+          kind get kubeconfig > $HOME/.kube/kind-config-kind
+          make kind-e2e-test
+
   test-image-build:
     permissions:
       contents: read  # for dorny/paths-filter to fetch a list of changed files

Makefile

@@ -75,11 +75,30 @@ image: clean-image ## Build image for a particular arch.
 		--build-arg BUILD_ID="$(BUILD_ID)" \
 		-t $(REGISTRY)/controller:$(TAG) rootfs
 
+.PHONY: image-chroot
+image-chroot: clean-chroot-image ## Build image for a particular arch.
+	echo "Building docker image ($(ARCH))..."
+	@docker build \
+		--no-cache \
+		--build-arg BASE_IMAGE="$(BASE_IMAGE)" \
+		--build-arg VERSION="$(TAG)" \
+		--build-arg TARGETARCH="$(ARCH)" \
+		--build-arg COMMIT_SHA="$(COMMIT_SHA)" \
+		--build-arg BUILD_ID="$(BUILD_ID)" \
+		-t $(REGISTRY)/controller-chroot:$(TAG) rootfs -f rootfs/Dockerfile.chroot
+
 .PHONY: clean-image
 clean-image: ## Removes local image
 	echo "removing old image $(REGISTRY)/controller:$(TAG)"
 	@docker rmi -f $(REGISTRY)/controller:$(TAG) || true
 
+
+.PHONY: clean-chroot-image
+clean-chroot-image: ## Removes local image
+	echo "removing old image $(REGISTRY)/controller-chroot:$(TAG)"
+	@docker rmi -f $(REGISTRY)/controller-chroot:$(TAG) || true
+
+
 .PHONY: build
 build:  ## Build ingress controller, debug tool and pre-stop hook.
 	@build/run-in-docker.sh \
@@ -221,3 +240,14 @@ release: ensure-buildx clean
 		--build-arg COMMIT_SHA="$(COMMIT_SHA)" \
 		--build-arg BUILD_ID="$(BUILD_ID)" \
 		-t $(REGISTRY)/controller:$(TAG) rootfs
+
+	@docker buildx build \
+		--no-cache \
+		--push \
+		--progress plain \
+		--platform $(subst $(SPACE),$(COMMA),$(PLATFORMS)) \
+		--build-arg BASE_IMAGE="$(BASE_IMAGE)" \
+		--build-arg VERSION="$(TAG)" \
+		--build-arg COMMIT_SHA="$(COMMIT_SHA)" \
+		--build-arg BUILD_ID="$(BUILD_ID)" \
+		-t $(REGISTRY)/controller-chroot:$(TAG) rootfs -f rootfs/Dockerfile.chroot

build/build.sh

@@ -69,3 +69,4 @@ go build \
     -X ${PKG}/version.COMMIT=${COMMIT_SHA} \
     -X ${PKG}/version.REPO=${REPO_INFO}" \
   -o "${TARGETS_DIR}/wait-shutdown" "${PKG}/cmd/waitshutdown"
+

build/test.sh

@@ -23,6 +23,7 @@ set -o errexit
 set -o nounset
 set -o pipefail
 
+mkdir -p /tmp/nginx
 if [ -z "${PKG}" ]; then
   echo "PKG must be set"
   exit 1

charts/ingress-nginx/README.md

@@ -306,6 +306,7 @@ Kubernetes: `>=1.19.0-0`
 | controller.hostPort.ports.https | int | `443` | 'hostPort' https port |
 | controller.hostname | object | `{}` | Optionally customize the pod hostname. |
 | controller.image.allowPrivilegeEscalation | bool | `true` |  |
+| controller.image.chroot | bool | `false` |  |
 | controller.image.digest | string | `"sha256:31f47c1e202b39fadecf822a9b76370bd4baed199a005b3e7d4d1455f4fd3fe2"` |  |
 | controller.image.image | string | `"ingress-nginx/controller"` |  |
 | controller.image.pullPolicy | string | `"IfNotPresent"` |  |

charts/ingress-nginx/templates/_helpers.tpl

@@ -43,11 +43,40 @@ capabilities:
   - ALL
   add:
   - NET_BIND_SERVICE
+  {{- if .Values.controller.image.chroot }}
+  - SYS_CHROOT
+  {{- end }}
 runAsUser: {{ .Values.controller.image.runAsUser }}
 allowPrivilegeEscalation: {{ .Values.controller.image.allowPrivilegeEscalation }}
 {{- end }}
 {{- end -}}
 
+{{/*
+Get specific image
+*/}}
+{{- define "ingress-nginx.image" -}}
+{{- if .chroot -}}
+{{- printf "%s-chroot" .image -}}
+{{- else -}}
+{{- printf "%s" .image -}}
+{{- end }}
+{{- end -}}
+
+{{/*
+Get specific image digest
+*/}}
+{{- define "ingress-nginx.imageDigest" -}}
+{{- if .chroot -}}
+{{- if .digestChroot -}}
+{{- printf "@%s" .digestChroot -}}
+{{- end }}
+{{- else -}}
+{{ if .digest -}}
+{{- printf "@%s" .digest -}}
+{{- end -}}
+{{- end -}}
+{{- end -}}
+
 {{/*
 Create a default fully qualified controller name.
 We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).

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

@@ -74,7 +74,7 @@ spec:
       containers:
         - name: {{ .Values.controller.containerName }}
           {{- with .Values.controller.image }}
-          image: "{{- if .repository -}}{{ .repository }}{{ else }}{{ .registry }}/{{ .image }}{{- end -}}:{{ .tag }}{{- if (.digest) -}} @{{.digest}} {{- end -}}"
+          image: "{{- if .repository -}}{{ .repository }}{{ else }}{{ .registry }}/{{ include "ingress-nginx.image" . }}{{- end -}}:{{ .tag }}{{ include "ingress-nginx.imageDigest" . }}"
           {{- end }}
           imagePullPolicy: {{ .Values.controller.image.pullPolicy }}
         {{- if .Values.controller.lifecycle }}

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

@@ -78,7 +78,7 @@ spec:
       containers:
         - name: {{ .Values.controller.containerName }}
           {{- with .Values.controller.image }}
-          image: "{{- if .repository -}}{{ .repository }}{{ else }}{{ .registry }}/{{ .image }}{{- end -}}:{{ .tag }}{{- if (.digest) -}} @{{.digest}} {{- end -}}"
+          image: "{{- if .repository -}}{{ .repository }}{{ else }}{{ .registry }}/{{ include "ingress-nginx.image" . }}{{- end -}}:{{ .tag }}{{ include "ingress-nginx.imageDigest" . }}"
           {{- end }}
           imagePullPolicy: {{ .Values.controller.image.pullPolicy }}
         {{- if .Values.controller.lifecycle }}

charts/ingress-nginx/values.yaml

@@ -16,13 +16,16 @@ commonLabels: {}
 controller:
   name: controller
   image:
+    ## Keep false as default for now!
+    chroot: false
     registry: k8s.gcr.io
     image: ingress-nginx/controller
     ## for backwards compatibility consider setting the full image url via the repository value below
     ## use *either* current default registry/image or repository format or installing chart by providing the values.yaml will fail
     ## repository:
     tag: "v1.1.3"
     digest: sha256:31f47c1e202b39fadecf822a9b76370bd4baed199a005b3e7d4d1455f4fd3fe2
+    # digestChroot: "" # TODO: Fill when we have it
     pullPolicy: IfNotPresent
     # www-data -> uid 101
     runAsUser: 101

cmd/nginx/flags.go

@@ -192,6 +192,8 @@ Takes the form "<host>:port". If not provided, no admission controller is starte
 		statusPort = flags.Int("status-port", 10246, `Port to use for the lua HTTP endpoint configuration.`)
 		streamPort = flags.Int("stream-port", 10247, "Port to use for the lua TCP/UDP endpoint configuration.")
 
+		internalLoggerAddress = flags.String("internal-logger-address", "127.0.0.1:11514", "Address to be used when binding internal syslogger")
+
 		profilerPort = flags.Int("profiler-port", 10245, "Port to use for expose the ingress controller Go profiler when it is enabled.")
 
 		statusUpdateInterval = flags.Int("status-update-interval", status.UpdateInterval, "Time interval in seconds in which the status should check if an update is required. Default is 60 seconds")
@@ -344,6 +346,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 		ValidationWebhook:         *validationWebhook,
 		ValidationWebhookCertPath: *validationWebhookCert,
 		ValidationWebhookKeyPath:  *validationWebhookKey,
+		InternalLoggerAddress:     *internalLoggerAddress,
 	}
 
 	if *apiserverHost != "" {

cmd/nginx/logger.go

@@ -0,0 +1,51 @@
+/*
+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 main
+
+import (
+	"fmt"
+
+	"k8s.io/klog/v2"
+
+	"gopkg.in/mcuadros/go-syslog.v2"
+)
+
+func logger(address string) {
+	channel := make(syslog.LogPartsChannel)
+	handler := syslog.NewChannelHandler(channel)
+
+	server := syslog.NewServer()
+
+	server.SetFormat(syslog.RFC3164)
+	server.SetHandler(handler)
+	if err := server.ListenUDP(address); err != nil {
+		klog.Fatalf("failed bind internal syslog: %w", err)
+	}
+
+	if err := server.Boot(); err != nil {
+		klog.Fatalf("failed to boot internal syslog: %w", err)
+	}
+	klog.Infof("Is Chrooted, starting logger")
+
+	for logParts := range channel {
+		fmt.Printf("%s\n", logParts["content"])
+	}
+
+	server.Wait()
+	klog.Infof("Stopping logger")
+
+}

cmd/nginx/main.go

@@ -152,6 +152,13 @@ func main() {
 	registerHealthz(nginx.HealthPath, ngx, mux)
 	registerMetrics(reg, mux)
 
+	_, errExists := os.Stat("/chroot")
+	if errExists == nil {
+		conf.IsChroot = true
+		go logger(conf.InternalLoggerAddress)
+
+	}
+
 	go startHTTPServer(conf.HealthCheckHost, conf.ListenPorts.Health, mux)
 	go ngx.Start()
 

go.mod

@@ -130,6 +130,7 @@ require (
 	google.golang.org/protobuf v1.27.1 // indirect
 	gopkg.in/go-playground/assert.v1 v1.2.1 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
+	gopkg.in/mcuadros/go-syslog.v2 v2.3.0 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect

go.sum

@@ -1214,6 +1214,8 @@ gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc=
 gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw=
 gopkg.in/ini.v1 v1.51.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
 gopkg.in/ini.v1 v1.66.2/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
+gopkg.in/mcuadros/go-syslog.v2 v2.3.0 h1:kcsiS+WsTKyIEPABJBJtoG0KkOS6yzvJ+/eZlhD79kk=
+gopkg.in/mcuadros/go-syslog.v2 v2.3.0/go.mod h1:l5LPIyOOyIdQquNg+oU6Z3524YwrcqEm0aKH+5zpt2U=
 gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24xpD6X8LsfU/+k=
 gopkg.in/resty.v1 v1.12.0/go.mod h1:mDo4pnntr5jdWRML875a/NmxYqAlA73dVijT2AXvQQo=
 gopkg.in/square/go-jose.v2 v2.2.2/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI=

internal/ingress/controller/checker_test.go

@@ -76,7 +76,7 @@ func TestNginxCheck(t *testing.T) {
 			})
 
 			// create pid file
-			os.MkdirAll("/tmp", file.ReadWriteByUser)
+			os.MkdirAll("/tmp/nginx", file.ReadWriteByUser)
 			pidFile, err := os.Create(nginx.PID)
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)

internal/ingress/controller/config/config.go

@@ -958,12 +958,11 @@ type TemplateConfig struct {
 	EnableMetrics            bool
 	MaxmindEditionFiles      *[]string
 	MonitorMaxBatchSize      int
-
-	PID            string
-	StatusPath     string
-	StatusPort     int
-	StreamPort     int
-	StreamSnippets []string
+	PID                      string
+	StatusPath               string
+	StatusPort               int
+	StreamPort               int
+	StreamSnippets           []string
 }
 
 // ListenPorts describe the ports required to run the

internal/ingress/controller/controller.go

@@ -120,6 +120,9 @@ type Configuration struct {
 
 	PostShutdownGracePeriod int
 	ShutdownGracePeriod     int
+
+	InternalLoggerAddress string
+	IsChroot              bool
 }
 
 // GetPublishService returns the Service used to set the load-balancer status of Ingresses.

internal/ingress/controller/nginx.go

@@ -575,6 +575,15 @@ func (n NGINXController) generateTemplate(cfg ngx_config.Configuration, ingressC
 
 	cfg.DefaultSSLCertificate = n.getDefaultSSLCertificate()
 
+	if n.cfg.IsChroot {
+		if cfg.AccessLogPath == "/var/log/nginx/access.log" {
+			cfg.AccessLogPath = fmt.Sprintf("syslog:server=%s", n.cfg.InternalLoggerAddress)
+		}
+		if cfg.ErrorLogPath == "/var/log/nginx/error.log" {
+			cfg.ErrorLogPath = fmt.Sprintf("syslog:server=%s", n.cfg.InternalLoggerAddress)
+		}
+	}
+
 	tc := ngx_config.TemplateConfig{
 		ProxySetHeaders:          setHeaders,
 		AddHeaders:               addHeaders,
@@ -614,7 +623,8 @@ func (n NGINXController) testTemplate(cfg []byte) error {
 	if len(cfg) == 0 {
 		return fmt.Errorf("invalid NGINX configuration (empty)")
 	}
-	tmpfile, err := os.CreateTemp("", tempNginxPattern)
+	tmpDir := os.TempDir() + "/nginx"
+	tmpfile, err := os.CreateTemp(tmpDir, tempNginxPattern)
 	if err != nil {
 		return err
 	}

internal/ingress/controller/process/nginx.go

@@ -20,7 +20,7 @@ import (
 	"os/exec"
 	"syscall"
 
-	"k8s.io/klog/v2"
+	klog "k8s.io/klog/v2"
 )
 
 // IsRespawnIfRequired checks if error type is exec.ExitError or not