PKCS#11 API - #663
Draft
Jens Topp (jenstopp) wants to merge 6 commits into
Draft
Conversation
The PKCS#11 module PRs (#594 and follow-ups) collect on feature/pkcs11 until the licensing question on the vendored OASIS headers is settled, then the branch merges to main as a whole. rust, codeql, coverage, and fw_uno filter pull_request/push by base branch, so PRs targeting feature/pkcs11 would get no CI without this; provider-matrix and engine-matrix already run on PRs to any base. GitHub evaluates these filters against the merge ref, so having the filter on the base branch itself is sufficient - main stays untouched. Drop this commit when the branch merges to main.
The copyright precheck requires the Microsoft MIT header on every tracked source file, and --fix stamps it in. Files vendored from third parties must keep their upstream header instead: overwriting it would misattribute the original authors. Add a third-party directory list, honored by both the check and --fix, seeded with plugins/pkcs11/include/pkcs11-v3.1 (the OASIS PKCS#11 v3.1 headers pkcs11.h/pkcs11t.h/pkcs11f.h, which carry the OASIS Open copyright and land via the PKCS#11 module PRs on this branch).
The template used \r separators, so generated headers carried the whole notice as one squashed comment line with stray CR bytes, and it omitted the trailing period in "Licensed under the MIT License." - the exact string `cargo xtask precheck --copyright` matches. Any committed cbindgen output therefore failed the copyright gate. Use the standard two-line // header the rest of the repo carries; regenerated headers now start with exactly the lines the gate expects.
Contributor
There was a problem hiding this comment.
Pull request overview
Sync/staging maintenance for the feature/pkcs11 branch to keep full CI running while PKCS#11 work accumulates and the licensing decision for vendored OASIS PKCS#11 v3.1 headers remains open. The core functional change is teaching the xtask copyright gate to exempt a specific vendored third-party directory so upstream notices are preserved.
Changes:
- Exempt
plugins/pkcs11/include/pkcs11-v3.1from the repo copyright header check/fix, with a focused unit test for the exemption matcher. - Extend several GitHub Actions workflows to trigger on
feature/pkcs11for push and pull_request events.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
xtask/src/copyright.rs |
Skips copyright check/fix for a vendored third-party directory and adds a unit test for path matching. |
.github/workflows/rust.yml |
Adds feature/pkcs11 to workflow triggers for push/PR. |
.github/workflows/fw_uno.yml |
Adds feature/pkcs11 to workflow triggers for push/PR. |
.github/workflows/coverage.yml |
Adds feature/pkcs11 to workflow triggers for push/PR. |
.github/workflows/codeql.yml |
Adds feature/pkcs11 to workflow triggers for push/PR. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
First implementation slice of a standalone PKCS#11 (Cryptoki) v3.1 module for
the AZIHSM, in C, independent of the OpenSSL provider. Links
azihsm_api_native and generates azihsm.h via cbindgen, mirroring plugins/ossl_prov.
Implemented:
- Library init, slot/token/mechanism enumeration (partitions -> slots), sessions
and the login state machine, and the C_GetInterface "PKCS 11" interface.
- C_Login: opens a session and provisions the partition lazily (only when the
device reports it is not yet provisioned; provisioning is one-shot per power
cycle, a session is the repeatable per-login primitive). Login is token-wide:
the AZIHSM session and login state live on the token (slot), shared across its
sessions, and are released by C_Logout, the last C_CloseSession on the slot, or
C_Finalize regardless of which session makes the call. The PKCS#11 PIN is
validated against the advertised range (4-16) and mapped to the fixed 16-byte
AZIHSM credential. CKU_SO and CKU_CONTEXT_SPECIFIC are rejected: the DDI has no
security-officer credential and there is no per-operation re-auth yet.
- Host object store behind a vtable seam (p11_objstore) so an in-memory backend
now can be replaced by a persistent one without touching callers. Because the
device authenticates the partition, not individual objects, the store enforces
PKCS#11 access control host-side: every object is bound to its slot/token,
handle operations reject cross-token access and private objects while logged
out (CKR_OBJECT_HANDLE_INVALID), and creating a private object requires a
logged-in session.
- C_Digest (SHA-256), host-side, as the one demonstrable crypto operation.
Everything else returns CKR_FUNCTION_NOT_SUPPORTED (generated stubs). The
azihsm_status -> CK_RV mapping lives in one place (p11_status.c), called only in
the HSM-binding layer.
Verified on the mock DDI: loads in pkcs11-tool as Cryptoki 3.1, slot 0 =
/dev/mcr-hsm-mock, C_Login runs the ceremony, SHA-256('abc') matches OpenSSL.
CI (.github/workflows/pkcs11.yml) is manual (workflow_dispatch) for now.
Signed-off-by: Jens Topp <jens.topp@9elements.com>
Comment on lines
+257
to
+262
| if (tmpl[i].ulValueLen < a->len) | ||
| { | ||
| tmpl[i].ulValueLen = CK_UNAVAILABLE_INFORMATION; | ||
| rv = CKR_BUFFER_TOO_SMALL; | ||
| continue; | ||
| } |
Comment on lines
+153
to
+158
| memset(o, 0, sizeof(*o)); | ||
| o->attrs = (mem_attr *)calloc(count ? count : 1, sizeof(mem_attr)); | ||
| if (o->attrs == NULL) | ||
| { | ||
| return CKR_HOST_MEMORY; | ||
| } |
Comment on lines
+108
to
+127
| /* Release an object, zeroing attribute values and the key body first — they | ||
| * may hold secrets. */ | ||
| static void free_object(mem_object *o) | ||
| { | ||
| for (CK_ULONG i = 0; i < o->attr_count; i++) | ||
| { | ||
| if (o->attrs[i].value != NULL) | ||
| { | ||
| memset(o->attrs[i].value, 0, o->attrs[i].len); | ||
| free(o->attrs[i].value); | ||
| } | ||
| } | ||
| free(o->attrs); | ||
| if (o->key_body != NULL) | ||
| { | ||
| memset(o->key_body, 0, o->key_body_len); | ||
| free(o->key_body); | ||
| } | ||
| memset(o, 0, sizeof(*o)); | ||
| } |
Comment on lines
+367
to
+382
| if (st != AZIHSM_OK && st != AZIHSM_PARTITION_ALREADY_PROVISIONED && | ||
| st != AZIHSM_VAULT_APP_LIMIT_REACHED) | ||
| { | ||
| AZIHSM_PKCS11_LOG("provision failed: %d", (int)st); | ||
| return azihsm_pkcs11_ckr_from_azihsm((int)st); | ||
| } | ||
| st = azihsm_sess_open(sl->hsm_dev, &creds, NULL, &sess); | ||
| } | ||
| if (st != AZIHSM_OK) | ||
| { | ||
| AZIHSM_PKCS11_LOG("sess_open failed: %d", (int)st); | ||
| return azihsm_pkcs11_ckr_from_azihsm((int)st); | ||
| } | ||
| *out_session = sess; | ||
| AZIHSM_PKCS11_LOG("login ok (dev=%u sess=%u)", sl->hsm_dev, sess); | ||
| return CKR_OK; |
Token objects now survive process restarts and are shared across processes. An opt-in file backend (enable with AZIHSM_PKCS11_PERSIST, store directory via AZIHSM_PKCS11_STORE_DIR, default /var/lib/azihsm/pkcs11) plugs in behind the existing object-store seam with no caller changes. The in-memory backend stays the default and keeps serving session objects for now. Objects persist as versioned, validated-on-decode records: the opaque masked-blob key body travels as a distinguished field, reachable only through new set/get key-body seam ops and never via C_GetAttributeValue. Writes are crash-atomic and cross-process safe (a reader sees the old or the new record, never a torn one), corrupt records are rejected, and object handles are never reused — even across a crash mid-create. PKCS#11 access control carries to disk: token isolation, private objects invisible without login (no existence leak), and sensitive or non-extractable secret attributes withheld. Store files are owner-only (0700/0600) with path-traversal defense, credentials are wiped from memory after initialization, and v1 refuses to persist private plaintext secrets the device does not protect (at-rest encryption is a later phase). Object create/destroy/find and attribute get/set all work against the file backend, verified end to end across processes; four standalone unit harnesses including a multi-process stress test run as a CI job, needing neither a device nor libcrypto.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a PKCS#11 correctness bug (ulValueLen handling on CKR_BUFFER_TOO_SMALL) and leaves sensitive config material uncleared on stack in the HSM-binding layer.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
plugins/azihsm_pkcs11/src/azihsm_pkcs11_objstore_mem.c:261
- When the caller's attribute buffer is too small, PKCS#11 requires
ulValueLento be set to the required size so the caller can retry. Setting it toCK_UNAVAILABLE_INFORMATIONbreaks the standard two-call/resize pattern and contradicts the function’s own comment.
if (tmpl[i].ulValueLen < a->len)
{
tmpl[i].ulValueLen = CK_UNAVAILABLE_INFORMATION;
rv = CKR_BUFFER_TOO_SMALL;
continue;
plugins/azihsm_pkcs11/src/azihsm_pkcs11_hsm.c:352
azihsm_pkcs11_configholds sensitive credential material; aftermake_creds()copies what it needs,cfgshould be wiped so later early-returns (provision failure / sess_open failure) don’t leave secrets on the stack.
struct azihsm_credentials creds;
azihsm_pkcs11_config cfg;
azihsm_pkcs11_config_load(&cfg);
make_creds(&creds, &cfg, pin, pin_len);
- Files reviewed: 44/44 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+300
to
+311
| azihsm_pkcs11_config cfg; | ||
| azihsm_pkcs11_config_load(&cfg); | ||
| struct azihsm_buffer obk_buf = { cfg.obk, sizeof(cfg.obk) }; | ||
| struct azihsm_owner_backup_key_config obk_cfg = { AZIHSM_OWNER_BACKUP_KEY_SOURCE_CALLER, | ||
| &obk_buf, | ||
| NULL }; | ||
| struct azihsm_buffer sig_buf = { sig, sizeof(sig) }; | ||
| struct azihsm_buffer pub_buf = { pub, (uint32_t)pub_len }; | ||
| struct azihsm_pota_endorsement_data pdata = { &sig_buf, &pub_buf }; | ||
| struct azihsm_pota_endorsement pota = { AZIHSM_POTA_ENDORSEMENT_SOURCE_CALLER, &pdata }; | ||
|
|
||
| return azihsm_part_init(dev, creds, NULL, NULL, &obk_cfg, &pota, NULL); |
| C_GenerateRandom CK_SESSION_HANDLE hSession, CK_BYTE_PTR RandomData, CK_ULONG ulRandomLen | ||
| C_GetFunctionStatus CK_SESSION_HANDLE hSession | ||
| C_CancelFunction CK_SESSION_HANDLE hSession | ||
| C_WaitForSlotEvent CK_FLAGS flags, CK_SLOT_ID_PTR pSlot, CK_VOID_PTR pRserved |
Jens Topp (jenstopp)
marked this pull request as ready for review
September 8, 2026 15:46
Jens Topp (jenstopp)
marked this pull request as draft
September 8, 2026 15:47
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new security-sensitive PKCS#11 surface area plus CI/copyright-policy changes and should receive final human review (including the ongoing third-party header licensing decision).
Review details
Suppressed comments (3)
plugins/azihsm_pkcs11/src/azihsm_pkcs11_objstore_mem.c:261
- When the caller-provided attribute buffer is too small, PKCS#11 expects ulValueLen to be set to the required length so the caller can retry. Setting ulValueLen to CK_UNAVAILABLE_INFORMATION here prevents correct two-call/retry behavior and contradicts the function comment about PKCS#11 §5.7.5 outcomes.
if (tmpl[i].ulValueLen < a->len)
{
tmpl[i].ulValueLen = CK_UNAVAILABLE_INFORMATION;
rv = CKR_BUFFER_TOO_SMALL;
continue;
plugins/azihsm_pkcs11/src/azihsm_pkcs11_store_record.c:322
- On CKR_BUFFER_TOO_SMALL, azihsm_pkcs11_meta_encode returns without updating *len. Setting *len to P11_META_SIZE mirrors the two-call sizing convention and makes retries easier.
plugins/azihsm_pkcs11/tools/pkcs11f_signatures.tsv:68 - Typo in parameter name: "pRserved" should be "pReserved" to match the PKCS#11 header signature and avoid propagating the typo if this TSV is re-used for future stub regeneration.
- Files reviewed: 44/44 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+512
to
+517
| if (tmpl[i].ulValueLen < a->len) | ||
| { | ||
| tmpl[i].ulValueLen = CK_UNAVAILABLE_INFORMATION; | ||
| ret = CKR_BUFFER_TOO_SMALL; | ||
| continue; | ||
| } |
Comment on lines
+144
to
+147
| if (*len < need) | ||
| { | ||
| return CKR_BUFFER_TOO_SMALL; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Long-running staging branch for the PKCS#11 module. All PKCS#11 PRs (#594 and follow-ups) merge into
feature/pkcs11instead ofmainuntil the licensing question on the vendored OASIS PKCS#11 v3.1 headers is settled.This PR is a draft on purpose — do not merge: it syncs main and the PKCS11 work, and runs the full CI suite on it.
Licensing status
The copyright CI gate is handled on this branch: the xtask copyright check now exempts vendored third-party directories (
plugins/pkcs11/include/pkcs11-v3.1), so the OASIS headers keep their upstream notice instead of being stamped with the Microsoft header so generated headers pass the check. A green gate does not settle the licensing question itself — whether the OASIS headers may be vendored here at all is still an open decision, and it is the blocker for marking this PR ready.Workflow
feature/pkcs11. They get the full regular CI: the branch is in the trigger filters .main: mergemainintofeature/pkcs11("update with merge commit", never rebase). The feature branch is shared! Every sync re-runs this PR's CI against latestmain.pkcs11.ymltriggers atmain, mark this PR ready, and merge with a merge commit to keep the one-commit-per-phase history.Note
Besides the merged feature PRs, the branch carries three infrastructure commits: the CI trigger filters for this branch, the third-party copyright exemption, and the cbindgen header-template fix.