[RFC] Implement X.509 certificate verification

[KonstantinShemyak]

This is a Request-For-Comments implementation of verifying X.509 certificate against set of intermediate CA certificates and set of trusted CA certificates.

It provides the following API:

certificate.verify(intermediates, trusted_roots, check_callback)

If a trusted chain cannot be built, NoValidTrustChain exception is raised. Otherwise, the method returns list of trust chains (lists of Certificate objects), each starting with self and ending in one of trusted_roots.

The interface is an attempt to get the best of "the new" and "the old" worlds:

• Go verify.
• OpenSSL verify.

The goal is to balance between principles "do not provide unsafe defaults" and "do not make The Right Thing too complicated". Differently from Go, the method:

• Takes no verification options in arguments, as the former can be arbitrarily complex. Any additional checks must be done in the callback.
• Does not by default trust to any "system trust roots", requiring the caller to pass them explicitly
  • Note: does cryptography provide an easy way for the user to do so if needed, like OpenSSL's SSL_CTX_set_default_verify_paths()?

Differently from OpenSSL, the method:

• Requires to pass the single-certificate verification callback (not just providing possibility to call X509_STORE_CTX_set_verify_cb()).
• The callback receives the full chain to which the checked certificate belongs and the index of the latter in the chain, not the fat context like int verify_callback(int ok, X509_STORE_CTX *ctx).

Remaining tasks which should be done:

• Understand what more testing is necessary and write corresponding tests.
• Update the documentation.
• Provide example callbacks which would, probably in addition to the default callback:
  • check KU/EKU extensions
  • perform CRL/OCSP checks
• Additions are pure Python and do not depend on any backend, but quickly looking I have not seen a way to implement the method in x509.base.Certificate. So I have put it the quick-and-dirty way into hazmat.backends.openssl.x509._Certificate, which is probably not the way it should be done.

Closes #2381 .

[ronf]

The callback receives the full chain to which the checked certificate belongs and the index of the latter in the chain, not the fat context like int verify_callback(int ok, X509_STORE_CTX *ctx).

Does this mean that the verify callback will be called sequentially on each certificate in a chain? If you are passing the full chain each time, why not have the callback verify the whole chain rather than one certificate at a time? If multiple chains can be built, I could see calling the callback on each of those possible chains until the callback returns that one of them is valid, but I'm not sure why you'd want to call the callback multiple times for each chain, passing in the index of the cert being verified, rather than letting the callback do this loop.

Is the callback expected to do all the checks itself, even for basic stuff like KU/EKU, CA flag, valid before/after? If so, I think we're definitely going to want cryptography to export implementations of these common checks, so people don't have to implement all of them themselves (and most likely get them wrong). While I can see the argument that trying to provide arguments for all the possible verification overrides could get very complicated, I don't think providing only "example" callback functions the right place to land there. Even if the caller is required to provide a callback, cryptography should at least provide solid implementations of each check as a library call that the callbacks can use for common things like checking basic constraints, key usage, expiration, revocation, and subject alt name checks. Something like EKU or subject alt name checks will require the caller to specify the values they want to match against, but there'd still be value in providing a standard function here I think, particularly for SAN where you have to deal with matching against IPv4 and IPv6 addresses in binary form along with string hostnames in many cases.

In AsyncSSH today, I'm actually using a mix of PyCA X509 code and pyOpenSSL code, with cert validation specifically requiring me to convert from a PyCA cert to a pyOpenSSL one (via crypto.X509.from_cryptography). I do this only to get at pyOpenSSL's chain building (in crypto.X509Store/X509StoreContext). It looks like this new API will provide that, and I'd love to get rid of the pyOpenSSL dependency, but right now I'm expecting pyOpenSSL to do the checking of attributes on all the intermediate certs during the chain building, and I don't have any callback defined for that. I do check other attributes of the end-entity certificate myself, but that's not done through a callback.

[baloo]

I might be missing something, but I believe we do not check the signature of the current cert with the issuer public key here which I believe should guard the add_edge call.

[baloo]

