addressed reviewer comments

client_test.go

@@ -601,6 +601,8 @@ var _ = Describe("Client", func() {
 				Eventually(cl.versionNegotiated.Get).Should(BeTrue())
 			})
 
+			// Illustrates that adversary that injects a version negotiation packet
+			// with no supported versions can break a connection.
 			It("errors if no matching version is found", func() {
 				sess := NewMockQuicSession(mockCtrl)
 				done := make(chan struct{})
@@ -692,24 +694,5 @@ var _ = Describe("Client", func() {
 			cl.handlePacket(composeVersionNegotiationPacket(connID, []protocol.VersionNumber{1234}))
 			Expect(cl.version).To(Equal(ver))
 		})
-
-		// Illustrates that adversary that injects a version negotiation packet
-		// with no supported versions can break a connection.
-		It("connection fails if no supported versions are found in version negotation packet", func() {
-			// Copy of existing test "errors if no matching version is found"
-			sess := NewMockQuicSession(mockCtrl)
-			done := make(chan struct{})
-			sess.EXPECT().destroy(gomock.Any()).Do(func(err error) {
-				defer GinkgoRecover()
-				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(ContainSubstring("No compatible QUIC version found."))
-				close(done)
-			})
-			cl.session = sess
-			cl.config = &Config{Versions: protocol.SupportedVersions}
-			cl.handlePacket(composeVersionNegotiationPacket(connID, []protocol.VersionNumber{1337}))
-			Eventually(done).Should(BeClosed())
-		})
-
 	})
 })

integrationtests/self/mitm_test.go

