Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TPM key import #194

Closed
wants to merge 1 commit into from
Closed

Conversation

gotthardp
Copy link
Contributor

May I help finalizing the import feature (#39), please? I took over the content of the #140 and fixed (most of the) review comments, except the following.

Pending questions:
"Can't we just import a public key only?" (@williamcroberts) -- Right now the tpm2tss-genkey generates both public and private portion. Consistently with this, the import function lets the user to import public&private portion of a pre-existing key.

"Shouldn't we make the arguments make consistent with tpm2-tools?" (@gotthardp) The tpm2-tools consistently use -u for the public portion and -r for the private portion. I'd suggest to stick to this convention and use -u/-r instead of -i/-k. Would you agree?

Is there anything else that should be addressed?

@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #194 into master will increase coverage by 0.12%.
The diff coverage is 72.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
+ Coverage   67.71%   67.84%   +0.12%     
==========================================
  Files           8        8              
  Lines        1112     1160      +48     
==========================================
+ Hits          753      787      +34     
- Misses        359      373      +14     
Impacted Files Coverage Δ
src/tpm2-tss-engine-common.c 76.40% <65.62%> (-1.37%) ⬇️
src/tpm2tss-genkey.c 59.11% <83.33%> (+2.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae070ca...55c5350. Read the comment docs.

@AndreasFuchsTPM
Copy link
Member

Awesome ! Thanks for taking up this task.

I'd be fine with using -u and -r for the keyfiles.

Please also sQuash both commits into one with you as the author. You can add the other authors using a Co-authored-by tag.
And also please add some explanatory commit message as well.

I'll be VERY happy to merge this then. :-)

src/tpm2tss-genkey.c Outdated Show resolved Hide resolved
Introduce a possibility to import keys created e.g. with tpm2_create.
Consumes the public (-u) and private (-r) portion of the TPM key and
produces the 'TSS2 PRIVATE KEY' file.

Signed-off-by: Petr Gotthard <[email protected]>
Co-authored-by: Andreas Fuchs <[email protected]>
Co-authored-by: Takahide Higuchi <[email protected]>
@williamcroberts
Copy link
Member

LGTM, we'll see what CI has to say and if Andreas has anymore feedback.

Copy link
Member

@AndreasFuchsTPM AndreasFuchsTPM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks a lot !

@williamcroberts
Copy link
Member

I used this to test GH Actions since it was an approved PR. Its been moved over to here and merged:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants