bpo-18233: Add SSLSocket.get_verified_chain() and SSLSocket.get_unverified_chain()

[chrisburr]

Based on the patch provided by Christian Heimes (christian.heimes) and updated by Mariusz Masztalerczuk (mmasztalerczuk). Updated to use SSL_get0_verified_chain in OpenSSL 1.1 as suggested by Jörn Heissler (joernheissler).

Tested with both OpenSSL 1.0.2 and 1.1.1 using the included test and:

import ssl
import socket
ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
ctx.load_default_certs()
good_hosts = [
    'google.com',
    'example.com',
]
bad_hosts = [
    'expired.badssl.com',
    'self-signed.badssl.com',
    'untrusted-root.badssl.com',
]
for host in good_hosts + bad_hosts:
    print('host =', host)
    ctx.verify_mode = ssl.CERT_NONE
    with ctx.wrap_socket(socket.socket(socket.AF_INET), server_hostname=host) as s:
        s.connect((host, 443))
        chain_no_validate = s.getpeercertchain(validate=False)
        try:
            chain_validate = s.getpeercertchain(validate=True)
        except Exception as e:
            if host in bad_hosts:
                print('Failed as expected with', e)
            else:
                raise Exception('Failed to validate good host')
        else:
            if host not in good_hosts:
                raise Exception('Managed to validate bad host')

https://bugs.python.org/issue18233

[njsmith]

I like that there's the option to get both the raw chain and the verified chain – these are different things and both are important for different purposes. But the docs should be clearer about the difference.

Maybe fetching the verified chain should only be supported when building against openssl 1.1.1? That would simplify this patch a lot, and follow the general trend of trying to get rid of custom cert verification logic in the ssl module.

For the unverified chain, the openssl docs say:

SSL_get_peer_cert_chain() returns a pointer to STACK_OF(X509) certificates forming the certificate chain sent by the peer. If called on the client side, the stack also contains the peer's certificate; if called on the server side, the peer's certificate must be obtained separately using SSL_get_peer_certificate(3).

I think this is confusing, and we should hide the confusing bit from python users. My preference would be for the python wrapper to always return the complete cert chain, including the leaf. So when necessary, the wrapper should manually call SSL_get_peer_certificate and add the leaf cert.

[chrisburr]

Thanks for taking a look so promptly.

Maybe fetching the verified chain should only be supported when building against openssl 1.1.1? That would simplify this patch a lot, and follow the general trend of trying to get rid of custom cert verification logic in the ssl module.

I'm very happy to do that if it's acceptable to only partially support openssl 1.0.2 and throw an exception if verified=True.

I think this is confusing, and we should hide the confusing bit from python users. My preference would be for the python wrapper to always return the complete cert chain, including the leaf. So when necessary, the wrapper should manually call SSL_get_peer_certificate and add the leaf cert.

I definitely agree, I'll implement it soon 👍

[chrisburr]

Maybe fetching the verified chain should only be supported when building against openssl 1.1.1? That would simplify this patch a lot, and follow the general trend of trying to get rid of custom cert verification logic in the ssl module.

I'm very happy to do that if it's acceptable to only partially support openssl 1.0.2 and throw an exception if verified=True.

I implemented it in df65d40 but I've changed my mind as it makes using getpeercertchain clunky. I don't think it should error in OpenSSL 1.0.2 with it's default arguments and I also don't think validate=False should be the default. If people disagree I'll cherry pick df65d40 into this PR.

[tiran]

OpenSSL 1.0.2 and 1.1.0 have reached EOL and are no longer supported by upstream. Please don't add workarounds / backports for these versions. Just make sure that Python can still be compiled with 1.0.2.

IMO there should be two new methods, one to get the raw peer cert chain and another one to get the "verified" chain. I put "verified" in quotes because the term is misleading. SSL_get0_verified_chain() can return a valid chain although the SSLSocket is configured to not require a valid chain and the verify result is not X509_V_OK. On the other hand the function can return NULL in combination with X509_V_OK, e.g. when a session is resumed.

Internally OpenSSL always builds and validates the chain, even with SSL_VERIFY_NONE. The flag SSL_VERIFY_NONE merely suppressed non-fatal validation errors.

I also like to get rid of binary madness and finally introduce proper certificate objects. I have a working implementation on my disk, but it needs a bit of polishing.

[tiran]

That's not necessarily correct. SSL_get_peer_cert_chain() returns whatever the client sends. It may just be the EE cert (end entity), EE+intermediates, EE+intermediate+root, EE with intermediates for primary and alternative chains, or whatever the admin for the site has configured. I guess the peer chain can even include unrelated junk.

It's also worth mentioning that the chain is not available with TLS session resumption.

[tiran]

SSL_R_CERTIFICATE_VERIFY_FAILED is always defined on 1.1.0+.

[tiran]

peer_chain can be NULL.

[tiran]

peer_chain can be NULL here.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[tiran]

By the way LibreSSL doesn't have SSL_get0_verified_chain(), too.

[chrisburr]

Thanks for the review @tiran!

IMO there should be two new methods, one to get the raw peer cert chain and another one to get the "verified" chain. I put "verified" in quotes because the term is misleading.

By the way LibreSSL doesn't have SSL_get0_verified_chain(), too.

That makes sense. I have two follow up questions:

1. Should I use snake case for the method names, i.e. getpeercertchain or get_peer_cert_chain? It seems like newer methods on SSLSocket do (e.g. get_channel_binding) but older ones like getpeercert don't.
2. If I split this addition and add a getverifiedchain method, is it acceptable to always raise an exception when using LibreSSL?

[tiran]

1. Please use the newer style get_egg_spam.
2. In case of OpenSSL 1.0.2 and LibreSSL the method should not be present. This allows users to perform a feature check with hasattr(). If you surround the whole C function with #ifdef then argument clinic does the rest for you.

[chrisburr]

Thanks for the quick reply. I've made the changes and tested them with both OpenSSL 1.0 and 1.1 and it behaves as you propose.

For the sake of the bot: I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

[njsmith]

You know, looking at this again today, I'd suggest naming the two methods: get_unverified_chain and get_verified_chain (or maybe get_(un)verified_peer_cert_chain or similar, depending on how verbose we want to be). The point is that if you're reading some code and see a call to the unverified method, we want it to be immediately obvious that it's unverified :-) If we call the method get_peer_cert_chain then it's easier to accidentally slip through an audit.

[chrisburr]

@tiran Sorry to pester you, but could you take another look when you get a chance?

[chrisburr]

@tiran Hi again, would you be able to take another look at this? I'd like to get it in before the 3.9 feature freeze if possible.

[dustin-johnson]

I stumbled on this pull request in my search for exactly this functionality. Is there anything I can do to help get this across the line?

[chrisburr]

I've just rebased to resolve a conflict from the clinic generated hash at the end of Modules/clinic/_ssl.c.h.

I'm also still very keen to get this in so I'm happy to do anything that is needed.

[chaen]

Just an extra push for this PR that I am really eager to see merged as well

[njsmith]

I poked @tiran about this PR on IRC just now, and he said:

• he wants to first add a (minimal) "certificate object" class, before adding new APIs that return raw PEM strings
• he has a draft implementation that needs some polish, and he plans to get to it before the 3.10 feature freeze
• but in case that gets delayed, he said: "If I don't get the certificate class landed before 3.10 beta freeze I'll merge the PR."

Hmm, and this makes me wonder: @tiran, do you want any help on finishing and landing the certificate object code? It seems like some folks here would be eager to help...

[chrisburr]

Hey @tiran, has there been any progress with the new certificate objects? I'm still happy to help out if it's useful.

[sigmavirus24]

@chrisburr I've been looking into this. https://github.com/python/cpython/blob/main/Modules/_ssl/cert.c has the new certificate class as far as I can tell. If you can get an SSLSocket object from the ssl library and do socket._sslobj.get_verified_chain() you'll see a list of them returned and calling get_info() on them gives you a similarly structured dictionary as getpeercert() does on SSLSocket.

[felixfontein]

It seems another PR got merged that provides a similar functionality: #109113