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.
 #
 # @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;
+    params->tls_authz = g_strdup(s->parameters.tls_authz);
     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;
+    }
+
     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("");
+    }
 
     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 : "");
 
         if (params->has_block_bitmap_mapping) {
             const BitmapMigrationNodeAliasList *bmnal;
-- 
2.26.2

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

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.

On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
> 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.

I guess it depends on what you mean by "optional" :-)

When I say they are all optional, I'm talking about from the POV
of the end users / mgmt who first configures a migration operation.

tls-creds only needs to be set if you want to enable TLS

tls-hostname only needs to be set if you need to override the
default hostname used for cert validation.

tls-authz only needs to be set if you want to enable access
control over migration clients.

IOW, all three are optional from the POV of configuring a
migration.

As with many things though, simple theory has turned into
messy reality, by virtue of this previous fixup:

  commit 4af245dc3e6e5c96405b3edb9d75657504256469
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Wed Mar 15 16:16:03 2017 +0000

    migration: use "" as the default for tls-creds/hostname
    
    The tls-creds parameter has a default value of NULL indicating
    that TLS should not be used. Setting it to non-NULL enables
    use of TLS. Once tls-creds are set to a non-NULL value via the
    monitor, it isn't possible to set them back to NULL again, due
    to current implementation limitations. The empty string is not
    a valid QObject identifier, so this switches to use "" as the
    default, indicating that TLS will not be used
    
    The tls-hostname parameter has a default value of NULL indicating
    the the hostname from the migrate connection URI should be used.
    Again, once tls-hostname is set non-NULL, to override the default
    hostname for x509 cert validation, it isn't possible to reset it
    back to NULL via the monitor. The empty string is not a valid
    hostname, so this switches to use "" as the default, indicating
    that the migrate URI hostname should be used.
    
    Using "" as the default for both, also means that the monitor
    commands "info migrate_parameters" / "query-migrate-parameters"
    will report existance of tls-creds/tls-parameters even when set
    to their default values.
    
    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
    Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    
    Signed-off-by: Juan Quintela <quintela@redhat.com>


I have a nasty feeling that libvirt relies on that last paragraph
to determine whether TLS is supported in QEMU or not too :-( Ideally
we should be able to report their existance, but also report that
they are set to NULL. I guess that could be considered a regression
at this point though.

So anyway, this explains why we have the wierd behaviour where
querying parameters always reports them as being set.

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

So tls_authz was following the wierd precedent used by tls_hostname
and tls_creds in always reporting its own existance, as the empty
string.

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

So they are all optional in terms of what needs to be set.

They are all always reported when querying parameters.

The main difference seems to be that internally we use NULL
as a default for tls_authz, and convert NULL to "" when reporting,
while for tls_creds/tls_hostname we convert NULL to "" immediately
so we always have "" internally.

Should we instead set tls_authz to "" internally straight away
like we do for tls_creds/tls_hostname, and then make the code
turn "" back into NULL at time of use.


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

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

> On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
>> 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.
>
> I guess it depends on what you mean by "optional" :-)

I meant "non-optional in the value of query-migrate-parameters".  The
comment were debating applies to that value, and nothing else.

> When I say they are all optional, I'm talking about from the POV
> of the end users / mgmt who first configures a migration operation.
>
> tls-creds only needs to be set if you want to enable TLS
>
> tls-hostname only needs to be set if you need to override the
> default hostname used for cert validation.
>
> tls-authz only needs to be set if you want to enable access
> control over migration clients.
>
> IOW, all three are optional from the POV of configuring a
> migration.

Understood.

> As with many things though, simple theory has turned into
> messy reality, by virtue of this previous fixup:
>
>   commit 4af245dc3e6e5c96405b3edb9d75657504256469
>   Author: Daniel P. Berrangé <berrange@redhat.com>
>   Date:   Wed Mar 15 16:16:03 2017 +0000
>
>     migration: use "" as the default for tls-creds/hostname
>     
>     The tls-creds parameter has a default value of NULL indicating
>     that TLS should not be used. Setting it to non-NULL enables
>     use of TLS. Once tls-creds are set to a non-NULL value via the
>     monitor, it isn't possible to set them back to NULL again, due
>     to current implementation limitations. The empty string is not
>     a valid QObject identifier, so this switches to use "" as the
>     default, indicating that TLS will not be used
>     
>     The tls-hostname parameter has a default value of NULL indicating
>     the the hostname from the migrate connection URI should be used.
>     Again, once tls-hostname is set non-NULL, to override the default
>     hostname for x509 cert validation, it isn't possible to reset it
>     back to NULL via the monitor. The empty string is not a valid
>     hostname, so this switches to use "" as the default, indicating
>     that the migrate URI hostname should be used.
>     
>     Using "" as the default for both, also means that the monitor
>     commands "info migrate_parameters" / "query-migrate-parameters"
>     will report existance of tls-creds/tls-parameters even when set
>     to their default values.
>     
>     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>     Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>     Reviewed-by: Eric Blake <eblake@redhat.com>
>     
>     Signed-off-by: Juan Quintela <quintela@redhat.com>
>
>
> I have a nasty feeling that libvirt relies on that last paragraph
> to determine whether TLS is supported in QEMU or not too :-( Ideally
> we should be able to report their existance, but also report that
> they are set to NULL. I guess that could be considered a regression
> at this point though.
>
> So anyway, this explains why we have the wierd behaviour where
> querying parameters always reports them as being set.

Yes.

What do you want me to change in my patch?

>> >> -    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 :)
>
> So tls_authz was following the wierd precedent used by tls_hostname
> and tls_creds in always reporting its own existance, as the empty
> string.
>
>> 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.
>
> So they are all optional in terms of what needs to be set.
>
> They are all always reported when querying parameters.
>
> The main difference seems to be that internally we use NULL
> as a default for tls_authz, and convert NULL to "" when reporting,
> while for tls_creds/tls_hostname we convert NULL to "" immediately
> so we always have "" internally.
>
> Should we instead set tls_authz to "" internally straight away
> like we do for tls_creds/tls_hostname, and then make the code
> turn "" back into NULL at time of use.

I don't know!  I'm merely trying to fix a crash bug I ran into :)

On Thu, Dec 17, 2020 at 02:07:01PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
> >> 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.
> >
> > I guess it depends on what you mean by "optional" :-)
> 
> I meant "non-optional in the value of query-migrate-parameters".  The
> comment were debating applies to that value, and nothing else.
> 
> > When I say they are all optional, I'm talking about from the POV
> > of the end users / mgmt who first configures a migration operation.
> >
> > tls-creds only needs to be set if you want to enable TLS
> >
> > tls-hostname only needs to be set if you need to override the
> > default hostname used for cert validation.
> >
> > tls-authz only needs to be set if you want to enable access
> > control over migration clients.
> >
> > IOW, all three are optional from the POV of configuring a
> > migration.
> 
> Understood.
> 
> > As with many things though, simple theory has turned into
> > messy reality, by virtue of this previous fixup:
> >
> >   commit 4af245dc3e6e5c96405b3edb9d75657504256469
> >   Author: Daniel P. Berrangé <berrange@redhat.com>
> >   Date:   Wed Mar 15 16:16:03 2017 +0000
> >
> >     migration: use "" as the default for tls-creds/hostname
> >     
> >     The tls-creds parameter has a default value of NULL indicating
> >     that TLS should not be used. Setting it to non-NULL enables
> >     use of TLS. Once tls-creds are set to a non-NULL value via the
> >     monitor, it isn't possible to set them back to NULL again, due
> >     to current implementation limitations. The empty string is not
> >     a valid QObject identifier, so this switches to use "" as the
> >     default, indicating that TLS will not be used
> >     
> >     The tls-hostname parameter has a default value of NULL indicating
> >     the the hostname from the migrate connection URI should be used.
> >     Again, once tls-hostname is set non-NULL, to override the default
> >     hostname for x509 cert validation, it isn't possible to reset it
> >     back to NULL via the monitor. The empty string is not a valid
> >     hostname, so this switches to use "" as the default, indicating
> >     that the migrate URI hostname should be used.
> >     
> >     Using "" as the default for both, also means that the monitor
> >     commands "info migrate_parameters" / "query-migrate-parameters"
> >     will report existance of tls-creds/tls-parameters even when set
> >     to their default values.
> >     
> >     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >     Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >     Reviewed-by: Eric Blake <eblake@redhat.com>
> >     
> >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> >
> > I have a nasty feeling that libvirt relies on that last paragraph
> > to determine whether TLS is supported in QEMU or not too :-( Ideally
> > we should be able to report their existance, but also report that
> > they are set to NULL. I guess that could be considered a regression
> > at this point though.
> >
> > So anyway, this explains why we have the wierd behaviour where
> > querying parameters always reports them as being set.
> 
> Yes.
> 
> What do you want me to change in my patch?
> 
> >> >> -    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 :)
> >
> > So tls_authz was following the wierd precedent used by tls_hostname
> > and tls_creds in always reporting its own existance, as the empty
> > string.
> >
> >> 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.
> >
> > So they are all optional in terms of what needs to be set.
> >
> > They are all always reported when querying parameters.
> >
> > The main difference seems to be that internally we use NULL
> > as a default for tls_authz, and convert NULL to "" when reporting,
> > while for tls_creds/tls_hostname we convert NULL to "" immediately
> > so we always have "" internally.
> >
> > Should we instead set tls_authz to "" internally straight away
> > like we do for tls_creds/tls_hostname, and then make the code
> > turn "" back into NULL at time of use.
> 
> I don't know!  I'm merely trying to fix a crash bug I ran into :)

Ok, if you don't mind which approach, then I'd vote for making
migration_instance_init() set  tls_authz to "", in common with
tls_hostname/tls_creds.

Then in migration_tls_channel_process_incoming we can turn the
"" back into NULL.

That way we'll have consistently used "" internally for all the
TLS related parameters.

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

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

