-
-
Notifications
You must be signed in to change notification settings - Fork 661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix support for DKIM signing when forwarding and aliasing is enabled #2776
Conversation
plugins/dkim_sign.js
Outdated
@@ -355,7 +355,8 @@ exports.get_sender_domain = function (connection) { | |||
} | |||
} | |||
|
|||
if (!txn.header) return domain; | |||
// In case of forwarding, only use the Envelope | |||
if (!txn.header || txn.notes.forward) return domain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a compound conditional? Does something else set txn.notes.forward
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alias forwarding plugins sets them (at least mine does). See here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that, but why make that a compound conditional? (those tend to increase in complexity over time.). Why not simply do this?
if (txn.notes.forward) return domain; // forwarding, only use the Envelope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that's possible if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that's possible if you prefer.
Simpler is better, and that's simpler. Anyone can glance at that and know exactly when it'll execute.
Forgive me if any of these are dumb questions. You're the most recent person to think through this issue so I'm asking now. There's several cases here and I'd like someone to be confident that this change does The Right Thing in all of these cases:
|
As previously.
If you forward a mail without any modification, you don't need to sign it since any previous signature remains valid. You might need to use SRS if the source domain is using SPF.
Aliasing only means you are the recipient SMTP server (typically, you received
This PR, no need to explain further. |
This is a very frequent use of a SMTP server. The typical description for a MTA like Haraka/Postfix/Sendmail/Exim serving in this role is a Message Submission Agent. You keep equating a mail server with a domain and that's too narrow. Many (most?) MTAs serve more than one domain. My own Haraka instance accepts and relays mail for many domains. |
Ok. If you forward but don't set |
I don't. I have some aliases I want to move into Haraka but have yet to do it. |
Here's the adapted code. To answer the question about forwarding only, in case you are using a Haraka as a MSA, if I understand correctly, the client should provide a If you intend to implement aliasing please check the 2 tutorials I've written here as it is easier to copy & modify than trying to understand each plugin specificity (DKIM_sign is a real pain to set up right since it implies so many external steps). |
I've looked at the adapted code, looks the same as before. Am I missing something? |
Ahem*... Cough... Well sorry, forgot to commit my changes before amending. Should be ok now. |
This partially solve DMARC failing and forwarded message considered as spam.
Fixes #2774
The other requirement are out of haraka's repository. One must enable SRS plugin (or use my alias_forward plugin which does both).