diff --git a/CHANGELOG.md b/CHANGELOG.md index de699d0..88372b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed + +- `grant login` no longer hangs on WSL2; grant now detects WSL and forces the file-based keyring. A token stored in the OS keyring becomes invisible — re-run `grant login` if prompted + ### Added - End-to-end self-update tests that replace a real, running binary (`internal/selfupdate/e2e_test.go`, build tag `selfupdate_e2e`). They compile two fixture binaries from a dependency-free module, execute one, and swap it through grant's own apply path while a process is still running from that image — so the Windows file-locking semantics behind the two-rename swap are actually exercised, not just the bookkeeping. Success and rollback paths are both covered, and the rolled-back binary is asserted to still run. No network access is required diff --git a/CLAUDE.md b/CLAUDE.md index b106ab7..5efd740 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -157,6 +157,27 @@ Custom `SCAAccessService` follows SDK conventions: - SDK profile: `~/.idsec/profiles/grant` (default; override via `IDSEC_PROFILES_FOLDER`) - Always resolve the profile directory with `profiles.GetProfilesFolder()` (SDK) — never hand-roll it. The SDK reads `os.Getenv("HOME")`, not `os.UserHomeDir()`; on Windows `HOME` is frequently unset, so it resolves to a **relative** `.idsec/profiles` under the process CWD. Any code that prints or computes the profile path must agree with the loader, so reproduce the SDK's behavior rather than "correcting" it +## Keyring +- The SDK stores the auth token via `pkg/common/keyring`. `GetKeyring(enforceBasic bool)` (`idsec_keyring.go:117-131`) picks: basic (file) keyring if Docker **or** its own `isWSL()` **or** `IDSEC_BASIC_KEYRING != ""` **or** `enforceBasic`; else the OS keyring on windows/darwin, and on linux the OS-provided (D-Bus/libsecret) keyring whenever `DBUS_SESSION_BUS_ADDRESS` is non-empty; else basic +- **SDK bug (v0.8.1):** `isWSL()` (`:90-95`) matches `"Microsoft"` **case-sensitively** against `/proc/version`. Modern WSL2 reports `-microsoft-standard-WSL2`, so it returns false. Under WSLg `DBUS_SESSION_BUS_ADDRESS` is set, the D-Bus keyring is chosen, and the call can block forever against an unresponsive `gnome-keyring-daemon` +- **The SDK's fallbacks cannot save you.** Both the OS-keyring wrapper and `SaveToken` (`:154-171`) fall back to the basic keyring only when the underlying call returns an **error**. A hang is not an error, so the fallback is unreachable. The hang is also not write-only — `LoadToken`/`GetPassword` can wedge before anything is ever written +- **`IDSEC_BASIC_KEYRING` semantics:** the SDK tests `os.Getenv(...) != ""`, so **any** non-empty value forces the file keyring, `0` and `false` included. Empty/unset does not itself force it — the other `GetKeyring` conditions (Docker, SDK-detected WSL, `enforceBasic`) still apply +- **grant's override:** `internal/keyringenv` detects WSL and sets `IDSEC_BASIC_KEYRING=1`. A non-empty existing value is preserved; an explicitly **empty** value is overwritten on WSL, because empty is precisely the dangerous setting. No-op off linux (the `GOOS` guard short-circuits before any filesystem access) +- Signals, OR'd, in the order the diagnostic reports them: `/run/WSL` exists → `/proc/sys/fs/binfmt_misc/WSLInterop` exists → `/proc/sys/kernel/osrelease` contains `microsoft`/`wsl` → `/proc/version` contains `microsoft` → non-empty `WSL_DISTRO_NAME`/`WSL_INTEROP`. All string matching is case-insensitive +- **Why the token sets differ:** `osrelease` is short and structured, so `wsl` is safe there — systemd matches `Microsoft`/`WSL` on it in `src/basic/virt.c`. (npm `is-wsl` is *not* a precedent for the `wsl` token: it matches only `microsoft`, on `os.release()` then `/proc/version`, before falling back to `WSLInterop`/`/run/WSL`, with everything gated behind `!isInsideContainer()`. It is a precedent for the markers.) `/proc/version` is free-form and carries the kernel build user, build host and compiler banner, so a bare `wsl` there would match a plain Linux box built on a host named `wsl-builder` — `microsoft` only +- **`/run/WSL` is the strongest single signal**, not the string matching: it survives custom kernels, `sudo`, systemd units and cron. `WSL_DISTRO_NAME` is lost under `sudo -i` (microsoft/WSL#5914) and absent in systemd units (#9719), and custom WSL2 kernels may carry neither token (#6911). snapd abandoned string matching for this marker after Launchpad #1991823. WSL's init creates it in `InteropServer::Create()` (`src/linux/init/util.cpp`) from an unguarded call in `ConfigInitializeInstance()` (`src/linux/init/config.cpp`), so it appears even when interop is disabled +- **`WSLInterop` is a supplement only** — but not for the reason previously recorded here. The old claim was "that binfmt entry exists only when interop is enabled". That is true on **WSL1**, where the per-distro registration is gated on `Config.InteropEnabled` (`src/linux/init/config.cpp:543-551`); on WSL2 the entry is registered at VM level and is kernel-global, so it is instead vulnerable to being wiped or shadowed VM-wide +- **All five signals are deliberately redundant, and the redundancy is about *visibility*, not reliability.** This is the point that stops someone simplifying this again — an attempt in `db73645` was reverted in `23ac108` for exactly this reason. The five sources fail **independently** because they are reached by different mechanisms: + - filesystem markers (`/run/WSL`, `WSLInterop`) are **namespace-local** — a chroot or mount namespace with a clean `/run`, or without `binfmt_misc` mounted, sees neither, no matter what init did + - proc paths (`osrelease`, `/proc/version`) are **independently maskable** — they are separate mount-visible paths, so one can be masked, omitted or replaced while the other is readable + - env vars (`WSL_DISTRO_NAME`, `WSL_INTEROP`) are the only ones **inherited across** chroot and mount-namespace boundaries, which is precisely where every marker above disappears +- **Content equivalence is not signal redundancy.** `/proc/version` and `osrelease` provably cannot *disagree*: `fs/proc/version.c` does `seq_printf(m, linux_proc_banner, utsname()->sysname, utsname()->release, utsname()->version)`, and `/proc/sys/kernel/osrelease` **is** `utsname()->release` (`kernel/utsname_sysctl.c`, `uts_kern_table` entry `osrelease` → `init_uts_ns.name.release`, resolved per-namespace by `get_uts()`); a UTS namespace change moves both together. Useful for reasoning about the *values*, but **not grounds for removing either** — identical content is no help when one path is not visible. Same shape of argument for the env vars: init sets `WSL_DISTRO_NAME` and creates `/run/WSL` in the same unguarded function, which proves init made the marker, **not** that a descendant process can still see it +- **Error asymmetry drives the tuning:** a false negative attempts the OS keyring under WSLg and hangs indefinitely with no error and no timeout (unrecoverable); a false positive is a file keyring on a plain Linux desktop (bounded security downgrade). Bias toward over-detection. A redundant check is the cheap side of that trade — do not trade it away for tidiness +- Rejected: container gating (`/run/WSL` is container-safe since a container gets its own `/run` tmpfs). Note the *old* justification — "inside a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks basic" — is **refuted**: WSL init injects `DBUS_SESSION_BUS_ADDRESS` explicitly for systemd-backed launches (`src/linux/init/init.cpp`), and ordinary `chroot` preserves it, so the SDK cannot be assumed to fall back on its own. Also rejected: `WSLENV` (user-configurable, often absent), shelling out to `systemd-detect-virt`, and third-party libs (`gookit/goutil` has the same case-sensitive `"Microsoft"` bug) +- Applied in `executeWithKeyringOverride` (`cmd/root.go`), called from `Execute()` before `rootCmd.Execute()` — the single deterministic entry point for the binary, ahead of every keyring access (`cmd/login.go`, `cmd/root.go`, `cmd/logout.go`). It **fails closed**: a `Setenv` error aborts before any command runs +- The applied notice is stashed in `keyringEnvNotice` and emitted from `PersistentPreRunE` inside the `if verbose` branch, so it is verbose-only and unit-testable with `spyLogger` (gating on `IDSEC_LOG_LEVEL` alone would be invisible to the spy) +- Backend switch caveat: a token written to the OS keyring is invisible to the file keyring. Worst case the user re-runs `grant login` + ## Authentication - Use the `/grant-login` skill when you need to authenticate to the grant CLI (e.g., before manual testing) - Skill definition: `.claude/skills/grant-login/SKILL.md` diff --git a/README.md b/README.md index ee4e54a..cff5dd7 100644 --- a/README.md +++ b/README.md @@ -174,6 +174,7 @@ favorites: |----------|-------------|---------| | `GRANT_CONFIG` | Custom path to app config YAML | `~/.grant/config.yaml` | | `IDSEC_LOG_LEVEL` | SDK log level (`DEBUG`, `INFO`, `CRITICAL`) — overrides `--verbose` | Not set | +| `IDSEC_BASIC_KEYRING` | Store the auth token in the SDK's encrypted file keyring instead of the OS keyring. **Any non-empty value forces file storage — including `0` and `false`.** Empty or unset does not itself force it (the SDK still picks file storage in Docker and in the cases it detects as WSL). grant sets it to `1` automatically when it detects WSL; an existing non-empty value is never overridden | Not set (auto-set to `1` on WSL) | ## Troubleshooting @@ -184,6 +185,7 @@ favorites: | "Failed to elevate" | Check `grant status` for active sessions; verify target/role names | | `grant env` errors for Azure/GCP | `env` is AWS-only — Azure and GCP return no credentials, use `grant` directly | | Permission denied accessing keyring (Linux) | Install and start `gnome-keyring` or `kwalletmanager` | +| `grant login` hangs forever with no output (Linux/WSL) | The OS keyring (D-Bus/libsecret) is unresponsive and the call never returns. grant forces the file keyring on WSL automatically — run with `--verbose` to confirm. On other Linux hosts, set `IDSEC_BASIC_KEYRING=1` and retry. Switching backends hides any token already stored in the OS keyring; just re-run `grant login` | ## Development diff --git a/cmd/keyring_override_test.go b/cmd/keyring_override_test.go new file mode 100644 index 0000000..ab7258f --- /dev/null +++ b/cmd/keyring_override_test.go @@ -0,0 +1,156 @@ +package cmd + +import ( + "errors" + "io" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// resetKeyringOverrideState restores the package globals this file mutates. +func resetKeyringOverrideState(t *testing.T) { + t.Helper() + origApply := keyringApply + origNotice := keyringEnvNotice + origVerbose := verbose + origLog := log + t.Cleanup(func() { + keyringApply = origApply + keyringEnvNotice = origNotice + verbose = origVerbose + log = origLog + }) +} + +// TestExecuteWithKeyringOverrideRunsBeforeCommand proves the keyring override +// is applied at startup, before any command code executes. +func TestExecuteWithKeyringOverrideRunsBeforeCommand(t *testing.T) { + resetKeyringOverrideState(t) + + var order []string + keyringApply = func() (bool, string, error) { + order = append(order, "keyring-override") + return true, "WSL detected (test); forcing file-based keyring", nil + } + keyringEnvNotice = "" + + cmd := newRootCommand(func(*cobra.Command, []string) error { + order = append(order, "command-run") + return nil + }) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{}) + + if err := executeWithKeyringOverride(cmd); err != nil { + t.Fatalf("executeWithKeyringOverride() error = %v", err) + } + + want := []string{"keyring-override", "command-run"} + if len(order) != len(want) || order[0] != want[0] || order[1] != want[1] { + t.Fatalf("execution order = %v, want %v", order, want) + } + if keyringEnvNotice == "" { + t.Error("keyringEnvNotice was not stashed after the override applied") + } +} + +func TestExecuteWithKeyringOverrideFailsClosed(t *testing.T) { + resetKeyringOverrideState(t) + + keyringApply = func() (bool, string, error) { + return false, "", errors.New("setenv denied") + } + keyringEnvNotice = "" + + ran := false + cmd := newRootCommand(func(*cobra.Command, []string) error { + ran = true + return nil + }) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{}) + + err := executeWithKeyringOverride(cmd) + if err == nil { + t.Fatal("executeWithKeyringOverride() error = nil, want an error") + } + if !strings.Contains(err.Error(), "setenv denied") { + t.Errorf("error = %q, want it to wrap the underlying cause", err) + } + if ran { + t.Error("command executed despite the keyring override failing; want fail-closed") + } +} + +func TestExecuteWithKeyringOverrideNoNoticeWhenNotApplied(t *testing.T) { + resetKeyringOverrideState(t) + + keyringApply = func() (bool, string, error) { return false, "not WSL", nil } + keyringEnvNotice = "" + + cmd := newRootCommand(func(*cobra.Command, []string) error { return nil }) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{}) + + if err := executeWithKeyringOverride(cmd); err != nil { + t.Fatalf("executeWithKeyringOverride() error = %v", err) + } + if keyringEnvNotice != "" { + t.Errorf("keyringEnvNotice = %q, want empty when the override was not applied", keyringEnvNotice) + } +} + +// TestKeyringNoticeIsVerboseOnly checks that the stashed notice reaches the +// logger only when --verbose is passed. +// +// Note: the spy logger records Info() calls regardless of the SDK log level, so +// gating purely on IDSEC_LOG_LEVEL would be invisible to this test. The +// implementation therefore gates the emission on the parsed --verbose flag +// itself, which is what this test asserts. +func TestKeyringNoticeIsVerboseOnly(t *testing.T) { + tests := []struct { + name string + args []string + notice string + wantLogs bool + }{ + {name: "verbose emits the notice", args: []string{"--verbose"}, notice: "WSL detected; forcing file-based keyring", wantLogs: true}, + {name: "non-verbose stays silent", args: []string{}, notice: "WSL detected; forcing file-based keyring", wantLogs: false}, + {name: "verbose with no notice logs nothing", args: []string{"--verbose"}, notice: "", wantLogs: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resetKeyringOverrideState(t) + + spy := &spyLogger{} + log = spy + keyringEnvNotice = tt.notice + verbose = false + + cmd := newRootCommand(func(*cobra.Command, []string) error { return nil }) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs(tt.args) + + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute() error = %v", err) + } + + var found bool + for _, entry := range spy.messages { + if strings.Contains(entry, "keyring") { + found = true + } + } + if found != tt.wantLogs { + t.Errorf("notice logged = %v, want %v (calls: %v)", found, tt.wantLogs, spy.messages) + } + }) + } +} diff --git a/cmd/root.go b/cmd/root.go index 8124b3d..729903f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -14,6 +14,7 @@ import ( survey "github.com/Iilun/survey/v2" "github.com/aaearon/grant-cli/internal/cache" "github.com/aaearon/grant-cli/internal/config" + "github.com/aaearon/grant-cli/internal/keyringenv" "github.com/aaearon/grant-cli/internal/sca" "github.com/aaearon/grant-cli/internal/sca/models" "github.com/aaearon/grant-cli/internal/ui" @@ -39,6 +40,18 @@ var verbose bool // so this stays false — allowing Execute() to suppress the verbose hint. var passedArgValidation bool +// keyringApply forces the SDK's file-based keyring backend when running under +// WSL. Injectable for tests. +var keyringApply = keyringenv.Apply + +// keyringEnvNotice holds the diagnostic produced when the keyring override is +// applied. It is stashed at startup (before --verbose is parsed) and emitted +// from PersistentPreRunE once the flag is known. +// +// It is set, never cleared: the binary applies the override exactly once per +// process. Tests that invoke the wrapper repeatedly must reset it themselves. +var keyringEnvNotice string + // elevateFlags holds the command-line flags for elevation type elevateFlags struct { provider string @@ -96,6 +109,9 @@ Examples: passedArgValidation = true if verbose { sdkconfig.EnableVerboseLogging("INFO") + if keyringEnvNotice != "" { + log.Info("%s", keyringEnvNotice) + } } else { sdkconfig.DisableVerboseLogging() } @@ -252,9 +268,25 @@ func NewRootCommandWithDeps( }) } +// executeWithKeyringOverride applies the WSL keyring override before running +// cmd, so no keyring access can happen against a backend that may hang. +// +// It fails closed: if the override cannot be applied the command never runs, +// because continuing would walk the user into an unbounded D-Bus hang. +func executeWithKeyringOverride(cmd *cobra.Command) error { + applied, reason, err := keyringApply() + if err != nil { + return fmt.Errorf("could not force the file-based keyring backend: %w (set IDSEC_BASIC_KEYRING=1 manually and retry)", err) + } + if applied { + keyringEnvNotice = reason + } + return cmd.Execute() +} + func Execute() { passedArgValidation = false - if err := rootCmd.Execute(); err != nil { + if err := executeWithKeyringOverride(rootCmd); err != nil { fmt.Fprintln(rootCmd.ErrOrStderr(), err) if !verbose && passedArgValidation { fmt.Fprintln(rootCmd.ErrOrStderr(), "Hint: re-run with --verbose for more details") diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go new file mode 100644 index 0000000..14c6c9d --- /dev/null +++ b/internal/keyringenv/keyringenv.go @@ -0,0 +1,189 @@ +// Package keyringenv detects WSL and forces the SDK's file-based ("basic") +// keyring backend before any keyring access happens. +// +// Why: idsec-sdk-golang's GetKeyring picks the OS-provided (D-Bus/libsecret) +// keyring on Linux whenever DBUS_SESSION_BUS_ADDRESS is set. Its own WSL guard +// matches "Microsoft" case-sensitively against /proc/version, which modern WSL2 +// kernels (".. -microsoft-standard-WSL2") do not satisfy. Under WSLg that D-Bus +// call can block forever, and the SDK's fallbacks only trigger on a returned +// error — a hang is never an error, so it is unrecoverable. Setting +// IDSEC_BASIC_KEYRING is the only lever available without vendoring the SDK. +package keyringenv + +import ( + "fmt" + "os" + "runtime" + "strings" +) + +const ( + // envVar is the SDK's IdsecBasicKeyringOverrideEnvVar. The SDK checks + // os.Getenv(envVar) != "", so ANY non-empty value forces the basic keyring. + envVar = "IDSEC_BASIC_KEYRING" + + procVersionPath = "/proc/version" + procOSReleasePath = "/proc/sys/kernel/osrelease" + + // runWSLPath is the marker directory the WSL init creates. It is the only + // signal that survives custom kernels, sudo, systemd units and cron, where + // the WSL_* env vars are absent (microsoft/WSL#5914, #9719) and a custom + // kernel may carry neither "microsoft" nor "wsl" in its version strings + // (microsoft/WSL#6911). snapd moved to exactly this marker after + // Launchpad #1991823. + runWSLPath = "/run/WSL" + + // wslInteropPath is a supplement to runWSLPath, never a replacement — that + // insufficiency is what Launchpad #1991823 documents. On WSL1 the + // per-distro registration is gated on Config.InteropEnabled + // (src/linux/init/config.cpp), so disabling interop in wsl.conf removes it; + // on WSL2 the entry is registered VM-wide and is kernel-global, so it can + // be wiped or shadowed for every distro at once. It is also absent in any + // mount namespace that does not mount binfmt_misc. + wslInteropPath = "/proc/sys/fs/binfmt_misc/WSLInterop" +) + +// Detector inspects the environment for WSL and applies the keyring override. +// The zero value is not usable; use New or the package-level Apply. +type Detector struct { + ReadFile func(string) ([]byte, error) + Exists func(string) bool + LookupEnv func(string) (string, bool) + Setenv func(string, string) error + GOOS string +} + +// New returns a Detector wired to the real OS. +func New() Detector { + return Detector{ + ReadFile: os.ReadFile, + Exists: func(path string) bool { + _, err := os.Stat(path) + return err == nil + }, + LookupEnv: os.LookupEnv, + Setenv: os.Setenv, + GOOS: runtime.GOOS, + } +} + +// IsWSL reports whether the current process runs under WSL. It is a no-op +// (false) off Linux. Unreadable /proc files are "no signal", never an error. +// +// There is no supported Microsoft API for this; the de-facto reference is +// microsoft/WSL#423, which systemd follows in src/basic/virt.c. +// +// The errors are asymmetric, which is what drives the tuning: +// - a false negative attempts the OS keyring under WSLg, which can block +// indefinitely with no error and no timeout — unrecoverable; +// - a false positive uses a file-based keyring on a plain Linux desktop — a +// real but bounded security downgrade. +// +// So bias toward over-detection, but via the /run/WSL marker, which is +// simultaneously broader and more specific, not via looser string matching. +func (d Detector) IsWSL() bool { + _, ok := d.wslSignal() + return ok +} + +// wslSignal returns the first WSL indicator found, and whether one was found. +// +// All five signals are deliberately kept. The redundancy is about VISIBILITY, +// not reliability, and that is why "this one is weaker" is never on its own a +// reason to delete one — they fail independently because each is reached by a +// different mechanism: +// +// - the filesystem markers are namespace-local: a chroot or mount namespace +// with a clean /run, or without binfmt_misc mounted, sees neither, whatever +// WSL's init did; +// - the two proc paths are independently maskable, being separate +// mount-visible paths; +// - the env vars are the only signal inherited ACROSS chroot and +// mount-namespace boundaries — exactly where every marker above vanishes. +// +// Beware the trap that produced (and reverted) commit db73645: content +// equivalence is not signal redundancy. /proc/version and osrelease provably +// cannot disagree — the kernel renders both from utsname()->release via the +// caller's UTS namespace (fs/proc/version.c; kernel/utsname_sysctl.c) — and +// init sets WSL_DISTRO_NAME and creates /run/WSL in the same unguarded +// function. Neither fact means a given process can still SEE the other source. +// Only delete a signal you can show is unreachable-when-the-others-are-not. +func (d Detector) wslSignal() (string, bool) { + if d.GOOS != "linux" { + return "", false + } + // Filesystem markers first: they are both broader and more specific than + // string matching, and they are what covers custom kernels. + for _, path := range []string{runWSLPath, wslInteropPath} { + if d.Exists(path) { + return path, true + } + } + // Then the kernel strings, case-insensitively — the case sensitivity is the + // SDK's actual bug. The token sets differ deliberately: + // - osrelease is short and structured ("6.18.33.2-microsoft-standard-WSL2"), + // so matching "wsl" there is safe. systemd does this (Microsoft||WSL on + // osrelease). npm's is-wsl does NOT: it matches only "microsoft", on + // os.release() and then /proc/version, before falling back to the two + // filesystem markers below — all gated behind !isInsideContainer(). + // - /proc/version is free-form and carries the kernel build user, build + // host and full compiler banner, so a bare "wsl" would match a plain + // Linux box built by user "wsl" or on host "wsl-builder". "microsoft" + // only. + for _, f := range []struct { + path string + tokens []string + }{ + {procOSReleasePath, []string{"microsoft", "wsl"}}, + {procVersionPath, []string{"microsoft"}}, + } { + data, err := d.ReadFile(f.path) + if err != nil { + continue + } + lower := strings.ToLower(string(data)) + for _, token := range f.tokens { + if strings.Contains(lower, token) { + return f.path, true + } + } + } + for _, key := range []string{"WSL_DISTRO_NAME", "WSL_INTEROP"} { + if v, ok := d.LookupEnv(key); ok && v != "" { + return key, true + } + } + return "", false +} + +// Apply forces the SDK's file-based keyring when running under WSL. +// +// It reports whether the override was applied and a human-readable reason. +// An existing non-empty IDSEC_BASIC_KEYRING is always preserved: any non-empty +// value already forces the safe basic keyring. An explicitly-empty value is +// the dangerous case — it selects the OS keyring — so on WSL it is treated +// exactly like unset and overwritten. +// +// Apply fails closed: a Setenv error is returned so the caller can abort +// rather than walk the user into a hang with no timeout. +func (d Detector) Apply() (applied bool, reason string, err error) { + if d.GOOS != "linux" { + return false, "not linux; keyring override not needed", nil + } + if v, ok := d.LookupEnv(envVar); ok && v != "" { + return false, envVar + " already set; leaving it alone", nil + } + signal, isWSL := d.wslSignal() + if !isWSL { + return false, "not WSL; keyring override not needed", nil + } + if err := d.Setenv(envVar, "1"); err != nil { + return false, "", fmt.Errorf("setting %s: %w", envVar, err) + } + return true, fmt.Sprintf("WSL detected (%s); forcing the file-based keyring via %s=1", signal, envVar), nil +} + +// Apply runs the detector against the real OS environment. +func Apply() (applied bool, reason string, err error) { + return New().Apply() +} diff --git a/internal/keyringenv/keyringenv_test.go b/internal/keyringenv/keyringenv_test.go new file mode 100644 index 0000000..651b1fa --- /dev/null +++ b/internal/keyringenv/keyringenv_test.go @@ -0,0 +1,417 @@ +package keyringenv + +import ( + "errors" + "os" + "strings" + "testing" +) + +// fakeFS builds a ReadFile func serving the given path->contents map. +// Paths absent from the map return an error (unreadable / missing). +func fakeFS(files map[string]string) func(string) ([]byte, error) { + return func(name string) ([]byte, error) { + if content, ok := files[name]; ok { + return []byte(content), nil + } + return nil, os.ErrNotExist + } +} + +// fakeExists builds an Exists func that reports true only for the given paths. +func fakeExists(paths ...string) func(string) bool { + return func(path string) bool { + for _, p := range paths { + if p == path { + return true + } + } + return false + } +} + +// fakeEnv builds a LookupEnv func from a map. Entries present in the map are +// "set" (even when their value is empty); absent keys are unset. +func fakeEnv(env map[string]string) func(string) (string, bool) { + return func(key string) (string, bool) { + v, ok := env[key] + return v, ok + } +} + +func TestDetectorIsWSL(t *testing.T) { + tests := []struct { + name string + goos string + files map[string]string + exists []string + env map[string]string + want bool + // wantSignal, when set, pins which signal wslSignal reports. It is what + // locks the documented precedence order; asserting only the boolean would + // let the order silently change. + wantSignal string + }{ + { + // Every signal present at once: /run/WSL must win. + name: "precedence: /run/WSL beats everything", + goos: "linux", + files: map[string]string{ + procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2", + procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2", + }, + exists: []string{runWSLPath, wslInteropPath}, + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04", "WSL_INTEROP": "/run/WSL/424_interop"}, + want: true, + wantSignal: runWSLPath, + }, + { + name: "precedence: WSLInterop beats the strings and env", + goos: "linux", + files: map[string]string{ + procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2", + procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2", + }, + exists: []string{wslInteropPath}, + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, + want: true, + wantSignal: wslInteropPath, + }, + { + name: "precedence: osrelease beats /proc/version and env", + goos: "linux", + files: map[string]string{ + procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2", + procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2", + }, + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, + want: true, + wantSignal: procOSReleasePath, + }, + { + name: "precedence: /proc/version beats env", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2"}, + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, + want: true, + wantSignal: procVersionPath, + }, + { + name: "precedence: WSL_DISTRO_NAME beats WSL_INTEROP", + goos: "linux", + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04", "WSL_INTEROP": "/run/WSL/424_interop"}, + want: true, + wantSignal: "WSL_DISTRO_NAME", + }, + { + // Verbatim string from the WSL2 host that exposed the bug. It carries + // both tokens, so it proves the end-to-end regression is caught but not + // which token did it — the isolated cases below do that. + name: "real WSL2 /proc/version (the regression)", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2 (root@builder) #1 SMP"}, + want: true, + }, + { + // The only signal that survives a custom kernel under sudo, a systemd + // unit or cron: no WSL_* env vars, no WSL strings in the kernel banner. + name: "/run/WSL marker alone, custom kernel with no WSL strings", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 6.6.0-custom (root@buildhost)", procOSReleasePath: "6.6.0-custom"}, + exists: []string{runWSLPath}, + want: true, + }, + { + name: "WSLInterop binfmt marker alone", + goos: "linux", + exists: []string{wslInteropPath}, + want: true, + }, + { + // Interop can be disabled in wsl.conf, so WSLInterop may be absent on a + // genuine WSL system; /run/WSL must still carry it. + name: "interop disabled: /run/WSL present, WSLInterop absent", + goos: "linux", + exists: []string{runWSLPath}, + want: true, + }, + { + name: "/proc/version: lowercase microsoft only", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard (root@builder)"}, + want: true, + }, + { + name: "/proc/version: uppercase Microsoft only (WSL1)", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 4.4.0-19041-Microsoft (Microsoft@Microsoft.com)"}, + want: true, + }, + { + // /proc/version is free-form: the build user, build host and compiler + // banner all appear in it, so a bare "wsl" token there is a false + // positive waiting to happen. osrelease is where "wsl" is matched. + name: "/proc/version: wsl only in the build host is NOT a signal", + goos: "linux", + files: map[string]string{ + procVersionPath: "Linux version 6.8.0-51-generic (builder@wsl-builder) (gcc (GCC) 13.2.0, GNU ld (GNU Binutils) 2.41)", + procOSReleasePath: "6.8.0-51-generic", + }, + want: false, + }, + { + name: "/proc/version: wsl token alone does not match", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 5.15.0-WSL-custom"}, + want: false, + }, + { + name: "osrelease only: microsoft token", + goos: "linux", + files: map[string]string{procOSReleasePath: "5.15.0-microsoft-standard"}, + want: true, + }, + { + name: "osrelease only: wsl token, no microsoft, no WSL2", + goos: "linux", + files: map[string]string{procOSReleasePath: "5.15.0-WSL-custom"}, + want: true, + }, + { + name: "osrelease only: uppercase WSL2 marker", + goos: "linux", + files: map[string]string{procOSReleasePath: "5.15.0-generic-WSL2"}, + want: true, + }, + { + name: "WSL_DISTRO_NAME only", + goos: "linux", + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, + want: true, + }, + { + name: "WSL_INTEROP only", + goos: "linux", + env: map[string]string{"WSL_INTEROP": "/run/WSL/424_interop"}, + want: true, + }, + { + name: "empty WSL_DISTRO_NAME is not a signal", + goos: "linux", + env: map[string]string{"WSL_DISTRO_NAME": ""}, + want: false, + }, + { + name: "plain linux", + goos: "linux", + files: map[string]string{ + procVersionPath: "Linux version 6.8.0-51-generic (buildd@lcy02)", + procOSReleasePath: "6.8.0-51-generic", + }, + want: false, + }, + { + name: "both proc files unreadable", + goos: "linux", + want: false, + }, + { + name: "non-linux GOOS never touches the filesystem or environment", + goos: "windows", + files: map[string]string{procVersionPath: "microsoft"}, + exists: []string{runWSLPath}, + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + accessed := false + d := Detector{ + ReadFile: func(name string) ([]byte, error) { + accessed = true + return fakeFS(tt.files)(name) + }, + Exists: func(path string) bool { + accessed = true + return fakeExists(tt.exists...)(path) + }, + LookupEnv: func(key string) (string, bool) { + accessed = true + return fakeEnv(tt.env)(key) + }, + GOOS: tt.goos, + } + if got := d.IsWSL(); got != tt.want { + t.Errorf("IsWSL() = %v, want %v", got, tt.want) + } + if signal, ok := d.wslSignal(); tt.wantSignal != "" && (!ok || signal != tt.wantSignal) { + t.Errorf("wslSignal() = %q (found=%v), want %q — precedence order changed", signal, ok, tt.wantSignal) + } + if tt.goos != "linux" && accessed { + t.Error("IsWSL() read the filesystem or environment on non-linux GOOS; expected short-circuit") + } + }) + } +} + +func TestDetectorApply(t *testing.T) { + wslFiles := map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2"} + plainFiles := map[string]string{procVersionPath: "Linux version 6.8.0-51-generic"} + + tests := []struct { + name string + goos string + files map[string]string + env map[string]string + setenvErr error + wantApplied bool + wantErr bool + wantSetenv bool + reasonHas string + }{ + { + name: "not linux is a no-op", + goos: "darwin", + wantSetenv: false, + reasonHas: "not linux", + }, + { + name: "explicit =1 is preserved", + goos: "linux", + files: wslFiles, + env: map[string]string{envVar: "1"}, + wantSetenv: false, + reasonHas: "already set", + }, + { + name: "explicit =0 is preserved (any non-empty value forces basic keyring)", + goos: "linux", + files: wslFiles, + env: map[string]string{envVar: "0"}, + wantSetenv: false, + reasonHas: "already set", + }, + { + name: "explicit =false is preserved", + goos: "linux", + files: wslFiles, + env: map[string]string{envVar: "false"}, + wantSetenv: false, + reasonHas: "already set", + }, + { + name: "explicitly empty value on WSL is overwritten", + goos: "linux", + files: wslFiles, + env: map[string]string{envVar: ""}, + wantApplied: true, + wantSetenv: true, + reasonHas: "WSL detected", + }, + { + name: "unset on WSL applies the override", + goos: "linux", + files: wslFiles, + wantApplied: true, + wantSetenv: true, + reasonHas: "WSL detected", + }, + { + name: "unset on plain linux does not apply", + goos: "linux", + files: plainFiles, + wantSetenv: false, + reasonHas: "not WSL", + }, + { + name: "explicitly empty value on plain linux is left alone", + goos: "linux", + files: plainFiles, + env: map[string]string{envVar: ""}, + wantSetenv: false, + reasonHas: "not WSL", + }, + { + name: "setenv failure fails closed", + goos: "linux", + files: wslFiles, + setenvErr: errors.New("boom"), + wantSetenv: true, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var setKey, setVal string + setCalls := 0 + accessed := false + d := Detector{ + ReadFile: func(name string) ([]byte, error) { + accessed = true + return fakeFS(tt.files)(name) + }, + Exists: func(path string) bool { + accessed = true + return fakeExists()(path) + }, + LookupEnv: func(key string) (string, bool) { + accessed = true + return fakeEnv(tt.env)(key) + }, + Setenv: func(k, v string) error { + accessed = true + setCalls++ + setKey, setVal = k, v + return tt.setenvErr + }, + GOOS: tt.goos, + } + + applied, reason, err := d.Apply() + + if (err != nil) != tt.wantErr { + t.Fatalf("Apply() error = %v, wantErr %v", err, tt.wantErr) + } + if applied != tt.wantApplied { + t.Errorf("Apply() applied = %v, want %v", applied, tt.wantApplied) + } + if tt.wantSetenv && setCalls != 1 { + t.Errorf("Setenv called %d times, want 1", setCalls) + } + if !tt.wantSetenv && setCalls != 0 { + t.Errorf("Setenv called %d times, want 0", setCalls) + } + if tt.wantSetenv { + if setKey != envVar || setVal != "1" { + t.Errorf("Setenv(%q, %q), want (%q, %q)", setKey, setVal, envVar, "1") + } + } + if tt.goos != "linux" && accessed { + t.Error("Apply() read the filesystem or environment, or wrote an env var, on non-linux GOOS; expected short-circuit") + } + if tt.reasonHas != "" && !strings.Contains(reason, tt.reasonHas) { + t.Errorf("reason = %q, want it to contain %q", reason, tt.reasonHas) + } + }) + } +} + +func TestPackageApplyUsesRealDefaults(t *testing.T) { + // Sanity: the package-level Apply must be wired to real os functions and + // must not panic or error on this host. + t.Setenv(envVar, "1") + applied, reason, err := Apply() + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + if applied { + t.Error("Apply() applied = true, want false when the env var is already set") + } + // On linux the env-var check wins; on other platforms the GOOS guard does. + if !strings.Contains(reason, "already set") && !strings.Contains(reason, "not linux") { + t.Errorf("reason = %q, want it to mention the var is already set or a non-linux host", reason) + } +}