8000 Add client.AttestOpts by josephlr · Pull Request #117 · google/go-tpm-tools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add client.AttestOpts #117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 25, 2021
Merged

Add client.AttestOpts #117

merged 3 commits into from
Jun 25, 2021

Conversation

josephlr
Copy link
Member

This will allow us to add functionality to client.Atttest in a backwards compatible way.

Note that like many similar Go interfaces (e.g. PublicKey, PrivateKey, DecrypterOpts, etc...) this AttestOpts interface has no methods. This just means we have to use casts to switch functionality, rather than using common methods.

Also, rename SealOpt to SealOpts and CertifyOpt to CertifyOpts. This is the more consistent naming scheme.

josephlr added 3 commits June 24, 2021 18:14
@josephlr
client: remove field name SealTarget and CertifyExpected
d5d2211 
This name is unnecessary and is inconsistent with how we handle other
options structures

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Rename {Certify,Seal}Opt -> {Certify,Seal}Opts
195c502 
This is more consistent with Golang naming.

Also, update the variable names to also be plural.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
client: Add AttestOpts
a05e195 
This will allow us to add options for attestation in the future. Right
now the AttestOpts must be nil (the default).

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr josephlr requested review from iKevinY, alexmwu and jkl73 June 25, 2021 01:24
iKevinY
iKevinY approved these changes Jun 25, 2021
View reviewed changes
Copy link
@iKevinY iKevinY left a comment

Choose a reason for hiding this comment

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

One pattern I've noticed in my (limited) experience with Go is methods taking in a opts ...FooOpt as the last parameter to allow for a variadic number of option objects to be passed in. Does that makes sense here in the context of each of the XOpts in go-tpm-tools rather than pluralizing all of the option structs?

If the current approach is preferable, then LGTM.

@josephlr
Copy link
Member Author
josephlr commented Jun 25, 2021 •
edited
Loading

One pattern I've noticed in my (limited) experience with Go is methods taking in a opts ...FooOpt as the last parameter to allow for a variadic number of option objects to be passed in. Does that makes sense here in the context of each of the XOpts in go-tpm-tools rather than pluralizing all of the option structs?

Ya I've seen this too, usually under the name "Functional Options" (see this Blog post by Rob Pike). Basically, each opt object in the variadic opts can configure the server, allowing you to feed in many options (or none). This is a good pattern when you have a bunch of orthogonal options and it makes sense to allow them to be specified independently.

However, this isn't the best pattern if:

  • you only want one or zero options
  • specifying multiple options would conflict

This is the case for SealOpts and CertifyOpts (e.g. CertifyCurrent and CertifyExpected would conflict). The Go crypto package has DecrypterOpts which works similarly. There is also the issue with Key.Reseal which takes both sealing and certifying parameters, so it might be hard to implement that using variadic functions.

Our eventual VerifyOpts will also work similarly, a user will only specify one way to get/check the PublicKey (nil won't be allowed in that case). I think the case for AttestOpts is less clear; we currently only have two use cases in mind:

  • nil: Don't do anything extra
  • GetGCEMetadata{ *metadata.Client }: Use the metadata.Client to retrieve the additional GCE metadata.

So I think using Opts instead of Opt is the right move for now (for consistency). But this is a really good point to bring up. We should see how the code evolves as we add more functionality. It may turn out we like the variadic approach better and will switch to it with an eventual v0.4 or v1.0.

@josephlr josephlr merged commit c82ef44 into google:master Jun 25, 2021
@josephlr josephlr deleted the opts branch June 25, 2021 07:58
@iKevinY
Copy link
iKevinY commented Jun 25, 2021

Makes sense; I wasn't sure how each of the options would interact with one another or potentially expand in the future (on the point of orthogonality), so thanks for clarifying that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iKevinY iKevinY iKevinY approved these changes

@jkl73 jkl73 jkl73 approved these changes

@alexmwu alexmwu Awaiting requested review from alexmwu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@josephlr @iKevinY @jkl73
0