From 5b27076a4c1acd0c1815a43e1b0440a277edda44 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 7 Mar 2019 16:16:02 +0900 Subject: [PATCH] return the local TLS error, but don't send it on the wire --- integrationtests/self/handshake_test.go | 3 +-- internal/handshake/crypto_setup.go | 15 ++++++++------- internal/handshake/crypto_setup_test.go | 2 +- internal/qerr/quic_error.go | 5 +++++ internal/qerr/quic_error_test.go | 21 ++++++++++++++------- session.go | 7 ++++++- 6 files changed, 35 insertions(+), 18 deletions(-) diff --git a/integrationtests/self/handshake_test.go b/integrationtests/self/handshake_test.go index 741b6f31..73767c9d 100644 --- a/integrationtests/self/handshake_test.go +++ b/integrationtests/self/handshake_test.go @@ -138,8 +138,7 @@ var _ = Describe("Handshake tests", func() { tlsConf, clientConfig, ) - // TODO: check the error returned locally here - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError("CRYPTO_ERROR: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs")) }) It("fails the handshake if the client fails to provide the requested client cert", func() { diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index bf510653..f1f4e540 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -65,7 +65,7 @@ type cryptoSetup struct { handleParamsCallback func([]byte) - alertChan chan error + alertChan chan uint8 // HandleData() sends errors on the messageErrChan messageErrChan chan error // handshakeDone is closed as soon as the go routine running qtls.Handshake() returns @@ -187,7 +187,7 @@ func newCryptoSetup( logger: logger, perspective: perspective, handshakeDone: make(chan struct{}), - alertChan: make(chan error), + alertChan: make(chan uint8), messageErrChan: make(chan error, 1), clientHelloWrittenChan: make(chan struct{}), messageChan: make(chan []byte, 100), @@ -213,10 +213,11 @@ func (h *cryptoSetup) ChangeConnectionID(id protocol.ConnectionID) error { func (h *cryptoSetup) RunHandshake() error { // Handle errors that might occur when HandleData() is called. handshakeComplete := make(chan struct{}) + handshakeErrChan := make(chan error, 1) go func() { defer close(h.handshakeDone) if err := h.conn.Handshake(); err != nil { - h.logger.Debugf("qlts.Handshake error: %s", err) + handshakeErrChan <- err return } close(handshakeComplete) @@ -229,9 +230,9 @@ func (h *cryptoSetup) RunHandshake() error { return errors.New("Handshake aborted") case <-handshakeComplete: // return when the handshake is done return nil - case err := <-h.alertChan: - <-h.handshakeDone - return err + case alert := <-h.alertChan: + err := <-handshakeErrChan + return qerr.CryptoError(alert, err.Error()) case err := <-h.messageErrChan: // If the handshake errored because of an error that occurred during HandleData(), // that error message will be more useful than the error message generated by Handshake(). @@ -462,7 +463,7 @@ func (h *cryptoSetup) WriteRecord(p []byte) (int, error) { } func (h *cryptoSetup) SendAlert(alert uint8) { - h.alertChan <- qerr.CryptoError(alert, "") + h.alertChan <- alert } func (h *cryptoSetup) GetSealer() (protocol.EncryptionLevel, Sealer) { diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go index cd541a68..2fdda7f8 100644 --- a/internal/handshake/crypto_setup_test.go +++ b/internal/handshake/crypto_setup_test.go @@ -128,7 +128,7 @@ var _ = Describe("Crypto Setup TLS", func() { go func() { defer GinkgoRecover() err := server.RunHandshake() - Expect(err).To(MatchError("CRYPTO_ERROR: tls: unexpected message")) + Expect(err).To(MatchError("CRYPTO_ERROR: local error: tls: unexpected message")) close(done) }() diff --git a/internal/qerr/quic_error.go b/internal/qerr/quic_error.go index f4eb476d..3a1718b0 100644 --- a/internal/qerr/quic_error.go +++ b/internal/qerr/quic_error.go @@ -45,6 +45,11 @@ func (e *QuicError) Error() string { return fmt.Sprintf("%s: %s", e.ErrorCode.String(), e.ErrorMessage) } +// IsCryptoError says if this error is a crypto error +func (e *QuicError) IsCryptoError() bool { + return e.ErrorCode.isCryptoError() +} + // Temporary says if the error is temporary. func (e *QuicError) Temporary() bool { return false diff --git a/internal/qerr/quic_error_test.go b/internal/qerr/quic_error_test.go index 7059c841..6701e39d 100644 --- a/internal/qerr/quic_error_test.go +++ b/internal/qerr/quic_error_test.go @@ -25,14 +25,21 @@ var _ = Describe("QUIC Transport Errors", func() { Expect(err.Error()).To(Equal("NO_ERROR: foobar")) }) - It("has a string representation for crypto errors with a message", func() { - err := CryptoError(42, "foobar") - Expect(err.Error()).To(Equal("CRYPTO_ERROR: foobar")) - }) + Context("crypto errors", func() { + It("has a string representation for crypto errors with a message", func() { + err := CryptoError(42, "foobar") + Expect(err.Error()).To(Equal("CRYPTO_ERROR: foobar")) + }) - It("has a string representation for crypto errors without a message", func() { - err := CryptoError(42, "") - Expect(err.Error()).To(Equal("CRYPTO_ERROR: tls: bad certificate")) + It("has a string representation for crypto errors without a message", func() { + err := CryptoError(42, "") + Expect(err.Error()).To(Equal("CRYPTO_ERROR: tls: bad certificate")) + }) + + It("says if an error is a crypto error", func() { + Expect(Error(FlowControlError, "").IsCryptoError()).To(BeFalse()) + Expect(CryptoError(42, "").IsCryptoError()).To(BeTrue()) + }) }) Context("ErrorCode", func() { diff --git a/session.go b/session.go index 81f812fb..48933a7a 100644 --- a/session.go +++ b/session.go @@ -1133,9 +1133,14 @@ func (s *session) sendPackedPacket(packet *packedPacket) error { } func (s *session) sendConnectionClose(quicErr *qerr.QuicError) error { + var reason string + // don't send details of crypto errors + if !quicErr.IsCryptoError() { + reason = quicErr.ErrorMessage + } packet, err := s.packer.PackConnectionClose(&wire.ConnectionCloseFrame{ ErrorCode: quicErr.ErrorCode, - ReasonPhrase: quicErr.ErrorMessage, + ReasonPhrase: reason, }) if err != nil { return err