diff --git a/CHANGELOG.md b/CHANGELOG.md index d9b37fe..a1f363c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version password without the current one, which then passed the backup's password check. Adding a user, setting a password, changing an e-mail or a role on the user form, and deleting a user now ask for your own account password, - as a backup does, under the same limit on wrong guesses. + as a backup does, under the same limit on wrong guesses - which now + counts wrong passwords only. - **A domain's rate limit is taken only by its own applications** (audit M1): the level-2 domain limit was read from `MAIL FROM`, which reaches the milter before Postfix holds it to the login, so a client that never diff --git a/build/entrypoint.sh b/build/entrypoint.sh index e92a802..dc3c3c9 100644 --- a/build/entrypoint.sh +++ b/build/entrypoint.sh @@ -150,11 +150,11 @@ rm -f /data/sasl/sasldb2.new # Postfix state under /data (architecture.md § Persistence). The panel writes # sender_login_maps; Postfix owns the on-disk queue tree under queue/. mkdir -p /data/postfix/queue -[ -e /data/postfix/sender_login_maps ] || : > /data/postfix/sender_login_maps +[ -e /data/postfix/sender_login_maps ] || [ -L /data/postfix/sender_login_maps ] || : > /data/postfix/sender_login_maps # Inbound relay maps (written by the panel when INBOUND_RELAY_ENABLE=true). # 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" + [ -e "/data/postfix/$f" ] || [ -L "/data/postfix/$f" ] || : > "/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 @@ -196,7 +196,7 @@ chmod 0755 /data/antispam # because parent directory has insecure permissions" when the directory's # group is not root and has write permission. mkdir -p /data/log -[ -e /data/log/mail.log ] || : > /data/log/mail.log +[ -e /data/log/mail.log ] || [ -L /data/log/mail.log ] || : > /data/log/mail.log chown -R postfix:selfpost /data/log chmod 2750 /data/log find /data/log -type f -exec chmod 0640 {} + diff --git a/docs/security.md b/docs/security.md index 15e8215..58b270e 100644 --- a/docs/security.md +++ b/docs/security.md @@ -503,7 +503,10 @@ operator can act on it. user and when a save sets a password, changes the e-mail (where reset links go) or changes the role - the signed-in user's own account included - and deleting a user asks for it too. A save of the name and the domain - assignments alone is not asked: neither reaches an account's secret. + assignments alone is not asked: neither reaches an account's secret. Only + wrong passwords count against the limiter; a right one gives its attempt + back, so an administrator adding users is not locked out of the next + backup. - Both files are secrets: a full backup carries DKIM keys, `sasldb2`, the panel users' password hashes, `docker-compose.yml`, `.env`, and the TLS private key from `certs/` when present; a domain export carries the DKIM key diff --git a/internal/web/auth/auth.go b/internal/web/auth/auth.go index 962b471..5621559 100644 --- a/internal/web/auth/auth.go +++ b/internal/web/auth/auth.go @@ -100,6 +100,14 @@ func (m *Module) AllowPasswordChange(userID int64) bool { return m.passwordLimiter.Allow("user:" + strconv.FormatInt(userID, 10)) } +// PasswordCheckPassed gives back the attempt AllowPasswordChange took for a +// check that found the right password: only wrong guesses count, so an +// administrator who adds ten users in a quarter of an hour is not locked out +// of the next backup (review of audit 2026-10-17 stage 0). +func (m *Module) PasswordCheckPassed(userID int64) { + m.passwordLimiter.Refund("user:" + strconv.FormatInt(userID, 10)) +} + // SessionToken returns the session token the request carries, if exactly one // cookie of that name is present. func (m *Module) SessionToken(r *http.Request) (string, bool) { diff --git a/internal/web/auth/links.go b/internal/web/auth/links.go index 28b31c8..27cb5eb 100644 --- a/internal/web/auth/links.go +++ b/internal/web/auth/links.go @@ -407,7 +407,7 @@ func loginInfo(r *http.Request) string { // Every log line that names a request's path goes through it: the origin // check's refusal and the duplicate-cookie line (audit 2026-10-17, A4). func LoggedPath(p string) string { - for _, prefix := range []string{"/login/reset/", "/confirm/"} { + for _, prefix := range []string{"/login/reset/", "/confirm/", "/setup/"} { if strings.HasPrefix(p, prefix) && len(p) > len(prefix) { return prefix + "…" } diff --git a/internal/web/auth/loggedpath_test.go b/internal/web/auth/loggedpath_test.go index b02514b..9c7a984 100644 --- a/internal/web/auth/loggedpath_test.go +++ b/internal/web/auth/loggedpath_test.go @@ -8,6 +8,7 @@ func TestLoggedPathHidesTokens(t *testing.T) { for in, want := range map[string]string{ "/login/reset/abcDEF123": "/login/reset/…", "/confirm/abcDEF123": "/confirm/…", + "/setup/abcDEF123": "/setup/…", "/login/reset": "/login/reset", "/confirm/": "/confirm/", "/account/profile": "/account/profile", diff --git a/internal/web/auth/ratelimit.go b/internal/web/auth/ratelimit.go index db3f794..14b74e0 100644 --- a/internal/web/auth/ratelimit.go +++ b/internal/web/auth/ratelimit.go @@ -74,6 +74,17 @@ func (r *rateLimiter) Allow(key string) bool { return true } +// Refund gives back the attempt Allow took for key, for an attempt that +// turned out not to be a guess (a right password). A bucket already at its +// limit stays there: a refund never unblocks. +func (r *rateLimiter) Refund(key string) { + r.mu.Lock() + defer r.mu.Unlock() + if b := r.buckets[key]; b != nil && b.count > 0 && b.count < r.max { + b.count-- + } +} + func (r *rateLimiter) makeRoom(now time.Time) { if r.maxBuckets <= 0 || len(r.buckets) < r.maxBuckets { return diff --git a/internal/web/handlers/handlers_backup.go b/internal/web/handlers/handlers_backup.go index f1e5326..523a228 100644 --- a/internal/web/handlers/handlers_backup.go +++ b/internal/web/handlers/handlers_backup.go @@ -353,6 +353,7 @@ func (h *Handlers) reauth(r *http.Request, what string) (int, string) { logf("panel: %s: user %d gave a wrong account password", what, p.ID) return http.StatusUnauthorized, "Your account password is incorrect." } + h.auth.PasswordCheckPassed(p.ID) return 0, "" } diff --git a/internal/web/handlers/handlers_users_reauth_test.go b/internal/web/handlers/handlers_users_reauth_test.go index bb7faf1..093d26c 100644 --- a/internal/web/handlers/handlers_users_reauth_test.go +++ b/internal/web/handlers/handlers_users_reauth_test.go @@ -134,3 +134,18 @@ func TestUserFormAsksForTheAccountPassword(t *testing.T) { t.Fatalf("delete with the right password = %d:\n%s", rec.Code, rec.Body.String()) } } + +// Only wrong guesses count against the account-password limit: a dozen +// right ones in a row - an administrator adding users - leave the next +// backup open (review of audit 2026-10-17 stage 0). +func TestRightAccountPasswordsDoNotUseUpTheLimit(t *testing.T) { + h, st := inboundHandlers(t) + root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + withAccountPassword(t, h, st, root) + for i := 0; i < 12; i++ { + form := url.Values{"username": {"new-user-" + idStr(int64(i))}, "password": {"a-long-enough-password-1"}, "role": {"global"}, "account_password": {testAccountPassword}} + if rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, form); rec.Code != http.StatusSeeOther { + t.Fatalf("user %d = %d:\n%.600s", i, rec.Code, rec.Body.String()) + } + } +}