Let a group share the control socket, the dockerd way

Who may reach the control socket was the user nebula runs as and nobody
else, and on windows nothing but an administrator. It now takes a
`group`: the socket is handed to the group and the kernel's check on
connect is the access control, as `dockerd -G` works. Membership is the
admin's business.

- `local_api.control.group`: a name, a gid on unix or a SID on windows;
  resolved when the config is read, so a group that does not exist is a
  config error naming the key, and a reload with one keeps the socket
  that is running
- unix: the socket is 0660 and owned by the group; a reload that changes
  only the access sets it on the socket in place
- windows: the group is an ACE on the pipe, (A;;GRGW;;;<sid>) after the
  owner, SYSTEM and Administrators, dockerd's descriptor; a changed
  group means a new pipe under the same name
- the peer check stands aside while a group shares the socket: on unix
  it sees the peer's uid and primary gid only, so it could not tell a
  member from anyone else, and the kernel already decided
- Access is one value of mode and group that the socket is bound with;
  the info API that follows takes the same key

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

examples/config.yml

@@ -247,6 +247,9 @@ punchy:
     # The absolute path where the control socket should live, or a full pipe name on
     # Windows. The last directory is created if missing.
     #socket: /var/run/nebula/ctl.sock
+    # A group (or account/group/SID on Windows) that should be allowed to access the
+    # socket.
+    #group: nebula
 
 # EXPERIMENTAL: relay support for networks that can't establish direct connections.
 relay:

localapi/access.go

@@ -0,0 +1,75 @@
+package localapi
+
+import (
+	"errors"
+	"fmt"
+	"os"
+)
+
+// Access is who may reach a socket beyond the account nebula runs as: the node's mode,
+// and the group that shares the socket when the config names one.
+//
+// It is the dockerd model (-G): the operator names a group, nebula hands the socket to
+// it, and who is in the group is managed outside nebula. On unix the mode is the
+// socket node's and the group becomes its owning group; on windows the group is an
+// entry in the pipe's security descriptor and the mode means nothing to a pipe.
+type Access struct {
+	Mode  os.FileMode
+	Group *Group
+}
+
+// Group is a group named in the config, resolved to what the platform checks.
+type Group struct {
+	// Name is what the config said.
+	Name string
+
+	// ID is the platform's identifier for it: a gid on unix, a SID on windows.
+	ID string
+}
+
+// groupSocketMode is the mode a socket takes when a group shares it and the config
+// names no other: the account nebula runs as and the group, and nobody else.
+const groupSocketMode os.FileMode = 0660
+
+// errReopenForAccess is returned by applyAccess on a platform whose access control is
+// fixed when the endpoint is created, so a change means creating it again.
+var errReopenForAccess = errors.New("access is fixed at listen time")
+
+// ownerOnly is the control socket's access when no group is configured: the account
+// nebula runs as, and nobody else.
+func ownerOnly() Access { return Access{Mode: ctlSocketMode} }
+
+// String renders the access for a log line.
+func (a Access) String() string {
+	if a.Group == nil {
+		return modeString(a.Mode)
+	}
+	return fmt.Sprintf("%s group %s (%s)", modeString(a.Mode), a.Group.Name, a.Group.ID)
+}
+
+func (a Access) equal(b Access) bool {
+	if a.Mode != b.Mode || (a.Group == nil) != (b.Group == nil) {
+		return false
+	}
+	return a.Group == nil || a.Group.ID == b.Group.ID
+}
+
+// groupFromConfig reads the group a key names, or nil when it names none. The name is
+// resolved here, when the config is read, so a group that does not exist is a config
+// error reported with the key rather than a bind that fails later.
+func groupFromConfig(c interface{ GetString(string, string) string }, key string) (*Group, error) {
+	name := c.GetString(key, "")
+	if name == "" {
+		return nil, nil
+	}
+
+	id, err := lookupGroupID(name)
+	if err != nil {
+		return nil, fmt.Errorf("%s: %w", key, err)
+	}
+	return &Group{Name: name, ID: id}, nil
+}
+
+func modeString(m os.FileMode) string {
+	return fmt.Sprintf("%04o", m)
+}

localapi/access_unix_test.go

