From eb4280637906dfa117b16b71a3302a93e655be7f Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 07:43:08 +0200 Subject: [PATCH] Make grafana prefer the vault's password, and let the secrets list only shrink (hq issue 342) grafana's client logged in with a token setting ahead of the vault's admin password, and its runtime config let a layer set token. The token setting is now ignored and said, token is protected, and the vault's password comes first. checks/secrets/known could gain a line hiding a newly unprotected key. merge-check.sh now judges it against the trunk's copy at the merge base, and refuses an added line. --- checks/secrets/known | 2 +- checks/secrets/main.go | 45 +++++++++++++++++--- checks/secrets/main_test.go | 47 +++++++++++++++++++++ merge-check.sh | 15 ++++++- modules/grafana/client.ts | 26 +++++++++--- modules/grafana/module.json | 2 +- modules/grafana/test/secret-setting.test.ts | 40 ++++++++++++++++++ 7 files changed, 161 insertions(+), 16 deletions(-) create mode 100644 modules/grafana/test/secret-setting.test.ts diff --git a/checks/secrets/known b/checks/secrets/known index b1d0003e..a82a571e 100644 --- a/checks/secrets/known +++ b/checks/secrets/known @@ -1,4 +1,4 @@ # Secrets nested below a key a mergeable file takes for its other values, which the controller cannot # protect alone yet (it protects top-level keys): " ". This list only shrinks: -# a line that no longer names a finding is refused (novox/hq issue 342). +# a line that no longer names a finding is refused, and so is one the trunk's copy does not hold (hq issue 342). searxng settings server.secret_key diff --git a/checks/secrets/main.go b/checks/secrets/main.go index 52513c3c..4a317967 100644 --- a/checks/secrets/main.go +++ b/checks/secrets/main.go @@ -11,10 +11,11 @@ // // A secret nested below a key the file takes for its other values (`server.secret_key` below `server`) cannot // be protected alone today: the controller protects top-level keys. Such a file is named in the file `known` -// beside this check, one line per key, until the controller refuses a nested key; a line there that no longer -// names a finding is refused too, so the list only shrinks. +// beside this check, one line per key, until the controller refuses a nested key. The list only shrinks: a +// line there that no longer names a finding is refused, and so is a line the trunk's copy (the merge base's, +// given with -base) does not hold. While the trunk holds no copy, the lines in seed are the trunk's. // -// go run . +// go run . [-base ] // // Exits 1 on a finding. package main @@ -22,6 +23,7 @@ package main import ( "bufio" "encoding/json" + "flag" "fmt" "os" "path/filepath" @@ -152,6 +154,23 @@ func Known(path string) (map[string]bool, error) { return out, s.Err() } +// seed is the known list as it was first written, standing for the trunk's copy until the trunk holds one. +var seed = map[string]bool{ + "searxng settings server.secret_key": true, +} + +// Added answers the lines of known that base does not hold: a line may leave the list, never join it. +func Added(known, base map[string]bool) []string { + var out []string + for line := range known { + if !base[line] { + out = append(out, line) + } + } + sort.Strings(out) + return out +} + // Judge splits the findings into those refused and the known lines that no longer name one. func Judge(found []Finding, known map[string]bool) (refused []Finding, stale []string) { seen := map[string]bool{} @@ -181,9 +200,11 @@ func remedy(key string) string { } func main() { + basePath := flag.String("base", "", "the trunk's copy of known (the merge base's); seed when absent") + flag.Parse() root := "." - if len(os.Args) > 1 { - root = os.Args[1] + if flag.NArg() > 0 { + root = flag.Arg(0) } found, err := Check(root) if err != nil { @@ -195,6 +216,18 @@ func main() { fmt.Fprintln(os.Stderr, err) os.Exit(1) } + base := seed + if *basePath != "" { + if base, err = Known(*basePath); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + } + added := Added(known, base) + for _, line := range added { + fmt.Printf("checks/secrets/known gains %q, which the trunk's copy does not hold: the list only shrinks — "+ + "protect the key instead (novox/hq issue 342)\n", line) + } refused, stale := Judge(found, known) for _, f := range refused { fmt.Printf("%s: %s fills %s with a vault secret, and a setting could replace it (novox/hq issue 342): %s\n", @@ -204,7 +237,7 @@ func main() { fmt.Printf("checks/secrets/known names %q, which no longer holds a secret a setting could replace: "+ "remove the line\n", line) } - if len(refused) > 0 || len(stale) > 0 { + if len(refused) > 0 || len(stale) > 0 || len(added) > 0 { os.Exit(1) } fmt.Printf("secrets: no setting can replace a vault secret in a mergeable file (%d known, nested)\n", len(known)) diff --git a/checks/secrets/main_test.go b/checks/secrets/main_test.go index 8ce090e3..48b0bd23 100644 --- a/checks/secrets/main_test.go +++ b/checks/secrets/main_test.go @@ -76,3 +76,50 @@ func TestTheCatalogueHoldsNoSecretASettingCouldReplace(t *testing.T) { t.Fatalf("refused %v, stale %v", refused, stale) } } + +func TestALineTheTrunkDoesNotHoldIsRefused(t *testing.T) { + trunk := map[string]bool{"b cfg server.secret_key": true} + // A key unprotected on the branch, and its line added to the list: refused, though it matches a finding. + branch := map[string]bool{"b cfg server.secret_key": true, "a cfg password": true} + if added := Added(branch, trunk); len(added) != 1 || added[0] != "a cfg password" { + t.Fatalf("added %v", added) + } + // A line removed is no addition. + if added := Added(map[string]bool{}, trunk); len(added) != 0 { + t.Fatalf("added %v", added) + } + // Until the trunk holds a copy, the seed stands for it, and the list as committed is within it. + known, err := Known("known") + if err != nil { + t.Fatal(err) + } + if added := Added(known, seed); len(added) != 0 { + t.Fatalf("known holds lines the seed does not: %v", added) + } +} + +func TestTheCheckRefusesALineAddedAgainstTheTrunk(t *testing.T) { + root := t.TempDir() + manifest(t, root, "a", `{"id":"cfg","type":"file","merge":"json","content":"{\"password\": \"${secret:admin}\"}"}`) + dir := t.TempDir() + base := filepath.Join(dir, "base") + if err := os.WriteFile(base, []byte("# the trunk's\n"), 0o644); err != nil { + t.Fatal(err) + } + branch := map[string]bool{"a cfg password": true} + found, err := Check(root) + if err != nil { + t.Fatal(err) + } + trunk, err := Known(base) + if err != nil { + t.Fatal(err) + } + refused, stale := Judge(found, branch) + if len(refused) != 0 || len(stale) != 0 { + t.Fatalf("the line hides the finding from Judge alone: %v %v", refused, stale) + } + if added := Added(branch, trunk); len(added) != 1 { + t.Fatalf("an added line passed: %v", added) + } +} diff --git a/merge-check.sh b/merge-check.sh index b9735308..59f73b1e 100755 --- a/merge-check.sh +++ b/merge-check.sh @@ -38,8 +38,19 @@ set -eu (cd checks/words && go test -count=1 . && go run . ../..) # The agent's home (novox/hq ADR 0266): no archive, and no file written into, under it. (cd checks/agenthome && go test -count=1 . && go run . ../..) -# The secrets (novox/hq issue 342): a password is not a setting, so no setting can replace the vault's. -(cd checks/secrets && go test -count=1 . && go run . ../..) +# The secrets (novox/hq issue 342): a password is not a setting, so no setting can replace the vault's. The +# known list only shrinks: judged against the trunk's copy at the merge base, a line it does not hold is refused. +secrets_base="" +secrets_ref="origin/${MESH_CHECK_BASE:-main}" +if merge_base=$(git merge-base HEAD "$secrets_ref" 2>/dev/null); then + if git cat-file -e "$merge_base:checks/secrets/known" 2>/dev/null; then + secrets_base=$(mktemp) + git show "$merge_base:checks/secrets/known" >"$secrets_base" + fi +else + echo "NOT COMPARED WITH THE TRUNK: no merge base with $secrets_ref; checks/secrets/known is held to the list it was first written with" +fi +(cd checks/secrets && go test -count=1 . && if [ -n "$secrets_base" ]; then go run . -base "$secrets_base" ../..; else go run . ../..; fi) # The lists that narrow the paths check may not grow unseen: the base branch's copies are what they are # judged against (MESH_CHECK_BASE, main when unset), read from the clone the check runs in. base_lists="" diff --git a/modules/grafana/client.ts b/modules/grafana/client.ts index 1e490698..3c1befc3 100644 --- a/modules/grafana/client.ts +++ b/modules/grafana/client.ts @@ -44,6 +44,17 @@ function meshConfig(file?: string): Record { catch { return {}; } } +/** + * A password, token or API key is the vault's, never a setting (novox/hq issue 342): a value for one of + * them in the settings file is ignored, and that is said once — by its key, never with its value. + */ +const saidIgnored = new Set(); +function ignoreSecretSetting(cfg: Record, key: string): void { + if (cfg[key] === undefined || saidIgnored.has(key)) return; + saidIgnored.add(key); + console.error(`the setting ${key} is ignored: it comes from the vault`); +} + export class GrafanaClient { readonly baseUrl: string; private readonly authHeader: string; @@ -54,21 +65,24 @@ export class GrafanaClient { } /** - * Build from the module's resolved environment. Auth is a service-account/API token - * (MESH_GRAFANA_TOKEN, sent as Bearer) when present, else HTTP basic with the admin password the - * module keeps as its own secret (MESH_GRAFANA_PASSWORD, user MESH_GRAFANA_USER, default admin). - * Throws when neither is configured — the module then contributes nothing rather than failing. + * Build from the module's resolved environment. Auth is HTTP basic with the vault's admin password, + * which the runtime config carries as `password` (kept protected, so no setting replaces it) or + * MESH_GRAFANA_PASSWORD (user MESH_GRAFANA_USER, default admin); else a service-account/API token from + * the environment (MESH_GRAFANA_TOKEN, sent as Bearer). A `token` in the settings file is never used: a + * credential is the vault's, never a setting (hq issue 342). Throws when neither is configured — the + * module then contributes nothing rather than failing. */ static fromEnv(env: NodeJS.ProcessEnv = process.env): GrafanaClient { const cfg = meshConfig(env.MESH_GRAFANA_CONFIG_FILE); const url = cfg.url ?? env.MESH_GRAFANA_URL ?? `http://127.0.0.1:${env.GRAFANA_PORT ?? "3000"}`; - const token = cfg.token ?? env.MESH_GRAFANA_TOKEN; - if (token) return new GrafanaClient(url, `Bearer ${token}`); + ignoreSecretSetting(cfg, "token"); const password = cfg.password ?? env.MESH_GRAFANA_PASSWORD; if (password) { const user = cfg.user ?? env.MESH_GRAFANA_USER ?? "admin"; return new GrafanaClient(url, `Basic ${Buffer.from(`${user}:${password}`).toString("base64")}`); } + const token = env.MESH_GRAFANA_TOKEN; + if (token) return new GrafanaClient(url, `Bearer ${token}`); throw new Error("no Grafana auth — set MESH_GRAFANA_TOKEN or MESH_GRAFANA_PASSWORD"); } diff --git a/modules/grafana/module.json b/modules/grafana/module.json index 52d565dd..8c16177f 100644 --- a/modules/grafana/module.json +++ b/modules/grafana/module.json @@ -142,7 +142,7 @@ "mode": "0600", "content": "{\n \"user\": \"admin\",\n \"password\": \"${secret:admin}\"\n}\n", "merge": "json", - "protected": ["password"] + "protected": ["password", "token"] } ], "requires": [ diff --git a/modules/grafana/test/secret-setting.test.ts b/modules/grafana/test/secret-setting.test.ts new file mode 100644 index 00000000..3e71cb5c --- /dev/null +++ b/modules/grafana/test/secret-setting.test.ts @@ -0,0 +1,40 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): grafana logs in +// with the vault's admin password, a `token` in the settings file is ignored, and that is said once, by the +// key alone. The runtime config keeps both keys protected, so the controller refuses a layer setting either. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { GrafanaClient } from "../dist/client.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +test("the vault's admin password is used, a token setting is ignored and said once, without its value", () => { + const said: string[] = []; + const err = mock.method(console, "error", (...args: unknown[]) => { said.push(args.join(" ")); }); + try { + const dir = mkdtempSync(join(tmpdir(), "grafana-342-")); + // What the runtime config holds once the vault filled it, with a token a layer carried beside it. + writeFileSync(join(dir, "config.json"), JSON.stringify({ user: "admin", password: VAULT, token: SETTING })); + const env = { MESH_GRAFANA_CONFIG_FILE: join(dir, "config.json") }; + const basic = "Basic " + Buffer.from(`admin:${VAULT}`).toString("base64"); + assert.equal((GrafanaClient.fromEnv(env) as any).authHeader, basic); + assert.equal((GrafanaClient.fromEnv(env) as any).authHeader, basic); + assert.deepEqual(said, ["the setting token is ignored: it comes from the vault"]); + assert.ok(!said.join("\n").includes(SETTING), "the setting's value is never said"); + } finally { + err.mock.restore(); + } +}); + +test("the runtime config keeps password and token protected", () => { + const manifest = JSON.parse(readFileSync(new URL("../module.json", import.meta.url), "utf8")) as { + resources: { id: string; protected?: string[] }[]; + }; + const runtime = manifest.resources.find((r) => r.id === "runtime-config"); + assert.deepEqual(runtime?.protected, ["password", "token"]); +});