[client] Improve route acl (#2705)

- Update nftables library to v0.2.0 - Mark traffic that was originally destined for local and applies the input rules in the forward chain if said traffic was redirected (e.g. by Docker) - Add nft rules to internal map only if flush was successful - Improve error message if handle is 0 (= not found or hasn't been refreshed) - Add debug logging when route rules are added - Replace nftables userdata (rule ID) with a rule hash
2025-08-16 10:08:12 +02:00 · 2024-10-10 15:54:34 +02:00
parent 208a2b7169
commit 09bdd271f1
11 changed files with 267 additions and 74 deletions
--- a/client/firewall/iptables/acl_linux.go
+++ b/client/firewall/iptables/acl_linux.go
@ -11,6 +11,7 @@ import (
 	log "github.com/sirupsen/logrus"

 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
+	nbnet "github.com/netbirdio/netbird/util/net"
 )

 const (
@ -21,13 +22,19 @@ const (
 	chainNameOutputRules = "NETBIRD-ACL-OUTPUT"
 )

+type entry struct {
+	spec     []string
+	position int
+}
+
 type aclManager struct {
 	iptablesClient     *iptables.IPTables
 	wgIface            iFaceMapper
 	routingFwChainName string

-	entries    map[string][][]string
-	ipsetStore *ipsetStore
+	entries         map[string][][]string
+	optionalEntries map[string][]entry
+	ipsetStore      *ipsetStore
 }

 func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper, routingFwChainName string) (*aclManager, error) {
@ -36,8 +43,9 @@ func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper, routi
 		wgIface:            wgIface,
 		routingFwChainName: routingFwChainName,

-		entries:    make(map[string][][]string),
-		ipsetStore: newIpsetStore(),
+		entries:         make(map[string][][]string),
+		optionalEntries: make(map[string][]entry),
+		ipsetStore:      newIpsetStore(),
 	}

 	err := ipset.Init()
@ -46,6 +54,7 @@ func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper, routi
 	}

 	m.seedInitialEntries()
+	m.seedInitialOptionalEntries()

 	err = m.cleanChains()
 	if err != nil {
@ -232,6 +241,19 @@ func (m *aclManager) cleanChains() error {
 		}
 	}

+	ok, err = m.iptablesClient.ChainExists("mangle", "PREROUTING")
+	if err != nil {
+		return fmt.Errorf("list chains: %w", err)
+	}
+	if ok {
+		for _, rule := range m.entries["PREROUTING"] {
+			err := m.iptablesClient.DeleteIfExists("mangle", "PREROUTING", rule...)
+			if err != nil {
+				log.Errorf("failed to delete rule: %v, %s", rule, err)
+			}
+		}
+	}
+
 	for _, ipsetName := range m.ipsetStore.ipsetNames() {
 		if err := ipset.Flush(ipsetName); err != nil {
 			log.Errorf("flush ipset %q during reset: %v", ipsetName, err)
@ -267,6 +289,17 @@ func (m *aclManager) createDefaultChains() error {
 		}
 	}

+	for chainName, entries := range m.optionalEntries {
+		for _, entry := range entries {
+			if err := m.iptablesClient.InsertUnique(tableName, chainName, entry.position, entry.spec...); err != nil {
+				log.Errorf("failed to insert optional entry %v: %v", entry.spec, err)
+				continue
+			}
+			m.entries[chainName] = append(m.entries[chainName], entry.spec)
+		}
+	}
+	clear(m.optionalEntries)
+
 	return nil
 }

@ -295,6 +328,22 @@ func (m *aclManager) seedInitialEntries() {
 	m.appendToEntries("FORWARD", append([]string{"-o", m.wgIface.Name()}, established...))
 }

+func (m *aclManager) seedInitialOptionalEntries() {
+	m.optionalEntries["FORWARD"] = []entry{
+		{
+			spec:     []string{"-m", "mark", "--mark", fmt.Sprintf("%#x", nbnet.PreroutingFwmark), "-j", chainNameInputRules},
+			position: 2,
+		},
+	}
+
+	m.optionalEntries["PREROUTING"] = []entry{
+		{
+			spec:     []string{"-t", "mangle", "-i", m.wgIface.Name(), "-m", "addrtype", "--dst-type", "LOCAL", "-j", "MARK", "--set-mark", fmt.Sprintf("%#x", nbnet.PreroutingFwmark)},
+			position: 1,
+		},
+	}
+}
+
 func (m *aclManager) appendToEntries(chainName string, spec []string) {
 	m.entries[chainName] = append(m.entries[chainName], spec)
 }
--- a/client/firewall/iptables/manager_linux.go
+++ b/client/firewall/iptables/manager_linux.go
@ -78,7 +78,7 @@ func (m *Manager) AddPeerFiltering(
 }

 func (m *Manager) AddRouteFiltering(
-	sources [] netip.Prefix,
+	sources []netip.Prefix,
 	destination netip.Prefix,
 	proto firewall.Protocol,
 	sPort *firewall.Port,
--- a/client/firewall/iptables/router_linux.go
+++ b/client/firewall/iptables/router_linux.go
@ -305,10 +305,7 @@ func (r *router) cleanUpDefaultForwardRules() error {

 	log.Debug("flushing routing related tables")
 	for _, chain := range []string{chainRTFWD, chainRTNAT} {
-		table := tableFilter
-		if chain == chainRTNAT {
-			table = tableNat
-		}
+		table := r.getTableForChain(chain)

 		ok, err := r.iptablesClient.ChainExists(table, chain)
 		if err != nil {
@ -329,15 +326,19 @@ func (r *router) cleanUpDefaultForwardRules() error {
 func (r *router) createContainers() error {
 	for _, chain := range []string{chainRTFWD, chainRTNAT} {
 		if err := r.createAndSetupChain(chain); err != nil {
-			return fmt.Errorf("create chain %s: %v", chain, err)
+			return fmt.Errorf("create chain %s: %w", chain, err)
 		}
 	}

 	if err := r.insertEstablishedRule(chainRTFWD); err != nil {
-		return fmt.Errorf("insert established rule: %v", err)
+		return fmt.Errorf("insert established rule: %w", err)
 	}

-	return r.addJumpRules()
+	if err := r.addJumpRules(); err != nil {
+		return fmt.Errorf("add jump rules: %w", err)
+	}
+
+	return nil
 }

 func (r *router) createAndSetupChain(chain string) error {
--- a/client/firewall/manager/firewall.go
+++ b/client/firewall/manager/firewall.go
@ -132,7 +132,7 @@ func SetLegacyManagement(router LegacyManager, isLegacy bool) error {
 // GenerateSetName generates a unique name for an ipset based on the given sources.
 func GenerateSetName(sources []netip.Prefix) string {
 	// sort for consistent naming
-	sortPrefixes(sources)
+	SortPrefixes(sources)

 	var sourcesStr strings.Builder
 	for _, src := range sources {
@ -170,9 +170,9 @@ func MergeIPRanges(prefixes []netip.Prefix) []netip.Prefix {
 	return merged
 }

-// sortPrefixes sorts the given slice of netip.Prefix in place.
+// SortPrefixes sorts the given slice of netip.Prefix in place.
 // It sorts first by IP address, then by prefix length (most specific to least specific).
-func sortPrefixes(prefixes []netip.Prefix) {
+func SortPrefixes(prefixes []netip.Prefix) {
 	sort.Slice(prefixes, func(i, j int) bool {
 		addrCmp := prefixes[i].Addr().Compare(prefixes[j].Addr())
 		if addrCmp != 0 {
--- a/client/firewall/nftables/acl_linux.go
+++ b/client/firewall/nftables/acl_linux.go
@ -11,12 +11,14 @@ import (
 	"time"

 	"github.com/google/nftables"
+	"github.com/google/nftables/binaryutil"
 	"github.com/google/nftables/expr"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"

 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/iface"
+	nbnet "github.com/netbirdio/netbird/util/net"
 )

 const (
@ -29,6 +31,7 @@ const (
 	chainNameInputFilter   = "netbird-acl-input-filter"
 	chainNameOutputFilter  = "netbird-acl-output-filter"
 	chainNameForwardFilter = "netbird-acl-forward-filter"
+	chainNamePrerouting    = "netbird-rt-prerouting"

 	allowNetbirdInputRuleID = "allow Netbird incoming traffic"
 )
@ -40,15 +43,14 @@ var (
 )

 type AclManager struct {
-	rConn               *nftables.Conn
-	sConn               *nftables.Conn
-	wgIface             iFaceMapper
-	routeingFwChainName string
+	rConn              *nftables.Conn
+	sConn              *nftables.Conn
+	wgIface            iFaceMapper
+	routingFwChainName string

 	workTable        *nftables.Table
 	chainInputRules  *nftables.Chain
 	chainOutputRules *nftables.Chain
-	chainFwFilter    *nftables.Chain

 	ipsetStore *ipsetStore
 	rules      map[string]*Rule
@ -61,7 +63,7 @@ type iFaceMapper interface {
 	IsUserspaceBind() bool
 }

-func newAclManager(table *nftables.Table, wgIface iFaceMapper, routeingFwChainName string) (*AclManager, error) {
+func newAclManager(table *nftables.Table, wgIface iFaceMapper, routingFwChainName string) (*AclManager, error) {
 	// sConn is used for creating sets and adding/removing elements from them
 	// it's differ then rConn (which does create new conn for each flush operation)
 	// and is permanent. Using same connection for both type of operations
@ -72,11 +74,11 @@ func newAclManager(table *nftables.Table, wgIface iFaceMapper, routeingFwChainNa
 	}

 	m := &AclManager{
-		rConn:               &nftables.Conn{},
-		sConn:               sConn,
-		wgIface:             wgIface,
-		workTable:           table,
-		routeingFwChainName: routeingFwChainName,
+		rConn:              &nftables.Conn{},
+		sConn:              sConn,
+		wgIface:            wgIface,
+		workTable:          table,
+		routingFwChainName: routingFwChainName,

 		ipsetStore: newIpsetStore(),
 		rules:      make(map[string]*Rule),
@ -462,9 +464,9 @@ func (m *AclManager) createDefaultChains() (err error) {
 	}

 	// netbird-acl-forward-filter
-	m.chainFwFilter = m.createFilterChainWithHook(chainNameForwardFilter, nftables.ChainHookForward)
-	m.addJumpRulesToRtForward() // to netbird-rt-fwd
-	m.addDropExpressions(m.chainFwFilter, expr.MetaKeyIIFNAME)
+	chainFwFilter := m.createFilterChainWithHook(chainNameForwardFilter, nftables.ChainHookForward)
+	m.addJumpRulesToRtForward(chainFwFilter) // to netbird-rt-fwd
+	m.addDropExpressions(chainFwFilter, expr.MetaKeyIIFNAME)

 	err = m.rConn.Flush()
 	if err != nil {
@ -472,10 +474,96 @@ func (m *AclManager) createDefaultChains() (err error) {
 		return fmt.Errorf(flushError, err)
 	}

+	if err := m.allowRedirectedTraffic(chainFwFilter); err != nil {
+		log.Errorf("failed to allow redirected traffic: %s", err)
+	}
+
 	return nil
 }

-func (m *AclManager) addJumpRulesToRtForward() {
+// Makes redirected traffic originally destined for the host itself (now subject to the forward filter)
+// go through the input filter as well. This will enable e.g. Docker services to keep working by accessing the
+// netbird peer IP.
+func (m *AclManager) allowRedirectedTraffic(chainFwFilter *nftables.Chain) error {
+	preroutingChain := m.rConn.AddChain(&nftables.Chain{
+		Name:     chainNamePrerouting,
+		Table:    m.workTable,
+		Type:     nftables.ChainTypeFilter,
+		Hooknum:  nftables.ChainHookPrerouting,
+		Priority: nftables.ChainPriorityMangle,
+	})
+
+	m.addPreroutingRule(preroutingChain)
+
+	m.addFwmarkToForward(chainFwFilter)
+
+	if err := m.rConn.Flush(); err != nil {
+		return fmt.Errorf(flushError, err)
+	}
+
+	return nil
+}
+
+func (m *AclManager) addPreroutingRule(preroutingChain *nftables.Chain) {
+	m.rConn.AddRule(&nftables.Rule{
+		Table: m.workTable,
+		Chain: preroutingChain,
+		Exprs: []expr.Any{
+			&expr.Meta{
+				Key:      expr.MetaKeyIIFNAME,
+				Register: 1,
+			},
+			&expr.Cmp{
+				Op:       expr.CmpOpEq,
+				Register: 1,
+				Data:     ifname(m.wgIface.Name()),
+			},
+			&expr.Fib{
+				Register:       1,
+				ResultADDRTYPE: true,
+				FlagDADDR:      true,
+			},
+			&expr.Cmp{
+				Op:       expr.CmpOpEq,
+				Register: 1,
+				Data:     binaryutil.NativeEndian.PutUint32(unix.RTN_LOCAL),
+			},
+			&expr.Immediate{
+				Register: 1,
+				Data:     binaryutil.NativeEndian.PutUint32(nbnet.PreroutingFwmark),
+			},
+			&expr.Meta{
+				Key:            expr.MetaKeyMARK,
+				Register:       1,
+				SourceRegister: true,
+			},
+		},
+	})
+}
+
+func (m *AclManager) addFwmarkToForward(chainFwFilter *nftables.Chain) {
+	m.rConn.InsertRule(&nftables.Rule{
+		Table: m.workTable,
+		Chain: chainFwFilter,
+		Exprs: []expr.Any{
+			&expr.Meta{
+				Key:      expr.MetaKeyMARK,
+				Register: 1,
+			},
+			&expr.Cmp{
+				Op:       expr.CmpOpEq,
+				Register: 1,
+				Data:     binaryutil.NativeEndian.PutUint32(nbnet.PreroutingFwmark),
+			},
+			&expr.Verdict{
+				Kind:  expr.VerdictJump,
+				Chain: m.chainInputRules.Name,
+			},
+		},
+	})
+}
+
+func (m *AclManager) addJumpRulesToRtForward(chainFwFilter *nftables.Chain) {
 	expressions := []expr.Any{
 		&expr.Meta{Key: expr.MetaKeyIIFNAME, Register: 1},
 		&expr.Cmp{
@ -485,13 +573,13 @@ func (m *AclManager) addJumpRulesToRtForward() {
 		},
 		&expr.Verdict{
 			Kind:  expr.VerdictJump,
-			Chain: m.routeingFwChainName,
+			Chain: m.routingFwChainName,
 		},
 	}

 	_ = m.rConn.AddRule(&nftables.Rule{
 		Table: m.workTable,
-		Chain: m.chainFwFilter,
+		Chain: chainFwFilter,
 		Exprs: expressions,
 	})
 }
@ -509,7 +597,7 @@ func (m *AclManager) createChain(name string) *nftables.Chain {
 	return chain
 }

-func (m *AclManager) createFilterChainWithHook(name string, hookNum nftables.ChainHook) *nftables.Chain {
+func (m *AclManager) createFilterChainWithHook(name string, hookNum *nftables.ChainHook) *nftables.Chain {
 	polAccept := nftables.ChainPolicyAccept
 	chain := &nftables.Chain{
 		Name:     name,
--- a/client/firewall/nftables/router_linux.go
+++ b/client/firewall/nftables/router_linux.go
@ -10,6 +10,7 @@ import (
 	"net/netip"
 	"strings"

+	"github.com/davecgh/go-spew/spew"
 	"github.com/google/nftables"
 	"github.com/google/nftables/binaryutil"
 	"github.com/google/nftables/expr"
@ -24,7 +25,7 @@ import (

 const (
 	chainNameRoutingFw  = "netbird-rt-fwd"
-	chainNameRoutingNat = "netbird-rt-nat"
+	chainNameRoutingNat = "netbird-rt-postrouting"
 	chainNameForward    = "FORWARD"

 	userDataAcceptForwardRuleIif = "frwacceptiif"
@ -149,7 +150,6 @@ func (r *router) loadFilterTable() (*nftables.Table, error) {
 }

 func (r *router) createContainers() error {
-
 	r.chains[chainNameRoutingFw] = r.conn.AddChain(&nftables.Chain{
 		Name:  chainNameRoutingFw,
 		Table: r.workTable,
@ -157,25 +157,26 @@ func (r *router) createContainers() error {

 	insertReturnTrafficRule(r.conn, r.workTable, r.chains[chainNameRoutingFw])

+	prio := *nftables.ChainPriorityNATSource - 1
+
 	r.chains[chainNameRoutingNat] = r.conn.AddChain(&nftables.Chain{
 		Name:     chainNameRoutingNat,
 		Table:    r.workTable,
 		Hooknum:  nftables.ChainHookPostrouting,
-		Priority: nftables.ChainPriorityNATSource - 1,
+		Priority: &prio,
 		Type:     nftables.ChainTypeNAT,
 	})

 	r.acceptForwardRules()

-	err := r.refreshRulesMap()
-	if err != nil {
+	if err := r.refreshRulesMap(); err != nil {
 		log.Errorf("failed to clean up rules from FORWARD chain: %s", err)
 	}

-	err = r.conn.Flush()
-	if err != nil {
+	if err := r.conn.Flush(); err != nil {
 		return fmt.Errorf("nftables: unable to initialize table: %v", err)
 	}
+
 	return nil
 }

@ -188,6 +189,7 @@ func (r *router) AddRouteFiltering(
 	dPort *firewall.Port,
 	action firewall.Action,
 ) (firewall.Rule, error) {
+
 	ruleKey := id.GenerateRouteRuleKey(sources, destination, proto, sPort, dPort, action)
 	if _, ok := r.rules[string(ruleKey)]; ok {
 		return ruleKey, nil
@ -248,9 +250,18 @@ func (r *router) AddRouteFiltering(
 		UserData: []byte(ruleKey),
 	}

-	r.rules[string(ruleKey)] = r.conn.AddRule(rule)
+	rule = r.conn.AddRule(rule)

-	return ruleKey, r.conn.Flush()
+	log.Tracef("Adding route rule %s", spew.Sdump(rule))
+	if err := r.conn.Flush(); err != nil {
+		return nil, fmt.Errorf(flushError, err)
+	}
+
+	r.rules[string(ruleKey)] = rule
+
+	log.Debugf("nftables: added route rule: sources=%v, destination=%v, proto=%v, sPort=%v, dPort=%v, action=%v", sources, destination, proto, sPort, dPort, action)
+
+	return ruleKey, nil
 }

 func (r *router) getIpSetExprs(sources []netip.Prefix, exprs []expr.Any) ([]expr.Any, error) {
@ -288,6 +299,10 @@ func (r *router) DeleteRouteRule(rule firewall.Rule) error {
 		return nil
 	}

+	if nftRule.Handle == 0 {
+		return fmt.Errorf("route rule %s has no handle", ruleKey)
+	}
+
 	setName := r.findSetNameInRule(nftRule)

 	if err := r.deleteNftRule(nftRule, ruleKey); err != nil {
@ -658,7 +673,7 @@ func (r *router) RemoveNatRule(pair firewall.RouterPair) error {
 		return fmt.Errorf("nftables: received error while applying rule removal for %s: %v", pair.Destination, err)
 	}

-	log.Debugf("nftables: removed rules for %s", pair.Destination)
+	log.Debugf("nftables: removed nat rules for %s", pair.Destination)
 	return nil
 }

--- a/client/firewall/nftables/router_linux_test.go
+++ b/client/firewall/nftables/router_linux_test.go
@ -314,6 +314,10 @@ func TestRouter_AddRouteFiltering(t *testing.T) {
 			ruleKey, err := r.AddRouteFiltering(tt.sources, tt.destination, tt.proto, tt.sPort, tt.dPort, tt.action)
 			require.NoError(t, err, "AddRouteFiltering failed")

+			t.Cleanup(func() {
+				require.NoError(t, r.DeleteRouteRule(ruleKey), "Failed to delete rule")
+			})
+
 			// Check if the rule is in the internal map
 			rule, ok := r.rules[ruleKey.GetRuleID()]
 			assert.True(t, ok, "Rule not found in internal map")
@ -346,10 +350,6 @@ func TestRouter_AddRouteFiltering(t *testing.T) {

 			// Verify actual nftables rule content
 			verifyRule(t, nftRule, tt.sources, tt.destination, tt.proto, tt.sPort, tt.dPort, tt.direction, tt.action, tt.expectSet)
-
-			// Clean up
-			err = r.DeleteRouteRule(ruleKey)
-			require.NoError(t, err, "Failed to delete rule")
 		})
 	}
 }