From 0a78712ef2b2c57351bc321bf62112d715ceb615 Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 9 Sep 2026 17:35:49 -0700 Subject: [PATCH] feat(mqtt): effective-config value type and the recreate decision (F02/F03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decision half of the client-configuration fix, as a pure header so every transition is a host test rather than a hardware run. `MqttEffectiveConfig` is the complete owned description of what a slot's client should be configured with — URI (its own copy, because `slot.broker_uri` is rewritten in place), transport, trust policy, auth mode, credential pointers, buffer size, keepalive. Two things are deliberate: - the transport is derived from the URI scheme, and the trust policy is normalised against it: an unencrypted transport verifies nothing, whatever certificate material was requested, so a plaintext endpoint can never look like it retained a verified policy; - absent credentials are represented as nullptr and written as "", never omitted. IDF's `esp_mqtt_set_if_config()` treats NULL as "leave unchanged", so a cleared wrapper pointer cannot erase an SDK-held credential, while an empty string overwrites it and leaves the CONNECT's username flag clear (verified on an ESP32-S3 against a local broker on IDF 4.4). `mqttConfigRecreateDecision()` says whether the existing client can be reconfigured in place. Reuse covers every credential and auth-mode change and any endpoint move within one scheme — the cases that matter for reconnect and token renewal, where a client create/destroy cycle is the fork's documented internal-heap fragmentation driver. Recreation is reserved for the three fields that cannot be overwritten safely: a transport (scheme) change, a trust-policy or CA-certificate change, and growth beyond the allocated buffer capacity, which IDF 4.4 fixes at client init. Client certificates are excluded on purpose: `setClientCertificate()` has no caller in the firmware, so modelling mutual TLS here would be untested configuration surface. Noted in the header. Nothing consumes this yet; the bridge is migrated onto it after the result propagation and generation-guard steps, which it depends on. --- src/helpers/MQTTEffectiveConfig.h | 213 ++++++++++++++++++ .../test_mqtt_effective_config.cpp | 203 +++++++++++++++++ 2 files changed, 416 insertions(+) create mode 100644 src/helpers/MQTTEffectiveConfig.h create mode 100644 test/test_mqtt_effective_config/test_mqtt_effective_config.cpp diff --git a/src/helpers/MQTTEffectiveConfig.h b/src/helpers/MQTTEffectiveConfig.h new file mode 100644 index 00000000..9408d524 --- /dev/null +++ b/src/helpers/MQTTEffectiveConfig.h @@ -0,0 +1,213 @@ +#pragma once + +#include +#include + +// The complete, owned description of what a slot's MQTT client should be +// configured with, plus the decision of whether the existing SDK client can be +// reconfigured in place or has to be recreated. +// +// Pure logic: no Arduino, esp-mqtt or PsychicMqttClient dependency, so every +// transition in the table below is exercised on the host +// (test/test_mqtt_effective_config). +// +// Why this exists (F02/F03). The bridge used to apply configuration field by +// field, skipping the cleanup of stale fields whenever +// `slot.initial_connect_done` was false — which `teardownSlot()` had just +// cleared, so normal live reconfiguration always skipped it. On hardware that +// sent the previous broker's JWT username in the CONNECT to a newly configured +// anonymous endpoint. Two rules fix it: +// +// 1. Every field is written on every apply. "Absent" is a value that gets +// written (the empty string), never an omission — IDF's +// `esp_mqtt_set_if_config()` treats NULL as "leave unchanged", so a +// cleared wrapper pointer cannot erase an SDK-held credential, while an +// empty string both overwrites it and leaves the CONNECT's username flag +// clear (verified on an ESP32-S3 against a local broker, IDF 4.4). +// 2. Where a field cannot be overwritten safely, the client is recreated. +// That is the exception, not the rule: esp-mqtt's stop/start cycle is the +// fork's documented internal-heap fragmentation driver, so recreation must +// never happen on the reconnect or token-renewal path. + +// Matches MQTTSlot::broker_uri so an effective config can hold its own copy. +#define MQTT_EFFECTIVE_URI_MAX 128 + +enum class MqttTransport : uint8_t { + Unknown = 0, + Tcp, // mqtt:// + Tls, // mqtts:// + Ws, // ws:// + Wss, // wss:// +}; + +enum class MqttTrust : uint8_t { + Plaintext = 0, // unencrypted transport: no server verification exists + Bundle, // shared CA bundle attach callback + PemCert, // one specific CA certificate +}; + +enum class MqttAuth : uint8_t { + None = 0, + UserPass, + Jwt, +}; + +struct MqttEffectiveConfig { + char uri[MQTT_EFFECTIVE_URI_MAX] = {0}; + MqttTransport transport = MqttTransport::Unknown; + MqttTrust trust = MqttTrust::Plaintext; + MqttAuth auth = MqttAuth::None; + // Borrowed pointers with a lifetime at least as long as the client's: preset + // certificates live in flash, credentials in the slot/bridge storage. + const char* pem = nullptr; + const char* username = nullptr; // nullptr == absent, written as "" + const char* password = nullptr; + uint16_t buffer_size = 0; + uint16_t keepalive = 0; + bool valid = false; // false until successfully built +}; + +// Client certificates are deliberately absent: `setClientCertificate()` has no +// caller in the firmware (no broker preset uses mutual TLS), so modelling it +// here would be untested configuration surface. Adding mutual TLS means adding +// it to this struct and to the recreate decision below. + +static inline MqttTransport mqttTransportFromUri(const char* uri) { + if (uri == nullptr) return MqttTransport::Unknown; + if (strncmp(uri, "mqtts://", 8) == 0) return MqttTransport::Tls; + if (strncmp(uri, "mqtt://", 7) == 0) return MqttTransport::Tcp; + if (strncmp(uri, "wss://", 6) == 0) return MqttTransport::Wss; + if (strncmp(uri, "ws://", 5) == 0) return MqttTransport::Ws; + return MqttTransport::Unknown; +} + +static inline bool mqttTransportIsEncrypted(MqttTransport t) { + return t == MqttTransport::Tls || t == MqttTransport::Wss; +} + +static inline const char* mqttTransportName(MqttTransport t) { + switch (t) { + case MqttTransport::Tcp: return "mqtt"; + case MqttTransport::Tls: return "mqtts"; + case MqttTransport::Ws: return "ws"; + case MqttTransport::Wss: return "wss"; + default: return "?"; + } +} + +// A credential the SDK must be told to forget is written as "" rather than +// left NULL. See rule 1 above. +static inline const char* mqttFieldOrEmpty(const char* v) { return v ? v : ""; } + +// Builds an effective config, deriving the transport from the URI and +// normalising the trust policy against it: an unencrypted transport verifies +// nothing, whatever certificate material was requested, so recording anything +// else would make a plaintext endpoint look like a trust change away from a +// verified one (and vice versa). Returns false for an empty or unrecognised +// URI, leaving *out invalid. +static inline bool mqttBuildEffectiveConfig(const char* uri, + MqttAuth auth, + const char* username, + const char* password, + MqttTrust requested_trust, + const char* pem, + uint16_t buffer_size, + uint16_t keepalive, + MqttEffectiveConfig* out) { + if (out == nullptr) return false; + *out = MqttEffectiveConfig(); + if (uri == nullptr || uri[0] == '\0') return false; + if (strlen(uri) >= MQTT_EFFECTIVE_URI_MAX) return false; + + const MqttTransport transport = mqttTransportFromUri(uri); + if (transport == MqttTransport::Unknown) return false; + + strncpy(out->uri, uri, MQTT_EFFECTIVE_URI_MAX - 1); + out->uri[MQTT_EFFECTIVE_URI_MAX - 1] = '\0'; + out->transport = transport; + out->auth = auth; + out->username = username; + out->password = password; + out->buffer_size = buffer_size; + out->keepalive = keepalive; + + if (!mqttTransportIsEncrypted(transport)) { + out->trust = MqttTrust::Plaintext; + out->pem = nullptr; + } else if (requested_trust == MqttTrust::PemCert && pem != nullptr) { + out->trust = MqttTrust::PemCert; + out->pem = pem; + } else if (requested_trust == MqttTrust::Bundle) { + out->trust = MqttTrust::Bundle; + } else { + // Encrypted transport with no usable trust material. Recorded as Plaintext + // trust so it is never confused with a verified policy; whether to connect + // at all is the caller's decision. + out->trust = MqttTrust::Plaintext; + } + + out->valid = true; + return true; +} + +struct MqttRecreateDecision { + bool recreate = false; + const char* reason = "reuse"; // static literal, for logs and tests +}; + +// Whether the SDK client that `applied` was written to can be reconfigured in +// place to reach `desired`. +// +// transport (scheme) change -> recreate. One `wss`->`mqtt` transition was +// observed working by reuse on hardware, but that proves one direction +// once, not that no websocket transport state survives the switch; the +// retained-`frame_state` bug class is real and unfixed upstream even in +// IDF 5.3, and this transition only happens when an operator changes the +// endpoint. +// trust change -> recreate. `cert_pem` cannot be cleared by +// writing "" (an empty PEM is a parse failure, not "no certificate"), and +// the bundle attach is a function pointer whose set_config semantics are +// not established on this build. Verification policy is the one field +// where a stale value silently weakens security. +// capacity growth -> recreate. IDF 4.4 allocates the MQTT buffers +// in `esp_mqtt_client_init()` and `esp_mqtt_set_config()` does not resize +// them; the wrapper's own reassembly buffer is likewise allocated once. +// everything else -> reuse, including every credential and +// auth-mode change, because those are rewritten unconditionally. +static inline MqttRecreateDecision mqttConfigRecreateDecision( + const MqttEffectiveConfig& applied, + const MqttEffectiveConfig& desired, + uint16_t allocated_buffer_size) { + MqttRecreateDecision d; + if (!desired.valid) { + d.recreate = false; + d.reason = "invalid-desired"; + return d; + } + if (!applied.valid) { + // Nothing applied yet: a fresh client needs configuring, not recreating. + d.reason = "first-apply"; + return d; + } + if (applied.transport != desired.transport) { + d.recreate = true; + d.reason = "transport-change"; + return d; + } + if (applied.trust != desired.trust) { + d.recreate = true; + d.reason = "trust-change"; + return d; + } + if (desired.trust == MqttTrust::PemCert && applied.pem != desired.pem) { + d.recreate = true; + d.reason = "ca-cert-change"; + return d; + } + if (desired.buffer_size > allocated_buffer_size) { + d.recreate = true; + d.reason = "buffer-growth"; + return d; + } + return d; +} diff --git a/test/test_mqtt_effective_config/test_mqtt_effective_config.cpp b/test/test_mqtt_effective_config/test_mqtt_effective_config.cpp new file mode 100644 index 00000000..70eadb00 --- /dev/null +++ b/test/test_mqtt_effective_config/test_mqtt_effective_config.cpp @@ -0,0 +1,203 @@ +#include "helpers/MQTTEffectiveConfig.h" + +#include + +#include + +namespace { + +const char kPemA[] = "-----BEGIN CERTIFICATE-----A"; +const char kPemB[] = "-----BEGIN CERTIFICATE-----B"; +const char kJwtUser[] = "v1_CC5D3CFD9C4C7B84"; +const char kToken[] = "eyJhbGciOi..."; + +MqttEffectiveConfig build(const char* uri, + MqttAuth auth = MqttAuth::None, + const char* user = nullptr, + const char* pass = nullptr, + MqttTrust trust = MqttTrust::Plaintext, + const char* pem = nullptr, + uint16_t buffer = 896) { + MqttEffectiveConfig c; + mqttBuildEffectiveConfig(uri, auth, user, pass, trust, pem, buffer, 75, &c); + return c; +} + +MqttRecreateDecision decide(const MqttEffectiveConfig& applied, + const MqttEffectiveConfig& desired, + uint16_t allocated = 896) { + return mqttConfigRecreateDecision(applied, desired, allocated); +} + +} // namespace + +TEST(MqttEffectiveConfig, TransportComesFromTheUriScheme) { + EXPECT_EQ(MqttTransport::Tcp, mqttTransportFromUri("mqtt://broker:1883")); + EXPECT_EQ(MqttTransport::Tls, mqttTransportFromUri("mqtts://broker:8883")); + EXPECT_EQ(MqttTransport::Ws, mqttTransportFromUri("ws://broker/mqtt")); + EXPECT_EQ(MqttTransport::Wss, mqttTransportFromUri("wss://broker:443/mqtt")); + EXPECT_EQ(MqttTransport::Unknown, mqttTransportFromUri("broker:1883")); + EXPECT_EQ(MqttTransport::Unknown, mqttTransportFromUri("")); + EXPECT_EQ(MqttTransport::Unknown, mqttTransportFromUri(nullptr)); + + EXPECT_TRUE(mqttTransportIsEncrypted(MqttTransport::Tls)); + EXPECT_TRUE(mqttTransportIsEncrypted(MqttTransport::Wss)); + EXPECT_FALSE(mqttTransportIsEncrypted(MqttTransport::Tcp)); + EXPECT_FALSE(mqttTransportIsEncrypted(MqttTransport::Ws)); +} + +TEST(MqttEffectiveConfig, BuildRejectsUnusableUris) { + MqttEffectiveConfig c; + EXPECT_FALSE(mqttBuildEffectiveConfig("", MqttAuth::None, nullptr, nullptr, + MqttTrust::Plaintext, nullptr, 896, 75, &c)); + EXPECT_FALSE(c.valid); + EXPECT_FALSE(mqttBuildEffectiveConfig(nullptr, MqttAuth::None, nullptr, nullptr, + MqttTrust::Plaintext, nullptr, 896, 75, &c)); + EXPECT_FALSE(mqttBuildEffectiveConfig("broker:1883", MqttAuth::None, nullptr, nullptr, + MqttTrust::Plaintext, nullptr, 896, 75, &c)); + + char too_long[MQTT_EFFECTIVE_URI_MAX + 16]; + memset(too_long, 'x', sizeof(too_long)); + memcpy(too_long, "mqtt://", 7); + too_long[sizeof(too_long) - 1] = '\0'; + EXPECT_FALSE(mqttBuildEffectiveConfig(too_long, MqttAuth::None, nullptr, nullptr, + MqttTrust::Plaintext, nullptr, 896, 75, &c)); +} + +TEST(MqttEffectiveConfig, BuildOwnsItsUriCopy) { + char uri[64]; + strcpy(uri, "mqtt://192.168.50.231:1883"); + MqttEffectiveConfig c = build(uri); + ASSERT_TRUE(c.valid); + + strcpy(uri, "mqtt://other:1883"); // slot.broker_uri is rewritten in place + EXPECT_STREQ("mqtt://192.168.50.231:1883", c.uri); +} + +// An unencrypted transport verifies nothing, whatever certificate material was +// requested. Recording anything else would make a plaintext endpoint look like +// it kept a verified policy. +TEST(MqttEffectiveConfig, TrustIsNormalisedAgainstTheTransport) { + MqttEffectiveConfig plain = build("mqtt://broker:1883", MqttAuth::None, nullptr, nullptr, + MqttTrust::Bundle, kPemA); + EXPECT_EQ(MqttTrust::Plaintext, plain.trust); + EXPECT_EQ(nullptr, plain.pem); + + MqttEffectiveConfig bundle = build("wss://broker:443/mqtt", MqttAuth::Jwt, kJwtUser, kToken, + MqttTrust::Bundle, nullptr); + EXPECT_EQ(MqttTrust::Bundle, bundle.trust); + + MqttEffectiveConfig pem = build("mqtts://broker:8883", MqttAuth::UserPass, "u", "p", + MqttTrust::PemCert, kPemA); + EXPECT_EQ(MqttTrust::PemCert, pem.trust); + EXPECT_EQ(kPemA, pem.pem); + + // Encrypted transport with no usable material is not a verified policy. + MqttEffectiveConfig none = build("mqtts://broker:8883", MqttAuth::None, nullptr, nullptr, + MqttTrust::PemCert, nullptr); + EXPECT_EQ(MqttTrust::Plaintext, none.trust); + EXPECT_EQ(nullptr, none.pem); +} + +// The six F02 transitions from the review, plus the one reproduced on hardware. +// None of them needs a new client: credentials are rewritten unconditionally. +TEST(MqttEffectiveConfig, CredentialAndAuthChangesReuseTheClient) { + const MqttEffectiveConfig jwt = build("wss://broker:443/mqtt", MqttAuth::Jwt, kJwtUser, kToken, + MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig anon = build("wss://broker:443/mqtt", MqttAuth::None, nullptr, nullptr, + MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig userpass = build("wss://broker:443/mqtt", MqttAuth::UserPass, "u", "p", + MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig renewed = build("wss://broker:443/mqtt", MqttAuth::Jwt, kJwtUser, + "eyJhbGciOi...new", MqttTrust::Bundle, nullptr); + + EXPECT_FALSE(decide(jwt, anon).recreate); // JWT -> anonymous + EXPECT_FALSE(decide(userpass, anon).recreate); // user/pass -> anonymous + EXPECT_FALSE(decide(anon, jwt).recreate); // none -> configured + EXPECT_FALSE(decide(userpass, jwt).recreate); // user/pass -> JWT + EXPECT_FALSE(decide(jwt, renewed).recreate); // token renewal + EXPECT_FALSE(decide(jwt, jwt).recreate); // ordinary reconnect + EXPECT_STREQ("reuse", decide(jwt, jwt).reason); +} + +// Host/port/path within one scheme is an endpoint move, not a structural one. +TEST(MqttEffectiveConfig, EndpointMoveWithinOneSchemeReusesTheClient) { + const MqttEffectiveConfig a = build("mqtt://192.168.50.231:1883"); + const MqttEffectiveConfig b = build("mqtt://192.168.50.9:1884"); + EXPECT_FALSE(decide(a, b).recreate); + + const MqttEffectiveConfig wss_a = build("wss://a.example:443/mqtt", MqttAuth::Jwt, kJwtUser, + kToken, MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig wss_b = build("wss://b.example:443/other", MqttAuth::Jwt, kJwtUser, + kToken, MqttTrust::Bundle, nullptr); + EXPECT_FALSE(decide(wss_a, wss_b).recreate); +} + +TEST(MqttEffectiveConfig, TransportChangeForcesRecreate) { + const MqttEffectiveConfig wss = build("wss://broker:443/mqtt", MqttAuth::Jwt, kJwtUser, kToken, + MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig tcp = build("mqtt://192.168.50.231:1883"); + const MqttEffectiveConfig tls = build("mqtts://broker:8883", MqttAuth::UserPass, "u", "p", + MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig ws = build("ws://broker/mqtt"); + + EXPECT_TRUE(decide(wss, tcp).recreate); + EXPECT_STREQ("transport-change", decide(wss, tcp).reason); + EXPECT_TRUE(decide(tcp, wss).recreate); + EXPECT_TRUE(decide(wss, tls).recreate); // both encrypted, still a scheme change + EXPECT_TRUE(decide(tcp, ws).recreate); // both plaintext, still a scheme change +} + +TEST(MqttEffectiveConfig, TrustChangeForcesRecreate) { + const MqttEffectiveConfig bundle = build("mqtts://broker:8883", MqttAuth::None, nullptr, nullptr, + MqttTrust::Bundle, nullptr); + const MqttEffectiveConfig pem_a = build("mqtts://broker:8883", MqttAuth::None, nullptr, nullptr, + MqttTrust::PemCert, kPemA); + const MqttEffectiveConfig pem_b = build("mqtts://broker:8883", MqttAuth::None, nullptr, nullptr, + MqttTrust::PemCert, kPemB); + + EXPECT_TRUE(decide(bundle, pem_a).recreate); + EXPECT_STREQ("trust-change", decide(bundle, pem_a).reason); + EXPECT_TRUE(decide(pem_a, bundle).recreate); + EXPECT_TRUE(decide(pem_a, pem_b).recreate); + EXPECT_STREQ("ca-cert-change", decide(pem_a, pem_b).reason); + EXPECT_FALSE(decide(pem_a, pem_a).recreate); +} + +// IDF 4.4 allocates the MQTT buffers at client init and does not resize them, +// and the wrapper's reassembly buffer is allocated once for the client's life. +TEST(MqttEffectiveConfig, BufferGrowthForcesRecreateButShrinkDoesNot) { + const MqttEffectiveConfig small = build("mqtt://broker:1883", MqttAuth::None, nullptr, nullptr, + MqttTrust::Plaintext, nullptr, 512); + const MqttEffectiveConfig large = build("mqtt://broker:1883", MqttAuth::Jwt, kJwtUser, kToken, + MqttTrust::Plaintext, nullptr, 896); + + EXPECT_TRUE(decide(small, large, /*allocated=*/512).recreate); + EXPECT_STREQ("buffer-growth", decide(small, large, 512).reason); + EXPECT_FALSE(decide(small, large, /*allocated=*/896).recreate) + << "a client that already owns the capacity does not need recreating"; + EXPECT_FALSE(decide(large, small, /*allocated=*/896).recreate); +} + +TEST(MqttEffectiveConfig, FirstApplyAndInvalidDesiredNeverRecreate) { + const MqttEffectiveConfig unset; + const MqttEffectiveConfig wss = build("wss://broker:443/mqtt", MqttAuth::Jwt, kJwtUser, kToken, + MqttTrust::Bundle, nullptr); + EXPECT_FALSE(unset.valid); + EXPECT_FALSE(decide(unset, wss).recreate); + EXPECT_STREQ("first-apply", decide(unset, wss).reason); + + EXPECT_FALSE(decide(wss, unset).recreate); + EXPECT_STREQ("invalid-desired", decide(wss, unset).reason); +} + +// Rule 1: absent is a value that gets written, never an omission. +TEST(MqttEffectiveConfig, AbsentFieldsAreWrittenAsEmptyStrings) { + EXPECT_STREQ("", mqttFieldOrEmpty(nullptr)); + EXPECT_STREQ("alice", mqttFieldOrEmpty("alice")); +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}