Support sending KeyUpdate in DTLS 1.3

Bug: 42290594
Change-Id: I38312da6425d25038f59dc5c65a17987d3688048
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73587
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/d1_lib.cc

@@ -77,7 +77,8 @@ DTLS1_STATE::DTLS1_STATE()
       handshake_write_overflow(false),
       handshake_read_overflow(false),
       sending_flight(false),
-      sending_ack(false) {}
+      sending_ack(false),
+      queued_key_update(QueuedKeyUpdate::kNone) {}
 
 DTLS1_STATE::~DTLS1_STATE() {}
 

ssl/d1_pkt.cc

@@ -222,6 +222,26 @@ ssl_open_record_t dtls1_process_ack(SSL *ssl, uint8_t *out_alert,
                   [](const auto &msg) { return msg.IsFullyAcked(); })) {
     dtls1_stop_timer(ssl);
     dtls_clear_outgoing_messages(ssl);
+
+    // DTLS 1.3 defers the key update to when the message is ACKed.
+    if (ssl->s3->key_update_pending) {
+      if (!tls13_rotate_traffic_key(ssl, evp_aead_seal)) {
+        return ssl_open_record_error;
+      }
+      ssl->s3->key_update_pending = false;
+    }
+
+    // Check for deferred messages.
+    if (ssl->d1->queued_key_update != QueuedKeyUpdate::kNone) {
+      int request_type =
+          ssl->d1->queued_key_update == QueuedKeyUpdate::kUpdateRequested
+              ? SSL_KEY_UPDATE_REQUESTED
+              : SSL_KEY_UPDATE_NOT_REQUESTED;
+      ssl->d1->queued_key_update = QueuedKeyUpdate::kNone;
+      if (!tls13_add_key_update(ssl, request_type)) {
+        return ssl_open_record_error;
+      }
+    }
   } else {
     // We may still be able to drop unused write epochs.
     dtls_clear_unused_write_epochs(ssl);

ssl/internal.h

@@ -2604,10 +2604,9 @@ const char *ssl_server_handshake_state(SSL_HANDSHAKE *hs);
 const char *tls13_client_handshake_state(SSL_HANDSHAKE *hs);
 const char *tls13_server_handshake_state(SSL_HANDSHAKE *hs);
 
-// tls13_add_key_update queues a KeyUpdate message on |ssl|. The
-// |update_requested| argument must be one of |SSL_KEY_UPDATE_REQUESTED| or
-// |SSL_KEY_UPDATE_NOT_REQUESTED|.
-bool tls13_add_key_update(SSL *ssl, int update_requested);
+// tls13_add_key_update queues a KeyUpdate message on |ssl|. |request_type| must
+// be one of |SSL_KEY_UPDATE_REQUESTED| or |SSL_KEY_UPDATE_NOT_REQUESTED|.
+bool tls13_add_key_update(SSL *ssl, int request_type);
 
 // tls13_post_handshake processes a post-handshake message. It returns true on
 // success and false on failure.
@@ -3238,8 +3237,10 @@ struct SSL3_STATE {
   // Channel ID and the |channel_id| field is filled in.
   bool channel_id_valid : 1;
 
-  // key_update_pending is true if we have a KeyUpdate acknowledgment
-  // outstanding.
+  // key_update_pending is true if we are in the process of sending a KeyUpdate
+  // message. As a DoS mitigation (and a requirement in DTLS), we never send
+  // more than one KeyUpdate at once. In DTLS, this tracks whether there is an
+  // unACKed KeyUpdate.
   bool key_update_pending : 1;
 
   // early_data_accepted is true if early data was accepted by the server.
@@ -3533,6 +3534,12 @@ struct DTLSSentRecord {
   uint32_t last_msg_end = 0;
 };
 
+enum class QueuedKeyUpdate {
+  kNone,
+  kUpdateNotRequested,
+  kUpdateRequested,
+};
+
 struct DTLS1_STATE {
   static constexpr bool kAllowUniquePtr = true;
 
@@ -3565,6 +3572,10 @@ struct DTLS1_STATE {
   bool sending_flight : 1;
   bool sending_ack : 1;
 
+  // queued_key_update, if not kNone, indicates we've queued a KeyUpdate message
+  // to send after the current flight is ACKed.
+  QueuedKeyUpdate queued_key_update : 2;
+
   uint16_t handshake_write_seq = 0;
   uint16_t handshake_read_seq = 0;
 

ssl/ssl_lib.cc

@@ -1121,12 +1121,7 @@ int SSL_key_update(SSL *ssl, int request_type) {
     return 0;
   }
 
-  if (!ssl->s3->key_update_pending &&
-      !tls13_add_key_update(ssl, request_type)) {
-    return 0;
-  }
-
-  return 1;
+  return tls13_add_key_update(ssl, request_type);
 }
 
 int SSL_shutdown(SSL *ssl) {

ssl/test/bssl_shim.cc

@@ -1196,6 +1196,12 @@ static bool DoExchange(bssl::UniquePtr<SSL_SESSION> *out_session,
     }
     if (!config->shim_shuts_down) {
       for (;;) {
+        if (config->key_update_before_read &&
+            !SSL_key_update(ssl, SSL_KEY_UPDATE_NOT_REQUESTED)) {
+          fprintf(stderr, "SSL_key_update failed.\n");
+          return false;
+        }
+
         // Read only 512 bytes at a time in TLS to ensure records may be
         // returned in multiple reads.
         size_t read_size = config->is_dtls ? 16384 : 512;

ssl/test/runner/common.go

@@ -2007,9 +2007,7 @@ type ProtocolBugs struct {
 	// OmitPublicName omits the server name extension from ClientHelloOuter.
 	OmitPublicName bool
 
-	// AllowEpochOverflow allows the DTLS write epoch to wrap around. The DTLS
-	// read epoch is never allowed to wrap around because that would be a bug in
-	// the shim.
+	// AllowEpochOverflow allows DTLS epoch numbers to wrap around.
 	AllowEpochOverflow bool
 }
 

ssl/test/runner/conn.go

@@ -1660,6 +1660,20 @@ func (c *Conn) processTLS13NewSessionTicket(newSessionTicket *newSessionTicketMs
 	return c.ackHandshake()
 }
 
+func (c *Conn) processKeyUpdate(keyUpdate *keyUpdateMsg) error {
+	epoch := c.in.epoch.epoch + 1
+	if epoch == 0 && !c.config.Bugs.AllowEpochOverflow {
+		return errors.New("tls: too many KeyUpdates")
+	}
+	if err := c.useInTrafficSecret(epoch, c.in.wireVersion, c.cipherSuite, updateTrafficSecret(c.cipherSuite.hash(), c.wireVersion, c.in.trafficSecret, c.isDTLS)); err != nil {
+		return err
+	}
+	if keyUpdate.keyUpdateRequest == keyUpdateRequested {
+		c.keyUpdateRequested = true
+	}
+	return c.ackHandshake()
+}
+
 func (c *Conn) handlePostHandshakeMessage() error {
 	msg, err := c.readHandshake()
 	if err != nil {
@@ -1690,21 +1704,10 @@ func (c *Conn) handlePostHandshakeMessage() error {
 
 	if keyUpdate, ok := msg.(*keyUpdateMsg); ok {
 		c.keyUpdateSeen = true
-
 		if c.config.Bugs.RejectUnsolicitedKeyUpdate {
 			return errors.New("tls: unexpected KeyUpdate message")
 		}
-		epoch := c.in.epoch.epoch + 1
-		if epoch == 0 {
-			return errors.New("tls: too many KeyUpdates")
-		}
-		if err := c.useInTrafficSecret(epoch, c.in.wireVersion, c.cipherSuite, updateTrafficSecret(c.cipherSuite.hash(), c.wireVersion, c.in.trafficSecret, c.isDTLS)); err != nil {
-			return err
-		}
-		if keyUpdate.keyUpdateRequest == keyUpdateRequested {
-			c.keyUpdateRequested = true
-		}
-		return c.ackHandshake()
+		return c.processKeyUpdate(keyUpdate)
 	}
 
 	c.sendAlert(alertUnexpectedMessage)
@@ -1717,27 +1720,16 @@ func (c *Conn) ReadKeyUpdate() error {
 	c.in.Lock()
 	defer c.in.Unlock()
 
-	msg, err := c.readHandshake()
+	keyUpdate, err := readHandshakeType[keyUpdateMsg](c)
 	if err != nil {
 		return err
 	}
 
-	keyUpdate, ok := msg.(*keyUpdateMsg)
-	if !ok {
-		c.sendAlert(alertUnexpectedMessage)
-		return fmt.Errorf("tls: unexpected message (%T) when reading KeyUpdate", msg)
-	}
-
 	if keyUpdate.keyUpdateRequest != keyUpdateNotRequested {
 		return errors.New("tls: received invalid KeyUpdate message")
 	}
 
-	epoch := c.in.epoch.epoch + 1
-	if epoch == 0 {
-		return errors.New("tls: too many KeyUpdates")
-	}
-
-	return c.useInTrafficSecret(epoch, c.in.wireVersion, c.cipherSuite, updateTrafficSecret(c.cipherSuite.hash(), c.wireVersion, c.in.trafficSecret, c.isDTLS))
+	return c.processKeyUpdate(keyUpdate)
 }
 
 func (c *Conn) Renegotiate() error {

ssl/test/runner/runner.go

@@ -627,6 +627,9 @@ type testCase struct {
 	sendKeyUpdates int
 	// keyUpdateRequest is the KeyUpdateRequest value to send in KeyUpdate messages.
 	keyUpdateRequest byte
+	// shimSendsKeyUpdateBeforeRead indicates the shim should send a KeyUpdate
+	// message before each read.
+	shimSendsKeyUpdateBeforeRead bool
 	// expectUnsolicitedKeyUpdate makes the test expect a one or more KeyUpdate
 	// messages while reading data from the shim. Don't use this in combination
 	// with any of the fields that send a KeyUpdate otherwise any received
@@ -1149,6 +1152,12 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool, tr
 			}
 		}
 
+		if test.shimSendsKeyUpdateBeforeRead {
+			if err := tlsConn.ReadKeyUpdate(); err != nil {
+				return err
+			}
+		}
+
 		testMessage := makeTestMessage(j, messageLen)
 		if _, err := tlsConn.Write(testMessage); err != nil {
 			return err
@@ -1214,6 +1223,14 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool, tr
 		}
 	}
 
+	// The shim will end attempting to read, sending one last KeyUpdate. Consume
+	// the KeyUpdate before closing the connection.
+	if test.shimSendsKeyUpdateBeforeRead {
+		if err := tlsConn.ReadKeyUpdate(); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -1659,6 +1676,10 @@ func runTest(dispatcher *shimDispatcher, statusChan chan statusMsg, test *testCa
 		flags = append(flags, "-tls-unique")
 	}
 
+	if test.shimSendsKeyUpdateBeforeRead {
+		flags = append(flags, "-key-update-before-read")
+	}
+
 	if *waitForDebugger {
 		flags = append(flags, "-wait-for-debugger")
 	}
@@ -22248,6 +22269,159 @@ func addKeyUpdateTests() {
 		flags:            []string{"-async"},
 	})
 
+	// In DTLS, we KeyUpdate before read, rather than write, because the
+	// KeyUpdate will not be applied before the shim reads the ACK.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-FromClient-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+		// Perform several message exchanges to update keys several times.
+		messageCount: 10,
+	})
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		testType: serverTest,
+		name:     "KeyUpdate-FromServer-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+		// Perform several message exchanges to update keys several times.
+		messageCount: 10,
+		// Avoid NewSessionTicket messages getting in the way of ReadKeyUpdate.
+		flags: []string{"-no-ticket"},
+	})
+
+	// If the shim has a pending unACKed flight, it defers sending KeyUpdate.
+	// BoringSSL does not support multiple outgoing flights at once.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-DeferredSend-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			// Request a client certificate, so the shim has more to send.
+			ClientAuth: RequireAnyClientCert,
+			Bugs: ProtocolBugs{
+				MaxPacketLength: 512,
+				ACKFlightDTLS: func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+					if received[len(received)-1].Type != typeFinished {
+						c.WriteACK(c.OutEpoch(), records)
+						return
+					}
+
+					// This test relies on the Finished flight being multiple
+					// records.
+					if len(records) <= 1 {
+						panic("shim sent Finished flight in one record")
+					}
+
+					// Before ACKing Finished, do some rounds of exchanging
+					// application data. Although the shim has already scheduled
+					// KeyUpdate, it should not send the KeyUpdate until it gets
+					// an ACK. (If it sent KeyUpdate, ReadAppData would report
+					// an unexpected record.)
+					msg := []byte("test")
+					for i := 0; i < 10; i++ {
+						c.WriteAppData(c.OutEpoch(), msg)
+						c.ReadAppData(c.InEpoch(), expectedReply(msg))
+					}
+
+					// ACK some of the Finished flight, but not all of it.
+					c.WriteACK(c.OutEpoch(), records[:1])
+
+					// The shim continues to defer KeyUpdate.
+					for i := 0; i < 10; i++ {
+						c.WriteAppData(c.OutEpoch(), msg)
+						c.ReadAppData(c.InEpoch(), expectedReply(msg))
+					}
+
+					// ACK the remainder.
+					c.WriteACK(c.OutEpoch(), records[1:])
+
+					// The shim should now send KeyUpdate. Return to the test
+					// harness, which will look for it.
+				},
+			},
+		},
+		shimCertificate:              &rsaChainCertificate,
+		shimSendsKeyUpdateBeforeRead: true,
+		flags:                        []string{"-mtu", "512"},
+	})
+
+	// The shim should not switch keys until it receives an ACK.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-WaitForACK-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				MaxPacketLength: 512,
+				ACKFlightDTLS: func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+					if received[0].Type != typeKeyUpdate {
+						c.WriteACK(c.OutEpoch(), records)
+						return
+					}
+
+					// Make the shim send application data. We have not yet
+					// ACKed KeyUpdate, so the shim should send at the previous
+					// epoch. Through each of these rounds, the shim will also
+					// try to KeyUpdate again. These calls will be suppressed
+					// because there is still an outstanding KeyUpdate.
+					msg := []byte("test")
+					for i := 0; i < 10; i++ {
+						c.WriteAppData(c.OutEpoch(), msg)
+						c.ReadAppData(c.InEpoch()-1, expectedReply(msg))
+					}
+
+					// ACK the KeyUpdate. Ideally we'd test a partial ACK, but
+					// BoringSSL's minimum MTU is such that KeyUpdate always
+					// fits in one record.
+					c.WriteACK(c.OutEpoch(), records)
+
+					// The shim should now send at the new epoch. Return to the
+					// test harness, which will enforce this.
+				},
+			},
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+	})
+
+	// Test that shim responds to KeyUpdate requests.
+	fixKeyUpdateReply := func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+		c.WriteACK(c.OutEpoch(), records)
+		if received[0].Type != typeKeyUpdate {
+			return
+		}
+		// This works around an awkward testing mismatch. The test
+		// harness expects the shim to immediately change keys, but
+		// the shim writes app data before seeing the ACK. The app
+		// data will be sent at the previous epoch. Consume this and
+		// prime the shim to resend its reply at the new epoch.
+		msg := makeTestMessage(int(received[0].Sequence)-2, 32)
+		c.ReadAppData(c.InEpoch()-1, expectedReply(msg))
+		c.WriteAppData(c.OutEpoch(), msg)
+	}
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-Requested-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				RejectUnsolicitedKeyUpdate: true,
+				ACKFlightDTLS:              fixKeyUpdateReply,
+			},
+		},
+		// Test the shim receiving many KeyUpdates in a row. They will be
+		// combined into one reply KeyUpdate.
+		sendKeyUpdates:   5,
+		messageLen:       32,
+		messageCount:     5,
+		keyUpdateRequest: keyUpdateRequested,
+	})
+
 	mergeNewSessionTicketAndKeyUpdate := func(f WriteFlightFunc) WriteFlightFunc {
 		return func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
 			// Send NewSessionTicket and the first KeyUpdate all together.
@@ -22358,12 +22532,15 @@ func addKeyUpdateTests() {
 
 	// Test KeyUpdate overflow conditions. Both the epoch number and the message
 	// number may overflow, in either the read or write direction.
-	//
-	// TODO(crbug.com/42290594): Test the epoch write number overflowing, once
-	// we implement sending KeyUpdates.
-	//
-	// TODO(crbug.com/42290594): Test the message write number overflowing, once
-	// we implement sending KeyUpdates.
+
+	// When the sender is the client, the first KeyUpdate is message 2 at epoch
+	// 3, so the epoch number overflows first.
+	const maxClientKeyUpdates = 0xffff - 3
+
+	// Test that the shim, as a server, rejects KeyUpdates at epoch 0xffff. RFC
+	// 9147 does not prescribe this limit, but we enforce it. See
+	// https://mailarchive.ietf.org/arch/msg/tls/6y8wTv8Q_IPM-PCcbCAmDOYg6bM/
+	// and https://www.rfc-editor.org/errata/eid8050
 	writeFlightKeyUpdate := func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
 		if next[0].Type == typeKeyUpdate {
 			// Exchange some data to avoid tripping KeyUpdate DoS limits.
@@ -22373,12 +22550,6 @@ func addKeyUpdateTests() {
 		}
 		c.WriteFlight(next)
 	}
-
-	// When the runner is the client, the first KeyUpdate is message 2 at epoch
-	// 3, so the epoch number overflows first. Test that the shim, as a server,
-	// rejects KeyUpdates at epoch 0xffff. RFC 9147 does not prescribe this
-	// limit, but we enforce it. See https://mailarchive.ietf.org/arch/msg/tls/6y8wTv8Q_IPM-PCcbCAmDOYg6bM/
-	// and https://www.rfc-editor.org/errata/eid8050
 	testCases = append(testCases, testCase{
 		testType: serverTest,
 		protocol: dtls,
@@ -22391,9 +22562,8 @@ func addKeyUpdateTests() {
 			},
 		},
 		// Avoid the NewSessionTicket messages interfering with the callback.
-		flags: []string{"-no-ticket"},
-		// Application data starts at epoch 3. Epoch 0xffff is the limit.
-		sendKeyUpdates:   0xffff - 3,
+		flags:            []string{"-no-ticket"},
+		sendKeyUpdates:   maxClientKeyUpdates,
 		keyUpdateRequest: keyUpdateNotRequested,
 	})
 	testCases = append(testCases, testCase{
@@ -22408,23 +22578,56 @@ func addKeyUpdateTests() {
 			},
 		},
 		// Avoid the NewSessionTicket messages interfering with the callback.
-		flags: []string{"-no-ticket"},
-		// Application data starts at epoch 3. Epoch 0xffff is the limit.
-		sendKeyUpdates:     0xffff - 3 + 1,
+		flags:              []string{"-no-ticket"},
+		sendKeyUpdates:     maxClientKeyUpdates + 1,
 		keyUpdateRequest:   keyUpdateNotRequested,
 		shouldFail:         true,
 		expectedError:      ":TOO_MANY_KEY_UPDATES:",
 		expectedLocalError: "remote error: unexpected message",
 	})
 
