Skip to content
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

Add untrusted-chain support to X509StoreContext #473

Open
wants to merge 6 commits into
base: master
from

Conversation

@akgood
Copy link

akgood commented Jun 1, 2016

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

This comment has been minimized.

Copy link

codecov-io commented Jun 1, 2016

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link
Contributor

hynek commented Aug 22, 2016

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.

chain_stack = _ffi.gc(chain_stack, cleanup)

for cert in self._chain:
copy = _lib.X509_dup(cert._x509)

This comment has been minimized.

Copy link
@tiran

tiran Aug 22, 2016

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.

This comment has been minimized.

Copy link
@akgood

akgood Aug 22, 2016

Author

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...)

This comment has been minimized.

Copy link
@hynek

hynek Aug 22, 2016

Contributor

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.

@akgood akgood force-pushed the akgood:master branch from e14798e to 81205e1 Aug 22, 2016
@akgood akgood force-pushed the akgood:master branch from 81205e1 to 35784f3 Aug 22, 2016
@reaperhulk

This comment has been minimized.

Copy link
Member

reaperhulk commented Aug 23, 2016

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

@tiran

This comment has been minimized.

Copy link

tiran commented Aug 23, 2016

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

This comment has been minimized.

Copy link
Member

Lukasa commented Aug 23, 2016

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

This comment has been minimized.

Copy link

tiran commented Aug 23, 2016

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

@akgood

This comment has been minimized.

Copy link
Author

akgood commented Aug 23, 2016

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

This comment has been minimized.

Copy link
Contributor

hynek commented Aug 23, 2016

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 comment has been minimized.

Copy link
Member

reaperhulk commented Aug 24, 2016

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

This comment has been minimized.

Copy link
Author

akgood commented Aug 31, 2016

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

This comment has been minimized.

Copy link
Contributor

hynek commented Sep 1, 2016

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

This comment has been minimized.

Copy link

gsmethells commented Mar 17, 2017

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

This comment has been minimized.

Copy link
Contributor

hynek commented Mar 24, 2017

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.

leap-code-o-matic pushed a commit to leapcode/leap_pycommon that referenced this pull request Jul 14, 2017
With the merge of platformTrust in twisted, the situation for cert chain
verification in linux improved a lot.

This patch implements fallbacks to do the following:

- Try to use whatever trust sources are found in the system. This means
that if ca-certificates is installed, pyopenssl will have a valid set of
root certificates and verification will likely work (twisted uses
platformTrust for this).

- If that fails, try to use certifi. We could/should depend on that from
now on, *but* it's not packaged before stretch.

- So, I'm not deprecating its usage right now, but this one should be
the last cacert.pem bundle that we ship with leap.common.

- If the cacert.pem from leap.common fails to be found, well, there's
nothing you can do. Your TOFU attempt with a cert coming from the
CArtel will fail.

Most of this MR should be sent as a patch upstream, see https://twistedmatrix.com/trac/ticket/6934
Also related: https://twistedmatrix.com/trac/ticket/9209

I think proper testing will depend on merging pyca/pyopenssl#473

- Resolves: #8958
- Release: 0.6.0
@akgood akgood force-pushed the akgood:master branch from 98c7128 to 08fdb82 May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.