fix: address code review findings from embedded wireguard-go PR

- Add explicit wgDevice.Up() call on darwin and windows (bug: device
  stayed in down state, preventing traffic flow)
- Move UAPI accept goroutine after all init succeeds to prevent races
- Fix Windows TOCTOU race: inline LUID lookup under lock in SetupRoutes
- Use errors.Is(windows.ERROR_OBJECT_ALREADY_EXISTS) instead of string matching
- Add context.Context to wgconfig.ApplyConfig for future cancellation
- Remove dead BuildServerConfig and its tests
- Migrate darwin addRouteViaInterface to netip.ParsePrefix, auto-detect IPv6
- Deduplicate wireguardMTU constant into shared util.go
- Inline replacePeers variable, fix %%v to %%w error wrapping
- Add explanatory comments for ifconfig shelling, time.Sleep, IPv6 filter
- Replace empty NSIS _StartService_Init with PP_NoOp

Author: sechmann

assets/windows/naisdevice.nsi

@@ -349,8 +349,6 @@ Function _StartService_Abort
 FunctionEnd
 
 ; Function to initialize any needed state, PP_NoOp to skip
-Function _StartService_Init
-FunctionEnd
 
 ; Function called on every step. Should push 0 to the stack to leave the page, any other value to continue (required)
 Function _StartService_Step
@@ -382,6 +380,6 @@ ${ProgressPage} \
         The final step is to start the background service.$\n$\n\
         Have a nais day!" \
     _StartService_Abort \
-    _StartService_Init \
+    PP_NoOp \
     _StartService_Step \
     PP_NoOp

internal/helper/helper.go

