PR70 fix

releasenotes/RELEASE_NOTES_1.17.2-zephcore.md

@@ -1,8 +1,7 @@
 # ZephCore 1.17.2-zephcore
 
-A **minor release.** Three small changes, two of which only affect specific hardware. If you are running
-1.17.1 on anything other than a XIAO nRF52840 or an Ikoka Nano 30 dBm, there is little reason to flash
-this.
+A **minor release.** Four changes, two of which only affect specific hardware, plus a hardening pass on
+how the node's identity is stored.
 
 > [!NOTE]
 > **Most people can skip this.** The one group that should not is **XIAO nRF52840 and Ikoka Nano 30 dBm**
@@ -45,12 +44,54 @@ counter, so a node that could hear the mesh but never got a word in looked compl
 
 All roles, all boards.
 
+## A node can no longer advertise a key it cannot sign for
+
+Your identity is a private key plus the public key derived from it. Those were stored side by side and
+read back on trust, so if the two halves ever stopped matching — a stale file, a foreign write — the node
+would advertise a public key it had no private key for and keep doing so through every reboot. Nothing
+ever re-checked. In that state adverts verify nowhere, direct messages to you cannot be decrypted, and
+the companion app is handed an exported key that contradicts what the node says about itself.
+
+One node in the field turned up like this after moving from Arduino MeshCore, which is how it was found.
+The public half is redundant — it can always be recomputed from the private key — so it is now treated as
+a checksum rather than as data: the node derives its public key on every boot and never reads the stored
+copy. If the two disagree, that is now visible in the log instead of silent.
+
+Storage also moves to Arduino MeshCore's byte order (`pub || prv`), so all three roles finally agree on
+one format and an identity file can be moved between the two projects. Every layout either project has
+ever written is still readable, including the 64-byte prv-only files older repeaters wrote — nothing
+needs migrating and no existing file is rewritten.
+
+If a file cannot be made sense of at all, the node tries each half as a private key and adopts one only
+if exactly one is genuine. Failing that it keeps the bytes as `_main.id.bad` and generates a fresh
+identity, so a broken key is never silently thrown away.
+
+Found and diagnosed by **Marcel Verdult** ([@marcelverdult](https://github.com/marcelverdult)) —
+[#70](https://github.com/liquidraver/ZephCore/pull/70). His writeup is what made the byte-order
+difference findable.
+
+All roles, all boards.
+
+> [!WARNING]
+> **Downgrading below 1.17.2 after a factory reset or a fresh install.** Nodes carried over from 1.17.1
+> keep their existing identity file untouched and are unaffected. But an identity *generated* on 1.17.2
+> (or re-imported into it) uses the new byte order, and 1.17.1 and earlier will misread it — the node
+> comes up with a broken identity. Nothing is lost: older firmware misreads the file but does not rewrite
+> it, so flashing 1.17.2 again restores the identity exactly as it was.
+
 ---
 
 ## Known limitations
 
 - **The two radio changes are not on-air validated** — both are build-verified and reasoned from the
   datasheet, not measured on a live mesh.
+- **The identity repair and regenerate paths are not exercised on hardware.** Every layout and failure
+  case is covered by host tests against the same crypto library the firmware uses, and all three roles
+  build clean, but no physical node has been put into a broken state and recovered. The normal path — a
+  healthy node loading a healthy identity — is unchanged apart from one extra key derivation at boot.
+- **How that one node's key halves stopped matching is still unexplained.** The byte-order difference
+  reproduces the symptom exactly, but it should not have been reachable on that hardware, so something
+  else may have put it there. What this release fixes is that the state can no longer go unnoticed.
 - **The reported X1 difficulty logging in to repeaters is not fixed here** and remains under
   investigation. The listen-before-talk reporting above will make that failure visible in the log if it
   is the cause, which is one of several open hypotheses.

zephcore/adapters/datastore/ZephyrDataStore.cpp

@@ -532,16 +532,40 @@ bool ZephyrDataStore::loadMainIdentity(mesh::LocalIdentity &identity)
 {
 	uint8_t buf[PRV_KEY_SIZE + PUB_KEY_SIZE + 32];
 	size_t len = 0;
-	if (!openRead(MAIN_ID_FILE, buf, sizeof(buf), len) || len < PRV_KEY_SIZE + PUB_KEY_SIZE) {
+	if (!openRead(MAIN_ID_FILE, buf, sizeof(buf), len) || len < PRV_KEY_SIZE) {
 		return false;
 	}
-	return identity.readFrom(buf, len);
+	if (identity.readFromStorage(buf, len)) {
+		return true;
+	}
+	if (identity.recoverFromStorage(buf, len)) {
+		/* Deliberately not re-persisted: the file is the only record of what
+		 * went wrong, and re-deriving costs a few ms per boot.  The repeated
+		 * warning is the point — this state should be visible, not papered
+		 * over silently. */
+		LOG_WRN("main identity pub/prv mismatch - advertising the pub its private key owns");
+		return true;
+	}
+
+	/* No layout coheres — the stored pub is not prv·B under any reading, so
+	 * the pair is unusable: adverts would verify nowhere, inbound DMs would
+	 * not decrypt, and CMD_EXPORT_PRIVATE_KEY would hand the app a key that
+	 * contradicts SELF_INFO.  Returning false makes the caller generate a
+	 * fresh identity, so park the bytes first rather than overwriting them —
+	 * the private key may still be extractable by hand. */
+	LOG_ERR("main identity incoherent (%d bytes) - keeping bytes at %s, regenerating",
+		(int)len, MAIN_ID_BAD_FILE);
+	fs_unlink(MAIN_ID_BAD_FILE);
+	if (fs_rename(MAIN_ID_FILE, MAIN_ID_BAD_FILE) < 0) {
+		LOG_ERR("failed to park corrupt identity - regenerating over it");
+	}
+	return false;
 }
 
 bool ZephyrDataStore::saveMainIdentity(const mesh::LocalIdentity &identity)
 {
 	uint8_t buf[PRV_KEY_SIZE + PUB_KEY_SIZE + 32];
-	size_t n = identity.writeTo(buf, sizeof(buf));
+	size_t n = identity.writeToStorage(buf, sizeof(buf));
 	if (n == 0) {
 		return false;
 	}

zephcore/adapters/datastore/ZephyrDataStore.h

@@ -75,6 +75,9 @@ private:
 	static constexpr const char *MNT_POINT = "/lfs";
 	static constexpr const char *PREFS_FILE = "/lfs/new_prefs";
 	static constexpr const char *MAIN_ID_FILE = "/lfs/_main.id";
+	/* Where an identity that parses under no known layout is parked before
+	 * a fresh one is generated over the top — see loadMainIdentity(). */
+	static constexpr const char *MAIN_ID_BAD_FILE = "/lfs/_main.id.bad";
 	static constexpr const char *SHUTDOWN_FILE = "/lfs/shutdn";
 
 	/* External QSPI flash (optional) - contacts, channels, blobs */

zephcore/app/RepeaterDataStore.cpp

@@ -69,13 +69,28 @@ bool RepeaterDataStore::loadIdentity(mesh::LocalIdentity& id) {
     LOG_DBG("loadIdentity: read %d bytes from %s", (int)n, path);
 
     if (n >= PRV_KEY_SIZE) {
-        if (id.readFrom(buf, n)) {
+        if (id.readFromStorage(buf, n)) {
             LOG_INF("Loaded identity from %s", path);
             return true;
         }
-        LOG_ERR("loadIdentity: readFrom failed");
+        if (id.recoverFromStorage(buf, n)) {
+            /* Not re-persisted on purpose — see ZephyrDataStore::loadMainIdentity. */
+            LOG_WRN("identity pub/prv mismatch - advertising the pub its private key owns");
+            return true;
+        }
+        LOG_ERR("loadIdentity: no coherent key layout in %d bytes", (int)n);
     }
 
+    /* Unusable pair — the caller regenerates, so park the bytes instead of
+     * letting a fresh identity overwrite them (same as the companion). */
+    char bad_path[56];
+    if (snprintf(bad_path, sizeof(bad_path), "%s.bad", path) < (int)sizeof(bad_path)) {
+        fs_unlink(bad_path);
+        if (fs_rename(path, bad_path) == 0) {
+            LOG_ERR("Identity file corrupt - kept at %s", bad_path);
+            return false;
+        }
+    }
     LOG_ERR("Identity file corrupt");
     return false;
 }
@@ -101,8 +116,10 @@ bool RepeaterDataStore::saveIdentity(const mesh::LocalIdentity& id) {
         return false;
     }
 
-    uint8_t buf[PRV_KEY_SIZE];
-    int len = id.writeTo(buf, sizeof(buf));
+    /* pub || prv, same as the companion and Arduino MeshCore.  Older builds
+     * wrote prv alone (64 bytes); readFromStorage() still accepts those. */
+    uint8_t buf[PUB_KEY_SIZE + PRV_KEY_SIZE];
+    int len = id.writeToStorage(buf, sizeof(buf));
     ssize_t n = fs_write(&file, buf, len);
     ret = fs_sync(&file);
     fs_close(&file);

zephcore/include/mesh/Identity.h

@@ -61,8 +61,46 @@ public:
 	void calcSharedSecret(uint8_t *secret, const uint8_t *other_pub_key) const;
 	static bool validatePrivateKey(const uint8_t prv[64]);
 
+	/* Arduino-compatible *buffer* format: prv || pub (or prv alone when the
+	 * buffer is only PRV_KEY_SIZE).  This is protocol-facing — it is what
+	 * CMD_EXPORT/IMPORT_PRIVATE_KEY and the `prv.key` CLI put on the wire —
+	 * so it must stay byte-identical to upstream.  Do NOT use it for
+	 * on-flash storage; use readFromStorage()/writeToStorage() instead. */
 	bool readFrom(const uint8_t *src, size_t len);
 	size_t writeTo(uint8_t *dest, size_t max_len) const;
+
+	/* On-flash format: pub || prv, matching Arduino MeshCore's IdentityStore
+	 * (LocalIdentity::writeTo(Stream&)).  Kept separate from the buffer
+	 * format above because the two orders differ and conflating them is
+	 * exactly how a node ends up advertising a key it cannot sign for. */
+	size_t writeToStorage(uint8_t *dest, size_t max_len) const;
+
+	/* Tolerant counterpart to writeToStorage(). Accepts every layout either
+	 * project has ever written and tells them apart by deriving the pub from
+	 * the candidate prv — the stored pub is a 256-bit checksum over the prv,
+	 * so a wrong guess cannot match:
+	 *
+	 *   len == 64  ->  prv only          (ZephCore repeater/room <= 1.17.x)
+	 *   len == 96  ->  pub || prv        (Arduino MeshCore, ZephCore >= 1.17.2)
+	 *   len == 96  ->  prv || pub        (ZephCore companion <= 1.17.x)
+	 *
+	 * pub_key is always the derived value, never the stored bytes, so a
+	 * loaded identity is coherent by construction.  Returns false when no
+	 * layout coheres — the pair is unusable and the caller must not run
+	 * with it. */
+	bool readFromStorage(const uint8_t *src, size_t len);
+
+	/* Last resort after readFromStorage() fails: one half of a 96-byte blob
+	 * may still be an intact private key (the other having been clobbered by
+	 * a stale/foreign write).  Tests both candidate halves with
+	 * validatePrivateKey() and adopts one only if exactly one qualifies —
+	 * ambiguity means we cannot tell which half is which, and guessing wrong
+	 * would cement a garbage identity.  Recovers in RAM only: the file is
+	 * left alone so the evidence survives and the warning repeats each boot.
+	 * Note this changes the node's advertised pub_key to the one its private
+	 * key actually owns; contacts must re-add it either way, since the pub it
+	 * was advertising was never usable. */
+	bool recoverFromStorage(const uint8_t *src, size_t len);
 };
 
 } /* namespace mesh */

zephcore/src/Identity.cpp

@@ -199,6 +199,65 @@ size_t LocalIdentity::writeTo(uint8_t *dest, size_t max_len) const
 	return PRV_KEY_SIZE + PUB_KEY_SIZE;
 }
 
+size_t LocalIdentity::writeToStorage(uint8_t *dest, size_t max_len) const
+{
+	if (max_len < PUB_KEY_SIZE + PRV_KEY_SIZE) return 0;
+	memcpy(dest, pub_key, PUB_KEY_SIZE);
+	memcpy(dest + PUB_KEY_SIZE, prv_key, PRV_KEY_SIZE);
+	return PUB_KEY_SIZE + PRV_KEY_SIZE;
+}
+
+bool LocalIdentity::readFromStorage(const uint8_t *src, size_t len)
+{
+	uint8_t derived[PUB_KEY_SIZE];
+
+	/* prv alone — nothing to disagree with, derive and go. */
+	if (len == PRV_KEY_SIZE) {
+		memcpy(prv_key, src, PRV_KEY_SIZE);
+		crypto_eddsa_scalarbase(pub_key, prv_key);
+		return true;
+	}
+	if (len != PUB_KEY_SIZE + PRV_KEY_SIZE) return false;
+
+	/* Canonical: pub || prv. */
+	crypto_eddsa_scalarbase(derived, src + PUB_KEY_SIZE);
+	if (memcmp(derived, src, PUB_KEY_SIZE) == 0) {
+		memcpy(prv_key, src + PUB_KEY_SIZE, PRV_KEY_SIZE);
+		memcpy(pub_key, derived, PUB_KEY_SIZE);
+		return true;
+	}
+
+	/* Legacy ZephCore companion: prv || pub. */
+	crypto_eddsa_scalarbase(derived, src);
+	if (memcmp(derived, src + PRV_KEY_SIZE, PUB_KEY_SIZE) == 0) {
+		memcpy(prv_key, src, PRV_KEY_SIZE);
+		memcpy(pub_key, derived, PUB_KEY_SIZE);
+		return true;
+	}
+
+	return false;
+}
+
+bool LocalIdentity::recoverFromStorage(const uint8_t *src, size_t len)
+{
+	if (len != PUB_KEY_SIZE + PRV_KEY_SIZE) return false;
+
+	/* Where the prv would sit under each layout. */
+	const uint8_t *cand[2] = { src + PUB_KEY_SIZE, src };  /* pub||prv, prv||pub */
+	int found = -1;
+
+	for (int i = 0; i < 2; i++) {
+		if (!validatePrivateKey(cand[i])) continue;
+		if (found >= 0) return false;  /* both plausible — refuse to guess */
+		found = i;
+	}
+	if (found < 0) return false;
+
+	memcpy(prv_key, cand[found], PRV_KEY_SIZE);
+	crypto_eddsa_scalarbase(pub_key, prv_key);
+	return true;
+}
+
 void LocalIdentity::sign(uint8_t *sig, const uint8_t *message, int msg_len) const
 {
 	signExpanded(sig, prv_key, pub_key, message, (size_t)msg_len);