teeattestation: add Document.VerifyPCR for per-instance PCRs#2196
teeattestation: add Document.VerifyPCR for per-instance PCRs#2196nadahalli wants to merge 2 commits into
Conversation
Add Document.VerifyPCR(index, expected) and VerifyExpectedPCRs(map) so a caller can assert per-instance PCRs (e.g. PCR4, the SHA384 of the parent instance ID) that differ per enclave and therefore cannot go in the shared trusted measurements that ValidateAndParse checks. It rejects an empty expected value, an absent or length-mismatched PCR, and an all-zero PCR (debug-mode enclaves emit all-zero PCRs, which must never be accepted as a real measurement).
|
👋 nadahalli, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
✅ API Diff Results -
|
There was a problem hiding this comment.
Pull request overview
Adds per-instance PCR verification helpers to the parsed Nitro attestation Document, enabling callers to validate PCRs that vary per enclave instance (e.g., PCR4) after ValidateAndParse completes the shared-measurement checks.
Changes:
- Added
Document.VerifyPCR(index, expected)with checks for empty expected values, absent/mismatched PCRs, all-zero PCRs (debug-mode), and mismatches. - Added
Document.VerifyExpectedPCRs(map[uint][]byte)as a convenience wrapper to verify multiple PCRs. - Added unit tests covering success and common failure cases for the new APIs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/teeattestation/nitro/validate.go | Adds VerifyPCR, VerifyExpectedPCRs, and an allZero helper for per-instance PCR validation. |
| pkg/teeattestation/nitro/validate_test.go | Adds tests for VerifyPCR and VerifyExpectedPCRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (d *Document) VerifyExpectedPCRs(expected map[uint][]byte) error { | ||
| for index, value := range expected { | ||
| if err := d.VerifyPCR(index, value); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
…ch error Address review: sort PCR indices ascending in VerifyExpectedPCRs so the reported first failure is deterministic, and include expected/actual bytes in the PCR mismatch error (PCRs are measurements, not secrets), matching the existing PCR0/1/2 error style.
| // length-mismatched, the PCR is all zero (debug-mode enclaves emit all-zero | ||
| // PCRs, which must never be accepted as a real measurement), or the values | ||
| // differ. | ||
| func (d *Document) VerifyPCR(index uint, expected []byte) error { |
There was a problem hiding this comment.
Why aren't we just returning a generic []bytes ID field from the verifyMeasurements function and just pass the PCR4 through there? We are really gating ourselves into Nitro here. Although that probably doesn't matter, it's still quite specific.
There was a problem hiding this comment.
This is already inside teeattestation/nitro with cose parsing, etc. I think it should be ok at this level. If we ever move to proper TEEAL, we will need to rework a lot of other code. I'd rather do that then.
There was a problem hiding this comment.
How about when we move this code back into confidential-compute once it is open sourced?
There was a problem hiding this comment.
This code was previously very simple. It would be nice to revert to that.
There was a problem hiding this comment.
See the original PR for reference: https://github.com/smartcontractkit/confidential-compute/pull/293/changes#diff-822b5dfcbad5be1fed590586f99dd1ba4d0cba29c1b45761d03f659c2007cc94R13.
By simply extending the interface to include a []bytes field our consumers didn't have to dig into Nitro details.
What
Adds
Document.VerifyPCR(index, expected)and aVerifyExpectedPCRs(map)convenience on the parsed attestationDocument(fromValidateAndParse).Why
ValidateAndParsechecks PCR0/1/2 against the sharedtrustedMeasurements(image measurements, identical across the pool). Per-instance PCRs, notably PCR4 (the SHA384 of the parent instance ID), differ per enclave, so they can't live in those shared measurements and need a separate per-enclave comparison. This is the building block for binding an attestation to a specific host instance.VerifyPCRrejects:Scope
Pure addition, no behavior change to existing validation. Consumers (confidential-compute pool.go, and potentially the relay DON) will call this against a per-enclave expected PCR4 sourced from config.