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

Update TCB Info verification logic. #38

Merged
merged 17 commits into from
Feb 16, 2024
Merged

Update TCB Info verification logic. #38

merged 17 commits into from
Feb 16, 2024

Conversation

vbalain
Copy link
Collaborator

@vbalain vbalain commented Feb 13, 2024

Verifier fails to verify TCB on TDX 1.5. TCB info shows error when attempting to use the Intel PCS API service to verify the TDX quote when COLLATERAL is enabled.
Updated logic as per the latest documentation -
https://api.portal.trustedservices.intel.com/content/documentation.html#pcs-tcb-info-tdx-v4

Screenshot 2024-01-31 at 10 25 01

@vbalain vbalain requested a review from jrjatin February 13, 2024 16:41
@vbalain vbalain self-assigned this Feb 13, 2024
pcs/pcs.go Show resolved Hide resolved
verify/verify.go Outdated
return false
}
}
return true
}

func isTdxTcbSvnHigherOrEqual(teeTcbSvn []byte, tdxTcbcomponents []pcs.TcbComponent) bool {
start := 0
if teeTcbSvn[0] > 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As per documentation, if TeeTcbSvn[1] value is >= 1 then comparison should be done starting index 2 instead of 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per your comment you should be using teeTcbSvn[1] but in the code you have used teeTcbSvn[0] for the check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a testcase for this?
We have some sample API responses in testing/testdata which are failing collateral verification due to OutofDate status, you can add new API response and your changes should give UpToDate status.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But you'll need to add a new TDX quote. Will a quote generated on one of our machines be suitable? @ivishwesh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you add a testcase for this? We have some sample API responses in testing/testdata which are failing collateral verification due to OutofDate status, you can add new API response and your changes should give UpToDate status.

I didn't see any failure in tests. How did you confirm the failure, can you elaborate on it?

Copy link
Collaborator

@jrjatin jrjatin Feb 15, 2024

Choose a reason for hiding this comment

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

While writing tests earlier we were having a quote(testing/testdata/tdx_prod_quote_SPR_E4.dat) which was OutofDate acc. to TcbInfo API response so I added a testcase which expects failure. Now you can add a new quote and a new sample API response(in testdata) and your changes should return status as UpToDate.
(@ivishwesh can confirm whether we can add a new TDX quote in testdata generated from our mach)

wantErr = `TCB Status is not "UpToDate", found "OutOfDate"`

Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed we will add new quote and unit tests in a following PR.

tools/check/check.go Outdated Show resolved Hide resolved
tools/check/check_test.go Outdated Show resolved Hide resolved
tools/check/check.go Outdated Show resolved Hide resolved
tools/check/check_test.go Outdated Show resolved Hide resolved
verify/verify.go Show resolved Hide resolved
verify/verify.go Show resolved Hide resolved
verify/verify.go Outdated Show resolved Hide resolved
verify/verify.go Outdated Show resolved Hide resolved
verify/verify.go Outdated Show resolved Hide resolved
verify/verify.go Outdated Show resolved Hide resolved
@vbalain vbalain merged commit 43efd1f into google:main Feb 16, 2024
6 checks passed
@jrjatin jrjatin mentioned this pull request Feb 19, 2024
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.

None yet

3 participants