Various fixes for Remote Attestation implementations #235

zaolin · 2021-02-25T15:02:20Z

Summary:

Fix the wrong usage of signature decoding by using DecodeSignature function (Remote Attestation)
Fix decodeCertify for wrong signature decoding
Make EncodeCreationData public
Introduce OutsideInfo to key creation for key labels
Improve tpmutil runner with retries to stabilize virtualized TPMs usage

@googlebot

@googlebot I fixed it. Signed-off-by: Philipp Deppenwiese <zaolin.daisuki@gmail.com>

chrisfenner · 2021-03-01T20:42:54Z

Hi zaolin, I apologize in advance for my slow response time as it is a busy week for me. I would be happy to review this PR and I'm targeting getting back to you by 2021-03-05.

zaolin · 2021-03-01T23:23:20Z

Hi @chrisfenner, no problem at all! :)

tpm2/structures.go

twitchy-jsonp · 2021-03-03T20:51:48Z

tpm2/structures.go

@@ -827,7 +845,8 @@ type CreationData struct {
 	OutsideInfo         tpmutil.U16Bytes
 }

-func (cd *CreationData) encode() ([]byte, error) {
+// EncodeCreationData encodes byte array to TPMS_CREATION_DATA message.
+func (cd *CreationData) EncodeCreationData() ([]byte, error) {


Why does this need to be exported?

(Not saying its wrong, but would like to understand the use-case. go-attestation package has been able to handle creation data by simply keeping it as bytes and only decoding when necessary, rather than doing round-trips through encode/decode).

We use it to generate TPMS_CREATE_DATA structures for various tests.

Also, the DecodeCreationData is already public. :)

twitchy-jsonp · 2021-03-03T20:55:57Z

tpmutil/run.go

-	if _, err := rw.Write(inb); err != nil {
-		return nil, 0, err
-	}
+	// f(t) = (2^t)ms, up to 2s


This is awesome :D

zaolin · 2021-03-05T14:24:06Z

@twitchy-jsonp Any update. Does it look good to go now?

chrisfenner

This change looks good to me, thank you @zaolin for all these fixes.

Unfortunately since this looks like a breaking change, we will need to strategize about how to merge. @twitchy-jsonp, @josephlr, do you think we should rev a version number?

we could introduce another flavor of the Certify and CertifyCreation commands that returns the whole signature. CertifyCreationEx and CertifyEx2? 🤢

tpm2/structures.go

tpm2/tpm2.go

twitchy-jsonp · 2021-03-08T17:38:41Z

I'm okay with just bumping a minor version number for this and asking users to make the (small) adjustment.

chrisfenner

This change looks good to me. If @twitchy-jsonp is OK merging as-is releasing a new minor version, then I support that plan (as opposed to some solution that involves either defining new "fixed" functions)

tomoveu · 2021-03-31T16:35:59Z

I just found out about this change and I have a question.

@chrisfenner Is this a "breaking change" or a "small adjustment"?

Because I thought go-tpm maintains API compatibility. And in my opinion, breaking existing API is a major thing.

@zaolin adding new API CreateKeyWithOutsideInfo that calls create with the new argument does not help. Because the underlying create now brakes API compatibility.

chrisfenner · 2021-03-31T17:31:56Z

Hey @tomoveu - Sorry for the inconvenience! I hope I can help.

First, a note about versioning and compatibility.
In general, go-tpm is on semantic major version 0 which means that from time to time, we have to make changes that break the existing API. Whether we use the words "breaking change" or "small adjustment", there's unfortunately change, so we don't do it except when needed (for example, to keep from having to create a lot of new flavors of existing APIs). I'd recommend depending on a specific released version of go-tpm, rather than HEAD of go-tpm main/master branch, so that such changes only affect you when you actively update dependencies, rather than transparently at inconvenient times.

The breaking API change
As far as I can see, there is no API compatibility issue with the public Create functions. Are you depending on the private create function, which took a refactor in this change? The breaking I was referring to was that code that depends on Certify* functions needs to deserialize the second parameter differently, into the structure that maps to TPMT_SIGNATURE (via DecodeSignature) instead of a raw RSA/ECC signature with scheme and hash algorithm unknown, to be passed directly to something like ecdsa.Verify. I'd be very interested to know if this change causes problems for users of public Create* APIs.

PolicySecret now returns three values (google/go-tpm#237) We never read the encoded value from Certify, so we are not affected by: google/go-tpm#235 Signed-off-by: Joe Richey <joerichey@google.com>

zaolin requested a review from a team as a code owner February 25, 2021 15:02

zaolin force-pushed the fix-attestation branch 10 times, most recently from 0bbd3af to e8dd2fc Compare February 27, 2021 13:03

zaolin changed the title ~~Improve access to data structures for remote attestation implementations~~ Various fixes Feb 27, 2021

zaolin force-pushed the fix-attestation branch from e8dd2fc to 83159d3 Compare February 27, 2021 13:32

Make EncodeCreationData accessible, introduce CreateKey labels

ecde583

@googlebot I fixed it. Signed-off-by: Philipp Deppenwiese <zaolin.daisuki@gmail.com>

zaolin force-pushed the fix-attestation branch from 83159d3 to ecde583 Compare February 27, 2021 13:38

zaolin assigned josephlr, twitchy-jsonp and chrisfenner and unassigned josephlr, twitchy-jsonp and chrisfenner Feb 27, 2021

zaolin changed the title ~~Various fixes~~ Various fixes for Remote Attestation implementations Feb 27, 2021

google deleted a comment from google-cla bot Feb 27, 2021

zaolin assigned josephlr, twitchy-jsonp and chrisfenner Feb 28, 2021

twitchy-jsonp suggested changes Mar 3, 2021

View reviewed changes

Update structures.go

3b23343

chrisfenner linked an issue Mar 5, 2021 that may be closed by this pull request

Certify, CertifyEx, and CertifyCreation return partial signature data #236

Closed

chrisfenner reviewed Mar 5, 2021

View reviewed changes

tpm2/structures.go Outdated Show resolved Hide resolved

tpm2/tpm2.go Show resolved Hide resolved

Update structures.go

8ac9477

chrisfenner approved these changes Mar 11, 2021

View reviewed changes

twitchy-jsonp approved these changes Mar 11, 2021

View reviewed changes

zaolin merged commit 4347b8d into master Mar 12, 2021

zaolin deleted the fix-attestation branch March 12, 2021 21:00

jkl73 mentioned this pull request Feb 9, 2022

Bump go-tpm version google/go-attestation#264

Merged

josephlr mentioned this pull request Feb 15, 2022

Update dependancies google/go-tpm-tools#163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes for Remote Attestation implementations #235

Various fixes for Remote Attestation implementations #235

zaolin commented Feb 25, 2021 •

edited

Loading

chrisfenner commented Mar 1, 2021

zaolin commented Mar 1, 2021

twitchy-jsonp Mar 3, 2021

flanfly Mar 4, 2021

zaolin Mar 4, 2021

twitchy-jsonp Mar 3, 2021

zaolin commented Mar 5, 2021

chrisfenner left a comment

twitchy-jsonp commented Mar 8, 2021

chrisfenner left a comment

tomoveu commented Mar 31, 2021

chrisfenner commented Mar 31, 2021

Various fixes for Remote Attestation implementations #235

Various fixes for Remote Attestation implementations #235

Conversation

zaolin commented Feb 25, 2021 • edited Loading

chrisfenner commented Mar 1, 2021

zaolin commented Mar 1, 2021

twitchy-jsonp Mar 3, 2021

Choose a reason for hiding this comment

flanfly Mar 4, 2021

Choose a reason for hiding this comment

zaolin Mar 4, 2021

Choose a reason for hiding this comment

twitchy-jsonp Mar 3, 2021

Choose a reason for hiding this comment

zaolin commented Mar 5, 2021

chrisfenner left a comment

Choose a reason for hiding this comment

twitchy-jsonp commented Mar 8, 2021

chrisfenner left a comment

Choose a reason for hiding this comment

tomoveu commented Mar 31, 2021

chrisfenner commented Mar 31, 2021

zaolin commented Feb 25, 2021 •

edited

Loading