Skip to content

Commit d32f570

Browse files
jmsperujmsperu
committed
address review (#12898): restore fails closed when the encryption state cannot be read
isEncryptedImage() now throws when qemu-img info returns nothing usable, so a restore never falls back to rsync for a file that might be LUKS-encrypted. qemu-img check exit 3 (leaked clusters) is accepted on restore as well; 2 still reports the backup as corrupt. Tests cover both, and the existing tests now stub a real plain-qcow2 qemu-img info result instead of relying on a null.
1 parent 1f596ed commit d32f570

2 files changed

Lines changed: 131 additions & 17 deletions

File tree

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper<RestoreBa
6060
private static final String FILE_PATH_PLACEHOLDER = "%s/%s";
6161
/** id of the qemu secret object that carries the LUKS passphrase on the qemu-img command line. */
6262
private static final String LUKS_SECRET_ID = "sec0";
63+
/** qemu-img check exit status for an image with leaked clusters only (no corruption). */
64+
private static final int QEMU_IMG_CHECK_LEAKS = 3;
6365

6466
// Flattens the backing-file chain into a single self-contained qcow2 written to the
6567
// destination volume path. Used when the source backup is an incremental whose qcow2
@@ -293,22 +295,41 @@ private String getBackupPath(String mountDirectory, String backupPath, String ba
293295
}
294296

295297
private boolean checkBackupFileImage(String backupPath, File keyFile) {
298+
int exitValue;
296299
if (!isEncryptedImage(backupPath)) {
297-
int exitValue = Script.runSimpleBashScriptForExitValue(String.format("qemu-img check %s", backupPath));
298-
return exitValue == 0;
300+
exitValue = Script.runSimpleBashScriptForExitValue(String.format("qemu-img check %s", backupPath));
301+
} else {
302+
List<String> cmd = new ArrayList<>(List.of("qemu-img", "check"));
303+
cmd.addAll(encryptedSourceArgs(backupPath, keyFile));
304+
exitValue = Script.executeCommandForExitValue(cmd.toArray(new String[0]));
299305
}
300-
List<String> cmd = new ArrayList<>(List.of("qemu-img", "check"));
301-
cmd.addAll(encryptedSourceArgs(backupPath, keyFile));
302-
return Script.executeCommandForExitValue(cmd.toArray(new String[0])) == 0;
306+
return isQemuImgCheckPass(exitValue, backupPath);
307+
}
308+
309+
/**
310+
* qemu-img check exits 0 for a clean image and 3 for leaked clusters, which waste space but leave the
311+
* data intact; both are a pass. 2 (corruption), 1 (check could not complete) and anything else fail.
312+
*/
313+
private boolean isQemuImgCheckPass(int exitValue, String backupPath) {
314+
if (exitValue == QEMU_IMG_CHECK_LEAKS) {
315+
logger.warn("qemu-img check found leaked clusters in backup file {}; the data is intact, continuing", backupPath);
316+
}
317+
return exitValue == 0 || exitValue == QEMU_IMG_CHECK_LEAKS;
303318
}
304319

305320
/**
306321
* True when qemu reports the backup qcow2 as encrypted (LUKS, produced by nasbackup.sh {@code -e}).
307-
* Reading the header needs no secret, so this works before any passphrase is involved.
322+
* Reading the header needs no secret, so this works before any passphrase is involved. Fails closed:
323+
* when qemu-img cannot read the header at all, the restore stops here instead of treating the file as
324+
* plain and copying a possibly encrypted image onto the volume.
308325
*/
309326
private boolean isEncryptedImage(String backupPath) {
310327
String info = Script.executeCommand("qemu-img", "info", "--output=json", backupPath);
311-
return info != null && info.replaceAll("\\s", "").contains("\"encrypted\":true");
328+
if (info == null || !info.contains("\"format\"")) {
329+
throw new CloudRuntimeException(String.format("Could not read the image header of backup file [%s] with qemu-img info, "
330+
+ "so it is not known whether it is encrypted; refusing to restore it.", backupPath));
331+
}
332+
return info.replaceAll("\\s", "").contains("\"encrypted\":true");
312333
}
313334

314335
/**

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java‎

Lines changed: 103 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,11 @@
5757
@RunWith(MockitoJUnitRunner.class)
5858
public class LibvirtRestoreBackupCommandWrapperTest {
5959

60+
/** Trimmed {@code qemu-img info --output=json} of a plain qcow2: no top-level "encrypted" key. */
61+
private static final String PLAIN_QCOW2_INFO = "{\"virtual-size\": 1048576, \"filename\": \"volume-123\", \"format\": \"qcow2\"}";
62+
/** Same for a LUKS-encrypted qcow2 written by nasbackup.sh -e. */
63+
private static final String ENCRYPTED_QCOW2_INFO = "{\"virtual-size\": 1048576, \"filename\": \"volume-123\", \"format\": \"qcow2\", \"encrypted\": true}";
64+
6065
private LibvirtRestoreBackupCommandWrapper wrapper;
6166
private LibvirtComputingResource libvirtComputingResource;
6267
private RestoreBackupCommand command;
@@ -96,7 +101,7 @@ public void testExecuteWithVmExistsNull() throws Exception {
96101
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
97102
.thenAnswer(invocation -> invocation.getArgument(0));
98103
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
99-
.thenReturn(null);
104+
.thenReturn(PLAIN_QCOW2_INFO);
100105
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
101106
.thenReturn(0);
102107
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
@@ -141,7 +146,7 @@ public void testExecuteWithVmExistsTrue() throws Exception {
141146

142147
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
143148
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
144-
.thenReturn(null);
149+
.thenReturn(PLAIN_QCOW2_INFO);
145150
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
146151
.thenReturn(0);
147152
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
@@ -182,7 +187,7 @@ public void testExecuteWithVmExistsFalse() throws Exception {
182187

183188
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
184189
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
185-
.thenReturn(null);
190+
.thenReturn(PLAIN_QCOW2_INFO);
186191
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
187192
.thenReturn(0);
188193
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
@@ -230,6 +235,8 @@ public void testExecuteWithCifsMountType() throws Exception {
230235
scriptMock.when(() ->
231236
Script.executePipedCommands(anyList(), anyLong()))
232237
.thenReturn(new Pair<>(0, "vda")); // Current device
238+
// qemu-img info reads the header; a plain qcow2 has no "encrypted" key
239+
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn(PLAIN_QCOW2_INFO);
233240

234241
filesMock.when(() -> Files.deleteIfExists(any(Path.class))).thenReturn(true);
235242

@@ -308,7 +315,7 @@ public void testExecuteWithBackupFileNotFound() throws Exception {
308315

309316
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
310317
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
311-
.thenReturn(null);
318+
.thenReturn(PLAIN_QCOW2_INFO);
312319
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
313320
.thenAnswer(invocation -> {
314321
String command = invocation.getArgument(0);
@@ -357,7 +364,7 @@ public void testExecuteWithCorruptBackupFile() throws Exception {
357364

358365
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
359366
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
360-
.thenReturn(null);
367+
.thenReturn(PLAIN_QCOW2_INFO);
361368
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
362369
.thenAnswer(invocation -> {
363370
String command = invocation.getArgument(0);
@@ -410,7 +417,7 @@ public void testExecuteWithRsyncFailure() throws Exception {
410417
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
411418
.thenAnswer(invocation -> invocation.getArgument(0));
412419
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
413-
.thenReturn(null);
420+
.thenReturn(PLAIN_QCOW2_INFO);
414421
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
415422
.thenAnswer(invocation -> {
416423
if (Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("rsync"::equals)) {
@@ -472,7 +479,7 @@ public void testExecuteWithAttachVolumeFailure() throws Exception {
472479
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
473480
.thenAnswer(invocation -> invocation.getArgument(0));
474481
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
475-
.thenReturn(null);
482+
.thenReturn(PLAIN_QCOW2_INFO);
476483
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
477484
.thenAnswer(invocation -> {
478485
if (Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("attach-disk"::equals)) {
@@ -573,7 +580,7 @@ public void testExecuteWithMultipleVolumes() throws Exception {
573580
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
574581
.thenReturn(0); // All commands success
575582
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
576-
.thenReturn(null);
583+
.thenReturn(PLAIN_QCOW2_INFO);
577584
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
578585
.thenReturn(0); // All commands success
579586

@@ -642,7 +649,7 @@ public void testEncryptedBackupIsCheckedAndDecryptedWithTheSecret() throws Excep
642649
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
643650
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class))).thenReturn(0);
644651
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
645-
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn("{\"encrypted\": true}");
652+
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn(ENCRYPTED_QCOW2_INFO);
646653
// mount/umount also run through executeCommandForExitValue (#14006); only qemu-img matters here
647654
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
648655
.thenAnswer(inv -> { recordQemuImg(qemuImgCalls, joinArgs(inv.getArguments(), 0)); return 0; });
@@ -663,6 +670,91 @@ public void testEncryptedBackupIsCheckedAndDecryptedWithTheSecret() throws Excep
663670
}
664671
}
665672

673+
private void stubPlainNfsRestoreOfExistingVm() {
674+
when(command.getVmName()).thenReturn("test-vm");
675+
when(command.getBackupPath()).thenReturn("backup/path");
676+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
677+
when(command.getBackupRepoType()).thenReturn("nfs");
678+
when(command.getMountOptions()).thenReturn("rw");
679+
when(command.isVmExists()).thenReturn(true);
680+
when(command.getWait()).thenReturn(60);
681+
PrimaryDataStoreTO primaryDataStore = Mockito.mock(PrimaryDataStoreTO.class);
682+
when(primaryDataStore.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
683+
when(command.getRestoreVolumePools()).thenReturn(Arrays.asList(primaryDataStore));
684+
when(command.getRestoreVolumePaths()).thenReturn(Arrays.asList("/var/lib/libvirt/images/volume-123"));
685+
when(command.getBackupVolumesUUIDs()).thenReturn(Arrays.asList("volume-123"));
686+
when(command.getBackupFiles()).thenReturn(Arrays.asList("volume-123"));
687+
when(command.getMountTimeout()).thenReturn(30);
688+
}
689+
690+
@Test
691+
public void testRestoreFailsClosedWhenEncryptionStateCannotBeDetermined() throws Exception {
692+
stubPlainNfsRestoreOfExistingVm();
693+
List<String> copyCalls = new ArrayList<>();
694+
695+
try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
696+
Path tempPath = Mockito.mock(Path.class);
697+
when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
698+
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);
699+
filesMock.when(() -> Files.deleteIfExists(any(Path.class))).thenReturn(true);
700+
701+
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
702+
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString())).thenAnswer(inv -> inv.getArgument(0));
703+
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
704+
// qemu-img info failed (missing binary, unreadable file, timeout): Script returns null
705+
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn(null);
706+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
707+
.thenAnswer(inv -> { copyCalls.add(joinArgs(inv.getArguments(), 1)); return 0; });
708+
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
709+
.thenAnswer(inv -> { copyCalls.add(joinArgs(inv.getArguments(), 0)); return 0; });
710+
711+
BackupAnswer answer = (BackupAnswer) wrapper.execute(command, libvirtComputingResource);
712+
713+
Assert.assertFalse("restore must fail when the encryption state is unknown", answer.getResult());
714+
Assert.assertTrue(answer.getDetails(), answer.getDetails().contains("refusing to restore"));
715+
Assert.assertTrue("nothing may be copied onto the volume: " + copyCalls,
716+
copyCalls.stream().noneMatch(c -> c.startsWith("rsync") || c.startsWith("qemu-img convert")));
717+
}
718+
}
719+
}
720+
721+
@Test
722+
public void testRestoreAcceptsLeakedClustersAsAPass() throws Exception {
723+
stubPlainNfsRestoreOfExistingVm();
724+
725+
try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
726+
Path tempPath = Mockito.mock(Path.class);
727+
when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
728+
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);
729+
filesMock.when(() -> Files.deleteIfExists(any(Path.class))).thenReturn(true);
730+
731+
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
732+
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString())).thenAnswer(inv -> inv.getArgument(0));
733+
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn(PLAIN_QCOW2_INFO);
734+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class))).thenReturn(0);
735+
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class))).thenReturn(0);
736+
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString())).thenAnswer(inv -> {
737+
String cmd = inv.getArgument(0);
738+
if (cmd.startsWith("qemu-img check")) {
739+
return 3; // leaked clusters only
740+
}
741+
if (cmd.contains("qemu-img info") && cmd.contains("backing-filename")) {
742+
return 1; // no backing chain: plain full backup
743+
}
744+
return 0;
745+
});
746+
747+
BackupAnswer answer = (BackupAnswer) wrapper.execute(command, libvirtComputingResource);
748+
Assert.assertTrue(answer.getDetails(), answer.getResult());
749+
750+
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(Mockito.startsWith("qemu-img check"))).thenReturn(2);
751+
BackupAnswer corrupt = (BackupAnswer) wrapper.execute(command, libvirtComputingResource);
752+
Assert.assertFalse("a corrupt backup (exit 2) must still be rejected", corrupt.getResult());
753+
Assert.assertTrue(corrupt.getDetails(), corrupt.getDetails().contains("is corrupt"));
754+
}
755+
}
756+
}
757+
666758
private String invokeGetDeviceToAttachDisk(String vmName) throws Exception {
667759
Method method = LibvirtRestoreBackupCommandWrapper.class.getDeclaredMethod("getDeviceToAttachDisk", String.class);
668760
method.setAccessible(true);
@@ -730,7 +822,7 @@ public void testEncryptedBackupWithoutPassphraseFailsClearly() throws Exception
730822
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
731823
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class))).thenReturn(0);
732824
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
733-
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn("{\"encrypted\": true}");
825+
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn(ENCRYPTED_QCOW2_INFO);
734826
List<String> qemuImgCalls = new ArrayList<>();
735827
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
736828
.thenAnswer(inv -> { recordQemuImg(qemuImgCalls, joinArgs(inv.getArguments(), 1)); return 0; });
@@ -907,6 +999,7 @@ public void testUnmountIsBoundedByTheConfiguredTimeout() throws Exception {
907999
return 0;
9081000
});
9091001
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
1002+
scriptMock.when(() -> Script.executeCommand(any(String[].class))).thenReturn(PLAIN_QCOW2_INFO);
9101003

9111004
wrapper.execute(command, libvirtComputingResource);
9121005

0 commit comments

Comments
 (0)