diff --git a/modules/docker/README.md b/modules/docker/README.md index 125b3de..c342572 100644 --- a/modules/docker/README.md +++ b/modules/docker/README.md @@ -126,9 +126,10 @@ A failure is an error naming how it failed, never an empty answer. | tool | | what | |---|---|---| | `docker_list` | r | every container: image, state, health, restarts, ports, mounts, compose project, `mesh_held`; filter by owner, state or name | -| `docker_inspect` | r | one container whole, **environment values left out** (names kept) | +| `docker_inspect` | r | one container whole, **environment values left out** (names kept), and its command line redacted as an exec's is | | `docker_logs` | r | the last lines of both streams, merged in order, with timestamps (default 200, at most 2000); **a secret the container printed is shown as `[redacted: ]`** | | `docker_secrets_in_logs` | r | which containers printed a secret they were given, **by name, never by value** (below) | +| `docker_secrets_in_events` | r | which secrets exec command lines carried, as the runtime recorded them in its events, **by name, never by value** (below) | | `docker_stats` | r | CPU, memory, I/O and process count per running container, heaviest first | | `docker_start` / `docker_stop` / `docker_restart` | a | one container. On a mesh-held one, the answer says the host restores its declared state at its next apply | | `docker_top` | r | the processes inside one container | @@ -137,7 +138,7 @@ A failure is an error naming how it failed, never an empty answer. | `docker_disk_usage` | r | `docker system df -v`: total, active and reclaimable per kind, with the largest of each | | `docker_networks` | r | networks, subnets, and the containers on each | | `docker_volumes` | r | volumes, who mounts each, whether the mesh holds one of them, anonymous or not, and sizes if asked | -| `docker_events` | r | the runtime's events over a window ending now (default 60 min, at most 24 h), without exec noise | +| `docker_events` | r | the runtime's events over a window ending now (default 60 min, at most 24 h), without exec noise unless asked; **an exec's command line is shown with any secret it carried as `[redacted: ]`** | | `docker_daemon_config` | r | `daemon.json` as on disk, `docker info`'s essentials, and keys the daemon has not taken yet | | `docker_unlabelled` | r | the containers the mesh does not hold: the cleanup list | | `docker_problems` | r | unhealthy, restarting, dead, killed for memory, failed, or restarted five times or more | @@ -168,6 +169,35 @@ and kept in their transcripts. Its answer says how many it redacted, and points A finding is a secret to rotate once the program stops printing it; recreating the container drops its old log (the runtime's file goes with the container). +## Secrets on an exec's command line (hq issue 282) + +The runtime records the command line of every exec — a `docker exec`, and a health check, which is one +— in its event stream (`exec_create: `). A program that passes a password to a +tool as an argument has given it to everyone who may ask the runtime what happened, for as long as the +runtime keeps its events, and to every transcript of a `docker_events` call. The mosquitto module did +that with the broker's admin password on every administrative call, until it handed it over on stdin. + +`docker_events` redacts, in every exec's command line, before it answers: + +- the values of that container's environment named like a secret, and the passwords in its URIs — + the same values `docker_logs` redacts; +- any URI carrying a password; +- by shape, whatever the source: the word after a flag that takes a password (`-P`, `--password`, + `--secret-key`, `--token`, …; `-a` for `redis-cli`; `-p` for `mosquitto_ctrl`, whose connect `-p` + is a port and is left alone as a number), a `NAME=value` whose name says secret, and the password a + `mosquitto_ctrl dynsec` command sets as an argument (`setClientPassword`, `init`). + +`docker_secrets_in_events` reads the exec events of a window ending now (60 minutes by default, at most +24 hours) and names each secret found by container, module, what it was and the program, with how many +execs carried it and the first and last time — **never the value or the command line**. A finding is +code to change first (the secret handed over as a file or on stdin), then a secret to rotate. The +runtime keeps a bounded number of events, so on a busy machine a long window reads only what it still +holds — which is also why a leaked value ages out of the event stream quickly, and not out of a +transcript that already copied it. + +`docker_inspect` shows a container's own command line (`Path`/`Args`, `Cmd`, `Entrypoint`) redacted +the same way. + ## Tests ``` diff --git a/modules/docker/cmd/docker-tools/cmdline.go b/modules/docker/cmd/docker-tools/cmdline.go new file mode 100644 index 0000000..5245d97 --- /dev/null +++ b/modules/docker/cmd/docker-tools/cmdline.go @@ -0,0 +1,256 @@ +package main + +// A secret on a command line (novox/hq issue 282). +// +// **The leak this catches.** The runtime records the command line of every exec — `docker exec`, and +// a health check, which is one — in its event stream, as the event's action (`exec_create: `). A program that hands a password to a tool as an argument (`-P `, +// `--password `, `PGPASSWORD= psql`) has therefore given it to everyone who may +// ask the runtime what happened, for as long as the runtime keeps its events — and, through +// docker_events, to every transcript of an agent that asked. The mosquitto module did exactly that +// with the broker's admin password, on every administrative call. +// +// **What is known here.** As for a log: the values of the container's environment named like a +// secret and the passwords inside its URIs, by name; and, whatever their source, the values a +// command line carries by its shape — the word after a flag that takes a password, a NAME=value +// whose name says secret, the password a dynsec command sets. A secret given as a file and passed by +// a flag the shapes do not know is not caught. +// +// **Never the value.** What is shown carries `[redacted: ]` in its place, and a finding +// names the container, the module and what it was, by name. + +import ( + "context" + "fmt" + "path" + "sort" + "strings" + "time" +) + +// passwordFlags take a secret as their next word, whatever the program. +var passwordFlags = map[string]bool{ + "-P": true, "--password": true, "--pass": true, "--passwd": true, "--secret": true, "--secret-key": true, + "--token": true, "--api-key": true, "--apikey": true, "--auth": true, +} + +// programFlags take a secret as their next word for one program only: elsewhere the same flag means +// something else (redis-cli's -a is its password; nft's -a is not). +var programFlags = map[string]map[string]bool{ + "redis-cli": {"-a": true}, + "keydb-cli": {"-a": true}, + "valkey-cli": {"-a": true}, + "mosquitto_ctrl": {"-p": true}, // createClient -p ; the connect -p is a port, and a port is ordinary +} + +// positionalSecret is where a dynsec command carries a password as an argument: the word that many +// places after the command's name. +var positionalSecret = map[string]int{"setClientPassword": 2, "init": 3} + +// commandSecret is one secret a command line carried, by what it was. +type commandSecret struct { + Name string + Value string +} + +// secretsOnCommandLine are the values a command line carries by their shape, by what each one is. +func secretsOnCommandLine(words []string) []commandSecret { + var out []commandSecret + add := func(name, value string) { + if len(value) < leastSecret || masked.MatchString(value) || ordinary.MatchString(value) { + return + } + out = append(out, commandSecret{name, value}) + } + program := "" + for i, w := range words { + base := path.Base(w) + if _, known := programFlags[base]; known || base == "mosquitto_ctrl" { + program = base + } + if flag, value, ok := strings.Cut(w, "="); ok && strings.HasPrefix(flag, "-") { + if passwordFlags[flag] || programFlags[program][flag] { + add("the value of "+flag, value) + } + continue + } + if name, value, ok := strings.Cut(w, "="); ok && name != "" && !strings.HasPrefix(name, "-") && + secretName.MatchString(name) && !notAValue.MatchString(name) && !strings.ContainsAny(name, "/:") { + add("the value of "+name, value) + continue + } + if i+1 < len(words) && (passwordFlags[w] || programFlags[program][w]) { + add("the word after "+w+" in a "+orProgram(program, words)+" command line", words[i+1]) + } + if program == "mosquitto_ctrl" { + if at, ok := positionalSecret[w]; ok && i+at < len(words) && dynsecVerb(words, i) { + add("the password given to dynsec "+w, words[i+at]) + } + } + } + return out +} + +// dynsecVerb says the word at i is a dynsec command's name: it follows "dynsec". +func dynsecVerb(words []string, i int) bool { + for j := i - 1; j >= 0; j-- { + if words[j] == "dynsec" { + return true + } + } + return false +} + +func orProgram(program string, words []string) string { + if program != "" { + return program + } + if len(words) > 0 { + return path.Base(words[0]) + } + return "program's" +} + +// redactCommand is a command line with every known secret, every password inside a URI and every +// value its shape says is a secret replaced by a mark naming what was there; and what was replaced, +// by name. +func redactCommand(command string, known []knownSecret) (string, []string) { + var names []string + for _, s := range known { + for _, f := range forms(s.Value) { + if strings.Contains(command, f) { + command = strings.ReplaceAll(command, f, "[redacted: "+s.Name+"]") + names = append(names, s.Name) + break + } + } + } + for _, s := range secretsOnCommandLine(strings.Fields(command)) { + if strings.Contains(command, s.Value) { + command = strings.ReplaceAll(command, s.Value, "[redacted: "+s.Name+"]") + names = append(names, s.Name) + } + } + if line, n := redact(command, nil); n > 0 { + command = line + names = append(names, "a password in a URI") + } + return command, names +} + +// execCommand is the command line an exec event carries, and whether it carries one. +func execCommand(action string) (verb, command string, ok bool) { + verb, command, ok = strings.Cut(action, ": ") + if !ok || !strings.HasPrefix(verb, "exec_") { + return "", "", false + } + return verb, command, true +} + +// envCache reads each container's environment once per call. +type envCache struct { + c *Client + ctx context.Context + seen map[string][]knownSecret +} + +func (e *envCache) of(id string) []knownSecret { + if e.seen == nil { + e.seen = map[string][]knownSecret{} + } + if k, ok := e.seen[id]; ok { + return k + } + env, _ := e.c.envOf(e.ctx, id) // a container gone since: its shapes are still caught + e.seen[id] = secretsIn(env) + return e.seen[id] +} + +// CommandLeak is one secret the runtime recorded on exec command lines: by name, never by value. +type CommandLeak struct { + Container string `json:"container"` + HeldBy string `json:"held_by,omitempty"` + Module string `json:"module,omitempty"` + Secret string `json:"secret"` + Execs int `json:"execs"` + Program string `json:"program"` + First string `json:"first"` + Last string `json:"last"` +} + +// SecretsInEvents reads the runtime's exec events in a window ending now and says which secrets +// their command lines carried, by container and name. +func (c *Client) SecretsInEvents(ctx context.Context, minutes int) (map[string]any, error) { + out, err := c.docker(ctx, "events", "--since", fmt.Sprintf("%dm", minutes), "--until", "0s", + "--filter", "type=container", "--filter", "event=exec_create", "--format", "{{json .}}") + if err != nil { + return nil, err + } + raw, err := jsonLines[runtimeEvent](out) + if err != nil { + return nil, err + } + envs := &envCache{c: c, ctx: ctx} + type key struct{ container, secret string } + found := map[key]*CommandLeak{} + execs := 0 + for _, e := range raw { + verb, command, ok := execCommand(e.Action) + if !ok || verb != "exec_create" { + continue // an exec_start repeats its exec_create's command line + } + execs++ + _, names := redactCommand(command, envs.of(e.Actor.ID)) + if len(names) == 0 { + continue + } + at := time.Unix(0, e.TimeNano).UTC().Format(time.RFC3339) + held := e.Actor.Attributes[MeshLabel] + module, _, _ := strings.Cut(held, ".") + program := "" + if f := strings.Fields(command); len(f) > 0 { + program = path.Base(f[0]) + } + for _, n := range names { + k := key{e.Actor.Attributes["name"], n} + l, ok := found[k] + if !ok { + l = &CommandLeak{Container: k.container, HeldBy: held, Module: module, Secret: n, Program: program, First: at} + found[k] = l + } + l.Execs++ + l.Last = at + } + } + leaks := []CommandLeak{} + for _, l := range found { + leaks = append(leaks, *l) + } + sort.Slice(leaks, func(i, j int) bool { + if leaks[i].Container != leaks[j].Container { + return leaks[i].Container < leaks[j].Container + } + return leaks[i].Secret < leaks[j].Secret + }) + verdict := fmt.Sprintf("no exec in the last %d minutes carried a secret on its command line", minutes) + if len(leaks) > 0 { + verdict = fmt.Sprintf("%d secret(s) on exec command lines the runtime recorded: the code that runs the exec must hand "+ + "them over another way (a file, stdin), and each is rotated once it does (novox/hq issue 282)", len(leaks)) + } + return map[string]any{ + "verdict": verdict, "leaks": leaks, "count": len(leaks), "execs_read": execs, "minutes": minutes, + "knows": "values of each container's environment named like a secret, passwords in URIs, and by shape: the word after " + + "a password flag, a NAME=value named like a secret, and the password a dynsec command sets", + "history": "the runtime keeps a bounded number of events, so a window longer than what it holds reads only what it still has", + }, nil +} + +// runtimeEvent is one line of `docker events --format '{{json .}}'`. +type runtimeEvent struct { + Type, Action string + Actor struct { + ID string + Attributes map[string]string + } + TimeNano int64 `json:"timeNano"` +} diff --git a/modules/docker/cmd/docker-tools/docker.go b/modules/docker/cmd/docker-tools/docker.go index d9a65ad..2fd3db0 100644 --- a/modules/docker/cmd/docker-tools/docker.go +++ b/modules/docker/cmd/docker-tools/docker.go @@ -345,6 +345,27 @@ func (c *Client) Inspect(ctx context.Context, ref string) (map[string]any, error return nil, fmt.Errorf("docker inspect answered something that is not one container") } obj := got[0] + // A container's command line is shown without the secrets it carries, as an exec's is (novox/hq + // issue 282): known from its environment, and by shape. + var known []knownSecret + if cfg, ok := obj["Config"].(map[string]any); ok { + if env, ok := cfg["Env"].([]any); ok { + list := make([]string, 0, len(env)) + for _, e := range env { + list = append(list, fmt.Sprint(e)) + } + known = secretsIn(list) + } + for _, k := range []string{"Cmd", "Entrypoint"} { + if words, ok := cfg[k].([]any); ok { + cfg[k] = redactWords(words, known) + } + } + } + if words, ok := obj["Args"].([]any); ok { + path, _ := obj["Path"].(string) + obj["Args"] = redactWords(append([]any{path}, words...), known)[1:] + } if cfg, ok := obj["Config"].(map[string]any); ok { if env, ok := cfg["Env"].([]any); ok { names := []string{} @@ -979,24 +1000,29 @@ func (c *Client) Events(ctx context.Context, minutes int, kind string, limit int if err != nil { return nil, err } - raw, err := jsonLines[struct { - Type, Action string - Actor struct { - ID string - Attributes map[string]string - } - TimeNano int64 `json:"timeNano"` - }](out) + raw, err := jsonLines[runtimeEvent](out) if err != nil { return nil, err } + // An exec's command line is shown without the secrets it carried (novox/hq issue 282): this answer + // is read by agents and kept in their transcripts, which would make it a second copy of the leak. + envs := &envCache{c: c, ctx: ctx} + redacted := map[string]bool{} events := []map[string]any{} for _, e := range raw { if !execs && strings.HasPrefix(e.Action, "exec_") { continue } + action := e.Action + if verb, command, ok := execCommand(action); ok { + shown, names := redactCommand(command, envs.of(e.Actor.ID)) + action = verb + ": " + shown + for _, n := range names { + redacted[n] = true + } + } _, held := e.Actor.Attributes[MeshLabel] - ev := map[string]any{"time": time.Unix(0, e.TimeNano).UTC().Format(time.RFC3339), "type": e.Type, "action": e.Action, + ev := map[string]any{"time": time.Unix(0, e.TimeNano).UTC().Format(time.RFC3339), "type": e.Type, "action": action, "id": shortID(e.Actor.ID), "name": e.Actor.Attributes["name"]} if e.Type == "container" { ev["mesh_held"] = held @@ -1011,7 +1037,41 @@ func (c *Client) Events(ctx context.Context, minutes int, kind string, limit int if len(events) > limit { events = events[len(events)-limit:] } - return map[string]any{"minutes": minutes, "count": total, "shown": len(events), "events": events}, nil + answer := map[string]any{"minutes": minutes, "count": total, "shown": len(events), "events": events} + if len(redacted) > 0 { + answer["redacted"] = sortedSet(redacted) + answer["leak"] = "exec command lines carried secrets, which the runtime keeps in its events; docker_secrets_in_events names them (novox/hq issue 282)" + } + return answer, nil +} + +func sortedSet(set map[string]bool) []string { + out := make([]string, 0, len(set)) + for k := range set { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// redactWords is a command's words with what redactCommand hides hidden, word by word. +func redactWords(words []any, known []knownSecret) []any { + list := make([]string, len(words)) + for i, w := range words { + list[i] = fmt.Sprint(w) + } + shapes := secretsOnCommandLine(list) + out := make([]any, len(list)) + for i, w := range list { + for _, s := range shapes { + if strings.Contains(w, s.Value) { + w = strings.ReplaceAll(w, s.Value, "[redacted: "+s.Name+"]") + } + } + w, _ = redact(w, known) + out[i] = w + } + return out } // restartOnly are the daemon keys the runtime reads only when it starts: a reload leaves them as diff --git a/modules/docker/cmd/docker-tools/main.go b/modules/docker/cmd/docker-tools/main.go index d4dfee1..a38c687 100644 --- a/modules/docker/cmd/docker-tools/main.go +++ b/modules/docker/cmd/docker-tools/main.go @@ -112,6 +112,22 @@ func tools(c *Client) []stdio.Tool { return c.SecretsInLogs(ctx, held, n) }, }, + { + Name: "docker_secrets_in_events", + Description: "Which secrets exec command lines carried in a window ending now (default the last 60 minutes, at most 24 hours) — the runtime records every exec's command line in its events, " + + "so a password passed as an argument is kept there for anyone who may ask it. By container, module and the secret's name, never its value. " + + "A finding is code to change (hand the secret over as a file or on stdin) and then a secret to rotate (novox/hq issue 282).", + Input: map[string]any{ + "minutes": map[string]any{"type": "integer", "description": "how far back (default 60, at most 1440)"}, + }, + Run: func(args map[string]any) (any, error) { + minutes, err := bounded(args, "minutes", 60, 1440) + if err != nil { + return nil, err + } + return c.SecretsInEvents(ctx, minutes) + }, + }, { Name: "docker_stats", Description: "What the running containers use now — CPU, memory, network and disk I/O, processes — the heaviest by memory first; or one container's.", @@ -211,8 +227,9 @@ func tools(c *Client) []stdio.Tool { }, }, { - Name: "docker_events", - Description: "What the runtime did in a window ending now (default the last 60 minutes, at most 24 hours): containers created, started, died, health changes, images pulled — with mesh_held. Exec events are left out unless asked.", + Name: "docker_events", + Description: "What the runtime did in a window ending now (default the last 60 minutes, at most 24 hours): containers created, started, died, health changes, images pulled — with mesh_held. Exec events are left out unless asked; " + + "an exec's command line is shown with any secret it carried as [redacted: ] — a value of the container's environment named like a secret, a password in a URI, the word after a password flag.", Input: map[string]any{ "minutes": map[string]any{"type": "integer", "description": "how far back (default 60, at most 1440)"}, "type": map[string]any{"type": "string", "description": "only one kind: container, image, network, volume, daemon, plugin or builder"}, diff --git a/modules/docker/cmd/docker-tools/secrets_test.go b/modules/docker/cmd/docker-tools/secrets_test.go index 2f9c680..ef2fa25 100644 --- a/modules/docker/cmd/docker-tools/secrets_test.go +++ b/modules/docker/cmd/docker-tools/secrets_test.go @@ -154,3 +154,109 @@ func keys(m map[string]string) []string { } return out } + +// The shape of the leak in hq issue 282: a broker's admin password on an exec's command line. The +// values are made up for the test. +const ( + adminPassword = "Adm1n-pass_word-xyz" + clientPassword = "Cl1ent-pass_word-abc" +) + +func TestACommandLineIsShownWithoutTheSecretsItCarried(t *testing.T) { + for _, tc := range []struct{ command, mark string }{ + {"mosquitto_ctrl -h 127.0.0.1 -p 1883 -u mesh-admin -P " + adminPassword + " dynsec listClients", "the word after -P"}, + {"mosquitto_ctrl -h 127.0.0.1 -p 1883 dynsec createClient alice -p " + clientPassword, "the word after -p in a mosquitto_ctrl"}, + {"mosquitto_ctrl -o /tmp/x dynsec setClientPassword alice " + clientPassword, "the password given to dynsec setClientPassword"}, + {"redis-cli -a " + adminPassword + " ping", "the word after -a"}, + {"env PGPASSWORD=" + adminPassword + " psql -U app", "the value of PGPASSWORD"}, + {"tool --password=" + adminPassword, "the value of --password"}, + {"psql postgresql://app:" + adminPassword + "@db/app", "a password in a URI"}, + } { + shown, names := redactCommand(tc.command, nil) + if strings.Contains(shown, adminPassword) || strings.Contains(shown, clientPassword) { + t.Errorf("%q: still carries the value: %q", tc.command, shown) + } + if len(names) == 0 || !strings.Contains(strings.Join(names, "|"), tc.mark) { + t.Errorf("%q: named %v, want %q", tc.command, names, tc.mark) + } + } + // A port, a path and a plain command are not secrets. + for _, plain := range []string{ + "mosquitto_ctrl -h 127.0.0.1 -p 1883 dynsec listClients", + "/usr/bin/lavinmqctl status", + "sh -c umask 077\nf=$(mktemp) || exit 1 mosquitto_ctrl -h 127.0.0.1 -p 1883 dynsec getClient alice", + "pg_dump -Fc -f /dumps/app.dump app", + } { + if shown, names := redactCommand(plain, nil); shown != plain || len(names) > 0 { + t.Errorf("%q: redacted %v as %q", plain, names, shown) + } + } + // What the container's environment holds is known by its name, wherever it appears. + known := []knownSecret{{"SERVER_PASSWORD", adminPassword}} + if shown, names := redactCommand("app login "+adminPassword, known); strings.Contains(shown, adminPassword) || + len(names) != 1 || names[0] != "SERVER_PASSWORD" { + t.Errorf("an environment secret on a command line: %q %v", shown, names) + } +} + +const execEvents = `{"Type":"container","Action":"exec_create: mosquitto_ctrl -h 127.0.0.1 -p 1883 -u mesh-admin -P ` + adminPassword + ` dynsec listClients","Actor":{"ID":"aaaaaaaaaaaaaaaa","Attributes":{"name":"mosquitto","mesh-host.id":"mosquitto.server","execID":"e1"}},"timeNano":1791320000000000000} +{"Type":"container","Action":"exec_start: mosquitto_ctrl -h 127.0.0.1 -p 1883 -u mesh-admin -P ` + adminPassword + ` dynsec listClients","Actor":{"ID":"aaaaaaaaaaaaaaaa","Attributes":{"name":"mosquitto","mesh-host.id":"mosquitto.server","execID":"e1"}},"timeNano":1791320000000100000} +{"Type":"container","Action":"exec_die","Actor":{"ID":"aaaaaaaaaaaaaaaa","Attributes":{"name":"mosquitto","exitCode":"0"}},"timeNano":1791320000000200000} +{"Type":"container","Action":"exec_create: /usr/bin/healthcheck","Actor":{"ID":"bbbbbbbbbbbbbbbb","Attributes":{"name":"other"}},"timeNano":1791320060000000000} +{"Type":"container","Action":"exec_create: mosquitto_ctrl -h 127.0.0.1 -p 1883 -u mesh-admin -P ` + adminPassword + ` dynsec getClient a","Actor":{"ID":"aaaaaaaaaaaaaaaa","Attributes":{"name":"mosquitto","mesh-host.id":"mosquitto.server","execID":"e2"}},"timeNano":1791320120000000000} +` + +func TestEventsShowAnExecsCommandLineWithoutItsSecrets(t *testing.T) { + f := (&fake{}). + on("docker events", Ran{Stdout: execEvents}). + on("docker container inspect --format {{json .Config.Env}}", Ran{Stdout: "[]\n"}) + got, err := client(f, 1000).Events(context.Background(), 30, "", 100, true) + if err != nil { + t.Fatal(err) + } + b, _ := json.Marshal(got) + if strings.Contains(string(b), adminPassword) { + t.Fatalf("the answer carries the value: %s", b) + } + if !strings.Contains(string(b), "[redacted: the word after -P") || got["leak"] == nil { + t.Fatalf("not marked as redacted: %s", b) + } +} + +func TestAScanOfExecEventsNamesEachSecretAndNeverItsValue(t *testing.T) { + f := (&fake{}). + on("docker events", Ran{Stdout: execEvents}). + on("docker container inspect --format {{json .Config.Env}}", Ran{Stdout: "[]\n"}) + got, err := client(f, 1000).SecretsInEvents(context.Background(), 60) + if err != nil { + t.Fatal(err) + } + b, _ := json.Marshal(got) + if strings.Contains(string(b), adminPassword) { + t.Fatalf("the finding carries the value: %s", b) + } + leaks := got["leaks"].([]CommandLeak) + if len(leaks) != 1 || leaks[0].Container != "mosquitto" || leaks[0].Module != "mosquitto" || leaks[0].Execs != 2 || + leaks[0].Program != "mosquitto_ctrl" || !strings.Contains(leaks[0].Secret, "-P") { + t.Fatalf("leaks: %+v", leaks) + } + if got["execs_read"] != 3 { + t.Fatalf("read %v exec_create events, want 3 (a start repeats a create and is not counted)", got["execs_read"]) + } + if !f.ran("docker events --since 60m --until 0s --filter type=container --filter event=exec_create") { + t.Fatalf("not asked for exec creations only: %+v", f.calls) + } +} + +func TestInspectShowsACommandLineWithoutItsSecrets(t *testing.T) { + obj := `[{"Path":"mosquitto_ctrl","Args":["-P","` + adminPassword + `","dynsec","listClients"],"Config":{"Env":["A=b"],"Cmd":["mosquitto_ctrl","-P","` + adminPassword + `"],"Labels":{}}}]` + f := (&fake{}).on("docker container inspect", Ran{Stdout: obj}) + got, err := client(f, 1000).Inspect(context.Background(), "mosquitto") + if err != nil { + t.Fatal(err) + } + b, _ := json.Marshal(got) + if strings.Contains(string(b), adminPassword) || !strings.Contains(string(b), "[redacted:") { + t.Fatalf("inspect: %s", b) + } +} diff --git a/modules/docker/module.json b/modules/docker/module.json index 3ee97da..b041705 100644 --- a/modules/docker/module.json +++ b/modules/docker/module.json @@ -17,6 +17,7 @@ "docker_inspect", "docker_logs", "docker_secrets_in_logs", + "docker_secrets_in_events", "docker_stats", "docker_start", "docker_stop", diff --git a/modules/keycloak/cmd/keycloak-provider/admin.go b/modules/keycloak/cmd/keycloak-provider/admin.go index c9ccaec..cd3afa6 100644 --- a/modules/keycloak/cmd/keycloak-provider/admin.go +++ b/modules/keycloak/cmd/keycloak-provider/admin.go @@ -331,7 +331,10 @@ func (g *Guard) Run(ctx context.Context) { // repairScript is Keycloak's own recovery, run inside its container (Keycloak 26). It reads the // temporary admin's password and then the mesh's admin password from standard input; nothing secret -// is on its command line or in its environment as docker sees it. Every step announces itself on +// is on its command line or in its environment as docker sees it. Nor on the command line of anything +// it runs (novox/hq issue 282): kcadm takes a password from KC_CLI_PASSWORD, set for the one command +// that needs it, and a command's environment is readable by its own user only, where its argv is +// readable by every user of the machine for as long as the JVM runs (verified against Keycloak 26.0.8). Every step announces itself on // stderr, so a failure names the step it failed at. const repairScript = `set -eu umask 077 @@ -352,8 +355,8 @@ cleanup() { if [ -z "$logged_in" ] && [ -n "$bootstrapped" ]; then # The bootstrap may have made the temporary admin and failed after (it did once, on a held port): # log in as it anyway, so it is removed rather than left behind. - "$bin/kcadm.sh" config credentials --config "$cfg" --server http://localhost:8080 --realm master \ - --user "$TMP_USER" --password "$TMP_PW" >/dev/null 2>&1 && logged_in=1 + KC_CLI_PASSWORD="$TMP_PW" "$bin/kcadm.sh" config credentials --config "$cfg" --server http://localhost:8080 \ + --realm master --user "$TMP_USER" >/dev/null 2>&1 && logged_in=1 fi if [ -n "$logged_in" ]; then tid=$(user_id "$TMP_USER" 2>/dev/null) @@ -373,7 +376,7 @@ step bootstrap-admin bootstrapped=1 "$bin/kc.sh" bootstrap-admin user --username "$TMP_USER" --password:env TMP_PW --http-management-port="$MGMT_PORT" >&2 step login -"$bin/kcadm.sh" config credentials --config "$cfg" --server http://localhost:8080 --realm master --user "$TMP_USER" --password "$TMP_PW" >&2 +KC_CLI_PASSWORD="$TMP_PW" "$bin/kcadm.sh" config credentials --config "$cfg" --server http://localhost:8080 --realm master --user "$TMP_USER" >&2 logged_in=1 step find-admin id=$(user_id "$ADMIN_USER") @@ -386,7 +389,7 @@ fi step enable-admin "$bin/kcadm.sh" update "users/$id" -r master --config "$cfg" -s enabled=true >&2 step set-password -"$bin/kcadm.sh" set-password -r master --config "$cfg" --username "$ADMIN_USER" --new-password "$NEW_PW" >&2 +KC_CLI_PASSWORD="$NEW_PW" "$bin/kcadm.sh" set-password -r master --config "$cfg" --username "$ADMIN_USER" >&2 step remove-temporary-admin echo "mesh-repair-done" >&2 ` diff --git a/modules/keycloak/cmd/keycloak-provider/admin_test.go b/modules/keycloak/cmd/keycloak-provider/admin_test.go index 9372efc..30c9e25 100644 --- a/modules/keycloak/cmd/keycloak-provider/admin_test.go +++ b/modules/keycloak/cmd/keycloak-provider/admin_test.go @@ -95,7 +95,7 @@ func TestARefusedAdminIsRepairedInsideTheContainerAndSaid(t *testing.T) { func TestTheScriptIsKeycloaksOwnRecovery(t *testing.T) { for _, want := range []string{ `kc.sh" bootstrap-admin user --username "$TMP_USER" --password:env TMP_PW --http-management-port="$MGMT_PORT"`, - `set-password -r master --config "$cfg" --username "$ADMIN_USER" --new-password "$NEW_PW"`, + `KC_CLI_PASSWORD="$NEW_PW" "$bin/kcadm.sh" set-password -r master --config "$cfg" --username "$ADMIN_USER"`, `umask 077`, `trap cleanup EXIT`, `rm -f "$cfg"`, `delete "users/$tid"`, } { if !strings.Contains(repairScript, want) { @@ -105,6 +105,12 @@ func TestTheScriptIsKeycloaksOwnRecovery(t *testing.T) { if strings.Contains(repairScript, "--cache") { t.Error("bootstrap-admin takes no --cache") } + // Nothing the script runs is given a password as an argument (novox/hq issue 282). + for _, never := range []string{`--password "$`, `--new-password`, `-p "$`} { + if strings.Contains(repairScript, never) { + t.Errorf("the script passes a password as an argument: %s", never) + } + } } func TestAnAdminThatLogsInIsLeftAlone(t *testing.T) { diff --git a/modules/minio/client.ts b/modules/minio/client.ts index f40edf4..cbbc9de 100644 --- a/modules/minio/client.ts +++ b/modules/minio/client.ts @@ -69,7 +69,6 @@ export class MinioClient { private readonly rootPassword: string; private readonly mcBin: string; private readonly mcConfigDir: string; - private aliasReady = false; constructor(opts: MinioOptions) { this.baseUrl = opts.endpoint.replace(/\/$/, ""); @@ -223,9 +222,13 @@ export class MinioClient { * generating one the consumer could never learn. The MinIO admin REST API encrypts this request * with a key derived (Argon2) from the root secret, which node built-ins cannot reproduce — so, as * hal did, the module drives the `mc` CLI, which the runtime image bundles. + * + * **The consumer's secret key is still an argument** (`--secret-key`): `mc admin user svcacct add` + * takes it no other way. It is not a container exec, so the container runtime does not record it, + * but the machine's process table shows it to every local user for the second `mc` runs (novox/hq + * issue 282, open): replacing it means the admin API's encrypted request made here. */ async createAccessKey(bucket: string, accessKey: string, secretKey: string): Promise { - await this.ensureAlias(); const policyPath = join(this.mcConfigDir, `policy-${accessKey}.json`); writeFileSync(policyPath, bucketPolicy(bucket), { mode: 0o600 }); try { @@ -242,18 +245,26 @@ export class MinioClient { } async removeAccessKey(accessKey: string): Promise { - await this.ensureAlias(); await this.mc("admin", "user", "svcacct", "rm", "mesh", accessKey); } - private async ensureAlias(): Promise { - if (this.aliasReady) return; - await this.mc("alias", "set", "mesh", this.baseUrl, this.rootUser, this.rootPassword); - this.aliasReady = true; - } - + /** + * Run `mc` against the alias `mesh`, which it is given as MC_HOST_mesh in its environment — the + * root user and password inside the URL — rather than by an `mc alias set` that carried the root + * password as an argument, readable in the machine's process table by every user while it ran, and + * kept it in the alias file after (novox/hq issue 282). A process's environment is readable by its + * own user only. `mc` reads MC_HOST_ before its configuration, so an alias file an earlier + * version wrote is not consulted. + */ private async mc(...args: string[]): Promise { - const { stdout } = await execFileAsync(this.mcBin, ["--config-dir", this.mcConfigDir, ...args], { timeout: 30_000 }); + const argv = ["--config-dir", this.mcConfigDir, ...args]; + if (argv.some((a) => a.includes(this.rootPassword))) { + throw new Error("refused to run mc: its command line would carry the root password (novox/hq issue 282)"); + } + const { stdout } = await execFileAsync(this.mcBin, argv, { + timeout: 30_000, + env: { ...process.env, MC_HOST_mesh: mcHost(this.baseUrl, this.rootUser, this.rootPassword) }, + }); return stdout.trim(); } @@ -353,3 +364,12 @@ function uriEncode(str: string, encodeSlash: boolean): string { } return out; } + +/** + * The MC_HOST_ URL `mc` reads an alias from: the endpoint with the credentials in its userinfo, + * each percent-encoded so a reserved character in either cannot end it early. + */ +export function mcHost(endpoint: string, user: string, password: string): string { + const u = new URL(endpoint); + return `${u.protocol}//${encodeURIComponent(user)}:${encodeURIComponent(password)}@${u.host}${u.pathname.replace(/\/$/, "")}`; +} diff --git a/modules/mosquitto/bootstrap/index.ts b/modules/mosquitto/bootstrap/index.ts index 53864fd..f757df9 100644 --- a/modules/mosquitto/bootstrap/index.ts +++ b/modules/mosquitto/bootstrap/index.ts @@ -20,8 +20,20 @@ // otherwise leave a root-owned file the broker at 1883 can neither read (if 0600) nor rewrite. // So after writing, it chowns the file to 1883:1883 and sets 0600 — the broker's to read and to // grow, and no one else's. This is the ownership question ADR 0052 left for the lab to settle. +// +// **And it keeps the broker's admin password the mesh's** (novox/hq issue 282). The admin password is +// an own secret the mesh may rotate (ADR 0228): it makes a new value, writes it, and this step runs +// again (its `restart-on` names the secret). The store holds the password's hash, which only the +// broker changes, and only for an admin who presents the password it holds — so this step keeps the +// value it last applied, in a 0600 file of the module's own state, and when the mesh's value is no +// longer that one it connects with the applied one and sets the new one, online, on stdin. The +// broker keeps running and loses no client; the step then records the new value as applied. Until +// it has, nothing of this module can administer the broker, so a re-key that cannot be done fails +// the step, loudly, by name and never by value. -import { chownSync, chmodSync, existsSync } from "node:fs"; +import { chownSync, chmodSync, existsSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; import { MosquittoClient } from "../client.js"; // The broker (eclipse-mosquitto) runs as this uid/gid; the seeded store must be its to read and @@ -32,16 +44,111 @@ const BROKER_GID = Number(process.env.MESH_MQTT_BROKER_GID ?? "1883") || 1883; // The same path the broker's `plugin_opt_config_file` names, reached through the shared data volume. const configFile = process.env.MESH_DYNSEC_FILE ?? "/mosquitto/data/dynamic-security.json"; +// Where this step keeps the admin password it last applied to the store (see the header). +const appliedFile = process.env.MESH_MQTT_ADMIN_APPLIED_FILE ?? ""; +// The broker's container, entered to re-key the admin and started if a re-key finds it stopped. +const container = process.env.MESH_MQTT_CTRL_CONTAINER ?? ""; + +const mosquitto = MosquittoClient.fromEnv(); +const wanted = readSecret(process.env.MESH_PROVISION_PASSWORD_FILE); +if (!wanted) throw new Error("the broker's admin password is not readable; nothing was seeded or re-keyed"); + if (existsSync(configFile)) { - // Already seeded — and possibly grown by the running plugin since. Leave it exactly as it is. + // Already seeded — and possibly grown by the running plugin since. Never rewritten here. console.log(`[mosquitto:bootstrap] ${configFile} already exists; leaving it untouched`); + await keepAdminPassword(); } else { - const mosquitto = MosquittoClient.fromEnv(); await mosquitto.initBootstrapFile(configFile); // Hand the store to the broker's user so it can read the seed and persist to it (see the header). chownSync(configFile, BROKER_UID, BROKER_GID); chmodSync(configFile, 0o600); + recordApplied(wanted); console.log( `[mosquitto:bootstrap] seeded ${configFile} with the dynsec admin client, owned by ${BROKER_UID}:${BROKER_GID}`, ); } + +/** + * Make the store's admin password the mesh's. Nothing to do when the value applied last is the + * mesh's; otherwise the broker is asked to take the new one from an admin holding the old. + */ +async function keepAdminPassword(): Promise { + if (!appliedFile) { + console.log("[mosquitto:bootstrap] no applied-password file is named; the admin password is not kept here"); + return; + } + const applied = readSecret(appliedFile); + if (applied === wanted) return; + + let now = await mosquitto.adminAccepted(wanted); + if (now === undefined && applied) now = await startAndAsk(wanted); + if (now === true) { + // The broker already takes it: a re-key that ran before its record was written, or the first run + // of this step on a store seeded before it kept a record. + recordApplied(wanted); + console.log("[mosquitto:bootstrap] the broker takes the mesh's admin password; recorded as applied"); + return; + } + if (now === undefined && applied) { + // Started, or not startable, and still not answering: the store keeps the old password until the + // broker takes the new one, so this step fails rather than records what did not happen. + throw new Error(`the mesh's admin password changed and the broker cannot be reached to take it: start ` + + `${container || "the broker"} and apply again (novox/hq issue 282)`); + } + if (now === undefined) { + // Nothing applied is known and the broker is not up: the store was seeded with the value it holds + // now as far as anything here can tell, and the first connection says otherwise if it was not. + recordApplied(wanted); + console.log("[mosquitto:bootstrap] the broker is not running; the mesh's admin password is recorded as applied, unverified"); + return; + } + if (!applied) { + throw new Error("the broker refuses the mesh's admin password, and no password applied before is recorded " + + "to change it with: the store's admin client was given another one (novox/hq issue 282)"); + } + const old = mosquitto.withAdminPassword(applied); + if ((await old.adminAccepted(applied)) !== true) { + throw new Error("the broker refuses both the mesh's admin password and the one applied before it; " + + "the admin client cannot be re-keyed from here (novox/hq issue 282)"); + } + await old.ctlWithPassword(wanted, "setClientPassword", mosquitto.adminUser); + if ((await mosquitto.adminAccepted(wanted)) !== true) { + throw new Error("the broker was asked to take the mesh's new admin password and still refuses it"); + } + recordApplied(wanted); + console.log("[mosquitto:bootstrap] the broker's admin password was replaced by the mesh's new one, online"); +} + +/** Start the broker's container when it is stopped, and ask again once it answers. */ +async function startAndAsk(password: string): Promise { + if (!container) return undefined; + try { + await promisify(execFile)("docker", ["start", container]); + } catch { + return undefined; // no such container yet: the apply creates it after this step + } + for (let i = 0; i < 20; i++) { + const answer = await mosquitto.adminAccepted(password); + if (answer !== undefined) return answer; + await new Promise((r) => setTimeout(r, 1000)); + } + return undefined; +} + +/** Record the value now applied: written whole, 0600, then moved into place. */ +function recordApplied(value: string): void { + if (!appliedFile) return; + const next = `${appliedFile}.next`; + writeFileSync(next, value, { mode: 0o600 }); + chmodSync(next, 0o600); + renameSync(next, appliedFile); +} + +function readSecret(path: string | undefined): string { + if (!path) return ""; + try { + return readFileSync(path, "utf8").trim(); + } catch { + return ""; + } +} diff --git a/modules/mosquitto/client.ts b/modules/mosquitto/client.ts index ae8ed2e..f5a19ec 100644 --- a/modules/mosquitto/client.ts +++ b/modules/mosquitto/client.ts @@ -77,44 +77,58 @@ export class MosquittoClient { return this.conn.port; } + get adminUser(): string { + return this.conn.adminUser; + } + /** * Run one `mosquitto_ctrl dynsec ` command against the broker as the admin client and return - * its stdout. Connects over MQTT with the verified connect flags `-h`/`-p`/`-u`/`-P`. A failed - * command rejects — a failure here is an error, not a success with a warning. + * its stdout. A failed command rejects — a failure here is an error, not a success with a warning. * - * The exit code cannot carry that verdict: `mosquitto_ctrl` 2.0.x exits 0 from its dynsec + * **No secret rides on a command line** (novox/hq issue 282). The admin's name and password reach + * `mosquitto_ctrl` as an options file (`-o`), and a password a command sets reaches it at its own + * prompt, both on stdin: a small shell, given no secret, reads the two admin lines into a 0600 file + * that it removes as it exits, and hands the rest of stdin to the prompts. A password on argv is + * kept by everything that records a command line — the container runtime records every exec's in + * its event stream, which `docker events` shows to anyone who may ask the runtime — and the + * earlier `-P ` put the broker's admin password there on every call. Verified against + * mosquitto_ctrl 2.1.2: `-o` takes the connect options from a file, and `createClient` without + * `-p` and `setClientPassword` without a password prompt for it twice, reading stdin. + * `assertNoSecret` refuses, before anything runs, an argv that still carries one. + * + * The exit code cannot carry the verdict: `mosquitto_ctrl` 2.x exits 0 from its dynsec * subcommands **even when they fail** — a "Client not found", an "already exists", a rejected * "Connection error: Not authorized", an "Unable to connect" all return status 0 and report the * failure only as a line of text, on stdout or stderr (verified live against 2.0.11). Trusting the * exit code is exactly how a `createClient` the broker refused reads back as a provisioned * consumer. So the combined output is scanned for the tool's error markers and a match is raised as * the failure it is. - * - * The admin password rides on argv (`-P`): mosquitto_ctrl 2.x exposes no password env var and no - * password file for a broker connection — its only non-interactive mechanism is `-P`, its only - * other mechanism an interactive prompt. This is a real mosquitto limitation, not a choice; unlike - * psql's PGPASSWORD there is nothing cleaner to reach for. The exposure is momentary and confined - * to this single-purpose runtime container; see the module README. */ async ctl(...args: string[]): Promise { + return this.ctlPrompted([], args); + } + + /** `ctl`, answering the command's password prompt with `password` — typed twice, as asked. */ + async ctlWithPassword(password: string, ...args: string[]): Promise { + return this.ctlPrompted([password, password], args); + } + + private async ctlPrompted(prompted: string[], args: string[]): Promise { const inside = this.conn.container !== undefined; - const base = [ - "-h", inside ? "127.0.0.1" : this.conn.host, - "-p", inside ? "1883" : String(this.conn.port), - "-u", this.conn.adminUser, - "-P", this.conn.adminPassword, - ]; + const connect = ["-h", inside ? "127.0.0.1" : this.conn.host, "-p", inside ? "1883" : String(this.conn.port)]; + oneLine("the admin user", this.conn.adminUser, true); + oneLine("the admin password", this.conn.adminPassword, true); + for (const p of prompted) oneLine("a client password", p, false); + const input = [this.conn.adminUser, this.conn.adminPassword, ...prompted].map((l) => l + "\n").join(""); let stdout: string; let stderr: string; try { - const [command, argv] = this.ctrl([...base, "dynsec", ...args]); - ({ stdout, stderr } = await run(command, argv, { - maxBuffer: 16 << 20, - timeout: 30_000, - })); + const [command, argv] = this.ctrl([...connect, "dynsec", ...args]); + this.assertNoSecret([command, ...argv], prompted); + ({ stdout, stderr } = await runWithInput(command, argv, input)); } catch (err) { - // A failed run's message repeats its argv, the admin password (-P) included; say what failed - // without it. + if (err instanceof SecretOnArgvError) throw err; + // Said without the argv or the input, so a failure carries nothing it was given. const e = err as { code?: unknown; signal?: unknown; stderr?: string; stdout?: string }; const detail = `${e.stderr ?? ""}${e.stdout ?? ""}`.trim().slice(0, 500); throw new Error(`mosquitto_ctrl dynsec ${args[0] ?? ""} could not run (${e.code ?? e.signal ?? "error"}): ${detail}`); @@ -126,6 +140,20 @@ export class MosquittoClient { return stdout; } + /** + * Refuse to run a command whose argv carries a secret this client holds: the admin password, or a + * password it was asked to set (novox/hq issue 282). Said by name, never by value. + */ + assertNoSecret(argv: readonly string[], others: readonly string[] = []): void { + const secrets: Array<[string, string]> = [["the broker's admin password", this.conn.adminPassword]]; + for (const o of others) secrets.push(["a client password", o]); + for (const [name, value] of secrets) { + if (value && argv.some((a) => a.includes(value))) { + throw new SecretOnArgvError(name, argv[0] ?? ""); + } + } + } + /** Whether a dynsec client with this username already exists. */ async clientExists(username: string): Promise { try { @@ -173,14 +201,14 @@ export class MosquittoClient { const role = username; // one role per client, named for it if (await this.clientExists(username)) { - await this.ctl("setClientPassword", username, password); + await this.ctlWithPassword(password, "setClientPassword", username); // A disabled client is refused like a wrong password, so the check the provisioner runs // reports it lost; applying again must enable it, or the two would disagree for ever. if (/Disabled:\s*true/i.test(await this.ctl("getClient", username))) { await this.ctl("enableClient", username); } } else { - await this.ctl("createClient", username, "-p", password); + await this.ctlWithPassword(password, "createClient", username); } // createRole and addRoleACL are one-shot: each rejects with an "already exists" when re-run @@ -250,31 +278,71 @@ export class MosquittoClient { * Write the Dynamic Security bootstrap file offline, creating the admin client the plugin loads at * broker startup. This is a one-time seed, NOT part of the reconcile loop: run once before the * broker first starts, against the same path the broker's `plugin_opt_config_file` names. It must - * never be a host-reconciled managed file — see the module README and novox/nox issue 011. + * never be a host-reconciled managed file — see novox/nox issue 011. */ async initBootstrapFile(configFile: string): Promise { - // `dynsec init [admin-password]` is an offline file operation — it does - // not connect to the broker. The password is a positional argument (omitting it prompts). + // `dynsec init ` is an offline file operation — it does not connect to + // the broker. Without the optional password argument it prompts for the password twice, and the + // answers are given on stdin (novox/hq issue 282): an argument would be on the command line of + // the container below, which the runtime keeps in what it says about the container. + oneLine("the admin password", this.conn.adminPassword, true); + const input = `${this.conn.adminPassword}\n${this.conn.adminPassword}\n`; + let command: string; + let argv: string[]; if (this.conn.image) { // Before the broker has ever started there is no container to enter: a throwaway one from the // broker's own image writes the file into the directory the broker will mount. - await run("docker", [ - "run", "--rm", "--entrypoint", "mosquitto_ctrl", + command = "docker"; + argv = [ + "run", "--rm", "-i", "--entrypoint", "mosquitto_ctrl", "-v", `${dirname(configFile)}:/mosquitto/data`, this.conn.image, - "dynsec", "init", `/mosquitto/data/${basename(configFile)}`, this.conn.adminUser, this.conn.adminPassword, - ], { maxBuffer: 16 << 20 }); - return; + "dynsec", "init", `/mosquitto/data/${basename(configFile)}`, this.conn.adminUser, + ]; + } else { + command = "mosquitto_ctrl"; + argv = ["dynsec", "init", configFile, this.conn.adminUser]; + } + this.assertNoSecret([command, ...argv]); + try { + await runWithInput(command, argv, input); + } catch (err) { + const e = err as { code?: unknown; signal?: unknown; stderr?: string; stdout?: string }; + const detail = `${e.stderr ?? ""}${e.stdout ?? ""}`.trim().slice(0, 500); + throw new Error(`mosquitto_ctrl dynsec init could not run (${e.code ?? e.signal ?? "error"}): ${detail}`); } - await run("mosquitto_ctrl", ["dynsec", "init", configFile, this.conn.adminUser, this.conn.adminPassword], { - maxBuffer: 16 << 20, - }); } - /** How `mosquitto_ctrl` is run here: inside the broker's container when one is named. */ + /** + * Whether the broker takes this admin password: the CONNACK of a connect as the admin client — 0 + * accepted, 4 or 5 refused — or undefined when the broker cannot be reached. Nothing rides on argv. + */ + async adminAccepted(password: string): Promise { + let code: number; + try { + code = await mqttConnack(this.conn.host, this.conn.port, this.conn.adminUser, password); + } catch { + return undefined; + } + if (code === 0) return true; + if (code === 4 || code === 5) return false; + throw new Error(`mosquitto answered the admin client with CONNACK ${code}`); + } + + /** The same client, connecting with another admin password. */ + withAdminPassword(adminPassword: string): MosquittoClient { + return new MosquittoClient({ ...this.conn, adminPassword }); + } + + /** + * How `mosquitto_ctrl` is run here: inside the broker's container when one is named, through the + * shell that turns the first two lines of stdin into its options file (OPTIONS_SHELL). + */ ctrl(argv: string[]): [string, string[]] { - if (this.conn.container) return ["docker", ["exec", this.conn.container, "mosquitto_ctrl", ...argv]]; - return ["mosquitto_ctrl", argv]; + if (this.conn.container) { + return ["docker", ["exec", "-i", this.conn.container, "sh", "-c", OPTIONS_SHELL, "mosquitto_ctrl", ...argv]]; + } + return ["sh", ["-c", OPTIONS_SHELL, "mosquitto_ctrl", ...argv]]; } } @@ -283,6 +351,67 @@ export function generatePassword(): string { return randomBytes(24).toString("base64url"); } +/** + * The shell `mosquitto_ctrl` runs under: it reads the admin's name and password from the first two + * lines of stdin into an options file only it can read, removed as it exits, and runs the tool with + * `-o` naming it — so neither is ever an argument. The rest of stdin is left to the tool's own + * password prompts: `read` takes a pipe one byte at a time and so stops at the second line. The + * script holds no secret; it is the same text on every call. + */ +export const OPTIONS_SHELL = [ + "umask 077", + 'f=$(mktemp) || exit 1', + "trap 'rm -f \"$f\"' EXIT", + 'IFS= read -r u || exit 1', + 'IFS= read -r p || exit 1', + `printf '%s\\n' "-u $u" "-P $p" > "$f"`, + 'mosquitto_ctrl -o "$f" "$@"', +].join("\n"); + +/** + * Why a dynsec command would put a password on a command line, or undefined (novox/hq issue 282). + * A command line is recorded by the container runtime for anyone who may ask it; a password is set + * through the provisioner, which answers mosquitto_ctrl's prompt on stdin, never through this tool. + */ +export function passwordOnArgv(parts: readonly string[]): string | undefined { + const [verb, ...rest] = parts; + if (verb === "init") return "init writes a new store with a password; the module's bootstrap seeds the store"; + if (verb === "createClient" && rest.includes("-p")) { + return "createClient -p puts the password on a command line; a client is created by provisioning it"; + } + if (verb === "setClientPassword" && rest.length > 1) { + return "setClientPassword with a password puts it on a command line; a client's password is the mesh's to set"; + } + return undefined; +} + +/** A secret found on a command line about to run: named, never quoted. */ +export class SecretOnArgvError extends Error { + constructor(secret: string, program: string) { + super(`refused to run ${program}: its command line would carry ${secret}, and a command line is recorded ` + + `where anyone who may ask the container runtime reads it (novox/hq issue 282)`); + this.name = "SecretOnArgvError"; + } +} + +/** + * Refuse a value that cannot be handed over as one line of stdin — or, for the admin's, as one word + * of an options file, which `mosquitto_ctrl` splits at spaces. Named, never quoted. + */ +function oneLine(what: string, value: string, oneWord: boolean): void { + if (/[\r\n]/.test(value) || (oneWord && /\s/.test(value))) { + throw new Error(`${what} ${oneWord ? "holds whitespace" : "spans lines"} and cannot be handed to mosquitto_ctrl on stdin`); + } +} + +/** Run a program with this text on its stdin, answering its stdout and stderr. */ +function runWithInput(command: string, argv: string[], input: string): Promise<{ stdout: string; stderr: string }> { + const p = run(command, argv, { maxBuffer: 16 << 20, timeout: 30_000 }); + p.child.stdin?.on("error", () => { /* a program that exits before reading all of it is judged by its output */ }); + p.child.stdin?.end(input); + return p; +} + /** Escape a string for literal use inside a RegExp. */ function escapeRegExp(s: string): string { return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); @@ -339,7 +468,7 @@ function readSecretFile(path: string | undefined): string | undefined { * Connect once over MQTT 3.1.1 with a username and password, return the broker's CONNACK return code, * and disconnect. A clean session under a throwaway client id, so no consumer session is taken over. */ -function mqttConnack(host: string, port: number, username: string, password: string): Promise { +export function mqttConnack(host: string, port: number, username: string, password: string): Promise { const str = (v: string): Buffer => { const b = Buffer.from(v, "utf8"); const len = Buffer.alloc(2); diff --git a/modules/mosquitto/module.json b/modules/mosquitto/module.json index d9740da..a9d5da1 100644 --- a/modules/mosquitto/module.json +++ b/modules/mosquitto/module.json @@ -35,7 +35,10 @@ "mqtt-topic": "${dir:grants}" }, "own-secrets": { - "admin": "${dir:mesh-state}/admin" + "admin": { + "path": "${dir:mesh-state}/admin", + "taken": "at-start" + } }, "listens": [ { @@ -112,7 +115,7 @@ "type": "file", "path": "${dir:state}/bootstrap.env", "mode": "0600", - "content": "MESH_PROVISION_MQTT=127.0.0.1:${port:1883}\nMESH_PROVISION_ADMIN_USER=mesh-admin\nMESH_PROVISION_PASSWORD_FILE=${dir:mesh-state}/admin\nMESH_DYNSEC_FILE=${dir:data}/dynamic-security.json\nMESH_MQTT_CTRL_IMAGE=eclipse-mosquitto@sha256:38c0da4f2ef84284d47b3b3eeea1cb3bdeabe81ee10caf0cd5c5ff61ee3ea408\n" + "content": "MESH_PROVISION_MQTT=127.0.0.1:${port:1883}\nMESH_PROVISION_ADMIN_USER=mesh-admin\nMESH_PROVISION_PASSWORD_FILE=${dir:mesh-state}/admin\nMESH_DYNSEC_FILE=${dir:data}/dynamic-security.json\nMESH_MQTT_CTRL_IMAGE=eclipse-mosquitto@sha256:38c0da4f2ef84284d47b3b3eeea1cb3bdeabe81ee10caf0cd5c5ff61ee3ea408\nMESH_MQTT_CTRL_CONTAINER=mosquitto\nMESH_MQTT_ADMIN_APPLIED_FILE=${dir:state}/admin.applied\n" }, { "id": "bootstrap", @@ -128,7 +131,8 @@ "${dir:state}/bootstrap.env" ], "restart-on": [ - "bootstrap-env" + "bootstrap-env", + "needs-admin" ] }, { diff --git a/modules/mosquitto/test/ctrl.test.ts b/modules/mosquitto/test/ctrl.test.ts index 495c6b1..9069943 100644 --- a/modules/mosquitto/test/ctrl.test.ts +++ b/modules/mosquitto/test/ctrl.test.ts @@ -1,19 +1,87 @@ // Run after `npm run build`. // mosquitto_ctrl runs inside the broker's own container when the manifest names it, so a machine // needs no mosquitto package (whose index may be too stale to install from) and the tool always -// matches the broker's version. +// matches the broker's version. No secret is ever on its command line (novox/hq issue 282). import assert from "node:assert/strict"; +import { chmodSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { test } from "node:test"; -import { MosquittoClient } from "../dist/client.js"; // compiled: client.ts uses parameter properties, which type stripping cannot run +import { MosquittoClient, OPTIONS_SHELL, passwordOnArgv } from "../dist/client.js"; // compiled: client.ts uses parameter properties, which type stripping cannot run -const env = { MESH_PROVISION_MQTT: "127.0.0.1:21883", MESH_MQTT_PASSWORD: "pw" }; +// Made up for the test. +const ADMIN = "admin-pw-for-the-test-0123"; +const CONSUMER = "consumer-pw-for-the-test-4567"; +const env = { MESH_PROVISION_MQTT: "127.0.0.1:21883", MESH_MQTT_PASSWORD: ADMIN }; -test("named, the broker's container runs mosquitto_ctrl", () => { +test("named, the broker's container runs mosquitto_ctrl under the options shell, stdin open", () => { const c = MosquittoClient.fromEnv({ ...env, MESH_MQTT_CTRL_CONTAINER: "mosquitto" }); - assert.deepEqual(c.ctrl(["dynsec", "listClients"]), ["docker", ["exec", "mosquitto", "mosquitto_ctrl", "dynsec", "listClients"]]); + assert.deepEqual(c.ctrl(["dynsec", "listClients"]), + ["docker", ["exec", "-i", "mosquitto", "sh", "-c", OPTIONS_SHELL, "mosquitto_ctrl", "dynsec", "listClients"]]); }); -test("unnamed, this machine's mosquitto_ctrl runs", () => { +test("unnamed, this machine's mosquitto_ctrl runs under the same shell", () => { const c = MosquittoClient.fromEnv(env); - assert.deepEqual(c.ctrl(["dynsec", "listClients"]), ["mosquitto_ctrl", ["dynsec", "listClients"]]); + assert.deepEqual(c.ctrl(["dynsec", "listClients"]), ["sh", ["-c", OPTIONS_SHELL, "mosquitto_ctrl", "dynsec", "listClients"]]); +}); + +test("the options shell itself holds no secret", () => { + assert.ok(!OPTIONS_SHELL.includes(ADMIN)); + assert.match(OPTIONS_SHELL, /mosquitto_ctrl -o "\$f" "\$@"/); +}); + +test("an argv carrying the admin password or a password being set is refused, by name", () => { + const c = MosquittoClient.fromEnv(env); + assert.throws(() => c.assertNoSecret(["mosquitto_ctrl", "-P", ADMIN]), (e: Error) => + /admin password/.test(e.message) && !e.message.includes(ADMIN)); + assert.throws(() => c.assertNoSecret(["mosquitto_ctrl", "createClient", "x", "-p", CONSUMER], [CONSUMER]), (e: Error) => + /a client password/.test(e.message) && !e.message.includes(CONSUMER)); + c.assertNoSecret(["mosquitto_ctrl", "-h", "127.0.0.1", "dynsec", "listClients"], [CONSUMER]); +}); + +test("the admin tool refuses a dynsec command that would put a password on a command line", () => { + assert.ok(passwordOnArgv(["createClient", "x", "-p", "pw"])); + assert.ok(passwordOnArgv(["setClientPassword", "x", "pw"])); + assert.ok(passwordOnArgv(["init", "/f", "admin", "pw"])); + assert.equal(passwordOnArgv(["createClient", "x"]), undefined); + assert.equal(passwordOnArgv(["getClient", "x"]), undefined); +}); + +// A mosquitto_ctrl stand-in on PATH: it records its argv, the options file it was given and what is +// left on stdin, so the whole hand-over is checked without a broker. +function fakeCtrl(): { dir: string; seen: () => { argv: string; options: string; stdin: string } } { + const dir = mkdtempSync(join(tmpdir(), "mosq-ctrl-")); + const script = `#!/bin/sh +printf '%s\\n' "$@" > "${dir}/argv" +[ "$1" = "-o" ] && cat "$2" > "${dir}/options" && stat -c %a "$2" >> "${dir}/options" +cat > "${dir}/stdin" +echo ok +`; + writeFileSync(join(dir, "mosquitto_ctrl"), script); + chmodSync(join(dir, "mosquitto_ctrl"), 0o755); + return { + dir, + seen: () => ({ + argv: readFileSync(join(dir, "argv"), "utf8"), + options: readFileSync(join(dir, "options"), "utf8"), + stdin: readFileSync(join(dir, "stdin"), "utf8"), + }), + }; +} + +test("the admin's name and password reach mosquitto_ctrl in a 0600 options file, and a password being set at its prompt", async () => { + const fake = fakeCtrl(); + const path = process.env.PATH; + process.env.PATH = `${fake.dir}:${path}`; + try { + const c = MosquittoClient.fromEnv(env); + await c.ctlWithPassword(CONSUMER, "createClient", "alice"); + const seen = fake.seen(); + assert.ok(!seen.argv.includes(ADMIN) && !seen.argv.includes(CONSUMER), "a password reached argv"); + assert.equal(seen.options, `-u mesh-admin\n-P ${ADMIN}\n600\n`); + assert.equal(seen.stdin, `${CONSUMER}\n${CONSUMER}\n`); + assert.match(seen.argv, /^-o\n.+\n-h\n127\.0\.0\.1\n-p\n21883\ndynsec\ncreateClient\nalice\n$/); + } finally { + process.env.PATH = path; + } }); diff --git a/modules/mosquitto/tools/index.ts b/modules/mosquitto/tools/index.ts index bece477..f636494 100644 --- a/modules/mosquitto/tools/index.ts +++ b/modules/mosquitto/tools/index.ts @@ -2,7 +2,7 @@ // client. They return structured data; the mesh serves them through the sdk's tool harness. import { registerModuleTools, type ToolDefinition } from "@novox/mesh-sdk/tools"; -import { MosquittoClient } from "../client.js"; +import { MosquittoClient, passwordOnArgv } from "../client.js"; export function getMosquittoTools(mosquitto: MosquittoClient): ToolDefinition[] { return [ @@ -30,6 +30,8 @@ export function getMosquittoTools(mosquitto: MosquittoClient): ToolDefinition[] run: async (args) => { const parts = tokenize(String(args.command ?? "")); if (parts.length === 0) throw new Error("mqtt_ctrl: empty command"); + const why = passwordOnArgv(parts); + if (why) throw new Error(`mqtt_ctrl: ${why}`); const output = await mosquitto.ctl(...parts); return { command: parts.join(" "), output }; },