chore: Great simplification of SEV key exchange by frankdavid · Pull Request #11354 · dfinity/ic

frankdavid · 2026-08-27T13:19:08Z

We used to keep two keyslots (and two metadata tokens) in the Store's LUKS header. The previous GuestOS's key and the current one. The upgrade VM stored the previous key in a file on the Var partition, and only after it rebooted as the default VM did it unlock the Store with that key and rotate in its own keyslot.

With detached headers, each boot slot carries its own header copy on its Var partition, so we can simplify the passphrase rotation. Each header now holds exactly one keyslot and one metadata token (valid for the corresponding GuestOS), and the previous-key file is gone.

In the new flow, the old GuestOS hands its derived key and header to the upgrade VM; the upgrade VM writes the header to its own Var partition and switches the key to its own derived key right away. This wasn't possible with attached headers because the upgrade VM could not write the shared Store partition.

Benefits:

simpler key rotation
only a single key/token
no need to temporarily store the key on var

…on-simplifications # Conflicts: # Cargo.Bazel.json.lock

frankdavid · 2026-08-27T13:19:50Z


+/// Wrapper around encrypted disk operations to allow testing guest upgrades without setting up
+/// block devices which requires root.
+#[mockall::automock]


Just moved this from the guest_disk crate since it's only useful for testing the upgrade mechanism.

zeropath-ai · 2026-08-27T13:20:04Z

✅ No security or compliance issues detected. Reviewed everything up to 067f88b.

Security Overview

🔎 Scanned files: 18 changed file(s)
🔗 Scan Link: https://zeropath.com/app/repositories/6992424c-495e-4621-b441-efd6821a4be6?scanId=014b3ea1-5d35-42f9-8de8-dab819145d11&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/ic_os/guest_upgrade/client/src/lib.rs Introduce DiskCryptoOps trait and DefaultDiskCryptoOps implementation ► rs/ic_os/guest_upgrade/client/src/main.rs Use DefaultDiskCryptoOps instead of previous SEV store crypto ops ► rs/ic_os/guest_upgrade/client/src/lib.rs Add mockable DiskCryptoOps trait and adapt DiskEncryptionKeyExchangeClientAgent to use Arc<Box>> and adopt_store_artifacts flow ► rs/ic_os/guest_upgrade/tests/src/lib.rs Update tests to use MockDiskCryptoOps and adapt imports/usage ► rs/ic_os/os_tools/guest_disk/src/sev.rs Refactor to use destroy_keyslots_except_first and adapt related logic ► rs/ic_os/os_tools/guest_disk/src/metrics.rs Include num_tokens in luks parameters metrics
Bug Fix / Refactor	► rs/ic_os/os_tools/guest_disk/src/sev.rs Replace can_open_store usage with can_open that uses header location and SEV-derived key ► rs/ic_os/os_tools/guest_disk/src/crypt.rs Add num_tokens tracking for LUKS2 parameters and adjust related logic ► rs/ic_os/os_tools/guest_disk/src/lib.rs Remove deprecated DEFAULT_PREVIOUS_SEV_KEY_PATH constant and related usage ► rs/ic_os/os_tools/guest_disk/src/main.rs Adjust main to stop passing previous_key_path; rely on store header path only ► rs/ic_os/os_tools/guest_disk/Cargo.toml Remove mockall as explicit dependency in several sections ► rs/ic_os/os_tools/guest_disk/BUILD.bazel Update dependencies to reflect removal/addition of mockall and related tools

frankdavid · 2026-08-27T13:21:06Z

-pub fn can_open_store(
+/// Reads the launch measurement and TCB version from the SEV firmware's attestation
+/// report, for storage in the LUKS2 keyslot metadata token.
+fn get_sev_metadata_for_luks(sev_firmware: &mut dyn SevGuestFirmware) -> Result<SevMetadata> {


This used to be method, was moved to be a free-standing function.

Copilot

Pull request overview

Simplifies SEV Store key exchange by immediately re-keying per-slot detached LUKS headers and removing previous-key persistence.

Changes:

Replaces the two-keyslot flow with one keyslot and metadata token.
Moves key-exchange cryptographic operations into the upgrade client.
Adds detached-header migration tests and token-count metrics.

Reviewed changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`rs/ic_os/os_tools/guest_disk/src/tests.rs`	Updates SEV encryption tests.
`rs/ic_os/os_tools/guest_disk/src/sev.rs`	Implements direct header re-keying.
`rs/ic_os/os_tools/guest_disk/src/metrics.rs`	Exports token count.
`rs/ic_os/os_tools/guest_disk/src/main.rs`	Removes previous-key configuration.
`rs/ic_os/os_tools/guest_disk/src/lib.rs`	Removes previous-key path constant.
`rs/ic_os/os_tools/guest_disk/src/crypt.rs`	Adds token counting and legacy cleanup.
`rs/ic_os/os_tools/guest_disk/Cargo.toml`	Removes unused dependencies.
`rs/ic_os/os_tools/guest_disk/BUILD.bazel`	Aligns Bazel dependencies.
`rs/ic_os/guest_upgrade/tests/src/lib.rs`	Updates exchange tests and mocks.
`rs/ic_os/guest_upgrade/client/src/main.rs`	Uses client-owned crypto operations.
`rs/ic_os/guest_upgrade/client/src/lib.rs`	Copies and re-keys received headers.
`rs/ic_os/guest_upgrade/client/Cargo.toml`	Adds mocking dependency.
`rs/ic_os/guest_upgrade/client/BUILD.bazel`	Adds Bazel mocking dependency.
`ic-os/components/guestos/init/setup-encryption/setup-data-encryption.service`	Documents detached-header usage.
`Cargo.lock`	Updates Rust dependency ownership.
`Cargo.Bazel.toml.lock`	Updates Bazel’s Cargo lock data.

Suppressed comments (2)

rs/ic_os/guest_upgrade/client/src/lib.rs:269

The final per-slot header is published before re-keying succeeds. rekey changes keyslot 0 before cleaning legacy slots and replacing metadata, so a failure in either later step leaves this path unlockable by the new key; on retry, can_open returns true and the client signals success without completing that cleanup. Re-key a temporary sibling header and atomically rename it into place only after the entire operation succeeds.

        tokio::fs::write(&self.store_luks_header_path, &luks_header)

rs/ic_os/os_tools/guest_disk/src/crypt.rs:373

Unlike the keyslot-status loop above, this treats every token-status lookup error as a present token. A libcryptsetup/I/O error will therefore be hidden and exported as an inflated num_tokens value instead of making parameter extraction fail. Propagate status errors so the metric cannot report fabricated header state.

    let num_tokens = (0..LUKS2_N_TOKENS)
        .filter(|&token_id| {
            !matches!(
                crypt_device.token_handle().status(token_id),
                Ok(CryptTokenInfo::Invalid) | Ok(CryptTokenInfo::Inactive)
            )
        })
        .count();

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

rs/ic_os/os_tools/guest_disk/src/crypt.rs:374

An error returned by token_handle().status() currently satisfies this negated match and is counted as a present token. That can publish an inflated num_tokens value instead of surfacing that the LUKS header could not be inspected, unlike the keyslot loop above which propagates status errors. Collect/propagate each token status before counting active token entries.

            !matches!(
                crypt_device.token_handle().status(token_id),
                Ok(CryptTokenInfo::Invalid) | Ok(CryptTokenInfo::Inactive)
            )

Bownairo · 2026-08-29T00:15:58Z

        // Increment port for each test case so tests can run in parallel
        let server_port = FREE_PORT.fetch_add(1, Ordering::Relaxed);

+        let can_open_disk = config.can_open_disk;


What do you think about handling rekey the same way? Something like config.should_rekey_explode. I think that saves the Arc and mut stuff, and then all of this would be consistent and handled in the same place.

Bownairo · 2026-08-29T00:58:16Z

+    // TODO: Legacy headers may carry more than one IC key metadata token. Once all nodes
+    // have been updated (i.e., the num_tokens metric is 1 everywhere), this removal can
+    // be deleted so that only one token is written.


I don't follow. Even once we're down to a single key, we still need to clear out the old metadata when we rekey and write a new one, don't we?

frankdavid added 3 commits August 27, 2026 14:43

chore: Great simplification of SEV key exchange

8200d8d

Fix test

a789ea9

Merge remote-tracking branch 'origin/master' into frankdavid/encrypti…

430c449

…on-simplifications # Conflicts: # Cargo.Bazel.json.lock

frankdavid requested a review from a team as a code owner August 27, 2026 13:19

github-actions Bot added the chore label Aug 27, 2026

frankdavid commented Aug 27, 2026

View reviewed changes

frankdavid requested a balanced review from Copilot August 27, 2026 20:59

Copilot started reviewing on behalf of frankdavid August 27, 2026 21:00 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread rs/ic_os/guest_upgrade/tests/src/lib.rs Outdated

Comment thread rs/ic_os/os_tools/guest_disk/src/tests.rs Outdated

Comment thread rs/ic_os/os_tools/guest_disk/src/crypt.rs Outdated

Comment thread rs/ic_os/guest_upgrade/client/src/lib.rs

frankdavid added 3 commits August 28, 2026 12:39

Address copilot comments

b8a4ac8

Improve DiskCryptoOps mocking

bc5a667

Improve DiskCryptoOps mocking

d03f95e

github-actions Bot added the @node label Aug 28, 2026

frankdavid requested a balanced review from Copilot August 28, 2026 16:52

Copilot started reviewing on behalf of frankdavid August 28, 2026 16:52 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread rs/ic_os/guest_upgrade/client/src/lib.rs Outdated

Comment thread rs/ic_os/guest_upgrade/README.md Outdated

Address more copilot

41e7360

frankdavid requested a balanced review from Copilot August 28, 2026 17:50

Copilot started reviewing on behalf of frankdavid August 28, 2026 17:50 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Don't count Err in num_tokens

067f88b

Bownairo reviewed Aug 29, 2026

View reviewed changes

Navigation Menu

Uh oh!

chore: Great simplification of SEV key exchange - #11354

chore: Great simplification of SEV key exchange#11354
frankdavid wants to merge 8 commits into
masterfrom
frankdavid/encryption-simplifications

frankdavid commented Aug 27, 2026

Uh oh!

frankdavid Aug 27, 2026

Uh oh!

zeropath-ai Bot commented Aug 27, 2026 •

edited

Loading

Uh oh!

frankdavid Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Bownairo Aug 29, 2026

Uh oh!

Bownairo Aug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

frankdavid commented Aug 27, 2026

Uh oh!

frankdavid Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

zeropath-ai Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankdavid Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Bownairo Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Bownairo Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zeropath-ai Bot commented Aug 27, 2026 •

edited

Loading