From fe2040c65ea2fbb5ba66a9f21bc4f73be9383b98 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 07:32:19 +0200 Subject: [PATCH 1/2] Keep every password, token and API key the vault's, never a setting (hq issue 342) A setting outranked the vault: baserow, grafana, letta and unifi merged a layer's password over the ${secret:...} in their runtime config, and the gitea, home-assistant, icecast, influxdb, mailu and nextcloud clients read the settings file before the vault's file. A secret in a setting cannot be rotated and is read back by the settings verb. The four files now keep their password protected, so the controller refuses a setting of it. The six clients ignore a secret in the settings file and say so once, by key and never with the value; icecast reads its admin password from the vault's file. checks/secrets refuses a mergeable file whose secret a layer could replace; searxng's nested server.secret_key is listed as known until the controller protects a nested key. --- checks/secrets/go.mod | 3 + checks/secrets/known | 4 + checks/secrets/main.go | 211 ++++++++++++++++++ checks/secrets/main_test.go | 78 +++++++ merge-check.sh | 5 + modules/baserow/module.json | 3 +- modules/gitea/client.ts | 17 +- modules/gitea/test/secret-setting.test.ts | 41 ++++ modules/grafana/module.json | 3 +- modules/home-assistant/client.ts | 14 +- modules/home-assistant/package.json | 2 +- .../test/secret-setting.test.ts | 38 ++++ modules/icecast/client.ts | 22 +- modules/icecast/module.json | 3 +- modules/icecast/test/secret-setting.test.ts | 39 ++++ modules/influxdb/client.ts | 14 +- modules/influxdb/test/secret-setting.test.ts | 38 ++++ modules/letta/module.json | 3 +- modules/mailu/client.ts | 14 +- modules/mailu/test/secret-setting.test.ts | 38 ++++ modules/nextcloud/client.ts | 14 +- modules/nextcloud/test/secret-setting.test.ts | 38 ++++ modules/unifi/module.json | 3 +- 23 files changed, 632 insertions(+), 13 deletions(-) create mode 100644 checks/secrets/go.mod create mode 100644 checks/secrets/known create mode 100644 checks/secrets/main.go create mode 100644 checks/secrets/main_test.go create mode 100644 modules/gitea/test/secret-setting.test.ts create mode 100644 modules/home-assistant/test/secret-setting.test.ts create mode 100644 modules/icecast/test/secret-setting.test.ts create mode 100644 modules/influxdb/test/secret-setting.test.ts create mode 100644 modules/mailu/test/secret-setting.test.ts create mode 100644 modules/nextcloud/test/secret-setting.test.ts diff --git a/checks/secrets/go.mod b/checks/secrets/go.mod new file mode 100644 index 00000000..1a0dd13d --- /dev/null +++ b/checks/secrets/go.mod @@ -0,0 +1,3 @@ +module git.novox.be/novox/mesh-catalog/checks/secrets + +go 1.22 diff --git a/checks/secrets/known b/checks/secrets/known new file mode 100644 index 00000000..b1d0003e --- /dev/null +++ b/checks/secrets/known @@ -0,0 +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). +searxng settings server.secret_key diff --git a/checks/secrets/main.go b/checks/secrets/main.go new file mode 100644 index 00000000..52513c3c --- /dev/null +++ b/checks/secrets/main.go @@ -0,0 +1,211 @@ +// Command secrets refuses, in every module's manifest, a mergeable file that a setting could fill with a +// password, a token or an API key in place of the vault's (novox/hq issue 342). +// +// A password is not a setting: it comes from the key vault, and it can be rotated there. A file that says +// `"merge": "json"` takes the keys of the mesh-wide and the node's layers over its own content, so a key the +// content fills with `${secret:}` is one a layer could replace, and the vault's value would lose to it +// until somebody removed it by hand. The controller refuses a setting of a key the file keeps as `protected`, +// naming the layer and the key, never the value. This check holds every such file to that: each top-level key +// whose value holds `${secret:…}`, anywhere below it, is either listed in the file's `protected`, or left out +// of the file's `takes-settings` (novox/hq ADR 0326), so that no layer reaches it. +// +// 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. +// +// go run . +// +// Exits 1 on a finding. +package main + +import ( + "bufio" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" +) + +// Finding is one key of one mergeable file a layer could fill in place of the vault. +type Finding struct { + Module, ID, Key string +} + +func (f Finding) String() string { return f.Module + " " + f.ID + " " + f.Key } + +// Check reads every manifest under root/modules and answers each secret-bearing key a layer could replace. +func Check(root string) ([]Finding, error) { + manifests, err := filepath.Glob(filepath.Join(root, "modules", "*", "module.json")) + if err != nil { + return nil, err + } + sort.Strings(manifests) + var out []Finding + for _, path := range manifests { + raw, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var m struct { + Module string `json:"module"` + Resources []map[string]any `json:"resources"` + } + if err := json.Unmarshal(raw, &m); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + for _, r := range m.Resources { + if how, _ := r["merge"].(string); how == "" { + continue + } + id, _ := r["id"].(string) + content, _ := r["content"].(string) + var doc map[string]any + if strings.TrimSpace(content) == "" { + continue + } + if err := json.Unmarshal([]byte(content), &doc); err != nil { + return nil, fmt.Errorf("%s: %s merges as JSON and its content is not JSON: %w", m.Module, id, err) + } + protected := set(r["protected"]) + takes, names := r["takes-settings"] + taken := set(takes) + for _, key := range sortedKeys(doc) { + for _, at := range secretsBelow(key, doc[key]) { + switch { + case protected[key]: + case names && !taken[key]: + default: + out = append(out, Finding{m.Module, id, at}) + } + } + } + } + } + return out, nil +} + +// secretsBelow answers the dotted path of every string at or below value that names a vault secret. +func secretsBelow(at string, value any) []string { + switch v := value.(type) { + case string: + if strings.Contains(v, "${secret:") { + return []string{at} + } + case map[string]any: + var out []string + for _, k := range sortedKeys(v) { + out = append(out, secretsBelow(at+"."+k, v[k])...) + } + return out + case []any: + var out []string + for i, e := range v { + out = append(out, secretsBelow(fmt.Sprintf("%s[%d]", at, i), e)...) + } + return out + } + return nil +} + +func set(v any) map[string]bool { + out := map[string]bool{} + list, _ := v.([]any) + for _, e := range list { + if s, ok := e.(string); ok { + out[s] = true + } + } + return out +} + +func sortedKeys(m map[string]any) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// Known reads the lines of the known list: " ", # comments and blank lines skipped. +func Known(path string) (map[string]bool, error) { + f, err := os.Open(path) + if os.IsNotExist(err) { + return map[string]bool{}, nil + } + if err != nil { + return nil, err + } + defer f.Close() + out := map[string]bool{} + s := bufio.NewScanner(f) + for s.Scan() { + line := strings.TrimSpace(s.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + out[strings.Join(strings.Fields(line), " ")] = true + } + return out, s.Err() +} + +// 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{} + for _, f := range found { + seen[f.String()] = true + if !known[f.String()] { + refused = append(refused, f) + } + } + for line := range known { + if !seen[line] { + stale = append(stale, line) + } + } + sort.Strings(stale) + return refused, stale +} + +// remedy says how a finding is put right. +func remedy(key string) string { + top, _, nested := strings.Cut(key, ".") + if !nested { + return fmt.Sprintf("list %q in the file's \"protected\", so the controller refuses a setting of it", top) + } + return fmt.Sprintf("the controller protects a top-level key only; list %q in the file's \"protected\" if "+ + "nothing else below it is the operator's, or leave it out of the file's \"takes-settings\"", top) +} + +func main() { + root := "." + if len(os.Args) > 1 { + root = os.Args[1] + } + found, err := Check(root) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + known, err := Known("known") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + 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", + f.Module, f.ID, f.Key, remedy(f.Key)) + } + for _, line := range stale { + 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 { + 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 new file mode 100644 index 00000000..8ce090e3 --- /dev/null +++ b/checks/secrets/main_test.go @@ -0,0 +1,78 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func manifest(t *testing.T, root, module, resources string) { + t.Helper() + dir := filepath.Join(root, "modules", module) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + body := `{"module":"` + module + `","resources":[` + resources + `]}` + if err := os.WriteFile(filepath.Join(dir, "module.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestASecretASettingCouldReplaceIsRefused(t *testing.T) { + root := t.TempDir() + // Refused: the key holds the vault's secret and nothing keeps a layer off it. + manifest(t, root, "a", `{"id":"cfg","type":"file","merge":"json","content":"{\"password\": \"${secret:admin}\"}"}`) + // Refused: nested below a key the file takes. + manifest(t, root, "b", `{"id":"cfg","type":"file","merge":"json","takes-settings":["server"],`+ + `"content":"{\"server\": {\"secret_key\": \"${secret:s}\", \"port\": 1}}"}`) + // Kept: protected. + manifest(t, root, "c", `{"id":"cfg","type":"file","merge":"json","protected":["password"],`+ + `"content":"{\"password\": \"${secret:admin}\", \"user\": \"admin\"}"}`) + // Kept: the file names its keys, and not this one. + manifest(t, root, "d", `{"id":"cfg","type":"file","merge":"json","takes-settings":["ui"],`+ + `"content":"{\"token\": \"${secret:t}\", \"ui\": {}}"}`) + // Kept: a file that does not merge takes no setting. + manifest(t, root, "e", `{"id":"cfg","type":"file","content":"{\"token\": \"${secret:t}\"}"}`) + // Refused: protecting another key does not protect this one; a list element counts too. + manifest(t, root, "f", `{"id":"cfg","type":"file","merge":"json","protected":["user"],`+ + `"content":"{\"users\": [{\"password\": \"${secret:p}\"}]}"}`) + found, err := Check(root) + if err != nil { + t.Fatal(err) + } + want := []string{"a cfg password", "b cfg server.secret_key", "f cfg users[0].password"} + if len(found) != len(want) { + t.Fatalf("found %v, want %v", found, want) + } + for i, f := range found { + if f.String() != want[i] { + t.Fatalf("found %v, want %v", found, want) + } + } +} + +func TestTheKnownListOnlyShrinks(t *testing.T) { + found := []Finding{{"b", "cfg", "server.secret_key"}, {"a", "cfg", "password"}} + refused, stale := Judge(found, map[string]bool{"b cfg server.secret_key": true, "gone cfg token": true}) + if len(refused) != 1 || refused[0].String() != "a cfg password" { + t.Fatalf("refused %v", refused) + } + if len(stale) != 1 || stale[0] != "gone cfg token" { + t.Fatalf("stale %v", stale) + } +} + +func TestTheCatalogueHoldsNoSecretASettingCouldReplace(t *testing.T) { + found, err := Check("../..") + if err != nil { + t.Fatal(err) + } + known, err := Known("known") + if err != nil { + t.Fatal(err) + } + refused, stale := Judge(found, known) + if len(refused) > 0 || len(stale) > 0 { + t.Fatalf("refused %v, stale %v", refused, stale) + } +} diff --git a/merge-check.sh b/merge-check.sh index b2bddd63..b9735308 100755 --- a/merge-check.sh +++ b/merge-check.sh @@ -30,11 +30,16 @@ # 5. the agent's home (novox/hq ADR 0266): no module unpacks an archive, or writes into a file, under # ${machine:agent-home} — the node-engine does both after checking the path, and the agent account owns # that home and could swap a link in between. +# 6. the secrets (novox/hq issue 342): no mergeable file fills a key with a vault secret that a setting could +# replace — the key is `protected`, or left out of the file's `takes-settings`. checks/secrets/known names +# the nested ones the controller cannot protect alone yet, and only shrinks. 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 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/baserow/module.json b/modules/baserow/module.json index f80fab30..366ab9c0 100644 --- a/modules/baserow/module.json +++ b/modules/baserow/module.json @@ -103,7 +103,8 @@ "path": "${dir:mesh-state}/config.json", "mode": "0600", "content": "{\n \"password\": \"${secret:admin}\",\n \"host\": \"${bound:route:name}\"\n}\n", - "merge": "json" + "merge": "json", + "protected": ["password"] } ], "build": { diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index a2401df9..2a92d80d 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -138,6 +138,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 GiteaClient { readonly baseUrl: string; private readonly tokens: TokenSource; @@ -151,7 +162,8 @@ export class GiteaClient { /** * Build from the module's resolved environment. The URL comes from MESH_GITEA_URL (the mesh's own * name), falling back to the bare GITEA_URL and to the forge's loopback port. The token, in order: - * one configured in settings or the environment (MESH_GITEA_TOKEN / GITEA_TOKEN), which wins; else + * one configured in the environment (MESH_GITEA_TOKEN / GITEA_TOKEN), which wins — never a setting, which is + * ignored and said (hq issue 342); else * one the module mints for itself with the admin account the vault delivered and keeps in its own * state (token.ts; hq issue 100). Throws only when neither is possible, naming what is missing, * rather than hand back a client that fails on first use. @@ -159,7 +171,8 @@ export class GiteaClient { static fromEnv(env: NodeJS.ProcessEnv = process.env): GiteaClient { const cfg = meshConfig(env.MESH_GITEA_CONFIG_FILE); const url = cfg.url ?? env.MESH_GITEA_URL ?? env.GITEA_URL ?? `http://127.0.0.1:${env.GITEA_PORT ?? "3000"}`; - const configured = cfg.token ?? env.MESH_GITEA_TOKEN ?? env.GITEA_TOKEN; + ignoreSecretSetting(cfg, "token"); + const configured = env.MESH_GITEA_TOKEN ?? env.GITEA_TOKEN; if (configured) return new GiteaClient(url, new ConfiguredToken(configured)); return new GiteaClient(url, MintedToken.fromEnv(url, env)); } diff --git a/modules/gitea/test/secret-setting.test.ts b/modules/gitea/test/secret-setting.test.ts new file mode 100644 index 00000000..025376c7 --- /dev/null +++ b/modules/gitea/test/secret-setting.test.ts @@ -0,0 +1,41 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): a value for it in +// the settings file the mesh merges is ignored, the vault's is used, and that is said once, by the key alone. +// Run against the compiled module, the way the runtime loads it. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { GiteaClient } from "../dist/client.js"; +import { ConfiguredToken, MintedToken } from "../dist/token.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +function delivered(settings: Record): Record { + const dir = mkdtempSync(join(tmpdir(), "gitea-342-")); + writeFileSync(join(dir, "config.json"), JSON.stringify(settings)); + writeFileSync(join(dir, "vault.secret"), VAULT + "\n"); + return { dir, config: join(dir, "config.json"), vault: join(dir, "vault.secret") }; +} + +test("the vault's token is used, a setting of it 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 at = delivered({ token: SETTING }); + const env = { MESH_GITEA_CONFIG_FILE: at.config, MESH_GITEA_URL: "http://127.0.0.1:1", MESH_GITEA_ADMIN_USER: "mesh-admin", MESH_GITEA_ADMIN_PASSWORD_FILE: at.vault, MESH_GITEA_STATE_DIR: at.dir }; + const first = GiteaClient.fromEnv(env); + const second = GiteaClient.fromEnv(env); + // No token from the setting: the module mints its own with the vault's admin account. + assert.ok((first as any).tokens instanceof MintedToken); + assert.ok((second as any).tokens instanceof MintedToken); + assert.ok(!((first as any).tokens instanceof ConfiguredToken)); + 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(); + } +}); diff --git a/modules/grafana/module.json b/modules/grafana/module.json index 9ac69bcc..52d565dd 100644 --- a/modules/grafana/module.json +++ b/modules/grafana/module.json @@ -141,7 +141,8 @@ "path": "${dir:mesh-state}/config.json", "mode": "0600", "content": "{\n \"user\": \"admin\",\n \"password\": \"${secret:admin}\"\n}\n", - "merge": "json" + "merge": "json", + "protected": ["password"] } ], "requires": [ diff --git a/modules/home-assistant/client.ts b/modules/home-assistant/client.ts index 62e8323b..5ab0339f 100644 --- a/modules/home-assistant/client.ts +++ b/modules/home-assistant/client.ts @@ -27,6 +27,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`); +} + /** Read a secret the mesh mounted at a file path (an own-secret delivered by `secret accept`); * absent or unreadable yields undefined so callers fall back rather than crash. */ function readSecret(file?: string): string | undefined { @@ -53,7 +64,8 @@ export class HomeAssistantClient { static fromEnv(env: NodeJS.ProcessEnv = process.env): HomeAssistantClient { const cfg = meshConfig(env.MESH_HOMEASSISTANT_CONFIG_FILE); const url = cfg.url ?? env.MESH_HOMEASSISTANT_URL ?? `http://127.0.0.1:${env.HOMEASSISTANT_PORT ?? "8123"}`; - const token = cfg.token ?? readSecret(env.MESH_HOMEASSISTANT_TOKEN_FILE) ?? env.MESH_HOMEASSISTANT_TOKEN; + ignoreSecretSetting(cfg, "token"); + const token = readSecret(env.MESH_HOMEASSISTANT_TOKEN_FILE) ?? env.MESH_HOMEASSISTANT_TOKEN; if (!token) throw new Error("no Home Assistant token — set MESH_HOMEASSISTANT_TOKEN"); return new HomeAssistantClient(url, token); } diff --git a/modules/home-assistant/package.json b/modules/home-assistant/package.json index 311ce134..e6a7ad8c 100644 --- a/modules/home-assistant/package.json +++ b/modules/home-assistant/package.json @@ -7,7 +7,7 @@ "scripts": { "build": "tsc client.ts index.ts tools/index.ts provisions/hass.ts provisions/probe.ts provisions/connections.ts provisions/mesh.ts provisions/index.ts --module NodeNext --moduleResolution NodeNext --target ES2022 --outDir dist", "typecheck": "tsc -p tsconfig.json", - "test": "node --test --experimental-strip-types 'test/*.test.ts'" + "test": "npm run build && node --test --experimental-strip-types 'test/*.test.ts'" }, "dependencies": { "@novox/mesh-sdk": "^0.1.0" diff --git a/modules/home-assistant/test/secret-setting.test.ts b/modules/home-assistant/test/secret-setting.test.ts new file mode 100644 index 00000000..736e154d --- /dev/null +++ b/modules/home-assistant/test/secret-setting.test.ts @@ -0,0 +1,38 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): a value for it in +// the settings file the mesh merges is ignored, the vault's is used, and that is said once, by the key alone. +// Run against the compiled module, the way the runtime loads it. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { HomeAssistantClient } from "../dist/client.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +function delivered(settings: Record): Record { + const dir = mkdtempSync(join(tmpdir(), "home-assistant-342-")); + writeFileSync(join(dir, "config.json"), JSON.stringify(settings)); + writeFileSync(join(dir, "vault.secret"), VAULT + "\n"); + return { dir, config: join(dir, "config.json"), vault: join(dir, "vault.secret") }; +} + +test("the vault's token is used, a setting of it 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 at = delivered({ token: SETTING }); + const env = { MESH_HOMEASSISTANT_CONFIG_FILE: at.config, MESH_HOMEASSISTANT_TOKEN_FILE: at.vault }; + const first = HomeAssistantClient.fromEnv(env); + const second = HomeAssistantClient.fromEnv(env); + assert.equal((first as any).token, VAULT); + assert.equal((second as any).token, VAULT); + 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(); + } +}); diff --git a/modules/icecast/client.ts b/modules/icecast/client.ts index 60f1b3a1..bf69069c 100644 --- a/modules/icecast/client.ts +++ b/modules/icecast/client.ts @@ -42,6 +42,24 @@ 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`); +} + +/** A secret delivered as a file, trimmed; undefined when there is none. */ +function readSecret(path: string | undefined): string | undefined { + if (!path) return undefined; + try { return readFileSync(path, "utf8").trim() || undefined; } + catch { return undefined; } +} + export class IcecastClient { readonly baseUrl: string; private readonly authHeader?: string; @@ -57,7 +75,9 @@ export class IcecastClient { static fromEnv(env: NodeJS.ProcessEnv = process.env): IcecastClient { const cfg = meshConfig(env.MESH_ICECAST_CONFIG_FILE); const url = cfg.url ?? (env.MESH_ICECAST_URL ?? `http://127.0.0.1:${env.ICECAST_PORT ?? "8000"}`); - return new IcecastClient(url, cfg.user ?? env.MESH_ICECAST_ADMIN_USER, cfg.password ?? env.MESH_ICECAST_ADMIN_PASSWORD); + ignoreSecretSetting(cfg, "password"); + const password = readSecret(env.MESH_ICECAST_ADMIN_PASSWORD_FILE) ?? env.MESH_ICECAST_ADMIN_PASSWORD; + return new IcecastClient(url, cfg.user ?? env.MESH_ICECAST_ADMIN_USER ?? "admin", password); } async getStatus(): Promise { diff --git a/modules/icecast/module.json b/modules/icecast/module.json index 2e320759..4c241ebc 100644 --- a/modules/icecast/module.json +++ b/modules/icecast/module.json @@ -120,7 +120,8 @@ ], "env": { "MESH_ICECAST_URL": "http://127.0.0.1:${port:8000}", - "MESH_ICECAST_CONFIG_FILE": "${dir:mesh-state}/config.json" + "MESH_ICECAST_CONFIG_FILE": "${dir:mesh-state}/config.json", + "MESH_ICECAST_ADMIN_PASSWORD_FILE": "${dir:state}/admin.secret" } } ] diff --git a/modules/icecast/test/secret-setting.test.ts b/modules/icecast/test/secret-setting.test.ts new file mode 100644 index 00000000..b344d64e --- /dev/null +++ b/modules/icecast/test/secret-setting.test.ts @@ -0,0 +1,39 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): a value for it in +// the settings file the mesh merges is ignored, the vault's is used, and that is said once, by the key alone. +// Run against the compiled module, the way the runtime loads it. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { IcecastClient } from "../dist/client.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +function delivered(settings: Record): Record { + const dir = mkdtempSync(join(tmpdir(), "icecast-342-")); + writeFileSync(join(dir, "config.json"), JSON.stringify(settings)); + writeFileSync(join(dir, "vault.secret"), VAULT + "\n"); + return { dir, config: join(dir, "config.json"), vault: join(dir, "vault.secret") }; +} + +test("the vault's password is used, a setting of it 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 at = delivered({ password: SETTING }); + const env = { MESH_ICECAST_CONFIG_FILE: at.config, MESH_ICECAST_ADMIN_PASSWORD_FILE: at.vault }; + const first = IcecastClient.fromEnv(env); + const second = IcecastClient.fromEnv(env); + const basic = "Basic " + Buffer.from(`admin:${VAULT}`).toString("base64"); + assert.equal((first as any).authHeader, basic); + assert.equal((second as any).authHeader, basic); + assert.deepEqual(said, ["the setting password 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(); + } +}); diff --git a/modules/influxdb/client.ts b/modules/influxdb/client.ts index 0a3f8ab0..849e6547 100644 --- a/modules/influxdb/client.ts +++ b/modules/influxdb/client.ts @@ -43,6 +43,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`); +} + /** A secret delivered as a file, trimmed; undefined when there is none, so the caller can fall back. */ function tokenFromFile(file?: string): string | undefined { if (!file) return undefined; @@ -71,7 +82,8 @@ export class InfluxDBClient { const url = cfg.url ?? env.MESH_INFLUXDB_URL ?? `http://127.0.0.1:${env.INFLUXDB_PORT ?? "8086"}`; // The token reaches the process as a file (novox/hq ADR 0086); the environment variable stays // only for a workstation running the tools by hand. - const token = cfg.token ?? tokenFromFile(env.MESH_INFLUXDB_TOKEN_FILE) ?? env.MESH_INFLUXDB_TOKEN; + ignoreSecretSetting(cfg, "token"); + const token = tokenFromFile(env.MESH_INFLUXDB_TOKEN_FILE) ?? env.MESH_INFLUXDB_TOKEN; if (!token) throw new Error("no InfluxDB token — set MESH_INFLUXDB_TOKEN_FILE"); const org = cfg.org ?? env.MESH_INFLUXDB_ORG ?? "mesh"; return new InfluxDBClient(url, token, org); diff --git a/modules/influxdb/test/secret-setting.test.ts b/modules/influxdb/test/secret-setting.test.ts new file mode 100644 index 00000000..3af7bb65 --- /dev/null +++ b/modules/influxdb/test/secret-setting.test.ts @@ -0,0 +1,38 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): a value for it in +// the settings file the mesh merges is ignored, the vault's is used, and that is said once, by the key alone. +// Run against the compiled module, the way the runtime loads it. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { InfluxDBClient } from "../dist/client.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +function delivered(settings: Record): Record { + const dir = mkdtempSync(join(tmpdir(), "influxdb-342-")); + writeFileSync(join(dir, "config.json"), JSON.stringify(settings)); + writeFileSync(join(dir, "vault.secret"), VAULT + "\n"); + return { dir, config: join(dir, "config.json"), vault: join(dir, "vault.secret") }; +} + +test("the vault's token is used, a setting of it 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 at = delivered({ token: SETTING }); + const env = { MESH_INFLUXDB_CONFIG_FILE: at.config, MESH_INFLUXDB_TOKEN_FILE: at.vault }; + const first = InfluxDBClient.fromEnv(env); + const second = InfluxDBClient.fromEnv(env); + assert.equal((first as any).token, VAULT); + assert.equal((second as any).token, VAULT); + 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(); + } +}); diff --git a/modules/letta/module.json b/modules/letta/module.json index 50013b0c..4174b99c 100644 --- a/modules/letta/module.json +++ b/modules/letta/module.json @@ -114,7 +114,8 @@ "path": "${dir:mesh-state}/config.json", "mode": "0600", "content": "{\n \"password\": \"${secret:server-password}\"\n}\n", - "merge": "json" + "merge": "json", + "protected": ["password"] } ], "build": { diff --git a/modules/mailu/client.ts b/modules/mailu/client.ts index f269c927..38fff9a6 100644 --- a/modules/mailu/client.ts +++ b/modules/mailu/client.ts @@ -52,6 +52,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`); +} + /** Read a secret from the file the mesh mounted it at (an own-secret), if the pointing env is set. */ function readSecret(path: string | undefined): string | undefined { if (!path) return undefined; @@ -81,7 +92,8 @@ export class MailuClient { static fromEnv(env: NodeJS.ProcessEnv = process.env): MailuClient { const cfg = meshConfig(env.MESH_MAILU_CONFIG_FILE); const url = cfg.url ?? env.MESH_MAILU_URL; - const apiKey = cfg.apiKey ?? readSecret(env.MESH_MAILU_API_KEY_FILE) ?? env.MESH_MAILU_API_KEY; + ignoreSecretSetting(cfg, "apiKey"); + const apiKey = readSecret(env.MESH_MAILU_API_KEY_FILE) ?? env.MESH_MAILU_API_KEY; if (!url || !apiKey) { throw new Error("Mailu is not configured — set MESH_MAILU_URL and the API token own-secret"); } diff --git a/modules/mailu/test/secret-setting.test.ts b/modules/mailu/test/secret-setting.test.ts new file mode 100644 index 00000000..cad28553 --- /dev/null +++ b/modules/mailu/test/secret-setting.test.ts @@ -0,0 +1,38 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): a value for it in +// the settings file the mesh merges is ignored, the vault's is used, and that is said once, by the key alone. +// Run against the compiled module, the way the runtime loads it. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { MailuClient } from "../dist/client.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +function delivered(settings: Record): Record { + const dir = mkdtempSync(join(tmpdir(), "mailu-342-")); + writeFileSync(join(dir, "config.json"), JSON.stringify(settings)); + writeFileSync(join(dir, "vault.secret"), VAULT + "\n"); + return { dir, config: join(dir, "config.json"), vault: join(dir, "vault.secret") }; +} + +test("the vault's apiKey is used, a setting of it 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 at = delivered({ apiKey: SETTING }); + const env = { MESH_MAILU_CONFIG_FILE: at.config, MESH_MAILU_URL: "http://127.0.0.1:1/api/v1", MESH_MAILU_API_KEY_FILE: at.vault }; + const first = MailuClient.fromEnv(env); + const second = MailuClient.fromEnv(env); + assert.equal((first as any).apiKey, VAULT); + assert.equal((second as any).apiKey, VAULT); + assert.deepEqual(said, ["the setting apiKey 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(); + } +}); diff --git a/modules/nextcloud/client.ts b/modules/nextcloud/client.ts index e93d9c7b..32ec8a9f 100644 --- a/modules/nextcloud/client.ts +++ b/modules/nextcloud/client.ts @@ -32,6 +32,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 NextcloudClient { constructor( private readonly container: string, @@ -53,7 +64,8 @@ export class NextcloudClient { const ocsUrl = cfg.url ?? env.MESH_NEXTCLOUD_URL ?? `http://127.0.0.1:${env.NEXTCLOUD_PORT ?? "80"}`; const adminUser = cfg.user ?? env.MESH_NEXTCLOUD_ADMIN_USER ?? "admin"; const passwordFile = env.MESH_NEXTCLOUD_ADMIN_PASSWORD_FILE; - const adminPassword = cfg.password ?? env.MESH_NEXTCLOUD_ADMIN_PASSWORD + ignoreSecretSetting(cfg, "password"); + const adminPassword = env.MESH_NEXTCLOUD_ADMIN_PASSWORD ?? (passwordFile ? readFileSync(passwordFile, "utf8").trim() : undefined); if (!adminPassword) { throw new Error("no Nextcloud admin password — set MESH_NEXTCLOUD_ADMIN_PASSWORD_FILE " + diff --git a/modules/nextcloud/test/secret-setting.test.ts b/modules/nextcloud/test/secret-setting.test.ts new file mode 100644 index 00000000..6b61269d --- /dev/null +++ b/modules/nextcloud/test/secret-setting.test.ts @@ -0,0 +1,38 @@ +// A password, a token or an API key is the vault's, never a setting (novox/hq issue 342): a value for it in +// the settings file the mesh merges is ignored, the vault's is used, and that is said once, by the key alone. +// Run against the compiled module, the way the runtime loads it. + +import { test, mock } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { NextcloudClient } from "../dist/client.js"; + +const SETTING = "one-a-setting-carried"; +const VAULT = "the-vault-made-this"; + +function delivered(settings: Record): Record { + const dir = mkdtempSync(join(tmpdir(), "nextcloud-342-")); + writeFileSync(join(dir, "config.json"), JSON.stringify(settings)); + writeFileSync(join(dir, "vault.secret"), VAULT + "\n"); + return { dir, config: join(dir, "config.json"), vault: join(dir, "vault.secret") }; +} + +test("the vault's password is used, a setting of it 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 at = delivered({ password: SETTING }); + const env = { MESH_NEXTCLOUD_CONFIG_FILE: at.config, MESH_NEXTCLOUD_ADMIN_PASSWORD_FILE: at.vault }; + const first = NextcloudClient.fromEnv(env); + const second = NextcloudClient.fromEnv(env); + assert.equal((first as any).adminPassword, VAULT); + assert.equal((second as any).adminPassword, VAULT); + assert.deepEqual(said, ["the setting password 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(); + } +}); diff --git a/modules/unifi/module.json b/modules/unifi/module.json index 4c5ddd67..65b457cc 100644 --- a/modules/unifi/module.json +++ b/modules/unifi/module.json @@ -131,7 +131,8 @@ "path": "${dir:mesh-state}/config.json", "mode": "0600", "content": "{\n \"site\": \"default\",\n \"password\": \"${secret:controller}\"\n}\n", - "merge": "json" + "merge": "json", + "protected": ["password"] } ], "requires": [ From eb4280637906dfa117b16b71a3302a93e655be7f Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 07:43:08 +0200 Subject: [PATCH 2/2] 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"]); +});