From 1bfbbf4bd80689acc57a79d3ed196b12a7a1f653 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 19 Sep 2026 16:46:55 -0700 Subject: [PATCH] build(mqtt): make the preset table byte-identical across channels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preset table is fleet state, not a build detail. A slot's preset is stored in /mqtt.json by name, and firmware that does not know a name does not merely ignore it: MQTTPrefsSerializer repairs it to "none" and CommonCLI writes the repaired file back to flash. A node that rolls back to a channel missing a preset therefore loses that slot permanently, and re-upgrading does not bring it back. The parity gate compared preset names only, deliberately allowing URL, CA and credential drift. It now requires src/helpers/MQTTPresets.h to be byte-identical between the channels, which also catches that drift — the two channels are meant to dial the same brokers. That is only workable if the file holds no channel-specific code, so mqttPresetEnforcesTokenExp() moves to the new MQTTPresetPolicy.h. It was the sole difference between the two channels' copies, and with it moved they match exactly today. Policy keyed off the table belongs there from now on; the table itself stays pure data. The older name-only comparison stays available without --exact for ad-hoc use, and the checker's self-test now covers both modes, including that --exact rejects a config-only change the name check waves through. --- .../workflows/check-mqtt-preset-parity.yml | 12 +- scripts/check_mqtt_preset_parity.py | 107 +++++++++++++++++- src/helpers/MQTTPresetPolicy.h | 34 ++++++ src/helpers/MQTTPresets.h | 18 --- src/helpers/bridges/MQTTBridge.h | 1 + 5 files changed, 145 insertions(+), 27 deletions(-) create mode 100644 src/helpers/MQTTPresetPolicy.h diff --git a/.github/workflows/check-mqtt-preset-parity.yml b/.github/workflows/check-mqtt-preset-parity.yml index e1039a40..7b7d663d 100644 --- a/.github/workflows/check-mqtt-preset-parity.yml +++ b/.github/workflows/check-mqtt-preset-parity.yml @@ -1,7 +1,11 @@ name: Check MQTT Preset Name Parity -# Ensures observer-firmware and observer-firmware-dev share the same built-in -# MQTT preset *names* (config details may differ). See scripts/check_mqtt_preset_parity.py. +# Ensures observer-firmware and observer-firmware-dev ship an identical +# src/helpers/MQTTPresets.h. A slot's preset is stored in /mqtt.json by name, and +# firmware that does not know a name repairs it to "none" and writes the file +# back, so a node that rolls back between channels loses that slot for good. +# Behaviour that differs per channel belongs in MQTTPresetPolicy.h, which is not +# compared. See scripts/check_mqtt_preset_parity.py. permissions: contents: read @@ -81,9 +85,9 @@ jobs: - name: Self-test checker run: python3 scripts/check_mqtt_preset_parity.py --self-test - - name: Compare preset names + - name: Compare preset tables run: | - python3 scripts/check_mqtt_preset_parity.py \ + python3 scripts/check_mqtt_preset_parity.py --exact \ /tmp/preset-parity/prod.h \ /tmp/preset-parity/dev.h \ --label-a observer-firmware \ diff --git a/scripts/check_mqtt_preset_parity.py b/scripts/check_mqtt_preset_parity.py index d213a258..039bc6ee 100755 --- a/scripts/check_mqtt_preset_parity.py +++ b/scripts/check_mqtt_preset_parity.py @@ -1,13 +1,25 @@ #!/usr/bin/env python3 -"""Compare MQTT built-in preset *names* between two MQTTPresets.h files. +"""Compare the MQTT broker preset table between two MQTTPresets.h files. -Only the first string field of each ``MQTT_PRESETS`` entry is compared (set -equality, case-sensitive). URL, auth, CA, keepalive, and credentials are -ignored so channel branches may diverge on config details without failing CI. +With ``--exact`` (what CI uses) the two files must be byte-identical. The table +is fleet state, not just a build detail: a slot's preset is stored in +/mqtt.json by NAME, and firmware that does not know a name does not merely +ignore it — MQTTPrefsSerializer repairs it to "none" and the repaired file is +written back to flash. A node that rolls back from one channel to the other +therefore loses that slot permanently. Byte equality also catches URL, CA and +credential drift, which a name-only check passes silently even though the two +channels are meant to dial the same brokers. + +Channel-specific behaviour belongs in MQTTPresetPolicy.h, which is not compared, +so this file can stay identical while the channels differ elsewhere. + +Without ``--exact`` only the first string field of each ``MQTT_PRESETS`` entry +is compared (set equality, case-sensitive). That is the older, weaker check, +kept for ad-hoc use. Usage:: - python3 scripts/check_mqtt_preset_parity.py FILE_A FILE_B \\ + python3 scripts/check_mqtt_preset_parity.py FILE_A FILE_B --exact \\ --label-a observer-firmware --label-b observer-firmware-dev python3 scripts/check_mqtt_preset_parity.py --self-test @@ -137,6 +149,49 @@ def compare( ] +def compare_exact( + path_a: Path, + path_b: Path, + *, + label_a: str, + label_b: str, +) -> list[str]: + """Return error lines if the two files differ byte for byte.""" + + data_a = path_a.read_bytes() + data_b = path_b.read_bytes() + if data_a == data_b: + return [] + + import difflib + + errors = [f"{label_a} and {label_b} do not match byte for byte."] + try: + diff = list( + difflib.unified_diff( + data_a.decode("utf-8").splitlines(), + data_b.decode("utf-8").splitlines(), + fromfile=label_a, + tofile=label_b, + lineterm="", + n=1, + ) + ) + except UnicodeDecodeError: + errors.append("(binary difference; cannot render a text diff)") + return errors + + # Enough to identify the drift without pasting the whole table into a log. + errors.extend(diff[:60]) + if len(diff) > 60: + errors.append(f"... {len(diff) - 60} more diff line(s)") + errors.append( + "Preset rows must be identical on both channels. Behaviour that differs " + "per channel belongs in MQTTPresetPolicy.h." + ) + return errors + + def load_names(path: Path) -> set[str]: text = path.read_text(encoding="utf-8") names, _ = parse_presets(text, source=str(path)) @@ -237,6 +292,26 @@ static const MQTTPresetDef MQTT_PRESETS[MQTT_PRESET_COUNT] = {{ print("self-test failed: URL scheme was treated as a comment.", file=sys.stderr) return 1 + # --exact: identical bytes pass, any drift fails — including a change + # that a name-only comparison waves through. + if compare_exact(equal_a, equal_a, label_a="a", label_b="a-copy"): + print("self-test failed: a file was not equal to itself.", file=sys.stderr) + return 1 + if compare(load_names(equal_a), load_names(equal_b), label_a="a", label_b="b"): + print("self-test failed: fixture assumption broken.", file=sys.stderr) + return 1 + exact_errs = compare_exact(equal_a, equal_b, label_a="a", label_b="b") + if not exact_errs or "byte for byte" not in exact_errs[0]: + print( + "self-test failed: --exact accepted files that differ only in " + f"config: {exact_errs!r}", + file=sys.stderr, + ) + return 1 + if not any("MQTTPresetPolicy.h" in line for line in exact_errs): + print("self-test failed: --exact failure omitted the remedy.", file=sys.stderr) + return 1 + # Silence unused path in fixture layout. dupes.write_text("// unused\n", encoding="utf-8") @@ -250,6 +325,11 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument("file_b", type=Path, nargs="?", help="Second MQTTPresets.h (e.g. dev)") parser.add_argument("--label-a", default="file_a", help="Label for file_a in reports") parser.add_argument("--label-b", default="file_b", help="Label for file_b in reports") + parser.add_argument( + "--exact", + action="store_true", + help="Require the two files to be byte-identical (what CI enforces)", + ) parser.add_argument("--self-test", action="store_true") args = parser.parse_args(argv) @@ -259,6 +339,23 @@ def main(argv: list[str] | None = None) -> int: if args.file_a is None or args.file_b is None: parser.error("FILE_A and FILE_B are required unless --self-test is set") + if args.exact: + try: + errors = compare_exact( + args.file_a, args.file_b, label_a=args.label_a, label_b=args.label_b + ) + except OSError as error: + print(f"MQTT preset parity check could not run: {error}", file=sys.stderr) + return 2 + if errors: + print("MQTT preset parity check failed:", *errors, sep="\n ", file=sys.stderr) + return 1 + print( + f"MQTT preset parity check passed: {args.label_a} and {args.label_b} " + "have identical preset tables." + ) + return 0 + try: names_a = load_names(args.file_a) names_b = load_names(args.file_b) diff --git a/src/helpers/MQTTPresetPolicy.h b/src/helpers/MQTTPresetPolicy.h new file mode 100644 index 00000000..a5a8ddce --- /dev/null +++ b/src/helpers/MQTTPresetPolicy.h @@ -0,0 +1,34 @@ +#pragma once + +#include "MQTTPresets.h" + +#include + +// Policy that keys off a preset rather than describing one. +// +// MQTTPresets.h is data: it is the broker table, and it is kept byte-identical +// between the observer-firmware and observer-firmware-dev channels so a node +// that rolls back cannot meet a preset name its firmware does not know. An +// unknown name is not merely ignored — MQTTPrefsSerializer repairs it to "none" +// and the repaired /mqtt.json is written back, so the operator's slot is gone +// for good. Channel-specific behaviour therefore lives here instead, where the +// two channels are free to differ. + +// True when the broker tears down a live session once its JWT passes exp, so the +// renewal must proactively bounce the connection to present a fresh token. +// +// Default true, because getting this wrong the safe way costs a re-handshake and +// getting it wrong the unsafe way costs an outage. waev is the exception: its +// operator confirmed (2026-08-11) that their servers do not disconnect on expiry, +// so a live session there needs only its credentials refreshed for the next +// reconnect. waev is also the only preset with a short token_lifetime, so it was +// the only one bouncing often — every ~47 min, and each bounce's re-handshake can +// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board. +// +// Keyed by name rather than a struct field on purpose: adding a field would mean +// re-ordering a dozen positional initialisers in the table, where a mistake is +// silent. +static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) { + if (!preset || !preset->name) return true; // custom/audience slots: assume enforced + return strcmp(preset->name, "waev") != 0; +} diff --git a/src/helpers/MQTTPresets.h b/src/helpers/MQTTPresets.h index 8b9d4b1f..5537b031 100644 --- a/src/helpers/MQTTPresets.h +++ b/src/helpers/MQTTPresets.h @@ -47,24 +47,6 @@ struct MQTTPresetDef { // Braces match topic placeholders ({device}/{iata}); never send this string to the broker. static const char MQTT_USERPASS_USERNAME_PUBKEY[] = "{pubkey}"; -// True when the broker tears down a live session once its JWT passes exp, so the -// renewal must proactively bounce the connection to present a fresh token. -// -// Default true, because getting this wrong the safe way costs a re-handshake and -// getting it wrong the unsafe way costs an outage. waev is the exception: its -// operator confirmed (2026-08-11) that their servers do not disconnect on expiry, -// so a live session there needs only its credentials refreshed for the next -// reconnect. waev is also the only preset with a short token_lifetime, so it was -// the only one bouncing often — every ~47 min, and each bounce's re-handshake can -// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board. -// -// Keyed by name rather than a struct field on purpose: adding a field would mean -// re-ordering a dozen positional initialisers below, where a mistake is silent. -static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) { - if (!preset || !preset->name) return true; // custom/audience slots: assume enforced - return strcmp(preset->name, "waev") != 0; -} - static inline bool mqttPresetUsesDevicePubkeyUsername(const MQTTPresetDef* preset) { return preset && preset->auth_type == MQTT_AUTH_USERPASS && preset->userpass_username && diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index 7b662da6..1126da0b 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -10,6 +10,7 @@ #include "helpers/JWTHelper.h" #include "helpers/MQTTPacketFilter.h" #include "helpers/MQTTPresets.h" +#include "helpers/MQTTPresetPolicy.h" #include "helpers/MQTTLifecycle.h" #include "helpers/AlertFaultPolicy.h" #include "helpers/MQTTEffectiveConfig.h"