[libvirt] [PATCH v2 00/15] Add support for LUKS encrypted devices

v1: http://www.redhat.com/archives/libvir-list/2016-June/msg00804.html Differences since v1 (beyond those patches already pushed) Patch 1: Adjust via recent comments for patch 8 of previous series Patch 2: Already ACK'd, adjust slightly based on merge conflicts Patch 3: Used to be patch 14 - Use VIR_SECRET_USAGE_TYPE_PASSPHRASE (instead of _KEY) - Use "usage.id" (instead of "usage.key") Patch 4: Used by be patch 11 (wasn't reviewed) Patch 5: Split from patch 13 for separate endian code to read a 16 bit value Patch 6: NEW - based slightly on former patch 12 - No longer use cryptType - Use versionSize instead in order to decode verision data as 16 or 32 bits Patch 7: Former patch 13 with adjustments based on previous patches Patch 8-9: Former patch 15-16 w/ adjustments from review and to keep up with other changes Patch 10: NEW - Reaction to former patch 17 comments with respect to a file name. Need a way to build a path to temporarily save the secret where that path is not in the pool. Chose the "stateDir", but since storage_driver is the only place that knows, added helper API to access. Patch 11: Former patch 17 with adjustments from code review and to handle other changes so far Patches 12-14: NEW - Really a bug fix submitted as a separate patch (although there are a few differences here), but I need it for patch 15 Patch 15: Former patch 19 plus adjustments for hotplug. John Ferlan (15): qemu: Change protocol parameter for secret setup qemu: Remove authdef from secret setup conf: Add new secret type "passphrase" util: Add 'usage' for encryption util: Introduce virReadBufInt16LE and virReadBufInt16BE util: Modify the FileTypeInfo to add a version size util: Add 'luks' to the FileTypeInfo encryption: Add luks parsing for storageencryption encryption: Add <cipher> and <ivgen> to encryption storage: Introduce virStoragePoolObjBuildTempFilePath storage: Add support to create a luks volume qemu: Remove type from qemuBuildSecretInfoProps qemu: Make qemuBuildSecretInfoProps global qemu: Add secinfo for hotplug virtio disk qemu: Add luks support for domain disk docs/aclpolkit.html.in | 4 + docs/formatsecret.html.in | 62 ++++- docs/formatstorageencryption.html.in | 116 ++++++++- docs/schemas/secret.rng | 10 + docs/schemas/storagecommon.rng | 57 ++++- include/libvirt/libvirt-secret.h | 3 +- src/access/viraccessdriverpolkit.c | 13 + src/conf/domain_conf.c | 11 + src/conf/secret_conf.c | 26 +- src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 5 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 23 +- src/qemu/qemu_command.h | 4 + src/qemu/qemu_domain.c | 126 +++++----- src/qemu/qemu_hotplug.c | 107 ++++++++- src/qemu/qemu_process.c | 19 +- src/storage/storage_backend.c | 266 +++++++++++++++++++-- src/storage/storage_backend.h | 3 +- src/storage/storage_backend_fs.c | 10 +- src/storage/storage_backend_gluster.c | 2 + src/storage/storage_driver.c | 24 ++ src/storage/storage_driver.h | 6 +- src/util/virendian.h | 24 ++ src/util/virqemu.c | 23 ++ src/util/virqemu.h | 6 + src/util/virstorageencryption.c | 152 ++++++++++-- src/util/virstorageencryption.h | 17 +- src/util/virstoragefile.c | 84 +++++-- src/util/virstoragefile.h | 1 + .../qemuxml2argv-encrypted-disk-usage.args | 24 ++ .../qemuxml2argv-encrypted-disk-usage.xml | 32 +++ .../qemuxml2argv-luks-disk-cipher.args | 36 +++ .../qemuxml2argv-luks-disk-cipher.xml | 41 ++++ .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 +++ tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 41 ++++ tests/qemuxml2argvtest.c | 12 +- .../qemuxml2xmlout-encrypted-disk-usage.xml | 36 +++ .../qemuxml2xmlout-luks-disk-cipher.xml | 45 ++++ .../qemuxml2xmlout-luks-disks.xml | 45 ++++ tests/qemuxml2xmltest.c | 3 + tests/secretxml2xmlin/usage-passphrase.xml | 7 + tests/secretxml2xmltest.c | 1 + tests/storagevolxml2argvtest.c | 3 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 23 ++ tests/storagevolxml2xmlin/vol-luks.xml | 21 ++ tests/storagevolxml2xmlout/vol-luks-cipher.xml | 23 ++ tests/storagevolxml2xmlout/vol-luks.xml | 21 ++ tests/storagevolxml2xmltest.c | 2 + tests/virendiantest.c | 18 ++ 50 files changed, 1495 insertions(+), 181 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml create mode 100644 tests/secretxml2xmlin/usage-passphrase.xml create mode 100644 tests/storagevolxml2xmlin/vol-luks-cipher.xml create mode 100644 tests/storagevolxml2xmlin/vol-luks.xml create mode 100644 tests/storagevolxml2xmlout/vol-luks-cipher.xml create mode 100644 tests/storagevolxml2xmlout/vol-luks.xml -- 2.5.5

Rather than assume/pass the protocol to the qemuDomainSecretPlainSetup and qemuDomainSecretAESSetup, set and pass the secretUsageType based on the src->protocol type. This will eventually be used by the virSecretGetSecretString call Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 62 ++++++++++++++++---------------------------------- 1 file changed, 20 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4a5378f..ca49db1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -814,7 +814,7 @@ qemuDomainHostdevPrivateDispose(void *obj) /* qemuDomainSecretPlainSetup: * @conn: Pointer to connection * @secinfo: Pointer to secret info - * @protocol: Protocol for secret + * @secretUsageType: The virSecretUsageType * @authdef: Pointer to auth data * * Taking a secinfo, fill in the plaintext information @@ -824,19 +824,15 @@ qemuDomainHostdevPrivateDispose(void *obj) static int qemuDomainSecretPlainSetup(virConnectPtr conn, qemuDomainSecretInfoPtr secinfo, - virStorageNetProtocol protocol, + virSecretUsageType secretUsageType, virStorageAuthDefPtr authdef) { - int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; - secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN; if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0) return -1; - if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) - secretType = VIR_SECRET_USAGE_TYPE_CEPH; - - return virSecretGetSecretString(conn, &authdef->seclookupdef, secretType, + return virSecretGetSecretString(conn, &authdef->seclookupdef, + secretUsageType, &secinfo->s.plain.secret, &secinfo->s.plain.secretlen); } @@ -847,7 +843,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, * @priv: pointer to domain private object * @secinfo: Pointer to secret info * @srcalias: Alias of the disk/hostdev used to generate the secret alias - * @protocol: Protocol for secret + * @secretUsageType: The virSecretUsageType * @authdef: Pointer to auth data * * Taking a secinfo, fill in the AES specific information using the @@ -859,7 +855,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, qemuDomainObjPrivatePtr priv, qemuDomainSecretInfoPtr secinfo, const char *srcalias, - virStorageNetProtocol protocol, + virSecretUsageType secretUsageType, virStorageAuthDefPtr authdef) { int ret = -1; @@ -869,34 +865,11 @@ qemuDomainSecretAESSetup(virConnectPtr conn, size_t secretlen = 0; uint8_t *ciphertext = NULL; size_t ciphertextlen = 0; - int secretType = VIR_SECRET_USAGE_TYPE_NONE; secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES; if (VIR_STRDUP(secinfo->s.aes.username, authdef->username) < 0) return -1; - switch ((virStorageNetProtocol)protocol) { - case VIR_STORAGE_NET_PROTOCOL_RBD: - secretType = VIR_SECRET_USAGE_TYPE_CEPH; - break; - - case VIR_STORAGE_NET_PROTOCOL_NONE: - case VIR_STORAGE_NET_PROTOCOL_NBD: - case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: - case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - 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_TFTP: - case VIR_STORAGE_NET_PROTOCOL_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("protocol '%s' cannot be used for encrypted secrets"), - virStorageNetProtocolTypeToString(protocol)); - return -1; - } - if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias))) return -1; @@ -909,7 +882,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, goto cleanup; /* Grab the unencoded secret */ - if (virSecretGetSecretString(conn, &authdef->seclookupdef, secretType, + if (virSecretGetSecretString(conn, &authdef->seclookupdef, secretUsageType, &secret, &secretlen) < 0) goto cleanup; @@ -943,7 +916,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, * @priv: pointer to domain private object * @secinfo: Pointer to secret info * @srcalias: Alias of the disk/hostdev used to generate the secret alias - * @protocol: Protocol for secret + * @secretUsageType: The virSecretUsageType * @authdef: Pointer to auth data * * If we have the encryption API present and can support a secret object, then @@ -958,17 +931,18 @@ qemuDomainSecretSetup(virConnectPtr conn, qemuDomainObjPrivatePtr priv, qemuDomainSecretInfoPtr secinfo, const char *srcalias, - virStorageNetProtocol protocol, + virSecretUsageType secretUsageType, virStorageAuthDefPtr authdef) { if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && - protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { - if (qemuDomainSecretAESSetup(conn, priv, secinfo, - srcalias, protocol, authdef) < 0) + secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) { + if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, + secretUsageType, authdef) < 0) return -1; } else { - if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0) + if (qemuDomainSecretPlainSetup(conn, secinfo, secretUsageType, + authdef) < 0) return -1; } return 0; @@ -1015,13 +989,17 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI || src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { + virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); if (VIR_ALLOC(secinfo) < 0) return -1; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) + secretUsageType = VIR_SECRET_USAGE_TYPE_CEPH; + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, - src->protocol, src->auth) < 0) + secretUsageType, src->auth) < 0) goto error; diskPriv->secinfo = secinfo; @@ -1086,7 +1064,7 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, return -1; if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias, - VIR_STORAGE_NET_PROTOCOL_ISCSI, + VIR_SECRET_USAGE_TYPE_ISCSI, iscsisrc->auth) < 0) goto error; -- 2.5.5

