From b531c4748674feaf715a51c40e7e2624f3fed065 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 22 Sep 2026 18:29:06 +0200 Subject: [PATCH] Refuse an opening ufw would merge into a found rule that does other than a plain allow, and read log types in either place (hq ADR 0103) --- internal/firewall/firewall.go | 41 +++++++++++++++++++++++++++--- internal/firewall/firewall_test.go | 37 ++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/internal/firewall/firewall.go b/internal/firewall/firewall.go index fdc52db..789af22 100644 --- a/internal/firewall/firewall.go +++ b/internal/firewall/firewall.go @@ -479,6 +479,7 @@ func words(rule string) []string { type ufwRule struct { route bool action, in, out string + log string from, fromPort, to string port, proto, app string comment string @@ -488,8 +489,23 @@ type ufwRule struct { // in on mesh0 to any port 5432 proto tcp` — into the fields ufw compares. Not ok for anything it // does not recognise, which is then never taken to answer an opening. func parseRule(rule string) (ufwRule, bool) { - w := words(rule) r := ufwRule{from: "any", to: "any", comment: comment(rule)} + // A log type may stand after the action or after the direction; either way it is a property + // of the rule, not of where it matches. The word after `comment` is the comment, whatever it + // says. + var w []string + all := words(rule) + for i := 0; i < len(all); i++ { + switch { + case all[i] == "comment" && i+1 < len(all): + w = append(w, all[i], all[i+1]) + i++ + case all[i] == "log" || all[i] == "log-all": + r.log = all[i] + default: + w = append(w, all[i]) + } + } i := 0 if i < len(w) && w[i] == "route" { r.route = true @@ -585,9 +601,13 @@ func isPorts(s string) bool { return true } -// sameAs is whether ufw would take two rules for one — everything but the comment equal. +// sameAs is whether ufw would take two rules for one: everything equal but the comment, the +// action and the log type. Adding one beside the other updates it in place — its comment, and its +// action or log type — rather than adding a second. func (r ufwRule) sameAs(o ufwRule) bool { r.comment, o.comment = "", "" + r.action, o.action = "", "" + r.log, o.log = "", "" return r == o } @@ -658,6 +678,7 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converge present := false var stale []string satisfiedBy := "" + want, _ := parseRule(strings.Join(Rule(o), " ")) for _, rule := range rules { c := comment(rule) switch { @@ -666,7 +687,21 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converge case markedFor(c, o.ID): stale = append(stale, rule) default: - if parsed, ok := parseRule(rule); ok && satisfiedBy == "" && parsed.admits(o) { + parsed, ok := parseRule(rule) + if !ok { + continue + } + // ufw would take the mesh's rule for this one and rewrite its action or log type: + // an operator's refusal, or a limit, would silently become an allow — and removing the + // opening would then delete it. A plain allow answers the opening; anything else is a + // conflict the operator decides (novox/hq ADR 0103). + if parsed.sameAs(want) && (parsed.action != "allow" || parsed.log != "") { + return Converged{}, fmt.Errorf("ufw holds %q, which ufw takes for the same rule as the "+ + "mesh's opening for %s, differing in what it does; adding the opening would change "+ + "it, so nothing was added. Change or remove that rule, or have the mesh stop "+ + "declaring the opening", rule, o.Target()) + } + if satisfiedBy == "" && parsed.admits(o) { satisfiedBy = rule } } diff --git a/internal/firewall/firewall_test.go b/internal/firewall/firewall_test.go index 33e88ad..6d0d98c 100644 --- a/internal/firewall/firewall_test.go +++ b/internal/firewall/firewall_test.go @@ -658,7 +658,6 @@ func TestARuleThatDoesNotAnswerTheOpeningLeavesItToBeAdded(t *testing.T) { "allow from 192.0.2.0/24 to any port 5671 proto tcp", // narrower: from one range "allow in on eth0 to any port 5671 proto tcp", // narrower: one interface "allow 5671/udp", // another protocol - "deny 5671/tcp", // refuses "route allow 5671/tcp", // another path "allow to 192.0.2.1 port 5671 proto tcp", // one address } { @@ -681,3 +680,39 @@ func TestAnOpeningWhoseFoundRuleIsGoneIsAddedAgain(t *testing.T) { t.Fatalf("the opening was not added once nothing answered it: %+v %v", done, err) } } + +func TestARuleUfwWouldMergeThatDoesOtherThanAllowRefusesTheOpening(t *testing.T) { + // ufw takes two rules differing only in action or log type for one, and adding the mesh's + // would turn the operator's refusal into an allow (novox/hq ADR 0103). + for _, operators := range []string{ + "deny 5671/tcp", + "reject 5671/tcp", + "limit 5671/tcp", + "allow log 5671/tcp", + "allow log-all proto tcp to any port 5671", + "deny in log to any port 5671 proto tcp comment 'operator note'", + } { + f := &fakeUFW{installed: true, active: true, rules: []string{operators}} + _, err := Converge(context.Background(), f.run, opening("adoption.bus", 5671, "everywhere", "incoming", 0)) + if err == nil || !strings.Contains(err.Error(), operators) { + t.Errorf("%q: the conflict was not refused naming the rule: %v", operators, err) + } + if f.added() != 0 || len(f.rules) != 1 || f.rules[0] != operators { + t.Errorf("%q: something was added or changed: %v %v", operators, f.asked, f.rules) + } + } +} + +func TestALogTypeIsReadInEitherPlace(t *testing.T) { + for rule, want := range map[string]string{ + "allow log 22/tcp": "allow log 22 tcp in=", + "allow in log-all on mesh0 to any port 5432 proto tcp": "allow log-all 5432 tcp in=mesh0", + "route deny log in on mesh0 to any port 80 proto tcp": "deny log 80 tcp in=mesh0", + "allow 22/tcp comment 'log'": "allow 22 tcp in=", + } { + r, ok := parseRule(rule) + if got := r.action + " " + r.log + " " + r.port + " " + r.proto + " in=" + r.in; !ok || got != want { + t.Errorf("%q read as %q (%v), want %q", rule, got, ok, want) + } + } +}