Harden windowless kubectl exec authentication - #12950
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
🟡 Changes recommended
Kubeconfig context selection and preflight cancellation handling have unresolved moderate issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens windowless Windows kubectl authentication for PostgreSQL operations.
Changes:
- Adds kubeconfig exec-auth validation and shared
kubectlconstruction. - Preserves stdin, cancellation, arguments, and readiness diagnostics.
- Adds cross-platform and Windows regression coverage.
- Updates CI, documentation, and spellcheck configuration.
File summaries
| File | Summary |
|---|---|
pkg/cli/pgbackup/pgbackup.go |
Routes PostgreSQL operations through the guarded command helper. |
pkg/cli/pgbackup/kubectl.go |
Adds command construction and exec-auth preflight validation. |
pkg/cli/pgbackup/kubectl_windows_test.go |
Tests Windows console behavior and process cleanup. |
pkg/cli/pgbackup/kubectl_test.go |
Covers command, input, error, and cancellation behavior. |
pkg/cli/pgbackup/kubectl_other_test.go |
Enables shared regression tests on non-Windows systems. |
pkg/cli/pgbackup/kubectl_config_test.go |
Tests kubeconfig selection, merging, defaults, and validation. |
docs/architecture/rad-cli.md |
Documents the guard and its limitations. |
.github/workflows/unit-tests.yaml |
Adds pgbackup to Windows CI tests. |
.cspellignore |
Allows the pgbackup term in spellcheck. |
Review details
Suppressed comments (2)
pkg/cli/pgbackup/kubectl.go:56
- Passing an empty
contextNamemakes client-go'sConfirmUsablevalidateconfig.CurrentContextfor the existence check, even whenoverrides.CurrentContextselects an explicit context. If the current context is stale or missing butkubeContextis valid, this preflight rejects a command that kubectl would run with the explicit context. PasskubeContextas the constructor's context name (leaving it empty only for current-context fallback), and add a regression case with a missing current context.
selected, err := clientcmd.NewNonInteractiveClientConfig(*config, "", overrides, rules).MergedRawConfig()
pkg/cli/pgbackup/kubectl_windows_test.go:41
- The attached-console filter does not match the actual subtest name
broken config is inspected only without a consolebecause it usesbroken_config. Consequently this helper only exercisesAlwaysunder an attached console and never verifies that malformed configuration remains pass-through there; match the real subtest name or add a dedicated attached-case filter.
{name: "attached", flags: windows.CREATE_NEW_CONSOLE, tests: "^TestKubectlHelper$/^(Always|broken_config)"},
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Reject selected kubeconfig exec-auth requiring interaction before pgbackup kubectl launches, while preserving finite restore stdin and command diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Honor explicit contexts even when current-context is stale, prefer caller cancellation over preflight errors, and align the documented Windows regression command with CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
b815a2f to
da8010a
Compare
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12950 +/- ##
==========================================
+ Coverage 59.78% 59.96% +0.17%
==========================================
Files 778 779 +1
Lines 45918 45944 +26
==========================================
+ Hits 27454 27551 +97
+ Misses 18464 18393 -71 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closing for now because this is not a priority. We can revisit the scope of this work later. |
Summary
Guard every PostgreSQL pod lookup, readiness, backup, and restore kubectl launch with the shared
process.IsWindowless()policy. In Windows no-console mode, inspect the selected kubeconfig exec-auth configuration without authenticating or invoking its plugin, and rejectinteractiveMode: Alwayswith guidance to configure non-interactive Kubernetes credentials or use an attached console.Use client-go's standard file-list merging, context selection, API defaults, and selected-config validation. Disable kubeconfig migration during this read-only check. Existing Radius config helpers load a single file, so they cannot supply kubectl's
KUBECONFIGmerge semantics here.Never,IfAvailable, and unused exec-auth configurations remain allowed; malformed selected configuration is not silently ignored. Explicit contexts remain usable when an unrelated current context is stale, and caller cancellation takes precedence over preflight errors.Keep lookup/readiness/backup stdin at EOF and preserve restore's
kubectl exec -iwith every SQL byte followed by EOF. Preserve arguments, namespace, pod selection, caller cancellation, command error causes, diagnostics, and the existing--timeout=120sreadiness limit. Readiness failures are no longer all described as timeouts. Attached-console and non-Windows authentication behavior remains unchanged.Reason for change
Closes #12949.
Parent: #12887, section 3. PostgreSQL password flags and the other remaining parent tasks are not included.
This PR targets
main. #12946 has merged as25d92745e60973eedd04c337303477123cad922c. The branch has been rebased onto that currentmaincommit, replaying only the four kubectl-specific commits after the old dependency head. The old dependency commits are no longer part of this PR, andpkg/processhas no changes relative tomain. The requiredprocess.IsWindowless()function now comes frommain.How to test
Run
go test ./pkg/cli/pgbackup ./pkg/process -count=1 -timeout=2m. The fixtures use temporary kubeconfigs and fake executables; no cluster or real credentials are required. On Windows, the suite runs a no-console helper usingFreeConsoleand a real attached-console helper usingCREATE_NEW_CONSOLE, and contains their descendants in a kill-on-close Job Object.Coverage includes all four launch paths; actual EOF and large, exact SQL input; argument and output preservation; explicit/current context selection including stale current contexts; first-file-wins and split-file
KUBECONFIGmerging; default home loading;Alwaysrejection before kubectl/plugin launch; rechecking backup/restore after lookup;Never/IfAvailablepass-through; API mode defaults/validation; broken config; no-backup no-op; ordinary exit errors; and caller cancellation before/during preflight and execution. Regression cases for stale current-context and canceled callers were reproduced against the original implementation before applying the review fixes.After the rebase, the native Windows amd64 suites for
pkg/cli/pgbackupandpkg/processpassed on the updated dependencies frommain.git range-diffconfirms all four replayed patches are unchanged, andgit diff --checkpasses. No conflict resolutions or new implementation changes were needed.Earlier validation also covered Linux with
-race, targetedgolangci-lint(zero issues), Windows arm64 test-binary cross-compilation, the full CI spellcheck glob (573 files, zero issues), and Markdown checks for the updated contributor guide. Native arm64 execution is registered in the existing Windows CI matrix but was not run locally. Test-only safety bounds do not add production timeouts.Markdown checks previously reported the same pre-existing table-format issues at
docs/architecture/rad-cli.mdlines 20, 27, and 35 on both the dependency baseline and this branch; unrelated tables are unchanged. No live-cluster or third-party authentication-plugin validation was performed. This guard does not guarantee completion for arbitrary plugins that ignore interaction modes or cancellation. Native arm64 execution remains a CI validation step.File change summary
pkg/cli/pgbackup/kubectl.gopkg/cli/pgbackup/pgbackup.gopkg/cli/pgbackup/kubectl_test.gopkg/cli/pgbackup/kubectl_config_test.gopkg/cli/pgbackup/kubectl_windows_test.gopkg/cli/pgbackup/kubectl_other_test.go.github/workflows/unit-tests.yaml.cspellignoredocs/architecture/rad-cli.mddocs/contributing/contributing-code/contributing-code-cli/README.md