Merge pull request #362 from bigbrett/client-id-alias

client ID fixes

Author: billphipps

docs/src-ja/chapter03.md

@@ -46,7 +46,7 @@ whCommClientConfig commClientCfg = {
             .transport_cb      = transportMemClientCb,
             .transport_context = (void*)transportMemClientCtx,
             .transport_config  = (void*)transportMemCfg,
-            .client_id         = 123, /* 一意のクライアント識別子 */
+            .client_id         = 1, /* 一意のクライアント識別子。1-15（WOLFHSM_CFG_GLOBAL_KEYSなしでは0-15） */
 };
 
 /* 手順3: クライアント設定の割り当てと初期化 */

docs/src-ja/chapter05.md

@@ -88,7 +88,7 @@ whCommClientConfig commClientCfg[1] = {{
              .transport_cb      = transportMemClientCb,
              .transport_context = (void*)transportMemClientCtx,
              .transport_config  = (void*)transportMemCfg,
-             .client_id         = 123, /* 一意のクライアント識別子 */
+             .client_id         = 1, /* 一意のクライアント識別子。1-15（WOLFHSM_CFG_GLOBAL_KEYSなしでは0-15） */
 }};
 
 /* ステップ3: クライアント設定の割り当てと初期化 */
@@ -150,7 +150,7 @@ whCommClientConfig commClientCfg = {{
              .transport_cb      = posixTransportTcpCb,
              .transport_context = (void*)posixTransportTcpCtx,
              .transport_config  = (void*)posixTransportTcpCfg,
-             .client_id         = 123, /* 一意のクライアント識別子 */
+             .client_id         = 1, /* 一意のクライアント識別子。1-15（WOLFHSM_CFG_GLOBAL_KEYSなしでは0-15） */
 }};
 
 /* 以降のステップは同じ... */

docs/src/chapter03.md

@@ -45,7 +45,7 @@ whCommClientConfig commClientCfg = {
              .transport_cb      = transportMemClientCb,
              .transport_context = (void*)transportMemClientCtx,
              .transport_config  = (void*)transportMemCfg,
-             .client_id         = 123, /* unique client identifier */
+             .client_id         = 1, /* unique client identifier, 1-15 (or 0-15 without WOLFHSM_CFG_GLOBAL_KEYS) */
 };
 
 /* Step 3: Allocate and initialize the client configuration */

docs/src/chapter05.md

@@ -90,7 +90,7 @@ whCommClientConfig commClientCfg[1] = {{
              .transport_cb      = transportMemClientCb,
              .transport_context = (void*)transportMemClientCtx,
              .transport_config  = (void*)transportMemCfg,
-             .client_id         = 123, /* unique client identifier */
+             .client_id         = 1, /* unique client identifier, 1-15 (or 0-15 without WOLFHSM_CFG_GLOBAL_KEYS) */
 }};
 
 /* Step 3: Allocate and initialize the client configuration */
@@ -149,7 +149,7 @@ whCommClientConfig commClientCfg = {{
              .transport_cb      = posixTransportTcpCb,
              .transport_context = (void*)posixTransportTcpCtx,
              .transport_config  = (void*)posixTransportTcpCfg,
-             .client_id         = 123, /* unique client identifier */
+             .client_id         = 1, /* unique client identifier, 1-15 (or 0-15 without WOLFHSM_CFG_GLOBAL_KEYS) */
 }};
 
 /* Subsequent steps remain the same... */

src/wh_server.c

