[libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}

John Ferlan posted 13 patches 8 years, 11 months ago
There is a newer version of this series
[libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
Posted by John Ferlan 8 years, 11 months ago
Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
used with a migrate or nbd TLS object

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++---------------------
 2 files changed, 124 insertions(+), 37 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index be44843..dd3cfd5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn,
 }
 
 
+/* qemuDomainSecretMigrateDestroy:
+ * @migSecinfo: Pointer to the secinfo from the incoming def
+ *
+ * Clear and destroy memory associated with the secret
+ */
+void
+qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo)
+{
+    if (!*migSecinfo)
+        return;
+
+    qemuDomainSecretInfoFree(migSecinfo);
+}
+
+
+/* qemuDomainSecretMigratePrepare
+ * @conn: Pointer to connection
+ * @priv: pointer to domain private object
+ * @srcAlias: Alias to use (either migrate or nbd)
+ * @secretUUID: UUID for the secret from the cfg (migrate or nbd)
+ *
+ * Create and prepare the qemuDomainSecretInfoPtr to be used for either
+ * a migration or nbd. Unlike other domain secret prepare functions, this
+ * is only expected to be called for a single object/instance. Theoretically
+ * the object could be reused, although that results in keeping a secret
+ * stored in memory for perhaps longer than expected or necessary.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+qemuDomainSecretMigratePrepare(virConnectPtr conn,
+                               qemuDomainObjPrivatePtr priv,
+                               const char *srcAlias,
+                               const char *secretUUID)
+{
+    virSecretLookupTypeDef seclookupdef = {0};
+    qemuDomainSecretInfoPtr secinfo = NULL;
+
+    if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("malformed %s TLS secret uuid in qemu.conf"),
+                       srcAlias);
+        return -1;
+    }
+    seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID;
+
+    if (VIR_ALLOC(secinfo) < 0)
+        return -1;
+
+    if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias,
+                              VIR_SECRET_USAGE_TYPE_TLS, NULL,
+                              &seclookupdef, false) < 0)
+        goto error;
+
+    if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("TLS X.509 requires encrypted secrets "
+                         "to be supported"));
+        goto error;
+    }
+    priv->migSecinfo = secinfo;
+
+    return 0;
+
+ error:
+    qemuDomainSecretInfoFree(&secinfo);
+    return -1;
+}
+
+
+
 /* qemuDomainSecretDestroy:
  * @vm: Domain object
  *
@@ -1634,6 +1705,8 @@ qemuDomainObjPrivateFree(void *data)
 
     VIR_FREE(priv->libDir);
     VIR_FREE(priv->channelTargetDir);
+
+    qemuDomainSecretMigrateDestroy(&priv->migSecinfo);
     qemuDomainMasterKeyFree(priv);
 
     VIR_FREE(priv);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 524a672..f796306 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -175,6 +175,43 @@ VIR_ENUM_DECL(qemuDomainNamespace)
 bool qemuDomainNamespaceEnabled(virDomainObjPtr vm,
                                 qemuDomainNamespace ns);
 
+/* Type of domain secret */
+typedef enum {
+    VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0,
+    VIR_DOMAIN_SECRET_INFO_TYPE_AES,  /* utilize GNUTLS_CIPHER_AES_256_CBC */
+
+    VIR_DOMAIN_SECRET_INFO_TYPE_LAST
+} qemuDomainSecretInfoType;
+
+typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain;
+typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr;
+struct _qemuDomainSecretPlain {
+    char *username;
+    uint8_t *secret;
+    size_t secretlen;
+};
+
+# define QEMU_DOMAIN_AES_IV_LEN 16   /* 16 bytes for 128 bit random */
+                                     /*    initialization vector */
+typedef struct _qemuDomainSecretAES qemuDomainSecretAES;
+typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr;
+struct _qemuDomainSecretAES {
+    char *username;
+    char *alias;      /* generated alias for secret */
+    char *iv;         /* base64 encoded initialization vector */
+    char *ciphertext; /* encoded/encrypted secret */
+};
+
+typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo;
+typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr;
+struct _qemuDomainSecretInfo {
+    qemuDomainSecretInfoType type;
+    union {
+        qemuDomainSecretPlain plain;
+        qemuDomainSecretAES aes;
+    } s;
+};
+
 typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
 typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
 struct _qemuDomainObjPrivate {
@@ -246,48 +283,15 @@ struct _qemuDomainObjPrivate {
 
     /* note whether memory device alias does not correspond to slot number */
     bool memAliasOrderMismatch;
+
+    /* for migration's using TLS with a secret (not to be saved in our */
+    /* private XML). */
+    qemuDomainSecretInfoPtr migSecinfo;
 };
 
 # define QEMU_DOMAIN_PRIVATE(vm)	\
     ((qemuDomainObjPrivatePtr) (vm)->privateData)
 
-/* Type of domain secret */
-typedef enum {
-    VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0,
-    VIR_DOMAIN_SECRET_INFO_TYPE_AES,  /* utilize GNUTLS_CIPHER_AES_256_CBC */
-
-    VIR_DOMAIN_SECRET_INFO_TYPE_LAST
-} qemuDomainSecretInfoType;
-
-typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain;
-typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr;
-struct _qemuDomainSecretPlain {
-    char *username;
-    uint8_t *secret;
-    size_t secretlen;
-};
-
-# define QEMU_DOMAIN_AES_IV_LEN 16   /* 16 bytes for 128 bit random */
-                                     /*    initialization vector */
-typedef struct _qemuDomainSecretAES qemuDomainSecretAES;
-typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr;
-struct _qemuDomainSecretAES {
-    char *username;
-    char *alias;      /* generated alias for secret */
-    char *iv;         /* base64 encoded initialization vector */
-    char *ciphertext; /* encoded/encrypted secret */
-};
-
-typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo;
-typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr;
-struct _qemuDomainSecretInfo {
-    qemuDomainSecretInfoType type;
-    union {
-        qemuDomainSecretPlain plain;
-        qemuDomainSecretAES aes;
-    } s;
-};
-
 # define QEMU_DOMAIN_DISK_PRIVATE(disk)	\
     ((qemuDomainDiskPrivatePtr) (disk)->privateData)
 
@@ -763,6 +767,16 @@ int qemuDomainSecretChardevPrepare(virConnectPtr conn,
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
     ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
 
+void qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo)
+    ATTRIBUTE_NONNULL(1);
+
+int qemuDomainSecretMigratePrepare(virConnectPtr conn,
+                                   qemuDomainObjPrivatePtr priv,
+                                   const char *srcAlias,
+                                   const char *secretUUID)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+    ATTRIBUTE_NONNULL(4);
+
 void qemuDomainSecretDestroy(virDomainObjPtr vm)
     ATTRIBUTE_NONNULL(1);
 
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
Posted by Jiri Denemark 8 years, 11 months ago
On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
> Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
> used with a migrate or nbd TLS object
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++---------------------
>  2 files changed, 124 insertions(+), 37 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index be44843..dd3cfd5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn,
>  }
>  
>  
> +/* qemuDomainSecretMigrateDestroy:
> + * @migSecinfo: Pointer to the secinfo from the incoming def
> + *
> + * Clear and destroy memory associated with the secret
> + */
> +void
> +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo)
> +{
> +    if (!*migSecinfo)
> +        return;
> +
> +    qemuDomainSecretInfoFree(migSecinfo);
> +}

