other: test normalizations

[georglauterbach]

Subject

I would like to contribute to the project

Description

We need a follow-up of #3732. Tasks:

•  use .txt suffix for _send_email and _nc_wrapper
•  in smtp_delivery.bats, remove the need for existing/userX and remove these files (ref: https://github.com/docker-mailserver/docker-mailserver/pull/3732/files#r1438975373)
•  add a new helper _send_email_unchecked that works like _send_email but has no assert_success directly afterward to check that all went well and add such an assert_success in _send_email
• use --auth PLAIN instead --auth LOGIN ?

[georglauterbach]

Re-write _send_email_and_get_id

function _send_email_and_get_id() {
  export "${1:?Mail ID must be set for _send_email_and_get_id}"
  local -n MAIL_ID=${1}
  shift 1

  _wait_for_empty_mail_queue_in_container
  _send_email "${@}"
  _wait_for_empty_mail_queue_in_container

  # The unique ID Postfix (and other services) use may be different in length
  # on different systems (e.g. amd64 (11) vs aarch64 (10)). Hence, we use a
  # range to safely capture it.
  MAIL_ID=$(_exec_in_container tac /var/log/mail.log              \
    | grep -E -m 1 'postfix/smtpd.*: [A-Z0-9]+: client=localhost' \
    | grep -E -o '[A-Z0-9]{9,12}' || true)

  assert_not_equal "${MAIL_ID}" ''
}

This avoids subshells that mess with checks (e.g. this changes makes the final check assert_not_equal work properly).

---

Reminder to myself: this change is already stashed; just pop the stash.

[polarathene]

Just going to dump some notes that have since gone a bit stale but were related to the changes PRs for this issue addressed.

No expectation for anyone to read through all this, but there may be something helpful when troubleshooting the changes associated to this issue.

---

Initial swaks PR change overview (Outdated)

• Renames:
  • test/test-files/ renamed to test/files/
  • test/files/email-templates/ renamed to test/files/emails/
  • test/files/auth/ and test/files/emails/ restructured to use common file prefixes as folder names instead for grouping related files.
• Added swaks package to replace nc where possible for sending email:
  • _send_email() adapted to swaks from nc, _nc_wrapper() helper provides equivalent support where swaks is not compatible.
  • test/files/emails/ removed SMTP commands, file contents only contain the DATA portion for providing to swaks via --data.
  • test/files/emails/ adds an nc_raw/ directory for files that are not swaks compatible, and thus must go through _nc_wrapper() instead of _send_email().
    • dsn/ is for dsn.bats, where swaks needs to support the equivalent of NOTIFY=success,failure for the RCPT TO SMTP command (--to in swaks).
    • postscreen.bats needs to test a misbehaving client that does not wait their turn, thus we use _nc_wrapper() here, since it sends all input to the destination at once.
    • permit_docker.bats relies on smtp-only.txt only as a generic mail to test toggling permission to send via network trust (PERMIT_DOCKER). Blocked by test file as not yet migrated to new testing format (required to access the _send_email helper).
  • test/files/auth/ no longer has SMTP Auth (adapted to swaks with --quit-after AUTH). Remaining files rely on _nc_wrapper() for IMAP / POP3 auth related testing.

TODO:

• nc_raw/smtp-ehlo.txt: Not necessary, swaks --quit-after FIRST-HELO

---

Follow-up PRs

• Mail intended to be submitted from a client to DMS (most over port 465 with auth): --ehlo mail.example.test + --from [email protected] + --protocol SSMTPA (port 465 with auth)
• Mail intended to be delivered to DMS from an external MTA (most tests): --ehlo mail.external.tld + --from [email protected] + --protocol ESMTP (uses port 25 implicitly, this is the default --protocol value)
  • non-existing-user.txt: Removable, only needs to send a generic mail with --to [email protected].
  • amavis/spam.txt + rspamd/spam.txt can be replaced by a generic mail that sends the standard GTUBE with --from [email protected].
  • amavis/virus.txt + rspamd/virus.txt can likewise do the same with EICAR and --from [email protected].
  • emails/auth/ are all for testing the SPOOF_PROTECTION=1 feature.
    • These are specifically testing the authenticated DMS account against the envelope sender (--from).
    • Sending a generic mail is sufficient, while --auth-user / --from can be provided separately as overrides.
  • emails/existing/ are all generic (with the exception of the envelope-recipient via --to) for testing Postfix features related to routing mail delivery.
    • smtp_delivery.bats tests against each mail by asserting against their Subject header.
    • Additionally user-and-cc-local-alias.txt needs to append the CC header Cc: Existing Local Alias <[email protected]>
      • The alias (alias2) resolves to [email protected].
      • Alias resolution is asserted in test-case: redirects mail to external aliases, however that alias is used in another mail sent which makes the test-case not reliable for coverage on the CC header?
      • NOTE: The file name incorrectly has a local-alias suffix while the alias is to an external domain? (resolved since all files from this group were removed by a follow-up PR)
• Docs permalink update: tests: Use swaks instead of nc for sending mail #3732 (comment)
• Changing external.tld => external-third-party.test + localhost.localdomain => example.test, etc changes: tests: Use swaks instead of nc for sending mail #3732 (comment)
• postgrey.bats whitelist test-case, investigate removing workaround with a separate client container and not sending mail direct to Postgrey port: tests: Use swaks instead of nc for sending mail #3732 (comment)
• postscreen.bats split container (needs resolvable sender address domain, and likely TXT DNS record for SPF after that): tests: Use swaks instead of nc for sending mail #3732 (comment)
• Instead of -tlsc (or the long option name --tls-on-connect), prefer setting --protocol with SSMTP or SSMTPA which is equivalent to specifying options --auth PLAIN --port 465 -tlsc. Although better alternative going forward would be to have a separate base config for swaks that includes the auth options. Usage in tests would then change to providing a different config with --config path/to/swaks/config/with/auth.

Maintenance notes

• ESMTP / EHLO and when it's important vs SMTP / HELO.
• Be mindful when searching up docs for swaks, the correct docs source is here but appears to have had broken Github rendering support since Jan 2017.
  • For --data option, the default template options are covered here (note: permalink, this is rendered properly).
  • When adding/modifying headers you have the --add-header and --header options, see the swaks output + behaviour difference between these two here.
    • --header will override an existing header if it exists, otherwise add the header, prefer it unless you need multiples of the same header.
    • Output shared is related to the Message-ID header and log snippets related to DMS which may be a useful reference.
      • Related was another review comment comparing Postfix Queue ID between rspamd and amavis/spamassassin along with rejection response from rspamd.
      • Another review comment details Postfix Queue ID generation. This is now included as a reference in the relevant test method that uses regex pattern to match the value.
• postgrey_whitelist.txt importance of blank line: tests: Use swaks instead of nc for sending mail #3732 (comment)
• postscreen.bats context (could probably improve the ENV docs as linked comment describes): tests: Use swaks instead of nc for sending mail #3732 (comment)
• The migration to swaks identified some mail headers in DATA that was discarded](sieve/pipe and sieve/spam-folder), these were mostly contextual / complimentary but could have been useful towards filtering. Presently they are very simple.
• smtponly.bats: PR context related to the adjustment for swaks
• smtp_delivery.bats: SMTPUTF8 test-case PR context for change to leverage swaks features.
• tests.bats: ENV SPOOF_PROTECTION=1 auth test PR context (forging test-case) + PR context (alias test-case). Current location.