@@ -204,6 +204,10 @@ static int _wh_Server_HandleCommRequest(whServerContext* server,
         wh_MessageComm_TranslateInitRequest(magic,
                 (whMessageCommInitRequest*)req_packet, &req);
 
+        if (req.client_id > WH_CLIENT_ID_MAX) {
+            *out_resp_size = 0;
+            return WH_ERROR_BADARGS;
+        }
 #ifdef WOLFHSM_CFG_GLOBAL_KEYS
         /* USER=0 is reserved for global keys, client_id must be non-zero */
         if (req.client_id == WH_KEYUSER_GLOBAL) {

test/wh_test_clientserver.c

@@ -516,6 +516,49 @@ static int _clientServerSequentialTestConnectCb(void*           context,
                                   connected);
 }
 
+/* Drive one INIT cycle on an already-initialized client/server pair to verify
+ * the server enforces the [0, WH_CLIENT_ID_MAX] bound on client_id (so values
+ * that would be silently truncated by the 4-bit USER field of whKeyId — e.g.
+ * 16 aliasing the USER=0 global namespace, 17 aliasing client 1 — are
+ * rejected at INIT instead of breaking per-client key isolation). Mutates
+ * client->comm->client_id; caller is responsible for restoring it.
+ *
+ * Note: boundary coverage is limited to [0, 255] because
+ * client->comm->client_id is uint8_t; values above 255 are truncated by the
+ * host-side comm field before reaching the wire. */
+static int _testInitClientIdBoundary(whClientContext* client,
+                                     whServerContext* server,
+                                     uint32_t client_id, int expect_ok)
+{
+    uint32_t resp_client_id = 0;
+    uint32_t resp_server_id = 0;
+    int      rc;
+    uint8_t  prior_server_client_id = server->comm->client_id;
+
+    client->comm->client_id = (uint8_t)client_id;
+
+    WH_TEST_RETURN_ON_FAIL(wh_Client_CommInitRequest(client));
+    WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server));
+    rc = wh_Client_CommInitResponse(client, &resp_client_id, &resp_server_id);
+
+    if (expect_ok) {
+        WH_TEST_ASSERT_RETURN(rc == WH_ERROR_OK);
+        WH_TEST_ASSERT_RETURN(resp_client_id == client_id);
+    }
+    else {
+        /* Server returns BADARGS with size=0; client decodes that as ABORTED.
+         */
+        WH_TEST_ASSERT_RETURN(rc == WH_ERROR_ABORTED);
+        /* Rejection must not corrupt server-side identity: a regression that
+         * moved the assignment in _wh_Server_HandleCommRequest above the
+         * bound check would still produce ABORTED on the wire but would
+         * leave server->comm->client_id holding the rejected value. */
+        WH_TEST_ASSERT_RETURN(server->comm->client_id ==
+                              prior_server_client_id);
+    }
+    return WH_ERROR_OK;
+}
+
 static int _testOutOfBoundsNvmReads(whClientContext* client,
                                     whServerContext* server, whNvmId id)
 {
@@ -739,6 +782,24 @@ int whTest_ClientServerSequential(whTestNvmBackendType nvmType)
     WH_TEST_RETURN_ON_FAIL(wh_Client_CommInitResponse(client, &client_id, &server_id));
     WH_TEST_ASSERT_RETURN(client_id == client->comm->client_id);
 
+    /* Verify INIT rejects out-of-range client_id values that would otherwise
+     * be silently truncated by the 4-bit USER field of whKeyId. */
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 16, 0));
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 17, 0));
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 32, 0));
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 255, 0));
+#ifdef WOLFHSM_CFG_GLOBAL_KEYS
+    /* USER=0 is reserved for global keys */
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 0, 0));
+#else
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 0, 1));
+#endif
+    WH_TEST_RETURN_ON_FAIL(
+        _testInitClientIdBoundary(client, server, WH_CLIENT_ID_MAX, 1));
+    /* Restore default client_id so the rest of the sequential test runs with
+     * the expected per-client identity on both ends. */
+    WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(
+        client, server, WH_TEST_DEFAULT_CLIENT_ID, 1));
 
     /* Send the comm info message */
     WH_TEST_RETURN_ON_FAIL(wh_Client_CommInfoRequest(client));

test/wh_test_posix_threadsafe_stress.c

@@ -629,7 +629,8 @@ static int initClientServerPair(StressTestContext* ctx, int pairIndex)
     pair->clientCommConfig.transport_cb      = &clientTransportCb;
     pair->clientCommConfig.transport_context = &pair->clientTransportCtx;
     pair->clientCommConfig.transport_config  = &pair->tmConfig;
-    pair->clientCommConfig.client_id         = (uint16_t)(100 + pairIndex);
+    /* client_id must fit in WH_CLIENT_ID_MAX (4-bit USER field) */
+    pair->clientCommConfig.client_id = (uint8_t)(1 + pairIndex);
 
     /* Configure client */
     pair->clientConfig.comm = &pair->clientCommConfig;

wolfhsm/wh_keyid.h

@@ -46,6 +46,15 @@ typedef uint16_t whKeyId;
 #define WH_KEYTYPE_MASK 0xF000
 #define WH_KEYTYPE_SHIFT 12
 
+/* Maximum valid client_id. The USER field of whKeyId is 4 bits, so client_id
+ * must fit in [0, WH_CLIENT_ID_MAX]. Values outside this range would be
+ * silently truncated by WH_MAKE_KEYID, breaking per-client key isolation, so
+ * the server rejects them at WH_MESSAGE_COMM_ACTION_INIT. With
+ * WOLFHSM_CFG_GLOBAL_KEYS enabled, value 0 is reserved for the global-keys
+ * namespace and is also rejected. Derived from WH_KEYUSER_MASK so the bound
+ * stays in sync if the USER field is ever widened. */
+#define WH_CLIENT_ID_MAX (WH_KEYUSER_MASK >> WH_KEYUSER_SHIFT)
+
 /*
  * Client-facing key flags (temporary, stripped by server during translation)
  *
@@ -110,7 +119,9 @@ typedef uint16_t whKeyId;
  * @param type Key type to use as the TYPE field. Input value is ignored and
  *  WH_KEYTYPE_WRAPPED is used if the input clientId has the
  *  WH_CLIENT_KEYID_WRAPPED flag set.
- * @param clientId Client identifier to use as USER field
+ * @param clientId Client identifier to use as USER field. Must be in
+ *  [0, WH_CLIENT_ID_MAX]; the server enforces this at INIT so callers may
+ *  assume the value fits in the 4-bit USER field.
  * @param reqId Requested keyId from client (may include flags)
  * @return Server-internal keyId with TYPE, USER, and ID fields properly set.
  */