Revert "[client] Trust wg interface in firewalld to bypass owner-flagged chains (#5928)"

This reverts commit 064ec1c832.

client/firewall/firewalld/firewalld.go

@@ -1,11 +0,0 @@
-// Package firewalld integrates with the firewalld daemon so NetBird can place
-// its wg interface into firewalld's "trusted" zone. This is required because
-// firewalld's nftables chains are created with NFT_CHAIN_OWNER on recent
-// versions, which returns EPERM to any other process that tries to insert
-// rules into them. The workaround mirrors what Tailscale does: let firewalld
-// itself add the accept rules to its own chains by trusting the interface.
-package firewalld
-
-// TrustedZone is the firewalld zone name used for interfaces whose traffic
-// should bypass firewalld filtering.
-const TrustedZone = "trusted"

client/firewall/firewalld/firewalld_linux.go

@@ -1,260 +0,0 @@
-//go:build linux
-
-package firewalld
-
-import (
-	"context"
-	"errors"
-	"fmt"
-	"os/exec"
-	"strings"
-	"sync"
-	"time"
-
-	"github.com/godbus/dbus/v5"
-	log "github.com/sirupsen/logrus"
-)
-
-const (
-	dbusDest      = "org.fedoraproject.FirewallD1"
-	dbusPath      = "/org/fedoraproject/FirewallD1"
-	dbusRootIface = "org.fedoraproject.FirewallD1"
-	dbusZoneIface = "org.fedoraproject.FirewallD1.zone"
-
-	errZoneAlreadySet = "ZONE_ALREADY_SET"
-	errAlreadyEnabled = "ALREADY_ENABLED"
-	errUnknownIface   = "UNKNOWN_INTERFACE"
-	errNotEnabled     = "NOT_ENABLED"
-
-	// callTimeout bounds each individual DBus or firewall-cmd invocation.
-	// A fresh context is created for each call so a slow DBus probe can't
-	// exhaust the deadline before the firewall-cmd fallback gets to run.
-	callTimeout = 3 * time.Second
-)
-
-var (
-	errDBusUnavailable = errors.New("firewalld dbus unavailable")
-
-	// trustLogOnce ensures the "added to trusted zone" message is logged at
-	// Info level only for the first successful add per process; repeat adds
-	// from other init paths are quieter.
-	trustLogOnce sync.Once
-
-	parentCtxMu sync.RWMutex
-	parentCtx   context.Context = context.Background()
-)
-
-// SetParentContext installs a parent context whose cancellation aborts any
-// in-flight TrustInterface call. It does not affect UntrustInterface, which
-// always uses a fresh Background-rooted timeout so cleanup can still run
-// during engine shutdown when the engine context is already cancelled.
-func SetParentContext(ctx context.Context) {
-	parentCtxMu.Lock()
-	parentCtx = ctx
-	parentCtxMu.Unlock()
-}
-
-func getParentContext() context.Context {
-	parentCtxMu.RLock()
-	defer parentCtxMu.RUnlock()
-	return parentCtx
-}
-
-// TrustInterface places iface into firewalld's trusted zone if firewalld is
-// running. It is idempotent and best-effort: errors are returned so callers
-// can log, but a non-running firewalld is not an error. Only the first
-// successful call per process logs at Info. Respects the parent context set
-// via SetParentContext so startup-time cancellation unblocks it.
-func TrustInterface(iface string) error {
-	parent := getParentContext()
-	if !isRunning(parent) {
-		return nil
-	}
-	if err := addTrusted(parent, iface); err != nil {
-		return fmt.Errorf("add %s to firewalld trusted zone: %w", iface, err)
-	}
-	trustLogOnce.Do(func() {
-		log.Infof("added %s to firewalld trusted zone", iface)
-	})
-	log.Debugf("firewalld: ensured %s is in trusted zone", iface)
-	return nil
-}
-
-// UntrustInterface removes iface from firewalld's trusted zone if firewalld
-// is running. Idempotent. Uses a Background-rooted timeout so it still runs
-// during shutdown after the engine context has been cancelled.
-func UntrustInterface(iface string) error {
-	if !isRunning(context.Background()) {
-		return nil
-	}
-	if err := removeTrusted(context.Background(), iface); err != nil {
-		return fmt.Errorf("remove %s from firewalld trusted zone: %w", iface, err)
-	}
-	return nil
-}
-
-func newCallContext(parent context.Context) (context.Context, context.CancelFunc) {
-	return context.WithTimeout(parent, callTimeout)
-}
-
-func isRunning(parent context.Context) bool {
-	ctx, cancel := newCallContext(parent)
-	ok, err := isRunningDBus(ctx)
-	cancel()
-	if err == nil {
-		return ok
-	}
-	if errors.Is(err, errDBusUnavailable) || errors.Is(err, context.DeadlineExceeded) {
-		ctx, cancel = newCallContext(parent)
-		defer cancel()
-		return isRunningCLI(ctx)
-	}
-	return false
-}
-
-func addTrusted(parent context.Context, iface string) error {
-	ctx, cancel := newCallContext(parent)
-	err := addDBus(ctx, iface)
-	cancel()
-	if err == nil {
-		return nil
-	}
-	if !errors.Is(err, errDBusUnavailable) {
-		log.Debugf("firewalld: dbus add failed, falling back to firewall-cmd: %v", err)
-	}
-	ctx, cancel = newCallContext(parent)
-	defer cancel()
-	return addCLI(ctx, iface)
-}
-
-func removeTrusted(parent context.Context, iface string) error {
-	ctx, cancel := newCallContext(parent)
-	err := removeDBus(ctx, iface)
-	cancel()
-	if err == nil {
-		return nil
-	}
-	if !errors.Is(err, errDBusUnavailable) {
-		log.Debugf("firewalld: dbus remove failed, falling back to firewall-cmd: %v", err)
-	}
-	ctx, cancel = newCallContext(parent)
-	defer cancel()
-	return removeCLI(ctx, iface)
-}
-
-func isRunningDBus(ctx context.Context) (bool, error) {
-	conn, err := dbus.SystemBus()
-	if err != nil {
-		return false, fmt.Errorf("%w: %v", errDBusUnavailable, err)
-	}
-	obj := conn.Object(dbusDest, dbusPath)
-
-	var zone string
-	if err := obj.CallWithContext(ctx, dbusRootIface+".getDefaultZone", 0).Store(&zone); err != nil {
-		return false, fmt.Errorf("firewalld getDefaultZone: %w", err)
-	}
-	return true, nil
-}
-
-func isRunningCLI(ctx context.Context) bool {
-	if _, err := exec.LookPath("firewall-cmd"); err != nil {
-		return false
-	}
-	return exec.CommandContext(ctx, "firewall-cmd", "--state").Run() == nil
-}
-
-func addDBus(ctx context.Context, iface string) error {
-	conn, err := dbus.SystemBus()
-	if err != nil {
-		return fmt.Errorf("%w: %v", errDBusUnavailable, err)
-	}
-	obj := conn.Object(dbusDest, dbusPath)
-
-	call := obj.CallWithContext(ctx, dbusZoneIface+".addInterface", 0, TrustedZone, iface)
-	if call.Err == nil {
-		return nil
-	}
-
-	if dbusErrContains(call.Err, errAlreadyEnabled) {
-		return nil
-	}
-
-	if dbusErrContains(call.Err, errZoneAlreadySet) {
-		move := obj.CallWithContext(ctx, dbusZoneIface+".changeZoneOfInterface", 0, TrustedZone, iface)
-		if move.Err != nil {
-			return fmt.Errorf("firewalld changeZoneOfInterface: %w", move.Err)
-		}
-		return nil
-	}
-
-	return fmt.Errorf("firewalld addInterface: %w", call.Err)
-}
-
-func removeDBus(ctx context.Context, iface string) error {
-	conn, err := dbus.SystemBus()
-	if err != nil {
-		return fmt.Errorf("%w: %v", errDBusUnavailable, err)
-	}
-	obj := conn.Object(dbusDest, dbusPath)
-
-	call := obj.CallWithContext(ctx, dbusZoneIface+".removeInterface", 0, TrustedZone, iface)
-	if call.Err == nil {
-		return nil
-	}
-
-	if dbusErrContains(call.Err, errUnknownIface) || dbusErrContains(call.Err, errNotEnabled) {
-		return nil
-	}
-
-	return fmt.Errorf("firewalld removeInterface: %w", call.Err)
-}
-
-func addCLI(ctx context.Context, iface string) error {
-	if _, err := exec.LookPath("firewall-cmd"); err != nil {
-		return fmt.Errorf("firewall-cmd not available: %w", err)
-	}
-
-	// --change-interface (no --permanent) binds the interface for the
-	// current runtime only; we do not want membership to persist across
-	// reboots because netbird re-asserts it on every startup.
-	out, err := exec.CommandContext(ctx,
-		"firewall-cmd", "--zone="+TrustedZone, "--change-interface="+iface,
-	).CombinedOutput()
-	if err != nil {
-		return fmt.Errorf("firewall-cmd change-interface: %w: %s", err, strings.TrimSpace(string(out)))
-	}
-	return nil
-}
-
-func removeCLI(ctx context.Context, iface string) error {
-	if _, err := exec.LookPath("firewall-cmd"); err != nil {
-		return fmt.Errorf("firewall-cmd not available: %w", err)
-	}
-
-	out, err := exec.CommandContext(ctx,
-		"firewall-cmd", "--zone="+TrustedZone, "--remove-interface="+iface,
-	).CombinedOutput()
-	if err != nil {
-		msg := strings.TrimSpace(string(out))
-		if strings.Contains(msg, errUnknownIface) || strings.Contains(msg, errNotEnabled) {
-			return nil
-		}
-		return fmt.Errorf("firewall-cmd remove-interface: %w: %s", err, msg)
-	}
-	return nil
-}
-
-func dbusErrContains(err error, code string) bool {
-	if err == nil {
-		return false
-	}
-	var de dbus.Error
-	if errors.As(err, &de) {
-		for _, b := range de.Body {
-			if s, ok := b.(string); ok && strings.Contains(s, code) {
-				return true
-			}
-		}
-	}
-	return strings.Contains(err.Error(), code)
-}

client/firewall/firewalld/firewalld_linux_test.go

@@ -1,49 +0,0 @@
-//go:build linux
-
-package firewalld
-
-import (
-	"errors"
-	"testing"
-
-	"github.com/godbus/dbus/v5"
-)
-
-func TestDBusErrContains(t *testing.T) {
-	tests := []struct {
-		name string
-		err  error
-		code string
-		want bool
-	}{
-		{"nil error", nil, errZoneAlreadySet, false},
-		{"plain error match", errors.New("ZONE_ALREADY_SET: wt0"), errZoneAlreadySet, true},
-		{"plain error miss", errors.New("something else"), errZoneAlreadySet, false},
-		{
-			"dbus.Error body match",
-			dbus.Error{Name: "org.fedoraproject.FirewallD1.Exception", Body: []any{"ZONE_ALREADY_SET: wt0"}},
-			errZoneAlreadySet,
-			true,
-		},
-		{
-			"dbus.Error body miss",
-			dbus.Error{Name: "org.fedoraproject.FirewallD1.Exception", Body: []any{"INVALID_INTERFACE"}},
-			errAlreadyEnabled,
-			false,
-		},
-		{
-			"dbus.Error non-string body falls back to Error()",
-			dbus.Error{Name: "x", Body: []any{123}},
-			"x",
-			true,
-		},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			got := dbusErrContains(tc.err, tc.code)
-			if got != tc.want {
-				t.Fatalf("dbusErrContains(%v, %q) = %v; want %v", tc.err, tc.code, got, tc.want)
-			}
-		})
-	}
-}

