diff --git a/client/cmd/service_allow_group_test.go b/client/cmd/service_allow_group_test.go index c90d53704..9f0479761 100644 --- a/client/cmd/service_allow_group_test.go +++ b/client/cmd/service_allow_group_test.go @@ -100,6 +100,36 @@ func TestDaemonSocketPrincipals(t *testing.T) { }) } +// 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})) + }) +} + func TestCutKind(t *testing.T) { tests := []struct { value string diff --git a/client/cmd/service_controller.go b/client/cmd/service_controller.go index bd112494c..1e35287e3 100644 --- a/client/cmd/service_controller.go +++ b/client/cmd/service_controller.go @@ -155,14 +155,17 @@ func (p *program) serve(daemonListener, jsonListener *socketListener, allowed [] defer jsonListener.Close() } - // restrict is a no-op for a nil listener and for a non-unix one. + // Returned rather than logged: a socket whose access could not be set is + // either open to accounts that must not reach the daemon, or unreachable by + // the ones that must. Both are worse than the caller's fatal exit, and + // swallowing this would leave a service the manager still reports as running + // with no usable socket. restrict is a no-op for a nil listener, which is + // what a disabled JSON socket is. if err := daemonListener.restrict("daemon", allowed); err != nil { - log.Error(err) - return nil + return err } if err := jsonListener.restrict("daemon JSON", allowed); err != nil { - log.Error(err) - return nil + return err } serverInstance := server.New(p.ctx, util.FindFirstLogPath(logFiles), configPath, profilesDisabled, updateSettingsDisabled, captureEnabled, networksDisabled) diff --git a/client/cmd/service_socket.go b/client/cmd/service_socket.go index 0d97ebd2d..bf1393afe 100644 --- a/client/cmd/service_socket.go +++ b/client/cmd/service_socket.go @@ -25,12 +25,21 @@ type socketListener struct { // 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, allowed) //nolint:staticcheck if err != nil { //nolint:staticcheck // always errors on non-Windows builds @@ -113,11 +122,23 @@ func removeStaleUnixSocketForAddress(addr string) { } // 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. +// 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 != "unix" { + if l == nil || l.network == "npipe" { + return nil + } + + 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 }