Re: [libvirt] [PATCH v3 6/7] qemu: Set up the migrate TLS objects for target

[Jiri Denemark]

On Wed, Mar 22, 2017 at 21:40:04 -0400, John Ferlan wrote:
> On 03/22/2017 12:26 PM, Jiri Denemark wrote:
> > On Fri, Mar 17, 2017 at 14:39:00 -0400, John Ferlan wrote:
...
> >> Since incoming migrations that don't reach the Finish stage will be
> >> killed in qemuProcessRecoverMigrationIn and the only purpose at that
> >> point would be to free memory, it's not necessary to set up any sort
> >> of recovery. Additionally, subsequent migrations will check if the
> >> migration parameters are set and adjust them appropriately if for
> >> some reason libvirtd restarts after setting the Finish marker, but
> >> before actually resetting the environment.
> > 
> > Sure, migrations will do that, but what about snapshots, saving a domain
> > to a file and similar stuff which internally uses migration too. Do we
> > properly reset the parameters for them too? I think we should delete the
> > objects and reset migration parameters in qemuProcessRecoverMigrationIn
> > anyway.
> 
> Why would someone use TLS for snapshots, saving domain to a fail, and
> other similar stuff?  What am I missing?  I don't mind adding the extra
> call. If the TLS parameters *only* get adjusted by libvirt during
> PrepareAny and Run, but both those phases cause reset of the migration
> back to the original source on failure *and* Cancel does the reset -
> then I'm missing the connection.

If libvirtd restarts after entering QEMU_MIGRATION_PHASE_FINISH3 but
before completely finishing the migration we need to decide whether to
keep the domain running. The decision is based on the domain state, we kill
the domain unless its state is already VIR_DOMAIN_RUNNING. But the TLS
parameters may still be set at that point. If we don't reset them when
libvirtd starts again they will still be set when someone tries to do a
snapshot (or similar thing which uses "migrate" QMP command). Thus QEMU
will try to use TLS, which will obviously fail. That's why we can't rely
on just PrepareAny/Run to clear them, we need to make sure they are
always cleared when migration finishes.

...
> >> @@ -246,47 +283,18 @@ struct _qemuDomainObjPrivate {
> >>  
> >>  /* note whether memory device alias does not correspond to slot 
> >> number */
> >>  bool memAliasOrderMismatch;
> >> -};
> >>  
> >> -# define QEMU_DOMAIN_PRIVATE(vm)  \
> >> -((qemuDomainObjPrivatePtr) (vm)->privateData)
> >> +/* for migrations using TLS with a secret (not to be saved in our */
> >> +/* private XML). */
> >> +qemuDomainSecretInfoPtr migSecinfo;
> >>  
> >> -/* 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;
> >> +/* Used when fetching/storing the current 'tls-creds' migration 
> >> setting */
> >> +/* (not to be saved in our private XML). */
> >> +char *migTLSAlias;
> > 
> > I'm not quite sure why we need to store the original value.
> > 
> 
> It determines whether or not TLS is possible...  It can ensure that when
> not using TLS for migration that we set the params to "" if they aren't
> already set that way.

Which is in fact only needed because the check is done in the Begin
phase while the parameters are set in the Perform phase. It could have
been just a bool flag, though. Not a big deal.

