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

Expose OpenSSL verification results in SSLError #72369

Closed
yan12125 mannequin opened this issue Sep 16, 2016 · 15 comments
Closed

Expose OpenSSL verification results in SSLError #72369

yan12125 mannequin opened this issue Sep 16, 2016 · 15 comments
Assignees
Labels
3.7 (EOL) end of life topic-SSL type-feature A feature request or enhancement

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented Sep 16, 2016

BPO 28182
Nosy @tiran, @alex, @dstufft, @yan12125
PRs
  • bpo-28182: Expose OpenSSL verification results #3412
  • bpo-28182: restore backwards compatibility #3464
  • Files
  • expose-x509-verify-result.patch
  • ssl_certverifyerror.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 2017-09-08.19:03:23.190>
    created_at = <Date 2016-09-16.15:06:43.582>
    labels = ['expert-SSL', 'type-feature', '3.7']
    title = 'Expose OpenSSL verification results in SSLError'
    updated_at = <Date 2017-09-08.21:48:01.766>
    user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2017-09-08.21:48:01.766>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2017-09-08.19:03:23.190>
    closer = 'christian.heimes'
    components = ['SSL']
    creation = <Date 2016-09-16.15:06:43.582>
    creator = 'yan12125'
    dependencies = []
    files = ['44741', '44742']
    hgrepos = []
    issue_num = 28182
    keywords = ['patch']
    message_count = 15.0
    messages = ['276724', '276898', '276904', '276906', '276914', '276951', '277030', '277033', '277037', '277039', '277042', '277449', '301718', '301719', '301741']
    nosy_count = 5.0
    nosy_names = ['janssen', 'christian.heimes', 'alex', 'dstufft', 'yan12125']
    pr_nums = ['3412', '3464']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue28182'
    versions = ['Python 3.7']

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 16, 2016

    This was originally a post at python-ideas. Now I reformat it to be more like a feature request.

    Currently, Python raises SSLError with reason=CERTIFICATE_VERIFY_FAILED for all kinds of certificate verification failures. This results in difficulties in debugging SSL errors for others. (Some downstream bug reports: [1][2]) In OpenSSL, such errors are further divided into several kinds. For example, expired certificates result in X509_V_ERR_CERT_HAS_EXPIRED, and typical self-signed certificates fall into X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT. The error code can be retrieved via SSL_get_verify_result and human readable messages are available from X509_verify_cert_error_string. I hope I can get error messages like this: (Omit URLError to avoid verbose messages)

    $ ./python -c 'import urllib.request; urllib.request.urlopen("https://self-signed.badssl.com/")'
    Traceback (most recent call last):
      File "/home/yen/Projects/cpython/Lib/urllib/request.py", line 1318, in do_open
        encode_chunked=req.has_header('Transfer-encoding'))
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1239, in request
        self._send_request(method, url, body, headers, encode_chunked)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1285, in _send_request
        self.endheaders(body, encode_chunked=encode_chunked)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1234, in endheaders
        self._send_output(message_body, encode_chunked=encode_chunked)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1026, in _send_output
        self.send(msg)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 964, in send
        self.connect()
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1400, in connect
        server_hostname=server_hostname)
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 401, in wrap_socket
        _context=self, _session=session)
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 808, in __init__
        self.do_handshake()
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 1061, in do_handshake
        self._sslobj.do_handshake()
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 683, in do_handshake
        self._sslobj.do_handshake()
    ssl.SSLError: [SSL: CERTIFICATE_VERIFY_FAILED: DEPTH_ZERO_SELF_SIGNED_CERT] certificate verify failed: self signed certificate (_ssl.c:752)

    And for expired certificates:

    $ ./python -c 'import urllib.request; urllib.request.urlopen("https://expired.badssl.com/")'
    Traceback (most recent call last):
      File "/home/yen/Projects/cpython/Lib/urllib/request.py", line 1318, in do_open
        encode_chunked=req.has_header('Transfer-encoding'))
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1239, in request
        self._send_request(method, url, body, headers, encode_chunked)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1285, in _send_request
        self.endheaders(body, encode_chunked=encode_chunked)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1234, in endheaders
        self._send_output(message_body, encode_chunked=encode_chunked)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1026, in _send_output
        self.send(msg)
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 964, in send
        self.connect()
      File "/home/yen/Projects/cpython/Lib/http/client.py", line 1400, in connect
        server_hostname=server_hostname)
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 401, in wrap_socket
        _context=self, _session=session)
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 808, in __init__
        self.do_handshake()
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 1061, in do_handshake
        self._sslobj.do_handshake()
      File "/home/yen/Projects/cpython/Lib/ssl.py", line 683, in do_handshake
        self._sslobj.do_handshake()
    ssl.SSLError: [SSL: CERTIFICATE_VERIFY_FAILED: CERT_HAS_EXPIRED] certificate verify failed: certificate has expired (_ssl.c:752)

    I've once tried to achieve it, but my CPython knowledge is way too limited to give a good enough patch.

    [1] ytdl-org/youtube-dl#10574
    [2] ytdl-org/youtube-dl#7309

    @yan12125 yan12125 mannequin added the 3.7 (EOL) end of life label Sep 16, 2016
    @yan12125 yan12125 mannequin assigned tiran Sep 16, 2016
    @yan12125 yan12125 mannequin added topic-SSL type-feature A feature request or enhancement labels Sep 16, 2016
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 18, 2016

    Here's a quick try. I didn't add tests and update docs as it's my first serious patch to CPython and I'm not sure whether my approach is OK or not.

    @tiran
    Copy link
    Member

    tiran commented Sep 18, 2016

    Good work! I completely forgot that the SSL object holds the last verification error in its struct. This allows the ssl module to print some information when cert verification fails. It's still not perfect, because it is missing information about the the failing certificate. It's much better than no reason at all.

    I took your patch and simplified it a bit. I removed the sub reason attribute, too. We can add it later and use an enum.IntEnum instead of the old hack.

    ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:766)

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 18, 2016

    That looks much better. I should have create a subclass of SSLError.

    Here's a minor concern: fill_and_set_sslerror adds a new argument for verification errors. If someone else wants to support more errors, this function would have more arguments, which sounds bad for me - or we can postpone discussions until there's really such a need?

    @tiran
    Copy link
    Member

    tiran commented Sep 18, 2016

    You don't have to be concerned about additional arguments. fill_and_set_sslerror() is an internal helper function. In fact it's a helper function for two other helper functions. Let's postpone the discussion until the argument sizes grows out of proportion.

    @tiran
    Copy link
    Member

    tiran commented Sep 19, 2016

    What do we do about ssl.CertificateError? It's not a subclass of SSLError and raised by match_hostname().

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 20, 2016

    With this change: (tested with OpenSSL git-master)

    @@ -632,20 +651,22 @@ newPySSLSocket(PySSLContext *sslctx, PyS
             SSL_set_bio(self->ssl, inbio->bio, outbio->bio);
         }
         mode = SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER;
     #ifdef SSL_MODE_AUTO_RETRY
         mode |= SSL_MODE_AUTO_RETRY;
     #endif
         SSL_set_mode(self->ssl, mode);
     
    +    if (server_hostname != NULL) {
     #if HAVE_SNI
    -    if (server_hostname != NULL)
             SSL_set_tlsext_host_name(self->ssl, server_hostname);
     #endif
    +        SSL_set1_host(self->ssl, server_hostname);
    +    }
     
         /* If the socket is in non-blocking mode or timeout mode, set the BIO
          * to non-blocking mode (blocking is the default)
          */
         if (sock && sock->sock_timeout >= 0) {
             BIO_set_nbio(SSL_get_rbio(self->ssl), 1);
             BIO_set_nbio(SSL_get_wbio(self->ssl), 1);
         }

    When connecting to https://wrong.host.badssl.com/, the error is:
    ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch (_ssl.c:768)

    With this change in mind, an idea is to drop the Python implementation of match_hostname and rely on OpenSSL's checking mechanism (do_x509_check). As a result:

    • ssl.CertificateError can be either an alias of ssl.SSLCertVerificationError or a subclass of it
    • When verify_result is X509_V_ERR_HOSTNAME_MISMATCH, the error message is formatted with more information following the current approach in match_hostname ("hostname XXX doesn't match YYY...")

    @tiran
    Copy link
    Member

    tiran commented Sep 20, 2016

    Yes, I'm planning to use the feature in 3.7. First I have to finish my PEP and get consents that I can drop support for OpenSSL 1.0.1 and earlier. We still support older versions but the feature is only available in 1.0.2+. I also need to come up with a solution to check IP addresses, too. It requires a different function.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 20, 2016

    That's great. OpenSSL plans to drop 1.0.1 branch support after 2016/12/31. [1] I guess it's OK to drop 1.0.1 support in 3.7.

    Thanks for constantly improving SSL/TLS support in CPython!

    [1] https://www.openssl.org/source/

    @tiran
    Copy link
    Member

    tiran commented Sep 20, 2016

    I'm familiar with the release cycles of OpenSSL. In fact I want to tie support for OpenSSL versions to the release cycle of OpenSSL. Python core dev is a bit ... special. :) I can't just drop support. Some developers are opposing my plans and want to keep support for OpenSSL 1.0.1 and earlier.

    Enjoy this mail thread: https://mail.python.org/pipermail/python-dev/2016-August/145907.html

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 20, 2016

    I'm familiar with the release cycles of OpenSSL.

    Oh I shouldn't say something trivial :)

    I know that thread. Hope I can help something on persuading others.

    @tiran
    Copy link
    Member

    tiran commented Sep 26, 2016

    For hostname verification it might be a good idea to add a replacement for ssl.CertificateError.

    @tiran
    Copy link
    Member

    tiran commented Sep 8, 2017

    New changeset b3ad0e5 by Christian Heimes in branch 'master':
    bpo-28182: Expose OpenSSL verification results (bpo-3412)
    b3ad0e5

    @tiran
    Copy link
    Member

    tiran commented Sep 8, 2017

    The ssl module now reports cause of validation failure:

    >>> import ssl
    >>> import ssl, socket
    >>> ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
    >>> sock = ctx.wrap_socket(socket.socket(), server_hostname='www.python.org')
    >>> sock.connect(('www.python.org', 443))
    Traceback (most recent call last):
    ...
    ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:825)

    @tiran tiran closed this as completed Sep 8, 2017
    @tiran
    Copy link
    Member

    tiran commented Sep 8, 2017

    New changeset 0915360 by Christian Heimes in branch 'master':
    bpo-28182: restore backwards compatibility (bpo-3464)
    0915360

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant