Add tests for packed handshake records in TLS.
I'm surprised we'd never tested this. In addition to splitting handshake
records up, one may pack multiple handshakes into a single record, as
they fit. Generalize the DTLS handshake flush hook to do this in TLS as
well.
Change-Id: Ia546d18c7c56ba45e50f489c5b53e1fcd6404f51
Reviewed-on: https://boringssl-review.googlesource.com/8650
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 0e5d9e4..eabd0c6 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -767,15 +767,19 @@
// Diffie-Hellman group. The generator used is always two.
DHGroupPrime *big.Int
- // PackHandshakeFragments, if true, causes handshake fragments to be
- // packed into individual handshake records, up to the specified record
- // size.
+ // PackHandshakeFragments, if true, causes handshake fragments in DTLS
+ // to be packed into individual handshake records, up to the specified
+ // record size.
PackHandshakeFragments int
- // PackHandshakeRecords, if true, causes handshake records to be packed
- // into individual packets, up to the specified packet size.
+ // PackHandshakeRecords, if true, causes handshake records in DTLS to be
+ // packed into individual packets, up to the specified packet size.
PackHandshakeRecords int
+ // PackHandshakeFlight, if true, causes each handshake flight in TLS to
+ // be packed into records, up to the largest size record available.
+ PackHandshakeFlight bool
+
// EnableAllCiphers, if true, causes all configured ciphers to be
// enabled.
EnableAllCiphers bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index 551c6bc..c33ac0c 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -77,6 +77,10 @@
input *block // application record waiting to be read
hand bytes.Buffer // handshake record waiting to be read
+ // pendingFlight, if PackHandshakeFlight is enabled, is the buffer of
+ // handshake data to be split into records at the end of the flight.
+ pendingFlight bytes.Buffer
+
// DTLS state
sendHandshakeSeq uint16
recvHandshakeSeq uint16
@@ -934,6 +938,15 @@
return c.dtlsWriteRecord(typ, data)
}
+ if c.config.Bugs.PackHandshakeFlight && typ == recordTypeHandshake {
+ c.pendingFlight.Write(data)
+ return len(data), nil
+ }
+
+ return c.doWriteRecord(typ, data)
+}
+
+func (c *Conn) doWriteRecord(typ recordType, data []byte) (n int, err error) {
recordHeaderLen := tlsRecordHeaderLen
b := c.out.newBlock()
first := true
@@ -1031,6 +1044,23 @@
return
}
+func (c *Conn) flushHandshake() error {
+ if c.isDTLS {
+ return c.dtlsFlushHandshake()
+ }
+
+ for c.pendingFlight.Len() > 0 {
+ var buf [maxPlaintext]byte
+ n, _ := c.pendingFlight.Read(buf[:])
+ if _, err := c.doWriteRecord(recordTypeHandshake, buf[:n]); err != nil {
+ return err
+ }
+ }
+
+ c.pendingFlight.Reset()
+ return nil
+}
+
func (c *Conn) doReadHandshake() ([]byte, error) {
if c.isDTLS {
return c.dtlsDoReadHandshake()
@@ -1217,6 +1247,7 @@
if c.config.Bugs.SendHelloRequestBeforeEveryAppDataRecord {
c.writeRecord(recordTypeHandshake, []byte{typeHelloRequest, 0, 0, 0})
+ c.flushHandshake()
}
// SSL 3.0 and TLS 1.0 are susceptible to a chosen-plaintext
@@ -1269,6 +1300,7 @@
helloReq = c.config.Bugs.BadHelloRequest
}
c.writeRecord(recordTypeHandshake, helloReq)
+ c.flushHandshake()
}
c.handshakeComplete = false
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index f4c5f71..48b8a1b 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -242,10 +242,6 @@
}
func (c *Conn) dtlsFlushHandshake() error {
- if !c.isDTLS {
- return nil
- }
-
// This is a test-only DTLS implementation, so there is no need to
// retain |c.pendingFragments| for a future retransmit.
var fragments [][]byte
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 8e8f7d7..a2f6f65 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -225,7 +225,7 @@
helloBytes = hello.marshal()
c.writeRecord(recordTypeHandshake, helloBytes)
}
- c.dtlsFlushHandshake()
+ c.flushHandshake()
if err := c.simulatePacketLoss(nil); err != nil {
return err
@@ -249,7 +249,7 @@
hello.cookie = helloVerifyRequest.cookie
helloBytes = hello.marshal()
c.writeRecord(recordTypeHandshake, helloBytes)
- c.dtlsFlushHandshake()
+ c.flushHandshake()
if err := c.simulatePacketLoss(nil); err != nil {
return err
@@ -342,7 +342,7 @@
// Finished.
if err := c.simulatePacketLoss(func() {
c.writeRecord(recordTypeHandshake, hs.finishedBytes)
- c.dtlsFlushHandshake()
+ c.flushHandshake()
}); err != nil {
return err
}
@@ -549,7 +549,7 @@
hs.writeClientHash(certVerify.marshal())
c.writeRecord(recordTypeHandshake, certVerify.marshal())
}
- c.dtlsFlushHandshake()
+ c.flushHandshake()
hs.finishedHash.discardHandshakeBuffer()
@@ -893,7 +893,7 @@
c.writeRecord(recordTypeHandshake, postCCSBytes[:5])
postCCSBytes = postCCSBytes[5:]
}
- c.dtlsFlushHandshake()
+ c.flushHandshake()
if !c.config.Bugs.SkipChangeCipherSpec &&
c.config.Bugs.EarlyChangeCipherSpec == 0 {
@@ -914,7 +914,7 @@
if !c.config.Bugs.SkipFinished {
c.writeRecord(recordTypeHandshake, postCCSBytes)
- c.dtlsFlushHandshake()
+ c.flushHandshake()
}
return nil
}
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 94f5923..771beea 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -76,7 +76,7 @@
// Finished.
if err := c.simulatePacketLoss(func() {
c.writeRecord(recordTypeHandshake, hs.finishedBytes)
- c.dtlsFlushHandshake()
+ c.flushHandshake()
}); err != nil {
return err
}
@@ -154,7 +154,7 @@
return false, errors.New("dtls: short read from Rand: " + err.Error())
}
c.writeRecord(recordTypeHandshake, helloVerifyRequest.marshal())
- c.dtlsFlushHandshake()
+ c.flushHandshake()
if err := c.simulatePacketLoss(nil); err != nil {
return false, err
@@ -614,7 +614,7 @@
helloDone := new(serverHelloDoneMsg)
hs.writeServerHash(helloDone.marshal())
c.writeRecord(recordTypeHandshake, helloDone.marshal())
- c.dtlsFlushHandshake()
+ c.flushHandshake()
var pub crypto.PublicKey // public key for client auth, if any
@@ -894,7 +894,7 @@
c.writeRecord(recordTypeHandshake, postCCSBytes[:5])
postCCSBytes = postCCSBytes[5:]
}
- c.dtlsFlushHandshake()
+ c.flushHandshake()
if !c.config.Bugs.SkipChangeCipherSpec {
ccs := []byte{1}
@@ -914,7 +914,7 @@
if !c.config.Bugs.SkipFinished {
c.writeRecord(recordTypeHandshake, postCCSBytes)
- c.dtlsFlushHandshake()
+ c.flushHandshake()
}
c.cipherSuite = hs.suite
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 7596485..b8f7088 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2918,10 +2918,39 @@
}
}
+type stateMachineTestConfig struct {
+ protocol protocol
+ async bool
+ splitHandshake, packHandshakeFlight bool
+}
+
// Adds tests that try to cover the range of the handshake state machine, under
// various conditions. Some of these are redundant with other tests, but they
// only cover the synchronous case.
-func addStateMachineCoverageTests(async, splitHandshake bool, protocol protocol) {
+func addAllStateMachineCoverageTests() {
+ for _, async := range []bool{false, true} {
+ for _, protocol := range []protocol{tls, dtls} {
+ addStateMachineCoverageTests(stateMachineTestConfig{
+ protocol: protocol,
+ async: async,
+ })
+ addStateMachineCoverageTests(stateMachineTestConfig{
+ protocol: protocol,
+ async: async,
+ splitHandshake: true,
+ })
+ if protocol == tls {
+ addStateMachineCoverageTests(stateMachineTestConfig{
+ protocol: protocol,
+ async: async,
+ packHandshakeFlight: true,
+ })
+ }
+ }
+ }
+}
+
+func addStateMachineCoverageTests(config stateMachineTestConfig) {
var tests []testCase
// Basic handshake, with resumption. Client and server,
@@ -3024,7 +3053,7 @@
// Setting SSL_VERIFY_PEER allows anonymous clients.
flags: []string{"-verify-peer"},
})
- if protocol == tls {
+ if config.protocol == tls {
tests = append(tests, testCase{
testType: clientTest,
name: "ClientAuth-NoCertificate-Client-SSL3",
@@ -3247,7 +3276,7 @@
},
})
- if protocol == tls {
+ if config.protocol == tls {
tests = append(tests, testCase{
name: "Renegotiate-Client",
config: Config{
@@ -3486,24 +3515,28 @@
}
for _, test := range tests {
- test.protocol = protocol
- if protocol == dtls {
+ test.protocol = config.protocol
+ if config.protocol == dtls {
test.name += "-DTLS"
}
- if async {
+ if config.async {
test.name += "-Async"
test.flags = append(test.flags, "-async")
} else {
test.name += "-Sync"
}
- if splitHandshake {
+ if config.splitHandshake {
test.name += "-SplitHandshakeRecords"
test.config.Bugs.MaxHandshakeRecordLength = 1
- if protocol == dtls {
+ if config.protocol == dtls {
test.config.Bugs.MaxPacketLength = 256
test.flags = append(test.flags, "-mtu", "256")
}
}
+ if config.packHandshakeFlight {
+ test.name += "-PackHandshakeFlight"
+ test.config.Bugs.PackHandshakeFlight = true
+ }
testCases = append(testCases, test)
}
}
@@ -5856,13 +5889,7 @@
addCECPQ1Tests()
addKeyExchangeInfoTests()
addTLS13RecordTests()
- for _, async := range []bool{false, true} {
- for _, splitHandshake := range []bool{false, true} {
- for _, protocol := range []protocol{tls, dtls} {
- addStateMachineCoverageTests(async, splitHandshake, protocol)
- }
- }
- }
+ addAllStateMachineCoverageTests()
var wg sync.WaitGroup
