x/crypto/ssh: return msgNewKeys for a short-circuited first kex.

If one of both sides is slow, the first kex completes implicitly. The
first kex also produces msgNewKeys, and this must be read to ensure
that the authentication code is not confused by it.

Before this fix, the problem could be reproduced by inserting a sleep
just before the requestInitialKeyChange call.

Fixes #15198

Change-Id: I602db5dd37b2d8556c88ab4cdb693ccf90147a3d
Reviewed-on: https://go-review.googlesource.com/23137
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

ssh/handshake.go

@@ -174,15 +174,7 @@ func (t *handshakeTransport) readOnePacket() ([]byte, error) {
 
 	t.mu.Lock()
 
-	// By default, a key exchange is hidden from higher layers by
-	// translating it into msgIgnore.
-	successPacket := []byte{msgIgnore}
-	if t.sessionID == nil {
-		// sendKexInit() for the first kex waits for
-		// msgNewKeys so the authentication process is
-		// guaranteed to happen over an encrypted transport.
-		successPacket = []byte{msgNewKeys}
-	}
+	firstKex := t.sessionID == nil
 
 	err = t.enterKeyExchangeLocked(p)
 	if err != nil {
@@ -192,7 +184,7 @@ func (t *handshakeTransport) readOnePacket() ([]byte, error) {
 	}
 
 	if debugHandshake {
-		log.Printf("%s exited key exchange, err %v", t.id(), err)
+		log.Printf("%s exited key exchange (first %v), err %v", t.id(), firstKex, err)
 	}
 
 	// Unblock writers.
@@ -207,6 +199,17 @@ func (t *handshakeTransport) readOnePacket() ([]byte, error) {
 	}
 
 	t.readSinceKex = 0
+
+	// By default, a key exchange is hidden from higher layers by
+	// translating it into msgIgnore.
+	successPacket := []byte{msgIgnore}
+	if firstKex {
+		// sendKexInit() for the first kex waits for
+		// msgNewKeys so the authentication process is
+		// guaranteed to happen over an encrypted transport.
+		successPacket = []byte{msgNewKeys}
+	}
+
 	return successPacket, nil
 }
 
@@ -225,16 +228,15 @@ const (
 // close the underlying transport. This function is safe for
 // concurrent use by multiple goroutines.
 func (t *handshakeTransport) sendKexInit(isFirst keyChangeCategory) error {
+	var err error
+
 	t.mu.Lock()
 	// If this is the initial key change, but we already have a sessionID,
 	// then do nothing because the key exchange has already completed
 	// asynchronously.
-	if isFirst && t.sessionID != nil {
-		t.mu.Unlock()
-		return nil
+	if !isFirst || t.sessionID == nil {
+		_, _, err = t.sendKexInitLocked(isFirst)
 	}
-
-	_, _, err := t.sendKexInitLocked(isFirst)
 	t.mu.Unlock()
 	if err != nil {
 		return err