Skip to content

Conversation

@antoinelochet
Copy link
Contributor

@antoinelochet antoinelochet commented Dec 10, 2025

There is a slight difference between reference PKCS#11 3.2 functions header and the one from p11kit merged in #827
Definition of CK_FLAGS_PTR was also missing

Summary by CodeRabbit

  • Refactor
    • Updated cryptographic operation API parameters for improved type consistency.
    • Added new type definition aliases for flag parameters.
    • Standardized type declarations in compatibility layer.

Note: These are API-level changes that may require updates for developers using this library.

✏️ Tip: You can customize this high-level summary in your review settings.

@antoinelochet antoinelochet requested a review from a team as a code owner December 10, 2025 15:07
@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

Updates to the PKCS#11 header file include a function signature modification for C_DecapsulateKey to accept ciphertext_len as a value rather than a pointer, and the addition of a public CK_FLAGS_PTR typedef alias for pointer-to-flags type definitions.

Changes

Cohort / File(s) Summary
PKCS#11 Header Modifications
src/lib/pkcs11/pkcs11.h
Modified C_DecapsulateKey function signature to accept ciphertext_len as unsigned long value instead of pointer. Added public CK_FLAGS_PTR typedef as pointer alias for CK_FLAGS.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

  • Verify all callers of C_DecapsulateKey have been updated to pass the value directly rather than a pointer reference
  • Confirm CK_FLAGS_PTR typedef is correctly defined and no duplication exists between sections
  • Check for any compatibility implications or deprecation concerns with the function signature change

Suggested reviewers

  • jschlyter

Poem

🐰 A cryptographic header takes a bow,
Function calls simplified—no pointers now!
New flags alias added with care,
PKCS#11 changes float through the air. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing the PKCS#11 3.2 C_Decapsulate function definition to match the specification.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8940193 and 131de2d.

📒 Files selected for processing (1)
  • src/lib/pkcs11/pkcs11.h (2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.

Applied to files:

  • src/lib/pkcs11/pkcs11.h
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.

Applied to files:

  • src/lib/pkcs11/pkcs11.h
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.

Applied to files:

  • src/lib/pkcs11/pkcs11.h
🔇 Additional comments (2)
src/lib/pkcs11/pkcs11.h (2)

3056-3056: LGTM: Adds missing CK_FLAGS_PTR typedef for completeness.

This typedef was missing from the compatibility layer and is now correctly positioned among the other pointer type definitions (CK_BYTE_PTR, CK_CHAR_PTR, etc.). It provides consistency with the PKCS#11 3.2 specification.


2844-2852: C_DecapsulateKey signature change aligns with PKCS#11 3.2 semantics.

The parameter change from unsigned long *ciphertext_len to unsigned long ciphertext_len is correct: in decapsulation, ciphertext and its length are inputs to the function (specifying what to decapsulate), not outputs. This matches the PKCS#11 3.2 specification semantics.

Verification of implementation consistency requires manual review to confirm that:

  • SoftHSM's C_DecapsulateKey implementation has been updated to match this signature
  • All call sites pass the value instead of a pointer reference

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah looks like this is wrong in p11kit as I just copied it from there. Might be good to also report it to them so next time when we copy new header, we don't have the same issue.

@bukka bukka merged commit 70e0d4c into softhsm:main Dec 11, 2025
9 checks passed
@antoinelochet antoinelochet deleted the bugfix/pkcs11_32 branch December 14, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants