Opened 15 months ago

Closed 9 months ago

Last modified 6 months ago

#6663 enhancement closed fixed (fixed)

Allow CertificateOptions to set acceptable SSL ciphers

Reported by: hynek Owned by: hynek
Priority: normal Milestone:
Component: core Keywords:
Cc: hs@…, zooko, mithrandi@…, tobias.oberstein@… Branch: branches/acceptable-ciphers-6663-2
(diff, github, buildbot, log)
Author: hynek Launchpad Bug:

Description

This is pretty important: we need to expose PyOpenSSL’s set_cipher_list to CertificateOptions so users can configure their acceptable SSL ciphers. zooko already called for it in #2061.

What happens if not, can be witnessed with our web page: we allow MD5 hashes and DES ciphers which are both patently insecure.

This is not an OpenSSL issue – these are options that need to be configurable if you want to deploy OpenSSL-based services in a responsible way. E.g. it’s impossible to configure a web server that offers perfect forward secrecy.

I’m going to plug my own work and use http://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/ as an example: it doesn’t matter whether you agree or not with my recommendations, we need to give our users the necessary control over the ciphers if we expect them to be able to use Twisted w/o an HTTP proxy. And we also should set half-way secure defaults (like no DES & MD5) which people will have to break on purpose if they need to for some reason.

I propose to add an option called ciphers to CertificateOptions which take an OpenSSL cipher string that is pretty universal (the same you pass into Apache or nginx) and pass it into the contexts.

Change History (66)

comment:1 Changed 15 months ago by exarkun

This seems strongly connected to <https://twistedmatrix.com/trac/ticket/5514>, though perhaps not a duplicate.

I propose to add an option called ciphers to CertificateOptions which take an OpenSSL cipher string that is pretty universal (the same you pass into Apache or nginx) and pass it into the contexts.

CertificateOptions is the explicitly "this isn't necessarily OpenSSL" interface, so let's not introduce OpenSSL-syntax strings into that API.

I think a list of strings naming individual ciphers would be a step up. Since OpenSSL's cipher string syntax supports groups and negation, this probably isn't equally expressive though.

Stepping back, forcing each application to make this decision is counter to the idea of promoting security by default. How can we promote security by default here?

Since it seems OpenSSL isn't going to take responsibility for this, either pyOpenSSL or Twisted needs to in order for applications using SSL via Twisted to have this property. The idea of putting this logic into pyOpenSSL is tempting because pyOpenSSL is actually focused on SSL and cryptography. Twisted certainly wants to offer security-related functionality to users, but the Twisted project is *far* from focused on these issues.

But pyOpenSSL is almost exclusively a direct OpenSSL wrapper, so it's not clear how this would fit in. Still, at least as a direction to explore, I like that more than I like the idea of putting this in Twisted.

But since there seems to be no real movement anywhere else, maybe it's time for Twisted to take the lead on this. I'd welcome an API that allows an easy transition of this responsibility to the SSL library, but that fills the gap until some SSL library bothers to accept that responsibility.

How about a method on CertificateOptions or a (another! woo!) flag to its __init__ that just says "give me only high quality ciphers"? Does it need to be any more specific than that?

comment:2 follow-up: Changed 15 months ago by hynek

CertificateOptions is the explicitly "this isn't necessarily OpenSSL" interface, so let's not introduce OpenSSL-syntax strings into that API.

Let’s introduce an AcceptableCiphers class then that has a fromOpenSSLCipherString factory method.

Since it seems OpenSSL isn't going to take responsibility for this, either pyOpenSSL or Twisted needs to in order for applications using SSL via Twisted to have this property.

In my view OpenSSL and by extension pyOpenSSL are crypto tools that have sharp edges for people who know what they are doing. Twisted OTOH is an enduser toolkit that should actively help its users to write reasonably secure software.

How about a method on CertificateOptions or a (another! woo!) flag to its init that just says "give me only high quality ciphers"? Does it need to be any more specific than that?

Unfortunately it has to, since that would make a few presumptions that unfortunately aren’t true:

  1. That there is a consent on high quality ciphers: for example right now there are roughly two fractions who agree what is the lesser evil: RC4 or AES-CBC. We can’t make this call because it also depends on the user audience.
  2. Even if that were true, it assumes the crypto landscape doesn’t change. Setting a onlySecureCiphers=True option on a 13.2 in 2014 it is probably a false sense of security. Things change basically monthly and administrators need to be able to react to that in a timely manner – not wait for the next Twisted release. Not to speak about backward compatibility problems if we change this.
  3. And finally every company has different requirements. A web shop that needs to support IE 6 has different requirements than a bank. I would prefer if Twisted would be useable by both.

We could make the cipher argument with the class I mentioned and make something like HIGH:!MD5:!NULL a default – that would probably fix #5514 too. But we need ways for both loosening and tightening the requirements.

comment:3 Changed 15 months ago by exarkun

And finally every company has different requirements. A web shop that needs to support IE 6 has different requirements than a bank. I would prefer if Twisted would be useable by both.

The existence of a "give me the secure ciphers, I don't care what they are" API doesn't preclude the use of other - already existing - APIs for cipher selection. If you know you need to support IE 6 or that you're only using SSL as a joke, you can use those other - already existing - APIs to set things up how you want.

This shouldn't prevent us from making an API for the average developer who isn't going to spend time researching the up-to-the-minute published security properties of each cipher supported by the SSL implementation. For those people, it should be easy to get something that's *probably* the best known configuration and at the very least is better than what they would cobble together if they had to do it themselves.

Even if that were true, it assumes the crypto landscape doesn’t change. Setting a onlySecureCiphers=True option on a 13.2 in 2014 it is probably a false sense of security. Things change basically monthly and administrators need to be able to react to that in a timely manner – not wait for the next Twisted release. Not to speak about backward compatibility problems if we change this.

This seems to be motivation to have the meaning of "securest ciphers" be configurable, perhaps on a site-wide basis. This isn't an area where Twisted has often ventured in the past and it there are difficult issues to work out (distribution tools for Python don't exactly make it easy to have site configuration), but it's probably not impossible.

As far as compatibility goes, I think we can have an API where we say the meaning may change over time. That's basically point of the API *I'm* talking about, at least. If there were a correct answer today that would be correct tomorrow, I could just tell people what it is and they could use it in their code.

That there is a consent on high quality ciphers: for example right now there are roughly two fractions who agree what is the lesser evil: RC4 or AES-CBC. We can’t make this call because it also depends on the user audience.

Flip a coin. :)

Okay, so I stand by what I wrote above (except the coin bit), but a *start* to resolving this issue would certainly be letting the user of CertificateOptions specify which ciphers to use by naming them (or naming groups, or negating names of things, whatever). So your idea of AcceptableCiphers.fromOpenSSLCipherString sounds like something to investigate. I'm still not really sure what an AcceptableCiphers is, though. Does it just hold on to the OpenSSL-format ciphers string? How does CertificateOptions make use of this? I don't see a way around tying the implementation to OpenSSL - which isn't necessarily a reason not to do this, but maybe we shouldn't bother pretending this is an abstraction if it can't work with anything but OpenSSL.

Also, thanks for pushing on this issue. It will be really cool, one day, to be able to say that Twisted SSL applications are actually as secure as SSL applications can be made.

comment:4 Changed 15 months ago by hynek

Okay, I think I slightly misunderstood. Of course, I’m arguing for the same thing: there should be an easy way to make a Twisted installation halfway secure.