Unrelated to swaks migration:

• Tests for a regexp alias (postfix-regexp.cf support) coverage:
  • Resolves to external address could use a more appropriate address (as linked review comment suggests) to communicate the context better. Current location.
  • Resolves to local address, which differs by resolving to an internal address. Current location
• Other aliases (postfix-virtual.cf) test coverage:
  • Standard alias (Current location, incorrectly labeled as "external").
  • Standard alias with recipient delimiter (Current location, original PR that introduced this alias recipient feature).
  • Wildcard / Catch-All 2nd domain aliased to a primary domain address (Current location).
  • This CC alias mail sent collides with a similar alias test mail where test-case would pass for either. There is no separate test coverage on the CC address. See reference section below for history details on why CC was used and what it's actually intended to test (relevance is lost since switching from Dovecot LDA to LMTP). Related review comment.
• The root mail test lacks contextual comment (Current location. Internal mail like update notifications or from Amavis / SA (via cronjob) needs to deliver mail without authentication and bypassing other checks, such as not requiring PERMIT_DOCKER=container and bypassing spam filtering. sendmail / mail commands will accomplish that and drop it straight into the Postfix delivery queue instead. swaks is not usable for that functionality AFAIK.
• privacy.bats still relies on --data to send a generic mail with a User-Agent header, swaks should be able to remove that file and explicitly set the header in the test via --header option. PR context.

References

May still have some relevance/value when refactoring existing tests and with recent changes this context would potentially be even less obvious and not fun to track down 😅

Original text files history:

• Grouped into emails/existing/ folder in PR 3732: tests: Use swaks instead of nc for sending mail #3732
• Replaced with swaks equivalent commands in PR 3747: tests: normalizations #3747

local-alias.txt vs external-alias.txt (and similar variants where local/external is reversed in usage):

• postfix-accounts.cf is configured with a [email protected], which would include otherdomain.tld as a local DMS managed mail domain...thus locally managed (at least it is these days).
  • Originally added in Oct 2015 by project author.
  • Follow-up PR shortly after introduces the first few "email templates", amavis-virus + amavis-spam, and the alias external + local usage but resolving to aliases to the opposite domain-part implied by the filenames.
  • May 2016 introduces regexp aliases in tests with local and external variants correctly resolving to their implied domain-part via postfix-regexp.cf:
    • local variant: test + 2 digits prefix (local-part prefix to a localhost.localdomain domain-part) used in regexp-alias-local.txt (as [email protected]) has a regexp alias to [email protected].
    • external variant: bounce (local-part prefix to any domain-part) used in regexp-alias-external.txt (as [email protected]) has a regexp alias to the same [email protected].

user-and-cc-local-alias.txt:

• Originally added July 2016 to resolve a multiple recipient delivery issue with Postfix + Dovecot (alias usage is not relevant). Common enough issue online.
• Oct 2016 removed dovecot_destination_recipient_limit=1 from Postfix main.cf due to "unused parameter" warning spam in logs.
  • The parameter name has a dynamic prefix and actually documented as transport_destination_recipient_limit, where the transport prefix maps to a transport name defined in master.cf. It is a parameter relevant to pipe(8) (Postfix delivery to external command).
  • Earlier in Oct 2016, Dovecot delivery was changed to LMTP instead of Dovecot LDA (which was configured as the dovecot transport in master.cf). Hence the warning log spam issue that followed.
  • As docs for the Postfix master.cf pipe daemon describe under "Single-Recipient Delivery", multi-recipient delivery can be problematic when Delivered-to: / X-Original-To: message headers are prepended.
    • Which transport_destination_recipient_limit can configure per-transport, whilst all other delivery agents (lmtp, pipe, smtp, virtual) otherwise get their default limit from default_destination_recipient_limit (default: 50).
    • Previously with the dovecot transport in master.cf this used flags=DRhu which as the pipe daemon docs (on flags section) states, requires the limit parameter is set to 1 to correctly work.
    • This configuration allowed supporting those specific headers with delivering to multiple recipients, with the only difference in overhead by delivering to each recipient sequentially instead of in bulk to Dovecot to handle on it's end.
  • Feb 2017 issue of the change to LMTP affecting Dovecot Sieve, since the X-Original-To: header was not available, the mail that arrived for Sieve to process had already resolved the alias and lost this context that the additional header would have provided.
    • There are a few workarounds such as:
      • Using another Postfix feature to conditionally match the recipient to prepend the header, but it removes privacy and probably doesn't work as desired with multiple recipients involved. Another comment explains some technical details why delivering to multiple recipients had to be limited to processing one at a time.
      • Adjusting the sieve filter for different headers, but this is not compatible with BCC recipients. Another use-case is supporting a catch-all address to direct mail to a folder via sieve script.
    • Besides pipe, it is also supported by local and virtual delivery agents, but not smtp and lmtp delivery agents.
      • 7 years later with the Postfix 3.3.0 release (Feb 2018), the virtual and local transports gained support for enable_original_recipient (default: yes), which prepends the X-Original-To header. This may have been for the ability to set enable_original_recipient=no according to the parameter docs.
      • In Mar 2020 the Postfix 3.5 release lmtp and smtp delivery agents gained support for Delivered-To: / X-Original-To: header additions with the same flags=DO syntax that pipe supported, which likewise rely on transport_destination_recipient_limit being set to 1 to work.

catchall-local.txt:

• Added July 2016.
• Also shows vhost for otherdomain.tld, so that is treated as locally managed.

emails/root-email.txt:

• postmaster local-part regexp alias with any domain-part to [email protected]
• Introduced April 2018, testing the system root account delivers mail to the configured postmaster address (ENV POSTMASTER_ADDRESS). Test has mail delivered to root, normally aliased to POSTMASTER_ADDRESS, while the regexp alias to user account is used as fallback.
• Neither helper/setup.bash or smtp_delivery.bats (only user of root-email.txt) sets the POSTMASTER_ADDRESS ENV. This will potentially be set in future which may affect this in a subtle way? 🤷

existing/user1 was a common file used for generic mail testing. With the switch to swaks this is roughly the defaults, but there was still some inconsistencies (sometimes intentional by specifying defaults explicitly as options for visibility) vs how it used to be.

---

swaks related references

Most mail can use the following as base config and override via CLI options as needed (this was initial work towards advising a swaks config file for base/defaults):

# Envelope sender and recipient:
--from: [email protected]
--to: [email protected]
# Sender and recipient mail headers:
--h-From: "External mail client" <[email protected]>
--h-To: "Existing Local User" <[email protected]>

Prior to swaks we relied on netcat / nc to stream a series of SMTP commands with the common format / convention:

HELO mail.external.tld
MAIL FROM: [email protected]
RCPT TO: <recipient here>
DATA
<data content here>

.
QUIT

For context, the SMTP commands are removed in favor of retaining only the DATA content.

• A PR introduced swaks with fallback config (helper method explicit CLI options with defaults) that represents the now dropped files most common SMTP commands.
• The remaining content is fed into swaks command via --data and STDIN (a filename path should work too when prefixed with @).

We also switched to ESMTP with EHLO for all mail when adopting swaks, previously that was only used when necessary (eg: SMTP Auth).

When I was originally putting together a comment about suggesting to provide default swaks config via file (before the helper method with CLI options was proposed), I cited the swaks docs with (paraphrased):

Options can be given to swaks in three ways (config, env, CLI).

• When swaks evaluates its options:
  • It first looks for a configuration file (either in a default location or specified with --config).
  • Then it evaluates any options in environment variables.
  • Finally, it evaluates command line options.
• At each round of processing, any options set earlier will be overridden.
• Additionally, any option can be prefixed with no- to cause swaks to forget that the variable had previously been set.