@@ -0,0 +1,141 @@
+//go:build !windows
+
+package localapi
+
+import (
+	"context"
+	"os"
+	"os/user"
+	"strconv"
+	"syscall"
+	"testing"
+
+	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/test"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// ownGroup returns a group this process may hand a socket to: its own effective group,
+// by name and by gid, which is the one chown accepts from an unprivileged user.
+func ownGroup(t *testing.T) (name, gid string) {
+	t.Helper()
+	gid = strconv.Itoa(os.Getegid())
+	g, err := user.LookupGroupId(gid)
+	require.NoError(t, err)
+	return g.Name, gid
+}
+
+func socketGroup(t *testing.T, path string) uint32 {
+	t.Helper()
+	fi, err := os.Lstat(path)
+	require.NoError(t, err)
+	st, ok := fi.Sys().(*syscall.Stat_t)
+	require.True(t, ok)
+	return uint32(st.Gid)
+}
+
+// TestLookupGroupID: a group is named as the host names it or by its gid; a gid the
+// host does not list is accepted as chown would accept it, and a name it does not list
+// is a mistake to report.
+func TestLookupGroupID(t *testing.T) {
+	name, gid := ownGroup(t)
+
+	got, err := lookupGroupID(name)
+	require.NoError(t, err)
+	assert.Equal(t, gid, got)
+
+	got, err = lookupGroupID(gid)
+	require.NoError(t, err)
+	assert.Equal(t, gid, got)
+
+	got, err = lookupGroupID("4000000000")
+	require.NoError(t, err)
+	assert.Equal(t, "4000000000", got)
+
+	_, err = lookupGroupID("no-such-group-nebula-test")
+	require.ErrorContains(t, err, "does not exist")
+}
+
+// TestControlAccessFromConfig: no group is the owner-only socket; a group is 0660 with
+// that group; a group the host does not know is a config error naming the key.
+func TestControlAccessFromConfig(t *testing.T) {
+	name, gid := ownGroup(t)
+	load := func(body string) (Access, error) {
+		c := config.NewC(test.NewLogger())
+		require.NoError(t, c.LoadString(body))
+		return controlAccessFrom(c)
+	}
+
+	acc, err := load("local_api: {}\n")
+	require.NoError(t, err)
+	assert.Equal(t, ownerOnly(), acc)
+	assert.Nil(t, acc.Group)
+
+	acc, err = load("local_api:\n  control:\n    group: " + name + "\n")
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0660), acc.Mode)
+	require.NotNil(t, acc.Group)
+	assert.Equal(t, name, acc.Group.Name)
+	assert.Equal(t, gid, acc.Group.ID)
+
+	_, err = load("local_api:\n  control:\n    group: no-such-group-nebula-test\n")
+	require.ErrorContains(t, err, CtlGroupKey)
+	assert.ErrorContains(t, err, "does not exist")
+}
+
+// TestControlSocketIsHandedToTheGroup: the socket is created with the group's gid and
+// 0660, and a reload that takes the group away tightens the socket in place, without
+// moving it.
+func TestControlSocketIsHandedToTheGroup(t *testing.T) {
+	name, gid := ownGroup(t)
+	want, err := strconv.ParseUint(gid, 10, 32)
+	require.NoError(t, err)
+
+	s := NewServer(test.NewLogger(), basicRegistry())
+	path := testSocket(t)
+	require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0660, Group: &Group{Name: name, ID: gid}}))
+	defer s.Stop()
+
+	fi, err := os.Lstat(path)
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0660), fi.Mode().Perm())
+	assert.Equal(t, uint32(want), socketGroup(t, path))
+
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
+	fi, err = os.Lstat(path)
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0600), fi.Mode().Perm())
+	assert.Equal(t, path, s.Path())
+
+	_, err = (&Client{Path: path}).Describe()
+	require.NoError(t, err, "the same listener is still serving")
+}
+
+// TestControlSocketWithAGroupTrustsTheKernel: with a group sharing the socket, the
+// kernel's check on connect is the access control and the daemon's peer check stands
+// aside, so a connection the peer check alone would refuse is served. Without a group
+// the same connection is refused; see TestPeerCheckRefusesAnotherUser.
+func TestControlSocketWithAGroupTrustsTheKernel(t *testing.T) {
+	if !peerCheckSupported {
+		t.Skip("no peer credentials on this platform")
+	}
+	name, gid := ownGroup(t)
+
+	me := currentUID()
+	asUID(t, me+1)
+	s := NewServer(test.NewLogger(), basicRegistry())
+	asUID(t, me)
+
+	path := testSocket(t)
+	require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0660, Group: &Group{Name: name, ID: gid}}))
+	defer s.Stop()
+
+	_, err := (&Client{Path: path}).Describe()
+	require.NoError(t, err, "the socket's group admitted us; the peer check must not refuse")
+
+	// Taking the group away on a reload puts the peer check back.
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
+	_, err = (&Client{Path: path}).Describe()
+	require.ErrorContains(t, err, "not permitted")
+}

localapi/access_windows_test.go

@@ -0,0 +1,44 @@
+//go:build windows
+
+package localapi
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
+)
+
+// TestLookupGroupIDOnWindows: an account name resolves to its SID, a SID written out
+// is accepted as it is, and an account the host does not know is reported.
+func TestLookupGroupIDOnWindows(t *testing.T) {
+	users, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid)
+	require.NoError(t, err)
+
+	got, err := lookupGroupID("Users")
+	require.NoError(t, err)
+	assert.Equal(t, users.String(), got)
+
+	got, err = lookupGroupID(users.String())
+	require.NoError(t, err)
+	assert.Equal(t, users.String(), got)
+
+	_, err = lookupGroupID("no-such-account-nebula-test")
+	require.ErrorContains(t, err, "does not exist")
+}
+
+// TestSDDLAdmitsTheGroup: the descriptor is the owner, SYSTEM and Administrators with
+// full access, plus the group with what a client needs and nothing generic, so a
+// member cannot create an instance of the pipe; see pipeClientAccess.
+func TestSDDLAdmitsTheGroup(t *testing.T) {
+	sddl, err := sddlFor(ownerOnly())
+	require.NoError(t, err)
+	assert.NotContains(t, sddl, "S-1-5-32-545")
+
+	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;;0x120083;;;S-1-5-32-545)")
+	assert.Zero(t, pipeClientAccess&windows.FILE_APPEND_DATA, "FILE_APPEND_DATA is FILE_CREATE_PIPE_INSTANCE on a pipe")
+	assert.Zero(t, pipeClientAccess&(windows.GENERIC_READ|windows.GENERIC_WRITE|windows.GENERIC_ALL))
+}

localapi/group_unix.go

@@ -0,0 +1,34 @@
+//go:build !windows
+
+package localapi
+
+import (
+	"fmt"
+	"os/user"
+	"strconv"
+)
+
+// lookupGroupID resolves a group's name, or a numeric gid, to the gid as a string. A
+// gid that the group database does not list is accepted as chown would accept it: the
+// operator may be naming a group the host will learn about later, or one that only a
+// directory service knows.
+func lookupGroupID(name string) (string, error) {
+	if g, err := user.LookupGroup(name); err == nil {
+		return g.Gid, nil
+	}
+
+	if _, err := strconv.ParseUint(name, 10, 32); err == nil {
+		return name, nil
+	}
+
+	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)
+	if err != nil {
+		return 0, fmt.Errorf("group %q resolved to %q, which is not a gid", g.Name, g.ID)
+	}
+	return int(gid), nil
+}

