Add untrusted-chain support to X509StoreContext

[akgood]

For X509StoreContext to verify any non-trivial (i.e. depth > 1) certificate chain, we need to be able to specify an additional set of certificates that OpenSSL can use for issuer lookups, but that are not ultimately trusted.

[codecov-io]

Codecov Report

Merging #473 into master will increase coverage by 0.01%.
The diff coverage is 95.38%.

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage   97.05%   97.06%   +0.01%     
==========================================
  Files          18       18              
  Lines        5703     5762      +59     
  Branches      395      401       +6     
==========================================
+ Hits         5535     5593      +58     
- Misses        112      113       +1     
  Partials       56       56

Impacted Files	Coverage Δ
tests/test_crypto.py	98.94% <100%> (+0.02%)	⬆️
src/OpenSSL/crypto.py	96.84% <89.65%> (-0.02%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57c2f03...a44dd2c. Read the comment docs.

[hynek]

First of all sorry that nobody got around to review this PR which obviously took a lot of work and seems like a very useful addition to pyOpenSSL!

I will try to help you get it in and maybe mobilize people who know more about this stuff.

In the meantime could you please:

• rebase
• add .. versionadded tags to changed APIs

Thank you and sorry.

[tiran]

Why do you create a duplicate of the cert instead of bumping the reference count by one? Duplication is inefficient. OpenSSL 1.1. has X509_up_ref(). For OpenSSL < 1.1 you can use something like CRYPTO_add(x->references, 1, CRYPTO_LOCK_X509).

It might be more efficient to create the stack once in set_chain() and then re-use the chain here.

[akgood]

I couldn't find a binding for X509_up_ref() in cryptography, and didn't want to make this diff depend on adding one. If this has changed, I'd be happy to fix it.

As for creating the stack in set_chain(), I think I considered that but ended up doing it this way because it was easier to reason about memory management (but, it's been a while...)

[hynek]

Generally speaking it’s okay to ask for bindings…just list them all on a comment on the ticket (i.e. not here which will vanish once the line changes :)) and we’ll see what can be done. We’re depending or rather recent versions anyway so it’s it’s OK.

[reaperhulk]

CI failure appears to be a bug in current twisted trunk. Filed here: https://twistedmatrix.com/trac/ticket/8779#ticket

[tiran]

I explained this to @hynek on IRC yesterday. I really hate to be the bringer of bad news, but here we go!

PyOpenSSL's X509StoreContext should not be used in production code ever. It's ok to use it in test code to manually verify a certificate. But please never use it to perform additional checks during a TLS handshake. Instead PyOpenSSL should give users access X509_STORE_CTX in the verify callback.

It's far from trivial to configure a X509_STORE_CTX correctly. For starters the verification parameters are a nested lookup chain with information from the SSL_CTX->param, SSL->param and X509_STORE_CTX->param. PyOpenSSL does not even expose all fields and setters to configure all flags. There are many more aspects like store lookups, callbacks, DANE.

X509StoreContext.verify_certificate() performs chain building, signature verification and X509 verification all over again. It doubles the amount of work. It slows down connections.

Validation after handshake is too late. You want to validate the cert chain before you send sensible information like a TLS client cert to a rogue server.

[Lukasa]

Is there any reason we cannot simply wrap the X509_STORE_CTX in the X509StoreContext object? Like by having a _from_cffi private constructor?

[tiran]

Sure, we could do that, but X509StoreContext is not particularly useful. https://pyopenssl.readthedocs.io/en/stable/api/crypto.html#x509storecontext-objects

[akgood]

Actually, my use-case here doesn't involve TLS handshakes at all; I needed this to validate some application-layer signatures.

Given that, I imagine someone might shortly chime in and say, it'd ultimately be better to have functionality like this live in cryptograpy.x509 than pyOpenSSL. I would completely agree with that sentiment! But, X509StoreContext already existed here, so adding untrusted certs to it was a much easier change than building up a new API from scratch...

[hynek]

Hmmm, how hard would it be to add it to cryptography and wire it back into pyOpenSSL? Nowadays we try to avoid adding code to pyOpenSSL unless necessary. @reaperhulk?

[reaperhulk]

This is one of those times where the pieces for this (a validation API) don't exist at all in cryptography, so pyOpenSSL is IMO the right place to add it for now.

[akgood]

What are we thinking in terms of next steps here (if any)?

I'd be happy to update the code to use a (to-be-added) binding for sk_x509_pop_free() rather than my custom loop.

I'm more ambivalent about X509_up_ref() - as @tiran points out this is only available in OpenSSL 1.1.0+ so somebody would probably have to create a "backported" version following the CUSTOMIZATIONS strategy e.g. in /~https://github.com/pyca/cryptography/blob/master/src/_cffi_src/openssl/x509.py#L398. This is perhaps more effort than I, personally, feel like spending here. (Maybe it's also worth mentioning that the use of X509_dup() for cases like this is precedented in pyOpenSSL, e.g. /~https://github.com/pyca/pyopenssl/blob/master/src/OpenSSL/SSL.py#L621)

[hynek]

My view here is that I’d love to have something this PR wants to provide (i.e. more verification options to mainly used outside TLS—which should be documented) but @tiran & @reaperhulk are the poor souls that “understand” OpenSSL so it’s kind of up to you three to make that happen.

It would be great if either could say whether there’s a way forward and if so, which one. :)

[gsmethells]

The following link led me to this pull request. It does not give me warm fuzzies about using pyOpenSSL in its current state:

https://mail.python.org/pipermail/cryptography-dev/2016-August/000676.html

And the lack of any action on this pull request in more than 6 months is also concerning. What happened here? Why is this pull request still open? Was this accidentally abandoned? Is there another pull request providing a solution instead?

Are untrusted intermediate certs supported in pyOpenSSL?

[hynek]

I think what happened follows from my last message here: ATM I know of two people who could help to get this merged. Neither is being paid for helping on this project so there is no expectation for them to do so. The path forward is basically to either find a way to convince them to spend their free time on something they don’t care about or to find someone else with appropriate OpenSSL skills.

[orosam]

I've already started working on the same functionality when I bumped into this PR (I should have looked earlier). Is there any chance of this getting merged? If it's any help, I'd gladly bring it up-to-date with the current master branch.

[reaperhulk]

@orosam if you're willing to take this over, update it, and bring it up to the standard of the other PR you submitted then I'll review it.

[orosam]

This PR can be closed as #948 has been merged.