diff --git a/firewall.go b/firewall.go index b58d8bb8..66313b04 100644 --- a/firewall.go +++ b/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) { diff --git a/firewall_test.go b/firewall_test.go index 565db30d..36489d8e 100644 --- a/firewall_test.go +++ b/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{}) diff --git a/interface.go b/interface.go index 96d44446..5d19be1e 100644 --- a/interface.go +++ b/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 +} diff --git a/interface_test.go b/interface_test.go index b66f1e81..7f9bc22b 100644 --- a/interface_test.go +++ b/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") +}