> On Thu, Dec 17, 2020 at 02:07:01PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote:
>> >> 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.
>> >
>> > I guess it depends on what you mean by "optional" :-)
>> 
>> I meant "non-optional in the value of query-migrate-parameters".  The
>> comment were debating applies to that value, and nothing else.
>> 
>> > When I say they are all optional, I'm talking about from the POV
>> > of the end users / mgmt who first configures a migration operation.
>> >
>> > tls-creds only needs to be set if you want to enable TLS
>> >
>> > tls-hostname only needs to be set if you need to override the
>> > default hostname used for cert validation.
>> >
>> > tls-authz only needs to be set if you want to enable access
>> > control over migration clients.
>> >
>> > IOW, all three are optional from the POV of configuring a
>> > migration.
>> 
>> Understood.
>> 
>> > As with many things though, simple theory has turned into
>> > messy reality, by virtue of this previous fixup:
>> >
>> >   commit 4af245dc3e6e5c96405b3edb9d75657504256469
>> >   Author: Daniel P. Berrangé <berrange@redhat.com>
>> >   Date:   Wed Mar 15 16:16:03 2017 +0000
>> >
>> >     migration: use "" as the default for tls-creds/hostname
>> >     
>> >     The tls-creds parameter has a default value of NULL indicating
>> >     that TLS should not be used. Setting it to non-NULL enables
>> >     use of TLS. Once tls-creds are set to a non-NULL value via the
>> >     monitor, it isn't possible to set them back to NULL again, due
>> >     to current implementation limitations. The empty string is not
>> >     a valid QObject identifier, so this switches to use "" as the
>> >     default, indicating that TLS will not be used
>> >     
>> >     The tls-hostname parameter has a default value of NULL indicating
>> >     the the hostname from the migrate connection URI should be used.
>> >     Again, once tls-hostname is set non-NULL, to override the default
>> >     hostname for x509 cert validation, it isn't possible to reset it
>> >     back to NULL via the monitor. The empty string is not a valid
>> >     hostname, so this switches to use "" as the default, indicating
>> >     that the migrate URI hostname should be used.
>> >     
>> >     Using "" as the default for both, also means that the monitor
>> >     commands "info migrate_parameters" / "query-migrate-parameters"
>> >     will report existance of tls-creds/tls-parameters even when set
>> >     to their default values.
>> >     
>> >     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> >     Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >     Reviewed-by: Eric Blake <eblake@redhat.com>
>> >     
>> >     Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >
>> >
>> > I have a nasty feeling that libvirt relies on that last paragraph
>> > to determine whether TLS is supported in QEMU or not too :-( Ideally
>> > we should be able to report their existance, but also report that
>> > they are set to NULL. I guess that could be considered a regression
>> > at this point though.
>> >
>> > So anyway, this explains why we have the wierd behaviour where
>> > querying parameters always reports them as being set.
>> 
>> Yes.
>> 
>> What do you want me to change in my patch?
>> 
>> >> >> -    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 :)
>> >
>> > So tls_authz was following the wierd precedent used by tls_hostname
>> > and tls_creds in always reporting its own existance, as the empty
>> > string.
>> >
>> >> 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.
>> >
>> > So they are all optional in terms of what needs to be set.
>> >
>> > They are all always reported when querying parameters.
>> >
>> > The main difference seems to be that internally we use NULL
>> > as a default for tls_authz, and convert NULL to "" when reporting,
>> > while for tls_creds/tls_hostname we convert NULL to "" immediately
>> > so we always have "" internally.
>> >
>> > Should we instead set tls_authz to "" internally straight away
>> > like we do for tls_creds/tls_hostname, and then make the code
>> > turn "" back into NULL at time of use.
>> 
>> I don't know!  I'm merely trying to fix a crash bug I ran into :)
>
> Ok, if you don't mind which approach, then I'd vote for making
> migration_instance_init() set  tls_authz to "", in common with
> tls_hostname/tls_creds.
>
> Then in migration_tls_channel_process_incoming we can turn the
> "" back into NULL.
>
> That way we'll have consistently used "" internally for all the
> TLS related parameters.

I suffered mental garbage collection during the Christmas break, and can
no longer make heads or tails out of all this.

I might have to drop the series on the floor :(

* Markus Armbruster (armbru@redhat.com) 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>

Yes, but I'd like an Ack from Dan as well for this one

Reviewed-by: Dr. David Alan Gilbert <dgilbert@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.
>  #
>  # @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;
> +    params->tls_authz = g_strdup(s->parameters.tls_authz);
>      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;
> +    }
> +
>      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("");
> +    }
>  
>      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 : "");
>  
>          if (params->has_block_bitmap_mapping) {
>              const BitmapMigrationNodeAliasList *bmnal;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Dan: Can you please see if this makes sense to you, it did to me
at the time.

Dave

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Markus Armbruster (armbru@redhat.com) 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>
> 
> Yes, but I'd like an Ack from Dan as well for this one
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@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.
> >  #
> >  # @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;
> > +    params->tls_authz = g_strdup(s->parameters.tls_authz);
> >      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;
> > +    }
> > +
> >      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("");
> > +    }
> >  
> >      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 : "");
> >  
> >          if (params->has_block_bitmap_mapping) {
> >              const BitmapMigrationNodeAliasList *bmnal;
> > -- 
> > 2.26.2
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK