-
Notifications
You must be signed in to change notification settings - Fork 160
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
Implement TPM2_Commit #296
Conversation
Define `TPMS_SCHEME_ECDAA` as TPMSSchemeECDAA. See definition in Part 2: Structures, section 11.1.18.
section 11.1.10 --> section 11.1.19
Define the TPMS_SIG_SCHEME_ECDAA as the SigSchemeECDAA. See definition in Part 2: Structures, section 11.2.1.3.
Define the TPM2B_ECC_POINT as the ECCPoint/TPM2BECCPoint. See definition in Part 2: Structures, section 11.2.5.3.
Update the test of TPM2_Commit to use TPM2_CreateLoaded instead of TPM2_Create/TPM2_Load.
Note that this commit is a work in progress, so the test on this commit not work.
Remove the space between the comma-separated type annotations.
tpm2b.Private of CreateLoadedResponse is not a pointer.
Create another key that is not the primary key when testing the TPM2_Commit.
tpm2.ECPoint => tpm2b.ECCPoint
Thank you for your review! But I found the bug around parsing the response. |
Count => Counter
Add flush context for the primary key when testing TPM2_Commit.
Private => ECCPoint
It was a misunderstanding, the TPM2_Commit on
|
I fixed some points which you commented on. |
direct/tpm2/commit_test.go
Outdated
}, | ||
} | ||
|
||
_, err = commit.Execute(thetpm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the output response with some off-tpm validation would be good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm...Any good idea to test this output...?
I have just an idea that implements full ECDAA Setup/Join for this, but it's complex and we should avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @chrisfenner is a better person to discuss this with, however he is out at the moment and will be back next week. In the meantime, I will add him to be a reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new to ECDAA signing, but I think you should be able to use TPM2_Sign (which is already implemented) to produce an ECDAA signature using the commit value. You'll have to grab the commit counter from the Commit command and use it on the ECDAA scheme structure in the Sign command.
Bonus points: Call Sign again with the same counter value and expect it to fail :)
No need to validate the signature at this point, unless you really want to. I imagine that belongs as part of a larger body of ECDAA sample code that someone might work on (maybe you or someone else) as part of a separate effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience, I was on vacation and then catching up on work after I returned from vacation :)
The functionality looks great, thanks for the change! I have a few notes on the test to make it tighter and leaner but also cover more of an end-to-end scenario.
direct/tpm2/commit_test.go
Outdated
}, | ||
} | ||
|
||
_, err = commit.Execute(thetpm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new to ECDAA signing, but I think you should be able to use TPM2_Sign (which is already implemented) to produce an ECDAA signature using the commit value. You'll have to grab the commit counter from the Commit command and use it on the ECDAA scheme structure in the Sign command.
Bonus points: Call Sign again with the same counter value and expect it to fail :)
No need to validate the signature at this point, unless you really want to. I imagine that belongs as part of a larger body of ECDAA sample code that someone might work on (maybe you or someone else) as part of a separate effort.
I will update it as your review told me, on next week... (I'm too busy until next Saturday...) |
These deferred flushes define and deferred right after the resources that need to be freed are created.
@chrisfenner So could you re-review and merge it if it is ok? |
Thank you for the change! |
#287