Since the virSecretDefParseUsage ensures each of the fields is present, no need to check during virSecretDefFormatUsage Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/secret_conf.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index de9e6cf..d510645 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -286,23 +286,15 @@ virSecretDefFormatUsage(virBufferPtr buf, break; case VIR_SECRET_USAGE_TYPE_VOLUME: - if (def->usage.volume != NULL) - virBufferEscapeString(buf, "<volume>%s</volume>\n", - def->usage.volume); + virBufferEscapeString(buf, "<volume>%s</volume>\n", def->usage.volume); break; case VIR_SECRET_USAGE_TYPE_CEPH: - if (def->usage.ceph != NULL) { - virBufferEscapeString(buf, "<name>%s</name>\n", - def->usage.ceph); - } + virBufferEscapeString(buf, "<name>%s</name>\n", def->usage.ceph); break; case VIR_SECRET_USAGE_TYPE_ISCSI: - if (def->usage.target != NULL) { - virBufferEscapeString(buf, "<target>%s</target>\n", - def->usage.target); - } + virBufferEscapeString(buf, "<target>%s</target>\n", def->usage.target); break; default: -- 2.5.5

Add a new secret type known as "passphrase" - it will handle adding the secret objects that need a passphrase without a specific username. The format is: <secret ...> <uuid>...</uuid> ... <usage type='passphrase'> <name>mumblyfratz</name> </usage> </secret> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/aclpolkit.html.in | 4 +++ docs/formatsecret.html.in | 57 ++++++++++++++++++++++++++++-- docs/schemas/secret.rng | 10 ++++++ include/libvirt/libvirt-secret.h | 3 +- src/access/viraccessdriverpolkit.c | 13 +++++++ src/conf/secret_conf.c | 22 +++++++++++- src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 5 +++ tests/secretxml2xmlin/usage-passphrase.xml | 7 ++++ tests/secretxml2xmltest.c | 1 + 10 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-passphrase.xml diff --git a/docs/aclpolkit.html.in b/docs/aclpolkit.html.in index dae0814..4d0307d 100644 --- a/docs/aclpolkit.html.in +++ b/docs/aclpolkit.html.in @@ -224,6 +224,10 @@ <td>secret_usage_target</td> <td>Name of the associated iSCSI target, if any</td> </tr> + <tr> + <td>secret_usage_name</td> + <td>Name of be associated passphrase secret, if any</td> + </tr> </tbody> </table> diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index c39d2a7..0d53c11 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -41,8 +41,9 @@ <dd> Specifies what this secret is used for. A mandatory <code>type</code> attribute specifies the usage category, currently - only <code>volume</code>, <code>ceph</code> and <code>iscsi</code> - are defined. Specific usage categories are described below. + only <code>volume</code>, <code>ceph</code>, <code>iscsi</code>, + and <code>passphrase</code> are defined. Specific usage categories + are described below. </dd> </dl> @@ -241,5 +242,57 @@ &lt;secret usage='libvirtiscsi'/&gt; &lt;/auth&gt; </pre> + + <h3><a name="passphraseUsageType">Usage type "passphrase"</a></h3> + + <p> + This secret is a general purpose secret to be used by various libvirt + objects to provide a single passphrase as required by the object in + order to perform its authentication. + <span class="since">Since 2.0.0</span>. The following is an example + of a secret.xml file: + </p> + + <pre> + # cat secret.xml + &lt;secret ephemeral='no' private='yes'&gt; + &lt;description&gt;sample passphrase secret&lt;/description&gt; + &lt;usage type='passphrase'&gt; + &lt;name&gt;name_example&lt;/name&gt; + &lt;/usage&gt; + &lt;/secret&gt; + + # virsh secret-define secret.xml + Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created + + # virsh secret-list + UUID Usage + ----------------------------------------------------------- + 718c71bd-67b5-4a2b-87ec-a24e8ca200dc passphrase name_example + # + + </pre> + + <p> + A secret may also be defined via the + <a href="html/libvirt-libvirt-secret.html#virSecretDefineXML"> + <code>virSecretDefineXML</code></a> API. + + Once the secret is defined, a secret value will need to be set. This + value would be the same used to create and use the volume. + The following is a simple example of using + <code>virsh secret-set-value</code> to set the secret value. The + <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> + <code>virSecretSetValue</code></a> API may also be used to set + a more secure secret without using printable/readable characters. + </p> + + <pre> + # MYSECRET=`printf %s "letmein" | base64` + # virsh secret-set-value 718c71bd-67b5-4a2b-87ec-a24e8ca200dc $MYSECRET + Secret value set + + </pre> + </body> </html> diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng index e21e700..cac8560 100644 --- a/docs/schemas/secret.rng +++ b/docs/schemas/secret.rng @@ -36,6 +36,7 @@ <ref name='usagevolume'/> <ref name='usageceph'/> <ref name='usageiscsi'/> + <ref name='usagepassphrase'/> <!-- More choices later --> </choice> </element> @@ -71,4 +72,13 @@ </element> </define> + <define name='usagepassphrase'> + <attribute name='type'> + <value>passphrase</value> + </attribute> + <element name='name'> + <ref name='genericName'/> + </element> + </define> + </grammar> diff --git a/include/libvirt/libvirt-secret.h b/include/libvirt/libvirt-secret.h index 3e5cdf6..55b11e0 100644 --- a/include/libvirt/libvirt-secret.h +++ b/include/libvirt/libvirt-secret.h @@ -4,7 +4,7 @@ * Description: Provides APIs for the management of secrets * Author: Daniel Veillard <veillard(a)redhat.com&gt; * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -43,6 +43,7 @@ typedef enum { VIR_SECRET_USAGE_TYPE_VOLUME = 1, VIR_SECRET_USAGE_TYPE_CEPH = 2, VIR_SECRET_USAGE_TYPE_ISCSI = 3, + VIR_SECRET_USAGE_TYPE_PASSPHRASE = 4, # ifdef VIR_ENUM_SENTINELS VIR_SECRET_USAGE_TYPE_LAST diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index 89bc890..99b867f 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -338,6 +338,19 @@ virAccessDriverPolkitCheckSecret(virAccessManagerPtr manager, virAccessPermSecretTypeToString(perm), attrs); } break; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: { + const char *attrs[] = { + "connect_driver", driverName, + "secret_uuid", uuidstr, + "secret_usage_name", secret->usage.name, + NULL, + }; + + return virAccessDriverPolkitCheck(manager, + "secret", + virAccessPermSecretTypeToString(perm), + attrs); + } break; } } diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index d510645..a973aa9 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "secret_conf.h" #include "virsecretobj.h" +#include "virstring.h" #include "virerror.h" #include "virxml.h" #include "viruuid.h" @@ -38,7 +39,7 @@ VIR_LOG_INIT("conf.secret_conf"); VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, - "none", "volume", "ceph", "iscsi") + "none", "volume", "ceph", "iscsi", "passphrase") const char * virSecretUsageIDForDef(virSecretDefPtr def) @@ -56,6 +57,9 @@ virSecretUsageIDForDef(virSecretDefPtr def) case VIR_SECRET_USAGE_TYPE_ISCSI: return def->usage.target; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + return def->usage.name; + default: return NULL; } @@ -85,6 +89,10 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def->usage.target); break; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + VIR_FREE(def->usage.name); + break; + default: VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); break; @@ -145,6 +153,14 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (!(def->usage.name = virXPathString("string(./usage/name)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("passphrase usage specified, but name is missing")); + return -1; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -297,6 +313,10 @@ virSecretDefFormatUsage(virBufferPtr buf, virBufferEscapeString(buf, "<target>%s</target>\n", def->usage.target); break; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + virBufferEscapeString(buf, "<name>%s</name>\n", def->usage.name); + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 4584403..c34880f 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -40,6 +40,7 @@ struct _virSecretDef { char *volume; /* May be NULL */ char *ceph; char *target; + char *name; } usage; }; diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 30a5e80..6714a00 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -237,6 +237,11 @@ virSecretObjSearchName(const void *payload, if (STREQ(secret->def->usage.target, data->usageID)) found = 1; break; + + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (STREQ(secret->def->usage.name, data->usageID)) + found = 1; + break; } cleanup: diff --git a/tests/secretxml2xmlin/usage-passphrase.xml b/tests/secretxml2xmlin/usage-passphrase.xml new file mode 100644 index 0000000..2b94b80 --- /dev/null +++ b/tests/secretxml2xmlin/usage-passphrase.xml @@ -0,0 +1,7 @@ +<secret ephemeral='no' private='no'> + <uuid>f52a81b2-424e-490c-823d-6bd4235bc572</uuid> + <description>Sample Passphrase Secret</description> + <usage type='passphrase'> + <name>mumblyfratz</name> + </usage> +</secret> diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index 8dcbb40..c444e4d 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -80,6 +80,7 @@ mymain(void) DO_TEST("usage-volume"); DO_TEST("usage-ceph"); DO_TEST("usage-iscsi"); + DO_TEST("usage-passphrase"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.5.5

Add parse and format of the luks/passphrase secret including tests for volume XML parsing. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatsecret.html.in | 7 +++- docs/formatstorageencryption.html.in | 26 ++++++++++++- docs/schemas/storagecommon.rng | 2 + src/qemu/qemu_process.c | 6 +++ src/storage/storage_backend.c | 3 +- src/storage/storage_backend_fs.c | 7 +++- src/storage/storage_backend_gluster.c | 2 + src/util/virstorageencryption.c | 2 +- src/util/virstorageencryption.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 45 ++++++++++++++++++++++ .../qemuxml2xmlout-luks-disks.xml | 1 + tests/qemuxml2xmltest.c | 1 + tests/storagevolxml2xmlin/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmlout/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmltest.c | 1 + 15 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml create mode 100644 tests/storagevolxml2xmlin/vol-luks.xml create mode 100644 tests/storagevolxml2xmlout/vol-luks.xml diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 0d53c11..928f910 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -248,7 +248,12 @@ <p> This secret is a general purpose secret to be used by various libvirt objects to provide a single passphrase as required by the object in - order to perform its authentication. + order to perform its authentication. For example, this secret will + be used either by the + <a href="formatstorage.html#StorageVol">storage volume</a> in order to + provide the passphrase to encrypt a luks volume or by the + <a href="formatdomain.html#elementsDisks">disk device</a> in order to + provide the passphrase to decrypt the luks volume for usage. <span class="since">Since 2.0.0</span>. The following is an example of a secret.xml file: </p> diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in index 540ab18..be73054 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -56,8 +56,20 @@ the <code>secret</code> element is not present during volume creation, a secret is automatically generated and attached to the volume. </p> + <h3><a name="StorageEncryptionLuks">"luks" format</a></h3> + <p> + The <code>luks</code> format is specific to a luks encrypted volume + and the secret used in order to either encrypt or decrypt the volume. + A single <code>&lt;secret type='passphrase'...&gt;</code> element is + expected. The secret may be referenced via either a <code>uuid</code> or + <code>usage</code> attribute. One of the two must be present. When + present for volume creation, the secret will be used in order for + volume encryption. When present for domain usage, the secret will + be used as the passphrase to decrypt the volume. + <span class="since">Since 2.0.0</span>. + </p> - <h2><a name="example">Example</a></h2> + <h2><a name="example">Examples</a></h2> <p> Here is a simple example, specifying use of the <code>qcow</code> format: @@ -67,5 +79,17 @@ &lt;encryption format='qcow'&gt; &lt;secret type='passphrase' uuid='c1f11a6d-8c5d-4a3e-ac7a-4e171c5e0d4a' /&gt; &lt;/encryption&gt;</pre> + + <p> + Here is a simple example, specifying use of the <code>luks</code> format + where it's assumed that a <code>secret</code> has been defined using a + <code>usage</code> element with a <code>id</code> of "luks_example": + </p> + <pre> + &lt;encryption format='luks'&gt; + &lt;secret type='passphrase' usage='luks_example'/&gt; + &lt;/encryption&gt; + </pre> + </body> </html> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index c5b71de..63b55b4 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -12,6 +12,7 @@ <choice> <value>default</value> <value>qcow</value> + <value>luks</value> </choice> </attribute> <zeroOrMore> @@ -81,6 +82,7 @@ <value>fat</value> <value>vhd</value> <value>ploop</value> + <value>luks</value> <ref name='storageFormatBacking'/> </choice> </define> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7d56ec8..d4c49eb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2411,6 +2411,12 @@ qemuProcessInitPasswords(virConnectPtr conn, !virDomainDiskGetSource(vm->def->disks[i])) continue; + if (vm->def->disks[i]->src->encryption->format != + VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT && + vm->def->disks[i]->src->encryption->format != + VIR_STORAGE_ENCRYPTION_FORMAT_QCOW) + continue; + VIR_FREE(secret); if (qemuProcessGetVolumeQcowPassphrase(conn, vm->def->disks[i], diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d6a451d..97f6ffe 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1027,8 +1027,7 @@ virStorageBackendCreateQemuImgCheckEncryption(int format, } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("qcow volume encryption unsupported with " - "volume format %s"), type); + _("volume encryption unsupported with format %s"), type); return -1; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 839a2c7..0a12ecb 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -157,7 +157,12 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, case VIR_STORAGE_FILE_QCOW2: (*encryption)->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; break; - default: + + case VIR_STORAGE_FILE_LUKS: + (*encryption)->format = VIR_STORAGE_ENCRYPTION_FORMAT_LUKS; + break; + + case VIR_STORAGE_ENCRYPTION_FORMAT_LAST: break; } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0085052..eda060d 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -321,6 +321,8 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (vol->target.format == VIR_STORAGE_FILE_QCOW || vol->target.format == VIR_STORAGE_FILE_QCOW2) vol->target.encryption->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; + if (vol->target.format == VIR_STORAGE_FILE_LUKS) + vol->target.encryption->format = VIR_STORAGE_ENCRYPTION_FORMAT_LUKS; } vol->target.features = meta->features; meta->features = NULL; diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index afb44da..8575416 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -43,7 +43,7 @@ VIR_ENUM_IMPL(virStorageEncryptionSecret, VIR_ENUM_IMPL(virStorageEncryptionFormat, VIR_STORAGE_ENCRYPTION_FORMAT_LAST, - "default", "qcow") + "default", "qcow", "luks") static void virStorageEncryptionSecretFree(virStorageEncryptionSecretPtr secret) diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index c68c66e..5e1be3b 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -48,6 +48,7 @@ typedef enum { /* "default" is only valid for volume creation */ VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0, VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ + VIR_STORAGE_ENCRYPTION_FORMAT_LUKS, VIR_STORAGE_ENCRYPTION_FORMAT_LAST, } virStorageEncryptionFormatType; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -0,0 +1,45 @@ +<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-i440fx-2.1'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='luks'/> + <source file='/storage/guest_disks/encryptdisk'/> + <target dev='vda' bus='virtio'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='luks'/> + <source file='/storage/guest_disks/encryptdisk2'/> + <target dev='vdb' bus='virtio'/> + <encryption format='luks'> + <secret type='passphrase' usage='mycluster_myname'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' 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/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml new file mode 120000 index 0000000..b59dc67 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-luks-disks.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index d045fd4..6c566b3 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -502,6 +502,7 @@ mymain(void) DO_TEST("encrypted-disk"); DO_TEST("encrypted-disk-usage"); + DO_TEST("luks-disks"); DO_TEST("memtune"); DO_TEST("memtune-unlimited"); DO_TEST("blkiotune"); diff --git a/tests/storagevolxml2xmlin/vol-luks.xml b/tests/storagevolxml2xmlin/vol-luks.xml new file mode 100644 index 0000000..eb4dc41 --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-luks.xml @@ -0,0 +1,21 @@ +<volume> + <name>LuksDemo.img</name> + <key>/var/lib/libvirt/images/LuksDemo.img</key> + <source> + </source> + <capacity unit="G">5</capacity> + <allocation>294912</allocation> + <target> + <path>/var/lib/libvirt/images/LuksDemo.img</path> + <format type='luks'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='luks'> + <secret type='passphrase' usage='mumblyfratz'/> + </encryption> + </target> +</volume> diff --git a/tests/storagevolxml2xmlout/vol-luks.xml b/tests/storagevolxml2xmlout/vol-luks.xml new file mode 100644 index 0000000..5b764b7 --- /dev/null +++ b/tests/storagevolxml2xmlout/vol-luks.xml @@ -0,0 +1,21 @@ +<volume type='file'> + <name>LuksDemo.img</name> + <key>/var/lib/libvirt/images/LuksDemo.img</key> + <source> + </source> + <capacity unit='bytes'>5368709120</capacity> + <allocation unit='bytes'>294912</allocation> + <target> + <path>/var/lib/libvirt/images/LuksDemo.img</path> + <format type='luks'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='luks'> + <secret type='passphrase' usage='mumblyfratz'/> + </encryption> + </target> +</volume> diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index f722452..a36a706 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -105,6 +105,7 @@ mymain(void) DO_TEST("pool-dir", "vol-qcow2-lazy"); DO_TEST("pool-dir", "vol-qcow2-0.10-lazy"); DO_TEST("pool-dir", "vol-qcow2-nobacking"); + DO_TEST("pool-dir", "vol-luks"); DO_TEST("pool-disk", "vol-partition"); DO_TEST("pool-logical", "vol-logical"); DO_TEST("pool-logical", "vol-logical-backing"); -- 2.5.5

For a luks device, allow the configuration of a specific cipher to be used for encrypting the volume. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatstorageencryption.html.in | 83 ++++++++++++- docs/schemas/storagecommon.rng | 44 ++++++- src/conf/domain_conf.c | 11 ++ src/util/virstorageencryption.c | 138 +++++++++++++++++++++ src/util/virstorageencryption.h | 14 +++ .../qemuxml2argv-luks-disk-cipher.xml | 45 +++++++ .../qemuxml2xmlout-luks-disk-cipher.xml | 1 + tests/qemuxml2xmltest.c | 1 + tests/storagevolxml2xmlin/vol-luks-cipher.xml | 23 ++++ tests/storagevolxml2xmlout/vol-luks-cipher.xml | 23 ++++ tests/storagevolxml2xmltest.c | 1 + 11 files changed, 378 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml create mode 100644 tests/storagevolxml2xmlin/vol-luks-cipher.xml create mode 100644 tests/storagevolxml2xmlout/vol-luks-cipher.xml diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in index be73054..0f8401e 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -68,6 +68,60 @@ be used as the passphrase to decrypt the volume. <span class="since">Since 2.0.0</span>. </p> + <p> + For volume creation, it is possible to specify the encryption + algorithm used to encrypt the luks volume. The following two + optional elements may be provided for that purpose. It is hypervisor + dependent as to which algorithms are supported. The default algorithm + used by the storage driver backend when using qemu-img to create + the volume is 'aes-256-cbc' using 'essiv' for initialization vector + generation and 'sha256' hash algorithm for both the cipher and the + initialization vector generation. + </p> + + <dl> + <dt><code>cipher</code></dt> + <dd>This element describes the cipher algorithm to be used to either + encrypt or decrypt the luks volume. This element has the following + attributes: + <dl> + <dt><code>name</code></dt> + <dd>The name of the cipher algorithm used for data encryption, + such as 'aes', 'des', 'cast5', 'serpent', 'twofish', etc. + Support of the specific algorithm is storage driver + implementation dependent.</dd> + <dt><code>size</code></dt> + <dd>The size of the cipher in bits, such as '256', '192', '128', + etc. Support of the specific size for a specific cipher is + hypervisor dependent.</dd> + <dt><code>mode</code></dt> + <dd>An optional cipher algorithm mode such as 'cbc', 'xts', + 'ecb', etc. Support of the specific cipher mode is + hypervisor dependent.</dd> + <dt><code>hash</code></dt> + <dd>An optional master key hash algorithm such as 'md5', 'sha1', + 'sha256', etc. Support of the specific hash algorithm is + hypervisor dependent.</dd> + </dl> + </dd> + <dt><code>ivgen</code></dt> + <dd>This optional element describes the initialization vector + generation algorithm used in conjunction with the + <code>cipher</code>. If the <code>cipher</code> is not provided, + then an error will be generated by the parser. + <dl> + <dt><code>name</code></dt> + <dd>The name of the algorithm, such as 'plain', 'plain64', + 'essiv', etc. Support of the specific algorithm is hypervisor + dependent.</dd> + <dt><code>hash</code></dt> + <dd>An optional hash algorithm such as 'md5', 'sha1', 'sha256', + etc. Support of the specific ivgen hash algorithm is hypervisor + dependent.</dd> + </dl> + </dd> + </dl> + <h2><a name="example">Examples</a></h2> @@ -81,9 +135,12 @@ &lt;/encryption&gt;</pre> <p> - Here is a simple example, specifying use of the <code>luks</code> format - where it's assumed that a <code>secret</code> has been defined using a - <code>usage</code> element with a <code>id</code> of "luks_example": + Assuming a <a href="formatsecret.html#luksUsageType"> + <code>luks secret</code></a> is already defined using a + <code>usage</code> element with an <code>name</code> of "luks_example", + a simple example specifying use of the <code>luks</code> format + for either volume creation without a specific cipher being defined or + as part of a domain volume definition: </p> <pre> &lt;encryption format='luks'&gt; @@ -91,5 +148,25 @@ &lt;/encryption&gt; </pre> + <p> + Here is an example, specifying use of the <code>luks</code> format for + a specific cipher algorihm for volume creation: + </p> + <pre> + &lt;volume&gt; + &lt;name&gt;twofish.luks&lt;/name&gt; + &lt;capacity unit='G'&gt;5&lt;/capacity&gt; + &lt;target&gt; + &lt;path&gt;/var/lib/libvirt/images/demo.luks&lt;/path&gt; + &lt;format type='luks'/&gt; + &lt;encryption format='luks'&gt; + &lt;secret type='passphrase' usage='luks_example'/&gt; + &lt;cipher name='twofish' size='256' mode='cbc' hash='sha256'/&gt; + &lt;ivgen name='plain64' hash='sha256'/&gt; + &lt;/encryption&gt; + &lt;/target&gt; + &lt;/volume&gt; + </pre> + </body> </html> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 63b55b4..316fbae 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -15,9 +15,19 @@ <value>luks</value> </choice> </attribute> - <zeroOrMore> - <ref name='secret'/> - </zeroOrMore> + <interleave> + <zeroOrMore> + <ref name='secret'/> + </zeroOrMore> + <optional> + <element name='cipher'> + <ref name='keycipher'/> + </element> + <element name='ivgen'> + <ref name='keyivgen'/> + </element> + </optional> + </interleave> </element> </define> @@ -136,4 +146,32 @@ </optional> </define> + <define name='keycipher'> + <attribute name='name'> + <text/> + </attribute> + <attribute name='size'> + <ref name="unsignedInt"/> + </attribute> + <optional> + <attribute name='mode'> + <text/> + </attribute> + <attribute name='hash'> + <text/> + </attribute> + </optional> + </define> + + <define name='keyivgen'> + <attribute name='name'> + <text/> + </attribute> + <optional> + <attribute name='hash'> + <text/> + </attribute> + </optional> + </define> + </grammar> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 79d15c8..8d3a132 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7857,6 +7857,17 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->startupPolicy = val; } + if (encryption) { + if (encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && + encryption->encinfo.cipher_name) { + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("supplying the <cipher> for a domain is " + "unnecessary")); + goto error; + } + } + def->dst = target; target = NULL; def->src->auth = authdef; diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index 8575416..754444d 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -35,6 +35,7 @@ #include "viruuid.h" #include "virfile.h" #include "virsecret.h" +#include "virstring.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -46,6 +47,17 @@ VIR_ENUM_IMPL(virStorageEncryptionFormat, "default", "qcow", "luks") static void +virStorageEncryptionInfoDefFree(virStorageEncryptionInfoDefPtr def) +{ + VIR_FREE(def->cipher_name); + VIR_FREE(def->cipher_mode); + VIR_FREE(def->cipher_hash); + VIR_FREE(def->ivgen_name); + VIR_FREE(def->ivgen_hash); +} + + +static void virStorageEncryptionSecretFree(virStorageEncryptionSecretPtr secret) { if (!secret) @@ -63,6 +75,7 @@ virStorageEncryptionFree(virStorageEncryptionPtr enc) for (i = 0; i < enc->nsecrets; i++) virStorageEncryptionSecretFree(enc->secrets[i]); + virStorageEncryptionInfoDefFree(&enc->encinfo); VIR_FREE(enc->secrets); VIR_FREE(enc); } @@ -80,6 +93,23 @@ virStorageEncryptionSecretCopy(const virStorageEncryptionSecret *src) return ret; } + +static int +virStorageEncryptionInfoDefCopy(const virStorageEncryptionInfoDef *src, + virStorageEncryptionInfoDefPtr dst) +{ + dst->cipher_size = src->cipher_size; + if (VIR_STRDUP(dst->cipher_name, src->cipher_name) < 0 || + VIR_STRDUP(dst->cipher_mode, src->cipher_mode) < 0 || + VIR_STRDUP(dst->cipher_hash, src->cipher_hash) < 0 || + VIR_STRDUP(dst->ivgen_name, src->ivgen_name) < 0 || + VIR_STRDUP(dst->ivgen_hash, src->ivgen_hash) < 0) + return -1; + + return 0; +} + + virStorageEncryptionPtr virStorageEncryptionCopy(const virStorageEncryption *src) { @@ -100,6 +130,9 @@ virStorageEncryptionCopy(const virStorageEncryption *src) goto error; } + if (virStorageEncryptionInfoDefCopy(&src->encinfo, &ret->encinfo) < 0) + goto error; + return ret; error: @@ -153,6 +186,63 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, return NULL; } + +static int +virStorageEncryptionInfoParseCipher(xmlNodePtr info_node, + virStorageEncryptionInfoDefPtr info) +{ + int ret = -1; + char *size_str = NULL; + + if (!(info->cipher_name = virXMLPropString(info_node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing cipher info name string")); + goto cleanup; + } + + if ((size_str = virXMLPropString(info_node, "size")) && + virStrToLong_uip(size_str, NULL, 10, &info->cipher_size) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse cipher info size string '%s'"), + size_str); + goto cleanup; + } + + if (!size_str) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cipher info missing 'size' attribute")); + goto cleanup; + } + + /* Optional */ + info->cipher_mode = virXMLPropString(info_node, "mode"); + info->cipher_hash = virXMLPropString(info_node, "hash"); + + ret = 0; + + cleanup: + VIR_FREE(size_str); + return ret; +} + + +static int +virStorageEncryptionInfoParseIvgen(xmlNodePtr info_node, + virStorageEncryptionInfoDefPtr info) +{ + if (!(info->ivgen_name = virXMLPropString(info_node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing ivgen info name string")); + return -1; + } + + /* Optional */ + info->ivgen_hash = virXMLPropString(info_node, "hash"); + + return 0; +} + + static virStorageEncryptionPtr virStorageEncryptionParseXML(xmlXPathContextPtr ctxt) { @@ -196,6 +286,28 @@ virStorageEncryptionParseXML(xmlXPathContextPtr ctxt) VIR_FREE(nodes); } + if (ret->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { + xmlNodePtr tmpnode; + + if ((tmpnode = virXPathNode("./cipher[1]", ctxt))) { + if (virStorageEncryptionInfoParseCipher(tmpnode, &ret->encinfo) < 0) + goto cleanup; + } + + if ((tmpnode = virXPathNode("./ivgen[1]", ctxt))) { + /* If no cipher node, then fail */ + if (!ret->encinfo.cipher_name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage encryption cipher")); + goto cleanup; + } + + if (virStorageEncryptionInfoParseIvgen(tmpnode, &ret->encinfo) < 0) + goto cleanup; + } + } + + return ret; cleanup: @@ -250,6 +362,28 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, return 0; } + +static void +virStorageEncryptionInfoDefFormat(virBufferPtr buf, + const virStorageEncryptionInfoDef *enc) +{ + virBufferAsprintf(buf, "<cipher name='%s' size='%u'", + enc->cipher_name, enc->cipher_size); + if (enc->cipher_mode) + virBufferAsprintf(buf, " mode='%s'", enc->cipher_mode); + if (enc->cipher_hash) + virBufferAsprintf(buf, " hash='%s'", enc->cipher_hash); + virBufferAddLit(buf, "/>\n"); + + if (enc->ivgen_name) { + virBufferAsprintf(buf, "<ivgen name='%s'", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferAsprintf(buf, " hash='%s'", enc->ivgen_hash); + virBufferAddLit(buf, "/>\n"); + } +} + + int virStorageEncryptionFormat(virBufferPtr buf, virStorageEncryptionPtr enc) @@ -270,6 +404,10 @@ virStorageEncryptionFormat(virBufferPtr buf, return -1; } + if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && + enc->encinfo.cipher_name) + virStorageEncryptionInfoDefFormat(buf, &enc->encinfo); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</encryption>\n"); diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index 5e1be3b..fa439fb 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -44,6 +44,18 @@ struct _virStorageEncryptionSecret { virSecretLookupTypeDef seclookupdef; }; +/* It's possible to dictate the cipher and if necessary iv */ +typedef struct _virStorageEncryptionInfoDef virStorageEncryptionInfoDef; +typedef virStorageEncryptionInfoDef *virStorageEncryptionInfoDefPtr; +struct _virStorageEncryptionInfoDef { + unsigned int cipher_size; + char *cipher_name; + char *cipher_mode; + char *cipher_hash; + char *ivgen_name; + char *ivgen_hash; +}; + typedef enum { /* "default" is only valid for volume creation */ VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0, @@ -61,6 +73,8 @@ struct _virStorageEncryption { size_t nsecrets; virStorageEncryptionSecretPtr *secrets; + + virStorageEncryptionInfoDef encinfo; }; virStorageEncryptionPtr virStorageEncryptionCopy(const virStorageEncryption *src) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml @@ -0,0 +1,45 @@ +<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-i440fx-2.1'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='luks'/> + <source file='/storage/guest_disks/encryptdisk'/> + <target dev='vda' bus='virtio'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='luks'/> + <source file='/storage/guest_disks/encryptdisk2'/> + <target dev='vdb' bus='virtio'/> + <encryption format='luks'> + <secret type='passphrase' usage='mycluster_myname'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' 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/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml new file mode 120000 index 0000000..fa55233 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6c566b3..2056c03 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -503,6 +503,7 @@ mymain(void) DO_TEST("encrypted-disk"); DO_TEST("encrypted-disk-usage"); DO_TEST("luks-disks"); + DO_TEST("luks-disk-cipher"); DO_TEST("memtune"); DO_TEST("memtune-unlimited"); DO_TEST("blkiotune"); diff --git a/tests/storagevolxml2xmlin/vol-luks-cipher.xml b/tests/storagevolxml2xmlin/vol-luks-cipher.xml new file mode 100644 index 0000000..009246f --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-luks-cipher.xml @@ -0,0 +1,23 @@ +<volume> + <name>LuksDemo.img</name> + <key>/var/lib/libvirt/images/LuksDemo.img</key> + <source> + </source> + <capacity unit="G">5</capacity> + <allocation>294912</allocation> + <target> + <path>/var/lib/libvirt/images/LuksDemo.img</path> + <format type='luks'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='luks'> + <secret type='passphrase' usage='mumblyfratz'/> + <cipher name='serpent' size='256' mode='cbc' hash='sha256'/> + <ivgen name='plain64' hash='sha256'/> + </encryption> + </target> +</volume> diff --git a/tests/storagevolxml2xmlout/vol-luks-cipher.xml b/tests/storagevolxml2xmlout/vol-luks-cipher.xml new file mode 100644 index 0000000..9014849 --- /dev/null +++ b/tests/storagevolxml2xmlout/vol-luks-cipher.xml @@ -0,0 +1,23 @@ +<volume type='file'> + <name>LuksDemo.img</name> + <key>/var/lib/libvirt/images/LuksDemo.img</key> + <source> + </source> + <capacity unit='bytes'>5368709120</capacity> + <allocation unit='bytes'>294912</allocation> + <target> + <path>/var/lib/libvirt/images/LuksDemo.img</path> + <format type='luks'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='luks'> + <secret type='passphrase' usage='mumblyfratz'/> + <cipher name='serpent' size='256' mode='cbc' hash='sha256'/> + <ivgen name='plain64' hash='sha256'/> + </encryption> + </target> +</volume> diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index a36a706..db82bea 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -106,6 +106,7 @@ mymain(void) DO_TEST("pool-dir", "vol-qcow2-0.10-lazy"); DO_TEST("pool-dir", "vol-qcow2-nobacking"); DO_TEST("pool-dir", "vol-luks"); + DO_TEST("pool-dir", "vol-luks-cipher"); DO_TEST("pool-disk", "vol-partition"); DO_TEST("pool-logical", "vol-logical"); DO_TEST("pool-logical", "vol-logical-backing"); -- 2.5.5

On 06/25/2016 01:46 AM, Ján Tomko wrote: OK OK OK Good question! Unfortunately we have no way of knowing what any driver supports so what are we validating them against? I thought of creating a connection API that could/would fetch all the cipher/ivgen algorithm information from the hypervisor that's going to be using them. Then I investigated qemu to see what type of API was available only to find nothing "obvious". So for now, I just left these as a string. I suppose we could "mock up" a qemu response, but it's a lot more effort saved for another day. If bad values are provided, qemu-img will fall over and play dead during the volume create. Gone. Gone OK, done. Done Thanks for the look/time... John

On Fri, Jun 24, 2016 at 04:53:35PM -0400, John Ferlan wrote: NACK to this function. Old qemu-img supports the -f option and correctly errors out on unsupported LUKS format. There is no reason to probe for it. This won't be needed either. FWIW the whole point of _virStorageBackendQemuImgInfo was to separate command line building from the volume definition, to allow reuse in qemuDomainSnapshotCreateInactiveExternal where we don't deal with a volume. But I never got around to finishing it. Please use virStorageEncryptionInfoDefPtr instead of virStorageVolDefPtr here. Can vol->target.encryption be NULL here? Spurious whitespace change. What is the purpose of this comment? Using VIR_DISPOSE seems superstitious, considering that: we don't use it even once in the secret code and the secrets are in memory all the time. These strings should be escaped or otherwise validated. ACK with the possible segfault fixed and virStorageBackendQemuImgSupportsLuks removed. Jan

Introduce a helper to help determine if a disk src could be possibly used for a disk secret... Going to need this for hot unplug. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 20 +++++++++++++++----- src/qemu/qemu_domain.h | 3 +++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index da5bb79..938b524 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -972,6 +972,20 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) } +bool +qemuDomainSecretDiskCapable(virStorageSourcePtr 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)) + return true; + + return false; +} + + /* qemuDomainSecretDiskPrepare: * @conn: Pointer to connection * @priv: pointer to domain private object @@ -989,11 +1003,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, virStorageSourcePtr src = disk->src; qemuDomainSecretInfoPtr secinfo = NULL; - if (conn && !virStorageSourceIsEmpty(src) && - virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK && - src->auth && - (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI || - src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { + if (conn && qemuDomainSecretDiskCapable(src)) { virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c87dcc7..c49f31c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -670,6 +670,9 @@ void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv); void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1); +bool qemuDomainSecretDiskCapable(virStorageSourcePtr src) + ATTRIBUTE_NONNULL(1); + int qemuDomainSecretDiskPrepare(virConnectPtr conn, qemuDomainObjPrivatePtr priv, virDomainDiskDefPtr disk) -- 2.5.5

Commit id 'a1344f70a' added AES secret processing for RBD when starting up a guest. As such, when the hotplug code calls qemuDomainSecretDiskPrepare an AES secret could be added to the disk about to be hotplugged. If an AES secret was added, then the hotplug code would need to generate the secret object because qemuBuildDriveStr would add the "password-secret=" to the returned 'driveStr' rather than the base64 encoded password. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f695903..fbe3cb8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -310,6 +310,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, bool releaseaddr = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); + virJSONValuePtr secobjProps = NULL; + qemuDomainDiskPrivatePtr diskPriv; + qemuDomainSecretInfoPtr secinfo; if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -342,6 +345,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + secinfo = diskPriv->secinfo; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) + goto error; + } + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; @@ -354,9 +364,15 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; - /* Attach the device - 2 step process */ + /* Attach the device - possible 3 step process */ qemuDomainObjEnterMonitor(driver, vm); + if (secobjProps && qemuMonitorAddObject(priv->mon, "secret", + secinfo->s.aes.alias, + secobjProps) < 0) + goto failaddobjsecret; + secobjProps = NULL; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -374,6 +390,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, ret = 0; cleanup: + virJSONValueFree(secobjProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -393,8 +410,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, } failadddrive: + if (secobjProps) + ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); + + failaddobjsecret: if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; + secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -2791,6 +2813,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, const char *src = virDomainDiskGetSource(disk); qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; + char *objAlias = NULL; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -2801,7 +2824,24 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) return -1; + /* Let's look for some markers for a secret object and create an alias + * object to be used to attempt to delete the object that was created */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && + qemuDomainSecretDiskCapable(disk->src)) { + + if (!(objAlias = qemuDomainGetSecretAESAlias(disk->info.alias))) { + VIR_FREE(drivestr); + return -1; + } + } + qemuDomainObjEnterMonitor(driver, vm); + + /* If it fails, then so be it - it was a best shot */ + if (objAlias) + ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + VIR_FREE(objAlias); + qemuMonitorDriveDel(priv->mon, drivestr); VIR_FREE(drivestr); if (qemuDomainObjExitMonitor(driver, vm) < 0) -- 2.5.5

