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

@@ -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

README.md

@@ -21,13 +21,18 @@ failure path exits non-zero.
 ```
 certmanager <common_name> [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 <path>`, which always wins;
-2. `$XDG_CONFIG_HOME/certmanager/config.yaml`, if the file exists;
+1. `--config <path>`, 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=...`).

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
 			}

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()

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

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())
 	}
 }
 

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)