Attestation Report versioning Update

[DGonzalezVillal]

In spec 1.56 of the SEV firmware a new version of the attestation report was introduced.

Here we are introducing a way to version the attestation report that keeps security and backwards compatibility.

The main AttestationReport is now an enum that will contain the different versions of the attestation report. There are 2 new structs for the Attestation Report, one for each version. There is a new trait called Attestable that all the attestation reports will implement, this will allow users to attest their report regardless of the version.

The ReportRsp will now contain raw bytes, rather than the Attestation Report Strucutre. The AttestationReport Enum has a TryFrom bytes that will return the appropriate attestation report version according to the first 4 bytes of the raw data.

Structs consumed by the attestation report that now have new fields depending on the version, are now also versioned, and each report will consume the appropriate version of that struct (look at PlatInfo).

[DGonzalezVillal]

This is a possible solution to the attestation report versioning issue. I would appreciate thoughts and concerns around this approach.

[larrydewey]

One nit

[larrydewey]

CC: @fitzthum

[tylerfanelli]

Some small changes, but overall I think this LGTM and the dealing with different versions of attestation reports is handled nicely.

[fitzthum]

Looks fine. It's important to support different report versions and generally to decouple the version of the crate from the version of the platform.

This will add complexity for users of the crate. It's really handy that both reports can be attested in the same way, but retrieving values from the reports will require some conditional logic (potentially even if it's a value that both report types support). Keep in mind that we are likely to have more report versions as time goes on.

This complexity is sort of inevitable, but there might be ways to mitigate it. For instance, you might introduce another trait with some helper functions that expose values that are common to the report. The reports are mostly the same after all. Maybe there is more that can be shared.

As an aside, there isn't much documentation or many examples for this crate. It's beyond the scope of this PR, but if there some more examples about usage, it might help to understand the scope of this kind of change.

Will this lead to a major release?

[tylerfanelli]

I wonder if it's moreso worthwhile to define AttestationReport as a trait rather than an enum, and have all common values of an attestation report be addressable by trait methods rather than struct members. This may alleviate some of the complexity with dealing with different attestation report versions.

@DGonzalezVillal does something like this on the Attestable trait, where you can access the underlying measurable bytes. But these attestation reports are very similar aside from a few differences. Much of the struct members are identical. Instead, you could impl an AttestationReport trait that users could use to access struct members that are shared between different versions. I don't think these would change that much in new versions.

In this way, you would only have parse the specific version when addressing data that is specific to that version of an attestation report.

The hope would be that for most use cases, users can just have a dyn AttestationReport and access the trait methods, rather than needing to parse the specific version when you are addressing data (like measurable bytes, for example) that is present in both reports.

[tylerfanelli]

My above comment is expanding a bit on what @fitzthum puts succinctly here:

This complexity is sort of inevitable, but there might be ways to mitigate it. For instance, you might introduce another trait with some helper functions that expose values that are common to the report. The reports are mostly the same after all. Maybe there is more that can be shared.

[DGonzalezVillal]

@tylerfanelli @fitzthum @larrydewey Thanks for the comments!

I've gotten similar feedback internally were maybe a trait rather than an enum to access shared fields by the Attestation Reports may lead to easier use and less complexity. I will modify the PR accordingly and re-request a review.

As to what you are saying on usage examples, we would usually implement the changes on snpguest/snphost as an example and we also have a guide that would need to be updated to show people how to use it. Maybe adding examples to the README could be beneficial too?

This will lead to a major release. Some other big changes we are expecting on the next release is the removal of Legacy SEV support moving forward, since we believe in general people will no longer be using it moving forward.

[tylerfanelli]

Lets hold off on this major release until all of those changes (remove SEV module, etc) are in. The updated firmware isn't generally available yet, correct?

[DGonzalezVillal]

@tylerfanelli @fitzthum @larrydewey

Hey everyone,

I spent some time exploring how we could refactor the changes so that different Attestation Report versions implement an AttestationReport trait rather than using an enum. However, this approach introduced more complexity, particularly in the API functions that call the SNP_GET_REPORT IOCTL.

Here’s what the trait might look like:

The trait looks something like this:

pub trait AttestationReport: Display + Attestable {   
    fn version(&self) -> u32;
}

The idea was to define getter functions for the relevant fields and initially implement the get_report function with a return type like this:

 pub fn get_report(
        &mut self,
        message_version: Option<u32>,
        data: Option<[u8; 64]>,
        vmpl: Option<u32>,
    ) -> Result<impl AttestationReport, UserApiError>

However, while building this out, I realized it wouldn’t work because the return type isn’t consistent. The PSP generates different report structures depending on the version, so we can’t return the same concrete type every time. Instead, the return type needs to be a dynamically boxed AttestationReport.

