Refuse to serve when a configured socket restriction cannot be applied

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

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 err
-		return nil
 	}
 	if err := jsonListener.restrict("daemon JSON", allowed); err != nil {
-		log.Error(err)
+		return err
-		return nil
 	}
 
 	serverInstance := server.New(p.ctx, util.FindFirstLogPath(logFiles), configPath, profilesDisabled, updateSettingsDisabled, captureEnabled, networksDisabled)

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
+// out of --allow-group. It is a no-op for a nil listener, which is what a
-// is not a Unix socket: a named pipe carries its access rules in the security
+// disabled JSON socket is, and for a named pipe, which carries its access rules
-// descriptor it was created with.
+// 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
 	}