classification
Title: SSL releated deprecation for 3.6
Type: security Stage: patch review
Components: Documentation, Library (Lib), SSL Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: Lukasa, alex, christian.heimes, dstufft, giampaolo.rodola, janssen, martin.panter, ncoghlan, orsenthil, python-dev, serhiy.storchaka, vstinner
Priority: high Keywords: patch

Created on 2016-09-08 15:19 by christian.heimes, last changed 2017-10-22 07:59 by serhiy.storchaka.

Files
File name Uploaded Description Edit
ssl_deprecations.patch christian.heimes, 2016-09-09 09:03
Messages (28)
msg275043 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-08 15:19
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.
msg275056 - (view) Author: Cory Benfield (Lukasa) * Date: 2016-09-08 15:53
10/10, yes. I will happily provide code review for this.
msg275109 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-09-08 18:40
> - 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
msg275127 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-08 19:50
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):
msg275129 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-09-08 19:53
I like the idea of using SSLContext as the obvious and only choice to
"configure" SSL.
msg275134 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-08 20:07
memo to me: check if SSLContext.wrap_socket() can deal with a fileno as sock argument.
msg275144 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-08 20:42
First draft of a patch: https://github.com/tiran/cpython/commits/feature/ssl_deprecation
msg275230 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-09-09 01:10
urllib.request.urlopen() should be affected too right?
msg275243 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2016-09-09 03:35
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.
msg275291 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-09 09:03
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.
msg275300 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-09-09 10:20
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?
msg275306 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-09 11:09
+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).
msg275308 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-09 11:13
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.
msg275603 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-10 09:04
+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".
msg275699 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-10 21:23
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
msg275700 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-10 21:27
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?
msg275726 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-11 02:44
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)
msg275727 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2016-09-11 02:45
An implicit global SSL Context? It kinda sounds a bit gross.
msg275728 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2016-09-11 02:50
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.
msg275730 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-11 04:02
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.
msg275737 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-11 06:45
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%.
msg275739 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-11 07:43
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)
msg275767 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-09-11 10:26
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.
msg275816 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-11 17:54
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
msg275817 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-11 17:56
Thanks for the report. "./python -Werror -m test -uall test_httplib" is now passing for me.
msg275843 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-11 20:03
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

----------------------------------------------------------------------
msg275853 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-11 20:47
New changeset 57e88d1159fc by Christian Heimes in branch 'default':
Issue #28022: Catch another deprecation warning in imaplib
https://hg.python.org/cpython/rev/57e88d1159fc
msg304733 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-22 07:59
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.
History
Date User Action Args
2017-10-22 07:59:05serhiy.storchakasetmessages: + msg304733
2016-09-15 07:48:59christian.heimessetcomponents: + SSL
2016-09-11 20:47:11python-devsetmessages: + msg275853
2016-09-11 20:03:04serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg275843
2016-09-11 17:56:38christian.heimessetmessages: + msg275817
2016-09-11 17:54:53python-devsetmessages: + msg275816
2016-09-11 10:26:18martin.pantersetmessages: + msg275767
2016-09-11 07:43:41ncoghlansetmessages: + msg275739
2016-09-11 06:45:05christian.heimessetmessages: + msg275737
2016-09-11 04:02:45ncoghlansetmessages: + msg275730
2016-09-11 02:50:13dstufftsetmessages: + msg275728
2016-09-11 02:45:47dstufftsetmessages: + msg275727
2016-09-11 02:44:02ncoghlansetmessages: + msg275726
2016-09-10 21:27:08christian.heimessetmessages: + msg275700
2016-09-10 21:23:51python-devsetnosy: + python-dev
messages: + msg275699
2016-09-10 09:04:33ncoghlansetmessages: + msg275603
2016-09-09 11:13:40christian.heimessetmessages: + msg275308
2016-09-09 11:09:38ncoghlansetnosy: + ncoghlan
messages: + msg275306
2016-09-09 10:20:24martin.pantersetmessages: + msg275300
2016-09-09 09:04:31christian.heimessetstage: needs patch -> patch review
2016-09-09 09:03:53christian.heimessetfiles: + ssl_deprecations.patch
keywords: + patch
2016-09-09 09:03:18christian.heimessetmessages: + msg275291
2016-09-09 03:35:52orsenthilsetmessages: + msg275243
2016-09-09 01:10:59martin.pantersetnosy: + martin.panter
messages: + msg275230
2016-09-08 22:48:44christian.heimeslinkissue20762 superseder
2016-09-08 20:42:26christian.heimessetmessages: + msg275144
2016-09-08 20:07:55christian.heimessetmessages: + msg275134
2016-09-08 20:01:34orsenthilsetnosy: + orsenthil
2016-09-08 19:53:41vstinnersetmessages: + msg275129
2016-09-08 19:50:26christian.heimessetmessages: + msg275127
2016-09-08 18:40:13vstinnersetnosy: + vstinner
messages: + msg275109
2016-09-08 15:53:08Lukasasetnosy: + Lukasa
messages: + msg275056
2016-09-08 15:19:02christian.heimescreate