Add no opening a found ufw rule already answers, since ufw takes rules differing only in comment for one, as captured on a lab machine (hq ADR 0103)

2026-09-22 18:06:47 +02:00
parent da65f84c45
commit 52e139d96f
6 changed files with 433 additions and 28 deletions
@@ -151,14 +151,17 @@ func (f *fakeUFW) iptables(name string, args []string) (string, error) {
 	return strings.Join(out, "\n") + "\n", nil
 }

-func canonical(args []string) string {
-	var route, in, port, proto, comment string
+// canonical is a rule the way ufw prints it back, as captured (testdata/ufw-comment-only.txt): the
+// short form `allow 5671/tcp` for a rule on no interface, the long form `allow in on mesh0 to any
+// port 5432 proto tcp` for one on an interface; the comment last.
+func canonical(args []string) (rule, commentText string) {
+	var route, in, port, proto string
 	for i := 0; i < len(args); i++ {
 		switch args[i] {
 		case "route":
 			route = "route "
 		case "in":
-			in = "in on " + args[i+2] + " "
+			in = args[i+2]
 			i += 2
 		case "port":
 			port = args[i+1]
@@ -167,15 +170,14 @@ func canonical(args []string) string {
 			proto = args[i+1]
 			i++
 		case "comment":
-			comment = args[i+1]
+			commentText = args[i+1]
 			i++
 		}
 	}
-	line := route + "allow " + in + port + "/" + proto
-	if comment != "" {
-		line += " comment '" + comment + "'"
+	if in != "" {
+		return route + "allow in on " + in + " to any port " + port + " proto " + proto, commentText
 	}
-	return line
+	return route + "allow " + port + "/" + proto, commentText
 }

 func (f *fakeUFW) run(_ context.Context, name string, args ...string) (string, error) {
@@ -235,8 +237,21 @@ func (f *fakeUFW) run(_ context.Context, name string, args ...string) (string, e
 		}
 		return "", errors.New("Could not delete non-existent rule")
 	default:
-		f.rules = append(f.rules, canonical(args))
-		return "Rule added\n", nil
+		rule, note := canonical(args)
+		line := rule
+		if note != "" {
+			line += " comment '" + note + "'"
+		}
+		// As the real ufw does (testdata/ufw-comment-only.txt): a rule differing from one it holds
+		// only in its comment is the same rule, and its comment is replaced.
+		for i, r := range f.rules {
+			if bare, _, _ := strings.Cut(r, " comment '"); bare == rule {
+				f.rules[i] = line
+				return "Rule updated\nRule updated (v6)\n", nil
+			}
+		}
+		f.rules = append(f.rules, line)
+		return "Rule added\nRule added (v6)\n", nil
 	}
 }

@@ -276,14 +291,14 @@ func TestAnOpeningIsAddedOnceAndMarkedAsTheMeshs(t *testing.T) {
 	o := opening("adoption.opening-tcp-5671-incoming", 5671, "everywhere", "incoming", 0)

 	action, err := Converge(context.Background(), f.run, o)
-	if err != nil || action != "created" {
+	if err != nil || action.Action != "created" {
 		t.Fatalf("first converge: %q %v", action, err)
 	}
 	if !strings.Contains(f.rules[1], "comment 'mesh-host adoption.opening-tcp-5671-incoming ") {
 		t.Errorf("the rule is not marked as the mesh's: %v", f.rules)
 	}
 	action, err = Converge(context.Background(), f.run, o)
-	if err != nil || action != "unchanged" {
+	if err != nil || action.Action != "unchanged" {
 		t.Fatalf("second converge: %q %v", action, err)
 	}
 	if f.added() != 1 {
@@ -299,7 +314,7 @@ func TestAnOpeningLostToAReloadIsAddedAgain(t *testing.T) {
 	}
 	f.rules = nil // what a reload that lost the rule leaves
 	action, err := Converge(context.Background(), f.run, o)
-	if err != nil || action != "created" || len(f.rules) != 1 {
+	if err != nil || action.Action != "created" || len(f.rules) != 1 {
 		t.Fatalf("a lost opening was not put back: %q %v %v", action, err, f.rules)
 	}
 }
@@ -310,7 +325,7 @@ func TestAChangedOpeningReplacesOnlyItsOwnRule(t *testing.T) {
 		t.Fatal(err)
 	}
 	action, err := Converge(context.Background(), f.run, opening("adoption.x", 5671, "mesh", "incoming", 0))
-	if err != nil || action != "updated" {
+	if err != nil || action.Action != "updated" {
 		t.Fatalf("%q %v", action, err)
 	}
 	if len(f.rules) != 3 || f.rules[0] != "allow 22/tcp" || f.rules[1] != "allow 8080/tcp comment 'someone else'" ||
@@ -550,3 +565,119 @@ func TestARefusalOfEveryoneButSomeIsStillAFirewall(t *testing.T) {
 		t.Error("a legacy refusal of all but a range was not counted")
 	}
 }
+
+// Defends novox/hq ADR 0103: an opening a found rule already answers is not added, because ufw
+// takes two rules differing only in their comment for one (testdata/ufw-comment-only.txt).
+
+func TestUfwTakesTheMeshsRuleAndTheOperatorsForOne(t *testing.T) {
+	// The capture: each mesh rule answered "Rule updated" beside the operator's equivalent.
+	raw := captured(t, "ufw-comment-only.txt")
+	if strings.Count(raw, "Rule updated\n") != 3 {
+		t.Fatalf("the capture no longer shows ufw updating an equivalent rule:\n%s", raw)
+	}
+	for _, c := range []struct {
+		operators string
+		o         *declaration.Opening
+	}{
+		{"route allow 8080/tcp", opening("adoption.opening-tcp-8080-forwarded", 20001, "everywhere", "forwarded", 8080)},
+		{"allow 5671/tcp", opening("adoption.opening-tcp-5671-incoming", 5671, "everywhere", "incoming", 0)},
+		{"allow in on mesh0 to any port 5432 proto tcp comment 'operator note'", opening("adoption.opening-tcp-5432-incoming", 5432, "mesh", "incoming", 0)},
+	} {
+		theirs, ok := parseRule(c.operators)
+		mine, ok2 := parseRule(strings.Join(Rule(c.o), " ") + " comment '" + Mark(c.o) + "'")
+		if !ok || !ok2 || !theirs.sameAs(mine) {
+			t.Errorf("%q and the mesh's %v are one rule to ufw, and read as two", c.operators, Rule(c.o))
+		}
+		if !theirs.admits(c.o) {
+			t.Errorf("%q does not read as answering %s", c.operators, c.o.Target())
+		}
+	}
+}
+
+func TestEveryCapturedRuleFormIsRead(t *testing.T) {
+	want := map[string]string{
+		"allow 22/tcp": "tcp 22 in= from=any", "allow 9200": " 9200 in= from=any",
+		"allow from 192.0.2.0/24 to any port 9300 proto tcp": "tcp 9300 in= from=192.0.2.0/24",
+		"allow in on eth0 to any port 9301 proto tcp":        "tcp 9301 in=eth0 from=any",
+		"allow 9500:9510/tcp":                                "tcp 9500:9510 in= from=any",
+		"allow 80,443/tcp":                                   "tcp 80,443 in= from=any",
+		"allow in on mesh0 to any port 5432 proto tcp":       "tcp 5432 in=mesh0 from=any",
+		"route allow 8080/tcp":                               "tcp 8080 in= from=any",
+		"allow 9900/tcp":                                     "tcp 9900 in= from=any",
+	}
+	rules, err := added(context.Background(), func(context.Context, string, ...string) (string, error) {
+		return captured(t, "ufw-forms.txt"), nil
+	})
+	if err != nil || len(rules) != 15 {
+		t.Fatalf("read %d rules: %v", len(rules), err)
+	}
+	for _, rule := range rules {
+		r, ok := parseRule(rule)
+		if !ok {
+			t.Errorf("a rule ufw printed was not read: %q", rule)
+			continue
+		}
+		if w, listed := want[rule]; listed {
+			if got := r.proto + " " + r.port + " in=" + r.in + " from=" + r.from; got != w {
+				t.Errorf("%q read as %q, want %q", rule, got, w)
+			}
+		}
+	}
+}
+
+func TestAnOpeningAFoundRuleAnswersIsNotAddedAndItsRemovalLeavesTheRule(t *testing.T) {
+	for _, c := range []struct {
+		name, operators string
+		o               *declaration.Opening
+	}{
+		{"forwarded, the same rule", "route allow 8080/tcp", opening("adoption.fwd", 20001, "everywhere", "forwarded", 8080)},
+		{"incoming, the same rule", "allow 5671/tcp", opening("adoption.bus", 5671, "everywhere", "incoming", 0)},
+		{"with a comment of its own", "allow in on mesh0 to any port 5432 proto tcp comment 'operator note'", opening("adoption.store", 5432, "mesh", "incoming", 0)},
+		{"broader: from anywhere", "allow 5432/tcp", opening("adoption.store", 5432, "mesh", "incoming", 0)},
+		{"broader: any protocol, a range", "allow 5000:5100", opening("adoption.registry", 5000, "everywhere", "incoming", 0)},
+	} {
+		f := &fakeUFW{installed: true, active: true, rules: []string{"allow 22/tcp", c.operators}}
+		done, err := Converge(context.Background(), f.run, c.o)
+		if err != nil {
+			t.Fatalf("%s: %v", c.name, err)
+		}
+		if done.SatisfiedBy != c.operators || done.Action != "unchanged" || f.added() != 0 {
+			t.Errorf("%s: %+v, asked %v", c.name, done, f.asked)
+		}
+		if n, err := Remove(context.Background(), f.run, c.o.ID); err != nil || n != 0 {
+			t.Errorf("%s: removing the opening removed %d: %v", c.name, n, err)
+		}
+		if len(f.rules) != 2 || f.rules[1] != c.operators {
+			t.Errorf("%s: the operator's rule did not survive: %v", c.name, f.rules)
+		}
+	}
+}
+
+func TestARuleThatDoesNotAnswerTheOpeningLeavesItToBeAdded(t *testing.T) {
+	for _, operators := range []string{
+		"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
+	} {
+		f := &fakeUFW{installed: true, active: true, rules: []string{operators}}
+		done, err := Converge(context.Background(), f.run, opening("adoption.bus", 5671, "everywhere", "incoming", 0))
+		if err != nil || done.Action != "created" || done.SatisfiedBy != "" {
+			t.Errorf("%q: %+v %v", operators, done, err)
+		}
+	}
+}
+
+func TestAnOpeningWhoseFoundRuleIsGoneIsAddedAgain(t *testing.T) {
+	f := &fakeUFW{installed: true, active: true, rules: []string{"allow 5671/tcp"}}
+	o := opening("adoption.bus", 5671, "everywhere", "incoming", 0)
+	if done, err := Converge(context.Background(), f.run, o); err != nil || done.SatisfiedBy == "" {
+		t.Fatalf("%+v %v", done, err)
+	}
+	f.rules = nil // the operator deleted theirs
+	if done, err := Converge(context.Background(), f.run, o); err != nil || done.Action != "created" {
+		t.Fatalf("the opening was not added once nothing answered it: %+v %v", done, err)
+	}
+}