Re: [PATCH v1 5/7] qemu: add multi-secret support in _qemuDomainStorageSourcePrivate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux