Skip to content

/ cpython

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

bpo-18233: Add SSLSocket.getpeercertchain() #17938

Open
wants to merge 3 commits into
base: master
from
Open

bpo-18233: Add SSLSocket.getpeercertchain() #17938

wants to merge 3 commits into from
+243 −1

Conversation

@chrisburr
Copy link

chrisburr commented Jan 10, 2020 •
edited

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
@chrisburr chrisburr requested a review from tiran as a code owner Jan 10, 2020
blurb-it bot and others added 2 commits Jan 10, 2020
@blurb-it @chrisburr
📜🤖 Added by blurb_it.
09c580c
@chrisburr
bpo-18233: Add SSLSocket.getpeercertchain()
Loading status checks…
8586656 
Based on the patch provided by Christian Heimes (christian.heimes) and updated by Mariusz Masztalerczuk (mmasztalerczuk).
@chrisburr chrisburr force-pushed the chrisburr:fix-issue-18233 branch from 4f88c3d to 8586656 Jan 10, 2020
@njsmith

This comment has been minimized.

Sign in to view
Copy link
Contributor

njsmith commented Jan 10, 2020

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

This comment has been minimized.

Sign in to view
Copy link
Author

chrisburr commented Jan 10, 2020

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
bpo-18233: Always return peer certificate from SSLSocket.getpeercertc…
Loading status checks…
a5dc709 
…hain()
@chrisburr

This comment has been minimized.

Sign in to view
Copy link
Author

chrisburr commented Jan 13, 2020 •
edited

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiran tiran

Assignees
No one assigned
Labels
CLA signed awaiting review
Projects
None yet
Milestone
No milestone
4 participants
@chrisburr @njsmith @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.