From 297767b2e369493a34f051ebc00258202b4c74c5 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Wed, 19 Nov 2025 11:06:34 -0600 Subject: [PATCH] warn user if they configure a firewall rule that will allow way more traffic than you might expect (#1513) * warn user if they accidentally configure a firewall rule that will allow way more traffic than you might expect * add groups-contains-any warning --- firewall.go | 78 +++++++++++++++++++++++++++++++++++++----------- firewall_test.go | 60 +++++++++++++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 20 deletions(-) diff --git a/firewall.go b/firewall.go index 6bf470d..101767a 100644 --- a/firewall.go +++ b/firewall.go @@ -8,6 +8,7 @@ import ( "hash/fnv" "net/netip" "reflect" + "slices" "strconv" "strings" "sync" @@ -337,7 +338,6 @@ func AddFirewallRulesFromConfig(l *logrus.Logger, inbound bool, c *config.C, fw } for i, t := range rs { - var groups []string r, err := convertRule(l, t, table, i) if err != nil { return fmt.Errorf("%s rule #%v; %s", table, i, err) @@ -347,23 +347,10 @@ func AddFirewallRulesFromConfig(l *logrus.Logger, inbound bool, c *config.C, fw return fmt.Errorf("%s rule #%v; only one of port or code should be provided", table, i) } - if r.Host == "" && len(r.Groups) == 0 && r.Group == "" && r.Cidr == "" && r.LocalCidr == "" && r.CAName == "" && r.CASha == "" { + if r.Host == "" && len(r.Groups) == 0 && r.Cidr == "" && r.LocalCidr == "" && r.CAName == "" && r.CASha == "" { return fmt.Errorf("%s rule #%v; at least one of host, group, cidr, local_cidr, ca_name, or ca_sha must be provided", table, i) } - if len(r.Groups) > 0 { - groups = r.Groups - } - - if r.Group != "" { - // Check if we have both groups and group provided in the rule config - if len(groups) > 0 { - return fmt.Errorf("%s rule #%v; only one of group or groups should be defined, both provided", table, i) - } - - groups = []string{r.Group} - } - var sPort, errPort string if r.Code != "" { errPort = "code" @@ -408,7 +395,11 @@ func AddFirewallRulesFromConfig(l *logrus.Logger, inbound bool, c *config.C, fw } } - err = fw.AddRule(inbound, proto, startPort, endPort, groups, r.Host, cidr, localCidr, r.CAName, r.CASha) + if warning := r.sanity(); warning != nil { + l.Warnf("%s rule #%v; %s", table, i, warning) + } + + err = fw.AddRule(inbound, proto, startPort, endPort, r.Groups, r.Host, cidr, localCidr, r.CAName, r.CASha) if err != nil { return fmt.Errorf("%s rule #%v; `%s`", table, i, err) } @@ -922,7 +913,6 @@ type rule struct { Code string Proto string Host string - Group string Groups []string Cidr string LocalCidr string @@ -964,7 +954,8 @@ func convertRule(l *logrus.Logger, p any, table string, i int) (rule, error) { l.Warnf("%s rule #%v; group was an array with a single value, converting to simple value", table, i) m["group"] = v[0] } - r.Group = toString("group", m) + + singleGroup := toString("group", m) if rg, ok := m["groups"]; ok { switch reflect.TypeOf(rg).Kind() { @@ -981,9 +972,60 @@ func convertRule(l *logrus.Logger, p any, table string, i int) (rule, error) { } } + //flatten group vs groups + if singleGroup != "" { + // Check if we have both groups and group provided in the rule config + if len(r.Groups) > 0 { + return r, fmt.Errorf("only one of group or groups should be defined, both provided") + } + r.Groups = []string{singleGroup} + } + return r, nil } +// sanity returns an error if the rule would be evaluated in a way that would short-circuit a configured check on a wildcard value +// rules are evaluated as "port AND proto AND (ca_sha OR ca_name) AND (host OR group OR groups OR cidr) AND local_cidr" +func (r *rule) sanity() error { + //port, proto, local_cidr are AND, no need to check here + //ca_sha and ca_name don't have a wildcard value, no need to check here + groupsEmpty := len(r.Groups) == 0 + hostEmpty := r.Host == "" + cidrEmpty := r.Cidr == "" + + if (groupsEmpty && hostEmpty && cidrEmpty) == true { + return nil //no content! + } + + groupsHasAny := slices.Contains(r.Groups, "any") + if groupsHasAny && len(r.Groups) > 1 { + return fmt.Errorf("groups spec [%s] contains the group '\"any\". This rule will ignore the other groups specified", r.Groups) + } + + if r.Host == "any" { + if !groupsEmpty { + return fmt.Errorf("groups specified as %s, but host=any will match any host, regardless of groups", r.Groups) + } + + if !cidrEmpty { + return fmt.Errorf("cidr specified as %s, but host=any will match any host, regardless of cidr", r.Cidr) + } + } + + if groupsHasAny { + if !hostEmpty && r.Host != "any" { + return fmt.Errorf("groups spec [%s] contains the group '\"any\". This rule will ignore the specified host %s", r.Groups, r.Host) + } + if !cidrEmpty { + return fmt.Errorf("groups spec [%s] contains the group '\"any\". This rule will ignore the specified cidr %s", r.Groups, r.Cidr) + } + } + + //todo alert on cidr-any + + return nil +} + func parsePort(s string) (startPort, endPort int32, err error) { if s == "any" { startPort = firewall.PortAny diff --git a/firewall_test.go b/firewall_test.go index a645b5c..1aaddce 100644 --- a/firewall_test.go +++ b/firewall_test.go @@ -1040,7 +1040,7 @@ func TestFirewall_convertRule(t *testing.T) { r, err := convertRule(l, c, "test", 1) assert.Contains(t, ob.String(), "test rule #1; group was an array with a single value, converting to simple value") require.NoError(t, err) - assert.Equal(t, "group1", r.Group) + assert.Equal(t, []string{"group1"}, r.Groups) // Ensure group array of > 1 is errord ob.Reset() @@ -1060,7 +1060,63 @@ func TestFirewall_convertRule(t *testing.T) { r, err = convertRule(l, c, "test", 1) require.NoError(t, err) - assert.Equal(t, "group1", r.Group) + assert.Equal(t, []string{"group1"}, r.Groups) +} + +func TestFirewall_convertRuleSanity(t *testing.T) { + l := test.NewLogger() + ob := &bytes.Buffer{} + l.SetOutput(ob) + + noWarningPlease := []map[string]any{ + {"group": "group1"}, + {"groups": []any{"group2"}}, + {"host": "bob"}, + {"cidr": "1.1.1.1/1"}, + {"groups": []any{"group2"}, "host": "bob"}, + {"cidr": "1.1.1.1/1", "host": "bob"}, + {"groups": []any{"group2"}, "cidr": "1.1.1.1/1"}, + {"groups": []any{"group2"}, "cidr": "1.1.1.1/1", "host": "bob"}, + } + for _, c := range noWarningPlease { + r, err := convertRule(l, c, "test", 1) + require.NoError(t, err) + require.NoError(t, r.sanity(), "should not generate a sanity warning, %+v", c) + } + + yesWarningPlease := []map[string]any{ + {"group": "group1"}, + {"groups": []any{"group2"}}, + {"cidr": "1.1.1.1/1"}, + {"groups": []any{"group2"}, "host": "bob"}, + {"cidr": "1.1.1.1/1", "host": "bob"}, + {"groups": []any{"group2"}, "cidr": "1.1.1.1/1"}, + {"groups": []any{"group2"}, "cidr": "1.1.1.1/1", "host": "bob"}, + } + for _, c := range yesWarningPlease { + c["host"] = "any" + r, err := convertRule(l, c, "test", 1) + require.NoError(t, err) + err = r.sanity() + require.Error(t, err, "I wanted a warning: %+v", c) + } + //reset the list + yesWarningPlease = []map[string]any{ + {"group": "group1"}, + {"groups": []any{"group2"}}, + {"cidr": "1.1.1.1/1"}, + {"groups": []any{"group2"}, "host": "bob"}, + {"cidr": "1.1.1.1/1", "host": "bob"}, + {"groups": []any{"group2"}, "cidr": "1.1.1.1/1"}, + {"groups": []any{"group2"}, "cidr": "1.1.1.1/1", "host": "bob"}, + } + for _, c := range yesWarningPlease { + r, err := convertRule(l, c, "test", 1) + require.NoError(t, err) + r.Groups = append(r.Groups, "any") + err = r.sanity() + require.Error(t, err, "I wanted a warning: %+v", c) + } } type testcase struct {