fix: force file-based keyring on WSL to prevent grant login hanging - #59
Merged
Conversation
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.
aaearon
marked this pull request as draft
August 14, 2026 05:35
aaearon
marked this pull request as ready for review
August 14, 2026 05:35
/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.
…ersion 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.
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.
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.
This reverts commit db73645. 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.
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.
This was referenced Aug 14, 2026
aaearon
added a commit
that referenced
this pull request
Aug 14, 2026
…verstating it Codex review of ed738ad. Three defects, one of them real. **The guard did not cover PersistentPreRunE.** It was installed inside RunE, and Cobra runs the root pre-run first — which on --verbose emits the WSL keyring notice through the package-level `log`, an SDK logger built on os.Stdout. That line went straight into kubectl's protocol stream. The guard now goes up in executeWithKeyringOverride, where #59 already established that startup work belongs, ahead of every Cobra hook. Which commands get it is driven by a `grant.stdout: protocol` annotation on the command rather than by matching a path, and the target is resolved with cobra.Command.Find, which strips flags and executes nothing. A resolution failure just means no guard, i.e. the previous behaviour. reserveStdout now nests, so the RunE reservation returns the outer guard's writer with a no-op Release and the outer owner restores stdout. **Layer 1 was described as sufficient. It is not.** Writers that capture os.Stdout at init escape a pointer swap — github.com/pkg/browser's package var and grant's own `log` are both examples, and the second is exactly what leaked above. Only the descriptor layer contains them, and that layer does not exist on Windows. The comments and CLAUDE.md now say which writers each layer covers and state plainly that init-time captured writers are uncontained on Windows, instead of claiming every named writer is handled. **Two cmd tests asserted POSIX modes without a Windows skip**, so they failed rather than skipped on the windows-latest leg — the branch was not green, contrary to the earlier report. TestExecCredentialCacheFilePermissions skips outright; TestKubeconfigFilePermissionsAndBackup keeps its portable backup assertions and guards only the mode check. internal/k8s already handled its equivalents this way. README claimed an unconditional 0600 guarantee for the kubeconfig and the credential cache. That is POSIX-only, and grant inspects neither ACLs nor ownership on Windows. README now says so, matching CLAUDE.md.
aaearon
added a commit
that referenced
this pull request
Aug 14, 2026
…verstating it Codex review of ed738ad. Three defects, one of them real. **The guard did not cover PersistentPreRunE.** It was installed inside RunE, and Cobra runs the root pre-run first — which on --verbose emits the WSL keyring notice through the package-level `log`, an SDK logger built on os.Stdout. That line went straight into kubectl's protocol stream. The guard now goes up in executeWithKeyringOverride, where #59 already established that startup work belongs, ahead of every Cobra hook. Which commands get it is driven by a `grant.stdout: protocol` annotation on the command rather than by matching a path, and the target is resolved with cobra.Command.Find, which strips flags and executes nothing. A resolution failure just means no guard, i.e. the previous behaviour. reserveStdout now nests, so the RunE reservation returns the outer guard's writer with a no-op Release and the outer owner restores stdout. **Layer 1 was described as sufficient. It is not.** Writers that capture os.Stdout at init escape a pointer swap — github.com/pkg/browser's package var and grant's own `log` are both examples, and the second is exactly what leaked above. Only the descriptor layer contains them, and that layer does not exist on Windows. The comments and CLAUDE.md now say which writers each layer covers and state plainly that init-time captured writers are uncontained on Windows, instead of claiming every named writer is handled. **Two cmd tests asserted POSIX modes without a Windows skip**, so they failed rather than skipped on the windows-latest leg — the branch was not green, contrary to the earlier report. TestExecCredentialCacheFilePermissions skips outright; TestKubeconfigFilePermissionsAndBackup keeps its portable backup assertions and guards only the mode check. internal/k8s already handled its equivalents this way. README claimed an unconditional 0600 guarantee for the kubeconfig and the credential cache. That is POSIX-only, and grant inspects neither ACLs nor ownership on Windows. README now says so, matching CLAUDE.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P0.
grant loginhangs forever with no output on WSL2. Found by live-tenant testing, not review.Root cause
The SDK's WSL detection is case-sensitive:
Modern WSL2 kernels report lowercase —
6.18.33.2-microsoft-standard-WSL2— soisWSL()returns false. Under WSLgDBUS_SESSION_BUS_ADDRESSis set, soGetKeyringselects the OS-provided D-Bus keyring, and the call tognome-keyring-daemonblocks indefinitely. Observed: the process parks on a futex with one ESTAB unix socket to/run/user/1000/bus, no timeout, no error, no output.No SDK fallback can save it. Both
IdsecOSProvidedKeyringandSaveTokenfall back to the basic keyring only when the underlying call returns an error. A blocked call never returns and never errors. This is also not write-only —LoadToken/GetPasswordruns during authentication before any write and can hang identically.Fix
New
internal/keyringenvdetects WSL via four signals —/proc/versionand/proc/sys/kernel/osrelease(both case-insensitive, matchingmicrosoftorwsl), plusWSL_DISTRO_NAMEandWSL_INTEROP— and setsIDSEC_BASIC_KEYRING=1before any keyring access. Wired intoExecute()as its first action, which runs before Cobra parsing and before every keyring call site.Environment precedence, which is counter-intuitive: the SDK checks
os.Getenv(...) != "", so any non-empty value forces the safe basic keyring —IDSEC_BASIC_KEYRING=0and=falseincluded. The dangerous case is an explicitly-set empty value, which leaves the OS keyring in play. So this preserves any non-empty value and overwrites only unset-or-empty on detected WSL.Fails closed. If
Setenvfails on a detected WSL host, startup aborts with the manual workaround rather than continuing into the known hang.Two SDK seams were considered and rejected:
IdsecAuthBase.CacheKeyringcovers only the outer keyring grant supplies, not ones Identity authentication builds internally;keyring.OSKeyringOpenis process-global mutable state and would couple grant to99designs/keyring, an SDK transitive dependency.Verification
On a real WSL2 host with
IDSEC_BASIC_KEYRINGunset:grant login --helpreturns immediately, andgrant version --verbosereportsWSL detected (/proc/version); forcing the file-based keyring via IDSEC_BASIC_KEYRING=1. Without--verbose, nothing extra is printed.internal/keyringenvneeds no build tags — it short-circuits onGOOS != "linux"before touching/proc.GOOS=windows go vet ./...andGOOS=darwin go vet ./...both pass.Tests cover ten
IsWSLcases (including the lowercase regression and that an emptyWSL_DISTRO_NAMEis not a signal), nineApplycases (including=0preserved, explicitly-empty overwritten on WSL, andSetenvfailure failing closed), plus a startup-hook test that records an ordered trace proving the override runs before command execution rather than only testing the detector in isolation.Note for existing users
Switching backends makes a token previously stored in the OS keyring invisible. On WSL that token almost certainly does not exist, since login could never complete. Worst case: re-run
grant login.Follow-ups, not in this PR
cyberark/idsec-sdk-golangcovering both the case-insensitive detection and the uncancellable OS-keyring operations.cmd/logout.gocallsGetKeyring(true)(always basic) while login may have used the OS keyring — a pre-existing mismatch, now consistent on WSL as a side effect.