Implement CKM_RSA_AES_KEY_WRAP mechanism for key wrap/unwrap #794
Conversation
WalkthroughSupport for the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant HSM as SoftHSM
App->>HSM: C_WrapKey(mech=CKM_RSA_AES_KEY_WRAP, wrappingKey, key)
HSM->>HSM: Validate mechanism params
HSM->>HSM: Generate temporary AES key
HSM->>HSM: Wrap target key with AES key (CKM_AES_KEY_WRAP_PAD)
HSM->>HSM: Wrap AES key with RSA key (CKM_RSA_PKCS_OAEP)
HSM->>App: Return concatenated wrapped blobs
App->>HSM: C_UnwrapKey(mech=CKM_RSA_AES_KEY_WRAP, unwrappingKey, wrappedBlob)
HSM->>HSM: Validate mechanism params
HSM->>HSM: Split wrappedBlob into wrapped AES key and wrapped target key
HSM->>HSM: Unwrap AES key with RSA key (CKM_RSA_PKCS_OAEP)
HSM->>HSM: Unwrap target key with AES key (CKM_AES_KEY_WRAP_PAD)
HSM->>App: Return handle to unwrapped key
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
👮 Files not reviewed due to content moderation or server errors (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/lib/SoftHSM.h (1)
490-493: Consistency note – duplicate parameter-check helpers
MechParamCheckRSAAESKEYWRAPis declared right afterMechParamCheckRSAPKCSOAEP.
Given both perform OAEP-parameter validation, consider refactoring into a single helper that accepts the mechanism type to avoid code duplication inSoftHSM.cpp.src/lib/test/AsymWrapUnwrapTests.h (1)
50-53: Unit-test helper visibility
rsaWrapUnwrapPvtis declaredprotected, yet it appears to be used only inside this test class. Making itprivatetightens the interface and prevents accidental re-use in derived tests.src/lib/test/AsymWrapUnwrapTests.cpp (2)
74-80: Remove the commented out variable declaration.The additions of
CKA_CLASS,CKA_KEY_TYPE,CKA_ENCRYPT, andCKA_VERIFYattributes are correct and follow PKCS#11 standards. However, the commented outbFalsevariable should be removed to keep the code clean.CK_BYTE pubExp[] = {0x01, 0x00, 0x01}; CK_BYTE subject[] = { 0x12, 0x34 }; // dummy CK_BYTE id[] = { 123 } ; // dummy -// CK_BBOOL bFalse = CK_FALSE; CK_BBOOL bTrue = CK_TRUE;
120-123: Consider C++ compatibility for the designated initializer syntax.The C99 designated initializer syntax used here may not be compatible with older C++ standards (pre-C++20). For better portability, consider using traditional initialization.
CK_RSA_AES_KEY_WRAP_PARAMS rsa_aes_params = { - .aes_key_bits = 256, - .oaep_params = &oaepParams + 256, // aes_key_bits + &oaepParams // oaep_params };src/lib/SoftHSM.cpp (2)
7326-7328: Sensitive material not wiped
emphKeyValue(encrypted form of the temporary AES key) remains in memory after use.
For consistency withemphkeydatawipe it as well:- emphkeydata.wipe(); - /* missing */ - emphKeyValue.wipe(); + emphkeydata.wipe(); + emphKeyValue.wipe();[security]
13122-13131: Over-restrictive parameter validationSpec restricts OAEP defaults to SHA-1 by default, but permitting SHA-256/384 is common practice and already supported elsewhere in SoftHSM. Consider relaxing the checks to accept other permitted digests & MGF combos, or surface
CKR_MECHANISM_PARAM_INVALIDonly when the digest is unsupported by the underlying crypto backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/SoftHSM.cpp(9 hunks)src/lib/SoftHSM.h(1 hunks)src/lib/crypto/AsymmetricAlgorithm.cpp(1 hunks)src/lib/crypto/AsymmetricAlgorithm.h(1 hunks)src/lib/pkcs11/pkcs11.h(3 hunks)src/lib/test/AsymWrapUnwrapTests.cpp(6 hunks)src/lib/test/AsymWrapUnwrapTests.h(1 hunks)
🔇 Additional comments (8)
src/lib/crypto/AsymmetricAlgorithm.h (1)
64-72: ```shell
#!/bin/bashLocate the enum definition to confirm ordering
grep -R -n "enum class Type" src/lib/crypto/AsymmetricAlgorithm.h || true
Find all references to AsymMech::Type to see how it's used
grep -R -n "AsymMech::Type" . || true
Identify switch statements on the Type enum
grep -R -n "switch.*Type" . || true
Check for casts of the enum to integer
grep -R -n "static_cast<.*Type" . || true
Look for to_string or serialization helpers for the enum
grep -R -n -E "to_string.*Type|from_string.*Type" . || true
Search for stream operators involving the enum
grep -R -n "operator<<.*Type" . || true
</details> <details> <summary>src/lib/crypto/AsymmetricAlgorithm.cpp (1)</summary> `146-153`: **Good: `isWrappingMech` extended correctly** `RSA_AES_KEY_WRAP` is now recognised as a wrapping mechanism, keeping the wrapper gate in sync with the new feature. Nothing further needed here. </details> <details> <summary>src/lib/pkcs11/pkcs11.h (1)</summary> `982-986`: **Struct definition looks OK – please cross-check macro collision** The field names rely on earlier compatibility macros (`aes_key_bits`, `oaep_params`). Ensure no unintended macro substitution occurs when `CRYPTOKI_GNU` is **not** defined, otherwise the member names will differ between compatibility and GNU modes. If necessary, wrap the struct definition with `#undef` / `#define` to make the intent explicit. </details> <details> <summary>src/lib/test/AsymWrapUnwrapTests.cpp (5)</summary> `47-47`: **LGTM! Test suite is now activated.** Uncommenting the test suite registration enables these important wrap/unwrap tests. --- `96-105`: **LGTM! Private key attributes are properly configured.** The additions of `CKA_CLASS`, `CKA_KEY_TYPE`, `CKA_DECRYPT`, `CKA_SIGN`, and `CKA_EXTRACTABLE` attributes are correct and necessary for RSA private key operations and wrapping functionality. --- `115-190`: **LGTM! Comprehensive test implementation for CKM_RSA_AES_KEY_WRAP.** The test function properly validates the new RSA AES key wrap mechanism by: - Testing buffer size estimation - Verifying proper error handling for insufficient buffers - Wrapping and unwrapping RSA private keys - Validating the unwrapped key works correctly through sign/verify operations --- `231-235`: **LGTM! Proper delegation for asymmetric key wrapping.** The early return and delegation to `rsaWrapUnwrapPvt` is correct since `CKM_RSA_AES_KEY_WRAP` handles asymmetric key wrapping, unlike the other mechanisms that wrap symmetric keys. --- `313-343`: **LGTM! Comprehensive test coverage for all key combinations.** The test now properly validates `CKM_RSA_AES_KEY_WRAP` alongside existing mechanisms for all combinations of: - Session vs Token keys - Public vs Private visibility This ensures the new mechanism works correctly in all deployment scenarios. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
4 participants
This CKM_RSA_AES_KEY_WRAP implementation enables the wrapping and unwrapping of target asymmetric keys of any length and type using an RSA key. A corresponding test case is included to validate the functionality.
Summary by CodeRabbit