tpm2-software / tpm2-pkcs11

A PKCS#11 interface for TPM2 hardware
https://tpm2-software.github.io
Other
278 stars 104 forks source link

Add support for importing TPM2 keys with PKCS11 vendor attributes #866

Closed wxleong closed 5 months ago

wxleong commented 5 months ago

Resolves #861

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 56.66667% with 325 lines in your changes missing coverage. Please review.

Project coverage is 71.45%. Comparing base (3f8d6e4) to head (3088b43). Report is 4 commits behind head on master.

:exclamation: Current head 3088b43 differs from pull request most recent head ca60b14

Please upload reports for the commit ca60b14 to get more accurate results.

Files Patch % Lines
tools/key_import/import.c 49.52% 266 Missing :warning:
src/lib/tpm.c 70.65% 27 Missing :warning:
src/lib/key.c 75.96% 25 Missing :warning:
src/lib/token.c 58.33% 5 Missing :warning:
src/lib/object.c 81.81% 2 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wxleong commented 2 months ago

@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.

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: https://github.com/tpm2-software/tpm2-pkcs11/issues/861

williamcroberts commented 2 months ago

@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.

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.

Yes I see that, the overall premise is fine, but the implementation is wrong, more below.

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.

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.

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?

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 https://github.com/tpm2-software/tpm2-pkcs11/commit/50a636bdb19220bf926cf6664e3084a315fe4480 is being reverted. However, feel free and re-submit it under C_CreateObject.

wxleong commented 2 months ago

Noted. i'll get this reimplemented.