tests: Use swaks instead of nc for sending mail

[georglauterbach]

Description

Use swaks instead of nc. This PR is a blocker for properly implementing #3727. Review commit by commit.

Fixes #3728

Type of change

• Improvement (non-breaking change that does improve existing functionality)
• This change requires a documentation update

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (README.md or the documentation under docs/)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have added information about changes made in this PR to CHANGELOG.md

[github-actions]

Documentation preview for this PR is ready! 🎉

Built with commit: 9e29a54

[polarathene]

• Docs permalink will be invalid. You'll need to defer that as a follow-up PR once this is merged, or split out the relocation of files to a separate PR and get that merged first.
• Some feedback on the reworked helper methods
• postscreen.bats feedback. Changes introduced are not obvious why, lacks context.

The final 2 commits where the bulk of changes with diffs exist is yet to be reviewed.

[polarathene]

While I prefer keeping git blame happy and smaller PRs, I doubt anyone else was going to get around to this :P

Thanks for putting in the time and effort! ❤️

---

• Commit: rename test/test-files -> test/files

  Additional prefix is pretty useless, and I wanted to do this for a long time now.

• Commit: rename test/files/nc_templates -> test/files/nc

  These are not templates, but files that are "simply" used with nc.

• Commit: create structure for emails

👍

NOTE: You'll want to extract out docs/content/config/security/ssl.md from the first commit though (or just append commits and we'll squash it all, since I'm probably one of the very few that extensively uses git blame to care 😂 ).

---

Commit: use swaks now instead of nc

The wrapper is made so custom options can be applied with flags, and common arguments are defaults.
The actual invocation is easy to read but requires the e-mail files to be adjusted. This will happen in a latter commit.

🎉 🚀

---

Commit: move e-mails and adjust contents slightly

Basically remove the -templates suffix (because, again, they are not templates) and adjust the contents by removing the first parts (like EHLO:, RCPT TO:, MAIL FROM:).

Renames with modifications to those same files in a single commit 😭

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

• Prior commit introduces swaks with fallback config that represents the dropped SMTP commands.
• The remaining content is fed into swaks command via --data and STDIN (a filename path should work just as well btw).

The bulk of our "templates" had a fairly consistent set of values, with most using the following command pattern:

HELO mail.external.tld
MAIL FROM: user@external.tld
RCPT TO: <recipient here>
DATA
<data content here>

.
QUIT

There is some drift, and this PR also adds some inconsistencies. I'll touch on that separately.

---

Commit: adjust tests to use new functions
Commit: fixed serial tests and openssl invocations

I haven't got around to reviewing these two properly yet. I'll tackle them in my next review pass.

[polarathene]

@polarathene do you have any idea where to look? The test afterward passes fine, actually all other tests pass..

The supervisord config was recently updated for Postgrey to log to it's own file. The test was changed accordingly and passed #3724

Doesn't seem like it'd be related to that 🤔

---

The test-case has a TODO comment questioning if it was testing correctly in the first place:

• This allows to bypass the SMTP protocol on port 25, and send data directly to Postgrey instead.
• Appears to be a workaround due to client_name=localhost when sent from Postfix.
• Could send over port 25 if whitelisting localhost,
  • However this does not help verify that the actual client HELO address is properly whitelisted?
  • It'd also cause the earlier greylist test to fail.
    TODO: Actually confirm whitelist feature works correctly as these test cases are using a workaround

Presumably this:

workaround due to client_name=localhost when sent from Postfix.

Would not result in localhost if sent from a separate container? (like the postscreen.bats tests).

I'm note sure why the test fails now though, since all we're doing is using nc to send to Postgrey directly:

https://github.com/docker-mailserver/docker-mailserver/blob/0889b0ff063a37b482113a684d934e4bd728a33c/test/test-files/nc_templates/postgrey_whitelist.txt

This is reliant upon:

https://github.com/docker-mailserver/docker-mailserver/blob/0889b0ff063a37b482113a684d934e4bd728a33c/test/config/whitelist_clients.local

While the test-case after that passes has relatively the same configs:

https://github.com/docker-mailserver/docker-mailserver/blob/0889b0ff063a37b482113a684d934e4bd728a33c/test/test-files/nc_templates/postgrey_whitelist_recipients.txt

