SNP Attestation #108

tylerfanelli · 2023-01-17T16:06:57Z

No description provided.

slp · 2023-01-24T10:57:58Z

So far is looking really good, minor details aside. Do you want a full review now or should we wait until this PR is no longer a draft?

tylerfanelli · 2023-01-24T17:45:27Z

@slp I don't suppose a full review is necessary yet. I'm fine with your comment, and just wanted to gauge your initial thoughts. Thanks!

tylerfanelli · 2023-02-27T07:06:50Z

@slp This is ready for full review.

examples/launch-tee.c

init/init.c

slp · 2023-03-03T16:12:04Z

init/init.c

-		return NULL;
-	}
-
-	if (mount("securityfs", "/sfs", "securityfs",


We still need this code for SEV/SEV-ES. After opening krun-sev.json, we need to check the "tee" field and either use this code or, for the SNP case, use the new one.

Fixed in dcd5828

slp · 2023-03-03T16:15:37Z

init/tee/kbs/kbs_curl.c

+static int
+KBS_CURL_ERR(char *errmsg)
+{
+        printf("ERROR (kbs_curl_post): %s\n", errmsg);


I'm not entirely sure this function is useful. We're losing the ability to pass format strings (varargs) and it's used from more places than kbs_curl_post, so the function name is confusing.

Probably a macro that uses __func__ to print the function name and supports passing varargs to printf would work better.

Fixed in 28e1566

slp · 2023-03-03T16:19:58Z

init/tee/kbs/kbs_types.c

+
+        kbs_attestation_marshal_tee_pubkey(json, mod, exp);
+
+        sprintf(buf, "\"tee-evidence\":\"{\\\"report\\\":\\\"{");


Is it worth marshaling the entire report into JSON? Seems quite expensive to me when compared to, for instance, treating the entire report as a binary blob, encoding it in an hexadecimal string and sending it to the Attestation Server. Then the AS can decode the hexadecimal string into a [u8] array, and transmute it into a sev::firmware::guest::types::AttestationReport.

Hex encoded in b01b723

slp · 2023-03-03T16:22:52Z

init/tee/snp_attest.h

+
+/*
+ * SNP attestation report structure. Based off of the attestation report
+ * structure described in firmware version 1.51.


Is there any chance they've changed this struct again in 1.52? I had to put a 32 bit padding before report_data to align properly its contents.

Fixed in b01b723

slp

I see some really nice work here. I've tested and works fine, except for the problem with the snp_report misalignment and the measurement comparison (I suspect we might need to update the vmsa blob too).

In addition to the comments in the code, I'd also suggest that, for consistency with init.c, please use tabs instead of spaces and always use curly braces even when the conditional has a single statement (we should also put a coding style somewhere).

tylerfanelli · 2023-03-05T05:01:49Z

@slp Another note. I'll be removing the certificate code and modifying SNP_GET_EXT_REPORT to SNP_GET_REPORT, since we're basically leaving it up to the attestation server to retrieve the certificates.

tylerfanelli · 2023-03-07T06:53:12Z

init/tee/kbs/kbs_types.c

-        sprintf(buf, "\\\\\\\"plat_info\\\\\\\":%lu,", report->platform_info);
+        OPENSSL_buf2hexstr_ex(report_hexstr, 0x1000, &report_hexstr_len,
+                (unsigned char *) report, sizeof(*report), '\0');
+        report_hexstr[report_hexstr_len] = '\0';


TODO: Is this required? Or does OpenSSL null-terminate the string?

tylerfanelli · 2023-03-07T19:45:12Z

@slp Can you re-review? I've added changes fixing all of your original concerns.

slp · 2023-03-09T17:21:22Z

I've been testing this and it's working great after fixing the SNP measurement generation in libkrunfw. I've also confirmed still works fine with SEV/SEV-ES.

Could you please rebase so we can merge it?

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

For SNP, we don't care much for krun_set_tee_config(), as the attestation is done from the guest. Instead, wrtie the TEE config file to the data disk and allow the guest to read it from the init binary. Also including an example TEE config disk image "snp-config-data-disk.img" with the workload ID "test" and attestation URL of localhost to allow for a user to test the attestation functionality. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

With the updates allowing a TEE example to submit an disk image containing a TEE config file, we can now mount the image and read the TEE config in the init binary. We do this (and parse the TEE config) for the workload ID and attestation URL of the guest. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

The initial phase of a KBS attestation is the CHALLENGE. The CHALLENGE serves as the function that an attestation server uses to generate an ephermal nonce and send it back to the client for inclusion in the SNP attestation report. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

Given the nonce from the attestation server (KBS CHALLENGE), include this nonce in the attestation and retrieve an attestation report. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

The TEE public key is used for validating a guest's attestation report with the attestation server. The attestation's response will eventually be encrypted with this public key, so only the guest will be able to decrypt the message. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

In the ATTESTATION phase, the guest will marshal of its input data (SNP attestation report, certificate chain, etc..) and send it to the KBS attestation server to attest the workload. If there is no error reported for this step, a client can assume that the attestation was successful. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

A JSON string is returned from the attestation server (specifically, a JSON string of a kbs_types::Challenge). Parse this struct to read the string value of the nonce. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

…ssphrase Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

With firmware update 1.52, the firmware required extra padding bits to align properly. Furthermore, instead of traversing each field and writing it out to the JSON buffer, use OpenSSL to hex encode the report before sending to the attestation server. Due to the fact that we now submit the SNP generation (which allows the attestation server to retrieve the certificate chain), we don't need to send the certificate chain to the attestation server from libkrun. Thus, only grab the report using SNP_GET_REPORT instead of SNP_GET_EXT_REPORT. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

tylerfanelli · 2023-03-09T19:44:12Z

@slp Rebased :) thanks!

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

tylerfanelli requested review from crobinso and slp as code owners January 17, 2023 16:06

tylerfanelli marked this pull request as draft January 17, 2023 16:08

tylerfanelli force-pushed the init_snp branch from 86f5ed5 to a7ac571 Compare February 14, 2023 19:39

tylerfanelli marked this pull request as ready for review February 14, 2023 19:40

tylerfanelli marked this pull request as draft February 15, 2023 06:20

tylerfanelli force-pushed the init_snp branch 2 times, most recently from 7290354 to 401ed7e Compare February 21, 2023 03:11

tylerfanelli force-pushed the init_snp branch from 401ed7e to 908d172 Compare February 27, 2023 07:05

tylerfanelli marked this pull request as ready for review February 27, 2023 07:05

tylerfanelli force-pushed the init_snp branch from 908d172 to 83af19c Compare February 27, 2023 07:08

slp reviewed Mar 3, 2023

View reviewed changes

examples/launch-tee.c Show resolved Hide resolved

slp reviewed Mar 3, 2023

View reviewed changes

init/init.c Outdated Show resolved Hide resolved

slp reviewed Mar 3, 2023

View reviewed changes

slp requested changes Mar 3, 2023

View reviewed changes

tylerfanelli force-pushed the init_snp branch from 83af19c to 4ef7d0d Compare March 7, 2023 06:51

tylerfanelli commented Mar 7, 2023

View reviewed changes

tylerfanelli force-pushed the init_snp branch from 4ef7d0d to b01b723 Compare March 8, 2023 03:36

tylerfanelli added 3 commits March 9, 2023 14:42

lib: Document where virtio-blk failed (root or data block) on error

dcfabcd

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

tylerfanelli added 10 commits March 9, 2023 14:43

init/tee: Retrieve an SNP attestation report with given nonce

789dc9b

Given the nonce from the attestation server (KBS CHALLENGE), include this nonce in the attestation and retrieve an attestation report. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

init/tee: Parse nonce from KBS server challenge

3323d61

A JSON string is returned from the attestation server (specifically, a JSON string of a kbs_types::Challenge). Parse this struct to read the string value of the nonce. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

init/tee: Implement functionality to retrieve and decrypt workload pa…

90c7e62

…ssphrase Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

init: Determine which get_luks_passphrase to use from "tee" config field

0e08184

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

kbs/curl: Remove KBS_CURL_ERR for macro

b99647b

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

kbs/attest: Include generation in SnpAttestation JSON

31d7199

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

tylerfanelli force-pushed the init_snp branch from b01b723 to 39e2aa1 Compare March 9, 2023 19:43

devices: Fix clippy errors

3953995

Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

tylerfanelli force-pushed the init_snp branch from b666543 to 3953995 Compare March 10, 2023 05:25

slp approved these changes Mar 10, 2023

View reviewed changes

slp merged commit 10dcdf1 into containers:main Mar 10, 2023

tylerfanelli deleted the init_snp branch March 29, 2024 03:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNP Attestation #108

SNP Attestation #108

tylerfanelli commented Jan 17, 2023 •

edited

Loading

slp commented Jan 24, 2023

tylerfanelli commented Jan 24, 2023

tylerfanelli commented Feb 27, 2023

slp Mar 3, 2023

tylerfanelli Mar 7, 2023

slp Mar 3, 2023

tylerfanelli Mar 7, 2023

slp Mar 3, 2023

tylerfanelli Mar 7, 2023 •

edited

Loading

slp Mar 3, 2023

tylerfanelli Mar 7, 2023 •

edited

Loading

slp left a comment

tylerfanelli commented Mar 5, 2023

tylerfanelli Mar 7, 2023

tylerfanelli commented Mar 7, 2023

slp commented Mar 9, 2023

tylerfanelli commented Mar 9, 2023


		kbs_attestation_marshal_tee_pubkey(json, mod, exp);

		sprintf(buf, "\"tee-evidence\":\"{\\\"report\\\":\\\"{");

SNP Attestation #108

SNP Attestation #108

Conversation

tylerfanelli commented Jan 17, 2023 • edited Loading

slp commented Jan 24, 2023

tylerfanelli commented Jan 24, 2023

tylerfanelli commented Feb 27, 2023

slp Mar 3, 2023

Choose a reason for hiding this comment

tylerfanelli Mar 7, 2023

Choose a reason for hiding this comment

slp Mar 3, 2023

Choose a reason for hiding this comment

tylerfanelli Mar 7, 2023

Choose a reason for hiding this comment

slp Mar 3, 2023

Choose a reason for hiding this comment

tylerfanelli Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

slp Mar 3, 2023

Choose a reason for hiding this comment

tylerfanelli Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

slp left a comment

Choose a reason for hiding this comment

tylerfanelli commented Mar 5, 2023

tylerfanelli Mar 7, 2023

Choose a reason for hiding this comment

tylerfanelli commented Mar 7, 2023

slp commented Mar 9, 2023

tylerfanelli commented Mar 9, 2023

tylerfanelli commented Jan 17, 2023 •

edited

Loading

tylerfanelli Mar 7, 2023 •

edited

Loading

tylerfanelli Mar 7, 2023 •

edited

Loading