-	// When the runner is a server, the first KeyUpdate is message 7 (SH, EE, C,
-	// CV, Fin, NST, NST) at epoch 3, so the message number overflows first.
+	// Test that the shim, as a client, notices its epoch overflow condition
+	// when asked to send too many KeyUpdates. The shim sends KeyUpdate before
+	// every read, including reading connection close, so the number of
+	// KeyUpdates is one more than the message count.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-MaxWriteEpoch-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+		messageCount:                 maxClientKeyUpdates - 1,
+	})
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-WriteEpochOverflow-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				// The shim does not notice the overflow until immediately after
+				// sending KeyUpdate, so tolerate the overflow on the runner.
+				AllowEpochOverflow: true,
+			},
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+		messageCount:                 maxClientKeyUpdates,
+		shouldFail:                   true,
+		expectedError:                ":TOO_MANY_KEY_UPDATES:",
+	})
+
+	// When the sender is a server that doesn't send tickets, the first
+	// KeyUpdate is message 5 (SH, EE, C, CV, Fin) at epoch 3, so the message
+	// number overflows first.
+	const maxServerKeyUpdates = 0xffff - 5
+
 	// Test that the shim, as a client, does not allow the value to wraparound.
 	testCases = append(testCases, testCase{
 		protocol: dtls,
 		name:     "KeyUpdate-ReadMessageOverflow-DTLS",
 		config: Config{
-			MaxVersion: VersionTLS13,
+			MaxVersion:             VersionTLS13,
+			SessionTicketsDisabled: true,
 			Bugs: ProtocolBugs{
 				AllowEpochOverflow: true,
 				WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
@@ -22450,9 +22653,38 @@ func addKeyUpdateTests() {
 				},
 			},
 		},
-		sendKeyUpdates:   0xffff - 7 + 1,
+		sendKeyUpdates:   maxServerKeyUpdates + 1,
 		keyUpdateRequest: keyUpdateNotRequested,
-		flags:            []string{"-async"},
+		flags:            []string{"-async", "-expect-no-session"},
+	})
+
+	// Test that the shim, as a server, notices its message overflow condition,
+	// when asked to send too many KeyUpdates.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		testType: serverTest,
+		name:     "KeyUpdate-MaxWriteMessage-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+		messageCount:                 maxServerKeyUpdates,
+		// Avoid NewSessionTicket messages getting in the way of ReadKeyUpdate.
+		flags: []string{"-no-ticket"},
+	})
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		testType: serverTest,
+		name:     "KeyUpdate-WriteMessageOverflow-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		shimSendsKeyUpdateBeforeRead: true,
+		messageCount:                 maxServerKeyUpdates + 1,
+		shouldFail:                   true,
+		expectedError:                ":overflow:",
+		// Avoid NewSessionTicket messages getting in the way of ReadKeyUpdate.
+		flags: []string{"-no-ticket"},
 	})
 }
 

