Merge branch 'main' into feat-post_quantum_ml_kem

# Conflicts:
#	client/internal/peer/conn.go
#	client/internal/peer/handshaker.go

client/internal/daemonaddr/grpc.go

@@ -0,0 +1,42 @@
+package daemonaddr
+
+import (
+	"os"
+	"strconv"
+
+	log "github.com/sirupsen/logrus"
+)
+
+const (
+	// EnvMaxRecvMsgSize overrides the default gRPC max receive message size for
+	// connections to the daemon. Value is in bytes.
+	EnvMaxRecvMsgSize = "NB_DAEMON_GRPC_MAX_MSG_SIZE"
+
+	// defaultMaxRecvMsgSize is the max gRPC receive message size used for daemon
+	// connections when EnvMaxRecvMsgSize is unset or invalid. It overrides the
+	// gRPC library default of 4 MB, which a detailed status already exceeds on a
+	// network of a few thousand peers.
+	defaultMaxRecvMsgSize = 1024 * 1024 * 16
+)
+
+// MaxRecvMsgSize returns the max gRPC receive message size for daemon connections
+// from the environment, or defaultMaxRecvMsgSize (16 MB) if unset or invalid.
+func MaxRecvMsgSize() int {
+	val := os.Getenv(EnvMaxRecvMsgSize)
+	if val == "" {
+		return defaultMaxRecvMsgSize
+	}
+
+	size, err := strconv.Atoi(val)
+	if err != nil {
+		log.Warnf("invalid %s value %q, using default: %v", EnvMaxRecvMsgSize, val, err)
+		return defaultMaxRecvMsgSize
+	}
+
+	if size <= 0 {
+		log.Warnf("invalid %s value %d, must be positive, using default", EnvMaxRecvMsgSize, size)
+		return defaultMaxRecvMsgSize
+	}
+
+	return size
+}

client/internal/daemonaddr/grpc_test.go

@@ -0,0 +1,112 @@
+package daemonaddr
+
+import (
+	"context"
+	"net"
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/status"
+
+	"github.com/netbirdio/netbird/client/proto"
+)
+
+func TestMaxRecvMsgSize(t *testing.T) {
+	tests := []struct {
+		name     string
+		envValue string
+		expected int
+	}{
+		{name: "unset returns default", envValue: "", expected: defaultMaxRecvMsgSize},
+		{name: "non-numeric returns default", envValue: "abc", expected: defaultMaxRecvMsgSize},
+		{name: "negative returns default", envValue: "-1", expected: defaultMaxRecvMsgSize},
+		{name: "zero returns default", envValue: "0", expected: defaultMaxRecvMsgSize},
+		{name: "valid value is used", envValue: "33554432", expected: 33554432},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Set first so the previous value is restored on cleanup, then unset to
+			// exercise the absent case.
+			t.Setenv(EnvMaxRecvMsgSize, tc.envValue)
+			if tc.envValue == "" {
+				require.NoError(t, os.Unsetenv(EnvMaxRecvMsgSize), "unset the override")
+			}
+
+			assert.Equal(t, tc.expected, MaxRecvMsgSize(), "max receive message size")
+		})
+	}
+}
+
+// bigStatusServer answers Status with a response larger than gRPC's 4 MB default
+// receive limit, which is what a detailed status on a large network looks like.
+type bigStatusServer struct {
+	proto.UnimplementedDaemonServiceServer
+	payload string
+}
+
+func (s *bigStatusServer) Status(context.Context, *proto.StatusRequest) (*proto.StatusResponse, error) {
+	return &proto.StatusResponse{Status: s.payload}, nil
+}
+
+func startBigStatusServer(t *testing.T, payload string) string {
+	t.Helper()
+
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err, "listen on loopback")
+
+	srv := grpc.NewServer()
+	proto.RegisterDaemonServiceServer(srv, &bigStatusServer{payload: payload})
+	go func() {
+		_ = srv.Serve(listener)
+	}()
+	t.Cleanup(srv.Stop)
+
+	return "tcp://" + listener.Addr().String()
+}
+
+func TestDialTargetAcceptsAStatusOverTheGrpcDefault(t *testing.T) {
+	payload := strings.Repeat("x", 5*1024*1024)
+	addr := startBigStatusServer(t, payload)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	target, opts := DialTarget(addr)
+	conn, err := grpc.NewClient(target, opts...)
+	require.NoError(t, err, "dial the daemon")
+	t.Cleanup(func() { _ = conn.Close() })
+
+	resp, err := proto.NewDaemonServiceClient(conn).Status(ctx, &proto.StatusRequest{})
+	require.NoError(t, err, "a detailed status must not be rejected for its size")
+	assert.Len(t, resp.GetStatus(), len(payload), "the whole response must arrive")
+}
+
+// TestDialTargetRaisesTheDefaultLimit is the negative control: the same response
+// over a connection carrying gRPC's own defaults is refused, which is the failure
+// reported by `netbird status -d` on a large deployment.
+func TestDialTargetRaisesTheDefaultLimit(t *testing.T) {
+	payload := strings.Repeat("x", 5*1024*1024)
+	addr := startBigStatusServer(t, payload)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	conn, err := grpc.NewClient(
+		strings.TrimPrefix(addr, "tcp://"),
+		grpc.WithTransportCredentials(insecure.NewCredentials()),
+	)
+	require.NoError(t, err, "dial with the library defaults")
+	t.Cleanup(func() { _ = conn.Close() })
+
+	_, err = proto.NewDaemonServiceClient(conn).Status(ctx, &proto.StatusRequest{})
+	require.Error(t, err, "the library default must reject this response")
+	assert.Equal(t, codes.ResourceExhausted, status.Code(err), "gRPC rejects an oversized message")
+}

client/internal/daemonaddr/pipe.go

@@ -36,7 +36,10 @@ const (
 // address. The npipe scheme needs a context dialer because gRPC has no
 // named-pipe resolver; unix and tcp are handled by gRPC itself.
 func DialTarget(addr string) (string, []grpc.DialOption) {
-	opts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}
+	opts := []grpc.DialOption{
+		grpc.WithTransportCredentials(insecure.NewCredentials()),
+		grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(MaxRecvMsgSize())),
+	}
 
 	if name, ok := strings.CutPrefix(addr, pipeScheme); ok {
 		paths := PipePaths(name)

client/internal/dns/host_windows.go

@@ -124,19 +124,9 @@ func newHostManager(wgInterface WGIface) (*registryConfigurator, error) {
 		return nil, err
 	}
 
-	var useGPO bool
-	k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE)
-	if err != nil {
-		log.Debugf("failed to open GPO DNS policy root: %v", err)
-	} else {
-		closer(k)
-		useGPO = true
-		log.Infof("detected GPO DNS policy configuration, using policy store")
-	}
-
 	configurator := &registryConfigurator{
 		guid: guid,
-		gpo:  useGPO,
+		gpo:  useGPOPolicyStore(),
 	}
 
 	origNameservers, err := configurator.captureOriginalNameservers()
@@ -576,14 +566,22 @@ func (r *registryConfigurator) setInterfaceRegistryKeyStringValue(key, value str
 	return nil
 }
 
+// deleteInterfaceRegistryKeyProperty removes a value from the interface key.
+// A value that is already gone, or an interface key that is, is not an error:
+// the caller asked for the value not to be there, and a cleanup that runs twice
+// has to reach its later steps on the second run as well.
 func (r *registryConfigurator) deleteInterfaceRegistryKeyProperty(propertyKey string) error {
 	regKey, err := r.getInterfaceRegistryKey()
-	if err != nil {
+	switch {
+	case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND):
+		log.Debugf("interface key of %s does not exist, nothing to delete %s from", r.guid, propertyKey)
+		return nil
+	case err != nil:
 		return fmt.Errorf("get interface registry key: %w", err)
 	}
 	defer closer(regKey)
 
-	if err := regKey.DeleteValue(propertyKey); err != nil {
+	if err := regKey.DeleteValue(propertyKey); err != nil && !errors.Is(err, registry.ErrNotExist) {
 		return fmt.Errorf("delete registry key %s: %w", propertyKey, err)
 	}
 	return nil
@@ -612,7 +610,12 @@ func (r *registryConfigurator) restoreHostDNS() error {
 
 	go r.flushDNSCache()
 
-	return nil
+	// Last, and only on the way out, once no rule of ours is left: during a
+	// session the store is where the rules of this run live, and emptying it
+	// mid-session would have the next rule recreate it anyway. Propagated so a
+	// failure keeps the shutdown state for the next run to retry, rather than
+	// leaving the store to hold up every rule change from here on.
+	return removeEmptyGPOPolicyStore()
 }
 
 // removeDNSMatchPolicies deletes every NRPT rule this client may have created,
@@ -651,6 +654,73 @@ func (r *registryConfigurator) restoreUncleanShutdownDNS() error {
 	return r.restoreHostDNS()
 }
 
+// useGPOPolicyStore reports whether NRPT rules have to go into the group policy
+// store, and clears an empty one out of the way first.
+//
+// The order is the point. A store left empty by an earlier run would otherwise
+// decide this run too, sending its rules somewhere the resolver only reads when
+// the policy engine next applies DNS client policy. Removing it before the
+// choice is made leaves the local store authoritative for the whole session,
+// including the first one after an upgrade.
+func useGPOPolicyStore() bool {
+	if err := removeEmptyGPOPolicyStore(); err != nil {
+		// Nothing to retry against here: the worst case is the run going
+		// through the group policy store, which is where it would have gone
+		// before this check existed.
+		log.Warnf("%v", err)
+	}
+
+	k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE)
+	if err != nil {
+		log.Debugf("failed to open GPO DNS policy root: %v", err)
+		return false
+	}
+	closer(k)
+
+	log.Infof("detected GPO DNS policy configuration, using policy store")
+	return true
+}
+
+// removeEmptyGPOPolicyStore deletes the group policy DnsPolicyConfig key once
+// nothing is left in it. The key survives the deletion of the last rule it
+// held, and the client treats its presence as "group policy configures the
+// NRPT", so an empty one left behind keeps every later run writing rules there.
+// Rules in that store reach the resolver only when the policy engine next
+// applies DNS client policy, and a rule this client writes belongs to no GPO,
+// so nothing schedules that application: both adding and removing a rule are
+// held up by a minute or more, and for a removal that is a catch-all rule
+// resolving every name over an interface that no longer exists. With the store
+// absent the local one is authoritative and a change applies at once.
+//
+// A store that still holds rules, values or subkeys of somebody else's is left
+// alone.
+func removeEmptyGPOPolicyStore() error {
+	k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE)
+	switch {
+	case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND):
+		return nil
+	case err != nil:
+		return fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err)
+	}
+
+	info, err := k.Stat()
+	closer(k)
+	if err != nil {
+		return fmt.Errorf("stat HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err)
+	}
+
+	if info.SubKeyCount != 0 || info.ValueCount != 0 {
+		return nil
+	}
+
+	if err := registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot); err != nil {
+		return fmt.Errorf("delete empty HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err)
+	}
+
+	log.Infof("removed the empty GPO DNS policy store, leaving the local one authoritative")
+	return nil
+}
+
 // listNRPTRuleKeys returns the names of our NRPT rule keys under a policy store
 // root. An absent root holds nothing to clean up, which is the normal state of
 // the GPO store on a machine without DNS Client policy.

