Allow key verification to work for unknown nodes. (#10669)

* Allow key verification to work for unknown nodes. * trunk * More reliable admin key decryption * Add admin key fallback tests * Actually check haveRemoteKey * Logging cleanup * Address review feedback - Persist the committed key + manually-verified flag in commitVerifiedRemoteNode via saveToDisk(SEGMENT_NODEDATABASE), replacing the "todo: initiate save" - Guard the CryptoEngine pending-key slot with a dedicated internal lock; the Router reads it while already holding the non-recursive cryptLock, so the accessors cannot reuse that lock - Draw the security number from the hardware RNG (CryptRNG fallback) under cryptLock instead of random(); on nRF52 the entropy fill toggles the same CC310 the BLE task's packet crypto uses - Return true after fully handling the hash2 response (restores develop behavior; consistent with the hash1 branch) - Take uint32_t in the number-picker callbacks so 8-digit hex nodenums can't truncate through int - Trim over-long comments flagged by review * feat(tests): add deterministic tests for admin session-key behavior --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
2026-07-14 21:38:41 -05:00
parent fc91a69ca4
commit 952c825167
12 changed files with 632 additions and 65 deletions
@@ -1 +1 @@
-31
+32
@@ -7,9 +7,11 @@
 class AdminModuleTestShim : public AdminModule
 {
  public:
+    using AdminModule::checkPassKey; // session-key gate seam (see test_admin_session_repro)
    using AdminModule::handleReceivedProtobuf;
    using AdminModule::handleSetConfig;
    using AdminModule::handleSetModuleConfig;
+    using AdminModule::setPassKey;

    // With an "open edit transaction" saveChanges() is a pure no-op: no reloadConfig/saveToDisk/reboot.
    void deferSaves() { hasOpenEditTransaction = true; }
@@ -0,0 +1,153 @@
+// Deterministic reproduction of the admin session-key behavior discussed on PR #10669
+// (ndoo's "Admin message without session_key!" report).
+//
+// Drives the REAL incoming-admin path (AdminModule::handleReceivedProtobuf) with a remote
+// (from != 0) PKC-authorized set_owner, exercising the exact checkPassKey/setPassKey gate.
+// A local (from == 0) admin bypasses that gate, so the bug only reproduces from != 0.
+
+#include "MeshTypes.h" // include BEFORE TestUtil.h
+#include "TestUtil.h"
+#include <unity.h>
+
+#if !(MESHTASTIC_EXCLUDE_PKI)
+
+#include "mesh/Channels.h"
+#include "mesh/NodeDB.h"
+#include "modules/AdminModule.h"
+#include "support/AdminModuleTestShim.h"
+#include "support/MockMeshService.h"
+#include <cstring>
+
+static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A;
+static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B; // authorized admin, sends remote admin to us
+static const uint8_t ADMIN_KEY[32] = {0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb,
+                                      0xcc, 0xdd, 0xee, 0xff, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+                                      0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x20};
+
+static MockMeshService *mockService = nullptr;
+static AdminModuleTestShim *admin = nullptr;
+
+// A remote, PKC-authorized set_owner. `session` (if non-empty) is the session_passkey the client presents.
+static meshtastic_MeshPacket makeRemoteSetOwner(const char *newLongName, const uint8_t *session, size_t sessionLen,
+                                                meshtastic_AdminMessage &out)
+{
+    out = meshtastic_AdminMessage_init_zero;
+    out.which_payload_variant = meshtastic_AdminMessage_set_owner_tag;
+    strncpy(out.set_owner.long_name, newLongName, sizeof(out.set_owner.long_name) - 1);
+    if (session) {
+        out.session_passkey.size = sessionLen;
+        memcpy(out.session_passkey.bytes, session, sessionLen);
+    }
+
+    meshtastic_MeshPacket mp = meshtastic_MeshPacket_init_zero;
+    mp.from = ADMIN_NODE; // REMOTE: this is what makes the session gate apply
+    mp.channel = 0;
+    mp.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
+    mp.pki_encrypted = true; // arrived over PKC
+    mp.public_key.size = 32;
+    memcpy(mp.public_key.bytes, ADMIN_KEY, 32); // matches config.security.admin_key[0] -> authorized
+    return mp;
+}
+
+void setUp(void)
+{
+    mockService = new MockMeshService();
+    service = mockService;
+    admin = new AdminModuleTestShim();
+    admin->deferSaves(); // no disk/reboot side effects when a setter is accepted
+
+    if (!nodeDB)
+        nodeDB = new NodeDB();
+    myNodeInfo.my_node_num = LOCAL_NODE;
+
+    config = meshtastic_LocalConfig_init_zero;
+    // Authorize ADMIN_NODE's key as an admin key so the PKC path accepts it and we reach the session gate.
+    config.security.admin_key[0].size = 32;
+    memcpy(config.security.admin_key[0].bytes, ADMIN_KEY, 32);
+
+    owner = meshtastic_User_init_zero;
+    strncpy(owner.long_name, "Original", sizeof(owner.long_name) - 1);
+
+    channels.initDefaults();
+    channels.onConfigChanged();
+}
+
+void tearDown(void)
+{
+    service = nullptr;
+    delete mockService;
+    mockService = nullptr;
+    delete admin;
+    admin = nullptr;
+}
+
+// ndoo's report: a setter from a remote node with NO valid session is rejected, and the node's
+// expected session key is all-zero because it has minted none since boot.
+void test_remote_setter_without_session_is_rejected(void)
+{
+    meshtastic_AdminMessage m;
+    meshtastic_MeshPacket mp = makeRemoteSetOwner("Hijacked", nullptr, 0, m);
+
+    admin->handleReceivedProtobuf(mp, &m);
+    admin->drainReply();
+
+    // Rejected at the session gate -> owner unchanged (this is ndoo's "Admin message without session_key!").
+    TEST_ASSERT_EQUAL_STRING("Original", owner.long_name);
+}
+
+// The node's session key is minted only by setPassKey (which runs when it answers an admin GET),
+// so before any GET the expected key is all-zero and any presented key mismatches.
+void test_expected_session_key_is_zero_before_any_get(void)
+{
+    uint8_t zero[8] = {0};
+    meshtastic_AdminMessage probe = meshtastic_AdminMessage_init_zero;
+    probe.session_passkey.size = 8;
+    memcpy(probe.session_passkey.bytes, zero, 8); // even all-zeros must not authorize a state change
+    // A fresh module has minted no session; a stale/guessed key does not match.
+    // (checkPassKey also requires size==8 AND session_time freshness.)
+    uint8_t stale[8] = {0x29, 0x04, 0xb4, 0x78, 0xd8, 0x68, 0xa7, 0xff}; // ndoo's presented key
+    meshtastic_AdminMessage staleMsg = meshtastic_AdminMessage_init_zero;
+    staleMsg.session_passkey.size = 8;
+    memcpy(staleMsg.session_passkey.bytes, stale, 8);
+    TEST_ASSERT_FALSE(admin->checkPassKey(&staleMsg)); // Expected: 00..00 vs Incoming: 29 04 b4 78.. -> reject
+}
+
+// The fix path: once the node answers a GET (setPassKey mints/returns the key), the session gate
+// accepts a setter carrying that key. Asserting the gate (checkPassKey) directly is the mechanism;
+// driving the full handleSetOwner would need the NodeInfoModule scaffolding, out of scope here.
+void test_session_gate_accepts_key_from_a_get_response(void)
+{
+    // Simulate the node answering an admin GET: setPassKey mints the session and writes it into the response.
+    meshtastic_AdminMessage getResponse = meshtastic_AdminMessage_init_zero;
+    admin->setPassKey(&getResponse);
+    TEST_ASSERT_EQUAL(8, getResponse.session_passkey.size); // node handed the client a session key
+
+    // A setter carrying that exact key passes the gate (would be accepted).
+    meshtastic_AdminMessage good = meshtastic_AdminMessage_init_zero;
+    good.session_passkey = getResponse.session_passkey;
+    TEST_ASSERT_TRUE(admin->checkPassKey(&good));
+
+    // A setter carrying a stale/guessed key still fails (no session replay).
+    meshtastic_AdminMessage bad = meshtastic_AdminMessage_init_zero;
+    bad.session_passkey.size = 8;
+    uint8_t stale[8] = {0x29, 0x04, 0xb4, 0x78, 0xd8, 0x68, 0xa7, 0xff};
+    memcpy(bad.session_passkey.bytes, stale, 8);
+    TEST_ASSERT_FALSE(admin->checkPassKey(&bad));
+}
+
+#endif // !(MESHTASTIC_EXCLUDE_PKI)
+
+void setup()
+{
+    delay(10);
+    initializeTestEnvironment();
+    UNITY_BEGIN();
+#if !(MESHTASTIC_EXCLUDE_PKI)
+    RUN_TEST(test_remote_setter_without_session_is_rejected);
+    RUN_TEST(test_expected_session_key_is_zero_before_any_get);
+    RUN_TEST(test_session_gate_accepts_key_from_a_get_response);
+#endif
+    exit(UNITY_END());
+}
+
+void loop() {}
@@ -0,0 +1,223 @@
+// Tests for the admin-key fallback in Router::perhapsDecode: a PKI unicast from an unknown sender is
+// tried against each configured admin_key; on success the packet decodes and the key is persisted to
+// NodeDB. Drives the real crypto + NodeDB path with packets encrypted exactly as an admin radio would.
+
+#include "MeshTypes.h" // include BEFORE TestUtil.h
+#include "TestUtil.h"
+#include <unity.h>
+
+// The whole feature is compiled out when PKI is excluded.
+#if !(MESHTASTIC_EXCLUDE_PKI)
+
+#include "mesh/Channels.h"
+#include "mesh/CryptoEngine.h"
+#include "mesh/NodeDB.h"
+#include "mesh/RadioInterface.h" // MESHTASTIC_PKC_OVERHEAD
+#include "mesh/Router.h"
+#include <cstring>
+#include <pb_encode.h>
+#include <vector>
+
+static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A; // us (the receiver)
+static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B; // an authorized admin, absent from our NodeDB
+static constexpr uint32_t PKT_ID = 0x12345678;
+
+// MockNodeDB - inject nodes with controlled public keys (meshNodes/numMeshNodes are public on NodeDB).
+// Mirrors test/test_packet_signing.
+class MockNodeDB : public NodeDB
+{
+  public:
+    void clearTestNodes()
+    {
+        testNodes.clear();
+        meshNodes = &testNodes;
+        numMeshNodes = 0;
+    }
+
+    void addNode(NodeNum num)
+    {
+        meshtastic_NodeInfoLite node = meshtastic_NodeInfoLite_init_zero;
+        node.num = num;
+        testNodes.push_back(node);
+        meshNodes = &testNodes;
+        numMeshNodes = testNodes.size();
+    }
+
+    void setPublicKey(NodeNum num, const uint8_t *pubKey)
+    {
+        meshtastic_NodeInfoLite *n = getMeshNode(num);
+        TEST_ASSERT_NOT_NULL(n);
+        n->public_key.size = 32;
+        memcpy(n->public_key.bytes, pubKey, 32);
+    }
+
+    std::vector<meshtastic_NodeInfoLite> testNodes;
+};
+
+static MockNodeDB *mockNodeDB = nullptr;
+
+// Keypairs, regenerated fresh each test in setUp(). "our" == the receiver, "admin" == the sender.
+static uint8_t ourPub[32], ourPriv[32];
+static uint8_t adminPub[32], adminPriv[32];
+
+// Store a 32-byte key into config.security.admin_key[slot].
+static void setAdminKey(int slot, const uint8_t *key32)
+{
+    config.security.admin_key[slot].size = 32;
+    memcpy(config.security.admin_key[slot].bytes, key32, 32);
+    if (slot + 1 > (int)config.security.admin_key_count)
+        config.security.admin_key_count = slot + 1;
+}
+
+// Build a PKI-encrypted unicast from `from` to us, encrypted with `senderPriv` against our public key,
+// leaving the engine holding our private key afterwards (as during receive) so perhapsDecode can decrypt.
+static meshtastic_MeshPacket makePkiPacket(NodeNum from, meshtastic_PortNum port, size_t payloadLen, const uint8_t *senderPriv)
+{
+    meshtastic_Data data = meshtastic_Data_init_zero;
+    data.portnum = port;
+    data.payload.size = payloadLen;
+    for (size_t i = 0; i < payloadLen; i++)
+        data.payload.bytes[i] = (uint8_t)(i & 0xff);
+
+    uint8_t plain[meshtastic_Constants_DATA_PAYLOAD_LEN];
+    size_t plainLen = pb_encode_to_bytes(plain, sizeof(plain), &meshtastic_Data_msg, &data);
+    TEST_ASSERT_TRUE_MESSAGE(plainLen > 0, "pb_encode_to_bytes failed in test setup");
+
+    meshtastic_NodeInfoLite_public_key_t ourPubStruct;
+    ourPubStruct.size = 32;
+    memcpy(ourPubStruct.bytes, ourPub, 32);
+
+    meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero;
+    p.from = from;
+    p.to = LOCAL_NODE;
+    p.id = PKT_ID;
+    p.channel = 0; // PKI packets carry channel hash 0
+    p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag;
+
+    // Encrypt AS the sender: shared secret = DH(senderPriv, ourPub).
+    crypto->setDHPrivateKey(const_cast<uint8_t *>(senderPriv));
+    bool ok = crypto->encryptCurve25519(p.to, p.from, ourPubStruct, p.id, plainLen, plain, p.encrypted.bytes);
+    TEST_ASSERT_TRUE_MESSAGE(ok, "encryptCurve25519 failed in test setup");
+    p.encrypted.size = plainLen + MESHTASTIC_PKC_OVERHEAD;
+
+    // Restore the engine to our private key, as it is when receiving.
+    crypto->setDHPrivateKey(ourPriv);
+    return p;
+}
+
+// Assert the packet decoded via PKI and that we learned `expectedKey` for its sender.
+static void assertDecodedAndLearned(meshtastic_MeshPacket *p, const uint8_t *expectedKey)
+{
+    TEST_ASSERT_EQUAL(meshtastic_MeshPacket_decoded_tag, p->which_payload_variant);
+    TEST_ASSERT_TRUE(p->pki_encrypted);
+    TEST_ASSERT_EQUAL(meshtastic_PortNum_PRIVATE_APP, p->decoded.portnum);
+    TEST_ASSERT_EQUAL(32, p->public_key.size);
+    TEST_ASSERT_EQUAL_MEMORY(expectedKey, p->public_key.bytes, 32);
+
+    meshtastic_NodeInfoLite *learned = mockNodeDB->getMeshNode(p->from);
+    TEST_ASSERT_NOT_NULL_MESSAGE(learned, "sender should have been created in NodeDB");
+    TEST_ASSERT_EQUAL_MESSAGE(32, learned->public_key.size, "sender key should have been persisted");
+    TEST_ASSERT_EQUAL_MEMORY_MESSAGE(expectedKey, learned->public_key.bytes, 32, "persisted key mismatch");
+}
+
+void setUp(void)
+{
+    // Construct the mock FIRST: the NodeDB ctor can reload persisted host state and repopulate globals.
+    mockNodeDB = new MockNodeDB();
+    mockNodeDB->clearTestNodes();
+    nodeDB = mockNodeDB;
+
+    config = meshtastic_LocalConfig_init_zero;
+    owner = meshtastic_User_init_zero;
+    myNodeInfo.my_node_num = LOCAL_NODE; // drives isToUs()/getFrom()
+
+    channels.initDefaults();
+    channels.onConfigChanged();
+
+    // Fresh keypairs for us and the admin (independent, valid Curve25519 pairs).
+    crypto->generateKeyPair(ourPub, ourPriv);
+    crypto->generateKeyPair(adminPub, adminPriv);
+
+    // perhapsDecode's PKI gate requires that we have our own key (getMeshNode(p->to)->public_key).
+    mockNodeDB->addNode(LOCAL_NODE);
+    mockNodeDB->setPublicKey(LOCAL_NODE, ourPub);
+
+    // During receive the engine holds our private key.
+    crypto->setDHPrivateKey(ourPriv);
+}
+
+void tearDown(void)
+{
+    delete mockNodeDB;
+    mockNodeDB = nullptr;
+    nodeDB = nullptr;
+}
+
+// Admin key in slot 0: a DM from an unknown sender decrypts via the fallback, and the key is persisted.
+void test_admin_key_slot0_decrypts_and_persists(void)
+{
+    setAdminKey(0, adminPub);
+    TEST_ASSERT_NULL_MESSAGE(mockNodeDB->getMeshNode(ADMIN_NODE), "precondition: sender is unknown to us");
+
+    meshtastic_MeshPacket p = makePkiPacket(ADMIN_NODE, meshtastic_PortNum_PRIVATE_APP, 16, adminPriv);
+    TEST_ASSERT_EQUAL(DECODE_SUCCESS, perhapsDecode(&p));
+
+    assertDecodedAndLearned(&p, adminPub);
+}
+
+// The loop scans every admin slot, not just [0]: a key provisioned only in slot 2 still works.
+void test_admin_key_slot2_only_decrypts(void)
+{
+    setAdminKey(2, adminPub); // slots 0 and 1 left empty
+
+    meshtastic_MeshPacket p = makePkiPacket(ADMIN_NODE, meshtastic_PortNum_PRIVATE_APP, 16, adminPriv);
+    TEST_ASSERT_EQUAL(DECODE_SUCCESS, perhapsDecode(&p));
+
+    assertDecodedAndLearned(&p, adminPub);
+}
+
+// No admin key configured + unknown sender: nothing decrypts, and we must NOT invent a key for anyone.
+void test_no_admin_key_unknown_sender_not_decoded(void)
+{
+    // config (incl. admin_key) is zeroed by setUp(); ADMIN_NODE is absent from NodeDB.
+    meshtastic_MeshPacket p = makePkiPacket(ADMIN_NODE, meshtastic_PortNum_PRIVATE_APP, 16, adminPriv);
+
+    TEST_ASSERT_NOT_EQUAL(DECODE_SUCCESS, perhapsDecode(&p));
+    TEST_ASSERT_NOT_EQUAL(meshtastic_MeshPacket_decoded_tag, p.which_payload_variant);
+    TEST_ASSERT_NULL_MESSAGE(mockNodeDB->getMeshNode(ADMIN_NODE), "must not learn a key when nothing decrypted");
+}
+
+// A configured admin key that is NOT the sender's must fail authentication (no bogus key learned).
+void test_wrong_admin_key_does_not_decode(void)
+{
+    uint8_t otherPub[32], otherPriv[32];
+    crypto->generateKeyPair(otherPub, otherPriv); // unrelated key
+    crypto->setDHPrivateKey(ourPriv);             // restore receive key (generateKeyPair changed it)
+    setAdminKey(0, otherPub);                     // admin slot holds a key that did NOT encrypt the packet
+
+    meshtastic_MeshPacket p = makePkiPacket(ADMIN_NODE, meshtastic_PortNum_PRIVATE_APP, 16, adminPriv);
+
+    TEST_ASSERT_NOT_EQUAL(DECODE_SUCCESS, perhapsDecode(&p));
+    TEST_ASSERT_NOT_EQUAL(meshtastic_MeshPacket_decoded_tag, p.which_payload_variant);
+    TEST_ASSERT_NULL(mockNodeDB->getMeshNode(ADMIN_NODE));
+}
+
+#endif // !(MESHTASTIC_EXCLUDE_PKI)
+
+void setup()
+{
+    delay(10);
+    delay(2000);
+
+    initializeTestEnvironment();
+    UNITY_BEGIN();
+#if !(MESHTASTIC_EXCLUDE_PKI)
+    RUN_TEST(test_admin_key_slot0_decrypts_and_persists);
+    RUN_TEST(test_admin_key_slot2_only_decrypts);
+    RUN_TEST(test_no_admin_key_unknown_sender_not_decoded);
+    RUN_TEST(test_wrong_admin_key_does_not_decode);
+#endif
+    exit(UNITY_END());
+}
+
+void loop() {}