From 0f767ce529e84c862d985880959dcbc264a17324 Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Wed, 21 Oct 2026 11:16:00 -0400 Subject: [PATCH] Add a local control socket and nebula ctl (#1975) Add support for accessing the previously ssh-only debug commands via `nebula ctl .` - unix socket (named pipe on windows) for communication - sockets scoped to the user + an optional group - in cmd/nebula and cmd/nebula-service, but not nebula.Main Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 5 + cmd/internal/ctl/ctl.go | 420 ++++++++++++ cmd/internal/ctl/ctl_test.go | 294 +++++++++ cmd/internal/ctl/ctl_unix_test.go | 42 ++ cmd/nebula-service/main.go | 23 +- cmd/nebula-service/service.go | 13 + cmd/nebula/main.go | 23 +- commands.go | 676 +++++++++----------- commands/command.go | 309 +++++++-- commands/command_test.go | 101 +++ commands/registry.go | 385 +++++++++-- commands/registry_test.go | 460 ++++++++++---- commands/render.go | 179 ++++++ commands/render_test.go | 105 +++ commands/request.go | 119 ++++ commands/request_test.go | 115 ++++ commands/writer.go | 48 +- commands_test.go | 84 ++- config/config.go | 20 +- config/config_test.go | 39 ++ e2e/commands_output_test.go | 7 +- e2e/testdata/commands_output.golden | 744 ++++++++++++++++------ examples/config.yml | 20 + go.mod | 1 + go.sum | 2 + localapi/access.go | 79 +++ localapi/access_unix_test.go | 176 ++++++ localapi/access_windows_test.go | 44 ++ localapi/client.go | 163 +++++ localapi/group_unix.go | 39 ++ localapi/group_windows.go | 30 + localapi/path.go | 58 ++ localapi/path_test.go | 52 ++ localapi/peercred_bsd.go | 25 + localapi/peercred_linux.go | 25 + localapi/peercred_netbsd.go | 62 ++ localapi/peercred_openbsd.go | 34 + localapi/peercred_windows.go | 20 + localapi/platform_unix.go | 302 +++++++++ localapi/platform_unix_test.go | 358 +++++++++++ localapi/platform_windows.go | 255 ++++++++ localapi/platform_windows_test.go | 186 ++++++ localapi/protocol.go | 167 +++++ localapi/protocol_test.go | 102 +++ localapi/server.go | 570 +++++++++++++++++ localapi/server_test.go | 950 ++++++++++++++++++++++++++++ localapi/wire.go | 144 +++++ localapi/wire_test.go | 175 +++++ ssh_commands_test.go | 69 +- ssh_reload_test.go | 64 +- sshd/session.go | 31 +- sshd_reload_test.go | 8 +- test/helpers.go | 12 + testdata/ssh_commands.golden | 72 ++- 54 files changed, 7581 insertions(+), 925 deletions(-) create mode 100644 cmd/internal/ctl/ctl.go create mode 100644 cmd/internal/ctl/ctl_test.go create mode 100644 cmd/internal/ctl/ctl_unix_test.go create mode 100644 commands/command_test.go create mode 100644 commands/render.go create mode 100644 commands/render_test.go create mode 100644 commands/request.go create mode 100644 commands/request_test.go create mode 100644 localapi/access.go create mode 100644 localapi/access_unix_test.go create mode 100644 localapi/access_windows_test.go create mode 100644 localapi/client.go create mode 100644 localapi/group_unix.go create mode 100644 localapi/group_windows.go create mode 100644 localapi/path.go create mode 100644 localapi/path_test.go create mode 100644 localapi/peercred_bsd.go create mode 100644 localapi/peercred_linux.go create mode 100644 localapi/peercred_netbsd.go create mode 100644 localapi/peercred_openbsd.go create mode 100644 localapi/peercred_windows.go create mode 100644 localapi/platform_unix.go create mode 100644 localapi/platform_unix_test.go create mode 100644 localapi/platform_windows.go create mode 100644 localapi/platform_windows_test.go create mode 100644 localapi/protocol.go create mode 100644 localapi/protocol_test.go create mode 100644 localapi/server.go create mode 100644 localapi/server_test.go create mode 100644 localapi/wire.go create mode 100644 localapi/wire_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f941ab89..668a9b75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- A control socket that provides access to all of the ssh debug server's commands along with support + for running `nebula ctl ` as a client from the CLI. + ### Removed - The SSH debug server's `start-cpu-profile`, `stop-cpu-profile`, `save-heap-profile`, `save-mutex-profile`, and diff --git a/cmd/internal/ctl/ctl.go b/cmd/internal/ctl/ctl.go new file mode 100644 index 00000000..f8f5072f --- /dev/null +++ b/cmd/internal/ctl/ctl.go @@ -0,0 +1,420 @@ +package ctl + +import ( + "bytes" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "log/slog" + "os" + "strings" + + "github.com/slackhq/nebula/commands" + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/localapi" +) + +// Word is the argument that selects the control client instead of the daemon. +const Word = "ctl" + +// Subcommand handles the one bare word a daemon binary accepts before its flags. `nebula +// ctl ` runs the client. Any other first argument that is not a flag is a +// mistyped subcommand, reported with a usage line: the daemon's flag parser would ignore +// the stray word and start nebula, so `nebula clt` would bring up a second daemon +// instead of reporting a typo. It returns the exit code and true when the process is +// done, and false when argv is the daemon's to parse. Both daemon binaries, cmd/nebula +// and cmd/nebula-service, call it before anything else, so each has a nebula ctl and +// neither has the hazard. A word that follows the daemon's flags instead is Trailing's. +func Subcommand(argv []string) (code int, done bool) { + if len(argv) < 2 || strings.HasPrefix(argv[1], "-") { + return 0, false + } + + if argv[1] == Word { + return Run(argv[2:]), true + } + + return unknownCommand(argv[1]), true +} + +// Trailing handles the words the daemon's flag parser leaves unparsed: flag.Parse stops +// at the first argument that is not a flag and hands the rest back. rest is flag.Args() +// after the daemon has parsed, and configPath is the config the daemon's -config named, +// "" when it did not. Both daemon binaries call it right after flag.Parse, so that +// `nebula -config x ctl identity` runs the client and `nebula -config x clt` reports a +// typo, as Subcommand does for a word at the front of the line; a daemon that ignored +// the words would start in their place. +// +// A leading ctl word runs the client on what follows, with the daemon's -config carried +// over: an operator who typed it meant the daemon running on that config, and the client +// derives the same socket from it. Any other word is a mistake, reported as Subcommand +// reports one. +func Trailing(rest []string, configPath string) (code int, done bool) { + if len(rest) == 0 { + return 0, false + } + + if rest[0] != Word { + return unknownCommand(rest[0]), true + } + + args := rest[1:] + if configPath != "" { + // First, so a -config on the ctl side of the line still wins: Parse applies + // flags in order and the last value set is the one read. + args = append([]string{"-" + commands.FlagConfig, configPath}, args...) + } + + return Run(args), true +} + +// unknownCommand reports a bare word that is neither a flag nor ctl, with the one usage +// line that says what the two forms of the command line are. +func unknownCommand(word string) int { + fmt.Fprintf(os.Stderr, "nebula: unknown command %q\n\n", word) + fmt.Fprintf(os.Stderr, "Run nebula with flags to start it, or `nebula %s ` to control one that is already running.\n", Word) + return 1 +} + +// Run implements `nebula ctl [args]` +// +// args is everything after the `ctl` word. +func Run(args []string) int { + fs := flag.NewFlagSet("nebula ctl", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + + configPath := fs.String(commands.FlagConfig, "", "Path to the config of the running nebula, used to locate its control socket") + socketPath := fs.String(commands.FlagSocket, "", "Path to the control socket, instead of the one the config implies") + fs.Usage = func() { + fmt.Fprintf(os.Stderr, "Usage: nebula ctl [flags] [command flags]\n\n") + fmt.Fprintf(os.Stderr, "Inspects and controls a nebula already running on this machine.\n") + fmt.Fprintf(os.Stderr, "Run it as the user who ran nebula, or as a member of the group %s names.\n\n", localapi.CtlGroupKey) + fmt.Fprintf(os.Stderr, "Flags, which may come before or after the command:\n") + fs.PrintDefaults() + fmt.Fprintf(os.Stderr, "\nRun `nebula ctl help` for the available commands.\n") + } + + own, cmdArgs := splitLine(fs, args) + if err := fs.Parse(own); err != nil { + // fs has already printed the usage, or the problem. Asking for help is not a + // failure, and a script that chains on `nebula ctl -h` should not see one. + if errors.Is(err, flag.ErrHelp) { + return 0 + } + return 1 + } + + if len(cmdArgs) == 0 { + cmdArgs = []string{"help"} + } + + path, err := resolveSocket(*socketPath, *configPath) + if err != nil { + fmt.Fprintf(os.Stderr, "%s\n", err) + return 1 + } + + client := &localapi.Client{Path: path} + + // Ask the daemon what it serves before parsing anything. The flags belong to the + // command, which lives over there, so this is what lets the CLI keep its usual shape + // while the wire protocol stays a named-parameter API rather than a shipped argv. + schema, err := client.Describe() + if err != nil { + // A daemon that answered was reached, and what it said, or that it could not be + // read, is the whole message. + var refused *localapi.DaemonError + var unread *localapi.ReplyError + if errors.As(err, &refused) || errors.As(err, &unread) { + fmt.Fprintf(os.Stderr, "nebula: %s\n", err) + return 1 + } + + // Otherwise the operating system said why, and it is passed on as it came: a + // permission denied or a missing socket says more about what to do than a + // guess at the operator's setup would. + fmt.Fprintf(os.Stderr, "nebula: could not reach the control socket at %s: %s\n", path, err) + return 1 + } + + req, printedHelp, err := parseCommand(schema, cmdArgs) + if err != nil { + fmt.Fprintf(os.Stderr, "%s\n", err) + return 1 + } + + if printedHelp { + return 0 + } + + res, err := client.Invoke(string(req.name), req.Params, req.Args, wireFormat(req.Format)) + if err != nil { + fmt.Fprintf(os.Stderr, "nebula: %s\n", err) + return 1 + } + + out := output(res, req.Format) + if out != "" { + fmt.Fprint(os.Stdout, out) + if !strings.HasSuffix(out, "\n") { + fmt.Fprintln(os.Stdout) + } + } + + // Error set is the one failure signal, whether the request never ran or the command + // ran and refused. A refusal asked for in JSON has already gone to stdout as the + // {ok, reason} object, and is not said again on stderr; the exit status carries it. + if res.Error == "" { + return 0 + } + + if len(res.Result) == 0 && len(res.Text) == 0 { + msg := res.Error + if !strings.HasSuffix(msg, "\n") { + msg += "\n" + } + fmt.Fprint(os.Stderr, msg) + } + + return 1 +} + +// output renders what the daemon answered for the format the line asked for. A text +// request is answered in Text with the bytes the command wrote, printed as they are; a +// JSON request is answered in Result with the value itself, printed as it came, since a +// value that happens to be a string is still JSON to the script reading it. Indentation +// is the client's business: whitespace does not survive a JSON envelope, and the daemon +// has no reason to care how its answer is laid out. The bytes are re-indented rather +// than decoded and encoded again, which would sort the keys and round the numbers. +func output(res *localapi.Response, format commands.Format) string { + switch format { + case commands.FormatJSON: + return string(res.Result) + case commands.FormatPrettyJSON: + var buf bytes.Buffer + if err := json.Indent(&buf, res.Result, "", " "); err != nil { + return string(res.Result) + } + return buf.String() + default: + return string(res.Text) + } +} + +// ctlRequest is one parsed nebula ctl command line: the command's name, which only the +// daemon can resolve, and the line as every surface parses it. The Request's Command is +// nil here, since the CLI holds a schema rather than the command, and its Format is +// FormatUnset when the line named none, which to nebula ctl means text: its default +// differs from the API's. +type ctlRequest struct { + name commands.Name + commands.Request +} + +// wireFormat is what a request asks the daemon for. Pretty is the client's business, +// since whitespace does not survive a JSON envelope, so it travels as json. No +// preference is text, nebula ctl's default: a person gets the human rendering wherever +// one exists. +func wireFormat(f commands.Format) string { + switch f { + case commands.FormatJSON, commands.FormatPrettyJSON: + return "json" + default: + return "text" + } +} + +// parseCommand turns the user's command line into a ctlRequest, using the schema the +// daemon supplied. The line is parsed with the same rules the daemon applies to an ssh +// line, through ParseArgs, and ParseLine reads what was set into the Request to send, so +// a line means the same on both. +// +// printedHelp reports that help was written for the command instead, and there is +// nothing to send. +func parseCommand(schema []commands.CommandSchema, argv []string) (req ctlRequest, printedHelp bool, err error) { + name := commands.Name(argv[0]) + + var found *commands.CommandSchema + for i := range schema { + if schema[i].Name == name { + found = &schema[i] + break + } + } + + if found == nil { + // Let the daemon answer. It knows whether this is a typo or a command it holds + // back, and its message for the second case is better than anything guessable + // from the schema alone. + return ctlRequest{name: name, Request: commands.Request{Args: argv[1:]}}, false, nil + } + + // The flag package's own report is a bare message followed by a usage dump in a layout + // that does not match WriteCommandHelp, so it is silenced and the error is reported + // once, below, with a pointer at the help this command actually prints. + fs := flag.NewFlagSet(string(name), flag.ContinueOnError) + fs.SetOutput(io.Discard) + + for _, f := range found.Flags { + if f.Bool { + fs.Bool(f.Name, f.Default == "true", f.Usage) + continue + } + fs.String(f.Name, f.Default, f.Usage) + } + + operands, err := commands.ParseArgs(fs, argv[1:]) + if err != nil { + if errors.Is(err, flag.ErrHelp) { + // The same rendering the daemon uses for `help `, so -h and help agree. + _ = commands.WriteCommandHelp(*found, os.Stdout) + return ctlRequest{}, true, nil + } + + // Say what flag did: an unknown flag, a missing value and a malformed value are + // different mistakes, and "does not take" would be wrong for two of them. + return ctlRequest{}, false, fmt.Errorf("nebula: %s: %s (see 'nebula ctl %s -h')", name, err, name) + } + + // Only what the user actually set travels. A parameter left alone is the daemon's + // default to apply, not the client's to assert, and the presentation flags are the + // client's to interpret, never sent. + // + // Where the command has no human form the daemon says so, and only once the command + // has run: a refusal ("No vpn address was provided") always has a human form, so + // telling the user to pass -json before finding out whether the command would even + // accept the line sends them to fix the wrong thing. That is why the schema does not + // say which commands have a human form: nothing should decide on it before running. + return ctlRequest{name: name, Request: commands.ParseLine(fs, operands)}, false, nil +} + +// splitLine divides a nebula ctl line into the client's own flags, for fs to parse, and +// the command line to send, which begins with the command word. The client's flags may +// come before the command or after it: `nebula ctl -config x identity` and +// `nebula ctl identity -config x` mean the same, so an operator who has typed a command +// and then remembers which daemon they meant does not have to retype the line. +// +// Before the command word every flag is the client's, whatever it is, so an unknown one +// is reported by fs, and -h is the client's usage. After it only the +// flags fs defines are lifted out, and only those: the command's own flags are unknown +// here, since they come from the daemon the line has yet to name, and -h after the +// command asks for that command's help. Which command flags take a value is unknown too, +// so the client's names are reserved in the commands package and no command can give +// one a value that would be lifted from here. A "--" before the command is consumed, as +// fs.Parse would have; one after it belongs to the command line and ends the lifting, +// so an operand spelled like a client flag can still be passed. +func splitLine(fs *flag.FlagSet, args []string) (own, cmd []string) { + seenCommand := false + for i := 0; i < len(args); i++ { + a := args[i] + + if a == "--" { + if seenCommand { + return own, append(cmd, args[i:]...) + } + seenCommand = true + continue + } + + if !seenCommand { + if len(a) < 2 || a[0] != '-' { + seenCommand = true + cmd = append(cmd, a) + continue + } + + own = append(own, a) + if f, hasValue := commands.LookupArg(fs, a); f != nil && !hasValue && !commands.IsBoolFlag(f) && i+1 < len(args) { + i++ + own = append(own, args[i]) + } + continue + } + + if len(a) >= 2 && a[0] == '-' { + // The same reading of the argument ParseArgs makes, so a word this lifts as a + // client flag's value is one the daemon's parser would have read the same way. + if f, hasValue := commands.LookupArg(fs, a); f != nil { + own = append(own, a) + if !hasValue && !commands.IsBoolFlag(f) && i+1 < len(args) { + i++ + own = append(own, args[i]) + } + continue + } + } + + cmd = append(cmd, a) + } + + return own, cmd +} + +// resolveSocket decides which socket to talk to. It never searches: the answer is the +// socket the operator named, or the one the config implies, and nothing else. +// +// An explicit flag wins. Otherwise the config is read, the one the operator named or the +// one found next to the executable, and the path is the socket that config implies: its +// local_api.control.socket if set, else the same derivation the daemon applies. +func resolveSocket(socketFlag, configFlag string) (string, error) { + if socketFlag != "" { + return socketFlag, nil + } + + return pathFromConfig(configFlag) +} + +// pathFromConfig returns the socket path the daemon would be serving: the one +// local_api.control.socket names in the config, so an operator who moved the socket does +// not have to repeat themselves on every invocation, else the default. +// +// A config the operator did not name is a guess: config.DefaultPath looks next to the +// executable, and a package that installs a default config there while systemd starts +// the daemon with another will make it the wrong guess. It is still the only answer +// that involves no searching. A guess that cannot be read is not an error, it is no +// guess: a member of the socket's group cannot read a root daemon's config, and is +// answered by the default path, where the daemon serves unless that config moved it. +// A config the operator named is theirs to make readable. +func pathFromConfig(configFlag string) (string, error) { + return socketFromConfigFile(configFlag, configFlag != "") +} + +func socketFromConfigFile(configPath string, named bool) (string, error) { + if !named { + p, err := config.DefaultPath() + if err != nil { + return localapi.DefaultSocketPath, nil + } + configPath = p + } + + // The config is loaded only to find the socket path. Loading logs lines that belong + // to the daemon, not to someone running a read only command, and mixing them into + // the CLI's output would corrupt a -json result, so they are discarded. + c := config.NewC(slog.New(slog.DiscardHandler)) + if err := c.Load(configPath); err != nil { + if !named { + return localapi.DefaultSocketPath, nil + } + // The operating system's error, as it came: permission denied on a root daemon's + // config says what to do. + return "", fmt.Errorf("nebula: could not read %s: %w", configPath, err) + } + + // A daemon running this config serves no socket; dialing the path would only say + // that nothing is there. + if !localapi.ControlEnabled(c) { + return "", fmt.Errorf("nebula: the control socket is disabled in %s by %s", configPath, localapi.CtlEnabledKey) + } + + // The same call the daemon makes, so the two agree on the path and on what makes a + // path unusable. + p, err := localapi.SocketPathFromConfig(c) + if err != nil { + return "", fmt.Errorf("nebula: %w", err) + } + + return p, nil +} diff --git a/cmd/internal/ctl/ctl_test.go b/cmd/internal/ctl/ctl_test.go new file mode 100644 index 00000000..b42a9d98 --- /dev/null +++ b/cmd/internal/ctl/ctl_test.go @@ -0,0 +1,294 @@ +package ctl + +import ( + "encoding/json" + "flag" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/slackhq/nebula/commands" + "github.com/slackhq/nebula/localapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestResolveSocketReportsAnUnreadableConfig: an operator who named a config must hear +// that it could not be read, rather than have the command steered anywhere else. +func TestResolveSocketReportsAnUnreadableConfig(t *testing.T) { + missing := filepath.Join(t.TempDir(), "typo.yml") + + _, err := resolveSocket("", missing) + require.Error(t, err) + assert.Contains(t, err.Error(), missing) +} + +// TestResolveSocketGuessedConfigNeedNotBeReadable: a config nobody named is a guess, and +// one that cannot be read, a root daemon's config read by a member of the socket's +// group, means the default path rather than an error. +func TestResolveSocketGuessedConfigNeedNotBeReadable(t *testing.T) { + missing := filepath.Join(t.TempDir(), "config.yml") + + p, err := socketFromConfigFile(missing, false) + require.NoError(t, err) + assert.Equal(t, localapi.DefaultSocketPath, p) + + _, err = socketFromConfigFile(missing, true) + require.ErrorContains(t, err, missing) +} + +// nowhereSocket is a socket name nothing listens on, spelled the way the platform +// requires: a full pipe name on windows, an absolute path elsewhere. +var nowhereSocket = map[bool]string{true: `\\.\pipe\nebula-nowhere`, false: "/nowhere/ctl.sock"}[runtime.GOOS == "windows"] + +// namedSocketConfig is a config that names nowhereSocket as the control socket. Single +// quotes keep a pipe name's backslashes literal in YAML. +var namedSocketConfig = "local_api:\n control:\n socket: '" + nowhereSocket + "'\n" + +// TestResolveSocketTrustsANamedConfig: a config the operator named yields the socket it +// implies even when nothing is listening there, so a dial failure names that socket. +func TestResolveSocketTrustsANamedConfig(t *testing.T) { + cfg := filepath.Join(t.TempDir(), "config.yml") + require.NoError(t, os.WriteFile(cfg, []byte(namedSocketConfig), 0600)) + + p, err := resolveSocket("", cfg) + require.NoError(t, err) + assert.Equal(t, nowhereSocket, p) +} + +// TestResolveSocketSaysTheSocketIsDisabled: a config that turns the control socket off +// is answered with that, not with a dial to a path nothing serves. -socket still wins. +func TestResolveSocketSaysTheSocketIsDisabled(t *testing.T) { + cfg := filepath.Join(t.TempDir(), "config.yml") + require.NoError(t, os.WriteFile(cfg, []byte("local_api:\n control:\n enabled: false\n"), 0600)) + + _, err := resolveSocket("", cfg) + require.ErrorContains(t, err, "the control socket is disabled in "+cfg+" by local_api.control.enabled") + + p, err := resolveSocket("/explicit.sock", cfg) + require.NoError(t, err) + assert.Equal(t, "/explicit.sock", p) +} + +// TestResolveSocketUsesTheDefaultWithoutAnOverride: a config that does not name the +// socket means the default path, the same one the daemon serves with that config, and +// no config at all means the default too, since that is where a daemon started without +// an override serves. Nothing is searched for. +func TestResolveSocketUsesTheDefaultWithoutAnOverride(t *testing.T) { + cfg := filepath.Join(t.TempDir(), "config.yml") + require.NoError(t, os.WriteFile(cfg, []byte("static_host_map: {}\n"), 0600)) + + p, err := resolveSocket("", cfg) + require.NoError(t, err) + assert.Equal(t, localapi.DefaultSocketPath, p) + + // The test binary has no config next to it. + p, err = resolveSocket("", "") + require.NoError(t, err) + assert.Equal(t, localapi.DefaultSocketPath, p) +} + +func TestResolveSocketFlagWins(t *testing.T) { + p, err := resolveSocket("/explicit.sock", "/does/not/matter.yml") + require.NoError(t, err) + assert.Equal(t, "/explicit.sock", p) +} + +// TestParseCommandReportsWhatFlagSaid covers the three ways a flag line goes wrong, which +// are different mistakes: an unknown flag, a valid flag with a missing value, and a +// valid flag with a value it cannot parse. +func TestParseCommandReportsWhatFlagSaid(t *testing.T) { + schema := []commands.CommandSchema{{ + Name: "change-remote", + Flags: []commands.FlagSchema{ + {Name: "address"}, + {Name: "by-index", Bool: true}, + }, + }} + + cases := map[string]struct { + argv []string + want string + }{ + "missing value": {[]string{"change-remote", "-address"}, "flag needs an argument: -address"}, + "bad bool": {[]string{"change-remote", "-by-index=maybe"}, "invalid boolean value \"maybe\" for -by-index"}, + "unknown flag": {[]string{"change-remote", "-nope"}, "flag provided but not defined: -nope"}, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + _, _, err := parseCommand(schema, tc.argv) + require.Error(t, err) + require.ErrorContains(t, err, "nebula: change-remote: "+tc.want) + require.ErrorContains(t, err, "see 'nebula ctl change-remote -h'") + assert.NotContains(t, err.Error(), "does not take") + }) + } +} + +// TestParseCommandOffPresentationFlagIsNoPreference: the daemon treats -json=false as +// no preference, so nebula ctl must too, or the same line renders differently on the +// two surfaces. Only a true value asks for JSON, and -pretty implies it. +func TestParseCommandOffPresentationFlagIsNoPreference(t *testing.T) { + schema := []commands.CommandSchema{{ + Name: "device-info", + Flags: []commands.FlagSchema{ + {Name: commands.FlagJSON, Bool: true, Default: "false"}, + {Name: commands.FlagPretty, Bool: true, Default: "false"}, + }, + }} + + for _, tc := range []struct { + argv []string + want commands.Format + wire string + }{ + {[]string{"device-info"}, commands.FormatUnset, "text"}, + {[]string{"device-info", "-json=false"}, commands.FormatUnset, "text"}, + {[]string{"device-info", "-json"}, commands.FormatJSON, "json"}, + {[]string{"device-info", "-pretty"}, commands.FormatPrettyJSON, "json"}, + } { + t.Run(strings.Join(tc.argv[1:], " "), func(t *testing.T) { + req, printedHelp, err := parseCommand(schema, tc.argv) + require.NoError(t, err) + assert.False(t, printedHelp) + assert.Equal(t, tc.want, req.Format) + assert.Equal(t, tc.wire, wireFormat(req.Format)) + assert.Nil(t, req.Params, "presentation flags are never sent as parameters") + }) + } +} + +// TestParseCommandReadsTheLineAsTheDaemonDoes: nebula ctl parses the line with the same +// rules as the daemon, so an operand after "--" reaches the command on both and a +// negative number before it is the flag it would be at an ssh prompt. +func TestParseCommandReadsTheLineAsTheDaemonDoes(t *testing.T) { + schema := []commands.CommandSchema{{Name: "set"}} + + req, printedHelp, err := parseCommand(schema, []string{"set", "--", "-1"}) + require.NoError(t, err) + assert.False(t, printedHelp) + assert.Equal(t, commands.Name("set"), req.name) + assert.Nil(t, req.Params) + assert.Equal(t, []string{"-1"}, req.Args) + + _, _, err = parseCommand(schema, []string{"set", "-1"}) + require.ErrorContains(t, err, "flag provided but not defined: -1") +} + +// TestOutputRendersForTheFormatAsked: what the daemon answered is read by the format the +// line asked for, never by sniffing the bytes. A text answer is the command's rendering, +// printed as it came; a JSON answer whose value is a string stays quoted, since to the +// script reading it that string is the JSON. +func TestOutputRendersForTheFormatAsked(t *testing.T) { + text := &localapi.Response{Text: []byte("Log level is: info\n")} + assert.Equal(t, "Log level is: info\n", output(text, commands.FormatUnset)) + assert.Equal(t, "Log level is: info\n", output(text, commands.FormatText)) + + str := &localapi.Response{Result: json.RawMessage(`"some string"`)} + assert.Equal(t, `"some string"`, output(str, commands.FormatJSON)) + assert.Equal(t, `"some string"`, output(str, commands.FormatPrettyJSON)) + + // Keys stay in the order the daemon wrote them and a uint64 past 2^53 is not rounded, + // which decoding into `any` and encoding again would do. + obj := &localapi.Response{Result: json.RawMessage(`{"z":18446744073709551615,"a":{"y":[1,2],"b":true}}`)} + //nolint:testifylint // the layout is what is under test, which JSONEq would not see + assert.Equal(t, `{"z":18446744073709551615,"a":{"y":[1,2],"b":true}}`, output(obj, commands.FormatJSON)) + //nolint:testifylint // as above + assert.Equal(t, "{\n \"z\": 18446744073709551615,\n \"a\": {\n \"y\": [\n 1,\n 2\n ],\n \"b\": true\n }\n}", output(obj, commands.FormatPrettyJSON)) + + bad := &localapi.Response{Result: json.RawMessage(`{"a":`)} + assert.Equal(t, `{"a":`, output(bad, commands.FormatPrettyJSON), "what cannot be indented is printed as it came") + + none := &localapi.Response{} + assert.Empty(t, output(none, commands.FormatJSON)) + assert.Empty(t, output(none, commands.FormatUnset)) +} + +// TestSplitLineLiftsOwnFlagsFromAnywhere: -config and -socket are the client's wherever +// they appear, before the command or after it, in either spelling, so the two orders +// mean the same line. Everything else after the command word stays in order for the +// command's own parse, including flags and operands that happen to look like values. +func TestSplitLineLiftsOwnFlagsFromAnywhere(t *testing.T) { + newFS := func() *flag.FlagSet { + fs := flag.NewFlagSet("nebula ctl", flag.ContinueOnError) + fs.String(commands.FlagConfig, "", "") + fs.String(commands.FlagSocket, "", "") + return fs + } + + for _, tc := range []struct { + args []string + wantOwn []string + wantCmd []string + }{ + {nil, nil, nil}, + {[]string{"identity"}, nil, []string{"identity"}}, + {[]string{"-config", "x", "identity"}, []string{"-config", "x"}, []string{"identity"}}, + {[]string{"identity", "-config", "x"}, []string{"-config", "x"}, []string{"identity"}}, + {[]string{"identity", "-config=x"}, []string{"-config=x"}, []string{"identity"}}, + {[]string{"identity", "--socket", "s"}, []string{"--socket", "s"}, []string{"identity"}}, + {[]string{"-socket", "s", "identity", "-config", "x"}, []string{"-socket", "s", "-config", "x"}, []string{"identity"}}, + // The command's flags and operands are left where they are, around the lifted ones. + {[]string{"print-tunnel", "10.0.0.1", "-config", "x", "-json"}, []string{"-config", "x"}, []string{"print-tunnel", "10.0.0.1", "-json"}}, + {[]string{"change-remote", "-address", "1.2.3.4:5", "-socket", "s", "10.0.0.1"}, []string{"-socket", "s"}, []string{"change-remote", "-address", "1.2.3.4:5", "10.0.0.1"}}, + // -h before the command is the client's usage; after it, the command's help. + {[]string{"-h"}, []string{"-h"}, nil}, + {[]string{"identity", "-h"}, nil, []string{"identity", "-h"}}, + // An unknown flag before the command is the client's to report, and does not + // swallow the command word as its value. + {[]string{"-nope", "identity"}, []string{"-nope"}, []string{"identity"}}, + // A -config with nothing after it is left for fs.Parse to report. + {[]string{"identity", "-config"}, []string{"-config"}, []string{"identity"}}, + // "--" before the command is consumed, as fs.Parse consumed it; after the command + // it belongs to the line and ends the lifting. + {[]string{"--", "identity", "-config", "x"}, []string{"-config", "x"}, []string{"identity"}}, + {[]string{"identity", "--", "-config", "x"}, nil, []string{"identity", "--", "-config", "x"}}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + own, cmd := splitLine(newFS(), tc.args) + assert.Equal(t, tc.wantOwn, own) + assert.Equal(t, tc.wantCmd, cmd) + }) + } +} + +// TestTrailingHandlesWordsAfterTheDaemonsFlags: what flag.Parse leaves behind is never +// a daemon to start. A leading ctl runs the client on the rest, with the daemon's +// -config carried over; anything else is a mistyped subcommand. Only -h is run here, +// since the client's help needs no daemon: it proves the words reach Run and that a +// carried -config is accepted by it. +func TestTrailingHandlesWordsAfterTheDaemonsFlags(t *testing.T) { + code, done := Trailing(nil, "") + assert.False(t, done) + assert.Equal(t, 0, code) + + code, done = Trailing([]string{"clt"}, "") + assert.True(t, done) + assert.Equal(t, 1, code) + + code, done = Trailing([]string{Word, "-h"}, "") + assert.True(t, done) + assert.Equal(t, 0, code) + + code, done = Trailing([]string{Word, "-h"}, "/etc/nebula/config.yml") + assert.True(t, done) + assert.Equal(t, 0, code) +} + +// TestSubcommandOnlyReadsTheFirstWord: a flag first means the line is the daemon's to +// parse, and whatever follows the flags is Trailing's to judge. +func TestSubcommandOnlyReadsTheFirstWord(t *testing.T) { + _, done := Subcommand([]string{"nebula", "-config", "x", Word}) + assert.False(t, done) + + code, done := Subcommand([]string{"nebula", "clt"}) + assert.True(t, done) + assert.Equal(t, 1, code) + + code, done = Subcommand([]string{"nebula", Word, "-h"}) + assert.True(t, done) + assert.Equal(t, 0, code) +} diff --git a/cmd/internal/ctl/ctl_unix_test.go b/cmd/internal/ctl/ctl_unix_test.go new file mode 100644 index 00000000..c3adb361 --- /dev/null +++ b/cmd/internal/ctl/ctl_unix_test.go @@ -0,0 +1,42 @@ +//go:build !windows + +package ctl + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/slackhq/nebula/localapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestResolveSocketDiagnosesAnOverlongOverride: the CLI applies the daemon's own rule to +// the override, so it says what the daemon's log says rather than dialing a path the +// kernel would truncate and reporting the generic failure. The rule is sun_path's, which +// a named pipe does not have, so this is a unix test. +func TestResolveSocketDiagnosesAnOverlongOverride(t *testing.T) { + cfg := filepath.Join(t.TempDir(), "config.yml") + long := "/" + strings.Repeat("x", 120) + require.NoError(t, os.WriteFile(cfg, []byte("local_api:\n control:\n socket: "+long+"\n"), 0600)) + + _, err := resolveSocket("", cfg) + require.Error(t, err) + require.ErrorContains(t, err, "too long for this platform") + assert.ErrorContains(t, err, localapi.CtlSocketKey) +} + +// TestResolveSocketRefusesARelativeOverride: the CLI applies the daemon's rule here too, +// so a relative path in the config is reported as the daemon's log reports it rather +// than resolved against wherever nebula ctl happened to be run from. +func TestResolveSocketRefusesARelativeOverride(t *testing.T) { + cfg := filepath.Join(t.TempDir(), "config.yml") + require.NoError(t, os.WriteFile(cfg, []byte("local_api:\n control:\n socket: run/ctl.sock\n"), 0600)) + + _, err := resolveSocket("", cfg) + require.Error(t, err) + require.ErrorContains(t, err, "must be an absolute path") + assert.ErrorContains(t, err, localapi.CtlSocketKey) +} diff --git a/cmd/nebula-service/main.go b/cmd/nebula-service/main.go index d385bb8c..da5f5b4a 100644 --- a/cmd/nebula-service/main.go +++ b/cmd/nebula-service/main.go @@ -8,7 +8,9 @@ import ( "strings" "github.com/slackhq/nebula" + "github.com/slackhq/nebula/cmd/internal/ctl" "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/localapi" "github.com/slackhq/nebula/logging" "github.com/slackhq/nebula/util" ) @@ -32,6 +34,11 @@ func init() { } func main() { + // commands are either `nebula ctl ...` or `nebula - ...` + if code, done := ctl.Subcommand(os.Args); done { + os.Exit(code) + } + serviceFlag := flag.String("service", "", "Control the system service.") configPath := flag.String("config", "", "Path to either a file or directory to load configuration from") configTest := flag.Bool("test", false, "Test the config and print the end result. Non zero exit indicates a faulty config") @@ -40,6 +47,12 @@ func main() { flag.Parse() + // A word after the daemon's flags is `ctl` in the other place an operator puts it, + // or a mistyped subcommand, and either way not a daemon to start; see Trailing. + if code, done := ctl.Trailing(flag.Args(), *configPath); done { + os.Exit(code) + } + if *printVersion { fmt.Printf("Version: %s\n", Build) os.Exit(0) @@ -104,9 +117,17 @@ func main() { os.Exit(1) } + ctl := localapi.Wire(ctrl.Context(), l, c, ctrl.Commands()) + go ctrl.ShutdownBlock() - if err := ctrl.Wait(); err != nil { + err := ctrl.Wait() + + // os.Exit skips deferred calls, so the socket is removed here, before either exit + // below, rather than left to a deferred Stop that would never run. + ctl.Stop() + + if err != nil { l.Error("Nebula stopped due to fatal error", "error", err) os.Exit(2) } diff --git a/cmd/nebula-service/service.go b/cmd/nebula-service/service.go index 9ef8cd5a..b34801fa 100644 --- a/cmd/nebula-service/service.go +++ b/cmd/nebula-service/service.go @@ -8,6 +8,7 @@ import ( "github.com/kardianos/service" "github.com/slackhq/nebula" "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/localapi" "github.com/slackhq/nebula/logging" ) @@ -17,6 +18,7 @@ type program struct { configPath *string build string control *nebula.Control + ctl *localapi.Server } func (p *program) Start(s service.Service) error { @@ -50,10 +52,16 @@ func (p *program) Start(s service.Service) error { return err } + // Now that we're started, stand up the local control socket. + p.ctl = localapi.Wire(p.control.Context(), l, c, p.control.Commands()) + // Nebula can stop itself on a fatal packet reader error, make sure to log it if it happens. go func() { if err := p.control.Wait(); err != nil { logger.Error(fmt.Sprintf("Nebula stopped due to fatal error: %v", err)) + // os.Exit skips deferred calls and races the socket's own context watcher, + // so the socket is removed here, as cmd/nebula does. + p.ctl.Stop() os.Exit(2) } }() @@ -63,6 +71,11 @@ func (p *program) Start(s service.Service) error { func (p *program) Stop(s service.Service) error { logger.Info("Nebula service stopping.") + + if p.ctl != nil { + p.ctl.Stop() + } + if p.control == nil { return nil } diff --git a/cmd/nebula/main.go b/cmd/nebula/main.go index 31da63e4..295dc767 100644 --- a/cmd/nebula/main.go +++ b/cmd/nebula/main.go @@ -8,7 +8,9 @@ import ( "strings" "github.com/slackhq/nebula" + "github.com/slackhq/nebula/cmd/internal/ctl" "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/localapi" "github.com/slackhq/nebula/logging" "github.com/slackhq/nebula/util" ) @@ -32,6 +34,11 @@ func init() { } func main() { + // commands are either `nebula ctl ...` or `nebula - ...` + if code, done := ctl.Subcommand(os.Args); done { + os.Exit(code) + } + configPath := flag.String("config", "", "Path to either a file or directory to load configuration from") configTest := flag.Bool("test", false, "Test the config and print the end result. Non zero exit indicates a faulty config") printVersion := flag.Bool("version", false, "Print version") @@ -39,6 +46,12 @@ func main() { flag.Parse() + // A word after the daemon's flags is `ctl` in the other place an operator puts it, + // or a mistyped subcommand, and either way not a daemon to start; see Trailing. + if code, done := ctl.Trailing(flag.Args(), *configPath); done { + os.Exit(code) + } + if *printVersion { fmt.Printf("Version: %s\n", Build) os.Exit(0) @@ -90,10 +103,18 @@ func main() { os.Exit(1) } + ctl := localapi.Wire(ctrl.Context(), l, c, ctrl.Commands()) + go ctrl.ShutdownBlock() notifyReady(l) - if err := ctrl.Wait(); err != nil { + err := ctrl.Wait() + + // os.Exit skips deferred calls, so the socket is removed here, before either exit + // below, rather than left to a deferred Stop that would never run. + ctl.Stop() + + if err != nil { l.Error("Nebula stopped due to fatal error", "error", err) os.Exit(2) } diff --git a/commands.go b/commands.go index 67a783cb..17c6ae23 100644 --- a/commands.go +++ b/commands.go @@ -1,48 +1,33 @@ package nebula import ( - "bytes" "encoding/json" "flag" "fmt" + "io" "log/slog" "maps" "net/netip" "sort" "strings" + "github.com/slackhq/nebula/cert" "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. +// reads; the ssh debug server is a consumer of it, not its owner, which is why this +// is not ssh.go. See the commands package for what a Command is. type listHostMapFlags struct { - Json bool - Pretty bool ByIndex bool } -// 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 printTunnelFlags struct { - Pretty bool + Raw bool } type changeRemoteFlags struct { @@ -57,195 +42,210 @@ type createTunnelFlags struct { Address string } -type deviceInfoFlags struct { - Json bool - Pretty bool +// lighthouseInfo is one entry of list-lighthouse-addrmap. +type lighthouseInfo struct { + VpnAddr string `json:"vpnAddr"` + Addrs *CacheMap `json:"addrs"` +} + +// deviceInfo is the result of device-info. +type deviceInfo struct { + Name string `json:"name"` + Cidr []netip.Prefix `json:"cidr"` +} + +// certPEM is the result of print-cert -raw. The PEM form is a different thing to emit, +// not a different formatting of the certificate, so it is its own shape. +type certPEM struct { + PEM string `json:"pem"` +} + +// Result shapes for print-relays. Field names are the JSON keys; changing them would +// change the output. +type relayFor struct { + Error string `json:",omitempty"` + Type string + State string + PeerAddr netip.Addr + LocalIndex uint32 + RemoteIndex uint32 + RelayedThrough []netip.Addr +} + +type relayOutput struct { + NebulaAddr netip.Addr + RelayForAddrs []relayFor +} + +type relaysOutput struct { + Relays []*relayOutput +} + +type versionInfo struct { + Version string `json:"version"` +} + +type logLevelInfo struct { + Level string `json:"level"` +} + +type logFormatInfo struct { + Format string `json:"format"` +} + +// defineListHostMapFlags declares the flags list-hostmap and list-pending-hostmap share. +func defineListHostMapFlags(fl *flag.FlagSet, s *listHostMapFlags) { + fl.BoolVar(&s.ByIndex, "by-index", false, "gets all hosts in the hostmap from the index table") } func attachCommands(l *slog.Logger, c *config.C, reg *commands.Registry, f *Interface) { - reg.Register(&commands.Command{ + reg.Register(commands.Flagged(&commands.Command{ Name: commands.ListHostmap, ShortDescription: "List all known previously connected hosts", - Flags: listHostMapFlagSet, - Callback: func(fs any, a []string, w commands.StringWriter) error { - return listHostMapCommand(f.hostMap, fs, w) - }, - }) + Text: commands.Typed(textListHostMap), + }, defineListHostMapFlags, func(s *listHostMapFlags, a []string) (commands.Result, error) { + return runListHostMap(f.hostMap, s) + })) - reg.Register(&commands.Command{ + reg.Register(commands.Flagged(&commands.Command{ Name: commands.ListPendingHostmap, ShortDescription: "List all handshaking hosts", - Flags: listHostMapFlagSet, - Callback: func(fs any, a []string, w commands.StringWriter) error { - return listHostMapCommand(f.handshakeManager, fs, w) - }, - }) + Text: commands.Typed(textListHostMap), + }, defineListHostMapFlags, func(s *listHostMapFlags, a []string) (commands.Result, error) { + return runListHostMap(f.handshakeManager, s) + })) reg.Register(&commands.Command{ Name: commands.ListLighthouseAddrmap, ShortDescription: "List all lighthouse map entries", - Flags: func() (*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") - return fl, &s - }, - Callback: func(fs any, a []string, w commands.StringWriter) error { - return listLighthouseAddrmapCommand(f.lightHouse, fs, w) + Run: func(fs any, a []string) (commands.Result, error) { + return runListLighthouseMap(f.lightHouse) }, + Text: commands.Typed(textListLighthouseMap), }) reg.Register(&commands.Command{ 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 reloadCommand(l, c, w) + // The line goes out before the reload: a reload that restarts the ssh server ends + // the session that asked, and this is what it sees. + Run: func(fs any, a []string) (commands.Result, error) { + return commands.Did("Reloading config"), nil + }, + Then: func() commands.Result { + return runReload(l, c) }, }) reg.Register(&commands.Command{ Name: commands.LogLevel, ShortDescription: "Gets or sets the current log level", - Callback: func(fs any, a []string, w commands.StringWriter) error { - return logLevelCommand(l, fs, a, w) + Run: func(fs any, a []string) (commands.Result, error) { + return runLogLevel(l, a) }, + Text: commands.Typed(textLogLevel), }) reg.Register(&commands.Command{ Name: commands.LogFormat, ShortDescription: "Gets or sets the current log format", - Callback: func(fs any, a []string, w commands.StringWriter) error { - return logFormatCommand(l, fs, a, w) + Run: func(fs any, a []string) (commands.Result, error) { + return runLogFormat(l, a) }, + Text: commands.Typed(textLogFormat), }) reg.Register(&commands.Command{ Name: commands.Version, ShortDescription: "Prints the currently running version of nebula", - Callback: func(fs any, a []string, w commands.StringWriter) error { - return versionCommand(f, fs, a, w) + Run: func(fs any, a []string) (commands.Result, error) { + return commands.Value(versionInfo{Version: f.version}), nil }, + Text: commands.Typed(textVersion), }) reg.Register(&commands.Command{ Name: commands.DeviceInfo, ShortDescription: "Prints information about the network device.", - Flags: func() (*flag.FlagSet, any) { - fl := flag.NewFlagSet("", flag.ContinueOnError) - 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 deviceInfoCommand(f, fs, w) + Run: func(fs any, a []string) (commands.Result, error) { + return commands.Value(runDeviceInfo(f)), nil }, + Text: commands.Typed(textDeviceInfo), }) - reg.Register(&commands.Command{ + reg.Register(commands.Flagged(&commands.Command{ Name: commands.PrintCert, 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 := 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 printCertCommand(f, fs, a, w) - }, - }) + Text: textPrintCert, + }, func(fl *flag.FlagSet, s *printCertFlags) { + fl.BoolVar(&s.Raw, "raw", false, "prints the PEM encoded certificate; with -json, an object holding it") + }, func(s *printCertFlags, a []string) (commands.Result, error) { + return runPrintCert(f, s, a) + })) reg.Register(&commands.Command{ Name: commands.PrintTunnel, ShortDescription: "Prints json details about a tunnel for the provided vpn addr", - Flags: func() (*flag.FlagSet, any) { - fl := flag.NewFlagSet("", flag.ContinueOnError) - 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 printTunnelCommand(f, fs, a, w) + Run: func(fs any, a []string) (commands.Result, error) { + return runPrintTunnel(f, a) }, + // No Text on purpose: nothing human readable exists yet, and shipping an interim + // form would invite scripts against it. ssh defaults these to JSON; ctl asks. + JSONOnly: true, }) reg.Register(&commands.Command{ Name: commands.PrintRelays, ShortDescription: "Prints json details about all relay info", - Flags: func() (*flag.FlagSet, any) { - fl := flag.NewFlagSet("", flag.ContinueOnError) - 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 printRelaysCommand(f, fs, a, w) + Run: func(fs any, a []string) (commands.Result, error) { + return commands.Value(runPrintRelays(f)), nil }, + // No Text on purpose; see print-tunnel. + JSONOnly: true, }) - reg.Register(&commands.Command{ + reg.Register(commands.Flagged(&commands.Command{ Name: commands.ChangeRemote, 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 := 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 changeRemoteCommand(f, fs, a, w) - }, - }) + }, func(fl *flag.FlagSet, s *changeRemoteFlags) { + fl.StringVar(&s.Address, "address", "", "The new remote address, ip:port") + }, func(s *changeRemoteFlags, a []string) (commands.Result, error) { + return runChangeRemote(f, s, a) + })) - reg.Register(&commands.Command{ + reg.Register(commands.Flagged(&commands.Command{ Name: commands.CloseTunnel, ShortDescription: "Closes a tunnel for the provided vpn addr", - Flags: func() (*flag.FlagSet, any) { - fl := flag.NewFlagSet("", flag.ContinueOnError) - 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 closeTunnelCommand(f, fs, a, w) - }, - }) + }, func(fl *flag.FlagSet, s *closeTunnelFlags) { + fl.BoolVar(&s.LocalOnly, "local-only", false, "Disables notifying the remote that the tunnel is shutting down") + }, func(s *closeTunnelFlags, a []string) (commands.Result, error) { + return runCloseTunnel(f, s, a) + })) - reg.Register(&commands.Command{ + reg.Register(commands.Flagged(&commands.Command{ Name: commands.CreateTunnel, ShortDescription: "Creates a tunnel for the provided vpn address", 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 := 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 createTunnelCommand(f, fs, a, w) - }, - }) + }, func(fl *flag.FlagSet, s *createTunnelFlags) { + fl.StringVar(&s.Address, "address", "", "Optionally provide a real remote address, ip:port ") + }, func(s *createTunnelFlags, a []string) (commands.Result, error) { + return runCreateTunnel(f, s, a) + })) reg.Register(&commands.Command{ Name: commands.QueryLighthouse, 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 queryLighthouseCommand(f, fs, a, w) + Run: func(fs any, a []string) (commands.Result, error) { + return runQueryLighthouse(f, a) }, + // No Text on purpose; see print-tunnel. + JSONOnly: true, }) } -func listHostMapCommand(hl controlHostLister, a any, w commands.StringWriter) error { - fs, ok := a.(*listHostMapFlags) - if !ok { - return fmt.Errorf("internal error: expected *listHostMapFlags but was %T", a) - } - +// runListHostMap computes the hostmap listing. Presentation is Render's job. +func runListHostMap(hl controlHostLister, fs *listHostMapFlags) (commands.Result, error) { var hm []ControlHostInfo if fs.ByIndex { hm = listHostMapIndexes(hl) @@ -257,39 +257,21 @@ func listHostMapCommand(hl controlHostLister, a any, w commands.StringWriter) er return hm[i].VpnAddrs[0].Compare(hm[j].VpnAddrs[0]) < 0 }) - if fs.Json || fs.Pretty { - js := json.NewEncoder(w.GetWriter()) - if fs.Pretty { - js.SetIndent("", " ") - } + return commands.Value(hm), nil +} - if err := js.Encode(hm); err != nil { +// textListHostMap is the human rendering, shared by every surface. +func textListHostMap(hm []ControlHostInfo, w io.Writer) error { + for _, h := range hm { + if _, err := fmt.Fprintf(w, "%s: %s\n", h.VpnAddrs, h.RemoteAddrs); err != nil { return err } - - } else { - for _, v := range hm { - err := w.WriteLine(fmt.Sprintf("%s: %s", v.VpnAddrs, v.RemoteAddrs)) - if err != nil { - return err - } - } } return nil } -func listLighthouseAddrmapCommand(lightHouse *LightHouse, a any, w commands.StringWriter) error { - fs, ok := a.(*listHostMapFlags) - if !ok { - return fmt.Errorf("internal error: expected *listHostMapFlags but was %T", a) - } - - type lighthouseInfo struct { - VpnAddr string `json:"vpnAddr"` - Addrs *CacheMap `json:"addrs"` - } - +func runListLighthouseMap(lightHouse *LightHouse) (commands.Result, error) { lightHouse.RLock() addrMap := make([]lighthouseInfo, len(lightHouse.addrMap)) x := 0 @@ -306,56 +288,64 @@ func listLighthouseAddrmapCommand(lightHouse *LightHouse, a any, w commands.Stri return strings.Compare(addrMap[i].VpnAddr, addrMap[j].VpnAddr) < 0 }) - if fs.Json || fs.Pretty { - js := json.NewEncoder(w.GetWriter()) - if fs.Pretty { - js.SetIndent("", " ") - } + return commands.Value(addrMap), nil +} - if err := js.Encode(addrMap); err != nil { +func textListLighthouseMap(addrMap []lighthouseInfo, w io.Writer) error { + for _, e := range addrMap { + b, err := json.Marshal(e.Addrs) + if err != nil { return err } - - } else { - for _, v := range addrMap { - b, err := json.Marshal(v.Addrs) - if err != nil { - return err - } - err = w.WriteLine(fmt.Sprintf("%s: %s", v.VpnAddr, string(b))) - if err != nil { - return err - } + if _, err := fmt.Fprintf(w, "%s: %s\n", e.VpnAddr, string(b)); err != nil { + return err } } return nil } -// 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) { +func textVersion(vi versionInfo, w io.Writer) error { + _, err := fmt.Fprintf(w, "%s\n", vi.Version) + return err +} + +// vpnAddrOperand parses the vpn address a command takes as its first operand. noun is +// how the command's messages name it, "vpn address" or "vpn addr", which the output +// golden pins per command. With no operand, or one that does not parse, ok is false and +// res is the refusal to return. +func vpnAddrOperand(a []string, noun string) (vpnAddr netip.Addr, res commands.Result, ok bool) { if len(a) == 0 { - return netip.Addr{}, commands.Fail(w, "No vpn address was provided") + return netip.Addr{}, commands.Failed("No vpn address was provided"), false } vpnAddr, err := netip.ParseAddr(a[0]) if err != nil { - return netip.Addr{}, commands.Fail(w, fmt.Sprintf("The provided %s could not be parsed: %s", noun, a[0])) + return netip.Addr{}, commands.Failed("The provided %s could not be parsed: %s", noun, a[0]), false } - return vpnAddr, nil + return vpnAddr, commands.Result{}, true } -func versionCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - return w.WriteLine(ifce.version) +// tunnelOperand is vpnAddrOperand followed by the lookup of the tunnel it names. +func tunnelOperand(ifce *Interface, a []string, noun string) (*HostInfo, commands.Result, bool) { + vpnAddr, res, ok := vpnAddrOperand(a, noun) + if !ok { + return nil, res, false + } + + hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) + if hostInfo == nil { + return nil, commands.Failed("Could not find tunnel for %s: %v", noun, a[0]), false + } + + return hostInfo, commands.Result{}, true } -func queryLighthouseCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - vpnAddr, err := parseVpnAddr(a, "vpn address", w) - if err != nil { - return err +func runQueryLighthouse(ifce *Interface, a []string) (commands.Result, error) { + vpnAddr, res, ok := vpnAddrOperand(a, "vpn address") + if !ok { + return res, nil } var cm *CacheMap @@ -363,59 +353,52 @@ func queryLighthouseCommand(ifce *Interface, fs any, a []string, w commands.Stri if rl != nil { cm = rl.CopyCache() } - return json.NewEncoder(w.GetWriter()).Encode(cm) + return commands.Value(cm), nil } -func closeTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - flags, ok := fs.(*closeTunnelFlags) +func runCloseTunnel(ifce *Interface, flags *closeTunnelFlags, a []string) (commands.Result, error) { + hostInfo, res, ok := tunnelOperand(ifce, a, "vpn address") if !ok { - return fmt.Errorf("internal error: expected *closeTunnelFlags but was %T", fs) - } - - vpnAddr, err := parseVpnAddr(a, "vpn address", w) - if err != nil { - return err - } - - hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) - if hostInfo == nil { - return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0])) + return res, nil } if !flags.LocalOnly { - ifce.sendCloseTunnel(hostInfo) + ifce.send( + header.CloseTunnel, + 0, + hostInfo.ConnectionState, + hostInfo, + []byte{}, + make([]byte, 12, 12), + make([]byte, mtu), + ) } ifce.closeTunnel(hostInfo) - return w.WriteLine("Closed") + return commands.Did("Closed"), nil } -func createTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - flags, ok := fs.(*createTunnelFlags) +func runCreateTunnel(ifce *Interface, flags *createTunnelFlags, a []string) (commands.Result, error) { + + vpnAddr, res, ok := vpnAddrOperand(a, "vpn address") if !ok { - return fmt.Errorf("internal error: expected *createTunnelFlags but was %T", fs) + return res, nil } - vpnAddr, err := parseVpnAddr(a, "vpn address", w) - if err != nil { - return err + if hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr); hostInfo != nil { + return commands.Failed("Tunnel already exists"), nil } - hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) - if hostInfo != nil { - return commands.Fail(w, "Tunnel already exists") - } - - hostInfo = ifce.handshakeManager.QueryVpnAddr(vpnAddr) - if hostInfo != nil { - return commands.Fail(w, "Tunnel already handshaking") + if hostInfo := ifce.handshakeManager.QueryVpnAddr(vpnAddr); hostInfo != nil { + return commands.Failed("Tunnel already handshaking"), nil } var addr netip.AddrPort if flags.Address != "" { + var err error addr, err = netip.ParseAddrPort(flags.Address) if err != nil { - return commands.Fail(w, "Address could not be parsed") + return commands.Failed("Address could not be parsed"), nil } } @@ -428,176 +411,141 @@ func createTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringW } }) - return w.WriteLine("Created") + return commands.Did("Created"), nil } -func changeRemoteCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - flags, ok := fs.(*changeRemoteFlags) - if !ok { - return fmt.Errorf("internal error: expected *changeRemoteFlags but was %T", fs) - } +func runChangeRemote(ifce *Interface, flags *changeRemoteFlags, a []string) (commands.Result, error) { - vpnAddr, err := parseVpnAddr(a, "vpn address", w) - if err != nil { - return err + if len(a) == 0 { + return commands.Failed("No vpn address was provided"), nil } if flags.Address == "" { - return commands.Fail(w, "No address was provided") + return commands.Failed("No address was provided"), nil } addr, err := netip.ParseAddrPort(flags.Address) if err != nil { - return commands.Fail(w, "Address could not be parsed") + return commands.Failed("Address could not be parsed"), nil } - hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) - if hostInfo == nil { - return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn address: %v", a[0])) + hostInfo, res, ok := tunnelOperand(ifce, a, "vpn address") + if !ok { + return res, nil } hostInfo.SetRemote(addr) - return w.WriteLine("Changed") + return commands.Did("Changed"), nil } -func logLevelCommand(l *slog.Logger, fs any, a []string, w commands.StringWriter) error { +func runLogLevel(l *slog.Logger, a []string) (commands.Result, error) { ctrl, ok := l.Handler().(interface { GetLevel() slog.Level SetLevel(slog.Level) }) if !ok { - return commands.Fail(w, "Log level is not reconfigurable on this logger") + return commands.Failed("Log level is not reconfigurable on this logger"), nil } - if len(a) == 0 { - return w.WriteLine(fmt.Sprintf("Log level is: %s", logging.LevelName(ctrl.GetLevel()))) + if len(a) > 0 { + level, err := logging.ParseLevel(strings.ToLower(a[0])) + if err != nil { + return commands.Failed("Unknown log level %s. Possible log levels: trace, debug, info, warn, error", a), nil + } + + ctrl.SetLevel(level) } - level, err := logging.ParseLevel(strings.ToLower(a[0])) - if err != nil { - 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()))) + return commands.Value(logLevelInfo{Level: logging.LevelName(ctrl.GetLevel())}), nil } -func logFormatCommand(l *slog.Logger, fs any, a []string, w commands.StringWriter) error { +func textLogLevel(li logLevelInfo, w io.Writer) error { + _, err := fmt.Fprintf(w, "Log level is: %s\n", li.Level) + return err +} + +func runLogFormat(l *slog.Logger, a []string) (commands.Result, error) { ctrl, ok := l.Handler().(interface { GetFormat() string SetFormat(string) error }) if !ok { - return commands.Fail(w, "Log format is not reconfigurable on this logger") + return commands.Failed("Log format is not reconfigurable on this logger"), nil } - if len(a) == 0 { - return w.WriteLine(fmt.Sprintf("Log format is: %s", ctrl.GetFormat())) + if len(a) > 0 { + if err := ctrl.SetFormat(strings.ToLower(a[0])); err != nil { + return commands.Failed("Unable to set log format: %s", err), nil + } } - if err := ctrl.SetFormat(strings.ToLower(a[0])); err != nil { - return commands.Fail(w, fmt.Sprintf("Unable to set log format: %s", err)) - } - return w.WriteLine(fmt.Sprintf("Log format is: %s", ctrl.GetFormat())) + return commands.Value(logFormatInfo{Format: ctrl.GetFormat()}), nil } -func printCertCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - args, ok := fs.(*printCertFlags) - if !ok { - return fmt.Errorf("internal error: expected *printCertFlags but was %T", fs) - } +func textLogFormat(fi logFormatInfo, w io.Writer) error { + _, err := fmt.Fprintf(w, "Log format is: %s\n", fi.Format) + return err +} - cert := ifce.pki.getCertState().GetDefaultCertificate() +func runPrintCert(ifce *Interface, args *printCertFlags, a []string) (commands.Result, error) { + + c := ifce.pki.getCertState().GetDefaultCertificate() if len(a) > 0 { - vpnAddr, err := parseVpnAddr(a, "vpn addr", w) - if err != nil { - return err + hostInfo, res, ok := tunnelOperand(ifce, a, "vpn addr") + if !ok { + return res, nil } - - hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) - if hostInfo == nil { - return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0])) + // A tunnel still handshaking, or one being torn down, has no certificate. + cc := hostInfo.GetCert() + if cc == nil { + return commands.Failed("Could not find tunnel for vpn addr: %v", a[0]), nil } - - cert = hostInfo.GetCert().Certificate - } - - if args.Json || args.Pretty { - b, err := cert.MarshalJSON() - if err != nil { - return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err)) - } - - if args.Pretty { - buf := new(bytes.Buffer) - 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) + c = cc.Certificate } if args.Raw { - b, err := cert.MarshalPEM() + b, err := c.MarshalPEM() if err != nil { - return commands.Fail(w, fmt.Sprintf("Unable to marshal the certificate: %s", err)) + return commands.Failed("Unable to marshal the certificate: %s", err), nil } - - return w.WriteBytes(b) + return commands.Value(certPEM{PEM: string(b)}), nil } - return w.WriteLine(cert.String()) + return commands.Value(c), nil } -func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - args, ok := fs.(*printTunnelFlags) - if !ok { - return fmt.Errorf("internal error: expected *printTunnelFlags but was %T", fs) +// textPrintCert takes the value untyped: print-cert returns a certificate or, with -raw, +// its PEM, and those are two things to emit rather than two renderings of one. +func textPrintCert(v any, w io.Writer) error { + switch c := v.(type) { + case certPEM: + _, err := io.WriteString(w, c.PEM) + return err + case cert.Certificate: + _, err := io.WriteString(w, c.String()+"\n") + return err + default: + return fmt.Errorf("expected a certificate, got %T", v) } +} +func runPrintRelays(ifce *Interface) any { ifce.hostMap.RLock() relays := maps.Clone(ifce.hostMap.Relays) ifce.hostMap.RUnlock() - type RelayFor struct { - Error string `json:",omitempty"` - Type string - State string - PeerAddr netip.Addr - LocalIndex uint32 - RemoteIndex uint32 - RelayedThrough []netip.Addr - } - - type RelayOutput struct { - NebulaAddr netip.Addr - RelayForAddrs []RelayFor - } - - type CmdOutput struct { - Relays []*RelayOutput - } - - co := CmdOutput{} - - enc := json.NewEncoder(w.GetWriter()) - - if args.Pretty { - enc.SetIndent("", " ") - } + co := relaysOutput{} for k, v := range relays { - ro := RelayOutput{NebulaAddr: v.vpnAddrs[0]} + ro := relayOutput{NebulaAddr: v.vpnAddrs[0]} co.Relays = append(co.Relays, &ro) relayHI := ifce.hostMap.QueryVpnAddr(v.vpnAddrs[0]) if relayHI == nil { - ro.RelayForAddrs = append(ro.RelayForAddrs, RelayFor{Error: "could not find hostinfo"}) + ro.RelayForAddrs = append(ro.RelayForAddrs, relayFor{Error: "could not find hostinfo"}) continue } for _, vpnAddr := range relayHI.relayState.CopyRelayForIps() { - rf := RelayFor{} + rf := relayFor{} r, ok := relayHI.relayState.GetRelayForByAddr(vpnAddr) if ok { t := "" @@ -610,21 +558,21 @@ func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWr t = "unknown" } - s := "" + st := "" switch r.State { case Requested: - s = "requested" + st = "requested" case Established: - s = "established" + st = "established" default: - s = "unknown" + st = "unknown" } rf.LocalIndex = r.LocalIndex rf.RemoteIndex = r.RemoteIndex rf.PeerAddr = r.PeerAddr rf.Type = t - rf.State = s + rf.State = st if rf.LocalIndex != k { rf.Error = fmt.Sprintf("hostmap LocalIndex '%v' does not match RelayState LocalIndex", k) } @@ -637,74 +585,40 @@ func printRelaysCommand(ifce *Interface, fs any, a []string, w commands.StringWr ro.RelayForAddrs = append(ro.RelayForAddrs, rf) } } - return enc.Encode(co) + + return co } -func printTunnelCommand(ifce *Interface, fs any, a []string, w commands.StringWriter) error { - args, ok := fs.(*printTunnelFlags) +func runPrintTunnel(ifce *Interface, a []string) (commands.Result, error) { + hostInfo, res, ok := tunnelOperand(ifce, a, "vpn addr") if !ok { - return fmt.Errorf("internal error: expected *printTunnelFlags but was %T", fs) + return res, nil } - vpnAddr, err := parseVpnAddr(a, "vpn addr", w) - if err != nil { - return err - } - - hostInfo := ifce.hostMap.QueryVpnAddr(vpnAddr) - if hostInfo == nil { - return commands.Fail(w, fmt.Sprintf("Could not find tunnel for vpn addr: %v", a[0])) - } - - enc := json.NewEncoder(w.GetWriter()) - if args.Pretty { - enc.SetIndent("", " ") - } - - return enc.Encode(copyHostInfo(hostInfo, ifce.hostMap.GetPreferredRanges())) + return commands.Value(copyHostInfo(hostInfo, ifce.hostMap.GetPreferredRanges())), nil } -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"` - Cidr []netip.Prefix `json:"cidr"` - }{ +func runDeviceInfo(ifce *Interface) deviceInfo { + d := deviceInfo{ Name: ifce.inside.Name(), Cidr: make([]netip.Prefix, len(ifce.inside.Networks())), } - - copy(data.Cidr, ifce.inside.Networks()) - - if flags.Json || flags.Pretty { - js := json.NewEncoder(w.GetWriter()) - if flags.Pretty { - js.SetIndent("", " ") - } - - return js.Encode(data) - } else { - return w.WriteLine(fmt.Sprintf("name=%v cidr=%v", data.Name, data.Cidr)) - } + copy(d.Cidr, ifce.inside.Networks()) + return d } -// 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") +func textDeviceInfo(d deviceInfo, w io.Writer) error { + _, err := fmt.Fprintf(w, "name=%v cidr=%v\n", d.Name, d.Cidr) + return err +} + +// runReload reloads the config and reports how it went. A reload that failed is a +// refusal, exit 1 over ssh; one that merely needs a restart is not. +func runReload(l *slog.Logger, c *config.C) commands.Result { res := c.ReloadConfig() res.Log(l) - if err := w.WriteLine(res.String()); writeErr == nil { - writeErr = err - } - if writeErr != nil { - return writeErr - } if res.Err != nil || res.Failed { - return &commands.Failure{Msg: res.String()} + return commands.Failed("%s", res.String()) } - return nil + return commands.Did("%s", res.String()) } diff --git a/commands/command.go b/commands/command.go index b849ad0e..1fbf1a30 100644 --- a/commands/command.go +++ b/commands/command.go @@ -1,36 +1,73 @@ package commands import ( + "errors" "flag" + "fmt" + "io" + "strings" ) // CommandFlags is called before help or command execution to parse command line flags. // It should return a flag.FlagSet instance and a pointer to the struct that will contain parsed flags +// +// Every call must build a fresh FlagSet and a fresh struct. Nothing may be shared between +// calls: a command runs on more than one surface and on more than one goroutine. type CommandFlags func() (*flag.FlagSet, any) -// CommandCallback is called when your command should execute. -// 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. -// 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 +// RunFunc computes a command's Result. +// +// fs is the pointer returned by Command.Flags, if there was one; -h, -help, -json and +// -pretty are reserved and handled before Run is called. a is the operands: whatever +// was not consumed as a flag. The Result is rendered by whichever surface asked: Value +// for a command that reports, Did or Failed for one that acts, Nothing for one with +// nothing to say. An error is for a failure of the command itself; a bad argument is +// Failed, so the caller hears why in its own format. +type RunFunc func(fs any, a []string) (Result, error) + +// TextFunc renders a Result's Value for a person. See Typed for declaring one against +// the type the command actually returns. +type TextFunc func(v any, w io.Writer) error type Command struct { Name Name ShortDescription string Help string - Flags CommandFlags - Callback CommandCallback + Flags CommandFlags + + // Run produces the command's Result, leaving presentation to Render. + Run RunFunc + + // Text renders a Result's Value for a person. Both surfaces call it, so a command + // reads the same over ssh as it does through the control socket. It never sees an + // outcome: Render prints those itself, the same way for every command. Leaving it + // nil declares that no human rendering of the Value exists yet, which JSONOnly says + // up front; Render then refuses text rather than inventing an interim form for + // people to script against. + Text TextFunc + + // JSONOnly declares that the command's Value has no human rendering yet, so a surface + // whose default is text asks for JSON instead, and the schema says so to a client. It + // is a statement that no such form exists, not a fallback. A refusal is still + // rendered as a sentence, since that always has a human form. + JSONOnly bool + + // Then, when set, is the second half of a command that acts: Run says what is about + // to happen and Then does it and says how it went, with its Result rendered after + // Run's and deciding the outcome. A Run that refused is the outcome, and Then does + // not run. It exists for a command whose action can cut off + // its own reply: reload restarts the ssh server when the sshd keys changed, which + // ends the session that asked, so Run's line has to be on the wire before the reload + // begins. A surface that answers in one piece, the control socket, runs Then before + // it answers and carries both. + Then func() Result } -// 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. +// Failure is the error a surface gets for a refusal the person has already been shown: +// a Run that returned Failed, a command that is not there, a line that would not parse. +// The message it rendered is the whole story, so the surface reports the failure, exit +// status 1 over ssh, and does not log it. type Failure struct { Msg string } @@ -39,45 +76,225 @@ 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 - fs any - ) - +// commandFlags returns the FlagSet a command is driven through, on any surface: the +// command's own flags, if it declares any, with the two presentation flags added. own +// reports whether the command declared any of its own, which is what tells a caller +// whether to say a parameter is unknown or that the command takes none at all. +// +// The presentation flags are defined here rather than by each command, because how a +// result is displayed is not a property of the command that produced it. This is also +// the one definition of them: the ssh line parses into it, nebula ctl parses its line +// against the schema derived from it, and help renders from that schema. They are +// command line flags only: a socket request chooses its rendering with the envelope's +// format field, and Invoke refuses them as parameters. +func commandFlags(c *Command) (fl *flag.FlagSet, fs any, own bool) { if c.Flags != nil { fl, fs = c.Flags() - if fl != nil { - // SetOutput() here in case fl.Parse dumps usage. - fl.SetOutput(w.GetWriter()) - err := fl.Parse(args) - if err != nil { - // fl.Parse has dumped error information to the user via the w writer. - return err + } + own = fl != nil + if fl == nil { + fl = flag.NewFlagSet(string(c.Name), flag.ContinueOnError) + } + + addPresentationFlags(fl) + + // What Parse prints after a bad flag, and for -h: the command's usage as help renders + // it, so a line that went wrong is answered with the same text as `help `. + fl.Usage = func() { _ = WriteCommandHelp(schemaOf(c, fl), fl.Output()) } + + return fl, fs, own +} + +// addPresentationFlags defines the two presentation flags on fl. +func addPresentationFlags(fl *flag.FlagSet) { + fl.Bool(FlagJSON, false, "outputs as json") + fl.Bool(FlagPretty, false, "pretty prints json, assumes -json") +} + +// Flagged fills in c's Flags and Run as a pair: define declares the flags on a fresh F, +// and run is handed that F. The struct the flags fill is the struct Run reads by +// construction, so no Run has to assert the type it was given or decide what to do +// when the assertion fails. +func Flagged[F any](c *Command, define func(*flag.FlagSet, *F), run func(*F, []string) (Result, error)) *Command { + c.Flags = func() (*flag.FlagSet, any) { + fl := flag.NewFlagSet("", flag.ContinueOnError) + s := new(F) + define(fl, s) + return fl, s + } + c.Run = func(fs any, a []string) (Result, error) { + s, ok := fs.(*F) + if !ok { + return Result{}, fmt.Errorf("%s was handed %T for its flags, not %T", c.Name, fs, s) + } + return run(s, a) + } + return c +} + +// isPresentationFlag reports whether name is one of the two flags commandFlags adds. +func isPresentationFlag(name string) bool { + return name == FlagJSON || name == FlagPretty +} + +// execLine runs an ssh command line against c, reporting problems to w. The line is +// parsed into the command's own FlagSet and Run reads it directly, so a flag.Value whose +// String does not re-parse, flag.Func above all, arrives as the line said. Invoke fills +// the same FlagSet through Set. +func (r *Registry) execLine(c *Command, args []string, w Output) error { + fl, fs, _ := commandFlags(c) + + // Parse reports a bad flag to this writer, with the command's usage after it. -h, + // -help and --help take the same path with nothing wrong: the usage is the whole + // answer, and Parse says so with ErrHelp. A -h that is a flag's value or follows -- + // is read as the operand it is, since ParseArgs is the one reading of the line. + fl.SetOutput(w.Err) + operands, err := ParseArgs(fl, args) + if err != nil { + if errors.Is(err, flag.ErrHelp) { + return nil + } + return err + } + + format := presentationFormat(fl) + + // An error from Run is something the command could not do rather than a refusal it + // chose, but the person at the prompt still hears it; the surface logs it too. + res, err := c.Run(fs, operands) + if err != nil { + _, _ = fmt.Fprintln(w.Err, err.Error()) + return err + } + + // Run's reply goes out first, and then the action, if the command has one and Run + // did not refuse, whose Result is the outcome. In JSON only the outcome travels: + // the promise is for a person to read, and a program expects one document. + if c.Then == nil || !res.OK || r.formatFor(c, res, format) == FormatText { + if err := r.renderLine(c, res, format, w); err != nil { + return err + } + } + if c.Then != nil && res.OK { + res = c.Then() + if err := r.renderLine(c, res, format, w); err != nil { + return err + } + } + + if !res.OK { + return &Failure{Msg: res.Reason} + } + return nil +} + +// formatFor is the format a Result is rendered in. The line wins when it asked for one; +// otherwise the surface that owns the registry decides, from the command and its +// Result, so each surface can keep its own default. +func (r *Registry) formatFor(c *Command, res Result, format Format) Format { + if format == FormatUnset { + return r.defaultFormatFor(c, res) + } + return format +} + +// renderLine writes one Result of a command line to w; see formatFor. An empty Result +// writes nothing. +func (r *Registry) renderLine(c *Command, res Result, format Format, w Output) error { + if res.Empty() { + return nil + } + + if err := Render(c, res, r.formatFor(c, res, format), w.Out); err != nil { + if errors.Is(err, ErrNoTextRendering) { + _, _ = fmt.Fprintln(w.Err, err.Error()) + } + return err + } + return nil +} + +// ParseArgs parses args with fl and returns the operands. +// +// It differs from fl.Parse in two ways, and both surfaces parse through here so a line +// means the same typed at an ssh prompt or to nebula ctl. +// +// Flags and operands may come in any order, which is the one way it differs from +// fl.Parse. Parse stops at the first operand, so `print-tunnel 10.0.0.1 -json` would +// reach the command with "-json" as a second operand and the flag silently dropped. Here +// an operand ends nothing: parsing resumes after it, and only "--" ends the flags, with +// everything after it an operand. What counts as a flag is Parse's rule: every argument +// that starts with "-", a negative number included, so one that is meant as an operand +// goes after "--". +func ParseArgs(fl *flag.FlagSet, args []string) ([]string, error) { + operands := make([]string, 0, len(args)) + + for len(args) > 0 { + // Walk the flags the way Parse will, to the first argument it would not read as a + // flag: an operand or the terminator. Everything before it is Parse's. + end := len(args) + for i := 0; i < len(args); { + a := args[i] + if a == "--" { + if err := fl.Parse(args[:i]); err != nil { + return nil, err + } + return append(operands, args[i+1:]...), nil + } + + if len(a) < 2 || a[0] != '-' { + end = i + break + } + + f, hasValue := LookupArg(fl, a) + if f == nil { + // An undefined flag: leave it for Parse to report. + i++ + continue + } + + if hasValue || IsBoolFlag(f) { + i++ + } else { + // The flag's value is the next argument, whatever it looks like. + i += 2 } - args = fl.Args() } + + if err := fl.Parse(args[:end]); err != nil { + return nil, err + } + + if end == len(args) { + break + } + + operands = append(operands, args[end]) + args = args[end+1:] } - return c.Callback(fs, args, w) + return operands, nil } -func checkHelpArgs(args []string) bool { - for _, a := range args { - if a == "-h" || a == "-help" { - return true - } +// LookupArg reads arg the way Parse does: one or two leading dashes, the flag's name, and +// an optional "=value". f is the flag of fl by that name, nil when fl has none, and +// hasValue reports that the value came with the argument rather than in the one after it. +// It is the one reading of an argument's shape: ParseArgs walks a line with it, and +// nebula ctl lifts its own flags out of a line with it, so the two cannot disagree about +// which word is a flag's value. +func LookupArg(fl *flag.FlagSet, arg string) (f *flag.Flag, hasValue bool) { + name := strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-") + if eq := strings.IndexByte(name, '='); eq >= 0 { + name, hasValue = name[:eq], true } + return fl.Lookup(name), hasValue +} - return false +// IsBoolFlag reports whether f is satisfied by its presence alone and never reads the +// argument after it as its value. The flag package marks these with an unexported +// interface, which is the only way to tell. +func IsBoolFlag(f *flag.Flag) bool { + bf, ok := f.Value.(interface{ IsBoolFlag() bool }) + return ok && bf.IsBoolFlag() } diff --git a/commands/command_test.go b/commands/command_test.go new file mode 100644 index 00000000..6cbd6195 --- /dev/null +++ b/commands/command_test.go @@ -0,0 +1,101 @@ +package commands + +import ( + "bytes" + "flag" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestParseArgsAllowsFlagsAfterOperands pins the one way ParseArgs departs from fl.Parse, +// that a flag may follow an operand, and that it departs in no other: a negative number +// is a flag, as Parse reads it, and goes after "--" when it is meant as an operand. +func TestParseArgsAllowsFlagsAfterOperands(t *testing.T) { + cases := map[string]struct { + args []string + wantOps []string + wantN string + wantV bool + wantJSON bool + wantErr string + }{ + "flag after operand": {args: []string{"x", "-json"}, wantOps: []string{"x"}, wantJSON: true}, + "flags around": {args: []string{"-v", "x", "-n", "1", "y"}, wantOps: []string{"x", "y"}, wantV: true, wantN: "1"}, + "value after operand": {args: []string{"x", "-n", "-1"}, wantOps: []string{"x"}, wantN: "-1"}, + "equals value": {args: []string{"-n=-1", "x"}, wantOps: []string{"x"}, wantN: "-1"}, + "terminator after": {args: []string{"x", "--", "-json", "-1"}, wantOps: []string{"x", "-json", "-1"}}, + "terminator": {args: []string{"--", "-json"}, wantOps: []string{"-json"}}, + "defined numeric": {args: []string{"-2", "x"}, wantOps: []string{"x"}, wantV: true}, + "negative number": {args: []string{"-1"}, wantErr: "flag provided but not defined: -1"}, + "bad flag after": {args: []string{"x", "-nope"}, wantErr: "flag provided but not defined: -nope"}, + "undefined flag": {args: []string{"-nope", "x"}, wantErr: "flag provided but not defined: -nope"}, + "missing value": {args: []string{"-n"}, wantErr: "flag needs an argument: -n"}, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + fl := flag.NewFlagSet("t", flag.ContinueOnError) + fl.SetOutput(&bytes.Buffer{}) + n := fl.String("n", "", "") + v := fl.Bool("v", false, "") + two := fl.Bool("2", false, "a flag that happens to be a number") + asJSON := fl.Bool(FlagJSON, false, "") + + ops, err := ParseArgs(fl, tc.args) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.wantOps, ops) + assert.Equal(t, tc.wantN, *n) + assert.Equal(t, tc.wantV, *v || *two) + assert.Equal(t, tc.wantJSON, *asJSON) + }) + } +} + +// TestDispatchFlagAfterOperand: the same through a registered command with no Flags of +// its own, which only has the presentation flags to find. +func TestDispatchFlagAfterOperand(t *testing.T) { + out := &bytes.Buffer{} + require.NoError(t, testRegistry().DispatchArgs([]string{"served", "x", "-json"}, MergedOutput(out))) + assert.Equal(t, "\"read x\"\n", out.String()) +} + +// TestLookupArg pins the reading of an argument's shape that ParseArgs and nebula ctl +// share. +func TestLookupArg(t *testing.T) { + fl := flag.NewFlagSet("t", flag.ContinueOnError) + fl.String("n", "", "") + fl.Bool("v", false, "") + + for _, tc := range []struct { + arg string + name string + hasValue bool + }{ + {"-n", "n", false}, + {"--n", "n", false}, + {"-n=1", "n", true}, + {"--n=", "n", true}, + {"-v", "v", false}, + {"-nope", "", false}, + {"-1", "", false}, + } { + f, hasValue := LookupArg(fl, tc.arg) + if tc.name == "" { + assert.Nil(t, f, tc.arg) + } else { + require.NotNil(t, f, tc.arg) + assert.Equal(t, tc.name, f.Name, tc.arg) + } + assert.Equal(t, tc.hasValue, hasValue, tc.arg) + } + + assert.True(t, IsBoolFlag(fl.Lookup("v"))) + assert.False(t, IsBoolFlag(fl.Lookup("n"))) +} diff --git a/commands/registry.go b/commands/registry.go index 60b663ab..78be15f9 100644 --- a/commands/registry.go +++ b/commands/registry.go @@ -2,7 +2,9 @@ package commands import ( "cmp" + "flag" "fmt" + "io" "maps" "slices" "strings" @@ -11,14 +13,38 @@ import ( "github.com/anmitsu/go-shlex" ) -// Registry holds a set of commands. New is the only way to make one. +// Registry holds a set of commands. // -// It is safe for concurrent use. Commands may be registered while a surface is serving -// from the registry: the ssh server clones it per session, and a clone may overlap a -// Register. +// It is safe for concurrent use. Commands may be registered while surfaces are serving +// from the registry: the ssh server clones it per session and the control socket filters +// it per request, and either may overlap a Register. type Registry struct { mu sync.RWMutex commands map[Name]*Command + + // full is set on a filtered registry and points at the registry it was filtered + // from, so a command that's filtered can be reported as such. + full *Registry + + // reason explains, in the terms an operator cares about, why this registry withholds + // a command they can name. It is supplied by whoever built the filter, because why a + // surface declines a command is that surface's business, not the registry's. + reason string + + // DefaultFormat decides how a result is rendered when the command line named no + // format. It sees the command and its Result, and applies to values and refusals + // alike: the registry does not special-case either. The renderer is shared between + // surfaces; what each asks for by default is not, so the surface that owns this + // registry sets it. Nil means text. + DefaultFormat func(c *Command, r Result) Format +} + +// defaultFormatFor resolves the surface's default for c's Result. +func (r *Registry) defaultFormatFor(c *Command, res Result) Format { + if r.DefaultFormat == nil { + return FormatText + } + return r.DefaultFormat(c, res) } // New returns an empty Registry with only `help` available. @@ -32,15 +58,111 @@ func (r *Registry) registerHelp() { r.Register(&Command{ Name: Help, ShortDescription: "prints available commands or help for specific usage info", - Callback: func(a any, args []string, w StringWriter) error { - return r.help(args, w) + Run: func(a any, args []string) (Result, error) { + if len(args) == 0 { + return Value(r.Schema()), nil + } + + if c := r.Get(Name(args[0])); c != nil { + return Value(r.schemaFor(c)), nil + } + + if err := r.unavailable(Name(args[0])); err != nil { + return Failed("%s", err), nil + } + + return Failed("Command not available %s", args[0]), nil }, + Text: renderHelp, }) } -// 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. +// renderHelp is help's human rendering: the command list, or one command's usage. +func renderHelp(v any, w io.Writer) error { + switch h := v.(type) { + case []CommandSchema: + return writeCommandList(h, w) + case CommandSchema: + return WriteCommandHelp(h, w) + default: + return fmt.Errorf("expected a command schema, got %T", v) + } +} + +func writeCommandList(cmds []CommandSchema, w io.Writer) error { + if _, err := io.WriteString(w, "Available commands:\n"); err != nil { + return err + } + + // In the order the schema came, which is the display order; see Registry.All. + lines := make([]string, 0, len(cmds)) + for _, c := range cmds { + lines = append(lines, fmt.Sprintf("%s - %s", c.Name, c.Short)) + } + + _, err := io.WriteString(w, strings.Join(lines, "\n")+"\n\n") + return err +} + +// WriteCommandHelp renders one command's usage. It is the single rendering of a +// CommandSchema for a person: the daemon uses it for `help ` on both surfaces, +// and a client that already holds the schema uses it for `-h`, so the two cannot drift. +func WriteCommandHelp(c CommandSchema, w io.Writer) error { + if _, err := fmt.Fprintf(w, "%s - %s\n", c.Name, c.Short); err != nil { + return err + } + + if c.Help != "" { + if _, err := fmt.Fprintf(w, " %s\n", c.Help); err != nil { + return err + } + } + + // Same layout as flag.PrintDefaults, including its habit of leaving out a default + // that is the zero value. + for _, f := range c.Flags { + kind := "" + if !f.Bool { + kind = " string" + } + def := "" + if f.Default != "" && f.Default != "false" { + def = fmt.Sprintf(" (default %s)", f.Default) + } + if _, err := fmt.Fprintf(w, " -%s%s\n \t%s%s\n", f.Name, kind, f.Usage, def); err != nil { + return err + } + } + + return nil +} + +// Register adds a command that can be run by a user. +// +// It panics if the command declares a reserved flag. That is a programming error, and +// surfacing it here, at startup, is better than at the moment an operator first types +// the command. func (r *Registry) Register(c *Command) { + if c.Run == nil { + panic(fmt.Sprintf("commands: %s has no Run", c.Name)) + } + + if c.Flags != nil { + if fl, _ := c.Flags(); fl != nil { + for _, name := range reservedFlags { + if fl.Lookup(name) != nil { + panic(fmt.Sprintf("commands: %s declares reserved flag -%s", c.Name, name)) + } + } + } + } + + r.insert(c) +} + +// insert adds c without the reserved flag check, for commands that have already passed +// it on another registry. +func (r *Registry) insert(c *Command) { r.mu.Lock() defer r.mu.Unlock() r.commands[c.Name] = c @@ -68,7 +190,8 @@ func (r *Registry) Match(prefix string) []string { return cmds } -// All returns every registered command sorted by name, which is the order help lists them. +// All returns every registered command sorted by name, which is the order help lists +// them and the schema describes them. func (r *Registry) All() []*Command { r.mu.RLock() defer r.mu.RUnlock() @@ -78,104 +201,228 @@ func (r *Registry) All() []*Command { return cmds } -// 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. +// Clone returns a copy holding the same commands. +// +// help is re-registered on the copy rather than carried over, because the source's help +// is bound to the source and would list its commands, not the copy's, once a caller +// registers something on the copy. func (r *Registry) Clone() *Registry { r.mu.RLock() - c := &Registry{commands: maps.Clone(r.commands)} + out := &Registry{ + commands: maps.Clone(r.commands), + full: r.full, + reason: r.reason, + DefaultFormat: r.DefaultFormat, + } r.mu.RUnlock() - c.registerHelp() - return c + + out.registerHelp() + return out +} + +// Filter returns a filtered copy of the registry. +// +// Keep should return true for commands that should be included in the copy. +// +// reason says, to an operator who names a command the copy left out, why this surface +// does not serve it; "" leaves the registry to say only that it does not. +func (r *Registry) Filter(keep func(*Command) bool, reason string) *Registry { + out := &Registry{commands: map[Name]*Command{}, full: r, reason: reason, DefaultFormat: r.DefaultFormat} + + for _, cmd := range r.All() { + if keep(cmd) { + out.insert(cmd) + } + } + + // Bind help to the filtered set so it only advertises what this surface serves. + out.registerHelp() + return out } // 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. +// the caller to log, not to show; see RunFunc and Failure. +func (r *Registry) Dispatch(line string, w Output) error { + // A line shlex cannot split, an unbalanced quote. Reported on Err so the output + // stream stays parseable. args, err := shlex.Split(line, true) if err != nil { - return Fail(w, "could not parse the command line: "+err.Error()) + msg := "could not parse the command line: " + err.Error() + if _, err := fmt.Fprintln(w.Err, msg); err != nil { + return err + } + return &Failure{Msg: msg} } return r.DispatchArgs(args, w) } // DispatchArgs runs an already tokenized command line. The error is as for Dispatch. -func (r *Registry) DispatchArgs(args []string, w StringWriter) error { +// +// An empty line and an unknown command both end up running `help`, the first with the +// arguments help itself would take; ` -h` is answered by the command's own +// FlagSet, with the same rendering, in execLine. +func (r *Registry) DispatchArgs(args []string, w Output) error { if len(args) == 0 { - r.dumpCommands(w) - return nil + args = []string{"help"} } c := r.Get(Name(args[0])) if c == 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 { + if err := r.unavailable(Name(args[0])); err != nil { + // Saying why, rather than "did not understand", keeps the rule legible and + // points at a surface that will actually run the command. + _, _ = fmt.Fprintln(w.Err, err) return err } - r.dumpCommands(w) + // Only the first word was looked up, so only it is named. + msg := fmt.Sprintf("did not understand: %s", args[0]) + if _, err := fmt.Fprintln(w.Out, msg); err != nil { + return err + } + + if err := r.DispatchArgs([]string{"help"}, w); err != nil { + return err + } return &Failure{Msg: msg} } - if checkHelpArgs(args) { - // Through the registered help command, so -h and `help ` are one path. - return execCommand(r.Get(Help), []string{string(c.Name)}, w) - } - - return execCommand(c, args[1:], w) + return r.execLine(c, args[1:], w) } -// dumpCommands writes the list of available commands to w. -func (r *Registry) dumpCommands(w StringWriter) { - err := w.WriteLine("Available commands:") - if err != nil { - return - } - - // In the order All gives, which is the display order. - cmds := make([]string, 0) - for _, l := range r.All() { - cmds = append(cmds, fmt.Sprintf("%s - %s", l.Name, l.ShortDescription)) - } - - _ = w.Write(strings.Join(cmds, "\n") + "\n\n") -} - -// help writes the command list, or usage for the single command named in a. -func (r *Registry) help(a []string, w StringWriter) error { - // Just typed help - if len(a) == 0 { - r.dumpCommands(w) +// unavailable explains why name cannot be run here when this registry was filtered from +// one that holds it: the command exists, and this surface withholds it, for the reason +// whoever built the filter gave. It is nil when name is simply unknown, so the two cases +// read differently to an operator. +func (r *Registry) unavailable(name Name) error { + if r.full == nil { return nil } - // We are printing a specific commands help text - cmd := r.Get(Name(a[0])) - if cmd == nil { - return Fail(w, "Command not available "+a[0]) + c := r.full.Get(name) + if c == nil { + return nil } - if err := w.WriteLine(fmt.Sprintf("%s - %s", cmd.Name, cmd.ShortDescription)); err != nil { - return err + reason := r.reason + if reason == "" { + reason = "this surface does not serve it" } - if cmd.Help != "" { - if err := w.WriteLine(fmt.Sprintf(" %s", cmd.Help)); err != nil { - return err + return fmt.Errorf("%s is not available on this surface: %s", c.Name, reason) +} + +// Resolve returns the command registered under name, or says why there is none: the +// filter's reason for one this surface withholds, or unknown for a name that does not +// exist. +func (r *Registry) Resolve(name Name) (*Command, error) { + if c := r.Get(name); c != nil { + return c, nil + } + + if err := r.unavailable(name); err != nil { + return nil, err + } + + return nil, fmt.Errorf("unknown command %q", name) +} + +// setParams sets each named parameter on fl, in the order of names, and stops at the +// first that is unknown or does not parse. own says whether the command declared flags +// of its own, which decides between "takes no parameters" and "has no parameter". +// +// The presentation flags are on fl, since commandFlags put them there, but they are not +// parameters: a request has one place to choose its rendering, the envelope's format +// field, and a client that sends them here is told where that is rather than having a +// second, weaker way to say the same thing. +func setParams(fl *flag.FlagSet, name string, names []string, params map[string]string, own bool) error { + for _, k := range names { + if isPresentationFlag(k) { + return fmt.Errorf("%s is a command line flag, not a parameter; choose the rendering with the request's format field", k) } - } - if cmd.Flags != nil { - fs, _ := cmd.Flags() - if fs != nil { - fs.SetOutput(w.GetWriter()) - fs.PrintDefaults() + f := fl.Lookup(k) + if f == nil { + if !own { + return fmt.Errorf("%s takes no parameters", name) + } + return fmt.Errorf("%s has no parameter %q", name, k) + } + + v := params[k] + if err := fl.Set(k, v); err != nil { + if IsBoolFlag(f) { + // The flag package's own words for this are "parse error". + return fmt.Errorf("%s: expected a boolean, got %q", k, v) + } + return fmt.Errorf("%s: %w", k, err) } } return nil } + +// Schema describes every command in the registry, so a client can offer them without +// having been built against this daemon. +func (r *Registry) Schema() []CommandSchema { + all := r.All() + out := make([]CommandSchema, 0, len(all)) + for _, c := range all { + out = append(out, r.schemaFor(c)) + } + return out +} + +// schemaFor describes one command. It is the one derivation, for the whole schema and +// for `help ` alike. +func (r *Registry) schemaFor(c *Command) CommandSchema { + fl, _, _ := commandFlags(c) + return schemaOf(c, fl) +} + +// schemaOf describes c as driven through fl, the FlagSet commandFlags built for it. The +// presentation flags are included because a command line client parses its line +// against this and help renders from it; they are listed after the command's own, +// which is where readers expect them. Invoke refuses them as parameters. +func schemaOf(c *Command, fl *flag.FlagSet) CommandSchema { + cs := CommandSchema{Name: c.Name, Short: c.ShortDescription, Help: c.Help} + + var own, shared []FlagSchema + fl.VisitAll(func(f *flag.Flag) { + fs := FlagSchema{ + Name: f.Name, + Bool: IsBoolFlag(f), + Default: f.DefValue, + Usage: f.Usage, + } + if isPresentationFlag(f.Name) { + shared = append(shared, fs) + return + } + own = append(own, fs) + }) + cs.Flags = append(own, shared...) + + return cs +} + +// CommandSchema describes one command well enough for a client to offer it. +type CommandSchema struct { + Name Name `json:"name"` + Short string `json:"short,omitempty"` + Help string `json:"help,omitempty"` + Flags []FlagSchema `json:"flags,omitempty"` +} + +// FlagSchema describes one of a command's named parameters. +type FlagSchema struct { + Name string `json:"name"` + + // Bool reports a parameter that takes no value, which is what tells a command line + // client whether `-pretty` swallows the word after it. + Bool bool `json:"bool,omitempty"` + + Default string `json:"default,omitempty"` + Usage string `json:"usage,omitempty"` +} diff --git a/commands/registry_test.go b/commands/registry_test.go index 40ec0af7..b29f5b08 100644 --- a/commands/registry_test.go +++ b/commands/registry_test.go @@ -4,173 +4,417 @@ import ( "bytes" "errors" "flag" + "io" "strings" + "sync" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// 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 commands that write" + +// textLine is a Text renderer for test commands whose result is one string. +func textLine(v any, w io.Writer) error { + _, err := io.WriteString(w, v.(string)+"\n") + return err +} + func testRegistry() *Registry { r := New() r.Register(&Command{ Name: "served", ShortDescription: "a command this surface serves", - Callback: func(fs any, a []string, w StringWriter) error { - return w.WriteLine("read " + strings.Join(a, ",")) + Run: func(fs any, a []string) (Result, error) { + return Value("read " + strings.Join(a, ",")), nil }, + Text: textLine, }) r.Register(&Command{ Name: "withheld", ShortDescription: "a command this surface withholds", - Callback: func(fs any, a []string, w StringWriter) error { - return w.WriteLine("wrote") + Run: func(fs any, a []string) (Result, error) { + return Value("wrote"), nil }, + Text: textLine, }) return r } -// TestFlagUsageGoesToTheWriter pins that a parse failure's usage lands on the command's -// writer. -func TestFlagUsageGoesToTheWriter(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) + for _, c := range ro.All() { + names = append(names, string(c.Name)) + } + + assert.ElementsMatch(t, []string{"help", "served"}, names) +} + +// 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. +func TestRefusesWithheldByName(t *testing.T) { + out, errOut := &bytes.Buffer{}, &bytes.Buffer{} + w := Output{Out: out, Err: errOut} + + err := testRegistry().Filter(notWithheld, testReason).DispatchArgs([]string{"withheld"}, w) + + require.Error(t, err) + assert.Contains(t, err.Error(), "withheld is not available on this surface") + assert.Empty(t, out.String(), "a withheld command must produce no output") + assert.Contains(t, errOut.String(), "withheld is not available on this surface") + assert.Contains(t, errOut.String(), testReason, + "the registry should render the explanation the filter supplied, not one of its own") +} + +// TestWithheldIsNotReportedAsUnknown keeps the two cases distinct: an operator who +// mistypes and one who asked for a real command should not get the same answer. +func TestWithheldIsNotReportedAsUnknown(t *testing.T) { + out, errOut := &bytes.Buffer{}, &bytes.Buffer{} + err := testRegistry().Filter(notWithheld, testReason).DispatchArgs([]string{"no-such-command"}, Output{Out: out, Err: errOut}) + + var f *Failure + require.ErrorAs(t, err, &f, "an unknown command is a failure to the surface") + assert.Contains(t, out.String(), "did not understand: no-such-command") +} + +// paramRegistry holds one command with a bool flag and a duration flag, and reports what +// the command saw when it ran. +func paramRegistry(t *testing.T) (*Registry, *[]paramsSeen) { + t.Helper() + + var seen []paramsSeen r := New() r.Register(&Command{ - Name: "emit", - ShortDescription: "emits", + Name: "set", + ShortDescription: "sets things", Flags: func() (*flag.FlagSet, any) { fl := flag.NewFlagSet("", flag.ContinueOnError) - fl.Bool("json", false, "emit json") - return fl, nil + s := paramsSeen{} + fl.BoolVar(&s.Verbose, "verbose", false, "say more") + fl.DurationVar(&s.Wait, "wait", 0, "how long") + return fl, &s + }, + Run: func(fs any, a []string) (Result, error) { + seen = append(seen, *fs.(*paramsSeen)) + return Value("ok"), nil }, - Callback: func(fs any, a []string, w StringWriter) error { return nil }, }) - merged := &bytes.Buffer{} - require.Error(t, r.DispatchArgs([]string{"emit", "-nope"}, NewStringWriter(merged))) - assert.Contains(t, merged.String(), "flag provided but not defined") + return r, &seen } -// TestDispatchReportsUnparseableLine: a line shlex rejects, an unbalanced quote, is a -// failure the user is told about. +type paramsSeen struct { + Verbose bool + Wait time.Duration +} + +// TestInvokeReportsParametersInNameOrder: parameters arrive as an unordered object, so +// with two bad ones the error must not depend on map iteration. Name order decides. +func TestInvokeReportsParametersInNameOrder(t *testing.T) { + r, _ := paramRegistry(t) + + cases := map[string]struct { + params map[string]string + want string + }{ + "two unknown": {map[string]string{"zed": "1", "alpha": "1"}, `set has no parameter "alpha"`}, + "unknown before bad bool": {map[string]string{"aaa": "1", "verbose": "maybe"}, `set has no parameter "aaa"`}, + "bad bool before unknown": {map[string]string{"verbose": "maybe", "zzz": "1"}, `verbose: expected a boolean, got "maybe"`}, + "bad value before bad bool": {map[string]string{"wait": "soon", "verbose": "maybe"}, `verbose: expected a boolean`}, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // Enough runs that a map-order dependence would show. + for i := 0; i < 50; i++ { + _, err := Invoke(Request{Command: r.Get("set"), Params: tc.params}) + require.Error(t, err) + assert.ErrorContains(t, err, tc.want) + } + }) + } +} + +// TestInvokeWithABadParameterSetsNothing: a request with any bad parameter must not run +// the command, and must leave nothing behind for the next request to see. The good +// parameter sorts before the bad one, so a one-pass Invoke would have applied it. +func TestInvokeWithABadParameterSetsNothing(t *testing.T) { + r, seen := paramRegistry(t) + + _, err := Invoke(Request{Command: r.Get("set"), Params: map[string]string{"verbose": "true", "wait": "soon"}}) + require.ErrorContains(t, err, "wait:") + assert.Empty(t, *seen, "the command must not run with a partial parameter set") + + v, err := Invoke(Request{Command: r.Get("set")}) + require.NoError(t, err) + assert.Equal(t, "ok", v.Value) + require.Len(t, *seen, 1) + assert.Equal(t, paramsSeen{}, (*seen)[0], "nothing from the failed request carried over") + + _, err = Invoke(Request{Command: r.Get("set"), Params: map[string]string{"verbose": "true", "wait": "2s"}}) + require.NoError(t, err) + require.Len(t, *seen, 2) + assert.Equal(t, paramsSeen{Verbose: true, Wait: 2 * time.Second}, (*seen)[1]) +} + +func TestHelpListsOnlyServedCommands(t *testing.T) { + out := &bytes.Buffer{} + require.NoError(t, testRegistry().Filter(notWithheld, testReason).DispatchArgs(nil, MergedOutput(out))) + + assert.Contains(t, out.String(), "served") + assert.NotContains(t, out.String(), "withheld") +} + +// TestDispatchReportsUnparseableLine pins that a line shlex rejects says so on Err rather +// than coming back silent, so an operator with an unbalanced quote learns the quote is the +// problem instead of assuming the command is broken. func TestDispatchReportsUnparseableLine(t *testing.T) { - out := &bytes.Buffer{} - 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: ") + out, errOut := &bytes.Buffer{}, &bytes.Buffer{} + testRegistry().Dispatch(`served "unterminated`, Output{Out: out, Err: errOut}) + assert.Empty(t, out.String()) + assert.Contains(t, errOut.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) +// TestRegisterRejectsReservedFlags: a command that declares a reserved flag is refused +// at Register rather than when an operator first runs it; see reservedFlags. +func TestRegisterRejectsReservedFlags(t *testing.T) { + for _, name := range []string{"json", "pretty", "h", "help", "config", "socket"} { + t.Run(name, func(t *testing.T) { + r := New() + assert.PanicsWithValue(t, "commands: clash declares reserved flag -"+name, func() { + r.Register(&Command{ + Name: "clash", + Flags: func() (*flag.FlagSet, any) { + fl := flag.NewFlagSet("clash", flag.ContinueOnError) + fl.Bool(name, false, "") + return fl, nil + }, + Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }, + }) + }) + }) + } } -type failingWriter struct{} +// TestRegistryIsSafeForConcurrentUse runs the operations the surfaces perform, filtering +// and cloning per request or session, against a stream of registrations. It exists to +// be run under the race detector: the map underneath is not goroutine safe. +func TestRegistryIsSafeForConcurrentUse(t *testing.T) { + r := testRegistry() -func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("gone") } - -// TestAllSortsByName: registration order does not matter. -func TestAllSortsByName(t *testing.T) { - r := New() - for _, n := range []Name{"zz", Version, "aa", ChangeRemote} { - r.Register(&Command{Name: n}) + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + for j := 0; j < 50; j++ { + r.Register(&Command{ + Name: Name("late-" + strings.Repeat("a", i) + "-" + strings.Repeat("b", j)), + ShortDescription: "registered after the surfaces started", + Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }, + }) + } + }(i) } - var got []Name - for _, c := range r.All() { - got = append(got, c.Name) + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 50; j++ { + f := r.Filter(notWithheld, testReason) + _ = f.Get("served") + _ = f.Schema() + _ = r.Clone().Match("late-") + } + }() } - assert.Equal(t, []Name{"aa", ChangeRemote, Help, Version, "zz"}, got) + + wg.Wait() + + f := r.Filter(notWithheld, testReason) + assert.Len(t, f.Match("late-"), 8*50, "every late registration is visible once the writers finish") } -// TestHelpFlagAgreesWithHelpCommand pins that -h answers through the registered help -// 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) { +// TestCloneHelpSeesCloneCommands: the ssh session registers logout on its clone, and +// the source does not learn of it. Help on the clone answers for the clone, and -h on a +// command prints what `help ` does, on a clone as on the source: both render +// through WriteCommandHelp, and Clone rebinds help to the copy. +func TestCloneHelpSeesCloneCommands(t *testing.T) { r := testRegistry() c := r.Clone() c.Register(&Command{ Name: Logout, ShortDescription: "ends the session", - Callback: func(fs any, a []string, w StringWriter) error { return nil }, + Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }, }) - - flag := &bytes.Buffer{} - c.Dispatch("logout -h", NewStringWriter(flag)) - cmd := &bytes.Buffer{} - c.Dispatch("help logout", NewStringWriter(cmd)) - assert.Equal(t, cmd.String(), 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() + assert.Nil(t, r.Get(Logout), "the source is not affected") 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()) + require.NoError(t, c.DispatchArgs([]string{"help"}, MergedOutput(out))) + assert.Contains(t, out.String(), "logout") - 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()) + for _, name := range []string{"logout", "served"} { + flag := &bytes.Buffer{} + c.Dispatch(name+" -h", MergedOutput(flag)) + cmd := &bytes.Buffer{} + c.Dispatch("help "+name, MergedOutput(cmd)) + assert.Equal(t, cmd.String(), flag.String(), name) + assert.Contains(t, flag.String(), name+" - ", name) + } } -// TestDispatchReturnsTheCommandsError: what the callback returns reaches the caller, so -// a surface can log it. -func TestDispatchReturnsTheCommandsError(t *testing.T) { +// TestThenRunsAfterTheReply pins the order Then exists for: Run's line is on the writer +// before the action begins, Then's Result follows it, and Then's outcome is the +// command's. +func TestThenRunsAfterTheReply(t *testing.T) { + out := &bytes.Buffer{} + seen := "never ran" + outcome := Did("acted") + r := New() r.Register(&Command{ - Name: "fails", - Callback: func(fs any, a []string, w StringWriter) error { - return errors.New("boom") + Name: "act", + ShortDescription: "replies, then acts", + Run: func(fs any, a []string) (Result, error) { + return Did("about to act"), nil + }, + Then: func() Result { + seen = out.String() + return outcome }, }) - 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") + require.NoError(t, r.DispatchArgs([]string{"act"}, MergedOutput(out))) + assert.Equal(t, "about to act\n", seen, "the line was written before the action") + assert.Equal(t, "about to act\nacted\n", out.String()) + + out.Reset() + outcome = Failed("could not act") + var f *Failure + require.ErrorAs(t, r.DispatchArgs([]string{"act"}, MergedOutput(out)), &f) + assert.Equal(t, "about to act\ncould not act\n", out.String()) + + // In JSON only the outcome travels: one document for the program that asked. + out.Reset() + require.ErrorAs(t, r.DispatchArgs([]string{"act", "-json"}, MergedOutput(out)), &f) + assert.JSONEq(t, `{"ok":false,"reason":"could not act"}`, out.String()) + + // A Run that refused is the outcome; the action does not happen. + ran := false + r.Register(&Command{ + Name: "refuse", + ShortDescription: "refuses, so never acts", + Run: func(fs any, a []string) (Result, error) { + return Failed("not like that"), nil + }, + Then: func() Result { + ran = true + return Did("acted anyway") + }, + }) + out.Reset() + require.ErrorAs(t, r.DispatchArgs([]string{"refuse"}, MergedOutput(out)), &f) + assert.Equal(t, "not like that\n", out.String()) + assert.False(t, ran) + out.Reset() + require.ErrorAs(t, r.DispatchArgs([]string{"refuse", "-json"}, MergedOutput(out)), &f) + assert.JSONEq(t, `{"ok":false,"reason":"not like that"}`, out.String()) + assert.False(t, ran) } -// 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) { +// TestWriteCommandHelpIsTheOneRendering pins the layout both `help ` on the +// daemon and `-h` in the client print, so the two cannot drift apart again. +func TestWriteCommandHelpIsTheOneRendering(t *testing.T) { + cs := CommandSchema{ + Name: "change-remote", + Short: "changes a remote", + Help: "Give it an address.", + Flags: []FlagSchema{ + {Name: "address", Usage: "the new remote", Default: "1.2.3.4:4242"}, + {Name: "by-index", Bool: true, Usage: "match by index"}, + {Name: "json", Bool: true, Usage: "outputs as json"}, + }, + } + 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()) + require.NoError(t, WriteCommandHelp(cs, out)) + + assert.Equal(t, "change-remote - changes a remote\n"+ + " Give it an address.\n"+ + " -address string\n \tthe new remote (default 1.2.3.4:4242)\n"+ + " -by-index\n \tmatch by index\n"+ + " -json\n \toutputs as json\n", out.String()) +} + +// addrFlags is the flag struct of a test command that takes -address. +type addrFlags struct{ Address string } + +// TestHelpFlagIsReadWhereParseReadsIt: -h asks for help only where the line's parser +// reads it as a flag. As a flag's value or after -- it reaches the command as the +// operand it is, and help is rendered by the same function `help ` uses. +func TestHelpFlagIsReadWhereParseReadsIt(t *testing.T) { + var seen []string + r := New() + r.Register(Flagged(&Command{Name: "x", ShortDescription: "takes an address"}, + func(fl *flag.FlagSet, s *addrFlags) { fl.StringVar(&s.Address, "address", "", "an address") }, + func(s *addrFlags, a []string) (Result, error) { + seen = append(seen, s.Address+"|"+strings.Join(a, ",")) + return Nothing(), nil + })) + + for _, line := range []string{"x -h", "x -help", "x --help", "x 10.0.0.1 -h", "x -address=1 -h"} { + out := &bytes.Buffer{} + r.Dispatch(line, MergedOutput(out)) + assert.Contains(t, out.String(), "x - takes an address\n", line) + assert.Contains(t, out.String(), "-address string", line) + } + assert.Empty(t, seen, "asking for help must not run the command") + + for _, line := range []string{"x -address -h", "x -- -h"} { + out := &bytes.Buffer{} + r.Dispatch(line, MergedOutput(out)) + assert.Empty(t, out.String(), line) + } + assert.Equal(t, []string{"-h|", "|-h"}, seen) +} + +// TestBadFlagIsAnsweredWithTheCommandsHelp: a flag the command does not have is reported +// on the error stream followed by the command's usage, in the rendering help uses. +func TestBadFlagIsAnsweredWithTheCommandsHelp(t *testing.T) { + out, errOut := &bytes.Buffer{}, &bytes.Buffer{} + r := New() + r.Register(&Command{Name: "x", ShortDescription: "has no flags", Run: func(fs any, a []string) (Result, error) { return Nothing(), nil }}) + + require.Error(t, r.DispatchArgs([]string{"x", "-nope"}, Output{Out: out, Err: errOut})) + assert.Empty(t, out.String()) + assert.Equal(t, "flag provided but not defined: -nope\nx - has no flags\n -json\n \toutputs as json\n -pretty\n \tpretty prints json, assumes -json\n", errOut.String()) +} + +// TestRunErrorIsShown: an error Run returns reaches the person at the prompt on the +// error stream, as well as the surface that logs it. +func TestRunErrorIsShown(t *testing.T) { + r := New() + r.Register(&Command{ + Name: "broken", + ShortDescription: "cannot do what it is asked", + Run: func(fs any, a []string) (Result, error) { + return Result{}, errors.New("no certificate is loaded") + }, + }) + + out, errOut := &bytes.Buffer{}, &bytes.Buffer{} + require.EqualError(t, r.DispatchArgs([]string{"broken"}, Output{Out: out, Err: errOut}), "no certificate is loaded") + assert.Empty(t, out.String()) + assert.Equal(t, "no certificate is loaded\n", errOut.String()) } diff --git a/commands/render.go b/commands/render.go new file mode 100644 index 00000000..bd068bf4 --- /dev/null +++ b/commands/render.go @@ -0,0 +1,179 @@ +package commands + +import ( + "encoding/json" + "errors" + "fmt" + "io" +) + +// ErrNoTextRendering is returned by Render when a command's Value has no human rendering +// and text was asked for anyway. A command declares this by leaving Text nil and saying +// so with JSONOnly; it is not a fallback situation but a statement that no such form +// exists yet, so nobody builds a script around an interim one. Each surface's default +// policy avoids ever asking. +var ErrNoTextRendering = errors.New("no human readable output yet; pass -json or -pretty") + +// Format is how a result should be presented. +type Format int + +const ( + // FormatUnset is a request that expressed no preference. A surface resolves it with + // its own default before rendering; Render itself treats it as FormatText. + FormatUnset Format = iota + + // FormatText renders for a person. It is what an operator gets by default on every + // surface with a human at the other end. + FormatText + + // FormatJSON renders the result as compact JSON. + FormatJSON + + // FormatPrettyJSON renders the result as indented JSON. + FormatPrettyJSON +) + +// Flag names for the two presentation flags. They are offered on every command rather +// than declared by each one, because how a result is displayed is not a property of the +// command that produced it. +const ( + FlagJSON = "json" + FlagPretty = "pretty" +) + +// Flag names nebula ctl takes for itself, anywhere on its line, to say which daemon to +// talk to; see reservedFlags. +const ( + FlagConfig = "config" + FlagSocket = "socket" +) + +// reservedFlags are handled before a command's Run is called, so a command may not +// declare them itself. -h and -help are intercepted as a request for usage; -json and +// -pretty are added to every command's FlagSet by commandFlags, on every surface, so a +// command that defined either would make that definition panic with "flag redefined" +// on whichever goroutine ran the command; and -config and -socket are nebula ctl's own, +// taken off the line before the client knows which command it names, so a command +// that declared either would never receive it there. +var reservedFlags = []string{"h", "help", FlagJSON, FlagPretty, FlagConfig, FlagSocket} + +// Render writes r in the requested format. Every surface calls it, so a hostmap looks +// the same over ssh as it does through the control socket, and a change to either +// rendering happens once. +func Render(c *Command, r Result, f Format, w io.Writer) error { + switch f { + case FormatJSON: + return renderJSON(r.Payload(), w, false) + + case FormatPrettyJSON: + return renderJSON(r.Payload(), w, true) + + default: + // An outcome reads the same for every command: its reason, as a sentence. Only a + // Value needs the command's own rendering, so Text never sees anything else. + if r.Value == nil { + _, err := io.WriteString(w, r.Reason+"\n") + return err + } + + if c != nil && c.Text != nil { + return c.Text(r.Value, w) + } + + name := "this command" + if c != nil { + name = string(c.Name) + } + return fmt.Errorf("%s has %w", name, ErrNoTextRendering) + } +} + +func renderJSON(v any, w io.Writer, indent bool) error { + enc := json.NewEncoder(w) + if indent { + enc.SetIndent("", " ") + } + + return enc.Encode(v) +} + +// Result is what every Run returns: whether the command worked, a sentence saying so, +// and, for a command that reports rather than acts, the value it produced. +// +// Value and Reason are alternatives. A command that reports returns Value(v), and the +// surfaces render v, as JSON or through the command's Text. A command that acts, or any +// command refusing its arguments, returns Did or Failed, and the surfaces render the +// Outcome: its Reason is the sentence these commands have always printed. Nothing is for +// a command with nothing to say. +// +// The envelope itself never reaches a caller. Payload is what JSON carries, and Render +// hands Text only the Value, so a command's renderer sees the one type it returns on +// success and is never asked to recognise an Outcome. +type Result struct { + Outcome + Value any +} + +// Outcome is the part of a Result that says whether the command worked and, in words a +// caller can show someone, why. It is what a program receives from a command that acts +// rather than reports: closing a tunnel, changing a remote, reloading config. +// +// One shape serves all of them. Designing a payload per outcome would be work without a +// reader. Reason is set either way, so a success says what it did rather than leaving +// the caller to infer it from a bare true. +type Outcome struct { + OK bool `json:"ok"` + Reason string `json:"reason"` +} + +// Value is the Result of a command that reports: v is what it has to show, and it must +// marshal to JSON. +func Value(v any) Result { + return Result{Outcome: Outcome{OK: true}, Value: v} +} + +// Failed is a convenience for the common "it did not work, here is why" case. +func Failed(format string, args ...any) Result { + return Result{Outcome: Outcome{Reason: fmt.Sprintf(format, args...)}} +} + +// Did is a convenience for the common "it worked, here is what happened" case. +func Did(format string, args ...any) Result { + return Result{Outcome: Outcome{OK: true, Reason: fmt.Sprintf(format, args...)}} +} + +// Nothing is the Result of a command with nothing to say: it did its work, and the +// surface sends an empty, successful reply without rendering anything. +func Nothing() Result { + return Result{Outcome: Outcome{OK: true}} +} + +// Empty reports a Result with nothing to render: no Value and no Reason. The zero Result +// counts, so a Run that returns Result{} says nothing rather than failing silently. +func (r Result) Empty() bool { + return r.Value == nil && r.Reason == "" +} + +// Payload is what a program receives: the Value when there is one, otherwise the +// Outcome. No surface sends the envelope itself, so print-tunnel -json emits the tunnel +// and close-tunnel -json emits {ok, reason}. +func (r Result) Payload() any { + if r.Value != nil { + return r.Value + } + return r.Outcome +} + +// Typed adapts a renderer for one result type to the TextFunc a Command declares. The +// value Render passes is the command's own, see Result, so all that is left is to name +// its type once here instead of asserting it at the top of every renderer. A renderer +// paired with a command that returns something else fails here, saying what arrived. +func Typed[T any](render func(T, io.Writer) error) TextFunc { + return func(v any, w io.Writer) error { + t, ok := v.(T) + if !ok { + return fmt.Errorf("expected %T, got %T", t, v) + } + return render(t, w) + } +} diff --git a/commands/render_test.go b/commands/render_test.go new file mode 100644 index 00000000..9eb3bf36 --- /dev/null +++ b/commands/render_test.go @@ -0,0 +1,105 @@ +package commands + +import ( + "bytes" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRenderPrintsAnOutcomeWithoutText: a command that acts declares no Text. Its +// outcome renders as the reason, the same for every command, and as {ok, reason} in +// JSON, which is the shape these commands have always emitted. +func TestRenderPrintsAnOutcomeWithoutText(t *testing.T) { + c := &Command{Name: "act"} + + out := &bytes.Buffer{} + require.NoError(t, Render(c, Did("Closed"), FormatText, out)) + assert.Equal(t, "Closed\n", out.String()) + + out.Reset() + require.NoError(t, Render(c, Failed("No vpn address was provided"), FormatText, out)) + assert.Equal(t, "No vpn address was provided\n", out.String()) + + out.Reset() + require.NoError(t, Render(c, Failed("nope"), FormatJSON, out)) + assert.JSONEq(t, `{"ok":false,"reason":"nope"}`, out.String()) +} + +// TestRenderHandsTextOnlyTheValue pins the guarantee Result exists for: a command's +// renderer sees the type it returns on success and never the envelope or an outcome, so +// a report command that refuses its arguments needs nothing in its Text to cope. +func TestRenderHandsTextOnlyTheValue(t *testing.T) { + var got any + c := &Command{ + Name: "report", + Text: func(v any, w io.Writer) error { + got = v + _, err := io.WriteString(w, "rendered\n") + return err + }, + } + + out := &bytes.Buffer{} + require.NoError(t, Render(c, Value("data"), FormatText, out)) + assert.Equal(t, "data", got) + assert.Equal(t, "rendered\n", out.String()) + + got, out = nil, &bytes.Buffer{} + require.NoError(t, Render(c, Failed("refused"), FormatText, out)) + assert.Nil(t, got, "Text must not be called for an outcome") + assert.Equal(t, "refused\n", out.String()) + + out.Reset() + require.NoError(t, Render(c, Value("data"), FormatJSON, out)) + assert.JSONEq(t, `"data"`, out.String(), "JSON carries the value, never the envelope") +} + +// TestRenderRefusesTextForAValueWithoutText: leaving Text nil is a statement that no +// human form of the value exists yet, and asking for one is an error that says so. +func TestRenderRefusesTextForAValueWithoutText(t *testing.T) { + out := &bytes.Buffer{} + err := Render(&Command{Name: "print-tunnel"}, Value(map[string]int{"index": 7}), FormatText, out) + require.ErrorIs(t, err, ErrNoTextRendering) + require.ErrorContains(t, err, "print-tunnel has no human readable output yet") + assert.Empty(t, out.String()) +} + +// TestTypedNamesTheTypeOnce: Typed replaces the assertion at the top of every renderer. +// The right type reaches the renderer; the wrong one is reported, naming both. +func TestTypedNamesTheTypeOnce(t *testing.T) { + type info struct{ Name string } + render := Typed(func(i info, w io.Writer) error { + _, err := io.WriteString(w, i.Name+"\n") + return err + }) + + out := &bytes.Buffer{} + require.NoError(t, render(info{Name: "tun0"}, out)) + assert.Equal(t, "tun0\n", out.String()) + + err := render("not an info", out) + require.Error(t, err) + assert.ErrorContains(t, err, "expected commands.info, got string") +} + +// TestResultShapes pins what each constructor means to a surface: Empty is the one that +// renders nothing, Payload is what a program receives, and OK is set wherever a reason +// or value says the command worked. +func TestResultShapes(t *testing.T) { + assert.True(t, Nothing().Empty()) + assert.True(t, Result{}.Empty(), "the zero Result says nothing rather than failing silently") + assert.False(t, Did("done").Empty()) + assert.False(t, Failed("no").Empty()) + assert.False(t, Value("x").Empty()) + + assert.True(t, Value("x").OK) + assert.True(t, Did("done").OK) + assert.False(t, Failed("no").OK) + + assert.Equal(t, "x", Value("x").Payload()) + assert.Equal(t, Outcome{OK: true, Reason: "done"}, Did("done").Payload()) + assert.Equal(t, Outcome{Reason: "no"}, Failed("no").Payload()) +} diff --git a/commands/request.go b/commands/request.go new file mode 100644 index 00000000..1e9e822f --- /dev/null +++ b/commands/request.go @@ -0,0 +1,119 @@ +package commands + +import ( + "flag" + "maps" + "slices" +) + +// Request is one command to run, as every surface hands it to Invoke: the command, its +// options by name in the form their flags parse, its operands, and the rendering the +// line asked for, if any. +// +// nebula ctl builds one with ParseLine and it travels as a JSON object to the control +// socket. An ssh line never becomes one: it is parsed straight into the command's own +// FlagSet; see execLine. +type Request struct { + // Command is the command to run, resolved by the surface at its edge. See + // Registry.Resolve for the one that tells an unknown name from a withheld one. + Command *Command + + // Params carries the options the line set, by flag name, each in the form its flag + // parses from. An option left alone is absent, so the command's default applies. + Params map[string]string + + // Args carries the operands: whatever the line did not consume as a flag. + Args []string + + // Format is the rendering the line asked for with -json or -pretty, or FormatUnset + // when it expressed no preference. It is ParseLine's reading of the line for the + // client that sends the Request; Invoke does not read it, since the surface that + // runs a Request renders with the format it agreed with its caller. + Format Format +} + +// ParseLine reads a FlagSet that ParseArgs has filled, and the operands it returned, +// into a Request to send over the wire. Command is left for the caller to set. +// +// Every flag the line set other than the presentation flags travels by name, as its +// flag's String, which is what the daemon's Set parses. That round trip is exact for +// the bool and string flags a client builds from the schema; it is the client's to make, +// and the ssh path does not make it, since it runs the FlagSet it parsed. +func ParseLine(fl *flag.FlagSet, operands []string) Request { + req := Request{Args: operands, Format: presentationFormat(fl)} + + fl.Visit(func(f *flag.Flag) { + if isPresentationFlag(f.Name) { + return + } + if req.Params == nil { + req.Params = map[string]string{} + } + req.Params[f.Name] = f.Value.String() + }) + + return req +} + +// presentationFormat is the one reading of -json and -pretty from a parsed line, so a +// line means the same on every surface: only a flag set to true asks for JSON, since one +// set to false expresses no preference, and -pretty implies -json. FormatUnset means the +// line expressed no preference and the surface's own default applies. +func presentationFormat(fl *flag.FlagSet) Format { + var asJSON, pretty bool + fl.Visit(func(f *flag.Flag) { + switch f.Name { + case FlagJSON: + asJSON = flagIsTrue(f) + case FlagPretty: + pretty = flagIsTrue(f) + } + }) + + switch { + case pretty: + return FormatPrettyJSON + case asJSON: + return FormatJSON + default: + return FormatUnset + } +} + +// flagIsTrue reports whether a bool flag was set to true. The flag package's bool value +// is a flag.Getter whose Get returns the bool, so this never falls back to parsing text. +func flagIsTrue(f *flag.Flag) bool { + g, ok := f.Value.(flag.Getter) + if !ok { + return false + } + b, _ := g.Get().(bool) + return b +} + +// Invoke runs one Request and returns the command's Result, unrendered. +// +// The parameters are applied by flag name, in name order: a request from the wire is a +// JSON object with no order of its own, and which of two bad parameters is reported must +// not change from one request to the next. An unknown parameter is an error rather than +// something quietly ignored, because a caller that misspells one deserves to hear about +// it instead of getting a default. A bad parameter means Run is never called, and the +// FlagSet is fresh for this call, so nothing set before it is seen by anyone. +// +// Nothing here writes anywhere and nothing here renders. What a surface does with the +// Result, marshal its Payload or run the command's own text rendering through Render, +// is the surface's decision, and keeping that out of here is what lets a caller tell a +// refusal from a success after the text has been produced. The presentation flags are +// not parameters: a Request says how it wants rendering in Format, and sending them as +// parameters is refused rather than read as a preference. +func Invoke(req Request) (Result, error) { + c := req.Command + names := slices.Sorted(maps.Keys(req.Params)) + + fl, fs, own := commandFlags(c) + if err := setParams(fl, string(c.Name), names, req.Params, own); err != nil { + return Result{}, err + } + + return c.Run(fs, req.Args) +} diff --git a/commands/request_test.go b/commands/request_test.go new file mode 100644 index 00000000..fd5becc6 --- /dev/null +++ b/commands/request_test.go @@ -0,0 +1,115 @@ +package commands + +import ( + "flag" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func lineFlags() *flag.FlagSet { + fl := flag.NewFlagSet("set", flag.ContinueOnError) + fl.Bool("verbose", false, "") + fl.Duration("wait", 0, "") + fl.String("label", "none", "") + fl.Bool(FlagJSON, false, "") + fl.Bool(FlagPretty, false, "") + return fl +} + +// TestParseLineReadsThePresentationFlagsOnce: presentationFormat is the one reading of +// -json and -pretty, for every surface, and ParseLine carries its answer. A flag set to +// false expresses no preference, and neither flag ever travels as a parameter. +func TestParseLineReadsThePresentationFlagsOnce(t *testing.T) { + for _, tc := range []struct { + args []string + want Format + }{ + {nil, FormatUnset}, + {[]string{"-json=false"}, FormatUnset}, + {[]string{"-pretty=false"}, FormatUnset}, + {[]string{"-json"}, FormatJSON}, + {[]string{"-pretty"}, FormatPrettyJSON}, + {[]string{"-pretty", "-json=false"}, FormatPrettyJSON}, + } { + fl := lineFlags() + operands, err := ParseArgs(fl, tc.args) + require.NoError(t, err) + + req := ParseLine(fl, operands) + assert.Equal(t, tc.want, req.Format, "%v", tc.args) + assert.Nil(t, req.Params, "%v: presentation flags are not parameters", tc.args) + } +} + +// TestParseLineCarriesWhatWasSetInParseableForm: only the flags the line set travel, +// each as the string its flag parses from, so the daemon's Invoke can apply them to a +// fresh FlagSet and get the same values back. Operands come through untouched, those +// after "--" included. +func TestParseLineCarriesWhatWasSetInParseableForm(t *testing.T) { + fl := lineFlags() + operands, err := ParseArgs(fl, []string{"-verbose", "-wait", "2s", "x", "--", "-1"}) + require.NoError(t, err) + + req := ParseLine(fl, operands) + assert.Equal(t, map[string]string{"verbose": "true", "wait": "2s"}, req.Params, "label was left alone and must be absent") + assert.Equal(t, []string{"x", "-1"}, req.Args) + assert.Equal(t, FormatUnset, req.Format) + + // The round trip a client makes: what ParseLine produced is what Invoke applies. + r, seen := paramRegistry(t) + req.Command = r.Get("set") + _, err = Invoke(req) + require.NoError(t, err) + require.Len(t, *seen, 1) + assert.Equal(t, paramsSeen{Verbose: true, Wait: 2 * time.Second}, (*seen)[0]) +} + +// TestResolveTellsUnknownFromWithheld: the two cases read differently to an operator, +// and Resolve is the edge that decides which one a name from the wire is. +func TestResolveTellsUnknownFromWithheld(t *testing.T) { + r := testRegistry().Filter(notWithheld, testReason) + + c, err := r.Resolve("served") + require.NoError(t, err) + assert.Equal(t, Name("served"), c.Name) + + _, err = r.Resolve("withheld") + require.ErrorContains(t, err, "withheld is not available on this surface") + + _, err = r.Resolve("no-such-command") + require.ErrorContains(t, err, `unknown command "no-such-command"`) +} + +// TestLineRunsTheFlagSetItParsed: the ssh path hands Run the FlagSet the line was parsed +// into, with no trip through a Request. A flag.Func has no String worth re-parsing, so a +// round trip would hand Run an empty value and no error; the line's value must arrive. +func TestLineRunsTheFlagSetItParsed(t *testing.T) { + var seen []string + r := New() + r.Register(&Command{ + Name: "mode", + Flags: func() (*flag.FlagSet, any) { + fl := flag.NewFlagSet("", flag.ContinueOnError) + s := new(string) + fl.Func("mode", "the mode", func(v string) error { + *s = "set:" + v + return nil + }) + return fl, s + }, + Run: func(fs any, a []string) (Result, error) { + seen = append(seen, *fs.(*string)) + return Value("ok"), nil + }, + }) + + var out, errw strings.Builder + r.Dispatch("mode -json -mode fast", Output{Out: &out, Err: &errw}) + require.Equal(t, []string{"set:fast"}, seen) + assert.Empty(t, errw.String()) + assert.Equal(t, "\"ok\"\n", out.String(), "-json was read from the same FlagSet") +} diff --git a/commands/writer.go b/commands/writer.go index 5ea2d804..52c423b7 100644 --- a/commands/writer.go +++ b/commands/writer.go @@ -2,38 +2,22 @@ package commands import "io" -// StringWriter is how a command talks back to whoever invoked it: one stream, which the -// ssh surface points at its channel. -type StringWriter interface { - WriteLine(string) error - Write(string) error - WriteBytes([]byte) error - GetWriter() io.Writer +// Output is where a command line surface's text goes. +// +// Out and Err are distinct so a command's primary output can stay machine readable on +// a surface that separates the two. The ssh surface points both at the same channel and +// sees a single merged stream. The local +// control socket does not use one at all: it invokes by named parameter and carries the +// returned value, so there is no stream for a command to write to. +type Output struct { + // Out receives what a command produces. + Out io.Writer + + // Err receives usage text, refusals and errors. + Err io.Writer } -type stringWriter struct { - w io.Writer -} - -// NewStringWriter returns a StringWriter that writes to w. -func NewStringWriter(w io.Writer) StringWriter { - return &stringWriter{w: w} -} - -func (w *stringWriter) WriteLine(s string) error { - return w.Write(s + "\n") -} - -func (w *stringWriter) Write(s string) error { - _, err := w.w.Write([]byte(s)) - return err -} - -func (w *stringWriter) WriteBytes(b []byte) error { - _, err := w.w.Write(b) - return err -} - -func (w *stringWriter) GetWriter() io.Writer { - return w.w +// MergedOutput sends output and errors to the same writer. +func MergedOutput(w io.Writer) Output { + return Output{Out: w, Err: w} } diff --git a/commands_test.go b/commands_test.go index 068a5080..caf91682 100644 --- a/commands_test.go +++ b/commands_test.go @@ -2,10 +2,14 @@ package nebula import ( "bytes" + "encoding/json" "io" "net/netip" "testing" + "time" + "github.com/slackhq/nebula/cert" + "github.com/slackhq/nebula/cert_test" "github.com/slackhq/nebula/commands" "github.com/slackhq/nebula/logging" "github.com/slackhq/nebula/test" @@ -13,19 +17,20 @@ import ( "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. +// TestLogFormatRefusalIsShown: a format the logger does not know is a refusal the user +// sees, rather than a bare error, which over ssh was silence. func TestLogFormatRefusalIsShown(t *testing.T) { l := logging.NewLogger(io.Discard) + + res, err := runLogFormat(l, []string{"bogus"}) + require.NoError(t, err) + assert.False(t, res.OK) + assert.Equal(t, "Unable to set log format: unknown log format `bogus`. possible formats: [text json]", res.Reason) + + res, err = runLogFormat(l, []string{"JSON"}) + require.NoError(t, err) 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)) + require.NoError(t, commands.Typed(textLogFormat)(res.Value, out)) assert.Equal(t, "Log format is: json\n", out.String()) } @@ -41,9 +46,12 @@ func TestPrintRelaysReportsErrorsAsText(t *testing.T) { 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()) + marshal := func() string { + b, err := json.Marshal(runPrintRelays(ifce)) + require.NoError(t, err) + return string(b) + } + assert.JSONEq(t, `{"Relays":[{"NebulaAddr":"10.0.0.1","RelayForAddrs":[{"Error":"could not find hostinfo","Type":"","State":"","PeerAddr":"","LocalIndex":0,"RemoteIndex":0,"RelayedThrough":null}]}]}`, marshal()) // The same relay host, found, relaying for a peer whose index matches: no Error key. hi := hm.Relays[1] @@ -53,15 +61,51 @@ func TestPrintRelaysReportsErrorsAsText(t *testing.T) { 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()) + 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}]}]}`, marshal()) // 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"`) + assert.Contains(t, marshal(), `"Error":"hostmap LocalIndex '7' does not match RelayState LocalIndex"`) +} + +// tunnelFixture is an Interface with our own certificate loaded, one peer behind an +// established tunnel and one still handshaking, which has no certificate yet. +func tunnelFixture(t *testing.T) *Interface { + t.Helper() + + l := test.NewLogger() + me := netip.MustParseAddr("10.0.0.1") + peer := netip.MustParsePrefix("10.0.0.2/24") + + ca, _, caKey, _ := cert_test.NewTestCaCert(cert.Version2, cert.Curve_CURVE25519, time.Time{}, time.Time{}, nil, nil, nil) + peerCert, _, _, _ := cert_test.NewTestCert(cert.Version2, cert.Curve_CURVE25519, ca, caKey, "peer", time.Time{}, time.Time{}, + []netip.Prefix{peer}, nil, []string{"web", "db"}) + + hm := newHostMap(l) + hm.Hosts[peer.Addr()] = &HostInfo{ + vpnAddrs: []netip.Addr{peer.Addr()}, + ConnectionState: &ConnectionState{peerCert: &cert.CachedCertificate{Certificate: peerCert}}, + } + hm.Hosts[netip.MustParseAddr("10.0.0.3")] = &HostInfo{ + vpnAddrs: []netip.Addr{netip.MustParseAddr("10.0.0.3")}, + ConnectionState: &ConnectionState{}, + } + + return &Interface{pki: newTestPKI(t, "me", []netip.Addr{me}), hostMap: hm} +} + +// TestPrintCertOfATunnelWithoutACertificate: a tunnel still handshaking, or one being +// torn down, has no certificate, and print-cert refuses rather than panicking. +func TestPrintCertOfATunnelWithoutACertificate(t *testing.T) { + f := tunnelFixture(t) + + r, err := runPrintCert(f, &printCertFlags{}, []string{"10.0.0.3"}) + require.NoError(t, err) + assert.False(t, r.OK) + assert.Equal(t, "Could not find tunnel for vpn addr: 10.0.0.3", r.Reason) + + r, err = runPrintCert(f, &printCertFlags{}, []string{"10.0.0.2"}) + require.NoError(t, err) + assert.True(t, r.OK, "a tunnel with a certificate is printed") } diff --git a/config/config.go b/config/config.go index 19095156..747e4772 100644 --- a/config/config.go +++ b/config/config.go @@ -32,6 +32,11 @@ type C struct { // Set by the first reload that needs a restart and never cleared. Guarded by reloadLock needsRestart bool + + // callbackLock guards callbacks. Registration is not confined to startup: CatchHUP is + // running from inside Main, and anything wired up after Main returns registers while + // a HUP could already be reloading, so append and iteration have to be serialized. + callbackLock sync.Mutex } func NewC(l *slog.Logger) *C { @@ -78,9 +83,22 @@ func (c *C) LoadString(raw string) error { // These functions should return quickly or spawn their own go routine if they will take a while. // Use r to report anything that failed or needs a restart to apply. func (c *C) RegisterReloadCallback(f func(c *C, r *Reload)) { + c.callbackLock.Lock() + defer c.callbackLock.Unlock() c.callbacks = append(c.callbacks, f) } +// reloadCallbacks returns a snapshot of the registered callbacks. Reloads iterate the +// snapshot rather than the live slice, so a registration from another goroutine cannot +// race the loop; a callback registered while a reload is in progress runs from the next +// reload on. callbackLock is not held during the calls themselves, so a callback that +// registers another callback does not deadlock. +func (c *C) reloadCallbacks() []func(*C, *Reload) { + c.callbackLock.Lock() + defer c.callbackLock.Unlock() + return append(make([]func(*C, *Reload), 0, len(c.callbacks)), c.callbacks...) +} + // InitialLoad returns true if this is the first load of the config, and ReloadConfig has not been called yet. func (c *C) InitialLoad() bool { return c.oldSettings == nil @@ -204,7 +222,7 @@ func (c *C) ReloadConfigString(raw string) ReloadResult { func (c *C) runCallbacks() ReloadResult { r := &Reload{c: c} - for _, v := range c.callbacks { + for _, v := range c.reloadCallbacks() { v(c, r) } diff --git a/config/config_test.go b/config/config_test.go index e5d21e5b..b3262087 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "sync" "testing" "time" @@ -220,3 +221,41 @@ firewall: } assert.Equal(t, expected, m) } + +// TestRegisterReloadCallbackDuringReload: registration is not confined to startup. The +// HUP handler runs from inside nebula.Main, and anything wired after Main returns +// registers while a reload may already be iterating the callbacks. Under -race this is +// the data race on the slice; without it, it checks a callback registered mid-reload is +// picked up by the next one. +func TestRegisterReloadCallbackDuringReload(t *testing.T) { + l := test.NewLogger() + c := NewC(l) + require.NoError(t, c.LoadString("a: 1")) + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + c.RegisterReloadCallback(func(*C, *Reload) {}) + } + }() + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + assert.NoError(t, c.ReloadConfigString("a: 1").Err) + } + }() + wg.Wait() + + // A callback that registers another callback must not deadlock, and the new one + // runs from the following reload on. + var ran int + c.RegisterReloadCallback(func(c *C, _ *Reload) { + c.RegisterReloadCallback(func(*C, *Reload) { ran++ }) + }) + require.NoError(t, c.ReloadConfigString("a: 1").Err) + assert.Equal(t, 0, ran, "registered during this reload, so not run by it") + require.NoError(t, c.ReloadConfigString("a: 1").Err) + assert.Equal(t, 1, ran, "and run by the next") +} diff --git a/e2e/commands_output_test.go b/e2e/commands_output_test.go index 7f2ae288..8db79a0b 100644 --- a/e2e/commands_output_test.go +++ b/e2e/commands_output_test.go @@ -69,12 +69,15 @@ func TestCommandOutput(t *testing.T) { // help on the clone lists and describes. There is no session to end here. reg := control.Commands().Clone() reg.Register(sshd.LogoutCommand(func() {})) + // Dispatch with ssh's default-format policy, so the no-flag column records what an + // operator at an ssh prompt sees. + reg.DefaultFormat = sshd.DefaultFormat // 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{} - err := reg.DispatchArgs(args, commands.NewStringWriter(buf)) + err := reg.DispatchArgs(args, commands.MergedOutput(buf)) return buf.String(), err } @@ -94,6 +97,8 @@ func TestCommandOutput(t *testing.T) { {"print-cert", nil}, {"print-cert", []string{"-raw"}}, {"print-cert", []string{"10.222.0.9"}}, + // A flag after the operand is still a flag, on both surfaces. + {"print-cert", []string{"10.222.0.9", "-json"}}, {"print-cert", []string{"10.222.0.250"}}, {"print-cert", []string{"not-an-address"}}, {"print-tunnel", nil}, diff --git a/e2e/testdata/commands_output.golden b/e2e/testdata/commands_output.golden index aa23b482..01f6e11a 100644 --- a/e2e/testdata/commands_output.golden +++ b/e2e/testdata/commands_output.golden @@ -2,10 +2,12 @@ $ version e2e-test $ version -json -e2e-test +{"version":"e2e-test"} $ version -pretty -e2e-test +{ + "version": "e2e-test" +} $ device-info name= cidr=[10.222.0.1/24] @@ -104,7 +106,7 @@ $ print-cert } $ print-cert -json -{"details":{"curve":"CURVE25519","groups":[],"isCa":false,"issuer":"","name":"output-golden","networks":["10.222.0.1/24"],"notAfter":"