#2061 defect closed fixed (fixed)
_sslverify.py: Wrong use of param caCerts in OpenSSLCertificateOptions
Reported by: | dq | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | core | Keywords: | |
Cc: | Ralph Meijer, Thijs Triemstra, zooko@…, davidsarah, Hynek Schlawack | Branch: |
branches/ssl-chain-cert-2061
branch-diff, diff-cov, branch-cov, buildbot |
Author: | tomprince |
Description
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));
Attachments (4)
Change History (53)
comment:1 Changed 16 years ago by
comment:2 Changed 16 years ago by
Cc: | Ralph Meijer added |
---|
comment:3 Changed 16 years ago by
Priority: | normal → high |
---|
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.
comment:4 Changed 13 years ago by
Filed in pyOpenSSL as https://bugs.launchpad.net/pyopenssl/+bug/364185
comment:5 Changed 13 years ago by
Owner: | changed from Glyph to Jean-Paul Calderone |
---|
I am probably not the right person to take care of this.
comment:6 Changed 13 years ago by
Cc: | Thijs Triemstra added |
---|
comment:7 Changed 12 years ago by
The pyOpenSSL ticket is resolved. So hopefully we could do the right thing w/ pyOpenSSL 0.10 now.
comment:8 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:9 Changed 11 years ago by
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?
comment:10 Changed 11 years ago by
Cc: | zooko@… added |
---|
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:
[source: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.
comment:11 Changed 11 years ago by
Notice the ticket description talks about OpenSSLCertificateOptions
but zooko is talking about DefaultOpenSSLContextFactory
.
comment:12 Changed 10 years ago by
comment:13 Changed 10 years ago by
Cc: | davidsarah added |
---|
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)
?
comment:14 Changed 10 years ago by
Probably (with the usual warning against ever using hasattr
for anything).
comment:15 Changed 9 years ago by
Somewhat confident that #6258 was a duplicate of this (so I have marked it as such).
Changed 9 years ago by
Attachment: | use-chain-certs-if-possible-2061.patch added |
---|
Add support for chained certificates if available.
comment:16 Changed 9 years ago by
Keywords: | review added |
---|
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.
comment:17 Changed 9 years ago by
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.
comment:18 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|
comment:19 Changed 9 years ago by
Status: | new → assigned |
---|
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.
comment:20 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Hynek Schlawack |
Status: | assigned → new |
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:
- 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. - 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. test_chainLoaderNotAvailable
asserts thatssl.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 theDefaultOpenSSLContextFactory
(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).- 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:
- There is no verification that the fakes are properly constructed in the first place, save manual inspection.
- 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.
comment:21 Changed 9 years ago by
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 X509
s 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.
Changed 9 years ago by
Attachment: | chained-certs-2061.patch added |
---|
comment:22 Changed 9 years ago by
Cc: | Hynek Schlawack added |
---|---|
Keywords: | review added |
Owner: | Hynek Schlawack deleted |
I’ve added support for chains in OpenSSLCertificateOptions
as a new optional argument.
Let me know if it needs more love.
comment:23 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/ssl-chain-cert-2061 |
(In [37900]) Branching to ssl-chain-cert-2061.
comment:24 Changed 9 years ago by
comment:25 follow-up: 26 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Hynek Schlawack |
extra
inextraChianCert
seems extraneous (but I'm prepared to be convinced otherwise. Also,chainCertificate
orcertificateChain
seem nicer._contextFactory
needs documentation- OpenSSLOptions.test_extraChainFilesAreAddedIfSupplied is missing a docstring. (It'd also be nice if the methods of
FakeContext
had some documentation). - 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.
comment:26 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Hynek Schlawack deleted |
Replying to tom.prince:
extra
inextraChianCert
seems extraneous (but I'm prepared to be convinced otherwise. Also,chainCertificate
orcertificateChain
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.
_contextFactory
needs documentation
I hope plain comment is enough?
- 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…
- 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.
comment:27 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:30 follow-up: 31 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Hynek Schlawack |
Status: | assigned → new |
Thanks. Just a few minor points:
- Please file a ticket against pyOpenSSL to provide a test-friendly implementation of the
Context
API (either as an in-memory fake or by makingContext
itself more introspectable). Add a link to the ticket toFakeContext
. - For now, please verify the methods of
FakeContext
at least have matching signatures with the methods ofContext
(inspect.getargspec
or define an interface and verifyContext
andFakeContext
against it). - Add instance variable documentation to the
FakeContext
class docstring - The
test_constructorSetsExtraChain
docstring is hard to read. "Setting C{extraCertChain} sets both if ..." - sets both what? - 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 pyOpenSSLContext
object. 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.- in the implementation,
extraCertChain or []
is a pattern to avoid. Write out the if statement to avoid surprising behavior whenextraChainCert == []
. - 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!
comment:31 Changed 9 years ago by
Replying to exarkun:
Thanks!
- Please file a ticket against pyOpenSSL to provide a test-friendly implementation of the
Context
API (either as an in-memory fake or by makingContext
itself more introspectable). Add a link to the ticket toFakeContext
.
https://bugs.launchpad.net/pyopenssl/+bug/1173899
- For now, please verify the methods of
FakeContext
at least have matching signatures with the methods ofContext
(inspect.getargspec
or define an interface and verifyContext
andFakeContext
against it).
I’m afraid that’s impossible. A Tom confirmed, C functions aren’t introspectible. Gotta wait for point 1…
- Add instance variable documentation to the
FakeContext
class doctoring
Done, I hope it’s okay like this.
- The
test_constructorSetsExtraChain
docstring is hard to read. "Setting C{extraCertChain} sets both if ..." - sets both what?
Yeah, me fail English.
- 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 pyOpenSSLContext
object.
Indeed.
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.
- in the implementation,
extraCertChain or []
is a pattern to avoid. Write out the if statement to avoid surprising behavior whenextraChainCert == []
.
Done.
- 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.
Changed 9 years ago by
Attachment: | chained-certs-2061-feedback-2.patch added |
---|
comment:32 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Hynek Schlawack to Jean-Paul Calderone |
comment:35 Changed 9 years ago by
comment:36 Changed 9 years ago by
Keywords: | review removed |
---|
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.
comment:37 follow-up: 38 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:38 Changed 9 years ago by
comment:39 follow-up: 40 Changed 9 years ago by
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!
comment:40 follow-up: 46 Changed 9 years ago by
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.
comment:41 follow-up: 42 Changed 9 years ago by
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.
comment:42 Changed 9 years ago by
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?
comment:43 follow-up: 44 Changed 9 years ago by
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?
comment:44 Changed 9 years ago by
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 :).
comment:45 Changed 9 years ago by
So, in keeping with previous topics, I've created a wiki page.
Security? I guess that was meant to be Plan/Security.
comment:46 Changed 9 years ago by
Replying to glyph:
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.
This is the standard Someone Else's Problem trap. OpenSSL devs aren't going to fix any of this. They believe it is Someone Else's Problem to choose settings and configurations for that person's specific use case. Twisted devs think it is Someone Else's Problem. As a web developer who sets up a site using Twisted, I'd like it to be Someone Else's Problem to choose secure configuration settings, so that when I set up a web site, it comes with working security out of the box. I don't think there's anyone who can't pass this buck to Someone Else. I'd be happy to help the Twisted project with it, if they decide to fix it, or to help the pyOpenSSL project, or OpenSSL, or whoever. My bet is that the OpenSSL folks can ignore their users having insecure settings longer than you can.
comment:47 Changed 9 years ago by
Sorry, I guess this topic in comment:46 belongs in a separate ticket.
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.