Skip to content

Conversation

@SorinGFS
Copy link
Contributor

@SorinGFS SorinGFS commented Oct 17, 2020

What does it do?

Basically a check for existance of the dkim-private.pem file is performed, and if the file exists in the project root folder it will be used in provider option, except the case where different options are passed for provider.

Now, if user provides that key and everything is correctly set the result will be dkim=pass (1024-bit key) or (2048-bit key) in validations made by receiving email providers. Its tested!

More details I will provide in the same forum topic where this issue was started : https://forum.strapi.io/t/unsolved-problem-emails-goes-to-spam/512/7?u=soringfs

Why is it needed?

DKIM pass at receiving email providers

Related issue(s)/PR(s)

N/A

Basically a check for existance of the `dkim-private.pem` file is performed, and if the file exists in the project root folder it will be used in provider option, except the case where different options are passed for provider.

Now, if user provides that key and everything is correctly set the result will be `dkim=pass (1024-bit key) or (2048-bit key)` in validations made by receiving email providers. Its tested!

More details I will provide in the same forum topic where this issue was started : https://forum.strapi.io/t/unsolved-problem-emails-goes-to-spam/512/7?u=soringfs
@SorinGFS SorinGFS requested a review from a team October 17, 2020 19:18
@codecov
Copy link

codecov bot commented Oct 17, 2020

Codecov Report

Merging #8376 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8376   +/-   ##
=======================================
  Coverage   33.19%   33.19%           
=======================================
  Files        1220     1220           
  Lines       13618    13618           
  Branches     1357     1357           
=======================================
  Hits         4521     4521           
  Misses       8212     8212           
  Partials      885      885           
Flag Coverage Δ
#front 24.72% <ø> (ø)
#unit 54.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fad9fda...55c3635. Read the comment docs.

Copy link
Member

@derrickmehaffy derrickmehaffy left a comment

Choose a reason for hiding this comment

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

Can you add some of the extra options into the doc README for this provider please?
https://github.com/strapi/strapi/blob/master/packages/strapi-provider-email-sendmail/README.md

@lauriejim
Copy link
Contributor

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/unsolved-problem-emails-goes-to-spam/512/7

@SorinGFS
Copy link
Contributor Author

Can you add some of the extra options into the doc README for this provider please?
https://github.com/strapi/strapi/blob/master/packages/strapi-provider-email-sendmail/README.md

Ofc, I will.

@derrickmehaffy
Copy link
Member

@SorinGFS no need to do another PR: #8378 you can just update your forked branch and we will see it here :)

@SorinGFS
Copy link
Contributor Author

@derrickmehaffy

@SorinGFS no need to do another PR: #8378 you can just update your forked branch and we will see it here :)

Ok, sorry, I updated my fork and deleted the PR #8378 . I used the github interface to update and I'm not yet familiar with this kind of work...

@derrickmehaffy derrickmehaffy self-requested a review October 19, 2020 15:03
@strapi-cla
Copy link

strapi-cla commented Nov 30, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Hi, I understand the goal of this PR but it contradicts the goal of the providerOptions being passthrough options only.

A user can add there dkim config already by simply doing the fs.read in the config/plugins.js or passing it manually if they prefer. I don't see a good reason to add this specific behavior in the provider code. I think this PR could onl be a doc update to give a concrete example on how to load and use a dkim file.

Hope this makes sense

@alexandrebodin alexandrebodin added source: plugin:email issue: enhancement Issue suggesting an enhancement to an existing feature labels Dec 14, 2020
@SorinGFS
Copy link
Contributor Author

SorinGFS commented Dec 14, 2020 via email

@SorinGFS SorinGFS closed this Mar 31, 2021
@derrickmehaffy
Copy link
Member

Hi,

Thank you for your considerations. The reason for adding this behavior is to avoid customizing Strapi while benefiting from dkim validation. Since Sendmail is the default email provider installed by Strapi, users who do not have programming experience but know how to configure dkim could benefit from validation out of the box by simply placing the dkim key into that folder.
The goal of the providerOptions being passthrough options only is not altered since if when one adds its own providerOptions they will prevail.
I see this PR only as a first step to be able to add the dkim key directly in the admin panel.

Hope this makes sense

Understood and I agree but the missing half to this is the DNS settings, this gets into a far more complicated task of configuring DNS txt records for some of the most common DNS providers and doesn't help those without DNS (local projects).

@SorinGFS
Copy link
Contributor Author

Understood and I agree but the missing half to this is the DNS settings, this gets into a far more complicated task of configuring DNS txt records for some of the most common DNS providers and doesn't help those without DNS (local projects).

We can put the DNS settings directly in the registrar, maybe I'm wrong but I don't know any registrar that doesn't let you set any type of DNS record you want. And as I wrote in this solution for dkim pass we don’t neccessarily need to have an email implementation.

Understanding DKIM can be misleading, in fact it is about encrypting a part of the email with a private key, and the server receiving that email looks at the domain that sent that email and the name of the key and asks DNS the public key with which he can decrypt that encrypted portion of the email. If the receiving server cannot complete the procedure that email is marked as spam. Therefore, only those who hold the private key can send emails on behalf of that domain. Using DKIM you can have many domains or subdomains that send emails on behalf of a parent domain, because as I said before it is not necessary to have an email implementation. Myself when I tested that PR I used a new Strapi installation and I had no other email implementation.

PS - why did you wait over 100 days for this replay? I closed this PR and opened another one as @alexandrebodin suggested

A user can add there dkim config already by simply doing the fs.read in the config/plugins.js or passing it manually if they prefer. I don't see a good reason to add this specific behavior in the provider code. I think this PR could onl be a doc update to give a concrete example on how to load and use a dkim file.

@derrickmehaffy
Copy link
Member

server receiving that email looks at the domain that sent that email and the name of the key and asks DNS the public key

This is what I'm referring to, we have no documentation on how to create DNS txt records, we cannot make the assumption that the user has and understands how a domain + DNS works.

What we would need is documentation on how to create the DNS txt record in x number of the most popular DNS providers. This is the exact same reason why none of our documentation has information on getting an SSL certificate. Certain options (Let's Encrypt) have various options for validating a server to issue the certificate, one of which is the DNS-01 Auth system which automatically creates DNS txt records instead of the .well-known.

@derrickmehaffy
Copy link
Member

PS - why did you wait over 100 days for this replay? I closed this PR and opened another one as @alexandrebodin suggested

I had over 2,000 GitHub notifications to read through, and I read all of them.

@SorinGFS
Copy link
Contributor Author

SorinGFS commented Apr 1, 2021

I had over 2,000 GitHub notifications to read through, and I read all of them.

🤒 I'm waiting with patience to see when the quicksand at Strapi will calm down.

@betoflakes
Copy link

Hi, What I need to put in privateKey, the file name or the content of the key?

@SorinGFS
Copy link
Contributor Author

@betoflakes

Hi, What I need to put in privateKey, the file name or the content of the key?

The content of the key.

@betoflakes
Copy link

Oh!. Now, everything have sense. thank you @SorinGFS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

issue: enhancement Issue suggesting an enhancement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants