kevin/meshtastic_firmware
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

Return the secret sentinel for remote admin config gets (#11093)

Browse Source 
writeSecret is a setter, so calling it on the NETWORK_CONFIG get path was a no-op: the buffer
already holds the stored psk, never the sentinel. MQTT_CONFIG returned the broker password
verbatim.

Both get paths now return secretReserved when req.from != 0, and the matching set paths call
writeSecret so a read-modify-write round trip keeps the stored value.
This commit is contained in:
Thomas Göttgens
2026-07-20 18:38:23 -05:00
committed by GitHub
co-authored by GitHub
parent fb75410e53
commit 6f522aad17
3 changed files with 124 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/modules/AdminModule.cpp
+17 -4
View File
@@ -929,11 +929,15 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
config.power.on_battery_shutdown_after_secs = 30;
}
break;
case meshtastic_Config_network_tag:
case meshtastic_Config_network_tag: {
LOG_INFO("Set config: WiFi");
config.has_network = true;
char prevPsk[sizeof(config.network.wifi_psk)];
memcpy(prevPsk, config.network.wifi_psk, sizeof(prevPsk));
config.network = c.payload_variant.network;
writeSecret(config.network.wifi_psk, sizeof(config.network.wifi_psk), prevPsk);
break;
}
case meshtastic_Config_display_tag:
LOG_INFO("Set config: Display");
config.has_display = true;
@@ -1194,7 +1198,12 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
// Disable Bluetooth to prevent interference during MQTT configuration
disableBluetooth();
moduleConfig.has_mqtt = true;
moduleConfig.mqtt = c.payload_variant.mqtt;
{
char prevPass[sizeof(moduleConfig.mqtt.password)];
memcpy(prevPass, moduleConfig.mqtt.password, sizeof(prevPass));
moduleConfig.mqtt = c.payload_variant.mqtt;
writeSecret(moduleConfig.mqtt.password, sizeof(moduleConfig.mqtt.password), prevPass);
}
#endif
break;
case meshtastic_ModuleConfig_serial_tag:
@@ -1451,8 +1460,9 @@ void AdminModule::handleGetConfig(const meshtastic_MeshPacket &req, const uint32
LOG_INFO("Get config: Network");
res.get_config_response.which_payload_variant = meshtastic_Config_network_tag;
res.get_config_response.payload_variant.network = config.network;
writeSecret(res.get_config_response.payload_variant.network.wifi_psk,
sizeof(res.get_config_response.payload_variant.network.wifi_psk), config.network.wifi_psk);
if (req.from != 0)
strncpy(res.get_config_response.payload_variant.network.wifi_psk, secretReserved,
sizeof(res.get_config_response.payload_variant.network.wifi_psk));
break;
case meshtastic_AdminMessage_ConfigType_DISPLAY_CONFIG:
LOG_INFO("Get config: Display");
@@ -1523,6 +1533,9 @@ void AdminModule::handleGetModuleConfig(const meshtastic_MeshPacket &req, const
configName = "MQTT";
res.get_module_config_response.which_payload_variant = meshtastic_ModuleConfig_mqtt_tag;
res.get_module_config_response.payload_variant.mqtt = moduleConfig.mqtt;
if (req.from != 0)
strncpy(res.get_module_config_response.payload_variant.mqtt.password, secretReserved,
sizeof(res.get_module_config_response.payload_variant.mqtt.password));
break;
case meshtastic_AdminMessage_ModuleConfigType_SERIAL_CONFIG:
configName = "Serial";
test/support/AdminModuleTestShim.h
+1
View File
@@ -9,6 +9,7 @@ class AdminModuleTestShim : public AdminModule
public:
using AdminModule::checkPassKey; // session-key gate seam (see test_admin_session_repro)
using AdminModule::handleGetConfig;
using AdminModule::handleGetModuleConfig;
using AdminModule::handleReceivedProtobuf;
using AdminModule::handleSetConfig;
using AdminModule::handleSetModuleConfig;
test/test_admin_session_repro/test_main.cpp
+106
View File
@@ -250,6 +250,107 @@ void test_local_security_config_keeps_private_key(void)
admin->drainReply();
}
// Decode the NetworkConfig / MqttConfig out of the response a handler queued in myReply.
static bool decodeNetworkFromReply(meshtastic_MeshPacket *reply, meshtastic_Config_NetworkConfig &out)
{
meshtastic_AdminMessage am = meshtastic_AdminMessage_init_zero;
if (!reply || reply->which_payload_variant != meshtastic_MeshPacket_decoded_tag)
return false;
if (!pb_decode_from_bytes(reply->decoded.payload.bytes, reply->decoded.payload.size, &meshtastic_AdminMessage_msg, &am))
return false;
if (am.which_payload_variant != meshtastic_AdminMessage_get_config_response_tag ||
am.get_config_response.which_payload_variant != meshtastic_Config_network_tag)
return false;
out = am.get_config_response.payload_variant.network;
return true;
}
static bool decodeMqttFromReply(meshtastic_MeshPacket *reply, meshtastic_ModuleConfig_MQTTConfig &out)
{
meshtastic_AdminMessage am = meshtastic_AdminMessage_init_zero;
if (!reply || reply->which_payload_variant != meshtastic_MeshPacket_decoded_tag)
return false;
if (!pb_decode_from_bytes(reply->decoded.payload.bytes, reply->decoded.payload.size, &meshtastic_AdminMessage_msg, &am))
return false;
if (am.which_payload_variant != meshtastic_AdminMessage_get_module_config_response_tag ||
am.get_module_config_response.which_payload_variant != meshtastic_ModuleConfig_mqtt_tag)
return false;
out = am.get_module_config_response.payload_variant.mqtt;
return true;
}
// A remote requester gets the sentinel; the set path swaps the stored value back.
void test_remote_network_config_omits_wifi_psk(void)
{
strcpy(config.network.wifi_psk, "hunter2hunter2");
meshtastic_MeshPacket req = makeGetConfigRequest(ADMIN_NODE);
admin->handleGetConfig(req, meshtastic_AdminMessage_ConfigType_NETWORK_CONFIG);
meshtastic_Config_NetworkConfig net;
TEST_ASSERT_TRUE(decodeNetworkFromReply(admin->reply(), net));
TEST_ASSERT_EQUAL_STRING_MESSAGE("sekrit", net.wifi_psk, "remote network config must not carry the real psk");
admin->drainReply();
}
// Control: the local path still receives the stored psk.
void test_local_network_config_keeps_wifi_psk(void)
{
strcpy(config.network.wifi_psk, "hunter2hunter2");
meshtastic_MeshPacket req = makeGetConfigRequest(0);
admin->handleGetConfig(req, meshtastic_AdminMessage_ConfigType_NETWORK_CONFIG);
meshtastic_Config_NetworkConfig net;
TEST_ASSERT_TRUE(decodeNetworkFromReply(admin->reply(), net));
TEST_ASSERT_EQUAL_STRING_MESSAGE("hunter2hunter2", net.wifi_psk, "local client must still receive the psk");
admin->drainReply();
}
void test_remote_mqtt_config_omits_password(void)
{
strcpy(moduleConfig.mqtt.password, "brokerpass");
meshtastic_MeshPacket req = makeGetConfigRequest(ADMIN_NODE);
admin->handleGetModuleConfig(req, meshtastic_AdminMessage_ModuleConfigType_MQTT_CONFIG);
meshtastic_ModuleConfig_MQTTConfig mqtt;
TEST_ASSERT_TRUE(decodeMqttFromReply(admin->reply(), mqtt));
TEST_ASSERT_EQUAL_STRING_MESSAGE("sekrit", mqtt.password, "remote mqtt config must not carry the broker password");
admin->drainReply();
}
void test_local_mqtt_config_keeps_password(void)
{
strcpy(moduleConfig.mqtt.password, "brokerpass");
meshtastic_MeshPacket req = makeGetConfigRequest(0);
admin->handleGetModuleConfig(req, meshtastic_AdminMessage_ModuleConfigType_MQTT_CONFIG);
meshtastic_ModuleConfig_MQTTConfig mqtt;
TEST_ASSERT_TRUE(decodeMqttFromReply(admin->reply(), mqtt));
TEST_ASSERT_EQUAL_STRING_MESSAGE("brokerpass", mqtt.password, "local client must still receive the password");
admin->drainReply();
}
// A client that GETs and writes the config straight back must not wipe the stored value.
void test_set_config_sentinel_psk_preserves_stored_value(void)
{
strcpy(config.network.wifi_psk, "hunter2hunter2");
meshtastic_Config c = meshtastic_Config_init_zero;
c.which_payload_variant = meshtastic_Config_network_tag;
c.payload_variant.network = config.network;
strcpy(c.payload_variant.network.wifi_psk, "sekrit");
admin->deferSaves();
admin->handleSetConfig(c, true);
TEST_ASSERT_EQUAL_STRING_MESSAGE("hunter2hunter2", config.network.wifi_psk,
"a read-modify-write round trip must not wipe the psk");
admin->drainReply();
}
// An admin response carries no session passkey and its sender is not an admin-key holder, so a
// request we sent is the only thing vouching for it. A get_module_config_response from a node we
// never queried is not.
@@ -431,6 +532,11 @@ void setup()
RUN_TEST(test_session_gate_accepts_key_from_a_get_response);
RUN_TEST(test_remote_security_config_omits_private_key);
RUN_TEST(test_local_security_config_keeps_private_key);
RUN_TEST(test_remote_network_config_omits_wifi_psk);
RUN_TEST(test_local_network_config_keeps_wifi_psk);
RUN_TEST(test_remote_mqtt_config_omits_password);
RUN_TEST(test_local_mqtt_config_keeps_password);
RUN_TEST(test_set_config_sentinel_psk_preserves_stored_value);
RUN_TEST(test_unsolicited_response_is_not_solicited);
RUN_TEST(test_response_after_our_request_is_solicited);
RUN_TEST(test_request_to_one_node_does_not_admit_another);