I'd think that would do the job instead (roughly tested):

        for cert in all_certs:
            issuer_name = cert.issuer
            for issuer_candidate in all_certs:
                if issuer_candidate.subject == issuer_name:
                    pkey = issuer_candidate.public_key()

                    try:
                        if isinstance(pkey, rsa.RSAPublicKeyWithSerialization):
                            pkey.verify(
                                signature=cert.signature,
                                data=cert.tbs_certificate_bytes,
                                padding=padding.PKCS1v15(),
                                algorithm=cert.signature_hash_algorithm,
                            )
                        elif isinstance(
                                pkey,
                                ec.EllipticCurvePublicKeyWithSerialization):
                            pkey.verify(
                                signature=cert.signature,
                                data=cert.tbs_certificate_bytes,
                                signature_algorithm=cert
                                .signature_hash_algorithm,
                            )
                        elif isinstance(
                                pkey, dsa.DSAPublicKeyWithSerialization):
                            pkey.verify(
                                signature=cert.signature,
                                data=cert.tbs_certificate_bytes,
                                algorithm=cert.signature_hash_algorithm,
                            )
                        else:
                            raise UnsupportedAlgorithm(
                                '{} verification is not implemented'
                                .format(pkey.__class__)
                            )

                        g.add_edge(cert, issuer_candidate)

                    except InvalidSignature:
                        pass

[KonstantinShemyak]

Definitely so! Thank you for the code sketch!
I decided that this check deserves function on its own and implemented it in #5116. Will update this PR when the latter (hopefully) will be merged. (The code requried minor change for EC key, verify() in that case requires signature_algorithm being instance of ec.ECDSA.)

[KonstantinShemyak]

@ronf Thank you for the deep comments!

Does this mean that the verify callback will be called sequentially on each certificate in a chain? If you are passing the full chain each time, why not have the callback verify the whole chain rather than one certificate at a time?

You are correct. There is no reason to call the callback for each certificate, not for the whole chain.

My thought was that "API users are more used to work with certificates, not with chains", but now this looks like a moot justification as the API user will receive the chain anyway (and very likely will have to handle it, as for example requirements on KU/EKU bits are probably different for leaf certificates, intermediate certificates and root certificates).

The next version of the PR will pass the whole chain to the callback as you suggest.

Is the callback expected to do all the checks itself, even for basic stuff like KU/EKU, CA flag, valid before/after? If so, I think we're definitely going to want cryptography to export implementations of these common checks, so people don't have to implement all of them themselves (and most likely get them wrong).

Absolutely so! This is work-in-progress. (I'm especially thrilled by CRL/OCSP checks 🙄 Pointers to "industry practices" to related decision making examples are welcome.)

[KonstantinShemyak]

@baloo What is the practice in the project - I'd love to cleanup the commits, "hide the sausage making" by squashing the review comments into their "parents", and force-push. This will, naturally, require additional work from the reviewer. Happy to follow the conventions.

[baloo]

@KonstantinShemyak I never contributed to this project, I'm afraid I can't help you with that :( I was in need of x509 chain validation and merely passing by :)

[KonstantinShemyak]

Branch force-pushed with the following changes:

• Corrections suggested by @baloo squashed to their corresponding commits
  • With one exception: I'm not (yet?) silently calling the default certificate verification callback in case when none is passed by the user. I want to hear a word from project maintainers on this, and also that default callback must be ready (it is not).
• The branch is based on WIP: Add boolean Certificate.is_issued_by(issuer) #5116 .

[reaperhulk]

I haven't reviewed this yet (and probably won't be able to until after I find the time to fix the absurdity of our CI yet again, sigh), but you may find https://github.com/alex/x509-validator interesting as it represents a partial attempt at solving this that we never finished.

One big item: validation like this is obnoxiously tricky so any validator should pull in very large test vector sets to gain some confidence in the edges.

[kislyuk]

@reaperhulk would be great if someone could review this - it's blocking those who want to move off of PyOpenSSL.

[jvanasco]

I'm not sure if you are still working on this or not. If so, it would be great if this were able to handle partial chains, like openssl verify -partial_chain does.

For example, if a certificate chain is:

 `???` > `intermediate_1` > `intermediate_2` >`certificate`

OpenSSL commandline supports:

openssl verify -partial_chain -trusted intermediate_1.pem intermediate_2.pem
openssl verify -partial_chain -trusted intermediate_2.pem certificate.pem

Ideally, the following test cases would pass, without needing the self-signed root ???.

