Daniel P. Berrangé <berra...@redhat.com> writes: > On Thu, Dec 17, 2020 at 02:07:01PM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <berra...@redhat.com> writes: >> >> > On Mon, Dec 14, 2020 at 11:14:34AM +0100, Markus Armbruster wrote: >> >> Daniel P. Berrangé <berra...@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é <berra...@redhat.com> >> >> >> Signed-off-by: Markus Armbruster <arm...@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é <berra...@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 <berra...@redhat.com> >> > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> >> > Reviewed-by: Eric Blake <ebl...@redhat.com> >> > >> > Signed-off-by: Juan Quintela <quint...@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 :(