* fix(pki): re-derive NodeNum when setting a region mints the identity key
A node's mesh address is derived from its identity key:
my_node_num == crc32Buffer(config.security.public_key.bytes, 32)
NodeDB::createNewIdentity() is what establishes that, and NodeDB::
generateCryptoKeyPair() is the only thing that called it.
CryptoEngine::ensurePkiKeys() generates or re-derives the keypair and writes
security.public_key, security.private_key and user.public_key - but never
re-derives my_node_num. Boot-time keygen is suppressed while the LoRa region is
UNSET (generateCryptoKeyPair()'s regionBlocksKeygen guard), so on a fresh device
my_node_num is still the MAC-derived value from pickNewNodeNum(). The user then
sets the region - the stock onboarding flow - ensurePkiKeys() mints a key, and
the invariant is broken.
The node then signs its broadcasts (Router.cpp signs when !pki_encrypted &&
(owner.is_licensed || isBroadcast(p->to))). Every receiver runs
verifyFirstContactNodeInfo, fails crc32Buffer(user.public_key) != p->from, and
drops the NodeInfo. The node's identity beacons are invisible to the mesh.
Nothing reboots to repair it: AdminModule sets requiresReboot = false for LoRa
changes ("All LoRa radio changes apply live via configChanged observer") and
MenuHandler ends at service->reloadConfig(changes).
Four call sites reached ensurePkiKeys():
1. AdminModule set_config LORA, region first set (phone app - the common path)
2. MenuHandler applyLoraRegion (on-device region picker)
3. InkHUD MenuApplet applyLoRaRegion (schedules a reboot, so it
self-healed at next boot)
4. portduino wasm wasm_set_region
The reference implementation was already in the tree: the *licensed* branch of
call site 1, thirteen lines below the broken unlicensed one, calls
nodeDB->generateCryptoKeyPair() (which reaches createNewIdentity()) and widens
the persisted mask with SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE.
Rather than repeat that at four call sites, the key-mint is routed through one
chokepoint that owns both halves of the identity: NodeDB::ensurePkiIdentity()
calls crypto->ensurePkiKeys() and then createNewIdentity(). It lives in NodeDB
because createNewIdentity() operates on the devicestate/node-DB globals, which
CryptoEngine deliberately does not touch - ensurePkiKeys() takes the security
config and user by reference precisely so it stays free of that dependency, and
it is unit-tested against a standalone CryptoEngine.
ensurePkiIdentity() returns true only when my_node_num actually moved
(createNewIdentity() early-returns when the key is unchanged, so a repeat region
change does not disturb the self entry or force a needless flash write). Callers
use that to widen their save mask; my_node_num lives in devicestate and the self
row moves in the node DB, so both segments must be persisted or the fix would
revert at the next boot. SEGMENT_CONFIG, which carries the key itself, is
already unconditional on all four paths.
The InkHUD reboot is left as-is. It is now redundant for this invariant, but it
covers the rest of that menu's behaviour and a redundant reboot is not a bug.
Adds test_handleSetConfig_persistsUnlicensedFirstRegionIdentity, the unlicensed
twin of the existing licensed test, asserting both the segment mask and
my_node_num == crc32(public_key).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(NodeDB): trim identity-recovery comments and guard the WASM nodeDB deref
Two review asks, no behaviour change on any built target.
Copilot flagged the unguarded nodeDB deref in the WASM region setter; it is the
only ensurePkiIdentity() call site that did not check the pointer first.
The rest is comment length. AGENTS.md:83 caps code comments at two lines, and the
identity-recovery comments across the four call sites plus the NodeDB.h doc block
ran to four and six lines. The rationale they carried is in the commit messages
and the PR body, which is where AGENTS.md says it belongs.
The PR's own fix in AdminModule.cpp is deliberately untouched.
* fix(NodeDB): keep the identity move authoritative when the self record cannot be created
createNewIdentity() removes the old node entry and assigns myNodeInfo.my_node_num
before it tries to create the row for the new number. If getOrCreateMeshNode()
came back null it returned false, so the first-region callers left
SEGMENT_DEVICESTATE and SEGMENT_NODEDATABASE out of the save mask.
The number had already moved in RAM at that point, and the freshly minted key
goes to flash under SEGMENT_CONFIG regardless. The next boot therefore reloads
the old number alongside the new key, which is exactly the
crc32(public_key) != my_node_num break this path exists to prevent, reached
through the error branch instead of the happy one.
Rolling the number back is not an option either, since the key has already been
replaced by the time this runs. So the move is now reported as the fact it is and
the missing self record is logged separately; getOrCreateMeshNode() will recreate
that row on the next contact. Reachable when the self record is absent and the
table is full of protected nodes.
Reported by CodeRabbit on #11426.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(NodeDB): require a full 32-byte key when demoting to the warm tier
meshtastic_User.public_key is a wire `bytes` field with max_size 32, so any
size in 0..32 decodes off the air, and nothing validates it on ingress:
NodeInfoModule hands the decoded User straight to NodeDB::updateUser, whose
PKI gates are all `== 32` and so fall through for a partial key, and
TypeConversions::CopyUserToNodeInfoLite then stores it with the short size.
demoteOldestHotNodesToWarm() admitted that partial key into the warm tier on
a `size > 0` gate. WarmNodeEntry has no length field - it distinguishes "has
a key" from "no key" purely by all-zero - so N real bytes plus 32-N zeros
become indistinguishable from a genuine key. copyPublicKeyAuthoritative()
then hands that fabricated key back with size = 32 and reports it
AUTHORITATIVE, and re-admission writes size = 32 into the hot store. From
then on updateUser's key pin permanently rejects the node's real NodeInfo,
and DMs to it are encrypted to a key nobody holds.
Require a full 32-byte key, so a partial one is absorbed as "no key"
(nullptr) rather than as a truncated one. WarmNodeStore::place() already
treats a null key as keyless and clears the slot's stale key when
repurposing it. This aligns the site with its two siblings, which both
already gate on `size == 32` (the purge path in cleanupMeshDB and the
runtime eviction in getOrCreateMeshNode).
The ingress gap - updateUser accepting a 1..31-byte key at all - is a
separate, larger change and is left for its own review.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(NodeDB): shorten warm-demotion comment to two lines
Repo guideline (AGENTS.md): keep code comments to one or two lines. Retains the
non-obvious invariant - warm entries have no key length field - and drops the
restated detail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(NodeDB): cover short-key demotion into the warm tier
A warm record stores 32 raw key bytes with no length field, so a partial
hot-store key is indistinguishable from a real one once demoted. The
public_key.size == 32 gate in demoteOldestHotNodesToWarm() is what keeps a
truncated key from being laundered into a full-looking warm key, but nothing
exercised it.
test_migration_dropsShortKeyOnDemotion overflows the hot store with one node
carrying a 31-byte key and asserts it lands as a keyless placeholder while a
genuine 32-byte key still survives. push() grows a keySize parameter to seed
the partial key, and clearWarm() gives the test an empty warm tier, which it
needs because the warm store outlives setUp() and a prior run's warm.dat.
Verified to discriminate: with the size gate reverted to size > 0 the new
test fails on "a 31-byte key must not be demoted as if it were a full key",
and passes again once restored.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(NodeDB): assert the keyless placeholder carries last_heard
The test only proved a warm metadata row survived the demotion, not that the
placeholder does the job the nullptr is there for, which is preserving
last_heard when the key is dropped.
Asserting the value needed the seeds fixing first. Warm entries pack role,
protected category and the xeddsa flag into the low 7 bits of last_heard
(WARM_TIME_MASK is 0xFFFFFF80), so warm time has 128 second granularity and the
old seeds of 1, 2, 3 all quantised to 0. They are now multiples of 128, which
keeps the demotion ordering identical and makes the values survive the round
trip. Real last_heard is epoch seconds, so this is closer to production than
the old counter was.
Reads the entry through WarmNodeStore::take() rather than getOrCreateMeshNode(),
which does not restore last_heard from the warm tier and would have been
asserting a path that does not exist.
Reported by CodeRabbit on #11431.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
installDefaultConfig() restores a preserved identity key when the config
is reset with preserveKey=true. On that path it called:
printBytes("Restored key", config.security.private_key.bytes,
config.security.private_key.size);
printBytes() hex-dumps the buffer to LOG_DEBUG, so this emitted all 32
bytes of the raw X25519 identity private key to the serial/BLE debug log.
Debug logs are not a private channel. They are routinely captured over
serial or BLE and pasted verbatim into GitHub issues, Discord threads and
support requests. Anyone who reads such a log recovers the node's identity
private key, and can then impersonate the node and decrypt every PKI direct
message addressed to it -- past messages included, since the key is
long-lived and the DH shared secret is static per node pair. There is no
revocation story short of generating a new identity.
Replaced with a LOG_DEBUG that records that a key was restored and contains
no key-derived bytes. The restore/no-restore signal is the genuinely useful
diagnostic here ("did my key survive the reset?"), and it costs nothing to
keep; the bytes were never what made the line useful. Log level is unchanged
-- printBytes() already logged at LOG_DEBUG.
Deliberately NOT a truncated prefix or a hash. A prefix is still key
material: it hands an attacker free bytes and shrinks the search space.
A hash is a confirmation oracle -- it lets anyone holding a candidate key
verify it against the log, which is exactly the check an attacker needs.
Neither is a compromise; both leak. If a log line survives at all it must
carry zero key-derived bytes.
Sites changed:
- src/mesh/NodeDB.cpp:988 -- the only full private-key dump in src/.
Audited and deliberately left alone:
- NodeDB.cpp:3552,3604 ("Incoming Pubkey", "Saved Pubkey") -- public keys,
published to the mesh by design; not secret.
- CryptoEngine.cpp:245,285 -- nonces, not key material.
- CryptoEngine.cpp:246,286 -- first 8 bytes of the derived shared_key, and
AdminModule.cpp:2006,2013,2014 -- the 8-byte admin session passkey.
Both are secrets rather than public values, but neither is the identity
private key and both are out of scope for this fix; noted for follow-up.
No unused-variable fallout: private_key_temp is still read by the memcpy
above, and printBytes() is still used by the two pubkey sites, so the
meshUtils.h include is still required.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Clients send `add_contact` before every text-message DM, because a phone
often holds a larger contact database (with public keys) than the radio
can keep. That makes `addFromContact()` the highest-volume key-write path
on the device - and it had no protection against key erasure.
Its only key guard covered the manually-verified case: if the local entry
was marked manually verified and the incoming contact was not, a key
mismatch aborted the update. Every ordinary entry fell straight through to
`CopyUserToNodeInfoLite()`, which assigns `public_key` unconditionally. So
a SharedContact with `has_user` set and an empty `public_key` overwrote a
peer's stored, XEdDSA-proven key with zeros - and `addFromContact()` calls
`saveNodeDatabaseToDisk()`, so the erasure survived a reboot. Subsequent
DMs to that peer then failed with PKI_SEND_FAIL_PUBLIC_KEY, with no way to
recover until the peer's NodeInfo was re-exchanged.
`public_key` is a singular (non-optional) bytes field, so "absent" and
"empty" both decode to size 0; a client that simply has no key for a
contact is indistinguishable on the wire from one asking to clear it.
The fix is deliberately narrow: keep the stored key when the entry already
holds a full 32-byte key and the incoming contact does not. A well-formed
32-byte contact key still updates the entry exactly as before.
Deliberately NOT changed here:
- `updateUser()`'s first-key-wins pin is not applied to this path. Clients
legitimately use add_contact to supply keys the radio never had and to
update them (QR-code contact sharing); a blanket pin would break that
documented flow. Only erasure is refused.
- `CopyUserToNodeInfoLite()` itself is untouched - it has many other
callers (self-record refresh, updateUser, warm-tier rehydration), so the
guard lives at this call site.
- The manually-verified branch is unchanged.
- Node-number validation (reserved/broadcast/self) on this path remains
open and is tracked separately.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* esp32: release BTDM heap when Bluetooth inactive
Release ESP-IDF BTDM memory after config load when Bluetooth is disabled or WiFi is enabled, recovering heap on ESP32 targets where BLE won’t be used for this boot.
* Address BT memory release review comments
---------
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Make the PowerFSM DARK-to-LS timeout immediate when Bluetooth support is compiled out or config.bluetooth.enabled is false. The configured wait_bluetooth_secs default behavior is unchanged when Bluetooth is enabled.
* fix: release Heltec v4 FEM sleep holds
* fix: increase FEM power settle delay
* Fix heltec_V4 documentation
Fixing the heltec_v4 documentation for enabling the PA after sleep.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Add comment for the next guy about why 5ms was chosen.
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
* Add ESP32 Power Management lessons learned document
Documents our experimentation with ESP-IDF DFS and why it doesn't
work well for Meshtastic (RTOS locks, BLE locks, USB issues).
Proposes simpler alternative: manual setCpuFrequencyMhz() control
with explicit triggers for when to go fast vs slow.
* docs(prompts): fix markdown fence language tags
* docs: remove ESP32 power management notes
* Add ESP32 Power Management lessons learned document
Documents our experimentation with ESP-IDF DFS and why it doesn't
work well for Meshtastic (RTOS locks, BLE locks, USB issues).
Proposes simpler alternative: manual setCpuFrequencyMhz() control
with explicit triggers for when to go fast vs slow.
* docs(prompts): fix markdown fence language tags
* docs: remove ESP32 power management notes
Adds ROUTER_LATE and CLIENT_BASE to preferred rebroadcaster check
(skip unsolicited NodeInfo) and prevents ROUTER_LATE from setting
rebroadcast mode to NONE, which would silently break relaying.
ROUTER_LATE is an infrastructure relay that should use the same power
assumptions, interval defaults, and LoRa wake behavior as ROUTER.
Without this, ROUTER_LATE uses client-class defaults and will not
wake on LoRa activity from deep sleep, making it a broken relay.
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
ROUTER_LATE already has high base intervals and should not be further
scaled by congestion. TAK_TRACKER is a tracker variant that should
skip congestion scaling like TRACKER does.
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
ROUTER and ROUTER_LATE should not accumulate favorites by sending DMs.
Also replaces magic number 12 with meshtastic_Config_DeviceConfig_Role_CLIENT_BASE.
ROUTER_LATE should be treated as an impolite telemetry role like
ROUTER, responding to multi-hop broadcast requests and using
aggressive send timing without channel utilization checks.
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
ROUTER_LATE now preserves node_info_broadcast_secs during factory
reset and auto-enables Store & Forward server mode, matching ROUTER
infrastructure behavior.
* Add ESP32 Power Management lessons learned document
Documents our experimentation with ESP-IDF DFS and why it doesn't
work well for Meshtastic (RTOS locks, BLE locks, USB issues).
Proposes simpler alternative: manual setCpuFrequencyMhz() control
with explicit triggers for when to go fast vs slow.
* Added a lambda function to clear startup output in the MQTT unit test to ensure a clean state before and after the MQTT subscription process.
This is a non-breaking change that increases the internal representation
of node counts from uint8_t (max 255) to uint16_t (max 65535) to support
larger mesh networks, particularly on ESP32-S3 devices with PSRAM.
Changes:
- NodeStatus: numOnline, numTotal, lastNumTotal (uint8_t -> uint16_t)
- ProtobufModule: numOnlineNodes (uint8_t -> uint16_t)
- MapApplet: loop counters changed to size_t for consistency with getNumMeshNodes()
- NodeStatus: Fixed log format to use %u for unsigned integers
Note: Default class methods keep uint32_t for numOnlineNodes parameter
to match the public API and allow flexibility, even though internal node
counts use uint16_t (max 65535 nodes).
This change does NOT affect protobuf definitions, maintaining wire
compatibility with existing clients and devices.
* Add seenRecently = true if wasUpgraded is true but unable to remove from queue (i.e. already sent/processed).
* Consistent comment between FloodingRouter and HopRouter
* Add seenRecently = true if wasUpgraded is true but unable to remove from queue (i.e. already sent/processed).
* Consistent comment between FloodingRouter and HopRouter
