Fixed #31885 -- Updated SMTP Email Backend to use an SSLContext. #13305
Conversation
carltongibson
changed the title
|
I think this also would require some changes to docs/topics/email.txt Update: I see this and more was already raised in the ticket https://code.djangoproject.com/ticket/31885 |
|
@lsaavedr Do you have time to keep working on this? |
|
@felixxm yeah what do you need to fix that? |
|
it's ok? 🥺 |
There was a problem hiding this comment.
Hi @isaavedr. Thanks. It looks good.
I've asked on the mailing list re the additional settings here — if that weren't an option, we'd probably add the kwargs and then use a subclass to contain setting them in __init__() — I just want to see what people say, since once we add a setting we do have to live with it.
Let's see how that goes. (In any one case another setting isn't the end of the world but they grow...)
There was a problem hiding this comment.
Hi @lsaavedr — I think I was having a brain freeze this week — via the mailing list discussion, as per the SMTP.starttls docs, that you already linked in the ticket description 🤦 — keyfile and certfile are themselves deprecated, so as well as adding the new settings we should deprecate the old ones for removal.
Are you OK to add that deprecation? See the Deprecating a feature section of the contributing guide.
We'd need to raise warnings if the old settings are used, and if the kwargs to EmailBackend are used. Then have tests checking warnings are raised. Plus versionchanged notes and an addition to deprecation.txt. — Let me know if you want/need any input on that.
Other than that extra step, looks super. Thanks for the input! 🥇
| @@ -468,7 +468,7 @@ can :ref:`write your own email backend <topic-custom-email-backend>`. | |||
| SMTP backend | |||
| ~~~~~~~~~~~~ | |||
|
|
|||
| .. class:: backends.smtp.EmailBackend(host=None, port=None, username=None, password=None, use_tls=None, fail_silently=False, use_ssl=None, timeout=None, ssl_keyfile=None, ssl_certfile=None, **kwargs) | |||
| .. class:: backends.smtp.EmailBackend(host=None, port=None, username=None, password=None, use_tls=None, fail_silently=False, use_ssl=None, timeout=None, ssl_keyfile=None, ssl_certfile=None, ssl_cafile=None, ssl_capath=None, ssl_cadata=None, **kwargs) | |||
There was a problem hiding this comment.
We need a versionchanged annotation to go with this.
|
@carltongibson Thanks for you time spend in this, but I need a little more of you attention: using keyfile and certfile is not deprecated, that is a good method of authentication (clients) and I think that this method will no deprecated in many years, or decades (pass this parameters in starttls function is deprecated in favor of using context to pass this parameters). I am add the using of CA parameters to make the server authentication, that parameters are used by the client to check the authentication of the server, is the other side of the authentication process, thanks and regards! |
|
Hi @lsaavedr — Right, so you're (optionally) specifying a particular key to use... — I guess worst case scenario here is we just add the settings, but it feels like we're adding too many pass-through parameters in order to expose too much of the Does that make sense? Ideally we pick a clean default API, and then allow folks to subclass if they need to access every bell and whistle. 🤔 |
|
No, today is not optional, always the client must be check the authenticity of the server (with CA parameters), it is like (and with the same sense) the green padlock in all https websites, back in the pass two old decades was optional (client authentication and server authentication is needed). |
|
Okay... but there's a difference between using the system provided certs and loading a specific private key no? If you don't provide the ssl key and cert here, the server will just need to have a valid key from a CA no? (I need to have a play and test this.) |
|
Or another way of phrasing is, shouldn't the implementation we provide by default just use create_default_context and be done with it? |
|
yes, you are right, from the doc (purpose is SERVER_AUTH by default):
with this patch it is used the system CA by default |
|
OK... so my thought is that we could deprecate the For folks wanting specify a specific key/cert — you want to do this yes? — we could then say to subclass How would that seem to you? For me, we keep the normal case simple — simpler than now even — but still have the advanced case available if it's needed. |
|
sound good to me... |
|
Yes? Super! (It covers your use-case yes?) Are you up for adjusting the PR?
Seem OK to you? |
|
:'( |
|
Hey @lsaavedr I don't understand your comment. Do you not agree with the idea? Happy to discuss more (and look again in more depth next week) if you think we should do something different? most people don't need to provide a specific key/cert pair no? (They can just use then system certs to determine validity of a key). Maybe I've missed the point. I was going from the doc and code, rather than testing against a server here, so ... Thanks! |
django/core/mail/backends/smtp.py
Outdated
| # If local_hostname is not specified, socket.getfqdn() gets used. | ||
| # For performance, we use the cached FQDN for local_hostname. | ||
| connection_params = {'local_hostname': DNS_NAME.get_fqdn()} | ||
| if self.timeout is not None: | ||
| connection_params['timeout'] = self.timeout | ||
| if self.use_ssl: | ||
| connection_params.update({ | ||
| 'keyfile': self.ssl_keyfile, | ||
| 'certfile': self.ssl_certfile, | ||
| 'context': context |
There was a problem hiding this comment.
No reason to use dict.update() anymore.
There was a problem hiding this comment.
- Add versionadded annotations to the settings in docs/ref/settings.txt
- Add the new settings to the "Core Settings Topical Index" at the bottom of docs/ref/settings.txt
- Is the "certfile must be specified" error tested? (and please add a period). I'd expecte a test with
assertRaisesMessage(). - I don't think the deprecation is needed as a part of this. At the very least it should likely be a separate commit, but it could probably be done as a separate ticket unless I missed the reason.
django/core/mail/backends/smtp.py
Outdated
| self.use_tls = settings.EMAIL_USE_TLS if use_tls is None else use_tls | ||
| self.use_ssl = settings.EMAIL_USE_SSL if use_ssl is None else use_ssl | ||
| # client cert | ||
| self.ssl_keyfile = ( |
There was a problem hiding this comment.
I would stick with the one line if/else statements. The style guide says, "Don’t limit lines of code to 79 characters if it means the code looks significantly uglier or is harder to read."
django/core/mail/backends/smtp.py
Outdated
| context = ssl.create_default_context( | ||
| cafile=self.ssl_cafile, | ||
| capath=self.ssl_capath, | ||
| cadata=self.ssl_cadata |
There was a problem hiding this comment.
Include a trailing comma in cases like this so that if more items are added later, this line doesn't have to be modified again.
django/core/mail/backends/smtp.py
Outdated
| **kwargs): | ||
| super().__init__(fail_silently=fail_silently) | ||
|
|
|
OK, update: following discussion on #13728, let's push this forwards adding the settings now. I'll then create a new ticket to look at moving the pass-through parameters into the Email Backend (so we have 1 or 2 settings, not one for every option of the underlying protocol) I'll address Tim's comments now, and then we'll have a last look. |
There was a problem hiding this comment.
Hi @lsaavedr — sorry for the back and forth here, but I need to ask for a change of approach.
Previously, I was concerned about adding the extra settings which are merely pass-through parameters, and which most users never need. (As per our discussion, if ssl_cafile &co are None create_default_context() will just use the system CA certificates, which is what almost all users are going to want.)
Looking again I'm still concerned about the exact same thing. It's an unwarranted increase in the API surface area. Doubting myself due to the discussion, I asked for a second opinion from @felixxm and he agreed.
What we'd like it so add the kwargs to EmailBackend, have the code use an SSLContext as suggested, and then recommend in the docs that folks subclass if they need to set the parameters.
At the same time, in a second commit we think we should deprecate EMAIL_SSL_CERTFILE and EMAIL_SSL_KEYFILE, for the same reasons.
(There's an argument for moving all these settings into the backend, leaving just BACKEND, but with HOST &co, at least the majority of users need to set these.)
I hope that makes sense.
Previously you expressed dissatisfaction when I asked for these changes. Sorry about that. Bottom line is we're just trying to get it right, keep the quality as high as we can, and we need to remember that once a change is in we have to live with it.
Are you though up for finishing this off? I understand if not but if so we can deassign it, and likely get it picked up by another contributor.
Thanks for your efforts!
|
Hi @carltongibson using system CA to server certificates it's okay, but please be aware to not remove the optional client certificate, these are important and always good option in any email settings, thanks! |
|
Hey @lsaavedr -- yes, we're happy to add the kwargs to EmailBackend, so if you want it, you'd just creat a subclass and set the values in init. |
|
Hey Carlton, I worked on my review comments a little while ago as I was considering finishing this. I pushed them here for now. Not sure I agree with the subclass approach. I continued the django-developers discussion. |
|
Thanks @timgraham. I will follow up there in the morning. |
* add CA parameters
* add parameters test
|
Fixed in 2848e5d. |
ticket-31885
set ssl cert and key outside a SSLContext object are deprecated and we need add CA parameters to make the server authentication