Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSL releated deprecation for 3.6 #72209

Closed
tiran opened this issue Sep 8, 2016 · 30 comments
Closed

SSL releated deprecation for 3.6 #72209

tiran opened this issue Sep 8, 2016 · 30 comments
Assignees
Labels
3.10 docs Documentation in the Doc dir expert-SSL stdlib Python modules in the Lib dir type-security A security issue

Comments

@tiran
Copy link
Member

tiran commented Sep 8, 2016

BPO 28022
Nosy @ncoghlan, @orsenthil, @vstinner, @giampaolo, @tiran, @kousu, @alex, @vadmium, @serhiy-storchaka, @dstufft, @Lukasa
Files
  • ssl_deprecations.patch
  • 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/tiran'
    closed_at = <Date 2021-04-17.10:48:13.818>
    created_at = <Date 2016-09-08.15:19:02.658>
    labels = ['type-security', 'expert-SSL', 'library', '3.10', 'docs']
    title = 'SSL releated deprecation for 3.6'
    updated_at = <Date 2021-08-08.19:17:15.658>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2021-08-08.19:17:15.658>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2021-04-17.10:48:13.818>
    closer = 'christian.heimes'
    components = ['Documentation', 'Library (Lib)', 'SSL']
    creation = <Date 2016-09-08.15:19:02.658>
    creator = 'christian.heimes'
    dependencies = []
    files = ['44492']
    hgrepos = []
    issue_num = 28022
    keywords = ['patch']
    message_count = 30.0
    messages = ['275043', '275056', '275109', '275127', '275129', '275134', '275144', '275230', '275243', '275291', '275300', '275306', '275308', '275603', '275699', '275700', '275726', '275727', '275728', '275730', '275737', '275739', '275767', '275816', '275817', '275843', '275853', '304733', '399231', '399235']
    nosy_count = 13.0
    nosy_names = ['ncoghlan', 'janssen', 'orsenthil', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'kousu', 'alex', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'dstufft', 'Lukasa']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue28022'
    versions = ['Python 3.10']

    @tiran
    Copy link
    Member Author

    tiran commented Sep 8, 2016

    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.

    @tiran tiran added the 3.7 label Sep 8, 2016
    @tiran tiran self-assigned this Sep 8, 2016
    @tiran tiran added stdlib Python modules in the Lib dir docs Documentation in the Doc dir type-security A security issue labels Sep 8, 2016
    @Lukasa
    Copy link
    Mannequin

    Lukasa mannequin commented Sep 8, 2016

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

    @vstinner
    Copy link
    Member

    vstinner commented Sep 8, 2016

    • 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

    @tiran
    Copy link
    Member Author

    tiran commented Sep 8, 2016

    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):

    @vstinner
    Copy link
    Member

    vstinner commented Sep 8, 2016

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

    @tiran
    Copy link
    Member Author

    tiran commented Sep 8, 2016

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

    @tiran
    Copy link
    Member Author

    tiran commented Sep 8, 2016

    @vadmium
    Copy link
    Member

    vadmium commented Sep 9, 2016

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

    @orsenthil
    Copy link
    Member

    orsenthil commented Sep 9, 2016

    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.

    @tiran
    Copy link
    Member Author

    tiran commented Sep 9, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Sep 9, 2016

    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?

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 9, 2016

    +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).

    @tiran
    Copy link
    Member Author

    tiran commented Sep 9, 2016

    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.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 10, 2016

    +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".

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2016

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

    @tiran
    Copy link
    Member Author

    tiran commented Sep 10, 2016

    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?

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 11, 2016

    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)

    @dstufft
    Copy link
    Member

    dstufft commented Sep 11, 2016

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

    @dstufft
    Copy link
    Member

    dstufft commented Sep 11, 2016

    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.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 11, 2016

    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.

    @tiran
    Copy link
    Member Author

    tiran commented Sep 11, 2016

    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%.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 11, 2016

    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)

    @vadmium
    Copy link
    Member

    vadmium commented Sep 11, 2016

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 11, 2016

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

    @tiran
    Copy link
    Member Author

    tiran commented Sep 11, 2016

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

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Sep 11, 2016

    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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 11, 2016

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

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Oct 22, 2017

    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.

    @tiran tiran added 3.10 and removed 3.7 labels Oct 21, 2020
    @tiran tiran closed this as completed Apr 17, 2021
    @kousu
    Copy link
    Mannequin

    kousu mannequin commented Aug 8, 2021

    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":

    ssl_context = ssl._create_stdlib_context(certfile=certfile,

    which is actually "ssl._create_unverified_context":

    _create_stdlib_context = _create_unverified_context

    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:

    context.check_hostname = check_hostname

    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 :)

    @tiran
    Copy link
    Member Author

    tiran commented Aug 8, 2021

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

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 docs Documentation in the Doc dir expert-SSL stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants