I like to deprecate some SSL related parts of Python:

- ssl.wrap_socket() is a horrible abomination. People should use SSLContext.wrap_socket() instead
- all certfile/cert_file, keyfile/key_file and check_hostname arguments. Use context / ssl_context instead.
- make ftplib, imaplib, nntplib, pop3lib, smtplib etc. validate certs by default.

10/10, yes. I will happily provide code review for this.

> - make ftplib, imaplib, nntplib, pop3lib, smtplib etc. validate certs by default.

I'm not sure about this one:
http://legacy.python.org/dev/peps/pep-0476/#other-protocols

Another deprecation: I like to deprecate all arguments from SSLSocket.__init__() and require users to go through SSLContext.wrap_socket(). It's going to make the implementation much simpler. The argument list is just crazy:

class SSLSocket(socket):
    def __init__(self, sock=None, keyfile=None, certfile=None,
                 server_side=False, cert_reqs=CERT_NONE,
                 ssl_version=PROTOCOL_TLS, ca_certs=None,
                 do_handshake_on_connect=True,
                 family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None,
                 suppress_ragged_eofs=True, npn_protocols=None, ciphers=None,
                 server_hostname=None,
                 _context=None):

I like the idea of using SSLContext as the obvious and only choice to
"configure" SSL.

memo to me: check if SSLContext.wrap_socket() can deal with a fileno as sock argument.

First draft of a patch: https://github.com/tiran/cpython/commits/feature/ssl_deprecation

urllib.request.urlopen() should be affected too right?

Yes, urllib.request.urlopen needs an update too. It takes those certfile and keyfile and usage of those could be deprecated in favor of context.

I have deprecated cafile, capath and cadefault for urlopen(). The function didn't pop up on my radar because I was looking for certfile and cert_file, not cafile. I also added deprecations to the documentation of SSLSocket.read and write.

How does that tie in with SSLObject.read() and write(). I have never used this class, but the documentation refers back to SSLSocket.

It seems there are no recv() etc methods to use as an alternative. So maybe deprecate read() and write() on SSLSocket only, and leave SSLObject intact?

+1 for directing all programmatic configuration through SSLContext

However, implicitly verifying certificates for protocols other than HTTPS needs to be contingent on a properly designed approach to configuration that leaves informed users in full control of the behaviour of their systems - while I'm fully supportive of secure-by-default behaviour to protect unaware users, it's also the case that most other protocols haven't had the forcing function of web browser behaviour encouraging them to improve their certificate handling, and even that's still in a tragically bad state once you get away from the public web.

The file based scheme in PEP 493, https://www.python.org/dev/peps/pep-0493/#backporting-pep-476-to-earlier-python-versions, was deliberately written to be potentially suitable for expansion to other protocols, but actually using it for that purpose would require the definition of a new feature PEP targeting 3.7 (which may then potentially be pitched for backporting to earlier versions as a subsequent proposal).

In the mean time I have reconsidered my position. How about we *document* that a future version of Python will very all TLS/SSL connections by default. Users have to explicitly pass an unverified context if they still want the old behavior.

+1 for a common note in all affected modules along the lines of "An appropriately configured SSLContext should be provided for any use cases that involve accepting self-signed certificates, privately signed certificates, or any other kind of certificate that won't validate against the default system certificate trust store.

It is expected that a future version of Python will switch to explicitly verifying SSL certificates for all SSL/TLS protected connections by default (due to the widespread use of self-signed and privately signed certificates for other protocols, full verification is currently the default only for HTTPS)."

Regarding ssl.wrap_socket(), would it be feasible to provide a migration path to a situation where that's just a thin wrapper around ssl.get_default_context().wrap_socket()?

Comparing the parameter lists:

>>> module_params - method_params
{'ciphers', 'keyfile', 'ca_certs', 'ssl_version', 'cert_reqs', 'certfile'}
>>> method_params - module_params
{'server_hostname'}

That means the real problems are the ciphers, keyfile, ca_certs, ssl_version, cert_reqs and certfile parameters and the internal use of SSLContext() rather than get_default_context(), rather than the essential idea of providing a shorthand spelling for "wrap a socket with the default SSL/TLS settings".

New changeset aed3c541b2f1 by Christian Heimes in branch 'default':
Issue #28022: Deprecate ssl-related arguments in favor of SSLContext.
https://hg.python.org/cpython/rev/aed3c541b2f1

I have pushed all deprecation except ssl.wrap_socket(). Nick raised some concerns. I like to discourage people to use it because it hurts performance and is no longer best practice. How about we mark the function as legacy function and move it to a less prominent place in the documentation?

I asked in more detail about this on the list, but my main question is why can't wrap_socket() be fixed by doing a rip-and-replace on its internals (e.g. by using a model similar to the one in random, where there's an implicit global Random instance that gets invoked if you use the module level API instead of creating your own instance), rather than having to tell users to change *their* code.

Like Random, I'd like to see SSLContext as a lower level implementation detail that's there for when people need it, but can be largely ignored if they just want the default behaviours (i.e. system trust store with python-dev specified SSL/TLS settings)

An implicit global SSL Context? It kinda sounds a bit gross.

Thinking about that more, it's a bit harder than the Random module as well. The only state the random module has to worry about is the seed and internal state of the RNG.

However, many of the arguments to ssl.wrap_socket change the SSLContext options for things like what ciphers are active, what trust stores, etc. So we couldn't have a single SSLContext at the global level without removing those options from wrap_socket. Otherwise we'd need some sort of dict of SSLContexts that keyed off of the options passed to wrap_socket.

That sounds like the "re" module would be a better exemplar for an SSL module convenience API design than "random" then - that has a similar model of needing an LRU cache for the compiled patterns for performance reasons, while still making working with the compiled form in your own code optional (which means you don't need to find a place to store it to gain the performance benefits of pattern reuse).

It would need to be a hidden cache, though - since SSLContext objects are mutable, it wouldn't be a good idea to expose any implicitly shared ones.

The performance benefit is not worth the risk. For 10 httplib requests to pypi.python.org, a shared SSLContext is about 5% faster than a new context for each request. Session resumption improves the simple test case by another 20%.

Leaving the option of context caching entirely to the caller would definitely make things simpler - my main interest is just in avoiding a hard compatibility break for folks that aren't doing anything particularly wrong, by which I mean specifically cases where a wrap_socket() implementation like this one would continue to work for them:

    def wrap_socket(sock, *args, *kwds):
        return ssl.get_default_context().wrap_socket(sock, *args, **kwds)

New test failure when using -Werror:

======================================================================
ERROR: test_local_bad_hostname (test.test_httplib.HTTPSTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/media/disk/home/proj/python/cpython/Lib/test/test_httplib.py", line 1646, in test_local_bad_hostname
    check_hostname=True)
  File "/media/disk/home/proj/python/cpython/Lib/http/client.py", line 1373, in __init__
    DeprecationWarning, 2)
DeprecationWarning: key_file, cert_file and check_hostname are deprecated, use a custom context instead.

New changeset 2e541e994927 by Christian Heimes in branch 'default':
Issue 28022: Catch deprecation warning in test_httplib, reported by Martin Panter
https://hg.python.org/cpython/rev/2e541e994927

Thanks for the report. "./python -Werror -m test -uall test_httplib" is now passing for me.

test_imaplib is failed too.

======================================================================
ERROR: test_logincapa_with_client_certfile (test.test_imaplib.RemoteIMAP_SSLTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/serhiy/py/cpython-debug/Lib/test/test_imaplib.py", line 641, in test_logincapa_with_client_certfile
    _server = self.imap_class(self.host, self.port, certfile=CERTFILE)
  File "/home/serhiy/py/cpython-debug/Lib/imaplib.py", line 1273, in __init__
    "custom ssl_context instead", DeprecationWarning, 2)
DeprecationWarning: keyfile and certfile are deprecated, use acustom ssl_context instead

----------------------------------------------------------------------

New changeset 57e88d1159fc by Christian Heimes in branch 'default':
Issue #28022: Catch another deprecation warning in imaplib
https://hg.python.org/cpython/rev/57e88d1159fc

Shouldn't this issue be closed?

Since SSL context arguments are not supported in 2.7, the deprecated arguments can't be removed until EOL of 2.7.

Hello everyone, and thank you as usual for all your hard work keeping the python ecosystem going.

I saw that the start of this thread said it was going to

> - make ftplib, imaplib, nntplib, pop3lib, smtplib etc. validate certs by default.

but this hasn't been done, at least not for imaplib. imaplib is still calling the undocumented "ssl._create_stdlib_context":

https://github.com/python/cpython/blob/2b496e79293a8b80e8ba0e514e186b3b1467b64b/Lib/imaplib.py#L1320

which is actually "ssl._create_unverified_context":

https://github.com/python/cpython/blob/2b496e79293a8b80e8ba0e514e186b3b1467b64b/Lib/ssl.py#L842

which is indeed unverified: despite defaulting to PROTOCOL_TLS_CLIENT, which "enables CERT_REQUIRED and check_hostname by default.", it overrides that by setting check_hostname=False:

https://github.com/python/cpython/blob/2b496e79293a8b80e8ba0e514e186b3b1467b64b/Lib/ssl.py#L811



To demonstrate, check out this tester script:

$ cat a.py 
import os, imaplib

with imaplib.IMAP4_SSL(os.environ.get('HOSTNAME')) as S: 
    print(S.login(os.environ.get('USERNAME'), os.environ.get('PASSWORD')))
$ HOSTNAME=46.23.90.174 USERNAME=test1 PASSWORD=test1test1 python3 a.py 
('OK', [b'Logged in'])

I don't have a cert for 46.23.90.174 (no one will give out certs for IPs!), so this is wrong!

In order to actually enable verification you need to know the incantation. It's not that long but it is subtle and frighteningly easy to get wrong. Here it is:

$ cat a.py 
import os, ssl, imaplib

ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
ctx.load_default_certs()

with imaplib.IMAP4_SSL(os.environ.get('HOSTNAME'), ssl_context=ctx) as S: 
    print(S.login(os.environ.get('USERNAME'), os.environ.get('PASSWORD')))
$ HOSTNAME=46.23.90.174 USERNAME=test1 PASSWORD=test1test1 python3 a.py 
Traceback (most recent call last):
  File "a.py", line 6, in <module>
    with imaplib.IMAP4_SSL(os.environ.get('HOSTNAME'), ssl_context=ctx) as S: 
  File "/usr/lib/python3.6/imaplib.py", line 1288, in __init__
    IMAP4.__init__(self, host, port)
  File "/usr/lib/python3.6/imaplib.py", line 198, in __init__
    self.open(host, port)
  File "/usr/lib/python3.6/imaplib.py", line 1301, in open
    IMAP4.open(self, host, port)
  File "/usr/lib/python3.6/imaplib.py", line 299, in open
    self.sock = self._create_socket()
  File "/usr/lib/python3.6/imaplib.py", line 1293, in _create_socket
    server_hostname=self.host)
  File "/usr/lib/python3.6/ssl.py", line 407, in wrap_socket
    _context=self, _session=session)
  File "/usr/lib/python3.6/ssl.py", line 817, in __init__
    self.do_handshake()
  File "/usr/lib/python3.6/ssl.py", line 1077, in do_handshake
    self._sslobj.do_handshake()
  File "/usr/lib/python3.6/ssl.py", line 694, in do_handshake
    match_hostname(self.getpeercert(), self.server_hostname)
  File "/usr/lib/python3.6/ssl.py", line 327, in match_hostname
    % (hostname, ', '.join(map(repr, dnsnames))))
ssl.CertificateError: hostname '46.23.90.174' doesn't match either of 'comms.kousu.ca', 'comms3.kousu.ca'


I can see from this thread there were some concerns about breaking people's self-signed certs back in 2016. But it's five years later now and letsencrypt is super common now, and most servers and clients are enforcing TLS, especially when credentials are involved.

Could this be revisited?

Thanks for any attention you have gifted to this :)

The part with "make ftplib, imaplib, nntplib, pop3lib, smtplib etc. validate certs by default" was not implemented. These modules still default to unverified connections.