@casperklein @polarathene should we cram this into v15.0.0?

If we merge this, this can be in v15 👍

I wonder, what the initial motivation for syslog:root was:

It's world writable or writable by group which is not "root")

root@mail:/# ls -ahld /var/log/mail
drwxr-xr-x 2 syslog root 4.0K Feb  5 04:49 /var/log/mail

It's neither world writeable nor writable by group which is not "root" 🤔

Please verify that we do not accidentally include a potential regression with this change right before the v15.0.0 release. I do not know enough about the permissions at the moment, hence I am not sure whether the actual permissions need a change or whether this PR's script change is accurate.

TL;DR:

• This verbose comment is for my benefit for technical details / historical context of how we ended up here 😅
• Skip to the next comment for actionable feedback.

I wonder, what the initial motivation for syslog:root was

Unclear, it was committed to the Dockerfile in April 2016, the only context is a reference to the DMS v2 tracking issue but nothing in that issue thread or linked commit provides any context.

EDIT: Assumption is rsyslog runs as the syslog user (unexpected if we explicitly need to create a syslog user for it to function though), which if true answers the ownership requirement for /var/log/mail which is our own centralized log dir for DMS (prior to introducing supervisor, but both are required atm AFAIK). root group ownership is likely to infer that nothing else needs access via group permissions.

Additionally when deploying a container as rootless there is a common pattern to only change the runtime user while keeping a root group (docker run --user 1000:0), but that isn't too relevant here, definitely wasn't when this was implemented.

You could also use SGID bit IIRC to have any service with write permission to a directory to enforce files created are assigned the common parent directory group.

It's neither world writeable nor writable by group which is not "root" 🤔

That'd depend on the context. /var/log/mail is a location DMS has volume mounted, which like /var/mail means a bind mount replaces all ownership/permissions assigned at the image (hence why these runtime fixes came about, and why this type of thing belongs in mail-state.sh (setup-stack.sh, see below) as we'd skip these kind of fixes during container restarts otherwise).

For /var/mail this removes the SGID bit that Debian otherwise has on the folder, I don't think we restore that since I identified it (I have an open issue with all the details regarding this concern and related issues to resolve).

As the linked bug report shows, their bind mount volume has modified permissions to 777. That or it's a k8s / helm related config difference for how the volume mount is handled. Perhaps that's why the bug report was raised at the helm repo and not here?

Related history / context

We do have this other related change in the Dockerfile:

That was introduced in Jan 2018 during the base image swap from Ubuntu 16.04 to Debian 9 Stretch (PR):

rsyslog does not add syslog user and has different conf-structure

While this runtime snippet:

https://github.com/docker-mailserver/docker-mailserver/blame/1a938dfb15e94f43f1af5ddaf7b8276aba92a825/target/scripts/startup/setup.d/log.sh#L3-L11

Originates from Oct 2019 (associated issue + PR). Basically belongs in mail-state.sh AFAIK (setup-stack.sh, see below).

The Oct 2019 issue was specifically about /var/log/mail/clamav.log failing, and expecting clamav:clamav permissions, although the PR/commit is clamav:adm for the clamav.log and freshclam.log files, along with syslog:root for /var/log/mail.

Not sure if we're actually leveraging the supervisor ClamAV logs for anything:

