None of the TLS/SSL classes for ftp, imap, nntp, pop and smtp have support for match_hostname() in order to verify that the SSL cert matches the host name. I'm not sure how we can handle the problem without creating backwards incompatibilities.

The patch implements check_hostname in order to match SSL certs with the peer's hostname in ftp, imap, nntp, pop and smtp library. So far the patch needs more tests and doc updates.

I consider the new feature a security fix. Right now everybody with any valid TLS/SSL certificate can claim that its certificate is valid for 'smtp.google.com'.

I don't know enough about SSL to make a "feature vs bug/security fix" ruling on this.  Can a second person who does--maybe Bill Janssen?--chime in?

I agree with Christian, mail.stufft.io should not be able to masquerade as smtp.google.com and being able to do so is a pretty big security hole.

I think adding an API in bugfix releases must be an exceptional decision and I'm honestly not convinced this issue justifies it.
It'd be more convincing if there was actually demand for this feature (e.g. from higher-level library authors).

Probably the higher level libraries don't even realize it's not happening, it's similar to the issue of SSL validation for HTTPS connections where a vast swathe of people didn't even realize that their certificates weren't being validated.

I agree with Antoine. The five attached bug reports only refer to Python 3.4.

> Probably the higher level libraries don't even realize it's not
> happening, it's similar to the issue of SSL validation for HTTPS
> connections where a vast swathe of people didn't even realize that
> their certificates weren't being validated.

Exactly. And we didn't add certificate checking in bugfix releases in
the name of security.

Now I appreciate that it would be a bit of a pity to wait for 3.5 to add
this, so perhaps Larry wants to make an exception to the 3.4 feature
freeze so that this feature can come in (assuming Christian's patch is
ready).

For a true security fix, the default for check_hostname would have to be True.  However, that will create a lot of backward compatibility problems for questionable gain.

I think Larry should make an exception for 3.4 and allow this new feature.

Okay, you have my permission to add match_hostname() and fix the security hole you cite.  Can you have it done soon, so it can bake for a while in trunk before we cut beta 2?

I assumed we were talking about 3.4 and didn't even notice that the issues had 3.3 and 3.2 attached to it.

My patch could be much simpler and easier if we could just drop support for ancient versions of OpenSSL. My idea requires at least OpenSSL 0.9.8f (release 2007) with SNI support. Six years are a lot for crypto software. All relevant platforms with vendor support have a more recent version of OpenSSL, too.

>>> context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
>>> context.verify_mode = ssl.CERT_REQUIRED
>>> context.check_hostname = True
>>> context.wrap_socket(sock, server_hostname="www.example.org")

server_hostname is used to for server name indicator (SNI) as well as the hostname for match_hostname(). It would remove lots and lots of code duplication, too.

The check_hostname takes care about invalid combinations, too:

>>> context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
>>> context.verify_mode == ssl.CERT_NONE
True
>>> context.check_hostname = True
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED

> My patch could be much simpler and easier if we could just drop
> support for ancient versions of OpenSSL. My idea requires at least
> OpenSSL 0.9.8f (release 2007) with SNI support. 

What are you talking about? Your patch doesn't use HAS_SNI.

The patches in the dependency tickets are using SNI. The problem is, a non-None server_hostname argument raises an error when OpenSSL doesn't support the feature.

Here is a demo patch for my idea. It makes it very easy to add hostname matching to existing code. All it takes is the "server_hostname" argument to wrap_socket() and a new property "check_hostname" for the SSLContext object. The rest is done in do_handshake().

> The patches in the dependency tickets are using SNI.

They all check for HAS_SNI, which is simple enough.
If you want to discuss a minimum supported OpenSSL version, that's fine with me, but it should be a separate discussion (on python-dev?).

If you think your patches are not ready, then please say so, so that I don't review them.

New changeset aa531135bc6b by Christian Heimes in branch 'default':
Issue #19509: Add SSLContext.check_hostname to match the peer's certificate
http://hg.python.org/cpython/rev/aa531135bc6b

I have addressed the five libraries in five sub-issues. http.client and asyncio.selector_events need small adjustments to use the new feature. Please review the patch.

The asyncio patch looks fine, but I'd like to see a unittest that actually fails (or mocks failing) the hostname check where it is now performed (in wrap_socket() IIUC?), to make sure that the exception is propagated out properly.

