On 10/17/2016 10:11 AM, Pavel Hrdina wrote: > On Fri, Oct 14, 2016 at 04:23:07PM -0400, John Ferlan wrote: >> Add the secret object prior to the chardev tcp so the 'passwordid=' can >> be added if the domain XML has a <secret> for the chardev TLS. >> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> src/qemu/qemu_command.c | 32 ++++++- >> src/qemu/qemu_command.h | 1 + >> src/qemu/qemu_domain.c | 99 +++++++++++++++++++++- >> src/qemu/qemu_domain.h | 16 +++- >> src/qemu/qemu_hotplug.c | 1 + >> src/qemu/qemu_process.c | 6 +- >> ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 +++++++++ >> ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 +++++++++++ >> tests/qemuxml2argvtest.c | 18 ++++ >> 9 files changed, 252 insertions(+), 9 deletions(-) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index aaf7018..b2dfee0 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -695,6 +695,7 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, >> * @tlspath: path to the TLS credentials >> * @listen: boolen listen for client or server setting >> * @verifypeer: boolean to enable peer verification (form of authorization) >> + * @secalias: if one exists, the alias of the security object for passwordid >> * @qemuCaps: capabilities >> * @propsret: json properties to return >> * >> @@ -706,6 +707,7 @@ int >> qemuBuildTLSx509BackendProps(const char *tlspath, >> bool isListen, >> bool verifypeer, >> + const char *secalias, >> virQEMUCapsPtr qemuCaps, >> virJSONValuePtr *propsret) >> { >> @@ -731,6 +733,10 @@ qemuBuildTLSx509BackendProps(const char *tlspath, >> NULL) < 0) >> goto cleanup; >> >> + if (secalias && >> + virJSONValueObjectAdd(*propsret, "s:passwordid", secalias, NULL) < 0) >> + goto cleanup; >> + >> ret = 0; >> >> cleanup: >> @@ -745,6 +751,7 @@ qemuBuildTLSx509BackendProps(const char *tlspath, >> * @tlspath: path to the TLS credentials >> * @listen: boolen listen for client or server setting >> * @verifypeer: boolean to enable peer verification (form of authorization) >> + * @addpasswordid: boolean to handle adding passwordid to object >> * @inalias: Alias for the parent to generate object alias >> * @qemuCaps: capabilities >> * >> @@ -757,6 +764,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, >> const char *tlspath, >> bool isListen, >> bool verifypeer, >> + bool addpasswordid, >> const char *inalias, >> virQEMUCapsPtr qemuCaps) >> { >> @@ -764,11 +772,16 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, >> char *objalias = NULL; >> virJSONValuePtr props = NULL; >> char *tmp = NULL; >> + char *secalias = NULL; >> >> - if (qemuBuildTLSx509BackendProps(tlspath, isListen, verifypeer, >> - qemuCaps, &props) < 0) >> + if (addpasswordid && >> + !(secalias = qemuDomainGetSecretAESAlias(inalias, false))) >> return -1; >> >> + if (qemuBuildTLSx509BackendProps(tlspath, isListen, verifypeer, secalias, >> + qemuCaps, &props) < 0) >> + goto cleanup; >> + >> if (!(objalias = qemuAliasTLSObjFromChardevAlias(inalias))) >> goto cleanup; >> >> @@ -784,6 +797,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, >> virJSONValueFree(props); >> VIR_FREE(objalias); >> VIR_FREE(tmp); >> + VIR_FREE(secalias); >> return ret; >> } >> >> @@ -4946,6 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, >> if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, >> dev->data.tcp.listen, >> cfg->chardevTLSx509verify, >> + !!cfg->chardevTLSx509secretUUID, >> alias, qemuCaps) < 0) >> goto error; >> >> @@ -8542,6 +8557,19 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager, >> >> /* Use -chardev with -device if they are available */ >> if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) { >> + qemuDomainChardevPrivatePtr chardevPriv = >> + QEMU_DOMAIN_CHARDEV_PRIVATE(serial); >> + qemuDomainSecretInfoPtr secinfo = chardevPriv->secinfo; >> + >> + /* Add the secret object first if necessary. The >> + * secinfo is added only to a TCP serial device during >> + * qemuDomainSecretChardevPrepare. Subsequent called >> + * functions can just check the config fields */ >> + if (cfg->chardevTLS && cfg->chardevTLSx509secretUUID && >> + serial->source.type == VIR_DOMAIN_CHR_TYPE_TCP && > > This needs to test also serial->source.data.tcp.haveTLS. > True, of course that depends on the order of the patches too. So if these 4 get accepted, but 1/5 is not accepted, then this would have to move to patch 1 and be handled with that. >> + qemuBuildObjectSecretCommandLine(cmd, secinfo) < 0) >> + return -1; >> + >> if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, >> &serial->source, >> serial->info.alias, >> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h >> index 2f2a6ff..a793fb6 100644 >> --- a/src/qemu/qemu_command.h >> +++ b/src/qemu/qemu_command.h >> @@ -69,6 +69,7 @@ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, >> int qemuBuildTLSx509BackendProps(const char *tlspath, >> bool isListen, >> bool verifypeer, >> + const char *secalias, >> virQEMUCapsPtr qemuCaps, >> virJSONValuePtr *propsret); >> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index a07f981..02c67ae 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -1042,7 +1042,8 @@ qemuDomainSecretSetup(virConnectPtr conn, >> if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && >> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && >> (secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH || >> - secretUsageType == VIR_SECRET_USAGE_TYPE_VOLUME)) { >> + secretUsageType == VIR_SECRET_USAGE_TYPE_VOLUME || >> + secretUsageType == VIR_SECRET_USAGE_TYPE_TLS)) { >> if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, >> secretUsageType, username, >> seclookupdef, isLuks) < 0) >> @@ -1223,6 +1224,91 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, >> } >> >> >> +/* qemuDomainSecretChardevDestroy: >> + * @disk: Pointer to a chardev definition >> + * >> + * Clear and destroy memory associated with the secret >> + */ >> +void >> +qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) >> +{ >> + qemuDomainChardevPrivatePtr chardevPriv = >> + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); >> + >> + if (!chardevPriv || !chardevPriv->secinfo) >> + return; >> + >> + qemuDomainSecretInfoFree(&chardevPriv->secinfo); >> +} >> + >> + >> +/* qemuDomainSecretChardevPrepare: >> + * @conn: Pointer to connection >> + * @driver: Pointer to driver object >> + * @priv: pointer to domain private object >> + * @chardev: Pointer to a chardev definition >> + * >> + * For a serial TCP character device, generate a qemuDomainSecretInfo to be >> + * used by the command line code to generate the secret for the tls-creds >> + * to use. >> + * >> + * Returns 0 on success, -1 on failure >> + */ >> +int >> +qemuDomainSecretChardevPrepare(virConnectPtr conn, >> + virQEMUDriverPtr driver, >> + qemuDomainObjPrivatePtr priv, >> + virDomainChrDefPtr chardev) >> +{ >> + virQEMUDriverConfigPtr cfg; >> + virSecretLookupTypeDef seclookupdef = {0}; >> + qemuDomainSecretInfoPtr secinfo = NULL; >> + >> + if (!conn || chardev->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL || > > The check for !conn is redundant because the declaration uses > ATTRIBUTE_NONNULL for the first attribute. > OK... It's cut-n-paste of Disk and Hostdev Prepare which should also change (separate patch). I actually have quite a few of these built up from a Coverity build which actually checks; whereas, the normal build doesn't. I'll adjust this one and send the other ones separately. >> + chardev->source.type != VIR_DOMAIN_CHR_TYPE_TCP) >> + return 0; >> + >> + cfg = virQEMUDriverGetConfig(driver); >> + if (cfg->chardevTLS && cfg->chardevTLSx509secretUUID) { >> + qemuDomainChardevPrivatePtr chardevPriv = >> + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); >> + >> + if (virUUIDParse(cfg->chardevTLSx509secretUUID, >> + seclookupdef.u.uuid) < 0) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("malformed chardev TLS secret uuid in qemu.conf")); >> + goto error; >> + } >> + seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; >> + >> + if (VIR_ALLOC(secinfo) < 0) >> + goto error; >> + >> + if (qemuDomainSecretSetup(conn, priv, secinfo, chardev->info.alias, >> + VIR_SECRET_USAGE_TYPE_TLS, NULL, >> + &seclookupdef, false) < 0) >> + goto error; >> + >> + if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("TLS X.509 requires encrypted secrets " >> + "to be supported")); >> + goto error; >> + } >> + >> + chardevPriv->secinfo = secinfo; >> + } >> + >> + virObjectUnref(cfg); >> + return 0; >> + >> + error: >> + virObjectUnref(cfg); >> + qemuDomainSecretInfoFree(&secinfo); >> + return -1; >> +} >> + >> + >> /* qemuDomainSecretDestroy: >> * @vm: Domain object >> * >> @@ -1239,11 +1325,15 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) >> >> for (i = 0; i < vm->def->nhostdevs; i++) >> qemuDomainSecretHostdevDestroy(vm->def->hostdevs[i]); >> + >> + for (i = 0; i < vm->def->nserials; i++) >> + qemuDomainSecretChardevDestroy(vm->def->serials[i]); >> } >> >> >> /* qemuDomainSecretPrepare: >> * @conn: Pointer to connection >> + * @driver: Pointer to driver object >> * @vm: Domain object >> * >> * For any objects that may require an auth/secret setup, create a >> @@ -1256,6 +1346,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) >> */ >> int >> qemuDomainSecretPrepare(virConnectPtr conn, >> + virQEMUDriverPtr driver, >> virDomainObjPtr vm) >> { >> qemuDomainObjPrivatePtr priv = vm->privateData; >> @@ -1272,6 +1363,12 @@ qemuDomainSecretPrepare(virConnectPtr conn, >> return -1; >> } >> >> + for (i = 0; i < vm->def->nserials; i++) { >> + if (qemuDomainSecretChardevPrepare(conn, driver, priv, >> + vm->def->serials[i]) < 0) >> + return -1; >> + } >> + >> return 0; >> } >> >> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h >> index 707a995..cc063d1 100644 >> --- a/src/qemu/qemu_domain.h >> +++ b/src/qemu/qemu_domain.h >> @@ -726,11 +726,23 @@ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, >> virDomainHostdevDefPtr hostdev) >> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> >> +void qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) >> + ATTRIBUTE_NONNULL(1); >> + >> +int qemuDomainSecretChardevPrepare(virConnectPtr conn, >> + virQEMUDriverPtr driver, >> + qemuDomainObjPrivatePtr priv, >> + virDomainChrDefPtr chardev) >> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) >> + ATTRIBUTE_NONNULL(4); >> + >> void qemuDomainSecretDestroy(virDomainObjPtr vm) >> ATTRIBUTE_NONNULL(1); >> >> -int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm) >> - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); >> +int qemuDomainSecretPrepare(virConnectPtr conn, >> + virQEMUDriverPtr driver, >> + virDomainObjPtr vm) >> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> >> int qemuDomainDefValidateDiskLunSource(const virStorageSource *src) >> ATTRIBUTE_NONNULL(1); >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index f4d7c17..aad7fa1 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -1734,6 +1734,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, >> if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, >> dev->data.tcp.listen, >> cfg->chardevTLSx509verify, >> + NULL, >> priv->qemuCaps, >> &tlsProps) < 0) >> goto cleanup; >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index 0f5a11b..643741d 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -5092,7 +5092,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, >> size_t i; >> char *nodeset = NULL; >> qemuDomainObjPrivatePtr priv = vm->privateData; >> - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > > The cfg cleanup could be moved to separate patch. > OK. >> virCapsPtr caps; >> >> if (!(caps = virQEMUDriverGetCapabilities(driver, false))) >> @@ -5158,8 +5157,8 @@ qemuProcessPrepareDomain(virConnectPtr conn, >> if (qemuDomainMasterKeyCreate(vm) < 0) >> goto cleanup; >> >> - VIR_DEBUG("Add secrets to disks and hostdevs"); >> - if (qemuDomainSecretPrepare(conn, vm) < 0) >> + VIR_DEBUG("Add secrets to disks, hostdevs, and chardevs"); >> + if (qemuDomainSecretPrepare(conn, driver, vm) < 0) >> goto cleanup; >> >> for (i = 0; i < vm->def->nchannels; i++) { >> @@ -5188,7 +5187,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, >> cleanup: >> VIR_FREE(nodeset); >> virObjectUnref(caps); >> - virObjectUnref(cfg); >> return ret; >> } >> >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args >> new file mode 100644 >> index 0000000..5859c74 >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args >> @@ -0,0 +1,38 @@ >> +LC_ALL=C \ >> +PATH=/bin \ >> +HOME=/home/test \ >> +USER=test \ >> +LOGNAME=test \ >> +QEMU_AUDIO_DRV=none \ >> +/usr/bin/qemu \ >> +-name QEMUGuest1 \ >> +-S \ >> +-object secret,id=masterKey0,format=raw,\ >> +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ >> +-M pc \ >> +-m 214 \ >> +-smp 1,sockets=1,cores=1,threads=1 \ >> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ >> +-nographic \ >> +-nodefconfig \ >> +-nodefaults \ >> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ >> +server,nowait \ >> +-mon chardev=charmonitor,id=monitor,mode=readline \ >> +-no-acpi \ >> +-boot c \ >> +-usb \ >> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ >> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ >> +-chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ >> +localport=1111 \ >> +-device isa-serial,chardev=charserial0,id=serial0 \ >> +-object secret,id=serial1-secret0,\ >> +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ >> +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ >> +-object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ >> +endpoint=client,verify-peer=yes,passwordid=serial1-secret0 \ >> +-chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ >> +tls-creds=objserial1_tls0 \ >> +-device isa-serial,chardev=charserial1,id=serial1 \ >> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml >> new file mode 100644 >> index 0000000..23c244b >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml >> @@ -0,0 +1,50 @@ >> +<domain type='qemu'> >> + <name>QEMUGuest1</name> >> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >> + <memory unit='KiB'>219136</memory> >> + <currentMemory unit='KiB'>219136</currentMemory> >> + <vcpu placement='static'>1</vcpu> >> + <os> >> + <type arch='i686' machine='pc'>hvm</type> >> + <boot dev='hd'/> >> + </os> >> + <clock offset='utc'/> >> + <on_poweroff>destroy</on_poweroff> >> + <on_reboot>restart</on_reboot> >> + <on_crash>destroy</on_crash> >> + <devices> >> + <emulator>/usr/bin/qemu</emulator> >> + <disk type='block' device='disk'> >> + <source dev='/dev/HostVG/QEMUGuest1'/> >> + <target dev='hda' bus='ide'/> >> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> >> + </disk> >> + <controller type='usb' index='0'> >> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> >> + </controller> >> + <controller type='ide' index='0'> >> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> >> + </controller> >> + <controller type='pci' index='0' model='pci-root'/> >> + <serial type='udp'> >> + <source mode='bind' host='127.0.0.1' service='1111'/> >> + <source mode='connect' host='127.0.0.1' service='2222'/> >> + <target port='0'/> >> + </serial> >> + <serial type='tcp'> >> + <source mode='connect' host='127.0.0.1' service='5555' tls='yes'/> >> + <protocol type='raw'/> >> + <target port='0'/> >> + </serial> >> + <console type='udp'> >> + <source mode='bind' host='127.0.0.1' service='1111'/> >> + <source mode='connect' host='127.0.0.1' service='2222'/> >> + <target type='serial' port='0'/> >> + </console> >> + <input type='mouse' bus='ps2'/> >> + <input type='keyboard' bus='ps2'/> >> + <memballoon model='virtio'> >> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> >> + </memballoon> >> + </devices> >> +</domain> >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >> index c65c769..1ca91d3 100644 >> --- a/tests/qemuxml2argvtest.c >> +++ b/tests/qemuxml2argvtest.c >> @@ -1165,6 +1165,24 @@ mymain(void) >> DO_TEST("serial-tcp-tlsx509-chardev-notls", >> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, >> QEMU_CAPS_OBJECT_TLS_CREDS_X509); >> + VIR_FREE(driver.config->chardevTLSx509certdir); >> + if (VIR_STRDUP_QUIET(driver.config->chardevTLSx509certdir, "/etc/pki/libvirt-chardev") < 0) >> + return EXIT_FAILURE; >> + driver.config->chardevTLSx509verify = 1; > > Setting the chardevTLSx509verify is not strictly required to test the generated > command line if secret is required. The verify feature should has its own test > case. > OK, right... Should be a separate patch, although it's almost a two-fer ;-)... Making this change alters the .args file to have 'verify-peer=no' instead of verify-peer=yes. Tks - John > Pavel > >> + if (VIR_STRDUP_QUIET(driver.config->chardevTLSx509secretUUID, >> + "6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea") < 0) >> + return EXIT_FAILURE; >> +# ifdef HAVE_GNUTLS_CIPHER_ENCRYPT >> + DO_TEST("serial-tcp-tlsx509-secret-chardev", >> + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, >> + QEMU_CAPS_OBJECT_SECRET, >> + QEMU_CAPS_OBJECT_TLS_CREDS_X509); >> +# else >> + DO_TEST_FAILURE("serial-tcp-tlsx509-secret-chardev", >> + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, >> + QEMU_CAPS_OBJECT_SECRET, >> + QEMU_CAPS_OBJECT_TLS_CREDS_X509); >> +# endif >> driver.config->chardevTLS = 0; >> VIR_FREE(driver.config->chardevTLSx509certdir); >> DO_TEST("serial-many-chardev", >> -- >> 2.7.4 >> >> -- >> libvir-list mailing list >> libvir-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list