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

[Daniel P . Berrangé]

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