diff --git a/.gitignore b/.gitignore index 699b28241b..40f04f8d23 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,7 @@ platformio.local.ini .build-wt-*/ .wt-*/ scripts/__pycache__/* + +# Reduced-TLS mbedTLS archives, ~6 MB per arch, fetched by scripts/fetch_mbedtls_4k.sh. +# Not committed because they must be rebuilt for every espressif32 platform bump. +.mbedtls-4k/ diff --git a/docs/mbedtls-tls-footprint.md b/docs/mbedtls-tls-footprint.md index 0985c4f655..70ea6db18d 100644 --- a/docs/mbedtls-tls-footprint.md +++ b/docs/mbedtls-tls-footprint.md @@ -136,6 +136,36 @@ fix was `rm -rf` the package and `pio pkg install` to re-download stock. A prepe search path keeps the change scoped to one env, because the linker takes each archive member from the first archive that satisfies an undefined symbol. +### How the archives are distributed + +They are not committed: ~6 MB per architecture, and they have to be rebuilt for every +`platformio/espressif32` bump, so committing them would grow history permanently and go +stale silently. Instead they are published as a release asset and fetched on demand: + +``` +scripts/fetch_mbedtls_4k.sh esp32s3 # download + verify against the manifest +MESHCORE_REDUCED_TLS=1 pio run -e Heltec_v3_repeater_observer_mqtt +``` + +- `scripts/mbedtls_4k_manifest.txt` — per-arch sha256 of each archive. **Update it on every + platform bump**, together with the published asset. +- `scripts/fetch_mbedtls_4k.sh` — downloads into `.mbedtls-4k//` (gitignored) and + verifies. `MBEDTLS_4K_LOCAL=` copies from a local build tree instead of downloading. +- `scripts/mbedtls_4k.py` — wired into `esp32_base.extra_scripts`, but a **no-op unless + `MESHCORE_REDUCED_TLS=1`**, so ordinary builds need no artifact and behave as before. + +The opt-in path is deliberately loud, because both ways this can go wrong produce a +firmware that looks correct and silently lacks the change: + +| failure | what happens without a guard | guard | +|---|---|---| +| directory missing or partial | linker ignores an unusable `-L` and resolves mbedTLS from the framework | pre-build: hard error | +| archives stale after a platform bump | links the wrong build | pre-build: sha256 vs manifest | +| `-L` present but outranked | framework archives win, flag is inert | post-link: `firmware.map` must resolve every `libmbed*.a` to `.mbedtls-4k/` | + +That last one is the reason the post-link check exists rather than trusting the flag: a +build flag reaching the compiler proves nothing about what got linked. + ## How to verify it worked 1. `strings`/`grep` the new `sdkconfig.h` for the four settings. diff --git a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp index abe49c88e3..ba1dc5f19d 100644 --- a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp +++ b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp @@ -433,8 +433,18 @@ void PsychicMqttClient::connect() } } - ESP_ERROR_CHECK_WITHOUT_ABORT(esp_mqtt_client_start(_client)); - ESP_LOGI(TAG, "MQTT client started."); + esp_err_t start_result = esp_mqtt_client_start(_client); + ESP_ERROR_CHECK_WITHOUT_ABORT(start_result); + if (start_result == ESP_OK) + { + _started = true; + ESP_LOGI(TAG, "MQTT client started."); + } + else + { + // Reporting success here hides the one state reconnect() cannot recover from. + ESP_LOGE(TAG, "MQTT client failed to start: %s", esp_err_to_name(start_result)); + } } void PsychicMqttClient::reconnect() @@ -489,9 +499,40 @@ void PsychicMqttClient::disconnect() } esp_mqtt_client_stop(_client); + _started = false; ESP_LOGI(TAG, "MQTT client stopped."); } +void PsychicMqttClient::softDisconnect(unsigned long timeout_ms) +{ + if (_client == nullptr) + { + ESP_LOGW(TAG, "MQTT client not started."); + return; + } + + if (!_connected) + { + // Nothing to close; leaving the task alone is the whole point. + return; + } + + ESP_LOGI(TAG, "Disconnecting MQTT transport (client task retained)."); + _stopMqttClient = false; + esp_mqtt_client_disconnect(_client); + + unsigned long waited = 0; + while (!_stopMqttClient && waited < timeout_ms) + { + vTaskDelay(10 / portTICK_PERIOD_MS); + waited += 10; + } + if (!_stopMqttClient) + { + ESP_LOGW(TAG, "softDisconnect: no DISCONNECTED event in %lums", timeout_ms); + } +} + void PsychicMqttClient::forceStop() { if (_client == nullptr) @@ -506,6 +547,7 @@ void PsychicMqttClient::forceStop() } ESP_ERROR_CHECK_WITHOUT_ABORT(esp_mqtt_client_stop(_client)); _connected = false; + _started = false; ESP_LOGI(TAG, "MQTT client forcefully stopped."); } diff --git a/lib/PsychicMqttClient/src/PsychicMqttClient.h b/lib/PsychicMqttClient/src/PsychicMqttClient.h index 42cf8d5c3d..28e53fc3a1 100644 --- a/lib/PsychicMqttClient/src/PsychicMqttClient.h +++ b/lib/PsychicMqttClient/src/PsychicMqttClient.h @@ -368,6 +368,29 @@ class PsychicMqttClient */ void disconnect(); + /** + * @brief Closes the transport but leaves the client task running. + * + * disconnect() ends with esp_mqtt_client_stop(), which ends the client task + * and returns its 6 KiB stack to the heap right as a TLS handshake vacates + * two 16 KiB mbedTLS record buffers — the stack then lands in that hole and + * the largest free block ratchets down. This variant omits the stop, so the + * task and its stack stay put. Pair it with reconnect(). + * + * @param timeout_ms how long to wait for the DISCONNECTED event before + * giving up. Bounded on purpose: disconnect()'s wait is + * unbounded and a lost event would wedge the caller. + */ + void softDisconnect(unsigned long timeout_ms = 5000); + + /** + * @brief True once esp_mqtt_client_start() has succeeded and no stop has run. + * + * reconnect() silently does nothing on a stopped client, so callers that + * want to avoid stop/start must check this and fall back to connect(). + */ + bool isStarted() const { return _started; } + /** * @brief Forcefully stops the MQTT client and disconnects from the server. * This does not trigger the onDisconnect callbacks. @@ -478,6 +501,7 @@ class PsychicMqttClient bool _connected = false; bool _stopMqttClient = false; bool _config_dirty = true; + bool _started = false; // Runtime cap on the esp-mqtt outbox for QoS 0 async publishes (bytes). // 0 = disabled. Enforced in publish(); not an esp-mqtt config field. diff --git a/platformio.ini b/platformio.ini index 6c012f1e52..73ce373b86 100644 --- a/platformio.ini +++ b/platformio.ini @@ -64,6 +64,8 @@ platform = platformio/espressif32@6.11.0 monitor_filters = esp32_exception_decoder extra_scripts = pre:scripts/generate_webconfig_html.py +; No-op unless MESHCORE_REDUCED_TLS=1; see docs/mbedtls-tls-footprint.md. + pre:scripts/mbedtls_4k.py merge-bin.py build_flags = ${arduino_base.build_flags} -D ESP32_PLATFORM diff --git a/platformio.local.ini.hold b/platformio.local.ini.hold deleted file mode 100644 index e0d89578f4..0000000000 --- a/platformio.local.ini.hold +++ /dev/null @@ -1,15 +0,0 @@ -; Local-only override (gitignored) pointing the Heltec V3 observer env at a custom -; framework whose mbedTLS archives were rebuilt with an asymmetric TLS record buffer: -; CONFIG_MBEDTLS_ASYMMETRIC_CONTENT_LEN=y -; CONFIG_MBEDTLS_SSL_IN_CONTENT_LEN=16384 (unchanged — a peer may send a 16 KiB record) -; CONFIG_MBEDTLS_SSL_OUT_CONTENT_LEN=4096 (was 16384) -; -; Expected: ~12 KiB less internal DRAM per TLS connection, ~24 KiB across two broker slots. -; Built from the shipped sdkconfig verbatim plus those three lines, so the archives differ -; only by this change. See docs/mbedtls-tls-footprint.md. -; -; Absolute path, hence local-only: not committable. - -[env:Heltec_v3_repeater_observer_mqtt] -platform_packages = - framework-arduinoespressif32 @ file:///Users/adam/framework-arduinoespressif32-tlsfix diff --git a/scripts/fetch_mbedtls_4k.sh b/scripts/fetch_mbedtls_4k.sh new file mode 100755 index 0000000000..f1e43c778d --- /dev/null +++ b/scripts/fetch_mbedtls_4k.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# Fetch the reduced-TLS mbedTLS archives into .mbedtls-4k//. +# +# These archives are built from the shipped sdkconfig plus three lines +# (CONFIG_MBEDTLS_ASYMMETRIC_CONTENT_LEN=y, IN_CONTENT_LEN 16384, +# OUT_CONTENT_LEN 4096) and save ~12 KiB of internal DRAM per TLS connection. +# See docs/mbedtls-tls-footprint.md for the rationale and the build recipe. +# +# They are not committed: ~6 MB per architecture, and they must be rebuilt for +# every platform bump, so they are published as a release asset keyed on the +# espressif32 platform version instead. +# +# scripts/fetch_mbedtls_4k.sh [arch] # default: esp32s3 +# +# Set MBEDTLS_4K_LOCAL to skip the download and copy from a local build tree: +# MBEDTLS_4K_LOCAL=~/mbedtls-4k-esp32s3/staged scripts/fetch_mbedtls_4k.sh +set -euo pipefail + +ARCH="${1:-esp32s3}" +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +DEST="$REPO_ROOT/.mbedtls-4k/$ARCH" +MANIFEST="$REPO_ROOT/scripts/mbedtls_4k_manifest.txt" +BASE_URL="${MBEDTLS_4K_BASE_URL:-https://github.com/agessaman/MeshCore/releases/download/mbedtls-4k}" + +if [ ! -f "$MANIFEST" ]; then + echo "error: missing $MANIFEST" >&2 + exit 1 +fi + +# Manifest lines: . Blank lines and # comments ignored. +expected="$(awk -v a="$ARCH" '$1 == a && $0 !~ /^#/ {print $2" "$3}' "$MANIFEST")" +if [ -z "$expected" ]; then + echo "error: no manifest entries for arch '$ARCH'" >&2 + echo "known arches: $(awk '$0 !~ /^#/ && NF {print $1}' "$MANIFEST" | sort -u | tr '\n' ' ')" >&2 + exit 1 +fi + +mkdir -p "$DEST" + +if [ -n "${MBEDTLS_4K_LOCAL:-}" ]; then + echo "copying from $MBEDTLS_4K_LOCAL" + while read -r _sha name; do + cp "$MBEDTLS_4K_LOCAL/$name" "$DEST/$name" + done <<< "$expected" +else + TARBALL="mbedtls-4k-$ARCH.tar.gz" + echo "downloading $BASE_URL/$TARBALL" + tmp="$(mktemp -d)" + trap 'rm -rf "$tmp"' EXIT + curl -fsSL "$BASE_URL/$TARBALL" -o "$tmp/$TARBALL" + tar -xzf "$tmp/$TARBALL" -C "$tmp" + while read -r _sha name; do + # Accept the archive whether or not the tarball has a leading directory. + found="$(find "$tmp" -name "$name" -type f | head -1)" + if [ -z "$found" ]; then + echo "error: $name missing from $TARBALL" >&2 + exit 1 + fi + cp "$found" "$DEST/$name" + done <<< "$expected" +fi + +# Verify every archive against the manifest. A wrong or truncated archive would +# otherwise link silently and produce a firmware without the reduced buffers. +cd "$DEST" +if command -v shasum >/dev/null 2>&1; then + echo "$expected" | shasum -a 256 -c - +else + echo "$expected" | sha256sum -c - +fi + +echo "ok: $ARCH archives verified in $DEST" diff --git a/scripts/mbedtls_4k.py b/scripts/mbedtls_4k.py new file mode 100644 index 0000000000..6718cae425 --- /dev/null +++ b/scripts/mbedtls_4k.py @@ -0,0 +1,169 @@ +"""Link the reduced-TLS mbedTLS archives, and prove they were actually linked. + +Opt in per build with MESHCORE_REDUCED_TLS=1. Off by default, so an ordinary build +needs no 6 MB artifact and behaves exactly as before. + + MESHCORE_REDUCED_TLS=1 pio run -e Heltec_v3_repeater_observer_mqtt + +The archives lower the mbedTLS outbound record buffer from 16 KiB to 4 KiB, saving +~12 KiB of internal DRAM per TLS connection on non-PSRAM observers. The inbound +buffer stays at 16 KiB, so the contiguous allocation a handshake needs is unchanged +— this buys headroom, it does not move that floor. See docs/mbedtls-tls-footprint.md. + +Two failure modes this guards against, both of which produce a firmware that looks +fine and silently lacks the change: + + - a -L pointing at a missing or partial directory. The linker ignores an + unusable search path and quietly resolves mbedTLS from the framework instead. + - archives that do not match the manifest, e.g. left over from an earlier + platform version. + +So the opt-in path verifies every archive by sha256 before the build, and after the +link re-reads firmware.map to confirm every libmbed*.a came from our directory. +""" +Import("env") + +import hashlib +import os +import sys + +REQUIRED = ("libmbedcrypto.a", "libmbedtls_2.a", "libmbedtls.a", "libmbedx509.a") + + +def _fail(msg): + print("\n*** reduced-TLS build failed ***", file=sys.stderr) + print(msg, file=sys.stderr) + print( + "\nFetch the archives with: scripts/fetch_mbedtls_4k.sh " + "\nOr build without them by unsetting MESHCORE_REDUCED_TLS.", + file=sys.stderr, + ) + env.Exit(1) + + +def _sha256(path): + h = hashlib.sha256() + with open(path, "rb") as fh: + for chunk in iter(lambda: fh.read(1 << 20), b""): + h.update(chunk) + return h.hexdigest() + + +def _manifest(project_dir, arch): + path = os.path.join(project_dir, "scripts", "mbedtls_4k_manifest.txt") + if not os.path.isfile(path): + _fail("missing scripts/mbedtls_4k_manifest.txt") + wanted = {} + with open(path) as fh: + for line in fh: + line = line.strip() + if not line or line.startswith("#"): + continue + parts = line.split() + if len(parts) == 3 and parts[0] == arch: + wanted[parts[2]] = parts[1] + return wanted + + +if os.environ.get("MESHCORE_REDUCED_TLS", "") not in ("1", "true", "yes"): + Return() + +project_dir = env.subst("$PROJECT_DIR") +arch = env.BoardConfig().get("build.mcu", "") +if not arch: + _fail("could not determine board MCU, so cannot pick an archive set") + +staged = os.path.join(project_dir, ".mbedtls-4k", arch) +if not os.path.isdir(staged): + _fail("no archives for %s at %s" % (arch, staged)) + +wanted = _manifest(project_dir, arch) +if not wanted: + _fail("manifest has no entries for arch '%s'" % arch) + +for name in REQUIRED: + archive = os.path.join(staged, name) + if not os.path.isfile(archive): + _fail("missing %s" % archive) + if name not in wanted: + _fail("%s is not in the manifest for %s" % (name, arch)) + actual = _sha256(archive) + if actual != wanted[name]: + _fail( + "%s does not match the manifest\n expected %s\n actual %s\n" + "Rebuild it for this platform version, or re-run the fetch script." + % (archive, wanted[name], actual) + ) + +# Prepend so these satisfy mbedTLS symbols ahead of the framework's own copies: +# the linker takes each archive member from the first archive that resolves it. +env.Prepend(LIBPATH=[staged]) +print("reduced-TLS: linking mbedTLS from %s (verified)" % staged) + + +def _verify_map(source, target, env): + """Confirm every mbedTLS archive in the link came from our directory. + + Fails closed. Anything that stops this from *proving* the link — no map, an + unparsable map, a short archive list — is a failure, not a warning. A warning + here would leave exactly the hole the check exists to close: an opt-in build + that succeeds while silently linking the framework's 16 KiB buffers. + """ + # Derive the map name from PROGNAME rather than hardcoding firmware.map, so a + # renamed program cannot leave us inspecting a stale or absent file. + map_path = os.path.join(env.subst("$BUILD_DIR"), + env.subst("${PROGNAME}") + ".map") + if not os.path.isfile(map_path): + legacy = os.path.join(env.subst("$BUILD_DIR"), "firmware.map") + map_path = legacy if os.path.isfile(legacy) else map_path + if not os.path.isfile(map_path): + print("\n*** reduced-TLS: no linker map at %s ***" % map_path, + file=sys.stderr) + print("Cannot prove the reduced-TLS archives were linked. Ensure the env " + "emits a map (-Wl,-Map), or unset MESHCORE_REDUCED_TLS.", + file=sys.stderr) + env.Exit(1) + return + # The map records whatever the linker was given, which for a -L hit is a path + # relative to the linker's cwd (the project dir). Resolve before comparing, or + # every one of our own archives reads as stray. + staged_real = os.path.realpath(staged) + stray = set() + seen = set() + with open(map_path, errors="replace") as fh: + for line in fh: + for token in line.split(): + if "libmbed" not in token or ".a" not in token: + continue + path = token.split("(")[0] + base = os.path.basename(path) + if not base.startswith("libmbed") or not base.endswith(".a"): + continue + seen.add(base) + resolved = os.path.realpath(os.path.join(project_dir, path)) + if os.path.dirname(resolved) != staged_real: + stray.add(path) + if stray: + print("\n*** reduced-TLS: archives linked from the WRONG place ***", + file=sys.stderr) + for path in sorted(stray): + print(" " + path, file=sys.stderr) + env.Exit(1) + return + # Every required archive must appear. Seeing only some of them means the rest + # resolved somewhere this parse did not recognise, which is not proof of anything. + missing = [name for name in REQUIRED if name not in seen] + if missing: + print("\n*** reduced-TLS: %s names only %d of %d archives ***" + % (os.path.basename(map_path), len(seen), len(REQUIRED)), + file=sys.stderr) + print(" missing: " + ", ".join(missing), file=sys.stderr) + print("Either the map format changed or mbedTLS was resolved elsewhere; " + "the reduced buffers cannot be assumed.", file=sys.stderr) + env.Exit(1) + return + print("reduced-TLS: confirmed all %d archives linked from %s" + % (len(REQUIRED), staged)) + + +env.AddPostAction("$BUILD_DIR/${PROGNAME}.elf", _verify_map) diff --git a/scripts/mbedtls_4k_manifest.txt b/scripts/mbedtls_4k_manifest.txt new file mode 100644 index 0000000000..06c0f38509 --- /dev/null +++ b/scripts/mbedtls_4k_manifest.txt @@ -0,0 +1,12 @@ +# Reduced-TLS mbedTLS archives (OUT_CONTENT_LEN 4096, IN_CONTENT_LEN 16384). +# Format: +# +# Rebuild these for every espressif32 platform bump — the archives must match the +# rest of the framework they link against. Built as of platformio/espressif32@6.11.0 +# from the recipe in docs/mbedtls-tls-footprint.md. +# +# Fetch with: scripts/fetch_mbedtls_4k.sh esp32s3 +esp32s3 01629f635b33ffa2c1fdfcd8ac52327cd3a92e8d4e7c9a32d92974e0cfdfe398 libmbedcrypto.a +esp32s3 07e7a09847589fefc35bc8d2f739d556535d535acd7185a9824b2af8edd5b05a libmbedtls_2.a +esp32s3 e12bcc8d76a368e819987f266e73c265178d6b6675f05d0e29014bb543c402af libmbedtls.a +esp32s3 c1e10324e19f6d7763737f72d9032cec832bdfce60c7a7d381a5fcfc7dcad573 libmbedx509.a diff --git a/src/helpers/ESP32WsTransportFix.cpp b/src/helpers/ESP32WsTransportFix.cpp index 2e14bb33e9..a69e0b0885 100644 --- a/src/helpers/ESP32WsTransportFix.cpp +++ b/src/helpers/ESP32WsTransportFix.cpp @@ -33,13 +33,24 @@ // Fix: [esp32_base] adds `-Wl,--wrap=esp_transport_ws_init`, so every // creation of a WS transport (esp-mqtt does one per wss slot) is routed // through __wrap_esp_transport_ws_init below, which replaces the freshly -// allocated 1024-byte buffer with a (WS_BUFFER_SIZE + 1)-byte one. The -// out-of-bounds index WS_BUFFER_SIZE then lands on our extra byte and the -// handshake fails cleanly ("Upgrade" header not found) instead of corrupting -// the heap. Upstream fixed this in ESP-IDF 5.x, so this file compiles to a +// allocated 1024-byte buffer with a (WS_BUFFER_SIZE + 1)-byte one, so the +// out-of-bounds index WS_BUFFER_SIZE lands on our extra byte instead of the +// heap canary. Upstream fixed this in ESP-IDF 5.x, so this file compiles to a // pass-through there and can be deleted (together with the --wrap flag) when // the fork moves to Arduino core 3.x. // +// Scope: this stops the heap corruption and NOTHING else. It does not make an +// oversized response fail the handshake — v4.4's read loop also exits on +// `header_len < WS_BUFFER_SIZE` going false, and then still accepts the +// connection if "Sec-WebSocket-Accept:" was inside those first bytes (it comes +// early, so it usually is). ws_connect() therefore returns success on a partial +// header block and the unread remainder arrives as the first "payload" read, +// where the deframer parses HTTP bytes as a frame header. Observed on hardware +// 2026-08-12: a large Cloudflare CSP header produced exactly that, surfacing as +// `Invalid MSG_TYPE response: 3`. Only IDF 5.2+ fixes it (it requires the +// "\r\n\r\n" delimiter, preserves the bytes after it, and fails cleanly when the +// buffer fills); it cannot be patched here because transport_ws.c is precompiled. +// // transport_ws_t below is copied verbatim from ESP-IDF release/v4.4 // transport_ws.c (the struct is file-private, so it is not in any shipped // header). Source fidelity was verified against the shipped binary: addr2line diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index 0ab4be12b1..ebbf497566 100644 --- a/src/helpers/MQTTConnectionPolicy.h +++ b/src/helpers/MQTTConnectionPolicy.h @@ -17,6 +17,7 @@ static const uint8_t kMaxFailuresAtMaxBackoff = 3; static const uint32_t kDefaultJwtLifetimeSecs = 86400UL; static const uint32_t kMaxJwtStaggerSecs = 300UL; static const uint32_t kMinimumValidEpoch = 1000000000UL; +static const uint32_t kJwtReconnectSafetyMarginSecs = 60UL; static const uint32_t kJwtClockThreshold = 1735689600UL; // 2025-01-01 UTC // A wall clock at or past this instant was set from a real source (NTP or an // admin); anything earlier is the firmware's unset-clock default (1715770351, @@ -157,6 +158,20 @@ static inline bool tokenNeedsRenewal(bool time_synced, uint32_t current_time, return current_time >= token_expires_at - renewal_buffer_secs; } +// A reconnect may keep its credentials only when their validity is known to +// outlast the next handshake; uncertainty refreshes them before reconnecting. +static inline bool canReuseJwtForReconnect(bool time_synced, bool has_token, + bool force_mint, + uint32_t current_time, + uint32_t token_expires_at) { + return time_synced && + has_token && + !force_mint && + token_expires_at >= kMinimumValidEpoch && + current_time < token_expires_at && + (token_expires_at - current_time) > kJwtReconnectSafetyMarginSecs; +} + static inline bool renewalAttemptAllowed(uint32_t now, uint32_t last_attempt) { return elapsedMs(now, last_attempt) >= kRenewalThrottleMs; } diff --git a/src/helpers/MQTTPresets.h b/src/helpers/MQTTPresets.h index b5f342c8b5..1083026dcf 100644 --- a/src/helpers/MQTTPresets.h +++ b/src/helpers/MQTTPresets.h @@ -47,6 +47,24 @@ 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.cpp b/src/helpers/bridges/MQTTBridge.cpp index 735999e97e..7983f1617e 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -704,6 +704,7 @@ MQTTBridge::MQTTBridge(NodePrefs *prefs, MQTTPrefs *obs, mesh::PacketManager *mg _slots[i].last_log_time = 0; _slots[i].port = 1883; _slot_reconfigure_pending[i] = false; + _slot_force_jwt_mint[i] = false; _status_publish_pending[i] = false; } @@ -768,17 +769,10 @@ void MQTTBridge::allocateRuntimeBuffers() { _json_scratch_buffer ? "PSRAM" : "stack fallback"); #endif -#if defined(WITH_MQTT_NEIGHBORS) - // Persistent neighbors JSON buffer, heap-allocated on every board: too large to - // keep inline in the bridge object the way the non-PSRAM status/packet buffers - // are. psram_malloc() falls back to internal DRAM, so this works without PSRAM. - // Unlike status/packet there is no stack fallback — a nullptr simply disables - // publishing (requestPublishNeighbors/publishNeighbors both no-op on nullptr). - _neighbors_json_buffer = static_cast(MQTTRuntimeBufferLifecycle::allocateIfMissing( - _neighbors_json_buffer, NEIGHBORS_JSON_BUFFER_SIZE, psram_malloc)); - MQTT_DEBUG_PRINTLN("Neighbors buffer: %s", - _neighbors_json_buffer ? "ready" : "unavailable"); -#endif + // The neighbors JSON buffer is NOT allocated here — requestPublishNeighbors() + // allocates it on first use, so a node with mqtt.neighbors off never pays its + // 4 KB. mqtt.neighbors is read live with no bridge restart, so gating on the + // pref here would leave a runtime enable with no buffer. } void MQTTBridge::releaseRuntimeBuffers() { @@ -795,7 +789,7 @@ void MQTTBridge::releaseRuntimeBuffers() { _json_scratch_doc.clear(); #if defined(WITH_MQTT_NEIGHBORS) - // Paired with the unconditional allocation in allocateRuntimeBuffers(). + // Paired with the lazy allocation in requestPublishNeighbors(); no-op if never used. _neighbors_json_buffer = static_cast(MQTTRuntimeBufferLifecycle::release( _neighbors_json_buffer, psram_free)); _neighbors_publish_len = 0; @@ -1608,6 +1602,7 @@ bool MQTTBridge::ensureSlotClient(int index) { slot.client->onConnect([this, index](bool sessionPresent) { MQTT_DEBUG_PRINTLN("MQTT%d connected", index + 1); _slots[index].connected = true; + _slot_force_jwt_mint[index] = false; // NOTE: reconnect_backoff / max_backoff_failures are NOT reset here. // A CONNACK alone doesn't prove the link is healthy — a broker that // accepts and then drops within seconds would reset the ladder every @@ -1654,6 +1649,7 @@ bool MQTTBridge::ensureSlotClient(int index) { _slots[index].last_sock_errno = error.esp_transport_sock_errno; _slots[index].last_error_time = millis(); if (error.error_type == MQTT_ERROR_TYPE_CONNECTION_REFUSED) { + _slot_force_jwt_mint[index] = true; // Broker rejected the MQTT CONNECT itself — not a transport failure. // return code: 1=protocol, 2=client-id rejected, 3=server unavailable, // 4=bad username/password, 5=not authorized. Codes 3/4/5 point at a @@ -1775,9 +1771,11 @@ bool MQTTBridge::setupSlot(int index) { } // Reconfigure path: if we're re-applying (e.g. after a preset change), stop - // the existing connection cleanly first. The client object (and its mbedTLS - // context) is reused; setCredentials / setServer below overwrite the config - // fields in place before connect() restarts the ESP-IDF client. + // the existing connection cleanly first. The client object is reused, but its + // mbedTLS context is NOT — closing the transport destroys the TLS session, + // record buffers, and peer certificate, and the next connect() reallocates + // them. setCredentials / setServer below overwrite the config fields in place + // before connect() restarts the ESP-IDF client. if (slot.initial_connect_done) { if (slot.client->connected()) { slot.client->disconnect(); @@ -1806,6 +1804,8 @@ bool MQTTBridge::setupSlot(int index) { slot.max_backoff_failures = 0; slot.circuit_breaker_tripped = false; slot.last_reconnect_attempt = 0; + // The refusal that set this belonged to the credentials being cleared here. + _slot_force_jwt_mint[index] = false; } bool uses_jwt = (slot.preset && slot.preset->auth_type == MQTT_AUTH_JWT) || slot.audience[0] != '\0'; @@ -1995,8 +1995,27 @@ void MQTTBridge::teardownSlot(int index) { slot.last_reconnect_attempt = 0; slot.last_log_time = 0; slot.last_deferred_log_ms = 0; + // The refusal that set this belonged to the credentials being cleared here. + _slot_force_jwt_mint[index] = false; +} + +// A stopped client needs connect(): reconnect() is a documented no-op on one, so reaching +// it here would strand the slot. The WiFi-transition teardown stops a client while leaving +// initial_connect_done set, so the ladder does see this state. +void MQTTBridge::reconnectSlotClient(int index) { + if (index < 0 || index >= RUNTIME_MQTT_SLOTS) return; + MQTTSlot& slot = _slots[index]; + if (slot.client == nullptr) return; + + if (!slot.client->isStarted()) { + MQTT_DEBUG_PRINTLN("MQTT%d start (client was stopped)", index + 1); + slot.client->connect(); + return; + } + slot.client->reconnect(); } + void MQTTBridge::maintainSlotConnections() { if (!_identity) return; @@ -2136,15 +2155,37 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns (time_synced && old_token_expires_at >= 1000000000 && current_time >= (old_token_expires_at - renewal_buffer)); - if (old_token_expired_or_imminent || !slot.client->connected()) { + // Only bounce for exp if this broker actually enforces it. A broker that + // leaves live sessions alone past expiry needs the fresh token at the next + // reconnect, not now, and the bounce's re-handshake is where contiguity goes. + const bool exp_forces_bounce = + old_token_expired_or_imminent && mqttPresetEnforcesTokenExp(slot.preset); + if (!exp_forces_bounce && old_token_expired_or_imminent && slot.client->connected()) { + MQTT_DEBUG_PRINTLN("MQTT%d token renewed, no bounce (broker does not enforce exp)", + index + 1); + } + if (exp_forces_bounce || !slot.client->connected()) { // Disconnect + reconnect with fresh credentials, reusing existing client // to avoid internal heap leak/fragmentation from destroy/create cycles MQTT_DEBUG_PRINTLN("MQTT%d token renewal: reconnecting with fresh credentials", index + 1); - if (slot.client->connected()) { - slot.client->disconnect(); // stops the client internally + MQTT_TRACE_HEAP("renewal:before-bounce", index); + if (slot.client->isStarted()) { + // Keep the esp-mqtt task alive across the handshake. disconnect() + // would stop it, returning its 6 KiB stack into the hole the two + // 16 KiB mbedTLS record buffers just vacated — which is what walks + // the largest free block down 16 KiB at a time on non-PSRAM boards. + slot.client->softDisconnect(); + MQTT_TRACE_HEAP("renewal:after-disconnect", index); + slot.client->setCredentials(_jwt_username, slot.auth_token); + MQTT_TRACE_HEAP("renewal:after-credentials", index); + slot.client->reconnect(); + } else { + // Client was stopped (teardown/reconfigure). reconnect() is a no-op + // on a stopped client, so this path must start it. + slot.client->setCredentials(_jwt_username, slot.auth_token); + slot.client->connect(); } - slot.client->setCredentials(_jwt_username, slot.auth_token); - slot.client->connect(); // restart stopped client; reconnect() fails silently on a stopped client + MQTT_TRACE_HEAP("renewal:after-reconnect", index); reconnect_attempted = true; _last_slot_reconnect_ms = now_millis; MQTT_DEBUG_PRINTLN("MQTT%d int_heap=%d at token renewal reconnect", index + 1, @@ -2169,6 +2210,59 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns // persistent clients (Phase 1), the mbedTLS context is allocated once at // startup and the preflight is no longer necessary. + const auto prepareJwtReconnect = [&](bool force_mint, int backoff_level) { + const bool has_token = slot.auth_token && slot.auth_token[0] != '\0'; + const unsigned long expires_at = slot.token_expires_at; + const bool remaining_known = time_synced && + expires_at >= MQTTConnectionPolicy::kMinimumValidEpoch; + const unsigned long remaining_secs = current_time < expires_at + ? expires_at - current_time + : 0; + const bool force_mint_after_refusal = _slot_force_jwt_mint[index]; + force_mint = force_mint || force_mint_after_refusal; + const bool reuse_token = MQTTConnectionPolicy::canReuseJwtForReconnect( + time_synced, has_token, force_mint, static_cast(current_time), + static_cast(expires_at)); + const char* mint_reason = "none"; + if (!reuse_token) { + if (backoff_level < 0) { + mint_reason = "circuit-breaker-probe"; + } else if (force_mint_after_refusal) { + mint_reason = "connection-refused"; + } else if (!time_synced) { + mint_reason = "clock-unsynced"; + } else if (!has_token) { + mint_reason = "empty-token"; + } else if (expires_at < MQTTConnectionPolicy::kMinimumValidEpoch) { + mint_reason = "invalid-expiry"; + } else if (current_time >= expires_at) { + mint_reason = "expired"; + } else { + mint_reason = "safety-margin"; + } + } + const char* mint_result = reuse_token ? "REUSED" : "FAILED"; + if (!reuse_token && createSlotAuthToken(index)) { + slot.client->setCredentials(_jwt_username, slot.auth_token); + mint_result = "OK"; + } + char remaining_text[24]; + if (remaining_known) { + snprintf(remaining_text, sizeof(remaining_text), "%lus", remaining_secs); + } else { + strncpy(remaining_text, "unknown", sizeof(remaining_text)); + } + if (backoff_level >= 0) { + MQTT_DEBUG_PRINTLN("MQTT%d JWT reconnect backoff=%d token=%s mint_reason=%s result=%s remaining=%s", + index + 1, backoff_level, reuse_token ? "REUSE" : "MINT", mint_reason, + mint_result, remaining_text); + } else { + MQTT_DEBUG_PRINTLN("MQTT%d JWT circuit-breaker probe token=%s mint_reason=%s result=%s remaining=%s", + index + 1, reuse_token ? "REUSE" : "MINT", mint_reason, mint_result, + remaining_text); + } + }; + // Periodic probe for circuit-breaker-tripped slots (recovery from transient outages) // Attempts a single reconnect every 30 minutes to see if the server has come back if (slot.circuit_breaker_tripped && !reconnect_attempted) { @@ -2185,17 +2279,11 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns _radio ? _radio->getRadioState() : -1, (_radio && _radio->getLastRecvMillis() > 0) ? (_ms->getMillis() - _radio->getLastRecvMillis()) : 0); if (slot_uses_jwt) { - // Regenerate or refresh token, then reconnect the persistent client. - // Reaching the ladder at all means setupSlot() ran, so the client object - // and its mbedTLS context are live and no full setup is needed here. - if (createSlotAuthToken(index)) { - slot.client->setCredentials(_jwt_username, slot.auth_token); - MQTT_DEBUG_PRINTLN("MQTT%d circuit breaker probe (fresh token)", index + 1); - } - slot.client->reconnect(); - } else { - slot.client->reconnect(); + prepareJwtReconnect(true, -1); } + // Via the helper: reconnect() is a no-op on a client the WiFi-drop path + // stopped, which would probe forever without ever starting it. + reconnectSlotClient(index); // If the connect callback fires and sets slot.connected = true, // it will clear circuit_breaker_tripped via the onConnect handler } @@ -2225,22 +2313,14 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns reconnect_attempted = true; _last_slot_reconnect_ms = now_millis; if (slot_uses_jwt) { - // Always lightweight reconnect on the persistent client. A stale/expired - // token is handled by regenerating it in place and updating credentials - // — no teardown is needed because the client and its mbedTLS context - // persist for the bridge lifetime. - if (createSlotAuthToken(index)) { - slot.client->setCredentials(_jwt_username, slot.auth_token); - MQTT_DEBUG_PRINTLN("MQTT%d reconnect (fresh token, backoff %d)", index + 1, slot.reconnect_backoff); - } else { - MQTT_DEBUG_PRINTLN("MQTT%d reconnect (token refresh failed, backoff %d)", index + 1, slot.reconnect_backoff); - } - slot.client->reconnect(); + prepareJwtReconnect(false, slot.reconnect_backoff); } else { // Non-JWT slots — lightweight reconnect on existing client. MQTT_DEBUG_PRINTLN("MQTT%d reconnect (non-JWT, backoff %d)", index + 1, slot.reconnect_backoff); - slot.client->reconnect(); } + // Via the helper: reconnect() is a no-op on a client the WiFi-drop path + // stopped, which would back off forever without ever starting it. + reconnectSlotClient(index); } } } @@ -3625,10 +3705,24 @@ void MQTTBridge::setNeighborsSchedule(NeighborsPhase phase, uint32_t secs_until_ } void MQTTBridge::requestPublishNeighbors(const char* json, size_t len) { - if (!_neighbors_json_buffer || !json || len == 0) return; + if (!json || len == 0) return; // Drop a new snapshot while one is still being published (Core 0 clears the // flag when done). Acquire pairs with the task loop's release store. if (_neighbors_publish_pending.load(std::memory_order_acquire)) return; + // Allocating here means a stopped bridge must not: a discovery started before the + // stop can finish after it, and releaseRuntimeBuffers() has already run, so the + // allocation would be retained with no task left to consume it. isRunning() is the + // same flag end() guards on. + if (!isRunning()) return; + // Allocated on first use so a node with neighbors off never pays the 4 KB. + // Cross-core safe: the release store below publishes this pointer, and the task + // loop only reads it after the matching acquire load. + _neighbors_json_buffer = static_cast(MQTTRuntimeBufferLifecycle::allocateIfMissing( + _neighbors_json_buffer, NEIGHBORS_JSON_BUFFER_SIZE, psram_malloc)); + if (!_neighbors_json_buffer) { + MQTT_DEBUG_PRINTLN("Neighbors buffer unavailable, dropping snapshot"); + return; + } if (len >= NEIGHBORS_JSON_BUFFER_SIZE) { len = NEIGHBORS_JSON_BUFFER_SIZE - 1; } @@ -3942,7 +4036,9 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { if (createSlotAuthToken(i)) { _slots[i].client->setCredentials(_jwt_username, _slots[i].auth_token); } - _slots[i].client->reconnect(); + // Reuse the transport — the fault is stale credentials, not the transport — + // but via the helper, so a stopped client is started rather than no-opped. + reconnectSlotClient(i); } } } diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index eb7ab94349..bda752a6fe 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -36,6 +36,19 @@ class MeshSNMPAgent; // Forward declaration #define MQTT_DEBUG_PRINTLN(...) {} #endif +// Largest-free-block trace around the reconnect lifecycle. On non-PSRAM boards the +// mbedTLS record buffers are two 16 KiB internal-DRAM blocks, so what matters is the +// largest contiguous block, not the free total — a soak can show flat free heap while +// max_alloc walks down. Costs two heap_caps calls per reconnect, so it stays on. +#if defined(MQTT_DEBUG) && defined(ARDUINO) && defined(ESP32) + #define MQTT_TRACE_HEAP(point, idx) \ + MQTT_DEBUG_PRINTLN("HEAPTRACE slot=%d %s free=%u max=%u", (int)(idx) + 1, point, \ + (unsigned)heap_caps_get_free_size(MALLOC_CAP_INTERNAL), \ + (unsigned)heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL)) +#else + #define MQTT_TRACE_HEAP(point, idx) do {} while(0) +#endif + #ifdef WITH_MQTT_BRIDGE // Periodic neighbors publication keys off the mesh neighbor cache (sized by @@ -215,6 +228,10 @@ class MQTTBridge : public BridgeBase { // Pending slot reconfigure: set from CLI (Core 1), processed by MQTT task (Core 0) volatile bool _slot_reconfigure_pending[RUNTIME_MQTT_SLOTS]; + // A broker refusal can invalidate an otherwise clock-valid JWT. The esp-mqtt + // callback sets this and the bridge loop consumes it; byte access is atomic. + volatile bool _slot_force_jwt_mint[RUNTIME_MQTT_SLOTS]; + // Pending on-connect status publish: set from the onConnect callback (which // runs on the esp-mqtt event task, NOT this bridge task), consumed by the MQTT // task (Core 0). publishStatusToSlot() touches the shared status doc/buffer/ @@ -434,6 +451,9 @@ class MQTTBridge : public BridgeBase { int activatedSlotCount() const; bool canActivateSlot(int index) const; void teardownSlot(int index); // Disconnect the slot's client (keeps the object alive) + // Reconnect a slot, starting it instead when the client is stopped (reconnect() is a + // no-op on a stopped client). See the definition. + void reconnectSlotClient(int index); void maintainSlotConnections(); // Maintain all slot connections (token renewal, reconnect) void maintainSlotConnection(int index, unsigned long now_millis, unsigned long current_time, bool time_synced, bool& reconnect_attempted, bool& teardown_attempted); bool createSlotAuthToken(int index); // Create/renew JWT token for a slot diff --git a/test/mocks/Arduino.h b/test/mocks/Arduino.h index 77499fe414..a3d5b2765e 100644 --- a/test/mocks/Arduino.h +++ b/test/mocks/Arduino.h @@ -2,8 +2,15 @@ #include #include +// The real Arduino.h pulls in stdlib.h, so device code reaches atoi/atol/atof/strtoul +// without including it. Mirror that here or those sources fail only on the native build. +#include #include "Stream.h" +using std::atof; +using std::atoi; +using std::atol; + inline uint32_t g_mock_millis = 0; using std::isnan; diff --git a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp index 496a1c434e..55c58793d7 100644 --- a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp +++ b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp @@ -137,6 +137,22 @@ TEST(MQTTConnectionPolicy, SyncedClockRenewsInvalidExpiredOrImminentTokens) { EXPECT_TRUE(Policy::tokenNeedsRenewal(true, expires + 1U, expires, 300U)); } +TEST(MQTTConnectionPolicy, JwtReconnectReusesOnlyProvenValidCredentials) { + const uint32_t now = 1735689600U; + const uint32_t usable_expiry = now + Policy::kJwtReconnectSafetyMarginSecs + 1U; + + EXPECT_TRUE(Policy::canReuseJwtForReconnect(true, true, false, now, usable_expiry)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect( + true, true, false, now, Policy::kMinimumValidEpoch - 1U)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, true, false, now, 0U)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect( + true, true, false, now, now + Policy::kJwtReconnectSafetyMarginSecs)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, true, false, now, now - 1U)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, false, false, now, usable_expiry)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(false, true, false, now, usable_expiry)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, true, true, now, usable_expiry)); +} + TEST(MQTTConnectionPolicy, RenewalThrottleHasExactBoundaryAndHandlesRollover) { EXPECT_FALSE(Policy::renewalAttemptAllowed(59999U, 0U)); EXPECT_TRUE(Policy::renewalAttemptAllowed(60000U, 0U));