feat: require SEV launch measurements for new GuestOS versions - #11052
feat: require SEV launch measurements for new GuestOS versions#11052r-birkner wants to merge 1 commit into
Conversation
r-birkner
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to f5ba971. Security Overview
Detected Code Changes
|
unreleased_changelog.md has been updated.
There was a problem hiding this comment.
Pull request overview
This PR tightens ReviseElectedGuestosVersions validation so that electing a new GuestOS/replica version requires SEV-SNP launch measurements, preventing versions that cannot be attested from becoming electable.
Changes:
- Treat
guest_launch_measurementsas a required “elect version” parameter (must be set/unset together with the other election parameters) and improve validation errors by listing missing parameters. - Validate provided launch measurements in
ReviseElectedGuestosVersionsPayload::validateto catch malformed data before proposal submission/execution. - Update system/integration tests and changelog to reflect the new requirement.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rs/tests/networking/nns_delegation_test.rs | Passes launch measurements when electing an upgrade version in the networking test flow. |
| rs/registry/canister/unreleased_changelog.md | Documents the new validation requirement and updated rejection behavior/error messaging. |
| rs/registry/canister/src/mutations/do_revise_elected_replica_versions/tests.rs | Adds unit tests covering valid/invalid/missing launch measurement scenarios for the payload. |
| rs/registry/canister/src/mutations/do_revise_elected_replica_versions.rs | Enforces “all-or-nothing” election parameters including measurements and validates measurement contents. |
| rs/nns/integration_tests/src/upgrades_handler.rs | Updates integration tests to include measurements on election and adds negative cases for missing/empty measurements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn guest_launch_measurements_for_test() -> GuestLaunchMeasurements { | ||
| GuestLaunchMeasurements { | ||
| guest_launch_measurements: vec![GuestLaunchMeasurement { | ||
| measurement: vec![0x42; 48], | ||
| metadata: Some(GuestLaunchMeasurementMetadata { | ||
| kernel_cmdline: Some("foo=bar".to_string()), | ||
| vcpu_type: None, | ||
| }), | ||
| }], | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you convert this (0-argument new function) into a lazy_static!?
| fn guest_launch_measurements_for_test() -> GuestLaunchMeasurements { | ||
| GuestLaunchMeasurements { | ||
| guest_launch_measurements: vec![GuestLaunchMeasurement { | ||
| measurement: vec![0x42; 48], | ||
| metadata: Some(GuestLaunchMeasurementMetadata { | ||
| kernel_cmdline: Some("foo=bar".to_string()), | ||
| vcpu_type: None, | ||
| }), | ||
| }], | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there some constant somewhere for the length of this?
| .unwrap_or_default(), | ||
| guest_launch_measurements: elect | ||
| .as_ref() | ||
| .map(|_| guest_launch_measurements_for_test()), |
There was a problem hiding this comment.
map that doesn't actually use the argument is kind of sus. I mean, I have no better idea. This is just highly highly unusual. Yes, existing code already does this. That code is also highly sus.
| guest_launch_measurements: elect | ||
| .as_ref() | ||
| .map(|_| guest_launch_measurements_for_test()), | ||
| replica_version_to_elect: elect, |
There was a problem hiding this comment.
I would have put this first, since this is the thing that's really important. Lead with the lede.
Ofc, the code already put this in a weird position -> do not change in this PR.
There was a problem hiding this comment.
I think the usual pattern here is to inline tests.
I guess Claude noticed this, but Consistency is king. Or did you choose to do this in a separate file?
Don't get me wrong: I love separate file (because it maximizes build caching), but consistency is king. My plan is to do a sweep, and move tests to separate files. But for now, let's be consistent.
| let payload = ReviseElectedGuestosVersionsPayload { | ||
| replica_version_to_elect: Some(REPLICA_VERSION_ID.to_string()), | ||
| guest_launch_measurements: Some(guest_launch_measurements()), | ||
| ..Default::default() |
There was a problem hiding this comment.
This is a very short test, so this is not so important, but try to point out the defect in the input. Here, it's hard to see, there is nothing you can directly point at, because the missing fields are taken care of by this line. Still, you could do something like
| ..Default::default() | |
| // release_package_* fields are missing. | |
| ..Default::default() |
| @@ -104,31 +105,68 @@ pub struct ReviseElectedGuestosVersionsPayload { | |||
| impl ReviseElectedGuestosVersionsPayload { | |||
| pub fn is_electing_a_version(&self) -> Result<bool, String> { | |||
There was a problem hiding this comment.
I know the code was already like this (so, do not fix it), but it is totally wacky for is_whatever to return Result instead of bool. Yes, I understand, it's a Result<bool, ...>, but that is not what people expect from is_whatever, at least I certainly do not.
| // Leave breadcrumbs: which parameters were missing. | ||
| let mut unset_params = Vec::new(); | ||
| for (name, is_set) in elect_params { | ||
| if !is_set { | ||
| unset_params.push(name); | ||
| } | ||
| } |
There was a problem hiding this comment.
You are a good person. Rare.
| Ok(()) | ||
| } else { | ||
| Err("At least one version has to be elected or unelected.".into()) | ||
| if !self.is_electing_a_version()? && !self.is_unelecting_a_version() { |
There was a problem hiding this comment.
| if !self.is_electing_a_version()? && !self.is_unelecting_a_version() { | |
| let is_making_a_change = self.is_electing_a_version()? || self.is_unelecting_a_version() | |
| if !is_making_a_change { |
There was a problem hiding this comment.
Seems verbose. I think all you need to say is
Guest launch measurements is now required (when electing a new Guestos version).
I think this more strongly leads with the led/avoids burying the lede.
Electing a GuestOS version now requires SEV-SNP launch measurements. Without a
measurement, nodes running the version cannot be attested, so a version that
lacks one should never become electable in the first place.
Note for release tooling: a
ReviseElectedGuestosVersionsproposal submittedwithout
--guest-launch-measurements-pathwill now be rejected.