attestation_policy: unify the PCR path across platforms#38
attestation_policy: unify the PCR path across platforms#38fangge1212 wants to merge 1 commit intotrusted-execution-clusters:mainfrom
Conversation
| ref.value == actual | ||
| } | ||
|
|
||
| executables := 3 if { |
There was a problem hiding this comment.
This defines executables again, and hardware and configuration are left unchanged. Shouldn't all those be written once, and with the new logic?
There was a problem hiding this comment.
Different platforms expose different fields for validation — for example, the azure_snp platform provides a measurement field, while azure_tdx does not. In my view, we can only unify the PCR validation logic. Each platform should still have its own rules to validate any additional, platform-specific fields.
There was a problem hiding this comment.
Ah, true. However, from some testing from my side, the lower block doesn't seem to fire at all and reduce is also undefined -- had you tested this with anything?
e: and probably other PCR values would still be good? and the one for local TPM are formatted differently if you check at the top
There was a problem hiding this comment.
You are right, I tested on azure tdx, it didn't report any error so I thought it passed. I will update the code and test again
There was a problem hiding this comment.
I updated the code, could you have a try again?
There was a problem hiding this comment.
Okay now I can see where it's going a bit, but I still have some concerns.
- Can this be reconciled with the existing
executables :=block for Azure vTPM SNP? - SNP uses
pcr4(like local), notpcr04like TDX
787485e to
16b9bc2
Compare
16b9bc2 to
d7e72fb
Compare
| ref.value == actual | ||
| } | ||
|
|
||
| executables := 3 if { |
There was a problem hiding this comment.
Okay now I can see where it's going a bit, but I still have some concerns.
- Can this be reconciled with the existing
executables :=block for Azure vTPM SNP? - SNP uses
pcr4(like local), notpcr04like TDX
| get_by_path(obj, path) = result if { | ||
| count(path) == 1 | ||
| result := obj[path[0]] | ||
| } else = result if { | ||
| count(path) == 2 | ||
| result := obj[path[0]][path[1]] | ||
| } |
There was a problem hiding this comment.
Ah, I tried to resolve this functional style like the reduce from the previous draft but Rego isn't actually Turing complete. But this exists: https://www.openpolicyagent.org/docs/policy-reference/builtins/object#builtin-object-objectget
There was a problem hiding this comment.
Can this be reconciled with the existing executables := block for Azure vTPM SNP?
Thanks for reminding me. executables is initially assigned to 33, then reassigned a new value once the validation block passes. To ensure we get a meaningful validation result, we should consolidate all rules for executables into a single block.
SNP uses pcr4 (like local), not pcr04 like TDX
I know that both azure snp and azure tdx use pcr04. Do you mean a local SNP?
Ah, I tried to resolve this functional style like the reduce from the previous draft but Rego isn't actually Turing complete. But this exists: https://www.openpolicyagent.org/docs/policy-reference/builtins/object#builtin-object-objectget
Wow, this works for me: result := object.get(obj, path, false)
|
I would prefer we start with PCR checks duplicated for each platform so that we make it very explicit what we check for each platforms. Reading this I can not easily figure out what would be checked. |
|
Either we manage to dispatch on the platform and have distinct functions for each platform or we have something very linear like: https://github.com/confidential-containers/trustee/blob/main/attestation-service/src/token/ear_default_policy_cpu.rego |
I have same feeling that duplicating the checks for each platform looks more clear. |
d7e72fb to
23aa492
Compare
I will check with Alice to confirm the requirement again. |
|
The reason why I create a ticket for unifying the TPM was that Dehan and I had some mismatches with the PCR checking on Azure and the local development which caused some debugging. TBH, I was more thinking at some trustee refactoring to unify the APIs rather then doing this in our policy, but this valuable here as well. About the trustee changes, it will modify their API so we need to agree the changement with them |
Currently, the PCR formats in the claim is as:
Azure SNP and Azure TDX already have similar format. Then we just need to change the raw TPM format to align with the Azure'sm, it will be fine, right? Also please note that even if we unify the TPM claim format, we would still need platform-specific handling functions in the policy rego. Because each platform can have their own data to validate other than PCRs. |
Previously, attestation policies had to handle different PCR path formats: - Azure SNP: input.azsnpvtpm.tpm.pcr01 - Azure TDX: input.aztdxvtpm.tpm.pcr01 - Local TPM: input.tpm.pcr[1] This change introduces a generic `get_by_path()` helper and rewrites PCR validation rules to dynamically traverse nested objects based on a platform-specific path array. As a result, the same set of rules can now be reused for multiple platforms without duplicating policy logic. Benefits: - Removes platform-specific hardcoding from rules - Simplifies maintenance and extension to future platforms - Makes PCR verification more consistent and extensible Signed-off-by: Fangge Jin <fjin@redhat.com>
Previously, attestation policies had to handle different PCR path formats:
This change introduces a generic
get_by_path()helper and rewrites PCR validation rules to dynamically traverse nested objects based on a platform-specific path array. As a result, the same set of rules can now be reused for multiple platforms without duplicating policy logic.Benefits: