Update SNP Verifier with report and init claims

[fitzthum]

Also, move the SNP testing data into the test_data directory.

@mkulke This seems like it's going to have somewhat incorrect implications for the AZ Verifier (since we don't want to use the init data from the report for that). Not sure what the best way to reconcile is.

@Xynnn007 It seems like we don't specify what format the report_data and init_data should be returned in the claims map. I am assuming we want base64.

I am planning to also add to the parsed claims doc once #248 is merged.

[Xynnn007]

Now SGX and TDX are using hex as the claims. Maybe we should have a standard for all platforms? All using base64 or hex? It would help the consumer without worrying about the encoding of the claims.

[Xynnn007]

btw #248 is mergwd

[fitzthum]

Now SGX and TDX are using hex as the claims. Maybe we should have a standard for all platforms? All using base64 or hex? It would help the consumer without worrying about the encoding of the claims.

Yeah I think it has to be standardized or we aren't going to be able to move the checks out of individual verifiers.

[Xynnn007]

Yeah I think it has to be standardized or we aren't going to be able to move the checks out of individual verifiers.

Another idea is whether we need to standardize other claims? This could be a potential work for all platforms. Or, we should record the different encoding algorithm in the document.

[mkulke]

If the claims are generic for each TEE, should we add them individually in the TEE-specific parts of a the verification logic? can we just add them after the verification has passed?

https://github.com/confidential-containers/kbs/blob/eebaa977e0a23febb5318994cb0e5ba9daa7f8d5/attestation-service/attestation-service/src/lib.rs#L102-L116

(IMO, it probably would be cleaner to have them added in the verifier crate, so there would be evidence -> claim_set logic, be but we're already messing with the claim (flattening) in the attestation-service snippet above, so that might be ok)

[mkulke]

I recommend decoupling test cleanups into a discrete PR. We should really attempt to reduce the number of drive-by fixes. Non-controversial but necessary quality improvements could be merged quickly, and we can focus our review-time on parts which require more scrutiny. untangling unrelated parts from change-sets is a cognitive overhead.

[fitzthum]

If the claims are generic for each TEE, should we add them individually in the TEE-specific parts of a the verification logic? can we just add them after the verification has passed?

As of today we could add the expected init_data and report_data values to the claims map after the evidence has been verified, but I think the next step is to remove verification of those fields from the verifiers. As in, we will stop passing in init_data and report_data to the verifier. Instead, the verifier will just return the values as part of the claims map and the AS will make sure they are correct. I think this approach will make the interface a bit cleaner and help decouple verification from evaluation. One downside is that it will expose the report_data which is not necessary and might be confusing.

@mkulke my question for you is whether there is anything wrong with exposing the init_data for an Azure guest. I am assuming it is consumed by azure to provision the vTPM. Is it okay to include this in the claims map and assume that people won't check it?

Generally I agree that smaller PRs are better, but I don't mind simple drive-by fixes here and there, especially if it increases the likelihood that things get fixed.

[mkulke]

@fitzthum

As of today we could add the expected init_data and report_data values to the claims map after the evidence has been verified, but I think the next step is to remove verification of those fields from the verifiers.

For az-{snp,tdx}-vtpm TEEs we would have to make changes to support this, at the moment nonce/report_data verification is coupled with signature validation in an encapsulated library fn, like this:

but in principle the verification can be split, I think. What would be the projected timeframe for such a change?

Instead, the verifier will just return the values as part of the claims map and the AS will make sure they are correct. I think this approach will make the interface a bit cleaner and help decouple verification from evaluation. One downside is that it will expose the report_data which is not necessary and might be confusing.

Is there a reason why report_data should be part of a claims map? It's a property of a concrete RCAR exchange and hence not useful for a consumer of a claims map, is it?

Evidence extraction logic for report_data would still need to be implemented individually for each TEE, even if the verification is moved to upper layers. I would still recommend to have this close to the verifier implementation, maybe we can change the trait signatures, so a given TEE's evidence needs to implement report_data():

trait ExtractReportData {
    fn report_data(&self) -> &[u8];
}

type ClaimsMap = (); // dummy

trait Verifier {
    type Evidence: ExtractReportData;
    
    fn evaluate(&self, evidence: &Self::Evidence) -> Result<ClaimsMap>;
}


struct MyTee;
type MyEvidence = [u8;64]; // dummy

impl ExtractReportData for MyEvidence {
    fn report_data(&self) -> &[u8] {
        todo!();
    }
}

impl Verifier for MyTee {
    type Evidence = MyEvidence;
    
    fn evaluate(&self, evidence: &MyEvidence) -> Result<ClaimsMap> {
       todo!(); 
    }
}

I am assuming [init-data] is consumed by azure to provision the vTPM.

I'm not sure I understand. Init-data is a CoCo concept, how would that be related to provisioning of a vTPM?

[fitzthum]

but in principle the verification can be split, I think. What would be the projected timeframe for such a change?

I don't think there is any specific timeline. It would be nice to get things sorted out before we implement support for validating the init-data via the policy i.e. passing the expected init-data into AS and to the OPA verifier.

Is there a reason why report_data should be part of a claims map? It's a property of a concrete RCAR exchange and hence not useful for a consumer of a claims map, is it?

report_data should not be part of the claims map imo, but I don't think it's a huge problem if it is.

Evidence extraction logic for report_data would still need to be implemented individually for each TEE, even if the verification is moved to upper layers.

Yes, hence we need changes to each verifier before we can go forward with this. The traits you suggest are interesting. To me I am fine with leaving the verifier implementation completely open as long as it somehow returns a claims map. We could think about adding an extra method to the verifier trait to get the report data, but I am a little wary of separating evaluation of the evidence from parsing of the evidence.

I'm not sure I understand. Init-data is a CoCo concept, how would that be related to provisioning of a vTPM?

I just mean that for AZ SNP verifier the init_data will be the host_data from the report (with these changes, at least) and I am assuming that the host_data field is used by Azure for some kind of provisioning that is out of scope of the guest owner

[mkulke]

I just mean that for AZ SNP verifier the init_data will be the host_data from the report (with these changes, at least) and I am assuming that the host_data field is used by Azure for some kind of provisioning that is out of scope of the guest owner

a thanks, understood. having report_data and init_data in the claims would be misleading indeed,

Can you add this change to bottom of the az-snp-vtpm verifier logic, so we unset the respective keys from the json map?

...
        let mut claim = parse_tee_evidence(&snp_report);
        drop_unsupported_claims(&mut claim)?;
        Ok(claim)
    }
}

