Merge pull request #2134 from binwiederhier/adminsock-server

Admin socket: single-user lookup, safer socket file handling, Docker socket directory

Dockerfile

@@ -15,5 +15,9 @@ LABEL org.opencontainers.image.version="$VERSION"
 RUN apk add --no-cache tzdata
 COPY ntfy /usr/bin
 
+# Admin socket directory, writable by any uid (sticky, like /tmp), so the admin socket also works
+# when the container runs as a non-root user (-u UID:GID); the socket itself is created with mode 0660
+RUN mkdir -p /run/ntfy && chmod 1777 /run/ntfy
+
 EXPOSE 80/tcp
 ENTRYPOINT ["ntfy"]

Dockerfile-arm

@@ -17,5 +17,9 @@ LABEL org.opencontainers.image.version="$VERSION"
 
 COPY ntfy /usr/bin
 
+# Admin socket directory, writable by any uid (sticky, like /tmp), so the admin socket also works
+# when the container runs as a non-root user (-u UID:GID); the socket itself is created with mode 0660
+RUN mkdir -p /run/ntfy && chmod 1777 /run/ntfy
+
 EXPOSE 80/tcp
 ENTRYPOINT ["ntfy"]

Dockerfile-build

@@ -75,5 +75,9 @@ LABEL org.opencontainers.image.version="$VERSION"
 
 COPY --from=builder /app/dist/ntfy_linux_server/ntfy /usr/bin/ntfy
 
+# Admin socket directory, writable by any uid (sticky, like /tmp), so the admin socket also works
+# when the container runs as a non-root user (-u UID:GID); the socket itself is created with mode 0660
+RUN mkdir -p /run/ntfy && chmod 1777 /run/ntfy
+
 EXPOSE 80/tcp
 ENTRYPOINT ["ntfy"]

server/config.go

