Revise QUIC encryption secret APIs.
The original API separated when keys were exported to QUIC from when
they were "active". This means the obligations on QUIC are unclear. For
instance we added SSL_quic_read_level and SSL_quic_write_level to
capture when keys were active, yet QUICHE never used them anyway. It
would be better to defer releasing keys to when they are needed.
In particular, we should align our API with the guidelines in
https://github.com/quicwg/base-drafts/issues/3173.
This means we need separate read and write callbacks, which
unfortunately means the invariants around ACKs must now be covered in
prose.
We'll likely remove SSL_quic_read_level and SSL_quic_write_level in a
later CL as QUIC has no need to know this anymore. (It should simply
feed handshake data to BoringSSL as it sees it and, if we reject it,
report a suitably error.) The notion of a "current" encryption level is
a little odd given the interaction between 0-RTT and
ServerHello..Finished ACKs.
Update-Note: This is an incompatible change to SSL_QUIC_METHOD.
BORINGSSL_API_VERSION can be used to distinguish the two revisions.
Bug: 303
Change-Id: I6c9dca1ae156d26a80c366a4ba969dcb04e65349
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40127
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/include/openssl/base.h b/include/openssl/base.h
index e347c09..8d73f77 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -184,7 +184,7 @@
// A consumer may use this symbol in the preprocessor to temporarily build
// against multiple revisions of BoringSSL at the same time. It is not
// recommended to do so for longer than is necessary.
-#define BORINGSSL_API_VERSION 9
+#define BORINGSSL_API_VERSION 10
#if defined(BORINGSSL_SHARED_LIBRARY)
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 1fedaf8..234b088 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3133,10 +3133,11 @@
// When configured for QUIC, |SSL_do_handshake| will drive the handshake as
// before, but it will not use the configured |BIO|. It will call functions on
// |SSL_QUIC_METHOD| to configure secrets and send data. If data is needed from
-// the peer, it will return |SSL_ERROR_WANT_READ|. When received, the caller
-// should call |SSL_provide_quic_data| and then |SSL_do_handshake| to continue
-// the handshake. After the handshake is complete, the caller should call
-// |SSL_provide_quic_data| for any post-handshake data, followed by
+// the peer, it will return |SSL_ERROR_WANT_READ|. As the caller receives data
+// it can decrypt, it calls |SSL_provide_quic_data|. Subsequent
+// |SSL_do_handshake| calls will then consume that data and progress the
+// handshake. After the handshake is complete, the caller should continue to
+// call |SSL_provide_quic_data| for any post-handshake data, followed by
// |SSL_process_quic_post_handshake| to process it. It is an error to call
// |SSL_read| and |SSL_write| in QUIC.
//
@@ -3147,13 +3148,6 @@
// confirm the handshake. As a client, |SSL_ERROR_EARLY_DATA_REJECTED| and
// |SSL_reset_early_data_reject| behave as usual.
//
-// Note that secrets for an encryption level may be available to QUIC before the
-// level is active in TLS. Callers should use |SSL_quic_read_level| to determine
-// the active read level for |SSL_provide_quic_data|. |SSL_do_handshake| will
-// pass the active write level to |SSL_QUIC_METHOD| when writing data. Callers
-// can use |SSL_quic_write_level| to query the active write level when
-// generating their own errors.
-//
// See https://tools.ietf.org/html/draft-ietf-quic-tls-15#section-4.1 for more
// details.
//
@@ -3176,31 +3170,51 @@
// ssl_quic_method_st (aka |SSL_QUIC_METHOD|) describes custom QUIC hooks.
struct ssl_quic_method_st {
- // set_encryption_secrets configures the read and write secrets for the given
- // encryption level. This function will always be called before an encryption
- // level other than |ssl_encryption_initial| is used. Note, however, that
- // secrets for a level may be configured before TLS is ready to send or accept
- // data at that level.
+ // set_read_secret configures the read secret and cipher suite for the given
+ // encryption level. It returns one on success and zero to terminate the
+ // handshake with an error. It will be called at most once per encryption
+ // level.
//
- // When reading packets at a given level, the QUIC implementation must send
- // ACKs at the same level, so this function provides read and write secrets
- // together. The exception is |ssl_encryption_early_data|, where secrets are
- // only available in the client to server direction. The other secret will be
- // NULL. The server acknowledges such data at |ssl_encryption_application|,
- // which will be configured in the same |SSL_do_handshake| call.
+ // BoringSSL will not release read keys before QUIC may use them. Once a level
+ // has been initialized, QUIC may begin processing data from it. Handshake
+ // data should be passed to |SSL_provide_quic_data| and application data (if
+ // |level| is |ssl_encryption_early_data| or |ssl_encryption_application|) may
+ // be processed according to the rules of the QUIC protocol.
//
- // This function should use |SSL_get_current_cipher| to determine the TLS
- // cipher suite.
+ // QUIC ACKs packets at the same encryption level they were received at,
+ // except that client |ssl_encryption_early_data| (0-RTT) packets trigger
+ // server |ssl_encryption_application| (1-RTT) ACKs. BoringSSL will always
+ // install ACK-writing keys with |set_write_secret| before the packet-reading
+ // keys with |set_read_secret|. This ensures the caller can always ACK any
+ // packet it decrypts. Note this means the server installs 1-RTT write keys
+ // before 0-RTT read keys.
//
- // TODO(davidben): The advice to use |SSL_get_current_cipher| does not work
- // for 0-RTT rejects on the client. As part of the fix to
- // https://crbug.com/boringssl/303, we will add an explicit cipher suite
- // parameter.
+ // The converse is not true. An encryption level may be configured with write
+ // secrets a roundtrip before the corresponding secrets for reading ACKs is
+ // available.
+ int (*set_read_secret)(SSL *ssl, enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher, const uint8_t *secret,
+ size_t secret_len);
+ // set_write_secret behaves like |set_read_secret| but configures the write
+ // secret and cipher suite for the given encryption level. It will be called
+ // at most once per encryption level.
//
- // It returns one on success and zero on error.
- int (*set_encryption_secrets)(SSL *ssl, enum ssl_encryption_level_t level,
- const uint8_t *read_secret,
- const uint8_t *write_secret, size_t secret_len);
+ // BoringSSL will not release write keys before QUIC may use them. If |level|
+ // is |ssl_encryption_early_data| or |ssl_encryption_application|, QUIC may
+ // begin sending application data at |level|. However, note that BoringSSL
+ // configures server |ssl_encryption_application| write keys before the client
+ // Finished. This allows QUIC to send half-RTT data, but the handshake is not
+ // confirmed at this point and, if requesting client certificates, the client
+ // is not yet authenticated.
+ //
+ // See |set_read_secret| for additional invariants between packets and their
+ // ACKs.
+ //
+ // Note that, on 0-RTT reject, the |ssl_encryption_early_data| write secret
+ // may use a different cipher suite from the other keys.
+ int (*set_write_secret)(SSL *ssl, enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher, const uint8_t *secret,
+ size_t secret_len);
// add_handshake_data adds handshake data to the current flight at the given
// encryption level. It returns one on success and zero on error.
//
@@ -3208,6 +3222,9 @@
// single handshake flight may include multiple encryption levels. Callers
// should defer writing data to the network until |flush_flight| to better
// pack QUIC packets into transport datagrams.
+ //
+ // If |level| is not |ssl_encryption_initial|, this function will not be
+ // called before |level| is initialized with |set_write_secret|.
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
@@ -3216,6 +3233,9 @@
int (*flush_flight)(SSL *ssl);
// send_alert sends a fatal alert at the specified encryption level. It
// returns one on success and zero on error.
+ //
+ // If |level| is not |ssl_encryption_initial|, this function will not be
+ // called before |level| is initialized with |set_write_secret|.
int (*send_alert)(SSL *ssl, enum ssl_encryption_level_t level, uint8_t alert);
};
@@ -3228,15 +3248,22 @@
const SSL *ssl, enum ssl_encryption_level_t level);
// SSL_quic_read_level returns the current read encryption level.
+//
+// TODO(davidben): Is it still necessary to expose this function to callers?
+// QUICHE does not use it.
OPENSSL_EXPORT enum ssl_encryption_level_t SSL_quic_read_level(const SSL *ssl);
// SSL_quic_write_level returns the current write encryption level.
+//
+// TODO(davidben): Is it still necessary to expose this function to callers?
+// QUICHE does not use it.
OPENSSL_EXPORT enum ssl_encryption_level_t SSL_quic_write_level(const SSL *ssl);
// SSL_provide_quic_data provides data from QUIC at a particular encryption
-// level |level|. It is an error to call this function outside of the handshake
-// or with an encryption level other than the current read level. It returns one
-// on success and zero on error.
+// level |level|. It returns one on success and zero on error. Note this
+// function will return zero if the handshake is not expecting data from |level|
+// at this time. The QUIC implementation should then close the connection with
+// an error.
OPENSSL_EXPORT int SSL_provide_quic_data(SSL *ssl,
enum ssl_encryption_level_t level,
const uint8_t *data, size_t len);
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index fa692c5..e64e456 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -461,11 +461,6 @@
!tls13_derive_early_secret(hs)) {
return ssl_hs_error;
}
- if (ssl->quic_method == nullptr &&
- !tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_seal,
- ssl->session.get(), hs->early_traffic_secret())) {
- return ssl_hs_error;
- }
// Stash the early data session, so connection properties may be queried out
// of it.
@@ -496,7 +491,9 @@
// Defer releasing the 0-RTT key to after certificate reverification, so the
// QUIC implementation does not accidentally write data too early.
- if (!tls13_set_early_secret_for_quic(hs)) {
+ if (!tls13_set_traffic_key(hs->ssl, ssl_encryption_early_data, evp_aead_seal,
+ hs->early_session.get(),
+ hs->early_traffic_secret())) {
return ssl_hs_error;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 78c56b6..91036ae 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1364,17 +1364,9 @@
Span<const uint8_t> traffic_secret);
// tls13_derive_early_secret derives the early traffic secret. It returns true
-// on success and false on error. Unlike with other traffic secrets, this
-// function does not pass the keys to QUIC. Call
-// |tls13_set_early_secret_for_quic| to do so. This is done to due to an
-// ordering complication around resolving HelloRetryRequest on the server.
+// on success and false on error.
bool tls13_derive_early_secret(SSL_HANDSHAKE *hs);
-// tls13_set_early_secret_for_quic passes the early traffic secrets, as
-// derived by |tls13_derive_early_secret|, to QUIC. It returns true on success
-// and false on error.
-bool tls13_set_early_secret_for_quic(SSL_HANDSHAKE *hs);
-
// tls13_derive_handshake_secrets derives the handshake traffic secret. It
// returns true on success and false on error.
bool tls13_derive_handshake_secrets(SSL_HANDSHAKE *hs);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 9104bc1..c838bf4 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -4806,6 +4806,20 @@
static_assert(ssl_encryption_application < kNumQUICLevels,
"kNumQUICLevels is wrong");
+const char *LevelToString(ssl_encryption_level_t level) {
+ switch (level) {
+ case ssl_encryption_initial:
+ return "initial";
+ case ssl_encryption_early_data:
+ return "early data";
+ case ssl_encryption_handshake:
+ return "handshake";
+ case ssl_encryption_application:
+ return "application";
+ }
+ return "<unknown>";
+}
+
class MockQUICTransport {
public:
enum class Role { kClient, kServer };
@@ -4828,61 +4842,68 @@
levels_[level].cipher == peer_->levels_[level].cipher;
}
- bool HasSecrets(ssl_encryption_level_t level) const {
- return !levels_[level].write_secret.empty() ||
- !levels_[level].read_secret.empty();
+ bool HasReadSecret(ssl_encryption_level_t level) const {
+ return !levels_[level].read_secret.empty();
}
- bool SetEncryptionSecrets(ssl_encryption_level_t level,
- const uint8_t *read_secret,
- const uint8_t *write_secret, size_t secret_len,
- const SSL_CIPHER *cipher) {
- if (HasSecrets(level)) {
- ADD_FAILURE() << "duplicate keys configured";
+ bool HasWriteSecret(ssl_encryption_level_t level) const {
+ return !levels_[level].write_secret.empty();
+ }
+
+ bool SetReadSecret(ssl_encryption_level_t level, const SSL_CIPHER *cipher,
+ Span<const uint8_t> secret) {
+ if (HasReadSecret(level)) {
+ ADD_FAILURE() << LevelToString(level) << " read secret configured twice";
+ return false;
+ }
+
+ if (role_ == Role::kClient && level == ssl_encryption_early_data) {
+ ADD_FAILURE() << "Unexpected early data read secret";
+ return false;
+ }
+
+ ssl_encryption_level_t ack_level =
+ level == ssl_encryption_early_data ? ssl_encryption_application : level;
+ if (!HasWriteSecret(ack_level)) {
+ ADD_FAILURE() << LevelToString(level)
+ << " read secret configured before ACK write secret";
return false;
}
if (cipher == nullptr) {
- ADD_FAILURE() << "current cipher unavailable";
+ ADD_FAILURE() << "Unexpected null cipher";
return false;
}
- bool expect_read_secret = true, expect_write_secret = true;
- if (level == ssl_encryption_early_data) {
- if (role_ == Role::kClient) {
- expect_read_secret = false;
- } else {
- expect_write_secret = false;
- if (!HasSecrets(ssl_encryption_application)) {
- ADD_FAILURE() << "early secrets installed without keys to ACK them";
- return false;
- }
- }
- }
-
- if (expect_read_secret) {
- if (read_secret == nullptr) {
- ADD_FAILURE() << "read secret was unexpectedly null";
- return false;
- }
- levels_[level].read_secret.assign(read_secret, read_secret + secret_len);
- } else if (read_secret != nullptr) {
- ADD_FAILURE() << "unexpected read secret";
+ if (level != ssl_encryption_early_data &&
+ SSL_CIPHER_get_id(cipher) != levels_[level].cipher) {
+ ADD_FAILURE() << "Cipher suite inconsistent";
return false;
}
- if (expect_write_secret) {
- if (write_secret == nullptr) {
- ADD_FAILURE() << "write secret was unexpectedly null";
- return false;
- }
- levels_[level].write_secret.assign(write_secret,
- write_secret + secret_len);
- } else if (write_secret != nullptr) {
- ADD_FAILURE() << "unexpected write secret";
+ levels_[level].read_secret.assign(secret.begin(), secret.end());
+ levels_[level].cipher = SSL_CIPHER_get_id(cipher);
+ return true;
+ }
+
+ bool SetWriteSecret(ssl_encryption_level_t level, const SSL_CIPHER *cipher,
+ Span<const uint8_t> secret) {
+ if (HasWriteSecret(level)) {
+ ADD_FAILURE() << LevelToString(level) << " write secret configured twice";
return false;
}
+ if (role_ == Role::kServer && level == ssl_encryption_early_data) {
+ ADD_FAILURE() << "Unexpected early data write secret";
+ return false;
+ }
+
+ if (cipher == nullptr) {
+ ADD_FAILURE() << "Unexpected null cipher";
+ return false;
+ }
+
+ levels_[level].write_secret.assign(secret.begin(), secret.end());
levels_[level].cipher = SSL_CIPHER_get_id(cipher);
return true;
}
@@ -4890,7 +4911,8 @@
bool WriteHandshakeData(ssl_encryption_level_t level,
Span<const uint8_t> data) {
if (levels_[level].write_secret.empty()) {
- ADD_FAILURE() << "data written before keys configured";
+ ADD_FAILURE() << LevelToString(level)
+ << " write secret not yet configured";
return false;
}
levels_[level].write_data.insert(levels_[level].write_data.end(),
@@ -4905,7 +4927,8 @@
}
if (levels_[level].write_secret.empty()) {
- ADD_FAILURE() << "alert sent before keys configured";
+ ADD_FAILURE() << LevelToString(level)
+ << " write secret not yet configured";
return false;
}
@@ -4979,8 +5002,11 @@
MockQUICTransport *server() { return &server_; }
bool SecretsMatch(ssl_encryption_level_t level) const {
- return client_.HasSecrets(level) && server_.HasSecrets(level) &&
- client_.PeerSecretsMatch(level);
+ // We only need to check |HasReadSecret| and |HasWriteSecret| on |client_|.
+ // |PeerSecretsMatch| checks that |server_| is analogously configured.
+ return client_.PeerSecretsMatch(level) &&
+ client_.HasWriteSecret(level) &&
+ (level == ssl_encryption_early_data || client_.HasReadSecret(level));
}
private:
@@ -5110,16 +5136,27 @@
EXPECT_EQ(SSL_do_handshake(server_.get()), 1);
}
- // The following functions may be configured on an |SSL_QUIC_METHOD| as
- // default implementations.
+ // Returns a default SSL_QUIC_METHOD. Individual methods may be overwritten by
+ // the test.
+ SSL_QUIC_METHOD DefaultQUICMethod() {
+ return SSL_QUIC_METHOD{
+ SetReadSecretCallback, SetWriteSecretCallback, AddHandshakeDataCallback,
+ FlushFlightCallback, SendAlertCallback,
+ };
+ }
- static int SetEncryptionSecretsCallback(SSL *ssl,
- ssl_encryption_level_t level,
- const uint8_t *read_key,
- const uint8_t *write_key,
- size_t key_len) {
- return TransportFromSSL(ssl)->SetEncryptionSecrets(
- level, read_key, write_key, key_len, SSL_get_current_cipher(ssl));
+ static int SetReadSecretCallback(SSL *ssl, ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher,
+ const uint8_t *secret, size_t secret_len) {
+ return TransportFromSSL(ssl)->SetReadSecret(
+ level, cipher, MakeConstSpan(secret, secret_len));
+ }
+
+ static int SetWriteSecretCallback(SSL *ssl, ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher,
+ const uint8_t *secret, size_t secret_len) {
+ return TransportFromSSL(ssl)->SetWriteSecret(
+ level, cipher, MakeConstSpan(secret, secret_len));
}
static int AddHandshakeDataCallback(SSL *ssl,
@@ -5152,12 +5189,7 @@
// Test a full handshake and resumption work.
TEST_F(QUICMethodTest, Basic) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
g_last_session = nullptr;
@@ -5193,12 +5225,7 @@
// Test that HelloRetryRequest in QUIC works.
TEST_F(QUICMethodTest, HelloRetryRequest) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
@@ -5217,13 +5244,38 @@
ExpectHandshakeSuccess();
}
+// Test that, even in a 1-RTT handshake, the server installs keys at the right
+// time. Half-RTT keys are available early, but 1-RTT read keys are deferred.
+TEST_F(QUICMethodTest, HalfRTTKeys) {
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+
+ ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
+ ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
+ ASSERT_TRUE(CreateClientAndServer());
+
+ // The client sends ClientHello.
+ ASSERT_EQ(SSL_do_handshake(client_.get()), -1);
+ ASSERT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(client_.get(), -1));
+
+ // The server reads ClientHello and sends ServerHello..Finished.
+ ASSERT_TRUE(ProvideHandshakeData(server_.get()));
+ ASSERT_EQ(SSL_do_handshake(server_.get()), -1);
+ ASSERT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1));
+
+ // At this point, the server has half-RTT write keys, but it cannot access
+ // 1-RTT read keys until client Finished.
+ EXPECT_TRUE(transport_->server()->HasWriteSecret(ssl_encryption_application));
+ EXPECT_FALSE(transport_->server()->HasReadSecret(ssl_encryption_application));
+
+ // Finish up the client and server handshakes.
+ ASSERT_TRUE(CompleteHandshakesForQUIC());
+
+ // Both sides can now exchange 1-RTT data.
+ ExpectHandshakeSuccess();
+}
+
TEST_F(QUICMethodTest, ZeroRTTAccept) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
SSL_CTX_set_early_data_enabled(client_ctx_.get(), 1);
@@ -5241,8 +5293,7 @@
ASSERT_EQ(SSL_do_handshake(client_.get()), 1);
EXPECT_TRUE(SSL_in_early_data(client_.get()));
// The transport should have keys for sending 0-RTT data.
- EXPECT_TRUE(
- transport_->client()->HasSecrets(ssl_encryption_early_data));
+ EXPECT_TRUE(transport_->client()->HasWriteSecret(ssl_encryption_early_data));
// The server will consume the ClientHello and also enter the early data
// state.
@@ -5250,9 +5301,10 @@
ASSERT_EQ(SSL_do_handshake(server_.get()), 1);
EXPECT_TRUE(SSL_in_early_data(server_.get()));
EXPECT_TRUE(transport_->SecretsMatch(ssl_encryption_early_data));
- // The transport should have keys for sending half-RTT data.
- EXPECT_TRUE(
- transport_->server()->HasSecrets(ssl_encryption_application));
+ // At this point, the server has half-RTT write keys, but it cannot access
+ // 1-RTT read keys until client Finished.
+ EXPECT_TRUE(transport_->server()->HasWriteSecret(ssl_encryption_application));
+ EXPECT_FALSE(transport_->server()->HasReadSecret(ssl_encryption_application));
// Finish up the client and server handshakes.
ASSERT_TRUE(CompleteHandshakesForQUIC());
@@ -5268,12 +5320,7 @@
}
TEST_F(QUICMethodTest, ZeroRTTReject) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
SSL_CTX_set_early_data_enabled(client_ctx_.get(), 1);
@@ -5303,14 +5350,16 @@
ASSERT_EQ(SSL_do_handshake(client_.get()), 1);
EXPECT_TRUE(SSL_in_early_data(client_.get()));
// The transport should have keys for sending 0-RTT data.
- EXPECT_TRUE(transport_->client()->HasSecrets(ssl_encryption_early_data));
+ EXPECT_TRUE(
+ transport_->client()->HasWriteSecret(ssl_encryption_early_data));
// The server will consume the ClientHello, but it will not accept 0-RTT.
ASSERT_TRUE(ProvideHandshakeData(server_.get()));
ASSERT_EQ(SSL_do_handshake(server_.get()), -1);
EXPECT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1));
EXPECT_FALSE(SSL_in_early_data(server_.get()));
- EXPECT_FALSE(transport_->server()->HasSecrets(ssl_encryption_early_data));
+ EXPECT_FALSE(
+ transport_->server()->HasReadSecret(ssl_encryption_early_data));
// The client consumes the server response and signals 0-RTT rejection.
for (;;) {
@@ -5343,12 +5392,7 @@
}
TEST_F(QUICMethodTest, NoZeroRTTKeysBeforeReverify) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
SSL_CTX_set_early_data_enabled(client_ctx_.get(), 1);
@@ -5375,7 +5419,7 @@
SSL_ERROR_WANT_CERTIFICATE_VERIFY);
// The early data keys have not yet been released.
- EXPECT_FALSE(transport_->client()->HasSecrets(ssl_encryption_early_data));
+ EXPECT_FALSE(transport_->client()->HasWriteSecret(ssl_encryption_early_data));
// After the verification completes, the handshake progresses to the 0-RTT
// point and releases keys.
@@ -5386,19 +5430,14 @@
});
ASSERT_EQ(SSL_do_handshake(client_.get()), 1);
EXPECT_TRUE(SSL_in_early_data(client_.get()));
- EXPECT_TRUE(transport_->client()->HasSecrets(ssl_encryption_early_data));
+ EXPECT_TRUE(transport_->client()->HasWriteSecret(ssl_encryption_early_data));
}
// Test only releasing data to QUIC one byte at a time on request, to maximize
// state machine pauses. Additionally, test that existing asynchronous callbacks
// still work.
TEST_F(QUICMethodTest, Async) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
@@ -5474,12 +5513,9 @@
return 1;
};
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- add_handshake_data,
- flush_flight,
- SendAlertCallback,
- };
+ SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+ quic_method.add_handshake_data = add_handshake_data;
+ quic_method.flush_flight = flush_flight;
ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
@@ -5506,12 +5542,8 @@
MakeConstSpan(data, len));
};
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- add_handshake_data,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+ quic_method.add_handshake_data = add_handshake_data;
ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
@@ -5540,24 +5572,23 @@
EXPECT_EQ(ERR_GET_LIB(err), ERR_LIB_SSL);
EXPECT_EQ(ERR_GET_REASON(err), SSL_R_EXCESS_HANDSHAKE_DATA);
- // The client sends an alert in response to this.
+ // The client sends an alert in response to this. The alert is sent at
+ // handshake level because we install write secrets before read secrets and
+ // the error is discovered when installing the read secret. (How to send
+ // alerts on protocol syntax errors near key changes is ambiguous in general.)
ASSERT_TRUE(transport_->client()->has_alert());
- EXPECT_EQ(transport_->client()->alert_level(), ssl_encryption_initial);
+ EXPECT_EQ(transport_->client()->alert_level(), ssl_encryption_handshake);
EXPECT_EQ(transport_->client()->alert(), SSL_AD_UNEXPECTED_MESSAGE);
// Sanity-check client did get far enough to process the ServerHello and
// install keys.
- EXPECT_TRUE(transport_->client()->HasSecrets(ssl_encryption_handshake));
+ EXPECT_TRUE(transport_->client()->HasReadSecret(ssl_encryption_handshake));
+ EXPECT_TRUE(transport_->client()->HasWriteSecret(ssl_encryption_handshake));
}
// Test that |SSL_provide_quic_data| will reject data at the wrong level.
TEST_F(QUICMethodTest, ProvideWrongLevel) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
@@ -5597,12 +5628,7 @@
}
TEST_F(QUICMethodTest, TooMuchData) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
@@ -5622,12 +5648,7 @@
// Provide invalid post-handshake data.
TEST_F(QUICMethodTest, BadPostHandshake) {
- const SSL_QUIC_METHOD quic_method = {
- SetEncryptionSecretsCallback,
- AddHandshakeDataCallback,
- FlushFlightCallback,
- SendAlertCallback,
- };
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
g_last_session = nullptr;
diff --git a/ssl/test/mock_quic_transport.cc b/ssl/test/mock_quic_transport.cc
index e63ccbf..23445cb 100644
--- a/ssl/test/mock_quic_transport.cc
+++ b/ssl/test/mock_quic_transport.cc
@@ -42,18 +42,21 @@
write_secrets_(ssl_encryption_application + 1),
ssl_(ssl) {}
-bool MockQuicTransport::SetSecrets(enum ssl_encryption_level_t level,
- const uint8_t *read_secret,
- const uint8_t *write_secret,
- size_t secret_len) {
- if (read_secret) {
- read_secrets_[level].resize(secret_len);
- memcpy(read_secrets_[level].data(), read_secret, secret_len);
- }
- if (write_secret) {
- write_secrets_[level].resize(secret_len);
- memcpy(write_secrets_[level].data(), write_secret, secret_len);
- }
+bool MockQuicTransport::SetReadSecret(enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher,
+ const uint8_t *secret,
+ size_t secret_len) {
+ // TODO(davidben): Assert the various encryption secret invariants.
+ read_secrets_[level].assign(secret, secret + secret_len);
+ return true;
+}
+
+bool MockQuicTransport::SetWriteSecret(enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher,
+ const uint8_t *secret,
+ size_t secret_len) {
+ // TODO(davidben): Assert the various encryption secret invariants.
+ write_secrets_[level].assign(secret, secret + secret_len);
return true;
}
diff --git a/ssl/test/mock_quic_transport.h b/ssl/test/mock_quic_transport.h
index 21f7dc6..6dfed5b 100644
--- a/ssl/test/mock_quic_transport.h
+++ b/ssl/test/mock_quic_transport.h
@@ -25,8 +25,12 @@
public:
explicit MockQuicTransport(bssl::UniquePtr<BIO> bio, SSL *ssl);
- bool SetSecrets(enum ssl_encryption_level_t level, const uint8_t *read_secret,
- const uint8_t *write_secret, size_t secret_len);
+ bool SetReadSecret(enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher, const uint8_t *secret,
+ size_t secret_len);
+ bool SetWriteSecret(enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher, const uint8_t *secret,
+ size_t secret_len);
bool ReadHandshake();
bool WriteHandshakeData(enum ssl_encryption_level_t level,
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index f497704..3f524ff 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -1136,12 +1136,18 @@
return ssl_select_cert_success;
}
-static int SetQuicEncryptionSecrets(SSL *ssl, enum ssl_encryption_level_t level,
- const uint8_t *read_secret,
- const uint8_t *write_secret,
- size_t secret_len) {
- return GetTestState(ssl)->quic_transport->SetSecrets(
- level, read_secret, write_secret, secret_len);
+static int SetQuicReadSecret(SSL *ssl, enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher, const uint8_t *secret,
+ size_t secret_len) {
+ return GetTestState(ssl)->quic_transport->SetReadSecret(level, cipher, secret,
+ secret_len);
+}
+
+static int SetQuicWriteSecret(SSL *ssl, enum ssl_encryption_level_t level,
+ const SSL_CIPHER *cipher, const uint8_t *secret,
+ size_t secret_len) {
+ return GetTestState(ssl)->quic_transport->SetWriteSecret(level, cipher,
+ secret, secret_len);
}
static int AddQuicHandshakeData(SSL *ssl, enum ssl_encryption_level_t level,
@@ -1161,7 +1167,8 @@
}
static const SSL_QUIC_METHOD g_quic_method = {
- SetQuicEncryptionSecrets,
+ SetQuicReadSecret,
+ SetQuicWriteSecret,
AddQuicHandshakeData,
FlushQuicFlight,
SendQuicAlert,
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 9418185..7228471 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -75,20 +75,22 @@
// We do not currently implement DTLS 1.3 and, in QUIC, the caller handles
// 0-RTT data, so we can skip installing 0-RTT keys and act as if there is one
// write level. If we implement DTLS 1.3, we'll need to model this better.
- if (level == ssl_encryption_initial) {
- bssl::UniquePtr<SSLAEADContext> null_ctx =
- SSLAEADContext::CreateNullCipher(SSL_is_dtls(ssl));
- if (!null_ctx || !ssl->method->set_write_state(ssl, ssl_encryption_initial,
- std::move(null_ctx))) {
- return false;
- }
- ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version);
- } else {
- assert(level == ssl_encryption_handshake);
- if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
- hs->new_session.get(),
- hs->client_handshake_secret())) {
- return false;
+ if (ssl->quic_method == nullptr) {
+ if (level == ssl_encryption_initial) {
+ bssl::UniquePtr<SSLAEADContext> null_ctx =
+ SSLAEADContext::CreateNullCipher(SSL_is_dtls(ssl));
+ if (!null_ctx || !ssl->method->set_write_state(ssl, ssl_encryption_initial,
+ std::move(null_ctx))) {
+ return false;
+ }
+ ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version);
+ } else {
+ assert(level == ssl_encryption_handshake);
+ if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
+ hs->new_session.get(),
+ hs->client_handshake_secret())) {
+ return false;
+ }
}
}
@@ -437,18 +439,15 @@
if (!tls13_advance_key_schedule(hs, dhe_secret) ||
!ssl_hash_message(hs, msg) ||
- !tls13_derive_handshake_secrets(hs) ||
- !tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open,
- hs->new_session.get(),
- hs->server_handshake_secret())) {
+ !tls13_derive_handshake_secrets(hs)) {
return ssl_hs_error;
}
- // If currently sending early data, we defer installing client traffic keys to
- // when the early data stream is closed. See |close_early_data|. Note if the
- // server has already rejected 0-RTT via HelloRetryRequest, |in_early_data| is
- // already false.
- if (!hs->in_early_data) {
+ // If currently sending early data over TCP, we defer installing client
+ // traffic keys to when the early data stream is closed. See
+ // |close_early_data|. Note if the server has already rejected 0-RTT via
+ // HelloRetryRequest, |in_early_data| is already false.
+ if (!hs->in_early_data || ssl->quic_method != nullptr) {
if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_seal,
hs->new_session.get(),
hs->client_handshake_secret())) {
@@ -456,6 +455,12 @@
}
}
+ if (!tls13_set_traffic_key(ssl, ssl_encryption_handshake, evp_aead_open,
+ hs->new_session.get(),
+ hs->server_handshake_secret())) {
+ return ssl_hs_error;
+ }
+
ssl->method->next_message(ssl);
hs->tls13_state = state_read_encrypted_extensions;
return ssl_hs_ok;
@@ -803,12 +808,12 @@
}
// Derive the final keys and enable them.
- if (!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_open,
- hs->new_session.get(),
- hs->server_traffic_secret_0()) ||
- !tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal,
+ if (!tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_seal,
hs->new_session.get(),
hs->client_traffic_secret_0()) ||
+ !tls13_set_traffic_key(ssl, ssl_encryption_application, evp_aead_open,
+ hs->new_session.get(),
+ hs->server_traffic_secret_0()) ||
!tls13_derive_resumption_secret(hs)) {
return ssl_hs_error;
}
diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc
index bd12f63..3a2e4e5 100644
--- a/ssl/tls13_enc.cc
+++ b/ssl/tls13_enc.cc
@@ -142,9 +142,35 @@
const SSL_SESSION *session,
Span<const uint8_t> traffic_secret) {
uint16_t version = ssl_session_protocol_version(session);
-
UniquePtr<SSLAEADContext> traffic_aead;
- if (ssl->quic_method == nullptr) {
+ if (ssl->quic_method != nullptr) {
+ // Pass the traffic secrets to QUIC.
+ if (direction == evp_aead_open) {
+ if (!ssl->quic_method->set_read_secret(ssl, level, session->cipher,
+ traffic_secret.data(),
+ traffic_secret.size())) {
+ return false;
+ }
+ } else {
+ if (!ssl->quic_method->set_write_secret(ssl, level, session->cipher,
+ traffic_secret.data(),
+ traffic_secret.size())) {
+ return false;
+ }
+ }
+
+ // QUIC only uses |ssl| for handshake messages, which never use early data
+ // keys, so we return installing anything. This avoids needing to have two
+ // secrets active at once in 0-RTT.
+ if (level == ssl_encryption_early_data) {
+ return true;
+ }
+
+ // Install a placeholder SSLAEADContext so that SSL accessors work. The
+ // encryption itself will be handled by the SSL_QUIC_METHOD.
+ traffic_aead =
+ SSLAEADContext::CreatePlaceholderForQUIC(version, session->cipher);
+ } else {
// Look up cipher suite properties.
const EVP_AEAD *aead;
size_t discard;
@@ -173,17 +199,9 @@
return false;
}
-
traffic_aead = SSLAEADContext::Create(direction, session->ssl_version,
SSL_is_dtls(ssl), session->cipher,
key, Span<const uint8_t>(), iv);
- } else {
- // Install a placeholder SSLAEADContext so that SSL accessors work. The
- // encryption itself will be handled by the SSL_QUIC_METHOD.
- traffic_aead =
- SSLAEADContext::CreatePlaceholderForQUIC(version, session->cipher);
- // QUIC never installs early data keys at the TLS layer.
- assert(level != ssl_encryption_early_data);
}
if (!traffic_aead) {
@@ -237,47 +255,6 @@
return true;
}
-bool tls13_set_early_secret_for_quic(SSL_HANDSHAKE *hs) {
- SSL *const ssl = hs->ssl;
- if (ssl->quic_method == nullptr) {
- return true;
- }
- if (ssl->server) {
- if (!ssl->quic_method->set_encryption_secrets(
- ssl, ssl_encryption_early_data, hs->early_traffic_secret().data(),
- /*write_secret=*/nullptr, hs->early_traffic_secret().size())) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR);
- return false;
- }
- } else {
- if (!ssl->quic_method->set_encryption_secrets(
- ssl, ssl_encryption_early_data, /*read_secret=*/nullptr,
- hs->early_traffic_secret().data(),
- hs->early_traffic_secret().size())) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_INTERNAL_ERROR);
- return false;
- }
- }
- return true;
-}
-
-static bool set_quic_secrets(SSL_HANDSHAKE *hs, ssl_encryption_level_t level,
- Span<const uint8_t> client_write_secret,
- Span<const uint8_t> server_write_secret) {
- SSL *const ssl = hs->ssl;
- assert(client_write_secret.size() == server_write_secret.size());
- if (ssl->quic_method == nullptr) {
- return true;
- }
- if (!ssl->server) {
- std::swap(client_write_secret, server_write_secret);
- }
- return ssl->quic_method->set_encryption_secrets(
- ssl, level,
- /*read_secret=*/client_write_secret.data(),
- /*write_secret=*/server_write_secret.data(), client_write_secret.size());
-}
-
bool tls13_derive_handshake_secrets(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
if (!derive_secret(hs, hs->client_handshake_secret(),
@@ -287,10 +264,7 @@
!derive_secret(hs, hs->server_handshake_secret(),
label_to_span(kTLS13LabelServerHandshakeTraffic)) ||
!ssl_log_secret(ssl, "SERVER_HANDSHAKE_TRAFFIC_SECRET",
- hs->server_handshake_secret()) ||
- !set_quic_secrets(hs, ssl_encryption_handshake,
- hs->client_handshake_secret(),
- hs->server_handshake_secret())) {
+ hs->server_handshake_secret())) {
return false;
}
@@ -313,10 +287,7 @@
label_to_span(kTLS13LabelExporter)) ||
!ssl_log_secret(ssl, "EXPORTER_SECRET",
MakeConstSpan(ssl->s3->exporter_secret,
- ssl->s3->exporter_secret_len)) ||
- !set_quic_secrets(hs, ssl_encryption_application,
- hs->client_traffic_secret_0(),
- hs->server_traffic_secret_0())) {
+ ssl->s3->exporter_secret_len))) {
return false;
}
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index f796260..6730f83 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -718,19 +718,6 @@
SSL *const ssl = hs->ssl;
if (ssl->s3->early_data_accepted) {
- // We defer releasing the early traffic secret to QUIC to this point. First,
- // the early traffic secret is derived before ECDHE, but ECDHE may later
- // reject 0-RTT. We only release the secret after 0-RTT is fully resolved.
- //
- // Second, 0-RTT data is acknowledged with 1-RTT keys. Both are derived as
- // part of the ServerHello flight, but future TLS extensions may insert an
- // asynchronous point in the middle of this flight. We defer releasing the
- // 0-RTT keys to ensure the QUIC implementation never installs read keys
- // without the write keys to send the corresponding ACKs.
- if (!tls13_set_early_secret_for_quic(hs)) {
- return ssl_hs_error;
- }
-
// If accepting 0-RTT, we send tickets half-RTT. This gets the tickets on
// the wire sooner and also avoids triggering a write on |SSL_read| when
// processing the client Finished. This requires computing the client
@@ -779,9 +766,7 @@
static enum ssl_hs_wait_t do_read_second_client_flight(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
if (ssl->s3->early_data_accepted) {
- // QUIC never receives handshake messages under 0-RTT keys.
- if (ssl->quic_method == nullptr &&
- !tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_open,
+ if (!tls13_set_traffic_key(ssl, ssl_encryption_early_data, evp_aead_open,
hs->new_session.get(),
hs->early_traffic_secret())) {
return ssl_hs_error;
