From 5208845191ff7edae9bca3dfb7f4d6cac3ceef72 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 15 Mar 2021 13:20:23 +0800 Subject: [PATCH] introduce a logging.CloseReason for version negotiation errors --- integrationtests/self/mitm_test.go | 2 +- logging/close_reason.go | 10 ++++++++++ logging/close_reason_test.go | 21 ++++++++++++++++++++- session.go | 18 ++++++++++++++++-- session_test.go | 10 ++++++++-- 5 files changed, 55 insertions(+), 6 deletions(-) diff --git a/integrationtests/self/mitm_test.go b/integrationtests/self/mitm_test.go index dd7ea54b..860da5af 100644 --- a/integrationtests/self/mitm_test.go +++ b/integrationtests/self/mitm_test.go @@ -371,7 +371,7 @@ var _ = Describe("MITM test", func() { } err := runTest(delayCb) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("No compatible QUIC version found.")) + Expect(err.Error()).To(ContainSubstring("no compatible QUIC version found")) }) // times out, because client doesn't accept subsequent real retry packets from server diff --git a/logging/close_reason.go b/logging/close_reason.go index e35e4742..135a3be4 100644 --- a/logging/close_reason.go +++ b/logging/close_reason.go @@ -13,6 +13,7 @@ type CloseReason struct { timeout *TimeoutReason statelessResetToken *StatelessResetToken + versions []VersionNumber } // NewApplicationCloseReason creates a new CloseReason for an application error. @@ -35,6 +36,11 @@ func NewStatelessResetCloseReason(token StatelessResetToken) CloseReason { return CloseReason{statelessResetToken: &token} } +// NewVersionNegotiationError creates a new CloseReason for a version negotiation error. +func NewVersionNegotiationError(versions []VersionNumber) CloseReason { + return CloseReason{versions: versions} +} + // ApplicationError gets the application error. func (r *CloseReason) ApplicationError() (errorCode ApplicationError, remote bool, ok bool) { if r.applicationError == nil { @@ -66,3 +72,7 @@ func (r *CloseReason) StatelessReset() (token StatelessResetToken, ok bool) { } return *r.statelessResetToken, true } + +func (r *CloseReason) VersionNegotiation() (versions []VersionNumber, ok bool) { + return r.versions, len(r.versions) > 0 +} diff --git a/logging/close_reason_test.go b/logging/close_reason_test.go index 80cef958..6fc352f0 100644 --- a/logging/close_reason_test.go +++ b/logging/close_reason_test.go @@ -26,6 +26,11 @@ var _ = Describe("Close Reason", func() { ExpectWithOffset(1, ok).To(BeFalse()) } + checkNotVN := func(r CloseReason) { + _, ok := r.VersionNegotiation() + ExpectWithOffset(1, ok).To(BeFalse()) + } + It("application errors", func() { r := NewApplicationCloseReason(1337, true) errorCode, remote, ok := r.ApplicationError() @@ -35,6 +40,7 @@ var _ = Describe("Close Reason", func() { checkNotTransportError(r) checkNotStatelessReset(r) checkNotTimeout(r) + checkNotVN(r) }) It("transport errors", func() { @@ -46,6 +52,7 @@ var _ = Describe("Close Reason", func() { checkNotApplicationError(r) checkNotStatelessReset(r) checkNotTimeout(r) + checkNotVN(r) }) It("transport errors", func() { @@ -55,7 +62,7 @@ var _ = Describe("Close Reason", func() { Expect(timeout).To(Equal(TimeoutReasonIdle)) checkNotApplicationError(r) checkNotTransportError(r) - checkNotStatelessReset(r) + checkNotVN(r) }) It("stateless resets", func() { @@ -66,5 +73,17 @@ var _ = Describe("Close Reason", func() { checkNotApplicationError(r) checkNotTransportError(r) checkNotTimeout(r) + checkNotVN(r) + }) + + It("version negotiation errors", func() { + r := NewVersionNegotiationError([]VersionNumber{1, 2, 3}) + vn, ok := r.VersionNegotiation() + Expect(ok).To(BeTrue()) + Expect(vn).To(Equal([]VersionNumber{1, 2, 3})) + checkNotApplicationError(r) + checkNotTransportError(r) + checkNotTimeout(r) + checkNotStatelessReset(r) }) }) diff --git a/session.go b/session.go index aedb16ab..b8a6c768 100644 --- a/session.go +++ b/session.go @@ -123,6 +123,15 @@ func (errCloseForRecreating) Is(target error) bool { return ok } +type errVersionNegotiation struct { + ourVersions []protocol.VersionNumber + theirVersions []protocol.VersionNumber +} + +func (e errVersionNegotiation) Error() string { + return fmt.Sprintf("no compatible QUIC version found (we support %s, server offered %s)", e.ourVersions, e.theirVersions) +} + // A Session is a QUIC session type session struct { // Destination connection ID used during the handshake. @@ -1061,8 +1070,10 @@ func (s *session) handleVersionNegotiationPacket(p *receivedPacket) { } newVersion, ok := protocol.ChooseSupportedVersion(s.config.Versions, supportedVersions) if !ok { - //nolint:stylecheck - s.destroyImpl(fmt.Errorf("No compatible QUIC version found. We support %s, server offered %s.", s.config.Versions, supportedVersions)) + s.destroyImpl(errVersionNegotiation{ + ourVersions: s.config.Versions, + theirVersions: supportedVersions, + }) s.logger.Infof("No compatible QUIC version found.") return } @@ -1425,8 +1436,11 @@ func (s *session) handleCloseError(closeErr closeError) { // timeout errors are logged as soon as they occur (to distinguish between handshake and idle timeouts) if nerr, ok := closeErr.err.(net.Error); !ok || !nerr.Timeout() { var resetErr statelessResetErr + var vnErr errVersionNegotiation if errors.As(closeErr.err, &resetErr) { s.tracer.ClosedConnection(logging.NewStatelessResetCloseReason(resetErr.token)) + } else if errors.As(closeErr.err, &vnErr) { + s.tracer.ClosedConnection(logging.NewVersionNegotiationError(vnErr.theirVersions)) } else if quicErr.IsApplicationError() { s.tracer.ClosedConnection(logging.NewApplicationCloseReason(quicErr.ErrorCode, closeErr.remote)) } else { diff --git a/session_test.go b/session_test.go index 60a4aa23..5c88f70d 100644 --- a/session_test.go +++ b/session_test.go @@ -2630,9 +2630,12 @@ var _ = Describe("Client Session", func() { errChan <- sess.run() }() sessionRunner.EXPECT().Remove(srcConnID).MaxTimes(1) + var closeReason logging.CloseReason gomock.InOrder( tracer.EXPECT().ReceivedVersionNegotiationPacket(gomock.Any(), gomock.Any()), - tracer.EXPECT().ClosedConnection(gomock.Any()), + tracer.EXPECT().ClosedConnection(gomock.Any()).Do(func(r logging.CloseReason) { + closeReason = r + }), tracer.EXPECT().Close(), ) cryptoSetup.EXPECT().Close() @@ -2641,7 +2644,10 @@ var _ = Describe("Client Session", func() { Eventually(errChan).Should(Receive(&err)) Expect(err).To(HaveOccurred()) Expect(err).ToNot(BeAssignableToTypeOf(&errCloseForRecreating{})) - Expect(err.Error()).To(ContainSubstring("No compatible QUIC version found")) + Expect(err.Error()).To(ContainSubstring("no compatible QUIC version found")) + vns, ok := closeReason.VersionNegotiation() + Expect(ok).To(BeTrue()) + Expect(vns).To(ContainElement(logging.VersionNumber(12345678))) }) It("ignores Version Negotiation packets that offer the current version", func() {