client/firewall/firewalld/firewalld_other.go

@@ -1,25 +0,0 @@
-//go:build !linux
-
-package firewalld
-
-import "context"
-
-// SetParentContext is a no-op on non-Linux platforms because firewalld only
-// runs on Linux.
-func SetParentContext(context.Context) {
-	// intentionally empty: firewalld is a Linux-only daemon
-}
-
-// TrustInterface is a no-op on non-Linux platforms because firewalld only
-// runs on Linux.
-func TrustInterface(string) error {
-	// intentionally empty: firewalld is a Linux-only daemon
-	return nil
-}
-
-// UntrustInterface is a no-op on non-Linux platforms because firewalld only
-// runs on Linux.
-func UntrustInterface(string) error {
-	// intentionally empty: firewalld is a Linux-only daemon
-	return nil
-}

client/firewall/iptables/manager_linux.go

@@ -12,7 +12,6 @@ import (
 	log "github.com/sirupsen/logrus"
 
 	nberrors "github.com/netbirdio/netbird/client/errors"
-	"github.com/netbirdio/netbird/client/firewall/firewalld"
 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/iface/wgaddr"
 	"github.com/netbirdio/netbird/client/internal/statemanager"
@@ -87,12 +86,6 @@ func (m *Manager) Init(stateManager *statemanager.Manager) error {
 		log.Warnf("raw table not available, notrack rules will be disabled: %v", err)
 	}
 
-	// Trust after all fatal init steps so a later failure doesn't leave the
-	// interface in firewalld's trusted zone without a corresponding Close.
-	if err := firewalld.TrustInterface(m.wgIface.Name()); err != nil {
-		log.Warnf("failed to trust interface in firewalld: %v", err)
-	}
-
 	// persist early to ensure cleanup of chains
 	go func() {
 		if err := stateManager.PersistState(context.Background()); err != nil {
@@ -198,12 +191,6 @@ func (m *Manager) Close(stateManager *statemanager.Manager) error {
 		merr = multierror.Append(merr, fmt.Errorf("reset router: %w", err))
 	}
 
-	// Appending to merr intentionally blocks DeleteState below so ShutdownState
-	// stays persisted and the crash-recovery path retries firewalld cleanup.
-	if err := firewalld.UntrustInterface(m.wgIface.Name()); err != nil {
-		merr = multierror.Append(merr, err)
-	}
-
 	// attempt to delete state only if all other operations succeeded
 	if merr == nil {
 		if err := stateManager.DeleteState(&ShutdownState{}); err != nil {
@@ -230,11 +217,6 @@ func (m *Manager) AllowNetbird() error {
 	if err != nil {
 		return fmt.Errorf("allow netbird interface traffic: %w", err)
 	}
-
-	if err := firewalld.TrustInterface(m.wgIface.Name()); err != nil {
-		log.Warnf("failed to trust interface in firewalld: %v", err)
-	}
-
 	return nil
 }
 

client/firewall/nftables/manager_linux.go

@@ -14,7 +14,6 @@ import (
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 
-	"github.com/netbirdio/netbird/client/firewall/firewalld"
 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/iface/wgaddr"
 	"github.com/netbirdio/netbird/client/internal/statemanager"
@@ -218,10 +217,6 @@ func (m *Manager) AllowNetbird() error {
 		return fmt.Errorf("flush allow input netbird rules: %w", err)
 	}
 
-	if err := firewalld.TrustInterface(m.wgIface.Name()); err != nil {
-		log.Warnf("failed to trust interface in firewalld: %v", err)
-	}
-
 	return nil
 }
 

client/firewall/nftables/router_linux.go

@@ -19,7 +19,6 @@ import (
 	"golang.org/x/sys/unix"
 
 	nberrors "github.com/netbirdio/netbird/client/errors"
-	"github.com/netbirdio/netbird/client/firewall/firewalld"
 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 	nbid "github.com/netbirdio/netbird/client/internal/acl/id"
 	"github.com/netbirdio/netbird/client/internal/routemanager/ipfwdstate"
@@ -41,8 +40,6 @@ const (
 	chainNameForward       = "FORWARD"
 	chainNameMangleForward = "netbird-mangle-forward"
 
-	firewalldTableName = "firewalld"
-
 	userDataAcceptForwardRuleIif = "frwacceptiif"
 	userDataAcceptForwardRuleOif = "frwacceptoif"
 	userDataAcceptInputRule      = "inputaccept"
@@ -136,10 +133,6 @@ func (r *router) Reset() error {
 		merr = multierror.Append(merr, fmt.Errorf("remove accept filter rules: %w", err))
 	}
 
-	if err := firewalld.UntrustInterface(r.wgIface.Name()); err != nil {
-		merr = multierror.Append(merr, err)
-	}
-
 	if err := r.removeNatPreroutingRules(); err != nil {
 		merr = multierror.Append(merr, fmt.Errorf("remove filter prerouting rules: %w", err))
 	}
@@ -287,10 +280,6 @@ func (r *router) createContainers() error {
 		log.Errorf("failed to add accept rules for the forward chain: %s", err)
 	}
 
-	if err := firewalld.TrustInterface(r.wgIface.Name()); err != nil {
-		log.Warnf("failed to trust interface in firewalld: %v", err)
-	}
-
 	if err := r.refreshRulesMap(); err != nil {
 		log.Errorf("failed to refresh rules: %s", err)
 	}
@@ -1330,13 +1319,6 @@ func (r *router) isExternalChain(chain *nftables.Chain) bool {
 		return false
 	}
 
-	// Skip firewalld-owned chains. Firewalld creates its chains with the
-	// NFT_CHAIN_OWNER flag, so inserting rules into them returns EPERM.
-	// We delegate acceptance to firewalld by trusting the interface instead.
-	if chain.Table.Name == firewalldTableName {
-		return false
-	}
-
 	// Skip all iptables-managed tables in the ip family
 	if chain.Table.Family == nftables.TableFamilyIPv4 && isIptablesTable(chain.Table.Name) {
 		return false

client/firewall/uspfilter/allow_netbird.go

@@ -3,9 +3,6 @@
 package uspfilter
 
 import (
-	log "github.com/sirupsen/logrus"
-
-	"github.com/netbirdio/netbird/client/firewall/firewalld"
 	"github.com/netbirdio/netbird/client/internal/statemanager"
 )
 
@@ -19,9 +16,6 @@ func (m *Manager) Close(stateManager *statemanager.Manager) error {
 	if m.nativeFirewall != nil {
 		return m.nativeFirewall.Close(stateManager)
 	}
-	if err := firewalld.UntrustInterface(m.wgIface.Name()); err != nil {
-		log.Warnf("failed to untrust interface in firewalld: %v", err)
-	}
 	return nil
 }
 
@@ -30,8 +24,5 @@ func (m *Manager) AllowNetbird() error {
 	if m.nativeFirewall != nil {
 		return m.nativeFirewall.AllowNetbird()
 	}
-	if err := firewalld.TrustInterface(m.wgIface.Name()); err != nil {
-		log.Warnf("failed to trust interface in firewalld: %v", err)
-	}
 	return nil
 }

client/firewall/uspfilter/common/iface.go

@@ -9,7 +9,6 @@ import (
 
 // IFaceMapper defines subset methods of interface required for manager
 type IFaceMapper interface {
-	Name() string
 	SetFilter(device.PacketFilter) error
 	Address() wgaddr.Address
 	GetWGDevice() *wgdevice.Device

client/firewall/uspfilter/filter_test.go

@@ -31,20 +31,12 @@ var logger = log.NewFromLogrus(logrus.StandardLogger())
 var flowLogger = netflow.NewManager(nil, []byte{}, nil).GetLogger()
 
 type IFaceMock struct {
-	NameFunc        func() string
 	SetFilterFunc   func(device.PacketFilter) error
 	AddressFunc     func() wgaddr.Address
 	GetWGDeviceFunc func() *wgdevice.Device
 	GetDeviceFunc   func() *device.FilteredDevice
 }
 
-func (i *IFaceMock) Name() string {
-	if i.NameFunc == nil {
-		return "wgtest"
-	}
-	return i.NameFunc()
-}
-
 func (i *IFaceMock) GetWGDevice() *wgdevice.Device {
 	if i.GetWGDeviceFunc == nil {
 		return nil

client/internal/engine.go

@@ -26,7 +26,6 @@ import (
 
 	nberrors "github.com/netbirdio/netbird/client/errors"
 	"github.com/netbirdio/netbird/client/firewall"
-	"github.com/netbirdio/netbird/client/firewall/firewalld"
 	firewallManager "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/iface"
 	"github.com/netbirdio/netbird/client/iface/device"
@@ -605,8 +604,6 @@ func (e *Engine) createFirewall() error {
 		return nil
 	}
 
-	firewalld.SetParentContext(e.ctx)
-
 	var err error
 	e.firewall, err = firewall.NewFirewall(e.wgInterface, e.stateManager, e.flowManager.GetLogger(), e.config.DisableServerRoutes, e.config.MTU)
 	if err != nil {