From b87baa6dd6e466941ea288daf211718582da415c Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Wed, 12 Aug 2026 00:51:37 +0300 Subject: [PATCH] fix: pin logrotate config mode in image and fail loud on bad permissions COPY --chmod makes /etc/logrotate.d/mail 0644 regardless of build context file modes (Windows tar sync). logrotate-loop preflight exits non-zero when logrotate would ignore the config. E2e covers mode, forced rotation, and a group-writable context build. Co-Authored-By: Composer 2.5 Co-authored-by: Cursor --- CHANGELOG.md | 9 ++++ build/Dockerfile | 26 +++++----- build/logrotate-loop.sh | 41 ++++++++++++++- build/supervisord.conf | 5 +- docs/development.md | 6 ++- docs/plans/logrotate-mode.md | 99 +++++++----------------------------- docs/roadmap.md | 1 - test/e2e/logrotate_check.go | 97 +++++++++++++++++++++++++++++++++++ test/e2e/main_test.go | 6 +++ 9 files changed, 191 insertions(+), 99 deletions(-) create mode 100644 test/e2e/logrotate_check.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 46caae9..2c3de1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ## [Unreleased] +### Fixed + +- image: `mail.log` rotation no longer silently stops when the build context + ships `logrotate-mail.conf` with group/other write bits (common after a + Windows checkout sync). Runtime `COPY --chmod` pins config and script modes + in the Dockerfile; `logrotate-loop.sh` refuses a config logrotate would + ignore. E2e checks mode `644`, forced rotation, and a group-writable context + build. + ## [1.2.2] - 2026-08-12 Status page layout after 1.2.1: paired cards in a wide column, denser machine diff --git a/build/Dockerfile b/build/Dockerfile index 0631979..6215816 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -88,20 +88,20 @@ COPY --from=build /out/selfpost-backup /usr/local/bin/selfpost-backup # Licence text shipped with the image (AGPL-3.0 conveyance). The panel also # serves the same text at /license from an embedded copy. -COPY LICENSE NOTICE /usr/share/doc/selfpost/ +COPY --chmod=0644 LICENSE NOTICE /usr/share/doc/selfpost/ -COPY build/opendkim.conf /etc/opendkim.conf -COPY build/logrotate-mail.conf /etc/logrotate.d/mail -COPY build/postfix-wrapper.sh /usr/local/bin/postfix-wrapper.sh -COPY build/postfix-config.sh /usr/local/bin/postfix-config.sh -COPY build/postfix-cert-reload.sh /usr/local/bin/postfix-cert-reload.sh -COPY build/logrotate-loop.sh /usr/local/bin/logrotate-loop.sh -COPY build/crashexit.py /usr/local/bin/crashexit.py -COPY build/entrypoint.sh /usr/local/bin/entrypoint.sh -COPY build/supervisord.conf /etc/supervisor/supervisord.conf -RUN chmod +x /usr/local/bin/postfix-wrapper.sh /usr/local/bin/postfix-config.sh \ - /usr/local/bin/postfix-cert-reload.sh /usr/local/bin/logrotate-loop.sh \ - /usr/local/bin/crashexit.py /usr/local/bin/entrypoint.sh +# File modes are pinned here so a build context copied from a checkout without +# POSIX permissions (e.g. Windows tar sync) cannot land group-writable config +# that logrotate would silently ignore — see docs/plans/logrotate-mode.md. +COPY --chmod=0644 build/opendkim.conf /etc/opendkim.conf +COPY --chmod=0644 build/logrotate-mail.conf /etc/logrotate.d/mail +COPY --chmod=0755 build/postfix-wrapper.sh /usr/local/bin/postfix-wrapper.sh +COPY --chmod=0755 build/postfix-config.sh /usr/local/bin/postfix-config.sh +COPY --chmod=0755 build/postfix-cert-reload.sh /usr/local/bin/postfix-cert-reload.sh +COPY --chmod=0755 build/logrotate-loop.sh /usr/local/bin/logrotate-loop.sh +COPY --chmod=0755 build/crashexit.py /usr/local/bin/crashexit.py +COPY --chmod=0755 build/entrypoint.sh /usr/local/bin/entrypoint.sh +COPY --chmod=0644 build/supervisord.conf /etc/supervisor/supervisord.conf # Published submission ports: 465 (smtps, primary) and 587 (submission, optional) # plus the panel on 8080. Outbound delivery dials remote MXs on 25 as a client, diff --git a/build/logrotate-loop.sh b/build/logrotate-loop.sh index c934d21..4a16970 100644 --- a/build/logrotate-loop.sh +++ b/build/logrotate-loop.sh @@ -14,10 +14,49 @@ # often than daily — polling merely bounds how late a legitimate rotation runs. set -eu +CONFIG=/etc/logrotate.d/mail INTERVAL="${LOGROTATE_INTERVAL_SECONDS:-21600}" +# logrotate refuses configs writable by group or others and exits 0 while +# ignoring them — fail here so supervisord reports the fault. +logrotate_config_ok() { + mode=$(stat -c '%a' "$CONFIG") + mode=${mode#0} + grp=$(( (mode / 10) % 10 )) + oth=$(( mode % 10 )) + case $grp in 2|3|6|7) return 1 ;; esac + case $oth in 2|3|6|7) return 1 ;; esac + return 0 +} + +logrotate_config_fatal() { + echo "logrotate-loop: refusing to run: $CONFIG mode $(stat -c '%a' "$CONFIG") is writable by group or others" >&2 + exit 1 +} + +if ! logrotate_config_ok; then + logrotate_config_fatal +fi + +run_logrotate() { + out=$(logrotate "$CONFIG" 2>&1) || { + echo "$out" >&2 + return 1 + } + case "$out" in + *Ignoring*|*Potentially\ dangerous\ mode*) + echo "$out" >&2 + logrotate_config_fatal + ;; + esac + return 0 +} + while true; do - if logrotate /etc/logrotate.d/mail; then + if ! logrotate_config_ok; then + logrotate_config_fatal + fi + if run_logrotate; then : else echo "logrotate-loop: logrotate failed, will retry after ${INTERVAL}s" >&2 diff --git a/build/supervisord.conf b/build/supervisord.conf index 99b3234..fef65fc 100644 --- a/build/supervisord.conf +++ b/build/supervisord.conf @@ -104,8 +104,9 @@ stderr_logfile=/dev/stderr stderr_logfile_maxbytes=0 ; Periodic logrotate for /data/log/mail.log (spec 9, 10: daily, 7-14 files kept -; in the image). Runs as root so logrotate can read/rotate the log; never exits -; non-zero, so it neither trips the crashexit listener nor needs restarting. +; in the image). Runs as root so logrotate can read/rotate the log. Exits +; non-zero when the config is group/other-writable (logrotate would ignore it +; silently); autorestart surfaces BACKOFF on the Status page. [program:logrotate] command=/usr/local/bin/logrotate-loop.sh priority=400 diff --git a/docs/development.md b/docs/development.md index 0bfb555..93f36b8 100644 --- a/docs/development.md +++ b/docs/development.md @@ -162,8 +162,10 @@ docker build -f build/Dockerfile -t selfpost:dev --build-arg VERSION=dev . ``` The Dockerfile has a build stage (`go vet`, `go build` with `VERSION`) and a -runtime stage (Debian + mail stack). See [architecture.md](architecture.md) § -Image and processes. +runtime stage (Debian + mail stack). Runtime config and scripts use `COPY +--chmod` so file modes in the image do not depend on how the build context was +synced (e.g. a Windows checkout widening permissions on `logrotate-mail.conf`). +See [architecture.md](architecture.md) § Image and processes. --- diff --git a/docs/plans/logrotate-mode.md b/docs/plans/logrotate-mode.md index ba73bd9..e16ecdd 100644 --- a/docs/plans/logrotate-mode.md +++ b/docs/plans/logrotate-mode.md @@ -1,89 +1,28 @@ # Plan: logrotate-mode (mail.log stops rotating in some images) -**Status:** candidate -**Version:** patch; no schema, no configuration surface. -**Order:** independent. Worth doing before anything that lets an instance run -unattended for months. +**Status:** done +**Version:** patch; no schema, no configuration surface. --- -## What was observed +## Summary -The container log carries, on every start: +`mail.log` stopped rotating when `/etc/logrotate.d/mail` landed in the image +with group/other write permission (e.g. build context from a Windows tar sync). +logrotate ignores such configs but exits 0, so the loop looked healthy while +the log grew without bound. -``` -warning: Potentially dangerous mode on /etc/logrotate.d/mail: 0664 -error: Ignoring /etc/logrotate.d/mail because it is writable by group or others. -``` +## What shipped -logrotate refuses a configuration file that group or others may write, so -`mail.log` is never rotated in an image with that mode. It grows until the -volume does. +- [`build/Dockerfile`](../../build/Dockerfile): `COPY --chmod` pins config + (`0644`) and script (`0755`) modes so the image no longer depends on checkout + file modes. +- [`build/logrotate-loop.sh`](../../build/logrotate-loop.sh): preflight and + per-iteration checks refuse group/other-writable configs; logrotate stderr + mentioning `Ignoring` is fatal. +- [`test/e2e/logrotate_check.go`](../../test/e2e/logrotate_check.go): asserts + mode `644`, forced `logrotate -f`, and that a group-writable context file + still produces `644` in the image. -Measured across three images built on the same host from the same commit range: - -| Image built from | Mode of `/etc/logrotate.d/mail` | -|---|---| -| a sync made two days earlier | `0644` — works | -| a later sync (`tar -czf -` pipe from a Windows checkout) | `0666` | -| a later sync (`git archive` from the same checkout) | `0664` | - -So the file is fine in the repository (git records `100644`) and is spoiled on -the way into the build context. `COPY build/logrotate-mail.conf -/etc/logrotate.d/mail` ([build/Dockerfile](../../build/Dockerfile)) takes the -mode from the context as it finds it, and an archive produced from a checkout -without POSIX permissions carries the umask-widened mode instead of the one git -recorded. - -**Not established:** whether images built by the release workflow are affected. -They are built from a checkout on Linux, where the mode should survive as -`0644`, but the published image could not be pulled to check. Confirm before -concluding that only locally built images have this. - -## Why it deserves a plan rather than a one-line fix - -Three separate things are wrong, and fixing only the visible one leaves the -other two. - -1. **The image trusts the build context's file modes.** Every `COPY` in the - Dockerfile has this property, not just this one; the scripts happen to be - `chmod +x`-ed afterwards, which is why they were never noticed. -2. **The failure is silent.** `logrotate-loop.sh` runs - `logrotate /etc/logrotate.d/mail` and only reports a failure on a non-zero - exit — but logrotate *ignores* the file and exits 0, so the loop reports - nothing and the operator's only clue is a warning printed once at start. -3. **Nothing checks the outcome.** No test or health check notices that - `mail.log` has not rotated, and the panel's Status page has no view of it. - -## Directions to weigh - -- `COPY --chmod=0644` on the configuration files (and an explicit mode on the - scripts instead of the later `chmod +x`), which makes the image's file modes a - property of the Dockerfile rather than of whoever built it. Needs a check of - the minimum BuildKit version the project is willing to require. -- Or an explicit `chmod` in the same `RUN` that already fixes the scripts — - cruder, no build-time requirement. -- Make `logrotate-loop.sh` fail loudly: `logrotate` has `--debug`-free ways to - be told to care, but the simplest reliable check is that the loop verifies - the configuration is readable-and-not-writable before entering the loop, and - exits non-zero so supervisord reports it. -- Consider whether the e2e stack should assert that a rotation actually happens - (it can run with a short `LOGROTATE_INTERVAL_SECONDS`). - -## Done when - -- An image built from a Windows checkout and one built by the release workflow - both carry `0644`, and rotation runs in both. -- A configuration logrotate would ignore makes the container say so in a way an - operator will see, rather than exiting 0. -- The dev loop's sync step cannot silently widen file modes again, or the image - no longer cares if it does. - -## Risks - -- Low blast radius, but it touches the image's startup path — a mistake here is - a container that will not start rather than a log that does not rotate. -- The `create 0640 postfix selfpost` line in the rotate configuration is load - bearing (see the comment in `logrotate-loop.sh`: a postlogd-triggered recreate - lands the file unreadable by the unprivileged panel). Any rework of the - configuration must keep it. +`create 0640 postfix selfpost` in [`build/logrotate-mail.conf`](../../build/logrotate-mail.conf) +is unchanged — required for panel readability after rotation. diff --git a/docs/roadmap.md b/docs/roadmap.md index 211cd12..3420de8 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -29,7 +29,6 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | inbound-relay | Inbound relay (backup-MX / forwarding) | **agreed** | [plans/inbound-relay.md](plans/inbound-relay.md) | | contributing | `CONTRIBUTING.md` | candidate | — | | dmarc-reports | DMARC aggregate report ingestion and panel UI | candidate | [plans/dmarc-reports.md](plans/dmarc-reports.md) | -| logrotate-mode | `mail.log` stops rotating in some builds | candidate | [plans/logrotate-mode.md](plans/logrotate-mode.md) | | panel-docs | In-panel operator documentation | candidate | — | **Recommended order** (not binding): **inbound-relay** first among agreed diff --git a/test/e2e/logrotate_check.go b/test/e2e/logrotate_check.go new file mode 100644 index 0000000..98fde8c --- /dev/null +++ b/test/e2e/logrotate_check.go @@ -0,0 +1,97 @@ +package e2e + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// checkLogrotateConfigMode verifies the image pins /etc/logrotate.d/mail at 0644 +// so logrotate will not silently ignore it (docs/plans/logrotate-mode.md). +func checkLogrotateConfigMode(s *stack) error { + mode, err := s.execIn("selfpost", "stat", "-c", "%a", "/etc/logrotate.d/mail") + if err != nil { + return fmt.Errorf("stat logrotate config: %w", err) + } + mode = strings.TrimSpace(mode) + if mode != "644" { + return fmt.Errorf("/etc/logrotate.d/mail mode is %q, want 644", mode) + } + return nil +} + +// checkLogrotateRotation forces a rotation and checks the recreated mail.log is +// panel-readable (0640 postfix:selfpost per build/logrotate-mail.conf). +func checkLogrotateRotation(s *stack) error { + const logPath = "/data/log/mail.log" + marker := "e2e-logrotate-marker\n" + if _, err := s.execIn("selfpost", "sh", "-c", + fmt.Sprintf("printf %q >> %s", marker, logPath)); err != nil { + return fmt.Errorf("write mail.log: %w", err) + } + if _, err := s.execIn("selfpost", "logrotate", "-f", "/etc/logrotate.d/mail"); err != nil { + return fmt.Errorf("logrotate -f: %w", err) + } + rotated, err := s.execIn("selfpost", "sh", "-c", "test -f /data/log/mail.log.1 && echo yes") + if err != nil || strings.TrimSpace(rotated) != "yes" { + return fmt.Errorf("expected /data/log/mail.log.1 after forced rotation (out=%q err=%v)", rotated, err) + } + mode, err := s.execIn("selfpost", "stat", "-c", "%a", logPath) + if err != nil { + return fmt.Errorf("stat rotated mail.log: %w", err) + } + if strings.TrimSpace(mode) != "640" { + return fmt.Errorf("new %s mode is %q, want 640", logPath, strings.TrimSpace(mode)) + } + body, err := s.execIn("selfpost", "grep", "-F", strings.TrimSpace(marker), "/data/log/mail.log.1") + if err != nil || !strings.Contains(body, strings.TrimSpace(marker)) { + return fmt.Errorf("rotated file does not contain marker (out=%q err=%v)", body, err) + } + return nil +} + +// TestImageBuildPreservesLogrotateMode builds from a context where +// logrotate-mail.conf is group-writable and checks the image still ships 0644. +func TestImageBuildPreservesLogrotateMode(t *testing.T) { + conf := filepath.Join(h.repoRoot, "build", "logrotate-mail.conf") + info, err := os.Stat(conf) + if err != nil { + t.Fatalf("stat source config: %v", err) + } + origMode := info.Mode().Perm() + + if err := os.Chmod(conf, origMode|0o020); err != nil { + t.Fatalf("chmod g+w source config: %v", err) + } + t.Cleanup(func() { + _ = os.Chmod(conf, origMode) + }) + + tag := "selfpost:e2e-logrotate-mode" + build := exec.Command("docker", "build", + "-f", filepath.Join(h.repoRoot, "build", "Dockerfile"), + "-t", tag, + "--build-arg", "VERSION=e2e", + h.repoRoot, + ) + build.Env = os.Environ() + if out, err := build.CombinedOutput(); err != nil { + t.Fatalf("docker build with group-writable context config: %v\n%s", err, out) + } + t.Cleanup(func() { + _, _ = exec.Command("docker", "rmi", "-f", tag).CombinedOutput() + }) + + run := exec.Command("docker", "run", "--rm", tag, "stat", "-c", "%a", "/etc/logrotate.d/mail") + run.Env = os.Environ() + out, err := run.CombinedOutput() + if err != nil { + t.Fatalf("stat in image: %v\n%s", err, out) + } + if strings.TrimSpace(string(out)) != "644" { + t.Fatalf("image logrotate config mode is %q, want 644", strings.TrimSpace(string(out))) + } +} diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index 4135598..8a0deb5 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -105,6 +105,12 @@ func TestE2E(t *testing.T) { if err := checkSupervisorProcesses(h); err != nil { t.Fatal(err) } + if err := checkLogrotateConfigMode(h); err != nil { + t.Fatal(err) + } + if err := checkLogrotateRotation(h); err != nil { + t.Fatal(err) + } if err := waitForPanelReady(); err != nil { t.Fatal(err) }