feat(tee): TDX quote parsing, versions 4 and 5 - #1186
Conversation
New aleph_tee::tdx::quote module, sibling of the SNP report parser: parse_tdx_quote() walks header, v5 body descriptor, TD report body (1.0 and 1.5), signature data and the nested certification envelopes through a bounds-checked cursor. Structure only: signatures, the PCK chain and TCB policy are the next increments. The v5 body sits at offset 54, after a body descriptor; parsing it at the v4 offset (48) fails silently, shifting every register by six bytes into plausible-looking digests. The parser dispatches on the header version, and the fixture tests pin exact register values from two independent v5 sources. signed_region captures the exact bytes the quote signature covers: everything before the signature-data length field, descriptor included on v5. Confirmed against all four vendored fixtures, whose signatures verify over that range under their embedded attestation keys. Trailing bytes after the signature data are ignored (quotes arrive in padded fixed-size buffers); one upstream fixture deliberately carries trailing garbage to test exactly that. Fixtures are byte-identical vendored copies from Phala dcap-qvl (MIT), google/go-tdx-guest (Apache-2.0) and automata-dcap-attestation (MIT), with provenance and vendoring-time checks in the fixture README.
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The parsing logic itself is excellent: the offset math matches the Intel TDX quote v4/v5 layout (48-byte header, TDREPORT10/15 at 584/648 with report_data last, v5 descriptor at offset 48, 384-byte QE report, type-6-wrapping-type-5 cert envelopes), all length fields are bounds-checked with no panics on untrusted input, the strict envelope-exactness checks are the right call, and the QE-binding hash recomputation in assert_structure is a clever self-validating test design. However, the PR does not compile from a fresh checkout: the four binary fixtures referenced by include_bytes! (quote.rs:380-384) were never committed because the global .bin rule in .gitignore (line 14) silently swallowed them — git ls-files shows only README.md under tests/fixtures/tdx/, and no fetch script exists. CI (cargo test --locked) will therefore fail, and the vendoring-time signature verification documented in the README is not reproducible by anyone else. Follow the existing precedent in the same .gitignore (the !...supervisor-daemon/tests/fixtures/.sqlite3 exception) or rename to the upstream .dat extension, then commit the fixture files.
rust/crates/aleph-tee/src/tdx/quote.rs (line 380): Blocking: the four fixture .bin files this and the next three include_bytes! point at are not in the commit. The global *.bin rule at .gitignore:14 ignored them during git add, so git ls-files shows only README.md in tests/fixtures/tdx/, and no script fetches them. A fresh clone fails to compile the crate (include_bytes! is resolved at compile time), and CI's cargo test --locked (test-rust.yml) will fail. Fix by adding !rust/crates/aleph-tee/tests/fixtures/tdx/*.bin to .gitignore (mirroring the existing !rust/crates/supervisor-daemon/tests/fixtures/*.sqlite3 exception on line 12) and committing the files, or renaming to the upstream .dat extension (not ignored). Ironically the committed README warns "Do not regenerate or re-encode them; tests pin exact register values" — the files it describes are missing.
rust/crates/aleph-tee/src/tdx/quote.rs (line 258): Non-blocking: once the fixtures are committed: signed_region is the one field defined by fiat in this increment, and nothing in the diff actually proves a signature verifies over it (the README records the check as vendoring-time work, which nobody else can reproduce). Consider a test verifying the ECDSA signature over signed_region with the embedded attestation_key for one fixture — k256 is already a workspace dependency — so the framing contract is pinned by CI rather than by prose.
rust/crates/aleph-tee/src/tdx/quote.rs (line 163): Nit: the error always says "truncated quote" even when take is called from parse_signature_data's inner cursor, so a short nested certification field reads "truncated quote: nested certification data type needs 2 bytes at offset 738", which is confusing. A context label (or just "truncated input") would read better. Cosmetic only.
rust/crates/aleph-tee/src/tdx/quote.rs (line 553): Nit: rejects_truncation_everywhere sweeps only the v4 fixture; a v5 sweep would additionally exercise the body-descriptor region (offsets 48-54) and the 1.5 extension tail, which are the offsets unique to this PR's new logic.
The global *.bin ignore rule silently dropped them from the previous commit, leaving include_bytes! pointing at files only present locally. Exempt the fixture directory, like the supervisor-daemon sqlite3 fixtures one rule above.
The framing contract (the ECDSA P-256 signature covers everything before the signature-data length field, body descriptor included on v5) was previously pinned only by the fixture README. Verify it in CI with the embedded attestation key, for all four fixtures, and assert that any other framing fails. Uses openssl, already a dependency of the SNP verifier; k256 would not do (secp256k1, not P-256).
The truncation sweep only walked the v4 fixture, never reaching the offsets unique to v5: the body descriptor and the TD report 1.5 extension tail. Add a v5 sweep. The cursor error said "truncated quote" even when walking the nested signature-data buffer, where the reported offset is relative to that buffer, not the quote; say "truncated input" instead.
The attest agent embeds aleph-tee, so the new module moves the measured chain. Values match what CI computed for this branch.
|
All four findings taken, four commits:
Also re-seeded |
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Excellent, security-conscious parsing code. I independently verified every empirical claim in the PR description: the v4/v5 layouts match go-tdx-guest's reference implementation field-for-field; all four fixture signatures verify over exactly the signed_region the code captures (632/702 bytes) and fail over a one-byte truncation; the QE binding hash at qe_report[320..352] matches SHA-256(attestation_key || qe_auth_data); the type-6/type-5 envelope nesting consumes exactly on all fixtures; all pinned register hex values in the tests match an independent parse; and all four fixtures are byte-identical to upstream with the licenses (MIT, Apache-2.0, MIT) as documented in the README. Bounds checking is airtight (checked_add, named truncation errors, no reachable panics, no unsafe code), and the trailing-bytes policy is sound since nothing after the signature data feeds any verified value. The three nits below are non-blocking polish items.
rust/crates/aleph-tee/src/tdx/quote.rs (line 597): The magic number PHALA_V4.len() - 71 is sig_end - 1 in disguise: the fixture carries 70 bytes of zero padding after the signature data, so this is the largest prefix that still truncates within the signature data — any prefix of length 4936..5005 would parse successfully. A short comment pinning that relationship (or computing the bound from the parsed structure) would keep the test meaningful if the fixture is ever replaced.
rust/crates/aleph-tee/src/tdx/quote.rs (line 296): The only accepted-format combination without test coverage is a v5 quote carrying a TD report 1.0 body (descriptor type 2): the body_size == TD_REPORT15_SIZE branch is only exercised as false-by-default on v4. A synthetic test mutating a v5 fixture's descriptor to type 2 / size 584 and asserting a successful parse with v15 == None and signed_region.len() == 54 + 584 would close that gap. (No fixture ships with this combination, so it must be synthesized.)
rust/crates/aleph-tee/tests/fixtures/tdx/README.md (line 12): Attribution nit: Apache-2.0 redistribution technically requires including the license text, not just naming it. For test fixtures this is a gray area, but dropping the Apache-2.0 text (or a NOTICE) into this directory alongside the README would make the vendoring airtight. Also note the Phala file lives on the master branch (not main), which matters for anyone re-verifying byte-identity.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev-2.1 #1186 +/- ##
===========================================
+ Coverage 78.40% 78.74% +0.33%
===========================================
Files 145 145
Lines 15400 15766 +366
Branches 972 994 +22
===========================================
+ Hits 12075 12415 +340
- Misses 3157 3178 +21
- Partials 168 173 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Drop the two go-tdx-guest fixtures (Apache-2.0): their coverage is either duplicated by the remaining quotes or synthesized in-test. The production v4 sample's trailing-marker case becomes a synthesized test appending bytes to the v4 quote; the second v5 sample goes (the remaining one is the richer, with non-zero mrservicetd); every fixture still self-validates cryptographically (quote signature, QE binding). The two remaining quotes come from independent MIT sources whose licence texts now sit next to the files, satisfying the notice condition, and the README pins the exact upstream commits so byte-identity stays re-verifiable against fixed refs. Files renamed to version-neutral names; test constants follow.
… body The truncation sweep's largest-failing-prefix entry was a magic number (fixture length minus the padding plus one). Replace it with a test deriving the signature-data end from the quote's own length field, asserting the parse fails one byte before it and succeeds exactly at it, which also pins where padding tolerance begins. A v5 quote carrying a TD report 1.0 body is the one accepted format combination with no public fixture; splice one from the v5 fixture (same header, a type-2/584 descriptor, truncated body, original signature section) and assert it parses with no 1.5 extension and a 638-byte signed region.
|
All three nits taken, with one change of course on the licence one: rather than importing the Apache-2.0 text, the two go-tdx-guest fixtures are dropped entirely (35fe672). Their coverage was either duplicated or synthesizable: the trailing-marker case is now a synthesized test appending bytes to the v4 quote, the second v5 sample goes (the remaining one carries the richer non-zero mrservicetd), and both remaining quotes still self-validate cryptographically per run. The fixture set is now MIT-only from two independent sources, with both licence texts vendored next to the files and the upstream commits pinned in the README for byte-identity re-verification. The other two nits (6537ef4): the magic sweep entry is replaced by a test deriving the signature-data end from the quote's own length field, failing one byte before and parsing exactly at it, so the padding-tolerance boundary is pinned positively; and the v5-with-TD-report-1.0 combination is covered by a spliced synthetic (a naive descriptor mutation alone would misframe the signature section, so the splice rebuilds header + type-2 descriptor + truncated body + original signature data). Golden measurements verified unchanged locally. |
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This is a well-crafted, independently verifiable parsing layer. I re-derived the entire layout from the fixture bytes with a separate Python implementation and everything matches: the 48-byte header, the v5 body descriptor at offset 54, all pinned register values, the QE-report attestation-key binding hash, and — most importantly — both ECDSA signatures verify over exactly the proposed signed_region (632 bytes for v4, 702 including the descriptor for v5), with off-by-one and v4-offset negative controls failing as expected. The v5 descriptor-is-signed trap this PR guards against is real and genuinely pinned by the tests. Parsing is strictly bounds-checked with overflow-safe length arithmetic and exact envelope-size enforcement; the deferral of all verification decisions is consistently documented and consistently applied. Remaining findings are non-blocking: the PR description overstates the fixture count (four fixtures incl. two go-tdx-guest Apache-2.0 quotes; the tree vendors two MIT fixtures) and understates the test count (16, not 13), and the golden-measurement regeneration is plausibly explained by aleph-tee being linked into the measured attest-agent but deserves a confirmation note in the PR description.
rust/crates/aleph-tee/tests/fixtures/tdx/README.md (line 8): The PR description is out of sync with the tree: it claims four vendored fixtures (including go-tdx-guest tdx_prod_quote_SPR_E4 and quote_sample_v5 under Apache-2.0) and that all four fixtures parse, but the tree vendors exactly two MIT fixtures, and the tests exercise only those two. The description also says 13 tests where the module has 16. The README here is accurate — it is the description that over-claims. Consider syncing the description (or vendoring the go-tdx-guest production fixture, whose trailing-garbage case is only synthesized here) so reviewers are not told about coverage that does not exist.
nix/golden-measurements.json (line 2): The golden launch measurements changed, but the PR description frames this as a pure parsing addition with nothing existing changing. The change is plausibly legitimate — aleph-tee is linked into aleph-attest-agent, which ships in the measured initrd (nix/initrd.nix:104), so the new pub mod tdx recompiles the measured agent — but per the warning in check-golden-measurements.sh, an unrelated or stale regeneration would silently mask a reproducibility regression. Please confirm these four values were produced by check-golden-measurements.sh --update on this exact branch state, and add a sentence to the PR description explaining the golden bump so it does not read as incidental noise.
rust/crates/aleph-tee/src/tdx/quote.rs (line 113): Minor: qe_report is the only fixed-size field stored as a Vec (attestation_key, quote_signature, and qe_report_signature are all fixed arrays, and qe_report.len() == 384 is an invariant asserted in tests). A [u8; QE_REPORT_SIZE] would make that invariant a type instead of a runtime property, and it is opaque either way. Not blocking.
rust/crates/aleph-tee/src/tdx/quote.rs (line 598): Nit on the comment: any prefix of a valid quote must fail is not quite true — a prefix ending exactly at the signature-data end parses fine (which padding_tolerance_starts_exactly_at_the_signature_data_end proves). The chosen lengths all avoid the boundary so the test is right; only the prose over-claims. Consider any prefix ending inside the signature data must fail.
rust/crates/aleph-tee/src/tdx/quote.rs (line 622): Style: the raw offsets here and at lines 481/634 (632, 636, 54 + 648 + 4 + 64 + 64 + 2) could be spelled with the module constants — HEADER_SIZE + TD_REPORT15_SIZE + 4 + 64 + 64 + 2 — so the numbers stay tied to their definition. Purely cosmetic; the comments already explain the arithmetic.
It is the only fixed-size signature-data field that was runtime-length Vec; the 384-byte invariant is now the type, and the test asserting the length goes.
The truncation-sweep comment claimed any prefix of a valid quote must fail, which the padding-boundary test itself disproves at exactly the signature-data end; scope the claim. The hardcoded 632/636 offsets and the v5 certification-size arithmetic now derive from HEADER_SIZE and the body-size constants.
aleph-tee compiles into the measured attest agent, so the qe_report field type change moves the launch measurements.
|
All five taken (three commits + a description rewrite):
|
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Excellent, well-documented, rigorously tested PR. I independently re-parsed both fixtures in Python following the Intel quote v4/v5 spec: all field offsets, sizes, nested envelope framing, and exact-size consumption checks in the parser are correct; the ECDSA signature verifies over exactly the parser's signed_region (including the v5 body descriptor, confirming the six-byte-shift trap is handled right) and fails over any other framing; and the QE-report binding hash at qe_report[320..352] confirms the signature-data layout. The parser is bounds-checked and panic-free on hostile input, the trailing-bytes tolerance is cryptographically outside anything verified, the golden-measurement re-seed rationale is sound (aleph-attest-agent path-depends on aleph-tee and ships in the measured initrd), and the 16 tests meaningfully pin the fragile parts (exact register values, padding boundary, truncation sweeps, rejection paths). Only two trivial nits: A test comment that contradicts its own assertion and the README. Approve.
rust/crates/aleph-tee/src/tdx/quote.rs (line 476): This comment contradicts both the assertion below it ("fixture must carry trailing padding") and the README ("70 bytes of zero padding after the signature data"). As written, "carries zero padding" reads as if there is no padding. Suggest: "The v4 fixture carries 70 bytes of zero padding after the signature data."
rust/crates/aleph-tee/src/tdx/quote.rs (line 317): Worth an explicit note that this deliberately scopes the parser to fresh Intel-QE quotes: certification data type 5 at the outer level (cached-collateral layout) is rejected, which is a decision increments 3-4 should revisit if cached collateral is ever needed. The doc comment partially covers this, so feel free to drop.
New aleph_tee::tdx::quote module, sibling of the SNP report parser: parse_tdx_quote() walks header, v5 body descriptor, TD report body (1.0 and 1.5), signature data and the nested certification envelopes through a bounds-checked cursor. Structure only: signatures, the PCK chain and TCB policy are the next increments. The v5 body sits at offset 54, after a body descriptor; parsing it at the v4 offset (48) fails silently, shifting every register by six bytes into plausible-looking digests. The parser dispatches on the header version, and the fixture tests pin exact register values from two independent v5 sources. signed_region captures the exact bytes the quote signature covers: everything before the signature-data length field, descriptor included on v5. Confirmed against all four vendored fixtures, whose signatures verify over that range under their embedded attestation keys. Trailing bytes after the signature data are ignored (quotes arrive in padded fixed-size buffers); one upstream fixture deliberately carries trailing garbage to test exactly that.
2 participants
Increment 2 of the Intel TDX design (#1144): quote parsing in
aleph_tee::tdx, versions 4 and 5. Additive except for the newpub mod tdxline — and one deliberate consequence of that line: the golden launch measurements move (see below).tdx/quote.rsparse_tdx_quote()walks header → (v5 body descriptor) → TD report body → signature data → nested certification data, through a bounds-checked cursor that names the field in every truncation error. Structure only: no signature, chain, or TCB decision is made at this layer (that's increments 3-4).signed_regioncaptures the exact bytes the quote signature covers (everything before the signature-data length field; for v5 that includes the descriptor). Not just prose: a test verifies the ECDSA P-256 signature oversigned_regionwith the embedded attestation key for both fixtures, and asserts any other framing fails.extract_report_data()andextract_registers() -> TdxRegisters {mrtd, rtmr1, rtmr2, mrconfigid}.rtmr0/rtmr3stay readable on the body but out of the pinned set (deployment parameters and the reserved launch-TCB commitment respectively).Fixtures (
tests/fixtures/tdx/, with a provenance/licence README)SHA-256(att_key || auth_data).Tests (16): both fixtures parse with exact pinned registers; signature and QE-binding verification as above; the synthesized trailing-bytes and v5/TD-1.0 cases; rejection of SGX quotes, versions other than 4/5, non-ECDSA-P256 key types, SGX body descriptors, descriptor/envelope size mismatches, oversized length fields, and truncation sweeps over both fixture layouts plus the derived padding boundary.
Golden measurements:
nix/golden-measurements.jsonis re-seeded in this PR. aleph-tee links into aleph-attest-agent, which ships in the measured initrd, so the new module recompiles the measured agent and legitimately moves the launch measurements. The values were produced bynix/check-golden-measurements.sh --updateon this branch, match what CI computed independently for the same tree, and a plain check re-run after the later test-only commits reports "Golden measurements match" (test and fixture files do not feed the measured build).🤖 Generated with Claude Code
https://claude.ai/code/session_01DUWEzYLgSidwttzQGtzB43