Repository navigation
smtplib's SMTP.connect() should store the server name in ._host for .starttls() #70039
Description
| BPO | 25852 |
|---|---|
| Nosy | @warsaw, @tiran, @bitdancer, @wking |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/bitdancer'
closed_at = None
created_at = <Date 2015-12-12.22:59:22.447>
labels = ['3.7', 'type-bug', 'library', 'expert-email']
title = "smtplib's SMTP.connect() should store the server name in ._host for .starttls()"
updated_at = <Date 2017-09-07.02:07:21.156>
user = 'https://github.com/wking'bugs.python.org fields:
activity = <Date 2017-09-07.02:07:21.156>
actor = 'christian.heimes'
assignee = 'r.david.murray'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'email']
creation = <Date 2015-12-12.22:59:22.447>
creator = 'labrat'
dependencies = []
files = ['41291', '41293', '42067', '44495']
hgrepos = []
issue_num = 25852
keywords = ['patch']
message_count = 5.0
messages = ['256299', '256305', '261151', '275054', '275297']
nosy_count = 6.0
nosy_names = ['barry', 'christian.heimes', 'r.david.murray', 'SilentGhost', 'labrat', 'gigaplastik']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue25852'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']
Activity
wking commentedon Dec 12, 2015
With the current tip, starttls uses ._host when calling wrap_socket 1, but ._host is only setup in SMTP.__init__ 2. Before bpo-22921 3 starttls would ignore ._host when SNI wasn't available locally. But as far as I can tell, starttls has never used _host when connection happens via an explicit connect() call. This leads to errors like 4:
>>> smtp = smtplib.SMTP() >>> smtp.connect(host="smtp.gmail.com", port=587) >>> smtp.ehlo() >>> smtp.starttls() File "smtp_test.py", line 10, in <module> smtp.starttls() File "/usr/lib/python3.4/smtplib.py", line 676, in starttls server_hostname=server_hostname) File "/usr/lib/python3.4/ssl.py", line 344, in wrap_socket _context=self) File "/usr/lib/python3.4/ssl.py", line 540, in __init__ self.do_handshake() File "/usr/lib/python3.4/ssl.py", line 767, in do_handshake self._sslobj.do_handshake() ssl.SSLError: [SSL: TLSV1_ALERT_DECODE_ERROR] tlsv1 alert decode error (_ssl.c:598)I think a better approach would be to move the ._host set into .connect (patch attached). It would still happen in SMTP(host=…) because 5, but would also allow starttls when users use SMTP() and then call connect(host=…) explicitly.
I've formatted the patch with Git, but its simple enough that it should be easy to apply in Mercurial. Still, let me know if I can make applying it easier by rerolling the patch.
SilentGhost commentedon Dec 13, 2015
Here is a usable patch, there doesn't seem to be a test for starttls, but it's still would be a good idea to add a test for this issue.
gigaplastik commentedon Mar 3, 2016
Found the same issue independently, but I believe my version of the patch is a little more thoughtful. Since the host is allowed to be supplied in 'hostname:port' format the assignment to ._host should be made _after_ checking (and probably parsing) this format.
The reason for this is that ._host is passed to ssl.SSLContext.wrap_socket method where it is used for SNI, defined in [1]. According to this RFC, "[c]urrently, the only server names supported are DNS hostnames; ... Literal IPv4 and IPv6 addresses are not permitted in [HostName]."
Checking if hostname passed is really a DNS name and not an IP address is up to ssl library, but here, in .connect method, at least the port number should be stripped off.
[1] https://tools.ietf.org/html/rfc4366.html
tiran commentedon Sep 8, 2016
Please provide a more recent patch with a unit test.
gigaplastik commentedon Sep 9, 2016
Here is the updated (3.6.0a4+) patch for the issue with the unit test included.
My unit test only checks for this issue particular artifacts, it does not test that .starttls actually works. This is because the current test suite for smtplib (Lib/test/test_smtplib.py) does not implement SSL/TLS functionality neither in its "debugging server", nor in its "simulated server" and implementing it on my own is beyond my expertise. Nevertheless, the provided unit test does guarantee .starttls wouldn't break because of the empty ._host attribute.
The overall Python test suite also runs successfully with these patches applied.
smtplib: store the server name in ._host in .connect()
smtplib: store the server name in ._host in .connect()
gh-70039: smtplib: store the server name in ._host in .connect() (#11…
pythongh-70039: smtplib: store the server name in ._host in .connect() (
pythongh-70039: smtplib: store the server name in ._host in .connect() (
[3.14] gh-70039: smtplib: store the server name in ._host in .connect…
[3.13] gh-70039: smtplib: store the server name in ._host in .connect…
bitdancer commentedon Apr 8, 2026
Merged and backported. Sorry for the merge delay, but I wasn't notified that the tests were passing now ;)
Thanks all. It's nice to retire an old issue. Let's hope it stays retired.
pythongh-70039: smtplib: store the server name in ._host in .connect() (