Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Nov 13, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
> parameter" added MigrationParameters member @tls-authz.  Whereas the
> other members aren't really optional (see commit 1bda8b3c695), this
> one is genuinely optional: migration_instance_init() leaves it absent,
> and migration_tls_channel_process_incoming() passes it to
> qcrypto_tls_session_new(), which checks for null.
> 
> Commit d2f1d29b95 has a number of issues, though:
> 
> * When qmp_query_migrate_parameters() copies migration parameters into
>   its reply, it ignores has_tls_authz, and assumes true instead.  When
>   it is false,
> 
>   - HMP info migrate_parameters prints the null pointer (crash bug on
>     some systems), and
> 
>   - QMP query-migrate-parameters replies "tls-authz": "" (because the
>     QObject output visitor silently maps null pointer to "", which it
>     really shouldn't).
> 
>   The HMP defect was noticed and fixed in commit 7cd75cbdb8
>   'migration: use "" instead of (null) for tls-authz'.  Unfortunately,
>   the fix papered over the real bug: it made
>   qmp_query_migrate_parameters() map null tls_authz to "".  It also
>   dropped the check for has_tls_authz from
>   hmp_info_migrate_parameters().
> 
>   Revert, and fix qmp_query_migrate_parameters() not to screw up
>   has_tls_authz.  No change to HMP.  QMP now has "tls-authz" in the
>   reply only when it's actually present in
>   migrate_get_current()->parameters.  If we prefer to remain
>   bug-compatible, we should make tls_authz non-optional there.
> 
> * migrate_params_test_apply() neglects to apply tls_authz.  Currently
>   harmless, because migrate_params_check() doesn't care.  Fix it
>   anyway.
> 
> * qmp_migrate_set_parameters() crashes:
> 
>     {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
> 
>   Add the necessary rewrite of null to "".  For background
>   information, see commit 01fa559826 "migration: Use JSON null instead
>   of "" to reset parameter to default".
> 
> Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/migration.json   |  2 +-
>  migration/migration.c | 17 ++++++++++++++---
>  monitor/hmp-cmds.c    |  2 +-
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3c75820527..688e8da749 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -928,7 +928,7 @@
>  ##
>  # @MigrationParameters:
>  #
> -# The optional members aren't actually optional.
> +# The optional members aren't actually optional, except for @tls-authz.

and tls-hostname and tls-creds.

>  #
>  # @announce-initial: Initial delay (in milliseconds) before sending the
>  #                    first announce (Since 4.0)
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa55a9..cad56fbf8c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>      params->has_tls_hostname = true;
>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> -    params->has_tls_authz = true;
> -    params->tls_authz = g_strdup(s->parameters.tls_authz ?
> -                                 s->parameters.tls_authz : "");
> +    params->has_tls_authz = s->parameters.has_tls_authz;

I'm kind of confused why has_tls_authz needs to be handled differently
from tls_hostname and tls_creds - both of these are optional to
the same extent that tls_authz is AFAIR.

> +    params->tls_authz = g_strdup(s->parameters.tls_authz);

This makes it match what is done for tls_hostname/creds though
which makes sense.

>      params->has_max_bandwidth = true;
>      params->max_bandwidth = s->parameters.max_bandwidth;
>      params->has_downtime_limit = true;
> @@ -1433,6 +1432,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>          dest->tls_hostname = params->tls_hostname->u.s;
>      }
>  
> +    if (params->has_tls_authz) {
> +        assert(params->tls_authz->type == QTYPE_QSTRING);
> +        dest->tls_authz = params->tls_authz->u.s;
> +    }
> +

Makes sense, as it was missed previously

>      if (params->has_max_bandwidth) {
>          dest->max_bandwidth = params->max_bandwidth;
>      }
> @@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>          params->tls_hostname->type = QTYPE_QSTRING;
>          params->tls_hostname->u.s = strdup("");
>      }
> +    /* TODO Rewrite "" to null instead */
> +    if (params->has_tls_authz
> +        && params->tls_authz->type == QTYPE_QNULL) {
> +        qobject_unref(params->tls_authz->u.n);
> +        params->tls_authz->type = QTYPE_QSTRING;
> +        params->tls_authz->u.s = strdup("");
> +    }

Makes sense, as it matches what was done for tls_creds/tls_hostname

>  
>      migrate_params_test_apply(params, &tmp);
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..492789248f 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>              params->max_postcopy_bandwidth);
>          monitor_printf(mon, "%s: '%s'\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> -            params->tls_authz);
> +            params->has_tls_authz ? params->tls_authz : "");

Again, I'm confused why it needs to be handled differently from
tls_creds / tls_hostname, which are also optional. It feels like
either we need to change all three, or none of them.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|