[go] crypto/tls: buffer handshake messages.

[Adam Langley (Gerrit)]

Adam Langley uploaded a change:
https://go-review.googlesource.com/23609

crypto/tls: buffer handshake messages.

This change causes TLS handshake messages to be buffered and written in
a single Write to the underlying net.Conn.

There are two reasons to want to do this:

Firstly, it's slightly preferable to do this in order to save sending
several, small packets over the network where a single one will do.

Secondly, since 37c28759ca46cf381a466e32168a793165d9c9e9 errors from
Write have been returned from a handshake. This means that, if a peer
closes the connection during a handshake, a “broken pipe” error may
result from tls.Conn.Handshake(). This can mask any, more detailed,
fatal alerts that the peer may have sent because a read will never
happen.

Buffering handshake messages means that the peer will not receive, and
possibly reject, any of a flow while it's still being written.

Fixes #15709

Change-Id: I38dcff1abecc06e52b2de647ea98713ce0fb9a21
---
M src/crypto/tls/conn.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go
4 files changed, 51 insertions(+), 7 deletions(-)



diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go
index 40c1744..87bef23 100644
--- a/src/crypto/tls/conn.go
+++ b/src/crypto/tls/conn.go
@@ -71,10 +71,12 @@
clientProtocolFallback bool

// input/output
- in, out halfConn // in.Mutex < out.Mutex
- rawInput *block // raw input, right off the wire
- input *block // application data waiting to be read
- hand bytes.Buffer // handshake data waiting to be read
+ in, out halfConn // in.Mutex < out.Mutex
+ rawInput *block // raw input, right off the wire
+ input *block // application data waiting to be read
+ hand bytes.Buffer // handshake data waiting to be read
+ buffering bool // whether records are buffered in sendBuf
+ sendBuf []byte // a buffer of records waiting to be sent

// bytesSent counts the bytes of application data sent.
// packetsSent counts packets.
@@ -803,6 +805,30 @@
return n
}

+// c.out.Mutex <= L.
+func (c *Conn) write(data []byte) (int, error) {
+ if c.buffering {
+ c.sendBuf = append(c.sendBuf, data...)
+ return len(data), nil
+ }
+
+ n, err := c.conn.Write(data)
+ c.bytesSent += int64(n)
+ return n, err
+}
+
+func (c *Conn) flush() (int, error) {
+ if len(c.sendBuf) == 0 {
+ return 0, nil
+ }
+
+ n, err := c.conn.Write(c.sendBuf)
+ c.bytesSent += int64(n)
+ c.sendBuf = nil
+ c.buffering = false
+ return n, err
+}
+
// writeRecordLocked writes a TLS record with the given type and payload
to the
// connection and updates the record layer state.
// c.out.Mutex <= L.
@@ -862,10 +888,9 @@
}
copy(b.data[recordHeaderLen+explicitIVLen:], data)
c.out.encrypt(b, explicitIVLen)
- if _, err := c.conn.Write(b.data); err != nil {
+ if _, err := c.write(b.data); err != nil {
return n, err
}
- c.bytesSent += int64(m)
n += m
data = data[m:]
}
diff --git a/src/crypto/tls/handshake_client.go
b/src/crypto/tls/handshake_client.go
index 475737b..f789e6f 100644
--- a/src/crypto/tls/handshake_client.go
+++ b/src/crypto/tls/handshake_client.go
@@ -206,6 +206,7 @@
hs.finishedHash.Write(hs.hello.marshal())
hs.finishedHash.Write(hs.serverHello.marshal())

+ c.buffering = true
if isResume {
if err := hs.establishKeys(); err != nil {
return err
@@ -220,6 +221,9 @@
if err := hs.sendFinished(c.clientFinished[:]); err != nil {
return err
}
+ if _, err := c.flush(); err != nil {
+ return err
+ }
} else {
if err := hs.doFullHandshake(); err != nil {
return err
@@ -230,6 +234,9 @@
if err := hs.sendFinished(c.clientFinished[:]); err != nil {
return err
}
+ if _, err := c.flush(); err != nil {
+ return err
+ }
c.clientFinishedIsFirst = true
if err := hs.readSessionTicket(); err != nil {
return err
diff --git a/src/crypto/tls/handshake_client_test.go
b/src/crypto/tls/handshake_client_test.go
index 40b0770..98be2cf 100644
--- a/src/crypto/tls/handshake_client_test.go
+++ b/src/crypto/tls/handshake_client_test.go
@@ -983,7 +983,7 @@

func TestFailedWrite(t *testing.T) {
// Test that a write error during the handshake is returned.
- for _, breakAfter := range []int{0, 1, 2, 3} {
+ for _, breakAfter := range []int{0, 1} {
c, s := net.Pipe()
done := make(chan bool)

diff --git a/src/crypto/tls/handshake_server.go
b/src/crypto/tls/handshake_server.go
index cf617df..1aac729 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -52,6 +52,7 @@
}

// For an overview of TLS handshaking, see
https://tools.ietf.org/html/rfc5246#section-7.3
+ c.buffering = true
if isResume {
// The client has included a session ticket and so we do an abbreviated
handshake.
if err := hs.doResumeHandshake(); err != nil {
@@ -69,6 +70,9 @@
}
}
if err := hs.sendFinished(c.serverFinished[:]); err != nil {
+ return err
+ }
+ if _, err := c.flush(); err != nil {
return err
}
c.clientFinishedIsFirst = false
@@ -89,10 +93,14 @@
return err
}
c.clientFinishedIsFirst = true
+ c.buffering = true
if err := hs.sendSessionTicket(); err != nil {
return err
}
if err := hs.sendFinished(nil); err != nil {
+ return err
+ }
+ if _, err := c.flush(); err != nil {
return err
}
}
@@ -430,6 +438,10 @@
return err
}

+ if _, err := c.flush(); err != nil {
+ return err
+ }
+
var pub crypto.PublicKey // public key for client auth, if any

msg, err := c.readHandshake()

--
https://go-review.googlesource.com/23609

[Andrew Gerrand (Gerrit)]

Andrew Gerrand has posted comments on this change.

crypto/tls: buffer handshake messages.

Patch Set 1: Run-TryBot+1 Code-Review+2

--
https://go-review.googlesource.com/23609
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/tls: buffer handshake messages.

Patch Set 1:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=29ff5990

--
https://go-review.googlesource.com/23609
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-HasComments: No

[Adam Langley (Gerrit)]

Reviewers: Andrew Gerrand

Adam Langley uploaded a new patch set:

https://go-review.googlesource.com/23609

crypto/tls: buffer handshake messages.

This change causes TLS handshake messages to be buffered and written in
a single Write to the underlying net.Conn.

There are two reasons to want to do this:

Firstly, it's slightly preferable to do this in order to save sending
several, small packets over the network where a single one will do.

Secondly, since 37c28759ca46cf381a466e32168a793165d9c9e9 errors from
Write have been returned from a handshake. This means that, if a peer
closes the connection during a handshake, a “broken pipe” error may
result from tls.Conn.Handshake(). This can mask any, more detailed,
fatal alerts that the peer may have sent because a read will never
happen.

Buffering handshake messages means that the peer will not receive, and
possibly reject, any of a flow while it's still being written.

Fixes #15709

Change-Id: I38dcff1abecc06e52b2de647ea98713ce0fb9a21
---
M src/crypto/tls/conn.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go

4 files changed, 93 insertions(+), 7 deletions(-)

[Andrew Gerrand (Gerrit)]

Andrew Gerrand has posted comments on this change.

crypto/tls: buffer handshake messages.

Patch Set 2: Run-TryBot+1

--
https://go-review.googlesource.com/23609
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/tls: buffer handshake messages.

Patch Set 2:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=53517ab1

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/tls: buffer handshake messages.

Patch Set 2: TryBot-Result+1

TryBots are happy.

[Andrew Gerrand (Gerrit)]

Andrew Gerrand has submitted this change and it was merged.

crypto/tls: buffer handshake messages.

This change causes TLS handshake messages to be buffered and written in
a single Write to the underlying net.Conn.

There are two reasons to want to do this:

Firstly, it's slightly preferable to do this in order to save sending
several, small packets over the network where a single one will do.

Secondly, since 37c28759ca46cf381a466e32168a793165d9c9e9 errors from
Write have been returned from a handshake. This means that, if a peer
closes the connection during a handshake, a “broken pipe” error may
result from tls.Conn.Handshake(). This can mask any, more detailed,
fatal alerts that the peer may have sent because a read will never
happen.

Buffering handshake messages means that the peer will not receive, and
possibly reject, any of a flow while it's still being written.

Fixes #15709

Change-Id: I38dcff1abecc06e52b2de647ea98713ce0fb9a21

Reviewed-on: https://go-review.googlesource.com/23609
Reviewed-by: Andrew Gerrand <a...@golang.org>
Run-TryBot: Andrew Gerrand <a...@golang.org>
TryBot-Result: Gobot Gobot <go...@golang.org>

---
M src/crypto/tls/conn.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go

4 files changed, 93 insertions(+), 7 deletions(-)

Approvals:
Andrew Gerrand: Looks good to me, approved; Run TryBots
Gobot Gobot: TryBots succeeded

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

crypto/tls: buffer handshake messages.

Patch Set 3:

(1 comment)

https://go-review.googlesource.com/#/c/23609/3//COMMIT_MSG
Commit Message:

Line 17: Secondly, since 37c28759ca46cf381a466e32168a793165d9c9e9 errors
from
I see this is committed, but, next time: s|
37c28759ca46cf381a466e32168a793165d9c9e9|http://golang.org/cl/19990|. We
can make the golang.org CL stable for all future uses, which is less true
of the git ref. Thanks.

--
https://go-review.googlesource.com/23609
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-HasComments: Yes