[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 1/6] migration: Fix and clean up around @tls-authz |
Date: |
Thu, 17 Dec 2020 14:07:01 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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 :)