diff --git a/internal/apply/opening.go b/internal/apply/opening.go index ff6ee1c..95012e9 100644 --- a/internal/apply/opening.go +++ b/internal/apply/opening.go @@ -85,12 +85,18 @@ func applyOpening(ctx context.Context, o *declaration.Opening, run Runner, kind out.Detail = "no firewall found; nothing filters this port" return out, nil case firewall.UFW: - action, err := firewall.Converge(ctx, run, o) + done, err := firewall.Converge(ctx, run, o) if err != nil { return out, err } - out.Action = action + out.Action = done.Action out.Detail = "through ufw, marked " + firewall.Mark(o) + if done.SatisfiedBy != "" { + // ufw would take a rule differing only in its comment for the same one, so the + // mesh's is not added beside it (novox/hq ADR 0103). + out.Detail = "satisfied by a rule found in ufw (" + done.SatisfiedBy + + "); the mesh added nothing and will remove nothing" + } return out, nil } return out, fmt.Errorf("no firewall is known for this node, so %s cannot be opened", o.Target()) diff --git a/internal/apply/opening_test.go b/internal/apply/opening_test.go index f4dc76d..c02aa79 100644 --- a/internal/apply/opening_test.go +++ b/internal/apply/opening_test.go @@ -287,3 +287,20 @@ func TestAFlipThatFailsKeepsTheGuardAndTheOpenings(t *testing.T) { t.Error("the guard is still recorded after the completed flip") } } + +func TestAnOpeningAFoundRuleAnswersIsReportedSatisfied(t *testing.T) { + // novox/hq ADR 0103: the mesh adds nothing beside a rule ufw would take for the same one. + dir := t.TempDir() + u := &ufwMachine{installed: true, active: true, rules: []string{"allow 22/tcp", "allow 5671/tcp"}} + report, _, err := applyWith(t, adopted(t, `{"taken":[]}`, busOpening+","+withConf(dir)), store.State{}, u.run) + if err != nil { + t.Fatal(err) + } + o := outcomeOf(report, "adoption.opening-tcp-5671-incoming") + if o.Action != "unchanged" || !strings.Contains(o.Detail, "satisfied by a rule found in ufw (allow 5671/tcp)") { + t.Errorf("the opening was not reported satisfied: %+v", o) + } + if len(u.rules) != 2 || u.index("ufw allow") >= 0 { + t.Errorf("a rule was added beside the found one: %v", u.rules) + } +} diff --git a/internal/firewall/firewall.go b/internal/firewall/firewall.go index d2630c3..fdc52db 100644 --- a/internal/firewall/firewall.go +++ b/internal/firewall/firewall.go @@ -119,8 +119,8 @@ func Refusing(ruleset string, ufwActive bool) []string { type rule struct{ table, chain, line string } type chainOf struct { base, dropping, accepts bool - policyLine string - jumpedFrom []string + policyLine string + jumpedFrom []string } chains := map[string]*chainOf{} // by "table\x00chain" tableAccepts := map[string]bool{} @@ -469,17 +469,195 @@ func words(rule string) []string { return out } +// A ufw rule, as `ufw show added` prints it or as it is given, reduced to what ufw compares. +// +// **ufw treats two rules that differ only in their comment as one rule.** Measured on a lab +// machine (testdata/ufw-comment-only.txt): adding `route allow proto tcp to any port 8080 comment +// 'mesh-host …'` beside an operator's `route allow 8080/tcp` answers "Rule updated", and the +// operator's rule now carries the mesh's mark — so removing the opening later would delete the +// operator's rule. The same holds for an incoming rule and for one with a comment of its own. +type ufwRule struct { + route bool + action, in, out string + from, fromPort, to string + port, proto, app string + comment string +} + +// parseRule reads a rule in either of ufw's forms — the short `allow 22/tcp` and the long `allow +// 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)} + i := 0 + if i < len(w) && w[i] == "route" { + r.route = true + i++ + } + if i >= len(w) { + return r, false + } + switch w[i] { + case "allow", "deny", "reject", "limit": + r.action = w[i] + default: + return r, false + } + i++ + for i < len(w) && (w[i] == "in" || w[i] == "out") { + dir := w[i] + i++ + iface := "" + if i+1 < len(w) && w[i] == "on" { + iface = w[i+1] + i += 2 + } + if dir == "in" { + r.in = iface + } else { + r.out = iface + } + } + if i < len(w) && w[i] != "from" && w[i] != "to" && w[i] != "proto" && w[i] != "comment" && + w[i] != "app" && w[i] != "log" && w[i] != "log-all" { + // The short form: a port with its protocol, a bare port, or an application's name. + port, proto, hasProto := strings.Cut(w[i], "/") + if isPorts(port) { + r.port = port + if hasProto { + r.proto = proto + } + } else { + r.app = w[i] + } + i++ + } + for ; i < len(w); i++ { + next := func() string { + if i+1 < len(w) { + i++ + return w[i] + } + return "" + } + switch w[i] { + case "from": + r.from = next() + if i+1 < len(w) && w[i+1] == "port" { + i++ + r.fromPort = next() + } + case "to": + r.to = next() + if i+1 < len(w) && w[i+1] == "port" { + i++ + r.port = next() + } + case "port": + r.port = next() + case "proto": + r.proto = next() + case "app": + r.app = next() + case "comment": + next() + case "log", "log-all": + default: + return r, false + } + } + if r.proto == "any" { + r.proto = "" + } + return r, true +} + +func isPorts(s string) bool { + if s == "" { + return false + } + for _, c := range s { + if (c < '0' || c > '9') && c != ':' && c != ',' { + return false + } + } + return true +} + +// sameAs is whether ufw would take two rules for one — everything but the comment equal. +func (r ufwRule) sameAs(o ufwRule) bool { + r.comment, o.comment = "", "" + return r == o +} + +// admits is whether a rule already lets through what an opening says: the same path, allowed from +// any source to any address of this machine, on the opening's port and protocol — or on any +// protocol — and on any interface, or the private network's for an opening from it. +func (r ufwRule) admits(o *declaration.Opening) bool { + want, ok := parseRule(strings.Join(Rule(o), " ")) + if !ok || r.route != want.route || r.action != "allow" || r.out != "" || r.app != "" || + r.from != "any" || r.fromPort != "" || r.to != "any" { + return false + } + if r.proto != "" && r.proto != want.proto { + return false + } + if r.in != "" && r.in != want.in { + return false + } + return portsInclude(r.port, want.port) +} + +// portsInclude is whether a ufw port list — 80, 80,443, or 8000:8100 — names a port. +func portsInclude(list, port string) bool { + p, err := strconv.Atoi(port) + if err != nil { + return false + } + for _, part := range strings.Split(list, ",") { + lo, hi, isRange := strings.Cut(part, ":") + a, err := strconv.Atoi(lo) + if err != nil { + continue + } + b := a + if isRange { + if b, err = strconv.Atoi(hi); err != nil { + continue + } + } + if a <= p && p <= b { + return true + } + } + return false +} + +// Converged is what converging an opening did. SatisfiedBy names the rule already there that +// answers the opening, when one does; the mesh then adds nothing, and so will remove nothing. +type Converged struct { + Action string + SatisfiedBy string +} + // Converge makes one opening true in ufw: its marked rule present, and any rule marked for it that // no longer describes it deleted. Nothing unmarked is touched. The outcome is created, updated or // unchanged, read back from ufw rather than assumed. -func Converge(ctx context.Context, run Runner, o *declaration.Opening) (string, error) { +// +// **An opening a rule already answers is not added** (novox/hq ADR 0103). If ufw holds a rule not +// marked for this opening that already admits what it says — the operator's, or one the mesh +// added for another opening — the opening is satisfied by it: adding the mesh's would take that +// rule over if it differs only in its comment, and removing the opening would then delete it. +func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converged, error) { rules, err := added(ctx, run) if err != nil { - return "", err + return Converged{}, err } mark := Mark(o) present := false var stale []string + satisfiedBy := "" for _, rule := range rules { c := comment(rule) switch { @@ -487,25 +665,45 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (string, present = true case markedFor(c, o.ID): stale = append(stale, rule) + default: + if parsed, ok := parseRule(rule); ok && satisfiedBy == "" && parsed.admits(o) { + satisfiedBy = rule + } } } if present && len(stale) == 0 { - return "unchanged", nil + return Converged{Action: "unchanged"}, nil } for _, rule := range stale { if _, err := run(ctx, "ufw", deletion(rule)...); err != nil { - return "", fmt.Errorf("deleting the mesh's stale ufw rule %q: %w", rule, err) + return Converged{}, fmt.Errorf("deleting the mesh's stale ufw rule %q: %w", rule, err) } } + if !present && satisfiedBy != "" { + after, err := added(ctx, run) + if err != nil { + return Converged{}, err + } + for _, rule := range after { + if markedFor(comment(rule), o.ID) { + return Converged{}, fmt.Errorf("ufw still lists a stale rule marked for %s after deleting it", o.ID) + } + } + action := "unchanged" + if len(stale) > 0 { + action = "updated" + } + return Converged{Action: action, SatisfiedBy: satisfiedBy}, nil + } if !present { args := append(Rule(o), "comment", mark) if _, err := run(ctx, "ufw", args...); err != nil { - return "", fmt.Errorf("adding the ufw rule for %s: %w", o.Target(), err) + return Converged{}, fmt.Errorf("adding the ufw rule for %s: %w", o.Target(), err) } } after, err := added(ctx, run) if err != nil { - return "", err + return Converged{}, err } found, leftover := false, 0 for _, rule := range after { @@ -517,15 +715,15 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (string, } } if !found { - return "", fmt.Errorf("ufw was asked for %s and does not list it afterwards", o.Target()) + return Converged{}, fmt.Errorf("ufw was asked for %s and does not list it afterwards", o.Target()) } if leftover > 0 { - return "", fmt.Errorf("ufw still lists %d stale rule(s) marked for %s after deleting them", leftover, o.ID) + return Converged{}, fmt.Errorf("ufw still lists %d stale rule(s) marked for %s after deleting them", leftover, o.ID) } if len(stale) > 0 { - return "updated", nil + return Converged{Action: "updated"}, nil } - return "created", nil + return Converged{Action: "created"}, nil } // Remove deletes the rules marked for one opening, and nothing else. diff --git a/internal/firewall/firewall_test.go b/internal/firewall/firewall_test.go index 878fc71..33e88ad 100644 --- a/internal/firewall/firewall_test.go +++ b/internal/firewall/firewall_test.go @@ -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) + } +} diff --git a/internal/firewall/testdata/ufw-comment-only.txt b/internal/firewall/testdata/ufw-comment-only.txt new file mode 100644 index 0000000..13f758c --- /dev/null +++ b/internal/firewall/testdata/ufw-comment-only.txt @@ -0,0 +1,37 @@ +$ ufw allow 22/tcp +Rules updated +Rules updated (v6) +$ ufw --force enable +Firewall is active and enabled on system startup +$ ufw route allow 8080/tcp +Rule added +Rule added (v6) +$ ufw route allow proto tcp to any port 8080 comment 'mesh-host adoption.opening-tcp-8080-forwarded 1a2b3c4d' +Rule updated +Rule updated (v6) +$ ufw allow 5671/tcp +Rule added +Rule added (v6) +$ ufw allow proto tcp to any port 5671 comment 'mesh-host adoption.opening-tcp-5671-incoming 1a2b3c4d' +Rule updated +Rule updated (v6) +$ ufw allow in on mesh0 to any port 5432 proto tcp comment 'operator note' +Rule added +Rule added (v6) +$ ufw allow in on mesh0 proto tcp to any port 5432 comment 'mesh-host adoption.opening-tcp-5432-incoming 1a2b3c4d' +Rule updated +Rule updated (v6) +$ ufw show added +Added user rules (see 'ufw status' for running firewall): +ufw allow 22/tcp +ufw route allow 8080/tcp comment 'mesh-host adoption.opening-tcp-8080-forwarded 1a2b3c4d' +ufw allow 5671/tcp comment 'mesh-host adoption.opening-tcp-5671-incoming 1a2b3c4d' +ufw allow in on mesh0 to any port 5432 proto tcp comment 'mesh-host adoption.opening-tcp-5432-incoming 1a2b3c4d' +$ ufw route delete allow 8080/tcp comment 'mesh-host adoption.opening-tcp-8080-forwarded 1a2b3c4d' +Rule deleted +Rule deleted (v6) +$ ufw show added +Added user rules (see 'ufw status' for running firewall): +ufw allow 22/tcp +ufw allow 5671/tcp comment 'mesh-host adoption.opening-tcp-5671-incoming 1a2b3c4d' +ufw allow in on mesh0 to any port 5432 proto tcp comment 'mesh-host adoption.opening-tcp-5432-incoming 1a2b3c4d' diff --git a/internal/firewall/testdata/ufw-forms.txt b/internal/firewall/testdata/ufw-forms.txt new file mode 100644 index 0000000..a39c9c0 --- /dev/null +++ b/internal/firewall/testdata/ufw-forms.txt @@ -0,0 +1,16 @@ +Added user rules (see 'ufw status' for running firewall): +ufw allow 22/tcp +ufw allow 9200 +ufw allow from 192.0.2.0/24 to any port 9300 proto tcp +ufw allow in on eth0 to any port 9301 proto tcp +ufw deny 9400/tcp +ufw limit 2222/tcp +ufw allow 9500:9510/tcp +ufw route allow in on mesh0 out on docker0 to any port 8082 proto tcp +ufw allow to 192.0.2.1 port 9600 proto tcp +ufw allow 9700/udp +ufw route allow in on mesh0 to any port 8083 proto tcp +ufw allow 9900/tcp +ufw allow 80,443/tcp +ufw allow in on mesh0 to any port 5432 proto tcp +ufw route allow 8080/tcp