TLSv1.3: Extensions refactor and add EncryptedExtensions message#2015
TLSv1.3: Extensions refactor and add EncryptedExtensions message#2015mattcaswell wants to merge 44 commits intoopenssl:masterfrom
Conversation
mattcaswell
force-pushed
the
tls1_3-encrypted-extensions
branch
from
710d5a5 to
578e06c
Compare
|
Rebased this on top of the latest master. I also fixed a bug I encountered in the process and added a couple of extra tests. This PR is now only dependant on #1975, so please ignore the first 7 commits. |
There are some minor differences in the format of a ServerHello in TLSv1.3.
At this stage the message is just empty. We need to fill it in with extension data.
Subsequent commits will pull other extensions code into this file.
This builds on the work started in 1ab3836 and extends is so that each extension has its own identified parsing functions, as well as an allowed context identifying which messages and protocols it is relevant for. Subsequent commits will do a similar job for the ServerHello extensions. This will enable us to have common functions for processing extension blocks no matter which of the multiple messages they are received from. In TLSv1.3 a number of different messages have extension blocks, and some extensions have moved from one message to another when compared to TLSv1.2.
Add support for construction of extensions
Later we will have client extensions code too.
…work This lays the foundation for a later move to have the extensions built and placed into the correct message for TLSv1.3 (e.g. ServerHello or EncryptedExtensions).
The _clienthello_ in the extensions parsing functions is overly specific. Better to keep the convention to just _client_
Remove some functions that are no longer needed now that we have the new extension framework.
Because extensions were keyed by type which is sparse, we were continually scanning the list to find the one we wanted. The way we stored them also had the side effect that we were running initialisers/finalisers in a different oder to the parsers. In this commit we change things so that we instead key on an index value for each extension.
In TLS1.3 some ServerHello extensions remain in the ServerHello, while others move to the EncryptedExtensions message. This commit performs that move.
Extend test_tls13messages to additionally check the expected extensions under different options given to s_client/s_server.
Repeat for various handshake types
The s_server option -status_file has been added so this test can be enabled.
test/recipes/70-test_sslmessages.t
Outdated
| checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, | ||
| checkhandshake::DEFAULT_EXTENSIONS | ||
| | checkhandshake::STATUS_REQUEST_CLI_EXTENSION, | ||
| "status_request handshake test (client)"); |
There was a problem hiding this comment.
Indentation is off by one space on all three lines above
test/recipes/70-test_sslmessages.t
Outdated
| $proxy->start(); | ||
| checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, | ||
| checkhandshake::DEFAULT_EXTENSIONS, | ||
| "status_request handshake test (server)"); |
test/recipes/70-test_sslmessages.t
Outdated
| checkhandshake::DEFAULT_EXTENSIONS | ||
| | checkhandshake::STATUS_REQUEST_CLI_EXTENSION | ||
| | checkhandshake::STATUS_REQUEST_SRV_EXTENSION, | ||
| "status_request handshake test"); |
There was a problem hiding this comment.
Indentation off by one on the four lines above
test/recipes/70-test_sslmessages.t
Outdated
| checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, | ||
| checkhandshake::DEFAULT_EXTENSIONS | ||
| | checkhandshake::SERVER_NAME_CLI_EXTENSION, | ||
| "Server name handshake test (client)"); |
There was a problem hiding this comment.
Indentation is off on the last line above
test/recipes/70-test_sslmessages.t
Outdated
| $proxy->start(); | ||
| checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, | ||
| checkhandshake::DEFAULT_EXTENSIONS, | ||
| "Server name handshake test (server)"); |
test/recipes/70-test_tls13messages.t
Outdated
| checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, | ||
| checkhandshake::DEFAULT_EXTENSIONS | ||
| | checkhandshake::STATUS_REQUEST_CLI_EXTENSION, | ||
| "status_request handshake test (client)"); |
test/recipes/70-test_tls13messages.t
Outdated
| $proxy->start(); | ||
| checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, | ||
| checkhandshake::DEFAULT_EXTENSIONS, | ||
| "status_request handshake test (server)"); |
util/TLSProxy/EncryptedExtensions.pm
Outdated
| if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) { | ||
| $extensions .= pack("n", $key); | ||
| $extensions .= pack("n", length($extdata)); | ||
| $extensions .= $extdata; |
util/TLSProxy/EncryptedExtensions.pm
Outdated
| { | ||
| my $self = shift; | ||
| if (@_) { | ||
| $self->{extension_data} = shift; |
util/TLSProxy/Proxy.pm
Outdated
| { | ||
| my $self = shift; | ||
| if (@_) { | ||
| $self->{reneg} = shift; |
The indentation was a bit off in some of the perl files following the extensions refactor.
Travis was indicating a bogus uninit var warning. This fixes it.
|
Two new commits pushed. One is to address all of the indentation issues found by @levitte. The other is to address a travis failure. |
ssl/statem/statem_locl.h
Outdated
|
|
||
| __owur int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context, | ||
| RAW_EXTENSION **res, int *al); | ||
| __owur int tls_parse_extension(SSL *s, unsigned int idx, int context, |
There was a problem hiding this comment.
In ssl/statem/extensions.c, the definition is:
int tls_parse_extension(SSL *s, TLSEXT_INDEX idx, int context,
RAW_EXTENSION *exts, int *al);
It seems that --strict-warnings doesn't catch this. The VMS build does, though...
|
New commit pushed, fixing |
|
I'm currently hitting a failing |
|
It was a false alarm |
richsalz
left a comment
richsalz
left a comment
There was a problem hiding this comment.
+1 from me, with the caveat that I have not looked at the perl stuff at all.
|
Thanks. For the perl, Richard has been looking at that. @levitte, is the perl good to go now? |
|
Oh...you beat me to it! How should I reflect your respective plus ones in the Reviewed-by headers?? List you both for all commits? |
|
i'm in favor of a merge/squash to one commit which makes it easy. |
Yikes!! There are 44 commits in this PR...that sounds like a really bad idea. |
|
why? who really cares about most of the review changes? and if you use the "merged from..." syntax, folks can see the detailed commit history in the PR. Okay if you don't want to do it, just keep my name off the Perl code. |
|
All into just one is a bad idea, but perhaps some reduction? |
|
... or, I don't really care much. |
Because history is really important when analysing issues in the future. It helps you to understand how the code came to be as it is and some of the motivation behind that. Mega-commits also make
The perl code isn't separated out into separate commits. Typically I will make a change in the C, and then make a mirroring change in the perl so the tests still pass and then commit the whole lot in one go. How about this: I add both of you in the reviewed-by headers, but add a free text note before them saying: "Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich Salz". It will be a pain to add that to all the commits (addrev doesn't help here) - but I'd rather do that than one mega-commit. |
|
As an interested observer, I will note that though the commit mail sends a single diff for all the commits pushed together, I will frequently go and look at 'git log -p --reverse' output to try to understand the individual changes better. This doesn't work very well when there are sweeping fixup commits at the end of the chain that fix things that "look funny" in the earlier commits, so I end up back at the one giant diff, where it's more work to figure out what each chunk of diff is doing. Of course, I will deal with whatever you throw me, and it's not worth spending huge amounts of time rebasing to get things perfect, but I just wanted to get some more input in. |
|
@mattcaswell that's fine. |
|
@levitte does that work for you too? |
|
Yup |
I know what you mean, and generally I have a preference where there is a fix-up to do, to go back and |
| OPENSSL_free(s->s3->alpn_selected); | ||
| s->s3->alpn_selected = NULL; | ||
| if (s->server) { | ||
| s->s3->alpn_selected_len = 0; |
There was a problem hiding this comment.
Any reason not to move this outside the conditional?
| */ | ||
| /* TODO(TLS1.3): Add support for DHE groups */ | ||
| pcurves = s->tlsext_supportedgroupslist; | ||
| if (!tls1_get_curvelist(s, 0, &pcurves, &num_curves)) { |
There was a problem hiding this comment.
tls1_get_curvelist() does not read from its third argument, so the assignment in the previous line is dead code.
Since this idiom appears a few times in this file, it seems likely that the intent was to pass &s->tlsext_supportedgrouplist as the argument, but making the pcurves local const unsigned char **pcurves results in a "assigning to 'const unsigned char **' from 'unsigned char **' discards qualifiers in nested pointer types" error. And I didn't want to put the time in to get past that when I wasn't even sure that the right answer wasn't to remove the dead assignments.
There was a problem hiding this comment.
I think the right answer is to remove the dead assignments.
|
|
||
| /* Ignore this if we have no SRTP profiles */ | ||
| if (SSL_get_srtp_profiles(s) == NULL) | ||
| return 1; |
There was a problem hiding this comment.
We might be more friendly to the ecosystem if we moved this check after the parity check that happens next.
There was a problem hiding this comment.
I'm not really sure it makes a difference TBH. It's highly unlikely that we are ever going to receive this extension unless the client already knows that the server supports it. This is just a sanity check. If there are any clients out there that don't obey the parity check rule then they are never going to work with SRTP so they won't survive very long in the ecosystem anyway!
| * rest to verify the structure, but don't process them. | ||
| */ | ||
| if (found) | ||
| continue; |
There was a problem hiding this comment.
This seems to allow a client willing to send multiple key_shares to have us use client preference; do we want to enforce server preference?
There was a problem hiding this comment.
I guess that would be nice to have. I added it to my TLS1.3 "nice to have" TODO list.
4 participants
Checklist
Description of change
This is a major refactor/rewrite of the extension parsing and construction code. TLSv1.3 introduces a number of new messages where extensions may appear. Some extensions that previously appeared in the ServerHello, now move to the EncryptedExtensions message. This PR introduces a new extension framework which can be reused for whichever message needs to use it. Each built-in extension is given a definition consisting of:
Finally this is all brought together to introduced the new EncryptedExtensions message. I've also updated the ServerHello to be in the new TLSv1.3 format.