From dd0fefdcb788c2a8fc17f0a259427a6d380ce27a Mon Sep 17 00:00:00 2001 From: Mix Date: Sun, 11 Oct 2026 20:43:10 +0300 Subject: [PATCH] backup: the panel's archive is the command's, private key included Owner, 2026-10-11: the private key is needed, and a backup from the panel and one from the console have to be the same. The commit that made the panel's backup work skipped a file under certs/ the panel could not open - on the test server the private key, which the certificate hook leaves to root and Postfix's group. That gave an archive with a certificate and no key, and two kinds of archive. - Params.Unprivileged is gone. Nothing is left out on account of who makes the archive; the queue is out for everyone (previous commit). - When the panel cannot read a file under certs/, backup.Check says so before the download and the panel refuses with what to do on the host: chgrp 998 certs/privkey.pem && chmod 640. backup.UnreadableCert tells that case from any other failure. - 998 is the container's selfpost group, which holds the panel and Postfix. Its id was whatever groupadd gave; the Dockerfile now fixes it at the value it has always had, because an operator addresses it by number from the host. - Guide: method 1 is the same archive as method 2, with the permission the key needs and a note for whatever renews the certificate. Help and the box follow the design commit before this one. - e2e: the restore scenario first makes the key unreadable and expects the refusal naming the file and the way out, then takes the backup and expects the key in it; nothing is put back by hand before the restart. go vet ./internal/backup/ ./internal/web/handlers/: ok go test ./internal/backup/ ./cmd/... ./internal/web/...: ok make e2e on the test server: ok Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 12 ++-- build/Dockerfile | 5 +- docs/guide.md | 24 +++++--- internal/backup/backup.go | 72 +++++++++--------------- internal/backup/backup_test.go | 33 +++++++++-- internal/web/handlers/handlers_backup.go | 29 ++++++---- internal/web/view/help.go | 3 +- internal/web/view/templates/backup.html | 2 +- test/e2e/restore_test.go | 41 ++++++++------ test/e2e/stage.go | 10 +++- 10 files changed, 138 insertions(+), 93 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7222e43..d3f28fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,11 +33,13 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version and the panel is not that user ("open /data/postfix/queue/active: permission denied" in the log). `selfpost-backup` run with `docker exec` was not affected - it runs as root. The queue is no longer part of a backup - (see Changed), and the panel's archive leaves out a file in `certs/` it may - not open (usually the private key, named in the log); the page, Help and - the guide say so, and the guide says to put the key back before starting a - restored instance. Everything is read through before the download starts, - so a failure is a message on the page and not a file cut short. + (see Changed), so the two archives are the same again. Everything is read + through before the download starts, so a failure is a message on the page + and not a file cut short. One thing the panel may still be unable to read + is the private key in `certs/`: it then refuses and says what to do - + `chgrp 998 certs/privkey.pem && chmod 640 certs/privkey.pem` on the host, + 998 being the container's `selfpost` group, whose id is now fixed in the + image. ### Security diff --git a/build/Dockerfile b/build/Dockerfile index 25ba6f1..b0bd4c5 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -62,7 +62,10 @@ RUN useradd --system --no-create-home --shell /usr/sbin/nologin panel # entrypoint.sh) — lets OpenDKIM read the panel-owned keys and lets Postfix read # the sasldb2/sender map, and lets the panel reach the supervisor control socket # to signal OpenDKIM/Postfix reloads without any process running as root. -RUN groupadd --system selfpost \ +# The id is fixed, and it is the one the group has always had: an operator +# gives the TLS private key on the host to this group by number, so that the +# panel can put it into a full backup (guide § Taking a full backup). +RUN groupadd --system --gid 998 selfpost \ && usermod -aG selfpost panel \ && usermod -aG selfpost opendkim \ && usermod -aG selfpost postfix diff --git a/docs/guide.md b/docs/guide.md index b0cb6ae..218ffe4 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -560,17 +560,25 @@ instance keeps running, what goes into the archive, and what happens on restore. **1. Panel — while the container is running (no downtime).** *Server › Backup* → *Full backup*. *Encrypt with a password* is ticked; untick it for a plain -archive. The archive of the CLI below, less one thing the panel may not read: -it runs as its own user, not as root, and **a file in `certs/` it cannot open** -is left out — usually the private key (the panel's log names it). Put the key -back into `certs/` before starting a restored instance. If anything else -cannot be read, the panel says so and downloads nothing. +archive. Same archive as the CLI below. The panel runs as its own user, not as +root, so **the private key in `certs/` has to be readable by it**: give the +file to the container's `selfpost` group, which has the fixed id 998 and holds +both the panel and Postfix — + +```sh +chgrp 998 certs/privkey.pem && chmod 640 certs/privkey.pem +``` + +— and have whatever renews the certificate do the same (a certbot deploy hook, +for one). 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. **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 snapshot via `VACUUM INTO`, then a gzip tar of `data/` (minus diagnostics) plus -deploy files. `docker exec` runs the command as root, so this archive has -everything in `certs/`, the private key included. +deploy files. `docker exec` runs the command as root, so it reads the private +key whatever its permissions. ```sh docker exec selfpost-backup > selfpost-backup.tar.gz @@ -626,7 +634,7 @@ methods avoid it by stopping first. | SQLite consistency | `VACUUM INTO` snapshot | Safe when stopped | Safe when stopped | | `data/log/` | Excluded | Included (optional exclude) | Included (optional exclude) | | Postfix queue | No | Yes | Yes | -| `certs/` private key | CLI: yes. Panel: only if the panel may read it | Yes | No — deploy files separate | +| `certs/` private key | Yes (the panel needs it readable by group 998) | Yes | No — deploy files separate | | `data/manifest.json` | Written (version stamp) | Absent | Absent | | Restore version check | Yes — must match image tag | No | No | | Post-restore *Resync* | Yes, on first boot | No | No | diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 307c5ed..e7f091d 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -80,12 +80,6 @@ type Params struct { Version string DeployRoot string OnWarn func(string) - // Unprivileged says the archive is made by the panel, which runs as its - // own user and not as root. A file under certs/ it may not open - usually - // the private key, root and Postfix's group only - is then left out - // instead of failing the whole archive, and OnWarn names it. The - // selfpost-backup command, run as root, leaves this unset and takes it. - Unprivileged bool } // QueueDir is the Postfix queue, relative to the data directory. No backup @@ -164,11 +158,11 @@ func Create(w io.Writer, p Params) error { return err } - if err := addTree(tw, p.DataDir, DataArchivePrefix, excludedFromArchive, p.skipData()); err != nil { + if err := addTree(tw, p.DataDir, DataArchivePrefix, excludedFromArchive, skipQueue); err != nil { return err } - if err := addDeployFiles(tw, p.DeployRoot, p.OnWarn, p.skipCert()); err != nil { + if err := addDeployFiles(tw, p.DeployRoot, p.OnWarn); err != nil { return err } @@ -204,36 +198,9 @@ func validateDeployRoot(deployRoot string) error { // left out of the archive. A directory left out is not descended into. type skipFunc func(path, rel string, d fs.DirEntry) bool -// skipData leaves the Postfix queue out of every archive. -func (p Params) skipData() skipFunc { - return func(_, rel string, _ fs.DirEntry) bool { - return filepath.ToSlash(rel) == QueueDir - } -} - -// skipCert leaves a file under certs/ that cannot be opened out of an -// unprivileged archive, and says so through OnWarn. -func (p Params) skipCert() skipFunc { - if !p.Unprivileged { - return nil - } - return func(path, rel string, d fs.DirEntry) bool { - if d.IsDir() { - return false - } - f, err := os.Open(path) - if err == nil { - f.Close() - return false - } - if !errors.Is(err, fs.ErrPermission) { - return false - } - if p.OnWarn != nil { - p.OnWarn(CertsDirName + "/" + filepath.ToSlash(rel) + " is not readable by the panel and is left out of the backup") - } - return true - } +// skipQueue leaves the Postfix queue out of every archive. +func skipQueue(_, rel string, _ fs.DirEntry) bool { + return filepath.ToSlash(rel) == QueueDir } // Check reads through everything Create would put into the archive, without @@ -241,6 +208,10 @@ func (p Params) skipCert() skipFunc { // streams its archive to the browser, so once Create has begun a failure can // only cut the download short; this is run first, while a refusal can still be // shown as a page. +// +// The panel is not root, and nothing is left out on that account: its archive +// is the one selfpost-backup makes, or it is not made. What it most likely +// cannot read is the private key under certs/ - see UnreadableCert. func Check(p Params) error { if p.DataDir == "" || p.DBPath == "" { return fmt.Errorf("backup: DataDir and DBPath are required") @@ -248,15 +219,28 @@ func Check(p Params) error { if err := ValidateDeployRoot(p.DeployRoot); err != nil { return err } - quiet := p - quiet.OnWarn = nil - if err := addTree(nil, p.DataDir, DataArchivePrefix, excludedFromArchive, quiet.skipData()); err != nil { + if err := addTree(nil, p.DataDir, DataArchivePrefix, excludedFromArchive, skipQueue); err != nil { return err } - return addDeployFiles(nil, p.DeployRoot, nil, quiet.skipCert()) + return addDeployFiles(nil, p.DeployRoot, nil) } -func addDeployFiles(tw *tar.Writer, deployRoot string, onWarn func(string), skip skipFunc) error { +// UnreadableCert reports whether err, from Check or Create, is a file under +// the deploy root's certs/ that the caller is not allowed to read, and names +// it relative to the deploy root ("certs/privkey.pem"). +func UnreadableCert(err error, deployRoot string) (name string, ok bool) { + var pathErr *fs.PathError + if !errors.As(err, &pathErr) || !errors.Is(err, fs.ErrPermission) { + return "", false + } + rel, relErr := filepath.Rel(filepath.Join(deployRoot, CertsDirName), pathErr.Path) + if relErr != nil || rel == ".." || strings.HasPrefix(filepath.ToSlash(rel), "../") { + return "", false + } + return CertsDirName + "/" + filepath.ToSlash(rel), true +} + +func addDeployFiles(tw *tar.Writer, deployRoot string, onWarn func(string)) error { for _, name := range []string{ComposeFileName, EnvFileName} { src := filepath.Join(deployRoot, name) info, err := os.Stat(src) @@ -281,7 +265,7 @@ func addDeployFiles(tw *tar.Writer, deployRoot string, onWarn func(string), skip } return fmt.Errorf("backup: stat %s: %w", CertsDirName, err) } - return addTree(tw, certsDir, CertsDirName+"/", nil, skip) + return addTree(tw, certsDir, CertsDirName+"/", nil, nil) } // addTree walks root and adds every regular file (and directory, to preserve diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 01d6ab3..9edd6ed 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -5,7 +5,9 @@ import ( "bytes" "compress/gzip" "encoding/json" + "fmt" "io" + "io/fs" "os" "path/filepath" "strings" @@ -236,11 +238,11 @@ func TestCheckRestoreWrongFormatRejected(t *testing.T) { } } -// The panel's archive: like any other it has no Postfix queue, the rest is -// there, and Check agrees that it can be made. -func TestCreateUnprivilegedLeavesTheQueueOut(t *testing.T) { +// No archive holds the Postfix queue; the rest is there, the private key +// included, and Check agrees that it can be made. +func TestCreateLeavesTheQueueOut(t *testing.T) { dataDir, dbPath, deployRoot := seedDataDir(t) - p := Params{DataDir: dataDir, DBPath: dbPath, Version: "1.2.3", DeployRoot: deployRoot, Unprivileged: true} + p := Params{DataDir: dataDir, DBPath: dbPath, Version: "1.2.3", DeployRoot: deployRoot} if err := Check(p); err != nil { t.Fatalf("Check: %v", err) @@ -262,6 +264,7 @@ func TestCreateUnprivilegedLeavesTheQueueOut(t *testing.T) { DataArchivePrefix + "postfix/sender_login_maps", ComposeFileName, EnvFileName, CertsDirName + "/fullchain.pem", + CertsDirName + "/privkey.pem", } { if _, ok := files[name]; !ok { t.Errorf("archive missing %s", name) @@ -269,6 +272,28 @@ func TestCreateUnprivilegedLeavesTheQueueOut(t *testing.T) { } } +// A file under certs/ that may not be read is told apart from any other +// failure, so the panel can say what to do about it. +func TestUnreadableCert(t *testing.T) { + root := filepath.Join("srv", "deploy") + denied := func(path string) error { + return fmt.Errorf("backup: open %s: %w", path, &fs.PathError{Op: "open", Path: path, Err: fs.ErrPermission}) + } + if name, ok := UnreadableCert(denied(filepath.Join(root, "certs", "privkey.pem")), root); !ok || name != "certs/privkey.pem" { + t.Errorf("certs/privkey.pem: got %q, %v", name, ok) + } + if name, ok := UnreadableCert(denied(filepath.Join(root, ".env")), root); ok { + t.Errorf(".env was taken for a certificate file: %q", name) + } + if name, ok := UnreadableCert(denied(filepath.Join("data", "postfix", "x")), root); ok { + t.Errorf("a data file was taken for a certificate file: %q", name) + } + missing := &fs.PathError{Op: "open", Path: filepath.Join(root, "certs", "privkey.pem"), Err: fs.ErrNotExist} + if _, ok := UnreadableCert(missing, root); ok { + t.Error("a missing file was taken for one that may not be read") + } +} + // Check finds what Create would stumble over, before anything is written. func TestCheckReportsWhatCannotBeRead(t *testing.T) { dataDir, dbPath, deployRoot := seedDataDir(t) diff --git a/internal/web/handlers/handlers_backup.go b/internal/web/handlers/handlers_backup.go index 588e761..783606d 100644 --- a/internal/web/handlers/handlers_backup.go +++ b/internal/web/handlers/handlers_backup.go @@ -70,14 +70,11 @@ func (h *Handlers) HandleBackup(w http.ResponseWriter, r *http.Request) { h.renderBackupPageWith(w, r, http.StatusBadRequest, "", pwErr) return } - // The panel is not root: its archive leaves out the Postfix queue and a - // certificate file it may not open (backup.Params.Unprivileged). params := backup.Params{ - DataDir: h.cfg.DataDir, - DBPath: h.cfg.DBPath, - Version: h.cfg.Version, - DeployRoot: h.cfg.DeployRoot, - Unprivileged: true, + DataDir: h.cfg.DataDir, + DBPath: h.cfg.DBPath, + Version: h.cfg.Version, + DeployRoot: h.cfg.DeployRoot, OnWarn: func(msg string) { logf("panel: full backup: %s", msg) }, @@ -90,11 +87,11 @@ func (h *Handlers) HandleBackup(w http.ResponseWriter, r *http.Request) { // Everything is read through once before the first byte goes out. Past the // headers a failure can only cut the download short - which is what the // button did from 1.3.0 to 2.0.0, with a 200 and ten bytes, because the - // queue was not the panel's to read. + // queue was not the panel's to read. Nothing is skipped to make the archive + // possible: it is the one selfpost-backup makes, or it is refused. if err := backup.Check(params); err != nil { logf("panel: full backup: %v", err) - h.renderBackupPageWith(w, r, http.StatusInternalServerError, "", - "Could not read everything a backup holds. Nothing was downloaded; the reason is in the panel's log.") + h.renderBackupPageWith(w, r, http.StatusInternalServerError, "", unreadableBackupErr(err, h.cfg.DeployRoot)) return } @@ -333,6 +330,18 @@ func decryptErrorMessage(err error) string { } } +// unreadableBackupErr phrases what backup.Check found. The usual case is the +// private key: the panel runs as its own user, and a key readable by root +// alone is not its to read. The way out is on the host, so the message says +// what to do there (guide § Taking a full backup). +func unreadableBackupErr(err error, deployRoot string) string { + if name, ok := backup.UnreadableCert(err, deployRoot); ok { + 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." + } + return "Could not read everything a backup holds. Nothing was downloaded; the reason is in the panel's log." +} + // deployBackupErr phrases a pre-flight backup failure for the operator. func deployBackupErr(err error) string { if strings.Contains(err.Error(), "DeployRoot") || strings.Contains(err.Error(), "deploy root") { diff --git a/internal/web/view/help.go b/internal/web/view/help.go index 6b66ae4..60a770a 100644 --- a/internal/web/view/help.go +++ b/internal/web/view/help.go @@ -193,7 +193,8 @@ var serverTopics = []HelpTopic{ ". The Postfix queue is not in it. The backup is taken while the server is running, and Postfix keeps moving messages through the queue, "+ "so a copy could hold a message twice or miss it; mail still waiting for delivery is therefore not carried over, and a restored server "+ "starts with an empty queue. To keep the queue, stop the container and archive the project directory instead. The private key in ", Code("certs/"), - " is usually missing too, because the panel is not allowed to read it; ", Code("selfpost-backup"), " run in the container takes the key. It holds secrets, so leave Encrypt with a password ticked: the file is sealed as ", Code(".spbk"), + " has to be readable by the panel: give it to group 998 on the host (", Code("chgrp 998"), ", ", Code("chmod 640"), + "). Until then the panel refuses to make a backup and names the file, rather than hand over an archive without the key. It holds secrets, so leave Encrypt with a password ticked: the file is sealed as ", Code(".spbk"), ", the password is not stored, and without it nothing can be recovered. To restore, unpack it into an empty project directory (decrypt a ", Code(".spbk"), " first with ", Code("selfpost-backup -decrypt"), ") and start the same SelfPost version that made it; the panel refuses data from another version. The reverse-proxy configuration and ", diff --git a/internal/web/view/templates/backup.html b/internal/web/view/templates/backup.html index 6e712a2..126e0cb 100644 --- a/internal/web/view/templates/backup.html +++ b/internal/web/view/templates/backup.html @@ -13,7 +13,7 @@
{{template "box_open" .Full}} {{template "body_open"}} -

data/ (database, DKIM keys, credentials), docker-compose.yml, .env and certs/ in one archive. The Postfix queue is left out: it cannot be copied reliably while the server is running. So is a certificate file the panel may not read. Restore into an empty project directory and start the same SelfPost version. The reverse-proxy vhost is not included.

+

data/ (database, DKIM keys, credentials), docker-compose.yml, .env and certs/ in one archive. The Postfix queue is left out: it cannot be copied reliably while the server is running. Restore into an empty project directory and start the same SelfPost version. The reverse-proxy vhost is not included.

diff --git a/test/e2e/restore_test.go b/test/e2e/restore_test.go index 5d2bae2..d1dee90 100644 --- a/test/e2e/restore_test.go +++ b/test/e2e/restore_test.go @@ -17,12 +17,30 @@ import ( // testFullBackupRestore takes a full backup from the panel, throws the // installation away and brings it back the way the guide tells an operator to // (§ Full backup and restore): unpack the archive into an empty project -// directory, put the private key back, start the same image. What has to come back is what an operator +// directory, start the same image. What has to come back is what an operator // cannot recreate - the administrator, the domain with its DKIM key, the // application with its password - and the proof is the same one the first // send gave: a message signed with the restored key verifies against the // record that was published before the backup. func testFullBackupRestore(t *testing.T, sc *scenario) { + // A private key the panel may not read - the state a server is in until + // its operator gives the key to group 998. The panel does not make an + // archive without it: it refuses, names the file and says what to do. + keyPath := filepath.Join(h.stageDir, "certs", "privkey.pem") + if err := os.Chmod(keyPath, 0o600); err != nil { + t.Fatal(err) + } + resp, body, err := sc.panel.postForm("/server/backup", url.Values{}) + if err != nil { + t.Fatalf("full backup with an unreadable key: %v", err) + } + if resp.StatusCode != 500 || !strings.Contains(body, "not allowed to read certs/privkey.pem") || !strings.Contains(body, "chgrp 998") { + t.Fatalf("full backup with an unreadable key: status %d, want a refusal that names the file and the way out:\n%.800s", resp.StatusCode, body) + } + if err := os.Chmod(keyPath, 0o644); err != nil { + t.Fatal(err) + } + resp, archive, err := sc.panel.postForm("/server/backup", url.Values{}) if err != nil { t.Fatalf("full backup: %v", err) @@ -41,14 +59,6 @@ func testFullBackupRestore(t *testing.T, sc *scenario) { // The installation goes: containers down, then everything the archive is // supposed to bring back. The fake zone and the sink's directory are the // outside world and stay. - // The private key is the operator's to keep: the panel may not read it (it - // is the host user's, mode 0600, here as on a real server) and its archive - // comes without it. - keyPath := filepath.Join(h.stageDir, "certs", "privkey.pem") - key, err := os.ReadFile(keyPath) - if err != nil { - t.Fatalf("read the stand's private key: %v", err) - } h.reclaimData() h.down() for _, name := range []string{"data", "certs", "docker-compose.yml", ".env"} { @@ -60,21 +70,16 @@ func testFullBackupRestore(t *testing.T, sc *scenario) { if err != nil { t.Fatalf("unpack the backup into the empty project directory: %v", err) } - for _, want := range []string{"docker-compose.yml", ".env", "data/manifest.json", "data/selfpost.db", "certs/fullchain.pem"} { + for _, want := range []string{"docker-compose.yml", ".env", "data/manifest.json", "data/selfpost.db", "certs/fullchain.pem", "certs/privkey.pem"} { if !names[want] { t.Fatalf("the backup has no %s (it has %d entries)", want, len(names)) } } - // The panel's archive is the one without the Postfix queue (guide § Taking - // a full backup): the restored instance has to build an empty one. + // No backup holds the Postfix queue (guide § Full backup and restore): the + // restored instance has to build an empty one. for name := range names { if name == "data/postfix/queue" || strings.HasPrefix(name, "data/postfix/queue/") { - t.Fatalf("the panel's backup holds %s", name) - } - } - if !names["certs/privkey.pem"] { - if err := os.WriteFile(keyPath, key, 0o600); err != nil { - t.Fatalf("put the private key back: %v", err) + t.Fatalf("the backup holds %s", name) } } diff --git a/test/e2e/stage.go b/test/e2e/stage.go index b03c265..454bbae 100644 --- a/test/e2e/stage.go +++ b/test/e2e/stage.go @@ -113,5 +113,13 @@ func writeSelfSignedCert(certPath, keyPath string) error { return err } keyPEM := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) - return os.WriteFile(keyPath, keyPEM, 0o600) + // Readable by everyone, which a real key must never be. On a server the + // operator gives the key to the container's selfpost group (998) so the + // panel can put it into a full backup; the runner of this suite is not + // always root and cannot hand a file to a group it is not in. The key is + // thrown away with the stage directory. + if err := os.WriteFile(keyPath, keyPEM, 0o644); err != nil { + return err + } + return os.Chmod(keyPath, 0o644) }