Add an optional --allow-group flag restricting the daemon sockets (#7478)

This commit is contained in:
Viktor Liu
2026-09-16 13:30:10 +02:00
committed by GitHub
parent abb94ad2d2
commit 15ed6f8f15
25 changed files with 1482 additions and 45 deletions
+17
View File
@@ -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,15 @@ 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. ` +
`Accounts outside it cannot connect at all, so nothing the daemon exposes is reachable from them. ` +
`On Windows the daemon's own account, LocalSystem and elevated administrators keep access regardless. ` +
`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 <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. ` +
+158
View File
@@ -0,0 +1,158 @@
//go:build !ios && !android
package cmd
import (
"fmt"
"slices"
"strings"
"time"
"github.com/netbirdio/netbird/client/internal/ipcauth"
"github.com/netbirdio/netbird/client/mdm"
)
// resolveAllowGroups turns configured group values 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`.
//
// Each value is split on commas, so one managed-configuration string listing
// several principals behaves like the repeated flag. Neither a Unix group name
// nor a Windows account name may contain a comma, so nothing is lost by it.
func resolveAllowGroups(values []string) ([]string, error) {
var resolved []string
for _, value := range values {
for _, entry := range strings.Split(value, ",") {
entry = strings.TrimSpace(entry)
if entry == "" {
continue
}
principal, err := resolveAllowGroup(entry)
if err != nil {
return nil, fmt.Errorf("resolve allowed group %q: %w", entry, err)
}
if value := principal.String(); !slices.Contains(resolved, value) {
resolved = append(resolved, value)
}
}
}
if err := checkAllowGroupSet(resolved); err != nil {
return nil, err
}
return resolved, nil
}
// bootResolveTimeout bounds the group resolution the daemon does while
// starting. A value already in kind:value form resolves without a lookup, so
// this only bites on a name, which getent, NSS or LSA may answer from a
// directory service that is slow or unreachable.
const bootResolveTimeout = 5 * time.Second
// resolveAllowGroupsBounded resolves the configured groups without letting a
// directory lookup hold up the daemon's start indefinitely.
//
// The lookup runs on its own goroutine because the platform calls underneath it
// take no context: on timeout the daemon stops waiting and refuses to serve,
// while the goroutine finishes into a buffered channel nobody reads. Refusing
// is the same answer a failed resolution gets, since a restriction that cannot
// be evaluated must not become a socket open to everybody.
func resolveAllowGroupsBounded(values []string) ([]string, error) {
return resolveAllowGroupsWithin(values, bootResolveTimeout, resolveAllowGroups)
}
// resolveAllowGroupsWithin is resolveAllowGroupsBounded with the timeout and
// the resolver supplied, so a test can drive the deadline without waiting on
// one or needing a directory service that hangs.
func resolveAllowGroupsWithin(values []string, timeout time.Duration, resolve func([]string) ([]string, error)) ([]string, error) {
type outcome struct {
principals []string
err error
}
done := make(chan outcome, 1)
go func() {
principals, err := resolve(values)
done <- outcome{principals, err}
}()
timer := time.NewTimer(timeout)
defer timer.Stop()
select {
case res := <-done:
return res.principals, res.err
case <-timer.C:
return nil, fmt.Errorf("resolving the allowed groups took longer than %s: configure them as resolved principals (%s:<id> or %s:<SID>) so the daemon needs no directory lookup while starting",
timeout, ipcauth.KindGID, ipcauth.KindSID)
}
}
// daemonSocketPrincipals returns the principals the daemon restricts its
// sockets to, and the configuration that asked for them. An MDM policy
// overrides the install-time --allow-group in both directions, as the other
// MDM-overridable service flags do: a managed host can be restricted without a
// reinstall, and a managed empty value lifts a restriction the install set.
//
// A configured value that cannot be resolved is an error rather than an
// unrestricted socket. The daemon then does not serve at all, which is loud
// enough for an administrator to find and correct, where a silently ignored
// restriction would leave every local account reaching the daemon on a host
// meant to be locked down.
func daemonSocketPrincipals(policy *mdm.Policy) ([]string, string, error) {
values, source := allowGroups, "--allow-group"
if policy.HasKey(mdm.KeyAllowGroups) {
source = "MDM policy " + mdm.KeyAllowGroups
managed, ok := policy.GetStringSlice(mdm.KeyAllowGroups)
if !ok {
// The key is managed but holds something that is not a list of
// strings. Falling back to the install-time value, or to no
// restriction at all, would apply an access rule the administrator
// did not write.
return nil, source, fmt.Errorf("%s: managed value is not a list of principals", source)
}
values = managed
}
resolved, err := resolveAllowGroupsBounded(values)
if err != nil {
return nil, source, fmt.Errorf("%s: %w", source, err)
}
return resolved, source, nil
}
// typedPrincipal parses a value that carries an explicit kind, as
// ipcauth.Principal renders it. ok is false when the value carries no kind at
// all, 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. A value that has a kind the shared type does not know is an
// error rather than a name to look up.
func typedPrincipal(value string, want ipcauth.PrincipalKind) (ipcauth.Principal, bool, error) {
if !strings.Contains(value, ":") {
return ipcauth.Principal{}, false, nil
}
principal, ok := ipcauth.ParsePrincipal(value)
if !ok {
return ipcauth.Principal{}, false, fmt.Errorf("not a principal, use a name or %s:<value>", want)
}
if principal.Kind != want {
return ipcauth.Principal{}, false, fmt.Errorf("unsupported principal kind %q on this platform, use a name or %s:<value>", principal.Kind, want)
}
return principal, true, nil
}
// principalOfKind parses a stored principal that must be of the given kind.
func principalOfKind(value string, want ipcauth.PrincipalKind) (ipcauth.Principal, error) {
principal, ok := ipcauth.ParsePrincipal(value)
if !ok || principal.Kind != want {
return ipcauth.Principal{}, fmt.Errorf("not a %s principal: %q", want, value)
}
return principal, nil
}
+234
View File
@@ -0,0 +1,234 @@
//go:build !ios && !android
package cmd
import (
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/netbirdio/netbird/client/internal/ipcauth"
"github.com/netbirdio/netbird/client/mdm"
)
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 TestResolveAllowGroups_SplitsCommaSeparatedEntries(t *testing.T) {
// One managed-configuration string listing several principals, as a
// Windows REG_SZ delivers them.
resolved, err := resolveAllowGroups([]string{testAllowGroupPrincipal + ", " + testAllowGroupPrincipal})
require.NoError(t, err)
assert.Equal(t, []string{testAllowGroupPrincipal}, resolved)
}
func TestDaemonSocketPrincipals(t *testing.T) {
original := allowGroups
t.Cleanup(func() { allowGroups = original })
t.Run("no configuration leaves the sockets open", func(t *testing.T) {
allowGroups = nil
resolved, _, err := daemonSocketPrincipals(mdm.NewPolicy(nil))
require.NoError(t, err)
assert.Empty(t, resolved)
})
t.Run("the install-time flag applies when nothing is managed", func(t *testing.T) {
allowGroups = []string{testAllowGroupPrincipal}
resolved, source, err := daemonSocketPrincipals(mdm.NewPolicy(nil))
require.NoError(t, err)
assert.Equal(t, []string{testAllowGroupPrincipal}, resolved)
assert.Contains(t, source, "--allow-group")
})
t.Run("an MDM policy overrides the install-time flag", func(t *testing.T) {
allowGroups = nil
policy := mdm.NewPolicy(map[string]any{mdm.KeyAllowGroups: testAllowGroupPrincipal})
resolved, source, err := daemonSocketPrincipals(policy)
require.NoError(t, err)
assert.Equal(t, []string{testAllowGroupPrincipal}, resolved)
assert.Contains(t, source, mdm.KeyAllowGroups)
})
t.Run("an empty MDM value lifts an install-time restriction", func(t *testing.T) {
allowGroups = []string{testAllowGroupPrincipal}
policy := mdm.NewPolicy(map[string]any{mdm.KeyAllowGroups: ""})
resolved, source, err := daemonSocketPrincipals(policy)
require.NoError(t, err)
assert.Empty(t, resolved)
assert.Contains(t, source, mdm.KeyAllowGroups)
})
// A managed value that cannot be resolved must not fall back to the
// install-time flag or to an open socket: the host was meant to be locked
// down, so the daemon refuses to serve instead.
t.Run("an unresolvable MDM value is an error", func(t *testing.T) {
allowGroups = []string{testAllowGroupPrincipal}
policy := mdm.NewPolicy(map[string]any{mdm.KeyAllowGroups: "no-such-group-08b1f0c4"})
_, _, err := daemonSocketPrincipals(policy)
require.Error(t, err)
assert.Contains(t, err.Error(), mdm.KeyAllowGroups)
})
// A managed key holding something that is not a list of principals must not
// read as "no policy set" and quietly hand the decision back to the
// install-time value.
t.Run("a malformed MDM value is an error, not a fallback", func(t *testing.T) {
allowGroups = []string{testAllowGroupPrincipal}
for name, value := range map[string]any{
"number": 42,
"bool": true,
"map": map[string]any{"group": "x"},
} {
t.Run(name, func(t *testing.T) {
policy := mdm.NewPolicy(map[string]any{mdm.KeyAllowGroups: value})
_, source, err := daemonSocketPrincipals(policy)
require.Error(t, err)
assert.Contains(t, source, mdm.KeyAllowGroups, "the managed key must be named as the source that failed")
})
}
})
}
// A TCP listener can express neither a socket mode nor a security descriptor,
// so a restriction configured against one must stop the daemon rather than be
// dropped while it goes on serving every caller that can reach the port.
func TestTCPListenerRefusesARestriction(t *testing.T) {
t.Run("listenOnAddress refuses before binding", func(t *testing.T) {
listener, err := listenOnAddress("tcp://127.0.0.1:0", []string{testAllowGroupPrincipal})
require.Error(t, err)
require.Nil(t, listener)
assert.Contains(t, err.Error(), "tcp")
})
t.Run("listenOnAddress still serves tcp when nothing is configured", func(t *testing.T) {
listener, err := listenOnAddress("tcp://127.0.0.1:0", nil)
require.NoError(t, err)
t.Cleanup(func() { assert.NoError(t, listener.Close()) })
assert.NoError(t, listener.restrict("daemon", nil))
})
t.Run("restrict refuses a listener that cannot carry the restriction", func(t *testing.T) {
listener := &socketListener{network: "tcp", address: "127.0.0.1:41731"}
require.Error(t, listener.restrict("daemon", []string{testAllowGroupPrincipal}))
assert.NoError(t, listener.restrict("daemon", nil))
})
t.Run("a disabled json socket is not an error", func(t *testing.T) {
var listener *socketListener
assert.NoError(t, listener.restrict("daemon JSON", []string{testAllowGroupPrincipal}))
})
}
// A group name makes the daemon ask a directory service while it is starting.
// That lookup has to be bounded, or an unreachable LDAP or AD backend holds up
// the start until the service manager gives up on it.
func TestResolveAllowGroupsWithin(t *testing.T) {
t.Run("a resolution that answers in time is passed through", func(t *testing.T) {
resolved, err := resolveAllowGroupsWithin([]string{"anything"}, time.Minute,
func([]string) ([]string, error) { return []string{testAllowGroupPrincipal}, nil })
require.NoError(t, err)
assert.Equal(t, []string{testAllowGroupPrincipal}, resolved)
})
t.Run("a resolution error is passed through", func(t *testing.T) {
_, err := resolveAllowGroupsWithin([]string{"anything"}, time.Minute,
func([]string) ([]string, error) { return nil, assert.AnError })
require.ErrorIs(t, err, assert.AnError)
})
// The daemon must stop waiting and refuse to serve, which is the same
// answer a failed resolution gets: a restriction it cannot evaluate must
// never become a socket open to everybody.
t.Run("a lookup that never answers is an error, not an empty restriction", func(t *testing.T) {
blocked := make(chan struct{})
t.Cleanup(func() { close(blocked) })
resolved, err := resolveAllowGroupsWithin([]string{"netbird-users"}, 10*time.Millisecond,
func([]string) ([]string, error) {
<-blocked
return nil, nil
})
require.Error(t, err)
assert.Empty(t, resolved, "a timeout must not yield an unrestricted socket")
assert.Contains(t, err.Error(), "gid:<id>", "the error should say how to avoid the lookup")
})
}
func TestTypedPrincipal(t *testing.T) {
t.Run("a value with no kind is a name to look up", func(t *testing.T) {
for _, value := range []string{"netbird-users", `NETBIRD\Users`, "1000"} {
_, typed, err := typedPrincipal(value, ipcauth.KindGID)
require.NoError(t, err, value)
assert.False(t, typed, "%q carries no kind", value)
}
})
t.Run("a value of the wanted kind is parsed", func(t *testing.T) {
principal, typed, err := typedPrincipal("gid:1000", ipcauth.KindGID)
require.NoError(t, err)
assert.True(t, typed)
assert.Equal(t, ipcauth.KindGID, principal.Kind)
assert.Equal(t, "1000", principal.Value)
})
t.Run("a kind for another platform is an error, not a name", func(t *testing.T) {
_, _, err := typedPrincipal("sid:S-1-5-32-544", ipcauth.KindGID)
require.Error(t, err)
})
t.Run("an unknown kind is an error, not a name", func(t *testing.T) {
for _, value := range []string{"user:alice", "gid:"} {
_, _, err := typedPrincipal(value, ipcauth.KindGID)
require.Error(t, err, value)
}
})
}
func TestPrincipalOfKind(t *testing.T) {
principal, err := principalOfKind("gid:1000", ipcauth.KindGID)
require.NoError(t, err)
assert.Equal(t, "1000", principal.Value)
_, err = principalOfKind("sid:S-1-5-32-544", ipcauth.KindGID)
assert.Error(t, err, "a principal of another kind must not be read as this one")
_, err = principalOfKind("1000", ipcauth.KindGID)
assert.Error(t, err, "an untyped value is not a principal")
_, err = principalOfKind("gid:", ipcauth.KindGID)
assert.Error(t, err, "an empty value is not a principal")
}
+210
View File
@@ -0,0 +1,210 @@
//go:build !windows && !ios && !android
package cmd
import (
"fmt"
"net"
"os"
"path/filepath"
"strconv"
"syscall"
"github.com/netbirdio/netbird/client/internal/getent"
"github.com/netbirdio/netbird/client/internal/ipcauth"
)
// 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
ownerOnlySocketMode os.FileMode = 0600
)
// listenUnixPrivate binds a Unix socket at the narrowest mode the configuration
// allows, so it is never briefly more open than it should be. A Unix socket's
// mode is checked at connect() rather than at accept(), so a socket that is
// momentarily world-writable can be connected to before the daemon narrows it,
// and that caller stays connected afterwards.
//
// Where no group is configured the final mode is reached at the bind itself and
// nothing touches the path afterwards, which is what keeps the historical
// unrestricted socket free of a chmod that could follow a symlink another
// account planted. A restricted socket binds owner-only and applySocketAccess
// hands it to the group, under the checks that step carries.
//
// The umask is process-wide, so a file another goroutine creates during the
// bind would inherit it. That is why Start defers every asynchronous step until
// the listeners exist: at this point the daemon is still single-threaded, and
// the window is the bind call alone. A background task introduced above the
// listeners in Start would reopen this, which is what the note there is for.
func listenUnixPrivate(address string, allowed []string) (net.Listener, error) {
mode := openSocketMode
if len(allowed) > 0 {
mode = ownerOnlySocketMode
}
previous := syscall.Umask(int(^mode & 0o777))
listener, err := net.Listen("unix", address)
syscall.Umask(previous)
if err != nil {
return nil, err
}
return listener, nil
}
// resolveAllowGroup resolves one --allow-group value to a "gid:<id>"
// 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) (ipcauth.Principal, error) {
principal, typed, err := typedPrincipal(value, ipcauth.KindGID)
if err != nil {
return ipcauth.Principal{}, err
}
if typed {
return gidPrincipal(principal.Value)
}
if _, err := parseGID(value); err == nil {
return gidPrincipal(value)
}
group, err := getent.LookupGroupName(value)
if err != nil {
return ipcauth.Principal{}, 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 hands a socket to the configured group at 0660. It does
// nothing when no group is configured: listenUnixPrivate already bound such a
// socket at its final mode, and touching the path again would only add a chmod
// that could follow something another account put there.
//
// 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 {
return nil
}
// The listener just bound this path, so anything else standing there now is
// something another account substituted. Checked before either call below,
// neither of which should ever act on a name the daemon did not create.
if err := requireSocketFile(path); err != nil {
return err
}
principal, err := principalOfKind(principals[0], ipcauth.KindGID)
if err != nil {
return err
}
gid, err := parseGID(principal.Value)
if err != nil {
return err
}
if err := requireTrustedSocketDir(filepath.Dir(path)); err != nil {
return err
}
// Lchown, not Chown: chown follows symlinks, so a daemon running as root
// would otherwise hand an arbitrary target away to the configured group if
// the name were swapped between the check above and here.
if err := os.Lchown(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
}
// requireSocketFile reports whether path is the socket the listener created,
// without following a symlink standing in its place.
func requireSocketFile(path string) error {
info, err := os.Lstat(path)
if err != nil {
return fmt.Errorf("stat socket: %w", err)
}
if info.Mode()&os.ModeSocket == 0 {
return fmt.Errorf("%s is not a socket (mode %s), refusing to change its access", path, info.Mode())
}
return nil
}
// requireTrustedSocketDir refuses to apply a restriction inside a directory
// where another account could swap the socket for something else between the
// check and the change. That is only true of a directory some other account can
// write to: a sticky directory is fine, since only the owner of an entry may
// replace it there.
//
// The default locations are root-owned, so this rejects nothing an ordinary
// install does. It exists because the socket paths are configurable.
func requireTrustedSocketDir(dir string) error {
info, err := os.Stat(dir)
if err != nil {
return fmt.Errorf("stat socket directory: %w", err)
}
mode := info.Mode()
if mode.Perm()&0o022 != 0 && mode&os.ModeSticky == 0 {
return fmt.Errorf("socket directory %s is writable by other accounts (mode %s), refusing to restrict a socket that they can replace", dir, mode.Perm())
}
stat, ok := info.Sys().(*syscall.Stat_t)
if !ok {
return fmt.Errorf("cannot read ownership of socket directory %s", dir)
}
if stat.Uid != 0 && stat.Uid != uint32(os.Geteuid()) {
return fmt.Errorf("socket directory %s is owned by uid %d, which is neither root nor this daemon", dir, stat.Uid)
}
return nil
}
// gidPrincipal renders a GID as a principal in its canonical decimal form, so
// that spellings of the same group ("gid:01" and "gid:1") produce one principal
// rather than two that later look like a request to use two groups.
func gidPrincipal(gid string) (ipcauth.Principal, error) {
parsed, err := parseGID(gid)
if err != nil {
return ipcauth.Principal{}, err
}
principal, ok := ipcauth.ParsePrincipal(ipcauth.GIDPrincipal(uint32(parsed)))
if !ok {
return ipcauth.Principal{}, fmt.Errorf("build gid principal for %d", parsed)
}
return principal, nil
}
// unchangedGID is the value chown reads as "leave the group alone". A
// configured GID that lands on it would silently keep whatever group the socket
// already had, so it is rejected rather than applied.
const unchangedGID = 1<<32 - 1
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)
}
if gid == unchangedGID {
return 0, fmt.Errorf("gid %d is not a usable group", gid)
}
return int(gid), nil
}
+240
View File
@@ -0,0 +1,240 @@
//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"
"github.com/netbirdio/netbird/client/internal/ipcauth"
)
// 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, ipcauth.KindGID, principal.Kind)
assert.Equal(t, "gid:0", principal.String())
})
}
}
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.String())
}
// Spellings of the same GID must collapse to one principal, otherwise
// checkAllowGroupSet reads them as a request for two groups and refuses.
func TestResolveAllowGroups_CanonicalisesGIDs(t *testing.T) {
resolved, err := resolveAllowGroups([]string{"gid:01", "gid:1", "1"})
require.NoError(t, err)
assert.Equal(t, []string{"gid:1"}, resolved)
}
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"},
// chown reads this as "leave the group alone", so applying it would
// leave the socket on whatever group it already had.
{name: "the unchanged-gid sentinel", value: "gid:4294967295"},
}
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) {
// With nothing configured the socket already carries its final mode from
// the bind, so this must not touch the path at all: a chmod here is the one
// that could follow a symlink another account planted.
t.Run("no principals leaves the socket alone", func(t *testing.T) {
path := listenTestSocket(t)
before := socketMode(t, path)
require.NoError(t, applySocketAccess(path, nil))
assert.Equal(t, before, 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"}))
})
// A symlink standing where the listener put its socket is something another
// account substituted, and chowning it as root would hand its target away.
t.Run("a path that is not a socket is refused", func(t *testing.T) {
dir, err := os.MkdirTemp("", "nb-sock")
require.NoError(t, err)
t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) })
target := filepath.Join(dir, "target")
require.NoError(t, os.WriteFile(target, []byte("not a socket"), 0600))
link := filepath.Join(dir, "d.sock")
require.NoError(t, os.Symlink(target, link))
gid := strconv.Itoa(os.Getgid())
require.Error(t, applySocketAccess(link, []string{"gid:" + gid}))
require.NoError(t, applySocketAccess(link, nil), "with nothing configured there is nothing to apply")
// Either way the substituted target keeps the mode it was created with.
info, err := os.Stat(target)
require.NoError(t, err)
assert.Equal(t, os.FileMode(0600), info.Mode().Perm())
})
// Restricting a socket in a directory other accounts can write to is
// refused: they can replace the entry between the check and the change.
t.Run("an untrusted socket directory is refused", func(t *testing.T) {
dir, err := os.MkdirTemp("", "nb-sock")
require.NoError(t, err)
t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) })
require.NoError(t, os.Chmod(dir, 0777))
path := filepath.Join(dir, "d.sock")
listener, err := net.Listen("unix", path)
require.NoError(t, err)
t.Cleanup(func() { assert.NoError(t, listener.Close()) })
gid := strconv.Itoa(os.Getgid())
require.Error(t, applySocketAccess(path, []string{"gid:" + gid}))
// Leaving it unrestricted is still allowed: that is the historical
// behaviour and grants nothing the mode did not already grant.
assert.NoError(t, applySocketAccess(path, nil))
})
}
// The kernel checks a Unix socket's mode at connect(), not at accept(), so a
// socket that is briefly wider than intended can be connected to before the
// daemon narrows it, and that caller stays connected afterwards. The bind must
// therefore land on the final mode, whatever umask the service manager used.
func TestListenUnixPrivate_BindsAtTheFinalMode(t *testing.T) {
// A umask the daemon might have inherited from its service manager. Nonzero
// and not one of the masks under test, so it proves both that the bind mode
// does not depend on it and that it is put back afterwards.
const callerUmask = 0o027
tests := []struct {
name string
allowed []string
want os.FileMode
}{
{name: "unrestricted binds open, so nothing has to widen it later", want: 0666},
{name: "restricted binds owner-only, for applySocketAccess to hand to the group",
allowed: []string{"gid:0"}, want: 0600},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
previous := syscall.Umask(callerUmask)
t.Cleanup(func() { syscall.Umask(previous) })
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 := listenUnixPrivate(path, tc.allowed)
require.NoError(t, err)
t.Cleanup(func() { assert.NoError(t, listener.Close()) })
// Immediately after the bind, so nothing else can have moved it.
restored := syscall.Umask(callerUmask)
assert.Equal(t, callerUmask, restored, "listenUnixPrivate must restore the umask it changed")
assert.Equal(t, tc.want, socketMode(t, path))
})
}
}
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
}
+96
View File
@@ -0,0 +1,96 @@
//go:build windows
package cmd
import (
"fmt"
"net"
"golang.org/x/sys/windows"
"github.com/netbirdio/netbird/client/internal/ipcauth"
)
// resolveAllowGroup resolves one --allow-group value to a "sid:<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) (ipcauth.Principal, error) {
principal, typed, err := typedPrincipal(value, ipcauth.KindSID)
if err != nil {
return ipcauth.Principal{}, err
}
if typed {
return sidPrincipal(principal.Value)
}
if _, err := windows.StringToSid(value); err == nil {
return sidPrincipal(value)
}
sid, _, _, err := windows.LookupSID("", value)
if err != nil {
return ipcauth.Principal{}, fmt.Errorf("look up account: %w", err)
}
return sidPrincipal(sid.String())
}
// checkAllowGroupSet accepts any number of principals: a pipe descriptor holds
// one ACE per principal.
func checkAllowGroupSet([]string) error { return nil }
// applySocketAccess applies the restriction to a Unix socket, which on Windows
// it cannot: AF_UNIX sockets there carry no mode, and the daemon has no way to
// keep another local process off one. Serving it unrestricted would be the
// fail-open this flag exists to prevent, so a configured restriction is an
// error instead.
//
// The pipe transport is unaffected: its access lives in the security descriptor
// it is created with, and restrict never routes a named pipe here.
func applySocketAccess(path string, principals []string) error {
if len(principals) == 0 {
return nil
}
return fmt.Errorf("cannot restrict the unix socket %s on windows: it carries no access mode, serve the daemon on npipe:// instead", path)
}
// listenUnixPrivate binds a Unix socket. Windows has no umask and no mode on
// these sockets, so binding is all there is to do; a configured restriction is
// refused by applySocketAccess before the daemon serves.
func listenUnixPrivate(address string, _ []string) (net.Listener, error) {
return net.Listen("unix", address)
}
// 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 _, value := range principals {
principal, err := principalOfKind(value, ipcauth.KindSID)
if err != nil {
return "", err
}
sids = append(sids, principal.Value)
}
return ipcauth.RestrictedPipeSDDL(sids), nil
}
// sidPrincipal validates a SID and renders it in its canonical form, so that
// two spellings of the same SID produce one principal.
func sidPrincipal(value string) (ipcauth.Principal, error) {
sid, err := windows.StringToSid(value)
if err != nil {
return ipcauth.Principal{}, fmt.Errorf("parse SID %q: %w", value, err)
}
principal, ok := ipcauth.ParsePrincipal(ipcauth.SIDPrincipal(sid.String()))
if !ok {
return ipcauth.Principal{}, fmt.Errorf("build sid principal for %q", sid.String())
}
return principal, nil
}
@@ -0,0 +1,131 @@
//go:build windows
package cmd
import (
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/sys/windows"
"github.com/netbirdio/netbird/client/internal/ipcauth"
)
// 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, ipcauth.KindSID, principal.Kind)
assert.Equal(t, "sid:"+sidAdministrators, principal.String())
})
}
}
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.String())
}
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"}))
}
// A Unix socket on Windows carries no mode, so a restriction configured
// against one cannot be applied and must stop the daemon rather than leave the
// socket open to every local process.
func TestApplySocketAccess_UnixSocketCannotBeRestricted(t *testing.T) {
assert.NoError(t, applySocketAccess(`C:\ProgramData\Netbird\netbird.sock`, nil),
"an unrestricted unix socket is the historical behaviour and stays allowed")
err := applySocketAccess(`C:\ProgramData\Netbird\netbird.sock`, []string{testAllowGroupPrincipal})
require.Error(t, err)
assert.Contains(t, err.Error(), "npipe://", "the error should name the transport that can carry the restriction")
}
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 only that a restricted
// descriptor is accepted by ListenPipe and the pipe is created. Whether the
// descriptor actually denies an outside principal is not asserted here: that
// needs a second account and a connect attempt, so it is covered by the
// allowedPipeSDDL assertions above plus manual testing.
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)
}
+83 -22
View File
@@ -16,6 +16,7 @@ import (
"github.com/netbirdio/netbird/client/internal/daemonaddr"
"github.com/netbirdio/netbird/client/internal/ipcauth"
"github.com/netbirdio/netbird/client/mdm"
"github.com/netbirdio/netbird/client/proto"
"github.com/netbirdio/netbird/client/server"
"github.com/netbirdio/netbird/client/system"
@@ -64,9 +65,6 @@ func (p *program) Start(svc service.Service) error {
return err
}
// Collect static system and platform information
system.UpdateStaticInfoAsync()
// A daemon installed before named-pipe support has the loopback TCP address
// persisted. Move it to the named pipe so an upgraded daemon can identify
// its callers instead of silently serving an unauthenticated socket.
@@ -89,11 +87,22 @@ func (p *program) Start(svc service.Service) error {
)
p.serv = grpc.NewServer(opts...)
daemonListener, jsonListener, err := listenDaemonSockets()
daemonListener, jsonListener, err := p.listenRestricted()
if err != nil {
// Logged as well as returned: the service manager is the only other
// place this surfaces, and it reports a service that will not start
// without saying why. Refusing to serve on a host whose lockdown
// cannot be applied is deliberate, so the reason has to be findable.
log.Errorf("failed to apply the daemon socket restriction, not serving: %v", err)
return err
}
// Started only once the sockets exist. Binding them adjusts the process
// umask for the length of the bind, and a goroutine creating a file in that
// window would inherit it, so nothing asynchronous may be in flight before
// this point. Keep any future background work below the listeners too.
system.UpdateStaticInfoAsync()
go func() {
// Fatal here rather than inside serve, so serve's deferred listener
// closes run before the process exits.
@@ -104,12 +113,73 @@ func (p *program) Start(svc service.Service) error {
return nil
}
// listenRestricted opens the daemon sockets and applies the configured access
// restriction to them before returning, so no caller can reach a socket that is
// still open to everybody. Both listeners are closed again if the restriction
// cannot be applied, and the error is returned rather than handled later, so the
// service manager sees a start that failed instead of one that succeeded and
// then died.
//
// An unreadable MDM source is an error here for the same reason a bad value is:
// on a managed host it may be carrying the restriction, and treating it as
// absent would serve every local account instead.
func (p *program) listenRestricted() (*socketListener, *socketListener, error) {
// A nil fetcher leaves the platform-native source authoritative, which is
// what a desktop daemon wants. This runs before the Server exists, so it
// cannot borrow the Loader the Server owns.
policy, err := mdm.NewLoader(nil).LoadWithError()
if err != nil {
return nil, nil, err
}
allowed, source, err := daemonSocketPrincipals(policy)
if err != nil {
return nil, nil, err
}
if len(allowed) > 0 {
log.Infof("daemon sockets are restricted to %v by %s", allowed, source)
}
daemonListener, jsonListener, err := listenDaemonSockets(allowed)
if err != nil {
return nil, nil, err
}
if err := restrictListeners(daemonListener, jsonListener, allowed); err != nil {
closeListeners(daemonListener, jsonListener)
return nil, nil, err
}
return daemonListener, jsonListener, nil
}
// restrictListeners applies the access restriction to both sockets. restrict is
// a no-op for a nil listener, which is what a disabled JSON socket is.
func restrictListeners(daemonListener, jsonListener *socketListener, allowed []string) error {
if err := daemonListener.restrict("daemon", allowed); err != nil {
return err
}
return jsonListener.restrict("daemon JSON", allowed)
}
func closeListeners(listeners ...*socketListener) {
for _, l := range listeners {
if l == nil {
continue
}
if err := l.Close(); err != nil {
log.Debugf("close daemon listener: %v", err)
}
}
}
// 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 +189,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)
@@ -130,26 +200,17 @@ func listenDaemonSockets() (*socketListener, *socketListener, error) {
return daemonListener, jsonListener, nil
}
// serve brings up the daemon server on an already-open control socket and blocks
// 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.
// serve brings up the daemon server on listeners that are already open and
// already restricted, and blocks 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 {
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 {
log.Error(err)
return nil
}
if err := jsonListener.chmodUnixSocket("daemon JSON"); err != nil {
log.Error(err)
return nil
}
serverInstance := server.New(p.ctx, util.FindFirstLogPath(logFiles), configPath, profilesDisabled, updateSettingsDisabled, captureEnabled, networksDisabled)
p.authzGate.SetState(serverInstance)
if err := serverInstance.Start(); err != nil {
+16
View File
@@ -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)
+17
View File
@@ -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)
}
+19 -2
View File
@@ -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
+1 -1
View File
@@ -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")
}
+11 -6
View File
@@ -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)
+48 -7
View File
@@ -20,15 +20,29 @@ 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.
//
// A TCP address cannot express either, so a restriction configured against one
// is refused here, before anything is bound. Serving it anyway would leave the
// daemon reachable by anything that can open a socket to the port, on a host
// configured to be locked down.
func listenOnAddress(addr string, allowed []string) (*socketListener, error) {
network, address, err := parseListenAddress(addr)
if err != nil {
return nil, err
}
if network == "tcp" && len(allowed) > 0 {
return nil, fmt.Errorf("cannot restrict %s to %v: a tcp listener carries no local access control, use a unix socket or npipe://", addr, allowed)
}
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
@@ -36,6 +50,17 @@ func listenOnAddress(addr string) (*socketListener, error) {
if network == "unix" {
removeStaleUnixSocket(address)
// A Unix socket accepts connections the moment it is bound, and the
// kernel checks its mode at connect() rather than at accept(), so the
// socket is bound at the narrowest mode the configuration allows rather
// than bound wide and narrowed after: a caller that gets in during such
// a window stays connected once the mode changes.
listener, err := listenUnixPrivate(address, allowed)
if err != nil {
return nil, err
}
return &socketListener{Listener: listener, network: network, address: address}, nil
}
listener, err := net.Listen(network, address)
@@ -107,13 +132,29 @@ func removeStaleUnixSocketForAddress(addr string) {
removeStaleUnixSocket(address)
}
func (l *socketListener) chmodUnixSocket(description string) error {
if l == nil || l.network != "unix" {
// 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, which is what a
// disabled JSON socket is, and for a named pipe, which carries its access rules
// in the security descriptor it was created with.
//
// Any other transport that cannot express the restriction is an error rather
// than a socket served without one. listenOnAddress refuses the same
// combination before binding; this is the backstop that keeps a transport added
// later from silently inheriting the unrestricted path.
func (l *socketListener) restrict(description string, allowed []string) error {
if l == nil || l.network == "npipe" {
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 l.network != "unix" {
if len(allowed) > 0 {
return fmt.Errorf("cannot restrict the %s %s listener to %v", description, l.network, allowed)
}
return nil
}
if err := applySocketAccess(l.address, allowed); err != nil {
return fmt.Errorf("restrict %s socket %s: %w", description, l.address, err)
}
return nil
}