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

Supporting functionalities for direct migration #283

Merged
merged 2 commits into from
Jun 29, 2022

Conversation

matt-tsai
Copy link
Contributor

@matt-tsai matt-tsai commented Jun 10, 2022

Main changes include: ReadWrite interface for transport TPMs, adding util to grab HandleValue.

@matt-tsai matt-tsai requested a review from a team as a code owner June 10, 2022 21:50
direct/transport/tpm.go Outdated Show resolved Hide resolved
Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

One idea worth considering here is which direction we want to support conversion for the TPM types. We can support:

  1. Using the new TPM type (transport.TPM) with the old TPM interface (io.ReadWriter/io.ReadWriteCloser) and library.
  2. Using the old TPM type (io.ReadWriter) with the new TPM interface (transport.TPM) and library

This PR implements (2), but I'm wondering if we should implement (1) instead.

direct/tpm2/tpm2.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
@matt-tsai
Copy link
Contributor Author

One idea worth considering here is which direction we want to support conversion for the TPM types. We can support:

  1. Using the new TPM type (transport.TPM) with the old TPM interface (io.ReadWriter/io.ReadWriteCloser) and library.
  2. Using the old TPM type (io.ReadWriter) with the new TPM interface (transport.TPM) and library

This PR implements (2), but I'm wondering if we should implement (1) instead.

This PR implements 2 because the old tests still need RWC and the Direct Migration needs a RW in order to be made a transport tpm used for Execute. Correct me if I'm wrong but eventually we will only have 1 interface.
I think the option (option 2 in this case) where we change RW into transport TPMs might be wiser as it would require less changes to direct and would also be backwards compatible.

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

@matt-tsai let us know when this is ready for review again (and the TODO comments are cleaned up).

direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/open_other.go Show resolved Hide resolved
direct/tpm2/tpm2.go Show resolved Hide resolved
Added and changed names of private Types and return types of OpenTPM.
Copy link
Member

@chrisfenner chrisfenner left a comment

Choose a reason for hiding this comment

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

Thanks for the change, looks great. Only teeny nits

direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
direct/transport/tpm.go Outdated Show resolved Hide resolved
Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

Other than the Read implementation, LGTM

@josephlr josephlr merged commit 08548c1 into google:tpmdirect Jun 29, 2022
@josephlr
Copy link
Member

Nice work on this!!!

@matt-tsai matt-tsai deleted the signerMigration branch July 12, 2022 20:15
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.

4 participants