audit 3: backup CLI, envelope budget, directory sync, docs; findings of the reviews

Steps 3-6 of stage 3 and what the two reviews found in steps 1-6, in one commit: the review fixes sit in the same hunks. Step 3, CLI backup (M3, M5, L1, L2; L4 of the stage's list): - backup.snapshotDB refuses a database that is not there and opens the source mode=ro (measured: mode=ro creates no file). - selfpost-backup holds an encrypting password to validate.MinSecretFilePasswordLen; -o of both directions writes a temporary file beside the name and renames it whole; -decrypt peeks the first chunk (password and type authenticated) before the output exists. - what a backup leaves out (a link under data/, a link under certs/ to a directory, a socket...) is told to OnWarn; certs/ with no file is backup.ErrNoCerts, refused by Check and Create, named by the panel. - parseTrustedProxies: a bare address takes /128 when written with a colon (::ffff:10.0.0.1/32 was ::/32). Step 4, secretfile (M4, L3): scrypt's V at most 128 MiB, r <= 64, p <= 2; bytes after the last chunk are ErrCorrupt. Step 5 (L7): atomicfile.SyncDir after the rename, used by Write, replaceSASLDB and the CLI; atomicfile_test.go. Step 6: security.md and guide on backup and restore. Reviews (technical, security): - H1: a restore of a server with DMARC on was refused on every start - the archive records no owner, tar as root left the DMARC maps root's, and the entrypoint only chowned the maps it named. It now hands every file beside queue/ in /data/postfix to panel:selfpost 0640; e2e writes an extra map before the backup and finds it panel:selfpost 640 after a root unpack. - the comparison reads regular files only (Lstat, size, LimitReader); a record unpacked again wins over the earlier check's mark; ConsumeRestore removes both; the mark is synced; a journal file beside a freshly unpacked database is refused. - AttachRefusalLine refuses an empty line as the old statement did; skipped entries named in words; -decrypt of an empty payload is no failure; guide wording on the shell's '>' and a killed CLI; the in-place restore's 'mv .' (older than this stage) fixed. Tested on the test server: go vet + go test of 25 packages, make e2e (all pass, 643 s) on this tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-15 10:38:39 +03:00
parent 508ac96c79
commit 242515c305
24 changed files with 901 additions and 90 deletions
@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"compress/gzip"
 	"crypto/sha256"
+	"database/sql"
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
@@ -522,3 +523,143 @@ func TestCertsLinksAreFollowed(t *testing.T) {
 		t.Fatalf("Check with a dangling link: %v", err)
 	}
 }
