Code

Opened 3 years ago

Closed 12 months ago

Last modified 12 months ago

#17471 closed New feature (fixed)

Add ability to use smtplib.SMTP_SSL connection when sending email

Reported by: dje Owned by: nobody
Component: Core (Mail) Version: master
Severity: Normal Keywords: smtp sprints-django-ar
Cc: sorin, charettes, senko Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

There was a ticket opened three years ago to report this issue and it was brushed off with no investigation: https://code.djangoproject.com/ticket/9575. I've attached my patch which completely resolved the problem on my local machine. The problem seems to arise when using TLS, with username/password and port 465. I didn't have any servers with a port other than 465 to test with, but the previous bug report mentioned changing the port worked for them.

To reproduce the issue, I'd recommend using Gmail's servers (since they are free):
EMAIL_HOST_USER = 'user'
EMAIL_HOST_PASSWORD = 'pass'
EMAIL_USE_TLS = True
EMAIL_HOST = 'smtp.gmail.com'
EMAIL_PORT = 465

The servers I've tested it against are not just Gmail, so this is clearly not Google's issue. There may be an issue with the underlying Python SMTP library, but patching this in Django was the quickest way for me to get my project back up and running.

Attachments (5)

smtp_patch.txt (1.1 KB) - added by dje 3 years ago.
svn diff patch
ssl_setting_patch.txt (6.8 KB) - added by dje 3 years ago.
ssl_setting_patch.txt
ssl_setting_patch.diff (6.8 KB) - added by anonymous 3 years ago.
same as the same-named attachment with .txt extension, only with .diff as extension to allow for easy trac viewing
patch_ticket_17471_b.txt (7.1 KB) - added by areski 14 months ago.
patch_ticket_17471_c.txt (8.1 KB) - added by senko 12 months ago.
Update to previous patch to suppress UnicodeDecodeError

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by dje

svn diff patch

comment:1 Changed 3 years ago by kmtracey

  • Component changed from Uncategorized to Core (Mail)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Summary changed from Using TLS and port 465 for SMTP email causing Django to hang sending. to Add ability to use smtplib.SMTP_SSL connection when sending email
  • Type changed from Bug to New feature

Google's docs now say that port 465 is for SSL, while TLS/STARTTLS is 587 (they didn't used to be that specific as to which port was for what protocol). What Django implements, when EMAIL_USE_TLS is True, is starttls. So the proper port to use for Django speaking to GMail servers, today, given that Django implements TLS and not SSL, is 587, not 465.

The patch as presented can't be used in Django as is. It re-interprets Django's EMAIL_USE_TLS setting to mean "use an smtplib.SMTP_SSL connection". Such a connection doesn't work for servers who implement TLS, not SSL. For example if you use the patched code against GMail's server port 587 you get an exception: SSLError: (1, '_ssl.c:503: error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol'). So if we were to change the code as is done in the patch, suddenly everyone who had a working config talking to a TLS server would be broken.

It seems like what is needed to talk to an SSL-implementing server (like GMail port 465) is yet another option (sigh) to tell the email code to use an smtplib.SMTP_SSL connection rather than doing the starttls thing. Note smtplib.SMTP_SSL is new in Python 2.6 so we cannot support this new type of email security except when running under Python 2.6 (and higher).

Changing this to a feature request rather than a bug, and changing the summary to match. It's not a bug if things don't work correctly if they've been misconfigured, and telling Django to use tls against an email server who is expecting ssl and not tls is a misconfiguration. But what we are missing on the Djagno side is apparently any way to do the ssl connection instead of TLS, and if there are common servers that implement only SSL and not TLS then Django likely should support talking to them.

Last edited 3 years ago by kmtracey (previous) (diff)

comment:2 Changed 3 years ago by dje

Thank you for the correction. I'll follow up with the providers I was having trouble with to see if their documentation is wrong. One such provider is Amazon's SES service, so they are becoming quite a common server (I personally am developing two applications using them). They indicate TLS, but it only works with the SSL connection.

comment:3 Changed 3 years ago by dje

  • Cc dj.facebook@… added
  • Has patch unset
  • Type changed from New feature to Bug

So I've spent a while reading through the RFC specs on TLSv1 and discussing with the developers of a few of the providers I was having issues with and it turns out that this is actually a *bug* and not a new feature. TLSv1 has two operational modes: explicit and implicit when forming the connection/handshake. Django incorrectly assumes that all TLS connections are explicit connections. Explicit connections use STARTTLS and are generally accepted on port 587. Implicit connections (which may also fall back to SSL) are typically made over port 465 and should not use the STARTTLS command.

I'm changing this back to a bug as it is a misinterpretation of the spec and only half implemented.

comment:4 Changed 3 years ago by dje

  • Cc dj.facebook@… removed

comment:5 Changed 3 years ago by aaugustin

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

I'll skip the bug vs. feature request debate; it doesn't really matter.

To sum up:

  • Currently, Django supports only explicit connections (with STARTTLS);
  • This ticket is about supporting implicit connections too;
  • The change must be backwards compatible, which is likely to require yet another setting.

comment:6 Changed 3 years ago by dje

  • Type changed from Bug to New feature

Yeah, sorry about switching it back to a bug. It probably should have been left as a feature. I've attached a patch that should maintain backwards compatibility. The only parts I was unsure about was adding use_ssl to the constructor and whether to call starttls if they set TLS and SSL to true. Technically, they are supposed to be mutually exclusive, which is how I coded it.

What's in the patch:
A new setting: EMAIL_USE_SSL
Updated documentation.
Added tests around USE_TLS and USE_SSL

Changed 3 years ago by dje

ssl_setting_patch.txt

Changed 3 years ago by anonymous

same as the same-named attachment with .txt extension, only with .diff as extension to allow for easy trac viewing

comment:7 Changed 20 months ago by sorin

  • Cc sorin added

comment:8 Changed 17 months ago by ezequielsz

I just send a pull request for this, this is the link https://github.com/django/django/pull/792

comment:9 Changed 17 months ago by fgallina

  • Keywords sprints-django-ar added

comment:10 Changed 17 months ago by charettes

  • Cc charettes added
  • Version changed from 1.3 to master

Reviewed the PR.

Changed 14 months ago by areski

comment:11 Changed 14 months ago by areski

During djangocon eu, did review of the patch and PR
please find a new rebase patch in attachment and a new PR
https://github.com/django/django/pull/1124

comment:12 Changed 14 months ago by claudep

This is almost RFC, but on Python 3.2, I'm getting this output (but the test succeeds anyway):

.error: uncaptured python exception, closing channel <smtpd.SMTPChannel connected 127.0.0.1:47904 at 0x38db710> (<class 'UnicodeDecodeError'>:'utf-8' codec can't decode byte 0x99 in position 12: invalid start byte [/usr/lib/python3.2/asyncore.py|read|83] [/usr/lib/python3.2/asyncore.py|handle_read_event|449] [/usr/lib/python3.2/asynchat.py|handle_read|190] [/usr/lib/python3.2/smtpd.py|collect_incoming_data|278] [/home/claude/virtualenvs/djangogit32/lib/python3.2/encodings/utf_8.py|decode|16])

If this is reproducible on other systems, it would be nice if this could be silenced...

Changed 12 months ago by senko

Update to previous patch to suppress UnicodeDecodeError

comment:13 Changed 12 months ago by senko

  • Cc senko added
  • Patch needs improvement unset

I've updated the previous patch to use a fake SMTPChannel which ignores the UnicodeDecodeError.

comment:14 Changed 12 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 59ebe39812858ba37e83ab0ee886f676980d472d:

Fixed #17471 -- Added smtplib.SMTP_SSL connection option for SMTP backend

Thanks dj.facebook at gmail.com for the report and initial patch
and Areski Belaid and senko for improvements.

comment:15 Changed 12 months ago by timo

#13142 is a duplicate which contains some additional settings for the cert/key files.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.