TLSv1.3: Add TLSv1.3 SSL_METHOD and other base plumbing changes by mattcaswell · Pull Request #1804 · openssl/openssl

mattcaswell · 2016-10-30T09:36:44Z

Checklist

documentation is added or updated
tests are added or updated

Description of change

Add the SSL_METHOD for TLSv1.3 and all other base changes required.

Includes addition of the various options to s_server/s_client. Also adds one of the new TLS1.3 ciphersuites.

This isn't "real" TLS1.3!! It's identical to TLS1.2 apart from the protocol name/number and the ciphersuite...and the ciphersuite is just a renamed TLS1.2 one (not a "real" TLS1.3 ciphersuite).

This will form the basis for future TLS1.3 implementation work.

As this is not a working TLS1.3 implementation at this stage it is disabled by default. Use "enable-tls1_3" to built it.

FdaSilvaYY · 2016-10-30T12:53:48Z

test/testutil.c

@mattcaswell : any reason for this magic value ?

Doh! Yes, there is a reason. It's debug code that crept in! It shouldn't be there.

richsalz · 2016-10-30T13:20:54Z

don't we use tls11 and tls12 elsewhere? Why tls1_3 here?

richsalz · 2016-10-30T13:42:55Z

doc/man3/SSL_CTX_new.pod

I think we should make it wrong to create a hole

And the TLS 1.3 version negotiation actually would allow expression such holes. We also added functions that don't allow creating such holes as documented in the previous paragraph. We even document what we do when you do create holes below.

But actually disallowing the creation of a hole would be a breaking change, so probably not appropriate for 1.1.1. Possibly for 1.2.0....but in any case out of scope of this PR.

you can say "no holes if you use 1.3" since 1.3 is new stuff. but okay.

This is not exactly what @richsalz was proposing, but if you make the API call error out and do nothing when attempting to create a hole, then the calling application has to have extra logic for what order to disable versions in, in order to succeed. I suppose that SSL_{connect,accept} could implement the check and avoid that particular failure mode, if needed.

kroeckx · 2016-10-30T16:10:15Z

include/openssl/tls1.h

Is there a reason you don't follow the pattern we use for the other ciphers? I would expect AES128-GCM-SHA256. I'm not sure where the "TLS" part comes from. Note that we already have a cipher named AES128-GCM-SHA256 and I'm not sure it can handle multiple with the same name or not.

Selection of ciphers by name will get confused on duplicates. We went with AEAD-AES128-GCM-SHA256 which isn't great but avoids the collision with static RSA ciphers. (Whyyyy did they have to be named that way??? :-( )

Although we're probably also going to just disconnect the 1.3 ciphers from the old cipher language entirely. Otherwise everything which configures lists of ciphers will break in a world where 1.3 is on by default and the cipher selection in 1.3 involves far fewer complex tradeoffs between broken things, Cartesian products, etc.

TLS1.3 ciphersuites are quite different to <=TLS1.2 ciphersuites. They only specify the AEAD alg, and HKDF hash alg. Key exchange/auth are negotiated separately. So while, by our old name scheme AES128-GCM-SHA256 might sound right, it really isn't...and it has a different id to that already existing ciphersuite. TLS1.3 ciphersuites do not work for <TLS1.3. I'm not sure that AEAD-AES128-GCM-SHA256 is quite right either....the AEAD bit somehow implies that the TLS1.2 ciphersuites aren't AEAD! TLS1-3-AES128-GCM-SHA256 or similar might be ok, but that's not actually far from what I've got. I went with the above name because, given that the old name scheme doesn't really work for TLS1.3 anyway, I thought it was a good opportunity to align with the standard names. This is the name as it is specified in the RFC. I've never really understood why our names have to be different (well ok, I know its for "historical reasons", but its annoying).

The number of people who are familiar with OpenSSL names greatly exceeds the few-dozen people who are familiar with the "official" RFC names.

Before we go too far worry about names, I agree with @davidben that we should decouple the cipher spec from the current CIPHERS variable.

I don't overly care too much whether its the standard name or not (although I do think standardisation would be a good thing). It needs to be somehow different to the current naming scheme, to disambiguate it from the existing ciphersuites. If we don't like my suggested approach of going with the RFC names, then please make an alternative suggestion.

WRT, decoupling from the current cipherstring processing: that may well be where we end up...but that's for a later stage in the development. Not right now.

mattcaswell · 2016-10-30T23:52:04Z

@richsalz

don't we use tls11 and tls12 elsewhere? Why tls1_3 here?

Your comment was just standalone and not against a particular line, so I'm not sure which specific usage you are referring to. Generally though we use tls1, tls1_1 and tls1_2 at the moment (e.g. in option flag names etc), so tls1_3 is consistent.

mattcaswell · 2016-10-31T00:04:04Z

Updated commits pushed to remove the accidentally added debug code.

richsalz · 2016-10-30T13:45:02Z

ssl/s3_lib.c

shouldnt this bre wrapped in no_tls13 ifdef?

I'm not sure. We don't do that at the moment for any of the TLSv1.2 specific ciphers. I'd rather not litter the code with lots of ifdef if we don't have to, and it won't break if you compile without TLSv1.3 (and you won't be able to use these ciphers)....? Will do if you really want it though.

and if we go down that route would we also have to put ifdef's around all the other ciphers for if you disabled everything but TLSv1.3? Doesn't sound nice :-(

richsalz · 2016-10-30T13:47:34Z

ssl/ssl_lib.c

make a switch? at least get rid of the else after return

Done (made it a switch)

richsalz · 2016-10-31T01:28:41Z

I am concerned about the "get to it later."
I suggest "TLS13_AES" and "TLS1_CHACHAPOLY"

vdukhovni · 2016-10-31T07:40:41Z

In retrospect, the entire "SSL_OP_NO_..." interface was a bad idea, and we've replaced it with a semantically cleaner interface for setting the min/max supported protocol versions. Users should be strongly encouraged to use that instead.

As for holes with SSL_OP_NO_... they should still be avoided. The client-side semantics should likely remain that only the protocols below the lowest hole end up enabled. Alternatively, the client-side hello setup could always fail if holes are configured.

Changing the behaviour of existing functions strongly deters adoption of the toolkits in OS distributions. So we can only introduce better interfaces, but need to very much avoid changing existing ones.

mattcaswell · 2016-10-31T09:56:12Z

I am concerned about the "get to it later."

@richsalz your comment was standalone again...I'm assuming this was in reference to my comment:

WRT, decoupling from the current cipherstring processing: that may well be where we end up...but that's for a later stage in the development. Not right now.

If so, then there is a huge pile of stuff that we need to get to later that this PR raises....like the whole of the rest of the TLS1.3 implementation ;-). This is just one aspect! At this stage we're really not in a position to do anything much with the cipherstring selection processing. I've only added a single cipherstring...and its just a TLS1.2 cipher under the hood really. I think we really need to get some experience of actually implementing real TLS1.3 ciphersuites before we look at this...not to mention that without the underlying ciphersuite logic, doing cipherstring rule processing would be difficult to test. I've added a TODO in the code though so we don't forget about it. There's going to be a lot of TODOs over the coming PRs for stuff we need to "get to later".

I suggest "TLS13_AES" and "TLS1_CHACHAPOLY"

I assume you meant "TLS13_CHACHAPOLY". I'd like to avoid the "13" bit of that. It might make sense now, but in a few years time when TLS1.4 comes along with backwards compatible ciphersuites, its going to be annoying and difficult to get rid of. Also are you suggesting to use "_" as the separator? I used it, because that's what standard RFC names use. All the rest of our ciphersuites use "-" (i.e. minus sign). Also, did you really mean "TLS13_AES"? Or perhaps you meant "TLS13_AES_128_GCM_SHA256"? You really need to include the mode and hash.

Just dropping the "13" bit of the above gives you "TLS_AES_128_GCM_SHA256"...which is the same as my original proposal...or "TLS-AES-128-GCM-SHA256" if you prefer "-" as the separator.

mattcaswell · 2016-10-31T10:04:14Z

Added a couple of commits, to insert a TODO around cipherstring selection, and convert a big "if" into a "switch" as per review comments.

richsalz · 2016-10-31T10:55:17Z

I prefer to see the TLS13- prefix in the cipher names for now. We can clean it up later.

richsalz · 2016-10-31T11:01:25Z

note that we have OPENSSL_NO_CHACHA and OPENSSL_NO_POLY135 (I think it silly that there are two, but so be it). So you need that in the cipher table, too?

