From 4db673c2889c729519c5ba997d26b1fa721acc8f Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Sun, 2 Feb 2025 17:06:34 +0100 Subject: [PATCH 1/4] Manage the ip forwarding sysctl setting in global way --- client/firewall/iptables/router_linux.go | 19 ++++++++ client/firewall/nftables/router_linux.go | 29 ++++++++++-- .../routemanager/ipfwdstate/ipfwdstate.go | 46 +++++++++++++++++++ .../routemanager/server_nonandroid.go | 8 ---- .../routemanager/systemops/systemops_linux.go | 5 ++ 5 files changed, 94 insertions(+), 13 deletions(-) create mode 100644 client/internal/routemanager/ipfwdstate/ipfwdstate.go diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index d3441c69a3a..9dafde90047 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -16,6 +16,7 @@ import ( nberrors "github.com/netbirdio/netbird/client/errors" firewall "github.com/netbirdio/netbird/client/firewall/manager" "github.com/netbirdio/netbird/client/internal/acl/id" + "github.com/netbirdio/netbird/client/internal/routemanager/ipfwdstate" "github.com/netbirdio/netbird/client/internal/routemanager/refcounter" "github.com/netbirdio/netbird/client/internal/statemanager" nbnet "github.com/netbirdio/netbird/util/net" @@ -76,6 +77,7 @@ type router struct { legacyManagement bool stateManager *statemanager.Manager + ipFwdState *ipfwdstate.IPForwardingState } func newRouter(iptablesClient *iptables.IPTables, wgIface iFaceMapper) (*router, error) { @@ -83,6 +85,7 @@ func newRouter(iptablesClient *iptables.IPTables, wgIface iFaceMapper) (*router, iptablesClient: iptablesClient, rules: make(map[string][]string), wgIface: wgIface, + ipFwdState: ipfwdstate.NewIPForwardingState(), } r.ipsetCounter = refcounter.New( @@ -217,6 +220,10 @@ func (r *router) deleteIpSet(setName string) error { // AddNatRule inserts an iptables rule pair into the nat chain func (r *router) AddNatRule(pair firewall.RouterPair) error { + if err := r.ipFwdState.RequestForwarding(); err != nil { + return err + } + if r.legacyManagement { log.Warnf("This peer is connected to a NetBird Management service with an older version. Allowing all traffic for %s", pair.Destination) if err := r.addLegacyRouteRule(pair); err != nil { @@ -243,6 +250,10 @@ func (r *router) AddNatRule(pair firewall.RouterPair) error { // RemoveNatRule removes an iptables rule pair from forwarding and nat chains func (r *router) RemoveNatRule(pair firewall.RouterPair) error { + if err := r.ipFwdState.ReleaseForwarding(); err != nil { + log.Errorf("failed to disable sysctl IP forwarding: %v", err) + } + if err := r.removeNatRule(pair); err != nil { return fmt.Errorf("remove nat rule: %w", err) } @@ -575,6 +586,10 @@ func (r *router) updateState() { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { + if err := r.ipFwdState.ReleaseForwarding(); err != nil { + return nil, err + } + ruleKey := rule.ID() if _, exists := r.rules[ruleKey+dnatSuffix]; exists { return rule, nil @@ -669,6 +684,10 @@ func (r *router) rollbackRules(rules map[string]ruleInfo) error { } func (r *router) DeleteDNATRule(rule firewall.Rule) error { + if err := r.ipFwdState.ReleaseForwarding(); err != nil { + log.Errorf("failed to disable sysctl IP forwarding: %v", err) + } + ruleKey := rule.ID() var merr *multierror.Error diff --git a/client/firewall/nftables/router_linux.go b/client/firewall/nftables/router_linux.go index 3a96ea39bfe..e5b90e7026f 100644 --- a/client/firewall/nftables/router_linux.go +++ b/client/firewall/nftables/router_linux.go @@ -21,6 +21,7 @@ import ( nberrors "github.com/netbirdio/netbird/client/errors" firewall "github.com/netbirdio/netbird/client/firewall/manager" "github.com/netbirdio/netbird/client/internal/acl/id" + "github.com/netbirdio/netbird/client/internal/routemanager/ipfwdstate" "github.com/netbirdio/netbird/client/internal/routemanager/refcounter" nbnet "github.com/netbirdio/netbird/util/net" ) @@ -56,16 +57,18 @@ type router struct { ipsetCounter *refcounter.Counter[string, []netip.Prefix, *nftables.Set] wgIface iFaceMapper + ipFwdState *ipfwdstate.IPForwardingState legacyManagement bool } func newRouter(workTable *nftables.Table, wgIface iFaceMapper) (*router, error) { r := &router{ - conn: &nftables.Conn{}, - workTable: workTable, - chains: make(map[string]*nftables.Chain), - rules: make(map[string]*nftables.Rule), - wgIface: wgIface, + conn: &nftables.Conn{}, + workTable: workTable, + chains: make(map[string]*nftables.Chain), + rules: make(map[string]*nftables.Rule), + wgIface: wgIface, + ipFwdState: ipfwdstate.NewIPForwardingState(), } r.ipsetCounter = refcounter.New( @@ -464,6 +467,10 @@ func (r *router) deleteNftRule(rule *nftables.Rule, ruleKey string) error { // AddNatRule appends a nftables rule pair to the nat chain func (r *router) AddNatRule(pair firewall.RouterPair) error { + if err := r.ipFwdState.RequestForwarding(); err != nil { + return err + } + if err := r.refreshRulesMap(); err != nil { return fmt.Errorf(refreshRulesMapError, err) } @@ -890,6 +897,10 @@ func (r *router) removeAcceptForwardRulesIptables(ipt *iptables.IPTables) error // RemoveNatRule removes the prerouting mark rule func (r *router) RemoveNatRule(pair firewall.RouterPair) error { + if err := r.ipFwdState.ReleaseForwarding(); err != nil { + log.Errorf("failed to disable sysctl IP forwarding: %v", err) + } + if err := r.refreshRulesMap(); err != nil { return fmt.Errorf(refreshRulesMapError, err) } @@ -951,6 +962,10 @@ func (r *router) refreshRulesMap() error { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { + if err := r.ipFwdState.ReleaseForwarding(); err != nil { + return nil, err + } + ruleKey := rule.ID() if _, exists := r.rules[ruleKey+dnatSuffix]; exists { return rule, nil @@ -1174,6 +1189,10 @@ func (r *router) addDnatMasq(rule firewall.ForwardRule, protoNum uint8, ruleKey } func (r *router) DeleteDNATRule(rule firewall.Rule) error { + if err := r.ipFwdState.ReleaseForwarding(); err != nil { + log.Errorf("failed to disable sysctl IP forwarding: %v", err) + } + ruleKey := rule.ID() if err := r.refreshRulesMap(); err != nil { diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go new file mode 100644 index 00000000000..6fcdb0e03f2 --- /dev/null +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -0,0 +1,46 @@ +package ipfwdstate + +import ( + "github.com/netbirdio/netbird/client/internal/routemanager/systemops" +) + +type IPForwardingState struct { + enabledCounter int +} + +func NewIPForwardingState() *IPForwardingState { + return &IPForwardingState{} +} + +func (f *IPForwardingState) RequestForwarding() error { + if f.enabledCounter != 0 { + f.enabledCounter++ + return nil + } + + if err := systemops.EnableIPForwarding(); err != nil { + return err + } + f.enabledCounter = 1 + + return nil +} + +func (f *IPForwardingState) ReleaseForwarding() error { + if f.enabledCounter == 0 { + return nil + } + + if f.enabledCounter > 1 { + f.enabledCounter-- + return nil + } + + // if failed to disable IP forwarding we anyway decrement the counter + f.enabledCounter = 0 + + if err := systemops.DisableIPForwarding(); err != nil { + return err + } + return nil +} diff --git a/client/internal/routemanager/server_nonandroid.go b/client/internal/routemanager/server_nonandroid.go index b60cb318e67..6ff80e52dcf 100644 --- a/client/internal/routemanager/server_nonandroid.go +++ b/client/internal/routemanager/server_nonandroid.go @@ -13,7 +13,6 @@ import ( firewall "github.com/netbirdio/netbird/client/firewall/manager" "github.com/netbirdio/netbird/client/iface" "github.com/netbirdio/netbird/client/internal/peer" - "github.com/netbirdio/netbird/client/internal/routemanager/systemops" "github.com/netbirdio/netbird/route" ) @@ -70,13 +69,6 @@ func (m *serverRouter) updateRoutes(routesMap map[route.ID]*route.Route) error { m.routes[id] = newRoute } - if len(m.routes) > 0 { - err := systemops.EnableIPForwarding() - if err != nil { - return err - } - } - return nil } diff --git a/client/internal/routemanager/systemops/systemops_linux.go b/client/internal/routemanager/systemops/systemops_linux.go index 1da92cc8059..3736819d191 100644 --- a/client/internal/routemanager/systemops/systemops_linux.go +++ b/client/internal/routemanager/systemops/systemops_linux.go @@ -375,6 +375,11 @@ func EnableIPForwarding() error { return err } +func DisableIPForwarding() error { + _, err := sysctl.Set(ipv4ForwardingPath, 0, false) + return err +} + // entryExists checks if the specified ID or name already exists in the rt_tables file // and verifies if existing names start with "netbird_". func entryExists(file *os.File, id int) (bool, error) { From b9029e09918387ff25f22b24e1dbd5f5280fc315 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Sun, 2 Feb 2025 17:11:06 +0100 Subject: [PATCH 2/4] Change logging --- client/firewall/iptables/router_linux.go | 4 ++-- client/firewall/nftables/router_linux.go | 4 ++-- client/internal/routemanager/ipfwdstate/ipfwdstate.go | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index 9dafde90047..f2392f2abba 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -251,7 +251,7 @@ func (r *router) AddNatRule(pair firewall.RouterPair) error { // RemoveNatRule removes an iptables rule pair from forwarding and nat chains func (r *router) RemoveNatRule(pair firewall.RouterPair) error { if err := r.ipFwdState.ReleaseForwarding(); err != nil { - log.Errorf("failed to disable sysctl IP forwarding: %v", err) + log.Errorf("%v", err) } if err := r.removeNatRule(pair); err != nil { @@ -685,7 +685,7 @@ func (r *router) rollbackRules(rules map[string]ruleInfo) error { func (r *router) DeleteDNATRule(rule firewall.Rule) error { if err := r.ipFwdState.ReleaseForwarding(); err != nil { - log.Errorf("failed to disable sysctl IP forwarding: %v", err) + log.Errorf("%v", err) } ruleKey := rule.ID() diff --git a/client/firewall/nftables/router_linux.go b/client/firewall/nftables/router_linux.go index e5b90e7026f..70a2595eec4 100644 --- a/client/firewall/nftables/router_linux.go +++ b/client/firewall/nftables/router_linux.go @@ -898,7 +898,7 @@ func (r *router) removeAcceptForwardRulesIptables(ipt *iptables.IPTables) error // RemoveNatRule removes the prerouting mark rule func (r *router) RemoveNatRule(pair firewall.RouterPair) error { if err := r.ipFwdState.ReleaseForwarding(); err != nil { - log.Errorf("failed to disable sysctl IP forwarding: %v", err) + log.Errorf("%v", err) } if err := r.refreshRulesMap(); err != nil { @@ -1190,7 +1190,7 @@ func (r *router) addDnatMasq(rule firewall.ForwardRule, protoNum uint8, ruleKey func (r *router) DeleteDNATRule(rule firewall.Rule) error { if err := r.ipFwdState.ReleaseForwarding(); err != nil { - log.Errorf("failed to disable sysctl IP forwarding: %v", err) + log.Errorf("%v", err) } ruleKey := rule.ID() diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go index 6fcdb0e03f2..b7cd779541a 100644 --- a/client/internal/routemanager/ipfwdstate/ipfwdstate.go +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -1,6 +1,7 @@ package ipfwdstate import ( + "fmt" "github.com/netbirdio/netbird/client/internal/routemanager/systemops" ) @@ -19,7 +20,7 @@ func (f *IPForwardingState) RequestForwarding() error { } if err := systemops.EnableIPForwarding(); err != nil { - return err + return fmt.Errorf("failed to enable IP forwarding with sysctl: %w", err) } f.enabledCounter = 1 @@ -40,7 +41,7 @@ func (f *IPForwardingState) ReleaseForwarding() error { f.enabledCounter = 0 if err := systemops.DisableIPForwarding(); err != nil { - return err + return fmt.Errorf("failed to disable IP forwarding with sysctl: %w", err) } return nil } From 59303305098a1db31d8db6a34a677ff8bda54446 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Sun, 2 Feb 2025 17:36:59 +0100 Subject: [PATCH 3/4] Fix typo and add log --- client/firewall/iptables/router_linux.go | 2 +- client/firewall/nftables/router_linux.go | 2 +- client/internal/routemanager/ipfwdstate/ipfwdstate.go | 6 ++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index f2392f2abba..e9dfbd7ab57 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -586,7 +586,7 @@ func (r *router) updateState() { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { - if err := r.ipFwdState.ReleaseForwarding(); err != nil { + if err := r.ipFwdState.RequestForwarding(); err != nil { return nil, err } diff --git a/client/firewall/nftables/router_linux.go b/client/firewall/nftables/router_linux.go index 70a2595eec4..6f7ebde5ac3 100644 --- a/client/firewall/nftables/router_linux.go +++ b/client/firewall/nftables/router_linux.go @@ -962,7 +962,7 @@ func (r *router) refreshRulesMap() error { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { - if err := r.ipFwdState.ReleaseForwarding(); err != nil { + if err := r.ipFwdState.RequestForwarding(); err != nil { return nil, err } diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go index b7cd779541a..efb012d0e2f 100644 --- a/client/internal/routemanager/ipfwdstate/ipfwdstate.go +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -2,6 +2,9 @@ package ipfwdstate import ( "fmt" + + log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/client/internal/routemanager/systemops" ) @@ -23,6 +26,7 @@ func (f *IPForwardingState) RequestForwarding() error { return fmt.Errorf("failed to enable IP forwarding with sysctl: %w", err) } f.enabledCounter = 1 + log.Info("IP forwarding enabled") return nil } @@ -43,5 +47,7 @@ func (f *IPForwardingState) ReleaseForwarding() error { if err := systemops.DisableIPForwarding(); err != nil { return fmt.Errorf("failed to disable IP forwarding with sysctl: %w", err) } + log.Info("IP forwarding disabled") + return nil } From cf48c66b0bc787fafd9a8629e091e8d15daa36bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Mon, 3 Feb 2025 11:25:51 +0100 Subject: [PATCH 4/4] Add comment --- client/internal/routemanager/ipfwdstate/ipfwdstate.go | 8 +++----- client/internal/routemanager/systemops/systemops_linux.go | 5 ----- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go index efb012d0e2f..da81c18f9cd 100644 --- a/client/internal/routemanager/ipfwdstate/ipfwdstate.go +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -8,6 +8,8 @@ import ( "github.com/netbirdio/netbird/client/internal/routemanager/systemops" ) +// IPForwardingState is a struct that keeps track of the IP forwarding state. +// todo: read initial state of the IP forwarding from the system and reset the state based on it type IPForwardingState struct { enabledCounter int } @@ -44,10 +46,6 @@ func (f *IPForwardingState) ReleaseForwarding() error { // if failed to disable IP forwarding we anyway decrement the counter f.enabledCounter = 0 - if err := systemops.DisableIPForwarding(); err != nil { - return fmt.Errorf("failed to disable IP forwarding with sysctl: %w", err) - } - log.Info("IP forwarding disabled") - + // todo call systemops.DisableIPForwarding() return nil } diff --git a/client/internal/routemanager/systemops/systemops_linux.go b/client/internal/routemanager/systemops/systemops_linux.go index 3736819d191..1da92cc8059 100644 --- a/client/internal/routemanager/systemops/systemops_linux.go +++ b/client/internal/routemanager/systemops/systemops_linux.go @@ -375,11 +375,6 @@ func EnableIPForwarding() error { return err } -func DisableIPForwarding() error { - _, err := sysctl.Set(ipv4ForwardingPath, 0, false) - return err -} - // entryExists checks if the specified ID or name already exists in the rt_tables file // and verifies if existing names start with "netbird_". func entryExists(file *os.File, id int) (bool, error) {