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.

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

deps/verifier/src/snp/mod.rs

+        StatusCode::OK => {
+            let vcek_rsp_bytes: Vec<u8> =
+                vcek_rsp.bytes().context("Unable to parse VCEK")?.to_vec();
+            let key= CertTableEntry{cert_type: CertType::VCEK, data: vcek_rsp_bytes};


nit: key= -> key =. Also you probably want some spaces around the {. I'm surprised this survived cargo fmt check.

If appropriate, please mark this comment as Resolved.

there are more formatting/lint issues. we do have a problem in the build pipeline, the verifier crate is not linted/formatted.

can you run

cargo fmt cargo clippy -p verifier -- -D warnings

on your code in the mean time and fix the warnings?

Ran both the commands and the latest commit reflects the same

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!

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]> Co-Authored-By: Xynnn_ <[email protected]>

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)? }

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 2 times, most recently from 3baca83 to 8005c31 Compare November 15, 2024 18:38

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

fitzthum reviewed Nov 15, 2024

View reviewed changes

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

fitzthum Nov 4, 2024

AdithyaKrishnan Nov 4, 2024

mkulke Nov 5, 2024

mkulke Nov 5, 2024

AdithyaKrishnan Nov 15, 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

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

Are you sure you want to change the base?

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

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

fitzthum Nov 15, 2024 •

edited

Loading