cryptocb: add WC_ALGO_TYPE_KEYSTORE for hardware key store operations - #11336
cryptocb: add WC_ALGO_TYPE_KEYSTORE for hardware key store operations#11336Frauschi wants to merge 1 commit into
Conversation
50b4b63 to
27e079b
Compare
|
|
Jenkins retest this please - history lost. |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11336
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
27e079b to
648db08
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11336
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
648db08 to
122bfc6
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11336
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
122bfc6 to
8518604
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11336
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
|
Jenkins retest this please |
padelsbach
left a comment
There was a problem hiding this comment.
Nice addition of this helpful feature. A few minor nits from me. Over to @bigbrett
| const byte* keyRef; | ||
| word32 keyRefSz; | ||
| word32* keyType; /* out: enum wc_KeyStoreKeyType */ | ||
| word32* keySz; /* out: key size in BITS */ |
There was a problem hiding this comment.
Nit: can we use keyBits, keySzBits or similar to avoid conflating with other uses of keySz in this union?
There was a problem hiding this comment.
Fixed. Moved to keyBits
| \sa wc_KeyStore_Derive | ||
| */ | ||
| int wc_KeyStore_GetInfo(int devId, const byte* keyRef, word32 keyRefSz, | ||
| word32* keyType, word32* keySz, word32* attrs, const void* ctx); |
There was a problem hiding this comment.
Nit: should probably have a blank line at the end of this file
| return BAD_FUNC_ARG; | ||
| } | ||
|
|
||
| dev = wc_CryptoCb_FindDevice(devId, WC_ALGO_TYPE_KEYSTORE); |
There was a problem hiding this comment.
the GetInfo() call below clears out the params it is about to modify:
if (keyType != NULL) {
*keyType = 0;
}
if (keySz != NULL) {
*keySz = 0;
}
if (attrs != NULL) {
*attrs = 0;
}
Should we do that here too? And any vars in the other new functions?
There was a problem hiding this comment.
Fixed where appropriate
8518604 to
9f7358c
Compare
A hardware key store holds keys with a lifetime of their own, and none of the existing callbacks can manage them. WC_ALGO_TYPE_SETKEY and WC_ALGO_TYPE_EXPORT_KEY are bound to a wolfCrypt key object and carry material for that object's own use: neither can name a stored key, ask for one to be created exportable, or make one outlive the object that used it. Wrapped keys add a second reason, since a wrapped blob never becomes plaintext on this side of the boundary at all. Add a dedicated algorithm type covering the operations that manage a stored key, whether or not they cross that boundary: WC_KEYSTORE_IMPORT_PLAIN place plaintext key material into a slot WC_KEYSTORE_EXPORT_PLAIN read a stored key back as plaintext WC_KEYSTORE_IMPORT_WRAPPED unwrap a blob directly into a slot WC_KEYSTORE_EXPORT_WRAPPED wrap a stored key back out WC_KEYSTORE_DERIVE derive slot to slot without touching RAM WC_KEYSTORE_DELETE destroy a stored key WC_KEYSTORE_GET_INFO query what a slot holds The plaintext pair is what most key stores outside the secure-element class offer, and it is the shape wc_Pkcs11StoreKey() already implements privately: that function takes a Pkcs11Token* rather than a devId, so an application using it is bound to PKCS#11 even though the operation is generic. Devices that hold keys which may never appear in plaintext decline the pair and offer only the wrapped form. Delete is deliberately separate from WC_ALGO_TYPE_FREE: freeing a wolfCrypt key object must never destroy the hardware key it refers to. A key reference is not a new naming scheme. It is the identifier WOLF_PRIVATE_KEY_ID already uses, so the bytes that name a key here are the bytes wc_ecc_init_id() or wc_AesInit_Id() take to bind an object to that key, and the bytes read back from key->id afterwards. wolfCrypt copies them through without inspection. Two details come from building the NXP EdgeLock port against this surface, which is what a design like this needs before it is fixed in place. Every operation that creates a key takes keyType, because a key store has to know what a key is for before it can set its permissions, and the material does not always say. Raw bytes carry no metadata, and neither does a bare RFC 3394 wrap, which is a pure data transformation. A vendor container that carries its own property word stays authoritative over what it holds and keyType is then a cross-check, which catches the right blob going into the wrong kind of slot; a device must refuse a mismatch rather than silently prefer one source. WC_KEYSTORE_KEY_NONE leaves the choice to the device. The export operations take no keyType, since the key already exists and GET_INFO reports it. attrs travels with every creating operation for the same reason, but as a source only. A format carrying no attributes of its own leaves attrs as the device's only word on what the new key may do. A container that carries them wins and attrs is ignored, and it cannot usefully be cross-checked there: a container's attributes are inside the wrap and are not known until the key exists, whereas keyType is checkable up front against the reference being imported into. wc_KeyStore_GetInfo() is how a caller confirms what an import produced. keyType also fixes how plaintext material is encoded, so no further argument is needed to say. A symmetric type takes the raw key bytes; an asymmetric type takes DER, a private key as PKCS#8 PrivateKeyInfo and a public key as SubjectPublicKeyInfo. Stating it matters: WC_KEYSTORE_KEY_ECC_SIGN and a byte string do not otherwise say whether the bytes are a scalar, SEC1 or PKCS#8, and two devices could each pick differently and both be defensible. Keys that act on other keys need naming. Without WC_KEYSTORE_KEY_WRAP and WC_KEYSTORE_KEY_DERIVE a device asked about a wrapping key can only answer NONE, which is indistinguishable from an empty slot and defeats the point of GET_INFO. Measured on hardware, a 256-bit wrapping key reported type 0 with 256 bits, which tells a caller nothing about what it may do with it. Both are listed ahead of the algorithm types, since they name what a key acts on rather than which algorithm it serves. The algorithm types cover what the two other backends this was checked against actually store. wc_Pkcs11StoreKey() handles RSA, ECC and ML-KEM today, and wolfHSM's WH_KEY_ALGO_ENUM names RSA, ECC, Curve25519, Ed25519, ML-DSA, ML-KEM, LMS and XMSS, so a shorter vocabulary would have left the facility unusable to both for anything but symmetric keys. An algorithm serving two purposes a key store grants separately is split, since one key doing both is the key-separation problem and a caller has to be able to ask for the narrower key. RSA and ECC are the only two: PKCS#11 carries CKA_SIGN and CKA_DECRYPT independently for RSA, and CKA_SIGN against CKA_DERIVE for ECC. wolfCrypt's own PKCS#11 layer honours that for ECC, selecting from the ecc_key flags, but sets both CKA_DECRYPT and CKA_SIGN unconditionally for RSA because no equivalent RsaKey flag exists; expressing the distinction here is what would let that be tightened later. The remaining types serve one purpose each and are not split. Signature use is listed before agreement or transport use throughout. Argument order follows one rule: two interchangeable control words are never left adjacent, because a transposition between them compiles cleanly and surfaces much later as an unrelated-looking failure. An earlier arrangement had attrs and kdfType separated only by a pointer and a length, and swapping them silently created a key without the exportable attribute, whose export then failed two operations away from the mistake. Beyond that each control word sits beside what it describes. wc_KeyStore_ImportPlain is the one place the two aims collide and the first wins: attrs sits at the tail of every creating operation rather than beside keyType, because WC_KEYSTORE_ATTR_EXPORTABLE and WC_KEYSTORE_KEY_WRAP are both 1 and a swap would be silent. All three creating operations therefore read keyRef, keyRefSz, keyType, then what they draw the key from, then attrs. kdfType refers to enum wc_KdfType, and WC_KDF_TYPE_NONE asks for the device's own derivation, which is all many key stores offer. Attributes are fixed when a key is created; hardware generally burns them in, so there is no operation here to change them afterwards. They round-trip: a device that can represent one must report it back through GET_INFO, so a caller can ask whether an export is permitted rather than attempting one and interpreting the error. Attributes the device cannot represent read as absent, and out parameters it does not fill are cleared rather than left holding the caller's stack. Gated behind WOLF_CRYPTO_CB_KEYSTORE, with tests in wolfcrypt/test and tests/api that assert each operation reaches the device carrying the arguments the caller passed, not merely that the call returned zero.
9f7358c to
6b46c69
Compare
Why
wolfCrypt can already use a key that lives in a hardware key store: attach a
WOLF_PRIVATE_KEY_IDidentifier to a key object and the crypto callback does the rest. It cannot manage that key's lifetime. There is no way to put a key into the store, take one out, derive one, destroy one, or ask what a slot holds.The existing callbacks cannot be stretched to cover it:
WC_ALGO_TYPE_SETKEYandWC_ALGO_TYPE_EXPORT_KEYare bound to a wolfCrypt key object and carry material for that object's own use. Neither can name a stored key, ask for one to be created exportable, or make one outlive the object. So today every secure element port invents its own vendor API for this, and applications using one are welded to that vendor.This is a prerequisite for the NXP RW612 EdgeLock (ELS + PKC) crypto callback port, which follows in a separate PR and cannot express its key store at all without it. That port is the first consumer and is the reason this lands now, but nothing here is NXP-specific.
What this adds
WC_ALGO_TYPE_KEYSTORE, with seven operations:IMPORT_PLAIN/EXPORT_PLAINIMPORT_WRAPPED/EXPORT_WRAPPEDDERIVEDELETEGET_INFOPublic entry points in
wolfssl/wolfcrypt/wc_keystore.h, dispatch incryptocb.c, all behindWOLF_CRYPTO_CB_KEYSTORE(--enable-cryptocbutils=keystore). Off by default; no existing code path changes.Backends it was validated against
Three: the EdgeLock port above,
wc_Pkcs11StoreKey(), and wolfHSM's key cache. The type vocabulary and the plaintext encoding rule come from the latter two.wc_Pkcs11StoreKey()is the same operation asIMPORT_PLAIN, reachable only through aPkcs11Token*, so an application using it is bound to PKCS#11 even though the operation is generic. Its AES and HMAC cases map directly onto this API.Neither backend is fully covered yet. PKCS#11's asymmetric cases and several of wolfHSM's key cache features have no expression here, and each of those is purely additive: appending an operation or an enum value breaks no existing device, so none has to be settled now.
Design points
Delete is separate from
WC_ALGO_TYPE_FREE. Folding it in would makewc_ecc_free()silently destructive for any hardware-backed key.A key reference is not a new naming scheme. It is the identifier
WOLF_PRIVATE_KEY_IDalready uses, so the bytes naming a key here are the byteswc_ecc_init_id()takes and the bytes read back fromkey->id.keyTypeandattrsare on every operation that creates a key, and neither export.keyTypealso fixes the plaintext encoding: raw bytes for a symmetric type, DER for an asymmetric one. Where a format carries no metadata (a bare RFC 3394 wrap, or raw bytes) both are the device's only source. Where a container carries its own, the container wins:keyTypebecomes a cross-check a device must refuse a mismatch on, andattrsis ignored, since a container's attributes are inside the wrap and unknowable until the key exists.RSA and ECC are split by purpose, because a key store grants their two purposes separately (
CKA_SIGNagainstCKA_DECRYPT, andCKA_SIGNagainstCKA_DERIVE) and one key doing both is the key-separation problem. Everything else is single-purpose. Note that wolfCrypt's own PKCS#11 layer honours this for ECC but sets bothCKA_DECRYPTandCKA_SIGNunconditionally for RSA, for want of anRsaKeyflag; having the distinction here is what would let that be tightened.Argument order is a safety property. Two interchangeable control words are never left adjacent, because a transposition between them compiles cleanly and surfaces much later as an unrelated-looking failure.
attrstherefore sits at the tail of every creating operation rather than besidekeyType:WC_KEYSTORE_ATTR_EXPORTABLEandWC_KEYSTORE_KEY_WRAPare both 1, so a swap would be silent.Testing
Tests in
wolfcrypt/testandtests/apiassert that each operation reaches the device carrying the arguments the caller passed, not merely that it returned zero, plus argument validation before any device is consulted, the size-query path, the cleared-output contract, andCRYPTOCB_UNAVAILABLEfrom every entry point when nothing is registered. Verified to fail as well as pass: transposingkeyTypeandattrsat a call site makesunit.testexit non-zero.