Merge pull request #630 from lucas-clemente/fix-521

add a quic.Config option to set the handshake timeout

Changelog.md

@@ -5,4 +5,5 @@
 - Add a `quic.Config` option for QUIC versions
 - Add a `quic.Config` option to request truncation of the connection ID from a server
 - Add a `quic.Config` option to configure the source address validation
+- Add a `quic.Config` option to configure the handshake timeout
 - Various bugfixes

client.go

@@ -118,9 +118,15 @@ func populateClientConfig(config *Config) *Config {
 		versions = protocol.SupportedVersions
 	}
 
+	handshakeTimeout := protocol.DefaultHandshakeTimeout
+	if config.HandshakeTimeout != 0 {
+		handshakeTimeout = config.HandshakeTimeout
+	}
+
 	return &Config{
 		TLSConfig:                     config.TLSConfig,
 		Versions:                      versions,
+		HandshakeTimeout:              handshakeTimeout,
 		RequestConnectionIDTruncation: config.RequestConnectionIDTruncation,
 	}
 }

client_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"net"
+	"time"
 
 	"github.com/lucas-clemente/quic-go/protocol"
 	"github.com/lucas-clemente/quic-go/qerr"
@@ -153,9 +154,21 @@ var _ = Describe("Client", func() {
 			close(done)
 		})
 
-		It("uses all supported versions, if none are specified in the quic.Config", func() {
+		It("setups with the right values", func() {
+			config := &Config{
+				HandshakeTimeout:              1337 * time.Minute,
+				RequestConnectionIDTruncation: true,
+			}
+			c := populateClientConfig(config)
+			Expect(c.HandshakeTimeout).To(Equal(1337 * time.Minute))
+			Expect(c.RequestConnectionIDTruncation).To(BeTrue())
+		})
+
+		It("fills in default values if options are not set in the Config", func() {
 			c := populateClientConfig(&Config{})
 			Expect(c.Versions).To(Equal(protocol.SupportedVersions))
+			Expect(c.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout))
+			Expect(c.RequestConnectionIDTruncation).To(BeFalse())
 		})
 
 		It("errors when receiving an invalid first packet from the server", func(done Done) {
@@ -310,7 +323,7 @@ var _ = Describe("Client", func() {
 		Expect(cconn.(*conn).pconn).To(Equal(packetConn))
 		Expect(hostname).To(Equal("quic.clemente.io"))
 		Expect(version).To(Equal(cl.version))
-		Expect(conf).To(Equal(config))
+		Expect(conf.Versions).To(Equal(config.Versions))
 		close(done)
 	})
 

integrationtests/handshake/rtt.go

@@ -120,4 +120,15 @@ var _ = Describe("Handshake integration tets", func() {
 		Expect(err).To(HaveOccurred())
 		Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.CryptoTooManyRejects))
 	})
+
+	It("doesn't complete the handshake when the handshake timeout is too short", func() {
+		serverConfig.HandshakeTimeout = 2 * rtt
+		runServerAndProxy()
+		_, err := quic.DialAddr(proxy.LocalAddr().String(), &quic.Config{TLSConfig: &tls.Config{InsecureSkipVerify: true}})
+		Expect(err).To(HaveOccurred())
+		Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.HandshakeTimeout))
+		// 2 RTTs during the timeout
+		// plus 1 RTT: the timer starts 0.5 RTTs after sending the first packet, and the CONNECTION_CLOSE needs another 0.5 RTTs to reach the client
+		expectDurationInRTTs(3)
+	})
 })

interface.go