Here’s the resulting implementation of the get_report function:

pub fn get_report(
        &mut self,
        message_version: Option<u32>,
        data: Option<[u8; 64]>,
        vmpl: Option<u32>,
    ) -> Result<Box<dyn AttestationReport>, UserApiError> {
        let mut input = ReportReq::new(data, vmpl)?;
        let mut response = ReportRsp::default();

        let mut request: GuestRequest<ReportReq, ReportRsp> =
            GuestRequest::new(message_version, &mut input, &mut response);

        SNP_GET_REPORT
            .ioctl(&mut self.0, &mut request)
            .map_err(|_| map_fw_err(request.fw_err.into()))?;

        // Make sure response status is successful
        if response.status != 0 {
            Err(FirmwareError::from(response.status))?
        }

        let raw_report = response.report.as_slice();

        let version = u32::from_le_bytes([raw_report[0], raw_report[1], raw_report[2], raw_report[3]]);

        // Return the appropriate report version
        match version {
            2 => {
                let report_v2: AttestationReportV2 = raw_report.as_slice().try_into()?;
                Ok(Box::new(report_v2))
            }
            3 => {
                let report_v3: AttestationReportV3 = raw_report.as_slice().try_into()?;
                Ok(Box::new(report_v3))
            }
            _ => Err(AttestationReportError::UnsupportedReportVersion(version))?,
        }

    }

To enable dynamic dispatch for AttestationReport, the trait must be unsized, which introduces limitations. For example, we can’t use traits like TryFrom directly on it.

While this approach isn’t inherently unworkable, it adds significant complexity. It can make the code harder for users to understand and maintain. For instance, extended report requests would have a return type like this:

pub fn get_ext_report(
        &mut self,
        message_version: Option<u32>,
        data: Option<[u8; 64]>,
        vmpl: Option<u32>,
    ) -> Result<(Box<dyn AttestationReport>, Option<Vec<CertTableEntry>>), UserApiError>

This already triggers a very complex type used Clippy warning.

Given these trade-offs, I still think using an enum is the best approach for handling different versions. We can introduce a separate trait for accessing report fields, as @fitzthum previously suggested, but the API itself should manage version differences using an enum.

Before I proceed further with this approach, I’d like to hear your thoughts!

[DGonzalezVillal]

Lets hold off on this major release until all of those changes (remove SEV module, etc) are in. The updated firmware isn't generally available yet, correct?

This firmware has already been slowly rolled out to customers already. It is still not out for the general public, but people already have access to it.

[tylerfanelli]

While this approach isn’t inherently unworkable, it adds significant complexity. It can make the code harder for users to understand and maintain. For instance, extended report requests would have a return type like this:

pub fn get_ext_report(
        &mut self,
        message_version: Option<u32>,
        data: Option<[u8; 64]>,
        vmpl: Option<u32>,
    ) -> Result<(Box<dyn AttestationReport>, Option<Vec<CertTableEntry>>), UserApiError>

This already triggers a very complex type used Clippy warning.

Given these trade-offs, I still think using an enum is the best approach for handling different versions. We can introduce a separate trait for accessing report fields, as @fitzthum previously suggested, but the API itself should manage version differences using an enum.

Before I proceed further with this approach, I’d like to hear your thoughts!

Whichever way we choose, it will add complexity. Yes, this is a complex type being returned on get_ext_report, but this doesn't convince me it's not the way to go. Doing a match around multiple attestation report types introduces much more complexity to me.

[DGonzalezVillal]

Couple of last thoughts on the dynamic vs enum approach.

Using an enum ensures that the returned value is one of the predefined versions of the AttestationReport. While you may not know the specific version at compile time, you can be confident it will match one of the defined enum variants. For example:

trait AttestationReport {
    fn version(&self) -> u32;
    fn abi_major(&self) -> u8;
    fn v3_unique(&self) -> Option<u8>;
}

/// Trait allows anyone to implement their own structure or functionality, as it is a Trait-only requirement.
struct BogusReport {
    version: u32,
    abi_major: u8,
    v3_unique: u8
}

Using an enum ensures that the returned value is one of the predefined versions of the AttestationReport. While you may not know the specific version at compile time, you can be confident it will match one of the defined enum variants. For example:
Enum:

// Example structures
struct AttestationReportV2 { version: u8}
struct AttestationReportV3 { version: u8, v3_field: u8}

// Still using Attestable trait
trait Attestable {
    fn attest(&self) -> bool;
}

// Example enum
enum AttestationReport {
    V2(AttestationReportV2),
    V3(AttestationReportV3)
}

