Add teeattestation package for TEE attestation validation#1899
Add teeattestation package for TEE attestation validation#1899
Conversation
|
👋 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! |
nadahalli
force-pushed
the
tejaswi/tee-attestation
branch
from
0ea8118 to
2edce5f
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new teeattestation package providing platform-agnostic domain-separated hashing and AWS Nitro Enclave attestation validation, along with a fake attestor for testing.
Changes:
- Introduces
pkg/teeattestation/hash.gowithDomainHashfor domain-separated SHA-256 hashing - Adds
pkg/teeattestation/nitro/validate.gowithValidateAttestationfor verifying AWS Nitro attestation documents against expected PCRs and user data - Adds
pkg/teeattestation/nitro/fake/with aFakeAttestorthat produces COSE Sign1 documents verifiable without real Nitro hardware
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/teeattestation/hash.go | Domain-separated SHA-256 hashing primitive |
| pkg/teeattestation/hash_test.go | Tests for DomainHash |
| pkg/teeattestation/nitro/validate.go | AWS Nitro attestation validation with PCR and user data checks |
| pkg/teeattestation/nitro/fake/fake.go | Fake attestor producing valid COSE Sign1 docs for testing |
| pkg/teeattestation/nitro/fake/fake_test.go | Tests for FakeAttestor |
| go.mod | Adds github.com/hf/nitrite dependency |
Comments suppressed due to low confidence (1)
pkg/teeattestation/nitro/validate.go:1
- The
cosePayloadstruct has anUnprotectedfield of typecbor.RawMessage, but it is not set when constructingouterinfake.go(line 147). CBORnil/zero-value forRawMessagemay serialize differently than the empty CBOR map ({}) that COSE Sign1 expects for the unprotected header. Ifnitrite.Verifyis strict about this, it could fail. This comment applies tofake.goline 147, referencing the struct in the same file.
// Package nitro provides AWS Nitro Enclave attestation validation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/teeattestation/nitro/validate.go
Outdated
|
|
||
| // ValidateAttestation verifies an AWS Nitro attestation document against | ||
| // expected user data and trusted PCR measurements. | ||
| func ValidateAttestation(attestation, expectedUserData, trustedMeasurements []byte, caRootsPEM string) error { |
There was a problem hiding this comment.
Intentional. Empty string means 'use production default'. All existing callers pass empty string for production and a non-empty PEM for tests. Changing this would break the ergonomics for no practical gain.
| "pcr1": hex.EncodeToString(f.pcrs[1]), | ||
| "pcr2": hex.EncodeToString(f.pcrs[2]), | ||
| } | ||
| b, _ := json.Marshal(m) |
There was a problem hiding this comment.
Added a comment. Changing the signature would break callers for an error that can't happen (json.Marshal on map[string]string).
There was a problem hiding this comment.
Alternatively, since it is simple and fixed, you could use fmt.Sprintf 🤷
nadahalli
force-pushed
the
tejaswi/tee-attestation
branch
2 times, most recently
from
f9dede0 to
2c727f6
Compare
pkg/teeattestation/nitro/validate.go
Outdated
|
|
||
| // ValidateAttestation verifies an AWS Nitro attestation document against | ||
| // expected user data and trusted PCR measurements. | ||
| func ValidateAttestation(attestation, expectedUserData, trustedMeasurements []byte, caRootsPEM string) error { |
There was a problem hiding this comment.
nit: rename to ValidateNitroAttestation
There was a problem hiding this comment.
It's inside the nitro folder. That should be enough, no?
There was a problem hiding this comment.
Ah, yeah. Forgot about that.
|
Would be nice to put this behind a /privacy/ or /confidential-compute/ root package and mark that thing as being Privacy codeowners. That way we can maintain better autonomy for approvals. |
Let's add privacy as the owner of this folder. I don't want to add a privacy specific top-level package. |
sure |
|
LGTM, will ship when the linting is fixed. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 767c996 | Previous: c7b8c13 | Ratio |
|---|---|---|---|
BenchmarkKeystore_Sign/nop/in-process |
753.4 ns/op |
356.1 ns/op |
2.12 |
This comment was automatically generated by workflow using github-action-benchmark.
nadahalli
force-pushed
the
tejaswi/tee-attestation
branch
from
eef5bc4 to
af5248c
Compare
nadahalli
force-pushed
the
tejaswi/tee-attestation
branch
from
51c7387 to
76bf69e
Compare
Summary
pkg/teeattestation/with platform-agnostic domain-separated hashing (DomainHash)pkg/teeattestation/nitro/with AWS Nitro attestation validation (ValidateAttestation, PCR types, default CA roots)pkg/teeattestation/nitro/fake/withFakeAttestorfor testing without Nitro hardwaregithub.com/hf/nitrite(Nitro attestation verifier)Nitro-specific code is isolated in
nitro/so GCP Confidential Computing etc. can be added as sibling packages later.Used by both
confidential-compute(enclave-side) andchainlink(relay DON handler) for attestation validation and domain-separated hashing. It is an almost direct copy from https://github.com/smartcontractkit/confidential-compute/tree/main/enclave-client/attestation-validator