Add tests for bidirectional shutdown.
Now that it even works at all (type = 0 bug aside), add tests for it.
Test both close_notify being received before and after SSL_shutdown is
called. In the latter case, have the peer send some junk to be ignored
to test that works.
Also test that SSL_shutdown fails on unclean shutdown and that quiet
shutdowns ignore it.
BUG=526437
Change-Id: Iff13b08feb03e82f21ecab0c66d5f85aec256137
Reviewed-on: https://boringssl-review.googlesource.com/5769
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index f841293..2b7e29b 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -753,6 +753,15 @@
// ExpectedCustomExtension, if not nil, contains the expected contents
// of a custom extension.
ExpectedCustomExtension *string
+
+ // NoCloseNotify, if true, causes the close_notify alert to be skipped
+ // on connection shutdown.
+ NoCloseNotify bool
+
+ // ExpectCloseNotify, if true, requires a close_notify from the peer on
+ // shutdown. Records from the peer received after close_notify is sent
+ // are not discard.
+ ExpectCloseNotify bool
}
func (c *Config) serverInit() {
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index f09cb7c..42bc840 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -648,10 +648,10 @@
if err := b.readFromUntil(c.conn, recordHeaderLen); err != nil {
// RFC suggests that EOF without an alertCloseNotify is
// an error, but popular web sites seem to do this,
- // so we can't make it an error.
- // if err == io.EOF {
- // err = io.ErrUnexpectedEOF
- // }
+ // so we can't make it an error, outside of tests.
+ if err == io.EOF && c.config.Bugs.ExpectCloseNotify {
+ err = io.ErrUnexpectedEOF
+ }
if e, ok := err.(net.Error); !ok || !e.Temporary() {
c.in.setErrorLocked(err)
}
@@ -740,6 +740,10 @@
c.sendAlert(alertInternalError)
return c.in.setErrorLocked(errors.New("tls: application data record requested before handshake complete"))
}
+ case recordTypeAlert:
+ // Looking for a close_notify. Note: unlike a real
+ // implementation, this is not tolerant of additional records.
+ // See the documentation for ExpectCloseNotify.
}
Again:
@@ -802,7 +806,7 @@
// A client might need to process a HelloRequest from
// the server, thus receiving a handshake message when
// application data is expected is ok.
- if !c.isClient {
+ if !c.isClient || want != recordTypeApplicationData {
return c.in.setErrorLocked(c.sendAlert(alertNoRenegotiation))
}
}
@@ -1260,10 +1264,22 @@
c.handshakeMutex.Lock()
defer c.handshakeMutex.Unlock()
- if c.handshakeComplete {
+ if c.handshakeComplete && !c.config.Bugs.NoCloseNotify {
alertErr = c.sendAlert(alertCloseNotify)
}
+ // Consume a close_notify from the peer if one hasn't been received
+ // already. This avoids the peer from failing |SSL_shutdown| due to a
+ // write failing.
+ if c.handshakeComplete && alertErr == nil && c.config.Bugs.ExpectCloseNotify {
+ for c.in.error() == nil {
+ c.readRecord(recordTypeAlert)
+ }
+ if c.in.error() != io.EOF {
+ alertErr = c.in.error()
+ }
+ }
+
if err := c.conn.Close(); err != nil {
return err
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 950c02a..7ada5f1 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -191,6 +191,10 @@
// shimWritesFirst controls whether the shim sends an initial "hello"
// message before doing a roundtrip with the runner.
shimWritesFirst bool
+ // shimShutsDown, if true, runs a test where the shim shuts down the
+ // connection immediately after the handshake rather than echoing
+ // messages from the runner.
+ shimShutsDown bool
// renegotiate indicates the the connection should be renegotiated
// during the exchange.
renegotiate bool
@@ -270,6 +274,7 @@
tlsConn = Client(conn, config)
}
}
+ defer tlsConn.Close()
if err := tlsConn.Handshake(); err != nil {
return err
@@ -420,6 +425,11 @@
tlsConn.SendAlert(alertLevelWarning, alertUnexpectedMessage)
}
+ if test.shimShutsDown {
+ // The shim will not respond.
+ continue
+ }
+
buf := make([]byte, len(testMessage))
if test.protocol == dtls {
bufTmp := make([]byte, len(buf)+1)
@@ -547,6 +557,10 @@
flags = append(flags, "-shim-writes-first")
}
+ if test.shimShutsDown {
+ flags = append(flags, "-shim-shuts-down")
+ }
+
if test.exportKeyingMaterial > 0 {
flags = append(flags, "-export-keying-material", strconv.Itoa(test.exportKeyingMaterial))
flags = append(flags, "-export-label", test.exportLabel)
@@ -1847,8 +1861,29 @@
noSessionCache: true,
flags: []string{"-expect-no-session"},
},
+ {
+ name: "Unclean-Shutdown",
+ config: Config{
+ Bugs: ProtocolBugs{
+ NoCloseNotify: true,
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ flags: []string{"-check-close-notify"},
+ shouldFail: true,
+ expectedError: "Unexpected SSL_shutdown result: -1 != 1",
+ },
+ {
+ name: "Unclean-Shutdown-Ignored",
+ config: Config{
+ Bugs: ProtocolBugs{
+ NoCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ },
}
-
testCases = append(testCases, basicTests...)
}
@@ -2561,6 +2596,33 @@
resumeSession: true,
expectChannelID: true,
})
+
+ // Bidirectional shutdown with the runner initiating.
+ tests = append(tests, testCase{
+ name: "Shutdown-Runner",
+ config: Config{
+ Bugs: ProtocolBugs{
+ ExpectCloseNotify: true,
+ },
+ },
+ flags: []string{"-check-close-notify"},
+ })
+
+ // Bidirectional shutdown with the shim initiating. The runner,
+ // in the meantime, sends garbage before the close_notify which
+ // the shim must ignore.
+ tests = append(tests, testCase{
+ name: "Shutdown-Shim",
+ config: Config{
+ Bugs: ProtocolBugs{
+ ExpectCloseNotify: true,
+ },
+ },
+ shimShutsDown: true,
+ sendEmptyRecords: 1,
+ sendWarningAlerts: 1,
+ flags: []string{"-check-close-notify"},
+ })
} else {
tests = append(tests, testCase{
name: "SkipHelloVerifyRequest",