On Thu, Jun 23, 2016 at 13:28:57 -0400, John Ferlan wrote:
Rather than assume/pass the protocol to the qemuDomainSecretPlainSetup and qemuDomainSecretAESSetup, set and pass the secretUsageType based on the src->protocol type. This will eventually be used by the virSecretGetSecretString call
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 62 ++++++++++++++++---------------------------------- 1 file changed, 20 insertions(+), 42 deletions(-)
ACK

Rather than pass authdef, pass the 'authdef->username' and the '&authdef->secdef' Note that a username may be NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ca49db1..dca8970 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -815,7 +815,8 @@ qemuDomainHostdevPrivateDispose(void *obj) * @conn: Pointer to connection * @secinfo: Pointer to secret info * @secretUsageType: The virSecretUsageType - * @authdef: Pointer to auth data + * @username: username to use for authentication (may be NULL) + * @seclookupdef: Pointer to seclookupdef data * * Taking a secinfo, fill in the plaintext information * @@ -825,14 +826,14 @@ static int qemuDomainSecretPlainSetup(virConnectPtr conn, qemuDomainSecretInfoPtr secinfo, virSecretUsageType secretUsageType, - virStorageAuthDefPtr authdef) + const char *username, + virSecretLookupTypeDefPtr seclookupdef) { secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN; - if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0) + if (VIR_STRDUP(secinfo->s.plain.username, username) < 0) return -1; - return virSecretGetSecretString(conn, &authdef->seclookupdef, - secretUsageType, + return virSecretGetSecretString(conn, seclookupdef, secretUsageType, &secinfo->s.plain.secret, &secinfo->s.plain.secretlen); } @@ -844,7 +845,8 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, * @secinfo: Pointer to secret info * @srcalias: Alias of the disk/hostdev used to generate the secret alias * @secretUsageType: The virSecretUsageType - * @authdef: Pointer to auth data + * @username: username to use for authentication (may be NULL) + * @seclookupdef: Pointer to seclookupdef data * * Taking a secinfo, fill in the AES specific information using the * @@ -856,7 +858,8 @@ qemuDomainSecretAESSetup(virConnectPtr conn, qemuDomainSecretInfoPtr secinfo, const char *srcalias, virSecretUsageType secretUsageType, - virStorageAuthDefPtr authdef) + const char *username, + virSecretLookupTypeDefPtr seclookupdef) { int ret = -1; uint8_t *raw_iv = NULL; @@ -867,7 +870,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, size_t ciphertextlen = 0; secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES; - if (VIR_STRDUP(secinfo->s.aes.username, authdef->username) < 0) + if (VIR_STRDUP(secinfo->s.aes.username, username) < 0) return -1; if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias))) @@ -882,7 +885,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, goto cleanup; /* Grab the unencoded secret */ - if (virSecretGetSecretString(conn, &authdef->seclookupdef, secretUsageType, + if (virSecretGetSecretString(conn, seclookupdef, secretUsageType, &secret, &secretlen) < 0) goto cleanup; @@ -917,7 +920,8 @@ qemuDomainSecretAESSetup(virConnectPtr conn, * @secinfo: Pointer to secret info * @srcalias: Alias of the disk/hostdev used to generate the secret alias * @secretUsageType: The virSecretUsageType - * @authdef: Pointer to auth data + * @username: username to use for authentication (may be NULL) + * @seclookupdef: Pointer to seclookupdef data * * If we have the encryption API present and can support a secret object, then * build the AES secret; otherwise, build the Plain secret. This is the magic @@ -932,17 +936,19 @@ qemuDomainSecretSetup(virConnectPtr conn, qemuDomainSecretInfoPtr secinfo, const char *srcalias, virSecretUsageType secretUsageType, - virStorageAuthDefPtr authdef) + const char *username, + virSecretLookupTypeDefPtr seclookupdef) { if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, - secretUsageType, authdef) < 0) + secretUsageType, username, + seclookupdef) < 0) return -1; } else { if (qemuDomainSecretPlainSetup(conn, secinfo, secretUsageType, - authdef) < 0) + username, seclookupdef) < 0) return -1; } return 0; @@ -999,7 +1005,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, secretUsageType = VIR_SECRET_USAGE_TYPE_CEPH; if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, - secretUsageType, src->auth) < 0) + secretUsageType, src->auth->username, + &src->auth->seclookupdef) < 0) goto error; diskPriv->secinfo = secinfo; @@ -1065,7 +1072,8 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias, VIR_SECRET_USAGE_TYPE_ISCSI, - iscsisrc->auth) < 0) + iscsisrc->auth->username, + &iscsisrc->auth->seclookupdef) < 0) goto error; hostdevPriv->secinfo = secinfo; -- 2.5.5

On Thu, Jun 23, 2016 at 13:28:58 -0400, John Ferlan wrote:
Rather than pass authdef, pass the 'authdef->username' and the '&authdef->secdef'
Note that a username may be NULL.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
ACK

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'> <id>mumblyfratz</id> </usage> </secret> Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 26 +++++++++++++- src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 5 +++ tests/secretxml2xmlin/usage-passphrase.xml | 7 ++++ tests/secretxml2xmltest.c | 1 + 10 files changed, 123 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..1d31b6d 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_id</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 599cb38..79c4082 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 @@ <secret usage='libvirtiscsi'/> </auth> </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 + <secret ephemeral='no' private='yes'> + <description>sample passphrase secret</description> + <usage type='passphrase'> + <id>id_example</id> + </usage> + </secret> + + # virsh secret-define secret.xml + Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created + + # virsh secret-list + UUID Usage + ----------------------------------------------------------- + 718c71bd-67b5-4a2b-87ec-a24e8ca200dc passphrase id_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..fc188ba 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='id'> + <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@redhat.com> * - * 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..1f955f0 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_id", secret->usage.id, + NULL, + }; + + return virAccessDriverPolkitCheck(manager, + "secret", + virAccessPermSecretTypeToString(perm), + attrs); + } break; } } diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index de9e6cf..77477b6 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.id; + 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.id); + break; + default: VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); break; @@ -92,6 +100,7 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def); } + static int virSecretDefParseUsage(xmlXPathContextPtr ctxt, virSecretDefPtr def) @@ -145,6 +154,14 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (!(def->usage.id = virXPathString("string(./usage/id)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("passphrase usage specified, but id is missing")); + return -1; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -305,6 +322,13 @@ virSecretDefFormatUsage(virBufferPtr buf, } break; + case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (def->usage.id != NULL) { + virBufferEscapeString(buf, "<id>%s</id>\n", + def->usage.id); + } + 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..768e6e9 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 *id; } usage; }; diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index c46d22c..a2b4801 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.id, 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..7ebe0a4 --- /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'> + <id>mumblyfratz</id> + </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