+
+// A wrong SELFPOST_DB_PATH is refused, and nothing is created where it
+// points: SQLite makes a database of a path that is not one, and an archive
+// of that empty database restores a server that offers its first-run setup
+// (audit 2026-10-12, M3). The snapshot opens the source read-only, which is
+// what refuses it should the file go between the check and the open.
+func TestSnapshotRefusesAMissingDatabase(t *testing.T) {
+	dataDir, _, deployRoot := seedDataDir(t)
+	missing := filepath.Join(dataDir, "elsewhere.db")
+	var buf bytes.Buffer
+	err := Create(&buf, Params{DataDir: dataDir, DBPath: missing, Version: "1.0.0", DeployRoot: deployRoot})
+	if err == nil {
+		t.Fatal("a backup of a database that is not there was made")
+	}
+	if _, statErr := os.Stat(missing); !os.IsNotExist(statErr) {
+		t.Fatalf("the refused backup created %s (stat err = %v)", missing, statErr)
+	}
+
+	db, err := sql.Open("sqlite", "file:"+missing+"?mode=ro")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer db.Close()
+	if err := db.Ping(); err == nil {
+		t.Fatal("mode=ro opened a database that is not there")
+	}
+	if _, statErr := os.Stat(missing); !os.IsNotExist(statErr) {
+		t.Fatalf("mode=ro created %s (stat err = %v)", missing, statErr)
+	}
+}
+
+// certs/ there and empty is refused by Check and Create alike: the archive
+// would restore a server without its certificate (audit 2026-10-12, L2).
+func TestEmptyCertsIsRefused(t *testing.T) {
+	dataDir, dbPath, deployRoot := seedDataDir(t)
+	for _, f := range []string{"fullchain.pem", "privkey.pem"} {
+		os.Remove(filepath.Join(deployRoot, CertsDirName, f))
+	}
+	p := Params{DataDir: dataDir, DBPath: dbPath, Version: "1.0.0", DeployRoot: deployRoot}
+	if err := Check(p); err == nil || !strings.Contains(err.Error(), "certs/ holds no file") {
+		t.Errorf("Check with an empty certs/ = %v", err)
+	}
+	if err := Create(io.Discard, p); err == nil || !strings.Contains(err.Error(), "certs/ holds no file") {
+		t.Errorf("Create with an empty certs/ = %v", err)
+	}
+}
+
+// What a backup leaves out it says it leaves out: a link under data/, and a
+// link under certs/ to a directory (audit 2026-10-12, L2).
+func TestBackupWarnsOfWhatItLeavesOut(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("symbolic links need privileges on Windows; the container is Linux")
+	}
+	dataDir, dbPath, deployRoot := seedDataDir(t)
+	if err := os.Symlink("sasl/sasldb2", filepath.Join(dataDir, "a-link")); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(t.TempDir(), filepath.Join(deployRoot, CertsDirName, "a-directory")); err != nil {
+		t.Fatal(err)
+	}
+	var warnings []string
+	p := Params{DataDir: dataDir, DBPath: dbPath, Version: "1.0.0", DeployRoot: deployRoot,
+		OnWarn: func(s string) { warnings = append(warnings, s) }}
+	if err := Create(io.Discard, p); err != nil {
+		t.Fatalf("Create: %v", err)
+	}
+	got := strings.Join(warnings, "\n")
+	for _, want := range []string{
+		"data/a-link is a link, not a file: the backup does not carry it",
+		"certs/a-directory is a link to a directory, not a file: the backup does not carry it",
+	} {
+		if !strings.Contains(got, want) {
+			t.Errorf("no warning %q in:\n%s", want, got)
+		}
+	}
+}
+
+// A link or a pipe where the record names a file is refused, not followed or
+// read: a crafted archive must not make the start read outside /data, or
+// wait (security review of audit stage 3).
+func TestCheckRestoreReadsOnlyRegularFiles(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("symbolic links need privileges on Windows; the container is Linux")
+	}
+	path := unpackData(t, createArchive(t, "1.0.0"))
+	dir := filepath.Dir(path)
+	sasl := filepath.Join(dir, "sasl", "sasldb2")
+	outside := filepath.Join(t.TempDir(), "outside")
+	writeFile(t, outside, "SASLDB")
+	os.Remove(sasl)
+	if err := os.Symlink(outside, sasl); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := CheckRestore(path, "1.0.0"); err == nil || !strings.Contains(err.Error(), "data/sasl/sasldb2 is not the file") {
+		t.Fatalf("a link to a file of the same contents = %v, want refused", err)
+	}
+}
+
+// A journal file beside the restored database is from what the directory
+// held before; SQLite would take it up, so the restore is refused.
+func TestCheckRestoreRefusesAJournalBesideTheDatabase(t *testing.T) {
+	for _, side := range []string{"-wal", "-shm", "-journal"} {
+		path := unpackData(t, createArchive(t, "1.0.0"))
+		writeFile(t, filepath.Join(filepath.Dir(path), "selfpost.db"+side), "old")
+		if _, err := CheckRestore(path, "1.0.0"); err == nil || !strings.Contains(err.Error(), "selfpost.db"+side+" is beside the database") {
+			t.Errorf("%s beside the database = %v, want refused", side, err)
+		}
+	}
+}
+
+// The archive unpacked again after a start that checked it is checked again:
+// its record wins over the mark of the earlier check, and a file it cut short
+// is refused. A finished restore leaves neither record.
+func TestARecordUnpackedAgainIsChecked(t *testing.T) {
+	archive := createArchive(t, "1.0.0")
+	path := unpackData(t, archive)
+	dir := filepath.Dir(path)
+	if _, err := CheckRestore(path, "1.0.0"); err != nil {
+		t.Fatal(err)
+	}
+	files := readArchive(t, archive)
+	writeFile(t, filepath.Join(dir, ContentsName), files[DataArchivePrefix+ContentsName])
+	writeFile(t, filepath.Join(dir, "sasl", "sasldb2"), "SAS")
+	if _, err := CheckRestore(path, "1.0.0"); err == nil || !strings.Contains(err.Error(), "data/sasl/sasldb2 is not the file") {
+		t.Fatalf("a second unpack cut short = %v, want refused", err)
+	}
+	writeFile(t, filepath.Join(dir, "sasl", "sasldb2"), files[DataArchivePrefix+"sasl/sasldb2"])
+	if _, err := CheckRestore(path, "1.0.0"); err != nil {
+		t.Fatalf("the second unpack whole: %v", err)
+	}
+	writeFile(t, filepath.Join(dir, ContentsName), files[DataArchivePrefix+ContentsName])
+	if err := ConsumeRestore(path); err != nil {
+		t.Fatal(err)
+	}
+	for _, name := range []string{ManifestName, ContentsName, contentsCheckedName} {
+		if _, err := os.Stat(filepath.Join(dir, name)); !os.IsNotExist(err) {
+			t.Errorf("%s left after ConsumeRestore", name)
+		}
+	}
+}