diff --git a/Changelog.md b/Changelog.md index 4365a9ea..457a0a6f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,9 @@ # Changelog +## v0.20.0 (unreleased) + +- Remove the `quic.Config.HandshakeTimeout`. Introduce a `quic.Config.HandshakeIdleTimeout`. + ## v0.17.1 (2020-06-20) - Supports QUIC WG draft-29. diff --git a/client_test.go b/client_test.go index bbdf35f5..68bcc92b 100644 --- a/client_test.go +++ b/client_test.go @@ -140,7 +140,7 @@ var _ = Describe("Client", func() { sess.EXPECT().HandshakeComplete().Return(context.Background()) return sess } - _, err := DialAddr("localhost:17890", tlsConf, &Config{HandshakeTimeout: time.Millisecond}) + _, err := DialAddr("localhost:17890", tlsConf, &Config{HandshakeIdleTimeout: time.Millisecond}) Expect(err).ToNot(HaveOccurred()) Eventually(remoteAddrChan).Should(Receive(Equal("127.0.0.1:17890"))) }) @@ -458,7 +458,7 @@ var _ = Describe("Client", func() { It("setups with the right values", func() { tokenStore := NewLRUTokenStore(10, 4) config := &Config{ - HandshakeTimeout: 1337 * time.Minute, + HandshakeIdleTimeout: 1337 * time.Minute, MaxIdleTimeout: 42 * time.Hour, MaxIncomingStreams: 1234, MaxIncomingUniStreams: 4321, @@ -467,7 +467,7 @@ var _ = Describe("Client", func() { TokenStore: tokenStore, } c := populateClientConfig(config, false) - Expect(c.HandshakeTimeout).To(Equal(1337 * time.Minute)) + Expect(c.HandshakeIdleTimeout).To(Equal(1337 * time.Minute)) Expect(c.MaxIdleTimeout).To(Equal(42 * time.Hour)) Expect(c.MaxIncomingStreams).To(BeEquivalentTo(1234)) Expect(c.MaxIncomingUniStreams).To(BeEquivalentTo(4321)) @@ -513,7 +513,7 @@ var _ = Describe("Client", func() { It("fills in default values if options are not set in the Config", func() { c := populateClientConfig(&Config{}, false) Expect(c.Versions).To(Equal(protocol.SupportedVersions)) - Expect(c.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout)) + Expect(c.HandshakeIdleTimeout).To(Equal(protocol.DefaultHandshakeIdleTimeout)) Expect(c.MaxIdleTimeout).To(Equal(protocol.DefaultIdleTimeout)) }) }) diff --git a/config.go b/config.go index 7e211e7b..c8479c5c 100644 --- a/config.go +++ b/config.go @@ -2,6 +2,9 @@ package quic import ( "errors" + "time" + + "github.com/lucas-clemente/quic-go/internal/utils" "github.com/lucas-clemente/quic-go/internal/protocol" ) @@ -12,6 +15,10 @@ func (c *Config) Clone() *Config { return © } +func (c *Config) handshakeTimeout() time.Duration { + return utils.MaxDuration(protocol.DefaultHandshakeTimeout, 2*c.HandshakeIdleTimeout) +} + func validateConfig(config *Config) error { if config == nil { return nil @@ -56,9 +63,9 @@ func populateConfig(config *Config) *Config { if len(versions) == 0 { versions = protocol.SupportedVersions } - handshakeTimeout := protocol.DefaultHandshakeTimeout - if config.HandshakeTimeout != 0 { - handshakeTimeout = config.HandshakeTimeout + handshakeIdleTimeout := protocol.DefaultHandshakeIdleTimeout + if config.HandshakeIdleTimeout != 0 { + handshakeIdleTimeout = config.HandshakeIdleTimeout } idleTimeout := protocol.DefaultIdleTimeout if config.MaxIdleTimeout != 0 { @@ -87,7 +94,7 @@ func populateConfig(config *Config) *Config { return &Config{ Versions: versions, - HandshakeTimeout: handshakeTimeout, + HandshakeIdleTimeout: handshakeIdleTimeout, MaxIdleTimeout: idleTimeout, AcceptToken: config.AcceptToken, KeepAlive: config.KeepAlive, diff --git a/config_test.go b/config_test.go index 71aa3e76..eb9a04cb 100644 --- a/config_test.go +++ b/config_test.go @@ -51,7 +51,7 @@ var _ = Describe("Config", func() { f.Set(reflect.ValueOf([]VersionNumber{1, 2, 3})) case "ConnectionIDLength": f.Set(reflect.ValueOf(8)) - case "HandshakeTimeout": + case "HandshakeIdleTimeout": f.Set(reflect.ValueOf(time.Second)) case "MaxIdleTimeout": f.Set(reflect.ValueOf(time.Hour)) @@ -77,6 +77,17 @@ var _ = Describe("Config", func() { } return c } + + It("uses 10s handshake timeout for short handshake idle timeouts", func() { + c := &Config{HandshakeIdleTimeout: time.Second} + Expect(c.handshakeTimeout()).To(Equal(protocol.DefaultHandshakeTimeout)) + }) + + It("uses twice the handshake idle timeouts for the handshake timeout, for long handshake idle timeouts", func() { + c := &Config{HandshakeIdleTimeout: time.Second * 11 / 2} + Expect(c.handshakeTimeout()).To(Equal(11 * time.Second)) + }) + Context("cloning", func() { It("clones function fields", func() { var calledAcceptToken bool @@ -126,7 +137,7 @@ var _ = Describe("Config", func() { It("populates empty fields with default values", func() { c := populateConfig(&Config{}) Expect(c.Versions).To(Equal(protocol.SupportedVersions)) - Expect(c.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout)) + Expect(c.HandshakeIdleTimeout).To(Equal(protocol.DefaultHandshakeIdleTimeout)) Expect(c.MaxReceiveStreamFlowControlWindow).To(BeEquivalentTo(protocol.DefaultMaxReceiveStreamFlowControlWindow)) Expect(c.MaxReceiveConnectionFlowControlWindow).To(BeEquivalentTo(protocol.DefaultMaxReceiveConnectionFlowControlWindow)) Expect(c.MaxIncomingStreams).To(BeEquivalentTo(protocol.DefaultMaxIncomingStreams)) diff --git a/http3/roundtrip_test.go b/http3/roundtrip_test.go index aa136952..d9dbe668 100644 --- a/http3/roundtrip_test.go +++ b/http3/roundtrip_test.go @@ -109,7 +109,7 @@ var _ = Describe("RoundTripper", func() { }) It("uses the quic.Config, if provided", func() { - config := &quic.Config{HandshakeTimeout: time.Millisecond} + config := &quic.Config{HandshakeIdleTimeout: time.Millisecond} var receivedConfig *quic.Config dialAddr = func(addr string, tlsConf *tls.Config, config *quic.Config) (quic.EarlySession, error) { receivedConfig = config @@ -118,7 +118,7 @@ var _ = Describe("RoundTripper", func() { rt.QuicConfig = config _, err := rt.RoundTrip(req1) Expect(err).To(MatchError("handshake error")) - Expect(receivedConfig.HandshakeTimeout).To(Equal(config.HandshakeTimeout)) + Expect(receivedConfig.HandshakeIdleTimeout).To(Equal(config.HandshakeIdleTimeout)) }) It("uses the custom dialer, if provided", func() { diff --git a/http3/server_test.go b/http3/server_test.go index 31b49a00..9d43443e 100644 --- a/http3/server_test.go +++ b/http3/server_test.go @@ -522,7 +522,7 @@ var _ = Describe("Server", func() { } It("uses the quic.Config to start the QUIC server", func() { - conf := &quic.Config{HandshakeTimeout: time.Nanosecond} + conf := &quic.Config{HandshakeIdleTimeout: time.Nanosecond} var receivedConf *quic.Config quicListenAddr = func(addr string, _ *tls.Config, config *quic.Config) (quic.EarlyListener, error) { receivedConf = config diff --git a/integrationtests/self/handshake_drop_test.go b/integrationtests/self/handshake_drop_test.go index 1d17f9df..c9e04cf0 100644 --- a/integrationtests/self/handshake_drop_test.go +++ b/integrationtests/self/handshake_drop_test.go @@ -37,9 +37,9 @@ var _ = Describe("Handshake drop tests", func() { startListenerAndProxy := func(dropCallback quicproxy.DropCallback, doRetry bool, longCertChain bool, version protocol.VersionNumber) { conf := getQuicConfig(&quic.Config{ - MaxIdleTimeout: timeout, - HandshakeTimeout: timeout, - Versions: []protocol.VersionNumber{version}, + MaxIdleTimeout: timeout, + HandshakeIdleTimeout: timeout, + Versions: []protocol.VersionNumber{version}, }) if !doRetry { conf.AcceptToken = func(net.Addr, *quic.Token) bool { return true } @@ -88,9 +88,9 @@ var _ = Describe("Handshake drop tests", func() { fmt.Sprintf("localhost:%d", proxy.LocalPort()), getTLSClientConfig(), getQuicConfig(&quic.Config{ - MaxIdleTimeout: timeout, - HandshakeTimeout: timeout, - Versions: []protocol.VersionNumber{version}, + MaxIdleTimeout: timeout, + HandshakeIdleTimeout: timeout, + Versions: []protocol.VersionNumber{version}, }), ) Expect(err).ToNot(HaveOccurred()) @@ -126,9 +126,9 @@ var _ = Describe("Handshake drop tests", func() { fmt.Sprintf("localhost:%d", proxy.LocalPort()), getTLSClientConfig(), getQuicConfig(&quic.Config{ - MaxIdleTimeout: timeout, - HandshakeTimeout: timeout, - Versions: []protocol.VersionNumber{version}, + MaxIdleTimeout: timeout, + HandshakeIdleTimeout: timeout, + Versions: []protocol.VersionNumber{version}, }), ) Expect(err).ToNot(HaveOccurred()) @@ -159,9 +159,9 @@ var _ = Describe("Handshake drop tests", func() { fmt.Sprintf("localhost:%d", proxy.LocalPort()), getTLSClientConfig(), getQuicConfig(&quic.Config{ - MaxIdleTimeout: timeout, - HandshakeTimeout: timeout, - Versions: []protocol.VersionNumber{version}, + MaxIdleTimeout: timeout, + HandshakeIdleTimeout: timeout, + Versions: []protocol.VersionNumber{version}, }), ) Expect(err).ToNot(HaveOccurred()) diff --git a/integrationtests/self/handshake_rtt_test.go b/integrationtests/self/handshake_rtt_test.go index be95b567..1f0725c6 100644 --- a/integrationtests/self/handshake_rtt_test.go +++ b/integrationtests/self/handshake_rtt_test.go @@ -145,7 +145,7 @@ var _ = Describe("Handshake RTT tests", func() { serverConfig.AcceptToken = func(_ net.Addr, _ *quic.Token) bool { return false } - clientConfig.HandshakeTimeout = 500 * time.Millisecond + clientConfig.HandshakeIdleTimeout = 500 * time.Millisecond runServerAndProxy() _, err := quic.DialAddr( fmt.Sprintf("localhost:%d", proxy.LocalAddr().(*net.UDPAddr).Port), @@ -153,6 +153,8 @@ var _ = Describe("Handshake RTT tests", func() { clientConfig, ) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("Handshake did not complete in time")) + nerr, ok := err.(net.Error) + Expect(ok).To(BeTrue()) + Expect(nerr.Timeout()).To(BeTrue()) }) }) diff --git a/integrationtests/self/mitm_test.go b/integrationtests/self/mitm_test.go index ca897a28..c260b69a 100644 --- a/integrationtests/self/mitm_test.go +++ b/integrationtests/self/mitm_test.go @@ -336,9 +336,9 @@ var _ = Describe("MITM test", func() { fmt.Sprintf("localhost:%d", proxy.LocalPort()), getTLSClientConfig(), getQuicConfig(&quic.Config{ - Versions: []protocol.VersionNumber{version}, - ConnectionIDLength: connIDLen, - HandshakeTimeout: 2 * time.Second, + Versions: []protocol.VersionNumber{version}, + ConnectionIDLength: connIDLen, + HandshakeIdleTimeout: 2 * time.Second, }), ) return err diff --git a/integrationtests/self/timeout_test.go b/integrationtests/self/timeout_test.go index 898dd415..17129a3b 100644 --- a/integrationtests/self/timeout_test.go +++ b/integrationtests/self/timeout_test.go @@ -66,7 +66,7 @@ var _ = Describe("Timeout tests", func() { _, err := quic.DialAddr( "localhost:12345", getTLSClientConfig(), - getQuicConfig(&quic.Config{HandshakeTimeout: 10 * time.Millisecond}), + getQuicConfig(&quic.Config{HandshakeIdleTimeout: 10 * time.Millisecond}), ) errChan <- err }() @@ -431,8 +431,8 @@ var _ = Describe("Timeout tests", func() { fmt.Sprintf("localhost:%d", ln.Addr().(*net.UDPAddr).Port), getTLSClientConfig(), getQuicConfig(&quic.Config{ - HandshakeTimeout: handshakeTimeout, - MaxIdleTimeout: handshakeTimeout, + HandshakeIdleTimeout: handshakeTimeout, + MaxIdleTimeout: handshakeTimeout, }), ) if err != nil { @@ -464,9 +464,9 @@ var _ = Describe("Timeout tests", func() { "localhost:0", getTLSConfig(), getQuicConfig(&quic.Config{ - HandshakeTimeout: handshakeTimeout, - MaxIdleTimeout: handshakeTimeout, - KeepAlive: true, + HandshakeIdleTimeout: handshakeTimeout, + MaxIdleTimeout: handshakeTimeout, + KeepAlive: true, }), ) Expect(err).ToNot(HaveOccurred()) diff --git a/interface.go b/interface.go index a3846d11..99afd92c 100644 --- a/interface.go +++ b/interface.go @@ -217,10 +217,10 @@ type Config struct { // If used for a server, or dialing on a packet conn, a 4 byte connection ID will be used. // When dialing on a packet conn, the ConnectionIDLength value must be the same for every Dial call. ConnectionIDLength int - // 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 + // HandshakeIdleTimeout is the idle timeout before completion of the handshake. + // Specifically, if we don't receive any packet from the peer within this time, the connection attempt is aborted. + // If this value is zero, the timeout is set to 5 seconds. + HandshakeIdleTimeout time.Duration // MaxIdleTimeout is the maximum duration that may pass without any incoming network activity. // The actual value for the idle timeout is the minimum of this value and the peer's. // This value only applies after the handshake has completed. diff --git a/internal/protocol/params.go b/internal/protocol/params.go index a124f3aa..ab2e22fb 100644 --- a/internal/protocol/params.go +++ b/internal/protocol/params.go @@ -98,6 +98,9 @@ const MinRemoteIdleTimeout = 5 * time.Second // DefaultIdleTimeout is the default idle timeout const DefaultIdleTimeout = 30 * time.Second +// DefaultHandshakeIdleTimeout is the default idle timeout used before handshake completion. +const DefaultHandshakeIdleTimeout = 5 * time.Second + // DefaultHandshakeTimeout is the default timeout for a connection until the crypto handshake succeeds. const DefaultHandshakeTimeout = 10 * time.Second diff --git a/server_test.go b/server_test.go index f95a5907..210ad6fc 100644 --- a/server_test.go +++ b/server_test.go @@ -124,7 +124,7 @@ var _ = Describe("Server", func() { Expect(err).ToNot(HaveOccurred()) server := ln.(*baseServer) Expect(server.config.Versions).To(Equal(protocol.SupportedVersions)) - Expect(server.config.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout)) + Expect(server.config.HandshakeIdleTimeout).To(Equal(protocol.DefaultHandshakeIdleTimeout)) Expect(server.config.MaxIdleTimeout).To(Equal(protocol.DefaultIdleTimeout)) Expect(reflect.ValueOf(server.config.AcceptToken)).To(Equal(reflect.ValueOf(defaultAcceptToken))) Expect(server.config.KeepAlive).To(BeFalse()) @@ -136,19 +136,19 @@ var _ = Describe("Server", func() { supportedVersions := []protocol.VersionNumber{protocol.VersionTLS} acceptToken := func(_ net.Addr, _ *Token) bool { return true } config := Config{ - Versions: supportedVersions, - AcceptToken: acceptToken, - HandshakeTimeout: 1337 * time.Hour, - MaxIdleTimeout: 42 * time.Minute, - KeepAlive: true, - StatelessResetKey: []byte("foobar"), + Versions: supportedVersions, + AcceptToken: acceptToken, + HandshakeIdleTimeout: 1337 * time.Hour, + MaxIdleTimeout: 42 * time.Minute, + KeepAlive: true, + StatelessResetKey: []byte("foobar"), } ln, err := Listen(conn, tlsConf, &config) Expect(err).ToNot(HaveOccurred()) server := ln.(*baseServer) Expect(server.sessionHandler).ToNot(BeNil()) Expect(server.config.Versions).To(Equal(supportedVersions)) - Expect(server.config.HandshakeTimeout).To(Equal(1337 * time.Hour)) + Expect(server.config.HandshakeIdleTimeout).To(Equal(1337 * time.Hour)) Expect(server.config.MaxIdleTimeout).To(Equal(42 * time.Minute)) Expect(reflect.ValueOf(server.config.AcceptToken)).To(Equal(reflect.ValueOf(acceptToken))) Expect(server.config.KeepAlive).To(BeTrue()) diff --git a/session.go b/session.go index 4261d042..f29136ea 100644 --- a/session.go +++ b/session.go @@ -587,18 +587,22 @@ runLoop: s.logger.Debugf("Sending a keep-alive PING to keep the connection alive.") s.framer.QueueControlFrame(&wire.PingFrame{}) s.keepAlivePingSent = true - } else if !s.handshakeComplete && now.Sub(s.sessionCreationTime) >= s.config.HandshakeTimeout { + } else if !s.handshakeComplete && now.Sub(s.sessionCreationTime) >= s.config.handshakeTimeout() { if s.tracer != nil { s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonHandshake)) } s.destroyImpl(qerr.NewTimeoutError("Handshake did not complete in time")) continue - } else if s.handshakeComplete && now.Sub(s.idleTimeoutStartTime()) >= s.idleTimeout { - if s.tracer != nil { - s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonIdle)) + } else { + idleTimeoutStartTime := s.idleTimeoutStartTime() + if (!s.handshakeComplete && now.Sub(idleTimeoutStartTime) >= s.config.HandshakeIdleTimeout) || + (s.handshakeComplete && now.Sub(idleTimeoutStartTime) >= s.idleTimeout) { + if s.tracer != nil { + s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonIdle)) + } + s.destroyImpl(qerr.NewTimeoutError("No recent network activity")) + continue } - s.destroyImpl(qerr.NewTimeoutError("No recent network activity")) - continue } if err := s.sendPackets(); err != nil { @@ -646,7 +650,7 @@ func (s *session) nextKeepAliveTime() time.Time { func (s *session) maybeResetTimer() { var deadline time.Time if !s.handshakeComplete { - deadline = s.sessionCreationTime.Add(s.config.HandshakeTimeout) + deadline = s.sessionCreationTime.Add(utils.MinDuration(s.config.handshakeTimeout(), s.config.HandshakeIdleTimeout)) } else { if keepAliveTime := s.nextKeepAliveTime(); !keepAliveTime.IsZero() { deadline = keepAliveTime diff --git a/session_test.go b/session_test.go index 2d42847d..09c513d6 100644 --- a/session_test.go +++ b/session_test.go @@ -1978,6 +1978,7 @@ var _ = Describe("Session", func() { It("does not use the idle timeout before the handshake complete", func() { sess.handshakeComplete = false + sess.config.HandshakeIdleTimeout = 9999 * time.Second sess.config.MaxIdleTimeout = 9999 * time.Second sess.lastPacketReceivedTime = time.Now().Add(-time.Minute) packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(quicErr *qerr.QuicError) (*coalescedPacket, error) { @@ -2007,6 +2008,35 @@ var _ = Describe("Session", func() { Eventually(sess.Context().Done()).Should(BeClosed()) }) + It("closes the session due to the idle timeout before handshake", func() { + sess.config.HandshakeIdleTimeout = 0 + packer.EXPECT().PackCoalescedPacket().AnyTimes() + sessionRunner.EXPECT().Remove(gomock.Any()).AnyTimes() + cryptoSetup.EXPECT().Close() + gomock.InOrder( + tracer.EXPECT().ClosedConnection(gomock.Any()).Do(func(reason logging.CloseReason) { + timeout, ok := reason.Timeout() + Expect(ok).To(BeTrue()) + Expect(timeout).To(Equal(logging.TimeoutReasonIdle)) + }), + tracer.EXPECT().Close(), + ) + done := make(chan struct{}) + sess.handshakeComplete = false + go func() { + defer GinkgoRecover() + cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) + cryptoSetup.EXPECT().GetSessionTicket().MaxTimes(1) + err := sess.run() + nerr, ok := err.(net.Error) + Expect(ok).To(BeTrue()) + Expect(nerr.Timeout()).To(BeTrue()) + Expect(err.Error()).To(ContainSubstring("No recent network activity")) + close(done) + }() + Eventually(done).Should(BeClosed()) + }) + It("closes the session due to the idle timeout after handshake", func() { packer.EXPECT().PackCoalescedPacket().AnyTimes() gomock.InOrder(