From e6af5a118fb2e6894abc2eafc5f4568e47765677 Mon Sep 17 00:00:00 2001
From: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
Date: Wed, 2 Mar 2022 22:22:17 +0100
Subject: [PATCH] Adjust envrionment variables - more sensible defaults (#2428)

The new setup will now set env variables on one place and on one place
only. The old setup used two separate places wich is not DRY and
confusing.

Some default values changed:

1. PFLOGSUMM_TRIGGER: logrotate => none
2. REPORT_SENDER: mailserver-report@HOSTNAME => mailserver-report@DOMAIN
3. REPORT_RECIPIENT: "0" => POSTMASTER_ADDRESS

One env variable was renamed: REPORT_INTERVAL => LOGROTATE_INTERVAL

I believe these defaults to be more sensible, especially the REPORT_RECIPIENT
address. The PFLOGSUMM_TRIGGER value was changed to `none` because otherwise
people would start getting daily Postfix log summary reports automatically.
Now, this is opt-in, and reports are sent only when enabled properly.

Some of the variables changed were marked as deprecated. I removed the note,
as the variables now bear some (sane) defaults again for other variables
(i.e.) REPORT_RECIPIENT is now default for other recipient addresses.

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Casper <casperklein@users.noreply.github.com>
---
 docs/content/config/environment.md    | 55 +++++++++++----------------
 mailserver.env                        | 30 ++++++++-------
 target/scripts/start-mailserver.sh    | 25 +++++++-----
 target/scripts/startup/setup-stack.sh | 36 ------------------
 test/mail_with_ldap.bats              | 10 +++--
 test/tests.bats                       |  1 +
 6 files changed, 62 insertions(+), 95 deletions(-)

diff --git a/docs/content/config/environment.md b/docs/content/config/environment.md
index b181a8ec..062a20f8 100644
--- a/docs/content/config/environment.md
+++ b/docs/content/config/environment.md
@@ -249,7 +249,7 @@ Note: More information at <https://dovecot.org/doc/dovecot-example.conf>
 
 ##### PFLOGSUMM_TRIGGER
 
-Enables regular pflogsumm mail reports.
+Enables regular Postfix log summary ("pflogsumm") mail reports.
 
 - **not set** => No report
 - daily_cron => Daily report for the previous day
@@ -260,14 +260,14 @@ If this is not set and reports are enabled with the old options, logrotate will
 
 ##### PFLOGSUMM_RECIPIENT
 
-Recipient address for pflogsumm reports.
+Recipient address for Postfix log summary reports.
 
-- **not set** => Use REPORT_RECIPIENT or POSTMASTER_ADDRESS
+- **not set** => Use POSTMASTER_ADDRESS
 - => Specify the recipient address(es)
 
 ##### PFLOGSUMM_SENDER
 
-Sender address (`FROM`) for pflogsumm reports if pflogsumm reports are enabled.
+Sender address (`FROM`) for pflogsumm reports (if Postfix log summary reports are enabled).
 
 - **not set** => Use REPORT_SENDER
 - => Specify the sender address
@@ -294,48 +294,39 @@ Sender address (`FROM`) for logwatch reports if logwatch reports are enabled.
 - **not set** => Use REPORT_SENDER
 - => Specify the sender address
 
-##### REPORT_RECIPIENT (deprecated)
+##### REPORT_RECIPIENT
 
-Enables a report being sent (created by pflogsumm) on a regular basis.
+Defines who receives reports (if they are enabled).
 
-- **0** => Report emails are disabled unless enabled by other options
-- 1 => Using POSTMASTER_ADDRESS as the recipient
+- **empty** => Use POSTMASTER_ADDRESS
 - => Specify the recipient address
 
-##### REPORT_SENDER (deprecated)
+##### REPORT_SENDER
 
-Change the sending address for mail report
+Defines who sends reports (if they are enabled).
 
-- **empty** => mailserver-report@hostname
-- => Specify the report sender (From) address
-
-##### REPORT_INTERVAL (deprecated)
-
-Changes the interval in which logs are rotated and a report is being sent (deprecated).
-
-- **daily** => Send a daily report
-- weekly => Send a report every week
-- monthly => Send a report every month
-
-Note: This variable used to control logrotate inside the container and sent the pflogsumm report when the logs were rotated.
-It is still supported for backwards compatibility, but the new option LOGROTATE_INTERVAL has been added that only rotates
-the logs.
+- **empty** => `mailserver-report@<YOUR DOMAIN>`
+- => Specify the sender address
 
 ##### LOGROTATE_INTERVAL
 
-Defines the interval in which the mail log is being rotated.
+Changes the interval in which a report is being sent.
 
-- **daily** => Rotate daily.
-- weekly => Rotate weekly.
+- **weekly** => Rotate weekly.
+- daily => Rotate daily.
 - monthly => Rotate monthly.
 
-Note that only the log inside the container is affected.
-The full log output is still available via `docker logs mailserver` (_or your respective container name_).
-If you want to control logrotation for the docker generated logfile, see: [Docker Logging Drivers](https://docs.docker.com/config/containers/logging/configure/).
+!!! note
 
-Also note that by default the logs are lost when the container is recycled. To keep the logs, mount a volume.
+    `LOGROTATE_INTERVAL` only manages `logrotate` within the container for services we manage internally.
+    
+    The entire log output for the container is still available via `docker logs mailserver` (or your respective container name). If you want to configure external log rotation for that container output as well, : [Docker Logging Drivers](https://docs.docker.com/config/containers/logging/configure/).
+    
+    By default, the logs are lost when the container is destroyed (eg: re-creating via `docker-compose down && docker-compose up -d`). To keep the logs, mount a volume (to `/var/log/mail/`).
 
-Finally the logrotate interval **may** affect the period for generated reports. That is the case when the reports are triggered by log rotation.
+!!! note
+
+    This variable can also determine the interval for Postfix's log summary reports, see [`PFLOGSUMM_TRIGGER`](#pflogsumm_trigger).
 
 #### SpamAssassin
 
diff --git a/mailserver.env b/mailserver.env
index abac0d53..8ffdd88e 100644
--- a/mailserver.env
+++ b/mailserver.env
@@ -217,24 +217,28 @@ LOGWATCH_RECIPIENT=
 # => Specify the sender address
 LOGWATCH_SENDER=
 
-# Enables a report being sent (created by pflogsumm) on a regular basis. (deprecated)
-# **0** => Report emails are disabled
-# 1 => Using POSTMASTER_ADDRESS as the recipient
+# Defines who receives reports if they are enabled.
+# **empty** => ${POSTMASTER_ADDRESS}
 # => Specify the recipient address
-REPORT_RECIPIENT=0
+REPORT_RECIPIENT=
 
-# Change the sending address for mail report (deprecated)
-# **empty** => mailserver-report@hostname
-# => Specify the report sender (From) address
+# Defines who sends reports if they are enabled.
+# **empty** => mailserver-report@${DOMAINNAME}
+# => Specify the sender address
 REPORT_SENDER=
 
-# Changes the interval in which a report is being sent. (deprecated)
-# **daily** => Send a daily report
-# weekly => Send a report every week
+# Changes the interval in which a report is being sent.
+# **weekly** => Send a report every week
+# daily => Send a daily report
 # monthly => Send a report every month
 #
-# Note: This Variable actually controls logrotate inside the container and rotates the log depending on this setting. The main log output is still available in its entirety via `docker logs mail` (Or your respective container name). If you want to control logrotation for the docker generated logfile see: [Docker Logging Drivers](https://docs.docker.com/config/containers/logging/configure/)
-REPORT_INTERVAL=daily
+# Note: This Variable actually controls logrotate inside the container
+# and rotates the log depending on this setting. The main log output is
+# still available in its entirety via `docker logs mail` (Or your
+# respective container name). If you want to control logrotation for
+# the Docker-generated logfile see:
+# https://docs.docker.com/config/containers/logging/configure/
+LOGROTATE_INTERVAL=weekly
 
 # Choose TCP/IP protocols for postfix to use
 # **all** => All possible protocols.
@@ -269,7 +273,7 @@ SPAMASSASSIN_SPAM_TO_INBOX=1
 # Note: only has an effect if `ENABLE_SPAMASSASSIN=1`
 ENABLE_SPAMASSASSIN_KAM=0
 
-# this setting needs `SPAMASSASSIN_SPAM_TO_INBOX=1`
+# spam messages will be moved in the Junk folder (SPAMASSASSIN_SPAM_TO_INBOX=1 required)
 MOVE_SPAM_TO_JUNK=1
 
 # add spam info headers if at, or above that level:
diff --git a/target/scripts/start-mailserver.sh b/target/scripts/start-mailserver.sh
index 1c70462c..f234d14c 100755
--- a/target/scripts/start-mailserver.sh
+++ b/target/scripts/start-mailserver.sh
@@ -3,19 +3,23 @@
 # shellcheck source=./helpers/index.sh
 source /usr/local/bin/helpers/index.sh
 
-unset FUNCS_SETUP FUNCS_FIX FUNCS_CHECK FUNCS_MISC
-unset DAEMONS_START HOSTNAME DOMAINNAME
-
-#shellcheck disable=SC2034
+# shellcheck disable=SC2034
 declare -A VARS
 declare -a FUNCS_SETUP FUNCS_FIX FUNCS_CHECK FUNCS_MISC DAEMONS_START
 
+_obtain_hostname_and_domainname
+
 # ------------------------------------------------------------
 # ? <<
 # --
 # ? >> Setup of default and global values / variables
 # ------------------------------------------------------------
 
+# These variables must be defined first; They are used as default values for other variables.
+VARS[POSTMASTER_ADDRESS]="${POSTMASTER_ADDRESS:=postmaster@${DOMAINNAME}}"
+VARS[REPORT_RECIPIENT]="${REPORT_RECIPIENT:=${POSTMASTER_ADDRESS}}"
+VARS[REPORT_SENDER]="${REPORT_SENDER:=mailserver-report@${DOMAINNAME}}"
+
 VARS[AMAVIS_LOGLEVEL]="${AMAVIS_LOGLEVEL:=0}"
 VARS[DEFAULT_RELAY_HOST]="${DEFAULT_RELAY_HOST:=}"
 VARS[DMS_DEBUG]="${DMS_DEBUG:=0}"
@@ -41,12 +45,17 @@ VARS[FAIL2BAN_BLOCKTYPE]="${FAIL2BAN_BLOCKTYPE:=drop}"
 VARS[FETCHMAIL_PARALLEL]="${FETCHMAIL_PARALLEL:=0}"
 VARS[FETCHMAIL_POLL]="${FETCHMAIL_POLL:=300}"
 VARS[LDAP_START_TLS]="${LDAP_START_TLS:=no}"
-VARS[LOGROTATE_INTERVAL]="${LOGROTATE_INTERVAL:=${REPORT_INTERVAL:-daily}}"
+VARS[LOGROTATE_INTERVAL]="${LOGROTATE_INTERVAL:=weekly}"
 VARS[LOGWATCH_INTERVAL]="${LOGWATCH_INTERVAL:=none}"
+VARS[LOGWATCH_RECIPIENT]="${LOGWATCH_RECIPIENT:=${REPORT_RECIPIENT}}"
+VARS[LOGWATCH_SENDER]="${LOGWATCH_SENDER:=${REPORT_SENDER}}"
 VARS[MOVE_SPAM_TO_JUNK]="${MOVE_SPAM_TO_JUNK:=1}"
 VARS[NETWORK_INTERFACE]="${NETWORK_INTERFACE:=eth0}"
 VARS[ONE_DIR]="${ONE_DIR:=1}"
-VARS[OVERRIDE_HOSTNAME]="${OVERRIDE_HOSTNAME}"
+VARS[OVERRIDE_HOSTNAME]="${OVERRIDE_HOSTNAME:-}"
+VARS[PFLOGSUMM_RECIPIENT]="${PFLOGSUMM_RECIPIENT:=${REPORT_RECIPIENT}}"
+VARS[PFLOGSUMM_SENDER]="${PFLOGSUMM_SENDER:=${REPORT_SENDER}}"
+VARS[PFLOGSUMM_TRIGGER]="${PFLOGSUMM_TRIGGER:=none}"
 VARS[PERMIT_DOCKER]="${PERMIT_DOCKER:=none}"
 VARS[POSTFIX_INET_PROTOCOLS]="${POSTFIX_INET_PROTOCOLS:=all}"
 VARS[POSTFIX_MAILBOX_SIZE_LIMIT]="${POSTFIX_MAILBOX_SIZE_LIMIT:=0}"
@@ -57,7 +66,6 @@ VARS[POSTGREY_MAX_AGE]="${POSTGREY_MAX_AGE:=35}"
 VARS[POSTGREY_TEXT]="${POSTGREY_TEXT:=Delayed by Postgrey}"
 VARS[POSTSCREEN_ACTION]="${POSTSCREEN_ACTION:=enforce}"
 VARS[RELAY_HOST]="${RELAY_HOST:=}"
-VARS[REPORT_RECIPIENT]="${REPORT_RECIPIENT:="0"}"
 VARS[SA_KILL]=${SA_KILL:="6.31"}
 VARS[SA_SPAM_SUBJECT]=${SA_SPAM_SUBJECT:="***SPAM*** "}
 VARS[SA_TAG]=${SA_TAG:="2.0"}
@@ -70,11 +78,8 @@ VARS[SSL_TYPE]="${SSL_TYPE:=}"
 VARS[SUPERVISOR_LOGLEVEL]="${SUPERVISOR_LOGLEVEL:=warn}"
 VARS[TLS_LEVEL]="${TLS_LEVEL:=modern}"
 VARS[UPDATE_CHECK_INTERVAL]="${UPDATE_CHECK_INTERVAL:=1d}"
-# shellcheck disable=SC2034
 VARS[VIRUSMAILS_DELETE_DELAY]="${VIRUSMAILS_DELETE_DELAY:=7}"
 
-_obtain_hostname_and_domainname
-
 # ------------------------------------------------------------
 # ? << Setup of default and global values / variables
 # --
diff --git a/target/scripts/startup/setup-stack.sh b/target/scripts/startup/setup-stack.sh
index edf474e7..f7cda1bf 100644
--- a/target/scripts/startup/setup-stack.sh
+++ b/target/scripts/startup/setup-stack.sh
@@ -42,42 +42,6 @@ function _setup_default_vars
 {
   _notify 'task' 'Setting up default variables'
 
-  # update POSTMASTER_ADDRESS - must be done done after _check_hostname
-  POSTMASTER_ADDRESS="${POSTMASTER_ADDRESS:=postmaster@${DOMAINNAME}}"
-
-  # update REPORT_SENDER - must be done done after _check_hostname
-  REPORT_SENDER="${REPORT_SENDER:=mailserver-report@${HOSTNAME}}"
-  LOGWATCH_SENDER="${LOGWATCH_SENDER:=${REPORT_SENDER}}"
-  PFLOGSUMM_SENDER="${PFLOGSUMM_SENDER:=${REPORT_SENDER}}"
-
-  # set PFLOGSUMM_TRIGGER here for backwards compatibility
-  # when REPORT_RECIPIENT is on the old method should be used
-  # ! needs to be a string comparison
-  if [[ ${REPORT_RECIPIENT} == '0' ]]
-  then
-    PFLOGSUMM_TRIGGER="${PFLOGSUMM_TRIGGER:=none}"
-  else
-    PFLOGSUMM_TRIGGER="${PFLOGSUMM_TRIGGER:=logrotate}"
-  fi
-
-  # expand address to simplify the rest of the script
-  if [[ ${REPORT_RECIPIENT} == '0' ]] || [[ ${REPORT_RECIPIENT} == '1' ]]
-  then
-    REPORT_RECIPIENT="${POSTMASTER_ADDRESS}"
-  fi
-
-  PFLOGSUMM_RECIPIENT="${PFLOGSUMM_RECIPIENT:=${REPORT_RECIPIENT}}"
-  LOGWATCH_RECIPIENT="${LOGWATCH_RECIPIENT:=${REPORT_RECIPIENT}}"
-
-  VARS[LOGWATCH_RECIPIENT]="${LOGWATCH_RECIPIENT}"
-  VARS[LOGWATCH_SENDER]="${LOGWATCH_SENDER}"
-  VARS[PFLOGSUMM_RECIPIENT]="${PFLOGSUMM_RECIPIENT}"
-  VARS[PFLOGSUMM_SENDER]="${PFLOGSUMM_SENDER}"
-  VARS[PFLOGSUMM_TRIGGER]="${PFLOGSUMM_TRIGGER}"
-  VARS[POSTMASTER_ADDRESS]="${POSTMASTER_ADDRESS}"
-  VARS[REPORT_RECIPIENT]="${REPORT_RECIPIENT}"
-  VARS[REPORT_SENDER]="${REPORT_SENDER}"
-
   : >/root/.bashrc     # make DMS variables available in login shells and their subprocesses
   : >/etc/dms-settings # this file can be sourced by other scripts
 
diff --git a/test/mail_with_ldap.bats b/test/mail_with_ldap.bats
index a297f731..e7fc390a 100644
--- a/test/mail_with_ldap.bats
+++ b/test/mail_with_ldap.bats
@@ -5,8 +5,9 @@ function setup_file() {
   docker build -f Dockerfile -t ldap --no-cache .
   popd || return 1
 
-  export FQDN_MAIL='mail.my-domain.com'
-  export FQDN_LDAP='ldap.my-domain.com'
+  export DOMAIN='my-domain.com'
+  export FQDN_MAIL="mail.${DOMAIN}"
+  export FQDN_LDAP="ldap.${DOMAIN}"
   export FQDN_LOCALHOST_A='localhost.localdomain'
   export FQDN_LOCALHOST_B='localhost.otherdomain'
   export DMS_TEST_NETWORK='test-network-ldap'
@@ -33,6 +34,7 @@ function setup_file() {
     -e DOVECOT_TLS=no \
     -e DOVECOT_USER_FILTER="(&(objectClass=PostfixBookMailAccount)(uniqueIdentifier=%n))" \
     -e ENABLE_LDAP=1 \
+    -e PFLOGSUMM_TRIGGER=logrotate \
     -e ENABLE_SASLAUTHD=1 \
     -e LDAP_BIND_DN=cn=admin,dc=localhost,dc=localdomain \
     -e LDAP_BIND_PW=admin \
@@ -228,11 +230,11 @@ function teardown_file() {
 
 @test "checking pflogsum delivery" {
   # checking default sender is correctly set when env variable not defined
-  run docker exec mail_with_ldap grep "mailserver-report@${FQDN_MAIL}" /etc/logrotate.d/maillog
+  run docker exec mail_with_ldap grep "mailserver-report@${DOMAIN}" /etc/logrotate.d/maillog
   assert_success
 
   # checking default logrotation setup
-  run docker exec mail_with_ldap grep "daily" /etc/logrotate.d/maillog
+  run docker exec mail_with_ldap grep "weekly" /etc/logrotate.d/maillog
   assert_success
 }
 
diff --git a/test/tests.bats b/test/tests.bats
index f4580f0c..afd01051 100644
--- a/test/tests.bats
+++ b/test/tests.bats
@@ -20,6 +20,7 @@ setup_file() {
     -e ENABLE_QUOTAS=1 \
     -e ENABLE_SPAMASSASSIN=1 \
     -e ENABLE_SRS=1 \
+    -e PFLOGSUMM_TRIGGER=logrotate \
     -e ENABLE_UPDATE_CHECK=0 \
     -e PERMIT_DOCKER=container \
     -e PERMIT_DOCKER=host \