Skip to content

GitLab

T
tangled
Commit 4e0215d0 authored by David Benjamin's avatar David Benjamin Committed by Commit Bot
Browse files
Options

Remove SSLConfig::false_start_enabled 

It is now only set in tests, where it is a largely a no-op because False
Start since acquired an ALPN requirement. When False Start is triggered,
we do need to worry about post-Connect() handshake data and sessions
being established late, but TLS 1.3 has the same problem unavoidably.

Abstract the ticket-forcing logic behind a new helper so we can easily
add it as needed in the future.

(Firefox actually no longer requires ALPN for False Start. This may be
worth considering, in which case we may need to revisit some more of
these tests. Additionally, SpawnedTestServer doesn't support TLS 1.3 and
most of these tests haven't moved to EmbeddedTestServer, which may also
require fixing up tests.)

Bug: none
Change-Id: I444ee12b66aed88898d1492ee0df29817c155348
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1708675
Auto-Submit: David Benjamin <davidben@chromium.org>
Reviewed-by: default avatarSteven Valdez <svaldez@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686506}
parent cb7925c3
Hide whitespace changes
Inline Side-by-side
Showing with 43 additions and 73 deletions
net/socket/ssl_client_socket_impl.cc
View file @ 4e0215d0
......@@ -865,8 +865,7 @@ int SSLClientSocketImpl::Init() {
mode.ConfigureFlag(SSL_MODE_RELEASE_BUFFERS, true);
mode.ConfigureFlag(SSL_MODE_CBC_RECORD_SPLITTING, true);
mode.ConfigureFlag(SSL_MODE_ENABLE_FALSE_START,
ssl_config_.false_start_enabled);
mode.ConfigureFlag(SSL_MODE_ENABLE_FALSE_START, true);
SSL_set_mode(ssl_.get(), mode.set_mask);
SSL_clear_mode(ssl_.get(), mode.clear_mask);
......
net/socket/ssl_client_socket_unittest.cc
View file @ 4e0215d0
......@@ -1267,6 +1267,32 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
}
};
// Sends an HTTP request on the socket and reads the response. This may be used
// to ensure some data has been consumed from the server.
int MakeHTTPRequest(StreamSocket* socket) {
base::StringPiece request = "GET / HTTP/1.0\r\n\r\n";
TestCompletionCallback callback;
while (!request.empty()) {
auto request_buffer =
base::MakeRefCounted<StringIOBuffer>(request.as_string());
int rv = callback.GetResult(
socket->Write(request_buffer.get(), request_buffer->size(),
callback.callback(), TRAFFIC_ANNOTATION_FOR_TESTS));
if (rv < 0) {
return rv;
}
request = request.substr(rv);
}
auto response_buffer = base::MakeRefCounted<IOBuffer>(1024);
int rv = callback.GetResult(
socket->Read(response_buffer.get(), 1024, callback.callback()));
if (rv < 0) {
return rv;
}
return OK;
}
// Provides a response to the 0RTT request indicating whether it was received
// as early data.
class ZeroRTTResponse : public test_server::HttpResponse {
......@@ -1384,13 +1410,7 @@ class SSLClientSocketZeroRTTTest : public SSLClientSocketTest {
// Use the socket for an HTTP request to ensure we've processed the
// post-handshake TLS 1.3 ticket.
constexpr base::StringPiece kRequest = "GET / HTTP/1.0\r\n\r\n";
if (kRequest.size() != WriteAndWait(kRequest))
return false;
scoped_refptr<IOBuffer> buf = base::MakeRefCounted<IOBuffer>(4096);
if (ReadAndWait(buf.get(), 4096) <= 0)
return false;
EXPECT_THAT(MakeHTTPRequest(ssl_socket_.get()), IsOk());
SSLInfo ssl_info;
EXPECT_TRUE(GetSSLInfo(&ssl_info));
......@@ -1745,14 +1765,10 @@ TEST_F(SSLClientSocketTest, Connect_WithSynchronousError) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to avoid handshake non-determinism.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
SynchronousErrorStreamSocket* raw_transport = transport.get();
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
raw_transport->SetNextWriteError(ERR_CONNECTION_RESET);
......@@ -1776,14 +1792,10 @@ TEST_P(SSLClientSocketReadTest, Read_WithSynchronousError) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to avoid handshake non-determinism.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
SynchronousErrorStreamSocket* raw_transport = transport.get();
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
......@@ -1834,13 +1846,9 @@ TEST_F(SSLClientSocketTest, Write_WithSynchronousError) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to avoid handshake non-determinism.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
......@@ -1903,13 +1911,9 @@ TEST_F(SSLClientSocketTest, Write_WithSynchronousErrorNoRead) {
int rv = callback.GetResult(counting_socket->Connect(callback.callback()));
ASSERT_THAT(rv, IsOk());
// Disable TLS False Start to avoid handshake non-determinism.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(counting_socket), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
rv = callback.GetResult(sock->Connect(callback.callback()));
ASSERT_THAT(rv, IsOk());
......@@ -2008,13 +2012,9 @@ TEST_P(SSLClientSocketReadTest, Read_DeleteWhilePendingFullDuplex) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to avoid handshake non-determinism.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
std::unique_ptr<SSLClientSocket> sock = CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config);
SSLConfig());
rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
......@@ -2094,13 +2094,9 @@ TEST_P(SSLClientSocketReadTest, Read_WithWriteError) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to avoid handshake non-determinism.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
......@@ -2204,14 +2200,10 @@ TEST_P(SSLClientSocketReadTest, Read_WithZeroReturn) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to ensure the handshake has completed.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
SynchronousErrorStreamSocket* raw_transport = transport.get();
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
......@@ -2241,13 +2233,9 @@ TEST_P(SSLClientSocketReadTest, Read_WithAsyncZeroReturn) {
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
// Disable TLS False Start to ensure the handshake has completed.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;
std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
std::move(transport), spawned_test_server()->host_port_pair(),
ssl_config));
SSLConfig()));
rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_THAT(rv, IsOk());
......@@ -3223,8 +3211,6 @@ TEST_F(SSLClientSocketTest, SessionResumptionAlpn) {
// First, perform a full handshake.
SSLConfig ssl_config;
// Disable TLS False Start to ensure the handshake has completed.
ssl_config.false_start_enabled = false;
ssl_config.alpn_protos.push_back(kProtoHTTP2);
int rv;
ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
......@@ -3234,6 +3220,10 @@ TEST_F(SSLClientSocketTest, SessionResumptionAlpn) {
EXPECT_EQ(SSLInfo::HANDSHAKE_FULL, ssl_info.handshake_type);
EXPECT_EQ(kProtoHTTP2, sock_->GetNegotiatedProtocol());
// TLS 1.2 with False Start and TLS 1.3 cause the ticket to arrive later, so
// use the socket to ensure the session ticket has been picked up.
EXPECT_THAT(MakeHTTPRequest(sock_.get()), IsOk());
// The next connection should resume; ALPN should be renegotiated.
ssl_config.alpn_protos.clear();
ssl_config.alpn_protos.push_back(kProtoHTTP11);
......@@ -5766,23 +5756,9 @@ TEST_P(SSLHandshakeDetailsTest, Metrics) {
histograms.ExpectUniqueSample("Net.SSLHandshakeDetails",
GetParam().expected_initial, 1);
// Use the socket to ensure the session ticket has been picked up.
base::StringPiece request = "GET / HTTP/1.0\r\n\r\n";
TestCompletionCallback callback;
while (!request.empty()) {
auto request_buffer =
base::MakeRefCounted<StringIOBuffer>(request.as_string());
rv = callback.GetResult(
sock_->Write(request_buffer.get(), request_buffer->size(),
callback.callback(), TRAFFIC_ANNOTATION_FOR_TESTS));
ASSERT_GT(rv, 0);
request = request.substr(rv);
}
auto response_buffer = base::MakeRefCounted<IOBuffer>(1024);
rv = callback.GetResult(
sock_->Read(response_buffer.get(), 1024, callback.callback()));
ASSERT_GT(rv, 0);
// TLS 1.2 with False Start and TLS 1.3 cause the ticket to arrive later, so
// use the socket to ensure the session ticket has been picked up.
EXPECT_THAT(MakeHTTPRequest(sock_.get()), IsOk());
}
// Make a resumption connection.
......
net/socket/ssl_server_socket_unittest.cc
View file @ 4e0215d0
......@@ -380,8 +380,6 @@ class SSLServerSocketTest : public PlatformTest,
ASSERT_TRUE(key);
server_ssl_private_key_ = WrapOpenSSLPrivateKey(bssl::UpRef(key->key()));
client_ssl_config_.false_start_enabled = false;
// Certificate provided by the host doesn't need authority.
client_ssl_config_.allowed_bad_certs.emplace_back(
server_cert_, CERT_STATUS_AUTHORITY_INVALID);
......
net/ssl/ssl_config.cc
View file @ 4e0215d0
......@@ -23,7 +23,6 @@ SSLConfig::CertAndStatus::~CertAndStatus() = default;
SSLConfig::SSLConfig()
: early_data_enabled(false),
false_start_enabled(true),
require_ecdhe(false),
ignore_certificate_errors(false),
disable_cert_verification_network_fetches(false),
......
net/ssl/ssl_config.h
View file @ 4e0215d0
......@@ -75,8 +75,6 @@ struct NET_EXPORT SSLConfig {
// If unsure, do not enable this option.
bool early_data_enabled;
bool false_start_enabled; // True if we'll use TLS False Start.
// If true, causes only ECDHE cipher suites to be enabled.
bool require_ecdhe;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment