Allow accessing a connection's verfied certificate chain by ShaneHarvey · Pull Request #894 · pyca/pyopenssl (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of review 😄
| 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)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also refactored this with get_peer_cert_chain.
| return None |
|---|
| cert_stack = _lib.SSL_get_peer_cert_chain(self._ssl) |
| if cert_stack == _ffi.NULL: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this happen? If it should never happen we should do _openssl_assert
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| # 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can't trigger this branch use _openssl_assert
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| chain = _create_certificate_chain() |
|---|
| [(cakey, cacert), (ikey, icert), (skey, scert)] = chain |
| serverContext = Context(TLSv1_METHOD) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @reaperhulk. This is ready for another look.
| return None |
|---|
| cert_stack = _lib.SSL_get_peer_cert_chain(self._ssl) |
| if cert_stack == _ffi.NULL: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| chain = _create_certificate_chain() |
|---|
| [(cakey, cacert), (ikey, icert), (skey, scert)] = chain |
| serverContext = Context(TLSv1_METHOD) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| 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)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also refactored this with get_peer_cert_chain.
| # 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing right now
Please also add a CHANGELOG.rst entry for this new feature!
….X509 object at 0x7fdbb59daad0>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few final comments + please add a CHANGELOG entry
| return None |
|---|
| pystorectx = X509StoreContext(pystore, pycert) |
| pystorectx._add_chain(cert_stack) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just set _chain directly and remove the setter since it doesn't hold any logic.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| # :meth:`_init` have no adverse affect. |
|---|
| self._init() |
| def _add_chain(self, chain): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed once the other comment is addressed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a Changelog entry and removed _add_chain.
| # :meth:`_init` have no adverse affect. |
|---|
| self._init() |
| def _add_chain(self, chain): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| return None |
|---|
| pystorectx = X509StoreContext(pystore, pycert) |
| pystorectx._add_chain(cert_stack) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thanks for working with us!
2 participants
