diff mbox

[PULL,25/28] migration: define 'tls-creds' and 'tls-hostname' migration parameters

Message ID 69ef1f36b0f882fc5ba9491fb272fa5f83ac1d3d.1464242913.git.amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah May 26, 2016, 6:12 a.m. UTC 
From: "Daniel P. Berrange" <berrange@redhat.com>

Define two new migration parameters to be used with TLS encryption.
The 'tls-creds' parameter provides the ID of an instance of the
'tls-creds' object type, or rather a subclass such as 'tls-creds-x509'.
Providing these credentials will enable use of TLS on the migration
data stream.

If using x509 certificates, together with a migration URI that does
not include a hostname, the 'tls-hostname' parameter provides the
hostname to use when verifying the server's x509 certificate. This
allows TLS to be used in combination with fd: and exec: protocols
where a TCP connection is established by a 3rd party outside of
QEMU.

NB, this requires changing the migrate_set_parameter method in the
HMP to accept a 's' (string) value instead of 'i' (integer). This
is backwards compatible, because the parsing of strings allows the
quotes to be optional, thus any integer is also a valid string.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1461751518-12128-26-git-send-email-berrange@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hmp-commands.hx       |  2 +-
 hmp.c                 | 44 ++++++++++++++++++++++++++++++++------
 migration/migration.c | 14 +++++++++++++
 qapi-schema.json      | 58 ++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 108 insertions(+), 10 deletions(-)

Comments

Eric Blake May 26, 2016, 3:05 p.m. UTC | #1 
On 05/26/2016 12:12 AM, Amit Shah wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> Define two new migration parameters to be used with TLS encryption.
> The 'tls-creds' parameter provides the ID of an instance of the
> 'tls-creds' object type, or rather a subclass such as 'tls-creds-x509'.
> Providing these credentials will enable use of TLS on the migration
> data stream.
> 

> +++ b/qapi-schema.json

> +# @tls-hostname: hostname of the target host for the migration. This is
> +#                required when using x509 based TLS credentials and the
> +#                migration URI does not already include a hostname. For
> +#                example if using fd: or exec: based migration, the
> +#                hostname must be provided so that the server's x509
> +#                certificate identity canbe validated. (Since 2.7)

s/canbe/can be/


> +#
> +# @tls-hostname: hostname of the target host for the migration. This is
> +#                required when using x509 based TLS credentials and the
> +#                migration URI does not already include a hostname. For
> +#                example if using fd: or exec: based migration, the
> +#                hostname must be provided so that the server's x509
> +#                certificate identity canbe validated. (Since 2.7)

and again

> @@ -667,6 +702,21 @@
>  #                          auto-converge detects that migration is not making
>  #                          progress. The default value is 10. (Since 2.7)
>  #
> +# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> +#             establishing a TLS connection over the migration data channel.
> +#             On the outgoing side of the migration, the credentials must
> +#             be for a 'client' endpoint, while for the incoming side the
> +#             credentials must be for a 'server' endpoint. Setting this
> +#             will enable TLS for all migrations. The default is unset,
> +#             resulting in unsecured migration at the QEMU level. (Since 2.6)

Missed a swap to call out 2.7

> +#
> +# @tls-hostname: hostname of the target host for the migration. This is
> +#                required when using x509 based TLS credentials and the
> +#                migration URI does not already include a hostname. For
> +#                example if using fd: or exec: based migration, the
> +#                hostname must be provided so that the server's x509
> +#                certificate identity canbe validated. (Since 2.6)

can be, 2.7
Amit Shah May 27, 2016, 10:02 a.m. UTC | #2 
On (Thu) 26 May 2016 [09:05:37], Eric Blake wrote:

> > @@ -667,6 +702,21 @@
> >  #                          auto-converge detects that migration is not making
> >  #                          progress. The default value is 10. (Since 2.7)
> >  #
> > +# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> > +#             establishing a TLS connection over the migration data channel.
> > +#             On the outgoing side of the migration, the credentials must
> > +#             be for a 'client' endpoint, while for the incoming side the
> > +#             credentials must be for a 'server' endpoint. Setting this
> > +#             will enable TLS for all migrations. The default is unset,
> > +#             resulting in unsecured migration at the QEMU level. (Since 2.6)
> 
> Missed a swap to call out 2.7

Ugh, I did miss a few didn't I?

Dan, can you please follow up with the fixes Eric sugests?

Thanks,



		Amit
