Repository navigation
Misc fixes - #50
Merged
Merged
Misc fixes#50
Conversation
test_static_mem_init() set wolfCert's default heap, then every test's wolfcert_init(NULL) reset it to NULL. The no-malloc tests still passed because wolfSSL falls back to its global heap hint, so wolfCert's own heap-hint plumbing was never exercised with a pool. The tests now hand the pool to wolfcert_init through test_heap_hint().
The last-error state used __thread on GCC and Clang unless __STDC_NO_THREADS__ was defined. Apple clang defines it, so the state was global on macOS while the docs said it was per-thread. It now uses wolfSSL's THREAD_LS_T: per-thread when wolfSSL is built with HAVE_THREAD_LS, which its configure enables where the compiler supports it, and one global otherwise. wolfSSL also leaves THREAD_LS_T empty under NO_THREAD_LS and on FreeRTOS and Zephyr, and the docs say so. This drops WOLFCERT_NO_THREAD_LOCAL. On Zephyr the ~300-byte state therefore no longer sits in .tbss, which Zephyr copies onto every thread's stack (qemu_x86 sample: 340 to 44 bytes). The Kconfig no longer implies THREAD_LOCAL_STORAGE, and the README's idle-stack sizing note is gone. test_smoke checks that an error on a second thread does not show up on the first, wherever THREAD_LS_T is thread-local; it failed on macOS before this change. examples/user_settings.h.example notes HAVE_THREAD_LS for hosted targets whose wolfSSL settings share the file.
There was a problem hiding this comment.
🟡 Changes recommended
SCEP pinning remains optional and Basic credentials can still persist in the request buffer after cleanup.
2 open findings
What changed in this PR
This PR applies follow-up correctness, security, testing, API, and documentation improvements identified during PR #49.
Changes:
- Aligns last-error storage with wolfSSL thread-local behavior and reduces Zephyr TLS overhead.
- Exercises static heap hints and strengthens EST parsing, authentication cleanup, and certificate assertions.
- Removes an unused public field and adds SCEP fingerprint support plus documentation updates.
| File | Description |
|---|---|
zephyr/samples/wolfcert_est_client/boards/frdm_mcxn947_mcxn947_cpu0.conf |
Removes obsolete TLS stack allowance. |
zephyr/README.md |
Removes obsolete per-thread TLS sizing guidance. |
zephyr/Kconfig |
Stops implying thread-local storage. |
zephyr/CMakeLists.txt |
Removes the retired TLS override. |
wolfcert/types.h |
Clarifies key parameters and removes unused CSR fields. |
wolfcert/status.h |
Documents wolfSSL-dependent error-state behavior. |
tests/unit/test_transport.c |
Passes the static heap hint. |
tests/unit/test_store.c |
Passes the static heap hint. |
tests/unit/test_smoke.c |
Tests heap initialization and per-thread errors. |
tests/unit/test_server_ca_store.c |
Passes the static heap hint. |
tests/unit/test_scep_msg.c |
Passes the static heap hint. |
tests/unit/test_parse_negative.c |
Passes the static heap hint. |
tests/unit/test_keygen.c |
Passes the static heap hint. |
tests/unit/test_est.c |
Passes the static heap hint. |
tests/unit/test_csr.c |
Passes the static heap hint. |
tests/unit/test_csr_attrs.c |
Passes the static heap hint. |
tests/test_static_mem.h |
Exposes the test pool’s heap hint. |
tests/integration/test_est_tls_roundtrip.c |
Validates issued certificate CNs. |
tests/CMakeLists.txt |
Links the smoke test with thread support. |
src/internal.c |
Uses wolfSSL’s thread-local qualifier. |
src/est/est_server.c |
Hardens chunk bounds and credential cleanup. |
scripts/ci/build-wolfssl.sh |
Propagates unknown-configuration failures. |
README.md |
Documents missing tls-unique binding. |
Makefile.am |
Links the smoke test with pthreads. |
examples/user_settings.h.example |
Documents HAVE_THREAD_LS. |
examples/enroll_scep.c |
Adds optional CA fingerprint verification. |
docs/MIGRATING-FROM-WOLFSCEP.md |
Corrects error-state terminology. |
docs/EMBEDDED.md |
Documents global error-state fallback. |
docs/ARCHITECTURE.md |
Updates error-state architecture guidance. |
CONTRIBUTING.md |
Removes obsolete TLS sizing advice. |
AGENTS.md |
Updates integration guidance for error state. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
check_basic_auth() left the plaintext user:pass in a stack buffer and freed the base64 copy without zeroing it. The request's Authorization value is now zeroed before it is freed, and so is the stack buffer parse_request() reads the header block into.
The decode pass checked ri + csz and body_sz + csz, which can wrap on a 32-bit size_t. chunked_body_complete() already rejects any chunk that does not fit before the decode pass runs, so this was not reachable, but the check should not depend on that. The client decoder in http.c already uses this form.
GetCACert is unauthenticated over plain HTTP, and the example went straight from it to PKCSReq. It now requires the CA's SHA-256 fingerprint as its third argument and checks it with wolfcert_scep_verify_ca_fingerprint before enrolling. The migration guide no longer describes the example as lacking the check.
The assert on the issued certificate's subject was ORed with issued.len > 0 and so could not fail. It now decodes the certificate and compares the subject CN.
wolfcert_csr_build never put it into the CSR, so callers setting it had their attributes dropped without an error. wolfSSL's CSR encoder has no way to pass raw attributes through; the customize callback and wolfcert_csr_attrs_apply cover what it was meant for. This changes the layout of WolfCertCertMeta.
For ECC it is the curve size in bits (256, 384, 521), not a curve id.
RFC 7030 section 3.5 has the client put tls-unique into the CSR and the server check it. wolfCert does neither, and TLS 1.3 does not define tls-unique.
resolve_flags ran in a process substitution, so its exit 2 only ended the subshell and the script went on to build with no configure flags.
Member
Author
|
@wolfSSL-Fenrir-bot review balanced |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #50
Scan targets checked: wolfcert-src, wolfcert-bugs
Coverage: 6 of 14 in-scope changed file(s) opened by the reviewer; not opened: tests/unit/test_csr.c, tests/unit/test_csr_attrs.c, tests/unit/test_keygen.c, tests/unit/test_parse_negative.c, tests/unit/test_scep_msg.c, tests/unit/test_server_ca_store.c, tests/unit/test_store.c, tests/unit/test_transport.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Balanced
yosuke-wolfssl
approved these changes
Oct 13, 2026
yosuke-wolfssl
left a comment
yosuke-wolfssl
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me and any tool didn't find blocking issues.
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups found while trimming the comments in #49.
wolfcert_init, so wolfCert's own heap-hint path is exercised with a pool.THREAD_LS_T: per-thread withHAVE_THREAD_LS, global otherwise. It was silently global on macOS, where Apple clang defines__STDC_NO_THREADS__. On Zephyr it leaves.tbss(340 to 44 bytes on the qemu_x86 sample), so the Kconfig no longer impliesTHREAD_LOCAL_STORAGE.test_smokechecks it across two threads.enroll_scepexample requires the CA's SHA-256 fingerprint and checks it before enrolling.build-wolfssl.shstops on an unknown config instead of building with no flags.WolfCertKeyCfg.paramcomment, and the README notes the missing tls-unique binding (RFC 7030 section 3.5).API changes:
WolfCertCertMeta.csr_attributes_deris removed, which changes the struct layout.wolfcert_csr_buildnever encoded it, so callers lost their attributes silently; use the customize callback orwolfcert_csr_attrs_apply.WOLFCERT_NO_THREAD_LOCALis gone. Builds whose wolfSSL settings come fromuser_settings.hneedHAVE_THREAD_LSfor per-thread error state (examples/user_settings.h.examplenotes it).