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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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.

Really?  See [*] below.

>>  #
>>  # @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->has_tls_creds = true;
>>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>>      params->has_tls_hostname = true;
>>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);

[*] Looks non-optional to me.

>> -    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.

I'm kind of confused about pretty much everything around here :)

The patch hunk is part of the revert of flawed commit 7cd75cbdb8.  We
need to revert both parts or none.

One difference between tls_authz and the others is in
migration_instance_init(): it leaves params->tls_authz null, unlike
->tls_hostname and ->tls_creds.

Hmm, it sets ->has_ for none of them.  Wrong.  If we set ->FOO, we must
also set ->has_FOO = true, and if we leave ->has_FOO false, we should
leave ->FOO null.

Another difference is in migration_tls_channel_process_incoming():
s->parameters.tls_creds must not be null (it's used unchecked in
migration_tls_get_creds()), while s->parameters.tls_authz may be
(qcrypto_tls_session_new() checks).

We need to make up our minds what is optional and what isn't.

>> +    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

Second item in the commit message's list.

>>      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

Third item.

>>  
>>      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.

This is the other part of the revert of flawed commit 7cd75cbdb8.  We
need to revert both parts or none.