From 4ff55f052acebb4231463d379e95b5f43ea687e1 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Tue, 22 Sep 2026 22:50:05 +1000 Subject: [PATCH] Fail a bare invocation when stdout is not a terminal Puppet's generate() reads stdout through a pipe with combine: true, so help text on a zero exit lands in the catalog. Help now requires an interactive stdout; a piped one exits non-zero with nothing on stdout. - gate the no-args help path on an isTerminal(stdout) check - reject an explicitly empty --config instead of resolving it - ignore a relative XDG_CONFIG_HOME, per the XDG spec - give the integration subprocess a hermetic HOME/XDG_CONFIG_HOME - pass []string{} to run(), so cobra cannot fall back to os.Args --- AGENTS.md | 7 +- README.md | 13 +++- cmd/certmanager/main.go | 46 +++++++++++-- cmd/certmanager/main_test.go | 99 ++++++++++++++++++++++++++-- internal/config/config.go | 6 +- internal/config/config_test.go | 20 +++++- test/integration/certmanager_test.go | 50 ++++++++++++++ 7 files changed, 219 insertions(+), 22 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 079f3a8..b6fbe32 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,9 +46,10 @@ dist/ # build output: binaries, RPM, checksums (not comm wires it to the real streams and `os.Exit`. Cobra runs with `SilenceUsage` and `SilenceErrors` so nothing reaches stderr unless the run failed. - **Flat CLI**: the root command takes `common_name` positionally - (`cobra.MaximumNArgs(1)`); no args prints help, args-less flags fail. `ca`, - `completion` and `help` are the only subcommands; `--config` is persistent so - `ca` shares it. + (`cobra.MaximumNArgs(1)`); no args prints help only when stdout is a terminal + and otherwise fails, args-less flags fail. `ca`, `completion` and `help` are + the only subcommands; `--config` is persistent so `ca` shares it, and an + explicitly empty `--config` is rejected. - **`ca` is unauthenticated**: `GET {addr}/v1/{mount}/ca_chain` sends no `X-Vault-Token` and loads the config through `config.LoadCA`, which requires only `vault.addr`. The body is parsed as PEM, never sniffed — Vault labels it diff --git a/README.md b/README.md index b9118fa..1e85850 100644 --- a/README.md +++ b/README.md @@ -21,13 +21,18 @@ failure path exits non-zero. ``` certmanager [flags] # issue a certificate certmanager ca [flags] # print the CA chain -certmanager # print help +certmanager # print help (interactive terminals only) ``` The common name is positional, with no `pki` subcommand in front of it — that is the form Puppet calls, and it is the only issuing form. `ca`, `completion` and `help` are the subcommands; every other first argument is a common name. +A bare invocation prints help and exits zero only when stdout is a terminal. +Puppet's `generate()` reads stdout through a pipe, so there a missing common +name exits non-zero with nothing on stdout rather than feeding help text into +the catalog. + | Flag | Short | Type | Default | Description | |---|---|---|---|---| | `--alt-names` | `-a` | comma list | `""` | DNS subject alternative names | @@ -110,8 +115,10 @@ YAML, everything nested under `vault:`. ### Config resolution order -1. `--config `, which always wins; -2. `$XDG_CONFIG_HOME/certmanager/config.yaml`, if the file exists; +1. `--config `, which always wins; an empty `--config ''` is an error, + not a request to resolve; +2. `$XDG_CONFIG_HOME/certmanager/config.yaml`, if `$XDG_CONFIG_HOME` is + absolute and the file exists; 3. `$HOME/.config/certmanager/config.yaml`, if the file exists; 4. `/opt/certmanager/config.yaml`, the path baked in at build time (`make build CONFIG_PATH=...`). diff --git a/cmd/certmanager/main.go b/cmd/certmanager/main.go index ad0a241..c3fb629 100644 --- a/cmd/certmanager/main.go +++ b/cmd/certmanager/main.go @@ -26,7 +26,11 @@ func main() { // it writes nothing to stderr: Puppet's generate() runs this binary with // combine: true, which splices stderr into the stdout that parsejson() reads. func run(args []string, stdout, stderr io.Writer) int { - cmd := rootCmd() + return runWith(args, stdout, stderr, isTerminal(stdout)) +} + +func runWith(args []string, stdout, stderr io.Writer, interactive bool) int { + cmd := rootCmd(interactive) cmd.SetArgs(args) cmd.SetOut(stdout) cmd.SetErr(stderr) @@ -38,7 +42,27 @@ func run(args []string, stdout, stderr io.Writer) int { return 0 } -func rootCmd() *cobra.Command { +// isTerminal reports whether w is a terminal. Puppet's generate() captures +// stdout through a pipe, so anything else is a machine caller. +func isTerminal(w io.Writer) bool { + f, ok := w.(*os.File) + if !ok { + return false + } + info, err := f.Stat() + return err == nil && info.Mode()&os.ModeCharDevice != 0 +} + +// resolveConfig turns the --config value into a path. An explicitly empty +// --config is a malformed call, not a request to resolve. +func resolveConfig(cmd *cobra.Command, explicit string) (string, error) { + if explicit == "" && cmd.Flags().Changed("config") { + return "", fmt.Errorf("--config requires a path") + } + return config.Resolve(explicit), nil +} + +func rootCmd(interactive bool) *cobra.Command { var ( cfgFile string altNames string @@ -57,15 +81,19 @@ func rootCmd() *cobra.Command { SilenceErrors: true, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 0 { - // A bare invocation is someone asking what this is; flags - // without a common name is a malformed call and must fail. - if cmd.Flags().NFlag() == 0 { + // Help only for a human at a terminal: under generate() an + // argv that lost its common name must fail the compile. + if interactive && cmd.Flags().NFlag() == 0 { return cmd.Help() } return fmt.Errorf("a common name is required") } - cfg, err := config.Load(config.Resolve(cfgFile)) + path, err := resolveConfig(cmd, cfgFile) + if err != nil { + return err + } + cfg, err := config.Load(path) if err != nil { return err } @@ -116,7 +144,11 @@ func caCmd(cfgFile *string) *cobra.Command { SilenceUsage: true, SilenceErrors: true, RunE: func(cmd *cobra.Command, _ []string) error { - cfg, err := config.LoadCA(config.Resolve(*cfgFile)) + path, err := resolveConfig(cmd, *cfgFile) + if err != nil { + return err + } + cfg, err := config.LoadCA(path) if err != nil { return err } diff --git a/cmd/certmanager/main_test.go b/cmd/certmanager/main_test.go index 4df7d93..e8e8934 100644 --- a/cmd/certmanager/main_test.go +++ b/cmd/certmanager/main_test.go @@ -772,11 +772,11 @@ func TestRunCAFailures(t *testing.T) { } } -// A bare invocation is a human at a terminal, not Puppet: print help and exit -// zero rather than failing on the missing positional. -func TestRunNoArgsPrintsHelp(t *testing.T) { +// A bare invocation at a terminal is a human asking what this is: print help +// and exit zero rather than failing on the missing positional. +func TestRunNoArgsOnATerminalPrintsHelp(t *testing.T) { var stdout, stderr bytes.Buffer - if code := run(nil, &stdout, &stderr); code != 0 { + if code := runWith([]string{}, &stdout, &stderr, true); code != 0 { t.Fatalf("exit code = %d, stderr = %q", code, stderr.String()) } if !strings.Contains(stdout.String(), "Usage:") { @@ -790,6 +790,97 @@ func TestRunNoArgsPrintsHelp(t *testing.T) { } } +// generate() splices stderr into stdout and reads the result as JSON, so an +// argv that lost its common name must fail loudly with nothing on stdout. +func TestRunNoArgsWithoutATerminalFails(t *testing.T) { + var stdout, stderr bytes.Buffer + code := runWith([]string{}, &stdout, &stderr, false) + if code == 0 { + t.Fatalf("exit code = 0, want non-zero (stdout %q)", stdout.String()) + } + if stdout.Len() != 0 { + t.Errorf("stdout = %q, want empty", stdout.String()) + } + if !strings.Contains(stderr.String(), "a common name is required") { + t.Errorf("stderr = %q, want it to name the missing common name", stderr.String()) + } +} + +// run() decides interactivity from the real stdout, so a piped stdout must not +// take the help path. +func TestRunNoArgsPipedStdoutFails(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + defer func() { _ = r.Close() }() + + var stderr bytes.Buffer + code := run([]string{}, w, &stderr) + if err := w.Close(); err != nil { + t.Fatalf("close: %v", err) + } + out, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read: %v", err) + } + if code == 0 { + t.Fatalf("exit code = 0, want non-zero (stdout %q)", out) + } + if len(out) != 0 { + t.Errorf("stdout = %q, want empty", out) + } +} + +func TestIsTerminal(t *testing.T) { + var buf bytes.Buffer + if isTerminal(&buf) { + t.Error("a bytes.Buffer is not a terminal") + } + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + defer func() { _ = r.Close() }() + defer func() { _ = w.Close() }() + if isTerminal(w) { + t.Error("a pipe is not a terminal") + } + + tty, err := os.OpenFile("/dev/tty", os.O_WRONLY, 0) + if err != nil { + t.Skipf("no controlling terminal: %v", err) + } + defer func() { _ = tty.Close() }() + if !isTerminal(tty) { + t.Error("/dev/tty is a terminal") + } +} + +// --config with an empty value is a malformed call, not a request to resolve. +func TestRunRejectsEmptyConfigFlag(t *testing.T) { + srv, _ := newVaultStub(t, stubOptions{}) + seedXDGConfig(t, srv.URL) + + for _, args := range [][]string{ + {"host.example.com", "--config", "", "--json"}, + {"ca", "--config", ""}, + } { + var stdout, stderr bytes.Buffer + code := run(args, &stdout, &stderr) + if code == 0 { + t.Fatalf("%v: exit code = 0, want non-zero (stdout %q)", args, stdout.String()) + } + if stdout.Len() != 0 { + t.Errorf("%v: stdout = %q, want empty", args, stdout.String()) + } + if !strings.Contains(stderr.String(), "--config requires a path") { + t.Errorf("%v: stderr = %q", args, stderr.String()) + } + } +} + // seedXDGConfig writes a config where DefaultPath() looks first. func seedXDGConfig(t *testing.T, addr string) { t.Helper() diff --git a/internal/config/config.go b/internal/config/config.go index 2648b83..d341629 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -40,7 +40,8 @@ func FallbackPath() string { return defaultPath } // are only taken when the file is actually there, so an unset or unusable // HOME lands on the Puppet-managed path instead of a path that cannot exist. func DefaultPath() string { - if base := os.Getenv("XDG_CONFIG_HOME"); base != "" { + // The XDG spec requires a relative XDG_CONFIG_HOME to be ignored. + if base := os.Getenv("XDG_CONFIG_HOME"); filepath.IsAbs(base) { if p := filepath.Join(base, "certmanager", "config.yaml"); isFile(p) { return p } @@ -53,7 +54,8 @@ func DefaultPath() string { return defaultPath } -// Resolve picks the config path for a run: an explicit --config always wins. +// Resolve picks the config path for a run: a non-empty explicit --config +// always wins, an empty one means "resolve". func Resolve(explicit string) string { if explicit != "" { return explicit diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e4687fe..3ad41c1 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -204,7 +204,7 @@ func TestDefaultPathIgnoresADirectory(t *testing.T) { func TestResolvePrefersExplicitPath(t *testing.T) { xdg := t.TempDir() - seedConfig(t, xdg) + resolved := seedConfig(t, xdg) t.Setenv("XDG_CONFIG_HOME", xdg) t.Setenv("HOME", t.TempDir()) @@ -212,8 +212,22 @@ func TestResolvePrefersExplicitPath(t *testing.T) { if got := Resolve(explicit); got != explicit { t.Errorf("Resolve(%q) = %q, want the explicit path", explicit, got) } - if got := Resolve(""); got == explicit { - t.Error("Resolve(\"\") returned the explicit path") + if got := Resolve(""); got != resolved { + t.Errorf("Resolve(\"\") = %q, want the resolved path %q", got, resolved) + } +} + +// The XDG spec says a non-absolute XDG_CONFIG_HOME must be ignored, so a +// config sitting under the working directory cannot be picked up. +func TestDefaultPathIgnoresRelativeXDG(t *testing.T) { + wd := t.TempDir() + seedConfig(t, filepath.Join(wd, "xdg")) + t.Chdir(wd) + t.Setenv("XDG_CONFIG_HOME", "xdg") + t.Setenv("HOME", t.TempDir()) + + if got := DefaultPath(); got != FallbackPath() { + t.Errorf("DefaultPath() = %q, want %q", got, FallbackPath()) } } diff --git a/test/integration/certmanager_test.go b/test/integration/certmanager_test.go index b3a0799..5981d1a 100644 --- a/test/integration/certmanager_test.go +++ b/test/integration/certmanager_test.go @@ -272,9 +272,24 @@ func writeConfig(t *testing.T, dir, roleID string) string { return path } +// hermeticEnv strips the developer's environment from the subprocess. The +// tests claim to exercise the baked-in config path, so a real +// ~/.config/certmanager/config.yaml must not resolve ahead of it and point the +// binary at a production Vault. +func hermeticEnv(t *testing.T) []string { + t.Helper() + empty := t.TempDir() + return []string{ + "PATH=/usr/bin:/bin", + "HOME=" + empty, + "XDG_CONFIG_HOME=" + empty, + } +} + func runCertmanager(t *testing.T, bin string, args ...string) (int, string, string) { t.Helper() cmd := exec.Command(bin, args...) + cmd.Env = hermeticEnv(t) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -474,6 +489,41 @@ func TestCAChainNeedsNoCredentials(t *testing.T) { } } +// A developer with a real ~/.config/certmanager/config.yaml must not have it +// leak into the subprocess, which would run the suite against their Vault. +func TestIgnoresTheDevelopersConfig(t *testing.T) { + waitForVault(t) + roleID := setupVault(t) + + dir := t.TempDir() + bin := buildBinary(t, dir, writeConfig(t, dir, roleID)) + + poisoned := t.TempDir() + cfgDir := filepath.Join(poisoned, "certmanager") + if err := os.MkdirAll(cfgDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + body := "vault:\n addr: http://127.0.0.1:1\n mount_point: nope\n role_name: nope\n" + + " approle_path: approle\n role_id: nope\n" + if err := os.WriteFile(filepath.Join(cfgDir, "config.yaml"), []byte(body), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + t.Setenv("XDG_CONFIG_HOME", poisoned) + t.Setenv("HOME", poisoned) + + code, stdout, stderr := runCertmanager(t, bin, commonName, "--json") + if code != 0 { + t.Fatalf("exit code %d, stderr: %s", code, stderr) + } + var doc map[string]string + if err := json.Unmarshal([]byte(stdout), &doc); err != nil { + t.Fatalf("stdout is not JSON: %v\n%s", err, stdout) + } + if doc["certificate"] == "" { + t.Error("no certificate issued; the baked-in config path was not used") + } +} + func TestCAChainFailsCleanlyForAnUnknownMount(t *testing.T) { waitForVault(t) roleID := setupVault(t)