cmake: add more build options in line with automake#10834
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10834
No scan targets match the changed files in this PR. Review skipped.
|
Jenkins: retest this please |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 4 total — 3 posted, 1 skipped
Posted findings
- [Medium] App-bundle sub-option forcing runs before options are defined, degrading cache entries —
CMakeLists.txt:250-534 - [Medium] Inconsistent WOLFSSL_LINUX_KM gating silently no-ops some kernel options —
CMakeLists.txt:2940-2960 - [Low] Repeated empty placeholder comment blocks in options.h.in —
cmake/options.h.in:811-817
Skipped findings
- [Medium] New build options ship with no test/CI coverage
Review generated by Skoll via Claude/Codex
Updates - ~100 individual options added (ciphers, TLS features, debug/test, caches, kernel-module, key-size numerics, asm) + fixed WOLFSSL_32BIT's missing define. - 37 app-integration bundles (openssh, nginx, haproxy, openvpn, wpas, apachehttpd, jni, wolfclu, wolfsentry, ...) - each force-enables its sub-options and emits its defines; all build clean and match ./configure --enable-X exactly (miss=0/extra=0). - Each option wired in 3 places: add_option + define in CMakeLists.txt, #cmakedefine in options.h.in, source selection where needed. Verified by real libwolfssl.so builds.
Frauschi
left a comment
There was a problem hiding this comment.
Skoll review (follow-up). Thanks for the earlier fixes — I confirmed all three of my previous comments are addressed (force_option via GLOBAL property preserves cache type/help; LINUXKM_PIE/BENCHMARKS now warn when WOLFSSL_LINUX_KM is off; the empty options.h.in parity headers are gone).
One blocking issue remains, plus a few suggestions/nits below.
| ) | ||
| endif() | ||
|
|
||
| add_option("WOLFSSL_CAMELLIA" |
There was a problem hiding this comment.
🔴 [High / BLOCK] Late crypto options never get their source files compiled (bundle path)
generate_build_flags() (line 2847) reads WOLFSSL_CAMELLIA/MD2/RIPEMD/SRP/LIBZ/BLAKE2*/RC2/CRL_MONITOR to decide which .c files enter LIB_SOURCES (see cmake/functions.cmake:107,883 etc.), but these options are only add_option()'d here at 2998+ — after 2847.
An explicit -DWOLFSSL_CAMELLIA=yes on the command line still works (the cache var exists before configure runs). But force_option() records the force as a GLOBAL property that is applied at add_option() time (after 2847), so any bundle that forces one of these late options is broken: OPENSSH→RIPEMD, BUMP→MD2, LIBEST→SRP, JNI→CRL_MONITOR emit the feature -D define while the implementation .c is never added to the build → missing symbols in the .so (LIBZ is worse: zlib is found+linked but compress.c is absent).
Recommendation: Move every add_option() whose variable is consumed by generate_build_flags()/generate_lib_src_list to before line 2847, then verify with -DWOLFSSL_OPENSSH=yes that ripemd.c (and camellia.c/srp.c/compress.c for their bundles) actually appear in LIB_SOURCES.
| foreach(_o WOLFSSL_MD5 WOLFSSL_OCSP WOLFSSL_OCSPSTAPLING WOLFSSL_OCSPSTAPLING_V2 WOLFSSL_OLD_TLS) | ||
| force_option(${_o} "yes") | ||
| endforeach() | ||
| list(APPEND WOLFSSL_DEFINITIONS "-DFP_MAX_BITS=16384" "-DHAVE_ALPN" "-DHAVE_ANON" "-DHAVE_CERTIFICATE_STATUS_REQUEST" "-DHAVE_CERTIFICATE_STATUS_REQUEST_V2" "-DHAVE_COMP_KEY" "-DHAVE_CRL" "-DHAVE_EXT_CACHE" "-DHAVE_EX_DATA" "-DHAVE_MAX_FRAGMENT" "-DHAVE_OCSP" "-DHAVE_OPENSSL_CMD" "-DHAVE_SECURE_RENEGOTIATION" "-DHAVE_SESSION_TICKET" "-DHAVE_TRUNCATED_HMAC" "-DHAVE_TRUSTED_CA" "-DKEEP_OUR_CERT" "-DKEEP_PEER_CERT" "-DNO_SESSION_CACHE_REF" "-DOPENSSL_ALL" "-DOPENSSL_COMPATIBLE_DEFAULTS" "-DOPENSSL_EXTRA" "-DRSA_MAX_SIZE=8192" "-DSESSION_CERTS" "-DSP_INT_BITS=8192" "-DWC_RSA_NO_PADDING" "-DWOLFSSL_ALLOW_SSLV3" "-DWOLFSSL_ALT_CERT_CHAINS" "-DWOLFSSL_ALWAYS_KEEP_SNI" "-DWOLFSSL_ALWAYS_VERIFY_CB" "-DWOLFSSL_CERT_GEN" "-DWOLFSSL_CERT_NAME_ALL" "-DWOLFSSL_CERT_REQ" "-DWOLFSSL_CHECK_ALERT_ON_ERR" "-DWOLFSSL_EITHER_SIDE" "-DWOLFSSL_ERROR_CODE_OPENSSL" "-DWOLFSSL_HAPROXY" "-DWOLFSSL_KEEP_RNG_SEED_FD_OPEN" "-DWOLFSSL_KEY_GEN" "-DWOLFSSL_PRIORITIZE_PSK" "-DWOLFSSL_SIGNER_DER_CERT" "-DWOLFSSL_TICKET_HAVE_ID" "-DWOLFSSL_TLS13_NO_PEEK_HANDSHAKE_DONE" "-DWOLFSSL_TRUST_PEER_CERT") |
There was a problem hiding this comment.
🟡 [Medium] Conflicting numeric -D macros from bundle + max-key-size options
HAProxy hard-codes -DFP_MAX_BITS=16384 -DSP_INT_BITS=8192 -DRSA_MAX_SIZE=8192; BUMP uses different values (8192/4096/4096); the new WOLFSSL_MAX_RSA_BITS/WOLFSSL_MAX_ECC_BITS block also appends -DFP_MAX_BITS=/-DSP_INT_BITS=/-DRSA_MAX_SIZE=. If a bundle and the max-key options are both active (or two bundles combine), WOLFSSL_DEFINITIONS ends up with two -DFP_MAX_BITS= entries carrying different values → -Wmacro-redefined, an order-dependent effective value, and the options.h-populating loop can silently disagree with the compile line.
Recommendation: Resolve the numeric size macros into single scalar variables and append each -D once.
| # zlib compression support | ||
| add_option("WOLFSSL_LIBZ" "Enable zlib compression support (default: disabled)" "no" "yes;no") | ||
| if(WOLFSSL_LIBZ) | ||
| find_package(ZLIB REQUIRED) |
There was a problem hiding this comment.
🔵 [Low] Parity options after the WOLFSSL_USER_SETTINGS reset leak defines into user_settings builds
Line 2855 does set(WOLFSSL_DEFINITIONS "-DWOLFSSL_USER_SETTINGS") to intentionally discard all cmake-option-derived defines. The application bundles run before the reset, so they're correctly wiped. But the new 'Additional configuration options' block (2909+) runs after it, so WOLFSSL_CAMELLIA/RC2/LIBZ still append their -D flags and side effects even in a user_settings build — and WOLFSSL_LIBZ here even runs find_package(ZLIB REQUIRED) + links zlib. Inconsistent with the bundle handling.
Recommendation: Consolidate all option processing before the USER_SETTINGS reset.
| elseif(WOLFSSL_HARDEN_TLS STREQUAL "128") | ||
| list(APPEND WOLFSSL_DEFINITIONS "-DWOLFSSL_HARDEN_TLS=128") | ||
| else() | ||
| message(FATAL_ERROR "Invalid value for WOLFSSL_HARDEN_TLS -- must be yes, no, 112, or 128.") |
There was a problem hiding this comment.
🔧 [Nit] Unreachable FATAL_ERROR in WOLFSSL_HARDEN_TLS value handling
add_option("WOLFSSL_HARDEN_TLS" ... "no" "no;yes;112;128") coerces any value not in the STRINGS list down to yes/no (functions.cmake:43-60) before this block runs, so an out-of-range value like 999 is already yes and this else() message(FATAL_ERROR ...) can never fire — dead/misleading validation.
Recommendation: Use a raw cache STRING for value-bearing options that need their own range validation (mirroring WOLFSSL_CONTEXT_EXTRA_USER_DATA).
| endif() | ||
|
|
||
| # RIPEMD | ||
| add_option("WOLFSSL_RIPEMD" "Enable RIPEMD-160 (default: disabled)" "no" "yes;no") |
There was a problem hiding this comment.
🔧 [Nit] Redundant duplicate -D defines between bundles and standalone options
The OPENSSH bundle appends -DWOLFSSL_RIPEMD and force_option()s WOLFSSL_RIPEMD, whose add_option block here appends -DWOLFSSL_RIPEMD again → duplicate identical -D on the compile line. Harmless (same value) but noisy; several bundles have similar overlaps (e.g. MD2 in BUMP).
Recommendation: Drop the hard-coded defines from bundles when the forced option already emits them.
Description
Updates
Testing
None