...
> >> +/* qemuMigrationCheckTLSCreds
> >> + * @driver: pointer to qemu driver
> >> + * @vm: domain object
> >> + * @asyncJob: migration job to join
> >> + *
> >> + * Query the migration parameters looking for the 'tls-creds' parameter.
> >> + * The parameter was initially supported in QEMU 2.7; however, there was
> >> + * no mechanism provided to clear the parameter. For QEMU 2.9, a change
> >> + * was made to allow setting the parameter to an empty string in order
> >> + * to clear. An additional change was made to initialize the parameter
> >> + * to the empty string. Although still not perfect since it's possible
> >> + * that a pre-2.9 release set the string to something and we should not
> > 
> > How would this happen? QEMU won't magically set it to something by
> > itself. And we don't need to care about someone messing up with the
> > monitor or command line manually.
> 
> Yes it does - 2.9-rc0 initializes it to "" while it was NULL beforehand.
> See QEMU commit '4af245dc3'.

We don't care about TLS migration with QEMU older than 2.9 since
tls-creds won't be listed in the reply from query-migrate-parameters.
All QEMU versions between 2.7 and 2.9 support tls-cred but the default
value disables the use of TLS. That's why any comment mentioning this in
the code is irrelevant. We just 

Re: [libvirt] [PATCH v3 6/7] qemu: Set up the migrate TLS objects for target

[John Ferlan]

On 03/22/2017 12:26 PM, Jiri Denemark wrote:
> On Fri, Mar 17, 2017 at 14:39:00 -0400, John Ferlan wrote:
>> If the migration flags indicate this migration will be using TLS,
>> then set up the destination during the prepare phase once the target
>> domain has been started to add the TLS objects to perform the migration.
>>
>> This will create at least an "-object tls-creds-x509,endpoint=server,..."
>> and potentially an "-object secret,..." to handle the passphrase response
> 
> Looks like a leftover from previous versions where the objects were not
> hotplugged on both sides of migration.
> 

Hmm. thought I adjusted this already... Must've been thinking about it...

>> to access the TLS credentials. The alias/id used for the TLS objects
>> will contain "libvirt_migrate" as a mechanism to signify that libvirt
>> started the migration on the target (reaping benefits possibly).
>>
>> Once the objects are created, the code will set the "tls-creds" and
>> "tls-hostname" migration parameters to signify usage of TLS.
>>
>> During the Finish phase we'll be sure to attempt to clear the
>> migration parameters and delete those objects (whether or not they
>> were created).
>>
>> Since incoming migrations that don't reach the Finish stage will be
>> killed in qemuProcessRecoverMigrationIn and the only purpose at that
>> point would be to free memory, it's not necessary to set up any sort
>> of recovery. Additionally, subsequent migrations will check if the
>> migration parameters are set and adjust them appropriately if for
>> some reason libvirtd restarts after setting the Finish marker, but
>> before actually resetting the environment.
> 
> Sure, migrations will do that, but what about snapshots, saving a domain
> to a file and similar stuff which internally uses migration too. Do we
> properly reset the parameters for them too? I think we should delete the
> objects and reset migration parameters in qemuProcessRecoverMigrationIn
> anyway.
> 

Why would someone use TLS for snapshots, saving domain to a fail, and
other similar stuff?  What am I missing?  I don't mind adding the extra
call. If the TLS parameters *only* get adjusted by libvirt during
PrepareAny and Run, but both those phases cause reset of the migration
back to the original source on failure *and* Cancel does the reset -
then I'm missing the connection.

>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_domain.c|   7 +-
>>  src/qemu/qemu_domain.h|  91 +++-
>>  src/qemu/qemu_migration.c | 344 
>> ++
>>  3 files changed, 403 insertions(+), 39 deletions(-)
> ...
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 1f266bf..1dd3b1c 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
> ...
>> @@ -246,47 +283,18 @@ struct _qemuDomainObjPrivate {
>>  
>>  /* note whether memory device alias does not correspond to slot number 
>> */
>>  bool memAliasOrderMismatch;
>> -};
>>  
>> -# define QEMU_DOMAIN_PRIVATE(vm)\
>> -((qemuDomainObjPrivatePtr) (vm)->privateData)
>> +/* for migrations using TLS with a secret (not to be saved in our */
>> +/* private XML). */
>> +qemuDomainSecretInfoPtr migSecinfo;
>>  
>> -/* 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;
>> +/* Used when fetching/storing the current 'tls-creds' migration setting 
>> */
>> +/* (not to be saved in our private XML). */
>> +char *migTLSAlias;
> 
> I'm not quite sure why we need to store the original value.
> 

It determines whether or not TLS is possible...  It can ensure that when
not using TLS for migration that we set the params to "" if they aren't
already set that way.

>>  };
> ...
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 66a5062..42074f0 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -85,6 +85,8 @@ VIR_ENUM_IMPL(qemuMigrationCompressMethod, 
>> QEMU_MIGRATION_COMPRESS_LAST,
>>"mt",
>>  );
>>  
>> +#define QEMU_MIGRATION_TLS_ALIAS_BASE "libvirt_migrate"
>> +
>>  enum qemuMigrationCookieFlags {
>>  QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>>  QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
>> @@ -1488,6 +1490,164 @@ qemuMigrationEatCookie(virQEMUDriverPtr driver,
>>  return NULL;
>>  }
>>  
>> +/* qemuMigrationCheckTLSCreds
>> + * @driver: pointer to qemu driver
>> + * @vm: domain object
>> + * @asyncJob: migration job to join
>> + *
>> + * Query the migration parameters looking for the 

Re: [libvirt] [PATCH v3 6/7] qemu: Set up the migrate TLS objects for target

[Jiri Denemark]

On Fri, Mar 17, 2017 at 14:39:00 -0400, John Ferlan wrote:
> If the migration flags indicate this migration will be using TLS,
> then set up the destination during the prepare phase once the target
> domain has been started to add the TLS objects to perform the migration.
> 
> This will create at least an "-object tls-creds-x509,endpoint=server,..."
> and potentially an "-object secret,..." to handle the passphrase response

Looks like a leftover from previous versions where the objects were not
hotplugged on both sides of migration.

> to access the TLS credentials. The alias/id used for the TLS objects
> will contain "libvirt_migrate" as a mechanism to signify that libvirt
> started the migration on the target (reaping benefits possibly).
> 
> Once the objects are created, the code will set the "tls-creds" and
> "tls-hostname" migration parameters to signify usage of TLS.
> 
> During the Finish phase we'll be sure to attempt to clear the
> migration parameters and delete those objects (whether or not they
> were created).
> 
> Since incoming migrations that don't reach the Finish stage will be
> killed in qemuProcessRecoverMigrationIn and the only purpose at that
> point would be to free memory, it's not necessary to set up any sort
> of recovery. Additionally, subsequent migrations will check if the
> migration parameters are set and adjust them appropriately if for
> some reason libvirtd restarts after setting the Finish marker, but
> before actually resetting the environment.

Sure, migrations will do that, but what about snapshots, saving a domain
to a file and similar stuff which internally uses migration too. Do we
properly reset the parameters for them too? I think we should delete the
objects and reset migration parameters in qemuProcessRecoverMigrationIn
anyway.

> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_domain.c|   7 +-
>  src/qemu/qemu_domain.h|  91 +++-
>  src/qemu/qemu_migration.c | 344 
> ++
>  3 files changed, 403 insertions(+), 39 deletions(-)
...
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 1f266bf..1dd3b1c 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
...
> @@ -246,47 +283,18 @@ struct _qemuDomainObjPrivate {
>  
>  /* note whether memory device alias does not correspond to slot number */
>  bool memAliasOrderMismatch;
> -};
>  
> -# define QEMU_DOMAIN_PRIVATE(vm) \
> -((qemuDomainObjPrivatePtr) (vm)->privateData)
> +/* for migrations using TLS with a secret (not to be saved in our */
> +/* private XML). */
> +qemuDomainSecretInfoPtr migSecinfo;
>  
> -/* 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;
> +/* Used when fetching/storing the current 'tls-creds' migration setting 
> */
> +/* (not to be saved in our private XML). */
> +char *migTLSAlias;

I'm not quite sure why we need to store the original value.

