fix(daemon): require admin access for POSTs and file pull API (#406)

Most of this was introduced in PR #358, when we ported the AccessChecker
changes from snapd, but accidentally set all the WriteAccess fields to
UserAccess{} instead of AdminAccess{}. Previously there was a
r.Method=="GET" check in Command.canAccess that handled this case.

Additionally:

- We lock down the files "pull" API to require admin. Even though it's a
read (GET), this meant any user could potentially read sensitive files.
- We lock down the task-websocket endpoint to admin. This is a GET
endpoint, but these websockets are used by exec to send stdin/out/err
and commands to the exec'd process, so they should require admin too.

I've added some tests for these to ensure we don't accidentally change
them in future, without noticing. How valuable these tests are I'm not
sure, as they only cover a subset of the API endpoints, but it seems
better than nothing.

internals/daemon/api.go

@@ -35,7 +35,7 @@ var API = []*Command{{
 }, {
 	Path:        "/v1/warnings",
 	ReadAccess:  UserAccess{},
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	GET:         v1GetWarnings,
 	POST:        v1AckWarnings,
 }, {
@@ -45,7 +45,7 @@ var API = []*Command{{
 }, {
 	Path:        "/v1/changes/{id}",
 	ReadAccess:  UserAccess{},
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	GET:         v1GetChange,
 	POST:        v1PostChange,
 }, {
@@ -55,13 +55,13 @@ var API = []*Command{{
 }, {
 	Path:        "/v1/services",
 	ReadAccess:  UserAccess{},
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	GET:         v1GetServices,
 	POST:        v1PostServices,
 }, {
 	Path:        "/v1/services/{name}",
 	ReadAccess:  UserAccess{},
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	GET:         v1GetService,
 	POST:        v1PostService,
 }, {
@@ -70,12 +70,12 @@ var API = []*Command{{
 	GET:        v1GetPlan,
 }, {
 	Path:        "/v1/layers",
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	POST:        v1PostLayers,
 }, {
 	Path:        "/v1/files",
-	ReadAccess:  UserAccess{},
-	WriteAccess: UserAccess{},
+	ReadAccess:  AdminAccess{}, // some files are sensitive, so require admin
+	WriteAccess: AdminAccess{},
 	GET:         v1GetFiles,
 	POST:        v1PostFiles,
 }, {
@@ -84,15 +84,15 @@ var API = []*Command{{
 	GET:        v1GetLogs,
 }, {
 	Path:        "/v1/exec",
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	POST:        v1PostExec,
 }, {
 	Path:       "/v1/tasks/{task-id}/websocket/{websocket-id}",
-	ReadAccess: UserAccess{},
+	ReadAccess: AdminAccess{}, // used by exec, so require admin
 	GET:        v1GetTaskWebsocket,
 }, {
 	Path:        "/v1/signals",
-	WriteAccess: UserAccess{},
+	WriteAccess: AdminAccess{},
 	POST:        v1PostSignals,
 }, {
 	Path:       "/v1/checks",
@@ -101,7 +101,7 @@ var API = []*Command{{
 }, {
 	Path:        "/v1/notices",
 	ReadAccess:  UserAccess{},
-	WriteAccess: UserAccess{},
+	WriteAccess: UserAccess{}, // any user is allowed to add a notice with their own uid
 	GET:         v1GetNotices,
 	POST:        v1PostNotices,
 }, {

internals/daemon/daemon_test.go

@@ -22,8 +22,10 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"syscall"
 	"testing"
@@ -1248,6 +1250,127 @@ services:
 	c.Check(tasks[0].Kind(), Equals, "stop")
 }
 
+func (s *daemonSuite) TestWritesRequireAdminAccess(c *C) {
+	for _, cmd := range API {
+		if cmd.Path == "/v1/notices" {
+			// Any user is allowed to add a notice with their own uid.
+			continue
+		}
+		switch cmd.WriteAccess.(type) {
+		case OpenAccess, UserAccess:
+			c.Errorf("%s WriteAccess should be AdminAccess, not %T", cmd.Path, cmd.WriteAccess)
+		}
+	}
+
+	// File pull (read) may be sensitive, so requires admin access too.
+	cmd := apiCmd("/v1/files")
+	switch cmd.ReadAccess.(type) {
+	case OpenAccess, UserAccess:
+		c.Errorf("%s ReadAccess should be AdminAccess, not %T", cmd.Path, cmd.WriteAccess)
+	}
+
+	// Task websockets (GET) is used for exec, so requires admin access too.
+	cmd = apiCmd("/v1/tasks/{task-id}/websocket/{websocket-id}")
+	switch cmd.ReadAccess.(type) {
+	case OpenAccess, UserAccess:
+		c.Errorf("%s ReadAccess should be AdminAccess, not %T", cmd.Path, cmd.WriteAccess)
+	}
+}
+
+func (s *daemonSuite) TestAPIAccessLevels(c *C) {
+	_ = s.newDaemon(c)
+
+	tests := []struct {
+		method string
+		path   string
+		body   string
+		uid    int // -1 means no peer cred user
+		status int
+	}{
+		{"GET", "/v1/system-info", ``, -1, http.StatusOK},
+
+		{"GET", "/v1/health", ``, -1, http.StatusOK},
+
+		{"GET", "/v1/warnings", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/warnings", ``, 42, http.StatusOK},
+		{"GET", "/v1/warnings", ``, 0, http.StatusOK},
+		{"POST", "/v1/warnings", ``, -1, http.StatusUnauthorized},
+		{"POST", "/v1/warnings", ``, 42, http.StatusUnauthorized},
+		{"POST", "/v1/warnings", ``, 0, http.StatusBadRequest},
+
+		{"GET", "/v1/changes", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/changes", ``, 42, http.StatusOK},
+		{"GET", "/v1/changes", ``, 0, http.StatusOK},
+
+		{"GET", "/v1/services", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/services", ``, 42, http.StatusOK},
+		{"GET", "/v1/services", ``, 0, http.StatusOK},
+		{"POST", "/v1/services", ``, -1, http.StatusUnauthorized},
+		{"POST", "/v1/services", ``, 42, http.StatusUnauthorized},
+		{"POST", "/v1/services", ``, 0, http.StatusBadRequest},
+
+		{"POST", "/v1/layers", ``, -1, http.StatusUnauthorized},
+		{"POST", "/v1/layers", ``, 42, http.StatusUnauthorized},
+		{"POST", "/v1/layers", ``, 0, http.StatusBadRequest},
+
+		{"GET", "/v1/files?action=list&path=/", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/files?action=list&path=/", ``, 42, http.StatusUnauthorized}, // even reading files requires admin
+		{"GET", "/v1/files?action=list&path=/", ``, 0, http.StatusOK},
+		{"POST", "/v1/files", `{}`, -1, http.StatusUnauthorized},
+		{"POST", "/v1/files", `{}`, 42, http.StatusUnauthorized},
+		{"POST", "/v1/files", `{}`, 0, http.StatusBadRequest},
+
+		{"GET", "/v1/logs", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/logs", ``, 42, http.StatusOK},
+		{"GET", "/v1/logs", ``, 0, http.StatusOK},
+
+		{"POST", "/v1/exec", `{}`, -1, http.StatusUnauthorized},
+		{"POST", "/v1/exec", `{}`, 42, http.StatusUnauthorized},
+		{"POST", "/v1/exec", `{}`, 0, http.StatusBadRequest},
+
+		{"POST", "/v1/signals", `{}`, -1, http.StatusUnauthorized},
+		{"POST", "/v1/signals", `{}`, 42, http.StatusUnauthorized},
+		{"POST", "/v1/signals", `{}`, 0, http.StatusBadRequest},
+
+		{"GET", "/v1/checks", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/checks", ``, 42, http.StatusOK},
+		{"GET", "/v1/checks", ``, 0, http.StatusOK},
+
+		{"GET", "/v1/notices", ``, -1, http.StatusUnauthorized},
+		{"GET", "/v1/notices", ``, 42, http.StatusOK},
+		{"GET", "/v1/notices", ``, 0, http.StatusOK},
+		{"POST", "/v1/notices", `{}`, -1, http.StatusUnauthorized},
+		{"POST", "/v1/notices", `{}`, 42, http.StatusBadRequest},
+		{"POST", "/v1/notices", `{}`, 0, http.StatusBadRequest},
+	}
+
+	for _, test := range tests {
+		remoteAddr := ""
+		if test.uid >= 0 {
+			remoteAddr = fmt.Sprintf("pid=100;uid=%d;socket=;", test.uid)
+		}
+		requestURL, err := url.Parse("http://localhost" + test.path)
+		c.Assert(err, IsNil)
+		request := &http.Request{
+			Method:     test.method,
+			URL:        requestURL,
+			Body:       io.NopCloser(strings.NewReader(test.body)),
+			RemoteAddr: remoteAddr,
+		}
+		recorder := httptest.NewRecorder()
+		cmd := apiCmd(requestURL.Path)
+		cmd.ServeHTTP(recorder, request)
+
+		response := recorder.Result()
+		if response.StatusCode != test.status {
+			// Log response body to make it easier to debug if the test fails.
+			c.Logf("%s %s uid=%d: expected %d, got %d; response body:\n%s",
+				test.method, test.path, test.uid, test.status, response.StatusCode, recorder.Body.String())
+		}
+		c.Assert(response.StatusCode, Equals, test.status)
+	}
+}
+
 type rebootSuite struct{}
 
 var _ = Suite(&rebootSuite{})