fn drop_unsupported_claims(claim: &mut serde_json::Value) -> Result<()> {
    let claim_map = claim
        .as_object_mut()
        .ok_or(anyhow::anyhow!("couldn't get claim map"))?;
    claim_map.remove("init_data");
    claim_map.remove("report_data");
    Ok(())
}

[fitzthum]

Can you add this change to bottom of the az-snp-vtpm verifier logic, so we unset the respective keys from the json map?

If we drop both of these the AS isn't going to be able to verify that the report_data is correct. I guess we could drop the init_data from the AZ SNP claims. The check in the AS for the init_data can maybe be optional?

[mkulke]

Can you add this change to bottom of the az-snp-vtpm verifier logic, so we unset the respective keys from the json map?

If we drop both of these the AS isn't going to be able to verify that the report_data is correct. I guess we could drop the init_data from the AZ SNP claims. The check in the AS for the init_data can maybe be optional?

re report_data:

as long as the signature to verifier::evaluate() doesn't change and we still receive expected_report_data as argument, we can just pass it to the claim after verification and verify it again in upper layers:

async fn evaluate(
      &self,
      evidence: &[u8],
====> expected_report_data: &ReportData,
      expected_init_data_hash: &InitDataHash,
)

If the signature changes, and we don't receive expected_report_data any more, then there will be breakage, b/c atm we cannot extract the nonce from the evidence.

re init_data:

at the moment it's still not entirely clear to me how we want to curate init_data in attestation-agent on a peerpod. If we can ignore it for the time being until we know more, that would be fine w/ me.

[fitzthum]

If the signature changes, and we don't receive expected_report_data any more, then there will be breakage, b/c atm we cannot extract the nonce from the evidence.

If there is no way to extract the report data from the evidence, then #228 might not really be feasible.

[mkulke]

If there is no way to extract the report data from the evidence, then #228 might not really be feasible.

Ah, I just meant it won‘t work automagically ootb, we need to make a crate release + bump a dep in this repo before we can use the respective code to do that. in principle #228 shouldn‘t be blocked, just needs prepwork

[fitzthum]

Ah, I just meant it won‘t work automagically ootb, we need to make a crate release + bump a dep in this repo before we can use the respective code to do that. in principle #228 shouldn‘t be blocked, just needs prepwork

Ok, in that case let me add the code you suggested and once we are ready we can tweak things to work better for #228

btw I think in the long term we can just have the AS remove the report_data from the claims map before it does the policy evaluation.

[fitzthum]

Ok, I forget what was happening with this PR. I think we ran into some confusion regarding how to handle init_data and report_data. I have dropped the commit that adds these to the SNP claims map. Now this PR just moves the test files and adds a description of the SNP claims to the doc. PTAL @mkulke @Xynnn007

[Xynnn007]

Thanks for this. The PR brings me to old times.. ; P

About the initdata and report_data claim. We can talk about it further in future. Meanwhile this PR looks good to me. Some comments.

[Xynnn007]

Might not related to the PR. But if we can have an agreement that the binary values should be all showed as hex or base64? see Line 250 in this file (I cannot add comment to line out of this PR)

[fitzthum]

Yeah let's take this in a separate issue but I think it is probably a good idea.

[Xynnn007]

Ok. See #372. And I think other fields of SNP should also do the same, such as https://github.com/confidential-containers/trustee/blob/main/attestation-service/verifier/src/snp/mod.rs#L240

[mkulke]

lgtm

[fitzthum]

Rebased to pick up the vlek change. I think the comments are accounted for so I merged before I forget about this PR again.