Update az snp / tdx vtpm dependency to 0.5

[surajssd]

No description provided.

[fitzthum]

I think you'll want to do cargo update az-snp-vtpm and commit the Cargo.lock

[surajssd]

I think I will wait until confidential-containers/guest-components#436 is merged.

[fitzthum]

The stable check seems to have a legit failure. Maybe there were changes to the crate?

[surajssd]

The stable check seems to have a legit failure. Maybe there were changes to the crate?

Yes that's right. I am on it.

[surajssd]

cc: @mkulke for review of the updated tests.

[surajssd]

Can someone authorize the test runs?

[mkulke]

the az-tdx-vtpm verifier has been merged since, should we adjust it in the same PR?

regarding the switch to v0.5: this changed the body of the evidence. we might want to consider versioning the evidence that we produce on the attester side. if someone sends evidence with older guest-components to this verifier it'll fail to serialize, with a potentially confusing error. :/

if we'd send { version: "v1" , ... } from guest-components, we could verify this here with a readable error msg: "Evidence version < v1" or something

Alternatively we can change the Evidence type here to have pcrs: Option<...> and fail with a similar error if it's None. but I find the first option preferable

[surajssd]

the az-tdx-vtpm verifier has been merged since, should we adjust it in the same PR?

What does this entail?

regarding the switch to v0.5: this changed the body of the evidence. we might want to consider versioning the evidence that we produce on the attester side. if someone sends evidence with older guest-components to this verifier it'll fail to serialize, with a potentially confusing error. :/

if we'd send { version: "v1" , ... } from guest-components, we could verify this here with a readable error msg: "Evidence version < v1" or something

How do I do this? As in add the version where?

[mkulke]

the az-tdx-vtpm verifier has been merged since, should we adjust it in the same PR?

What does this entail?

bumping the az-tdx crate too, and a similar replacement of the evidence fixtures I guess. should be pretty much 1:1 to the az-snp changes

[mkulke]

regarding the switch to v0.5: this changed the body of the evidence. we might want to consider versioning the evidence that we produce on the attester side. if someone sends evidence with older guest-components to this verifier it'll fail to serialize, with a potentially confusing error. :/
if we'd send { version: "v1" , ... } from guest-components, we could verify this here with a readable error msg: "Evidence version < v1" or something

How do I do this? As in add the version where?

The version would have to be added to the evidence type in guest-components and similarly in the verifier module here:

https://github.com/confidential-containers/kbs/blob/3003ced913bf83fa11d3ef753bb621f9cd030ae8/attestation-service/verifier/src/az_snp_vtpm/mod.rs#L25-L30

At the moment the evidence is serialized as json and maybe serde will happily ignore an additional property quote.pcrs when deserializing, so nothing breaks immediately.

but we would have to introduce such a field once we actually use the pcrs field, so I'd rather do it here. I would suggest this PR back to draft, until a corresponding change in guest-components has been merged.

[surajssd]

Once confidential-containers/guest-components#460 merged, I will udpate this PR to use 0.5.1 for az-cvm-vtpm crates and also update the guest components.

[Xynnn007]

@surajssd confidential-containers/guest-components#460 is merged

[mkulke]

small nits, otherwise lgtm

[mkulke]

There is a dependency to guest-components in ./kbs/tools/client/Cargo.toml we might want the rev in this PR too to reflect the changes on the attester side.

[mkulke]

There is a quote.verify_pcrs() check missing in each verifier.

Also, I don't think there is a pub fn to retrieve the PCRs from the quote yet. We can address this is a later PR when we actually consume the PCRs, as this probably needs a new minor release in the az-*-vtpm crates.

missing pcr verification

[surajssd]

There is a quote.verify_pcrs() check missing in each verifier.

Why don't we use the generic verify function from the library? https://github.com/kinvolk/azure-cvm-tooling/blob/de7ee4881fe7de01a0253d2e9960363f940ed682/az-cvm-vtpm/src/vtpm/verify.rs#L38-L49

Rather than reimplementing them here: https://github.com/confidential-containers/kbs/blob/3003ced913bf83fa11d3ef753bb621f9cd030ae8/attestation-service/verifier/src/az_tdx_vtpm/mod.rs#L76-L94

[mkulke]

Why don't we use the generic verify function from the library?

Ah, good question. There is a pending refactoring that will move the verification of report_data/nonce to from the verifiers to an upper level, so we cannot use the unified verify() method anymore, the nonce is just added as report_data to the claims and then verified somewhere else in some non-tee specific place, iiuic.

Rather than reimplementing them here:

it's not re-implementing. we're actually calling the library verification, we just have to extract the pubkey prior to sig verification.

Finally, in the doc-comments on both verifier fns the individual verification steps are enumerated. it would be good to add that we perform a PCR digest check here.

[surajssd]

There is a quote.verify_pcrs() check missing in each verifier.

@mkulke done.

[mkulke]

lgtm

[surajssd]

@fitzthum if the code looks good to you can we merge this?