crypto/tls: revert "add HandshakeContext method to Conn"

This reverts CL 246338.

Reason for revert: waiting for 1.17 release cycle

Updates #32406

Change-Id: I074379039041e086c62271d689b4b7f442281663
Reviewed-on: https://go-review.googlesource.com/c/go/+/269697
Run-TryBot: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>

common.go

@@ -7,7 +7,6 @@ package tls
 import (
 	"bytes"
 	"container/list"
-	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"crypto/ed25519"
@@ -444,16 +443,6 @@ type ClientHelloInfo struct {
 	// config is embedded by the GetCertificate or GetConfigForClient caller,
 	// for use with SupportsCertificate.
 	config *Config
-
-	// ctx is the context of the handshake that is in progress.
-	ctx context.Context
-}
-
-// Context returns the context of the handshake that is in progress.
-// This context is a child of the context passed to HandshakeContext,
-// if any, and is canceled when the handshake concludes.
-func (c *ClientHelloInfo) Context() context.Context {
-	return c.ctx
 }
 
 // CertificateRequestInfo contains information from a server's
@@ -472,16 +461,6 @@ type CertificateRequestInfo struct {
 
 	// Version is the TLS version that was negotiated for this connection.
 	Version uint16
-
-	// ctx is the context of the handshake that is in progress.
-	ctx context.Context
-}
-
-// Context returns the context of the handshake that is in progress.
-// This context is a child of the context passed to HandshakeContext,
-// if any, and is canceled when the handshake concludes.
-func (c *CertificateRequestInfo) Context() context.Context {
-	return c.ctx
 }
 
 // RenegotiationSupport enumerates the different levels of support for TLS

conn.go

@@ -8,7 +8,6 @@ package tls
 
 import (
 	"bytes"
-	"context"
 	"crypto/cipher"
 	"crypto/subtle"
 	"crypto/x509"
@@ -28,7 +27,7 @@ type Conn struct {
 	// constant
 	conn        net.Conn
 	isClient    bool
-	handshakeFn func(context.Context) error // (*Conn).clientHandshake or serverHandshake
+	handshakeFn func() error // (*Conn).clientHandshake or serverHandshake
 
 	// handshakeStatus is 1 if the connection is currently transferring
 	// application data (i.e. is not currently processing a handshake).
@@ -1191,7 +1190,7 @@ func (c *Conn) handleRenegotiation() error {
 	defer c.handshakeMutex.Unlock()
 
 	atomic.StoreUint32(&c.handshakeStatus, 0)
-	if c.handshakeErr = c.clientHandshake(context.Background()); c.handshakeErr == nil {
+	if c.handshakeErr = c.clientHandshake(); c.handshakeErr == nil {
 		c.handshakes++
 	}
 	return c.handshakeErr
@@ -1374,61 +1373,8 @@ func (c *Conn) closeNotify() error {
 // first Read or Write will call it automatically.
 //
 // For control over canceling or setting a timeout on a handshake, use
-// HandshakeContext or the Dialer's DialContext method instead.
+// the Dialer's DialContext method.
 func (c *Conn) Handshake() error {
-	return c.HandshakeContext(context.Background())
-}
-
-// HandshakeContext runs the client or server handshake
-// protocol if it has not yet been run.
-//
-// The provided Context must be non-nil. If the context is canceled before
-// the handshake is complete, the handshake is interrupted and an error is returned.
-// Once the handshake has completed, cancellation of the context will not affect the
-// connection.
-//
-// Most uses of this package need not call HandshakeContext explicitly: the
-// first Read or Write will call it automatically.
-func (c *Conn) HandshakeContext(ctx context.Context) error {
-	// Delegate to unexported method for named return
-	// without confusing documented signature.
-	return c.handshakeContext(ctx)
-}
-
-func (c *Conn) handshakeContext(ctx context.Context) (ret error) {
-	handshakeCtx, cancel := context.WithCancel(ctx)
-	// Note: defer this before starting the "interrupter" goroutine
-	// so that we can tell the difference between the input being canceled and
-	// this cancellation. In the former case, we need to close the connection.
-	defer cancel()
-
-	// Start the "interrupter" goroutine, if this context might be canceled.
-	// (The background context cannot).
-	//
-	// The interrupter goroutine waits for the input context to be done and
-	// closes the connection if this happens before the function returns.
-	if ctx.Done() != nil {
-		done := make(chan struct{})
-		interruptRes := make(chan error, 1)
-		defer func() {
-			close(done)
-			if ctxErr := <-interruptRes; ctxErr != nil {
-				// Return context error to user.
-				ret = ctxErr
-			}
-		}()
-		go func() {
-			select {
-			case <-handshakeCtx.Done():
-				// Close the connection, discarding the error
-				_ = c.conn.Close()
-				interruptRes <- handshakeCtx.Err()
-			case <-done:
-				interruptRes <- nil
-			}
-		}()
-	}
-
 	c.handshakeMutex.Lock()
 	defer c.handshakeMutex.Unlock()
 
@@ -1442,7 +1388,7 @@ func (c *Conn) handshakeContext(ctx context.Context) (ret error) {
 	c.in.Lock()
 	defer c.in.Unlock()
 
-	c.handshakeErr = c.handshakeFn(handshakeCtx)
+	c.handshakeErr = c.handshakeFn()
 	if c.handshakeErr == nil {
 		c.handshakes++
 	} else {

handshake_client.go

@@ -6,7 +6,6 @@ package tls
 
 import (
 	"bytes"
-	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"crypto/ed25519"
@@ -25,7 +24,6 @@ import (
 
 type clientHandshakeState struct {
 	c            *Conn
-	ctx          context.Context
 	serverHello  *serverHelloMsg
 	hello        *clientHelloMsg
 	suite        *cipherSuite
@@ -136,7 +134,7 @@ func (c *Conn) makeClientHello() (*clientHelloMsg, ecdheParameters, error) {
 	return hello, params, nil
 }
 
-func (c *Conn) clientHandshake(ctx context.Context) (err error) {
+func (c *Conn) clientHandshake() (err error) {
 	if c.config == nil {
 		c.config = defaultConfig()
 	}
@@ -200,7 +198,6 @@ func (c *Conn) clientHandshake(ctx context.Context) (err error) {
 	if c.vers == VersionTLS13 {
 		hs := &clientHandshakeStateTLS13{
 			c:           c,
-			ctx:         ctx,
 			serverHello: serverHello,
 			hello:       hello,
 			ecdheParams: ecdheParams,
@@ -215,7 +212,6 @@ func (c *Conn) clientHandshake(ctx context.Context) (err error) {
 
 	hs := &clientHandshakeState{
 		c:           c,
-		ctx:         ctx,
 		serverHello: serverHello,
 		hello:       hello,
 		session:     session,
@@ -544,7 +540,7 @@ func (hs *clientHandshakeState) doFullHandshake() error {
 		certRequested = true
 		hs.finishedHash.Write(certReq.marshal())
 
-		cri := certificateRequestInfoFromMsg(hs.ctx, c.vers, certReq)
+		cri := certificateRequestInfoFromMsg(c.vers, certReq)
 		if chainToSend, err = c.getClientCertificate(cri); err != nil {
 			c.sendAlert(alertInternalError)
 			return err
@@ -884,11 +880,10 @@ func (c *Conn) verifyServerCertificate(certificates [][]byte) error {
 
 // certificateRequestInfoFromMsg generates a CertificateRequestInfo from a TLS
 // <= 1.2 CertificateRequest, making an effort to fill in missing information.
-func certificateRequestInfoFromMsg(ctx context.Context, vers uint16, certReq *certificateRequestMsg) *CertificateRequestInfo {
+func certificateRequestInfoFromMsg(vers uint16, certReq *certificateRequestMsg) *CertificateRequestInfo {
 	cri := &CertificateRequestInfo{
 		AcceptableCAs: certReq.certificateAuthorities,
 		Version:       vers,
-		ctx:           ctx,
 	}
 
 	var rsaAvail, ecAvail bool

handshake_client_test.go

@@ -6,7 +6,6 @@ package tls
 
 import (
 	"bytes"
-	"context"
 	"crypto/rsa"
 	"crypto/x509"
 	"encoding/base64"
@@ -21,7 +20,6 @@ import (
 	"os/exec"
 	"path/filepath"
 	"reflect"
-	"runtime"
 	"strconv"
 	"strings"
 	"testing"
@@ -2513,37 +2511,3 @@ func testResumptionKeepsOCSPAndSCT(t *testing.T, ver uint16) {
 			serverConfig.Certificates[0].SignedCertificateTimestamps, ccs.SignedCertificateTimestamps)
 	}
 }
-
-func TestClientHandshakeContextCancellation(t *testing.T) {
-	c, s := localPipe(t)
-	serverConfig := testConfig.Clone()
-	serverErr := make(chan error, 1)
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-	go func() {
-		defer close(serverErr)
-		defer s.Close()
-		conn := Server(s, serverConfig)
-		_, err := conn.readClientHello(ctx)
-		cancel()
-		serverErr <- err
-	}()
-	cli := Client(c, testConfig)
-	err := cli.HandshakeContext(ctx)
-	if err == nil {
-		t.Fatal("Client handshake did not error when the context was canceled")
-	}
-	if err != context.Canceled {
-		t.Errorf("Unexpected client handshake error: %v", err)
-	}
-	if err := <-serverErr; err != nil {
-		t.Errorf("Unexpected server error: %v", err)
-	}
-	if runtime.GOARCH == "wasm" {
-		t.Skip("conn.Close does not error as expected when called multiple times on WASM")
-	}
-	err = cli.Close()
-	if err == nil {
-		t.Error("Client connection was not closed when the context was canceled")
-	}
-}

handshake_client_tls13.go

@@ -6,7 +6,6 @@ package tls
 
 import (
 	"bytes"
-	"context"
 	"crypto"
 	"crypto/hmac"
 	"crypto/rsa"
@@ -18,7 +17,6 @@ import (
 
 type clientHandshakeStateTLS13 struct {
 	c           *Conn
-	ctx         context.Context
 	serverHello *serverHelloMsg
 	hello       *clientHelloMsg
 	ecdheParams ecdheParameters
@@ -557,7 +555,6 @@ func (hs *clientHandshakeStateTLS13) sendClientCertificate() error {
 		AcceptableCAs:    hs.certReq.certificateAuthorities,
 		SignatureSchemes: hs.certReq.supportedSignatureAlgorithms,
 		Version:          c.vers,
-		ctx:              hs.ctx,
 	})
 	if err != nil {
 		return err

handshake_server.go

@@ -5,7 +5,6 @@
 package tls
 
 import (
-	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"crypto/ed25519"
@@ -24,7 +23,6 @@ import (
 // It's discarded once the handshake has completed.
 type serverHandshakeState struct {
 	c            *Conn
-	ctx          context.Context
 	clientHello  *clientHelloMsg
 	hello        *serverHelloMsg
 	suite        *cipherSuite
@@ -39,8 +37,8 @@ type serverHandshakeState struct {
 }
 
 // serverHandshake performs a TLS handshake as a server.
-func (c *Conn) serverHandshake(ctx context.Context) error {
+func (c *Conn) serverHandshake() error {
-	clientHello, err := c.readClientHello(ctx)
+	clientHello, err := c.readClientHello()
 	if err != nil {
 		return err
 	}
@@ -48,7 +46,6 @@ func (c *Conn) serverHandshake(ctx context.Context) error {
 	if c.vers == VersionTLS13 {
 		hs := serverHandshakeStateTLS13{
 			c:           c,
-			ctx:         ctx,
 			clientHello: clientHello,
 		}
 		return hs.handshake()
@@ -56,7 +53,6 @@ func (c *Conn) serverHandshake(ctx context.Context) error {
 
 	hs := serverHandshakeState{
 		c:           c,
-		ctx:         ctx,
 		clientHello: clientHello,
 	}
 	return hs.handshake()
@@ -128,7 +124,7 @@ func (hs *serverHandshakeState) handshake() error {
 }
 
 // readClientHello reads a ClientHello message and selects the protocol version.
-func (c *Conn) readClientHello(ctx context.Context) (*clientHelloMsg, error) {
+func (c *Conn) readClientHello() (*clientHelloMsg, error) {
 	msg, err := c.readHandshake()
 	if err != nil {
 		return nil, err
@@ -142,7 +138,7 @@ func (c *Conn) readClientHello(ctx context.Context) (*clientHelloMsg, error) {
 	var configForClient *Config
 	originalConfig := c.config
 	if c.config.GetConfigForClient != nil {
-		chi := clientHelloInfo(ctx, c, clientHello)
+		chi := clientHelloInfo(c, clientHello)
 		if configForClient, err = c.config.GetConfigForClient(chi); err != nil {
 			c.sendAlert(alertInternalError)
 			return nil, err
@@ -224,7 +220,7 @@ func (hs *serverHandshakeState) processClientHello() error {
 		}
 	}
 
-	hs.cert, err = c.config.getCertificate(clientHelloInfo(hs.ctx, c, hs.clientHello))
+	hs.cert, err = c.config.getCertificate(clientHelloInfo(c, hs.clientHello))
 	if err != nil {
 		if err == errNoCertificates {
 			c.sendAlert(alertUnrecognizedName)
@@ -832,7 +828,7 @@ func (c *Conn) processCertsFromClient(certificate Certificate) error {
 	return nil
 }
 
-func clientHelloInfo(ctx context.Context, c *Conn, clientHello *clientHelloMsg) *ClientHelloInfo {
+func clientHelloInfo(c *Conn, clientHello *clientHelloMsg) *ClientHelloInfo {
 	supportedVersions := clientHello.supportedVersions
 	if len(clientHello.supportedVersions) == 0 {
 		supportedVersions = supportedVersionsFromMax(clientHello.vers)
@@ -848,6 +844,5 @@ func clientHelloInfo(ctx context.Context, c *Conn, clientHello *clientHelloMsg)
 		SupportedVersions: supportedVersions,
 		Conn:              c.conn,
 		config:            c.config,
-		ctx:               ctx,
 	}
 }

handshake_server_test.go

@@ -6,7 +6,6 @@ package tls
 
 import (
 	"bytes"
-	"context"
 	"crypto"
 	"crypto/elliptic"
 	"crypto/x509"
@@ -18,7 +17,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -40,12 +38,10 @@ func testClientHelloFailure(t *testing.T, serverConfig *Config, m handshakeMessa
 		cli.writeRecord(recordTypeHandshake, m.marshal())
 		c.Close()
 	}()
-	ctx := context.Background()
 	conn := Server(s, serverConfig)
-	ch, err := conn.readClientHello(ctx)
+	ch, err := conn.readClientHello()
 	hs := serverHandshakeState{
 		c:           conn,
-		ctx:         ctx,
 		clientHello: ch,
 	}
 	if err == nil {
@@ -1425,11 +1421,9 @@ func TestSNIGivenOnFailure(t *testing.T) {
 		c.Close()
 	}()
 	conn := Server(s, serverConfig)
-	ctx := context.Background()
+	ch, err := conn.readClientHello()
-	ch, err := conn.readClientHello(ctx)
 	hs := serverHandshakeState{
 		c:           conn,
-		ctx:         ctx,
 		clientHello: ch,
 	}
 	if err == nil {
@@ -1683,46 +1677,6 @@ func TestMultipleCertificates(t *testing.T) {
 	}
 }
 
-func TestServerHandshakeContextCancellation(t *testing.T) {
-	c, s := localPipe(t)
-	clientConfig := testConfig.Clone()
-	clientErr := make(chan error, 1)
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-	go func() {
-		defer close(clientErr)
-		defer c.Close()
-		clientHello := &clientHelloMsg{
-			vers:               VersionTLS10,
-			random:             make([]byte, 32),
-			cipherSuites:       []uint16{TLS_RSA_WITH_RC4_128_SHA},
-			compressionMethods: []uint8{compressionNone},
-		}
-		cli := Client(c, clientConfig)
-		_, err := cli.writeRecord(recordTypeHandshake, clientHello.marshal())
-		cancel()
-		clientErr <- err
-	}()
-	conn := Server(s, testConfig)
-	err := conn.HandshakeContext(ctx)
-	if err == nil {
-		t.Fatal("Server handshake did not error when the context was canceled")
-	}
-	if err != context.Canceled {
-		t.Errorf("Unexpected server handshake error: %v", err)
-	}
-	if err := <-clientErr; err != nil {
-		t.Errorf("Unexpected client error: %v", err)
-	}
-	if runtime.GOARCH == "wasm" {
-		t.Skip("conn.Close does not error as expected when called multiple times on WASM")
-	}
-	err = conn.Close()
-	if err == nil {
-		t.Error("Server connection was not closed when the context was canceled")
-	}
-}
-
 func TestAESCipherReordering(t *testing.T) {
 	currentAESSupport := hasAESGCMHardwareSupport
 	defer func() { hasAESGCMHardwareSupport = currentAESSupport; initDefaultCipherSuites() }()

handshake_server_tls13.go

@@ -6,7 +6,6 @@ package tls
 
 import (
 	"bytes"
-	"context"
 	"crypto"
 	"crypto/hmac"
 	"crypto/rsa"
@@ -24,7 +23,6 @@ const maxClientPSKIdentities = 5
 
 type serverHandshakeStateTLS13 struct {
 	c               *Conn
-	ctx             context.Context
 	clientHello     *clientHelloMsg
 	hello           *serverHelloMsg
 	sentDummyCCS    bool
@@ -376,7 +374,7 @@ func (hs *serverHandshakeStateTLS13) pickCertificate() error {
 		return c.sendAlert(alertMissingExtension)
 	}
 
-	certificate, err := c.config.getCertificate(clientHelloInfo(hs.ctx, c, hs.clientHello))
+	certificate, err := c.config.getCertificate(clientHelloInfo(c, hs.clientHello))
 	if err != nil {
 		if err == errNoCertificates {
 			c.sendAlert(alertUnrecognizedName)

tls.go

@@ -25,6 +25,7 @@ import (
 	"net"
 	"os"
 	"strings"
+	"time"
 )
 
 // Server returns a new TLS server side connection
@@ -115,16 +116,28 @@ func DialWithDialer(dialer *net.Dialer, network, addr string, config *Config) (*
 }
 
 func dial(ctx context.Context, netDialer *net.Dialer, network, addr string, config *Config) (*Conn, error) {
-	if netDialer.Timeout != 0 {
+	// We want the Timeout and Deadline values from dialer to cover the
-		var cancel context.CancelFunc
+	// whole process: TCP connection and TLS handshake. This means that we
-		ctx, cancel = context.WithTimeout(ctx, netDialer.Timeout)
+	// also need to start our own timers now.
-		defer cancel()
+	timeout := netDialer.Timeout
-	}
 
 	if !netDialer.Deadline.IsZero() {
-		var cancel context.CancelFunc
+		deadlineTimeout := time.Until(netDialer.Deadline)
-		ctx, cancel = context.WithDeadline(ctx, netDialer.Deadline)
+		if timeout == 0 || deadlineTimeout < timeout {
-		defer cancel()
+			timeout = deadlineTimeout
+		}
+	}
+
+	// hsErrCh is non-nil if we might not wait for Handshake to complete.
+	var hsErrCh chan error
+	if timeout != 0 || ctx.Done() != nil {
+		hsErrCh = make(chan error, 2)
+	}
+	if timeout != 0 {
+		timer := time.AfterFunc(timeout, func() {
+			hsErrCh <- timeoutError{}
+		})
+		defer timer.Stop()
 	}
 
 	rawConn, err := netDialer.DialContext(ctx, network, addr)
@@ -151,10 +164,34 @@ func dial(ctx context.Context, netDialer *net.Dialer, network, addr string, conf
 	}
 
 	conn := Client(rawConn, config)
-	if err := conn.HandshakeContext(ctx); err != nil {
+
+	if hsErrCh == nil {
+		err = conn.Handshake()
+	} else {
+		go func() {
+			hsErrCh <- conn.Handshake()
+		}()
+
+		select {
+		case <-ctx.Done():
+			err = ctx.Err()
+		case err = <-hsErrCh:
+			if err != nil {
+				// If the error was due to the context
+				// closing, prefer the context's error, rather
+				// than some random network teardown error.
+				if e := ctx.Err(); e != nil {
+					err = e
+				}
+			}
+		}
+	}
+
+	if err != nil {
 		rawConn.Close()
 		return nil, err
 	}
+
 	return conn, nil
 }