From b49b5494b4394b22d1e0c27bbf5d86e5537cc883 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Tue, 8 Sep 2026 18:14:41 +0200 Subject: [PATCH] Add an optional --allow-group flag restricting the daemon sockets to a group --- client/cmd/service.go | 16 ++ client/cmd/service_allow_group.go | 70 +++++++++ client/cmd/service_allow_group_test.go | 79 ++++++++++ client/cmd/service_allow_group_unix.go | 102 +++++++++++++ client/cmd/service_allow_group_unix_test.go | 137 ++++++++++++++++++ client/cmd/service_allow_group_windows.go | 71 +++++++++ .../cmd/service_allow_group_windows_test.go | 114 +++++++++++++++ client/cmd/service_controller.go | 29 ++-- client/cmd/service_installer.go | 16 ++ client/cmd/service_params.go | 17 +++ client/cmd/service_params_test.go | 21 ++- client/cmd/service_pipe_other.go | 2 +- client/cmd/service_pipe_windows.go | 17 ++- client/cmd/service_socket.go | 21 ++- client/internal/getent/cgo_unix.go | 5 + client/internal/getent/nocgo_unix.go | 19 +++ client/internal/ipcauth/creds_windows.go | 44 ++++++ 17 files changed, 755 insertions(+), 25 deletions(-) create mode 100644 client/cmd/service_allow_group.go create mode 100644 client/cmd/service_allow_group_test.go create mode 100644 client/cmd/service_allow_group_unix.go create mode 100644 client/cmd/service_allow_group_unix_test.go create mode 100644 client/cmd/service_allow_group_windows.go create mode 100644 client/cmd/service_allow_group_windows_test.go diff --git a/client/cmd/service.go b/client/cmd/service.go index f1c65fe05..30436418e 100644 --- a/client/cmd/service.go +++ b/client/cmd/service.go @@ -31,6 +31,14 @@ var ( serviceEnvVars []string jsonSocket string enableJSONSocket bool + // allowGroups holds the --allow-group values as given: group or account + // names, or principals already in kind:value form. resolveAllowGroups turns + // them into the principals the daemon enforces. + allowGroups []string + // resolvedAllowGroups holds those principals after the install-time + // resolution, for persisting and for the arguments the installed service + // runs with. + resolvedAllowGroups []string ) type program struct { @@ -63,6 +71,14 @@ func init() { serviceCmd.PersistentFlags().BoolVar(&enableJSONSocket, "enable-json-socket", false, "Enables the HTTP/JSON API socket served by grpc-gateway. To persist, use: netbird service install --enable-json-socket") serviceCmd.PersistentFlags().StringVar(&jsonSocket, "json-socket", defaultJSONSocket, "HTTP/JSON API socket address [unix|tcp]://[path|host:port]. Requires --enable-json-socket to serve. To persist, use: netbird service install --enable-json-socket --json-socket") + allowGroupDesc := `Restricts the daemon control socket and the JSON socket to the given group. ` + + `Local accounts outside it cannot connect at all, so nothing the daemon exposes is reachable from them. ` + + `Takes a group name, or a numeric GID on Unix and a SID on Windows; ` + + `Unix accepts a single group, Windows a comma-separated list of groups or accounts. ` + + `Names are resolved when the service is installed, LDAP, SSSD and Active Directory groups included. ` + + `To persist, use: netbird service install --allow-group` + serviceCmd.PersistentFlags().StringSliceVar(&allowGroups, "allow-group", nil, allowGroupDesc) + rootCmd.PersistentFlags().StringVarP(&serviceName, "service", "s", defaultServiceName, "Netbird system service name") serviceEnvDesc := `Sets extra environment variables for the service. ` + `You can specify a comma-separated list of KEY=VALUE pairs. ` + diff --git a/client/cmd/service_allow_group.go b/client/cmd/service_allow_group.go new file mode 100644 index 000000000..b4d2088da --- /dev/null +++ b/client/cmd/service_allow_group.go @@ -0,0 +1,70 @@ +//go:build !ios && !android + +package cmd + +import ( + "fmt" + "slices" + "strings" +) + +// Principal kinds an --allow-group value resolves to. The kind:value form is +// the same one profile owners use, so a value in service.json or in a service +// unit says which namespace it belongs to instead of being a bare number that +// means one thing on Unix and another on Windows. +const ( + allowGroupKindGID = "gid" + allowGroupKindSID = "sid" +) + +// resolveAllowGroups turns the values given on --allow-group into the typed +// principals the daemon enforces on its sockets, dropping empty entries and +// duplicates. Names are resolved through the platform's directory service, so +// an entry that does not resolve is an error: a restriction the daemon cannot +// evaluate must not become one that admits everybody. Values already in +// kind:value form are validated and passed through, which is the form an +// installed service hands to `service run`. +func resolveAllowGroups(values []string) ([]string, error) { + var resolved []string + for _, value := range values { + value = strings.TrimSpace(value) + if value == "" { + continue + } + + principal, err := resolveAllowGroup(value) + if err != nil { + return nil, fmt.Errorf("resolve --allow-group %q: %w", value, err) + } + if !slices.Contains(resolved, principal) { + resolved = append(resolved, principal) + } + } + + if err := checkAllowGroupSet(resolved); err != nil { + return nil, err + } + return resolved, nil +} + +// cutKind splits a value on the kind separator. ok is false when the value +// carries no kind, which is the case for a plain group or account name: neither +// a Unix group name nor a Windows account name may contain a colon, so the +// separator is unambiguous. +func cutKind(value string) (kind, rest string, ok bool) { + kind, rest, ok = strings.Cut(value, ":") + if !ok || rest == "" { + return "", value, false + } + return kind, rest, true +} + +// principalValue returns the value part of a kind:value principal of the +// expected kind. +func principalValue(principal, kind string) (string, bool) { + got, value, ok := strings.Cut(principal, ":") + if !ok || got != kind || value == "" { + return "", false + } + return value, true +} diff --git a/client/cmd/service_allow_group_test.go b/client/cmd/service_allow_group_test.go new file mode 100644 index 000000000..78a3c862a --- /dev/null +++ b/client/cmd/service_allow_group_test.go @@ -0,0 +1,79 @@ +//go:build !ios && !android + +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveAllowGroups_NoValuesLeavesSocketOpen(t *testing.T) { + for name, values := range map[string][]string{ + "nil": nil, + "empty slice": {}, + "empty string": {""}, + "only whitespace": {" ", "\t"}, + } { + t.Run(name, func(t *testing.T) { + resolved, err := resolveAllowGroups(values) + require.NoError(t, err) + assert.Empty(t, resolved) + }) + } +} + +func TestResolveAllowGroups_Deduplicates(t *testing.T) { + resolved, err := resolveAllowGroups([]string{testAllowGroupPrincipal, testAllowGroupPrincipal, " " + testAllowGroupPrincipal + " "}) + require.NoError(t, err) + assert.Equal(t, []string{testAllowGroupPrincipal}, resolved) +} + +func TestResolveAllowGroups_UnresolvableIsAnError(t *testing.T) { + _, err := resolveAllowGroups([]string{"no-such-group-08b1f0c4"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no-such-group-08b1f0c4") +} + +func TestCutKind(t *testing.T) { + tests := []struct { + value string + kind string + rest string + ok bool + }{ + {value: "gid:1000", kind: "gid", rest: "1000", ok: true}, + {value: "sid:S-1-5-32-544", kind: "sid", rest: "S-1-5-32-544", ok: true}, + {value: "netbird-users", rest: "netbird-users"}, + {value: "S-1-5-32-544", rest: "S-1-5-32-544"}, + {value: `NETBIRD\Users`, rest: `NETBIRD\Users`}, + // A kind with no value is not a kind: it must not be mistaken for one + // and accepted as an empty principal. + {value: "gid:", rest: "gid:"}, + } + + for _, tc := range tests { + t.Run(tc.value, func(t *testing.T) { + kind, rest, ok := cutKind(tc.value) + assert.Equal(t, tc.ok, ok) + assert.Equal(t, tc.kind, kind) + assert.Equal(t, tc.rest, rest) + }) + } +} + +func TestPrincipalValue(t *testing.T) { + value, ok := principalValue("gid:1000", allowGroupKindGID) + assert.True(t, ok) + assert.Equal(t, "1000", value) + + _, ok = principalValue("sid:S-1-5-32-544", allowGroupKindGID) + assert.False(t, ok, "a principal of another kind must not be read as this one") + + _, ok = principalValue("1000", allowGroupKindGID) + assert.False(t, ok, "an untyped value is not a principal") + + _, ok = principalValue("gid:", allowGroupKindGID) + assert.False(t, ok, "an empty value is not a principal") +} diff --git a/client/cmd/service_allow_group_unix.go b/client/cmd/service_allow_group_unix.go new file mode 100644 index 000000000..f5e3b2391 --- /dev/null +++ b/client/cmd/service_allow_group_unix.go @@ -0,0 +1,102 @@ +//go:build !windows && !ios && !android + +package cmd + +import ( + "fmt" + "os" + "strconv" + + "github.com/netbirdio/netbird/client/internal/getent" +) + +// Socket modes. openSocketMode is the historical one: any local account may +// connect, and what it may then do is decided from its kernel-attested +// identity. restrictedSocketMode is what --allow-group installs, where the +// kernel refuses the connect() outright for an account outside the group. +const ( + openSocketMode os.FileMode = 0666 + restrictedSocketMode os.FileMode = 0660 +) + +// resolveAllowGroup resolves one --allow-group value to a "gid:" +// principal. A numeric value, with or without the prefix, is the GID itself; +// anything else is a group name resolved through NSS, so groups that only +// LDAP, SSSD or winbind know about work as well as ones in /etc/group. +func resolveAllowGroup(value string) (string, error) { + if kind, rest, ok := cutKind(value); ok { + if kind != allowGroupKindGID { + return "", fmt.Errorf("unsupported principal kind %q, use a group name or %s:", kind, allowGroupKindGID) + } + return gidPrincipal(rest) + } + + if _, err := parseGID(value); err == nil { + return gidPrincipal(value) + } + + group, err := getent.LookupGroupName(value) + if err != nil { + return "", fmt.Errorf("look up group: %w", err) + } + return gidPrincipal(group.Gid) +} + +// checkAllowGroupSet rejects more than one principal: a Unix socket carries a +// single owning group, so a second one could not be enforced and must not be +// accepted as though it were. +func checkAllowGroupSet(principals []string) error { + if len(principals) > 1 { + return fmt.Errorf("--allow-group takes a single group on this platform, got %d: %v", len(principals), principals) + } + return nil +} + +// applySocketAccess sets the access the socket grants to other accounts: the +// allowed group at 0660, or every local account at 0666 when no group is +// configured. +// +// The owner is left untouched so a daemon running as an ordinary user, as in a +// rootless container, keeps access to the socket it created. The group is set +// before the mode is widened, so the window between the two is one where the +// group has no access rather than one where it has access it should not. +func applySocketAccess(path string, principals []string) error { + if len(principals) == 0 { + if err := os.Chmod(path, openSocketMode); err != nil { + return fmt.Errorf("set mode %#o: %w", openSocketMode, err) + } + return nil + } + + value, ok := principalValue(principals[0], allowGroupKindGID) + if !ok { + return fmt.Errorf("not a %s principal: %q", allowGroupKindGID, principals[0]) + } + gid, err := parseGID(value) + if err != nil { + return err + } + + if err := os.Chown(path, -1, gid); err != nil { + return fmt.Errorf("set group to gid %d: %w", gid, err) + } + if err := os.Chmod(path, restrictedSocketMode); err != nil { + return fmt.Errorf("set mode %#o: %w", restrictedSocketMode, err) + } + return nil +} + +func gidPrincipal(gid string) (string, error) { + if _, err := parseGID(gid); err != nil { + return "", err + } + return allowGroupKindGID + ":" + gid, nil +} + +func parseGID(value string) (int, error) { + gid, err := strconv.ParseUint(value, 10, 32) + if err != nil { + return 0, fmt.Errorf("parse gid %q: %w", value, err) + } + return int(gid), nil +} diff --git a/client/cmd/service_allow_group_unix_test.go b/client/cmd/service_allow_group_unix_test.go new file mode 100644 index 000000000..7e5dde264 --- /dev/null +++ b/client/cmd/service_allow_group_unix_test.go @@ -0,0 +1,137 @@ +//go:build !windows && !ios && !android + +package cmd + +import ( + "net" + "os" + "path/filepath" + "strconv" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/getent" +) + +// testAllowGroupPrincipal is a principal that resolves on any Unix host. +const testAllowGroupPrincipal = "gid:0" + +func TestResolveAllowGroup_NumericGID(t *testing.T) { + for _, value := range []string{"0", "gid:0"} { + t.Run(value, func(t *testing.T) { + principal, err := resolveAllowGroup(value) + require.NoError(t, err) + assert.Equal(t, "gid:0", principal) + }) + } +} + +func TestResolveAllowGroup_ByName(t *testing.T) { + // The name of this process's primary group, so the test does not assume + // what gid 0 is called: Linux says "root", macOS says "wheel". + gid := strconv.Itoa(os.Getgid()) + group, err := getent.LookupGroupID(gid) + if err != nil { + t.Skipf("gid %s has no name on this host: %v", gid, err) + } + + principal, err := resolveAllowGroup(group.Name) + require.NoError(t, err) + assert.Equal(t, "gid:"+gid, principal) +} + +func TestResolveAllowGroup_Rejects(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "windows principal", value: "sid:S-1-5-32-544"}, + {name: "unknown kind", value: "user:alice"}, + {name: "non-numeric gid", value: "gid:wheel"}, + {name: "negative gid", value: "gid:-1"}, + {name: "unknown group", value: "no-such-group-08b1f0c4"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := resolveAllowGroup(tc.value) + assert.Error(t, err) + }) + } +} + +// A Unix socket carries one owning group, so a second one could not be +// enforced and must be refused rather than silently dropped. +func TestCheckAllowGroupSet_SingleGroupOnly(t *testing.T) { + assert.NoError(t, checkAllowGroupSet(nil)) + assert.NoError(t, checkAllowGroupSet([]string{"gid:0"})) + assert.Error(t, checkAllowGroupSet([]string{"gid:0", "gid:1"})) +} + +func TestApplySocketAccess(t *testing.T) { + t.Run("no principals leaves the socket open", func(t *testing.T) { + path := listenTestSocket(t) + + require.NoError(t, applySocketAccess(path, nil)) + assert.Equal(t, os.FileMode(0666), socketMode(t, path)) + }) + + t.Run("a principal hands the socket to that group", func(t *testing.T) { + path := listenTestSocket(t) + // The process's own primary group: chown to any other group needs + // privileges the test does not have. + gid := os.Getgid() + + require.NoError(t, applySocketAccess(path, []string{"gid:" + strconv.Itoa(gid)})) + assert.Equal(t, os.FileMode(0660), socketMode(t, path)) + assert.Equal(t, uint32(gid), socketGID(t, path)) + }) + + t.Run("a principal of another platform is refused", func(t *testing.T) { + path := listenTestSocket(t) + + require.Error(t, applySocketAccess(path, []string{"sid:S-1-5-32-544"})) + }) + + t.Run("an unparseable gid is refused", func(t *testing.T) { + path := listenTestSocket(t) + + require.Error(t, applySocketAccess(path, []string{"gid:wheel"})) + }) +} + +func listenTestSocket(t *testing.T) string { + t.Helper() + + // Short, because the sun_path of a Unix socket is about 100 bytes and + // t.TempDir() embeds the test name. + dir, err := os.MkdirTemp("", "nb-sock") + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) }) + + path := filepath.Join(dir, "d.sock") + listener, err := net.Listen("unix", path) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, listener.Close()) }) + + return path +} + +func socketMode(t *testing.T, path string) os.FileMode { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) + return info.Mode().Perm() +} + +func socketGID(t *testing.T, path string) uint32 { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) + stat, ok := info.Sys().(*syscall.Stat_t) + require.True(t, ok, "stat of %s is not a syscall.Stat_t", path) + return stat.Gid +} diff --git a/client/cmd/service_allow_group_windows.go b/client/cmd/service_allow_group_windows.go new file mode 100644 index 000000000..f108f3897 --- /dev/null +++ b/client/cmd/service_allow_group_windows.go @@ -0,0 +1,71 @@ +//go:build windows + +package cmd + +import ( + "fmt" + + "golang.org/x/sys/windows" + + "github.com/netbirdio/netbird/client/internal/ipcauth" +) + +// resolveAllowGroup resolves one --allow-group value to a "sid:" +// principal. A value already in SID form, with or without the prefix, is +// validated and canonicalised; anything else is an account name resolved with +// LookupAccountName, which goes through LSA and so resolves domain groups on a +// joined machine as readily as local ones. +// +// Both a group and a user account are accepted. The DACL grants a SID without +// caring which it is, and an administrator restricting the daemon to a single +// service account should not have to create a group for it. +func resolveAllowGroup(value string) (string, error) { + if kind, rest, ok := cutKind(value); ok { + if kind != allowGroupKindSID { + return "", fmt.Errorf("unsupported principal kind %q, use an account name or %s:", kind, allowGroupKindSID) + } + return sidPrincipal(rest) + } + + if _, err := windows.StringToSid(value); err == nil { + return sidPrincipal(value) + } + + sid, _, _, err := windows.LookupSID("", value) + if err != nil { + return "", fmt.Errorf("look up account: %w", err) + } + return allowGroupKindSID + ":" + sid.String(), nil +} + +// checkAllowGroupSet accepts any number of principals: a pipe descriptor holds +// one ACE per principal. +func checkAllowGroupSet([]string) error { return nil } + +// applySocketAccess is a no-op on Windows, where access is decided by the +// security descriptor the pipe is created with rather than by a mode set on it +// afterwards. See allowedPipeSDDL. +func applySocketAccess(string, []string) error { return nil } + +// allowedPipeSDDL renders the security descriptor for the daemon control pipe. +// An empty principal list yields the descriptor that lets any local caller +// connect. +func allowedPipeSDDL(principals []string) (string, error) { + sids := make([]string, 0, len(principals)) + for _, principal := range principals { + sid, ok := principalValue(principal, allowGroupKindSID) + if !ok { + return "", fmt.Errorf("not a %s principal: %q", allowGroupKindSID, principal) + } + sids = append(sids, sid) + } + return ipcauth.RestrictedPipeSDDL(sids), nil +} + +func sidPrincipal(value string) (string, error) { + sid, err := windows.StringToSid(value) + if err != nil { + return "", fmt.Errorf("parse SID %q: %w", value, err) + } + return allowGroupKindSID + ":" + sid.String(), nil +} diff --git a/client/cmd/service_allow_group_windows_test.go b/client/cmd/service_allow_group_windows_test.go new file mode 100644 index 000000000..419b57a6f --- /dev/null +++ b/client/cmd/service_allow_group_windows_test.go @@ -0,0 +1,114 @@ +//go:build windows + +package cmd + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +// accountName returns the name the local system knows a SID by. +func accountName(sid string) (string, error) { + parsed, err := windows.StringToSid(sid) + if err != nil { + return "", err + } + account, domain, _, err := parsed.LookupAccount("") + if err != nil { + return "", err + } + if domain == "" { + return account, nil + } + return domain + `\` + account, nil +} + +// sidAdministrators is BUILTIN\Administrators, a group present on every +// Windows install, localised name and all. +const sidAdministrators = "S-1-5-32-544" + +// testAllowGroupPrincipal is a principal that resolves on any Windows host. +const testAllowGroupPrincipal = "sid:" + sidAdministrators + +func TestResolveAllowGroup_SID(t *testing.T) { + for _, value := range []string{sidAdministrators, "sid:" + sidAdministrators, strings.ToLower(sidAdministrators)} { + t.Run(value, func(t *testing.T) { + principal, err := resolveAllowGroup(value) + require.NoError(t, err) + assert.Equal(t, "sid:"+sidAdministrators, principal) + }) + } +} + +func TestResolveAllowGroup_ByName(t *testing.T) { + // The well-known SID resolves to whatever the account is called in this + // install's language, and that name must resolve back to the same SID. + name, err := accountName(sidAdministrators) + require.NoError(t, err) + + principal, err := resolveAllowGroup(name) + require.NoError(t, err) + assert.Equal(t, "sid:"+sidAdministrators, principal) +} + +func TestResolveAllowGroup_Rejects(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "unix principal", value: "gid:0"}, + {name: "unknown kind", value: "user:alice"}, + {name: "malformed SID", value: "sid:S-1-not-a-sid"}, + {name: "unknown account", value: "no-such-account-08b1f0c4"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := resolveAllowGroup(tc.value) + assert.Error(t, err) + }) + } +} + +// A pipe descriptor holds one ACE per principal, so any number is enforceable. +func TestCheckAllowGroupSet_AcceptsAny(t *testing.T) { + assert.NoError(t, checkAllowGroupSet(nil)) + assert.NoError(t, checkAllowGroupSet([]string{"sid:" + sidAdministrators, "sid:S-1-5-18"})) +} + +func TestAllowedPipeSDDL(t *testing.T) { + t.Run("no principals leaves the pipe open", func(t *testing.T) { + sddl, err := allowedPipeSDDL(nil) + require.NoError(t, err) + assert.Contains(t, sddl, "(A;;GA;;;WD)", "an unconfigured pipe stays open to every local caller") + }) + + t.Run("a principal replaces the Everyone ACE", func(t *testing.T) { + sddl, err := allowedPipeSDDL([]string{"sid:S-1-5-21-1-2-3-1001"}) + require.NoError(t, err) + assert.NotContains(t, sddl, "(A;;GA;;;WD)") + assert.Contains(t, sddl, "(A;;GA;;;S-1-5-21-1-2-3-1001)") + assert.Contains(t, sddl, "(A;;GA;;;SY)", "LocalSystem runs the daemon") + assert.Contains(t, sddl, "(A;;GA;;;BA)", "an elevated caller is never locked out") + assert.True(t, strings.HasPrefix(sddl, "D:P"), "the DACL must stay protected: %s", sddl) + }) + + t.Run("a principal of another platform is refused", func(t *testing.T) { + _, err := allowedPipeSDDL([]string{"gid:0"}) + require.Error(t, err) + }) +} + +// TestListenNamedPipe_RestrictedDescriptor covers the descriptor reaching the +// listener, which is the only part of the restriction that cannot be asserted +// from allowedPipeSDDL alone. +func TestListenNamedPipe_RestrictedDescriptor(t *testing.T) { + listener, path, err := listenNamedPipe("netbird-test-"+t.Name(), []string{"sid:" + sidAdministrators}) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, listener.Close()) }) + assert.NotEmpty(t, path) +} diff --git a/client/cmd/service_controller.go b/client/cmd/service_controller.go index 55cf11d1f..f1f1c6f12 100644 --- a/client/cmd/service_controller.go +++ b/client/cmd/service_controller.go @@ -89,7 +89,15 @@ func (p *program) Start(svc service.Service) error { ) p.serv = grpc.NewServer(opts...) - daemonListener, jsonListener, err := listenDaemonSockets() + allowed, err := resolveAllowGroups(allowGroups) + if err != nil { + return err + } + if len(allowed) > 0 { + log.Infof("daemon sockets are restricted to %v", allowed) + } + + daemonListener, jsonListener, err := listenDaemonSockets(allowed) if err != nil { return err } @@ -97,7 +105,7 @@ func (p *program) Start(svc service.Service) error { go func() { // Fatal here rather than inside serve, so serve's deferred listener // closes run before the process exits. - if err := p.serve(daemonListener, jsonListener); err != nil { + if err := p.serve(daemonListener, jsonListener, allowed); err != nil { log.Fatalf("failed to %v", err) } }() @@ -107,9 +115,10 @@ func (p *program) Start(svc service.Service) error { // listenDaemonSockets opens the daemon control socket and, when it is enabled, the // JSON gateway socket. The control socket is closed again if the second one fails, // so a failed start leaves nothing listening. The returned JSON listener is nil -// when the socket is disabled. -func listenDaemonSockets() (*socketListener, *socketListener, error) { - daemonListener, err := listenOnAddress(daemonAddr) +// when the socket is disabled. allowed holds the resolved --allow-group +// principals, empty when both sockets are left open to every local account. +func listenDaemonSockets(allowed []string) (*socketListener, *socketListener, error) { + daemonListener, err := listenOnAddress(daemonAddr, allowed) if err != nil { return nil, nil, fmt.Errorf("listen daemon interface: %w", err) } @@ -119,7 +128,7 @@ func listenDaemonSockets() (*socketListener, *socketListener, error) { return daemonListener, nil, nil } - jsonListener, err := listenOnAddress(jsonSocket) + jsonListener, err := listenOnAddress(jsonSocket, allowed) if err != nil { if cerr := daemonListener.Close(); cerr != nil { log.Debugf("close daemon listener: %v", cerr) @@ -134,18 +143,18 @@ func listenDaemonSockets() (*socketListener, *socketListener, error) { // until it stops. jsonListener is nil when the JSON socket is disabled. A returned // error means the daemon cannot run at all and the caller is expected to exit; the // failures it recovers from on its own are logged here. -func (p *program) serve(daemonListener, jsonListener *socketListener) error { +func (p *program) serve(daemonListener, jsonListener *socketListener, allowed []string) error { defer daemonListener.Close() if jsonListener != nil { defer jsonListener.Close() } - // chmodUnixSocket is a no-op for a nil listener and for a non-unix one. - if err := daemonListener.chmodUnixSocket("daemon"); err != nil { + // restrict is a no-op for a nil listener and for a non-unix one. + if err := daemonListener.restrict("daemon", allowed); err != nil { log.Error(err) return nil } - if err := jsonListener.chmodUnixSocket("daemon JSON"); err != nil { + if err := jsonListener.restrict("daemon JSON", allowed); err != nil { log.Error(err) return nil } diff --git a/client/cmd/service_installer.go b/client/cmd/service_installer.go index ae2dfb9fa..e5ec88380 100644 --- a/client/cmd/service_installer.go +++ b/client/cmd/service_installer.go @@ -71,6 +71,13 @@ func buildServiceArguments() []string { args = append(args, "--enable-json-socket", "--json-socket", jsonSocket) } + // The resolved principals rather than the names the administrator typed, so + // the daemon needs no directory lookup on the boot path and a group renamed + // or recreated after the install cannot silently change who may connect. + for _, principal := range resolvedAllowGroups { + args = append(args, "--allow-group", principal) + } + return args } @@ -114,6 +121,15 @@ func createServiceConfigForInstall() (*service.Config, error) { return nil, err } + // Resolved here, where a name that does not exist fails the install in front + // of the administrator, rather than at boot where it would leave the daemon + // unable to serve anybody. + resolved, err := resolveAllowGroups(allowGroups) + if err != nil { + return nil, err + } + resolvedAllowGroups = resolved + svcConfig, err := newSVCConfig() if err != nil { return nil, fmt.Errorf("create service config: %w", err) diff --git a/client/cmd/service_params.go b/client/cmd/service_params.go index 750b22ae6..1d108f98e 100644 --- a/client/cmd/service_params.go +++ b/client/cmd/service_params.go @@ -34,6 +34,13 @@ type serviceParams struct { DisableNetworks bool `json:"disable_networks,omitempty"` EnableJSONSocket bool `json:"enable_json_socket,omitempty"` ServiceEnvVars map[string]string `json:"service_env_vars,omitempty"` + // AllowGroups holds the --allow-group values as the administrator gave + // them, so a reinstall restricts the daemon to the same group even if its + // ID has changed in the meantime. + AllowGroups []string `json:"allow_groups,omitempty"` + // AllowGroupIDs holds what those values resolved to at install time, in + // kind:value form. This is what the installed service actually enforces. + AllowGroupIDs []string `json:"allow_group_ids,omitempty"` } // serviceParamsPath returns the path to the service params file. @@ -87,6 +94,8 @@ func currentServiceParams() *serviceParams { EnableCapture: captureEnabled, DisableNetworks: networksDisabled, EnableJSONSocket: enableJSONSocket, + AllowGroups: allowGroups, + AllowGroupIDs: resolvedAllowGroups, } if len(serviceEnvVars) > 0 { @@ -173,6 +182,14 @@ func applyServiceParams(cmd *cobra.Command, params *serviceParams) { networksDisabled = params.DisableNetworks } + // The names, not the resolved IDs: the resolution is redone on every + // install so a group that has been recreated with a different ID keeps + // working. Passing --allow-group "" leaves the flag Changed with no + // values, which drops the restriction rather than restoring the saved one. + if !serviceCmd.PersistentFlags().Changed("allow-group") { + allowGroups = params.AllowGroups + } + applyServiceEnvParams(cmd, params) } diff --git a/client/cmd/service_params_test.go b/client/cmd/service_params_test.go index 94f98a0ce..b7a018d0d 100644 --- a/client/cmd/service_params_test.go +++ b/client/cmd/service_params_test.go @@ -409,9 +409,19 @@ func TestServiceParams_FieldsCoveredInFunctions(t *testing.T) { applyFields[k] = v } + // AllowGroupIDs records what AllowGroups resolved to at install time. It is + // derived, so restoring it would pin an install to a stale ID instead of + // resolving the names again. + fieldsNotRestored := map[string]bool{ + "AllowGroupIDs": true, + } + for _, field := range structFields { assert.Contains(t, currentFields, field, "serviceParams field %q is not captured in currentServiceParams()", field) + if fieldsNotRestored[field] { + continue + } assert.Contains(t, applyFields, field, "serviceParams field %q is not restored in applyServiceParams()/applyServiceEnvParams()", field) } @@ -431,9 +441,13 @@ func TestServiceParams_BuildArgsCoversAllFlags(t *testing.T) { installerFile, err := parser.ParseFile(fset, "service_installer.go", nil, 0) require.NoError(t, err) - // Fields that are handled outside of buildServiceArguments (env vars go through newSVCConfig). + // Fields that are handled outside of buildServiceArguments: + // ServiceEnvVars goes through newSVCConfig() EnvVars. AllowGroups is + // carried as AllowGroupIDs instead: the service runs with the resolved + // principals, not with the names they were resolved from. fieldsNotInArgs := map[string]bool{ "ServiceEnvVars": true, + "AllowGroups": true, } buildFields := extractFuncGlobalRefs(t, installerFile, "buildServiceArguments") @@ -459,7 +473,8 @@ func TestServiceParams_BuildArgsCoversAllFlags(t *testing.T) { // (builtins, boilerplate, loop variables). nonParamGlobals := map[string]bool{ "args": true, "append": true, "string": true, "_": true, - "logFile": true, // range variable over logFiles + "logFile": true, // range variable over logFiles + "principal": true, // range variable over resolvedAllowGroups } for ref := range buildFields { if nonParamGlobals[ref] { @@ -566,6 +581,8 @@ func fieldToGlobalVar(field string) string { "DisableNetworks": "networksDisabled", "EnableJSONSocket": "enableJSONSocket", "ServiceEnvVars": "serviceEnvVars", + "AllowGroups": "allowGroups", + "AllowGroupIDs": "resolvedAllowGroups", } if v, ok := m[field]; ok { return v diff --git a/client/cmd/service_pipe_other.go b/client/cmd/service_pipe_other.go index c7cc72469..df6a4eb2e 100644 --- a/client/cmd/service_pipe_other.go +++ b/client/cmd/service_pipe_other.go @@ -9,6 +9,6 @@ import ( // listenNamedPipe is Windows-only: no other platform serves the daemon on a // named pipe. -func listenNamedPipe(string) (net.Listener, string, error) { +func listenNamedPipe(string, []string) (net.Listener, string, error) { return nil, "", fmt.Errorf("named pipes are only supported on Windows") } diff --git a/client/cmd/service_pipe_windows.go b/client/cmd/service_pipe_windows.go index b6e860f51..73f72690e 100644 --- a/client/cmd/service_pipe_windows.go +++ b/client/cmd/service_pipe_windows.go @@ -11,23 +11,28 @@ import ( log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/client/internal/daemonaddr" - "github.com/netbirdio/netbird/client/internal/ipcauth" ) // listenNamedPipe creates the daemon control pipe and reports the path it ended -// up on. The security descriptor lets any local caller connect, as a Unix socket -// at 0666 does, and the privileged operations are authorized separately from the -// caller's token. +// up on. Without allowed principals the security descriptor lets any local +// caller connect, as a Unix socket at 0666 does, and the privileged operations +// are authorized separately from the caller's token; with them, only those +// principals may open the pipe at all. // // The protected name comes first so that an unprivileged process cannot take the // name before the service does. Creating it requires being an administrator or // LocalSystem, so a daemon an ordinary user runs themselves, as in netstack mode, // falls back to the plain name; clients try both and check who serves them. -func listenNamedPipe(name string) (net.Listener, string, error) { +func listenNamedPipe(name string, allowed []string) (net.Listener, string, error) { + sddl, err := allowedPipeSDDL(allowed) + if err != nil { + return nil, "", err + } + var errs []error for _, path := range daemonaddr.PipePaths(name) { listener, err := winio.ListenPipe(path, &winio.PipeConfig{ - SecurityDescriptor: ipcauth.DefaultPipeSDDL(), + SecurityDescriptor: sddl, }) if err != nil { log.Debugf("not serving the daemon on %s: %v", path, err) diff --git a/client/cmd/service_socket.go b/client/cmd/service_socket.go index bf3122f7c..0d97ebd2d 100644 --- a/client/cmd/service_socket.go +++ b/client/cmd/service_socket.go @@ -20,15 +20,20 @@ type socketListener struct { address string } -func listenOnAddress(addr string) (*socketListener, error) { +// listenOnAddress opens the daemon listener for addr. allowed holds the +// resolved principals from --allow-group, empty when the socket is left open to +// every local account; on Windows they go into the pipe's security descriptor, +// on Unix they are applied to the socket file by applySocketAccess once the +// listener exists. +func listenOnAddress(addr string, allowed []string) (*socketListener, error) { network, address, err := parseListenAddress(addr) if err != nil { return nil, err } if network == "npipe" { - listener, path, err := listenNamedPipe(address) //nolint:staticcheck - if err != nil { //nolint:staticcheck // always errors on non-Windows builds + listener, path, err := listenNamedPipe(address, allowed) //nolint:staticcheck + if err != nil { //nolint:staticcheck // always errors on non-Windows builds return nil, err } return &socketListener{Listener: listener, network: network, address: path}, nil @@ -107,13 +112,17 @@ func removeStaleUnixSocketForAddress(addr string) { removeStaleUnixSocket(address) } -func (l *socketListener) chmodUnixSocket(description string) error { +// restrict sets the access the socket file grants, from the principals resolved +// out of --allow-group. It is a no-op for a nil listener, and for anything that +// is not a Unix socket: a named pipe carries its access rules in the security +// descriptor it was created with. +func (l *socketListener) restrict(description string, allowed []string) error { if l == nil || l.network != "unix" { return nil } - if err := os.Chmod(l.address, 0666); err != nil { - return fmt.Errorf("failed setting %s permissions for %s: %w", description, l.address, err) + if err := applySocketAccess(l.address, allowed); err != nil { + return fmt.Errorf("restrict %s socket %s: %w", description, l.address, err) } return nil } diff --git a/client/internal/getent/cgo_unix.go b/client/internal/getent/cgo_unix.go index 2853aafff..cff6427ea 100644 --- a/client/internal/getent/cgo_unix.go +++ b/client/internal/getent/cgo_unix.go @@ -29,6 +29,11 @@ func LookupGroupID(gid string) (*user.Group, error) { return user.LookupGroupId(gid) } +// LookupGroupName looks up a group by name. +func LookupGroupName(name string) (*user.Group, error) { + return user.LookupGroup(name) +} + // GroupIDs returns the IDs of the groups the user is a member of; libc's // getgrouplist handles NSS groups natively. func GroupIDs(u *user.User) ([]string, error) { diff --git a/client/internal/getent/nocgo_unix.go b/client/internal/getent/nocgo_unix.go index 94d8ea6a9..363ea250b 100644 --- a/client/internal/getent/nocgo_unix.go +++ b/client/internal/getent/nocgo_unix.go @@ -88,6 +88,25 @@ func LookupGroupID(gid string) (*user.Group, error) { return g, nil } +// LookupGroupName looks up a group by name, falling back to getent if os/user +// fails. +func LookupGroupName(name string) (*user.Group, error) { + g, err := user.LookupGroup(name) + if err == nil { + return g, nil + } + + stdErr := err + log.Debugf("os/user.LookupGroup(%q) failed, trying getent: %v", name, err) + + g, _, getentErr := groupLookup(name) + if getentErr != nil { + log.Debugf("getent fallback for group %q also failed: %v", name, getentErr) + return nil, stdErr + } + return g, nil +} + // GroupIDs returns the IDs of the groups the user is a member of. // NOTE: unlike the lookups above, which try the standard library first, this // intentionally tries `id -G` first because without cgo, user.GroupIds only diff --git a/client/internal/ipcauth/creds_windows.go b/client/internal/ipcauth/creds_windows.go index a7a13172c..54906773d 100644 --- a/client/internal/ipcauth/creds_windows.go +++ b/client/internal/ipcauth/creds_windows.go @@ -7,6 +7,8 @@ import ( "fmt" "net" "runtime" + "slices" + "strings" log "github.com/sirupsen/logrus" "golang.org/x/sys/windows" @@ -38,6 +40,48 @@ func DefaultPipeSDDL() string { return "D:P(A;;GA;;;SY)(A;;GA;;;WD)" } +// RestrictedPipeSDDL returns the security descriptor for a daemon control pipe +// that only the named principals may open, replacing DefaultPipeSDDL's ACE for +// Everyone. Each SID is a user or group SID in string form; the caller is +// expected to have resolved and validated them already. An empty list yields +// the default descriptor, so a missing configuration cannot silently produce a +// pipe nobody can reach. +// +// Three ACEs are always present besides the configured ones: +// +// SY LocalSystem, the account the daemon runs as when installed as a service +// BA BUILTIN\Administrators, so an elevated caller is never locked out +// the daemon's own user SID, so a daemon an ordinary user runs themselves can +// still dial itself, which is what the JSON gateway does +// +// BUILTIN\Administrators carries no access for a UAC-filtered administrator, +// whose token has that group deny-only, which matches the authorization model: +// such a caller is not privileged either. +func RestrictedPipeSDDL(sids []string) string { + if len(sids) == 0 { + return DefaultPipeSDDL() + } + + allowed := []string{"SY", "BA"} + if selfIdentity.Known() && selfIdentity.SID != "" { + allowed = append(allowed, selfIdentity.SID) + } + for _, sid := range sids { + if !slices.Contains(allowed, sid) { + allowed = append(allowed, sid) + } + } + + var b strings.Builder + b.WriteString("D:P") + for _, sid := range allowed { + b.WriteString("(A;;GA;;;") + b.WriteString(sid) + b.WriteString(")") + } + return b.String() +} + // NewTransportCredentials returns gRPC transport credentials that derive the // caller's identity from the named-pipe client token. //