On Thu, Jun 23, 2016 at 13:28:59 -0400, John Ferlan wrote:
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'> <id>mumblyfratz</id> </usage> </secret>
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 26 +++++++++++++- src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 5 +++ tests/secretxml2xmlin/usage-passphrase.xml | 7 ++++ tests/secretxml2xmltest.c | 1 + 10 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-passphrase.xml
diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 599cb38..79c4082 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in
[..]
@@ -241,5 +242,57 @@ <secret usage='libvirtiscsi'/> </auth> </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 + <secret ephemeral='no' private='yes'> + <description>sample passphrase secret</description> + <usage type='passphrase'> + <id>id_example</id>
'id' implies a number. Any reason for not using 'name'?
+ </usage> + </secret> + + # virsh secret-define secret.xml + Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created + + # virsh secret-list + UUID Usage + ----------------------------------------------------------- + 718c71bd-67b5-4a2b-87ec-a24e8ca200dc passphrase id_example
Header is misaligned.
+ # + + </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/src/conf/secret_conf.c b/src/conf/secret_conf.c index de9e6cf..77477b6 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c
@@ -92,6 +100,7 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def); }
+
Spurious whitespace change.
static int virSecretDefParseUsage(xmlXPathContextPtr ctxt, virSecretDefPtr def) @@ -145,6 +154,14 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break;
+ case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (!(def->usage.id = virXPathString("string(./usage/id)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("passphrase usage specified, but id is missing")); + return -1;
This diallows missing ID.
+ } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -305,6 +322,13 @@ virSecretDefFormatUsage(virBufferPtr buf, } break;
+ case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (def->usage.id != NULL) {
This allows missing id.
+ virBufferEscapeString(buf, "<id>%s</id>\n", + def->usage.id); + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"),

On 06/24/2016 06:28 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:28:59 -0400, John Ferlan wrote:
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'> <id>mumblyfratz</id> </usage> </secret>
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 26 +++++++++++++- src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 5 +++ tests/secretxml2xmlin/usage-passphrase.xml | 7 ++++ tests/secretxml2xmltest.c | 1 + 10 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-passphrase.xml
diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 599cb38..79c4082 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in
[..]
@@ -241,5 +242,57 @@ <secret usage='libvirtiscsi'/> </auth> </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 + <secret ephemeral='no' private='yes'> + <description>sample passphrase secret</description> + <usage type='passphrase'> + <id>id_example</id>
'id' implies a number. Any reason for not using 'name'?
Name is fine with me. Id is just shorter to type.
+ </usage> + </secret> + + # virsh secret-define secret.xml + Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created + + # virsh secret-list + UUID Usage + ----------------------------------------------------------- + 718c71bd-67b5-4a2b-87ec-a24e8ca200dc passphrase id_example
Header is misaligned.
Ironically the others are off too... This was some cut-n-paste, followed by deletion of a line in order to grab the output of the actual command that I ran... I'll fix it (and others too) - I assume those are trivial...
+ # + + </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/src/conf/secret_conf.c b/src/conf/secret_conf.c index de9e6cf..77477b6 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c
@@ -92,6 +100,7 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def); }
+
Spurious whitespace change.
static int virSecretDefParseUsage(xmlXPathContextPtr ctxt, virSecretDefPtr def) @@ -145,6 +154,14 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break;
+ case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (!(def->usage.id = virXPathString("string(./usage/id)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("passphrase usage specified, but id is missing")); + return -1;
This diallows missing ID.
+ } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -305,6 +322,13 @@ virSecretDefFormatUsage(virBufferPtr buf, } break;
+ case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + if (def->usage.id != NULL) {
This allows missing id.
True - but it follows other elements of the case. I could trivially change those as well. John
+ virBufferEscapeString(buf, "<id>%s</id>\n", + def->usage.id); + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"),

In order to use more common code and set up for a future type, modify the encryption secret to allow the "usage" attribute or the "uuid" attribute to define the secret. The "usage" in the case of a volume secret would be the path to the volume. This code will make use of the virSecretLookup{Parse|Format}Secret common code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorageencryption.html.in | 15 ++++++--- docs/schemas/storagecommon.rng | 11 +++++-- src/qemu/qemu_process.c | 13 +++----- src/storage/storage_backend.c | 3 +- src/storage/storage_backend_fs.c | 3 +- src/util/virstorageencryption.c | 26 ++++++---------- src/util/virstorageencryption.h | 3 +- .../qemuxml2argv-encrypted-disk-usage.args | 24 +++++++++++++++ .../qemuxml2argv-encrypted-disk-usage.xml | 32 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-encrypted-disk-usage.xml | 36 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 132 insertions(+), 36 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in index 04c3346..fae86eb 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -25,10 +25,17 @@ <p> The <code>encryption</code> tag can currently contain a sequence of <code>secret</code> tags, each with mandatory attributes <code>type</code> - and <code>uuid</code>. The only currently defined value of - <code>type</code> is <code>passphrase</code>. <code>uuid</code> - refers to a secret known to libvirt. libvirt can use a secret value - previously set using <code>virSecretSetValue()</code>, or, if supported + and either <code>uuid</code> or + <code>usage</code> (<span class="since">since 2.0.0</span>). + The only currently defined value of + <code>type</code> is <code>passphrase</code>. The <code>uuid</code> + refers to a secret known to libvirt by it's "uuid" value (from the + output of a <code>virsh secret-list</code>. The <code>usage</code> + is the path to the volume as it appears in the volume + <code>source</code> element. A secret value can be set in libvirt by + using either <code>virsh secret-set-value</code> or the + <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> + <code>virSecretSetValue</code></a> API. Alternatively, if supported by the particular volume format and driver, automatically generate a secret value at the time of volume creation, and store it using the specified <code>uuid</code>. diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 7c04462..c5b71de 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -27,9 +27,14 @@ <value>passphrase</value> </choice> </attribute> - <attribute name='uuid'> - <ref name="UUID"/> - </attribute> + <choice> + <attribute name='uuid'> + <ref name="UUID"/> + </attribute> + <attribute name='usage'> + <text/> + </attribute> + </choice> </element> </define> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 63da600..7d56ec8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -70,6 +70,7 @@ #include "virnuma.h" #include "virstring.h" #include "virhostdev.h" +#include "secret_util.h" #include "storage/storage_driver.h" #include "configmake.h" #include "nwfilter_conf.h" @@ -377,7 +378,6 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, char **secretRet, size_t *secretLen) { - virSecretPtr secret; char *passphrase; unsigned char *data; size_t size; @@ -416,14 +416,9 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, goto cleanup; } - secret = conn->secretDriver->secretLookupByUUID(conn, - enc->secrets[0]->uuid); - if (secret == NULL) - goto cleanup; - data = conn->secretDriver->secretGetValue(secret, &size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - virObjectUnref(secret); - if (data == NULL) + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, + VIR_SECRET_USAGE_TYPE_VOLUME, + &data, &size) < 0) goto cleanup; if (memchr(data, '\0', size) != NULL) { diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d041530..fd76e21 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -648,7 +648,8 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, goto cleanup; enc_secret->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE; - memcpy(enc_secret->uuid, secret->uuid, VIR_UUID_BUFLEN); + enc_secret->seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + memcpy(enc_secret->seclookupdef.u.uuid, secret->uuid, VIR_UUID_BUFLEN); enc->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; enc->secrets[0] = enc_secret; /* Space for secrets[0] allocated above */ enc_secret = NULL; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index a11df36..cac6bb8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1318,7 +1318,8 @@ virStorageBackendFileSystemLoadDefaultSecrets(virConnectPtr conn, vol->target.encryption->secrets[0] = encsec; encsec->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE; - virSecretGetUUID(sec, encsec->uuid); + encsec->seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + virSecretGetUUID(sec, encsec->seclookupdef.u.uuid); virObjectUnref(sec); return 0; diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index 8105158..afb44da 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -34,6 +34,7 @@ #include "virerror.h" #include "viruuid.h" #include "virfile.h" +#include "virsecret.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -114,6 +115,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, virStorageEncryptionSecretPtr ret; char *type_str = NULL; char *uuidstr = NULL; + char *usagestr = NULL; if (VIR_ALLOC(ret) < 0) return NULL; @@ -133,21 +135,12 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, type_str); goto cleanup; } - VIR_FREE(type_str); - if ((uuidstr = virXPathString("string(./@uuid)", ctxt))) { - if (virUUIDParse(uuidstr, ret->uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("malformed volume encryption uuid '%s'"), - uuidstr); - goto cleanup; - } - VIR_FREE(uuidstr); - } else { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing volume encryption uuid")); + if (virSecretLookupParseSecret(node, &ret->seclookupdef) < 0) goto cleanup; - } + + VIR_FREE(type_str); + ctxt->node = old_node; return ret; @@ -155,6 +148,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, VIR_FREE(type_str); virStorageEncryptionSecretFree(ret); VIR_FREE(uuidstr); + VIR_FREE(usagestr); ctxt->node = old_node; return NULL; } @@ -244,7 +238,6 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, virStorageEncryptionSecretPtr secret) { const char *type; - char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!(type = virStorageEncryptionSecretTypeToString(secret->type))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -252,9 +245,8 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, return -1; } - virUUIDFormat(secret->uuid, uuidstr); - virBufferAsprintf(buf, "<secret type='%s' uuid='%s'/>\n", - type, uuidstr); + virSecretLookupFormatSecret(buf, type, &secret->seclookupdef); + return 0; } diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index 04641b1..c68c66e 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -25,6 +25,7 @@ # include "internal.h" # include "virbuffer.h" +# include "virsecret.h" # include "virutil.h" # include <libxml/tree.h> @@ -40,7 +41,7 @@ typedef struct _virStorageEncryptionSecret virStorageEncryptionSecret; typedef virStorageEncryptionSecret *virStorageEncryptionSecretPtr; struct _virStorageEncryptionSecret { int type; /* virStorageEncryptionSecretType */ - unsigned char uuid[VIR_UUID_BUFLEN]; + virSecretLookupTypeDef seclookupdef; }; typedef enum { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args b/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args new file mode 100644 index 0000000..4371413 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name encryptdisk \ +-S \ +-M pc \ +-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 \ +-drive file=/storage/guest_disks/encryptdisk,format=qcow2,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml b/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml new file mode 100644 index 0000000..6c5d87d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml @@ -0,0 +1,32 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/storage/guest_disks/encryptdisk'/> + <target dev='vda' bus='virtio'/> + <encryption format='qcow'> + <secret type='passphrase' usage='/storage/guest_disks/encryptdisk'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a4b8bf4..ca52ab0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1341,6 +1341,7 @@ mymain(void) driver.caps->host.cpu = cpuDefault; DO_TEST("encrypted-disk", NONE); + DO_TEST("encrypted-disk-usage", NONE); DO_TEST("memtune", NONE); DO_TEST("memtune-unlimited", NONE); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml new file mode 100644 index 0000000..ec6413f --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml @@ -0,0 +1,36 @@ +<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='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/storage/guest_disks/encryptdisk'/> + <target dev='vda' bus='virtio'/> + <encryption format='qcow'> + <secret type='passphrase' usage='/storage/guest_disks/encryptdisk'/> + </encryption> + <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/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7db9cb7..d045fd4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -501,6 +501,7 @@ mymain(void) DO_TEST("pci-serial-dev-chardev"); DO_TEST("encrypted-disk"); + DO_TEST("encrypted-disk-usage"); DO_TEST("memtune"); DO_TEST("memtune-unlimited"); DO_TEST("blkiotune"); -- 2.5.5

On Thu, Jun 23, 2016 at 13:29:00 -0400, John Ferlan wrote:
In order to use more common code and set up for a future type, modify the encryption secret to allow the "usage" attribute or the "uuid" attribute to define the secret. The "usage" in the case of a volume secret would be the path to the volume.
This code will make use of the virSecretLookup{Parse|Format}Secret common code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorageencryption.html.in | 15 ++++++--- docs/schemas/storagecommon.rng | 11 +++++-- src/qemu/qemu_process.c | 13 +++----- src/storage/storage_backend.c | 3 +- src/storage/storage_backend_fs.c | 3 +- src/util/virstorageencryption.c | 26 ++++++---------- src/util/virstorageencryption.h | 3 +- .../qemuxml2argv-encrypted-disk-usage.args | 24 +++++++++++++++ .../qemuxml2argv-encrypted-disk-usage.xml | 32 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-encrypted-disk-usage.xml | 36 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 132 insertions(+), 36 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml
diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in index 04c3346..fae86eb 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -25,10 +25,17 @@ <p> The <code>encryption</code> tag can currently contain a sequence of <code>secret</code> tags, each with mandatory attributes <code>type</code> - and <code>uuid</code>. The only currently defined value of - <code>type</code> is <code>passphrase</code>. <code>uuid</code> - refers to a secret known to libvirt. libvirt can use a secret value - previously set using <code>virSecretSetValue()</code>, or, if supported + and either <code>uuid</code> or + <code>usage</code> (<span class="since">since 2.0.0</span>). + The only currently defined value of + <code>type</code> is <code>passphrase</code>. The <code>uuid</code> + refers to a secret known to libvirt by it's "uuid" value (from the + output of a <code>virsh secret-list</code>. The <code>usage</code>
I don't think it's necessary to describe how to use virsh here.
+ is the path to the volume as it appears in the volume
This looks wrong. Passprhase type secrets list 'name' or 'id' as usage not the path. This contradicts changes in previous patch.
+ <code>source</code> element. A secret value can be set in libvirt by + using either <code>virsh secret-set-value</code> or the
Again. Mentioning the API is good enoguh.
+ <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> + <code>virSecretSetValue</code></a> API. Alternatively, if supported by the particular volume format and driver, automatically generate a secret value at the time of volume creation, and store it using the specified <code>uuid</code>.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 63da600..7d56ec8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c
[...]
@@ -416,14 +416,9 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, goto cleanup; }
- secret = conn->secretDriver->secretLookupByUUID(conn, - enc->secrets[0]->uuid); - if (secret == NULL) - goto cleanup; - data = conn->secretDriver->secretGetValue(secret, &size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - virObjectUnref(secret); - if (data == NULL) + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, + VIR_SECRET_USAGE_TYPE_VOLUME,
Wrong type.
+ &data, &size) < 0) goto cleanup;
if (memchr(data, '\0', size) != NULL) {

On 06/24/2016 09:25 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:00 -0400, John Ferlan wrote:
In order to use more common code and set up for a future type, modify the encryption secret to allow the "usage" attribute or the "uuid" attribute to define the secret. The "usage" in the case of a volume secret would be the path to the volume.
This code will make use of the virSecretLookup{Parse|Format}Secret common code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorageencryption.html.in | 15 ++++++--- docs/schemas/storagecommon.rng | 11 +++++-- src/qemu/qemu_process.c | 13 +++----- src/storage/storage_backend.c | 3 +- src/storage/storage_backend_fs.c | 3 +- src/util/virstorageencryption.c | 26 ++++++---------- src/util/virstorageencryption.h | 3 +- .../qemuxml2argv-encrypted-disk-usage.args | 24 +++++++++++++++ .../qemuxml2argv-encrypted-disk-usage.xml | 32 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-encrypted-disk-usage.xml | 36 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 132 insertions(+), 36 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk-usage.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-encrypted-disk-usage.xml
diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in index 04c3346..fae86eb 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -25,10 +25,17 @@ <p> The <code>encryption</code> tag can currently contain a sequence of <code>secret</code> tags, each with mandatory attributes <code>type</code> - and <code>uuid</code>. The only currently defined value of - <code>type</code> is <code>passphrase</code>. <code>uuid</code> - refers to a secret known to libvirt. libvirt can use a secret value - previously set using <code>virSecretSetValue()</code>, or, if supported + and either <code>uuid</code> or + <code>usage</code> (<span class="since">since 2.0.0</span>). + The only currently defined value of + <code>type</code> is <code>passphrase</code>. The <code>uuid</code> + refers to a secret known to libvirt by it's "uuid" value (from the + output of a <code>virsh secret-list</code>. The <code>usage</code>
I don't think it's necessary to describe how to use virsh here.
Ok - removed.
+ is the path to the volume as it appears in the volume
This looks wrong. Passprhase type secrets list 'name' or 'id' as usage not the path. This contradicts changes in previous patch.
Looks weird, but that's how the matching is done... The contents of the "usage" field are matched against the secret's <usage>'s subelement field.
+ <code>source</code> element. A secret value can be set in libvirt by + using either <code>virsh secret-set-value</code> or the
Again. Mentioning the API is good enoguh.
+ <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> + <code>virSecretSetValue</code></a> API. Alternatively, if supported by the particular volume format and driver, automatically generate a secret value at the time of volume creation, and store it using the specified <code>uuid</code>.
Does the following work? The <code>encryption</code> tag can currently contain a sequence of <code>secret</code> tags, each with mandatory attributes <code>type</code> and either <code>uuid</code> or <code>usage</code> (<span class="since">since 2.0.0</span>). The only currently defined value of <code>type</code> is <code>passphrase</code>. The <code>uuid</code> is "uuid" of the <code>secret</code> while <code>usage</code> is the value "usage" subelement field. A secret value can be set in libvirt by the <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> <code>virSecretSetValue</code></a> API. Alternatively, if supported
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 63da600..7d56ec8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c
[...]
@@ -416,14 +416,9 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, goto cleanup; }
- secret = conn->secretDriver->secretLookupByUUID(conn, - enc->secrets[0]->uuid); - if (secret == NULL) - goto cleanup; - data = conn->secretDriver->secretGetValue(secret, &size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - virObjectUnref(secret); - if (data == NULL) + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, + VIR_SECRET_USAGE_TYPE_VOLUME,
Wrong type.
Huh? This is the "old" VOLUME method not the new PASSPHRASE method. So VOLUME here is correct. John
+ &data, &size) < 0) goto cleanup;
if (memchr(data, '\0', size) != NULL) {

In order to read 16 bits of data in the native format and convert add the 16 bit macros to match existing 32 and 64 bit code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virendian.h | 24 ++++++++++++++++++++++++ tests/virendiantest.c | 18 ++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/util/virendian.h b/src/util/virendian.h index eefe48c..97940bd 100644 --- a/src/util/virendian.h +++ b/src/util/virendian.h @@ -90,4 +90,28 @@ ((uint32_t)(uint8_t)((buf)[2]) << 16) | \ ((uint32_t)(uint8_t)((buf)[3]) << 24)) +/** + * virReadBufInt16BE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 2 bytes at BUF as a big-endian 16-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt16BE(buf) \ + (((uint16_t)(uint8_t)((buf)[0]) << 8) | \ + (uint16_t)(uint8_t)((buf)[1])) + +/** + * virReadBufInt16LE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 2 bytes at BUF as a little-endian 16-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt16LE(buf) \ + ((uint16_t)(uint8_t)((buf)[0]) | \ + ((uint16_t)(uint8_t)((buf)[1]) << 8)) + #endif /* __VIR_ENDIAN_H__ */ diff --git a/tests/virendiantest.c b/tests/virendiantest.c index 4072507..f858e5c 100644 --- a/tests/virendiantest.c +++ b/tests/virendiantest.c @@ -50,6 +50,15 @@ test1(const void *data ATTRIBUTE_UNUSED) if (virReadBufInt32LE(array + 9) != 0x8d8c8b8aU) goto cleanup; + if (virReadBufInt16BE(array) != 0x0102U) + goto cleanup; + if (virReadBufInt16BE(array + 11) != 0x8c8dU) + goto cleanup; + if (virReadBufInt16LE(array) != 0x0201U) + goto cleanup; + if (virReadBufInt16LE(array + 11) != 0x8d8cU) + goto cleanup; + ret = 0; cleanup: return ret; @@ -81,6 +90,15 @@ test2(const void *data ATTRIBUTE_UNUSED) if (virReadBufInt32LE(array + 9) != 0x8d8c8b8aU) goto cleanup; + if (virReadBufInt16BE(array) != 0x0102U) + goto cleanup; + if (virReadBufInt16BE(array + 11) != 0x8c8dU) + goto cleanup; + if (virReadBufInt16LE(array) != 0x0201U) + goto cleanup; + if (virReadBufInt16LE(array + 11) != 0x8d8cU) + goto cleanup; + ret = 0; cleanup: return ret; -- 2.5.5

On Thu, Jun 23, 2016 at 13:29:01 -0400, John Ferlan wrote:
In order to read 16 bits of data in the native format and convert add the 16 bit macros to match existing 32 and 64 bit code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virendian.h | 24 ++++++++++++++++++++++++ tests/virendiantest.c | 18 ++++++++++++++++++ 2 files changed, 42 insertions(+)
ACK

The version field historically has been a 4 byte data; however, an upcoming new type will use a 2 byte version. So let's adjust for that now. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 60 +++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index de4955b..37e9798 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1,7 +1,7 @@ /* * virstoragefile.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2014, 2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -120,10 +120,12 @@ struct FileTypeInfo { * to check at head of file */ const char *extension; /* Optional file extension to check */ enum lv_endian endian; /* Endianness of file format */ + int versionOffset; /* Byte offset from start of file * where we find version number, * -1 to always fail the version test, * -2 to always pass the version test */ + int versionSize; /* Size in bytes of version data (0, 2, or 4) */ int versionNumbers[FILE_TYPE_VERSIONS_LAST]; /* Version numbers to validate. Zeroes are ignored. */ int sizeOffset; /* Byte offset from start of file @@ -189,15 +191,15 @@ qedGetBackingStore(char **, int *, const char *, size_t); static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_RAW] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_DIR] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ 0, NULL, NULL, - LV_LITTLE_ENDIAN, 64, {0x20000}, + LV_LITTLE_ENDIAN, 64, 4, {0x20000}, 32+16+16+4+4+4+4+4, 8, 1, -1, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { @@ -206,7 +208,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { modprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1 */ /* Untested */ 0, NULL, NULL, - LV_LITTLE_ENDIAN, -1, {0}, + LV_LITTLE_ENDIAN, -1, 0, {0}, -1, 0, 0, -1, NULL, NULL }, [VIR_STORAGE_FILE_DMG] = { @@ -214,60 +216,60 @@ static struct FileTypeInfo const fileTypeInfo[] = { * /usr/share/misc/magic lists double magic (both offsets * would have to match) but then disables that check. */ 0, NULL, ".dmg", - 0, -1, {0}, + 0, -1, 0, {0}, -1, 0, 0, -1, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", ".iso", - LV_LITTLE_ENDIAN, -2, {0}, + LV_LITTLE_ENDIAN, -2, 0, {0}, -1, 0, 0, -1, NULL, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", NULL, - LV_BIG_ENDIAN, 12, {0x10000}, + LV_BIG_ENDIAN, 12, 4, {0x10000}, 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", ".vdi", - LV_LITTLE_ENDIAN, 68, {0x00010001}, + LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL, NULL}, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", NULL, LV_LITTLE_ENDIAN, - -2, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0, + -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0, PLOOP_SIZE_MULTIPLIER, -1, NULL, NULL }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", NULL, - LV_BIG_ENDIAN, 4, {2}, + LV_BIG_ENDIAN, 4, 4, {2}, 4+4+1024+4, 8, 1, -1, cowGetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", NULL, - LV_BIG_ENDIAN, 4, {1}, + LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW1_HDR_CRYPT, qcow1GetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", NULL, - LV_BIG_ENDIAN, 4, {2, 3}, + LV_BIG_ENDIAN, 4, 4, {2, 3}, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, qcow2GetFeatures }, [VIR_STORAGE_FILE_QED] = { /* http://wiki.qemu.org/Features/QED */ 0, "QED", NULL, - LV_LITTLE_ENDIAN, -2, {0}, + LV_LITTLE_ENDIAN, -2, 0, {0}, QED_HDR_IMAGE_SIZE, 8, 1, -1, qedGetBackingStore, NULL }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", NULL, - LV_LITTLE_ENDIAN, 4, {1, 2}, + LV_LITTLE_ENDIAN, 4, 4, {1, 2}, 4+4+4, 8, 512, -1, vmdk4GetBackingStore, NULL }, }; @@ -635,13 +637,25 @@ virStorageFileMatchesVersion(int format, if (fileTypeInfo[format].versionOffset == -2) return true; - if ((fileTypeInfo[format].versionOffset + 4) > buflen) + if ((fileTypeInfo[format].versionOffset + + fileTypeInfo[format].versionSize) > buflen) return false; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) - version = virReadBufInt32LE(buf + fileTypeInfo[format].versionOffset); - else - version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) { + if (fileTypeInfo[format].versionSize == 4) + version = virReadBufInt32LE(buf + + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt16LE(buf + + fileTypeInfo[format].versionOffset); + } else { + if (fileTypeInfo[format].versionSize == 4) + version = virReadBufInt32BE(buf + + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt16BE(buf + + fileTypeInfo[format].versionOffset); + } for (i = 0; i < FILE_TYPE_VERSIONS_LAST && fileTypeInfo[format].versionNumbers[i]; -- 2.5.5

On Thu, Jun 23, 2016 at 13:29:02 -0400, John Ferlan wrote:
The version field historically has been a 4 byte data; however, an upcoming new type will use a 2 byte version. So let's adjust for that now.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 60 +++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index de4955b..37e9798 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1,7 +1,7 @@ /* * virstoragefile.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2014, 2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -120,10 +120,12 @@ struct FileTypeInfo { * to check at head of file */ const char *extension; /* Optional file extension to check */ enum lv_endian endian; /* Endianness of file format */ + int versionOffset; /* Byte offset from start of file * where we find version number, * -1 to always fail the version test, * -2 to always pass the version test */ + int versionSize; /* Size in bytes of version data (0, 2, or 4) */
If the size is 0 ...
int versionNumbers[FILE_TYPE_VERSIONS_LAST]; /* Version numbers to validate. Zeroes are ignored. */ int sizeOffset; /* Byte offset from start of file
[...]
@@ -635,13 +637,25 @@ virStorageFileMatchesVersion(int format, if (fileTypeInfo[format].versionOffset == -2) return true;
- if ((fileTypeInfo[format].versionOffset + 4) > buflen) + if ((fileTypeInfo[format].versionOffset + + fileTypeInfo[format].versionSize) > buflen) return false;
- if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) - version = virReadBufInt32LE(buf + fileTypeInfo[format].versionOffset); - else - version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) {
... code below shouldn't be executed at all.
+ if (fileTypeInfo[format].versionSize == 4) + version = virReadBufInt32LE(buf + + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt16LE(buf + + fileTypeInfo[format].versionOffset); + } else { + if (fileTypeInfo[format].versionSize == 4) + version = virReadBufInt32BE(buf + + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt16BE(buf + + fileTypeInfo[format].versionOffset); + }
ACK with the change.

On 06/24/2016 09:28 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:02 -0400, John Ferlan wrote:
The version field historically has been a 4 byte data; however, an upcoming new type will use a 2 byte version. So let's adjust for that now.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 60 +++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index de4955b..37e9798 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1,7 +1,7 @@ /* * virstoragefile.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2014, 2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -120,10 +120,12 @@ struct FileTypeInfo { * to check at head of file */ const char *extension; /* Optional file extension to check */ enum lv_endian endian; /* Endianness of file format */ + int versionOffset; /* Byte offset from start of file * where we find version number, * -1 to always fail the version test, * -2 to always pass the version test */ + int versionSize; /* Size in bytes of version data (0, 2, or 4) */
If the size is 0 ...
int versionNumbers[FILE_TYPE_VERSIONS_LAST]; /* Version numbers to validate. Zeroes are ignored. */ int sizeOffset; /* Byte offset from start of file
[...]
@@ -635,13 +637,25 @@ virStorageFileMatchesVersion(int format, if (fileTypeInfo[format].versionOffset == -2) return true;
So you would like to see: if (fileTypeInfo[format].versionSize == 0) return false;
- if ((fileTypeInfo[format].versionOffset + 4) > buflen) + if ((fileTypeInfo[format].versionOffset + + fileTypeInfo[format].versionSize) > buflen) return false;
- if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) - version = virReadBufInt32LE(buf + fileTypeInfo[format].versionOffset); - else - version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) {
... code below shouldn't be executed at all.
It wouldn't be because for all size = 0, the versionOffset == -1 or -2 we will have returned false much sooner. I *did* have a check at one time for size == 0, then return false, but took it out. I can replace it though
+ if (fileTypeInfo[format].versionSize == 4) + version = virReadBufInt32LE(buf + + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt16LE(buf + + fileTypeInfo[format].versionOffset); + } else { + if (fileTypeInfo[format].versionSize == 4) + version = virReadBufInt32BE(buf + + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt16BE(buf + + fileTypeInfo[format].versionOffset); + }
ACK with the change.

[...]
@@ -635,13 +637,25 @@ virStorageFileMatchesVersion(int format, if (fileTypeInfo[format].versionOffset == -2) return true;
So you would like to see:
if (fileTypeInfo[format].versionSize == 0) return false;
Sent before I thought about this option: /* A positive versionOffset, requires using a valid versionSize */ if (fileTypeInfo[format].versionSize != 2 && fileTypeInfo[format].versionSize != 4) return false; John

Add the ability to detect a luks encrypted device. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 24 ++++++++++++++++++++++-- src/util/virstoragefile.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 37e9798..59927ea 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -63,7 +63,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, "cloop", "dmg", "iso", "vpc", "vdi", /* Not direct file formats, but used for various drivers */ - "fat", "vhd", "ploop", + "fat", "vhd", "ploop", "luks", /* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk") @@ -189,6 +189,13 @@ qedGetBackingStore(char **, int *, const char *, size_t); #define PLOOP_IMAGE_SIZE_OFFSET 36 #define PLOOP_SIZE_MULTIPLIER 512 +#define LUKS_HDR_MAGIC_LEN 6 +#define LUKS_HDR_VERSION_LEN 2 + +/* Format described by qemu commit id '3e308f20e' */ +#define LUKS_HDR_VERSION_OFFSET LUKS_HDR_MAGIC_LEN + + static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, @@ -244,6 +251,13 @@ static struct FileTypeInfo const fileTypeInfo[] = { -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0, PLOOP_SIZE_MULTIPLIER, -1, NULL, NULL }, + /* Magic is 'L','U','K','S', 0xBA, 0xBE + * Set sizeOffset = -1 and let hypervisor handle */ + [VIR_STORAGE_FILE_LUKS] = { + 0, "\x4c\x55\x4b\x53\xba\xbe", NULL, + LV_BIG_ENDIAN, LUKS_HDR_VERSION_OFFSET, 2, {1}, + -1, 0, 0, -1, NULL, NULL + }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", NULL, @@ -626,7 +640,7 @@ virStorageFileMatchesVersion(int format, char *buf, size_t buflen) { - int version; + int version = 0; size_t i; /* Validate version number info */ @@ -838,6 +852,12 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, goto cleanup; } + if (meta->format == VIR_STORAGE_FILE_LUKS) { + /* By definition, this is encrypted */ + if (!meta->encryption && VIR_ALLOC(meta->encryption) < 0) + goto cleanup; + } + VIR_FREE(meta->backingStoreRaw); if (fileTypeInfo[meta->format].getBackingStore != NULL) { int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 71a8b3a..78beaf4 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -74,6 +74,7 @@ typedef enum { VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, VIR_STORAGE_FILE_PLOOP, + VIR_STORAGE_FILE_LUKS, /* Not a format, but a marker: all formats below this point have * libvirt support for following a backing chain */ -- 2.5.5

On Thu, Jun 23, 2016 at 13:29:03 -0400, John Ferlan wrote:
Add the ability to detect a luks encrypted device.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 24 ++++++++++++++++++++++-- src/util/virstoragefile.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-)
ACK

Add parse and format of the luks/passphrase secret including tests for volume XML parsing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 41 ++++++++++++++++++++ .../qemuxml2xmlout-luks-disks.xml | 45 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + tests/storagevolxml2xmlin/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmlout/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmltest.c | 1 + 15 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml create mode 100644 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 79c4082..578c80e 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 fae86eb..3a08192 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -59,8 +59,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><secret type='passphrase'...></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: @@ -70,5 +82,17 @@ <encryption format='qcow'> <secret type='passphrase' uuid='c1f11a6d-8c5d-4a3e-ac7a-4e171c5e0d4a' /> </encryption></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> + <encryption format='luks'> + <secret type='passphrase' usage='luks_example'/> + </encryption> + </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 fd76e21..89d5962 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 cac6bb8..bc4f238 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..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -0,0 +1,41 @@ +<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'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-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/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

On Thu, Jun 23, 2016 at 13:29:04 -0400, John Ferlan wrote:
Add parse and format of the luks/passphrase secret including tests for volume XML parsing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 41 ++++++++++++++++++++ .../qemuxml2xmlout-luks-disks.xml | 45 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + tests/storagevolxml2xmlin/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmlout/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmltest.c | 1 + 15 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml create mode 100644 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/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml new file mode 100644 index 0000000..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -0,0 +1,41 @@
[...]
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml
This is the diff of the above files. $ diff -u tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml --- tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml 2016-06-24 15:37:13.215501639 +0200 +++ tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml 2016-06-24 15:37:13.215501639 +0200 @@ -32,14 +32,10 @@ </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='usb' index='0'/> <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> + <memballoon model='virtio'/> </devices> </domain> Use of a separate output file doesn't make any sense. ACK if you get rid of tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml

On 06/24/2016 09:45 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:04 -0400, John Ferlan wrote:
Add parse and format of the luks/passphrase secret including tests for volume XML parsing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 41 ++++++++++++++++++++ .../qemuxml2xmlout-luks-disks.xml | 45 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + tests/storagevolxml2xmlin/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmlout/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmltest.c | 1 + 15 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml create mode 100644 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/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml new file mode 100644 index 0000000..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -0,0 +1,41 @@
[...]
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml
This is the diff of the above files.
$ diff -u tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml --- tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml 2016-06-24 15:37:13.215501639 +0200 +++ tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml 2016-06-24 15:37:13.215501639 +0200 @@ -32,14 +32,10 @@ </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='usb' index='0'/> <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> + <memballoon model='virtio'/> </devices> </domain>
Use of a separate output file doesn't make any sense.
ACK if you get rid of tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml
OK - I know this was a "more recent" change to not have extra output files, but it wasn't something I followed closely enough to make it part of my "routine" when adding new tests. Tks - John

On Fri, Jun 24, 2016 at 11:32:43 -0400, John Ferlan wrote:
On 06/24/2016 09:45 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:04 -0400, John Ferlan wrote:
Add parse and format of the luks/passphrase secret including tests for volume XML parsing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 | 41 ++++++++++++++++++++ .../qemuxml2xmlout-luks-disks.xml | 45 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + tests/storagevolxml2xmlin/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmlout/vol-luks.xml | 21 ++++++++++ tests/storagevolxml2xmltest.c | 1 + 15 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml create mode 100644 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/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml new file mode 100644 index 0000000..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -0,0 +1,41 @@
[...]
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml
This is the diff of the above files.
$ diff -u tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml --- tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml 2016-06-24 15:37:13.215501639 +0200 +++ tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml 2016-06-24 15:37:13.215501639 +0200 @@ -32,14 +32,10 @@ </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='usb' index='0'/> <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> + <memballoon model='virtio'/> </devices> </domain>
Use of a separate output file doesn't make any sense.
ACK if you get rid of tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml
OK - I know this was a "more recent" change to not have extra output files, but it wasn't something I followed closely enough to make it part of my "routine" when adding new tests.
Sorry for that. Apparently this is no longer the case. Previously (until 1.3.1 afaik) it was not necessary to provide the output file, but it recently started to be. Sigh. More useless stuff in the repo. Peter

