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

TLSv1.3 0-RTT support #5450

Open
wants to merge 2 commits into
base: master
from

Conversation

@duke8253
Copy link
Contributor

duke8253 commented May 8, 2019

Enabling TLSv1.3 0-RTT support as mentioned in issue #4128.

@duke8253 duke8253 added the TLS label May 8, 2019
@duke8253 duke8253 added this to the 9.0.0 milestone May 8, 2019
@duke8253 duke8253 requested review from shinrich and masaori335 May 8, 2019
@duke8253 duke8253 self-assigned this May 8, 2019
iocore/net/SSLUtils.cc Outdated Show resolved Hide resolved
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 3 times, most recently from c0485ab to 37c6be1 May 8, 2019
@duke8253

This comment has been minimized.

Copy link
Contributor Author

duke8253 commented May 9, 2019

Is there a way to check if ci is building with openssl 1.1.1?

iocore/net/P_SSLUtils.h Outdated Show resolved Hide resolved
iocore/net/SSLUtils.cc Outdated Show resolved Hide resolved
@masaori335

This comment has been minimized.

Copy link
Contributor

masaori335 commented May 9, 2019

@duke8253 It's not checked directly, but there're checks of OpenSSL v1.1.1 API on configure.
e.g. Jenkins Ubuntu

checking whether to enable TLSv1.3 ciphersuites configuration is supported... yes

For some jobs, downloading docker image might be another option.

@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch from 37c6be1 to f2b536e May 9, 2019
iocore/net/P_SSLUtils.h Outdated Show resolved Hide resolved
iocore/net/SSLConfig.cc Outdated Show resolved Hide resolved
iocore/net/SSLConfig.cc Outdated Show resolved Hide resolved
iocore/net/SSLNetVConnection.cc Outdated Show resolved Hide resolved
iocore/net/SSLStats.cc Outdated Show resolved Hide resolved
iocore/net/SSLUtils.cc Outdated Show resolved Hide resolved
mgmt/RecordsConfig.cc Outdated Show resolved Hide resolved
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 2 times, most recently from 3a78d64 to 255c96e May 9, 2019
@duke8253 duke8253 requested a review from zwoop May 9, 2019
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch from 255c96e to d8d4684 May 16, 2019
@duke8253

This comment has been minimized.

Copy link
Contributor Author

duke8253 commented May 16, 2019

Updated the docs and comments a little to reflect the conversation here.

@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 2 times, most recently from ba30c2c to 246e261 May 17, 2019
@duke8253

This comment has been minimized.

Copy link
Contributor Author

duke8253 commented May 20, 2019

We will have at least two new separated PRs that's related:

  • anti-replay support
  • allow configs in ssl_server_name.yaml
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 2 times, most recently from 3067305 to d67908d May 23, 2019
@duke8253

This comment has been minimized.

Copy link
Contributor Author

duke8253 commented May 23, 2019

Changed to use MIOBuffer.

@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 4 times, most recently from cfa3f43 to d196d59 May 28, 2019
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 3 times, most recently from b38d367 to 9c2640d Oct 15, 2019
@zwoop

This comment has been minimized.

Copy link
Contributor

zwoop commented Oct 15, 2019

This completely breaks TLS < v1.3. I think there's some places where you either have to make sure that

server_max_early_data = 0

