From d622fbd8b82eecaf5c2866ace1e90d30c5b4d71f Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 31 Aug 2026 17:26:46 +0000 Subject: [PATCH] fix(dns): enable rewrites the supervised bridge's unit itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last step anyone still had to run by hand: moshcode dns service --proxy-probe --write `dns enable` could detect the proxy, install it, start it and trust its root, and still leave every Moshpit name answering its origin — because what a resolver answers with is fixed when it spawns, and the bridge was already running. `startDaemon` saw a live pidfile, reported "already running", and left it alone. Correct, and the reason proxy mode arrived a reboot late. Stopping the bridge did not help either: the unit is `Restart=always`, so systemd put the same ExecStart back within the second. The only thing that changes a supervised bridge's mind is rewriting its unit and restarting it, and `enable` could not do that. It escalates, and the bridge is a *user* unit — so `systemctl --user` from there addressed root's session, which has no bridge in it and never has. The unit that had to change belongs to a session the escalated half of the command cannot see. So an escalated run now drops back to the invoking user and derives the runtime directory their session bus lives in, rather than inheriting one that points at root's. Same reason `servicePaths` now resolves the operator's home instead of `homedir()`: escalated, that was /root, so the unit was being written somewhere systemd would never look for it. `installService` also restarts rather than only `enable --now`, which starts a stopped unit and does nothing whatever to a running one — the change would have landed on disk and nowhere else. No name is invented and no handshake is attempted. `enable` knows the proxy address because it started the proxy, which is why the probe that needed a real registered name is not on this path at all. Upstreams are read back off the unit's own ExecStart rather than rediscovered. By this point the machine may already be routing every lookup at the bridge from an earlier enable, so asking the resolver for its upstreams can answer "this bridge" — a bridge whose upstream is itself resolves nothing at all. Four things fell out of it: - `disable` killed the pid and printed "bridge stopped" on a machine where systemd restarted it within the second. It now stops the unit through systemd and leaves the file, since turning resolution off for an afternoon should not delete a unit the operator may have written. - The closing note told a supervised machine the bridge does not survive a reboot. It does now, and advice that is wrong is how advice stops being read. - A rollback signalled the pid of a bridge it had not started and announced it had removed it. Both untrue, and the second one louder. - A restart that never answers is a failure. `systemctl restart` returns as soon as a Type=simple unit forks, so it returns 0 for a bridge that forked and died — and the next thing this run does is point every lookup on the machine at that port. doas publishes a user name and no uid, which would have addressed root's session silently; the uid falls back to the owner of the operator's home. Two stubs moved into the enable test harness. Left to their defaults they reach the real machine — one starts an installed proxy, the other writes /etc/systemd/resolved.conf.d and restarts systemd-resolved — so a test that forgot them either edited the box running the suite or failed on EACCES from somewhere that read as a bug in whatever it was testing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ThnQwoieWt8VR6N7gtgnhp --- src/dns-service.mjs | 177 +++++++++++++++++++++++-- src/dns.mjs | 133 +++++++++++++++++-- test/dns-enable-rollback.test.mjs | 207 ++++++++++++++++++++++++++++++ test/dns-service.test.mjs | 197 +++++++++++++++++++++++++++- 4 files changed, 687 insertions(+), 27 deletions(-) diff --git a/src/dns-service.mjs b/src/dns-service.mjs index a54c604..3c58f08 100644 --- a/src/dns-service.mjs +++ b/src/dns-service.mjs @@ -29,9 +29,8 @@ // it, and the entry is the script this very command was invoked from. Nothing // is guessed and nothing depends on PATH. import { spawn } from "node:child_process"; -import { mkdir, rm, writeFile } from "node:fs/promises"; -import { existsSync } from "node:fs"; -import { homedir } from "node:os"; +import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { existsSync, statSync } from "node:fs"; import { dirname, join } from "node:path"; import { operatorHome } from "./trust.mjs"; @@ -48,10 +47,49 @@ export const UNIT_NAME = "moshcode-dns.service"; * /run/user//moshpit-dns.pid for the daemon and for the person asking * after it. Under a system unit those are two different paths. */ -export function servicePaths({ system = false, home = homedir() } = {}) { +export function servicePaths({ system = false, home = operatorHome(), env = process.env } = {}) { return system ? { path: join("/etc/systemd/system", UNIT_NAME), systemctl: ["systemctl"], scope: "system" } - : { path: join(home, ".config/systemd/user", UNIT_NAME), systemctl: ["systemctl", "--user"], scope: "user" }; + : { path: join(home, ".config/systemd/user", UNIT_NAME), systemctl: userSystemctl(env), scope: "user" }; +} + +/** + * How to reach the operator's own systemd from wherever this is running. + * + * `systemctl --user` talks to the session of whoever is running it. `dns enable` + * escalates, so from there it is root's session — which has no bridge in it, has + * never had one, and reports every query about one as "not loaded". Meanwhile + * the operator's bridge keeps running with whatever it started with. + * + * That is why enabling proxy mode could be detected, written, and still not take + * effect: the unit that had to change belongs to a session the escalated half of + * the command cannot see. + * + * So an escalated run drops back to the invoking user, and hands them the runtime + * directory their session bus lives in — deriving it rather than inheriting it, + * because sudo does not carry XDG_RUNTIME_DIR across and the default under sudo + * points at root's. + */ +export function userSystemctl(env = process.env, { home = operatorHome({ env }), owner = ownerOf } = {}) { + const user = env.SUDO_USER || env.DOAS_USER; + // Not escalated, or escalated from root itself: the session in reach is the + // right one. + if (!user || user === "root") return ["systemctl", "--user"]; + // sudo publishes the uid; doas publishes only the name. Falling back to the + // owner of the operator's home covers that, and covers an escalator that + // publishes neither — without it, a doas machine would quietly address root's + // session, which has no bridge in it and never will. + const uid = env.SUDO_UID || env.DOAS_UID || owner(home); + if (uid === null || uid === undefined) return ["systemctl", "--user"]; + return ["sudo", "-u", user, "env", `XDG_RUNTIME_DIR=/run/user/${uid}`, "systemctl", "--user"]; +} + +function ownerOf(path) { + try { + return statSync(path).uid; + } catch { + return null; + } } /** @@ -127,6 +165,8 @@ export function serviceUnit({ return lines.join("\n"); } +const defaultRead = async (path) => readFile(path, "utf8").catch(() => ""); + function run(command, args) { return new Promise((resolve) => { const child = spawn(command, args, { stdio: ["ignore", "pipe", "pipe"] }); @@ -140,8 +180,8 @@ function run(command, args) { } /** Write the unit and start it. Returns the steps taken, in order, for printing. */ -export async function installService(unit, { system = false, home = homedir(), exec = run } = {}) { - const { path, systemctl, scope } = servicePaths({ system, home }); +export async function installService(unit, { system = false, home = operatorHome(), exec = run, env = process.env } = {}) { + const { path, systemctl, scope } = servicePaths({ system, home, env }); const steps = []; try { await mkdir(dirname(path), { recursive: true }); @@ -152,7 +192,14 @@ export async function installService(unit, { system = false, home = homedir(), e } const [cmd, ...flags] = systemctl; - for (const args of [[...flags, "daemon-reload"], [...flags, "enable", "--now", UNIT_NAME]]) { + // Enable *and* restart. `enable --now` starts a stopped unit and does nothing + // to a running one, so rewriting the unit to add `--proxy` would leave the old + // bridge running without it — the change on disk, no change in behaviour. + for (const args of [ + [...flags, "daemon-reload"], + [...flags, "enable", UNIT_NAME], + [...flags, "restart", UNIT_NAME], + ]) { const result = await exec(cmd, args); steps.push({ step: `${cmd} ${args.join(" ")}`, ok: result.ok, error: result.error }); if (!result.ok) return { ok: false, path, scope, steps }; @@ -160,9 +207,119 @@ export async function installService(unit, { system = false, home = homedir(), e return { ok: true, path, scope, steps }; } +/** + * The `--upstream` servers an installed unit already forwards to. + * + * Read back rather than recomputed. A supervised bridge needs upstreams to hand + * the clearnet to, and the machine may already be routing every lookup at that + * bridge — so asking the system resolver what its upstreams are can answer + * "this bridge", and a bridge whose upstream is itself resolves nothing at all. + * Whatever the unit was working with is the safe answer to keep. + */ +export function unitUpstreams(text) { + const line = String(text ?? "").split("\n").find((l) => l.startsWith("ExecStart=")); + if (!line) return []; + const parts = line.trim().split(/\s+/); + const found = []; + for (let i = 0; i < parts.length; i += 1) { + if (parts[i] !== "--upstream") continue; + const value = parts[i + 1]; + if (!value || value.startsWith("--")) continue; + if (!found.includes(value)) found.push(value); + } + return found; +} + +/** + * Re-describe the bridge unit to match the run happening now, and restart it so + * the description becomes the truth. + * + * This is the step that made proxy mode arrive one reboot late. A supervised + * bridge is not started by `enable`: it is already up under `Restart=always`, + * so `startDaemon` finds a live pidfile and reports "already running" — true, + * and useless, because what a resolver answers with is fixed when it spawns. A + * bridge that came up before the proxy existed goes on answering origins + * forever, and stopping it by hand does not help, since systemd brings the same + * ExecStart straight back. + * + * The only thing that changes a supervised bridge's mind is rewriting its unit + * and restarting it. That is all this is. + * + * With no unit installed it does nothing and says so. An unsupervised machine + * is `startDaemon`'s business, and writing a unit here would be `enable` + * quietly making the bridge outlive a reboot on a machine that never asked for + * that — a different decision, and one `dns service --write` exists to make. + */ +export async function refreshService({ + entry, + port, + registryBase = null, + proxy = null, + system = false, + home = operatorHome(), + env = process.env, + exec = run, + exists = existsSync, + read = defaultRead, +} = {}) { + const { path, scope } = servicePaths({ system, home, env }); + if (!exists(path)) return { refreshed: false, reason: "no unit installed", path, scope, upstreams: [], steps: [] }; + + const current = await read(path); + const upstreams = unitUpstreams(current); + const unit = serviceUnit({ system, entry, port, registryBase, upstreams, proxy }); + + // Deliberately not short-circuited on `current === unit`. Matching text says + // the unit describes the right bridge, not that the bridge is running it: a + // unit can be installed and stopped, installed and never enabled, or running + // what it was spawned with before the file last changed. Since the whole + // point here is to make what is running match what is written, the enable and + // restart happen either way, and cost a moment of no resolver during a + // command that is already rewriting the machine's routing. + + const result = await installService(unit, { system, home, env, exec }); + return { + refreshed: result.ok, + reason: result.ok ? null : "systemctl refused the unit", + path, + scope, + upstreams, + steps: result.steps, + }; +} + +/** + * Stop the supervised bridge and leave the unit where it is. + * + * `stopDaemon` cannot do this. It reads the pidfile and signals that process, + * which is right for a bridge started by hand and useless for one systemd owns: + * the unit is `Restart=always`, so the pid dies and the same ExecStart is back + * within the second. `disable` printed "bridge stopped" and left a bridge + * running — on a machine whose routing had just been put back, so the bridge + * was still up, still answering, and no longer on anybody's path. + * + * The unit file stays. Removing it is a different decision than turning + * resolution off for an afternoon, and `enable` re-enables what it finds — so + * leaving it costs nothing and deleting it would quietly take away a unit the + * operator may have written themselves. + */ +export async function stopService({ system = false, home = operatorHome(), env = process.env, exec = run, exists = existsSync } = {}) { + const { path, systemctl, scope } = servicePaths({ system, home, env }); + if (!exists(path)) return { stopped: false, reason: "no unit installed", path, scope, steps: [] }; + const [cmd, ...flags] = systemctl; + const result = await exec(cmd, [...flags, "disable", "--now", UNIT_NAME]); + return { + stopped: result.ok, + reason: result.ok ? null : (result.error || "systemctl refused"), + path, + scope, + steps: [{ step: `${cmd} ${flags.join(" ")} disable --now ${UNIT_NAME}`, ok: result.ok, error: result.error }], + }; +} + /** Stop it and take the unit away. Missing is not a failure — removal is idempotent. */ -export async function removeService({ system = false, home = homedir(), exec = run } = {}) { - const { path, systemctl, scope } = servicePaths({ system, home }); +export async function removeService({ system = false, home = operatorHome(), exec = run, env = process.env } = {}) { + const { path, systemctl, scope } = servicePaths({ system, home, env }); const [cmd, ...flags] = systemctl; const steps = []; for (const args of [[...flags, "disable", "--now", UNIT_NAME]]) { diff --git a/src/dns.mjs b/src/dns.mjs index 82bc4bf..c2098a5 100644 --- a/src/dns.mjs +++ b/src/dns.mjs @@ -2148,6 +2148,59 @@ export async function verifyResolution({ return { ok: checks.every((c) => c.ok), checks }; } +/** + * Wait for a bridge that systemd has just restarted to start answering. + * + * `startDaemon` cannot be asked this. It spawns, watches its own child, and + * decides from a pidfile — none of which describes a unit that systemd owns and + * has just cycled. Calling it here would either report the pre-restart pid as + * "already running" or, on a pidfile not yet rewritten, spawn a second bridge + * against the one systemd is bringing up. + * + * `Type=simple` reports active the moment the process forks, so systemd saying + * the restart worked is not yet a resolver that answers. Hence the probe. + * + * A timeout is reported as started-but-unverified rather than as a failure, the + * same way `startDaemon` treats a live process that has not answered yet: the + * unit is active, and refusing this machine its DNS over a slow first registry + * fetch would be the worse mistake. + */ +export async function supervisedReady({ + host = DEFAULT_HOST, + port, + probe = probeResolver, + status = daemonStatus, + timeoutMs = READY_TIMEOUT_MS, + sleep = (ms) => new Promise((r) => setTimeout(r, ms)), +} = {}) { + const deadline = Date.now() + timeoutMs; + let answered = false; + while (Date.now() < deadline) { + if (await probe({ host, port }).catch(() => false)) { + answered = true; + break; + } + await sleep(150); + } + const current = await Promise.resolve(status()).catch(() => null); + if (!answered) { + // Reported as a failure, unlike `startDaemon`'s slow-but-alive case, and + // for a reason that does not apply there: that one has watched its own + // child and knows it is running. Nothing here has. `systemctl restart` + // returns as soon as a Type=simple unit forks, so it returns 0 for a bridge + // that forked and died — and the next thing this run does is point every + // lookup on the machine at that port. Refusing is the safe direction. + return { + started: false, + alreadyRunning: false, + pid: current?.pid ?? null, + supervised: true, + error: `${UNIT_NAME} restarted but the bridge did not answer on ${host}:${port}`, + }; + } + return { started: true, pid: current?.pid ?? null, alreadyRunning: false, supervised: true, verified: true }; +} + const defaultReadMaybe = async (path) => { const { readFile: rf } = await import("node:fs/promises"); return rf(path, "utf8").catch(() => null); @@ -2407,10 +2460,10 @@ import { readFile, writeFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { isRealTld } from "./iana-tlds.mjs"; -import { installService, removeService, serviceUnit, servicePaths, UNIT_NAME, ensureProxyService, removeProxyService, proxyServicePaths, proxyWrapperPath } from "./dns-service.mjs"; +import { installService, refreshService, removeService, serviceUnit, servicePaths, stopService, UNIT_NAME, ensureProxyService, removeProxyService, proxyServicePaths, proxyWrapperPath } from "./dns-service.mjs"; import { applyPlan, daemonStatus, describePlan, detectPlatform, disablePlan, enablePlan, - probeResolver, requiredPort, startDaemon, stopDaemon, + probeResolver, READY_TIMEOUT_MS, requiredPort, startDaemon, stopDaemon, } from "./dns-system.mjs"; import { escalateSelf } from "./escalate.mjs"; @@ -2508,10 +2561,13 @@ export async function dnsCommand(args = [], out = console.log, deps = {}) { presenceImpl = bridgePresence, exists = existsSync, startBridge = startDaemon, + refreshBridge = refreshService, + bridgeReady = supervisedReady, proxyReachableImpl = proxyReachable, findLocalProxyImpl = findLocalProxy, autoTrustImpl = createAutoTrust, stopBridge = stopDaemon, + stopSupervised = stopService, // The two proxy-service calls, injected for the same reason as every // other system call here: a test must be able to exercise the branch // without shelling out to systemctl or writing to /etc. @@ -3224,6 +3280,19 @@ export async function dnsCommand(args = [], out = console.log, deps = {}) { return 1; } + // The supervised bridge first, and by asking systemd rather than by + // signalling a pid. `stopDaemon` kills what the pidfile names, and the + // unit is `Restart=always` — so the process died, systemd replaced it + // within the second, and this command reported "bridge stopped" on a + // machine where the bridge was still up and answering, just no longer on + // anything's path. + const unsupervised = await stopSupervised(); + if (unsupervised.reason !== "no unit installed") { + out(unsupervised.stopped + ? ` ok ${UNIT_NAME} stopped and disabled` + : ` -- could not stop ${UNIT_NAME} (${unsupervised.reason}) — it will restart itself`); + } + const stopped = await stopBridge(); out(stopped.stopped ? " ok bridge stopped" : ` ok bridge was not running${stopped.reason ? ` (${stopped.reason})` : ""}`); @@ -3412,17 +3481,42 @@ export async function dnsCommand(args = [], out = console.log, deps = {}) { } } + // A bridge under systemd is re-described, not started — and this is the step + // whose absence left the last of this to be done by hand. + // + // `startBridge` below reports "already running" for a supervised bridge and + // leaves it alone. That is correct, and it is also why proxy mode arrived a + // reboot late: what a resolver answers with is fixed when it spawns, so a + // bridge that came up before the proxy existed goes on answering origins no + // matter what this run decides. Stopping it does not help either, since + // `Restart=always` brings the same ExecStart back. + // + // So the unit is rewritten to match the run happening now, and restarted. + // v4 by preference: `dns start --proxy` takes one address and probes both + // families itself, so handing it the v4 loopback lets it find ::1 too + // rather than pinning the answer to one family. + const proxyArg = proxyAddress ? (proxyAddress.v4 || proxyAddress.v6) : null; + + let refreshed = null; + if (!reusing && platform === "linux") { + refreshed = await refreshBridge({ entry: cliEntry(), port: wanted, registryBase, proxy: proxyArg }); + for (const step of refreshed.steps || []) { + out(` ${step.ok ? "ok " : "-- "} ${step.step}${step.error ? ` — ${step.error}` : ""}`); + } + if (refreshed.refreshed) { + const forwarding = refreshed.upstreams?.length ? `, forwarding the clearnet to ${refreshed.upstreams.join(", ")}` : ""; + out(` ok ${UNIT_NAME} restarted with proxy mode ${proxyArg ? "on" : "off"}${forwarding}`); + } else if (refreshed.reason !== "no unit installed") { + out(` -- could not update ${UNIT_NAME} (${refreshed.reason})`); + out(" the bridge already running keeps the mode it started with"); + } + } + const started = reusing ? { started: false, pid: reusing.pid, alreadyRunning: true, reused: true } - : await startBridge({ - port: wanted, - registryBase, - entry: cliEntry(), - // v4 by preference: `dns start --proxy` takes one address and probes - // both families itself, so handing it the v4 loopback lets it find ::1 - // too rather than pinning the answer to one family. - proxy: proxyAddress ? (proxyAddress.v4 || proxyAddress.v6) : null, - }); + : refreshed?.refreshed + ? await bridgeReady({ host: DEFAULT_HOST, port: wanted }) + : await startBridge({ port: wanted, registryBase, entry: cliEntry(), proxy: proxyArg }); // The routing this is about to install is catch-all — every lookup on the // machine, not just Moshpit ones — so a bridge that did not come up is not // a degraded feature, it is the machine's resolver pointed at nothing. @@ -3504,7 +3598,13 @@ export async function dnsCommand(args = [], out = console.log, deps = {}) { out(`Moshpit names now resolve on this machine. Try: moshcode dns resolve ${moshpitProbe || ""}`); out(`Routing covers the ${tlds.length} TLDs claimed right now. New ones do not route`); out("until you re-run this — there is no common suffix to match, so every TLD is listed."); - out("Note: the bridge does not yet survive a reboot. Re-run `moshcode dns enable` after one."); + // Only where it is still true. On a supervised machine the unit was just + // enabled and restarted, so the bridge does come back — and telling + // someone to re-run a command they do not need is how advice stops being + // read at all. + out(started.supervised + ? `Note: ${UNIT_NAME} brings the bridge back after a reboot.` + : "Note: the bridge does not yet survive a reboot. Re-run `moshcode dns enable` after one."); return 0; } @@ -3512,7 +3612,14 @@ export async function dnsCommand(args = [], out = console.log, deps = {}) { report(outcome.rolledBack.results); // Started by this run and no longer routed to, so leaving it would be a // process holding 5354 that the next enable's preflight refuses to run past. - if (started.started) { + // + // A supervised bridge is exempt: this run did not start it, only restarted + // it, so it was holding that port before the run and is meant to go on + // holding it. Signalling its pid would not stop it anyway — `Restart=always` + // replaces it within the second — so the only thing the old line achieved + // there was printing "remove bridge started by this run" about a bridge + // that was neither started by this run nor removed. + if (started.started && !started.supervised) { const stopped = await stopBridge(); if (stopped.stopped) out(" ok remove bridge started by this run"); } diff --git a/test/dns-enable-rollback.test.mjs b/test/dns-enable-rollback.test.mjs index b436547..c70770b 100644 --- a/test/dns-enable-rollback.test.mjs +++ b/test/dns-enable-rollback.test.mjs @@ -463,6 +463,20 @@ function noSystem() { // whatever holds 443 on the machine running the suite. findLocalProxyImpl: async () => ({ found: false, why: null, address: { v4: null, v6: null } }), startBridge: async () => ({ started: true, pid: 1, alreadyRunning: false }), + // No systemd unit on this machine, which is the state these tests were + // written in. Stubbed rather than left to the real one, which would read + // the suite runner's own ~/.config/systemd/user and, on a developer's box, + // restart their actual bridge. + refreshBridge: async () => ({ refreshed: false, reason: "no unit installed", upstreams: [], steps: [] }), + stopSupervised: async () => ({ stopped: false, reason: "no unit installed", steps: [] }), + // Both of these reach the real machine when left to their defaults: one + // looks for an installed proxy wrapper and starts it, the other writes + // /etc/systemd/resolved.conf.d and restarts systemd-resolved. A test that + // forgets to stub them does not fail cleanly — it either edits the system + // running the suite or, unprivileged, fails on EACCES from somewhere that + // reads as a bug in whatever it was actually testing. + proxyWrapper: () => null, + applyWith: async () => ({ saved: { ok: true }, applied: { ok: true, results: [] }, verified: { ok: true, checks: [] }, rolledBack: null, backups: [] }), stopBridge: async () => ({ stopped: true, reason: null }), dropins: async () => [], readManifest: async () => null, @@ -538,3 +552,196 @@ test("with no proxy installed, nothing is claimed and DNS still comes up", async assert.equal(startedWith?.proxy, null); assert.match(lines.join("\n"), /no pinned-TLS proxy installed/); }); + +/* ------------------- a supervised bridge is re-described, not left alone ---*/ + +// The last thing that had to be done by hand. `startBridge` reports "already +// running" for a bridge systemd owns and leaves it alone — correct, and the +// reason proxy mode arrived a reboot late: what a resolver answers with is +// fixed when it spawns, so a bridge that came up before the proxy existed goes +// on answering origins whatever this run decides. Stopping it does not help +// either, because `Restart=always` brings the same ExecStart back. + +test("an installed unit is rewritten with proxy mode and restarted", async () => { + const lines = []; + let asked = null; + let startedByHand = false; + const code = await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + proxyWrapper: () => "/home/x/.local/bin/moshpit-proxy", + ensureProxy: async () => ({ ok: true, steps: [] }), + refreshBridge: async (opts) => { + asked = opts; + return { refreshed: true, reason: null, upstreams: ["1.1.1.1"], steps: [{ step: "systemctl --user restart moshcode-dns.service", ok: true }] }; + }, + bridgeReady: async () => ({ started: true, pid: 99, alreadyRunning: false, supervised: true, verified: true }), + startBridge: async () => { startedByHand = true; return { started: true, pid: 1, alreadyRunning: false }; }, + }); + + assert.equal(code, 0); + assert.equal(asked?.proxy, "127.0.0.1", "the unit has to carry --proxy, or the bridge still answers origins"); + assert.equal(startedByHand, false, "systemd owns this bridge — starting a second one is how two bridges end up on the port"); + assert.match(lines.join("\n"), /restarted with proxy mode on/); + assert.match(lines.join("\n"), /forwarding the clearnet to 1\.1\.1\.1/); +}); + +test("with no unit installed, nothing is said about one and the daemon is started", async () => { + const lines = []; + let startedByHand = false; + const code = await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + startBridge: async () => { startedByHand = true; return { started: true, pid: 1, alreadyRunning: false }; }, + }); + + assert.equal(code, 0); + assert.equal(startedByHand, true); + assert.doesNotMatch(lines.join("\n"), /could not update/, "an unsupervised machine is not a failure to report"); +}); + +test("a unit that restarts but never answers refuses the machine its routing", async () => { + // `systemctl restart` returns as soon as a Type=simple unit forks, so it + // returns 0 for a bridge that forked and died. The next thing enable does is + // point every lookup on the machine at that port. + const lines = []; + let applied = false; + const code = await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + refreshBridge: async () => ({ refreshed: true, reason: null, upstreams: [], steps: [] }), + bridgeReady: async () => ({ started: false, alreadyRunning: false, pid: null, supervised: true, error: "moshcode-dns.service restarted but the bridge did not answer on 127.0.0.1:5354" }), + applyWith: async () => { applied = true; return { ok: true, steps: [] }; }, + }); + + assert.equal(code, 1); + assert.equal(applied, false, "nothing may be written when the bridge is not answering"); + assert.match(lines.join("\n"), /did not answer/); + assert.match(lines.join("\n"), /Nothing has been changed/); +}); + +test("a unit that cannot be updated is reported, and the run continues", async () => { + // A refusal from systemctl is worth naming — proxy mode will not take effect + // — but it is not a reason to leave the machine without DNS. + const lines = []; + let startedByHand = false; + const code = await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + refreshBridge: async () => ({ refreshed: false, reason: "systemctl refused the unit", upstreams: [], steps: [{ step: "systemctl --user restart moshcode-dns.service", ok: false, error: "Interactive authentication required" }] }), + startBridge: async () => { startedByHand = true; return { started: true, pid: 1, alreadyRunning: false }; }, + }); + + assert.equal(code, 0); + assert.equal(startedByHand, true); + assert.match(lines.join("\n"), /could not update moshcode-dns\.service/); + assert.match(lines.join("\n"), /keeps the mode it started with/); +}); + +/* ------------------ turning it off has to stop a bridge systemd is holding -*/ + +test("disable stops the unit through systemd, not by signalling a pid", async () => { + // `stopDaemon` kills what the pidfile names, and the unit is Restart=always: + // the process dies and systemd has the same ExecStart back within the second. + // So disable printed "bridge stopped" on a machine where the bridge was still + // up and answering — just no longer on anything's path. + const lines = []; + let askedSystemd = false; + const code = await dnsCommand(["disable"], (l) => lines.push(String(l)), { + ...noSystem(), + dropins: async () => [{ name: "moshpit.conf", content: "[Resolve]\nDNS=127.0.0.1:5354\nDomains=~.\n" }], + stopSupervised: async () => { askedSystemd = true; return { stopped: true, reason: null, steps: [] }; }, + }); + + assert.equal(code, 0); + assert.equal(askedSystemd, true, "killing the pid alone leaves a bridge systemd will restart"); + assert.match(lines.join("\n"), /moshcode-dns\.service stopped and disabled/); +}); + +test("disable says nothing about a unit on a machine that has none", async () => { + const lines = []; + const code = await dnsCommand(["disable"], (l) => lines.push(String(l)), { + ...noSystem(), + dropins: async () => [{ name: "moshpit.conf", content: "[Resolve]\nDNS=127.0.0.1:5354\nDomains=~.\n" }], + }); + + assert.equal(code, 0); + assert.doesNotMatch(lines.join("\n"), /moshcode-dns\.service/); +}); + +test("a unit that will not stop is named, because it is about to come back", async () => { + const lines = []; + const code = await dnsCommand(["disable"], (l) => lines.push(String(l)), { + ...noSystem(), + dropins: async () => [{ name: "moshpit.conf", content: "[Resolve]\nDNS=127.0.0.1:5354\nDomains=~.\n" }], + stopSupervised: async () => ({ stopped: false, reason: "Interactive authentication required", steps: [] }), + }); + + assert.equal(code, 0, "a bridge that will not stop is not a reason to leave the routing in place"); + assert.match(lines.join("\n"), /could not stop moshcode-dns\.service/); + assert.match(lines.join("\n"), /it will restart itself/); +}); + +test("a supervised machine is not told to re-run enable after a reboot", async () => { + // The unit was just enabled and restarted, so the bridge does come back. + // Telling someone to re-run a command they do not need is how advice stops + // being read at all. + const lines = []; + await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + refreshBridge: async () => ({ refreshed: true, reason: null, upstreams: [], steps: [] }), + bridgeReady: async () => ({ started: true, pid: 99, alreadyRunning: false, supervised: true, verified: true }), + }); + + const out = lines.join("\n"); + assert.match(out, /moshcode-dns\.service brings the bridge back after a reboot/); + assert.doesNotMatch(out, /does not yet survive a reboot/); +}); + +test("an unsupervised machine is still told the bridge dies at reboot", async () => { + const lines = []; + await dnsCommand(["enable"], (l) => lines.push(String(l)), { ...noSystem() }); + assert.match(lines.join("\n"), /does not yet survive a reboot/); +}); + +test("a rollback leaves a supervised bridge where it found it", async () => { + // It was holding the port before this run and is meant to go on holding it. + // Signalling its pid would not stop it anyway — Restart=always replaces it + // within the second — so the old line printed "remove bridge started by this + // run" about a bridge that was neither started by this run nor removed. + const lines = []; + let killed = false; + const code = await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + refreshBridge: async () => ({ refreshed: true, reason: null, upstreams: [], steps: [] }), + bridgeReady: async () => ({ started: true, pid: 99, alreadyRunning: false, supervised: true, verified: true }), + applyWith: async () => ({ + saved: { ok: true }, + applied: { ok: true, results: [] }, + verified: { ok: false, checks: [{ name: "pit.moshcode.sh", kind: "clearnet", ok: false, error: "ENOTFOUND" }] }, + rolledBack: { ok: true, results: [] }, + backups: [], + }), + stopBridge: async () => { killed = true; return { stopped: true, reason: null }; }, + }); + + assert.equal(code, 1); + assert.equal(killed, false, "systemd would put it straight back, so the kill is noise that reads as a fact"); + assert.doesNotMatch(lines.join("\n"), /remove bridge started by this run/); +}); + +test("a rollback still cleans up a bridge this run really did start", async () => { + const lines = []; + let killed = false; + await dnsCommand(["enable"], (l) => lines.push(String(l)), { + ...noSystem(), + startBridge: async () => ({ started: true, pid: 7, alreadyRunning: false }), + applyWith: async () => ({ + saved: { ok: true }, + applied: { ok: true, results: [] }, + verified: { ok: false, checks: [{ name: "pit.moshcode.sh", kind: "clearnet", ok: false, error: "ENOTFOUND" }] }, + rolledBack: { ok: true, results: [] }, + backups: [], + }), + stopBridge: async () => { killed = true; return { stopped: true, reason: null }; }, + }); + + assert.equal(killed, true, "an unsupervised bridge left on 5354 is what the next preflight refuses to run past"); + assert.match(lines.join("\n"), /remove bridge started by this run/); +}); diff --git a/test/dns-service.test.mjs b/test/dns-service.test.mjs index f016fea..5fded55 100644 --- a/test/dns-service.test.mjs +++ b/test/dns-service.test.mjs @@ -19,12 +19,12 @@ // that looks plausible and dies at 203/EXEC with nothing useful in the journal. import test from "node:test"; import assert from "node:assert/strict"; -import { mkdtemp, mkdir, writeFile } from "node:fs/promises"; +import { mkdtemp, mkdir, readFile, writeFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; -import { installService, removeService, serviceUnit, servicePaths, UNIT_NAME } from "../src/dns-service.mjs"; +import { installService, refreshService, removeService, serviceUnit, servicePaths, stopService, unitUpstreams, userSystemctl, UNIT_NAME } from "../src/dns-service.mjs"; import { proxyProbeFromArgs, captureRestorePoint, probeName } from "../src/dns.mjs"; import { proxyServiceUnit, proxyServicePaths, PROXY_UNIT_NAME } from "../src/dns-service.mjs"; import { rootIsNarrow, ensureProxyService } from "../src/dns-service.mjs"; @@ -89,11 +89,28 @@ test("install writes the unit, then reloads and enables — and stops at the fir const result = await installService(unit(), { home, exec }); assert.equal(result.ok, false); - assert.deepEqual(calls, ["systemctl --user daemon-reload", `systemctl --user enable --now ${UNIT_NAME}`]); + assert.deepEqual(calls, ["systemctl --user daemon-reload", `systemctl --user enable ${UNIT_NAME}`]); assert.equal(existsSync(join(home, ".config/systemd/user", UNIT_NAME)), true, "the file is written before systemctl is asked about it"); assert.equal(result.steps.at(-1).error, "Failed to enable", "the reason systemctl gave is carried back, not swallowed"); }); +test("install restarts, so a rewritten unit actually takes effect", async () => { + // `enable --now` starts a stopped unit and does nothing at all to a running + // one. That is why rewriting the unit to add `--proxy` used to change the + // file and nothing else: the bridge kept running with the arguments it was + // spawned with, and proxy mode arrived on the next reboot. + const home = await scratch(); + const calls = []; + const exec = async (cmd, args) => { + calls.push(args.join(" ")); + return { ok: true }; + }; + + const result = await installService(unit(), { home, exec }); + assert.equal(result.ok, true); + assert.deepEqual(calls, ["--user daemon-reload", `--user enable ${UNIT_NAME}`, `--user restart ${UNIT_NAME}`]); +}); + test("remove takes the unit away even when it was never enabled", async () => { const home = await scratch(); // `disable` on a unit systemd has never heard of is an error, and removal @@ -388,3 +405,175 @@ test("--proxy-probe wins, for a machine that needs a specific real name", () => assert.equal(probeName(["hacker"], ["--proxy-probe"]), "a.hacker"); assert.equal(probeName(["hacker"], ["--proxy-probe", "--write"]), "a.hacker"); }); + +/* --------------------------------- reaching the operator's own systemd ---- */ + +// `systemctl --user` talks to the session of whoever runs it, and `dns enable` +// escalates. From there it is root's session: no bridge in it, none ever, and +// every question about one answered "not loaded" — while the operator's bridge +// keeps running with whatever it was spawned with. That is why the unit could +// be rewritten and still not take effect. + +test("an escalated run drives the invoking user's systemd, not root's", () => { + assert.deepEqual( + userSystemctl({ SUDO_USER: "ettinger", SUDO_UID: "1000" }), + ["sudo", "-u", "ettinger", "env", "XDG_RUNTIME_DIR=/run/user/1000", "systemctl", "--user"], + ); +}); + +test("an unescalated run uses the session it is already in", () => { + assert.deepEqual(userSystemctl({}), ["systemctl", "--user"]); +}); + +test("root running this directly is not sent back through sudo to itself", () => { + // SUDO_USER=root happens with `sudo -u root`, and also survives in the + // environment of anything root spawns afterwards. + assert.deepEqual(userSystemctl({ SUDO_USER: "root", SUDO_UID: "0" }), ["systemctl", "--user"]); +}); + +test("doas publishes a name and no uid, so the uid comes off the operator's home", () => { + // sudo sets SUDO_UID; doas sets DOAS_USER alone. Without this the doas case + // addresses root's session — which has no bridge in it and never will — and + // does it silently. + assert.deepEqual( + userSystemctl({ DOAS_USER: "ettinger" }, { home: "/home/ettinger", owner: () => 1000 }), + ["sudo", "-u", "ettinger", "env", "XDG_RUNTIME_DIR=/run/user/1000", "systemctl", "--user"], + ); +}); + +test("an unresolvable uid falls back rather than building a command with 'null' in it", () => { + assert.deepEqual( + userSystemctl({ SUDO_USER: "ettinger" }, { home: "/home/gone", owner: () => null }), + ["systemctl", "--user"], + ); +}); + +/* ------------------------------------- upstreams are kept, not recomputed - */ + +test("the upstreams already in a unit are read back off its ExecStart", () => { + const text = [ + "[Service]", + "ExecStart=/usr/bin/node /home/x/bin/moshcode.mjs dns start --port 5354 --upstream 1.1.1.1 --upstream 8.8.8.8#53 --proxy 127.0.0.1", + ].join("\n"); + assert.deepEqual(unitUpstreams(text), ["1.1.1.1", "8.8.8.8#53"]); +}); + +test("a unit with no upstreams, and a malformed one, read as none rather than throwing", () => { + assert.deepEqual(unitUpstreams("[Service]\nExecStart=/usr/bin/node x dns start --port 5354"), []); + assert.deepEqual(unitUpstreams("ExecStart=/usr/bin/node x dns start --upstream --proxy 127.0.0.1"), [], "a flag is not an upstream"); + assert.deepEqual(unitUpstreams(""), []); + assert.deepEqual(unitUpstreams(null), []); +}); + +/* ------------------------------------------------- refreshing the unit ---- */ + +test("with no unit installed, refresh does nothing and says which", async () => { + const home = await scratch(); + let ran = false; + const result = await refreshService({ + entry: "/home/x/bin/moshcode.mjs", port: 5354, home, + exec: async () => { ran = true; return { ok: true }; }, + }); + assert.equal(result.refreshed, false); + assert.equal(result.reason, "no unit installed"); + assert.equal(ran, false, "an unsupervised machine is startDaemon's business"); +}); + +test("refresh keeps the unit's upstreams and adds the proxy this run found", async () => { + const home = await scratch(); + const unitPath = join(home, ".config/systemd/user", UNIT_NAME); + await mkdir(dirname(unitPath), { recursive: true }); + await writeFile(unitPath, [ + "[Service]", + "ExecStart=/usr/bin/node /home/x/bin/moshcode.mjs dns start --port 5354 --upstream 9.9.9.9", + "", + ].join("\n")); + + const calls = []; + const result = await refreshService({ + entry: "/home/x/bin/moshcode.mjs", port: 5354, registryBase: "https://pit.moshcode.sh", proxy: "127.0.0.1", + home, exec: async (cmd, args) => { calls.push(args.join(" ")); return { ok: true }; }, + }); + + assert.equal(result.refreshed, true); + assert.deepEqual(result.upstreams, ["9.9.9.9"]); + + const written = await readFile(unitPath, "utf8"); + assert.match(written, /--proxy 127\.0\.0\.1/); + assert.match(written, /--upstream 9\.9\.9\.9/, "dropping the upstreams would leave the bridge with nothing to forward the clearnet to"); + assert.deepEqual(calls, ["--user daemon-reload", `--user enable ${UNIT_NAME}`, `--user restart ${UNIT_NAME}`]); +}); + +test("an unchanged unit is still enabled and restarted", async () => { + // Matching text says the unit describes the right bridge, not that the bridge + // is running it: installed-and-stopped and running-something-older are both + // real states, and both look identical to a text comparison. + const home = await scratch(); + const unitPath = join(home, ".config/systemd/user", UNIT_NAME); + await mkdir(dirname(unitPath), { recursive: true }); + const settled = serviceUnit({ entry: "/home/x/bin/moshcode.mjs", port: 5354, proxy: "127.0.0.1" }); + await writeFile(unitPath, settled); + + const calls = []; + const result = await refreshService({ + entry: "/home/x/bin/moshcode.mjs", port: 5354, proxy: "127.0.0.1", + home, exec: async (cmd, args) => { calls.push(args.join(" ")); return { ok: true }; }, + }); + + assert.equal(result.refreshed, true); + assert.ok(calls.includes(`--user restart ${UNIT_NAME}`)); +}); + +test("systemctl refusing is carried back rather than reported as success", async () => { + const home = await scratch(); + const unitPath = join(home, ".config/systemd/user", UNIT_NAME); + await mkdir(dirname(unitPath), { recursive: true }); + await writeFile(unitPath, "[Service]\nExecStart=/usr/bin/node x dns start --port 5354\n"); + + const result = await refreshService({ + entry: "/home/x/bin/moshcode.mjs", port: 5354, home, + exec: async (cmd, args) => (args.includes("restart") + ? { ok: false, error: "Interactive authentication required" } + : { ok: true }), + }); + + assert.equal(result.refreshed, false); + assert.equal(result.reason, "systemctl refused the unit"); + assert.equal(result.steps.at(-1).error, "Interactive authentication required"); +}); + +/* -------------------------------------------------- stopping, not removing -*/ + +test("stop disables the unit now and leaves the file alone", async () => { + const home = await scratch(); + const unitPath = join(home, ".config/systemd/user", UNIT_NAME); + await mkdir(dirname(unitPath), { recursive: true }); + await writeFile(unitPath, "[Service]\nExecStart=/usr/bin/node x dns start --port 5354\n"); + + const calls = []; + const result = await stopService({ home, exec: async (cmd, args) => { calls.push(args.join(" ")); return { ok: true }; } }); + + assert.equal(result.stopped, true); + assert.deepEqual(calls, [`--user disable --now ${UNIT_NAME}`], "--now, or the unit stays running until reboot"); + assert.equal(existsSync(unitPath), true, "turning resolution off for an afternoon must not delete the unit"); +}); + +test("stop on a machine with no unit asks systemd nothing", async () => { + const home = await scratch(); + let ran = false; + const result = await stopService({ home, exec: async () => { ran = true; return { ok: true }; } }); + assert.equal(result.stopped, false); + assert.equal(result.reason, "no unit installed"); + assert.equal(ran, false); +}); + +test("a refusal from systemctl comes back with the reason it gave", async () => { + const home = await scratch(); + const unitPath = join(home, ".config/systemd/user", UNIT_NAME); + await mkdir(dirname(unitPath), { recursive: true }); + await writeFile(unitPath, "[Service]\n"); + + const result = await stopService({ home, exec: async () => ({ ok: false, error: "Failed to disable unit" }) }); + assert.equal(result.stopped, false); + assert.equal(result.reason, "Failed to disable unit"); +});