diff --git a/internal/ateomnet/net.go b/internal/ateomnet/net.go index 91203a8e04..ee0902fb88 100644 --- a/internal/ateomnet/net.go +++ b/internal/ateomnet/net.go @@ -238,10 +238,8 @@ func InstallActorNftablesRules(egressPort uint16) error { // listener. REDIRECT preserves SO_ORIGINAL_DST for the CONNECT authority. // * postrouting: masquerade traffic not handled by the TCP tunnel, notably // DNS over UDP, so hostname resolution continues to work. - // * forward: accept forwarded packets between the actor veth and pod eth0. - // - // TODO: Restrict the compatibility masquerade to DNS traffic sent to the - // configured cluster resolver and drop all other non-tunneled actor egress. + // * forward: drop actor UDP egress to any port but DNS, and accept the rest + // of the packets forwarded between the actor veth and pod eth0. if err := RemoveActorNftablesRules(); err != nil { return err } @@ -286,6 +284,9 @@ func InstallActorNftablesRules(egressPort uint16) error { Priority: nftables.ChainPriorityFilter, Policy: &acceptPolicy, }) + // Order matters: the accept below is a catch-all, so the drop has to precede + // it. + c.AddRule(actorNonDNSUDPDropRule(table, forward)) c.AddRule(&nftables.Rule{ Table: table, Chain: forward, @@ -343,13 +344,13 @@ func IPPayloadEqual(offset uint32, ip string) []expr.Any { } } -func TCPProtocol() []expr.Any { +func l4ProtocolEqual(proto byte) []expr.Any { return []expr.Any{ &expr.Meta{Key: expr.MetaKeyL4PROTO, Register: 1}, &expr.Cmp{ Op: expr.CmpOpEq, Register: 1, - Data: []byte{unix.IPPROTO_TCP}, + Data: []byte{proto}, }, } } @@ -361,7 +362,7 @@ func ActorEgressRedirectRule(table *nftables.Table, chain *nftables.Chain, port if port == 0 { return nil } - exprs := append(IPSourceEqual(ActorVethIP), TCPProtocol()...) + exprs := append(IPSourceEqual(ActorVethIP), l4ProtocolEqual(unix.IPPROTO_TCP)...) exprs = append(exprs, &expr.Immediate{ Register: 1, @@ -372,6 +373,36 @@ func ActorEgressRedirectRule(table *nftables.Table, chain *nftables.Chain, port return &nftables.Rule{Table: table, Chain: chain, Exprs: exprs} } +// actorNonDNSUDPDropRule returns the forward-chain rule that drops actor UDP +// egress to every destination port but [dnsPort]. +// +// The rule counts what it drops: a workload that legitimately needs UDP shows +// up as a rising counter in `nft list table ip ateom_actor` rather than as an +// unexplained timeout. +func actorNonDNSUDPDropRule(table *nftables.Table, chain *nftables.Chain) *nftables.Rule { + // dnsPort is the only destination port on which actor UDP egress is forwarded. + const dnsPort = 53 + + exprs := append(IPSourceEqual(ActorVethIP), l4ProtocolEqual(unix.IPPROTO_UDP)...) + exprs = append(exprs, + // Destination port, at offset 2 of the UDP header. + &expr.Payload{ + DestRegister: 1, + Base: expr.PayloadBaseTransportHeader, + Offset: 2, + Len: 2, + }, + &expr.Cmp{ + Op: expr.CmpOpNeq, + Register: 1, + Data: binaryutil.BigEndian.PutUint16(dnsPort), + }, + &expr.Counter{}, + &expr.Verdict{Kind: expr.VerdictDrop}, + ) + return &nftables.Rule{Table: table, Chain: chain, Exprs: exprs} +} + // CreateNetNSWithoutSwitching creates a named netns and returns its handle, // restoring the caller's current netns before returning. func CreateNetNSWithoutSwitching(name string) (netns.NsHandle, error) { @@ -491,8 +522,8 @@ func SetupActorNetwork(ctx context.Context, cfg NetworkConfig) (retErr error) { // Kubernetes-provided eth0 out of the worker pod. // // The nftables rules installed here redirect actor TCP egress to atunnel - // when configured and masquerade traffic the TCP tunnel does not handle - // (notably DNS over UDP). + // when configured, masquerade traffic the TCP tunnel does not handle + // (notably DNS over UDP), and drop actor UDP egress to any other port. // // Clean up stale state from a failed prior activation before creating the // next actor-side network. The worker currently runs one actor at a time. diff --git a/internal/ateomnet/net_linux_test.go b/internal/ateomnet/net_linux_test.go index b9c8ac45f4..36588db5b7 100644 --- a/internal/ateomnet/net_linux_test.go +++ b/internal/ateomnet/net_linux_test.go @@ -19,11 +19,13 @@ package ateomnet import ( "context" "errors" + "net" "runtime" "testing" "github.com/agent-substrate/substrate/internal/roottest" "github.com/google/nftables" + "github.com/google/nftables/expr" "github.com/vishvananda/netlink" "github.com/vishvananda/netns" ) @@ -239,6 +241,111 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) { }) } +// addForwardingTarget gives the pod netns somewhere to forward actor packets +// to, standing in for the real pod's eth0 and default route. Without it a +// forwarded packet is dropped for want of a route before it ever reaches the +// forward hook the rules under test live on. The device is a dummy, so the +// packets go nowhere after that, which is all the assertions need. +func addForwardingTarget(t *testing.T, cidr string) { + t.Helper() + link := &netlink.Dummy{LinkAttrs: netlink.LinkAttrs{Name: "target0"}} + if err := netlink.LinkAdd(link); err != nil { + t.Fatalf("creating the forwarding target link: %v", err) + } + if err := netlink.AddrAdd(link, MustParseAddr(cidr)); err != nil { + t.Fatalf("addressing the forwarding target link: %v", err) + } + if err := netlink.LinkSetUp(link); err != nil { + t.Fatalf("bringing up the forwarding target link: %v", err) + } +} + +// droppedUDPPackets reads the packet count off the forward chain's counted +// rule, which [actorNonDNSUDPDropRule] is. +func droppedUDPPackets(t *testing.T) uint64 { + t.Helper() + c := &nftables.Conn{} + tables, err := c.ListTablesOfFamily(nftables.TableFamilyIPv4) + if err != nil { + t.Fatalf("listing nftables tables: %v", err) + } + for _, table := range tables { + if table.Name != ActorNftTableName { + continue + } + rules, err := c.GetRules(table, &nftables.Chain{Name: "forward", Table: table}) + if err != nil { + t.Fatalf("listing forward chain rules: %v", err) + } + for _, rule := range rules { + for _, e := range rule.Exprs { + if counter, ok := e.(*expr.Counter); ok { + return counter.Packets + } + } + } + t.Fatalf("forward chain has no counted rule, got %d rules", len(rules)) + } + t.Fatalf("nftables table %q is missing", ActorNftTableName) + return 0 +} + +// sendUDP sends one datagram to addr and reports whether the local send +// succeeded. UDP has no acknowledgement, so a successful send says nothing +// about delivery -- the drop is observed through the nftables counter instead. +func sendUDP(t *testing.T, addr string) { + t.Helper() + conn, err := net.Dial("udp4", addr) + if err != nil { + t.Fatalf("dialing %s: %v", addr, err) + } + defer conn.Close() + if _, err := conn.Write([]byte("probe")); err != nil { + t.Fatalf("sending a datagram to %s: %v", addr, err) + } +} + +// TestActorNonDNSUDPIsDropped covers the forward-chain rule behaviorally: only +// TCP is redirected into atunnel, so UDP on any port but 53 must not reach the +// masquerade, and DNS must still get through or the sandbox cannot resolve +// anything. +func TestActorNonDNSUDPIsDropped(t *testing.T) { + roottest.Require(t, "creating network namespaces, veth pairs, and nftables rules") + ctx := context.Background() + + withTestNetNS(t, func(interior netns.NsHandle) { + requireNftables(t) + + const target = "192.0.2.1" + addForwardingTarget(t, "192.0.2.254/24") + if err := SetupActorNetwork(ctx, NetworkConfig{InteriorNetNS: interior}); err != nil { + t.Fatalf("SetupActorNetwork: %v", err) + } + + before := droppedUDPPackets(t) + if err := NetNSDo(ctx, interior, func(context.Context) error { + sendUDP(t, net.JoinHostPort(target, "53")) + return nil + }); err != nil { + t.Fatalf("sending DNS from the interior netns: %v", err) + } + if got := droppedUDPPackets(t); got != before { + t.Errorf("DNS datagram was dropped: counter went from %d to %d", before, got) + } + + if err := NetNSDo(ctx, interior, func(context.Context) error { + sendUDP(t, net.JoinHostPort(target, "443")) + sendUDP(t, net.JoinHostPort(target, "9999")) + return nil + }); err != nil { + t.Fatalf("sending non-DNS UDP from the interior netns: %v", err) + } + if got := droppedUDPPackets(t); got != before+2 { + t.Errorf("dropped packets = %d, want %d: non-DNS UDP reached the masquerade", got, before+2) + } + }) +} + // TestSetupActorNetworkHostVethHWAddr covers the micro-VM requirement: a CH // snapshot freezes the guest's ARP entry for the gateway, so the worker-side // veth MAC has to be exactly the one the caller asked for, on every pod.