feat: grant k8s — cluster listing, kubeconfig, elevation and exec-credential - #55
feat: grant k8s — cluster listing, kubeconfig, elevation and exec-credential#55aaearon wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-class Kubernetes support to grant-cli via a new grant k8s command group, wrapping the SDK’s SCA Kubernetes service and layering grant-owned UX (listing/selection), kubeconfig merge safety, and a hidden exec-credential plugin flow.
Changes:
- Introduces
grant k8s list,grant k8s elevate,grant k8s kubeconfig, and hiddengrant k8s exec-credential(kubectl exec plugin). - Adds
internal/k8swrapper (SDK seam, context boundary enforcement, kubeconfig merge/rewrite utilities, exec-credential caching). - Adds cluster caching (
internal/cache/cached_clusters.go) and cluster selector UI (internal/ui/cluster_selector.go), plus docs/changelog updates and dependency graph expansion for SDK k8s support.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the new grant k8s command group, usage, and constraints. |
| internal/ui/cluster_selector.go | Adds interactive cluster selection UI for k8s flows. |
| internal/ui/cluster_selector_test.go | Unit tests for cluster selector formatting, sorting, and TTY behavior. |
| internal/k8s/service.go | Wraps SDK k8s service with grant-shaped models, validation, and context handling. |
| internal/k8s/service_test.go | Unit tests for wrapper behavior (list/evaluate/elevate/kubeconfig generation). |
| internal/k8s/kubeconfig.go | Implements kubeconfig parse/merge, exec-command rewrite, atomic write, and backup. |
| internal/k8s/kubeconfig_test.go | Unit tests for merge semantics, exec rewrite, atomic write, and backups. |
| internal/k8s/execcred.go | Implements evaluate→elevate→credential flow (direct/proxy) with interaction gating. |
| internal/k8s/execcred_test.go | Unit tests for exec-credential flow branching and parameter propagation. |
| internal/k8s/execcred_cache.go | Adds on-disk ExecCredential cache with secure permissions and expiry validation. |
| internal/k8s/execcred_cache_test.go | Unit tests for cache correctness, expiry handling, and permissions. |
| internal/cache/cached_clusters.go | Adds cached cluster lister decorator (mirrors eligibility caching). |
| internal/cache/cached_clusters_test.go | Unit tests for cluster caching behavior and keys. |
| cmd/output_types.go | Adds JSON output structs for k8s list/elevate/kubeconfig. |
| cmd/k8s.go | Registers the k8s parent command and bootstrapping/caching helpers. |
| cmd/k8s_list.go | Implements grant k8s list (text/JSON) and provider validation. |
| cmd/k8s_list_test.go | Unit tests for list output, provider validation, auth gating, and errors. |
| cmd/k8s_kubeconfig.go | Implements kubeconfig generation, merge/write, exec rewrite, and reporting. |
| cmd/k8s_kubeconfig_test.go | Unit tests for merge behavior, stdout mode, env honoring, JSON output, and partial failures. |
| cmd/k8s_integration_test.go | Integration tests for help output and clean failure without auth. |
| cmd/k8s_exec_credential.go | Implements hidden kubectl exec-credential command and cache integration. |
| cmd/k8s_exec_credential_test.go | Unit tests for stdout discipline, API negotiation, caching, and non-interactive behavior. |
| cmd/k8s_elevate.go | Implements grant k8s elevate with arg/interactive resolution and JSON/text output. |
| cmd/k8s_elevate_test.go | Unit tests for elevate resolution, role override, JSON output, and non-TTY behavior. |
| cmd/interfaces.go | Adds DI interfaces for k8s lister/elevator/kubeconfig/exec-credential provider. |
| cmd/commands.go | Registers the k8s command group in the CLI. |
| CLAUDE.md | Updates project guidance for the k8s feature and its dependency decision. |
| CHANGELOG.md | Records new k8s commands/features and the dependency/binary impact. |
| go.mod | Adds indirect modules pulled in by SDK k8s support (Azure/AWS/JWE/etc.). |
| go.sum | Records checksums for newly introduced transitive modules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // SelectCluster presents an interactive selector for choosing a cluster. | ||
| func SelectCluster(clusters []k8s.Cluster) (*k8s.Cluster, error) { | ||
| if !IsInteractive() { | ||
| return nil, fmt.Errorf("%w; pass a cluster name or --fqdn, or run 'grant k8s list' to see eligible clusters", ErrNotInteractive) |
| func parseExecCredentialFlags(cmd *cobra.Command) execCredentialFlags { | ||
| var f execCredentialFlags | ||
| f.csp, _ = cmd.Flags().GetString("csp") | ||
| f.fqdn, _ = cmd.Flags().GetString("fqdn") | ||
| f.roleID, _ = cmd.Flags().GetString("role-id") | ||
| f.organizationID, _ = cmd.Flags().GetString("organization-id") | ||
| f.namespace, _ = cmd.Flags().GetString("namespace") | ||
| return f | ||
| } |
Two blockers and five majors from the PR #55 review. Blockers: - exec-credential no longer corrupts the kubectl protocol under --verbose. The SDK logger is built with log.New(os.Stdout, ...) and re-reads IDSEC_LOG_LEVEL on every call, so the root pre-run's INFO level put log lines on stdout ahead of the credential JSON. The command now forces CRITICAL for its duration (restoring it after) and routes diagnostics to stderr via the SDK's kubectl-login channel. The old test missed this because it ran the child command without the root PersistentPreRunE; it now drives the real root, redirects os.Stdout to a pipe, and asserts the pipe stays empty. Verified the test fails when the fix is reverted. - exec-credential no longer authenticates before it knows whether it needs to. Exec info parsing, flag validation and the credential cache lookup all happen first, so a cache hit — or a malformed apiVersion — can never trigger a browser/MFA prompt. On a miss it uses LoadAuthentication (keyring read, never prompts) and only escalates to interactive Authenticate when kubectl said stdin is available. Majors: - Azure identity binding is no longer silently disabled: the Idira session JWT is now populated and an Azure request without one is refused rather than authenticating as whoever is logged into the Azure CLI. - Credential cache hardening: writes create a fresh 0600 file and rename over the target (a loose pre-existing file is replaced, never written into); reads reject symlinks, non-regular files, loose file or directory modes, foreign ownership, and payloads with no credential material; the cache key now includes the organization so tenants cannot collide. - connectionMethod fails closed — anything that is not exactly direct or proxy is rejected before elevation instead of running the direct flow. - Rewritten exec stanzas always carry interactiveMode (IfAvailable), which client-go REQUIRES for the v1 API and without which the kubeconfig is rejected before grant is invoked. - $KUBECONFIG resolution follows kubectl's write rule: the first entry that exists, or the last when none do — no longer creating an earlier nonexistent file that shadows the real config. Minors: - runWithContext is documented honestly as a caller-side timeout that abandons rather than cancels, including the leaked goroutine and the shared-header mutation it can cause afterwards. - BackupOnce uses O_CREATE|O_EXCL so concurrent runs cannot clobber the backup, and refuses a symlinked or non-regular source. R-5a flag names and the certificateData hard-fail were both confirmed correct against SDK source during this review.
Handoff — PR paused, converted to draftUpdated 2026-08-14 at An earlier revision of this comment claimed "both blockers fixed", "every writer contained by layer 1", and "green". All three were wrong. They are corrected here. StatusPaused after three Codex review cycles, each of which found defects the previous missed, including defects introduced by the previous cycle's fixes. A fourth cycle reviewed the fixes below and found a real leak plus two overstatements, which are also addressed here. Branch head: What works
Resolved across the review cycles: auth/cache ordering, Azure identity binding via The rebase
No conflict required a judgement call about product naming or about #59/#60 behaviour. The second rebase (onto Blocker 1 — stdout contamination: reduced, not eliminatedFixed in
The guard is installed before Cobra's pre-run. The first version installed it inside The two layers do not cover the same writers, and layer 1 alone is not sufficient:
Concrete init-time capturers in grant's own build graph:
Tests: Blocker 2 — Windows symlink checks: fixed and CI-verifiedFixed in Replaced with a real primitive per platform: On the dependency constraint: this uses The symlink tests used to skip on Windows, which is exactly how the regression survived: the contract was verified only where it happened to be true. They now run everywhere, skipping only when the machine itself will not create a symlink. Confirmed at Lower-severity items — all fixed, all confirmed
Two POSIX-mode tests that were failing on Windows — now confirmed fixed
Verbatim from the The first skips as intended; the second runs and passes, because only its mode half is guarded. The whole Known gaps, deliberately not closed
Cost, re-measured against current
|
main (43ee422) |
this branch | delta | |
|---|---|---|---|
modules (go list -deps) |
33 | 49 | +16 |
binary (linux/amd64, -trimpath -s -w) |
11,845,924 B | 14,315,812 B | +2,469,888 B (+20.8%) |
The +16 modules figure from the roadmap holds exactly against current main.
Verified — and precisely how far
Local (Linux): make build, make test, make test-race, make lint, gofmt -s -l . all clean. GOOS=windows go vet ./... clean; Windows test binaries for ./cmd and ./internal/k8s compile. Cross-built windows/amd64, linux/arm64, darwin/arm64 (the last two because the descriptor redirect is Dup3 on Linux and Dup2 on the BSDs).
Windows behaviour is verified by CI, not by hand on a Windows machine — but it has now been through a full Windows run. Run 31787967746 at 633a5ba: test (windows-latest): success, test (ubuntu-latest): success. Every package reports ok on both legs, and the self-update end-to-end step (idle and running targets, replace and rollback) passes on both. The earlier caveat that the test fixes "have not yet been through a full Windows run" no longer applies. Nothing here has run against a Kubernetes tenant.
Never verified — no k8s tenant was available
- The real
exec.commandembedded in the DPA-generated kubeconfig. (The rewrite targets{idsec, idsec-cli, ark, ark-cli}defensively; SDK flag names--csp/--role-id/--fqdn/--organization-id/--namespaceare confirmed correct permodels/idsec_sca_k8s_elevate.go:89-94.) - The direct/proxy connection split.
- A real kubectl round-trip.
- The Azure
az loginpath. - The SDK token providers (AWS STS presign, Azure CLI, DPA JWE) sit behind an injected seam and have never executed.
To resume — re-verify in this order
- Confirm the stdout boundary end to end. The guard now sits ahead of Cobra's pre-run; test a real WSL
grant --verbose k8s exec-credentialinvocation, and test writers that capturedos.Stdoutat init on Windows, where layer 2 does not apply. Run the complete Windows CI suite and confirm the POSIX-mode assertions corrected inDone at578fccabehave, rather than assuming from the Linux run.633a5ba— see above. Re-run it against whatever head you resume from.- Exercise the DPA-generated kubeconfig shape and the exec-command rewrite against a real tenant.
- Run an actual kubectl round-trip for both the direct and proxy paths, including AWS IDC and the Azure CLI identity binding.
- Validate Windows cache ACL ownership and privacy before treating cached credentials there as equivalently protected to POSIX.
The open product question is still whether exec-credential should ship at all without live-cluster validation — grant k8s list and kubeconfig are lower risk and could ship separately.
Full context: docs/plan-2026-08-13-roadmap.md, Item 5. The PAUSED section of that file lives on main, not on this branch; it is refreshed in #62.
PR #55 is paused as a draft after three Codex review cycles. Two blockers remain (exec-credential stdout contamination, Windows symlink checks bypassed), plus three lower-severity items. Nothing was validated against a real k8s tenant.
Two blockers and five majors from the PR #55 review. Blockers: - exec-credential no longer corrupts the kubectl protocol under --verbose. The SDK logger is built with log.New(os.Stdout, ...) and re-reads IDSEC_LOG_LEVEL on every call, so the root pre-run's INFO level put log lines on stdout ahead of the credential JSON. The command now forces CRITICAL for its duration (restoring it after) and routes diagnostics to stderr via the SDK's kubectl-login channel. The old test missed this because it ran the child command without the root PersistentPreRunE; it now drives the real root, redirects os.Stdout to a pipe, and asserts the pipe stays empty. Verified the test fails when the fix is reverted. - exec-credential no longer authenticates before it knows whether it needs to. Exec info parsing, flag validation and the credential cache lookup all happen first, so a cache hit — or a malformed apiVersion — can never trigger a browser/MFA prompt. On a miss it uses LoadAuthentication (keyring read, never prompts) and only escalates to interactive Authenticate when kubectl said stdin is available. Majors: - Azure identity binding is no longer silently disabled: the Idira session JWT is now populated and an Azure request without one is refused rather than authenticating as whoever is logged into the Azure CLI. - Credential cache hardening: writes create a fresh 0600 file and rename over the target (a loose pre-existing file is replaced, never written into); reads reject symlinks, non-regular files, loose file or directory modes, foreign ownership, and payloads with no credential material; the cache key now includes the organization so tenants cannot collide. - connectionMethod fails closed — anything that is not exactly direct or proxy is rejected before elevation instead of running the direct flow. - Rewritten exec stanzas always carry interactiveMode (IfAvailable), which client-go REQUIRES for the v1 API and without which the kubeconfig is rejected before grant is invoked. - $KUBECONFIG resolution follows kubectl's write rule: the first entry that exists, or the last when none do — no longer creating an earlier nonexistent file that shadows the real config. Minors: - runWithContext is documented honestly as a caller-side timeout that abandons rather than cancels, including the leaked goroutine and the shared-header mutation it can cause afterwards. - BackupOnce uses O_CREATE|O_EXCL so concurrent runs cannot clobber the backup, and refuses a symlinked or non-regular source. R-5a flag names and the certificateData hard-fail were both confirmed correct against SDK source during this review.
…imed The non-interactive hint from the cluster selector told users to "pass a cluster name or --fqdn". grant k8s elevate has no --fqdn flag; the cluster is a positional argument and --fqdn exists only on the hidden exec-credential command. Flagged by Copilot on PR #55. CLAUDE.md described the stdout reservation as "two switches, both required" and the no-follow flag as "syscall.O_NOFOLLOW / 0", both of which described the defects rather than the behaviour. It now records the stdout guard and its honest per-platform guarantee, the Windows reparse point check and why golang.org/x/sys is a direct dependency, the backup-integrity and readEntry rules, and states plainly that checkPrivateToCurrentUser on Windows is a gap rather than an equivalent.
0a9907b to
ed738ad
Compare
Wires the SDK's pkg/services/sca/k8s into grant behind a thin, grant-shaped wrapper and lands the first command on top of it. - internal/k8s: Service wrapper with grant-owned Cluster/Connection/ ElevateResult models, CSP validation, wrapped errors, and context propagation (direct for GenerateKubeconfigParallel, enforced at the wrapper boundary for the SDK entry points that take no context) - internal/cache: CachedClusterLister decorator (clusters_<csp> keys) - internal/ui: cluster selector quartet with TTY detection - cmd: grant k8s parent + grant k8s list (--provider, --refresh, --output json) Implements decision D-4/5B: importing the SDK k8s package adds exactly the 16 modules enumerated in the roadmap (Azure SDK, azidentity, MSAL, armauthorization, aws-sdk-go-v2 + sts + credentials, smithy-go, go-jose/v4, pkg/browser, godebug). This is a deliberate departure from the zero-new-deps goal, recorded in CLAUDE.md. Untested against a live Kubernetes cluster.
Completes the grant k8s surface with the kubectl-protocol and filesystem half of the feature. - grant k8s elevate [cluster]: JIT elevation for one cluster, resolving by name, FQDN or cluster ID, with the interactive picker (and ErrNotInteractive plus a 'grant k8s list' hint) when omitted - grant k8s kubeconfig: fetch the DPA-generated kubeconfig and MERGE it. Entries grant owns are named grant-<csp>-<name>; colliding grant entries are replaced and reported, everything else survives untouched (a yaml.Node tree is edited in place so comments and structure are preserved). current-context changes only with --set-current-context. Atomic write via a temp file in the same directory + fsync + rename, mode 0600 (an existing narrower mode is preserved, a group/world readable one is re-secured with a warning), parent dir 0700, and a one-time <target>.grant.bak. $KUBECONFIG list form honoured. Escape hatches --stdout and --file; --file rather than --output, which is the global text|json flag and must not be shadowed - grant k8s exec-credential (Hidden): kubectl credential plugin. Stdout carries the ExecCredential JSON and nothing else. The response apiVersion echoes the one kubectl requests via KUBERNETES_EXEC_INFO (v1beta1 and v1); spec.interactive gates flows that would open a browser or run az login, which now fail fast rather than hanging. Credentials are cached at 0600 until their stamped expiry — the SDK's refresh buffer is applied once at the source and never again - internal/k8s: kubeconfig merge/write, ExecCredential cache with an injectable clock, and the evaluate -> elevate -> direct/proxy credential orchestration behind an injectable seam R-5a is implemented defensively: exec stanzas are rewritten to point at grant only when the command basename is a known official CLI binary, carrying over only flags grant understands. The real shape of the DPA-generated kubeconfig could not be observed without a live tenant. Untested against a live Kubernetes cluster.
Two blockers and five majors from the PR #55 review. Blockers: - exec-credential no longer corrupts the kubectl protocol under --verbose. The SDK logger is built with log.New(os.Stdout, ...) and re-reads IDSEC_LOG_LEVEL on every call, so the root pre-run's INFO level put log lines on stdout ahead of the credential JSON. The command now forces CRITICAL for its duration (restoring it after) and routes diagnostics to stderr via the SDK's kubectl-login channel. The old test missed this because it ran the child command without the root PersistentPreRunE; it now drives the real root, redirects os.Stdout to a pipe, and asserts the pipe stays empty. Verified the test fails when the fix is reverted. - exec-credential no longer authenticates before it knows whether it needs to. Exec info parsing, flag validation and the credential cache lookup all happen first, so a cache hit — or a malformed apiVersion — can never trigger a browser/MFA prompt. On a miss it uses LoadAuthentication (keyring read, never prompts) and only escalates to interactive Authenticate when kubectl said stdin is available. Majors: - Azure identity binding is no longer silently disabled: the Idira session JWT is now populated and an Azure request without one is refused rather than authenticating as whoever is logged into the Azure CLI. - Credential cache hardening: writes create a fresh 0600 file and rename over the target (a loose pre-existing file is replaced, never written into); reads reject symlinks, non-regular files, loose file or directory modes, foreign ownership, and payloads with no credential material; the cache key now includes the organization so tenants cannot collide. - connectionMethod fails closed — anything that is not exactly direct or proxy is rejected before elevation instead of running the direct flow. - Rewritten exec stanzas always carry interactiveMode (IfAvailable), which client-go REQUIRES for the v1 API and without which the kubeconfig is rejected before grant is invoked. - $KUBECONFIG resolution follows kubectl's write rule: the first entry that exists, or the last when none do — no longer creating an earlier nonexistent file that shadows the real config. Minors: - runWithContext is documented honestly as a caller-side timeout that abandons rather than cancels, including the leaked goroutine and the shared-header mutation it can cause afterwards. - BackupOnce uses O_CREATE|O_EXCL so concurrent runs cannot clobber the backup, and refuses a symlinked or non-regular source. R-5a flag names and the certificateData hard-fail were both confirmed correct against SDK source during this review.
… cache on Windows Second review pass on the grant k8s surface. Blocker — the SDK has a stdout-reservation switch that was never flipped. Silencing the loggers was only half the job: during interactive auth the SDK prints its browser-redirect message straight to os.Stdout unless config.IsStdoutReservedForData() is true (pkg/auth/identity/idsec_identity.go :546-556), and that path is reachable on a cache miss with spec.interactive:true. reserveStdoutForProtocol now calls sdkconfig.ReserveStdoutForData() and releases it on exit. The stdout test reproduces the SDK's exact branch inside the credential flow and asserts the redirect text never reaches the pipe standing in for the process stdout; verified it fails when the call is removed. Regression — the credential cache could never hit on Windows. Go synthesizes Windows FileMode bits from a single read-only attribute (every file 0666, every directory 0777) and os.Chmod there has no ACL semantics, so the POSIX perm&0o077 checks rejected everything and kubectl would have re-authenticated on every call. Permission, ownership and chmod logic now sit behind a posixPermissions build-tag const, alongside openNoFollowFlag; the same gate is applied to targetFileMode, which would otherwise have warned on every Windows run. Added a windows-tagged regression test and GOOS=windows go vet to the verify sequence. Minors: - Closed the TOCTOU in both readers. The credential cache and BackupOnce now open with O_NOFOLLOW and validate through fstat, so the file that is checked is the file that is read. BackupOnce had no private-directory guarantee to fall back on, so this mattered more there. - Cache rejections are no longer silent. CredentialCache.Warn reports the rejected path and reason on stderr before the command degrades into a fresh login; ordinary misses stay quiet. - Documented that the env vars and the stdout reservation are process-global (safe: one command per process) and that a runWithContext-abandoned goroutine can log after the restore — though only once the JSON is out. - Corrected the LoadAuthentication contract comment: (nil, nil) is not universal; unusable refresh state returns an error (pkg/auth/idsec_isp_auth.go:144). The code already handled both.
kubectl's exec-credential protocol requires stdout to carry the ExecCredential JSON and nothing else. The previous fix called sdkconfig.ReserveStdoutForData(), which moves exactly one SDK message — the browser redirect — to stderr. The SDK also drives Survey prompts for PIN entry, MFA-method selection, OOB verification and username/password, whose default Stdio.Out is os.Stdout and which consult no reservation flag at all. All four are reachable on a cache miss via Authenticate. Enumerating writers is the wrong shape for this problem: the list is not knowable from here and grows with every SDK release. This takes the boundary instead. The command duplicates the descriptor behind stdout, re-points that descriptor at stderr, sets os.Stdout = os.Stderr, and writes the JSON to the saved descriptor. Two layers, because they catch different writers: the os.Stdout swap contains everything that resolves os.Stdout when it runs, and the descriptor swap also contains writers that captured it at init (github.com/pkg/browser holds such a var) and subprocesses that inherit it. Release is deferred, so it runs on panic. The descriptor layer is POSIX-only. A Windows handle cannot be re-pointed and SetStdHandle does not affect an already-built *os.File, so there the guarantee is the os.Stdout swap alone; the comments say that rather than implying parity. No module was added: dup/dup2/dup3 come from syscall. The old test reproduced the browser-message branch specifically, which is why it stayed green while four prompts walked past it. The new test asserts the property instead: an arbitrary write to os.Stdout from inside the credential flow does not reach the process's real stdout, and what does reach it parses as exactly one ExecCredential. It runs without cmd.SetOut so the JSON travels the production route.
openNoFollowFlag was syscall.O_NOFOLLOW on POSIX and 0 on Windows, so every symlink check on Windows was a no-op. The credential cache opened symlinks normally and f.Stat() then described the target rather than the link, with the ownership and mode validation skipped alongside; BackupOnce dereferenced a symlinked kubeconfig while its doc comment promised it refuses one. That regressed an earlier Lstat rejection. Mapping a security primitive to zero on a platform that lacks its spelling is not a port. Windows has the primitive. CreateFile with FILE_FLAG_OPEN_REPARSE_POINT opens the reparse point itself rather than its target, so the handle refers to the link; asking that handle for its attributes then reports whether we were handed one, with no window in between for the path to be swapped. That is the same descriptor-based discipline the POSIX path uses. The flag constant becomes openNoFollowRead(path), with isSymlinkOpenError to classify the failure. golang.org/x/sys moves from indirect to direct — it was already in the Windows build graph, so no module was added. The symlink tests skipped on Windows, which is how this survived: the contract was checked only where it happened to be true. They now run everywhere, skipping only when the machine itself will not create a symlink (Windows without Developer Mode or admin). Three lower-severity items from the same review, all in this code: - readEntry treated every non-ENOENT open failure as "symlink, or not a regular file", deleted the entry and discarded the real error, so descriptor exhaustion or a transient I/O fault would destroy a valid credential. Only a symlink earns removal now; everything else propagates. - Get warned about the cache being "not usable" when the directory did not exist, which is every first run on a new install — CacheDir() returns the path without creating it. A missing directory is now an ordinary quiet miss. The existing test missed this by creating the directory first. - BackupOnce could leave a partial <target>.grant.bak after a failed write, and the next run treats any existing backup as complete and goes on to replace the kubeconfig. A failed write now removes it; the write step is a package var so the failure is testable.
…imed The non-interactive hint from the cluster selector told users to "pass a cluster name or --fqdn". grant k8s elevate has no --fqdn flag; the cluster is a positional argument and --fqdn exists only on the hidden exec-credential command. Flagged by Copilot on PR #55. CLAUDE.md described the stdout reservation as "two switches, both required" and the no-follow flag as "syscall.O_NOFOLLOW / 0", both of which described the defects rather than the behaviour. It now records the stdout guard and its honest per-platform guarantee, the Windows reparse point check and why golang.org/x/sys is a direct dependency, the backup-integrity and readEntry rules, and states plainly that checkPrivateToCurrentUser on Windows is a gap rather than an equivalent.
…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.
578fcca to
633a5ba
Compare
Stacked on #47 (needs SDK v0.8.1). Merge order: #50 → #47 → this.
Adds
grant k8s list,grant k8s kubeconfig,grant k8s elevate, and a hiddengrant k8s exec-credential(kubectl plugin protocol, stdout JSON only).Dependency cost — much lower than projected
Importing the SDK's
pkg/services/sca/k8swas expected to cost 2–3× binary size. Measured:The linker drops the unreachable bulk of the Azure/AWS SDKs, and no new advisory is reachable from grant's code. This is the justified departure from the zero-new-dependencies goal recorded as D-4/5B: the zero-dep alternative provably cannot deliver
exec-credential, since direct AWS needssts, direct Azure needsazidentity, and proxy needsgo-josefor JWE.Design notes
Grant owns the UI, selector, cache, kubeconfig merge and command layer; the SDK owns transport and credential flows.
GenerateKubeconfigParallelreceives the caller's context directly. Every other SDK entry point accepts no context and issues its request oncontext.Background()internally, sorunWithContextis a caller-side timeout, not context propagation: onctx.Doneit unblocks the caller and abandons the call — the goroutine leaks until the request finishes, its result is dropped, and it can still mutate shared SDK client headers afterwards. Do not read a returnedcontext.Canceledas "the request stopped".cmd/root.gois deliberately untouched —bootstrapK8sServiceandbuildCachedClusterListerlive incmd/k8s.goto minimise conflict surface with #53.Kubeconfig safety: merge rather than overwrite,
0600/0700modes on POSIX (Windows has no ACL enforcement — see the handoff), never widens an existing mode, atomic write via a temp file in the same directory, backup on first write,current-contextuntouched without explicit opt-in, and$KUBECONFIGlist form honoured. Preservation is semantic rather than byte-literal — ayaml.Nodetree is edited in place, preserving comments and key order, but re-encoding normalises indentation to 2 spaces.Two deliberate deviations
--filerather than--output <path>onkubeconfig.--output/-ois grant's globaltext|jsonpersistent flag; a local--outputwould shadow it, sogrant k8s kubeconfig --output jsonwould write a file namedjson.Defensive exec-command rewriting (risk R-5a). The DPA builds the kubeconfig server-side and nothing in v0.8.1 reveals the embedded
exec.command; the SDK reaches its token providers through a CLI verb namedkubectl-login, which strongly implies the response points atidsec/ark. Rewriting happens only when the command basename is in{idsec, idsec-cli, ark, ark-cli}(.exestripped), carrying over--csp/--fqdn/--role-id/--organization-id/--namespace; anything else (e.g.gke-gcloud-auth-plugin) passes through untouched. If the real args use different flag spellings, recognised flags are silently dropped — the fallback isexec-credentialerroring on a missing--fqdnrather than misauthenticating.Not verifiable without a live cluster
The generated kubeconfig's actual shape (R-5a); the direct/proxy split and therefore which credential path gets real exercise; whether
spec.interactivegating matches real kubectl end to end; Azureaz loginbehaviour; whetherevaluatealways returnscertificateDataon proxy results (currently a hard failure if a JWE token arrives without it); and the azure →azure_resourceCSP segment mapping.make test,make lint,make build,make test-integrationpass.make test-racefails only on the pre-existinginternal/uirace fixed by #52.