From 7dd807086205e6d2ad95ff95266810017fdf2411 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:33:43 +0200 Subject: [PATCH 1/7] fix: force file-based keyring on WSL to stop grant login hanging The idsec SDK detects WSL by matching "Microsoft" case-sensitively against /proc/version. Modern WSL2 kernels report "-microsoft-standard-WSL2", so detection fails; WSLg sets DBUS_SESSION_BUS_ADDRESS, so GetKeyring returns the OS-provided D-Bus keyring, which can block indefinitely against an unresponsive gnome-keyring-daemon. The SDK's fallbacks only trigger on a returned error, and a hang is never an error, so it is unrecoverable. Add internal/keyringenv, which detects WSL case-insensitively via /proc/version, /proc/sys/kernel/osrelease, WSL_DISTRO_NAME and WSL_INTEROP, and sets IDSEC_BASIC_KEYRING=1 before any keyring access. Any existing non-empty value is preserved (the SDK treats every non-empty value as "force basic"); an explicitly empty value is overwritten on WSL because empty is the setting that selects the hanging backend. Applied from Execute() ahead of rootCmd.Execute(), and fails closed: a Setenv error aborts rather than walking the user into the hang. The diagnostic is emitted only under --verbose. --- CHANGELOG.md | 4 + CLAUDE.md | 10 + README.md | 2 + cmd/keyring_override_test.go | 156 +++++++++++++++ cmd/root.go | 31 ++- internal/keyringenv/keyringenv.go | 116 +++++++++++ internal/keyringenv/keyringenv_test.go | 267 +++++++++++++++++++++++++ 7 files changed, 585 insertions(+), 1 deletion(-) create mode 100644 cmd/keyring_override_test.go create mode 100644 internal/keyringenv/keyringenv.go create mode 100644 internal/keyringenv/keyringenv_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index de699d0..35a469d 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` (and any other command that touches the token cache) no longer hangs forever on WSL2. The SDK detects WSL by matching `Microsoft` case-sensitively against `/proc/version`; modern WSL2 kernels report `-microsoft-standard-WSL2`, so detection failed and — because WSLg sets `DBUS_SESSION_BUS_ADDRESS` — the OS-provided D-Bus keyring was selected, where the call can block indefinitely. The SDK's fallbacks only fire on a returned *error*, so a hang was unrecoverable. grant now detects WSL itself (case-insensitive `/proc/version` and `/proc/sys/kernel/osrelease`, plus `WSL_DISTRO_NAME`/`WSL_INTEROP`) and sets `IDSEC_BASIC_KEYRING=1` before any keyring access. An existing non-empty `IDSEC_BASIC_KEYRING` is always preserved. Note that switching backends makes a token previously stored in the OS keyring invisible; worst case, re-run `grant login` + ### 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..20730da 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -157,6 +157,16 @@ 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. Only unset/empty selects the OS keyring +- **grant's override:** `internal/keyringenv` detects WSL (case-insensitive `/proc/version` + `/proc/sys/kernel/osrelease`, plus non-empty `WSL_DISTRO_NAME`/`WSL_INTEROP`; no-op off linux) 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 +- 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..8fa1f39 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 enables it — including `0` and `false`.** Only unset (or empty) selects the OS keyring. 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..37f7b48 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,15 @@ 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. +var keyringEnvNotice string + // elevateFlags holds the command-line flags for elevation type elevateFlags struct { provider string @@ -96,6 +106,9 @@ Examples: passedArgValidation = true if verbose { sdkconfig.EnableVerboseLogging("INFO") + if keyringEnvNotice != "" { + log.Info("%s", keyringEnvNotice) + } } else { sdkconfig.DisableVerboseLogging() } @@ -252,9 +265,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..739294f --- /dev/null +++ b/internal/keyringenv/keyringenv.go @@ -0,0 +1,116 @@ +// 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" +) + +// 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) + 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, + 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. +func (d Detector) IsWSL() bool { + _, ok := d.wslSignal() + return ok +} + +// wslSignal returns the first WSL indicator found, and whether one was found. +func (d Detector) wslSignal() (string, bool) { + if d.GOOS != "linux" { + return "", false + } + for _, f := range []struct { + path string + tokens []string + }{ + {procVersionPath, []string{"microsoft", "wsl"}}, + {procOSReleasePath, []string{"microsoft", "wsl2"}}, + } { + 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..4046359 --- /dev/null +++ b/internal/keyringenv/keyringenv_test.go @@ -0,0 +1,267 @@ +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 + } +} + +// 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 + env map[string]string + want bool + }{ + { + name: "lowercase microsoft in /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, + }, + { + name: "uppercase Microsoft in /proc/version (WSL1)", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 4.4.0-19041-Microsoft (Microsoft@Microsoft.com)"}, + want: true, + }, + { + name: "wsl token in /proc/version only", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 5.15.0-WSL2-custom"}, + want: true, + }, + { + name: "osrelease only", + goos: "linux", + files: map[string]string{procOSReleasePath: "5.15.0-microsoft-standard-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 reads /proc", + goos: "windows", + files: map[string]string{procVersionPath: "microsoft"}, + env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + readCalled := false + d := Detector{ + ReadFile: func(name string) ([]byte, error) { + readCalled = true + return fakeFS(tt.files)(name) + }, + LookupEnv: fakeEnv(tt.env), + GOOS: tt.goos, + } + if got := d.IsWSL(); got != tt.want { + t.Errorf("IsWSL() = %v, want %v", got, tt.want) + } + if tt.goos != "linux" && readCalled { + t.Error("IsWSL() read /proc 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 + d := Detector{ + ReadFile: fakeFS(tt.files), + LookupEnv: fakeEnv(tt.env), + Setenv: func(k, v string) error { + 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.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) + } +} From c8bade79f0cbe1c929b4ce78c4427cefc49cef86 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:40:47 +0200 Subject: [PATCH 2/7] fix: check both /proc sources for the generic wsl marker /proc/sys/kernel/osrelease was matched against "microsoft" and "wsl2", so an osrelease carrying only a "WSL" marker was missed when no other signal was available. Both files now check both tokens. The osrelease fixture contained microsoft and WSL2 together and so could not prove either token on its own; the /proc/version fixtures had the same overlap. Split into single-token cases, keeping the verbatim host string as an explicit end-to-end regression case. Also correct the README/CLAUDE.md claim that only unset/empty selects the OS keyring (Docker, SDK-detected WSL and enforceBasic still force file storage), condense the CHANGELOG entry to one line per the new convention, and note that keyringEnvNotice is set but never cleared. --- CHANGELOG.md | 2 +- CLAUDE.md | 2 +- README.md | 2 +- cmd/root.go | 3 +++ internal/keyringenv/keyringenv.go | 17 ++++++------- internal/keyringenv/keyringenv_test.go | 33 +++++++++++++++++++++----- 6 files changed, 40 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35a469d..88372b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Fixed -- `grant login` (and any other command that touches the token cache) no longer hangs forever on WSL2. The SDK detects WSL by matching `Microsoft` case-sensitively against `/proc/version`; modern WSL2 kernels report `-microsoft-standard-WSL2`, so detection failed and — because WSLg sets `DBUS_SESSION_BUS_ADDRESS` — the OS-provided D-Bus keyring was selected, where the call can block indefinitely. The SDK's fallbacks only fire on a returned *error*, so a hang was unrecoverable. grant now detects WSL itself (case-insensitive `/proc/version` and `/proc/sys/kernel/osrelease`, plus `WSL_DISTRO_NAME`/`WSL_INTEROP`) and sets `IDSEC_BASIC_KEYRING=1` before any keyring access. An existing non-empty `IDSEC_BASIC_KEYRING` is always preserved. Note that switching backends makes a token previously stored in the OS keyring invisible; worst case, re-run `grant login` +- `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 diff --git a/CLAUDE.md b/CLAUDE.md index 20730da..150fe54 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -161,7 +161,7 @@ Custom `SCAAccessService` follows SDK conventions: - 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. Only unset/empty selects the OS keyring +- **`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 (case-insensitive `/proc/version` + `/proc/sys/kernel/osrelease`, plus non-empty `WSL_DISTRO_NAME`/`WSL_INTEROP`; no-op off linux) 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 - 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) diff --git a/README.md b/README.md index 8fa1f39..cff5dd7 100644 --- a/README.md +++ b/README.md @@ -174,7 +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 enables it — including `0` and `false`.** Only unset (or empty) selects the OS keyring. 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) | +| `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 diff --git a/cmd/root.go b/cmd/root.go index 37f7b48..729903f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -47,6 +47,9 @@ 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 diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go index 739294f..b103273 100644 --- a/internal/keyringenv/keyringenv.go +++ b/internal/keyringenv/keyringenv.go @@ -57,21 +57,18 @@ func (d Detector) wslSignal() (string, bool) { if d.GOOS != "linux" { return "", false } - for _, f := range []struct { - path string - tokens []string - }{ - {procVersionPath, []string{"microsoft", "wsl"}}, - {procOSReleasePath, []string{"microsoft", "wsl2"}}, - } { - data, err := d.ReadFile(f.path) + // Both files are checked for both tokens: "microsoft" catches the standard + // kernel strings, "wsl" catches custom kernels that only carry the WSL/WSL2 + // marker. Matching is case-insensitive — that is the SDK's actual bug. + for _, path := range []string{procVersionPath, procOSReleasePath} { + data, err := d.ReadFile(path) if err != nil { continue } lower := strings.ToLower(string(data)) - for _, token := range f.tokens { + for _, token := range []string{"microsoft", "wsl"} { if strings.Contains(lower, token) { - return f.path, true + return path, true } } } diff --git a/internal/keyringenv/keyringenv_test.go b/internal/keyringenv/keyringenv_test.go index 4046359..e4e6375 100644 --- a/internal/keyringenv/keyringenv_test.go +++ b/internal/keyringenv/keyringenv_test.go @@ -36,27 +36,48 @@ func TestDetectorIsWSL(t *testing.T) { want bool }{ { - name: "lowercase microsoft in /proc/version (the regression)", + // 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, }, { - name: "uppercase Microsoft in /proc/version (WSL1)", + 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, }, { - name: "wsl token in /proc/version only", + name: "/proc/version: wsl token only", + goos: "linux", + files: map[string]string{procVersionPath: "Linux version 5.15.0-WSL-custom"}, + want: true, + }, + { + 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{procVersionPath: "Linux version 5.15.0-WSL2-custom"}, + files: map[string]string{procOSReleasePath: "5.15.0-WSL-custom"}, want: true, }, { - name: "osrelease only", + name: "osrelease only: uppercase WSL2 marker", goos: "linux", - files: map[string]string{procOSReleasePath: "5.15.0-microsoft-standard-WSL2"}, + files: map[string]string{procOSReleasePath: "5.15.0-generic-WSL2"}, want: true, }, { From 9d4e4b092ff1137a9cbc94161af862235746b7a6 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:49:56 +0200 Subject: [PATCH 3/7] fix: detect WSL via /run/WSL marker, drop bare wsl token from /proc/version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add filesystem markers, checked first: /run/WSL, plus /proc/sys/fs/binfmt_misc/WSLInterop as a supplement (that binfmt entry exists only when interop is enabled in wsl.conf, so it cannot stand alone). /run/WSL is the only signal that 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 a custom WSL2 kernel may carry neither "microsoft" nor "wsl" (#6911). Until now there was no signal at all for a systemd unit on such a kernel. snapd moved to this marker for the same reason (Launchpad #1991823). Stop matching a bare "wsl" in /proc/version. That string is free-form and includes the kernel build user, build host and compiler banner, so a plain Linux box built on a host named "wsl-builder" would match — a false positive we introduced ourselves. Neither systemd nor npm is-wsl matches it there. /proc/version now checks "microsoft" only; osrelease, which is short and structured, keeps both tokens. Detector gains an injected Exists func. Correct the doc comment, which credited the "wsl" token with covering custom kernels, and record the error asymmetry that justifies biasing toward over-detection: a false negative hangs forever with no error and no timeout, a false positive is a bounded security downgrade. --- CLAUDE.md | 7 ++- internal/keyringenv/keyringenv.go | 64 ++++++++++++++++--- internal/keyringenv/keyringenv_test.go | 86 +++++++++++++++++++++----- 3 files changed, 132 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 150fe54..309acd6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,7 +162,12 @@ Custom `SCAAccessService` follows SDK conventions: - **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 (case-insensitive `/proc/version` + `/proc/sys/kernel/osrelease`, plus non-empty `WSL_DISTRO_NAME`/`WSL_INTEROP`; no-op off linux) 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 +- **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 (this is what systemd and npm `is-wsl` do). `/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 load-bearing signal**, not the string matching: it is the only one that 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. `WSLInterop` is a supplement only — that binfmt entry exists only when interop is enabled in `wsl.conf` +- **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 — but via `/run/WSL`, which is broader *and* more specific, never via looser string matching +- Rejected: container gating (inside a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks basic; `/run/WSL` is container-safe anyway since a container gets its own `/run` tmpfs), `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` diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go index b103273..e171748 100644 --- a/internal/keyringenv/keyringenv.go +++ b/internal/keyringenv/keyringenv.go @@ -24,12 +24,26 @@ const ( 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 exists only when Windows interop is enabled in wsl.conf, + // so it is a supplement to runWSLPath, never a replacement — that + // insufficiency is what Launchpad #1991823 documents. + 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 @@ -38,7 +52,11 @@ type Detector struct { // New returns a Detector wired to the real OS. func New() Detector { return Detector{ - ReadFile: os.ReadFile, + ReadFile: os.ReadFile, + Exists: func(path string) bool { + _, err := os.Stat(path) + return err == nil + }, LookupEnv: os.LookupEnv, Setenv: os.Setenv, GOOS: runtime.GOOS, @@ -47,6 +65,18 @@ func New() Detector { // 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 @@ -57,18 +87,36 @@ func (d Detector) wslSignal() (string, bool) { if d.GOOS != "linux" { return "", false } - // Both files are checked for both tokens: "microsoft" catches the standard - // kernel strings, "wsl" catches custom kernels that only carry the WSL/WSL2 - // marker. Matching is case-insensitive — that is the SDK's actual bug. - for _, path := range []string{procVersionPath, procOSReleasePath} { - data, err := d.ReadFile(path) + // 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. This is what systemd and npm's is-wsl do. + // - /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 []string{"microsoft", "wsl"} { + for _, token := range f.tokens { if strings.Contains(lower, token) { - return path, true + return f.path, true } } } diff --git a/internal/keyringenv/keyringenv_test.go b/internal/keyringenv/keyringenv_test.go index e4e6375..21a0273 100644 --- a/internal/keyringenv/keyringenv_test.go +++ b/internal/keyringenv/keyringenv_test.go @@ -18,6 +18,18 @@ func fakeFS(files map[string]string) func(string) ([]byte, error) { } } +// 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) { @@ -29,11 +41,12 @@ func fakeEnv(env map[string]string) func(string) (string, bool) { func TestDetectorIsWSL(t *testing.T) { tests := []struct { - name string - goos string - files map[string]string - env map[string]string - want bool + name string + goos string + files map[string]string + exists []string + env map[string]string + want bool }{ { // Verbatim string from the WSL2 host that exposed the bug. It carries @@ -44,6 +57,29 @@ func TestDetectorIsWSL(t *testing.T) { 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", @@ -57,10 +93,22 @@ func TestDetectorIsWSL(t *testing.T) { want: true, }, { - name: "/proc/version: wsl token only", + // /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: true, + want: false, }, { name: "osrelease only: microsoft token", @@ -113,30 +161,35 @@ func TestDetectorIsWSL(t *testing.T) { want: false, }, { - name: "non-linux GOOS never reads /proc", - goos: "windows", - files: map[string]string{procVersionPath: "microsoft"}, - env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, - want: false, + name: "non-linux GOOS never touches the filesystem", + 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) { - readCalled := false + fsTouched := false d := Detector{ ReadFile: func(name string) ([]byte, error) { - readCalled = true + fsTouched = true return fakeFS(tt.files)(name) }, + Exists: func(path string) bool { + fsTouched = true + return fakeExists(tt.exists...)(path) + }, LookupEnv: fakeEnv(tt.env), GOOS: tt.goos, } if got := d.IsWSL(); got != tt.want { t.Errorf("IsWSL() = %v, want %v", got, tt.want) } - if tt.goos != "linux" && readCalled { - t.Error("IsWSL() read /proc on non-linux GOOS; expected short-circuit") + if tt.goos != "linux" && fsTouched { + t.Error("IsWSL() touched the filesystem on non-linux GOOS; expected short-circuit") } }) } @@ -235,6 +288,7 @@ func TestDetectorApply(t *testing.T) { setCalls := 0 d := Detector{ ReadFile: fakeFS(tt.files), + Exists: fakeExists(), LookupEnv: fakeEnv(tt.env), Setenv: func(k, v string) error { setCalls++ From 83166047d9064e238fdd5c07214c0f289379df09 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:58:09 +0200 Subject: [PATCH 4/7] test: pin WSL signal precedence and correct the is-wsl attribution The tests asserted only booleans, so the documented signal order could have been reordered silently. Add a wantSignal field and five cases with multiple signals present at once, each pinning which one wins. Verified by mutation: swapping the marker order or the two string sources fails exactly the affected precedence cases. The non-linux short-circuit probe now also trips on LookupEnv, and the Apply table gained the same probe across ReadFile, Exists, LookupEnv and Setenv, so "no access at all off linux" is fully asserted rather than filesystem-only. Correct the npm is-wsl attribution: it does not match "wsl" on osrelease. It matches "microsoft" only, on os.release() then /proc/version, then falls back to WSLInterop/run/WSL existence, all gated behind !isInsideContainer(). systemd is the precedent for the osrelease "wsl" token; is-wsl is a precedent for the markers. --- CLAUDE.md | 2 +- internal/keyringenv/keyringenv.go | 5 +- internal/keyringenv/keyringenv_test.go | 97 +++++++++++++++++++++++--- 3 files changed, 91 insertions(+), 13 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 309acd6..e26bf2c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -164,7 +164,7 @@ Custom `SCAAccessService` follows SDK conventions: - **`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 (this is what systemd and npm `is-wsl` do). `/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 +- **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 load-bearing signal**, not the string matching: it is the only one that 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. `WSLInterop` is a supplement only — that binfmt entry exists only when interop is enabled in `wsl.conf` - **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 — but via `/run/WSL`, which is broader *and* more specific, never via looser string matching - Rejected: container gating (inside a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks basic; `/run/WSL` is container-safe anyway since a container gets its own `/run` tmpfs), `WSLENV` (user-configurable, often absent), shelling out to `systemd-detect-virt`, and third-party libs (`gookit/goutil` has the same case-sensitive `"Microsoft"` bug) diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go index e171748..6047234 100644 --- a/internal/keyringenv/keyringenv.go +++ b/internal/keyringenv/keyringenv.go @@ -97,7 +97,10 @@ func (d Detector) wslSignal() (string, bool) { // 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. This is what systemd and npm's is-wsl do. + // 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" diff --git a/internal/keyringenv/keyringenv_test.go b/internal/keyringenv/keyringenv_test.go index 21a0273..651b1fa 100644 --- a/internal/keyringenv/keyringenv_test.go +++ b/internal/keyringenv/keyringenv_test.go @@ -47,7 +47,62 @@ func TestDetectorIsWSL(t *testing.T) { 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 @@ -161,7 +216,7 @@ func TestDetectorIsWSL(t *testing.T) { want: false, }, { - name: "non-linux GOOS never touches the filesystem", + name: "non-linux GOOS never touches the filesystem or environment", goos: "windows", files: map[string]string{procVersionPath: "microsoft"}, exists: []string{runWSLPath}, @@ -172,24 +227,30 @@ func TestDetectorIsWSL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - fsTouched := false + accessed := false d := Detector{ ReadFile: func(name string) ([]byte, error) { - fsTouched = true + accessed = true return fakeFS(tt.files)(name) }, Exists: func(path string) bool { - fsTouched = true + accessed = true return fakeExists(tt.exists...)(path) }, - LookupEnv: fakeEnv(tt.env), - GOOS: tt.goos, + 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 tt.goos != "linux" && fsTouched { - t.Error("IsWSL() touched the filesystem on non-linux GOOS; expected short-circuit") + 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") } }) } @@ -286,11 +347,22 @@ func TestDetectorApply(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var setKey, setVal string setCalls := 0 + accessed := false d := Detector{ - ReadFile: fakeFS(tt.files), - Exists: fakeExists(), - LookupEnv: fakeEnv(tt.env), + 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 @@ -317,6 +389,9 @@ func TestDetectorApply(t *testing.T) { 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) } From db73645a38f85758eef8fb24628de658d94795e5 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 08:14:04 +0200 Subject: [PATCH 5/7] refactor: drop the two redundant WSL detection signals The detector checked five signals; two of them could never independently decide anything. Both are removed, leaving /run/WSL -> WSLInterop -> osrelease. No user-visible behaviour change, so the CHANGELOG entry stands. /proc/version is redundant with /proc/sys/kernel/osrelease by kernel construction. fs/proc/version.c formats the file as seq_printf(m, linux_proc_banner, utsname()->sysname, utsname()->release, utsname()->version); and /proc/sys/kernel/osrelease *is* utsname()->release -- kernel/utsname_sysctl.c registers uts_kern_table entry "osrelease" over init_uts_ns.name.release, resolved against the caller's UTS namespace by get_uts(). So "microsoft" in the release component cannot match unless osrelease matches too. The file's other substrings are LINUX_COMPILE_BY, LINUX_COMPILE_HOST and LINUX_COMPILER (init/version.c) -- build metadata, where a match is a false positive rather than extra coverage. WSL1 loses nothing: it exposes osrelease as 4.4.0-19041-Microsoft, and the WSL team named both files together in microsoft/WSL#423. WSL_DISTRO_NAME / WSL_INTEROP are redundant with /run/WSL. WSL's init setenv's WSL_DISTRO_NAME in ConfigInitializeInstance() (src/linux/init/config.cpp), the same function that ~100 lines later, with no conditional in between, creates /run/WSL via InteropServer::Create() under FATAL_ERROR -- a distro that cannot create the marker does not boot. The var therefore cannot be set in a process whose distro lacks the marker. WSL_INTEROP is narrower still, exported only when interop is enabled. Both also point the wrong way for the error asymmetry, being absent under sudo -i (#5914), in systemd units (#9719), in cron and over ssh (#12647), exactly where /run/WSL keeps working. snapd and npm is-wsl both check the markers and decline the env vars. Detector.LookupEnv stays: Apply() still needs it for the IDSEC_BASIC_KEYRING precedence check. Tests: the precedence regression-lock is preserved for the three remaining signals. The /proc/version-only and env-var-only cases are replaced by TestProcVersionIsNeverRead and TestWSLSignalReadsNoEnvVars, which assert the removed sources are not consulted at all, so a future re-add fails loudly. The former /proc/version false-positive case ("wsl" in the build host) survives as a microsoft-builder fixture. --- CLAUDE.md | 11 +- internal/keyringenv/keyringenv.go | 93 ++++++----- internal/keyringenv/keyringenv_test.go | 212 +++++++++++++------------ 3 files changed, 175 insertions(+), 141 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e26bf2c..b87a77d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,9 +163,14 @@ Custom `SCAAccessService` follows SDK conventions: - **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 load-bearing signal**, not the string matching: it is the only one that 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. `WSLInterop` is a supplement only — that binfmt entry exists only when interop is enabled in `wsl.conf` +- 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`. String matching is case-insensitive +- **Why the token set:** `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 a precedent for the filesystem markers, not for the `wsl` token +- **`/run/WSL` is the load-bearing signal**, not the string matching: it is the only one that survives custom kernels, `sudo`, systemd units and cron. WSL's init creates it in `InteropServer::Create()` (`src/linux/init/util.cpp`, `WSL_TEMP_FOLDER = RUN_FOLDER "/WSL"`), called **unconditionally** from `ConfigInitializeInstance()` (`src/linux/init/config.cpp`) under `FATAL_ERROR` — a distro that cannot create it does not boot. Not gated on the `wsl.conf` interop setting, not gated on WSL1-vs-WSL2, kernel-independent. snapd abandoned string matching for this marker after Launchpad #1991823 +- **`WSLInterop` is a supplement only:** on WSL1 the per-distro registration is gated on `Config.InteropEnabled` (`src/linux/init/config.cpp`), and on WSL2 the VM-level entry is kernel-global, so systemd-binfmt can shadow it under a different name (microsoft/WSL#13449) +- **Two signals were removed as redundant (do not re-add).** Redundant means every configuration they detect is already detected by a signal that remains — not merely that they are weaker: + - **`/proc/version`** is redundant with `osrelease` *by kernel construction*: `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()`). So `microsoft` in `/proc/version`'s release component cannot match unless `osrelease` matches too. The rest of the file is `LINUX_COMPILE_BY`/`LINUX_COMPILE_HOST`/`LINUX_COMPILER` (`init/version.c`) — build metadata, where a match is a **false positive**, not coverage. WSL1 is covered too: it exposes `osrelease` as `4.4.0-19041-Microsoft`, and the WSL team named both files together in microsoft/WSL#423. Locked by `TestProcVersionIsNeverRead` + - **`WSL_DISTRO_NAME` / `WSL_INTEROP`** are redundant with `/run/WSL`: init `setenv`s `WSL_DISTRO_NAME` in `ConfigInitializeInstance()`, the same function that ~100 lines later, with no conditional in between, creates `/run/WSL` under `FATAL_ERROR`. The var therefore cannot be set in a process whose distro did not create the marker. `WSL_INTEROP` is narrower still — exported only when interop is enabled (`src/linux/init/init.cpp`). They also point the wrong way for the error asymmetry, being absent under `sudo -i` (microsoft/WSL#5914), in systemd units (#9719), in cron and over SSH (#12647), exactly where `/run/WSL` keeps working. snapd and npm `is-wsl` both decline to check them. Locked by `TestWSLSignalReadsNoEnvVars`. Note `Detector.LookupEnv` stays — `Apply()` needs it for the `IDSEC_BASIC_KEYRING` precedence check + - Residual theoretical gap, accepted: a chroot/private mount namespace with a fresh `/run` and no `binfmt_misc`, *combined with* a custom kernel whose `osrelease` was renamed, would inherit the env vars and detect nothing. No report of anyone hitting this exists, and in a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks the basic keyring there - **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 — but via `/run/WSL`, which is broader *and* more specific, never via looser string matching - Rejected: container gating (inside a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks basic; `/run/WSL` is container-safe anyway since a container gets its own `/run` tmpfs), `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 diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go index 6047234..d3e81cc 100644 --- a/internal/keyringenv/keyringenv.go +++ b/internal/keyringenv/keyringenv.go @@ -22,20 +22,27 @@ const ( // os.Getenv(envVar) != "", so ANY non-empty value forces the basic keyring. envVar = "IDSEC_BASIC_KEYRING" - procVersionPath = "/proc/version" + // procOSReleasePath exposes utsname()->release. /proc/version is NOT read: + // the kernel builds it from the very same field, so it can never be the + // deciding signal. See the note on wslSignal. 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 is the load-bearing marker. WSL's init creates it in + // InteropServer::Create() (src/linux/init/util.cpp, WSL_TEMP_FOLDER = + // RUN_FOLDER "/WSL"), called unconditionally from ConfigInitializeInstance() + // (src/linux/init/config.cpp) under FATAL_ERROR — a distro that cannot + // create it does not boot. Not gated on the wsl.conf interop setting, not + // gated on WSL1-vs-WSL2, and independent of the kernel, so it survives + // custom kernels (microsoft/WSL#6911), sudo, systemd units and cron. snapd + // moved to this marker after Launchpad #1991823; npm's is-wsl uses it too. runWSLPath = "/run/WSL" - // wslInteropPath exists only when Windows interop is enabled in wsl.conf, - // so it is a supplement to runWSLPath, never a replacement — that - // insufficiency is what Launchpad #1991823 documents. + // wslInteropPath is a supplement to runWSLPath, never a replacement: on + // WSL1 the per-distro registration is gated on Config.InteropEnabled + // (src/linux/init/config.cpp), and on WSL2 the VM-level entry is + // kernel-global, so systemd-binfmt can shadow it under a different name + // (microsoft/WSL#13449). That insufficiency is what Launchpad #1991823 + // documents. wslInteropPath = "/proc/sys/fs/binfmt_misc/WSLInterop" ) @@ -83,6 +90,22 @@ func (d Detector) IsWSL() bool { } // wslSignal returns the first WSL indicator found, and whether one was found. +// +// Three signals, deliberately. Two more were considered and removed as +// redundant — redundant meaning every configuration they detect is already +// detected by one of the three, not merely that they are weaker: +// +// - /proc/version. See the note below. +// - WSL_DISTRO_NAME / WSL_INTEROP. WSL's init sets WSL_DISTRO_NAME in +// ConfigInitializeInstance() (src/linux/init/config.cpp), the same function +// that, ~100 lines later and with no conditional between them, creates +// /run/WSL under FATAL_ERROR. So WSL_DISTRO_NAME cannot be set in a process +// whose distro did not create /run/WSL — the var is strictly narrower. +// WSL_INTEROP is narrower still: it is exported only when interop is +// enabled (src/linux/init/init.cpp). They also point the wrong way for our +// error asymmetry, being absent under `sudo -i` (microsoft/WSL#5914), in +// systemd units (#9719), in cron and over SSH, exactly where /run/WSL keeps +// working. snapd and npm's is-wsl both decline to check them. func (d Detector) wslSignal() (string, bool) { if d.GOOS != "linux" { return "", false @@ -94,40 +117,32 @@ func (d Detector) wslSignal() (string, bool) { 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 - } + // Then the kernel release string, case-insensitively — the case sensitivity + // is the SDK's actual bug. osrelease is short and structured + // ("6.18.33.2-microsoft-standard-WSL2"), so matching "wsl" there is safe; + // systemd does the same (Microsoft||WSL on osrelease, src/basic/virt.c). + // + // /proc/version is deliberately NOT read. The kernel formats it as + // linux_proc_banner with utsname()->release as its second %s + // (fs/proc/version.c version_proc_show), and /proc/sys/kernel/osrelease is + // that identical field (kernel/utsname_sysctl.c, uts_kern_table entry + // "osrelease" -> init_uts_ns.name.release, resolved per-namespace by + // get_uts). So "microsoft" in /proc/version's release component can never + // match without osrelease matching too. Its remaining substrings are the + // build user, build host and compiler banner, where a match is a false + // positive, not coverage — a plain Linux box built on a host named + // "microsoft-builder" would trip it. WSL1 exposes osrelease as well + // ("4.4.0-19041-Microsoft"); the WSL team named both files together in + // microsoft/WSL#423. + data, err := d.ReadFile(procOSReleasePath) + if err == nil { lower := strings.ToLower(string(data)) - for _, token := range f.tokens { + for _, token := range []string{"microsoft", "wsl"} { if strings.Contains(lower, token) { - return f.path, true + return procOSReleasePath, true } } } - for _, key := range []string{"WSL_DISTRO_NAME", "WSL_INTEROP"} { - if v, ok := d.LookupEnv(key); ok && v != "" { - return key, true - } - } return "", false } diff --git a/internal/keyringenv/keyringenv_test.go b/internal/keyringenv/keyringenv_test.go index 651b1fa..41e255d 100644 --- a/internal/keyringenv/keyringenv_test.go +++ b/internal/keyringenv/keyringenv_test.go @@ -54,70 +54,49 @@ func TestDetectorIsWSL(t *testing.T) { }{ { // 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", - }, + name: "precedence: /run/WSL beats everything", + goos: "linux", + files: map[string]string{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", - }, + name: "precedence: WSLInterop beats osrelease", + goos: "linux", + files: map[string]string{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", + name: "precedence: osrelease is the last signal", goos: "linux", - files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2"}, + files: map[string]string{procOSReleasePath: "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", + wantSignal: procOSReleasePath, }, { - // Verbatim string from the WSL2 host that exposed the bug. It carries + // Verbatim osrelease 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)", + name: "real WSL2 osrelease (the regression)", goos: "linux", - files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2 (root@builder) #1 SMP"}, + files: map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2\n"}, 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", + // The case /run/WSL exists for. A custom kernel (microsoft/WSL#6911) + // carries neither token; interop disabled in wsl.conf drops the WSL1 + // binfmt registration and unsets WSL_INTEROP; and under sudo, a systemd + // unit or cron the WSL_* env vars are gone too. WSL's init creates + // /run/WSL regardless of all three, so it must still carry detection. + name: "/run/WSL alone: custom kernel, interop disabled, no env vars", goos: "linux", - files: map[string]string{procVersionPath: "Linux version 6.6.0-custom (root@buildhost)", procOSReleasePath: "6.6.0-custom"}, + files: map[string]string{procOSReleasePath: "6.6.0-custom"}, exists: []string{runWSLPath}, want: true, }, @@ -128,43 +107,14 @@ func TestDetectorIsWSL(t *testing.T) { 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", + // WSL1's osrelease. The SDK's bug is that it matches "Microsoft" + // case-sensitively; this locks the case-insensitive match from the + // other direction. + name: "osrelease only: uppercase Microsoft (WSL1)", goos: "linux", - files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard (root@builder)"}, + files: map[string]string{procOSReleasePath: "4.4.0-19041-Microsoft"}, 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", @@ -184,41 +134,35 @@ func TestDetectorIsWSL(t *testing.T) { want: true, }, { - name: "WSL_DISTRO_NAME only", + // The WSL_* env vars are no longer signals. WSL's init sets + // WSL_DISTRO_NAME in the same unconditional function that creates + // /run/WSL under FATAL_ERROR (src/linux/init/config.cpp), so this + // combination — env vars set, every marker absent, osrelease clean — + // is not reachable on a real WSL system. Treating it as WSL would be a + // pure false positive. + name: "WSL_* env vars alone are not a signal", 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": ""}, + env: map[string]string{ + "WSL_DISTRO_NAME": "Ubuntu-22.04", + "WSL_INTEROP": "/run/WSL/424_interop", + }, 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: "plain linux", + goos: "linux", + files: map[string]string{procOSReleasePath: "6.8.0-51-generic"}, + want: false, }, { - name: "both proc files unreadable", + name: "osrelease unreadable and nothing else present", goos: "linux", want: false, }, { name: "non-linux GOOS never touches the filesystem or environment", goos: "windows", - files: map[string]string{procVersionPath: "microsoft"}, + files: map[string]string{procOSReleasePath: "microsoft"}, exists: []string{runWSLPath}, env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, want: false, @@ -256,9 +200,79 @@ func TestDetectorIsWSL(t *testing.T) { } } +// TestProcVersionIsNeverRead locks the removal of the /proc/version signal. +// +// The kernel builds /proc/version from utsname()->release +// (fs/proc/version.c: seq_printf(m, linux_proc_banner, ..., utsname()->release, +// ...)), and /proc/sys/kernel/osrelease *is* that field +// (kernel/utsname_sysctl.c: uts_kern_table entry "osrelease" -> +// init_uts_ns.name.release). So "microsoft" in the release component of +// /proc/version can never match without osrelease matching first. Everything +// else in that file — build user, build host, compiler banner — is a false +// positive source, not extra coverage. +// +// Reading it again would reintroduce exactly that: this fixture is a plain +// Linux box whose kernel was built on a host named "microsoft-builder". +func TestProcVersionIsNeverRead(t *testing.T) { + const procVersionPath = "/proc/version" + + var readPaths []string + d := Detector{ + ReadFile: func(name string) ([]byte, error) { + readPaths = append(readPaths, name) + return fakeFS(map[string]string{ + procVersionPath: "Linux version 6.8.0-51-generic (builder@microsoft-builder) (gcc (GCC) 13.2.0)", + procOSReleasePath: "6.8.0-51-generic", + })(name) + }, + Exists: fakeExists(), + LookupEnv: fakeEnv(nil), + GOOS: "linux", + } + + if d.IsWSL() { + t.Error("IsWSL() = true on a plain Linux box; /proc/version must not be a signal") + } + for _, p := range readPaths { + if p == procVersionPath { + t.Errorf("read %s; it is redundant with %s and must not be consulted", procVersionPath, procOSReleasePath) + } + } +} + +// TestWSLSignalReadsNoEnvVars locks the removal of the WSL_DISTRO_NAME / +// WSL_INTEROP signals. +// +// WSL's init sets WSL_DISTRO_NAME in ConfigInitializeInstance() +// (src/linux/init/config.cpp), the same function that ~100 lines later, with no +// conditional in between, creates /run/WSL under FATAL_ERROR. A process can +// therefore never see WSL_DISTRO_NAME on a distro that did not create +// /run/WSL, which makes the var strictly narrower than the marker. It is also +// absent exactly where the marker still works (sudo -i, systemd units, cron, +// ssh), so re-adding it would buy nothing and only add false positives. +// +// Note this pins wslSignal specifically — Apply() still reads LookupEnv for the +// IDSEC_BASIC_KEYRING precedence check, which must keep working. +func TestWSLSignalReadsNoEnvVars(t *testing.T) { + var lookedUp []string + d := Detector{ + ReadFile: fakeFS(map[string]string{procOSReleasePath: "6.8.0-51-generic"}), + Exists: fakeExists(), + LookupEnv: func(key string) (string, bool) { lookedUp = append(lookedUp, key); return "Ubuntu-22.04", true }, + GOOS: "linux", + } + + if d.IsWSL() { + t.Error("IsWSL() = true with every marker absent; the WSL_* env vars must not be signals") + } + if len(lookedUp) != 0 { + t.Errorf("wslSignal looked up env vars %v; they are redundant with %s and must not be consulted", lookedUp, runWSLPath) + } +} + 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"} + wslFiles := map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2"} + plainFiles := map[string]string{procOSReleasePath: "6.8.0-51-generic"} tests := []struct { name string From 23ac108a8d3f753e968eb75fd08a430b768d7e28 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 08:21:04 +0200 Subject: [PATCH 6/7] Revert "refactor: drop the two redundant WSL detection signals" This reverts commit db73645a38f85758eef8fb24628de658d94795e5. Both removals failed the redundancy bar. The mistake was conflating content equivalence with signal redundancy: two sources can be guaranteed to hold the same bytes and still not be interchangeable, because they can differ in *visibility*. /proc/version. The kernel-source finding stands -- fs/proc/version.c and /proc/sys/kernel/osrelease both render utsname()->release through the caller's UTS namespace, so their contents cannot disagree. But they are separate mount-visible paths. A chroot or mount namespace can expose /proc/version while masking, omitting or replacing /proc/sys/kernel/osrelease. There the old detector succeeded and the simplified one returned false. Identical content does not help when the file is not visible. WSL_DISTRO_NAME / WSL_INTEROP. The second call site is real: init sets WSL_DISTRO_NAME (config.cpp) and then creates /run/WSL via an unguarded InteropServer::Create() in the same function, so current distro init does create the marker even with interop disabled. But that proves only that *init* created it, not that every descendant retaining the env var can still see it. Environment variables cross chroot and mount-namespace boundaries; /run/WSL does not. With a clean /run, no binfmt and a renamed osrelease, the env var is the only surviving signal. The mitigation claimed for that gap is also wrong: DBUS_SESSION_BUS_ADDRESS is not reliably absent in such contexts. WSL init injects it explicitly for systemd-backed launches (init.cpp), and ordinary chroot preserves it, so the SDK cannot be assumed to fall back to the basic keyring. Restores all five signals and the precedence order /run/WSL -> WSLInterop -> osrelease -> /proc/version -> env vars. TestWSLSignalReadsNoEnvVars goes with the revert rather than surviving it: it locked in exactly the namespace false negative described above. Given the error asymmetry -- a false negative hangs the OS keyring under WSLg with no error and no timeout, unrecoverably -- a redundant check is the cheap side of the trade. --- CLAUDE.md | 11 +- internal/keyringenv/keyringenv.go | 93 +++++------ internal/keyringenv/keyringenv_test.go | 212 ++++++++++++------------- 3 files changed, 141 insertions(+), 175 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b87a77d..e26bf2c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,14 +163,9 @@ Custom `SCAAccessService` follows SDK conventions: - **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`. String matching is case-insensitive -- **Why the token set:** `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 a precedent for the filesystem markers, not for the `wsl` token -- **`/run/WSL` is the load-bearing signal**, not the string matching: it is the only one that survives custom kernels, `sudo`, systemd units and cron. WSL's init creates it in `InteropServer::Create()` (`src/linux/init/util.cpp`, `WSL_TEMP_FOLDER = RUN_FOLDER "/WSL"`), called **unconditionally** from `ConfigInitializeInstance()` (`src/linux/init/config.cpp`) under `FATAL_ERROR` — a distro that cannot create it does not boot. Not gated on the `wsl.conf` interop setting, not gated on WSL1-vs-WSL2, kernel-independent. snapd abandoned string matching for this marker after Launchpad #1991823 -- **`WSLInterop` is a supplement only:** on WSL1 the per-distro registration is gated on `Config.InteropEnabled` (`src/linux/init/config.cpp`), and on WSL2 the VM-level entry is kernel-global, so systemd-binfmt can shadow it under a different name (microsoft/WSL#13449) -- **Two signals were removed as redundant (do not re-add).** Redundant means every configuration they detect is already detected by a signal that remains — not merely that they are weaker: - - **`/proc/version`** is redundant with `osrelease` *by kernel construction*: `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()`). So `microsoft` in `/proc/version`'s release component cannot match unless `osrelease` matches too. The rest of the file is `LINUX_COMPILE_BY`/`LINUX_COMPILE_HOST`/`LINUX_COMPILER` (`init/version.c`) — build metadata, where a match is a **false positive**, not coverage. WSL1 is covered too: it exposes `osrelease` as `4.4.0-19041-Microsoft`, and the WSL team named both files together in microsoft/WSL#423. Locked by `TestProcVersionIsNeverRead` - - **`WSL_DISTRO_NAME` / `WSL_INTEROP`** are redundant with `/run/WSL`: init `setenv`s `WSL_DISTRO_NAME` in `ConfigInitializeInstance()`, the same function that ~100 lines later, with no conditional in between, creates `/run/WSL` under `FATAL_ERROR`. The var therefore cannot be set in a process whose distro did not create the marker. `WSL_INTEROP` is narrower still — exported only when interop is enabled (`src/linux/init/init.cpp`). They also point the wrong way for the error asymmetry, being absent under `sudo -i` (microsoft/WSL#5914), in systemd units (#9719), in cron and over SSH (#12647), exactly where `/run/WSL` keeps working. snapd and npm `is-wsl` both decline to check them. Locked by `TestWSLSignalReadsNoEnvVars`. Note `Detector.LookupEnv` stays — `Apply()` needs it for the `IDSEC_BASIC_KEYRING` precedence check - - Residual theoretical gap, accepted: a chroot/private mount namespace with a fresh `/run` and no `binfmt_misc`, *combined with* a custom kernel whose `osrelease` was renamed, would inherit the env vars and detect nothing. No report of anyone hitting this exists, and in a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks the basic keyring there +- 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 load-bearing signal**, not the string matching: it is the only one that 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. `WSLInterop` is a supplement only — that binfmt entry exists only when interop is enabled in `wsl.conf` - **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 — but via `/run/WSL`, which is broader *and* more specific, never via looser string matching - Rejected: container gating (inside a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks basic; `/run/WSL` is container-safe anyway since a container gets its own `/run` tmpfs), `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 diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go index d3e81cc..6047234 100644 --- a/internal/keyringenv/keyringenv.go +++ b/internal/keyringenv/keyringenv.go @@ -22,27 +22,20 @@ const ( // os.Getenv(envVar) != "", so ANY non-empty value forces the basic keyring. envVar = "IDSEC_BASIC_KEYRING" - // procOSReleasePath exposes utsname()->release. /proc/version is NOT read: - // the kernel builds it from the very same field, so it can never be the - // deciding signal. See the note on wslSignal. + procVersionPath = "/proc/version" procOSReleasePath = "/proc/sys/kernel/osrelease" - // runWSLPath is the load-bearing marker. WSL's init creates it in - // InteropServer::Create() (src/linux/init/util.cpp, WSL_TEMP_FOLDER = - // RUN_FOLDER "/WSL"), called unconditionally from ConfigInitializeInstance() - // (src/linux/init/config.cpp) under FATAL_ERROR — a distro that cannot - // create it does not boot. Not gated on the wsl.conf interop setting, not - // gated on WSL1-vs-WSL2, and independent of the kernel, so it survives - // custom kernels (microsoft/WSL#6911), sudo, systemd units and cron. snapd - // moved to this marker after Launchpad #1991823; npm's is-wsl uses it too. + // 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: on - // WSL1 the per-distro registration is gated on Config.InteropEnabled - // (src/linux/init/config.cpp), and on WSL2 the VM-level entry is - // kernel-global, so systemd-binfmt can shadow it under a different name - // (microsoft/WSL#13449). That insufficiency is what Launchpad #1991823 - // documents. + // wslInteropPath exists only when Windows interop is enabled in wsl.conf, + // so it is a supplement to runWSLPath, never a replacement — that + // insufficiency is what Launchpad #1991823 documents. wslInteropPath = "/proc/sys/fs/binfmt_misc/WSLInterop" ) @@ -90,22 +83,6 @@ func (d Detector) IsWSL() bool { } // wslSignal returns the first WSL indicator found, and whether one was found. -// -// Three signals, deliberately. Two more were considered and removed as -// redundant — redundant meaning every configuration they detect is already -// detected by one of the three, not merely that they are weaker: -// -// - /proc/version. See the note below. -// - WSL_DISTRO_NAME / WSL_INTEROP. WSL's init sets WSL_DISTRO_NAME in -// ConfigInitializeInstance() (src/linux/init/config.cpp), the same function -// that, ~100 lines later and with no conditional between them, creates -// /run/WSL under FATAL_ERROR. So WSL_DISTRO_NAME cannot be set in a process -// whose distro did not create /run/WSL — the var is strictly narrower. -// WSL_INTEROP is narrower still: it is exported only when interop is -// enabled (src/linux/init/init.cpp). They also point the wrong way for our -// error asymmetry, being absent under `sudo -i` (microsoft/WSL#5914), in -// systemd units (#9719), in cron and over SSH, exactly where /run/WSL keeps -// working. snapd and npm's is-wsl both decline to check them. func (d Detector) wslSignal() (string, bool) { if d.GOOS != "linux" { return "", false @@ -117,32 +94,40 @@ func (d Detector) wslSignal() (string, bool) { return path, true } } - // Then the kernel release string, case-insensitively — the case sensitivity - // is the SDK's actual bug. osrelease is short and structured - // ("6.18.33.2-microsoft-standard-WSL2"), so matching "wsl" there is safe; - // systemd does the same (Microsoft||WSL on osrelease, src/basic/virt.c). - // - // /proc/version is deliberately NOT read. The kernel formats it as - // linux_proc_banner with utsname()->release as its second %s - // (fs/proc/version.c version_proc_show), and /proc/sys/kernel/osrelease is - // that identical field (kernel/utsname_sysctl.c, uts_kern_table entry - // "osrelease" -> init_uts_ns.name.release, resolved per-namespace by - // get_uts). So "microsoft" in /proc/version's release component can never - // match without osrelease matching too. Its remaining substrings are the - // build user, build host and compiler banner, where a match is a false - // positive, not coverage — a plain Linux box built on a host named - // "microsoft-builder" would trip it. WSL1 exposes osrelease as well - // ("4.4.0-19041-Microsoft"); the WSL team named both files together in - // microsoft/WSL#423. - data, err := d.ReadFile(procOSReleasePath) - if err == nil { + // 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 []string{"microsoft", "wsl"} { + for _, token := range f.tokens { if strings.Contains(lower, token) { - return procOSReleasePath, true + 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 } diff --git a/internal/keyringenv/keyringenv_test.go b/internal/keyringenv/keyringenv_test.go index 41e255d..651b1fa 100644 --- a/internal/keyringenv/keyringenv_test.go +++ b/internal/keyringenv/keyringenv_test.go @@ -54,49 +54,70 @@ func TestDetectorIsWSL(t *testing.T) { }{ { // Every signal present at once: /run/WSL must win. - name: "precedence: /run/WSL beats everything", - goos: "linux", - files: map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2"}, + 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 osrelease", - goos: "linux", - files: map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2"}, + 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 is the last signal", - goos: "linux", - files: map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2"}, + 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, }, { - // Verbatim osrelease from the WSL2 host that exposed the bug. It carries + 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 osrelease (the regression)", + name: "real WSL2 /proc/version (the regression)", goos: "linux", - files: map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2\n"}, + files: map[string]string{procVersionPath: "Linux version 6.18.33.2-microsoft-standard-WSL2 (root@builder) #1 SMP"}, want: true, }, { - // The case /run/WSL exists for. A custom kernel (microsoft/WSL#6911) - // carries neither token; interop disabled in wsl.conf drops the WSL1 - // binfmt registration and unsets WSL_INTEROP; and under sudo, a systemd - // unit or cron the WSL_* env vars are gone too. WSL's init creates - // /run/WSL regardless of all three, so it must still carry detection. - name: "/run/WSL alone: custom kernel, interop disabled, no env vars", + // 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{procOSReleasePath: "6.6.0-custom"}, + files: map[string]string{procVersionPath: "Linux version 6.6.0-custom (root@buildhost)", procOSReleasePath: "6.6.0-custom"}, exists: []string{runWSLPath}, want: true, }, @@ -107,14 +128,43 @@ func TestDetectorIsWSL(t *testing.T) { want: true, }, { - // WSL1's osrelease. The SDK's bug is that it matches "Microsoft" - // case-sensitively; this locks the case-insensitive match from the - // other direction. - name: "osrelease only: uppercase Microsoft (WSL1)", + // 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{procOSReleasePath: "4.4.0-19041-Microsoft"}, + 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", @@ -134,35 +184,41 @@ func TestDetectorIsWSL(t *testing.T) { want: true, }, { - // The WSL_* env vars are no longer signals. WSL's init sets - // WSL_DISTRO_NAME in the same unconditional function that creates - // /run/WSL under FATAL_ERROR (src/linux/init/config.cpp), so this - // combination — env vars set, every marker absent, osrelease clean — - // is not reachable on a real WSL system. Treating it as WSL would be a - // pure false positive. - name: "WSL_* env vars alone are not a signal", + name: "WSL_DISTRO_NAME only", goos: "linux", - env: map[string]string{ - "WSL_DISTRO_NAME": "Ubuntu-22.04", - "WSL_INTEROP": "/run/WSL/424_interop", - }, + 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{procOSReleasePath: "6.8.0-51-generic"}, - 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: "osrelease unreadable and nothing else present", + 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{procOSReleasePath: "microsoft"}, + files: map[string]string{procVersionPath: "microsoft"}, exists: []string{runWSLPath}, env: map[string]string{"WSL_DISTRO_NAME": "Ubuntu-22.04"}, want: false, @@ -200,79 +256,9 @@ func TestDetectorIsWSL(t *testing.T) { } } -// TestProcVersionIsNeverRead locks the removal of the /proc/version signal. -// -// The kernel builds /proc/version from utsname()->release -// (fs/proc/version.c: seq_printf(m, linux_proc_banner, ..., utsname()->release, -// ...)), and /proc/sys/kernel/osrelease *is* that field -// (kernel/utsname_sysctl.c: uts_kern_table entry "osrelease" -> -// init_uts_ns.name.release). So "microsoft" in the release component of -// /proc/version can never match without osrelease matching first. Everything -// else in that file — build user, build host, compiler banner — is a false -// positive source, not extra coverage. -// -// Reading it again would reintroduce exactly that: this fixture is a plain -// Linux box whose kernel was built on a host named "microsoft-builder". -func TestProcVersionIsNeverRead(t *testing.T) { - const procVersionPath = "/proc/version" - - var readPaths []string - d := Detector{ - ReadFile: func(name string) ([]byte, error) { - readPaths = append(readPaths, name) - return fakeFS(map[string]string{ - procVersionPath: "Linux version 6.8.0-51-generic (builder@microsoft-builder) (gcc (GCC) 13.2.0)", - procOSReleasePath: "6.8.0-51-generic", - })(name) - }, - Exists: fakeExists(), - LookupEnv: fakeEnv(nil), - GOOS: "linux", - } - - if d.IsWSL() { - t.Error("IsWSL() = true on a plain Linux box; /proc/version must not be a signal") - } - for _, p := range readPaths { - if p == procVersionPath { - t.Errorf("read %s; it is redundant with %s and must not be consulted", procVersionPath, procOSReleasePath) - } - } -} - -// TestWSLSignalReadsNoEnvVars locks the removal of the WSL_DISTRO_NAME / -// WSL_INTEROP signals. -// -// WSL's init sets WSL_DISTRO_NAME in ConfigInitializeInstance() -// (src/linux/init/config.cpp), the same function that ~100 lines later, with no -// conditional in between, creates /run/WSL under FATAL_ERROR. A process can -// therefore never see WSL_DISTRO_NAME on a distro that did not create -// /run/WSL, which makes the var strictly narrower than the marker. It is also -// absent exactly where the marker still works (sudo -i, systemd units, cron, -// ssh), so re-adding it would buy nothing and only add false positives. -// -// Note this pins wslSignal specifically — Apply() still reads LookupEnv for the -// IDSEC_BASIC_KEYRING precedence check, which must keep working. -func TestWSLSignalReadsNoEnvVars(t *testing.T) { - var lookedUp []string - d := Detector{ - ReadFile: fakeFS(map[string]string{procOSReleasePath: "6.8.0-51-generic"}), - Exists: fakeExists(), - LookupEnv: func(key string) (string, bool) { lookedUp = append(lookedUp, key); return "Ubuntu-22.04", true }, - GOOS: "linux", - } - - if d.IsWSL() { - t.Error("IsWSL() = true with every marker absent; the WSL_* env vars must not be signals") - } - if len(lookedUp) != 0 { - t.Errorf("wslSignal looked up env vars %v; they are redundant with %s and must not be consulted", lookedUp, runWSLPath) - } -} - func TestDetectorApply(t *testing.T) { - wslFiles := map[string]string{procOSReleasePath: "6.18.33.2-microsoft-standard-WSL2"} - plainFiles := map[string]string{procOSReleasePath: "6.8.0-51-generic"} + 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 From 90288c2c22a4e995541f29c214c7ae8fd8489b34 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 08:22:25 +0200 Subject: [PATCH 7/7] docs: record why the five WSL signals are redundant on purpose The wording predated the reverted simplification and was self-contradicting: it claimed the signal set covered "every configuration" and said "do not re-add", then documented a residual gap two paragraphs later. That framing is what made db73645 look justified. Replace it with the actual reason all five stay: the redundancy is about VISIBILITY, not reliability. The signals fail independently because each is reached by a different mechanism -- filesystem markers are namespace-local, the two proc paths are separately mount-visible and independently maskable, and the env vars are the only ones inherited across chroot and mount-namespace boundaries, which is exactly where the markers vanish. So "this signal is weaker" is never on its own grounds for deleting it. Also records the trap explicitly: content equivalence is not signal redundancy. Two sources can be guaranteed to hold the same bytes and still not be interchangeable. Keeps the two findings from the reverted work that remain correct and useful, now framed as reasoning aids rather than grounds for removal: - the kernel-source proof that /proc/version and osrelease cannot disagree in content (fs/proc/version.c renders utsname()->release via linux_proc_banner; kernel/utsname_sysctl.c maps osrelease onto that same field through get_uts()); - the corrected WSLInterop gating -- interop-gated per-distro on WSL1 (config.cpp:543-551), kernel-global VM-wide on WSL2, so the previous blanket "exists only when interop is enabled" was wrong. Corrects the rejected-alternatives note too: the claim that DBUS_SESSION_BUS_ADDRESS is rarely set in containers is refuted -- WSL init injects it for systemd-backed launches and chroot preserves it, so the SDK cannot be assumed to fall back to the basic keyring by itself. --- CLAUDE.md | 12 +++++++++--- internal/keyringenv/keyringenv.go | 31 ++++++++++++++++++++++++++++--- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e26bf2c..5efd740 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -165,9 +165,15 @@ Custom `SCAAccessService` follows SDK conventions: - **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 load-bearing signal**, not the string matching: it is the only one that 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. `WSLInterop` is a supplement only — that binfmt entry exists only when interop is enabled in `wsl.conf` -- **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 — but via `/run/WSL`, which is broader *and* more specific, never via looser string matching -- Rejected: container gating (inside a container `DBUS_SESSION_BUS_ADDRESS` is rarely set, so the SDK already picks basic; `/run/WSL` is container-safe anyway since a container gets its own `/run` tmpfs), `WSLENV` (user-configurable, often absent), shelling out to `systemd-detect-virt`, and third-party libs (`gookit/goutil` has the same case-sensitive `"Microsoft"` bug) +- **`/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` diff --git a/internal/keyringenv/keyringenv.go b/internal/keyringenv/keyringenv.go index 6047234..14c6c9d 100644 --- a/internal/keyringenv/keyringenv.go +++ b/internal/keyringenv/keyringenv.go @@ -33,9 +33,13 @@ const ( // Launchpad #1991823. runWSLPath = "/run/WSL" - // wslInteropPath exists only when Windows interop is enabled in wsl.conf, - // so it is a supplement to runWSLPath, never a replacement — that - // insufficiency is what Launchpad #1991823 documents. + // 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" ) @@ -83,6 +87,27 @@ func (d Detector) IsWSL() bool { } // 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