From 7fa50f24d2a082c7d0af94ef519af10313b16904 Mon Sep 17 00:00:00 2001 From: Logan Cusano Date: Sun, 4 Oct 2026 01:09:52 -0400 Subject: [PATCH] approve_node never rotates an existing key; node update-check endpoint; real Update Node button (node-26#14, #15) - approve_node: reuses node_keys/{id}.api_key when one exists (still re-syncs dynsec with it, still never overwrites node_keys). Re-clicking Approve can no longer lock a live node out. Reissue stays explicit. - POST /nodes/{id}/request-update (admin): sets update_requested_at on the node doc AND sends MQTT node_update best-effort, so offline/locked-out nodes still get it. - POST /nodes/{id}/update-check (node, inline auth: Bearer api_key for THAT node or X-Pickup-Secret): host updater's HTTPS check-in. Returns only update_requested_at; records updater_last_check_at / sha / applied result. Unknown node -> 401 (no node_id oracle). C2 cannot name a ref, image or command. - Frontend: "Update Node Software" (admin only) calls request-update, honest confirm text; node page shows updater check-in and last update state. Tests: tests/test_node_provisioning.py (12). Co-Authored-By: Claude Opus 5.5 --- drb-c2-core/app/routers/enrollment.py | 85 +++++++++- drb-c2-core/app/routers/nodes.py | 44 ++++- drb-c2-core/tests/test_node_provisioning.py | 171 ++++++++++++++++++++ drb-frontend/app/nodes/[id]/page.tsx | 55 +++++-- drb-frontend/lib/c2api.ts | 6 + drb-frontend/lib/types.ts | 7 + 6 files changed, 352 insertions(+), 16 deletions(-) create mode 100644 drb-c2-core/tests/test_node_provisioning.py diff --git a/drb-c2-core/app/routers/enrollment.py b/drb-c2-core/app/routers/enrollment.py index 18976c9..ab70cfc 100644 --- a/drb-c2-core/app/routers/enrollment.py +++ b/drb-c2-core/app/routers/enrollment.py @@ -17,11 +17,13 @@ Auth is the token headers checked inline below, not the app-wide Firebase/service-key dependency the rest of routers/nodes.py uses. """ import hashlib +import re import secrets import time -from typing import Optional +from datetime import datetime, timezone +from typing import Literal, Optional from fastapi import APIRouter, HTTPException, Header, Request -from pydantic import BaseModel +from pydantic import BaseModel, Field from app.config import settings from app.internal import firestore as fstore from app.internal.logger import logger @@ -216,3 +218,82 @@ async def get_node_credentials(node_id: str, x_pickup_secret: Optional[str] = He return CredentialsResponse(approval_status=approval_status) return CredentialsResponse(approval_status=approval_status, api_key=key_doc["api_key"]) + + +# --------------------------------------------------------------------------- +# Host updater check-in (node-26#15) +# +# A node's host-level updater (Client/scripts/drb-update.sh, a systemd timer +# installed by install.sh) calls this over HTTPS on a timer. It is the +# out-of-band channel: it works for a node that is offline from MQTT or +# locked out of the broker entirely, which is exactly when an update or a +# credential fix is needed most. +# +# Auth, checked inline for THIS node_id only (never a fleet credential): +# Authorization: Bearer -> must equal node_keys/{id}.api_key +# X-Pickup-Secret: -> must hash to nodes/{id}.pickup_secret_hash +# The pickup secret is accepted so a node whose api_key is missing/stale can +# still be told to update — it is the same credential that already returns +# the api_key itself, so it grants nothing new. +# +# The response is a single timestamp. The node compares it with the last +# request it applied and, if newer, pulls the repo/registry/ref IT was +# installed with. C2 cannot name a ref, an image or a command here, so a +# compromised C2 or a stolen node credential can at most make a node +# re-pull its own pinned software. +# --------------------------------------------------------------------------- +_SHA_RE = re.compile(r"^[0-9a-f]{7,40}$") + + +class UpdateCheckRequest(BaseModel): + sha: Optional[str] = Field(None, max_length=40) + applied_request_at: Optional[str] = Field(None, max_length=64) + result: Optional[Literal["ok", "failed"]] = None + + +class UpdateCheckResponse(BaseModel): + update_requested_at: Optional[str] = None + + +async def _authenticate_node( + node_id: str, authorization: Optional[str], x_pickup_secret: Optional[str], +) -> dict: + node = await fstore.doc_get("nodes", node_id) + if not node: + # 401, not 404: this route must not double as a node_id oracle. + raise HTTPException(401, "Invalid node credentials") + if authorization and authorization.lower().startswith("bearer "): + presented = authorization[7:].strip() + key_doc = await fstore.doc_get("node_keys", node_id) + stored = (key_doc or {}).get("api_key") or "" + if stored and presented and secrets.compare_digest(stored, presented): + return node + if x_pickup_secret and node.get("pickup_secret_hash"): + if secrets.compare_digest(_hash_secret(x_pickup_secret), node["pickup_secret_hash"]): + return node + raise HTTPException(401, "Invalid node credentials") + + +@router.post("/{node_id}/update-check", response_model=UpdateCheckResponse) +async def node_update_check( + node_id: str, + body: UpdateCheckRequest, + authorization: Optional[str] = Header(None), + x_pickup_secret: Optional[str] = Header(None), +): + node = await _authenticate_node(node_id, authorization, x_pickup_secret) + + now = datetime.now(timezone.utc).isoformat() + updates: dict = {"updater_last_check_at": now} + if body.sha and _SHA_RE.match(body.sha): + updates["updater_sha"] = body.sha + requested = node.get("update_requested_at") + if body.applied_request_at and requested and body.applied_request_at == requested and body.result: + updates["update_applied_at"] = now + updates["update_result"] = body.result + try: + await fstore.doc_update("nodes", node_id, updates) + except Exception as e: # bookkeeping only — never fail the node's check on it + logger.warning(f"update-check: could not record check-in for {node_id!r}: {e}") + + return UpdateCheckResponse(update_requested_at=requested) diff --git a/drb-c2-core/app/routers/nodes.py b/drb-c2-core/app/routers/nodes.py index c24e5d8..cfc8018 100644 --- a/drb-c2-core/app/routers/nodes.py +++ b/drb-c2-core/app/routers/nodes.py @@ -1,4 +1,5 @@ import secrets +from datetime import datetime, timezone from typing import Dict, List, Optional from fastapi import APIRouter, HTTPException, Depends, Query from pydantic import BaseModel @@ -43,7 +44,17 @@ async def approve_node(node_id: str, _: dict = Depends(require_admin_token)): if not node: raise HTTPException(404, f"Node '{node_id}' not found.") - api_key = secrets.token_hex(32) + # node-26#14: approve must NEVER rotate a key that already exists. The old + # behaviour minted a fresh key on every click and overwrote node_keys, so + # re-clicking Approve on a live node (or approving after a re-run) silently + # invalidated the key already on the device and locked it out of MQTT with + # no remote recovery. Rotation is an explicit, separate action + # (POST /nodes/{id}/reissue-key). An existing key is reused as-is; the + # dynsec upsert below still runs, so a second approve doubles as a safe + # "re-sync this node's broker login" without changing the credential. + key_doc = await fstore.doc_get("node_keys", node_id) + existing_key = (key_doc or {}).get("api_key") + api_key = existing_key or secrets.token_hex(32) # dynsec FIRST, Firestore second: if the broker rejects/never confirms # the new client, we must not tell Firestore (and the admin UI) the @@ -56,13 +67,40 @@ async def approve_node(node_id: str, _: dict = Depends(require_admin_token)): logger.error(f"Approve {node_id!r}: dynsec upsert failed, NOT writing Firestore: {e}") raise HTTPException(502, f"Could not provision MQTT credentials for node: {e}") - await fstore.doc_set("node_keys", node_id, {"node_id": node_id, "api_key": api_key}, merge=False) + if not existing_key: + await fstore.doc_set("node_keys", node_id, {"node_id": node_id, "api_key": api_key}, merge=False) await fstore.doc_update("nodes", node_id, {"approval_status": "approved"}) # TODO(mqtt-cutover): drop this MQTT push once nodes pull their key via # GET /nodes/{id}/credentials (routers/enrollment.py) exclusively — see # MQTT-PUBLIC-AUTH-PLAN.md "Rollout order" step 6. Kept for node-26. mqtt_handler.publish_node_key(node_id, api_key) - return {"ok": True} + if existing_key: + logger.info(f"Approve {node_id!r}: key already exists — reused, not rotated") + return {"ok": True, "key_rotated": False, "already_had_key": bool(existing_key)} + + +@router.post("/{node_id}/request-update") +async def request_node_update(node_id: str, _: dict = Depends(require_admin_token)): + """Ask a node to run its host-level updater (node-26#15). + + Two channels, so a node that is offline or locked out of MQTT still gets + it: (1) ``update_requested_at`` on the node doc, which the node's host + updater reads over HTTPS on its timer (POST /nodes/{id}/update-check in + routers/enrollment.py); (2) the MQTT ``node_update`` command, best-effort, + which makes an online node's updater run within seconds. + + Deliberately carries NO payload the node acts on beyond "update now": no + ref, no image name, no command. The node pulls only the repo/registry and + ref it was installed with — C2 cannot steer what code a node runs. + """ + node = await fstore.doc_get("nodes", node_id) + if not node: + raise HTTPException(404, f"Node '{node_id}' not found.") + requested_at = datetime.now(timezone.utc).isoformat() + await fstore.doc_update("nodes", node_id, {"update_requested_at": requested_at}) + mqtt_sent = mqtt_handler.send_command(node_id, {"action": "node_update"}) + logger.info(f"Update requested for {node_id!r} at {requested_at} (mqtt_sent={mqtt_sent})") + return {"ok": True, "update_requested_at": requested_at, "mqtt_sent": mqtt_sent} @router.delete("/{node_id}", status_code=204) diff --git a/drb-c2-core/tests/test_node_provisioning.py b/drb-c2-core/tests/test_node_provisioning.py new file mode 100644 index 0000000..d4de945 --- /dev/null +++ b/drb-c2-core/tests/test_node_provisioning.py @@ -0,0 +1,171 @@ +""" +node-26#14 / #15 — approve must not rotate an existing key, and the host +updater's HTTPS channel (request-update + update-check). +""" +from unittest.mock import AsyncMock, MagicMock, patch + +from fastapi.testclient import TestClient + +from app.main import app +from app.internal.auth import require_admin_token, require_service_or_firebase_token +from app.routers import enrollment, nodes + +client = TestClient(app) + + +def setup_function(): + app.dependency_overrides[require_admin_token] = lambda: {"admin": True} + app.dependency_overrides[require_service_or_firebase_token] = lambda: {"admin": True} + + +def teardown_function(): + app.dependency_overrides.pop(require_admin_token, None) + app.dependency_overrides.pop(require_service_or_firebase_token, None) + + +def _store(node, key_doc): + return AsyncMock(side_effect=lambda c, i: {"nodes": node, "node_keys": key_doc}.get(c)) + + +# --------------------------------------------------------------------------- +# approve_node +# --------------------------------------------------------------------------- + +def _approve(node, key_doc): + with patch.object(nodes.fstore, "doc_get", _store(node, key_doc)), \ + patch.object(nodes.fstore, "doc_set", AsyncMock()) as doc_set, \ + patch.object(nodes.fstore, "doc_update", AsyncMock()) as doc_update, \ + patch.object(nodes.dynsec, "upsert_node_client", AsyncMock()) as upsert, \ + patch.object(nodes.mqtt_handler, "publish_node_key", MagicMock()) as publish: + resp = client.post("/nodes/n1/approve") + return resp, doc_set, doc_update, upsert, publish + + +def test_approve_already_keyed_node_reuses_key_and_never_overwrites_node_keys(): + node = {"node_id": "n1", "approval_status": "approved"} + resp, doc_set, doc_update, upsert, publish = _approve(node, {"node_id": "n1", "api_key": "live-key"}) + assert resp.status_code == 200 + assert resp.json()["key_rotated"] is False + doc_set.assert_not_awaited() + upsert.assert_awaited_once_with("n1", "live-key") + publish.assert_called_once_with("n1", "live-key") + + +def test_approve_new_node_mints_a_key(): + resp, doc_set, doc_update, upsert, publish = _approve({"node_id": "n1", "approval_status": "pending"}, None) + assert resp.status_code == 200 + (_, _, written), kwargs = doc_set.await_args + assert len(written["api_key"]) == 64 + assert kwargs == {"merge": False} + upsert.assert_awaited_once_with("n1", written["api_key"]) + doc_update.assert_awaited_once_with("nodes", "n1", {"approval_status": "approved"}) + + +def test_approve_twice_keeps_the_same_key(): + store = {"nodes": {"node_id": "n1", "approval_status": "pending"}, "node_keys": None} + + async def doc_get(c, i): + return store[c] + + async def doc_set(c, i, data, merge=False): + store[c] = data + + with patch.object(nodes.fstore, "doc_get", doc_get), \ + patch.object(nodes.fstore, "doc_set", doc_set), \ + patch.object(nodes.fstore, "doc_update", AsyncMock()), \ + patch.object(nodes.dynsec, "upsert_node_client", AsyncMock()), \ + patch.object(nodes.mqtt_handler, "publish_node_key", MagicMock()): + client.post("/nodes/n1/approve") + first = store["node_keys"]["api_key"] + client.post("/nodes/n1/approve") + assert store["node_keys"]["api_key"] == first + + +# --------------------------------------------------------------------------- +# request-update (admin) +# --------------------------------------------------------------------------- + +def test_request_update_sets_flag_even_when_mqtt_is_down(): + with patch.object(nodes.fstore, "doc_get", _store({"node_id": "n1"}, None)), \ + patch.object(nodes.fstore, "doc_update", AsyncMock()) as doc_update, \ + patch.object(nodes.mqtt_handler, "send_command", MagicMock(return_value=False)) as cmd: + resp = client.post("/nodes/n1/request-update") + assert resp.status_code == 200 + body = resp.json() + assert body["mqtt_sent"] is False + doc_update.assert_awaited_once_with("nodes", "n1", {"update_requested_at": body["update_requested_at"]}) + cmd.assert_called_once_with("n1", {"action": "node_update"}) + + +def test_request_update_requires_admin(): + app.dependency_overrides.pop(require_admin_token, None) + resp = client.post("/nodes/n1/request-update") + assert resp.status_code in (401, 403) + + +# --------------------------------------------------------------------------- +# update-check (node, HTTPS) +# --------------------------------------------------------------------------- + +NODE = { + "node_id": "n1", + "pickup_secret_hash": enrollment._hash_secret("pickup-1"), + "update_requested_at": "2026-10-04T12:00:00+00:00", +} + + +def _check(headers, body=None, node=NODE, key_doc=None): + key_doc = key_doc if key_doc is not None else {"api_key": "k" * 64} + # A node holds no Firebase/service credential: prove the route needs none. + app.dependency_overrides.clear() + with patch.object(enrollment.fstore, "doc_get", _store(node, key_doc)), \ + patch.object(enrollment.fstore, "doc_update", AsyncMock()) as doc_update: + resp = client.post("/nodes/n1/update-check", json=body or {}, headers=headers) + return resp, doc_update + + +def test_update_check_with_api_key_returns_request(): + resp, doc_update = _check({"Authorization": "Bearer " + "k" * 64}, {"sha": "abc1234"}) + assert resp.status_code == 200 + assert resp.json() == {"update_requested_at": NODE["update_requested_at"]} + (_, _, updates), _ = doc_update.await_args + assert updates["updater_sha"] == "abc1234" + assert "update_applied_at" not in updates + + +def test_update_check_with_pickup_secret_works_for_a_keyless_node(): + resp, _ = _check({"X-Pickup-Secret": "pickup-1"}, key_doc={}) + assert resp.status_code == 200 + + +def test_update_check_rejects_wrong_or_missing_credentials(): + assert _check({"Authorization": "Bearer wrong"})[0].status_code == 401 + assert _check({"X-Pickup-Secret": "wrong"})[0].status_code == 401 + assert _check({})[0].status_code == 401 + + +def test_update_check_does_not_reveal_unknown_node_ids(): + resp, _ = _check({"X-Pickup-Secret": "pickup-1"}, node=None) + assert resp.status_code == 401 + + +def test_update_check_empty_stored_key_never_matches_empty_bearer(): + resp, _ = _check({"Authorization": "Bearer "}, key_doc={"api_key": ""}) + assert resp.status_code == 401 + + +def test_update_check_records_applied_request(): + resp, doc_update = _check( + {"Authorization": "Bearer " + "k" * 64}, + {"applied_request_at": NODE["update_requested_at"], "result": "ok", "sha": "not-a-sha!"}, + ) + assert resp.status_code == 200 + (_, _, updates), _ = doc_update.await_args + assert updates["update_result"] == "ok" + assert "update_applied_at" in updates + assert "updater_sha" not in updates # junk sha is never written + + +def test_update_check_rejects_unknown_result_value(): + resp, _ = _check({"Authorization": "Bearer " + "k" * 64}, {"result": "rm -rf /"}) + assert resp.status_code == 422 diff --git a/drb-frontend/app/nodes/[id]/page.tsx b/drb-frontend/app/nodes/[id]/page.tsx index a884a03..9c6a785 100644 --- a/drb-frontend/app/nodes/[id]/page.tsx +++ b/drb-frontend/app/nodes/[id]/page.tsx @@ -146,6 +146,32 @@ export default function NodeDetailPage() { } } + async function handleRequestUpdate() { + if ( + !confirm( + "Update this node's software?\n\n" + + "The node pulls the latest release it was installed to track and recreates its containers " + + "(radio capture pauses for about a minute). An online node starts within seconds; an offline " + + "or locked-out node picks it up on its next check-in (up to ~15 minutes). Nodes installed " + + "before the host updater existed need install.sh re-run once.", + ) + ) + return; + setSending(true); + try { + const r = await c2api.requestNodeUpdate(id); + alert( + r.mqtt_sent + ? "Update requested. The node should restart within a minute." + : "Update requested. The node is not reachable over MQTT right now; it will update on its next HTTPS check-in.", + ); + } catch (err) { + alert(err instanceof Error ? err.message : "Update request failed."); + } finally { + setSending(false); + } + } + async function handleApprove() { setApproving(true); try { @@ -251,6 +277,15 @@ export default function NodeDetailPage() { ["System", system?.name ?? "Unassigned"], ["Location", `${node.lat}, ${node.lon}`], ["Last Seen", node.last_seen ? new Date(node.last_seen).toLocaleString() : "never"], + ["Updater Check-in", node.updater_last_check_at ? new Date(node.updater_last_check_at).toLocaleString() : "never"], + ...(node.update_requested_at + ? [[ + "Last Update", + node.update_applied_at && node.update_applied_at >= node.update_requested_at + ? `${node.update_result === "failed" ? "FAILED" : "applied"} ${new Date(node.update_applied_at).toLocaleString()}` + : `requested ${new Date(node.update_requested_at).toLocaleString()} — pending`, + ]] + : []), ["Configured", node.configured ? "Yes" : "No"], ["Node Type", node.node_type ?? "fixed"], ...(node.node_type !== "portable" ? [["Enforce Timeout", node.enforce_override_timeout ? "Yes" : "No"]] : []), @@ -302,17 +337,15 @@ export default function NodeDetailPage() { > Restart OP25 - + {isAdmin && ( + + )}