Skip to content
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

New: Native DKIM capability for emails #16421

Open
wants to merge 3 commits into
base: 3.x
Choose a base branch
from
Open

Conversation

krisznet
Copy link

What does it do?

Adding native DKIM capabilities to MODX without creating a custom email script. After setting up the correct values in system settings, all mail sent by MODx will be signed.

Why is it needed?

More and more companies moving to DKIM and also using 2-factor authentication for email accounts, so even secure SMTP is not an option for those. SPF record is often not enough to be able to deliver email reliably.

How to test

Regular mail and SMTP should work like before. If you add DKIM details in settings the Emailer should use it and the email should be signed with the correct key.

Related issue(s)/PR(s)

Resolves #16396

@zaigham
Copy link
Contributor

zaigham commented Apr 12, 2023

@krisznet You just need to commit translations in core/lexicon/en/* folder, the translations will be handled via Crowdin.

@Bruno17
Copy link
Contributor

Bruno17 commented Apr 12, 2023

Had that same issue and got this answers on slack:
https://modxcommunity.slack.com/archives/C042Q4YS7/p1681303237481139?thread_ts=1678262173.500949&cid=C042Q4YS7
I could solve it then by setting up SMTP and DNS correctly

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.59%. Comparing base (69a7d6d) to head (5f138ae).
Report is 134 commits behind head on 3.x.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16421      +/-   ##
============================================
- Coverage     21.75%   21.59%   -0.17%     
- Complexity    10478    10566      +88     
============================================
  Files           561      561              
  Lines         31590    31946     +356     
============================================
+ Hits           6872     6898      +26     
- Misses        24718    25048     +330     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@krisznet
Copy link
Author

krisznet commented Apr 12, 2023

Had that same issue and got this answers on slack: https://modxcommunity.slack.com/archives/C042Q4YS7/p1681303237481139?thread_ts=1678262173.500949&cid=C042Q4YS7 I could solve it then by setting up SMTP and DNS correctly

@Bruno17 The problem is that some company has strict email policy and 2FA for emails, so the SMTP not an option. And it is 2023, DKIM and DMARC is essential.

@Omeryl
Copy link
Member

Omeryl commented Apr 13, 2023

Huh? In what case is the company completely fine with you setting a random DKIM key in the DNS, but won't provide you with SMTP credentials for an account authorized to send for the domain? Storing these in a web application is a terrifying prospect, the mail server is a much better place to be signing emails. Furthermore, if an email fails to send one time with mail(), it won't be retried. Mail servers retry every so often on an exponential back off. This is better for the internet, sometimes receiving mail servers go down (even Google!).

@krisznet
Copy link
Author

Huh? In what case is the company completely fine with you setting a random DKIM key in the DNS, but won't provide you with SMTP credentials for an account authorized to send for the domain? Storing these in a web application is a terrifying prospect, the mail server is a much better place to be signing emails. Furthermore, if an email fails to send one time with mail(), it won't be retried. Mail servers retry every so often on an exponential back off. This is better for the internet, sometimes receiving mail servers go down (even Google!).

@Omeryl I agree that SMTP is more reliable but sometimes you can't use it because of the technical limitations. If 2FA is required for emails SMTP is not an option. You can set up DKIM selectors, so you can just create a key solely for this purpose. If it is compromised, you can delete it and set up a new one. The private key can be stored in a file or DB and can be encrypted. If your database and/or filesystem are compromised, you are already in big trouble.

@JoshuaLuckers
Copy link
Contributor

@opengeek @Mark-H i would love to get your opinion on this, to me it seems like a great addition even if it’s not the preferred solution (to have it as option in the CMS).

Copy link
Member

@opengeek opengeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All lexicon entries other than the en/ sources must be removed from this PR.

I'm also not clear on how this works by just setting some options.

@Bruno17
Copy link
Contributor

Bruno17 commented Apr 18, 2023

my idea was to add a plugin event to modPHPMailer::send
'onMailerSend' or something like that, where we could hook in and add for example this DKIM stuff, if needed.

@opengeek
Copy link
Member

my idea was to add a plugin event to modPHPMailer::send 'onMailerSend' or something like that, where we could hook in and add for example this DKIM stuff, if needed.

I guess my question is why wouldn't this be settings of an extra that installs this plugin?

@Bruno17
Copy link
Contributor

Bruno17 commented Apr 18, 2023

my idea was to add a plugin event to modPHPMailer::send 'onMailerSend' or something like that, where we could hook in and add for example this DKIM stuff, if needed.

I guess my question is why wouldn't this be settings of an extra that installs this plugin?

Yeah. But thats currently not possible, because there isn't an event like that.

@opengeek
Copy link
Member

my idea was to add a plugin event to modPHPMailer::send 'onMailerSend' or something like that, where we could hook in and add for example this DKIM stuff, if needed.

I guess my question is why wouldn't this be settings of an extra that installs this plugin?

Yeah. But thats currently not possible, because there isn't an event like that.

But this PR does not add an event. I guess I'm still confused how this is supposed to work as is in the PR.

@Bruno17
Copy link
Contributor

Bruno17 commented Apr 18, 2023

my idea was to add a plugin event to modPHPMailer::send 'onMailerSend' or something like that, where we could hook in and add for example this DKIM stuff, if needed.

I guess my question is why wouldn't this be settings of an extra that installs this plugin?

Yeah. But thats currently not possible, because there isn't an event like that.

But this PR does not add an event. I guess I'm still confused how this is supposed to work as is in the PR.

oh.. its not my PR. The Opener has added his new systemsettings as default attributes into modMail
https://github.com/modxcms/revolution/pull/16421/files#diff-919cb7414fac156184fcd82007638be9e5946883c0348c3c6957ea355060d390

@opengeek
Copy link
Member

From what I can see, all of these "settings" are part of modPHPMailer. modMail is an interface that is intended to define common settings for any mailer implementation. In order to set the modPHPMailer settings, those entries can be manually added to system settings when needed, and they will just work. So I'm not sure of the actual necessity of this core change.

@krisznet
Copy link
Author

my idea was to add a plugin event to modPHPMailer::send 'onMailerSend' or something like that, where we could hook in and add for example this DKIM stuff, if needed.

I guess my question is why wouldn't this be settings of an extra that installs this plugin?

Yeah. But thats currently not possible, because there isn't an event like that.

But this PR does not add an event. I guess I'm still confused how this is supposed to work as is in the PR.

oh.. its not my PR. The Opener has added his new systemsettings as default attributes into modMail https://github.com/modxcms/revolution/pull/16421/files#diff-919cb7414fac156184fcd82007638be9e5946883c0348c3c6957ea355060d390

Hey guys, if you read the original PR and the Issue it is described clearly. DKIM capability is already in modPHPMailer but you can't use it through modMail with any existing extension like FormIt. I added the settings and also modified modMail, so it uses those settings. That is it. It is much easier than creating a plugin and it is not much code and also not a difficult change.

@krisznet
Copy link
Author

From what I can see, all of these "settings" are part of modPHPMailer. modMail is an interface that is intended to define common settings for any mailer implementation. In order to set the modPHPMailer settings, those entries can be manually added to system settings when needed, and they will just work. So I'm not sure of the actual necessity of this core change.

@opengeek You are right but not 100% right. You can use DKIM if you write a custom snippet but can't use it with emails sent by modX itself or an extra like FormIt.

@opengeek
Copy link
Member

From what I can see, all of these "settings" are part of modPHPMailer. modMail is an interface that is intended to define common settings for any mailer implementation. In order to set the modPHPMailer settings, those entries can be manually added to system settings when needed, and they will just work. So I'm not sure of the actual necessity of this core change.

@opengeek You are right but not 100% right. You can use DKIM if you write a custom snippet but can't use it with emails sent by modX itself or an extra like FormIt.

I see now. If the default attributes are not set, the initialization does not look for those attributes in the settings. Let me think on this. It would, in my opinion, be better if we modified the initialization of modPHPMailer to look for attributes from MODX settings in all cases. Otherwise, we will run into other options in this mailer—or in any mailer implementation—that would have to manually be added to the modMail interface. And this is not really the job of an interface.

@krisznet
Copy link
Author

From what I can see, all of these "settings" are part of modPHPMailer. modMail is an interface that is intended to define common settings for any mailer implementation. In order to set the modPHPMailer settings, those entries can be manually added to system settings when needed, and they will just work. So I'm not sure of the actual necessity of this core change.

@opengeek You are right but not 100% right. You can use DKIM if you write a custom snippet but can't use it with emails sent by modX itself or an extra like FormIt.

I see now. If the default attributes are not set, the initialization does not look for those attributes in the settings. Let me think on this. It would, in my opinion, be better if we modified the initialization of modPHPMailer to look for attributes from MODX settings in all cases. Otherwise, we will run into other options in this mailer—or in any mailer implementation—that would have to manually be added to the modMail interface. And this is not really the job of an interface.

All other settings are in the modMail interface as well like SMTP settings and charset, etc. That is why I put it there.

@cla-bot

This comment was marked as resolved.

@krisznet krisznet requested a review from opengeek April 18, 2023 23:55
@krisznet
Copy link
Author

All lexicon entries other than the en/ sources must be removed from this PR.

I'm also not clear on how this works by just setting some options.

@opengeek I removed the unwanted lexicon changes.

@opengeek

This comment was marked as resolved.

@krisznet

This comment was marked as resolved.

@opengeek

This comment was marked as resolved.

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Apr 19, 2023
@cla-bot

This comment was marked as resolved.

@JoshuaLuckers JoshuaLuckers added the pr/review-needed Pull request requires review and testing. label Apr 20, 2023
opengeek
opengeek previously approved these changes Apr 25, 2023
@opengeek opengeek added this to the v3.1.0 milestone Apr 27, 2023
Copy link
Contributor

@JoshuaLuckers JoshuaLuckers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mark-H @opengeek mail_dkim_privatekeyfile, mail_dkim_privatekeystring and mail_dkim_passphrase should also be added to the list of things we need to unset in the following two places right?

unset($c['password'], $c['username'], $c['mail_smtp_pass'], $c['mail_smtp_user'], $c['proxy_password'], $c['proxy_username'], $c['connections'], $c['connection_init'], $c['connection_mutable'], $c['dbname'], $c['database'], $c['table_prefix'], $c['driverOptions'], $c['dsn'], $c['session_name'], $c['assets_path'], $c['base_path'], $c['cache_path'], $c['connectors_path'], $c['core_path'], $c['friendly_alias_translit_class_path'], $c['manager_path'], $c['processors_path']);

unset($c['password'], $c['username'], $c['mail_smtp_pass'], $c['mail_smtp_user'], $c['proxy_password'], $c['proxy_username'], $c['connections'], $c['connection_init'], $c['connection_mutable'], $c['dbname'], $c['database'], $c['table_prefix'], $c['driverOptions'], $c['dsn'], $c['session_name'], $c['cache_path'], $c['connectors_path'], $c['friendly_alias_translit_class_path'], $c['manager_path'], $c['processors_path']);

_build/data/transport.core.system_settings.php Outdated Show resolved Hide resolved
@Mark-H Mark-H added blocked Active participation around the pull request or issue required. Consensus is not reached. and removed pr/review-needed Pull request requires review and testing. labels Feb 10, 2024
Co-authored-by: Joshua Lückers - Jacobsen <joshualuckers@me.com>
@Mark-H Mark-H added pr/review-needed Pull request requires review and testing. and removed blocked Active participation around the pull request or issue required. Consensus is not reached. labels Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable DKIM capabilities by default via settings
7 participants