Repository navigation
Make CKM_CONCATENATE_* a build option - #282
Conversation
Every other derive mechanism can be compiled out, but the concatenation mechanisms added for wolfSSL#264 were always built. Add --enable-concatenate and the CMake option WOLFPKCS11_CONCATENATE, both on by default, which define WOLFPKCS11_CONCATENATE. A locked-down device with no use for hybrid key agreement can now drop the mechanisms, their helpers and their mechanism list entries. Restore WP11_DERIVE_SECRET_KEY, which the concatenation change removed, and add WOLFPKCS11_CONCATENATE to it. It keeps one list of the mechanisms that build a secret key, so the guards on the derive locals and helpers cannot drift apart again. WP11_MAX_SYM_KEY_SZ sizes the WP11_Data buffer allocated for every secret key. The 256-byte value for no-DH builds applied whenever ML-KEM was on, so an ML-KEM + ECC build grew each secret key by about 190 bytes whether or not it used concatenation. Tie it to concatenation instead, which also covers ECC-only builds: PSK || Z_ECDH on P-384 is 80 bytes. Wrap the definition in #ifndef so a build can set its own size, and fail the build when the size cannot hold an AES-256 key. test_hkdf_derive_aes_no_value_len now creates its base key with get_generic_key, so the concatenation test helpers can sit behind the new guard. test_concatenate_disabled checks that a build without the option neither advertises nor accepts the mechanisms.
Add a no_concatenate job, and a no_derive_secret_key job that builds with ECC, DH, HKDF, AES-CBC and concatenation disabled, so the code outside WP11_DERIVE_SECRET_KEY gets built and tested. No job built that combination before, which is how the mismatched guards behind F-10381 went unnoticed. The guard also follows WOLFSSL_HAVE_PRF, which wolfSSL defines whenever it has HMAC and KDF, so every job built against the shared wolfSSL had the guard on whatever the wolfPKCS11 options were. The new job builds a crypto-only wolfSSL with NO_KDF; the TLS layer and wolfSSL's own tests do not build without KDF. build-workflow.yml gains wolfssl_config and wolfssl_cflags inputs for it. The configuration failed to build or pass because of test guards that assumed ECC, DH, AES, AES-CBC, HKDF or PRF: pkcs11mtt.c only declared ckFalse with ECC or DH although test_attribute always uses it, pkcs11str.c referenced the AES-CBC vectors without AES-CBC and left pub unused, test_pkcs5_pbkdf2_key_gen was built without PRF but only registered with it and expected CKK_AES and CKK_HKDF keys without AES or HKDF, and decrypt_final_bufsize_test and the failed-encode case of object_store_decode_test ran AES-CBC unconditionally. Guard them.
|
@claude review |
|
@wolfSSL-Fenrir-bot review balanced |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are identified for the change making concatenation support optional at build time.
No open issues 🎉
What changed in this PR
Makes the concatenation mechanisms introduced in #279 optional at build time, while keeping them enabled by default. This lets smaller wolfPKCS11 builds omit concatenation and its larger default secret-key buffer.
Changes:
- Add concatenation controls for Autotools, CMake, and Visual Studio, and guard mechanism registration and implementation.
- Restore conditional secret-key derivation support and allow overriding the symmetric-key buffer size.
- Adjust tests for disabled algorithms and add CI configurations without concatenation or secret-key derivation.
| File | Description |
|---|---|
wolfpkcs11/internal.h |
Make buffer sizing configurable and guard concatenation declarations. |
tests/pkcs11v3test.c |
Guard hybrid-agreement tests on concatenation support. |
tests/pkcs11test.c |
Test disabled concatenation and correct feature guards. |
tests/pkcs11str.c |
Guard concatenation and AES-CBC test code. |
tests/pkcs11mtt.c |
Make the shared boolean available without ECC or DH. |
tests/object_store_decode_test.c |
Guard AES-CBC-dependent storage tests. |
tests/decrypt_final_bufsize_test.c |
Skip the CBC-PAD test when AES-CBC is unavailable. |
src/slot.c |
Conditionally advertise concatenation mechanisms. |
src/internal.c |
Conditionally compile concatenation helpers. |
src/crypto.c |
Restore derivation guards and conditionally compile concatenation. |
README.md |
Document the CMake concatenation option. |
IDE/VisualStudio/user_settings.h |
Enable concatenation by default for Visual Studio. |
configure.ac |
Add the Autotools concatenation option and status output. |
CMakeLists.txt |
Add the default-enabled CMake concatenation option. |
cmake/options.h.in |
Export the concatenation setting in the generated header. |
.github/workflows/unit-test.yml |
Add builds without concatenation or secret-key derivation. |
.github/workflows/build-workflow.yml |
Accept custom wolfSSL configuration and compiler flags. |
Review effort: Balanced
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #282
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Not reviewed (no scan target covers them): .github/workflows/build-workflow.yml, .github/workflows/unit-test.yml, CMakeLists.txt, IDE/VisualStudio/user_settings.h, README.md, cmake/options.h.in, configure.ac
Coverage: 4 of 10 in-scope changed file(s) opened by the reviewer; not opened: src/slot.c, tests/decrypt_final_bufsize_test.c, tests/object_store_decode_test.c, tests/pkcs11mtt.c, tests/pkcs11v3test.c, wolfpkcs11/internal.h
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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes the preprocessor guards around the key-derivation path in wp11_C_DeriveKey and the key-buffer sizing in a crypto library, so a human look is still worthwhile.
What was reviewed:
- The WP11_DERIVE_SECRET_KEY guard in src/crypto.c: the locals, SetDerivedStates, SymmKeyLen and the shared derived-key object creation and zeroing are covered together, so no derive mechanism is left without them.
- WP11_MAX_SYM_KEY_SZ in wolfpkcs11/internal.h: the default build still gets 256 because concatenation is on by default, the override keeps the existing bounds checks in internal.c and slot.c, and the new #error is a correct lower bound.
- The option is wired consistently across CMakeLists.txt, configure.ac, options.h.in, README and the Visual Studio user_settings.h, and the slot.c mechanism list/info entries use the same guard.
Extended reasoning...
The change makes the CKM_CONCATENATE_* derive mechanisms a build option. It adds #ifdef guards in src/crypto.c, src/internal.c, src/slot.c and wolfpkcs11/internal.h, plus build-system wiring, CI jobs and test-guard updates across 17 files. It touches the PKCS#11 key-derivation path, which is security-sensitive code, and it changes how WP11_MAX_SYM_KEY_SZ is chosen (the size of the stored secret-key buffer) for non-default configurations. Default builds compile the same code as before. Because this is crypto-library code and the guard correctness depends on many build-configuration combinations, I deferred rather than approved.
Follow-up to #279 from review: concatenation can now be compiled out, and the larger secret-key buffer only applies when it is built in.
Changes
--enable-concatenate/ CMakeWOLFPKCS11_CONCATENATE(default on) for the threeCKM_CONCATENATE_*mechanisms; the Visual Studiouser_settings.hdefines it too.WP11_DERIVE_SECRET_KEYwithWOLFPKCS11_CONCATENATEadded to it.WP11_MAX_SYM_KEY_SZis 256 in no-DH builds only with concatenation (was: with ML-KEM), and can be overridden.Testing
test_concatenate_disabledchecks the mechanisms are neither listed nor accepted when compiled out.no_concatenatejob.no_derive_secret_keyjob with ECC, DH, HKDF, AES-CBC and concatenation off, against a crypto-only wolfSSL built withNO_KDFsoWOLFSSL_HAVE_PRFis off too.build-workflow.ymlgainswolfssl_configandwolfssl_cflagsinputs.