From 08087795cd039285e9469f317ad82eb28da91bd0 Mon Sep 17 00:00:00 2001 From: Mix Date: Sat, 17 Oct 2026 22:40:24 +0300 Subject: [PATCH] docs, comments: the drift the audit listed (audit 2026-10-17, stage 6) The lines under "Doc and comment drift" that the findings' own commits did not already fix: - security.md: a newer confirmation link replaces the older, a reset link is not replaced; html/template renders every page (no themes); the container keeps eight capabilities, named, not "KILL". - architecture.md: ImportKey writes over a name's key, as an import must; the error text reaches the browser on the Backup page's import refusal. - guide: with Caddy the key is in Caddy's data, so a full backup refuses until certs/ holds a copy (or back up caddy-data). - comments: sasldb2 holds application passwords in the clear (store, app/password.go); texthash folds ASCII (sender.go); the supervisor socket's group is supervisor (rpc.go); cert-reload's OpenSSL measurement was 3.0.x of bookworm; sendReset's limits (links.go). Migration 0013's comment stays as shipped. Co-Authored-By: Claude Opus 5.5 --- build/postfix-cert-reload.sh | 3 ++- docs/architecture.md | 7 +++++-- docs/guide.md | 6 ++++++ docs/security-audit-2026-10-17.md | 7 +++++-- docs/security.md | 16 ++++++++++------ internal/app/password.go | 3 ++- internal/milter/sender.go | 11 ++++++----- internal/store/applications.go | 4 ++-- internal/supervisor/rpc.go | 2 +- internal/web/auth/links.go | 3 ++- 10 files changed, 41 insertions(+), 21 deletions(-) diff --git a/build/postfix-cert-reload.sh b/build/postfix-cert-reload.sh index 59a3236..f96acb1 100644 --- a/build/postfix-cert-reload.sh +++ b/build/postfix-cert-reload.sh @@ -43,7 +43,8 @@ reload() { # same_pair says whether a certificate file and a private key file belong # together, by comparing the public key each one prints. openssl-x509(1) and # openssl-pkey(1) do not say the two print it byte for byte alike; measured -# in the image, 2026-10-15, OpenSSL 3.0.22: identical for RSA-2048, P-256 and +# in the image, 2026-10-15, OpenSSL 3.0.x of bookworm, before trixie's 3.5 +# (re-read on 3.5.7 is due with the next look at this script): identical for RSA-2048, P-256 and # P-384 keys, and for a P-256 key written in SEC1 form ("EC PRIVATE KEY") - # certbot 2.0 and later issue ECDSA keys by default (assumptions C-X1). same_pair() { diff --git a/docs/architecture.md b/docs/architecture.md index 5c8d5f0..70f0ccf 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -243,7 +243,9 @@ then OpenDKIM did, in a program of its own (`internal/domain`, `Keys`; `DKIM_DIR`, default `/data/dkim`). A key is made when a domain is added and an existing one is reused, never overwritten: that would invalidate the record already published - (`EnsureKey`). A domain export carries the key and an import writes it. A + (`EnsureKey`). A domain export carries the key and an import writes it, + over whatever key the name had (`ImportKey`: the import is the domain + with exactly that key). A deleted domain's key directory is removed once the selector table no longer names it; one a deletion had to leave, Reload configuration removes (`Service.Resync`). @@ -1031,7 +1033,8 @@ A validation refusal is shown as the text the validator wrote (on the form, next to the field, or in a notice above the first box). Anything else — a database error, `saslpasswd2`, a Postfix reload — is logged and the page gets a fixed sentence saying so; the error text never reaches the -browser. A template error renders a bare `500` rather than a half-written page +browser - save on the global-only Backup page, where an import's refusal +names what is wrong with the file (security.md § Output and process). A template error renders a bare `500` rather than a half-written page (`Engine.Render` buffers). ### Sessions diff --git a/docs/guide.md b/docs/guide.md index 4b381ed..9415280 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -407,6 +407,12 @@ docker compose -f docker-compose.yml -f caddy/docker-compose.caddy.yml up -d Verify Caddy's on-disk cert path for your version before relying on the default mount — see the comment at the top of the Caddy compose fragment. +With Caddy the certificate lives in Caddy's own data, not in `certs/`, and +Caddy writes its keys readable by itself alone: the panel's full backup then +refuses with "certs/ holds no certificate". Back up `./caddy/caddy-data` +with the project directory instead, or keep a copy of the pair in `certs/`, +the key given to group 10997 (`chgrp 10997`, `chmod 640`), refreshed after +each renewal. **Traefik (containerised).** Edit the `Host(...)` label and ACME email in [deploy/traefik/docker-compose.traefik.yml](../deploy/traefik/docker-compose.traefik.yml), diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 3687323..25dba86 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -855,8 +855,11 @@ the argument. M6, M8 in code (a session from a longer window is cut at the next check; three headers signed; a long header line logged); A6, A7, W7 by wording in security.md - each is what the code should do; the README note. -- [ ] **Doc drift sweep** — every line under "Doc and comment drift" above. - — **Sonnet**, 1 h. +- [x] **Doc drift sweep** — every line under "Doc and comment drift" above. + — **Sonnet**, 1 h. *Done 2026-10-17:* + every line, each with the finding that fixed it or in this sweep; left as + history: migration `0013`'s comment (a migration is not edited after it + shipped); the live compose is re-copied at the next rollout. - **Accept, with the argument:** *the setuid/setgid binaries of the base image* — inert under `no-new-privileges`, which the shipped compose sets and the e2e exercises; removing Debian's set is a maintenance cost at every diff --git a/docs/security.md b/docs/security.md index 4930502..0afe6c4 100644 --- a/docs/security.md +++ b/docs/security.md @@ -171,8 +171,10 @@ The panel is exposed to the internet — the items below are **not optional**. is 256 random bits (`crypto/rand`); `user_links` keeps its SHA-256 only, so the database or a backup opens nothing. A confirmation link lives 24 hours, a reset link 30 minutes; each is taken once, in the transaction - that deletes it (`DELETE … RETURNING`), and a newer link of the same kind - replaces the older. A reset link holds only while the account's address is + that deletes it (`DELETE … RETURNING`); a newer confirmation link replaces + the older one, while a reset link is not replaced - the one in the mailbox + works until it is used or expires, and another is not sent within ten + minutes of it. A reset link holds only while the account's address is still the confirmed one it was sent to; a confirmation link only while the address is the one it confirms. The link's host is `SELFPOST_HOSTNAME`, never the request's `Host`, which an attacker would choose to receive the @@ -278,8 +280,8 @@ operator can act on it. ### Output and process -- Rendering goes through `html/template` with auto-escaping (queue, log, - journal, themes). +- Rendering goes through `html/template` with auto-escaping (queue, logs, + journal, every page). - The panel process is **not root** (`user=panel` in supervisord); path access is granted through the `selfpost` group with minimal permissions. The supervisord control socket, which can stop or restart any program, is the @@ -299,8 +301,10 @@ operator can act on it. `2.14.4` the guide gave the key to `selfpost`, so every Postfix daemon - an smtpd talking to the internet included - could read it (security audit 2026-10-17, I1; measured on the test server with `docker exec -u postfix`). -- **The container keeps `KILL`** (`deploy/docker-compose.yml`, `cap_add`; - every other capability not listed there is dropped). Without it root +- **The container keeps eight capabilities**, `KILL` among them + (`deploy/docker-compose.yml`, `cap_add`: NET_BIND_SERVICE, CHOWN, FOWNER, + FSETID, SETUID, SETGID, DAC_OVERRIDE, KILL, each with its reason there; + every other is dropped). Without it root cannot signal a process of another uid (kill(2), capabilities(7); measured on the test server, 2026-10-16), and two root processes must: supervisord stops and restarts the panel, and Postfix's master ends its children, diff --git a/internal/app/password.go b/internal/app/password.go index 9e27703..1f70b86 100644 --- a/internal/app/password.go +++ b/internal/app/password.go @@ -13,7 +13,8 @@ const passwordBytes = 24 // generatePassword returns a strong, URL-safe random password for an // application's SASL account. The panel generates it, shows it once and never -// stores the plaintext (security.md); sasldb2 keeps only the hashed form. +// stores it in its own database; sasldb2 keeps it, in the clear, as Cyrus +// does for PLAIN and LOGIN (security.md). // // base64url output keeps the password to a safe ASCII alphabet with no shell or // SMTP-special characters, so it survives being typed into client configuration diff --git a/internal/milter/sender.go b/internal/milter/sender.go index 4f39d93..bebe217 100644 --- a/internal/milter/sender.go +++ b/internal/milter/sender.go @@ -202,11 +202,12 @@ func splitTableLine(line string) (key, value string, ok bool) { // tries for a local domain is never a key here: every key the panel writes // has an "@" (internal/postfix renderSenderLoginMaps). // -// Case is folded in ASCII only. Postfix folds as Unicode (src/util/casefold.c -// with ICU, which Debian builds with), but every key and login the panel -// writes is ASCII; a Unicode fold would only let a look-alike pass - the -// Kelvin sign for "k" - for an address that is not the domain's (review of -// stage 2c). +// Case is folded in ASCII only, as Postfix folds the keys of the texthash +// map the panel writes (src/util/dict_thash.c:113 and dict_ht.c:58-64, +// lowercase(): ASCII; security audit 2026-10-17 corrected this comment, which +// said Unicode). Every key and login the panel writes is ASCII; a Unicode fold +// would only let a look-alike pass - the Kelvin sign for "k" - for an address +// that is not the domain's (review of stage 2c). func (b *Bindings) Allowed(login, addr string) (bool, error) { b.mu.Lock() defer b.mu.Unlock() diff --git a/internal/store/applications.go b/internal/store/applications.go index 7e5b2b6..0a1b1a9 100644 --- a/internal/store/applications.go +++ b/internal/store/applications.go @@ -29,8 +29,8 @@ const ( // Application is a SASL account bound to a single domain (product.md § // Multi-domain model). The password is never stored here — only in sasldb2, -// hashed — so it can be shown exactly once at creation/regeneration -// (security.md). Addresses is populated only in 'list' mode. +// which Cyrus keeps in the clear (PLAIN and LOGIN need it; security.md) — so +// it can be shown exactly once at creation/regeneration. Addresses is populated only in 'list' mode. type Application struct { ID int64 DomainID int64 diff --git a/internal/supervisor/rpc.go b/internal/supervisor/rpc.go index a6ae74c..5a6d67f 100644 --- a/internal/supervisor/rpc.go +++ b/internal/supervisor/rpc.go @@ -15,7 +15,7 @@ import ( ) // socketPath is supervisord's control socket: build/supervisord.conf, -// [unix_http_server] file=, group selfpost, which the panel is in. +// [unix_http_server] file=, group supervisor, whose one member is the panel. const socketPath = "/run/supervisor.sock" // runOnceFor bounds RunOnce: a reload that is still running after it is diff --git a/internal/web/auth/links.go b/internal/web/auth/links.go index 08cbae2..0340d1f 100644 --- a/internal/web/auth/links.go +++ b/internal/web/auth/links.go @@ -168,7 +168,8 @@ func (m *Module) HandleReset(w http.ResponseWriter, r *http.Request) { } // sendReset mails the account named username a reset link if its address is -// confirmed, at most three an hour. +// confirmed: not within resetSpacing of the last, five a day (then one +// notice) and five an hour. func (m *Module) sendReset(username string) { if username == "" || m.mailer == nil || m.cfg.Hostname == "" { return