Verifier fetches collateral and verifies quotes the policy will always reject

[samlaf]

Found this while I was reviewing #70 because the no-cache mode is what makes the fetch happen on every message.
It's in the verify path, not the construction API which is why I'm creating this separate issue however.
Have not reviewed it myself, so if Claude hallucinated this feel free to close.

Claude Analysis

The policy check runs after verification, so a peer can make the verifier do
the full DCAP work — including an outbound collateral fetch — for an
attestation type the policy never accepts.

What happens

verify_attestation matches on the attestation type carried by the incoming
message. has_remote_attestation() is consulted only in the
AttestationType::None arm (crates/attestation/src/lib.rs:475). The
AzureTdx and DcapTdx | GcpTdx arms verify unconditionally, and the policy
check happens afterwards at crates/attestation/src/lib.rs:527:

let measurements = match attestation_type {
    AttestationType::None => {
        if self.has_remote_attestation() { ... }   // only checked here
    }
    AttestationType::DcapTdx | AttestationType::GcpTdx => {
        let (measurements, quote) = dcap::verify_dcap_attestation(...).await?;
        ...
    }
};

// Do a measurement / attestation type policy check
self.measurement_policy.check_measurement_with_gcp_cache(...)?;

So both of these do the full work before rejecting:

• A verifier built with expect_none() that receives a DCAP quote.
• A policy accepting only AzureTdx that receives a DcapTdx quote.

verify_attestation_sync has the same shape (lib.rs:551 and lib.rs:608).

Impact

Per connection, for a quote the policy can never accept, an unauthenticated
peer gets the verifier to run full DCAP signature verification and — on a
cache miss — an outbound HTTPS fetch to the PCCS endpoint or Intel PCS. The
rejection still happens, so this is wasted work rather than a soundness
problem. It matters most with no in-process cache, where every such message
is a fresh fetch.

Suggested fix

MeasurementRecord already carries the type
(crates/attestation/src/measurements.rs:323), so the policy can answer this
directly:

impl MeasurementPolicy {
    /// Whether any accepted record covers this attestation type
    pub fn accepts_attestation_type(&self, attestation_type: AttestationType) -> bool {
        self.accepted_measurements.iter().any(|r| r.attestation_type == attestation_type)
    }
}

Then reject at the top of both verify functions, before the match:

if !self.measurement_policy.accepts_attestation_type(attestation_type) {
    return Err(AttestationError::AttestationTypeNotAccepted);
}

That also subsumes the existing has_remote_attestation() check in the
None arm, which could then go away.

[ameba23]

Yep, agree this is an issue and it would be better to reject mismatched attestation types before potentially fetching collateral.

We could maybe even go a step further and check the measurements before fetching collateral - but this might be tricky to do without parsing the quote twice, as i think the dcap_qvl verifier method does quote parsing and verification together.

Also worth noting this would be mitigated by #65 - so its another argument for adopting that pattern.