Test DTLS record/packet packing more aggressively.
Application records may be packed with other application data records or
with handshake records. We also were never testing CCS and handshake
being packed together. Implement this by moving the packing logic to the
bottom of BoGo's DTLS record layer.
Change-Id: Iabc14ec4ce7b99ed1f923ce9164077efe948c7a0
Reviewed-on: https://boringssl-review.googlesource.com/21844
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>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index c36b503..068cf71 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -952,10 +952,22 @@
// record size.
PackHandshakeFragments int
- // PackHandshakeRecords, if true, causes handshake records in DTLS to be
- // packed into individual packets, up to the specified packet size.
+ // PackHandshakeRecords, if non-zero, causes handshake and
+ // ChangeCipherSpec records in DTLS to be packed into individual
+ // packets, up to the specified packet size.
PackHandshakeRecords int
+ // PackAppDataWithHandshake, if true, extends PackHandshakeRecords to
+ // additionally include the first application data record sent after the
+ // final Finished message in a handshake. (If the final Finished message
+ // is sent by the peer, this option has no effect.) This requires that
+ // the runner rather than shim speak first in a given test.
+ PackAppDataWithHandshake bool
+
+ // SplitAndPackAppData, if true, causes application data in DTLS to be
+ // split into two records each and packed into one packet.
+ SplitAndPackAppData bool
+
// PackHandshakeFlight, if true, causes each handshake flight in TLS to
// be packed into records, up to the largest size record available.
PackHandshakeFlight bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index ea33811..0edbe5c 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -96,6 +96,7 @@
handMsg []byte // pending assembled handshake message
handMsgLen int // handshake message length, not including the header
pendingFragments [][]byte // pending outgoing handshake fragments.
+ pendingPacket []byte // pending outgoing packet.
keyUpdateRequested bool
seenOneByteRecord bool
@@ -1400,10 +1401,6 @@
c.out.Lock()
defer c.out.Unlock()
- // Flush any pending handshake data. PackHelloRequestWithFinished may
- // have been set and the handshake not followed by Renegotiate.
- c.flushHandshake()
-
if err := c.out.err; err != nil {
return 0, err
}
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index bd111e0..b97f829 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -129,25 +129,37 @@
}
func (c *Conn) dtlsWriteRecord(typ recordType, data []byte) (n int, err error) {
+ // Only handshake messages are fragmented.
if typ != recordTypeHandshake {
reorder := typ == recordTypeChangeCipherSpec && c.config.Bugs.ReorderChangeCipherSpec
- // Flush pending handshake messages before writing a new record.
+ // Flush pending handshake messages before encrypting a new record.
if !reorder {
- err = c.dtlsFlushHandshake()
+ err = c.dtlsPackHandshake()
if err != nil {
return
}
}
- // Only handshake messages are fragmented.
- n, err = c.dtlsWriteRawRecord(typ, data)
- if err != nil {
- return
+ if typ == recordTypeApplicationData && len(data) > 1 && c.config.Bugs.SplitAndPackAppData {
+ _, err = c.dtlsPackRecord(typ, data[:len(data)/2], false)
+ if err != nil {
+ return
+ }
+ _, err = c.dtlsPackRecord(typ, data[len(data)/2:], true)
+ if err != nil {
+ return
+ }
+ n = len(data)
+ } else {
+ n, err = c.dtlsPackRecord(typ, data, false)
+ if err != nil {
+ return
+ }
}
if reorder {
- err = c.dtlsFlushHandshake()
+ err = c.dtlsPackHandshake()
if err != nil {
return
}
@@ -158,12 +170,19 @@
if err != nil {
return n, c.sendAlertLocked(alertLevelError, err.(alert))
}
+ } else {
+ // ChangeCipherSpec is part of the handshake and not
+ // flushed until dtlsFlushPacket.
+ err = c.dtlsFlushPacket()
+ if err != nil {
+ return
+ }
}
return
}
if c.out.cipher == nil && c.config.Bugs.StrayChangeCipherSpec {
- _, err = c.dtlsWriteRawRecord(recordTypeChangeCipherSpec, []byte{1})
+ _, err = c.dtlsPackRecord(recordTypeChangeCipherSpec, []byte{1}, false)
if err != nil {
return
}
@@ -243,7 +262,9 @@
return
}
-func (c *Conn) dtlsFlushHandshake() error {
+// dtlsPackHandshake packs the pending handshake flight into the pending
+// record. Callers should follow up with dtlsFlushPacket to write the packets.
+func (c *Conn) dtlsPackHandshake() error {
// This is a test-only DTLS implementation, so there is no need to
// retain |c.pendingFragments| for a future retransmit.
var fragments [][]byte
@@ -265,7 +286,6 @@
}
maxRecordLen := c.config.Bugs.PackHandshakeFragments
- maxPacketLen := c.config.Bugs.PackHandshakeRecords
// Pack handshake fragments into records.
var records [][]byte
@@ -285,42 +305,39 @@
}
}
- // Format them into packets.
- var packets [][]byte
+ // Send the records.
for _, record := range records {
- b, err := c.dtlsSealRecord(recordTypeHandshake, record)
+ _, err := c.dtlsPackRecord(recordTypeHandshake, record, false)
if err != nil {
return err
}
-
- if i := len(packets) - 1; len(packets) > 0 && len(packets[i])+len(b.data) <= maxPacketLen {
- packets[i] = append(packets[i], b.data...)
- } else {
- // The sealed record will be appended to and reused by
- // |c.out|, so copy it.
- packets = append(packets, append([]byte{}, b.data...))
- }
- c.out.freeBlock(b)
}
- // Send all the packets.
- for _, packet := range packets {
- if _, err := c.conn.Write(packet); err != nil {
- return err
- }
- }
return nil
}
-// dtlsSealRecord seals a record into a block from |c.out|'s pool.
-func (c *Conn) dtlsSealRecord(typ recordType, data []byte) (b *block, err error) {
+func (c *Conn) dtlsFlushHandshake() error {
+ if err := c.dtlsPackHandshake(); err != nil {
+ return err
+ }
+ if err := c.dtlsFlushPacket(); err != nil {
+ return err
+ }
+
+ return nil
+}
+
+// dtlsPackRecord packs a single record to the pending packet, flushing it
+// if necessary. The caller should call dtlsFlushPacket to flush the current
+// pending packet afterwards.
+func (c *Conn) dtlsPackRecord(typ recordType, data []byte, mustPack bool) (n int, err error) {
recordHeaderLen := dtlsRecordHeaderLen
maxLen := c.config.Bugs.MaxHandshakeRecordLength
if maxLen <= 0 {
maxLen = 1024
}
- b = c.out.newBlock()
+ b := c.out.newBlock()
explicitIVLen := 0
explicitIVIsSeq := false
@@ -372,23 +389,30 @@
}
copy(b.data[recordHeaderLen+explicitIVLen:], data)
c.out.encrypt(b, explicitIVLen, typ)
+
+ // Flush the current pending packet if necessary.
+ if !mustPack && len(b.data)+len(c.pendingPacket) > c.config.Bugs.PackHandshakeRecords {
+ err = c.dtlsFlushPacket()
+ if err != nil {
+ c.out.freeBlock(b)
+ return
+ }
+ }
+
+ // Add the record to the pending packet.
+ c.pendingPacket = append(c.pendingPacket, b.data...)
+ c.out.freeBlock(b)
+ n = len(data)
return
}
-func (c *Conn) dtlsWriteRawRecord(typ recordType, data []byte) (n int, err error) {
- b, err := c.dtlsSealRecord(typ, data)
- if err != nil {
- return
+func (c *Conn) dtlsFlushPacket() error {
+ if len(c.pendingPacket) == 0 {
+ return nil
}
-
- _, err = c.conn.Write(b.data)
- if err != nil {
- return
- }
- n = len(data)
-
- c.out.freeBlock(b)
- return
+ _, err := c.conn.Write(c.pendingPacket)
+ c.pendingPacket = nil
+ return err
}
func (c *Conn) dtlsDoReadHandshake() ([]byte, error) {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 0c23793..ef6a464 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -1622,7 +1622,9 @@
}
}
- c.flushHandshake()
+ if !isResume || !c.config.Bugs.PackAppDataWithHandshake {
+ c.flushHandshake()
+ }
return nil
}
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index f67cc94..0ffb72c 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -80,7 +80,7 @@
return err
}
}
- if err := hs.sendFinished(c.firstFinished[:]); err != nil {
+ if err := hs.sendFinished(c.firstFinished[:], isResume); err != nil {
return err
}
// Most retransmits are triggered by a timeout, but the final
@@ -120,7 +120,7 @@
if err := hs.sendSessionTicket(); err != nil {
return err
}
- if err := hs.sendFinished(nil); err != nil {
+ if err := hs.sendFinished(nil, isResume); err != nil {
return err
}
}
@@ -1800,7 +1800,7 @@
return nil
}
-func (hs *serverHandshakeState) sendFinished(out []byte) error {
+func (hs *serverHandshakeState) sendFinished(out []byte, isResume bool) error {
c := hs.c
finished := new(finishedMsg)
@@ -1845,8 +1845,8 @@
}
}
- if !c.config.Bugs.PackHelloRequestWithFinished {
- // Defer flushing until renegotiation.
+ if isResume || (!c.config.Bugs.PackHelloRequestWithFinished && !c.config.Bugs.PackAppDataWithHandshake) {
+ // Defer flushing until Renegotiate() or Write().
c.flushHandshake()
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index afa8bd1..9ec0d80 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -837,6 +837,13 @@
if err != nil {
return err
}
+ if config.Bugs.SplitAndPackAppData {
+ m, err := tlsConn.Read(bufTmp[n:])
+ if err != nil {
+ return err
+ }
+ n += m
+ }
if n != len(buf) {
return fmt.Errorf("bad reply; length mismatch (%d vs %d)", n, len(buf))
}
@@ -2293,17 +2300,6 @@
flags: []string{"-async"},
},
{
- protocol: dtls,
- name: "PackDTLSHandshake",
- config: Config{
- Bugs: ProtocolBugs{
- MaxHandshakeRecordLength: 2,
- PackHandshakeFragments: 20,
- PackHandshakeRecords: 200,
- },
- },
- },
- {
name: "SendEmptyRecords-Pass",
sendEmptyRecords: 32,
},
@@ -2796,6 +2792,27 @@
shouldFail: true,
expectedLocalError: "local error: record overflow",
},
+ {
+ // Test that DTLS can handle multiple application data
+ // records in a single packet.
+ protocol: dtls,
+ name: "SplitAndPackAppData-DTLS",
+ config: Config{
+ Bugs: ProtocolBugs{
+ SplitAndPackAppData: true,
+ },
+ },
+ },
+ {
+ protocol: dtls,
+ name: "SplitAndPackAppData-DTLS-Async",
+ config: Config{
+ Bugs: ProtocolBugs{
+ SplitAndPackAppData: true,
+ },
+ },
+ flags: []string{"-async"},
+ },
}
testCases = append(testCases, basicTests...)
@@ -3931,11 +3948,11 @@
}
type stateMachineTestConfig struct {
- protocol protocol
- async bool
- splitHandshake bool
- packHandshakeFlight bool
- implicitHandshake bool
+ protocol protocol
+ async bool
+ splitHandshake bool
+ packHandshake bool
+ implicitHandshake bool
}
// Adds tests that try to cover the range of the handshake state machine, under
@@ -3958,13 +3975,11 @@
async: async,
splitHandshake: true,
})
- if protocol == tls {
- addStateMachineCoverageTests(stateMachineTestConfig{
- protocol: protocol,
- async: async,
- packHandshakeFlight: true,
- })
- }
+ addStateMachineCoverageTests(stateMachineTestConfig{
+ protocol: protocol,
+ async: async,
+ packHandshake: true,
+ })
}
}
}
@@ -4603,7 +4618,7 @@
config: Config{
MaxVersion: VersionTLS12,
Bugs: ProtocolBugs{
- PackHelloRequestWithFinished: config.packHandshakeFlight,
+ PackHelloRequestWithFinished: config.packHandshake,
},
},
sendHalfHelloRequest: true,
@@ -4913,9 +4928,16 @@
test.flags = append(test.flags, "-mtu", "256")
}
}
- if config.packHandshakeFlight {
- test.name += "-PackHandshakeFlight"
- test.config.Bugs.PackHandshakeFlight = true
+ if config.packHandshake {
+ test.name += "-PackHandshake"
+ if config.protocol == dtls {
+ test.config.Bugs.MaxHandshakeRecordLength = 2
+ test.config.Bugs.PackHandshakeFragments = 20
+ test.config.Bugs.PackHandshakeRecords = 1500
+ test.config.Bugs.PackAppDataWithHandshake = true
+ } else {
+ test.config.Bugs.PackHandshakeFlight = true
+ }
}
if config.implicitHandshake {
test.name += "-ImplicitHandshake"