# certificate.verify(intermediates, trusted_roots, check_callback)
certificate.verify([intermediate_2,], [intermediate_1], )
intermediate_2.verify(None, [intermediate_1], )

[KonstantinShemyak]

@jvanasco I have not updated the branch since long time, but it is supposed to do exactly what you expect:

For example, if a certificate chain is:

 `???` > `intermediate_1` > `intermediate_2` >`certificate`

[...]

Ideally, the following test cases would pass, without needing the self-signed root ???.

# certificate.verify(intermediates, trusted_roots, check_callback)
certificate.verify([intermediate_2,], [intermediate_1], )
intermediate_2.verify(None, [intermediate_1], )

Both sketched tests would pass. It is nowhere required that the trusted root be self-signed. (It could be required in check_callback if desired so by the user of the API.)

[tiran]

For example, if a certificate chain is:

 `???` > `intermediate_1` > `intermediate_2` >`certificate`

Pet peeve: The term "certificate chain" is an oversimplification and only applies in the most trivial case. Path building is much more complicated and messy. For Let's Encrypt uses cross-signing with multiple possible trust paths. https://tools.ietf.org/html/rfc4158 has some great ASCII art for cross-signing and trust meshes.

Both sketched tests would pass. It is nowhere required that the trusted root be self-signed.

Actually it depends on the implementation and configuration. For example OpenSSL never trusts intermediate certificate in the trust store unless X509_VERIFY_PARAM_set_flags flag X509_V_FLAG_PARTIAL_CHAIN is set.

[jvanasco]

Pet peeve: The term "certificate chain" is an oversimplification and only applies in the most trivial case. Path building is much more complicated and messy. For Let's Encrypt uses cross-signing with multiple possible trust paths.

In my example, the intent is that we are testing the integrity of an identified chain. A lot of other developers have fought with this same problem for many years. Path building and validating trust are discrete needs which are unrelated to verifying the integrity of chains or partial chains.

If you want to make your letsEncrypt example
Even more complex, I had not realized something about letsencrypt until a few days ago- their x3 and x4 intermediates reused the same keys as x1 and x2, doubling the possible trust paths of certificates issued in that period (all 4 intermediates have been retired).

[KonstantinShemyak]

Pet peeve: The term "certificate chain" is an oversimplification and only applies in the most trivial case. Path building is much more complicated and messy. For Let's Encrypt uses cross-signing with multiple possible trust paths. https://tools.ietf.org/html/rfc4158 has some great ASCII art for cross-signing and trust meshes.

Definitely so, and the proposed implementation accounts for this. It's the reason to build the directed graph.

Both sketched tests would pass. It is nowhere required that the trusted root be self-signed.

Actually it depends on the implementation and configuration. For example OpenSSL never trusts intermediate certificate in the trust store unless X509_VERIFY_PARAM_set_flags flag X509_V_FLAG_PARTIAL_CHAIN is set.

@tiran The proposed implementation, as I said, does not care is the trusted certificate self-signed or not. I think it makes more sense than the described behavior of OpenSSL. If you see the reasons why the latter behavior should be the default, please tell. (But "because OpenSSL does so" would not be accepted as valid argument 😄)

[kislyuk]

@reaperhulk @KonstantinShemyak is there anything I can do to help this along? It looks like the PR needs to be rebased.

[KonstantinShemyak]

The branch is rebased on top of (also rebased and updated) #5116 . The latter is valuable by itself (not only as a prerequisite for this PR). In fact yet another PR doing the same has been submitted (#5367), I have justified my decisions in the parts where they differ.

This PR is still RFC, my plan is to implement suggestions by @ronf, add tests and add a number of example policy callbacks for verification.

[reaperhulk]

This is in progress and there are several PRs that have already landed (with many more to come). Thanks for the work you did originally on this -- even though we didn't ultimately land this it helped us understand the demand for this!

[jvanasco]

This is in progress and there are several PRs that have already landed (with many more to come). Thanks for the work you did originally on this -- even though we didn't ultimately land this it helped us understand the demand for this!

@reaperhulk Would it be possible to create an Issue that correlates all these things together? For those of us that need this functionality, that would make it much easier to monitor for when a fix lands.

[reaperhulk]

@jvanasco here you go! #9812

[jvanasco]

@reaperhulk Thank you so much!