From 30e50e68dc8e364ce64a32700fd6fac45842f981 Mon Sep 17 00:00:00 2001 From: admin Date: Wed, 28 Oct 2026 08:45:03 -0400 Subject: [PATCH 1/6] test(deploy): make pintest.Home 0700 under umask 002 (D-0697) --- internal/deploy/pintest/pintest.go | 20 ++++++++++++---- .../deploy/pintest/pintest_umask_unix_test.go | 24 +++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 internal/deploy/pintest/pintest_umask_unix_test.go diff --git a/internal/deploy/pintest/pintest.go b/internal/deploy/pintest/pintest.go index 4baf0086c..15bc12e35 100644 --- a/internal/deploy/pintest/pintest.go +++ b/internal/deploy/pintest/pintest.go @@ -21,8 +21,9 @@ import ( // Home returns a fresh directory suitable as $HOME for a pin-store test. // It uses t.TempDir() when that location's ancestors are sealed (the normal -// case, byte-for-byte the old behaviour) and otherwise a directory under -// /tmp, removed when the test ends. When no location is sealed the test is +// case) and otherwise a directory under /tmp, removed when the test ends. The +// directory is chmod 0700 either way, so a umask of 002 does not make HOME +// itself a group-writable ancestor. When no location is sealed the test is // skipped with the reason instead of failing on the host's layout. func Home(t testing.TB) string { t.Helper() @@ -30,7 +31,7 @@ func Home(t testing.TB) string { return t.TempDir() // the production check is skipped on windows } if Sealed(os.TempDir()) { - return t.TempDir() + return private(t, t.TempDir()) } for _, base := range []string{"/tmp", "/private/tmp"} { if !Sealed(base) { @@ -41,12 +42,23 @@ func Home(t testing.TB) string { continue } t.Cleanup(func() { _ = os.RemoveAll(dir) }) - return dir + return private(t, dir) } t.Skipf("no temp location with sealed ancestors (TMPDIR=%q): the pinned-host-keys rule refuses group/world-writable parents", os.Getenv("TMPDIR")) return "" } +// private makes dir 0700. t.TempDir creates its dir with 0777 &^ umask, so on a +// host with umask 002 it is 0775 and is itself the group-writable ancestor of +// $HOME/.config/nself that the pin store refuses (D-0697). +func private(t testing.TB, dir string) string { + t.Helper() + if err := os.Chmod(dir, 0o700); err != nil { + t.Fatal(err) + } + return dir +} + // Sealed reports whether dir passes the same ancestor rule as production // (sdk/go/remote ancestorOK): every directory above it, on the path as given // and with symlinks resolved, is owned by the current user or root, and is not diff --git a/internal/deploy/pintest/pintest_umask_unix_test.go b/internal/deploy/pintest/pintest_umask_unix_test.go new file mode 100644 index 000000000..70500b075 --- /dev/null +++ b/internal/deploy/pintest/pintest_umask_unix_test.go @@ -0,0 +1,24 @@ +//go:build !windows + +package pintest + +import ( + "os" + "syscall" + "testing" +) + +// D-0697: under umask 002 t.TempDir() is 0775, which the pin store refuses as a +// group-writable ancestor of $HOME/.config/nself. Home must hand back 0700. +func TestHomeIsPrivateUnderGroupWritableUmask(t *testing.T) { + old := syscall.Umask(0o002) + defer syscall.Umask(old) + home := Home(t) + fi, err := os.Stat(home) + if err != nil { + t.Fatal(err) + } + if got := fi.Mode().Perm(); got != 0o700 { + t.Fatalf("Home mode = %#o, want 0700", got) + } +} From 4fce148e067aeca8503fd086154d45382a217c86 Mon Sep 17 00:00:00 2001 From: admin Date: Wed, 28 Oct 2026 08:45:03 -0400 Subject: [PATCH 2/6] fix(promote): capture the compose ps listing with the images listing and replay it (D-0701) --- .github/wiki/cmd-deploy.md | 6 +++ cmd/commands/deploy_promote.go | 2 +- internal/controlplane/promote/digest_parse.go | 13 ++--- .../controlplane/promote/source_manifest.go | 30 +++++++++++- .../promote/source_manifest_test.go | 47 +++++++++++++++++++ 5 files changed, 90 insertions(+), 8 deletions(-) diff --git a/.github/wiki/cmd-deploy.md b/.github/wiki/cmd-deploy.md index 90abf459b..047a83095 100644 --- a/.github/wiki/cmd-deploy.md +++ b/.github/wiki/cmd-deploy.md @@ -107,6 +107,12 @@ The steps are resolve, confirm, verify, preflight, source-health, capture, build compare, snapshot, save, apply, migrate, health, and record. Builds use the target cascade in a scratch directory. Production values never enter the operator's tree. +Local-server promotion on a Windows operator machine is unproven end to end. The +local runner starts every command through `sh -c`, so it needs a POSIX `sh` on +`PATH`, and the model resolver gives that process a minimal environment (`PATH`, +`HOME`, `DOCKER_CONFIG`, `XDG_CONFIG_HOME`) without `SYSTEMROOT`, which Windows +programs often need. Promote from Linux or macOS, or from WSL. + Compare proves per-platform image digest equality before taking a snapshot or changing the target. It resolves every staged compose fragment with the target's environment files and captures the source using its compose manifest. Each service's diff --git a/cmd/commands/deploy_promote.go b/cmd/commands/deploy_promote.go index 2ccfa2d0d..516743c0a 100644 --- a/cmd/commands/deploy_promote.go +++ b/cmd/commands/deploy_promote.go @@ -254,7 +254,7 @@ func runPromoteEnvironments(cmd *cobra.Command, source, target string) error { // resolveModel isolates Compose interpolation from the operator's shell values. func (r localPromoteRemote) resolveModel(ctx context.Context, command string) ([]byte, error) { - c := exec.CommandContext(ctx, "sh", "-c", command) //nolint:gosec // fixed command and validated, quoted paths + c := exec.CommandContext(ctx, "sh", "-c", command) //nolint:gosec // fixed command fragments; remote paths are validated and every path is shell-quoted (local paths are quoted only) c.Dir = r.root c.Env = []string{"PATH=" + os.Getenv("PATH")} for _, key := range []string{"HOME", "DOCKER_CONFIG", "XDG_CONFIG_HOME"} { diff --git a/internal/controlplane/promote/digest_parse.go b/internal/controlplane/promote/digest_parse.go index 60c38bae2..90f89756e 100644 --- a/internal/controlplane/promote/digest_parse.go +++ b/internal/controlplane/promote/digest_parse.go @@ -46,6 +46,9 @@ func parseComposeRows[T any](raw []byte, what string) ([]T, error) { return rows, nil } +// composePSCommand lists every container of the source project with its service. +const composePSCommand = "docker compose ps --all --format json" + // composeContainer is one row of `docker compose ps --format json`. type composeContainer struct { Name string `json:"Name"` @@ -56,15 +59,13 @@ type composeContainer struct { // Compose's images JSON carries ContainerName and no Service (checked against // Compose 5.6), so rows without a service are mapped through `docker compose ps`. // A container that ps does not name keeps an empty service and fails closed. +// CaptureSourceManifest snapshots the ps listing next to the images listing, so +// the mapping here sees the same containers the images listing named. func resolveImageServices(ctx context.Context, run Runner, images []composeImage) ([]composeImage, error) { - missing := false - for _, image := range images { - missing = missing || image.Service == "" - } - if !missing { + if !missingService(images) { return images, nil } - raw, err := run(ctx, "docker compose ps --all --format json") + raw, err := run(ctx, composePSCommand) if err != nil { return nil, fmt.Errorf("list source compose containers: %w", err) } diff --git a/internal/controlplane/promote/source_manifest.go b/internal/controlplane/promote/source_manifest.go index 529bf5fac..0921236c1 100644 --- a/internal/controlplane/promote/source_manifest.go +++ b/internal/controlplane/promote/source_manifest.go @@ -79,7 +79,35 @@ func CaptureSourceManifest(ctx context.Context, run Runner, root string) (Runner return run(c, command) } raw, err := captured(ctx, "docker compose images --format json") - return captured, raw, err + if err != nil { + return captured, raw, err + } + // Images rows without a service are mapped through ps. Capture that listing + // now and replay it, so a container recreated or renamed before the compare + // cannot be mapped to another service. + if rows, perr := parseComposeImages(raw); perr == nil && missingService(rows) { + ps, err := captured(ctx, composePSCommand) + if err != nil { + return captured, raw, fmt.Errorf("list source compose containers: %w", err) + } + replay := func(c context.Context, command string) ([]byte, error) { + if command == composePSCommand { + return ps, nil + } + return captured(c, command) + } + return replay, raw, nil + } + return captured, raw, nil +} + +func missingService(images []composeImage) bool { + for _, image := range images { + if image.Service == "" { + return true + } + } + return false } // sourceManifest reads optional host manifests; every entry is validated before use. diff --git a/internal/controlplane/promote/source_manifest_test.go b/internal/controlplane/promote/source_manifest_test.go index 25dfff24c..6ab6bcb54 100644 --- a/internal/controlplane/promote/source_manifest_test.go +++ b/internal/controlplane/promote/source_manifest_test.go @@ -123,3 +123,50 @@ func TestResolvedComposeImagesLocalPaths(t *testing.T) { } } } + +// D-0701: the ps listing that maps containers to services is read at capture, +// next to the images listing, and replayed at compare. A container that moves +// to another service in between cannot change the mapping. +func TestCaptureSourceManifestReplaysPS(t *testing.T) { + ctx := context.Background() + psReads := 0 + ps := []string{`{"Name":"proj-api-1","Service":"api"}` + "\n" + `{"Name":"proj-db-1","Service":"db"}`, `{"Name":"proj-api-1","Service":"db"}` + "\n" + `{"Name":"proj-db-1","Service":"api"}`} + run := func(_ context.Context, command string) ([]byte, error) { + switch { + case strings.Contains(command, ".nself/compose"): + return []byte("__NSELF_ABSENT__"), nil + case strings.HasSuffix(command, " images --format json"): + return []byte(`[{"ID":"sha256:a","ContainerName":"proj-api-1","Repository":"example.invalid/api","Tag":"v1"},{"ID":"sha256:d","ContainerName":"proj-db-1","Repository":"example.invalid/db","Tag":"v1"}]`), nil + case strings.HasSuffix(command, " ps --all --format json"): + psReads++ + return []byte(ps[min(psReads-1, len(ps)-1)]), nil // the second read is the swapped one + case strings.Contains(command, " config --format json"): + return []byte(`{"services":{"api":{"image":"example.invalid/api:v1"},"db":{"image":"example.invalid/db:v1"}}}`), nil + case strings.HasPrefix(command, "docker image inspect"): + repo := "example.invalid/api" + if strings.Contains(command, "sha256:d") { + repo = "example.invalid/db" + } + return []byte(`["` + repo + `@sha256:` + strings.Repeat("c", 64) + `"]|linux/amd64`), nil + } + return nil, fmt.Errorf("unexpected command %s", command) + } + lookup := func(_ context.Context, ref string) (map[string]string, error) { + return map[string]string{"linux/amd64": "sha256:" + strings.Repeat(map[bool]string{true: "a", false: "b"}[strings.Contains(ref, "/api")], 64)}, nil + } + captured, raw, err := CaptureSourceManifest(ctx, run, "/srv/fixture") + if err != nil || psReads != 1 { + t.Fatalf("capture: %v, ps reads %d", err, psReads) + } + images, err := CaptureManifestSource(ctx, captured, raw, lookup) + if err != nil || psReads != 1 { + t.Fatalf("compare: %v, ps reads %d (the compare must not list containers again)", err, psReads) + } + got := map[string]string{} + for _, image := range images { + got[image.Service] = image.Ref + } + if got["api"] != "example.invalid/api:v1" || got["db"] != "example.invalid/db:v1" { + t.Fatalf("mapping followed the later ps listing: %#v", got) + } +} From 88e6904af255a12c98678f072e829afcdfc9a3df Mon Sep 17 00:00:00 2001 From: admin Date: Wed, 28 Oct 2026 08:45:03 -0400 Subject: [PATCH 3/6] test(verify): pin the evidence report keys against goldens from the real ci plugin (D-1217) --- .../promote/verify/evidence_contract_test.go | 56 +++++++++ .../promote/verify/testdata/README.md | 21 ++++ .../testdata/evidence-verify-pass.golden.json | 105 +++++++++++++++++ .../evidence-verify-rejected.golden.json | 10 ++ .../evidence-verify-release-class.golden.json | 110 ++++++++++++++++++ 5 files changed, 302 insertions(+) create mode 100644 internal/controlplane/promote/verify/evidence_contract_test.go create mode 100644 internal/controlplane/promote/verify/testdata/README.md create mode 100644 internal/controlplane/promote/verify/testdata/evidence-verify-pass.golden.json create mode 100644 internal/controlplane/promote/verify/testdata/evidence-verify-rejected.golden.json create mode 100644 internal/controlplane/promote/verify/testdata/evidence-verify-release-class.golden.json diff --git a/internal/controlplane/promote/verify/evidence_contract_test.go b/internal/controlplane/promote/verify/evidence_contract_test.go new file mode 100644 index 000000000..4b687c83e --- /dev/null +++ b/internal/controlplane/promote/verify/evidence_contract_test.go @@ -0,0 +1,56 @@ +package verify + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// goldenRunner replays a golden captured from the real ci plugin +// (testdata/README.md) as the plugin's stdout with the given exit status. +func goldenRunner(t *testing.T, name string, exit int) EvidenceRunner { + t.Helper() + raw, err := os.ReadFile(filepath.Join("testdata", name)) + if err != nil { + t.Fatal(err) + } + return func(context.Context, string, string) ([]byte, int, error) { return raw, exit, nil } +} + +// D-1217: the report keys read here (data.class, data.result, data.quarantined, +// error.message) are the ones the real plugin prints. +func TestEvidenceReaderMatchesPluginGoldens(t *testing.T) { + ctx := context.Background() + t.Run("keys decode from the real report", func(t *testing.T) { + for _, name := range []string{"evidence-verify-pass.golden.json", "evidence-verify-release-class.golden.json"} { + raw, err := os.ReadFile(filepath.Join("testdata", name)) + if err != nil { + t.Fatal(err) + } + var rep evidenceReport + if err := json.Unmarshal(raw, &rep); err != nil || rep.Data.Class == "" || rep.Data.Result == "" || rep.Data.Quarantined == nil { + t.Errorf("%s: class %q result %q quarantined %v: a key the reader needs is missing", name, rep.Data.Class, rep.Data.Result, rep.Data.Quarantined) + } + } + }) + t.Run("a passing default-class report is refused for its class", func(t *testing.T) { + err := checkEvidence(ctx, goldenRunner(t, "evidence-verify-pass.golden.json", 0), "evidence.json", "rev") + if err == nil || !strings.Contains(err.Error(), `class "default" result "pass"`) { + t.Fatalf("err = %v, want E497 naming class default result pass", err) + } + }) + t.Run("a release-class passing report is accepted when the plugin exits 0", func(t *testing.T) { + if err := checkEvidence(ctx, goldenRunner(t, "evidence-verify-release-class.golden.json", 0), "evidence.json", "rev"); err != nil { + t.Fatalf("err = %v, want nil: class/result/quarantined were not read from the real report", err) + } + }) + t.Run("a refusal carries the plugin's error message", func(t *testing.T) { + err := checkEvidence(ctx, goldenRunner(t, "evidence-verify-rejected.golden.json", 11), "evidence.json", "rev") + if codeOf(err) != "E497" || !strings.Contains(err.Error(), "evidence rejected") { + t.Fatalf("err = %v, want E497 with the plugin's message", err) + } + }) +} diff --git a/internal/controlplane/promote/verify/testdata/README.md b/internal/controlplane/promote/verify/testdata/README.md new file mode 100644 index 000000000..54137a6ae --- /dev/null +++ b/internal/controlplane/promote/verify/testdata/README.md @@ -0,0 +1,21 @@ +# evidence verify goldens + +Real stdout of `ci evidence verify --file --at-revision --json`, +captured from the ci plugin (nself-org/plugins, free/ci, plugin version 1.2.13, +plugins commit 076f5063f245cbe0fcd2d05120eef2a477679e6c), built with +`go build -mod=vendor ./cmd` and run against a scratch git repository +(`NSELF_V15=1`, `NSELF_CI_REPO`, `NSELF_CI_HOME`, one `kind: test` job). + +- `evidence-verify-pass.golden.json`: a passing default-class document (exit 0). +- `evidence-verify-release-class.golden.json`: the same document with its check + kind edited to `release`, which the plugin classes as `release` (exit 11: the + edit breaks the document's digests, so the plugin rejects it, but the report + still carries the class). A passing release-class run needs a container + executor, which the capture host did not have. +- `evidence-verify-rejected.golden.json`: a rejected document (exit 11), the + error envelope. + +`evidence_contract_test.go` feeds these through the same reader promote uses, so +a plugin rename of `data.class`, `data.result`, `data.quarantined` or +`error.message` fails here instead of turning every promote into E497. +To refresh after a plugin report change, rerun the command above and replace the files. diff --git a/internal/controlplane/promote/verify/testdata/evidence-verify-pass.golden.json b/internal/controlplane/promote/verify/testdata/evidence-verify-pass.golden.json new file mode 100644 index 000000000..d451537e7 --- /dev/null +++ b/internal/controlplane/promote/verify/testdata/evidence-verify-pass.golden.json @@ -0,0 +1,105 @@ +{ + "schema_version": "1", + "command": "ci evidence verify", + "data": { + "revision": "4613469d35e5185e840146bb8029963dd98e117a", + "result": "pass", + "class": "default", + "binding": { + "state": "bound", + "reason": null + }, + "freshness": { + "state": "current", + "reason": null + }, + "quarantined": false, + "evidence": { + "schema": "ci.evidence/v1", + "revision": "4613469d35e5185e840146bb8029963dd98e117a", + "repo_id": "local:4710ee8d7d0fd5ea52e5912f29b59b25dde280901f343827487fce28e0a9fb5c", + "run_id": "r20261028T124124-6c6f249b", + "pipeline": { + "id": "default", + "name": "default", + "kind": "default" + }, + "trigger": "local", + "source_trust": "owner", + "policy_digest": "9a1bfbc95f4d2fd4600559bea48a6e2e9a7112852fea1bd7898fb347ebd4f31f", + "input_digests": { + "config": "59deaad650ed51e4a92143e1ed3d51da2b7e795f388efca079d826ef17075830", + "lockfiles": [ + { + "path": "go.sum", + "sha256": "363b7201a7fdf356fae5489cc44ec14f1526cb7fa905728bbec28a155967e755" + } + ], + "generated": [] + }, + "worktree_state": "clean", + "tracked_changes_digest": null, + "binding": { + "state": "bound", + "reason": null + }, + "runner": { + "id": "local", + "provider": "local", + "platform": "darwin/arm64", + "isolation": "process" + }, + "producer": { + "kind": "coordinator-recorded", + "version": "1" + }, + "timings": { + "queued_at": "2026-10-28T12:41:24Z", + "started_at": "2026-10-28T12:41:24Z", + "finished_at": "2026-10-28T12:41:25Z", + "duration_ms": 240 + }, + "tool_versions": { + "git": "2.56.0" + }, + "checks": [ + { + "id": "unit", + "job_id": "unit", + "kind": "test", + "tool": "", + "tool_version": null, + "required": true, + "substantive": true, + "result": "pass", + "findings": 0, + "excerpt": "", + "log_ref": null + } + ], + "artifacts": [], + "retries": [], + "selection": { + "mode": "full", + "reason": "selection.no_base", + "digest": "247589de6d9f5793deeafb412aad0162a418d4caf0c5d0d38881e1af2e3cb82b" + }, + "placement_ref": "sched:decision/r20261028T124124-6c6f249b.unit-385cfdbc.a1", + "result": "pass", + "counts": { + "pass": 1, + "fail": 0, + "skip": 0, + "error": 0, + "total": 1 + }, + "freshness": { + "state": "current", + "reason": null + }, + "signature": { + "kind": "coordinator-recorded" + } + } + } +} diff --git a/internal/controlplane/promote/verify/testdata/evidence-verify-rejected.golden.json b/internal/controlplane/promote/verify/testdata/evidence-verify-rejected.golden.json new file mode 100644 index 000000000..e5125ef7f --- /dev/null +++ b/internal/controlplane/promote/verify/testdata/evidence-verify-rejected.golden.json @@ -0,0 +1,10 @@ +{ + "schema_version": "1", + "command": "ci evidence verify", + "error": { + "code": "E629", + "message": "E685: evidence rejected: evidence.invalid (schema validation at validating https://nself.org/schemas/ci-evidence.v1.schema.json: unexpected additional properties [\"not\"])", + "exit_code": 11, + "class": "other" + } +} diff --git a/internal/controlplane/promote/verify/testdata/evidence-verify-release-class.golden.json b/internal/controlplane/promote/verify/testdata/evidence-verify-release-class.golden.json new file mode 100644 index 000000000..d16201ac1 --- /dev/null +++ b/internal/controlplane/promote/verify/testdata/evidence-verify-release-class.golden.json @@ -0,0 +1,110 @@ +{ + "schema_version": "1", + "command": "ci evidence verify", + "data": { + "revision": "4613469d35e5185e840146bb8029963dd98e117a", + "result": "pass", + "class": "release", + "binding": { + "state": "bound", + "reason": null + }, + "freshness": { + "state": "current", + "reason": null + }, + "quarantined": false, + "evidence": { + "schema": "ci.evidence/v1", + "revision": "4613469d35e5185e840146bb8029963dd98e117a", + "repo_id": "local:4710ee8d7d0fd5ea52e5912f29b59b25dde280901f343827487fce28e0a9fb5c", + "run_id": "r20261028T124124-6c6f249b", + "pipeline": { + "id": "default", + "name": "default", + "kind": "default" + }, + "trigger": "local", + "source_trust": "owner", + "policy_digest": "9a1bfbc95f4d2fd4600559bea48a6e2e9a7112852fea1bd7898fb347ebd4f31f", + "input_digests": { + "config": "59deaad650ed51e4a92143e1ed3d51da2b7e795f388efca079d826ef17075830", + "lockfiles": [ + { + "path": "go.sum", + "sha256": "363b7201a7fdf356fae5489cc44ec14f1526cb7fa905728bbec28a155967e755" + } + ], + "generated": [] + }, + "worktree_state": "clean", + "tracked_changes_digest": null, + "binding": { + "state": "bound", + "reason": null + }, + "runner": { + "id": "local", + "provider": "local", + "platform": "darwin/arm64", + "isolation": "process" + }, + "producer": { + "kind": "coordinator-recorded", + "version": "1" + }, + "timings": { + "queued_at": "2026-10-28T12:41:24Z", + "started_at": "2026-10-28T12:41:24Z", + "finished_at": "2026-10-28T12:41:25Z", + "duration_ms": 240 + }, + "tool_versions": { + "git": "2.56.0" + }, + "checks": [ + { + "id": "unit", + "job_id": "unit", + "kind": "release", + "tool": "", + "tool_version": null, + "required": true, + "substantive": true, + "result": "pass", + "findings": 0, + "excerpt": "", + "log_ref": null + } + ], + "artifacts": [], + "retries": [], + "selection": { + "mode": "full", + "reason": "selection.no_base", + "digest": "247589de6d9f5793deeafb412aad0162a418d4caf0c5d0d38881e1af2e3cb82b" + }, + "placement_ref": "sched:decision/r20261028T124124-6c6f249b.unit-385cfdbc.a1", + "result": "pass", + "counts": { + "pass": 1, + "fail": 0, + "skip": 0, + "error": 0, + "total": 1 + }, + "freshness": { + "state": "current", + "reason": null + }, + "signature": { + "kind": "coordinator-recorded" + } + } + }, + "meta": { + "warnings": [ + "the document differs from the evidence this machine recorded for run r20261028T124124-6c6f249b" + ] + } +} From 7ff02645d4a1a7e858d8de2400e4ce29b70b0fdd Mon Sep 17 00:00:00 2001 From: admin Date: Wed, 28 Oct 2026 08:45:03 -0400 Subject: [PATCH 4/6] fix(deploy): empty server_name sets prove no front ownership; prune rechecks the set in the script (D-0778, D-0824) --- internal/deploy/apply_remote_front.go | 5 +- .../deploy/apply_remote_front_guard_test.go | 72 ++++++++++++++++++- internal/deploy/apply_remote_front_test.go | 5 +- internal/deploy/remote_front_check.go | 12 +++- internal/deploy/remote_front_guard.go | 36 +++++++--- 5 files changed, 115 insertions(+), 15 deletions(-) diff --git a/internal/deploy/apply_remote_front.go b/internal/deploy/apply_remote_front.go index 23c40eb9e..01a335ccc 100644 --- a/internal/deploy/apply_remote_front.go +++ b/internal/deploy/apply_remote_front.go @@ -238,7 +238,10 @@ func (fa *frontApply) sync(ctx context.Context, o ApplyOptions, res *ApplyResult for _, k := range kept { emit(o.Out, " %s", k) } - pruned, err := pruneFrontMarked(ctx, r, front, prune) + pruned, kept2, err := pruneFrontMarked(ctx, r, front, prune, fa.names) + for _, k := range kept2 { + emit(o.Out, " %s", k) + } for _, f := range pruned { emit(o.Out, " [pruned] %s/%s (generated file the new set dropped)", front, f) fa.changed = append(fa.changed, f) diff --git a/internal/deploy/apply_remote_front_guard_test.go b/internal/deploy/apply_remote_front_guard_test.go index 87430845a..dac87dafa 100644 --- a/internal/deploy/apply_remote_front_guard_test.go +++ b/internal/deploy/apply_remote_front_guard_test.go @@ -241,7 +241,7 @@ func TestFrontMountCoversLinkedDirs(t *testing.T) { {"sites link the mount does not cover", map[string]string{"nginx/sites": "/srv/web/sites-v2"}, "bind|/srv/web/nginx|/etc/nginx\n", false}, {"sites link covered by its own mount", map[string]string{"nginx/sites": "/srv/web/sites-v2"}, "bind|/srv/web/nginx|/etc/nginx\nbind|/srv/web/sites-v2|/etc/nginx/sites\n", true}, {"nginx link mounted as a path", map[string]string{"nginx": "/srv/web/nginx-real", "nginx/sites": "/srv/web/nginx-real/sites"}, "bind|/srv/web/nginx|/etc/nginx\n", true}, - {"nginx link, mount of a parent real path", map[string]string{"nginx": "/srv/web/nginx-real"}, "bind|/srv/web/nginx-real|/etc/nginx\nbind|/srv/web/nginx|/etc/x\n", true}, + {"nginx link, mount of a parent real path", map[string]string{"nginx": "/srv/web/nginx-real"}, "bind|/srv/web/nginx-real|/etc/nginx\nbind|/srv/web/nginx|/etc/nginx/x\n", true}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -368,3 +368,73 @@ func TestFrontSyncCreatesOnlyAMissingSitesLeaf(t *testing.T) { t.Fatalf("no plain mkdir of the leaf (%d) or resolve (%d)", mk, resolve) } } + +// D-0778: two empty server_name sets are no evidence that a marked file at a +// recorded path is still this project's: it is refused (E485) when it would be +// overwritten and kept when it would be pruned. A table case covers sameNames. +func TestFrontEmptyNameSetsAreNotEqual(t *testing.T) { + for _, c := range []struct { + a, b []string + want bool + }{ + {nil, nil, false}, + {[]string{}, nil, false}, + {[]string{"a.example.org"}, nil, false}, + {nil, []string{"a.example.org"}, false}, + {[]string{"a.example.org"}, []string{"a.example.org"}, true}, + {[]string{"a.example.org", "b.example.org"}, []string{"a.example.org"}, false}, + } { + if got := sameNames(c.a, c.b); got != c.want { + t.Errorf("sameNames(%v, %v) = %v, want %v", c.a, c.b, got, c.want) + } + } + + const rel = "nginx/sites/snippet.conf" + const noNames = markedConf + "upstream u { server 127.0.0.1:1; }\n" // marker, no server_name + front, stage := t.TempDir(), t.TempDir() + writeFile(t, filepath.Join(front, rel), noNames, 0o644) + writeFile(t, filepath.Join(stage, rel), noNames, 0o644) + f := newFakeRemote(t) + ctx := context.Background() + + v := frontView{Stage: stage, Files: []string{rel}, Prev: []string{rel}, Names: map[string][]string{rel: nil}} + if err := checkFrontForeign(ctx, f, front, v); !e485(err) { + t.Errorf("checkFrontForeign over an empty recorded set: err = %v, want E485", err) + } + v = frontView{Stage: stage, Prev: []string{rel}, Names: map[string][]string{rel: nil}} + prune, kept, err := frontPrunable(ctx, f, front, v) + if err != nil || len(prune) != 0 || len(kept) != 1 { + t.Errorf("frontPrunable over an empty recorded set: prune %v kept %v err %v, want kept only", prune, kept, err) + } +} + +// D-0824: the prune script rechecks the server_name set at the moment of the +// delete, so a file replaced after the check (marker kept, other names) stays. +func TestFrontPruneRechecksServerNamesInScript(t *testing.T) { + needTools(t) + const ours = markedConf + "server {\n server_name b.example.org a.example.org; # c.example.org\n}\nserver { server_name a.example.org; }\n" + const theirs = markedConf + "server { server_name api.other.example.org; }\n" + const rel = "nginx/sites/x.conf" + front := t.TempDir() + f := newFakeRemote(t) + ctx := context.Background() + recorded := map[string][]string{rel: {"a.example.org", "b.example.org"}} + + writeFile(t, filepath.Join(front, rel), theirs, 0o644) + removed, kept, err := pruneFrontMarked(ctx, f, front, []string{rel}, recorded) + if err != nil || len(removed) != 0 || len(kept) != 1 || !exists(filepath.Join(front, rel)) { + t.Fatalf("a replaced file: removed %v kept %v err %v", removed, kept, err) + } + writeFile(t, filepath.Join(front, rel), markedConf+"upstream u { server 127.0.0.1:1; }\n", 0o644) + if removed, _, _ = pruneFrontMarked(ctx, f, front, []string{rel}, recorded); len(removed) != 0 || !exists(filepath.Join(front, rel)) { + t.Fatalf("a file with no server_name was removed: %v", removed) + } + writeFile(t, filepath.Join(front, rel), ours, 0o644) + if removed, _, _ = pruneFrontMarked(ctx, f, front, []string{rel}, map[string][]string{}); len(removed) != 0 || !exists(filepath.Join(front, rel)) { + t.Fatalf("a path with no recorded set was removed: %v", removed) + } + removed, kept, err = pruneFrontMarked(ctx, f, front, []string{rel}, recorded) + if err != nil || len(removed) != 1 || removed[0] != rel || len(kept) != 0 || exists(filepath.Join(front, rel)) { + t.Fatalf("the file as recorded: removed %v kept %v err %v", removed, kept, err) + } +} diff --git a/internal/deploy/apply_remote_front_test.go b/internal/deploy/apply_remote_front_test.go index 3461aeb2e..6baf57e4b 100644 --- a/internal/deploy/apply_remote_front_test.go +++ b/internal/deploy/apply_remote_front_test.go @@ -246,6 +246,7 @@ func TestFrontNginxContainerByLabels(t *testing.T) { {"two match", psRows(row("aaa1", "n1", front), row("ccc3", "n2", real)), mount(front + "/nginx"), "", "found 2"}, {"not a plain id", psRows(row("a;rm -rf x", "n1", front)), mount(front + "/nginx"), "", "not a plain container id"}, {"no mount of nginx", psRows(row("aaa1", "n1", front)), mount(front+"/ssl") + mount(front), "", "does not bind-mount"}, + {"nginx mounted outside /etc/nginx", psRows(row("aaa1", "n1", front)), "bind|" + front + "/nginx|/srv/elsewhere\n", "", "does not bind-mount"}, {"a volume is not a bind mount", psRows(row("aaa1", "n1", front)), "volume|" + front + "/nginx|/etc/nginx\n", "", "does not bind-mount"}, } for _, c := range cases { @@ -497,9 +498,9 @@ func TestFrontContainerStopsBeforeReload(t *testing.T) { func TestFrontPruneOnlyOwnShipped(t *testing.T) { x := frontFixture(t) sites := filepath.Join(x.front, "nginx", "sites") - writeFile(t, filepath.Join(sites, "old.conf"), markedConf+"# shipped by the previous deploy\n", 0o644) + writeFile(t, filepath.Join(sites, "old.conf"), markedConf+"server { server_name old.example.org; }\n# shipped by the previous deploy\n", 0o644) writeFile(t, filepath.Join(sites, "mine-but-edited.conf"), "# hand edited, marker removed\n", 0o644) - x.o.Set.Front.PreviousNames = map[string]map[string][]string{"fake-host": {"nginx/sites/old.conf": {}}} // old.conf shipped with no server_name + x.o.Set.Front.PreviousNames = map[string]map[string][]string{"fake-host": {"nginx/sites/old.conf": {"old.example.org"}}} // old.conf as recorded x.o.Set.Front.Previous = map[string][]string{ "fake-host": {"nginx/sites/old.conf", "nginx/sites/mine-but-edited.conf", "nginx/sites/api.conf", ".env", "nginx/sites/../../.env"}, "other-host": {"nginx/sites/sibling.conf"}, // another host's record never prunes here diff --git a/internal/deploy/remote_front_check.go b/internal/deploy/remote_front_check.go index 7a7819cbd..9160178ef 100644 --- a/internal/deploy/remote_front_check.go +++ b/internal/deploy/remote_front_check.go @@ -14,7 +14,7 @@ package deploy // what the record says this project shipped there (or what it ships // now). A path without a recorded set (an old record) is treated // like an unknown path: its set must equal the shipped file's, and -// it is never pruned. An empty set never matches an unknown path. +// it is never pruned. An empty set never matches (sameNames). import ( "context" @@ -32,7 +32,11 @@ type frontView struct { Names map[string][]string // that record's server_name set per path; a missing key is "unknown" } -func sameNames(a, b []string) bool { return strings.Join(a, " ") == strings.Join(b, " ") } +// sameNames reports whether two server_name sets are equal. An empty set is no +// evidence of ownership, so it equals nothing, not even another empty set. +func sameNames(a, b []string) bool { + return len(a) > 0 && len(b) > 0 && strings.Join(a, " ") == strings.Join(b, " ") +} // checkFrontForeign refuses (E485, nothing sent) to replace a host file that is // not this project's. @@ -63,7 +67,7 @@ func checkFrontForeign(ctx context.Context, r Remote, front string, v frontView) if sameNames(theirs, rec) || sameNames(theirs, ours) { continue } - } else if len(theirs) > 0 && len(ours) > 0 && sameNames(theirs, ours) { + } else if sameNames(theirs, ours) { continue } return errs.New("E485", fmt.Sprintf("%q in the fronting stack is a generated file of another project: its server_name set is [%s], this deploy ships [%s]", @@ -102,6 +106,8 @@ func frontPrunable(ctx context.Context, r Remote, front string, v frontView) (pr kept = append(kept, fmt.Sprintf("[kept] %s/%s (no generated marker; not pruned)", front, p)) case !have: kept = append(kept, fmt.Sprintf("[kept] %s/%s (no server_name set recorded for it; not pruned)", front, p)) + case len(rec) == 0 || len(serverNameSet(conf)) == 0: + kept = append(kept, fmt.Sprintf("[kept] %s/%s (an empty server_name set proves no ownership; not pruned)", front, p)) case !sameNames(serverNameSet(conf), rec): kept = append(kept, fmt.Sprintf("[kept] %s/%s (server_name changed since this project shipped it; not pruned)", front, p)) default: diff --git a/internal/deploy/remote_front_guard.go b/internal/deploy/remote_front_guard.go index 6188494c9..e53486f93 100644 --- a/internal/deploy/remote_front_guard.go +++ b/internal/deploy/remote_front_guard.go @@ -121,36 +121,56 @@ func localServerNames(stage, rel string) ([]string, error) { // pruneFrontMarked removes, from the candidates (front paths this host's record // says the previous deploy shipped), the files that carry the strict build -// marker. It returns the files it removed. -func pruneFrontMarked(ctx context.Context, r Remote, front string, candidates []string) ([]string, error) { +// marker and, inside the same script, whose server_name set is still the one in +// names (the set read at the check, so a file replaced since then stays). It +// returns the files it removed and a [kept] line for each file it left because +// its set changed. +func pruneFrontMarked(ctx context.Context, r Remote, front string, candidates []string, names map[string][]string) (removed, kept []string, err error) { var cand []string for _, f := range candidates { if frontPathOK(f) { cand = append(cand, f) } } - var removed []string pat := q("^" + strings.ReplaceAll(BuildNginxMarker, " ", "[ ]")) for len(cand) > 0 { n := min(len(cand), 50) - body := fmt.Sprintf(`if [ -f "$2" ] && head -n 5 -- "$2" 2>/dev/null | grep -q %s; then rm -f -- "$2" && printf '%%s\n' "$1"; fi`, pat) + var want strings.Builder + for _, f := range cand[:n] { + if set := names[f]; len(set) > 0 { + fmt.Fprintf(&want, "%s) want=%s;; ", q(f), q(strings.Join(set, " "))) + } + } + body := strings.NewReplacer("@WANT@", want.String(), "@PAT@", pat, "@MAX@", fmt.Sprint(frontConfMax)).Replace(frontPruneBody) out, err := r.Run(ctx, shScript(guardedScript(front, cand[:n], body))) if err != nil { - return removed, fmt.Errorf("pruning generated files on %s: %w", r.Label(), err) + return removed, kept, fmt.Errorf("pruning generated files on %s: %w", r.Label(), err) } if err := refusedPaths(out); err != nil { - return removed, err + return removed, kept, err } asked := map[string]bool{} for _, f := range cand[:n] { asked[f] = true } for _, f := range strings.Split(strings.TrimSpace(string(out)), "\n") { - if asked[f] { + if p, ok := strings.CutPrefix(f, "KEPT "); ok && asked[p] { + kept = append(kept, fmt.Sprintf("[kept] %s/%s (server_name changed since it was checked; not pruned)", front, p)) + } else if asked[f] { removed = append(removed, f) } } cand = cand[n:] } - return removed, nil + return removed, kept, nil } + +// frontPruneBody is the per-path prune step: the marker rule, then the same +// server_name set as serverNameSet (comments dropped, sorted, unique) compared +// with the recorded one. A path with no recorded set is never removed. +const frontPruneBody = `if [ -f "$2" ] && head -n 5 -- "$2" 2>/dev/null | grep -q @PAT@; then +want=; case "$1" in @WANT@esac +got=$(head -c @MAX@ -- "$2" 2>/dev/null | sed 's/#.*//' | grep -oE '(^|[^[:alnum:]_])server_name[[:space:]]+[^;]+;' | sed -E 's/^.*server_name[[:space:]]+//; s/;$//' | tr -s '[:space:]' '\n' | grep -v '^$' | LC_ALL=C sort -u | tr '\n' ' ') +got=${got% } +if [ -n "$want" ] && [ "$got" = "$want" ]; then rm -f -- "$2" && printf '%s\n' "$1"; else printf 'KEPT %s\n' "$1"; fi +fi` From b827e4f8113175bcbbe42be7f8172fb42cd76f52 Mon Sep 17 00:00:00 2001 From: admin Date: Wed, 28 Oct 2026 08:45:08 -0400 Subject: [PATCH 5/6] fix(deploy): v1.5 remote apply hardening nits (D-0824) guardedScript exits 1 if any act failed, build contexts resolve against the first compose file, checkMounts needs a destination under /etc/nginx, the scratch clear refuses symlinked .nself ancestors, the shipped list .new file is removed before the write, and the front cert find only treats a missing dir as empty. --- cmd/commands/deploy_scratch_fronted.go | 7 +++ cmd/commands/deploy_scratch_test.go | 42 ++++++++++++++ internal/deploy/remote_buildctx.go | 2 +- internal/deploy/remote_fixes_test.go | 22 ++++++++ internal/deploy/remote_front.go | 6 +- internal/deploy/remote_hardening_test.go | 55 +++++++++++++++++++ internal/deploy/remote_layout.go | 7 ++- internal/deploy/remote_sync.go | 4 +- internal/deploy/target_snapshot_front.go | 2 +- internal/deploy/target_snapshot_front_test.go | 20 +++++++ 10 files changed, 160 insertions(+), 7 deletions(-) diff --git a/cmd/commands/deploy_scratch_fronted.go b/cmd/commands/deploy_scratch_fronted.go index 329534ca3..3983b1a0c 100644 --- a/cmd/commands/deploy_scratch_fronted.go +++ b/cmd/commands/deploy_scratch_fronted.go @@ -85,6 +85,13 @@ func finishScratchStage(w io.Writer, set deploy.ArtifactSet, stale []string) (de // clearScratch removes the scratch root of env (both layouts). func clearScratch(workdir, env string) error { root := scratchDirFor(workdir, env) + // RemoveAll follows a symlinked ancestor, so a planted .nself or + // .nself/deploy-build link would aim the delete at another tree. + for _, anc := range []string{filepath.Join(workdir, ".nself"), filepath.Dir(root)} { + if fi, err := os.Lstat(anc); err == nil && fi.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("deploy: %s is a symlink; refusing to clear the scratch directory through it", anc) + } + } if err := os.RemoveAll(root); err != nil { return fmt.Errorf("deploy: clearing %s: %w", root, err) } diff --git a/cmd/commands/deploy_scratch_test.go b/cmd/commands/deploy_scratch_test.go index 538d9c226..e8c7323d8 100644 --- a/cmd/commands/deploy_scratch_test.go +++ b/cmd/commands/deploy_scratch_test.go @@ -198,3 +198,45 @@ func isolateDeployConfigEnv(t *testing.T) { } } } + +// D-0824: clearScratch never deletes through a symlinked .nself ancestor. +func TestClearScratchRefusesSymlinkedAncestor(t *testing.T) { + for _, tc := range []struct { + name string + link func(work, other string) string // returns the planted link's path + }{ + {".nself", func(work, other string) string { return filepath.Join(work, ".nself") }}, + {".nself/deploy-build", func(work, other string) string { + _ = os.MkdirAll(filepath.Join(work, ".nself"), 0o755) + return filepath.Join(work, ".nself", "deploy-build") + }}, + } { + t.Run(tc.name, func(t *testing.T) { + work, other := t.TempDir(), t.TempDir() + victim := filepath.Join(other, "prod", "keep.txt") // what scratchDirFor(work, "prod") would hit through the link + wf(t, victim, "keep\n") + if tc.name == ".nself" { + wf(t, filepath.Join(other, "deploy-build", "prod", "keep.txt"), "keep\n") + victim = filepath.Join(other, "deploy-build", "prod", "keep.txt") + } + if err := os.Symlink(other, tc.link(work, other)); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if err := clearScratch(work, "prod"); err == nil { + t.Error("clearScratch went through a symlinked ancestor without an error") + } + if _, err := os.Stat(victim); err != nil { + t.Errorf("the file behind the link was deleted: %v", err) + } + }) + } + // A plain tree is still cleared. + work := t.TempDir() + wf(t, filepath.Join(work, ".nself", "deploy-build", "prod", "stale.txt"), "x") + if err := clearScratch(work, "prod"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(work, ".nself", "deploy-build", "prod")); err == nil { + t.Error("the scratch root was not cleared") + } +} diff --git a/internal/deploy/remote_buildctx.go b/internal/deploy/remote_buildctx.go index 1dbebfb64..a3a74bd4e 100644 --- a/internal/deploy/remote_buildctx.go +++ b/internal/deploy/remote_buildctx.go @@ -54,7 +54,7 @@ func CheckBuildContexts(ctx context.Context, r Remote, set ArtifactSet) error { } dir = expandVars(dir, env) if !path.IsAbs(dir) { - dir = path.Join(path.Dir(remote), dir) + dir = path.Join(path.Dir(set.Compose[0]), dir) // Compose resolves every fragment against the first file's directory } dir = path.Clean(dir) file := "" diff --git a/internal/deploy/remote_fixes_test.go b/internal/deploy/remote_fixes_test.go index 364f0f802..41ad84e7e 100644 --- a/internal/deploy/remote_fixes_test.go +++ b/internal/deploy/remote_fixes_test.go @@ -6,6 +6,7 @@ import ( "context" "errors" "os" + "path" "path/filepath" "strings" "testing" @@ -103,6 +104,27 @@ func TestRestoreRemovesWhatTheDeployCreated(t *testing.T) { } } +// D-0824: a relative build context of a later compose fragment resolves against +// the first file's directory, as Compose does, not the fragment's own. +func TestBuildContextRelativeToFirstComposeFile(t *testing.T) { + f, o, rp := applyFixture(t, "") + first := o.Set.Compose[0] + nested := path.Join(path.Dir(first), "plugins", "x", "compose.yml") + writeFile(t, filepath.Join(o.Set.StageDir, strings.TrimPrefix(nested, rp+"/")), "services:\n x:\n build: ./present\n", 0o644) + o.Set.Compose = append(o.Set.Compose, nested) + // present exists only under the first file's directory, not under plugins/x. + writeFile(t, filepath.Join(rp, "present", "Dockerfile"), "FROM scratch\n", 0o644) + if err := CheckBuildContexts(context.Background(), f, o.Set); err != nil { + t.Fatalf("a context relative to the first compose file was refused: %v", err) + } + // A directory that exists only next to the fragment is what Compose never reads. + writeFile(t, filepath.Join(rp, "plugins", "x", "only", "Dockerfile"), "FROM scratch\n", 0o644) + writeFile(t, filepath.Join(o.Set.StageDir, strings.TrimPrefix(nested, rp+"/")), "services:\n x:\n build: ./only\n", 0o644) + if err := CheckBuildContexts(context.Background(), f, o.Set); err == nil { + t.Fatal("a context that exists only next to the fragment passed") + } +} + // F8: a service whose build context the deploy cannot provide is refused before // the first remote write; a context that exists on the host passes. func TestBuildContextRefused(t *testing.T) { diff --git a/internal/deploy/remote_front.go b/internal/deploy/remote_front.go index 05720fb89..00f2a8c66 100644 --- a/internal/deploy/remote_front.go +++ b/internal/deploy/remote_front.go @@ -107,7 +107,8 @@ func describeRows(rows []psRow) string { } // checkMounts requires a bind mount of /nginx or /nginx/sites -// (given or real path), so a file shipped there is the file the container reads. +// (given or real path) at /etc/nginx or below it in the container, so a file +// shipped there is the file the container reads. func (c frontContainer) checkMounts(ctx context.Context, r Remote, front, realFront string, linked map[string]string) error { out, err := r.Run(ctx, shScript(fmt.Sprintf(`docker inspect --type container --format %s %s`, q(`{{range .Mounts}}{{.Type}}|{{.Source}}|{{.Destination}}{{"\n"}}{{end}}`), q(c.ID)))) if err != nil { @@ -130,6 +131,9 @@ func (c frontContainer) checkMounts(ctx context.Context, r Remote, front, realFr } src := path.Clean(parts[1]) seen = append(seen, safeText(parts[1])+" -> "+safeText(parts[2])) + if dst := path.Clean(parts[2]); dst != "/etc/nginx" && !strings.HasPrefix(dst, "/etc/nginx/") { + continue // the container does not read nginx config from there + } roots = append(roots, src) found = found || ok[src] for _, d := range frontDirs { // a mounted source that is itself a link reaches that link's real dir diff --git a/internal/deploy/remote_hardening_test.go b/internal/deploy/remote_hardening_test.go index a5e5f70be..3d4b60564 100644 --- a/internal/deploy/remote_hardening_test.go +++ b/internal/deploy/remote_hardening_test.go @@ -6,6 +6,7 @@ import ( "context" "os" "os/exec" + "path/filepath" "runtime" "strings" "testing" @@ -241,3 +242,57 @@ func TestEnvFilesAreSentWithChmod600(t *testing.T) { t.Errorf("rsync flags %v for lists %q: env files need --chmod=F600, others must not carry it", p.flags, p.lists) } } + +// D-0824: a failed rm for an earlier path is not hidden by a later path that +// succeeds (the script's exit status was the last loop iteration's). +func TestGuardedDeleteReportsEarlierFailure(t *testing.T) { + needTools(t) + if os.Geteuid() == 0 { + t.Skip("root ignores directory modes") + } + f := newFakeRemote(t) + rp := t.TempDir() + writeFile(t, rp+"/nginx/sites/a.conf", markedConf, 0o644) + writeFile(t, rp+"/nginx/other/b.conf", markedConf, 0o644) + if err := os.Chmod(rp+"/nginx/sites", 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(rp+"/nginx/sites", 0o755) }) + paths := []string{"nginx/sites/a.conf", "nginx/other/b.conf"} + if _, err := PruneMarked(context.Background(), f, rp, paths); err == nil { + t.Fatal("PruneMarked: a failed rm was reported as success") + } + if !exists(rp+"/nginx/sites/a.conf") || exists(rp+"/nginx/other/b.conf") { + t.Fatal("the failing path must stay and the other one must still be pruned") + } + writeFile(t, rp+"/nginx/other/b.conf", markedConf, 0o644) + if err := removeCreated(context.Background(), f, rp, paths); err == nil { + t.Fatal("removeCreated: a failed rm was reported as success") + } +} + +// D-0824: the shipped list is written to a fresh .new file; a symlink planted at +// that name is replaced, not followed. +func TestWriteShippedDoesNotFollowPlantedLink(t *testing.T) { + needTools(t) + f := newFakeRemote(t) + rp := t.TempDir() + victim := filepath.Join(t.TempDir(), "victim") + writeFile(t, victim, "keep me\n", 0o644) + if err := os.MkdirAll(rp+"/.nself", 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, rp+"/"+ShippedFile+".new"); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + set := ArtifactSet{RemotePath: rp, Files: []string{"nginx/sites/a.conf"}} + if err := writeShipped(context.Background(), f, set); err != nil { + t.Fatal(err) + } + if got := readFile(t, victim); got != "keep me\n" { + t.Errorf("the planted link was followed: %q", got) + } + if got := readFile(t, rp+"/"+ShippedFile); got != "nginx/sites/a.conf\n" { + t.Errorf("shipped list = %q", got) + } +} diff --git a/internal/deploy/remote_layout.go b/internal/deploy/remote_layout.go index fb97e82ab..74406c10c 100644 --- a/internal/deploy/remote_layout.go +++ b/internal/deploy/remote_layout.go @@ -87,7 +87,8 @@ exit 0`, q(remotePath), strings.Join(quoteAll(dirs), " "), strings.Join(quoteAll // remotePath. Every path is resolved first; one whose directory leaves the real // remote_path (or that is itself a symlink) prints "REFUSE " and nothing // is run. The directory is resolved again just before each act. A path whose -// directory does not exist is skipped. +// directory does not exist is skipped. Every path is tried; the script exits 1 +// if any act failed, not just the last one. func guardedScript(remotePath string, paths []string, body string) string { return fmt.Sprintf(`rp=%s; cd -P "$rp" || exit 1; base=$(pwd -P); set -- %s act() { %s; } @@ -95,7 +96,9 @@ real_of() { [ -d "$base/$(dirname "$1")" ] || return 2; x=$(cd -P "$base/$(dirna refuse=0 for f in "$@"; do if [ -L "$base/$f" ]; then printf 'REFUSE %%s\n' "$f"; refuse=1; continue; fi; real_of "$f" >/dev/null; [ $? -eq 1 ] && { printf 'REFUSE %%s\n' "$f"; refuse=1; }; done if [ "$refuse" = 1 ]; then exit 0; fi -for f in "$@"; do d=$(real_of "$f") || continue; act "$f" "$d/$(basename "$f")"; done`, q(remotePath), strings.Join(quoteAll(paths), " "), body) +rc=0 +for f in "$@"; do d=$(real_of "$f") || continue; act "$f" "$d/$(basename "$f")" || rc=1; done +exit $rc`, q(remotePath), strings.Join(quoteAll(paths), " "), body) } // refusedPaths turns the REFUSE lines of a guardedScript into an error. diff --git a/internal/deploy/remote_sync.go b/internal/deploy/remote_sync.go index 959db7806..aad8fb79b 100644 --- a/internal/deploy/remote_sync.go +++ b/internal/deploy/remote_sync.go @@ -75,8 +75,8 @@ func checkConfDFiles(files []string) error { func writeShipped(ctx context.Context, r Remote, set ArtifactSet) error { file := path.Join(set.RemotePath, ShippedFile) lines := append(append([]string{}, set.Files...), set.EnvFiles...) - script := fmt.Sprintf(`umask 077; mkdir -p %s; printf '%%s\n' %s > %s && mv -f %s %s`, - q(path.Dir(file)), strings.Join(quoteAll(lines), " "), q(file+".new"), q(file+".new"), q(file)) + script := fmt.Sprintf(`umask 077; mkdir -p %s; rm -f -- %s; printf '%%s\n' %s > %s && mv -f %s %s`, + q(path.Dir(file)), q(file+".new"), strings.Join(quoteAll(lines), " "), q(file+".new"), q(file+".new"), q(file)) if _, err := r.Run(ctx, shScript(script)); err != nil { return fmt.Errorf("recording the shipped file list on %s: %w", r.Label(), err) } diff --git a/internal/deploy/target_snapshot_front.go b/internal/deploy/target_snapshot_front.go index 2b8444bec..6c27bbc01 100644 --- a/internal/deploy/target_snapshot_front.go +++ b/internal/deploy/target_snapshot_front.go @@ -19,7 +19,7 @@ import ( // TakeFrontSSL lists the certificate dirs of the fronting stack at front. func TakeFrontSSL(ctx context.Context, r Remote, front string) ([]CertDir, error) { - pems, err := r.Run(ctx, shScript(fmt.Sprintf(`find -L %s -maxdepth 2 -name '*.pem' -print 2>/dev/null || true`, q(path.Join(front, "ssl", "certificates"))))) + pems, err := r.Run(ctx, shScript(fmt.Sprintf(`d=%s; [ -d "$d" ] || exit 0; find -L "$d" -maxdepth 2 -name '*.pem' -print`, q(path.Join(front, "ssl", "certificates"))))) if err != nil { return nil, fmt.Errorf("target snapshot: listing the fronting stack's certificates: %w", err) } diff --git a/internal/deploy/target_snapshot_front_test.go b/internal/deploy/target_snapshot_front_test.go index c8e17cb1f..8958eaf47 100644 --- a/internal/deploy/target_snapshot_front_test.go +++ b/internal/deploy/target_snapshot_front_test.go @@ -2,6 +2,7 @@ package deploy import ( "context" + "os" "path/filepath" "strings" "testing" @@ -48,3 +49,22 @@ func TestTargetSnapshotFrontSSL(t *testing.T) { t.Errorf("unsafe cert dir name: %v", err) } } + +// D-0824: only a missing certificates dir is empty; a find that cannot read one +// is an error, not "no certificates". +func TestTargetSnapshotFrontSSLFindErrorIsNotEmpty(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads every directory") + } + f := newFakeRemote(t) + front := t.TempDir() + certs := filepath.Join(front, "ssl", "certificates") + writeFile(t, filepath.Join(certs, "api-example-com", "fullchain.pem"), "CERT", 0o644) + if err := os.Chmod(certs, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(certs, 0o755) }) + if got, err := TakeFrontSSL(context.Background(), f, front); err == nil { + t.Errorf("an unreadable certificates dir listed as %+v with no error", got) + } +} From 0445a058f97d057da84dfdf6e80ca815ffc323b7 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 17:05:00 -0400 Subject: [PATCH 6/6] refactor(deploy): extract front-check helpers to internal/frontcheck to satisfy package-size ratchet --- internal/deploy/remote_front_check.go | 21 ++++------ internal/deploy/remote_front_guard.go | 59 +++------------------------ internal/frontcheck/conf.go | 46 +++++++++++++++++++++ internal/frontcheck/prunebody.go | 13 ++++++ internal/frontcheck/samename.go | 11 +++++ 5 files changed, 84 insertions(+), 66 deletions(-) create mode 100644 internal/frontcheck/conf.go create mode 100644 internal/frontcheck/prunebody.go create mode 100644 internal/frontcheck/samename.go diff --git a/internal/deploy/remote_front_check.go b/internal/deploy/remote_front_check.go index 9160178ef..9bff213f0 100644 --- a/internal/deploy/remote_front_check.go +++ b/internal/deploy/remote_front_check.go @@ -22,6 +22,7 @@ import ( "strings" "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/frontcheck" ) // frontView is what the host-file checks need to know. @@ -32,12 +33,6 @@ type frontView struct { Names map[string][]string // that record's server_name set per path; a missing key is "unknown" } -// sameNames reports whether two server_name sets are equal. An empty set is no -// evidence of ownership, so it equals nothing, not even another empty set. -func sameNames(a, b []string) bool { - return len(a) > 0 && len(b) > 0 && strings.Join(a, " ") == strings.Join(b, " ") -} - // checkFrontForeign refuses (E485, nothing sent) to replace a host file that is // not this project's. func checkFrontForeign(ctx context.Context, r Remote, front string, v frontView) error { @@ -54,7 +49,7 @@ func checkFrontForeign(ctx context.Context, r Remote, front string, v frontView) if !exists { continue } - if !hasBuildMarker(conf) { + if !frontcheck.HasBuildMarker(conf, BuildNginxMarker) { return errs.New("E485", fmt.Sprintf("unmarked file would be overwritten: %q in the fronting stack", p)). WithFix("move or delete that hand-written file on the host (or add the nself generated marker to it), then deploy again; nothing was sent") } @@ -62,12 +57,12 @@ func checkFrontForeign(ctx context.Context, r Remote, front string, v frontView) if err != nil { return err } - theirs := serverNameSet(conf) + theirs := frontcheck.ServerNameSet(conf) if rec, have := v.Names[p]; known[p] && have { - if sameNames(theirs, rec) || sameNames(theirs, ours) { + if frontcheck.SameNames(theirs, rec) || frontcheck.SameNames(theirs, ours) { continue } - } else if sameNames(theirs, ours) { + } else if frontcheck.SameNames(theirs, ours) { continue } return errs.New("E485", fmt.Sprintf("%q in the fronting stack is a generated file of another project: its server_name set is [%s], this deploy ships [%s]", @@ -102,13 +97,13 @@ func frontPrunable(ctx context.Context, r Remote, front string, v frontView) (pr } rec, have := v.Names[p] switch { - case !hasBuildMarker(conf): + case !frontcheck.HasBuildMarker(conf, BuildNginxMarker): kept = append(kept, fmt.Sprintf("[kept] %s/%s (no generated marker; not pruned)", front, p)) case !have: kept = append(kept, fmt.Sprintf("[kept] %s/%s (no server_name set recorded for it; not pruned)", front, p)) - case len(rec) == 0 || len(serverNameSet(conf)) == 0: + case len(rec) == 0 || len(frontcheck.ServerNameSet(conf)) == 0: kept = append(kept, fmt.Sprintf("[kept] %s/%s (an empty server_name set proves no ownership; not pruned)", front, p)) - case !sameNames(serverNameSet(conf), rec): + case !frontcheck.SameNames(frontcheck.ServerNameSet(conf), rec): kept = append(kept, fmt.Sprintf("[kept] %s/%s (server_name changed since this project shipped it; not pruned)", front, p)) default: prune = append(prune, p) diff --git a/internal/deploy/remote_front_guard.go b/internal/deploy/remote_front_guard.go index e53486f93..8ff1c1067 100644 --- a/internal/deploy/remote_front_guard.go +++ b/internal/deploy/remote_front_guard.go @@ -28,47 +28,10 @@ import ( "io" "os" "path/filepath" - "regexp" - "sort" "strings" -) - -// frontConfMax caps how much of a host conf is read. -const frontConfMax = 262144 - -var serverNameRe = regexp.MustCompile(`\bserver_name[ \t]+([^;]+);`) - -// hasBuildMarker is the strict marker rule on conf text. -func hasBuildMarker(conf string) bool { - for i, line := range strings.Split(conf, "\n") { - if i >= 5 { - break - } - if strings.HasPrefix(line, BuildNginxMarker) { - return true - } - } - return false -} -// serverNameSet is the sorted, unique server_name values of the conf (comments dropped). -func serverNameSet(conf string) []string { - seen := map[string]bool{} - for _, line := range strings.Split(conf, "\n") { - line, _, _ = strings.Cut(line, "#") - for _, m := range serverNameRe.FindAllStringSubmatch(line, -1) { - for _, n := range strings.Fields(m[1]) { - seen[n] = true - } - } - } - out := make([]string, 0, len(seen)) - for n := range seen { - out = append(out, n) - } - sort.Strings(out) - return out -} + "github.com/nself-org/cli/internal/frontcheck" +) // readFrontConfs reads (head -c) the existing regular, non-symlink files among // paths in the front root, in one quoted script. A nonce line frames each file. @@ -81,7 +44,7 @@ func readFrontConfs(ctx context.Context, r Remote, front string, paths []string) _, _ = rand.Read(nb[:]) nonce := "@@" + hex.EncodeToString(nb[:]) script := fmt.Sprintf(`cd %s 2>/dev/null || exit 0; for f in %s; do if [ -f "$f" ] && [ ! -L "$f" ]; then printf '%%s %%s\n' %s "$f"; head -c %d -- "$f"; printf '\n'; fi; done; exit 0`, - q(front), strings.Join(quoteAll(paths), " "), q(nonce), frontConfMax) + q(front), strings.Join(quoteAll(paths), " "), q(nonce), frontcheck.ConfMax) raw, err := r.Run(ctx, shScript(script)) if err != nil { return nil, fmt.Errorf("reading the fronting stack's existing sites on %s: %w", r.Label(), err) @@ -112,11 +75,11 @@ func localServerNames(stage, rel string) ([]string, error) { return nil, err } defer func() { _ = f.Close() }() - b, err := io.ReadAll(io.LimitReader(f, frontConfMax)) + b, err := io.ReadAll(io.LimitReader(f, frontcheck.ConfMax)) if err != nil { return nil, err } - return serverNameSet(string(b)), nil + return frontcheck.ServerNameSet(string(b)), nil } // pruneFrontMarked removes, from the candidates (front paths this host's record @@ -141,7 +104,7 @@ func pruneFrontMarked(ctx context.Context, r Remote, front string, candidates [] fmt.Fprintf(&want, "%s) want=%s;; ", q(f), q(strings.Join(set, " "))) } } - body := strings.NewReplacer("@WANT@", want.String(), "@PAT@", pat, "@MAX@", fmt.Sprint(frontConfMax)).Replace(frontPruneBody) + body := strings.NewReplacer("@WANT@", want.String(), "@PAT@", pat, "@MAX@", fmt.Sprint(frontcheck.ConfMax)).Replace(frontcheck.PruneBody) out, err := r.Run(ctx, shScript(guardedScript(front, cand[:n], body))) if err != nil { return removed, kept, fmt.Errorf("pruning generated files on %s: %w", r.Label(), err) @@ -164,13 +127,3 @@ func pruneFrontMarked(ctx context.Context, r Remote, front string, candidates [] } return removed, kept, nil } - -// frontPruneBody is the per-path prune step: the marker rule, then the same -// server_name set as serverNameSet (comments dropped, sorted, unique) compared -// with the recorded one. A path with no recorded set is never removed. -const frontPruneBody = `if [ -f "$2" ] && head -n 5 -- "$2" 2>/dev/null | grep -q @PAT@; then -want=; case "$1" in @WANT@esac -got=$(head -c @MAX@ -- "$2" 2>/dev/null | sed 's/#.*//' | grep -oE '(^|[^[:alnum:]_])server_name[[:space:]]+[^;]+;' | sed -E 's/^.*server_name[[:space:]]+//; s/;$//' | tr -s '[:space:]' '\n' | grep -v '^$' | LC_ALL=C sort -u | tr '\n' ' ') -got=${got% } -if [ -n "$want" ] && [ "$got" = "$want" ]; then rm -f -- "$2" && printf '%s\n' "$1"; else printf 'KEPT %s\n' "$1"; fi -fi` diff --git a/internal/frontcheck/conf.go b/internal/frontcheck/conf.go new file mode 100644 index 000000000..4f62373bc --- /dev/null +++ b/internal/frontcheck/conf.go @@ -0,0 +1,46 @@ +// Copyright © 2024 nself-org. All rights reserved. + +package frontcheck + +import ( + "regexp" + "sort" + "strings" +) + +// ConfMax caps how much of a host conf is read. +const ConfMax = 262144 + +var serverNameRe = regexp.MustCompile(`\bserver_name[ \t]+([^;]+);`) + +// HasBuildMarker is the strict marker rule on conf text. +func HasBuildMarker(conf, marker string) bool { + for i, line := range strings.Split(conf, "\n") { + if i >= 5 { + break + } + if strings.HasPrefix(line, marker) { + return true + } + } + return false +} + +// ServerNameSet is the sorted, unique server_name values of the conf (comments dropped). +func ServerNameSet(conf string) []string { + seen := map[string]bool{} + for _, line := range strings.Split(conf, "\n") { + line, _, _ = strings.Cut(line, "#") + for _, m := range serverNameRe.FindAllStringSubmatch(line, -1) { + for _, n := range strings.Fields(m[1]) { + seen[n] = true + } + } + } + out := make([]string, 0, len(seen)) + for n := range seen { + out = append(out, n) + } + sort.Strings(out) + return out +} diff --git a/internal/frontcheck/prunebody.go b/internal/frontcheck/prunebody.go new file mode 100644 index 000000000..63650e8d1 --- /dev/null +++ b/internal/frontcheck/prunebody.go @@ -0,0 +1,13 @@ +// Copyright © 2024 nself-org. All rights reserved. + +package frontcheck + +// PruneBody is the per-path prune step: the marker rule, then the same +// server_name set as ServerNameSet (comments dropped, sorted, unique) compared +// with the recorded one. A path with no recorded set is never removed. +const PruneBody = `if [ -f "$2" ] && head -n 5 -- "$2" 2>/dev/null | grep -q @PAT@; then +want=; case "$1" in @WANT@esac +got=$(head -c @MAX@ -- "$2" 2>/dev/null | sed 's/#.*//' | grep -oE '(^|[^[:alnum:]_])server_name[[:space:]]+[^;]+;' | sed -E 's/^.*server_name[[:space:]]+//; s/;$//' | tr -s '[:space:]' '\n' | grep -v '^$' | LC_ALL=C sort -u | tr '\n' ' ') +got=${got% } +if [ -n "$want" ] && [ "$got" = "$want" ]; then rm -f -- "$2" && printf '%s\n' "$1"; else printf 'KEPT %s\n' "$1"; fi +fi` diff --git a/internal/frontcheck/samename.go b/internal/frontcheck/samename.go new file mode 100644 index 000000000..e214e9420 --- /dev/null +++ b/internal/frontcheck/samename.go @@ -0,0 +1,11 @@ +// Copyright © 2024 nself-org. All rights reserved. + +package frontcheck + +import "strings" + +// SameNames reports whether two server_name sets are equal. An empty set is no +// evidence of ownership, so it equals nothing, not even another empty set. +func SameNames(a, b []string) bool { + return len(a) > 0 && len(b) > 0 && strings.Join(a, " ") == strings.Join(b, " ") +}