This is a useless wrapper, please drop it.

> +/* qemuDomainSecretMigratePrepare
> + * @conn: Pointer to connection
> + * @priv: pointer to domain private object
> + * @srcAlias: Alias to use (either migrate or nbd)
> + * @secretUUID: UUID for the secret from the cfg (migrate or nbd)
> + *
> + * Create and prepare the qemuDomainSecretInfoPtr to be used for either
> + * a migration or nbd. Unlike other domain secret prepare functions, this
> + * is only expected to be called for a single object/instance. Theoretically
> + * the object could be reused, although that results in keeping a secret
> + * stored in memory for perhaps longer than expected or necessary.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +qemuDomainSecretMigratePrepare(virConnectPtr conn,
> +                               qemuDomainObjPrivatePtr priv,
> +                               const char *srcAlias,
> +                               const char *secretUUID)
> +{
> +    virSecretLookupTypeDef seclookupdef = {0};
> +    qemuDomainSecretInfoPtr secinfo = NULL;
> +
> +    if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("malformed %s TLS secret uuid in qemu.conf"),

[1]

> +                       srcAlias);
> +        return -1;
> +    }
> +    seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID;
> +
> +    if (VIR_ALLOC(secinfo) < 0)
> +        return -1;
> +
> +    if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias,
> +                              VIR_SECRET_USAGE_TYPE_TLS, NULL,
> +                              &seclookupdef, false) < 0)
> +        goto error;
> +
> +    if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("TLS X.509 requires encrypted secrets "
> +                         "to be supported"));
> +        goto error;
> +    }
> +    priv->migSecinfo = secinfo;
> +
> +    return 0;
> +
> + error:
> +    qemuDomainSecretInfoFree(&secinfo);
> +    return -1;
> +}

Almost all lines in this functions were just copy-pasted from
qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can
just make it a function which lookups the secinfo and you can do the
rest in the caller.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
Posted by Jiri Denemark 8 years, 11 months ago
On Mon, Feb 20, 2017 at 16:43:54 +0100, Jiri Denemark wrote:
> On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
> > Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
> > used with a migrate or nbd TLS object
...
> > +qemuDomainSecretMigratePrepare(virConnectPtr conn,
> > +                               qemuDomainObjPrivatePtr priv,
> > +                               const char *srcAlias,
> > +                               const char *secretUUID)
> > +{
> > +    virSecretLookupTypeDef seclookupdef = {0};
> > +    qemuDomainSecretInfoPtr secinfo = NULL;
> > +
> > +    if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("malformed %s TLS secret uuid in qemu.conf"),
> 
> [1]

Oops, ignore this part of my reply :-)

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
Posted by John Ferlan 8 years, 11 months ago

On 02/20/2017 10:43 AM, Jiri Denemark wrote:
> On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
>> Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
>> used with a migrate or nbd TLS object
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++
>>  src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++---------------------
>>  2 files changed, 124 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index be44843..dd3cfd5 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn,
>>  }
>>  
>>  
>> +/* qemuDomainSecretMigrateDestroy:
>> + * @migSecinfo: Pointer to the secinfo from the incoming def
>> + *
>> + * Clear and destroy memory associated with the secret
>> + */
>> +void
>> +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo)
>> +{
>> +    if (!*migSecinfo)
>> +        return;
>> +
>> +    qemuDomainSecretInfoFree(migSecinfo);
>> +}
> 
> This is a useless wrapper, please drop it.
> 

Sure - it's like an automatic reaction - have allocation need free
function... Will use qemuDomainSecretInfoFree  instead...

It does get used later in patch 12 and come to think of it, probably
needs to be added to patch 13 in the cleanup of the Run.

>> +/* qemuDomainSecretMigratePrepare
>> + * @conn: Pointer to connection
>> + * @priv: pointer to domain private object
>> + * @srcAlias: Alias to use (either migrate or nbd)
>> + * @secretUUID: UUID for the secret from the cfg (migrate or nbd)
>> + *
>> + * Create and prepare the qemuDomainSecretInfoPtr to be used for either
>> + * a migration or nbd. Unlike other domain secret prepare functions, this
>> + * is only expected to be called for a single object/instance. Theoretically
>> + * the object could be reused, although that results in keeping a secret
>> + * stored in memory for perhaps longer than expected or necessary.
>> + *
>> + * Returns 0 on success, -1 on failure
>> + */
>> +int
>> +qemuDomainSecretMigratePrepare(virConnectPtr conn,
>> +                               qemuDomainObjPrivatePtr priv,
>> +                               const char *srcAlias,
>> +                               const char *secretUUID)
>> +{
>> +    virSecretLookupTypeDef seclookupdef = {0};
>> +    qemuDomainSecretInfoPtr secinfo = NULL;
>> +
>> +    if (virUUIDParse(secretUUID, seclookupdef.u.uuid) < 0) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("malformed %s TLS secret uuid in qemu.conf"),
> 
> [1]
> 
>> +                       srcAlias);
>> +        return -1;
>> +    }
>> +    seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID;
>> +
>> +    if (VIR_ALLOC(secinfo) < 0)
>> +        return -1;
>> +
>> +    if (qemuDomainSecretSetup(conn, priv, secinfo, srcAlias,
>> +                              VIR_SECRET_USAGE_TYPE_TLS, NULL,
>> +                              &seclookupdef, false) < 0)
>> +        goto error;
>> +
>> +    if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                       _("TLS X.509 requires encrypted secrets "
>> +                         "to be supported"));
>> +        goto error;
>> +    }
>> +    priv->migSecinfo = secinfo;
>> +
>> +    return 0;
>> +
>> + error:
>> +    qemuDomainSecretInfoFree(&secinfo);
>> +    return -1;
>> +}
> 
> Almost all lines in this functions were just copy-pasted from
> qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can
> just make it a function which lookups the secinfo and you can do the
> rest in the caller.
> 
> Jirka
> 

Sure. no problem... I'll create:

static qemuDomainSecretInfoPtr
qemuDomainSecretTLSObjectPrepare(virConnectPtr conn,
                                 qemuDomainObjPrivatePtr priv,
                                 const char *srcAlias,
                                 const char *secretUUID)


which grabs the guts and does the right thing... I'll also adjust the
UUIDParse error message to just be "malformed secret uuid '%s' in
qemu.conf" passing the secretUUID string for the error message


Tks -

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}
Posted by Jiri Denemark 8 years, 11 months ago
On Mon, Feb 20, 2017 at 15:14:21 -0500, John Ferlan wrote:
> 
> 
> On 02/20/2017 10:43 AM, Jiri Denemark wrote:
> > On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
> >> Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
> >> used with a migrate or nbd TLS object
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >>  src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++
> >>  src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++---------------------
> >>  2 files changed, 124 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >> index be44843..dd3cfd5 100644
> >> --- a/src/qemu/qemu_domain.c
> >> +++ b/src/qemu/qemu_domain.c
> >> @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn,
> >>  }
> >>  
> >>  
> >> +/* qemuDomainSecretMigrateDestroy:
> >> + * @migSecinfo: Pointer to the secinfo from the incoming def
> >> + *
> >> + * Clear and destroy memory associated with the secret
> >> + */
> >> +void
> >> +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo)
> >> +{
> >> +    if (!*migSecinfo)
> >> +        return;
> >> +
> >> +    qemuDomainSecretInfoFree(migSecinfo);
> >> +}
> > 
> > This is a useless wrapper, please drop it.
> > 
> 
> Sure - it's like an automatic reaction - have allocation need free
> function... Will use qemuDomainSecretInfoFree  instead...

It would make sense if migSecinfo was a new structure, but in this case
we would just end up with two functions usable for freeing a single
structure. Which is just asking for an inconsistent usage. And that's
what happened even in your series; sometimes qemuDomainSecretInfoFree is
used to free migSecinfo and sometime it's the new wrapper you call to do
the job.

> >> +int
> >> +qemuDomainSecretMigratePrepare(virConnectPtr conn,
> >> +                               qemuDomainObjPrivatePtr priv,
> >> +                               const char *srcAlias,
> >> +                               const char *secretUUID)
> > 
> > Almost all lines in this functions were just copy-pasted from
> > qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can
> > just make it a function which lookups the secinfo and you can do the
> > rest in the caller.
> 
> Sure. no problem... I'll create:
> 
> static qemuDomainSecretInfoPtr
> qemuDomainSecretTLSObjectPrepare(virConnectPtr conn,
>                                  qemuDomainObjPrivatePtr priv,
>                                  const char *srcAlias,
>                                  const char *secretUUID)

Great, but since the structure it creates is qemuDomainSecretInfo, can
you just call it qemuDomainSecretInfoNew or something similar to this?

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list