From a203d044bba01ba2b1dcf2044eb3176d4ca3fb13 Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Fri, 16 Oct 2026 00:22:47 -0400 Subject: [PATCH] Fail the reload when a config entry is skipped with a warning (#1952) --- hostmap.go | 12 +++++++----- hostmap_test.go | 13 +++++++++++++ lighthouse.go | 9 ++++++--- lighthouse_test.go | 30 +++++++++++++++++++++++++++++- main.go | 2 +- ssh.go | 12 ++++++++++-- sshd_reload_test.go | 26 +++++++++++++++++++++++++- 7 files changed, 91 insertions(+), 13 deletions(-) diff --git a/hostmap.go b/hostmap.go index a3eb0c29..24dc6411 100644 --- a/hostmap.go +++ b/hostmap.go @@ -316,9 +316,9 @@ type cachedPacketMetrics struct { func NewHostMapFromConfig(l *slog.Logger, c *config.C) *HostMap { hm := newHostMap(l) - hm.reload(c, true) - c.RegisterReloadCallback(func(c *config.C, _ *config.Reload) { - hm.reload(c, false) + hm.reload(c, true, nil) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + hm.reload(c, false, r) }) l.Info("Main HostMap created", "preferredRanges", hm.GetPreferredRanges()) @@ -337,7 +337,8 @@ func newHostMap(l *slog.Logger) *HostMap { } } -func (hm *HostMap) reload(c *config.C, initial bool) { +// r is nil on the initial load +func (hm *HostMap) reload(c *config.C, initial bool, r *config.Reload) { if initial || c.HasChanged("preferred_ranges") { var preferredRanges []netip.Prefix rawPreferredRanges := c.GetStringSlice("preferred_ranges", []string{}) @@ -348,8 +349,9 @@ func (hm *HostMap) reload(c *config.C, initial bool) { if err != nil { hm.l.Warn("Failed to parse preferred ranges, ignoring", "error", err, - "range", rawPreferredRanges, + "range", rawPreferredRange, ) + r.Failed() continue } diff --git a/hostmap_test.go b/hostmap_test.go index b522af70..e1984b3e 100644 --- a/hostmap_test.go +++ b/hostmap_test.go @@ -447,3 +447,16 @@ func TestHostInfo_markOutOnly(t *testing.T) { assert.True(t, h.markOut(6), "a relayed send must not consume the epoch edge") assert.False(t, h.markOut(6)) } + +// A preferred range that doesn't parse is skipped and the rest are used, the reload says one was skipped +func TestHostMap_reloadBadPreferredRangeFails(t *testing.T) { + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.LoadString("preferred_ranges: [10.0.0.0/8]\n")) + hm := NewHostMapFromConfig(l, c) + + res := c.ReloadConfigString("preferred_ranges: [10.0.0.0/8, nope]\n") + require.NoError(t, res.Err) + assert.True(t, res.Failed) + assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, hm.GetPreferredRanges()) +} diff --git a/lighthouse.go b/lighthouse.go index 5e610111..49cf8b14 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -138,14 +138,14 @@ func NewLightHouseFromConfig(ctx context.Context, l *slog.Logger, c *config.C, c h.metrics = newLighthouseMetrics() } - err := h.reload(c, true) + err := h.reload(c, true, nil) if err != nil { return nil, err } c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { r.RestartRequiredIfChanged("lighthouse.am_lighthouse", "listen.port", "handshakes.query_buffer", "stats.lighthouse_metrics") - err := h.reload(c, false) + err := h.reload(c, false, r) switch v := err.(type) { case *util.ContextualError: v.Log(l) @@ -193,7 +193,8 @@ func (lh *LightHouse) GetUpdateInterval() int64 { return lh.interval.Load() } -func (lh *LightHouse) reload(c *config.C, initial bool) error { +// r is nil on the initial load +func (lh *LightHouse) reload(c *config.C, initial bool, r *config.Reload) error { if initial || c.HasChanged("lighthouse.advertise_addrs") { rawAdvAddrs := c.GetStringSlice("lighthouse.advertise_addrs", []string{}) advAddrs := make([]netip.AddrPort, 0) @@ -228,6 +229,7 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error { "addr", rawAddr, "entry", i+1, ) + r.Failed() continue } @@ -386,6 +388,7 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error { "relay", v, "error", err, ) + r.Failed() } else { lh.l.Info("Read relay from config", "relay", v) relaysForMe = append(relaysForMe, configRIP) diff --git a/lighthouse_test.go b/lighthouse_test.go index 4d1c5368..23d0efd5 100644 --- a/lighthouse_test.go +++ b/lighthouse_test.go @@ -296,7 +296,7 @@ func TestLighthouse_reload(t *testing.T) { require.NoError(t, err) c.ReloadConfigString(string(rc)) - err = lh.reload(c, false) + err = lh.reload(c, false, nil) require.NoError(t, err) } @@ -1014,3 +1014,31 @@ func TestLighthouse_reloadRestartRequired(t *testing.T) { require.NoError(t, res.Err) assert.True(t, res.RestartRequired) } + +// A relay that doesn't parse is skipped and the rest are used, the reload says one was skipped +func TestLighthouse_reloadBadRelayFails(t *testing.T) { + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.LoadString("listen:\n port: 4242\n")) + lh, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(netip.MustParsePrefix("10.128.0.1/24")), nil, nil) + require.NoError(t, err) + + res := c.ReloadConfigString("listen:\n port: 4242\nrelay:\n relays: [10.128.0.2, nope]\n") + require.NoError(t, res.Err) + assert.True(t, res.Failed) + assert.Equal(t, []netip.Addr{netip.MustParseAddr("10.128.0.2")}, lh.GetRelaysForMe()) +} + +// An advertise_addrs entry inside our own network is skipped and the rest are used, the reload says one was skipped +func TestLighthouse_reloadAdvertiseAddrInNetworkFails(t *testing.T) { + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.LoadString("listen:\n port: 4242\n")) + lh, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(netip.MustParsePrefix("10.128.0.1/24")), nil, nil) + require.NoError(t, err) + + res := c.ReloadConfigString("listen:\n port: 4242\nlighthouse:\n advertise_addrs: [\"1.2.3.4:4242\", \"10.128.0.5:4242\"]\n") + require.NoError(t, res.Err) + assert.True(t, res.Failed) + assert.Equal(t, []netip.AddrPort{netip.MustParseAddrPort("1.2.3.4:4242")}, lh.GetAdvertiseAddrs()) +} diff --git a/main.go b/main.go index e7adfd58..105bc6d9 100644 --- a/main.go +++ b/main.go @@ -78,7 +78,7 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev wireSSHReload(l, ssh, c) var sshStart func() if c.GetBool("sshd.enabled", false) { - sshStart, err = configSSH(l, ssh, c) + sshStart, err = configSSH(l, ssh, c, nil) if err != nil { l.Warn("Failed to configure sshd, ssh debugging will not be available", "error", err) sshStart = nil diff --git a/ssh.go b/ssh.go index 716f105d..ab4fd022 100644 --- a/ssh.go +++ b/ssh.go @@ -78,7 +78,7 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) { } return } - sshRun, err := configSSH(l, ssh, c) + sshRun, err := configSSH(l, ssh, c, r) if err != nil { l.Error("Failed to reconfigure the sshd", "error", err) ssh.Stop() @@ -117,7 +117,8 @@ func loadSSHHostKey(c *config.C) ([]byte, error) { // updates the passed-in SSHServer. On success, it returns a function // that callers may invoke to run the configured ssh server. On // failure, it returns nil, error. -func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) { +// r is nil at startup, entries that are skipped with a warning fail it on a reload +func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C, r *config.Reload) (func(), error) { listen := c.GetString("sshd.listen", "") if listen == "" { return nil, fmt.Errorf("sshd.listen must be provided") @@ -150,6 +151,7 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) err := ssh.AddTrustedCA(caAuthorizedKey) if err != nil { l.Warn("SSH CA had an error, ignoring", "error", err, "sshCA", caAuthorizedKey) + r.Failed() continue } } @@ -161,12 +163,14 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) kDef, ok := rk.(map[string]any) if !ok { l.Warn("Authorized user had an error, ignoring", "sshKeyConfig", rk) + r.Failed() continue } user, ok := kDef["user"].(string) if !ok { l.Warn("Authorized user is missing the user field", "sshKeyConfig", rk) + r.Failed() continue } @@ -180,6 +184,7 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) "sshKeyConfig", rk, "sshKey", v, ) + r.Failed() continue } @@ -191,6 +196,7 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) "sshKeyConfig", rk, "sshKey", subK, ) + r.Failed() continue } @@ -200,12 +206,14 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) "error", err, "sshKeyConfig", sk, ) + r.Failed() continue } } default: l.Warn("Authorized user is missing the keys field or was not understood", "sshKeyConfig", rk) + r.Failed() } } } else { diff --git a/sshd_reload_test.go b/sshd_reload_test.go index b7f8a6fa..4d650dc0 100644 --- a/sshd_reload_test.go +++ b/sshd_reload_test.go @@ -47,7 +47,7 @@ func newSSHDReloadHarness(t *testing.T) *sshdReloadHarness { h.server, err = sshd.NewSSHServer(t.Context(), l) require.NoError(t, err) wireSSHReload(l, h.server, h.c) - run, err := configSSH(l, h.server, h.c) + run, err := configSSH(l, h.server, h.c, nil) require.NoError(t, err) go run() t.Cleanup(h.server.Stop) @@ -226,3 +226,27 @@ func TestSSHDReload_BadHostKeyFileStops(t *testing.T) { assert.True(t, h.c.ReloadConfigString(h.config()).Failed) require.Eventually(t, func() bool { return !h.server.Running() }, 5*time.Second, 10*time.Millisecond) } + +// A trusted CA or authorized user that is skipped with a warning fails the reload, the valid ones are still used +func TestSSHDReload_SkippedEntriesFail(t *testing.T) { + cases := map[string]string{ + "bad trusted ca": " trusted_cas: [nope]\n", + "user entry not a map": " - nope\n", + "user missing": " - keys: [nope]\n", + "bad key": " - user: other\n keys: nope\n", + "key in a list not a string": " - user: other\n keys: [5]\n", + "bad key in a list": " - user: other\n keys: [nope]\n", + "keys missing": " - user: other\n", + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + h := newSSHDReloadHarness(t) + h.extra = extra + res := h.c.ReloadConfigString(h.config()) + require.NoError(t, res.Err) + assert.True(t, res.Failed) + require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond) + h.connect(t) + }) + } +}