Add ExportPublicKey API for cached asymmetric keys#346
Add ExportPublicKey API for cached asymmetric keys#346Frauschi wants to merge 3 commits intowolfSSL:mainfrom
Conversation
bigbrett
left a comment
There was a problem hiding this comment.
Overall, looks great. A few nits and suggetsions, with some architectural things as well.
One thing I'm not sure about is the bypass of the NONEXPORTABLE attribute. I totally see why you would do it this way, but I don't want to prevent a customer from making an object fully nonexportable (e.g. unreadable) for whatever reason. Therefore I wonder if it makes sense to have additional attributes here that only apply to keys? Happy to brainstorm this.
| * @return int Returns 0 on success, or a negative error code on failure. | ||
| */ | ||
| int wh_Client_KeyExportPublicDmaRequest(whClientContext* c, whKeyId keyId, | ||
| uint16_t algo, const void* keyAddr, |
There was a problem hiding this comment.
I'm not sure how I feel about this being const. While technically correct, as this function doesn't actually modify the data pointed to, it tells the server to modify it, so the const contract is quite disingenuous.
There was a problem hiding this comment.
Yeah, you're right on this. I removed the const qualifier.
| /* Public-key algorithm selector used by APIs that must interpret a cached | ||
| * key's DER contents (e.g. WH_KEY_EXPORT_PUBLIC), since NVM metadata does | ||
| * not carry an algorithm type. Values are on-wire, append-only. */ | ||
| enum WH_KEY_ALGO_ENUM { |
There was a problem hiding this comment.
Hmmmm wish we could use enum wc_PkType from wolfssl/wolfcrypt/types.h here but if we are to differentiate between PQC subtypes then unfortunately we can't. No change necessary
There was a problem hiding this comment.
I'm also not a fan of the design decision to wrap all PQC algorithms behind the two generic WC_PK_TYPE_PQC_KEM_ and WC_PK_TYPE_PQC_SIG_ types in enum wc_PkType. But changing this now would be a big break for all CryptoCb users, so thats what we have to live with...
| if ((resp_group != WH_MESSAGE_GROUP_KEY) || | ||
| (resp_action != WH_KEY_EXPORT_PUBLIC_DMA) || |
There was a problem hiding this comment.
Dont need to do this anymore, since it is checked in wh_Client_RecvResponse() against the last sent kind (which is the fused group/action
| if (labelSz > sizeof(resp->label)) { | ||
| memcpy(label, resp->label, WH_NVM_LABEL_LEN); | ||
| } | ||
| else { | ||
| memcpy(label, resp->label, labelSz); | ||
| } |
There was a problem hiding this comment.
nit: condense to single memcpy with labelSz clamped if it exceeds WH_NVM_LABEL_LEN. See how you did it elsewhere in this file.
| if ((label_len > 0) && (label != NULL)) { | ||
| if (label_len > WH_NVM_LABEL_LEN) { | ||
| label_len = WH_NVM_LABEL_LEN; | ||
| } | ||
| memcpy(label, keyLabel, label_len); | ||
| } |
There was a problem hiding this comment.
why are we redundantly re-clamping the label here? Isn't this already done in wh_Client_KeyExportPublic()?
There was a problem hiding this comment.
The clamping here is currently necessary as we initially read the exported label in the local keyLabel buffer with its own size (not necessarily the same size as the user provided buffer). wh_Client_KeyExportPublic()clamps for the internal buffer only.
We could directly export the label in the user provided label buffer via wh_Client_KeyExportPublic() to prevent both the additional buffer and the second clamping, but that would have the side-effect of also writing into that buffer even when the key deserialization fails afterward.
I haven't done any changes yet, but could do the above if desired.
| /* Exporting only the public half of a public-key object is considered | ||
| * non-sensitive and is intentionally not gated by NONEXPORTABLE. */ |
There was a problem hiding this comment.
hmmm I don't know if I agree with this - NONEXPORTABLE is a generic object-level attribute that doesn't care if the object is a key. I wonder if we need a new attribute/flag. Currently we have "sensitive" which means that it can never be exported in unwrapped form so perhaps it makes sense to have a new flag for public and private halves of a key? PUBNONEXPORTABLE and PRIVNONEXPORTABLE? Let me think on this ....
There was a problem hiding this comment.
Haven't done any changes for this.
This is definitely a broader design decision worth discussing. We can add two new flags PUBNONEXPORTABLE and PRIVNONEXPORTABLE, but how would these interplay with the existing NONEXPORTABLE?
Currently, I cannot see any realistic use case in which one would prevent exporting the public key, but having the option for it is valid. So the public key of an asymmetric key would be exportable always, except for when PUBNONEXPORTABLE is set on key generation or import.
The private part would work similarly (prevent export only when PRIVNONEXPORTABLE is set on the object). The question is now how to handle the generic NONEXPORTABLE? I see these options now:
NONEXPORTABLEapplies to both public and private parts, so bothPUBNONEXPORTABLEandPRIVNONEXPORTABLEare set on the key.NONEXPORTABLEapplies only to the private part, so onlyPRIVNONEXPORTABLEis set on the key. The public part could still be exported, as it is non-sensitive.
As I currently lack a realistic use case for public key protection, I'm in favor for option 2. That maps to the typical use cases of asymmetric key generation for authentication (send the public key to a peer for signature verification) as well as key agreement (send the public key to a peer for ECDH/KEM encapsulation).
There was a problem hiding this comment.
it is indeed a handy simplification to have it work the way you designed it. I'm just wondering if there is ever a case where you woulndt want to export pubkey. They are all contrived but it is a slight change to the object model. Ill think on it, probs fine for now
| RsaKey rsa[1]; | ||
| int pub_ret; | ||
| ret = wc_InitRsaKey_ex(rsa, NULL, INVALID_DEVID); | ||
| if (ret == 0) { | ||
| ret = wh_Server_CacheExportRsaKey( | ||
| server, serverKeyId, rsa); | ||
| if (ret == 0) { | ||
| pub_ret = wc_RsaKeyToPublicDer( | ||
| rsa, stage, (word32)stageMax); | ||
| if (pub_ret > 0) { | ||
| der_len = (uint16_t)pub_ret; | ||
| } | ||
| else { | ||
| ret = (pub_ret == 0) | ||
| ? WH_ERROR_ABORTED | ||
| : pub_ret; | ||
| } | ||
| } | ||
| wc_FreeRsaKey(rsa); | ||
| } | ||
| } break; |
There was a problem hiding this comment.
Can you unify the wolfCrypt public extraction and epxort logic for each algo into helper functions to keep the switch statement smaller? Seems you should be able to share this across DMA/non-DMA versions to prevent duplication too.
There was a problem hiding this comment.
Fixed as suggested.
Introduces a new keystore action WH_KEY_EXPORT_PUBLIC that re-emits only the public portion of a cached public-key object, so callers that need a public key for a client-side operation (signature verification, key transport, etc.) no longer have to pull private material out of the HSM. A new WH_KS_OP_EXPORT_PUBLIC policy branch gates the path and intentionally bypasses NONEXPORTABLE since public material is non-sensitive. Wired end-to-end for RSA, ECC, Ed25519, Curve25519, and ML-DSA, with per-algorithm client wrappers (wh_Client_<Algo>ExportPublicKey) and smoke tests that round-trip real operations (sign/verify, ECDH) against the exported public keys, plus a negative test for unknown keyId. Also adds a DMA variant (WH_KEY_EXPORT_PUBLIC_DMA) with a generic client transport and an ML-DSA-specific wrapper, byte-identity cross-validation against the non-DMA path, and a NOSPACE bounds-check test. Documentation added to docs/src/chapter05.md and docs/src-ja/chapter05.md. New message structs registered in the padding-check test.
84c66a6 to
719669c
Compare
|
Addressed the review comments (except for two which are open for discussion). I also had to increase |
Add ExportPublicKey API for cached asymmetric key objects
Summary
Adds a dedicated path for extracting only the public half of a cached public-key keypair, so callers that need a public key on the client side (signature verification, certificate building, key transport, etc.) no longer have to pull the private material out of the HSM.
Previously, the only way to get the public half of a cached keypair was to call
wh_Client_<Algo>ExportKey(), which goes through the algorithm-agnosticwh_Client_KeyExport()and returns the raw cached DER — including any private material. For the common case "I cached a keypair for on-HSM signing and I just need the public key on the client," shipping the private key defeats the security benefit of caching.This PR adds:
WH_KEY_EXPORT_PUBLIC+ per-algorithm client wrappers.WH_KEY_EXPORT_PUBLIC_DMA+wh_Client_KeyExportPublicDmageneric transport + per-algorithm DMA wrapper for ML-DSA (matches the existing ML-DSA-only DMA-export precedent).Algorithms wired end-to-end
wh_Client_RsaExportPublicKeywh_Client_EccExportPublicKeywh_Client_Ed25519ExportPublicKeywh_Client_Curve25519ExportPublicKeywh_Client_MlDsaExportPublicKeywh_Client_MlDsaExportPublicKeyDmaDesign notes
WH_KEY_EXPORTinstead of duplicating it per algorithm. The selector is a newWH_KEY_ALGO_*enum inwolfhsm/wh_common.h.NONEXPORTABLEcarve-out.WH_NVM_FLAGS_NONEXPORTABLEblocks full-export but not public-only export, because public material is non-sensitive and blocking it would make cached keys unusable for any external verification or key-transport use case. This is a dedicatedWH_KS_OP_EXPORT_PUBLICbranch in_KeystoreCheckPolicy(not a silent bypass) and is called out explicitly in the docs.cacheBuf/cacheMetausing the existingwh_Crypto_*DeserializeKeyDerhelpers (which already fall back to public-only decode), then re-emits public-only DER via the matchingwc_*PublicKeyToDer. No new server-side crypto helpers introduced.resp_packet(the DMA response struct itself only occupies the header), thenwhServerDma_CopyToClients it into the client-provided buffer. The response sent over the wire is justsizeof(resp).WH_ERROR_NOTFOUND.wc_*PublicKeyToDerreturning 0 →WH_ERROR_ABORTED(explicitly, not a silent zero-length success). Too-small DMA client buffer →WH_ERROR_NOSPACE.Wire protocol
New keystore actions (appended to
enum WH_KEY_ENUMso existing numeric values are preserved):WH_KEY_EXPORT_PUBLICWH_KEY_EXPORT_PUBLIC_DMA(underWOLFHSM_CFG_DMA)Each takes a
uint16_t algoselector alongside the standardkeyId. Integrators with custom transports route these the same way they routeWH_KEY_EXPORT/WH_KEY_EXPORT_DMA.Test plan
End-to-end per algorithm:
NONEXPORTABLEcached key → full export denied withWH_ERROR_ACCESS→wh_Client_RsaExportPublicKeysucceeds → client-sidewc_RsaPublicEncrypt/ HSM-sidewc_RsaPrivateDecryptround-trips plaintext. Includes wrong-algo and unknown-keyId (WH_ERROR_NOTFOUND) negative cases.type == ECC_PUBLICKEY.wh_Client_Ed25519Sign, client verifies withwc_ed25519_verify_msg; assertspubKeySet==1 && privKeySet==0.local_priv·hsm_pub == hsm_priv·local_pub.wh_Client_MlDsaExportPublicKey→ assertspubKeySet==1 && prvKeySet==0.DMA-specific coverage:
wh_Client_KeyExportPublicDma(generic transport).wh_Client_MlDsaExportPublicKeyDma+ flag assertions, plus a byte-identity check comparing DMA-path DER vs. non-DMA-path DER for the same cached key, plus aWH_ERROR_NOSPACEnegative test with an undersized client buffer.Docs updated in
docs/src/chapter05.mdanddocs/src-ja/chapter05.md.