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