From ddd93a8b71abba5f3e534e4197bc14e8d60295c6 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Tue, 22 Sep 2026 23:48:59 +1000 Subject: [PATCH] Assert the hermetic env replaces the ambient one, not extends it The shape test passed on `return os.Environ()`; only a Vault-backed test caught it. Move the helper and its test out from behind the integration tag and reject any entry outside PATH/HOME/XDG_CONFIG_HOME. --- test/integration/certmanager_test.go | 36 --------------- test/integration/doc.go | 6 +-- test/integration/hermetic_env_test.go | 66 +++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 39 deletions(-) create mode 100644 test/integration/hermetic_env_test.go diff --git a/test/integration/certmanager_test.go b/test/integration/certmanager_test.go index 568fc1a..e837d49 100644 --- a/test/integration/certmanager_test.go +++ b/test/integration/certmanager_test.go @@ -272,42 +272,6 @@ 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, - } -} - -// Neither leg of the resolution order announces itself when it is missing: an -// unset HOME or XDG_CONFIG_HOME silently falls through to whatever the -// developer has. Both must therefore be set to somewhere empty, not dropped. -func TestHermeticEnvSetsBothConfigLegs(t *testing.T) { - env := hermeticEnv(t) - for _, key := range []string{"HOME=", "XDG_CONFIG_HOME="} { - idx := slices.IndexFunc(env, func(e string) bool { return strings.HasPrefix(e, key) }) - if idx < 0 { - t.Fatalf("%s is unset in the subprocess env: %v", key, env) - } - dir := strings.TrimPrefix(env[idx], key) - if !filepath.IsAbs(dir) { - t.Errorf("%s%s is not an absolute path", key, dir) - } - for _, rel := range []string{"certmanager/config.yaml", ".config/certmanager/config.yaml"} { - if _, err := os.Stat(filepath.Join(dir, rel)); err == nil { - t.Errorf("%s%s already holds %s", key, dir, rel) - } - } - } -} - func runCertmanager(t *testing.T, bin string, args ...string) (int, string, string) { t.Helper() cmd := exec.Command(bin, args...) diff --git a/test/integration/doc.go b/test/integration/doc.go index dde368b..a7d8776 100644 --- a/test/integration/doc.go +++ b/test/integration/doc.go @@ -1,5 +1,5 @@ // Package integration holds the end-to-end tests that run against a real Vault -// dev server. The tests are behind the `integration` build tag; this file -// keeps the package buildable for a plain `go test ./...`, which otherwise -// fails on a package whose files are all excluded by build constraints. +// dev server. Those tests are behind the `integration` build tag; the +// subprocess environment they share, and its own test, are not, so a plain +// `go test ./...` still guards the helper the tagged tests depend on. package integration diff --git a/test/integration/hermetic_env_test.go b/test/integration/hermetic_env_test.go new file mode 100644 index 0000000..3bbc3bb --- /dev/null +++ b/test/integration/hermetic_env_test.go @@ -0,0 +1,66 @@ +package integration + +import ( + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +// 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, + } +} + +// Neither leg of the resolution order announces itself when it is missing: an +// unset HOME or XDG_CONFIG_HOME silently falls through to whatever the +// developer has. Both must therefore be set to somewhere empty, not dropped. +// Setting them is not enough either: the env has to replace the ambient one +// rather than extend it, so anything the developer exported is asserted gone. +func TestHermeticEnvSetsBothConfigLegs(t *testing.T) { + t.Setenv("CERTMANAGER_AMBIENT_MARKER", "leaked") + t.Setenv("HOME", filepath.Join(t.TempDir(), "developer")) + + env := hermeticEnv(t) + + allowed := []string{"PATH", "HOME", "XDG_CONFIG_HOME"} + for _, entry := range env { + key, _, ok := strings.Cut(entry, "=") + if !ok { + t.Errorf("%q is not a KEY=VALUE entry", entry) + continue + } + if !slices.Contains(allowed, key) { + t.Errorf("%s is carried over from the ambient environment; want only %v", key, allowed) + } + } + + for _, key := range []string{"HOME=", "XDG_CONFIG_HOME="} { + idx := slices.IndexFunc(env, func(e string) bool { return strings.HasPrefix(e, key) }) + if idx < 0 { + t.Fatalf("%s is unset in the subprocess env: %v", key, env) + } + dir := strings.TrimPrefix(env[idx], key) + if !filepath.IsAbs(dir) { + t.Errorf("%s%s is not an absolute path", key, dir) + } + if ambient := os.Getenv(strings.TrimSuffix(key, "=")); ambient != "" && dir == ambient { + t.Errorf("%s%s is the ambient value, not a scratch directory", key, dir) + } + for _, rel := range []string{"certmanager/config.yaml", ".config/certmanager/config.yaml"} { + if _, err := os.Stat(filepath.Join(dir, rel)); err == nil { + t.Errorf("%s%s already holds %s", key, dir, rel) + } + } + } +}