From a7f94d89b60eaf2b6a56fb881c9e87c02719c007 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 24 Oct 2018 23:07:03 +0700 Subject: [PATCH] only allow a single retry --- client.go | 22 ++++++------- client_test.go | 36 ++++++++++++--------- integrationtests/self/handshake_rtt_test.go | 3 +- internal/protocol/server_parameters.go | 3 -- server_tls.go | 5 +-- server_tls_test.go | 1 - 6 files changed, 33 insertions(+), 37 deletions(-) diff --git a/client.go b/client.go index 0a26fba93..280c3fcdb 100644 --- a/client.go +++ b/client.go @@ -27,8 +27,7 @@ type client struct { packetHandlers packetHandlerManager - token []byte - numRetries int + token []byte versionNegotiated bool // has the server accepted our version receivedVersionNegotiationPacket bool @@ -478,19 +477,18 @@ func (c *client) handleVersionNegotiationPacket(hdr *wire.Header) error { func (c *client) handleRetryPacket(hdr *wire.Header) { c.logger.Debugf("<- Received Retry") hdr.Log(c.logger) - // A server that performs multiple retries must use a source connection ID of at least 8 bytes. - // Only a server that won't send additional Retries can use shorter connection IDs. - if hdr.OrigDestConnectionID.Len() < protocol.MinConnectionIDLenInitial { - c.logger.Debugf("Received a Retry with a too short Original Destination Connection ID: %d bytes, must have at least %d bytes.", hdr.OrigDestConnectionID.Len(), protocol.MinConnectionIDLenInitial) - return - } if !hdr.OrigDestConnectionID.Equal(c.destConnID) { - c.logger.Debugf("Received spoofed Retry. Original Destination Connection ID: %s, expected: %s", hdr.OrigDestConnectionID, c.destConnID) + c.logger.Debugf("Ignoring spoofed Retry. Original Destination Connection ID: %s, expected: %s", hdr.OrigDestConnectionID, c.destConnID) return } - c.numRetries++ - if c.numRetries > protocol.MaxRetries { - c.session.destroy(qerr.CryptoTooManyRejects) + if hdr.SrcConnectionID.Equal(c.destConnID) { + c.logger.Debugf("Ignoring Retry, since the server didn't change the Source Connection ID.") + return + } + // If a token is already set, this means that we already received a Retry from the server. + // Ignore this Retry packet. + if len(c.token) > 0 { + c.logger.Debugf("Ignoring Retry, since a Retry was already received.") return } c.destConnID = hdr.SrcConnectionID diff --git a/client_test.go b/client_test.go index 287138c12..bf3fff08c 100644 --- a/client_test.go +++ b/client_test.go @@ -598,7 +598,7 @@ var _ = Describe("Client", func() { Expect(sessions).To(BeEmpty()) }) - It("only accepts 3 retries", func() { + It("only accepts a single retry", func() { manager := NewMockPacketHandlerManager(mockCtrl) manager.EXPECT().Add(gomock.Any(), gomock.Any()).Do(func(id protocol.ConnectionID, handler packetHandler) { go handler.handlePacket(&receivedPacket{ @@ -606,7 +606,7 @@ var _ = Describe("Client", func() { IsLongHeader: true, Type: protocol.PacketTypeRetry, Token: []byte("foobar"), - SrcConnectionID: connID, + SrcConnectionID: protocol.ConnectionID{1, 2, 3, 4}, DestConnectionID: id, OrigDestConnectionID: connID, Version: protocol.VersionTLS, @@ -619,18 +619,23 @@ var _ = Describe("Client", func() { config := &Config{Versions: []protocol.VersionNumber{protocol.VersionTLS}} cl.config = config - sessions := make(chan quicSession, protocol.MaxRetries+1) - for i := 0; i < protocol.MaxRetries+1; i++ { - run := make(chan error) - sess := NewMockQuicSession(mockCtrl) - sess.EXPECT().run().DoAndReturn(func() error { - return <-run - }) - sess.EXPECT().destroy(gomock.Any()).Do(func(e error) { - run <- e - }) - sessions <- sess - } + sessions := make(chan quicSession, 2) + run := make(chan error) + sess := NewMockQuicSession(mockCtrl) + sess.EXPECT().run().DoAndReturn(func() error { + defer GinkgoRecover() + var err error + Eventually(run).Should(Receive(&err)) + return err + }) + sess.EXPECT().destroy(gomock.Any()).Do(func(e error) { + run <- e + }) + sessions <- sess + doneErr := errors.New("nothing to do") + sess = NewMockQuicSession(mockCtrl) + sess.EXPECT().run().Return(doneErr) + sessions <- sess newTLSClientSession = func( _ connection, @@ -648,8 +653,7 @@ var _ = Describe("Client", func() { return <-sessions, nil } _, err := Dial(packetConn, addr, "quic.clemente.io:1337", nil, config) - Expect(err).To(HaveOccurred()) - Expect(err.(qerr.ErrorCode)).To(Equal(qerr.CryptoTooManyRejects)) + Expect(err).To(MatchError(doneErr)) Expect(sessions).To(BeEmpty()) }) }) diff --git a/integrationtests/self/handshake_rtt_test.go b/integrationtests/self/handshake_rtt_test.go index 67cc74ae1..f58878a12 100644 --- a/integrationtests/self/handshake_rtt_test.go +++ b/integrationtests/self/handshake_rtt_test.go @@ -200,6 +200,7 @@ var _ = Describe("Handshake RTT tests", func() { serverConfig.AcceptCookie = func(_ net.Addr, _ *quic.Cookie) bool { return false } + clientConfig.HandshakeTimeout = 500 * time.Millisecond runServerAndProxy() _, err := quic.DialAddr( proxy.LocalAddr().String(), @@ -207,7 +208,7 @@ var _ = Describe("Handshake RTT tests", func() { clientConfig, ) Expect(err).To(HaveOccurred()) - Expect(err.(qerr.ErrorCode)).To(Equal(qerr.CryptoTooManyRejects)) + Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.HandshakeTimeout)) }) }) }) diff --git a/internal/protocol/server_parameters.go b/internal/protocol/server_parameters.go index aa61b3d75..aa92c8223 100644 --- a/internal/protocol/server_parameters.go +++ b/internal/protocol/server_parameters.go @@ -149,6 +149,3 @@ const MinPacingDelay time.Duration = 100 * time.Microsecond // DefaultConnectionIDLength is the connection ID length that is used for multiplexed connections // if no other value is configured. const DefaultConnectionIDLength = 4 - -// MaxRetries is the maximum number of Retries a client will do before failing the connection. -const MaxRetries = 3 diff --git a/server_tls.go b/server_tls.go index 01508df3c..437bee2fc 100644 --- a/server_tls.go +++ b/server_tls.go @@ -118,9 +118,6 @@ func (s *serverTLS) handleInitialImpl(p *receivedPacket) (quicSession, protocol. mconf := s.mintConf.Clone() mconf.ExtensionHandler = extHandler - // A server is allowed to perform multiple Retries. - // It doesn't make much sense, but it's something that our API allows. - // In that case it must use a source connection ID of at least 8 bytes. connID, err := protocol.GenerateConnectionID(s.config.ConnectionIDLength) if err != nil { return nil, nil, err @@ -152,7 +149,7 @@ func (s *serverTLS) sendRetry(remoteAddr net.Addr, hdr *wire.Header) error { if err != nil { return err } - connID, err := protocol.GenerateConnectionIDForInitial() + connID, err := protocol.GenerateConnectionID(s.config.ConnectionIDLength) if err != nil { return err } diff --git a/server_tls_test.go b/server_tls_test.go index 16326ed57..4a2024cfc 100644 --- a/server_tls_test.go +++ b/server_tls_test.go @@ -79,7 +79,6 @@ var _ = Describe("Stateless TLS handling", func() { replyHdr := parseHeader(conn.dataWritten.Bytes()) Expect(replyHdr.Type).To(Equal(protocol.PacketTypeRetry)) Expect(replyHdr.SrcConnectionID).ToNot(Equal(hdr.DestConnectionID)) - Expect(replyHdr.SrcConnectionID.Len()).To(BeNumerically(">=", protocol.MinConnectionIDLenInitial)) Expect(replyHdr.DestConnectionID).To(Equal(hdr.SrcConnectionID)) Expect(replyHdr.OrigDestConnectionID).To(Equal(hdr.DestConnectionID)) Expect(replyHdr.Token).ToNot(BeEmpty())