From 79199dd84c8149abee909cebfa8393deffa5fad5 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 10 Jun 2018 22:30:37 -0400 Subject: [PATCH 1/2] Run as user dropping privileges --- internal/file/filesystem.go | 9 +-------- internal/ingress/controller/checker.go | 8 +++++--- internal/ingress/controller/checker_test.go | 4 ++-- rootfs/Dockerfile | 11 ++++++++++- rootfs/etc/nginx/nginx.conf | 2 +- rootfs/etc/nginx/template/nginx.tmpl | 8 +++++++- test/manifests/ingress-controller/mandatory.yaml | 10 ++++++++-- 7 files changed, 34 insertions(+), 18 deletions(-) diff --git a/internal/file/filesystem.go b/internal/file/filesystem.go index 53d2036a3..3ad2f531b 100644 --- a/internal/file/filesystem.go +++ b/internal/file/filesystem.go @@ -35,7 +35,7 @@ func NewLocalFS() (Filesystem, error) { fs := filesystem.DefaultFs{} for _, directory := range directories { - err := fs.MkdirAll(directory, 0655) + err := fs.MkdirAll(directory, 0777) if err != nil { return nil, err } @@ -97,12 +97,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 } diff --git a/internal/ingress/controller/checker.go b/internal/ingress/controller/checker.go index 0d6eaca7c..92096eee6 100644 --- a/internal/ingress/controller/checker.go +++ b/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) diff --git a/internal/ingress/controller/checker_test.go b/internal/ingress/controller/checker_test.go index cb3aca44e..050eb1e46 100644 --- a/internal/ingress/controller/checker_test.go +++ b/internal/ingress/controller/checker_test.go @@ -60,8 +60,8 @@ func TestNginxCheck(t *testing.T) { }) // create pid file - fs.MkdirAll("/run", 0655) - pidFile, err := fs.Create("/run/nginx.pid") + fs.MkdirAll("/tmp", 0655) + pidFile, err := fs.Create(nginxPID) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/rootfs/Dockerfile b/rootfs/Dockerfile index 4f438d556..68254efd1 100644 --- a/rootfs/Dockerfile +++ b/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"] diff --git a/rootfs/etc/nginx/nginx.conf b/rootfs/etc/nginx/nginx.conf index bb36624ce..6f8e86b90 100644 --- a/rootfs/etc/nginx/nginx.conf +++ b/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 {} diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index d2a38e147..7a6e729f6 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/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 }}; diff --git a/test/manifests/ingress-controller/mandatory.yaml b/test/manifests/ingress-controller/mandatory.yaml index fe17a87db..b349f9f67 100644 --- a/test/manifests/ingress-controller/mandatory.yaml +++ b/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 From 7ded31d7a834c06c1f1b55dfc561271510b48c43 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Tue, 12 Jun 2018 08:40:40 -0400 Subject: [PATCH 2/2] Create file permission constants --- internal/file/filesystem.go | 8 +++++++- internal/ingress/annotations/auth/main.go | 16 +--------------- internal/ingress/controller/checker_test.go | 3 ++- internal/ingress/controller/nginx.go | 6 +++--- internal/ingress/controller/store/store.go | 2 +- internal/net/dns/dns_test.go | 14 ++++++++------ internal/watch/file_watcher_test.go | 12 +++++++----- 7 files changed, 29 insertions(+), 32 deletions(-) diff --git a/internal/file/filesystem.go b/internal/file/filesystem.go index 3ad2f531b..4a66ed263 100644 --- a/internal/file/filesystem.go +++ b/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, 0777) + err := fs.MkdirAll(directory, ReadWriteByUser) if err != nil { return nil, err } diff --git a/internal/ingress/annotations/auth/main.go b/internal/ingress/annotations/auth/main.go index 5b465f9df..3a6a3fae4 100644 --- a/internal/ingress/annotations/auth/main.go +++ b/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, 0777) + err := ioutil.WriteFile(filename, val, file.ReadWriteByUser) if err != nil { return ing_errors.LocationDenied{ Reason: errors.Wrap(err, "unexpected error creating password file"), diff --git a/internal/ingress/controller/checker_test.go b/internal/ingress/controller/checker_test.go index 050eb1e46..4e2385cf4 100644 --- a/internal/ingress/controller/checker_test.go +++ b/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,7 +61,7 @@ func TestNginxCheck(t *testing.T) { }) // create pid file - fs.MkdirAll("/tmp", 0655) + fs.MkdirAll("/tmp", file.ReadWriteByUser) pidFile, err := fs.Create(nginxPID) if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index d300629f8..87df06979 100644 --- a/internal/ingress/controller/nginx.go +++ b/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 } diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index ac4cceab2..ea697179b 100644 --- a/internal/ingress/controller/store/store.go +++ b/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) } } diff --git a/internal/net/dns/dns_test.go b/internal/net/dns/dns_test.go index 979d65c32..bd2243ae7 100644 --- a/internal/net/dns/dns_test.go +++ b/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 os.Remove(file.Name()) + defer f.Close() + 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) diff --git a/internal/watch/file_watcher_test.go b/internal/watch/file_watcher_test.go index 5733cd07c..83a37ea0b 100644 --- a/internal/watch/file_watcher_test.go +++ b/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 os.Remove(file.Name()) + defer f.Close() + 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: