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

tmputil: introduce non-global encoding logic #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ericchiang
Copy link
Member

Right now it's not possible to use the tpm and tpm2 packages
simultaneously. Refactor the encoding logic into separate structs, so
using one doesn't impact the other, while maintaining the top level API.

As a follow up, refactor the tpm and tpm2 packages to use the encoders.
For example instead of:

tpmutil.Pack(digest)

Use:

tpmutil.TPM2.Pack(digest)

cc @awly

@awly
Copy link
Contributor

awly commented Apr 12, 2019

cc @twitchy-jsonp @DenisKarch
We were just talking about the same issue.

@twitchy-jsonp
Copy link
Contributor

twitchy-jsonp commented Apr 12, 2019

We mentioned this in the team chat and Eric mentioned he had a fix lying around for it. Thanks for digging it up!

LGTM. Happy to do the big refactor to switch all le methods into using the more specific encoder types.

@ericchiang
Copy link
Member Author

@twitchy-jsonp said you'd discussed introducing a new interface instead. If there are bigger issues with the encoding between 1.2/2.0 that need fixing, that seems like a better way to go.

This was intended to be as small a diff as possible. I imagine a follow up would look like:

diff --git a/tpm2/constants.go b/tpm2/constants.go
index ea954e7..eb16aba 100644
--- a/tpm2/constants.go
+++ b/tpm2/constants.go
@@ -25,9 +25,7 @@ import (
        "github.com/google/go-tpm/tpmutil"
 )
 
-func init() {
-       tpmutil.UseTPM20LengthPrefixSize()
-}
+var encoding = tpmutil.TPM2
 
 // MAX_DIGEST_BUFFER is the maximum size of []byte request or response fields.
 // Typically used for chunking of big blobs of data (such as for hashing or
diff --git a/tpm2/tpm2.go b/tpm2/tpm2.go
index 898f1de..dd61c79 100644
--- a/tpm2/tpm2.go
+++ b/tpm2/tpm2.go
@@ -37,7 +37,7 @@ func GetRandom(rw io.ReadWriter, size uint16) ([]byte, error) {
        }
 
        var randBytes []byte
-       if _, err := tpmutil.Unpack(resp, &randBytes); err != nil {
+       if _, err := encoding.Unpack(resp, &randBytes); err != nil {
                return nil, err
        }
        return randBytes, nil
@@ -53,7 +53,7 @@ func FlushContext(rw io.ReadWriter, handle tpmutil.Handle) error {
 
 func encodeTPMLPCRSelection(sel PCRSelection) ([]byte, error) {
        if len(sel.PCRs) == 0 {
-               return tpmutil.Pack(uint32(0))
+               return encoding.Pack(uint32(0))
        }
 
        // PCR selection is a variable-size bitmask, where position of a set bit is

tpmutil/encoding.go Outdated Show resolved Hide resolved
// UseTPM12LengthPrefixSize makes Pack/Unpack use TPM 1.2 encoding for byte
// arrays.
// UseTPM12LengthPrefixSize makes the package level Pack/Unpack functions use
// TPM 1.2 encoding for byte arrays.
func UseTPM12LengthPrefixSize() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to UseTPM12Encoding

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're okay with backward incompatible changes in tpmutil, should we just remove the global encoder?

tpmutil/encoding.go Outdated Show resolved Hide resolved
tpmutil/encoding_test.go Show resolved Hide resolved
@awly
Copy link
Contributor

awly commented Apr 12, 2019

@ericchiang there already is a new type for marshaling customization https://github.com/google/go-tpm/blob/master/tpmutil/structures.go#L54-L60
And at least one case where a byte slice in 1.2 is encoded differently than others https://github.com/google/go-tpm/blob/master/tpm/structures.go#L245-L287

I think both approaches will be equally noisy to implement. Since custom interface is needed regardless for that one use case, maybe we can cut out any knowledge of protocol version from tpmutil and instead let version-specific packages define byte slice encoding?

Right now it's not possible to use the tpm and tpm2 packages
simultaneously. Refactor the encoding logic into separate structs, so
using one doesn't impact the other, while maintaining the top level API.

As a follow up, refactor the tpm and tpm2 packages to use the encoders.
For example instead of:

	tpmutil.Pack(digest)

Use:

	var encoding = tpmutil.Encoding1_2
	encoding.Pack(digest)
@ericchiang
Copy link
Member Author

I think both approaches will be equally noisy to implement. Since custom interface is needed regardless for that one use case, maybe we can cut out any knowledge of protocol version from tpmutil and instead let version-specific packages define byte slice encoding?

Maybe internal packages that implement this encoding?

That sounds good to me. Though it's going to be a pain to have to iterate through every use of a []byte and replace it with the TPM 1.2 and TPM 2.0 wrappers.

@awly awly added the kokoro:run Enable presubmit tests for non-org members label Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kokoro:run Enable presubmit tests for non-org members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants