TUN-6592: Decrement TTL and return ICMP time exceed if it's 0

Author: chungthuang

connection/quic.go

@@ -51,7 +51,7 @@ type QUICConnection struct {
 	sessionManager datagramsession.Manager
 	// datagramMuxer mux/demux datagrams from quic connection
 	datagramMuxer        quicpogs.BaseDatagramMuxer
-	packetRouter         *packetRouter
+	packetRouter         *packet.Router
 	controlStreamHandler ControlStreamHandler
 	connOptions          *tunnelpogs.ConnectionOptions
 }
@@ -65,7 +65,7 @@ func NewQUICConnection(
 	connOptions *tunnelpogs.ConnectionOptions,
 	controlStreamHandler ControlStreamHandler,
 	logger *zerolog.Logger,
-	icmpProxy ingress.ICMPProxy,
+	icmpRouter packet.ICMPRouter,
 ) (*QUICConnection, error) {
 	session, err := quic.DialAddr(edgeAddr.String(), tlsConfig, quicConfig)
 	if err != nil {
@@ -75,15 +75,12 @@ func NewQUICConnection(
 	sessionDemuxChan := make(chan *packet.Session, demuxChanCapacity)
 	var (
 		datagramMuxer quicpogs.BaseDatagramMuxer
-		pr            *packetRouter
+		pr            *packet.Router
 	)
-	if icmpProxy != nil {
-		pr = &packetRouter{
-			muxer:     quicpogs.NewDatagramMuxerV2(session, logger, sessionDemuxChan),
-			icmpProxy: icmpProxy,
-			logger:    logger,
-		}
-		datagramMuxer = pr.muxer
+	if icmpRouter != nil {
+		datagramMuxerV2 := quicpogs.NewDatagramMuxerV2(session, logger, sessionDemuxChan)
+		pr = packet.NewRouter(datagramMuxerV2, &returnPipe{muxer: datagramMuxerV2}, icmpRouter, logger)
+		datagramMuxer = datagramMuxerV2
 	} else {
 		datagramMuxer = quicpogs.NewDatagramMuxer(session, logger, sessionDemuxChan)
 	}
@@ -139,7 +136,7 @@ func (q *QUICConnection) Serve(ctx context.Context) error {
 	if q.packetRouter != nil {
 		errGroup.Go(func() error {
 			defer cancel()
-			return q.packetRouter.serve(ctx)
+			return q.packetRouter.Serve(ctx)
 		})
 	}
 
@@ -348,50 +345,6 @@ func (q *QUICConnection) UpdateConfiguration(ctx context.Context, version int32,
 	return q.orchestrator.UpdateConfig(version, config)
 }
 
-type packetRouter struct {
-	muxer     *quicpogs.DatagramMuxerV2
-	icmpProxy ingress.ICMPProxy
-	logger    *zerolog.Logger
-}
-
-func (pr *packetRouter) serve(ctx context.Context) error {
-	icmpDecoder := packet.NewICMPDecoder()
-	for {
-		pk, err := pr.muxer.ReceivePacket(ctx)
-		if err != nil {
-			return err
-		}
-		icmpPacket, err := icmpDecoder.Decode(pk)
-		if err != nil {
-			pr.logger.Err(err).Msg("Failed to decode ICMP packet from quic datagram")
-			continue
-		}
-
-		flowPipe := muxerResponder{muxer: pr.muxer}
-		if err := pr.icmpProxy.Request(icmpPacket, &flowPipe); err != nil {
-			pr.logger.Err(err).
-				Str("src", icmpPacket.Src.String()).
-				Str("dst", icmpPacket.Dst.String()).
-				Interface("type", icmpPacket.Type).
-				Msg("Failed to send ICMP packet")
-			continue
-		}
-	}
-}
-
-// muxerResponder wraps DatagramMuxerV2 to satisfy the packet.FunnelUniPipe interface
-type muxerResponder struct {
-	muxer *quicpogs.DatagramMuxerV2
-}
-
-func (mr *muxerResponder) SendPacket(dst netip.Addr, pk packet.RawPacket) error {
-	return mr.muxer.SendPacket(pk)
-}
-
-func (mr *muxerResponder) Close() error {
-	return nil
-}
-
 // streamReadWriteAcker is a light wrapper over QUIC streams with a callback to send response back to
 // the client.
 type streamReadWriteAcker struct {
@@ -538,3 +491,16 @@ func (np *nopCloserReadWriter) Close() error {
 
 	return nil
 }
+
+// returnPipe wraps DatagramMuxerV2 to satisfy the packet.FunnelUniPipe interface
+type returnPipe struct {
+	muxer *quicpogs.DatagramMuxerV2
+}
+
+func (rp *returnPipe) SendPacket(dst netip.Addr, pk packet.RawPacket) error {
+	return rp.muxer.SendPacket(pk)
+}
+
+func (rp *returnPipe) Close() error {
+	return nil
+}

ingress/icmp_darwin.go

@@ -115,7 +115,7 @@ func (snf echoFunnelID) String() string {
 	return strconv.FormatUint(uint64(snf), 10)
 }
 
-func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (ICMPProxy, error) {
+func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (*icmpProxy, error) {
 	conn, err := newICMPConn(listenIP)
 	if err != nil {
 		return nil, err

ingress/icmp_generic.go

@@ -3,14 +3,29 @@
 package ingress
 
 import (
+	"context"
 	"fmt"
 	"net/netip"
 	"runtime"
 	"time"
 
 	"github.com/rs/zerolog"
+
+	"github.com/cloudflare/cloudflared/packet"
 )
 
-func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (ICMPProxy, error) {
-	return nil, fmt.Errorf("ICMP proxy is not implemented on %s", runtime.GOOS)
+var errICMPProxyNotImplemented = fmt.Errorf("ICMP proxy is not implemented on %s", runtime.GOOS)
+
+type icmpProxy struct{}
+
+func (ip icmpProxy) Request(pk *packet.ICMP, responder packet.FunnelUniPipe) error {
+	return errICMPProxyNotImplemented
+}
+
+func (ip *icmpProxy) Serve(ctx context.Context) error {
+	return errICMPProxyNotImplemented
+}
+
+func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (*icmpProxy, error) {
+	return nil, errICMPProxyNotImplemented
 }

ingress/icmp_linux.go

@@ -28,7 +28,7 @@ type icmpProxy struct {
 	idleTimeout      time.Duration
 }
 
-func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (ICMPProxy, error) {
+func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (*icmpProxy, error) {
 	if err := testPermission(listenIP); err != nil {
 		return nil, err
 	}

ingress/icmp_posix.go

@@ -96,6 +96,7 @@ func (ief *icmpEchoFlow) returnToSrc(reply *echoReply) error {
 			Src:      reply.from,
 			Dst:      ief.Src,
 			Protocol: layers.IPProtocol(reply.msg.Type.Protocol()),
+			TTL:      packet.DefaultTTL,
 		},
 		Message: reply.msg,
 	}

ingress/icmp_windows.go

@@ -224,7 +224,7 @@ type icmpProxy struct {
 	encoderPool sync.Pool
 }
 
-func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (ICMPProxy, error) {
+func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (*icmpProxy, error) {
 	var (
 		srcSocketAddr *sockAddrIn6
 		handle        uintptr
@@ -302,6 +302,7 @@ func (ip *icmpProxy) handleEchoReply(request *packet.ICMP, echoReq *icmp.Echo, d
 			Src:      request.Dst,
 			Dst:      request.Src,
 			Protocol: layers.IPProtocol(request.Type.Protocol()),
+			TTL:      packet.DefaultTTL,
 		},
 		Message: &icmp.Message{
 			Type: replyType,

ingress/icmp_windows_test.go

@@ -134,7 +134,6 @@ func TestSendEchoErrors(t *testing.T) {
 func testSendEchoErrors(t *testing.T, listenIP netip.Addr) {
 	proxy, err := newICMPProxy(listenIP, &noopLogger, time.Second)
 	require.NoError(t, err)
-	winProxy := proxy.(*icmpProxy)
 
 	echo := icmp.Echo{
 		ID:   6193,
@@ -145,7 +144,7 @@ func testSendEchoErrors(t *testing.T, listenIP netip.Addr) {
 	if listenIP.Is6() {
 		documentIP = netip.MustParseAddr("2001:db8::1")
 	}
-	resp, err := winProxy.icmpEchoRoundtrip(documentIP, &echo)
+	resp, err := proxy.icmpEchoRoundtrip(documentIP, &echo)
 	require.Error(t, err)
 	require.Nil(t, resp)
 }

ingress/origin_icmp_proxy.go

@@ -26,34 +26,26 @@ var (
 	errPacketNil = fmt.Errorf("packet is nil")
 )
 
-// ICMPProxy sends ICMP messages and listens for their responses
-type ICMPProxy interface {
-	// Serve starts listening for responses to the requests until context is done
-	Serve(ctx context.Context) error
-	// Request sends an ICMP message
-	Request(pk *packet.ICMP, responder packet.FunnelUniPipe) error
-}
-
 type icmpRouter struct {
-	ipv4Proxy ICMPProxy
-	ipv6Proxy ICMPProxy
+	ipv4Proxy *icmpProxy
+	ipv6Proxy *icmpProxy
 }
 
-// NewICMPProxy doesn't return an error if either ipv4 proxy or ipv6 proxy can be created. The machine might only
+// NewICMPRouter doesn't return an error if either ipv4 proxy or ipv6 proxy can be created. The machine might only
 // support one of them
-func NewICMPProxy(logger *zerolog.Logger) (ICMPProxy, error) {
+func NewICMPRouter(logger *zerolog.Logger) (*icmpRouter, error) {
 	// TODO: TUN-6741: don't bind to all interface
 	ipv4Proxy, ipv4Err := newICMPProxy(netip.IPv4Unspecified(), logger, funnelIdleTimeout)
 	ipv6Proxy, ipv6Err := newICMPProxy(netip.IPv6Unspecified(), logger, funnelIdleTimeout)
 	if ipv4Err != nil && ipv6Err != nil {
 		return nil, fmt.Errorf("cannot create ICMPv4 proxy: %v nor ICMPv6 proxy: %v", ipv4Err, ipv6Err)
 	}
 	if ipv4Err != nil {
-		logger.Warn().Err(ipv4Err).Msg("failed to create ICMPv4 proxy, only ICMPv6 proxy is created")
+		logger.Debug().Err(ipv4Err).Msg("failed to create ICMPv4 proxy, only ICMPv6 proxy is created")
 		ipv4Proxy = nil
 	}
 	if ipv6Err != nil {
-		logger.Warn().Err(ipv6Err).Msg("failed to create ICMPv6 proxy, only ICMPv4 proxy is created")
+		logger.Debug().Err(ipv6Err).Msg("failed to create ICMPv6 proxy, only ICMPv4 proxy is created")
 		ipv6Proxy = nil
 	}
 	return &icmpRouter{

ingress/origin_icmp_proxy_test.go

@@ -30,24 +30,24 @@ var (
 // Note: if this test fails on your device under Linux, then most likely you need to make sure that your user
 // is allowed in ping_group_range. See the following gist for how to do that:
 // https://github.com/ValentinBELYN/icmplib/blob/main/docs/6-use-icmplib-without-privileges.md
-func TestICMPProxyEcho(t *testing.T) {
-	testICMPProxyEcho(t, true)
-	testICMPProxyEcho(t, false)
+func TestICMPRouterEcho(t *testing.T) {
+	testICMPRouterEcho(t, true)
+	testICMPRouterEcho(t, false)
 }
 
-func testICMPProxyEcho(t *testing.T, sendIPv4 bool) {
+func testICMPRouterEcho(t *testing.T, sendIPv4 bool) {
 	const (
 		echoID = 36571
 		endSeq = 20
 	)
 
-	proxy, err := NewICMPProxy(&noopLogger)
+	router, err := NewICMPRouter(&noopLogger)
 	require.NoError(t, err)
 
 	proxyDone := make(chan struct{})
 	ctx, cancel := context.WithCancel(context.Background())
 	go func() {
-		proxy.Serve(ctx)
+		router.Serve(ctx)
 		close(proxyDone)
 	}()
 
@@ -67,6 +67,7 @@ func testICMPProxyEcho(t *testing.T, sendIPv4 bool) {
 			Src:      localIP,
 			Dst:      localIP,
 			Protocol: protocol,
+			TTL:      packet.DefaultTTL,
 		}
 	}
 
@@ -88,7 +89,7 @@ func testICMPProxyEcho(t *testing.T, sendIPv4 bool) {
 					},
 				},
 			}
-			require.NoError(t, proxy.Request(&pk, &responder))
+			require.NoError(t, router.Request(&pk, &responder))
 			responder.validate(t, &pk)
 		}
 	}
@@ -97,7 +98,7 @@ func testICMPProxyEcho(t *testing.T, sendIPv4 bool) {
 }
 
 // TestICMPProxyRejectNotEcho makes sure it rejects messages other than echo
-func TestICMPProxyRejectNotEcho(t *testing.T) {
+func TestICMPRouterRejectNotEcho(t *testing.T) {
 	msgs := []icmp.Message{
 		{
 			Type: ipv4.ICMPTypeDestinationUnreachable,
@@ -122,7 +123,7 @@ func TestICMPProxyRejectNotEcho(t *testing.T) {
 			},
 		},
 	}
-	testICMPProxyRejectNotEcho(t, localhostIP, msgs)
+	testICMPRouterRejectNotEcho(t, localhostIP, msgs)
 	msgsV6 := []icmp.Message{
 		{
 			Type: ipv6.ICMPTypeDestinationUnreachable,
@@ -147,11 +148,11 @@ func TestICMPProxyRejectNotEcho(t *testing.T) {
 			},
 		},
 	}
-	testICMPProxyRejectNotEcho(t, localhostIPv6, msgsV6)
+	testICMPRouterRejectNotEcho(t, localhostIPv6, msgsV6)
 }
 
-func testICMPProxyRejectNotEcho(t *testing.T, srcDstIP netip.Addr, msgs []icmp.Message) {
-	proxy, err := NewICMPProxy(&noopLogger)
+func testICMPRouterRejectNotEcho(t *testing.T, srcDstIP netip.Addr, msgs []icmp.Message) {
+	router, err := NewICMPRouter(&noopLogger)
 	require.NoError(t, err)
 
 	responder := echoFlowResponder{
@@ -168,10 +169,11 @@ func testICMPProxyRejectNotEcho(t *testing.T, srcDstIP netip.Addr, msgs []icmp.M
 				Src:      srcDstIP,
 				Dst:      srcDstIP,
 				Protocol: protocol,
+				TTL:      packet.DefaultTTL,
 			},
 			Message: &m,
 		}
-		require.Error(t, proxy.Request(&pk, &responder))
+		require.Error(t, router.Request(&pk, &responder))
 	}
 }
 

packet/decoder.go

@@ -16,8 +16,8 @@ func FindProtocol(p []byte) (layers.IPProtocol, error) {
 	}
 	switch version {
 	case 4:
-		if len(p) < ipv4HeaderLen {
-			return 0, fmt.Errorf("IPv4 packet should have at least %d bytes, got %d bytes", ipv4HeaderLen, len(p))
+		if len(p) < ipv4MinHeaderLen {
+			return 0, fmt.Errorf("IPv4 packet should have at least %d bytes, got %d bytes", ipv4MinHeaderLen, len(p))
 		}
 		// Protocol is in the 10th byte of IPv4 header
 		return layers.IPProtocol(p[9]), nil