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

[Daniel P . Berrangé]

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