Skip to content

Commit 53e1dd0

Browse files
sudo87dhslove
authored andcommitted
Mask vncPasswd being logged in agent.log (apache#12404)
1 parent f646dc1 commit 53e1dd0

3 files changed

Lines changed: 39 additions & 160 deletions

File tree

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

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
163163
final String target = command.getDestinationIp();
164164
xmlDesc = dm.getXMLDesc(xmlFlag);
165165
if (logger.isDebugEnabled()) {
166-
logger.debug(String.format("VM [%s] with XML configuration [%s] will be migrated to host [%s].", vmName, xmlDesc, target));
166+
logger.debug("VM {} with XML configuration {} will be migrated to host {}.", vmName, maskSensitiveInfoInXML(xmlDesc), target);
167167
}
168168

169169
// Limit the VNC password in case the length is greater than 8 characters
@@ -178,7 +178,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
178178
logger.debug(String.format("Editing mount path of ISO from %s to %s", oldIsoVolumePath, newIsoVolumePath));
179179
xmlDesc = replaceDiskSourceFile(xmlDesc, newIsoVolumePath, vmName);
180180
if (logger.isDebugEnabled()) {
181-
logger.debug(String.format("Replaced disk mount point [%s] with [%s] in Instance [%s] XML configuration. New XML configuration is [%s].", oldIsoVolumePath, newIsoVolumePath, vmName, xmlDesc));
181+
logger.debug("Replaced disk mount point {} with {} in Instance {} XML configuration. New XML configuration is {}.", oldIsoVolumePath, newIsoVolumePath, vmName, maskSensitiveInfoInXML(xmlDesc));
182182
}
183183
}
184184

@@ -209,23 +209,23 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
209209

210210
if (migrateStorage) {
211211
if (logger.isDebugEnabled()) {
212-
logger.debug(String.format("Changing VM [%s] volumes during migration to host: [%s].", vmName, target));
212+
logger.debug("Changing VM {} volumes during migration to host: {}.", vmName, target);
213213
}
214214
xmlDesc = replaceStorage(xmlDesc, mapMigrateStorage, migrateStorageManaged);
215215
if (logger.isDebugEnabled()) {
216-
logger.debug(String.format("Changed VM [%s] XML configuration of used storage. New XML configuration is [%s].", vmName, xmlDesc));
216+
logger.debug("Changed VM {} XML configuration of used storage. New XML configuration is {}.", vmName, maskSensitiveInfoInXML(xmlDesc));
217217
}
218218
migrateDiskLabels = getMigrateStorageDeviceLabels(disks, mapMigrateStorage);
219219
}
220220

221221
Map<String, DpdkTO> dpdkPortsMapping = command.getDpdkInterfaceMapping();
222222
if (MapUtils.isNotEmpty(dpdkPortsMapping)) {
223223
if (logger.isTraceEnabled()) {
224-
logger.trace(String.format("Changing VM [%s] DPDK interfaces during migration to host: [%s].", vmName, target));
224+
logger.trace("Changing VM {} DPDK interfaces during migration to host: {}.", vmName, target);
225225
}
226226
xmlDesc = replaceDpdkInterfaces(xmlDesc, dpdkPortsMapping);
227227
if (logger.isDebugEnabled()) {
228-
logger.debug(String.format("Changed VM [%s] XML configuration of DPDK interfaces. New XML configuration is [%s].", vmName, xmlDesc));
228+
logger.debug("Changed VM {} XML configuration of DPDK interfaces. New XML configuration is {}.", vmName, maskSensitiveInfoInXML(xmlDesc));
229229
}
230230
}
231231

@@ -240,7 +240,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
240240
}
241241

242242
//run migration in thread so we can monitor it
243-
logger.info(String.format("Starting live migration of instance [%s] to destination host [%s] having the final XML configuration: [%s].", vmName, dconn.getURI(), xmlDesc));
243+
logger.info("Starting live migration of instance {} to destination host {} having the final XML configuration: {}.", vmName, dconn.getURI(), maskSensitiveInfoInXML(xmlDesc));
244244
final ExecutorService executor = Executors.newFixedThreadPool(1);
245245
boolean migrateNonSharedInc = command.isMigrateNonSharedInc() && !migrateStorageManaged;
246246