>  };
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 66a5062..42074f0 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -85,6 +85,8 @@ VIR_ENUM_IMPL(qemuMigrationCompressMethod, 
> QEMU_MIGRATION_COMPRESS_LAST,
>"mt",
>  );
>  
> +#define QEMU_MIGRATION_TLS_ALIAS_BASE "libvirt_migrate"
> +
>  enum qemuMigrationCookieFlags {
>  QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>  QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
> @@ -1488,6 +1490,164 @@ qemuMigrationEatCookie(virQEMUDriverPtr driver,
>  return NULL;
>  }
>  
> +/* qemuMigrationCheckTLSCreds
> + * @driver: pointer to qemu driver
> + * @vm: domain object
> + * @asyncJob: migration job to join
> + *
> + * Query the migration parameters looking for the 'tls-creds' parameter.
> + * The parameter was initially supported in QEMU 2.7; however, there was
> + * no mechanism provided to clear the parameter. For QEMU 2.9, a change
> + * was made to allow setting the parameter to an empty string in order
> + * to clear. An additional change was made to initialize the parameter
> + * to the empty string. Although still not perfect since it's possible
> + * that a pre-2.9 release set the string to something and we should not

How would this happen? QEMU won't magically set it to something by
itself. And we don't need to care about someone messing up with the
monitor or command line manually.

Anyway, I don't see a real value in repeating the story over and over.
We just ask for the default parameter values and if TLS parameters are
not there, they are 

[libvirt] [PATCH v3 6/7] qemu: Set up the migrate TLS objects for target

[John Ferlan]

If the migration flags indicate this migration will be using TLS,
then set up the destination during the prepare phase once the target
domain has been started to add the TLS objects to perform the migration.

This will create at least an "-object tls-creds-x509,endpoint=server,..."
and potentially an "-object secret,..." to handle the passphrase response
to access the TLS credentials. The alias/id used for the TLS objects
will contain "libvirt_migrate" as a mechanism to signify that libvirt
started the migration on the target (reaping benefits possibly).

Once the objects are created, the code will set the "tls-creds" and
"tls-hostname" migration parameters to signify usage of TLS.

During the Finish phase we'll be sure to attempt to clear the
migration parameters and delete those objects (whether or not they
were created).

Since incoming migrations that don't reach the Finish stage will be
killed in qemuProcessRecoverMigrationIn and the only purpose at that
point would be to free memory, it's not necessary to set up any sort
of recovery. Additionally, subsequent migrations will check if the
migration parameters are set and adjust them appropriately if for
some reason libvirtd restarts after setting the Finish marker, but
before actually resetting the environment.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_domain.c|   7 +-
 src/qemu/qemu_domain.h|  91 +++-
 src/qemu/qemu_migration.c | 344 ++
 3 files changed, 403 insertions(+), 39 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c239a06..f4636ed 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -782,7 +782,7 @@ qemuDomainSecretAESClear(qemuDomainSecretAES secret)
 }
 
 
-static void
+void
 qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo)
 {
 if (!*secinfo)
@@ -1186,7 +1186,7 @@ qemuDomainSecretInfoNew(virConnectPtr conn,
  *
  * Returns qemuDomainSecretInfoPtr or NULL on error.
  */
-static qemuDomainSecretInfoPtr
+qemuDomainSecretInfoPtr
 qemuDomainSecretInfoTLSNew(virConnectPtr conn,
qemuDomainObjPrivatePtr priv,
const char *srcAlias,
@@ -1677,6 +1677,9 @@ qemuDomainObjPrivateFree(void *data)
 
 VIR_FREE(priv->libDir);
 VIR_FREE(priv->channelTargetDir);
+
+qemuDomainSecretInfoFree(>migSecinfo);
+VIR_FREE(priv->migTLSAlias);
 qemuDomainMasterKeyFree(priv);
 
 VIR_FREE(priv);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 1f266bf..1dd3b1c 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,47 +283,18 @@ struct _qemuDomainObjPrivate {
 
 /* note whether memory device alias does not correspond to slot number */
 bool memAliasOrderMismatch;
-};
 
-# define QEMU_DOMAIN_PRIVATE(vm)   \
-((qemuDomainObjPrivatePtr) (vm)->privateData)
+/* for migrations using TLS with a secret (not to be saved in our */
+/* private XML). */
+qemuDomainSecretInfoPtr migSecinfo;
 
-/* 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