Add the WolfCertTransport to extend the portability on embedded platforms - #15
Add the WolfCertTransport to extend the portability on embedded platforms#15yosuke-wolfssl wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors wolfCert’s HTTP/TLS I/O so the library no longer assumes BSD sockets: all network traffic (including TLS records) is routed through a new WolfCertTransport vtable, with the POSIX sockets implementation moved into a removable src/net_posix.c. This improves portability for embedded/RTOS targets while keeping the default POSIX path as the built-in transport.
Changes:
- Introduces
WolfCertTransportand threads it through request/session/server configs; deprecates the fd-basedconnect_cb. - Removes POSIX syscalls/headers from core HTTP and CSR paths by adding a CBIO bridge for wolfSSL and a portable IP-literal parser.
- Adds build/config gating for “platform pieces” (built-in transport and POSIX store), updates tests/CI/docs, and adds
WOLFCERT_ERR_CONN_CLOSED.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| wolfcert/types.h | Adds WolfCertTransport and extends config structs with transport. |
| wolfcert/options.h.in | Adds generated feature macros for platform gating. |
| wolfcert/http.h | Exposes transport in public HTTP request/session configs and updates docs/comments. |
| wolfcert/errors.h | Adds WOLFCERT_ERR_CONN_CLOSED. |
| wolfcert/check_config.h | Enforces HAVE_SNI unless WOLFCERT_NO_SNI is defined. |
| src/http.c | Core refactor: transport-owned I/O, wolfSSL CBIO bridge, locale-stable header comparisons, legacy adapter. |
| src/net_posix.c | Implements built-in POSIX WolfCertTransport + legacy connect adapter. |
| src/internal.h | Declares wolfcert_parse_ip and transport helpers/externs. |
| src/internal.c | Implements portable IPv4/IPv6 literal parsing. |
| src/csr.c | Switches SAN iPAddress encoding to wolfcert_parse_ip (removes inet_pton). |
| src/store.c | Compile-gates POSIX store backend and provides stubs when disabled. |
| src/errors.c | Adds strerror text for WOLFCERT_ERR_CONN_CLOSED. |
| src/est/est_client.c | Propagates transport from server cfg into HTTP request/session configs. |
| src/scep/scep_client.c | Propagates transport from server cfg into HTTP request/session configs. |
| cli/wolfcert_client.c | Drops explicit connect_cb use so CLI uses the default built-in transport. |
| tests/unit/test_transport.c | New unit tests covering scripted transport behavior and edge cases. |
| tests/unit/test_http.c | Adds conflict test for connect_cb + transport; marks skip when builtin transport is off. |
| tests/unit/test_store.c | Skips POSIX store tests when POSIX store is compiled out. |
| tests/unit/test_parse_negative.c | Adds negative/positive coverage for new IP literal parser. |
| tests/unit/test_est.c | Marks skip when builtin transport is off. |
| tests/unit/test_csr.c | Adds CSR SAN iPAddress assertions (v4/v6). |
| tests/integration/test_tls_http.c | Marks skip when builtin transport is off. |
| tests/CMakeLists.txt | Adds test_transport, gates socket tests on builtin transport, sets skip return codes. |
| CMakeLists.txt | Adds options for POSIX store / builtin transport and gates sources accordingly. |
| configure.ac | Adds autoconf flags for builtin transport / POSIX store and emits options.h macros. |
| Makefile.am | Gates src/net_posix.c and socket-driven tests on builtin transport. |
| examples/user_settings.h.example | Documents the new platform feature macros for user-settings builds. |
| scripts/ci/build-wolfssl.sh | Enables SNI in the CI wolfSSL build. |
| scripts/ci/freestanding-user_settings.h | Adds a no-sockets/no-files freestanding settings bundle for CI gating. |
| scripts/ci/compile-freestanding.sh | New compile-only “no POSIX headers” gate for portable sources. |
| .github/workflows/pr.yml | Adds CI matrix row for “platform pieces off” and a freestanding ARM compile job. |
| .github/workflows/nightly.yml | Adds nightly run for “platform pieces off” build. |
| README.md | Documents transport vtable portability in feature overview and enables SNI in example configure line. |
| docs/ARCHITECTURE.md | Adds full transport contract/bridge documentation. |
| docs/EMBEDDED.md | Documents targets without sockets/filesystem and new build flags/macros. |
| docs/MIGRATING-FROM-WOLFSCEP.md | Updates migration guidance to use WolfCertTransport instead of connect_cb. |
| docs/CI.md | Documents new CI gates (cmake-no-builtin-transport, no-posix-arm). |
| CLAUDE.md | Updates canonical wolfSSL configure line to include --enable-sni. |
Suppressed comments (1)
src/http.c:502
- wolfcert_cbio_send collapses WOLFCERT_ERR_WANT_READ into WOLFSSL_CBIO_ERR_WANT_WRITE. A transport write() can legitimately return WANT_READ in non-blocking mode; mapping it to WANT_WRITE makes wolfSSL report the wrong condition and breaks event-loop readiness handling.
switch (r) {
case WOLFCERT_ERR_WANT_READ:
case WOLFCERT_ERR_WANT_WRITE:
return WOLFSSL_CBIO_ERR_WANT_WRITE;
case WOLFCERT_ERR_CONN_CLOSED:
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #15
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
43e1e76 to
ba1aec2
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #15
Scan targets checked: none
Failed targets: wolfcert-bugs, wolfcert-src
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #15
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
ba1aec2 to
c48b404
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #15
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
c48b404 to
d154d01
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #15
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Blocking findings require changes before merge.
wolfIO_Recv and wolfIO_Send are declared under USE_WOLFSSL_IO || WOLFSSL_USER_IO || HAVE_HTTP_CLIENT, but defined only under USE_WOLFSSL_IO, which wolfSSL auto-defines only when none of WOLFSSL_USER_IO, MICRIUM, WOLFSSL_CONTIKI or WOLFSSL_NO_SOCK is set. The built-in transport therefore compiled but failed to link against such a wolfSSL, with undefined references to both symbols. It now calls recv and send directly and translates errno itself: EINTR retries, EAGAIN/EWOULDBLOCK map to the caller's WANT_READ or WANT_WRITE, ECONNRESET and EPIPE to CONN_CLOSED. That covers a superset of what TranslateIoReturnCode reported, and lets the file drop its wolfssl/wolfio.h and wolfssl/error-ssl.h includes.
5dbba8a to
31df8f3
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #15
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
The issues are solved
|
Hi @Frauschi , |
| /* Deprecated: use WolfCertTransport, never both on one config (BAD_ARG). An | ||
| * fd cannot carry a non-socket handle, which is why this is going away. | ||
| * Return it blocking; its own SO_RCVTIMEO / SO_SNDTIMEO then bound the I/O. */ | ||
| typedef int (*WolfCertConnectFn)(const char* host, int port, |
There was a problem hiding this comment.
As I consider wolfCert currently still an "alpha" or at max a "beta", I think we don't have to go through a deprecation process for the WolfCertConnectFn callback. I'd outright completely remove it with its context parameter and fully replace it with the new WolfCertTransport.
| * this request. NULL = library default. */ | ||
| void* heap; | ||
|
|
||
| const WolfCertTransport* transport; |
There was a problem hiding this comment.
Should we take that as a full parameter here instead of a pointer? When it is a pointer here, the user has to create a standalone WolfCertTransport structure somewhere with the same lifetime as the WolfCertServerCfg. If we take a copy of that instead here in the config, the user can inline the WolfCertTransport object and doesn't have to handle it separately.
| extern const WolfCertTransport wolfcert_posix_transport; | ||
| extern const WolfCertTransport wolfcert_legacy_transport; | ||
| int wolfcert_transport_is_fd_backed(const WolfCertTransport* t); | ||
| int wolfcert_legacy_connect(WolfCertConnectFn cb, void* cb_ctx, |
There was a problem hiding this comment.
I'd remove the whole legacy stuff together with the connect_cb (see comment in wolfcert/types.h).
Frauschi
left a comment
There was a problem hiding this comment.
Read through the transport series. The shape is right: making the POSIX path one instance of the vtable instead of a privileged path is the correct call, and routing TLS records through the same read/write pair - so the default build exercises the bridge on every test - is the detail that makes it trustworthy rather than just plausible.
One blocker. The two new WOLFCERT_HAVE_* platform gates have no check_config.h guard, so a WOLFCERT_USER_SETTINGS integrator carrying a user_settings.h written before this PR gets a library that compiles and links clean and then fails every request at runtime with no diagnostic. CI can't see it because the header-only job copies the updated example.
The rest is mostly contract gaps in src/net_posix.c. That file is the one every port will copy, so the errno mapping and the timeout_ms > 0 handling are worth getting right there rather than in each integrator's glue. Beyond that: dial() doesn't validate what connect returns, and the session_fd comments in est.h / scep.h still promise a descriptor that a custom transport cannot give.
Nothing here re-opens the SIGPIPE, CBIO-direction or deferred-transport-test threads - those are already answered.
| /* SNI. wolfSSL leaves HAVE_SNI off by default on most cross builds; without it | ||
| * a hosted EST endpoint serves its default certificate and verification then | ||
| * fails. WOLFCERT_NO_SNI accepts that trade for a smaller build. */ | ||
| #if !defined(HAVE_SNI) && !defined(WOLFCERT_NO_SNI) |
There was a problem hiding this comment.
You added a guard here for HAVE_SNI, but not for the two macros this PR introduces. WOLFCERT_HAVE_BUILTIN_TRANSPORT and WOLFCERT_HAVE_POSIX_STORE are resolved only from the generated options.h or the application's own user_settings.h, and an absent macro means "compiled out": dial() refuses every connection with WOLFCERT_ERR_BAD_ARG and wolfcert_store_posix_open() returns NULL.
Every user_settings.h written before this PR defines neither. So a WOLFCERT_USER_SETTINGS integrator upgrades, compiles and links clean, and then loses all networking and the file store at runtime with no diagnostic anywhere. CI can't catch it because the header-only job copies the updated examples/user_settings.h.example. CMake and autoconf are unaffected - they always emit both macros.
Please require an explicit opt-out the way the SNI check does:
#if !defined(WOLFCERT_HAVE_BUILTIN_TRANSPORT) && !defined(WOLFCERT_NO_BUILTIN_TRANSPORT)
#error "Define WOLFCERT_HAVE_BUILTIN_TRANSPORT for the built-in POSIX socket transport, or WOLFCERT_NO_BUILTIN_TRANSPORT to confirm you supply WolfCertServerCfg.transport yourself."
#endifand the same for WOLFCERT_HAVE_POSIX_STORE.
| return WOLFCERT_ERR(WOLFCERT_ERR_BAD_ARG, "http", | ||
| "transport must supply connect, read, write and disconnect"); | ||
|
|
||
| rc = t->connect(t->ctx, host, port, timeout_ms, &c->handle); |
There was a problem hiding this comment.
dial() range-checks every other transport callback but passes connect's return straight through. A transport that returns a positive value - a descriptor, a handle, or a byte-count-style 1, which is exactly what its sibling read/write callbacks do - gets two things wrong at once. c->connected stays 0, so conn_close() never calls disconnect on a connection the transport really did open, breaking the "disconnect runs exactly once per successful connect" contract this PR documents. And the positive value propagates out of wolfcert_http_request() and on through wolfcert_est_* / wolfcert_scep_*, all of which do if (rc != WOLFCERT_OK) return rc; - so a caller using the usual if (rc < 0) idiom reads it as success.
Normalising it is two lines:
if (rc != WOLFCERT_OK)
return rc < 0 ? rc : WOLFCERT_ERR(WOLFCERT_ERR_IO, "http",
"transport connect returned %d, not 0 or a WOLFCERT_ERR_*", rc);While you're here: connect's return contract isn't stated in the WolfCertTransport comment in types.h - only read and write get one. Worth adding. Same for the other easy mistake in a hand-written connect: returning OK on a path that never writes *conn leaves handle 0 in play, which for the shipped posix_disconnect means close(0).
|
|
||
| /* poll() can report a readiness the transfer then declines. Only an | ||
| * unbounded caller waits again; the others report it. */ | ||
| if (!WOLFCERT_WOULDBLOCK(errno)) |
There was a problem hiding this comment.
The commit message says the built-in transport maps "ECONNRESET and EPIPE to CONN_CLOSED", but both posix_read and posix_write collapse every non-EAGAIN errno into WOLFCERT_ERR_IO, so the two are indistinguishable from a genuine fault.
It bites on exactly the path CONN_CLOSED was added for: a server that ends an unframed response body with an RST rather than an orderly FIN gives ECONNRESET -> WOLFCERT_ERR_IO -> read_body fails the whole request, where CONN_CLOSED would have ended the body and returned what arrived. Since this file is the reference implementation every port copies, either add the mapping or correct the commit message and the contract docs so nobody copies behaviour that isn't there.
| * undefined. Returned as -1 if the session is not open. Intended for | ||
| * event loops - the caller polls for POLLIN / POLLOUT depending on | ||
| * the last WOLFCERT_ERR_WANT_READ / _WANT_WRITE the library returned. */ | ||
| /* Socket descriptor of the open session, for event loops: poll POLLIN / |
There was a problem hiding this comment.
You narrowed the contract here, but the two headers an application actually reads for the session APIs didn't follow. wolfcert/est.h:266 and wolfcert/scep.h:269 still say only "Socket fd of the backing HTTP session - hand to poll/epoll/kqueue", and wolfcert/scep.h:239 still names polling wolfcert_scep_session_fd() as the sole way to drive a _nb loop.
Both forward straight to this function, so a caller with a custom transport follows the header comment and polls -1 forever. Worth mirroring this wording into both.
| } | ||
|
|
||
| if (sni_host != NULL) { | ||
| #ifdef HAVE_SNI |
There was a problem hiding this comment.
check_config.h treats WOLFCERT_NO_SNI as the opt-out and docs/EMBEDDED.md describes it as "define WOLFCERT_NO_SNI to build without it", but the call site keys off wolfSSL's macro instead. Against a wolfSSL that does have SNI compiled in, defining WOLFCERT_NO_SNI changes nothing at all - the extension still goes out. It's really WOLFCERT_ALLOW_NO_SNI. Either honour it here, or reword EMBEDDED.md and the #error text to say it only waives the wolfSSL build requirement.
Worth a release note separately: check_config.h reaches applications through types.h, so the new HAVE_SNI #error fires when a user's code compiles, not just the library - including SCEP-only integrations that never open a TLS connection. Everyone who followed the previously documented configure line hits it.
| #ifdef HAVE_SNI | |
| #if defined(HAVE_SNI) && !defined(WOLFCERT_NO_SNI) |
| test_csr_LDADD = libwolfcert.la $(WOLFSSL_LIBS) | ||
| test_store_SOURCES = tests/unit/test_store.c | ||
| test_store_LDADD = libwolfcert.la $(WOLFSSL_LIBS) | ||
| if WOLFCERT_HAVE_BUILTIN_TRANSPORT |
There was a problem hiding this comment.
This if WOLFCERT_HAVE_BUILTIN_TRANSPORT / check_PROGRAMS += test_net / endif block lands in the middle of the run of test_*_SOURCES / _LDADD pairs, splitting test_store_* from test_net_* and leaving test_transport_* sitting between the conditional and the test_net_SOURCES it gates.
Functionally fine - automake handles a conditionally-listed program with unconditional _SOURCES. But the file's convention elsewhere, including the libwolfcert_la_SOURCES change earlier in this same diff, is to keep check_PROGRAMS composition together at the top and the per-program variables below, and this is the file check-buildsystem-parity.sh greps. Worth moving up next to the check_PROGRAMS list.
| return WOLFCERT_ERR_IO; | ||
|
|
||
| /* This socket is ours, so poll() can own the timeout semantics. */ | ||
| if (set_nonblock(fd) != WOLFCERT_OK) { |
There was a problem hiding this comment.
posix_connect sets O_NONBLOCK on every connection and then emulates blocking internally with poll(fd, ..., -1). Before this PR that fcntl only ran when cfg->nonblocking was set, so a blocking session's descriptor stayed blocking.
wolfcert_http_session_fd() still hands that descriptor to the application, so anyone who took the fd from a blocking session and did direct I/O on it - imposing their own idle timeout, a getsockopt/setsockopt, a drain before close - now gets EAGAIN where they previously blocked, with nothing in the API signalling the change.
Either pass the blocking mode into posix_connect, or - if the unconditional O_NONBLOCK is deliberate, which the legacy_read / legacy_write split suggests it is - say in wolfcert/http.h that the descriptor is always non-blocking and is for readiness polling only.
| } | ||
|
|
||
| /* ---- WolfCertTransport instance ----------------------------------------- */ | ||
|
|
There was a problem hiding this comment.
types.h and ARCHITECTURE.md 4.6 both document three timeout_ms modes for read/write, and the built-in implementation gets > 0 wrong in two places. posix_wait reports an expired positive timeout as WOLFCERT_ERR_IO, indistinguishable from a real I/O failure. And when poll() reports a readiness the transfer then declines with EAGAIN, posix_read/posix_write fall into if (timeout_ms >= 0) return WOLFCERT_ERR_WANT_READ; - telling a caller who asked for a bounded wait "would block" without the budget having been spent.
Nothing in wolfCert reaches it today; it only ever passes 0 or -1. It matters because EMBEDDED.md points integrators at this file as the model for their FreeRTOS+TCP / NetX / lwIP / wolfIP glue, so the bug gets copied outward. Either track a deadline using the mono_ms() already in this file, or drop the > 0 mode from the documented contract rather than leaving a reference implementation that gets it wrong.
| do { | ||
| rc = recv(c->fd, buf, len, 0); | ||
| if (c->ssl == NULL) { | ||
| r = c->t->read(c->t->ctx, c->handle, (uint8_t*)buf, len, |
There was a problem hiding this comment.
Wording point on these three guards. The comment describes catching a transport that "already overran the buffer", but by the time the count is inspected the write into buf has happened - the check bounds the accounting (sm_rx_len, rx->len), not the write. Same at nb_read_some and wolfcert_cbio_recv.
The accounting guard is worth keeping; it's only the claim that's too strong. Worth saying in ARCHITECTURE.md 4.6 that respecting len is the transport's responsibility and exceeding it is undefined, so a glue-file author doesn't assume the library is backstopping them.
Problem
wolfCert could not run on a stack without BSD sockets. It had a hook for
opening a connection (
connect_cb, shaped as a file descriptor) but keptownership of the socket lifecycle:
close,fcntl,sendandrecvsatinline in
src/http.c, and the raw descriptor went into wolfSSL throughwolfSSL_set_fd, which overwrites any custom CBIO context the application hadinstalled.
src/csr.candsrc/store.calso required POSIX headers.Fix (
src/http.c)A
WolfCertTransportvtable now owns every byte on the wire, TLS recordsincluded:
src/net_posix.ccarries the built-in POSIX instance, making it oneimplementation of the vtable rather than a privileged path. It calls
recvand
senddirectly and depends on none of wolfSSL's optionalwolfIO_Send/wolfIO_Recvhelpers, which are absent underWOLFSSL_USER_IO,MICRIUM,WOLFSSL_CONTIKIandWOLFSSL_NO_SOCK.read/writepair, andwolfSSL_set_fdis gone from the client. The bridgeis on every path including the default one, so the existing round-trip suite
exercises it.
src/http.cperforms no syscall and includes no POSIX header. Neither dosrc/csr.c, where a local IP-literal parser replacesinet_pton, orsrc/store.c, whose file backend is now compile-gated.Public additions, all backward compatible:
WolfCertTransport, plus atransportfield at the end of the three config structswolfcert/types.h,wolfcert/http.hWOLFCERT_ERR_CONN_CLOSED(-16)wolfcert/errors.hWOLFCERT_HAVE_BUILTIN_TRANSPORT,WOLFCERT_HAVE_POSIX_STOREwolfcert/options.hThree behavioural changes:
*_session_fd()returns-1on a caller-supplied transport, which has nodescriptor to offer. The built-in path is unaffected.
HAVE_SNIis now required. wolfSSL leaves it off by default on mostcross builds, and without it a hosted EST endpoint serves its default
certificate and fails verification. Define
WOLFCERT_NO_SNIto build withoutit when every endpoint serves a single certificate.
connect_cbis deprecated and adapted onto the vtable internally. Setting ittogether with
transportisWOLFCERT_ERR_BAD_ARG.Together these let wolfCert run on a stack with no BSD sockets, with the POSIX
path as one instance of the vtable rather than a special case.
Tests
tests/unit/test_transport.cdrives the whole HTTP path through a scriptedtransport with no sockets, threads or TLS, so it runs in every build
configuration: handle
0is a valid handle,disconnectruns exactly once, anincomplete vtable is rejected before anything is dialled, the parser survives a
byte-at-a-time feed, a body may end at
CONN_CLOSED, and a transport thatwrongly returns
0is treated as a close instead of spinning theread-until-close loop.
Verification
-Werror. 27 of 27 tests pass on thedefault configuration, and 11 of 11 with 3 skipped when the built-in
transport and POSIX store are compiled out.
no-posix-armgate compiles all 15 portable sources for a Cortex-M4against wolfSSL headers only. Negative controls confirm it fails when a POSIX
include reappears in
src/http.corsrc/csr.c.bisect safe.