Fail the reload on a startup only value that would fail a restart, and stop asking for tun.dev restarts that change nothing (#1953)

examples/config.yml

@@ -261,7 +261,7 @@ tun:
   disabled: false
   # Name of the device. If not set, a default will be chosen by the OS.
   # For macOS: if set, must be in the form `utun[0-9]+`.
-  # For NetBSD: Required to be set, must be in the form `tun[0-9]+`
+  # For OpenBSD and NetBSD: if set, must be in the form `tun[0-9]+`, otherwise the first free one is used.
   # Not reloadable.
   dev: nebula1
   # Toggles forwarding of local broadcast packets, the address of which depends on the ip/mask encoded in pki.cert

overlay/tun_bsd_dev.go

@@ -0,0 +1,45 @@
+//go:build (openbsd || netbsd) && !e2e_testing
+
+package overlay
+
+import (
+	"errors"
+	"fmt"
+	"log/slog"
+	"os"
+	"regexp"
+	"runtime"
+
+	"golang.org/x/sys/unix"
+)
+
+var deviceNameRE = regexp.MustCompile(`^tun[0-9]+$`)
+
+// tunDevNeedsRestart is true when tun.dev asks for a particular tun that isn't the running one. Anything else takes the
+// first free one, and the one we have will do
+func tunDevNeedsRestart(name, running string) bool {
+	return deviceNameRE.MatchString(name) && name != running
+}
+
+// openTun opens /dev/name, or the first /dev/tunN nothing else has open when name is empty or not tunN
+func openTun(l *slog.Logger, name string) (int, string, error) {
+	if name != "" && !deviceNameRE.MatchString(name) {
+		// The name doesn't mean anything here, so like darwin a bad one isn't worth refusing to start over
+		l.Warn("interface name must be tun[0-9]+ on "+runtime.GOOS+", ignoring", "name", name)
+		name = ""
+	}
+	if name != "" {
+		fd, err := unix.Open("/dev/"+name, os.O_RDWR, 0)
+		return fd, name, err
+	}
+	for i := 0; ; i++ {
+		name = fmt.Sprintf("tun%d", i)
+		fd, err := unix.Open("/dev/"+name, os.O_RDWR, 0)
+		if err == nil {
+			return fd, name, nil
+		}
+		if !errors.Is(err, unix.EBUSY) {
+			return -1, "", fmt.Errorf("no free tun device, %s: %w", name, err)
+		}
+	}
+}

overlay/tun_bsd_reload_test.go

@@ -0,0 +1,80 @@
+//go:build (openbsd || netbsd) && !e2e_testing
+
+package overlay
+
+import (
+	"net/netip"
+	"testing"
+
+	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/test"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// A tun.dev that isn't tunN is ignored like darwin does, and the first free tun is used. Only naming a different tun
+// asks for a restart
+func TestTunReloadDevRestart(t *testing.T) {
+	newWithDev := func(dev string) (*config.C, *tun, error) {
+		c := config.NewC(test.NewLogger())
+		require.NoError(t, c.LoadString("tun:\n  dev: "+dev+"\n"))
+		tn, err := newTun(c, test.NewLogger(), []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, false)
+		if err == nil {
+			t.Cleanup(func() { _ = tn.Close() })
+		}
+		return c, tn, err
+	}
+	if _, _, err := newWithDev(""); err != nil {
+		t.Skipf("can't create a tun device here: %v", err)
+	}
+	c, tn, err := newWithDev("nope")
+	require.NoError(t, err, "a bad name should fall back to a free tun")
+	require.Regexp(t, `^tun[0-9]+$`, tn.Device)
+
+	for _, dev := range []string{"nope", "", tn.Device, "nope"} {
+		res := c.ReloadConfigString("tun:\n  dev: " + dev + "\n")
+		require.NoError(t, res.Err)
+		assert.False(t, res.Failed, dev)
+		assert.False(t, res.RestartRequired, dev)
+	}
+
+	other := "tun0"
+	if tn.Device == other {
+		other = "tun1"
+	}
+	res := c.ReloadConfigString("tun:\n  dev: " + other + "\n")
+	require.NoError(t, res.Err)
+	assert.Equal(t, []string{"tun.dev"}, res.RestartKeys)
+}
+
+// An empty tun.dev takes the first tun nothing else has open. Reloading it empty, or naming the one we have, needs
+// no restart
+func TestTunEmptyDevPicksAFreeOne(t *testing.T) {
+	newEmpty := func() (*config.C, *tun, error) {
+		c := config.NewC(test.NewLogger())
+		require.NoError(t, c.LoadString("tun: {}\n"))
+		tn, err := newTun(c, test.NewLogger(), []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, false)
+		if err == nil {
+			t.Cleanup(func() { _ = tn.Close() })
+		}
+		return c, tn, err
+	}
+	c, first, err := newEmpty()
+	if err != nil {
+		t.Skipf("can't create a tun device here: %v", err)
+	}
+	_, second, err := newEmpty()
+	require.NoError(t, err, "the second tun should skip the one the first has open")
+	assert.NotEqual(t, first.Device, second.Device)
+
+	for _, cfg := range []string{
+		"tun: {}\npunchy:\n  punch: true\n",
+		"tun:\n  dev: " + first.Device + "\n",
+		"tun: {}\n",
+	} {
+		res := c.ReloadConfigString(cfg)
+		require.NoError(t, res.Err)
+		assert.False(t, res.Failed)
+		assert.False(t, res.RestartRequired, cfg)
+	}
+}

overlay/tun_darwin.go

@@ -80,17 +80,31 @@ type ifreqAlias6 struct {
 	Lifetime   addrLifetime
 }
 
+// utunIndex is the utun tun.dev asks for, or -1 for whichever is free. ok is false for a name that isn't utunN, which
+// also gets whichever is free
+func utunIndex(name string) (index int, ok bool) {
+	if name == "" || name == "utun" {
+		return -1, true
+	}
+	if _, err := fmt.Sscanf(name, "utun%d", &index); err != nil || index < 0 {
+		return -1, false
+	}
+	return index, true
+}
+
+// tunDevNeedsRestart is true when tun.dev asks for a particular utun that isn't the running one. Anything else takes
+// whichever is free, and the one we have will do
+func tunDevNeedsRestart(name, running string) bool {
+	index, _ := utunIndex(name)
+	return index >= 0 && fmt.Sprintf("utun%d", index) != running
+}
+
 func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*tun, error) {
-	name := c.GetString("tun.dev", "")
-	ifIndex := -1
-	if name != "" && name != "utun" {
-		_, err := fmt.Sscanf(name, "utun%d", &ifIndex)
-		if err != nil || ifIndex < 0 {
-			// NOTE: we don't make this error so we don't break existing
-			// configs that set a name before it was used.
-			l.Warn("interface name must be utun[0-9]+ on Darwin, ignoring")
-			ifIndex = -1
-		}
+	ifIndex, ok := utunIndex(c.GetString("tun.dev", ""))
+	if !ok {
+		// NOTE: we don't make this error so we don't break existing
+		// configs that set a name before it was used.
+		l.Warn("interface name must be utun[0-9]+ on Darwin, ignoring")
 	}
 
 	fd, err := unix.Socket(unix.AF_SYSTEM, unix.SOCK_DGRAM, unix.AF_SYS_CONTROL)
@@ -114,7 +128,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 		return nil, fmt.Errorf("SYS_CONNECT: %v", err)
 	}
 
-	name, err = unix.GetsockoptString(fd, unix.AF_SYS_CONTROL, _UTUN_OPT_IFNAME)
+	name, err := unix.GetsockoptString(fd, unix.AF_SYS_CONTROL, _UTUN_OPT_IFNAME)
 	if err != nil {
 		return nil, fmt.Errorf("failed to retrieve tun name: %w", err)
 	}
@@ -138,7 +152,10 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 	}
 
 	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
-		r.RestartRequiredIfChanged("tun.dev", "tun.mtu")
+		r.RestartRequiredIfChanged("tun.mtu")
+		if tunDevNeedsRestart(c.GetString("tun.dev", ""), t.Device) {
+			r.RestartRequiredIfChanged("tun.dev")
+		}
 		err := t.reload(c, false, r)
 		if err != nil {
 			util.LogWithContextIfNeeded("failed to reload tun device", err, t.l)

overlay/tun_darwin_route_test.go

@@ -103,3 +103,16 @@ func TestTunReloadLeavesRoutesItNeverInstalled(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, lo0.Index, routeInterfaceIn(msgs, netip.MustParsePrefix(cidr)))
 }
+
+// Only naming a different utun asks for a restart. Empty, plain utun and names that aren't utunN take whichever is free
+func TestTunDevNeedsRestart(t *testing.T) {
+	for name, want := range map[string]bool{
+		"":      false,
+		"utun":  false,
+		"nope":  false,
+		"utun7": false,
+		"utun3": true,
+	} {
+		assert.Equal(t, want, tunDevNeedsRestart(name, "utun7"), name)
+	}
+}

overlay/tun_netbsd.go

@@ -8,7 +8,6 @@ import (
 	"log/slog"
 	"net/netip"
 	"os"
-	"regexp"
 	"sync/atomic"
 	"syscall"
 	"unsafe"
@@ -67,24 +66,12 @@ type tun struct {
 	fd          int
 }
 
-var deviceNameRE = regexp.MustCompile(`^tun[0-9]+$`)
-
 func newTunFromFd(_ *config.C, _ *slog.Logger, _ int, _ []netip.Prefix) (*tun, error) {
 	return nil, fmt.Errorf("newTunFromFd not supported in NetBSD")
 }
 
 func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*tun, error) {
-	// Try to open tun device
-	var err error
-	deviceName := c.GetString("tun.dev", "")
-	if deviceName == "" {
-		return nil, fmt.Errorf("a device name in the format of /dev/tunN must be specified")
-	}
-	if !deviceNameRE.MatchString(deviceName) {
-		return nil, fmt.Errorf("a device name in the format of /dev/tunN must be specified")
-	}
-
-	fd, err := unix.Open("/dev/"+deviceName, os.O_RDWR, 0)
+	fd, deviceName, err := openTun(l, c.GetString("tun.dev", ""))
 	if err != nil {
 		return nil, err
 	}
@@ -109,7 +96,10 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 	}
 
 	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
-		r.RestartRequiredIfChanged("tun.dev", "tun.mtu")
+		r.RestartRequiredIfChanged("tun.mtu")
+		if tunDevNeedsRestart(c.GetString("tun.dev", ""), t.Device) {
+			r.RestartRequiredIfChanged("tun.dev")
+		}
 		err := t.reload(c, false, r)
 		if err != nil {
 			util.LogWithContextIfNeeded("failed to reload tun device", err, t.l)

overlay/tun_openbsd.go

@@ -8,7 +8,6 @@ import (
 	"log/slog"
 	"net/netip"
 	"os"
-	"regexp"
 	"sync/atomic"
 	"syscall"
 	"unsafe"
@@ -58,24 +57,12 @@ type tun struct {
 	fd          int
 }
 
-var deviceNameRE = regexp.MustCompile(`^tun[0-9]+$`)
-
 func newTunFromFd(_ *config.C, _ *slog.Logger, _ int, _ []netip.Prefix) (*tun, error) {
 	return nil, fmt.Errorf("newTunFromFd not supported in openbsd")
 }
 
 func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*tun, error) {
-	// Try to open tun device
-	var err error
-	deviceName := c.GetString("tun.dev", "")
-	if deviceName == "" {
-		return nil, fmt.Errorf("a device name in the format of /dev/tunN must be specified")
-	}
-	if !deviceNameRE.MatchString(deviceName) {
-		return nil, fmt.Errorf("a device name in the format of /dev/tunN must be specified")
-	}
-
-	fd, err := unix.Open("/dev/"+deviceName, os.O_RDWR, 0)
+	fd, deviceName, err := openTun(l, c.GetString("tun.dev", ""))
 	if err != nil {
 		return nil, err
 	}
@@ -100,7 +87,10 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 	}
 
 	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
-		r.RestartRequiredIfChanged("tun.dev", "tun.mtu")
+		r.RestartRequiredIfChanged("tun.mtu")
+		if tunDevNeedsRestart(c.GetString("tun.dev", ""), t.Device) {
+			r.RestartRequiredIfChanged("tun.dev")
+		}
 		err := t.reload(c, false, r)
 		if err != nil {
 			util.LogWithContextIfNeeded("failed to reload tun device", err, t.l)

overlay/tun_windows.go

@@ -109,7 +109,15 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*w
 	disableDNSRegistration(l, *guid)
 
 	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
-		r.RestartRequiredIfChanged("tun.dev", "tun.mtu", "tun.network_category")
+		r.RestartRequiredIfChanged("tun.dev", "tun.mtu")
+		// Compare with the running category, not the last config. A restart with one we can't parse would fail, so say
+		// that now rather than ask for one, and going back to the running one after a bad reload needs no restart
+		if cat, setCat, err := parseNetworkCategory(c.GetString("tun.network_category", "private")); err != nil {
+			t.l.Error("Invalid tun.network_category, a restart with it would fail", "error", err)
+			r.Failed()
+		} else if cat != t.networkCategory || setCat != t.setCategory {
+			r.RestartRequiredIfChanged("tun.network_category")
+		}
 		r.RestartRequiredIfChanged(interfaceBypassKeys...)
 		err := t.reload(c, false, r)
 		if err != nil {

overlay/tun_windows_route_test.go

@@ -181,6 +181,27 @@ func TestWinTun_ReloadRouteAlreadyGone(t *testing.T) {
 	})
 }
 
+// A network category we can't parse would fail the restart, so it fails the reload instead of asking for one
+func TestWinTun_ReloadInvalidNetworkCategoryFails(t *testing.T) {
+	rt := newRouteTest(t, unsafeRoutes())
+	for range 2 {
+		res := rt.c.ReloadConfigString(unsafeRoutes() + "  network_category: nope\n")
+		require.NoError(t, res.Err)
+		assert.True(t, res.Failed, "a bad category that didn't change since the last reload is still bad")
+		assert.False(t, res.RestartRequired)
+	}
+
+	// Fixing it back to the running category needs nothing
+	res := rt.c.ReloadConfigString(unsafeRoutes() + "  network_category: private\n")
+	require.NoError(t, res.Err)
+	assert.False(t, res.Failed)
+	assert.False(t, res.RestartRequired)
+
+	res = rt.c.ReloadConfigString(unsafeRoutes() + "  network_category: public\n")
+	require.NoError(t, res.Err)
+	assert.Equal(t, []string{"tun.network_category"}, res.RestartKeys)
+}
+
 // forFamilies runs f with two unsafe_routes CIDRs of each address family, the tun's own network stays IPv4
 func forFamilies(t *testing.T, f func(t *testing.T, a, b string)) {
 	t.Run("ipv4", func(t *testing.T) { f(t, "10.251.1.0/24", "10.251.2.0/24") })

pki.go

@@ -61,7 +61,18 @@ func NewPKIFromConfig(l *slog.Logger, c *config.C) (*PKI, error) {
 	}
 
 	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
-		r.RestartRequiredIfChanged("cipher")
+		// Compare with the running cipher, not the last config. A restart with one we can't run would fail, so say that
+		// now rather than ask for one, and going back to the running one after a bad reload needs no restart
+		if cipher := c.GetString("cipher", "aes"); !validCipher(cipher) {
+			l.Error("Unknown cipher, a restart with it would fail", "cipher", cipher)
+			r.Failed()
+		} else if cipher == "chachapoly" && fipsOnly() {
+			// newCipherSuite refuses it at startup
+			l.Error("ChaChaPoly is not allowed in FIPS 140-only mode, a restart with it would fail", "cipher", cipher)
+			r.Failed()
+		} else if cipher != pki.getCertState().cipher {
+			r.RestartRequiredIfChanged("cipher")
+		}
 		rErr := pki.reload(c, false, r)
 		if rErr != nil {
 			util.LogWithContextIfNeeded("Failed to reload PKI from config", rErr, l)
@@ -116,16 +127,21 @@ func (p *PKI) reload(c *config.C, initial bool, r *config.Reload) error {
 	return nil
 }
 
+// fipsOnly is fips140.Enforced, a variable so a test can run the FIPS 140-only reload check without GODEBUG
+var fipsOnly = fips140.Enforced
+
+// validCipher is true for the ciphers nebula can run. Each post-handshake CipherState in noiseutil hardcodes its own
+// nonce endianness, so there's nothing to set up for either.
+func validCipher(cipher string) bool {
+	return cipher == "aes" || cipher == "chachapoly"
+}
+
 func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError {
 	var cipher string
 	var currentState *CertState
 	if initial {
 		cipher = c.GetString("cipher", "aes")
-		switch cipher {
-		case "aes", "chachapoly":
-			// Each post-handshake CipherState in noiseutil hardcodes its own
-			// nonce endianness now, so there's nothing to set up here.
-		default:
+		if !validCipher(cipher) {
 			return util.NewContextualError(
 				"unknown cipher",
 				m{"cipher": cipher},

pki_test.go

@@ -1,6 +1,7 @@
 package nebula
 
 import (
+	"crypto/fips140"
 	"fmt"
 	"net/netip"
 	"strings"
@@ -128,6 +129,23 @@ func TestPKIReloadRestartRequired(t *testing.T) {
 	assert.True(t, res.RestartRequired)
 	assert.Equal(t, "aes", p.getCertState().cipher)
 
+	// A cipher we can't run would fail the restart, so it fails the reload instead of asking for one
+	c4 := config.NewC(test.NewLogger())
+	require.NoError(t, c4.LoadString(pkiConfig("aes", "10.1.0.2/24")))
+	_, err = NewPKIFromConfig(test.NewLogger(), c4)
+	require.NoError(t, err)
+	for range 2 {
+		res = c4.ReloadConfigString(pkiConfig("nope", "10.1.0.2/24"))
+		require.NoError(t, res.Err)
+		assert.True(t, res.Failed, "a bad cipher that didn't change since the last reload is still bad")
+		assert.False(t, res.RestartRequired)
+	}
+	// Fixing it back to the running cipher needs nothing
+	res = c4.ReloadConfigString(pkiConfig("aes", "10.1.0.2/24"))
+	require.NoError(t, res.Err)
+	assert.False(t, res.Failed)
+	assert.False(t, res.RestartRequired)
+
 	// A cert that keeps our networks applies without a restart
 	c3 := config.NewC(test.NewLogger())
 	require.NoError(t, c3.LoadString(pkiConfig("aes", "10.1.0.2/24")))
@@ -178,3 +196,24 @@ func TestPKIReloadNewVersionWithOtherNetworks(t *testing.T) {
 		assert.Equal(t, []netip.Prefix{netip.MustParsePrefix(tc.network)}, p.getCertState().myVpnNetworks, "accepted either way, as before")
 	}
 }
+
+// FIPS 140-only mode refuses chachapoly at startup, so reloading to it fails rather than asks for a restart
+func TestPKIReloadChachapolyInFIPSOnlyFails(t *testing.T) {
+	fipsOnly = func() bool { return true }
+	t.Cleanup(func() { fipsOnly = fips140.Enforced })
+	ca, _, caKey, caPEM := cert_test.NewTestCaCert(cert.Version2, cert.Curve_P256, time.Time{}, time.Time{}, nil, nil, nil)
+	_, _, keyPEM, certPEM := cert_test.NewTestCert(cert.Version2, cert.Curve_P256, ca, caKey, "host", time.Time{}, time.Time{}, []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, nil, nil)
+	pkiConfig := func(cipher string) string {
+		return fmt.Sprintf("cipher: %s\npki:\n  ca: %q\n  cert: %q\n  key: %q\n", cipher, caPEM, certPEM, keyPEM)
+	}
+
+	c := config.NewC(test.NewLogger())
+	require.NoError(t, c.LoadString(pkiConfig("aes")))
+	_, err := NewPKIFromConfig(test.NewLogger(), c)
+	require.NoError(t, err)
+
+	res := c.ReloadConfigString(pkiConfig("chachapoly"))
+	require.NoError(t, res.Err)
+	assert.True(t, res.Failed)
+	assert.False(t, res.RestartRequired)
+}