| Message ID | 20180615155103.11924-4-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Daniel P. Berrangé (berrange@redhat.com) wrote: > From: "Daniel P. Berrange" <berrange@redhat.com> > > The QEMU instance that runs as the server for the migration data > transport (ie the target QEMU) needs to be able to configure access > control so it can prevent unauthorized clients initiating an incoming > migration. This adds a new 'tls-authz' migration parameter that is used > to provide the QOM ID of a QAuthZ subclass instance that provides the > access control check. This is checked against the x509 certificate > obtained during the TLS handshake. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> I'd appreciate an example of using it, either in the migration docs or the commit message. > --- > hmp.c | 9 +++++++++ > migration/migration.c | 8 ++++++++ > migration/tls.c | 2 +- > qapi/migration.json | 12 +++++++++++- > 4 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/hmp.c b/hmp.c > index 74e18db103..bef8ea2531 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -370,6 +370,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "%s: %" PRIu64 "\n", > MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), > params->xbzrle_cache_size); > + monitor_printf(mon, " %s: '%s'\n", > + MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), > + params->has_tls_authz ? params->tls_authz : ""); > } > > qapi_free_MigrationParameters(params); > @@ -1632,6 +1635,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > p->tls_hostname->type = QTYPE_QSTRING; > visit_type_str(v, param, &p->tls_hostname->u.s, &err); > break; > + case MIGRATION_PARAMETER_TLS_AUTHZ: > + p->has_tls_authz = true; > + p->tls_authz = g_new0(StrOrNull, 1); > + p->tls_authz->type = QTYPE_QSTRING; > + visit_type_str(v, param, &p->tls_authz->u.s, &err); > + break; > case MIGRATION_PARAMETER_MAX_BANDWIDTH: > p->has_max_bandwidth = true; > /* > diff --git a/migration/migration.c b/migration/migration.c > index 1e99ec9b7e..d14c8d7003 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -645,6 +645,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); > params->has_max_bandwidth = true; > params->max_bandwidth = s->parameters.max_bandwidth; > params->has_downtime_limit = true; > @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); > } > > + if (params->has_tls_authz) { > + g_free(s->parameters.tls_authz); > + assert(params->tls_authz->type == QTYPE_QSTRING); > + s->parameters.tls_authz = g_strdup(params->tls_authz->u.s); > + } > + > if (params->has_max_bandwidth) { > s->parameters.max_bandwidth = params->max_bandwidth; > if (s->to_dst_file) { > diff --git a/migration/tls.c b/migration/tls.c > index 3b9e8c9263..5171afc6c4 100644 > --- a/migration/tls.c > +++ b/migration/tls.c > @@ -94,7 +94,7 @@ void migration_tls_channel_process_incoming(MigrationState *s, > > tioc = qio_channel_tls_new_server( > ioc, creds, > - NULL, /* XXX pass ACL name */ > + s->parameters.tls_authz, > errp); > if (!tioc) { > return; > diff --git a/qapi/migration.json b/qapi/migration.json > index f7e10ee90f..b9ba34e3a6 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -488,6 +488,10 @@ > # hostname must be provided so that the server's x509 > # certificate identity can be validated. (Since 2.7) > # > +# @tls-authz: ID of the 'authz' object subclass that provides access control > +# checking of the TLS x509 certificate distinguished name. (Since > +# 2.13) > +# Oops, 2.13 strikes again :-) Other than that, OK from migration and HMP. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > # @max-bandwidth: to set maximum speed for migration. maximum speed in > # bytes per second. (Since 2.8) > # > @@ -522,7 +526,7 @@ > { 'enum': 'MigrationParameter', > 'data': ['compress-level', 'compress-threads', 'decompress-threads', > 'cpu-throttle-initial', 'cpu-throttle-increment', > - 'tls-creds', 'tls-hostname', 'max-bandwidth', > + 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > 'x-multifd-channels', 'x-multifd-page-count', > 'xbzrle-cache-size' ] } > @@ -605,6 +609,7 @@ > '*cpu-throttle-increment': 'int', > '*tls-creds': 'StrOrNull', > '*tls-hostname': 'StrOrNull', > + '*tls-authz': 'StrOrNull', > '*max-bandwidth': 'int', > '*downtime-limit': 'int', > '*x-checkpoint-delay': 'int', > @@ -667,6 +672,10 @@ > # associated with the migration URI, if any. (Since 2.9) > # Note: 2.8 reports this by omitting tls-hostname instead. > # > +# @tls-authz: ID of the 'authz' object subclass that provides access control > +# checking of the TLS x509 certificate distinguished name. (Since > +# 2.13) > +# > # @max-bandwidth: to set maximum speed for migration. maximum speed in > # bytes per second. (Since 2.8) > # > @@ -704,6 +713,7 @@ > '*cpu-throttle-increment': 'uint8', > '*tls-creds': 'str', > '*tls-hostname': 'str', > + '*tls-authz': 'str', > '*max-bandwidth': 'size', > '*downtime-limit': 'uint64', > '*x-checkpoint-delay': 'uint32', > -- > 2.17.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jun 15, 2018 at 06:54:23PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > From: "Daniel P. Berrange" <berrange@redhat.com> > > > > The QEMU instance that runs as the server for the migration data > > transport (ie the target QEMU) needs to be able to configure access > > control so it can prevent unauthorized clients initiating an incoming > > migration. This adds a new 'tls-authz' migration parameter that is used > > to provide the QOM ID of a QAuthZ subclass instance that provides the > > access control check. This is checked against the x509 certificate > > obtained during the TLS handshake. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > I'd appreciate an example of using it, either in the migration docs or > the commit message. Hmm, yes, it's an oversight to have missed an example in this commit message. > > > --- > > hmp.c | 9 +++++++++ > > migration/migration.c | 8 ++++++++ > > migration/tls.c | 2 +- > > qapi/migration.json | 12 +++++++++++- > > 4 files changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/hmp.c b/hmp.c > > index 74e18db103..bef8ea2531 100644 > > --- a/hmp.c > > +++ b/hmp.c > > @@ -370,6 +370,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > > monitor_printf(mon, "%s: %" PRIu64 "\n", > > MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), > > params->xbzrle_cache_size); > > + monitor_printf(mon, " %s: '%s'\n", > > + MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), > > + params->has_tls_authz ? params->tls_authz : ""); > > } > > > > qapi_free_MigrationParameters(params); > > @@ -1632,6 +1635,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > > p->tls_hostname->type = QTYPE_QSTRING; > > visit_type_str(v, param, &p->tls_hostname->u.s, &err); > > break; > > + case MIGRATION_PARAMETER_TLS_AUTHZ: > > + p->has_tls_authz = true; > > + p->tls_authz = g_new0(StrOrNull, 1); > > + p->tls_authz->type = QTYPE_QSTRING; > > + visit_type_str(v, param, &p->tls_authz->u.s, &err); > > + break; > > case MIGRATION_PARAMETER_MAX_BANDWIDTH: > > p->has_max_bandwidth = true; > > /* > > diff --git a/migration/migration.c b/migration/migration.c > > index 1e99ec9b7e..d14c8d7003 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -645,6 +645,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); > > params->has_max_bandwidth = true; > > params->max_bandwidth = s->parameters.max_bandwidth; > > params->has_downtime_limit = true; > > @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > > s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); > > } > > > > + if (params->has_tls_authz) { > > + g_free(s->parameters.tls_authz); > > + assert(params->tls_authz->type == QTYPE_QSTRING); > > + s->parameters.tls_authz = g_strdup(params->tls_authz->u.s); > > + } > > + > > if (params->has_max_bandwidth) { > > s->parameters.max_bandwidth = params->max_bandwidth; > > if (s->to_dst_file) { > > diff --git a/migration/tls.c b/migration/tls.c > > index 3b9e8c9263..5171afc6c4 100644 > > --- a/migration/tls.c > > +++ b/migration/tls.c > > @@ -94,7 +94,7 @@ void migration_tls_channel_process_incoming(MigrationState *s, > > > > tioc = qio_channel_tls_new_server( > > ioc, creds, > > - NULL, /* XXX pass ACL name */ > > + s->parameters.tls_authz, > > errp); > > if (!tioc) { > > return; > > diff --git a/qapi/migration.json b/qapi/migration.json > > index f7e10ee90f..b9ba34e3a6 100644 > > --- a/qapi/migration.json > > +++ b/qapi/migration.json > > @@ -488,6 +488,10 @@ > > # hostname must be provided so that the server's x509 > > # certificate identity can be validated. (Since 2.7) > > # > > +# @tls-authz: ID of the 'authz' object subclass that provides access control > > +# checking of the TLS x509 certificate distinguished name. (Since > > +# 2.13) > > +# > > Oops, 2.13 strikes again :-) > > Other than that, OK from migration and HMP. > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > # @max-bandwidth: to set maximum speed for migration. maximum speed in > > # bytes per second. (Since 2.8) > > # > > @@ -522,7 +526,7 @@ > > { 'enum': 'MigrationParameter', > > 'data': ['compress-level', 'compress-threads', 'decompress-threads', > > 'cpu-throttle-initial', 'cpu-throttle-increment', > > - 'tls-creds', 'tls-hostname', 'max-bandwidth', > > + 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', > > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > > 'x-multifd-channels', 'x-multifd-page-count', > > 'xbzrle-cache-size' ] } > > @@ -605,6 +609,7 @@ > > '*cpu-throttle-increment': 'int', > > '*tls-creds': 'StrOrNull', > > '*tls-hostname': 'StrOrNull', > > + '*tls-authz': 'StrOrNull', > > '*max-bandwidth': 'int', > > '*downtime-limit': 'int', > > '*x-checkpoint-delay': 'int', > > @@ -667,6 +672,10 @@ > > # associated with the migration URI, if any. (Since 2.9) > > # Note: 2.8 reports this by omitting tls-hostname instead. > > # > > +# @tls-authz: ID of the 'authz' object subclass that provides access control > > +# checking of the TLS x509 certificate distinguished name. (Since > > +# 2.13) > > +# > > # @max-bandwidth: to set maximum speed for migration. maximum speed in > > # bytes per second. (Since 2.8) > > # > > @@ -704,6 +713,7 @@ > > '*cpu-throttle-increment': 'uint8', > > '*tls-creds': 'str', > > '*tls-hostname': 'str', > > + '*tls-authz': 'str', > > '*max-bandwidth': 'size', > > '*downtime-limit': 'uint64', > > '*x-checkpoint-delay': 'uint32', > > -- > > 2.17.0 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> wrote: > From: "Daniel P. Berrange" <berrange@redhat.com> ..... It is not just the fault of this patch, but as you are the one doing the tls bits on migration... > @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); > } > > + if (params->has_tls_authz) { > + g_free(s->parameters.tls_authz); > + assert(params->tls_authz->type == QTYPE_QSTRING); We really try hard not to use assert() on migration code (yes, it is an ongoing effort). The code around this is something like: static void migrate_params_test_apply(MigrateSetParameters *params, MigrationParameters *dest) { [...] if (params->has_compress_level) { dest->compress_level = params->compress_level; } [...] if (params->has_tls_creds) { assert(params->tls_creds->type == QTYPE_QSTRING); dest->tls_creds = g_strdup(params->tls_creds->u.s); } [...] } Ok, tls code is the one with strings, but still. static void migrate_params_apply(MigrateSetParameters *params, Error **errp) { [...] if (params->has_compress_level) { s->parameters.compress_level = params->compress_level; } [...] if (params->has_tls_creds) { g_free(s->parameters.tls_creds); assert(params->tls_creds->type == QTYPE_QSTRING); s->parameters.tls_creds = g_strdup(params->tls_creds->u.s); } } And this other function: static bool migrate_params_check(MigrationParameters *params, Error **errp) { if (params->has_compress_level && (params->compress_level > 9)) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", "is invalid, it should be in the range of 0 to 9"); return false; } [...] } Where we don't check anything for tls. Perhaps we can move the asserts here? We can also try to merge migrate_params_check and migrate_params_test_apply() into a single function, but it is not completely trivial at the moment. Wondering if we can do it better. Later, Juan.
On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote: > Daniel P. Berrangé <berrange@redhat.com> wrote: > > From: "Daniel P. Berrange" <berrange@redhat.com> > > ..... > > > It is not just the fault of this patch, but as you are the one doing the > tls bits on migration... > > > > @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > > s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); > > } > > > > + if (params->has_tls_authz) { > > + g_free(s->parameters.tls_authz); > > + assert(params->tls_authz->type == QTYPE_QSTRING); > > We really try hard not to use assert() on migration code (yes, it is an > ongoing effort). The code around this is something like: > > static void migrate_params_test_apply(MigrateSetParameters *params, > MigrationParameters *dest) > { > [...] > > if (params->has_compress_level) { > dest->compress_level = params->compress_level; > } > > [...] > > if (params->has_tls_creds) { > assert(params->tls_creds->type == QTYPE_QSTRING); > dest->tls_creds = g_strdup(params->tls_creds->u.s); > } > [...] > } > > Ok, tls code is the one with strings, but still. My understanding is that because we declared this parameter to have "str" type in the QAPI schema, the QAPI code should already guarantee that "params->tls_creds->type == QTYPE_QSTRING" is true. IOW, the assert should never fail, and if it does, that would be a good thing as if QAPI wasn't validating input correctly something very bad has gone wrong. > static bool migrate_params_check(MigrationParameters *params, Error **errp) > { > if (params->has_compress_level && > (params->compress_level > 9)) { > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", > "is invalid, it should be in the range of 0 to 9"); > return false; This is different because QAPI schema merely declares it to be an int, so nothing in the QAPI input validation will do range checking. So this codepath is definitely reachable by users feeding bad input. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> wrote: > On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote: >> Daniel P. Berrangé <berrange@redhat.com> wrote: >> > From: "Daniel P. Berrange" <berrange@redhat.com> >> >> ..... >> >> >> It is not just the fault of this patch, but as you are the one doing the >> tls bits on migration... >> >> >> > @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) >> > s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); >> > } >> > >> > + if (params->has_tls_authz) { >> > + g_free(s->parameters.tls_authz); >> > + assert(params->tls_authz->type == QTYPE_QSTRING); >> >> We really try hard not to use assert() on migration code (yes, it is an >> ongoing effort). The code around this is something like: >> >> static void migrate_params_test_apply(MigrateSetParameters *params, >> MigrationParameters *dest) >> { >> [...] >> >> if (params->has_compress_level) { >> dest->compress_level = params->compress_level; >> } >> >> [...] >> >> if (params->has_tls_creds) { >> assert(params->tls_creds->type == QTYPE_QSTRING); >> dest->tls_creds = g_strdup(params->tls_creds->u.s); >> } >> [...] >> } >> >> Ok, tls code is the one with strings, but still. > > My understanding is that because we declared this parameter to > have "str" type in the QAPI schema, the QAPI code should already > guarantee that "params->tls_creds->type == QTYPE_QSTRING" is > true. > > IOW, the assert should never fail, and if it does, that would > be a good thing as if QAPI wasn't validating input correctly > something very bad has gone wrong. > > >> static bool migrate_params_check(MigrationParameters *params, Error **errp) >> { >> if (params->has_compress_level && >> (params->compress_level > 9)) { >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", >> "is invalid, it should be in the range of 0 to 9"); >> return false; > > This is different because QAPI schema merely declares it to be an > int, so nothing in the QAPI input validation will do range checking. > So this codepath is definitely reachable by users feeding bad input. ok then. Thanks for the explanation. Later, Juan.
diff --git a/hmp.c b/hmp.c index 74e18db103..bef8ea2531 100644 --- a/hmp.c +++ b/hmp.c @@ -370,6 +370,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%s: %" PRIu64 "\n", MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), params->xbzrle_cache_size); + monitor_printf(mon, " %s: '%s'\n", + MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), + params->has_tls_authz ? params->tls_authz : ""); } qapi_free_MigrationParameters(params); @@ -1632,6 +1635,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->tls_hostname->type = QTYPE_QSTRING; visit_type_str(v, param, &p->tls_hostname->u.s, &err); break; + case MIGRATION_PARAMETER_TLS_AUTHZ: + p->has_tls_authz = true; + p->tls_authz = g_new0(StrOrNull, 1); + p->tls_authz->type = QTYPE_QSTRING; + visit_type_str(v, param, &p->tls_authz->u.s, &err); + break; case MIGRATION_PARAMETER_MAX_BANDWIDTH: p->has_max_bandwidth = true; /* diff --git a/migration/migration.c b/migration/migration.c index 1e99ec9b7e..d14c8d7003 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -645,6 +645,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); params->has_max_bandwidth = true; params->max_bandwidth = s->parameters.max_bandwidth; params->has_downtime_limit = true; @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s); } + if (params->has_tls_authz) { + g_free(s->parameters.tls_authz); + assert(params->tls_authz->type == QTYPE_QSTRING); + s->parameters.tls_authz = g_strdup(params->tls_authz->u.s); + } + if (params->has_max_bandwidth) { s->parameters.max_bandwidth = params->max_bandwidth; if (s->to_dst_file) { diff --git a/migration/tls.c b/migration/tls.c index 3b9e8c9263..5171afc6c4 100644 --- a/migration/tls.c +++ b/migration/tls.c @@ -94,7 +94,7 @@ void migration_tls_channel_process_incoming(MigrationState *s, tioc = qio_channel_tls_new_server( ioc, creds, - NULL, /* XXX pass ACL name */ + s->parameters.tls_authz, errp); if (!tioc) { return; diff --git a/qapi/migration.json b/qapi/migration.json index f7e10ee90f..b9ba34e3a6 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -488,6 +488,10 @@ # hostname must be provided so that the server's x509 # certificate identity can be validated. (Since 2.7) # +# @tls-authz: ID of the 'authz' object subclass that provides access control +# checking of the TLS x509 certificate distinguished name. (Since +# 2.13) +# # @max-bandwidth: to set maximum speed for migration. maximum speed in # bytes per second. (Since 2.8) # @@ -522,7 +526,7 @@ { 'enum': 'MigrationParameter', 'data': ['compress-level', 'compress-threads', 'decompress-threads', 'cpu-throttle-initial', 'cpu-throttle-increment', - 'tls-creds', 'tls-hostname', 'max-bandwidth', + 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', 'x-multifd-channels', 'x-multifd-page-count', 'xbzrle-cache-size' ] } @@ -605,6 +609,7 @@ '*cpu-throttle-increment': 'int', '*tls-creds': 'StrOrNull', '*tls-hostname': 'StrOrNull', + '*tls-authz': 'StrOrNull', '*max-bandwidth': 'int', '*downtime-limit': 'int', '*x-checkpoint-delay': 'int', @@ -667,6 +672,10 @@ # associated with the migration URI, if any. (Since 2.9) # Note: 2.8 reports this by omitting tls-hostname instead. # +# @tls-authz: ID of the 'authz' object subclass that provides access control +# checking of the TLS x509 certificate distinguished name. (Since +# 2.13) +# # @max-bandwidth: to set maximum speed for migration. maximum speed in # bytes per second. (Since 2.8) # @@ -704,6 +713,7 @@ '*cpu-throttle-increment': 'uint8', '*tls-creds': 'str', '*tls-hostname': 'str', + '*tls-authz': 'str', '*max-bandwidth': 'size', '*downtime-limit': 'uint64', '*x-checkpoint-delay': 'uint32',