diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index bdfd4bb51..dadabc491 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -815,6 +815,23 @@ static void reconcileAccelerometerThread(bool wasOn, bool nowOn, bool otherFeatu } #endif +// A "regenerate keys" client sends a blank SecurityConfig holding only the new private key, rather than the +// config it read from us. Detect that shape - new private key, every other field at its proto default - so it +// isn't mistaken for "and clear everything else". +static bool isBareKeypairRotation(const meshtastic_Config_SecurityConfig &incoming, + const meshtastic_Config_SecurityConfig ¤t) +{ + if (incoming.private_key.size != 32) + return false; + if (current.private_key.size == 32 && memcmp(incoming.private_key.bytes, current.private_key.bytes, 32) == 0) + return false; + + return incoming.admin_key_count == 0 && !incoming.is_managed && !incoming.serial_enabled && !incoming.debug_log_api_enabled && + !incoming.admin_channel_enabled && + incoming.packet_signature_policy == + meshtastic_Config_SecurityConfig_PacketSignaturePolicy_PACKET_SIGNATURE_POLICY_COMPATIBLE; +} + void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) { auto changes = SEGMENT_CONFIG; @@ -1104,6 +1121,16 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) incoming.private_key = config.security.private_key; incoming.public_key = config.security.public_key; } + // Rotating the keypair must not drop the admin keys - that locks the owner out of remote admin with no + // recourse but a physical connection. Clearing admin keys still works via a SET that leaves the private + // key alone and sends an empty list. + if (isBareKeypairRotation(incoming, config.security)) { + LOG_INFO("Security set is a bare keypair rotation; preserving remaining security config"); + meshtastic_Config_SecurityConfig rotated = config.security; + rotated.public_key = incoming.public_key; // usually empty; derived from the private key below + rotated.private_key = incoming.private_key; + incoming = rotated; + } config.security = incoming; #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN) && !(MESHTASTIC_EXCLUDE_PKI) // First provisioning (no key) generates one; a private key supplied without its public key derives it. diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index 3ce99fcb6..673d4590f 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -1197,6 +1197,80 @@ static void test_handleSetConfig_security_acceptsSuppliedKeypair() TEST_ASSERT_EQUAL_MEMORY(expectedPub, config.security.public_key.bytes, 32); } +// Issue #11073: "regenerate keys" sends a blank SecurityConfig holding only the new private key. Replacing +// the whole struct with it wiped the admin keys, locking the owner out of remote admin. +static void test_handleSetConfig_security_rotationPreservesAdminKeys() +{ + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0x11, 32); + config.security.public_key.size = 32; + memset(config.security.public_key.bytes, 0x22, 32); + config.security.admin_key_count = 2; + config.security.admin_key[0].size = 32; + memset(config.security.admin_key[0].bytes, 0xAA, 32); + config.security.admin_key[1].size = 32; + memset(config.security.admin_key[1].bytes, 0xBB, 32); + config.security.is_managed = true; + config.security.serial_enabled = true; + config.security.packet_signature_policy = + meshtastic_Config_SecurityConfig_PacketSignaturePolicy_PACKET_SIGNATURE_POLICY_STRICT; + + // Exactly what the regenerate dialog emits. + meshtastic_Config c = meshtastic_Config_init_zero; + c.which_payload_variant = meshtastic_Config_security_tag; + c.payload_variant.security.private_key.size = 32; + memset(c.payload_variant.security.private_key.bytes, 0x33, 32); + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + uint8_t expectedPriv[32]; + memset(expectedPriv, 0x33, 32); + TEST_ASSERT_EQUAL_UINT(32, config.security.private_key.size); + TEST_ASSERT_EQUAL_MEMORY(expectedPriv, config.security.private_key.bytes, 32); + + uint8_t expectedAdmin0[32], expectedAdmin1[32]; + memset(expectedAdmin0, 0xAA, 32); + memset(expectedAdmin1, 0xBB, 32); + TEST_ASSERT_EQUAL_UINT(2, config.security.admin_key_count); + TEST_ASSERT_EQUAL_UINT(32, config.security.admin_key[0].size); + TEST_ASSERT_EQUAL_MEMORY(expectedAdmin0, config.security.admin_key[0].bytes, 32); + TEST_ASSERT_EQUAL_UINT(32, config.security.admin_key[1].size); + TEST_ASSERT_EQUAL_MEMORY(expectedAdmin1, config.security.admin_key[1].bytes, 32); + TEST_ASSERT_TRUE(config.security.is_managed); + TEST_ASSERT_TRUE(config.security.serial_enabled); + TEST_ASSERT_EQUAL(meshtastic_Config_SecurityConfig_PacketSignaturePolicy_PACKET_SIGNATURE_POLICY_STRICT, + config.security.packet_signature_policy); +} + +// The escape hatch: a SET that leaves the private key alone still clears admin keys. +static void test_handleSetConfig_security_clearsAdminKeysWhenKeypairUnchanged() +{ + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0x11, 32); + config.security.public_key.size = 32; + memset(config.security.public_key.bytes, 0x22, 32); + config.security.admin_key_count = 1; + config.security.admin_key[0].size = 32; + memset(config.security.admin_key[0].bytes, 0xAA, 32); + + // Same private key we already hold, empty admin key list. + meshtastic_Config c = meshtastic_Config_init_zero; + c.which_payload_variant = meshtastic_Config_security_tag; + c.payload_variant.security.private_key.size = 32; + memset(c.payload_variant.security.private_key.bytes, 0x11, 32); + c.payload_variant.security.public_key.size = 32; + memset(c.payload_variant.security.public_key.bytes, 0x22, 32); + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_EQUAL_UINT(0, config.security.admin_key_count); + TEST_ASSERT_EQUAL_UINT(0, config.security.admin_key[0].size); +} + static void test_regionInfo_supportsPreset() { const RegionInfo *eu868 = getRegion(meshtastic_Config_LoRaConfig_RegionCode_EU_868); @@ -1540,6 +1614,8 @@ void setup() RUN_TEST(test_handleSetConfig_fromLocal_customBandwidthNonZeroPreserved); RUN_TEST(test_handleSetConfig_security_preservesKeypairWhenPrivateOmitted); RUN_TEST(test_handleSetConfig_security_acceptsSuppliedKeypair); + RUN_TEST(test_handleSetConfig_security_rotationPreservesAdminKeys); + RUN_TEST(test_handleSetConfig_security_clearsAdminKeysWhenKeypairUnchanged); RUN_TEST(test_regionInfo_supportsPreset); RUN_TEST(test_checkConfigRegion_quietCheckReportsReason); RUN_TEST(test_handleSetConfig_fromOthers_siblingLockedPresetSwapsRegion);