From 166f6d8aae6575f6c2485090aa25007ae8629b4d Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 09:22:15 +0100 Subject: [PATCH 1/2] Keep container diagnostics bootstrap credentials out of copied commands Reuse the complete-download and preflight private-token entry boundary for both monitoring modes. Keep issued credentials separate and no-store, use the installer-owned token path in the diagnostic service reference, and reject structural unit injection before minting. Add executable root/sudo, history, transport, failure cleanup and systemd grammar regressions alongside the subsystem contracts. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 19 ++ .../v6/internal/subsystems/api-contracts.md | 19 ++ .../internal/subsystems/security-privacy.md | 17 ++ .../internal/subsystems/storage-recovery.md | 13 ++ internal/api/agent_install_command_shared.go | 34 +-- .../api/agent_install_command_shared_test.go | 12 +- .../container_install_command_test.go | 219 ++++++++++++++++++ internal/api/configapi/install_command.go | 30 ++- internal/api/contract_test.go | 12 +- internal/api/router.go | 8 +- .../router_low_coverage_additional_test.go | 16 +- internal/api/security_regression_test.go | 53 +++++ 12 files changed, 419 insertions(+), 33 deletions(-) create mode 100644 internal/api/configapi/container_install_command_test.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 2c16d51b43..0bf3d27588 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -3452,6 +3452,25 @@ Agent` secondary handoff against the live setup wizard instead of relying ## Current State +### Credential-safe container diagnostics bootstrap (1 October 2026) + +The existing container-runtime migration command now shares the Proxmox +complete-download/preflight/private-input implementation in +`internal/api/configapi/install_command.go`. The authenticated response keeps +its separate `token` field; `installCommand` never interpolates it. A terminal +prompt inside the root/sudo child creates a bounded 0700/0600 handoff, passes +only its path, and removes it on success, failure or handled signals. Optional +auth needs neither a credential nor a terminal. Default host-plus-Docker and +explicit workload-only modes, their persisted scopes, 30-second interval and +plain-HTTP continuity remain unchanged; neither mode adds command execution. +The diagnostic systemd reference uses the default installer's protected +`/var/lib/pulse-agent/token`, not a token literal or secret environment value. +It is not a replacement for the complete installer-generated unit, especially +on custom-state or privilege-profile installations. Executable root/sudo, +history, argv, cleanup and real TLS-download fixtures are in +`configapi/container_install_command_test.go`; they are not native installation +or Docker/Podman estate acceptance. + ### Credential-safe Proxmox bootstrap (1 October 2026) Proxmox initial bootstrap keeps credentials out of copied shell source. The current PVE/PBS agent command fetches a complete installer, runs its credential-free preflight, then privately prompts inside the root/sudo Bash child. Its 0700 directory and 0600 token file are owned by that child, passed only by path and removed on exit or handled signals. The UI reveals credentials separately, clears cached setup material on close and discards late issuance after close/reset. No exec scope is added. Existing fleet reporting is unchanged; newly enrolling an older agent with a newer server requires the current installer. New agents and the root installer still validate coherent old-server artifacts during rolling upgrades, but never execute their command strings. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 9298790eed..1fcbf79804 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -4579,6 +4579,25 @@ auto-register mutation boundary. ## Current State +### Credential-safe container diagnostics response (1 October 2026) + +`POST /api/diagnostics/docker/prepare-token` retains its fields and durable, +tenant-bound monitoring-token issuance. Only the separate `token` field may +contain the credential; `installCommand` is now a credential-free single line +with complete download, installer preflight, then root/sudo private terminal +input and token-file handoff. The response is `Cache-Control: no-store`. +`systemdServiceSnippet` is a default-Linux reference to the installer's private +state file, with no raw token in environment or process arguments. It explicitly +instructs callers to use `installCommand` for the complete service and retain +installer-generated units for custom state/privilege profiles. Unit URL data is +quoted with literal percent specifiers; structural control characters fail +before token issuance. Existing module flags/scopes, optional-token omission, +normalised target and plain-HTTP policy are unchanged. No new endpoint, +credential authority, execution scope or trust exception is introduced. +`agent_install_command_shared_test.go`, `contract_test.go`, +`router_low_coverage_additional_test.go` and `security_regression_test.go` pin +the response, hosted-origin boundary, persisted scope and safe unit grammar. + ### Credential-safe Proxmox bootstrap (1 October 2026) The current Proxmox setup artifact supersedes the former token-bearing command/download contract: `command`, `commandWithEnv` and `commandWithoutEnv` are identical credential-free, single-line transports. `downloadURL` equals the tokenless `url`; downloads embed no setup token. `setupToken` (or agent-install `token`) is returned separately through the authenticated issuance response for an explicit reveal, never interpolated by the consumer. Settings uses the existing token-reveal dialog and tells users to run the command before pasting at its silent terminal prompt. Setup cache remains bound to endpoint/mode/live five-minute expiry and is discarded on modal close. The rendered scripts accept a bounded private `PULSE_SETUP_TOKEN_FILE` before mutation, unexport the secret, and put registration credentials in stdin, not process arguments. Legacy explicit query-token downloads remain accepted for compatibility, but no current artifact generates those URLs. Failed attempted registration returns nonzero and never echoes the response body. Runtime and root-installer consumers validate either the complete new artifact or the coherent old-server artifact; mixed transports fail closed. diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index ad199b6b40..8303de1c7e 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -956,6 +956,23 @@ tokens, and path-normalization variants. ## Current State +### Container diagnostics keeps copied material credential-free (1 October 2026) + +Container migration-token issuance returns its secret separately on the +existing authenticated route, with a no-store response. Copied install source, +download URLs and the diagnostic systemd reference must never contain that +secret, a raw token process argument or a secret-bearing environment directive. +The installer receives a bounded private file only after complete download and +preflight, from silent root/sudo terminal input; failure and handled signals +clean the handoff. The service reference points to the canonical default +Linux private state file and does not replace installer-owned profile policy. +URL percent specifiers are literal in the systemd environment, and structural +control characters are rejected before minting. Monitoring scopes, tenant +resolution, persistence rollback and HTTPS verification remain unchanged. +`TestSecurityContainerDiagnosticsRejectsUnitInjectionBeforeTokenIssuance` and +`TestSecurityContainerDiagnosticsServiceReferenceUsesPrivateState` exercise +that boundary, including real systemd grammar validation without installation. + ### Node TLS verification preference is operator-owned A PVE connection's `VerifySSL` value is security-relevant, so the stored diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 1f6e4df2cb..98e08f6225 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -2640,6 +2640,19 @@ vdev layout is reported` in ## Current State +### Container diagnostics shares private bootstrap transport (1 October 2026) + +Recovery-adjacent diagnostics now reuse the canonical complete installer +fetch/preflight and root/sudo private token-entry boundary, rather than exposing +credentials in a copied pipeline or reference service. Its existing token +response remains separate, no-store and durably bound to the unchanged +host-plus-Docker or workload-only scopes. The default-Linux service reference +uses a protected token path and instructs users to retain the actual +installer-generated unit for custom state and privilege profiles. Invalid +unit structure fails before issuance. This changes no storage permission, +retention, recovery operation or success condition; executable bootstrap and +response proof is not persistent-data upgrade/rollback acceptance. + ### Credential-safe Proxmox bootstrap (1 October 2026) The shared Proxmox setup boundary now receives its bootstrap credential from private terminal input via a bounded 0700/0600 file, before native setup mutation, rather than from copied shell literals or newly generated token-bearing download URLs. Existing registration grant binding, PVE/PBS monitoring scopes, and explicit non-rotating Audit/Repair remain unchanged. A private-file validation failure stops setup, and failed attempted registration exits nonzero with fixed guidance rather than echoed credential-bearing responses. This supersedes historical shared-boundary requirements for token-bearing commands and manual download URLs; it does not change storage permissions, recovery or retention. diff --git a/internal/api/agent_install_command_shared.go b/internal/api/agent_install_command_shared.go index 0a0a5d70f6..ad3d1763bd 100644 --- a/internal/api/agent_install_command_shared.go +++ b/internal/api/agent_install_command_shared.go @@ -118,26 +118,26 @@ func containerRuntimeAgentHostFlag(enableHost bool) string { } func buildContainerRuntimeAgentInstallCommand(baseURL string, token string, enableHost bool) string { - normalizedBaseURL := normalizeAgentInstallBaseURL(baseURL) - installScriptURL := normalizedBaseURL + "/install.sh" - command := fmt.Sprintf(`curl -fsSL %s | bash -s -- \ - --url %s \ - --enable-docker \ - %s \ - --interval 30s`, - posixShellQuote(installScriptURL), posixShellQuote(normalizedBaseURL), containerRuntimeAgentHostFlag(enableHost)) + return configapi.BuildContainerRuntimeAgentInstallCommand(baseURL, token, enableHost) +} - if trimmedToken := strings.TrimSpace(token); trimmedToken != "" { - command += fmt.Sprintf(` \ - --token %s`, posixShellQuote(trimmedToken)) +// The diagnostic reference must not compete with the installer's complete +// service renderer or expose the issued token. Its default Linux state path +// matches scripts/install.sh; custom/least-privilege profiles keep their +// installer-generated unit instead of replacing it with this reference. +func buildContainerRuntimeAgentServiceSnippet(baseURL string, enableHost bool) (string, error) { + baseURL = normalizeAgentInstallBaseURL(baseURL) + // Environment= does not expand shell variables, but systemd does expand % + // specifiers. Escape those as well as the unit's quoting/control boundary. + urlLine, err := quoteSystemdEnvironment("PULSE_URL", strings.ReplaceAll(baseURL, "%", "%%")) + if err != nil { + return "", err } - - if installBaseURLRequiresInsecure(normalizedBaseURL) { - command += ` \ - --insecure` + trustArgs := "" + if installBaseURLRequiresInsecure(baseURL) { + trustArgs = " --insecure" } - - return withPrivilegeEscalation(command) + return fmt.Sprintf("# Default Linux reference only. Run installCommand to create the private token file and complete service.\n# Keep the installer-generated unit for custom state directories or privilege profiles.\n[Service]\nType=simple\n%s\nExecStart=/usr/local/bin/pulse-agent --url ${PULSE_URL} --token-file /var/lib/pulse-agent/token --enable-docker %s --interval 30s%s\nRestart=always\nRestartSec=5s\nUser=root", urlLine, containerRuntimeAgentHostFlag(enableHost), trustArgs), nil } func buildSetupScriptCommand(scriptURL string, token string) string { diff --git a/internal/api/agent_install_command_shared_test.go b/internal/api/agent_install_command_shared_test.go index 5be1d54180..3c495430a5 100644 --- a/internal/api/agent_install_command_shared_test.go +++ b/internal/api/agent_install_command_shared_test.go @@ -207,14 +207,18 @@ func TestBuildContainerRuntimeAgentInstallCommand_UsesLifecycleTransport(t *test require.Contains(t, command, posixShellQuote("https://pulse.example.com/base/install.sh")) require.Contains(t, command, "https://pulse.example.com/base") - require.Contains(t, command, "--token "+posixShellQuote("token-123")) + require.NotContains(t, command, "token-123") + require.Contains(t, command, `--token-file "$token_file"`) require.Contains(t, command, "--enable-docker") require.Contains(t, command, "--enable-host") require.NotContains(t, command, "--enable-host=false") require.Contains(t, command, "--interval 30s") - require.Contains(t, command, `| { if [ "$(id -u)" -eq 0 ]; then bash -s --`) - require.Contains(t, command, `elif command -v sudo >/dev/null 2>&1; then sudo bash -s --`) - require.NotContains(t, command, "| sudo bash -s -- --url") + require.Contains(t, command, `sudo bash -c`) + require.Contains(t, command, "--preflight-only --output json --non-interactive") + require.Contains(t, command, `read -r -s -p`) + require.NotContains(t, command, "| bash") + require.NotContains(t, command, "| sudo") + require.NotContains(t, command, "\n") } func TestBuildContainerRuntimeAgentInstallCommand_OmitsTokenAndAddsInsecureForHTTP(t *testing.T) { diff --git a/internal/api/configapi/container_install_command_test.go b/internal/api/configapi/container_install_command_test.go new file mode 100644 index 0000000000..811801a77f --- /dev/null +++ b/internal/api/configapi/container_install_command_test.go @@ -0,0 +1,219 @@ +package configapi + +import ( + "encoding/json" + "encoding/pem" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestContainerBootstrapHistoryAndShellData(t *testing.T) { + root, env := bootstrapFixture(t, recordingInstaller) + marker := filepath.Join(root, "must-not-exist") + baseURL := `https://pulse.example/base/' ; touch ` + marker + ` ; #` + token := `synthetic-' ; touch ` + marker + ` ; #` + command := BuildContainerRuntimeAgentInstallCommand(baseURL, token, false) + history := filepath.Join(root, "shell-history") + payload, err := json.Marshal(map[string]string{"command": command, "input": token, "history": history}) + if err != nil { + t.Fatal(err) + } + cmd := exec.Command("python3", "-c", bootstrapPTYRunner) + cmd.Stdin = strings.NewReader(string(payload)) + cmd.Env = append(os.Environ(), env...) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("interactive container bootstrap: %v\n%s", err, out) + } + hist, err := os.ReadFile(history) + if err != nil || !strings.Contains(string(hist), command) { + t.Fatal("history control did not record the copied command") + } + args, err := os.ReadFile(filepath.Join(root, "argv")) + if err != nil || !strings.Contains(string(args), "--url\n"+baseURL+"\n") { + t.Fatal("shell quoting changed the installer destination") + } + if strings.Contains(string(hist), token) || strings.Contains(string(args), token) || strings.Contains(string(out), token) { + t.Fatal("private input reached history, argv or terminal output") + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatal("shell data was executed as source") + } + assertContainerBootstrapCleanup(t, root) +} + +func assertContainerBootstrapCleanup(t *testing.T, root string) { + t.Helper() + directories, err := os.ReadFile(filepath.Join(root, "private-directories")) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + for _, path := range strings.Fields(string(directories)) { + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatal("container bootstrap left a private handoff directory") + } + } +} + +func TestContainerBootstrapPrivateRootAndSudoModes(t *testing.T) { + for _, enableHost := range []bool{true, false} { + for _, uid := range []string{"0", "1000"} { + name := "workload-only" + if enableHost { + name = "host-and-workload" + } + t.Run(name+"_uid_"+uid, func(t *testing.T) { + root, env := bootstrapFixture(t, recordingInstaller) + env = append(env, "FAKE_ID_UID="+uid) + token := strings.Repeat("e", 32) + command := BuildContainerRuntimeAgentInstallCommand("https://pulse.example/base///", token, enableHost) + out, err := runBootstrap(t, command, token, env, true) + if err != nil { + t.Fatalf("container bootstrap: %v\n%s", err, out) + } + captured, err := os.ReadFile(filepath.Join(root, "captured-token")) + if err != nil || string(captured) != token { + t.Fatal("private handoff lost the issued token") + } + args, err := os.ReadFile(filepath.Join(root, "argv")) + if err != nil { + t.Fatal(err) + } + secretEnv, err := os.ReadFile(filepath.Join(root, "secret-env")) + if err != nil || string(secretEnv) != ":" || strings.Contains(string(out), token) || strings.Contains(string(args), token) || strings.Contains(command, token) { + t.Fatal("credential reached source, argv, environment or terminal output") + } + hostArg := "--enable-host=false\n" + if enableHost { + hostArg = "--enable-host\n" + } + for _, arg := range []string{"--url\nhttps://pulse.example/base\n", "--enable-docker\n", hostArg, "--interval\n30s\n", "--non-interactive\n", "--preflight-only\n"} { + if !strings.Contains(string(args), arg) { + t.Fatalf("installer lost required argument %q", arg) + } + } + if strings.Contains(string(args), "--enable-commands") || strings.Contains(string(args), "--insecure") { + t.Fatal("container monitoring silently broadened execution or TLS policy") + } + assertContainerBootstrapCleanup(t, root) + }) + } + } +} + +func TestContainerBootstrapFailuresStopAndClean(t *testing.T) { + for _, tc := range []struct { + name, env, input string + tty bool + }{ + {"no-terminal", "", "", false}, + {"blank-token", "", "", true}, + {"oversized-token", "", strings.Repeat("x", 4097), true}, + {"interrupt-input", "", "\x03", true}, + {"partial-download", "FAKE_CURL_EXIT=22", "", false}, + {"preflight-failure", "FAKE_PREFLIGHT_EXIT=44", "", false}, + {"sudo-refusal", "FAKE_SUDO_EXIT=1", "", false}, + {"install-failure", "FAKE_INSTALL_EXIT=9", strings.Repeat("f", 32), true}, + {"terminate", "FAKE_INSTALL_SIGNAL=TERM", strings.Repeat("f", 32), true}, + {"hangup", "FAKE_INSTALL_SIGNAL=HUP", strings.Repeat("f", 32), true}, + } { + t.Run(tc.name, func(t *testing.T) { + installer := strings.Replace(recordingInstaller, `exit "${FAKE_INSTALL_EXIT:-0}"`, `if [ -n "${FAKE_INSTALL_SIGNAL:-}" ]; then kill -s "$FAKE_INSTALL_SIGNAL" "$PPID"; fi +exit "${FAKE_INSTALL_EXIT:-0}"`, 1) + root, env := bootstrapFixture(t, installer) + env = append(env, tc.env) + if tc.name == "sudo-refusal" { + env = append(env, "FAKE_ID_UID=1000") + } + command := BuildContainerRuntimeAgentInstallCommand("https://pulse.example", "synthetic-not-in-command", true) + out, err := runBootstrap(t, command, tc.input, env, tc.tty) + if err == nil { + t.Fatal("failed or interrupted bootstrap appeared successful") + } + if tc.input != "" && tc.input != "\x03" && strings.Contains(string(out), tc.input) { + t.Fatal("token echoed on failure") + } + if tc.name != "install-failure" && tc.name != "terminate" && tc.name != "hangup" { + if _, err := os.Stat(filepath.Join(root, "captured-token")); !os.IsNotExist(err) { + t.Fatal("installer ran before its prerequisite succeeded") + } + } + assertContainerBootstrapCleanup(t, root) + }) + } +} + +func TestContainerBootstrapOptionalAuthNeedsNoTerminal(t *testing.T) { + root, env := bootstrapFixture(t, `#!/bin/bash +set -eu +printf '%s\n' "$@" >> "$FAKE_ROOT/argv" +[[ " $* " != *" --token"* ]] +if [[ " $* " != *" --preflight-only "* ]]; then touch "$FAKE_ROOT/installed"; fi +`) + command := BuildContainerRuntimeAgentInstallCommand("http://pulse.example:7655/", "", false) + out, err := runBootstrap(t, command, "", env, false) + if err != nil { + t.Fatalf("optional-auth bootstrap: %v\n%s", err, out) + } + if strings.Contains(command, "read -r") || !strings.Contains(command, "--insecure") || strings.Contains(command, "-kfs") { + t.Fatal("optional auth changed credential or HTTP trust policy") + } + if _, err := os.Stat(filepath.Join(root, "installed")); err != nil { + t.Fatal("token-optional installer did not run") + } + assertContainerBootstrapCleanup(t, root) +} + +// Exercise real curl/TLS download and the executable private handoff, not a +// system installation or a real Docker/Podman estate. The installer is modeled. +func TestContainerBootstrapTLSDownload(t *testing.T) { + curlPath, err := exec.LookPath("curl") + if err != nil { + t.Fatal("curl required for transport proof") + } + for _, status := range []int{200, 401, 403, 503} { + t.Run(http.StatusText(status), func(t *testing.T) { + root, env := bootstrapFixture(t, recordingInstaller) + token := strings.Repeat("a", 32) + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/install.sh" || r.URL.RawQuery != "" || r.Header.Get("X-API-Token") != "" || r.Header.Get("Authorization") != "" { + t.Error("download carried credentials or an unexpected target") + } + w.WriteHeader(status) + w.Write([]byte(recordingInstaller)) + })) + defer server.Close() + writeExecutable(t, filepath.Join(root, "bin", "curl"), "#!/bin/sh\nprintf '%s\\n' \"$@\" >> \"$FAKE_ROOT/argv\"\nexec "+posixShellQuote(curlPath)+" \"$@\"\n") + command := BuildContainerRuntimeAgentInstallCommand(server.URL, token, true) + if status == 200 { + if _, err := runBootstrap(t, command, "", env, false); err == nil { + t.Fatal("untrusted TLS was accepted") + } + } + ca := filepath.Join(root, "ca.pem") + if err := os.WriteFile(ca, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: server.Certificate().Raw}), 0600); err != nil { + t.Fatal(err) + } + env = append(env, "CURL_CA_BUNDLE="+ca) + out, err := runBootstrap(t, command, token, env, true) + if (status == 200) != (err == nil) { + t.Fatalf("trusted TLS download status %d: %v\n%s", status, err, out) + } + args, _ := os.ReadFile(filepath.Join(root, "argv")) + if strings.Contains(string(out), token) || strings.Contains(string(args), token) { + t.Fatal("transport exposed the credential") + } + if status != 200 { + if _, err := os.Stat(filepath.Join(root, "captured-token")); !os.IsNotExist(err) { + t.Fatal("installer ran after a failed download") + } + } + assertContainerBootstrapCleanup(t, root) + }) + } +} diff --git a/internal/api/configapi/install_command.go b/internal/api/configapi/install_command.go index 49c48a1831..0293eef748 100644 --- a/internal/api/configapi/install_command.go +++ b/internal/api/configapi/install_command.go @@ -39,10 +39,38 @@ func BuildProxmoxAgentInstallCommand(opts AgentInstallCommandOptions) string { args += ` --token-file "$token_file"` } args += trustArgs + " --non-interactive" + return privateAgentBootstrapCommand(baseURL, curlFlags, trustArgs, args, strings.TrimSpace(opts.Token) != "") +} + +// BuildContainerRuntimeAgentInstallCommand uses the same complete-download, +// preflight and private credential-entry boundary as the Proxmox installer. +// Token selects the prompt; its value is never included in the copied command. +func BuildContainerRuntimeAgentInstallCommand(baseURL, token string, enableHost bool) string { + baseURL = strings.TrimRight(strings.TrimSpace(baseURL), "/") + args := " --url " + posixShellQuote(baseURL) + " --enable-docker" + if enableHost { + args += " --enable-host" + } else { + args += " --enable-host=false" + } + args += " --interval 30s" + needsToken := strings.TrimSpace(token) != "" + if needsToken { + args += ` --token-file "$token_file"` + } + trustArgs := "" + if strings.HasPrefix(strings.ToLower(baseURL), "http://") { + trustArgs = " --insecure" + } + args += trustArgs + " --non-interactive" + return privateAgentBootstrapCommand(baseURL, "-fsSL", trustArgs, args, needsToken) +} + +func privateAgentBootstrapCommand(baseURL, curlFlags, trustArgs, args string, needsToken bool) string { preflight := "bash \"$install_script\" --url " + posixShellQuote(baseURL) + " --preflight-only --output json --non-interactive" + trustArgs + ";" return privateBootstrapCommand(baseURL+"/install.sh", curlFlags, preflight, - "bash \"$1\""+args+";", strings.TrimSpace(opts.Token) != "", "Pulse agent token") + "bash \"$1\""+args+";", needsToken, "Pulse agent token") } // privateBootstrapCommand uses POSIX outer grammar for single-line paste hosts. diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index e0b98e9c25..4080117b6d 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -6875,11 +6875,11 @@ func TestContract_DiagnosticsDockerPrepareTokenInstallCommandUsesLifecycleTransp if strings.Contains(got, "--disable-host") { t.Fatalf("install command preserved stale disable-host flag: %s", got) } - if !strings.Contains(got, `| { if [ "$(id -u)" -eq 0 ]; then bash -s --`) { - t.Fatalf("install command missing governed root-or-sudo wrapper: %s", got) + if !strings.Contains(got, `sudo bash -c`) || !strings.Contains(got, `--token-file "$token_file"`) || !strings.Contains(got, "--preflight-only") { + t.Fatal("install command missing private-token/preflight root-or-sudo boundary") } - if strings.Contains(got, "curl -fsSL "+posixShellQuote(baseURL+"/install.sh")+" | sudo bash -s --") { - t.Fatalf("install command preserved raw sudo pipe instead of governed wrapper: %s", got) + if strings.Contains(got, "token-123") || strings.Contains(got, "| bash") || strings.Contains(got, "| sudo") { + t.Fatal("install command exposed a credential or executes a partial fetch") } } @@ -23056,8 +23056,8 @@ func TestContract_RequestOriginCannotRetargetTokenBearingCommands(t *testing.T) baseURL := newRouter().resolvePublicURL(req) diagnosticsCommand := buildContainerRuntimeAgentInstallCommand(baseURL, "diagnostics-secret", true) - if !strings.Contains(diagnosticsCommand, autoDetectedURL) || !strings.Contains(diagnosticsCommand, "diagnostics-secret") { - t.Fatalf("diagnostics command did not retain safe target and token: %q", diagnosticsCommand) + if !strings.Contains(diagnosticsCommand, autoDetectedURL) || !strings.Contains(diagnosticsCommand, `--token-file "$token_file"`) || strings.Contains(diagnosticsCommand, "diagnostics-secret") { + t.Fatal("diagnostics command did not retain the safe target and separate credential-entry boundary") } if strings.Contains(diagnosticsCommand, "attacker") { t.Fatalf("diagnostics command retained attacker-controlled origin bytes: %q", diagnosticsCommand) diff --git a/internal/api/router.go b/internal/api/router.go index 1bdba5ae80..f4a4bfb22a 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -7957,6 +7957,12 @@ func (r *Router) handleDiagnosticsDockerPrepareToken(w http.ResponseWriter, req return } + systemdSnippet, err := buildContainerRuntimeAgentServiceSnippet(baseURL, enableHost) + if err != nil { + writeConfigAgentInstallBaseURLUnavailable(w) + return + } + rawToken, err := auth.GenerateAPIToken() if err != nil { log.Error().Err(err).Msg("Failed to generate container runtime migration token") @@ -8018,7 +8024,6 @@ func (r *Router) handleDiagnosticsDockerPrepareToken(w http.ResponseWriter, req config.Mu.Unlock() installCommand := buildContainerRuntimeAgentInstallCommand(baseURL, rawToken, enableHost) - systemdSnippet := fmt.Sprintf("[Service]\nType=simple\nEnvironment=\"PULSE_URL=%s\"\nEnvironment=\"PULSE_TOKEN=%s\"\nExecStart=/usr/local/bin/pulse-agent --url %s --token %s --enable-docker %s --interval 30s\nRestart=always\nRestartSec=5s\nUser=root", baseURL, rawToken, baseURL, rawToken, containerRuntimeAgentHostFlag(enableHost)) response := map[string]any{ "success": true, @@ -8034,6 +8039,7 @@ func (r *Router) handleDiagnosticsDockerPrepareToken(w http.ResponseWriter, req "enableHost": enableHost, } + w.Header().Set("Cache-Control", "no-store") if err := utils.WriteJSONResponse(w, response); err != nil { log.Error().Err(err).Msg("Failed to serialize container runtime token migration response") } diff --git a/internal/api/router_low_coverage_additional_test.go b/internal/api/router_low_coverage_additional_test.go index a264481de7..3e6a8ce694 100644 --- a/internal/api/router_low_coverage_additional_test.go +++ b/internal/api/router_low_coverage_additional_test.go @@ -179,11 +179,19 @@ func TestHandleDiagnosticsDockerPrepareToken_Success(t *testing.T) { if !strings.Contains(payload["installCommand"].(string), "--enable-docker") { t.Fatalf("expected install command to enable docker metrics: %q", payload["installCommand"]) } - if !strings.Contains(payload["installCommand"].(string), `| { if [ "$(id -u)" -eq 0 ]; then bash -s --`) { - t.Fatalf("expected install command to use lifecycle privilege wrapper: %q", payload["installCommand"]) + if !strings.Contains(payload["installCommand"].(string), `sudo bash -c`) || !strings.Contains(payload["installCommand"].(string), `--token-file "$token_file"`) { + t.Fatal("expected install command to preserve private token input through root or sudo") } - if strings.Contains(payload["installCommand"].(string), "| sudo bash -s -- --url") { - t.Fatalf("expected install command to preserve the governed root-or-sudo wrapper instead of a raw sudo pipe: %q", payload["installCommand"]) + for _, field := range []string{"installCommand", "systemdServiceSnippet"} { + if strings.Contains(payload[field].(string), payload["token"].(string)) || strings.Contains(payload[field].(string), "PULSE_TOKEN=") || strings.Contains(payload[field].(string), "--token ") { + t.Fatalf("credential leaked into copied %s", field) + } + } + if rec.Header().Get("Cache-Control") != "no-store" { + t.Fatal("issued credential response can be cached") + } + if !strings.Contains(payload["systemdServiceSnippet"].(string), "--token-file /var/lib/pulse-agent/token") { + t.Fatal("reference snippet does not use the installer's default private token path") } if !strings.Contains(payload["systemdServiceSnippet"].(string), "--enable-host") || strings.Contains(payload["systemdServiceSnippet"].(string), "--enable-host=false") { diff --git a/internal/api/security_regression_test.go b/internal/api/security_regression_test.go index 15a5d4c5f4..4282af41c2 100644 --- a/internal/api/security_regression_test.go +++ b/internal/api/security_regression_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" "strconv" "strings" @@ -27,6 +28,58 @@ import ( "github.com/rcourtman/pulse-go-rewrite/pkg/auth" ) +func TestSecurityContainerDiagnosticsRejectsUnitInjectionBeforeTokenIssuance(t *testing.T) { + for _, url := range []string{"https://pulse.example\nEnvironment=INJECTED=true", "https://pulse.example/\rpath", "https://pulse.example/\x00"} { + t.Run(strconv.Quote(url), func(t *testing.T) { + monitor, state, _ := newTestMonitor(t) + state.DockerHosts = []models.DockerHost{{ID: "host-1"}} + cfg := &config.Config{PublicURL: url} + router := &Router{monitor: monitor, config: cfg} + req := httptest.NewRequest(http.MethodPost, "/api/diagnostics/docker/prepare-token", strings.NewReader(`{"agentId":"host-1"}`)) + rec := httptest.NewRecorder() + router.handleDiagnosticsDockerPrepareToken(rec, req) + if rec.Code != http.StatusServiceUnavailable || len(cfg.APITokens) != 0 { + t.Fatal("unsafe diagnostic unit target minted a token or produced a usable response") + } + }) + } +} + +func TestSecurityContainerDiagnosticsServiceReferenceUsesPrivateState(t *testing.T) { + for _, enableHost := range []bool{true, false} { + for _, baseURL := range []string{"https://pulse.example/base", "http://pulse.example:7655", `https://pulse.example/path%20space/%n/$VALUE/"quoted"`} { + snippet, err := buildContainerRuntimeAgentServiceSnippet(baseURL+"/", enableHost) + if err != nil { + t.Fatal(err) + } + urlLine := "Environment=" + strconv.Quote("PULSE_URL="+strings.ReplaceAll(baseURL, "%", "%%")) + if !strings.Contains(snippet, urlLine) || !strings.Contains(snippet, "--url ${PULSE_URL}") || !strings.Contains(snippet, "--token-file /var/lib/pulse-agent/token") { + t.Fatal("service reference changed URL data or lost the private credential path") + } + if strings.Contains(snippet, "PULSE_TOKEN=") || strings.Contains(snippet, "--token ") || strings.Contains(snippet, "--enable-commands") { + t.Fatal("service reference exposes a credential or broadens command authority") + } + if !strings.Contains(snippet, containerRuntimeAgentHostFlag(enableHost)+" --interval") { + t.Fatal("service reference lost its explicit host-monitoring mode") + } + if strings.Contains(snippet, "--insecure") != strings.HasPrefix(baseURL, "http://") { + t.Fatal("service reference changed the transport trust policy") + } + // Validate real systemd unit grammar, without installing a service or + // pretending the diagnostic reference is the complete governed unit. + unit := filepath.Join(t.TempDir(), "container-reference.service") + body := strings.Replace(snippet, "/usr/local/bin/pulse-agent", "/bin/true", 1) + if err := os.WriteFile(unit, []byte(body+"\n"), 0600); err != nil { + t.Fatal(err) + } + out, err := exec.Command("systemd-analyze", "verify", "--man=no", unit).CombinedOutput() + if err != nil { + t.Fatalf("systemd reference syntax: %v\n%s", err, out) + } + } + } +} + type wsRawMessage struct { Type agentexec.MessageType `json:"type"` Payload json.RawMessage `json:"payload,omitempty"` From 413d5ae7ea4838957a70237fc023bd614d8dc6dc Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 10:04:11 +0100 Subject: [PATCH 2/2] Bound private token input without terminal truncation The executable overflow regression and a bounded Bash PTY probe show canonical terminals truncate long input before the size check. Read a bounded character count with echo held off, drain an invalid line before returning to the calling shell, and restore its modes on every exit. Pin maximum-size preservation, overflow history safety and terminal restoration. Contract-Neutral: Repair bounded private terminal entry, overflow draining and mode restoration; no issuance, scope, install transport or response-shape delta. Change-source: pulse-maintainer --- .../container_install_command_test.go | 46 +++++++++++++++++++ internal/api/configapi/install_command.go | 13 +++++- .../api/configapi/install_command_test.go | 8 ++++ 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/internal/api/configapi/container_install_command_test.go b/internal/api/configapi/container_install_command_test.go index 811801a77f..76ab4b1917 100644 --- a/internal/api/configapi/container_install_command_test.go +++ b/internal/api/configapi/container_install_command_test.go @@ -47,6 +47,52 @@ func TestContainerBootstrapHistoryAndShellData(t *testing.T) { assertContainerBootstrapCleanup(t, root) } +func TestContainerBootstrapOverflowDoesNotBecomeShellHistory(t *testing.T) { + root, env := bootstrapFixture(t, recordingInstaller) + token := strings.Repeat("x", 10000) + "-OVERFLOW_HISTORY_MARKER" + command := BuildContainerRuntimeAgentInstallCommand("https://pulse.example", "separate-issued-token", true) + history := filepath.Join(root, "shell-history") + payload, err := json.Marshal(map[string]string{"command": command, "input": token, "history": history}) + if err != nil { + t.Fatal(err) + } + cmd := exec.Command("python3", "-c", bootstrapPTYRunner) + cmd.Stdin = strings.NewReader(string(payload)) + cmd.Env = append(os.Environ(), env...) + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("overflowed input was accepted") + } + hist, err := os.ReadFile(history) + if err != nil || !strings.Contains(string(hist), command) { + t.Fatal("history control did not record the copied command") + } + for _, output := range []string{string(out), string(hist)} { + if strings.Contains(output, "OVERFLOW_HISTORY_MARKER") || strings.Contains(output, strings.Repeat("x", 512)) { + t.Fatal("discarded credential input was echoed or returned to shell history") + } + } + if _, err := os.Stat(filepath.Join(root, "captured-token")); !os.IsNotExist(err) { + t.Fatal("installer ran after overflowed input") + } + assertContainerBootstrapCleanup(t, root) +} + +func TestContainerBootstrapPreservesMaximumAcceptedInput(t *testing.T) { + root, env := bootstrapFixture(t, recordingInstaller) + token := strings.Repeat("a", 4096) + command := BuildContainerRuntimeAgentInstallCommand("https://pulse.example", "separate-issued-token", true) + out, err := runBootstrap(t, command, token, env, true) + if err != nil { + t.Fatalf("maximum-size private input: %v\n%s", err, out) + } + captured, err := os.ReadFile(filepath.Join(root, "captured-token")) + if err != nil || string(captured) != token { + t.Fatal("terminal silently truncated an accepted token") + } + assertContainerBootstrapCleanup(t, root) +} + func assertContainerBootstrapCleanup(t *testing.T, root string) { t.Helper() directories, err := os.ReadFile(filepath.Join(root, "private-directories")) diff --git a/internal/api/configapi/install_command.go b/internal/api/configapi/install_command.go index 0293eef748..8c514f782e 100644 --- a/internal/api/configapi/install_command.go +++ b/internal/api/configapi/install_command.go @@ -83,9 +83,18 @@ func privateBootstrapCommand(scriptURL, curlFlags, preflight, run string, needsT privileged = append(privileged, `token_dir=$(mktemp -d /tmp/pulse-agent-bootstrap.XXXXXX);`, `token_file="$token_dir/token";`, - `cleanup() { unset pulse_token; rm -f -- "$token_file"; rmdir -- "$token_dir"; };`, + `unset pulse_token pulse_discard tty_state;`, + `cleanup() { unset pulse_token pulse_discard; if [ -n "${tty_state:-}" ]; then stty "$tty_state" /dev/null || :; fi; rm -f -- "$token_file"; rmdir -- "$token_dir"; };`, `trap cleanup EXIT; trap 'exit 129' HUP; trap 'exit 130' INT; trap 'exit 143' TERM;`, - `if ! IFS= read -r -s -p `+posixShellQuote(prompt+" (paste at this prompt, not in the command): ")+` pulse_token &2; exit 1; fi;`, + // A normal canonical tty silently truncates long lines (4095 bytes on + // Linux). Bounded character input preserves valid input and detects + // overflow; keep echo off while draining the rest of an invalid line + // so it cannot become a second command in the caller's shell history. + `if ! tty_state=$(stty -g &2; exit 1; fi;`, + `stty -echo &2; exit 1; fi;`, + `if [ "${#pulse_token}" -gt 4096 ]; then while IFS= read -r -s -n 4097 pulse_discard /dev/tty;`, `if [ -z "$pulse_token" ] || [ "${#pulse_token}" -gt 4096 ]; then echo "A non-empty token of at most 4096 characters is required." >&2; exit 1; fi;`, `printf %s "$pulse_token" > "$token_file"; unset pulse_token;`, diff --git a/internal/api/configapi/install_command_test.go b/internal/api/configapi/install_command_test.go index b35d089dd9..caf1c13311 100644 --- a/internal/api/configapi/install_command_test.go +++ b/internal/api/configapi/install_command_test.go @@ -16,6 +16,8 @@ const bootstrapPTYRunner = ` import errno, fcntl, json, os, pty, select, signal, subprocess, sys, termios, time p = json.load(sys.stdin) master, slave = pty.openpty() +tty_watch = os.dup(slave) +initial_tty = termios.tcgetattr(tty_watch) def session(): os.setsid() fcntl.ioctl(0, termios.TIOCSCTTY, 0) @@ -48,6 +50,12 @@ else: os.killpg(child.pid, signal.SIGKILL) output += b"\nPTY fixture deadline expired\n" code = child.wait() +final_tty = termios.tcgetattr(tty_watch) +mode_mask = termios.ECHO | termios.ICANON +if (initial_tty[3] & mode_mask) != (final_tty[3] & mode_mask): + output += b"\nBootstrap did not restore terminal echo/canonical mode\n" + code = 1 +os.close(tty_watch) os.close(master) sys.stdout.buffer.write(output) sys.exit(code if code >= 0 else 128 - code)