client/internal/dns/host_windows_test.go

@@ -8,6 +8,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sys/windows/registry"
+
+	"github.com/netbirdio/netbird/client/internal/winregistry"
 )
 
 // TestNRPTEntriesCleanupOnConfigChange tests that old NRPT entries are properly cleaned up
@@ -405,3 +407,130 @@ func TestNRPTDomainBatching(t *testing.T) {
 		})
 	}
 }
+
+// TestRemoveEmptyGPOPolicyStore verifies that cleanup takes the GPO policy
+// store itself with it once our rules are gone, since the store existing keeps
+// the local one from being applied, and that a store with somebody else's rule
+// in it is left alone.
+func TestRemoveEmptyGPOPolicyStore(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping registry integration test in short mode")
+	}
+
+	t.Cleanup(func() { cleanupRegistryKeys(t) })
+	cleanupRegistryKeys(t)
+
+	testIP := netip.MustParseAddr("100.64.0.1")
+	cfg := &registryConfigurator{gpo: true}
+
+	// a store holding a rule of ours is kept, because the rule is still applied
+	require.NoError(t, cfg.addDNSMatchPolicy([]string{".example.com"}, testIP))
+	exists, err := registryKeyExists(gpoDnsPolicyConfigMatchPath + "-0")
+	require.NoError(t, err)
+	require.True(t, exists, "Should write the rule to the GPO policy store")
+
+	require.NoError(t, removeEmptyGPOPolicyStore())
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should keep a policy store that still holds a rule")
+
+	// once the rules are gone the store goes with them
+	require.NoError(t, cfg.removeDNSMatchPolicies())
+	require.NoError(t, removeEmptyGPOPolicyStore())
+
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.False(t, exists, "Should remove the GPO policy store once it is empty")
+
+	// A store is not ours to remove while somebody else has a rule in it. The
+	// rule is written volatile like our own: the rules above created the parent
+	// chain volatile, and Windows refuses a stable subkey under a volatile
+	// parent.
+	foreignRule := GPODNSPolicyConfigRoot + `\{2A3B4C5D-6E7F-4041-8283-84858687888A}`
+	foreignKey, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, foreignRule, registry.SET_VALUE)
+	require.NoError(t, err, "Should create a foreign GPO rule")
+	foreignKey.Close()
+	t.Cleanup(func() {
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignRule)
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot)
+	})
+
+	require.NoError(t, cfg.removeDNSMatchPolicies())
+	require.NoError(t, removeEmptyGPOPolicyStore())
+
+	exists, err = registryKeyExists(foreignRule)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should not remove a foreign rule")
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should keep a policy store that still holds a foreign rule")
+}
+
+// TestDeleteInterfaceRegistryKeyPropertyTwice verifies that removing a value
+// that is already gone, or one on an interface key that is, reports success.
+// Teardown runs again after a failed cleanup, and the steps that follow this
+// one have to be reached on that second run.
+func TestDeleteInterfaceRegistryKeyPropertyTwice(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping registry integration test in short mode")
+	}
+
+	testGUID := "{12345678-1234-1234-1234-123456789ABC}"
+	interfacePath := InterfaceConfigPath + `\` + testGUID
+	testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE)
+	require.NoError(t, err, "Should create test interface registry key")
+	testKey.Close()
+	t.Cleanup(func() {
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath)
+	})
+
+	cfg := &registryConfigurator{guid: testGUID}
+
+	require.NoError(t, cfg.setInterfaceRegistryKeyStringValue(interfaceConfigSearchListKey, "example.com"))
+	require.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey))
+	assert.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey),
+		"Should report success for a value that is already gone")
+
+	// and with the interface key itself gone, as it is once the adapter is
+	require.NoError(t, registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath))
+	assert.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey),
+		"Should report success when the interface key does not exist")
+}
+
+// TestUseGPOPolicyStoreClearsEmptyStore verifies that the store is cleared
+// before it is consulted, so an empty one left by an earlier run does not send
+// this run's rules to the group policy store. A store somebody else has a rule
+// in still decides where the rules go.
+func TestUseGPOPolicyStoreClearsEmptyStore(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping registry integration test in short mode")
+	}
+
+	t.Cleanup(func() { cleanupRegistryKeys(t) })
+	cleanupRegistryKeys(t)
+
+	// the leftover an earlier run used to keep, which the client read as
+	// "group policy configures the NRPT" for every run after it
+	emptyStore, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.SET_VALUE)
+	require.NoError(t, err, "Should create the GPO policy store")
+	emptyStore.Close()
+
+	assert.False(t, useGPOPolicyStore(), "An empty store should not decide where the rules go")
+	exists, err := registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.False(t, exists, "Should clear the empty store before consulting it")
+
+	foreignRule := GPODNSPolicyConfigRoot + `\{2A3B4C5D-6E7F-4041-8283-84858687888A}`
+	foreignKey, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, foreignRule, registry.SET_VALUE)
+	require.NoError(t, err, "Should create a foreign GPO rule")
+	foreignKey.Close()
+	t.Cleanup(func() {
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignRule)
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot)
+	})
+
+	assert.True(t, useGPOPolicyStore(), "A store holding a rule should decide where the rules go")
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should keep a store that holds a rule")
+}

client/internal/ebpf/ebpf/bpf_bpfeb.go

@@ -1,148 +0,0 @@
-// Code generated by bpf2go; DO NOT EDIT.
-//go:build mips || mips64 || ppc64 || s390x
-
-package ebpf
-
-import (
-	"bytes"
-	_ "embed"
-	"fmt"
-	"io"
-
-	"github.com/cilium/ebpf"
-)
-
-// loadBpf returns the embedded CollectionSpec for bpf.
-func loadBpf() (*ebpf.CollectionSpec, error) {
-	reader := bytes.NewReader(_BpfBytes)
-	spec, err := ebpf.LoadCollectionSpecFromReader(reader)
-	if err != nil {
-		return nil, fmt.Errorf("can't load bpf: %w", err)
-	}
-
-	return spec, err
-}
-
-// loadBpfObjects loads bpf and converts it into a struct.
-//
-// The following types are suitable as obj argument:
-//
-//	*bpfObjects
-//	*bpfPrograms
-//	*bpfMaps
-//
-// See ebpf.CollectionSpec.LoadAndAssign documentation for details.
-func loadBpfObjects(obj interface{}, opts *ebpf.CollectionOptions) error {
-	spec, err := loadBpf()
-	if err != nil {
-		return err
-	}
-
-	return spec.LoadAndAssign(obj, opts)
-}
-
-// bpfSpecs contains maps and programs before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfSpecs struct {
-	bpfProgramSpecs
-	bpfMapSpecs
-	bpfVariableSpecs
-}
-
-// bpfProgramSpecs contains programs before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfProgramSpecs struct {
-	NbXdpProg *ebpf.ProgramSpec `ebpf:"nb_xdp_prog"`
-}
-
-// bpfMapSpecs contains maps before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfMapSpecs struct {
-	NbFeatures           *ebpf.MapSpec `ebpf:"nb_features"`
-	NbWgProxySettingsMap *ebpf.MapSpec `ebpf:"nb_wg_proxy_settings_map"`
-}
-
-// bpfVariableSpecs contains global variables before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfVariableSpecs struct {
-	FlagFeatureWgProxy *ebpf.VariableSpec `ebpf:"flag_feature_wg_proxy"`
-	MapKeyFeatures     *ebpf.VariableSpec `ebpf:"map_key_features"`
-	MapKeyProxyPort    *ebpf.VariableSpec `ebpf:"map_key_proxy_port"`
-	MapKeyWgPort       *ebpf.VariableSpec `ebpf:"map_key_wg_port"`
-	ProxyPort          *ebpf.VariableSpec `ebpf:"proxy_port"`
-	WgPort             *ebpf.VariableSpec `ebpf:"wg_port"`
-}
-
-// bpfObjects contains all objects after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfObjects struct {
-	bpfPrograms
-	bpfMaps
-	bpfVariables
-}
-
-func (o *bpfObjects) Close() error {
-	return _BpfClose(
-		&o.bpfPrograms,
-		&o.bpfMaps,
-	)
-}
-
-// bpfMaps contains all maps after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfMaps struct {
-	NbFeatures           *ebpf.Map `ebpf:"nb_features"`
-	NbWgProxySettingsMap *ebpf.Map `ebpf:"nb_wg_proxy_settings_map"`
-}
-
-func (m *bpfMaps) Close() error {
-	return _BpfClose(
-		m.NbFeatures,
-		m.NbWgProxySettingsMap,
-	)
-}
-
-// bpfVariables contains all global variables after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfVariables struct {
-	FlagFeatureWgProxy *ebpf.Variable `ebpf:"flag_feature_wg_proxy"`
-	MapKeyFeatures     *ebpf.Variable `ebpf:"map_key_features"`
-	MapKeyProxyPort    *ebpf.Variable `ebpf:"map_key_proxy_port"`
-	MapKeyWgPort       *ebpf.Variable `ebpf:"map_key_wg_port"`
-	ProxyPort          *ebpf.Variable `ebpf:"proxy_port"`
-	WgPort             *ebpf.Variable `ebpf:"wg_port"`
-}
-
-// bpfPrograms contains all programs after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfPrograms struct {
-	NbXdpProg *ebpf.Program `ebpf:"nb_xdp_prog"`
-}
-
-func (p *bpfPrograms) Close() error {
-	return _BpfClose(
-		p.NbXdpProg,
-	)
-}
-
-func _BpfClose(closers ...io.Closer) error {
-	for _, closer := range closers {
-		if err := closer.Close(); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// Do not access this directly.
-//
-//go:embed bpf_bpfeb.o
-var _BpfBytes []byte

client/internal/ebpf/ebpf/bpf_bpfel.go

@@ -1,148 +0,0 @@
-// Code generated by bpf2go; DO NOT EDIT.
-//go:build 386 || amd64 || arm || arm64 || loong64 || mips64le || mipsle || ppc64le || riscv64 || wasm
-
-package ebpf
-
-import (
-	"bytes"
-	_ "embed"
-	"fmt"
-	"io"
-
-	"github.com/cilium/ebpf"
-)
-
-// loadBpf returns the embedded CollectionSpec for bpf.
-func loadBpf() (*ebpf.CollectionSpec, error) {
-	reader := bytes.NewReader(_BpfBytes)
-	spec, err := ebpf.LoadCollectionSpecFromReader(reader)
-	if err != nil {
-		return nil, fmt.Errorf("can't load bpf: %w", err)
-	}
-
-	return spec, err
-}
-
-// loadBpfObjects loads bpf and converts it into a struct.
-//
-// The following types are suitable as obj argument:
-//
-//	*bpfObjects
-//	*bpfPrograms
-//	*bpfMaps
-//
-// See ebpf.CollectionSpec.LoadAndAssign documentation for details.
-func loadBpfObjects(obj interface{}, opts *ebpf.CollectionOptions) error {
-	spec, err := loadBpf()
-	if err != nil {
-		return err
-	}
-
-	return spec.LoadAndAssign(obj, opts)
-}
-
-// bpfSpecs contains maps and programs before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfSpecs struct {
-	bpfProgramSpecs
-	bpfMapSpecs
-	bpfVariableSpecs
-}
-
-// bpfProgramSpecs contains programs before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfProgramSpecs struct {
-	NbXdpProg *ebpf.ProgramSpec `ebpf:"nb_xdp_prog"`
-}
-
-// bpfMapSpecs contains maps before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfMapSpecs struct {
-	NbFeatures           *ebpf.MapSpec `ebpf:"nb_features"`
-	NbWgProxySettingsMap *ebpf.MapSpec `ebpf:"nb_wg_proxy_settings_map"`
-}
-
-// bpfVariableSpecs contains global variables before they are loaded into the kernel.
-//
-// It can be passed ebpf.CollectionSpec.Assign.
-type bpfVariableSpecs struct {
-	FlagFeatureWgProxy *ebpf.VariableSpec `ebpf:"flag_feature_wg_proxy"`
-	MapKeyFeatures     *ebpf.VariableSpec `ebpf:"map_key_features"`
-	MapKeyProxyPort    *ebpf.VariableSpec `ebpf:"map_key_proxy_port"`
-	MapKeyWgPort       *ebpf.VariableSpec `ebpf:"map_key_wg_port"`
-	ProxyPort          *ebpf.VariableSpec `ebpf:"proxy_port"`
-	WgPort             *ebpf.VariableSpec `ebpf:"wg_port"`
-}
-
-// bpfObjects contains all objects after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfObjects struct {
-	bpfPrograms
-	bpfMaps
-	bpfVariables
-}
-
-func (o *bpfObjects) Close() error {
-	return _BpfClose(
-		&o.bpfPrograms,
-		&o.bpfMaps,
-	)
-}
-
-// bpfMaps contains all maps after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfMaps struct {
-	NbFeatures           *ebpf.Map `ebpf:"nb_features"`
-	NbWgProxySettingsMap *ebpf.Map `ebpf:"nb_wg_proxy_settings_map"`
-}
-
-func (m *bpfMaps) Close() error {
-	return _BpfClose(
-		m.NbFeatures,
-		m.NbWgProxySettingsMap,
-	)
-}
-
-// bpfVariables contains all global variables after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfVariables struct {
-	FlagFeatureWgProxy *ebpf.Variable `ebpf:"flag_feature_wg_proxy"`
-	MapKeyFeatures     *ebpf.Variable `ebpf:"map_key_features"`
-	MapKeyProxyPort    *ebpf.Variable `ebpf:"map_key_proxy_port"`
-	MapKeyWgPort       *ebpf.Variable `ebpf:"map_key_wg_port"`
-	ProxyPort          *ebpf.Variable `ebpf:"proxy_port"`
-	WgPort             *ebpf.Variable `ebpf:"wg_port"`
-}
-
-// bpfPrograms contains all programs after they have been loaded into the kernel.
-//
-// It can be passed to loadBpfObjects or ebpf.CollectionSpec.LoadAndAssign.
-type bpfPrograms struct {
-	NbXdpProg *ebpf.Program `ebpf:"nb_xdp_prog"`
-}
-
-func (p *bpfPrograms) Close() error {
-	return _BpfClose(
-		p.NbXdpProg,
-	)
-}
-
-func _BpfClose(closers ...io.Closer) error {
-	for _, closer := range closers {
-		if err := closer.Close(); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// Do not access this directly.
-//
-//go:embed bpf_bpfel.o
-var _BpfBytes []byte

client/internal/ebpf/ebpf/manager_linux.go

@@ -1,115 +0,0 @@
-package ebpf
-
-import (
-	_ "embed"
-	"net"
-	"sync"
-
-	"github.com/cilium/ebpf/link"
-	"github.com/cilium/ebpf/rlimit"
-	log "github.com/sirupsen/logrus"
-
-	"github.com/netbirdio/netbird/client/internal/ebpf/manager"
-)
-
-const (
-	mapKeyFeatures uint32 = 0
-
-	featureFlagWGProxy = 0b00000001
-)
-
-var (
-	singleton     manager.Manager
-	singletonLock = &sync.Mutex{}
-)
-
-// required packages libbpf-dev, libc6-dev-i386-amd64-cross
-
-// GeneralManager is used to load multiple eBPF programs with a custom check (if then) done in prog.c
-// The manager simply adds a feature (byte) of each program to a map that is shared between the userspace and kernel.
-// When packet arrives, the C code checks for each feature (if it is set) and executes each enabled program (e.g., wg_proxy.c).
-//
-//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -cc clang-14 bpf src/prog.c -- -I /usr/x86_64-linux-gnu/include -include src/bpf_map_def.h
-type GeneralManager struct {
-	lock         sync.Mutex
-	link         link.Link
-	featureFlags uint16
-	bpfObjs      bpfObjects
-}
-
-// GetEbpfManagerInstance return a static eBpf Manager instance
-func GetEbpfManagerInstance() manager.Manager {
-	singletonLock.Lock()
-	defer singletonLock.Unlock()
-	if singleton != nil {
-		return singleton
-	}
-	singleton = &GeneralManager{}
-	return singleton
-}
-
-func (tf *GeneralManager) setFeatureFlag(feature uint16) {
-	tf.featureFlags |= feature
-}
-
-func (tf *GeneralManager) loadXdp() error {
-	if tf.link != nil {
-		return nil
-	}
-	// it required for Docker
-	err := rlimit.RemoveMemlock()
-	if err != nil {
-		return err
-	}
-
-	iFace, err := net.InterfaceByName("lo")
-	if err != nil {
-		return err
-	}
-
-	// load pre-compiled programs into the kernel.
-	err = loadBpfObjects(&tf.bpfObjs, nil)
-	if err != nil {
-		return err
-	}
-
-	tf.link, err = link.AttachXDP(link.XDPOptions{
-		Program:   tf.bpfObjs.NbXdpProg,
-		Interface: iFace.Index,
-	})
-
-	if err != nil {
-		_ = tf.bpfObjs.Close()
-		tf.link = nil
-		return err
-	}
-	return nil
-}
-
-func (tf *GeneralManager) unsetFeatureFlag(feature uint16) error {
-	tf.lock.Lock()
-	defer tf.lock.Unlock()
-	tf.featureFlags &^= feature
-
-	if tf.link == nil {
-		return nil
-	}
-
-	if tf.featureFlags == 0 {
-		return tf.close()
-	}
-
-	return tf.bpfObjs.NbFeatures.Put(mapKeyFeatures, tf.featureFlags)
-}
-
-func (tf *GeneralManager) close() error {
-	log.Debugf("detach ebpf program ")
-	err := tf.bpfObjs.Close()
-	if err != nil {
-		log.Warnf("failed to close eBpf objects: %s", err)
-	}
-
-	err = tf.link.Close()
-	tf.link = nil
-	return err
-}

client/internal/ebpf/ebpf/manager_linux_test.go

@@ -1,31 +0,0 @@
-package ebpf
-
-import (
-	"testing"
-)
-
-func TestManager_setFeatureFlag(t *testing.T) {
-	mgr := GeneralManager{}
-	mgr.setFeatureFlag(featureFlagWGProxy)
-	if mgr.featureFlags != featureFlagWGProxy {
-		t.Errorf("invalid feature state")
-	}
-
-	mgr.setFeatureFlag(featureFlagWGProxy)
-	if mgr.featureFlags != featureFlagWGProxy {
-		t.Errorf("setting a flag twice must be idempotent, got: %d", mgr.featureFlags)
-	}
-}
-
-func TestManager_unsetFeatureFlag(t *testing.T) {
-	mgr := GeneralManager{}
-	mgr.setFeatureFlag(featureFlagWGProxy)
-
-	err := mgr.unsetFeatureFlag(featureFlagWGProxy)
-	if err != nil {
-		t.Errorf("unexpected error: %s", err)
-	}
-	if mgr.featureFlags != 0 {
-		t.Errorf("invalid feature state, expected: %d, got: %d", 0, mgr.featureFlags)
-	}
-}

client/internal/ebpf/ebpf/src/bpf_map_def.h

@@ -1,16 +0,0 @@
-// libbpf 1.0 removed struct bpf_map_def, but the programs here keep the legacy
-// map definitions: they load on kernels built without BTF, which BTF-style
-// (SEC(".maps")) definitions do not. Define the struct ourselves so the
-// programs compile against current libbpf headers.
-#ifndef NB_BPF_MAP_DEF_H
-#define NB_BPF_MAP_DEF_H
-
-struct bpf_map_def {
-	unsigned int type;
-	unsigned int key_size;
-	unsigned int value_size;
-	unsigned int max_entries;
-	unsigned int map_flags;
-};
-
-#endif

client/internal/ebpf/ebpf/src/prog.c

@@ -1,54 +0,0 @@
-#include <stdbool.h>
-#include <linux/if_ether.h> // ETH_P_IP
-#include <linux/udp.h>
-#include <linux/ip.h>
-#include <netinet/in.h>
-#include <linux/bpf.h>
-#include <bpf/bpf_helpers.h>
-#include "wg_proxy.c"
-
-const __u16 flag_feature_wg_proxy = 0b01;
-
-const __u32 map_key_features = 0;
-struct bpf_map_def SEC("maps") nb_features = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(__u16),
-	.max_entries = 10,
-};
-
-SEC("xdp")
-int nb_xdp_prog(struct xdp_md *ctx) {
-    __u16 *features;
-    features = bpf_map_lookup_elem(&nb_features, &map_key_features);
-    if (!features) {
-        return XDP_PASS;
-    }
-
-    void *data     = (void *)(long)ctx->data;
-    void *data_end = (void *)(long)ctx->data_end;
-    struct ethhdr *eth = data;
-    struct iphdr  *ip  = (data + sizeof(struct ethhdr));
-    struct udphdr *udp = (data + sizeof(struct ethhdr) + sizeof(struct iphdr));
-
-    // return early if not enough data
-    if (data + sizeof(struct ethhdr) + sizeof(struct iphdr) + sizeof(struct udphdr) > data_end){
-        return XDP_PASS;
-    }
-
-    // skip non IPv4 packages
-    if (eth->h_proto != htons(ETH_P_IP)) {
-       return XDP_PASS;
-    }
-
-    // skip non UPD packages
-    if (ip->protocol != IPPROTO_UDP) {
-       return XDP_PASS;
-    }
-
-    if (*features & flag_feature_wg_proxy) {
-        xdp_wg_proxy(ip, udp);
-    }
-    return XDP_PASS;
-}
-char _license[] SEC("license") = "GPL";

client/internal/ebpf/ebpf/src/readme.md

@@ -1,27 +0,0 @@
-# XDP programs
-
-`prog.c` is attached to the `lo` device and dispatches to the features enabled in the
-`nb_features` map. The only feature is the WireGuard proxy (`wg_proxy.c`): it rewrites
-loopback UDP sent from the WireGuard listen port so it reaches the userspace relay proxy
-port instead, and swaps the peer endpoint port into the source so the proxy can tell
-peers apart.
-
-Maps use the legacy `struct bpf_map_def` form, defined in `bpf_map_def.h` because libbpf
-1.0 removed it. They load on kernels built without BTF, which BTF-style (`SEC(".maps")`)
-definitions do not.
-
-Regenerate the objects with `go generate ./client/internal/ebpf/ebpf/`; it needs
-`clang-14`. Loading a regenerated object needs root, attaching it needs `bpf_link`
-(kernel >= 5.7), and only one XDP program can own `lo` at a time.
-
-# Debug
-
-The CONFIG_BPF_EVENTS kernel module is required for bpf_printk.
-Apply this code to use bpf_printk
-```
-#define bpf_printk(fmt, ...)                                                   \
-  ({                                                                           \
-    char ____fmt[] = fmt;                                                      \
-    bpf_trace_printk(____fmt, sizeof(____fmt), ##__VA_ARGS__);                 \
-  })
-```

client/internal/ebpf/ebpf/src/wg_proxy.c

@@ -1,60 +0,0 @@
-const __u32 map_key_proxy_port = 0;
-const __u32 map_key_wg_port = 1;
-
-struct bpf_map_def SEC("maps") nb_wg_proxy_settings_map = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(__u16),
-	.max_entries = 10,
-};
-
-__u16 proxy_port = 0;
-__u16 wg_port = 0;
-
-bool read_port_settings() {
-    __u16 *value;
-    value = bpf_map_lookup_elem(&nb_wg_proxy_settings_map, &map_key_proxy_port);
-    if (!value) {
-        return false;
-    }
-
-    proxy_port = *value;
-
-    value = bpf_map_lookup_elem(&nb_wg_proxy_settings_map, &map_key_wg_port);
-    if (!value) {
-        return false;
-    }
-    wg_port = htons(*value);
-
-    return true;
-}
-
-int xdp_wg_proxy(struct iphdr  *ip, struct udphdr *udp) {
-    if (proxy_port == 0 || wg_port == 0) {
-        if (!read_port_settings()){
-            return XDP_PASS;
-        }
-        // bpf_printk("proxy port: %d, wg port: %d", proxy_port, wg_port);
-    }
-
-    // 2130706433 = 127.0.0.1
-    if (ip->daddr != htonl(2130706433)) {
-        return XDP_PASS;
-    }
-
-    if (udp->source != wg_port){
-        return XDP_PASS;
-    }
-
-    __be16 new_src_port = udp->dest;
-    __be16 new_dst_port = htons(proxy_port);
-    udp->dest = new_dst_port;
-    udp->source = new_src_port;
-
-    // The ports are covered by the UDP checksum. This is an IPv4 loopback hop
-    // and the payload is already integrity-protected, so clear the checksum (a
-    // zero UDP checksum means "not computed" for IPv4) rather than leave a
-    // stale value the kernel would drop as UDP_CSUM.
-    udp->check = 0;
-	return XDP_PASS;
-}

client/internal/ebpf/ebpf/wg_proxy_linux.go

@@ -1,41 +0,0 @@
-package ebpf
-
-import log "github.com/sirupsen/logrus"
-
-const (
-	mapKeyProxyPort uint32 = 0
-	mapKeyWgPort    uint32 = 1
-)
-
-func (tf *GeneralManager) LoadWgProxy(proxyPort, wgPort int) error {
-	log.Debugf("load ebpf WG proxy")
-	tf.lock.Lock()
-	defer tf.lock.Unlock()
-
-	err := tf.loadXdp()
-	if err != nil {
-		return err
-	}
-
-	err = tf.bpfObjs.NbWgProxySettingsMap.Put(mapKeyProxyPort, uint16(proxyPort))
-	if err != nil {
-		return err
-	}
-
-	err = tf.bpfObjs.NbWgProxySettingsMap.Put(mapKeyWgPort, uint16(wgPort))
-	if err != nil {
-		return err
-	}
-
-	tf.setFeatureFlag(featureFlagWGProxy)
-	err = tf.bpfObjs.NbFeatures.Put(mapKeyFeatures, tf.featureFlags)
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-func (tf *GeneralManager) FreeWGProxy() error {
-	log.Debugf("free ebpf WG proxy")
-	return tf.unsetFeatureFlag(featureFlagWGProxy)
-}

client/internal/ebpf/instantiater_linux.go

@@ -1,15 +0,0 @@
-//go:build !android
-
-package ebpf
-
-import (
-	"github.com/netbirdio/netbird/client/internal/ebpf/ebpf"
-	"github.com/netbirdio/netbird/client/internal/ebpf/manager"
-)
-
-// GetEbpfManagerInstance is a wrapper function. This encapsulation is required because if the code import the internal
-// ebpf package the Go compiler will include the object files. But it is not supported on Android. It can cause instant
-// panic on older Android version.
-func GetEbpfManagerInstance() manager.Manager {
-	return ebpf.GetEbpfManagerInstance()
-}

client/internal/ebpf/instantiater_nonlinux.go

@@ -1,10 +0,0 @@
-//go:build !linux || android
-
-package ebpf
-
-import "github.com/netbirdio/netbird/client/internal/ebpf/manager"
-
-// GetEbpfManagerInstance return error because ebpf is not supported on all os
-func GetEbpfManagerInstance() manager.Manager {
-	panic("unsupported os")
-}

client/internal/ebpf/manager/manager.go

@@ -1,7 +0,0 @@
-package manager
-
-// Manager is used to load multiple eBPF programs. E.g., the WireGuard proxy
-type Manager interface {
-	LoadWgProxy(proxyPort, wgPort int) error
-	FreeWGProxy() error
-}

client/internal/elevate/trusted.go

@@ -6,6 +6,17 @@ import (
 	"path/filepath"
 )
 
+// CheckOnlyOwnerWritable reports an error unless path, and every directory
+// leading to it, is owned by an account that can already act with the privileges
+// the caller holds, and is writable by nobody else.
+//
+// Exported for callers outside elevation that read a file while privileged and
+// then act on what it says: the same question this package asks of an
+// executable, asked of a configuration file.
+func CheckOnlyOwnerWritable(path string) error {
+	return checkOnlyOwnerWritable(path)
+}
+
 // trustedSelf returns the path of this executable, provided it is one we are
 // willing to have run as root.
 //

client/internal/engine.go

@@ -671,10 +671,6 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL)
 	}
 	e.wgDevice.Store(e.wgInterface.GetWGDevice())
 
-	// Set up notrack rules immediately after proxy is listening to prevent
-	// conntrack entries from being created before the rules are in place
-	e.setupWGProxyNoTrack()
-
 	// Start after interface is up since port may have been resolved from 0 or changed if occupied
 	e.shutdownWg.Add(1)
 	go func() {
@@ -841,23 +837,6 @@ func (e *Engine) initFirewall() error {
 	return nil
 }
 
-// setupWGProxyNoTrack configures connection tracking exclusion for WireGuard proxy traffic.
-// This prevents conntrack/MASQUERADE from affecting loopback traffic between WireGuard and the eBPF proxy.
-func (e *Engine) setupWGProxyNoTrack() {
-	if e.firewall == nil {
-		return
-	}
-
-	proxyPort := e.wgInterface.GetProxyPort()
-	if proxyPort == 0 {
-		return
-	}
-
-	if err := e.firewall.SetupEBPFProxyNoTrack(proxyPort, uint16(e.config.WgPort)); err != nil {
-		log.Warnf("failed to setup ebpf proxy notrack: %v", err)
-	}
-}
-
 func (e *Engine) blockLanAccess() {
 	if e.config.BlockInbound {
 		// no need to set up extra deny rules if inbound is already blocked in general
@@ -1104,7 +1083,11 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error {
 			// back to empty if the FQDN doesn't have the expected shape.
 			dnsName = extractDNSDomainFromFQDN(pc.GetFqdn())
 		}
-		result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName)
+		// With the firewall disabled there is no ACL manager to program, so
+		// RoutesFirewallRules would be built and then dropped. On a peer that
+		// routes many network resources that is the single most expensive
+		// step of the sync.
+		result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName, e.config.DisableFirewall)
 		if err != nil {
 			return fmt.Errorf("decode network map envelope: %w", err)
 		}

client/internal/engine_test.go

@@ -65,7 +65,6 @@ type MockWGIface struct {
 	GetStatsFunc               func() (map[string]configurer.WGStats, error)
 	GetInterfaceGUIDStringFunc func() (string, error)
 	GetProxyFunc               func() wgproxy.Proxy
-	GetProxyPortFunc           func() uint16
 	GetNetFunc                 func() *netstack.Net
 	LastActivitiesFunc         func() map[string]monotime.Time
 }
@@ -162,12 +161,6 @@ func (m *MockWGIface) GetProxy() wgproxy.Proxy {
 	return m.GetProxyFunc()
 }
 
-func (m *MockWGIface) GetProxyPort() uint16 {
-	if m.GetProxyPortFunc != nil {
-		return m.GetProxyPortFunc()
-	}
-	return 0
-}
 
 func (m *MockWGIface) GetNet() *netstack.Net {
 	return m.GetNetFunc()

client/internal/iface_common.go

@@ -28,7 +28,6 @@ type wgIfaceBase interface {
 	Up() (*udpmux.UniversalUDPMuxDefault, error)
 	UpdateAddr(newAddr wgaddr.Address) error
 	GetProxy() wgproxy.Proxy
-	GetProxyPort() uint16
 	UpdatePeer(peerKey string, allowedIps []netip.Prefix, keepAlive time.Duration, endpoint *net.UDPAddr, preSharedKey *wgtypes.Key) error
 	RemoveEndpointAddress(key string) error
 	RemovePeer(peerKey string) error

client/internal/ipcauth/forward_test.go

@@ -38,7 +38,7 @@ func asDaemon(t *testing.T, id Identity) {
 	prevID, prevKnown, prevDelegate := selfIdentity, selfKnown, selfMayDelegate
 	t.Cleanup(func() { selfIdentity, selfKnown, selfMayDelegate = prevID, prevKnown, prevDelegate })
 	selfIdentity, selfKnown = id, true
-	selfMayDelegate = !id.IsPrivileged()
+	selfMayDelegate = mayDelegate(id)
 }
 
 func TestCallerIdentity_DirectConnections(t *testing.T) {

client/internal/ipcauth/identity.go

@@ -18,7 +18,8 @@ import (
 	"google.golang.org/grpc/peer"
 )
 
-// Well-known Windows SIDs that identify a fully privileged principal.
+// Well-known Windows SIDs. Only LocalSystem and BUILTIN\Administrators identify a
+// privileged principal; the service accounts are shared by unrelated services.
 const (
 	sidLocalSystem    = "S-1-5-18"     // NT AUTHORITY\SYSTEM
 	sidLocalService   = "S-1-5-19"     // NT AUTHORITY\LOCAL SERVICE
@@ -67,9 +68,9 @@ func (i Identity) IsWindows() bool {
 // user-to-root boundary.
 //
 // On Windows the decision comes from the caller's token rather than from
-// account names or group RIDs: an elevated token, one of the service accounts
-// the daemon itself may run as, or a token with BUILTIN\Administrators
-// enabled. A UAC-filtered administrator has that group marked deny-only, and
+// account names or group RIDs: an elevated token, the LocalSystem SID, or a
+// token with BUILTIN\Administrators enabled. LocalService and NetworkService
+// are not privileged by SID. A UAC-filtered administrator has that group marked deny-only, and
 // deny-only groups are dropped when the identity is captured, so such a
 // caller is correctly reported as unprivileged. Domain group memberships
 // (Domain Admins and friends) are deliberately not consulted: they say
@@ -83,8 +84,7 @@ func (i Identity) IsPrivileged() bool {
 		return true
 	}
 
-	switch i.SID {
-	case sidLocalSystem, sidLocalService, sidNetworkService:
+	if i.SID == sidLocalSystem {
 		return true
 	}
 

client/internal/ipcauth/identity_test.go

@@ -64,3 +64,58 @@ func TestIdentitySameUser(t *testing.T) {
 		})
 	}
 }
+
+func TestIdentityIsPrivileged(t *testing.T) {
+	tests := []struct {
+		name string
+		id   Identity
+		want bool
+	}{
+		{
+			name: "Root",
+			id:   Identity{UID: 0, GID: 0},
+			want: true,
+		},
+		{
+			name: "Non-root",
+			id:   Identity{UID: 1000, GID: 1000},
+			want: false,
+		},
+		{
+			name: "Local system windows",
+			id:   Identity{SID: sidLocalSystem},
+			want: true,
+		},
+		{
+			name: "Windows elevated",
+			id:   Identity{SID: "S-1-5-21-1927267129-3959769253-3036563910-1001", Elevated: true},
+			want: true,
+		},
+		{
+			name: "Admin group windows",
+			id:   Identity{SID: "S-1-5-21-1927267129-3959769253-3036563910-1001", Groups: []string{sidAdministrators}},
+			want: true,
+		},
+		{
+			name: "Regular user windows",
+			id:   Identity{SID: "S-1-5-21-1927267129-3959769253-3036563910-1001"},
+			want: false,
+		},
+		{
+			name: "Network service windows",
+			id:   Identity{SID: sidNetworkService},
+			want: false,
+		},
+		{
+			name: "Local service windows",
+			id:   Identity{SID: sidLocalService},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, tt.id.IsPrivileged())
+		})
+	}
+}

client/internal/ipcauth/privileged.go

@@ -45,7 +45,15 @@ func init() {
 	// matching there would let a non-elevated shell of an administrator account
 	// act as an administrator, which is the boundary the token check exists to
 	// keep.
-	selfMayDelegate = !id.IsPrivileged()
+	selfMayDelegate = mayDelegate(id)
+}
+
+// mayDelegate reports whether a daemon running as id may extend its authority to
+// callers sharing its identity. The shared service accounts are excluded: their
+// SID is held by unrelated services, so matching on it would grant them the
+// daemon's authority.
+func mayDelegate(id Identity) bool {
+	return !id.IsPrivileged() && id.SID != sidLocalService && id.SID != sidNetworkService
 }
 
 // IsDaemonSelf reports whether an identity is this very process. The JSON gateway

client/internal/ipcauth/privileged_test.go

@@ -98,7 +98,7 @@ func TestIsPrivilegedCaller_SelfRule(t *testing.T) {
 			t.Cleanup(func() { selfIdentity, selfKnown, selfMayDelegate = prevID, prevKnown, prevDelegate })
 
 			selfIdentity, selfKnown = tt.self, tt.selfKnown
-			selfMayDelegate = tt.selfKnown && !tt.self.IsPrivileged()
+			selfMayDelegate = tt.selfKnown && mayDelegate(tt.self)
 
 			if got := IsPrivilegedCaller(tt.caller); got != tt.want {
 				t.Fatalf("IsPrivilegedCaller(%v) with daemon %v = %t, want %t",
@@ -132,3 +132,28 @@ func TestIsPrivilegedCaller_ThisProcess(t *testing.T) {
 		t.Errorf("an unrelated identity %v was treated as privileged", other)
 	}
 }
+
+// The shared service accounts are held by unrelated services, so a daemon running
+// as one of them must not extend its authority to every process with that SID.
+func TestMayDelegate(t *testing.T) {
+	tests := []struct {
+		name string
+		self Identity
+		want bool
+	}{
+		{name: "unprivileged unix user", self: Identity{UID: 1000}, want: true},
+		{name: "root", self: Identity{UID: 0}, want: false},
+		{name: "unprivileged windows user", self: Identity{SID: "S-1-5-21-1-2-3-1001"}, want: true},
+		{name: "elevated windows user", self: Identity{SID: "S-1-5-21-1-2-3-1001", Elevated: true}, want: false},
+		{name: "local system", self: Identity{SID: sidLocalSystem}, want: false},
+		{name: "local service", self: Identity{SID: sidLocalService}, want: false},
+		{name: "network service", self: Identity{SID: sidNetworkService}, want: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := mayDelegate(tt.self); got != tt.want {
+				t.Errorf("mayDelegate(%+v) = %v, want %v", tt.self, got, tt.want)
+			}
+		})
+	}
+}

client/internal/peer/conn.go

@@ -176,9 +176,10 @@ type Conn struct {
 	// used to store the remote Rosenpass key for Relayed connection in case of connection update from ice
 	rosenpassRemoteKey []byte
 
-	wgProxyICE   wgproxy.Proxy
-	wgProxyRelay wgproxy.Proxy
-	handshaker   *Handshaker
+	wgProxyICE     wgproxy.Proxy
+	wgProxyRelay   wgproxy.Proxy
+	relayedConnRef *relayClient.Conn
+	handshaker     *Handshaker
 
 	guard *guard.Guard
 	wg    sync.WaitGroup
@@ -625,7 +626,7 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
 
-	if conn.ctx.Err() != nil {
+	if conn.ctx.Err() != nil || rci.relayedConn.Context().Err() != nil {
 		if err := rci.relayedConn.Close(); err != nil {
 			conn.Log.Warnf("failed to close unnecessary relayed connection: %v", err)
 		}
@@ -640,7 +641,9 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 		conn.Log.Errorf("failed to add relayed net.Conn to local proxy: %v", err)
 		return
 	}
-	wgProxy.SetDisconnectListener(conn.onRelayDisconnected)
+	wgProxy.SetDisconnectListener(func() {
+		conn.onRelayDisconnected(rci.relayedConn)
+	})
 
 	conn.dumpState.NewLocalProxy()
 
@@ -648,7 +651,7 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 
 	if conn.isICEActive() {
 		conn.Log.Debugf("do not switch to relay because current priority is: %s", conn.currentConnPriority.String())
-		conn.setRelayedProxy(wgProxy)
+		conn.setRelayedProxy(wgProxy, rci.relayedConn)
 		conn.statusRelay.SetConnected()
 		conn.updateRelayStatus(rci.relayedConn.RemoteAddr().String(), rci.rosenpassPubKey, pqOK, time.Now())
 		return
@@ -679,15 +682,26 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 	conn.rosenpassRemoteKey = rci.rosenpassPubKey
 	conn.currentConnPriority = conntype.Relay
 	conn.statusRelay.SetConnected()
-	conn.setRelayedProxy(wgProxy)
+	conn.setRelayedProxy(wgProxy, rci.relayedConn)
 	conn.updateRelayStatus(rci.relayedConn.RemoteAddr().String(), rci.rosenpassPubKey, pqOK, updateTime)
 	conn.Log.Infof("start to communicate with peer via relay")
 	conn.doOnConnected(rci.rosenpassPubKey, rci.rosenpassAddr, updateTime)
 }
 
-func (conn *Conn) onRelayDisconnected() {
+// onRelayDisconnected reports the teardown of a relayed connection. relayedConn
+// names the connection the signal belongs to, so a signal that arrives after
+// its connection was replaced is ignored instead of tearing down its successor.
+// A nil relayedConn means the caller does not track generations and the current
+// connection is always torn down.
+func (conn *Conn) onRelayDisconnected(relayedConn *relayClient.Conn) {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
+
+	if relayedConn != nil && conn.relayedConnRef != relayedConn {
+		conn.Log.Debugf("ignoring relay disconnect of a superseded connection")
+		return
+	}
+
 	conn.handleRelayDisconnectedLocked()
 }
 
@@ -711,6 +725,7 @@ func (conn *Conn) handleRelayDisconnectedLocked() {
 		_ = conn.wgProxyRelay.CloseConn()
 		conn.wgProxyRelay = nil
 	}
+	conn.relayedConnRef = nil
 
 	changed := conn.statusRelay.Get() != worker.StatusDisconnected
 	if changed {
@@ -1020,13 +1035,14 @@ func (conn *Conn) logTraceConnState() {
 	}
 }
 
-func (conn *Conn) setRelayedProxy(proxy wgproxy.Proxy) {
+func (conn *Conn) setRelayedProxy(proxy wgproxy.Proxy, relayedConn *relayClient.Conn) {
 	if conn.wgProxyRelay != nil {
 		if err := conn.wgProxyRelay.CloseConn(); err != nil {
 			conn.Log.Warnf("failed to close deprecated wg proxy conn: %v", err)
 		}
 	}
 	conn.wgProxyRelay = proxy
+	conn.relayedConnRef = relayedConn
 }
 
 // onWGHandshakeSuccess is called when the first WireGuard handshake is detected

client/internal/peer/handshaker.go

@@ -159,7 +159,7 @@ func (h *Handshaker) notifyListeners(remoteOfferAnswer *OfferAnswer) {
 }
 
 func (h *Handshaker) handleRemoteOffer(remoteOfferAnswer OfferAnswer) {
-	h.log.Infof("received offer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials())
+	h.log.Infof("received offer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t, relay server: %s, relay IP: %s", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials(), remoteOfferAnswer.RelaySrvAddress, remoteOfferAnswer.RelaySrvIP)
 	h.onSignalReceived(&remoteOfferAnswer)
 
 	// If we are the controller running the KEM, a responder's offer is handled by
@@ -180,7 +180,7 @@ func (h *Handshaker) handleRemoteOffer(remoteOfferAnswer OfferAnswer) {
 }
 
 func (h *Handshaker) handleRemoteAnswer(remoteOfferAnswer OfferAnswer) {
-	h.log.Infof("received answer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials())
+	h.log.Infof("received answer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t, relay server: %s, relay IP: %s", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials(), remoteOfferAnswer.RelaySrvAddress, remoteOfferAnswer.RelaySrvIP)
 	h.onSignalReceived(&remoteOfferAnswer)
 
 	// Feed the KEM answer (derive+store PSK) BEFORE bringing up the connection so the WG
@@ -309,7 +309,7 @@ func (h *Handshaker) sendOffer() error {
 	if h.config.PQ != nil {
 		offer.MlkemPayload, offer.MlkemPort = h.config.PQ.OfferPayload(h.config.Key)
 	}
-	h.log.Debugf("sending offer with serial: %s", offer.SessionIDString())
+	h.log.Debugf("sending offer with serial: %s, relay server: %s, relay IP: %s", offer.SessionIDString(), offer.RelaySrvAddress, offer.RelaySrvIP)
 
 	return h.signaler.SignalOffer(offer, h.config.Key)
 }
@@ -323,7 +323,7 @@ func (h *Handshaker) sendAnswer(remoteOffer *OfferAnswer) error {
 		}
 		answer.MlkemPayload, answer.MlkemPort = h.config.PQ.AnswerPayload(h.config.Key, recvOffer)
 	}
-	h.log.Debugf("sending answer with serial: %s", answer.SessionIDString())
+	h.log.Debugf("sending answer with serial: %s, relay server: %s, relay IP: %s", answer.SessionIDString(), answer.RelaySrvAddress, answer.RelaySrvIP)
 
 	return h.signaler.SignalAnswer(answer, h.config.Key)
 }

client/internal/peer/status.go

@@ -830,8 +830,8 @@ func (d *Status) SetSessionExpiresAt(deadline time.Time) {
 // "none" would blank the UI at the exact moment it should say the session
 // ended.
 func (d *Status) GetSessionExpiresAt() time.Time {
-	d.mux.Lock()
-	defer d.mux.Unlock()
+	d.mux.RLock()
+	defer d.mux.RUnlock()
 	return d.sessionExpiresAt
 }
 

client/internal/peer/worker_ice.go

@@ -121,11 +121,8 @@ func (w *WorkerICE) OnNewOffer(remoteOfferAnswer *OfferAnswer) {
 			}
 		}
 
-		sessionID, err := NewICESessionID()
-		if err != nil {
-			w.log.Errorf("failed to create new session ID: %s", err)
-		}
-		w.sessionID = sessionID
+		// Keep the ID already advertised to the remote. Answers do not get a
+		// reply, so changing it here makes the next offer restart both sides.
 		w.abandonNegotiation()
 	}
 
@@ -205,6 +202,9 @@ func (w *WorkerICE) Close() {
 	w.muxAgent.Lock()
 	defer w.muxAgent.Unlock()
 
+	if w.agent != nil || w.agentConnecting {
+		w.renewSessionID()
+	}
 	if w.agent != nil {
 		w.agentDialerCancel()
 		if err := w.agent.Close(); err != nil {
@@ -366,16 +366,23 @@ func (w *WorkerICE) closeAgent(agent *icemaker.ThreadSafeAgent, cancel context.C
 	// Only the owner of the current session may reset its state: a stale dial
 	// goroutine waking after a newer attempt must not clobber it.
 	if w.agent == agent {
-		sessionID, err := NewICESessionID()
-		if err != nil {
-			w.log.Errorf("failed to create new session ID: %s", err)
-		}
-		w.sessionID = sessionID
+		w.renewSessionID()
 		w.abandonNegotiation()
 	}
 	return sessionChanged
 }
 
+// renewSessionID starts a new local session, so the remote treats our next offer
+// or answer as a restart. Caller holds muxAgent.
+func (w *WorkerICE) renewSessionID() {
+	sessionID, err := NewICESessionID()
+	if err != nil {
+		w.log.Errorf("failed to create new session ID: %s", err)
+		return
+	}
+	w.sessionID = sessionID
+}
+
 // abandonNegotiation drops all recorded ICE session state so the worker treats the
 // next offer as a fresh start instead of a duplicate of a dead negotiation. The
 // agent and agentConnecting flags must change together: leaving one stale wedges

client/internal/peer/worker_ice_session_test.go

@@ -0,0 +1,375 @@
+package peer
+
+import (
+	"context"
+	"fmt"
+	"net"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	icemaker "github.com/netbirdio/netbird/client/internal/peer/ice"
+)
+
+func TestWorkerICE_RemoteRestartPreservesAdvertisedSession(t *testing.T) {
+	w := newTestWorkerICE(t)
+	t.Cleanup(w.Close)
+	w.dialFunc = parkDial
+	advertised := w.SessionID()
+	remoteSession := ICESessionID("remote-first")
+	offer := OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "remoteufrag", Pwd: "remote-password-long-enough"},
+		SessionID:      &remoteSession,
+	}
+	w.OnNewOffer(&offer)
+	require.True(t, w.InProgress(), "the first remote session must start ICE")
+	w.muxAgent.Lock()
+	firstAgent := w.agent
+	w.muxAgent.Unlock()
+
+	// The same callback handles answers. A changed remote ID must not create
+	// an unannounced local ID that makes the remote restart on our next offer.
+	secondSession := ICESessionID("remote-restarted")
+	answer := offer
+	answer.SessionID = &secondSession
+	w.OnNewOffer(&answer)
+	assert.Equal(t, advertised, w.SessionID(), "following a remote restart must keep our advertised ID")
+	w.muxAgent.Lock()
+	secondAgent := w.agent
+	w.muxAgent.Unlock()
+	assert.NotSame(t, firstAgent, secondAgent, "the changed remote session must still rebuild ICE")
+
+	w.OnNewOffer(&answer)
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Same(t, secondAgent, w.agent, "a repeated answer must keep the replacement agent")
+}
+
+func TestWorkerICE_LocalCloseChangesAdvertisedSession(t *testing.T) {
+	w := newTestWorkerICE(t)
+	dialStarted := make(chan struct{})
+	dialDone := make(chan struct{})
+	w.dialFunc = func(ctx context.Context, _ *icemaker.ThreadSafeAgent, _ *OfferAnswer) (net.Conn, error) {
+		close(dialStarted)
+		defer close(dialDone)
+		<-ctx.Done()
+		return nil, ctx.Err()
+	}
+	session := ICESessionID("remote-session")
+	w.OnNewOffer(&OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "remoteufrag", Pwd: "remote-password-long-enough"},
+		SessionID:      &session,
+	})
+	<-dialStarted
+	advertised := w.SessionID()
+	w.Close()
+	assert.NotEqual(t, advertised, w.SessionID(), "a local teardown must tell the remote to restart")
+	closedSession := w.SessionID()
+
+	// The abandoned dial goroutine cleans up after Close returned.
+	<-dialDone
+	assert.Never(t, func() bool { return w.SessionID() != closedSession }, 200*time.Millisecond, 10*time.Millisecond,
+		"the late cleanup of a closed negotiation must not restart again")
+	w.Close()
+	assert.Equal(t, closedSession, w.SessionID(), "closing an idle worker must not restart again")
+}
+
+// parkDial stands in for the ICE dial. It never connects and returns once the
+// negotiation is abandoned, so a test decides when a negotiation fails.
+func parkDial(ctx context.Context, _ *icemaker.ThreadSafeAgent, _ *OfferAnswer) (net.Conn, error) {
+	<-ctx.Done()
+	return nil, ctx.Err()
+}
+
+func newTestSessionID(t *testing.T) ICESessionID {
+	t.Helper()
+	sid, err := NewICESessionID()
+	require.NoError(t, err)
+	return sid
+}
+
+// handshakeSide is one end of a simulated signaling exchange.
+type handshakeSide interface {
+	// message builds the offer or answer the side would send now.
+	message() OfferAnswer
+	// receive hands a remote offer or answer to the side's ICE logic.
+	receive(msg OfferAnswer)
+	// teardowns counts negotiations the side tore down to follow a remote restart.
+	teardowns() int
+	// failAgent ends the side's current negotiation as an ICE failure does.
+	failAgent()
+}
+
+// workerSide drives a real WorkerICE.
+type workerSide struct {
+	t        *testing.T
+	w        *WorkerICE
+	replaced int
+}
+
+func newWorkerSide(t *testing.T) *workerSide {
+	t.Helper()
+	w := newTestWorkerICE(t)
+	w.dialFunc = parkDial
+	t.Cleanup(w.Close)
+	return &workerSide{t: t, w: w}
+}
+
+func (s *workerSide) message() OfferAnswer {
+	sid := s.w.SessionID()
+	ufrag, pwd := s.w.GetLocalUserCredentials()
+	return OfferAnswer{IceCredentials: IceCredentials{UFrag: ufrag, Pwd: pwd}, SessionID: &sid}
+}
+
+func (s *workerSide) receive(msg OfferAnswer) {
+	before := s.agent()
+	s.w.OnNewOffer(&msg)
+	if after := s.agent(); before != nil && after != before {
+		s.replaced++
+	}
+}
+
+func (s *workerSide) teardowns() int { return s.replaced }
+
+func (s *workerSide) agent() *icemaker.ThreadSafeAgent {
+	s.w.muxAgent.Lock()
+	defer s.w.muxAgent.Unlock()
+	return s.w.agent
+}
+
+// failAgent runs the cleanup the dial goroutine or the Failed state callback
+// performs when the current negotiation dies.
+func (s *workerSide) failAgent() {
+	s.t.Helper()
+	s.w.muxAgent.Lock()
+	agent, cancel := s.w.agent, s.w.agentDialerCancel
+	s.w.muxAgent.Unlock()
+	require.NotNil(s.t, agent, "failing requires a running negotiation")
+	s.w.closeAgent(agent, cancel)
+}
+
+// legacySide models a remote peer running a release from before this change:
+// when it follows a remote restart it also picks a new session ID of its own,
+// which it announces only with its next offer or answer.
+type legacySide struct {
+	t         *testing.T
+	sessionID ICESessionID
+	remoteID  ICESessionID
+	hasAgent  bool
+	replaced  int
+}
+
+func newLegacySide(t *testing.T) *legacySide {
+	return &legacySide{t: t, sessionID: newTestSessionID(t)}
+}
+
+func (s *legacySide) message() OfferAnswer {
+	sid := s.sessionID
+	return OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "legacyufrag", Pwd: "legacy-password-long-enough"},
+		SessionID:      &sid,
+	}
+}
+
+func (s *legacySide) receive(msg OfferAnswer) {
+	if msg.SessionID == nil {
+		s.hasAgent = true
+		return
+	}
+	if s.hasAgent {
+		if *msg.SessionID == s.remoteID {
+			return
+		}
+		s.replaced++
+		s.sessionID = newTestSessionID(s.t)
+	}
+	s.hasAgent = true
+	s.remoteID = *msg.SessionID
+}
+
+func (s *legacySide) teardowns() int { return s.replaced }
+
+func (s *legacySide) failAgent() {
+	s.hasAgent = false
+	s.remoteID = ""
+	s.sessionID = newTestSessionID(s.t)
+}
+
+// exchange runs one guard-driven round in the order Handshaker.Listen uses: the
+// answerer handles the offer and answers with the session ID it holds
+// afterwards, and the offerer handles the answer without replying.
+func exchange(offerer, answerer handshakeSide) {
+	answerer.receive(offerer.message())
+	offerer.receive(answerer.message())
+}
+
+// offerPattern decides which side's guard sends the offer in a round.
+type offerPattern struct {
+	name   string
+	picker func(round int, local, remote handshakeSide) (offerer, answerer handshakeSide)
+}
+
+var offerPatterns = []offerPattern{
+	{
+		// A routing peer whose relay is down keeps offering on its own.
+		name: "local peer offers",
+		picker: func(_ int, local, remote handshakeSide) (handshakeSide, handshakeSide) {
+			return local, remote
+		},
+	},
+	{
+		name: "both peers offer",
+		picker: func(round int, local, remote handshakeSide) (handshakeSide, handshakeSide) {
+			if round%2 == 0 {
+				return local, remote
+			}
+			return remote, local
+		},
+	},
+}
+
+// assertSettles runs guard rounds and requires the pair to stop restarting
+// each other: at most maxTeardowns in total, and none once half the rounds ran.
+func assertSettles(t *testing.T, pattern offerPattern, local, remote handshakeSide, maxTeardowns int) {
+	t.Helper()
+	const rounds = 10
+
+	total := func() int { return local.teardowns() + remote.teardowns() }
+	start := total()
+	var halfway int
+	for round := range rounds {
+		if round == rounds/2 {
+			halfway = total()
+		}
+		offerer, answerer := pattern.picker(round, local, remote)
+		exchange(offerer, answerer)
+	}
+
+	assert.LessOrEqual(t, total()-start, maxTeardowns, "the peers must not keep restarting each other")
+	assert.Equal(t, halfway, total(), "the negotiation must be stable in the later rounds")
+}
+
+// establish runs the first offer and answer, so both sides negotiate.
+func establish(t *testing.T, local, remote handshakeSide) {
+	t.Helper()
+	exchange(local, remote)
+	require.Zero(t, local.teardowns()+remote.teardowns(), "the first exchange must not restart anything")
+}
+
+func TestICESession_SettlesAfterAgentFailure(t *testing.T) {
+	sides := []struct {
+		name   string
+		remote func(t *testing.T) handshakeSide
+	}{
+		{name: "current remote", remote: func(t *testing.T) handshakeSide { return newWorkerSide(t) }},
+		{name: "legacy remote", remote: func(t *testing.T) handshakeSide { return newLegacySide(t) }},
+	}
+	failures := []struct {
+		name string
+		fail func(local, remote handshakeSide)
+	}{
+		{name: "remote agent fails", fail: func(_, remote handshakeSide) { remote.failAgent() }},
+		{name: "local agent fails", fail: func(local, _ handshakeSide) { local.failAgent() }},
+		{name: "both agents fail", fail: func(local, remote handshakeSide) {
+			local.failAgent()
+			remote.failAgent()
+		}},
+	}
+
+	for _, side := range sides {
+		for _, failure := range failures {
+			for _, pattern := range offerPatterns {
+				t.Run(fmt.Sprintf("%s/%s/%s", side.name, failure.name, pattern.name), func(t *testing.T) {
+					local := newWorkerSide(t)
+					remote := side.remote(t)
+					establish(t, local, remote)
+
+					failure.fail(local, remote)
+					assertSettles(t, pattern, local, remote, 2)
+				})
+			}
+		}
+	}
+}
+
+// TestICESession_LocalCloseRestartsRemote covers an explicit teardown, as on a
+// WireGuard handshake timeout. The remote must start over as well, or it keeps
+// answering from the negotiation this side just abandoned.
+func TestICESession_LocalCloseRestartsRemote(t *testing.T) {
+	for _, pattern := range offerPatterns {
+		t.Run(pattern.name, func(t *testing.T) {
+			local := newWorkerSide(t)
+			remote := newWorkerSide(t)
+			establish(t, local, remote)
+
+			local.w.Close()
+			assertSettles(t, pattern, local, remote, 1)
+			assert.Equal(t, 1, remote.teardowns(), "the remote must restart its negotiation exactly once")
+		})
+	}
+}
+
+func TestICESession_DuplicateMessagesKeepNegotiation(t *testing.T) {
+	local := newWorkerSide(t)
+	remote := newWorkerSide(t)
+
+	offer := local.message()
+	remote.receive(offer)
+	answer := remote.message()
+	local.receive(answer)
+
+	// Signaling may deliver the same message again, and a peer answers every
+	// offer, including repeats of one it already handled.
+	remote.receive(offer)
+	local.receive(answer)
+	local.receive(remote.message())
+
+	assert.Zero(t, local.teardowns(), "a repeated answer must not restart the negotiation")
+	assert.Zero(t, remote.teardowns(), "a repeated offer must not restart the negotiation")
+}
+
+// TestICESession_RemoteWithoutSessionIDKeepsNegotiation covers remote peers
+// too old to send session IDs: once negotiating, their messages cannot tell a
+// restart from a repeat, so they must not tear anything down.
+func TestICESession_RemoteWithoutSessionIDKeepsNegotiation(t *testing.T) {
+	local := newWorkerSide(t)
+	unversioned := OfferAnswer{IceCredentials: IceCredentials{UFrag: "oldufrag", Pwd: "old-password-long-enough"}}
+
+	local.receive(unversioned)
+	require.NotNil(t, local.agent(), "a message without a session ID must still start ICE")
+	advertised := local.w.SessionID()
+
+	for range 3 {
+		local.receive(unversioned)
+	}
+	assert.Zero(t, local.teardowns(), "messages without a session ID must not restart the negotiation")
+	assert.Equal(t, advertised, local.w.SessionID(), "the advertised session must not change")
+}
+
+// TestWorkerICE_StaleCleanupKeepsAdvertisedSession covers the cleanup of a
+// replaced negotiation finishing late, from its dial goroutine or its Closed
+// state callback. It must neither pick a new session ID, an unannounced local
+// restart, nor disturb the negotiation that replaced it.
+func TestWorkerICE_StaleCleanupKeepsAdvertisedSession(t *testing.T) {
+	local := newWorkerSide(t)
+	remote := newWorkerSide(t)
+	establish(t, local, remote)
+
+	local.w.muxAgent.Lock()
+	oldAgent, oldCancel := local.w.agent, local.w.agentDialerCancel
+	local.w.muxAgent.Unlock()
+
+	remote.failAgent()
+	exchange(local, remote)
+	require.Equal(t, 1, local.teardowns(), "the local side must follow the remote restart")
+	advertised := local.w.SessionID()
+	current := local.agent()
+
+	local.w.closeAgent(oldAgent, oldCancel)
+
+	assert.Equal(t, advertised, local.w.SessionID(), "a stale cleanup must not change the advertised session")
+	assert.Same(t, current, local.agent(), "a stale cleanup must keep the current negotiation")
+	assertSettles(t, offerPatterns[1], local, remote, 0)
+}

client/internal/peer/worker_relay.go

@@ -3,7 +3,6 @@ package peer
 import (
 	"context"
 	"errors"
-	"net"
 	"net/netip"
 	"sync"
 	"sync/atomic"
@@ -14,7 +13,7 @@ import (
 )
 
 type RelayConnInfo struct {
-	relayedConn     net.Conn
+	relayedConn     *relayClient.Conn
 	rosenpassPubKey []byte
 	rosenpassAddr   string
 }
@@ -27,7 +26,7 @@ type WorkerRelay struct {
 	conn         *Conn
 	relayManager *relayClient.Manager
 
-	relayedConn net.Conn
+	relayedConn *relayClient.Conn
 	relayLock   sync.Mutex
 
 	relaySupportedOnRemotePeer atomic.Bool
@@ -80,12 +79,7 @@ func (w *WorkerRelay) OnNewOffer(remoteOfferAnswer *OfferAnswer) {
 	w.relayedConn = relayedConn
 	w.relayLock.Unlock()
 
-	err = w.relayManager.AddCloseListener(srv, w.onRelayClientDisconnected)
-	if err != nil {
-		log.Errorf("failed to add close listener: %s", err)
-		_ = relayedConn.Close()
-		return
-	}
+	go w.watchRelayedConn(relayedConn)
 
 	w.log.Debugf("peer conn opened via Relay: %s", srv)
 	go w.conn.onRelayConnectionIsReady(RelayConnInfo{
@@ -109,12 +103,15 @@ func (w *WorkerRelay) RelayIsSupportedLocally() bool {
 
 func (w *WorkerRelay) CloseConn() {
 	w.relayLock.Lock()
-	defer w.relayLock.Unlock()
-	if w.relayedConn == nil {
+	conn := w.relayedConn
+	w.relayedConn = nil
+	w.relayLock.Unlock()
+
+	if conn == nil {
 		return
 	}
 
-	if err := w.relayedConn.Close(); err != nil {
+	if err := conn.Close(); err != nil {
 		w.log.Warnf("failed to close relay connection: %v", err)
 	}
 }
@@ -133,6 +130,8 @@ func (w *WorkerRelay) preferredRelayServer(myRelayAddress, remoteRelayAddress st
 	return remoteRelayAddress
 }
 
-func (w *WorkerRelay) onRelayClientDisconnected() {
-	go w.conn.onRelayDisconnected()
+func (w *WorkerRelay) watchRelayedConn(relayedConn *relayClient.Conn) {
+	<-relayedConn.Context().Done()
+
+	w.conn.onRelayDisconnected(relayedConn)
 }

client/internal/profilemanager/active_state_test.go

@@ -0,0 +1,76 @@
+package profilemanager
+
+import (
+	"fmt"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Regression test: a concurrent Get and Set of the ActiveProfileState will
+// fail on Windows since the write is a temp file renamed over an open file.
+// Windows will refuse to replace a file another handle holds open by default.
+func TestActiveProfileState_ReadsDoNotBreakAConcurrentWrite(t *testing.T) {
+	withTempConfigDir(t, func(configDir string) {
+		withPatchedGlobals(t, configDir, func() {
+			sm := &ServiceManager{}
+			require.NoError(t, sm.CreateDefaultProfile())
+			require.NoError(t, sm.SetActiveProfileStateToDefault())
+
+			const switched = ID("0123456789abcdef0123456789abcdef")
+			const rounds = 50
+
+			var wg sync.WaitGroup
+			errs := make(chan error, 128)
+
+			for i := 0; i < 8; i++ {
+				wg.Add(1)
+				go func() {
+					defer wg.Done()
+					for r := 0; r < rounds; r++ {
+						state, err := sm.GetActiveProfileState()
+						if err != nil {
+							errs <- fmt.Errorf("read: %w", err)
+							return
+						}
+						if state.ID != defaultProfileName && state.ID != switched {
+							errs <- fmt.Errorf("read: active profile is %q, which no writer wrote", state.ID)
+							return
+						}
+					}
+				}()
+			}
+
+			for i := 0; i < 2; i++ {
+				wg.Add(1)
+				go func() {
+					defer wg.Done()
+					for r := 0; r < rounds; r++ {
+						id := switched
+						if r%2 == 0 {
+							id = defaultProfileName
+						}
+						if err := sm.SetActiveProfileState(&ActiveProfileState{ID: id, Username: "testuser"}); err != nil {
+							errs <- fmt.Errorf("switch: %w", err)
+							return
+						}
+					}
+				}()
+			}
+
+			wg.Wait()
+			close(errs)
+
+			for err := range errs {
+				assert.NoError(t, err, "a switch and a read of the active profile state must not collide")
+			}
+
+			state, err := sm.GetActiveProfileState()
+			require.NoError(t, err)
+			assert.Contains(t, []ID{defaultProfileName, switched}, state.ID,
+				"the file holds whichever switch landed last, not a mix of the two")
+		})
+	})
+}

client/internal/wincmd/system32_windows.go

@@ -0,0 +1,30 @@
+// Package wincmd locates the Windows utilities the client shells out to.
+package wincmd
+
+import (
+	"path/filepath"
+
+	log "github.com/sirupsen/logrus"
+	"golang.org/x/sys/windows"
+)
+
+// defaultSystem32Dir is where the system directory is on every supported
+// install, used only when the API that reports it fails.
+const defaultSystem32Dir = `C:\Windows\System32`
+
+// System32 returns the full path of a Windows utility under the system
+// directory.
+//
+// PATH is deliberately not consulted. The daemon runs as LocalSystem with an
+// environment of its own, so whoever can place an entry in that PATH chooses
+// which binary runs with those privileges. The system directory is read from
+// the API rather than from %SystemRoot% for the same reason.
+func System32(command string) string {
+	sysDir, err := windows.GetSystemDirectory()
+	if err != nil {
+		log.Warnf("Failed to locate the Windows system directory, falling back to %s: %v", defaultSystem32Dir, err)
+		sysDir = defaultSystem32Dir
+	}
+
+	return filepath.Join(sysDir, command+".exe")
+}

client/internal/wincmd/system32_windows_test.go

@@ -0,0 +1,31 @@
+package wincmd
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSystem32IgnoresPATH(t *testing.T) {
+	// A directory holding something that would win a PATH lookup, in front of
+	// everything else: the daemon runs as LocalSystem, so a PATH entry must not
+	// be able to decide what it executes.
+	planted := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(planted, "netsh.exe"), []byte("not really netsh"), 0o600))
+	t.Setenv("PATH", planted+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	got := System32("netsh")
+
+	assert.True(t, filepath.IsAbs(got), "the path must be absolute, got %q", got)
+	assert.NotContains(t, got, planted, "a PATH entry must not be consulted")
+	assert.True(t, strings.EqualFold(filepath.Base(got), "netsh.exe"), "unexpected file name in %q", got)
+
+	// The system directory is what Windows reports it to be, not %SystemRoot%,
+	// which the same caller could have set alongside PATH.
+	t.Setenv("SystemRoot", planted)
+	assert.Equal(t, got, System32("netsh"), "%SystemRoot% must not move the lookup")
+}