Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and integration blockers (FFI CK_INTERFACE layout risk, missing nav.yml sync, incorrect mechanisms/logging docs, and an FFI null-out-pointer crash in C_GetFunctionList).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the PKCS#11 surface to cover Cryptoki v3.x “interfaces” discovery (provider side) and adds an additive v3 capability probe for HSM consumers, alongside documentation, tests, and changelog entries to support the rollout.
Changes:
- Add
C_GetInterfaceList/C_GetInterface/C_LoginUser, a v3.0 function list table, and non-null v3-only stubs in the PKCS#11 provider/module. - Add consumer-side v3 interface capability probing (
supports_pkcs11_v3_interfaces/list_pkcs11_v3_interfaces) and a SoftHSM2 additive probe test. - Add docs + ADR + changelog updates describing PKCS#11 v3 scope and behavior (profiles, logging, mechanisms).
File summaries
| File | Description |
|---|---|
README.md |
Link “PKCS#11 integrations” to the new provider-module documentation page. |
lychee.toml |
Exclude eviden.com from link checking due to crawler 503 responses. |
documentation/docs/SUMMARY.md |
Add the new PKCS#11 provider doc page to mdBook navigation (but nav.yml is not updated). |
documentation/docs/integrations/pkcs11_provider.md |
New provider reference page (mechanisms + logging sections need correction). |
documentation/docs/hsm_support/hsm_operations.md |
Document consumer-side v2.40 compatibility + additive v3 capability probe; link to ADR. |
documentation/docs/adr/2026-09-03-pkcs11-v3-scope-decision-ffi-foundation.md |
New ADR; scope text contradicts actual provider-side changes in this PR. |
crate/hsm/softhsm2/src/tests.rs |
Add an ignored integration test asserting v3 probe is additive for SoftHSM2 (v2.40). |
crate/hsm/base_hsm/src/pkcs11_v3.rs |
New consumer-side v3 interface parsing/types (FFI layout risk called out in comments). |
crate/hsm/base_hsm/src/lib.rs |
Export InterfaceDescriptor and register the new pkcs11_v3 module. |
crate/hsm/base_hsm/src/hsm_lib.rs |
Resolve C_GetInterfaceList best-effort and implement interface listing with allocation cap. |
crate/crypto/src/openssl/ocsp.rs |
Minor import reordering (warn). |
crate/clients/pkcs11/provider/src/tests.rs |
Add provider-side tests for v3 interface discovery, C_LoginUser, and profile objects. |
crate/clients/pkcs11/provider/src/lib.rs |
Share backend init across v2/v3 entrypoints; implement C_GetInterfaceList/C_GetInterface. |
crate/clients/pkcs11/module/src/sessions.rs |
Add supported profile IDs and return CKO_PROFILE objects in find-objects flow. |
crate/clients/pkcs11/module/src/pkcs11.rs |
Add v3.0 function list table + CK_INTERFACE descriptor + v3-only non-null stubs + C_LoginUser. |
crate/clients/pkcs11/module/src/core/object.rs |
Make CKO_PROFILE objects public (CKA_PRIVATE = CK_FALSE). |
CHANGELOG/pkcs11_v3.md |
Branch changelog capturing provider+consumer PKCS#11 v3 work, docs, and tests. |
Review details
Suppressed comments (4)
crate/hsm/base_hsm/src/hsm_lib.rs:323
- Same as above:
#[expect(unsafe_code)]is likely to produceunfulfilled_lint_expectationswarnings (unlessunsafe_codeis enabled at warn/deny) and is a new lint attribute on freshly added code. The// SAFETY:comment is sufficient.
let mut buffer = vec![CkInterface::default(); count_usize];
#[expect(unsafe_code)]
// SAFETY: `buffer` was allocated using the exact count returned by the first
// call, as required by the PKCS#11 v3.0 specification for `C_GetInterfaceList`.
let rv = unsafe { get_interface_list(buffer.as_mut_ptr(), &raw mut count) };
crate/clients/pkcs11/provider/src/tests.rs:694
- Same issue here:
#[expect(unsafe_code)]can createunfulfilled_lint_expectationswarnings unless the crate enablesunsafe_codeat warn/deny. Prefer relying on the existing// SAFETY:comments around theunsafecalls instead of the lint attribute.
#[test]
#[serial]
#[expect(unsafe_code)]
fn test_get_interface_rejects_mismatches() -> Pkcs11Result<()> {
let _backend = initialize_backend()?;
crate/clients/pkcs11/provider/src/tests.rs:759
- Same here:
#[expect(unsafe_code)]is likely to causeunfulfilled_lint_expectationswarnings unlessunsafe_codeis enabled. Removing it should keep the test intent intact (the unsafe blocks are already locally documented).
#[test]
#[serial]
#[expect(unsafe_code)]
fn test_c_login_user() -> Pkcs11Result<()> {
let _backend = initialize_backend()?;
crate/clients/pkcs11/provider/src/tests.rs:815
- Same issue:
#[expect(unsafe_code)]may triggerunfulfilled_lint_expectationswarnings unlessunsafe_codeis enabled at warn/deny. Prefer removing the lint attribute and keeping the existing// SAFETY:documentation for each unsafe call.
#[test]
#[serial]
#[expect(unsafe_code)]
fn test_profile_objects_self_declared() -> Pkcs11Result<()> {
let _backend = initialize_backend()?;
- Files reviewed: 17/17 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b8c71b5 to
f7316a5
Compare
Adds a minimal, hand-written, additive FFI surface (CK_INTERFACE, C_GetInterfaceList) to HsmLib so the KMS can detect PKCS#11 v3.0 interface support without changing any existing v2.40 function resolution. Fully backward-compatible: symbol resolution is best-effort and never fails HsmLib::instantiate. - New crate/hsm/base_hsm/src/pkcs11_v3.rs: CkInterface, CkCGetInterfaceList, InterfaceDescriptor, and a pure parse_interfaces() with unit tests. - HsmLib gains supports_pkcs11_v3_interfaces() and list_pkcs11_v3_interfaces(), following the existing two-call PKCS#11 convention (count then fill), with a defense-in-depth plausibility cap (MAX_PLAUSIBLE_PKCS11_V3_INTERFACES) on the allocation. - Ignored SoftHSM2 integration test asserting the probe is additive (v2.40 library reports no v3.0 support, existing behavior unaffected). - ADR documenting the hand-write-vs-fork-pkcs11-sys scope decision. - mdBook doc section on PKCS#11 protocol version compatibility. - Branch changelog entry. Note: crate/crypto/src/openssl/ocsp.rs import-order is a pre-existing nightly/stable rustfmt drift from a prior merge, auto-fixed here only because the pre-commit nightly-cargo-format hook enforces it workspace-wide. Closes #1153
…rofile self-declaration Fills the gaps identified in the review comment on #1153 (#1153 (comment)): zero v3.0 functions were previously implemented on this branch, C_LoginUser was missing entirely, and the pre-existing CKO_PROFILE mechanism was dead code never wired up. - Implement C_GetInterfaceList/C_GetInterface (additive; C_GetFunctionList keeps working unchanged for v2.40-only consumers). - Implement C_LoginUser (delegates to the same logic as C_Login). - Add the 21 v3.0-only stub entry points required for a non-null CK_FUNCTION_LIST_3_0 (C_SessionCancel + message-based crypto family), all returning CKR_FUNCTION_NOT_SUPPORTED. - Wire up CKO_PROFILE object emission in load_find_context_by_class, self-declaring CKP_BASELINE_PROVIDER, CKP_EXTENDED_PROVIDER, CKP_AUTHENTICATION_TOKEN, CKP_PUBLIC_CERTIFICATES_TOKEN. - Fix CKA_PRIVATE on CKO_PROFILE objects (CK_TRUE -> CK_FALSE): profile objects must be discoverable pre-login per the OASIS spec. - Add 4 integration tests; add mdbook doc (documentation/docs/integrations/pkcs11_provider.md); add changelog entry. - lychee.toml: exclude eviden.com (returns 503 to automated crawlers, unrelated pre-existing broken link found while committing docs). No breaking change: all additions are new, optional entry points and objects; existing v2.40 behavior is unchanged. Closes #1153
- Reject (rather than mis-route to the profile-only fast path) a template that combines CKA_PROFILE_ID with an explicit, different CKA_CLASS (e.g. CKO_PRIVATE_KEY): no object satisfies both constraints, so it must yield no matches instead of returning an unrelated profile object. - Omit CKP_PUBLIC_CERTIFICATES_TOKEN from the self-declared profile list while OIDC-pin-as-access-token mode is active: C_Logout clears the registered backend in that mode, so certificate discovery stops working post-logout and the "discoverable without login" guarantee no longer holds.
parse_utf8_argument previously allocated and copied the caller-supplied ulPinLen/ulUsernameLen bytes unconditionally. A malicious or misbehaving caller could pass an extremely large length and exhaust process memory before UTF-8 validation ever runs. Reject lengths above a generous 4096-byte cap before allocating.
C_GetInterface previously required an exact {major, minor} match against
the implemented interface version (3.1), so a conformant v3.0 consumer
explicitly requesting {major: 3, minor: 0} was rejected with
CKR_ARGUMENTS_BAD even though a v3.1 implementation is a strict superset
of v3.0. Accept any requested minor version up to the implemented one;
only a minor version above it (e.g. 3.2) is now rejected as unsupported.
Updates the existing rejects_mismatches test accordingly and adds
coverage for the now-accepted compatible-minor-version case.
…, logging - Remove CKM_DSA from the supported-mechanisms table (never implemented; C_GetMechanismInfo rejects it with CKR_MECHANISM_INVALID) and add the actually-supported RSA PKCS#1 v1.5/PSS and EdDSA mechanisms that were missing from the table. - Update the interfaces-discovery section to reflect that C_GetInterface now accepts a compatible v3.0 version request, not only the exact 3.1 version it implements. - Clarify that COSMIAN_PKCS11_LOGGING_FOLDER (Linux only) overrides the default .cosmian log directory.
…d call sites Commit a1fc425 renamed the C_GetFunctionList error messages to ensure_backend_registered (the check is now shared with C_GetInterface's backend-registration path), but log-reference.md still listed the old message text. Remove the 3 stale [REMOVED] rows and document the 3 renamed entries with their actual Variables/Notes content.
…ool interop df24e06 added user-type validation to C_Login/C_LoginUser that rejected every user type except CKU_USER with CKR_USER_TYPE_INVALID, breaking 'pkcs11-tool --login-type so' (used by the LUKS CI test and by any real-world SO-role client). This module exposes a single implicit backend identity per slot with no separate Security Officer role, so CKU_SO is now treated identically to CKU_USER instead of being rejected, matching the pre-v3.0 behavior (which accepted any user type). Updates test_c_login_user's CKU_SO assertion accordingly.
…discovery Interface discovery (C_GetInterfaceList/C_GetInterface) calls register_backend_if_absent via ensure_backend_registered(PreserveExisting), which could not distinguish an explicit C_Logout (BACKEND == None) from the initial pre-login state (also BACKEND == None). This let a client call C_Logout in OIDC-pin mode, then trigger interface discovery, silently resurrecting the pre-login backend from the static ckms.toml configuration and undoing the logout. Add a LOGGED_OUT sentinel: clear_backend() sets it, register_backend() (an explicit re-registration, e.g. a fresh login) clears it, and register_backend_if_absent() now no-ops while it is set.
Session::destroy_object unconditionally forwarded revoke_object/destroy_object
calls to the backend for any object type, including Object::Profile — a
synthetic, module-local object with no real KMS-backed counterpart.
Object::Profile's remote_id() ("pkcs11-profile:<id>") is only a local
namespacing convention, not a cryptographically-guaranteed-unique identifier;
if a real KMS object's unique identifier happened to collide with it,
destroying the profile handle would send real revoke/destroy KMIP requests
against that unrelated object.
Reject destruction of Object::Profile objects before ever calling the
backend, returning the new ModuleError::ActionProhibited (CKR_ACTION_PROHIBITED).
register_backend()/clear_backend() previously mutated the LOGGED_OUT flag after releasing the BACKEND write lock, and register_backend_if_absent() checked LOGGED_OUT before acquiring it. A concurrent call could interleave between the two operations and leave the pair inconsistent (e.g. BACKEND holding a fresh backend while LOGGED_OUT is still true, or the reverse), letting interface discovery resurrect a backend that C_Logout had just revoked. Mutate/check LOGGED_OUT while still holding the BACKEND write guard in all three functions so the two pieces of state change atomically together.
cargo test kryoptic_conformance (name-substring filter) matched 0 tests because the sole #[test] was named test_kryoptic_pkcs11_v3_conformance_suite. --test kryoptic_conformance (target name) always worked, but the mismatch made the test look like it didn't exist. Renamed the fn to kryoptic_conformance.
PKCS#11 v3.0 interface discovery foundation:
C_GetInterfaceList/C_GetInterface,C_LoginUser, and self-declared conformance profiles — additive only, existing v2.40 consumers unaffected.C_GetInterfaceList/C_GetInterface) with a v3 function-pointer table populated once viaOnce; keeps legacyC_GetFunctionList(v2.40) untouched.C_LoginUser, syntheticCKO_PROFILEobjects (baseline / extended-provider / auth-token / public-certs), andCKA_UNIQUE_IDattribute support.CKU_CONTEXT_SPECIFIC/unknown user types; fixesC_GetAttributeValuebuffer-too-small handling (security advisory COSMIAN-2026-022).C_GetInterfaceListcapability probe (capped at 4096 interfaces).Closes #1153