Merge pull request #2619 from aledbf/non-root

Run as user dropping privileges

internal/file/filesystem.go

@@ -25,6 +25,12 @@ import (
 	"k8s.io/kubernetes/pkg/util/filesystem"
 )
 
+// ReadWriteByUser defines linux permission to read and write files for the owner user
+const ReadWriteByUser = 0660
+
+// ReadByUserGroup defines linux permission to read files by the user and group owner/s
+const ReadByUserGroup = 0640
+
 // Filesystem is an interface that we can use to mock various filesystem operations
 type Filesystem interface {
 	filesystem.Filesystem
@@ -35,7 +41,7 @@ func NewLocalFS() (Filesystem, error) {
 	fs := filesystem.DefaultFs{}
 
 	for _, directory := range directories {
-		err := fs.MkdirAll(directory, 0655)
+		err := fs.MkdirAll(directory, ReadWriteByUser)
 		if err != nil {
 			return nil, err
 		}
@@ -97,12 +103,5 @@ func NewFakeFS() (Filesystem, error) {
 		}
 	}
 
-	fakeFs.MkdirAll("/run", 0655)
-	fakeFs.MkdirAll("/proc", 0655)
-	fakeFs.MkdirAll("/etc/nginx/template", 0655)
-
-	fakeFs.MkdirAll(DefaultSSLDirectory, 0655)
-	fakeFs.MkdirAll(AuthDirectory, 0655)
-
 	return fakeFs, nil
 }

internal/ingress/annotations/auth/main.go

@@ -19,8 +19,6 @@ package auth
 import (
 	"fmt"
 	"io/ioutil"
-	"os"
-	"path"
 	"regexp"
 
 	"github.com/pkg/errors"
@@ -86,17 +84,6 @@ type auth struct {
 
 // NewParser creates a new authentication annotation parser
 func NewParser(authDirectory string, r resolver.Resolver) parser.IngressAnnotation {
-	os.MkdirAll(authDirectory, 0755)
-
-	currPath := authDirectory
-	for currPath != "/" {
-		currPath = path.Dir(currPath)
-		err := os.Chmod(currPath, 0755)
-		if err != nil {
-			break
-		}
-	}
-
 	return auth{r, authDirectory}
 }
 
@@ -157,8 +144,7 @@ func dumpSecret(filename string, secret *api.Secret) error {
 		}
 	}
 
-	// TODO: check permissions required
+	err := ioutil.WriteFile(filename, val, file.ReadWriteByUser)
-	err := ioutil.WriteFile(filename, val, 0777)
 	if err != nil {
 		return ing_errors.LocationDenied{
 			Reason: errors.Wrap(err, "unexpected error creating password file"),

internal/ingress/controller/checker.go

@@ -26,6 +26,8 @@ import (
 	"github.com/pkg/errors"
 )
 
+const nginxPID = "/tmp/nginx.pid"
+
 // Name returns the healthcheck name
 func (n NGINXController) Name() string {
 	return "nginx-ingress-controller"
@@ -58,13 +60,13 @@ func (n *NGINXController) Check(_ *http.Request) error {
 	if err != nil {
 		return errors.Wrap(err, "unexpected error reading /proc directory")
 	}
-	f, err := n.fileSystem.ReadFile("/run/nginx.pid")
+	f, err := n.fileSystem.ReadFile(nginxPID)
 	if err != nil {
-		return errors.Wrap(err, "unexpected error reading /run/nginx.pid")
+		return errors.Wrapf(err, "unexpected error reading %v", nginxPID)
 	}
 	pid, err := strconv.Atoi(strings.TrimRight(string(f), "\r\n"))
 	if err != nil {
-		return errors.Wrap(err, "unexpected error reading the PID from /run/nginx.pid")
+		return errors.Wrapf(err, "unexpected error reading the nginx PID from %v", nginxPID)
 	}
 	_, err = fs.NewProc(pid)
 

internal/ingress/controller/checker_test.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apiserver/pkg/server/healthz"
 	"k8s.io/kubernetes/pkg/util/filesystem"
 
+	"k8s.io/ingress-nginx/internal/file"
 	ngx_config "k8s.io/ingress-nginx/internal/ingress/controller/config"
 )
 
@@ -60,8 +61,8 @@ func TestNginxCheck(t *testing.T) {
 	})
 
 	// create pid file
-	fs.MkdirAll("/run", 0655)
+	fs.MkdirAll("/tmp", file.ReadWriteByUser)
-	pidFile, err := fs.Create("/run/nginx.pid")
+	pidFile, err := fs.Create(nginxPID)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

internal/ingress/controller/nginx.go

@@ -423,7 +423,7 @@ func (n NGINXController) testTemplate(cfg []byte) error {
 		return err
 	}
 	defer tmpfile.Close()
-	err = ioutil.WriteFile(tmpfile.Name(), cfg, 0644)
+	err = ioutil.WriteFile(tmpfile.Name(), cfg, file.ReadWriteByUser)
 	if err != nil {
 		return err
 	}
@@ -647,7 +647,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
 				return err
 			}
 			defer tmpfile.Close()
-			err = ioutil.WriteFile(tmpfile.Name(), content, 0644)
+			err = ioutil.WriteFile(tmpfile.Name(), content, file.ReadWriteByUser)
 			if err != nil {
 				return err
 			}
@@ -666,7 +666,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
 		}
 	}
 
-	err = ioutil.WriteFile(cfgPath, content, 0644)
+	err = ioutil.WriteFile(cfgPath, content, file.ReadWriteByUser)
 	if err != nil {
 		return err
 	}

internal/ingress/controller/store/store.go

@@ -699,7 +699,7 @@ func (s *k8sStore) setConfig(cmap *corev1.ConfigMap) {
 			glog.Warningf("unexpected error decoding key ssl-session-ticket-key: %v", err)
 			s.backendConfig.SSLSessionTicketKey = ""
 		}
-		ioutil.WriteFile("/etc/nginx/tickets.key", d, 0644)
+		ioutil.WriteFile("/etc/nginx/tickets.key", d, file.ReadWriteByUser)
 	}
 }
 

internal/net/dns/dns_test.go

@@ -21,6 +21,8 @@ import (
 	"net"
 	"os"
 	"testing"
+
+	"k8s.io/ingress-nginx/internal/file"
 )
 
 func TestGetDNSServers(t *testing.T) {
@@ -32,22 +34,22 @@ func TestGetDNSServers(t *testing.T) {
 		t.Error("expected at least 1 nameserver in /etc/resolv.conf")
 	}
 
-	file, err := ioutil.TempFile("", "fw")
+	f, err := ioutil.TempFile("", "fw")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	defer file.Close()
+	defer f.Close()
-	defer os.Remove(file.Name())
+	defer os.Remove(f.Name())
 
-	ioutil.WriteFile(file.Name(), []byte(`
+	ioutil.WriteFile(f.Name(), []byte(`
 	# comment
 	; comment
 	nameserver 2001:4860:4860::8844
 	nameserver 2001:4860:4860::8888
 	nameserver 8.8.8.8
-	`), 0644)
+	`), file.ReadWriteByUser)
 
-	defResolvConf = file.Name()
+	defResolvConf = f.Name()
 	s, err = GetSystemNameServers()
 	if err != nil {
 		t.Fatalf("unexpected error reading /etc/resolv.conf file: %v", err)

internal/watch/file_watcher_test.go

@@ -21,6 +21,8 @@ import (
 	"os"
 	"testing"
 	"time"
+
+	"k8s.io/ingress-nginx/internal/file"
 )
 
 func prepareTimeout() chan bool {
@@ -33,15 +35,15 @@ func prepareTimeout() chan bool {
 }
 
 func TestFileWatcher(t *testing.T) {
-	file, err := ioutil.TempFile("", "fw")
+	f, err := ioutil.TempFile("", "fw")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	defer file.Close()
+	defer f.Close()
-	defer os.Remove(file.Name())
+	defer os.Remove(f.Name())
 	count := 0
 	events := make(chan bool, 10)
-	fw, err := NewFileWatcher(file.Name(), func() {
+	fw, err := NewFileWatcher(f.Name(), func() {
 		count++
 		if count != 1 {
 			t.Fatalf("expected 1 but returned %v", count)
@@ -58,7 +60,7 @@ func TestFileWatcher(t *testing.T) {
 		t.Fatalf("expected no events before writing a file")
 	case <-timeoutChan:
 	}
-	ioutil.WriteFile(file.Name(), []byte{}, 0644)
+	ioutil.WriteFile(f.Name(), []byte{}, file.ReadWriteByUser)
 	select {
 	case <-events:
 	case <-timeoutChan:

rootfs/Dockerfile

@@ -20,7 +20,8 @@ WORKDIR  /etc/nginx
 
 RUN clean-install \
   diffutils \
-  dumb-init
+  dumb-init \
+  libcap2-bin 
 
 # Create symlinks to redirect nginx logs to stdout and stderr docker log collector
 # This only works if nginx is started with CMD or ENTRYPOINT
@@ -30,6 +31,14 @@ RUN mkdir -p /var/log/nginx \
 
 COPY . /
 
+RUN setcap cap_net_bind_service=+ep /usr/sbin/nginx \
+  && setcap cap_net_bind_service=+ep /nginx-ingress-controller
+
+RUN mkdir -p /etc/ingress-controller/ssl /etc/ingress-controller/auth \
+  && chown -R www-data.www-data /etc/nginx /etc/ingress-controller
+
+USER www-data
+
 ENTRYPOINT ["/usr/bin/dumb-init"]
 
 CMD ["/nginx-ingress-controller"]

rootfs/etc/nginx/nginx.conf

@@ -1,5 +1,5 @@
 # A very simple nginx configuration file that forces nginx to start.
-pid /run/nginx.pid;
+pid /tmp/nginx.pid;
 
 events {}
 http {}

rootfs/etc/nginx/template/nginx.tmpl

@@ -7,6 +7,9 @@
 {{ $proxyHeaders := .ProxySetHeaders }}
 {{ $addHeaders := .AddHeaders }}
 
+# setup custom paths that do not require root access
+pid /tmp/nginx.pid;
+
 {{ if $cfg.EnableModsecurity }}
 load_module /etc/nginx/modules/ngx_http_modsecurity_module.so;
 {{ end }}
@@ -20,7 +23,6 @@ worker_processes {{ $cfg.WorkerProcesses }};
 worker_cpu_affinity {{ $cfg.WorkerCpuAffinity }};
 {{ end }}
 
-pid /run/nginx.pid;
 {{ if ne .MaxOpenFiles 0 }}
 worker_rlimit_nofile {{ .MaxOpenFiles }};
 {{ end }}
@@ -115,6 +117,10 @@ http {
     keepalive_timeout  {{ $cfg.KeepAlive }}s;
     keepalive_requests {{ $cfg.KeepAliveRequests }};
 
+    client_body_temp_path           /tmp/client-body;
+    fastcgi_temp_path               /tmp/fastcgi-temp;
+    proxy_temp_path                 /tmp/proxy-temp;
+
     client_header_buffer_size       {{ $cfg.ClientHeaderBufferSize }};
     client_header_timeout           {{ $cfg.ClientHeaderTimeout }}s;
     large_client_header_buffers     {{ $cfg.LargeClientHeaderBuffers }};

test/manifests/ingress-controller/mandatory.yaml

@@ -251,6 +251,14 @@ spec:
             - --publish-service=$(POD_NAMESPACE)/ingress-nginx
             - --annotations-prefix=nginx.ingress.kubernetes.io
             - --watch-namespace=${NAMESPACE}
+          securityContext:
+            capabilities:
+                drop:
+                - ALL
+                add:
+                - NET_BIND_SERVICE
+            # www-data -> 33
+            runAsUser: 33
           env:
             - name: POD_NAME
               valueFrom:
@@ -284,5 +292,3 @@ spec:
             periodSeconds: 10
             successThreshold: 1
             timeoutSeconds: 1
-          securityContext:
-            privileged: true