underhill_attestation: allow fallback from GspKey to GspById #2055
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the guest state encryption policy behavior to allow fallback from GspKey to GspById when strict encryption policy is not enabled. The change enables more flexible encryption handling while maintaining security requirements when strict policy is active.
- Updates GspKey policy documentation to reflect fallback behavior
- Modifies encryption requirement logic to consider strict policy setting
- Adjusts logging and warning conditions for policy mismatches
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| vm/devices/get/get_protocol/src/dps_json.rs | Updates documentation for GspKey enum variant to describe fallback behavior |
| openhcl/underhill_attestation/src/lib.rs | Implements fallback logic from GspKey to GspById when strict encryption is disabled |
| || (matches!( | ||
| guest_state_encryption_policy, | ||
| GuestStateEncryptionPolicy::GspKey | ||
| ); | ||
| ) && strict_encryption_policy); |
There was a problem hiding this comment.
[nitpick] The parentheses around the matches! expression create unnecessary complexity. The logical precedence already ensures correct evaluation without the extra parentheses.
Copilot uses AI. Check for mistakes.
| if !matches!( | ||
| guest_state_encryption_policy, | ||
| GuestStateEncryptionPolicy::None | ||
| GuestStateEncryptionPolicy::GspById | GuestStateEncryptionPolicy::Auto |
There was a problem hiding this comment.
[nitpick] The logic has been inverted with !matches! which makes it harder to follow. Consider using positive logic by matching the policies that should trigger the warning: GuestStateEncryptionPolicy::None | GuestStateEncryptionPolicy::GspKey.
Copilot uses AI. Check for mistakes.
| if !matches!( | ||
| guest_state_encryption_policy, | ||
| GuestStateEncryptionPolicy::None | GuestStateEncryptionPolicy::GspById | ||
| GuestStateEncryptionPolicy::GspKey | GuestStateEncryptionPolicy::Auto |
There was a problem hiding this comment.
[nitpick] Similar to the previous comment, the inverted logic with !matches! reduces readability. Consider using positive matching for the policies that should trigger this condition: GuestStateEncryptionPolicy::None | GuestStateEncryptionPolicy::GspById.
Copilot uses AI. Check for mistakes.
tjones60
added
the
backport_2505
|
tbh copilot's feedback is pretty valid here, the !matches is a bit weird. |
It might look weird, but it makes more sense to me: Here I want: If we are not using the encryption method that was requested, log this warning. The positive version is less clear, and could become incorrect in the future if different encryption policies are added (Hardware sealing?) |
tjones60
force-pushed
the
tweak_encryption_policy
branch
from
2fc8dab to
70bf6c7
Compare
I realized I can just swap the if order. |
When strict encryption policy is not enabled, allow the HCL to GspById if GspKey is not available.