Opened 3 years ago

Closed 3 years ago

#6772 enhancement closed fixed (fixed)

twisted.internet.ssl.CertificateOptions should default to supporting TLSv1.1 and TLSv1.2 (in addition to TLSv1.0)

Reported by: amluto Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: hynek Branch: branches/default-ssl-version-6772
(github, coverage, patch, buildbot, log)
Author: exarkun

Description (last modified by exarkun)

It currently defaults to supporting TLS v1.0 only. This is good - in that it excludes SSLv2 and SSLv3. It is bad in that it excludes the newer TLSv1.1 and TLSv1.2.

An implementation strategy (sadly not an obvious one) for making this change with the OpenSSL API is to create a Context using SSLv23_METHOD and then set the OP_NO_SSLv2 and OP_NO_SSLv3 options.

Change History (10)

comment:1 Changed 3 years ago by exarkun

  • Description modified (diff)
  • Summary changed from internet._sslverify.OpenSSLCertificateOptions should default to SSLv23_METHOD to twisted.internet.ssl.CertificateOptions should default to supporting TLSv1.1 and TLSv1.2 (in addition to TLSv1.0)

comment:2 Changed 3 years ago by exarkun

Care should also be taken to not break uses like CertificateOptions(method=SSLv3_METHOD) - that is, if the application explicitly requests a version of SSL/TLS, CertificateOptions shouldn't override that choice.

Perhaps this means that the default should be SSLv23_METHOD w/ OP_NO_SSLv{2,3} but if any value at all is passed for method then the disable options are disabled?

Unfortunately there is no way to specify additional options via the CertificateOptions interface - so, for example, there's no way for an application to say that it wants TLSv1.1 or TLSv1.2 since the way that must be spelled using the OpenSSL API is:

ctx = Context(SSLv23_METHOD)
ctx.set_options(OP_NO_SSLv2 | OP_NO_SSLv3 | OP_NO_TLSv1)

Perhaps we should fix the signature of CertificateOptions so that it accepts a list of methods that are acceptable? This preserves the somewhat abstract interface, provides the additional functionality, and is not too difficult to translate into the underlying OpenSSL API calls. Also perhaps (nay, almost certainly) this is fodder for a future ticket, not something that needs to be resolved here.

comment:3 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/default-ssl-version-6772

(In [40326]) Branching to 'default-ssl-version-6772'

comment:4 Changed 3 years ago by exarkun

  • Keywords review added

The branch changes the default to SSLv23_METHOD and adds use of OP_NO_SSLv3 when necessary.

comment:5 Changed 3 years ago by hynek

  • Cc hynek added

comment:6 Changed 3 years ago by hynek

  • Keywords review removed
  • Owner set to exarkun

Thanks for working on this, we need to get our TLS into 2013.

  • I’ve forced a new build.
  • The parens in for (opt, exclude) in self._EXCLUSION_OPS.items(): aren’t necessary and IMHO hinder readability.
  • I find the name of test_SSLv2IsDisabledForSSLv23 misleading since it’s primarily testing for the lack of NO_SSLv3 if SSL_v23 is specified explicitly. Checking that v2 is always off seems like another test to me.
  • Having a self._options makes me sort of uneasy because one doesn’t know where the values come from. But since everything gets set up in the constructor it’s okay I guess.
  • There is no explicit way to achieve the default state except for setting it to None, are we okay with that? What if someone wants to allow only TLS 1.2? I have the feeling that our current version mechanism are profoundly broken. It’s not only bound to OpenSSL, the underlying mechanisms of OpenSSL are also terrible (as OpenSSL is wont to be…). I guess this is a new ticket, but I’d really like an argument that gets a list/tuple of constants of transport versions it wants to support. It wouldn’t be hard to make method support that either (it could be either an pyopenssl thing which would be deprecated or a tuple). I understand this shouldn’t hinder us from making our current API more secure. Now I just saw that you wrote just that in one of the comments. :)

Next steps:

  • I would like to see a ticket for the method selection and would help to make it happen.
  • I leave it to your judgement to decide whether you want to put it again for review; I for one trust you can fix the things I’ve mentioned. :)

comment:7 Changed 3 years ago by hynek

One more thing: I forgot to actually test the code in real life. But yes, it works:

$ openssl s_client -tls1_2 -connect localhost:443
[...]
New, TLSv1/SSLv3, Cipher is AES256-GCM-SHA384
Server public key is 2048 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : AES256-GCM-SHA384

and

$ openssl s_client -tls1_1 -connect localhost:443
[...]
New, TLSv1/SSLv3, Cipher is AES256-SHA
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
SSL-Session:
    Protocol  : TLSv1.1
    Cipher    : AES256-SHA

Me likes. :)

comment:8 Changed 3 years ago by exarkun

twistedchecker complains that there’s not empty line btw the @ivar and @type of OpenSSLCertificateOptions

This doesn't make sense as far as I can tell. Sounds like <https://bugs.launchpad.net/twistedchecker/+bug/1093502>.

The changes to the SSL import logic added a new pyflakes warning: https://buildbot.twistedmatrix.com/builders/pyflakes/builds/1149/steps/pyflakes/logs/new%20pyflakes%20errors

Yea. Maybe pyflakes upstream has fixed this spurious warning (doesn't look like it)? It is spurious though...

pydoctor doesn’t like the reference to OpenSSL.SSL.Context

Hmm. Indeed. Yet a reference it is. I think this is something we should fix. I should be able to link cross-project. pydoctor already supports this with the stdlib, perhaps we just need to supply the right glue for each third-party project want it to work for.

The parens in for (opt, exclude) in self._EXCLUSION_OPS.items(): aren’t necessary and IMHO hinder readability.

Hmm. I don't feel very strongly either way (I guess I thought they helped readability or I wouldn't have typed them though :). Twisted is split on the issue. Approximately 150 for loops with unnecessary parens and approximately 250 without. I'd say it's a judgement call and not something to call out in a review (except perhaps in for (foo) in bar: :). If you disagree, please file a ticket to update the coding standard. I'm happy to make this change this time though.

I find the name of test_SSLv2IsDisabledForSSLv23 misleading since it’s primarily testing for the lack of NO_SSLv3 if SSL_v23 is specified explicitly. Checking that v2 is always off seems like another test to me.

Hmm, I see what you mean. Good names don't jump out at me, but I can think of a simpler name at least.

Having a self._options makes me sort of uneasy because one doesn’t know where the values come from. But since everything gets set up in the constructor it’s okay I guess.

Given the awkwardness of the OpenSSL APIs we have to work with... yea. It's not clear how this might be done better. The only idea I have is that the options could be re-computed every time a new Context is to be created. I don't think that particularly helps readability.

There is no explicit way to achieve the default state except for setting it to None, are we okay with that?

I guess I'm okay with that for now. Another way to say this is "The way to achieve the default state is to pass None for the method." :) That sounds like the usual idiom for defaults.

What if someone wants to allow only TLS 1.2?

With a sufficiently new pyOpenSSL, they can pass TLSv1_2_METHOD. With pyOpenSSL 0.13 and older I don't think it's possible to declare you only want TLS 1.2.

I have the feeling that our current version mechanism are profoundly broken. It’s not only bound to OpenSSL, the underlying mechanisms of OpenSSL are also terrible (as OpenSSL is wont to be…). I guess this is a new ticket, but I’d really like an argument that gets a list/tuple of constants of transport versions it wants to support.

I agree completely. See #6800.

comment:9 Changed 3 years ago by exarkun

(In [40461]) address some review feedback

refs #6772

comment:10 Changed 3 years ago by exarkun

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

(In [40462]) Merge default-ssl-version-6772

Author: exarkun Reviewer: hynek Fixes: #6772

Change twisted.internet.ssl.CertificateOptions so that if no protocol version is explicitly requested then any version of TLS (currently v1.0, v1.1, or v1.2) will be accepted. Previously only the TLSv1.0 protocol was allowed in this case.

Note: See TracTickets for help on using tickets.