New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Ensure state persisted to /var/mail-state
retains correct group
#3011
fix: Ensure state persisted to /var/mail-state
retains correct group
#3011
Conversation
Update: We can perform a CI failure:
Full build log
Related workflow logs for bug reporting: Run docker/build-push-action@v3.3.0
Docker info
Buildx version
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's never nice to use a workaround, good job at implementing it! LGTM 👍🏼
Worked around the issue by treating it a separate stage to avoid the double file size That might affect the image cache still however. I have noticed in the CI build workflow that we've gone from 345MB cache to 786MB, but I'm not entirely sure if that's a new min size 😅 The image build itself seems to remain at the The review feedback to align paths in Probably because the separate ClamAV stage is like 460MB, and then the usual 230MB layer in the later stage it's |
0c7ba58
Below sections is just reference / info, not too important for review :) I looked into this a bit more and found we can avoid the extra 230-460MB increase to build cache by eagerly adding the I've matched how the package creates the user (source linked below), and with an explicit UID + GID, we can now use Now build cache remains at 345MB, keeping rate of eviction from CI cache storage low 👍 Users + Groups we addWhile working on this PR, I looked into the users we explicitly create or are added from packages installed. For the majority I've looked up the source for how they were created and documented that all below. Might be useful reference for maintenance someday? 😅 We could technically create those users upfront ourselves.There isn't much benefit doing that except for when I experimented with splitting different package installs into separate stages and using a naive That used a fair amount of additional space as One benefit is a central place of where each user is setup, although there's a small chance of maintenance drift from upstream at risk. And should we change these due to add/removing packages, where the UID/GID values shift anyway, then it's not preventing the main problem 😅 (hence these Setup users script (with package source references): #!/bin/bash
# -eE :: exit on error (do this in functions as well)
# -u :: show (and exit) when using unset variables
# -o pipefail :: exit on error in pipes
set -eE -u -o pipefail
function _setup_users() {
addgroup --quiet --system --force-badname ssl-cert
addgroup --quiet --system crontab
addgroup --quiet --system postdrop
# `--disabled-password` and `--disabled-login` aren't necessary, results in roughly the same /etc/shadow output
# `--disabled-password` allows for login that is not password-based. It appears to be default for `--system`, retained for clarity.
local COMMON=(
--quiet
--system
--disabled-password
)
# clamav + fetchmail: Should both prefer `nologin` to `/bin/false`
# for their shell as it is effectively equivalent.
# fetchmail installs with a home group of nobody, so we add that user without `--group`
adduser "${COMMON[@]}" --home '/var/lib/fetchmail' fetchmail
COMMON+=('--group')
adduser "${COMMON[@]}" --home '/run/opendkim' opendkim
adduser "${COMMON[@]}" --home '/run/opendmarc' opendmarc
adduser "${COMMON[@]}" --home '/var/lib/redis' redis
# Amavis needs a shell for Dockerfile `razor-admin` lines:
adduser "${COMMON[@]}" \
--home '/var/lib/amavis' \
--gecos 'AMaViS system user' \
--shell /bin/sh \
amavis
COMMON+=('--no-create-home')
adduser "${COMMON[@]}" --home '/nonexistent' policyd-spf
adduser "${COMMON[@]}" --home '/var/spool/postfix' postfix
adduser "${COMMON[@]}" --home '/var/lib/postgrey' postgrey
adduser "${COMMON[@]}" --home '/var/lib/postsrsd' postsrsd
adduser "${COMMON[@]}" --home '/var/lib/spamassassin' debian-spamd
adduser "${COMMON[@]}" \
--home '/usr/lib/dovecot' \
--gecos 'Dovecot mail server' \
dovecot
adduser "${COMMON[@]}" \
--home '/nonexistent' \
--gecos 'Dovecot login user' \
dovenull
adduser "${COMMON[@]}" \
--home '/var/lib/rspamd' \
--gecos 'rspamd spam filtering system' \
--force-badname \
_rspamd
# Explicit UID set (--group will use it also for GID), for use with `COPY --chown`
adduser "${COMMON[@]}" --home '/var/lib/clamav' --uid 200 clamav
# Add clamav and amavis users to each others group
adduser clamav amavis
adduser amavis clamav
# rsyslog package in Debian (unlike in Ubuntu) does not add the syslog user
adduser "${COMMON[@]}" --home '/nonexistent' syslog
# DMS user (used by Dovecot?):
# NOTE: `adduser` isn't suitable for non-interactive creation of users with passwords.
# NOTE: This requires openssl to already be installed. A group is implicitly created matching user name and uid values.
useradd --uid 5000 --home-dir '/home/docker' --shell '/bin/bash' --password "$(echo docker | openssl passwd -1 -stdin)" docker
}
_setup_users
### Additional References ###
# Docs for adduser + useradd:
# https://manpages.debian.org/bullseye/adduser/adduser.8.en.html
# https://manpages.debian.org/bullseye/passwd/useradd.8.en.html
# Default additions to /etc/passwd without this script:
#
# postfix:x:101:102::/var/spool/postfix:/usr/sbin/nologin
# fetchmail:x:102:65534::/var/lib/fetchmail:/bin/false
# postsrsd:x:103:104::/var/lib/postsrsd:/usr/sbin/nologin
# opendkim:x:104:106::/run/opendkim:/usr/sbin/nologin
# opendmarc:x:105:107::/run/opendmarc:/usr/sbin/nologin
# clamav:x:106:108::/var/lib/clamav:/bin/false
# postgrey:x:107:109::/var/lib/postgrey:/usr/sbin/nologin
# policyd-spf:x:108:110::/nonexistent:/usr/sbin/nologin
# debian-spamd:x:109:111::/var/lib/spamassassin:/usr/sbin/nologin
# amavis:x:110:112:AMaViS system user,,,:/var/lib/amavis:/bin/sh
# dovecot:x:111:113:Dovecot mail server,,,:/usr/lib/dovecot:/usr/sbin/nologin
# dovenull:x:112:114:Dovecot login user,,,:/nonexistent:/usr/sbin/nologin
# _rspamd:x:113:115:rspamd spam filtering system,,,:/var/lib/rspamd:/usr/sbin/nologin
# redis:x:114:116::/var/lib/redis:/usr/sbin/nologin
# syslog:x:115:65534::/home/syslog:/usr/sbin/nologin
# docker:x:5000:5000::/home/docker:/bin/bash
# Default additions to /etc/group without this script:
#
# ssl-cert:x:101:
# postfix:x:102:
# postdrop:x:103:
# postsrsd:x:104:
# crontab:x:105:
# opendkim:x:106:
# opendmarc:x:107:
# clamav:x:108:amavis
# postgrey:x:109:
# policyd-spf:x:110:
# debian-spamd:x:111:
# amavis:x:112:clamav
# dovecot:x:113:
# dovenull:x:114:
# _rspamd:x:115:
# redis:x:116:
# docker:x:5000:
# Default additions to /etc/shadow without this script:
# '*' is no password, cannot login as these system users.
# --disabled-login causes the locked '!' value for clamav,spamassassin,rspamd.
#
# postfix:*:19377:0:99999:7:::
# fetchmail:*:19377:0:99999:7:::
# postsrsd:*:19377:0:99999:7:::
# opendkim:*:19377:0:99999:7:::
# opendmarc:*:19377:0:99999:7:::
# clamav:!:19377:0:99999:7:::
# postgrey:*:19377:0:99999:7:::
# policyd-spf:*:19377:0:99999:7:::
# debian-spamd:!:19377:0:99999:7:::
# amavis:*:19377:0:99999:7:::
# dovecot:*:19377:0:99999:7:::
# dovenull:*:19377:0:99999:7:::
# _rspamd:!:19377:0:99999:7:::
# redis:*:19377:0:99999:7:::
# syslog:*:19377:0:99999:7:::
# docker:$1$/ZRgRKw5$kXOPJlnHGZLVzNV8Tfcav/:19377:0:99999:7:::
### Package sources for users and groups created above ###
# amavis (adduser --system --ingroup amavis --home /var/lib/amavis --gecos "AMaViS system user" --shell /bin/sh --quiet --disabled-password amavis)
# https://packages.debian.org/source/bullseye/amavisd-new
# https://salsa.debian.org/debian/amavisd-new/-/blob/dda3c9ef0ec85ac8bf4a3f26caa82e96fbb6d381/debian/amavisd-new.postinst#L53-55
# clamav (adduser --system --no-create-home --quiet --disabled-password --disabled-login --shell /bin/false --group --home /var/lib/clamav clamav)
# https://packages.debian.org/source/bullseye/clamav
# https://salsa.debian.org/clamav-team/clamav/-/blob/4a25d28c0379c4b034d60c686f63f3d518d85286/debian/clamav-base.postinst.in#L35-38
# clamav installs cron package, adds crontab group
# addgroup --system crontab
# https://packages.debian.org/source/bullseye/cron
# https://salsa.debian.org/debian/cron/-/blob/bf78c756347c4533b1eba1ee24aadf6952b03531/debian/cron-daemon-common.postinst#L13
# user: postfix, groups: postfix postdrop
# addgroup --system postdrop
# addgroup --system postfix
# adduser --system --home /var/spool/postfix --no-create-home --disabled-password --ingroup postfix postfix
# https://packages.debian.org/source/bullseye/postfix
# https://salsa.debian.org/postfix-team/postfix-dev/-/blob/ddcd3d9659338f44de47c2e8ece866ea0ade85c5/debian/postfix.postinst#L210-213
# https://salsa.debian.org/postfix-team/postfix-dev/-/blob/ddcd3d9659338f44de47c2e8ece866ea0ade85c5/debian/postfix.postinst#L21-22
# ssl-cert package installed as part of postfix package
# addgroup --quiet --system --force-badname ssl-cert
# https://packages.debian.org/source/bullseye/ssl-cert
# https://salsa.debian.org/apache-team/ssl-cert/-/blob/6d7133f393d044297418a34ca0738276005e55de/debian/postinst#L9
# opendkim (adduser --quiet --system --group --home /run/opendkim opendkim)
# https://packages.debian.org/source/bullseye/opendkim
# https://salsa.debian.org/debian/opendkim/-/blob/3a900f7373dce378c425ca832dea7bfa3e230d3d/debian/opendkim.postinst#L24
# opendmarc (adduser --quiet --system --group --home /run/opendmarc opendmarc)
# https://packages.debian.org/source/bullseye/opendmarc
# https://salsa.debian.org/kitterman/opendmarc/-/blob/b2ba99c5918d0b50048239090f61d985fe970e30/debian/opendmarc.postinst#L28
# postsrsd (adduser --quiet --system --group --no-create-home --home /var/lib/postsrsd postsrsd)
# https://packages.debian.org/source/bullseye/postsrsd
# https://salsa.debian.org/debian/postsrsd/-/blob/9d0c2fbc1b5df858d0fd27763eb4cc2f6f0d9352/debian/postsrsd.postinst#L34
# postgrey
# Git source presently in transition to salsa.debian.org, lacking debian post-install script for postgrey user?
# Installation output does indicate `--no-create-home` used, but prior `/var/lib/postgrey` is created or `chown postgrey:postgrey`.
# https://packages.debian.org/source/bullseye/postgrey
# https://salsa.debian.org/debian/postgrey
# fetchmail (Missing Debian package source)
# https://packages.debian.org/source/bullseye/fetchmail
# policyd-spf (adduser --quiet --system --group --no-create-home --home /nonexistent policyd-spf)
# https://packages.debian.org/source/bullseye/postfix-policyd-spf-perl
# https://salsa.debian.org/kitterman/postfix-policyd-spf-perl/-/blob/38b5b4d4b83f564651261f830e895ee2769ce62e/debian/postfix-policyd-spf-perl.postinst#L7-10
# debian-spamd (adduser --system --group --disabled-login --disabled-password --home /var/lib/spamassassin --no-create-home debian-spamd)
# https://packages.debian.org/source/bullseye/spamassassin
# https://salsa.debian.org/debian/spamassassin/-/blob/ead27ee27f508f0b91f408e688b6446b15cd3018/debian/spamassassin.postinst#L8-10
# dovecot (adduser --system --group --home /usr/lib/dovecot --gecos "Dovecot mail server" --no-create-home --disabled-password --quiet dovecot || true)
# dovenull (adduser --system --group --home /nonexistent --no-create-home --gecos "Dovecot login user" --disabled-password --quiet dovenull || true)
# https://packages.debian.org/source/bullseye/dovecot
# https://salsa.debian.org/debian/dovecot/-/blob/5092256610ddd679e366402abbe7854058f8a78b/debian/dovecot-core.postinst#L35-39
# _rspamd (adduser --quiet --system --group --home /var/lib/rspamd --no-create-home --disabled-login --gecos "rspamd spam filtering system" --force-badname _rspamd)
# https://packages.debian.org/source/bullseye/rspamd
# https://salsa.debian.org/debian/rspamd/-/blob/895f9465cc3559f6f50ba7093d61b57173192f46/debian/postinst#L37-45
# redis (adduser --system --home /var/lib/redis --quiet --group redis || true)
# https://packages.debian.org/source/bullseye/redis
# https://salsa.debian.org/lamby/pkg-redis/-/blob/9f2df070e3a23e5eae432f8e44d3925f1d291830/debian/redis-tools.postinst#L23-28 Package install layer weightI was curious and observed the following breakdown ( Image layer weight breakdown:
And then we add the 230MB ClamAV files to
|
1e74581
1e74581
to
d89cf4a
Compare
- `/var/lib/postfix` dir and content is `postfix:postfix`, not `postfix:root`. - `/var/spool/postfix` is `root:root` not `postfix:root` like it's content. - Add additional comments, including ownership changes by Postfix to `/var/spool/postfix` when process starts / restarts.
These were all fine except for clamav not using the correct clamav group. `fetchmail` group is `nogroup` as per the group set by the debian package. Additionally formatted the `-eq 1 ]]` content to align on the same columns, and added additional comment about the purpose of this `chown -R` usage so that it's clear what bug / breakage it's attempting to prevent / fix.
The last condition doesn't get triggered at all AFAIK. Nor does it make sense to make a folder path with `mkdir -p` to symlink to when the container does not have anything to copy over? - If that was for files, the `mkdir -p` approach seems invalid? - If it was for a directory that could come up later, it should instead be created in advance? None of the current values for `FILES` seem to hit this path. Removing as it doesn't seem relevant to current support. Symlinking was done for each case, I've opted to just perform that after the conditional instead. Additional inline docs added for additional context.
This was handled separately for some reason. It belongs with the other services handling this fix in `misc-stack.sh`. The `-h` option isn't relevant, when paired with `-R` it has no effect.
The UID and GID were copied over but would not match `clamav` user and group due to numeric ID mismatch between containers. `--chown=clamav` fixes that.
Avoids increasing the image weight from this change by leveraging `COPY` in the final stage.
The `scratch` approach wasn't great. A single layer invalidation in the previous stage would result in a new 600MB layer to store. `make build` with this change seems to barely be affected by such if a change came before copying over the linked stage, although with `buildx` and the `docker-container` driver with `--load` it would take much longer to import and seemed to keep adding storage. Possibly because I was testing with a minimal `buildx` command, that wasn't leveraging proper cache options?
Review feedback Co-authored-by: Casper <casperklein@users.noreply.github.com>
No apparent advantage of a `COPY --link` initially in separate stage. Just `COPY --chown` in the separate stage and `COPY --link` the stage content. 230MB less in build cache used.
Creating the user before the package is installed allows to ensure a fixed numeric ID that we can provide to `--chown` that is compatible with `--link`. This keeps the build cache minimal for CI, without being anymore complex as a workaround than the separate stage was for the most part.
5d0b4b8
322dad8
to
5d0b4b8
Compare
Err ok that's a first for me. I thought a rebase on master button kept the approval like it does when you update with a merge commit from master 🤔 Won't be doing that again then, sorry about that 😅 |
@georglauterbach are you alright with this PR being merged? Or do you want to wait until your rspamd PR is merged first to avoid dealing with the likely merge conflict on |
Go ahead 🚀 |
Description
This PR covers changes mostly related to
misc-stack.sh
:chown -R
is more explicit about not only setting the user, but also the correct group. Postfix also needed a little adjustment there.misc-stack.sh
rspamd ENV condition to add toFILES
array when enabled, not disabled. This typo was missed during initial review.Likewise we've had a similar issue with our
Dockerfile
copying over ClamAV database files from their official image. This prompted reviewingmisc-stack.sh
which should fix the issue for the user.Fixes #2942
References
misc-stack.sh
, particularly for Postfix. It may be useful reference for any future maintainer stumbling on these kind of changes.chown -R
usage inmisc-stack.sh
(and making it more clear about the purpose via comment).COPY --link
was introduced into theDockerfile
in thev11.2
release in this Sep 2022 PR.chown -R
fixes for other users/groupsmisc-stack.sh
appear to have originated in this May 2017 PR (a fix for this March 2017 issue), presumably based off the earlier Feb 2017 PR for Amavis.Type of change
Checklist: