Azure detection + attestation report fetch #16

tylerfanelli · 2023-07-25T05:10:13Z

This PR introduces two new features:

Detecting if snpguest is running inside of a Azure CVM
If so, fetching the attestation report from the vTPM, rather than the /dev/sev-guest device

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

tylerfanelli · 2023-07-25T05:24:41Z

Closes #5

tylerfanelli · 2023-07-25T05:25:48Z

CC @berrange

src/hyperv/mod.rs

berrange · 2023-07-25T07:57:23Z

src/report.rs

-        .get_report(None, Some(request_data), args.vmpl)
-        .context("Failed to get report.")?;
+    let att_report = if hv {
+        hyperv::report::get()?


For args.vmpl we need to mandate that it is 0, because the Azure HCL report is created at VMPL 0

I'm not sure what todo for args.random - we can't honour the current semantics for either true or false value - the data won't be random, and the user also can't provide their own data. Basically the report data is fixed at what HCL added, which is the TPM attestation key. We should however still write out the fixed HCL report data to args.request_file I think

So, we can fail if VMPL > 1 && Hyper-V report is detected. Should the command fail if args.request_file is given with a message Hyper-V request data is fixed and cannot be changed, do not submit a request file as input? The same could be said with args.random. Maybe these args should be re-considered for the command if so.

Essentially we have three scenarios

User provides a request file as input

User requests random data as input

Input is pre-determined by platform

We could add a third argument --platform which is mutually exclusive with --random

--request-data - user provided input, unchanged on output

--random - auto-generated input, output ./random-request-file.txt

--random, --request-data PATH - auto-generated input, output $PATH

--platform - pre-determined input, output ./platform-request-file.txt

--platform, --request-data PATH - pre-detrermined input, output $PATH

In 0c80f34 I removed the --random flag altogether. The user is instead required to give a "request file" as an argument.

For regular guests, the data in the request data file will be used as the data in the attestation report.

For Hyper-V guests, the platform data already present in the report is written to the request file.

Although a bit unavoidable here, I don't like the request-file arg being interpreted differently based on what platform you're running on. What do you think?

In 0c80f34 I removed the --random flag altogether. The user is instead required to give a "request file" as an argument.

Being able to omit the request file and just have the tool create a random nonce was desirable behaviour IMHO

Although a bit unavoidable here, I don't like the request-file arg being interpreted differently based on what platform you're running on. What do you think?

I think it is highly undesirable, because the behaviour is not discoverable / reportable.

With the 5 scenarios I illustrated above, each combination of args has clearly defined platform independent semantics. The command will either complete with the documented semantics, or an error will be reported if the platform can't honour that particular combination of args.

I second @berrange comment

In 0c80f34 I removed the --random flag altogether. The user is instead required to give a "request file" as an argument.

For regular guests, the data in the request data file will be used as the data in the attestation report.

For Hyper-V guests, the platform data already present in the report is written to the request file.

Although a bit unavoidable here, I don't like the request-file arg being interpreted differently based on what platform you're running on. What do you think?

@tylerfanelli was there a reason we wanted to remove this option? I must have missed this from my quick review. I agree with @berrange, this is a nice feature for users who care less about generating the request data, and just want to attest the report.

@larrydewey The motivation to remove --random was to "clean up" the arguments of this command and try to give some uniformity, so that the experience was the same regardless if one was running on Azure or not. Corner cases such as

--random and --platform are mutually exclusive

--random must fail on Azure

--platform must fail on non-Azure
And the option inclusion of --request-file. I don't want to confuse users on the different flags and why/how they are used. If --random is desirable however, I'll change this to use --platform, and document it's prospective use cases (i.e. in Azure)

I removed the commit and added the --platform flag. To try and complicate things less, a user is required to input the name of a file to write the request data, rather than defaulting to {random, platform}-request-data.txt. I can add support for those "default" files as well if the feature is desirable?

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

larrydewey

CC @DGonzalezVillal Just a heads up on the API changes made here.

DGonzalezVillal

I just have a couple of questions/comments on the workflow, and the certificates command. Everything else looks good

DGonzalezVillal · 2023-07-27T20:53:33Z

src/main.rs

@@ -51,9 +53,10 @@ fn main() -> Result<()> {
    env_logger::init();

    let snpguest = SnpGuest::from_args();
+    let hv = hyperv::present();


Do we want to check for Hyper-V every single time the tool is used? Why wouldn't we just check whenever people request a report?

There will be parity with other commands.

I agree, I think it would make more sense to only check for Hyper-V when we are potentially going to interact with the PSP. All other functionality from the tool does not rely on architecture. The only other locations I could potentially think this check would be useful is for enabling/disabling --random and/or extended certs, as Hyper-V disallows the use of these flags

An example I currently see is certs. In non-Azure, certs are fetched from the KDS. However, in Azure they are fetched from another MS-defined endpoint.

Makes sense

src/main.rs

src/certs.rs

larrydewey

Revoking approval until discussion is complete.

Currently, with Hyper-V guests, the report_data of an attestation report is generated beforehand by the platform. This indicates that a user cannot write some optional data to be included in the report. With --platform enabled, the pre-generated platform data will be written to the file pointed to by $REQUEST_PATH (or "platform-request-file.txt" if no $REQUEST_PATH is given as input). Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>

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

larrydewey

LGTM

src/hyperv/mod.rs

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

DGonzalezVillal

LGTM

tylerfanelli added 4 commits July 24, 2023 18:18

verify: Fix clippy warning

3af8a24

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

Reformat with cargo fmt

c685f16

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

hyperv: Detect if in a Hyper-V SEV-SNP guest

92c21b6

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

hyperv: Fetch attestation report from Azure vTPM

e704493

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

berrange reviewed Jul 25, 2023

View reviewed changes

src/hyperv/mod.rs Outdated Show resolved Hide resolved

berrange reviewed Jul 25, 2023

View reviewed changes

tylerfanelli added 2 commits July 25, 2023 13:21

hyperv: Check for hypervisor guest + Hyper-V functions

b8983b4

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

hyperv: Require VMPL 0 for attestation report

c7ad0fe

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

tylerfanelli force-pushed the azure branch from d823a7d to c7ad0fe Compare July 25, 2023 18:05

larrydewey approved these changes Jul 25, 2023

View reviewed changes

tylerfanelli force-pushed the azure branch 2 times, most recently from 0c80f34 to 60fd74e Compare July 27, 2023 18:48

tylerfanelli requested a review from larrydewey July 27, 2023 18:49

larrydewey approved these changes Jul 27, 2023

View reviewed changes

DGonzalezVillal reviewed Jul 27, 2023

View reviewed changes

larrydewey requested changes Jul 28, 2023

View reviewed changes

tylerfanelli force-pushed the azure branch 2 times, most recently from 9c36a17 to 1ad76b1 Compare August 1, 2023 01:04

tylerfanelli added 2 commits July 31, 2023 22:59

docs: Document report --platform flag

deea7e3

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

tylerfanelli force-pushed the azure branch from 1ad76b1 to deea7e3 Compare August 1, 2023 03:02

tylerfanelli requested review from berrange, larrydewey and DGonzalezVillal August 1, 2023 03:02

larrydewey approved these changes Aug 4, 2023

View reviewed changes

src/hyperv/mod.rs Outdated Show resolved Hide resolved

azure: Use mutable variable instead of consistently recreating

49a6f10

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

DGonzalezVillal approved these changes Aug 7, 2023

View reviewed changes

tylerfanelli merged commit 4aaf540 into virtee:main Aug 8, 2023

tylerfanelli deleted the azure branch June 11, 2024 02:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure detection + attestation report fetch #16

Azure detection + attestation report fetch #16

tylerfanelli commented Jul 25, 2023

tylerfanelli commented Jul 25, 2023

tylerfanelli commented Jul 25, 2023

berrange Jul 25, 2023

tylerfanelli Jul 25, 2023

berrange Jul 25, 2023 •

edited

Loading

tylerfanelli Jul 27, 2023

berrange Jul 28, 2023

DGonzalezVillal Jul 28, 2023

larrydewey Jul 28, 2023

tylerfanelli Jul 28, 2023

tylerfanelli Aug 1, 2023 •

edited

Loading

larrydewey left a comment

DGonzalezVillal left a comment

DGonzalezVillal Jul 27, 2023

tylerfanelli Jul 27, 2023

larrydewey Jul 28, 2023 •

edited

Loading

tylerfanelli Jul 28, 2023

larrydewey Aug 4, 2023

larrydewey left a comment

larrydewey left a comment

DGonzalezVillal left a comment

Azure detection + attestation report fetch #16

Azure detection + attestation report fetch #16

Conversation

tylerfanelli commented Jul 25, 2023

tylerfanelli commented Jul 25, 2023

tylerfanelli commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berrange Jul 25, 2023 • edited Loading

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

tylerfanelli Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

larrydewey left a comment

Choose a reason for hiding this comment

DGonzalezVillal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrydewey Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrydewey left a comment

Choose a reason for hiding this comment

larrydewey left a comment

Choose a reason for hiding this comment

DGonzalezVillal left a comment

Choose a reason for hiding this comment

berrange Jul 25, 2023 •

edited

Loading

tylerfanelli Aug 1, 2023 •

edited

Loading

larrydewey Jul 28, 2023 •

edited

Loading