Skip to content

refactor(test-utils): read the collateral straight from the captured public_data.json - #4143

Merged
pbeza merged 1 commit into
mainfrom
4140-derive-collateral-fixture
Aug 14, 2026
Merged

refactor(test-utils): read the collateral straight from the captured public_data.json#4143
pbeza merged 1 commit into
mainfrom
4140-derive-collateral-fixture

Conversation

@pbeza

@pbeza pbeza commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Follow-up on @gilcu3's code review comment: #4109 (comment).

Closes #4140

@pbeza
pbeza requested a review from gilcu3 August 13, 2026 15:37
@pbeza
pbeza force-pushed the 4140-derive-collateral-fixture branch 4 times, most recently from 032b74b to 85558a8 Compare August 14, 2026 08:24
@pbeza pbeza changed the title refactor(test-utils): generate the collateral fixture instead of reshaping it refactor(test-utils): read the collateral straight from the captured public_data.json Aug 14, 2026
Base automatically changed from 3787-regenerate-fixtures to main August 14, 2026 11:10
…aping it

`create-assets.sh` hex-encoded four collateral fields with hand-rolled `jq`
arithmetic because the fixtures predate a type change: `Collateral` used to
deserialize through a hex parser, so hex was the only shape that loaded. It and
`QuoteBytes` are now plain serde types over `Vec<u8>`, so the captured shape
reads directly, and `HexVec` already serializes to hex — no encoding to write
either way.

`collateral.json` is now derived from the committed capture by a test that
writes it under `UPDATE_FIXTURES=1` and otherwise asserts it matches, the same
pattern the verifier's borsh fixture uses. Its bytes are unchanged.

`collateral()` returns the typed value, which lets the verifier test and the
TEE authority test stop hand-decoding hex, and leaves the test-only hex parser
with no users at all.
@pbeza
pbeza force-pushed the 4140-derive-collateral-fixture branch from 85558a8 to bbaf519 Compare August 14, 2026 11:20
@pbeza

pbeza commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice simplification!

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Pull request overview

This removes the hand-rolled JSON→Collateral parser (collateral_from_json / collateral_from_str / CollateralError) from the attestation crate and its dedicated test file, and instead builds the test collateral directly from the committed public_data.json capture using plain serde (tee-verifier-interface's serde feature). test_utils::attestation::collateral() now returns a typed Collateral instead of a serde_json::Value, which lets tee-verifier's test drop its 20-line re-parsing helper and tee-authority's fixture helper drop its manual hex decode. The hex-encoded collateral.json survives only as the contract-DTO fixture; it is no longer emitted by create-assets.sh but is instead derived and pinned by a new equality test, with the regeneration step documented in the assets README.

Changes:

  • Delete the test-utils-gated JSON collateral parser and crates/attestation/tests/collateral.rs, plus the now-unused assert_matches dev-dependency and [[test]] stanza
  • test_utils::attestation::collateral() returns Collateral, deserialized from TEST_PUBLIC_DATA_STRING with trailing NUL terminators stripped from string fields
  • Callers simplified: tee-verifier/tests/verify_quote.rs drops make_collateral(), tee-authority reads .pck_crl directly
  • collateral.json generation moves out of create-assets.sh into a new UPDATE_FIXTURES-style test in test-utils; README step 8 updated

Reviewed changes

Per-file summary
File Description
Cargo.lock Drops assert_matches from the attestation dev-dependency graph
crates/attestation/Cargo.toml Removes assert_matches dev-dep and the collateral [[test]] stanza
crates/attestation/src/collateral.rs Removes the test-utils-gated JSON parser; file is now a single pub use
crates/attestation/tests/collateral.rs Deleted (8 tests covering the removed parser)
crates/tee-authority/src/tee_authority.rs fixture_pck_crl() reads the typed field instead of hex-decoding a JSON value
crates/tee-verifier/tests/verify_quote.rs Drops make_collateral(); uses test_utils::attestation::collateral() directly
crates/test-utils/Cargo.toml Enables tee-verifier-interface/serde
crates/test-utils/assets/README.md Removes collateral.json from the script output list; adds the fixture-regeneration command to step 8
crates/test-utils/assets/create-assets.sh Removes the jq hex/NUL-stripping pipeline that produced collateral.json
crates/test-utils/src/attestation.rs Adds TEST_PUBLIC_DATA_STRING, captured_collateral(), retypes collateral(), adds the collateral.json fixture test

I verified that the committed collateral.json is byte-identical to what the removed jq pipeline produces from the current public_data.json, and that pck_certificate_chain is the only field carrying a NUL (a single trailing one), so the new trim_end_matches and the old jq truncate-at-first-NUL agree on this capture. CI is green on all checks.

Findings

Blocking (must fix before merge):

  • crates/test-utils/src/attestation.rs:204 — the assertion compares against TEST_COLLATERAL_STRING, which include_str! bakes in at compile time, not against the file the test just wrote. In the only situation the UPDATE_FIXTURES branch exists for — a stale collateral.json after a fresh public_data.json — the run writes the correct file and then fails the assert against the old baked-in contents. Only a second invocation passes (cargo fingerprints include_str! inputs, so it recompiles). That makes the command this PR adds to README step 8 (UPDATE_FIXTURES=1 cargo test -p test-utils collateral_fixture) report a failure on the run that is supposed to fix things, while the README text says only "the last run fails". crates/tee-verifier/tests/verify_quote.rs:203 already avoids this by reading the file back at runtime; mirror it:

    let committed = std::fs::read_to_string(path).expect("collateral.json is readable");
    
    // Then
    assert_eq!(
        expected, committed,
        "collateral.json is stale; regenerate with UPDATE_FIXTURES=1"
    );

    TEST_COLLATERAL_STRING stays needed by mock_dto_dstack_attestation, so nothing else changes.

Non-blocking (nits, follow-ups, suggestions):

  • crates/attestation/Cargo.toml:10 — after this PR, crates/attestation/src/collateral.rs:12,15 were the only #[cfg(feature = "test-utils")] sites in the crate, so test-utils = [] now gates nothing. The dead feature is still declared and requested from four places: crates/attestation/Cargo.toml:33 (dev-dep self-reference), crates/mpc-attestation/Cargo.toml:10 (test-utils = ["attestation/test-utils"]) and :35, and crates/test-utils/Cargo.toml:10. Since this is a cleanup PR, dropping the feature and its enablers here keeps the config honest.
  • crates/test-utils/src/attestation.rs:196 — the byte-for-byte comparison silently depends on serde_json's preserve_order feature (nothing in this workspace's manifests enables it; it arrives transitively, as Cargo.lock's indexmap dep on serde_json shows). Without it, to_string_pretty emits keys sorted alphabetically rather than in public_data.json's order and the test fails — or, under UPDATE_FIXTURES=1, silently reorders the committed fixture. Comparing parsed Values instead of raw strings would remove that coupling, at the cost of not pinning formatting.
  • crates/test-utils/assets/README.md:50 — this note still sits directly under "This will regenerate the following files" / "All files will be written into the specified output directory", but collateral.json is no longer among them. Worth stating there that it is produced by the collateral_fixture test in step 8, so a reader following the steps top-down does not expect it after step 3.

⚠️ Issues found

@pbeza

pbeza commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@barakeinav1 could you review this when you’re back on Sunday (unless someone beats you to it)? Thanks!

@kevindeforth kevindeforth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pbeza
pbeza added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit 850ed5d Aug 14, 2026
15 checks passed
@pbeza
pbeza deleted the 4140-derive-collateral-fixture branch August 14, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Derive the attestation fixtures from the captured public_data.json instead of reshaping them

3 participants