From b847b46ef7b35b2efdc79be62cf7fceabf275fc5 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Tue, 15 Sep 2026 17:33:50 -0700 Subject: [PATCH] Retry identity startup and harden replay state and permissions Read identities up to three times before allowing replacement, publish only complete reads, and require new startup keys to be saved with bounded retries. Use authoritative replay-file metadata throughout persistence and clock recovery. Validate setperm input before narrowing or mutating roles, and add production-path regression coverage. --- .github/workflows/run-unit-tests.yml | 1 + examples/kiss_modem/main.cpp | 5 +- examples/simple_repeater/MyMesh.cpp | 6 +- examples/simple_repeater/main.cpp | 4 +- examples/simple_room_server/MyMesh.cpp | 6 +- examples/simple_room_server/main.cpp | 4 +- examples/simple_secure_chat/main.cpp | 4 +- examples/simple_sensor/SensorMesh.cpp | 6 +- examples/simple_sensor/main.cpp | 4 +- src/helpers/ClientACL.cpp | 28 +++++--- src/helpers/ClientLoginPersistence.h | 24 ++++--- src/helpers/IdentityStore.cpp | 66 ++++++++++-------- src/helpers/IdentityStore.h | 3 + .../client_acl_cli/test_client_acl_cli.cpp | 49 +++++++++++++- .../client_acl_spiffs/mocks/Arduino.h | 7 +- .../test_client_acl_spiffs.cpp | 38 ++++++++++- .../companion_uncached_storage/test.cpp | 3 + test/fixtures/contact_cache/mocks/Identity.h | 4 ++ test/test_client_acl_cli.py | 12 +++- test/test_client_acl_spiffs.py | 4 +- test/test_identity_and_settings_recovery.py | 67 +++++++++++++++++++ 21 files changed, 272 insertions(+), 73 deletions(-) diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index ec3f9de7..bcf94115 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -147,6 +147,7 @@ jobs: python3 -B test/test_elrs_power.py python3 -B test/test_ota_heap_context.py python3 -B test/test_client_acl_spiffs.py + python3 -B test/test_client_acl_cli.py -v - name: Verify ESP32 USB sleep and G3 button wake run: python3 -B test/test_esp32_usb_sleep.py diff --git a/examples/kiss_modem/main.cpp b/examples/kiss_modem/main.cpp index 5efdf091..760d8cd1 100644 --- a/examples/kiss_modem/main.cpp +++ b/examples/kiss_modem/main.cpp @@ -58,15 +58,16 @@ void loadOrCreateIdentity() { bool identity_ready = true; if (needs_identity) { identity_ready = mesh::generateUsableLocalIdentity(identity, radio_new_identity); - if (identity_ready) store.save("_main", identity); + if (identity_ready) identity_ready = store.saveWithRetry("_main", identity); } #if defined(ESP32_PLATFORM) mesh::discardESP32TrueRandom(); #endif if (!identity_ready) { - MESH_DEBUG_PRINTLN("Identity generation exhausted all attempts; rebooting"); + MESH_DEBUG_PRINTLN("Identity generation or persistence failed after retries; rebooting"); board.reboot(); + halt(); // Never let setup continue if a platform's reboot returns. } } diff --git a/examples/simple_repeater/MyMesh.cpp b/examples/simple_repeater/MyMesh.cpp index df0837e6..494a57bc 100644 --- a/examples/simple_repeater/MyMesh.cpp +++ b/examples/simple_repeater/MyMesh.cpp @@ -11974,8 +11974,10 @@ void MyMesh::handleCommand(uint32_t sender_timestamp, ClientInfo* sender, char * uint8_t pubkey[PUB_KEY_SIZE]; if (hex_len > 0 && hex_len <= PUB_KEY_SIZE * 2 && (hex_len & 1) == 0 && mesh::Utils::fromHex(pubkey, (int)(hex_len / 2), hex)) { - uint8_t perms = atoi(sp); - if (acl.applyPermissions(self_id, pubkey, (int)(hex_len / 2), perms)) { + uint32_t perms; + if (!mesh::cli::parseUnsignedIntegerStrict(sp, perms) || perms > UINT8_MAX) { + strcpy(reply, "Err - permissions must be 0-255"); + } else if (acl.applyPermissions(self_id, pubkey, (int)(hex_len / 2), static_cast(perms))) { mesh::scheduleLazyPersistenceMutation( dirty_contacts_expiry, contacts_save_failures, futureMillis(LAZY_CONTACTS_WRITE_DELAY)); diff --git a/examples/simple_repeater/main.cpp b/examples/simple_repeater/main.cpp index a6b8ac30..b03a99cf 100644 --- a/examples/simple_repeater/main.cpp +++ b/examples/simple_repeater/main.cpp @@ -177,14 +177,14 @@ void setup() { if (needs_identity) { MESH_DEBUG_PRINTLN("Generating new keypair"); identity_ready = mesh::generateUsableLocalIdentity(the_mesh.self_id, radio_new_identity); - if (identity_ready) store.save("_main", the_mesh.self_id); + if (identity_ready) identity_ready = store.saveWithRetry("_main", the_mesh.self_id); } #if defined(ESP32_PLATFORM) mesh::discardESP32TrueRandom(); #endif if (!identity_ready) { - MESH_DEBUG_PRINTLN("Identity generation exhausted all attempts; rebooting"); + MESH_DEBUG_PRINTLN("Identity generation or persistence failed after retries; rebooting"); board.reboot(); return; } diff --git a/examples/simple_room_server/MyMesh.cpp b/examples/simple_room_server/MyMesh.cpp index a8b8ff3f..bee3a640 100644 --- a/examples/simple_room_server/MyMesh.cpp +++ b/examples/simple_room_server/MyMesh.cpp @@ -2376,8 +2376,10 @@ void MyMesh::handleCommand(uint32_t sender_timestamp, char *command, char *reply uint8_t pubkey[PUB_KEY_SIZE]; if (hex_len > 0 && hex_len <= PUB_KEY_SIZE * 2 && (hex_len & 1) == 0 && mesh::Utils::fromHex(pubkey, (int)(hex_len / 2), hex)) { - uint8_t perms = atoi(sp); - if (acl.applyPermissions(self_id, pubkey, (int)(hex_len / 2), perms)) { + uint32_t perms; + if (!mesh::cli::parseUnsignedIntegerStrict(sp, perms) || perms > UINT8_MAX) { + strcpy(reply, "Err - permissions must be 0-255"); + } else if (acl.applyPermissions(self_id, pubkey, (int)(hex_len / 2), static_cast(perms))) { mesh::scheduleLazyPersistenceMutation( dirty_contacts_expiry, contacts_save_failures, futureMillis(LAZY_CONTACTS_WRITE_DELAY)); diff --git a/examples/simple_room_server/main.cpp b/examples/simple_room_server/main.cpp index 88ab8c3e..b1bf87b1 100644 --- a/examples/simple_room_server/main.cpp +++ b/examples/simple_room_server/main.cpp @@ -103,14 +103,14 @@ void setup() { bool identity_ready = true; if (needs_identity) { identity_ready = mesh::generateUsableLocalIdentity(the_mesh.self_id, radio_new_identity); - if (identity_ready) store.save("_main", the_mesh.self_id); + if (identity_ready) identity_ready = store.saveWithRetry("_main", the_mesh.self_id); } #if defined(ESP32_PLATFORM) mesh::discardESP32TrueRandom(); #endif if (!identity_ready) { - MESH_DEBUG_PRINTLN("Identity generation exhausted all attempts; rebooting"); + MESH_DEBUG_PRINTLN("Identity generation or persistence failed after retries; rebooting"); board.reboot(); return; } diff --git a/examples/simple_secure_chat/main.cpp b/examples/simple_secure_chat/main.cpp index 787fe58c..76c13250 100644 --- a/examples/simple_secure_chat/main.cpp +++ b/examples/simple_secure_chat/main.cpp @@ -348,14 +348,14 @@ public: identity_ready = mesh::generateUsableLocalIdentity( self_id, [this]() { return mesh::LocalIdentity(getRNG()); }); #endif - if (identity_ready) store.save("_main", self_id); + if (identity_ready) identity_ready = store.saveWithRetry("_main", self_id); } #if defined(ESP32_PLATFORM) mesh::discardESP32TrueRandom(); #endif if (!identity_ready) { - MESH_DEBUG_PRINTLN("Identity generation exhausted all attempts; rebooting"); + MESH_DEBUG_PRINTLN("Identity generation or persistence failed after retries; rebooting"); board.reboot(); return; } diff --git a/examples/simple_sensor/SensorMesh.cpp b/examples/simple_sensor/SensorMesh.cpp index dd4e7924..b3a0c1b4 100644 --- a/examples/simple_sensor/SensorMesh.cpp +++ b/examples/simple_sensor/SensorMesh.cpp @@ -626,8 +626,10 @@ void SensorMesh::handleCommand(uint32_t sender_timestamp, char* command, char* r uint8_t pubkey[PUB_KEY_SIZE]; if (hex_len > 0 && hex_len <= PUB_KEY_SIZE * 2 && (hex_len & 1) == 0 && mesh::Utils::fromHex(pubkey, (int)(hex_len / 2), hex)) { - uint8_t perms = atoi(sp); - if (acl.applyPermissions(self_id, pubkey, (int)(hex_len / 2), perms)) { + uint32_t perms; + if (!mesh::cli::parseUnsignedIntegerStrict(sp, perms) || perms > UINT8_MAX) { + strcpy(reply, "Err - permissions must be 0-255"); + } else if (acl.applyPermissions(self_id, pubkey, (int)(hex_len / 2), static_cast(perms))) { mesh::scheduleLazyPersistenceMutation( dirty_contacts_expiry, contacts_save_failures, futureMillis(LAZY_CONTACTS_WRITE_DELAY)); diff --git a/examples/simple_sensor/main.cpp b/examples/simple_sensor/main.cpp index 3c449a33..e8ff788d 100644 --- a/examples/simple_sensor/main.cpp +++ b/examples/simple_sensor/main.cpp @@ -116,14 +116,14 @@ void setup() { if (needs_identity) { MESH_DEBUG_PRINTLN("Generating new keypair"); identity_ready = mesh::generateUsableLocalIdentity(the_mesh.self_id, radio_new_identity); - if (identity_ready) store.save("_main", the_mesh.self_id); + if (identity_ready) identity_ready = store.saveWithRetry("_main", the_mesh.self_id); } #if defined(ESP32_PLATFORM) mesh::discardESP32TrueRandom(); #endif if (!identity_ready) { - MESH_DEBUG_PRINTLN("Identity generation exhausted all attempts; rebooting"); + MESH_DEBUG_PRINTLN("Identity generation or persistence failed after retries; rebooting"); board.reboot(); return; } diff --git a/src/helpers/ClientACL.cpp b/src/helpers/ClientACL.cpp index 17df9b20..64e92b13 100644 --- a/src/helpers/ClientACL.cpp +++ b/src/helpers/ClientACL.cpp @@ -283,7 +283,9 @@ static bool readClientLoginReplayCeiling( bool* found) { *ceiling = 0; *found = false; - if (!fs->exists(mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH)) return true; + bool present = false; + if (!mesh::filePresence(fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH, present)) return false; + if (!present) return true; if (!validateLoginReplayFileIntegrity( fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH)) { return false; @@ -338,7 +340,9 @@ static bool writeClientLoginReplayCeiling( mesh::ClientLoginReplayReservationAction action) { if (action == mesh::ClientLoginReplayReservationAction::None) return true; #if defined(NRF52_PLATFORM) - if (fs->exists(mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH) + bool primary_exists = false; + if (!mesh::filePresence(fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH, primary_exists)) return false; + if (primary_exists && !validateLoginReplayFileIntegrity( fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH)) { return false; @@ -352,7 +356,9 @@ static bool writeClientLoginReplayCeiling( File source = mesh::emptyFile(fs); size_t record_count = 0; - if (fs->exists(mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH)) { + bool source_exists = false; + if (!mesh::filePresence(fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH, source_exists)) return false; + if (source_exists) { source = openRead(fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH); // A previously present image becoming unreadable is not a first login. // Check again after opening so disappearance/truncation cannot underflow @@ -487,7 +493,11 @@ bool ClientACL::clampLoginReplayTimestamps( if (_fs == NULL || !login_replay_store_available || now == 0) return false; ClientLoginReplayClampResult pending = {}; - if (_fs->exists(mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH)) { + bool primary = false, backup = false, temp = false; + if (!mesh::filePresence(_fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH, primary) + || !mesh::filePresence(_fs, mesh::CLIENT_LOGIN_REPLAY_BACKUP_PATH, backup) + || !mesh::filePresence(_fs, mesh::CLIENT_LOGIN_REPLAY_TEMP_PATH, temp)) return false; + if (primary) { File source = openRead(_fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH); uint32_t original_crc = 0; const bool valid = copyClampedLoginReplay( @@ -546,8 +556,7 @@ bool ClientACL::clampLoginReplayTimestamps( return false; } } - } else if (_fs->exists(mesh::CLIENT_LOGIN_REPLAY_BACKUP_PATH) - || _fs->exists(mesh::CLIENT_LOGIN_REPLAY_TEMP_PATH)) { + } else if (backup || temp) { // Recovery is performed by load(). Do not mistake an interrupted/corrupt // transaction for a never-created store during this explicit operation. login_replay_store_available = false; @@ -585,10 +594,11 @@ void ClientACL::load(FILESYSTEM* fs, const mesh::LocalIdentity& self_id) { // rename. The live image remains authoritative. mesh::removeClientLoginReplayArtifact( _fs, mesh::CLIENT_LOGIN_REPLAY_TEMP_PATH); + bool replay_exists = false; login_replay_store_available = - !_fs->exists(mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH) - || validateLoginReplayFileIntegrity( - _fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH); + mesh::filePresence(_fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH, replay_exists) + && (!replay_exists || validateLoginReplayFileIntegrity( + _fs, mesh::CLIENT_LOGIN_REPLAY_PRIMARY_PATH)); #else login_replay_store_available = mesh::recoverClientLoginReplayFiles( _fs, validateLoginReplayFileIntegrity); diff --git a/src/helpers/ClientLoginPersistence.h b/src/helpers/ClientLoginPersistence.h index 1d7e4742..91066811 100644 --- a/src/helpers/ClientLoginPersistence.h +++ b/src/helpers/ClientLoginPersistence.h @@ -3,6 +3,7 @@ #include #include #include +#include "FilePresence.h" namespace mesh { @@ -162,14 +163,18 @@ inline bool validateClientLoginReplayImage(const uint8_t* image, template bool removeClientLoginReplayArtifact(Filesystem* fs, const char* path) { - if (!fs->exists(path)) return true; - fs->remove(path); - return !fs->exists(path); + bool present = false; + if (!filePresence(fs, path, present)) return false; + if (!present) return true; + return fs->remove(path) && filePresence(fs, path, present) && !present; } template bool recoverClientLoginReplayFiles(Filesystem* fs, Validator is_valid) { - if (fs->exists(CLIENT_LOGIN_REPLAY_PRIMARY_PATH) + bool primary = false, backup = false; + if (!filePresence(fs, CLIENT_LOGIN_REPLAY_PRIMARY_PATH, primary) + || !filePresence(fs, CLIENT_LOGIN_REPLAY_BACKUP_PATH, backup)) return false; + if (primary && is_valid(fs, CLIENT_LOGIN_REPLAY_PRIMARY_PATH)) { // The validated primary is already authoritative. Cleanup failure must // not disable replay protection or make committed state appear absent. @@ -177,7 +182,7 @@ bool recoverClientLoginReplayFiles(Filesystem* fs, Validator is_valid) { removeClientLoginReplayArtifact(fs, CLIENT_LOGIN_REPLAY_BACKUP_PATH); return true; } - if (fs->exists(CLIENT_LOGIN_REPLAY_BACKUP_PATH) + if (backup && is_valid(fs, CLIENT_LOGIN_REPLAY_BACKUP_PATH)) { if (!removeClientLoginReplayArtifact(fs, CLIENT_LOGIN_REPLAY_PRIMARY_PATH) @@ -190,8 +195,7 @@ bool recoverClientLoginReplayFiles(Filesystem* fs, Validator is_valid) { } // A never-created store is the upgrade/first-boot case. An invalid live or // backup image is different: retain it for diagnosis and fail closed. - return !fs->exists(CLIENT_LOGIN_REPLAY_PRIMARY_PATH) - && !fs->exists(CLIENT_LOGIN_REPLAY_BACKUP_PATH) + return !primary && !backup && removeClientLoginReplayArtifact(fs, CLIENT_LOGIN_REPLAY_TEMP_PATH); } @@ -204,9 +208,9 @@ bool publishClientLoginReplayTemp(Filesystem* fs, removeClientLoginReplayArtifact(fs, CLIENT_LOGIN_REPLAY_TEMP_PATH); return false; } - if (fs->exists(CLIENT_LOGIN_REPLAY_BACKUP_PATH)) return false; - - const bool had_primary = fs->exists(CLIENT_LOGIN_REPLAY_PRIMARY_PATH); + bool backup = false, had_primary = false; + if (!filePresence(fs, CLIENT_LOGIN_REPLAY_BACKUP_PATH, backup) || backup + || !filePresence(fs, CLIENT_LOGIN_REPLAY_PRIMARY_PATH, had_primary)) return false; if (had_primary && !fs->rename(CLIENT_LOGIN_REPLAY_PRIMARY_PATH, CLIENT_LOGIN_REPLAY_BACKUP_PATH)) { diff --git a/src/helpers/IdentityStore.cpp b/src/helpers/IdentityStore.cpp index 38531f85..baafa0f8 100644 --- a/src/helpers/IdentityStore.cpp +++ b/src/helpers/IdentityStore.cpp @@ -24,47 +24,55 @@ bool IdentityStore::recover(const char* name) { } bool IdentityStore::load(const char *name, mesh::LocalIdentity& id) { - if (!recover(name)) return false; - bool loaded = false; - char filename[40]; - sprintf(filename, "%s/%s.id", _dir, name); - if (_fs->exists(filename)) { -#if defined(RP2040_PLATFORM) - File file = _fs->open(filename, "r"); -#else - File file = _fs->open(filename); -#endif - if (file) { - loaded = id.readFrom(file); - file.close(); - } - } - return loaded; + return load(name, id, nullptr, 0); } bool IdentityStore::load(const char *name, mesh::LocalIdentity& id, char display_name[], int max_name_sz) { - if (!recover(name)) return false; - bool loaded = false; char filename[40]; - sprintf(filename, "%s/%s.id", _dir, name); - if (_fs->exists(filename)) { + if (snprintf(filename, sizeof(filename), "%s/%s.id", _dir, name) + >= (int)sizeof(filename)) return false; + for (unsigned attempt = 0; attempt < IO_ATTEMPTS; ++attempt) { + if (!recover(name)) continue; + bool present = false; + if (!mesh::filePresence(_fs, filename, present)) continue; + if (!present) return false; #if defined(RP2040_PLATFORM) File file = _fs->open(filename, "r"); #else File file = _fs->open(filename); #endif - if (file) { - loaded = id.readFrom(file); - - int n = max_name_sz; // up to 32 bytes - if (n > 32) n = 32; - file.read((uint8_t *) display_name, n); - display_name[n - 1] = 0; // ensure null terminator - + if (!file || file.isDirectory()) { file.close(); + continue; } + mesh::LocalIdentity loaded; + if (!loaded.readFrom(file)) { + file.close(); + continue; + } + // The legacy display name is optional. A key-only image remains valid; + // neither a short name nor a partial key may leak into the caller's state. + if (display_name != nullptr && max_name_sz > 0) { + const int n = max_name_sz > 32 ? 32 : max_name_sz; + char loaded_name[32]; + if (file.read(reinterpret_cast(loaded_name), n) == n) { + loaded_name[n - 1] = 0; + memcpy(display_name, loaded_name, n); + } + } + file.close(); + id = loaded; + return true; } - return loaded; + // Callers may provision a replacement after bounded recovery is exhausted. + return false; +} + +bool IdentityStore::saveWithRetry(const char* name, const mesh::LocalIdentity& id) { + for (unsigned attempt = 0; attempt < IO_ATTEMPTS; ++attempt) { + if (save(name, id)) return true; + } + return false; } bool IdentityStore::save(const char *name, const mesh::LocalIdentity& id) { diff --git a/src/helpers/IdentityStore.h b/src/helpers/IdentityStore.h index c8f7aefa..2c079bc6 100644 --- a/src/helpers/IdentityStore.h +++ b/src/helpers/IdentityStore.h @@ -15,6 +15,7 @@ class IdentityStore { FILESYSTEM* _fs; const char* _dir; public: + static constexpr unsigned IO_ATTEMPTS = 3; IdentityStore(FILESYSTEM& fs, const char* dir): _fs(&fs), _dir(dir) { } void begin() { @@ -25,5 +26,7 @@ public: bool load(const char *name, mesh::LocalIdentity& id); bool load(const char *name, mesh::LocalIdentity& id, char display_name[], int max_name_sz); bool save(const char *name, const mesh::LocalIdentity& id); + // Startup must not run with a new identity that was never made durable. + bool saveWithRetry(const char *name, const mesh::LocalIdentity& id); bool save(const char *name, const mesh::LocalIdentity& id, const char display_name[]); }; diff --git a/test/fixtures/client_acl_cli/test_client_acl_cli.cpp b/test/fixtures/client_acl_cli/test_client_acl_cli.cpp index 0fe19b73..e85e704d 100644 --- a/test/fixtures/client_acl_cli/test_client_acl_cli.cpp +++ b/test/fixtures/client_acl_cli/test_client_acl_cli.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,13 @@ static Stream console; Stream& usbConsolePort() { return console; } } +static mesh::LocalIdentity self_id; +static unsigned long dirty_contacts_expiry = 0; +static uint8_t contacts_save_failures = 0; +static const unsigned long LAZY_CONTACTS_WRITE_DELAY = 5000; +static unsigned long futureMillis(unsigned long delay) { return delay + 1; } +static unsigned gps_updates = 0; +static void updateGpsTelemetryPolicy() { ++gps_updates; } #include "production.h" static ClientInfo* add(ClientACL& acl, unsigned index, uint8_t permissions) { @@ -244,6 +252,44 @@ static void repeater_delegation_stays_denied() { } } +static void permissions_validate_before_mutation() { + for (auto handler : {repeaterCommand, roomCommand, sensorCommand}) { + for (uint32_t timestamp : {0U, 12345U}) { + FakeFilesystem fs; + ClientACL acl; + acl.load(&fs, self_id); + auto* admin = add(acl, 0, PERM_ACL_ADMIN); + auto* target = add(acl, 1, PERM_ACL_ADMIN); + uint8_t target_key[PUB_KEY_SIZE]; + memcpy(target_key, target->id.pub_key, sizeof(target_key)); + char key[65]; mesh::Utils::toHex(key, target_key, sizeof(target_key)); + CHECK(acl.save(&fs)); + const auto durable = fs.files; + for (const char* value : {"", "invalid", "-1", "256", "259", "3bad", "3.0", + "+", "4294967296", "99999999999999999999999"}) { + char command[140], reply[160] = {}; + snprintf(command, sizeof(command), "setperm %s %s", key, value); + dirty_contacts_expiry = 0; + gps_updates = 0; + handler(acl, timestamp ? admin : nullptr, timestamp, command, reply); + CHECK(strncmp(reply, "Err", 3) == 0); + CHECK(acl.getNumClients() == 2 && acl.getClient(target_key, PUB_KEY_SIZE)->isAdmin()); + CHECK(dirty_contacts_expiry == 0 && fs.files == durable); + CHECK(gps_updates == 0); + } + for (unsigned value : {1U, 2U, 3U, 5U, 255U, 0U}) { + char command[140], reply[160] = {}; + snprintf(command, sizeof(command), "setperm %s %u", key, value); + dirty_contacts_expiry = 0; + handler(acl, timestamp ? admin : nullptr, timestamp, command, reply); + CHECK(strcmp(reply, "OK") == 0 && dirty_contacts_expiry != 0); + auto* changed = acl.getClient(target_key, PUB_KEY_SIZE); + CHECK(value == 0 ? changed == nullptr : changed && changed->permissions == value); + } + } + } +} + int main() { empty_and_single_entry(); skips_inactive_and_preserves_full_keys(); @@ -254,5 +300,6 @@ int main() { listing_does_not_mutate_acl(); actual_role_dispatch_handles_radio_and_local(); repeater_delegation_stays_denied(); - puts("9 ACL CLI checks passed"); + permissions_validate_before_mutation(); + puts("10 ACL CLI checks passed"); } diff --git a/test/fixtures/client_acl_spiffs/mocks/Arduino.h b/test/fixtures/client_acl_spiffs/mocks/Arduino.h index f0c5ef2c..411290cd 100644 --- a/test/fixtures/client_acl_spiffs/mocks/Arduino.h +++ b/test/fixtures/client_acl_spiffs/mocks/Arduino.h @@ -61,7 +61,10 @@ public: size_t fail_read_open = 0; size_t fail_read_after = 0; - bool exists(const char* path) const { return files.count(path) != 0; } + // ESP32 VFS exists() opens the file; stat/rename use the namespace instead. + bool exists(const char* path) const { + return files.count(path) != 0 && unreadable.count(path) == 0; + } File open(const char* path, const char* mode = "r", bool = false) { const std::string name(path); if (mode[0] == 'r') { @@ -86,7 +89,7 @@ public: } bool rename(const char* from, const char* to) { if (fail_rename_from == from || fail_rename_from_paths.count(from) - || !exists(from) || exists(to)) return false; + || files.count(from) == 0 || files.count(to) != 0) return false; files[to] = files[from]; files.erase(from); return true; diff --git a/test/fixtures/client_acl_spiffs/test_client_acl_spiffs.cpp b/test/fixtures/client_acl_spiffs/test_client_acl_spiffs.cpp index c3b3562f..5353c37d 100644 --- a/test/fixtures/client_acl_spiffs/test_client_acl_spiffs.cpp +++ b/test/fixtures/client_acl_spiffs/test_client_acl_spiffs.cpp @@ -515,10 +515,46 @@ static void incomplete_acl_load_is_never_authoritative() { } } +static void replay_metadata_errors_never_erase_authority() { + for (unsigned fault = 0; fault < 2; ++fault) { + FakeFilesystem fs; + ClientACL acl; + acl.load(&fs, SELF); + CHECK(acl.authorizeLoginTimestamp(KEY, 100, 0, PERM_ACL_ADMIN)); + auto* client = acl.putClient(mesh::Identity(KEY), PERM_ACL_ADMIN); + CHECK(client); + client->last_timestamp = 500; + const auto original = fs.files[PRIMARY]; + if (fault == 0) fs.unreadable.insert(PRIMARY); + else fs.metadata_error = true; + CHECK(!mesh::recoverClientLoginReplayFiles(&fs, validateLoginReplayFileIntegrity)); + uint32_t ceiling; + bool found; + CHECK(!readClientLoginReplayCeiling(&fs, KEY, &ceiling, &found)); + CHECK(!writeClientLoginReplayCeiling(&fs, KEY, 900, + mesh::ClientLoginReplayReservationAction::CreateNew)); + ClientLoginReplayClampResult result{}; + CHECK(!acl.clampLoginReplayTimestamps(KEY, 50, result)); + check_empty_result(result); + CHECK(client->last_timestamp == 500); + CHECK(!acl.authorizeLoginTimestamp(KEY, 99, 0, PERM_ACL_READ_ONLY)); + CHECK(fs.files[PRIMARY] == original); + ClientACL reboot; + reboot.load(&fs, SELF); + CHECK(!reboot.authorizeLoginTimestamp(KEY, 99, 0, PERM_ACL_READ_ONLY)); + CHECK(fs.files[PRIMARY] == original); + fs.unreadable.clear(); fs.metadata_error = false; + reboot.load(&fs, SELF); + CHECK(!reboot.authorizeLoginTimestamp(KEY, 100, 0, PERM_ACL_ADMIN)); + CHECK(reboot.authorizeLoginTimestamp(KEY, 161, 0, PERM_ACL_ADMIN)); + } +} + int main() { const struct { const char* name; void (*run)(); } tests[] = { {"allocation failure preserves clients", allocation_failure_preserves_saved_clients}, {"incomplete ACL is not authoritative", incomplete_acl_load_is_never_authoritative}, + {"replay read and metadata failures stay closed", replay_metadata_errors_never_erase_authority}, {"missing read differs from empty file", missing_read_is_not_empty_file}, {"first admin and monotonic retries", first_admin_and_retries}, {"reboot preserves ceiling", reboot_preserves_ceiling}, @@ -548,5 +584,5 @@ int main() { test.run(); std::printf("PASS: %s\n", test.name); } - std::puts("26 ClientACL SPIFFS checks passed"); + std::puts("27 ClientACL SPIFFS checks passed"); } diff --git a/test/fixtures/companion_uncached_storage/test.cpp b/test/fixtures/companion_uncached_storage/test.cpp index 71cce579..10474f09 100644 --- a/test/fixtures/companion_uncached_storage/test.cpp +++ b/test/fixtures/companion_uncached_storage/test.cpp @@ -22,6 +22,7 @@ public: File(FakeFilesystem* filesystem, const char* name, bool write) : fs(filesystem), path(name), writing(write) {} explicit operator bool() const { return fs != nullptr; } + bool isDirectory() const { return false; } bool open(const char* name, uint8_t mode); size_t read(uint8_t* bytes, size_t length); size_t write(const uint8_t* bytes, size_t length); @@ -41,6 +42,7 @@ public: unsigned fail_open_remaining = std::numeric_limits::max(); int stat_error = 0; unsigned fail_rename = 0, renames = 0, writes = 0; + unsigned read_opens = 0; std::vector snapshots; bool exists(const char* path) const { #if defined(ESP32_PLATFORM) @@ -54,6 +56,7 @@ public: void _unlockFS() {} FakeFilesystem* _getFS() { return this; } File open(const char* path, const char* mode = "r", bool = false) { + if (*mode == 'r') ++read_opens; if (fail_open == path && fail_open_remaining != 0) { --fail_open_remaining; return File(); diff --git a/test/fixtures/contact_cache/mocks/Identity.h b/test/fixtures/contact_cache/mocks/Identity.h index f518766c..d33a1840 100644 --- a/test/fixtures/contact_cache/mocks/Identity.h +++ b/test/fixtures/contact_cache/mocks/Identity.h @@ -12,6 +12,10 @@ class LocalIdentity : public Identity { public: uint8_t private_key[64] = {}; mutable unsigned derivations = 0; + template bool readFrom(Reader& in) { + return in.read(pub_key, sizeof(pub_key)) == sizeof(pub_key) + && in.read(private_key, sizeof(private_key)) == sizeof(private_key); + } size_t writeTo(uint8_t* out, size_t size) const { if (size < 96) return 0; memcpy(out, private_key, 64); diff --git a/test/test_client_acl_cli.py b/test/test_client_acl_cli.py index 9121b001..8143e375 100644 --- a/test/test_client_acl_cli.py +++ b/test/test_client_acl_cli.py @@ -26,6 +26,7 @@ def role_handler(role, source): body = extract_braced(source, f"void {owner}::handleCommand(uint32_t sender_timestamp,") zero_guard = re.search(r"if \([^\n]+sender_timestamp == 0\) sender_timestamp = 1;", body).group() prefix = extract_braced(body, "if (strlen(command) > 4 && command[2] == '|')") + permissions = extract_braced(body, 'if (memcmp(command, "setperm ", 8) == 0)') paged = extract_braced(body, "if (mesh::cli::handleACLGet(") local = extract_braced(body, 'if (sender_timestamp == 0 && strcmp(command, "get acl") == 0)') # The room source opens its next conditional branch's preprocessor guard @@ -46,7 +47,7 @@ static void {role}Command(ClientACL& acl, ClientInfo* sender, {prefix} mesh::cli::normalizeCommandVerb(command); {guard} - {paged} else {local} else strcpy(reply, "unhandled"); + {permissions} else {paged} else {local} else strcpy(reply, "unhandled"); }} """ @@ -61,7 +62,10 @@ class ClientAclCliTest(unittest.TestCase): hex_chars = re.search(r"static const char hex_chars\[\].*;", utils).group() generated = "namespace mesh {\n" + hex_chars + "\n" generated += extract_braced(utils, "void Utils::toHex(") + "\n" - generated += extract_braced(utils, "void Utils::printHex(") + "\n}\n" + for signature in ("void Utils::printHex(", "static uint8_t hexVal(", + "bool Utils::isHexChar(", "bool Utils::fromHex("): + generated += extract_braced(utils, signature) + "\n" + generated += "}\n" for signature in ( "static bool commandFamilyMatches(", "static bool isCommonManagerReadOnlyAllowed(", "static bool isRegionMgrAllowed(", "static bool isFilterMgrAllowed(", @@ -80,6 +84,8 @@ class Stream; namespace mesh { class Utils { public: static void toHex(char*, const uint8_t*, size_t); static void printHex(Stream&, const uint8_t*, size_t); + static bool isHexChar(char); + static bool fromHex(uint8_t*, int, const char*); }; } """) (work / "production.h").write_text(generated) @@ -96,7 +102,7 @@ namespace mesh { class Utils { public: self.assertEqual(result.returncode, 0, result.stdout + result.stderr) checked = subprocess.run([str(binary)], capture_output=True, text=True, timeout=10) self.assertEqual(checked.returncode, 0, checked.stdout + checked.stderr) - self.assertIn("9 ACL CLI checks passed", checked.stdout) + self.assertIn("10 ACL CLI checks passed", checked.stdout) def test_remote_dispatch_stays_behind_existing_admin_guards(self): for role, path in ROLES.items(): diff --git a/test/test_client_acl_spiffs.py b/test/test_client_acl_spiffs.py index 979d6714..a9933ff3 100644 --- a/test/test_client_acl_spiffs.py +++ b/test/test_client_acl_spiffs.py @@ -25,8 +25,8 @@ class ClientAclSpiffsTest(unittest.TestCase): self.assertEqual(compiled.returncode, 0, compiled.stdout + compiled.stderr) checked = subprocess.run([str(binary)], capture_output=True, text=True, timeout=10) self.assertEqual(checked.returncode, 0, checked.stdout + checked.stderr) - self.assertIn("26 ClientACL SPIFFS checks passed", checked.stdout) - self.assertEqual(checked.stdout.count("PASS:"), 26) + self.assertIn("27 ClientACL SPIFFS checks passed", checked.stdout) + self.assertEqual(checked.stdout.count("PASS:"), 27) if __name__ == "__main__": diff --git a/test/test_identity_and_settings_recovery.py b/test/test_identity_and_settings_recovery.py index 100c4a77..6c7d0b6d 100644 --- a/test/test_identity_and_settings_recovery.py +++ b/test/test_identity_and_settings_recovery.py @@ -54,11 +54,68 @@ class IdentityAndSettingsRecovery(unittest.TestCase): program += 'namespace mesh { template \n' + extract_braced( presence, 'bool filePresence(') + '\n}\n' for signature in ('bool IdentityStore::recover(', + 'bool IdentityStore::load(const char *name, mesh::LocalIdentity& id)', + 'bool IdentityStore::load(const char *name, mesh::LocalIdentity& id, char display_name[], int max_name_sz)', + 'bool IdentityStore::saveWithRetry(', 'bool IdentityStore::save(const char *name, const mesh::LocalIdentity& id)', 'bool IdentityStore::save(const char *name, const mesh::LocalIdentity& id, const char display_name[])'): program += extract_braced(source, signature) + '\n' program += r''' int main() { + // Startup reads reopen a transiently failing file and never publish a + // partially read key. Exhausted reads still permit provisioning a new key. + for (bool display : {false, true}) { + for (unsigned misses : {0U, 1U, 2U, 3U}) { + filesystem = FakeFilesystem(); + IdentityStore store(filesystem, ""); + mesh::LocalIdentity saved, live; + memset(saved.pub_key, 7, PUB_KEY_SIZE); + memset(saved.private_key, 8, PRV_KEY_SIZE); + memset(live.pub_key, 9, PUB_KEY_SIZE); + assert(store.save("main", saved, "saved name")); + char name[32] = "unchanged"; + filesystem.fail_open = "/main.id"; + filesystem.fail_open_remaining = misses; + filesystem.read_opens = 0; + const bool loaded = display ? store.load("main", live, name, sizeof(name)) + : store.load("main", live); + assert(loaded == (misses < IdentityStore::IO_ATTEMPTS)); + assert(filesystem.read_opens == (misses < 3 ? misses + 1 : 3)); + if (loaded) { + assert(memcmp(live.pub_key, saved.pub_key, PUB_KEY_SIZE) == 0); + assert(memcmp(live.private_key, saved.private_key, PRV_KEY_SIZE) == 0); + if (display) assert(strcmp(name, "saved name") == 0); + } else { + assert(live.pub_key[0] == 9 && strcmp(name, "unchanged") == 0); + assert(store.saveWithRetry("main", live)); + } + } + } + filesystem = FakeFilesystem(); + IdentityStore boot_store(filesystem, ""); + mesh::LocalIdentity old_identity, new_identity; + memset(old_identity.pub_key, 7, PUB_KEY_SIZE); + memset(new_identity.pub_key, 9, PUB_KEY_SIZE); + assert(boot_store.save("main", old_identity)); + const auto previous = filesystem.files["/main.id"]; + filesystem.max_read = 3; + filesystem.read_opens = 0; + char optional_name[32] = "keep default"; + assert(!boot_store.load("main", new_identity, optional_name, sizeof(optional_name))); + assert(filesystem.read_opens == 3 && new_identity.pub_key[0] == 9); + assert(strcmp(optional_name, "keep default") == 0); + filesystem.max_read = std::numeric_limits::max(); + assert(boot_store.load("main", new_identity, optional_name, sizeof(optional_name))); + assert(strcmp(optional_name, "keep default") == 0); // key-only image is valid + memset(new_identity.pub_key, 9, PUB_KEY_SIZE); + filesystem.fail_open = "/main.id.tmp"; + filesystem.fail_open_remaining = 2; + assert(boot_store.saveWithRetry("main", new_identity)); + assert(filesystem.files["/main.id"] != previous); + const auto committed = filesystem.files["/main.id"]; + filesystem.fail_open_remaining = std::numeric_limits::max(); + assert(!boot_store.saveWithRetry("main", old_identity)); + assert(filesystem.files["/main.id"] == committed); for (bool display : {false, true}) { for (unsigned fault = 0; fault < 4; ++fault) { filesystem = FakeFilesystem(); @@ -97,6 +154,16 @@ int main() { ''' compile_run(program, platform) + def test_startup_requires_durable_generated_identity(self): + for role in ('simple_repeater', 'simple_room_server', 'simple_sensor', + 'kiss_modem', 'simple_secure_chat'): + with self.subTest(role=role): + source = (ROOT / 'examples' / role / 'main.cpp').read_text() + self.assertIn('if (identity_ready) identity_ready = store.saveWithRetry(', source) + failed = extract_braced(source, 'if (!identity_ready)') + self.assertIn('board.reboot();', failed) + self.assertIn('halt();' if role == 'kiss_modem' else 'return;', failed) + def test_esp32_preferences_and_channels_recover_or_reset(self): store = (ROOT / 'examples/companion_radio/DataStore.cpp').read_text() prefs = (ROOT / 'examples/companion_radio/NodePrefs.h').read_text()