For a luks device, allow the configuration of a specific cipher to be used for encrypting the volume. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorageencryption.html.in | 81 +++++++++++++- docs/schemas/storagecommon.rng | 44 +++++++- src/conf/domain_conf.c | 11 ++ src/util/virstorageencryption.c | 124 +++++++++++++++++++++ src/util/virstorageencryption.h | 13 +++ .../qemuxml2argv-luks-disk-cipher.xml | 41 +++++++ .../qemuxml2xmlout-luks-disk-cipher.xml | 45 ++++++++ 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, 401 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 100644 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 3a08192..80111e3 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -71,6 +71,58 @@ 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 + for QEMU 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 hypervisor 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> @@ -84,9 +136,12 @@ </encryption></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>id</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> <encryption format='luks'> @@ -94,5 +149,25 @@ </encryption> </pre> + <p> + Here is an example, specifying use of the <code>luks</code> format for + a specific cipher algorihm for volume creation: + </p> + <pre> + <volume> + <name>twofish.luks</name> + <capacity unit='G'>5</capacity> + <target> + <path>/var/lib/libvirt/images/demo.luks</path> + <format type='luks'/> + <encryption format='luks'> + <secret type='passphrase' usage='luks_example'/> + <cipher name='twofish' size='256' mode='cbc' hash='sha256'/> + <ivgen name='plain64' hash='sha256'/> + </encryption> + </target> + </volume> + </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 9443281..1c3bdc4 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->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..6544123 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,15 @@ VIR_ENUM_IMPL(virStorageEncryptionFormat, "default", "qcow", "luks") static void +virStorageEncryptionInfoDefFree(virStorageEncryptionInfoDefPtr def) +{ + VIR_FREE(def->name); + VIR_FREE(def->mode); + VIR_FREE(def->hash); +} + + +static void virStorageEncryptionSecretFree(virStorageEncryptionSecretPtr secret) { if (!secret) @@ -63,6 +73,8 @@ virStorageEncryptionFree(virStorageEncryptionPtr enc) for (i = 0; i < enc->nsecrets; i++) virStorageEncryptionSecretFree(enc->secrets[i]); + virStorageEncryptionInfoDefFree(&enc->cipher); + virStorageEncryptionInfoDefFree(&enc->ivgen); VIR_FREE(enc->secrets); VIR_FREE(enc); } @@ -80,6 +92,20 @@ virStorageEncryptionSecretCopy(const virStorageEncryptionSecret *src) return ret; } + +static int +virStorageEncryptionInfoDefCopy(const virStorageEncryptionInfoDef *src, + virStorageEncryptionInfoDefPtr dst) +{ + if (VIR_STRDUP(dst->name, src->name) < 0 || + VIR_STRDUP(dst->mode, src->mode) < 0 || + VIR_STRDUP(dst->hash, src->hash) < 0) + return -1; + + return 0; +} + + virStorageEncryptionPtr virStorageEncryptionCopy(const virStorageEncryption *src) { @@ -100,6 +126,12 @@ virStorageEncryptionCopy(const virStorageEncryption *src) goto error; } + if (virStorageEncryptionInfoDefCopy(&src->cipher, &ret->cipher) < 0) + goto error; + + if (virStorageEncryptionInfoDefCopy(&src->ivgen, &ret->ivgen) < 0) + goto error; + return ret; error: @@ -153,6 +185,51 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, return NULL; } + +static int +virStorageEncryptionInfoParse(xmlNodePtr info_node, + virStorageEncryptionInfoDefPtr info, + bool require_size) +{ + int ret = -1; + char *size_str = NULL; + + if (!(info->name = virXMLPropString(info_node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing secret key info name string")); + goto cleanup; + } + + /* Check for a size string - it's required for cipher, but not for ivgen + * if provided for ivgen then just ignore */ + if (require_size) { + if ((size_str = virXMLPropString(info_node, "size")) && + virStrToLong_uip(size_str, NULL, 10, &info->size) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse secret key info size string '%s'"), + size_str); + goto cleanup; + } + + if (!size_str) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("secret key info missing 'size' attribute")); + goto cleanup; + } + } + + /* Optional */ + info->mode = virXMLPropString(info_node, "mode"); + info->hash = virXMLPropString(info_node, "hash"); + + ret = 0; + + cleanup: + VIR_FREE(size_str); + return ret; +} + + static virStorageEncryptionPtr virStorageEncryptionParseXML(xmlXPathContextPtr ctxt) { @@ -196,6 +273,28 @@ virStorageEncryptionParseXML(xmlXPathContextPtr ctxt) VIR_FREE(nodes); } + if (ret->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { + xmlNodePtr tmpnode; + + if ((tmpnode = virXPathNode("./cipher[1]", ctxt))) { + if (virStorageEncryptionInfoParse(tmpnode, &ret->cipher, true) < 0) + goto cleanup; + } + + if ((tmpnode = virXPathNode("./ivgen[1]", ctxt))) { + /* If no cipher node, then fail */ + if (!ret->cipher.name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage encryption cipher")); + goto cleanup; + } + + if (virStorageEncryptionInfoParse(tmpnode, &ret->ivgen, false) < 0) + goto cleanup; + } + } + + return ret; cleanup: @@ -250,6 +349,28 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, return 0; } + +static void +virStorageEncryptionInfoDefFormat(virBufferPtr buf, + const virStorageEncryption *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 +391,9 @@ virStorageEncryptionFormat(virBufferPtr buf, return -1; } + if (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && enc->cipher.name) + virStorageEncryptionInfoDefFormat(buf, enc); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</encryption>\n"); diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index 5e1be3b..34bba03 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -44,6 +44,16 @@ struct _virStorageEncryptionSecret { virSecretLookupTypeDef seclookupdef; }; +/* For a key type it's possible to dictate the cipher and if necessary iv */ +typedef struct _virStorageEncryptionInfoDef virStorageEncryptionInfoDef; +typedef virStorageEncryptionInfoDef *virStorageEncryptionInfoDefPtr; +struct _virStorageEncryptionInfoDef { + unsigned int size; + char *name; + char *mode; + char *hash; +}; + typedef enum { /* "default" is only valid for volume creation */ VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0, @@ -61,6 +71,9 @@ struct _virStorageEncryption { size_t nsecrets; virStorageEncryptionSecretPtr *secrets; + + virStorageEncryptionInfoDef cipher; + virStorageEncryptionInfoDef ivgen; }; 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..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml @@ -0,0 +1,41 @@ +<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'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-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/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 Thu, Jun 23, 2016 at 13:29:05 -0400, John Ferlan wrote:
For a luks device, allow the configuration of a specific cipher to be used for encrypting the volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorageencryption.html.in | 81 +++++++++++++- docs/schemas/storagecommon.rng | 44 +++++++- src/conf/domain_conf.c | 11 ++ src/util/virstorageencryption.c | 124 +++++++++++++++++++++ src/util/virstorageencryption.h | 13 +++ .../qemuxml2argv-luks-disk-cipher.xml | 41 +++++++ .../qemuxml2xmlout-luks-disk-cipher.xml | 45 ++++++++ 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, 401 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 100644 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 3a08192..80111e3 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -71,6 +71,58 @@ 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 + for QEMU is 'aes-256-cbc' using 'essiv' for initialization vector
At this point this is the default not for qemu, but for the 'DIR' backend for the storage driver that is using qemu-img.
+ 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 hypervisor dependent.</dd>
This is storage driver implementation dependent.
+ <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>
@@ -84,9 +136,12 @@ </encryption></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>id</code> of "luks_example",
s/id/name/
+ 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> <encryption format='luks'> @@ -94,5 +149,25 @@ </encryption> </pre>
+ <p> + Here is an example, specifying use of the <code>luks</code> format for + a specific cipher algorihm for volume creation: + </p> + <pre> + <volume> + <name>twofish.luks</name> + <capacity unit='G'>5</capacity> + <target> + <path>/var/lib/libvirt/images/demo.luks</path> + <format type='luks'/> + <encryption format='luks'> + <secret type='passphrase' usage='luks_example'/> + <cipher name='twofish' size='256' mode='cbc' hash='sha256'/> + <ivgen name='plain64' hash='sha256'/> + </encryption> + </target> + </volume> + </pre> + </body> </html>
[...]
diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index 5e1be3b..34bba03 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -44,6 +44,16 @@ struct _virStorageEncryptionSecret { virSecretLookupTypeDef seclookupdef; };
+/* For a key type it's possible to dictate the cipher and if necessary iv */ +typedef struct _virStorageEncryptionInfoDef virStorageEncryptionInfoDef; +typedef virStorageEncryptionInfoDef *virStorageEncryptionInfoDefPtr; +struct _virStorageEncryptionInfoDef { + unsigned int size; + char *name; + char *mode; + char *hash;
I'd either store the IV generator info here or add a different structure. It seems weird to reuse it just because it has two fields with same name.
+}; + typedef enum { /* "default" is only valid for volume creation */ VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0,
[...]
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..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml
[...]
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml
Again the output file is not really needed: $ diff -u qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml --- qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml 2016-06-24 15:55:42.059454563 +0200 +++ qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml 2016-06-24 15:55:42.059454563 +0200 @@ -32,10 +32,14 @@ </encryption> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </disk> - <controller type='usb' index='0'/> + <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'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> </devices> </domain> Difference to the test files added in previous patch makes this test even less useful: $ diff -u qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml qemuxml2argvdata/qemuxml2argv-luks-disks.xml $

On 06/24/2016 10:10 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:05 -0400, John Ferlan wrote:
For a luks device, allow the configuration of a specific cipher to be used for encrypting the volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorageencryption.html.in | 81 +++++++++++++- docs/schemas/storagecommon.rng | 44 +++++++- src/conf/domain_conf.c | 11 ++ src/util/virstorageencryption.c | 124 +++++++++++++++++++++ src/util/virstorageencryption.h | 13 +++ .../qemuxml2argv-luks-disk-cipher.xml | 41 +++++++ .../qemuxml2xmlout-luks-disk-cipher.xml | 45 ++++++++ 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, 401 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 100644 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 3a08192..80111e3 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -71,6 +71,58 @@ 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 + for QEMU is 'aes-256-cbc' using 'essiv' for initialization vector
At this point this is the default not for qemu, but for the 'DIR' backend for the storage driver that is using qemu-img.
Does this wording suffice: 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 hypervisor dependent.</dd>
This is storage driver implementation dependent.
OK
+ <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>
@@ -84,9 +136,12 @@ </encryption></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>id</code> of "luks_example",
s/id/name/
Right.
+ 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> <encryption format='luks'> @@ -94,5 +149,25 @@ </encryption> </pre>
+ <p> + Here is an example, specifying use of the <code>luks</code> format for + a specific cipher algorihm for volume creation: + </p> + <pre> + <volume> + <name>twofish.luks</name> + <capacity unit='G'>5</capacity> + <target> + <path>/var/lib/libvirt/images/demo.luks</path> + <format type='luks'/> + <encryption format='luks'> + <secret type='passphrase' usage='luks_example'/> + <cipher name='twofish' size='256' mode='cbc' hash='sha256'/> + <ivgen name='plain64' hash='sha256'/> + </encryption> + </target> + </volume> + </pre> + </body> </html>
[...]
diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index 5e1be3b..34bba03 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -44,6 +44,16 @@ struct _virStorageEncryptionSecret { virSecretLookupTypeDef seclookupdef; };
+/* For a key type it's possible to dictate the cipher and if necessary iv */ +typedef struct _virStorageEncryptionInfoDef virStorageEncryptionInfoDef; +typedef virStorageEncryptionInfoDef *virStorageEncryptionInfoDefPtr; +struct _virStorageEncryptionInfoDef { + unsigned int size; + char *name; + char *mode; + char *hash;
I'd either store the IV generator info here or add a different structure. It seems weird to reuse it just because it has two fields with same name.
Geez and I thought it was more useful to reuse code... I'll adjust though, no big deal. It's all one larger happy structure now.
+}; + typedef enum { /* "default" is only valid for volume creation */ VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0,
[...]
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..00399cf --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml
[...]
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml new file mode 100644 index 0000000..9ce15c0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml
Again the output file is not really needed: $ diff -u qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml --- qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml 2016-06-24 15:55:42.059454563 +0200 +++ qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml 2016-06-24 15:55:42.059454563 +0200 @@ -32,10 +32,14 @@ </encryption> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </disk> - <controller type='usb' index='0'/> + <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'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> </devices> </domain>
Difference to the test files added in previous patch makes this test even less useful:
$ diff -u qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml qemuxml2argvdata/qemuxml2argv-luks-disks.xml $
OK same as before... Fixed John

Create a function to return a temporary file path to be used in a mkostemp type call using the path to the stateDir + pool->def->name + vol->name Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 24 ++++++++++++++++++++++++ src/storage/storage_driver.h | 6 +++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4b5419d..92a1fb9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3608,3 +3608,27 @@ virStoragePoolObjFindPoolByUUID(const unsigned char *uuid) storageDriverUnlock(); return pool; } + + +/* + * virStoragePoolObjBuildTempFilePath + * @pool: pool object pointer + * @vol: volume definition + * + * Generate a name for a temporary file using the driver stateDir + * as a path, the pool name, and the volume name to be used as input + * for a mkostemp + * + * Returns a string pointer on success, NULL on failure + */ +char * +virStoragePoolObjBuildTempFilePath(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) + +{ + char *tmp = NULL; + + ignore_value(virAsprintf(&tmp, "%s/%s.%s.secret.XXXXXX", + driver->stateDir, pool->def->name, vol->name)); + return tmp; +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 912c232..99c58bc 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -1,7 +1,7 @@ /* * storage_driver.h: core driver for storage APIs * - * Copyright (C) 2006-2008, 2014 Red Hat, Inc. + * Copyright (C) 2006-2008, 2014-2016 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -65,6 +65,10 @@ storagePoolLookupByTargetPath(virConnectPtr conn, const char *path) ATTRIBUTE_NONNULL(2); +char *virStoragePoolObjBuildTempFilePath(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + int storageRegister(void); #endif /* __VIR_STORAGE_DRIVER_H__ */ -- 2.5.5

On Thu, Jun 23, 2016 at 13:29:06 -0400, John Ferlan wrote:
Create a function to return a temporary file path to be used in a mkostemp type call using the path to the stateDir + pool->def->name + vol->name
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 24 ++++++++++++++++++++++++ src/storage/storage_driver.h | 6 +++++- 2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 912c232..99c58bc 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -1,7 +1,7 @@ /* * storage_driver.h: core driver for storage APIs * - * Copyright (C) 2006-2008, 2014 Red Hat, Inc. + * Copyright (C) 2006-2008, 2014-2016 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or
ACK without the noise.

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 1d. change file protections to 0400 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@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 260 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 ++++ src/util/virqemu.h | 6 + tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 275 insertions(+), 21 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b118d1e..9160b22 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2163,6 +2163,7 @@ virProcessWait; # util/virqemu.h +virQEMUBuildLuksOpts; virQEMUBuildObjectCommandlineFromJSON; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 89d5962..8918f3e 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" @@ -880,6 +883,7 @@ virStoragePloopResize(virStorageVolDefPtr vol, enum { QEMU_IMG_BACKING_FORMAT_OPTIONS = 0, QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, + QEMU_IMG_FORMAT_LUKS, }; static bool @@ -907,6 +911,27 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) return ret; } + +static bool +virStorageBackendQemuImgSupportsLuks(const char *qemuimg) +{ + bool ret = false; + int exitstatus = -1; + virCommandPtr cmd = virCommandNewArgList(qemuimg, "create", "-o", "?", + "-f", "luks", "/dev/null", NULL); + + if (virCommandRun(cmd, &exitstatus) < 0) + goto cleanup; + + if (exitstatus == 0) + ret = true; + + cleanup: + virCommandFree(cmd); + return ret; +} + + static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { @@ -915,12 +940,18 @@ virStorageBackendQEMUImgBackingFormat(const char *qemuimg) * out what else we have */ int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS; - /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer - * understands. Since we still support QEMU 0.12 and newer, we need - * to be able to handle the previous format as can be set via a - * compat=0.10 option. */ - if (virStorageBackendQemuImgSupportsCompat(qemuimg)) - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; + /* QEMU 2.6 added support for luks - let's check for that. + * If available, then we can also assume OPTIONS_COMPAT is present */ + if (virStorageBackendQemuImgSupportsLuks(qemuimg)) { + ret = QEMU_IMG_FORMAT_LUKS; + } else { + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer + * understands. Since we still support QEMU 0.12 and newer, we need + * to be able to handle the previous format as can be set via a + * compat=0.10 option. */ + if (virStorageBackendQemuImgSupportsCompat(qemuimg)) + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; + } return ret; } @@ -941,21 +972,31 @@ struct _virStorageBackendQemuImgInfo { const char *inputPath; const char *inputFormatStr; int inputFormat; + + char *secretAlias; + const char *secretPath; }; + static int -virStorageBackendCreateQemuImgOpts(char **opts, +virStorageBackendCreateQemuImgOpts(virStorageEncryptionPtr 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) { + 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 +1066,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 +1126,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; @@ -1121,6 +1184,7 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, static int virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, + virStorageVolDefPtr vol, int imgformat, struct _virStorageBackendQemuImgInfo info) { @@ -1130,7 +1194,8 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) info.compat = "0.10"; - if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) + if (virStorageBackendCreateQemuImgOpts(vol->target.encryption, + &opts, info) < 0) return -1; if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); @@ -1140,6 +1205,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 +1252,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,6 +1265,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .compat = vol->target.compat, .features = vol->target.features, .nocow = vol->target.nocow, + .secretPath = secretPath, + .secretAlias = NULL, }; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1192,6 +1297,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) @@ -1207,7 +1324,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, conn, vol) < 0) return NULL; - /* Size in KB */ info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024); @@ -1226,11 +1342,21 @@ 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 && + virStorageBackendCreateQemuImgSecretObject(cmd, vol, &info) < 0) { + VIR_FREE(info.secretAlias); virCommandFree(cmd); return NULL; } + if (virStorageBackendCreateQemuImgSetOptions(cmd, vol, imgformat, + info) < 0) { + VIR_FREE(info.secretAlias); + virCommandFree(cmd); + return NULL; + } + VIR_FREE(info.secretAlias); + if (info.inputPath) virCommandAddArg(cmd, info.inputPath); virCommandAddArg(cmd, info.path); @@ -1240,6 +1366,84 @@ 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; + } + + /* Since we don't have a file, just go to cleanup using NULL secretPath */ + 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; + } + } + + if (chmod(secretPath, 0400) < 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 +1455,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, char *create_tool; int imgformat; virCommandPtr cmd; + char *secretPath = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); @@ -1266,8 +1471,21 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (imgformat < 0) goto cleanup; + if (vol->target.format == VIR_STORAGE_FILE_LUKS) { + if (imgformat < QEMU_IMG_FORMAT_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("this version of '%s' does not support luks"), + create_tool); + goto cleanup; + } + 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 +1493,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..a56fb3f 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, + virStorageEncryptionPtr 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) { + virBufferAsprintf(buf, "cipher-alg=%s-%u,", + enc->cipher.name, enc->cipher.size); + if (enc->cipher.mode) + virBufferAsprintf(buf, "cipher-mode=%s,", enc->cipher.mode); + if (enc->cipher.hash) + virBufferAsprintf(buf, "hash-alg=%s,", enc->cipher.hash); + if (enc->ivgen.name) + virBufferAsprintf(buf, "ivgen-alg=%s,", enc->ivgen.name); + if (enc->ivgen.hash) + virBufferAsprintf(buf, "ivgen-hash-alg=%s,", enc->ivgen.hash); + } +} diff --git a/src/util/virqemu.h b/src/util/virqemu.h index af4ec1d..dfb79b9 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, + virStorageEncryptionPtr 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

