feat(tee): implement SEV-SNP and TDX collectors over configfs-TSM - #86
Conversation
Both providers had the defect agentrust-io#74 fixed for TPM: detect() returned True wherever the platform's device node existed while attest() raised unconditionally, so on a bare-metal SNP or TDX guest the provider was selected and then failed, with an error claiming the platform was absent on a machine that had it. BaseProvider states that pair must agree, and two of three hardware providers broke it. Detection also missed Azure entirely. Azure runs SEV-SNP behind a Hyper-V paravisor, so the guest sees no /dev/sev-guest; on the very CVM this project ran its attested-peer validation on, SevSnpProvider.detect() returned False. Azure stays out of scope for this collector, since a paravisor-mediated guest cannot set REPORT_DATA, but attest() now says so and points at the vTPM path instead of reporting a generic absence. Collection goes through the kernel configfs-TSM interface, one interface for both platforms, superseding the per-platform ioctls. Neither collector has run on real silicon; they are exercised against a simulated configfs tree and synthetic reports, and the docs say so. Also defines the key-and-nonce binding for SEV-SNP and TDX, which only TPM had, so the other two had no way to commit the offered channel key and their verifiers' expected_report_data had nothing to compare against. The derivation is now shared by all three providers with a per-platform prefix. Signed-off-by: Susan Poudel <susanpdl77@gmail.com>
a9fe79d to
af40953
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
imran-siddique
left a comment
There was a problem hiding this comment.
Approving. I rebased this onto current main myself (the conflict was docs/hardware-validation.md, where #88 had corrected the not-yet-validated list and this branch added collector entries; both are kept). Verified locally on the rebased head: 308 passed, 3 skipped, ruff clean.
Three things I want on the record as read rather than skimmed.
derive_binding length-prefixes each field instead of delimiting, and the test proves the case it defends: ("a|b", "c") and ("a", "b|c") must not collide. Since nonce is caller-supplied, that is reachable rather than theoretical.
collect_report using a fresh entry name per call is the right instinct, and the docstring names the race a fixed name creates: two concurrent collectors share one entry, the second inblob write changes the report the first is about to read, and a peer ships a report committing someone else's key.
Dropping auxblob on the SNP side rather than passing AMD's GUID-tagged DER table off as attestation_key_chain_pem is the honest call, and keeping the PCK chain on the TDX side because it genuinely is inside the quote is the right asymmetry.
One non-blocking follow-up: in collect_report, outblob is read before provider is checked. Reading outblob is what makes the platform generate and sign the report, so on a mismatched provider a signed report over the caller's binding is produced and then discarded. Nothing is returned and the entry is removed, so I do not think it is exploitable, but reading provider first costs nothing.
Keeping LIMITATIONS.md and docs/hardware-validation.md honest about neither collector having run on real silicon is the reason this can merge at all.
Reading outblob is what makes the platform generate and sign a report, so checking the provider afterwards meant a mismatched guest signed a report over the caller's binding which was then discarded. Nothing was returned and the entry was removed either way, so this was not a disclosure, but it asked the hardware to sign something no one could use. The provider check now gates the read, and a test asserts outblob is never read on a mismatch so the ordering cannot quietly regress. The fake configfs tree now generates the report on outblob read rather than on inblob write, which is the kernel's actual sequence and is what makes that assertion mean anything. Follow-up requested on review of #86. Signed-off-by: Susan Poudel <susanpdl77@gmail.com>
Gives SEV-SNP and TDX the treatment #74 gave TPM. Part of #47 (Tier 3).
The bug
BaseProviderstates the contract plainly: "a provider returns True fromdetectonly whereattestcan actually produce evidence on that host. Returning True and then raising is the one combination to avoid, because the provider gets selected and then fails."Two of the three hardware providers broke it.
SevSnpProvider.detect()returned True whenever/dev/sev-guestexisted, andattest()raised unconditionally, so on a bare-metal SNP guest the provider was selected and then failed, with an error claiming no SEV-SNP guest was present on a machine that is one. TDX was identical with/dev/tdx_guest. That is exactly the defect #73 described and #74 fixed for TPM.Detection also missed Azure entirely. Azure runs SNP behind a Hyper-V paravisor, so the guest sees no
/dev/sev-guest.docs/hardware-validation.mdrecords this for theStandard_DC2ads_v5used in the July 27 attested-peer run ("no/dev/sev-guest, the expected paravisor shape"), which meansSevSnpProvider.detect()returned False on the very machine where cA2A proved the flow works.This has been latent because nothing in
src/calleddetect(). Only tests did. #52 addsselect_provider, which is the first production consumer, so this stops being latent as soon as that lands.What changed
Collection goes through the kernel configfs-TSM interface (
/sys/kernel/config/tsm/report, Linux 6.7+). One interface serves both platforms, differing only in the provider name, soca2a_runtime.tee.tsmis shared rather than copied per provider. It supersedes the per-platform ioctls.detect()now probes what its own collector needs, and where a host cannot collect,attest()names the missing piece.Azure stays out of scope for the SNP collector, because a paravisor-mediated guest cannot set
REPORT_DATAat all (the paravisor binds the vTPM AK there).attest()says that and points at the vTPM path, rather than reporting a generic absence.One detail worth a second opinion: the entry name is unique per call. A fixed name is a race, since two processes collecting at once would share one entry and the second
inblobwrite would move the report the first is about to read.agent-manifestuses a fixed name and swallowsFileExistsError, which is the shape I avoided here.The binding, which is the part to review
Only TPM had a defined key-and-nonce binding. SEV-SNP and TDX had none, so there was no defined way to commit the offered channel key and the verifiers'
expected_report_datahad nothing to compare against. Implementingattest()for them requires defining one, and since a peer and its verifier must derive identical bytes, that is a normative addition todocs/spec/attestation.md:with
ca2a-snp-v1|andca2a-tdx-v1|alongside the existingca2a-tpm-v1|, written left-aligned and zero-padded into the 64-byte field. Domain-separated so one platform's report cannot be replayed as another's, and length-prefixed for the reason the TPM binding already documents. The derivation now lives once inca2a_runtime.tee.bindingand all three providers use it;tpm_qualifying_datakeeps its signature and its bytes.Worth noting the GCP TDX harness in
docs/hardware-validation.mdused a plainsha256(channel_pub || nonce). This does not match that, deliberately, since the ad-hoc form has neither domain separation nor length prefixes.What this does not do
Neither collector has run on real SEV-SNP or TDX silicon. They are exercised against a simulated configfs tree and synthetic reports, so this is code that should work, not a validated capability, and LIMITATIONS, ROADMAP, the spec and
hardware-validation.mdall say exactly that. The natural hardware runs are a non-paravisor SNP guest and a GCP C3, andhardware-validation.mdlists what each run should confirm.I also left
auxblobon the floor for SNP. The kernel returns AMD's certificate table there as GUID-tagged DER, not PEM. Parsing a binary layout I have never seen from real hardware would be a guess, and passing it through asattestation_key_chain_pemwould be a wrong one, so a verifier still fetches the VCEK from the AMD KDS. Parsing it is what would make SNP appraisal fully offline, and it wants hardware to check against. This felt like the same lesson as the six-byte-early TDX parse that passed CI while rejecting every genuine quote.Drive-by doc fixes
Found while checking my own claims, happy to split out if preferred:
docs/hardware-validation.mdlisted the live attested peer run and the cross-operator cross-TEE run under "Not yet validated", while documenting both in full sections above it.docs/spec/error-codes.mdstill said the TDX and TPM backends were not implemented, which feat(tee): implement TPM attest() and carry verifiable evidence (#73) #74 already made untrue.component-model.mdandfailure-modes.mdsaid SEV-SNP and TDX have no collector.Testing
tests/unit/test_snp_tdx_attest.py, 30 tests: the binding (including that a field boundary cannot be shifted and that the three platforms differ), the configfs-TSM path against a simulated tree, both providers' detect/attest pairs including the agreement invariant, and the failure modes (wrong provider, empty report, a report committing something else, the Azure case, a non-TDX quote).282 passed, 3 skipped. ruff, mypy and bandit clean.