[PATCH 00/11] qemu: Fix crash when attempting to use authentication for HTTP backed disks
The crash is fixed by properly instantiating the auth object. Also this series cleans up some problems noticed along the way. Peter Krempa (11): conf: schema: Fix alignment in 'diskSourceNetworkProtocolHTTPS' schema definition qemuDomainValidateStorageSource: Reorganize encryption config validation qemuDomainValidateStorageSource: Add validation of 'encryption' support qemuBlockStorageSourceGetRBDProps: Simplify handling of encryption format virDomainDiskDefValidateSourceChainOne: Reject authentication for protocols which don't support it qemuDomainStorageSourceHasAuth: Don't decide based on protocol qemu: domain: Inline qemuDomainDiskHasEncryptionSecret qemu: domain: Inline qemuDomainStorageSourceHasAuth conf: schemas: Split out definition for 'ftp' protocol conf: schema: Allow authentication for FTP(S) and HTTP(S) disks tests: qemuxml2*: Add testing of authenticated http/ftp disks src/conf/domain_validate.c | 34 ++++++ src/conf/schemas/domaincommon.rng | 35 +++++- src/qemu/qemu_block.c | 10 +- src/qemu/qemu_domain.c | 103 ++++++------------ src/qemu/qemu_domain.h | 3 - .../disk-cdrom-network.x86_64-4.1.0.args | 6 +- .../disk-cdrom-network.x86_64-latest.args | 6 +- tests/qemuxml2argvdata/disk-cdrom-network.xml | 6 + .../disk-encryption-wrong.x86_64-latest.err | 1 + .../disk-encryption-wrong.xml | 37 +++++++ ...-network-rbd-encryption.x86_64-latest.args | 2 +- .../disk-network-rbd-encryption.xml | 2 +- tests/qemuxml2argvtest.c | 1 + .../disk-cdrom-network.x86_64-latest.xml | 72 ++++++++++++ ...k-network-rbd-encryption.x86_64-latest.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 16 files changed, 233 insertions(+), 88 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.xml create mode 100644 tests/qemuxml2xmloutdata/disk-cdrom-network.x86_64-latest.xml -- 2.35.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index c0c14fe558..bf4d6e4b6f 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1995,7 +1995,7 @@ <ref name="diskSourceNetworkProtocolHTTPCookies"/> </optional> <ref name="diskSourceNetworkProtocolPropsCommon"/> - </interleave> + </interleave> </element> </define> -- 2.35.1
Move the two ad-hoc checks below into the block which already tests whether encryption is requested. If we first disallow the old-style qcow2 encryption we can remove a whole block of validation later on. Also the capability check for qcow2+luks can be simplified by moving it into the same block. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++++----------------------------- 1 file changed, 17 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 00c209313b..f3d9b2e48e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4821,25 +4821,6 @@ qemuDomainValidateStorageSource(virStorageSource *src, return -1; } - if ((src->format == VIR_STORAGE_FILE_QCOW || - src->format == VIR_STORAGE_FILE_QCOW2) && - src->encryption && - (src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT || - src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("old qcow/qcow2 encryption is not supported")); - return -1; - } - - if (src->format == VIR_STORAGE_FILE_QCOW2 && - src->encryption && - src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_QCOW2_LUKS)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("LUKS encrypted QCOW2 images are not supported by this QEMU")); - return -1; - } - if (src->format == VIR_STORAGE_FILE_FAT && actualType != VIR_STORAGE_TYPE_VOLUME && actualType != VIR_STORAGE_TYPE_DIR) { @@ -5019,6 +5000,13 @@ qemuDomainValidateStorageSource(virStorageSource *src, } if (src->encryption) { + if (src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT || + src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("old qcow/qcow2 encryption is not supported")); + return -1; + } + switch (src->encryption->engine) { case VIR_STORAGE_ENCRYPTION_ENGINE_QEMU: switch ((virStorageEncryptionFormatType) src->encryption->format) { @@ -5040,38 +5028,29 @@ qemuDomainValidateStorageSource(virStorageSource *src, } break; + case VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_ENCRYPTION)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("librbd encryption is not supported by this QEMU binary")); return -1; } - - switch ((virStorageEncryptionFormatType) src->encryption->format) { - case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS: - case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS2: - break; - - case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("librbd encryption engine only supports luks/luks2 formats")); - return -1; - - case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT: - case VIR_STORAGE_ENCRYPTION_FORMAT_LAST: - default: - virReportEnumRangeError(virStorageEncryptionFormatType, - src->encryption->format); - return -1; - } - break; + case VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT: case VIR_STORAGE_ENCRYPTION_ENGINE_LAST: virReportEnumRangeError(virStorageEncryptionEngine, src->encryption->engine); return -1; } + + if (src->format == VIR_STORAGE_FILE_QCOW2 && + src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_QCOW2_LUKS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("LUKS encrypted QCOW2 images are not supported by this QEMU")); + return -1; + } } if (src->tlsHostname) { -- 2.35.1
Reject encryption requests for unsupported image format types. Add negative test for the rejected cases as well as modify 'disk-network-rbd-encryption' case to validate that with librbd encryption the format doesn matter. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++ .../disk-encryption-wrong.x86_64-latest.err | 1 + .../disk-encryption-wrong.xml | 37 +++++++++++++++++++ ...-network-rbd-encryption.x86_64-latest.args | 2 +- .../disk-network-rbd-encryption.xml | 2 +- tests/qemuxml2argvtest.c | 1 + ...k-network-rbd-encryption.x86_64-latest.xml | 2 +- 7 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f3d9b2e48e..b5abf99951 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5012,6 +5012,12 @@ qemuDomainValidateStorageSource(virStorageSource *src, switch ((virStorageEncryptionFormatType) src->encryption->format) { case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS: case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: + if (src->format != VIR_STORAGE_FILE_QCOW2 && + src->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("encryption is supported only with 'raw' and 'qcow2' image format")); + return -1; + } break; case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS2: @@ -5035,6 +5041,13 @@ qemuDomainValidateStorageSource(virStorageSource *src, _("librbd encryption is not supported by this QEMU binary")); return -1; } + + if (actualType != VIR_STORAGE_TYPE_NETWORK && + src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("librbd encryption is supported only with RBD backed disks")); + return -1; + } break; case VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT: diff --git a/tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err b/tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err new file mode 100644 index 0000000000..e52340be07 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err @@ -0,0 +1 @@ +unsupported configuration: encryption is supported only with 'raw' and 'qcow2' image format diff --git a/tests/qemuxml2argvdata/disk-encryption-wrong.xml b/tests/qemuxml2argvdata/disk-encryption-wrong.xml new file mode 100644 index 0000000000..d0671721f7 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-encryption-wrong.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>encryptdisk</name> + <uuid>496898a6-e6ff-f7c8-5dc2-3cf410945ee9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' 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-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='vmdk'/> + <source file='/storage/guest_disks/encryptdisk'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args b/tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args index 2de29d8174..d5712cb0ba 100644 --- a/tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args +++ b/tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args @@ -42,7 +42,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-encryptdisk/.config \ -device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x5","drive":"libvirt-2-format","id":"virtio-disk2"}' \ -object '{"qom-type":"secret","id":"libvirt-1-format-encryption-secret0","data":"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1","keyid":"masterKey0","iv":"AAECAwQFBgcICQoLDA0ODw==","format":"base64"}' \ -blockdev '{"driver":"rbd","pool":"pool","image":"image2","server":[{"host":"mon1.example.org","port":"6321"},{"host":"mon2.example.org","port":"6322"},{"host":"mon3.example.org","port":"6322"}],"encrypt":{"format":"luks2","key-secret":"libvirt-1-format-encryption-secret0"},"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ --blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"vmdk","file":"libvirt-1-storage"}' \ -device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x6","drive":"libvirt-1-format","id":"virtio-disk3"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x3"}' \ diff --git a/tests/qemuxml2argvdata/disk-network-rbd-encryption.xml b/tests/qemuxml2argvdata/disk-network-rbd-encryption.xml index eeadbfeeba..d1fcf2da61 100644 --- a/tests/qemuxml2argvdata/disk-network-rbd-encryption.xml +++ b/tests/qemuxml2argvdata/disk-network-rbd-encryption.xml @@ -51,7 +51,7 @@ <target dev='vdc' bus='virtio'/> </disk> <disk type='network' device='disk'> - <driver name='qemu' type='raw'/> + <driver name='qemu' type='vmdk'/> <source protocol='rbd' name='pool/image2'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 41fd032f19..1f080daba7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1374,6 +1374,7 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-network-rbd"); DO_TEST_CAPS_VER_PARSE_ERROR("disk-network-rbd-encryption", "6.0.0"); DO_TEST_CAPS_LATEST("disk-network-rbd-encryption"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-encryption-wrong"); DO_TEST_CAPS_VER_FAILURE("disk-network-rbd-no-colon", "4.1.0"); DO_TEST_CAPS_LATEST("disk-network-rbd-no-colon"); DO_TEST_CAPS_VER("disk-network-sheepdog", "4.1.0"); diff --git a/tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xml index a91504202a..99bba52db5 100644 --- a/tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xml @@ -57,7 +57,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </disk> <disk type='network' device='disk'> - <driver name='qemu' type='raw'/> + <driver name='qemu' type='vmdk'/> <source protocol='rbd' name='pool/image2'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> -- 2.35.1
On a Friday in 2022, Peter Krempa wrote:
Reject encryption requests for unsupported image format types.
Add negative test for the rejected cases as well as modify 'disk-network-rbd-encryption' case to validate that with librbd encryption the format doesn matter.
*doesn't Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++ .../disk-encryption-wrong.x86_64-latest.err | 1 + .../disk-encryption-wrong.xml | 37 +++++++++++++++++++ ...-network-rbd-encryption.x86_64-latest.args | 2 +- .../disk-network-rbd-encryption.xml | 2 +- tests/qemuxml2argvtest.c | 1 + ...k-network-rbd-encryption.x86_64-latest.xml | 2 +- 7 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.xml
Remove the impossible error message about the 'qcow2' encryption format not being supported. We validated before that it can't happen. Additionally the code can be simplified by removing error handling from impossible code paths as the last resort is virJSONValueCreate not allowing NULL argument with the 's:' modifier. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 3d961c8b39..60e03d418e 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -882,7 +882,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, g_autoptr(virJSONValue) servers = NULL; virJSONValue *ret = NULL; g_autoptr(virJSONValue) encrypt = NULL; - const char *encformat; + const char *encformat = NULL; const char *username = NULL; g_autoptr(virJSONValue) authmodes = NULL; const char *keysecret = NULL; @@ -911,16 +911,10 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, break; case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("librbd encryption engine only supports luks/luks2 formats")); - return NULL; - case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT: case VIR_STORAGE_ENCRYPTION_FORMAT_LAST: default: - virReportEnumRangeError(virStorageEncryptionFormatType, - src->encryption->format); - return NULL; + break; } if (virJSONValueObjectAdd(&encrypt, -- 2.35.1
Only certain disk protocols support authentication. Add validation for this field. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_validate.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 68190fc3e2..3f03feee4f 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -473,10 +473,44 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef *disk) static int virDomainDiskDefValidateSourceChainOne(const virStorageSource *src) { + virStorageType actualType = virStorageSourceGetActualType(src); + if (src->type == VIR_STORAGE_TYPE_NETWORK && src->auth) { virStorageAuthDef *authdef = src->auth; int actUsage; + if (actualType != VIR_STORAGE_TYPE_NETWORK) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("authentication is supported only for network backed disks")); + return -1; + } + + switch ((virStorageNetProtocol) src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_RBD: + break; + + case VIR_STORAGE_NET_PROTOCOL_NBD: + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_VXHS: + case VIR_STORAGE_NET_PROTOCOL_NFS: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("authentication is not supported for protocol '%s'"), + virStorageNetProtocolTypeToString(src->protocol)); + return -1; + + case VIR_STORAGE_NET_PROTOCOL_NONE: + case VIR_STORAGE_NET_PROTOCOL_LAST: + break; + } + if ((actUsage = virSecretUsageTypeFromString(authdef->secrettype)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown secret type '%s'"), -- 2.35.1
The validation should be the only point to decide whether authentication is supported for a disk backing protocol. The rest of the code can then simply always enable it. This also fixes a crash when authentication is requested e.g. for a HTTP backed disk as the blockdev props formatter expects that it was already set up. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b5abf99951..0486826fc7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1236,9 +1236,7 @@ qemuDomainStorageSourceHasAuth(virStorageSource *src) { if (!virStorageSourceIsEmpty(src) && virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK && - src->auth && - (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI || - src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) + src->auth) return true; return false; -- 2.35.1
Since we are already checking that the encryption format can be only _LUKS and _LUKS2 this wrapper function doesn't make much sense any more. The only one caller can do this internally. The move of virStorageSourceIsEmpty is correct as there are no secrets to setup if the disk is empty anyways. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0486826fc7..1eb15c8989 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1243,19 +1243,6 @@ qemuDomainStorageSourceHasAuth(virStorageSource *src) } -static bool -qemuDomainDiskHasEncryptionSecret(virStorageSource *src) -{ - if (!virStorageSourceIsEmpty(src) && src->encryption && - (src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS || - src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS2) && - src->encryption->nsecrets > 0) - return true; - - return false; -} - - static qemuDomainSecretInfo * qemuDomainSecretStorageSourcePrepareCookies(qemuDomainObjPrivate *priv, virStorageSource *src, @@ -1291,7 +1278,10 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, { qemuDomainStorageSourcePrivate *srcPriv; bool hasAuth = qemuDomainStorageSourceHasAuth(src); - bool hasEnc = qemuDomainDiskHasEncryptionSecret(src); + bool hasEnc = src->encryption && src->encryption->nsecrets > 0; + + if (virStorageSourceIsEmpty(src)) + return 0; if (!hasAuth && !hasEnc && src->ncookies == 0) return 0; -- 2.35.1
The iSCSI hostdev code doesn't require the check for the empty drive and the check for the protocol because those are already guaranteed at that point. In qemuDomainSecretStorageSourcePrepare we don't need to check the network disk type either as it's now guaranteed by the definition validator. Thus both callers can simply check whether src->auth is present and the helper can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 19 +++---------------- src/qemu/qemu_domain.h | 3 --- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1eb15c8989..7974cdb00b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1231,18 +1231,6 @@ qemuDomainSecretDiskDestroy(virDomainDiskDef *disk) } -bool -qemuDomainStorageSourceHasAuth(virStorageSource *src) -{ - if (!virStorageSourceIsEmpty(src) && - virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK && - src->auth) - return true; - - return false; -} - - static qemuDomainSecretInfo * qemuDomainSecretStorageSourcePrepareCookies(qemuDomainObjPrivate *priv, virStorageSource *src, @@ -1277,13 +1265,12 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, const char *aliasformat) { qemuDomainStorageSourcePrivate *srcPriv; - bool hasAuth = qemuDomainStorageSourceHasAuth(src); bool hasEnc = src->encryption && src->encryption->nsecrets > 0; if (virStorageSourceIsEmpty(src)) return 0; - if (!hasAuth && !hasEnc && src->ncookies == 0) + if (!src->auth && !hasEnc && src->ncookies == 0) return 0; if (!(src->privateData = qemuDomainStorageSourcePrivateNew())) @@ -1291,7 +1278,7 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); - if (hasAuth) { + if (src->auth) { virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI; if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) @@ -5663,7 +5650,7 @@ qemuDomainDeviceHostdevDefPostParseRestoreSecAlias(virDomainHostdevDef *hostdev, if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI || scsisrc->protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI || - !qemuDomainStorageSourceHasAuth(iscsisrc->src)) + !iscsisrc->src->auth) return 0; if (!(priv = qemuDomainStorageSourcePrivateFetch(iscsisrc->src))) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 0415a34908..c7125722e0 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -870,9 +870,6 @@ void qemuDomainSecretInfoDestroy(qemuDomainSecretInfo *secinfo); void qemuDomainSecretDiskDestroy(virDomainDiskDef *disk) ATTRIBUTE_NONNULL(1); -bool qemuDomainStorageSourceHasAuth(virStorageSource *src) - ATTRIBUTE_NONNULL(1); - qemuDomainSecretInfo * qemuDomainSecretInfoTLSNew(qemuDomainObjPrivate *priv, const char *srcAlias, -- 2.35.1
Separate it so that further addition can target 'ftp' only. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index bf4d6e4b6f..c68acaa222 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -2046,13 +2046,31 @@ </element> </define> + <define name="diskSourceNetworkProtocolFTP"> + <element name="source"> + <interleave> + <attribute name="protocol"> + <choice> + <value>ftp</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceCommon"/> + <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="encryption"/> + </optional> + <ref name="diskSourceNetworkProtocolPropsCommon"/> + </interleave> + </element> + </define> + <define name="diskSourceNetworkProtocolSimple"> <element name="source"> <interleave> <attribute name="protocol"> <choice> <value>sheepdog</value> - <value>ftp</value> <value>tftp</value> </choice> </attribute> @@ -2159,6 +2177,7 @@ <ref name="diskSourceNetworkProtocolHTTP"/> <ref name="diskSourceNetworkProtocolHTTPS"/> <ref name="diskSourceNetworkProtocolFTPS"/> + <ref name="diskSourceNetworkProtocolFTP"/> <ref name="diskSourceNetworkProtocolSimple"/> <ref name="diskSourceNetworkProtocolVxHS"/> <ref name="diskSourceNetworkProtocolNFS"/> -- 2.35.1
The code already handles this so we just need to allow it in the schema. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index c68acaa222..8afb0dadd4 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1995,6 +1995,9 @@ <ref name="diskSourceNetworkProtocolHTTPCookies"/> </optional> <ref name="diskSourceNetworkProtocolPropsCommon"/> + <optional> + <ref name="diskAuth"/> + </optional> </interleave> </element> </define> @@ -2020,6 +2023,9 @@ <ref name="diskSourceNetworkProtocolHTTPCookies"/> </optional> <ref name="diskSourceNetworkProtocolPropsCommon"/> + <optional> + <ref name="diskAuth"/> + </optional> </interleave> </element> </define> @@ -2042,6 +2048,9 @@ <ref name="diskSourceNetworkProtocolSSLVerify"/> </optional> <ref name="diskSourceNetworkProtocolPropsCommon"/> + <optional> + <ref name="diskAuth"/> + </optional> </interleave> </element> </define> @@ -2061,6 +2070,9 @@ <ref name="encryption"/> </optional> <ref name="diskSourceNetworkProtocolPropsCommon"/> + <optional> + <ref name="diskAuth"/> + </optional> </interleave> </element> </define> -- 2.35.1
Extend the 'disk-cdrom-network' to cover this instance. This also validates that the parameters of -blockdev conform to the QAPI schema. Also add the xml2xml variant of this test case. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-cdrom-network.x86_64-4.1.0.args | 6 +- .../disk-cdrom-network.x86_64-latest.args | 6 +- tests/qemuxml2argvdata/disk-cdrom-network.xml | 6 ++ .../disk-cdrom-network.x86_64-latest.xml | 72 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/disk-cdrom-network.x86_64-latest.xml diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-4.1.0.args b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-4.1.0.args index 00030f208d..34bd38622a 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-4.1.0.args +++ b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-4.1.0.args @@ -28,9 +28,11 @@ QEMU_AUDIO_DRV=none \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -drive file=ftp://host.name:21/url/path/file.iso,format=raw,if=none,id=drive-ide0-0-0,readonly=on \ -device ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ --drive file=ftps://host.name:990/url/path/file.iso,format=raw,if=none,id=drive-ide0-0-1,readonly=on \ +-object secret,id=ide0-0-1-auth-secret0,data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=ftps://host.name:990/url/path/file.iso,file.password-secret=ide0-0-1-auth-secret0,format=raw,if=none,id=drive-ide0-0-1,readonly=on \ -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ --drive 'file=https://host.name:443/url/path/file.iso?test=val,format=raw,if=none,id=drive...' \ +-object secret,id=ide0-1-0-auth-secret0,data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive 'file=https://host.name:443/url/path/file.iso?test=val,file.password-secret=ide0-1...' \ -device ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args index 6bc09072eb..267e4cb47b 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args +++ b/tests/qemuxml2argvdata/disk-cdrom-network.x86_64-latest.args @@ -30,10 +30,12 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -blockdev '{"driver":"ftp","url":"ftp://host.name:21/url/path/file.iso","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-3-format","read-only":true,"driver":"raw","file":"libvirt-3-storage"}' \ -device '{"driver":"ide-cd","bus":"ide.0","unit":0,"drive":"libvirt-3-format","id":"ide0-0-0","bootindex":1}' \ --blockdev '{"driver":"ftps","url":"ftps://host.name:990/url/path/file.iso","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ +-object '{"qom-type":"secret","id":"libvirt-2-storage-auth-secret0","data":"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1","keyid":"masterKey0","iv":"AAECAwQFBgcICQoLDA0ODw==","format":"base64"}' \ +-blockdev '{"driver":"ftps","url":"ftps://host.name:990/url/path/file.iso","username":"testuser","password-secret":"libvirt-2-storage-auth-secret0","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"raw","file":"libvirt-2-storage"}' \ -device '{"driver":"ide-cd","bus":"ide.0","unit":1,"drive":"libvirt-2-format","id":"ide0-0-1"}' \ --blockdev '{"driver":"https","url":"https://host.name:443/url/path/file.iso?test=val","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-object '{"qom-type":"secret","id":"libvirt-1-storage-auth-secret0","data":"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1","keyid":"masterKey0","iv":"AAECAwQFBgcICQoLDA0ODw==","format":"base64"}' \ +-blockdev '{"driver":"https","url":"https://host.name:443/url/path/file.iso?test=val","username":"testuser","password-secret":"libvirt-1-storage-auth-secret0","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":true,"driver":"raw","file":"libvirt-1-storage"}' \ -device '{"driver":"ide-cd","bus":"ide.1","unit":0,"drive":"libvirt-1-format","id":"ide0-1-0"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ diff --git a/tests/qemuxml2argvdata/disk-cdrom-network.xml b/tests/qemuxml2argvdata/disk-cdrom-network.xml index 14872d8889..40c53dd8fc 100644 --- a/tests/qemuxml2argvdata/disk-cdrom-network.xml +++ b/tests/qemuxml2argvdata/disk-cdrom-network.xml @@ -32,6 +32,9 @@ <driver name='qemu' type='raw'/> <source protocol='ftps' name='/url/path/file.iso'> <host name='host.name' port='990'/> + <auth username='testuser'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> </source> <target dev='hdb' bus='ide'/> <readonly/> @@ -41,6 +44,9 @@ <driver name='qemu' type='raw'/> <source protocol='https' name='/url/path/file.iso' query='test=val'> <host name='host.name' port='443'/> + <auth username='testuser'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> </source> <target dev='hdc' bus='ide'/> <readonly/> diff --git a/tests/qemuxml2xmloutdata/disk-cdrom-network.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-cdrom-network.x86_64-latest.xml new file mode 100644 index 0000000000..86f8ccf921 --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-cdrom-network.x86_64-latest.xml @@ -0,0 +1,72 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='network' device='cdrom'> + <driver name='qemu' type='raw'/> + <source protocol='ftp' name='/url/path/file.iso'> + <host name='host.name' port='21'/> + </source> + <target dev='hda' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='cdrom'> + <driver name='qemu' type='raw'/> + <source protocol='ftps' name='/url/path/file.iso'> + <host name='host.name' port='990'/> + <auth username='testuser'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='hdb' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </disk> + <disk type='network' device='cdrom'> + <driver name='qemu' type='raw'/> + <source protocol='https' name='/url/path/file.iso' query='test=val'> + <host name='host.name' port='443'/> + <auth username='testuser'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 16d2269423..58ee29cd33 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -322,6 +322,7 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-aio-io_uring"); DO_TEST_NOCAPS("disk-cdrom"); DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid"); + DO_TEST_CAPS_LATEST("disk-cdrom-network"); DO_TEST("disk-cdrom-bus-other", QEMU_CAPS_DEVICE_USB_STORAGE); DO_TEST_NOCAPS("disk-floppy"); DO_TEST("disk-usb-device", QEMU_CAPS_DEVICE_USB_STORAGE); -- 2.35.1
On a Friday in 2022, Peter Krempa wrote:
The crash is fixed by properly instantiating the auth object.
Also this series cleans up some problems noticed along the way.
Peter Krempa (11): conf: schema: Fix alignment in 'diskSourceNetworkProtocolHTTPS' schema definition qemuDomainValidateStorageSource: Reorganize encryption config validation qemuDomainValidateStorageSource: Add validation of 'encryption' support qemuBlockStorageSourceGetRBDProps: Simplify handling of encryption format virDomainDiskDefValidateSourceChainOne: Reject authentication for protocols which don't support it qemuDomainStorageSourceHasAuth: Don't decide based on protocol qemu: domain: Inline qemuDomainDiskHasEncryptionSecret qemu: domain: Inline qemuDomainStorageSourceHasAuth conf: schemas: Split out definition for 'ftp' protocol conf: schema: Allow authentication for FTP(S) and HTTP(S) disks tests: qemuxml2*: Add testing of authenticated http/ftp disks
src/conf/domain_validate.c | 34 ++++++ src/conf/schemas/domaincommon.rng | 35 +++++- src/qemu/qemu_block.c | 10 +- src/qemu/qemu_domain.c | 103 ++++++------------ src/qemu/qemu_domain.h | 3 - .../disk-cdrom-network.x86_64-4.1.0.args | 6 +- .../disk-cdrom-network.x86_64-latest.args | 6 +- tests/qemuxml2argvdata/disk-cdrom-network.xml | 6 + .../disk-encryption-wrong.x86_64-latest.err | 1 + .../disk-encryption-wrong.xml | 37 +++++++ ...-network-rbd-encryption.x86_64-latest.args | 2 +- .../disk-network-rbd-encryption.xml | 2 +- tests/qemuxml2argvtest.c | 1 + .../disk-cdrom-network.x86_64-latest.xml | 72 ++++++++++++ ...k-network-rbd-encryption.x86_64-latest.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 16 files changed, 233 insertions(+), 88 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/disk-encryption-wrong.xml create mode 100644 tests/qemuxml2xmloutdata/disk-cdrom-network.x86_64-latest.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa