_sslverify.py: Wrong use of param caCerts in OpenSSLCertificateOptions

[dq]

Wrong use of param caCerts in the constructor of class OpenSSLCertificateOptions in file _sslverify.py.

Two complaints, mainly referring to the documentation of caCers.

a) The parameter caCertsFile does not exist.

b) These certificates are NOT SENT to the client, but used server-side to check correctness when receiving a client certificate.

The documented functionality would actually be provided when calling the pyOpenSSL exposed method ctx.load_client_ca(pemfile), which implements the code fragment from the following quote:

Quoting from the openssl documentation (openssl-0.9.8c/CHANGES.SSLeay)

If you want to use client certificates then you have to add in a bit of extra stuff in that a SSLv3 server sends a list of those CAs that it will accept certificates from ... so you have to provide a list to SSLeay otherwise certain browsers will not send client certs.

SSL_CTX_set_client_CA_list(ctx,SSL_load_client_CA_file(s_cert_file));

[exarkun]

The use of filenames in API is quite stupid. Unfortunately, PyOpenSSL does not expose SSL_CTX_set_client_CA_list independently of SSL_load_client_CA_file, so Twisted can't do much about this right now. I suppose we could write certs out to a temporary file, but this seems somewhat sketchy to me.

The best fix would be to expose SSL_CTX_set_client_CA_list in PyOpenSSL.

[glyph]

I do think that as a temporary measure we should seriously consider writing the certs to a temporary file. It's unfortunate that it is a required step, but it is likely that the necessary data is residing in a file on disk already, and so we can probably skip it in most cases.

[glyph]

Filed in pyOpenSSL as  https://bugs.launchpad.net/pyopenssl/+bug/364185

[glyph]

I am probably not the right person to take care of this.

[exarkun]

The pyOpenSSL ticket is resolved. So hopefully we could do the right thing w/ pyOpenSSL 0.10 now.

[thijs]

Replying to dq:

Two complaints, mainly referring to the documentation of caCers. - The parameter caCertsFile does not exist. - These certificates are NOT SENT to the client, but used server-side to check correctness when receiving a client certificate.

Both of these doc issues seem to be corrected in r30155?

[zooko]

Adam Langley told me this about my new SSL-enabled web site:

I note that you're still missing your intermediates on leastauthority.com

Here are the two intermediates you need:

...

Normally you should be able to append them to the certificate file and everything would work. However, bug to a bug in TwistedWeb I don't think that's going to work.

Here's the source that I think you're using:

tags/releases/twisted-10.0.0/twisted/internet/ssl.py#L98

Note that it's calling use_certificate_file, which reads a *single* certificate from the file. It should be use_certificate_chain_file. If you make that change, and append the intermediates to your certificate file, everything should just work.

Here's the OpenSSL docs for those functions:  http://www.openssl.org/docs/ssl/SSL_CTX_use_certificate.html

You can check with:

    openssl s_client -tls1 -connect leastauthority.com:443 -showcerts

You should see three certificates returned by the server.

I used this web site, suggested by agl, to get more information about my server:

 https://www.ssllabs.com/ssldb/analyze.html?d=leastauthority.com

It reported "Chain issues: Incomplete". (By the way it also reported "vulneable to BEAST attack", which sounds like something out of Magic: the Gathering...)

I tried agl's suggested openssl command-line, and it showed the server returning one certificate. Then I applied agl's suggested patch:

zooko@ip-10-122-222-129:~$ diff -u ssl.py-orig ssl.py
--- ssl.py-orig 2011-11-16 00:47:25.000000000 +0000
+++ ssl.py      2011-11-16 00:47:04.000000000 +0000
@@ -95,7 +95,7 @@
             # Disallow SSLv2!  It's insecure!  SSLv3 has been around since
             # 1996.  It's time to move on.
             ctx.set_options(SSL.OP_NO_SSLv2)
-            ctx.use_certificate_file(self.certificateFileName)
+            ctx.use_certificate_chain_file(self.certificateFileName)
             ctx.use_privatekey_file(self.privateKeyFileName)
             self._context = ctx
 

Now the openssl command-line shows the server returning three certificates, and the ssllabs.com site says "Chain issues: None".

By the way, I came to this ticket by searching for use_certificate_chain_file and thus finding #3012, which was closed as being a duplicate of this ticket.

[exarkun]

Notice the ticket description talks about OpenSSLCertificateOptions but zooko is talking about DefaultOpenSSLContextFactory.

[exarkun]

Since the superior API is available in pyOpenSSL 0.10, and #4974 deprecates support for pyOpenSSL <0.10, and #5014 removes that support, this ticket should be much easier to resolve once #5014 is resolved. That involves waiting about one more year, though.

[davidsarah]

Can we not do something like:

    if hasattr(ctx, 'use_certificate_chain_file'):
        ctx.use_certificate_chain_file(self.certificateFileName)
    else:
        log.msg("certificate chain will not be complete -- fix by using pyOpenSSL >= 0.10")
        ctx.use_certificate_file(self.certificateFileName)

?

[exarkun]

Probably (with the usual warning against ever using hasattr for anything).

[exarkun]

Somewhat confident that #6258 was a duplicate of this (so I have marked it as such).

[hynek]

Add support for chained certificates if available.

[hynek]

With the disclaimer, that I’m not sure if I did Fakes and warnings right, I’d like to propose this patch that handles both outdated PyOpenSSL as well as non-PEM certificates use_certificate_file should support.

TIA for any useful feedback.

[hynek]

FTR, I’m not sure how serious the “never be used for anything” was meant since hasattr appears 420+ times in Twisted’s codebase. I could solve it using a simple

try:
    self._contextFactory.use_certificate_chain_file
except AttributeError:
…
else:
…

too if you’d prefer that.

[exarkun]

FTR, I’m not sure how serious the “never be used for anything” was meant since hasattr appears 420+ times in Twisted’s codebase.

I think you mis-grepped. It only appears 209 times in current trunk@HEAD, and only 158 of those are outside of test modules. :) This is still far too many uses, and it would be fewer in a perfect world.

There are a number of ways to avoid using hasattr; the example you gave suites just fine, but if you'd rather stick to an expression, getattr(target, name, default) is pretty good.

[exarkun]

Thanks for this patch. However, it appears as though the wrong API is being changed. The patch changes DefaultOpenSSLContextFactory rather than OpenSSLCertificateOptions. Usage of the latter should be encouraged to supersede usage of the former; this will be more difficult if the former has more functionality than the latter. Adding a new parameter to OpenSSLCertificateOptions.__init__ which represented the certificates in the certificate chain and implementing this feature would address this concern. If you're actually interested in adding new functionality to the older context factory, you could add a new parameter/attribute to it as well. I consider the latter optional, though.

Beyond that, some comments about what the patch does do:

1. This introduces another warning to every program that uses DefaultOpenSSLContextFactory with pyOpenSSL < 0.10. #4974 already introduced a deprecation warning for this configuration, so a new warning is redundant. The trouble with the proposed API change is that there is no way to tell if an application is actually asking for a chain file to be loaded or not, so no way to know if the application is missing out on needed functionality by having an older version of pyOpenSSL in use. To me, this suggests that the API is not a great one.
2. The changes to twisted/test/test_ssl.py contain naming and coding standard violations. Please read the coding standard document and follow its guidelines in future patches.
3. test_chainLoaderNotAvailable asserts that ssl.DefaultOpenSSLContextFactory.cacheContext is warned about the old version of pyOpenSSL in use. This is a confusing warning. cacheContext is basically an implementation detail, and it's part of Twisted, not part of the deployment system or an application. Emitting a warning blaming this function for doing something wrong is at best useless noise and at worst actively misleading. Warnings should be pointed at the offending code. In this case, I suppose that would be whatever constructed the DefaultOpenSSLContextFactory (but the previous point about not being able to actually know whether there is anything to warn about or not has an influence on this, too).
4. As far as the fakes go, they're really no worse than what is already present. The main problem with this style of testing is that it is fragile:
   1. There is no verification that the fakes are properly constructed in the first place, save manual inspection.
   2. There is no automated check to verify that the fakes continue to bear a resemblance to the objects they are faking.

If you wanted to write some really good tests, then writing some verified fakes would be a good step in that direction. I don't think there is a good explanation of all the techniques for the "verified" part of that - or at least, I am not aware of one. If you'd like guidance, I think this would be a good topic to raise on IRC in the dev channel or on the mailing list.

Thanks again for your contribution.

[hynek]

Thank you very much for taking the time to review!

So, I like to add an argument called certificateChain for OpenSSLCertificateOptions which would take a list of X509s to be consistent with certificate and privateKey -- at least until #6274 is possibly resolved. It would use Context.add_extra_chain_cert() which is from PyOpenSSL 0.6 if I read the bazaar log correctly.

That said, I would very much appreciate any guidance on verified fakes and I tried to bring it up on IRC several times with little luck. A pointer to a test that uses current best practices would be great already.

[hynek]

I’ve added support for chains in OpenSSLCertificateOptions as a new optional argument.

Let me know if it needs more love.

[tomprince]

(In [37900]) Branching to ssl-chain-cert-2061.

[tomprince]

(In [37901]) Apply chained-certs-2061.patch from hynek.

Refs: #2061

[tom.prince]

1. extra in extraChianCert seems extraneous (but I'm prepared to be convinced otherwise. Also, chainCertificate or certificateChain seem nicer.
2. _contextFactory needs documentation
3. OpenSSLOptions.test_extraChainFilesAreAddedIfSupplied is missing a docstring. (It'd also be nice if the methods of FakeContext had some documentation).
4. It would be nice if there were some functional test. The code looks reasonable, but I don't know enough about about ssl and pyopenssl to verify it, and the current tests for this functionality don't actually exercise pyopenssl.

[hynek]

Incorporate feedback

[hynek]

Replying to tom.prince:

1. extra in extraChianCert seems extraneous (but I'm prepared to be convinced otherwise. Also, chainCertificate or certificateChain seem nicer.

I discussed this with JP at the PyCon sprint, the reasoning behind the name is the following: Various servers use various approaches in implementing chains. Apache uses a separate option with multiple files, nginx just concats all certificates into one file. Even pyOpenSSL itself isn’t consistent in this regard: there’s both use_certificate_chain_file which behaves like nginx as well as add_extra_chain_cert which adds a certificate to the chain. Using the word extra makes this explicit and clear about its intent.

2. _contextFactory needs documentation

I hope plain comment is enough?

3. OpenSSLOptions.test_extraChainFilesAreAddedIfSupplied is missing a docstring. (It'd also be nice if the methods of FakeContext had some documentation).

I’ve added a doc string and expanded the documentation of FakeContext a bit – I couldn’t come up with a useful way to document the individual methods…

4. It would be nice if there were some functional test. The code looks reasonable, but I don't know enough about about ssl and pyopenssl to verify it, and the current tests for this functionality don't actually exercise pyopenssl.

This led to an extensive discussion on IRC with JP and glyph with the result that this should be put into a separate ticket because it’s really hard to do in a meaningful way. I’ve written a test script though that demonstrates the usage:  https://gist.github.com/hynek/4095c70615a223ecc1df

Additionally I’ve added a test that ensures that the context factory still works if chain certificates are supplied.

[exarkun]

(In [38225]) Apply chained-certs-2061-feedback-1.patch

refs #2061

[exarkun]

Some recent  build results.

[exarkun]

Thanks. Just a few minor points:

1. Please file a ticket against pyOpenSSL to provide a test-friendly implementation of the Context API (either as an in-memory fake or by making Context itself more introspectable). Add a link to the ticket to FakeContext.
2. For now, please verify the methods of FakeContext at least have matching signatures with the methods of Context (inspect.getargspec or define an interface and verify Context and FakeContext against it).
3. Add instance variable documentation to the FakeContext class docstring
4. The test_constructorSetsExtraChain docstring is hard to read. "Setting C{extraCertChain} sets both if ..." - sets both what?
5. Instead of saying "respected" in the test_extraChainFilesAreAddedIfSupplied docstring, say what is done with the object. The certificates it contains are added as extra chain certificates on the pyOpenSSL Context object.
6. test_extraChainDoesNotBreakPyOpenSSL, meh. I guess this test is fine. It's not really great, though, but I don't have any suggestions for improving it right now.
7. in the implementation, extraCertChain or [] is a pattern to avoid. Write out the if statement to avoid surprising behavior when extraChainCert == [].
8. This ticket originally reported more than one problem. None of them is actually about certificate chains not being properly supported. Please file a ticket for supporting the "client CA list" feature mentioned in the ticket description.

Thanks again!

[hynek]

Replying to exarkun:

Thanks!

1. Please file a ticket against pyOpenSSL to provide a test-friendly implementation of the Context API (either as an in-memory fake or by making Context itself more introspectable). Add a link to the ticket to FakeContext.

 https://bugs.launchpad.net/pyopenssl/+bug/1173899

1. For now, please verify the methods of FakeContext at least have matching signatures with the methods of Context (inspect.getargspec or define an interface and verify Context and FakeContext against it).

I’m afraid that’s impossible. A Tom confirmed, C functions aren’t introspectible. Gotta wait for point 1…

1. Add instance variable documentation to the FakeContext class doctoring

Done, I hope it’s okay like this.

1. The test_constructorSetsExtraChain docstring is hard to read. "Setting C{extraCertChain} sets both if ..." - sets both what?

Yeah, me fail English.

1. Instead of saying "respected" in the test_extraChainFilesAreAddedIfSupplied docstring, say what is done with the object. The certificates it contains are added as extra chain certificates on the pyOpenSSL Context object.

Indeed.

1. test_extraChainDoesNotBreakPyOpenSSL, meh. I guess this test is fine. It's not really great, though, but I don't have any suggestions for improving it right now.

Well I’m not in love with it but at least it somewhat shows the API is used correctly until we have better means.

1. in the implementation, extraCertChain or [] is a pattern to avoid. Write out the if statement to avoid surprising behavior when extraChainCert == [].

Done.

1. This ticket originally reported more than one problem. None of them is actually about certificate chains not being properly supported. Please file a ticket for supporting the "client CA list" feature mentioned in the ticket description.

#6490

[exarkun]

(In [38297]) Apply chained-certs-2061-feedback-2.patch

refs #2061

[exarkun]

(In [38303]) Remove an unused import

refs #2061

[exarkun]

(In [38304]) Tiny tweaks to the test implementation

• Make FakeContext new-style
• Drop the unnecessary lambda when overriding _contextFactory

refs #2061

[exarkun]

Thanks. Sorry for suggesting you try doing something impossible. :(

I made a couple tiny tweaks, referenced above. Otherwise, looks good to me now. I'll go ahead and merge the branch. Thank you very much for your persistence on this ticket.

[exarkun]

(In [38305]) Merge ssl-chain-cert-2061

Author: hynek Reviewer: exarkun, tom.prince Fixes: #2061

Add support for certificate chains to twisted.internet.ssl.CertificateOptions.

[glyph]

Replying to exarkun:

(In [38305]) Merge ssl-chain-cert-2061 Author: hynek Reviewer: exarkun, tom.prince Fixes: #2061 Add support for certificate chains to twisted.internet.ssl.CertificateOptions.

Thanks, hynek! Thanks, exarkun! It's really nice to see this ticket finally closed :-D.

[zooko]

Thank you so much hynek, exarkun, and tom.prince for fixing this! I just upgraded my server at  https://LeastAuthority.com and accidentally overwrote a monkey-patch I had installed there to work-around this defect. I'm happy to see it fixed in Twisted now. (Now what? Maybe I should install Twisted's current head from revision control onto my server...)

 https://www.ssllabs.com/ssltest/analyze.html?d=leastauthority.com

Currently offers the following five complaints:

• BEAST attack Vulnerable INSECURE (more info)
• Compression Yes INSECURE (more info)
• RC4 Yes PROBLEMATIC (more info)
• TLS_RSA_WITH_DES_CBC_SHA (0x9) WEAK 56

•    Certification Paths
     Path #1: Trusted
     1 	Sent by server 	leastauthority.com
     SHA1: eaea043d67149f263b418af04bd53da3e3d677c8
     RSA 2048 bits / SHA1withRSA
     2 	Extra download 	RapidSSL CA
     SHA1: c039a3269ee4b8e82d00c53fa797b5a19e836f47
     RSA 2048 bits / SHA1withRSA
     3 	In trust store 	GeoTrust Global CA
     SHA1: de28f4a4ffe5b92fa3c503d1a349a7f9962a8212
     RSA 2048 bits / SHA1withRSA 
  

That last one of the five (the "extra download" part of that last one) will, I believe, be fixed by this branch. Thanks again!

[glyph]

Replying to zooko:

* BEAST attack Vulnerable INSECURE (more info) * Compression Yes INSECURE (more info) * RC4 Yes PROBLEMATIC (more info) * TLS_RSA_WITH_DES_CBC_SHA (0x9) WEAK 56

Erm... isn't turning all of this junk off (or enabling the long-implemented countermeasures, in the case of BEAST) the responsibility of OpenSSL itself, i.e. your platform vendor? I really hope we don't need to patch Twisted to fix these things.

[hynek]

Unfortunately that are configuration options. Some can be enforced by the SSL implementations but others can’t because you may need something insecure to cater to old clients (like old phones). There’s also some disagreement what ciphers are the lesser evil etc.

Ideally we’d offer a way to set all of the options I mention in  http://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/ for Twisted too.

[glyph]

Replying to hynek:

Unfortunately that are configuration options. Some can be enforced by the SSL implementations but others can’t because you may need something insecure to cater to old clients (like old phones). There’s also some disagreement what ciphers are the lesser evil etc. Ideally we’d offer a way to set all of the options I mention in  http://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/ for Twisted too.

Separate ticket for that?

[exarkun]

Separate ticket for that?

It probably bears some discussion first. Someone want to schedule a meeting to more fully articulate the problem and explore some possible shapes for a solution?

[glyph]

Replying to exarkun:

Separate ticket for that?

It probably bears some discussion first. Someone want to schedule a meeting to more fully articulate the problem and explore some possible shapes for a solution?

So, setting up live meetings so that we type a bunch of stuff into IRC which is summarily lost seems like not a great idea. We should start exploring by writing stuff down.

Still, I guess I sort of understand your reluctance to open a bunch if tiny fiddly tickets now, with no particular arrangement to them or overall plan. So, in keeping with previous topics, I've created a wiki page.

Perhaps in advance of our quarterly all-hands meeting we can take some useful notes there, and if we do manage to get together on IRC, we can discuss things that we're actually writing someplace persistent :).

[exarkun]

So, in keeping with previous topics, I've created a wiki page.

Security? I guess that was meant to be Plan/Security.