Daniel P. Berrangé May 31, 2016, 9:22 a.m. UTC | #3 
On Fri, May 27, 2016 at 03:32:14PM +0530, Amit Shah wrote:
> On (Thu) 26 May 2016 [09:05:37], Eric Blake wrote:
> 
> > > @@ -667,6 +702,21 @@
> > >  #                          auto-converge detects that migration is not making
> > >  #                          progress. The default value is 10. (Since 2.7)
> > >  #
> > > +# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> > > +#             establishing a TLS connection over the migration data channel.
> > > +#             On the outgoing side of the migration, the credentials must
> > > +#             be for a 'client' endpoint, while for the incoming side the
> > > +#             credentials must be for a 'server' endpoint. Setting this
> > > +#             will enable TLS for all migrations. The default is unset,
> > > +#             resulting in unsecured migration at the QEMU level. (Since 2.6)
> > 
> > Missed a swap to call out 2.7
> 
> Ugh, I did miss a few didn't I?
> 
> Dan, can you please follow up with the fixes Eric sugests?

Yes, will do.

Regards,
Daniel
diff mbox

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4f4f60a..98b4b1a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1008,7 +1008,7 @@  ETEXI
 
     {
         .name       = "migrate_set_parameter",
-        .args_type  = "parameter:s,value:i",
+        .args_type  = "parameter:s,value:s",
         .params     = "parameter value",
         .help       = "Set the parameter for migration",
         .mhandler.cmd = hmp_migrate_set_parameter,
diff --git a/hmp.c b/hmp.c
index a464ca9..a4b1d3d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -294,6 +294,12 @@  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, " %s: %" PRId64,
             MigrationParameter_lookup[MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT],
             params->cpu_throttle_increment);
+        monitor_printf(mon, " %s: '%s'",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_CREDS],
+            params->tls_creds ? : "");
+        monitor_printf(mon, " %s: '%s'",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
+            params->tls_hostname ? : "");
         monitor_printf(mon, "\n");
     }
 
@@ -1243,13 +1249,17 @@  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
 void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
 {
     const char *param = qdict_get_str(qdict, "parameter");
-    int value = qdict_get_int(qdict, "value");
+    const char *valuestr = qdict_get_str(qdict, "value");
+    long valueint = 0;
     Error *err = NULL;
     bool has_compress_level = false;
     bool has_compress_threads = false;
     bool has_decompress_threads = false;
     bool has_cpu_throttle_initial = false;
     bool has_cpu_throttle_increment = false;
+    bool has_tls_creds = false;
+    bool has_tls_hostname = false;
+    bool use_int_value = false;
     int i;
 
     for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
@@ -1257,25 +1267,46 @@  void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
             switch (i) {
             case MIGRATION_PARAMETER_COMPRESS_LEVEL:
                 has_compress_level = true;
+                use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_COMPRESS_THREADS:
                 has_compress_threads = true;
+                use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
                 has_decompress_threads = true;
+                use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
                 has_cpu_throttle_initial = true;
+                use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
                 has_cpu_throttle_increment = true;
                 break;
+            case MIGRATION_PARAMETER_TLS_CREDS:
+                has_tls_creds = true;
+                break;
+            case MIGRATION_PARAMETER_TLS_HOSTNAME:
+                has_tls_hostname = true;
+                break;
+            }
+
+            if (use_int_value) {
+                if (qemu_strtol(valuestr, NULL, 10, &valueint) < 0) {
+                    error_setg(&err, "Unable to parse '%s' as an int",
+                               valuestr);
+                    goto cleanup;
+                }
             }
-            qmp_migrate_set_parameters(has_compress_level, value,
-                                       has_compress_threads, value,
-                                       has_decompress_threads, value,
-                                       has_cpu_throttle_initial, value,
-                                       has_cpu_throttle_increment, value,
+
+            qmp_migrate_set_parameters(has_compress_level, valueint,
+                                       has_compress_threads, valueint,
+                                       has_decompress_threads, valueint,
+                                       has_cpu_throttle_initial, valueint,
+                                       has_cpu_throttle_increment, valueint,
+                                       has_tls_creds, valuestr,
+                                       has_tls_hostname, valuestr,
                                        &err);
             break;
         }
@@ -1285,6 +1316,7 @@  void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         error_setg(&err, QERR_INVALID_PARAMETER, param);
     }
 
+ cleanup:
     if (err) {
         error_report_err(err);
     }
diff --git a/migration/migration.c b/migration/migration.c
index 27064af..9b037ab 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -536,6 +536,8 @@  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->decompress_threads = s->parameters.decompress_threads;
     params->cpu_throttle_initial = s->parameters.cpu_throttle_initial;
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
+    params->tls_creds = g_strdup(s->parameters.tls_creds);
+    params->tls_hostname = g_strdup(s->parameters.tls_hostname);
 
     return params;
 }
@@ -737,6 +739,10 @@  void qmp_migrate_set_parameters(bool has_compress_level,
                                 int64_t cpu_throttle_initial,
                                 bool has_cpu_throttle_increment,
                                 int64_t cpu_throttle_increment,
+                                bool has_tls_creds,
+                                const char *tls_creds,
+                                bool has_tls_hostname,
+                                const char *tls_hostname,
                                 Error **errp)
 {
     MigrationState *s = migrate_get_current();
@@ -788,6 +794,14 @@  void qmp_migrate_set_parameters(bool has_compress_level,
     if (has_cpu_throttle_increment) {
         s->parameters.cpu_throttle_increment = cpu_throttle_increment;
     }
+    if (has_tls_creds) {
+        g_free(s->parameters.tls_creds);
+        s->parameters.tls_creds = g_strdup(tls_creds);
+    }
+    if (has_tls_hostname) {
+        g_free(s->parameters.tls_hostname);
+        s->parameters.tls_hostname = g_strdup(tls_hostname);
+    }
 }
 
 
diff --git a/qapi-schema.json b/qapi-schema.json
index e8c0353..8483bdf 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -617,11 +617,28 @@ 
 # @cpu-throttle-increment: throttle percentage increase each time
 #                          auto-converge detects that migration is not making
 #                          progress. The default value is 10. (Since 2.7)
+#
+# @tls-creds: ID of the 'tls-creds' object that provides credentials for
+#             establishing a TLS connection over the migration data channel.
+#             On the outgoing side of the migration, the credentials must
+#             be for a 'client' endpoint, while for the incoming side the
+#             credentials must be for a 'server' endpoint. Setting this
+#             will enable TLS for all migrations. The default is unset,
+#             resulting in unsecured migration at the QEMU level. (Since 2.7)
+#
+# @tls-hostname: hostname of the target host for the migration. This is
+#                required when using x509 based TLS credentials and the
+#                migration URI does not already include a hostname. For
+#                example if using fd: or exec: based migration, the
+#                hostname must be provided so that the server's x509
+#                certificate identity canbe validated. (Since 2.7)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
-           'cpu-throttle-initial', 'cpu-throttle-increment'] }
+           'cpu-throttle-initial', 'cpu-throttle-increment',
+           'tls-creds', 'tls-hostname'] }
 
 #
 # @migrate-set-parameters
@@ -641,6 +658,22 @@ 
 # @cpu-throttle-increment: throttle percentage increase each time
 #                          auto-converge detects that migration is not making
 #                          progress. The default value is 10. (Since 2.7)
+#
+# @tls-creds: ID of the 'tls-creds' object that provides credentials for
+#             establishing a TLS connection over the migration data channel.
+#             On the outgoing side of the migration, the credentials must
+#             be for a 'client' endpoint, while for the incoming side the
+#             credentials must be for a 'server' endpoint. Setting this
+#             will enable TLS for all migrations. The default is unset,
+#             resulting in unsecured migration at the QEMU level. (Since 2.7)
+#
+# @tls-hostname: hostname of the target host for the migration. This is
+#                required when using x509 based TLS credentials and the
+#                migration URI does not already include a hostname. For
+#                example if using fd: or exec: based migration, the
+#                hostname must be provided so that the server's x509
+#                certificate identity canbe validated. (Since 2.7)
+#
 # Since: 2.4
 ##
 { 'command': 'migrate-set-parameters',
@@ -648,7 +681,9 @@ 
             '*compress-threads': 'int',
             '*decompress-threads': 'int',
             '*cpu-throttle-initial': 'int',
-            '*cpu-throttle-increment': 'int'} }
+            '*cpu-throttle-increment': 'int',
+            '*tls-creds': 'str',
+            '*tls-hostname': 'str'} }
 
 #
 # @MigrationParameters
@@ -667,6 +702,21 @@ 
 #                          auto-converge detects that migration is not making
 #                          progress. The default value is 10. (Since 2.7)
 #
+# @tls-creds: ID of the 'tls-creds' object that provides credentials for
+#             establishing a TLS connection over the migration data channel.
+#             On the outgoing side of the migration, the credentials must
+#             be for a 'client' endpoint, while for the incoming side the
+#             credentials must be for a 'server' endpoint. Setting this
+#             will enable TLS for all migrations. The default is unset,
+#             resulting in unsecured migration at the QEMU level. (Since 2.6)
+#
+# @tls-hostname: hostname of the target host for the migration. This is
+#                required when using x509 based TLS credentials and the
+#                migration URI does not already include a hostname. For
+#                example if using fd: or exec: based migration, the
+#                hostname must be provided so that the server's x509
+#                certificate identity canbe validated. (Since 2.6)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -674,7 +724,9 @@ 
             'compress-threads': 'int',
             'decompress-threads': 'int',
             'cpu-throttle-initial': 'int',
-            'cpu-throttle-increment': 'int'} }
+            'cpu-throttle-increment': 'int',
+            'tls-creds': 'str',
+            'tls-hostname': 'str'} }
 ##
 # @query-migrate-parameters
 #