From f103919bf1a36a738f81ca0238829980f3eb38ec Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 10 Mar 2019 09:55:32 +0900 Subject: [PATCH] fix handling of HelloRetryRequests --- go.mod | 2 +- go.sum | 4 ++-- internal/handshake/crypto_setup.go | 25 +++++++++++++++++++++++++ internal/handshake/crypto_setup_test.go | 8 ++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 1004d46dd..d5d22a975 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 5ca8c48f2..e52e5910e 100644 --- a/go.sum +++ b/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= diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index f1f4e5409..eda65e48b 100644 --- a/internal/handshake/crypto_setup.go +++ b/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 diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go index c6ddf18ec..614b7bc05 100644 --- a/internal/handshake/crypto_setup_test.go +++ b/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()