https://github.com/docker-mailserver/docker-mailserver/blob/0889b0ff063a37b482113a684d934e4bd728a33c/test/config/whitelist_recipients

---

It may be affected by prior test-cases, where your changes aren't producing the same interaction anymore? (you could verify by bringing back the removed test helper method from that file)

I'd first tail the log to verify that it's actually getting the content we want to check for.

It's probably unlikely, but perhaps the blank line you dropped affects the outcome with nc too? 🤷‍♂️

[georglauterbach]

The remaining content is fed into swaks command via --data and STDIN (a filename path should work just as well btw).

I tried this, many times, with different configurations, but it never worked, so I used the STDIN approach. Might be worth adding a comment.

It's probably unlikely, but perhaps the blank line you dropped affects the outcome with nc too? 🤷‍♂️

This was literally it, oh dear...

[georglauterbach]

We're now seeing this error in postscreen.bats:

$ make clean generate-accounts test/postscreen
postscreen.bats
 ✓ [Postscreen] should fail send when talking out of turn [195]
 ✗ [Postscreen] should successfully pass postscreen and get postfix greeting message (respecting postscreen_greet_wait time) [6408]
   (from function `assert_success' in file test/test_helper/bats-assert/src/assert_success.bash, line 42,
    in test file test/tests/parallel/set1/spam_virus/postscreen.bats, line 55)
     `assert_success' failed
   
   -- command failed --
   status : 24
   output (19 lines):
     === Trying 172.17.0.2:25...
     === Connected to 172.17.0.2.
     <-  220-mail.example.test ESMTP
     <-  220 mail.example.test ESMTP
      -> EHLO mail.external.tld
     <-  250-mail.example.test
     <-  250-PIPELINING
     <-  250-SIZE 10240000
     <-  250-ETRN
     <-  250-ENHANCEDSTATUSCODES
     <-  250-8BITMIME
     <-  250 CHUNKING
      -> MAIL FROM:<user@external.tld>
     <-  250 2.1.0 Ok
      -> RCPT TO:<user1@localhost.localdomain>
     <** 450 4.1.8 <user@external.tld>: Sender address rejected: Domain not found
      -> QUIT
     <-  221 2.0.0 Bye
     === Connection closed with remote host.
   --
   

2 tests, 1 failure in 11 seconds

make: *** [Makefile:70: test/postscreen] Error 1

ref: https://github.com/docker-mailserver/docker-mailserver/actions/runs/7370346547/job/20056634732?pr=3732

[polarathene]

It's probably unlikely, but perhaps the blank line you dropped affects the outcome with nc too? 🤷‍♂️

This was literally it, oh dear...

Interesting that it doesn't seem relevant to other tests that are using similar files missing the blank line.. Could be that your removed line version was lacking the trailing LF (but then editorconfig rule should have caught it).

---

We're now seeing this error in postscreen.bats

You've changed the From: data header, but I thought the _send_email() default would override that (user@external.tld, same as the original MAIL FROM: sender address). (EDIT: annnnd it clearly does as the log shows)

https://github.com/docker-mailserver/docker-mailserver/pull/3732/files#r1438741770

The test was previously handled this way:

# NOTE: Sometimes fails on first attempt (trying too soon?),
# Instead of a `run` + asserting partial, Using repeat + internal grep match:
_repeat_until_success_or_timeout 10 _should_wait_turn_speaking_smtp \
    "${CONTAINER2_NAME}" \
    "${CONTAINER1_IP}" \
    '/tmp/docker-mailserver-test/email-templates/postscreen.txt' \
    '220 mail.example.test ESMTP'

While now it is:

# Send from mail client container (CONTAINER2_NAME) to DMS server container (CONTAINER1_NAME):
CONTAINER_NAME=${CONTAINER2_NAME} _send_email --server "${CONTAINER1_IP}" 'postscreen'
assert_success

The pass condition has changed. Originally we only cared about that first response from DMS as success, now you're expecting the full transaction to be successful. Looks like we were working around the issue 😝

-> MAIL FROM:<user@external.tld>
<-  250 2.1.0 Ok
-> RCPT TO:<user1@localhost.localdomain>
<** 450 4.1.8 <user@external.tld>: Sender address rejected: Domain not found

Probably making a DNS lookup, and .tld is not a valid TLD. No worries we can fix that, but it may expect the IP to match the client container IP too after that? (and likewise check for SPF, which we don't support atm until switching test-suite to compose.yaml)

This failure condition will be due to the sender restriction, specifically reject_unknown_sender_domain.

We recently had an issue related to the reject_unknown_sender_domain restriction (last portion of the quote, although the earlier portion is relevant context, related to the quoted failure snippet above):

smtpd_sender_restrictions may be the one responsible, it becomes applicable when mail arrives due to smtpd_delay_reject=yes (default) I think:

Wait until the RCPT TO command before evaluating $smtpd_client_restrictions, $smtpd_helo_restrictions and $smtpd_sender_restrictions

SMTP command specific restrictions described under smtpd_recipient_restrictions. When recipient restrictions are listed under smtpd_sender_restrictions, they have effect only with smtpd_delay_reject = yes, so that $smtpd_sender_restrictions is evaluated at the time of the RCPT TO command.

reject_unknown_sender_domain
Reject the request when Postfix is not the final destination for the sender address, and the MAIL FROM domain has
1) no DNS MX and no DNS A record, or
2) a malformed MX record such as a record with a zero-length MX hostname.

This highlights a reason for why I want tests that send mail to ideally be handled from a separate container and MTA (HELO/EHLO) + sender domain to better test in conditions that reflect actual deployment :)

a) Without throwing CoreDNS into the mix, I think we could use --add-host 'external.test:1.2.3.4' as another arg for CONTAINER1_NAME, but you'd need the IP in advance 😖
b) Likewise, using --hostname on CONTAINER2_NAME could help, but Dockers DNS only links containers this way on a custom network not the default docker0 bridge used (legacy).

Both are simple to manage with compose.yaml, while a probably requires b (custom network) anyway. You can refer to LDAP or the current OAuth PR for how we approach that.

Alternatively, less correct solutions:

• Keep the current workaround, assert partial on the output like before instead of success of the transaction.
• Use a different workaround:
  • Adjust the sender address to be example.test like rspamd tests do, and the restriction will ignore it as the sender address belongs to the DMS container receiving the mail.
  • Send from within the DMS container, instead of a separate client container (like many other tests do). This is more preferable of the two, since it would be consistent with other tests and can be addressed later with the next test-suite refactor phase (compose.yaml, after all tests migrated from legacy helpers)

[polarathene]

Reviewing the next two commits now (removal of SMTP commands from email "templates" + test adjustments), this'll take me a while.

I'll provide a new checklist summary after that, so we can quickly get this review wrapped up and PR merged 🥳

[polarathene]

Sorry took a bit longer than expected. Not complete but most of it is covered.

No time tonight to summarize unfortunately. Some of the feedback is just contextual notes without any action required in this PR.

[georglauterbach]

I hope I addressed all of your very valuable feedback now. I have opened an issue to track the follow-up changes to this PR (that ought to be easier to come up with, and review - fingers crossed). From 1a65775 onward, review commit by commit; I grouped the changes. I also resolved the conversations that I addressed, and I left the feedback unresolved that can (and will) be resolved only later.

[georglauterbach]

Oh, I thought I fixed everything. The last issue in the test about disabled ClamAV/SA could be resolved by adding --env PERMIT_DOCKER=container to ´CUSTOM_SETUP_ARGUMENTS` (I just tested it). You can directly apply this @polarathene if you think this is fine :)

[polarathene]

Looks to be in a pretty good state now 👍

Didn't have as much time today, but will return tomorrow. Should be good for @casperklein to weigh in if he wants to review.

[georglauterbach]

@casperklein do you want to review as well or is it okay if we merge with @polarathene's review alone?

[casperklein]

I can't say when I'll have time for this. So if tests are passing and you both think the changes are fine, go ahead and merge 👍

[polarathene]

I'll apply these changes and merge.

• SMTP Smuggling fix PR will be tested against this first, then merged.
• I'll go over existing PRs open that need to be rebased and adjusted to accommodate changes here.
• Debian Bookworm base image update PR will get same treatment, not sure if I'll merge that before other pending PRs 😅 Whatever is less friction.
• I'll then go back to my notes/feedback on this PR to raise/document future work related to what was discussed here 👍