fix: order of DKIM/DMARC milters matters#3082
fix: order of DKIM/DMARC milters matters#3082georglauterbach merged 2 commits intodocker-mailserver:masterfrom
Conversation
| function _setup_dkim_dmarc | ||
| { | ||
| if [[ ${ENABLE_OPENDMARC} -eq 1 ]] | ||
| if [[ ${ENABLE_OPENDKIM} -eq 1 ]] |
There was a problem hiding this comment.
it works for our servers (opendkim + opendmarc enabled) but the return 0 is lost here.
which syntactically was useless just preventing an error in the current state of the code (after lot's of refactoring) but made me think why it was there in the first place.
should opendmarc be enabled without opendkim (and policyd-spf) or should there be at least a warning that all mails for domains with DMARC enable are incorrectly rejected if you run opendmarc without policyd-spf or without opendkim?
for a RFC 7489 compliant mail receiver setup if opendmarc is enabled (ENABLE_OPENDMARC), policyd-spf (always enabled) and opendkim (ENABLE_OPENDKIM) must be enabled.
There was a problem hiding this comment.
should opendmarc be enabled without opendkim (and policyd-spf) or should there be at least a warning that all mails for domains with DMARC enable are incorrectly rejected if you run opendmarc without policyd-spf or without opendkim?
There is definitely room for improvements. This PR only aims to fix the current regression. At the moment, I don't have much spare time. So looking into it, will take same weeks.
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
I think this will conflict with a related PR by @georglauterbach
One could also probably just append both milter values to an array and afterwards instead of similar logic for both, just have one edit after both dkim + dmarc options are handled to add enabled milters. That way the existing dmarc first would still keep dmarc first in the array, then dkim (probably worth noting order is important with a comment though).
|
Yeah, I had actually hoped to get my PR merged before, but I'm lacking the reviews 😅 I can take care of the merge conflicts, but I'd really love to merge my PRs then :) I can also add a comment about the order. |
georglauterbach
changed the title
georglauterbach
changed the title
You could probably just squeeze in the fix in that PR technically and we could close this one. No need to swap the order like this PR is doing with moving conditional blocks. Just append to an array and after the two update the config like I said? (although I recall you changing opendkim conditional into an early return guard instead, which I wasn't a fan of to skip some indentation since it wasn't consistent with the related dmarc conditional 😅 ) |
I will just go ahead and merge this one so it's out on
I will resolve the conflicts and adapt your strategy :) This way the work @casperklein has put into the PR is not wasted IMO :) |
I resolved the conflicts and after looking at the code, I decided against the proposed solution of using an array/string that is appended to Postfix's milters at the end due to unnecessarry complexity it would add IMO. The current solution is absolutely fine when proper comments are in place (which I also added). Moreover, I added a warning to the log that enabling OpenDMARC but disabling OpenDKIM is not compliant with RFC 7489 (see #3082 (comment)).
4 participants
Description
This PR fixes a regression introduced with #3016, where the postfix setting
smtpd_milterschanged the order of the valuesfrom
smtpd_milters = $dkim_milter $dmarc_milterto
smtpd_milters = $dmarc_milter $dkim_milter.The changed order led to this kind of DMARC failures:
Details
I switched back the order of the two conditions:
[[ ${ENABLE_OPENDMARC} -eq 1 ]][[ ${ENABLE_OPENDKIM} -eq 1 ]]Type of change
Checklist:
docs/)