Take the group's access away when the config does, on every platform

- unix: with no group configured the socket is chowned back to the
  daemon's own group, through the same held inode as the mode, so a
  reload that drops the key while keeping a group-readable mode no
  longer leaves the old group admitted until a restart
- windows: the group's ACE grants FILE_GENERIC_READ|FILE_GENERIC_WRITE
  without FILE_CREATE_PIPE_INSTANCE, which GENERIC_WRITE carries on a
  pipe and which would let a member create further instances under the
  name and answer other clients; dockerd grants it, this does not

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

localapi/access_unix_test.go

@@ -200,3 +200,34 @@ func TestSocketIsHandedToTheGroup(t *testing.T) {
 	assert.Equal(t, os.FileMode(0664), fi.Mode().Perm())
 	assert.Equal(t, path, s.Path())
 }
+
+// TestRemovingTheGroupHandsTheSocketBack: a reload that drops the group key leaves the
+// mode the operator set but returns the socket to the daemon's own group, so the old
+// group's members are no longer admitted. The socket is first handed to a group this
+// process belongs to other than its own, which not every host offers.
+func TestRemovingTheGroupHandsTheSocketBack(t *testing.T) {
+	own := os.Getegid()
+	groups, err := os.Getgroups()
+	require.NoError(t, err)
+	other := -1
+	for _, g := range groups {
+		if g != own {
+			other = g
+			break
+		}
+	}
+	if other < 0 {
+		t.Skip("this process belongs to no group but its own")
+	}
+	gid := strconv.Itoa(other)
+
+	s, path := serveInfo(t, infoRegistry(), 0600)
+	require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0660, Group: &Group{Name: gid, ID: gid}}))
+	assert.Equal(t, uint32(other), socketGroup(t, path))
+
+	require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0660}))
+	assert.Equal(t, uint32(own), socketGroup(t, path), "the daemon's own group again")
+	fi, err := os.Lstat(path)
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0660), fi.Mode().Perm(), "the mode is the operator's")
+}

localapi/access_windows_test.go

@@ -37,5 +37,5 @@ func TestSDDLAdmitsTheGroup(t *testing.T) {
 
 	sddl, err = sddlFor(Access{Mode: groupSocketMode, Group: &Group{Name: "Users", ID: "S-1-5-32-545"}})
 	require.NoError(t, err)
-	assert.Contains(t, sddl, "(A;;GA;;;SY)(A;;GA;;;BA)(A;;GRGW;;;S-1-5-32-545)")
+	assert.Contains(t, sddl, "(A;;GA;;;SY)(A;;GA;;;BA)(A;;0x12019b;;;S-1-5-32-545)")
 }

localapi/chmod_bsd.go

@@ -27,14 +27,14 @@ func applyAccess(path string, acc Access) error {
 		return err
 	}
 
-	if acc.Group != nil {
-		gid, err := gidOf(acc.Group)
-		if err != nil {
-			return err
-		}
-		if err := unix.Fchownat(unix.AT_FDCWD, path, -1, gid, unix.AT_SYMLINK_NOFOLLOW); err != nil {
-			return &os.PathError{Op: "chown", Path: path, Err: err}
-		}
+	// With no group configured the socket is handed back to the daemon's own group, so a
+	// reload that drops the key takes the access away; see the linux side.
+	gid, err := gidFor(acc)
+	if err != nil {
+		return err
+	}
+	if err := unix.Fchownat(unix.AT_FDCWD, path, -1, gid, unix.AT_SYMLINK_NOFOLLOW); err != nil {
+		return &os.PathError{Op: "chown", Path: path, Err: err}
 	}
 
 	if err := unix.Fchmodat(unix.AT_FDCWD, path, uint32(acc.Mode.Perm()), unix.AT_SYMLINK_NOFOLLOW); err != nil {

localapi/chmod_linux.go

@@ -55,19 +55,19 @@ func applyAccess(path string, acc Access) error {
 	held := "/proc/self/fd/" + strconv.Itoa(fd)
 
 	// The group first: a chown by root clears the setgid bit, which a socket never has,
-	// but the mode is pinned afterwards either way.
-	if acc.Group != nil {
-		gid, err := gidOf(acc.Group)
-		if err != nil {
-			return err
-		}
-		err = unix.Chown(held, -1, gid)
-		if err == unix.ENOENT {
-			err = unix.Fchownat(unix.AT_FDCWD, path, -1, gid, unix.AT_SYMLINK_NOFOLLOW)
-		}
-		if err != nil {
-			return &os.PathError{Op: "chown", Path: path, Err: err}
-		}
+	// but the mode is pinned afterwards either way. With no group configured the socket
+	// is handed back to the daemon's own, so taking the key out of the config on a reload
+	// takes the access away as surely as a restart would.
+	gid, err := gidFor(acc)
+	if err != nil {
+		return err
+	}
+	err = unix.Chown(held, -1, gid)
+	if err == unix.ENOENT {
+		err = unix.Fchownat(unix.AT_FDCWD, path, -1, gid, unix.AT_SYMLINK_NOFOLLOW)
+	}
+	if err != nil {
+		return &os.PathError{Op: "chown", Path: path, Err: err}
 	}
 
 	perm := uint32(acc.Mode.Perm())

localapi/group_unix.go

@@ -4,6 +4,7 @@ package localapi
 
 import (
 	"fmt"
+	"os"
 	"os/user"
 	"strconv"
 )
@@ -24,11 +25,15 @@ func lookupGroupID(name string) (string, error) {
 	return "", fmt.Errorf("group %q does not exist on this host", name)
 }
 
-// gidOf returns the group's gid as the number chown takes.
-func gidOf(g *Group) (int, error) {
-	gid, err := strconv.ParseUint(g.ID, 10, 32)
+// gidFor returns the gid the socket is owned by under acc: the configured group's, or
+// the daemon's own effective group when none is configured.
+func gidFor(acc Access) (int, error) {
+	if acc.Group == nil {
+		return os.Getegid(), nil
+	}
+	gid, err := strconv.ParseUint(acc.Group.ID, 10, 32)
 	if err != nil {
-		return 0, fmt.Errorf("group %q resolved to %q, which is not a gid", g.Name, g.ID)
+		return 0, fmt.Errorf("group %q resolved to %q, which is not a gid", acc.Group.Name, acc.Group.ID)
 	}
 	return int(gid), nil
 }

localapi/platform_windows.go

@@ -118,10 +118,14 @@ func isAdministrator() bool {
 }
 
 // sddlFor grants full access to the account nebula is running as, to SYSTEM and to
-// Administrators, and read and write, which is what a client's connect takes, to the
-// group acc names, if any; nobody else. P makes the DACL protected, so it does not
-// inherit entries that would widen it. It is dockerd's descriptor for its pipe, with
-// its -G group in the same place.
+// Administrators, and to the group acc names, if any, what a client's connect takes
+// and no more; nobody else. P makes the DACL protected, so it does not inherit entries
+// that would widen it. It is dockerd's descriptor for its pipe, with its -G group in the
+// same place, except for the group's mask: dockerd grants GENERIC_READ|GENERIC_WRITE,
+// and on a pipe GENERIC_WRITE carries FILE_CREATE_PIPE_INSTANCE, which would let a
+// member create further instances under the name and answer other clients itself. The
+// mask here is FILE_GENERIC_READ|FILE_GENERIC_WRITE with that right left out, which
+// opens the pipe and reads and writes it and nothing else.
 //
 // Administrators is the counterpart of the uid 0 allowance in the unix peer check. The
 // service normally runs as LocalSystem, so without it an operator in an elevated shell
@@ -137,11 +141,16 @@ func sddlFor(acc Access) (string, error) {
 
 	sddl := fmt.Sprintf("D:P(A;;GA;;;%s)(A;;GA;;;SY)(A;;GA;;;BA)", user.User.Sid.String())
 	if acc.Group != nil {
-		sddl += fmt.Sprintf("(A;;GRGW;;;%s)", acc.Group.ID)
+		sddl += fmt.Sprintf("(A;;%s;;;%s)", groupPipeMask, acc.Group.ID)
 	}
 	return sddl, nil
 }
 
+// groupPipeMask is the access a configured group gets on the pipe: FILE_GENERIC_READ
+// (0x120089) and FILE_GENERIC_WRITE (0x120116) without FILE_CREATE_PIPE_INSTANCE (0x4),
+// written as SDDL writes a mask it has no letters for.
+const groupPipeMask = "0x12019b"
+
 // removeSocket is a no-op: a named pipe disappears when its last handle closes, so
 // there is nothing to unlink and no stale instance to clean up.
 func removeSocket(path string, id socketID) {}