From 4128e8a478551ce939a6094032db69e1657d80c4 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 25 Sep 2026 10:27:49 -0400 Subject: [PATCH 1/2] libvirt: Use the secure boot VARS file as the domain's NVRAM With --secure-boot-keys, bcvk wrote _OVMF_VARS.fd into the storage pool and passed it to libvirt as the NVRAM template. libvirt copied it into its own NVRAM file, which `virsh undefine --nvram` removes, but the template was bcvk's and nothing removed it. It was also only created if it didn't exist yet, so a VM re-created under the same name (after `libvirt rm`, or with --replace) silently got the previous VM's enrolled keys instead of the ones passed. A UKI signed with the new keys then fails with "Access Denied", which looks like a signing problem rather than a stale file. Point at the file itself instead of using it as a template, so it is the domain's NVRAM and libvirt removes it on `undefine --nvram`, which rm and rm-all already pass. It is also always regenerated, since libvirt uses an existing NVRAM file as is and a transient VM (which is never undefined) leaves its file behind. Generated-by: AI Signed-off-by: Colin Walters --- crates/kit/src/libvirt/domain.rs | 37 ++++++++++++++-------------- crates/kit/src/libvirt/run.rs | 8 +++--- crates/kit/src/libvirt/secureboot.rs | 32 +++++++++++++----------- 3 files changed, 40 insertions(+), 37 deletions(-) diff --git a/crates/kit/src/libvirt/domain.rs b/crates/kit/src/libvirt/domain.rs index 7115c1efb..4da410560 100644 --- a/crates/kit/src/libvirt/domain.rs +++ b/crates/kit/src/libvirt/domain.rs @@ -52,8 +52,8 @@ pub struct DomainBuilder { tpm: bool, ovmf_code_path: Option, // Custom OVMF_CODE path for secure boot ovmf_code_format: Option, // Format of OVMF_CODE (raw, qcow2) - nvram_template: Option, // Custom NVRAM template with enrolled keys - nvram_format: Option, // Format of NVRAM template (raw, qcow2) + nvram_path: Option, // NVRAM file with enrolled keys + nvram_format: Option, // Format of the NVRAM file (raw, qcow2) firmware_log: Option, // OVMF debug log output via isa-debugcon virtio_console_log: Option, // Virtio console log file path (hvc0 — OS/journald) serial_console_log: Option, // Serial console log file path (ttyS0 — UEFI/bootloader) @@ -89,7 +89,7 @@ impl DomainBuilder { tpm: true, // Default to enabled ovmf_code_path: None, ovmf_code_format: None, - nvram_template: None, + nvram_path: None, nvram_format: None, firmware_log: None, virtio_console_log: None, @@ -190,12 +190,13 @@ impl DomainBuilder { self } - /// Set custom NVRAM template path and format with enrolled secure boot keys + /// Set the NVRAM file path and format, e.g. with enrolled secure boot keys /// + /// The file is used as is, and removed by `virsh undefine --nvram`. /// Format must be specified (either "raw" or "qcow2") and should come from /// the QEMU firmware interop JSON descriptors. - pub fn with_nvram_template(mut self, path: &str, format: &str) -> Self { - self.nvram_template = Some(path.to_string()); + pub fn with_nvram(mut self, path: &str, format: &str) -> Self { + self.nvram_path = Some(path.to_string()); self.nvram_format = Some(format.to_string()); self } @@ -340,21 +341,17 @@ impl DomainBuilder { } writer.write_text_element_with_attrs("loader", ovmf_code, &loader_attrs)?; - // Add NVRAM element if template is specified - if let Some(ref nvram_template) = self.nvram_template { + // Add NVRAM element if a path is specified + if let Some(ref nvram_path) = self.nvram_path { // Format is required and comes from QEMU firmware interop JSON descriptors let nvram_fmt = self .nvram_format .as_deref() - .expect("nvram_format must be set when nvram_template is set"); + .expect("nvram_format must be set when nvram_path is set"); writer.write_text_element_with_attrs( "nvram", - "", // Empty content, template attr provides the source - &[ - ("template", nvram_template), - ("templateFormat", nvram_fmt), - ("format", nvram_fmt), - ], + nvram_path, + &[("format", nvram_fmt)], )?; } } else if secure_boot { @@ -847,16 +844,18 @@ mod tests { .with_name("test-custom-secboot") .with_firmware(FirmwareType::UefiSecure) .with_ovmf_code_path("/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd", "raw") - .with_nvram_template("/var/lib/libvirt/qemu/nvram/custom_VARS.fd", "raw") + .with_nvram("/var/lib/libvirt/images/custom_VARS.fd", "raw") .build_xml() .unwrap(); // Should have custom loader path assert!(xml.contains("/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd")); - // Should have nvram template - assert!(xml.contains("nvram")); - assert!(xml.contains("template=\"/var/lib/libvirt/qemu/nvram/custom_VARS.fd\"")); + // The NVRAM is the custom file itself, not a template libvirt copies + assert!( + xml.contains(r#"/var/lib/libvirt/images/custom_VARS.fd"#) + ); + assert!(!xml.contains("template=")); // Should have secure loader attributes assert!(xml.contains("readonly=\"yes\"")); diff --git a/crates/kit/src/libvirt/run.rs b/crates/kit/src/libvirt/run.rs index 67eaa645e..cb3e46ace 100644 --- a/crates/kit/src/libvirt/run.rs +++ b/crates/kit/src/libvirt/run.rs @@ -1211,10 +1211,10 @@ fn create_libvirt_domain_from_disk( eyre::ensure!(opts.firmware == FirmwareType::UefiSecure); - // Place the OVMF vars file in the libvirt storage pool so it's lifecycled with the VM + // The VARS file is the domain's NVRAM, so `virsh undefine --nvram` removes it let pool_path = get_libvirt_storage_pool_path(global_opts.connect.as_deref()) .context("Failed to get libvirt storage pool path for secure boot vars")?; - let vars_output_path = pool_path.join(format!("{}_OVMF_VARS.fd", domain_name)); + let vars_output_path = pool_path.join(secureboot::vars_filename(domain_name)); info!("Setting up secure boot configuration from {}", keys); let config = secureboot::setup_secure_boot(&keys, &vars_output_path) @@ -1281,14 +1281,14 @@ fn create_libvirt_domain_from_disk( let firmware_info = crate::libvirt::secureboot::find_secure_boot_firmware() .context("Failed to find secure boot firmware")?; let sb_vars_path = sb_config - .vars_template + .vars_path .canonicalize_utf8() .context("Canonicalizing secureboot vars path")?; // Use the formats from the firmware descriptors domain_builder = domain_builder .with_ovmf_code_path(firmware_info.code_path.as_str(), &firmware_info.code_format) - .with_nvram_template(sb_vars_path.as_str(), &sb_config.vars_format); + .with_nvram(sb_vars_path.as_str(), &sb_config.vars_format); // Add secure boot keys path to metadata for reference domain_builder = diff --git a/crates/kit/src/libvirt/secureboot.rs b/crates/kit/src/libvirt/secureboot.rs index a0b7c1082..c891be7b1 100644 --- a/crates/kit/src/libvirt/secureboot.rs +++ b/crates/kit/src/libvirt/secureboot.rs @@ -128,9 +128,9 @@ pub struct FirmwareInfo { pub struct SecureBootConfig { /// Directory containing the secure boot keys pub key_dir: Utf8PathBuf, - /// Path to custom OVMF_VARS template with enrolled keys - pub vars_template: Utf8PathBuf, - /// Format of the NVRAM template file (raw, qcow2) + /// Path to the domain's OVMF_VARS with enrolled keys + pub vars_path: Utf8PathBuf, + /// Format of the OVMF_VARS file (raw, qcow2) pub vars_format: String, /// GUID for the key owner #[allow(dead_code)] @@ -260,10 +260,17 @@ pub fn customize_ovmf_vars( Ok(()) } +/// File name of the per-domain OVMF_VARS with enrolled keys, which is kept in +/// the libvirt storage pool. +pub(crate) fn vars_filename(domain_name: &str) -> String { + format!("{domain_name}_OVMF_VARS.fd") +} + /// Load and setup secure boot configuration from existing keys /// -/// The `vars_output_path` should be in the libvirt storage pool so that -/// the OVMF vars file is lifecycled with the VM (e.g., deleted with `--nvram`). +/// The `vars_output_path` becomes the domain's NVRAM, so libvirt removes it +/// when the domain is undefined with `--nvram`. It is always regenerated: a +/// file left behind (e.g. by a transient VM) may have different keys enrolled. pub fn setup_secure_boot( key_dir: &Utf8Path, vars_output_path: &Utf8Path, @@ -274,19 +281,16 @@ pub fn setup_secure_boot( // Find the system firmware (includes format info) let firmware_info = find_firmware_from_descriptors(true)?; - // Check if custom vars template already exists at the output path - if !vars_output_path.exists() { - tracing::info!( - "Creating custom OVMF_VARS template with enrolled keys at {}", - vars_output_path - ); - customize_ovmf_vars(&keys, &firmware_info.vars_path, vars_output_path)?; - } + tracing::info!( + "Creating OVMF_VARS with enrolled keys at {}", + vars_output_path + ); + customize_ovmf_vars(&keys, &firmware_info.vars_path, vars_output_path)?; // virt-fw-vars preserves the input format, so the output has the same format as the input Ok(SecureBootConfig { key_dir: key_dir.to_owned(), - vars_template: vars_output_path.to_owned(), + vars_path: vars_output_path.to_owned(), vars_format: firmware_info.vars_format, guid: keys.guid, }) From d22d3bca8581826e5bd7242893b446329bc9586e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 28 Sep 2026 16:15:48 -0400 Subject: [PATCH 2/2] tests: Check secure boot keys aren't reused when re-creating a VM Enroll one key set, remove the VM and check its NVRAM file is gone, then put the old file back (as an older bcvk or a transient VM leaves it) and re-create the VM under the same name with another key set: the new PK must be the one enrolled. This is the case where a UKI signed with the new keys failed with "Access Denied". Generated-by: AI Signed-off-by: Colin Walters --- .../src/tests/libvirt_verb.rs | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/crates/integration-tests/src/tests/libvirt_verb.rs b/crates/integration-tests/src/tests/libvirt_verb.rs index e741990ff..ed74e9139 100644 --- a/crates/integration-tests/src/tests/libvirt_verb.rs +++ b/crates/integration-tests/src/tests/libvirt_verb.rs @@ -1201,3 +1201,101 @@ fn test_libvirt_run_journal_output() -> TestResult { Ok(()) } integration_test!(test_libvirt_run_journal_output); + +/// Generate a secure boot key set (PK, KEK, db, GUID.txt) whose certificates +/// have `CN=-`. +fn generate_secure_boot_keys(sh: &xshell::Shell, dir: &std::path::Path, name: &str) -> TestResult { + std::fs::create_dir_all(dir)?; + for key in ["PK", "KEK", "db"] { + let keyfile = dir.join(format!("{key}.key")); + let crtfile = dir.join(format!("{key}.crt")); + let subject = format!("/CN={name}-{key}/"); + cmd!( + sh, + "openssl req -x509 -newkey rsa:2048 -nodes -days 1 -subj {subject} -keyout {keyfile} -out {crtfile}" + ) + .quiet() + .ignore_stderr() + .run()?; + } + std::fs::write(dir.join("GUID.txt"), uuid::Uuid::new_v4().to_string())?; + Ok(()) +} + +/// Recreating a VM under the same name must enroll the new secure boot keys +/// rather than reuse the old VM's OVMF_VARS, and `libvirt rm` must remove +/// that file (libvirt does, as the domain's NVRAM). +fn test_libvirt_secure_boot_vars_not_reused() -> TestResult { + let sh = shell()?; + if cmd!(sh, "virt-fw-vars --help") + .quiet() + .ignore_stdout() + .ignore_stderr() + .run() + .is_err() + { + println!("Skipping test: virt-fw-vars not found"); + return Ok(()); + } + let bck = get_bck_command()?; + let test_image = &get_test_image(); + let label = LIBVIRT_INTEGRATION_TEST_LABEL; + let domain_name = format!("test-sb-vars-{}", random_suffix()); + let tmp = tempfile::TempDir::new()?; + let (keys_a, keys_b) = (tmp.path().join("a"), tmp.path().join("b")); + generate_secure_boot_keys(&sh, &keys_a, "bcvk-test-a")?; + generate_secure_boot_keys(&sh, &keys_b, "bcvk-test-b")?; + + defer! { + cleanup_domain(&domain_name); + } + + let run_with_keys = |keys: &std::path::Path| { + cmd!( + sh, + "{bck} libvirt run --filesystem ext4 --name {domain_name} --label {label} --secure-boot-keys {keys} {test_image}" + ) + .run() + }; + // Returns the NVRAM path and the enrolled PK's subject + let enrolled_pk = || -> anyhow::Result<(String, String)> { + let xml = cmd!(sh, "virsh dumpxml {domain_name}").read()?; + let dom = parse_xml_dom(&xml).map_err(|e| anyhow::anyhow!("parsing domain XML: {e}"))?; + let nvram = dom + .find("nvram") + .map(|n| n.text_content().trim().to_owned()) + .filter(|p| !p.is_empty()) + .ok_or_else(|| anyhow::anyhow!("no nvram path in domain XML"))?; + let vars = cmd!(sh, "virt-fw-vars -i {nvram} --print --verbose") + .ignore_stderr() + .read()?; + let subject = vars + .lines() + .skip_while(|l| !l.starts_with("name=PK ")) + .find_map(|l| l.trim().strip_prefix("subject ")) + .ok_or_else(|| anyhow::anyhow!("no PK enrolled in {nvram}"))? + .to_owned(); + Ok((nvram, subject)) + }; + + run_with_keys(&keys_a)?; + let (nvram, subject) = enrolled_pk()?; + assert_eq!(subject, "CN=bcvk-test-a-PK"); + let stale = tmp.path().join("stale_VARS.fd"); + std::fs::copy(&nvram, &stale)?; + + cmd!(sh, "{bck} libvirt rm --force --stop {domain_name}").run()?; + assert!( + !std::path::Path::new(&nvram).exists(), + "libvirt rm left {nvram} behind" + ); + + // Put the old file back, as an older bcvk or a transient VM leaves it + std::fs::copy(&stale, &nvram)?; + run_with_keys(&keys_b)?; + let (_, subject) = enrolled_pk()?; + assert_eq!(subject, "CN=bcvk-test-b-PK"); + + Ok(()) +} +integration_test!(test_libvirt_secure_boot_vars_not_reused);