Allow accessing a connection's verfied certificate chain #894

ShaneHarvey · 2020-03-03T19:00:03Z

Add X509StoreContext.get_verified_chain using X509_STORE_CTX_get1_chain.
Add Connection.get_verified_chain using SSL_get0_verified_chain if
available (ie OpenSSL 1.1+) and X509StoreContext.get_verified_chain
otherwise.

Motivation: PyMongo is implementing OCSP support and we're using PyOpenSSL to do so (thanks!). As part of the client's OCSP callback we would ideally have access to the verified peer certificate chain to find the peer's issuer cert. This change allows us to do that by exposing the verified peer certificate chain.

References:

https://www.openssl.org/docs/man1.1.0/man3/X509_STORE_CTX_get1_chain.html
https://www.openssl.org/docs/man1.1.0/man3/SSL_get0_verified_chain.html
The MongoDB server's polyfill implementation of SSL_get0_verified_chain for OpenSSL 1.0.1/1.0.2: https://github.com/mongodb/mongo/blob/r4.4.0/src/mongo/util/net/ssl_manager_openssl.cpp#L312-L327

Fixes #740.

ShaneHarvey · 2020-08-05T01:09:16Z

@alex @reaperhulk will this PR ever be considered? I'd be happy to rebase it.

reaperhulk

First round of review 😄

reaperhulk · 2020-08-05T20:18:58Z

src/OpenSSL/SSL.py

+            result = []
+            for i in range(_lib.sk_X509_num(cert_stack)):
+                # TODO could incref instead of dup here
+                cert = _lib.X509_dup(_lib.sk_X509_value(cert_stack, i))


Yeah let's X509_up_ref this instead of duping. Be sure to assert on the result for any call you make (X509_up_ref returns 1 on 1.1.0+ and the refcount in 1.0.2 I believe so it should probably be _openssl_assert(res >= 1).

Done. I also refactored this with get_peer_cert_chain.

reaperhulk · 2020-08-05T20:19:21Z

src/OpenSSL/SSL.py

+            return None
+
+        cert_stack = _lib.SSL_get_peer_cert_chain(self._ssl)
+        if cert_stack == _ffi.NULL:


When does this happen? If it should never happen we should do _openssl_assert

NULL is returned if the peer did not present a certificate: https://www.openssl.org/docs/man1.1.0/man3/SSL_get_peer_cert_chain.html

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). If the peer did not present a certificate, NULL is returned.

However, we already know that the peer did present a certificate because self.get_peer_certificate() is not None. I replaced this with _openssl_assert(cert_stack != _ffi.NULL).

src/OpenSSL/crypto.py

reaperhulk · 2020-08-05T20:23:07Z

src/OpenSSL/crypto.py

+        # Note: X509_STORE_CTX_get1_chain returns a deep copy of the chain.
+        cert_stack = _lib.X509_STORE_CTX_get1_chain(self._store_ctx)
+        if cert_stack == _ffi.NULL:
+            # TODO: This is untested.


If we can't trigger this branch use _openssl_assert

Done. I think _openssl_assert is correct since we already called X509_verify_cert successfully:
https://www.openssl.org/docs/man1.0.2/man3/X509_STORE_CTX_get1_chain.html

X509_STORE_CTX_get1_chain() returns a complete validate chain if a previous call to X509_verify_cert() is successful.

src/OpenSSL/crypto.py

reaperhulk · 2020-08-05T20:25:16Z

tests/test_ssl.py

+        chain = _create_certificate_chain()
+        [(cakey, cacert), (ikey, icert), (skey, scert)] = chain
+
+        serverContext = Context(TLSv1_METHOD)


Could you change all these to SSLv23_METHOD? TLSv1_METHOD won't work in Ubuntu 20.04 (which is what our CI just switched to) because TLS 1.0 is disabled. SSLv23_METHOD is an awful name but will allow up to TLS 1.3.

reaperhulk · 2020-08-05T20:27:36Z

For a little context here, we're generally reluctant to add new features to pyOpenSSL because we don't have the time/bandwidth to review things effectively and generally we want people using pyca/cryptography where possible. However, pyca/cryptography has no intention of having a TLS API any time soon so features like this are appropriate to build and review here. Even if we only review when we're feeling exceedingly guilty...

Add X509StoreContext.get_verified_chain using X509_STORE_CTX_get1_chain. Add Connection.get_verified_chain using SSL_get0_verified_chain if available (ie OpenSSL 1.1+) and X509StoreContext.get_verified_chain otherwise. Fixes pyca#740.

ShaneHarvey

Thanks @reaperhulk. This is ready for another look.

ShaneHarvey · 2020-08-05T20:29:37Z

src/OpenSSL/SSL.py

+            return None
+
+        cert_stack = _lib.SSL_get_peer_cert_chain(self._ssl)
+        if cert_stack == _ffi.NULL:


NULL is returned if the peer did not present a certificate: https://www.openssl.org/docs/man1.1.0/man3/SSL_get_peer_cert_chain.html

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). If the peer did not present a certificate, NULL is returned.

However, we already know that the peer did present a certificate because self.get_peer_certificate() is not None. I replaced this with _openssl_assert(cert_stack != _ffi.NULL).

src/OpenSSL/crypto.py

ShaneHarvey · 2020-08-05T21:21:15Z

tests/test_ssl.py

+        chain = _create_certificate_chain()
+        [(cakey, cacert), (ikey, icert), (skey, scert)] = chain
+
+        serverContext = Context(TLSv1_METHOD)


ShaneHarvey · 2020-08-05T21:27:09Z

src/OpenSSL/SSL.py

+            result = []
+            for i in range(_lib.sk_X509_num(cert_stack)):
+                # TODO could incref instead of dup here
+                cert = _lib.X509_dup(_lib.sk_X509_value(cert_stack, i))


Done. I also refactored this with get_peer_cert_chain.

src/OpenSSL/crypto.py

ShaneHarvey · 2020-08-05T21:42:54Z

src/OpenSSL/crypto.py

+        # Note: X509_STORE_CTX_get1_chain returns a deep copy of the chain.
+        cert_stack = _lib.X509_STORE_CTX_get1_chain(self._store_ctx)
+        if cert_stack == _ffi.NULL:
+            # TODO: This is untested.


Done. I think _openssl_assert is correct since we already called X509_verify_cert successfully:
https://www.openssl.org/docs/man1.0.2/man3/X509_STORE_CTX_get1_chain.html

X509_STORE_CTX_get1_chain() returns a complete validate chain if a previous call to X509_verify_cert() is successful.

reaperhulk · 2020-08-05T22:20:54Z

Tests are failing right now

reaperhulk · 2020-08-05T22:21:20Z

Please also add a CHANGELOG.rst entry for this new feature!

….X509 object at 0x7fdbb59daad0>

reaperhulk

A few final comments + please add a CHANGELOG entry

reaperhulk · 2020-08-05T22:41:42Z

src/OpenSSL/SSL.py

+            return None
+
+        pystorectx = X509StoreContext(pystore, pycert)
+        pystorectx._add_chain(cert_stack)


Let's just set _chain directly and remove the setter since it doesn't hold any logic.

reaperhulk · 2020-08-05T22:41:59Z

src/OpenSSL/crypto.py

        # Make the store context available for use after instantiating this
        # class by initializing it now. Per testing, subsequent calls to
        # :meth:`_init` have no adverse affect.
        self._init()

+    def _add_chain(self, chain):


This can be removed once the other comment is addressed.

src/OpenSSL/crypto.py

ShaneHarvey

Added a Changelog entry and removed _add_chain.

ShaneHarvey · 2020-08-05T23:35:04Z

src/OpenSSL/crypto.py

        # Make the store context available for use after instantiating this
        # class by initializing it now. Per testing, subsequent calls to
        # :meth:`_init` have no adverse affect.
        self._init()

+    def _add_chain(self, chain):


ShaneHarvey · 2020-08-05T23:35:10Z

src/OpenSSL/SSL.py

+            return None
+
+        pystorectx = X509StoreContext(pystore, pycert)
+        pystorectx._add_chain(cert_stack)


reaperhulk · 2020-08-05T23:48:59Z

Thanks for working with us!

ShaneHarvey mentioned this pull request Mar 3, 2020

Retrieve chain after certificate validation - what should API look like? #740

Closed

reaperhulk reviewed Aug 5, 2020

View reviewed changes

ShaneHarvey added 4 commits August 5, 2020 13:41

TLSv1_METHOD -> SSLv23_METHOD

6851254

Use X509_up_ref instead of X509_dup

263c08c

Add _openssl_assert where appropriate

ba78fb2

ShaneHarvey force-pushed the add_verified_chain branch from 9d1a99e to ba78fb2 Compare August 5, 2020 21:44

ShaneHarvey added 2 commits August 5, 2020 14:50

SSL_get_peer_cert_chain should not be null

0e07516

Reformat with black

87f19e3

ShaneHarvey commented Aug 5, 2020

View reviewed changes

ShaneHarvey added 2 commits August 5, 2020 15:30

Fix <OpenSSL.crypto.X509 object at 0x7fdbb59e8050> != <OpenSSL.crypto…

6d11a89

….X509 object at 0x7fdbb59daad0>

Add Changelog entry

cc951ec

reaperhulk reviewed Aug 5, 2020

View reviewed changes

Remove _add_chain

63b0401

ShaneHarvey commented Aug 5, 2020

View reviewed changes

reaperhulk approved these changes Aug 5, 2020

View reviewed changes

reaperhulk merged commit 33c5499 into pyca:master Aug 5, 2020

ShaneHarvey deleted the add_verified_chain branch August 6, 2020 00:28

github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow accessing a connection's verfied certificate chain #894

Allow accessing a connection's verfied certificate chain #894

ShaneHarvey commented Mar 3, 2020 •

edited

Loading

ShaneHarvey commented Aug 5, 2020 •

edited

Loading

reaperhulk left a comment

reaperhulk Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk commented Aug 5, 2020

ShaneHarvey left a comment

ShaneHarvey Aug 5, 2020

ShaneHarvey Aug 5, 2020

ShaneHarvey Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk commented Aug 5, 2020

reaperhulk commented Aug 5, 2020

reaperhulk left a comment

reaperhulk Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk Aug 5, 2020

ShaneHarvey Aug 5, 2020

ShaneHarvey left a comment

ShaneHarvey Aug 5, 2020

ShaneHarvey Aug 5, 2020

reaperhulk commented Aug 5, 2020

Allow accessing a connection's verfied certificate chain #894

Allow accessing a connection's verfied certificate chain #894

Conversation

ShaneHarvey commented Mar 3, 2020 • edited Loading

ShaneHarvey commented Aug 5, 2020 • edited Loading

reaperhulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk commented Aug 5, 2020

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk commented Aug 5, 2020

reaperhulk commented Aug 5, 2020

reaperhulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk commented Aug 5, 2020

ShaneHarvey commented Mar 3, 2020 •

edited

Loading

ShaneHarvey commented Aug 5, 2020 •

edited

Loading