@@ -52,7 +52,7 @@ func (dhs *DeviceHelperServer) Teardown(
 	dhs.log.WithField("interface", dhs.config.Interface).Info("removing network interface and all routes")
 	err := dhs.osConfigurator.TeardownInterface(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("tearing down interface: %v", err)
+		return nil, fmt.Errorf("tearing down interface: %w", err)
 	}
 
 	return &pb.TeardownResponse{}, nil

internal/helper/helper_darwin.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"net/netip"
 	"os"
 	"os/exec"
 	"strings"
@@ -22,8 +23,6 @@ import (
 	"github.com/nais/device/pkg/pb"
 )
 
-const wireguardMTU = 1360
-
 type DarwinConfigurator struct {
 	helperConfig Config
 
@@ -46,7 +45,7 @@ func (c *DarwinConfigurator) Prerequisites() error {
 }
 
 func (c *DarwinConfigurator) SyncConf(ctx context.Context, cfg *pb.Configuration) error {
-	return wgconfig.ApplyConfig(c.helperConfig.Interface, cfg)
+	return wgconfig.ApplyConfig(ctx, c.helperConfig.Interface, cfg)
 }
 
 func (c *DarwinConfigurator) SetupRoutes(ctx context.Context, gateways []*pb.Gateway) (int, error) {
@@ -61,17 +60,20 @@ func (c *DarwinConfigurator) SetupRoutes(ctx context.Context, gateways []*pb.Gat
 			if strings.HasPrefix(cidr, TunnelNetworkPrefix) {
 				continue
 			}
-			if err := addRouteViaInterface(cidr, iface, false); err != nil {
+			if err := addRouteViaInterface(cidr, iface); err != nil {
 				return routesAdded, fmt.Errorf("add IPv4 route %s: %w", cidr, err)
 			}
 			routesAdded++
 		}
 
 		for _, cidr := range gw.GetRoutesIPv6() {
+			// TunnelNetworkPrefix is an IPv4 prefix ("10.255.24.") so this check
+			// is a no-op for IPv6 CIDRs. It's kept for consistency with the IPv4
+			// loop and as a safeguard in case the prefix changes in the future.
 			if strings.HasPrefix(cidr, TunnelNetworkPrefix) {
 				continue
 			}
-			if err := addRouteViaInterface(cidr, iface, true); err != nil {
+			if err := addRouteViaInterface(cidr, iface); err != nil {
 				return routesAdded, fmt.Errorf("add IPv6 route %s: %w", cidr, err)
 			}
 			routesAdded++
@@ -106,6 +108,11 @@ func (c *DarwinConfigurator) SetupInterface(ctx context.Context, cfg *pb.Configu
 	}
 	wgDev := device.NewDevice(tunDev, conn.NewDefaultBind(), logger)
 
+	if err := wgDev.Up(); err != nil {
+		wgDev.Close()
+		return fmt.Errorf("bring up wireguard device: %w", err)
+	}
+
 	// UAPI socket allows wgctrl to configure the device
 	fileUAPI, err := ipc.UAPIOpen(ifaceName)
 	if err != nil {
@@ -120,21 +127,15 @@ func (c *DarwinConfigurator) SetupInterface(ctx context.Context, cfg *pb.Configu
 		return fmt.Errorf("listen on UAPI socket: %w", err)
 	}
 
-	go func() {
-		for {
-			uapiConn, err := uapi.Accept()
-			if err != nil {
-				return
-			}
-			go wgDev.IpcHandle(uapiConn)
-		}
-	}()
-
 	c.wgDevice = wgDev
 	c.tunDev = tunDev
 	c.uapi = uapi
 
-	// Configure IP addresses and bring the interface up
+	// Configure IP addresses and bring the interface up.
+	// We shell out to ifconfig because macOS has no stable public API for assigning
+	// addresses to utun interfaces — the required SIOCAIFADDR_IN6 / SIOCSIFADDR
+	// ioctls are undocumented and vary across OS versions. ifconfig is the standard
+	// tool used by the macOS networking stack itself.
 	commands := [][]string{
 		{"ifconfig", ifaceName, "inet", cfg.GetDeviceIPv4() + "/21", cfg.GetDeviceIPv4(), "alias"},
 		{"ifconfig", ifaceName, "inet6", cfg.GetDeviceIPv6() + "/64", "alias"},
@@ -149,7 +150,9 @@ func (c *DarwinConfigurator) SetupInterface(ctx context.Context, cfg *pb.Configu
 			return fmt.Errorf("running %v: %w: %v", cmd, err, string(out))
 		}
 
-		time.Sleep(100 * time.Millisecond) // avoid serializable race conditions with kernel
+		// Small delay between ifconfig calls to avoid kernel ENOMEM / EBUSY
+		// errors when rapidly configuring addresses on a freshly-created utun.
+		time.Sleep(100 * time.Millisecond)
 	}
 
 	// Add /21 route for the tunnel network
@@ -159,11 +162,23 @@ func (c *DarwinConfigurator) SetupInterface(ctx context.Context, cfg *pb.Configu
 		return fmt.Errorf("lookup interface %q: %w", ifaceName, err)
 	}
 
-	if err := addRouteViaInterface(cfg.GetDeviceIPv4()+"/21", iface, false); err != nil {
+	if err := addRouteViaInterface(cfg.GetDeviceIPv4()+"/21", iface); err != nil {
 		c.closeLocked()
 		return fmt.Errorf("add tunnel network route: %w", err)
 	}
 
+	// Start accepting UAPI connections only after all initialization has succeeded.
+	// This ensures wgctrl can't race against incomplete interface setup.
+	go func() {
+		for {
+			uapiConn, err := uapi.Accept()
+			if err != nil {
+				return
+			}
+			go wgDev.IpcHandle(uapiConn)
+		}
+	}()
+
 	return nil
 }
 
@@ -196,8 +211,8 @@ func (c *DarwinConfigurator) closeLocked() {
 
 // addRouteViaInterface adds a route for the given CIDR through the specified
 // network interface using BSD routing sockets.
-func addRouteViaInterface(cidr string, iface *net.Interface, ipv6 bool) error {
-	_, ipNet, err := net.ParseCIDR(cidr)
+func addRouteViaInterface(cidr string, iface *net.Interface) error {
+	prefix, err := netip.ParsePrefix(cidr)
 	if err != nil {
 		return fmt.Errorf("parse CIDR %q: %w", cidr, err)
 	}
@@ -210,21 +225,25 @@ func addRouteViaInterface(cidr string, iface *net.Interface, ipv6 bool) error {
 
 	addrs := make([]route.Addr, syscall.RTAX_MAX)
 
-	if ipv6 {
-		var dst [16]byte
-		copy(dst[:], ipNet.IP.To16())
+	if prefix.Addr().Is6() {
+		dst := prefix.Addr().As16()
 		addrs[syscall.RTAX_DST] = &route.Inet6Addr{IP: dst}
 
 		var mask [16]byte
-		copy(mask[:], ipNet.Mask)
+		ones := prefix.Bits()
+		for i := range ones {
+			mask[i/8] |= 1 << (7 - i%8)
+		}
 		addrs[syscall.RTAX_NETMASK] = &route.Inet6Addr{IP: mask}
 	} else {
-		var dst [4]byte
-		copy(dst[:], ipNet.IP.To4())
+		dst := prefix.Addr().As4()
 		addrs[syscall.RTAX_DST] = &route.Inet4Addr{IP: dst}
 
 		var mask [4]byte
-		copy(mask[:], ipNet.Mask)
+		ones := prefix.Bits()
+		for i := range ones {
+			mask[i/8] |= 1 << (7 - i%8)
+		}
 		addrs[syscall.RTAX_NETMASK] = &route.Inet4Addr{IP: mask}
 	}
 

internal/helper/helper_linux.go

@@ -13,8 +13,6 @@ import (
 	"github.com/nais/device/pkg/pb"
 )
 
-const wireguardMTU = 1360
-
 func New(helperConfig Config) *LinuxConfigurator {
 	return &LinuxConfigurator{
 		helperConfig: helperConfig,
@@ -32,7 +30,7 @@ func (c *LinuxConfigurator) Prerequisites() error {
 }
 
 func (c *LinuxConfigurator) SyncConf(ctx context.Context, cfg *pb.Configuration) error {
-	return wgconfig.ApplyConfig(c.helperConfig.Interface, cfg)
+	return wgconfig.ApplyConfig(ctx, c.helperConfig.Interface, cfg)
 }
 
 func (c *LinuxConfigurator) SetupRoutes(ctx context.Context, gateways []*pb.Gateway) (int, error) {

internal/helper/helper_windows.go

@@ -2,13 +2,15 @@ package helper
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
 	"net/netip"
 	"os"
 	"strings"
 	"sync"
 
+	"golang.org/x/sys/windows"
 	"golang.zx2c4.com/wireguard/conn"
 	"golang.zx2c4.com/wireguard/device"
 	"golang.zx2c4.com/wireguard/ipc"
@@ -19,8 +21,6 @@ import (
 	"github.com/nais/device/pkg/pb"
 )
 
-const wireguardMTU = 1360
-
 type WindowsConfigurator struct {
 	helperConfig Config
 
@@ -43,15 +43,23 @@ func (c *WindowsConfigurator) Prerequisites() error {
 }
 
 func (c *WindowsConfigurator) SyncConf(ctx context.Context, cfg *pb.Configuration) error {
-	return wgconfig.ApplyConfig(c.helperConfig.Interface, cfg)
+	return wgconfig.ApplyConfig(ctx, c.helperConfig.Interface, cfg)
 }
 
 func (c *WindowsConfigurator) SetupRoutes(ctx context.Context, gateways []*pb.Gateway) (int, error) {
-	ifLUID, err := c.interfaceLUID()
-	if err != nil {
-		return 0, err
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	if c.tunDev == nil {
+		return 0, fmt.Errorf("TUN device not initialized")
 	}
 
+	nativeTun, ok := c.tunDev.(*tun.NativeTun)
+	if !ok {
+		return 0, fmt.Errorf("unexpected TUN device type %T", c.tunDev)
+	}
+	ifLUID := winipcfg.LUID(nativeTun.LUID())
+
 	routesAdded := 0
 	for _, gw := range gateways {
 		for _, cidr := range append(gw.GetRoutesIPv4(), gw.GetRoutesIPv6()...) {
@@ -72,7 +80,7 @@ func (c *WindowsConfigurator) SetupRoutes(ctx context.Context, gateways []*pb.Ga
 			}
 
 			if err := ifLUID.AddRoute(dst, nextHop, 0); err != nil {
-				if strings.Contains(err.Error(), "already exists") {
+				if errors.Is(err, windows.ERROR_OBJECT_ALREADY_EXISTS) {
 					continue
 				}
 				return routesAdded, fmt.Errorf("add route %s: %w", cidr, err)
@@ -103,22 +111,17 @@ func (c *WindowsConfigurator) SetupInterface(ctx context.Context, cfg *pb.Config
 	}
 	wgDev := device.NewDevice(tunDev, conn.NewDefaultBind(), logger)
 
+	if err := wgDev.Up(); err != nil {
+		wgDev.Close()
+		return fmt.Errorf("bring up wireguard device: %w", err)
+	}
+
 	uapi, err := ipc.UAPIListen(c.helperConfig.Interface)
 	if err != nil {
 		wgDev.Close()
 		return fmt.Errorf("listen on UAPI named pipe: %w", err)
 	}
 
-	go func() {
-		for {
-			uapiConn, err := uapi.Accept()
-			if err != nil {
-				return
-			}
-			go wgDev.IpcHandle(uapiConn)
-		}
-	}()
-
 	c.wgDevice = wgDev
 	c.tunDev = tunDev
 	c.uapi = uapi
@@ -154,6 +157,18 @@ func (c *WindowsConfigurator) SetupInterface(ctx context.Context, cfg *pb.Config
 		}
 	}
 
+	// Start accepting UAPI connections only after all initialization has succeeded.
+	// This ensures wgctrl can't race against incomplete interface setup.
+	go func() {
+		for {
+			uapiConn, err := uapi.Accept()
+			if err != nil {
+				return
+			}
+			go wgDev.IpcHandle(uapiConn)
+		}
+	}()
+
 	return nil
 }
 
@@ -182,18 +197,3 @@ func (c *WindowsConfigurator) closeLocked() {
 	}
 	c.tunDev = nil
 }
-
-func (c *WindowsConfigurator) interfaceLUID() (winipcfg.LUID, error) {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	if c.tunDev == nil {
-		return 0, fmt.Errorf("TUN device not initialized")
-	}
-
-	nativeTun, ok := c.tunDev.(*tun.NativeTun)
-	if !ok {
-		return 0, fmt.Errorf("unexpected TUN device type %T", c.tunDev)
-	}
-	return winipcfg.LUID(nativeTun.LUID()), nil
-}

internal/helper/util.go

@@ -14,6 +14,9 @@ import (
 
 const (
 	TunnelNetworkPrefix = "10.255.24."
+
+	// wireguardMTU is the MTU used for WireGuard tunnel interfaces across all platforms.
+	wireguardMTU = 1360
 )
 
 func ZipLogFiles(files []string, log logrus.FieldLogger) (string, error) {
@@ -33,7 +36,7 @@ func ZipLogFiles(files []string, log logrus.FieldLogger) (string, error) {
 		}
 		logFile, err := os.Open(filename)
 		if err != nil {
-			return "nil", fmt.Errorf("%s %v", filename, err)
+			return "nil", fmt.Errorf("%s: %w", filename, err)
 		}
 		zipEntryWiter, err := zipWriter.Create(filepath.Base(filename))
 		if err != nil {