localapi/group_windows.go

@@ -0,0 +1,30 @@
+//go:build windows
+
+package localapi
+
+import (
+	"fmt"
+	"strings"
+
+	"golang.org/x/sys/windows"
+)
+
+// lookupGroupID resolves an account name, local or domain qualified, or a SID written
+// out, to the SID as a string, which is how the pipe's security descriptor names it.
+// Any account the host can resolve is accepted, as docker accepts any SID for its -G: a
+// group is the natural thing to name, but the descriptor does not care.
+func lookupGroupID(name string) (string, error) {
+	if strings.HasPrefix(strings.ToUpper(name), "S-1-") {
+		sid, err := windows.StringToSid(name)
+		if err != nil {
+			return "", fmt.Errorf("%q is not a SID: %w", name, err)
+		}
+		return sid.String(), nil
+	}
+
+	sid, _, _, err := windows.LookupSID("", name)
+	if err != nil {
+		return "", fmt.Errorf("account %q does not exist on this host: %w", name, err)
+	}
+	return sid.String(), nil
+}

localapi/path.go

@@ -10,6 +10,27 @@ const DirName = "nebula"
 // config file they agree on the socket with nothing more said.
 const CtlSocketKey = "local_api.control.socket"
 
+// CtlGroupKey is the optional config key naming a group that shares the control socket.
+// Unset, the socket admits the account nebula runs as and nobody else. Set, the socket
+// is handed to the group, 0660 on unix and an entry in the pipe's security descriptor on
+// windows, and the kernel's check on connect is the access control, as dockerd's -G
+// works; the daemon's own peer check stands aside, since on unix it could not tell a
+// member of the group from anyone else.
+const CtlGroupKey = "local_api.control.group"
+
+// controlAccessFrom returns the control socket's access a config implies: the group
+// CtlGroupKey names, if any, and the mode that goes with having one or not.
+func controlAccessFrom(c interface{ GetString(string, string) string }) (Access, error) {
+	group, err := groupFromConfig(c, CtlGroupKey)
+	if err != nil {
+		return Access{}, err
+	}
+	if group == nil {
+		return ownerOnly(), nil
+	}
+	return Access{Mode: groupSocketMode, Group: group}, nil
+}
+
 // DefaultSocketPath is the control socket when the config names none: one fixed path per
 // platform, see defaultSocketPath. Both the daemon and nebula ctl use it, which is what
 // lets the endpoint work with no configuration on either side.

localapi/platform_unix.go

@@ -44,18 +44,20 @@ func withPeerFD(conn net.Conn, f func(fd int) error) error {
 // in for root, or for a stranger, without owning a second account on the host.
 var currentUID = func() uint32 { return uint32(os.Geteuid()) }
 
-// listen binds the socket at path.
+// listen binds the socket at path with the given access.
 //
-// Access control is the socket's own mode, 0600, which the node is created with and
-// which is pinned after bind, together with the peer check the server applies on accept:
-// connect(2) needs write permission on the socket file on every unix we build for. The
+// Access control is the socket's own mode and group, which the node is created no
+// looser than and which are set after bind, together with the peer check the server
+// applies on accept unless a group shares the socket, in which case the kernel's check
+// is the whole of it: connect(2) needs write permission on the socket file on every
+// unix we build for. The
 // directory is not part of that, and is never chmoded: a directory a service manager
 // made for us is served from at whatever mode it came with (systemd's
 // RuntimeDirectoryMode= is 0755 by default), and one an operator pointed us into keeps
 // whatever else it holds readable. The one we make ourselves is 0755, searchable by
 // everyone, so that an unprivileged nebula ctl can see a root daemon's socket and say
 // so; see ServedByRootFor.
-func listen(path string) (net.Listener, error) {
+func listen(path string, acc Access) (net.Listener, error) {
 	// The kernel silently truncates an over long sun_path and bind then fails with
 	// "invalid argument", which tells an operator nothing. Say what is actually wrong.
 	if pathTooLong(path) {
@@ -69,7 +71,7 @@ func listen(path string) (net.Listener, error) {
 	// mistake to report, not a tree to build. A directory made here is remembered, so
 	// that removeSocket can take it away again once nothing is left in it.
 	//
-	// 0755 gives nothing away: the socket is 0600. What it gives is a directory others
+	// 0755 gives nothing away: the socket's own mode is the access control. What it gives is a directory others
 	// can look into, which is what ServedByRootFor needs. Mkdir applies the umask, so
 	// the mode is pinned afterwards, as the socket's is; only on a directory this call
 	// created.
@@ -107,9 +109,9 @@ func listen(path string) (net.Listener, error) {
 		ul.SetUnlinkOnClose(false)
 	}
 
-	// The umask only promises no looser than 0600; this pins the mode exactly, and is
-	// the one place a failure to secure the socket is reported.
-	if err := os.Chmod(path, 0600); err != nil {
+	// The umask only promises no looser than 0600; this pins the mode and the group
+	// exactly, and is the one place a failure to secure the socket is reported.
+	if err := applyAccess(path, acc); err != nil {
 		release(ln, path)
 		return nil, fmt.Errorf("could not secure %s: %w", path, err)
 	}
@@ -117,6 +119,21 @@ func listen(path string) (net.Listener, error) {
 	return ln, nil
 }
 
+// applyAccess sets the owning group, when there is one, and the mode of the socket node
+// at path. The chown is Lchown, so a link at the path is not followed to its target.
+func applyAccess(path string, acc Access) error {
+	if acc.Group != nil {
+		gid, err := gidOf(acc.Group)
+		if err != nil {
+			return err
+		}
+		if err := os.Lchown(path, -1, gid); err != nil {
+			return err
+		}
+	}
+	return os.Chmod(path, acc.Mode.Perm())
+}
+
 // owner returns the uid that owns the file fi describes.
 func owner(fi os.FileInfo) (uint32, bool) {
 	st, ok := fi.Sys().(*syscall.Stat_t)
@@ -259,11 +276,15 @@ func checkSocketPath(key, value string) error {
 	return nil
 }
 
-// logListening is the line written when the socket is up. Every unix this builds for
-// checks the peer's credentials on each connection, on top of the socket's mode, so the
-// line has only the path to add.
-func logListening(l *slog.Logger, path string) {
-	l.Info("Local control socket listening", "path", path)
+// logListening is the line written when the socket is up. It says what the access
+// control is: the socket's mode plus the peer check on each connection, or, when a group
+// shares the socket, the mode and group alone.
+func logListening(l *slog.Logger, path string, acc Access) {
+	if acc.Group != nil {
+		l.Info("Local control socket listening, access decided by the socket's group", "path", path, "access", acc.String())
+		return
+	}
+	l.Info("Local control socket listening", "path", path, "access", acc.String())
 }
 
 func isSocket(fi os.FileInfo) bool { return fi.Mode()&os.ModeSocket != 0 }

localapi/platform_unix_test.go

@@ -45,7 +45,7 @@ func TestListenLeavesAServiceManagersDirectoryAlone(t *testing.T) {
 	require.NoError(t, os.Mkdir(dir, 0755))
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(context.Background(), filepath.Join(dir, "s.sock")))
+	require.NoError(t, s.Rebind(context.Background(), filepath.Join(dir, "s.sock"), ownerOnly()))
 	t.Cleanup(s.Stop)
 
 	fi, err := os.Stat(dir)
@@ -66,11 +66,11 @@ func TestListenCreatesOneLevelOnly(t *testing.T) {
 	base := privateBase(t)
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(context.Background(), filepath.Join(base, DirName, "s.sock")))
+	require.NoError(t, s.Rebind(context.Background(), filepath.Join(base, DirName, "s.sock"), ownerOnly()))
 	s.Stop()
 
 	dir := filepath.Join(base, "missing", DirName)
-	err := s.Rebind(context.Background(), filepath.Join(dir, "s.sock"))
+	err := s.Rebind(context.Background(), filepath.Join(dir, "s.sock"), ownerOnly())
 	require.Error(t, err)
 	require.ErrorContains(t, err, "could not create "+dir)
 	assert.NoDirExists(t, filepath.Join(base, "missing"))
@@ -82,9 +82,9 @@ func TestReleaseRemovesTheDirectoryItMade(t *testing.T) {
 	base := privateBase(t)
 
 	made := filepath.Join(base, DirName)
-	first, err := listen(filepath.Join(made, "a.sock"))
+	first, err := listen(filepath.Join(made, "a.sock"), ownerOnly())
 	require.NoError(t, err)
-	second, err := listen(filepath.Join(made, "b.sock"))
+	second, err := listen(filepath.Join(made, "b.sock"), ownerOnly())
 	require.NoError(t, err)
 
 	release(first, filepath.Join(made, "a.sock"))
@@ -94,7 +94,7 @@ func TestReleaseRemovesTheDirectoryItMade(t *testing.T) {
 
 	given := filepath.Join(base, "given")
 	require.NoError(t, os.Mkdir(given, 0755))
-	ln, err := listen(filepath.Join(given, "s.sock"))
+	ln, err := listen(filepath.Join(given, "s.sock"), ownerOnly())
 	require.NoError(t, err)
 	release(ln, filepath.Join(given, "s.sock"))
 	assert.DirExists(t, given)
@@ -109,12 +109,12 @@ func TestListenRestoresTheUmask(t *testing.T) {
 	t.Cleanup(func() { syscall.Umask(old) })
 
 	base := privateBase(t)
-	ln, err := listen(filepath.Join(base, DirName, "s.sock"))
+	ln, err := listen(filepath.Join(base, DirName, "s.sock"), ownerOnly())
 	require.NoError(t, err)
 	t.Cleanup(func() { ln.Close() })
 	assert.Equal(t, 0022, syscall.Umask(0022), "umask after a successful listen")
 
-	_, err = listen(filepath.Join(base, DirName, "s.sock"))
+	_, err = listen(filepath.Join(base, DirName, "s.sock"), ownerOnly())
 	require.Error(t, err, "the path is already bound")
 	assert.Equal(t, 0022, syscall.Umask(0022), "umask after a failed listen")
 }
@@ -130,7 +130,7 @@ func TestListenServesFromASharedDirectory(t *testing.T) {
 	require.NoError(t, os.WriteFile(cfg, []byte("{}\n"), 0644))
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(context.Background(), filepath.Join(dir, "ctl.sock")))
+	require.NoError(t, s.Rebind(context.Background(), filepath.Join(dir, "ctl.sock"), ownerOnly()))
 	t.Cleanup(s.Stop)
 
 	fi, err := os.Stat(dir)
@@ -148,7 +148,7 @@ func TestOverlongPathIsDiagnosed(t *testing.T) {
 	}
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	err := s.Rebind(context.Background(), filepath.Join(long, "s.sock"))
+	err := s.Rebind(context.Background(), filepath.Join(long, "s.sock"), ownerOnly())
 
 	require.Error(t, err)
 	assert.ErrorContains(t, err, "byte limit for a unix socket")
@@ -170,7 +170,7 @@ func TestNamedPathIsUsedAsGiven(t *testing.T) {
 	path := filepath.Join(openDir(t, base), DirName, "s.sock")
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	t.Cleanup(s.Stop)
 
 	_, err := (&Client{Path: path}).Describe()
@@ -283,7 +283,7 @@ func TestPeerCheckRefusesAnotherUser(t *testing.T) {
 	asUID(t, me)
 
 	path := testSocket(t)
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	defer s.Stop()
 
 	_, err := (&Client{Path: path}).Describe()
@@ -299,7 +299,7 @@ func TestPeerCheckRefusesAnotherUser(t *testing.T) {
 // bound its own, and a second unlink would take that daemon's socket from under it.
 func TestListenerDoesNotUnlinkOnClose(t *testing.T) {
 	path := filepath.Join(privateBase(t), DirName, "s.sock")
-	old, err := listen(path)
+	old, err := listen(path, ownerOnly())
 	require.NoError(t, err)
 
 	// A newer daemon's start, with the old node gone and the path bound afresh.

localapi/platform_windows.go

@@ -3,6 +3,7 @@
 package localapi
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"log/slog"
@@ -37,15 +38,18 @@ var currentUID = func() uint32 { return 0 }
 // the access control, so nobody assumes a per-connection check that is not running:
 // there is none here, the pipe's security descriptor admits only the accounts listed in
 // ownerOnlySDDL, and the kernel enforces that at connect time.
-func logListening(l *slog.Logger, path string) {
+func logListening(l *slog.Logger, path string, acc Access) {
 	l.Info("Local control socket listening, access restricted by the pipe's security descriptor",
 		"path", path,
+		"access", acc.String(),
 		"reason", "the security descriptor built at listen time is the access check; the connecting process is not identified separately",
 	)
 }
 
-// listen creates the named pipe at path with a security descriptor that admits only the
-// user nebula runs as, SYSTEM and Administrators.
+// listen creates the named pipe at path with a security descriptor that admits the user
+// nebula runs as, SYSTEM and Administrators, and the group acc names, if any, for
+// reading and writing, which is what connecting to a pipe takes. acc's mode means
+// nothing to a pipe.
 //
 // The first instance is created, never opened, so a name some other process already
 // holds is refused rather than joined: that process would otherwise answer clients in
@@ -55,8 +59,8 @@ func logListening(l *slog.Logger, path string) {
 // daemon that is not an administrator cannot create under protectedPrefix at all, and
 // that is told apart so the operator is sent to elevate or to name a pipe elsewhere
 // rather than to look for a process that is not there.
-func listen(path string) (net.Listener, error) {
-	sddl, err := ownerOnlySDDL()
+func listen(path string, acc Access) (net.Listener, error) {
+	sddl, err := sddlFor(acc)
 	if err != nil {
 		return nil, fmt.Errorf("could not build a security descriptor for %s: %w", path, err)
 	}
@@ -114,15 +118,17 @@ func isAdministrator() bool {
 	return err == nil && member
 }
 
-// ownerOnlySDDL grants full access to the account nebula is running as, to SYSTEM and
-// to Administrators, and to nobody else. P makes the DACL protected, so it does not
-// inherit entries that would widen it.
+// sddlFor grants full access to the account nebula is running as, to SYSTEM and to
+// Administrators, and pipeClientAccess 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, except for
+// the group's mask; see pipeClientAccess.
 //
 // 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
 // could not reach the daemon at all; and an administrator can already stop the service,
 // so this grants no authority they did not have.
-func ownerOnlySDDL() (string, error) {
+func sddlFor(acc Access) (string, error) {
 	token := windows.GetCurrentProcessToken()
 
 	user, err := token.GetTokenUser()
@@ -130,10 +136,29 @@ func ownerOnlySDDL() (string, error) {
 		return "", err
 	}
 
-	sid := user.User.Sid.String()
-	return fmt.Sprintf("D:P(A;;GA;;;%s)(A;;GA;;;SY)(A;;GA;;;BA)", sid), nil
+	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;;0x%x;;;%s)", pipeClientAccess, acc.Group.ID)
+	}
+	return sddl, nil
 }
 
+// pipeClientAccess is what a client needs on the pipe to talk to the daemon, and all a
+// configured group is granted: read and write the data, read the pipe's attributes
+// and its security descriptor, which the owner check needs, and wait on it. Not the
+// generic rights. On a named pipe GENERIC_WRITE carries FILE_CREATE_PIPE_INSTANCE, the
+// right to put a second instance under the same name, and a group member with that
+// could sit on the name and answer other clients in the daemon's place; the client's
+// owner check would not catch it, since the instance it reached would be the
+// member's. A client therefore has to ask for exactly this, as dial does, and a
+// program that opens the info pipe with generic read and write is refused.
+const pipeClientAccess uint32 = windows.FILE_READ_DATA | windows.FILE_WRITE_DATA |
+	windows.FILE_READ_ATTRIBUTES | windows.READ_CONTROL | windows.SYNCHRONIZE
+
+// applyAccess cannot change a pipe's access in place: its security descriptor is fixed
+// when it is created, so a change means creating it again, which the caller does.
+func applyAccess(path string, acc Access) error { return errReopenForAccess }
+
 // 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) {}
@@ -145,7 +170,9 @@ func removeSocket(path string) {}
 // the check is what covers a CtlSocketKey override outside protectedPrefix, and costs
 // nothing to make on every name.
 func dial(path string, timeout time.Duration) (net.Conn, error) {
-	conn, err := winio.DialPipe(path, &timeout)
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+	conn, err := winio.DialPipeAccess(ctx, path, pipeClientAccess)
 	if err != nil {
 		return nil, err
 	}

localapi/platform_windows_test.go

@@ -120,7 +120,7 @@ func TestListenRefused(t *testing.T) {
 // promises a non-administrator.
 func TestListenProtectedName(t *testing.T) {
 	path := protectedPrefix + "nebula-test-" + t.Name()
-	ln, err := listen(path)
+	ln, err := listen(path, ownerOnly())
 	if !isAdministrator() {
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "requires an administrator")
@@ -149,7 +149,7 @@ func TestListenProtectedName(t *testing.T) {
 // is this account, or Administrators when the test is elevated.
 func TestDialAcceptsOwnPipe(t *testing.T) {
 	path := pipePrefix + "nebula-test-" + t.Name()
-	ln, err := listen(path)
+	ln, err := listen(path, ownerOnly())
 	require.NoError(t, err)
 	t.Cleanup(func() { ln.Close() })
 
@@ -173,12 +173,12 @@ func TestDialAcceptsOwnPipe(t *testing.T) {
 // served alongside, or in place of, the daemon.
 func TestListenRefusesHeldName(t *testing.T) {
 	path := pipePrefix + "nebula-test-" + t.Name()
-	first, err := listen(path)
+	first, err := listen(path, ownerOnly())
 	require.NoError(t, err)
 	t.Cleanup(func() { first.Close() })
 
 	var second net.Listener
-	second, err = listen(path)
+	second, err = listen(path, ownerOnly())
 	if second != nil {
 		second.Close()
 	}

localapi/server.go

@@ -9,9 +9,11 @@ import (
 	"log/slog"
 	"math"
 	"net"
+	"os"
 	"runtime/debug"
 	"strconv"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/slackhq/nebula/commands"
@@ -36,6 +38,11 @@ const (
 	defaultSlowAfter = 30 * time.Second
 )
 
+// ctlSocketMode is the control socket's mode when no group shares it: the user who ran
+// nebula, and nobody else. The mode itself is not configurable; what an operator can
+// choose is a group, CtlGroupKey, which makes it groupSocketMode.
+const ctlSocketMode os.FileMode = 0600
+
 // Server serves the read only portion of a command registry over a local socket.
 type Server struct {
 	l   *slog.Logger
@@ -52,6 +59,16 @@ type Server struct {
 
 	sem  chan struct{}
 	euid uint32
+
+	// access is what the socket at path was bound with, so a reload that changes only
+	// the group or the mode is applied without moving the socket.
+	access Access
+
+	// trustSocket is set while a group shares the socket. The kernel admitted the peer
+	// through the socket's mode and group, which is the whole access control then, and
+	// the peer check in serve stands aside: on unix it sees the peer's uid and primary
+	// gid only, so it could not tell a member of the group from anyone else.
+	trustSocket atomic.Bool
 }
 
 // servedHere decides what this surface will run.
@@ -106,16 +123,41 @@ func NewServer(l *slog.Logger, reg *commands.Registry) *Server {
 //
 // An error here is never fatal to the caller: nebula keeps forwarding packets without a
 // control endpoint, and the caller is expected to log and carry on.
-func (s *Server) Rebind(ctx context.Context, path string) error {
+func (s *Server) Rebind(ctx context.Context, path string, acc Access) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
+	reopening := false
 	if s.running && s.path == path {
-		return nil
+		if s.access.equal(acc) {
+			return nil
+		}
+
+		// Only the access changed. On unix it is set on the socket in place; on windows
+		// the descriptor is fixed when the pipe is created, so the pipe is created again.
+		// The name cannot be held twice, so the old one goes first, and a bind that then
+		// fails leaves the server down rather than on the access the operator asked to
+		// change; that is reported.
+		err := applyAccess(path, acc)
+		if err == nil {
+			s.access = acc
+			s.trustSocket.Store(acc.Group != nil)
+			s.l.Info("Changed the local control socket's access", "path", path, "access", acc.String())
+			return nil
+		}
+		if !errors.Is(err, errReopenForAccess) {
+			return fmt.Errorf("could not change the access of %s: %w", path, err)
+		}
+		reopening = true
+		release(s.ln, s.path)
+		s.ln, s.path, s.running, s.access = nil, "", false, Access{}
 	}
 
-	ln, err := listen(path)
+	ln, err := listen(path, acc)
 	if err != nil {
+		if reopening {
+			return fmt.Errorf("the local control socket was closed to change its access and could not be created again: %w", err)
+		}
 		return err
 	}
 
@@ -131,7 +173,9 @@ func (s *Server) Rebind(ctx context.Context, path string) error {
 	oldLn, oldPath := s.ln, s.path
 	s.ln = ln
 	s.path = path
+	s.access = acc
 	s.running = true
+	s.trustSocket.Store(acc.Group != nil)
 	sem := s.semaphore()
 
 	s.watchLocked(ctx)
@@ -144,7 +188,7 @@ func (s *Server) Rebind(ctx context.Context, path string) error {
 		release(oldLn, oldPath)
 	}
 
-	logListening(s.l, path)
+	logListening(s.l, path, acc)
 
 	go func() {
 		s.accept(ln, sem)
@@ -197,6 +241,7 @@ func (s *Server) Stop() {
 
 	release(s.ln, s.path)
 	s.running = false
+	s.access = Access{}
 	s.ln = nil
 	s.path = ""
 
@@ -314,10 +359,10 @@ func (s *Server) handle(conn net.Conn) {
 		}
 	}()
 
-	// Where the platform can identify the caller, check it. Where it cannot, the
-	// endpoint is still protected by the socket's mode or the pipe's security
-	// descriptor, which listen() set before serving.
-	if peerCheckSupported {
+	// Where the platform can identify the caller, check it. Where it cannot, or where a
+	// group shares the socket, the endpoint is protected by the socket's mode and group
+	// or the pipe's security descriptor, which listen() set before serving.
+	if peerCheckSupported && !s.trustSocket.Load() {
 		uid, err := peerUID(conn)
 		if err != nil {
 			s.l.Warn("Refusing local control connection with unreadable credentials", "error", err)

localapi/server_test.go

@@ -39,7 +39,7 @@ func serve(t *testing.T, reg *commands.Registry) (*Server, string) {
 
 	path := testSocket(t)
 	s := NewServer(test.NewLogger(), reg)
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	t.Cleanup(s.Stop)
 
 	return s, path
@@ -50,7 +50,7 @@ func serveAt(t *testing.T, path string) (*Server, func()) {
 	t.Helper()
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	return s, s.Stop
 }
 
@@ -183,7 +183,7 @@ func TestStaleSocketIsReplaced(t *testing.T) {
 	require.NoError(t, err, "test setup should leave a stale socket behind")
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(context.Background(), path), "a stale socket must not block startup")
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()), "a stale socket must not block startup")
 	defer s.Stop()
 
 	out, _ := run(t, path, "echo", "ok")
@@ -195,7 +195,7 @@ func TestLiveSocketIsNotStolen(t *testing.T) {
 	require.NotNil(t, first)
 
 	second := NewServer(test.NewLogger(), basicRegistry())
-	err := second.Rebind(context.Background(), path)
+	err := second.Rebind(context.Background(), path, ownerOnly())
 	assert.ErrorContains(t, err, "already in use", "a live endpoint must not be unlinked by a second instance")
 }
 
@@ -209,7 +209,7 @@ func TestLiveSocketIsNotStolenBeforeItsChmod(t *testing.T) {
 	require.NoError(t, os.Chmod(path, 0666))
 
 	second := NewServer(test.NewLogger(), basicRegistry())
-	err := second.Rebind(context.Background(), path)
+	err := second.Rebind(context.Background(), path, ownerOnly())
 	require.ErrorContains(t, err, "already in use", "a live socket at any mode is not thereby dead")
 
 	_, err = os.Stat(path)
@@ -223,7 +223,7 @@ func TestRebindMovesTheSocket(t *testing.T) {
 	s, path := serve(t, basicRegistry())
 	next := filepath.Join(filepath.Dir(path), "n.sock")
 
-	require.NoError(t, s.Rebind(context.Background(), next))
+	require.NoError(t, s.Rebind(context.Background(), next, ownerOnly()))
 	assert.Equal(t, next, s.Path())
 
 	out, _ := run(t, next, "echo", "moved")
@@ -236,7 +236,7 @@ func TestRebindMovesTheSocket(t *testing.T) {
 func TestRebindFailureKeepsTheOldSocket(t *testing.T) {
 	s, path := serve(t, basicRegistry())
 
-	err := s.Rebind(context.Background(), "/nonexistent-dir-for-tests/x.sock")
+	err := s.Rebind(context.Background(), "/nonexistent-dir-for-tests/x.sock", ownerOnly())
 	require.Error(t, err)
 	assert.Equal(t, path, s.Path(), "a bad path must leave the working endpoint alone")
 
@@ -252,16 +252,16 @@ func TestSemaphoreSurvivesRestart(t *testing.T) {
 	s := NewServer(test.NewLogger(), basicRegistry())
 	path := testSocket(t)
 
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	first := s.sem
 	require.NotNil(t, first)
 	s.Stop()
 
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	defer s.Stop()
 	assert.Equal(t, first, s.sem, "Rebind after Stop must keep the same semaphore")
 
-	require.NoError(t, s.Rebind(context.Background(), testSocket(t)))
+	require.NoError(t, s.Rebind(context.Background(), testSocket(t), ownerOnly()))
 	assert.Equal(t, first, s.sem, "Rebind must keep the same semaphore")
 }
 
@@ -274,12 +274,12 @@ func TestRestartIsWatchedOnItsOwnContext(t *testing.T) {
 	path := testSocket(t)
 
 	ctxA, cancelA := context.WithCancel(context.Background())
-	require.NoError(t, s.Rebind(ctxA, path))
+	require.NoError(t, s.Rebind(ctxA, path, ownerOnly()))
 	cancelA()
 	require.Eventually(t, func() bool { return s.Path() == "" }, time.Second, 10*time.Millisecond, "the first context must stop the server")
 
 	ctxB, cancelB := context.WithCancel(context.Background())
-	require.NoError(t, s.Rebind(ctxB, path))
+	require.NoError(t, s.Rebind(ctxB, path, ownerOnly()))
 	defer s.Stop()
 	assert.Equal(t, path, s.Path())
 
@@ -298,12 +298,12 @@ func TestStopDetachesTheWatcher(t *testing.T) {
 
 	ctxA, cancelA := context.WithCancel(context.Background())
 	t.Cleanup(cancelA)
-	require.NoError(t, s.Rebind(ctxA, path))
+	require.NoError(t, s.Rebind(ctxA, path, ownerOnly()))
 	s.Stop()
 
 	ctxB, cancelB := context.WithCancel(context.Background())
 	t.Cleanup(cancelB)
-	require.NoError(t, s.Rebind(ctxB, path))
+	require.NoError(t, s.Rebind(ctxB, path, ownerOnly()))
 	t.Cleanup(s.Stop)
 
 	cancelA()
@@ -364,7 +364,7 @@ func TestConcurrentClientsAreBounded(t *testing.T) {
 	s := NewServer(test.NewLogger(), reg)
 	s.maxConns = 2
 	path := testSocket(t)
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	defer s.Stop()
 
 	var wg sync.WaitGroup
@@ -408,7 +408,7 @@ func TestSlowCommandIsNamedInTheLog(t *testing.T) {
 	s.maxConns = 1
 	s.slowAfter = 20 * time.Millisecond
 	path := testSocket(t)
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	defer s.Stop()
 
 	var wg sync.WaitGroup
@@ -450,7 +450,7 @@ func TestPanickedCommandIsNotReportedAsStillRunning(t *testing.T) {
 	s := NewServer(test.NewLoggerWithOutput(&logs), reg)
 	s.slowAfter = 20 * time.Millisecond
 	path := testSocket(t)
-	require.NoError(t, s.Rebind(context.Background(), path))
+	require.NoError(t, s.Rebind(context.Background(), path, ownerOnly()))
 	defer s.Stop()
 
 	res, err := (&Client{Path: path}).Invoke("boom", nil, nil, "")
@@ -502,7 +502,7 @@ func (f *flakyListener) Accept() (net.Conn, error) {
 func TestAcceptSurvivesTransientErrors(t *testing.T) {
 	path := testSocket(t)
 	require.NoError(t, os.Mkdir(filepath.Dir(path), 0700))
-	ln, err := listen(path)
+	ln, err := listen(path, ownerOnly())
 	require.NoError(t, err)
 
 	s := NewServer(test.NewLogger(), basicRegistry())
@@ -537,7 +537,7 @@ func TestContextCancellationStops(t *testing.T) {
 	path := testSocket(t)
 
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(ctx, path))
+	require.NoError(t, s.Rebind(ctx, path, ownerOnly()))
 
 	cancel()
 	assert.Eventually(t, func() bool {
@@ -797,10 +797,10 @@ func TestRebindAfterFailedBindStillStopsOnCancel(t *testing.T) {
 	// listen creates one level only, so a socket directory under a parent that is not
 	// there yet fails to bind, the shape of a runtime directory that does not exist.
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.Error(t, s.Rebind(ctx, filepath.Join(base, "missing", "n", "s.sock")))
+	require.Error(t, s.Rebind(ctx, filepath.Join(base, "missing", "n", "s.sock"), ownerOnly()))
 
 	good := filepath.Join(base, "n", "s.sock")
-	require.NoError(t, s.Rebind(ctx, good))
+	require.NoError(t, s.Rebind(ctx, good, ownerOnly()))
 	t.Cleanup(s.Stop)
 	_, err := (&Client{Path: good}).Describe()
 	require.NoError(t, err)
@@ -891,14 +891,14 @@ func TestRebindRefusesDuringShutdown(t *testing.T) {
 	// Built by hand rather than through serve: the context watcher is installed once,
 	// by the first bind, and this test needs it on a context it can cancel.
 	s := NewServer(test.NewLogger(), basicRegistry())
-	require.NoError(t, s.Rebind(ctx, testSocket(t)))
+	require.NoError(t, s.Rebind(ctx, testSocket(t), ownerOnly()))
 	t.Cleanup(s.Stop)
 
 	cancel()
 	assert.Eventually(t, func() bool { return s.Path() == "" }, 2*time.Second, 20*time.Millisecond, "the watcher stops the server")
 
 	next := testSocket(t)
-	err := s.Rebind(ctx, next)
+	err := s.Rebind(ctx, next, ownerOnly())
 	require.Error(t, err)
 	require.ErrorContains(t, err, "shutting down")
 	assert.Empty(t, s.Path(), "the server stays stopped")
@@ -916,13 +916,13 @@ func TestRebindAndStopSerialize(t *testing.T) {
 
 	for i := 0; i < 50; i++ {
 		s := NewServer(test.NewLogger(), basicRegistry())
-		require.NoError(t, s.Rebind(context.Background(), a))
+		require.NoError(t, s.Rebind(context.Background(), a, ownerOnly()))
 
 		var wg sync.WaitGroup
 		wg.Add(2)
 		go func() {
 			defer wg.Done()
-			_ = s.Rebind(context.Background(), b)
+			_ = s.Rebind(context.Background(), b, ownerOnly())
 		}()
 		go func() {
 			defer wg.Done()

localapi/wire.go

@@ -69,8 +69,18 @@ func followControlConfig(ctx context.Context, l *slog.Logger, c *config.C, srv *
 			return
 		}
 
+		acc, err := controlAccessFrom(c)
+		if err != nil {
+			if srv.Path() == "" {
+				l.Warn("Local control socket is not available, its group is unusable", "error", err)
+			} else {
+				l.Warn("Ignoring unusable local control socket group from reloaded config, keeping the current socket", "error", err)
+			}
+			return
+		}
+
 		old := srv.Path()
-		err = srv.Rebind(ctx, want)
+		err = srv.Rebind(ctx, want, acc)
 		reportRebind(l, old, want, err, srv.Path())
 	}
 

localapi/wire_test.go

@@ -62,7 +62,7 @@ func TestRebindAfterReloadBoundTheSocket(t *testing.T) {
 	require.Equal(t, path, srv.Path(), "the reload callback does the first bind")
 
 	// Wire's second half.
-	require.NoError(t, srv.Rebind(ctx, path))
+	require.NoError(t, srv.Rebind(ctx, path, ownerOnly()))
 	assert.Equal(t, path, srv.Path())
 	out, errText := run(t, path, "echo", "hi")
 	assert.Empty(t, errText)