Merge pull request 'approve_node never rotates an existing key; node update-check endpoint; real Update Node button (node-26#14, #15)' (#194) from fix/node-14-15-update-endpoint into main

Merge #194: approve reuses existing key; node update-check + request-update endpoints (node-26#14, #15)

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 <api_key>   -> must equal node_keys/{id}.api_key
+#   X-Pickup-Secret: <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)

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}")
 
+    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)

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

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
         </button>
+        {isAdmin && (
           <button
             disabled={sending}
-          onClick={() => {
+            onClick={handleRequestUpdate}
-            if (confirm("Restart the node container to apply any pulled image updates?")) {
-              sendCommand("node_update");
-            }
-          }}
             className="px-4 py-2 bg-gray-800 hover:bg-gray-700 text-gray-300 rounded-lg text-sm font-mono transition-colors disabled:opacity-50"
           >
-          Update Node
+            Update Node Software
           </button>
+        )}
         <button
           onClick={() => setShowDiscordJoin(true)}
           className="px-4 py-2 bg-gray-800 hover:bg-gray-700 text-gray-300 rounded-lg text-sm font-mono transition-colors"

drb-frontend/lib/c2api.ts

@@ -65,6 +65,12 @@ export const c2api = {
   // Node approval
   approveNode: (id: string) =>
     request(`/nodes/${id}/approve`, { method: "POST" }),
+  /** node-26#15: flag the node for its host updater (HTTPS) and nudge it over MQTT. */
+  requestNodeUpdate: (id: string) =>
+    request<{ ok: boolean; update_requested_at: string; mqtt_sent: boolean }>(
+      `/nodes/${id}/request-update`,
+      { method: "POST" },
+    ),
   rejectNode: (id: string) =>
     request(`/nodes/${id}/reject`, { method: "POST" }),
   deleteNode: (id: string) =>

drb-frontend/lib/types.ts

@@ -72,6 +72,13 @@ export interface NodeRecord {
   is_overridden?: boolean;
   override_system_id?: string | null;
   override_timeout_at?: string | null;
+  /** node-26#15 host updater: last "Update Node" request, and what the node did with it. */
+  update_requested_at?: string | null;
+  update_applied_at?: string | null;
+  update_result?: "ok" | "failed" | null;
+  /** Last HTTPS check-in from the host updater — alive even when MQTT is not. */
+  updater_last_check_at?: string | null;
+  updater_sha?: string | null;
 }
 
 export interface SdrDevice {