So my general idea would be indeed an abstract class AcceptableCiphers which has a factory method like buildSecureButCompatible() (and possibly more). Internally we can have a concrete class OpenSSLAcceptableCiphers which is used on OpenSSL-based installations that has the aforementioned fromOpenSSLCipherString().

I don’t want us to write a new general way to specify ciphers for Twisted, that’s not gonna end well. Let’s give oblivious users an easy way to get a secure cipher list and advanced users will have to buckle up and use the appropriate way for their installation. Internally, the representation will be the native one respectively – for OpenSSLAcceptableCiphers just the cipher string.

*

Does that sounds like something I should work on?

comment:5 Changed 15 months ago by lvh

I think two things are very important

  • Have secure defaults in Twisted
  • Have an API for selecting acceptable ciphers

I'm not sure about a "secure ciphers only" *flag*:

  • "Secure" can change. Attacks only get better.
  • Making "secure" an active choice gives people a false sense of security. Make it the default instead.
  • I don't think another flag is necessary. If there's a generic API for selecting acceptable ciphers, why can't we provide a few pre-configured ones? Like ALL_CIPHERS, GOOD_CIPHERS, PFS_CIPHERS... This is already for people who *don't* want the secure default, and it doesn't sound too painful.
  • You mention that this flag wouldn't preclude using any other already existing API; but to be clear, are you referring to an API that already exists right now, or one that would be introduced in tandem? If so, how would they interact? I'm definitely -1 on having multiple ways to do things that can interact in ways to end up doing things you don't want. One obvious way to do it is the way to go, IMHO, and "secure" (at least our best guess) should be the default.

As for the API for the cipher selection thing: the OpenSSL spec is great. I don't know of any other spec that works on more than one specific project, it's very easy to parse and the identifiers appear to be the same as the ones used in the TLS RFCs.

Changing the default to being secure ciphersuites only (as per our current best-effort conception of what that means) will break backwards compatibility. Clients relying on the existence of DES+MD5 ciphersuites would be able to connect to older versions, not to newer ones. I suppose the CompatibilityPolicy asks us to at least submit a warning :) We could do so unconditionally at startup, if there's no way to detect an insecure cipher being used right now for a connection.

Thank you for working on this, Hynek!

comment:6 Changed 15 months ago by hynek

I fully agree that default should be secure, CertificateOptions(…, acceptableCiphers=None) should give you a reasonably secure and compatible cipher string. I also agree with the flag rationale.

However, how do GOOD_CIPHERS differ from PFS_CIPHERS? How do you add MD5 if you need it? ORing? We’re in for a lot of confusion and would have actively to research ciphers and their properties. I consider this out of scope.

Really, let’s just decide on a good enough cipher string (I would just yield to zooko at that), make it the default and if someone wants to tweak it for special purposes, they have to use the native way of doing so, currently an OpenSSL cipher string. Using a helper-class makes it reasonably future proof too.

As for backward compatibility: we should issue a warning indeed, however it clearly is a gross security issue so it should be fixed ASAP regardless.

comment:7 Changed 15 months ago by lvh

I figured GOOD_CIPHERS would be the default. HIGH:!MD5:!NULL or something? PFS_CIPHERS means only allow the ones that do PFS, obviously ;)

My only point there is that if you already have a mechanism for specifying acceptable ciphers, you don't need a flag, you just need a default acceptable cipher list. I definitely agree that behavior like combining ciphersuite lists using | or & or whatever is totally out of scope.

Further, I agree with everything you said.

comment:8 Changed 15 months ago by exarkun

As for the API for the cipher selection thing: the OpenSSL spec is great.

To what "spec" are you referring?

comment:9 Changed 15 months ago by lvh

Sorry for being unclear, I meant the OpenSSL chiphersuite specification mini-language, e.g. "HIGH:RC4:!MD5:!EXPORT:!NULL" (that's just one off the top of my head, I don't know if that's a good or sane one, but it should give you an idea which kind of specification I'm talking about :))

comment:10 Changed 15 months ago by hynek

That’s also the default I had in mind you’ll get if you pass acceptableCiphers=None. It’s very compatible so it should cause very little disturbance.

comment:11 Changed 14 months ago by zooko

We're currently working-around this issue for our website (https://LeastAuthority.com) and you can see our notes here: https://github.com/LeastAuthority/leastauthority.com/issues/92

comment:12 in reply to: ↑ 2 Changed 14 months ago by zooko

Replying to hynek:

  1. That there is a consent on high quality ciphers: for example right now there are roughly two fractions who agree what is the lesser evil: RC4 or AES-CBC.

No, it is now clear that RC4 is the greater evil. The browsers have deployed defenses against the "BEAST" attack on CBC (the defense is "1/n-1 record splitting"), and BEAST is an active attack which can only be used in some cases and which tends to leave evidence of the attempt. On the other hand, RC4 is apparently vulnerable to passive attacks, which are more serious.

(If I'm wrong and there actually *is* a faction who still prefers RC4 despite the recent results against it, I'd like to read about it!)

comment:13 Changed 14 months ago by zooko

Here is my favorite SSL configuration at the moment:

https://www.ssllabs.com/ssltest/analyze.html?d=https%3A%2F%2Fzooko.com

In case that has changed between the time I wrote this and the time you read it, these are the results with a variety of SSL clients simulated by ssllabs.com:

Chrome 28 / Win 7 	TLS 1.1 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
Firefox 10.0.12 ESR / Win 7 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
Firefox 17.0.7 ESR / Win 7 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
Firefox 21 / Fedora 19 	TLS 1.0 	TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)   FS 	128
Firefox 22 / Win 7 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
IE 7 / Vista 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
IE 8 / XP   No FS *		TLS 1.0 	SSL_RSA_WITH_3DES_EDE_CBC_SHA (0xa)   No FS 	168
IE 8-10 / Win 7 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
IE 11 / Win 8.1 	TLS 1.2 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS 	128
Opera 12.15 / Win 7 	TLS 1.0 	TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)   FS 	128
Opera 15 / Win 7 	TLS 1.1 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
Safari 5.1.9 / OS X 10.6.8 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
Safari 6 / iOS 6.0.1 	TLS 1.2 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS 	128
Safari 6.0.4 / OS X 10.8.4 	TLS 1.0 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS 	128
Safari 7 / OS X 10.9 	TLS 1.2 	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS 	128 

This is the openssl config (from my nginx config file):

  ssl_protocols TLSv1.1 TLSv1 TLSv1.2;
  ssl_ciphers ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:DHE-RSA-AES128-GCM-SHA256:DHE-ECDSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA256:DHE-ECDSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-ECDSA-AES128-SHA:EDH-DSS-DES-CBC3-SHA:DES-CBC3-SHA;

comment:14 follow-up: Changed 14 months ago by hynek

Ugh, I thought I can reply to the notification like for roundup, so here a repost for the record:

I’m not going to argue ciphers with you because you’re obviously right and I already wrote elsewhere that I’m going to full defer to your judgement here.

To explain where the above came from and eg. Qualys is still somewhat for RC4 as a fallback cipher: to the best of my knowledge[1], Apple’s desktop Safari browser still hasn’t activated record splitting in its latest version and is thus still vulnerable to BEAST (and doesn’t support TLS>1). But that’s probably a corner case enough to ignore in the defaults and will hopefully resolve itself in Mavericks.

[1]: Mostly from https://community.qualys.com/blogs/securitylabs/2013/03/19/rc4-in-tls-is-broken-now-what and I’m not aware of any changes.

FWIW, I’m afraid we’ll have to keep RC4 around as a fallback for really old browsers (IE6?) – at least in the default configuration. We’d break too many deploys otherwise.

comment:15 Changed 14 months ago by mithrandi

  • Cc mithrandi@… added

comment:16 in reply to: ↑ 14 ; follow-up: Changed 14 months ago by zooko

Replying to hynek:

Hi hynek! Thank you for pushing on this issue. I appreciate your effort to improve things for everyone.

First, on the topic of "AES-CBC vs. RC4":

To explain where the above came from and eg. Qualys is still somewhat for RC4 as a fallback cipher: to the best of my knowledge[1]


[1]: Mostly from https://community.qualys.com/blogs/securitylabs/2013/03/19/rc4-in-tls-is-broken-now-what and I’m not aware of any changes.

Hm. Okay, I stand corrected. I don't see why Ivan Ristić doesn't think, like I do, that the possibility of active BEAST attacks against those browsers that haven't deployed countermeasures is much less important than the possibility of cryptanalysis of RC4. But, you're right. He does think differently than I do about that.

See also http://www.isg.rhul.ac.uk/tls/ and http://blog.cryptographyengineering.com/2013/03/attack-of-week-rc4-is-kind-of-broken-in.html .

FWIW, I’m afraid we’ll have to keep RC4 around as a fallback for really old browsers (IE6?) – at least in the default configuration. We’d break too many deploys otherwise.

Now this is a different issue: security vs. backward-compatibility. It might make just as much sense to fall back to unencrypted http-no-s for IE6, because then the IE6 user will have a visible indicator that they have an insecure connection?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 14 months ago by mithrandi

Replying to zooko:

Replying to hynek:

FWIW, I’m afraid we’ll have to keep RC4 around as a fallback for really old browsers (IE6?) – at least in the default configuration. We’d break too many deploys otherwise.

Now this is a different issue: security vs. backward-compatibility. It might make just as much sense to fall back to unencrypted http-no-s for IE6, because then the IE6 user will have a visible indicator that they have an insecure connection?

There isn't any way to actually do this, is there? If IE6 fails to negotiate a TLS connection with the server, you'll just get an error message, there's no way to redirect the user to an http connection (and presumably it would be a security vulnerability if there was a way to make browsers do this).

comment:18 follow-up: Changed 14 months ago by hynek

Yeah that's not a viable option. I don't even think we have enough introspection to see the connection details as of now? I may be wrong here though.

Also NB, that we're talking about a general SSL solution here, not just browsers. The default we come up with here will affect all protocols that use SSL. if we want a special cipher string for HTTPS, that would be a different ticket after this is settled. The universal default should be a balance of security and compatibility though.

That said, if we set honor server ciphers a RC4 at the end of the cipher string won't do any damage except to those who wouldn't be able to connect at all otherwise.

(In that tune, I've finally given up on the qualys Beast dictatorship and updated my article giving two ciphers: one that gets one an A at qualys and one that gets a B, is IMHO more secure and is based on Zooko's with some explanations. Feedback by twitter/email/irc welcome)

comment:19 in reply to: ↑ 17 Changed 14 months ago by zooko

Replying to mithrandi:

Now this is a different issue: security vs. backward-compatibility. It might make just as much sense to fall back to unencrypted http-no-s for IE6, because then the IE6 user will have a visible indicator that they have an insecure connection?

There isn't any way to actually do this, is there? If IE6 fails to negotiate a TLS connection with the server, you'll just get an error message, there's no way to redirect the user to an http connection (and presumably it would be a security vulnerability if there was a way to make browsers do this).

Sorry to be confusing, what I meant was, you can configure your SSL to do a secure connection or nothing (e.g. my suggested config in comment:13). In that case, users whose web browser can't do a secure connection will presumably use the http-no-s version of the site.

By the way, I think that IE6 on Windows XP can do TLS_RSA_WITH_3DES_EDE_CBC_SHA and TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA, according to:

http://msdn.microsoft.com/en-us/library/windows/desktop/aa380512%28v=vs.85%29.aspx

So the specific example of IE6 is not, as far as I know, an example of a web browser that can't do a secure TLS connection.

comment:20 Changed 14 months ago by hynek

I’ve signed up for a test account at SauceLabs and you’re right: IE6 does support 3DES-SHA: http://glui.me/?i=y3j201tt98ghvpm/2013-08-17_at_20.05.png/

The full output of https://cc.dcsec.uni-hannover.de would be: http://glui.me/?i=66xfd9y2k8mj3oz/2013-08-17_at_20.04.png/

text only:

SpecCipher Suite NameKey SizeDescription
(00,04)RSA-RC4128-MD5128 BitKey exchange: RSA, encryption: RC4, MAC: MD5.
(00,05)RSA-RC4128-SHA128 BitKey exchange: RSA, encryption: RC4, MAC: SHA1.
(00,0a)RSA-3DES-EDE-SHA56 BitKey exchange: RSA, encryption: 3DES, MAC: SHA1.
(01,0080)RC4128-MD5128 BitKey exchange: RC4, encryption algorithm is unknown, MAC: MD5.
(07,00c0)DES192-EDE3-MD5168 BitKey exchange: Data Encryption Standard (DES), encryption algorithm is unknown, MAC: MD5.
(03,0080)RC2128-MD5128 BitKey exchange: RC2, encryption algorithm is unknown, MAC: MD5.
(00,09)RSA-DES-SHA56 BitKey exchange: RSA, encryption: DES, MAC: SHA1.
(06,0040)DES64-MD556 BitKey exchange: Data Encryption Standard (DES), encryption algorithm is unknown, MAC: MD5.
(00,64)RSA-EXPORT1024-RC456-SHA56 BitKey exchange: RSA, encryption: RC4, MAC: SHA1.
(00,62)RSA-EXPORT1024-DES-SHA56 BitKey exchange: RSA, encryption: DES, MAC: SHA1.
(00,03)RSA-EXPORT-RC440-MD540 BitKey exchange: RSA, encryption: RC4, MAC: MD5.
(00,06)RSA-EXPORT-RC2-CBC40-MD540 BitKey exchange: RSA, encryption: RC2, MAC: MD5.
(02,0080)RC4128-EXPORT40-MD540 BitKey exchange: RC4, encryption algorithm is unknown, MAC: MD5.
(04,0080)RC2128-EXPORT40-MD540 BitKey exchange: RC2, encryption algorithm is unknown, MAC: MD5.
(00,13)DHE-DSS-3DES-EDE-SHA168 BitKey exchange: DH, encryption: 3DES, MAC: SHA1.
(00,12)DHE-DSS-DES-SHA56 BitKey exchange: DH, encryption: DES, MAC: SHA1.
(00,63)DHE-DSS-EXPORT1024-DES-SHA56 BitKey exchange: DH, encryption: DES, MAC: SHA1.
(00,ff)EMPTY-RENEGOTIATION-INFO-SCSV0 BitUsed for secure renegotation.

As for fallbacks: I don’t think that works that way automatically (at least I wasn’t able to reproduce that using IE 6) and it doesn’t really help us with SMTP or generic STARTTLS. I don’t want to be the pedantic asshole here but it seems to me that Twisted has a pretty strict backward-compatibility policy so my suggestion would be to make the default very compatible (no simple DES, but RC4-SHA at the very end) with honor server ciphers so that the weak ones ideally won’t get used but we still don’t break people’s installations. We may consider having a different default cipher string for twisted.web?

Please tell me if I’m missing something, I’m really not comfortable “discussing” this. :|

comment:21 in reply to: ↑ 18 ; follow-up: Changed 14 months ago by zooko

Replying to hynek:

That said, if we set honor server ciphers a RC4 at the end of the cipher string won't do any damage except to those who wouldn't be able to connect at all otherwise.

That depends on the details of SSL/TLS's defenses against rollback attack. I'm not familiar with the details of that, but I think Daira is.

comment:22 in reply to: ↑ 21 Changed 14 months ago by zooko

Replying to zooko:

That depends on the details of SSL/TLS's defenses against rollback attack. I'm not familiar with the details of that, but I think Daira is.

And by "rollback" I actually meant "downgrade":

http://twistedmatrix.com/trac/ticket/5514#comment:1

comment:23 Changed 13 months ago by hynek

  • Author set to hynek
  • Branch set to branches/acceptable-ciphers-6663

(In [40133]) Branching to acceptable-ciphers-6663.

comment:24 Changed 13 months ago by hynek

  • Keywords review added

This is my first shot at this problem to get it moving.

Build results: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/acceptable-ciphers-6663

  • winxp32-py2.7 is out of disk space.
  • Twistedchecker complains about a few things about the class docstring that seem to be its confusion though.
  • I’ve also fixed some twisted checker warnings nearby. I’ve grouped the imports in ssl.py because they were longer than 79 chars and it looked silly when they took 3 lines each. I can revert that if you insist.

Otherwise I believe everything should be mostly clear and straightforward. If zooko wants a different default cipher string, let me know.

Sorry it took so long.

comment:25 Changed 13 months ago by exarkun

winxp32-py2.7 is out of disk space.

It isn't. The cause of this intermittent error is a pipe attached to stdout having a full buffer.

comment:26 Changed 13 months ago by exarkun

Can you help me understand the benefit of the OpenSSLAcceptableCiphers abstraction? What does this class offer that passing a string does not? (A string is a terrible data structure, we don't need to go into the reasons for that. What I'm asking about is how OpenSSLAcceptableCiphers is any different from just using a string.)

comment:27 Changed 13 months ago by hynek

Unless I misunderstood, you opposed in the first sentence of your first comment to make a OpenSSL-specific cipher format a part of the OpenSSL-agnostic API. :)

I don’t love it either and would be more than happy to use strings or just say “SSL-implementation dependent structure” that would be a string for OpenSSL (and honestly for any other implementation either, just a with different syntax).

comment:28 Changed 13 months ago by exarkun

Unless I misunderstood, you opposed in the first sentence of your first comment to make a OpenSSL-specific cipher format a part of the OpenSSL-agnostic API. :)

The place where I'm stuck in understanding is that the proposed branch appears to do just this.

The API being introduced here is "an object with an attribute named DEFAULT_CIPHER_LIST which is an OpenSSL syntax cipher string". Or, put another way, here's some code I could write with the new API:

from twisted.internet.ssl import AcceptableCiphers, CertificateOptions

class MyCiphers(AcceptableCiphers):
    DEFAULT_CIPHER_LIST = "MD5:DEFAULT"

options = CertificateOptions(..., acceptableCiphers=MyCiphers())

This seems equivalent to just making CertificateOptions accept the string directly - just slightly less direct.

comment:29 Changed 13 months ago by exarkun

Eh, the code I posted isn't quite right I suppose since OpenSSLAcceptableCiphers._cipherList is initialized as a class attribute and not as part of __init__. I'm not sure yet if I think that invalidates the overall point or just makes this API more confusing.

comment:30 Changed 13 months ago by hynek

If the API confuses you, it’s definitely too confusing. :)

Before I go into it, I’d like to re-iterate that making the argument a string is totally okay with me because every SSL framework will always have a way to configure the cipher suite using strings.

The current API was an attempt to step away from strings and give it an framework independent type that can be referred to in documentation.

You’re not supposed to subclass but to use the factory AcceptableCiphers.fromOpenSSLCipherString("LOW"). Or ideally leave the fingers off it and accept that our default is fine. :)

DEFAULT_CIPHER_LIST is intended for read-only introspection (that’s why it’s in all-caps to convey it’s a constant) – that could be enforced with a property. Or we could just leaving it out. _cipherList is private because its format could vary on other implementations but that is also sort true for DEFAULT_CIPHER_LIST so I would probably drop it.

The question is rather whether we don’t just accept the universe and make the argument a string. CertificateOptions should be used by all SSL functions thus it would be fine if the defaults are in there.

comment:31 follow-up: Changed 12 months ago by lvh

For what it's worth, Mozilla's official recommendation (obviously web-centric) gets rid of 3DES, but keeps RC4. In the motivation they do explain why RC4 is way back.

https://wiki.mozilla.org/Security/Server_Side_TLS

comment:32 in reply to: ↑ 31 Changed 12 months ago by hynek

Replying to lvh:

For what it's worth, Mozilla's official recommendation (obviously web-centric) gets rid of 3DES, but keeps RC4. In the motivation they do explain why RC4 is way back.

https://wiki.mozilla.org/Security/Server_Side_TLS

They don’t explain why RC4 instead of 3DES at all though.

I may be in a bubble by following several cryptographers but RC4 appears to be running joke nowadays. Barring they add some rationale, I don’t find it adds value to the discussion. Am I missing something?

comment:33 Changed 12 months ago by zooko

I tried the config they suggested and asked https://www.ssllabs.com/ssltest/analyze.html?d=zooko.com what would be the result with a bunch of simulated clients.

With my config (ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:DHE-RSA-AES128-GCM-SHA256:DHE-ECDSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA256:DHE-ECDSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-ECDSA-AES128-SHA:DES-CBC3-SHA), ssllabs said:

Chrome 30 / Win 7       TLS 1.2         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS     128
Firefox 10.0.12 ESR / Win 7     TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
Firefox 17.0.7 ESR / Win 7      TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
Firefox 21 / Fedora 19  TLS 1.0         TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)   FS    128
Firefox 24 / Win 7      TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
IE 6 / XP   No FS *             Fail**
IE 7 / Vista    TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
IE 8 / XP   No FS *             TLS 1.0         TLS_RSA_WITH_3DES_EDE_CBC_SHA (0xa)   No FS     168
IE 8-10 / Win 7         TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
IE 11 / Win 8.1         TLS 1.2         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS     128
Java 6u45       TLS 1.0         TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)   FS    128
Java 7u25       TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
OpenSSL 0.9.8y  TLS 1.0         TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)   FS    128
OpenSSL 1.0.1e  TLS 1.2         TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)   FS     128
Opera 12.15 / Win 7     TLS 1.0         TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)   FS    128
Opera 16 / Win 7        TLS 1.1         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
Safari 5.1.9 / OS X 10.6.8      TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
Safari 6 / iOS 6.0.1    TLS 1.2         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS     128
Safari 6.0.4 / OS X 10.8.4      TLS 1.0         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   FS        128
Safari 7 / OS X 10.9    TLS 1.2         TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)   FS     128 

With Mozilla's config, ssllabs said the same thing, except with this diff:

-IE 8 / XP   No FS *            TLS 1.0         TLS_RSA_WITH_3DES_EDE_CBC_SHA (0xa)   No FS     168
+IE 8 / XP   No FS *            TLS 1.0         TLS_RSA_WITH_RC4_128_SHA (0x5)   No FS  128

So the only practical difference known to ssllabs's simulator is that with Mozilla's config, IE8/XP would choose RC4 instead of 3DES. This seems like a really bad idea to me. As far as I know 3DES is very strong, and is, along with AES, one of the two best-studied ciphers in all of human history. In contrast, RC4 is weak, and recent discoveries have shown it to be even weaker (as it is used in TLS) than earlier believed.

Seems like a pretty bad choice for Mozilla to recommend it over 3DES…

comment:34 Changed 12 months ago by zooko

comment:35 Changed 12 months ago by lvh

Yes; considering client-side mitigations this seems strange at best, and it seems our current working draft is better :)

comment:36 Changed 12 months ago by lvh

As for the AcceptableCiphers object, the intent is to have an object that encapsulates a bunch of ciphersuites, independent from the OpenSSL string specification. Since we only support OpenSSL right now, that means the only implementation uses that string specification.

Perhaps we should define an interface for it. I propose we start with:

  • fromOpenSSLString classmethod str -> AcceptableCiphers
  • toOpenSSLString instancemethod of AcceptableCiphers -> str

For the current AcceptableCiphers object, that'd be an awfully simple class, but if we specify more one day we can add more interfaces, and use INewTLSImplementationAcceptableCiphers(oldAcceptableCiphersObject).asNewTLSImplementationString().

Are there any other review points left on this thing?

comment:37 Changed 12 months ago by oberstet

  • Cc tobias.oberstein@… added

comment:38 Changed 12 months ago by exarkun

  • Keywords review removed
  • Owner set to hynek

It seems two questions are being wrestled with in the conversation above. One of these is about what ciphers to give applications which don't ask for anything specifically. The other is about how to represent a choice about which ciphers to use. It sounds like there's already been lots of good input regarding the first question. I'll leave the decision about that to the people who clearly know quite a bit about the topic. This message is about the question of how to represent these choices.

What are the goals of the representation? First, to be sufficient to get the TLS library to do what we want. Second, to avoid difficult to detect accidental misspecification of the choice. Third, to be easily introspectable for ad hoc examination and (more importantly) automated unit testing. Fourth, to allow configuration to be portable between TLS implementations (this might come earlier on the list if there were any other usable TLS implementations).

A string like "ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:DH+AES:ECDH+3DES:DH+3DES:RSA+AES:RSA+3DES:!ADH:!AECDH:!MD5:!DSS" achieves (more or less) the first of these goals. It completely fails on the others. Compare it to "ECDH+AESGCMDH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:DH+AES:ECDH+3DES:DH+3DES:RSA+AES:RSA+3DES:!ADH:!AECDH:!MD5:!DSS". OpenSSL happily accepts either of these. They mean distinctly different things and to the human eye they appear quite similar. Detecting the difference between these in a unit test is a challenge as well (without resorting to the failure mode of copying the string into the test and hoping it was correct wherever it was copied from).

Adding an API layer on top of one of these strings can help - but it doesn't 'necessarily' help. AcceptableCiphers.fromOpenSSLCipherString(...) may look nice but if underneath it's just a _cipherList string that OpenSSLCertificateOptions reaches in and yanks out then it's really just the same as using a string directly. All we've done is obscure its value (to the extent that even the implementation needs to violate the abstraction to get the job done).

I discussed an alternate idea with hynek on IRC and he seemed agreeable. I'll describe it here now. There are two parts. One is simpler because it doesn't try to break things down beyond the level of individual ciphers. I'll describe that one first.

There is a class very much like AcceptableCiphers (perhaps there is an interface, IAcceptableCiphers, and the class implements that). As usual, the interface does not specify how the object is constructed. For this simpler interface, say that AcceptableCiphers.fromOpenSSLCipherString is still around and promises the same semantics as the implementation in the current version of the branch. What the interface does specify is a method for specifying the exact ciphers that are to be supported. This is done by return value: a list of unicode strings giving cipher names. The order of the list is significant - earlier items are preferred over later ones. Any cipher not in the list will not be used. The method accepts an argument as well. This is also a list of unicode strings giving cipher names. These are the ciphers supported by the TLS implementation. Any string not in this list will not be used even if it is included in the returned list.

Put more succinctly:

class IAcceptableCiphers(Interface):
    def selectCiphers(availableCiphers):
        """
        Choose which ciphers to allow to be negotiated on a TLS connection.

        @param availableCiphers: A L{list} of L{unicode} which gives the names of
            all ciphers supported by the TLS implementation in use.

        @return: A L{list} of L{unicode} which gives the names of the ciphers which
            may be negotiated on the TLS connection.  The result is ordered by
            preference with more preferred ciphers appearing earlier.
        """

The more complicated part expands on this idea by replacing cipher names with cipher objects. The cipher objects have attributes describing their cryptographic properties. For example, the hash function used for the MAC or the number of bits in the key.

CertificateOptions will accept an object providing this interface and use its selectCiphers method to learn how it should configure the Context it creates. Since CertificateOptions is really OpenSSLCertificateOptions it knows which OpenSSL APIs to call to get the list of supported ciphers. It knows how to turn this information into the availableCiphers list required by the interface and it knows how turn the returned list back into something OpenSSL can consume.

An alternate implementation of CertificateOptions for another TLS implementation should be able to interface similarly with it.

Thus, this interface still satisfies the first requirement above.

The second requirement is not particularly affected as long as AcceptableCiphers.fromOpenSSLCipherString is the only constructor for the only implementation of this interface. However, it is now at least possible to have other constructors and other implementations. fromCipherNameList is clearly possible. Custom implementations of the interface implementing more interesting rules can also be imagined. MozillaCiphers() is a nicer thing to drop into an application than AcceptableCiphers.fromOpenSSLCipherString("ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:ECDHE-RSA-RC4-SHA:ECDHE-ECDSA-RC4-SHA:RC4-SHA:HIGH:!aNULL:!eNULL:!EXPORT:!DES:!3DES:!MD5:!PSK") and is more easily maintained as a third-party library as well (better to upgrade python-mozilla-ciphers-definitions than to track down all the configuration files you've dropped this string into).

The third requirement is addressed to some extent as well. The return value of selectCiphers is slightly higher level than the OpenSSL cipher string (for at least having had the ":" interpreted and any special rules like "!EXPORT" or "HIGH" expanded). If we consider the second part where actual cipher objects are used then we come even closer to satisfying the third requirement.

Finally, since since the logic for setting up the TLS library is outside of AcceptableCiphers and the return value of selectCiphers doesn't include OpenSSL-specific blobs, there's at least a reasonable chance this interface is usable with other TLS implementations.

As far as implementation details go, here's how you get a cipher name list out of pyOpenSSL:

from OpenSSL.SSL import SSLv23_METHOD, Context, Connection
ctx = Context(SSLv23_METHOD)
ctx.set_cipher_list(b"ALL")
conn = Connection(ctx, None)
print conn.get_cipher_list()

The Context method may make a difference to the results. I'm not sure.

Given that, I hope the rest of the necessary glue is obvious.

comment:39 follow-up: Changed 12 months ago by oberstet

The "recommended ciphers" subject seems to be moving fast lately. I've been reading up a little and wanted to share. Rough collection.

It seems the use of ephemeral keys is good practice .. which boils down to ECDHE or DHE where E signifies ephemeral, and my theory is that this is controlled with OpenSSL not via cipher list, but via options OP_SINGLE_ECDH_USE and OP_SINGLE_DH_USE. I will check that tomorrow.

It seems that mitigating side-channel attacks is notoriously difficult to get right, e.g. for CBC mode, but also for authenticated encryption modes like GCM:

https://bugzilla.mozilla.org/show_bug.cgi?id=868948

Nevertheless, GCM-AES is shipping now:

https://code.google.com/p/chromium/issues/detail?id=255241

and the change of chosen cipher when Chrome/Twisted(patched) talk can be seen:

https://twitter.com/oberstet/status/393787076262387713/photo/1

However, this Google engineer is going even further:

https://www.imperialviolet.org/2013/10/07/chacha20.html
http://tools.ietf.org/html/draft-agl-tls-chacha20poly1305-02

He seems to conclude that the way forward is TLS1.2-only with GCM-AES (DHE or ECDHE, dunno) or ChaCha20-Poly1305 (DHE or ECDHE, dunno).

And ChaCha20-Poly1305 is actually already coming to Chrome (NSS), FF (NSS) (and also OpenSSL):

https://code.google.com/p/chromium/issues/detail?id=310768
https://bugzilla.mozilla.org/show_bug.cgi?id=917571

However, the single most exciting is this:

http://nacl.cr.yp.to/

It's Daniel Bernstein et al creation. And that author's name and the fact that its already picked by Google/Mozilla .. speaks for itself;)

