Merge pull request #3424 from lucas-clemente/fix-tls-error-handling2

don't ignore errors that occur when the TLS ClientHello is generated

connection.go

@@ -190,6 +190,7 @@ type connection struct {
 	clientHelloWritten    <-chan *wire.TransportParameters
 	earlyConnReadyChan    chan struct{}
 	handshakeCompleteChan chan struct{} // is closed when the handshake completes
+	sentFirstPacket       bool
 	handshakeComplete     bool
 	handshakeConfirmed    bool
 
@@ -1519,6 +1520,12 @@ func (s *connection) handleCloseError(closeErr *closeError) {
 		s.connIDGenerator.RemoveAll()
 		return
 	}
+	// Don't send out any CONNECTION_CLOSE if this is an error that occurred
+	// before we even sent out the first packet.
+	if s.perspective == protocol.PerspectiveClient && !s.sentFirstPacket {
+		s.connIDGenerator.RemoveAll()
+		return
+	}
 	connClosePacket, err := s.sendConnectionClose(e)
 	if err != nil {
 		s.logger.Debugf("Error sending CONNECTION_CLOSE: %s", err)
@@ -1760,6 +1767,7 @@ func (s *connection) sendPacket() (bool, error) {
 		if err != nil || packet == nil {
 			return false, err
 		}
+		s.sentFirstPacket = true
 		s.logCoalescedPacket(packet)
 		for _, p := range packet.packets {
 			if s.firstAckElicitingPacketAfterIdleSentTime.IsZero() && p.IsAckEliciting() {

connection_test.go

@@ -2477,6 +2477,7 @@ var _ = Describe("Client Connection", func() {
 		conn.packer = packer
 		cryptoSetup = mocks.NewMockCryptoSetup(mockCtrl)
 		conn.cryptoStreamHandler = cryptoSetup
+		conn.sentFirstPacket = true
 	})
 
 	It("changes the connection ID when receiving the first packet from the server", func() {
@@ -2568,6 +2569,25 @@ var _ = Describe("Client Connection", func() {
 		Expect(conn.handleAckFrame(ack, protocol.Encryption1RTT)).To(Succeed())
 	})
 
+	It("doesn't send a CONNECTION_CLOSE when no packet was sent", func() {
+		conn.sentFirstPacket = false
+		tracer.EXPECT().ClosedConnection(gomock.Any())
+		tracer.EXPECT().Close()
+		running := make(chan struct{})
+		cryptoSetup.EXPECT().RunHandshake().Do(func() {
+			close(running)
+			conn.closeLocal(errors.New("early error"))
+		})
+		cryptoSetup.EXPECT().Close()
+		connRunner.EXPECT().Remove(gomock.Any())
+		go func() {
+			defer GinkgoRecover()
+			conn.run()
+		}()
+		Eventually(running).Should(BeClosed())
+		Eventually(areConnsRunning).Should(BeFalse())
+	})
+
 	Context("handling tokens", func() {
 		var mockTokenStore *MockTokenStore
 

integrationtests/self/handshake_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/lucas-clemente/quic-go"
 	"github.com/lucas-clemente/quic-go/integrationtests/tools/israce"
 	"github.com/lucas-clemente/quic-go/internal/protocol"
+	"github.com/lucas-clemente/quic-go/internal/qerr"
 	"github.com/lucas-clemente/quic-go/logging"
 
 	. "github.com/onsi/ginkgo"
@@ -567,4 +568,37 @@ var _ = Describe("Handshake tests", func() {
 			Expect(token.IsRetryToken).To(BeTrue())
 		})
 	})
+
+	It("doesn't send any packets when generating the ClientHello fails", func() {
+		ln, err := net.ListenUDP("udp", nil)
+		Expect(err).ToNot(HaveOccurred())
+		done := make(chan struct{})
+		packetChan := make(chan struct{})
+		go func() {
+			defer GinkgoRecover()
+			defer close(done)
+			for {
+				_, _, err := ln.ReadFromUDP(make([]byte, protocol.MaxPacketBufferSize))
+				if err != nil {
+					return
+				}
+				packetChan <- struct{}{}
+			}
+		}()
+
+		tlsConf := getTLSClientConfig()
+		tlsConf.NextProtos = []string{""}
+		_, err = quic.DialAddr(
+			fmt.Sprintf("localhost:%d", ln.LocalAddr().(*net.UDPAddr).Port),
+			tlsConf,
+			nil,
+		)
+		Expect(err).To(MatchError(&qerr.TransportError{
+			ErrorCode:    qerr.InternalError,
+			ErrorMessage: "tls: invalid NextProtos value",
+		}))
+		Consistently(packetChan).ShouldNot(Receive())
+		ln.Close()
+		Eventually(done).Should(BeClosed())
+	})
 })

internal/handshake/crypto_setup.go

@@ -113,7 +113,8 @@ type cryptoSetup struct {
 
 	zeroRTTParameters      *wire.TransportParameters
 	clientHelloWritten     bool
-	clientHelloWrittenChan chan *wire.TransportParameters
+	clientHelloWrittenChan chan struct{} // is closed as soon as the ClientHello is written
+	zeroRTTParametersChan  chan<- *wire.TransportParameters
 
 	rttStats *utils.RTTStats
 
@@ -238,6 +239,7 @@ func newCryptoSetup(
 		tracer.UpdatedKeyFromTLS(protocol.EncryptionInitial, protocol.PerspectiveServer)
 	}
 	extHandler := newExtensionHandler(tp.Marshal(perspective), perspective, version)
+	zeroRTTParametersChan := make(chan *wire.TransportParameters, 1)
 	cs := &cryptoSetup{
 		tlsConf:                   tlsConf,
 		initialStream:             initialStream,
@@ -256,7 +258,8 @@ func newCryptoSetup(
 		perspective:               perspective,
 		handshakeDone:             make(chan struct{}),
 		alertChan:                 make(chan uint8),
-		clientHelloWrittenChan:    make(chan *wire.TransportParameters, 1),
+		clientHelloWrittenChan:    make(chan struct{}),
+		zeroRTTParametersChan:     zeroRTTParametersChan,
 		messageChan:               make(chan []byte, 100),
 		isReadingHandshakeMessage: make(chan struct{}),
 		closeChan:                 make(chan struct{}),
@@ -278,7 +281,7 @@ func newCryptoSetup(
 		GetAppDataForSessionState:  cs.marshalDataForSessionState,
 		SetAppDataFromSessionState: cs.handleDataFromSessionState,
 	}
-	return cs, cs.clientHelloWrittenChan
+	return cs, zeroRTTParametersChan
 }
 
 func (h *cryptoSetup) ChangeConnectionID(id protocol.ConnectionID) {
@@ -308,6 +311,15 @@ func (h *cryptoSetup) RunHandshake() {
 		close(handshakeComplete)
 	}()
 
+	if h.perspective == protocol.PerspectiveClient {
+		select {
+		case err := <-handshakeErrChan:
+			h.onError(0, err.Error())
+			return
+		case <-h.clientHelloWrittenChan:
+		}
+	}
+
 	select {
 	case <-handshakeComplete: // return when the handshake is done
 		h.mutex.Lock()
@@ -324,7 +336,13 @@ func (h *cryptoSetup) RunHandshake() {
 }
 
 func (h *cryptoSetup) onError(alert uint8, message string) {
-	h.runner.OnError(qerr.NewCryptoError(alert, message))
+	var err error
+	if alert == 0 {
+		err = &qerr.TransportError{ErrorCode: qerr.InternalError, ErrorMessage: message}
+	} else {
+		err = qerr.NewCryptoError(alert, message)
+	}
+	h.runner.OnError(err)
 }
 
 // Close closes the crypto setup.
@@ -645,12 +663,13 @@ func (h *cryptoSetup) WriteRecord(p []byte) (int, error) {
 		n, err := h.initialStream.Write(p)
 		if !h.clientHelloWritten && h.perspective == protocol.PerspectiveClient {
 			h.clientHelloWritten = true
+			close(h.clientHelloWrittenChan)
 			if h.zeroRTTSealer != nil && h.zeroRTTParameters != nil {
 				h.logger.Debugf("Doing 0-RTT.")
-				h.clientHelloWrittenChan <- h.zeroRTTParameters
+				h.zeroRTTParametersChan <- h.zeroRTTParameters
 			} else {
 				h.logger.Debugf("Not doing 0-RTT.")
-				h.clientHelloWrittenChan <- nil
+				h.zeroRTTParametersChan <- nil
 			}
 		}
 		return n, err

internal/handshake/crypto_setup_test.go

@@ -107,7 +107,7 @@ var _ = Describe("Crypto Setup TLS", func() {
 			defer GinkgoRecover()
 			server.RunHandshake()
 			Expect(sErrChan).To(Receive(MatchError(&qerr.TransportError{
-				ErrorCode:    0x10a,
+				ErrorCode:    0x100 + qerr.TransportErrorCode(alertUnexpectedMessage),
 				ErrorMessage: "local error: tls: unexpected message",
 			})))
 			close(done)
@@ -124,6 +124,44 @@ var _ = Describe("Crypto Setup TLS", func() {
 		Eventually(done).Should(BeClosed())
 	})
 
+	It("handles qtls errors occurring before during ClientHello generation", func() {
+		sErrChan := make(chan error, 1)
+		runner := NewMockHandshakeRunner(mockCtrl)
+		runner.EXPECT().OnError(gomock.Any()).Do(func(e error) { sErrChan <- e })
+		_, sInitialStream, sHandshakeStream := initStreams()
+		tlsConf := testdata.GetTLSConfig()
+		tlsConf.InsecureSkipVerify = true
+		tlsConf.NextProtos = []string{""}
+		cl, _ := NewCryptoSetupClient(
+			sInitialStream,
+			sHandshakeStream,
+			protocol.ConnectionID{},
+			nil,
+			nil,
+			&wire.TransportParameters{},
+			runner,
+			tlsConf,
+			false,
+			&utils.RTTStats{},
+			nil,
+			utils.DefaultLogger.WithPrefix("client"),
+			protocol.VersionTLS,
+		)
+
+		done := make(chan struct{})
+		go func() {
+			defer GinkgoRecover()
+			cl.RunHandshake()
+			close(done)
+		}()
+
+		Eventually(done).Should(BeClosed())
+		Expect(sErrChan).To(Receive(MatchError(&qerr.TransportError{
+			ErrorCode:    qerr.InternalError,
+			ErrorMessage: "tls: invalid NextProtos value",
+		})))
+	})
+
 	It("errors when a message is received at the wrong encryption level", func() {
 		sErrChan := make(chan error, 1)
 		_, sInitialStream, sHandshakeStream := initStreams()