refactor(attestation): drop dcap-qvl dependency from the attestation crate#3590
Conversation
…feature Separate Intel TDX DCAP cryptographic verification from the post-DCAP policy checks so the two can run in different places, and make dcap-qvl an optional dependency. Attestation::verify is split into: - verify_with_report: pure post-DCAP checks against an already-produced VerifiedReport, no dcap-qvl - verify_locally: full local DCAP + post-DCAP, behind the new local-verify feature (used off-chain by node, tee-authority, attestation-cli) - verify_mock_only: the Mock path, always compiled dcap-qvl moves behind local-verify, out of the attestation crate's default dependency graph. A new dcap_conversions module translates between the tee-verifier-interface DTOs and dcap-qvl types, pinned by byte-equal borsh-layout tests. The duplicate attestation Collateral and QuoteBytes newtypes collapse into re-exports of the tee-verifier-interface types (single source of truth); the interface crate gains an off-by-default serde feature for the node's /public_data payload. This is groundwork: mpc-contract's behavior is unchanged. Its synchronous attestation path now calls verify_locally (a byte-identical replacement for the old verify), so it still links dcap-qvl for now. Routing DCAP through a separate verifier contract, and dropping dcap-qvl from mpc-contract, is a follow-up.
There was a problem hiding this comment.
Pull request overview
Refactors the attestation verification APIs so DCAP cryptographic verification can be separated from post-DCAP policy checks, and makes dcap-qvl an optional dependency behind a new local-verify feature for off-chain/local verification.
Changes:
- Split verification into “post-DCAP only” (
verify_with_report) vs “full local DCAP + post-DCAP” (verify_locally, behindlocal-verify) and updated call sites accordingly. - Consolidated
QuoteBytes/Collateraltotee-verifier-interfaceas the single source of truth, adding an off-by-defaultserdefeature for input DTOs. - Added
dcap_conversionsboundary for off-chain bridging betweendcap-qvltypes and interface DTOs, pinned by Borsh-layout tests.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/test-utils/src/attestation.rs | Adjust fixtures to build QuoteBytes directly and parse collateral via new helper. |
| crates/tee-verifier-interface/src/lib.rs | Add optional serde derives for verifier input types (QuoteBytes, Collateral). |
| crates/tee-verifier-interface/Cargo.toml | Introduce off-by-default serde feature with no_std-friendly serde dep config. |
| crates/tee-authority/src/tee_authority.rs | Update collateral conversion usage and switch verification call to verify_locally; adjust docs. |
| crates/tee-authority/Cargo.toml | Enable mpc-attestation/local-verify for off-chain local verification path. |
| crates/node/src/trait_extensions/convert_to_contract_dto.rs | Update collateral DTO mapping after collapsing collateral types. |
| crates/node/src/tee/remote_attestation.rs | Switch remote attestation validation to verify_locally. |
| crates/node/Cargo.toml | Enable mpc-attestation/local-verify for node usage. |
| crates/mpc-attestation/tests/test_attestation_verification.rs | Gate integration tests behind local-verify; add agreement test for split paths. |
| crates/mpc-attestation/src/report_data.rs | Gate DCAP-quote-parsing unit test behind local-verify; adjust fixture usage. |
| crates/mpc-attestation/src/lib.rs | Re-export dcap_conversions only when local-verify is enabled. |
| crates/mpc-attestation/src/attestation.rs | Introduce verify_with_report + verify_mock_only + verify_locally and shared helpers. |
| crates/mpc-attestation/Cargo.toml | Add local-verify feature and dev setup for tests that require it. |
| crates/contract/src/tee/tee_state.rs | Switch contract attestation verification call to verify_locally. |
| crates/contract/src/dto_mapping.rs | Update collateral DTO conversions for new single-source collateral type. |
| crates/contract/Cargo.toml | Enable mpc-attestation/local-verify so current sync contract path still links DCAP. |
| crates/attestation/tests/collateral.rs | Update tests to use new collateral parse helpers rather than type methods. |
| crates/attestation/src/quote.rs | Remove local QuoteBytes newtype; re-export from tee-verifier-interface. |
| crates/attestation/src/measurements.rs | Switch VerifiedReport dependency to tee-verifier-interface. |
| crates/attestation/src/lib.rs | Add dcap_conversions module behind local-verify. |
| crates/attestation/src/dcap_conversions.rs | New: explicit interface↔dcap conversions with Borsh-layout pin tests. |
| crates/attestation/src/collateral.rs | Remove local collateral newtype; re-export interface type + add JSON parse helpers (test-utils). |
| crates/attestation/src/attestation.rs | Split DCAP and post-DCAP; add verify_with_report + verify_locally + dcap_report (feature-gated). |
| crates/attestation/Cargo.toml | Make dcap-qvl optional behind local-verify; forward borsh-schema feature to interface. |
| crates/attestation-cli/src/verify.rs | Switch CLI to full local verification via verify_locally. |
| crates/attestation-cli/Cargo.toml | Enable mpc-attestation/local-verify for CLI verification. |
| Cargo.lock | Reflect new feature/dependency edges (including optional serde and dev self-deps). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…me test; drop a clone Address self-review/Copilot/claude[bot] doc-drift nits on the DCAP split: - verify_locally docs no longer claim mpc-contract calls the verifier contract for DCAP; the contract calls verify_locally today, and the verifier-contract switch is a follow-up - drop the stale 'now 1 day' note on MAX_COLLATERAL_AGE; the contract expiry constant is still 7 days - rename the new integration test to the mandated __should_ form - dcap_report borrows the quote bytes instead of cloning them
verify_mock_only is only valid for Mock attestations; a Dstack input is caller-side misuse, not a verification failure. Add a debug_assert so it fails loudly in debug/test builds, keeping the release error path unchanged.
|
PR title type suggestion: This PR's primary change is dropping a dependency (dcap-qvl), which falls under the |
Extract the duplicated dcap_qvl <-> tee-verifier-interface conversions (previously a deliberate sibling copy in attestation/src/dcap_conversions.rs and tee-verifier/src/conversions.rs) into a new tee-verifier-conversions crate. Both the on-chain verifier contract and the off-chain attestation crate now re-export from it, so the Borsh-layout pin tests live in one place. Replace Attestation::verify_mock_only (which carried a debug_assert! for the impossible Dstack arm) with per-variant verification: MockAttestation::verify and a DstackVerify extension trait. Attestation::verify_with_report and verify_locally dispatch to these, so the invalid "Dstack on the mock path" case no longer exists in the type system.
dcap-qvl dependency from the attestation crate
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/attestation/tests/collateral.rs:66
- These signatures are parsed from hex into raw bytes, so
len()is in bytes. The assertions are fine, but the comments currently describe “64 hex chars (32 bytes)”, which doesn’t match the actual encoding (the fixture is 128 hex chars → 64 bytes). Updating the comments avoids confusion.
// TCB info signature should be 64 hex chars (32 bytes)
assert_eq!(collateral.tcb_info_signature.len(), 64);
// QE identity signature should be 64 hex chars (32 bytes)
assert_eq!(collateral.qe_identity_signature.len(), 64);
Pull request overviewRefactor that prepares Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
- Inline verify_mock_attestation into MockAttestation::verify; route re_verify's Mock arm through it. - Add AcceptedAttestation::dstack / ::mock constructors, replacing the free fn accepted_dstack_attestation and the inline struct literals. - Rename DstackAttestation::dcap_report to verify_dcap_quote (it runs DCAP verification and returns the report, rather than being a getter). - TODO(#3264) at the contract's verify_locally call site and on the local-verify feature flags / docs, marking the transitional in-WASM DCAP path the verifier-contract follow-up removes. - Tighten doc comments across the attestation/tee-verifier crates; drop function names and external party names from config-level comments so they don't go stale.
|
PR title type suggestion: Since the primary change is removing a dependency, the type prefix should be |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/attestation/tests/collateral.rs:66
- The parsed
tcb_info_signature/qe_identity_signaturefields areVec<u8>(decoded from hex), so this comment about “hex chars (32 bytes)” is misleading. The assertion value (64) matches the byte length for these ECDSA signatures; update the comment to reflect the actual units/encoding.
// TCB info signature should be 64 hex chars (32 bytes)
assert_eq!(collateral.tcb_info_signature.len(), 64);
// QE identity signature should be 64 hex chars (32 bytes)
assert_eq!(collateral.qe_identity_signature.len(), 64);
crates/tee-verifier-conversions/src/lib.rs:10
- This module doc says the crate needs
dcap-qvl+tee-verifier-interface+borsh, butborshis only used in the test suite (it’s a dev-dependency). Consider clarifying this to avoid implyingborshis required by downstream users of the conversions API.
netrome
left a comment
There was a problem hiding this comment.
Looks good to me. Didn't go deep as this doesn't seem to change any behavior. I second Barak's duplication concern but it's not a blocker imo.
Split the mock constraint check out of `verify` into a private `verify_constraints` returning `Result<(), _>`. `verify` calls it and wraps the result into an `AcceptedAttestation`; `re_verify`'s Mock arm calls it directly, so the periodic on-chain re-verification no longer builds (and discards) an `AcceptedAttestation`.
…DstackVerify::verify The Dstack arm of `Attestation::verify_locally` re-inlined `verify_dstack_mpc_hashes` + the `AcceptedAttestation::dstack` assembly that `DstackVerify::verify` already does. Route it through `verify_dcap_quote` + `DstackVerify::verify` instead, removing the duplication and running DCAP before the post-DCAP checks (matching the async verifier flow). Drop verify_with_report__should_agree_with_verify_locally: after the dedup, verify_locally is verify_dcap_quote plus the same path verify_with_report dispatches to, so the agreement holds by construction.
|
PR title type suggestion: This PR primarily removes a dependency (dcap-qvl) and updates code to reflect that change. The type prefix should probably be Suggested title: |
|
All nits addressed, @barakeinav1 @netrome. ✅ Feel free to take re-review/approve. cc @gilcu3 @kevindeforth if you’re up for reviewing too. |
| @@ -135,7 +137,7 @@ impl IntoContractType<Collateral> for dtos::Collateral { | |||
| qe_identity, | |||
| qe_identity_signature: qe_identity_signature.into(), | |||
| pck_certificate_chain, | |||
| }) | |||
| } | |||
There was a problem hiding this comment.
not sure I understand that comment, what conversion it refers to? in the diff what I see is that we are removing a conversion
There was a problem hiding this comment.
"This conversion" = the whole impl (the field-by-field mapping from the JSON DTO dtos::Collateral to the internal Collateral), not the line changed in the diff; that line just drops the old QuoteCollateralV3 wrapper.
Right now there are two Collateral types: the serde JSON one in near-mpc-contract-interface (dtos::Collateral) and the internal one (tee_verifier_interface::Collateral), so we need this impl to convert between them. #3494 removes the duplicate DTO so only one Collateral remains, at which point this impl is dead and can be deleted.
Closes #3621
Why
dcap-qvl(Intel TDX quote verification) is ~310 KB / ~21% of thempc-contractWASM, pushing it toward NEAR's 1.5 MiB deployment cap (NEP-509). The plan (design doc) offloads DCAP to a standalonetee-verifiercontract sompc-contractcan dropdcap-qvl. This PR is the prerequisite refactor — it doesn't move DCAP or shrink the WASM yet.This is the first reviewable slice of #3540, which integrates the standalone
tee-verifiercontract intompc-contract. That PR is too large to review in one pass, so it's being split into self-contained PRs that land one at a time.What changed
Attestation::verifyis split so the cryptographic DCAP step is separable from the post-DCAP policy checks (measurement allowlist, report-data binding, RTMR replay):verify_with_report(report, …)— post-DCAP checks against an already-verifiedVerifiedReport; nodcap-qvl. The contract will call this once DCAP runs in the verifier contract.mpc/crates/attestation/src/attestation.rs
Line 131 in 231e48d
verify_locally(…)— full DCAP + post-DCAP in one call, behind the newlocal-verifyfeature; byte-identical signature to the oldverify.mpc/crates/attestation/src/attestation.rs
Line 160 in 231e48d
dcap-qvlbecomes optional, so it's no longer inattestation's default build.Two supporting changes:
Attestation::verifynow lives on each variant:MockAttestation::verify(…)(no DCAP, so aMockis verifiable without aVerifiedReport) andDstackAttestation::verify(…)via aDstackVerifytrait.mpc/crates/mpc-attestation/src/attestation.rs
Line 77 in 231e48d
dcap_qvl↔tee-verifier-interfaceconversions move into a newtee-verifier-conversionscrate (renamed fromtee-verifier/src/conversions.rs), shared by thetee-verifiercontract and the off-chainverify_locallypath so the byte-layout pin tests live in one place instead of being duplicated.Why
local-verifyexistsSome callers still need full local DCAP:
mpc/crates/node/src/tee/remote_attestation.rs
Lines 105 to 113 in 9717045
tee-authority:mpc/crates/tee-authority/src/tee_authority.rs
Lines 771 to 776 in 9717045
attestation-cli:mpc/crates/attestation-cli/src/verify.rs
Lines 73 to 83 in 9717045
local-verifyis the opt-in that re-addsdcap-qvlfor them (gatingverify_locally):mpc/crates/mpc-attestation/src/attestation.rs
Line 266 in 231e48d
No behavior change (yet)
mpc-contractenableslocal-verifyand still verifies locally viaverify_locally(byte-identical to the oldverify), so it linksdcap-qvland behaves exactly as before. The WASM only shrinks in the follow-up, when the contract switches toverify_with_reportand dropslocal-verify.