diff --git a/examples/config.yml b/examples/config.yml index ef29812e..e04e9dbe 100644 --- a/examples/config.yml +++ b/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 diff --git a/overlay/tun_bsd_dev.go b/overlay/tun_bsd_dev.go new file mode 100644 index 00000000..6a21eea9 --- /dev/null +++ b/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) + } + } +} diff --git a/overlay/tun_bsd_reload_test.go b/overlay/tun_bsd_reload_test.go new file mode 100644 index 00000000..a6ce15b8 --- /dev/null +++ b/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) + } +} diff --git a/overlay/tun_darwin.go b/overlay/tun_darwin.go index dcde7fae..5a62f061 100644 --- a/overlay/tun_darwin.go +++ b/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) diff --git a/overlay/tun_darwin_route_test.go b/overlay/tun_darwin_route_test.go index a566dd38..5eda4813 100644 --- a/overlay/tun_darwin_route_test.go +++ b/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) + } +} diff --git a/overlay/tun_netbsd.go b/overlay/tun_netbsd.go index 056e5f98..720bfe39 100644 --- a/overlay/tun_netbsd.go +++ b/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) diff --git a/overlay/tun_openbsd.go b/overlay/tun_openbsd.go index d9aa7a30..9066d7a2 100644 --- a/overlay/tun_openbsd.go +++ b/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) diff --git a/overlay/tun_windows.go b/overlay/tun_windows.go index fcb6ac93..cca4eeed 100644 --- a/overlay/tun_windows.go +++ b/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 { diff --git a/overlay/tun_windows_route_test.go b/overlay/tun_windows_route_test.go index 1546dfd6..e9583f28 100644 --- a/overlay/tun_windows_route_test.go +++ b/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") }) diff --git a/pki.go b/pki.go index 0e4deea3..7956ee4a 100644 --- a/pki.go +++ b/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}, diff --git a/pki_test.go b/pki_test.go index 572d4c21..7f05b103 100644 --- a/pki_test.go +++ b/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) +}