diff --git a/AGENTS.md b/AGENTS.md index 1b2bfcb..30f5f72 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,7 +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. `run` derives interactivity from `isTerminal(stdout)` and hands it to `runWith`, which tests - call directly to pin either side. + call directly to pin either side. `isTerminal` is a terminal ioctl + (`x/term.IsTerminal`), not a character-device stat: `/dev/null` and the + `/dev/null` the runtime reopens onto a closed fd 1 are character devices, and + a stat would hand both of them the help-and-exit-zero path. - **Flat CLI**: the root command takes `common_name` positionally (`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 @@ -107,6 +110,12 @@ four JSON keys, the parsed X.509 CN/SANs/lifetime, and that a refused name exits non-zero with empty stdout. Dev mode is plain HTTP, so it does not exercise the TLS path. +`TestBareInvocationHelpsOnlyATerminal` drives the built binary once per kind of +fd 1 — pipe, regular file, unix socket, `/dev/null`, a closed fd 1 and a pty +allocated from `/dev/ptmx` — and only the pty may exit zero with help on +stdout. The pty is what pins the true branch: CI containers have no +controlling terminal, so a test that reached for `/dev/tty` would skip. + ## Gotchas - Changing the JSON key set or key order breaks `parsejson()` in diff --git a/README.md b/README.md index 1e85850..3862e0b 100644 --- a/README.md +++ b/README.md @@ -28,10 +28,11 @@ 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. +A bare invocation prints help and exits zero only when stdout is a terminal — +a pipe, a file, a socket, `/dev/null` and a closed fd 1 all exit non-zero with +nothing on stdout. Puppet's `generate()` reads stdout through a pipe, so there +a missing common name fails the compile rather than feeding help text into the +catalog. | Flag | Short | Type | Default | Description | |---|---|---|---|---| diff --git a/cmd/certmanager/main.go b/cmd/certmanager/main.go index c3fb629..bec4cc1 100644 --- a/cmd/certmanager/main.go +++ b/cmd/certmanager/main.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/spf13/cobra" + "golang.org/x/term" "git.unkin.net/unkin/certmanager/internal/config" "git.unkin.net/unkin/certmanager/internal/pki" @@ -43,14 +44,15 @@ func runWith(args []string, stdout, stderr io.Writer, interactive bool) int { } // isTerminal reports whether w is a terminal. Puppet's generate() captures -// stdout through a pipe, so anything else is a machine caller. +// stdout through a pipe, so anything else is a machine caller. The check is a +// terminal ioctl, not a character-device stat: /dev/null is a character device, +// and so is the /dev/null the Go runtime reopens onto a closed fd 1 at startup. 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 + return term.IsTerminal(int(f.Fd())) } // resolveConfig turns the --config value into a path. An explicitly empty diff --git a/cmd/certmanager/main_test.go b/cmd/certmanager/main_test.go index e8e8934..4dfd50f 100644 --- a/cmd/certmanager/main_test.go +++ b/cmd/certmanager/main_test.go @@ -13,6 +13,7 @@ import ( "fmt" "io" "math/big" + "net" "net/http" "net/http/httptest" "os" @@ -832,7 +833,10 @@ func TestRunNoArgsPipedStdoutFails(t *testing.T) { } } -func TestIsTerminal(t *testing.T) { +// Every stdout a machine caller can hand the binary must read as +// not-a-terminal. /dev/null is the trap: it is a character device, so a stat of +// the file mode calls it a terminal and lets help escape with exit zero. +func TestIsTerminalRejectsNonTerminals(t *testing.T) { var buf bytes.Buffer if isTerminal(&buf) { t.Error("a bytes.Buffer is not a terminal") @@ -842,20 +846,65 @@ func TestIsTerminal(t *testing.T) { 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") - } + t.Cleanup(func() { _ = r.Close(); _ = w.Close() }) - tty, err := os.OpenFile("/dev/tty", os.O_WRONLY, 0) + regular, err := os.Create(filepath.Join(t.TempDir(), "stdout")) if err != nil { - t.Skipf("no controlling terminal: %v", err) + t.Fatalf("create: %v", err) } - defer func() { _ = tty.Close() }() - if !isTerminal(tty) { - t.Error("/dev/tty is a terminal") + t.Cleanup(func() { _ = regular.Close() }) + + devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + t.Fatalf("open %s: %v", os.DevNull, err) } + t.Cleanup(func() { _ = devNull.Close() }) + + for name, f := range map[string]*os.File{ + "pipe": w, + "regular file": regular, + os.DevNull: devNull, + "unix socket": unixSocket(t), + } { + if isTerminal(f) { + t.Errorf("%s is not a terminal", name) + } + var stderr bytes.Buffer + if code := run(nil, f, &stderr); code == 0 { + t.Errorf("%s: bare invocation exit code = 0, want non-zero", name) + } + } +} + +// unixSocket returns a connected unix socket as an *os.File: the stdout a +// caller gets when it wires the binary straight into a service. +func unixSocket(t *testing.T) *os.File { + t.Helper() + path := filepath.Join(t.TempDir(), "s") + ln, err := net.Listen("unix", path) + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = ln.Close() }) + go func() { + conn, err := ln.Accept() + if err == nil { + _, _ = io.Copy(io.Discard, conn) + _ = conn.Close() + } + }() + + conn, err := net.Dial("unix", path) + if err != nil { + t.Fatalf("dial: %v", err) + } + t.Cleanup(func() { _ = conn.Close() }) + f, err := conn.(*net.UnixConn).File() + if err != nil { + t.Fatalf("socket file: %v", err) + } + t.Cleanup(func() { _ = f.Close() }) + return f } // --config with an empty value is a malformed call, not a request to resolve. diff --git a/cmd/certmanager/terminal_linux_test.go b/cmd/certmanager/terminal_linux_test.go new file mode 100644 index 0000000..0ed8782 --- /dev/null +++ b/cmd/certmanager/terminal_linux_test.go @@ -0,0 +1,25 @@ +//go:build linux + +package main + +import ( + "os" + "syscall" + "testing" +) + +// The help-and-exit-zero branch is the whole point of the gate, and no CI +// container has a controlling terminal to reach it through /dev/tty. A pty +// allocated from /dev/ptmx is a real terminal, so the true branch is pinned +// here instead of skipped. +func TestIsTerminalDetectsAPTY(t *testing.T) { + pty, err := os.OpenFile("/dev/ptmx", syscall.O_RDWR|syscall.O_NOCTTY, 0) + if err != nil { + t.Fatalf("open /dev/ptmx: %v", err) + } + t.Cleanup(func() { _ = pty.Close() }) + + if !isTerminal(pty) { + t.Error("a pty is a terminal") + } +} diff --git a/go.mod b/go.mod index 7bcb130..0b1aa05 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,8 @@ go 1.25.7 require ( github.com/spf13/cobra v1.10.2 + golang.org/x/sys v0.46.0 + golang.org/x/term v0.44.0 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index 47edb24..499532b 100644 --- a/go.sum +++ b/go.sum @@ -7,6 +7,10 @@ github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiT github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= +golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= +golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=