From 242515c305cdb30dcf383773930d37707c60a4bb Mon Sep 17 00:00:00 2001 From: Mix Date: Thu, 15 Oct 2026 10:38:39 +0300 Subject: [PATCH] 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 --- CHANGELOG.md | 53 ++++++- build/entrypoint.sh | 13 +- cmd/panel/main.go | 13 +- cmd/panel/proxies_test.go | 36 +++++ cmd/selfpost-backup/main.go | 96 ++++++++++-- cmd/selfpost-backup/main_test.go | 73 +++++++++ docs/audit-2026-10-12.md | 37 ++++- docs/guide.md | 43 +++++- docs/roadmap.md | 2 +- docs/schema-migrations.md | 2 +- docs/security.md | 32 +++- internal/app/sasl.go | 7 +- internal/atomicfile/atomicfile.go | 24 ++- internal/atomicfile/atomicfile_test.go | 113 ++++++++++++++ internal/backup/backup.go | 97 ++++++++++-- internal/backup/backup_test.go | 141 ++++++++++++++++++ internal/backup/contents.go | 59 ++++++-- internal/secretfile/secretfile.go | 35 ++++- internal/secretfile/secretfile_test.go | 66 +++++++- internal/store/queryplan_test.go | 15 +- internal/store/sendlog.go | 6 + internal/web/handlers/handlers_backup.go | 4 + internal/web/handlers/handlers_backup_test.go | 10 ++ test/e2e/restore_test.go | 14 ++ 24 files changed, 901 insertions(+), 90 deletions(-) create mode 100644 cmd/panel/proxies_test.go create mode 100644 internal/atomicfile/atomicfile_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index beebb5d..c70ade9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,46 @@ Migration `0010` (indexes only). domain's rows; a page of a domain administrator's log sorted the domain's whole log. Migration `0010` adds four indexes, and a test now holds each of these statements to its index (audit 2026-10-12, M2). - +- **`selfpost-backup` with a wrong `SELFPOST_DB_PATH` made a backup of + nothing.** SQLite created an empty database where the path pointed, and + the archive of it passed as a backup - one that restores a server offering + its first-run setup while it holds live DKIM keys. A database that is not + there is now refused, and the snapshot opens it read-only (audit + 2026-10-12, M3). +- **The CLI encrypted a backup with any password.** It now holds the + password to the panel's floor of 12 characters (audit 2026-10-12, M5). + Decrypting takes any password, as before. +- **`selfpost-backup -o FILE` left a half-written file** when it failed, + and `-decrypt -o FILE` with a wrong password emptied a file that may have + been a good archive. The output is written beside its name and takes it + only when whole; `-decrypt` checks the password on the first block before + it touches the output (audit 2026-10-12, L1). +- **A backup left links out without a word, and an empty `certs/` passed.** + An entry a backup does not carry - a link under `data/`, a link under + `certs/` to a directory - is now named in a warning (the CLI's standard + error, the panel's log), and a `certs/` with no file in it is refused: the + archive would restore a server without its certificate (audit + 2026-10-12, L2). +- **`TRUSTED_PROXY_CIDR=::ffff:10.0.0.1` trusted the wrong addresses.** A + bare IPv4-mapped address written as IPv6 was given "/32", which made it + `::/32`: every address beginning with 32 zero bits, `::1` among them, and + not the proxy meant. A bare address is now one host however it is written + (audit 2026-10-12, L4). +- **An encrypted file could make the panel allocate 4 GiB before its + password was checked.** The key-derivation parameters an encrypted backup + or export carries were capped so loosely that a crafted header, uploaded + to *Import a domain*, asked scrypt for 4 GiB and sixteen times the work. + A header may now ask for 128 MiB at most - four times what SelfPost + writes, with the rest of scrypt's memory under 32 KiB - and is refused as + damaged otherwise. Bytes after an envelope's + last block, such as a second file appended to it, are refused too (audit + 2026-10-12, M4, L3). +- **A power loss could bring back a map the panel had just replaced.** The + files the panel writes for Postfix and OpenDKIM were synced, but not the + directory their rename changed, so the old file could return while the + database said otherwise. The directory is synced after the rename now, as + `sasldb2` has been since 2.5.2, and the CLI's `-o` file too (audit + 2026-10-12, L7). - **A restore whose first start failed never ran its Resync.** The panel removed the restore manifest as soon as it had checked the version, before it opened the database and regenerated the mail maps from it; a start that @@ -31,9 +70,15 @@ Migration `0010` (indexes only). there. Every archive now ends with `data/contents.json`, the size and SHA-256 of each file under `data/`, and the first start after a restore compares `/data` with it before it opens anything: an archive unpacked in - part, or a file that differs, is refused with a message that names it, and - the directory is left as it was (guide § Restore). A damaged or cut domain - export was already refused before anything is created; a test now says so. + part, a file that differs, or a database journal file left beside the + database from before, is refused with a message that names it, and the + directory is left as it was (guide § Restore). The entrypoint now hands + every map under `data/postfix` to the panel, whatever its name - a restore + unpacked as root left the DMARC maps root's, and the comparison could not + read them. A damaged or cut domain export was already refused before + anything is created; a test now says so. +- **The guide's in-place restore moved `.`**, which `mv` refuses; it now + steps out of the project directory first. ## [2.5.2] - 2026-10-15 diff --git a/build/entrypoint.sh b/build/entrypoint.sh index a870261..5d427be 100644 --- a/build/entrypoint.sh +++ b/build/entrypoint.sh @@ -114,9 +114,16 @@ mkdir -p /data/postfix/queue # Empty files keep postfix check happy if the flag is on before any domain exists. for f in relay_domains transport relay_recipients tls_policy; do [ -e "/data/postfix/$f" ] || : > "/data/postfix/$f" - chown panel:selfpost "/data/postfix/$f" - chmod 0640 "/data/postfix/$f" done +# Every map here is the panel's to write and Postfix's to read - the sender +# map, the inbound and the DMARC maps alike - so each file beside queue/ is +# handed to the panel, group selfpost, whatever its name. A restored archive's +# files arrive owned by whoever unpacked them (a backup records no owner, and +# tar run as root makes them root's), and the first start after a restore +# reads every one of them to compare it with the archive's record: one left +# to root alone is a restore refused on every start (review of audit stage 3, +# H1 - the DMARC maps were not among the names listed here). +find /data/postfix -mindepth 1 -maxdepth 1 -type f -exec chown panel:selfpost {} + -exec chmod 0640 {} + chown panel:selfpost /data/postfix chmod 2750 /data/postfix @@ -128,8 +135,6 @@ chmod 2750 /data/postfix mkdir -p /data/antispam chown panel:selfpost /data/antispam chmod 0755 /data/antispam -chown panel:selfpost /data/postfix/sender_login_maps -chmod 0640 /data/postfix/sender_login_maps # Delivery log (architecture.md § Log tailer). postlogd writes it as user # `postfix`; the panel reads it for the log-tailer and the System log page. It diff --git a/cmd/panel/main.go b/cmd/panel/main.go index e6fae7d..76e9d65 100644 --- a/cmd/panel/main.go +++ b/cmd/panel/main.go @@ -265,10 +265,17 @@ func parseTrustedProxies(raw string) []*net.IPNet { continue } if !strings.Contains(cidr, "/") { - if ip := net.ParseIP(cidr); ip != nil && ip.To4() != nil { - cidr += "/32" - } else { + // The prefix length goes with how the address is written: an + // IPv4-mapped address written as IPv6 (::ffff:10.0.0.1) is a + // 128-bit address, and "/32" on it is ::/32 - every address + // whose first 32 bits are zero (::1 among them) trusted, and the + // host meant not at all (audit 2026-10-12, L4). Go 1.26 + // net.IPNet.Contains compares an IPv4 client with the mapped + // /128 as with 10.0.0.1/32 (net/ip.go, networkNumberAndMask). + if strings.Contains(cidr, ":") { cidr += "/128" + } else { + cidr += "/32" } } _, n, err := net.ParseCIDR(cidr) diff --git a/cmd/panel/proxies_test.go b/cmd/panel/proxies_test.go new file mode 100644 index 0000000..e1ee9c9 --- /dev/null +++ b/cmd/panel/proxies_test.go @@ -0,0 +1,36 @@ +package main + +import ( + "net" + "testing" +) + +// A bare address is one host however it is written; an IPv4-mapped one in +// IPv6 notation used to become ::/32 (audit 2026-10-12, L4). +func TestParseTrustedProxiesTakesABareAddressAsOneHost(t *testing.T) { + for _, c := range []struct { + entry string + trusts []string + refuses []string + }{ + {"10.0.0.1", []string{"10.0.0.1"}, []string{"10.0.0.2"}}, + {"::ffff:10.0.0.1", []string{"10.0.0.1", "::ffff:10.0.0.1"}, []string{"10.0.0.2", "::1", "0:0:1::1"}}, + {"2001:db8::1", []string{"2001:db8::1"}, []string{"2001:db8::2"}}, + {"10.0.0.0/8", []string{"10.1.2.3"}, []string{"11.0.0.1"}}, + } { + nets := parseTrustedProxies(c.entry) + if len(nets) != 1 { + t.Fatalf("%q parsed as %v", c.entry, nets) + } + for _, ip := range c.trusts { + if !nets[0].Contains(net.ParseIP(ip)) { + t.Errorf("%q (%v) does not trust %s", c.entry, nets[0], ip) + } + } + for _, ip := range c.refuses { + if nets[0].Contains(net.ParseIP(ip)) { + t.Errorf("%q (%v) trusts %s", c.entry, nets[0], ip) + } + } + } +} diff --git a/cmd/selfpost-backup/main.go b/cmd/selfpost-backup/main.go index 6e81d3c..92992b4 100644 --- a/cmd/selfpost-backup/main.go +++ b/cmd/selfpost-backup/main.go @@ -21,16 +21,21 @@ package main import ( + "bufio" + "errors" "flag" "fmt" "io" "os" "path/filepath" "strings" + "unicode/utf8" + "github.com/mixeme/selfpost/internal/atomicfile" "github.com/mixeme/selfpost/internal/backup" "github.com/mixeme/selfpost/internal/buildinfo" "github.com/mixeme/selfpost/internal/secretfile" + "github.com/mixeme/selfpost/internal/web/validate" ) // passwordEnv names the environment variable holding the encryption password. @@ -70,13 +75,20 @@ func run(outPath, password string) error { dataDir := envDefault("SELFPOST_DATA_DIR", "/data") dbPath := envDefault("SELFPOST_DB_PATH", filepath.Join(dataDir, "selfpost.db")) - w, closeOut, err := openOutput(outPath) + // The floor the panel holds the same files to: an encrypted backup is + // attacked offline, at leisure (security.md; audit 2026-10-12, M5). An + // empty password is no encryption, by design. + if password != "" && utf8.RuneCountInString(password) < validate.MinSecretFilePasswordLen { + return fmt.Errorf("the encryption password must be at least %d characters", validate.MinSecretFilePasswordLen) + } + + w, err := openOutput(outPath) if err != nil { return err } - defer closeOut() + defer w.abort() - sink := w + var sink io.Writer = w var env *secretfile.Writer if password != "" { env, err = secretfile.NewWriter(w, secretfile.TypeFullBackup, password) @@ -102,6 +114,9 @@ func run(outPath, password string) error { return err } } + if err := w.commit(); err != nil { + return err + } if outPath != "" { kind := "plain" if password != "" { @@ -133,17 +148,28 @@ func runDecrypt(inPath, outPath, password string) error { if err != nil { return err } + // The first chunk is opened before anything is written: a wrong password + // is told before the output is touched, and the envelope's type is + // authenticated only once a chunk opens (secretfile.Reader.Type; audit + // 2026-10-12, L1). + plain := bufio.NewReaderSize(env, 64<<10) + if _, err := plain.Peek(1); err != nil && !errors.Is(err, io.EOF) { + return err + } if env.Type() != secretfile.TypeFullBackup { return fmt.Errorf("that file is a %s, not a full backup", env.Type()) } - w, closeOut, err := openOutput(outPath) + w, err := openOutput(outPath) if err != nil { return err } - defer closeOut() + defer w.abort() - if _, err := io.Copy(w, env); err != nil { + if _, err := io.Copy(w, plain); err != nil { + return err + } + if err := w.commit(); err != nil { return err } if outPath != "" { @@ -152,17 +178,61 @@ func runDecrypt(inPath, outPath, password string) error { return nil } -// openOutput returns stdout, or a freshly created owner-only file: both plain -// and encrypted backups are secret enough to keep off other users' eyes. -func openOutput(outPath string) (io.Writer, func(), error) { +// output is where the CLI writes: stdout, or a file that takes its name only +// once it is whole. A backup or a decryption that fails half way - a wrong +// password, an envelope cut short - leaves whatever had the name before, and +// no half-written file under it (audit 2026-10-12, L1 and stage 3). The file +// is the owner's alone (os.CreateTemp makes it 0600): plain and encrypted +// backups are secret enough to keep off other users' eyes. +type output struct { + io.Writer + tmp *os.File // nil for stdout + path string +} + +func openOutput(outPath string) (*output, error) { if outPath == "" { - return os.Stdout, func() {}, nil + return &output{Writer: os.Stdout}, nil } - f, err := os.OpenFile(outPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + f, err := os.CreateTemp(filepath.Dir(outPath), "."+filepath.Base(outPath)+".tmp-*") if err != nil { - return nil, nil, err + return nil, err } - return f, func() { _ = f.Close() }, nil + return &output{Writer: f, tmp: f, path: outPath}, nil +} + +// commit puts the whole file under its name. +func (o *output) commit() error { + if o.tmp == nil { + return nil + } + f := o.tmp + o.tmp = nil + if err := f.Sync(); err != nil { + f.Close() + os.Remove(f.Name()) + return err + } + if err := f.Close(); err != nil { + os.Remove(f.Name()) + return err + } + if err := os.Rename(f.Name(), o.path); err != nil { + os.Remove(f.Name()) + return err + } + _ = atomicfile.SyncDir(filepath.Dir(o.path)) + return nil +} + +// abort removes a file that was not committed; after commit it does nothing. +func (o *output) abort() { + if o.tmp == nil { + return + } + o.tmp.Close() + os.Remove(o.tmp.Name()) + o.tmp = nil } // readPassword takes the password from the given file (first line) or, when no diff --git a/cmd/selfpost-backup/main_test.go b/cmd/selfpost-backup/main_test.go index 6d9e56c..dc2f243 100644 --- a/cmd/selfpost-backup/main_test.go +++ b/cmd/selfpost-backup/main_test.go @@ -162,3 +162,76 @@ func TestReadPassword(t *testing.T) { t.Error("-decrypt without a password was accepted") } } + +// An encrypted backup is held to the panel's password floor (audit +// 2026-10-12, M5); a refused one writes nothing. +func TestEncryptionPasswordFloor(t *testing.T) { + seedProject(t) + out := filepath.Join(t.TempDir(), "backup.spbk") + err := run(out, "eleven-char") + if err == nil || !strings.Contains(err.Error(), "at least 12 characters") { + t.Fatalf("an 11-character password = %v, want refused", err) + } + if _, statErr := os.Stat(out); !os.IsNotExist(statErr) { + t.Errorf("the refused backup wrote %s", out) + } +} + +// A wrong SELFPOST_DB_PATH makes no backup and no database (audit +// 2026-10-12, M3). +func TestBackupOfAMissingDatabaseIsRefused(t *testing.T) { + _, dataDir := seedProject(t) + missing := filepath.Join(dataDir, "typo.db") + t.Setenv("SELFPOST_DB_PATH", missing) + out := filepath.Join(t.TempDir(), "backup.tar.gz") + if err := run(out, ""); err == nil { + t.Fatal("a backup of a database that is not there was made") + } + for _, p := range []string{missing, out} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("the refused backup left %s (stat err = %v)", p, err) + } + } +} + +// -decrypt -o writes its file whole or not at all: a wrong password or an +// envelope cut short leaves the file that had the name before, and nothing +// else in its directory (audit 2026-10-12, L1 and stage 3). +func TestDecryptLeavesTheOutputAloneOnFailure(t *testing.T) { + seedProject(t) + dir := t.TempDir() + encrypted := filepath.Join(dir, "backup.spbk") + const password = "a long enough password" + if err := run(encrypted, password); err != nil { + t.Fatalf("create encrypted backup: %v", err) + } + sealed, err := os.ReadFile(encrypted) + if err != nil { + t.Fatal(err) + } + cut := filepath.Join(dir, "cut.spbk") + writeFile(t, cut, string(sealed[:len(sealed)-100])) + + outDir := t.TempDir() + out := filepath.Join(outDir, "backup.tar.gz") + const before = "the archive that was here" + for _, c := range []struct{ name, in, password, want string }{ + {"wrong password", encrypted, "the wrong password", "wrong password"}, + {"cut short", cut, password, "corrupted"}, + } { + t.Run(c.name, func(t *testing.T) { + writeFile(t, out, before) + err := runDecrypt(c.in, out, c.password) + if err == nil || !strings.Contains(err.Error(), c.want) { + t.Fatalf("runDecrypt = %v, want %q", err, c.want) + } + if got, _ := os.ReadFile(out); string(got) != before { + t.Errorf("the failed decryption changed %s to %d bytes", out, len(got)) + } + entries, _ := os.ReadDir(outDir) + if len(entries) != 1 { + t.Errorf("the failed decryption left files beside its output: %v", entries) + } + }) + } +} diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md index e180741..e249cae 100644 --- a/docs/audit-2026-10-12.md +++ b/docs/audit-2026-10-12.md @@ -1114,17 +1114,46 @@ those after it move up one patch number each. top-N). The statements are constants/builders the test explains (`TestSendLogQueriesUseTheirIndexes`, which replaces the hand-written plan check of `TestLimitRefusalsSince`). -- [ ] **CLI backup**: open the source `mode=ro`, refuse a missing file; password +- [x] **CLI backup**: open the source `mode=ro`, refuse a missing file; password floor of 12 shared with the panel (`validate.MinSecretFilePasswordLen`); `-decrypt -o` verifies the password (first chunk) before opening the output; `OnWarn` for skipped symlinks and `Check` fails when `certs/` is named but empty. — **Opus** -- [ ] **`secretfile` caps**: bound `128·N·r` to a stated budget (e.g. 128 MiB) + *Done 2026-10-15.* The source check is in `backup.snapshotDB`, so the + panel has it too; `mode=ro` measured not to create a missing file + (`TestSnapshotRefusesAMissingDatabase`). `-o` of both directions writes + a temporary file beside the name and renames it whole (a backup that + fails half way left a partial file too); `-decrypt` peeks the first + chunk - the password, and the envelope's type, authenticated - before + it opens the output. Since 2.5.2 links under `certs/` are followed, so + what is still left out and now warned of: a link under `data/`, a link + under `certs/` to a directory, a socket or the like; `certs/` with no + file refused by Check and Create alike. L4, in this stage's list without + a line: a bare address takes /128 when written with a colon, /32 + otherwise (`::ffff:10.0.0.1/32` was `::/32`). +- [x] **`secretfile` caps**: bound `128·N·r` to a stated budget (e.g. 128 MiB) and `p` to 1-2; refuse the rest with `ErrHeader`; reject trailing bytes after the last chunk. Tests. — **Opus** -- [ ] **`atomicfile`**: fsync the directory after rename; a test file for the + *Done 2026-10-15:* 128 MiB of scrypt's `V` (x/crypto v0.58.0 `Key`: + `make([]uint32, 32*N*r)`), `p` ≤ 2, `logN` ≤ 30 for the arithmetic; the + refusal is the existing `ErrCorrupt` (there is no `ErrHeader`), which the + import shows as "damaged or incomplete". One byte is read after the last + chunk; any is `ErrCorrupt`. `TestKDFBudget` (the old cap's 4 GiB among + the refused), `TestBytesAfterTheLastChunkRejected`. +- [x] **`atomicfile`**: fsync the directory after rename; a test file for the package. — **Opus** -- [ ] Docs: security.md § Backup; guide § Backup. — **Sonnet** + *Done 2026-10-15:* `atomicfile.SyncDir`, best effort as sasl.go's (the + file is in place either way; Go on Windows cannot sync a directory), used + by `Write`, `app.replaceSASLDB` (its inline copy replaced) and the CLI's + `-o`. `atomicfile_test.go`: create, replace, mode, nothing left beside; + an open reader keeps the old file; a failed write (missing directory, + a directory in the way) leaves the old state; SyncDir works on Linux. +- [x] Docs: security.md § Backup; guide § Backup. — **Sonnet** + *Done 2026-10-15:* security.md § Backup and domain export (what is warned + of, the missing database, the restore's record - integrity, not + authenticity -, the KDF budget, trailing bytes, the CLI's floor and + `-o`); guide § Restore (step 1), § Full backup (certs/, `-o`, the + database) and § Encrypting (the first block, a cut file, the floor). ### Stage 4 — panel behaviour (M8, M11, L10–L19) → `2.5.4` diff --git a/docs/guide.md b/docs/guide.md index f9839f0..cf958ea 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -724,7 +724,10 @@ into `/etc/letsencrypt`, the commands of "Reverse proxy" above give certbot's own files the group). A key readable by root alone is enough for Postfix, but then the panel refuses to make a backup and names the file: it does not hand over an archive without the key. Links under `certs/` are followed: the -archive holds the files they lead to. +archive holds the files they lead to. A `certs/` that is there with no file +in it is refused for the same reason; what a backup leaves out - a link to +a directory under `certs/`, a link under `data/` - it names in a warning (the +CLI's standard error, the panel's log). **2. `selfpost-backup` CLI — while the container is running (no downtime).** The panel button and this command call the same code path: a consistent SQLite @@ -737,9 +740,12 @@ docker exec selfpost-backup > selfpost-backup.tar.gz ``` Use `-o /path/inside/container` to write inside the container instead of -stdout. Optional encryption: set `SELFPOST_BACKUP_PASSWORD` or pass -`-password-file` — see [Encrypting a backup or -export](#encrypting-a-backup-or-export). +stdout; the file is written beside that name and takes it only when it is +whole, so a backup that fails leaves no half-written file and whatever had +the name before. A database that is not where `SELFPOST_DB_PATH` points is +refused, rather than archived empty. Optional encryption: set +`SELFPOST_BACKUP_PASSWORD` or pass `-password-file` — see [Encrypting a +backup or export](#encrypting-a-backup-or-export). **3. `tar` of the whole project directory — with the service stopped.** Stop the instance so nothing is writing to SQLite, then archive the operator @@ -832,7 +838,7 @@ back after a bad change): docker compose down # 2. Move the current project aside rather than deleting it -mv . ../selfpost.before-restore +cd .. && mv selfpost selfpost.before-restore mkdir selfpost && cd selfpost # 3. Unpack the backup into the fresh directory @@ -871,7 +877,15 @@ The last entry of every panel and CLI archive, `data/contents.json`, records each file under `data/` with its size and SHA-256, and the first start compares `/data` with it; a file that differs is named in the message. `docker-compose.yml`, `.env` and `certs/` are not compared — they are yours to -edit before that start. +edit before that start. A journal file beside the database +(`selfpost.db-wal`, `-shm`, `-journal`) is refused too: no archive holds one, +so it is left from what the directory held before, and SQLite would take it +up. To unpack again, unpack into an **empty** directory. + +The files are compared once: a first start that gets past the comparison +and then fails (its Resync, say) leaves the manifest, and the next +`docker compose up -d` runs the Resync without comparing again - that start +had the database open, so its files are no longer the archive's. **Moving to a different host** is the same flow: create an empty project directory, unpack the backup there, edit `.env` (and `docker-compose.yml` if @@ -957,9 +971,22 @@ unpacked into `/data`, which the CLI does with the same password: docker exec -i selfpost-backup -decrypt < backup.spbk > backup.tar.gz ``` +The password is checked on the file's first block, before the command +writes anything - but the shell's `>` has already emptied `backup.tar.gz` +before the command starts, so a wrong password leaves it empty; `-o` keeps +a file that was there. A file cut short fails at its end, with the command's +exit status non-zero, after what came before has been written: an archive +cut so is refused at the restore when it lost any file (see +[Restore](#restore) - the record is the archive's last entry). With `-o` the +output takes its name only when it is whole; a command killed outright +leaves a `.NAME.tmp-*` file beside it, readable by its owner alone, to +remove by hand. + The CLI also *writes* encrypted backups for scripted/cron use. The password -comes from `SELFPOST_BACKUP_PASSWORD` or `-password-file ` (first line), -never from a command-line argument, which would be visible in the process list. +must be at least 12 characters, as on the panel's forms (decrypting takes +any), and it comes from `SELFPOST_BACKUP_PASSWORD` or `-password-file ` +(first line), never from a command-line argument, which would be visible in +the process list. `-e` with the name only takes the value from your shell's environment, so it is not written on the command line: diff --git a/docs/roadmap.md b/docs/roadmap.md index 140deb9..49f2b51 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -42,7 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | -| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`, 2c `2.5.2`; next stage 3 → `2.5.3`) | 51/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | +| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`, 2c `2.5.2`; next stage 3 → `2.5.3`) | 55/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | **Recommended order** (not binding; owner, 2026-10-09: feature order follows development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and diff --git a/docs/schema-migrations.md b/docs/schema-migrations.md index 3af0aa1..c70b299 100644 --- a/docs/schema-migrations.md +++ b/docs/schema-migrations.md @@ -85,7 +85,7 @@ audit (`2.5.0`), `0009_refusal_line.sql` (`2.5.1`) and | 7 | `0007_delivery_log_days.sql` | 2.5.0 | DDL | `delivery_log_days`: what the reconcile sweep and the log tailer did, a row per UTC day, kept a year, shown nowhere - the measurement that decides the sweep's fate (audit 2026-10-12, stage 1). `send_log.reason` gains the values `reconcile` and `reconcile-contradicted` without a schema change | | 8 | `0008_tenant_boundary.sql` | 2.5.0 | DDL + data | `idx_applications_login_nocase`: an application login is unique without regard to case (audit 2026-10-12, H4). `send_log.domain_id` and `inbound_spam_log.inbound_domain_id` (`ON DELETE SET NULL`), each with an index, filled once by name from the registry as it was then: a row belongs to a domain by id, and a domain deleted and added again starts with an empty log (L12) | | 9 | `0009_refusal_line.sql` | 2.5.1 | DDL | `send_log.refusal_line`: Postfix's line about a refusal at MAIL FROM (a level-2 limit, a client address off the list), which has no queue id; the log tailer puts it on the row; `send_log.recovered`, a refusal row the tailer wrote from `mail.log` because the panel stopped before writing it; `sender_check_days.messages` (messages deferred mid-message, counted apart from the connections turned away) and `sender_check_days.estimated` (a day whose downtime is an upper bound) (audit 2026-10-12, stage 2b) | -| 10 | `0010_send_log_indexes.sql` | 2.5.3 | DDL | Four indexes of `send_log`, each for a question that walked the table or sorted a domain's log (audit 2026-10-12, M2, stage 3): `idx_send_log_open`, partial, the queued and deferred rows by time (the tailer's sweep every 5 min); `idx_send_log_domain_id` and `idx_send_log_app_login`, a page of one domain's or one application's log in id order; `idx_send_log_refusal_line`, partial, whether a refusal line is already on a row. `TestSendLogQueriesUseTheirIndexes` checks the plans of the statements the store runs | +| 10 | `0010_send_log_indexes.sql` | 2.5.3 | DDL | Four indexes of `send_log`, each for a question that walked the table or sorted a domain's log (audit 2026-10-12, M2, stage 3): `idx_send_log_open`, partial, the queued and deferred rows by time (the tailer's sweep every 5 min); `idx_send_log_domain_id` and `idx_send_log_app_login`, a page of one domain's or one application's log in id order; `idx_send_log_refusal_line`, partial, whether a refusal line is already on a row. `TestSendLogQueriesUseTheirIndexes` checks the plans of these and of the other by-time and by-scope statements the store runs | **Kind:** *DDL* — schema only; *data* — `INSERT`/`UPDATE` that must stay correct for operators upgrading from older 2.x images. diff --git a/docs/security.md b/docs/security.md index 1899bd3..b289830 100644 --- a/docs/security.md +++ b/docs/security.md @@ -308,6 +308,23 @@ operator can act on it. that the panel's user may read. `certs/` is the operator's, mounted read-only; only someone who can write the project directory places a link there, and only a global user can download the archive (`2.5.2`). + What a backup still leaves out - a link under `data/`, a link under + `certs/` to a directory - it names in a warning, and a `certs/` with no + file in it is refused rather than archived empty (`2.5.3`). +- A backup is made only of the database that is there: a wrong + `SELFPOST_DB_PATH` is refused, and the snapshot opens the source + read-only. An archive of an empty database would restore a server that + offers its first-run setup - a new global administrator - while it holds + live DKIM keys and `sasldb2` (`2.5.3`). +- The first start after a restore opens nothing until `/data` matches the + archive's last entry, `data/contents.json` (size and SHA-256 of each file + under `data/`): an archive unpacked in part - its manifest there, half its + database - is refused (`2.5.3`). The check is integrity, not authenticity: + whoever can write the project directory can write that record too. It + reads only regular files at the recorded names (a link or a pipe there is + refused, not followed), refuses a database journal file beside the + restored database, and is made once per restore: a start after one that + passed it and failed is not compared again while the manifest is the same. - Since 2.2.0 a full backup also carries, in the database, what Postfix logged about each message still in the send log (`send_log_lines`): the submitting client's address and SASL login, the message id, the receiving server and @@ -318,7 +335,12 @@ operator can act on it. scrypt (N=2¹⁵, r=8, p=1) → AES-256-GCM, streamed in 64 KiB chunks, each authenticated with the header, the chunk number, and an end-of-stream flag — a truncated or substituted file fails to open instead of silently restoring a - partial "tail". Format and wrapper: + partial "tail", and bytes after the last chunk are refused (`2.5.3`). The + key-derivation parameters travel in the header, so a reader is bounded in + what a header may ask before the password is checked: scrypt's array at + most 128 MiB (four times what SelfPost writes), r ≤ 64 so the rest stays + under 32 KiB, p ≤ 2 - *Import a domain* reads an uploaded file, and the + old cap let a header ask for 4 GiB (`2.5.3`). Format and wrapper: [internal/secretfile](../internal/secretfile/secretfile.go). - Extensions: `.spbk` (**S**elf**P**ost **b**ac**k**up — full backup), `.spde` (**S**elf**P**ost **d**omain **e**xport — domain export); unencrypted files @@ -329,7 +351,13 @@ operator can act on it. `-password-file`, never as an argument (the process list is readable by any process in the container). - Minimum password length matches the administrator password (12): the file - sits offline and can be attacked without a time limit. + sits offline and can be attacked without a time limit. The panel's forms and, + since `2.5.3`, the CLI hold an encrypting password to it; decrypting takes + any. +- The CLI's `-o FILE` is written beside its name and takes it only when + whole, so a failure - a wrong password to `-decrypt` among them, which is + checked on the first chunk before the output is touched - leaves the file + that was there (`2.5.3`). --- diff --git a/internal/app/sasl.go b/internal/app/sasl.go index b270197..fa65a86 100644 --- a/internal/app/sasl.go +++ b/internal/app/sasl.go @@ -12,6 +12,8 @@ import ( "path/filepath" "strings" "sync" + + "github.com/mixeme/selfpost/internal/atomicfile" ) // SASLDB manages the Cyrus SASL account database (sasldb2) the panel maintains @@ -118,10 +120,7 @@ func replaceSASLDB(path string, edit func(path string) error) error { // loss may bring the old database back, out of step with the panel's // records (review of stage 2c, F10). A directory that cannot be synced // - not on every system - leaves the change as durable as before. - if dir, err := os.Open(filepath.Dir(path)); err == nil { - _ = dir.Sync() - dir.Close() - } + _ = atomicfile.SyncDir(filepath.Dir(path)) return nil } diff --git a/internal/atomicfile/atomicfile.go b/internal/atomicfile/atomicfile.go index 5e8520f..3e1635d 100644 --- a/internal/atomicfile/atomicfile.go +++ b/internal/atomicfile/atomicfile.go @@ -1,3 +1,5 @@ +// Package atomicfile writes a file so that readers see the old file or the +// new one, whole, and a power loss after Write returns keeps the new one. package atomicfile import ( @@ -7,7 +9,13 @@ import ( ) // Write writes data to path via a temp file in the same directory followed by -// a rename, so readers only ever see the complete old or new file. +// a rename, so readers only ever see the complete old or new file. The file +// is synced before the rename and the directory after it: the rename is a +// change of the directory, and without its own sync a power loss may bring +// the old file back (fsync(2): syncing a file "does not necessarily ensure +// that the entry in the directory containing the file has also reached +// disk"; audit 2026-10-12, L7). Every map the panel writes for Postfix and +// OpenDKIM goes through here. func Write(path string, data []byte, perm os.FileMode) error { dir := filepath.Dir(path) tmp, err := os.CreateTemp(dir, ".tmp-*") @@ -41,5 +49,19 @@ func Write(path string, data []byte, perm os.FileMode) error { return fmt.Errorf("rename %s -> %s: %w", tmpName, path, err) } cleanup = false + _ = SyncDir(dir) return nil } + +// SyncDir makes a rename in dir durable. It is best effort for its callers: +// the new file is in place and readable either way, and not every system +// syncs a directory - Go on Windows cannot open one for it - so a failure +// leaves the change as durable as it was before this existed. +func SyncDir(dir string) error { + d, err := os.Open(dir) + if err != nil { + return err + } + defer d.Close() + return d.Sync() +} diff --git a/internal/atomicfile/atomicfile_test.go b/internal/atomicfile/atomicfile_test.go new file mode 100644 index 0000000..dda750d --- /dev/null +++ b/internal/atomicfile/atomicfile_test.go @@ -0,0 +1,113 @@ +package atomicfile + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// leftovers lists what is in dir besides the names given. +func leftovers(t *testing.T, dir string, names ...string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + keep := map[string]bool{} + for _, n := range names { + keep[n] = true + } + var out []string + for _, e := range entries { + if !keep[e.Name()] { + out = append(out, e.Name()) + } + } + return out +} + +// A new file and a replaced one hold what was written, with the mode asked +// for, and nothing is left beside them. +func TestWriteCreatesAndReplaces(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "map") + for _, body := range []string{"first\n", "second, longer than the first\n", ""} { + if err := Write(path, []byte(body), 0o640); err != nil { + t.Fatalf("Write %q: %v", body, err) + } + got, err := os.ReadFile(path) + if err != nil || string(got) != body { + t.Fatalf("file = %q, %v; want %q", got, err, body) + } + if runtime.GOOS != "windows" { + if info, _ := os.Stat(path); info.Mode().Perm() != 0o640 { + t.Errorf("mode = %v, want 0640", info.Mode().Perm()) + } + } + if rest := leftovers(t, dir, "map"); len(rest) != 0 { + t.Errorf("Write left %v", rest) + } + } +} + +// A reader that opened the old file reads the old file to its end: the new +// one takes the name, it does not overwrite the old one's bytes. +func TestAReaderKeepsTheOldFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows does not rename over an open file; the container is Linux") + } + path := filepath.Join(t.TempDir(), "map") + if err := Write(path, []byte("old contents\n"), 0o640); err != nil { + t.Fatal(err) + } + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if err := Write(path, []byte("new\n"), 0o640); err != nil { + t.Fatal(err) + } + buf := make([]byte, 64) + n, _ := f.Read(buf) + if string(buf[:n]) != "old contents\n" { + t.Errorf("the open reader read %q", buf[:n]) + } +} + +// A write that fails leaves the old file and no temporary one. +func TestAFailedWriteLeavesTheOldFile(t *testing.T) { + dir := t.TempDir() + if err := Write(filepath.Join(dir, "missing", "map"), []byte("x"), 0o640); err == nil { + t.Error("Write into a directory that is not there succeeded") + } + + // The name is a directory: the rename fails after the temporary file + // was written. + target := filepath.Join(dir, "taken") + if err := os.MkdirAll(filepath.Join(target, "inside"), 0o750); err != nil { + t.Fatal(err) + } + if err := Write(target, []byte("x"), 0o640); err == nil { + t.Error("Write over a directory succeeded") + } + if rest := leftovers(t, dir, "taken"); len(rest) != 0 { + t.Errorf("the failed Write left %v", rest) + } + if _, err := os.Stat(filepath.Join(target, "inside")); err != nil { + t.Errorf("the failed Write changed the directory in its way: %v", err) + } +} + +// The directory is synced where the system allows it; on Linux, where the +// container runs, it does. +func TestSyncDir(t *testing.T) { + err := SyncDir(t.TempDir()) + if runtime.GOOS == "linux" && err != nil { + t.Errorf("SyncDir on Linux = %v", err) + } + if err := SyncDir(filepath.Join(t.TempDir(), "missing")); err == nil { + t.Error("SyncDir of a directory that is not there = nil") + } +} diff --git a/internal/backup/backup.go b/internal/backup/backup.go index d5055bf..fb72472 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -166,7 +166,7 @@ func Create(w io.Writer, p Params) error { return err } - if err := addTree(a, p.DataDir, DataArchivePrefix, excludedFromArchive, skipQueue, false); err != nil { + if _, err := addTree(a, p.DataDir, DataArchivePrefix, excludedFromArchive, skipQueue, false, p.OnWarn); err != nil { return err } @@ -239,7 +239,7 @@ func Check(p Params) error { if err := ValidateDeployRoot(p.DeployRoot); err != nil { return err } - if err := addTree(nil, p.DataDir, DataArchivePrefix, excludedFromArchive, skipQueue, false); err != nil { + if _, err := addTree(nil, p.DataDir, DataArchivePrefix, excludedFromArchive, skipQueue, false, nil); err != nil { return err } return addDeployFiles(nil, p.DeployRoot, nil) @@ -297,20 +297,43 @@ func addDeployFiles(a *archive, deployRoot string, onWarn func(string)) error { // are links again, relative, into archive/ (eff-certbot docs, "Where are // my certificates?"; assumptions C-Q4, C-A1). The archive carries the // files they lead to, under the links' names. - return addTree(a, certsDir, CertsDirName+"/", nil, nil, true) + files, err := addTree(a, certsDir, CertsDirName+"/", nil, nil, true, onWarn) + if err != nil { + return err + } + // certs/ is what Postfix serves its certificate from (the compose file + // mounts it at /etc/postfix/tls). There and empty - or holding only what + // the archive cannot carry - it would give an archive that restores a + // server without its certificate, so there is none (audit 2026-10-12, + // L2). + if files == 0 { + return ErrNoCerts + } + return nil } +// ErrNoCerts is a certs/ that is there with no file a backup can carry. +var ErrNoCerts = errors.New("backup: " + CertsDirName + "/ holds no file this backup can carry, so the server it restores would have no certificate - put the PEM files (or links to them) there") + // addTree walks root and adds every regular file (and directory, to preserve -// empty ones and modes) to a under archivePrefix + path relative to root. +// empty ones and modes) to a under archivePrefix + path relative to root, and +// returns how many files it took. // When exclude is non-nil, top-level names relative to root are skipped; skip, // when non-nil, leaves out whatever it says. With a nil a nothing is written: // every file is opened and closed, which is how Check finds what cannot be read. // Non-regular, non-directory entries (symlinks, sockets) are skipped - except, // with follow, a symbolic link to a regular file, which is archived as that // file under the link's name; a link that leads nowhere this process can -// reach is an error, as an unreadable file is. -func addTree(a *archive, root, archivePrefix string, exclude map[string]bool, skip skipFunc, follow bool) error { - return filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { +// reach is an error, as an unreadable file is. Each entry left out that way +// is told to warn, when it is not nil: a backup does not leave something out +// without saying so (audit 2026-10-12, L2). +func addTree(a *archive, root, archivePrefix string, exclude map[string]bool, skip skipFunc, follow bool, warn func(string)) (files int, err error) { + skipped := func(name, what string) { + if warn != nil { + warn(name + " is " + what + ", not a file: the backup does not carry it") + } + } + err = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } @@ -362,6 +385,9 @@ func addTree(a *archive, root, archivePrefix string, exclude map[string]bool, sk // the listing and the open: not part of /data's state. return nil } + if err == nil { + files++ + } return err case follow && info.Mode()&fs.ModeSymlink != 0: target, err := os.Stat(path) @@ -369,13 +395,37 @@ func addTree(a *archive, root, archivePrefix string, exclude map[string]bool, sk return fmt.Errorf("backup: %s is a link to a file this container cannot reach - with certbot, /etc/letsencrypt is mounted at the same path (guide § Reverse proxy): %w", name, err) } if !target.Mode().IsRegular() { + skipped(name, "a link to a directory") return nil } - return writeTarFile(a, name, target.Mode().Perm(), path) + if err := writeTarFile(a, name, target.Mode().Perm(), path); err != nil { + return err + } + files++ + return nil + case info.Mode()&fs.ModeSymlink != 0: + skipped(name, "a link") + return nil default: + skipped(name, kindOf(info.Mode())) return nil } }) + return files, err +} + +// kindOf names what a non-regular, non-directory entry is, for a warning. +func kindOf(m fs.FileMode) string { + switch { + case m&fs.ModeSocket != 0: + return "a socket" + case m&fs.ModeNamedPipe != 0: + return "a named pipe" + case m&fs.ModeDevice != 0: + return "a device" + default: + return "neither a file nor a directory" + } } // writeTarBytes writes an in-memory file entry. @@ -430,6 +480,21 @@ func writeTarFile(a *archive, name string, mode fs.FileMode, srcPath string) err // applyQueryParams(): each _pragma is run as "pragma " on connect, // busy_timeout first.) func snapshotDB(dbPath string) (path string, cleanup func(), err error) { + // The database must be there: SQLite creates a file that is not, and an + // archive of that empty database restores a panel that offers its + // first-run setup on a server holding live keys (audit 2026-10-12, M3). + // mode=ro makes the open fail rather than create one, should the file go + // between the two: SQLite 3.53.4 creates a database only under "rwc", the + // default (https://sqlite.org/uri.html § 3.3), and VACUUM INTO is not a + // write to the source (lang_vacuum.html § 2.1, § 3) - both measured by + // TestSnapshotRefusesAMissingDatabase. + info, err := os.Stat(dbPath) + if err != nil { + return "", nil, fmt.Errorf("backup: the database: %w", err) + } + if !info.Mode().IsRegular() { + return "", nil, fmt.Errorf("backup: the database %s is not a file", dbPath) + } dir, err := os.MkdirTemp("", "selfpost-backup-") if err != nil { return "", nil, fmt.Errorf("backup: temp dir: %w", err) @@ -439,7 +504,7 @@ func snapshotDB(dbPath string) (path string, cleanup func(), err error) { // A short busy timeout lets VACUUM INTO wait out a brief writer rather than // failing immediately if the panel happens to be mid-write. - db, err := sql.Open("sqlite", "file:"+dbPath+"?_pragma=busy_timeout(5000)") + db, err := sql.Open("sqlite", "file:"+dbPath+"?mode=ro&_pragma=busy_timeout(5000)") if err != nil { cleanup() return "", nil, fmt.Errorf("backup: open database: %w", err) @@ -497,16 +562,18 @@ func CheckRestore(manifestPath, binaryVersion string) (restored bool, err error) // ConsumeRestore ends a restore once its first start has done what a restore // start does: the manifest is removed, so later starts (and in-place -// upgrades) are not gated by it, and then the record of the checked files. A -// start stopped between the two leaves the record alone, which nothing reads -// without a manifest and no backup carries. +// upgrades) are not gated by it, and then the record of the files, checked or +// unpacked again since. A start stopped between the two leaves the record +// alone, which nothing reads without a manifest and no backup carries. func ConsumeRestore(manifestPath string) error { if err := os.Remove(manifestPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("backup: consume restore manifest: %w", err) } - checked := filepath.Join(filepath.Dir(manifestPath), contentsCheckedName) - if err := os.Remove(checked); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("backup: consume restore manifest: %w", err) + for _, name := range []string{contentsCheckedName, ContentsName} { + err := os.Remove(filepath.Join(filepath.Dir(manifestPath), name)) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("backup: consume restore manifest: %w", err) + } } return nil } diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 63c7d3f..0431c04 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -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) + } + } +} diff --git a/internal/backup/contents.go b/internal/backup/contents.go index de435f3..9b862cc 100644 --- a/internal/backup/contents.go +++ b/internal/backup/contents.go @@ -12,6 +12,8 @@ import ( "path" "path/filepath" "strings" + + "github.com/mixeme/selfpost/internal/atomicfile" ) // FormatContents identifies the record of what an archive's data directory @@ -93,34 +95,49 @@ func errUnpackedPart(detail string) error { } // checkContents compares the data directory with the archive's record, once -// per restore (see contentsCheckedName). +// per restore (see contentsCheckedName). A record unpacked anew wins over the +// mark of an earlier check: the archive was unpacked again, and that is what +// the start runs on (review of audit stage 3). func checkContents(dataDir string) error { - checked := filepath.Join(dataDir, contentsCheckedName) - if c, err := readContents(checked); err == nil { - // A start before this one checked the files. It is the same restore - // when the manifest is still the one that record holds; a different - // archive unpacked over the directory since is checked anew. - if verifyFiles(dataDir, c, DataArchivePrefix+ManifestName) == nil { - return nil - } - } else if !errors.Is(err, os.ErrNotExist) { - return err - } - path := filepath.Join(dataDir, ContentsName) + checked := filepath.Join(dataDir, contentsCheckedName) c, err := readContents(path) if errors.Is(err, os.ErrNotExist) { + prev, perr := readContents(checked) + if perr == nil && verifyFiles(dataDir, prev, DataArchivePrefix+ManifestName) == nil { + // A start before this one checked the files, and the manifest is + // still the one that record holds: the same restore, whose + // files that start may have changed since. + return nil + } + if perr != nil && !errors.Is(perr, os.ErrNotExist) { + return perr + } return errUnpackedPart(DataArchivePrefix + ContentsName + ", its last entry, is not there") } if err != nil { return err } + // No backup carries the database's journal files. One beside the + // restored database is left from the database the directory held + // before, and opening this one would take it up: the first connection + // after a crash recovers from the WAL, and a checkpoint "takes content + // from the WAL file and transfers it back into the original database + // file" (https://sqlite.org/wal.html § 9, § 2.2). + for _, side := range []string{"-wal", "-shm", "-journal"} { + if _, err := os.Lstat(filepath.Join(dataDir, "selfpost.db"+side)); err == nil { + return errUnpackedPart("selfpost.db" + side + " is beside the database, and no archive holds one - it is left from what was in the directory before") + } + } if err := verifyFiles(dataDir, c, ""); err != nil { return err } if err := os.Rename(path, checked); err != nil { return fmt.Errorf("backup: mark the restored files checked: %w", err) } + // The start goes on to change the files; the mark must not be lost to a + // power cut after that, or the next start compares changed files. + _ = atomicfile.SyncDir(dataDir) return nil } @@ -151,15 +168,27 @@ func verifyFiles(dataDir string, c Contents, only string) error { if !ok || rel == "" || path.Clean(rel) != rel || rel == ".." || strings.HasPrefix(rel, "../") || path.IsAbs(rel) { return errUnpackedPart("the record names " + f.Name + ", which is not a file of the data directory") } - file, err := os.Open(filepath.Join(dataDir, filepath.FromSlash(rel))) + // The archive holds regular files only (archive.writeFile); a link + // or a pipe under the name is not followed or read - a crafted + // archive must not make the start read a file outside /data, or + // wait on a pipe (security review of audit stage 3). + full := filepath.Join(dataDir, filepath.FromSlash(rel)) + info, err := os.Lstat(full) if errors.Is(err, os.ErrNotExist) { return errUnpackedPart(f.Name + " is missing") } if err != nil { return fmt.Errorf("backup: check %s: %w", f.Name, err) } + if !info.Mode().IsRegular() || info.Size() != f.Size { + return errUnpackedPart(f.Name + " is not the file the archive holds") + } + file, err := os.Open(full) + if err != nil { + return fmt.Errorf("backup: check %s: %w", f.Name, err) + } h := sha256.New() - n, err := io.Copy(h, file) + n, err := io.Copy(h, io.LimitReader(file, f.Size+1)) file.Close() if err != nil { return fmt.Errorf("backup: check %s: %w", f.Name, err) diff --git a/internal/secretfile/secretfile.go b/internal/secretfile/secretfile.go index e8c46c6..a0dc56a 100644 --- a/internal/secretfile/secretfile.go +++ b/internal/secretfile/secretfile.go @@ -105,12 +105,19 @@ const ( defaultR = 8 defaultP = 1 - // maxLogN caps what a file may ask for, so a hostile header cannot make the - // reader allocate its way to an out-of-memory kill before it ever fails - // authentication. - maxLogN = 20 - maxR = 32 - maxP = 16 + // What a file may ask of the reader, so a hostile header cannot make it + // allocate its way to an out-of-memory kill, or spin, before it ever fails + // authentication (audit 2026-10-12, M4). golang.org/x/crypto v0.58.0, + // scrypt.Key, allocates V, 128·r·N bytes (make([]uint32, 32*N*r)), B, + // p·128·r bytes of PBKDF2 output, and XY, 256·r; its work is N·r·p. V is + // held to four times what the writer asks (128·2^15·8 = 32 MiB), r to 64 + // - the writer's is 8 - so B and XY stay under 32 KiB together (a header + // with N=2 and a huge r asked for 384 MiB in B and XY: security review of + // stage 3), p to 2 - the writer's is 1. The domain import reads a file + // any global user uploads, the CLI a file of any origin. + maxScryptMemory = 128 << 20 + maxR = 64 + maxP = 2 ) var ( @@ -180,8 +187,11 @@ func parseHeader(buf []byte) (*header, error) { h.salt = buf[i : i+saltLen] h.noncePrefix = buf[i+saltLen : i+saltLen+noncePrefixLen] - // Reject absurd work factors before spending any memory on them. - if h.logN < 1 || h.logN > maxLogN || h.r < 1 || h.r > maxR || h.p < 1 || h.p > maxP { + // Reject absurd work factors before spending any memory on them. logN + // under 31 and r under 65 keep the shift and the product in range; then + // the product is held to the budget. + if h.logN < 1 || h.logN > 30 || h.r < 1 || h.r > maxR || h.p < 1 || h.p > maxP || + 128*(uint64(1)< maxScryptMemory { return nil, fmt.Errorf("%w: unreasonable key-derivation parameters", ErrCorrupt) } return h, nil @@ -450,6 +460,15 @@ func (d *Reader) next() error { if err != nil { return ErrWrongPassword } + // The last chunk ends the file: bytes after it - another envelope + // appended, say - are damage, not something to ignore (audit + // 2026-10-12, L3). + var more [1]byte + if n, err := io.ReadFull(d.r, more[:]); n > 0 { + return fmt.Errorf("%w: bytes after the last chunk", ErrCorrupt) + } else if err != nil && !errors.Is(err, io.EOF) { + return fmt.Errorf("secretfile: read chunk: %w", err) + } d.done = true } d.plain = plain diff --git a/internal/secretfile/secretfile_test.go b/internal/secretfile/secretfile_test.go index 19720b6..8664436 100644 --- a/internal/secretfile/secretfile_test.go +++ b/internal/secretfile/secretfile_test.go @@ -2,6 +2,7 @@ package secretfile import ( "bytes" + "encoding/binary" "errors" "io" "testing" @@ -155,8 +156,71 @@ func TestEmptyPasswordRejected(t *testing.T) { func TestUnreasonableKDFParamsRejected(t *testing.T) { enc := encrypt(t, TypeFullBackup, "password password", []byte("x")) bad := bytes.Clone(enc) - bad[MagicLen+2] = 40 // logN far beyond maxLogN + bad[MagicLen+2] = 40 // logN far beyond the budget if _, err := NewReader(bytes.NewReader(bad), "password password"); !errors.Is(err, ErrCorrupt) { t.Fatalf("err = %v, want ErrCorrupt", err) } } + +// A header may ask for no more than the stated budget: 128·N·r bytes of +// scrypt's array at most 128 MiB, r at most 64, p at most 2 (audit +// 2026-10-12, M4; security review of stage 3). The writer's own parameters +// are a quarter of it. +func TestKDFBudget(t *testing.T) { + enc := encrypt(t, TypeFullBackup, "password password", []byte("x")) + header := func(logN uint8, r, p uint32) []byte { + h := bytes.Clone(enc[:headerLen]) + h[MagicLen+2] = logN + binary.BigEndian.PutUint32(h[MagicLen+3:], r) + binary.BigEndian.PutUint32(h[MagicLen+7:], p) + return h + } + for _, c := range []struct { + logN uint8 + r, p uint32 + ok bool + }{ + {defaultLogN, defaultR, defaultP, true}, + {17, 8, 2, true}, // 128 MiB, the budget + {20, 1, 1, true}, // 128 MiB another way + {18, 8, 1, false}, // 256 MiB + {15, 64, 1, false}, // 256 MiB + {20, 32, 16, false}, // what the old cap let through: 4 GiB + {15, 8, 3, false}, // p over 2 + {0, 8, 1, false}, // N of 1 + {31, 1, 1, false}, // past the shift + {1, 1 << 21, 1, false}, // r alone over the budget + {1, 1 << 19, 2, false}, // V small, B and XY 384 MiB + {11, 64, 2, true}, // r at its cap + {11, 65, 1, false}, // r past it + {30, 1 << 20, 1, false}, // the largest product, still refused + {15, 0, 1, false}, + {15, 8, 0, false}, + } { + _, err := parseHeader(header(c.logN, c.r, c.p)) + if c.ok && err != nil { + t.Errorf("logN=%d r=%d p=%d refused: %v", c.logN, c.r, c.p, err) + } + if !c.ok && !errors.Is(err, ErrCorrupt) { + t.Errorf("logN=%d r=%d p=%d = %v, want ErrCorrupt", c.logN, c.r, c.p, err) + } + } +} + +// Nothing may follow the last chunk: two envelopes end to end, or stray +// bytes, are damage rather than the first file (audit 2026-10-12, L3). +func TestBytesAfterTheLastChunkRejected(t *testing.T) { + first := encrypt(t, TypeFullBackup, "password password", []byte("first")) + second := encrypt(t, TypeFullBackup, "password password", []byte("second")) + for name, data := range map[string][]byte{ + "two envelopes": append(bytes.Clone(first), second...), + "one byte": append(bytes.Clone(first), 0), + } { + if _, _, err := decrypt(t, data, "password password"); !errors.Is(err, ErrCorrupt) { + t.Errorf("%s: err = %v, want ErrCorrupt", name, err) + } + } + if plain, _, err := decrypt(t, first, "password password"); err != nil || string(plain) != "first" { + t.Errorf("the envelope alone = %q, %v", plain, err) + } +} diff --git a/internal/store/queryplan_test.go b/internal/store/queryplan_test.go index 974de68..cfc6a44 100644 --- a/internal/store/queryplan_test.go +++ b/internal/store/queryplan_test.go @@ -25,12 +25,15 @@ func explain(t *testing.T, s *Store, query string, args ...any) string { return strings.Join(plan, "\n") } -// The send log is the one table that grows with the mail, and every question -// asked of it on a path that runs per message, per log line or every few -// minutes is answered from an index: measured with EXPLAIN QUERY PLAN against -// the statements the store runs, not copies of them (audit 2026-10-12, M2; -// stage 3). SQLite chooses without statistics here, as it does on a server - -// nothing runs ANALYZE. +// The send log is the one table that grows with the mail. The questions +// asked of it by time or by scope - per message (the level-2 count), per log +// line (a refusal line), every few minutes (the sweep, the notifier), per +// page and per statistics view - are answered from an index: measured with +// EXPLAIN QUERY PLAN against the statements the store runs, not copies of +// them (audit 2026-10-12, M2; stage 3). Those by queue id (UpdateStatus, +// ExpireMessage, RecoverRefusal) go by idx_send_log_queue_id and are not +// pinned here. SQLite chooses without statistics here, as it does on a +// server - nothing runs ANALYZE. func TestSendLogQueriesUseTheirIndexes(t *testing.T) { s := openTestStore(t) const since = "2026-10-01T00:00:00Z" diff --git a/internal/store/sendlog.go b/internal/store/sendlog.go index 4eff301..d76e903 100644 --- a/internal/store/sendlog.go +++ b/internal/store/sendlog.go @@ -328,6 +328,12 @@ const attachRefusalLineQuery = `UPDATE send_log SET refusal_line = ?1 WHERE id = // § 2); NOCASE folds ASCII only (datatype3.html § 7.1); ?1 is bound once and // used twice (lang_expr.html § 4). func (s *Store) AttachRefusalLine(clientIP, login, from string, at time.Time, line string) (bool, error) { + // An empty line is what "no line" is on a row; the statement's second + // term would let it through, the old statement did not (review of stage + // 3). No caller passes one. + if line == "" { + return false, nil + } res, err := s.db.Exec(attachRefusalLineQuery, line, ReasonLimit, ReasonClient, from, clientIP, at.Add(-refusalLineWindow).UTC().Format(time.RFC3339), diff --git a/internal/web/handlers/handlers_backup.go b/internal/web/handlers/handlers_backup.go index 82c7dcb..7aa9554 100644 --- a/internal/web/handlers/handlers_backup.go +++ b/internal/web/handlers/handlers_backup.go @@ -345,6 +345,10 @@ func unreadableBackupErr(err error, deployRoot string) string { return "The panel is not allowed to read " + name + ", and a backup without it would not bring the server back. " + "On the host, give the file to group 998 and make it group-readable (chgrp 998, chmod 640), then try again. Nothing was downloaded." } + if errors.Is(err, backup.ErrNoCerts) { + return "The project's certs/ holds no certificate, so a backup would bring the server back without one. " + + "On the host, put fullchain.pem and privkey.pem - or links to certbot's - in certs/, then try again. Nothing was downloaded." + } return "Could not read everything a backup holds. Nothing was downloaded; the reason is in the panel's log." } diff --git a/internal/web/handlers/handlers_backup_test.go b/internal/web/handlers/handlers_backup_test.go index 5154895..b9dacf2 100644 --- a/internal/web/handlers/handlers_backup_test.go +++ b/internal/web/handlers/handlers_backup_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/mixeme/selfpost/internal/backup" "github.com/mixeme/selfpost/internal/secretfile" "github.com/mixeme/selfpost/internal/web/validate" ) @@ -123,3 +124,12 @@ func TestBackupPageOffersEncryption(t *testing.T) { } } } + +// An empty certs/ is not "could not read": the page says what is missing and +// where it goes (review of audit stage 3). +func TestEmptyCertsMessage(t *testing.T) { + msg := unreadableBackupErr(fmt.Errorf("check: %w", backup.ErrNoCerts), "/selfpost-deploy") + if !strings.Contains(msg, "certs/ holds no certificate") || !strings.Contains(msg, "fullchain.pem") { + t.Errorf("message = %q", msg) + } +} diff --git a/test/e2e/restore_test.go b/test/e2e/restore_test.go index 60f2067..49e2280 100644 --- a/test/e2e/restore_test.go +++ b/test/e2e/restore_test.go @@ -41,6 +41,16 @@ func testFullBackupRestore(t *testing.T, sc *scenario) { t.Fatal(err) } + // A map under data/postfix by a name the entrypoint does not list - the + // DMARC maps were such, with DMARC on (review of audit stage 3, H1). The + // archive records no owner, and the unpack below runs as root, as an + // operator's tar does: the entrypoint has to hand the file back to the + // panel, which reads every recorded file on the first start. + if out, err := h.execIn("selfpost", "sh", "-c", + "echo 'x OK' > /data/postfix/e2e_extra_map && chown panel:selfpost /data/postfix/e2e_extra_map && chmod 0640 /data/postfix/e2e_extra_map"); err != nil { + t.Fatalf("write an extra map: %v\n%s", err, out) + } + resp, archive, err := sc.panel.postForm("/server/backup", url.Values{}) if err != nil { t.Fatalf("full backup: %v", err) @@ -104,6 +114,10 @@ func testFullBackupRestore(t *testing.T, sc *scenario) { t.Fatalf("%v\n%s", err, logTail(h.logs("selfpost"), "backup")) } + if out, _ := h.execIn("selfpost", "stat", "-c", "%U:%G %a", "/data/postfix/e2e_extra_map"); strings.TrimSpace(out) != "panel:selfpost 640" { + t.Errorf("the extra map after the restore is %q, want panel:selfpost 640", strings.TrimSpace(out)) + } + p, err := newPanelClient() if err != nil { t.Fatal(err)