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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.