Or change config to disallow no-chacha/no-poly if you enable tls 1.3

mattcaswell · 2016-10-31T11:04:28Z

Well I've not added any chacha/poly ciphersuites for TLS1.3 yet. I've only added a single AES one so I had something to test against. The others will be added when we actually do the proper ciphersuite implementation.

levitte · 2016-10-31T13:18:31Z

include/openssl/ssl.h

 # define SSL_OP_NO_TLSv1                                 0x04000000U
 # define SSL_OP_NO_TLSv1_2                               0x08000000U
 # define SSL_OP_NO_TLSv1_1                               0x10000000U
+# define SSL_OP_NO_TLSv1_3                               0x20000000U


At some point, we need to rethink this bitfield... the way it looks now, it's two more TLS versions and then we're toast.

By then we will probably have ditched it anyway, based on the other comments about "holes" above.

Includes addition of the various options to s_server/s_client. Also adds one of the new TLS1.3 ciphersuites. This isn't "real" TLS1.3!! It's identical to TLS1.2 apart from the protocol and the ciphersuite...and the ciphersuite is just a renamed TLS1.2 one (not a "real" TLS1.3 ciphersuite).

When matching a ciphersuite if we are given an id, make sure we use it otherwise we will match another ciphersuite which is identical except for the TLS version.

Also we disable TLS1.3 by default (use enable-tls1_3 to re-enable). This is because this is a WIP and will not be interoperable with any other TLS1.3 implementation. Finally, we fix some tests that started failing when TLS1.3 was disabled by default.

mattcaswell · 2016-11-02T11:56:59Z

Right I've renamed the ciphersuite to "TLS13-AES-128-GCM-SHA256" for now. I'm not entirely happy about it (I really don't like the TLS13 bit), but in the interests of getting things to move on I'll go with that for now. There was already a big TODO about reviewing the ciphersuite naming. Updated commits pushed.

Is there anything else holding this one up? I'd really like to get this merged.

mattcaswell · 2016-11-02T13:11:29Z

Merged. Thanks.

mattcaswell changed the title ~~Tls1 3 base~~ TLSv1.3: Add TLSv1.3 SSL_METHOD and other base plumbing changes Oct 30, 2016

mattcaswell mentioned this pull request Oct 30, 2016

TLSv1.3: Upgrade ossltest and TLSProxy #1805

Closed

1 task

FdaSilvaYY reviewed Oct 30, 2016

View reviewed changes

richsalz reviewed Oct 30, 2016

View reviewed changes

kroeckx reviewed Oct 30, 2016

View reviewed changes

mattcaswell force-pushed the tls1_3-base branch from 936ddbf to 5a6fb1d Compare October 31, 2016 00:03

richsalz suggested changes Oct 31, 2016

View reviewed changes

mattcaswell mentioned this pull request Oct 31, 2016

TLSv1.3: Use the supported_versions extension for version negotiation #1808

Closed

1 task

levitte reviewed Oct 31, 2016

View reviewed changes

mattcaswell added 4 commits November 2, 2016 11:50

Use ciphersuite id when matching if we've got one

22fe1b7

When matching a ciphersuite if we are given an id, make sure we use it otherwise we will match another ciphersuite which is identical except for the TLS version.

Convert a big "if" into a "switch"

1d0c6fd

mattcaswell force-pushed the tls1_3-base branch from ab35a78 to 1d0c6fd Compare November 2, 2016 11:55

richsalz approved these changes Nov 2, 2016

View reviewed changes

mattcaswell closed this Nov 2, 2016

nocturne-mit mentioned this pull request Jul 31, 2019

add SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version pyca/pyopenssl#860

Closed

Uh oh!

Conversation

mattcaswell commented Oct 30, 2016 • edited by levitte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Oct 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Oct 30, 2016

Uh oh!

mattcaswell commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Oct 31, 2016

Uh oh!

vdukhovni commented Oct 31, 2016

Uh oh!

mattcaswell commented Oct 31, 2016

Uh oh!

mattcaswell commented Oct 31, 2016

Uh oh!

richsalz commented Oct 31, 2016

Uh oh!

richsalz commented Oct 31, 2016

Uh oh!

mattcaswell commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Nov 2, 2016

Uh oh!

mattcaswell commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mattcaswell commented Oct 30, 2016 •

edited by levitte

Loading