From 816d80602ffc024afb670dfe18766092701d2639 Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Wed, 8 Jul 2026 10:15:31 +0200 Subject: [PATCH 01/19] [client] Update gopsutil to v4 (#6688) --- client/system/process.go | 2 +- client/system/process_test.go | 2 +- go.mod | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/system/process.go b/client/system/process.go index 07f69a212..fefa7d913 100644 --- a/client/system/process.go +++ b/client/system/process.go @@ -7,7 +7,7 @@ import ( "os" "slices" - "github.com/shirou/gopsutil/v3/process" + "github.com/shirou/gopsutil/v4/process" ) // getRunningProcesses returns a list of running process paths. The context bounds the work: diff --git a/client/system/process_test.go b/client/system/process_test.go index 44a1c8ba0..9d0a6b935 100644 --- a/client/system/process_test.go +++ b/client/system/process_test.go @@ -4,7 +4,7 @@ import ( "context" "testing" - "github.com/shirou/gopsutil/v3/process" + "github.com/shirou/gopsutil/v4/process" ) func Benchmark_getRunningProcesses(b *testing.B) { diff --git a/go.mod b/go.mod index e1c762607..d57c7b495 100644 --- a/go.mod +++ b/go.mod @@ -104,6 +104,7 @@ require ( github.com/redis/go-redis/v9 v9.7.3 github.com/rs/xid v1.3.0 github.com/shirou/gopsutil/v3 v3.24.4 + github.com/shirou/gopsutil/v4 v4.25.8 github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966 github.com/songgao/water v0.0.0-20200317203138-2b4b6d7c09d8 github.com/stretchr/testify v1.11.1 @@ -308,7 +309,6 @@ require ( github.com/russellhaering/goxmldsig v1.6.0 // indirect github.com/ryanuber/go-glob v1.0.0 // indirect github.com/rymdport/portal v0.4.2 // indirect - github.com/shirou/gopsutil/v4 v4.25.8 // indirect github.com/shoenig/go-m1cpu v0.2.1 // indirect github.com/shopspring/decimal v1.4.0 // indirect github.com/spf13/cast v1.7.0 // indirect From 7cd5c1732bb5374f21005073937c42f4d531e3c5 Mon Sep 17 00:00:00 2001 From: Theodor Midtlien Date: Wed, 8 Jul 2026 14:36:42 +0200 Subject: [PATCH 02/19] [client] Fix hanging status command during relay dial (#6694) * Add regression test for relay state lock * Make connect not hold a lock in openConnVia --- shared/relay/client/manager.go | 66 +++++++++----- shared/relay/client/manager_cleanup_test.go | 60 ++++++++++++ .../relay/client/manager_relaystates_test.go | 91 +++++++++++++++++++ 3 files changed, 196 insertions(+), 21 deletions(-) create mode 100644 shared/relay/client/manager_cleanup_test.go create mode 100644 shared/relay/client/manager_relaystates_test.go diff --git a/shared/relay/client/manager.go b/shared/relay/client/manager.go index e1515401e..2f2839d94 100644 --- a/shared/relay/client/manager.go +++ b/shared/relay/client/manager.go @@ -30,11 +30,16 @@ type RelayTrack struct { relayClient *Client err error created time.Time + // ready is closed once the dial started by openConnVia finishes (relayClient + // or err is set). Callers reusing a track wait on this instead of the track + // lock, so the dial never runs under rt.Lock. + ready chan struct{} } func NewRelayTrack() *RelayTrack { return &RelayTrack{ created: time.Now(), + ready: make(chan struct{}), } } @@ -326,34 +331,24 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string // check if already has a connection to the desired relay server m.relayClientsMutex.RLock() rt, ok := m.relayClients[serverAddress] - if ok { - rt.RLock() - m.relayClientsMutex.RUnlock() - defer rt.RUnlock() - if rt.err != nil { - return nil, rt.err - } - return rt.relayClient.OpenConn(ctx, peerKey) - } m.relayClientsMutex.RUnlock() + if ok { + return m.openConnOnTrack(ctx, rt, peerKey) + } // if not, establish a new connection but check it again (because changed the lock type) before starting the // connection m.relayClientsMutex.Lock() rt, ok = m.relayClients[serverAddress] if ok { - rt.RLock() m.relayClientsMutex.Unlock() - defer rt.RUnlock() - if rt.err != nil { - return nil, rt.err - } - return rt.relayClient.OpenConn(ctx, peerKey) + return m.openConnOnTrack(ctx, rt, peerKey) } - // create a new relay client and store it in the relayClients map + // Publish the track and release the map lock BEFORE dialing, so the dial does + // not run under rt.Lock (which would block RelayStates and the cleanup loop + // for the full dial). Concurrent callers find this track and wait on rt.ready. rt = NewRelayTrack() - rt.Lock() m.relayClients[serverAddress] = rt m.relayClientsMutex.Unlock() @@ -361,8 +356,10 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string relayClient.SetTransportFallback(m.transportFallback) err := relayClient.Connect(m.ctx) if err != nil { + rt.Lock() rt.err = err rt.Unlock() + close(rt.ready) m.relayClientsMutex.Lock() delete(m.relayClients, serverAddress) m.relayClientsMutex.Unlock() @@ -370,14 +367,34 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string } // if connection closed then delete the relay client from the list relayClient.SetOnDisconnectListener(m.onServerDisconnected) + rt.Lock() rt.relayClient = relayClient rt.Unlock() + close(rt.ready) - conn, err := relayClient.OpenConn(ctx, peerKey) - if err != nil { - return nil, err + return relayClient.OpenConn(ctx, peerKey) +} + +// openConnOnTrack opens a peer connection through an existing relay track, +// waiting for the dial started by another openConnVia call to finish. It waits +// on rt.ready rather than the track lock, so it neither holds nor contends the +// track lock across the dial. +func (m *Manager) openConnOnTrack(ctx context.Context, rt *RelayTrack, peerKey string) (net.Conn, error) { + select { + case <-rt.ready: + case <-ctx.Done(): + return nil, ctx.Err() } - return conn, nil + + rt.RLock() + defer rt.RUnlock() + if rt.err != nil { + return nil, rt.err + } + if rt.relayClient == nil { + return nil, ErrRelayClientNotConnected + } + return rt.relayClient.OpenConn(ctx, peerKey) } func (m *Manager) onServerConnected() { @@ -476,6 +493,13 @@ func (m *Manager) cleanUpUnusedRelays() { continue } + // dial still in progress (openConnVia publishes the track before Connect + // completes and no longer holds rt.Lock during it), nothing to clean up. + if rt.relayClient == nil { + rt.Unlock() + continue + } + if time.Since(rt.created) <= m.keepUnusedServerTime { rt.Unlock() continue diff --git a/shared/relay/client/manager_cleanup_test.go b/shared/relay/client/manager_cleanup_test.go new file mode 100644 index 000000000..6ac5daeac --- /dev/null +++ b/shared/relay/client/manager_cleanup_test.go @@ -0,0 +1,60 @@ +package client + +import ( + "context" + "net/netip" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// TestCleanUpUnusedRelays_DoesNotBlockOnRealHangingDial drives a real, hanging foreign +// relay dial and asserts cleanUpUnusedRelays does not stall behind it. +func TestCleanUpUnusedRelays_DoesNotBlockOnRealHangingDial(t *testing.T) { + serverAddr := stallingRelayListener(t) + + mCtx, mCancel := context.WithCancel(context.Background()) + t.Cleanup(mCancel) + + m := NewManager(mCtx, nil, "alice", 1280) + + dialDone := make(chan struct{}) + go func() { + defer close(dialDone) + _, _ = m.openConnVia(mCtx, serverAddr, "peerKey", netip.Addr{}) + }() + + // The track appears in the map once the dial is in flight. + require.Eventually(t, func() bool { + m.relayClientsMutex.RLock() + defer m.relayClientsMutex.RUnlock() + _, ok := m.relayClients[serverAddr] + return ok + }, 5*time.Second, 5*time.Millisecond, "relay dial did not start") + + cleanupDone := make(chan struct{}) + go func() { + defer close(cleanupDone) + m.cleanUpUnusedRelays() + }() + + select { + case <-cleanupDone: + case <-time.After(2 * time.Second): + t.Fatal("cleanUpUnusedRelays blocked on an in-progress relay dial while holding the relay map lock") + } + + m.relayClientsMutex.RLock() + _, stillTracked := m.relayClients[serverAddr] + m.relayClientsMutex.RUnlock() + require.True(t, stillTracked, "an in-progress relay dial must not be evicted by cleanup") + + // Release the hanging dial so the goroutine can exit cleanly. + mCancel() + select { + case <-dialDone: + case <-time.After(5 * time.Second): + t.Fatal("openConnVia did not return after context cancellation") + } +} diff --git a/shared/relay/client/manager_relaystates_test.go b/shared/relay/client/manager_relaystates_test.go new file mode 100644 index 000000000..f26323323 --- /dev/null +++ b/shared/relay/client/manager_relaystates_test.go @@ -0,0 +1,91 @@ +package client + +import ( + "context" + "net" + "net/netip" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// stallingRelayListener accepts TCP connections and holds them open without ever +// responding, so a relay handshake dialed against it blocks until its context is +// cancelled. It returns the "rel://host:port" URL to dial. +func stallingRelayListener(t *testing.T) string { + t.Helper() + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + var mu sync.Mutex + var conns []net.Conn + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + mu.Lock() + conns = append(conns, c) + mu.Unlock() + } + }() + t.Cleanup(func() { + _ = ln.Close() + mu.Lock() + for _, c := range conns { + _ = c.Close() + } + mu.Unlock() + }) + + return "rel://" + ln.Addr().String() +} + +// TestRelayStates_DoesNotBlockOnRealHangingDial is a regression test for +// RelayStates() called by a "status -d command" hanging behind an in-progress +// relay dial. +func TestRelayStates_DoesNotBlockOnRealHangingDial(t *testing.T) { + serverAddr := stallingRelayListener(t) + + mCtx, mCancel := context.WithCancel(context.Background()) + t.Cleanup(mCancel) + + m := NewManager(mCtx, nil, "alice", 1280) + + dialDone := make(chan struct{}) + go func() { + defer close(dialDone) + _, _ = m.openConnVia(mCtx, serverAddr, "peerKey", netip.Addr{}) + }() + + require.Eventually(t, func() bool { + m.relayClientsMutex.RLock() + defer m.relayClientsMutex.RUnlock() + _, ok := m.relayClients[serverAddr] + return ok + }, 5*time.Second, 5*time.Millisecond, "relay dial did not start") + + done := make(chan []RelayConnState, 1) + go func() { + done <- m.RelayStates() + }() + + select { + case states := <-done: + require.Empty(t, states, "a relay still being dialed carries no state and must be omitted") + case <-time.After(2 * time.Second): + t.Fatal("RelayStates blocked on a foreign relay whose Connect() is in progress") + } + + // Release the hanging dial so the goroutine can exit cleanly. + mCancel() + select { + case <-dialDone: + case <-time.After(5 * time.Second): + t.Fatal("openConnVia did not return after context cancellation") + } +} From 08e46aa62f3da6a4516dac68287084b736790214 Mon Sep 17 00:00:00 2001 From: blaugrau90 <61945343+blaugrau90@users.noreply.github.com> Date: Fri, 10 Jul 2026 12:20:57 +0200 Subject: [PATCH 03/19] [management] fix: prevent reverse proxy domain from being pushed as DNS search domain (#6498) SynthesizePrivateServiceZones created CustomZones for private services without setting SearchDomainDisabled, causing the reverse proxy domain to be injected as a search domain suffix on all connected peers. This broke local hostname resolution: short names like 'myserver' were expanded to 'myserver.app.example.com' (matching the reverse proxy domain) before local DNS search domains were tried. Fix: set SearchDomainDisabled: true so the zone is registered as a match-only supplemental resolver, consistent with the NonAuthoritative intent already expressed on the same zone. --- management/server/types/account.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/management/server/types/account.go b/management/server/types/account.go index 7a0a0054f..6be865a43 100644 --- a/management/server/types/account.go +++ b/management/server/types/account.go @@ -305,7 +305,8 @@ func (a *Account) SynthesizePrivateServiceZones(peerID string) []nbdns.CustomZon zone = &nbdns.CustomZone{ Domain: dns.Fqdn(serviceDomainZone), Records: []nbdns.SimpleRecord{}, - NonAuthoritative: true, + NonAuthoritative: true, + SearchDomainDisabled: true, } zonesByApex[serviceDomainZone] = zone } From 4d4cc551fdd0b8824ae4caf44af6586d43c044df Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:38:29 +0900 Subject: [PATCH 04/19] [client] Recover from rosenpass key desync (#6714) --- client/internal/engine.go | 2 +- client/internal/peer/conn.go | 42 ++- client/internal/peer/conn_test.go | 82 ++++++ client/internal/peer/wg_watcher.go | 14 +- client/internal/peer/wg_watcher_test.go | 72 ++++- client/internal/rosenpass/manager.go | 11 +- client/internal/rosenpass/manager_test.go | 26 +- client/internal/rosenpass/netbird_handler.go | 166 +++++++++--- .../rosenpass/netbird_handler_test.go | 250 ++++++++++++++++++ client/internal/rosenpass/seed.go | 17 ++ 10 files changed, 618 insertions(+), 64 deletions(-) create mode 100644 client/internal/rosenpass/netbird_handler_test.go diff --git a/client/internal/engine.go b/client/internal/engine.go index a08bea31b..5f2c43ab4 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -522,7 +522,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL) } else { log.Infof("running rosenpass in strict mode") } - e.rpManager, err = rosenpass.NewManager(e.config.PreSharedKey, e.config.WgIfaceName) + e.rpManager, err = rosenpass.NewManager(e.config.PreSharedKey, e.config.WgIfaceName, publicKey) if err != nil { return fmt.Errorf("create rosenpass manager: %w", err) } diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index fb468696f..f0625c853 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -30,6 +30,11 @@ import ( relayClient "github.com/netbirdio/netbird/shared/relay/client" ) +// wgTimeoutEscalationThreshold is the number of consecutive WireGuard +// handshake timeouts after which the rosenpass state for the peer is +// considered desynced and gets reset. +const wgTimeoutEscalationThreshold = 3 + // MetricsRecorder is an interface for recording peer connection metrics type MetricsRecorder interface { RecordConnectionStages( @@ -118,6 +123,9 @@ type Conn struct { wgWatcher *WGWatcher wgWatcherWg sync.WaitGroup wgWatcherCancel context.CancelFunc + // wgTimeouts counts consecutive WireGuard handshake timeouts without a + // successful handshake in between. Guarded by mu. + wgTimeouts int // used to store the remote Rosenpass key for Relayed connection in case of connection update from ice rosenpassRemoteKey []byte @@ -683,6 +691,29 @@ func (conn *Conn) onWGDisconnected() { default: conn.Log.Debugf("No active connection to close on WG timeout") } + + conn.escalateWGTimeoutLocked() +} + +// escalateWGTimeoutLocked resets the peer's rosenpass state after repeated +// handshake timeouts. With rosenpass enabled, persistent timeouts mean the +// preshared keys have desynced; the renewal exchange runs over the dead +// tunnel and cannot resync them. Reporting the peer disconnected drops its +// rosenpass state, so the next connection configuration programs the +// rendezvous key and the tunnel can bootstrap again. Callers must hold mu. +func (conn *Conn) escalateWGTimeoutLocked() { + if conn.config.RosenpassConfig.PubKey == nil { + return + } + + conn.wgTimeouts++ + if conn.wgTimeouts < wgTimeoutEscalationThreshold || conn.onDisconnected == nil { + return + } + conn.wgTimeouts = 0 + + conn.Log.Warnf("%d consecutive WireGuard handshake timeouts, resetting rosenpass state for peer", wgTimeoutEscalationThreshold) + conn.onDisconnected(conn.config.WgConfig.RemoteKey) } func (conn *Conn) updateRelayStatus(relayServerAddr string, rosenpassPubKey []byte, updateTime time.Time) { @@ -812,7 +843,7 @@ func (conn *Conn) enableWgWatcherIfNeeded(enabledTime time.Time) { conn.wgWatcherWg.Add(1) go func() { defer conn.wgWatcherWg.Done() - conn.wgWatcher.EnableWgWatcher(wgWatcherCtx, enabledTime, conn.onWGDisconnected, conn.onWGHandshakeSuccess) + conn.wgWatcher.EnableWgWatcher(wgWatcherCtx, enabledTime, conn.onWGDisconnected, conn.onWGHandshakeSuccess, conn.onWGCheckSuccess) }() } @@ -892,6 +923,15 @@ func (conn *Conn) onWGHandshakeSuccess(when time.Time) { conn.recordConnectionMetrics() } +// onWGCheckSuccess is called for every watcher check that observed a fresh +// handshake, including handshakes of connections that were already up when +// the watcher started. +func (conn *Conn) onWGCheckSuccess() { + conn.mu.Lock() + conn.wgTimeouts = 0 + conn.mu.Unlock() +} + // recordConnectionMetrics records connection stage timestamps as metrics func (conn *Conn) recordConnectionMetrics() { if conn.metricsRecorder == nil { diff --git a/client/internal/peer/conn_test.go b/client/internal/peer/conn_test.go index 59216b647..f2312a66a 100644 --- a/client/internal/peer/conn_test.go +++ b/client/internal/peer/conn_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/netbirdio/netbird/client/iface" @@ -304,3 +305,84 @@ func TestConn_presharedKey_RosenpassManaged(t *testing.T) { t.Fatalf("expected non-nil presharedKey before Rosenpass manages PSK") } } + +func newWGTimeoutTestConn(rosenpassEnabled bool, disconnected *[]string) *Conn { + cfg := ConnConfig{ + Key: "LLHf3Ma6z6mdLbriAJbqhX7+nM/B71lgw2+91q3LfhU=", + LocalKey: "RRHf3Ma6z6mdLbriAJbqhX7+nM/B71lgw2+91q3LfhU=", + WgConfig: WgConfig{RemoteKey: "LLHf3Ma6z6mdLbriAJbqhX7+nM/B71lgw2+91q3LfhU="}, + } + if rosenpassEnabled { + cfg.RosenpassConfig = RosenpassConfig{PubKey: []byte("dummykey")} + } + + conn := &Conn{ + ctx: context.Background(), + config: cfg, + Log: log.WithField("peer", cfg.Key), + metricsStages: &MetricsStages{}, + } + conn.SetOnDisconnected(func(remotePeer string) { + *disconnected = append(*disconnected, remotePeer) + }) + return conn +} + +// TestConn_onWGDisconnected_EscalatesToRosenpassReset: repeated handshake +// timeouts with rosenpass enabled mean the preshared keys have desynced. The +// renewal exchange runs over the dead tunnel and cannot resync them, so after +// wgTimeoutEscalationThreshold consecutive timeouts the conn must report the +// peer disconnected, dropping its rosenpass state so the next configuration +// programs the rendezvous key. +func TestConn_onWGDisconnected_EscalatesToRosenpassReset(t *testing.T) { + var disconnected []string + conn := newWGTimeoutTestConn(true, &disconnected) + + for i := 0; i < wgTimeoutEscalationThreshold-1; i++ { + conn.onWGDisconnected() + } + assert.Empty(t, disconnected, "escalation must not fire below the threshold") + + conn.onWGDisconnected() + assert.Equal(t, []string{conn.config.WgConfig.RemoteKey}, disconnected, + "reaching the threshold must report the peer disconnected once") + + for i := 0; i < wgTimeoutEscalationThreshold-1; i++ { + conn.onWGDisconnected() + } + assert.Len(t, disconnected, 1, "escalation must restart counting after firing") + + conn.onWGDisconnected() + assert.Len(t, disconnected, 2, "continued timeouts must escalate again") +} + +// TestConn_onWGDisconnected_CheckSuccessResetsEscalation: a successful +// handshake between timeouts means the tunnel recovered; the counter must +// start over. +func TestConn_onWGDisconnected_CheckSuccessResetsEscalation(t *testing.T) { + var disconnected []string + conn := newWGTimeoutTestConn(true, &disconnected) + + for i := 0; i < wgTimeoutEscalationThreshold-1; i++ { + conn.onWGDisconnected() + } + conn.onWGCheckSuccess() + + for i := 0; i < wgTimeoutEscalationThreshold-1; i++ { + conn.onWGDisconnected() + } + assert.Empty(t, disconnected, "handshake success must reset the timeout count") +} + +// TestConn_onWGDisconnected_NoEscalationWithoutRosenpass: without rosenpass +// there is no per-peer key state to reset; repeated timeouts must not report +// disconnects. +func TestConn_onWGDisconnected_NoEscalationWithoutRosenpass(t *testing.T) { + var disconnected []string + conn := newWGTimeoutTestConn(false, &disconnected) + + for i := 0; i < wgTimeoutEscalationThreshold*3; i++ { + conn.onWGDisconnected() + } + assert.Empty(t, disconnected, "escalation must be limited to rosenpass connections") +} diff --git a/client/internal/peer/wg_watcher.go b/client/internal/peer/wg_watcher.go index 4fc883d17..10c22153f 100644 --- a/client/internal/peer/wg_watcher.go +++ b/client/internal/peer/wg_watcher.go @@ -71,9 +71,11 @@ func (w *WGWatcher) PrepareInitialHandshake() (ok bool) { // EnableWgWatcher runs the WireGuard watcher loop using the handshake baseline captured by // PrepareInitialHandshake. The watcher runs until ctx is cancelled. Caller is responsible -// for context lifecycle management. -func (w *WGWatcher) EnableWgWatcher(ctx context.Context, enabledTime time.Time, onDisconnectedFn func(), onHandshakeSuccessFn func(when time.Time)) { - w.periodicHandshakeCheck(ctx, onDisconnectedFn, onHandshakeSuccessFn, enabledTime, w.initialHandshake) +// for context lifecycle management. onHandshakeSuccessFn is called only for the first +// handshake observed by this run, onCheckSuccessFn for every check that observed a fresh +// handshake, including the first. +func (w *WGWatcher) EnableWgWatcher(ctx context.Context, enabledTime time.Time, onDisconnectedFn func(), onHandshakeSuccessFn func(when time.Time), onCheckSuccessFn func()) { + w.periodicHandshakeCheck(ctx, onDisconnectedFn, onHandshakeSuccessFn, onCheckSuccessFn, enabledTime, w.initialHandshake) w.muEnabled.Lock() w.enabled = false @@ -90,7 +92,7 @@ func (w *WGWatcher) Reset() { } // wgStateCheck help to check the state of the WireGuard handshake and relay connection -func (w *WGWatcher) periodicHandshakeCheck(ctx context.Context, onDisconnectedFn func(), onHandshakeSuccessFn func(when time.Time), enabledTime time.Time, initialHandshake time.Time) { +func (w *WGWatcher) periodicHandshakeCheck(ctx context.Context, onDisconnectedFn func(), onHandshakeSuccessFn func(when time.Time), onCheckSuccessFn func(), enabledTime time.Time, initialHandshake time.Time) { w.log.Infof("WireGuard watcher started") timer := time.NewTimer(wgHandshakeOvertime) @@ -117,6 +119,10 @@ func (w *WGWatcher) periodicHandshakeCheck(ctx context.Context, onDisconnectedFn } } + if onCheckSuccessFn != nil && ctx.Err() == nil { + onCheckSuccessFn() + } + lastHandshake = *handshake resetTime := time.Until(handshake.Add(checkPeriod)) diff --git a/client/internal/peer/wg_watcher_test.go b/client/internal/peer/wg_watcher_test.go index 634d7974f..80f34f1a1 100644 --- a/client/internal/peer/wg_watcher_test.go +++ b/client/internal/peer/wg_watcher_test.go @@ -24,6 +24,72 @@ func (m *MocWgIface) disconnect() { m.stop = true } +type mockHandshakeStats struct { + mu sync.Mutex + handshake time.Time +} + +func (m *mockHandshakeStats) GetStats() (map[string]configurer.WGStats, error) { + m.mu.Lock() + defer m.mu.Unlock() + return map[string]configurer.WGStats{"": {LastHandshake: m.handshake}}, nil +} + +func (m *mockHandshakeStats) advance() { + m.mu.Lock() + defer m.mu.Unlock() + m.handshake = time.Now() +} + +// TestWGWatcher_CheckSuccessCallback: onCheckSuccessFn must fire for a fresh +// handshake even when the watcher started with an existing handshake baseline, +// the case where onHandshakeSuccessFn stays silent. +func TestWGWatcher_CheckSuccessCallback(t *testing.T) { + // checkPeriod bounds how stale a handshake may be before the watcher treats it + // as a suspended-machine timeout. The first check fires after wgHandshakeOvertime, + // so keep checkPeriod well above any scheduling jitter to avoid a false timeout + // converting the expected success into a disconnect on a loaded runner. + checkPeriod = 1 * time.Minute + wgHandshakeOvertime = 1 * time.Second + + mlog := log.WithField("peer", "tet") + // Use an old baseline so advance() yields a strictly newer handshake even on + // platforms with coarse clock resolution (Windows), where two time.Now() calls + // microseconds apart can return the same instant and read as a timed-out handshake. + stats := &mockHandshakeStats{handshake: time.Now().Add(-time.Hour)} + watcher := NewWGWatcher(mlog, stats, "", newStateDump("peer", mlog, &Status{})) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + require.True(t, watcher.PrepareInitialHandshake()) + + firstHandshake := make(chan struct{}, 1) + checkSuccess := make(chan struct{}, 1) + go watcher.EnableWgWatcher(ctx, time.Now(), func() {}, func(when time.Time) { + firstHandshake <- struct{}{} + }, func() { + select { + case checkSuccess <- struct{}{}: + default: + } + }) + + stats.advance() + + select { + case <-checkSuccess: + case <-time.After(10 * time.Second): + t.Errorf("timeout waiting for check success callback") + } + + select { + case <-firstHandshake: + t.Errorf("first-handshake callback must not fire for a non-zero baseline") + default: + } +} + func TestWGWatcher_EnableWgWatcher(t *testing.T) { checkPeriod = 5 * time.Second wgHandshakeOvertime = 1 * time.Second @@ -44,7 +110,7 @@ func TestWGWatcher_EnableWgWatcher(t *testing.T) { onDisconnected <- struct{}{} }, func(when time.Time) { mlog.Infof("onHandshakeSuccess: %v", when) - }) + }, nil) // wait for initial reading time.Sleep(2 * time.Second) @@ -73,7 +139,7 @@ func TestWGWatcher_ReEnable(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - watcher.EnableWgWatcher(ctx, time.Now(), func() {}, func(when time.Time) {}) + watcher.EnableWgWatcher(ctx, time.Now(), func() {}, func(when time.Time) {}, nil) }() cancel() @@ -89,7 +155,7 @@ func TestWGWatcher_ReEnable(t *testing.T) { onDisconnected := make(chan struct{}, 1) go watcher.EnableWgWatcher(ctx, time.Now(), func() { onDisconnected <- struct{}{} - }, func(when time.Time) {}) + }, func(when time.Time) {}, nil) time.Sleep(2 * time.Second) mocWgIface.disconnect() diff --git a/client/internal/rosenpass/manager.go b/client/internal/rosenpass/manager.go index 903753753..21dd751df 100644 --- a/client/internal/rosenpass/manager.go +++ b/client/internal/rosenpass/manager.go @@ -39,6 +39,7 @@ type rpServer interface { type Manager struct { ifaceName string + localWgKey wgtypes.Key spk []byte ssk []byte rpKeyHash string @@ -51,8 +52,9 @@ type Manager struct { wgIface PresharedKeySetter } -// NewManager creates a new Rosenpass manager -func NewManager(preSharedKey *wgtypes.Key, wgIfaceName string) (*Manager, error) { +// NewManager creates a new Rosenpass manager. localWgKey is the local +// WireGuard public key, used to derive the per-peer rendezvous key. +func NewManager(preSharedKey *wgtypes.Key, wgIfaceName string, localWgKey wgtypes.Key) (*Manager, error) { public, secret, err := rp.GenerateKeyPair() if err != nil { return nil, err @@ -62,6 +64,7 @@ func NewManager(preSharedKey *wgtypes.Key, wgIfaceName string) (*Manager, error) log.Tracef("generated new rosenpass key pair with public key %s", rpKeyHash) return &Manager{ ifaceName: wgIfaceName, + localWgKey: localWgKey, rpKeyHash: rpKeyHash, spk: public, ssk: secret, @@ -73,7 +76,7 @@ func NewManager(preSharedKey *wgtypes.Key, wgIfaceName string) (*Manager, error) // nil receiver in addPeer -> m.rpWgHandler.AddPeer. generateConfig will // replace it with a fresh handler on each Run() to clear stale peer // state from previous engine sessions. - rpWgHandler: NewNetbirdHandler(), + rpWgHandler: NewNetbirdHandler((*[32]byte)(preSharedKey), localWgKey), lock: sync.Mutex{}, }, nil } @@ -161,7 +164,7 @@ func (m *Manager) generateConfig() (rp.Config, error) { cfg.Peers = []rp.PeerConfig{} m.lock.Lock() - m.rpWgHandler = NewNetbirdHandler() + m.rpWgHandler = NewNetbirdHandler(m.preSharedKey, m.localWgKey) if m.wgIface != nil { m.rpWgHandler.SetInterface(m.wgIface) } diff --git a/client/internal/rosenpass/manager_test.go b/client/internal/rosenpass/manager_test.go index d74960d0d..69e18ac88 100644 --- a/client/internal/rosenpass/manager_test.go +++ b/client/internal/rosenpass/manager_test.go @@ -85,7 +85,7 @@ func newTestManager(spkFirstByte byte, mock *mockServer) *Manager { ssk: make([]byte, 32), rpKeyHash: "test-hash", rpPeerIDs: make(map[string]*rp.PeerID), - rpWgHandler: NewNetbirdHandler(), + rpWgHandler: NewNetbirdHandler(nil, wgtypes.Key{0x01}), server: mock, } } @@ -255,7 +255,7 @@ func TestAddPeer_NilServer_ReturnsErrorNoCrash(t *testing.T) { // issue #4341 cannot occur in the window between NewManager and Run(). func TestNewManager_PreInitializesHandler(t *testing.T) { psk := wgtypes.Key{} - m, err := NewManager(&psk, "wt0") + m, err := NewManager(&psk, "wt0", wgtypes.Key{0x01}) require.NoError(t, err) require.NotNil(t, m.rpWgHandler, "rpWgHandler must be initialized in NewManager") } @@ -329,10 +329,10 @@ func TestIsPresharedKeyInitialized_AddedButNotHandshaken_ReturnsFalse(t *testing require.False(t, m.IsPresharedKeyInitialized(wgKey)) } -// --- NetbirdHandler.outputKey ---------------------------------------------- +// --- NetbirdHandler.applyKey ---------------------------------------------- -func TestHandler_OutputKey_FirstCallUsesUpdateOnlyFalse(t *testing.T) { - h := NewNetbirdHandler() +func TestHandler_ApplyKey_FirstCallUsesUpdateOnlyFalse(t *testing.T) { + h := NewNetbirdHandler(nil, wgtypes.Key{0x01}) iface := &mockIface{} h.SetInterface(iface) @@ -348,8 +348,8 @@ func TestHandler_OutputKey_FirstCallUsesUpdateOnlyFalse(t *testing.T) { require.Equal(t, wgKey.String(), iface.calls[0].peerKey) } -func TestHandler_OutputKey_SubsequentCallsUseUpdateOnlyTrue(t *testing.T) { - h := NewNetbirdHandler() +func TestHandler_ApplyKey_SubsequentCallsUseUpdateOnlyTrue(t *testing.T) { + h := NewNetbirdHandler(nil, wgtypes.Key{0x01}) iface := &mockIface{} h.SetInterface(iface) @@ -364,8 +364,8 @@ func TestHandler_OutputKey_SubsequentCallsUseUpdateOnlyTrue(t *testing.T) { require.True(t, iface.calls[1].updateOnly, "subsequent rotations must use updateOnly=true") } -func TestHandler_OutputKey_NilInterface_NoCrashNoCall(t *testing.T) { - h := NewNetbirdHandler() +func TestHandler_ApplyKey_NilInterface_NoCrashNoCall(t *testing.T) { + h := NewNetbirdHandler(nil, wgtypes.Key{0x01}) // no SetInterface — iface remains nil pid := rp.PeerID{0x03} h.AddPeer(pid, "wt0", rp.Key(wgtypes.Key{})) @@ -374,8 +374,8 @@ func TestHandler_OutputKey_NilInterface_NoCrashNoCall(t *testing.T) { h.HandshakeCompleted(pid, rp.Key{}) } -func TestHandler_OutputKey_UnknownPeer_NoCall(t *testing.T) { - h := NewNetbirdHandler() +func TestHandler_ApplyKey_UnknownPeer_NoCall(t *testing.T) { + h := NewNetbirdHandler(nil, wgtypes.Key{0x01}) iface := &mockIface{} h.SetInterface(iface) @@ -384,7 +384,7 @@ func TestHandler_OutputKey_UnknownPeer_NoCall(t *testing.T) { } func TestHandler_RemovePeer_ClearsInitializedState(t *testing.T) { - h := NewNetbirdHandler() + h := NewNetbirdHandler(nil, wgtypes.Key{0x01}) iface := &mockIface{} h.SetInterface(iface) @@ -398,7 +398,7 @@ func TestHandler_RemovePeer_ClearsInitializedState(t *testing.T) { } func TestHandler_SetInterfaceAfterAddPeer_StillReceivesKey(t *testing.T) { - h := NewNetbirdHandler() + h := NewNetbirdHandler(nil, wgtypes.Key{0x01}) pid := rp.PeerID{0x05} wgKey := wgtypes.Key{0xEE} h.AddPeer(pid, "wt0", rp.Key(wgKey)) diff --git a/client/internal/rosenpass/netbird_handler.go b/client/internal/rosenpass/netbird_handler.go index 9de2409ef..672650ca7 100644 --- a/client/internal/rosenpass/netbird_handler.go +++ b/client/internal/rosenpass/netbird_handler.go @@ -18,19 +18,34 @@ type PresharedKeySetter interface { type wireGuardPeer struct { Interface string PublicKey rp.Key + // initialized is true once a completed exchange has set a + // Rosenpass-managed PSK for this peer. + initialized bool + // chainKey is the key output by the last completed exchange, advanced by + // one ratchet step on expiry. Nil until the first exchange completes and + // after the peer has fallen back to the rendezvous key. + chainKey *wgtypes.Key + // expiries counts failed renewals since the last completed exchange. + expiries int } type NetbirdHandler struct { - mu sync.Mutex - iface PresharedKeySetter - peers map[rp.PeerID]wireGuardPeer - initializedPeers map[rp.PeerID]bool + mu sync.Mutex + iface PresharedKeySetter + // preSharedKey is the account-level preshared key, used as the rendezvous + // key when set. Nil means the deterministic seed key is used instead. + preSharedKey *[32]byte + // localWgKey is the local WireGuard public key, one of the two inputs to + // the deterministic seed key. + localWgKey wgtypes.Key + peers map[rp.PeerID]*wireGuardPeer } -func NewNetbirdHandler() *NetbirdHandler { +func NewNetbirdHandler(preSharedKey *[32]byte, localWgKey wgtypes.Key) *NetbirdHandler { return &NetbirdHandler{ - peers: map[rp.PeerID]wireGuardPeer{}, - initializedPeers: map[rp.PeerID]bool{}, + preSharedKey: preSharedKey, + localWgKey: localWgKey, + peers: map[rp.PeerID]*wireGuardPeer{}, } } @@ -42,10 +57,16 @@ func (h *NetbirdHandler) SetInterface(iface PresharedKeySetter) { h.iface = iface } +// AddPeer registers a peer with the handler. Re-adding a known peer (every +// reconnection does) keeps its key recovery state. func (h *NetbirdHandler) AddPeer(pid rp.PeerID, intf string, pk rp.Key) { h.mu.Lock() defer h.mu.Unlock() - h.peers[pid] = wireGuardPeer{ + if existing, ok := h.peers[pid]; ok && existing.PublicKey == pk { + existing.Interface = intf + return + } + h.peers[pid] = &wireGuardPeer{ Interface: intf, PublicKey: pk, } @@ -55,7 +76,6 @@ func (h *NetbirdHandler) RemovePeer(pid rp.PeerID) { h.mu.Lock() defer h.mu.Unlock() delete(h.peers, pid) - delete(h.initializedPeers, pid) } // IsPeerInitialized returns true if Rosenpass has completed a handshake @@ -63,50 +83,120 @@ func (h *NetbirdHandler) RemovePeer(pid rp.PeerID) { func (h *NetbirdHandler) IsPeerInitialized(pid rp.PeerID) bool { h.mu.Lock() defer h.mu.Unlock() - return h.initializedPeers[pid] + peer, ok := h.peers[pid] + return ok && peer.initialized } +// HandshakeCompleted programs the freshly exchanged output key and resets the +// peer's key recovery state. func (h *NetbirdHandler) HandshakeCompleted(pid rp.PeerID, key rp.Key) { - h.outputKey(rp.KeyOutputReasonStale, pid, key) -} + psk := wgtypes.Key(key) -func (h *NetbirdHandler) HandshakeExpired(pid rp.PeerID) { - key, _ := rp.GeneratePresharedKey() - h.outputKey(rp.KeyOutputReasonStale, pid, key) -} - -func (h *NetbirdHandler) outputKey(_ rp.KeyOutputReason, pid rp.PeerID, psk rp.Key) { h.mu.Lock() - iface := h.iface - wg, ok := h.peers[pid] - isInitialized := h.initializedPeers[pid] - h.mu.Unlock() + defer h.mu.Unlock() - if iface == nil { - log.Warn("rosenpass: interface not set, cannot update preshared key") + peer, ok := h.peers[pid] + if !ok { return } + if peer.expiries > 0 { + log.Infof("rosenpass exchange completed for peer %s after %d expired renewals", wgtypes.Key(peer.PublicKey), peer.expiries) + } + // chainKey tracks the shared exchange output regardless of the local write + // outcome, so both ends still converge on the next expiry. + peer.chainKey = &psk + peer.expiries = 0 + if !h.applyKeyLocked(pid, psk, peer.initialized) { + return + } + peer.initialized = true +} +// HandshakeExpired replaces the expired key. The renewal exchange runs over +// the tunnel keyed by the PSK itself, so the replacement must be derivable on +// both ends without communication: the first expiry ratchets the last shared +// key forward, repeated expiries (and expiries without a completed exchange) +// fall back to the rendezvous key and drop the peer out of the initialized +// state so connection reconfigurations reprogram the rendezvous key as well. +func (h *NetbirdHandler) HandshakeExpired(pid rp.PeerID) { + h.mu.Lock() + defer h.mu.Unlock() + + peer, ok := h.peers[pid] if !ok { return } - peerKey := wgtypes.Key(wg.PublicKey).String() - pskKey := wgtypes.Key(psk) + peer.expiries++ - // Use updateOnly=true for later rotations (peer already has Rosenpass PSK) - // Use updateOnly=false for first rotation (peer has original/empty PSK) - if err := iface.SetPresharedKey(peerKey, pskKey, isInitialized); err != nil { + var psk wgtypes.Key + if peer.chainKey != nil && peer.expiries == 1 { + log.Infof("rosenpass key for peer %s expired without renewal, advancing to ratcheted key", wgtypes.Key(peer.PublicKey)) + psk = RatchetKey(*peer.chainKey) + peer.chainKey = &psk + } else { + rendezvous, err := h.rendezvousKey(peer) + if err != nil { + // Fail closed: without a rendezvous key the expired key must + // still be rotated out, even if the replacement is unusable. + log.Errorf("failed to derive rendezvous key, replacing expired key with a random one: %v", err) + h.applyRandomKeyLocked(pid) + return + } + log.Warnf("rosenpass key for peer %s expired %d times without renewal, falling back to the rendezvous key", wgtypes.Key(peer.PublicKey), peer.expiries) + psk = rendezvous + peer.chainKey = nil + peer.initialized = false + } + + h.applyKeyLocked(pid, psk, true) +} + +// rendezvousKey returns the key both ends converge on without communication: +// the account-level preshared key when configured, the deterministic seed key +// otherwise. It mirrors the key that peer connections program when Rosenpass +// does not manage the peer yet. +func (h *NetbirdHandler) rendezvousKey(peer *wireGuardPeer) (wgtypes.Key, error) { + if h.preSharedKey != nil { + return *h.preSharedKey, nil + } + + seed, err := DeterministicSeedKey(h.localWgKey.String(), wgtypes.Key(peer.PublicKey).String()) + if err != nil { + return wgtypes.Key{}, err + } + return *seed, nil +} + +// applyKeyLocked writes the preshared key for the peer to the WireGuard +// interface and reports whether the write succeeded. Callers must hold h.mu +// for the whole state-mutation-plus-write so that a concurrent completion and +// expiry cannot reorder their writes relative to the in-memory chain key. +func (h *NetbirdHandler) applyKeyLocked(pid rp.PeerID, psk wgtypes.Key, updateOnly bool) bool { + peer, ok := h.peers[pid] + if !ok { + return false + } + + if h.iface == nil { + log.Warn("rosenpass: interface not set, cannot update preshared key") + return false + } + + peerKey := wgtypes.Key(peer.PublicKey).String() + if err := h.iface.SetPresharedKey(peerKey, psk, updateOnly); err != nil { log.Errorf("Failed to apply rosenpass key: %v", err) + return false + } + + return true +} + +func (h *NetbirdHandler) applyRandomKeyLocked(pid rp.PeerID) { + key, err := rp.GeneratePresharedKey() + if err != nil { + log.Errorf("failed to generate random preshared key: %v", err) return } - - // Mark peer as isInitialized after the successful first rotation - if !isInitialized { - h.mu.Lock() - if _, exists := h.peers[pid]; exists { - h.initializedPeers[pid] = true - } - h.mu.Unlock() - } + h.applyKeyLocked(pid, wgtypes.Key(key), true) } diff --git a/client/internal/rosenpass/netbird_handler_test.go b/client/internal/rosenpass/netbird_handler_test.go new file mode 100644 index 000000000..9d91ba93b --- /dev/null +++ b/client/internal/rosenpass/netbird_handler_test.go @@ -0,0 +1,250 @@ +package rosenpass + +import ( + "testing" + + rp "cunicu.li/go-rosenpass" + "github.com/stretchr/testify/require" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" +) + +// handlerTestLink wires two NetbirdHandlers as the two ends of a single +// tunnel: handler A manages the rosenpass peer B and vice versa, the way two +// NetBird clients see each other. +type handlerTestLink struct { + handlerA, handlerB *NetbirdHandler + ifaceA, ifaceB *mockIface + pidA, pidB rp.PeerID + wgKeyA, wgKeyB wgtypes.Key +} + +func newHandlerTestLink(t *testing.T, preSharedKey *[32]byte) *handlerTestLink { + t.Helper() + + link := &handlerTestLink{ + ifaceA: &mockIface{}, + ifaceB: &mockIface{}, + } + link.pidA[0] = 0xaa + link.pidB[0] = 0xbb + link.wgKeyA[31] = 1 + link.wgKeyB[31] = 2 + + link.handlerA = NewNetbirdHandler(preSharedKey, link.wgKeyA) + link.handlerB = NewNetbirdHandler(preSharedKey, link.wgKeyB) + + link.handlerA.SetInterface(link.ifaceA) + link.handlerB.SetInterface(link.ifaceB) + + link.handlerA.AddPeer(link.pidB, "wt0", rp.Key(link.wgKeyB)) + link.handlerB.AddPeer(link.pidA, "wt0", rp.Key(link.wgKeyA)) + + return link +} + +// complete simulates a completed rosenpass exchange: both ends derive the +// same output key. +func (l *handlerTestLink) complete(osk rp.Key) { + l.handlerA.HandshakeCompleted(l.pidB, osk) + l.handlerB.HandshakeCompleted(l.pidA, osk) +} + +// expire simulates a failed key renewal on both ends. +func (l *handlerTestLink) expire() { + l.handlerA.HandshakeExpired(l.pidB) + l.handlerB.HandshakeExpired(l.pidA) +} + +func lastPSK(t *testing.T, m *mockIface) wgtypes.Key { + t.Helper() + m.mu.Lock() + defer m.mu.Unlock() + require.NotEmpty(t, m.calls, "expected at least one SetPresharedKey call") + return m.calls[len(m.calls)-1].psk +} + +func TestHandshakeCompleted_SetsKeyAndInitializes(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + require.Equal(t, wgtypes.Key(osk), lastPSK(t, link.ifaceA), "completed exchange must program the osk") + require.False(t, link.ifaceA.calls[0].updateOnly, "first rotation must not be update-only") + require.True(t, link.handlerA.IsPeerInitialized(link.pidB), "peer must be initialized after first completed exchange") + + link.complete(osk) + require.True(t, link.ifaceA.calls[1].updateOnly, "later rotations must be update-only") +} + +// TestHandshakeExpired_BothSidesConverge encodes the core recovery invariant: +// rosenpass renewals run over the tunnel that the PSK itself keys, so when a +// renewal fails on both ends, both ends must fall back to the same key or the +// tunnel can never handshake again. +func TestHandshakeExpired_BothSidesConverge(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + link.expire() + keyA := lastPSK(t, link.ifaceA) + keyB := lastPSK(t, link.ifaceB) + require.NotEqual(t, wgtypes.Key(osk), keyA, "expired key must be rotated out") + require.Equal(t, keyA, keyB, "both ends must converge on the same key after expiry") + + link.expire() + require.Equal(t, lastPSK(t, link.ifaceA), lastPSK(t, link.ifaceB), + "both ends must still converge after repeated expiries") +} + +// TestHandshakeExpired_ExpiryWithoutCompletionConverges covers the bootstrap +// case: the initial exchange never completed (the tunnel ran on the rendezvous +// key), so an expiry must not replace the working key with an unrecoverable +// one on either end. +func TestHandshakeExpired_ExpiryWithoutCompletionConverges(t *testing.T) { + link := newHandlerTestLink(t, nil) + + link.expire() + require.Equal(t, lastPSK(t, link.ifaceA), lastPSK(t, link.ifaceB), + "both ends must converge when the exchange never completed") +} + +// TestHandshakeExpired_RepeatedExpiryClearsInitialized: once renewals keep +// failing, the peer must drop out of the initialized state so the next +// connection reconfiguration reprograms the rendezvous key instead of +// preserving a poisoned rosenpass-managed key. +func TestHandshakeExpired_RepeatedExpiryClearsInitialized(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + link.expire() + link.expire() + + require.False(t, link.handlerA.IsPeerInitialized(link.pidB), + "repeated expiries must clear the initialized state") + require.False(t, link.handlerB.IsPeerInitialized(link.pidA), + "repeated expiries must clear the initialized state") +} + +// TestHandshakeCompleted_AfterExpiryRecovers: a completed exchange after a +// desync must fully reset the recovery state. +func TestHandshakeCompleted_AfterExpiryRecovers(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk1, osk2 rp.Key + osk1[0] = 1 + osk2[0] = 2 + + link.complete(osk1) + link.expire() + link.expire() + + link.complete(osk2) + require.Equal(t, wgtypes.Key(osk2), lastPSK(t, link.ifaceA), "new exchange must program the fresh osk") + require.True(t, link.handlerA.IsPeerInitialized(link.pidB), "peer must be initialized again after recovery") + + link.expire() + require.Equal(t, lastPSK(t, link.ifaceA), lastPSK(t, link.ifaceB), + "recovered link must converge again on the next expiry") + require.NotEqual(t, wgtypes.Key(osk2), lastPSK(t, link.ifaceA), "expired key must be rotated out") +} + +// TestHandshakeExpired_FirstExpiryRatchetsLastKey: the first expiry must +// derive the replacement from the last shared key, so an attacker who only +// blocks the renewal exchange gains nothing over the previous key. +func TestHandshakeExpired_FirstExpiryRatchetsLastKey(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + link.expire() + require.Equal(t, RatchetKey(wgtypes.Key(osk)), lastPSK(t, link.ifaceA), + "first expiry must program the ratcheted key") + require.True(t, link.handlerA.IsPeerInitialized(link.pidB), + "ratchet step must keep the peer initialized so reconfigurations preserve the key") +} + +// TestHandshakeExpired_RepeatedExpiryFallsBackToSeed: once the ratchet key +// also fails, both ends must land on the same key that peer connections +// program for uninitialized peers, so a reconnect completes the recovery. +func TestHandshakeExpired_RepeatedExpiryFallsBackToSeed(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + link.expire() + link.expire() + + seed, err := DeterministicSeedKey(link.wgKeyA.String(), link.wgKeyB.String()) + require.NoError(t, err) + require.Equal(t, *seed, lastPSK(t, link.ifaceA), "repeated expiry must fall back to the seed key") + require.Equal(t, *seed, lastPSK(t, link.ifaceB), "repeated expiry must fall back to the seed key") +} + +// TestHandshakeExpired_ConfiguredPSKUsedAsRendezvous: with an account-level +// preshared key configured, the fallback must be that key, matching what peer +// connections program for uninitialized peers. +func TestHandshakeExpired_ConfiguredPSKUsedAsRendezvous(t *testing.T) { + psk := &[32]byte{0x77} + link := newHandlerTestLink(t, psk) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + link.expire() + link.expire() + + require.Equal(t, wgtypes.Key(*psk), lastPSK(t, link.ifaceA), + "fallback must be the configured preshared key") + require.Equal(t, wgtypes.Key(*psk), lastPSK(t, link.ifaceB), + "fallback must be the configured preshared key on both ends") +} + +// TestHandshakeExpired_ExpiryWritesAreUpdateOnly: expiry replacements must +// never create a WireGuard peer that connection management has removed. +func TestHandshakeExpired_ExpiryWritesAreUpdateOnly(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + + link.expire() + link.expire() + + for _, call := range link.ifaceA.calls[1:] { + require.True(t, call.updateOnly, "expiry writes must be update-only") + } +} + +// TestAddPeer_ReAddKeepsRecoveryState: reconnections re-add the peer on every +// OnConnected; that must not reset the expiry chain state. +func TestAddPeer_ReAddKeepsRecoveryState(t *testing.T) { + link := newHandlerTestLink(t, nil) + + var osk rp.Key + osk[0] = 0x42 + link.complete(osk) + link.expire() + + link.handlerA.AddPeer(link.pidB, "wt0", rp.Key(link.wgKeyB)) + require.True(t, link.handlerA.IsPeerInitialized(link.pidB), + "re-adding a known peer must keep its state") + + link.expire() + seed, err := DeterministicSeedKey(link.wgKeyA.String(), link.wgKeyB.String()) + require.NoError(t, err) + require.Equal(t, *seed, lastPSK(t, link.ifaceA), + "second expiry after re-add must continue to the seed fallback") +} diff --git a/client/internal/rosenpass/seed.go b/client/internal/rosenpass/seed.go index 83aba1e0e..052c11ed4 100644 --- a/client/internal/rosenpass/seed.go +++ b/client/internal/rosenpass/seed.go @@ -1,11 +1,28 @@ package rosenpass import ( + "crypto/sha256" "fmt" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" ) +// ratchetLabel domain-separates the expiry ratchet from other uses of the +// rosenpass output key. +const ratchetLabel = "netbird-rosenpass-expiry-ratchet" + +// RatchetKey derives the successor preshared key from the previous Rosenpass +// output key. When a key expires without a completed renewal, both peers +// advance their last shared key by one ratchet step: the expired key is +// rotated out while both ends still converge on an identical, non-public +// replacement without communicating. +func RatchetKey(prev wgtypes.Key) wgtypes.Key { + input := make([]byte, 0, len(ratchetLabel)+len(prev)) + input = append(input, ratchetLabel...) + input = append(input, prev[:]...) + return sha256.Sum256(input) +} + // DeterministicSeedKey derives a 32-byte WireGuard preshared key from a pair // of peer public keys. Both peers, given the same key pair, produce the same // output regardless of which side runs the function: the inputs are ordered From 3d87547d952f5ada9df987bbe4f0f6d54372d77c Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:42:06 +0900 Subject: [PATCH 05/19] [client] Bump golang.org/x/crypto to v0.54.0 and Go toolchain to 1.25.12 (#6709) --- go.mod | 18 +++++++-------- go.sum | 32 +++++++++++++-------------- management/internals/server/server.go | 3 ++- signal/cmd/run.go | 3 ++- 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index d57c7b495..524068aaf 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/netbirdio/netbird go 1.25.5 -toolchain go1.25.11 +toolchain go1.25.12 require ( cunicu.li/go-rosenpass v0.5.42 @@ -19,8 +19,8 @@ require ( github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.9 github.com/vishvananda/netlink v1.3.1 - golang.org/x/crypto v0.50.0 - golang.org/x/sys v0.43.0 + golang.org/x/crypto v0.54.0 + golang.org/x/sys v0.47.0 golang.zx2c4.com/wireguard v0.0.0-20231211153847-12269c276173 golang.zx2c4.com/wireguard/wgctrl v0.0.0-20241231184526-a9ab2273dd10 golang.zx2c4.com/wireguard/windows v0.5.3 @@ -128,11 +128,11 @@ require ( goauthentik.io/api/v3 v3.2023051.3 golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b golang.org/x/mobile v0.0.0-20251113184115-a159579294ab - golang.org/x/mod v0.34.0 - golang.org/x/net v0.53.0 + golang.org/x/mod v0.37.0 + golang.org/x/net v0.56.0 golang.org/x/oauth2 v0.36.0 - golang.org/x/sync v0.20.0 - golang.org/x/term v0.42.0 + golang.org/x/sync v0.22.0 + golang.org/x/term v0.45.0 golang.org/x/time v0.15.0 google.golang.org/api v0.276.0 gopkg.in/yaml.v3 v3.0.1 @@ -332,8 +332,8 @@ require ( go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect golang.org/x/image v0.33.0 // indirect - golang.org/x/text v0.36.0 // indirect - golang.org/x/tools v0.43.0 // indirect + golang.org/x/text v0.40.0 // indirect + golang.org/x/tools v0.47.0 // indirect golang.zx2c4.com/wintun v0.0.0-20230126152724-0fa3db229ce2 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260401024825-9d38bb4040a9 // indirect gopkg.in/square/go-jose.v2 v2.6.0 // indirect diff --git a/go.sum b/go.sum index 7b29b7604..561416e8a 100644 --- a/go.sum +++ b/go.sum @@ -781,8 +781,8 @@ golang.org/x/crypto v0.18.0/go.mod h1:R0j02AL6hcrfOiy9T4ZYp/rcWeMxM3L6QYxlOuEG1m golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/crypto v0.31.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk= -golang.org/x/crypto v0.50.0 h1:zO47/JPrL6vsNkINmLoo/PH1gcxpls50DNogFvB5ZGI= -golang.org/x/crypto v0.50.0/go.mod h1:3muZ7vA7PBCE6xgPX7nkzzjiUq87kRItoJQM1Yo8S+Q= +golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= +golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b h1:M2rDM6z3Fhozi9O7NWsxAkg/yqS/lQJ6PmkyIV3YP+o= golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b/go.mod h1:3//PLf8L/X+8b4vuAfHzxeRUl04Adcb341+IGKfnqS8= golang.org/x/image v0.33.0 h1:LXRZRnv1+zGd5XBUVRFmYEphyyKJjQjCRiOuAP3sZfQ= @@ -799,8 +799,8 @@ golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.34.0 h1:xIHgNUUnW6sYkcM5Jleh05DvLOtwc6RitGHbDk4akRI= -golang.org/x/mod v0.34.0/go.mod h1:ykgH52iCZe79kzLLMhyCUzhMci+nQj+0XkbXpNYtVjY= +golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= +golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR5pz3Of3rY3CfYBVs4xY44aLks= @@ -819,8 +819,8 @@ golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk= golang.org/x/net v0.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY= golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= -golang.org/x/net v0.53.0 h1:d+qAbo5L0orcWAr0a9JweQpjXF19LMXJE8Ey7hwOdUA= -golang.org/x/net v0.53.0/go.mod h1:JvMuJH7rrdiCfbeHoo3fCQU24Lf5JJwT9W3sJFulfgs= +golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= +golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= golang.org/x/oauth2 v0.8.0/go.mod h1:yr7u4HXZRm1R1kBWqr/xKNqewf0plRYoB7sla+BCIXE= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= @@ -835,8 +835,8 @@ golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= -golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -872,8 +872,8 @@ golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.28.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.43.0 h1:Rlag2XtaFTxp19wS8MXlJwTvoh8ArU6ezoyFsMyCTNI= -golang.org/x/sys v0.43.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/telemetry v0.0.0-20240228155512-f48c80bd79b2/go.mod h1:TeRTkGYfJXctD9OcfyVLyj2J3IxLnKwHJR8f4D8a3YE= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= @@ -886,8 +886,8 @@ golang.org/x/term v0.16.0/go.mod h1:yn7UURbUtPyrVJPGPq404EukNFxcm/foM+bV/bfcDsY= golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/term v0.20.0/go.mod h1:8UkIAJTvZgivsXaD6/pH6U9ecQzZ45awqEOzuCvwpFY= golang.org/x/term v0.27.0/go.mod h1:iMsnZpn0cago0GOrHO2+Y7u7JPn5AylBrcoWkElMTSM= -golang.org/x/term v0.42.0 h1:UiKe+zDFmJobeJ5ggPwOshJIVt6/Ft0rcfrXZDLWAWY= -golang.org/x/term v0.42.0/go.mod h1:Dq/D+snpsbazcBG5+F9Q1n2rXV8Ma+71xEjTRufARgY= +golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= @@ -899,8 +899,8 @@ golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ= -golang.org/x/text v0.36.0 h1:JfKh3XmcRPqZPKevfXVpI1wXPTqbkE5f7JA92a55Yxg= -golang.org/x/text v0.36.0/go.mod h1:NIdBknypM8iqVmPiuco0Dh6P5Jcdk8lJL0CUebqK164= +golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= +golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= @@ -914,8 +914,8 @@ golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58= golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= -golang.org/x/tools v0.43.0 h1:12BdW9CeB3Z+J/I/wj34VMl8X+fEXBxVR90JeMX5E7s= -golang.org/x/tools v0.43.0/go.mod h1:uHkMso649BX2cZK6+RpuIPXS3ho2hZo4FVwfoy1vIk0= +golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= +golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/management/internals/server/server.go b/management/internals/server/server.go index 9411073ac..7fd06d947 100644 --- a/management/internals/server/server.go +++ b/management/internals/server/server.go @@ -15,7 +15,7 @@ import ( "go.opentelemetry.io/otel/metric" "golang.org/x/crypto/acme/autocert" "golang.org/x/net/http2" - "golang.org/x/net/http2/h2c" + "golang.org/x/net/http2/h2c" //nolint:staticcheck "google.golang.org/grpc" "github.com/netbirdio/netbird/encryption" @@ -382,6 +382,7 @@ func (s *BaseServer) serveGRPCWithHTTP(ctx context.Context, listener net.Listene // the following magic is needed to support HTTP2 without TLS // and still share a single port between gRPC and HTTP APIs h1s := &http.Server{ + //nolint:staticcheck // h2c also handles the HTTP/1 Upgrade mechanism, which http.Server's UnencryptedHTTP2 does not Handler: h2c.NewHandler(handler, &http2.Server{}), } err = h1s.Serve(listener) diff --git a/signal/cmd/run.go b/signal/cmd/run.go index 681222403..81e9cc926 100644 --- a/signal/cmd/run.go +++ b/signal/cmd/run.go @@ -16,7 +16,7 @@ import ( "go.opentelemetry.io/otel/metric" "golang.org/x/crypto/acme/autocert" "golang.org/x/net/http2" - "golang.org/x/net/http2/h2c" + "golang.org/x/net/http2/h2c" //nolint:staticcheck "github.com/netbirdio/netbird/shared/metrics" @@ -281,6 +281,7 @@ func serveHTTP(httpListener net.Listener, handler http.Handler) { go func() { // Use h2c to support HTTP/2 without TLS (needed for gRPC) h1s := &http.Server{ + //nolint:staticcheck // h2c also handles the HTTP/1 Upgrade mechanism, which http.Server's UnencryptedHTTP2 does not Handler: h2c.NewHandler(handler, &http2.Server{}), } err := h1s.Serve(httpListener) From 277d8e4c5352950e1ec4fbd21a3266f0412b09fe Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Tue, 14 Jul 2026 19:03:01 +0200 Subject: [PATCH 06/19] [proxy] enforce model allowlist for URL-routed providers (Bedrock/Vertex) (#6764) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes The Agent Network policy Guardrail "Model Allowlist" was not enforced for providers whose model travels in the URL/path rather than the JSON body — most visibly AWS Bedrock (reported in netbirdio/netbird#6751), and the same class applies to Google Vertex. Root cause: the `llm_guardrail` allowlist check **failed open**. `evaluateAllowlist` returned allow whenever the request model was absent from the metadata bag (`middleware.go`, `if !modelPresent { return nil }`). The model is stamped upstream by `llm_request_parser`; for body-routed providers (OpenAI/Anthropic) it comes from the JSON body, but for path-routed providers the model is recovered only when the request matches a recognized path shape (Bedrock `/model/{id}/{invoke|converse|...}`, Vertex `/v1/projects/.../publishers/.../models/...`). Any shape the parser did not recognize reached the guardrail with no model and was allowed regardless of the allowlist. Fix (provider-agnostic): **fail closed**. When an allowlist is configured and the model cannot be determined (absent or empty), the request is denied `403` with a distinct `llm_policy.model_unknown` reason. This closes the bypass for Bedrock, Vertex, and any future URL-routed provider in one place. When no allowlist is configured, behavior is unchanged. The model allowlist is enforced solely in the proxy `llm_guardrail`; management's `CheckLLMPolicyLimits` handles only token/budget caps, so no management change is required. ## Issue ticket number and link ## Stack - \#6726 - \#6764 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Bug fix that restores the documented allowlist behavior; no user-facing surface changes. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: \_\_ ## Tests - `llm_guardrail`: absent/empty model under a configured allowlist now denies (`model_unknown`); empty allowlist still allows a missing model (fail-closed only applies when a list is set); existing allow/deny/case-insensitive cases retained. - `llm_request_parser`: new parser→guardrail integration test drives real **Bedrock** (`/model/{id}/invoke`) and **Vertex** (`/v1/projects/.../models/...`) URL shapes and asserts allowed→200, disallowed→403 (`model_blocked`), and an unrecognized Bedrock action→403 (`model_unknown`, the #6751 regression guard). Note: a full through-tunnel e2e for the allowlist is intentionally deferred — the agent-network e2e (`WaitProxyPeer`) is currently red on `main`/`0.74.x` for an unrelated lazy-connection reason; it will be added once that harness gate is fixed. --- e2e/agentnetwork/chat_test.go | 9 +- e2e/agentnetwork/guardrail_test.go | 168 ++++++++++++++++++ e2e/harness/agentnetwork.go | 11 ++ e2e/harness/client.go | 16 ++ .../builtin/llm_guardrail/middleware.go | 34 +++- .../builtin/llm_guardrail/middleware_test.go | 37 +++- .../guardrail_allowlist_test.go | 106 +++++++++++ 7 files changed, 369 insertions(+), 12 deletions(-) create mode 100644 e2e/agentnetwork/guardrail_test.go create mode 100644 proxy/internal/middleware/builtin/llm_request_parser/guardrail_allowlist_test.go diff --git a/e2e/agentnetwork/chat_test.go b/e2e/agentnetwork/chat_test.go index 5e3a79273..487aa3cea 100644 --- a/e2e/agentnetwork/chat_test.go +++ b/e2e/agentnetwork/chat_test.go @@ -91,7 +91,7 @@ func availableProviders() []providerCase { if region == "" { region = "us-east-1" } - ps = append(ps, providerCase{name: "bedrock", catalogID: "bedrock_api", upstream: "https://bedrock-runtime." + region + ".amazonaws.com", apiKey: k, model: "us.anthropic.claude-haiku-4-5", kind: harness.WireMessages}) + ps = append(ps, providerCase{name: "bedrock", catalogID: "bedrock_api", upstream: "https://bedrock-runtime." + region + ".amazonaws.com", apiKey: k, model: "us.anthropic.claude-haiku-4-5", kind: harness.WireBedrock}) } return ps } @@ -224,9 +224,12 @@ func TestProvidersMatrix(t *testing.T) { var c int var b string var cerr error - if pc.kind == harness.WireVertex { + switch pc.kind { + case harness.WireVertex: c, b, cerr = cl.Vertex(ctx, settings.Endpoint, proxyIP, pc.project, pc.region, pc.model, "Reply with exactly: pong", sessionID) - } else { + case harness.WireBedrock: + c, b, cerr = cl.Bedrock(ctx, settings.Endpoint, proxyIP, pc.model, "Reply with exactly: pong", sessionID) + default: c, b, cerr = cl.Chat(ctx, settings.Endpoint, proxyIP, pc.kind, pc.model, "Reply with exactly: pong", sessionID) } if cerr == nil { diff --git a/e2e/agentnetwork/guardrail_test.go b/e2e/agentnetwork/guardrail_test.go new file mode 100644 index 000000000..bb952044f --- /dev/null +++ b/e2e/agentnetwork/guardrail_test.go @@ -0,0 +1,168 @@ +//go:build e2e + +package agentnetwork + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/e2e/harness" + "github.com/netbirdio/netbird/shared/management/http/api" +) + +// catalogModel returns the normalized catalog id the proxy stamps for a +// path-routed provider's configured model — the form the guardrail allowlist is +// compared against (region prefix / @version stripped). +func catalogModel(pc providerCase) string { + switch pc.kind { + case harness.WireBedrock: + return strings.TrimPrefix(pc.model, "us.") + case harness.WireVertex: + return strings.SplitN(pc.model, "@", 2)[0] + default: + return pc.model + } +} + +// disallowedModel returns a valid-shaped model id for the provider that is NOT +// the configured/allowed one, so the guardrail must reject it before the +// request ever reaches the upstream. +func disallowedModel(pc providerCase) string { + switch pc.kind { + case harness.WireBedrock: + return "us.anthropic.claude-opus-4-8" + case harness.WireVertex: + return "claude-opus-4-8@20250101" + default: + return "unlisted-model" + } +} + +// sendModel drives one request for the given model through the provider's native +// wire shape and returns the HTTP status. +func sendModel(ctx context.Context, t *testing.T, cl *harness.Client, endpoint, proxyIP string, pc providerCase, model string) int { + t.Helper() + var code int + var err error + switch pc.kind { + case harness.WireBedrock: + code, _, err = cl.Bedrock(ctx, endpoint, proxyIP, model, "Reply with exactly: pong", "") + case harness.WireVertex: + code, _, err = cl.Vertex(ctx, endpoint, proxyIP, pc.project, pc.region, model, "Reply with exactly: pong", "") + default: + code, _, err = cl.Chat(ctx, endpoint, proxyIP, pc.kind, model, "Reply with exactly: pong", "") + } + require.NoError(t, err, "request must reach the proxy for %s", pc.name) + return code +} + +// TestModelAllowlistEnforced provisions a Model Allowlist guardrail limiting each +// path-routed provider (Bedrock, Vertex) to its configured model, then drives +// requests over the tunnel: the allowed model returns 200 while a model outside +// the allowlist is denied 403 by the guardrail before it reaches the upstream. +// This is the coverage missing for #6751 — the model for these providers travels +// in the URL path, and the allowlist must be enforced there. +func TestModelAllowlistEnforced(t *testing.T) { + var providers []providerCase + for _, pc := range availableProviders() { + if pc.kind == harness.WireBedrock || pc.kind == harness.WireVertex { + providers = append(providers, pc) + } + } + if len(providers) == 0 { + t.Skip("no path-routed provider keys set (AWS_BEARER_TOKEN_BEDROCK / GOOGLE_VERTEX_*); source ~/.llm-keys") + } + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Minute) + defer cancel() + + grp, err := srv.API().Groups.Create(ctx, api.PostApiGroupsJSONRequestBody{Name: "e2e-allowlist"}) + require.NoError(t, err, "create group") + t.Cleanup(func() { _ = srv.API().Groups.Delete(context.Background(), grp.Id) }) + + ephemeral := false + sk, err := srv.API().SetupKeys.Create(ctx, api.PostApiSetupKeysJSONRequestBody{ + Name: "e2e-allowlist-client", + Type: "reusable", + ExpiresIn: 86400, + UsageLimit: 0, + AutoGroups: []string{grp.Id}, + Ephemeral: &ephemeral, + }) + require.NoError(t, err, "mint setup key") + + // Providers with their configured (allowed) models; the first bootstraps the cluster. + ids := make([]string, 0, len(providers)) + allowed := make([]string, 0, len(providers)) + for i, pc := range providers { + req := providerRequest(pc) + if i == 0 { + req.BootstrapCluster = ptr(harness.AgentNetworkCluster) + } + prov, perr := srv.CreateProvider(ctx, req) + require.NoError(t, perr, "create provider %s", pc.name) + id := prov.Id + ids = append(ids, id) + allowed = append(allowed, catalogModel(pc)) + t.Cleanup(func() { _ = srv.DeleteProvider(context.Background(), id) }) + } + + // Guardrail allowlisting exactly the configured models. + var gr api.AgentNetworkGuardrailRequest + gr.Name = "e2e-allowlist" + gr.Checks.ModelAllowlist.Enabled = true + gr.Checks.ModelAllowlist.Models = allowed + guard, err := srv.CreateGuardrail(ctx, gr) + require.NoError(t, err, "create guardrail") + t.Cleanup(func() { _ = srv.DeleteGuardrail(context.Background(), guard.Id) }) + + enabled := true + pol, err := srv.CreatePolicy(ctx, api.AgentNetworkPolicyRequest{ + Name: "e2e-allowlist", + Enabled: &enabled, + SourceGroups: []string{grp.Id}, + DestinationProviderIds: ids, + GuardrailIds: &[]string{guard.Id}, + }) + require.NoError(t, err, "create policy") + t.Cleanup(func() { _ = srv.DeletePolicy(context.Background(), pol.Id) }) + + settings, err := srv.GetSettings(ctx) + require.NoError(t, err, "read settings for endpoint") + require.NotEmpty(t, settings.Endpoint, "agent-network endpoint must be assigned") + + proxyToken, err := srv.CreateProxyTokenCLI(ctx, "e2e-proxy-allowlist") + require.NoError(t, err, "mint proxy token via CLI") + px, err := harness.StartProxy(ctx, srv, proxyToken) + require.NoError(t, err, "start proxy") + t.Cleanup(func() { _ = px.Terminate(context.Background()) }) + + cl, err := harness.StartClient(ctx, srv, sk.Key) + require.NoError(t, err, "start client") + t.Cleanup(func() { _ = cl.Terminate(context.Background()) }) + + require.NoError(t, cl.WaitConnected(ctx, 90*time.Second), "client must connect to management") + if err := cl.WaitProxyPeer(ctx, 180*time.Second); err != nil { + t.Fatalf("client did not see the proxy peer: %v\n=== proxy logs ===\n%s", err, px.Logs(context.Background())) + } + proxyIP, err := cl.ResolveProxyIP(ctx, settings.Endpoint) + require.NoError(t, err, "resolve agent-network endpoint to proxy IP") + + for _, pc := range providers { + pc := pc + t.Run(pc.name, func(t *testing.T) { + // The admin's allowlisted model is served end to end. + assert.Equal(t, 200, sendModel(ctx, t, cl, settings.Endpoint, proxyIP, pc, pc.model), + "allowlisted model must be permitted for %s", pc.name) + // A model outside the allowlist is rejected by the guardrail (before + // the upstream), regardless of whether it is a real catalog model. + assert.Equal(t, 403, sendModel(ctx, t, cl, settings.Endpoint, proxyIP, pc, disallowedModel(pc)), + "model outside the allowlist must be denied for %s", pc.name) + }) + } +} diff --git a/e2e/harness/agentnetwork.go b/e2e/harness/agentnetwork.go index 192385ab1..53aa8e342 100644 --- a/e2e/harness/agentnetwork.go +++ b/e2e/harness/agentnetwork.go @@ -107,6 +107,17 @@ func (c *Combined) DeletePolicy(ctx context.Context, id string) error { return anDelete(ctx, c, "/api/agent-network/policies/"+id) } +// CreateGuardrail creates an agent-network guardrail (e.g. a model allowlist) +// that can then be attached to a policy via its GuardrailIds. +func (c *Combined) CreateGuardrail(ctx context.Context, req api.AgentNetworkGuardrailRequest) (api.AgentNetworkGuardrail, error) { + return anRequest[api.AgentNetworkGuardrail](ctx, c, http.MethodPost, "/api/agent-network/guardrails", req) +} + +// DeleteGuardrail removes a guardrail by id. +func (c *Combined) DeleteGuardrail(ctx context.Context, id string) error { + return anDelete(ctx, c, "/api/agent-network/guardrails/"+id) +} + // GetSettings returns the account's agent-network settings row. It exists only // after the first provider create bootstraps it. func (c *Combined) GetSettings(ctx context.Context) (api.AgentNetworkSettings, error) { diff --git a/e2e/harness/client.go b/e2e/harness/client.go index 1ce8c0f6e..19210349f 100644 --- a/e2e/harness/client.go +++ b/e2e/harness/client.go @@ -194,6 +194,11 @@ const ( // WireVertex is the Anthropic-on-Vertex rawPredict shape: the client posts // the full Vertex model path and the proxy mints the SA OAuth token. WireVertex = "vertex" + // WireBedrock is the native AWS Bedrock InvokeModel shape: the model id + // travels in the URL path (/model/{id}/invoke), not the body, so the proxy + // routes by path. This is what a Bedrock SDK client sends and the shape the + // model-allowlist guardrail must enforce. + WireBedrock = "bedrock" ) // Chat issues a chat-completion POST to the agent-network endpoint over the @@ -226,6 +231,17 @@ func (cl *Client) Vertex(ctx context.Context, endpoint, proxyIP, project, region return cl.post(ctx, endpoint, proxyIP, path, body, withSessionID(nil, sessionID)) } +// Bedrock issues a native AWS Bedrock InvokeModel POST over the tunnel. The +// model id is carried in the request path (/model/{id}/invoke), so the proxy +// routes by path; the body uses the bedrock anthropic_version rather than a +// model field. A non-empty sessionID is sent as the universal x-session-id +// header the proxy records. +func (cl *Client) Bedrock(ctx context.Context, endpoint, proxyIP, model, prompt, sessionID string) (int, string, error) { + path := "/model/" + model + "/invoke" + body := fmt.Sprintf(`{"anthropic_version":"bedrock-2023-05-31","max_tokens":64,"messages":[{"role":"user","content":%q}]}`, prompt) + return cl.post(ctx, endpoint, proxyIP, path, body, withSessionID(nil, sessionID)) +} + // withSessionID appends the x-session-id header when sessionID is non-empty. func withSessionID(headers []string, sessionID string) []string { if sessionID == "" { diff --git a/proxy/internal/middleware/builtin/llm_guardrail/middleware.go b/proxy/internal/middleware/builtin/llm_guardrail/middleware.go index e6259f06f..eded877ac 100644 --- a/proxy/internal/middleware/builtin/llm_guardrail/middleware.go +++ b/proxy/internal/middleware/builtin/llm_guardrail/middleware.go @@ -25,6 +25,14 @@ const ( denyCodeModel = "llm_policy.model_blocked" denyReasonModel = "model_blocked" denyMessageModel = "model is not in the policy allowlist" + // Deny reason used when an allowlist is configured but the request model + // could not be determined. URL/path-routed providers (AWS Bedrock, Google + // Vertex, ...) carry the model outside the JSON body, so a request shape the + // parser does not recognise reaches the guardrail with no model. Such a + // request must be denied (fail closed), never waved through. + denyCodeModelUnknown = "llm_policy.model_unknown" + denyReasonModelUnknown = "model_unknown" + denyMessageModelUnknown = "request model could not be determined for the policy allowlist" ) // Middleware enforces the model allowlist and optionally captures the @@ -108,23 +116,37 @@ func (m *Middleware) evaluateAllowlist(model string, modelPresent bool) *middlew if len(m.cfg.ModelAllowlist) == 0 { return nil } - if !modelPresent { - return nil + // Fail closed: with an allowlist configured, a request whose model the + // upstream parser could not extract (absent or empty) must be denied rather + // than allowed. This is what enforces the allowlist for URL/path-routed + // providers (Bedrock, Vertex, ...) whose model lives outside the JSON body. + if !modelPresent || normaliseModel(model) == "" { + return denyModel("", denyCodeModelUnknown, denyMessageModelUnknown, denyReasonModelUnknown) } if m.modelInAllowlist(model) { return nil } + return denyModel(model, denyCodeModel, denyMessageModel, denyReasonModel) +} + +// denyModel builds a 403 deny Output for a model-allowlist rejection. model is +// included in the details only when non-empty. +func denyModel(model, code, message, reason string) *middleware.Output { + details := map[string]string{} + if model != "" { + details["model"] = model + } return &middleware.Output{ Decision: middleware.DecisionDeny, DenyStatus: 403, DenyReason: &middleware.DenyReason{ - Code: denyCodeModel, - Message: denyMessageModel, - Details: map[string]string{"model": model}, + Code: code, + Message: message, + Details: details, }, Metadata: []middleware.KV{ {Key: middleware.KeyLLMPolicyDecision, Value: "deny"}, - {Key: middleware.KeyLLMPolicyReason, Value: denyReasonModel}, + {Key: middleware.KeyLLMPolicyReason, Value: reason}, }, } } diff --git a/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go b/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go index 865dc07af..cd7e256dd 100644 --- a/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go +++ b/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go @@ -102,13 +102,44 @@ func TestAllowlistCaseInsensitive(t *testing.T) { } } -func TestAllowlistMissingModelKeyAllows(t *testing.T) { +func TestAllowlistMissingModelKeyDenies(t *testing.T) { + // Fail closed: with an allowlist configured, a request whose model the + // parser could not extract (URL/path-routed providers such as Bedrock or + // Vertex whose shape wasn't recognised) must be denied, not allowed. mw := New(Config{ModelAllowlist: []string{"gpt-4o"}}) out, err := mw.Invoke(context.Background(), newInput()) require.NoError(t, err) - assert.Equal(t, middleware.DecisionAllow, out.Decision, "missing model key must allow even with non-empty allowlist") + require.NotNil(t, out) + assert.Equal(t, middleware.DecisionDeny, out.Decision, "absent model must be denied when an allowlist is set") + assert.Equal(t, 403, out.DenyStatus, "deny status must be 403") + require.NotNil(t, out.DenyReason, "deny reason must be populated") + assert.Equal(t, "llm_policy.model_unknown", out.DenyReason.Code, "deny code must be model_unknown") dec, _ := metaValue(t, out.Metadata, middleware.KeyLLMPolicyDecision) - assert.Equal(t, "allow", dec, "decision must be allow when model key is absent") + assert.Equal(t, "deny", dec, "decision must be deny when model key is absent") + reason, _ := metaValue(t, out.Metadata, middleware.KeyLLMPolicyReason) + assert.Equal(t, "model_unknown", reason, "reason metadata must be model_unknown") +} + +func TestAllowlistEmptyModelValueDenies(t *testing.T) { + // A present-but-empty model is as undeterminable as an absent one. + mw := New(Config{ModelAllowlist: []string{"gpt-4o"}}) + out, err := mw.Invoke(context.Background(), newInput( + middleware.KV{Key: middleware.KeyLLMModel, Value: " "}, + )) + require.NoError(t, err) + require.NotNil(t, out) + assert.Equal(t, middleware.DecisionDeny, out.Decision, "empty model must be denied when an allowlist is set") + require.NotNil(t, out.DenyReason, "deny reason must be populated") + assert.Equal(t, "llm_policy.model_unknown", out.DenyReason.Code, "deny code must be model_unknown") +} + +func TestAllowlistEmptyListAllowsMissingModel(t *testing.T) { + // Without an allowlist there is nothing to enforce, so a missing model is + // still allowed — the fail-closed rule only applies when a list is set. + mw := New(Config{}) + out, err := mw.Invoke(context.Background(), newInput()) + require.NoError(t, err) + assert.Equal(t, middleware.DecisionAllow, out.Decision, "no allowlist must allow even without a model") } func TestPromptCaptureDisabledEmitsNoPrompt(t *testing.T) { diff --git a/proxy/internal/middleware/builtin/llm_request_parser/guardrail_allowlist_test.go b/proxy/internal/middleware/builtin/llm_request_parser/guardrail_allowlist_test.go new file mode 100644 index 000000000..0074411cc --- /dev/null +++ b/proxy/internal/middleware/builtin/llm_request_parser/guardrail_allowlist_test.go @@ -0,0 +1,106 @@ +package llm_request_parser + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/proxy/internal/middleware" + "github.com/netbirdio/netbird/proxy/internal/middleware/builtin/llm_guardrail" +) + +// runParserGuardrail runs the request parser then the model-allowlist guardrail +// in SlotOnRequest order, threading the parser's metadata into the guardrail the +// same way the real chain does. It returns the guardrail decision so tests can +// assert allowlist enforcement for URL/path-routed providers end to end. +func runParserGuardrail(t *testing.T, url string, body []byte, allowlist []string) *middleware.Output { + t.Helper() + parser := newMiddleware(t) + parsed, err := parser.Invoke(context.Background(), &middleware.Input{ + Slot: middleware.SlotOnRequest, + URL: url, + Body: body, + }) + require.NoError(t, err, "parser must not error") + + guard := llm_guardrail.New(llm_guardrail.Config{ModelAllowlist: allowlist}) + out, err := guard.Invoke(context.Background(), &middleware.Input{ + Slot: middleware.SlotOnRequest, + Metadata: parsed.Metadata, + }) + require.NoError(t, err, "guardrail must not error") + require.NotNil(t, out, "guardrail must return an output") + return out +} + +// TestModelAllowlist_URLRoutedProviders validates that the model allowlist is +// enforced for providers whose model travels in the URL path (AWS Bedrock, +// Google Vertex) rather than the JSON body. The "unknown action" case is the +// regression guard for #6751: a Bedrock request shape the parser cannot map to a +// model must fail closed under an allowlist instead of bypassing it. +func TestModelAllowlist_URLRoutedProviders(t *testing.T) { + const bedrockBody = `{"anthropic_version":"bedrock-2023-05-31","messages":[{"role":"user","content":"hi"}]}` + const vertexBody = `{"anthropic_version":"vertex-2023-10-16","messages":[{"role":"user","content":"hi"}]}` + + tests := []struct { + name string + url string + body string + allowlist []string + decision middleware.Decision + denyCode string + }{ + { + name: "bedrock allowed model passes", + url: "https://bedrock-runtime.us-east-1.amazonaws.com/model/us.anthropic.claude-haiku-4-5-v1:0/invoke", + body: bedrockBody, + allowlist: []string{"anthropic.claude-haiku-4-5"}, + decision: middleware.DecisionAllow, + }, + { + name: "bedrock disallowed model denied", + url: "https://bedrock-runtime.us-east-1.amazonaws.com/model/us.anthropic.claude-opus-4-8-v1:0/invoke", + body: bedrockBody, + allowlist: []string{"anthropic.claude-haiku-4-5"}, + decision: middleware.DecisionDeny, + denyCode: "llm_policy.model_blocked", + }, + { + name: "bedrock unknown action fails closed", + url: "https://bedrock-runtime.us-east-1.amazonaws.com/model/us.anthropic.claude-opus-4-8-v1:0/some-future-action", + body: bedrockBody, + allowlist: []string{"anthropic.claude-haiku-4-5"}, + decision: middleware.DecisionDeny, + denyCode: "llm_policy.model_unknown", + }, + { + name: "vertex disallowed model denied", + url: "/v1/projects/p/locations/global/publishers/anthropic/models/claude-opus-4-8@20250101:rawPredict", + body: vertexBody, + allowlist: []string{"claude-haiku-4-5"}, + decision: middleware.DecisionDeny, + denyCode: "llm_policy.model_blocked", + }, + { + name: "vertex allowed model passes", + url: "/v1/projects/p/locations/global/publishers/anthropic/models/claude-haiku-4-5@20250101:rawPredict", + body: vertexBody, + allowlist: []string{"claude-haiku-4-5"}, + decision: middleware.DecisionAllow, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := runParserGuardrail(t, tt.url, []byte(tt.body), tt.allowlist) + assert.Equal(t, tt.decision, out.Decision, "unexpected decision for %s", tt.name) + if tt.decision == middleware.DecisionDeny { + require.NotNil(t, out.DenyReason, "deny reason must be set for %s", tt.name) + assert.Equal(t, 403, out.DenyStatus, "deny status must be 403 for %s", tt.name) + assert.Equal(t, tt.denyCode, out.DenyReason.Code, "deny code for %s", tt.name) + } + }) + } +} From f0eed7564f3a9138962da1408986e4666d7137b5 Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Tue, 14 Jul 2026 20:13:00 +0200 Subject: [PATCH 07/19] [management] Remove proxy peer stale deduplication logic (#6768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes Removing a leftover from an initial implementation. We ended up resolving it on the client with status checks on the DNS response ## Issue ticket number and link ## Stack - \#6726 - \#6768 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: \_\_ ## Summary by CodeRabbit - **New Features** - Added support for Bedrock-native request routing in agent network scenarios. - Added guardrail management capabilities for creating and removing model allowlists. - **Bug Fixes** - Model allowlists now reject requests when the model is missing or blank. - Improved Rosenpass and WireGuard recovery after repeated handshake failures. - Improved relay connection handling so status and cleanup operations remain responsive during stalled connections. - Updated private service DNS zones to avoid unintended search-domain behavior. - **Tests** - Expanded coverage for model allowlists, handshake recovery, relay concurrency, and Bedrock routing. --- management/internals/modules/peers/manager.go | 50 ----- .../agentnetwork_proxypeer_restart_test.go | 199 ------------------ 2 files changed, 249 deletions(-) delete mode 100644 management/server/agentnetwork_proxypeer_restart_test.go diff --git a/management/internals/modules/peers/manager.go b/management/internals/modules/peers/manager.go index 239d6b09c..4ae9c2c82 100644 --- a/management/internals/modules/peers/manager.go +++ b/management/internals/modules/peers/manager.go @@ -224,30 +224,6 @@ func (m *managerImpl) CreateProxyPeer(ctx context.Context, accountID string, pee return nil } - // Dedupe stale embedded peer records for the same (account, cluster). - // The proxy generates a fresh WireGuard keypair on every startup - // (proxy/internal/roundtrip/netbird.go), so without this sweep the - // prior embedded peer would linger forever — holding its CGNAT IP - // allocation, polluting other peers' rosters, and (most visibly) - // leaving the synth DNS pointing at the dead address. The - // (account, cluster) tuple identifies "the embedded peer for this - // proxy instance at this cluster"; any record matching that tuple - // with a different pubkey is by definition stale and must go. - staleIDs, err := m.findStaleEmbeddedProxyPeers(ctx, accountID, cluster, peerKey) - if err != nil { - return fmt.Errorf("scan for stale embedded proxy peers: %w", err) - } - if len(staleIDs) > 0 { - // userID="" + checkConnected=false: the deletion is initiated - // by management itself on behalf of the freshly-registering - // proxy, not by an end user; the stale peer may still be - // marked Connected from its prior session, but its session is - // dead by definition (its key no longer exists). - if err := m.DeletePeers(ctx, accountID, staleIDs, "", false); err != nil { - return fmt.Errorf("delete stale embedded proxy peers %v: %w", staleIDs, err) - } - } - name := fmt.Sprintf("proxy-%s", xid.New().String()) newPeer := &peer.Peer{ Ephemeral: true, @@ -273,29 +249,3 @@ func (m *managerImpl) CreateProxyPeer(ctx context.Context, accountID string, pee return nil } - -// findStaleEmbeddedProxyPeers returns the peer IDs of embedded proxy peer -// records in accountID that target the same cluster but carry a different -// WireGuard pubkey than the freshly-registering one. Used by CreateProxyPeer -// to garbage-collect stale records left behind when the proxy restarts with a -// regenerated keypair. -func (m *managerImpl) findStaleEmbeddedProxyPeers(ctx context.Context, accountID, cluster, newKey string) ([]string, error) { - account, err := m.store.GetAccount(ctx, accountID) - if err != nil { - return nil, err - } - var stale []string - for _, p := range account.Peers { - if p == nil || !p.ProxyMeta.Embedded { - continue - } - if p.ProxyMeta.Cluster != cluster { - continue - } - if p.Key == newKey { - continue - } - stale = append(stale, p.ID) - } - return stale, nil -} diff --git a/management/server/agentnetwork_proxypeer_restart_test.go b/management/server/agentnetwork_proxypeer_restart_test.go deleted file mode 100644 index 1e4b8d016..000000000 --- a/management/server/agentnetwork_proxypeer_restart_test.go +++ /dev/null @@ -1,199 +0,0 @@ -package server - -import ( - "context" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/netbirdio/netbird/management/internals/modules/peers" - "github.com/netbirdio/netbird/management/internals/modules/agentnetwork" - agenttypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" - nbpeer "github.com/netbirdio/netbird/management/server/peer" - "github.com/netbirdio/netbird/management/server/permissions" - "github.com/netbirdio/netbird/management/server/store" - "github.com/netbirdio/netbird/management/server/types" -) - -// TestAgentNetwork_ProxyRestart_PropagatesNewPeerAndDropsStale is the no-mock -// regression guard for the bug the user reported: restarting the proxy creates -// a fresh embedded peer with a NEW WireGuard public key (the proxy generates -// the keypair on every startup at proxy/internal/roundtrip/netbird.go:312). -// The PRIOR embedded peer record is never deleted on management, so the -// account accumulates a stale peer holding a stale CGNAT IP. Other peers -// in the account either keep routing to the dead IP, or — if synth DNS -// picks the wrong record — never see the new IP at all. -// -// What this test exercises (no mocks): -// - real SQLite test store -// - real DefaultAccountManager, network-map controller, peer-update channels -// - real peers.Manager.CreateProxyPeer path (the very method the proxy -// invokes over gRPC on every startup) -// - real agentnetwork.Manager + synth chain so the client receives a -// concrete DNS record that must point at the LATEST proxy peer. -// -// Pre-fix expected behavior (red): two embedded peers exist after the -// "restart"; the synth DNS record points at the stale one; the client -// receives an update reflecting the new peer but the old one lingers. -// Post-fix expected behavior (green): exactly one embedded peer exists -// after restart (with the new key) AND the client's network map carries -// the synth DNS pointing at that new peer's CGNAT IP. -func TestAgentNetwork_ProxyRestart_PropagatesNewPeerAndDropsStale(t *testing.T) { - am, updateManager, err := createManager(t) - require.NoError(t, err, "createManager must succeed") - ctx := context.Background() - - const ( - accountID = "an-restart-acct" - adminUserID = "an-restart-admin" - groupAID = "an-restart-grp-A" - clusterAddr = "eu.proxy.netbird.io" - clientKey = "BhRPtynAAYRDy08+q4HTMsos8fs4plTP4NOSh7C1ry8=" - // Two different proxy pubkeys — the "before" and "after" of a - // proxy-process restart with fresh-keypair generation. - proxyKey1 = "Aaaaa1aaaaYRDy08+q4HTMsos8fs4plTP4NOSh7C1ry8=" - proxyKey2 = "Bbbbb2bbbbYRDy08+q4HTMsos8fs4plTP4NOSh7C1ry8=" - ) - - // --- Account scaffold --- - account := newAccountWithId(ctx, accountID, adminUserID, "an-restart.test", "", "", false) - require.NoError(t, am.Store.SaveAccount(ctx, account)) - - clientPeer := &nbpeer.Peer{ - Key: clientKey, - Name: "an-restart-client", - DNSLabel: "an-restart-client", - Meta: nbpeer.PeerSystemMeta{Hostname: "an-restart-client", GoOS: "linux", WtVersion: "development"}, - } - addedClient, _, _, _, err := am.AddPeer(ctx, "", "", adminUserID, clientPeer, false) - require.NoError(t, err, "AddPeer for client must succeed") - require.NoError(t, am.MarkPeerConnected(ctx, clientKey, accountID, time.Now().UnixNano(), &types.NetworkMap{}), - "MarkPeerConnected for the client peer must succeed (affected-peer fan-out skips disconnected peers)") - - // Place the client in group A so the synth policy reaches it. - account, err = am.Store.GetAccount(ctx, accountID) - require.NoError(t, err) - account.Groups[groupAID] = &types.Group{ID: groupAID, Name: "groupA", Peers: []string{addedClient.ID}} - require.NoError(t, am.Store.SaveAccount(ctx, account), "SaveAccount must persist group A") - - // --- Real peers + agent-network managers --- - permMgr := permissions.NewManager(am.Store) - peersMgr := peers.NewManager(am.Store, permMgr) - peersMgr.SetAccountManager(am) - peersMgr.SetNetworkMapController(am.networkMapController) - agentMgr := agentnetwork.NewManager(am.Store, permMgr, am, nil) - - // Subscribe BEFORE any state-mutating call so we don't lose the update - // that contains the synth DNS record. - clientCh := updateManager.CreateChannel(ctx, addedClient.ID) - t.Cleanup(func() { updateManager.CloseChannel(ctx, addedClient.ID) }) - drain(clientCh) - - // --- First proxy startup: register peer key K1, then mark it - // connected. In production the proxy follows CreateProxyPeer with the - // regular sync stream which lands on MarkPeerConnected; the synth DNS - // path filters out peers that aren't Connected (types/account.go:323), - // so without this step no DNS record would be emitted. - require.NoError(t, peersMgr.CreateProxyPeer(ctx, accountID, proxyKey1, clusterAddr), - "first CreateProxyPeer (proxy startup) must succeed") - - peer1ID, err := am.Store.GetPeerIDByKey(ctx, store.LockingStrengthNone, proxyKey1) - require.NoError(t, err, "proxy peer for K1 must be persisted after CreateProxyPeer") - require.NotEmpty(t, peer1ID) - - require.NoError(t, am.MarkPeerConnected(ctx, proxyKey1, accountID, time.Now().UnixNano(), &types.NetworkMap{}), - "MarkPeerConnected for K1 must succeed") - - account, err = am.Store.GetAccount(ctx, accountID) - require.NoError(t, err) - proxyIP1 := account.Peers[peer1ID].IP.String() - require.NotEmpty(t, proxyIP1, "K1 must have an assigned overlay IP") - - // --- Provider + policy. CreateProvider / CreatePolicy trigger the - // agentnetwork reconcile which runs UpdateAccountPeers; the resulting - // NetworkMap delivered to the client carries the synth DNS record - // pointing at K1's IP. --- - provider, err := agentMgr.CreateProvider(ctx, adminUserID, &agenttypes.Provider{ - AccountID: accountID, - ProviderID: "openai_api", - Name: "openai-test", - UpstreamURL: "https://api.openai.com", - APIKey: "sk-test-key", - Enabled: true, - Models: []agenttypes.ProviderModel{{ID: "gpt-5.4"}}, - }, clusterAddr) - require.NoError(t, err, "CreateProvider must succeed") - - _, err = agentMgr.CreatePolicy(ctx, adminUserID, &agenttypes.Policy{ - AccountID: accountID, - Name: "p1", - Enabled: true, - SourceGroups: []string{groupAID}, - DestinationProviderIDs: []string{provider.ID}, - }) - require.NoError(t, err, "CreatePolicy must succeed") - - settings, err := am.Store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID) - require.NoError(t, err) - fqdn := settings.Endpoint() - - rdata1 := awaitZoneRData(clientCh, clusterAddr, fqdn, true) - require.Equal(t, proxyIP1, rdata1, - "client must receive a synth DNS record pointing at K1's overlay IP after the synth path runs") - drain(clientCh) - - // --- Proxy restart: NEW keypair K2, same account, same cluster --- - require.NoError(t, peersMgr.CreateProxyPeer(ctx, accountID, proxyKey2, clusterAddr), - "second CreateProxyPeer (proxy restart with fresh keypair) must succeed") - - peer2ID, err := am.Store.GetPeerIDByKey(ctx, store.LockingStrengthNone, proxyKey2) - require.NoError(t, err, "proxy peer for K2 must be persisted after restart") - require.NotEmpty(t, peer2ID) - - require.NoError(t, am.MarkPeerConnected(ctx, proxyKey2, accountID, time.Now().UnixNano(), &types.NetworkMap{}), - "MarkPeerConnected for K2 must succeed") - - // In production the agent's sync stream pulls a fresh NetworkMap as - // part of its normal reconcile cadence; in this isolated test - // MarkPeerConnected's affected-peer fan-out can race the channel-side - // buffer in a way that swallows the synth-DNS-bearing update before - // our await reads it. Trigger an explicit account-wide fan-out so the - // assertion below tests what production actually delivers, not the - // in-test buffer race. - am.UpdateAccountPeers(ctx, accountID, types.UpdateReason{Resource: types.UpdateResourcePeer, Operation: types.UpdateOperationUpdate}) - - account, err = am.Store.GetAccount(ctx, accountID) - require.NoError(t, err) - proxyIP2 := account.Peers[peer2ID].IP.String() - require.NotEmpty(t, proxyIP2, "K2 must have an assigned overlay IP") - require.NotEqual(t, proxyIP1, proxyIP2, "K2 must get a different overlay IP than K1 (sanity)") - - // CRITICAL ASSERTION 1: K1 must no longer be in the store. The SqlStore - // returns ("", nil) for a missing key rather than NotFound, so assert - // on the returned ID being empty. - staleID, err := am.Store.GetPeerIDByKey(ctx, store.LockingStrengthNone, proxyKey1) - require.NoError(t, err, "GetPeerIDByKey for a missing peer must not error") - assert.Empty(t, staleID, - "stale embedded proxy peer K1 must be removed when a new embedded peer registers for the same (account, cluster); pre-fix this assertion fails because management never cleans up the prior peer record") - - // CRITICAL ASSERTION 2: exactly one embedded proxy peer remains, and it - // is K2. - account, err = am.Store.GetAccount(ctx, accountID) - require.NoError(t, err) - embeddedKeys := []string{} - for _, p := range account.Peers { - if p.ProxyMeta.Embedded { - embeddedKeys = append(embeddedKeys, p.Key) - } - } - assert.Equal(t, []string{proxyKey2}, embeddedKeys, - "after a proxy restart exactly one embedded proxy peer should remain — the one with the new key K2") - - // CRITICAL ASSERTION 3: the synth DNS record the client receives now - // points at K2's IP, not K1's. - rdata2 := awaitZoneRData(clientCh, clusterAddr, fqdn, true) - assert.Equal(t, proxyIP2, rdata2, - "after proxy restart, the client's synth DNS record must point at the NEW embedded peer's IP, not the stale K1 IP") -} From 3a2f773d655d88d16ed953fc2a114a4e690a1b08 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Wed, 15 Jul 2026 12:04:04 +0200 Subject: [PATCH 08/19] [client] preserve WireGuard key on interactive re-login (#6777) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NewAuth built a fresh in-memory config on every call via CreateInMemoryConfig, which generates a new WireGuard private key when none is set. The iOS Swift layer calls this on interactive re-login and writes the resulting config back to the profile's netbird.cfg, so each re-auth replaced the peer's persisted private key with a new one. A new key means a new public key, so the management server registered a brand-new peer on every re-authentication — named after the fallback hostname. Load the existing config with DirectUpdateOrCreateConfig when a config file is already present so re-login reuses the peer's persisted private key (and its identity). Only fall back to a fresh in-memory config for the first-time login when no config file exists yet (or after logout, which deletes the file). DirectUpdateOrCreateConfig uses non-atomic writes so it also works inside the tvOS App Group sandbox. This matches what Run() and LoginForMobile() already do. ## Describe your changes ## Issue ticket number and link ## Stack ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ ## Summary by CodeRabbit * **New Features** * Added support for loading or creating persistent configuration when a configuration file path is provided. * Continued support for in-memory configuration for temporary or first-time use. --- client/ios/NetBirdSDK/login.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/client/ios/NetBirdSDK/login.go b/client/ios/NetBirdSDK/login.go index 432133999..99486839b 100644 --- a/client/ios/NetBirdSDK/login.go +++ b/client/ios/NetBirdSDK/login.go @@ -44,10 +44,25 @@ type Auth struct { // NewAuth instantiate Auth struct and validate the management URL func NewAuth(cfgPath string, mgmURL string) (*Auth, error) { inputCfg := profilemanager.ConfigInput{ + ConfigPath: cfgPath, ManagementURL: mgmURL, } - cfg, err := profilemanager.CreateInMemoryConfig(inputCfg) + // Load the existing config when a config file is already present so an + // interactive re-login reuses the peer's persisted WireGuard private key + // (and thus its identity) instead of generating a fresh one. Generating a + // new key registers a brand-new peer on the management server on every + // re-auth (named after the fallback hostname). Only fall back to a fresh + // in-memory config for the first-time login when no config file exists yet. + // DirectUpdateOrCreateConfig uses non-atomic writes so it also works inside + // the tvOS App Group sandbox where atomic temp-file+rename is blocked. + var cfg *profilemanager.Config + var err error + if cfgPath != "" { + cfg, err = profilemanager.DirectUpdateOrCreateConfig(inputCfg) + } else { + cfg, err = profilemanager.CreateInMemoryConfig(inputCfg) + } if err != nil { return nil, err } From 62fc8d254e636c3053ae8a21c4ea075558a8273f Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Thu, 16 Jul 2026 01:18:47 +0900 Subject: [PATCH 09/19] [relay] Handle QUIC connections concurrently to prevent handshake head-of-line blocking (#6784) --- relay/server/listener/quic/listener.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/relay/server/listener/quic/listener.go b/relay/server/listener/quic/listener.go index 68f0e03c0..4c3b07571 100644 --- a/relay/server/listener/quic/listener.go +++ b/relay/server/listener/quic/listener.go @@ -51,7 +51,10 @@ func (l *Listener) Listen(acceptFn func(conn relaylistener.Conn)) error { log.Infof("QUIC client connected from: %s", session.RemoteAddr()) conn := NewConn(session) - acceptFn(conn) + // Run the accept handler (which performs the pre-auth handshake) in its + // own goroutine so a slow or stalled handshake cannot block accepting + // further connections. + go acceptFn(conn) } } From 63d60ba490794eebd0ad5ce77e4d31269e9c793b Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Fri, 17 Jul 2026 16:28:33 +0900 Subject: [PATCH 10/19] [client] Reject leading hyphen in getent input to prevent flag injection (#6787) --- client/ssh/server/getent_unix.go | 7 ++++++- client/ssh/server/getent_unix_test.go | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/client/ssh/server/getent_unix.go b/client/ssh/server/getent_unix.go index 18edb2fdf..a3a9641f8 100644 --- a/client/ssh/server/getent_unix.go +++ b/client/ssh/server/getent_unix.go @@ -69,7 +69,8 @@ func parseGetentPasswd(output string) (*user.User, string, error) { // validateGetentInput checks that the input is safe to pass to getent or id. // Allows POSIX usernames, numeric UIDs, and common NSS extensions -// (@ for Kerberos, $ for Samba, + for NIS compat). +// (@ for Kerberos, $ for Samba, + for NIS compat). A leading hyphen is +// rejected so the input can never be parsed as a command-line flag. func validateGetentInput(input string) bool { maxLen := 32 if runtime.GOOS == "linux" { @@ -80,6 +81,10 @@ func validateGetentInput(input string) bool { return false } + if input[0] == '-' { + return false + } + for _, r := range input { if isAllowedGetentChar(r) { continue diff --git a/client/ssh/server/getent_unix_test.go b/client/ssh/server/getent_unix_test.go index e44563b79..a73214e17 100644 --- a/client/ssh/server/getent_unix_test.go +++ b/client/ssh/server/getent_unix_test.go @@ -157,6 +157,9 @@ func TestValidateGetentInput(t *testing.T) { {"numeric UID", "1001", true}, {"dots and underscores", "alice.bob_test", true}, {"hyphen", "alice-bob", true}, + {"leading hyphen rejected", "-i", false}, + {"leading double hyphen rejected", "--no-idn", false}, + {"lone hyphen rejected", "-", false}, {"kerberos principal", "user@REALM", true}, {"samba machine account", "MACHINE$", true}, {"NIS compat", "+user", true}, From 099ae4bc6cc8ab95ef16343acb87c33b8197711c Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:10:22 +0200 Subject: [PATCH 11/19] [client] Sanitize peer FQDN/hostname in generated SSH config (#6805) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes Validate peer-supplied FQDN and hostname before they are written into the generated NetBird SSH client config (`client/ssh/config/manager.go`). These values originate from remote peers and were previously written verbatim into the config; malformed values (e.g. containing unexpected characters) could produce a broken or unintended config. FQDN/hostname are now checked with `domain.IsValidDomainNoWildcard`, and invalid, non-empty values are dropped with a warning. IPs are unaffected (already validated `netip.Addr`). Added a test covering malformed hostnames. ## Issue ticket number and link Internal input-validation hardening for peer-supplied hostnames in the generated SSH client config (`client/ssh/config/manager.go`). ## Stack - \#6726 - \#6805 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client SSH config generation. No public API, gRPC, CLI/service flag, or configuration change — only input validation on peer-supplied hostnames before they are written to the generated ssh\_config. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: N/A --- client/ssh/config/manager.go | 14 +++++++++-- client/ssh/config/manager_test.go | 39 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/client/ssh/config/manager.go b/client/ssh/config/manager.go index 20695cb4d..e15330739 100644 --- a/client/ssh/config/manager.go +++ b/client/ssh/config/manager.go @@ -14,6 +14,7 @@ import ( log "github.com/sirupsen/logrus" nbssh "github.com/netbirdio/netbird/client/ssh" + "github.com/netbirdio/netbird/shared/management/domain" ) const ( @@ -218,11 +219,20 @@ func (m *Manager) buildHostPatterns(peer PeerSSHInfo) []string { if peer.IPv6.IsValid() { hostPatterns = append(hostPatterns, peer.IPv6.String()) } - if peer.FQDN != "" { + // Peer FQDNs and hostnames originate from remote peers, so they must be + // validated as plain DNS names before being embedded in the ssh_config + // "Match host" pattern list. This prevents injection of arbitrary + // ssh_config directives via embedded quotes, whitespace, newlines, the + // comma pattern separator, or the "*"/"?" pattern metacharacters. + if domain.IsValidDomainNoWildcard(peer.FQDN) { hostPatterns = append(hostPatterns, peer.FQDN) + } else if peer.FQDN != "" { + log.Warnf("skipping peer FQDN with invalid characters in SSH config: %q", peer.FQDN) } - if peer.Hostname != "" && peer.Hostname != peer.FQDN { + if peer.Hostname != peer.FQDN && domain.IsValidDomainNoWildcard(peer.Hostname) { hostPatterns = append(hostPatterns, peer.Hostname) + } else if peer.Hostname != "" && peer.Hostname != peer.FQDN { + log.Warnf("skipping peer hostname with invalid characters in SSH config: %q", peer.Hostname) } return hostPatterns } diff --git a/client/ssh/config/manager_test.go b/client/ssh/config/manager_test.go index 8e6be40a3..f65d0ba6d 100644 --- a/client/ssh/config/manager_test.go +++ b/client/ssh/config/manager_test.go @@ -148,6 +148,45 @@ func TestManager_MatchHostFormat(t *testing.T) { "should use Match host with comma-separated patterns") } +func TestManager_HostPatternInjection(t *testing.T) { + tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test") + require.NoError(t, err) + defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }() + + manager := &Manager{ + sshConfigDir: filepath.Join(tempDir, "ssh_config.d"), + sshConfigFile: "99-netbird.conf", + } + + // A malicious peer FQDN/hostname attempts to break out of the Match host + // directive and inject arbitrary ssh_config (a ProxyCommand executing a + // command). It must be rejected, not written to the config. + peers := []PeerSSHInfo{ + { + Hostname: "evil\"\n ProxyCommand touch /tmp/pwned\nHost x", + IP: netip.MustParseAddr("100.125.1.1"), + FQDN: "evil\"\n ProxyCommand touch /tmp/pwned\nHost x.nb.internal", + }, + {Hostname: "peer2", IP: netip.MustParseAddr("100.125.1.2"), FQDN: "peer2.nb.internal"}, + } + + err = manager.SetupSSHClientConfig(peers) + require.NoError(t, err) + + configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile) + content, err := os.ReadFile(configPath) + require.NoError(t, err) + configStr := string(content) + + assert.NotContains(t, configStr, "ProxyCommand touch /tmp/pwned", + "injected directive must not appear in generated config") + assert.NotContains(t, configStr, "evil", + "malicious pattern must be dropped entirely") + // The valid peer must still be present, on a single Match host line. + assert.Contains(t, configStr, "Match host \"100.125.1.1,100.125.1.2,peer2.nb.internal,peer2\"", + "valid peers must survive, injected patterns dropped") +} + func TestManager_ForcedSSHConfig(t *testing.T) { // Set force environment variable t.Setenv(EnvForceSSHConfig, "true") From 6e3f4d8722d1c3f4482c44aec725aaba80c4512c Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:45:30 +0900 Subject: [PATCH 12/19] [client] Disable gVisor TCP RACK loss detection on Windows (#6808) --- .../firewall/uspfilter/forwarder/forwarder.go | 37 +++++++++++++++++++ go.mod | 2 +- go.sum | 4 +- 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/client/firewall/uspfilter/forwarder/forwarder.go b/client/firewall/uspfilter/forwarder/forwarder.go index 6291eb285..28320ad88 100644 --- a/client/firewall/uspfilter/forwarder/forwarder.go +++ b/client/firewall/uspfilter/forwarder/forwarder.go @@ -5,7 +5,9 @@ import ( "fmt" "net" "net/netip" + "os" "runtime" + "strconv" "sync" "time" @@ -31,6 +33,11 @@ const ( defaultMaxInFlight = 1024 iosReceiveWindow = 16384 iosMaxInFlight = 256 + + // envForceTCPRACK overrides the platform default for gVisor's RACK loss + // detection. Set to a truthy value to force RACK on, or a falsy value to + // force it off, on any platform. + envForceTCPRACK = "NB_FORCE_TCP_RACK" ) type Forwarder struct { @@ -152,6 +159,8 @@ func New(iface common.IFaceMapper, logger *nblog.Logger, flowLogger nftypes.Flow maxInFlight = iosMaxInFlight } + configureTCPRecovery(s) + tcpForwarder := tcp.NewForwarder(s, receiveWindow, maxInFlight, f.handleTCP) s.SetTransportProtocolHandler(tcp.ProtocolNumber, tcpForwarder.HandlePacket) @@ -466,3 +475,31 @@ func probeRawICMP(network, addr string, logger *nblog.Logger) bool { logger.Debug1("forwarder: raw %s socket access available", network) return true } + +// configureTCPRecovery disables gVisor's RACK loss detection on Windows, where +// it interacts poorly with the host and collapses throughput on routed TCP +// connections (gVisor issue #9778). Other platforms keep the default. The +// EnvForceTCPRACK environment variable overrides the platform default. +func configureTCPRecovery(s *stack.Stack) { + disableRACK := runtime.GOOS == "windows" + + if val := os.Getenv(envForceTCPRACK); val != "" { + force, err := strconv.ParseBool(val) + if err != nil { + log.Warnf("parse %s: %v", envForceTCPRACK, err) + } else { + disableRACK = !force + } + } + + if !disableRACK { + return + } + + opt := tcpip.TCPRecovery(0) + if err := s.SetTransportProtocolOption(tcp.ProtocolNumber, &opt); err != nil { + log.Warnf("disable TCP RACK loss detection: %v", err) + return + } + log.Info("forwarder: TCP RACK loss detection disabled") +} diff --git a/go.mod b/go.mod index 524068aaf..413c33697 100644 --- a/go.mod +++ b/go.mod @@ -346,7 +346,7 @@ replace github.com/kardianos/service => github.com/netbirdio/service v0.0.0-2024 replace github.com/getlantern/systray => github.com/netbirdio/systray v0.0.0-20231030152038-ef1ed2a27949 -replace golang.zx2c4.com/wireguard => github.com/netbirdio/wireguard-go v0.0.0-20260628102922-2834bebf6c1a +replace golang.zx2c4.com/wireguard => github.com/netbirdio/wireguard-go v0.0.0-20260717071248-8ec1ad32882f replace github.com/cloudflare/circl => codeberg.org/cunicu/circl v0.0.0-20230801113412-fec58fc7b5f6 diff --git a/go.sum b/go.sum index 561416e8a..19d5a2338 100644 --- a/go.sum +++ b/go.sum @@ -518,8 +518,8 @@ github.com/netbirdio/service v0.0.0-20240911161631-f62744f42502 h1:3tHlFmhTdX9ax github.com/netbirdio/service v0.0.0-20240911161631-f62744f42502/go.mod h1:CIMRFEJVL+0DS1a3Nx06NaMn4Dz63Ng6O7dl0qH0zVM= github.com/netbirdio/signal-dispatcher/dispatcher v0.0.0-20250805121659-6b4ac470ca45 h1:ujgviVYmx243Ksy7NdSwrdGPSRNE3pb8kEDSpH0QuAQ= github.com/netbirdio/signal-dispatcher/dispatcher v0.0.0-20250805121659-6b4ac470ca45/go.mod h1:5/sjFmLb8O96B5737VCqhHyGRzNFIaN/Bu7ZodXc3qQ= -github.com/netbirdio/wireguard-go v0.0.0-20260628102922-2834bebf6c1a h1:3CWK+yTvRKOcC0Q8VCTGy4l60TEb27CQVS7LkMxwjmw= -github.com/netbirdio/wireguard-go v0.0.0-20260628102922-2834bebf6c1a/go.mod h1:rpwXGsirqLqN2L0JDJQlwOboGHmptD5ZD6T2VmcqhTw= +github.com/netbirdio/wireguard-go v0.0.0-20260717071248-8ec1ad32882f h1:yRb7dsTh5BXYiVoQE1MMni62TcRjlJPA82QFoRcXIWg= +github.com/netbirdio/wireguard-go v0.0.0-20260717071248-8ec1ad32882f/go.mod h1:rpwXGsirqLqN2L0JDJQlwOboGHmptD5ZD6T2VmcqhTw= github.com/nfnt/resize v0.0.0-20180221191011-83c6a9932646 h1:zYyBkD/k9seD2A7fsi6Oo2LfFZAehjjQMERAvZLEDnQ= github.com/nfnt/resize v0.0.0-20180221191011-83c6a9932646/go.mod h1:jpp1/29i3P1S/RLdc7JQKbRpFeM1dOBd8T9ki5s+AY8= github.com/nicksnyder/go-i18n/v2 v2.5.1 h1:IxtPxYsR9Gp60cGXjfuR/llTqV8aYMsC472zD0D1vHk= From 3f8c4473783424e1642d2991b896add973249d97 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 17 Jul 2026 11:00:55 +0200 Subject: [PATCH 13/19] [client] Rename isValidAccessToken to reflect audience-only check (#6806) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes `isValidAccessToken` only decodes the JWT payload and checks the audience claim, but its name suggested full token validation. Rename it to `validateTokenAudience` and document what it does: a client-side audience/shape check on a token just obtained from the IdP over TLS. Token authenticity is enforced server-side by the management server, which verifies the signature against the IdP's JWKS (`shared/auth/jwt/validator.go`) on every request. Also harden the parser: a non-empty token lacking the three-part JWT structure caused an index-out-of-range panic (`strings.Split(token, ".")[1]`); the shape is now validated first. `parseEmailFromIDToken` is documented as best-effort UX data (login hint/display), never used for authorization. Added tests for audience matching, malformed tokens, and the panic regression. Changes: - Rename `isValidAccessToken` → `validateTokenAudience`; document that it does not verify the signature and that authenticity is enforced server-side. - Fix an index-out-of-range panic on a non-empty token lacking JWT structure (`strings.Split(token, ".")[1]`) by validating the three-part shape first. - Document `parseEmailFromIDToken` as best-effort/unverified, used only for the login-hint/display UX, never for an authorization decision. - Add `util_test.go` covering audience matching (string and array), missing audience, malformed payloads, and the panic regression. ## Issue ticket number and link Internal cleanup: rename a misleadingly-named client-side helper and harden JWT parsing against malformed input (`client/internal/auth/util.go`). ## Stack - `0.74.7-branch` - :warning: No PR associated with branch - \#6806 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client-side helper rename plus a panic hardening fix. No public API, gRPC, CLI/service flag, or configuration change; token authenticity enforcement (server-side JWKS verification) is unchanged. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: N/A *** View with Codesmith Autofix with Codesmith Need help on this PR? Tag /codesmith with what you need. Autofix is disabled. ## Summary by CodeRabbit - **Bug Fixes** - Improved access-token audience validation during device and PKCE authentication flows. - Malformed tokens now return clear validation errors instead of risking runtime failures. - Added support for validating both string and array audience claims. - **Tests** - Added coverage for malformed tokens, invalid claims, missing audiences, and panic prevention. --- client/internal/auth/device_flow.go | 2 +- client/internal/auth/pkce_flow.go | 7 +- client/internal/auth/util.go | 20 ++++-- client/internal/auth/util_test.go | 108 ++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 6 deletions(-) create mode 100644 client/internal/auth/util_test.go diff --git a/client/internal/auth/device_flow.go b/client/internal/auth/device_flow.go index e33765300..cf8b7a1f8 100644 --- a/client/internal/auth/device_flow.go +++ b/client/internal/auth/device_flow.go @@ -291,7 +291,7 @@ func (d *DeviceAuthorizationFlow) WaitToken(ctx context.Context, info AuthFlowIn UseIDToken: d.providerConfig.UseIDToken, } - err = isValidAccessToken(tokenInfo.GetTokenToUse(), d.providerConfig.Audience) + err = validateTokenAudience(tokenInfo.GetTokenToUse(), d.providerConfig.Audience) if err != nil { return TokenInfo{}, fmt.Errorf("validate access token failed with error: %v", err) } diff --git a/client/internal/auth/pkce_flow.go b/client/internal/auth/pkce_flow.go index 84fa8a214..91d6733ea 100644 --- a/client/internal/auth/pkce_flow.go +++ b/client/internal/auth/pkce_flow.go @@ -296,7 +296,7 @@ func (p *PKCEAuthorizationFlow) parseOAuthToken(token *oauth2.Token) (TokenInfo, audience = p.providerConfig.ClientID } - if err := isValidAccessToken(tokenInfo.GetTokenToUse(), audience); err != nil { + if err := validateTokenAudience(tokenInfo.GetTokenToUse(), audience); err != nil { return TokenInfo{}, fmt.Errorf("authentication failed: invalid access token - %w", err) } @@ -310,6 +310,11 @@ func (p *PKCEAuthorizationFlow) parseOAuthToken(token *oauth2.Token) (TokenInfo, return tokenInfo, nil } +// parseEmailFromIDToken extracts the email (or name) claim from an ID token +// without verifying its signature. The value is best-effort and used only as a +// UX convenience (login hint prefill and display); it never drives an +// authorization decision. The authoritative identity is established server-side +// from the signature-verified token. func parseEmailFromIDToken(token string) (string, error) { parts := strings.Split(token, ".") if len(parts) < 2 { diff --git a/client/internal/auth/util.go b/client/internal/auth/util.go index 31c81d701..1800584a2 100644 --- a/client/internal/auth/util.go +++ b/client/internal/auth/util.go @@ -20,14 +20,26 @@ func randomBytesInHex(count int) (string, error) { return hex.EncodeToString(buf), nil } -// isValidAccessToken is a simple validation of the access token -func isValidAccessToken(token string, audience string) error { +// validateTokenAudience checks that the token is a well-formed JWT whose +// audience claim matches the expected audience. +// +// It does NOT verify the token's cryptographic signature and therefore must not +// be treated as an authenticity check. The token is obtained by the client +// directly from the IdP token endpoint over TLS, and its signature is verified +// server-side by the management server against the IdP's JWKS +// (see shared/auth/jwt/validator.go). This function is only a client-side +// sanity check that the returned token targets the expected audience. +func validateTokenAudience(token string, audience string) error { if token == "" { return fmt.Errorf("token received is empty") } - encodedClaims := strings.Split(token, ".")[1] - claimsString, err := base64.RawURLEncoding.DecodeString(encodedClaims) + parts := strings.Split(token, ".") + if len(parts) != 3 { + return fmt.Errorf("token is not a well-formed JWT") + } + + claimsString, err := base64.RawURLEncoding.DecodeString(parts[1]) if err != nil { return err } diff --git a/client/internal/auth/util_test.go b/client/internal/auth/util_test.go new file mode 100644 index 000000000..7f225bb86 --- /dev/null +++ b/client/internal/auth/util_test.go @@ -0,0 +1,108 @@ +package auth + +import ( + "encoding/base64" + "encoding/json" + "testing" +) + +// makeJWT builds an unsigned JWT-shaped string (header.payload.signature) with +// the given claims payload. The signature part is arbitrary because +// validateTokenAudience intentionally does not verify it. +func makeJWT(t *testing.T, claims map[string]interface{}) string { + t.Helper() + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"RS256","typ":"JWT"}`)) + payloadBytes, err := json.Marshal(claims) + if err != nil { + t.Fatalf("marshal claims: %v", err) + } + payload := base64.RawURLEncoding.EncodeToString(payloadBytes) + return header + "." + payload + ".unverified-signature" +} + +func TestValidateTokenAudience(t *testing.T) { + tests := []struct { + name string + token string + audience string + wantErr bool + }{ + { + name: "empty token", + token: "", + audience: "netbird", + wantErr: true, + }, + { + name: "not a JWT - no dots", + token: "notajwt", + audience: "netbird", + wantErr: true, + }, + { + name: "not a JWT - two parts only", + token: "header.payload", + audience: "netbird", + wantErr: true, + }, + { + name: "matching string audience", + token: makeJWT(t, map[string]interface{}{"aud": "netbird"}), + audience: "netbird", + wantErr: false, + }, + { + name: "mismatching string audience", + token: makeJWT(t, map[string]interface{}{"aud": "other"}), + audience: "netbird", + wantErr: true, + }, + { + name: "matching audience in array", + token: makeJWT(t, map[string]interface{}{"aud": []interface{}{"other", "netbird"}}), + audience: "netbird", + wantErr: false, + }, + { + name: "mismatching audience array", + token: makeJWT(t, map[string]interface{}{"aud": []interface{}{"a", "b"}}), + audience: "netbird", + wantErr: true, + }, + { + name: "missing audience claim", + token: makeJWT(t, map[string]interface{}{"sub": "user"}), + audience: "netbird", + wantErr: true, + }, + { + name: "invalid base64 payload", + token: "header.!!!not-base64!!!.sig", + audience: "netbird", + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := validateTokenAudience(tc.token, tc.audience) + if tc.wantErr && err == nil { + t.Fatalf("expected error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("expected no error, got %v", err) + } + }) + } +} + +// TestValidateTokenAudienceNoPanic guards the regression where a non-empty +// token without the JWT dot structure caused an index-out-of-range panic. +func TestValidateTokenAudienceNoPanic(t *testing.T) { + inputs := []string{"a", ".", "a.", "aaaa", "no-dots-here"} + for _, in := range inputs { + if err := validateTokenAudience(in, "netbird"); err == nil { + t.Fatalf("expected error for malformed token %q, got nil", in) + } + } +} From 9906b9b1a1726cd093078db46c260d6aef93b681 Mon Sep 17 00:00:00 2001 From: dmitri-netbird Date: Fri, 17 Jul 2026 11:21:11 +0200 Subject: [PATCH 14/19] [management] fix a flake in account_test (#6811) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes In "TestDefaultAccountManager_UpdateAccountSettings_NetworkRangePreserved", in the beginning of the test, during account creation a random /16 subnet from 10.64.0./10 network is used. Later in the test a new range (10.99.0.0/16) is assigned to the account, but it's one of the possible subnets used during account creation, which sometimes leads to a collision and failed test. Using a network outside of the range of networks used during account creation fixes the issue. ## Issue ticket number and link ## Stack ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ --- View with Codesmith Autofix with Codesmith Need help on this PR? Tag /codesmith with what you need. Autofix is disabled. ## Summary by CodeRabbit * **Tests** * Updated account network range test coverage to verify peer IP reallocation with a distinct network range. Signed-off-by: Dmitri Dolguikh --- management/server/account_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/account_test.go b/management/server/account_test.go index 585e267d5..ee910630a 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -4269,7 +4269,7 @@ func TestDefaultAccountManager_UpdateAccountSettings_NetworkRangePreserved(t *te } // Sanity: an actually different range still triggers reallocation. - newRange := netip.MustParsePrefix("100.99.0.0/16") + newRange := netip.MustParsePrefix("100.60.0.0/16") _, err = manager.UpdateAccountSettings(ctx, account.Id, userID, &types.Settings{ PeerLoginExpirationEnabled: true, PeerLoginExpiration: types.DefaultPeerLoginExpiration, From 21fc5b81f69cc4cf4282a01c4472dc208f4ba4f7 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 17 Jul 2026 12:41:08 +0200 Subject: [PATCH 15/19] [management] allow disabling device code flow when using dex (#6809) --- go.mod | 2 +- go.sum | 4 +- idp/dex/config.go | 4 ++ idp/dex/provider.go | 2 +- idp/dex/provider_test.go | 87 +++++++++++++++++++++++++++++++ management/server/idp/embedded.go | 4 ++ 6 files changed, 99 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index b90b68446..3129c0ce6 100644 --- a/go.mod +++ b/go.mod @@ -335,7 +335,7 @@ replace github.com/cloudflare/circl => codeberg.org/cunicu/circl v0.0.0-20230801 replace github.com/pion/ice/v4 => github.com/netbirdio/ice/v4 v4.0.0-20250908184934-6202be846b51 -replace github.com/dexidp/dex => github.com/netbirdio/dex v0.244.1-0.20260512110716-8d70ad8647c1 +replace github.com/dexidp/dex => github.com/netbirdio/dex v0.244.1-0.20260716205454-a163de3129e5 replace github.com/dexidp/dex/api/v2 => github.com/netbirdio/dex/api/v2 v2.0.0-20260512110716-8d70ad8647c1 diff --git a/go.sum b/go.sum index ad43dc109..a69667355 100644 --- a/go.sum +++ b/go.sum @@ -476,8 +476,8 @@ github.com/morikuni/aec v1.0.0 h1:nP9CBfwrvYnBRgY6qfDQkygYDmYwOilePFkwzv4dU8A= github.com/morikuni/aec v1.0.0/go.mod h1:BbKIizmSmc5MMPqRYbxO4ZU0S0+P200+tUnFx7PXmsc= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= -github.com/netbirdio/dex v0.244.1-0.20260512110716-8d70ad8647c1 h1:4TaYr9O4xX0D2kszeOLclTiCbA3eHq3xWV+9ILJbIYs= -github.com/netbirdio/dex v0.244.1-0.20260512110716-8d70ad8647c1/go.mod h1:IHH+H8vK2GfqtIt5u/5OdPh18yk0oDHuj2vz5+Goetg= +github.com/netbirdio/dex v0.244.1-0.20260716205454-a163de3129e5 h1:3PwQv8aR46qN2u16+Dv6udnH3sbVKX5KrGwF35CKSI0= +github.com/netbirdio/dex v0.244.1-0.20260716205454-a163de3129e5/go.mod h1:IHH+H8vK2GfqtIt5u/5OdPh18yk0oDHuj2vz5+Goetg= github.com/netbirdio/dex/api/v2 v2.0.0-20260512110716-8d70ad8647c1 h1:neE7z+FPUkldl3faK/Jt+hJK2L+1XfQ1W33TQhU9m88= github.com/netbirdio/dex/api/v2 v2.0.0-20260512110716-8d70ad8647c1/go.mod h1:awuTyT29CYALpEyET0S307EgNlPWrc7fFKRAyhsO45M= github.com/netbirdio/easyjson v0.9.0 h1:6Nw2lghSVuy8RSkAYDhDv1thBVEmfVbKZnV7T7Z6Aus= diff --git a/idp/dex/config.go b/idp/dex/config.go index 56ed998c2..9e56eb6c0 100644 --- a/idp/dex/config.go +++ b/idp/dex/config.go @@ -613,6 +613,10 @@ func (c *YAMLConfig) ToServerConfig(stor storage.Storage, logger *slog.Logger) s cfg.SupportedResponseTypes = c.OAuth2.ResponseTypes } + if len(c.OAuth2.GrantTypes) > 0 { + cfg.AllowedGrantTypes = c.OAuth2.GrantTypes + } + // Apply expiry settings if c.Expiry.IDTokens != "" { if d, err := parseDuration(c.Expiry.IDTokens); err == nil { diff --git a/idp/dex/provider.go b/idp/dex/provider.go index 67aeb995f..c0b705f13 100644 --- a/idp/dex/provider.go +++ b/idp/dex/provider.go @@ -21,7 +21,7 @@ import ( "github.com/dexidp/dex/server/signer" "github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage/sql" - jose "github.com/go-jose/go-jose/v4" + "github.com/go-jose/go-jose/v4" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" "golang.org/x/crypto/bcrypt" diff --git a/idp/dex/provider_test.go b/idp/dex/provider_test.go index 3eb29db97..0fce1b2c9 100644 --- a/idp/dex/provider_test.go +++ b/idp/dex/provider_test.go @@ -595,3 +595,90 @@ enablePasswordDB: true assert.True(t, cfg.ContinueOnConnectorFailure, "buildDexConfig must set ContinueOnConnectorFailure to true so management starts even if an external IdP is down") } + +func TestToServerConfig_WiresGrantTypes(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "dex-grants-*") + require.NoError(t, err) + defer os.RemoveAll(tmpDir) + + stor := openTestStorage(t, tmpDir) + defer stor.Close() + + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + + grants := []string{"authorization_code", "refresh_token"} + cfg := &YAMLConfig{Issuer: "http://localhost:5599/oauth2", OAuth2: OAuth2{GrantTypes: grants}} + assert.Equal(t, grants, cfg.ToServerConfig(stor, logger).AllowedGrantTypes) + + empty := &YAMLConfig{Issuer: "http://localhost:5599/oauth2"} + assert.Empty(t, empty.ToServerConfig(stor, logger).AllowedGrantTypes) +} + +func newDeviceGuardProvider(t *testing.T, grantTypesYAML string) *Provider { + t.Helper() + + tmpDir, err := os.MkdirTemp("", "dex-devguard-*") + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(tmpDir) }) + + yamlContent := ` +issuer: http://localhost:5599/oauth2 +storage: + type: sqlite3 + config: + file: ` + filepath.Join(tmpDir, "dex.db") + ` +web: + http: 127.0.0.1:5599 +enablePasswordDB: true +` + grantTypesYAML + + configPath := filepath.Join(tmpDir, "config.yaml") + require.NoError(t, os.WriteFile(configPath, []byte(yamlContent), 0644)) + + yamlConfig, err := LoadConfig(configPath) + require.NoError(t, err) + + provider, err := NewProviderFromYAML(context.Background(), yamlConfig) + require.NoError(t, err) + t.Cleanup(func() { _ = provider.Stop(context.Background()) }) + return provider +} + +func TestHandler_BlocksDeviceEndpointsWhenDeviceGrantDisabled(t *testing.T) { + provider := newDeviceGuardProvider(t, ` +oauth2: + grantTypes: + - authorization_code + - refresh_token +`) + + devicePaths := []string{ + "/oauth2/device", + "/oauth2/device/code", + "/oauth2/device/token", + "/oauth2/device/auth/verify_code", + "/oauth2/device/callback", + } + for _, path := range devicePaths { + for _, method := range []string{http.MethodGet, http.MethodPost} { + req := httptest.NewRequest(method, path, nil) + rec := httptest.NewRecorder() + provider.Handler().ServeHTTP(rec, req) + assert.Equal(t, http.StatusNotFound, rec.Code, "%s %s must be blocked", method, path) + } + } + + req := httptest.NewRequest(http.MethodGet, "/oauth2/.well-known/openid-configuration", nil) + rec := httptest.NewRecorder() + provider.Handler().ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code) +} + +func TestHandler_AllowsDeviceEndpointsWhenGrantsDefault(t *testing.T) { + provider := newDeviceGuardProvider(t, "") + + req := httptest.NewRequest(http.MethodPost, "/oauth2/device/code", nil) + rec := httptest.NewRecorder() + provider.Handler().ServeHTTP(rec, req) + assert.NotEqual(t, http.StatusNotFound, rec.Code) +} diff --git a/management/server/idp/embedded.go b/management/server/idp/embedded.go index 821e6ff55..029749a25 100644 --- a/management/server/idp/embedded.go +++ b/management/server/idp/embedded.go @@ -76,6 +76,9 @@ type EmbeddedIdPConfig struct { DashboardPostLogoutRedirectURIs []string // StaticConnectors are additional connectors to seed during initialization StaticConnectors []dex.Connector + // GrantTypes restricts allowed OAuth2 grants; empty means all (Dex default). Omit the + // device_code grant to disable the device flow; keep authorization_code and refresh_token. + GrantTypes []string } // EmbeddedStorageConfig holds storage configuration for the embedded IdP. @@ -175,6 +178,7 @@ func (c *EmbeddedIdPConfig) ToYAMLConfig() (*dex.YAMLConfig, error) { }, OAuth2: dex.OAuth2{ SkipApprovalScreen: true, + GrantTypes: c.GrantTypes, }, Frontend: dex.Frontend{ Issuer: "NetBird", From b7b0d5796e988ac5f369d51d3a16b162d5fb9522 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:09:09 +0200 Subject: [PATCH 16/19] [client] Bind netstack SOCKS5 proxy to 127.0.0.1 by default (#6812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes In netstack mode the SOCKS5 proxy bridges local host applications into the userspace WireGuard stack (`client/iface/netstack/proxy.go`), so it only needs to be reachable from the same machine. It was binding to `0.0.0.0`, making an unauthenticated proxy reachable from the network — any host able to reach the port could relay traffic through the client into its NetBird overlay. Bind to `127.0.0.1` by default. Add `NB_SOCKS5_LISTENER_ADDRESS` to override the bind host for the rare case the proxy must be reachable from other hosts (e.g. a container gateway); it is validated as an IP and falls back to loopback. `ListenAddr` is split into `listenHost`/`listenPort` helpers, with tests. Behavior change: setups that relied on reaching the netstack SOCKS5 proxy from another host must now set `NB_SOCKS5_LISTENER_ADDRESS=0.0.0.0` explicitly. ## Issue ticket number and link Internal security hardening of the netstack SOCKS5 listener bind address ([client/iface/netstack/env.go](https://github.com/netbirdio/netbird/blob/main/client/iface/netstack/env.go)). ## Stack - `0.74.7-branch` - :warning: No PR associated with branch - \#6812 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [x] I added/updated documentation for this change - [ ] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: *** View with Codesmith Autofix with Codesmith Need help on this PR? Tag /codesmith with what you need. Autofix is disabled. ## Summary by CodeRabbit - **New Features** - Added configuration options for the SOCKS5 listener’s bind address and port. - SOCKS5 now defaults to listening only on the local machine for improved security. - Valid address and port overrides are supported, with safe defaults used for invalid values. - **Tests** - Added coverage for default settings and valid or invalid address and port configurations. --- client/iface/netstack/env.go | 55 +++++++++++++++++++++------ client/iface/netstack/env_test.go | 63 +++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 11 deletions(-) create mode 100644 client/iface/netstack/env_test.go diff --git a/client/iface/netstack/env.go b/client/iface/netstack/env.go index dd8cf29a3..b069301c1 100644 --- a/client/iface/netstack/env.go +++ b/client/iface/netstack/env.go @@ -3,14 +3,31 @@ package netstack import ( - "fmt" + "net" "os" "strconv" log "github.com/sirupsen/logrus" ) -const EnvUseNetstackMode = "NB_USE_NETSTACK_MODE" +const ( + EnvUseNetstackMode = "NB_USE_NETSTACK_MODE" + + // EnvSocks5ListenerPort overrides the port the SOCKS5 proxy listens on. + EnvSocks5ListenerPort = "NB_SOCKS5_LISTENER_PORT" + + // EnvSocks5ListenerAddress overrides the host/IP the SOCKS5 proxy binds to. + // The proxy is a bridge for local host applications into the userspace + // WireGuard netstack, so it binds to loopback by default. Override this only + // when the proxy must be reachable from other hosts (e.g. a container + // gateway); doing so exposes an unauthenticated SOCKS5 proxy on that + // address. + EnvSocks5ListenerAddress = "NB_SOCKS5_LISTENER_ADDRESS" + + // defaultSocks5Host is the loopback address the SOCKS5 proxy binds to unless + // overridden via EnvSocks5ListenerAddress. + defaultSocks5Host = "127.0.0.1" +) // IsEnabled todo: move these function to cmd layer func IsEnabled() bool { @@ -18,24 +35,40 @@ func IsEnabled() bool { } func ListenAddr() string { - sPort := os.Getenv("NB_SOCKS5_LISTENER_PORT") + return net.JoinHostPort(listenHost(), strconv.Itoa(listenPort())) +} + +// listenHost returns the host/IP the SOCKS5 proxy binds to. It defaults to +// loopback and only honors EnvSocks5ListenerAddress when it holds a valid IP. +func listenHost() string { + addr := os.Getenv(EnvSocks5ListenerAddress) + if addr == "" { + return defaultSocks5Host + } + if net.ParseIP(addr) == nil { + log.Warnf("invalid socks5 listener address %q, falling back to default: %s", addr, defaultSocks5Host) + return defaultSocks5Host + } + return addr +} + +// listenPort returns the port the SOCKS5 proxy binds to, defaulting to +// DefaultSocks5Port when EnvSocks5ListenerPort is unset or invalid. +func listenPort() int { + sPort := os.Getenv(EnvSocks5ListenerPort) if sPort == "" { - return listenAddr(DefaultSocks5Port) + return DefaultSocks5Port } port, err := strconv.Atoi(sPort) if err != nil { log.Warnf("invalid socks5 listener port, unable to convert it to int, falling back to default: %d", DefaultSocks5Port) - return listenAddr(DefaultSocks5Port) + return DefaultSocks5Port } if port < 1 || port > 65535 { log.Warnf("invalid socks5 listener port, it should be in the range 1-65535, falling back to default: %d", DefaultSocks5Port) - return listenAddr(DefaultSocks5Port) + return DefaultSocks5Port } - return listenAddr(port) -} - -func listenAddr(port int) string { - return fmt.Sprintf("0.0.0.0:%d", port) + return port } diff --git a/client/iface/netstack/env_test.go b/client/iface/netstack/env_test.go new file mode 100644 index 000000000..1083435a4 --- /dev/null +++ b/client/iface/netstack/env_test.go @@ -0,0 +1,63 @@ +//go:build !js + +package netstack + +import ( + "net" + "strconv" + "testing" +) + +func TestListenAddr_DefaultsToLoopback(t *testing.T) { + // No env overrides: must bind loopback, never all interfaces. + got := ListenAddr() + want := net.JoinHostPort("127.0.0.1", strconv.Itoa(DefaultSocks5Port)) + if got != want { + t.Fatalf("ListenAddr() = %q, want %q", got, want) + } +} + +func TestListenAddr_AddressOverride(t *testing.T) { + tests := []struct { + name string + env string + want string + }{ + {name: "valid override honored", env: "0.0.0.0", want: "0.0.0.0"}, + {name: "valid specific ip honored", env: "10.0.0.5", want: "10.0.0.5"}, + {name: "ipv6 loopback bracketed", env: "::1", want: "::1"}, + {name: "invalid falls back to loopback", env: "not-an-ip", want: "127.0.0.1"}, + {name: "empty falls back to loopback", env: "", want: "127.0.0.1"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(EnvSocks5ListenerAddress, tc.env) + want := net.JoinHostPort(tc.want, strconv.Itoa(DefaultSocks5Port)) + if got := ListenAddr(); got != want { + t.Fatalf("ListenAddr() = %q, want %q", got, want) + } + }) + } +} + +func TestListenAddr_PortOverride(t *testing.T) { + tests := []struct { + name string + env string + want int + }{ + {name: "valid port honored", env: "1081", want: 1081}, + {name: "non-numeric falls back", env: "abc", want: DefaultSocks5Port}, + {name: "out of range falls back", env: "70000", want: DefaultSocks5Port}, + {name: "zero falls back", env: "0", want: DefaultSocks5Port}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(EnvSocks5ListenerPort, tc.env) + want := net.JoinHostPort("127.0.0.1", strconv.Itoa(tc.want)) + if got := ListenAddr(); got != want { + t.Fatalf("ListenAddr() = %q, want %q", got, want) + } + }) + } +} From 41d7bf4bbdaff7d60723e6eb15ee1b9466fe34a6 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 17 Jul 2026 15:31:07 +0200 Subject: [PATCH 17/19] [client] Diagnose empty vs corrupt state (#6816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes When loadStateFile fails to unmarshal the state file, log whether the file is empty (0 bytes) or has malformed content, including the byte size. ## Issue ticket number and link ## Stack ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [x] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) aste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ ## Summary by CodeRabbit * **Bug Fixes** * Improved state-file loading warnings by distinguishing empty files from files containing malformed content. * Preserved existing recovery behavior for corrupted state files. --- client/internal/statemanager/manager.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/client/internal/statemanager/manager.go b/client/internal/statemanager/manager.go index 566905985..ca4194690 100644 --- a/client/internal/statemanager/manager.go +++ b/client/internal/statemanager/manager.go @@ -1,6 +1,7 @@ package statemanager import ( + "bytes" "context" "encoding/json" "errors" @@ -305,6 +306,11 @@ func (m *Manager) loadStateFile(deleteCorrupt bool) (map[string]json.RawMessage, var rawStates map[string]json.RawMessage if err := json.Unmarshal(data, &rawStates); err != nil { + if len(bytes.TrimSpace(data)) == 0 { + log.Warnf("state file %s is empty (%d bytes)", m.filePath, len(data)) + } else { + log.Warnf("state file %s has malformed content (%d bytes)", m.filePath, len(data)) + } m.handleCorruptedState(deleteCorrupt) return nil, fmt.Errorf("unmarshal states: %w", err) } From a59d7fba9532ade7b5445af7dd8bd7ee832225a8 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:41:57 +0200 Subject: [PATCH 18/19] [management] propagate auth grant types for combined server (#6817) --- combined/cmd/config.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/combined/cmd/config.go b/combined/cmd/config.go index fe350e52a..fcbc60dc9 100644 --- a/combined/cmd/config.go +++ b/combined/cmd/config.go @@ -145,6 +145,7 @@ type AuthConfig struct { CLIRedirectURIs []string `yaml:"cliRedirectURIs"` Owner *AuthOwnerConfig `yaml:"owner,omitempty"` DashboardPostLogoutRedirectURIs []string `yaml:"dashboardPostLogoutRedirectURIs"` + GrantTypes []string `yaml:"grantTypes"` } // AuthStorageConfig contains auth storage settings @@ -604,6 +605,7 @@ func (c *CombinedConfig) buildEmbeddedIdPConfig(mgmt ManagementConfig) (*idp.Emb DashboardRedirectURIs: mgmt.Auth.DashboardRedirectURIs, CLIRedirectURIs: mgmt.Auth.CLIRedirectURIs, DashboardPostLogoutRedirectURIs: mgmt.Auth.DashboardPostLogoutRedirectURIs, + GrantTypes: mgmt.Auth.GrantTypes, } if mgmt.Auth.Owner != nil && mgmt.Auth.Owner.Email != "" { From a1c9427d8004576e2cbb9e546d409847fa9df318 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Fri, 17 Jul 2026 23:05:32 +0900 Subject: [PATCH 19/19] [client] Evaluate IP fragments against firewall ACLs (#6781) --- client/firewall/uspfilter/filter.go | 324 ++++++++-- client/firewall/uspfilter/fragment.go | 204 +++++++ .../firewall/uspfilter/fragment_bench_test.go | 115 ++++ client/firewall/uspfilter/fragment_test.go | 554 ++++++++++++++++++ 4 files changed, 1144 insertions(+), 53 deletions(-) create mode 100644 client/firewall/uspfilter/fragment.go create mode 100644 client/firewall/uspfilter/fragment_bench_test.go create mode 100644 client/firewall/uspfilter/fragment_test.go diff --git a/client/firewall/uspfilter/filter.go b/client/firewall/uspfilter/filter.go index 91866dcab..7376e59ca 100644 --- a/client/firewall/uspfilter/filter.go +++ b/client/firewall/uspfilter/filter.go @@ -121,6 +121,7 @@ type Manager struct { udpTracker *conntrack.UDPTracker icmpTracker *conntrack.ICMPTracker tcpTracker *conntrack.TCPTracker + fragments *fragmentTracker forwarder atomic.Pointer[forwarder.Forwarder] pendingCapture atomic.Pointer[forwarder.PacketCapture] logger *nblog.Logger @@ -183,6 +184,41 @@ func (d *decoder) decodePacket(data []byte) error { } } +// decodeTransport decodes the transport header of a first fragment (which +// gopacket leaves undecoded) into the decoder and appends its layer type to +// decoded, so the ACL pipeline can evaluate it like a normal packet. It returns +// false if the protocol is unsupported or the header is truncated. +func (d *decoder) decodeTransport(proto layers.IPProtocol, payload []byte) bool { + var l4 gopacket.DecodingLayer + var layerType gopacket.LayerType + var minLen int + switch proto { + case layers.IPProtocolTCP: + l4, layerType, minLen = &d.tcp, layers.LayerTypeTCP, 20 + case layers.IPProtocolUDP: + l4, layerType, minLen = &d.udp, layers.LayerTypeUDP, 8 + case layers.IPProtocolICMPv4: + l4, layerType, minLen = &d.icmp4, layers.LayerTypeICMPv4, 8 + case layers.IPProtocolICMPv6: + l4, layerType, minLen = &d.icmp6, layers.LayerTypeICMPv6, 8 + default: + return false + } + + // Reject a fragment too small to hold the full transport header before + // decoding: it can't be ACL-evaluated (tiny-fragment attack), and skipping + // the decode avoids gopacket allocating an error on the drop path. + if len(payload) < minLen { + return false + } + + if err := l4.DecodeFromBytes(payload, gopacket.NilDecodeFeedback); err != nil { + return false + } + d.decoded = append(d.decoded, layerType) + return true +} + // Create userspace firewall manager constructor func Create(iface common.IFaceMapper, disableServerRoutes bool, flowLogger nftypes.FlowLogger, mtu uint16) (*Manager, error) { return create(iface, nil, disableServerRoutes, flowLogger, mtu) @@ -286,6 +322,8 @@ func create(iface common.IFaceMapper, nativeFirewall firewall.Manager, disableSe if err := m.localipmanager.UpdateLocalIPs(iface); err != nil { return nil, fmt.Errorf("update local IPs: %w", err) } + m.fragments = newFragmentTracker(m.logger) + if disableConntrack { log.Info("conntrack is disabled") } else { @@ -299,6 +337,7 @@ func create(iface common.IFaceMapper, nativeFirewall firewall.Manager, disableSe } } if err := iface.SetFilter(m); err != nil { + m.fragments.Close() return nil, fmt.Errorf("set filter: %w", err) } return m, nil @@ -694,6 +733,10 @@ func (m *Manager) resetState() { m.tcpTracker.Close() } + if m.fragments != nil { + m.fragments.Close() + } + if fwder := m.forwarder.Load(); fwder != nil { fwder.SetCapture(nil) fwder.Stop() @@ -1046,19 +1089,20 @@ func (m *Manager) filterInbound(packetData []byte, size int) bool { return true } - // TODO: pass fragments of routed packets to forwarder + // gopacket does not decode the transport header of any IP fragment, so + // fragments take a dedicated path: the first fragment's header is decoded + // and ACL-evaluated here, and the remaining fragments inherit its verdict. if fragment { - if m.logger.Enabled(nblog.LevelTrace) { - if d.decoded[0] == layers.LayerTypeIPv4 { - m.logger.Trace4("packet is a fragment: src=%v dst=%v id=%v flags=%v", - srcIP, dstIP, d.ip4.Id, d.ip4.Flags) - } else { - m.logger.Trace2("packet is an IPv6 fragment: src=%v dst=%v", srcIP, dstIP) - } - } - return false + return m.filterInboundFragment(d, srcIP, dstIP, size) } + return m.filterInboundDecoded(d, srcIP, dstIP, packetData, size) +} + +// filterInboundDecoded runs the ACL, DNAT and conntrack pipeline on a fully +// decoded (non-fragment) inbound packet. It returns true if the packet should +// be dropped. +func (m *Manager) filterInboundDecoded(d *decoder, srcIP, dstIP netip.Addr, packetData []byte, size int) bool { // TODO: optimize port DNAT by caching matched rules in conntrack if translated := m.translateInboundPortDNAT(packetData, d, srcIP, dstIP); translated { // Re-decode after port DNAT translation to update port information @@ -1089,33 +1133,226 @@ func (m *Manager) filterInbound(packetData []byte, size int) bool { return m.handleRoutedTraffic(d, srcIP, dstIP, packetData, size) } +// fragmentMeta holds the reassembly identity and layout of an IP fragment, +// extracted uniformly for IPv4 and IPv6. +type fragmentMeta struct { + key fragmentKey + // offset is the fragment offset in 8-byte units (zero for the first + // fragment). + offset uint16 + // moreFragments is the More Fragments bit. A first fragment with it unset is + // an IPv6 atomic fragment (a complete datagram, RFC 6946): it has no trailing + // fragments to inherit a verdict, so it must not be recorded. + moreFragments bool + proto layers.IPProtocol + // l4payload is the fragmentable payload of this fragment. For the first + // fragment it starts with the transport header. + l4payload []byte + // headerEndOctets is the first fragment's payload length in 8-byte units: + // the smallest offset a trailing fragment may start at without overlapping + // the inspected transport header. + headerEndOctets uint16 +} + +// fragmentMetadata extracts the fragment identity and layout from a decoded IP +// fragment. It returns false for fragments it can't interpret (e.g. an IPv6 +// fragment header shorter than 8 bytes), which are then dropped. +func fragmentMetadata(d *decoder, srcIP, dstIP netip.Addr) (fragmentMeta, bool) { + switch d.decoded[0] { + case layers.LayerTypeIPv4: + payload := d.ip4.Payload + return fragmentMeta{ + key: fragmentKey{srcIP: srcIP, dstIP: dstIP, id: uint32(d.ip4.Id), proto: uint8(d.ip4.Protocol)}, + offset: d.ip4.FragOffset, + moreFragments: d.ip4.Flags&layers.IPv4MoreFragments != 0, + proto: d.ip4.Protocol, + l4payload: payload, + headerEndOctets: octets(len(payload)), + }, true + + case layers.LayerTypeIPv6: + // IPv6 fragment extension header: 8 bytes, followed by the fragmentable + // payload. Layout: next header (1), reserved (1), offset+flags (2), id (4). + payload := d.ip6.Payload + if len(payload) < 8 { + return fragmentMeta{}, false + } + nextHeader := layers.IPProtocol(payload[0]) + offsetFlags := binary.BigEndian.Uint16(payload[2:4]) + id := binary.BigEndian.Uint32(payload[4:8]) + l4 := payload[8:] + return fragmentMeta{ + key: fragmentKey{srcIP: srcIP, dstIP: dstIP, id: id, proto: uint8(nextHeader)}, + offset: offsetFlags >> 3, + moreFragments: offsetFlags&1 != 0, + proto: nextHeader, + l4payload: l4, + headerEndOctets: octets(len(l4)), + }, true + + default: + return fragmentMeta{}, false + } +} + +// octets rounds a byte length up to whole 8-byte units, the granularity of the +// IP fragment offset field. +func octets(nbytes int) uint16 { + return uint16((nbytes + 7) / 8) +} + +// filterInboundFragment decides the fate of an IP fragment. gopacket stops +// decoding at the network layer for every fragment, so the first fragment's +// transport header is decoded and ACL-evaluated here and its verdict recorded; +// the remaining (headerless) fragments inherit that verdict. Anything that +// cannot be tied to an allowed, non-overlapping first fragment is dropped. +func (m *Manager) filterInboundFragment(d *decoder, srcIP, dstIP netip.Addr, size int) bool { + meta, ok := fragmentMetadata(d, srcIP, dstIP) + if !ok { + if m.logger.Enabled(nblog.LevelTrace) { + m.logger.Trace2("dropping unsupported fragment: src=%v dst=%v", srcIP, dstIP) + } + return true + } + + if meta.offset != 0 { + return m.filterTrailingFragment(meta, srcIP, dstIP) + } + + // A new first fragment supersedes any recorded verdict for this datagram, so + // a re-sent or overlapping offset-zero fragment can't inherit the old one. + m.fragments.poison(meta.key) + + // First fragment: decode its transport header so the ACL can evaluate it. A + // decode failure means the fragment is too small to hold the full transport + // header (RFC 1858 §3 tiny-fragment attack); it can't be evaluated, so drop it. + if !d.decodeTransport(meta.proto, meta.l4payload) { + if m.logger.Enabled(nblog.LevelTrace) { + m.logger.Trace3("dropping first fragment without full L4 header: src=%v dst=%v id=%v", + srcIP, dstIP, meta.key.id) + } + return true + } + + return m.filterFirstFragment(d, meta, srcIP, dstIP, size) +} + +// filterTrailingFragment applies a recorded first-fragment verdict to a +// non-first fragment. +func (m *Manager) filterTrailingFragment(meta fragmentMeta, srcIP, dstIP netip.Addr) bool { + switch m.fragments.verdict(meta.key, meta.offset) { + case fragmentAllow: + return false + case fragmentOverlap: + if m.logger.Enabled(nblog.LevelTrace) { + m.logger.Trace3("dropping overlapping fragment rewriting inspected header: src=%v dst=%v id=%v", + srcIP, dstIP, meta.key.id) + } + return true + default: + if m.logger.Enabled(nblog.LevelTrace) { + m.logger.Trace3("dropping fragment with no allowed first fragment: src=%v dst=%v id=%v", + srcIP, dstIP, meta.key.id) + } + return true + } +} + +// filterFirstFragment runs the verdict part of the inbound pipeline on a first +// fragment with its transport header decoded. It mirrors filterInboundDecoded +// but skips DNAT (port rewriting on fragments is unsupported) and forwarder +// injection (fragments are left to the stack to reassemble, not forwarded). +// Allowed fragments have their verdict recorded so the datagram's trailing +// fragments inherit it. +func (m *Manager) filterFirstFragment(d *decoder, meta fragmentMeta, srcIP, dstIP netip.Addr, size int) bool { + if m.stateful && m.isValidTrackedConnection(d, srcIP, dstIP, size) { + m.recordFirstFragment(meta) + return false + } + + if m.localipmanager.IsLocalIP(dstIP) { + ruleID, blocked := m.peerACLsBlock(srcIP, d, nil) + if blocked { + m.storeDropFlow("Dropping local first fragment (ACL denied): rule_id=%s proto=%v src=%s:%d dst=%s:%d", + d, srcIP, dstIP, ruleID, size) + return true + } + m.trackInbound(d, srcIP, dstIP, ruleID, size) + m.recordFirstFragment(meta) + return false + } + + if !m.routingEnabled.Load() { + if m.logger.Enabled(nblog.LevelTrace) { + m.logger.Trace2("Dropping routed fragment (routing disabled): src=%s dst=%s", srcIP, dstIP) + } + return true + } + if m.nativeRouter.Load() { + m.trackInbound(d, srcIP, dstIP, nil, size) + m.recordFirstFragment(meta) + return false + } + + // TODO: pass fragments of routed packets to the forwarder; until then + // allowed routed fragments go to the native stack. + srcPort, dstPort := getPortsFromPacket(d) + ruleID, pass := m.routeACLsPass(srcIP, dstIP, d.decoded[1], srcPort, dstPort) + if !pass { + m.storeDropFlow("Dropping routed first fragment (ACL denied): rule_id=%s proto=%v src=%s:%d dst=%s:%d", + d, srcIP, dstIP, ruleID, size) + return true + } + + m.recordFirstFragment(meta) + return false +} + +// recordFirstFragment caches an allowed first fragment's verdict for its +// trailing fragments to inherit. Atomic fragments (no More Fragments bit) are +// complete datagrams with no trailing fragments, so they are not cached and +// cannot exhaust the verdict table. +func (m *Manager) recordFirstFragment(meta fragmentMeta) { + if !meta.moreFragments { + return + } + m.fragments.recordAllowed(meta.key, meta.headerEndOctets) +} + +// storeDropFlow logs and records a netflow drop event for an inbound packet +// denied by the ACLs. msg is the trace format taking rule id, protocol, source +// and destination. +func (m *Manager) storeDropFlow(msg string, d *decoder, srcIP, dstIP netip.Addr, ruleID []byte, size int) { + pnum := getProtocolFromPacket(d) + srcPort, dstPort := getPortsFromPacket(d) + + if m.logger.Enabled(nblog.LevelTrace) { + m.logger.Trace6(msg, ruleID, pnum, srcIP, srcPort, dstIP, dstPort) + } + + m.flowLogger.StoreEvent(nftypes.EventFields{ + FlowID: uuid.New(), + Type: nftypes.TypeDrop, + RuleID: ruleID, + Direction: nftypes.Ingress, + Protocol: pnum, + SourceIP: srcIP, + DestIP: dstIP, + SourcePort: srcPort, + DestPort: dstPort, + // TODO: icmp type/code + RxPackets: 1, + RxBytes: uint64(size), + }) +} + // handleLocalTraffic handles local traffic. // If it returns true, the packet should be dropped. func (m *Manager) handleLocalTraffic(d *decoder, srcIP, dstIP netip.Addr, packetData []byte, size int) bool { ruleID, blocked := m.peerACLsBlock(srcIP, d, packetData) if blocked { - pnum := getProtocolFromPacket(d) - srcPort, dstPort := getPortsFromPacket(d) - - if m.logger.Enabled(nblog.LevelTrace) { - m.logger.Trace6("Dropping local packet (ACL denied): rule_id=%s proto=%v src=%s:%d dst=%s:%d", - ruleID, pnum, srcIP, srcPort, dstIP, dstPort) - } - - m.flowLogger.StoreEvent(nftypes.EventFields{ - FlowID: uuid.New(), - Type: nftypes.TypeDrop, - RuleID: ruleID, - Direction: nftypes.Ingress, - Protocol: pnum, - SourceIP: srcIP, - DestIP: dstIP, - SourcePort: srcPort, - DestPort: dstPort, - // TODO: icmp type/code - RxPackets: 1, - RxBytes: uint64(size), - }) + m.storeDropFlow("Dropping local packet (ACL denied): rule_id=%s proto=%v src=%s:%d dst=%s:%d", + d, srcIP, dstIP, ruleID, size) return true } @@ -1168,27 +1405,8 @@ func (m *Manager) handleRoutedTraffic(d *decoder, srcIP, dstIP netip.Addr, packe ruleID, pass := m.routeACLsPass(srcIP, dstIP, protoLayer, srcPort, dstPort) if !pass { - proto := getProtocolFromPacket(d) - - if m.logger.Enabled(nblog.LevelTrace) { - m.logger.Trace6("Dropping routed packet (ACL denied): rule_id=%s proto=%v src=%s:%d dst=%s:%d", - ruleID, proto, srcIP, srcPort, dstIP, dstPort) - } - - m.flowLogger.StoreEvent(nftypes.EventFields{ - FlowID: uuid.New(), - Type: nftypes.TypeDrop, - RuleID: ruleID, - Direction: nftypes.Ingress, - Protocol: proto, - SourceIP: srcIP, - DestIP: dstIP, - SourcePort: srcPort, - DestPort: dstPort, - // TODO: icmp type/code - RxPackets: 1, - RxBytes: uint64(size), - }) + m.storeDropFlow("Dropping routed packet (ACL denied): rule_id=%s proto=%v src=%s:%d dst=%s:%d", + d, srcIP, dstIP, ruleID, size) return true } diff --git a/client/firewall/uspfilter/fragment.go b/client/firewall/uspfilter/fragment.go new file mode 100644 index 000000000..accc54365 --- /dev/null +++ b/client/firewall/uspfilter/fragment.go @@ -0,0 +1,204 @@ +package uspfilter + +import ( + "context" + "net/netip" + "os" + "strconv" + "sync" + "time" + + nblog "github.com/netbirdio/netbird/client/firewall/uspfilter/log" +) + +const ( + // defaultFragmentTimeout bounds how long a first-fragment verdict is kept + // while the remaining fragments arrive. It mirrors the Linux IP reassembly + // timeout (net.ipv4.ipfrag_time). + defaultFragmentTimeout = 30 * time.Second + // fragmentCleanupInterval is how often expired verdicts are purged. + fragmentCleanupInterval = 10 * time.Second + // defaultMaxFragmentEntries caps the number of concurrently tracked + // fragmented datagrams. The table stays bounded because each datagram is a + // single small entry regardless of how many fragments it is split into, and + // the 13-bit IPv4 fragment-offset field limits any datagram to 64 KiB. + defaultMaxFragmentEntries = 16384 + + // EnvFragmentMaxEntries overrides defaultMaxFragmentEntries. + EnvFragmentMaxEntries = "NB_FRAGMENT_MAX_ENTRIES" +) + +// fragmentVerdict is the decision for a trailing (headerless) fragment. +type fragmentVerdict int + +const ( + // fragmentDeny drops the fragment: no allowed first fragment is on record. + fragmentDeny fragmentVerdict = iota + // fragmentAllow passes the fragment: it belongs to an allowed datagram and + // does not overlap the already-inspected transport header. + fragmentAllow + // fragmentOverlap drops the fragment and poisons its datagram: it overlaps + // the transport header the ACL inspected (RFC 1858 §4, RFC 3128; RFC 5722 + // requires discarding the whole datagram on overlap for IPv6). + fragmentOverlap +) + +// fragmentKey identifies a fragmented datagram. It matches the RFC 791 / RFC +// 8200 reassembly key: source, destination, protocol and identification. The id +// is 32-bit to hold both the IPv4 (16-bit) and IPv6 (32-bit) identification. +type fragmentKey struct { + srcIP netip.Addr + dstIP netip.Addr + id uint32 + proto uint8 +} + +// fragmentEntry records the verdict of an allowed first fragment. +type fragmentEntry struct { + // headerEndOctets is the offset, in 8-byte units, at which the first + // fragment's payload ended. A trailing fragment starting before this + // overlaps bytes the ACL already inspected and is rejected. + headerEndOctets uint16 + // recordedAt is when the first fragment was accepted. The verdict expires a + // fixed timeout later and is not refreshed, mirroring the kernel reassembly + // timer so a trailing-fragment flood can't keep a datagram alive. + recordedAt time.Time +} + +// fragmentTracker records the ACL verdict of a datagram's first fragment so the +// remaining fragments, which carry no L4 header, can inherit the decision +// without reassembling the datagram. Only allowed first fragments are stored; +// anything that cannot be tied to an allowed, non-overlapping first fragment is +// dropped (fail closed). +type fragmentTracker struct { + logger *nblog.Logger + mutex sync.Mutex + entries map[fragmentKey]fragmentEntry + timeout time.Duration + // maxEntries caps the table; atCapacity dedups the capacity warning until + // the table drains below the cap again. + maxEntries int + atCapacity bool + cleanupTicker *time.Ticker + cancel context.CancelFunc +} + +func newFragmentTracker(logger *nblog.Logger) *fragmentTracker { + ctx, cancel := context.WithCancel(context.Background()) + t := &fragmentTracker{ + logger: logger, + entries: make(map[fragmentKey]fragmentEntry), + timeout: defaultFragmentTimeout, + maxEntries: fragmentMaxEntries(logger), + cleanupTicker: time.NewTicker(fragmentCleanupInterval), + cancel: cancel, + } + go t.cleanupRoutine(ctx) + return t +} + +func fragmentMaxEntries(logger *nblog.Logger) int { + v := os.Getenv(EnvFragmentMaxEntries) + if v == "" { + return defaultMaxFragmentEntries + } + n, err := strconv.Atoi(v) + if err != nil || n <= 0 { + logger.Warn2("invalid %s=%q, using default", EnvFragmentMaxEntries, v) + return defaultMaxFragmentEntries + } + return n +} + +// recordAllowed stores the verdict of an allowed first fragment. headerEndOctets +// is the first fragment's payload length in 8-byte units. When the table is full +// the record is dropped, which fails closed: the datagram's trailing fragments +// will be denied. +func (t *fragmentTracker) recordAllowed(key fragmentKey, headerEndOctets uint16) { + t.mutex.Lock() + defer t.mutex.Unlock() + + if t.entries == nil { + return + } + if _, ok := t.entries[key]; !ok && len(t.entries) >= t.maxEntries { + if !t.atCapacity { + t.atCapacity = true + t.logger.Warn2("fragment verdict table at capacity (%d/%d): trailing fragments of new datagrams will be dropped", + len(t.entries), t.maxEntries) + } + return + } + t.entries[key] = fragmentEntry{ + headerEndOctets: headerEndOctets, + recordedAt: time.Now(), + } +} + +// poison drops any recorded verdict for a datagram, so its later fragments are +// denied until a new allowed first fragment is recorded. Called on every +// offset-zero fragment to defeat offset-zero overlap rewrites (RFC 3128). +func (t *fragmentTracker) poison(key fragmentKey) { + t.mutex.Lock() + defer t.mutex.Unlock() + delete(t.entries, key) +} + +// verdict decides the fate of a trailing fragment at fragOffsetOctets (the IPv4 +// fragment offset, in 8-byte units). A fragment overlapping the inspected +// header poisons the datagram: the entry is removed so all further fragments of +// that datagram are denied too. +func (t *fragmentTracker) verdict(key fragmentKey, fragOffsetOctets uint16) fragmentVerdict { + t.mutex.Lock() + defer t.mutex.Unlock() + + entry, ok := t.entries[key] + if !ok { + return fragmentDeny + } + if time.Since(entry.recordedAt) > t.timeout { + delete(t.entries, key) + return fragmentDeny + } + if fragOffsetOctets < entry.headerEndOctets { + delete(t.entries, key) + return fragmentOverlap + } + return fragmentAllow +} + +func (t *fragmentTracker) cleanupRoutine(ctx context.Context) { + defer t.cleanupTicker.Stop() + for { + select { + case <-t.cleanupTicker.C: + t.cleanup() + case <-ctx.Done(): + return + } + } +} + +func (t *fragmentTracker) cleanup() { + t.mutex.Lock() + defer t.mutex.Unlock() + + for key, entry := range t.entries { + if time.Since(entry.recordedAt) > t.timeout { + delete(t.entries, key) + } + } + + if len(t.entries) < t.maxEntries { + t.atCapacity = false + } +} + +// Close stops the cleanup routine and releases resources. +func (t *fragmentTracker) Close() { + t.cancel() + + t.mutex.Lock() + t.entries = nil + t.mutex.Unlock() +} diff --git a/client/firewall/uspfilter/fragment_bench_test.go b/client/firewall/uspfilter/fragment_bench_test.go new file mode 100644 index 000000000..a9e6d2d13 --- /dev/null +++ b/client/firewall/uspfilter/fragment_bench_test.go @@ -0,0 +1,115 @@ +package uspfilter + +import ( + "encoding/binary" + "testing" +) + +// benchFilterInbound drives filterInbound over a fixed packet in a tight loop. +// Packets are built once, outside the timed region, so the benchmark measures +// only pipeline cost, which is what an attacker can amplify. +func benchFilterInbound(b *testing.B, pkt []byte) { + b.Helper() + b.ReportAllocs() + b.SetBytes(int64(len(pkt))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + m := benchManager + m.filterInbound(pkt, len(pkt)) + } +} + +// benchManager is a package-level manager reused across fragment benchmarks so +// setup cost stays out of the timed region. +var benchManager *Manager + +func setupBenchManager(b *testing.B) *Manager { + b.Helper() + m := newFragmentTestManager(b) + allowUDP(b, m, 8080) + // Disable conntrack so the allowed-first-fragment path measures transport + // decode + ACL every iteration instead of matching the connection tracked + // on the first iteration. + m.stateful = false + benchManager = m + return m +} + +// BenchmarkInbound_NormalPacket is the baseline: a full, non-fragmented UDP +// packet that passes the ACL. Fragment paths should stay comparable to this. +func BenchmarkInbound_NormalPacket(b *testing.B) { + setupBenchManager(b) + pkt := normalUDPPacket(b, 8080, 32) + benchFilterInbound(b, pkt) +} + +// BenchmarkInbound_FirstFragmentAllowed measures the first-fragment path: +// transport decode + ACL evaluation + verdict record. +func BenchmarkInbound_FirstFragmentAllowed(b *testing.B) { + setupBenchManager(b) + pkt := firstFragmentUDP(b, 0x2000, 8080, 32) + benchFilterInbound(b, pkt) +} + +// BenchmarkInbound_TrailingFragmentAllowed measures the common trailing-fragment +// path: a single map lookup after the first fragment is on record. +func BenchmarkInbound_TrailingFragmentAllowed(b *testing.B) { + m := setupBenchManager(b) + first := firstFragmentUDP(b, 0x3000, 8080, 32) + m.filterInbound(first, len(first)) + pkt := trailingFragment(b, 0x3000, 5, false, 24) + benchFilterInbound(b, pkt) +} + +// BenchmarkInbound_TrailingFragmentNoFirst is the primary DoS vector: an +// attacker floods trailing fragments with no first fragment on record. Each is +// a map miss and must be cheap. +func BenchmarkInbound_TrailingFragmentNoFirst(b *testing.B) { + setupBenchManager(b) + pkt := trailingFragment(b, 0x4000, 185, false, 40) + benchFilterInbound(b, pkt) +} + +// BenchmarkInbound_TinyFirstFragment measures the tiny-fragment drop path: a +// first fragment too small to decode a transport header. +func BenchmarkInbound_TinyFirstFragment(b *testing.B) { + setupBenchManager(b) + pkt := trailingFragment(b, 0x5000, 0, true, 4) + benchFilterInbound(b, pkt) +} + +// BenchmarkInbound_TrailingFragmentDistinctIDs is the worst case for the +// verdict table: an attacker varies the datagram id on every packet so no first +// fragment ever matches. Verdict lookups always miss and nothing is recorded, +// so the table cannot grow. Each iteration rewrites the id field in place. +func BenchmarkInbound_TrailingFragmentDistinctIDs(b *testing.B) { + setupBenchManager(b) + pkt := trailingFragment(b, 0x6000, 185, false, 40) + m := benchManager + + b.ReportAllocs() + b.SetBytes(int64(len(pkt))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + // IPv4 identification field is at bytes 4:6. + binary.BigEndian.PutUint16(pkt[4:6], uint16(i)) + m.filterInbound(pkt, len(pkt)) + } +} + +// BenchmarkInbound_FirstFragmentDistinctIDs measures sustained first-fragment +// pressure with distinct ids: transport decode + ACL + verdict insert until the +// table caps, exercising the map growth and capacity guard. +func BenchmarkInbound_FirstFragmentDistinctIDs(b *testing.B) { + setupBenchManager(b) + pkt := firstFragmentUDP(b, 0x7000, 8080, 32) + m := benchManager + + b.ReportAllocs() + b.SetBytes(int64(len(pkt))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + binary.BigEndian.PutUint16(pkt[4:6], uint16(i)) + m.filterInbound(pkt, len(pkt)) + } +} diff --git a/client/firewall/uspfilter/fragment_test.go b/client/firewall/uspfilter/fragment_test.go new file mode 100644 index 000000000..6960e4dda --- /dev/null +++ b/client/firewall/uspfilter/fragment_test.go @@ -0,0 +1,554 @@ +package uspfilter + +import ( + "encoding/binary" + "net" + "net/netip" + "testing" + "time" + + "github.com/google/gopacket" + "github.com/google/gopacket/layers" + "github.com/stretchr/testify/require" + + fw "github.com/netbirdio/netbird/client/firewall/manager" + nbiface "github.com/netbirdio/netbird/client/iface" + "github.com/netbirdio/netbird/client/iface/device" + "github.com/netbirdio/netbird/client/iface/wgaddr" +) + +const ( + fragTestSrc = "100.10.0.1" + fragTestDst = "100.10.0.100" + fragTestSrcV6 = "fd00::1" + fragTestDstV6 = "fd00::100" +) + +func newFragmentTestManager(tb testing.TB) *Manager { + tb.Helper() + + ifaceMock := &IFaceMock{ + SetFilterFunc: func(device.PacketFilter) error { return nil }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr(fragTestDst), + Network: netip.MustParsePrefix("100.10.0.0/16"), + IPv6: netip.MustParseAddr(fragTestDstV6), + IPv6Net: netip.MustParsePrefix("fd00::/64"), + } + }, + } + + m, err := Create(ifaceMock, false, flowLogger, nbiface.DefaultMTU) + require.NoError(tb, err) + require.NoError(tb, m.UpdateLocalIPs()) + tb.Cleanup(func() { require.NoError(tb, m.Close(nil)) }) + return m +} + +// firstFragmentUDPTo builds the first fragment of a fragmented UDP datagram to +// the given destination: it carries the full UDP header plus payloadLen bytes +// of data, with the More Fragments flag set and offset zero. +func firstFragmentUDPTo(tb testing.TB, dst string, id uint16, dstPort uint16, payloadLen int) []byte { + tb.Helper() + + ip := &layers.IPv4{ + Version: 4, + TTL: 64, + Id: id, + Protocol: layers.IPProtocolUDP, + SrcIP: net.ParseIP(fragTestSrc), + DstIP: net.ParseIP(dst), + Flags: layers.IPv4MoreFragments, + } + udp := &layers.UDP{SrcPort: 40000, DstPort: layers.UDPPort(dstPort)} + require.NoError(tb, udp.SetNetworkLayerForChecksum(ip)) + + buf := gopacket.NewSerializeBuffer() + opts := gopacket.SerializeOptions{ComputeChecksums: true, FixLengths: true} + require.NoError(tb, gopacket.SerializeLayers(buf, opts, ip, udp, gopacket.Payload(make([]byte, payloadLen)))) + return buf.Bytes() +} + +func firstFragmentUDP(tb testing.TB, id uint16, dstPort uint16, payloadLen int) []byte { + tb.Helper() + return firstFragmentUDPTo(tb, fragTestDst, id, dstPort, payloadLen) +} + +// firstFragmentTCP builds the first fragment of a fragmented TCP datagram: the +// full 20-byte TCP header plus 12 bytes of data, with the More Fragments flag +// set and offset zero. +func firstFragmentTCP(tb testing.TB, id uint16, dstPort uint16) []byte { + tb.Helper() + + ip := &layers.IPv4{ + Version: 4, + TTL: 64, + Id: id, + Protocol: layers.IPProtocolTCP, + SrcIP: net.ParseIP(fragTestSrc), + DstIP: net.ParseIP(fragTestDst), + Flags: layers.IPv4MoreFragments, + } + tcp := &layers.TCP{SrcPort: 40000, DstPort: layers.TCPPort(dstPort), SYN: true, Window: 64240} + require.NoError(tb, tcp.SetNetworkLayerForChecksum(ip)) + + buf := gopacket.NewSerializeBuffer() + opts := gopacket.SerializeOptions{ComputeChecksums: true, FixLengths: true} + require.NoError(tb, gopacket.SerializeLayers(buf, opts, ip, tcp, gopacket.Payload(make([]byte, 12)))) + return buf.Bytes() +} + +// trailingFragmentTo builds a non-first fragment to the given destination: an +// IPv4 header at the given fragment offset (in 8-byte units) carrying raw +// payload and no L4 header. +func trailingFragmentTo(tb testing.TB, dst string, proto layers.IPProtocol, id uint16, fragOffsetOctets uint16, moreFragments bool, payloadLen int) []byte { + tb.Helper() + + ip := &layers.IPv4{ + Version: 4, + TTL: 64, + Id: id, + Protocol: proto, + SrcIP: net.ParseIP(fragTestSrc), + DstIP: net.ParseIP(dst), + FragOffset: fragOffsetOctets, + } + if moreFragments { + ip.Flags = layers.IPv4MoreFragments + } + + buf := gopacket.NewSerializeBuffer() + opts := gopacket.SerializeOptions{FixLengths: true} + require.NoError(tb, gopacket.SerializeLayers(buf, opts, ip, gopacket.Payload(make([]byte, payloadLen)))) + return buf.Bytes() +} + +func trailingFragment(tb testing.TB, id uint16, fragOffsetOctets uint16, moreFragments bool, payloadLen int) []byte { + tb.Helper() + return trailingFragmentTo(tb, fragTestDst, layers.IPProtocolUDP, id, fragOffsetOctets, moreFragments, payloadLen) +} + +// outboundUDPPacket builds a complete outbound UDP packet from the local +// address, used to establish conntrack state for reply-direction tests. +func outboundUDPPacket(tb testing.TB, srcPort, dstPort uint16) []byte { + tb.Helper() + + ip := &layers.IPv4{ + Version: 4, + TTL: 64, + Id: 1, + Protocol: layers.IPProtocolUDP, + SrcIP: net.ParseIP(fragTestDst), + DstIP: net.ParseIP(fragTestSrc), + } + udp := &layers.UDP{SrcPort: layers.UDPPort(srcPort), DstPort: layers.UDPPort(dstPort)} + require.NoError(tb, udp.SetNetworkLayerForChecksum(ip)) + + buf := gopacket.NewSerializeBuffer() + opts := gopacket.SerializeOptions{ComputeChecksums: true, FixLengths: true} + require.NoError(tb, gopacket.SerializeLayers(buf, opts, ip, udp, gopacket.Payload(make([]byte, 16)))) + return buf.Bytes() +} + +// normalUDPPacket builds a complete, non-fragmented UDP packet for baseline +// comparisons against the fragment paths. +func normalUDPPacket(tb testing.TB, dstPort uint16, payloadLen int) []byte { + tb.Helper() + + ip := &layers.IPv4{ + Version: 4, + TTL: 64, + Id: 1, + Protocol: layers.IPProtocolUDP, + SrcIP: net.ParseIP(fragTestSrc), + DstIP: net.ParseIP(fragTestDst), + } + udp := &layers.UDP{SrcPort: 40000, DstPort: layers.UDPPort(dstPort)} + require.NoError(tb, udp.SetNetworkLayerForChecksum(ip)) + + buf := gopacket.NewSerializeBuffer() + opts := gopacket.SerializeOptions{ComputeChecksums: true, FixLengths: true} + require.NoError(tb, gopacket.SerializeLayers(buf, opts, ip, udp, gopacket.Payload(make([]byte, payloadLen)))) + return buf.Bytes() +} + +func allowUDP(tb testing.TB, m *Manager, dstPort uint16) { + tb.Helper() + _, err := m.AddPeerFiltering(nil, net.ParseIP(fragTestSrc), fw.ProtocolUDP, nil, + &fw.Port{Values: []uint16{dstPort}}, fw.ActionAccept, "") + require.NoError(tb, err) +} + +// TestFragment_TrailingWithoutFirstDropped is the core bypass repro: a trailing +// fragment with no allowed first fragment on record must be dropped. Before the +// fix, filterInbound returned false (allow) for any fragment. +func TestFragment_TrailingWithoutFirstDropped(t *testing.T) { + m := newFragmentTestManager(t) + + frag := trailingFragment(t, 0x1234, 185, false, 40) + require.True(t, m.filterInbound(frag, len(frag)), + "trailing fragment without an allowed first fragment must be dropped") +} + +// TestFragment_AllowedFirstPassesTrailing verifies that once a first fragment +// passes the ACL, its trailing fragments inherit the allow verdict. +func TestFragment_AllowedFirstPassesTrailing(t *testing.T) { + m := newFragmentTestManager(t) + allowUDP(t, m, 8080) + + // First fragment: UDP header (8) + 32 payload = 40 octets -> headerEnd = 5. + first := firstFragmentUDP(t, 0x2222, 8080, 32) + require.False(t, m.filterInbound(first, len(first)), + "allowed first fragment should pass and be recorded") + + trailing := trailingFragment(t, 0x2222, 5, false, 24) + require.False(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment of an allowed datagram should pass") +} + +// TestFragment_DeniedFirstDropsTrailing verifies that a first fragment blocked +// by the ACL leaves no verdict, so its trailing fragments are dropped. +func TestFragment_DeniedFirstDropsTrailing(t *testing.T) { + m := newFragmentTestManager(t) + // No accept rule: local traffic defaults to deny. + + first := firstFragmentUDP(t, 0x3333, 9999, 32) + require.True(t, m.filterInbound(first, len(first)), + "first fragment to a blocked port should be dropped by the ACL") + + trailing := trailingFragment(t, 0x3333, 5, false, 24) + require.True(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment of a denied datagram must be dropped") +} + +// TestFragment_OverlappingHeaderDropped covers the RFC 1858 §4 / RFC 3128 +// overlapping-fragment rewrite: a trailing fragment starting inside the range +// the ACL already inspected is dropped and poisons the datagram. TCP is used so +// the overlap lands on real header bytes (the flags at byte 13). +func TestFragment_OverlappingHeaderDropped(t *testing.T) { + m := newFragmentTestManager(t) + _, err := m.AddPeerFiltering(nil, net.ParseIP(fragTestSrc), fw.ProtocolTCP, nil, + &fw.Port{Values: []uint16{8080}}, fw.ActionAccept, "") + require.NoError(t, err) + + // First fragment: TCP header (20) + 12 data = 32 bytes -> headerEnd = 4 octets. + first := firstFragmentTCP(t, 0x4444, 8080) + require.False(t, m.filterInbound(first, len(first))) + + // Overlapping fragment at offset 1 (byte 8) falls inside the inspected TCP + // header, so it could rewrite the flags or port on reassembly. + overlap := trailingFragmentTo(t, fragTestDst, layers.IPProtocolTCP, 0x4444, 1, true, 32) + require.True(t, m.filterInbound(overlap, len(overlap)), + "fragment overlapping the inspected header must be dropped") + + // The datagram is now poisoned: a later, non-overlapping fragment is also + // dropped because the verdict was removed. + later := trailingFragmentTo(t, fragTestDst, layers.IPProtocolTCP, 0x4444, 4, false, 24) + require.True(t, m.filterInbound(later, len(later)), + "fragments after an overlap must be dropped (datagram poisoned)") +} + +// TestFragment_OffsetZeroOverlapPoisons covers the RFC 3128 offset-zero rewrite: +// an allowed first fragment followed by a denied offset-zero fragment for the +// same datagram must not leave the earlier allow verdict in place. +func TestFragment_OffsetZeroOverlapPoisons(t *testing.T) { + m := newFragmentTestManager(t) + allowUDP(t, m, 8080) + + allowed := firstFragmentUDP(t, 0x5A5A, 8080, 32) + require.False(t, m.filterInbound(allowed, len(allowed)), + "allowed first fragment should pass and be recorded") + + // A second offset-zero fragment to a denied port supersedes the datagram's + // verdict; it is dropped and must not leave the allow in place. + denied := firstFragmentUDP(t, 0x5A5A, 9999, 32) + require.True(t, m.filterInbound(denied, len(denied)), + "denied offset-zero fragment must be dropped") + + trailing := trailingFragment(t, 0x5A5A, 5, false, 24) + require.True(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment must be denied after the datagram was poisoned") +} + +// TestFragment_TinyFirstDropped covers the tiny-fragment attack: a first +// fragment too small to contain the full transport header can't be +// ACL-evaluated and must be dropped. +func TestFragment_TinyFirstDropped(t *testing.T) { + m := newFragmentTestManager(t) + allowUDP(t, m, 8080) + + // IPv4 header + 4 raw bytes, MF set, offset 0: too small for the 8-byte UDP + // header, so it decodes to L3 only. + tiny := trailingFragment(t, 0x5555, 0, true, 4) + require.True(t, m.filterInbound(tiny, len(tiny)), + "tiny first fragment without a full L4 header must be dropped") +} + +// TestFragment_TCPFirstFragment verifies the TCP arm of the transport decode: a +// first fragment carrying the full 20-byte TCP header is ACL-evaluated and its +// trailing fragments inherit the verdict. +func TestFragment_TCPFirstFragment(t *testing.T) { + m := newFragmentTestManager(t) + _, err := m.AddPeerFiltering(nil, net.ParseIP(fragTestSrc), fw.ProtocolTCP, nil, + &fw.Port{Values: []uint16{8080}}, fw.ActionAccept, "") + require.NoError(t, err) + + // TCP header (20) + 12 data = 32 bytes -> headerEnd = 4 octets. + first := firstFragmentTCP(t, 0x6666, 8080) + require.False(t, m.filterInbound(first, len(first)), + "allowed TCP first fragment should pass and be recorded") + + trailing := trailingFragmentTo(t, fragTestDst, layers.IPProtocolTCP, 0x6666, 4, false, 24) + require.False(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment of an allowed TCP datagram should pass") +} + +// TestFragment_TCPTinyFirstDropped verifies the TCP minimum header length: 12 +// bytes would satisfy a UDP header but falls short of the 20-byte TCP header. +func TestFragment_TCPTinyFirstDropped(t *testing.T) { + m := newFragmentTestManager(t) + _, err := m.AddPeerFiltering(nil, net.ParseIP(fragTestSrc), fw.ProtocolTCP, nil, + &fw.Port{Values: []uint16{8080}}, fw.ActionAccept, "") + require.NoError(t, err) + + tiny := trailingFragmentTo(t, fragTestDst, layers.IPProtocolTCP, 0x7777, 0, true, 12) + require.True(t, m.filterInbound(tiny, len(tiny)), + "first fragment shorter than the TCP header must be dropped") +} + +// TestFragment_ConntrackAllowsFirstFragment verifies the conntrack branch: reply +// fragments of an outbound-established UDP flow pass without any inbound rule. +func TestFragment_ConntrackAllowsFirstFragment(t *testing.T) { + m := newFragmentTestManager(t) + + out := outboundUDPPacket(t, 12345, 40000) + require.False(t, m.filterOutbound(out, len(out))) + + first := firstFragmentUDP(t, 0x8888, 12345, 32) + require.False(t, m.filterInbound(first, len(first)), + "reply first fragment should pass via conntrack") + + trailing := trailingFragment(t, 0x8888, 5, false, 24) + require.False(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment of a tracked flow should pass") +} + +// TestFragment_RoutingDisabledDropsFragment verifies routed first fragments are +// dropped when routing is disabled. +func TestFragment_RoutingDisabledDropsFragment(t *testing.T) { + m := newFragmentTestManager(t) + m.routingEnabled.Store(false) + + first := firstFragmentUDPTo(t, "198.51.100.10", 0x9999, 8080, 32) + require.True(t, m.filterInbound(first, len(first)), + "routed first fragment must be dropped when routing is disabled") +} + +// TestFragment_RouteACL verifies the route-ACL branch: fragments to a non-local +// destination follow the route rules, allowed datagrams pass their trailing +// fragments and denied ones don't. +func TestFragment_RouteACL(t *testing.T) { + m := newFragmentTestManager(t) + m.routingEnabled.Store(true) + m.nativeRouter.Store(false) + + _, err := m.AddRouteFiltering( + []byte("rt-1"), + []netip.Prefix{netip.MustParsePrefix("100.10.0.0/16")}, + fw.Network{Prefix: netip.MustParsePrefix("198.51.100.0/24")}, + fw.ProtocolUDP, + nil, + &fw.Port{Values: []uint16{8080}}, + fw.ActionAccept, + ) + require.NoError(t, err) + + first := firstFragmentUDPTo(t, "198.51.100.10", 0xAAAA, 8080, 32) + require.False(t, m.filterInbound(first, len(first)), + "route-ACL-allowed first fragment should pass") + trailing := trailingFragmentTo(t, "198.51.100.10", layers.IPProtocolUDP, 0xAAAA, 5, false, 24) + require.False(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment of an allowed routed datagram should pass") + + denied := firstFragmentUDPTo(t, "198.51.100.10", 0xBBBB, 9999, 32) + require.True(t, m.filterInbound(denied, len(denied)), + "route-ACL-denied first fragment must be dropped") + deniedTrailing := trailingFragmentTo(t, "198.51.100.10", layers.IPProtocolUDP, 0xBBBB, 5, false, 24) + require.True(t, m.filterInbound(deniedTrailing, len(deniedTrailing)), + "trailing fragment of a denied routed datagram must be dropped") +} + +// TestFragment_ExpiredVerdictDropsTrailing verifies a verdict older than the +// tracker timeout no longer admits trailing fragments. +func TestFragment_ExpiredVerdictDropsTrailing(t *testing.T) { + m := newFragmentTestManager(t) + allowUDP(t, m, 8080) + + first := firstFragmentUDP(t, 0xCCCC, 8080, 32) + require.False(t, m.filterInbound(first, len(first))) + + m.fragments.mutex.Lock() + for key, entry := range m.fragments.entries { + entry.recordedAt = time.Now().Add(-defaultFragmentTimeout - time.Second) + m.fragments.entries[key] = entry + } + m.fragments.mutex.Unlock() + + trailing := trailingFragment(t, 0xCCCC, 5, false, 24) + require.True(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment after verdict expiry must be dropped") +} + +// TestFragment_CapacityFailsClosed verifies the table cap: at capacity, new +// datagram verdicts are not recorded (their trailing fragments are dropped) +// while already-recorded datagrams keep working. +func TestFragment_CapacityFailsClosed(t *testing.T) { + m := newFragmentTestManager(t) + allowUDP(t, m, 8080) + + m.fragments.mutex.Lock() + m.fragments.maxEntries = 1 + m.fragments.mutex.Unlock() + + first1 := firstFragmentUDP(t, 0x0101, 8080, 32) + require.False(t, m.filterInbound(first1, len(first1))) + + first2 := firstFragmentUDP(t, 0x0202, 8080, 32) + require.False(t, m.filterInbound(first2, len(first2)), + "first fragment itself still passes at capacity") + + trailing2 := trailingFragment(t, 0x0202, 5, false, 24) + require.True(t, m.filterInbound(trailing2, len(trailing2)), + "trailing fragment of an unrecorded datagram must be dropped at capacity") + + trailing1 := trailingFragment(t, 0x0101, 5, false, 24) + require.False(t, m.filterInbound(trailing1, len(trailing1)), + "already-recorded datagram should keep passing at capacity") +} + +// v6FragmentHeader builds the 8-byte IPv6 fragment extension header for the +// given inner protocol, offset (8-byte units), More Fragments bit and id. +func v6FragmentHeader(proto layers.IPProtocol, offsetOctets uint16, moreFragments bool, id uint32) []byte { + offsetFlags := offsetOctets << 3 + if moreFragments { + offsetFlags |= 1 + } + hdr := make([]byte, 8) + hdr[0] = uint8(proto) + binary.BigEndian.PutUint16(hdr[2:4], offsetFlags) + binary.BigEndian.PutUint32(hdr[4:8], id) + return hdr +} + +func v6UDPHeader(dstPort uint16, dataLen int) []byte { + hdr := make([]byte, 8) + binary.BigEndian.PutUint16(hdr[0:2], 40000) + binary.BigEndian.PutUint16(hdr[2:4], dstPort) + binary.BigEndian.PutUint16(hdr[4:6], uint16(8+dataLen)) + return hdr +} + +// firstFragmentUDPv6 builds the first fragment of a fragmented IPv6 UDP +// datagram: fragment header (offset 0, More Fragments set) + full UDP header + +// data. +func firstFragmentUDPv6(tb testing.TB, id uint32, dstPort uint16, dataLen int) []byte { + tb.Helper() + return fragmentUDPv6(tb, id, dstPort, dataLen, true) +} + +// fragmentUDPv6 builds an offset-zero IPv6 UDP fragment. With moreFragments +// false it is an atomic fragment (a complete datagram, RFC 6946). +func fragmentUDPv6(tb testing.TB, id uint32, dstPort uint16, dataLen int, moreFragments bool) []byte { + tb.Helper() + + ip := &layers.IPv6{ + Version: 6, + NextHeader: layers.IPProtocolIPv6Fragment, + HopLimit: 64, + SrcIP: net.ParseIP(fragTestSrcV6), + DstIP: net.ParseIP(fragTestDstV6), + } + payload := append(v6FragmentHeader(layers.IPProtocolUDP, 0, moreFragments, id), v6UDPHeader(dstPort, dataLen)...) + payload = append(payload, make([]byte, dataLen)...) + + buf := gopacket.NewSerializeBuffer() + require.NoError(tb, gopacket.SerializeLayers(buf, gopacket.SerializeOptions{FixLengths: true}, ip, gopacket.Payload(payload))) + return buf.Bytes() +} + +// trailingFragmentV6 builds a non-first IPv6 fragment: fragment header at the +// given offset carrying raw data and no transport header. +func trailingFragmentV6(tb testing.TB, id uint32, offsetOctets uint16, moreFragments bool, dataLen int) []byte { + tb.Helper() + + ip := &layers.IPv6{ + Version: 6, + NextHeader: layers.IPProtocolIPv6Fragment, + HopLimit: 64, + SrcIP: net.ParseIP(fragTestSrcV6), + DstIP: net.ParseIP(fragTestDstV6), + } + payload := append(v6FragmentHeader(layers.IPProtocolUDP, offsetOctets, moreFragments, id), make([]byte, dataLen)...) + + buf := gopacket.NewSerializeBuffer() + require.NoError(tb, gopacket.SerializeLayers(buf, gopacket.SerializeOptions{FixLengths: true}, ip, gopacket.Payload(payload))) + return buf.Bytes() +} + +// TestFragmentV6_TrailingWithoutFirstDropped verifies the IPv6 bypass is closed: +// a trailing fragment with no allowed first fragment is dropped. +func TestFragmentV6_TrailingWithoutFirstDropped(t *testing.T) { + m := newFragmentTestManager(t) + + frag := trailingFragmentV6(t, 0xAABBCCDD, 100, false, 40) + require.True(t, m.filterInbound(frag, len(frag)), + "IPv6 trailing fragment without an allowed first fragment must be dropped") +} + +// TestFragmentV6_AllowedFirstPassesTrailing verifies IPv6 fragments are +// evaluated like IPv4: an allowed first fragment lets its trailing fragments +// through. +func TestFragmentV6_AllowedFirstPassesTrailing(t *testing.T) { + m := newFragmentTestManager(t) + _, err := m.AddPeerFiltering(nil, net.ParseIP(fragTestSrcV6), fw.ProtocolUDP, nil, + &fw.Port{Values: []uint16{8080}}, fw.ActionAccept, "") + require.NoError(t, err) + + // First fragment: UDP header (8) + 32 data = 40 octets -> headerEnd = 5. + first := firstFragmentUDPv6(t, 0xAABBCCDD, 8080, 32) + require.False(t, m.filterInbound(first, len(first)), + "allowed IPv6 first fragment should pass and be recorded") + + trailing := trailingFragmentV6(t, 0xAABBCCDD, 5, false, 24) + require.False(t, m.filterInbound(trailing, len(trailing)), + "trailing fragment of an allowed IPv6 datagram should pass") +} + +// TestFragmentV6_AtomicNotCached verifies an IPv6 atomic fragment (fragment +// header with offset 0 and no More Fragments, a complete datagram per RFC 6946) +// is evaluated but not recorded, so a flood of allowed atomic fragments can't +// exhaust the verdict table. +func TestFragmentV6_AtomicNotCached(t *testing.T) { + m := newFragmentTestManager(t) + _, err := m.AddPeerFiltering(nil, net.ParseIP(fragTestSrcV6), fw.ProtocolUDP, nil, + &fw.Port{Values: []uint16{8080}}, fw.ActionAccept, "") + require.NoError(t, err) + + atomic := fragmentUDPv6(t, 0xA70301C, 8080, 16, false) + require.False(t, m.filterInbound(atomic, len(atomic)), + "allowed IPv6 atomic fragment should pass") + + m.fragments.mutex.Lock() + n := len(m.fragments.entries) + m.fragments.mutex.Unlock() + require.Zero(t, n, "atomic fragment must not create a verdict entry") + + // A genuine fragmented datagram (More Fragments set) is still recorded. + first := fragmentUDPv6(t, 0xBEEF, 8080, 32, true) + require.False(t, m.filterInbound(first, len(first))) + m.fragments.mutex.Lock() + n = len(m.fragments.entries) + m.fragments.mutex.Unlock() + require.Equal(t, 1, n, "genuine first fragment must record a verdict") +}