@@ -250,13 +250,17 @@ type Config struct {
 // DefaultListenAdminUnix returns the default admin socket path. On Windows there is none: unix socket
 // file permissions do not protect it there, so the CLI uses the database or --server instead.
 func DefaultListenAdminUnix() string {
-	switch runtime.GOOS {
+	return defaultListenAdminUnixFor(runtime.GOOS)
+}
+
+func defaultListenAdminUnixFor(goos string) string {
+	switch goos {
 	case "windows":
 		return ""
-	case "darwin":
-		return "/var/run/ntfy/admin.sock"
+	case "linux":
+		return "/run/ntfy/admin.sock"
 	}
-	return "/run/ntfy/admin.sock"
+	return "/var/run/ntfy/admin.sock" // macOS and the BSDs have no /run
 }
 
 // NewConfig instantiates a default new server config

server/server_admin.go

@@ -58,11 +58,16 @@ func (s *Server) handleVersion(w http.ResponseWriter, r *http.Request, v *visito
 }
 
 func (s *Server) handleUsersGet(w http.ResponseWriter, r *http.Request, v *visitor) error {
-	users, err := s.userManager.Users()
-	if err != nil {
-		return err
+	var (
+		users  []*user.User
+		grants map[string][]user.Grant
+		err    error
+	)
+	if username := r.URL.Query().Get("username"); username != "" {
+		users, grants, err = s.readUserAndGrants(username)
+	} else {
+		users, grants, err = s.readAllUsersAndGrants()
 	}
-	grants, err := s.userManager.AllGrants()
 	if err != nil {
 		return err
 	}
@@ -93,6 +98,32 @@ func (s *Server) handleUsersGet(w http.ResponseWriter, r *http.Request, v *visit
 	return s.writeJSON(w, usersResponse)
 }
 
+// readUserAndGrants returns one user, with its grants keyed by user ID, like readAllUsersAndGrants
+func (s *Server) readUserAndGrants(username string) ([]*user.User, map[string][]user.Grant, error) {
+	u, err := s.userManager.User(username)
+	if err != nil {
+		return nil, nil, mapUserErrorToHTTP(err)
+	}
+	grants, err := s.userManager.Grants(username)
+	if err != nil {
+		return nil, nil, err
+	}
+	return []*user.User{u}, map[string][]user.Grant{u.ID: grants}, nil
+}
+
+// readAllUsersAndGrants returns all users, with their grants keyed by user ID
+func (s *Server) readAllUsersAndGrants() ([]*user.User, map[string][]user.Grant, error) {
+	users, err := s.userManager.Users()
+	if err != nil {
+		return nil, nil, err
+	}
+	grants, err := s.userManager.AllGrants()
+	if err != nil {
+		return nil, nil, err
+	}
+	return users, grants, nil
+}
+
 func (s *Server) handleUsersAdd(w http.ResponseWriter, r *http.Request, v *visitor) error {
 	req, err := readJSONWithLimit[apiUserAddOrUpdateRequest](r.Body, jsonBodyBytesLimit, false)
 	if err != nil {
@@ -212,10 +243,10 @@ func (s *Server) handleUsersDelete(w http.ResponseWriter, r *http.Request, v *vi
 	u, err := s.userManager.User(req.Username)
 	if err != nil {
 		return mapUserErrorToHTTP(err)
-	} else if !u.IsUser() && !isAdminSocketRequest(r) {
-		return errHTTPUnauthorized.Wrap("can only remove regular users from API")
 	} else if u.Name == user.Everyone {
 		return errHTTPBadRequest.Wrap("cannot remove the everyone user")
+	} else if !u.IsUser() && !isAdminSocketRequest(r) {
+		return errHTTPForbiddenAdminSocketOnly
 	}
 	adminLog(v, r).Info("Removing user %s", req.Username)
 	if err := s.userManager.RemoveUser(req.Username); err != nil {
@@ -495,8 +526,9 @@ func (s *Server) handleAdminTiersDelete(w http.ResponseWriter, r *http.Request,
 }
 
 // listenAdminSocket creates the socket's directory, removes a stale socket file (one with no
-// server behind it) and listens. A socket that answers belongs to another running server; in
-// that case this one does not take it over.
+// server behind it) and listens. Anything at the path that is not a socket is left alone, in case
+// listen-admin-unix points somewhere it should not (e.g. at the database). A socket that answers
+// belongs to another running server; in that case this one does not take it over.
 func (s *Server) listenAdminSocket() (net.Listener, error) {
 	path := s.config.ListenAdminUnix
 	if err := os.MkdirAll(filepath.Dir(path), adminSocketDirMode); err != nil {
@@ -509,9 +541,15 @@ func (s *Server) listenAdminSocket() (net.Listener, error) {
 	if conn, err := net.DialTimeout("unix", path, adminSocketDialTimeout); err == nil {
 		conn.Close()
 		return nil, fmt.Errorf("another server is listening on %s", path)
+	} else if errors.Is(err, fs.ErrPermission) {
+		return nil, fmt.Errorf("%s belongs to a server of another user, set listen-admin-unix to a path of your own", path)
 	}
-	if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) {
-		return nil, err
+	if stat, err := os.Lstat(path); err == nil {
+		if stat.Mode().Type() != fs.ModeSocket {
+			return nil, fmt.Errorf("%s exists and is not a socket, refusing to replace it", path)
+		} else if err := os.Remove(path); err != nil {
+			return nil, err
+		}
 	}
 	listener, err := net.Listen("unix", path)
 	if err != nil {

server/server_admin_test.go

@@ -340,6 +340,7 @@ func TestUser_DontChangeAdminPassword(t *testing.T) {
 			"Authorization": util.BasicAuth("phil", "phil"),
 		})
 		require.Equal(t, 403, rr.Code)
+		require.Equal(t, 40302, toHTTPError(t, rr.Body.String()).Code)
 	})
 }
 
@@ -691,6 +692,30 @@ func TestAccess_RevokeBetweenAuthorizeAndSubscribe(t *testing.T) {
 	}
 }
 
+func TestUser_GetOne(t *testing.T) {
+	forEachBackend(t, func(t *testing.T, databaseURL string) {
+		s := newTestServer(t, newTestConfigWithAuthFile(t, databaseURL))
+		defer s.closeDatabases()
+		require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleAdmin, false))
+		require.Nil(t, s.userManager.AddUser("ben", "ben", user.RoleUser, false))
+		require.Nil(t, s.userManager.AllowAccess("ben", "bens-topic", user.PermissionRead))
+		auth := map[string]string{"Authorization": util.BasicAuth("phil", "phil")}
+
+		// Only the requested user, so the CLI does not download all users to look up one
+		rr := request(t, s, "GET", "/v1/users?username=ben", "", auth)
+		require.Equal(t, 200, rr.Code)
+		var users []*apiUserResponse
+		require.Nil(t, json.NewDecoder(rr.Body).Decode(&users))
+		require.Len(t, users, 1)
+		require.Equal(t, "ben", users[0].Username)
+		require.Equal(t, []*apiUserGrantResponse{{Topic: "bens-topic", Permission: "read-only"}}, users[0].Grants)
+
+		// Unknown user
+		rr = request(t, s, "GET", "/v1/users?username=nobody", "", auth)
+		require.Equal(t, 40031, toHTTPError(t, rr.Body.String()).Code)
+	})
+}
+
 func TestUser_GetWithDetails(t *testing.T) {
 	forEachBackend(t, func(t *testing.T, databaseURL string) {
 		conf := newTestConfigWithAuthFile(t, databaseURL)
@@ -893,7 +918,8 @@ func TestUser_DeleteAdmin(t *testing.T) {
 		rr := request(t, s, "DELETE", "/v1/users", `{"username": "admin"}`, map[string]string{
 			"Authorization": util.BasicAuth("phil", "phil"),
 		})
-		require.Equal(t, 401, rr.Code)
+		require.Equal(t, 403, rr.Code)
+		require.Equal(t, 40302, toHTTPError(t, rr.Body.String()).Code)
 
 		// Allowed over the socket
 		rr = requestAdminSocket(t, s, "DELETE", "/v1/users", `{"username": "admin"}`)
@@ -1187,6 +1213,7 @@ func TestPasswordReset_Failures(t *testing.T) {
 		require.Equal(t, 40905, toHTTPError(t, rr.Body.String()).Code)
 		rr = request(t, s, "POST", "/v1/users/password/reset", `{"username":"phil"}`, auth)
 		require.Equal(t, 403, rr.Code)
+		require.Equal(t, 40302, toHTTPError(t, rr.Body.String()).Code)
 
 		// No base URL
 		s.config.BaseURL = ""
@@ -1282,6 +1309,14 @@ func TestTiers_Failures(t *testing.T) {
 	})
 }
 
+func TestAdminSocket_DefaultPath(t *testing.T) {
+	require.Equal(t, "/run/ntfy/admin.sock", defaultListenAdminUnixFor("linux"))
+	require.Equal(t, "/var/run/ntfy/admin.sock", defaultListenAdminUnixFor("darwin"))
+	require.Equal(t, "/var/run/ntfy/admin.sock", defaultListenAdminUnixFor("freebsd"))
+	require.Equal(t, "/var/run/ntfy/admin.sock", defaultListenAdminUnixFor("openbsd"))
+	require.Equal(t, "", defaultListenAdminUnixFor("windows"))
+}
+
 func TestAdminSocket_ListenAndRemove(t *testing.T) {
 	forEachBackend(t, func(t *testing.T, databaseURL string) {
 		s, socket := newTestServerWithAdminSocket(t, newTestConfigWithAuthFile(t, databaseURL))
@@ -1345,7 +1380,10 @@ func TestAdminSocket_NonAdminRoutes(t *testing.T) {
 func TestAdminSocket_StaleSocketFile(t *testing.T) {
 	conf := newTestConfigWithAuthFile(t, "")
 	conf.ListenAdminUnix = filepath.Join(shortTempDir(t), "admin.sock")
-	require.Nil(t, os.WriteFile(conf.ListenAdminUnix, []byte("stale"), 0600))
+	stale, err := net.Listen("unix", conf.ListenAdminUnix)
+	require.Nil(t, err)
+	stale.(*net.UnixListener).SetUnlinkOnClose(false)
+	stale.Close() // Socket file stays behind, nobody listening (e.g. after a crash)
 	s := newTestServer(t, conf)
 	runTestServer(t, s)
 	waitFor(t, func() bool { return adminSocketListening(s) })
@@ -1355,6 +1393,22 @@ func TestAdminSocket_StaleSocketFile(t *testing.T) {
 	resp.Body.Close()
 }
 
+func TestAdminSocket_RegularFileNotRemoved(t *testing.T) {
+	// A misconfigured path (e.g. pointing at the database) must never be deleted
+	conf := newTestConfigWithAuthFile(t, "")
+	conf.ListenAdminUnix = filepath.Join(shortTempDir(t), "user.db")
+	require.Nil(t, os.WriteFile(conf.ListenAdminUnix, []byte("precious"), 0600))
+	s := newTestServer(t, conf)
+	output := captureLogUntil(t, "not a socket", func() {
+		runTestServer(t, s)
+	})
+	require.Contains(t, output, "not a socket")
+	require.False(t, adminSocketListening(s))
+	data, err := os.ReadFile(conf.ListenAdminUnix)
+	require.Nil(t, err)
+	require.Equal(t, "precious", string(data))
+}
+
 func TestAdminSocket_AnotherServerListening(t *testing.T) {
 	conf := newTestConfigWithAuthFile(t, "")
 	conf.ListenAdminUnix = filepath.Join(shortTempDir(t), "admin.sock")
@@ -1373,6 +1427,24 @@ func TestAdminSocket_AnotherServerListening(t *testing.T) {
 	require.Nil(t, err)
 }
 
+func TestAdminSocket_OtherUsersSocket(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("root can open any socket")
+	}
+	conf := newTestConfigWithAuthFile(t, "")
+	conf.ListenAdminUnix = filepath.Join(shortTempDir(t), "admin.sock")
+	other, err := net.Listen("unix", conf.ListenAdminUnix)
+	require.Nil(t, err)
+	defer other.Close()
+	require.Nil(t, os.Chmod(conf.ListenAdminUnix, 0)) // As seen by a user who may not open it
+	s := newTestServer(t, conf)
+	output := captureLogUntil(t, "belongs to a server of another user", func() {
+		runTestServer(t, s)
+	})
+	require.Contains(t, output, "belongs to a server of another user")
+	require.False(t, adminSocketListening(s))
+}
+
 func TestAdminSocket_UnwritableDirectory(t *testing.T) {
 	if os.Getuid() == 0 {
 		t.Skip("root can write anywhere")