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@xxxxxxxxxx>
> ---
> 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
