fix(review): close the three gaps found in stack review #23

Closed
maximus wants to merge 1 commit from review-fixes into issue-15-local-agent
4 changed files with 71 additions and 9 deletions
Showing only changes of commit e181a9691c - Show all commits

View file

@ -16,8 +16,14 @@ const path = require("node:path");
const TOKEN = "test-token";
// Every route reachable by the handler. Any new route must be added here.
const ROUTES = ["/health", "/defenseurs", "/defenseurs/findings", "/reports/scans"];
// Every GET route reachable by the handler. Any new route must be added here.
// POST /hosts/<id> is not a GET, so it gets its own describe block below —
// but it is covered, and any future route must be too.
const ROUTES = ["/health", "/defenseurs", "/defenseurs/findings", "/reports/scans", "/hosts"];
// Ingest token, distinct from the read token on purpose: a read token must not
// open the write path, and a write token must not open the read routes.
const INGEST_TOKEN = "test-ingest-token";
let tmpDir;
let server;
@ -67,6 +73,11 @@ beforeEach(async () => {
process.env.REPORTS_DIR = path.join(tmpDir, "reports");
process.env.DEFENSEURS_AGENTS_MAP_PATH = path.join(tmpDir, "agents-map.json");
process.env.DEFENSEURS_STATUS_PATH = path.join(tmpDir, "status.json");
process.env.HOSTS_DIR = path.join(tmpDir, "hosts");
process.env.HOSTS_ALLOWED_IDS = "thinkpad";
// Configured on purpose: an unset ingest token answers 503 before the header
// is ever read, which would hide the 401 these tests are here to pin.
process.env.HOSTS_INGEST_TOKEN = INGEST_TOKEN;
// Closed port: /health must never reach the real IdP. If the auth gate ever
// fails open, the request errors out fast instead of hitting production.
process.env.LOGTO_HEALTH_URL = "http://127.0.0.1:1/oidc/.well-known/openid-configuration";
@ -152,3 +163,39 @@ describe("routing", () => {
expect(body.error).toBe("Not found");
});
});
// The ingest path is the only write surface on this service, and the only one
// reachable with a token that is NOT the read token. Both directions of that
// separation are pinned here: a read token must not write, a write token must
// not read. The gate answers before the request body is ever buffered, so an
// unauthenticated caller cannot make the server hold 4 KiB on its behalf.
describe("auth gate — ingest route", () => {
test("401 on POST /hosts/<id> with no Authorization header", async () => {
const { status, body } = await request("/hosts/thinkpad", { method: "POST", auth: null });
expect(status).toBe(401);
expect(body.error).toBe("Unauthorized");
});
test("401 on POST /hosts/<id> with the read token", async () => {
const { status, body } = await request("/hosts/thinkpad", {
method: "POST",
auth: `Bearer ${TOKEN}`,
});
expect(status).toBe(401);
expect(body.error).toBe("Unauthorized");
});
test("401 on GET /hosts with the ingest token", async () => {
const { status, body } = await request("/hosts", { auth: `Bearer ${INGEST_TOKEN}` });
expect(status).toBe(401);
expect(body.error).toBe("Unauthorized");
});
// An unknown host id never reaches the handler: the route regex is the
// path-traversal control, so a malformed id is a 404, not a 403 — and that
// 404 arrives before authentication, like every other routing decision.
test("404 (not 401, not 403) on POST /hosts/../../etc/passwd", async () => {
const { status } = await request("/hosts/../../etc/passwd", { method: "POST", auth: null });
expect(status).toBe(404);
});
});

View file

@ -164,8 +164,13 @@ async function ingestOversized(route, body) {
try {
const { status } = await ingest(route, { body });
return status;
} catch {
return 413;
} catch (err) {
// A dead socket is NOT a 413. The whole client contract on this path is
// "you get a status code, not a transport error" — so surface the failure
// instead of folding it into the expected value. Returning 413 here would
// keep these tests green even if the server stopped writing the status and
// merely destroyed the connection.
return `transport error instead of a status: ${err.cause?.code || err.code || err.message}`;
}
}

View file

@ -67,16 +67,23 @@ value is the most common install failure.
HOSTS_API_URL=https://health.lacompagniemaximus.com
HOSTS_INGEST_TOKEN=<the ingest token>
HOST_ID=thinkpad
# NODE_BIN=/usr/bin/node # only if cron cannot find node (see below)
# NODE_BIN=/usr/bin/node # only if cron cannot find node (see below)
# HOST_AGENT_TIMEOUT_MS=10000 # push timeout, wall-clock; default 10000
```
```
chmod 600 ~/.config/maximus-host-agent.env
```
`HOSTS_INGEST_TOKEN` is the write-only token: it can push snapshots and nothing
else. It is deliberately *not* `HEALTH_TOKEN` — a stolen workstation must not
grant read access to the Defenseurs reports.
`HOSTS_INGEST_TOKEN` is the write-only token: it can push snapshots for an
allowlisted id and nothing else. It is deliberately *not* `HEALTH_TOKEN`, so
that the two can be rotated independently and so a workstation that only pushes
never needs the read token.
Do not read more into that separation than it gives you. On the ThinkPad it
buys no containment at all: that machine already stores `HEALTH_TOKEN` in
cleartext for the `defenseur-auto` cron, so losing the laptop compromises both
tokens. **Rotate them together** — see `secret-rotation-ops.md`.
`HOST_ID` must match `^[a-z0-9][a-z0-9-]{0,31}$` and be listed in the server's
`HOSTS_ALLOWED_IDS`, otherwise the push comes back `403`.

View file

@ -62,6 +62,9 @@ if ! command -v "$NODE_BIN" >/dev/null 2>&1; then
exit 1
fi
export HOSTS_API_URL HOSTS_INGEST_TOKEN HOST_ID
# HOST_AGENT_TIMEOUT_MS is optional and read by push-metrics.js. It must be
# exported too, otherwise setting it in the env file silently does nothing and
# the agent always runs on its 10s default.
export HOSTS_API_URL HOSTS_INGEST_TOKEN HOST_ID HOST_AGENT_TIMEOUT_MS
exec "$NODE_BIN" "$AGENT" "$@"