feat: Postfix permit DSN (Delivery Status Notification) only on authenticated ports (465 + 587) #3572
Conversation
There was a problem hiding this comment.
Thanks for spotting this issue and contributing a fix 😁 (and with tests! 🥳 )
You can address the feedback or wait for input from other maintainers which might approve of dropping the ENV.
Would it make sense to restrict to authenticated by default for v13? @casperklein @georglauterbach
If there is little concern for switching to this as default, we can probably drop the need for supporting an ENV with docs, (test is still relevant). Assuming the need to adjust the setting is niche, it should be supported well via our postfix-master.cf / postfix-main.cf overrides support, should anyone raise an issue about it that could be added to a FAQ entry?
Dockerfile
Outdated
| @@ -201,7 +201,7 @@ RUN echo 'Reason_Message = Message {rejectdefer} due to: {spf}.' >>/etc/postfix- | |||
|
|
|||
| COPY target/fetchmail/fetchmailrc /etc/fetchmailrc_general | |||
| COPY target/getmail/getmailrc /etc/getmailrc_general | |||
| COPY target/postfix/main.cf target/postfix/master.cf /etc/postfix/ | |||
| COPY target/postfix/main.cf target/postfix/master.cf target/postfix/esmtp_access /etc/postfix/ | |||
There was a problem hiding this comment.
Optional
If this config file is kept, append the .cidr suffix. Convention for postfix tables in this project that we supply appears to use the table type as the suffix?
Although for LDAP it is presently .cf 😕so I'd be fine with a common .cf too.
I know that an extension isn't required for the file, and it's fine to justify avoiding one.
There was a problem hiding this comment.
Of course, as you say, the change is only visual.
My idea was to use the same name as it's called in the Postfix documentation.
There was a problem hiding this comment.
Good point; I'd like to see the .cidr suffix as well :)
| EHLO mail | ||
| AUTH PLAIN YWRkZWRAbG9jYWxob3N0LmxvY2FsZG9tYWluAGFkZGVkQGxvY2FsaG9zdC5sb2NhbGRvbWFpbgBteXBhc3N3b3Jk | ||
| MAIL FROM: added@localhost.localdomain | ||
| RCPT TO: user1@localhost.localdomain NOTIFY=success,failure |
There was a problem hiding this comment.
Am I right to understand the difference from regular authenticated / unauthenticated templates is effectively only the NOTIFY=success,failure addition?
There was a problem hiding this comment.
Yes, NOTIFY=... should be the only difference here.
| _run_in_container setup email add 'added@localhost.localdomain' 'mypassword' | ||
| assert_success | ||
| _wait_until_change_detection_event_completes |
There was a problem hiding this comment.
Just so you're aware, the tests should provide some default accounts already setup, you could simplify here by using the account there for authenticating with.
That'd remove hte need for the change-detection helper too.
There was a problem hiding this comment.
Well, thank you very much, if only I had found it sooner.
I was unaware of any users other than user1.
Reference
Postfix docs on Backscatter briefly cite a DSN issue with Outlook 2003.
Related DSN RFCs for reference: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Could either @casperklein or @georglauterbach chime in (or express an intention to when time permits), to provide your input for this contribution please? My review notes where your opinion is requested. If no activity is received in a weeks time from either of you, I'll assume you trust me to make the call myself so that the contributor can address the feedback and the PR can be merged. |
There was a problem hiding this comment.
Very nice PR! Just a few touchups here and there. I mostly agree with @polarathene on the changes and added some feedback.
I highly appreciate the effort, and this is something we definitely need to take care of (since it may impact security).
I have only limited time to spare on this; I hope you excuse my superficial review. I trust @polarathene 100% with this PR though, so you needn't wait for my review to merge this PR after feedback has been applied.
Dockerfile
Outdated
| @@ -201,7 +201,7 @@ RUN echo 'Reason_Message = Message {rejectdefer} due to: {spf}.' >>/etc/postfix- | |||
|
|
|||
| COPY target/fetchmail/fetchmailrc /etc/fetchmailrc_general | |||
| COPY target/getmail/getmailrc /etc/getmailrc_general | |||
| COPY target/postfix/main.cf target/postfix/master.cf /etc/postfix/ | |||
| COPY target/postfix/main.cf target/postfix/master.cf target/postfix/esmtp_access /etc/postfix/ | |||
There was a problem hiding this comment.
Good point; I'd like to see the .cidr suffix as well :)
There was a problem hiding this comment.
If @casperklein later chimes in with input to approach this differently, I'll handle those changes for you.
Please make the following changes.
NOTE: You do not need to clean-up the commit history, keeping the existing commit history is fine as we apply a squash merge commit on approval. Slight benefit if we need to revert to prior revision of your contribution 👍
-
Update
target/postfix/main.cfto includesmtpd_discard_ehlo_keywords = silent-discard, dsn.- It will be the new default and enforced.
git blameshould be sufficient for now to reference this PR for more information on the setting/decision if anyone needs to (future maintainers / contributors).
-
Update
target/postfix/master.cfto include-o smtpd_discard_ehlo_keywords=for both submission(s) services. -
Update the tests based on review feedback to simplify them:
user1account instead of creatingadded+ remove change detection usage.- No
POSTFIX_DSNENV necessary. - While only the 2nd test case is needed, I'd like to keep the 1st and 3rd for robustness / reference.
- This can be supported by creating a
dsnfolder in the test config directory, which can contain thepostfix-main.cfandpostfix-master.cfoverride files (described in changelog entry below). - The 1st + 3rd test cases can selectively copy over the needed file which should be after
_init_with_defaultsline but before_common_container_setupline.
- This can be supported by creating a
-
Revert changes to:
Dockerfilemailserver.envpostfix.sh+variables-stack.sh- Delete
esmtp_access
-
dsn-authenticated.txt- Replace theAUTH PLAINline with theAUTH LOGINlines foruser1account. Swap theaddeduser inMAIL FROMandFromlines touser1, it's still valid to login asuser1and have their address as the sender and recipient. -
Document your change in
CHANGELOG.mdunder the "Unreleased => Breaking" section with:- Postfix now defaults to supporting DSNs (_[Delivery Status Notifications](https://github.com/docker-mailserver/docker-mailserver/pull/3572#issuecomment-1751880574)_) only for authenticated users. This is a security measure to reduce spammer abuse of your DMS instance as a backscatter source. - If you need to modify this change, please let us know by opening an issue / discussion. - You can [opt-out (_enable DSNs_) via the `postfix-main.cf` override support](https://docker-mailserver.github.io/docker-mailserver/v12.1/config/advanced/override-defaults/postfix/) using the contents: `smtpd_discard_ehlo_keywords =`. - Likewise for authenticated users, the submission(s) ports (465 + 587) are configured internally via `master.cf` to keep DSNs enabled (_since authentication protects from abuse_). If necessary, DSNs for authenticated users can be disabled via the `postfix-master.cf` override with the following contents: ``` submission/inet/smtpd_discard_ehlo_keywords = silent-discard, dsn submissions/inet/smtpd_discard_ehlo_keywords = silent-discard, dsn ```
Rendered preview:
- Postfix now defaults to supporting DSNs (Delivery Status Notifications) only for authenticated users. This is a security measure to reduce spammer abuse of your DMS instance as a backscatter source.
-
If you need to modify this change, please let us know by opening an issue / discussion.
-
You can opt-out (enable DSNs) via the
postfix-main.cfoverride support using the contents:smtpd_discard_ehlo_keywords =. -
Likewise for authenticated users, the submission(s) ports (465 + 587) are configured internally via
master.cfto keep DSNs enabled (since authentication protects from abuse).If necessary, DSNs for authenticated users can be disabled via the
postfix-master.cfoverride with the following contents:submission/inet/smtpd_discard_ehlo_keywords = silent-discard, dsn submissions/inet/smtpd_discard_ehlo_keywords = silent-discard, dsn
-
- Postfix now defaults to supporting DSNs (Delivery Status Notifications) only for authenticated users. This is a security measure to reduce spammer abuse of your DMS instance as a backscatter source.
Prior feedback relevance (obsoleted by above checklist)
Feedback to apply:
- Remove
esmtp_accessfile in favor ofsmtpd_discard_ehlo_keywords = silent-discard, dsn. Users can easily adjust this via our supported Postfix overrides. Additional flexibility like this file (and it's equivalent*_address_mapssetting) offers will be considered should users actually request a need for it.
Feedback to ignore:
.cidrextension for CIDR table config (no explicit demand anticipated, avoid maintenance burden).- ENV docs and feature logic (level of configuration not likely needed).
postconf -Pinstead ofsedto adjust overrides inmaster.cf(main.cfwill use thesmtpd_discard_ehlo_keywordssetting directly as a new default).
Thanks for the review! No worries, we are all busy.
Thank you for the detailed explanation. I actually like this approach better, but initially avoided making it the default because it might break some configurations.
The only thing I had to do differently here was that the entries in After changing this, the tests worked again. |
Good that we decided to test the other cases then 😁 That wasn't something I thought would be an issue though, I'll have to look at our script for it or update our related docs to address that gotcha. I'll return to this PR later today, my system is acting up and needs my attention to resolve (WSL2 just ate a bunch of disk to the point I'm out of disk space + RAM and is unresponsive to shutdown, I can't seem to terminate the related process eating 10GB of memory). |
|
Apologies for the delay!
Attempting to apply the setting with spaces (and double quote wrapped) via So doesn't seem like we can do anything about that 😅 Looks like all the feedback was addressed, so LGTM 👍 Thank you for the excellent contribution to DMS! 😎 |
polarathene
changed the title
POSTFIX_DSN= / configure how Postfix handles Delivery Status Notification (DSN)
polarathene
added
the
kind/improvement
|
Test cases 2 and 3 are failing when first sending the unauthenticated mail ( I lack time to investigate right now, but it failing at this assertion in the helper: docker-mailserver/test/helper/sending.bash Lines 32 to 33 in 811a769 The first test case passes, despite calling this method as well to send the unauthenticated template. |
| load "${REPOSITORY_ROOT}/test/helper/common" | ||
|
|
||
| BATS_TEST_NAME_PREFIX='[DSN] ' | ||
| CONTAINER_NAME='dms-test_dsn' |
There was a problem hiding this comment.
At a glance, I think the issue could be from here.
- The test goes straight into test cases, no
setup_file(). - The
teardown()default should handle this inbetween without issue though, so a sharedCONTAINER_NAMEshouldn't be an issue due to sequential test run 🤷♂️ (parallel is only relevant to separate test files concurrently, not their individual test cases within) - But given that the 2nd and 3rd test cases are the ones that fail, it does seem like the containers may have failed to start correctly, thus the attempt to send mail to port 25 fails.
|
I have to say this is very strange, the failures seem to be completely random. This was not the case with the old configuration (using |
That's interesting. I remember a recent PR a test case was revised for an error log check that was also proving to be spotty in behaviour unrelated to the PR:
I don't seem to have mentioned it explicitly there, but I think it was related to this report for sieve errors. Basically there is a non-deterministic behaviour with file creation during startup, and if that creates two related files with a timestamp at the exact same time, instead of one expecting to be slightly newer, then it gets caught by the error log check added in that other test. Doesn't seem to trip up our CI so far though AFAIK. This might be related, hard to say. Related, checking successful mail delivery: #3554 @georglauterbach you've worked on this helper IIRC, any advice? I assume we could maybe @allddd for the time being I think since this is the only test triggering the failure for that helper, we'll need to block merging the PR. It's not unusual for tests to be flimsy for some reason, but since it's for a common helper method used elsewhere something isn't right 😅 If I find time I'll look into it. My process is pretty simple, I just add a Success or failure in this case should be fine as we should be able to use the output to |
There was a problem hiding this comment.
Overall LGTM 👍🏼 Nice PR!
I have added review feedback that you can apply, but the test needs a bit more work. I know, annoying. Please have a look at this test for Rspamd that I wrote. Moreover, the Postscreen test should provide you with a structure of how a test with multiple containers should look like.
I advocate for using setup_file here, using export CONTAINER_NAME and then setting it on-demand. This provides a unified place for starting containers. Moreover, we need to pay attention to using _wait_for_service postfix and _wait_for_smtp_port_in_container which could be important for this test.
With my proposals when using export MAIL_IDX=$(_send_email_and_get_id ...) you'd also gain the advantage of better checks against logs for a specific email.
If you have questions about this, just ping me :)
|
|
||
| _init_with_defaults | ||
| # Unset `smtpd_discard_ehlo_keywords` to allow DSNs by default on any `smtpd` service: | ||
| mv "${TEST_TMP_CONFIG}/dsn/postfix-main.cf" "${TEST_TMP_CONFIG}/postfix-main.cf" |
There was a problem hiding this comment.
Why is mv and not cp being used here?
| mv "${TEST_TMP_CONFIG}/dsn/postfix-main.cf" "${TEST_TMP_CONFIG}/postfix-main.cf" | |
| cp "${TEST_TMP_CONFIG}/dsn/postfix-main.cf" "${TEST_TMP_CONFIG}/postfix-main.cf" |
There was a problem hiding this comment.
Normally we use mv as _init_with_defaults would copy the config dir into a temporary location for the container being setup (directory with the container name).
Since that is meant to be isolated, the pattern here is just moving from the dsn directory to the root test dir, as this whole directory is mounted into /tmp/docker-mailserver.
Not the greatest solution, but that's how it's been for quite some time. I'm not too bothered as it works, and the eventual plan is to switch to compose.yaml with :ro mounts where possible.
mv or cp, doesn't matter much either way 🤷♂️ (mv just communicates the above a bit better)
|
Thanks for the feedback @georglauterbach, it helped me get it stable again. :)
It is indeed nicer this way, especially with multiple containers.
No particular reason, it was used in the example @polarathene sent, so I just copied it.
I've tried to make it look as similar as possible. As for why I used it, I think it was because some of the first tests I looked at also use it.
I think that this actually solved the problem, thank you very much.
I thought this would be a great option as it would allow me to As we currently send email using Even if this was not the case, I'm not sure how accurate the ID extraction would be, as I send multiple emails and DSNs in the same container. |
Good spotting, that'd be from an earlier iteration when this approach was only focused on ENV adjustment, but now just appends args to the
Thanks @georglauterbach 🙏 I've not been working on tests for a while that I thought this was implicit, but we don't always need it to minimize container startup time, so thanks for catching that 👍 |
Description
Checking the Postifx configuration, I noticed that there were no settings related to DSN.
In my opinion, it is always a good idea to partially disable this, as it can have some unexpected (negative) effects.
What I mean by partial disabling is disabling it on port 25, which will stop incoming email from requesting a DSN.
If the server simply sends such notifications/emails to anyone who requests them, this can easily be abused and redirected to any email address by spoofing.
Even if the intention was not to send email back to the spoofed address and it was just normal spam, the backscatter can have an impact on reputation.
I have also noticed that such emails sent from a DMS instance reveal a little too much information.
For example, my tests showed that the email leaked whether an email address was an alias or not, as well as some of the software used on the server.
To ensure that this option works as intended, I have implemented some additional tests.
Apart from that, to make sure it wasn't interfering with anything else, I re-ran all the tests with
POSTFIX_DSN=0,1and2as the default options.This and manual testing showed no issues.
Type of change
Checklist:
docs/)