Merge pull request #1817 from lucas-clemente/tls-errors

rework crypto errors

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.0
+	github.com/marten-seemann/qtls v0.2.1
 	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.0 h1:SnGwbmSUjODZ3PPCG6N0GX0w30yvndyFmoNY2pbgW+s=
-github.com/marten-seemann/qtls v0.2.0/go.mod h1:xzjG7avBwGGbdZ8dTGxlBnLArsVKLvwmjgmPuiQEcYk=
+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/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_test.go

@@ -24,12 +24,14 @@ var _ = Describe("Handshake tests", func() {
 		server        quic.Listener
 		serverConfig  *quic.Config
 		acceptStopped chan struct{}
+		tlsServerConf *tls.Config
 	)
 
 	BeforeEach(func() {
 		server = nil
 		acceptStopped = make(chan struct{})
 		serverConfig = &quic.Config{}
+		tlsServerConf = testdata.GetTLSConfig()
 	})
 
 	AfterEach(func() {
@@ -42,7 +44,7 @@ var _ = Describe("Handshake tests", func() {
 	runServer := func() quic.Listener {
 		var err error
 		// start the server
-		server, err = quic.ListenAddr("localhost:0", testdata.GetTLSConfig(), serverConfig)
+		server, err = quic.ListenAddr("localhost:0", tlsServerConf, serverConfig)
 		Expect(err).ToNot(HaveOccurred())
 
 		go func() {
@@ -117,8 +119,11 @@ var _ = Describe("Handshake tests", func() {
 					}
 				})
 
-				It("accepts the certificate", func() {
+				JustBeforeEach(func() {
 					runServer()
+				})
+
+				It("accepts the certificate", func() {
 					_, err := quic.DialAddr(
 						fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
 						tlsConf,
@@ -128,17 +133,33 @@ var _ = Describe("Handshake tests", func() {
 				})
 
 				It("errors if the server name doesn't match", func() {
-					runServer()
 					_, err := quic.DialAddr(
 						fmt.Sprintf("127.0.0.1:%d", server.Addr().(*net.UDPAddr).Port),
 						tlsConf,
 						clientConfig,
 					)
-					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() {
+					tlsServerConf.ClientAuth = tls.RequireAndVerifyClientCert
+					sess, err := quic.DialAddr(
+						fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
+						tlsConf,
+						clientConfig,
+					)
+					Expect(err).ToNot(HaveOccurred())
+					// The error will occur after the client already finished the handshake.
+					errChan := make(chan error)
+					go func() {
+						defer GinkgoRecover()
+						_, err := sess.AcceptStream()
+						errChan <- err
+					}()
+					Eventually(errChan).Should(Receive(MatchError("CRYPTO_ERROR: tls: bad certificate")))
 				})
 
 				It("uses the ServerName in the tls.Config", func() {
-					runServer()
 					tlsConf.ServerName = "localhost"
 					_, err := quic.DialAddr(
 						fmt.Sprintf("127.0.0.1:%d", server.Addr().(*net.UDPAddr).Port),

internal/handshake/crypto_setup.go

@@ -8,6 +8,7 @@ import (
 	"io"
 
 	"github.com/lucas-clemente/quic-go/internal/protocol"
+	"github.com/lucas-clemente/quic-go/internal/qerr"
 	"github.com/lucas-clemente/quic-go/internal/utils"
 	"github.com/marten-seemann/qtls"
 )
@@ -64,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
@@ -186,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),
@@ -212,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)
@@ -228,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().
@@ -461,8 +463,7 @@ func (h *cryptoSetup) WriteRecord(p []byte) (int, error) {
 }
 
 func (h *cryptoSetup) SendAlert(alert uint8) {
-	// TODO(#1804): send the correct IETF QUIC error code
-	h.alertChan <- fmt.Errorf("TLS alert: %d", alert)
+	h.alertChan <- alert
 }
 
 func (h *cryptoSetup) GetSealer() (protocol.EncryptionLevel, Sealer) {

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("TLS alert: 10"))
+			Expect(err).To(MatchError("CRYPTO_ERROR: local error: tls: unexpected message"))
 			close(done)
 		}()
 

internal/qerr/error_codes.go

@@ -1,6 +1,10 @@
 package qerr
 
-import "fmt"
+import (
+	"fmt"
+
+	"github.com/marten-seemann/qtls"
+)
 
 // ErrorCode can be used as a normal error without reason.
 type ErrorCode uint16
@@ -19,10 +23,16 @@ const (
 	VersionNegotiationError ErrorCode = 0x9
 	ProtocolViolation       ErrorCode = 0xa
 	InvalidMigration        ErrorCode = 0xc
-	CryptoError             ErrorCode = 0x100
 )
 
+func (e ErrorCode) isCryptoError() bool {
+	return e >= 0x100 && e < 0x200
+}
+
 func (e ErrorCode) Error() string {
+	if e.isCryptoError() {
+		return fmt.Sprintf("%s: %s", e.String(), qtls.Alert(e-0x100).Error())
+	}
 	return e.String()
 }
 
@@ -52,9 +62,10 @@ func (e ErrorCode) String() string {
 		return "PROTOCOL_VIOLATION"
 	case InvalidMigration:
 		return "INVALID_MIGRATION"
-	case CryptoError:
-		return "CRYPTO_ERROR"
 	default:
-		return fmt.Sprintf("unknown error code: %d", e)
+		if e.isCryptoError() {
+			return "CRYPTO_ERROR"
+		}
+		return fmt.Sprintf("unknown error code: %#x", uint16(e))
 	}
 }

internal/qerr/errorcodes_test.go

@@ -35,6 +35,6 @@ var _ = Describe("error codes", func() {
 	})
 
 	It("has a string representation for unknown error codes", func() {
-		Expect(ErrorCode(1337).String()).To(Equal("unknown error code: 1337"))
+		Expect(ErrorCode(0x1337).String()).To(Equal("unknown error code: 0x1337"))
 	})
 })

internal/qerr/quic_error.go

@@ -30,10 +30,26 @@ func TimeoutError(errorMessage string) *QuicError {
 	}
 }
 
+// CryptoError create a new QuicError instance for a crypto error
+func CryptoError(tlsAlert uint8, errorMessage string) *QuicError {
+	return &QuicError{
+		ErrorCode:    0x100 + ErrorCode(tlsAlert),
+		ErrorMessage: errorMessage,
+	}
+}
+
 func (e *QuicError) Error() string {
+	if len(e.ErrorMessage) == 0 {
+		return e.ErrorCode.Error()
+	}
 	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

internal/qerr/quic_error_test.go

@@ -8,10 +8,37 @@ import (
 )
 
 var _ = Describe("QUIC Transport Errors", func() {
-	Context("QuicError", func() {
-		It("has a string representation", func() {
-			err := Error(FlowControlError, "foobar")
-			Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR: foobar"))
+	It("has a string representation", func() {
+		err := Error(FlowControlError, "foobar")
+		Expect(err.Timeout()).To(BeFalse())
+		Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR: foobar"))
+	})
+
+	It("has a string representation for empty error phrases", func() {
+		err := Error(FlowControlError, "")
+		Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR"))
+	})
+
+	It("has a string representation for timeout errors", func() {
+		err := TimeoutError("foobar")
+		Expect(err.Timeout()).To(BeTrue())
+		Expect(err.Error()).To(Equal("NO_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("says if an error is a crypto error", func() {
+			Expect(Error(FlowControlError, "").IsCryptoError()).To(BeFalse())
+			Expect(CryptoError(42, "").IsCryptoError()).To(BeTrue())
 		})
 	})
 
@@ -20,6 +47,11 @@ var _ = Describe("QUIC Transport Errors", func() {
 			var err error = StreamStateError
 			Expect(err).To(MatchError("STREAM_STATE_ERROR"))
 		})
+
+		It("recognizes crypto errors", func() {
+			err := ErrorCode(0x100 + 42)
+			Expect(err.Error()).To(Equal("CRYPTO_ERROR: tls: bad certificate"))
+		})
 	})
 
 	Context("ToQuicError", func() {

session.go

@@ -1144,9 +1144,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