Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions client/cmd/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@ package cmd

// Flag constants for system configuration
const (
disableClientRoutesFlag = "disable-client-routes"
disableServerRoutesFlag = "disable-server-routes"
disableDNSFlag = "disable-dns"
disableFirewallFlag = "disable-firewall"
blockLANAccessFlag = "block-lan-access"
blockInboundFlag = "block-inbound"
disableClientRoutesFlag = "disable-client-routes"
disableServerRoutesFlag = "disable-server-routes"
disableDefaultRouteFlag = "disable-default-route"
disableDNSFlag = "disable-dns"
disableFirewallFlag = "disable-firewall"
blockLANAccessFlag = "block-lan-access"
blockInboundFlag = "block-inbound"
)

var (
disableClientRoutes bool
disableServerRoutes bool
disableDefaultRoute bool
disableDNS bool
disableFirewall bool
blockLANAccess bool
Expand All @@ -27,6 +29,9 @@ func init() {
upCmd.PersistentFlags().BoolVar(&disableServerRoutes, disableServerRoutesFlag, false,
"Disable server routes. If enabled, the client won't act as a router for server routes received from the management service.")

upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
"Disable adding default route (0.0.0.0/0) to the system routing table while keeping it in WireGuard allowed IPs.")

Comment on lines +32 to +34
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Help text should mention IPv6 default route too.
The behavior skips both IPv4 and IPv6 defaults, but the flag description only cites 0.0.0.0/0. Consider clarifying to avoid user confusion.

Suggested text update
- upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
-     "Disable adding default route (0.0.0.0/0) to the system routing table while keeping it in WireGuard allowed IPs.")
+ upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
+     "Disable adding default routes (0.0.0.0/0, ::/0) to the system routing table while keeping them in WireGuard allowed IPs.")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
"Disable adding default route (0.0.0.0/0) to the system routing table while keeping it in WireGuard allowed IPs.")
upCmd.PersistentFlags().BoolVar(&disableDefaultRoute, disableDefaultRouteFlag, false,
"Disable adding default routes (0.0.0.0/0, ::/0) to the system routing table while keeping them in WireGuard allowed IPs.")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@client/cmd/system.go` around lines 32 - 34, Update the flag help text for
upCmd.PersistentFlags().BoolVar that uses disableDefaultRoute and
disableDefaultRouteFlag to explicitly mention both IPv4 and IPv6 default routes
(e.g. "Disable adding the IPv4 and IPv6 default routes (0.0.0.0/0 and ::/0) to
the system routing table while keeping them in WireGuard allowed IPs.") so users
understand the flag affects both families.

upCmd.PersistentFlags().BoolVar(&disableDNS, disableDNSFlag, false,
"Disable DNS. If enabled, the client won't configure DNS settings.")

Expand Down
10 changes: 10 additions & 0 deletions client/cmd/up.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,10 @@ func setupSetConfigReq(customDNSAddressConverted []byte, cmd *cobra.Command, pro
req.DisableServerRoutes = &disableServerRoutes
}

if cmd.Flag(disableDefaultRouteFlag).Changed {
req.DisableDefaultRoute = &disableDefaultRoute
}

if cmd.Flag(disableDNSFlag).Changed {
req.DisableDns = &disableDNS
}
Expand Down Expand Up @@ -532,6 +536,9 @@ func setupConfig(customDNSAddressConverted []byte, cmd *cobra.Command, configFil
if cmd.Flag(disableServerRoutesFlag).Changed {
ic.DisableServerRoutes = &disableServerRoutes
}
if cmd.Flag(disableDefaultRouteFlag).Changed {
ic.DisableDefaultRoute = &disableDefaultRoute
}
if cmd.Flag(disableDNSFlag).Changed {
ic.DisableDNS = &disableDNS
}
Expand Down Expand Up @@ -646,6 +653,9 @@ func setupLoginRequest(providedSetupKey string, customDNSAddressConverted []byte
if cmd.Flag(disableServerRoutesFlag).Changed {
loginRequest.DisableServerRoutes = &disableServerRoutes
}
if cmd.Flag(disableDefaultRouteFlag).Changed {
loginRequest.DisableDefaultRoute = &disableDefaultRoute
}
if cmd.Flag(disableDNSFlag).Changed {
loginRequest.DisableDns = &disableDNS
}
Expand Down
1 change: 1 addition & 0 deletions client/internal/connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ func createEngineConfig(key wgtypes.Key, config *profilemanager.Config, peerConf

DisableClientRoutes: config.DisableClientRoutes,
DisableServerRoutes: config.DisableServerRoutes || config.BlockInbound,
DisableDefaultRoute: config.DisableDefaultRoute,
DisableDNS: config.DisableDNS,
DisableFirewall: config.DisableFirewall,
BlockLANAccess: config.BlockLANAccess,
Expand Down
2 changes: 2 additions & 0 deletions client/internal/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ type EngineConfig struct {

DisableClientRoutes bool
DisableServerRoutes bool
DisableDefaultRoute bool
DisableDNS bool
DisableFirewall bool
BlockLANAccess bool
Expand Down Expand Up @@ -480,6 +481,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL)
PeerStore: e.peerStore,
DisableClientRoutes: e.config.DisableClientRoutes,
DisableServerRoutes: e.config.DisableServerRoutes,
DisableDefaultRoute: e.config.DisableDefaultRoute,
})
if err := e.routeManager.Init(); err != nil {
log.Errorf("Failed to initialize route manager: %s", err)
Expand Down
12 changes: 12 additions & 0 deletions client/internal/profilemanager/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ type ConfigInput struct {

DisableClientRoutes *bool
DisableServerRoutes *bool
DisableDefaultRoute *bool
DisableDNS *bool
DisableFirewall *bool
BlockLANAccess *bool
Expand Down Expand Up @@ -111,6 +112,7 @@ type Config struct {

DisableClientRoutes bool
DisableServerRoutes bool
DisableDefaultRoute bool
DisableDNS bool
DisableFirewall bool
BlockLANAccess bool
Expand Down Expand Up @@ -484,6 +486,16 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
updated = true
}

if input.DisableDefaultRoute != nil && *input.DisableDefaultRoute != config.DisableDefaultRoute {
if *input.DisableDefaultRoute {
log.Infof("disabling default route")
} else {
log.Infof("enabling default route")
}
config.DisableDefaultRoute = *input.DisableDefaultRoute
updated = true
}

if input.DisableDNS != nil && *input.DisableDNS != config.DisableDNS {
if *input.DisableDNS {
log.Infof("disabling DNS configuration")
Expand Down
10 changes: 10 additions & 0 deletions client/internal/routemanager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ type ManagerConfig struct {
PeerStore *peerstore.Store
DisableClientRoutes bool
DisableServerRoutes bool
DisableDefaultRoute bool
}

// DefaultManager is the default instance of a route manager
Expand Down Expand Up @@ -103,6 +104,7 @@ type DefaultManager struct {
useNewDNSRoute bool
disableClientRoutes bool
disableServerRoutes bool
disableDefaultRoute bool
activeRoutes map[route.HAUniqueID]client.RouteHandler
fakeIPManager *fakeip.Manager
dnsForwarderPort atomic.Uint32
Expand Down Expand Up @@ -133,6 +135,7 @@ func NewManager(config ManagerConfig) *DefaultManager {
peerStore: config.PeerStore,
disableClientRoutes: config.DisableClientRoutes,
disableServerRoutes: config.DisableServerRoutes,
disableDefaultRoute: config.DisableDefaultRoute,
activeRoutes: make(map[route.HAUniqueID]client.RouteHandler),
}
dm.dnsForwarderPort.Store(uint32(nbdns.ForwarderClientPort))
Expand Down Expand Up @@ -184,9 +187,16 @@ func (m *DefaultManager) setupRefCounters(useNoop bool) {

m.routeRefCounter = refcounter.New(
func(prefix netip.Prefix, _ struct{}) (struct{}, error) {
if m.disableDefaultRoute && (prefix == vars.Defaultv4 || prefix == vars.Defaultv6) {
log.Infof("Default route %s is disabled by user, skipping system route", prefix)
return struct{}{}, refcounter.ErrIgnore
}
return struct{}{}, m.sysOps.AddVPNRoute(prefix, toInterface())
},
func(prefix netip.Prefix, _ struct{}) error {
if m.disableDefaultRoute && (prefix == vars.Defaultv4 || prefix == vars.Defaultv6) {
return nil
}
return m.sysOps.RemoveVPNRoute(prefix, toInterface())
},
)
Expand Down
92 changes: 88 additions & 4 deletions client/internal/routemanager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/netip"
"testing"

"github.com/netbirdio/netbird/client/internal/routemanager/vars"
"github.com/netbirdio/netbird/client/internal/stdnet"
"golang.zx2c4.com/wireguard/wgctrl/wgtypes"

Expand All @@ -32,6 +33,7 @@ func TestManagerUpdateRoutes(t *testing.T) {
inputRoutes []*route.Route
inputSerial uint64
removeSrvRouter bool
disableDefaultRoute bool
serverRoutesExpected int
clientNetworkWatchersExpected int
clientNetworkWatchersExpectedAllowed int
Expand Down Expand Up @@ -205,6 +207,36 @@ func TestManagerUpdateRoutes(t *testing.T) {
clientNetworkWatchersExpected: 0,
clientNetworkWatchersExpectedAllowed: 1,
},
{
name: "Default Route With DisableDefaultRoute Still Creates Watcher",
disableDefaultRoute: true,
inputRoutes: []*route.Route{
{
ID: "a",
NetID: "routeA",
Peer: remotePeerKey1,
Network: netip.MustParsePrefix("0.0.0.0/0"),
NetworkType: route.IPv4Network,
Metric: 9999,
Masquerade: false,
Enabled: true,
SkipAutoApply: false,
},
{
ID: "b",
NetID: "routeB",
Peer: remotePeerKey1,
Network: netip.MustParsePrefix("10.0.0.0/8"),
NetworkType: route.IPv4Network,
Metric: 9999,
Masquerade: false,
Enabled: true,
},
},
inputSerial: 1,
clientNetworkWatchersExpected: 1,
clientNetworkWatchersExpectedAllowed: 2,
},
{
name: "Remove 1 Client Route",
inputInitRoutes: []*route.Route{
Expand Down Expand Up @@ -425,10 +457,11 @@ func TestManagerUpdateRoutes(t *testing.T) {
statusRecorder := peer.NewRecorder("https://mgm")
ctx := context.TODO()
routeManager := NewManager(ManagerConfig{
Context: ctx,
PublicKey: localPeerKey,
WGInterface: wgInterface,
StatusRecorder: statusRecorder,
Context: ctx,
PublicKey: localPeerKey,
WGInterface: wgInterface,
StatusRecorder: statusRecorder,
DisableDefaultRoute: testCase.disableDefaultRoute,
})

err = routeManager.Init()
Expand Down Expand Up @@ -462,3 +495,54 @@ func TestManagerUpdateRoutes(t *testing.T) {
})
}
}

func TestDisableDefaultRouteSkipsSystemRoute(t *testing.T) {
peerPrivateKey, _ := wgtypes.GeneratePrivateKey()
newNet, err := stdnet.NewNet(context.Background(), nil)
require.NoError(t, err)

opts := iface.WGIFaceOpts{
IFaceName: "utun4399",
Address: "100.65.65.2/24",
WGPort: 33100,
WGPrivKey: peerPrivateKey.String(),
MTU: iface.DefaultMTU,
TransportNet: newNet,
}
wgInterface, err := iface.NewWGIFace(opts)
require.NoError(t, err)
defer wgInterface.Close()

require.NoError(t, wgInterface.Create())

statusRecorder := peer.NewRecorder("https://mgm")

mgr := NewManager(ManagerConfig{
Context: context.TODO(),
PublicKey: localPeerKey,
WGInterface: wgInterface,
StatusRecorder: statusRecorder,
DisableDefaultRoute: true,
})
require.NoError(t, mgr.Init())
defer mgr.Stop(nil)

// Increment the route ref counter for the default v4 prefix.
// With DisableDefaultRoute=true, this should be ignored (ErrIgnore path).
_, err = mgr.routeRefCounter.Increment(vars.Defaultv4, struct{}{})
require.NoError(t, err, "increment should not return error for ignored default route")

// The key should NOT be tracked in the ref counter because ErrIgnore skips it.
_, ok := mgr.routeRefCounter.Get(vars.Defaultv4)
require.False(t, ok, "default v4 route should not be in route ref counter when DisableDefaultRoute is true")

// A non-default prefix should still be tracked normally.
normalPrefix := netip.MustParsePrefix("10.0.0.0/8")
_, err = mgr.routeRefCounter.Increment(normalPrefix, struct{}{})
// AddVPNRoute may fail without proper routing setup, but the ref counter should still track it.
// We only care that non-default routes are NOT ignored.
ref, ok := mgr.routeRefCounter.Get(normalPrefix)
if ok {
require.Equal(t, 1, ref.Count, "non-default route should have ref count 1")
}
}
Loading