feat(wifi): Protect the captive portal AP with WPA2/WPA3 by default

.changes/ap-name-and-password.md

@@ -0,0 +1,15 @@
+---
+kind: added
+---
+Set the captive portal's network name and password, protected by default
+
+## Release Note
+The captive portal's WiFi network can now have its own name and a WPA2/WPA3 password, set in the captive portal's new Access Point section or over serial with `ap ssid` and `ap password`
+
+The portal's network is now protected by default with the password `openshock`, including on hubs that update from an older version. It is public, so it only keeps out passers-by. Phones and computers that support WPA3 get a connection that someone listening nearby can't decrypt even with the password, but anyone who knows it can set up a fake network with the same name. Set your own password or open the network with `ap open`. A factory reset restores the default password, so a forgotten one can always be recovered over USB.
+
+- Use the stored `wifi.accessPointSSID` for the access point; it was saved but never applied. The default name still gets the hub's MAC appended
+- Add `wifi.accessPointPassword` (NVS key `ap_pass` in `wifi.cfg`, exported in jsonconfig only with sensitive data): defaulting to the new Kconfig option `OPENSHOCK_FW_AP_PASSWORD` (`openshock`, checked at compile time), empty for an open network, else 8-63 printable ASCII characters, with WPA2/WPA3-PSK transition mode (WPA3 clients use SAE, WPA2 clients still connect)
+- Add the `ap` serial command: `ap` shows the name and whether it is protected, `ap ssid <name|default>`, `ap password <password|default>` and `ap open`
+- Add `PUT /api/ap` to the captive portal API (form body: `ssid`, empty for the default; `protection` `default`, `open` or `custom` with `password`; answers with the saved `ssid` and `protection`), and the access point's name and protection, never its password, to the portal's Ready message
+- Changes apply the next time the captive portal starts

components/captive_portal/src/captiveportal/ApiHandlers.cpp

@@ -19,6 +19,9 @@ const char* const TAG = "CaptivePortalApi";
 #include "OtaUpdateConfig.h"
 #include "RateLimiter.h"
 #include "util/TaskUtils.h"
+#include "wifi/AccessPoint.h"
+#include "wifi/WiFiConfig.h"
+#include "wifi/WiFiConstants.h"
 #include "wifi/WiFiManager.h"
 #include "wifi/WiFiScanManager.h"
 
@@ -43,6 +46,8 @@ static const char* const JSON_ERR_PASSWORD_LONG   = "{\"error\":\"PasswordTooLon
 static const char* const JSON_ERR_CODE_REQUIRED   = "{\"error\":\"CodeRequired\"}";
 static const char* const JSON_ERR_INVALID_CHANNEL = "{\"error\":\"InvalidChannel\"}";
 static const char* const JSON_ERR_RATE_LIMITED    = "{\"error\":\"RateLimited\"}";
+static const char* const JSON_ERR_INVALID_AP_SSID = "{\"error\":\"InvalidApSsid\"}";
+static const char* const JSON_ERR_INVALID_AP_PASS = "{\"error\":\"InvalidApPassword\"}";
 
 using namespace OpenShock;
 using namespace OpenShock::CaptivePortal;
@@ -227,7 +232,7 @@ static esp_err_t apiWifiNetworksAdd(httpd_req_t* req)
   if (!ssid && ssid.error() == ParamError::Invalid) {
     return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_SSID);
   }
-  if (!ssid || ssid->empty() || ssid->length() > 32) {
+  if (!ssid || ssid->empty() || ssid->length() > WiFiConstants::kSsidMaxLength) {
     return sendResp(req, S400, HTTP::ContentType::JSON, !ssid || ssid->empty() ? JSON_ERR_MISSING_SSID : JSON_ERR_INVALID_SSID);
   }
 
@@ -238,10 +243,10 @@ static esp_err_t apiWifiNetworksAdd(httpd_req_t* req)
   }
   std::string password = std::move(passwordParam).value_or(std::string());
   if (!password.empty()) {
-    if (password.length() < 8) {
+    if (password.length() < WiFiConstants::kPassphraseMinLength) {
       return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_PASSWORD_SHORT);
     }
-    if (password.length() > 63) {
+    if (password.length() > WiFiConstants::kPassphraseMaxLength) {
       return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_PASSWORD_LONG);
     }
   }
@@ -385,6 +390,121 @@ static esp_err_t apiOtaCheck(httpd_req_t* req)
   return sendResp(req, S200, nullptr, {});
 }
 
+// PUT /api/ap, form body: any of
+//   ssid=<name>                      the access point's name; empty for the default (the prefix with the hub's MAC)
+//   protection=default|open|custom   the build's default password, none, or `password`
+//   password=<8-63 printable ASCII>  with protection=custom
+// Applied the next time the captive portal starts: applying it now would drop the client that asked, which is most
+// likely on this access point.
+//
+// Answers {"ssid": <name, "" for the default>, "protection": "default"|"open"|"custom"} as the hub reads the saved
+// settings, since a name equal to the bare prefix or a password equal to the build's default are the defaults.
+static esp_err_t sendAccessPointSettings(httpd_req_t* req)
+{
+  std::string ssid     = Config::GetWiFiAccessPointSSID();
+  std::string password = Config::GetWiFiAccessPointPassword();
+
+  const char* protection = "custom";
+  switch (AccessPoint::ProtectionOf(password)) {
+    case AccessPoint::Protection::Open:
+      protection = "open";
+      break;
+    case AccessPoint::Protection::DefaultPassword:
+      protection = "default";
+      break;
+    case AccessPoint::Protection::CustomPassword:
+      break;
+  }
+
+  OpenShock::JSON::StringWriter writer;
+  json_gen_str_t* gen = writer.gen();
+  json_gen_start_object(gen);
+  OpenShock::JSON::objSetString(gen, "ssid", AccessPoint::IsDefaultSSID(ssid) ? std::string_view() : std::string_view(ssid));
+  OpenShock::JSON::objSetString(gen, "protection", protection);
+  json_gen_end_object(gen);
+  std::string json = writer.finish();
+  return sendResp(req, S200, HTTP::ContentType::JSON, json);
+}
+
+static esp_err_t apiAccessPoint(httpd_req_t* req)
+{
+  auto body = recvBody(req);
+  if (!body) {
+    return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_MISSING_PARAM);
+  }
+
+  bool resetSsid = false;
+  std::optional<std::string> ssid;
+  auto ssidParam = getFormParam(*body, "ssid");
+  if (!ssidParam && ssidParam.error() != ParamError::Missing) {
+    return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_AP_SSID);
+  }
+  if (ssidParam) {
+    if (ssidParam->empty()) {
+      resetSsid = true;
+    } else if (!AccessPoint::IsValidSSID(*ssidParam)) {
+      return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_AP_SSID);
+    } else {
+      ssid = std::move(*ssidParam);
+    }
+  }
+
+  bool resetPassword = false;
+  std::optional<std::string> password;
+  auto protection = getFormParam(*body, "protection");
+  if (!protection && protection.error() != ParamError::Missing) {
+    return sendParamError(req, protection.error());
+  }
+  if (protection) {
+    if (*protection == "default") {
+      resetPassword = true;
+    } else if (*protection == "open") {
+      password = std::string();
+    } else if (*protection == "custom") {
+      auto passwordParam = getFormParam(*body, "password");
+      if (!passwordParam) {
+        return sendResp(req, S400, HTTP::ContentType::JSON, passwordParam.error() == ParamError::Missing ? JSON_ERR_PASSWORD_SHORT : JSON_ERR_INVALID_AP_PASS);
+      }
+      if (passwordParam->length() < WiFiConstants::kPassphraseMinLength) {
+        return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_PASSWORD_SHORT);
+      }
+      if (passwordParam->length() > WiFiConstants::kPassphraseMaxLength) {
+        return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_PASSWORD_LONG);
+      }
+      if (!AccessPoint::IsValidPassword(*passwordParam)) {
+        return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_AP_PASS);
+      }
+      password = std::move(*passwordParam);
+    } else {
+      return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_INVALID_PARAM);
+    }
+  }
+
+  if (!resetSsid && !ssid && !resetPassword && !password) {
+    return sendResp(req, S400, HTTP::ContentType::JSON, JSON_ERR_MISSING_PARAM);
+  }
+
+  // Both passed the checks above, so a refusal here is the config store failing to save
+  bool saved = true;
+  if (resetSsid) {
+    saved = Config::ResetWiFiAccessPointSSID();
+  } else if (ssid) {
+    saved = Config::SetWiFiAccessPointSSID(std::move(*ssid));
+  }
+  if (!saved) {
+    return sendResp(req, S500, HTTP::ContentType::JSON, JSON_ERR_INTERNAL);
+  }
+  if (resetPassword) {
+    saved = Config::ResetWiFiAccessPointPassword();
+  } else if (password) {
+    saved = Config::SetWiFiAccessPointPassword(std::move(*password));
+  }
+  if (!saved) {
+    return sendResp(req, S500, HTTP::ContentType::JSON, JSON_ERR_INTERNAL);
+  }
+  return sendAccessPointSettings(req);
+}
+
 void CaptivePortal::RegisterApiHandlers(httpd_handle_t server, CaptivePortalInstance* instance)
 {
   registerHandler<apiBoard>(server, "/api/board", HTTP_GET, instance);
@@ -402,4 +522,5 @@ void CaptivePortal::RegisterApiHandlers(httpd_handle_t server, CaptivePortalInst
   // changing it would let anyone in range install firmware. It can only be changed over serial (jsonconfig).
   registerHandler<apiOtaSettings>(server, "/api/ota/settings", HTTP_PUT, instance);
   registerHandler<apiOtaCheck>(server, "/api/ota/check", HTTP_POST, instance);
+  registerHandler<apiAccessPoint>(server, "/api/ap", HTTP_PUT, instance);
 }

components/captive_portal/src/captiveportal/Manager.cpp

@@ -11,14 +11,13 @@ const char* const TAG = "CaptivePortal";
 #include "captiveportal/CaptivePortalConfig.h"
 #include "captiveportal/CaptivePortalInstance.h"
 #include "CommandHandler.h"
-#include "FormatHelpers.h"
 #include "GatewayConnectionManager.h"
 #include "Logging.h"
 #include "Temporal.h"
+#include "wifi/AccessPoint.h"
 #include "wifi/WiFiConfig.h"
 
 #include <esp_event.h>
-#include <esp_mac.h>
 #include <esp_netif.h>
 #include <esp_timer.h>
 #include <esp_wifi.h>
@@ -29,6 +28,7 @@ const char* const TAG = "CaptivePortal";
 #include "util/RetiredList.h"
 #include "util/TaskUtils.h"
 
+#include <algorithm>
 #include <atomic>
 #include <cstring>
 #include <memory>
@@ -82,6 +82,15 @@ static SemaphoreHandle_t s_apStarted = nullptr;
 static char s_apIfName[8]                   = {};
 static constexpr TickType_t kApStartTimeout = pdMS_TO_TICKS(2000);
 
+// A marker posted to the default event loop: once its handler runs, every event posted before it has been handled
+ESP_EVENT_DEFINE_BASE(CAPTIVEPORTAL_PRIVATE_EVENTS);
+enum {
+  CAPTIVEPORTAL_PRIVATE_EVENT_LOOP_FLUSH,
+};
+static SemaphoreHandle_t s_loopFlushed = nullptr;
+static std::atomic<uint32_t> s_loopFlushSeen {0};  // the latest marker the loop has handled
+static uint32_t s_loopFlushPosted = 0;             // markers posted so far; manager task only
+
 static_assert(IPv4Utils::IsSlash24Host(CONFIG_OPENSHOCK_PORTAL_AP_IP), "CONFIG_OPENSHOCK_PORTAL_AP_IP must be a dotted-decimal IPv4 address with a host octet of 1-254");
 
 static void sendCommand(uint32_t command)
@@ -153,6 +162,59 @@ static void captiveportal_onapstart(void*, esp_event_base_t, int32_t, void*)
   xSemaphoreGive(s_apStarted);
 }
 
+static void captiveportal_onloopflush(void*, esp_event_base_t, int32_t, void* data)
+{
+  s_loopFlushSeen.store(*static_cast<const uint32_t*>(data));
+  xSemaphoreGive(s_loopFlushed);
+}
+
+// Waits until the default event loop has handled everything posted to it before this call
+static bool waitForEventLoop(TickType_t timeout)
+{
+  // Each call posts its own marker and waits for that one: a marker left over from a call that timed out can still be
+  // handled later, and must not pass for this call's.
+  uint32_t marker = ++s_loopFlushPosted;
+  xSemaphoreTake(s_loopFlushed, 0);
+  if (esp_event_post(CAPTIVEPORTAL_PRIVATE_EVENTS, CAPTIVEPORTAL_PRIVATE_EVENT_LOOP_FLUSH, &marker, sizeof(marker), timeout) != ESP_OK) {
+    return false;
+  }
+
+  TickType_t deadline = xTaskGetTickCount() + timeout;
+  while (static_cast<int32_t>(s_loopFlushSeen.load() - marker) < 0) {
+    TickType_t remaining = deadline - xTaskGetTickCount();
+    if (static_cast<int32_t>(remaining) <= 0 || xSemaphoreTake(s_loopFlushed, remaining) != pdTRUE) {
+      return false;
+    }
+  }
+  return true;
+}
+
+// The settings are validated by the wifi section (Normalize), so they fit the driver's fields
+static wifi_config_t buildApConfig()
+{
+  const std::string ssid     = AccessPoint::BroadcastSSID(Config::GetWiFiAccessPointSSID());
+  const std::string password = Config::GetWiFiAccessPointPassword();
+
+  wifi_config_t apConfig = {};
+  apConfig.ap.ssid_len   = std::min(ssid.size(), sizeof(apConfig.ap.ssid));
+  memcpy(apConfig.ap.ssid, ssid.data(), apConfig.ap.ssid_len);
+  apConfig.ap.channel = 1;
+  if (password.empty()) {
+    apConfig.ap.authmode = WIFI_AUTH_OPEN;
+  } else {
+    // WPA3 clients use SAE, which a sniffer that knows the password still can't decrypt; WPA2 clients keep working.
+    // PMF stays optional because WPA2 clients may lack it, and the zeroed config would otherwise pick hunt-and-peck only.
+    apConfig.ap.authmode         = WIFI_AUTH_WPA2_WPA3_PSK;
+    apConfig.ap.pmf_cfg.required = false;
+    apConfig.ap.sae_pwe_h2e      = WPA3_SAE_PWE_BOTH;
+    std::size_t passwordLen      = std::min(password.size(), sizeof(apConfig.ap.password) - 1);
+    memcpy(apConfig.ap.password, password.data(), passwordLen);
+  }
+  apConfig.ap.max_connection  = CONFIG_OPENSHOCK_PORTAL_MAX_CLIENTS;
+  apConfig.ap.beacon_interval = 100;
+  return apConfig;
+}
+
 static bool captiveportal_start()
 {
   if (GetInstance() != nullptr) {
@@ -196,27 +258,19 @@ static bool captiveportal_start()
   static bool apStartHandlerRegistered = false;
   if (!apStartHandlerRegistered) {
     esp_err_t evtErr = esp_event_handler_register(WIFI_EVENT, WIFI_EVENT_AP_START, captiveportal_onapstart, nullptr);
+    if (evtErr == ESP_OK) {
+      evtErr = esp_event_handler_register(CAPTIVEPORTAL_PRIVATE_EVENTS, CAPTIVEPORTAL_PRIVATE_EVENT_LOOP_FLUSH, captiveportal_onloopflush, nullptr);
+    }
     if (evtErr != ESP_OK) {
-      OS_LOGE(TAG, "Failed to register the AP start handler: %s", esp_err_to_name(evtErr));
+      OS_LOGE(TAG, "Failed to register the AP start handlers: %s", esp_err_to_name(evtErr));
       return false;
     }
     apStartHandlerRegistered = true;
   }
 
-  // AP SSID = prefix + this device's STA MAC (matches the old Arduino naming).
-  uint8_t mac[6];
-  esp_read_mac(mac, ESP_MAC_WIFI_STA);
-
-  wifi_config_t apConfig = {};
-  snprintf(reinterpret_cast<char*>(apConfig.ap.ssid), sizeof(apConfig.ap.ssid), "%s" BSSID_FMT, CONFIG_OPENSHOCK_FW_AP_PREFIX, BSSID_ARG(mac));
-  apConfig.ap.ssid_len        = strlen(reinterpret_cast<char*>(apConfig.ap.ssid));
-  apConfig.ap.channel         = 1;
-  apConfig.ap.authmode        = WIFI_AUTH_OPEN;
-  apConfig.ap.max_connection  = CONFIG_OPENSHOCK_PORTAL_MAX_CLIENTS;
-  apConfig.ap.beacon_interval = 100;
+  wifi_config_t apConfig = buildApConfig();
 
   s_apIfName[0] = '\0';
-  xSemaphoreTake(s_apStarted, 0);  // drop a start left over from an earlier run
 
   esp_err_t err = esp_wifi_set_mode(WIFI_MODE_APSTA);
   if (err != ESP_OK) {
@@ -233,10 +287,21 @@ static bool captiveportal_start()
 
   // The servers bind to the AP's lwIP interface, which only exists once the AP has started. Reading its name earlier
   // returns another interface's (the STA's), and would put the portal on the LAN.
-  if (!esp_netif_is_netif_up(s_apNetif) && xSemaphoreTake(s_apStarted, kApStartTimeout) != pdTRUE) {
-    OS_LOGE(TAG, "AP did not start");
-    esp_wifi_set_mode(WIFI_MODE_STA);
-    return false;
+  //
+  // Enabling the AP starts it with the config it had, and setting a different config (a new name or password, or the
+  // first start after boot) restarts it: stopped, its netif removed from lwIP, then started again under a new name.
+  // Both starts are queued on the event loop by now. Reading the name between them gets nothing (the netif is gone)
+  // or the first start's (which no longer exists), so let the loop handle them first, then take the start that stays.
+  if (!waitForEventLoop(kApStartTimeout)) {
+    OS_LOGW(TAG, "The event loop did not catch up with the AP start, the interface name may be stale");
+  }
+  if (!esp_netif_is_netif_up(s_apNetif)) {
+    xSemaphoreTake(s_apStarted, 0);  // a start that has since been stopped again
+    if (!esp_netif_is_netif_up(s_apNetif) && xSemaphoreTake(s_apStarted, kApStartTimeout) != pdTRUE) {
+      OS_LOGE(TAG, "AP did not start");
+      esp_wifi_set_mode(WIFI_MODE_STA);
+      return false;
+    }
   }
 
   err = esp_netif_get_netif_impl_name(s_apNetif, s_apIfName);
@@ -412,7 +477,8 @@ bool CaptivePortal::Init()
 
   s_forceCloseDone = xSemaphoreCreateBinary();
   s_apStarted      = xSemaphoreCreateBinary();
-  if (s_forceCloseDone == nullptr || s_apStarted == nullptr) {
+  s_loopFlushed    = xSemaphoreCreateBinary();
+  if (s_forceCloseDone == nullptr || s_apStarted == nullptr || s_loopFlushed == nullptr) {
     OS_LOGE(TAG, "Failed to create captive portal semaphores");
     delete initialState;
     return false;

components/common/Kconfig

@@ -38,6 +38,15 @@ menu "OpenShock defaults"
         help
             Prefix for the captive-portal SoftAP SSID (the device MAC is appended).
 
+    config OPENSHOCK_FW_AP_PASSWORD
+        string "Default SoftAP password"
+        default "openshock"
+        help
+            Default password for the captive-portal SoftAP, which runs WPA2/WPA3 transition mode.
+            It is public, but WPA3 (SAE) clients still get traffic a sniffer can't decrypt with it.
+            Empty for an open network, else 8-63 characters (checked at compile time).
+            Overridable at runtime; a factory reset restores it.
+
 endmenu
 
 # Every OpenShock task's stack and priority, in one place so the priority order between them

components/common/host_test/main/test_stringutils.cpp

@@ -17,6 +17,7 @@ TEST_CASE("StringTrim variants", "[common][strhelpers]")
   TEST_ASSERT_TRUE(StringTrim("   ") == "");
   TEST_ASSERT_TRUE(StringTrim("nospace") == "nospace");
   TEST_ASSERT_TRUE(StringTrim("\t\n x \r\n") == "x");
+  static_assert(StringTrim(" x ") == "x");
 }
 
 TEST_CASE("StringHasPrefix / HasSuffix", "[common][strhelpers]")
@@ -81,6 +82,17 @@ TEST_CASE("StringIContains / StringHasPrefixIC", "[common][strhelpers]")
   TEST_ASSERT_FALSE(StringHasPrefixIC("hello", "world"));
 }
 
+TEST_CASE("StringIsPrintableAscii", "[common][strhelpers]")
+{
+  TEST_ASSERT_TRUE(StringIsPrintableAscii(""));
+  TEST_ASSERT_TRUE(StringIsPrintableAscii(" correct horse ~battery!"));
+  TEST_ASSERT_FALSE(StringIsPrintableAscii("tab\there"));
+  TEST_ASSERT_FALSE(StringIsPrintableAscii(std::string_view("nul\0byte", 8)));
+  TEST_ASSERT_FALSE(StringIsPrintableAscii("del\x7F"));
+  TEST_ASSERT_FALSE(StringIsPrintableAscii("caf\xC3\xA9"));  // UTF-8, so bytes >= 0x80 on a signed char
+  static_assert(StringIsPrintableAscii("abc"));
+}
+
 TEST_CASE("StringTrimRight on all-whitespace input", "[util][string]")
 {
   TEST_ASSERT_TRUE(StringTrimRight("   ").empty());