Merge pull request #1822 from lucas-clemente/hello-retry-request

fix handling of HelloRetryRequests

go.mod

@@ -5,7 +5,7 @@ go 1.12
 require (
 	github.com/cheekybits/genny v1.0.0
 	github.com/golang/mock v1.2.0
-	github.com/marten-seemann/qtls v0.2.1
+	github.com/marten-seemann/qtls v0.2.2
 	github.com/onsi/ginkgo v1.7.0
 	github.com/onsi/gomega v1.4.3
 	golang.org/x/crypto v0.0.0-20190228161510-8dd112bcdc25

go.sum

@@ -8,8 +8,8 @@ github.com/golang/protobuf v1.2.0 h1:P3YflyNX/ehuJFLhxviNdFxQPkGK5cDcApsge1SqnvM
 github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
 github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
 github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
-github.com/marten-seemann/qtls v0.2.1 h1:MbFrPuLPxGVUJ3NYg+Ie9JMir4meVlNNzJSqhLFWRcw=
-github.com/marten-seemann/qtls v0.2.1/go.mod h1:xzjG7avBwGGbdZ8dTGxlBnLArsVKLvwmjgmPuiQEcYk=
+github.com/marten-seemann/qtls v0.2.2 h1:QcmNbsYmV0ByHRkBRhSik8rxmB3S/SPzd+LMlXTgyJM=
+github.com/marten-seemann/qtls v0.2.2/go.mod h1:xzjG7avBwGGbdZ8dTGxlBnLArsVKLvwmjgmPuiQEcYk=
 github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
 github.com/onsi/ginkgo v1.7.0 h1:WSHQ+IS43OoUrWtD1/bbclrwK8TTH5hzp+umCiuxHgs=
 github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=

integrationtests/self/handshake_rtt_test.go

@@ -2,7 +2,6 @@ package self_test
 
 import (
 	"crypto/tls"
-	"fmt"
 	"net"
 	"time"
 
@@ -17,11 +16,12 @@ import (
 
 var _ = Describe("Handshake RTT tests", func() {
 	var (
-		proxy         *quicproxy.QuicProxy
-		server        quic.Listener
-		serverConfig  *quic.Config
-		testStartedAt time.Time
-		acceptStopped chan struct{}
+		proxy           *quicproxy.QuicProxy
+		server          quic.Listener
+		serverConfig    *quic.Config
+		serverTLSConfig *tls.Config
+		testStartedAt   time.Time
+		acceptStopped   chan struct{}
 	)
 
 	rtt := 400 * time.Millisecond
@@ -29,6 +29,7 @@ var _ = Describe("Handshake RTT tests", func() {
 	BeforeEach(func() {
 		acceptStopped = make(chan struct{})
 		serverConfig = &quic.Config{}
+		serverTLSConfig = testdata.GetTLSConfig()
 	})
 
 	AfterEach(func() {
@@ -40,7 +41,7 @@ var _ = Describe("Handshake RTT tests", func() {
 	runServerAndProxy := func() {
 		var err error
 		// start the server
-		server, err = quic.ListenAddr("localhost:0", testdata.GetTLSConfig(), serverConfig)
+		server, err = quic.ListenAddr("localhost:0", serverTLSConfig, serverConfig)
 		Expect(err).ToNot(HaveOccurred())
 		// start the proxy
 		proxy, err = quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{
@@ -83,7 +84,6 @@ var _ = Describe("Handshake RTT tests", func() {
 		}
 		_, err := quic.DialAddr(proxy.LocalAddr().String(), nil, clientConfig)
 		Expect(err).To(HaveOccurred())
-		fmt.Println(err)
 		// Expect(err.(qerr.ErrorCode)).To(Equal(qerr.InvalidVersion))
 		expectDurationInRTTs(1)
 	})
@@ -113,7 +113,7 @@ var _ = Describe("Handshake RTT tests", func() {
 		expectDurationInRTTs(2)
 	})
 
-	It("is forward-secure after 1 RTTs when the server doesn't require a Cookie", func() {
+	It("establishes a connection in 1 RTT when the server doesn't require a Cookie", func() {
 		serverConfig.AcceptCookie = func(_ net.Addr, _ *quic.Cookie) bool {
 			return true
 		}
@@ -127,6 +127,21 @@ var _ = Describe("Handshake RTT tests", func() {
 		expectDurationInRTTs(1)
 	})
 
+	It("establishes a connection in 2 RTTs if a HelloRetryRequest is performed", func() {
+		serverConfig.AcceptCookie = func(_ net.Addr, _ *quic.Cookie) bool {
+			return true
+		}
+		serverTLSConfig.CurvePreferences = []tls.CurveID{tls.CurveP384}
+		runServerAndProxy()
+		_, err := quic.DialAddr(
+			proxy.LocalAddr().String(),
+			clientTLSConfig,
+			clientConfig,
+		)
+		Expect(err).ToNot(HaveOccurred())
+		expectDurationInRTTs(2)
+	})
+
 	It("doesn't complete the handshake when the server never accepts the Cookie", func() {
 		serverConfig.AcceptCookie = func(_ net.Addr, _ *quic.Cookie) bool {
 			return false

internal/handshake/crypto_setup.go

@@ -91,6 +91,12 @@ type cryptoSetup struct {
 
 	receivedWriteKey chan struct{}
 	receivedReadKey  chan struct{}
+	// WriteRecord does a non-blocking send on this channel.
+	// This way, handleMessage can see if qtls tries to write a message.
+	// This is necessary:
+	// for servers: to see if a HelloRetryRequest should be sent in response to a ClientHello
+	// for clients: to see if a ServerHello is a HelloRetryRequest
+	writeRecord chan struct{}
 
 	logger utils.Logger
 
@@ -193,6 +199,7 @@ func newCryptoSetup(
 		messageChan:            make(chan []byte, 100),
 		receivedReadKey:        make(chan struct{}),
 		receivedWriteKey:       make(chan struct{}),
+		writeRecord:            make(chan struct{}),
 		closeChan:              make(chan struct{}),
 	}
 	qtlsConf := cs.tlsConfigToQtlsConfig(tlsConf)
@@ -297,6 +304,11 @@ func (h *cryptoSetup) handleMessageForServer(msgType messageType) bool {
 	switch msgType {
 	case typeClientHello:
 		select {
+		case <-h.writeRecord:
+			// If qtls sends a HelloRetryRequest, it will only write the record.
+			// If it accepts the ClientHello, it will first read the transport parameters.
+			h.logger.Debugf("Sending HelloRetryRequest")
+			return false
 		case data := <-h.extHandler.TransportParameters():
 			h.handleParamsCallback(data)
 		case <-h.handshakeDone:
@@ -342,6 +354,12 @@ func (h *cryptoSetup) handleMessageForClient(msgType messageType) bool {
 	case typeServerHello:
 		// get the handshake write key
 		select {
+		case <-h.writeRecord:
+			// If qtls writes in response to a ServerHello, this means that this ServerHello
+			// is a HelloRetryRequest.
+			// Otherwise, we'd just wait for the Certificate message.
+			h.logger.Debugf("ServerHello is a HelloRetryRequest")
+			return false
 		case <-h.receivedWriteKey:
 		case <-h.handshakeDone:
 			return false
@@ -444,6 +462,13 @@ func (h *cryptoSetup) SetWriteKey(suite *qtls.CipherSuite, trafficSecret []byte)
 
 // WriteRecord is called when TLS writes data
 func (h *cryptoSetup) WriteRecord(p []byte) (int, error) {
+	defer func() {
+		select {
+		case h.writeRecord <- struct{}{}:
+		default:
+		}
+	}()
+
 	switch h.writeEncLevel {
 	case protocol.EncryptionInitial:
 		// assume that the first WriteRecord call contains the ClientHello

internal/handshake/crypto_setup_test.go

@@ -298,6 +298,14 @@ var _ = Describe("Crypto Setup TLS", func() {
 			Expect(serverErr).ToNot(HaveOccurred())
 		})
 
+		It("performs a HelloRetryRequst", func() {
+			serverConf := testdata.GetTLSConfig()
+			serverConf.CurvePreferences = []tls.CurveID{tls.CurveP384}
+			clientErr, serverErr := handshakeWithTLSConf(clientConf, serverConf)
+			Expect(clientErr).ToNot(HaveOccurred())
+			Expect(serverErr).ToNot(HaveOccurred())
+		})
+
 		It("handshakes with client auth", func() {
 			clientConf.Certificates = []tls.Certificate{generateCert()}
 			serverConf := testdata.GetTLSConfig()