Add generic port API, POSIX reference port, template port, and porting guide#297
Add generic port API, POSIX reference port, template port, and porting guide#297AlexLanzano wants to merge 1 commit intowolfSSL:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new platform abstraction layer (wh_Port_*) so the client/server example entry points can be platform-independent, and adds a POSIX reference port plus a template port and porting documentation. It also wires up CI to build/run the POSIX generic port and includes a couple of small build/test fixes.
Changes:
- Add
wolfhsm/wh_port.hdefining thewh_Port_*porting API and new platform-agnostic generic client/server entry points underexamples/generic/. - Add a POSIX reference port (
port/posix/) with standalone client/server Makefiles and configuration headers, plus a template port to serve as a starting point for new ports. - Add a porting guide (
docs/draft/porting.md) and a GitHub Actions workflow to build/run the POSIX generic port in multiple configurations.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_port.h | Defines the new wh_Port_* abstraction API consumed by generic examples. |
| test/wh_test_wolfcrypt_test.c | Tightens _whClientTask compile guards to avoid client-only build errors. |
| port/template/server/wolfhsm_cfg.h | Template server-side wolfHSM config header for new ports. |
| port/template/server/wh_server_port.c | Template server implementation stubs for wh_Port_*. |
| port/template/server/user_settings.h | Template server wolfSSL feature configuration. |
| port/template/client/wolfhsm_cfg.h | Template client-side wolfHSM config header for new ports. |
| port/template/client/wh_client_port.c | Template client implementation stubs for wh_Port_*. |
| port/template/client/user_settings.h | Template client wolfSSL feature configuration. |
| port/template/README.md | Instructions for creating a new port from the template. |
| port/skeleton/README.md | Removes the old skeleton port README (template replaces skeleton). |
| port/posix/server/wolfhsm_cfg.h | POSIX server wolfHSM config (TCP + FIFO notify + RAM flash). |
| port/posix/server/wh_posix_server_port.c | POSIX server wh_Port_* implementation (TCP + FIFO notify + NVM). |
| port/posix/server/user_settings.h | POSIX server wolfSSL feature configuration. |
| port/posix/server/Makefile | Build rules for POSIX generic server binary. |
| port/posix/client/wolfhsm_cfg.h | POSIX client wolfHSM config (tests/bench enabled). |
| port/posix/client/wh_posix_client_port.c | POSIX client wh_Port_* implementation (TCP + FIFO notify + echo loop). |
| port/posix/client/user_settings.h | POSIX client wolfSSL feature configuration. |
| port/posix/client/Makefile | Build rules for POSIX generic client binary (includes tests/bench). |
| port/posix/Makefile | Top-level POSIX port Makefile delegating to client/server builds. |
| examples/generic/wh_generic_server.c | New generic server entry point using wh_Port_*. |
| examples/generic/wh_generic_client.c | New generic client entry point using wh_Port_*. |
| examples/generic/README.md | Documentation for building/running the generic examples via a port. |
| docs/draft/porting.md | New porting guide describing transport/NVM and wh_Port_* integration. |
| benchmark/wh_bench.c | Fixes include ordering by including wh_settings.h before system headers. |
| .github/workflows/build-and-run-generic-port.yml | Adds CI to build and run the POSIX generic port in multiple configs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Create the FIFO if it doesn't exist (EEXIST is OK) */ | ||
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0666) != 0 && errno != EEXIST) { | ||
| return WH_ERROR_ABORTED; | ||
| } | ||
| /* O_RDWR prevents EOF when no writer is connected yet */ | ||
| notifyFd = open(WOLFHSM_CFG_PORT_NOTIFY_PATH, O_RDWR | O_NONBLOCK); | ||
| if (notifyFd < 0) { | ||
| return WH_ERROR_ABORTED; |
There was a problem hiding this comment.
The FIFO is created with mode 0666, making it world-writable in /tmp. For a reference port that may be run on multi-user systems, it’s safer to restrict permissions (e.g. 0600/0660) and/or validate the existing path is actually a FIFO before using it.
| requests, key operations, crypto operations, etc. Should call | ||
| `wh_Client_CommClose()` and `wh_Client_Cleanup()` before returning. |
There was a problem hiding this comment.
This description of wh_Port_RunClient() says it should call wh_Client_CommClose() and wh_Client_Cleanup() before returning, but the port API also has a separate wh_Port_CleanupClient() and the generic client calls that after wh_Port_RunClient(). Update the guide to reflect the intended lifecycle (either: RunClient does not perform cleanup, or CleanupClient is optional/merged).
| requests, key operations, crypto operations, etc. Should call | |
| `wh_Client_CommClose()` and `wh_Client_Cleanup()` before returning. | |
| requests, key operations, crypto operations, etc. It should not perform final | |
| client cleanup; that is handled separately (for example by `wh_Port_CleanupClient()`). |
| WOLFHSM_CFG_PRINTF("Server configured, waiting for connections...\n"); | ||
|
|
||
| while (1) { | ||
| for (size_t i = 0; i < WOLFHSM_CFG_PORT_SERVER_COUNT; ++i) { | ||
| if (!isConnected[i]) { | ||
| if (wh_Port_ClientConnected(i)) { | ||
| err = wh_Port_InitServer(i, &serverCfg[i], &serverCtx[i]); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Port_InitServer(%zu) failed: %d\n", i, err); | ||
| goto loop; | ||
| } | ||
|
|
||
| err = wh_Server_SetConnected(&serverCtx[i], | ||
| WH_COMM_CONNECTED); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Server_SetConnected(%zu) failed: %d\n", i, err); | ||
| goto loop; | ||
| } | ||
| isConnected[i] = 1; | ||
| WOLFHSM_CFG_PRINTF("Client connected on instance %zu\n", i); | ||
| } | ||
| } | ||
| else { | ||
| if (wh_Port_ClientDisconnected(i)) { | ||
| WOLFHSM_CFG_PRINTF("Client disconnected on instance %zu\n", | ||
| i); | ||
| err = wh_Port_CleanupServer(i, &serverCtx[i]); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Port_CleanupServer(%zu) failed: %d\n", i, err); | ||
| goto loop; | ||
| } | ||
| isConnected[i] = 0; | ||
| } | ||
| else { | ||
| err = wh_Server_HandleRequestMessage(&serverCtx[i]); | ||
| if (err != WH_ERROR_OK && err != WH_ERROR_NOTREADY) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Server_HandleRequestMessage(%zu) failed: " | ||
| "%d\n", | ||
| i, err); | ||
| goto loop; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The server loop is a tight poll with no backoff when there are no connections or when wh_Server_HandleRequestMessage() returns WH_ERROR_NOTREADY. This can peg a CPU core. Consider adding a small sleep/yield on the no-work paths (similar to the existing POSIX example server) to reduce idle CPU usage.
| run: cd port/posix/server && make -j ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl | ||
|
|
||
| - name: Build generic client | ||
| run: cd port/posix/client && make -j ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl |
There was a problem hiding this comment.
make -j ${{ matrix.flags }} is passing variable assignments (e.g. ASAN=1) as the argument to -j, which expects a job count; this will fail with "invalid -j argument" in non-Standard matrix entries. Pass ${{ matrix.flags }} as separate make arguments (e.g. make -j ${{ runner.cpu_cores }} ${{ matrix.flags }} ...) or drop -j if you don’t want parallelism.
| run: cd port/posix/server && make -j ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl | |
| - name: Build generic client | |
| run: cd port/posix/client && make -j ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl | |
| run: cd port/posix/server && make -j ${{ runner.cpu_cores }} ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl | |
| - name: Build generic client | |
| run: cd port/posix/client && make -j ${{ runner.cpu_cores }} ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl |
| # Run client (will connect, run workload, and exit) | ||
| timeout 30 client/Build/client.elf & | ||
| CLIENT_PID=$! | ||
|
|
||
| # Wait for client to finish | ||
| wait $CLIENT_PID | ||
| CLIENT_RC=$? | ||
|
|
There was a problem hiding this comment.
The workflow expects the client to "connect, run workload, and exit", but the command backgrounds timeout 30 client/... and waits. Once the generic client is fixed to exit normally, this can be simplified to run timeout 30 ... in the foreground so failures/timeouts propagate more straightforwardly (and you don’t need CLIENT_PID).
| * @param[out] clientCtx Pointer to the client context to initialize. | ||
| * @return 0 on success, negative error code on failure. | ||
| */ | ||
| int wh_Port_InitClient(whClientConfig* clientCfg, whClientContext* clientCtx); |
There was a problem hiding this comment.
Const-correctness: after wh_Port_ConfigureClient() populates clientCfg, wh_Port_InitClient() appears to treat it as read-only (and wh_Client_Init() takes const whClientConfig*). Consider making the clientCfg parameter const whClientConfig* to prevent accidental mutation in port implementations.
| int wh_Port_InitClient(whClientConfig* clientCfg, whClientContext* clientCtx); | |
| int wh_Port_InitClient(const whClientConfig* clientCfg, | |
| whClientContext* clientCtx); |
| * @param[out] serverCtx Pointer to the server context to initialize. | ||
| * @return 0 on success, negative error code on failure. | ||
| */ | ||
| int wh_Port_InitServer(size_t instance, whServerConfig* serverCfg, |
There was a problem hiding this comment.
Const-correctness: similar to the client path, wh_Port_InitServer() could likely take const whServerConfig* since wh_Port_ConfigureServer() is responsible for populating it and wh_Server_Init() does not need to mutate the config. Using const here would better document ownership/intent for port authors.
| int wh_Port_InitServer(size_t instance, whServerConfig* serverCfg, | |
| int wh_Port_InitServer(size_t instance, const whServerConfig* serverCfg, |
| WOLFHSM_CFG_PRINTF("Client finished\n"); | ||
| wh_Port_CleanupBoard(); | ||
|
|
||
| loop: | ||
| while (1); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
main() always falls through to the loop: label and spins forever, even after successful client run/cleanup. This prevents the generic client from exiting (and will cause the CI workflow's timeout 30 client/... to hit the timeout and fail). Consider returning err (or 0 on success) instead of the infinite loop, and ensure the failure path also calls wh_Port_CleanupBoard() as needed before returning.
| err = wh_Port_InitBoard(); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF("wh_Port_InitBoard failed: %d\n", err); | ||
| goto loop; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < WOLFHSM_CFG_PORT_SERVER_COUNT; ++i) { | ||
| err = wh_Port_ConfigureServer(i, &serverCfg[i]); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF("wh_Port_ConfigureServer(%zu) failed: %d\n", i, | ||
| err); | ||
| goto loop; | ||
| } | ||
| } | ||
|
|
||
| WOLFHSM_CFG_PRINTF("Server configured, waiting for connections...\n"); | ||
|
|
||
| while (1) { | ||
| for (size_t i = 0; i < WOLFHSM_CFG_PORT_SERVER_COUNT; ++i) { | ||
| if (!isConnected[i]) { | ||
| if (wh_Port_ClientConnected(i)) { | ||
| err = wh_Port_InitServer(i, &serverCfg[i], &serverCtx[i]); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Port_InitServer(%zu) failed: %d\n", i, err); | ||
| goto loop; | ||
| } | ||
|
|
||
| err = wh_Server_SetConnected(&serverCtx[i], | ||
| WH_COMM_CONNECTED); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Server_SetConnected(%zu) failed: %d\n", i, err); | ||
| goto loop; | ||
| } | ||
| isConnected[i] = 1; | ||
| WOLFHSM_CFG_PRINTF("Client connected on instance %zu\n", i); | ||
| } | ||
| } | ||
| else { | ||
| if (wh_Port_ClientDisconnected(i)) { | ||
| WOLFHSM_CFG_PRINTF("Client disconnected on instance %zu\n", | ||
| i); | ||
| err = wh_Port_CleanupServer(i, &serverCtx[i]); | ||
| if (err) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Port_CleanupServer(%zu) failed: %d\n", i, err); | ||
| goto loop; | ||
| } | ||
| isConnected[i] = 0; | ||
| } | ||
| else { | ||
| err = wh_Server_HandleRequestMessage(&serverCtx[i]); | ||
| if (err != WH_ERROR_OK && err != WH_ERROR_NOTREADY) { | ||
| WOLFHSM_CFG_PRINTF( | ||
| "wh_Server_HandleRequestMessage(%zu) failed: " | ||
| "%d\n", | ||
| i, err); | ||
| goto loop; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| loop: | ||
| while (1) | ||
| ; | ||
| } |
There was a problem hiding this comment.
On error paths this server entry point jumps to loop: and busy-spins forever. For the POSIX reference port (and CI), it’s more useful to return a non-zero exit code (and/or call wh_Port_CleanupBoard()) so failures are surfaced immediately instead of hanging indefinitely.
| int wh_Port_InitBoard(void) | ||
| { | ||
| /* Create the FIFO if it doesn't exist (EEXIST is OK) */ | ||
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0666) != 0 && errno != EEXIST) { |
There was a problem hiding this comment.
The notification FIFO is created with permissions 0666 in /tmp. That makes it world-writable, allowing other local users to spoof connect/disconnect notifications. Consider using 0600 (or 0660 with a dedicated group), and/or placing it in a per-user/per-process directory to reduce the risk of interference.
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0666) != 0 && errno != EEXIST) { | |
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0600) != 0 && errno != EEXIST) { |
AlexLanzano
force-pushed
the
generic-port
branch
from
d460551 to
c5d6979
Compare
AlexLanzano
force-pushed
the
generic-port
branch
from
c5d6979 to
d4285b0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Build generic server | ||
| run: cd port/posix/server && make -j ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl | ||
|
|
||
| - name: Build generic client | ||
| run: cd port/posix/client && make -j ${{ matrix.flags }} WOLFSSL_DIR=../../../wolfssl |
There was a problem hiding this comment.
The PR description says CI will 'build and run' the POSIX generic port, but the workflow currently only builds. To match the description (and the workflow name), add steps to run the server in the background, run the client, and then terminate/clean up the server (with a timeout to avoid hangs).
| int wh_Port_ClientConnected(size_t instance) | ||
| { | ||
| uint8_t msg; | ||
| ssize_t rc; | ||
|
|
||
| (void)instance; | ||
|
|
||
| if (notifyFd < 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| rc = read(notifyFd, &msg, 1); | ||
| if (rc == 1 && msg == 1) { | ||
| return 1; | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| int wh_Port_ClientDisconnected(size_t instance) | ||
| { | ||
| uint8_t msg; | ||
| ssize_t rc; | ||
|
|
||
| (void)instance; | ||
|
|
||
| if (notifyFd < 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| rc = read(notifyFd, &msg, 1); | ||
| if (rc == 1 && msg == 0) { | ||
| return 1; | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Both polling functions read and consume a byte from the same FIFO. This can drop or misclassify events (e.g., wh_Port_ClientDisconnected() will consume a connect byte and discard it, losing the next connect notification). Consider reading from the FIFO in only one place and storing the last (or queued) event in static state so ClientConnected/ClientDisconnected can check/clear without performing separate reads; also handle unexpected bytes without silently discarding valid events.
| signal(SIGTERM, cleanupHandler); | ||
|
|
||
| /* Create the FIFO if it doesn't exist (EEXIST is OK) */ | ||
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0666) != 0 && errno != EEXIST) { |
There was a problem hiding this comment.
Creating a well-known FIFO under /tmp with mode 0666 makes it world-writable, allowing other local users/processes to spoof connection notifications (and potentially interfere with server lifecycle). Prefer a more restrictive mode (e.g., 0600 or 0660) and/or a per-user/per-process path (e.g., include UID/PID in the filename or use a private directory with 0700).
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0666) != 0 && errno != EEXIST) { | |
| if (mkfifo(WOLFHSM_CFG_PORT_NOTIFY_PATH, 0600) != 0 && errno != EEXIST) { |
| wh_Port_CleanupBoard(); | ||
|
|
||
| loop: | ||
| while (1); |
There was a problem hiding this comment.
On error paths this spins forever. For POSIX (and especially for CI if you start running the binaries) this will hang rather than exit with a failure code. Consider returning a non-zero status from main after attempting any necessary cleanup (e.g., wh_Port_CleanupBoard()), and avoid the infinite loop in non-embedded builds.
| while (1); | |
| if (err != 0) { | |
| /* On error, ensure board is cleaned up and exit with failure. */ | |
| wh_Port_CleanupBoard(); | |
| return 1; | |
| } |
| while (1) | ||
| ; |
There was a problem hiding this comment.
The error path also spins forever, which makes the generic server unsuitable for automation (and contradicts the stated intent to run this in CI). Consider exiting main with a non-zero status on failure and performing cleanup (e.g., wh_Port_CleanupBoard()) rather than an infinite loop.
| while (1) | |
| ; | |
| /* On error, clean up board resources and exit with non-zero status. */ | |
| wh_Port_CleanupBoard(); | |
| return err ? err : 1; |
| `wh_Client_Init()` followed by `wh_Client_CommInit()`. | ||
|
|
||
| ```c | ||
| int wh_Port_RunClient(whClientContext* clientCtx); |
There was a problem hiding this comment.
This guidance appears inconsistent with the API shape introduced in this PR: cleanup is handled by the dedicated wh_Port_CleanupClient() function (and in the generic client flow, wh_Port_RunClient() returns and cleanup happens afterward). Update the guide to avoid instructing ports to close/cleanup inside wh_Port_RunClient(), or clarify that only wh_Port_CleanupClient() is responsible for teardown.
Summary
wh_Port_*abstraction API (wolfhsm/wh_port.h) that decouples the generic client/server examples from any specific platform,providing functions for board init/cleanup, client configure/init/run/cleanup, and server configure/init/cleanup/connection polling
examples/generic/) that use thewh_Port_*API to run a wolfHSM client/server pair on anyplatform that implements the port
port/posix/) using TCP transport, FIFO-based connection notifications, and RAM-simulated flash, withseparate client/server Makefiles and configuration headers
port/template/) with documented stubs and TODO comments as a starting point for new platform ports, replacing the oldport/skeleton/docs/draft/porting.md) covering transport, flash/NVM, thewh_Port_*API, configuration headers, and build systemsetup
_whClientTaskguard intest/wh_test_wolfcrypt_test.cto require bothWOLFHSM_CFG_ENABLE_CLIENTandWOLFHSM_CFG_ENABLE_SERVER(avoids compile error in client-only builds)
wh_settings.hinclude before system headers inbenchmark/wh_bench.cto fix include ordering