On Thu, Jun 23, 2016 at 13:29:07 -0400, John Ferlan wrote:
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 1d. change file protections to 0400
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@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 260 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 ++++ src/util/virqemu.h | 6 + tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 275 insertions(+), 21 deletions(-)
[...]
@@ -1240,6 +1366,84 @@ 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; + } + + /* Since we don't have a file, just go to cleanup using NULL secretPath */ + 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,
[1]
+ vol->target.perms->gid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to chown luks secret file")); + goto error; + } + } + + if (chmod(secretPath, 0400) < 0) {
I still don't understand this step. Owner [1] of the file is able to add the write permission back anyways.
+ 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,
ACK with either a very good explanation or the chmod gone.

On 06/24/2016 10:48 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:07 -0400, John Ferlan wrote:
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 1d. change file protections to 0400
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@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 260 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 ++++ src/util/virqemu.h | 6 + tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 275 insertions(+), 21 deletions(-)
[...]
@@ -1240,6 +1366,84 @@ 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; + } + + /* Since we don't have a file, just go to cleanup using NULL secretPath */ + 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,
[1]
+ vol->target.perms->gid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to chown luks secret file")); + goto error; + } + } + + if (chmod(secretPath, 0400) < 0) {
I still don't understand this step. Owner [1] of the file is able to add the write permission back anyways.
+ 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,
ACK with either a very good explanation or the chmod gone.
chmod is gone. Again, the set protections is a reaction to a comment Dan made in an earlier review: http://www.redhat.com/archives/libvir-list/2016-June/msg00021.html John

It's just a constant "secret" string anyway Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4fdb410..9331e65 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -510,7 +510,6 @@ qemuNetworkDriveGetPort(int protocol, /** * qemuBuildSecretInfoProps: * @secinfo: pointer to the secret info object - * @type: returns a pointer to a character string for object name * @props: json properties to return * * Build the JSON properties for the secret info type. @@ -520,14 +519,11 @@ qemuNetworkDriveGetPort(int protocol, */ static int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, - const char **type, virJSONValuePtr *propsret) { int ret = -1; char *keyid = NULL; - *type = "secret"; - if (!(keyid = qemuDomainGetMasterKeyAlias())) return -1; @@ -565,13 +561,12 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, { int ret = -1; virJSONValuePtr props = NULL; - const char *type; char *tmp = NULL; - if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) + if (qemuBuildSecretInfoProps(secinfo, &props) < 0) return -1; - if (!(tmp = virQEMUBuildObjectCommandlineFromJSON(type, + if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("secret", secinfo->s.aes.alias, props))) goto cleanup; -- 2.5.5

Need to create the object for a hotplug disk Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9331e65..5d82a4d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -517,7 +517,7 @@ qemuNetworkDriveGetPort(int protocol, * Returns 0 on success with the filled in JSON property; otherwise, * returns -1 on failure error message set. */ -static int +int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, virJSONValuePtr *propsret) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 9ff4edb..c4d0567 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -61,6 +61,10 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, const char *domainLibDir) ATTRIBUTE_NONNULL(15); +/* Generate the object properties for a secret */ +int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, + virJSONValuePtr *propsret); + /* Generate '-device' string for chardev device */ int qemuBuildChrDeviceStr(char **deviceStr, -- 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@redhat.com> --- src/qemu/qemu_hotplug.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f695903..235cb73 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); @@ -3389,6 +3411,8 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(detach); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -3422,12 +3446,14 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); - goto cleanup; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias) < 0) + goto faildel; } + + if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) + goto faildel; + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3437,6 +3463,12 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); return ret; + + faildel: + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); + goto cleanup; } static int -- 2.5.5

On Thu, Jun 23, 2016 at 13:29:10 -0400, John Ferlan wrote:
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@redhat.com> --- src/qemu/qemu_hotplug.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-)
[...]
@@ -3422,12 +3446,14 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); - goto cleanup; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias) < 0)
Additionally to the complaint about secinfo not being initialized. This is the incorrect place to delete this. You need to do it in qemuDomainRemoveDiskDevice. Also at that point you can't make the failure to delete the object fatal.
+ goto faildel; } + + if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) + goto faildel; +

On 06/24/2016 08:51 AM, Peter Krempa wrote:
On Thu, Jun 23, 2016 at 13:29:10 -0400, John Ferlan wrote:
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@redhat.com> --- src/qemu/qemu_hotplug.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-)
[...]
@@ -3422,12 +3446,14 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); - goto cleanup; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias) < 0)
Additionally to the complaint about secinfo not being initialized. This is the incorrect place to delete this. You need to do it in qemuDomainRemoveDiskDevice.
RemoveDiskDevice I thought was for SCSI and USB All those *still* confound me... DetachDisk/RemoveDisk...
Also at that point you can't make the failure to delete the object fatal.
Right... I'll post a v3 shortly with changes that weren't officially ACK'd, but it seems they were close, plus a few that were ACK w/ changes, plus two new ones borne out of review, and of course these last 2 patches. Thanks for the persistence! John
+ goto faildel; } + + if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) + goto faildel; +

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@redhat.com> --- src/qemu/qemu_command.c | 12 +++- src/qemu/qemu_domain.c | 50 +++++++++++------ src/qemu/qemu_hotplug.c | 65 +++++++++++++++++++--- .../qemuxml2argv-luks-disk-cipher.args | 36 ++++++++++++ .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 ++++++++++++ tests/qemuxml2argvtest.c | 11 +++- 6 files changed, 182 insertions(+), 28 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 5d82a4d..6ba9607 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) { @@ -1232,10 +1233,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, qemuBufferEscapeComma(&opt, source); virBufferAddLit(&opt, ","); - if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) virBufferAsprintf(&opt, "password-secret=%s,", secinfo->s.aes.alias); - } + + if (encinfo) + virQEMUBuildLuksOpts(&opt, disk->src->encryption, + encinfo->s.aes.alias); if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) @@ -1939,6 +1943,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) && @@ -1968,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 dca8970..53744c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -941,7 +941,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) < 0) @@ -989,27 +990,42 @@ 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)) { - - virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; + if (conn && !virStorageSourceIsEmpty(src)) { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - if (VIR_ALLOC(secinfo) < 0) - return -1; + if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK && + src->auth && + (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI || + src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { - if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) - secretUsageType = VIR_SECRET_USAGE_TYPE_CEPH; + virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; - if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, - secretUsageType, src->auth->username, - &src->auth->seclookupdef) < 0) - goto error; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) + secretUsageType = VIR_SECRET_USAGE_TYPE_CEPH; - diskPriv->secinfo = secinfo; + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + secretUsageType, src->auth->username, + &src->auth->seclookupdef) < 0) + goto error; + + diskPriv->secinfo = secinfo; + } + + if (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) < 0) + goto error; + + diskPriv->encinfo = secinfo; + } } return 0; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 235cb73..ab8fd38 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; @@ -364,7 +370,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; - /* Attach the device - possible 3 step process */ + /* Attach the device - possible 4 step process */ qemuDomainObjEnterMonitor(driver, vm); if (secobjProps && qemuMonitorAddObject(priv->mon, "secret", @@ -373,6 +379,12 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto failaddobjsecret; secobjProps = NULL; + if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto failaddencsecret; + encProps = NULL; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -391,6 +403,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, cleanup: virJSONValueFree(secobjProps); + virJSONValueFree(encProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -410,8 +423,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, } failadddrive: + if (encProps) + ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + + failaddencsecret: if (secobjProps) ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); + encProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ failaddobjsecret: if (qemuDomainObjExitMonitor(driver, vm) < 0) @@ -571,6 +589,9 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, 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; @@ -589,6 +610,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; @@ -598,9 +624,15 @@ qemuDomainAttachSCSIDisk(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 (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto failaddencsecret; + encProps = NULL; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -627,7 +659,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr); failadddrive: + if (encProps) + ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + + failaddencsecret: ignore_value(qemuDomainObjExitMonitor(driver, vm)); + encProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -3413,6 +3450,7 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(detach); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -3451,6 +3489,9 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, goto faildel; } + if (encinfo && qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias) < 0) + goto faildel; + if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) goto faildel; @@ -3478,6 +3519,8 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(detach); + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; if (qemuDomainDiskBlockJobIsActive(detach)) goto cleanup; @@ -3485,12 +3528,12 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); - goto cleanup; - } + if (encinfo && qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias) < 0) + goto faildel; + + if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) + goto faildel; + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3500,6 +3543,12 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); return ret; + + faildel: + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); + goto cleanup; } static int diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args new file mode 100644 index 0000000..6eebc87 --- /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-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk,key-secret=virtio-disk0-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-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk2,key-secret=virtio-disk1-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..6eebc87 --- /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-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk,key-secret=virtio-disk0-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-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk2,key-secret=virtio-disk1-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
participants (2)
-
John Ferlan
-
Peter Krempa