-
Notifications
You must be signed in to change notification settings - Fork 10
attestation_policy: unify the PCR path across platforms #38
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
base: main
Are you sure you want to change the base?
Conversation
policy/attestation-policy.rego
Outdated
| ref.value == actual | ||
| } | ||
|
|
||
| executables := 3 if { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code, could you have a try again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
policy/attestation-policy.rego
Outdated
| ref.value == actual | ||
| } | ||
|
|
||
| executables := 3 if { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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: