verifier: Fetch VCEK cert from KDS instead of bailing #555

AdithyaKrishnan · 2024-11-04T18:17:47Z

Fetch the VCEK cert from the KDS if it is absent in the cert chain instead of just printing a bail statement stating that the VCEK is not found.

cclaudio

Just a few minor comments.

cclaudio · 2024-11-04T19:54:45Z

deps/verifier/src/snp/mod.rs

+// Function to request vcek from KDS. Return vcek in der format.
+fn request_endorsement_key_kds(
+    att_report: AttestationReport,
+) -> Result<Vec<CertTableEntry>, anyhow::Error> {


The function is specific to download the VCEK, so I think it could be rename to something more specific such as fetch_vcek_from_kds()

Additionally, I think the function would be more self-contained if it returns Result<Vec<u8>>

Changed. If appropriate, please mark this comment as Resolved.

Additionally, I think the function would be more self-contained if it returns Result<Vec<u8>>

Tried this and got a type mismatch error

Btw I don't think you need anyhow::Error in in this return type.

fitzthum

Nice PR. Looks like this will address #456 which is actually a duplicate of #363

One thing to think about is that this could result in quite a few requests being made to the KDS. We should probably implement some kind of caching mechanism so that the verifier doesn't have to get the vcek for the same node again and again. We can probably do this in a follow-up.

Also, we might want to add vlek support at some point although I suspect this will require specifying a few more config params.

fitzthum · 2024-11-04T19:40:52Z

deps/verifier/Cargo.toml

@@ -49,6 +49,7 @@ strum.workspace = true
 veraison-apiclient = { git = "https://github.com/chendave/rust-apiclient", branch = "token", optional = true }
 ear = { git = "https://github.com/veraison/rust-ear", rev = "43f7f480d09ea2ebc03137af8fbcd70fe3df3468", optional = true }
 x509-parser = { version = "0.14.0", optional = true }
+reqwest = { version="0.12.9", features = ["blocking"] }


I you are already specifying the needed version of reqwest in the global Cargo file, you can just say request = workspace.true here.

Changed. If appropriate, please mark this comment as Resolved.

See Ding's suggestions about the Cargo file as well.

fitzthum · 2024-11-04T19:48:07Z

deps/verifier/src/snp/mod.rs

@@ -84,8 +85,10 @@ impl Verifier for Snp {
            cert_chain,
        } = serde_json::from_slice(evidence).context("Deserialize Quote failed.")?;

-        let Some(cert_chain) = cert_chain else {
-            bail!("Cert chain is unset");
+        let cert_chain = if let Some(chain) = cert_chain{


So the cert chain is type Option<Vec<Cert>> or something. This accounts for cases where the option is none, but what if the option is there but the vector is empty?

Ideally we should get the vcek even if there is a non-empty cert chain as long as the vcek/vlek isn't part of it.

Could this be part of the future follow-up PR?

We can leave check the cert types to a follow-up, but please add a case to this statement to catch an empty vector.

Added the empty vector check

deps/verifier/src/snp/mod.rs

fitzthum · 2024-11-04T19:51:10Z

deps/verifier/src/snp/mod.rs

+) -> Result<Vec<CertTableEntry>, anyhow::Error> {
+    // KDS URL parameters
+    const KDS_CERT_SITE: &str = "https://kdsintf.amd.com";
+    const KDS_VCEK: &str = "/vcek/v1";


might be better to put these consts at the top of the file rather than inside this function

Fixed. If appropriate, please mark this comment as Resolved.

deps/verifier/src/snp/mod.rs

cclaudio · 2024-11-04T20:12:26Z

Cargo.lock

It seems your Cargo.lock has more changes than required. You may need to restore it and apply the request = worspace.true change that Tobin suggested

If appropriate, please mark this comment as Resolved.

deps/verifier/src/snp/mod.rs

Xynnn007 · 2024-11-05T02:24:03Z

Cargo.toml

@@ -36,7 +36,7 @@ jsonwebtoken = { version = "9", default-features = false }
 log = "0.4.17"
 prost = "0.12"
 regorus = { version = "0.1.5", default-features = false, features = ["regex", "base64", "time"] }
-reqwest = { version = "0.12", default-features = false, features = ["default-tls"] }
+reqwest = { version = "0.12", default-features = false, features = ["default-tls", "blocking"] }


I noticed that the reqwest is used in fetch_vcek_from_kds. It is suggested to use reqwest::get (the async version) rather than blocking version, because the blocking version would hang on network I/O during which the process would wait.

I'm able to resolve most comments except this. Could we please have a conversation on slack?

deps/verifier/Cargo.toml

fitzthum · 2024-11-05T14:25:34Z

deps/verifier/src/snp/mod.rs

+            let key = CertTableEntry { cert_type: CertType::VCEK, data: vcek_rsp_bytes };
+            Ok(vec![key])
+        }
+        status => Err(anyhow::anyhow!("Unable to fetch VCEK from URL: {status:?}")),


anyhow::anyhow! -> anyhow!

fitzthum · 2024-11-15T19:55:24Z

deps/verifier/src/snp/mod.rs

+                chain
+            }
+        } else {
+            fetch_vcek_from_kds(report)?


I think you can simplify this by doing something like

let cert_chain = match cert_chain { Some(chain) if !chain.is_empty() => chain, _ => fetch_vcek_from_kds(report)? }

Fetch the VCEK cert from the KDS if it is absent in the cert chain instead of just printing a bail statement stating that the VCEK is not found. Signed-off-by: Adithya Krishnan Kannan <[email protected]>

Xynnn007

Thanks @AdithyaKrishnan ! LGTM. Please see @fitzthum and @cclaudio 's comments.

Xynnn007 · 2024-11-19T02:02:07Z

deps/verifier/src/snp/mod.rs

@@ -307,6 +313,41 @@ fn get_common_name(cert: &x509::X509) -> Result<String> {
    Ok(e.data().as_utf8()?.to_string())
 }

+// Function to request vcek from KDS asynchronously. Return vcek in der format.


We can change // to ///. This would automatically generate a document about the function when we publish the crate to crates.io.

Fixed. Further, would you like me to add descriptions to all the functions in mod.rs?

Per Ding's feedback, I'm testing the use of reqwest asynchronously with get instead of the earlier used blocking version. Signed-off-by: Adithya Krishnan Kannan <[email protected]>

Per Ding's suggestion, I've added a description of each function as part of documenting the code. Signed-off-by: Adithya Krishnan Kannan <[email protected]>

AdithyaKrishnan · 2024-11-19T16:07:17Z

Thanks @AdithyaKrishnan ! LGTM. Please see @fitzthum and @cclaudio 's comments.

@fitzthum and @cclaudio - I believe I've fixed all your concerns. Please let me know if there are any more to be resolved. Further, per Ding's suggestion, I've added a third commit that adds on to the documentation by providing descriptions of all functions.
Hoping this PR can be approved and merged at the earliest.

fitzthum

LGTM

cclaudio

LGTM. Thanks @AdithyaKrishnan

AdithyaKrishnan requested a review from a team as a code owner November 4, 2024 18:17

cclaudio reviewed Nov 4, 2024

View reviewed changes

fitzthum reviewed Nov 4, 2024

View reviewed changes

cclaudio reviewed Nov 4, 2024

View reviewed changes

AdithyaKrishnan force-pushed the main branch from 16e1ff3 to 0494cab Compare November 4, 2024 22:14

Xynnn007 reviewed Nov 5, 2024

View reviewed changes

fitzthum reviewed Nov 5, 2024

View reviewed changes

AdithyaKrishnan force-pushed the main branch 3 times, most recently from 8005c31 to 770fd0c Compare November 15, 2024 19:16

fitzthum reviewed Nov 15, 2024

View reviewed changes

verifier: Fetch VCEK cert from KDS instead of bailing

c1f88fc

Fetch the VCEK cert from the KDS if it is absent in the cert chain instead of just printing a bail statement stating that the VCEK is not found. Signed-off-by: Adithya Krishnan Kannan <[email protected]>

AdithyaKrishnan force-pushed the main branch from 770fd0c to c1f88fc Compare November 18, 2024 20:48

Xynnn007 approved these changes Nov 19, 2024

View reviewed changes

mkulke approved these changes Nov 19, 2024

View reviewed changes

Changing fn fetch_vcek_from_kds() to async

560165d

Per Ding's feedback, I'm testing the use of reqwest asynchronously with get instead of the earlier used blocking version. Signed-off-by: Adithya Krishnan Kannan <[email protected]>

AdithyaKrishnan force-pushed the main branch from 398f2c5 to 560165d Compare November 19, 2024 15:48

Add documentation describing each function

53655ae

Per Ding's suggestion, I've added a description of each function as part of documenting the code. Signed-off-by: Adithya Krishnan Kannan <[email protected]>

AdithyaKrishnan requested review from cclaudio and fitzthum November 19, 2024 17:07

fitzthum approved these changes Nov 19, 2024

View reviewed changes

cclaudio approved these changes Nov 19, 2024

View reviewed changes

fitzthum merged commit 55668c9 into confidential-containers:main Nov 19, 2024
18 checks passed

deeglaze mentioned this pull request Nov 21, 2024

Fetch KDS cert logic fails on non-Milan machines #591

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verifier: Fetch VCEK cert from KDS instead of bailing #555

verifier: Fetch VCEK cert from KDS instead of bailing #555

AdithyaKrishnan commented Nov 4, 2024

cclaudio left a comment

cclaudio Nov 4, 2024

cclaudio Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

fitzthum Nov 5, 2024

AdithyaKrishnan Nov 15, 2024

fitzthum left a comment

fitzthum Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

fitzthum Nov 5, 2024

fitzthum Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

fitzthum Nov 5, 2024

AdithyaKrishnan Nov 15, 2024

fitzthum Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

cclaudio Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

Xynnn007 Nov 5, 2024

AdithyaKrishnan Nov 15, 2024

fitzthum Nov 5, 2024

AdithyaKrishnan Nov 15, 2024

fitzthum Nov 15, 2024 •

edited

Loading

AdithyaKrishnan Nov 18, 2024

Xynnn007 left a comment •

edited

Loading

Xynnn007 Nov 19, 2024

AdithyaKrishnan Nov 19, 2024 •

edited

Loading

AdithyaKrishnan commented Nov 19, 2024

fitzthum left a comment

cclaudio left a comment

verifier: Fetch VCEK cert from KDS instead of bailing #555

verifier: Fetch VCEK cert from KDS instead of bailing #555

Conversation

AdithyaKrishnan commented Nov 4, 2024

cclaudio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdithyaKrishnan Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

AdithyaKrishnan commented Nov 19, 2024

fitzthum left a comment

Choose a reason for hiding this comment

cclaudio left a comment

Choose a reason for hiding this comment

fitzthum Nov 15, 2024 •

edited

Loading

Xynnn007 left a comment •

edited

Loading

AdithyaKrishnan Nov 19, 2024 •

edited

Loading