From acc86c7aca5d035d4ead8cd6359db0dfb0d942ec Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 12 Oct 2026 05:03:18 +0100 Subject: [PATCH] Retain existing state when startup import is configured Require a blank destination instead of using nodes.enc as a first-run sentinel. Recheck before writes, retain unknown or unreadable state, and leave deliberate authenticated transfer unchanged. Keep deployment .env provisioning and encrypted file precedence; explain the separate archive path in canonical and served help. Change-source: pulse-maintainer --- cmd/pulse/auto_import_test.go | 8 +-- docs/CONFIGURATION.md | 20 ++++-- docs/MIGRATION.md | 7 +- .../internal/subsystems/security-privacy.md | 21 ++++++ frontend-modern/public/docs/CONFIGURATION.md | 20 ++++-- frontend-modern/public/docs/MIGRATION.md | 7 +- pkg/server/auto_import_destination_test.go | 13 +++- pkg/server/server.go | 70 +++++++++++++++++-- pkg/server/service_health_test.go | 9 +++ scripts/tests/test_bootstrap_import_docs.py | 17 +++-- 10 files changed, 158 insertions(+), 34 deletions(-) diff --git a/cmd/pulse/auto_import_test.go b/cmd/pulse/auto_import_test.go index 2c0c25d121..e0f9113895 100644 --- a/cmd/pulse/auto_import_test.go +++ b/cmd/pulse/auto_import_test.go @@ -40,7 +40,7 @@ func TestPerformAutoImport_ValidPayload(t *testing.T) { t.Setenv("PULSE_DATA_DIR", dir) pass := "test-passphrase" - cp := config.NewConfigPersistence(dir) + cp := config.NewConfigPersistence(t.TempDir()) exported, err := cp.ExportConfig(pass) if err != nil { t.Fatalf("ExportConfig: %v", err) @@ -88,13 +88,13 @@ func TestPerformAutoImport_File(t *testing.T) { t.Setenv("PULSE_DATA_DIR", dir) pass := "test-passphrase" - cp := config.NewConfigPersistence(dir) + cp := config.NewConfigPersistence(t.TempDir()) exported, err := cp.ExportConfig(pass) if err != nil { t.Fatalf("ExportConfig: %v", err) } - importFile := filepath.Join(dir, "import.enc") + importFile := filepath.Join(t.TempDir(), "import.enc") if err := os.WriteFile(importFile, []byte(exported), 0600); err != nil { t.Fatalf("WriteFile: %v", err) } @@ -135,7 +135,7 @@ func TestPerformAutoImport_FileNormalizeError(t *testing.T) { t.Setenv("PULSE_DATA_DIR", dir) t.Setenv("PULSE_INIT_CONFIG_PASSPHRASE", "pass") - importFile := filepath.Join(dir, "empty.enc") + importFile := filepath.Join(t.TempDir(), "empty.enc") if err := os.WriteFile(importFile, []byte(" "), 0600); err != nil { t.Fatalf("write import file: %v", err) } diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index a046044993..aa362722ce 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -510,12 +510,16 @@ not resetting an existing installation. It uses the same encrypted Recovery settings form, not a full-state backup. Included settings and API-token records are **replaced, not merged**; imported SSO settings can also change access. -The startup gate checks for `nodes.enc` in the effective data directory -(`PULSE_DATA_DIR`, or the deployment default). If that file exists, startup -skips import. **This is a file-presence check, not a record of the first run.** -Other existing settings, API-token records or session files alone do not prevent -an import attempt. Do not delete `nodes.enc`, change the data path or remove -credentials to force one; resolve an unexpected path or access failure first. +The startup gate requires an **empty data directory** (`PULSE_DATA_DIR`, or +the deployment default), or a destination that does not yet exist. A regular +`.env` file for destination-local deployment overrides is the only exception. +Existing settings, API-token records, sessions, encryption keys, organisation +directories and unknown contents all prevent startup import, even without +`nodes.enc`. Unreadable directories and symbolic links are not treated as empty. +The importer checks the destination again before writing, but this is **not a +lock against another running Pulse process**. Do not delete `nodes.enc` or +other state, change the data path or remove credentials to force an import; +resolve an unexpected path or access failure first. For an existing installation, use the [reviewed transfer procedure](MIGRATION.md#transfer-the-included-configuration) instead, with a private backup of the destination before replacing settings. @@ -529,6 +533,10 @@ instead, with a private backup of the destination before replacing settings. An unreadable or invalid file fails the attempt; it does **not** fall back to the data variable. `PULSE_INIT_CONFIG_URL` is not supported by server startup; the separate hidden `pulse config auto-import` command has a different path. +Keep the input archive **outside the destination data directory**. An archive +placed inside it also makes the destination nonempty; startup skips import +and retains those contents. Use a separate private file or read-only mount, +not deletion of useful destination state. Keep the export owner-only and its passphrase in the deployment's private credential source, separate from the backup. Do not put a passphrase or bundle diff --git a/docs/MIGRATION.md b/docs/MIGRATION.md index eb0c8e1fc4..07c7cc50b6 100644 --- a/docs/MIGRATION.md +++ b/docs/MIGRATION.md @@ -35,9 +35,10 @@ SSO configuration **is included**, even though local login credentials are not. Automated provisioning uses the same limited scope. Read the [startup-import gate and safeguards](CONFIGURATION.md#auto-import-bootstrap) -before setting bootstrap variables: an existing installation without -`nodes.enc` is not protected by a first-run marker, and normal startup after -an import error does not confirm a successful transfer. +before setting bootstrap variables: startup import requires an empty destination +data directory (apart from a regular deployment `.env`), even without +`nodes.enc`. Keep its input archive outside that directory. Normal startup after +an import error or skipped import does not confirm a successful transfer. ### Full-state recovery diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index 32fd680def..c73164c8d2 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -18,6 +18,27 @@ ## Purpose +### Startup provisioning cannot replace retained access + +`pkg/server/server.go` admits environment-selected startup import only into an +absent or empty resolved data directory; a regular deployment `.env` is the +only allowed pre-start entry. Existing non-Proxmox settings, token/session +records, encryption keys, tenant directories and unknown contents all block it. +Directory inspection errors, non-directories and symlinks are not absence. +Admission reads no file contents and creates no persistence/key. The importer +rechecks before creating persistence or writing and audits a classified refusal +without a path, archive or passphrase. This is not cross-process exclusion: +provision an isolated destination and keep the source archive outside it. + +`TestStartupImportRetainsNonProxmoxAccess` exercises ordinary configuration +loading, retained agent-token validation and session/key bytes through both +production startup helpers. It also proves that deliberate `ImportConfig` +transfer still replaces the included records while keeping excluded local state. +`auto_import_destination_test.go` covers blank provisioning, `.env`, all +observed/unknown destination types, uncertain access, pre-write recheck and +file precedence/refusal. Encrypted export scope and authenticated Recovery +authorisation are unchanged; source proof is not installed transfer acceptance. + ### Explicit agent credential-file failure does not borrow authority The `cmd/pulse-agent/main.go` flag loader keeps `PULSE_TOKEN` separate from an diff --git a/frontend-modern/public/docs/CONFIGURATION.md b/frontend-modern/public/docs/CONFIGURATION.md index a046044993..aa362722ce 100644 --- a/frontend-modern/public/docs/CONFIGURATION.md +++ b/frontend-modern/public/docs/CONFIGURATION.md @@ -510,12 +510,16 @@ not resetting an existing installation. It uses the same encrypted Recovery settings form, not a full-state backup. Included settings and API-token records are **replaced, not merged**; imported SSO settings can also change access. -The startup gate checks for `nodes.enc` in the effective data directory -(`PULSE_DATA_DIR`, or the deployment default). If that file exists, startup -skips import. **This is a file-presence check, not a record of the first run.** -Other existing settings, API-token records or session files alone do not prevent -an import attempt. Do not delete `nodes.enc`, change the data path or remove -credentials to force one; resolve an unexpected path or access failure first. +The startup gate requires an **empty data directory** (`PULSE_DATA_DIR`, or +the deployment default), or a destination that does not yet exist. A regular +`.env` file for destination-local deployment overrides is the only exception. +Existing settings, API-token records, sessions, encryption keys, organisation +directories and unknown contents all prevent startup import, even without +`nodes.enc`. Unreadable directories and symbolic links are not treated as empty. +The importer checks the destination again before writing, but this is **not a +lock against another running Pulse process**. Do not delete `nodes.enc` or +other state, change the data path or remove credentials to force an import; +resolve an unexpected path or access failure first. For an existing installation, use the [reviewed transfer procedure](MIGRATION.md#transfer-the-included-configuration) instead, with a private backup of the destination before replacing settings. @@ -529,6 +533,10 @@ instead, with a private backup of the destination before replacing settings. An unreadable or invalid file fails the attempt; it does **not** fall back to the data variable. `PULSE_INIT_CONFIG_URL` is not supported by server startup; the separate hidden `pulse config auto-import` command has a different path. +Keep the input archive **outside the destination data directory**. An archive +placed inside it also makes the destination nonempty; startup skips import +and retains those contents. Use a separate private file or read-only mount, +not deletion of useful destination state. Keep the export owner-only and its passphrase in the deployment's private credential source, separate from the backup. Do not put a passphrase or bundle diff --git a/frontend-modern/public/docs/MIGRATION.md b/frontend-modern/public/docs/MIGRATION.md index eb0c8e1fc4..07c7cc50b6 100644 --- a/frontend-modern/public/docs/MIGRATION.md +++ b/frontend-modern/public/docs/MIGRATION.md @@ -35,9 +35,10 @@ SSO configuration **is included**, even though local login credentials are not. Automated provisioning uses the same limited scope. Read the [startup-import gate and safeguards](CONFIGURATION.md#auto-import-bootstrap) -before setting bootstrap variables: an existing installation without -`nodes.enc` is not protected by a first-run marker, and normal startup after -an import error does not confirm a successful transfer. +before setting bootstrap variables: startup import requires an empty destination +data directory (apart from a regular deployment `.env`), even without +`nodes.enc`. Keep its input archive outside that directory. Normal startup after +an import error or skipped import does not confirm a successful transfer. ### Full-state recovery diff --git a/pkg/server/auto_import_destination_test.go b/pkg/server/auto_import_destination_test.go index 1f77213a62..587d77f247 100644 --- a/pkg/server/auto_import_destination_test.go +++ b/pkg/server/auto_import_destination_test.go @@ -1,6 +1,7 @@ package server import ( + "encoding/base64" "fmt" "io/fs" "os" @@ -96,7 +97,13 @@ func TestAutoImportRetainsDestinationState(t *testing.T) { t.Fatal(err) } // Inert sentinels, including .db: no database or executable is opened. - if err := os.WriteFile(path, []byte("synthetic-retained-state"), 0600); err != nil { + state := []byte("synthetic-retained-state") + if name == ".encryption.key" { + // A valid synthetic key keeps the adverse control about admission, + // rather than terminating in the constructor's key validation. + state = []byte(base64.StdEncoding.EncodeToString(make([]byte, 32))) + } + if err := os.WriteFile(path, state, 0600); err != nil { t.Fatal(err) } setAutoImportInputs(t, dir, payload, passphrase) @@ -253,6 +260,10 @@ func TestAutoImportRejectsUncertainDestination(t *testing.T) { t.Setenv("PULSE_DATA_DIR", dir) if ShouldAutoImport() { t.Error("unknown or nonempty destination was admitted") + // The unchanged constructor can terminate the process on an + // invalid directory. The gate failure already proves that fault; + // exercise the direct refusal only after safe admission exists. + return } if err := PerformAutoImport(); err == nil { t.Error("unknown or nonempty destination allowed direct startup import") diff --git a/pkg/server/server.go b/pkg/server/server.go index 1e56e135aa..1e4a00d868 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -5,7 +5,9 @@ import ( "crypto/subtle" "crypto/tls" "encoding/base64" + "errors" "fmt" + "io" "net" "net/http" "os" @@ -166,7 +168,7 @@ func Run(ctx context.Context, version string) (runErr error) { return err } - // Check for auto-import on first startup + // Bootstrap only a blank destination, before ordinary loading creates state. if ShouldAutoImport() { if err := PerformAutoImport(); err != nil { log.Error().Err(err).Msg("Auto-import failed, continuing with normal startup") @@ -1070,14 +1072,59 @@ func startMetricsServer(ctx context.Context, addr string, metricsToken string, a } func ShouldAutoImport() bool { - configPath := config.ResolveRuntimeDataDir("") - - if _, err := os.Stat(filepath.Join(configPath, "nodes.enc")); err == nil { + if os.Getenv("PULSE_INIT_CONFIG_DATA") == "" && os.Getenv("PULSE_INIT_CONFIG_FILE") == "" { return false } - return os.Getenv("PULSE_INIT_CONFIG_DATA") != "" || - os.Getenv("PULSE_INIT_CONFIG_FILE") != "" + if err := checkAutoImportDestination(config.ResolveRuntimeDataDir("")); err != nil { + log.Warn().Err(err).Msg("Startup auto-import skipped; destination state is retained") + return false + } + return true +} + +var ( + errAutoImportDestinationNotEmpty = errors.New("startup import requires an empty data directory; use authenticated Recovery for an existing installation") + errAutoImportDestinationUnknown = errors.New("cannot inspect the startup import destination; resolve its path and access before importing") +) + +// checkAutoImportDestination must not construct persistence: doing so creates +// an encryption key before admission. nodes.enc is not a first-run marker for +// agent-only, TrueNAS or vSphere installations. Retain every unknown entry too, +// so new persistence stores cannot silently fall outside the bootstrap guard. +// A regular .env is the only exception for pre-start deployment overrides; +// keep the input archive outside the destination, not alongside retained state. +func checkAutoImportDestination(dataDir string) error { + info, err := os.Lstat(dataDir) + if os.IsNotExist(err) { + return nil + } + if err != nil || !info.IsDir() { + return errAutoImportDestinationUnknown + } + dir, err := os.Open(dataDir) + if err != nil { + return errAutoImportDestinationUnknown + } + defer dir.Close() + + // Read one entry at a time: admission does not need an unbounded listing, + // file contents, decryption or an attempt to repair the destination. + for { + entries, err := dir.ReadDir(1) + if err != nil && !errors.Is(err, io.EOF) { + return errAutoImportDestinationUnknown + } + for _, entry := range entries { + if entry.Name() == ".env" && entry.Type().IsRegular() { + continue + } + return errAutoImportDestinationNotEmpty + } + if errors.Is(err, io.EOF) { + return nil + } + } } func PerformAutoImport() error { @@ -1131,6 +1178,17 @@ func PerformAutoImport() error { } configPath := config.ResolveRuntimeDataDir("") + // Do not rely on an earlier ShouldAutoImport result or a caller using it. + // This is a pre-write recheck, not a lock against another Pulse process; + // provisioning still requires an isolated destination. + if err := checkAutoImportDestination(configPath); err != nil { + reason := "destination_unavailable" + if errors.Is(err, errAutoImportDestinationNotEmpty) { + reason = "destination_not_empty" + } + logAudit(false, reason) + return err + } persistence := config.NewConfigPersistence(configPath) if err := persistence.ImportConfig(encryptedData, configPass); err != nil { diff --git a/pkg/server/service_health_test.go b/pkg/server/service_health_test.go index 59af14f431..1c4c18fbc8 100644 --- a/pkg/server/service_health_test.go +++ b/pkg/server/service_health_test.go @@ -50,9 +50,18 @@ func TestStartupImportRetainsNonProxmoxAccess(t *testing.T) { if ShouldAutoImport() { t.Error("ordinary restart was admitted as fresh provisioning") } + capture := setCaptureAuditLogger(t) if err := PerformAutoImport(); err == nil { t.Error("startup import replaced existing access") } + if len(capture.events) != 1 || capture.events[0].Success || + !strings.Contains(capture.events[0].Details, "reason=destination_not_empty") { + t.Error("ordinary restart refusal lost its classified audit result") + } else if strings.Contains(capture.events[0].Details, dir) || + strings.Contains(capture.events[0].Details, payload) || + strings.Contains(capture.events[0].Details, passphrase) { + t.Error("ordinary restart audit disclosed private import inputs") + } if after := snapshotAutoImportDestination(t, dir); !reflect.DeepEqual(before, after) { t.Error("startup import changed the retained installation") } diff --git a/scripts/tests/test_bootstrap_import_docs.py b/scripts/tests/test_bootstrap_import_docs.py index 58d8b93a1a..0628b19825 100644 --- a/scripts/tests/test_bootstrap_import_docs.py +++ b/scripts/tests/test_bootstrap_import_docs.py @@ -24,13 +24,18 @@ class BootstrapImportDocsTest(unittest.TestCase): return bootstrap((ROOT / "docs/CONFIGURATION.md").read_text()) def assert_gate_and_result(self, text): - for boundary in ("`nodes.enc`", "`PULSE_DATA_DIR`", "file-presence check", - "not a record of the first run", "Other existing settings", - "do not prevent", "Do not delete `nodes.enc`", + for boundary in ("`nodes.enc`", "`PULSE_DATA_DIR`", "empty data directory", + "only exception", "API-token records", "encryption keys", + "unknown contents", "all prevent startup import", + "Unreadable directories and symbolic links", + "checks the destination again before writing", + "lock against another running Pulse process", + "Do not delete `nodes.enc`", "not proof that import succeeded", "startup continues", "normal configuration loading", "can itself fail"): self.assertIn(boundary, text) self.assertNotIn("auto-imports on first start", text) + self.assertNotIn("do not prevent an import attempt", text) def assert_transfer_scope(self, text): for boundary in ("MIGRATION.md#configuration-transfer", "not a full-state backup", @@ -59,7 +64,9 @@ class BootstrapImportDocsTest(unittest.TestCase): "owner-only", "private credential source", "separate from the backup", "command arguments, URLs, shared Compose files, logs or issue reports", "base64 is not redaction", "no passphrase-file variable", - "authenticated Recovery form"): + "authenticated Recovery form", + "outside the destination data directory", + "archive placed inside it", "retains those contents"): self.assertIn(boundary, text) self.assertNotRegex(text, r"PULSE_INIT_CONFIG_(?:PASSPHRASE_FILE|FORCE)") self.assertNotRegex(text, r"```(?:bash|sh|shell|yaml|dotenv)\b") @@ -81,7 +88,7 @@ class BootstrapImportDocsTest(unittest.TestCase): # Removing either boundary restores a real misleading interpretation. text = self.guide() for changed in ( - text.replace("not a record of the first run", "a record of the first run"), + text.replace("all prevent startup import", "do not prevent startup import"), text.replace("not proof that import succeeded", "proof that import succeeded"), ): with self.assertRaises(AssertionError):