// Implementing enum
impl Attestable for AttestationReport {
    fn attest(&self) -> bool {
        match self {
            Self::V2(_) => true,
            Self::V3(_) => false
        }
    }
}

// Implementing Getter functions for enum (no need to unwrap for common fields)
impl Test {
    fn version(&self) -> u8 {
        match self {
            Self::V2(report) => report.version,
            Self::V3(report) => report.version
        }
    fn additional_field(&self) -> Option<u8> {
        match self {
            Self::V2(_) => None,
            Self::V3(report) => Some(report.v3_field)
        }
    }
}

Trait:

trait AttestationReport {
    fn version(&self) -> u32;
    fn abi_major(&self) -> u8;
    fn v3_unique(&self) -> Option<u8>;
}

struct AttestationReportV2 {
    version: u32,
    abi_major: u8
}

impl AttestationReport for AttestationReportV2 {
    fn version(&self) -> u32 {
        self.version
    }
    fn abi_major(&self) -> u8 {
        self.abi_major
    }
    fn v3_unique(&self) -> Option<u8> {
        None
    }
}

struct AttestationReportV3 {
    version: u32,
    abi_major: u8,
    v3_unique: u8
}
impl AttestationReport for AttestationReportV3 {
    fn version(&self) -> u32 {
        self.version
    }
    fn abi_major(&self) -> u8 {
        self.abi_major
    }
    fn v3_unique(&self) -> Option<u8> {
        Some(self.v3_unique)
    }
}

As you can see, with traits, repetitive code is required for each implementation of the AttestationReport trait. Enums, on the other hand, allow centralized logic for common operations, reducing boilerplate.

[DGonzalezVillal]

@tylerfanelli

[fitzthum]

@DGonzalezVillal I think you probably will want to have a struct for each report type. It seems like a lot of messing around without that and users probably will want the ability to just deserialize a report of known version and access the fields. To me the most clear way to represent the idea that there are multiple versions with different fields is to have different structs, rather than to put logic into a bunch of different methods that implies this.

I think the common layer, either a trait or another struct, would live on top of that and give users who don't know/care about the version a way to access the fields.

[DGonzalezVillal]

@larrydewey @tylerfanelli @fitzthum

Hey guys, I made some changes and addressed some of the comments. The AttestationReport Enum now works as an interface that the users can use to reach any of the fields. This way they do not have to manually unwrap the report. They can Also use the enum to display the report.

If the field that the user is trying to access on the report is not available in the report version they provided, then an error will be raised. I don't know if you would rather this behavior, or instead just return a None value.

Let me know if you have any other comments.

[tylerfanelli]

@larrydewey We discussed a way for trait Attestable to be public (because enum AttestationReport implements it) yet not allow it to be impl by outside projects. What this able to be accomplished?

[DGonzalezVillal]

Like a pub(crate)?

The reason (honestly a lazy approach) it was made public was that the trait implementations are not being used in the crate when the ssl features are not enabled (attestable is only used in verify functions). Whenever openssl or cyrpto_nossl are not enabled, a dead code warning is raised by attestable. Making it public removes the warning. Instead, since we want it to be a crate only trait, I'll gate it behind the cfg options for ssl, and make it a pub(crate).

[tylerfanelli]

Like a pub(crate)?

The reason (honestly a lazy approach) it was made public was that the trait implementations are not being used in the crate when the ssl features are not enabled (attestable is only used in verify functions). Whenever openssl or cyrpto_nossl are not enabled, a dead code warning is raised by attestable. Making it public removes the warning. Instead, since we want it to be a crate only trait, I'll gate it behind the cfg options for ssl, and make it a pub(crate).

Just to clarify, I would like the trait to be usable (i.e. a user of the crate can call report.measurable_bytes() just fine) yet not implementable (i.e. a user outside of the crate couldn't write impl Attestable for _____). I wasn't sure if this was even possible but @larrydewey indicated it was. If it's not, then no problem, we can go with this. But pub(crate) seems to imply trait Attestable is only usable within the crate itself, which is not desirable.

[DGonzalezVillal]

I see what you mean now. I will look into it, we def do not want pub(crate)

[fitzthum]

Looks good. One note

[tylerfanelli]

Is this ready for another review? Still has a WIP in the PR title.

[DGonzalezVillal]

@larrydewey @tylerfanelli @fitzthum

Please re-review, while doing some testing on the library update, we found a bug around serialization and deserialization and so I've been playing around on the best way to solve it. This commit (7b51076) explains the issue and solution in depth. Please let me know if you have more questions or comments!

[DGonzalezVillal]

Fixed merging issues @tylerfanelli @larrydewey Let me know your thoughts