From b04d71d96a56463cb1d7c6d2dfaa03eb9336865b Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Tue, 20 Oct 2026 10:37:48 -0400 Subject: [PATCH] Compare a firewall reload with what the running firewall was built from (#1955) --- config/config.go | 15 ++++++++++++ firewall.go | 4 ++++ interface.go | 11 +++++---- interface_test.go | 61 ++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 83 insertions(+), 8 deletions(-) diff --git a/config/config.go b/config/config.go index ffa039be..19095156 100644 --- a/config/config.go +++ b/config/config.go @@ -86,6 +86,21 @@ func (c *C) InitialLoad() bool { return c.oldSettings == nil } +// Render returns keys and their values as yaml, for a subsystem to remember what it applied. Empty means they couldn't +// be rendered and should never match. +func (c *C) Render(keys ...string) string { + m := make(map[string]any, len(keys)) + for _, k := range keys { + m[k] = c.Get(k) + } + b, err := yaml.Marshal(m) + if err != nil { + c.l.Debug("Failed to render config", "keys", keys, "error", err) + return "" + } + return string(b) +} + // HasChanged checks if the underlying structure of the provided key has changed after a config reload. The value of // k in both the old and new settings will be serialized, the result of the string comparison is returned. // If k is an empty string the entire config is tested. diff --git a/firewall.go b/firewall.go index 5fb2d6de..6029d888 100644 --- a/firewall.go +++ b/firewall.go @@ -66,6 +66,9 @@ type Firewall struct { rules string rulesVersion uint16 + // source is the rendered firewall config this firewall was built from, the cert networks are kept above + source string + defaultLocalCIDRAny bool incomingMetrics firewallMetrics outgoingMetrics firewallMetrics @@ -246,6 +249,7 @@ func NewFirewallFromConfig(l *slog.Logger, cs *CertState, c *config.C) (*Firewal return nil, err } + fw.source = c.Render("firewall") return fw, nil } diff --git a/interface.go b/interface.go index 96d44446..e229f52a 100644 --- a/interface.go +++ b/interface.go @@ -537,11 +537,12 @@ func (f *Interface) reloadFirewall(c *config.C, r *config.Reload) { curCert = cs.getCertificate(cert.Version1) } - // The firewall builds its routableNetworks set from the certificate's UnsafeNetworks at construction. - // Check to see if that set has changed, and if so, rebuild the firewall. - certUnsafeChanged := curCert != nil && !slices.Equal(curCert.UnsafeNetworks(), f.firewall.unsafeNetworks) - - if !c.HasChanged("firewall") && !certUnsafeChanged { + // Compare with what the running firewall was built from, not the last config, so a firewall that failed to build + // is tried again. The cert is the one this reload brought, PKI registers its reload callback before this one + src := c.Render("firewall") + if src != "" && src == f.firewall.source && curCert != nil && + slices.Equal(curCert.Networks(), f.firewall.assignedNetworks) && + slices.Equal(curCert.UnsafeNetworks(), f.firewall.unsafeNetworks) { f.l.Debug("No firewall config change detected") return } diff --git a/interface_test.go b/interface_test.go index b66f1e81..8bd25763 100644 --- a/interface_test.go +++ b/interface_test.go @@ -135,8 +135,63 @@ func TestReloadFirewall_BadRulesFailTheReload(t *testing.T) { f := &Interface{pki: pki, firewall: fw, l: l} cfg.RegisterReloadCallback(f.reloadFirewall) - res := cfg.ReloadConfigString(rules("nope")) + // The running firewall still isn't built from what the config asks for, so the same reload fails again + for range 2 { + res := cfg.ReloadConfigString(rules("nope")) + require.NoError(t, res.Err) + assert.True(t, res.Failed) + assert.Same(t, fw, f.firewall) + } + + res := cfg.ReloadConfigString(rules("tcp")) require.NoError(t, res.Err) - assert.True(t, res.Failed) - assert.Same(t, fw, f.firewall) + assert.False(t, res.Failed) + assert.NotSame(t, fw, f.firewall) +} + +// Every input the firewall is built from rebuilds it when it changes, and nothing else does +func TestReloadFirewall_RebuildsOnAnyInput(t *testing.T) { + l := test.NewLogger() + networks := []netip.Prefix{netip.MustParsePrefix("10.0.0.1/24")} + base := "firewall:\n inbound:\n - port: any\n proto: any\n host: any\n" + setup := func() (*config.C, *Interface, *PKI) { + pki := &PKI{} + pki.cs.Store(&CertState{v2Cert: &dummyCert{version: cert.Version2, networks: networks}, initiatingVersion: cert.Version2}) + cfg := config.NewC(l) + require.NoError(t, cfg.LoadString(base)) + fw, err := NewFirewallFromConfig(l, pki.getCertState(), cfg) + require.NoError(t, err) + f := &Interface{pki: pki, firewall: fw, l: l} + cfg.RegisterReloadCallback(f.reloadFirewall) + return cfg, f, pki + } + setCert := func(pki *PKI, networks, unsafe []netip.Prefix) { + pki.cs.Store(&CertState{v2Cert: &dummyCert{version: cert.Version2, networks: networks, unsafeNetworks: unsafe}, initiatingVersion: cert.Version2}) + } + + cases := map[string]struct { + cfg string + cert func(*PKI) + want bool + }{ + "nothing": {base + "punchy:\n punch: true\n", nil, false}, + "rules": {base + " outbound:\n - port: any\n proto: any\n host: any\n", nil, true}, + "inbound_action": {base + " inbound_action: reject\n", nil, true}, + "conntrack": {base + " conntrack:\n tcp_timeout: 1m\n", nil, true}, + "networks": {base, func(p *PKI) { setCert(p, []netip.Prefix{netip.MustParsePrefix("10.0.0.2/24")}, nil) }, true}, + "unsafe_networks": {base, func(p *PKI) { setCert(p, networks, []netip.Prefix{netip.MustParsePrefix("192.168.0.0/24")}) }, true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cfg, f, pki := setup() + old := f.firewall + if tc.cert != nil { + tc.cert(pki) + } + res := cfg.ReloadConfigString(tc.cfg) + require.NoError(t, res.Err) + assert.False(t, res.Failed) + assert.Equal(t, tc.want, f.firewall != old) + }) + } }