Improve Wi-Fi onboarding and BLE provisioning#46
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Funkelfetisch
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esp32/sdkconfig.defaults.ble-provisioning`:
- Around line 9-11: Update makeIdentity() in the ESP32 BLE provisioning
implementation so Security 1 uses a unique, non-public per-device secret rather
than the MAC suffix mac[3..5]. Preserve the existing BLE service-name identity
separately, and ensure the generated secret is delivered through the established
out-of-band provisioning mechanism.
In `@src/core/NetworkModule.h`:
- Around line 325-338: Update the empty-SSID branch in onUpdate() to ensure AP
mode is actually started, reusing the existing behavior from
maybeApplyPendingWifiCredentials(): transition via startAP() when state_ is not
already State::AP, then report the status. Preserve clearing
wifiCredentialApplyPending_ and avoid restarting AP when it is already active.
In `@src/platform/esp32/platform_esp32_ble_provisioning.cpp`:
- Around line 23-37: Make the BleProvisioningState fields started and
initialized atomic, then update all accesses in bleProvEventCb,
bleProvisioningInit, and bleProvisioningStop to use atomic-safe reads and
writes. Preserve the existing branching and lifecycle behavior while ensuring
concurrent provisioning callbacks and main-thread teardown cannot race.
- Around line 66-75: Update makeIdentity so g_bleProv.pop is derived from a
private per-device secret or another non-advertised provisioning source, rather
than mac[3..5] used in serviceName. Keep the BLE service name generation
unchanged, and ensure the PoP remains available through the intended secure
channel or device labeling without being recoverable from the advertised name.
In `@test/unit/core/unit_NetworkModule.cpp`:
- Around line 66-117: Extend the NetworkModule regression coverage with a test
that sets a non-empty SSID, connects, then clears the SSID through
scheduler.setControl and verifies the module enters AP state using its state/API
assertion rather than only checking status text. Keep the test clock setup and
ClockGuard cleanup pattern, and ensure the assertion matches the specification
in docs/moonmodules/.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb426d11-8e5c-4171-9181-387836ea56c6
⛔ Files ignored due to path filters (1)
moondeck/build/build_esp32.pyis excluded by!**/build/**
📒 Files selected for processing (15)
docs/moonmodules/core/services.mdesp32/main/CMakeLists.txtesp32/main/Kconfig.projbuildesp32/main/idf_component.ymlesp32/sdkconfig.defaults.ble-provisioningsrc/core/BleProvisioningModule.hsrc/core/NetworkModule.hsrc/main.cppsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32.cppsrc/platform/esp32/platform_esp32_ble_provisioning.cppsrc/platform/platform.htest/unit/core/unit_NetworkModule.cpp
| # Use protocomm Security 1 (X25519 + PoP + AES-CTR), with a per-device PoP | ||
| # derived from the MAC suffix at runtime. | ||
| CONFIG_ESP_PROTOCOMM_SUPPORT_SECURITY_VERSION_1=y |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For Espressif network_provisioning Security 1, confirm that the proof of possession must remain a unique per-device secret and assess whether a MAC-derived value provides authentication.
💡 Result:
For Espressif's Security 1 provisioning scheme, the proof of possession (PoP) is intended to be a unique per-device secret [1][2]. It functions as a security measure to ensure that only an authorized user—typically the person in physical possession of the device—can provision it [1][2]. While the protocol allows for a "No Auth" mode (Null PoP) where the shared key is derived through key exchange only [1][2], the use of a unique PoP is the recommended practice for authentication [1][2]. Regarding whether a MAC-derived value provides authentication, it is important to distinguish between the MAC address and the PoP itself. The Security 1 protocol uses an X25519 key exchange to derive a shared secret [1][3][4]. If a PoP is provided, this shared secret is then combined (specifically, XORed) with the SHA-256 hash of the PoP string [4]. This PoP string must be a secret known to both the device and the provisioning client to successfully authenticate the session [1][2][4]. Using a MAC address as a "derived value" (or as the PoP itself) is generally insecure for authentication purposes because MAC addresses are typically broadcast in cleartext and are not secret [1]. Official documentation emphasizes the need for a "unique per-device secret" [1][2], which should be a distinct value rather than a device's publicly available identifier, to ensure the security of the provisioning process [1]. If you rely solely on a device's public MAC address, you do not achieve the intended authentication security, as it fails to serve as a proof of possession of a secret [1].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/provisioning/provisioning.html
- 2: https://docs.espressif.com/projects/esp-idf/en/v6.0.1/esp32/api-reference/provisioning/provisioning.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v5.5.1/esp32/api-reference/provisioning/wifi_provisioning.html
- 4: https://developer.espressif.com/blog/2026/05/simple-provisioning/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced file and related provisioning code.
git ls-files 'esp32/*' | sed -n '1,200p'
printf '\n--- search ---\n'
rg -n -S 'ble-provisioning|PoP|proof of possession|mac suffix|MAC suffix|service name|service_name|security version 1|SECURITY_VERSION_1|protocomm' esp32 . --hidden --glob '!**/.git/**'
printf '\n--- file ---\n'
cat -n esp32/sdkconfig.defaults.ble-provisioningRepository: MoonModules/projectMM
Length of output: 4228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/platform/esp32/platform_esp32_ble_provisioning.cpp
printf '\n--- docs excerpt ---\n'
sed -n '1,120p' docs/moonmodules/core/services.mdRepository: MoonModules/projectMM
Length of output: 13889
Replace the MAC-derived PoP with a real secret. In src/platform/esp32/platform_esp32_ble_provisioning.cpp, makeIdentity() sets both the BLE service name and Security 1 PoP from mac[3..5], so the value is public and guessable. Generate a unique per-device secret instead and deliver it out of band.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esp32/sdkconfig.defaults.ble-provisioning` around lines 9 - 11, Update
makeIdentity() in the ESP32 BLE provisioning implementation so Security 1 uses a
unique, non-public per-device secret rather than the MAC suffix mac[3..5].
Preserve the existing BLE service-name identity separately, and ensure the
generated secret is delivered through the established out-of-band provisioning
mechanism.
| void onUpdate(const char* controlName) override { | ||
| if constexpr (platform::hasWiFi) { | ||
| if (!controlName) return; | ||
| const bool ssidChanged = std::strcmp(controlName, "ssid") == 0; | ||
| const bool passwordChanged = std::strcmp(controlName, "password") == 0; | ||
| const bool connectRequested = std::strcmp(controlName, "connectWifi") == 0; | ||
| if (!ssidChanged && !passwordChanged && !connectRequested) return; | ||
|
|
||
| if (ssid_[0] == 0) { | ||
| wifiCredentialApplyPending_ = false; | ||
| std::snprintf(statusBuf_, sizeof(statusBuf_), "WiFi SSID empty; AP active"); | ||
| setStatus(statusBuf_, Severity::Status); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
onUpdate()'s empty-SSID branch never actually starts AP — status text lies about radio state.
Compare with maybeApplyPendingWifiCredentials() (line 823-826), whose empty-ssid branch calls startAP() when not already in State::AP. The onUpdate() branch at lines 333-338 does neither that nor delegate to it — it just clears wifiCredentialApplyPending_ and prints "WiFi SSID empty; AP active" without transitioning state_. Since wifiCredentialApplyPending_ is always false by the time this branch runs (and stays false), the delayed-apply path in maybeApplyPendingWifiCredentials() never fires either — so clearing the SSID field via the web UI (or via any code path routed through onUpdate) leaves the device in whatever state it was already in (e.g. still associated as ConnectedSta) while displaying a status claiming AP is active.
🐛 Proposed fix
if (ssid_[0] == 0) {
wifiCredentialApplyPending_ = false;
- std::snprintf(statusBuf_, sizeof(statusBuf_), "WiFi SSID empty; AP active");
- setStatus(statusBuf_, Severity::Status);
+ if (state_ != State::AP) startAP();
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void onUpdate(const char* controlName) override { | |
| if constexpr (platform::hasWiFi) { | |
| if (!controlName) return; | |
| const bool ssidChanged = std::strcmp(controlName, "ssid") == 0; | |
| const bool passwordChanged = std::strcmp(controlName, "password") == 0; | |
| const bool connectRequested = std::strcmp(controlName, "connectWifi") == 0; | |
| if (!ssidChanged && !passwordChanged && !connectRequested) return; | |
| if (ssid_[0] == 0) { | |
| wifiCredentialApplyPending_ = false; | |
| std::snprintf(statusBuf_, sizeof(statusBuf_), "WiFi SSID empty; AP active"); | |
| setStatus(statusBuf_, Severity::Status); | |
| return; | |
| } | |
| void onUpdate(const char* controlName) override { | |
| if constexpr (platform::hasWiFi) { | |
| if (!controlName) return; | |
| const bool ssidChanged = std::strcmp(controlName, "ssid") == 0; | |
| const bool passwordChanged = std::strcmp(controlName, "password") == 0; | |
| const bool connectRequested = std::strcmp(controlName, "connectWifi") == 0; | |
| if (!ssidChanged && !passwordChanged && !connectRequested) return; | |
| if (ssid_[0] == 0) { | |
| wifiCredentialApplyPending_ = false; | |
| if (state_ != State::AP) startAP(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/NetworkModule.h` around lines 325 - 338, Update the empty-SSID
branch in onUpdate() to ensure AP mode is actually started, reusing the existing
behavior from maybeApplyPendingWifiCredentials(): transition via startAP() when
state_ is not already State::AP, then report the status. Preserve clearing
wifiCredentialApplyPending_ and avoid restarting AP when it is already active.
| struct BleProvisioningState { | ||
| char serviceName[32] = {}; | ||
| char pop[8] = {}; | ||
| char pendingSsid[33] = {}; | ||
| char pendingPassword[64] = {}; | ||
| char* ssidOut = nullptr; | ||
| size_t ssidOutLen = 0; | ||
| char* passwordOut = nullptr; | ||
| size_t passwordOutLen = 0; | ||
| std::atomic<bool>* ready = nullptr; | ||
| char* statusBuf = nullptr; | ||
| size_t statusBufLen = 0; | ||
| bool initialized = false; | ||
| bool started = false; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race on g_bleProv.started/initialized across threads.
bleProvEventCb (dispatched on the provisioning manager's own task, per NETWORK_PROV_END/NETWORK_PROV_DEINIT at lines 113-119) mutates the plain bool started/initialized fields of the global g_bleProv, while bleProvisioningInit()/bleProvisioningStop() (called from the module's main-thread loop1s()/teardown()) read and write the very same fields with no atomics or lock. This is a data race — most likely to bite if teardown() calls bleProvisioningStop() at the same moment provisioning legitimately ends (NETWORK_PROV_END), since both paths branch on started/initialized to decide whether to call network_prov_mgr_stop_provisioning() vs deinit().
Make these fields std::atomic<bool> (or guard with a small mutex) to remove the race.
🔒 Proposed fix
- bool initialized = false;
- bool started = false;
+ std::atomic<bool> initialized{false};
+ std::atomic<bool> started{false};Also applies to: 109-120, 128-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/esp32/platform_esp32_ble_provisioning.cpp` around lines 23 - 37,
Make the BleProvisioningState fields started and initialized atomic, then update
all accesses in bleProvEventCb, bleProvisioningInit, and bleProvisioningStop to
use atomic-safe reads and writes. Preserve the existing branching and lifecycle
behavior while ensuring concurrent provisioning callbacks and main-thread
teardown cannot race.
| void makeIdentity(const ImprovDeviceInfo& info) { | ||
| uint8_t mac[6] = {}; | ||
| getMacAddress(mac); | ||
|
|
||
| const char* name = (info.name && info.name[0]) ? info.name : "projectMM"; | ||
| std::snprintf(g_bleProv.serviceName, sizeof(g_bleProv.serviceName), | ||
| "%.20s-%02X%02X%02X", name, mac[3], mac[4], mac[5]); | ||
| std::snprintf(g_bleProv.pop, sizeof(g_bleProv.pop), | ||
| "%02X%02X%02X", mac[3], mac[4], mac[5]); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
PoP is derived from the same bytes shown in the public BLE service name — provides no real proof-of-possession.
makeIdentity() builds both serviceName and pop from the identical mac[3..5] bytes: makeIdentity computes the service name and PoP from the same three MAC bytes (see lines 71-74). Since the service name is broadcast in the BLE advertisement so a phone/attacker can even find the device, anyone who reads that name can read the PoP suffix directly — Security1's "proof of possession" handshake ends up authenticating with a secret that is, by construction, already public. This defeats the purpose of choosing NETWORK_PROV_SECURITY_1 at all: any nearby attacker can push arbitrary Wi-Fi credentials or read the ones sent by the legitimate owner.
Derive the PoP from something that is not also embedded in the advertised name (e.g. a different portion of a per-device secret, or a value printed on a label/shown over the already-secure USB-serial Improv channel), so the security session's secret isn't self-defeating.
Also applies to: 166-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/esp32/platform_esp32_ble_provisioning.cpp` around lines 66 - 75,
Update makeIdentity so g_bleProv.pop is derived from a private per-device secret
or another non-advertised provisioning source, rather than mac[3..5] used in
serviceName. Keep the BLE service name generation unchanged, and ensure the PoP
remains available through the intended secure channel or device labeling without
being recoverable from the advertised name.
| namespace { | ||
| struct ClockGuard { ~ClockGuard() { mm::platform::setTestNowMs(0); } }; | ||
| } | ||
|
|
||
| // Web-written credentials drive the same STA apply path as Improv, after both fields settle. | ||
| TEST_CASE("NetworkModule web credential controls schedule WiFi STA apply") { | ||
| ClockGuard clockGuard; | ||
| mm::platform::setTestNowMs(1000); | ||
|
|
||
| mm::Scheduler scheduler; | ||
| auto* net = new mm::NetworkModule(); | ||
| net->setName("Network"); | ||
| net->setScheduler(&scheduler); | ||
| scheduler.addModule(net); | ||
| scheduler.setup(); | ||
|
|
||
| CHECK(scheduler.setControl("Network", "ssid", "{\"value\":\"homeAP\"}") == mm::Scheduler::SetControlResult::Ok); | ||
| CHECK_FALSE(net->wifiCredentialApplyPendingForTest()); | ||
| CHECK(std::strcmp(net->status(), "WiFi SSID saved; press connect") == 0); | ||
|
|
||
| CHECK(scheduler.setControl("Network", "password", "{\"value\":\"secret123\"}") == mm::Scheduler::SetControlResult::Ok); | ||
| CHECK(net->wifiCredentialApplyPendingForTest()); | ||
| CHECK(std::strcmp(net->status(), "WiFi credentials saved; connecting soon") == 0); | ||
|
|
||
| mm::platform::setTestNowMs(1000 + mm::NetworkModule::wifiCredentialApplyDelayMsForTest() + 1); | ||
| net->loop1s(); | ||
|
|
||
| CHECK_FALSE(net->wifiCredentialApplyPendingForTest()); | ||
| CHECK(std::strcmp(net->status(), "No network") == 0); | ||
| scheduler.teardown(); | ||
| } | ||
|
|
||
| // The explicit connect button lets an open network apply with an empty password. | ||
| TEST_CASE("NetworkModule connectWifi applies open-network credentials") { | ||
| ClockGuard clockGuard; | ||
| mm::platform::setTestNowMs(1000); | ||
|
|
||
| mm::Scheduler scheduler; | ||
| auto* net = new mm::NetworkModule(); | ||
| net->setName("Network"); | ||
| net->setScheduler(&scheduler); | ||
| scheduler.addModule(net); | ||
| scheduler.setup(); | ||
|
|
||
| CHECK(scheduler.setControl("Network", "ssid", "{\"value\":\"openAP\"}") == mm::Scheduler::SetControlResult::Ok); | ||
| CHECK_FALSE(net->wifiCredentialApplyPendingForTest()); | ||
| CHECK(scheduler.setControl("Network", "connectWifi", "{\"value\":1}") == mm::Scheduler::SetControlResult::Ok); | ||
| CHECK_FALSE(net->wifiCredentialApplyPendingForTest()); | ||
| CHECK(std::strcmp(net->status(), "No network") == 0); | ||
| scheduler.teardown(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Tests correctly use the setTestNowMs/ClockGuard seam and match current desktop-stub behavior. Given the empty-SSID bug flagged in NetworkModule.h (onUpdate never calls startAP() when SSID is cleared), consider adding a regression test here: write ssid to a non-empty value, connect, then write ssid to "" via scheduler.setControl and assert the state actually reflects AP (not just a status string). Happy to draft this test alongside the fix if useful.
As per path instructions, "Verify tests cover edge cases and match the specifications in docs/moonmodules/."
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 101-101: The function 'appearsInUi' is never used.
(unusedFunction)
[style] 113-113: The function 'lastSavedStr' is never used.
(unusedFunction)
[style] 104-104: The function 'setSystemModule' is never used.
(unusedFunction)
[style] 73-73: The function 'moduleCount' is never used.
(unusedFunction)
[style] 74-74: The function 'module' is never used.
(unusedFunction)
[style] 95-95: The function 'instance' is never used.
(unusedFunction)
[style] 85-85: The function 'acceptsChildRoles' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/core/unit_NetworkModule.cpp` around lines 66 - 117, Extend the
NetworkModule regression coverage with a test that sets a non-empty SSID,
connects, then clears the SSID through scheduler.setControl and verifies the
module enters AP state using its state/API assertion rather than only checking
status text. Keep the test clock setup and ClockGuard cleanup pattern, and
ensure the assertion matches the specification in docs/moonmodules/.
Source: Path instructions
Drops the public MAC-derived proof-of-possession path, uses security 0 instead of pretending to have a secret, makes BLE provisioning state atomic, and restores AP mode when an empty SSID is applied through web controls. Checks: check_specs.py; git diff --check; build_esp32.py --firmware esp32s3-n16r8 (projectMM.bin 0x1cd630, total image 1889720 bytes). Notes: This PR is BLE Wi-Fi provisioning only; it does not add BLE firmware OTA.
1 participant
Summary
Validation
git diff --check origin/main..HEADmoondeck/check/check_specs.pymoondeck/check/check_platform_boundary.pymoondeck/build/build_esp32.py --firmware esp32s3-n16r8esp32s3-n16r8image:0x1cd430, 55% app partition freeNot Run
Summary by CodeRabbit