@@ -71,6 +71,10 @@ type Config struct {
 	// This saves 8 bytes in the Public Header in every packet. However, if the IP address of the server changes, the connection cannot be migrated.
 	// Currently only valid for the client.
 	RequestConnectionIDTruncation bool
+	// HandshakeTimeout is the maximum duration that the cryptographic handshake may take.
+	// If the timeout is exceeded, the connection is closed.
+	// If this value is zero, the timeout is set to 10 seconds.
+	HandshakeTimeout time.Duration
 	// AcceptSTK determines if an STK is accepted.
 	// It is called with stk = nil if the client didn't send an STK.
 	// If not set, it verifies that the address matches, and that the STK was issued within the last 24 hours

protocol/server_parameters.go

@@ -128,8 +128,8 @@ const MaxIdleTimeoutServer = 1 * time.Minute
 // MaxIdleTimeoutClient is the idle timeout that the client suggests to the server
 const MaxIdleTimeoutClient = 2 * time.Minute
 
-// MaxTimeForCryptoHandshake is the default timeout for a connection until the crypto handshake succeeds.
-const MaxTimeForCryptoHandshake = 10 * time.Second
+// DefaultHandshakeTimeout is the default timeout for a connection until the crypto handshake succeeds.
+const DefaultHandshakeTimeout = 10 * time.Second
 
 // ClosedSessionDeleteTimeout the server ignores packets arriving on a connection that is already closed
 // after this time all information about the old connection will be deleted

server.go

@@ -106,15 +106,22 @@ func populateServerConfig(config *Config) *Config {
 	if len(versions) == 0 {
 		versions = protocol.SupportedVersions
 	}
+
 	vsa := defaultAcceptSTK
 	if config.AcceptSTK != nil {
 		vsa = config.AcceptSTK
 	}
 
+	handshakeTimeout := protocol.DefaultHandshakeTimeout
+	if config.HandshakeTimeout != 0 {
+		handshakeTimeout = config.HandshakeTimeout
+	}
+
 	return &Config{
-		TLSConfig: config.TLSConfig,
-		Versions:  versions,
-		AcceptSTK: vsa,
+		TLSConfig:        config.TLSConfig,
+		Versions:         versions,
+		HandshakeTimeout: handshakeTimeout,
+		AcceptSTK:        vsa,
 	}
 }
 

server_test.go

@@ -345,9 +345,10 @@ var _ = Describe("Server", func() {
 		supportedVersions := []protocol.VersionNumber{1, 3, 5}
 		acceptSTK := func(_ net.Addr, _ *STK) bool { return true }
 		config := Config{
-			TLSConfig: &tls.Config{},
-			Versions:  supportedVersions,
-			AcceptSTK: acceptSTK,
+			TLSConfig:        &tls.Config{},
+			Versions:         supportedVersions,
+			AcceptSTK:        acceptSTK,
+			HandshakeTimeout: 1337 * time.Hour,
 		}
 		ln, err := Listen(conn, &config)
 		Expect(err).ToNot(HaveOccurred())
@@ -356,6 +357,7 @@ var _ = Describe("Server", func() {
 		Expect(server.sessions).ToNot(BeNil())
 		Expect(server.scfg).ToNot(BeNil())
 		Expect(server.config.Versions).To(Equal(supportedVersions))
+		Expect(server.config.HandshakeTimeout).To(Equal(1337 * time.Hour))
 		Expect(reflect.ValueOf(server.config.AcceptSTK)).To(Equal(reflect.ValueOf(acceptSTK)))
 	})
 
@@ -365,6 +367,7 @@ var _ = Describe("Server", func() {
 		Expect(err).ToNot(HaveOccurred())
 		server := ln.(*server)
 		Expect(server.config.Versions).To(Equal(protocol.SupportedVersions))
+		Expect(server.config.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout))
 		Expect(reflect.ValueOf(server.config.AcceptSTK)).To(Equal(reflect.ValueOf(defaultAcceptSTK)))
 	})
 

session.go

@@ -327,8 +327,8 @@ runLoop:
 		if now.Sub(s.lastNetworkActivityTime) >= s.idleTimeout() {
 			s.close(qerr.Error(qerr.NetworkIdleTimeout, "No recent network activity."))
 		}
-		if !s.handshakeComplete && now.Sub(s.sessionCreationTime) >= protocol.MaxTimeForCryptoHandshake {
-			s.close(qerr.Error(qerr.NetworkIdleTimeout, "Crypto handshake did not complete in time."))
+		if !s.handshakeComplete && now.Sub(s.sessionCreationTime) >= s.config.HandshakeTimeout {
+			s.close(qerr.Error(qerr.HandshakeTimeout, "Crypto handshake did not complete in time."))
 		}
 		s.garbageCollectStreams()
 	}
@@ -354,7 +354,7 @@ func (s *session) maybeResetTimer() {
 		nextDeadline = utils.MinTime(nextDeadline, lossTime)
 	}
 	if !s.handshakeComplete {
-		handshakeDeadline := s.sessionCreationTime.Add(protocol.MaxTimeForCryptoHandshake)
+		handshakeDeadline := s.sessionCreationTime.Add(s.config.HandshakeTimeout)
 		nextDeadline = utils.MinTime(nextDeadline, handshakeDeadline)
 	}
 	if !s.receivedTooManyUndecrytablePacketsTime.IsZero() {

session_test.go

@@ -1396,15 +1396,17 @@ var _ = Describe("Session", func() {
 	Context("timeouts", func() {
 		It("times out due to no network activity", func(done Done) {
 			sess.lastNetworkActivityTime = time.Now().Add(-time.Hour)
-			sess.run() // Would normally not return
+			err := sess.run() // Would normally not return
+			Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.NetworkIdleTimeout))
 			Expect(mconn.written[0]).To(ContainSubstring("No recent network activity."))
 			Expect(sess.runClosed).To(BeClosed())
 			close(done)
 		})
 
 		It("times out due to non-completed crypto handshake", func(done Done) {
-			sess.sessionCreationTime = time.Now().Add(-time.Hour)
-			sess.run() // Would normally not return
+			sess.sessionCreationTime = time.Now().Add(-protocol.DefaultHandshakeTimeout).Add(-time.Second)
+			err := sess.run() // Would normally not return
+			Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.HandshakeTimeout))
 			Expect(mconn.written[0]).To(ContainSubstring("Crypto handshake did not complete in time."))
 			Expect(sess.runClosed).To(BeClosed())
 			close(done)
@@ -1414,7 +1416,8 @@ var _ = Describe("Session", func() {
 			sess.lastNetworkActivityTime = time.Now().Add(-time.Minute)
 			cpm.idleTime = 99999 * time.Second
 			sess.packer.connectionParameters = sess.connectionParameters
-			sess.run() // Would normally not return
+			err := sess.run() // Would normally not return
+			Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.NetworkIdleTimeout))
 			Expect(mconn.written[0]).To(ContainSubstring("No recent network activity."))
 			Expect(sess.runClosed).To(BeClosed())
 			close(done)
@@ -1424,7 +1427,8 @@ var _ = Describe("Session", func() {
 			close(aeadChanged)
 			cpm.idleTime = 0 * time.Millisecond
 			sess.packer.connectionParameters = sess.connectionParameters
-			sess.run() // Would normally not return
+			err := sess.run() // Would normally not return
+			Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.NetworkIdleTimeout))
 			Expect(mconn.written[0]).To(ContainSubstring("No recent network activity."))
 			Expect(sess.runClosed).To(BeClosed())
 			close(done)