From f60c9147d5f1ba95b4115f5b68bbb8bd8cf55e13 Mon Sep 17 00:00:00 2001 From: Josh Poole Date: Mon, 21 Sep 2026 14:58:36 +0100 Subject: [PATCH] 20260921 - Split "may I touch this" from "is this mine" before widening either NODE_ID_RE answered two different questions with one constant, and they fail in opposite directions. _guard() asks whether a name is one this script may create, reconfigure or delete. Too narrow there is safe: the worst case is refusing to act. reconcile() asks, in three places, whether a name is ours and therefore rubbish to sweep up. Too wide there is dangerous: an over-broad pattern claims something somebody built by hand, and --prune deletes it. retnode.com carries eight hand-built tunnels, several serving live customer nodes. Sharing one constant meant widening the node_id format for the first purpose silently widened it for the second. MAY_ACT_ON and OWNED_BY_US are identical today and deliberately separate, so the next format change has to answer both questions rather than one. Both now accept retg + 15 hex alongside ret + 8 hex. Checked before widening: nothing in the zone begins retg, across 27 tunnels, 43 DNS records and 25 Access applications, and the eight hand-built tunnel names match neither format. Tests pin that property rather than describing it: every one of the eight real names is asserted against OWNED_BY_US, and reconcile is driven end to end so a hand-built tunnel is never offered as an orphan from any of the three sweeps. The constant being right is not the same as it being used in all three. Verified against the live account: this script and the one on main produce byte-identical dry-run output, so the widening changes nothing today. Co-Authored-By: Claude Opus 5 (1M context) --- mender-auto-accept/node-id-test-vectors.json | 91 ++++++++++++++++++++ mender-auto-accept/test_tunnel_sync.py | 82 ++++++++++++++++++ mender-auto-accept/tunnel_sync.py | 45 +++++++--- 3 files changed, 208 insertions(+), 10 deletions(-) create mode 100644 mender-auto-accept/node-id-test-vectors.json diff --git a/mender-auto-accept/node-id-test-vectors.json b/mender-auto-accept/node-id-test-vectors.json new file mode 100644 index 0000000..fda2613 --- /dev/null +++ b/mender-auto-accept/node-id-test-vectors.json @@ -0,0 +1,91 @@ +{ + "_comment": [ + "Canonical test vectors for the node_id derivation. Vendored into every", + "repo that validates or derives a node_id, so the four implementations", + "are checked against one artefact rather than against each other.", + "", + "Generated by owl-os, which owns configuration/mender/identity/", + "mender-device-identity, the single implementation of the rule.", + "", + "Serials here are synthetic. Real board serials are hardware identifiers", + "and are not committed to any repo." + ], + "family_tag": "rpi-cpuinfo-serial", + "construction": "retg + first 15 hex chars of SHA-256(\":\")", + "pattern": "^ret(?:[0-9a-f]{8}|g[0-9a-f]{15})$", + "derive": [ + { + "serial": "0000000000000001", + "node_id": "retg3d02b56e8dc8eed", + "why": "minimum plausible serial" + }, + { + "serial": "00633d6e00000000", + "node_id": "retg99f4028bee349d8", + "why": "trailing zeros, so the old rule would have produced ret00000000" + }, + { + "serial": "123456789abcdef0", + "node_id": "retg0e0998b3b40e91c", + "why": "every hex digit" + }, + { + "serial": "ffffffffffffffff", + "node_id": "retg15804102b4e1a4c", + "why": "all ones" + }, + { + "serial": "a1b2c3d4e5f60718", + "node_id": "retgdfe16b5d4d22957", + "why": "ordinary" + } + ], + "valid": [ + { + "node_id": "ret7dd2cb0d", + "why": "legacy format, still carried by unmigrated nodes" + }, + { + "node_id": "retgec420d03ea4b064", + "why": "current format" + } + ], + "invalid": [ + { + "node_id": "Unknown", + "why": "retina-gui's get_node_id() returns this on failure" + }, + { + "node_id": "ret000000000", + "why": "the placeholder in retina-node/config/default.yml" + }, + { + "node_id": "retg", + "why": "prefix alone" + }, + { + "node_id": "retgec420d03ea4b06", + "why": "one hex char short of the current format" + }, + { + "node_id": "retgec420d03ea4b0644", + "why": "one hex char too long" + }, + { + "node_id": "retGEC420D03EA4B064", + "why": "uppercase" + }, + { + "node_id": "retg ec420d03ea4b064", + "why": "embedded space" + }, + { + "node_id": "ret1a2b3c4d5", + "why": "between the two formats, matching neither" + }, + { + "node_id": "mac=b8:27:eb:00:11:22", + "why": "the non-Pi fallback is not a node_id" + } + ] +} diff --git a/mender-auto-accept/test_tunnel_sync.py b/mender-auto-accept/test_tunnel_sync.py index c9ed095..d70431d 100644 --- a/mender-auto-accept/test_tunnel_sync.py +++ b/mender-auto-accept/test_tunnel_sync.py @@ -99,3 +99,85 @@ def test_the_final_rule_is_a_catch_all(): def test_another_node_is_not_served_by_this_tunnel(): for rule in tunnel_sync.build_ingress(NODE): assert rule.get("hostname") in (HOST, None) + + +# ── which names this script may touch, and which it owns ───────────── +# +# Two questions, one shape, opposite failure modes. They used to share a +# constant, so widening the node id format for one purpose silently widened it +# for the other — and the other decides what --prune deletes. + +#: Every tunnel on retnode.com as of 2026-09-21 that this script did not create. +#: Several serve live customer nodes. If any of these ever matches, --prune +#: deletes somebody's working tunnel. +HAND_BUILT = [ + "fairforest", + "jonathan-node-1", + "jonathan-node-2", + "joshOffice", + "mississippi", + "nightcrawler", + "sacremento", + "wilderness", +] + +LEGACY_ID = "ret4c844c20" +CURRENT_ID = "retgec420d03ea4b064" + + +@pytest.mark.parametrize("node_id", [LEGACY_ID, CURRENT_ID]) +def test_both_node_id_formats_may_be_acted_on(node_id): + """The fleet is migrated one node at a time, so both are live at once.""" + tunnel_sync._guard(node_id) + + +@pytest.mark.parametrize( + "name", + HAND_BUILT + ["", None, "retg", "ret000000000", "retgec420d03ea4b06", "Unknown"], +) +def test_guard_refuses_anything_that_is_not_a_node_id(name): + with pytest.raises(RuntimeError, match="refusing to act"): + tunnel_sync._guard(name) + + +@pytest.mark.parametrize("name", HAND_BUILT) +def test_hand_built_tunnels_are_not_ours_to_sweep(name): + """The property the orphan sweep rests on. Too narrow costs a lingering + orphan; too wide deletes a production tunnel.""" + assert not tunnel_sync.OWNED_BY_US.match(name) + + +@pytest.mark.parametrize("node_id", [LEGACY_ID, CURRENT_ID]) +def test_node_tunnels_in_either_format_are_ours(node_id): + assert tunnel_sync.OWNED_BY_US.match(node_id) + + +def test_reconcile_never_offers_a_hand_built_tunnel_as_an_orphan(): + """End to end, because the constant being right is not the same as it being + used in all three sweeps.""" + tunnels = [{"name": n, "id": f"id-{n}", "connections": []} for n in HAND_BUILT] + tunnels.append({"name": CURRENT_ID, "id": "id-node", "connections": []}) + + _, orphans, _ = tunnel_sync.reconcile( + wanted=set(), state={}, tunnels=tunnels, dns_records=[], access_apps=[] + ) + + assert [name for _, name, _ in orphans] == [CURRENT_ID] + + +def test_the_dns_and_access_sweeps_agree_with_the_tunnel_sweep(): + """All three read OWNED_BY_US. A hand-built hostname in the zone must not + be swept from any of them.""" + domain = tunnel_sync.REMOTE_ACCESS_DOMAIN + dns = [ + {"name": f"{n}.{domain}", "type": "CNAME", "id": f"dns-{n}"} + for n in HAND_BUILT + [CURRENT_ID] + ] + apps = [{"domain": f"{n}.{domain}", "id": f"app-{n}"} for n in HAND_BUILT + [CURRENT_ID]] + + _, orphans, _ = tunnel_sync.reconcile( + wanted=set(), state={}, tunnels=[], dns_records=dns, access_apps=apps + ) + + swept = {name for _, name, _ in orphans} + assert swept == {f"{CURRENT_ID}.{domain}"} diff --git a/mender-auto-accept/tunnel_sync.py b/mender-auto-accept/tunnel_sync.py index d737cfb..a3586a5 100644 --- a/mender-auto-accept/tunnel_sync.py +++ b/mender-auto-accept/tunnel_sync.py @@ -138,24 +138,47 @@ ABSENT = "absent" +#: Both node_id formats the fleet carries: ret<8 hex> is the legacy format, +#: retg<15 hex> the current one. Nodes are migrated one at a time, so both are +#: live at once and this script sees every node in the fleet. +_NODE_ID = r"ret(?:[0-9a-f]{8}|g[0-9a-f]{15})" + #: Names this script is allowed to create, reconfigure or delete. #: #: retnode.com already carries hand-built tunnels for live customer nodes, some #: serving several hostnames each, and ensure_tunnel() PUTs a tunnel's *entire* #: ingress config. A miscomputed name would therefore not fail, it would quietly #: replace a working tunnel's routing or delete a production DNS record. Nothing -#: existing is named ret<8 hex>, so pinning the shape makes that unreachable +#: existing is named like a node id, so pinning the shape makes that unreachable #: rather than merely unlikely. -NODE_ID_RE = re.compile(r"^ret[0-9a-f]{8}$") +#: +#: Too narrow here is safe: the worst case is refusing to act. +MAY_ACT_ON = re.compile(rf"^{_NODE_ID}$") + +#: Names this script considers its own, for deciding what is an orphan. +#: +#: Identical to MAY_ACT_ON today, and deliberately a separate constant, because +#: it answers the opposite question and fails the opposite way. "May I touch +#: this?" is safe to get wrong by being too narrow. "Is this mine, and therefore +#: rubbish to sweep up?" is *dangerous* to get wrong by being too wide: an +#: over-broad pattern claims something somebody built by hand, and --prune then +#: deletes it. +#: +#: They were one constant, so widening the format for one purpose silently +#: widened it for the other. Before adding a format here, check the zone: as of +#: 2026-09-21 the eight hand-built tunnels (fairforest, jonathan-node-1, +#: jonathan-node-2, joshOffice, mississippi, nightcrawler, sacremento, +#: wilderness) match neither format, and nothing in it begins retg. +OWNED_BY_US = re.compile(rf"^{_NODE_ID}$") def _guard(node_id): """Raise unless this is a name we are allowed to touch.""" - if not node_id or not NODE_ID_RE.match(node_id): + if not node_id or not MAY_ACT_ON.match(node_id): raise RuntimeError( - f"refusing to act on {node_id!r}: not a ret<8 hex> node id. " - f"retnode.com carries hand-built tunnels that this script must " - f"never reconfigure or delete." + f"refusing to act on {node_id!r}: not a ret<8 hex> or retg<15 hex> " + f"node id. retnode.com carries hand-built tunnels that this script " + f"must never reconfigure or delete." ) @@ -526,7 +549,9 @@ def reconcile(wanted, state, tunnels, dns_records, access_apps): Returns (repairs, orphans, notes): repairs things we believe exist but do not, or point somewhere wrong. Fixed by re-running ensure_tunnel, which is idempotent. - orphans ret<8 hex> tunnels and records nothing wants any more. Reported + orphans node-id-shaped tunnels and records nothing wants any more, by + OWNED_BY_US rather than MAY_ACT_ON: this decides what --prune + may delete, so a name we do not own must never match. Reported rather than deleted unless --prune, because a Mender outage that returned a short device list would otherwise look exactly like a fleet that had all opted out. @@ -592,14 +617,14 @@ def reconcile(wanted, state, tunnels, dns_records, access_apps): for tunnel in tunnels: name = tunnel["name"] - if NODE_ID_RE.match(name) and name not in wanted and name not in state: + if OWNED_BY_US.match(name) and name not in wanted and name not in state: orphans.append(("tunnel", name, tunnel["id"])) for app in access_apps: domain = app.get("domain") or "" node_id = domain.split(".")[0] if (domain.endswith("." + REMOTE_ACCESS_DOMAIN) - and NODE_ID_RE.match(node_id) + and OWNED_BY_US.match(node_id) and node_id not in wanted and node_id not in state): orphans.append(("access-app", domain, app["id"])) @@ -608,7 +633,7 @@ def reconcile(wanted, state, tunnels, dns_records, access_apps): node_id = name.split(".")[0] if (record.get("type") == "CNAME" and name.endswith("." + REMOTE_ACCESS_DOMAIN) - and NODE_ID_RE.match(node_id) + and OWNED_BY_US.match(node_id) and node_id not in wanted and node_id not in state): orphans.append(("dns", name, record["id"]))