firewall: Let a reply come back through another of our gateways for an unsafe network

firewall.go

@@ -43,6 +43,10 @@ type conn struct {
 type Firewall struct {
 	Conntrack *FirewallConntrack
 
+	// siblings finds the other gateways we route an unsafe network through, so a reply that comes back through a
+	// different gateway than the flow went out through can be matched to it. Nil turns that off.
+	siblings gatewaySiblings
+
 	InRules  *FirewallTable
 	OutRules *FirewallTable
 
@@ -426,6 +430,7 @@ var ErrNoMatchingRule = errors.New("no matching rule in firewall table")
 // returns nil if the packet should not be dropped.
 func (f *Firewall) Drop(fp firewall.Packet, incoming bool, h *HostInfo, caPool *cert.CAPool, localCache firewall.ConntrackCache) error {
 	// Make sure remote address matches nebula certificate, and determine how to treat it
+	unsafe := false
 	if h.networks == nil {
 		// Simple case: Certificate has one address and no unsafe networks
 		if h.vpnAddrs[0] != fp.RemoteAddr {
@@ -445,7 +450,7 @@ func (f *Firewall) Drop(fp firewall.Packet, incoming bool, h *HostInfo, caPool *
 			f.metrics(incoming).droppedRemoteAddr.Inc(1)
 			return ErrPeerRejected // reject for now, one day this may have different FW rules
 		case NetworkTypeUnsafe:
-			break // nothing special, one day this may have different FW rules
+			unsafe = true // nothing else special, one day this may have different FW rules
 		default:
 			f.metrics(incoming).droppedRemoteAddr.Inc(1)
 			return ErrUnknownNetworkType //should never happen
@@ -474,6 +479,9 @@ func (f *Firewall) Drop(fp firewall.Packet, incoming bool, h *HostInfo, caPool *
 
 	// We now know which firewall table to check against
 	if !table.match(fp, incoming, h.ConnectionState.peerCert, caPool) {
+		if unsafe && f.inheritConn(key, incoming, h, caPool) {
+			return nil
+		}
 		f.metrics(incoming).droppedNoRule.Inc(1)
 		return ErrNoMatchingRule
 	}
@@ -616,6 +624,57 @@ func (f *Firewall) addConn(key firewall.ConntrackKey, incoming bool, h *HostInfo
 	conntrack.Unlock()
 }
 
+// gatewaySiblings finds the other gateways we route an unsafe network through
+type gatewaySiblings interface {
+	// siblingPeers appends to buf the conntrack peer of every other gateway we route remote through, if h is one of
+	// those gateways
+	siblingPeers(remote netip.Addr, h *HostInfo, buf []netip.Addr) []netip.Addr
+}
+
+// inheritConn lets h carry a flow that another of our gateways for the same unsafe network already carries. We
+// spread the flows we open across the gateways, but the hosts behind them answer through whichever gateway their own
+// routing picks, so a reply can come back through a different gateway than the flow went out through. The flow is
+// checked against the rules that allowed it in the first place with h's certificate, the same check an entry gets
+// when its peer reconnects. Peers that aren't one of our gateways for the network never inherit a flow.
+func (f *Firewall) inheritConn(key firewall.ConntrackKey, incoming bool, h *HostInfo, caPool *cert.CAPool) bool {
+	if f.siblings == nil {
+		return false
+	}
+
+	var buf [4]netip.Addr
+	now := time.Now()
+	for _, peer := range f.siblings.siblingPeers(key.RemoteAddr, h, buf[:0]) {
+		sk := key
+		sk.Peer = peer
+		conntrack := f.Conntrack
+		conntrack.Lock()
+		c, ok := conntrack.Conns[sk]
+		live := ok && c.Expires.After(now)
+		var allowedIncoming bool
+		if live {
+			allowedIncoming = c.incoming
+		}
+		conntrack.Unlock()
+
+		// The rules for this packet's own direction already said no, only a flow allowed the other way can help
+		if !live || allowedIncoming == incoming {
+			continue
+		}
+
+		table := f.OutRules
+		if allowedIncoming {
+			table = f.InRules
+		}
+		if !table.match(key.Packet, allowedIncoming, h.ConnectionState.peerCert, caPool) {
+			continue
+		}
+
+		f.addConn(key, allowedIncoming, h)
+		return true
+	}
+	return false
+}
+
 // Evict checks if a conntrack entry has expired, if so it is removed, if not it is re-added to the wheel
 // Caller must own the connMutex lock!
 func (f *Firewall) evict(key firewall.ConntrackKey) {

firewall_test.go

@@ -1053,6 +1053,109 @@ func TestFirewall_ConntrackUnsafeNetworkPeers(t *testing.T) {
 	}
 }
 
+// fakeSiblings maps each gateway to the conntrack peers of the gateways it shares a route with
+type fakeSiblings map[*HostInfo][]netip.Addr
+
+func (s fakeSiblings) siblingPeers(_ netip.Addr, h *HostInfo, buf []netip.Addr) []netip.Addr {
+	return append(buf, s[h]...)
+}
+
+func TestFirewall_ConntrackGatewaySiblings(t *testing.T) {
+	l := test.NewLoggerWithOutput(&bytes.Buffer{})
+
+	myVpnNetworksTable := new(bart.Lite)
+	myVpnNetworksTable.Insert(netip.MustParsePrefix("192.0.2.1/24"))
+
+	owner := &dummyCert{
+		name:     "owner",
+		networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.1/24")},
+	}
+
+	// Routers for the same unsafe network. a and b are both our gateways for it, c only claims it in its cert
+	router := func(name string, addr string, groups ...string) *HostInfo {
+		inverted := map[string]struct{}{}
+		for _, g := range groups {
+			inverted[g] = struct{}{}
+		}
+		c := &cert.CachedCertificate{
+			Certificate: &dummyCert{
+				name:           name,
+				groups:         groups,
+				networks:       []netip.Prefix{netip.MustParsePrefix(addr + "/24")},
+				unsafeNetworks: []netip.Prefix{netip.MustParsePrefix("198.51.100.0/24")},
+			},
+			InvertedGroups: inverted,
+		}
+		h := &HostInfo{
+			ConnectionState: &ConnectionState{peerCert: c},
+			vpnAddrs:        []netip.Addr{netip.MustParseAddr(addr)},
+		}
+		h.buildNetworks(myVpnNetworksTable, c.Certificate)
+		return h
+	}
+	a := router("a", "192.0.2.2", "trusted")
+	b := router("b", "192.0.2.3", "trusted")
+	c := router("c", "192.0.2.4", "trusted")
+	siblings := fakeSiblings{a: {b.vpnAddrs[0]}, b: {a.vpnAddrs[0]}}
+
+	cp := cert.NewCAPool()
+	flow := firewall.Packet{
+		LocalAddr:  netip.MustParseAddr("192.0.2.1"),
+		RemoteAddr: netip.MustParseAddr("198.51.100.5"),
+		LocalPort:  55000,
+		RemotePort: 443,
+		Protocol:   iputil.IPProtocolTCP,
+	}
+
+	// We open flows through our gateways, nothing is allowed in on its own
+	newFirewall := func(outGroup string) *Firewall {
+		fw := NewFirewall(l, time.Second, time.Minute, time.Hour, owner)
+		require.NoError(t, fw.AddRule(false, firewall.ProtoAny, 0, 0, []string{outGroup}, "", "", "", "", ""))
+		fw.siblings = siblings
+		return fw
+	}
+
+	t.Run("reply through the other gateway", func(t *testing.T) {
+		fw := newFirewall("trusted")
+		require.NoError(t, fw.Drop(flow, false, b, cp, nil))
+		require.NoError(t, fw.Drop(flow, true, a, cp, nil))
+
+		// a has its own entry now, and keeps the flow when b's is gone
+		delete(fw.Conntrack.Conns, firewall.ConntrackKey{Packet: flow, Peer: b.vpnAddrs[0]})
+		require.NoError(t, fw.Drop(flow, true, a, cp, nil))
+	})
+
+	t.Run("not our gateway", func(t *testing.T) {
+		fw := newFirewall("trusted")
+		require.NoError(t, fw.Drop(flow, false, b, cp, nil))
+		assert.Equal(t, ErrNoMatchingRule, fw.Drop(flow, true, c, cp, nil))
+	})
+
+	t.Run("off without siblings", func(t *testing.T) {
+		fw := newFirewall("trusted")
+		fw.siblings = nil
+		require.NoError(t, fw.Drop(flow, false, b, cp, nil))
+		assert.Equal(t, ErrNoMatchingRule, fw.Drop(flow, true, a, cp, nil))
+	})
+
+	t.Run("expired sibling entry", func(t *testing.T) {
+		fw := newFirewall("trusted")
+		require.NoError(t, fw.Drop(flow, false, b, cp, nil))
+		fw.Conntrack.Conns[firewall.ConntrackKey{Packet: flow, Peer: b.vpnAddrs[0]}].Expires = time.Now().Add(-time.Second)
+		assert.Equal(t, ErrNoMatchingRule, fw.Drop(flow, true, a, cp, nil))
+	})
+
+	t.Run("the original rules must allow the new gateway", func(t *testing.T) {
+		fw := newFirewall("b-only")
+		b.ConnectionState.peerCert.InvertedGroups["b-only"] = struct{}{}
+		defer delete(b.ConnectionState.peerCert.InvertedGroups, "b-only")
+
+		require.NoError(t, fw.Drop(flow, false, b, cp, nil))
+		assert.Equal(t, ErrNoMatchingRule, fw.Drop(flow, true, a, cp, nil))
+	})
+
+}
+
 func TestFirewall_ConntrackPeerRehandshake(t *testing.T) {
 	l := test.NewLoggerWithOutput(&bytes.Buffer{})
 

interface.go

@@ -25,6 +25,7 @@ import (
 	"github.com/slackhq/nebula/overlay/batch"
 	"github.com/slackhq/nebula/overlay/tio"
 	"github.com/slackhq/nebula/ratelimit"
+	"github.com/slackhq/nebula/routing"
 	"github.com/slackhq/nebula/udp"
 )
 
@@ -268,6 +269,9 @@ func NewInterface(ctx context.Context, c *InterfaceConfig) (*Interface, error) {
 	ifce.reQueryWait.Store(int64(c.reQueryWait))
 
 	ifce.connectionManager.intf = ifce
+	if ifce.firewall != nil {
+		ifce.firewall.siblings = ifce
+	}
 
 	// Held until Close so waiting on the interface blocks until the resources are actually released
 	ifce.wg.Add(1)
@@ -554,6 +558,7 @@ func (f *Interface) reloadFirewall(c *config.C, r *config.Reload) {
 	}
 
 	oldFw := f.firewall
+	fw.siblings = f
 	conntrack := oldFw.Conntrack
 	conntrack.Lock()
 	defer conntrack.Unlock()
@@ -711,3 +716,24 @@ func (f *Interface) Close() error {
 	f.wg.Done()
 	return errors.Join(errs...)
 }
+
+// siblingPeers implements gatewaySiblings from the routes and tunnels we have now. Routes name a gateway by any of its
+// vpn addresses, conntrack keys a peer by its highest one, so each gateway is resolved through its tunnel. A gateway
+// we have no tunnel to can't be carrying a live flow.
+func (f *Interface) siblingPeers(remote netip.Addr, h *HostInfo, buf []netip.Addr) []netip.Addr {
+	gateways := f.inside.RoutesFor(remote)
+	isGateway := func(g routing.Gateway) bool { return slices.Contains(h.vpnAddrs, g.Addr()) }
+	if len(gateways) < 2 || !slices.ContainsFunc(gateways, isGateway) {
+		return buf
+	}
+
+	for _, g := range gateways {
+		if isGateway(g) {
+			continue
+		}
+		if s := f.hostMap.QueryVpnAddr(g.Addr()); s != nil && s != h {
+			buf = append(buf, s.vpnAddrs[len(s.vpnAddrs)-1])
+		}
+	}
+	return buf
+}

interface_test.go

@@ -4,8 +4,11 @@ import (
 	"net/netip"
 	"testing"
 
+	"github.com/gaissmai/bart"
 	"github.com/slackhq/nebula/cert"
 	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/overlay/overlaytest"
+	"github.com/slackhq/nebula/routing"
 	"github.com/slackhq/nebula/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -140,3 +143,42 @@ func TestReloadFirewall_BadRulesFailTheReload(t *testing.T) {
 	assert.True(t, res.Failed)
 	assert.Same(t, fw, f.firewall)
 }
+
+// routesTun is a NoopTun with a route table
+type routesTun struct {
+	overlaytest.NoopTun
+	routes *bart.Table[routing.Gateways]
+}
+
+func (t routesTun) RoutesFor(ip netip.Addr) routing.Gateways {
+	r, _ := t.routes.Lookup(ip)
+	return r
+}
+
+func TestInterface_SiblingPeers(t *testing.T) {
+	addr := netip.MustParseAddr
+	routes := new(bart.Table[routing.Gateways])
+	// a and b carry 198.51.100.0/24, the route names a by its v4 address
+	routes.Insert(netip.MustParsePrefix("198.51.100.0/24"), routing.Gateways{
+		routing.NewGateway(addr("192.0.2.2"), 1),
+		routing.NewGateway(addr("192.0.2.3"), 1),
+		routing.NewGateway(addr("192.0.2.5"), 1), // no tunnel to this one
+	})
+	routes.Insert(netip.MustParsePrefix("203.0.113.0/24"), routing.Gateways{routing.NewGateway(addr("192.0.2.2"), 1)})
+
+	hm := newHostMap(test.NewLogger())
+	a := &HostInfo{vpnAddrs: []netip.Addr{addr("192.0.2.2"), addr("fd00::2")}, localIndexId: 1}
+	b := &HostInfo{vpnAddrs: []netip.Addr{addr("192.0.2.3"), addr("fd00::3")}, localIndexId: 2}
+	c := &HostInfo{vpnAddrs: []netip.Addr{addr("192.0.2.4")}, localIndexId: 3}
+	for _, h := range []*HostInfo{a, b, c} {
+		hm.unlockedAddHostInfo(h, &Interface{})
+	}
+	f := &Interface{inside: routesTun{routes: routes}, hostMap: hm}
+
+	// Siblings come back as their highest vpn address, the one conntrack keys on
+	assert.Equal(t, []netip.Addr{addr("fd00::3")}, f.siblingPeers(addr("198.51.100.5"), a, nil))
+	assert.Equal(t, []netip.Addr{addr("fd00::2")}, f.siblingPeers(addr("198.51.100.5"), b, nil))
+	assert.Empty(t, f.siblingPeers(addr("198.51.100.5"), c, nil), "not one of our gateways for the network")
+	assert.Empty(t, f.siblingPeers(addr("203.0.113.5"), a, nil), "a single gateway route has no siblings")
+	assert.Empty(t, f.siblingPeers(addr("192.0.2.3"), a, nil), "no route")
+}