Browse Source

Serve every command on the control socket, with no profiling filter

The filter the control socket applied kept back the profiling commands,
which have since left nebula; what remained was a field nothing set, a
refusal naming a surface that no longer runs anything, and a copy of the
registry per request. The info API keeps Filter, for its identity set.

- Command.ForProfiling, servedHere and notServedHere go; the server serves its registry as it is
- the registry's Filter tests withhold by name, and the two the surface test already covered go
- TestLocalSocketSurface pins the served list alone

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Matt Richardson 22 hours ago
parent
commit
9339e2caef
5 changed files with 14 additions and 98 deletions
  1. 0 2
      commands/command.go
  2. 6 33
      commands/registry_test.go
  3. 3 30
      localapi/server.go
  4. 0 18
      localapi/server_test.go
  5. 5 15
      ssh_commands_test.go

+ 0 - 2
commands/command.go

@@ -34,8 +34,6 @@ type Command struct {
 	ShortDescription string
 	Help             string
 
-	ForProfiling bool
-
 	// Identity declares that the command reports only what a certificate says about a
 	// host: its name, addresses, groups and validity. Nothing operational: no underlay
 	// addresses, no configuration, nothing a command could change. The info API, which

+ 6 - 33
commands/registry_test.go

@@ -14,11 +14,11 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// notWithheld is a stand in for a surface's own policy; these tests use ForProfiling
-// because it is the only such flag today, not because the registry knows about it.
-func notWithheld(c *Command) bool { return !c.ForProfiling }
+// notWithheld is a stand in for a surface's own policy, which the registry knows nothing
+// of; see Command.Identity for the one the info API applies.
+func notWithheld(c *Command) bool { return c.Name != "withheld" }
 
-const testReason = "this surface does not run profiling commands"
+const testReason = "this surface does not run commands that write"
 
 // textLine is a Text renderer for test commands whose result is one string.
 func textLine(v any, w io.Writer) error {
@@ -39,7 +39,6 @@ func testRegistry() *Registry {
 	r.Register(&Command{
 		Name:             "withheld",
 		ShortDescription: "a command this surface withholds",
-		ForProfiling:     true,
 		Run: func(fs any, a []string) (Result, error) {
 			return Value("wrote"), nil
 		},
@@ -48,9 +47,8 @@ func testRegistry() *Registry {
 	return r
 }
 
-// TestLocalSocketServesEverythingElse: the surface serves commands by default and
-// withholds only those that write files on the host.
-func TestLocalSocketServesEverythingElse(t *testing.T) {
+// TestFilterKeepsWhatKeepSays: the copy holds the commands keep accepted, and help.
+func TestFilterKeepsWhatKeepSays(t *testing.T) {
 	ro := testRegistry().Filter(notWithheld, testReason)
 
 	names := make([]string, 0)
@@ -61,31 +59,6 @@ func TestLocalSocketServesEverythingElse(t *testing.T) {
 	assert.ElementsMatch(t, []string{"help", "served"}, names)
 }
 
-func TestUnannotatedCommandIsServed(t *testing.T) {
-	r := New()
-	r.Register(&Command{
-		Name:             "plain",
-		ShortDescription: "a command with no annotations",
-		Run:              func(fs any, a []string) (Result, error) { return Nothing(), nil },
-	})
-
-	cmd := r.Filter(notWithheld, testReason).Get("plain")
-	assert.NotNil(t, cmd, "commands reach the local socket unless they say otherwise")
-}
-
-func TestProfilingCommandIsWithheld(t *testing.T) {
-	r := New()
-	r.Register(&Command{
-		Name:             "save-a-profile",
-		ShortDescription: "drives the profiler",
-		ForProfiling:     true,
-		Run:              func(fs any, a []string) (Result, error) { return Nothing(), nil },
-	})
-
-	cmd := r.Filter(notWithheld, testReason).Get("save-a-profile")
-	assert.Nil(t, cmd, "a profiling command must not reach the local socket")
-}
-
 // TestRefusesWithheldByName covers a client that knows a command exists and asks for it
 // anyway. The filter lives on the serving side, so the answer must not depend on what
 // the client was told was available.

+ 3 - 30
localapi/server.go

@@ -61,28 +61,8 @@ type Server struct {
 	trustSocket atomic.Bool
 }
 
-// servedHere decides what this surface will run.
-//
-// Everything except the profiling commands, which stay on the ssh debug server. Profile
-// output is written on the host running nebula, under sshd.sandbox_dir, so a path typed
-// here would not mean what the person typing it expects; and the profiling commands are
-// one workflow, so splitting them would leave someone able to start a CPU profile in one
-// place and stop it only in another.
-func servedHere(c *commands.Command) bool {
-	return !c.ForProfiling
-}
-
-// notServedHere explains a refusal in terms an operator can act on, naming the surface
-// that will run the command instead.
-const notServedHere = "profiling is driven from the ssh debug server, where the sshd.sandbox_dir " +
-	"setting governs where profile output is written on the host running nebula"
-
-// NewServer returns a Server that will serve the commands in reg that this surface
-// accepts.
-//
-// reg is kept rather than filtered here, so a command registered on it after the server
-// is built is served too. Filtering happens per request, which is how the ssh server
-// treats the same registry, cloning it per session.
+// NewServer returns a Server that will serve every command in reg, including one
+// registered on it after the server is built.
 func NewServer(l *slog.Logger, reg *commands.Registry) *Server {
 	return &Server{
 		endpoint:  endpoint{l: l, name: "local control socket"},
@@ -123,13 +103,6 @@ func (s *Server) Rebind(ctx context.Context, path string, acc Access) error {
 	return err
 }
 
-// registry returns the commands this surface serves, filtered from the source at the
-// time of the call. It is a few allocations per request, which a control socket that
-// sees a request every few seconds at most never notices.
-func (s *Server) registry() *commands.Registry {
-	return s.src.Filter(servedHere, notServedHere)
-}
-
 // semaphore returns the connection limiter, creating it on first use. It is made once
 // and never replaced: a handler releases into the channel it acquired from, so swapping
 // in a fresh one across a Stop and Rebind would leave any handler still in flight
@@ -274,7 +247,7 @@ func (s *Server) serve(req *Request) *Response {
 	defer slow.Stop()
 
 	res := &Response{Version: ProtocolVersion}
-	reg := s.registry()
+	reg := s.src
 
 	if req.Describe {
 		schema, err := json.Marshal(reg.Schema())

+ 0 - 18
localapi/server_test.go

@@ -68,14 +68,6 @@ func echoCommand() *commands.Command {
 func basicRegistry() *commands.Registry {
 	reg := commands.New()
 	reg.Register(echoCommand())
-	reg.Register(&commands.Command{
-		Name:             "withheld",
-		ShortDescription: "a command this surface withholds",
-		ForProfiling:     true,
-		Run: func(fs any, a []string) (commands.Result, error) {
-			return commands.Value("mutated"), nil
-		},
-	})
 	reg.Register(&commands.Command{
 		Name:             "boom",
 		ShortDescription: "panics",
@@ -119,16 +111,6 @@ func TestServeAllowedCommand(t *testing.T) {
 	assert.Empty(t, errOut, "a command that worked reports no error")
 }
 
-// TestWithheldCommandIsRefused checks the filter at the wire: the daemon refuses even
-// though the client named the command directly.
-func TestWithheldCommandIsRefused(t *testing.T) {
-	_, path := serve(t, basicRegistry())
-
-	out, errOut := run(t, path, "withheld")
-	assert.Empty(t, out)
-	assert.Contains(t, errOut, "withheld is not available on this surface")
-}
-
 // TestPanicInCommandDoesNotKillTheDaemon is ADR 003's requirement: a broken command
 // costs a connection, not the overlay.
 func TestPanicInCommandDoesNotKillTheDaemon(t *testing.T) {

+ 5 - 15
ssh_commands_test.go

@@ -71,24 +71,14 @@ func joinLines(in []string) string {
 }
 
 // TestLocalSocketSurface pins which of nebula's commands the always-on local socket
-// serves. Commands are served by default, so this test is what turns "somebody forgot to
-// think about it" into a visible decision: adding a command fails here until the lists
-// below say where it belongs.
+// serves: every one registered. Adding a command fails here until the list below says
+// so, which turns "somebody forgot to think about it" into a visible decision.
 func TestLocalSocketSurface(t *testing.T) {
-	reg := registeredCommands(t)
-
-	var served, withheld []commands.Name
-	for _, c := range reg.All() {
-		if c.ForProfiling {
-			withheld = append(withheld, c.Name)
-		} else {
-			served = append(served, c.Name)
-		}
+	var served []commands.Name
+	for _, c := range registeredCommands(t).All() {
+		served = append(served, c.Name)
 	}
 
-	// Nothing is withheld today: the profiling commands, which were, are gone.
-	assert.Empty(t, withheld)
-
 	assert.ElementsMatch(t, []commands.Name{
 		commands.Help,
 		commands.Identity,