Promote SNP parsed claims#1358
Conversation
200ac02 to
caf8a6f
Compare
d508f54 to
73da71b
Compare
| | `hygon-csv` | Hygon CSV CVM | Confidential VM launch measurement and guest memory integrity; platform firmware and TCB state | | ||
| | `hygon-dcu` | Hygon Deep Computing Unit | Device firmware and security-version posture; device identity | | ||
| | `nvidia-hopper` | NVIDIA Hopper-class GPU attestation | Hopper GPU device firmware and security state | | ||
| | `nvidia-blackwell` | NVIDIA Blackwell-class GPU attestation | Blackwell GPU device firmware and security state | |
There was a problem hiding this comment.
Does it make sense to have separate entries for different generations? We don't do that for CPUs.
There was a problem hiding this comment.
When complex topologies exist, it's possible to distinguish different mounted devices. In fact, we also differentiated CPUs, such as AMD SEV and SNP, although SEV has been removed.
| | `hygon-dcu` | Hygon Deep Computing Unit | Device firmware and security-version posture; device identity | | ||
| | `nvidia-hopper` | NVIDIA Hopper-class GPU attestation | Hopper GPU device firmware and security state | | ||
| | `nvidia-blackwell` | NVIDIA Blackwell-class GPU attestation | Blackwell GPU device firmware and security state | | ||
| | `nvidia-switch-ls10` | NVIDIA LS10 switch attestation | Switch firmware and security posture; device or fabric identity | |
There was a problem hiding this comment.
probably just call this nvidia-nvswitch
There was a problem hiding this comment.
I saw there is Architecture::LS10 in codes. Is ls10 the only switch model serves hopper nvlink?
| #[derive( | ||
| Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash, EnumString, Display, AsRefStr, Copy, | ||
| )] | ||
| pub enum HardwareType { |
There was a problem hiding this comment.
I feel like maybe we should just have the hardware type be a string that is defined by the verifier rather than an enum that we need to centrally control
There was a problem hiding this comment.
This is only for better maintaince.
In fact, we initially attempted to enumerate all supported hardware at the Tee layer within kbs-types. However, with time passes, the Tee became a hybrid of vendor and implementation (for example, Nvidia handles both GPUs and switches; while Intel Tdx and Intel Sgx are two different enums).
Therefore, we needed a separate place to describe individual hardware models.
This allows the trustee to uniformly describe and maintain supported hardware in the documentation. Whenever a vendor needs to add new hardware, they can simply add it here. Verifier ad-hoc support would reduce maintainability.
There was a problem hiding this comment.
I think it's easier to maintain if we don't have a central enum tbh. We basically don't have to maintain anything. Verifiers can just report what they want for the type. I know the idea was to have a more clear scheme for how devices are identified, but idk if we actually know what the scheme should be yet.
|
Based on the discussion during the bi-weekly meeting. I think the most un-certainty for the PR is at which granularity we should define tee types. I get some context about the TDX affairs. They have tdx1.0, tdx1.5 (live migration/updation), tdx2.0 (tee i/o) and more versions. so we can see that different TDX has different capabilities. but they are not sure if version numbers may be iterated, e.g. 2.0 -> 2.1. A practical way might be not to distinguish the generations of different TDX for now. Instead, the claim |
|
Sorry, lost track of this one. I'm not really sure the best approach to the granularity. We could introduce two fields. One for the type and one for the generation. I think that until we figure out a scheme, we should just have the type be a string that is set by the verifier. |
72036ee to
c2cf9e6
Compare
|
The primary goal is to enable users to distinguish between different specific hardware from the claims, as different hardware protects different software and hardware. Another approach is to add a new field to the claims, without needing to ascend the hierarchy. This is because the rego policy supports syntax like
anyway, for TDX/SGX/SNP, which already have different Let me make this PR simpler and start to diving to nvidia things. |
c2cf9e6 to
4d68449
Compare
|
Ok. Sounds good. @hgowda-amd @amd-aliem you might want to take a look at this. Updating the tcb claims does have some downstream impact so let's make sure we have the best possible set of claims. |
| - `snp.evidence.current_tcb`: object. Current platform TCB at report generation time. | ||
| - `snp.evidence.reported_tcb`: object. TCB version used to derive the VCEK/VLEK that signed this report. **Policies should generally evaluate against this set.** | ||
| - `snp.evidence.committed_tcb`: object. Committed TCB version. | ||
| - `snp.evidence.launch_tcb`: object. Platform TCB at guest launch or import time. |
There was a problem hiding this comment.
In the past, I deliberately left out some of these for simplicity's sake. I think we probably should just report one, but it's quite confusing to determine which one is the most important. I actually think the reported tcb is not the best one, despite that being what we used in the past.
There was a problem hiding this comment.
Typically we would use Reported TCB since that is what would be used to fetch the VCEK certificate. I can see customers wanting to check for current TCB. Committed and launch TCB I am not really sure if it has any significance to be added in the report. I can reach out to some folks about this internally and get back.
|
@Xynnn007 we would like to discuss the changes made in this PR internally and evaluate if we have any impact or if these changes suggested makes sense. Please give me a couple of days to discuss it with my teammates and get back to you. |
|
@Xynnn007 Change to format of the generated token would break any relying party using SNP attestation with coco. Do we need to plan to convey that message to any coco customers? or would the release notes suffice this purpose? |
| "measurement": hex::encode(report.measurement), | ||
| "report_data": hex::encode(report.report_data), | ||
| "init_data": hex::encode(report.host_data), | ||
| "report_data": hex::encode(report.report_data), |
There was a problem hiding this comment.
do we need to include every single field in the attestation displayed in the token as well even if there are no rules for it in the policy?
There was a problem hiding this comment.
Yes, I hope so. At least fields that aren't evaluated (e.g., determining a value or size) in vendor-specific code logic (not just verifiers, but also services like TDX/CCA that connect to a remote hardware vendor's service to check firmware versions, SVN, etc.) need to be extracted into the claims.
The given default policy doesn't necessarily need to cover these fields, as it often requires RVPS to provide reference values, which we can't provide all at once. My next step plan is to provide user documentation to assist users in writing their own remote attestation policies. This way, (advanced) users can set conditions for all the fields they care about with more granularity, with all fields available.
There was a problem hiding this comment.
sounds good. thanks for the clarification.
| "vmpl": report.vmpl, | ||
| "sig_algo": report.sig_algo, | ||
| "current_tcb": { | ||
| "fmc": report.current_tcb.fmc, |
There was a problem hiding this comment.
"fmc" TCB measurement is available only in Turin. would this line throw NPE on milan or genoa?
There was a problem hiding this comment.
I added this in the README.md here https://github.com/confidential-containers/trustee/pull/1358/changes#diff-f396654b0e3e0b311c4049fdc3e98825b3f05dc44ecd46e777cfad2a4905e5beR274
it will be null for milan or genoa
|
|
||
| // Generate parsed claims | ||
| let claims = parse_tee_evidence(&attestation_report); | ||
| let claims = parse_tee_evidence(&attestation_report, ProcessorGeneration::Milan); |
There was a problem hiding this comment.
Is there a reason we are hardcoding this?
There was a problem hiding this comment.
The evidence VCEK_REPORT used by the unit test does not have cpuid_fam_id field, but the api of parse_tee_evidence needs the parameter. it's just used to call the parse_tee_evidence function and not used in the test logic.
A better way is to tidy the functions of SNP, to make the api clean for the black boxes.
I can do this in the PR.
|
@Xynnn007 If this is a valid concern, maybe we could leave the old fields and just do an additive change? Still update the default policy example, but token could work with both old and new policies. Or add some validation during policy load time. |
|
@amd-aliem Yes. This is a reasonable concern. Note that the old fields are flattened, and the new format is nested. The reason is that we also want to make the claim's structure consistent with the original evidence, and also bring those 'initdata', 'report_data' fields into a separate place isolated with original evidence fields. In future, I want to move all So what about this:
This is a step-by-step changing, towards a standard way but leave time for users to know. |
4d68449 to
bf71181
Compare
Adds more fields from the SNP evidence to the parsed claims, and use a nested structure for SNP claims. Updated the documents and default policy. Importantly, adds a `snp.generation` claim to tell the generation of the SNP platform. To better pass arguments for generation, adds macro `Copy` to ProcessorGeneration and thus change all references to directly variable. Assisted-by: Cursor Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Close confidential-containers#1427 Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
bf71181 to
d732921
Compare
|
@amd-aliem Updated with add-only way. PTAL |
Hi @Xynnn007 , From the AMD side, we’re still working through which claims need to be matched and how they should be evaluated. Because of that, I don’t think adding all the fields from the report to the default policy is the right approach yet. If you’re okay with it, could we hold off on merging this PR until we come back with a proposal? That would help ensure that both the token format and the set of claims being evaluated remain stable, since changes like these can have a direct impact on relying parties. |
That's fair. |
This is a simple PR to add a hardware-type extension to EAR submod.Also, draft a Trustee EAR profile that we can work on the following.Close #1364This PR adds all fields of SNP report to the claims with nested structure, to provide a more flexible way to customize their policies. Also updates the documents and polcies.