Skip to content

chore: Remove the wrapper script for Postfix (and disable chroot in master.cf)#3033

Merged
georglauterbach merged 4 commits intodocker-mailserver:masterfrom
polarathene:chore/remove-postfix-wrapper
Jan 29, 2023
Merged

chore: Remove the wrapper script for Postfix (and disable chroot in master.cf)#3033
georglauterbach merged 4 commits intodocker-mailserver:masterfrom
polarathene:chore/remove-postfix-wrapper

Conversation

@polarathene
Copy link
Copy Markdown
Member

@polarathene polarathene commented Jan 27, 2023 •
edited
Loading

Description

  • Motivation is to drop the wrapper script mainly due to the sleep usage.
  • Running Postfix in foreground for supervisord seems to be possible now, provided we drop the chroot jail config in master.cf.
  • Added benefit of not being concerned with chroot copy of system files (notably DNS config changes) falling out of sync (requires restarting Postfix process). A concern that may be more likely problematic as our changedetector service in the next release switches to postfix reload instead of having the process killed to restart it fully.

This change seems to be fine from what I can assess :)

Mailu master.cf and mailcow master.cf both are configured to not use chroot.

DMS history for current config

A discussion between maintainers in June 2022 referenced the following:

Please note that if Postfix is launched in chroot environment (which is default in many distros) with postfix start-fg, you will have the /var/spool/postfix/etc dir empty, postfix will not know your localtime, hosts, resolv.conf, you will not be able to deliver a single message.

However, while inside a Docker container, I would advise not to use chroot (it would be redundant).

Additional info regarding chroot and Postfix / Docker

Differences between master.cf files upstream

Current master branch of Postfix master.cf is the same as what current Alpine ships with Postfix 3.7.4, with the exception of the local_header_rewrite_clients=static:all lines.

Debian Bullseye differs in restrictions options for Port 587 + 465 services (submission(s)) and stripping comments, in addition to explicitly configuring y for the chroot column (and setting up chroot jail somewhere for Postfix).

Debian Bullseye default Postfix master.cf 
#
# Postfix master process configuration file.  For details on the format
# of the file, see the master(5) manual page (command: "man 5 master" or
# on-line: http://www.postfix.org/master.5.html).
#
# Do not forget to execute "postfix reload" after editing this file.
#
# ==========================================================================
# service type  private unpriv  chroot  wakeup  maxproc command + args
#               (yes)   (yes)   (no)    (never) (100)
# ==========================================================================
smtp      inet  n       -       y       -       -       smtpd
#smtp      inet  n       -       y       -       1       postscreen
#smtpd     pass  -       -       y       -       -       smtpd
#dnsblog   unix  -       -       y       -       0       dnsblog
#tlsproxy  unix  -       -       y       -       0       tlsproxy
#submission inet n       -       y       -       -       smtpd
#  -o syslog_name=postfix/submission
#  -o smtpd_tls_security_level=encrypt
#  -o smtpd_sasl_auth_enable=yes
#  -o smtpd_tls_auth_only=yes
#  -o smtpd_reject_unlisted_recipient=no
#  -o smtpd_client_restrictions=$mua_client_restrictions
#  -o smtpd_helo_restrictions=$mua_helo_restrictions
#  -o smtpd_sender_restrictions=$mua_sender_restrictions
#  -o smtpd_recipient_restrictions=
#  -o smtpd_relay_restrictions=permit_sasl_authenticated,reject
#  -o milter_macro_daemon_name=ORIGINATING
#smtps     inet  n       -       y       -       -       smtpd
#  -o syslog_name=postfix/smtps
#  -o smtpd_tls_wrappermode=yes
#  -o smtpd_sasl_auth_enable=yes
#  -o smtpd_reject_unlisted_recipient=no
#  -o smtpd_client_restrictions=$mua_client_restrictions
#  -o smtpd_helo_restrictions=$mua_helo_restrictions
#  -o smtpd_sender_restrictions=$mua_sender_restrictions
#  -o smtpd_recipient_restrictions=
#  -o smtpd_relay_restrictions=permit_sasl_authenticated,reject
#  -o milter_macro_daemon_name=ORIGINATING
#628       inet  n       -       y       -       -       qmqpd
pickup    unix  n       -       y       60      1       pickup
cleanup   unix  n       -       y       -       0       cleanup
qmgr      unix  n       -       n       300     1       qmgr
#qmgr     unix  n       -       n       300     1       oqmgr
tlsmgr    unix  -       -       y       1000?   1       tlsmgr
rewrite   unix  -       -       y       -       -       trivial-rewrite
bounce    unix  -       -       y       -       0       bounce
defer     unix  -       -       y       -       0       bounce
trace     unix  -       -       y       -       0       bounce
verify    unix  -       -       y       -       1       verify
flush     unix  n       -       y       1000?   0       flush
proxymap  unix  -       -       n       -       -       proxymap
proxywrite unix -       -       n       -       1       proxymap
smtp      unix  -       -       y       -       -       smtp
relay     unix  -       -       y       -       -       smtp
        -o syslog_name=postfix/$service_name
#       -o smtp_helo_timeout=5 -o smtp_connect_timeout=5
showq     unix  n       -       y       -       -       showq
error     unix  -       -       y       -       -       error
retry     unix  -       -       y       -       -       error
discard   unix  -       -       y       -       -       discard
local     unix  -       n       n       -       -       local
virtual   unix  -       n       n       -       -       virtual
lmtp      unix  -       -       y       -       -       lmtp
anvil     unix  -       -       y       -       1       anvil
scache    unix  -       -       y       -       1       scache
postlog   unix-dgram n  -       n       -       1       postlogd

Our own master.cf differs a bit more and probably could do with a sync/revisit in future.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Technically a breaking change. Although I don't expect this to cause any notable breakage, unless someone is using a custom / modified master.cf.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes

@polarathene
chore: Remove the wrapper script for Postfix
3921c62 
Postfix can run in foreground mode now, so long as it is not within a `chroot` jail, which is not necessary for a Docker container.
@polarathene polarathene added priority/low service/postfix area/scripts kind/improvement Improve an existing feature, configuration file or the documentation area/configuration (file) labels Jan 27, 2023
@polarathene polarathene added this to the v12.0.0 milestone Jan 27, 2023
@polarathene polarathene self-assigned this Jan 27, 2023
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Jan 27, 2023

I still need to look into if logging changes need to be handled as well, so this PR is not entirely complete. Would be good to review / discuss the current proposed change though 👍

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Jan 27, 2023 •
edited
Loading

Looks very good to me! If you need help investigating the test failure; let me know.

I think the changes proposed here make sense and we should go ahead with them.

@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Jan 27, 2023

If you need help investigating the test failure; let me know.

The current failure in CI I think I can resolve, but it's odd that it can timeout after 60 seconds, but sometimes does pass (at least locally).

Another random failure was with an rspamd test:

 ✗ [rspamd] detects and rejects virus [383]
   (from function `refute_output' in file test/test_helper/bats-assert/src/refute_output.bash, line 189,
    in test file test/tests/parallel/set1/spam_virus/rspamd.bats, line 88)
     `refute_output --partial "stored mail into mailbox 'INBOX'"' failed
   
   -- output should not contain substring --
   substring (1 lines):
     stored mail into mailbox 'INBOX'
   output (8 lines):
     Jan 27 09:45:15 mail postfix/smtpd[852]: DDB3BA15AB65: client=localhost[127.0.0.1]
     Jan 27 09:45:15 mail postfix/cleanup[863]: DDB3BA15AB65: message-id=<[email protected]>
     Jan 27 09:45:15 mail dovecot: lmtp([email protected])<866><883sJiud02NiAwAAUi6ngw>: sieve: msgid=<[email protected]>: stored mail into mailbox 'INBOX'
     Jan 27 09:45:15 mail postfix/lmtp[865]: 1E752A15AB59: to=<[email protected]>, relay=mail.example.test[/var/run/dovecot/lmtp], delay=0.81, delays=0.52/0.01/0.01/0.27, dsn=2.0.0, status=sent (250 2.0.0 <[email protected]> 883sJiud02NiAwAAUi6ngw Saved)
     Jan 27 09:45:15 mail dovecot: lmtp(866): Disconnect from local: Logged out (state=READY)
     Jan 27 09:45:15 mail postfix/qmgr[683]: 1E752A15AB59: removed
     Jan 27 09:45:15 mail postfix/cleanup[863]: DDB3BA15AB65: milter-reject: END-OF-MESSAGE from localhost[127.0.0.1]: 5.7.1 ClamAV FOUND VIRUS "Eicar-Signature"; from=<[email protected]> to=<[email protected]> proto=SMTP helo=<mail.example.test>
     Jan 27 09:45:15 mail postfix/smtpd[852]: disconnect from localhost[127.0.0.1] helo=1 mail=1 rcpt=1 data=0/1 quit=1 commands=4/5

I'm not sure if that's related to this PR change, it looks like a potential race condition as there seems to be two emails in those logs. The message ID between the mail stored in Dovecot Inbox and the one with the detected virus differ. It would seem that filtering on the message ID would be more reliable?

@polarathene

This comment was marked as resolved.

Sign in to view
@polarathene polarathene mentioned this pull request Jan 28, 2023
Merged
6 tasks
@polarathene polarathene force-pushed the chore/remove-postfix-wrapper branch from b5bf8b0 to 3921c62 Compare January 28, 2023 02:50
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Jan 28, 2023 •
edited
Loading

The CI failure with Amavis tests has been fixed and extracted out to a separate PR. When that's merged, and this PR updated to it, tests should pass 👍

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Jan 28, 2023 •
edited
Loading

I will provide a fix for the Rspamd test.

UPDATE: I will put that fix into #3026.

UPDATE2: @polarathene you mentioned filtering by ID, which lead me to integrating a solution that goes beyond just Rspamd adjustments. I hope this is fine as the adjustments for Rspamd are minimal.

georglauterbach added a commit that referenced this pull request Jan 28, 2023
@georglauterbach
add functionality for filtering mail log by ID
c342874 
This was not planned, but as @polarthene mentioned in
#3033 (comment)
, filtering the mail log by email ID would be (the only) correct
approach for the Rspamd test (to eliminate race conditions).

I asserted the currect state, and came to the conclusion that this might
(or actually is) something we want in more than one place. So I went
ahead and implemented a solution.

The solution for acquiring the ID is a bit slower because it ensures the
mail queue is empty _before_ and _after_ the mail is sent. This is the
tradeoff one has to make if they want to send multiple emails in one
test file and get their IDs.

I hope you like this approach. I will provide another PR that adjusts
our current tests to use these new functions.
georglauterbach added a commit that referenced this pull request Jan 28, 2023
@georglauterbach
add functionality for filtering mail log by ID
58b83fd 
This was not planned, but as @polarthene mentioned in
#3033 (comment)
, filtering the mail log by email ID would be (the only) correct
approach for the Rspamd test (to eliminate race conditions).

I asserted the currect state, and came to the conclusion that this might
(or actually is) something we want in more than one place. So I went
ahead and implemented a solution.

The solution for acquiring the ID is a bit slower because it ensures the
mail queue is empty _before_ and _after_ the mail is sent. This is the
tradeoff one has to make if they want to send multiple emails in one
test file and get their IDs.

I hope you like this approach. I will provide another PR that adjusts
our current tests to use these new functions.
@georglauterbach georglauterbach mentioned this pull request Jan 28, 2023
Merged
7 tasks
georglauterbach added a commit that referenced this pull request Jan 29, 2023
@georglauterbach
test helpers: add functionality for sending emails (#3026)
f496897 
* add functionality for filtering mail log by ID

This was not planned, but as @polarthene mentioned in
#3033 (comment)
, filtering the mail log by email ID would be (the only) correct
approach for the Rspamd test (to eliminate race conditions).

I asserted the currect state, and came to the conclusion that this might
(or actually is) something we want in more than one place. So I went
ahead and implemented a solution.

The solution for acquiring the ID is a bit slower because it ensures the
mail queue is empty _before_ and _after_ the mail is sent. This is the
tradeoff one has to make if they want to send multiple emails in one
test file and get their IDs.

I hope you like this approach. I will provide another PR that adjusts
our current tests to use these new functions.

* added note about our helper functions in the docs

I think our work for our custom test framework should be noted in the
docs for newcomers to better understand what they should do.

* adjust Rspamd test to use new helpers for sending

* improve filter helpers further

* add sanity check when acquiring mail ID

* re-add `refute_output` to test which should now work well
@georglauterbach georglauterbach merged commit 24d0c35 into docker-mailserver:master Jan 29, 2023
@polarathene polarathene mentioned this pull request Feb 23, 2023
Merged
7 tasks
@casperklein casperklein mentioned this pull request Feb 25, 2023
Closed
10 tasks
This was referenced Feb 26, 2023
Closed
Merged
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Feb 26, 2023 •
edited
Loading

For reference, the /var/mail-state volume will have some folders that Postfix used for chroot jail in prior releases. This content logs some warnings about mismatches (despite AFAIK not being relevant to operation of Postfix now).

It should be safe to remove the dev, etc, lib, usr directories from this state volume, based on insights described here. Release notes should probably mention that along with this PR in changelog.

@polarathene polarathene mentioned this pull request Feb 28, 2023
Merged
7 tasks
@polarathene polarathene mentioned this pull request Mar 31, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

Assignees

@polarathene polarathene

Labels

area/configuration (file) area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/low service/postfix

Projects

None yet

Milestone

v12.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@polarathene @georglauterbach @casperklein