Re: [Qemu-devel] [PATCH v3 24/27] migration: define 'tls-creds' and 'tls-hostname' migration parameters

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> Define two new migration parameters to be used with TLS encryption.
> The 'tls-creds' parameter provides the ID of an instance of the
> 'tls-creds' object type, or rather a subclass such as 'tls-creds-x509'.
> Providing these credentials will enable use of TLS on the migration
> data stream.
> 
> If using x509 certificates, together with a migration URI that does
> not include a hostname, the 'tls-hostname' parameter provides the
> hostname to use when verifying the server's x509 certificate. This
> allows TLS to be used in combination with fd: and exec: protocols
> where a TCP connection is established by a 3rd party outside of
> QEMU.
> 
> For the HMP this sadly requires adding a new monitor command
> 'migration_set_str_parameter', since the existing command
> 'migration_set_parameter' is fixed to take integer values.

Can you explain why?
The definition of the 's' string type in monitor.c says:
 * 's'          string (accept optional quote)

and hmp_block_stream already uses 's' for an integer parameter (why?).
So if you just changed the definition to take a :s parameter it would
work wouldn't it as long as you did an appropriate check in hmp_migrate_set_parameter?

Dave

> The QMP monitor is fine, since it has the more flexible
> 'migration_set_parameters' which takes the dict of params
> of arbitrary types per the QAPI schema.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hmp-commands.hx       | 15 +++++++++++++
>  hmp.c                 | 48 ++++++++++++++++++++++++++++++++++++++++++
>  hmp.h                 |  1 +
>  migration/migration.c | 14 +++++++++++++
>  qapi-schema.json      | 58 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 133 insertions(+), 3 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 664d794..883ef90 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1022,6 +1022,21 @@ Set the parameter @var{parameter} for migration.
>  ETEXI
>  
>      {
> +        .name       = "migrate_set_str_parameter",
> +        .args_type  = "parameter:s,value:s",
> +        .params     = "parameter value",
> +        .help       = "Set the parameter for migration",
> +        .mhandler.cmd = hmp_migrate_set_str_parameter,
> +        .command_completion = migrate_set_parameter_completion,
> +    },
> +
> +STEXI
> +@item migrate_set_str_parameter @var{parameter} @var{value}
> +@findex migrate_set_str_parameter
> +Set the parameter @var{parameter} for migration.
> +ETEXI
> +
> +    {
>          .name       = "migrate_start_postcopy",
>          .args_type  = "",
>          .params     = "",
> diff --git a/hmp.c b/hmp.c
> index a239e1e..c27c280 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -290,6 +290,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, " %s: %" PRId64,
>              MigrationParameter_lookup[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT],
>              params->x_cpu_throttle_increment);
> +        monitor_printf(mon, " %s: '%s'",
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_CREDS],
> +            params->tls_creds ? : "");
> +        monitor_printf(mon, " %s: '%s'",
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
> +            params->tls_hostname ? : "");
>          monitor_printf(mon, "\n");
>      }
>  
> @@ -1272,6 +1278,48 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                                         has_decompress_threads, value,
>                                         has_x_cpu_throttle_initial, value,
>                                         has_x_cpu_throttle_increment, value,
> +                                       false, NULL,
> +                                       false, NULL,
> +                                       &err);
> +            break;
> +        }
> +    }
> +
> +    if (i == MIGRATION_PARAMETER__MAX) {
> +        error_setg(&err, QERR_INVALID_PARAMETER, param);
> +    }
> +
> +    if (err) {
> +        error_report_err(err);
> +    }
> +}
> +
> +void hmp_migrate_set_str_parameter(Monitor *mon, const QDict *qdict)
> +{
> +    const char *param = qdict_get_str(qdict, "parameter");
> +    const char *valuestr = qdict_get_str(qdict, "value");
> +    Error *err = NULL;
> +    bool has_tls_creds = false;
> +    bool has_tls_hostname = false;
> +    int i;
> +
> +    for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
> +        if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
> +            switch (i) {
> +            case MIGRATION_PARAMETER_TLS_CREDS:
> +                has_tls_creds = true;
> +                break;
> +            case MIGRATION_PARAMETER_TLS_HOSTNAME:
> +                has_tls_hostname = true;
> +                break;
> +            }
> +            qmp_migrate_set_parameters(false, 0,
> +                                       false, 0,
> +                                       false, 0,
> +                                       false, 0,
> +                                       false, 0,
> +                                       has_tls_creds, valuestr,
> +                                       has_tls_hostname, valuestr,
>                                         &err);
>              break;
>          }
> diff --git a/hmp.h b/hmp.h
> index 093d65f..74873b4 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -67,6 +67,7 @@ void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict);
> +void hmp_migrate_set_str_parameter(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict);
>  void hmp_client_migrate_info(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict);
> diff --git a/migration/migration.c b/migration/migration.c
> index b3bdc31..c7bc1c7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -528,6 +528,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->decompress_threads = s->parameters.decompress_threads;
>      params->x_cpu_throttle_initial = s->parameters.x_cpu_throttle_initial;
>      params->x_cpu_throttle_increment = s->parameters.x_cpu_throttle_increment;
> +    params->tls_creds = g_strdup(s->parameters.tls_creds);
> +    params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>  
>      return params;
>  }
> @@ -728,6 +730,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                                  int64_t x_cpu_throttle_initial,
>                                  bool has_x_cpu_throttle_increment,
>                                  int64_t x_cpu_throttle_increment,
> +                                bool has_tls_creds,
> +                                const char *tls_creds,
> +                                bool has_tls_hostname,
> +                                const char *tls_hostname,
>                                  Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
> @@ -779,6 +785,14 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>      if (has_x_cpu_throttle_increment) {
>          s->parameters.x_cpu_throttle_increment = x_cpu_throttle_increment;
>      }
> +    if (has_tls_creds) {
> +        g_free(s->parameters.tls_creds);
> +        s->parameters.tls_creds = g_strdup(tls_creds);
> +    }
> +    if (has_tls_hostname) {
> +        g_free(s->parameters.tls_hostname);
> +        s->parameters.tls_hostname = g_strdup(tls_hostname);
> +    }
>  }
>  
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index ff89747..fbe8c5e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -615,11 +615,28 @@
>  # @x-cpu-throttle-increment: throttle percentage increase each time
>  #                            auto-converge detects that migration is not making
>  #                            progress. The default value is 10. (Since 2.5)
> +#
> +# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> +#             establishing a TLS connection over the migration data channel.
> +#             On the outgoing side of the migration, the credentials must
> +#             be for a 'client' endpoint, while for the incoming side the
> +#             credentials must be for a 'server' endpoint. Setting this
> +#             will enable TLS for all migrations. The default is unset,
> +#             resulting in unsecured migration at the QEMU level. (Since 2.6)
> +#
> +# @tls-hostname: hostname of the target host for the migration. This is
> +#                required when using x509 based TLS credentials and the
> +#                migration URI does not already include a hostname. For
> +#                example if using fd: or exec: based migration, the
> +#                hostname must be provided so that the server's x509
> +#                certificate identity canbe validated. (Since 2.6)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
>    'data': ['compress-level', 'compress-threads', 'decompress-threads',
> -           'x-cpu-throttle-initial', 'x-cpu-throttle-increment'] }
> +           'x-cpu-throttle-initial', 'x-cpu-throttle-increment',
> +           'tls-creds', 'tls-hostname'] }
>  
>  #
>  # @migrate-set-parameters
> @@ -639,6 +656,22 @@
>  # @x-cpu-throttle-increment: throttle percentage increase each time
>  #                            auto-converge detects that migration is not making
>  #                            progress. The default value is 10. (Since 2.5)
> +#
> +# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> +#             establishing a TLS connection over the migration data channel.
> +#             On the outgoing side of the migration, the credentials must
> +#             be for a 'client' endpoint, while for the incoming side the
> +#             credentials must be for a 'server' endpoint. Setting this
> +#             will enable TLS for all migrations. The default is unset,
> +#             resulting in unsecured migration at the QEMU level. (Since 2.6)
> +#
> +# @tls-hostname: hostname of the target host for the migration. This is
> +#                required when using x509 based TLS credentials and the
> +#                migration URI does not already include a hostname. For
> +#                example if using fd: or exec: based migration, the
> +#                hostname must be provided so that the server's x509
> +#                certificate identity canbe validated. (Since 2.6)
> +#
>  # Since: 2.4
>  ##
>  { 'command': 'migrate-set-parameters',
> @@ -646,7 +679,9 @@
>              '*compress-threads': 'int',
>              '*decompress-threads': 'int',
>              '*x-cpu-throttle-initial': 'int',
> -            '*x-cpu-throttle-increment': 'int'} }
> +            '*x-cpu-throttle-increment': 'int',
> +            '*tls-creds': 'str',
> +            '*tls-hostname': 'str'} }
>  
>  #
>  # @MigrationParameters
> @@ -665,6 +700,21 @@
>  #                            auto-converge detects that migration is not making
>  #                            progress. The default value is 10. (Since 2.5)
>  #
> +# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> +#             establishing a TLS connection over the migration data channel.
> +#             On the outgoing side of the migration, the credentials must
> +#             be for a 'client' endpoint, while for the incoming side the
> +#             credentials must be for a 'server' endpoint. Setting this
> +#             will enable TLS for all migrations. The default is unset,
> +#             resulting in unsecured migration at the QEMU level. (Since 2.6)
> +#
> +# @tls-hostname: hostname of the target host for the migration. This is
> +#                required when using x509 based TLS credentials and the
> +#                migration URI does not already include a hostname. For
> +#                example if using fd: or exec: based migration, the
> +#                hostname must be provided so that the server's x509
> +#                certificate identity canbe validated. (Since 2.6)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -672,7 +722,9 @@
>              'compress-threads': 'int',
>              'decompress-threads': 'int',
>              'x-cpu-throttle-initial': 'int',
> -            'x-cpu-throttle-increment': 'int'} }
> +            'x-cpu-throttle-increment': 'int',
> +            'tls-creds': 'str',
> +            'tls-hostname': 'str'} }
>  ##
>  # @query-migrate-parameters
>  #
> -- 
> 2.5.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK