Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

attester: add TSM REPORT module and move TDX to use it #434

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Jan 16, 2024

No description provided.

@mythi
Copy link
Contributor Author

mythi commented Jan 16, 2024

The CI error does not seem to be related to this PR (I can see it locally with main as well)

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is TSM report module already supported in kernel 6.7-rc1? I suggest that we add some comments inside the code. Also, if we want to leverage the ability, the payload builder side of kata-containers would also make some change. Do you have a plan for this?

attestation-agent/attester/Cargo.toml Outdated Show resolved Hide resolved
attestation-agent/attester/Cargo.toml Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
@mythi
Copy link
Contributor Author

mythi commented Jan 18, 2024

s TSM report module already supported in kernel 6.7-rc1? I suggest that we add some comments inside the code. Also, if we want to leverage the ability, the payload builder side of kata-containers would also make some change. Do you have a plan for this?

Linux 6.7 is available so TSM reports exist. I updated the commit message with the link to the ABI spec. There's no immediate additional changes needed on the Kata side other than just to get 6.7 used. I'm collaborating with @fidencio on that.

@fitzthum
Copy link
Member

Upstream CI issue is fixed btw. Please rebase at some point.

@mythi
Copy link
Contributor Author

mythi commented Jan 18, 2024

Upstream CI issue is fixed btw. Please rebase at some point.

done.

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's disscuss more

attestation-agent/attester/Cargo.toml Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
@mythi
Copy link
Contributor Author

mythi commented Jan 19, 2024

(rebased to trigger CI but no code changes at this time)

@mythi mythi force-pushed the tsm_report branch 2 times, most recently from b4e3780 to 4e190ae Compare January 19, 2024 18:37
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm hoping we can use TSM for SNP as well.

I left a few comments. I think you can probably simplify a lot how the features are used.

attestation-agent/attester/Cargo.toml Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@portersrc portersrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@fitzthum
Copy link
Member

fitzthum commented Feb 2, 2024

This is looking good @mythi. I left a couple more comments.

@mythi
Copy link
Contributor Author

mythi commented Feb 5, 2024

This is looking good @mythi. I left a couple more comments.

@fitzthum great comments, thanks!

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's make sure @Xynnn007 has another look at it

attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
@mythi mythi force-pushed the tsm_report branch 2 times, most recently from 72530e3 to 1bf0fcf Compare February 7, 2024 06:45
@mythi
Copy link
Contributor Author

mythi commented Feb 7, 2024

LGTM. Let's make sure @Xynnn007 has another look at it

@Xynnn007 there was one minor lint error after my updates yesterday but this should be good to go now.

Copy link
Member

@arronwy arronwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mythi LGTM!

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, lgtm. Some coding things to think about

.with_context(|| format!("TDX Attester: get_quote_ioctl() fallback failed after a TSM report error ({notsm})"))
},
|tsm| {
tsm.attestation_report(TsmReportData::Tdx(report_data.clone()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need .clone() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the short answer is 'yes' but I'm open to brainstorm how this gets passed to attestation_report()

attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tsm_report/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/tdx/mod.rs Show resolved Hide resolved
@mythi mythi force-pushed the tsm_report branch 2 times, most recently from 8874143 to e93788a Compare February 8, 2024 11:41
@mythi
Copy link
Contributor Author

mythi commented Feb 8, 2024

Added some tests too.

@mythi
Copy link
Contributor Author

mythi commented Feb 12, 2024

@fitzthum FYI, this has all review comments addressed and approvals from @arronwy and @Xynnn007

Linux 6.7 added a common ABI for CVMs to provide their attestation
reports. It's based on configfs and the quotes can be generated
using the 'TSM reports'.

Documentation:
https://www.kernel.org/doc/Documentation/ABI/testing/configfs-tsm

Add a tsm_report module that CoCo attesters can use to generate
quotes to be included in their attestation evidence.

Signed-off-by: Mikko Ylinen <[email protected]>
Move tdx attester to primarily use TSM reports to get
the quotes generated.

The ioctl() based get-quote mechanisms have never been
upstreamed so they can be considered 'deprecated'. However,
a feature switch is added to keep the old functionality available
for now.

Signed-off-by: Mikko Ylinen <[email protected]>
@mythi
Copy link
Contributor Author

mythi commented Feb 15, 2024

rebased to get CI green

@fitzthum fitzthum merged commit f629e9e into confidential-containers:main Feb 15, 2024
12 checks passed
@fitzthum
Copy link
Member

fitzthum commented Feb 15, 2024

Ok, sorry for the delays on this. Took a while since it touches some important things. We may need some follow-up work (for other platforms, for instance), but this seems like a good start.

Thanks @mythi

@mythi
Copy link
Contributor Author

mythi commented Feb 16, 2024

We may need some follow-up work (for other platforms, for instance), but this seems like a good start.

Agree that follow-up work is needed. Do you think it would be good to have an issue about it? I'm counting at least 4 future work items depending on how they land upstream: SEV, SEV SVSM, RTMR writes, maybe TDX get-report and ioctl deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants