fix: review findings on the token key

- Load the token key in Web::startup(), before either web server starts,
  so no token can be made with the all-zero key; log a failed
  pref.begin().
- createAPIToken checks mbedtls' return codes. On a failure it leaves
  the token empty, which tokenMatches never accepts, instead of hex
  from uninitialised stack.
- API_TOKEN_SIZE in ApiToken.h replaces the five char[65] buffers; the
  HMAC's description moves to createAPIToken, with the None form.
- handleLogin builds the candidate token once and compares it once.
- e2e: the serverId test skips on `secured` like its neighbours, names
  its values, and says it assumes the device sees this PC's address.
- KNOWN_ISSUES keeps #81, which #108 removes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs/KNOWN_ISSUES.md

@@ -5,6 +5,7 @@
 - [#77](https://github.com/cjkas/ESPSomfy-RTS/issues/77) Port 80 routes don't check login
 - [#78](https://github.com/cjkas/ESPSomfy-RTS/issues/78) CORS allows any origin
 - [#79](https://github.com/cjkas/ESPSomfy-RTS/issues/79) Websocket has no auth
+- [#81](https://github.com/cjkas/ESPSomfy-RTS/issues/81) `/login` has no rate limit
 - [#82](https://github.com/cjkas/ESPSomfy-RTS/issues/82) A saved WiFi passphrase or MQTT password can't be cleared from the UI
 - [#83](https://github.com/cjkas/ESPSomfy-RTS/issues/83) Installing "Main" from the update dialog always fails
 - [#84](https://github.com/cjkas/ESPSomfy-RTS/issues/84) Transmitting a frame stops `loop()` for about 365 ms

src/ApiToken.h

@@ -3,14 +3,16 @@
 #include <stddef.h>
 #include <stdint.h>
 
-// A token is HMAC-SHA256(token key, PIN or username:password + ":" + client
+// The token key's size: 32 random bytes the device makes on first boot and
-// IP) in hex.  The token key is 32 random bytes the device makes on first boot
+// keeps in NVS.  It never leaves the device.
-// and keeps in NVS: it never leaves the device.
 #define API_TOKEN_KEY_SIZE 32
+// A token's buffer: an HMAC-SHA256 in hex and its terminator.
+#define API_TOKEN_SIZE 65
 
 // True when given is the expected token.  It takes the same time whatever
 // given holds, so the time doesn't tell how much of a guess was right.  Only
-// expected's length shows, and every token has the same one.
+// expected's length shows, and every token has the same one.  An empty
+// expected token, which is what a failed HMAC leaves, matches nothing.
 inline bool tokenMatches(const char *expected, const char *given) {
   if(!expected || !given || !expected[0]) return false;
   uint8_t diff = 0;

src/DeviceOps.cpp

@@ -160,7 +160,7 @@ static void runSave(const proto_reply_to_t &to, settings_save_t *save) {
   const settings_section_t section = save->section;
   const uint16_t parts = settingsParts(section, req);
   bool reboot = false;
-  char token[65] = "";
+  char token[API_TOKEN_SIZE] = "";
   const security_types securityBefore = settings.Security.type;
   switch(section) {
     case settings_section_t::general:

src/entryPoints/Web.cpp

@@ -8,7 +8,6 @@
 #include <Preferences.h>
 #include <esp_random.h>
 #include <bootloader_random.h>
-#include "ApiToken.h"
 #include "ConfigSettings.h"
 #include "ConfigFile.h"
 #include "Utils.h"
@@ -66,11 +65,13 @@ static uint64_t loginNow() { return (uint64_t)esp_timer_get_time() / 1000; }
 // The tokens' HMAC key.  It has its own NVS namespace, which no settings
 // save clears and no backup holds, so only erasing NVS replaces it.
 static uint8_t tokenKey[API_TOKEN_KEY_SIZE];
-// Reads the token key, or makes one on first boot.  Runs before the radio is
+// Reads the token key, or makes one on first boot.  Runs before either web
-// on, so the hardware RNG gets its entropy from the bootloader's source.
+// server starts, so no token is made with an empty key, and before WiFi, so
+// the hardware RNG takes its entropy from the bootloader's source: calling it
+// once the radio is on would disturb WiFi.
 static void loadTokenKey() {
   Preferences pref;
-  pref.begin("TOKEN");
+  if(!pref.begin("TOKEN")) ESP_LOGE(TAG, "Couldn't open the token key's NVS namespace: every token changes at the next boot");
   if(pref.getBytes("key", tokenKey, sizeof(tokenKey)) != sizeof(tokenKey)) {
     bootloader_random_enable();
     esp_fill_random(tokenKey, sizeof(tokenKey));
@@ -108,6 +109,7 @@ AsyncWebServer asyncServer(80);
 AsyncWebServer asyncApiServer(8081);
 void Web::startup() {
   ESP_LOGI(TAG, "Launching web server...");
+  loadTokenKey();
   asyncApiServer.begin();
   ESP_LOGI(TAG, "Async API server started on port 8081");
 }
@@ -120,7 +122,7 @@ bool Web::isAuthenticated(AsyncWebServerRequest *request, bool cfg) {
   else if(!cfg && (settings.Security.permissions & static_cast<uint8_t>(security_permissions::ConfigOnly)) == 0x01) return true;
   else if(request->hasHeader("apikey")) {
     ESP_LOGD(TAG, "Checking API Key...");
-    char token[65];
+    char token[API_TOKEN_SIZE];
     memset(token, 0x00, sizeof(token));
     this->createAPIToken(request->client()->remoteIP(), token);
     if(!tokenMatches(token, request->getHeader("apikey")->value().c_str())) {
@@ -142,17 +144,24 @@ bool Web::createAPIPinToken(const IPAddress ipAddress, const char *pin, char *to
 bool Web::createAPIPasswordToken(const IPAddress ipAddress, const char *username, const char *password, char *token) {
   return this->createAPIToken((String(username) + ":" + String(password) + ":" + ipAddress.toString()).c_str(), token);
 }
+// Writes HMAC-SHA256(token key, payload) in hex to token, which holds
+// API_TOKEN_SIZE.  The payload is "username:password:ip" under a password,
+// "pin:ip" under a PIN and "ip" under None.  When mbedtls fails (it allocates
+// the HMAC context) token is left empty, which tokenMatches never accepts.
 bool Web::createAPIToken(const char *payload, char *token) {
     byte hmacResult[32];
     mbedtls_md_context_t ctx;
-    mbedtls_md_type_t md_type = MBEDTLS_MD_SHA256;
     mbedtls_md_init(&ctx);
-    mbedtls_md_setup(&ctx, mbedtls_md_info_from_type(md_type), 1);
+    bool ok = mbedtls_md_setup(&ctx, mbedtls_md_info_from_type(MBEDTLS_MD_SHA256), 1) == 0
-    mbedtls_md_hmac_starts(&ctx, tokenKey, sizeof(tokenKey));
+      && mbedtls_md_hmac_starts(&ctx, tokenKey, sizeof(tokenKey)) == 0
-    mbedtls_md_hmac_update(&ctx, (const unsigned char *)payload, strlen(payload));
+      && mbedtls_md_hmac_update(&ctx, (const unsigned char *)payload, strlen(payload)) == 0
-    mbedtls_md_hmac_finish(&ctx, hmacResult);
+      && mbedtls_md_hmac_finish(&ctx, hmacResult) == 0;
     mbedtls_md_free(&ctx);
     token[0] = '\0';
+    if(!ok) {
+      ESP_LOGE(TAG, "Couldn't make a token");
+      return false;
+    }
     for(int i = 0; i < sizeof(hmacResult); i++){
         char str[3];
         sprintf(str, "%02x", (int)hmacResult[i]);
@@ -161,11 +170,9 @@ bool Web::createAPIToken(const char *payload, char *token) {
     return true;
 }
 bool Web::createAPIToken(const IPAddress ipAddress, char *token) {
-    String payload;
+    if(settings.Security.type == security_types::Password) return createAPIPasswordToken(ipAddress, settings.Security.username, settings.Security.password, token);
-    if(settings.Security.type == security_types::Password) createAPIPasswordToken(ipAddress, settings.Security.username, settings.Security.password, token);
+    if(settings.Security.type == security_types::PinEntry) return createAPIPinToken(ipAddress, settings.Security.pin, token);
-    else if(settings.Security.type == security_types::PinEntry) createAPIPinToken(ipAddress, settings.Security.pin, token);
+    return createAPIToken(ipAddress.toString().c_str(), token);
-    else createAPIToken(ipAddress.toString().c_str(), token);
-    return true;
 }
 // =====================================================
 // Async API Handlers
@@ -359,7 +366,7 @@ void Web::handleController(AsyncWebServerRequest *request) {
 }
 void Web::handleLogin(AsyncWebServerRequest *request, JsonVariant &json) {
   if(request->method() == HTTP_OPTIONS) { request->send(200); return; }
-  char token[65];
+  char token[API_TOKEN_SIZE];
   memset(&token, 0x00, sizeof(token));
   this->createAPIToken(request->client()->remoteIP(), token);
   if(settings.Security.type == security_types::None) {
@@ -397,19 +404,11 @@ void Web::handleLogin(AsyncWebServerRequest *request, JsonVariant &json) {
     if(!obj["password"].isNull()) strlcpy(password, obj["password"], sizeof(password));
     if(!obj["pin"].isNull()) strlcpy(pin, obj["pin"], sizeof(pin));
   }
-  bool success = false;
+  // The token these credentials would get, against the stored ones'.
-  if(settings.Security.type == security_types::PinEntry) {
+  char ptok[API_TOKEN_SIZE] = "";
-    char ptok[65];
+  if(settings.Security.type == security_types::PinEntry) this->createAPIPinToken(ip, pin, ptok);
-    memset(ptok, 0x00, sizeof(ptok));
+  else if(settings.Security.type == security_types::Password) this->createAPIPasswordToken(ip, username, password, ptok);
-    this->createAPIPinToken(request->client()->remoteIP(), pin, ptok);
+  bool success = tokenMatches(token, ptok);
-    success = tokenMatches(token, ptok);
-  }
-  else if(settings.Security.type == security_types::Password) {
-    char ptok[65];
-    memset(ptok, 0x00, sizeof(ptok));
-    this->createAPIPasswordToken(request->client()->remoteIP(), username, password, ptok);
-    success = tokenMatches(token, ptok);
-  }
   if(success) {
     loginThrottle.succeeded(key);
     snprintf(g_async_content, sizeof(g_async_content),
@@ -789,7 +788,6 @@ static void timeRequest(AsyncWebServerRequest *request, ArMiddlewareNext next, c
 
 void Web::begin() {
   ESP_LOGI(TAG, "Creating Web MicroServices...");
-  loadTokenKey();
   asyncApiServer.addMiddleware([](AsyncWebServerRequest *r, ArMiddlewareNext next) { timeRequest(r, next, "api"); });
   asyncServer.addMiddleware([](AsyncWebServerRequest *r, ArMiddlewareNext next) { timeRequest(r, next, "web"); });
   // Async API Server (port 8081)

src/entryPoints/Web.h

@@ -1,6 +1,7 @@
 #include <ESPAsyncWebServer.h>
 #include <AsyncJson.h>
 #include "Somfy.h"
+#include "ApiToken.h"
 #ifndef webserver_h
 #define webserver_h
 
@@ -10,7 +11,7 @@ class Web {
     void startup();
     void begin();
     void loop();
-    // Auth helpers
+    // Auth helpers.  token holds API_TOKEN_SIZE; false leaves it empty.
     bool createAPIToken(const IPAddress ipAddress, char *token);
     bool createAPIToken(const char *payload, char *token);
     bool createAPIPinToken(const IPAddress ipAddress, const char *pin, char *token);

src/entryPoints/WebUiRoutes.cpp

@@ -601,7 +601,7 @@ void registerWebUiRoutes(AsyncWebServer &server) {
       JsonObject obj = json.as<JsonObject>();
       settings.Security.fromJSON(obj);
       settings.Security.save();
-      char token[65];
+      char token[API_TOKEN_SIZE];
       webServer.createAPIToken(request->client()->remoteIP(), token);
       JsonDocument sdoc;
       JsonObject sobj = sdoc.to<JsonObject>();

src/outputs/ProtoSocket.cpp

@@ -23,7 +23,6 @@
 #include "Utils.h"
 #include "SocketEmitter.h"
 #include "../entryPoints/Web.h"
-#include "ApiToken.h"
 #if HAS_ETHERNET
 #include <ETH.h>
 #endif
@@ -263,7 +262,7 @@ static bool authorized(AsyncWebServerRequest *request) {
   if(settings.Security.type == security_types::None) return true;
   const AsyncWebParameter *key = request->getParam("apikey");
   if(!key || !request->client()) return false;
-  char token[65];
+  char token[API_TOKEN_SIZE];
   webServer.createAPIToken(request->client()->remoteIP(), token);
   return tokenMatches(token, key->value().c_str());
 }

test/e2e/test_auth.py

@@ -38,22 +38,30 @@ def test_secured_refuses_a_wrong_token(host, secured):
     assert c.status == 401
 
 
-def test_secured_refuses_a_token_keyed_with_the_server_id(request, host, login_context, token):
+PASSWORD = 2  # the security type /loginContext reports for a password
-    """The token key used to be the public serverId, so anyone who saw a token could check PIN guesses against it."""
+
-    if token is None:
+
+def test_secured_refuses_a_token_keyed_with_the_server_id(request, host, secured, login_context, token):
+    """The token key used to be the public serverId, so anyone who saw a token could check PIN guesses against it.
+
+    It works out the token the old way, for this PC's address as the route to
+    the device picks it.  Through NAT or a VPN the device sees another address,
+    and the test passes without testing anything.
+    """
+    if not secured:
         pytest.skip("the device's security is None")
     with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as s:
         s.connect((host, 80))
-        me = s.getsockname()[0]
+        my_ip = s.getsockname()[0]
-    if login_context["type"] == 2:
+    if login_context["type"] == PASSWORD:
-        payload = f"{request.config.getoption('--username')}:{request.config.getoption('--password')}:{me}"
+        payload = f"{request.config.getoption('--username')}:{request.config.getoption('--password')}:{my_ip}"
     else:
-        payload = f"{request.config.getoption('--pin')}:{me}"
+        payload = f"{request.config.getoption('--pin')}:{my_ip}"
-    old = hmac.new(login_context["serverId"].encode(), payload.encode(), hashlib.sha256).hexdigest()
+    server_id_token = hmac.new(login_context["serverId"].encode(), payload.encode(), hashlib.sha256).hexdigest()
     # A bool, so a failure doesn't print the tokens.
-    same = old == token
+    is_device_token = server_id_token == token
-    assert not same, "the device's token is the one keyed with its serverId"
+    assert not is_device_token, "the device's token is the one keyed with its serverId"
-    c = ProtoClient(host, params={"apikey": old})
+    c = ProtoClient(host, params={"apikey": server_id_token})
     assert c.status == 401