Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for RFC8446 (the final TLSv1.3 version) #6741

Closed
wants to merge 4 commits into from

Conversation

@mattcaswell
Copy link
Member

commented Jul 18, 2018

This will remain as WIP until such time as the RFC is actually published.

@mattcaswell mattcaswell added this to the 1.1.1 milestone Jul 18, 2018

@kaduk

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

I would be super-happy if this could be verified against the updated vectors in https://tools.ietf.org/html/draft-ietf-tls-tls13-vectors-06

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

I would be super-happy if this could be verified against the updated vectors in https://tools.ietf.org/html/draft-ietf-tls-tls13-vectors-06

See #6746

@kaduk
Copy link
Contributor

left a comment

-1 for the strangeness in test_sslcertstatus (but maybe I'm just confused)

# define TLS1_3_VERSION_DRAFT 0x7f1c
# define TLS1_3_VERSION_DRAFT_TXT_26 "TLS 1.3 (draft 26)"
# define TLS1_3_VERSION_DRAFT_TXT_27 "TLS 1.3 (draft 27)"
# define TLS1_3_VERSION_DRAFT_TXT "TLS 1.3 (draft 28)"

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

What's the harm in keeping these around? They'll still be around in the wild for a little while, at least.

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 11, 2018

Contributor

I do not think they will be around very long, and this is an LTS release. I think the right decision is to remove them.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

I am strongly opposed to keeping these around. Once they're in a stable release they will be with us for a very long time. As Rich says this is an LTS release. Lets get rid of the excess baggage now while we can. I don't think support for this in the wild will be around for very long - and we should do everything we can to encourage people to switch to the final version asap.

This comment has been minimized.

Copy link
@t-j-h

t-j-h Aug 13, 2018

Member

I agree. Remove the baggage. We are only supporting the RFC version not earlier drafts.

/*
* TODO(TLS1.3): There is some discussion on the TLS list as to whether
* we should include versions <TLS1.2. For the moment we do. To be
* reviewed later.

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

(The final text is quite explicit that clients MUST send supported_versions containing all versions of TLS which chey are prepared to negotiate.)

/*
* TODO(TLS1.3): There is some discussion on the TLS list about
* whether to ignore versions <TLS1.2 in supported_versions. At the
* moment we honour them if present. To be reviewed later

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

(This is also pretty clear in the final text; the server MUST use only supported_versions for version negotiation, if present.)

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 11, 2018

Contributor

The spec only constraints 1.3 servers, so I think there might be some work.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

I'm not aware of anything we need to do in this area.

/* TODO(TLS1.3): Remove these lines before release */
{TLS1_3_VERSION_DRAFT_26, TLS1_3_VERSION_DRAFT_TXT_26},
{TLS1_3_VERSION_DRAFT_27, TLS1_3_VERSION_DRAFT_TXT_27},
{TLS1_3_VERSION_DRAFT, TLS1_3_VERSION_DRAFT_TXT},

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

(i.e., the draft versions might still be useful in tracing for a bit)

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 11, 2018

Contributor

I could go either way on this as it's only diagnostic stuff. But then you get into "why not 28?" and "why 26" discussions. Cleanest to just remove it all.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

For the same reasons as above I am strongly opposed to keeping these around. They should die die die!

This comment has been minimized.

Copy link
@t-j-h

t-j-h Aug 13, 2018

Member

Exterminate.
They should go.
We don't expect to see anything other than the RFC version going forward - correct?

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

Correct (once the draft version implementations that are in the wild have withered away and died - which I expect to happen quickly)

@@ -40,8 +40,6 @@ my $proxy = TLSProxy::Proxy->new(

#Test 1: Sending a status_request extension in both ClientHello and
#ServerHello but then omitting the CertificateStatus message is valid
#TODO(TLS1.3): Temporarily disabling this test in TLS1.3 until we've completed
#the move the status request extension to the Certificate message.
$proxy->clientflags("-status -no_tls1_3");

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

So we're removing the comment but leaving -no_tls1_3 in place?
That doesn't seem right.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

This test is only really relevant to TLSv1.2. It's related to the presence or absence of the CertificateStatus message which doesn't exist in TLSv1.3. The comment was added early on in the process of implementing TLSv1.3, and I think it is just stale and can be removed.

#TODO(TLS1.3): Fix before release
0x7f, 0x1c; #TLSv1.3 (draft 28)
0x03, 0x05, #TLSv1.4
0x03, 0x04; #TLSv1.3

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

We're going from one version listed to two versions listed (but the length is still 4 bytes)?

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

This change was actually separated out into a separate commit in the PR because it is also a bug fix. The length was incorrect before (it should have been 2), and it wasn't listing TLSv1.4 which it should have done. This fixes it. The length of 4, which was wrong before, is now right.

# SignatureScheme of TLS 1.3, from
# https://tools.ietf.org/html/draft-ietf-tls-tls13-20#appendix-B.3.1.3
# TODO(TLS1.3) update link to IANA registry after publication
# SignatureScheme of TLS 1.3 from RFC8446

This comment has been minimized.

Copy link
@kaduk

kaduk Aug 11, 2018

Contributor

The IANA registry link has been live for a while at https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-signaturescheme if we did want to link to IANA instead of just citing the RFC.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 13, 2018

Author Member

Fixed.

@richsalz
Copy link
Contributor

left a comment

I'm mostly okay with this, just one open issue here I think. @mattcaswell if you think there's no issue, reply and I will approve. The PSK stuff still has to be handled, as in a separate PR.

# define TLS1_3_VERSION_DRAFT 0x7f1c
# define TLS1_3_VERSION_DRAFT_TXT_26 "TLS 1.3 (draft 26)"
# define TLS1_3_VERSION_DRAFT_TXT_27 "TLS 1.3 (draft 27)"
# define TLS1_3_VERSION_DRAFT_TXT "TLS 1.3 (draft 28)"

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 11, 2018

Contributor

I do not think they will be around very long, and this is an LTS release. I think the right decision is to remove them.

/*
* TODO(TLS1.3): There is some discussion on the TLS list about
* whether to ignore versions <TLS1.2 in supported_versions. At the
* moment we honour them if present. To be reviewed later

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 11, 2018

Contributor

The spec only constraints 1.3 servers, so I think there might be some work.

/* TODO(TLS1.3): Remove these lines before release */
{TLS1_3_VERSION_DRAFT_26, TLS1_3_VERSION_DRAFT_TXT_26},
{TLS1_3_VERSION_DRAFT_27, TLS1_3_VERSION_DRAFT_TXT_27},
{TLS1_3_VERSION_DRAFT, TLS1_3_VERSION_DRAFT_TXT},

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 11, 2018

Contributor

I could go either way on this as it's only diagnostic stuff. But then you get into "why not 28?" and "why 26" discussions. Cleanest to just remove it all.

@mattcaswell mattcaswell changed the title WIP: Updates for RFC8446 (the final TLSv1.3 version) Updates for RFC8446 (the final TLSv1.3 version) Aug 13, 2018

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

I've taken this out of WIP, responded to all comments above, and pushed a fixup commit.

@kaduk
kaduk approved these changes Aug 13, 2018
Copy link
Contributor

left a comment

Okay, I'm sold.
Ship it (unless Rich doesn't get his answer)!

@t-j-h
t-j-h approved these changes Aug 13, 2018
@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

Pushed!!! Woop!

levitte pushed a commit that referenced this pull request Aug 15, 2018
Update code for the final RFC version of TLSv1.3 (RFC8446)
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #6741)
levitte pushed a commit that referenced this pull request Aug 15, 2018
Turn on TLSv1.3 downgrade protection by default
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #6741)
levitte pushed a commit that referenced this pull request Aug 15, 2018
Fix a bug in test_sslversions
The TLSv1.4 tolerance test wasn't testing what we thought it was.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #6741)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.