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! |
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 🤷
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. |
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.
eef5bc4 to
af5248c
Compare
|
|
||
| // DefaultCARoots is the AWS Nitro Enclaves root certificate. | ||
| // Downloaded from: https://aws-nitro-enclaves.amazonaws.com/AWS_NitroEnclaves_Root-G1.zip | ||
| const DefaultCARoots = "-----BEGIN CERTIFICATE-----\nMIICETCCAZagAwIBAgIRAPkxdWgbkK/hHUbMtOTn+FYwCgYIKoZIzj0EAwMwSTEL\nMAkGA1UEBhMCVVMxDzANBgNVBAoMBkFtYXpvbjEMMAoGA1UECwwDQVdTMRswGQYD\nVQQDDBJhd3Mubml0cm8tZW5jbGF2ZXMwHhcNMTkxMDI4MTMyODA1WhcNNDkxMDI4\nMTQyODA1WjBJMQswCQYDVQQGEwJVUzEPMA0GA1UECgwGQW1hem9uMQwwCgYDVQQL\nDANBV1MxGzAZBgNVBAMMEmF3cy5uaXRyby1lbmNsYXZlczB2MBAGByqGSM49AgEG\nBSuBBAAiA2IABPwCVOumCMHzaHDimtqQvkY4MpJzbolL//Zy2YlES1BR5TSksfbb\n48C8WBoyt7F2Bw7eEtaaP+ohG2bnUs990d0JX28TcPQXCEPZ3BABIeTPYwEoCWZE\nh8l5YoQwTcU/9KNCMEAwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUkCW1DdkF\nR+eWw5b6cp3PmanfS5YwDgYDVR0PAQH/BAQDAgGGMAoGCCqGSM49BAMDA2kAMGYC\nMQCjfy+Rocm9Xue4YnwWmNJVA44fA0P5W2OpYow9OYCVRaEevL8uO1XYru5xtMPW\nrfMCMQCi85sWBbJwKKXdS6BptQFuZbT73o/gBh1qUxl/nNr12UO8Yfwr6wPLb+6N\nIwLz3/Y=\n-----END CERTIFICATE-----\n" |
There was a problem hiding this comment.
according to the docs nitrite package already contains root CA cert
There was a problem hiding this comment.
as in the followup comment - when having root hard-coded it would be even better to avoid this import at all
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/hf/nitrite" |
There was a problem hiding this comment.
nitrite package seems suspicious - it's like 400 lines of code in total made by two contributors; does not seems like a rocket science what it does. maybe it could be entirely avoided?
There was a problem hiding this comment.
Given all the recent supply chain attacks. I am inclined to agree. Let me see if I can rewrite the entire thing from scratch, or worst case, copy the whole thing locally.
There was a problem hiding this comment.
thx. if you trust what they did you can fork it internally (that's on MIT), but imho such a short code that maybe not even worth it. also afaik it's mostly copy&pasta from aws page with attestation verification examples anyway ..
There was a problem hiding this comment.
They also rely on https://github.com/fxamacker/cbor - which is quite an active repo. So, we need to pin that to some known version. If we do that, we might as well just pin nitrite as well, no?
There was a problem hiding this comment.
The Nitro attestation verification path now lives in-tree under pkg/teeattestation/nitro/, so we no longer depend on the nitrite package. We now do COSE Sign1 parsing, CBOR decoding, cert-chain verification against the configured roots, etc.
I kept github.com/fxamacker/cbor/v2 as the only external parsing dependency and pinned it explicitly in go.mod to v2.9.0.
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