Test that the DTLS 1.3 server retransmits NewSessionTicket correctly.
Testing NewSessionTicket is tricky. First, BoringSSL sends two tickets
in a row. These are conceptually separate flights, but we test them as
one flight. I've added a mechanism in the testing callbacks to merge two
flights together.
Second, these tickets are sent concurrently with the runner's first test
message. The shim's reply will come in before any retransmit challenges.
We need to correct for this in the callback.
Bug: 42290594
Change-Id: Id6887c435f7d1d406976377ca66081869e3f1f78
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73230
Commit-Queue: Nick Harper <nharper@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index e691363..36e5a8d 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1293,12 +1293,12 @@
// WriteFlightDTLS, if not nil, overrides the default behavior for writing
// the flight in DTLS. See DTLSController for details.
- WriteFlightDTLS func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo)
+ WriteFlightDTLS WriteFlightFunc
// ACKFlightDTLS, if not nil, overrides the default behavior for
// acknowledging the final flight (of either the handshake or a
// post-handshake transaction) in DTLS. See DTLSController for details.
- ACKFlightDTLS func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo)
+ ACKFlightDTLS ACKFlightFunc
// MockQUICTransport is the mockQUICTransport used when testing
// QUIC interfaces.
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index f56c44d..4ced9f2 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -443,7 +443,15 @@
return err
}
- return nil
+ if c.receivedFlight != nil || c.receivedFlightRecords != nil || c.nextFlight != nil {
+ panic("tls: flight state changed while writing flight")
+ }
+ if controller.mergeIntoNextFlight {
+ c.previousFlight, c.receivedFlight, c.nextFlight, c.receivedFlightRecords = prev, received, next, records
+ }
+
+ // Flush any ACKs, etc., we may have written.
+ return c.dtlsFlushPacket()
}
func (c *Conn) dtlsFlushHandshake() error {
@@ -483,7 +491,15 @@
return err
}
- return nil
+ if c.previousFlight != nil || c.receivedFlight != nil || c.receivedFlightRecords != nil {
+ panic("tls: flight state changed while ACKing flight")
+ }
+ if controller.mergeIntoNextFlight {
+ c.previousFlight, c.receivedFlight, c.receivedFlightRecords = prev, received, records
+ }
+
+ // Flush any ACKs, etc., we may have written.
+ return c.dtlsFlushPacket()
}
// appendDTLS13RecordHeader appends to b the record header for a record of length
@@ -801,6 +817,9 @@
return c
}
+type WriteFlightFunc = func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo)
+type ACKFlightFunc = func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo)
+
// A DTLSController is passed to a test callback and allows the callback to
// customize how an individual flight is sent. This is used to test DTLS's
// retransmission logic.
@@ -808,9 +827,7 @@
// Although DTLS runs over a lossy, reordered channel, runner assumes a
// reliable, ordered channel. When simulating packet loss, runner processes the
// shim's "lost" flight as usual. But, instead of responding, it calls a
-// test-provided function of the form:
-//
-// func WriteFlight(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo)
+// test-provided function type WriteFlightFunc.
//
// WriteFlight will be called next as the flight for the runner to send. prev is
// the previous flight sent by the runner, and received is the most recent
@@ -837,9 +854,7 @@
//
// When the shim speaks last in a handshake or post-handshake transaction, there
// is no reply to implicitly acknowledge the flight. The runner will instead
-// call a second callback of the form:
-//
-// func ACKFlight(c *DTLSController, prev, received []DTLSMessage)
+// call a second callback type ACKFlightFunc.
//
// Like WriteFlight, ACKFlight may simulate packet loss with the DTLSController.
// It returns when it is ready to proceed. If not specified, it does nothing in
@@ -858,7 +873,8 @@
err error
// retransmitNeeded contains the list of fragments which the shim must
// retransmit.
- retransmitNeeded []DTLSFragment
+ retransmitNeeded []DTLSFragment
+ mergeIntoNextFlight bool
}
func newDTLSController(conn *Conn, received []DTLSMessage) DTLSController {
@@ -1375,3 +1391,10 @@
}
c.err = c.conn.config.Bugs.PacketAdaptor.ExpectNoNextTimeout()
}
+
+// MergeIntoNextFlight indicates the state from this flight should be merged
+// into the next WriteFlight or ACKFlight call. This allows the test to control
+// two independent post-handshake messages as a single unit.
+func (c *DTLSController) MergeIntoNextFlight() {
+ c.mergeIntoNextFlight = true
+}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 29c1231..54b19ee 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -727,6 +727,14 @@
return t.Conn.Write(b)
}
+func makeTestMessage(msgIdx int, messageLen int) []byte {
+ testMessage := make([]byte, messageLen)
+ for i := range testMessage {
+ testMessage[i] = 0x42 ^ byte(msgIdx)
+ }
+ return testMessage
+}
+
func expectedReply(b []byte) []byte {
ret := make([]byte, len(b))
for i, v := range b {
@@ -1129,10 +1137,7 @@
tlsConn.SendAlert(0x42, alertUnexpectedMessage)
}
- testMessage := make([]byte, messageLen)
- for i := range testMessage {
- testMessage[i] = 0x42 ^ byte(j)
- }
+ testMessage := makeTestMessage(j, messageLen)
tlsConn.Write(testMessage)
// Consume the shim prefix if needed.
@@ -11585,6 +11590,38 @@
useTimeouts = shortTimeouts
}
+ // Testing NewSessionTicket is tricky. First, BoringSSL sends two
+ // tickets in a row. These are conceptually separate flights, but we
+ // test them as one flight. Second, these tickets are sent
+ // concurrently with the runner's first test message. The shim's
+ // reply will come in before any retransmit challenges.
+ // handleNewSessionTicket corrects for both effects.
+ handleNewSessionTicket := func(f ACKFlightFunc) ACKFlightFunc {
+ if vers.version < VersionTLS13 {
+ return f
+ }
+ return func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ // BoringSSL sends two NewSessionTickets in a row.
+ if received[0].Type == typeNewSessionTicket && len(received) < 2 {
+ c.MergeIntoNextFlight()
+ return
+ }
+ // NewSessionTicket is sent in parallel with the runner's
+ // first application data. Consume the shim's reply.
+ testMessage := makeTestMessage(0, 32)
+ if received[0].Type == typeNewSessionTicket {
+ c.ReadAppData(c.InEpoch(), expectedReply(testMessage))
+ }
+ // Run the test, without any stray messages in the way.
+ f(c, prev, received, records)
+ // The test loop is expecting a reply to the first message.
+ // Prime the shim to send it again.
+ if received[0].Type == typeNewSessionTicket {
+ c.WriteAppData(c.OutEpoch(), testMessage)
+ }
+ }
+ }
+
// In all versions, the sender will retransmit the whole flight if
// it times out and hears nothing.
writeFlightBasic := func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
@@ -11601,10 +11638,19 @@
// Finally release the whole flight to the shim.
c.WriteFlight(next)
}
- ackFlightBasic := func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ ackFlightBasic := handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
if vers.version >= VersionTLS13 {
- // TODO(crbug.com/42290594): Implement retransmitting the
- // final flight in DTLS 1.3.
+ // In DTLS 1.3, final flights (either handshake or post-handshake)
+ // are retransmited until ACKed. Exercise every timeout but
+ // the last one (which would fail the connection).
+ for _, t := range useTimeouts[:len(useTimeouts)-1] {
+ c.ExpectNextTimeout(t)
+ c.AdvanceClock(t)
+ c.ReadRetransmit()
+ }
+ c.ExpectNextTimeout(useTimeouts[len(useTimeouts)-1])
+ // Finally ACK the flight.
+ c.WriteACK(c.OutEpoch(), records)
return
}
// In DTLS 1.2, the final flight is retransmitted on receipt of
@@ -11614,7 +11660,7 @@
c.WriteFlight(prev)
c.ReadRetransmit()
}
- }
+ })
testCases = append(testCases, testCase{
protocol: dtls,
name: "DTLS-Retransmit-Client-Basic" + suffix,
@@ -11887,7 +11933,7 @@
if len(received) > 0 {
c.ExpectNextTimeout(useTimeouts[0])
c.WriteACK(c.OutEpoch(), records)
- // After ACKing everything, the shim should stop the timer
+ // After everything is ACKed, the shim should stop the timer
// and wait for the next flight.
c.ExpectNoNextTimeout()
for _, t := range useTimeouts {
@@ -11896,6 +11942,15 @@
}
c.WriteFlight(next)
},
+ ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ c.ExpectNextTimeout(useTimeouts[0])
+ c.WriteACK(c.OutEpoch(), records)
+ // After everything is ACKed, the shim should stop the timer.
+ c.ExpectNoNextTimeout()
+ for _, t := range useTimeouts {
+ c.AdvanceClock(t)
+ }
+ }),
SequenceNumberMapping: func(in uint64) uint64 {
// Perturb sequence numbers to test that ACKs are sorted.
return in ^ 63
@@ -11935,6 +11990,15 @@
}
c.WriteFlight(next)
},
+ ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ for _, t := range useTimeouts[:len(useTimeouts)-1] {
+ if len(records) > 0 {
+ c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[len(records)-1]})
+ }
+ c.AdvanceClock(t)
+ records = c.ReadRetransmit()
+ }
+ }),
},
},
shimCertificate: &rsaChainCertificate,
@@ -11964,6 +12028,15 @@
}
c.WriteFlight(next)
},
+ ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ for _, t := range useTimeouts[:len(useTimeouts)-1] {
+ if len(records) > 0 {
+ c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[0]})
+ }
+ c.AdvanceClock(t)
+ records = c.ReadRetransmit()
+ }
+ }),
},
},
shimCertificate: &rsaChainCertificate,
@@ -12000,6 +12073,15 @@
}
c.WriteFlight(next)
},
+ ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ for _, t := range useTimeouts[:len(useTimeouts)-1] {
+ if len(records) > 0 {
+ c.WriteACK(c.OutEpoch(), []DTLSRecordNumberInfo{records[0]})
+ }
+ c.AdvanceClock(t)
+ records = c.ReadRetransmit()
+ }
+ }),
},
},
shimCertificate: &rsaChainCertificate,
@@ -12027,6 +12109,17 @@
}
c.WriteFlight(next)
},
+ ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ // Keep ACKing the same record over and over.
+ c.WriteACK(c.OutEpoch(), records[:1])
+ c.AdvanceClock(useTimeouts[0])
+ c.ReadRetransmit()
+ c.WriteACK(c.OutEpoch(), records[:1])
+ c.AdvanceClock(useTimeouts[1])
+ c.ReadRetransmit()
+ // ACK everything to clear the timer.
+ c.WriteACK(c.OutEpoch(), records)
+ }),
},
},
flags: flags,
@@ -12034,7 +12127,7 @@
// When ACKing ServerHello..Finished, the ServerHello might be
// ACKed at epoch 0 or epoch 2, depending on how far the client
- // received. Test tha epoch 0 is allowed by ACKing each packet
+ // received. Test that epoch 0 is allowed by ACKing each packet
// at the record it was received.
testCases = append(testCases, testCase{
protocol: dtls,
@@ -12060,8 +12153,8 @@
flags: flags,
})
- // However, records may not be ACKed at lower epoch than they
- // were received.
+ // However, records in the handshake may not be ACKed at lower
+ // epoch than they were received.
testCases = append(testCases, testCase{
protocol: dtls,
testType: serverTest,
@@ -12331,6 +12424,34 @@
expectedError: ":READ_TIMEOUT_EXPIRED:",
})
+ // If the server never receives an ACK for NewSessionTicket, it
+ // is eventually fatal.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ protocol: dtls,
+ name: "DTLS-Retransmit-Server-NewSessionTicketTimeout" + suffix,
+ config: Config{
+ MaxVersion: vers.version,
+ Bugs: ProtocolBugs{
+ ACKFlightDTLS: handleNewSessionTicket(func(c *DTLSController, prev, received []DTLSMessage, records []DTLSRecordNumberInfo) {
+ if received[0].Type != typeNewSessionTicket {
+ c.WriteACK(c.OutEpoch(), records)
+ return
+ }
+ // Time the peer out.
+ for _, t := range useTimeouts[:len(useTimeouts)-1] {
+ c.AdvanceClock(t)
+ c.ReadRetransmit()
+ }
+ c.AdvanceClock(useTimeouts[len(useTimeouts)-1])
+ }),
+ },
+ },
+ flags: flags,
+ shouldFail: true,
+ expectedError: ":READ_TIMEOUT_EXPIRED:",
+ })
+
// If generating the reply to a flight takes time (generating a
// CertificateVerify for a client certificate), the shim should
// send an ACK.
