From 12d50e6810a45958c41cb36c1c48afb9cf96e6af Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 19 May 2022 11:37:08 +0200 Subject: [PATCH 1/3] tighten typing of channel in the crypto setup --- internal/handshake/crypto_setup.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index 7543be99..51bd1a44 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -113,7 +113,7 @@ type cryptoSetup struct { zeroRTTParameters *wire.TransportParameters clientHelloWritten bool - clientHelloWrittenChan chan *wire.TransportParameters + clientHelloWrittenChan chan<- *wire.TransportParameters rttStats *utils.RTTStats @@ -238,6 +238,7 @@ func newCryptoSetup( tracer.UpdatedKeyFromTLS(protocol.EncryptionInitial, protocol.PerspectiveServer) } extHandler := newExtensionHandler(tp.Marshal(perspective), perspective, version) + clientHelloWrittenChan := make(chan *wire.TransportParameters, 1) cs := &cryptoSetup{ tlsConf: tlsConf, initialStream: initialStream, @@ -256,7 +257,7 @@ func newCryptoSetup( perspective: perspective, handshakeDone: make(chan struct{}), alertChan: make(chan uint8), - clientHelloWrittenChan: make(chan *wire.TransportParameters, 1), + clientHelloWrittenChan: clientHelloWrittenChan, messageChan: make(chan []byte, 100), isReadingHandshakeMessage: make(chan struct{}), closeChan: make(chan struct{}), @@ -278,7 +279,7 @@ func newCryptoSetup( GetAppDataForSessionState: cs.marshalDataForSessionState, SetAppDataFromSessionState: cs.handleDataFromSessionState, } - return cs, cs.clientHelloWrittenChan + return cs, clientHelloWrittenChan } func (h *cryptoSetup) ChangeConnectionID(id protocol.ConnectionID) { From c225299c844142710d7ca2f3b43d128a17ed91a1 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 19 May 2022 12:29:12 +0200 Subject: [PATCH 2/3] handle TLS errors that occur before the ClientHello has been written --- internal/handshake/crypto_setup.go | 32 +++++++++++++++----- internal/handshake/crypto_setup_test.go | 40 ++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index 51bd1a44..25b35cff 100644 --- a/internal/handshake/crypto_setup.go +++ b/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,7 +239,7 @@ func newCryptoSetup( tracer.UpdatedKeyFromTLS(protocol.EncryptionInitial, protocol.PerspectiveServer) } extHandler := newExtensionHandler(tp.Marshal(perspective), perspective, version) - clientHelloWrittenChan := make(chan *wire.TransportParameters, 1) + zeroRTTParametersChan := make(chan *wire.TransportParameters, 1) cs := &cryptoSetup{ tlsConf: tlsConf, initialStream: initialStream, @@ -257,7 +258,8 @@ func newCryptoSetup( perspective: perspective, handshakeDone: make(chan struct{}), alertChan: make(chan uint8), - clientHelloWrittenChan: clientHelloWrittenChan, + clientHelloWrittenChan: make(chan struct{}), + zeroRTTParametersChan: zeroRTTParametersChan, messageChan: make(chan []byte, 100), isReadingHandshakeMessage: make(chan struct{}), closeChan: make(chan struct{}), @@ -279,7 +281,7 @@ func newCryptoSetup( GetAppDataForSessionState: cs.marshalDataForSessionState, SetAppDataFromSessionState: cs.handleDataFromSessionState, } - return cs, clientHelloWrittenChan + return cs, zeroRTTParametersChan } func (h *cryptoSetup) ChangeConnectionID(id protocol.ConnectionID) { @@ -309,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() @@ -325,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. @@ -646,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 diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go index 5720321a..4717c181 100644 --- a/internal/handshake/crypto_setup_test.go +++ b/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() From 53be3ee5008a04ae30586785a761864de21fc77c Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 19 May 2022 13:21:49 +0200 Subject: [PATCH 3/3] don't send CONNECTION_CLOSE if error occurred before sending anything --- connection.go | 8 ++++++ connection_test.go | 20 +++++++++++++++ integrationtests/self/handshake_test.go | 34 +++++++++++++++++++++++++ 3 files changed, 62 insertions(+) diff --git a/connection.go b/connection.go index 47f4ca99..e0adc80c 100644 --- a/connection.go +++ b/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() { diff --git a/connection_test.go b/connection_test.go index 1677a6d2..c36681f7 100644 --- a/connection_test.go +++ b/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 diff --git a/integrationtests/self/handshake_test.go b/integrationtests/self/handshake_test.go index 8d3bea4d..dee162f7 100644 --- a/integrationtests/self/handshake_test.go +++ b/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()) + }) })