[libvirt] [PATCH 0/2] Use secret objects to pass iSCSI passwords
Fairly simple exercise since similar processing is used for RBD, just needed to check for the proper capabililty and then use it and test that it works. NB 1: Depending on order of submit either this patch will need to add the capability flag into caps_2.10.0 or the other patch will need to add it... Plan is to wait for the other patch to be ACK'd and then add the capability here NB 2: "Theoretically speaking" one could/should also consider the <auth> for the JSON virstoragefile source parse backing code; however, <auth> is not a <source> property rather it is a <disk> property and thus stored in QEMU_DOMAIN_DISK_PRIVATE. But for both RBD and iSCSI the "user" and "password-secret" options are -drive properties that could be parsed and handled (ok well mostly handled). In any case, the RBD code doesn't handle it (for now), so neither will the iSCSI code. John Ferlan (2): qemu: Get capabilities for iscsi usage of password-secret argument qemu: Use secret objects to pass iSCSI passwords src/qemu/qemu_capabilities.c | 4 ++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 12 files changed, 206 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml -- 2.9.5
Add the capability to use the blockdev-add query-qmp-schema option to find the 'password-secret' parameter that will allow the iSCSI code to use the master secret object (a/k/a AES) to encrypt the secret in an object and only need to provide the object id of tha secret on the command line thus obsfuscating the passphrase. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 5 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7ea6f4..86f27ee 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -439,6 +439,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-net.tx_queue_size", "chardev-reconnect", "virtio-gpu.max_outputs", + + /* 270 */ + "iscsi.password-secret", ); @@ -1811,6 +1814,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIntelIOMMU[] = { static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "blockdev-add/arg-type/options/+gluster/debug-level", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, { "blockdev-add/arg-type/+gluster/debug", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, + { "blockdev-add/arg-type/+iscsi/password-secret", QEMU_CAPS_ISCSI_PASSWORD_SECRET }, }; struct virQEMUCapsObjectTypeProps { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f32687d..665a772 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -426,6 +426,9 @@ typedef enum { QEMU_CAPS_CHARDEV_RECONNECT, /* -chardev reconnect */ QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS, /* -device virtio-(vga|gpu-*),max-outputs= */ + /* 265 */ + QEMU_CAPS_ISCSI_PASSWORD_SECRET, /* -drive file.driver=iscsi,...,password-secret= */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml index a373a6d..c5eb395 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml @@ -172,6 +172,7 @@ <flag name='vnc-multi-servers'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package> (v2.9.0)</package> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index e80782c..99ad44a 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -137,6 +137,7 @@ <flag name='vnc-multi-servers'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index 3641d03..bd446ff 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -220,6 +220,7 @@ <flag name='vnc-multi-servers'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package> (v2.9.0)</package> -- 2.9.5
On Tue, Sep 05, 2017 at 15:09:34 -0400, John Ferlan wrote:
Add the capability to use the blockdev-add query-qmp-schema option to find the 'password-secret' parameter that will allow the iSCSI code to use the master secret object (a/k/a AES) to encrypt the secret in an object and only need to provide the object id of tha secret on the command line thus obsfuscating the passphrase.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 5 files changed, 10 insertions(+)
ACK
On Tue, Sep 05, 2017 at 15:09:34 -0400, John Ferlan wrote:
Add the capability to use the blockdev-add query-qmp-schema option to find the 'password-secret' parameter that will allow the iSCSI code to use the master secret object (a/k/a AES) to encrypt the secret in an object and only need to provide the object id of tha secret on the command line thus obsfuscating the passphrase.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 5 files changed, 10 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7ea6f4..86f27ee 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -439,6 +439,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-net.tx_queue_size", "chardev-reconnect", "virtio-gpu.max_outputs", + + /* 270 */ + "iscsi.password-secret",
...
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f32687d..665a772 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -426,6 +426,9 @@ typedef enum { QEMU_CAPS_CHARDEV_RECONNECT, /* -chardev reconnect */ QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS, /* -device virtio-(vga|gpu-*),max-outputs= */
+ /* 265 */
... this is wrong.
+ QEMU_CAPS_ISCSI_PASSWORD_SECRET, /* -drive file.driver=iscsi,...,password-secret= */ + QEMU_CAPS_LAST /* this must always be the last item */
https://bugzilla.redhat.com/show_bug.cgi?id=1425757 The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found. So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize. Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ed8cb6e..6012538 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -591,6 +591,7 @@ qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, /* qemuBuildGeneralSecinfoURI: + * @protocol: Disk source protocol * @uri: Pointer to the URI structure to add to * @secinfo: Pointer to the secret info data (if present) * @@ -602,7 +603,8 @@ qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, * -1 and error message if fail to add secret information */ static int -qemuBuildGeneralSecinfoURI(virURIPtr uri, +qemuBuildGeneralSecinfoURI(virStorageNetProtocol protocol, + virURIPtr uri, qemuDomainSecretInfoPtr secinfo) { if (!secinfo) @@ -628,6 +630,16 @@ qemuBuildGeneralSecinfoURI(virURIPtr uri, break; case VIR_DOMAIN_SECRET_INFO_TYPE_AES: + /* NB: Cannot fill in @user here because that gets formatted into the + * URI as "iscsi://{username}@{host}:{port}/{iqn}%3A{target}/{lun}", + * but the expectation is it'll be a file parameter "user={username},". + * Still we don't want to return -1, so deal with this later */ + if (protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) + return 0; + else + return -1; + break; + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -841,7 +853,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) goto cleanup; - if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0) + if (qemuBuildGeneralSecinfoURI(src->protocol, uri, secinfo) < 0) goto cleanup; if (VIR_STRDUP(uri->server, src->hosts->name) < 0) @@ -1418,6 +1430,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, * filename=%s,...") instead of the legacy model (e.g."-drive * file=%s,..."), then the "file." prefix can be removed */ + if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) + virBufferAsprintf(buf, "file.user=%s,", secinfo->s.aes.username); + virBufferAsprintf(buf, "file.password-secret=%s,", secinfo->s.aes.alias); } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9cff501..348b8e5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1156,9 +1156,13 @@ qemuDomainSecretSetup(virConnectPtr conn, virSecretLookupTypeDefPtr seclookupdef, bool isLuks) { + bool iscsiHasPS = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_ISCSI_PASSWORD_SECRET); + if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && (usageType == VIR_SECRET_USAGE_TYPE_CEPH || + (usageType == VIR_SECRET_USAGE_TYPE_ISCSI && iscsiHasPS) || usageType == VIR_SECRET_USAGE_TYPE_VOLUME || usageType == VIR_SECRET_USAGE_TYPE_TLS)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args new file mode 100644 index 0000000..5808f5d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args @@ -0,0 +1,39 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-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 \ +-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 \ +-object secret,id=virtio-disk0-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example%3Astorage/1,\ +file.user=myname,file.password-secret=virtio-disk0-secret0,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example%3Astorage/2,\ +file.user=myname,file.password-secret=virtio-disk1-secret0,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\ +id=virtio-disk1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml new file mode 100644 index 0000000..63919f1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml @@ -0,0 +1,43 @@ +<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-system-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args new file mode 100644 index 0000000..c77703e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args @@ -0,0 +1,35 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest2 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,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 \ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example%3Astorage/1,\ +if=none,format=raw,id=drive-hostdev0 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ +drive=drive-hostdev0,id=hostdev0 \ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example%3Astorage/2,\ +if=none,format=raw,id=drive-hostdev1 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ +drive=drive-hostdev1,id=hostdev1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml new file mode 100644 index 0000000..0f63f98 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</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-system-i686</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='2' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='2' unit='5'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d6ada22..c0cfc66 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -917,6 +917,10 @@ mymain(void) DO_TEST("disk-drive-network-nbd-unix", NONE); DO_TEST("disk-drive-network-iscsi", NONE); DO_TEST("disk-drive-network-iscsi-auth", NONE); +# ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + DO_TEST("disk-drive-network-iscsi-auth-AES", + QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_ISCSI_PASSWORD_SECRET); +# endif DO_TEST("disk-drive-network-iscsi-lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_BLOCK); @@ -2301,6 +2305,12 @@ mymain(void) DO_TEST("hostdev-scsi-virtio-iscsi-auth", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); +# ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + DO_TEST("hostdev-scsi-virtio-iscsi-auth-AES", + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_OBJECT_SECRET, + QEMU_CAPS_ISCSI_PASSWORD_SECRET); +# endif DO_TEST("hostdev-scsi-vhost-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_VIRTIO_CCW); -- 2.9.5
On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
Most of the stuff here looks reasonable but I don't think we should mix the URI syntax with the file.param= syntax generated from the JSON objects. Since there's a capability when this is supported, the command line generator should use the new syntax. You can mark it in qemuDiskSourceNeedsProps so that it uses the new generator if it's needed and supported and implement the JSON generator. The rest should then work as expected.
On 09/12/2017 09:36 AM, Peter Krempa wrote:
On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
Most of the stuff here looks reasonable but I don't think we should mix the URI syntax with the file.param= syntax generated from the JSON objects. Since there's a capability when this is supported, the command line generator should use the new syntax.
You can mark it in qemuDiskSourceNeedsProps so that it uses the new generator if it's needed and supported and implement the JSON generator.
The rest should then work as expected.
This is certainly where things didn't exactly match up the way I thought you have desired the virstoragefile and virstoragetest code to work. In particular, it's the "user" and "password-secret" options that cause the disconnect since they're a @disk private object and not a @disk->src object. I considered a number of different ways to cheat, but came up empty on each, so I just followed the existing RBD code. One cannot reconstruct the <auth> element properly given that all the arguments have is a username and an alias, but would need to have either a "usage" or "uuid" string. The secret object doesn't contain that either, so it'd need to be stored somehow. Perhaps if the @secinfo moved from private into source that would help, although perhaps not following the RNG. Still there'd also have to be a way to save the string used in the original <auth> element used to look up the secret (either by uuid or by usage). Having the password-secret alias is OK, but really not useful. Maybe "some future" adjustment could modify the password-secret alias to be (for example) "virtio-disk0-%s-secret0" where the %s is either a UUID or a Usage string of the secret used to generate the object. Perhaps even the unsigned char UUID too so as to not have too many extra "-"'s to parse/read. That'd be an awful alias, but serve a purpose as well. A similar problem exists for RBD, but the RBD code in virstoragefile and virstoragetest totally ignore the authentication pieces... That syntax is a bit more hairly because it's only the RBD password-secret field that needs the "file." (or not if -drive driver=rbd,... was supported). John
On Tue, Sep 12, 2017 at 11:55:29 -0400, John Ferlan wrote:
On 09/12/2017 09:36 AM, Peter Krempa wrote:
On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
Most of the stuff here looks reasonable but I don't think we should mix the URI syntax with the file.param= syntax generated from the JSON objects. Since there's a capability when this is supported, the command line generator should use the new syntax.
You can mark it in qemuDiskSourceNeedsProps so that it uses the new generator if it's needed and supported and implement the JSON generator.
The rest should then work as expected.
This is certainly where things didn't exactly match up the way I thought you have desired the virstoragefile and virstoragetest code to work.
In particular, it's the "user" and "password-secret" options that cause the disconnect since they're a @disk private object and not a @disk->src object. I considered a number of different ways to cheat, but came up empty on each, so I just followed the existing RBD code.
Well and that is a problem. We can either consider the disk authentication secrests to be the same for every single level of the backing chain. In such case the user and secret are correctly placed in the 'Disk' structure. In such case, still every single level of the backing chain will eventually need to access them so that the authentication can be done for every member. We can also allow specifying a different one per every level of the backing chain, which would make more sense in some configurations and allow more flexibility. In such case we need to move them to the virStorageSource structure so that every level can have individual authentication data. In the long term I think that solution 1 would bite us and we'd need to add individual authentication for the volumes anyways.
One cannot reconstruct the <auth> element properly given that all the arguments have is a username and an alias, but would need to have either a "usage" or "uuid" string. The secret object doesn't contain that either, so it'd need to be stored somehow.
You mean from parsing of the backing chain? That won't be necessary.
Perhaps if the @secinfo moved from private into source that would help, although perhaps not following the RNG. Still there'd also have to be a
We don't follow the RNG in internal structures. In some cases the RNG does not make sense, but the strucutres can be changed to make sense.
way to save the string used in the original <auth> element used to look up the secret (either by uuid or by usage). Having the password-secret alias is OK, but really not useful.
That is needed only for formatting of the command line (or monitor command) and for removing the correct secret on unplug. Most of the times you can infer it. We can as well as store it and not have to generate it all the time. That is an implementation detail though.
Maybe "some future" adjustment could modify the password-secret alias to be (for example) "virtio-disk0-%s-secret0" where the %s is either a UUID or a Usage string of the secret used to generate the object. Perhaps even the unsigned char UUID too so as to not have too many extra "-"'s to parse/read. That'd be an awful alias, but serve a purpose as well.
No, this is not necessary. We can store the data in the status XML. The alias (in password secret) is needed only to remove them in case of unplug and it's not really necessary to trace back which secret was used to populate it.
A similar problem exists for RBD, but the RBD code in virstoragefile and virstoragetest totally ignore the authentication pieces... That syntax is a bit more hairly because it's only the RBD password-secret field that needs the "file." (or not if -drive driver=rbd,... was supported).
Ummm. The test does not need to deal with this necessarily. My issue is with the JSON structure generator, which should be fully used. And that generator is private to the qemu driver.
On 09/13/2017 03:08 AM, Peter Krempa wrote:
On Tue, Sep 12, 2017 at 11:55:29 -0400, John Ferlan wrote:
On 09/12/2017 09:36 AM, Peter Krempa wrote:
On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
Most of the stuff here looks reasonable but I don't think we should mix the URI syntax with the file.param= syntax generated from the JSON objects. Since there's a capability when this is supported, the command line generator should use the new syntax.
You can mark it in qemuDiskSourceNeedsProps so that it uses the new generator if it's needed and supported and implement the JSON generator.
The rest should then work as expected.
This is certainly where things didn't exactly match up the way I thought you have desired the virstoragefile and virstoragetest code to work.
In particular, it's the "user" and "password-secret" options that cause the disconnect since they're a @disk private object and not a @disk->src object. I considered a number of different ways to cheat, but came up empty on each, so I just followed the existing RBD code.
Well and that is a problem. We can either consider the disk authentication secrests to be the same for every single level of the backing chain. In such case the user and secret are correctly placed in the 'Disk' structure. In such case, still every single level of the backing chain will eventually need to access them so that the authentication can be done for every member.
We can also allow specifying a different one per every level of the backing chain, which would make more sense in some configurations and allow more flexibility. In such case we need to move them to the virStorageSource structure so that every level can have individual authentication data.
In the long term I think that solution 1 would bite us and we'd need to add individual authentication for the volumes anyways.
caveat: my knowledge of backing chain details is quite limited... Can different levels of the backing chain use different source servers? IOW: Assume the top level is: <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> <host name='example.com' port='3260'/> </source> could some layer deeper then have: <source protocol='iscsi' name='iqn.2013-07.com.example.adifferent:iscsi-nopool/1'> <host name='adifferent.example.com' port='3260'/> </source> ? If so, then <auth> should be a child of <source> rather than <disk>. If not, then <auth> should stay as child of <disk>. I would think it would be quite difficult to configure usage of multiple servers. Would that mean if one of the sources wasn't available or had some sort of failure, then the whole chain is invalid? The complexity is rather mind boggling, if not fragile...
One cannot reconstruct the <auth> element properly given that all the arguments have is a username and an alias, but would need to have either a "usage" or "uuid" string. The secret object doesn't contain that either, so it'd need to be stored somehow.
You mean from parsing of the backing chain? That won't be necessary.
I was thinking more about virstoragefile.c and virstoragetest.c. It's easy enough to fetch the user/password-secret in virStorageSourceParseBackingJSONiSCSI: + const char *user = virJSONValueObjectGetString(json, "user"); + const char *secret = virJSONValueObjectGetString(json, "password-secret"); But there's not much one can do with it as you get (for example): user = "myname" secret = "virtio-disk1-secret0" but an <auth> would look like: <auth username='myname'> <secret type='iscsi' usage='mycluster_myname'/> </auth> So to a degree it's not testable to build the XML as virstoragetest does. Sine the secret can be built up using the disk alias, it's perhaps not even worth storing away.
Perhaps if the @secinfo moved from private into source that would help, although perhaps not following the RNG. Still there'd also have to be a
We don't follow the RNG in internal structures. In some cases the RNG does not make sense, but the strucutres can be changed to make sense.
But we only support one <auth> per <disk>, so in order to be associated with <source>, the RNG would need to support that and the code would need to be modified such that <auth> is a child of <source>. We'd have to support "both" formats. If <auth> was a child of <disk>, then perhaps it only supports one-level depth. We could also read under <disk>, but move to under <source> on output if we were so inclined. Still that would then require every level of the backing chain to have it's own <auth> even if that is the same <auth> as the predecessor. I'm thinking and typing here, so I haven't thought through all the possibilities.
way to save the string used in the original <auth> element used to look up the secret (either by uuid or by usage). Having the password-secret alias is OK, but really not useful.
That is needed only for formatting of the command line (or monitor command) and for removing the correct secret on unplug. Most of the times you can infer it. We can as well as store it and not have to generate it all the time. That is an implementation detail though.
Again - I was thinking of the test environment that builds the XML from the JSON. I had actually started down this path originally, but got stuck at precisely that point. So I have code in a branch already that can do everything except the virstoragetest rebuild of <auth>.
Maybe "some future" adjustment could modify the password-secret alias to be (for example) "virtio-disk0-%s-secret0" where the %s is either a UUID or a Usage string of the secret used to generate the object. Perhaps even the unsigned char UUID too so as to not have too many extra "-"'s to parse/read. That'd be an awful alias, but serve a purpose as well.
No, this is not necessary. We can store the data in the status XML. The alias (in password secret) is needed only to remove them in case of unplug and it's not really necessary to trace back which secret was used to populate it.
OK... that's good because that would get ugly fast. Saving the secret alias in the status can be done, but is it useful since it's easily generated from the disk/drive alias?
A similar problem exists for RBD, but the RBD code in virstoragefile and virstoragetest totally ignore the authentication pieces... That syntax is a bit more hairly because it's only the RBD password-secret field that needs the "file." (or not if -drive driver=rbd,... was supported).
Ummm. The test does not need to deal with this necessarily. My issue is with the JSON structure generator, which should be fully used. And that generator is private to the qemu driver.
Well RBD would seemingly need to be updated as well, but it's a disjoint thought to the iSCSI, although still related since both RBD and iSCSI can use secrets in this manner. The usage of "-drive driver=XXX" was a separate thought. I wasn't sure if you were going down the path of converting usage of using that newer syntax rather than the "-drive file...". In a way, I suppose I'm surprised that usage of "file" was chosen over "driver=XXX". But that's a different discussion. One that keeps creeping back though as all the QEMU examples show the driver=XXX syntax and it seems they're trying to move away from the "file" syntax. John
On Wed, Sep 13, 2017 at 09:05:43 -0400, John Ferlan wrote:
On 09/13/2017 03:08 AM, Peter Krempa wrote:
On Tue, Sep 12, 2017 at 11:55:29 -0400, John Ferlan wrote:
On 09/12/2017 09:36 AM, Peter Krempa wrote:
On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
Most of the stuff here looks reasonable but I don't think we should mix the URI syntax with the file.param= syntax generated from the JSON objects. Since there's a capability when this is supported, the command line generator should use the new syntax.
You can mark it in qemuDiskSourceNeedsProps so that it uses the new generator if it's needed and supported and implement the JSON generator.
The rest should then work as expected.
This is certainly where things didn't exactly match up the way I thought you have desired the virstoragefile and virstoragetest code to work.
In particular, it's the "user" and "password-secret" options that cause the disconnect since they're a @disk private object and not a @disk->src object. I considered a number of different ways to cheat, but came up empty on each, so I just followed the existing RBD code.
Well and that is a problem. We can either consider the disk authentication secrests to be the same for every single level of the backing chain. In such case the user and secret are correctly placed in the 'Disk' structure. In such case, still every single level of the backing chain will eventually need to access them so that the authentication can be done for every member.
We can also allow specifying a different one per every level of the backing chain, which would make more sense in some configurations and allow more flexibility. In such case we need to move them to the virStorageSource structure so that every level can have individual authentication data.
In the long term I think that solution 1 would bite us and we'd need to add individual authentication for the volumes anyways.
caveat: my knowledge of backing chain details is quite limited...
Can different levels of the backing chain use different source servers?
IOW: Assume the top level is:
<source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> <host name='example.com' port='3260'/> </source>
could some layer deeper then have:
<source protocol='iscsi' name='iqn.2013-07.com.example.adifferent:iscsi-nopool/1'> <host name='adifferent.example.com' port='3260'/> </source>
Yes it could. In fact you can even change to different protocol. For now you need to be able to express it via the "backing_file" string in the qcow2 header. In the future, we will keep it in the XML (as we do when you start the VM, but that is output-only currently).
?
If so, then <auth> should be a child of <source> rather than <disk>. If not, then <auth> should stay as child of <disk>.
Um, yes. It should be a child of <source> in this case. At least for the nested elements. We also need a way to add <encryption> to source, since that is also per layer.
I would think it would be quite difficult to configure usage of multiple servers. Would that mean if one of the sources wasn't available or had some sort of failure, then the whole chain is invalid? The complexity is rather mind boggling, if not fragile...
You can even alternate between local files and remote storage, if you want to get even more weird ..., still it's possible even now, so we'll need to keep this functionality. There are use cases where your base-image is copied to every single host and the overlay is hosted on network, to decrease initial bandwidth.
One cannot reconstruct the <auth> element properly given that all the arguments have is a username and an alias, but would need to have either a "usage" or "uuid" string. The secret object doesn't contain that either, so it'd need to be stored somehow.
You mean from parsing of the backing chain? That won't be necessary.
I was thinking more about virstoragefile.c and virstoragetest.c. It's easy enough to fetch the user/password-secret in virStorageSourceParseBackingJSONiSCSI:
+ const char *user = virJSONValueObjectGetString(json, "user"); + const char *secret = virJSONValueObjectGetString(json, "password-secret");
But there's not much one can do with it as you get (for example):
user = "myname" secret = "virtio-disk1-secret0"
but an <auth> would look like:
<auth username='myname'> <secret type='iscsi' usage='mycluster_myname'/> </auth>
So to a degree it's not testable to build the XML as virstoragetest does. Sine the secret can be built up using the disk alias, it's perhaps not even worth storing away.
Yes, the field is rather useless. I'm even surprised that it's recorded into the backing file string. It may be used as a notification that authentication is required, but without actually adding the secret with correct name, the thing won't work anyways.
Perhaps if the @secinfo moved from private into source that would help, although perhaps not following the RNG. Still there'd also have to be a
We don't follow the RNG in internal structures. In some cases the RNG does not make sense, but the strucutres can be changed to make sense.
But we only support one <auth> per <disk>, so in order to be associated with <source>, the RNG would need to support that and the code would need to be modified such that <auth> is a child of <source>.
We'd have to support "both" formats. If <auth> was a child of <disk>, then perhaps it only supports one-level depth. We could also read under <disk>, but move to under <source> on output if we were so inclined. Still that would then require every level of the backing chain to have it's own <auth> even if that is the same <auth> as the predecessor. I'm thinking and typing here, so I haven't thought through all the possibilities.
Well, this was probably inherited as a legacy approach. We really need them per backing file though.
way to save the string used in the original <auth> element used to look up the secret (either by uuid or by usage). Having the password-secret alias is OK, but really not useful.
That is needed only for formatting of the command line (or monitor command) and for removing the correct secret on unplug. Most of the times you can infer it. We can as well as store it and not have to generate it all the time. That is an implementation detail though.
Again - I was thinking of the test environment that builds the XML from the JSON. I had actually started down this path originally, but got stuck at precisely that point. So I have code in a branch already that can do everything except the virstoragetest rebuild of <auth>.
Maybe "some future" adjustment could modify the password-secret alias to be (for example) "virtio-disk0-%s-secret0" where the %s is either a UUID or a Usage string of the secret used to generate the object. Perhaps even the unsigned char UUID too so as to not have too many extra "-"'s to parse/read. That'd be an awful alias, but serve a purpose as well.
No, this is not necessary. We can store the data in the status XML. The alias (in password secret) is needed only to remove them in case of unplug and it's not really necessary to trace back which secret was used to populate it.
OK... that's good because that would get ugly fast. Saving the secret alias in the status can be done, but is it useful since it's easily generated from the disk/drive alias?
And it will need to be generated from the node name or other backing file specific identifier, since each level can have it's own ...
A similar problem exists for RBD, but the RBD code in virstoragefile and virstoragetest totally ignore the authentication pieces... That syntax is a bit more hairly because it's only the RBD password-secret field that needs the "file." (or not if -drive driver=rbd,... was supported).
Ummm. The test does not need to deal with this necessarily. My issue is with the JSON structure generator, which should be fully used. And that generator is private to the qemu driver.
Well RBD would seemingly need to be updated as well, but it's a disjoint thought to the iSCSI, although still related since both RBD and iSCSI can use secrets in this manner.
The usage of "-drive driver=XXX" was a separate thought. I wasn't sure if you were going down the path of converting usage of using that newer syntax rather than the "-drive file...". In a way, I suppose I'm surprised that usage of "file" was chosen over "driver=XXX". But that's a different discussion. One that keeps creeping back though as all the QEMU examples show the driver=XXX syntax and it seems they're trying to move away from the "file" syntax.
The new syntax uses -blockdev to express any backing elements and then the -drive only refers to the backing element via node name. That's the main reason why I want to use the JSON->props style. It's the same for -blockdev and the blockdev-add QMP command and it's more flexible.
On Wed, Sep 13, 2017 at 03:24:42PM +0200, Peter Krempa wrote:
On Wed, Sep 13, 2017 at 09:05:43 -0400, John Ferlan wrote:
I was thinking more about virstoragefile.c and virstoragetest.c. It's easy enough to fetch the user/password-secret in virStorageSourceParseBackingJSONiSCSI:
+ const char *user = virJSONValueObjectGetString(json, "user"); + const char *secret = virJSONValueObjectGetString(json, "password-secret");
But there's not much one can do with it as you get (for example):
user = "myname" secret = "virtio-disk1-secret0"
but an <auth> would look like:
<auth username='myname'> <secret type='iscsi' usage='mycluster_myname'/> </auth>
So to a degree it's not testable to build the XML as virstoragetest does. Sine the secret can be built up using the disk alias, it's perhaps not even worth storing away.
Yes, the field is rather useless. I'm even surprised that it's recorded into the backing file string. It may be used as a notification that authentication is required, but without actually adding the secret with correct name, the thing won't work anyways.
That is a bug in QEMU - we should not be recording the 'password-secret" field in the backing store that is embedded in the qcow2 file. The IDs of the secret objects are only meaningful for the current invokation of QEMU / qemu-img / etc. So if a backing store needs a password, then libvirt needs to explicitly set the password-secret for the backing store, recursively Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 09/12/2017 09:36 AM, Peter Krempa wrote:
On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 19 ++++++++- src/qemu/qemu_domain.c | 4 ++ ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 10 +++++ 7 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
Most of the stuff here looks reasonable but I don't think we should mix the URI syntax with the file.param= syntax generated from the JSON objects. Since there's a capability when this is supported, the command line generator should use the new syntax.
You can mark it in qemuDiskSourceNeedsProps so that it uses the new generator if it's needed and supported and implement the JSON generator.
The rest should then work as expected.
One more thing I forgot in the first reply... There's also iSCSI on <hostdev> that uses this same code path, but isn't processed for qemuDiskSourceNeedsProps. I think a significant reworking of the code could be necessary - whether that outweighs the security aspects of providing the passphrase on the command line like is done now is up for "debate" I suppose. John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Peter Krempa