Handle implicit ACKs in DTLS 1.3
Receiving part of the next flight should implicitly ACK the previous
flight, in most cases. The final flight, and post-handshake messages are
tricky, but we'll handle and test those in later CLs.
Bug: 42290594
Change-Id: Ia47551a103c7ca224c305892f36e4a270c14a396
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72827
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index 16b32c1..841edfe 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -351,6 +351,7 @@
bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert,
Span<const uint8_t> record) {
+ bool implicit_ack = false;
CBS cbs = record;
while (CBS_len(&cbs) > 0) {
// Read a handshake fragment.
@@ -392,6 +393,27 @@
return false;
}
+ if (SSL_in_init(ssl) && ssl->s3->version != 0 &&
+ ssl_protocol_version(ssl) >= TLS1_3_VERSION) {
+ // During the handshake, if we receive any portion of the next flight, the
+ // peer must have received our most recent flight. In DTLS 1.3, this is an
+ // implicit ACK. See RFC 9147, Section 7.1.
+ //
+ // This only applies during the handshake. After the handshake, the next
+ // message may be part of a post-handshake transaction. It also does not
+ // apply immediately after the handshake. As a client, receiving a
+ // KeyUpdate or NewSessionTicket does not imply the server has received
+ // our Finished. The server may have sent those messages in half-RTT.
+ //
+ // TODO(crbug.com/42290594): If we're offering 0-RTT, the final version
+ // is not yet known to be DTLS 1.3 and we should not do this. Track early
+ // and final versions more accurately.
+ //
+ // TODO(crbug.com/42290594): Once post-handshake messages are working,
+ // write a test for the half-RTT KeyUpdate case.
+ implicit_ack = true;
+ }
+
if (msg_hdr.seq - ssl->d1->handshake_read_seq > SSL_MAX_HANDSHAKE_FLIGHT) {
// Ignore fragments too far in the future.
continue;
@@ -416,6 +438,11 @@
frag->reassembly.MarkRange(frag_off, frag_off + frag_len);
}
+ if (implicit_ack) {
+ dtls1_stop_timer(ssl);
+ dtls_clear_outgoing_messages(ssl);
+ }
+
return true;
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 362a763..7b08099 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -11548,12 +11548,9 @@
flags: flags,
})
- // In DTLS 1.2, receiving a part of the next flight should not stop
- // the retransmission timer.
- //
- // TODO(crbug.com/42290594): In DTLS 1.3, it should stop the timer
- // because we can use ACKs to request a retransmit. Test this.
if vers.version <= VersionTLS12 {
+ // In DTLS 1.2, receiving a part of the next flight should not stop
+ // the retransmission timer.
testCases = append(testCases, testCase{
protocol: dtls,
name: "DTLS-Retransmit-PartialProgress" + suffix,
@@ -11582,6 +11579,98 @@
},
flags: flags,
})
+ } else {
+ // In DTLS 1.3, receiving a part of the next flight implicitly ACKs
+ // the previous flight.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ protocol: dtls,
+ name: "DTLS-Retransmit-PartialProgress-Server" + suffix,
+ config: Config{
+ MaxVersion: vers.version,
+ DefaultCurves: []CurveID{}, // Force HelloRetryRequest.
+ Bugs: ProtocolBugs{
+ WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
+ // Send a portion of the first message. The rest was lost.
+ msg := next[0]
+ split := len(msg.Data) / 2
+ c.WriteFragments([]DTLSFragment{msg.Fragment(0, split)})
+ // This is enough to ACK the previous flight. The shim
+ // should stop retransmitting and even stop the timer.
+ //
+ // TODO(crbug.com/42290594): The shim should send a partial
+ // ACK to request that we retransmit.
+ for _, t := range useTimeouts {
+ c.AdvanceClock(t)
+ }
+ // "Retransmit" the rest of the flight. The shim should remember
+ // the portion that was already sent.
+ rest := []DTLSFragment{msg.Fragment(split, len(msg.Data)-split)}
+ for _, m := range next[1:] {
+ rest = append(rest, m.Fragment(0, len(m.Data)))
+ }
+ c.WriteFragments(rest)
+ },
+ },
+ },
+ flags: flags,
+ })
+
+ // When the shim is a client, receiving fragments before the version is
+ // known does not trigger this behavior.
+ testCases = append(testCases, testCase{
+ protocol: dtls,
+ name: "DTLS-Retransmit-PartialProgress-Client" + suffix,
+ config: Config{
+ MaxVersion: vers.version,
+ Bugs: ProtocolBugs{
+ WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
+ // Send a portion of the ServerHello. The rest was lost.
+ msg := next[0]
+ split := len(msg.Data) / 2
+ c.WriteFragments([]DTLSFragment{msg.Fragment(0, split)})
+
+ // The shim did not know this was DTLS 1.3, so it still
+ // retransmits ClientHello.
+ c.AdvanceClock(useTimeouts[0])
+ c.ReadRetransmit()
+
+ // Finish the ServerHello. The version is still not known,
+ // at the time the ServerHello fragment is processed, This
+ // is not as efficient as we could be; we could go back and
+ // implicitly ACK once the version is known. But the last
+ // byte of ServerHello will almost certainly be in the same
+ // packet as EncryptedExtensions, which will trigger the case
+ // below.
+ c.WriteFragments([]DTLSFragment{msg.Fragment(split, len(msg.Data)-split)})
+ c.AdvanceClock(useTimeouts[1])
+ c.ReadRetransmit()
+
+ // Send EncryptedExtensions. The shim now knows the version
+ // and implicitly ACKs everything.
+ c.WriteFragments([]DTLSFragment{next[1].Fragment(0, len(next[1].Data))})
+
+ // This is enough to ACK the previous flight. The shim
+ // should stop retransmitting and even stop the timer.
+ //
+ // TODO(crbug.com/42290594): The shim should send a partial
+ // ACK to request that we retransmit.
+ for _, t := range useTimeouts[2:] {
+ c.AdvanceClock(t)
+ }
+
+ // "Retransmit" the rest of the flight. The shim should remember
+ // the portion that was already sent.
+ var rest []DTLSFragment
+ for _, m := range next[2:] {
+ rest = append(rest, m.Fragment(0, len(m.Data)))
+ }
+ c.WriteFragments(rest)
+ },
+ },
+ },
+ flags: flags,
+ })
}
// Test that exceeding the timeout schedule hits a read