@@ -715,9 +715,7 @@ String replaceIpForVNCInDescFileAndNormalizePassword(String xmlDesc, final Strin
715715
graphElem = graphElem.replaceAll("passwd='([^\\s]+)'", "passwd='" + vncPassword + "'");
716716
}
717717
xmlDesc = xmlDesc.replaceAll(GRAPHICS_ELEM_START + CONTENTS_WILDCARD + GRAPHICS_ELEM_END, graphElem);
718-
if (logger.isDebugEnabled()) {
719-
logger.debug(String.format("Replaced the VNC IP address [%s] with [%s] in VM [%s].", originalGraphElem, graphElem, vmName));
720-
}
718+
logger.debug("Replaced the VNC IP address {} with {} in VM {}.", maskSensitiveInfoInXML(originalGraphElem), maskSensitiveInfoInXML(graphElem), vmName);
721719
}
722720
}
723721
return xmlDesc;
@@ -1036,4 +1034,10 @@ private boolean findSourceNode(Document doc, Node diskNode, String vmName, Strin
10361034
}
10371035
return false;
10381036
}
1037+
1038+
public static String maskSensitiveInfoInXML(String xmlDesc) {
1039+
if (xmlDesc == null) return null;
1040+
return xmlDesc.replaceAll("(graphics\\s+[^>]*type=['\"]vnc['\"][^>]*passwd=['\"])([^'\"]*)(['\"])",
1041+
"$1*****$3");
1042+
}
10391043
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,9 @@ public Answer execute(final StartCommand command, final LibvirtComputingResource
8383
}
8484

8585
libvirtComputingResource.createVifs(vmSpec, vm);
86-
87-
logger.debug("starting " + vmName + ": " + vm.toString());
86+
if (logger.isDebugEnabled()) {
87+
logger.debug("Starting {} : {}", vmName, LibvirtMigrateCommandWrapper.maskSensitiveInfoInXML(vm.toString()));
88+
}
8889
String vmInitialSpecification = vm.toString();
8990
String vmFinalSpecification = performXmlTransformHook(vmInitialSpecification, libvirtComputingResource);
9091
libvirtComputingResource.startVM(conn, vmName, vmFinalSpecification);

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

Lines changed: 22 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,7 @@ private Map<String, MigrateDiskInfo> createMapMigrateStorage(String sourceText,
658658
@Test
659659
public void testReplaceIpForVNCInDescFile() {
660660
final String targetIp = "192.168.22.21";
661-
final String result = libvirtMigrateCmdWrapper.replaceIpForVNCInDescFileAndNormalizePassword(fullfile, targetIp, null, "");
661+
final String result = libvirtMigrateCmdWrapper.replaceIpForVNCInDescFileAndNormalizePassword(fullfile, targetIp, "vncSecretPwd", "");
662662
assertEquals("transformation does not live up to expectation:\n" + result, targetfile, result);
663663
}
664664

@@ -1090,152 +1090,26 @@ public void replaceCdromIsoPathTest() throws ParserConfigurationException, IOExc
10901090
}
10911091

10921092
@Test
1093-
public void updateGpuDevicesIfNeededTestNoGpuDevice() throws Exception {
1094-
Mockito.doReturn(virtualMachineTOMock).when(migrateCommandMock).getVirtualMachine();
1095-
Mockito.doReturn(null).when(virtualMachineTOMock).getGpuDevice();
1096-
1097-
String result = libvirtMigrateCmdWrapper.updateGpuDevicesIfNeeded(migrateCommandMock, xmlWithoutGpuDevices, libvirtComputingResourceMock);
1098-
1099-
Assert.assertEquals("XML should remain unchanged when no GPU device is present", xmlWithoutGpuDevices, result);
1100-
}
1101-
1102-
@Test
1103-
public void updateGpuDevicesIfNeededTestNoDevicesSection() throws Exception {
1104-
List<VgpuTypesInfo> gpuDevices = createTestMixedGpuDevices();
1105-
GPUDeviceTO gpuDeviceTO = Mockito.mock(GPUDeviceTO.class);
1106-
Mockito.doReturn(gpuDevices).when(gpuDeviceTO).getGpuDevices();
1107-
1108-
Mockito.doReturn(virtualMachineTOMock).when(migrateCommandMock).getVirtualMachine();
1109-
Mockito.doReturn(gpuDeviceTO).when(virtualMachineTOMock).getGpuDevice();
1110-
1111-
String result = libvirtMigrateCmdWrapper.updateGpuDevicesIfNeeded(migrateCommandMock, xmlNoDevicesSection, libvirtComputingResourceMock);
1112-
1113-
Assert.assertEquals("XML should remain unchanged when no devices section is found", xmlNoDevicesSection, result);
1114-
}
1115-
1116-
@Test
1117-
public void updateGpuDevicesIfNeededTestWithPciDevice() throws Exception {
1118-
List<VgpuTypesInfo> gpuDevices = createTestPciGpuDevice();
1119-
GPUDeviceTO gpuDeviceTO = Mockito.mock(GPUDeviceTO.class);
1120-
Mockito.doReturn(gpuDevices).when(gpuDeviceTO).getGpuDevices();
1121-
1122-
Mockito.doReturn(virtualMachineTOMock).when(migrateCommandMock).getVirtualMachine();
1123-
Mockito.doReturn(gpuDeviceTO).when(virtualMachineTOMock).getGpuDevice();
1124-
1125-
String result = libvirtMigrateCmdWrapper.updateGpuDevicesIfNeeded(migrateCommandMock, xmlWithGpuDevices, libvirtComputingResourceMock);
1126-
1127-
// Verify that old GPU devices are removed and new ones are added
1128-
Assert.assertFalse("Old PCI device should be removed", result.contains("bus='0x01' slot='0x00'"));
1129-
Assert.assertFalse("Old MDEV device should be removed", result.contains("4b20d080-1b54-4048-85b3-a6a62d165c01"));
1130-
Assert.assertTrue("New PCI device should be added", result.contains("bus=\"0x02\""));
1131-
Assert.assertTrue("New PCI device should be added", result.contains("slot=\"0x00\""));
1132-
Assert.assertTrue("PCI device should have vfio driver", result.contains("name=\"vfio\""));
1133-
}
1134-
1135-
@Test
1136-
public void updateGpuDevicesIfNeededTestWithMdevDevice() throws Exception {
1137-
List<VgpuTypesInfo> gpuDevices = createTestMdevGpuDevice();
1138-
GPUDeviceTO gpuDeviceTO = Mockito.mock(GPUDeviceTO.class);
1139-
Mockito.doReturn(gpuDevices).when(gpuDeviceTO).getGpuDevices();
1140-
1141-
Mockito.doReturn(virtualMachineTOMock).when(migrateCommandMock).getVirtualMachine();
1142-
Mockito.doReturn(gpuDeviceTO).when(virtualMachineTOMock).getGpuDevice();
1143-
1144-
String result = libvirtMigrateCmdWrapper.updateGpuDevicesIfNeeded(migrateCommandMock, xmlWithGpuDevices, libvirtComputingResourceMock);
1145-
1146-
// Verify that old GPU devices are removed and new ones are added
1147-
Assert.assertFalse("Old PCI device should be removed", result.contains("bus='0x01' slot='0x00'"));
1148-
Assert.assertFalse("Old MDEV device should be removed", result.contains("4b20d080-1b54-4048-85b3-a6a62d165c01"));
1149-
Assert.assertTrue("New MDEV device should be added", result.contains("6f20d080-1b54-4048-85b3-a6a62d165c01"));
1150-
Assert.assertTrue("MDEV device should have display=off", result.contains("display=\"off\""));
1151-
}
1152-
1153-
@Test
1154-
public void updateGpuDevicesIfNeededTestWithMixedDevices() throws Exception {
1155-
List<VgpuTypesInfo> gpuDevices = createTestMixedGpuDevices();
1156-
GPUDeviceTO gpuDeviceTO = Mockito.mock(GPUDeviceTO.class);
1157-
Mockito.doReturn(gpuDevices).when(gpuDeviceTO).getGpuDevices();
1158-
1159-
Mockito.doReturn(virtualMachineTOMock).when(migrateCommandMock).getVirtualMachine();
1160-
Mockito.doReturn(gpuDeviceTO).when(virtualMachineTOMock).getGpuDevice();
1161-
1162-
String result = libvirtMigrateCmdWrapper.updateGpuDevicesIfNeeded(migrateCommandMock, xmlWithGpuDevices, libvirtComputingResourceMock);
1163-
1164-
// Verify both PCI and MDEV devices are added
1165-
Assert.assertTrue("PCI device should be added", result.contains("bus=\"0x02\""));
1166-
Assert.assertTrue("PCI device should be added", result.contains("slot=\"0x00\""));
1167-
Assert.assertTrue("MDEV device should be added", result.contains("6f20d080-1b54-4048-85b3-a6a62d165c01"));
1168-
1169-
// Count hostdev elements to ensure we have both
1170-
long hostdevCount = result.lines().filter(line -> line.contains("<hostdev")).count();
1171-
Assert.assertEquals("Should have 2 hostdev elements", 2, hostdevCount);
1172-
}
1173-
1174-
@Test
1175-
public void updateGpuDevicesIfNeededTestRemoveAllGpuDevices() throws Exception {
1176-
List<VgpuTypesInfo> gpuDevices = new ArrayList<>(); // Empty list
1177-
GPUDeviceTO gpuDeviceTO = Mockito.mock(GPUDeviceTO.class);
1178-
Mockito.doReturn(gpuDevices).when(gpuDeviceTO).getGpuDevices();
1179-
1180-
Mockito.doReturn(virtualMachineTOMock).when(migrateCommandMock).getVirtualMachine();
1181-
Mockito.doReturn(gpuDeviceTO).when(virtualMachineTOMock).getGpuDevice();
1182-
1183-
String result = libvirtMigrateCmdWrapper.updateGpuDevicesIfNeeded(migrateCommandMock, xmlWithoutGpuDevices, libvirtComputingResourceMock);
1184-
1185-
// Verify all GPU devices are removed
1186-
Assert.assertFalse("Old PCI device should be removed", result.contains("bus=\"0x01\""));
1187-
Assert.assertFalse("Old PCI device should be removed", result.contains("slot=\"0x00\""));
1188-
Assert.assertFalse("Old MDEV device should be removed", result.contains("4b20d080-1b54-4048-85b3-a6a62d165c01"));
1189-
1190-
// Verify no hostdev elements remain
1191-
long hostdevCount = result.lines().filter(line -> line.contains("<hostdev")).count();
1192-
Assert.assertEquals("Should have no hostdev elements", 0, hostdevCount);
1193-
}
1194-
1195-
// Helper methods for creating test GPU devices
1196-
private List<VgpuTypesInfo> createTestPciGpuDevice() {
1197-
List<VgpuTypesInfo> devices = new ArrayList<>();
1198-
VgpuTypesInfo pciDevice = new VgpuTypesInfo(
1199-
GpuDevice.DeviceType.PCI,
1200-
"NVIDIA Corporation Tesla T4",
1201-
"passthrough",
1202-
"02:00.0", // New bus address for destination host
1203-
"10de",
1204-
"NVIDIA Corporation",
1205-
"1eb8",
1206-
"Tesla T4"
1207-
);
1208-
pciDevice.setDisplay(false);
1209-
devices.add(pciDevice);
1210-
return devices;
1211-
}
1212-
1213-
private List<VgpuTypesInfo> createTestMdevGpuDevice() {
1214-
List<VgpuTypesInfo> devices = new ArrayList<>();
1215-
VgpuTypesInfo mdevDevice = new VgpuTypesInfo(
1216-
GpuDevice.DeviceType.MDEV,
1217-
"nvidia-63",
1218-
"GRID T4-2Q",
1219-
"6f20d080-1b54-4048-85b3-a6a62d165c01", // New UUID for destination host
1220-
"10de",
1221-
"NVIDIA Corporation",
1222-
"1eb8",
1223-
"Tesla T4"
1224-
);
1225-
mdevDevice.setDisplay(false);
1226-
devices.add(mdevDevice);
1227-
return devices;
1228-
}
1229-
1230-
private List<VgpuTypesInfo> createTestMixedGpuDevices() {
1231-
List<VgpuTypesInfo> devices = new ArrayList<>();
1232-
1233-
// Add PCI device
1234-
devices.addAll(createTestPciGpuDevice());
1235-
1236-
// Add MDEV device
1237-
devices.addAll(createTestMdevGpuDevice());
1238-
1239-
return devices;
1093+
public void testMaskVncPwdDomain() {
1094+
// Test case 1: Single quotes
1095+
String xml1 = "<graphics type='vnc' port='5900' passwd='secret123'/>";
1096+
String expected1 = "<graphics type='vnc' port='5900' passwd='*****'/>";
1097+
assertEquals(expected1, LibvirtMigrateCommandWrapper.maskSensitiveInfoInXML(xml1));
1098+
1099+
// Test case 2: Double quotes
1100+
String xml2 = "<graphics type=\"vnc\" port=\"5901\" passwd=\"mypassword\"/>";
1101+
String expected2 = "<graphics type=\"vnc\" port=\"5901\" passwd=\"*****\"/>";
1102+
assertEquals(expected2, LibvirtMigrateCommandWrapper.maskSensitiveInfoInXML(xml2));
1103+
1104+
// Test case 3: Non-VNC graphics (should remain unchanged)
1105+
String xml3 = "<graphics type='spice' port='5902' passwd='notvnc'/>";
1106+
assertEquals(xml3, LibvirtMigrateCommandWrapper.maskSensitiveInfoInXML(xml3));
1107+
1108+
// Test case 4: Multiple VNC entries in one string
1109+
String xml4 = "<graphics type='vnc' port='5900' passwd='a'/>\n" +
1110+
"<graphics type='vnc' port='5901' passwd='b'/>";
1111+
String expected4 = "<graphics type='vnc' port='5900' passwd='*****'/>\n" +
1112+
"<graphics type='vnc' port='5901' passwd='*****'/>";
1113+
assertEquals(expected4, LibvirtMigrateCommandWrapper.maskSensitiveInfoInXML(xml4));
12401114
}
12411115
}

0 commit comments

Comments
 (0)