The two separate ClamAV logs are configured only via the Dockerfile at build (?), probably should be extracted out to a script 🤷‍♂️ (even if that's only run during build-time)

Even with syslog, we have rsyslog run via supervisord. I haven't looked for config for this, presumably something has it run as the syslog user to enforce that requirement, root ownership is likely a side-effect of just not implying a need access by anything else via group:

Probably should address for DMS v15

DMS v12 release split the runtime fix for /var/log/mail + separate ClamAV log files, so now we have as of DMS v15 (unless changed before release),

Until we have a better location, the current location for such fixes is grouped into this method:

DMS v15 correctly runs that even for container restarts so we could move these changes into that 👍

Will eventually improve

I've covered extensive info on our current logging situation in the past (see the "What are you going to contribute?" section)

• April 2016 (DMS v2) centralized services logs to live at /var/log/mail to pair with a volume mount for persistence.
• August 2017 introduced Supervisord, and over time more services were properly configured to manage logs through stdout to collect service logs at a different location, while remaining syslog output is collected in our central mail log that the container outputs to stdout (for Docker's own log drivers to leverage), and our tests rely on this mail log file.

That'll get tackled once I've found time to return to the Vector syslog PR. With Vector we can better manage ingesting log files, syslog, etc and output to centralized logs for individual services or other formats for structured logs, etc.

UPDATE:

• Advice is to apply the interim fix this comment suggests.
• Pretty sure I grok the situation and history fully (well 99%), as detailed in my next comment.

should we cram this into v15.0.0?

I'm not sure if it makes sense to merge this PR, like @casperklein pointed out it doesn't look like this would be reproduced unless the user has modified permissions for their volume bind mount.

I don't know how likely that should be needed to support or expect. If this concern doesn't occur with our defaults, then we could just make the changes I mention in "Probably should address for DMS v15" section in my previous comment, which can add another line to reset permissions for /var/log/mail/ to the expected 755 (rwx rx rx)?

docker-mailserver/docker-mailserver-helm#137 (comment) mentions the helm / k8s config (sorry not too familiar with that) is a limitation for managing this, I'm not sure how compatible the fix I propose would be with the helm project, if someone can confirm it's compatible that'd be great 👍

EDIT: Assumption is rsyslog runs as the syslog user

rsyslogd runs as root:

root@mail:/# ps aux|grep rsyslogd
root      2332  0.0  0.0 152120  2936 ?        Sl   Feb16   0:01 /usr/sbin/rsyslogd -n

TL;DR: Ok I think I got the gist of it now 💪

• syslog user and related chown appears redundant, should be safe to remove 👍
• ClamAV related commands also should be fine to remove, given the fix at image build to ensure a fixed UID/GID (avoiding mapping mismatch when order of packages installed changes the incremental ID that'd otherwise have been assigned).

Am I confident in avoiding a breaking change? No. Technically we should also ensure ownership is changed away from syslog once removing that too, not that it should matter in practice. I'd need more time to go over everything I've documented here and verify, so perhaps best to delay that.

Earlier suggestion as an interim fix is still valid for DMS v15.

Summary:

• syslog ownership appears to be inherited from the Ubuntu to Debian base image switch.
  • This seems redundant and not actually required anymore. It's inclusion at runtime was with resolving the ClamAV ownership errors and not questioned/explained during review.
  • Base image upgrade from Ubuntu to Debian adding the user syslog as a fix would only have been to support the later chown command from DMS v2, not questioned during review.
  • DMS v2 would have likely added the chown for syslog for similar reason as explained below, a compatibility fix for volume mounts where the friendly user/group names map to different UID/GID between distro. Ubuntu potentially had rsyslog configured to use the syslog user, unlike with Debian.
• Ownership fixes were for ClamAV from what I can tell shouldn't have been needed
  • Likely occurred due to volume mount and an upgrade as this would have been before I introduced a fixed UID/GID for clamav, hence a mismatch during image upgrade.
  • ClamAV configs have the user clamav configured, and only a clamav group for the ClamAV socket. Thus correcting the owner of the log files at container startup was a fix that should no longer be necessary.

rsyslogd runs as root

It may still be configured somewhere to write as syslog?

Initial /var/log/mail/mail.log setup (Dockerfile adjusts rsyslog config to write syslog mail filtered logs to this file, our test suite checks this too):

Symlink created to /var/log/mail.log:

rsyslog package

Installing the package in debian:12-slim image creates /etc/rsyslog.conf with these config snippets:

###########################
#### GLOBAL DIRECTIVES ####
###########################

#
# Set the default permissions for all log files.
#
$FileOwner root
$FileGroup adm
$FileCreateMode 0640
$DirCreateMode 0755
$Umask 0022

#
# Where to place spool and state files
#
$WorkDirectory /var/spool/rsyslog

#
# Include all config files in /etc/rsyslog.d/
#
$IncludeConfig /etc/rsyslog.d/*.conf

###############
#### RULES ####
###############

#
# Log anything besides private authentication messages to a single log file
#
*.*;auth,authpriv.none          -/var/log/syslog

#
# Log commonly used facilities to their own log file
#
auth,authpriv.*                 /var/log/auth.log
cron.*                          -/var/log/cron.log
kern.*                          -/var/log/kern.log
mail.*                          -/var/log/mail.log
user.*                          -/var/log/user.log

#
# Emergencies are sent to everybody logged in.
#
*.emerg                         :omusrmsg:*

#################
#### MODULES ####
#################

module(load="imuxsock") # provides support for local system logging
module(load="imklog")   # provides kernel logging support
#module(load="immark")  # provides --MARK-- message capability

# provides UDP syslog reception
#module(load="imudp")
#input(type="imudp" port="514")

# provides TCP syslog reception
#module(load="imtcp")
#input(type="imtcp" port="514")

DMS Dockerfile changes to that config:

Observations

• mail-state.sh doesn't have any handling for persisting rsyslog state ($WorkDirectory /var/spool/rsyslog)
• We have rsyslog run as root via supervisord service, and the original rsyslog config /etc/rsyslog.conf shown above runs the process as root:adm with permissions for files/dirs configured fine, while /etc/rsyslog.d/ is empty.
• We modify rsyslog config to:
  • route syslog entries received marked with the mail facility from /var/log/mail.log (DMS makes this a symlink instead) to /var/log/mail/mail.log
  • Alter the filter for /var/log/syslog to prepend mail.none;mail.error (I forget this syntax but I've documented it somewhere in our issues 😅)
  • Disable the kernel logging since we're running in a container, not a host/system syslog process where that'd be relevant.

I have applied the interim fix I suggested: #4374

I do not know if that'll resolve the issue with k8s / helm but I don't think the proposed solution from this PR is the right fix? (especially since we'd be going back to root ownership in future)