kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Buffer up QUIC data within a level internally.

Browse Source 
Avoid forcing the QUIC implementation to buffer this when we already have code
to do it. This also avoids QUIC implementations relying on this hook being
called for each individual message.

Change-Id: If2d70f045a25da1aa2b10fdae262cae331da06b1
Reviewed-on: https://boringssl-review.googlesource.com/c/32785
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This commit is contained in:
David Benjamin 2018-10-30 19:45:22 -05:00 committed by CQ bot account: commit-bot@chromium.org
parent c8e0f90f83
commit cc9d935256
3 changed files with 79 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
include/openssl/ssl.h
View File

@ -3098,12 +3098,15 @@ struct ssl_quic_method_st {
int (*set_encryption_secrets)(SSL *ssl, enum ssl_encryption_level_t level, int (*set_encryption_secrets)(SSL *ssl, enum ssl_encryption_level_t level,
const uint8_t *read_secret, const uint8_t *read_secret,
const uint8_t *write_secret, size_t secret_len); const uint8_t *write_secret, size_t secret_len);
// add_message adds a message to the current flight at the given encryption // add_handshake_data adds handshake data to the current flight at the given
// level. A single handshake flight may include multiple encryption levels. // encryption level. It returns one on success and zero on error.
// Callers can defer writing data to the network until |flush_flight| for //
// optimal packing. It returns one on success and zero on error. // BoringSSL will pack data from a single encryption level together, but a
int (*add_message)(SSL *ssl, enum ssl_encryption_level_t level, // single handshake flight may include multiple encryption levels. Callers
const uint8_t *data, size_t len); // should defer writing data to the network until |flush_flight| to better
// pack QUIC packets into transport datagrams.
int (*add_handshake_data)(SSL *ssl, enum ssl_encryption_level_t level,
const uint8_t *data, size_t len);
// flush_flight is called when the current flight is complete and should be // flush_flight is called when the current flight is complete and should be
// written to the transport. Note a flight may contain data at several // written to the transport. Note a flight may contain data at several
// encryption levels. It returns one on success and zero on error. // encryption levels. It returns one on success and zero on error.

109
ssl/s3_both.cc
View File

@ -184,56 +184,49 @@ bool ssl3_finish_message(SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg) {
} }
bool ssl3_add_message(SSL *ssl, Array<uint8_t> msg) { bool ssl3_add_message(SSL *ssl, Array<uint8_t> msg) {
if (ssl->ctx->quic_method) { // Pack handshake data into the minimal number of records. This avoids
if (!ssl->ctx->quic_method->add_message(ssl, ssl->s3->write_level, // unnecessary encryption overhead, notably in TLS 1.3 where we send several
msg.data(), msg.size())) { // encrypted messages in a row. For now, we do not do this for the null
OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR); // cipher. The benefit is smaller and there is a risk of breaking buggy
return false; // implementations. Additionally, we tie this to draft-28 as a sanity check,
// on the off chance middleboxes have fixated on sizes.
//
// TODO(davidben): See if we can do this uniformly.
Span<const uint8_t> rest = msg;
if (ssl->ctx->quic_method == nullptr &&
(ssl->s3->aead_write_ctx->is_null_cipher() ||
ssl->version == TLS1_3_DRAFT23_VERSION)) {
while (!rest.empty()) {
Span<const uint8_t> chunk = rest.subspan(0, ssl->max_send_fragment);
rest = rest.subspan(chunk.size());
if (!add_record_to_flight(ssl, SSL3_RT_HANDSHAKE, chunk)) {
return false;
}
} }
} else { } else {
// Pack handshake data into the minimal number of records. This avoids while (!rest.empty()) {
// unnecessary encryption overhead, notably in TLS 1.3 where we send several // Flush if |pending_hs_data| is full.
// encrypted messages in a row. For now, we do not do this for the null if (ssl->s3->pending_hs_data &&
// cipher. The benefit is smaller and there is a risk of breaking buggy ssl->s3->pending_hs_data->length >= ssl->max_send_fragment &&
// implementations. Additionally, we tie this to draft-28 as a sanity check, !tls_flush_pending_hs_data(ssl)) {
// on the off chance middleboxes have fixated on sizes. return false;
//
// TODO(davidben): See if we can do this uniformly.
Span<const uint8_t> rest = msg;
if (ssl->s3->aead_write_ctx->is_null_cipher() ||
ssl->version == TLS1_3_DRAFT23_VERSION) {
while (!rest.empty()) {
Span<const uint8_t> chunk = rest.subspan(0, ssl->max_send_fragment);
rest = rest.subspan(chunk.size());
if (!add_record_to_flight(ssl, SSL3_RT_HANDSHAKE, chunk)) {
return false;
}
} }
} else {
while (!rest.empty()) {
// Flush if |pending_hs_data| is full.
if (ssl->s3->pending_hs_data &&
ssl->s3->pending_hs_data->length >= ssl->max_send_fragment &&
!tls_flush_pending_hs_data(ssl)) {
return false;
}
size_t pending_len = size_t pending_len =
ssl->s3->pending_hs_data ? ssl->s3->pending_hs_data->length : 0; ssl->s3->pending_hs_data ? ssl->s3->pending_hs_data->length : 0;
Span<const uint8_t> chunk = Span<const uint8_t> chunk =
rest.subspan(0, ssl->max_send_fragment - pending_len); rest.subspan(0, ssl->max_send_fragment - pending_len);
assert(!chunk.empty()); assert(!chunk.empty());
rest = rest.subspan(chunk.size()); rest = rest.subspan(chunk.size());
if (!ssl->s3->pending_hs_data) { if (!ssl->s3->pending_hs_data) {
ssl->s3->pending_hs_data.reset(BUF_MEM_new()); ssl->s3->pending_hs_data.reset(BUF_MEM_new());
} }
if (!ssl->s3->pending_hs_data || if (!ssl->s3->pending_hs_data ||
!BUF_MEM_append(ssl->s3->pending_hs_data.get(), chunk.data(), !BUF_MEM_append(ssl->s3->pending_hs_data.get(), chunk.data(),
chunk.size())) { chunk.size())) {
return false; return false;
}
} }
} }
} }
@ -249,16 +242,24 @@ bool ssl3_add_message(SSL *ssl, Array<uint8_t> msg) {
} }
bool tls_flush_pending_hs_data(SSL *ssl) { bool tls_flush_pending_hs_data(SSL *ssl) {
if (!ssl->s3->pending_hs_data || ssl->s3->pending_hs_data->length == 0 || if (!ssl->s3->pending_hs_data || ssl->s3->pending_hs_data->length == 0) {
ssl->ctx->quic_method) {
return true; return true;
} }
UniquePtr<BUF_MEM> pending_hs_data = std::move(ssl->s3->pending_hs_data); UniquePtr<BUF_MEM> pending_hs_data = std::move(ssl->s3->pending_hs_data);
return add_record_to_flight( auto data =
ssl, SSL3_RT_HANDSHAKE,
MakeConstSpan(reinterpret_cast<const uint8_t *>(pending_hs_data->data), MakeConstSpan(reinterpret_cast<const uint8_t *>(pending_hs_data->data),
pending_hs_data->length)); pending_hs_data->length);
if (ssl->ctx->quic_method) {
if (!ssl->ctx->quic_method->add_handshake_data(ssl, ssl->s3->write_level,
data.data(), data.size())) {
OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR);
return false;
}
return true;
}
return add_record_to_flight(ssl, SSL3_RT_HANDSHAKE, data);
} }
bool ssl3_add_change_cipher_spec(SSL *ssl) { bool ssl3_add_change_cipher_spec(SSL *ssl) {
@ -280,6 +281,10 @@ bool ssl3_add_change_cipher_spec(SSL *ssl) {
} }
int ssl3_flush_flight(SSL *ssl) { int ssl3_flush_flight(SSL *ssl) {
if (!tls_flush_pending_hs_data(ssl)) {
return -1;
}
if (ssl->ctx->quic_method) { if (ssl->ctx->quic_method) {
if (ssl->s3->write_shutdown != ssl_shutdown_none) { if (ssl->s3->write_shutdown != ssl_shutdown_none) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN); OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN);
@ -292,10 +297,6 @@ int ssl3_flush_flight(SSL *ssl) {
} }
} }
if (!tls_flush_pending_hs_data(ssl)) {
return -1;
}
if (ssl->s3->pending_flight == nullptr) { if (ssl->s3->pending_flight == nullptr) {
return 1; return 1;
} }

29
ssl/ssl_test.cc
View File

@ -4650,8 +4650,9 @@ class QUICMethodTest : public testing::Test {
write_key, key_len); write_key, key_len);
} }
static int AddMessageCallback(SSL *ssl, enum ssl_encryption_level_t level, static int AddHandshakeDataCallback(SSL *ssl,
const uint8_t *data, size_t len) { enum ssl_encryption_level_t level,
const uint8_t *data, size_t len) {
EXPECT_EQ(level, SSL_quic_write_level(ssl)); EXPECT_EQ(level, SSL_quic_write_level(ssl));
return TransportFromSSL(ssl)->WriteHandshakeData(level, return TransportFromSSL(ssl)->WriteHandshakeData(level,
MakeConstSpan(data, len)); MakeConstSpan(data, len));
@ -4681,7 +4682,7 @@ UnownedSSLExData<MockQUICTransport> QUICMethodTest::ex_data_;
TEST_F(QUICMethodTest, Basic) { TEST_F(QUICMethodTest, Basic) {
const SSL_QUIC_METHOD quic_method = { const SSL_QUIC_METHOD quic_method = {
SetEncryptionSecretsCallback, SetEncryptionSecretsCallback,
AddMessageCallback, AddHandshakeDataCallback,
FlushFlightCallback, FlushFlightCallback,
SendAlertCallback, SendAlertCallback,
}; };
@ -4732,7 +4733,7 @@ TEST_F(QUICMethodTest, Basic) {
TEST_F(QUICMethodTest, Async) { TEST_F(QUICMethodTest, Async) {
const SSL_QUIC_METHOD quic_method = { const SSL_QUIC_METHOD quic_method = {
SetEncryptionSecretsCallback, SetEncryptionSecretsCallback,
AddMessageCallback, AddHandshakeDataCallback,
FlushFlightCallback, FlushFlightCallback,
SendAlertCallback, SendAlertCallback,
}; };
@ -4793,8 +4794,8 @@ TEST_F(QUICMethodTest, Buffered) {
}; };
static UnownedSSLExData<BufferedFlight> buffered_flights; static UnownedSSLExData<BufferedFlight> buffered_flights;
auto add_message = [](SSL *ssl, enum ssl_encryption_level_t level, auto add_handshake_data = [](SSL *ssl, enum ssl_encryption_level_t level,
const uint8_t *data, size_t len) -> int { const uint8_t *data, size_t len) -> int {
BufferedFlight *flight = buffered_flights.Get(ssl); BufferedFlight *flight = buffered_flights.Get(ssl);
flight->data[level].insert(flight->data[level].end(), data, data + len); flight->data[level].insert(flight->data[level].end(), data, data + len);
return 1; return 1;
@ -4817,7 +4818,7 @@ TEST_F(QUICMethodTest, Buffered) {
const SSL_QUIC_METHOD quic_method = { const SSL_QUIC_METHOD quic_method = {
SetEncryptionSecretsCallback, SetEncryptionSecretsCallback,
add_message, add_handshake_data,
flush_flight, flush_flight,
SendAlertCallback, SendAlertCallback,
}; };
@ -4862,8 +4863,8 @@ TEST_F(QUICMethodTest, Buffered) {
// EncryptedExtensions in a single chunk, BoringSSL notices and rejects this on // EncryptedExtensions in a single chunk, BoringSSL notices and rejects this on
// key change. // key change.
TEST_F(QUICMethodTest, ExcessProvidedData) { TEST_F(QUICMethodTest, ExcessProvidedData) {
auto add_message = [](SSL *ssl, enum ssl_encryption_level_t level, auto add_handshake_data = [](SSL *ssl, enum ssl_encryption_level_t level,
const uint8_t *data, size_t len) -> int { const uint8_t *data, size_t len) -> int {
// Switch everything to the initial level. // Switch everything to the initial level.
return TransportFromSSL(ssl)->WriteHandshakeData(ssl_encryption_initial, return TransportFromSSL(ssl)->WriteHandshakeData(ssl_encryption_initial,
MakeConstSpan(data, len)); MakeConstSpan(data, len));
@ -4871,7 +4872,7 @@ TEST_F(QUICMethodTest, ExcessProvidedData) {
const SSL_QUIC_METHOD quic_method = { const SSL_QUIC_METHOD quic_method = {
SetEncryptionSecretsCallback, SetEncryptionSecretsCallback,
add_message, add_handshake_data,
FlushFlightCallback, FlushFlightCallback,
SendAlertCallback, SendAlertCallback,
}; };
@ -4891,8 +4892,8 @@ TEST_F(QUICMethodTest, ExcessProvidedData) {
// encryption. // encryption.
ASSERT_EQ(ssl_encryption_initial, SSL_quic_read_level(client_.get())); ASSERT_EQ(ssl_encryption_initial, SSL_quic_read_level(client_.get()));
// |add_message| incorrectly wrote everything at the initial level, so this // |add_handshake_data| incorrectly wrote everything at the initial level, so
// queues up ServerHello through Finished in one chunk. // this queues up ServerHello through Finished in one chunk.
ASSERT_TRUE(ProvideHandshakeData(client_.get())); ASSERT_TRUE(ProvideHandshakeData(client_.get()));
// The client reads ServerHello successfully, but then rejects the buffered // The client reads ServerHello successfully, but then rejects the buffered
@ -4917,7 +4918,7 @@ TEST_F(QUICMethodTest, ExcessProvidedData) {
TEST_F(QUICMethodTest, ProvideWrongLevel) { TEST_F(QUICMethodTest, ProvideWrongLevel) {
const SSL_QUIC_METHOD quic_method = { const SSL_QUIC_METHOD quic_method = {
SetEncryptionSecretsCallback, SetEncryptionSecretsCallback,
AddMessageCallback, AddHandshakeDataCallback,
FlushFlightCallback, FlushFlightCallback,
SendAlertCallback, SendAlertCallback,
}; };
@ -4962,7 +4963,7 @@ TEST_F(QUICMethodTest, ProvideWrongLevel) {
TEST_F(QUICMethodTest, TooMuchData) { TEST_F(QUICMethodTest, TooMuchData) {
const SSL_QUIC_METHOD quic_method = { const SSL_QUIC_METHOD quic_method = {
SetEncryptionSecretsCallback, SetEncryptionSecretsCallback,
AddMessageCallback, AddHandshakeDataCallback,
FlushFlightCallback, FlushFlightCallback,
SendAlertCallback, SendAlertCallback,
}; };