ssl/test/test_config.cc

@@ -471,6 +471,8 @@ const Flag<TestConfig> *FindFlag(const char *name) {
         BoolFlag("-export-traffic-secrets",
                  &TestConfig::export_traffic_secrets),
         BoolFlag("-key-update", &TestConfig::key_update),
+        BoolFlag("-key-update-before-read",
+                 &TestConfig::key_update_before_read),
         StringFlag("-expect-early-data-reason",
                    &TestConfig::expect_early_data_reason),
         BoolFlag("-expect-hrr", &TestConfig::expect_hrr),

ssl/test/test_config.h

@@ -208,6 +208,7 @@ struct TestConfig {
   bool server_preference = false;
   bool export_traffic_secrets = false;
   bool key_update = false;
+  bool key_update_before_read = false;
   std::string expect_early_data_reason;
   bool expect_hrr = false;
   bool expect_no_hrr = false;

ssl/tls13_both.cc

@@ -618,13 +618,31 @@ bool tls13_add_finished(SSL_HANDSHAKE *hs) {
   return true;
 }
 
-bool tls13_add_key_update(SSL *ssl, int update_requested) {
+bool tls13_add_key_update(SSL *ssl, int request_type) {
+  if (ssl->s3->key_update_pending) {
+    return true;
+  }
+
+  // We do not support multiple parallel outgoing flights. If there is an
+  // outgoing flight pending, queue the KeyUpdate for later.
+  if (SSL_is_dtls(ssl) && !ssl->d1->outgoing_messages.empty()) {
+    ssl->d1->queued_key_update = request_type == SSL_KEY_UPDATE_REQUESTED
+                                     ? QueuedKeyUpdate::kUpdateRequested
+                                     : QueuedKeyUpdate::kUpdateNotRequested;
+    return true;
+  }
+
   ScopedCBB cbb;
   CBB body_cbb;
   if (!ssl->method->init_message(ssl, cbb.get(), &body_cbb,
                                  SSL3_MT_KEY_UPDATE) ||
-      !CBB_add_u8(&body_cbb, update_requested) ||
-      !ssl_add_message_cbb(ssl, cbb.get()) ||
+      !CBB_add_u8(&body_cbb, request_type) ||
+      !ssl_add_message_cbb(ssl, cbb.get())) {
+    return false;
+  }
+
+  // In DTLS, the actual key update is deferred until KeyUpdate is ACKed.
+  if (!SSL_is_dtls(ssl) &&
       !tls13_rotate_traffic_key(ssl, evp_aead_seal)) {
     return false;
   }
@@ -654,10 +672,7 @@ static bool tls13_receive_key_update(SSL *ssl, const SSLMessage &msg) {
   }
 
   // Acknowledge the KeyUpdate
-  // TODO(crbug.com/42290594): Support sending KeyUpdate in DTLS 1.3.
-  if (!SSL_is_dtls(ssl) &&  //
-      key_update_request == SSL_KEY_UPDATE_REQUESTED &&
-      !ssl->s3->key_update_pending &&
+  if (key_update_request == SSL_KEY_UPDATE_REQUESTED &&
       !tls13_add_key_update(ssl, SSL_KEY_UPDATE_NOT_REQUESTED)) {
     return false;
   }