[PATCH v1 0/7] qemu: add support for librbd layered encryption
Starting from Ceph 0f93f745 (unreleased 18.0.0) and qemu 0f385a24 (unreleased 8.0.0), qemu and librbd users can use encrypted RBD cloned images, where the parent image is encrypted using a different scheme (e.g. different passphrase). Opening such image require supplying of multiple secrets. This patch series allows libvirt users to supply multiple secrets necessary for using such RBD images. For example: <encryption format='luks' engine='librbd'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80fb0'/> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> </encryption> Note that unlike the qemu and libvirt API, we don't allow the user to specify the format of the parent image, but just the passphrase. We do so to minimize the changes made in libvirt. To still be able to support RBD images where the parent is encrypted using a different format (e.g. LUKS2 cloned image of a LUKS parent), an additional patch series allowing for LUKS* (luks-any) format will be submitted. In high-level, this patch series does the following: - change the qemuBlockStorageSourceAttachData struct to support multiple secrets - change the qemuDomainStorageSourcePrivate struct to support multiple secrets - translate multiple secrets from virStorageEncryption to qemu private data I manually patched the qemu 8.0.0 replies file to reflect relevant qemu support, to allow my tests to run. Note that any build qemu will not support this feature, unless compiled while having a librbd that has this feature bundled. Or Ozeri (7): tests: qemucapabilitiesdata: Add rbd encryption layering qemu: capabilities: Introduce QEMU_CAPS_RBD_ENCRYPTION_LAYERING capability qemu: add support for multiple secret aliases qemu: add multi-secret support in qemuBlockStorageSourceAttachData qemu: add multi-secret support in _qemuDomainStorageSourcePrivate qemu: support pass-on of multiple secrets to _qemuDomainStorageSourcePrivate qemu: add support for librbd layered encryption docs/formatstorageencryption.rst | 11 +- src/conf/schemas/storagecommon.rng | 4 +- src/qemu/qemu_alias.c | 8 +- src/qemu/qemu_alias.h | 3 +- src/qemu/qemu_block.c | 70 ++++++++---- src/qemu/qemu_block.h | 5 +- src/qemu/qemu_blockjob.c | 6 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 31 +++-- src/qemu/qemu_domain.c | 106 ++++++++++++++---- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration_params.c | 2 +- tests/qemublocktest.c | 7 +- .../caps_8.0.0.x86_64.replies | 5 + .../caps_8.0.0.x86_64.xml | 1 + ...k-rbd-encryption-layering.x86_64-7.2.0.err | 1 + ...rbd-encryption-layering.x86_64-latest.args | 39 +++++++ .../disk-network-rbd-encryption-layering.xml | 40 +++++++ tests/qemuxml2argvtest.c | 2 + ...-rbd-encryption-layering.x86_64-latest.xml | 45 ++++++++ tests/qemuxml2xmltest.c | 1 + 23 files changed, 332 insertions(+), 63 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-7.2.0.err create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml create mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-encryption-layering.x86_64-latest.xml -- 2.25.1
RBD encryption layering support was added to qemu in 0f385a24. Signed-off-by: Or Ozeri<oro@il.ibm.com> --- tests/qemucapabilitiesdata/caps_8.0.0.x86_64.replies | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.replies index a41b3e1825..21df6c5e22 100644 --- a/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.replies @@ -17041,6 +17041,11 @@ { "name": "format", "type": "666" + }, + { + "name": "parent", + "default": null, + "type": "545" } ], "meta-type": "object" -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:06 -0600, Or Ozeri wrote:
RBD encryption layering support was added to qemu in 0f385a24.
Signed-off-by: Or Ozeri<oro@il.ibm.com> --- tests/qemucapabilitiesdata/caps_8.0.0.x86_64.replies | 5 +++++ 1 file changed, 5 insertions(+)
This patch won't be needed any more as I've recently updated the qemu caps dump and it now has this field.
This capability represents that qemu supports the layered encryption of RBD images, where a cloned image is encrypted with a possible different encryption than its parent image. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_8.0.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3cb5785baa..fe69a752ee 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -690,6 +690,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 445 */ "netdev.stream.reconnect", /* QEMU_CAPS_NETDEV_STREAM_RECONNECT */ + "rbd-encryption-layering", /* QEMU_CAPS_RBD_ENCRYPTION_LAYERING */ ); @@ -1554,6 +1555,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "blockdev-add/arg-type/+nvme", QEMU_CAPS_DRIVE_NVME }, { "blockdev-add/arg-type/+file/aio/^io_uring", QEMU_CAPS_AIO_IO_URING }, { "blockdev-add/arg-type/+rbd/encrypt", QEMU_CAPS_RBD_ENCRYPTION }, + { "blockdev-add/arg-type/+rbd/encrypt/parent", QEMU_CAPS_RBD_ENCRYPTION_LAYERING }, { "blockdev-add/arg-type/+nbd/tls-hostname", QEMU_CAPS_BLOCKDEV_NBD_TLS_HOSTNAME }, { "blockdev-snapshot/$allow-write-only-overlay", QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY }, { "chardev-add/arg-type/backend/+socket/data/reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d049f79dd9..1f29c691e7 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -669,6 +669,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 445 */ QEMU_CAPS_NETDEV_STREAM_RECONNECT, /* -netdev stream supports reconnect */ + QEMU_CAPS_RBD_ENCRYPTION_LAYERING, /* layered encryption support for Ceph RBD */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.xml index ce051d3f1c..b90ad6d831 100644 --- a/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_8.0.0.x86_64.xml @@ -206,6 +206,7 @@ <flag name='cryptodev-backend-lkcf'/> <flag name='pvpanic-pci'/> <flag name='netdev.stream.reconnect'/> + <flag name='rbd-encryption-layering'/> <version>7002050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100244</microcodeVersion> -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:07 -0600, Or Ozeri wrote:
This capability represents that qemu supports the layered encryption of RBD images, where a cloned image is encrypted with a possible different encryption than its parent image.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_8.0.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Change secret aliases from %s-%s-secret0 to %s-%s-secret%lu, which will later be used for storage encryption requiring more than a single secret. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_alias.c | 8 +++++--- src/qemu/qemu_alias.h | 3 ++- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration_params.c | 2 +- 5 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index a9809797d5..2e0a50b68b 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -801,17 +801,19 @@ qemuDomainGetMasterKeyAlias(void) /* qemuAliasForSecret: * @parentalias: alias of the parent object * @obj: optional sub-object of the parent device the secret is for + * @secret_idx: secret index number (0 in the case of a single secret) * * Generate alias for a secret object used by @parentalias device or one of * the dependencies of the device described by @obj. */ char * qemuAliasForSecret(const char *parentalias, - const char *obj) + const char *obj, + size_t secret_idx) { if (obj) - return g_strdup_printf("%s-%s-secret0", parentalias, obj); - return g_strdup_printf("%s-secret0", parentalias); + return g_strdup_printf("%s-%s-secret%lu", parentalias, obj, secret_idx); + return g_strdup_printf("%s-secret%lu", parentalias, secret_idx); } /* qemuAliasTLSObjFromSrcAlias diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index f13f4cc5f8..eae08020dc 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -86,7 +86,8 @@ char *qemuAliasFromHostdev(const virDomainHostdevDef *hostdev); char *qemuDomainGetMasterKeyAlias(void); char *qemuAliasForSecret(const char *parentalias, - const char *obj); + const char *obj, + size_t secret_idx); char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5fd140c85..80c9852dae 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1317,6 +1317,7 @@ qemuDomainSecretInfoSetup(qemuDomainObjPrivate *priv, * @priv: pointer to domain private object * @srcalias: Alias of the disk/hostdev used to generate the secret alias * @secretuse: specific usage for the secret (may be NULL if main object is using it) + * @secret_idx: secret index number (0 in the case of a single secret) * @usageType: The virSecretUsageType * @username: username to use for authentication (may be NULL) * @seclookupdef: Pointer to seclookupdef data @@ -1329,12 +1330,13 @@ static qemuDomainSecretInfo * qemuDomainSecretInfoSetupFromSecret(qemuDomainObjPrivate *priv, const char *srcalias, const char *secretuse, + size_t secret_idx, virSecretUsageType usageType, const char *username, virSecretLookupTypeDef *seclookupdef) { qemuDomainSecretInfo *secinfo; - g_autofree char *alias = qemuAliasForSecret(srcalias, secretuse); + g_autofree char *alias = qemuAliasForSecret(srcalias, secretuse, secret_idx); g_autofree uint8_t *secret = NULL; size_t secretlen = 0; VIR_IDENTITY_AUTORESTORE virIdentity *oldident = virIdentityElevateCurrent(); @@ -1384,7 +1386,7 @@ qemuDomainSecretInfoTLSNew(qemuDomainObjPrivate *priv, } seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; - return qemuDomainSecretInfoSetupFromSecret(priv, srcAlias, NULL, + return qemuDomainSecretInfoSetupFromSecret(priv, srcAlias, NULL, 0, VIR_SECRET_USAGE_TYPE_TLS, NULL, &seclookupdef); } @@ -1411,7 +1413,7 @@ qemuDomainSecretStorageSourcePrepareCookies(qemuDomainObjPrivate *priv, virStorageSource *src, const char *aliasprotocol) { - g_autofree char *secretalias = qemuAliasForSecret(aliasprotocol, "httpcookie"); + g_autofree char *secretalias = qemuAliasForSecret(aliasprotocol, "httpcookie", 0); g_autofree char *cookies = qemuBlockStorageSourceGetCookieString(src); return qemuDomainSecretInfoSetup(priv, secretalias, NULL, @@ -1460,7 +1462,7 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, usageType = VIR_SECRET_USAGE_TYPE_CEPH; if (!(srcPriv->secinfo = qemuDomainSecretInfoSetupFromSecret(priv, aliasprotocol, - "auth", + "auth", 0, usageType, src->auth->username, &src->auth->seclookupdef))) @@ -1469,7 +1471,7 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, if (hasEnc) { if (!(srcPriv->encinfo = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, - "encryption", + "encryption", 0, VIR_SECRET_USAGE_TYPE_VOLUME, NULL, &src->encryption->secrets[0]->seclookupdef))) @@ -11181,7 +11183,7 @@ qemuDomainPrepareHostdev(virDomainHostdevDef *hostdev, if (!(srcPriv->secinfo = qemuDomainSecretInfoSetupFromSecret(priv, backendalias, - NULL, + NULL, 0, usageType, src->auth->username, &src->auth->seclookupdef))) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da17525824..f15b4ea31f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1753,7 +1753,7 @@ qemuDomainDelChardevTLSObjects(virQEMUDriver *driver, * secret UUID and we have a serial TCP chardev, then formulate a * secAlias which we'll attempt to destroy. */ if (cfg->chardevTLSx509secretUUID && - !(secAlias = qemuAliasForSecret(inAlias, NULL))) + !(secAlias = qemuAliasForSecret(inAlias, NULL, 0))) return -1; qemuDomainObjEnterMonitor(vm); diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index bd09dcfb23..0d747580f4 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1129,7 +1129,7 @@ qemuMigrationParamsResetTLS(virDomainObj *vm, return; tlsAlias = qemuAliasTLSObjFromSrcAlias(QEMU_MIGRATION_TLS_ALIAS_BASE); - secAlias = qemuAliasForSecret(QEMU_MIGRATION_TLS_ALIAS_BASE, NULL); + secAlias = qemuAliasForSecret(QEMU_MIGRATION_TLS_ALIAS_BASE, NULL, 0); qemuDomainDelTLSObjects(vm, asyncJob, secAlias, tlsAlias); g_clear_pointer(&QEMU_DOMAIN_PRIVATE(vm)->migSecinfo, qemuDomainSecretInfoFree); -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:08 -0600, Or Ozeri wrote:
Change secret aliases from %s-%s-secret0 to %s-%s-secret%lu, which will later be used for storage encryption requiring more than a single secret.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_alias.c | 8 +++++--- src/qemu/qemu_alias.h | 3 ++- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration_params.c | 2 +- 5 files changed, 17 insertions(+), 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This commit changes the qemuBlockStorageSourceAttachData struct to support multiple secrets (instead of a single one before this commit). This will useful for storage encryption requiring more than a single secret. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_block.c | 35 ++++++++++++++++++++++++++--------- src/qemu/qemu_block.h | 5 +++-- src/qemu/qemu_blockjob.c | 6 ++++++ src/qemu/qemu_command.c | 21 +++++++++++++++++---- 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 5e700eff99..2e3e0f6572 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1310,6 +1310,7 @@ qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src) void qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachData *data) { + size_t i; if (!data) return; @@ -1319,12 +1320,16 @@ qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachData *data) virJSONValueFree(data->prmgrProps); virJSONValueFree(data->authsecretProps); virJSONValueFree(data->httpcookiesecretProps); - virJSONValueFree(data->encryptsecretProps); + for (i = 0; i < data->encryptsecretCount; ++i) { + virJSONValueFree(data->encryptsecretProps[i]); + g_free(data->encryptsecretAlias[i]); + } virJSONValueFree(data->tlsProps); virJSONValueFree(data->tlsKeySecretProps); g_free(data->tlsAlias); g_free(data->tlsKeySecretAlias); g_free(data->authsecretAlias); + g_free(data->encryptsecretProps); g_free(data->encryptsecretAlias); g_free(data->httpcookiesecretAlias); g_free(data->driveCmd); @@ -1435,10 +1440,12 @@ static int qemuBlockStorageSourceAttachApplyFormatDeps(qemuMonitor *mon, qemuBlockStorageSourceAttachData *data) { - if (data->encryptsecretProps && - qemuMonitorAddObject(mon, &data->encryptsecretProps, - &data->encryptsecretAlias) < 0) - return -1; + size_t i; + for (i = 0; i < data->encryptsecretCount; ++i) { + if (qemuMonitorAddObject(mon, &data->encryptsecretProps[i], + &data->encryptsecretAlias[i]) < 0) + return -1; + } return 0; } @@ -1524,6 +1531,7 @@ qemuBlockStorageSourceAttachRollback(qemuMonitor *mon, qemuBlockStorageSourceAttachData *data) { virErrorPtr orig_err; + size_t i; virErrorPreserveLast(&orig_err); @@ -1549,8 +1557,10 @@ qemuBlockStorageSourceAttachRollback(qemuMonitor *mon, if (data->authsecretAlias) ignore_value(qemuMonitorDelObject(mon, data->authsecretAlias, false)); - if (data->encryptsecretAlias) - ignore_value(qemuMonitorDelObject(mon, data->encryptsecretAlias, false)); + for (i = 0; i < data->encryptsecretCount; ++i) { + if (data->encryptsecretAlias[i]) + ignore_value(qemuMonitorDelObject(mon, data->encryptsecretAlias[i], false)); + } if (data->httpcookiesecretAlias) ignore_value(qemuMonitorDelObject(mon, data->httpcookiesecretAlias, false)); @@ -1605,8 +1615,15 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src) if (srcpriv->secinfo) data->authsecretAlias = g_strdup(srcpriv->secinfo->alias); - if (srcpriv->encinfo) - data->encryptsecretAlias = g_strdup(srcpriv->encinfo->alias); + if (srcpriv->encinfo) { + if (!data->encryptsecretAlias) { + data->encryptsecretCount = 1; + data->encryptsecretProps = g_new0(virJSONValue *, 1); + data->encryptsecretAlias = g_new0(char *, 1); + } + + data->encryptsecretAlias[0] = g_strdup(srcpriv->encinfo->alias); + } if (srcpriv->httpcookie) data->httpcookiesecretAlias = g_strdup(srcpriv->httpcookie->alias); diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 5a61a19da2..530d88d28e 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -89,8 +89,9 @@ struct qemuBlockStorageSourceAttachData { virJSONValue *authsecretProps; char *authsecretAlias; - virJSONValue *encryptsecretProps; - char *encryptsecretAlias; + size_t encryptsecretCount; + virJSONValue **encryptsecretProps; + char **encryptsecretAlias; virJSONValue *httpcookiesecretProps; char *httpcookiesecretAlias; diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a20cf1db62..818e90022c 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1336,9 +1336,15 @@ qemuBlockJobProcessEventConcludedCreate(virQEMUDriver *driver, /* the format node part was not attached yet, so we don't need to detach it */ backend->formatAttached = false; if (job->data.create.storage) { + size_t i; + backend->storageAttached = false; backend->storageSliceAttached = false; + for (i = 0; i < backend->encryptsecretCount; ++i) { + VIR_FREE(backend->encryptsecretAlias[i]); + } VIR_FREE(backend->encryptsecretAlias); + VIR_FREE(backend->encryptsecretProps); } if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4839d45a34..f5dcb46e42 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2108,15 +2108,21 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd, virQEMUCaps *qemuCaps) { char *tmp; + size_t i; if (qemuBuildObjectCommandline(cmd, data->prmgrProps, qemuCaps) < 0 || qemuBuildObjectCommandline(cmd, data->authsecretProps, qemuCaps) < 0 || - qemuBuildObjectCommandline(cmd, data->encryptsecretProps, qemuCaps) < 0 || qemuBuildObjectCommandline(cmd, data->httpcookiesecretProps, qemuCaps) < 0 || qemuBuildObjectCommandline(cmd, data->tlsKeySecretProps, qemuCaps) < 0 || qemuBuildObjectCommandline(cmd, data->tlsProps, qemuCaps) < 0) return -1; + for (i = 0; i < data->encryptsecretCount; ++i) { + if (qemuBuildObjectCommandline(cmd, data->encryptsecretProps[i], qemuCaps) < 0) { + return -1; + } + } + if (data->driveCmd) virCommandAddArgList(cmd, "-drive", data->driveCmd, NULL); @@ -10637,9 +10643,16 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src, qemuBuildSecretInfoProps(srcpriv->secinfo, &data->authsecretProps) < 0) return -1; - if (srcpriv->encinfo && - qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps) < 0) - return -1; + if (srcpriv->encinfo) { + if (!data->encryptsecretProps) { + data->encryptsecretCount = 1; + data->encryptsecretProps = g_new0(virJSONValue *, 1); + data->encryptsecretAlias = g_new0(char *, 1); + } + + if (qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps[0]) < 0) + return -1; + } if (srcpriv->httpcookie && qemuBuildSecretInfoProps(srcpriv->httpcookie, &data->httpcookiesecretProps) < 0) -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:09 -0600, Or Ozeri wrote:
This commit changes the qemuBlockStorageSourceAttachData struct to support multiple secrets (instead of a single one before this commit). This will useful for storage encryption requiring more than a single secret.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_block.c | 35 ++++++++++++++++++++++++++--------- src/qemu/qemu_block.h | 5 +++-- src/qemu/qemu_blockjob.c | 6 ++++++ src/qemu/qemu_command.c | 21 +++++++++++++++++---- 4 files changed, 52 insertions(+), 15 deletions(-)
[...]
@@ -1605,8 +1615,15 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src) if (srcpriv->secinfo) data->authsecretAlias = g_strdup(srcpriv->secinfo->alias);
- if (srcpriv->encinfo) - data->encryptsecretAlias = g_strdup(srcpriv->encinfo->alias); + if (srcpriv->encinfo) { + if (!data->encryptsecretAlias) {
If this were non-NULL ...
+ data->encryptsecretCount = 1; + data->encryptsecretProps = g_new0(virJSONValue *, 1); + data->encryptsecretAlias = g_new0(char *, 1); + } + + data->encryptsecretAlias[0] = g_strdup(srcpriv->encinfo->alias);
... then it's very likely that this was already filled, and you'd overwrite it. Given that qemuBlockStorageSourceDetachPrepare freshly allocates the 'data' object the above is impossible and the if condition you've added is tautological and thus pointless. I'll remove it before pushing.
+ }
if (srcpriv->httpcookie) data->httpcookiesecretAlias = g_strdup(srcpriv->httpcookie->alias);
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4839d45a34..f5dcb46e42 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c
[...]
@@ -10637,9 +10643,16 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src, qemuBuildSecretInfoProps(srcpriv->secinfo, &data->authsecretProps) < 0) return -1;
- if (srcpriv->encinfo && - qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps) < 0) - return -1; + if (srcpriv->encinfo) { + if (!data->encryptsecretProps) { + data->encryptsecretCount = 1; + data->encryptsecretProps = g_new0(virJSONValue *, 1); + data->encryptsecretAlias = g_new0(char *, 1); + } + + if (qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps[0]) < 0) + return -1; + }
Same as above. Although it's not that easy to see that the condition is tautological as the object is allocated outside of the funciton. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This commit changes the _qemuDomainStorageSourcePrivate struct to support multiple secrets (instead of a single one before this commit). This will useful for storage encryption requiring more than a single secret. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_block.c | 22 +++++++----- src/qemu/qemu_command.c | 20 ++++++----- src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_domain.h | 3 +- tests/qemublocktest.c | 7 ++-- 5 files changed, 91 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 2e3e0f6572..f6d21d2040 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -581,7 +581,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, if (virJSONValueObjectAdd(&encrypt, "s:format", encformat, - "s:key-secret", srcPriv->encinfo->alias, + "s:key-secret", srcPriv->encinfo[0]->alias, NULL) < 0) return NULL; } @@ -978,7 +978,7 @@ qemuBlockStorageSourceGetFormatLUKSProps(virStorageSource *src, { qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); - if (!srcPriv || !srcPriv->encinfo || !srcPriv->encinfo->alias) { + if (!srcPriv || !srcPriv->encinfo || !srcPriv->encinfo[0]->alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing secret info for 'luks' driver")); return -1; @@ -986,7 +986,7 @@ qemuBlockStorageSourceGetFormatLUKSProps(virStorageSource *src, if (virJSONValueObjectAdd(&props, "s:driver", "luks", - "s:key-secret", srcPriv->encinfo->alias, + "s:key-secret", srcPriv->encinfo[0]->alias, NULL) < 0) return -1; @@ -1054,7 +1054,7 @@ qemuBlockStorageSourceGetCryptoProps(virStorageSource *src, return virJSONValueObjectAdd(encprops, "s:format", encformat, - "s:key-secret", srcpriv->encinfo->alias, + "s:key-secret", srcpriv->encinfo[0]->alias, NULL); } @@ -1616,13 +1616,17 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src) data->authsecretAlias = g_strdup(srcpriv->secinfo->alias); if (srcpriv->encinfo) { + size_t i; + if (!data->encryptsecretAlias) { - data->encryptsecretCount = 1; - data->encryptsecretProps = g_new0(virJSONValue *, 1); - data->encryptsecretAlias = g_new0(char *, 1); + data->encryptsecretCount = srcpriv->enccount; + data->encryptsecretProps = g_new0(virJSONValue *, srcpriv->enccount); + data->encryptsecretAlias = g_new0(char *, srcpriv->enccount); } - data->encryptsecretAlias[0] = g_strdup(srcpriv->encinfo->alias); + for (i = 0; i < srcpriv->enccount; ++i) { + data->encryptsecretAlias[i] = g_strdup(srcpriv->encinfo[i]->alias); + } } if (srcpriv->httpcookie) @@ -1987,7 +1991,7 @@ qemuBlockStorageSourceCreateGetEncryptionLUKS(virStorageSource *src, if (srcpriv && srcpriv->encinfo) - keysecret = srcpriv->encinfo->alias; + keysecret = srcpriv->encinfo[0]->alias; if (virJSONValueObjectAdd(&props, "s:key-secret", keysecret, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f5dcb46e42..69f0d74b92 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1603,7 +1603,7 @@ qemuBuildDriveSourceStr(virDomainDiskDef *disk, { virStorageType actualType = virStorageSourceGetActualType(disk->src); qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); - qemuDomainSecretInfo *encinfo = NULL; + qemuDomainSecretInfo **encinfo = NULL; g_autoptr(virJSONValue) srcprops = NULL; bool rawluks = false; @@ -1647,12 +1647,12 @@ qemuBuildDriveSourceStr(virDomainDiskDef *disk, if (encinfo) { if (disk->src->format == VIR_STORAGE_FILE_RAW) { - virBufferAsprintf(buf, "key-secret=%s,", encinfo->alias); + virBufferAsprintf(buf, "key-secret=%s,", encinfo[0]->alias); rawluks = true; } else if (disk->src->format == VIR_STORAGE_FILE_QCOW2 && disk->src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { virBufferAddLit(buf, "encrypt.format=luks,"); - virBufferAsprintf(buf, "encrypt.key-secret=%s,", encinfo->alias); + virBufferAsprintf(buf, "encrypt.key-secret=%s,", encinfo[0]->alias); } } @@ -10644,14 +10644,18 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src, return -1; if (srcpriv->encinfo) { + size_t i; + if (!data->encryptsecretProps) { - data->encryptsecretCount = 1; - data->encryptsecretProps = g_new0(virJSONValue *, 1); - data->encryptsecretAlias = g_new0(char *, 1); + data->encryptsecretCount = srcpriv->enccount; + data->encryptsecretProps = g_new0(virJSONValue *, srcpriv->enccount); + data->encryptsecretAlias = g_new0(char *, srcpriv->enccount); } - if (qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps[0]) < 0) - return -1; + for (i = 0; i < srcpriv->enccount; ++i) { + if (qemuBuildSecretInfoProps(srcpriv->encinfo[i], &data->encryptsecretProps[i]) < 0) + return -1; + } } if (srcpriv->httpcookie && diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 80c9852dae..a3b9b57cfa 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -872,7 +872,13 @@ qemuDomainStorageSourcePrivateDispose(void *obj) qemuDomainStorageSourcePrivate *priv = obj; g_clear_pointer(&priv->secinfo, qemuDomainSecretInfoFree); - g_clear_pointer(&priv->encinfo, qemuDomainSecretInfoFree); + if (priv->encinfo) { + size_t i; + for (i = 0; i < priv->enccount; ++i) { + g_clear_pointer(&priv->encinfo[i], qemuDomainSecretInfoFree); + } + priv->encinfo = NULL; + } g_clear_pointer(&priv->httpcookie, qemuDomainSecretInfoFree); g_clear_pointer(&priv->tlsKeySecret, qemuDomainSecretInfoFree); g_clear_pointer(&priv->fdpass, qemuFDPassFree); @@ -1401,7 +1407,13 @@ qemuDomainSecretDiskDestroy(virDomainDiskDef *disk) for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { if ((srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(n))) { qemuDomainSecretInfoDestroy(srcPriv->secinfo); - qemuDomainSecretInfoDestroy(srcPriv->encinfo); + if (srcPriv->encinfo) { + size_t i; + + for (i = 0; i < srcPriv->enccount; ++i) { + qemuDomainSecretInfoDestroy(srcPriv->encinfo[i]); + } + } qemuDomainSecretInfoDestroy(srcPriv->tlsKeySecret); } } @@ -1470,12 +1482,14 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, } if (hasEnc) { - if (!(srcPriv->encinfo = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, - "encryption", 0, - VIR_SECRET_USAGE_TYPE_VOLUME, - NULL, - &src->encryption->secrets[0]->seclookupdef))) - return -1; + srcPriv->enccount = 1; + srcPriv->encinfo = g_new0(qemuDomainSecretInfo *, 1); + if (!(srcPriv->encinfo[0] = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, + "encryption", 0, + VIR_SECRET_USAGE_TYPE_VOLUME, + NULL, + &src->encryption->secrets[0]->seclookupdef))) + return -1; } if (src->ncookies && @@ -1964,13 +1978,14 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, virStorageSource *src) { qemuDomainStorageSourcePrivate *priv; + g_autofree xmlNodePtr *encnodes = NULL; g_autofree char *authalias = NULL; - g_autofree char *encalias = NULL; g_autofree char *httpcookiealias = NULL; g_autofree char *tlskeyalias = NULL; g_autofree char *thresholdEventWithIndex = NULL; bool fdsetPresent = false; unsigned int fdSetID; + int enccount; src->nodestorage = virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt); src->nodeformat = virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt); @@ -1983,13 +1998,16 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, src->pr->mgralias = virXPathString("string(./reservations/@mgralias)", ctxt); authalias = virXPathString("string(./objects/secret[@type='auth']/@alias)", ctxt); - encalias = virXPathString("string(./objects/secret[@type='encryption']/@alias)", ctxt); + if ((enccount = virXPathNodeSet("./objects/secret[@type='encryption']", ctxt, &encnodes)) < 0) + return -1; httpcookiealias = virXPathString("string(./objects/secret[@type='httpcookie']/@alias)", ctxt); tlskeyalias = virXPathString("string(./objects/secret[@type='tlskey']/@alias)", ctxt); fdsetPresent = virXPathUInt("string(./fdsets/fdset[@type='storage']/@id)", ctxt, &fdSetID) == 0; - if (authalias || encalias || httpcookiealias || tlskeyalias || fdsetPresent) { + if (authalias || (enccount > 0) || httpcookiealias || tlskeyalias || fdsetPresent) { + size_t i; + if (!src->privateData && !(src->privateData = qemuDomainStorageSourcePrivateNew())) return -1; @@ -1999,8 +2017,27 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->secinfo, &authalias) < 0) return -1; - if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->encinfo, &encalias) < 0) - return -1; + if (enccount > 0) { + xmlNodePtr tmp = ctxt->node; + + priv->enccount = enccount; + priv->encinfo = g_new0(qemuDomainSecretInfo *, enccount); + for (i = 0; i < enccount; ++i) { + g_autofree char *encalias = NULL; + + ctxt->node = encnodes[i]; + if (!(encalias = virXMLPropString(encnodes[i], "alias"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing alias on encryption secret #%lu"), i); + return -1; + } + + if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->encinfo[i], &encalias) < 0) + return -1; + } + + ctxt->node = tmp; + } if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->httpcookie, &httpcookiealias) < 0) return -1; @@ -2061,10 +2098,13 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src, return -1; if (srcPriv) { + size_t i; unsigned int fdSetID; qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->secinfo, "auth"); - qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->encinfo, "encryption"); + for (i = 0; i < srcPriv->enccount; ++i) { + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->encinfo[i], "encryption"); + } qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->httpcookie, "httpcookie"); qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->tlsKeySecret, "tlskey"); @@ -5631,9 +5671,14 @@ qemuDomainDeviceDiskDefPostParseRestoreSecAlias(virDomainDiskDef *disk, } if (restoreEncSecret) { + if (!priv->encinfo) { + priv->enccount = 1; + priv->encinfo = g_new0(qemuDomainSecretInfo *, 1); + } + encalias = g_strdup_printf("%s-luks-secret0", disk->info.alias); - if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->encinfo, &encalias) < 0) + if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->encinfo[0], &encalias) < 0) return -1; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 550397ee50..dda92b5da3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -295,7 +295,8 @@ struct _qemuDomainStorageSourcePrivate { qemuDomainSecretInfo *secinfo; /* data required for decryption of encrypted storage source */ - qemuDomainSecretInfo *encinfo; + size_t enccount; + qemuDomainSecretInfo **encinfo; /* secure passthrough of the http cookie */ qemuDomainSecretInfo *httpcookie; diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 010b52f4b3..2d790e2b2e 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -237,10 +237,11 @@ testQemuDiskXMLToJSONFakeSecrets(virStorageSource *src) } if (src->encryption) { - srcpriv->encinfo = g_new0(qemuDomainSecretInfo, 1); + srcpriv->encinfo = g_new0(qemuDomainSecretInfo *, 1); + srcpriv->encinfo[0] = g_new0(qemuDomainSecretInfo, 1); - srcpriv->encinfo->alias = g_strdup_printf("%s-encalias", - NULLSTR(src->nodeformat)); + srcpriv->encinfo[0]->alias = g_strdup_printf("%s-encalias", + NULLSTR(src->nodeformat)); } return 0; -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:10 -0600, Or Ozeri wrote:
This commit changes the _qemuDomainStorageSourcePrivate struct to support multiple secrets (instead of a single one before this commit). This will useful for storage encryption requiring more than a single secret.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_block.c | 22 +++++++----- src/qemu/qemu_command.c | 20 ++++++----- src/qemu/qemu_domain.c | 75 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_domain.h | 3 +- tests/qemublocktest.c | 7 ++-- 5 files changed, 91 insertions(+), 36 deletions(-)
[...]
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 2e3e0f6572..f6d21d2040 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -581,7 +581,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src,
if (virJSONValueObjectAdd(&encrypt, "s:format", encformat, - "s:key-secret", srcPriv->encinfo->alias, + "s:key-secret", srcPriv->encinfo[0]->alias, NULL) < 0) return NULL; }
So, will this be changed in an upcomming commit?
@@ -978,7 +978,7 @@ qemuBlockStorageSourceGetFormatLUKSProps(virStorageSource *src, { qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
Add a comment here stating that the encryption engine in qemu currently accepts just one secret and that the validation ensures that.
- if (!srcPriv || !srcPriv->encinfo || !srcPriv->encinfo->alias) { + if (!srcPriv || !srcPriv->encinfo || !srcPriv->encinfo[0]->alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing secret info for 'luks' driver")); return -1; @@ -986,7 +986,7 @@ qemuBlockStorageSourceGetFormatLUKSProps(virStorageSource *src,
if (virJSONValueObjectAdd(&props, "s:driver", "luks", - "s:key-secret", srcPriv->encinfo->alias, + "s:key-secret", srcPriv->encinfo[0]->alias, NULL) < 0) return -1;
@@ -1054,7 +1054,7 @@ qemuBlockStorageSourceGetCryptoProps(virStorageSource *src,
return virJSONValueObjectAdd(encprops, "s:format", encformat, - "s:key-secret", srcpriv->encinfo->alias, + "s:key-secret", srcpriv->encinfo[0]->alias, NULL); }
... so that's clear why we don't bother with looking at other members of the array in these two.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f5dcb46e42..69f0d74b92 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c
[...]
@@ -1647,12 +1647,12 @@ qemuBuildDriveSourceStr(virDomainDiskDef *disk,
if (encinfo) { if (disk->src->format == VIR_STORAGE_FILE_RAW) { - virBufferAsprintf(buf, "key-secret=%s,", encinfo->alias); + virBufferAsprintf(buf, "key-secret=%s,", encinfo[0]->alias); rawluks = true; } else if (disk->src->format == VIR_STORAGE_FILE_QCOW2 && disk->src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { virBufferAddLit(buf, "encrypt.format=luks,"); - virBufferAsprintf(buf, "encrypt.key-secret=%s,", encinfo->alias); + virBufferAsprintf(buf, "encrypt.key-secret=%s,", encinfo[0]->alias);
I'm okay that we don't bother about this case here, but you then must add a validation check which forbids multiple secrets with SD-card disks, as that frontend still uses this code path. [...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 80c9852dae..a3b9b57cfa 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -872,7 +872,13 @@ qemuDomainStorageSourcePrivateDispose(void *obj) qemuDomainStorageSourcePrivate *priv = obj;
g_clear_pointer(&priv->secinfo, qemuDomainSecretInfoFree); - g_clear_pointer(&priv->encinfo, qemuDomainSecretInfoFree); + if (priv->encinfo) { + size_t i; + for (i = 0; i < priv->enccount; ++i) { + g_clear_pointer(&priv->encinfo[i], qemuDomainSecretInfoFree); + } + priv->encinfo = NULL;
This leaks the 'encinfo' pointer array.
+ } g_clear_pointer(&priv->httpcookie, qemuDomainSecretInfoFree); g_clear_pointer(&priv->tlsKeySecret, qemuDomainSecretInfoFree); g_clear_pointer(&priv->fdpass, qemuFDPassFree);a
@@ -1470,12 +1482,14 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, }
if (hasEnc) { - if (!(srcPriv->encinfo = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, - "encryption", 0, - VIR_SECRET_USAGE_TYPE_VOLUME, - NULL, - &src->encryption->secrets[0]->seclookupdef))) - return -1; + srcPriv->enccount = 1; + srcPriv->encinfo = g_new0(qemuDomainSecretInfo *, 1); + if (!(srcPriv->encinfo[0] = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, + "encryption", 0, + VIR_SECRET_USAGE_TYPE_VOLUME, + NULL, + &src->encryption->secrets[0]->seclookupdef)))
Why don't you process multiple secrets here since struct _virStorageEncryption already has multiple secrets? [...]
@@ -1999,8 +2017,27 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->secinfo, &authalias) < 0) return -1;
- if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->encinfo, &encalias) < 0) - return -1; + if (enccount > 0) { + xmlNodePtr tmp = ctxt->node; + + priv->enccount = enccount; + priv->encinfo = g_new0(qemuDomainSecretInfo *, enccount); + for (i = 0; i < enccount; ++i) { + g_autofree char *encalias = NULL; + + ctxt->node = encnodes[i]; + if (!(encalias = virXMLPropString(encnodes[i], "alias"))) {
This does not use XPath .. so you don't need to modify 'ctxt->node' at all, including the 'tmp' variable. If you'd need to do it we have an automatic cleanup method fro that ....
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing alias on encryption secret #%lu"), i); + return -1;
... because e.g. this code path would not un-modify it.
+ } + + if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->encinfo[i], &encalias) < 0) + return -1; + } + + ctxt->node = tmp; + }
if (qemuStorageSourcePrivateDataAssignSecinfo(&priv->httpcookie, &httpcookiealias) < 0) return -1; @@ -2061,10 +2098,13 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src, return -1;
if (srcPriv) { + size_t i; unsigned int fdSetID;
qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->secinfo, "auth"); - qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->encinfo, "encryption"); + for (i = 0; i < srcPriv->enccount; ++i) { + qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->encinfo[i], "encryption"); + } qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->httpcookie, "httpcookie"); qemuStorageSourcePrivateDataFormatSecinfo(&objectsChildBuf, srcPriv->tlsKeySecret, "tlskey");
For the private data XML formatting and parsing I'd like to see a test scenario being added e.g. to tests/qemustatusxml2xmldata/modern-in.xml
This commit extends qemuDomainSecretStorageSourcePrepare to setup multiple qemu secrets as defined by virStorageSource->encryption. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_domain.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a3b9b57cfa..ffe29dc832 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1482,14 +1482,19 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, } if (hasEnc) { - srcPriv->enccount = 1; - srcPriv->encinfo = g_new0(qemuDomainSecretInfo *, 1); - if (!(srcPriv->encinfo[0] = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, - "encryption", 0, - VIR_SECRET_USAGE_TYPE_VOLUME, - NULL, - &src->encryption->secrets[0]->seclookupdef))) - return -1; + size_t nsecrets = src->encryption->nsecrets; + size_t i; + + srcPriv->enccount = nsecrets; + srcPriv->encinfo = g_new0(qemuDomainSecretInfo *, nsecrets); + for (i = 0; i < nsecrets; ++i) { + if (!(srcPriv->encinfo[i] = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, + "encryption", i, + VIR_SECRET_USAGE_TYPE_VOLUME, + NULL, + &src->encryption->secrets[i]->seclookupdef))) + return -1; + } } if (src->ncookies && -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:11 -0600, Or Ozeri wrote:
This commit extends qemuDomainSecretStorageSourcePrepare to setup multiple qemu secrets as defined by virStorageSource->encryption.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_domain.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a3b9b57cfa..ffe29dc832 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1482,14 +1482,19 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivate *priv, }
if (hasEnc) { - srcPriv->enccount = 1; - srcPriv->encinfo = g_new0(qemuDomainSecretInfo *, 1); - if (!(srcPriv->encinfo[0] = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, - "encryption", 0, - VIR_SECRET_USAGE_TYPE_VOLUME, - NULL, - &src->encryption->secrets[0]->seclookupdef))) - return -1; + size_t nsecrets = src->encryption->nsecrets; + size_t i; + + srcPriv->enccount = nsecrets; + srcPriv->encinfo = g_new0(qemuDomainSecretInfo *, nsecrets); + for (i = 0; i < nsecrets; ++i) { + if (!(srcPriv->encinfo[i] = qemuDomainSecretInfoSetupFromSecret(priv, aliasformat, + "encryption", i, + VIR_SECRET_USAGE_TYPE_VOLUME, + NULL, + &src->encryption->secrets[i]->seclookupdef))) + return -1; + }
Squash this into the previous commit. It will make more sense that way.
This commit enables libvirt users to use layered encryption of RBD images, using the librbd encryption engine. This allows opening of an encrypted cloned image whose parent is encrypted with a possibly different encryption key. To open such images, multiple encryption secrets are expected to be defined under the encryption XML tag. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- docs/formatstorageencryption.rst | 11 +++-- src/conf/schemas/storagecommon.rng | 4 +- src/qemu/qemu_block.c | 23 +++++++--- src/qemu/qemu_domain.c | 14 ++++++ ...k-rbd-encryption-layering.x86_64-7.2.0.err | 1 + ...rbd-encryption-layering.x86_64-latest.args | 39 ++++++++++++++++ .../disk-network-rbd-encryption-layering.xml | 40 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 + ...-rbd-encryption-layering.x86_64-latest.xml | 45 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 10 files changed, 169 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-7.2.0.err create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml create mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-encryption-layering.x86_64-latest.xml diff --git a/docs/formatstorageencryption.rst b/docs/formatstorageencryption.rst index 2c19473d6b..3b3e9ea379 100644 --- a/docs/formatstorageencryption.rst +++ b/docs/formatstorageencryption.rst @@ -28,7 +28,10 @@ network disks. If the engine tag is not specified, the ``qemu`` engine will be used by default (assuming the qemu driver is used). Note that ``librbd`` engine is currently only supported by the qemu VM driver, and is not supported by the storage driver. Furthermore, the storage driver currently ignores the ``engine`` -tag. +tag. :since:`since 9.3.0` RBD layered encryption is supported. Layered +encryption requires a secret per each encrypted layer. The first secret +corresponds to the (child) image itself, the second secret to the parent image, +and so forth. The ``encryption`` tag can currently contain a sequence of ``secret`` tags, each with mandatory attributes ``type`` and either ``uuid`` or ``usage`` ( @@ -55,7 +58,8 @@ added to libvirt. The ``luks`` format is specific to a luks encrypted volume and the secret is used in order to either encrypt during volume creation or decrypt the volume for usage by the domain. A single ``<secret type='passphrase'...>`` element is -expected. :since:`Since 2.1.0` . +expected (except for the case of RBD layered encryption mentioned above). +:since:`Since 2.1.0` . 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 @@ -102,7 +106,8 @@ can only be applied to RBD network disks (RBD images). Since the ``librbd`` engine is currently not supported by the libvirt storage driver, you cannot use it to control such disks. However, pre-formatted RBD luks2 disks can be loaded to a qemu VM using the qemu VM driver. A single -``<secret type='passphrase'...>`` element is expected. +``<secret type='passphrase'...>`` element is expected (except for the case of +RBD layered encryption mentioned above). Examples -------- diff --git a/src/conf/schemas/storagecommon.rng b/src/conf/schemas/storagecommon.rng index 4d6e646c9a..ff24ae9548 100644 --- a/src/conf/schemas/storagecommon.rng +++ b/src/conf/schemas/storagecommon.rng @@ -26,7 +26,9 @@ </optional> <optional> <interleave> - <ref name="secret"/> + <oneOrMore> + <ref name="secret"/> + </oneOrMore> <optional> <interleave> <element name="cipher"> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index f6d21d2040..e0f355874f 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -543,7 +543,8 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); g_autoptr(virJSONValue) servers = NULL; virJSONValue *ret = NULL; - g_autoptr(virJSONValue) encrypt = NULL; + g_autolist(virJSONValue) encrypts = NULL; + virJSONValue *null_encrypt = NULL; const char *encformat = NULL; const char *username = NULL; g_autoptr(virJSONValue) authmodes = NULL; @@ -563,6 +564,8 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, if (src->encryption && src->encryption->engine == VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD) { + size_t i; + switch ((virStorageEncryptionFormatType) src->encryption->format) { case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS: encformat = "luks"; @@ -579,11 +582,17 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, break; } - if (virJSONValueObjectAdd(&encrypt, - "s:format", encformat, - "s:key-secret", srcPriv->encinfo[0]->alias, - NULL) < 0) - return NULL; + for (i = src->encryption->nsecrets; i > 0; --i) { + virJSONValue *encrypt = NULL; + if (virJSONValueObjectAdd(&encrypt, + "s:format", encformat, + "s:key-secret", srcPriv->encinfo[i-1]->alias, + "A:parent", encrypts ? (virJSONValue **)&encrypts->data : &null_encrypt, + NULL) < 0) + return NULL; + + encrypts = g_list_prepend(encrypts, encrypt); + } } if (virJSONValueObjectAdd(&ret, @@ -592,7 +601,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, "S:snapshot", src->snapshot, "S:conf", src->configFile, "A:server", &servers, - "A:encrypt", &encrypt, + "A:encrypt", encrypts ? (virJSONValue **)&encrypts->data : &null_encrypt, "S:user", username, "A:auth-client-required", &authmodes, "S:key-secret", keysecret, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ffe29dc832..e336273588 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5195,6 +5195,12 @@ qemuDomainValidateStorageSource(virStorageSource *src, return -1; } + if (src->encryption->nsecrets > 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("qemu encryption engine expects only a single secret")); + return -1; + } + break; case VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD: @@ -5210,6 +5216,14 @@ qemuDomainValidateStorageSource(virStorageSource *src, _("librbd encryption is supported only with RBD backed disks")); return -1; } + + if (src->encryption->nsecrets > 1) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_ENCRYPTION_LAYERING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("librbd encryption layering is not supported by this QEMU binary")); + return -1; + } + } break; case VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT: diff --git a/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-7.2.0.err b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-7.2.0.err new file mode 100644 index 0000000000..73e5b2a1f3 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-7.2.0.err @@ -0,0 +1 @@ +unsupported configuration: librbd encryption layering is not supported by this QEMU binary diff --git a/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-latest.args b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-latest.args new file mode 100644 index 0000000000..c260eda5e8 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-latest.args @@ -0,0 +1,39 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-encryptdisk \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-encryptdisk/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-encryptdisk/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-encryptdisk/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=encryptdisk,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-encryptdisk/master-key.aes"}' \ +-machine pc-i440fx-2.1,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-accel tcg \ +-cpu qemu64 \ +-m 1024 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 496898a6-e6ff-f7c8-5dc2-3cf410945ee9 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-object '{"qom-type":"secret","id":"libvirt-1-format-encryption-secret0","data":"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1","keyid":"masterKey0","iv":"AAECAwQFBgcICQoLDA0ODw==","format":"base64"}' \ +-object '{"qom-type":"secret","id":"libvirt-1-format-encryption-secret1","data":"9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1","keyid":"masterKey0","iv":"AAECAwQFBgcICQoLDA0ODw==","format":"base64"}' \ +-blockdev '{"driver":"rbd","pool":"pool","image":"image","server":[{"host":"mon1.example.org","port":"6321"},{"host":"mon2.example.org","port":"6322"},{"host":"mon3.example.org","port":"6322"}],"encrypt":{"format":"luks","key-secret":"libvirt-1-format-encryption-secret0","parent":{"format":"luks","key-secret":"libvirt-1-format-encryption-secret1"}},"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x2","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x3"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml new file mode 100644 index 0000000000..cbde665958 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml @@ -0,0 +1,40 @@ +<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-system-x86_64</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <encryption format='luks' engine='librbd'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80fb0'/> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vda' bus='virtio'/> + </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/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f46fc29f32..08c912f588 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1277,6 +1277,8 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-network-rbd"); DO_TEST_CAPS_VER_PARSE_ERROR("disk-network-rbd-encryption", "6.0.0"); DO_TEST_CAPS_LATEST("disk-network-rbd-encryption"); + DO_TEST_CAPS_VER_PARSE_ERROR("disk-network-rbd-encryption-layering", "7.2.0"); + DO_TEST_CAPS_LATEST("disk-network-rbd-encryption-layering"); DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-encryption-wrong"); DO_TEST_CAPS_LATEST("disk-network-rbd-no-colon"); /* qemu-6.0 is the last qemu version supporting sheepdog */ diff --git a/tests/qemuxml2xmloutdata/disk-network-rbd-encryption-layering.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-network-rbd-encryption-layering.x86_64-latest.xml new file mode 100644 index 0000000000..03c07c2527 --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-network-rbd-encryption-layering.x86_64-latest.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> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <encryption format='luks' engine='librbd'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80fb0'/> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 69bff80376..95a2261b7d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -329,6 +329,7 @@ mymain(void) DO_TEST_NOCAPS("disk-network-gluster"); DO_TEST_NOCAPS("disk-network-rbd"); DO_TEST_CAPS_LATEST("disk-network-rbd-encryption"); + DO_TEST_CAPS_LATEST("disk-network-rbd-encryption-layering"); DO_TEST_NOCAPS("disk-network-source-auth"); DO_TEST_NOCAPS("disk-network-sheepdog"); DO_TEST_NOCAPS("disk-network-vxhs"); -- 2.25.1
On Mon, Mar 06, 2023 at 06:53:12 -0600, Or Ozeri wrote:
This commit enables libvirt users to use layered encryption of RBD images, using the librbd encryption engine. This allows opening of an encrypted cloned image whose parent is encrypted with a possibly different encryption key. To open such images, multiple encryption secrets are expected to be defined under the encryption XML tag.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- docs/formatstorageencryption.rst | 11 +++-- src/conf/schemas/storagecommon.rng | 4 +- src/qemu/qemu_block.c | 23 +++++++--- src/qemu/qemu_domain.c | 14 ++++++ ...k-rbd-encryption-layering.x86_64-7.2.0.err | 1 + ...rbd-encryption-layering.x86_64-latest.args | 39 ++++++++++++++++ .../disk-network-rbd-encryption-layering.xml | 40 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 + ...-rbd-encryption-layering.x86_64-latest.xml | 45 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 10 files changed, 169 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-7.2.0.err create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml create mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-encryption-layering.x86_64-latest.xml
[...]
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index f6d21d2040..e0f355874f 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -543,7 +543,8 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); g_autoptr(virJSONValue) servers = NULL; virJSONValue *ret = NULL; - g_autoptr(virJSONValue) encrypt = NULL; + g_autolist(virJSONValue) encrypts = NULL; + virJSONValue *null_encrypt = NULL; const char *encformat = NULL; const char *username = NULL; g_autoptr(virJSONValue) authmodes = NULL;
[...]
@@ -579,11 +582,17 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, break; }
- if (virJSONValueObjectAdd(&encrypt, - "s:format", encformat, - "s:key-secret", srcPriv->encinfo[0]->alias, - NULL) < 0) - return NULL; + for (i = src->encryption->nsecrets; i > 0; --i) { + virJSONValue *encrypt = NULL; + if (virJSONValueObjectAdd(&encrypt, + "s:format", encformat, + "s:key-secret", srcPriv->encinfo[i-1]->alias, + "A:parent", encrypts ? (virJSONValue **)&encrypts->data : &null_encrypt, + NULL) < 0) + return NULL; + + encrypts = g_list_prepend(encrypts, encrypt); + } }
if (virJSONValueObjectAdd(&ret, @@ -592,7 +601,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src, "S:snapshot", src->snapshot, "S:conf", src->configFile, "A:server", &servers, - "A:encrypt", &encrypt, + "A:encrypt", encrypts ? (virJSONValue **)&encrypts->data : &null_encrypt, "S:user", username, "A:auth-client-required", &authmodes, "S:key-secret", keysecret,
The use of the 'list' makes the code very opaque and confusing. You declare the list as an automatic list, thus the members will be freed, but the 'A:' formatter of virJSONValueObjectAdd actually consumes the pointer. So you basically are creating a list, where each member in the list is empty except for the top, as it was consumed by virJSONValueObjectAdd. That means you don't even need a list and it will make the code also more clear: You only need this: for (i = src->encryption->nsecrets; i > 0; --i) { g_autoptr(virJSONValue) new = NULL; /* we consume the lower layer 'encrypt' into a new object */ if (virJSONValueObjectAdd(&new, "s:format", encformat, "s:key-secret", srcPriv->encinfo[i-1]->alias, "A:parent", &encrypt, NULL) < 0) return NULL; encrypt = g_steal_pointer(&new); } } You can drop all the other hunks in the function. (except for adding size_t i).
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ffe29dc832..e336273588 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -5210,6 +5216,14 @@ qemuDomainValidateStorageSource(virStorageSource *src, _("librbd encryption is supported only with RBD backed disks")); return -1; } + + if (src->encryption->nsecrets > 1) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_ENCRYPTION_LAYERING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("librbd encryption layering is not supported by this QEMU binary")); + return -1; + }
As noted in previous patch you must here validate that also the disk is not an SD card.
+ } break;
case VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT:
[...]
diff --git a/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml new file mode 100644 index 0000000000..cbde665958 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-network-rbd-encryption-layering.xml @@ -0,0 +1,40 @@ +<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-system-x86_64</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <encryption format='luks' engine='librbd'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80fb0'/> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
Since the first secret is used for the current image and subsequent ones for the parents it results in config: "encrypt":{"format":"luks", "key-secret":"libvirt-1-format-encryption-secret0", "parent": { "format":"luks", "key-secret":"libvirt-1-format-encryption-secret1" }} You thus want to use at least 3 secrets to show the multiple layers the documentation talks about
+ </encryption> + </source> + <target dev='vda' bus='virtio'/> + </disk>
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Friday, 10 March 2023 11:47 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com; idryomov@gmail.com; Danny Harnik <DANNYH@il.ibm.com> Subject: [EXTERNAL] Re: [PATCH v1 7/7] qemu: add support for librbd layered encryption
@@ -5210,6 +5216,14 @@ qemuDomainValidateStorageSource(virStorageSource *src, _("librbd encryption is supported only with RBD backed disks")); return -1; } + + if (src->encryption->nsecrets > 1) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_ENCRYPTION_LAYERING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("librbd encryption layering is not supported by this QEMU binary")); + return -1; + }
As noted in previous patch you must here validate that also the disk is not an SD card.
I tried searching the code to understand these questions: 1. How to tell that a disk is an SD card? 2. Why should using multiple secrets be prevented on an SD card disk? And why is a single secret OK? I could not find an answer to question 2. But I count on your expertise so we can ignore this question. The first question however must be answered in order to implement the check you talked about. My guess is the answer is (disk->bus == VIR_DOMAIN_DISK_BUS_SD). Is this correct? But then, you said the check is to be placed inside qemuDomainValidateStorageSource, which has the virStorageSource, but not the parent virDomainDiskDef. Do you suggest to extend the signature of qemuDomainValidateStorageSource with an additional "bool isSdDisk"?
On Sun, Mar 12, 2023 at 11:46:36 +0000, Or Ozeri wrote:
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Friday, 10 March 2023 11:47 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com; idryomov@gmail.com; Danny Harnik <DANNYH@il.ibm.com> Subject: [EXTERNAL] Re: [PATCH v1 7/7] qemu: add support for librbd layered encryption
@@ -5210,6 +5216,14 @@ qemuDomainValidateStorageSource(virStorageSource *src, _("librbd encryption is supported only with RBD backed disks")); return -1; } + + if (src->encryption->nsecrets > 1) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_ENCRYPTION_LAYERING)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("librbd encryption layering is not supported by this QEMU binary")); + return -1; + }
As noted in previous patch you must here validate that also the disk is not an SD card.
I tried searching the code to understand these questions: 1. How to tell that a disk is an SD card?
As you've noted below: disk->bus == VIR_DOMAIN_DISK_BUS_SD
2. Why should using multiple secrets be prevented on an SD card disk? And why is a single secret OK?
I mentioned this in my reply of 5/7. You are modifying qemuBuildDriveSourceStr to ignore the additional secrets. The function is used to format the disk the old way via '-drive' and this function is at this point used only for SD cards. I don't think it's viable to add support for the nested secrets there, thus the need to report
I could not find an answer to question 2. But I count on your expertise so we can ignore this question. The first question however must be answered in order to implement the check you talked about. My guess is the answer is (disk->bus == VIR_DOMAIN_DISK_BUS_SD). Is this correct?
Yup.
But then, you said the check is to be placed inside qemuDomainValidateStorageSource, which has the virStorageSource, but not the parent virDomainDiskDef. Do you suggest to extend the signature of qemuDomainValidateStorageSource with an additional "bool isSdDisk"?
Hmm, no I'd rather not pass it down to qemuDomainValidateStorageSource in the end. Specifically '-drive' does not accept any of the backingStore image data, so it requires only validating 'disk->src': This leaves us with 2 options. 1) Since this is a niche feature and SD cards are also mostly unused (except for some arm boards). I don't think it could hit too many users. Thus if librbd+qemu report an (sensible?) error if you don't pass all the required secrets to unlock the image we can defer this specific case to pass errors from qemu. This would mean no actual code is required. 2) If the misconfiguration leaves qemu in a broken state for some reason (e.g. the guest OS reading garbage data rather than an error being reported) then please re-implement the specific check in a place where the full disk definition is available.
participants (3)
-
Or Ozeri -
Or Ozeri
-
Peter Krempa