Skip to content

Commit 99354ee

Browse files
cgwalterscgwalters
committed
libvirt: Only inject STORAGE_OPTS when bind-storage-ro is enabled
Previously, STORAGE_OPTS environment configuration was unconditionally injected into VMs via both tmpfiles.d and a systemd service unit. This caused VMs created without `--bind-storage-ro` to reference a non-existent `/run/host-container-storage` path in their environment. Change things to only inject when opts.bind_storage_ro is set. Additionally, refactor to use a single smbios_creds vector instead of separate mount_unit_smbios_creds and storage_opts_creds vectors, making the code cleaner and easier to follow. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>
1 parent ae915e8 commit 99354ee

File tree

2 files changed

+135
-24
lines changed

2 files changed

+135
-24
lines changed

‎crates/integration-tests/src/tests/libvirt_verb.rs‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -955,6 +955,116 @@ fn test_libvirt_run_label_functionality() -> Result<()> {
955955
Ok(())
956956
}
957957

958+
#[distributed_slice(INTEGRATION_TESTS)]
959+
static TEST_LIBVIRT_RUN_NO_STORAGE_OPTS_WITHOUT_BIND_STORAGE: IntegrationTest =
960+
IntegrationTest::new(
961+
"test_libvirt_run_no_storage_opts_without_bind_storage",
962+
test_libvirt_run_no_storage_opts_without_bind_storage,
963+
);
964+
965+
/// Test that STORAGE_OPTS credentials are NOT injected when --bind-storage-ro is not used
966+
fn test_libvirt_run_no_storage_opts_without_bind_storage() -> Result<()> {
967+
let test_image = get_test_image();
968+
969+
// Generate unique domain name for this test
970+
let domain_name = format!(
971+
"test-no-storage-opts-{}",
972+
std::time::SystemTime::now()
973+
.duration_since(std::time::UNIX_EPOCH)
974+
.unwrap()
975+
.as_secs()
976+
);
977+
978+
println!(
979+
"Testing that STORAGE_OPTS are not injected without --bind-storage-ro for domain: {}",
980+
domain_name
981+
);
982+
983+
// Cleanup any existing domain with this name
984+
cleanup_domain(&domain_name);
985+
986+
// Create domain WITHOUT --bind-storage-ro flag
987+
println!("Creating libvirt domain without --bind-storage-ro...");
988+
let create_output = run_bcvk(&[
989+
"libvirt",
990+
"run",
991+
"--name",
992+
&domain_name,
993+
"--label",
994+
LIBVIRT_INTEGRATION_TEST_LABEL,
995+
"--filesystem",
996+
"ext4",
997+
&test_image,
998+
])
999+
.expect("Failed to run libvirt run");
1000+
1001+
println!("Create stdout: {}", create_output.stdout);
1002+
println!("Create stderr: {}", create_output.stderr);
1003+
1004+
if !create_output.success() {
1005+
cleanup_domain(&domain_name);
1006+
panic!(
1007+
"Failed to create domain without --bind-storage-ro: {}",
1008+
create_output.stderr
1009+
);
1010+
}
1011+
1012+
println!("Successfully created domain: {}", domain_name);
1013+
1014+
// Dump the domain XML to verify STORAGE_OPTS credentials are not present
1015+
println!("Dumping domain XML to verify no STORAGE_OPTS credentials...");
1016+
let dumpxml_output = Command::new("virsh")
1017+
.args(&["dumpxml", &domain_name])
1018+
.output()
1019+
.expect("Failed to dump domain XML");
1020+
1021+
if !dumpxml_output.status.success() {
1022+
cleanup_domain(&domain_name);
1023+
let stderr = String::from_utf8_lossy(&dumpxml_output.stderr);
1024+
panic!("Failed to dump domain XML: {}", stderr);
1025+
}
1026+
1027+
let domain_xml = String::from_utf8_lossy(&dumpxml_output.stdout);
1028+
1029+
// Verify that the domain XML does NOT contain STORAGE_OPTS related credentials
1030+
// The bugfix ensures storage_opts_tmpfiles_d_lines() is only added when --bind-storage-ro is true
1031+
// These credentials appear as SMBIOS entries in the domain XML
1032+
1033+
// Check that bcvk-storage-opts is NOT present (this is the systemd unit name)
1034+
assert!(
1035+
!domain_xml.contains("bcvk-storage-opts"),
1036+
"Domain XML should NOT contain bcvk-storage-opts unit when --bind-storage-ro is not used. Found in XML."
1037+
);
1038+
println!("✓ Domain XML does not contain bcvk-storage-opts unit reference");
1039+
1040+
// Check that STORAGE_OPTS environment variable is NOT present in SMBIOS credentials
1041+
assert!(
1042+
!domain_xml.contains("STORAGE_OPTS"),
1043+
"Domain XML should NOT contain STORAGE_OPTS environment variable when --bind-storage-ro is not used. Found in XML."
1044+
);
1045+
println!("✓ Domain XML does not contain STORAGE_OPTS environment variable");
1046+
1047+
// Verify that hoststorage virtiofs tag is NOT present
1048+
assert!(
1049+
!domain_xml.contains("hoststorage"),
1050+
"Domain XML should NOT contain hoststorage virtiofs tag when --bind-storage-ro is not used. Found in XML."
1051+
);
1052+
println!("✓ Domain XML does not contain hoststorage virtiofs filesystem");
1053+
1054+
// Verify that bind-storage-ro metadata is NOT present
1055+
assert!(
1056+
!domain_xml.contains("bootc:bind-storage-ro"),
1057+
"Domain XML should NOT contain bind-storage-ro metadata when flag is not used. Found in XML."
1058+
);
1059+
println!("✓ Domain XML does not contain bind-storage-ro metadata");
1060+
1061+
// Cleanup domain
1062+
cleanup_domain(&domain_name);
1063+
1064+
println!("✓ Test passed: STORAGE_OPTS credentials are correctly excluded when --bind-storage-ro is not used");
1065+
Ok(())
1066+
}
1067+
9581068
#[distributed_slice(INTEGRATION_TESTS)]
9591069
static TEST_LIBVIRT_ERROR_HANDLING: IntegrationTest =
9601070
IntegrationTest::new("test_libvirt_error_handling", test_libvirt_error_handling);

‎crates/kit/src/libvirt/run.rs‎

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -961,12 +961,6 @@ fn create_libvirt_domain_from_disk(
961961
// Generate SMBIOS credential for SSH key injection and systemd environment configuration
962962
// Combine SSH key setup and storage opts for systemd contexts
963963
let mut tmpfiles_content = crate::credentials::key_to_root_tmpfiles_d(&public_key_content);
964-
tmpfiles_content.push_str(&crate::credentials::storage_opts_tmpfiles_d_lines());
965-
let encoded = data_encoding::BASE64.encode(tmpfiles_content.as_bytes());
966-
let smbios_cred = format!("io.systemd.credential.binary:tmpfiles.extra={encoded}");
967-
968-
// Generate SMBIOS credentials for storage opts unit (handles /etc/environment for PAM/SSH)
969-
let storage_opts_creds = crate::credentials::smbios_creds_for_storage_opts()?;
970964

971965
let memory = opts.resolved_memory_mb()?;
972966
let cpus = opts.resolved_cpus()?;
@@ -1063,8 +1057,8 @@ fn create_libvirt_domain_from_disk(
10631057
}
10641058
}
10651059

1066-
// Collect mount unit SMBIOS credentials and unit names
1067-
let mut mount_unit_smbios_creds = Vec::new();
1060+
// Collect SMBIOS credentials and mount unit names
1061+
let mut smbios_creds = Vec::new();
10681062
let mut mount_unit_names = Vec::new();
10691063

10701064
// Process bind mounts (read-write and read-only)
@@ -1073,7 +1067,7 @@ fn create_libvirt_domain_from_disk(
10731067
"bcvk-bind-",
10741068
false,
10751069
domain_builder,
1076-
&mut mount_unit_smbios_creds,
1070+
&mut smbios_creds,
10771071
&mut mount_unit_names,
10781072
)?;
10791073

@@ -1082,7 +1076,7 @@ fn create_libvirt_domain_from_disk(
10821076
"bcvk-bind-ro-",
10831077
true,
10841078
domain_builder,
1085-
&mut mount_unit_smbios_creds,
1079+
&mut smbios_creds,
10861080
&mut mount_unit_names,
10871081
)?;
10881082

@@ -1120,8 +1114,15 @@ fn create_libvirt_domain_from_disk(
11201114
let encoded_mount = data_encoding::BASE64.encode(mount_unit_content.as_bytes());
11211115
let mount_cred =
11221116
format!("io.systemd.credential.binary:systemd.extra-unit.{unit_name}={encoded_mount}");
1123-
mount_unit_smbios_creds.push(mount_cred);
1117+
smbios_creds.push(mount_cred);
11241118
mount_unit_names.push(unit_name);
1119+
1120+
// Add tmpfiles.d lines and systemd service credentials for STORAGE_OPTS
1121+
tmpfiles_content.push_str(&crate::credentials::storage_opts_tmpfiles_d_lines());
1122+
smbios_creds.extend(
1123+
crate::credentials::smbios_creds_for_storage_opts()
1124+
.context("Failed to generate storage opts credentials")?,
1125+
);
11251126
}
11261127

11271128
// Create a single dropin for local-fs.target that wants all mount units
@@ -1133,25 +1134,25 @@ fn create_libvirt_domain_from_disk(
11331134
let dropin_cred = format!(
11341135
"io.systemd.credential.binary:systemd.unit-dropin.local-fs.target~bcvk-mounts={encoded_dropin}"
11351136
);
1136-
mount_unit_smbios_creds.push(dropin_cred);
1137+
smbios_creds.push(dropin_cred);
11371138
}
11381139

1139-
// Build QEMU args with all SMBIOS credentials
1140-
let mut qemu_args = vec![
1141-
"-smbios".to_string(),
1142-
format!("type=11,value={}", smbios_cred),
1143-
];
1140+
let mut qemu_args = Vec::new();
11441141

1145-
// Add storage opts credentials (unit + dropin)
1146-
for storage_cred in storage_opts_creds {
1147-
qemu_args.push("-smbios".to_string());
1148-
qemu_args.push(format!("type=11,value={}", storage_cred));
1142+
// Build QEMU args with all SMBIOS credentials
1143+
{
1144+
let encoded = data_encoding::BASE64.encode(tmpfiles_content.as_bytes());
1145+
let smbios_cred = format!("io.systemd.credential.binary:tmpfiles.extra={encoded}");
1146+
qemu_args.extend([
1147+
"-smbios".to_string(),
1148+
format!("type=11,value={}", smbios_cred),
1149+
]);
11491150
}
11501151

1151-
// Add SMBIOS credentials for mount units
1152-
for mount_cred in mount_unit_smbios_creds {
1152+
// Add all SMBIOS credentials (mount units, storage opts, etc.)
1153+
for cred in smbios_creds {
11531154
qemu_args.push("-smbios".to_string());
1154-
qemu_args.push(format!("type=11,value={}", mount_cred));
1155+
qemu_args.push(format!("type=11,value={}", cred));
11551156
}
11561157

11571158
// Add extra SMBIOS credentials from opts

0 commit comments

Comments
 (0)