smtplib's SMTP.connect() should store the server name in ._host for .starttls()

[wking]

BPO	25852
Nosy	@warsaw, @tiran, @bitdancer, @wking
Files	0001-smtplib-Set-SMTP._host-in-.connect.patch: Patch shifting the _host save from SMTP.init to SMTP.connect issue25852.diff issue25852_v2.patch: Alternative patch issue25852_v3_with_tests.patch: Alternative updated patch with unit test

^{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']

Linked PRs

• gh-70039: smtplib: store the server name in ._host in .connect() #115259

[wking]

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]

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]

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]

Please provide a more recent patch with a unit test.

[gigaplastik]

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.