Repository navigation
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved PKCS#11 persistence and lookup defects can break key validation or signing, while missing test guards cause build failures when ECC is disabled.
3 open issues
What changed in this PR
Extends wolfCrypt’s PKCS#11 integration to retain named keys across sessions and explicitly delete them, while keeping unnamed keys such as TLS key shares temporary.
Changes:
- Adds persistent import, session-key persistence, and token-key deletion APIs.
- Adds opt-in persistent key generation, name-collision checks, and protective key attributes.
- Documents the new APIs and expands mock-token tests for key lifecycles and failure paths.
| File | Description |
|---|---|
wolfssl/wolfcrypt/wc_pkcs11.h |
Declares persistence APIs and token/session flags. |
wolfssl/wolfcrypt/pkcs11.h |
Adds PKCS#11 session-state constants. |
wolfcrypt/src/wc_pkcs11.c |
Implements persistent key management and key-generation behavior. |
tests/api/test_pkcs11.h |
Registers the new PKCS#11 tests. |
tests/api/test_pkcs11.c |
Extends the mock token and tests persistence, deletion, and key naming. |
doc/dox_comments/header_files/pkcs11.h |
Documents API requirements, behavior, and usage. |
Review effort: Balanced
wc_Pkcs11StoreKey creates session objects only, so an imported key is gone once the session closes. wc_Pkcs11StoreKey_ex takes a persistent flag that creates the objects with CKA_TOKEN set instead, so they stay on the token and can be used by id or label later. Persistent objects need a read/write session and a logged in user. A persistent private or secret key also gets CKA_PRIVATE and CKA_SENSITIVE set and CKA_EXTRACTABLE cleared, so it can be neither read without a login nor exported while it stays on the token. wolfSSL never reads the value of such a key back, only public attributes such as the RSA modulus. Public key objects get CKA_TOKEN set and CKA_PRIVATE cleared, as the default for CKA_PRIVATE is up to the token and a private public key would be hidden from sessions that are not logged in. A persistent RSA key gets a public key object on the token too, which public key operations on a key loaded by name look up. A persistent key needs an id or label, or it could not be found again, and neither may be in use by a token key of the same type: wolfSSL finds keys by either one, and two keys with one name make lookups ambiguous. The call fails with BAD_FUNC_ARG or BAD_STATE_E otherwise. An HMAC key is checked under its HMAC key type and as the generic secret that wc_Pkcs11StoreKey falls back to. Objects of other types, such as the certificate that usually shares the key's id, do not clash, since lookups check the key type. The create helpers take the setting from a new tokenObjects bit in Pkcs11Session, which Pkcs11OpenSession clears, so sessions used for crypto operations keep creating their temporary keys as session objects. wc_Pkcs11StoreKey calls the new function with persistent set to 0, and a NULL token or key now fails with BAD_FUNC_ARG. The key lookup helpers are built for ML-DSA and ML-KEM only builds too. A persistent store does not need a session from wc_Pkcs11Token_Open, as its objects outlive the session the call opens and closes. wc_Pkcs11StoreKey still returns BAD_STATE_E without one.
Keys generated through the PKCS#11 crypto callback, and keys imported with wc_Pkcs11StoreKey, are session objects. Key generation cannot know whether the caller wants to keep the key, so an application decides after the fact, for example once a certificate for the key has been issued. wc_Pkcs11PersistKey copies the session object to a token object with C_CopyObject, which PKCS#11 allows for keys that are not extractable, sets the protecting attributes wc_Pkcs11StoreKey_ex uses and the key's id and label, and destroys the session original. The session object is the handle in devCtx when key generation left one there, otherwise the session object with the key's label and id. RSA, ECC, ML-DSA, ML-KEM, AES and HMAC keys are supported. An HMAC key needs its hash type and is found under its HMAC key type or as a generic secret. For RSA, ECC, ML-DSA and ML-KEM keys that hold their public key, a token public key object is created too, because RSA public key operations, the check-private-key paths and ML-KEM encapsulation look the public key up by id or label for a key loaded by name. As wc_Pkcs11StoreKey_ex does, an ECC public key object is created only when the token supports CKM_ECDSA, and a failure to create it is ignored when the token's private key object cannot sign. For ECC, ML-DSA and ML-KEM, session public key objects with the same names are destroyed, so such a lookup cannot find a stale one. A key without its public part, such as one initialized by id or label only, gets a token copy of the session public key object with its names instead, and the session object is destroyed only after the copy succeeds. An RSA session public key never has a name, since neither wc_Pkcs11StoreKey nor session key generation gives it one. RSA, ECC, ML-DSA and ML-KEM keys then refer to the new private object through devCtx, which the free callback leaves alone. The key needs an id or label that no token key of its type has, and BAD_STATE_E is returned when devCtx already refers to a token object. On failure the token objects created so far are destroyed and the key is left as it was.
Freeing a key leaves its token objects alone, so an application that rotates a persistent key needs a way to remove the old one. wc_Pkcs11DeleteKey destroys every token object of the key type with the key's id and label, which covers the private key and the public key object wc_Pkcs11PersistKey or wc_Pkcs11StoreKey_ex created. Labels need not be unique, so an object must match every name the key has. For RSA, ECC, ML-DSA and ML-KEM keys a session object in devCtx is destroyed too and devCtx is cleared. If a C_DestroyObject fails part way, devCtx is cleared when its object is already gone, so the key is not left with a dead handle. A delete refused before destroying anything keeps devCtx. An HMAC key is removed under its HMAC key type and as a generic secret, and needs its hash type. Nothing to remove is not an error. Private keys that wolfSSL stores on the token carry CKA_PRIVATE, so a session without a user login does not find them, and a delete would remove only the public object and report success. The session state is checked with C_GetSessionInfo first, and BAD_STATE_E is returned unless the user is logged in. The session state values are added to pkcs11.h. The new tests also cover persisting and deleting RSA, ECC, ML-DSA, ML- KEM, AES and HMAC keys, the naming rules across the three functions, and failures of C_CopyObject, C_GetSessionInfo and C_DestroyObject.
wc_Pkcs11PersistKey makes a generated session key persistent after the fact, but some tokens cannot generate a usable session key at all. NXP's SMW module on the EdgeLock Enclave reports the public point only for token objects and returns an empty CKA_EC_POINT for session keys, and the ELE cannot import a plaintext key, so wolfSSL's ECC key generation failed there. wc_Pkcs11Token_SetKeyGenPersistent sets a keyGenOnToken bit in Pkcs11Token. While it is set, RSA, ECC, ML-DSA and ML-KEM key generation through the crypto callback opens a read/write session and creates a key that has an id or label as a token object, with the protecting attributes of wc_Pkcs11StoreKey_ex. A key without a name, such as a TLS ECDHE ephemeral key, stays a session object. The public key is kept as a token object with the key's names, as wc_Pkcs11PersistKey leaves it. Key generation fails with BAD_STATE_E when a token key of the type already has the id or label. A token key outlives a session that closes after key generation, so key generation without an open token session is allowed for it. The handle from the closed session is not kept, and the key is found later by its id or label.
bd9fd4e to
41fdead
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| XMEMCPY(key->label, label, (size_t)key->labelLen); | ||
| } | ||
|
|
||
| int test_wc_Pkcs11_KeyNames(void) |
There was a problem hiding this comment.
🟡 (optional) Builds that keep HAVE_ECC but define NO_PKCS11_ECC now fail the PKCS#11 tests at runtime, or fail to compile when RSA key generation is also off. The new ECC tests have no TEST_PKCS11_ECC guard. Each existing test in this file has one. Fix: wrap the ECC-specific bodies in #ifdef TEST_PKCS11_ECC, like test_wc_Pkcs11_FreeKeepsTokenKey, which covers the 5 sites listed. Same pattern at 5 sites (test_pkcs11.c:2514 test_wc_Pkcs11_KeyNames, :884 StoreKeyPersistent, :950 PersistKey, :1110 DeleteKey, :2120 PersistPaths).
Why this was flagged
The trigger is a build that defines NO_PKCS11_ECC while keeping HAVE_ECC. The file guard at tests/api/test_pkcs11.c:27 still compiles the file, but TEST_PKCS11_ECC (line 275) is not defined. In wolfcrypt/src/wc_pkcs11.c:52, NO_PKCS11_ECC undefines HAVE_ECC, so the PKCS11_KEY_TYPE_EC case of wc_Pkcs11StoreKey_ex/wc_Pkcs11PersistKey/wc_Pkcs11DeleteKey is compiled out. Those calls return NOT_COMPILED_IN, and the Expect checks for 0 or BAD_FUNC_ARG/BAD_STATE_E in test_wc_Pkcs11_StoreKeyPersistent, _PersistKey, _DeleteKey, _KeyNames and _PersistPaths then fail. When TEST_PKCS11_RSA_KEYGEN is also undefined, test_pkcs11_count (guarded at line 370) does not exist and the file fails to compile. On the base branch every test body sits under TEST_PKCS11_ECC or TEST_PKCS11_EC_KEYGEN, so these configurations build and pass there.
Verification: Trigger: a build with HAVE_PKCS11 and HAVE_ECC that also defines NO_PKCS11_ECC. TEST_PKCS11_ECC stays undefined (tests/api/test_pkcs11.c:275-277), and wolfcrypt/src/wc_pkcs11.c:52-54 drops the EC case, so wc_Pkcs11StoreKey_ex returns NOT_COMPILED_IN (line 2697-2698). So in this build those tests fail at runtime. The base file passed in this configuration.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12112
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/pkcs11.h, tests/api/test_pkcs11.h
Coverage: 4 of 4 in-scope changed file(s) opened by the reviewer
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Max
| WC_NO_ERR_TRACE(BAD_STATE_E)); | ||
| ExpectIntEQ(test_pkcs11_count(CK_TRUE), 2); | ||
|
|
||
| ExpectIntEQ(test_pkcs11_free_ecc(&token, &key), 0); |
There was a problem hiding this comment.
New unguarded ECC tests call helpers that only exist under TEST_PKCS11_ECC, breaking NO_PKCS11_ECC builds · Preprocessor-conditional security bypass / Incorrect macro…
test_pkcs11_free_ecc (line 817) only exists under #ifdef TEST_PKCS11_ECC, and test_pkcs11_count (line 370) only under TEST_PKCS11_ECC || TEST_PKCS11_RSA_KEYGEN. The new ECC tests (StoreKeyPersistent, PersistKey, DeleteKey, PersistPaths, KeyNames) have no guard, so a NO_PKCS11_ECC build no longer compiles.
Suggested fix: Wrap the bodies of the new ECC-based tests in #ifdef TEST_PKCS11_ECC, as test_wc_Pkcs11_FreeKeepsTokenKey does, so they compile out with NO_PKCS11_ECC.
Basis: ISO C99 6.5.1p2: an identifier used as a primary expression must be declared, so calling an undeclared test_pkcs11_free_ecc/test_pkcs11_count is ill-formed.
Description
This PR adds a way to keep keys on a PKCS#11 token across sessions. Until now every key wolfSSL imported or generated was a session object that disappeared with its session.
wc_Pkcs11StoreKey_ex(..., persistent)imports a key as token objects. Private and secret keys getCKA_PRIVATEandCKA_SENSITIVEand loseCKA_EXTRACTABLE; public keys clearCKA_PRIVATE.wc_Pkcs11PersistKeycopies a session key, from keygen orwc_Pkcs11StoreKey, to the token and pointsdevCtxat the copy.wc_Pkcs11DeleteKeyremoves a key's token objects by id and label.wc_Pkcs11Token_SetKeyGenPersistentmakes keygen create named keys as token objects; nameless keys such as TLS key shares stay session or software keys.Testing
test_wc_Pkcs11_StoreKeyPersistent,test_wc_Pkcs11_PersistKey,test_wc_Pkcs11_DeleteKey,test_wc_Pkcs11_KeyGenPersistent,test_wc_Pkcs11_KeyNames,test_wc_Pkcs11_SecretKeyPersist,test_wc_Pkcs11_RsaPersistDelete,test_wc_Pkcs11_PqcPersistDelete,test_wc_Pkcs11_PersistPaths,test_wc_Pkcs11_PqcKeyGen.CKA_PRIVATEtrue, as SoftHSM2 does, and checks the protecting attributes of every token key.Merging
Adds tests next to #11876 and #12111 in
tests/api/test_pkcs11.c, and changes the samepkcs11_dump_templateguard as #11876; whichever merges later needs a small rebase there.