comment:40 follow-up: Changed 12 months ago by oberstet

Regarding API: if I get this right, a Twisted user would provide an object that implements IAcceptableCiphers. The selectCiphers method on this user provided object is then called by Twisted, with a list of available ciphers implemented by the underlying TLS implementation.

However, a user might to change the priority of ciphers returned not only based on the availability of ciphers and his (personal) preference, but upon *how* a particular cipher is implemented.

For example, when I decide that I only want to use/prioritize GCM-AES if it is implemented in hardware in the underyling TLS (Intel AES-NI / PCLMULQDQ instruction is there), and otherwise use/prioritize ChaCha20-Poly1305, how would I do that?

Would Twisted call IAcceptableCiphers.selectCiphers then either with ECDHE-GCM-AES-HARDWARE or ECDHE-GCM-AES-SOFTWARE?

Note: this is as far as I understand the behavior that Adam Langley would ideally expect from a client connecting or a server providing.

comment:41 follow-up: Changed 12 months ago by oberstet

Another API thing: it seems that TLS cipher negotiation is inherently asymmetric: a client needs to announce a prioritized list of ciphers it is willing to accept, but a server might in principle then dynamically choose a fitting one, based on client list and/or other criteria (does the client connect from LAN or WAN, ..).

As far as I understand, the above API is symmetric: both client and server set a prioritized list of ciphers, and the chosen cipher will simply be the first that appears in both lists.

comment:42 in reply to: ↑ 39 Changed 12 months ago by hynek

Replying to oberstet:

It seems the use of ephemeral keys is good practice .. which boils down to ECDHE or DHE where E signifies ephemeral, and my theory is that this is controlled with OpenSSL not via cipher list, but via options OP_SINGLE_ECDH_USE and OP_SINGLE_DH_USE. I will check that tomorrow.

I’m afraid you’re mixing things up here: TLS uses Diffie-Hellman for DHE/EDH and ADH (which must not be used because it’s susceptible to MITM attacks). When the server starts it creates an ephemeral key using the parameter file. That key gets lost once the server finishes – thus forward secrecy. If you set OP_SINGLE_DH_USE it will create a new key for each handshake which is better for forward secrecy, but not necessary and also has some performance impact. As Laurens and I already discussed on IRC, we’re going to be conservative and set OP_SINGLE_DH_USE by default; but it does not mean it’s necessary for DHE.

For more details please refer to http://www.openssl.org/docs/ssl/SSL_CTX_set_tmp_dh_callback.html . Someone please correct me if I’ve got something wrong, but this is what the docs say.

comment:43 follow-up: Changed 12 months ago by oberstet

Yeah, I found the relevant bits in the meantime:

https://github.com/openssl/openssl/blob/master/ssl/s3_srvr.c#L1694
https://github.com/openssl/openssl/blob/master/ssl/s3_srvr.c#L1769

However, what I still don't get: since there are ciphers identifiers ECDH and ECDHE, how would I ever get ECDH if the server generates a new ephemeral key at startup? Or can that be also controlled somehow? And if I turned that off, it would then automatically only speak ECDH, not ECDHE?

comment:44 Changed 12 months ago by oberstet

Or, what is the difference between the following 2:

`
ECDHE-RSA-AES128-SHA256 TLSv1.2 Kx=ECDH Au=RSA Enc=AES(128) Mac=SHA256
ECDH-RSA-AES128-SHA256 TLSv1.2 Kx=ECDH/RSA Au=ECDH Enc=AES(128) Mac=SHA256
`

More about API: in the meantime, I came across http://safecurves.cr.yp.to/.

OpenSSL comes with a set of builtin curves. None of the builtin seem to be marked as "safe" on "safecurves".

On the other hand, OpenSSL allows to create curves at run-time from parameters. It's not hard, and we could expose in pyOpenSSL.

But I still don't grasp the full picture: who decides about the curve used with ECDH? Client, server, or how does that work?

Unfortunately, I cannot find out which curve is in use, e.g.

`
openssl.exe" s_client -host crossbardemo.tavendo.de -port 443
`

will tell me ECDHE-RSA-AES256-GCM-SHA384 is used, but with what curve?

comment:45 in reply to: ↑ 40 Changed 12 months ago by hynek

Replying to oberstet:

Regarding API: if I get this right, a Twisted user would provide an object that implements IAcceptableCiphers. The selectCiphers method on this user provided object is then called by Twisted, with a list of available ciphers implemented by the underlying TLS implementation.

However, a user might to change the priority of ciphers returned not only based on the availability of ciphers and his (personal) preference, but upon *how* a particular cipher is implemented.

For example, when I decide that I only want to use/prioritize GCM-AES if it is implemented in hardware in the underyling TLS (Intel AES-NI / PCLMULQDQ instruction is there), and otherwise use/prioritize ChaCha20-Poly1305, how would I do that?

Would Twisted call IAcceptableCiphers.selectCiphers then either with ECDHE-GCM-AES-HARDWARE or ECDHE-GCM-AES-SOFTWARE?

Note: this is as far as I understand the behavior that Adam Langley would ideally expect from a client connecting or a server providing.

As long as it’s about your preference of ciphers, you will do that in AcceptableCiphers. As far as I understood, the cipher list in selectCiphers is for

  1. having some leverage from within CertificateOptions to disable certain ciphers under certain circumstances (obvious example: don’t offer ECDH ciphers if the underlying PyOpenSSL doesn’t support it).
  2. future-proofing the API because eg. tlslite has no APIs to get their lists themselves.
  3. bring in the mode (your expanded cipher string may contain SSLv3 ciphers but if your context factory is in TLSv1+, they have to be dropped if we want a realistic factory introspection).

If you want to be smart about your cipher selection, AcceptableCiphers is IMHO the way to go.

comment:46 in reply to: ↑ 43 Changed 12 months ago by hynek

Replying to oberstet:

Yeah, I found the relevant bits in the meantime:

https://github.com/openssl/openssl/blob/master/ssl/s3_srvr.c#L1694
https://github.com/openssl/openssl/blob/master/ssl/s3_srvr.c#L1769

However, what I still don't get: since there are ciphers identifiers ECDH and ECDHE, how would I ever get ECDH if the server generates a new ephemeral key at startup? Or can that be also controlled somehow? And if I turned that off, it would then automatically only speak ECDH, not ECDHE?

As far as I understand the scattered docs, ECDH/ECDHE is different from DHE and I ignored it because I consider it out of scope for this ticket. If http://openssl.6102.n7.nabble.com/ECDH-vs-ECDHE-td19768.html is correct, the difference is indeed in the single use part.

The selection of curves is also a completely new can of worms; let’s focus on ciphers here please. Something like selectCurve or even rather AcceptableCurves can be added later on but is orthogonal to this ticket.

comment:47 in reply to: ↑ 41 Changed 12 months ago by hynek

Replying to oberstet:

Another API thing: it seems that TLS cipher negotiation is inherently asymmetric: a client needs to announce a prioritized list of ciphers it is willing to accept, but a server might in principle then dynamically choose a fitting one, based on client list and/or other criteria (does the client connect from LAN or WAN, ..).

Could you point me to a precedent of software doing such things please? I find the idea intriguing, but I doubt the current APIs allow for setting the cipher lists mid-handshake; at least a cursory search of the pyOpenSSL docs didn’t reveal anything. I agree that it would be an interesting feature though and would solve it by adding optional key-value parameters to selectCiphers once we have APIs to support that.

P.S. I just saw that we currently set OP_SINGLE_DH_USE by default already, so that point from previous messages is moot anyway. :)

comment:48 Changed 12 months ago by oberstet

I have found nothing in OpenSSL (left alone pyOpenSSL) that would allow dynamic cipher selection on server-side, and I am also not aware of any software doing that. I just wanted to bring it up since it's about API design.

I do think an API for such a thing would need to be more like:

class IClientCipherSelect(Interface):
    def selectCiphers(server, availableCiphers)
       """
       Filters ciphers offered by client - return a prioritized list of ciphers.
       `server` has additional info like server IP etc. Client may dynamically
       adjust offered ciphers based on `server` (static) info. This is called by
       the framework before TLS negotiation even starts, but after server hostname
       has been resolved etc.
       """

class IServerCipherSelect(Interface):
    def selectCipher(client, availableCiphers, offeredCiphers):
       """
       Returns 1 cipher, must be from list of offered and available ones.
       `client` has additional info like IP etc. This is called in the middle of
       TLS negotiation by the framework.
       """

PS: sorry for broken markup, Trac just sucks. Can't edit posted stuff etc.

Rgd curves: I agree, it's a whole new topic, however, from my current (clearly incomplete) understanding, EC ciphers are only uniquely identified when the curve to be used is also specified.

E.g. ECDHE-RSA-AES256-GCM-SHA384 + NIST P-224 is unique.

comment:49 Changed 12 months ago by oberstet

Rgd OP_SINGLE_DH_USE: I think the OpenSSL docs might miss an important point.

If the DH params used are sane (no weak group, strong primes), then it is safe to use DH with an ephemeral key generated at server start to achieve PFS - where ephemeral == lifetime of server run.

However, if an attacker manages to break into the server while the server is running (hasn't restarted), and he manages to get the ephemeral key plus the server private key plus the recorded encrypted traffic, then he breaks the PFS.

If your server has an uptime of months, that might be a real issue.

Of course you can mitigate by setting OP_SINGLE_DH_USE, but there is a real performance hit in particular with DH (little less with ECDH key) being generated for each and every session.

Personally, I won't use that in production, but will regenerate keys every N secs, where N is <1d e.g. and create a new TLS context accordingly (since OpenSSL seems to lack machinery for such thing).

comment:50 Changed 12 months ago by oberstet

FWIW, after considerable amount of reading / experimentation, this is the cipher list I'll be going to use for now:

ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:DHE-RSA-AES128-SHA

There are multiple reasons, like making the list explicit (avoiding the pattern matching language of OpenSSL) and short (it's exactly 6 ciphers) - if you want Windows XP support, you'll need to add another one. We don't.

Regarding the choice of elliptic curve with ECDH, we will use NIST P256 only. More details can be found here:

In addition, I've done a little more fine-tuning deactivating certain TLS options .. details are here:

comment:51 Changed 12 months ago by hynek

JFTR, I looked into the source code of nginx and it sets SSL_OP_SINGLE_DH_USE and SSL_OP_SINGLE_ECDH_USE unconditionally so we’re on the right track with our defaults in OpenSSLCertificateOptions.

comment:52 Changed 11 months ago by hynek

  • Branch changed from branches/acceptable-ciphers-6663 to branches/acceptable-ciphers-6663-2

(In [40955]) Branching to acceptable-ciphers-6663-2.

comment:53 Changed 11 months ago by hynek

  • Keywords review added

I’m happy to offer a first implementation of the mechanisms jp described for review. It’s the result of an fantastic sprint in Bristol everyone of you should have attended. :)

All bots at http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/acceptable-ciphers-6663-2 are all green except for twistedchecker whose complains are either benign, unavoidable, or don’t make any sense.

comment:54 Changed 11 months ago by hynek

  • Owner hynek deleted

comment:55 Changed 10 months ago by Alex

  • Keywords review removed
  • Owner set to hynek

Review notes:

  • ssl.xhtml should document the security considerations around utilizing SSLv3, as well as any possibly client concerns (i.e. document what hte impact of enabling it are)
  • The notes file could be written in slightly more idiomatic english as: "now allows the user to set the acceptable ciphers and uses secure ones by default"

comment:56 Changed 10 months ago by hynek

(In [41008]) Idiomize the topfile entry

refs #6663

comment:57 Changed 10 months ago by hynek

  • Keywords review added
  • Owner hynek deleted

Thanks, I have idiomized the top file.

As for ssl.xhtml: I consider adding information on various methods a separate issue and would happily work with you on it. However within this ticket, I’ve just reflowed the paragraph and fixed a factual error. I would prefer to paint the crypto bikeshed on a different ticket. :)

Therefore re-submitting for review.

comment:58 Changed 10 months ago by exarkun

  • Keywords review removed
  • Owner set to hynek

Thanks hynek. Very exciting work. :)

  1. doc/core/howto/ssl.xhtml
    1. Nice, thanks for updating this.
    2. Perhaps twisted.internet.interfaces.IAcceptableCiphers would make as much or more sense as the thing linked here? pydoctor will automatically generate a link to AcceptableCiphers from the IAcceptableCiphers page (see eg (https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.IAddress.html>). Or the docs could link to both.
  2. twisted/internet/_sslverify.py
    1. zope.interface import should go above Twisted imports and below stdlib imports.
    2. OpenSSLCertificateOptions.__init__ should definitely document the type of acceptableCiphers using the interface - L{IAcceptableCiphers} provider
    3. OpenSSLCertificateOptions.__init__ could already raise ValueError and failed to document this. Since you're adding a new case where this can be raised, can you add documentation at least for that new case (but if you document the other cases too that'd be pretty great :).
    4. The text of the new ValueError should be adjusted probably, it talks about a cipher string but it should probably talk about "IAcceptableCiphers" or something.
    5. OpenSSLCertificateOptions.cipherString can probably be private. There should also be some @ivar docs for it and we should be super clear about whether it is bytes or unicode (see comment below regarding ICipher.fullName).
    6. Similarly, _expandCipherString documents the type of cipherString as str but it should either be bytes or unicode.
    7. And again for OpenSSLAcceptableCiphers.fromOpenSSLCipherString
    8. if e.args[0][0][2] == 'no cipher match': I'm sorry about pyOpenSSL error reporting :(
    9. There are a bunch of links to information about selecting ciphers in the history of this ticket. Some of them might be useful as comments near the definition of defaultCiphers, I bet.
  3. twisted/internet/interfaces.py
    1. The documentation for ICipher.fullName should be very clear about which of the many possible string types this is. OpenSSL considers these things bytes but I suspect it uses an ASCII subset so we could consider it text and do the encoding and decoding, if we want. Since this is ostensibly user-facing information, text does seem to make more sense than bytes. I think?
  4. twisted/test/test_sslverify.py
    1. test_doesNotSwallowOtherSSLErrors doesn't need to use a MonkeyPatcher explicitly. trial's TestCase supplies a patch method (which also does the unpatching for you).
    2. test_eqSameClass and test_eqSameNameDifferentType make me sad once again for our lack of a re-usable testing helper to verify the common, obvious, standard behavior for == and !=. :/
    3. For test_doesNotSwallowOtherSSLErrors, can you get the desired Error by making some Context calls? That would seem to provide a better test since it would actually exercise the obscure, sad error matching code against pyOpenSSL instead of only against a hopefully-equivalent-but-who-really-knows fake implementation. (if not, oh well, I understand, there are lots of error cases that I know OpenSSL is capable of generating but that I am incapable of reliably triggering.)
    4. If test_returnsListOfICiphers fails because one of the objects does not provide ICipher then I expect it will be annoying to debug because it won't be clear which object (or even how many, since all will stop after one) did not provide the interface.
    5. Also, use the interface from the correct module - twisted.internet.interfaces not twisted.internet._sslverify.
  5. Here and there, a closing ) is included on a line by itself indented to match the statement where the matching ( began. I think the more common convention throughout Twisted is for it to be indented to match the lines enclosed by the ( / ) pair. I don't think this is part of the coding standard and I don't think twistedchecker complains about it. If you don't mind I think it'd be nice to be consistent with the latter convention (iow, indent the ) by one more level). This is a completely optional review comment. If I really want to enforce this then I'll have to get some people to agree it's worth being consistent about, fix the coding standard, and add a feature to twistedchecker...

Thanks again! I don't think there are any very serious issues pointed out by this review. Please let me know if I can clarify any of the comments or help with anything else!

comment:59 Changed 10 months ago by hynek

  • Keywords review added
  • Owner hynek deleted

I believe to have addressed all of your concerns with following exceptions:

  • 4.3 Do you really want to go there? Creating fake errors by doing something wrong seems very brittle to me, especially with something that’s brittle itself like OpenSSL.
  • 5 I really prefer my style a lot more since it looks better, it’s more readable, and frames explicitly the arguments – it appears like a unity. I believe the only reason for indented closing parens is that Emacs does that. ;)

There is one problem left unfortunately and to get attention I’m putting it back into review:

The generated twisted.internet.interfaces.IAcceptableCiphers.html doesn’t cope well with the renaming of twisted.internet._sslverify.OpenSSLAcceptableCiphers to twisted.internet.ssl.AcceptableCiphers:

Known implementations: twisted.internet._sslverify.OpenSSLAcceptableCiphers

Anyone any ideas how to fix this? As far as I can see it, this is the last remaining blocker.

Other comments on the proposed code are welcome too, of course.

comment:60 Changed 10 months ago by exarkun

4.3 Do you really want to go there? Creating fake errors by doing something wrong seems very brittle to me, especially with something that’s brittle itself like OpenSSL.

Huh. Weird. I would have said that what the test currently does is create a fake error and my suggestion was to provoke a real error. :) Put another way, I'd like to see the test and the implementation both trigger the same code paths in OpenSSL in order to produce the exception that the implementation then tries to handle. That seems better than the test and the implementation each following unrelated code paths?

I really prefer my style a lot more since it looks better, it’s more readable, and frames explicitly the arguments – it appears like a unity. I believe the only reason for indented closing parens is that Emacs does that. ;)

Hrm. I think that "Emacs does that" is as valid a justification as any other (or perhaps I should say all other justifications are just as invalid as "Emacs does that"). But, as I said, optional...

Anyone any ideas how to fix this? As far as I can see it, this is the last remaining blocker.

Seems like a bug that just needs to be fixed in pydoctor. I don't think fixing it needs to block this branch. We can update our pydoctor version at any time.

comment:61 Changed 10 months ago by hynek

There doesn’t seem to be anything open AFAICT.

comment:62 Changed 10 months ago by hynek

JFTR, the pydoctor bug is already fixed as of http://bazaar.launchpad.net/~mwhudson/pydoctor/dev/revision/610 .

comment:63 Changed 9 months ago by exarkun

  • Keywords review removed
  • Owner set to hynek
  1. The news fragment misspells the name of the changed object
  2. in the howto, Since Twisted uses a secure cipher string by default - instead of string perhaps collection or suite or even configuration.
  3. twisted/internet/_sslverify.py
    1. Hmm I wonder what multiple @raise ValueError in a single docstring does.
    2. The docs for fullName on OpenSSLCipher should do C{u"..."} instead of C{...} for the example name
  4. twisted/test/test_sslverify.py
    1. The IAcceptableCiphers implementation in test_honorsAcceptableCiphersArgument should declare what it implements.
    2. It'd be nice not to duplicate the skip message everywhere. A better pattern can be seen in how skipSSL is used in twisted/internet/test/test_endpoints.py.
    3. test_repr shouldn't use the word should.
    4. In the docstring for test_returnsListOfICiphers I would say Always returns instead of Returns always. Complete sentences are nice too, so there should be a subject (what always returns?). This can be It if you like, but I tend to prefer to repeat the name of the thing being tested.

That's all, I think. The only other comment I have is that I noticed that the Python 3 builder doesn't actually have pyOpenSSL installed. :( So I don't think the Python 3 compatibility of this code is actually being tested on buildbot anywhere. That'd be a good thing to fix (and then, since it has been untested for a while, I imagine there are things to fix - unless you did any Python 3 testing locally). The rest of these comments are all minor doc issues so please merge after they're addressed. Thanks.

comment:64 Changed 9 months ago by hynek

  • 3.1. It creates multiple raises entries and is nicely readable.
  • I run my tests locally with Python 3 and PyOpenSSL and it works fine. :)

The rest has been addressed.

comment:65 Changed 9 months ago by hynek

  • Resolution set to fixed
  • Status changed from new to closed

(In [41218]) Merge acceptable-ciphers-6663-2

Author: hynek
Reviewer: exarkun
Fixes: #6663

twisted.internet.ssl.CertificateOptions now allows the user to set acceptable
ciphers and uses secure ones by default.

comment:66 Changed 6 months ago by zooko

Hi folks, I was just looking at [41218]. Thank you for working on this! It looks like a good patch, and I'm glad to see progress.

This patch fixes #5514, so I'll close that one.

Note: See TracTickets for help on using tickets.