for TLS < v1.3 (such that those code paths don't trigger, effectively forcing it turned off for TLS < v1.3). Or, consistently add the checks like

if (SSL_version(ssl) >= TLS1_3_VERSION ...)

before doing anything that's new and specific to TLS v1.3 / early data. The compile time checks is not enough, since that only checks for availability, you have to be careful not to use this new functionality for TLS versions < 1.3.

Copy link
Contributor

zwoop left a comment

See failure comment.

iocore/net/SSLUtils.cc Outdated Show resolved Hide resolved
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch from 9c2640d to 80e8179 Oct 15, 2019
build/crypto.m4 Outdated Show resolved Hide resolved
#if TS_HAS_TLS_EARLY_DATA
SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
if (ssl_vc != nullptr && ssl_vc->read_from_early_data) {
ssl_vc->read_from_early_data = false;

This comment has been minimized.

Copy link
@maskit

maskit Oct 15, 2019

Member

I may misread the code, but what if the protocol is HTTP/2 and early data contains multiple requests?

https://tools.ietf.org/html/rfc8470#section-2

Conceptually, early data is concatenated with other application data to form a single stream. This can mean that requests are entirely contained within early data or that only part of a request is early. In a multiplexed protocol, like HTTP/2 [RFC7540] or HTTP/QUIC [HQ], multiple requests might be partially delivered in early data.

This comment has been minimized.

Copy link
@duke8253

duke8253 Oct 17, 2019

Author Contributor

Good point, I need to think about this a bit more.

This comment has been minimized.

Copy link
@zwoop

zwoop Oct 17, 2019

Contributor

I think this might be a showstopper. Either needs to be resolved, or we can not allow early data when H2 (or H3) is negotiated.

This comment has been minimized.

Copy link
@duke8253

duke8253 Oct 17, 2019

Author Contributor

disabling for h2/3 is the quick and dirty way of getting around this for now. if we want to have this in now I can make that happen.

Copy link
Contributor

zwoop left a comment

Still seeing failures with TLS < v1.3, similar same issue, where the early data code triggers even for a TLS v1.2 connection:

[Oct 15 23:38:48.334] [ET_NET 6] DEBUG: <SSLNetVConnection.cc:188 (make_ssl_connection)> (ssl_early_data) SSL_set_max_early_data: success
[Oct 15 23:38:48.334] [ET_NET 6] DEBUG: <SSLNetVConnection.cc:194 (make_ssl_connection)> (ssl_early_data) SSL_set_recv_max_early_data: success
[Oct 15 23:38:48.334] [ET_NET 6] DEBUG: <SSLNetVConnection.cc:200 (make_ssl_connection)> (ssl_early_data) Must disable anti-replay if external cache is used with 0-rtt.

This causes the read() to fail later, since data was consumed prematurely (I think).

@@ -1723,6 +1749,64 @@ SSLReadBuffer(SSL *ssl, void *buf, int64_t nbytes, int64_t &nread)
return SSL_ERROR_NONE;
}
ERR_clear_error();

#if TS_HAS_TLS_EARLY_DATA
if (SSL_version(ssl) >= TLS1_3_VERSION) {

This comment has been minimized.

Copy link
@maskit

maskit Oct 16, 2019

Member

Do we need this check here? netvc->early_data_reader should be nullptr if early data is not available. I'd add bool is_early_data_available() to SSLNetVC. IMO, the function should be defined in EarlyDataSupport class like ALPNSupport class because QUICNetVC should support it too, but it might be too much at the moment.

This comment has been minimized.

Copy link
@duke8253

duke8253 Oct 17, 2019

Author Contributor

We actually need this check here, the nullptr check isn't sufficient to avoid this situation:
enabled early data in config, but the actual tls version used is < 1.3

This comment has been minimized.

Copy link
@maskit

maskit Oct 17, 2019

Member

It didn't before the latest push because the reader was allocated inside the check for TLS version :)

I still think we can remove this check. For example, by resetting nullptr after checking netvc->early_data_reader->read_avail() == 0 in SSLAccept.

I think what we really need to check here is not TLS version but whether early data is available.

This comment has been minimized.

Copy link
@duke8253

duke8253 Oct 17, 2019

Author Contributor

yeah, but it was breaking before the last patch. :D
I'll think about that.

iocore/net/SSLConfig.cc Outdated Show resolved Hide resolved
iocore/net/SSLUtils.cc Outdated Show resolved Hide resolved
proxy/http/HttpTransact.cc Outdated Show resolved Hide resolved
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 4 times, most recently from 38013f8 to b959a5e Oct 17, 2019
@zwoop

This comment has been minimized.

Copy link
Contributor

zwoop commented Oct 17, 2019

@masaori335

this feels like it should be a directive in sni.yaml

@zwoop we don't use sni.yaml for anything. Do you mean that it should be optionally configurable there?

Yes.

TLSv1.3 0-RTT test

TLSv1.3 0-RTT anti-replay
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch from d48a907 to b3b828e Oct 24, 2019
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch 3 times, most recently from e610a22 to 2c7da53 Oct 25, 2019
@duke8253 duke8253 force-pushed the duke8253:master-0rtt branch from 2c7da53 to 50cd84f Oct 25, 2019
@zwoop zwoop added the OnDocs label Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
9 participants
You can’t perform that action at this time.