-
Notifications
You must be signed in to change notification settings - Fork 71
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
add cloud log flag: add records to cloud logging when fetching token from attestation verifier #417
Conversation
/gcbrun |
/gcbrun |
5c46471
to
51895c1
Compare
/gcbrun |
/gcbrun |
/gcbrun |
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.
What about testing?
Also, please address the documentation comments on #375. Either here or another PR is fine.
/gcbrun |
4f37af4
to
2fa358c
Compare
/gcbrun |
/gcbrun |
/gcbrun |
var cloudLogger *logging.Logger | ||
if cloudLog { | ||
if audience == "" { | ||
return errors.New("cloud logging requires the --audience flag") |
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.
capitalize Cloud Logging, as it's a proper noun
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.
go-staticcheck would give "error strings should not be capitalized (ST1005)"
@@ -72,7 +72,12 @@ func TestTokenWithGCEAK(t *testing.T) { | |||
} | |||
defer mockAttestationServer.Stop() | |||
|
|||
RootCmd.SetArgs([]string{"token", "--algo", op.algo, "--output", secretFile1, "--asAddr", mockAttestationServer.server.URL}) | |||
mockCloudLoggingServerAddress, err = newMockCloudLoggingServer() |
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 would have expected you to check the loggingHandler.logs
output matches up.
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.
The attestation would give different ak_cert since we are using tpm simulator. Do you have a reference or recommendation on a deterministic TPM simulator output?
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 create a new PR with the loggingHandler.logs
output matches up. I use a simple field comparison for attestation. Please let me know if there is a better way!
Breaking Changes: [launcher/cmd] Refactor verifier for issue google#419 * Unexport `cmd.Instance`, `cmd.MetadataServer`, `cmd.NewMetadataServer`. * Move package `verifier` from launcher to go-tpm-tools. * `verifier.Client`, `verifier.Challenge`, etc. * Move package `fake` from launcher to go-tpm-tools. * `fake.Claims`, `fake.NewClient`, etc. * Move package `rest` from launcher to go-tpm-tools. * `rest.NewClient`, `rest.BadRegionError`, etc. New Features: [cmd] Add new command token in the CLI tool google#375 [cmd] add records to cloud logging when fetching token from attestation verifier google#417 Bug Fixes: Statically link binaries built by goreleaser google#425 Other Changes: Update readme to include the instruction to use the prebuilt gotpm tool. google#424 New Contributors: @Ruide in google#375 @qinkunbao in google#424
Breaking Changes: [launcher/cmd] Refactor verifier for issue google#419 * Unexport `cmd.Instance`, `cmd.MetadataServer`, `cmd.NewMetadataServer`. * Move package `verifier` from launcher to go-tpm-tools. * `verifier.Client`, `verifier.Challenge`, etc. * Move package `fake` from launcher to go-tpm-tools. * `fake.Claims`, `fake.NewClient`, etc. * Move package `rest` from launcher to go-tpm-tools. * `rest.NewClient`, `rest.BadRegionError`, etc. New Features: [cmd] Add new command token in the CLI tool google#375 [cmd] add records to cloud logging when fetching token from attestation verifier google#417 Bug Fixes: Statically link binaries built by goreleaser google#425 Other Changes: Update readme to gotpm CLi instructions. google#424, google#426 New Contributors: @Ruide in google#375 @qinkunbao in google#424
No description provided.