diff --git a/CHANGELOG.md b/CHANGELOG.md index e136dde..7dfcc13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,12 @@ All notable changes to Housecarl are documented here. The format follows ### Fixed +- Changing how a quota counts lost or muddled what it used: per address + to a total started at nothing, per source to per destination kept the + sources' use as if it were the destinations', and other groups left + the old entries behind. Now a total starts at the addresses' sum, + wider groups add up their addresses, and what can't be carried starts + again with a warning (`quota_restarts`) on the change. - `hc stats top devices` listed each device twice (once for what it received, once for what it sent); it is one row with both again. - `hc rule add --before/--after/--first` made two changes (create at the diff --git a/docs/rules.md b/docs/rules.md index 45d8907..15f52a9 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -215,6 +215,20 @@ A per-address quota counts an address: a device that gets a new one starts again. With Steward's reservations IPv4 addresses stay; IPv6 is grouped by /64 (below). +Changing the amount, `over` or the resets keeps what was used. Changing +how the quota counts carries what can be carried: + +| Change | What was used | +|---|---| +| per address to one total | the addresses' sum | +| to wider groups (IPv6 /64 to /56) | added up per new group | +| a total to per address | starts again: a total can't be split | +| per source to per destination (or back) | starts again | +| to finer groups (/64 to /128) | starts again: a group can't be split | + +A change that starts a quota again says so (warning `quota_restarts`), +when it is staged for review and in `hc` (also with `-n`). + **Rules with a quota or a byte rate count every packet.** The built-in that accepts established connections comes before your rules, so a rule there only sees each connection's first packet. Rules that count traffic diff --git a/docs/validation.md b/docs/validation.md index b6d54bb..89bc2f7 100644 --- a/docs/validation.md +++ b/docs/validation.md @@ -97,6 +97,7 @@ These apply to rules, source NAT and redirects. | `rule_group` | error | a grouping prefix outside 1-32 (IPv4) or 1-128 (IPv6) | | `rule_traffic_accept` | error | a rule with a quota or a byte rate that accepts: it is checked before established connections are accepted, so it can only drop, reject or log | | `rule_every_packet_accept` | error | a rule checked against every packet (`every_packet`) that accepts: it comes before established connections are accepted, so it can only drop, reject or log | +| `quota_restarts` | warning | a change makes a rule count its quota so that what it used can't carry over (a total to per address, per source to per destination, finer groups): it starts again. Only on the change that does it | | `flowtable_traffic` | warning | a forward rule checked against every packet (a quota, a byte rate, `every_packet`) while the flowtable is on: it only sees each connection's first packets | | `reject_with` | error | `tcp-reset` on a rule that matches more than TCP, or with an action other than reject | | `log_prefix` | error | a log prefix over 100 characters | diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 01adbcd..514ce1f 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -191,6 +191,7 @@ func (e *Engine) commit(ctx context.Context, p *auth.Principal, description stri // Only report warnings the change brings; old ones are on the status // page. res.Warnings = newProblems(validate.Check(e.state, e.env()), problems) + res.Warnings = append(res.Warnings, quotaRestarts(e.state, next)...) if e.Firewall != nil { if err := e.Firewall.Check(ctx, next); err != nil { return res, &Error{Kind: KindFirewall, Message: "nftables refused the ruleset: " + err.Error()} @@ -230,6 +231,23 @@ func (e *Engine) commit(ctx context.Context, p *auth.Principal, description stri return res, nil } +// quotaRestarts warns of the rules whose quota starts again because they +// now count it in a way what they used can't be carried to. +func quotaRestarts(before, after *model.State) []validate.Problem { + was := map[int64]*model.Rule{} + for _, r := range before.Rules { + was[r.ID] = r + } + var out []validate.Problem + for _, r := range after.Rules { + if carry, why := model.QuotaCarry(was[r.ID], r); carry == model.QuotaRestarts { + out = append(out, validate.Problem{Severity: validate.Warning, Code: "quota_restarts", Object: r.Label(), + Message: "its quota starts again: " + why}) + } + } + return out +} + func newProblems(before, after []validate.Problem) []validate.Problem { seen := map[string]bool{} for _, p := range before { diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index c78f3fc..cf76966 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -173,3 +173,49 @@ func TestPositionsStayStableUntilCrowded(t *testing.T) { t.Errorf("order %v", order) } } + +// TestQuotaRestartsWarns: a change that counts a quota so what it used +// can't carry over warns; one that can (to a total, wider groups) doesn't. +func TestQuotaRestartsWarns(t *testing.T) { + e := newEngine(t) + ctx := context.Background() + var id int64 + if _, err := e.Apply(ctx, admin, "", Options{}, func(st *model.State, newID func() int64) error { + id = newID() + st.Rules = append(st.Rules, &model.Rule{ID: id, Chain: "forward", Action: "drop", Quota: &model.Quota{Bytes: 1 << 30, Over: true, PerSource: true}}) + return nil + }); err != nil { + t.Fatal(err) + } + change := func(f func(r *model.Rule)) []string { + res, err := e.Apply(ctx, admin, "", Options{DryRun: true}, func(st *model.State, _ func() int64) error { + f(st.ByID(model.KindRule, id).(*model.Rule)) + return nil + }) + if err != nil { + t.Fatal(err) + } + var codes []string + for _, w := range res.Warnings { + if w.Code == "quota_restarts" { + codes = append(codes, w.Message) + } + } + return codes + } + if w := change(func(r *model.Rule) { r.Quota.PerSource, r.Quota.PerDestination = false, true }); len(w) != 1 || !strings.Contains(w[0], "per source can't be counted per destination") { + t.Errorf("per source to per destination: %v", w) + } + if w := change(func(r *model.Rule) { r.Quota.PerSource = false }); len(w) != 0 { + t.Errorf("to a total (summed): %v", w) + } + if w := change(func(r *model.Rule) { r.GroupV6 = 56 }); len(w) != 0 { + t.Errorf("wider groups: %v", w) + } + if w := change(func(r *model.Rule) { r.GroupV6 = 128 }); len(w) != 1 { + t.Errorf("finer groups: %v", w) + } + if w := change(func(r *model.Rule) { r.Quota.Bytes = 2 << 30 }); len(w) != 0 { + t.Errorf("another amount: %v", w) + } +} diff --git a/internal/model/objects.go b/internal/model/objects.go index 5df8ad5..80b27e1 100644 --- a/internal/model/objects.go +++ b/internal/model/objects.go @@ -274,6 +274,45 @@ func (r *Rule) Groups() (v4, v6 int) { return v4, v6 } +// What becomes of what a rule's quota used when the rule changes +// (QuotaCarry). +const ( + QuotaKept = "" // counted as before: carried as it is + QuotaSummed = "summed" // per address to a total: the addresses' sum + QuotaRegrouped = "regrouped" // per address in wider groups: added up per group + QuotaRestarts = "restarts" // can't be carried: starts again +) + +// QuotaCarry says what becomes of what old's quota used when the rule +// becomes new (both the same rule, with quotas), and for QuotaRestarts +// why. The amount, over and the resets don't matter: only how it counts. +func QuotaCarry(old, new *Rule) (carry, why string) { + if old == nil || new == nil || old.Quota == nil || new.Quota == nil { + return QuotaKept, "" + } + was, is := old.Quota.PerAddress(), new.Quota.PerAddress() + words := map[string]string{"saddr": "per source", "daddr": "per destination", "": "in total"} + switch { + case was == "" && is == "": + return QuotaKept, "" + case is == "": + return QuotaSummed, "" + case was == "": + return QuotaRestarts, "a total can't be split by address, so each address starts at nothing" + case was != is: + return QuotaRestarts, "what was counted " + words[was] + " can't be counted " + words[is] + } + o4, o6 := old.Groups() + n4, n6 := new.Groups() + switch { + case o4 == n4 && o6 == n6: + return QuotaKept, "" + case n4 > o4 || n6 > o6: + return QuotaRestarts, "addresses are grouped more finely now, and a group's use can't be split" + } + return QuotaRegrouped, "" +} + // PortForward publishes a service on an inside host: incoming connections // to a public address and port are sent on (DNAT) to To[:ToPort], let // through the forward chain, and, with Hairpin, also work from inside. diff --git a/internal/model/quota_test.go b/internal/model/quota_test.go new file mode 100644 index 0000000..22c1010 --- /dev/null +++ b/internal/model/quota_test.go @@ -0,0 +1,26 @@ +package model + +import "testing" + +func TestQuotaCarry(t *testing.T) { + rule := func(per string, v4, v6 int) *Rule { + return &Rule{GroupV4: v4, GroupV6: v6, Quota: &Quota{Bytes: 1, PerSource: per == "src", PerDestination: per == "dst"}} + } + for _, c := range []struct { + old, new *Rule + want string + }{ + {rule("", 0, 0), rule("", 0, 0), QuotaKept}, + {rule("src", 0, 0), rule("src", 32, 64), QuotaKept}, // the defaults, written out + {rule("src", 0, 0), rule("", 0, 0), QuotaSummed}, + {rule("", 0, 0), rule("src", 0, 0), QuotaRestarts}, + {rule("src", 0, 0), rule("dst", 0, 0), QuotaRestarts}, + {rule("src", 0, 0), rule("src", 24, 56), QuotaRegrouped}, + {rule("src", 0, 0), rule("src", 24, 128), QuotaRestarts}, + {nil, rule("src", 0, 0), QuotaKept}, + } { + if got, _ := QuotaCarry(c.old, c.new); got != c.want { + t.Errorf("%+v -> %+v: %q, want %q", c.old, c.new, got, c.want) + } + } +} diff --git a/internal/nft/nft.go b/internal/nft/nft.go index de49101..35d9522 100644 --- a/internal/nft/nft.go +++ b/internal/nft/nft.go @@ -15,6 +15,7 @@ import ( "errors" "fmt" "net" + "net/netip" "os" "os/exec" "path/filepath" @@ -424,6 +425,9 @@ type Firewall struct { restore *QuotaUsage // adjust are quota changes the next apply makes (AdjustQuota). adjust []QuotaAdjust + // quotaRules are the rules with a quota in the ruleset in force, by + // ID: what their usage was counted by (nil before the first apply). + quotaRules map[int64]*model.Rule // flowDevs are the flowtable's devices in the ruleset in force, by // name, with their interface index then (0: left out, missing). flowDevs map[string]int @@ -510,6 +514,73 @@ func (u *QuotaUsage) apply(adj []QuotaAdjust) { } } +// carry keeps what quotas used when their rules now count differently +// (model.QuotaCarry): per address to a total, the sum; to wider groups, +// the groups' sums; what can't be carried starts again. was are the +// rules in force before (nil: nothing to compare with). +func (u *QuotaUsage) carry(was map[int64]*model.Rule, st *model.State) { + for _, r := range st.Rules { + old := was[r.ID] + carry, _ := model.QuotaCarry(old, r) + if carry == model.QuotaKept { + continue + } + name := render.QuotaName(r.ID) + sets := []string{name + "_v4", name + "_v6"} + switch carry { + case model.QuotaSummed: + var sum int64 + for _, set := range sets { + for _, e := range u.QuotaSets[set] { + sum += e.Used + } + } + if u.Quotas == nil { + u.Quotas = map[string]int64{} + } + u.Quotas[name] = sum + case model.QuotaRegrouped: + v4, v6 := r.Groups() + for _, set := range sets { + byKey := map[string]int64{} + var order []string + for _, e := range u.QuotaSets[set] { + a, err := netip.ParseAddr(e.Addr) + if err != nil { + continue + } + bits := v4 + if a.Is6() { + bits = v6 + } + p, _ := a.Prefix(bits) + k := p.Addr().String() + if _, ok := byKey[k]; !ok { + order = append(order, k) + } + byKey[k] += e.Used + } + if len(order) == 0 { + continue + } + var es []render.Element + for _, k := range order { + es = append(es, render.Element{Addr: k, Used: byKey[k]}) + } + u.QuotaSets[set] = es + } + continue + } + // What was counted the other way goes: a total, or the entries. + if carry != model.QuotaSummed { + delete(u.Quotas, name) + } + for _, set := range sets { + delete(u.QuotaSets, set) + } + } +} + // ResetQuota starts a rule's quota again: the named quota, or the // per-address quota sets (both families). func (f *Firewall) ResetQuota(ctx context.Context, ruleID int64, perAddress bool) error { @@ -579,6 +650,7 @@ func (f *Firewall) Apply(ctx context.Context, st *model.State) error { usage = old.QuotaUsage } usage.merge(f.restore) + usage.carry(f.quotaRules, st) usage.apply(f.adjust) f.adjust = nil opt.Quotas, opt.QuotaSets = usage.Quotas, usage.QuotaSets @@ -589,6 +661,14 @@ func (f *Firewall) Apply(ctx context.Context, st *model.State) error { return err } f.restore = nil + f.quotaRules = map[int64]*model.Rule{} + for _, r := range st.Rules { + if r.Quota != nil { + c, q := *r, *r.Quota // only the quota and the grouping are read + c.Quota = &q + f.quotaRules[r.ID] = &c + } + } f.flowDevs = map[string]int{} for _, d := range st.Settings.FlowtableDevices { if opt.Interfaces == nil || opt.Interfaces[d] { diff --git a/internal/nft/nft_test.go b/internal/nft/nft_test.go index 9ee965c..45ba76d 100644 --- a/internal/nft/nft_test.go +++ b/internal/nft/nft_test.go @@ -197,6 +197,21 @@ func TestQuotasInContainer(t *testing.T) { if t2, d2 := used(); t2 > total-1000 || t2 < total-2000 || d2 != 0 { t.Errorf("after giving back: total %d (was %d, gave 1000), per destination %d (was %d, reset)", t2, total, d2, dev) } + // Counted in total now: what the addresses used carries over. + ping("127.0.0.3") + _, dev = used() + st.Rules[1].Quota.PerDestination = false + if err := fw.Apply(ctx, st); err != nil { + t.Fatal(err) + } + tb, _ := fw.Table(ctx) + if tb.Quotas["quota_r2"] < dev || dev == 0 { + t.Errorf("per destination to a total: %d, was %d", tb.Quotas["quota_r2"], dev) + } + st.Rules[1].Quota.PerDestination = true + if err := fw.Apply(ctx, st); err != nil { + t.Fatal(err) + } // Reset by hand. if err := fw.ResetQuota(ctx, 1, false); err != nil { t.Fatal(err) @@ -211,7 +226,7 @@ func TestQuotasInContainer(t *testing.T) { // saved usage comes back. ping("127.0.0.2") ping("127.0.0.3") - tb, _ := fw.Table(ctx) + tb, _ = fw.Table(ctx) saved := tb.QuotaUsage exec.Command("docker", "exec", name, "nft", "delete", "table", "inet", "housecarl").Run() fw.RestoreQuotas(&saved) @@ -240,6 +255,45 @@ func TestQuotaAdjust(t *testing.T) { } } +// TestQuotaCarry: usage follows a rule counting its quota differently. +func TestQuotaCarry(t *testing.T) { + perSource := &model.Rule{ID: 2, Quota: &model.Quota{Bytes: 1 << 30, PerSource: true}} + usage := func() QuotaUsage { + return QuotaUsage{Quotas: map[string]int64{}, QuotaSets: map[string][]render.Element{ + "quota_r2_v4": {{Addr: "10.0.0.1", Used: 100}, {Addr: "10.0.0.2", Used: 50}}, + "quota_r2_v6": {{Addr: "2001:db8:0:1::", Used: 7}, {Addr: "2001:db8:0:2::", Used: 3}}}} + } + was := map[int64]*model.Rule{2: perSource} + with := func(f func(r *model.Rule)) *model.State { + r := *perSource + q := *perSource.Quota + r.Quota = &q + f(&r) + return &model.State{Rules: []*model.Rule{&r}} + } + u := usage() + u.carry(was, with(func(r *model.Rule) { r.Quota.PerSource = false })) + if u.Quotas["quota_r2"] != 160 || len(u.QuotaSets["quota_r2_v4"]) != 0 { + t.Errorf("to a total: %+v", u) + } + u = usage() + u.carry(was, with(func(r *model.Rule) { r.GroupV4, r.GroupV6 = 24, 48 })) + want := map[string][]render.Element{"quota_r2_v4": {{Addr: "10.0.0.0", Used: 150}}, "quota_r2_v6": {{Addr: "2001:db8::", Used: 10}}} + if !reflect.DeepEqual(u.QuotaSets, want) { + t.Errorf("wider groups: %+v", u.QuotaSets) + } + u = usage() + u.carry(was, with(func(r *model.Rule) { r.Quota.PerSource, r.Quota.PerDestination = false, true })) + if len(u.QuotaSets) != 0 { + t.Errorf("per destination now: %+v", u.QuotaSets) + } + u = usage() + u.carry(nil, with(func(r *model.Rule) { r.Quota.PerSource = false })) + if len(u.QuotaSets["quota_r2_v4"]) != 2 || u.Quotas["quota_r2"] != 0 { + t.Errorf("nothing to compare with (the first apply): %+v", u) + } +} + // TestDeviceCountsInContainer: the per-device counters are read back and // survive a reload. func TestDeviceCountsInContainer(t *testing.T) { diff --git a/scripts/e2e.sh b/scripts/e2e.sh index 74dcbe7..db86699 100755 --- a/scripts/e2e.sh +++ b/scripts/e2e.sh @@ -342,6 +342,15 @@ run "the device is back" 0 send 1000 run "the reset is audited" 0 H audit --action quota. contains "quota.reset" contains "quota.give" +# Counted in total now: what the device used carries over; per +# destination can't take it, and says so. +run "within the quota" 0 send 300000 +run "to one total" 0 H rule update "$QR" --quota 1mbytes,over +not_contains "quota_restarts" +run "the total carried over" 0 H quotas +contains "e2e quota) *[0-9.]* KB" +run "per destination starts again" 0 H rule update "$QR" --quota 1mbytes,over,per-destination +contains "quota_restarts" run "the quota rule goes" 0 H rule delete -y "$QR" # ---- the zone matrix, of the imported ruleset