2:09 p.m.
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
2:09 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Get capabilities for iscsi usage of password-secret argument
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(a)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
8:30 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Get capabilities for iscsi usage of password-secret argument
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(a)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
8:40 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Get capabilities for iscsi usage of password-secret argument
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(a)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 */
2:09 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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
8:36 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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.
10:55 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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
2:08 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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.
8:05 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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
8:24 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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.
8:42 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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 :|
4:43 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords
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(a)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
2627
days inactive
2635
days old
11 comments
3 participants
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Peter Krempa