On 06/25/2016 02:43 AM, Ján Tomko wrote: Understood, but I'm not a fan of that look. I'll keep it the way it is. So you think it looks better to: if (secobjProps && qemuMonitorAddObject(priv->mon, "secret", secinfo->s.aes.alias, secobjProps) < 0) { secobjProps = NULL; goto some-label-to-ExitMonitor; } secobjProps = NULL; That seems to go against other exit/error paths. Is it wrong the way it is? If not, I see no reason to change it. Just secinfo isn't enough, it needs to be secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES as well... If we're _TYPE_PLAIN, then there's nothing to do. I think keeping secobjProps is cleaner. I added code to save the error - although it seems that every one of those labels could end up in the same situation - going through code that could overwrite the error. It'll be fixed for the failure labels as a result of failure while in the monitor, but I think a future cleanup could/should encompass all those labels. cleanup of qemuProcessLaunch was designed to remove the Secrets that we saved away temporarily so that they wouldn't be kept in memory forever (e.g call to qemuDomainSecretDiskDestroy). Hence why the Attach processing needs the qemuDomainSecretDiskPrepare (and also calls qemuDomainSecretDiskDestroy). Thus, the detach processing, then can only work off the alias to remove. Tks - John

On Fri, Jun 24, 2016 at 04:53:39PM -0400, John Ferlan wrote: Since we have not allowed VIR_SECRET_USAGE_TYPE_PASSPHRASE before, can we error out if we don't have both VIR_CRYPTO_CIPHER_AES256CBC and QEMU_CAPS_OBJECT_SECRET to avoid putting plaintext on qemu command line? If you delete this outdated comment in 8/10, you don't have to update it here. s/failaddencsecret/delete_secinfo/ This can also overwrite the error. Don't we need to deal with secinfo like we do for virtio disks? s/failaddencsecret/exit_monitor/ ACK only if we don't need to deal with diskPriv->secinfo; Jan

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021 If the volume xml was looking to create a luks volume take the necessary steps in order to make that happen. The processing will be: 1. create a temporary file in the storage driver state dir path 1a. the file name will include the pool name, the volume name, secret, and XXXXXX for usage in the mkostemp call. 1b. mkostemp the file, initially setting mode to 0600 with current effective uid:gid as owner 1c. fetch the secret into a buffer and write that into the file 2. create a secret object 2a. use an alias combinding the volume name and "_luks0" 2b. add the file to the object 3. create/add luks options to the commandline 3a. at the very least a "key-secret" using the secret object alias 3b. if found in the XML the various "cipher" and "ivgen" options Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- This would need to *replace* the v3 of the series. Difference are completely from jtomko review: 1. Remove virStorageBackendQemuImgSupportsLuks 2. Remove changes in virStorageBackendQEMUImgBackingFormat 3. Add comment prior to _virStorageBackendQemuImgInfo 4. Add check for NULL enc prior to call to virQEMUBuildLuksOpts 5. Use virStorageEncryptionInfoDefPtr as argument in virStorageBackendCreateQemuImgSetOptions rather than vol 6. Changed order of args too... 7. Add local 'enc' to virStorageBackendCreateQemuImgCmdFromVol and only set it if (info.format == VIR_STORAGE_FILE_LUKS) and virStorageBackendCreateQemuImgSecretObject succeeds. Then pass it as argument to virStorageBackendCreateQemuImgSetOptions 8. Removed some unnecessary comment 9. Remove if (imgformat < QEMU_IMG_FORMAT_LUKS) and allow failure of qemu-img to be the failure error message. 10. Use virBufferEscapeString in virQEMUBuildLuksOpts 11. Removed more unnecessary comments for optional... 12. Use requested error message from code review 13. Use virBufferEscapeString in virStorageEncryptionInfoDefFormat src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 218 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 +++++ src/util/virqemu.h | 6 ++ tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 240 insertions(+), 14 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9ac033d..eea18dd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2169,6 +2169,7 @@ virProcessWait; # util/virqemu.h +virQEMUBuildLuksOpts; virQEMUBuildObjectCommandlineFromJSON; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 97f6ffe..a9caa69 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -55,11 +55,14 @@ #include "viralloc.h" #include "internal.h" #include "secret_conf.h" +#include "secret_util.h" #include "viruuid.h" #include "virstoragefile.h" #include "storage_backend.h" #include "virlog.h" #include "virfile.h" +#include "virjson.h" +#include "virqemu.h" #include "stat-time.h" #include "virstring.h" #include "virxml.h" @@ -907,6 +910,7 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) return ret; } + static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { @@ -925,6 +929,10 @@ virStorageBackendQEMUImgBackingFormat(const char *qemuimg) return ret; } +/* The _virStorageBackendQemuImgInfo separates the command line building from + * the volume definition so that qemuDomainSnapshotCreateInactiveExternal can + * use it without needing to deal with a volume. + */ struct _virStorageBackendQemuImgInfo { int format; const char *path; @@ -941,21 +949,36 @@ struct _virStorageBackendQemuImgInfo { const char *inputPath; const char *inputFormatStr; int inputFormat; + + char *secretAlias; + const char *secretPath; }; + static int -virStorageBackendCreateQemuImgOpts(char **opts, +virStorageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr enc, + char **opts, struct _virStorageBackendQemuImgInfo info) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (info.backingPath) - virBufferAsprintf(&buf, "backing_fmt=%s,", - virStorageFileFormatTypeToString(info.backingFormat)); - if (info.encryption) - virBufferAddLit(&buf, "encryption=on,"); - if (info.preallocate) - virBufferAddLit(&buf, "preallocation=metadata,"); + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (!enc) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing luks encryption information")); + goto error; + } + virQEMUBuildLuksOpts(&buf, enc, info.secretAlias); + } else { + if (info.backingPath) + virBufferAsprintf(&buf, "backing_fmt=%s,", + virStorageFileFormatTypeToString(info.backingFormat)); + if (info.encryption) + virBufferAddLit(&buf, "encryption=on,"); + if (info.preallocate) + virBufferAddLit(&buf, "preallocation=metadata,"); + } + if (info.nocow) virBufferAddLit(&buf, "nocow=on,"); @@ -1025,6 +1048,22 @@ virStorageBackendCreateQemuImgCheckEncryption(int format, if (virStorageGenerateQcowEncryption(conn, vol) < 0) return -1; } + } else if (format == VIR_STORAGE_FILE_LUKS) { + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported volume encryption format %d"), + vol->target.encryption->format); + return -1; + } + if (enc->nsecrets > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("too many secrets for luks encryption")); + return -1; + } + if (enc->nsecrets == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no secret provided for luks encryption")); + } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("volume encryption unsupported with format %s"), type); @@ -1069,6 +1108,12 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL; + if (info->format == VIR_STORAGE_FILE_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot set backing store for luks volume")); + return -1; + } + info->backingFormat = vol->target.backingStore->format; info->backingPath = vol->target.backingStore->path; @@ -1122,6 +1167,7 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, static int virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, int imgformat, + virStorageEncryptionInfoDefPtr enc, struct _virStorageBackendQemuImgInfo info) { char *opts = NULL; @@ -1130,7 +1176,7 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) info.compat = "0.10"; - if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) + if (virStorageBackendCreateQemuImgOpts(enc, &opts, info) < 0) return -1; if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); @@ -1140,6 +1186,43 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, } +/* Add a secret object to the command line: + * --object secret,id=$secretAlias,file=$secretPath + * + * NB: format=raw is assumed + */ +static int +virStorageBackendCreateQemuImgSecretObject(virCommandPtr cmd, + virStorageVolDefPtr vol, + struct _virStorageBackendQemuImgInfo *info) +{ + char *str = NULL; + virJSONValuePtr props = NULL; + char *commandStr = NULL; + + if (virAsprintf(&info->secretAlias, "%s_luks0", vol->name) < 0) { + VIR_FREE(str); + return -1; + } + VIR_FREE(str); + + if (virJSONValueObjectCreate(&props, "s:file", info->secretPath, NULL) < 0) + return -1; + + if (!(commandStr = virQEMUBuildObjectCommandlineFromJSON("secret", + info->secretAlias, + props))) { + virJSONValueFree(props); + return -1; + } + virJSONValueFree(props); + + virCommandAddArgList(cmd, "--object", commandStr, NULL); + + return 0; +} + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat */ @@ -1150,7 +1233,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags, const char *create_tool, - int imgformat) + int imgformat, + const char *secretPath) { virCommandPtr cmd = NULL; const char *type; @@ -1162,7 +1246,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .compat = vol->target.compat, .features = vol->target.features, .nocow = vol->target.nocow, + .secretPath = secretPath, + .secretAlias = NULL, }; + virStorageEncryptionInfoDefPtr enc = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1192,6 +1279,18 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (inputvol) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot use inputvol with luks volume")); + return NULL; + } + if (!info.encryption) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing encryption description")); + return NULL; + } + } if (inputvol && virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0) @@ -1226,10 +1325,22 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, if (info.backingPath) virCommandAddArgList(cmd, "-b", info.backingPath, NULL); - if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, info) < 0) { + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (virStorageBackendCreateQemuImgSecretObject(cmd, vol, &info) < 0) { + VIR_FREE(info.secretAlias); + virCommandFree(cmd); + return NULL; + } + enc = &vol->target.encryption->encinfo; + } + + if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, + enc, info) < 0) { + VIR_FREE(info.secretAlias); virCommandFree(cmd); return NULL; } + VIR_FREE(info.secretAlias); if (info.inputPath) virCommandAddArg(cmd, info.inputPath); @@ -1240,6 +1351,77 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return cmd; } + +static char * +virStorageBackendCreateQemuImgSecretPath(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + virStorageEncryptionPtr enc = vol->target.encryption; + char *secretPath = NULL; + int fd = -1; + uint8_t *secret = NULL; + size_t secretlen = 0; + + if (!enc) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing encryption description")); + return NULL; + } + + if (!conn || !conn->secretDriver || + !conn->secretDriver->secretLookupByUUID || + !conn->secretDriver->secretLookupByUsage || + !conn->secretDriver->secretGetValue) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to look up encryption secret")); + return NULL; + } + + if (!(secretPath = virStoragePoolObjBuildTempFilePath(pool, vol))) + goto cleanup; + + if ((fd = mkostemp(secretPath, O_CLOEXEC)) < 0) { + virReportSystemError(errno, "%s", + _("failed to open luks secret file for write")); + goto error; + } + + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, + VIR_SECRET_USAGE_TYPE_PASSPHRASE, + &secret, &secretlen) < 0) + goto error; + + if (safewrite(fd, secret, secretlen) < 0) { + virReportSystemError(errno, "%s", + _("failed to write luks secret file")); + goto error; + } + VIR_FORCE_CLOSE(fd); + + if ((vol->target.perms->uid != (uid_t) -1) && + (vol->target.perms->gid != (gid_t) -1)) { + if (chown(secretPath, vol->target.perms->uid, + vol->target.perms->gid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to chown luks secret file")); + goto error; + } + } + + cleanup: + VIR_DISPOSE_N(secret, secretlen); + VIR_FORCE_CLOSE(fd); + + return secretPath; + + error: + unlink(secretPath); + VIR_FREE(secretPath); + goto cleanup; +} + + int virStorageBackendCreateQemuImg(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -1251,6 +1433,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, char *create_tool; int imgformat; virCommandPtr cmd; + char *secretPath = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); @@ -1266,8 +1449,15 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (imgformat < 0) goto cleanup; + if (vol->target.format == VIR_STORAGE_FILE_LUKS) { + if (!(secretPath = + virStorageBackendCreateQemuImgSecretPath(conn, pool, vol))) + goto cleanup; + } + cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, pool, vol, inputvol, - flags, create_tool, imgformat); + flags, create_tool, + imgformat, secretPath); if (!cmd) goto cleanup; @@ -1275,6 +1465,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, virCommandFree(cmd); cleanup: + if (secretPath) { + unlink(secretPath); + VIR_FREE(secretPath); + } VIR_FREE(create_tool); return ret; } diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5bc622c..28e1a65 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -241,7 +241,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags, const char *create_tool, - int imgformat); + int imgformat, + const char *secretPath); /* ------- virStorageFile backends ------------ */ typedef struct _virStorageFileBackend virStorageFileBackend; diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 895168e..dd7a59f 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -140,3 +140,26 @@ virQEMUBuildObjectCommandlineFromJSON(const char *type, virBufferFreeAndReset(&buf); return ret; } + + +void +virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) +{ + virBufferAsprintf(buf, "key-secret=%s,", alias); + + /* If there's any cipher, then add that to the command line */ + if (enc->cipher_name) { + virBufferEscapeString(buf, "cipher-alg=%s-", enc->cipher_name); + virBufferAsprintf(buf, "%u,", enc->cipher_size); + if (enc->cipher_mode) + virBufferEscapeString(buf, "cipher-mode=%s,", enc->cipher_mode); + if (enc->cipher_hash) + virBufferEscapeString(buf, "hash-alg=%s,", enc->cipher_hash); + if (enc->ivgen_name) + virBufferEscapeString(buf, "ivgen-alg=%s,", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferEscapeString(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash); + } +} diff --git a/src/util/virqemu.h b/src/util/virqemu.h index af4ec1d..46e7ae2 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -26,9 +26,15 @@ # include "internal.h" # include "virjson.h" +# include "virstorageencryption.h" char *virQEMUBuildObjectCommandlineFromJSON(const char *type, const char *alias, virJSONValuePtr props); +void virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + #endif /* __VIR_QEMU_H_ */ diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index ccfe9ab..e300821 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -83,7 +83,8 @@ testCompareXMLToArgvFiles(bool shouldFail, cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, &poolobj, vol, inputvol, flags, - create_tool, imgformat); + create_tool, imgformat, + NULL); if (!cmd) { if (shouldFail) { virResetLastError(); -- 2.5.5

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021 Generate the luks command line using the AES secret key to encrypt the luks secret. A luks secret object will be in addition to a an AES secret. For hotplug, check if the encinfo exists and if so, add the AES secret for the passphrase for the secret object used to decrypt the device. Add tests for sample output Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- This *replaces* the v3 of the series, the differences are mostly from code review, but with one addition change to remove an unnecessary change: 1. Delete unnecessary comment 2. Use exit_monitor label instead of failaddencsecret 3. Followed secobjProps w/r/t clearing encProps (including error path) 4. Followed similar processing in qemuDomainAttachSCSIDisk 5. Remove the encinfo cleanup in qemuDomainDetachDiskDevice (missed that during my merge/changes). src/qemu/qemu_command.c | 9 +++ src/qemu/qemu_domain.c | 25 +++++++- src/qemu/qemu_hotplug.c | 74 +++++++++++++++++++++- .../qemuxml2argv-luks-disk-cipher.args | 36 +++++++++++ .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 +++++++++++ tests/qemuxml2argvtest.c | 11 +++- 6 files changed, 185 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 71e9e63..038a60c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1098,6 +1098,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus); if (idx < 0) { @@ -1237,6 +1238,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, secinfo->s.aes.alias); } + if (encinfo) + virQEMUBuildLuksOpts(&opt, &disk->src->encryption->encinfo, + encinfo->s.aes.alias); + if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) virBufferAsprintf(&opt, "format=%s,", @@ -1939,6 +1944,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk = def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; /* PowerPC pseries based VMs do not support floppy device */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && @@ -1967,6 +1973,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) return -1; + if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); optstr = qemuBuildDriveStr(disk, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d51e82b..5405365 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -945,7 +945,8 @@ qemuDomainSecretSetup(virConnectPtr conn, { if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && - secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) { + (secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH || + secretUsageType == VIR_SECRET_USAGE_TYPE_PASSPHRASE)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, secretUsageType, username, seclookupdef, isLuks) < 0) @@ -1005,11 +1006,14 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = NULL; - if (conn && qemuDomainSecretDiskCapable(src)) { + if (!conn) + return 0; + + if (qemuDomainSecretDiskCapable(src)) { virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); if (VIR_ALLOC(secinfo) < 0) return -1; @@ -1025,6 +1029,21 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, diskPriv->secinfo = secinfo; } + if (!virStorageSourceIsEmpty(src) && src->encryption && + src->format == VIR_STORAGE_FILE_LUKS) { + + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + VIR_SECRET_USAGE_TYPE_PASSPHRASE, NULL, + &src->encryption->secrets[0]->seclookupdef, + true) < 0) + goto error; + + diskPriv->encinfo = secinfo; + } + return 0; error: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da3e32a..ffbd8d9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -311,8 +311,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; + virJSONValuePtr encProps = NULL; qemuDomainDiskPrivatePtr diskPriv; qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr encinfo; if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -352,6 +354,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; } + encinfo = diskPriv->encinfo; + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0) + goto error; + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; @@ -371,6 +377,11 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, secobjProps) < 0) goto exit_monitor; + if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto failaddencsecret; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -386,6 +397,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, * for successful exit from monitor to clear; otherwise, error * paths wouldn't clean up properly */ secobjProps = NULL; + encProps = NULL; virDomainAuditDisk(vm, NULL, disk->src, "attach", true); @@ -394,6 +406,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, cleanup: virJSONValueFree(secobjProps); + virJSONValueFree(encProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -409,6 +422,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, } failadddrive: + if (encProps) { + if (!orig_err) + orig_err = virSaveLastError(); + ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + } + + failaddencsecret: if (secobjProps) { if (!orig_err) orig_err = virSaveLastError(); @@ -423,7 +443,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; - secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ + /* qemuMonitorAddObject consumes props on failure too */ + secobjProps = NULL; + encProps = NULL; failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -574,10 +596,14 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, virDomainDiskDefPtr disk) { qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err = NULL; char *drivestr = NULL; char *devstr = NULL; int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virJSONValuePtr encProps = NULL; + qemuDomainDiskPrivatePtr diskPriv; + qemuDomainSecretInfoPtr encinfo; if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -596,6 +622,11 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + encinfo = diskPriv->encinfo; + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0) + goto error; + if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) goto error; @@ -605,9 +636,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; - /* Attach the device - 2 step process */ qemuDomainObjEnterMonitor(driver, vm); + if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto exit_monitor; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -617,6 +652,11 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto failexitmonitor; + /* qemuMonitorAddObject consumes the props, but we need to wait + * for successful exit from monitor to clear; otherwise, error + * paths wouldn't clean up properly */ + encProps = NULL; + virDomainAuditDisk(vm, NULL, disk->src, "attach", true); virDomainDiskInsertPreAlloced(vm->def, disk); @@ -634,7 +674,17 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr); failadddrive: + orig_err = virSaveLastError(); + if (encProps) + ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + exit_monitor: ignore_value(qemuDomainObjExitMonitor(driver, vm)); + encProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -2821,6 +2871,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; char *objAlias = NULL; + char *encAlias = NULL; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -2843,6 +2894,20 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } + /* Similarly, if this is possible a device using LUKS encryption, we + * can remove the luks object password too + */ + if (!virStorageSourceIsEmpty(disk->src) && disk->src->encryption && + disk->src->format == VIR_STORAGE_FILE_LUKS) { + + if (!(encAlias = + qemuDomainGetSecretAESAlias(disk->info.alias, true))) { + VIR_FREE(objAlias); + VIR_FREE(drivestr); + return -1; + } + } + qemuDomainObjEnterMonitor(driver, vm); /* If it fails, then so be it - it was a best shot */ @@ -2850,6 +2915,11 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); VIR_FREE(objAlias); + /* If it fails, then so be it - it was a best shot */ + if (encAlias) + ignore_value(qemuMonitorDelObject(priv->mon, encAlias)); + VIR_FREE(encAlias); + qemuMonitorDriveDel(priv->mon, drivestr); VIR_FREE(drivestr); if (qemuDomainObjExitMonitor(driver, vm) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args new file mode 100644 index 0000000..efb5cb0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args @@ -0,0 +1,36 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name encryptdisk \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-encryptdisk/master-key.aes \ +-M pc-i440fx-2.1 \ +-m 1024 \ +-smp 1 \ +-uuid 496898a6-e6ff-f7c8-5dc2-3cf410945ee9 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-encryptdisk/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-object secret,id=virtio-disk0-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk,\ +key-secret=virtio-disk0-luks-secret0,format=luks,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk2,\ +key-secret=virtio-disk1-luks-secret0,format=luks,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args new file mode 100644 index 0000000..efb5cb0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args @@ -0,0 +1,36 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name encryptdisk \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-encryptdisk/master-key.aes \ +-M pc-i440fx-2.1 \ +-m 1024 \ +-smp 1 \ +-uuid 496898a6-e6ff-f7c8-5dc2-3cf410945ee9 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-encryptdisk/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-object secret,id=virtio-disk0-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk,\ +key-secret=virtio-disk0-luks-secret0,format=luks,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk2,\ +key-secret=virtio-disk1-luks-secret0,format=luks,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ca52ab0..aba346d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -62,10 +62,17 @@ fakeSecretLookupByUsage(virConnectPtr conn, return virGetSecret(conn, uuid, usageType, usageID); } +static virSecretPtr +fakeSecretLookupByUUID(virConnectPtr conn, + const unsigned char *uuid) +{ + return virGetSecret(conn, uuid, 0, ""); +} + static virSecretDriver fakeSecretDriver = { .connectNumOfSecrets = NULL, .connectListSecrets = NULL, - .secretLookupByUUID = NULL, + .secretLookupByUUID = fakeSecretLookupByUUID, .secretLookupByUsage = fakeSecretLookupByUsage, .secretDefineXML = NULL, .secretGetXMLDesc = NULL, @@ -1342,6 +1349,8 @@ mymain(void) DO_TEST("encrypted-disk", NONE); DO_TEST("encrypted-disk-usage", NONE); + DO_TEST("luks-disks", QEMU_CAPS_OBJECT_SECRET); + DO_TEST("luks-disk-cipher", QEMU_CAPS_OBJECT_SECRET); DO_TEST("memtune", NONE); DO_TEST("memtune-unlimited", NONE); -- 2.5.5