I'd rather have integration test with a real SSL connection in order to check the entire stack. asyncio doesn't have a test for CERT_REQUIRED yet. The attached patch tests three common cases: missing CA, missing server_hostname and a successful connection with full verification.

PS: The test uses keycert3.pem and pycacert.pem from the 3.4 test directory. keycert3.pem contains privat + public key and is signed by the CA in pycacert.pem.

I left my comment on the review. I forgot to mail the review.

See my messages on the review.  Is it absolutely necessary to close the socket when the hostname verification fails?

New changeset b6fce698e467 by Christian Heimes in branch 'default':
Issue #19509: Don't close the socket in do_handshake() when hostname verification fails.
http://hg.python.org/cpython/rev/b6fce698e467

New changeset 1a945fb875bf by Christian Heimes in branch 'default':
Issue 19509: Don't call match_hostname() twice in http.client.
http://hg.python.org/cpython/rev/1a945fb875bf

The new patch splits the test into three separate test cases.

Guido, Python 3.3 doesn't have the necessary CA and server cert files. The files were added to 3.4 for some other tests. The patch tries to load the files from 3.4's test directory and falls back to local copies. You have to copy/delete these files to test the patch:

$ hg rm Lib/test/test_asyncio/sample.*
$ hg cp Lib/test/ssl_key.pem Lib/test/ssl_cert.pem Lib/test/pycacert.pem Lib/test/keycert3.pem  Lib/test/test_asyncio/

Thanks Christian, this is almost right.  I'm uploading a patch that covers all bases:

- Works on Python 3.3 (TEST_HOME_DIR isn't defined there)
- Works on older Python 3.4 versions (check_hostname may not exist)
- Works on latest Python 3.4 version

New changeset 1605eda93392 by Christian Heimes in branch 'default':
Issue #19509: Finish implementation of check_hostname
http://hg.python.org/cpython/rev/1605eda93392

With the latest (revision 1605eda93392) I get four failures on OS X.  Three are like this (in all three selector types -- kqueue, select, poll):

======================================================================
ERROR: test_create_ssl_connection (test_events.SelectEventLoopTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_events.py", line 532, in test_create_ssl_connection
    tr, pr = self.loop.run_until_complete(f)
  File "/Users/guido/tulip/asyncio/base_events.py", line 177, in run_until_complete
    return future.result()
  File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
    raise self._exception
  File "/Users/guido/tulip/asyncio/tasks.py", line 276, in _step
    result = coro.throw(exc)
  File "/Users/guido/tulip/asyncio/base_events.py", line 388, in create_connection
    yield from waiter
  File "/Users/guido/tulip/asyncio/futures.py", line 320, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/Users/guido/tulip/asyncio/tasks.py", line 329, in _wakeup
    value = future.result()
  File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
    raise self._exception
  File "/Users/guido/tulip/asyncio/selector_events.py", line 618, in _on_handshake
    self._sock.do_handshake()
  File "/usr/local/lib/python3.4/ssl.py", line 748, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:599)

The last is similar in test_streams.py:

======================================================================
ERROR: test_open_connection_no_loop_ssl (test_streams.StreamReaderTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_streams.py", line 58, in test_open_connection_no_loop_ssl
    reader, writer = self.loop.run_until_complete(f)
  File "/Users/guido/tulip/asyncio/base_events.py", line 177, in run_until_complete
    return future.result()
  File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
    raise self._exception
  File "/Users/guido/tulip/asyncio/tasks.py", line 276, in _step
    result = coro.throw(exc)
  File "/Users/guido/tulip/asyncio/streams.py", line 43, in open_connection
    lambda: protocol, host, port, **kwds)
  File "/Users/guido/tulip/asyncio/base_events.py", line 388, in create_connection
    yield from waiter
  File "/Users/guido/tulip/asyncio/futures.py", line 320, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/Users/guido/tulip/asyncio/tasks.py", line 329, in _wakeup
    value = future.result()
  File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
    raise self._exception
  File "/Users/guido/tulip/asyncio/selector_events.py", line 618, in _on_handshake
    self._sock.do_handshake()
  File "/usr/local/lib/python3.4/ssl.py", line 748, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:599)


I get the same failures when I copy the changes to the Tulip repo.

That's fixed by revision ec1e7fc9b5a4.

Christian, can this bug be closed now, or is there more?

I'm closing this ticket. All features have been implemented and tests are passing, too.

I have created #19909 as a reminder for doc improvements.