Add attest and debugVerify command to gotpm #293
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
LGTM. Seems like you picked the newer commits after your initial commits, so this PR is showing some commits already in the master/main.
Make sure you rebase it correctly so only new commits are showing up in this PR, and there is no merge conflict.
https://stackoverflow.com/questions/9790448/how-to-update-a-pull-request-from-forked-repo
https://git-scm.com/book/en/v2/Git-Branching-Rebasing
Pranjali-2501
force-pushed
the
attest-cli
branch
from
0925377 to
ade02d3
Compare
|
Reopen this PR to merge attest-cli in go-tpm-tools. |
There was a problem hiding this comment.
Thanks for the update! Mostly nits.
Not sure what IDE you're using, but try to run "gofmt -s -w .go" to fix those lint issues (linter left some comments, you can see those here https://github.com/google/go-tpm-tools/pull/293/files)
Don't worry about the "confidential-space-image-presubmit-tests" check.
cmd/attest.go
Outdated
| --algo flag override the public key algorithm for attestation key. If not provided then | ||
| by default rsa is used. | ||
| --nonce attaches even length extra data to the report and guarantees a fresh quote. | ||
| --teenonce attaches a 64 bytes extra data to the attestation report of hardware and |
There was a problem hiding this comment.
64 bytes is just a coincidence that both TDX and SEV-SNP have the same length for their report_data field. A different TEE technology could have a different length.
There was a problem hiding this comment.
For now TDX and SEV-SNP have 64 byte report data feild.
So we will add comments in cmd/attest.go stating
" If hardware technology needs a variable length teenonce then please modify the flags description"
and change the description
"--teenonce attaches a 64 bytes extra data to the attestation report of TDX and SEV-SNP
hardware and guarantees a fresh quote."
cmd/attest.go
Outdated
| key or AK for a custom attestation key. By default it uses AK. | ||
| --algo flag override the public key algorithm for attestation key. If not provided then | ||
| by default rsa is used. | ||
| --nonce attaches even length extra data to the report and guarantees a fresh quote. |
There was a problem hiding this comment.
even length? I thought TPM 2.0 used 48 bytes. The "fresh quote" claim is only a guarantee if the user is providing a true nonce. It's not a claim that this library can make on its own.
There was a problem hiding this comment.
Yes, but if we give an odd length nonce , then it gives an error stating "Error: invalid argument "12345" for "--nonce" flag: encoding/hex: odd length hex string" .
Because it try to do hex-encoding while taking input in --nonce flag.
func addNonceFlag(cmd *cobra.Command) {
cmd.PersistentFlags().BytesHexVar(&nonce, "nonce", []byte{}, "hex encoded nonce for vTPM attestation, cannot be empty")
}
There was a problem hiding this comment.
Sure, but I think hex encoding implies the string should be even length. Even length data that it encodes is my interpretation of the text. Does the nonce not need to be 48 bytes long? If not, please describe the zero-filling/truncation behavior in the description.
There was a problem hiding this comment.
Nonce does not need to be 48 bytes long.
Do you want me to implement a zero filling behavior if nonce is of odd length?
There was a problem hiding this comment.
The answer here is "it depends". Specifically on the size of TPM2B_DATA, which is TPM2 implementation specific, since it also depends on the max name size. The upper bound on size IIRC is a 2byte hash identifier and the size of the digest for the hash function. Since TPM2s are required to support SHA256, we get a lower bound of 2+(256/8)=34. However, many TPMs support larger hash algorithms, like SHA512 which would yield 66 bytes. You can get the size by calling TPM2_GetCapability and checking TPM2_PT_MAX_DIGEST
This discussion should be moot though since we removed the nonce help text in attest in favor of the default flag help text.
| attestOpts.Nonce = nonce | ||
| if len(teeNonce) != 0 { | ||
| attestOpts.TEENonce = teeNonce | ||
| attestOpts.TEEDevice, err = client.CreateSevSnpDevice() |
There was a problem hiding this comment.
getTeeAttestationReport will choose the device if TEEDevice is not set, so I don't think you need to make that default decision here.
There was a problem hiding this comment.
In getTeeAttestationReport , there is a check
// TEEDevice can't be nil while TEENonce is non-nil
if opts.TEENonce != nil {
return fmt.Errorf("got non-nil TEENonce when TEEDevice is nil: %v", opts.TEENonce)
}
And if the user provides a TEENonce flag , then it always fails that check because TEEDevice is nil.
There was a problem hiding this comment.
Perhaps we should just default to using the nonce as the teenonce (and then remove the option). That's what client.Attest does if it successfully creates an SNP device but doesn't get a TEENonce.
Or, we can add a new flag requesting the hardware type (hwattestation?) as part of requesting hardware attestation. And teenonce would fail without that flag.
Either way, we need to be forwards compatible with new technologies like TDX.
There was a problem hiding this comment.
It was my intention for teenonce to take the value of nonce if teenonce is not populated.
| } | ||
| } | ||
|
|
||
| func TestAttestPass(t *testing.T) { |
There was a problem hiding this comment.
Any test for SEV-SNP simulation? Note there is a testclient device from go-sev-guest that will select hardware or fake based on the device path.
There was a problem hiding this comment.
Thank you @deeglaze .
I have added tests for SEV-SNP in attest_test.go .
cmd/attest_test.go
Outdated
| } | ||
| for _, op := range tests { | ||
| t.Run(op.name, func(t *testing.T) { | ||
| if len(op.teenonce) == 0 { |
There was a problem hiding this comment.
If the length is zero, copying anything into it will still result in a length 0 array.
You'd need to make a nonce of the expected length, say make([]byte, abi.ReportDataSize) from go-sev-guest's abi library before copying into it.
There was a problem hiding this comment.
But given that the test case type is [64]byte, len(op.teenonce) will never equal zero. You're just not populating the teenonce.
There was a problem hiding this comment.
Yes , I have put a wrong check . It will never pass. I will remove this check.
|
/gcbrun |
New Features: Add attest and verify command to gotpm google#293 Add tee_technology flag and test for tee_technology flag google#307 * intra-release breaking change Other Changes: Add OS Policy assignment tests for both debug and hardened. google#301 Add a wrapper for ExternalTPM google#302 Update to go-sev-guest v0.6.0 google#304 Update base image family to use cos-dev google#306 Update go-sev-guest to v0.6.1 google#308
New Features: Add attest and verify command to gotpm #293 Add tee_technology flag and test for tee_technology flag #307 * intra-release breaking change Other Changes: Add OS Policy assignment tests for both debug and hardened. #301 Add a wrapper for ExternalTPM #302 Update to go-sev-guest v0.6.0 #304 Update base image family to use cos-dev #306 Update go-sev-guest to v0.6.1 #308
The attest command generates an attestation report containing the
quote, attestation key and TCG event log. Users can specify the type
of key used (GCE or go-tpm-tools generated attestation key) , random
value for nonce, name of files that contains text format and binary
format of attestation report, TEEDevice and TEENonce for hardware
attestation report, GCE instance and project Metadata (if attestation
key is of GCE type) .
The debugVerify command uses a binary file that stores the attestation
report in binary format, nonce and output file to store verified machine
state of the attestation report.