Add support for importing TPM2 keys with PKCS11 vendor attributes #866
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #866 +/- ##
==========================================
- Coverage 72.43% 71.45% -0.99%
==========================================
Files 34 35 +1
Lines 9773 10484 +711
==========================================
+ Hits 7079 7491 +412
- Misses 2694 2993 +299 ☔ View full report in Codecov by Sentry. |
wxleong
force-pushed
the
impl-issue-861
branch
7 times, most recently
from
32f27ff
to
9a72b32
Compare
- Add support for importing TPM2 keys (as persistent handle or key objects) using PKCS11 vendor-specific attributes - Add a new CLI tool: key_import - Add integration test - Add docs/KEY_IMPORT_TOOL.md Signed-off-by: wenxin.leong <[email protected]>
There was a problem hiding this comment.
@AndreasFuchsTPM and @wxleong. Am I missing something here or did you essentially reimplement tpm2_ptool link commandlet?
Also, shouldn't this be done with C_CreateObject and not C_GenerateKeyPair?
I'm looking at this as a complete revert.
| { CKR_CRYPTOKI_ALREADY_INITIALIZED, "CKR_CRYPTOKI_ALREADY_INITIALIZED" }, | ||
| { CKR_MUTEX_BAD, "CKR_MUTEX_BAD" }, | ||
| { CKR_MUTEX_NOT_LOCKED, "CKR_MUTEX_NOT_LOCKED" }, | ||
| { CKR_VENDOR_DEFINED, "CKR_VENDOR_DEFINED" }, |
There was a problem hiding this comment.
You could have just stringified these with xstr macro
| CK_OBJECT_HANDLE pubkey; | ||
| CK_OBJECT_HANDLE privkey; | ||
|
|
||
| rv = C_GenerateKeyPair(session, mech, pub, pub_len, priv, priv_len, &pubkey, &privkey); |
There was a problem hiding this comment.
I do believe this should have been C_CreateObject called twice, once for the pub key and once for the private key as this doesn't generate a key pair.
tpm2_ptool imports keys by making direct changes to the database and does not support the FAPI backend. The new key_import tool, however, takes a different approach to importing pre-generated TPM keys (whether persisted or as key objects). It uses newly introduced PKCS#11 vendor-specific attributes, making it compatible with any backend. Additionally, it eliminates the need to rely on Python for the operation. As for why it was implemented under C_GenerateKeyPair instead of C_CreateObject, the decision was based on ease of use and code reuse, as the logical flow closely mirrors C_GenerateKeyPair. While it could have been implemented under C_CreateObject, this feedback unfortunately came a bit too late. Do you see this as a critical concern? You can find the initial proposal here: #861 |
Yes I see that, the overall premise is fine, but the implementation is wrong, more below.
You don't use an API for generating key pairs as a mechanism for import as it violates the PKCS11 API. If you need to share code flows and code between two areas of a piece of software, just refactor it and place common code in functions.
Critical concern as it breaks the API standard for PKCS11 and should be done under C_CreateObject. This software cannot be released. However, many of the fixes patches can stay. As of right now, I think only 50a636b is being reverted. However, feel free and re-submit it under C_CreateObject. |
|
Noted. i'll get this reimplemented. |
Resolves #861