From 723e18c78c34563be09c2fe4722c686d4b4c1e1f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 28 Nov 2018 11:41:39 +0700 Subject: [PATCH 1/3] don't treat handshake retransmissions separately --- packet_packer.go | 23 +---------------------- packet_packer_test.go | 25 ++++--------------------- session.go | 7 +------ 3 files changed, 6 insertions(+), 49 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index e9f37390c..4cf9abfbf 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -167,13 +167,9 @@ func (p *packetPacker) MaybePackAckPacket() (*packedPacket, error) { // For packets sent after completion of the handshake, it might happen that 2 packets have to be sent. // This can happen e.g. when a longer packet number is used in the header. func (p *packetPacker) PackRetransmission(packet *ackhandler.Packet) ([]*packedPacket, error) { - if packet.EncryptionLevel != protocol.Encryption1RTT { - p, err := p.packHandshakeRetransmission(packet) - return []*packedPacket{p}, err - } - var controlFrames []wire.Frame var streamFrames []*wire.StreamFrame + // TODO: treat CRYPTO frames separately for _, f := range packet.Frames { if sf, ok := f.(*wire.StreamFrame); ok { sf.DataLenPresent = true @@ -243,23 +239,6 @@ func (p *packetPacker) PackRetransmission(packet *ackhandler.Packet) ([]*packedP return packets, nil } -// packHandshakeRetransmission retransmits a handshake packet -func (p *packetPacker) packHandshakeRetransmission(packet *ackhandler.Packet) (*packedPacket, error) { - sealer, err := p.cryptoSetup.GetSealerWithEncryptionLevel(packet.EncryptionLevel) - if err != nil { - return nil, err - } - header := p.getHeader(packet.EncryptionLevel) - header.Type = packet.PacketType - raw, err := p.writeAndSealPacket(header, packet.Frames, sealer) - return &packedPacket{ - header: header, - raw: raw, - frames: packet.Frames, - encryptionLevel: packet.EncryptionLevel, - }, err -} - // PackPacket packs a new packet // the other controlFrames are sent in the next packet, but might be queued and sent in the next packet if the packet would overflow MaxPacketSize otherwise func (p *packetPacker) PackPacket() (*packedPacket, error) { diff --git a/packet_packer_test.go b/packet_packer_test.go index 7ba2a8e54..bbe6f433b 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -748,40 +748,23 @@ var _ = Describe("Packet packer", func() { sf := &wire.StreamFrame{Data: []byte("foobar")} It("packs a retransmission with the right encryption level", func() { + f := &wire.CryptoFrame{Data: []byte("foo")} pnManager.EXPECT().PeekPacketNumber().Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber().Return(protocol.PacketNumber(0x42)) sealingManager.EXPECT().GetSealerWithEncryptionLevel(protocol.EncryptionInitial).Return(sealer, nil) packet := &ackhandler.Packet{ PacketType: protocol.PacketTypeHandshake, EncryptionLevel: protocol.EncryptionInitial, - Frames: []wire.Frame{sf}, + Frames: []wire.Frame{f}, } p, err := packer.PackRetransmission(packet) Expect(err).ToNot(HaveOccurred()) Expect(p).To(HaveLen(1)) - Expect(p[0].header.Type).To(Equal(protocol.PacketTypeHandshake)) - Expect(p[0].frames).To(Equal([]wire.Frame{sf})) + Expect(p[0].header.Type).To(Equal(protocol.PacketTypeInitial)) + Expect(p[0].frames).To(Equal([]wire.Frame{f})) Expect(p[0].encryptionLevel).To(Equal(protocol.EncryptionInitial)) }) - // this should never happen, since non forward-secure packets are limited to a size smaller than MaxPacketSize, such that it is always possible to retransmit them without splitting the StreamFrame - It("refuses to send a packet larger than MaxPacketSize", func() { - pnManager.EXPECT().PeekPacketNumber().Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) - sealingManager.EXPECT().GetSealerWithEncryptionLevel(gomock.Any()).Return(sealer, nil) - packet := &ackhandler.Packet{ - EncryptionLevel: protocol.EncryptionHandshake, - Frames: []wire.Frame{ - &wire.StreamFrame{ - StreamID: 1, - Data: bytes.Repeat([]byte{'f'}, int(maxPacketSize)), - }, - }, - } - _, err := packer.PackRetransmission(packet) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("PacketPacker BUG: packet too large")) - }) - It("packs a retransmission for an Initial packet", func() { pnManager.EXPECT().PeekPacketNumber().Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber().Return(protocol.PacketNumber(0x42)) diff --git a/session.go b/session.go index 93c2107cf..7880697bc 100644 --- a/session.go +++ b/session.go @@ -903,12 +903,7 @@ func (s *session) maybeSendRetransmission() (bool, error) { break } - if retransmitPacket.EncryptionLevel != protocol.Encryption1RTT { - s.logger.Debugf("Dequeueing handshake retransmission for packet 0x%x", retransmitPacket.PacketNumber) - } else { - s.logger.Debugf("Dequeueing retransmission for packet 0x%x", retransmitPacket.PacketNumber) - } - + s.logger.Debugf("Dequeueing retransmission for packet 0x%x", retransmitPacket.PacketNumber) packets, err := s.packer.PackRetransmission(retransmitPacket) if err != nil { return false, err From 05434fecbc25eb65793ad59f7f7ad49d2ffdf2ba Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 28 Nov 2018 11:47:38 +0700 Subject: [PATCH 2/3] always use 4 byte packet numbers for long header packets When retransmitting a long header packet, we never have to split CRYPTO frames, since the header is guaranteed to have the same size. --- packet_packer.go | 8 +++++++- packet_packer_test.go | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index 4cf9abfbf..7c6785a73 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -169,8 +169,10 @@ func (p *packetPacker) MaybePackAckPacket() (*packedPacket, error) { func (p *packetPacker) PackRetransmission(packet *ackhandler.Packet) ([]*packedPacket, error) { var controlFrames []wire.Frame var streamFrames []*wire.StreamFrame - // TODO: treat CRYPTO frames separately for _, f := range packet.Frames { + // CRYPTO frames are treated as control frames here. + // Since we're making sure that the header can never be larger for a retransmission, + // we never have to split CRYPTO frames. if sf, ok := f.(*wire.StreamFrame); ok { sf.DataLenPresent = true streamFrames = append(streamFrames, sf) @@ -370,6 +372,10 @@ func (p *packetPacker) getHeader(encLevel protocol.EncryptionLevel) *wire.Extend if encLevel != protocol.Encryption1RTT { header.IsLongHeader = true + // Always send Initial and Handshake packets with the maximum packet number length. + // This simplifies retransmissions: Since the header can't get any larger, + // we don't need to split CRYPTO frames. + header.PacketNumberLen = protocol.PacketNumberLen4 header.SrcConnectionID = p.srcConnID // Set the length to the maximum packet size. // Since it is encoded as a varint, this guarantees us that the header will end up at most as big as GetLength() returns. diff --git a/packet_packer_test.go b/packet_packer_test.go index bbe6f433b..141edcec5 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -115,7 +115,8 @@ var _ = Describe("Packet packer", func() { h := packer.getHeader(protocol.EncryptionHandshake) Expect(h.IsLongHeader).To(BeTrue()) Expect(h.PacketNumber).To(Equal(protocol.PacketNumber(0x42))) - Expect(h.PacketNumberLen).To(Equal(protocol.PacketNumberLen2)) + // long headers always use 4 byte packet numbers, no matter what the packet number generator says + Expect(h.PacketNumberLen).To(Equal(protocol.PacketNumberLen4)) Expect(h.Version).To(Equal(packer.version)) }) From 46487e5267c4c7662157968cf44a64332a39f2ec Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 28 Nov 2018 11:50:13 +0700 Subject: [PATCH 3/3] remove unused constant NonForwardSecurePacketSizeReduction --- internal/protocol/params.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/protocol/params.go b/internal/protocol/params.go index 65b7ab276..1e46772fc 100644 --- a/internal/protocol/params.go +++ b/internal/protocol/params.go @@ -11,10 +11,6 @@ const MaxPacketSizeIPv6 = 1232 // MinStatelessResetSize is the minimum size of a stateless reset packet const MinStatelessResetSize = 1 + 20 + 16 -// NonForwardSecurePacketSizeReduction is the number of bytes a non forward-secure packet has to be smaller than a forward-secure packet -// This makes sure that those packets can always be retransmitted without splitting the contained StreamFrames -const NonForwardSecurePacketSizeReduction = 50 - const defaultMaxCongestionWindowPackets = 1000 // DefaultMaxCongestionWindow is the default for the max congestion window