diff --git a/ingress/icmp_darwin.go b/ingress/icmp_darwin.go index 8e69202c..ace04f23 100644 --- a/ingress/icmp_darwin.go +++ b/ingress/icmp_darwin.go @@ -23,7 +23,6 @@ import ( "github.com/cloudflare/cloudflared/packet" ) -// TODO: TUN-6654 Extend support to IPv6 type icmpProxy struct { srcFunnelTracker *packet.FunnelTracker echoIDTracker *echoIDTracker @@ -180,25 +179,56 @@ func (ip *icmpProxy) Serve(ctx context.Context) error { ip.srcFunnelTracker.ScheduleCleanup(ctx, ip.idleTimeout) }() buf := make([]byte, mtu) + icmpDecoder := packet.NewICMPDecoder() for { - n, src, err := ip.conn.ReadFrom(buf) + n, from, err := ip.conn.ReadFrom(buf) if err != nil { return err } - if err := ip.handleResponse(src, buf[:n]); err != nil { - ip.logger.Err(err).Str("src", src.String()).Msg("Failed to handle ICMP response") + reply, err := parseReply(from, buf[:n]) + if err != nil { + ip.logger.Debug().Err(err).Str("dst", from.String()).Msg("Failed to parse ICMP reply, continue to parse as full packet") + // In unit test, we found out when the listener listens on 0.0.0.0, the socket reads the full packet after + // the second reply + if err := ip.handleFullPacket(icmpDecoder, buf[:n]); err != nil { + ip.logger.Err(err).Str("dst", from.String()).Msg("Failed to parse ICMP reply as full packet") + } + continue + } + if !isEchoReply(reply.msg) { + ip.logger.Debug().Str("dst", from.String()).Msgf("Drop ICMP %s from reply", reply.msg.Type) + continue + } + if ip.sendReply(reply); err != nil { + ip.logger.Error().Err(err).Str("dst", from.String()).Msg("Failed to send ICMP reply") continue } } } -func (ip *icmpProxy) handleResponse(from net.Addr, rawMsg []byte) error { - reply, err := parseReply(from, rawMsg) +func (ip *icmpProxy) handleFullPacket(decoder *packet.ICMPDecoder, rawPacket []byte) error { + icmpPacket, err := decoder.Decode(packet.RawPacket{Data: rawPacket}) if err != nil { return err } - funnel, exists := ip.srcFunnelTracker.Get(echoFunnelID(reply.echo.ID)) - if !exists { + echo, err := getICMPEcho(icmpPacket.Message) + if err != nil { + return err + } + reply := echoReply{ + from: icmpPacket.Src, + msg: icmpPacket.Message, + echo: echo, + } + if ip.sendReply(&reply); err != nil { + return err + } + return nil +} + +func (ip *icmpProxy) sendReply(reply *echoReply) error { + funnel, ok := ip.srcFunnelTracker.Get(echoFunnelID(reply.echo.ID)) + if !ok { return packet.ErrFunnelNotFound } icmpFlow, err := toICMPEchoFlow(funnel) diff --git a/ingress/icmp_linux.go b/ingress/icmp_linux.go index f0917e2e..21da6ce1 100644 --- a/ingress/icmp_linux.go +++ b/ingress/icmp_linux.go @@ -110,26 +110,26 @@ func (ip *icmpProxy) Serve(ctx context.Context) error { func (ip *icmpProxy) listenResponse(flow *icmpEchoFlow, conn *icmp.PacketConn) error { buf := make([]byte, mtu) for { - n, src, err := conn.ReadFrom(buf) + n, from, err := conn.ReadFrom(buf) if err != nil { return err } - - if err := ip.handleResponse(flow, src, buf[:n]); err != nil { - ip.logger.Err(err).Str("dst", src.String()).Msg("Failed to handle ICMP response") + reply, err := parseReply(from, buf[:n]) + if err != nil { + ip.logger.Error().Err(err).Str("dst", from.String()).Msg("Failed to parse ICMP reply") + continue + } + if !isEchoReply(reply.msg) { + ip.logger.Debug().Str("dst", from.String()).Msgf("Drop ICMP %s from reply", reply.msg.Type) + continue + } + if err := flow.returnToSrc(reply); err != nil { + ip.logger.Err(err).Str("dst", from.String()).Msg("Failed to send ICMP reply") continue } } } -func (ip *icmpProxy) handleResponse(flow *icmpEchoFlow, from net.Addr, rawMsg []byte) error { - reply, err := parseReply(from, rawMsg) - if err != nil { - return err - } - return flow.returnToSrc(reply) -} - // originSender wraps icmp.PacketConn to implement packet.FunnelUniPipe interface type originSender struct { conn *icmp.PacketConn diff --git a/ingress/icmp_posix.go b/ingress/icmp_posix.go index 442017bb..927c02f9 100644 --- a/ingress/icmp_posix.go +++ b/ingress/icmp_posix.go @@ -113,8 +113,15 @@ type echoReply struct { } func parseReply(from net.Addr, rawMsg []byte) (*echoReply, error) { - // TODO: TUN-6654 Check for IPv6 - msg, err := icmp.ParseMessage(int(layers.IPProtocolICMPv4), rawMsg) + fromAddr, ok := netipAddr(from) + if !ok { + return nil, fmt.Errorf("cannot convert %s to netip.Addr", from) + } + proto := layers.IPProtocolICMPv4 + if fromAddr.Is6() { + proto = layers.IPProtocolICMPv6 + } + msg, err := icmp.ParseMessage(int(proto), rawMsg) if err != nil { return nil, err } @@ -122,10 +129,6 @@ func parseReply(from net.Addr, rawMsg []byte) (*echoReply, error) { if err != nil { return nil, err } - fromAddr, ok := netipAddr(from) - if !ok { - return nil, fmt.Errorf("cannot convert %s to netip.Addr", from) - } return &echoReply{ from: fromAddr, msg: msg, diff --git a/ingress/icmp_windows.go b/ingress/icmp_windows.go index ac34819c..f5b5ac48 100644 --- a/ingress/icmp_windows.go +++ b/ingress/icmp_windows.go @@ -145,6 +145,9 @@ type icmpProxy struct { } func newICMPProxy(listenIP netip.Addr, logger *zerolog.Logger, idleTimeout time.Duration) (ICMPProxy, error) { + if listenIP.Is6() { + return nil, fmt.Errorf("ICMPv6 not implemented for Windows") + } handle, _, err := IcmpCreateFile_proc.Call() // Windows procedure calls always return non-nil error constructed from the result of GetLastError. // Caller need to inspect the primary returned value diff --git a/ingress/origin_icmp_proxy.go b/ingress/origin_icmp_proxy.go index 50245c7b..c114eb75 100644 --- a/ingress/origin_icmp_proxy.go +++ b/ingress/origin_icmp_proxy.go @@ -8,6 +8,8 @@ import ( "github.com/rs/zerolog" "golang.org/x/net/icmp" + "golang.org/x/net/ipv4" + "golang.org/x/net/ipv6" "github.com/cloudflare/cloudflared/packet" ) @@ -32,8 +34,65 @@ type ICMPProxy interface { Request(pk *packet.ICMP, responder packet.FunnelUniPipe) error } -func NewICMPProxy(listenIP netip.Addr, logger *zerolog.Logger) (ICMPProxy, error) { - return newICMPProxy(listenIP, logger, funnelIdleTimeout) +type icmpRouter struct { + ipv4Proxy ICMPProxy + ipv6Proxy ICMPProxy +} + +// NewICMPProxy 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) { + // 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") + ipv4Proxy = nil + } + if ipv6Err != nil { + logger.Warn().Err(ipv6Err).Msg("failed to create ICMPv6 proxy, only ICMPv4 proxy is created") + ipv6Proxy = nil + } + return &icmpRouter{ + ipv4Proxy: ipv4Proxy, + ipv6Proxy: ipv6Proxy, + }, nil +} + +func (ir *icmpRouter) Serve(ctx context.Context) error { + if ir.ipv4Proxy != nil && ir.ipv6Proxy != nil { + errC := make(chan error, 2) + go func() { + errC <- ir.ipv4Proxy.Serve(ctx) + }() + go func() { + errC <- ir.ipv6Proxy.Serve(ctx) + }() + return <-errC + } + if ir.ipv4Proxy != nil { + return ir.ipv4Proxy.Serve(ctx) + } + if ir.ipv6Proxy != nil { + return ir.ipv6Proxy.Serve(ctx) + } + return fmt.Errorf("ICMPv4 proxy and ICMPv6 proxy are both nil") +} + +func (ir *icmpRouter) Request(pk *packet.ICMP, responder packet.FunnelUniPipe) error { + if pk.Dst.Is4() { + if ir.ipv4Proxy != nil { + return ir.ipv4Proxy.Request(pk, responder) + } + return fmt.Errorf("ICMPv4 proxy was not instantiated") + } + if ir.ipv6Proxy != nil { + return ir.ipv6Proxy.Request(pk, responder) + } + return fmt.Errorf("ICMPv6 proxy was not instantiated") } func getICMPEcho(msg *icmp.Message) (*icmp.Echo, error) { @@ -43,3 +102,7 @@ func getICMPEcho(msg *icmp.Message) (*icmp.Echo, error) { } return echo, nil } + +func isEchoReply(msg *icmp.Message) bool { + return msg.Type == ipv4.ICMPTypeEchoReply || msg.Type == ipv6.ICMPTypeEchoReply +} diff --git a/ingress/origin_icmp_proxy_test.go b/ingress/origin_icmp_proxy_test.go index fc6010c1..d3573894 100644 --- a/ingress/origin_icmp_proxy_test.go +++ b/ingress/origin_icmp_proxy_test.go @@ -5,6 +5,8 @@ import ( "fmt" "net" "net/netip" + "runtime" + "strings" "testing" "github.com/google/gopacket/layers" @@ -12,13 +14,15 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/net/icmp" "golang.org/x/net/ipv4" + "golang.org/x/net/ipv6" "github.com/cloudflare/cloudflared/packet" ) var ( - noopLogger = zerolog.Nop() - localhostIP = netip.MustParseAddr("127.0.0.1") + noopLogger = zerolog.Nop() + localhostIP = netip.MustParseAddr("127.0.0.1") + localhostIPv6 = netip.MustParseAddr("::1") ) // TestICMPProxyEcho makes sure we can send ICMP echo via the Request method and receives response via the @@ -28,12 +32,20 @@ var ( // 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) + if runtime.GOOS == "windows" { + t.Skip("TODO: TUN-6743: test ICMPv6 on Windows") + } + testICMPProxyEcho(t, false) +} + +func testICMPProxyEcho(t *testing.T, sendIPv4 bool) { const ( echoID = 36571 - endSeq = 100 + endSeq = 20 ) - proxy, err := NewICMPProxy(localhostIP, &noopLogger) + proxy, err := NewICMPProxy(&noopLogger) require.NoError(t, err) proxyDone := make(chan struct{}) @@ -48,37 +60,30 @@ func TestICMPProxyEcho(t *testing.T) { respChan: make(chan []byte, 1), } - ips := []packet.IP{ - { - Src: localhostIP, - Dst: localhostIP, - Protocol: layers.IPProtocolICMPv4, - }, + protocol := layers.IPProtocolICMPv6 + if sendIPv4 { + protocol = layers.IPProtocolICMPv4 } - - addrs, err := net.InterfaceAddrs() - require.NoError(t, err) - for _, addr := range addrs { - if ipnet, ok := addr.(*net.IPNet); ok { - ip := ipnet.IP - if !ipnet.IP.IsLoopback() && ip.IsPrivate() && ip.To4() != nil { - localIP := netip.MustParseAddr(ipnet.IP.String()) - ips = append(ips, packet.IP{ - Src: localIP, - Dst: localIP, - Protocol: layers.IPProtocolICMPv4, - }) - break - } + localIPs := getLocalIPs(t, sendIPv4) + ips := make([]*packet.IP, len(localIPs)) + for i, localIP := range localIPs { + ips[i] = &packet.IP{ + Src: localIP, + Dst: localIP, + Protocol: protocol, } } + var icmpType icmp.Type = ipv6.ICMPTypeEchoRequest + if sendIPv4 { + icmpType = ipv4.ICMPTypeEcho + } for seq := 0; seq < endSeq; seq++ { for i, ip := range ips { pk := packet.ICMP{ - IP: &ip, + IP: ip, Message: &icmp.Message{ - Type: ipv4.ICMPTypeEcho, + Type: icmpType, Code: 0, Body: &icmp.Echo{ ID: echoID + i, @@ -121,19 +126,52 @@ func TestICMPProxyRejectNotEcho(t *testing.T) { }, }, } - proxy, err := NewICMPProxy(localhostIP, &noopLogger) + testICMPProxyRejectNotEcho(t, localhostIP, msgs) + msgsV6 := []icmp.Message{ + { + Type: ipv6.ICMPTypeDestinationUnreachable, + Code: 3, + Body: &icmp.DstUnreach{ + Data: []byte("original packet"), + }, + }, + { + Type: ipv6.ICMPTypeTimeExceeded, + Code: 0, + Body: &icmp.TimeExceeded{ + Data: []byte("original packet"), + }, + }, + { + Type: ipv6.ICMPTypePacketTooBig, + Code: 0, + Body: &icmp.PacketTooBig{ + MTU: 1280, + Data: []byte("original packet"), + }, + }, + } + testICMPProxyRejectNotEcho(t, localhostIPv6, msgsV6) +} + +func testICMPProxyRejectNotEcho(t *testing.T, srcDstIP netip.Addr, msgs []icmp.Message) { + proxy, err := NewICMPProxy(&noopLogger) require.NoError(t, err) responder := echoFlowResponder{ decoder: packet.NewICMPDecoder(), respChan: make(chan []byte), } + protocol := layers.IPProtocolICMPv4 + if srcDstIP.Is6() { + protocol = layers.IPProtocolICMPv6 + } for _, m := range msgs { pk := packet.ICMP{ IP: &packet.IP{ - Src: localhostIP, - Dst: localhostIP, - Protocol: layers.IPProtocolICMPv4, + Src: srcDstIP, + Dst: srcDstIP, + Protocol: protocol, }, Message: &m, } @@ -166,8 +204,40 @@ func (efr *echoFlowResponder) validate(t *testing.T, echoReq *packet.ICMP) { require.Equal(t, decoded.Dst, echoReq.Src) require.Equal(t, echoReq.Protocol, decoded.Protocol) - require.Equal(t, ipv4.ICMPTypeEchoReply, decoded.Type) + if echoReq.Type == ipv4.ICMPTypeEcho { + require.Equal(t, ipv4.ICMPTypeEchoReply, decoded.Type) + } else { + require.Equal(t, ipv6.ICMPTypeEchoReply, decoded.Type) + } require.Equal(t, 0, decoded.Code) - require.NotZero(t, decoded.Checksum) + if echoReq.Type == ipv4.ICMPTypeEcho { + require.NotZero(t, decoded.Checksum) + } else { + // For ICMPv6, the kernel will compute the checksum during transmission unless pseudo header is not nil + require.Zero(t, decoded.Checksum) + } + require.Equal(t, echoReq.Body, decoded.Body) } + +func getLocalIPs(t *testing.T, ipv4 bool) []netip.Addr { + interfaces, err := net.Interfaces() + require.NoError(t, err) + localIPs := []netip.Addr{} + for _, i := range interfaces { + // Skip TUN devices + if strings.Contains(i.Name, "tun") { + continue + } + addrs, err := i.Addrs() + require.NoError(t, err) + for _, addr := range addrs { + if ipnet, ok := addr.(*net.IPNet); ok && (ipnet.IP.IsPrivate() || ipnet.IP.IsLoopback()) { + if (ipv4 && ipnet.IP.To4() != nil) || (!ipv4 && ipnet.IP.To4() == nil) { + localIPs = append(localIPs, netip.MustParseAddr(ipnet.IP.String())) + } + } + } + } + return localIPs +} diff --git a/supervisor/supervisor.go b/supervisor/supervisor.go index 52ce46a4..fc80ed1c 100644 --- a/supervisor/supervisor.go +++ b/supervisor/supervisor.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "net/netip" "strings" "time" @@ -117,12 +116,8 @@ func NewSupervisor(config *TunnelConfig, orchestrator *orchestration.Orchestrato connAwareLogger: log, } if useDatagramV2(config) { - // TODO: TUN-6654 listen for IPv6 and decide if it should listen on specific IP - listenIP, err := netip.ParseAddr("0.0.0.0") - if err != nil { - return nil, err - } - icmpProxy, err := ingress.NewICMPProxy(listenIP, config.Log) + // TODO: TUN-6701: Decouple upgrade of datagram v2 and using icmp proxy + icmpProxy, err := ingress.NewICMPProxy(config.Log) if err != nil { log.Logger().Warn().Err(err).Msg("Failed to create icmp proxy, will continue to use datagram v1") } else {