@@ -287,7 +287,7 @@ var _ = Describe("MITM test", func() {
 				// sendForgedVersionNegotiationPacket sends a fake VN packet with no supported versions
 				// from serverConn to client's remoteAddr
 				// expects hdr from an Initial packet intercepted from client
-				sendForgedVersionNegotationPacket := func(serverConn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) {
+				sendForgedVersionNegotationPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) {
 					defer GinkgoRecover()
 
 					// Create fake version negotiation packet with no supported versions
@@ -295,24 +295,22 @@ var _ = Describe("MITM test", func() {
 					packet, _ := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, versions)
 
 					// Send the packet
-					if _, err := serverConn.WriteTo(packet, remoteAddr); err != nil {
+					_, err := conn.WriteTo(packet, remoteAddr)
-						return
+					Expect(err).ToNot(HaveOccurred())
-					}
 				}
 
 				// sendForgedRetryPacket sends a fake Retry packet with a modified srcConnID
 				// from serverConn to client's remoteAddr
 				// expects hdr from an Initial packet intercepted from client
-				sendForgedRetryPacket := func(serverConn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) {
+				sendForgedRetryPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) {
 					defer GinkgoRecover()
 
 					var x byte = 0x12
 					fakeSrcConnID := protocol.ConnectionID{x, x, x, x, x, x, x, x}
 					retryPacket := testutils.ComposeRetryPacket(fakeSrcConnID, hdr.SrcConnectionID, hdr.DestConnectionID, []byte("token"), hdr.Version)
 
-					if _, err := serverConn.WriteTo(retryPacket, remoteAddr); err != nil {
+					_, err := conn.WriteTo(retryPacket, remoteAddr)
-						return
+					Expect(err).ToNot(HaveOccurred())
-					}
 				}
 
 				// Send a forged Initial packet with no frames to client
@@ -320,10 +318,9 @@ var _ = Describe("MITM test", func() {
 				sendForgedInitialPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) {
 					defer GinkgoRecover()
 
-					initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, testutils.NoFrame)
+					initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, nil)
-					if _, err := conn.WriteTo(initialPacket, remoteAddr); err != nil {
+					_, err := conn.WriteTo(initialPacket, remoteAddr)
-						return
+					Expect(err).ToNot(HaveOccurred())
-					}
 				}
 
 				// Send a forged Initial packet with ACK for random packet to client
@@ -332,10 +329,10 @@ var _ = Describe("MITM test", func() {
 					defer GinkgoRecover()
 
 					// Fake Initial with ACK for packet 2 (unsent)
-					initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, testutils.AckFrame)
+					ackFrame := testutils.ComposeAckFrame(2, 2)
-					if _, err := conn.WriteTo(initialPacket, remoteAddr); err != nil {
+					initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, []wire.Frame{ackFrame})
-						return
+					_, err := conn.WriteTo(initialPacket, remoteAddr)
-					}
+					Expect(err).ToNot(HaveOccurred())
 				}
 
 				// runTestFail succeeds if an error occurs in dialing
@@ -367,7 +364,7 @@ var _ = Describe("MITM test", func() {
 							hdr, _, _, err := wire.ParsePacket(raw, connIDLen)
 							Expect(err).ToNot(HaveOccurred())
 
-							if !(hdr.Type == protocol.PacketTypeInitial) {
+							if hdr.Type != protocol.PacketTypeInitial {
 								return 0
 							}
 
@@ -383,7 +380,6 @@ var _ = Describe("MITM test", func() {
 				// TODO: determine behavior when server does not send Retry packets
 				initialPacketIntercepted := false
 				It("fails when a forged retry packet with modified srcConnID is sent to client", func() {
-					// serverConfig.AcceptToken = func(net.Addr, *quic.Token) bool { return true }
 					delayCb := func(dir quicproxy.Direction, raw []byte) time.Duration {
 						if dir == quicproxy.DirectionIncoming && !initialPacketIntercepted {
 							defer GinkgoRecover()
@@ -407,7 +403,6 @@ var _ = Describe("MITM test", func() {
 				// it has already accepted an initial.
 				// TODO: determine behavior when server does not send Retry packets
 				It("fails when a forged initial packet is sent to client", func() {
-					// serverConfig.AcceptToken = func(net.Addr, *quic.Token) bool { return true }
 					delayCb := func(dir quicproxy.Direction, raw []byte) time.Duration {
 						if dir == quicproxy.DirectionIncoming {
 							defer GinkgoRecover()
@@ -415,7 +410,7 @@ var _ = Describe("MITM test", func() {
 							hdr, _, _, err := wire.ParsePacket(raw, connIDLen)
 							Expect(err).ToNot(HaveOccurred())
 
-							if !(hdr.Type == protocol.PacketTypeInitial) {
+							if hdr.Type != protocol.PacketTypeInitial {
 								return 0
 							}
 
@@ -435,7 +430,7 @@ var _ = Describe("MITM test", func() {
 							hdr, _, _, err := wire.ParsePacket(raw, connIDLen)
 							Expect(err).ToNot(HaveOccurred())
 
-							if !(hdr.Type == protocol.PacketTypeInitial) {
+							if hdr.Type != protocol.PacketTypeInitial {
 								return 0
 							}
 

internal/testutils/testutils.go

@@ -11,29 +11,18 @@ import (
 // Utilities for simulating packet injection and man-in-the-middle (MITM) attacker tests.
 // Do not use for non-testing purposes.
 
-// CryptoFrameType types copied from unexported messageType in crypto_setup.go
+// CryptoFrameType uses same types as messageType in crypto_setup.go
 type CryptoFrameType uint8
 
-const (
+// writePacket returns a new raw packet with the specified header and payload
-	//typeClientHello         CryptoFrameType = 1
+func writePacket(hdr *wire.ExtendedHeader, data []byte) []byte {
-	typeServerHello CryptoFrameType = 2
-	//typeNewSessionTicket    CryptoFrameType = 4
-	//typeEncryptedExtensions CryptoFrameType = 8
-	//typeCertificate         CryptoFrameType = 11
-	//typeCertificateRequest  CryptoFrameType = 13
-	//typeCertificateVerify   CryptoFrameType = 15
-	//typeFinished            CryptoFrameType = 20
-)
-
-// getRawPacket returns a new raw packet with the specified header and payload
-func getRawPacket(hdr *wire.ExtendedHeader, data []byte) []byte {
 	buf := &bytes.Buffer{}
 	hdr.Write(buf, protocol.VersionTLS)
 	return append(buf.Bytes(), data...)
 }
 
 // packRawPayload returns a new raw payload containing given frames
-func packRawPayload(version protocol.VersionNumber, frames ...wire.Frame) []byte {
+func packRawPayload(version protocol.VersionNumber, frames []wire.Frame) []byte {
 	buf := new(bytes.Buffer)
 	for _, cf := range frames {
 		cf.Write(buf, version)
@@ -61,7 +50,7 @@ func ComposeConnCloseFrame() *wire.ConnectionCloseFrame {
 	}
 }
 
-// ComposeAckFrame returns a new Ack Frame that ACKs packet 0
+// ComposeAckFrame returns a new Ack Frame that acknowledges all packets between smallest and largest
 func ComposeAckFrame(smallest protocol.PacketNumber, largest protocol.PacketNumber) *wire.AckFrame {
 	ackRange := wire.AckRange{
 		Smallest: smallest,
@@ -73,36 +62,17 @@ func ComposeAckFrame(smallest protocol.PacketNumber, largest protocol.PacketNumb
 	}
 }
 
-// InitialContents enumerates possible frames to include in forged Initial packets
-type InitialContents int
-
-const (
-	ServerHelloFrame InitialContents = iota + 1
-	AckFrame
-	ConnectionCloseFrame
-	NoFrame
-)
-
 // ComposeInitialPacket returns an Initial packet encrypted under key
-// (the original destination connection ID)
+// (the original destination connection ID) containing specified frames
-// contains frame of specified type
+func ComposeInitialPacket(srcConnID protocol.ConnectionID, destConnID protocol.ConnectionID, version protocol.VersionNumber, key protocol.ConnectionID, frames []wire.Frame) []byte {
-func ComposeInitialPacket(srcConnID protocol.ConnectionID, destConnID protocol.ConnectionID, version protocol.VersionNumber, key protocol.ConnectionID, frameType InitialContents) []byte {
 	sealer, _, _ := handshake.NewInitialAEAD(key, protocol.PerspectiveServer)
 
 	// compose payload
 	var payload []byte
-	switch frameType {
+	if len(frames) == 0 {
-	case ServerHelloFrame:
-		cf := ComposeCryptoFrame(typeServerHello, 20)
-		payload = packRawPayload(version, cf)
-	case AckFrame:
-		ack := ComposeAckFrame(2, 2) // ack packet 2
-		payload = packRawPayload(version, ack)
-	case ConnectionCloseFrame:
-		ccf := ComposeConnCloseFrame()
-		payload = packRawPayload(version, ccf)
-	case NoFrame:
 		payload = make([]byte, protocol.MinInitialPacketSize)
+	} else {
+		payload = packRawPayload(version, frames)
 	}
 
 	// compose Initial header
@@ -122,7 +92,7 @@ func ComposeInitialPacket(srcConnID protocol.ConnectionID, destConnID protocol.C
 		PacketNumber:    0x0,
 	}
 
-	raw := getRawPacket(hdr, payload)
+	raw := writePacket(hdr, payload)
 
 	// encrypt payload and header
 	payloadOffset := len(raw) - payloadSize
@@ -152,5 +122,5 @@ func ComposeRetryPacket(srcConnID protocol.ConnectionID, destConnID protocol.Con
 			Version:              version,
 		},
 	}
-	return getRawPacket(hdr, nil)
+	return writePacket(hdr, nil)
 }

session_test.go

@@ -1748,11 +1748,12 @@ var _ = Describe("Client Session", func() {
 			Expect(sess.handlePacketImpl(getPacket(hdr2, nil))).To(BeFalse())
 		})
 
-		// Illustrates that an injected Initial with an ACK frame for an unsent causes
+		// Illustrates that an injected Initial with an ACK frame for an unsent packet causes
 		// the connection to immediately break down
 		It("fails on Initial-level ACK for unsent packet", func() {
 			sessionRunner.EXPECT().Retire(gomock.Any())
-			initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, testutils.AckFrame)
+			ackFrame := testutils.ComposeAckFrame(0, 0)
+			initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, []wire.Frame{ackFrame})
 			Expect(sess.handlePacketImpl(wrapPacket(initialPacket))).To(BeFalse())
 		})
 
@@ -1760,7 +1761,8 @@ var _ = Describe("Client Session", func() {
 		// the connection to immediately break down
 		It("fails on Initial-level CONNECTION_CLOSE frame", func() {
 			sessionRunner.EXPECT().Remove(gomock.Any())
-			initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, testutils.ConnectionCloseFrame)
+			connCloseFrame := testutils.ComposeConnCloseFrame()
+			initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, []wire.Frame{connCloseFrame})
 			Expect(sess.handlePacketImpl(wrapPacket(initialPacket))).To(BeTrue())
 		})
 
@@ -1773,7 +1775,7 @@ var _ = Describe("Client Session", func() {
 			packer.EXPECT().ChangeDestConnectionID(newSrcConnID)
 
 			sess.handlePacketImpl(wrapPacket(testutils.ComposeRetryPacket(newSrcConnID, sess.destConnID, sess.destConnID, []byte("foobar"), sess.version)))
-			initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, testutils.NoFrame)
+			initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, nil)
 			Expect(sess.handlePacketImpl(wrapPacket(initialPacket))).To(BeFalse())
 		})