From 8a4bd8b10f248ae4e15a19d6842dbbc54e656c7a Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Mon, 19 Oct 2026 10:48:26 -0400 Subject: [PATCH] Report ssh command failures to the surface (#1959) Report ssh command failures to the surface (#1959) The ssh debug commands told the user when something went wrong but never told the surface that ran them, so `ssh host cmd` always exited 0 and a few failures were not reported at all. A failed command now returns its error, a failure the user has already heard about is a commands.Failure, and the ssh exec path exits 1 for either. The command bodies also lose their ssh prefixes and the duplication they carried over from ssh.go; both goldens pin that nothing else changed. - Dispatch returns the callback's error, or shlex's; the ssh session logs it at debug, and not at all for a Failure the user already saw - commands.Fail writes a message and returns it as a Failure in one step; an unknown command, `help` for a command that is not there, a failed reload and an unbalanced quote are failures - ssh exec exit-status is 1 for a command, flag or failure error, 0 otherwise; the e2e golden records the exit status for every case - Clone rebinds help to the copy, so an ssh session's help lists logout and `help logout` and `logout -h` answer; sshd.LogoutCommand is the session's logout, exported for tests - "did not understand" names the first word only, the one looked up; DispatchArgs takes just the tokens - log-format writes its refusal to the user, as log-level does - print-relays emits the relay error as a string and omits the key when there is none, where it printed {} or "Error":null - list-hostmap and list-lighthouse-addrmap -json return the encode error; print-cert reports a certificate that will not marshal - a flags type that is not the one registered is an internal error, not a silent success - command functions and flag types are Command and Flags; parseVpnAddr and listHostMapFlagSet replace the repeated parse block and flag set; close-tunnel sends through sendCloseTunnel Co-Authored-By: Claude Fable 5.1 --- commands.go | 328 ++++++++++++---------------- commands/command.go | 29 ++- commands/registry.go | 44 ++-- commands/registry_test.go | 97 +++++++- commands_test.go | 67 ++++++ control.go | 3 + e2e/commands_output_test.go | 26 ++- e2e/testdata/commands_output.golden | 177 +++++++++++++++ ssh_reload_test.go | 29 ++- sshd/session.go | 45 +++- sshd_reload_test.go | 42 +++- 11 files changed, 641 insertions(+), 246 deletions(-) create mode 100644 commands_test.go diff --git a/commands.go b/commands.go index 2c859e97..67a783cb 100644 --- a/commands.go +++ b/commands.go @@ -3,7 +3,6 @@ package nebula import ( "bytes" "encoding/json" - "errors" "flag" "fmt" "log/slog" @@ -14,42 +13,51 @@ import ( "github.com/slackhq/nebula/commands" "github.com/slackhq/nebula/config" - "github.com/slackhq/nebula/header" "github.com/slackhq/nebula/logging" ) // The commands nebula serves. attachCommands builds the one registry every surface // reads. See the commands package for what a Command is. -type sshListHostMapFlags struct { +type listHostMapFlags struct { Json bool Pretty bool ByIndex bool } -type sshPrintCertFlags struct { +// listHostMapFlagSet is the flag set list-hostmap and list-pending-hostmap share. +func listHostMapFlagSet() (*flag.FlagSet, any) { + fl := flag.NewFlagSet("", flag.ContinueOnError) + s := listHostMapFlags{} + fl.BoolVar(&s.Json, "json", false, "outputs as json with more information") + fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json") + fl.BoolVar(&s.ByIndex, "by-index", false, "gets all hosts in the hostmap from the index table") + return fl, &s +} + +type printCertFlags struct { Json bool Pretty bool Raw bool } -type sshPrintTunnelFlags struct { +type printTunnelFlags struct { Pretty bool } -type sshChangeRemoteFlags struct { +type changeRemoteFlags struct { Address string } -type sshCloseTunnelFlags struct { +type closeTunnelFlags struct { LocalOnly bool } -type sshCreateTunnelFlags struct { +type createTunnelFlags struct { Address string } -type sshDeviceInfoFlags struct { +type deviceInfoFlags struct { Json bool Pretty bool } @@ -58,32 +66,18 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte reg.Register(&commands.Command{ Name: commands.ListHostmap, ShortDescription: "List all known previously connected hosts", - Flags: func() (*flag.FlagSet, any) { - fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshListHostMapFlags{} - fl.BoolVar(&s.Json, "json", false, "outputs as json with more information") - fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json") - fl.BoolVar(&s.ByIndex, "by-index", false, "gets all hosts in the hostmap from the index table") - return fl, &s - }, + Flags: listHostMapFlagSet, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshListHostMap(f.hostMap, fs, w) + return listHostMapCommand(f.hostMap, fs, w) }, }) reg.Register(&commands.Command{ Name: commands.ListPendingHostmap, ShortDescription: "List all handshaking hosts", - Flags: func() (*flag.FlagSet, any) { - fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshListHostMapFlags{} - fl.BoolVar(&s.Json, "json", false, "outputs as json with more information") - fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json") - fl.BoolVar(&s.ByIndex, "by-index", false, "gets all hosts in the hostmap from the index table") - return fl, &s - }, + Flags: listHostMapFlagSet, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshListHostMap(f.handshakeManager, fs, w) + return listHostMapCommand(f.handshakeManager, fs, w) }, }) @@ -92,13 +86,13 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "List all lighthouse map entries", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshListHostMapFlags{} + s := listHostMapFlags{} fl.BoolVar(&s.Json, "json", false, "outputs as json with more information") fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshListLighthouseMap(f.lightHouse, fs, w) + return listLighthouseAddrmapCommand(f.lightHouse, fs, w) }, }) @@ -106,7 +100,7 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte Name: commands.Reload, ShortDescription: "Reloads configuration from disk, same as sending HUP to the process", Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshReload(l, c, w) + return reloadCommand(l, c, w) }, }) @@ -114,7 +108,7 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte Name: commands.LogLevel, ShortDescription: "Gets or sets the current log level", Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshLogLevel(l, fs, a, w) + return logLevelCommand(l, fs, a, w) }, }) @@ -122,7 +116,7 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte Name: commands.LogFormat, ShortDescription: "Gets or sets the current log format", Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshLogFormat(l, fs, a, w) + return logFormatCommand(l, fs, a, w) }, }) @@ -130,7 +124,7 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte Name: commands.Version, ShortDescription: "Prints the currently running version of nebula", Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshVersion(f, fs, a, w) + return versionCommand(f, fs, a, w) }, }) @@ -139,13 +133,13 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Prints information about the network device.", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshDeviceInfoFlags{} + s := deviceInfoFlags{} fl.BoolVar(&s.Json, "json", false, "outputs as json with more information") fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshDeviceInfo(f, fs, w) + return deviceInfoCommand(f, fs, w) }, }) @@ -154,14 +148,14 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Prints the current certificate being used or the certificate for the provided vpn addr", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshPrintCertFlags{} + s := printCertFlags{} fl.BoolVar(&s.Json, "json", false, "outputs as json") fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json, assumes -json") fl.BoolVar(&s.Raw, "raw", false, "raw prints the PEM encoded certificate, not compatible with -json or -pretty") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshPrintCert(f, fs, a, w) + return printCertCommand(f, fs, a, w) }, }) @@ -170,12 +164,12 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Prints json details about a tunnel for the provided vpn addr", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshPrintTunnelFlags{} + s := printTunnelFlags{} fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshPrintTunnel(f, fs, a, w) + return printTunnelCommand(f, fs, a, w) }, }) @@ -184,12 +178,12 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Prints json details about all relay info", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshPrintTunnelFlags{} + s := printTunnelFlags{} fl.BoolVar(&s.Pretty, "pretty", false, "pretty prints json") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshPrintRelays(f, fs, a, w) + return printRelaysCommand(f, fs, a, w) }, }) @@ -198,12 +192,12 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Changes the remote address used in the tunnel for the provided vpn addr", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshChangeRemoteFlags{} + s := changeRemoteFlags{} fl.StringVar(&s.Address, "address", "", "The new remote address, ip:port") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshChangeRemote(f, fs, a, w) + return changeRemoteCommand(f, fs, a, w) }, }) @@ -212,12 +206,12 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Closes a tunnel for the provided vpn addr", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshCloseTunnelFlags{} + s := closeTunnelFlags{} fl.BoolVar(&s.LocalOnly, "local-only", false, "Disables notifying the remote that the tunnel is shutting down") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshCloseTunnel(f, fs, a, w) + return closeTunnelCommand(f, fs, a, w) }, }) @@ -227,12 +221,12 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte Help: "The lighthouses will be queried for real addresses but you can provide one as well.", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - s := sshCreateTunnelFlags{} + s := createTunnelFlags{} fl.StringVar(&s.Address, "address", "", "Optionally provide a real remote address, ip:port ") return fl, &s }, Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshCreateTunnel(f, fs, a, w) + return createTunnelCommand(f, fs, a, w) }, }) @@ -241,15 +235,15 @@ func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Inte ShortDescription: "Query the lighthouses for the provided vpn address", Help: "This command is asynchronous. Only currently known udp addresses will be printed.", Callback: func(fs any, a []string, w commands.StringWriter) error { - return sshQueryLighthouse(f, fs, a, w) + return queryLighthouseCommand(f, fs, a, w) }, }) } -func sshListHostMap(hl controlHostLister, a any, w commands.StringWriter) error { - fs, ok := a.(*sshListHostMapFlags) +func listHostMapCommand(hl controlHostLister, a any, w commands.StringWriter) error { + fs, ok := a.(*listHostMapFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *listHostMapFlags but was %T", a) } var hm []ControlHostInfo @@ -269,9 +263,8 @@ func sshListHostMap(hl controlHostLister, a any, w commands.StringWriter) error js.SetIndent("", " ") } - err := js.Encode(hm) - if err != nil { - return nil + if err := js.Encode(hm); err != nil { + return err } } else { @@ -286,10 +279,10 @@ func sshListHostMap(hl controlHostLister, a any, w commands.StringWriter) error return nil } -func sshListLighthouseMap(lightHouse *LightHouse, a any, w commands.StringWriter) error { - fs, ok := a.(*sshListHostMapFlags) +func listLighthouseAddrmapCommand(lightHouse *LightHouse, a any, w commands.StringWriter) error { + fs, ok := a.(*listHostMapFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *listHostMapFlags but was %T", a) } type lighthouseInfo struct { @@ -319,9 +312,8 @@ func sshListLighthouseMap(lightHouse *LightHouse, a any, w commands.StringWriter js.SetIndent("", " ") } - err := js.Encode(addrMap) - if err != nil { - return nil + if err := js.Encode(addrMap); err != nil { + return err } } else { @@ -340,22 +332,30 @@ func sshListLighthouseMap(lightHouse *LightHouse, a any, w commands.StringWriter return nil } -func sshVersion(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - return w.WriteLine(fmt.Sprintf("%s", ifce.version)) -} - -func sshQueryLighthouse(ifce *Interface, fs any, a []string, w commands.StringWriter) error { +// parseVpnAddr parses the vpn address a command takes as its first operand, failing to w +// when there is none or it does not parse. noun is how the command's messages name it, +// "vpn address" or "vpn addr", which the output golden pins per command. +func parseVpnAddr(a []string, noun string, w commands.StringWriter) (netip.Addr, error) { if len(a) == 0 { - return w.WriteLine("No vpn address was provided") + return netip.Addr{}, commands.Fail(w, "No vpn address was provided") } vpnAddr, err := netip.ParseAddr(a[0]) if err != nil { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) + return netip.Addr{}, commands.Fail(w, fmt.Sprintf("The provided %s could not be parsed: %s", noun, a[0])) } - if !vpnAddr.IsValid() { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) + return vpnAddr, nil +} + +func versionCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + return w.WriteLine(ifce.version) +} + +func queryLighthouseCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + vpnAddr, err := parseVpnAddr(a, "vpn address", w) + if err != nil { + return err } var cm *CacheMap @@ -366,80 +366,56 @@ func sshQueryLighthouse(ifce *Interface, fs any, a []string, w commands.StringWr return json.NewEncoder(w.GetWriter()).Encode(cm) } -func sshCloseTunnel(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - flags, ok := fs.(*sshCloseTunnelFlags) +func closeTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + flags, ok := fs.(*closeTunnelFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *closeTunnelFlags but was %T", fs) } - if len(a) == 0 { - return w.WriteLine("No vpn address was provided") - } - - vpnAddr, err := netip.ParseAddr(a[0]) + vpnAddr, err := parseVpnAddr(a, "vpn address", w) if err != nil { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) - } - - if !vpnAddr.IsValid() { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) + return err } hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) if hostInfo == nil { - return w.WriteLine(fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0])) + return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0])) } if !flags.LocalOnly { - ifce.send( - header.CloseTunnel, - 0, - hostInfo.ConnectionState, - hostInfo, - []byte{}, - make([]byte, 12, 12), - make([]byte, mtu), - ) + ifce.sendCloseTunnel(hostInfo) } ifce.closeTunnel(hostInfo) return w.WriteLine("Closed") } -func sshCreateTunnel(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - flags, ok := fs.(*sshCreateTunnelFlags) +func createTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + flags, ok := fs.(*createTunnelFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *createTunnelFlags but was %T", fs) } - if len(a) == 0 { - return w.WriteLine("No vpn address was provided") - } - - vpnAddr, err := netip.ParseAddr(a[0]) + vpnAddr, err := parseVpnAddr(a, "vpn address", w) if err != nil { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) - } - - if !vpnAddr.IsValid() { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) + return err } hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) if hostInfo != nil { - return w.WriteLine(fmt.Sprintf("Tunnel already exists")) + return commands.Fail(w, "Tunnel already exists") } hostInfo = ifce.handshakeManager.QueryVpnAddr(vpnAddr) if hostInfo != nil { - return w.WriteLine(fmt.Sprintf("Tunnel already handshaking")) + return commands.Fail(w, "Tunnel already handshaking") } var addr netip.AddrPort if flags.Address != "" { addr, err = netip.ParseAddrPort(flags.Address) if err != nil { - return w.WriteLine("Address could not be parsed") + return commands.Fail(w, "Address could not be parsed") } } @@ -455,50 +431,42 @@ func sshCreateTunnel(ifce *Interface, fs any, a []string, w commands.StringWrite return w.WriteLine("Created") } -func sshChangeRemote(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - flags, ok := fs.(*sshChangeRemoteFlags) +func changeRemoteCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + flags, ok := fs.(*changeRemoteFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *changeRemoteFlags but was %T", fs) } - if len(a) == 0 { - return w.WriteLine("No vpn address was provided") + vpnAddr, err := parseVpnAddr(a, "vpn address", w) + if err != nil { + return err } if flags.Address == "" { - return w.WriteLine("No address was provided") + return commands.Fail(w, "No address was provided") } addr, err := netip.ParseAddrPort(flags.Address) if err != nil { - return w.WriteLine("Address could not be parsed") - } - - vpnAddr, err := netip.ParseAddr(a[0]) - if err != nil { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) - } - - if !vpnAddr.IsValid() { - return w.WriteLine(fmt.Sprintf("The provided vpn address could not be parsed: %s", a[0])) + return commands.Fail(w, "Address could not be parsed") } hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) if hostInfo == nil { - return w.WriteLine(fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0])) + return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0])) } hostInfo.SetRemote(addr) return w.WriteLine("Changed") } -func sshLogLevel(l *slog.Logger, fs any, a []string, w commands.StringWriter) error { +func logLevelCommand(l *slog.Logger, fs any, a []string, w commands.StringWriter) error { ctrl, ok := l.Handler().(interface { GetLevel() slog.Level SetLevel(slog.Level) }) if !ok { - return w.WriteLine("Log level is not reconfigurable on this logger") + return commands.Fail(w, "Log level is not reconfigurable on this logger") } if len(a) == 0 { @@ -507,20 +475,20 @@ func sshLogLevel(l *slog.Logger, fs any, a []string, w commands.StringWriter) er level, err := logging.ParseLevel(strings.ToLower(a[0])) if err != nil { - return w.WriteLine(fmt.Sprintf("Unknown log level %s. Possible log levels: trace, debug, info, warn, error", a)) + return commands.Fail(w, fmt.Sprintf("Unknown log level %s. Possible log levels: trace, debug, info, warn, error", a)) } ctrl.SetLevel(level) return w.WriteLine(fmt.Sprintf("Log level is: %s", logging.LevelName(ctrl.GetLevel()))) } -func sshLogFormat(l *slog.Logger, fs any, a []string, w commands.StringWriter) error { +func logFormatCommand(l *slog.Logger, fs any, a []string, w commands.StringWriter) error { ctrl, ok := l.Handler().(interface { GetFormat() string SetFormat(string) error }) if !ok { - return w.WriteLine("Log format is not reconfigurable on this logger") + return commands.Fail(w, "Log format is not reconfigurable on this logger") } if len(a) == 0 { @@ -528,31 +496,27 @@ func sshLogFormat(l *slog.Logger, fs any, a []string, w commands.StringWriter) e } if err := ctrl.SetFormat(strings.ToLower(a[0])); err != nil { - return err + return commands.Fail(w, fmt.Sprintf("Unable to set log format: %s", err)) } return w.WriteLine(fmt.Sprintf("Log format is: %s", ctrl.GetFormat())) } -func sshPrintCert(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - args, ok := fs.(*sshPrintCertFlags) +func printCertCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + args, ok := fs.(*printCertFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *printCertFlags but was %T", fs) } cert := ifce.pki.getCertState().GetDefaultCertificate() if len(a) > 0 { - vpnAddr, err := netip.ParseAddr(a[0]) + vpnAddr, err := parseVpnAddr(a, "vpn addr", w) if err != nil { - return w.WriteLine(fmt.Sprintf("The provided vpn addr could not be parsed: %s", a[0])) - } - - if !vpnAddr.IsValid() { - return w.WriteLine(fmt.Sprintf("The provided vpn addr could not be parsed: %s", a[0])) + return err } hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) if hostInfo == nil { - return w.WriteLine(fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0])) + return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0])) } cert = hostInfo.GetCert().Certificate @@ -561,16 +525,15 @@ func sshPrintCert(ifce *Interface, fs any, a []string, w commands.StringWriter) if args.Json || args.Pretty { b, err := cert.MarshalJSON() if err != nil { - return nil + return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err)) } if args.Pretty { buf := new(bytes.Buffer) - err := json.Indent(buf, b, "", " ") - b = buf.Bytes() - if err != nil { - return nil + if err := json.Indent(buf, b, "", " "); err != nil { + return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err)) } + b = buf.Bytes() } return w.WriteBytes(b) @@ -579,7 +542,7 @@ func sshPrintCert(ifce *Interface, fs any, a []string, w commands.StringWriter) if args.Raw { b, err := cert.MarshalPEM() if err != nil { - return nil + return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err)) } return w.WriteBytes(b) @@ -588,20 +551,18 @@ func sshPrintCert(ifce *Interface, fs any, a []string, w commands.StringWriter) return w.WriteLine(cert.String()) } -func sshPrintRelays(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - args, ok := fs.(*sshPrintTunnelFlags) +func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + args, ok := fs.(*printTunnelFlags) if !ok { - w.WriteLine(fmt.Sprintf("sshPrintRelays failed to convert args type")) - return nil + return fmt.Errorf("internal error: expected *printTunnelFlags but was %T", fs) } - relays := map[uint32]*HostInfo{} - ifce.hostMap.Lock() - maps.Copy(relays, ifce.hostMap.Relays) - ifce.hostMap.Unlock() + ifce.hostMap.RLock() + relays := maps.Clone(ifce.hostMap.Relays) + ifce.hostMap.RUnlock() type RelayFor struct { - Error error + Error string `json:",omitempty"` Type string State string PeerAddr netip.Addr @@ -632,11 +593,11 @@ func sshPrintRelays(ifce *Interface, fs any, a []string, w commands.StringWriter co.Relays = append(co.Relays, &ro) relayHI := ifce.hostMap.QueryVpnAddr(v.vpnAddrs[0]) if relayHI == nil { - ro.RelayForAddrs = append(ro.RelayForAddrs, RelayFor{Error: errors.New("could not find hostinfo")}) + ro.RelayForAddrs = append(ro.RelayForAddrs, RelayFor{Error: "could not find hostinfo"}) continue } for _, vpnAddr := range relayHI.relayState.CopyRelayForIps() { - rf := RelayFor{Error: nil} + rf := RelayFor{} r, ok := relayHI.relayState.GetRelayForByAddr(vpnAddr) if ok { t := "" @@ -665,7 +626,7 @@ func sshPrintRelays(ifce *Interface, fs any, a []string, w commands.StringWriter rf.Type = t rf.State = s if rf.LocalIndex != k { - rf.Error = fmt.Errorf("hostmap LocalIndex '%v' does not match RelayState LocalIndex", k) + rf.Error = fmt.Sprintf("hostmap LocalIndex '%v' does not match RelayState LocalIndex", k) } } relayedHI := ifce.hostMap.QueryVpnAddr(vpnAddr) @@ -676,35 +637,23 @@ func sshPrintRelays(ifce *Interface, fs any, a []string, w commands.StringWriter ro.RelayForAddrs = append(ro.RelayForAddrs, rf) } } - err := enc.Encode(co) - if err != nil { - return err - } - return nil + return enc.Encode(co) } -func sshPrintTunnel(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - args, ok := fs.(*sshPrintTunnelFlags) +func printTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { + args, ok := fs.(*printTunnelFlags) if !ok { - return nil + return fmt.Errorf("internal error: expected *printTunnelFlags but was %T", fs) } - if len(a) == 0 { - return w.WriteLine("No vpn address was provided") - } - - vpnAddr, err := netip.ParseAddr(a[0]) + vpnAddr, err := parseVpnAddr(a, "vpn addr", w) if err != nil { - return w.WriteLine(fmt.Sprintf("The provided vpn addr could not be parsed: %s", a[0])) - } - - if !vpnAddr.IsValid() { - return w.WriteLine(fmt.Sprintf("The provided vpn addr could not be parsed: %s", a[0])) + return err } hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) if hostInfo == nil { - return w.WriteLine(fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0])) + return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0])) } enc := json.NewEncoder(w.GetWriter()) @@ -715,7 +664,11 @@ func sshPrintTunnel(ifce *Interface, fs any, a []string, w commands.StringWriter return enc.Encode(copyHostInfo(hostInfo, ifce.hostMap.GetPreferredRanges())) } -func sshDeviceInfo(ifce *Interface, fs any, w commands.StringWriter) error { +func deviceInfoCommand(ifce *Interface, fs any, w commands.StringWriter) error { + flags, ok := fs.(*deviceInfoFlags) + if !ok { + return fmt.Errorf("internal error: expected flags to be deviceInfoFlags but was %+v", fs) + } data := struct { Name string `json:"name"` @@ -727,11 +680,6 @@ func sshDeviceInfo(ifce *Interface, fs any, w commands.StringWriter) error { copy(data.Cidr, ifce.inside.Networks()) - flags, ok := fs.(*sshDeviceInfoFlags) - if !ok { - return fmt.Errorf("internal error: expected flags to be sshDeviceInfoFlags but was %+v", fs) - } - if flags.Json || flags.Pretty { js := json.NewEncoder(w.GetWriter()) if flags.Pretty { @@ -744,13 +692,19 @@ func sshDeviceInfo(ifce *Interface, fs any, w commands.StringWriter) error { } } -// sshReload reloads the config and writes the result to the client. It reloads even if the client has gone away -func sshReload(l *slog.Logger, c *config.C, w commands.StringWriter) error { +// reloadCommand reloads the config and writes the result to the client. It reloads even if the client has gone away +func reloadCommand(l *slog.Logger, c *config.C, w commands.StringWriter) error { writeErr := w.WriteLine("Reloading config") res := c.ReloadConfig() res.Log(l) if err := w.WriteLine(res.String()); writeErr == nil { writeErr = err } - return writeErr + if writeErr != nil { + return writeErr + } + if res.Err != nil || res.Failed { + return &commands.Failure{Msg: res.String()} + } + return nil } diff --git a/commands/command.go b/commands/command.go index 3ff820d4..b849ad0e 100644 --- a/commands/command.go +++ b/commands/command.go @@ -9,12 +9,14 @@ import ( type CommandFlags func() (*flag.FlagSet, any) // CommandCallback is called when your command should execute. -// fs will be a a pointer to the struct provided by Command.Flags callback, if there was one. -h and -help are reserved +// fs will be a pointer to the struct provided by Command.Flags callback, if there was one. -h and -help are reserved // and handled automatically for you. // a will be any unconsumed arguments, if no Command.Flags was available this will be all the flags passed in. // w is the writer to use when sending messages back to the client. -// If an error is returned by the callback it is logged locally, the callback should handle messaging errors to the user -// where appropriate +// An error returned by the callback is handed back to whoever dispatched the command, which reports a failure: exit +// status 1 over ssh. The error is not shown to the user. For a failure the user caused or can act on, write the +// message and return it as a *Failure, which Fail does in one step; the surface then reports the failure without +// logging what the user was already told. Any other error is logged by the surface as well. type CommandCallback func(fs any, a []string, w StringWriter) error type Command struct { @@ -26,6 +28,27 @@ type Command struct { Callback CommandCallback } +// Failure is the error a command returns for a failure it has already explained to the user: +// the message it wrote is the whole story. A surface reports it as a failure and does not +// log it. +type Failure struct { + Msg string +} + +func (f *Failure) Error() string { + return f.Msg +} + +// Fail writes msg to w and returns it as a *Failure, so a command reports a failure to the +// user and to its surface in one line. If the write itself fails, that error is returned +// instead: the user was not told anything. +func Fail(w StringWriter, msg string) error { + if err := w.WriteLine(msg); err != nil { + return err + } + return &Failure{Msg: msg} +} + func execCommand(c *Command, args []string, w StringWriter) error { var ( fl *flag.FlagSet diff --git a/commands/registry.go b/commands/registry.go index aeeefebe..60b663ab 100644 --- a/commands/registry.go +++ b/commands/registry.go @@ -38,7 +38,8 @@ func (r *Registry) registerHelp() { }) } -// Register adds a command that can be run by a user. +// Register adds a command that can be run by a user. A command registered under a name +// already held replaces it; see Clone for what that means for Help. func (r *Registry) Register(c *Command) { r.mu.Lock() defer r.mu.Unlock() @@ -77,29 +78,32 @@ func (r *Registry) All() []*Command { return cmds } -// Clone returns a copy holding the same commands. +// Clone returns a copy holding the same commands, with help bound to the copy: a +// command registered on the clone alone is listed and described there, and one the +// clone withholds is not. That rebinding replaces whatever was registered under Help on +// the source, built-in or not. func (r *Registry) Clone() *Registry { r.mu.RLock() - defer r.mu.RUnlock() - return &Registry{commands: maps.Clone(r.commands)} + c := &Registry{commands: maps.Clone(r.commands)} + r.mu.RUnlock() + c.registerHelp() + return c } -// Dispatch parses a command line and runs it, reporting problems to w. -func (r *Registry) Dispatch(line string, w StringWriter) { - // A line shlex cannot split, an unbalanced quote, gets no answer. sshd was silent - // here too; this is a relocation, so it stays silent. +// Dispatch parses a command line and runs it, reporting problems to w. The error is for +// the caller to log, not to show; see CommandCallback. +func (r *Registry) Dispatch(line string, w StringWriter) error { + // A line shlex cannot split, an unbalanced quote. args, err := shlex.Split(line, true) if err != nil { - return + return Fail(w, "could not parse the command line: "+err.Error()) } - _ = r.DispatchArgs(args, line, w) + return r.DispatchArgs(args, w) } -// DispatchArgs runs an already tokenized command line. orig is what the user typed, -// used only for the "did not understand" message; callers that have no raw line may -// pass "" and the joined args are used instead. -func (r *Registry) DispatchArgs(args []string, orig string, w StringWriter) error { +// DispatchArgs runs an already tokenized command line. The error is as for Dispatch. +func (r *Registry) DispatchArgs(args []string, w StringWriter) error { if len(args) == 0 { r.dumpCommands(w) return nil @@ -107,16 +111,14 @@ func (r *Registry) DispatchArgs(args []string, orig string, w StringWriter) erro c := r.Get(Name(args[0])) if c == nil { - if orig == "" { - orig = strings.Join(args, " ") - } - - if err := w.WriteLine(fmt.Sprintf("did not understand: %s", orig)); err != nil { + // Only the first word was looked up, so only it is named. + msg := fmt.Sprintf("did not understand: %s", args[0]) + if err := w.WriteLine(msg); err != nil { return err } r.dumpCommands(w) - return nil + return &Failure{Msg: msg} } if checkHelpArgs(args) { @@ -154,7 +156,7 @@ func (r *Registry) help(a []string, w StringWriter) error { // We are printing a specific commands help text cmd := r.Get(Name(a[0])) if cmd == nil { - return w.WriteLine("Command not available " + a[0]) + return Fail(w, "Command not available "+a[0]) } if err := w.WriteLine(fmt.Sprintf("%s - %s", cmd.Name, cmd.ShortDescription)); err != nil { diff --git a/commands/registry_test.go b/commands/registry_test.go index ca788253..40ec0af7 100644 --- a/commands/registry_test.go +++ b/commands/registry_test.go @@ -2,6 +2,7 @@ package commands import ( "bytes" + "errors" "flag" "strings" "testing" @@ -45,18 +46,41 @@ func TestFlagUsageGoesToTheWriter(t *testing.T) { }) merged := &bytes.Buffer{} - require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, "", NewStringWriter(merged))) + require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, NewStringWriter(merged))) assert.Contains(t, merged.String(), "flag provided but not defined") } -// TestDispatchDropsUnparseableLine pins sshd's behaviour for a line shlex rejects: no -// answer at all. An operator with an unbalanced quote gets silence, as they always have. -func TestDispatchDropsUnparseableLine(t *testing.T) { +// TestDispatchReportsUnparseableLine: a line shlex rejects, an unbalanced quote, is a +// failure the user is told about. +func TestDispatchReportsUnparseableLine(t *testing.T) { out := &bytes.Buffer{} - testRegistry().Dispatch(`served "unterminated`, NewStringWriter(out)) - assert.Empty(t, out.String()) + err := testRegistry().Dispatch(`served "unterminated`, NewStringWriter(out)) + + var f *Failure + require.ErrorAs(t, err, &f) + assert.Equal(t, f.Msg+"\n", out.String()) + assert.Contains(t, out.String(), "could not parse the command line: ") } +// TestFailTellsTheUserAndTheSurface: Fail writes the message and returns it as a Failure; +// when the write fails, the user heard nothing, so the write error is what comes back. +func TestFailTellsTheUserAndTheSurface(t *testing.T) { + out := &bytes.Buffer{} + err := Fail(NewStringWriter(out), "no such tunnel") + var f *Failure + require.ErrorAs(t, err, &f) + assert.Equal(t, "no such tunnel", f.Msg) + assert.Equal(t, "no such tunnel\n", out.String()) + + err = Fail(NewStringWriter(failingWriter{}), "no such tunnel") + require.EqualError(t, err, "gone") + assert.NotErrorAs(t, err, &f) +} + +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("gone") } + // TestAllSortsByName: registration order does not matter. func TestAllSortsByName(t *testing.T) { r := New() @@ -72,8 +96,8 @@ func TestAllSortsByName(t *testing.T) { } // TestHelpFlagAgreesWithHelpCommand pins that -h answers through the registered help -// command. On a clone, help is bound to the source, so a command registered only on the -// clone is known to neither -h nor `help `, and the two never disagree. +// command, and that help on a clone describes the clone: a command registered only there +// is known to both -h and `help `, and one withheld from it is known to neither. func TestHelpFlagAgreesWithHelpCommand(t *testing.T) { r := testRegistry() c := r.Clone() @@ -88,10 +112,65 @@ func TestHelpFlagAgreesWithHelpCommand(t *testing.T) { cmd := &bytes.Buffer{} c.Dispatch("help logout", NewStringWriter(cmd)) assert.Equal(t, cmd.String(), flag.String()) - assert.Equal(t, "Command not available logout\n", flag.String()) + assert.Equal(t, "logout - ends the session\n", flag.String()) + + // The source never learns of it. + src := &bytes.Buffer{} + r.Dispatch("help logout", NewStringWriter(src)) + assert.Equal(t, "Command not available logout\n", src.String()) // And a command both hold is described either way. flag.Reset() c.Dispatch("served -h", NewStringWriter(flag)) assert.Equal(t, "served - a command this surface serves\n", flag.String()) } + +// TestCloneHelpListsTheClone: the command list help prints is the clone's, so a clone +// that withholds a command does not name it, and the source still does. +func TestCloneHelpListsTheClone(t *testing.T) { + r := testRegistry() + c := New() + c.Register(r.Get("served")) + c = c.Clone() + + out := &bytes.Buffer{} + c.Dispatch("help", NewStringWriter(out)) + assert.Equal(t, "Available commands:\n"+ + "help - prints available commands or help for specific usage info\n"+ + "served - a command this surface serves\n\n", out.String()) + + out.Reset() + c.Dispatch("help withheld", NewStringWriter(out)) + assert.Equal(t, "Command not available withheld\n", out.String()) + + out.Reset() + r.Dispatch("help withheld", NewStringWriter(out)) + assert.Equal(t, "withheld - a command this surface withholds\n", out.String()) +} + +// TestDispatchReturnsTheCommandsError: what the callback returns reaches the caller, so +// a surface can log it. +func TestDispatchReturnsTheCommandsError(t *testing.T) { + r := New() + r.Register(&Command{ + Name: "fails", + Callback: func(fs any, a []string, w StringWriter) error { + return errors.New("boom") + }, + }) + + out := &bytes.Buffer{} + require.EqualError(t, r.Dispatch("fails", NewStringWriter(out)), "boom") + assert.Empty(t, out.String(), "a command's own error is not shown; the command speaks for itself") +} + +// TestUnknownCommandNamesTheWord: only the first word is looked up, so only it is +// reported, followed by the command list, and the line is a failure to the surface. +func TestUnknownCommandNamesTheWord(t *testing.T) { + out := &bytes.Buffer{} + err := testRegistry().Dispatch(`nope "with args"`, NewStringWriter(out)) + var f *Failure + require.ErrorAs(t, err, &f) + assert.Equal(t, "did not understand: nope", f.Msg) + assert.True(t, strings.HasPrefix(out.String(), "did not understand: nope\nAvailable commands:\n"), out.String()) +} diff --git a/commands_test.go b/commands_test.go new file mode 100644 index 00000000..068a5080 --- /dev/null +++ b/commands_test.go @@ -0,0 +1,67 @@ +package nebula + +import ( + "bytes" + "io" + "net/netip" + "testing" + + "github.com/slackhq/nebula/commands" + "github.com/slackhq/nebula/logging" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestLogFormatRefusalIsShown: a format the logger does not know is reported to the user +// as a failure, rather than returned bare, which over ssh was silence. +func TestLogFormatRefusalIsShown(t *testing.T) { + l := logging.NewLogger(io.Discard) + out := &bytes.Buffer{} + w := commands.NewStringWriter(out) + + var f *commands.Failure + require.ErrorAs(t, logFormatCommand(l, nil, []string{"bogus"}, w), &f) + assert.Equal(t, "Unable to set log format: unknown log format `bogus`. possible formats: [text json]\n", out.String()) + + out.Reset() + require.NoError(t, logFormatCommand(l, nil, []string{"JSON"}, w)) + assert.Equal(t, "Log format is: json\n", out.String()) +} + +// TestPrintRelaysReportsErrorsAsText: the Error field was an error value, which encodes +// as {} and lost the message. It is the message now, and absent when there is none. +func TestPrintRelaysReportsErrorsAsText(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + relay := netip.MustParseAddr("10.0.0.1") + peer := netip.MustParseAddr("10.0.0.2") + + // A relay host that is in Relays but not in Hosts, so its hostinfo cannot be found. + hm.Relays[1] = &HostInfo{vpnAddrs: []netip.Addr{relay}} + ifce := &Interface{hostMap: hm} + + out := &bytes.Buffer{} + require.NoError(t, printRelaysCommand(ifce, &printTunnelFlags{}, nil, commands.NewStringWriter(out))) + assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Error":"could not find hostinfo","Type":"","State":"","PeerAddr":"","LocalIndex":0,"RemoteIndex":0,"RelayedThrough":null}]}]}`, out.String()) + + // The same relay host, found, relaying for a peer whose index matches: no Error key. + hi := hm.Relays[1] + r := &Relay{Type: ForwardingType, State: Established, PeerAddr: peer, LocalIndex: 1, RemoteIndex: 2} + hi.relayState = RelayState{ + relayForByAddr: map[netip.Addr]*Relay{peer: r}, + relayForByIdx: map[uint32]*Relay{r.LocalIndex: r}, + } + hm.Hosts[relay] = hi + + out.Reset() + require.NoError(t, printRelaysCommand(ifce, &printTunnelFlags{}, nil, commands.NewStringWriter(out))) + assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Type":"forwarding","State":"established","PeerAddr":"10.0.0.2","LocalIndex":1,"RemoteIndex":2,"RelayedThrough":null}]}]}`, out.String()) + + // And an index mismatch names it. + hm.Relays[7] = hi + delete(hm.Relays, 1) + out.Reset() + require.NoError(t, printRelaysCommand(ifce, &printTunnelFlags{}, nil, commands.NewStringWriter(out))) + assert.Contains(t, out.String(), `"Error":"hostmap LocalIndex '7' does not match RelayState LocalIndex"`) +} diff --git a/control.go b/control.go index 57cca794..f6bfdd1c 100644 --- a/control.go +++ b/control.go @@ -374,6 +374,9 @@ func (c *Control) CloseAllTunnels(excludeLighthouses bool) (closed int) { return } +// Commands returns the registry the daemon's surfaces serve from. It is the live +// registry, not a copy: a command registered on it is served to every ssh session that +// starts afterwards. Clone it to run commands without that reach. func (c *Control) Commands() *commands.Registry { return c.commands } diff --git a/e2e/commands_output_test.go b/e2e/commands_output_test.go index 331f4188..7f2ae288 100644 --- a/e2e/commands_output_test.go +++ b/e2e/commands_output_test.go @@ -14,6 +14,7 @@ import ( "github.com/slackhq/nebula/cert" "github.com/slackhq/nebula/cert_test" "github.com/slackhq/nebula/commands" + "github.com/slackhq/nebula/sshd" "github.com/slackhq/nebula/test" "github.com/stretchr/testify/require" ) @@ -64,16 +65,22 @@ func TestCommandOutput(t *testing.T) { control.Start() t.Cleanup(func() { control.Stop() }) + // As an ssh session does: a clone of the daemon's registry with logout added, which + // help on the clone lists and describes. There is no session to end here. reg := control.Commands().Clone() + reg.Register(sshd.LogoutCommand(func() {})) - run := func(args ...string) string { + // run dispatches as the ssh exec path does and returns the output and the error, which + // over ssh is exit status 1. + run := func(args ...string) (string, error) { buf := &bytes.Buffer{} - _ = reg.DispatchArgs(args, strings.Join(args, " "), commands.NewStringWriter(buf)) - return buf.String() + err := reg.DispatchArgs(args, commands.NewStringWriter(buf)) + return buf.String(), err } // Seed a pending tunnel so the listing and per-address commands have something to show. - seed := run("create-tunnel", "10.222.0.9") + seed, err := run("create-tunnel", "10.222.0.9") + require.NoError(t, err) require.Equal(t, "Created\n", seed, "seeding a tunnel should succeed before anything else is recorded") cases := []outputCase{ @@ -120,6 +127,9 @@ func TestCommandOutput(t *testing.T) { {"close-tunnel", []string{"not-an-address"}}, {"help", nil}, {"help", []string{"print-cert"}}, + {"help", []string{"logout"}}, + {"logout", []string{"-h"}}, + {"not-a-command", []string{"10.222.0.9"}}, // reload is last so that whatever a reload does to the daemon cannot leak into an // earlier case. In this harness it fails: the config was loaded from a string and // has no path to re-read, which is what the recorded result says. @@ -133,13 +143,19 @@ func TestCommandOutput(t *testing.T) { args = append(args, c.args...) fmt.Fprintf(got, "$ %s\n", strings.Join(args, " ")) - out := normalise(run(args...)) + out, err := run(args...) + out = normalise(out) got.WriteString(out) // Output that does not end in a newline is recorded as such, the way diff marks // it, so the golden sees that byte too and the next prompt still starts a line. if out != "" && !strings.HasSuffix(out, "\n") { got.WriteString("\\ No newline at end of output\n") } + // A failed command is recorded as the exit status an ssh caller sees, so the + // golden pins which outcomes count as failures. + if err != nil { + got.WriteString("exit status 1\n") + } got.WriteString("\n") } } diff --git a/e2e/testdata/commands_output.golden b/e2e/testdata/commands_output.golden index 2cfc65f9..aa23b482 100644 --- a/e2e/testdata/commands_output.golden +++ b/e2e/testdata/commands_output.golden @@ -158,66 +158,84 @@ $ print-cert -pretty -raw $ print-cert 10.222.0.9 Could not find tunnel for vpn addr: 10.222.0.9 +exit status 1 $ print-cert -json 10.222.0.9 Could not find tunnel for vpn addr: 10.222.0.9 +exit status 1 $ print-cert -pretty 10.222.0.9 Could not find tunnel for vpn addr: 10.222.0.9 +exit status 1 $ print-cert 10.222.0.250 Could not find tunnel for vpn addr: 10.222.0.250 +exit status 1 $ print-cert -json 10.222.0.250 Could not find tunnel for vpn addr: 10.222.0.250 +exit status 1 $ print-cert -pretty 10.222.0.250 Could not find tunnel for vpn addr: 10.222.0.250 +exit status 1 $ print-cert not-an-address The provided vpn addr could not be parsed: not-an-address +exit status 1 $ print-cert -json not-an-address The provided vpn addr could not be parsed: not-an-address +exit status 1 $ print-cert -pretty not-an-address The provided vpn addr could not be parsed: not-an-address +exit status 1 $ print-tunnel No vpn address was provided +exit status 1 $ print-tunnel -json flag provided but not defined: -json Usage: -pretty pretty prints json +exit status 1 $ print-tunnel -pretty No vpn address was provided +exit status 1 $ print-tunnel 10.222.0.9 Could not find tunnel for vpn addr: 10.222.0.9 +exit status 1 $ print-tunnel -json 10.222.0.9 flag provided but not defined: -json Usage: -pretty pretty prints json +exit status 1 $ print-tunnel -pretty 10.222.0.9 Could not find tunnel for vpn addr: 10.222.0.9 +exit status 1 $ print-tunnel 10.222.0.250 Could not find tunnel for vpn addr: 10.222.0.250 +exit status 1 $ print-tunnel -json 10.222.0.250 flag provided but not defined: -json Usage: -pretty pretty prints json +exit status 1 $ print-tunnel -pretty 10.222.0.250 Could not find tunnel for vpn addr: 10.222.0.250 +exit status 1 $ print-relays {"Relays":null} @@ -227,6 +245,7 @@ flag provided but not defined: -json Usage: -pretty pretty prints json +exit status 1 $ print-relays -pretty { @@ -235,183 +254,230 @@ $ print-relays -pretty $ query-lighthouse No vpn address was provided +exit status 1 $ query-lighthouse -json The provided vpn address could not be parsed: -json +exit status 1 $ query-lighthouse -pretty The provided vpn address could not be parsed: -pretty +exit status 1 $ query-lighthouse 10.222.0.9 {} $ query-lighthouse -json 10.222.0.9 The provided vpn address could not be parsed: -json +exit status 1 $ query-lighthouse -pretty 10.222.0.9 The provided vpn address could not be parsed: -pretty +exit status 1 $ query-lighthouse not-an-address The provided vpn address could not be parsed: not-an-address +exit status 1 $ query-lighthouse -json not-an-address The provided vpn address could not be parsed: -json +exit status 1 $ query-lighthouse -pretty not-an-address The provided vpn address could not be parsed: -pretty +exit status 1 $ log-level Log level is not reconfigurable on this logger +exit status 1 $ log-level -json Log level is not reconfigurable on this logger +exit status 1 $ log-level -pretty Log level is not reconfigurable on this logger +exit status 1 $ log-level debug Log level is not reconfigurable on this logger +exit status 1 $ log-level -json debug Log level is not reconfigurable on this logger +exit status 1 $ log-level -pretty debug Log level is not reconfigurable on this logger +exit status 1 $ log-level bogus Log level is not reconfigurable on this logger +exit status 1 $ log-level -json bogus Log level is not reconfigurable on this logger +exit status 1 $ log-level -pretty bogus Log level is not reconfigurable on this logger +exit status 1 $ log-level info Log level is not reconfigurable on this logger +exit status 1 $ log-level -json info Log level is not reconfigurable on this logger +exit status 1 $ log-level -pretty info Log level is not reconfigurable on this logger +exit status 1 $ log-format Log format is not reconfigurable on this logger +exit status 1 $ log-format -json Log format is not reconfigurable on this logger +exit status 1 $ log-format -pretty Log format is not reconfigurable on this logger +exit status 1 $ log-format json Log format is not reconfigurable on this logger +exit status 1 $ log-format -json json Log format is not reconfigurable on this logger +exit status 1 $ log-format -pretty json Log format is not reconfigurable on this logger +exit status 1 $ log-format text Log format is not reconfigurable on this logger +exit status 1 $ log-format -json text Log format is not reconfigurable on this logger +exit status 1 $ log-format -pretty text Log format is not reconfigurable on this logger +exit status 1 $ change-remote No vpn address was provided +exit status 1 $ change-remote -json flag provided but not defined: -json Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote -pretty flag provided but not defined: -pretty Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote 10.222.0.9 No address was provided +exit status 1 $ change-remote -json 10.222.0.9 flag provided but not defined: -json Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote -pretty 10.222.0.9 flag provided but not defined: -pretty Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote -address 10.94.0.10:4242 10.222.0.9 Could not find tunnel for vpn address: 10.222.0.9 +exit status 1 $ change-remote -json -address 10.94.0.10:4242 10.222.0.9 flag provided but not defined: -json Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote -pretty -address 10.94.0.10:4242 10.222.0.9 flag provided but not defined: -pretty Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote -address 10.94.0.10:4242 10.222.0.250 Could not find tunnel for vpn address: 10.222.0.250 +exit status 1 $ change-remote -json -address 10.94.0.10:4242 10.222.0.250 flag provided but not defined: -json Usage: -address string The new remote address, ip:port +exit status 1 $ change-remote -pretty -address 10.94.0.10:4242 10.222.0.250 flag provided but not defined: -pretty Usage: -address string The new remote address, ip:port +exit status 1 $ create-tunnel No vpn address was provided +exit status 1 $ create-tunnel -json flag provided but not defined: -json Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel -pretty flag provided but not defined: -pretty Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel 10.222.0.9 Tunnel already handshaking +exit status 1 $ create-tunnel -json 10.222.0.9 flag provided but not defined: -json Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel -pretty 10.222.0.9 flag provided but not defined: -pretty Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel -address 10.94.0.10:4242 10.222.0.10 Created @@ -421,72 +487,86 @@ flag provided but not defined: -json Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel -pretty -address 10.94.0.10:4242 10.222.0.10 flag provided but not defined: -pretty Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel not-an-address The provided vpn address could not be parsed: not-an-address +exit status 1 $ create-tunnel -json not-an-address flag provided but not defined: -json Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ create-tunnel -pretty not-an-address flag provided but not defined: -pretty Usage: -address string Optionally provide a real remote address, ip:port +exit status 1 $ close-tunnel No vpn address was provided +exit status 1 $ close-tunnel -json flag provided but not defined: -json Usage: -local-only Disables notifying the remote that the tunnel is shutting down +exit status 1 $ close-tunnel -pretty flag provided but not defined: -pretty Usage: -local-only Disables notifying the remote that the tunnel is shutting down +exit status 1 $ close-tunnel 10.222.0.250 Could not find tunnel for vpn address: 10.222.0.250 +exit status 1 $ close-tunnel -json 10.222.0.250 flag provided but not defined: -json Usage: -local-only Disables notifying the remote that the tunnel is shutting down +exit status 1 $ close-tunnel -pretty 10.222.0.250 flag provided but not defined: -pretty Usage: -local-only Disables notifying the remote that the tunnel is shutting down +exit status 1 $ close-tunnel not-an-address The provided vpn address could not be parsed: not-an-address +exit status 1 $ close-tunnel -json not-an-address flag provided but not defined: -json Usage: -local-only Disables notifying the remote that the tunnel is shutting down +exit status 1 $ close-tunnel -pretty not-an-address flag provided but not defined: -pretty Usage: -local-only Disables notifying the remote that the tunnel is shutting down +exit status 1 $ help Available commands: @@ -500,6 +580,7 @@ list-lighthouse-addrmap - List all lighthouse map entries list-pending-hostmap - List all handshaking hosts log-format - Gets or sets the current log format log-level - Gets or sets the current log level +logout - Ends the current session print-cert - Prints the current certificate being used or the certificate for the provided vpn addr print-relays - Prints json details about all relay info print-tunnel - Prints json details about a tunnel for the provided vpn addr @@ -510,9 +591,11 @@ version - Prints the currently running version of nebula $ help -json Command not available -json +exit status 1 $ help -pretty Command not available -pretty +exit status 1 $ help print-cert print-cert - Prints the current certificate being used or the certificate for the provided vpn addr @@ -525,19 +608,113 @@ print-cert - Prints the current certificate being used or the certificate for th $ help -json print-cert Command not available -json +exit status 1 $ help -pretty print-cert Command not available -pretty +exit status 1 + +$ help logout +logout - Ends the current session + +$ help -json logout +Command not available -json +exit status 1 + +$ help -pretty logout +Command not available -pretty +exit status 1 + +$ logout -h +logout - Ends the current session + +$ logout -json -h +logout - Ends the current session + +$ logout -pretty -h +logout - Ends the current session + +$ not-a-command 10.222.0.9 +did not understand: not-a-command +Available commands: +change-remote - Changes the remote address used in the tunnel for the provided vpn addr +close-tunnel - Closes a tunnel for the provided vpn addr +create-tunnel - Creates a tunnel for the provided vpn address +device-info - Prints information about the network device. +help - prints available commands or help for specific usage info +list-hostmap - List all known previously connected hosts +list-lighthouse-addrmap - List all lighthouse map entries +list-pending-hostmap - List all handshaking hosts +log-format - Gets or sets the current log format +log-level - Gets or sets the current log level +logout - Ends the current session +print-cert - Prints the current certificate being used or the certificate for the provided vpn addr +print-relays - Prints json details about all relay info +print-tunnel - Prints json details about a tunnel for the provided vpn addr +query-lighthouse - Query the lighthouses for the provided vpn address +reload - Reloads configuration from disk, same as sending HUP to the process +version - Prints the currently running version of nebula + +exit status 1 + +$ not-a-command -json 10.222.0.9 +did not understand: not-a-command +Available commands: +change-remote - Changes the remote address used in the tunnel for the provided vpn addr +close-tunnel - Closes a tunnel for the provided vpn addr +create-tunnel - Creates a tunnel for the provided vpn address +device-info - Prints information about the network device. +help - prints available commands or help for specific usage info +list-hostmap - List all known previously connected hosts +list-lighthouse-addrmap - List all lighthouse map entries +list-pending-hostmap - List all handshaking hosts +log-format - Gets or sets the current log format +log-level - Gets or sets the current log level +logout - Ends the current session +print-cert - Prints the current certificate being used or the certificate for the provided vpn addr +print-relays - Prints json details about all relay info +print-tunnel - Prints json details about a tunnel for the provided vpn addr +query-lighthouse - Query the lighthouses for the provided vpn address +reload - Reloads configuration from disk, same as sending HUP to the process +version - Prints the currently running version of nebula + +exit status 1 + +$ not-a-command -pretty 10.222.0.9 +did not understand: not-a-command +Available commands: +change-remote - Changes the remote address used in the tunnel for the provided vpn addr +close-tunnel - Closes a tunnel for the provided vpn addr +create-tunnel - Creates a tunnel for the provided vpn address +device-info - Prints information about the network device. +help - prints available commands or help for specific usage info +list-hostmap - List all known previously connected hosts +list-lighthouse-addrmap - List all lighthouse map entries +list-pending-hostmap - List all handshaking hosts +log-format - Gets or sets the current log format +log-level - Gets or sets the current log level +logout - Ends the current session +print-cert - Prints the current certificate being used or the certificate for the provided vpn addr +print-relays - Prints json details about all relay info +print-tunnel - Prints json details about a tunnel for the provided vpn addr +query-lighthouse - Query the lighthouses for the provided vpn address +reload - Reloads configuration from disk, same as sending HUP to the process +version - Prints the currently running version of nebula + +exit status 1 $ reload Reloading config Config reload failed, the new config did not load: no config files found at +exit status 1 $ reload -json Reloading config Config reload failed, the new config did not load: no config files found at +exit status 1 $ reload -pretty Reloading config Config reload failed, the new config did not load: no config files found at +exit status 1 diff --git a/ssh_reload_test.go b/ssh_reload_test.go index 2727d1c7..d2d417a6 100644 --- a/ssh_reload_test.go +++ b/ssh_reload_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "testing" + "github.com/slackhq/nebula/commands" "github.com/slackhq/nebula/config" "github.com/slackhq/nebula/test" "github.com/stretchr/testify/assert" @@ -26,7 +27,7 @@ func (w *lineWriter) Write(s string) error { return w.WriteLine(s) } func (w *lineWriter) WriteBytes(b []byte) error { return w.WriteLine(string(b)) } func (w *lineWriter) GetWriter() io.Writer { return io.Discard } -func TestSSHReloadReportsTheResult(t *testing.T) { +func TestReloadCommandReportsTheResult(t *testing.T) { dir := t.TempDir() write := func(body string) { require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte(body), 0o600)) @@ -43,32 +44,42 @@ func TestSSHReloadReportsTheResult(t *testing.T) { } }) - reload := func() []string { + // reload returns what the client was told. A reload that failed is a commands.Failure + // to the surface, exit 1 over ssh, with the result line as its message; one that + // merely needs a restart is not. + reload := func(failed bool) []string { w := &lineWriter{} - require.NoError(t, sshReload(l, c, w)) + err := reloadCommand(l, c, w) + if failed { + var f *commands.Failure + require.ErrorAs(t, err, &f) + assert.Equal(t, w.lines[len(w.lines)-1], f.Msg) + } else { + require.NoError(t, err) + } return w.lines } write("a: 2\nb: 1") - assert.Equal(t, []string{"Reloading config", "Config reload complete"}, reload()) + assert.Equal(t, []string{"Reloading config", "Config reload complete"}, reload(false)) fail = true write("a: 3\nb: 1") - assert.Equal(t, []string{"Reloading config", "Config reload completed with errors, review the logs"}, reload()) + assert.Equal(t, []string{"Reloading config", "Config reload completed with errors, review the logs"}, reload(true)) fail = false write("a: 3\nb: 2") - assert.Equal(t, []string{"Reloading config", "Config reload was completed but requires a restart to fully apply, keys [b]"}, reload()) + assert.Equal(t, []string{"Reloading config", "Config reload was completed but requires a restart to fully apply, keys [b]"}, reload(false)) write("a: [") - lines := reload() + lines := reload(true) require.Len(t, lines, 2) assert.Contains(t, lines[1], "Config reload failed, the new config did not load") assert.Contains(t, lines[1], dir, "names the config that did not load") } // A client that went away still gets the reload it asked for -func TestSSHReloadReloadsWhenTheClientIsGone(t *testing.T) { +func TestReloadCommandReloadsWhenTheClientIsGone(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte("a: 1"), 0o600)) l := test.NewLogger() @@ -78,6 +89,6 @@ func TestSSHReloadReloadsWhenTheClientIsGone(t *testing.T) { c.RegisterReloadCallback(func(*config.C, *config.Reload) { reloaded = true }) gone := errors.New("client is gone") - require.ErrorIs(t, sshReload(l, c, &lineWriter{err: gone}), gone) + require.ErrorIs(t, reloadCommand(l, c, &lineWriter{err: gone}), gone) assert.True(t, reloaded) } diff --git a/sshd/session.go b/sshd/session.go index 06f074f9..34c6e6bb 100644 --- a/sshd/session.go +++ b/sshd/session.go @@ -1,6 +1,7 @@ package sshd import ( + "errors" "log/slog" "strings" @@ -25,19 +26,25 @@ func NewSession(reg *commands.Registry, conn *ssh.ServerConn, chans <-chan ssh.N cancel: cancel, } - s.commands.Register(&commands.Command{ - Name: commands.Logout, - ShortDescription: "Ends the current session", - Callback: func(a any, args []string, w commands.StringWriter) error { - s.Close() - return nil - }, - }) + s.commands.Register(LogoutCommand(s.Close)) go s.handleChannels(chans) return s } +// LogoutCommand is the logout command a session registers on its clone of the registry, +// with end as what it does. Exported so a test can serve what a session serves. +func LogoutCommand(end func()) *commands.Command { + return &commands.Command{ + Name: commands.Logout, + ShortDescription: "Ends the current session", + Callback: func(fs any, a []string, w commands.StringWriter) error { + end() + return nil + }, + } +} + func (s *session) handleChannels(chans <-chan ssh.NewChannel) { defer s.Close() for newChannel := range chans { @@ -84,9 +91,13 @@ func (s *session) handleRequests(in <-chan *ssh.Request, channel ssh.Channel) { } req.Reply(true, nil) - s.dispatchCommand(payload.Value, commands.NewStringWriter(channel)) + err := s.dispatchCommand(payload.Value, commands.NewStringWriter(channel)) + // A command that failed exits 1, so `ssh host cmd` callers can tell. status := struct{ Status uint32 }{uint32(0)} + if err != nil { + status.Status = 1 + } channel.SendRequest("exit-status", false, ssh.Marshal(status)) channel.Close() return @@ -135,8 +146,20 @@ func (s *session) handleInput() { } } -func (s *session) dispatchCommand(line string, w commands.StringWriter) { - s.commands.Dispatch(line, w) +// dispatchCommand runs line and returns the command's error, which it has logged unless +// the command already told the user everything: the log line is for the operator reading +// the daemon's log, who learns nothing from a failure the user caused. +func (s *session) dispatchCommand(line string, w commands.StringWriter) error { + err := s.commands.Dispatch(line, w) + if err == nil { + return nil + } + + var f *commands.Failure + if !errors.As(err, &f) { + s.l.Debug("Command returned an error", "line", line, "error", err) + } + return err } func (s *session) Close() { diff --git a/sshd_reload_test.go b/sshd_reload_test.go index 8ac392d5..8528b559 100644 --- a/sshd_reload_test.go +++ b/sshd_reload_test.go @@ -6,10 +6,12 @@ import ( "crypto/rand" "crypto/rsa" "encoding/pem" + "errors" "fmt" "net" "os" "path/filepath" + "strings" "testing" "time" @@ -24,6 +26,7 @@ import ( type sshdReloadHarness struct { c *config.C + reg *commands.Registry server *sshd.SSHServer keyFile string signer ssh.Signer @@ -45,7 +48,8 @@ func newSSHDReloadHarness(t *testing.T) *sshdReloadHarness { h.c = config.NewC(l) require.NoError(t, h.c.LoadString(h.config())) - h.server, err = sshd.NewSSHServer(t.Context(), l, commands.New()) + h.reg = commands.New() + h.server, err = sshd.NewSSHServer(t.Context(), l, h.reg) require.NoError(t, err) wireSSHReload(l, h.server, h.c) run, err := configSSH(l, h.server, h.c, nil) @@ -239,3 +243,39 @@ func TestSSHDReload_SkippedEntriesFail(t *testing.T) { }) } } + +// TestSSHExecExitStatusReflectsTheCommand: an exec request exits 1 when the command +// returned an error and 0 otherwise, with the command's output either way. +func TestSSHExecExitStatusReflectsTheCommand(t *testing.T) { + h := newSSHDReloadHarness(t) + h.reg.Register(&commands.Command{ + Name: "fails", + Callback: func(fs any, a []string, w commands.StringWriter) error { + _ = w.WriteLine("nope") + return errors.New("failed") + }, + }) + client, _ := h.connect(t) + + sess, err := client.NewSession() + require.NoError(t, err) + out, err := sess.CombinedOutput("fails") + var exit *ssh.ExitError + require.ErrorAs(t, err, &exit) + assert.Equal(t, 1, exit.ExitStatus()) + assert.Equal(t, "nope\n", string(out)) + + sess, err = client.NewSession() + require.NoError(t, err) + out, err = sess.CombinedOutput("help fails") + require.NoError(t, err) + assert.Equal(t, "fails - \n", string(out)) + + // A word that is not a command is a failure too, with the command list as before. + sess, err = client.NewSession() + require.NoError(t, err) + out, err = sess.CombinedOutput("nope") + require.ErrorAs(t, err, &exit) + assert.Equal(t, 1, exit.ExitStatus()) + assert.True(t, strings.HasPrefix(string(out), "did not understand: nope\nAvailable commands:\n"), string(out)) +}