diff --git a/CHANGELOG.md b/CHANGELOG.md index f67d290..dffff2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,18 @@ All notable changes to Housecarl are documented here. The format follows ### Changed +- The ruleset's diff (Changes, `hc changeset diff`, `--dry-run`) names + the chain each part is in on its `@@` line (`@@ -40,6 +40,8 @@ chain + forward`), as `git diff` names the function, so a rule added deep in a + long chain says whether it is in input, forward or output. +- **History shows the ruleset's diff.** An entry's page (and `hc history + diff ID`, `GET /history/{id}/diff`) shows what the change did to the + ruleset, for entries made before this version too: the configuration + before and after it is got back from the current one and rendered by + this version. +- History lists a changed setting by its own fields + (`builtins.drop_invalid: false → true`), on the web page and in `hc + history show`, instead of the whole `builtins` before and after. - Set names starting with `ban_` are now reserved (`set_name_reserved`): ban lists' sets use them. - **Rules read as a direction.** A match's zones are written `lan → wan`, diff --git a/cmd/hc/admin.go b/cmd/hc/admin.go index 6e537d7..3ab9f61 100644 --- a/cmd/hc/admin.go +++ b/cmd/hc/admin.go @@ -66,6 +66,30 @@ func historyCmd() *cobra.Command { }) }, }) + cmd.AddCommand(&cobra.Command{ + Use: "diff ID", Short: "Show what a change did to the ruleset", Args: cobra.ExactArgs(1), + Long: `Shows the ruleset's diff for a change: the configuration before and after +it, rendered by this version of Housecarl.`, + RunE: func(cmd *cobra.Command, args []string) error { + c, err := newClient() + if err != nil { + return err + } + var d struct { + Diff string `json:"diff"` + } + if err := c.Get(ctx(), "/history/"+args[0]+"/diff", nil, &d); err != nil { + return err + } + return output(d, func(w io.Writer) { + if d.Diff == "" { + fmt.Fprintln(w, "the ruleset didn't change") + return + } + printDiff(w, d.Diff) + }) + }, + }) cmd.AddCommand(&cobra.Command{ Use: "undo ID", Short: "Undo a change", @@ -88,11 +112,13 @@ change touched has changed since; undo the later changes first.`, } // diffJSON lists the fields that differ between two versions of an -// object: "name: office -> lab". +// object: "name: office -> lab". An object within (settings' builtins) +// is compared field by field: "builtins.drop_invalid: false -> true". func diffJSON(before, after json.RawMessage) []string { var a, b map[string]any json.Unmarshal(before, &a) json.Unmarshal(after, &b) + a, b = flatten(a, "", map[string]any{}), flatten(b, "", map[string]any{}) keys := map[string]bool{} for k := range a { keys[k] = true @@ -132,6 +158,19 @@ func diffJSON(before, after json.RawMessage) []string { return out } +// flatten lifts the fields of objects within m to dotted keys; lists +// stay whole. +func flatten(m map[string]any, prefix string, out map[string]any) map[string]any { + for k, v := range m { + if o, ok := v.(map[string]any); ok && len(o) > 0 { + flatten(o, prefix+k+".", out) + } else { + out[prefix+k] = v + } + } + return out +} + func whoamiCmd() *cobra.Command { return &cobra.Command{ Use: "whoami", Short: "Show who you are to the server", Args: cobra.NoArgs, diff --git a/docs/api.md b/docs/api.md index b77e5b3..5d35f65 100644 --- a/docs/api.md +++ b/docs/api.md @@ -57,7 +57,9 @@ Calls that change the configuration answer: one). Missing when a person must confirm (`manual_confirm`). * `?dry_run=1` checks the change, with nftables too, and returns what it would do, with `"applied": false, "dry_run": true`, and `diff`: the - ruleset's unified diff. + ruleset's unified diff, each `@@` line followed by the chain (or other + block of the table) the hunk starts in, as in `@@ -40,6 +40,8 @@ chain + forward`. * `?changeset=ID` (or the header `X-Housecarl-Changeset: ID`) stages the change in that changeset instead ([Changesets](#changesets)): the answer has `"staged": true`, `changes` (this operation's) and `changeset`. @@ -237,6 +239,7 @@ list's `never`), `422 list_full`, `409` for a disabled list. |---|---| | `GET /history[?limit=&before=]` | changes, newest first | | `GET /history/{id}` | one, with each object before and after | +| `GET /history/{id}/diff` | `{"diff"}`: what it did to the ruleset, as a unified diff with each hunk's chain (the configuration before and after it, got back from the current one, rendered by this version) | | `POST /history/{id}/revert` | undo it; 409 if an object it touched has changed since, or it was undone already | | `POST /import/nft` | `{"json": NFT_JSON, "text": "nft -a list ruleset output", "defines": "ruleset file", "table": "inet main", "zones": {"enp2s0": "lan"}, "replace": false}` ([import.md](import.md)); answers with `state` (the imported configuration), `notes` and `raw` too | | `POST /takeover` | `{"tables": ["inet main"]}`: delete these tables (administrators); on probation over the network; 409 in shadow mode | diff --git a/docs/cli.md b/docs/cli.md index 208c4f1..ae00c06 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -60,7 +60,8 @@ applied and saved (history 12; undo with 'hc history undo 12') ``` `-n` shows what would change without changing it, with the diff of the -ruleset; nftables checks it too. +ruleset (each `@@` line names the chain the lines below it are in); +nftables checks it too. ## Changesets @@ -231,7 +232,8 @@ See [bans.md](bans.md). | `confirm` | keep the changes on probation; the ruleset is saved for the boot | | `rollback` | undo the changes on probation now | | `history list [--limit N]` | changes, newest first | -| `history show ID` | each object before and after | +| `history show ID` | each object before and after (a changed setting by its own fields: `builtins.drop_invalid: false -> true`) | +| `history diff ID` | what the change did to the ruleset | | `history undo ID` | apply the change backwards (refused if something it touched has changed since) | | `import [FILE\|-] [--text FILE] [--defines FILE] [--table T] [--zone IFACE=NAME]... [--replace]` | [import.md](import.md) | | `takeover TABLE... [-y]` | delete tables Housecarl's ruleset replaces (administrators); not in shadow mode | diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 73d33ca..45459dd 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -322,6 +322,22 @@ func Undo(st *model.State, h *model.HistoryEntry) error { return nil } +// Rewind applies a history entry backwards to st without Undo's check: +// stepping back through the entries newest first brings back the +// configuration as it was before each. +func Rewind(st *model.State, h *model.HistoryEntry) error { + for i := len(h.Changes) - 1; i >= 0; i-- { + c := h.Changes[i] + if c.Kind == model.KindSettings && c.Before == nil { + continue + } + if err := put(st, c.Kind, c.ID, c.Before); err != nil { + return err + } + } + return nil +} + // Replace makes the configuration next as a whole (imports). func (e *Engine) Replace(ctx context.Context, p *auth.Principal, description string, opts Options, next *model.State) (*Result, error) { e.mu.Lock() diff --git a/internal/server/diff_test.go b/internal/server/diff_test.go new file mode 100644 index 0000000..ddc536e --- /dev/null +++ b/internal/server/diff_test.go @@ -0,0 +1,72 @@ +package server + +import ( + "strings" + "testing" + + udiff "github.com/aymanbagabas/go-udiff" + + "housecarl/internal/api" + "housecarl/internal/model" +) + +// TestBlockHeaders: each hunk says which chain it is in, even when the +// chain's own line is far above it. +func TestBlockHeaders(t *testing.T) { + var b strings.Builder + b.WriteString("table inet housecarl {\n\tset lan_v4 { type ipv4_addr; }\n\tchain input {\n") + for i := 0; i < 20; i++ { + b.WriteString("\t\tcounter comment \"in\"\n") + } + b.WriteString("\t}\n\tchain forward {\n") + for i := 0; i < 20; i++ { + b.WriteString("\t\tcounter comment \"fw\"\n") + } + b.WriteString("\t}\n}\n") + now := b.String() + then := strings.Replace(now, "\t\tcounter comment \"in\"\n\t}", "\t\tcounter comment \"in\"\n\t\taccept comment \"new in\"\n\t}", 1) + then = strings.Replace(then, "\t\tcounter comment \"fw\"\n\t}", "\t\taccept comment \"new fw\"\n\t}", 1) + + diff := blockHeaders(udiff.Unified("a", "b", now, then), now, then) + var heads []string + for _, l := range strings.Split(diff, "\n") { + if strings.HasPrefix(l, "@@") { + heads = append(heads, l) + } + } + if len(heads) != 2 || !strings.HasSuffix(heads[0], "@@ chain input") || !strings.HasSuffix(heads[1], "@@ chain forward") { + t.Fatalf("hunk headers %q, want chain input then chain forward\n%s", heads, diff) + } + if blockHeaders("", now, then) != "" { + t.Fatal("no diff should stay empty") + } +} + +// TestHistoryDiff: an entry's diff is its own change, rebuilt from the +// current configuration however many entries came after it. +func TestHistoryDiff(t *testing.T) { + e := newEnv(t) + var mr api.MutationResponse + e.call(e.token, "POST", "/zones", model.Zone{Name: "lan", Interfaces: []string{"eth1"}}, nil) + e.call(e.token, "POST", "/rules", model.Rule{Chain: "input", Match: model.Match{Services: []string{"ssh"}}, Comment: "first"}, &mr) + first := mr.HistoryID + e.call(e.token, "POST", "/rules", model.Rule{Chain: "forward", Match: model.Match{Services: []string{"http"}}, Comment: "second"}, nil) + e.call(e.token, "PATCH", "/settings", map[string]any{"builtins": map[string]any{"drop_invalid": true}}, nil) + + var out struct{ Diff string } + if code := e.call(e.token, "GET", "/history/"+itoa(first)+"/diff", nil, &out); code != 200 { + t.Fatalf("diff: %d", code) + } + var added []string + for _, l := range strings.Split(out.Diff, "\n") { + if strings.HasPrefix(l, "+") && !strings.HasPrefix(l, "+++") { + added = append(added, l) + } + } + if len(added) == 0 || !strings.Contains(strings.Join(added, "\n"), ` first"`) || strings.Contains(out.Diff, ` second"`) || !strings.Contains(out.Diff, "@@ chain input") { + t.Errorf("entry %d's diff:\n%s", first, out.Diff) + } + if code := e.call(e.token, "GET", "/history/9999/diff", nil, nil); code != 404 { + t.Errorf("unknown entry: %d", code) + } +} diff --git a/internal/server/h_config.go b/internal/server/h_config.go index 7378725..dc05939 100644 --- a/internal/server/h_config.go +++ b/internal/server/h_config.go @@ -101,7 +101,68 @@ func (s *Server) rulesetDiff(before, after *model.State) string { if before == nil || after == nil { return "" } - return udiff.Unified("ruleset.nft (now)", "ruleset.nft (after)", s.fw.Render(before).Text, s.fw.Render(after).Text) + now, then := s.fw.Render(before).Text, s.fw.Render(after).Text + return blockHeaders(udiff.Unified("ruleset.nft (now)", "ruleset.nft (after)", now, then), now, then) +} + +// blockHeaders names, after each hunk's @@ line, the chain (or other +// block of the table) the hunk starts in, as git names the function: +// "@@ -40,6 +40,8 @@ chain forward". Without it a change deep in a long +// chain doesn't say which chain it is in. +func blockHeaders(diff, now, then string) string { + if diff == "" { + return diff + } + oldBlocks, newBlocks := enclosingBlocks(now), enclosingBlocks(then) + lines := strings.SplitAfter(diff, "\n") + for i, l := range lines { + if !strings.HasPrefix(l, "@@ ") { + continue + } + // "@@ -a,b +c,d @@": the new side's start, or the old side's + // when nothing of the hunk is left. + f := strings.Fields(l) + if len(f) < 4 || f[3] != "@@" { + continue + } + block := blockAt(newBlocks, f[2]) + if block == "" { + block = blockAt(oldBlocks, f[1]) + } + if block != "" { + lines[i] = strings.TrimRight(l, "\n") + " " + block + "\n" + } + } + return strings.Join(lines, "") +} + +// enclosingBlocks gives each line of a rendered ruleset the block of the +// table it is in ("chain input"), "" outside one. A block opens with a +// line of one tab ending in "{" and closes with "\t}". +func enclosingBlocks(text string) []string { + var out []string + cur := "" + for _, l := range strings.Split(text, "\n") { + if strings.HasPrefix(l, "\t") && !strings.HasPrefix(l, "\t\t") && strings.HasSuffix(l, "{") { + cur = strings.TrimSpace(strings.TrimSuffix(l, "{")) + } + out = append(out, cur) + if l == "\t}" { + cur = "" + } + } + return out +} + +// blockAt is the block at a hunk side's start ("+40,8" or "-40"); lines +// count from 1, and an empty side (",0") sits after its line. +func blockAt(blocks []string, side string) string { + start, _, _ := strings.Cut(side[1:], ",") + n, err := strconv.Atoi(start) + if err != nil || n < 1 || n > len(blocks) { + return "" + } + return blocks[n-1] } // kindOf maps the API's plural path names to kinds. @@ -616,6 +677,38 @@ func (s *Server) getHistory(w http.ResponseWriter, r *http.Request, p *auth.Prin return nil } +// historyDiff is the ruleset's diff for one history entry: the +// configurations before and after it, got back by stepping back from +// the current one through every later entry, rendered by this version. +func (s *Server) historyDiff(w http.ResponseWriter, r *http.Request, p *auth.Principal) error { + if err := require(p, auth.ResConfig, false); err != nil { + return err + } + id, err := pathID(r, "id") + if err != nil { + return err + } + hs, err := s.store.HistorySince(r.Context(), id) + if err != nil { + return err + } + if len(hs) == 0 || hs[len(hs)-1].ID != id { + return notFound("history entry %d not found", id) + } + st := s.eng.State() + var after *model.State + for _, h := range hs { + if h.ID == id { + after = st.Clone() + } + if err := engine.Rewind(st, h); err != nil { + return err + } + } + writeJSON(w, http.StatusOK, map[string]string{"diff": s.rulesetDiff(st, after)}) + return nil +} + func (s *Server) revertHistory(w http.ResponseWriter, r *http.Request, p *auth.Principal) error { if err := require(p, auth.ResConfig, true); err != nil { return err diff --git a/internal/server/routes.go b/internal/server/routes.go index 9b6cf72..198a58f 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -75,6 +75,7 @@ func (s *Server) routes() { m.HandleFunc("GET "+p+"/stats/heatmap", s.authed(s.statsHeatmap)) m.HandleFunc("GET "+p+"/history", s.authed(s.listHistory)) m.HandleFunc("GET "+p+"/history/{id}", s.authed(s.getHistory)) + m.HandleFunc("GET "+p+"/history/{id}/diff", s.authed(s.historyDiff)) m.HandleFunc("POST "+p+"/history/{id}/revert", s.authed(s.revertHistory)) m.HandleFunc("GET "+p+"/probation", s.authed(s.getProbation)) m.HandleFunc("POST "+p+"/confirm", s.authed(s.confirmChanges)) diff --git a/internal/store/history.go b/internal/store/history.go index f1796e2..aae99be 100644 --- a/internal/store/history.go +++ b/internal/store/history.go @@ -63,3 +63,21 @@ func (s *Store) GetHistory(ctx context.Context, id int64) (*model.HistoryEntry, } return e, err } + +// HistorySince lists the entries from id on, newest first. +func (s *Store) HistorySince(ctx context.Context, id int64) ([]*model.HistoryEntry, error) { + rows, err := s.db.QueryContext(ctx, `SELECT `+historyCols+` FROM history WHERE id >= ? ORDER BY id DESC`, id) + if err != nil { + return nil, err + } + defer rows.Close() + var out []*model.HistoryEntry + for rows.Next() { + e, err := scanHistory(rows) + if err != nil { + return nil, err + } + out = append(out, e) + } + return out, rows.Err() +} diff --git a/internal/web/static/assets/pages/history.js b/internal/web/static/assets/pages/history.js index c7342ba..4de1f5b 100644 --- a/internal/web/static/assets/pages/history.js +++ b/internal/web/static/assets/pages/history.js @@ -1,5 +1,5 @@ // History: every configuration change, with undo. -import { h, mount, table, time, confirmDialog, modal, errorBox, badge } from "../ui.js"; +import { h, mount, table, time, confirmDialog, modal, errorBox, badge, diffView } from "../ui.js"; import { get, change, can, state } from "../api.js"; import { rerender } from "../app.js"; @@ -29,7 +29,7 @@ async function undo(e) { } async function entry(el, id) { - const e = await get("/history/" + id); + const [e, d] = await Promise.all([get("/history/" + id), get("/history/" + id + "/diff").catch(() => null)]); mount(el, h("nav", { class: "back-link" }, h("a", { href: "#/history" }, "← History")), h("div", { class: "page-head" }, @@ -40,7 +40,11 @@ async function entry(el, id) { can("config", true) && !e.reverted_by ? h("div", { class: "actions" }, h("button", { onclick: () => undo(e) }, "Undo")) : null), e.changes.map((c) => h("div", { class: "panel" }, h("h3", null, action(c) + " " + c.kind + " ", h("span", { class: "mono" }, c.label)), - fieldsTable(c)))); + fieldsTable(c))), + d ? h("div", { class: "panel" }, + h("h3", null, "Ruleset"), + h("p", { class: "muted small" }, "What this change did to the ruleset, rendered by this version of Housecarl."), + diffView(d.diff)) : null); } function action(c) { @@ -50,9 +54,10 @@ function action(c) { } // fieldsTable lists the fields that differ (all of them for a create or -// delete). +// delete). An object within (settings' builtins) is listed by its own +// fields, "builtins.drop_invalid", so only what changed shows. function fieldsTable(c) { - const a = c.before || {}, b = c.after || {}; + const a = flatten(c.before || {}), b = flatten(c.after || {}); const keys = [...new Set([...Object.keys(a), ...Object.keys(b)])].sort(); const show = (v, k) => { if (v === undefined || v === "") return "–"; @@ -68,3 +73,14 @@ function fieldsTable(c) { c.after ? { label: c.before ? "After" : "Value", render: (k) => h("span", { class: "mono small" }, show(b[k], k)) } : null, ].filter(Boolean), rows); } + +// flatten lifts the fields of objects within an object to dotted keys +// ({builtins: {loopback: true}} → {"builtins.loopback": true}); lists +// stay whole. +function flatten(o, prefix = "", out = {}) { + for (const [k, v] of Object.entries(o)) { + if (v && typeof v === "object" && !Array.isArray(v) && Object.keys(v).length) flatten(v, prefix + k + ".", out); + else out[prefix + k] = v; + } + return out; +} diff --git a/internal/web/static/assets/style.css b/internal/web/static/assets/style.css index 5f214d5..348de70 100644 --- a/internal/web/static/assets/style.css +++ b/internal/web/static/assets/style.css @@ -409,6 +409,7 @@ input.wide { width: 100%; } .diff .add { background: var(--add); color: var(--add-text); } .diff .del { background: var(--del); color: var(--del-text); } .diff .hunk { background: var(--hunk); color: var(--accent); } +.diff .hunk .block { font-weight: 700; color: var(--text); } .diff .file { font-weight: 700; padding-top: 6px; } select.inline { width: auto; display: inline-block; } /* The rule dialog's optional parts: "+" buttons, and ✕ to close one. */ diff --git a/internal/web/static/assets/ui.js b/internal/web/static/assets/ui.js index a4c2fb5..4489f5f 100644 --- a/internal/web/static/assets/ui.js +++ b/internal/web/static/assets/ui.js @@ -200,7 +200,12 @@ export function diffView(text) { if (line.startsWith("+++") || line.startsWith("---")) cls = "file"; else if (line.startsWith("+")) cls = "add"; else if (line.startsWith("-")) cls = "del"; - else if (line.startsWith("@@")) cls = "hunk"; + else if (line.startsWith("@@")) { + // "@@ -a,b +c,d @@ chain forward": the chain the hunk is in. + const m = line.match(/^(@@ .*? @@)(?: (.+))?$/); + box.append(h("div", { class: "hunk" }, m ? [m[1], m[2] ? [" ", h("span", { class: "block" }, m[2])] : null] : line)); + continue; + } box.append(h("div", { class: cls }, line || " ")); } return box;