From 556e5c555981e1e70d2f88e8657aeaf51e11ff5f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 22 Mar 2018 18:48:26 +0000 Subject: [PATCH 1/4] implement parsing and writing of the HANDSHAKE_DONE frame --- internal/wire/frame_parser.go | 2 ++ internal/wire/frame_parser_test.go | 10 ++++++++++ internal/wire/handshake_done_frame.go | 28 +++++++++++++++++++++++++++ 3 files changed, 40 insertions(+) create mode 100644 internal/wire/handshake_done_frame.go diff --git a/internal/wire/frame_parser.go b/internal/wire/frame_parser.go index fbd66d44..4488ecc2 100644 --- a/internal/wire/frame_parser.go +++ b/internal/wire/frame_parser.go @@ -85,6 +85,8 @@ func (p *frameParser) parseFrame(r *bytes.Reader, typeByte byte, encLevel protoc frame, err = parsePathResponseFrame(r, p.version) case 0x1c, 0x1d: frame, err = parseConnectionCloseFrame(r, p.version) + case 0x1e: + frame, err = parseHandshakeDoneFrame(r, p.version) default: err = errors.New("unknown frame type") } diff --git a/internal/wire/frame_parser_test.go b/internal/wire/frame_parser_test.go index 1d204bdf..f3990b72 100644 --- a/internal/wire/frame_parser_test.go +++ b/internal/wire/frame_parser_test.go @@ -271,6 +271,15 @@ var _ = Describe("Frame parsing", func() { Expect(frame).To(Equal(f)) }) + It("unpacks HANDSHAKE_DONE frames", func() { + f := &HandshakeDoneFrame{} + buf := &bytes.Buffer{} + Expect(f.Write(buf, versionIETFFrames)).To(Succeed()) + frame, err := parser.ParseNext(bytes.NewReader(buf.Bytes()), protocol.Encryption1RTT) + Expect(err).ToNot(HaveOccurred()) + Expect(frame).To(Equal(f)) + }) + It("errors on invalid type", func() { _, err := parser.ParseNext(bytes.NewReader([]byte{0x42}), protocol.Encryption1RTT) Expect(err).To(MatchError("FRAME_ENCODING_ERROR (frame type: 0x42): unknown frame type")) @@ -308,6 +317,7 @@ var _ = Describe("Frame parsing", func() { &PathChallengeFrame{}, &PathResponseFrame{}, &ConnectionCloseFrame{}, + &HandshakeDoneFrame{}, } var framesSerialized [][]byte diff --git a/internal/wire/handshake_done_frame.go b/internal/wire/handshake_done_frame.go new file mode 100644 index 00000000..158d659f --- /dev/null +++ b/internal/wire/handshake_done_frame.go @@ -0,0 +1,28 @@ +package wire + +import ( + "bytes" + + "github.com/lucas-clemente/quic-go/internal/protocol" +) + +// A HandshakeDoneFrame is a HANDSHAKE_DONE frame +type HandshakeDoneFrame struct{} + +// ParseHandshakeDoneFrame parses a HandshakeDone frame +func parseHandshakeDoneFrame(r *bytes.Reader, _ protocol.VersionNumber) (*HandshakeDoneFrame, error) { + if _, err := r.ReadByte(); err != nil { + return nil, err + } + return &HandshakeDoneFrame{}, nil +} + +func (f *HandshakeDoneFrame) Write(b *bytes.Buffer, _ protocol.VersionNumber) error { + b.WriteByte(0x1e) + return nil +} + +// Length of a written frame +func (f *HandshakeDoneFrame) Length(_ protocol.VersionNumber) protocol.ByteCount { + return 1 +} From 08ec2f69fc09405edb6edaff237dbc5ae3a90683 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 22 Nov 2019 10:43:24 +0800 Subject: [PATCH 2/4] send a HANDSHAKE_DONE frame after handshake completion (as a server) --- session.go | 7 +++---- session_test.go | 5 ++++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/session.go b/session.go index 68d7407b..a3efa271 100644 --- a/session.go +++ b/session.go @@ -603,16 +603,14 @@ func (s *session) handleHandshakeComplete() { s.connIDGenerator.SetHandshakeComplete() s.sentPacketHandler.SetHandshakeComplete() - // The client completes the handshake first (after sending the CFIN). - // We need to make sure it learns about the server completing the handshake, - // in order to stop retransmitting handshake packets. - // They will stop retransmitting handshake packets when receiving the first 1-RTT packet. + if s.perspective == protocol.PerspectiveServer { token, err := s.tokenGenerator.NewToken(s.conn.RemoteAddr()) if err != nil { s.closeLocal(err) } s.queueControlFrame(&wire.NewTokenFrame{Token: token}) + s.queueControlFrame(&wire.HandshakeDoneFrame{}) } } @@ -855,6 +853,7 @@ func (s *session) handleFrame(f wire.Frame, pn protocol.PacketNumber, encLevel p err = s.handleNewConnectionIDFrame(frame) case *wire.RetireConnectionIDFrame: err = s.handleRetireConnectionIDFrame(frame) + case *wire.HandshakeDoneFrame: default: err = fmt.Errorf("unexpected frame type: %s", reflect.ValueOf(&frame).Elem().Type().Name()) } diff --git a/session_test.go b/session_test.go index b277b732..ede06093 100644 --- a/session_test.go +++ b/session_test.go @@ -1239,10 +1239,13 @@ var _ = Describe("Session", func() { Eventually(sess.Context().Done()).Should(BeClosed()) }) - It("sends a 1-RTT packet when the handshake completes", func() { + It("sends a HANDSHAKE_DONE frame when the handshake completes", func() { done := make(chan struct{}) sessionRunner.EXPECT().Retire(clientDestConnID) packer.EXPECT().PackPacket().DoAndReturn(func() (*packedPacket, error) { + frames, _ := sess.framer.AppendControlFrames(nil, protocol.MaxByteCount) + Expect(frames).ToNot(BeEmpty()) + Expect(frames[0].Frame).To(BeEquivalentTo(&wire.HandshakeDoneFrame{})) defer close(done) return &packedPacket{ header: &wire.ExtendedHeader{}, From f7fd5d2848f5d88a58abdecf8e4be846193e506f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 22 Nov 2019 10:58:16 +0800 Subject: [PATCH 3/4] drop Handshake keys as soon as the handshake completes (as a server) --- internal/handshake/crypto_setup.go | 9 +++++++++ internal/handshake/interface.go | 1 + internal/mocks/crypto_setup.go | 12 ++++++++++++ session.go | 2 ++ session_test.go | 3 +++ 5 files changed, 27 insertions(+) diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index f698db66..52446af9 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -563,6 +563,15 @@ func (h *cryptoSetup) dropInitialKeys() { h.logger.Debugf("Dropping Initial keys.") } +func (h *cryptoSetup) DropHandshakeKeys() { + h.mutex.Lock() + h.handshakeOpener = nil + h.handshakeSealer = nil + h.mutex.Unlock() + h.runner.DropKeys(protocol.EncryptionHandshake) + h.logger.Debugf("Dropping Handshake keys.") +} + func (h *cryptoSetup) GetInitialSealer() (LongHeaderSealer, error) { h.mutex.Lock() defer h.mutex.Unlock() diff --git a/internal/handshake/interface.go b/internal/handshake/interface.go index 1baee25a..1159266a 100644 --- a/internal/handshake/interface.go +++ b/internal/handshake/interface.go @@ -73,6 +73,7 @@ type CryptoSetup interface { HandleMessage([]byte, protocol.EncryptionLevel) bool SetLargest1RTTAcked(protocol.PacketNumber) + DropHandshakeKeys() ConnectionState() tls.ConnectionState GetInitialOpener() (LongHeaderOpener, error) diff --git a/internal/mocks/crypto_setup.go b/internal/mocks/crypto_setup.go index af24ed46..411bbd04 100644 --- a/internal/mocks/crypto_setup.go +++ b/internal/mocks/crypto_setup.go @@ -76,6 +76,18 @@ func (mr *MockCryptoSetupMockRecorder) ConnectionState() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConnectionState", reflect.TypeOf((*MockCryptoSetup)(nil).ConnectionState)) } +// DropHandshakeKeys mocks base method +func (m *MockCryptoSetup) DropHandshakeKeys() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DropHandshakeKeys") +} + +// DropHandshakeKeys indicates an expected call of DropHandshakeKeys +func (mr *MockCryptoSetupMockRecorder) DropHandshakeKeys() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DropHandshakeKeys", reflect.TypeOf((*MockCryptoSetup)(nil).DropHandshakeKeys)) +} + // Get1RTTOpener mocks base method func (m *MockCryptoSetup) Get1RTTOpener() (handshake.ShortHeaderOpener, error) { m.ctrl.T.Helper() diff --git a/session.go b/session.go index a3efa271..4665b641 100644 --- a/session.go +++ b/session.go @@ -51,6 +51,7 @@ type cryptoStreamHandler interface { RunHandshake() ChangeConnectionID(protocol.ConnectionID) SetLargest1RTTAcked(protocol.PacketNumber) + DropHandshakeKeys() io.Closer ConnectionState() tls.ConnectionState } @@ -610,6 +611,7 @@ func (s *session) handleHandshakeComplete() { s.closeLocal(err) } s.queueControlFrame(&wire.NewTokenFrame{Token: token}) + s.cryptoStreamHandler.DropHandshakeKeys() s.queueControlFrame(&wire.HandshakeDoneFrame{}) } } diff --git a/session_test.go b/session_test.go index ede06093..c48cd4a8 100644 --- a/session_test.go +++ b/session_test.go @@ -1204,6 +1204,7 @@ var _ = Describe("Session", func() { defer GinkgoRecover() <-finishHandshake cryptoSetup.EXPECT().RunHandshake() + cryptoSetup.EXPECT().DropHandshakeKeys() close(sess.handshakeCompleteChan) sess.run() }() @@ -1256,6 +1257,7 @@ var _ = Describe("Session", func() { go func() { defer GinkgoRecover() cryptoSetup.EXPECT().RunHandshake() + cryptoSetup.EXPECT().DropHandshakeKeys() close(sess.handshakeCompleteChan) sess.run() }() @@ -1506,6 +1508,7 @@ var _ = Describe("Session", func() { go func() { defer GinkgoRecover() cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) + cryptoSetup.EXPECT().DropHandshakeKeys().MaxTimes(1) close(sess.handshakeCompleteChan) err := sess.run() nerr, ok := err.(net.Error) From 12922bdec9e6683f535e81fd8c809e988251f113 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 22 Nov 2019 11:06:34 +0800 Subject: [PATCH 4/4] drop Handshake keys when receiving HANDSHAKE_DONE (as a client) --- internal/handshake/crypto_setup.go | 21 ++++++++++----------- session.go | 9 +++++++++ session_test.go | 9 +++++++++ 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index 52446af9..9c28683e 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -221,13 +221,6 @@ func (h *cryptoSetup) ChangeConnectionID(id protocol.ConnectionID) { func (h *cryptoSetup) SetLargest1RTTAcked(pn protocol.PacketNumber) { h.aead.SetLargestAcked(pn) - // drop handshake keys - if h.handshakeOpener != nil { - h.handshakeOpener = nil - h.handshakeSealer = nil - h.logger.Debugf("Dropping Handshake keys.") - h.runner.DropKeys(protocol.EncryptionHandshake) - } } func (h *cryptoSetup) RunHandshake() { @@ -564,12 +557,18 @@ func (h *cryptoSetup) dropInitialKeys() { } func (h *cryptoSetup) DropHandshakeKeys() { + var dropped bool h.mutex.Lock() - h.handshakeOpener = nil - h.handshakeSealer = nil + if h.handshakeOpener != nil { + h.handshakeOpener = nil + h.handshakeSealer = nil + dropped = true + } h.mutex.Unlock() - h.runner.DropKeys(protocol.EncryptionHandshake) - h.logger.Debugf("Dropping Handshake keys.") + if dropped { + h.runner.DropKeys(protocol.EncryptionHandshake) + h.logger.Debugf("Dropping Handshake keys.") + } } func (h *cryptoSetup) GetInitialSealer() (LongHeaderSealer, error) { diff --git a/session.go b/session.go index 4665b641..3b4d49c6 100644 --- a/session.go +++ b/session.go @@ -856,6 +856,7 @@ func (s *session) handleFrame(f wire.Frame, pn protocol.PacketNumber, encLevel p case *wire.RetireConnectionIDFrame: err = s.handleRetireConnectionIDFrame(frame) case *wire.HandshakeDoneFrame: + err = s.handleHandshakeDoneFrame() default: err = fmt.Errorf("unexpected frame type: %s", reflect.ValueOf(&frame).Elem().Type().Name()) } @@ -974,6 +975,14 @@ func (s *session) handleRetireConnectionIDFrame(f *wire.RetireConnectionIDFrame) return s.connIDGenerator.Retire(f.SequenceNumber) } +func (s *session) handleHandshakeDoneFrame() error { + if s.perspective == protocol.PerspectiveServer { + return qerr.Error(qerr.ProtocolViolation, "received a HANDSHAKE_DONE frame") + } + s.cryptoStreamHandler.DropHandshakeKeys() + return nil +} + func (s *session) handleAckFrame(frame *wire.AckFrame, pn protocol.PacketNumber, encLevel protocol.EncryptionLevel) error { if err := s.sentPacketHandler.ReceivedAck(frame, pn, encLevel, s.lastPacketReceivedTime); err != nil { return err diff --git a/session_test.go b/session_test.go index c48cd4a8..848227e0 100644 --- a/session_test.go +++ b/session_test.go @@ -404,6 +404,10 @@ var _ = Describe("Session", func() { Expect(sess.handleFrame(ccf, 0, protocol.EncryptionUnspecified)).To(Succeed()) Eventually(sess.Context().Done()).Should(BeClosed()) }) + + It("errors on HANDSHAKE_DONE frames", func() { + Expect(sess.handleHandshakeDoneFrame()).To(MatchError("PROTOCOL_VIOLATION: received a HANDSHAKE_DONE frame")) + }) }) It("tells its versions", func() { @@ -1734,6 +1738,11 @@ var _ = Describe("Client Session", func() { Expect(sess.handleSinglePacket(&receivedPacket{buffer: getPacketBuffer()}, hdr)).To(BeTrue()) }) + It("handles HANDSHAKE_DONE frames", func() { + cryptoSetup.EXPECT().DropHandshakeKeys() + Expect(sess.handleHandshakeDoneFrame()).To(Succeed()) + }) + Context("handling tokens", func() { var mockTokenStore *MockTokenStore