Re: [Qemu-devel] [PATCH v1 20/22] migration: support TLS encryption with TCP migration backend

[Dr. David Alan Gilbert]

* Daniel P. Berrange (address@hidden) wrote:
> This extends the TCP migration backend so that it can make use
> of QIOChannelTLS to provide transparent TLS encryption. To
> trigger enablement the URI on the incoming and outgoing sides
> should have 'tls-creds=ID' appended, eg
> 
>    tcp:$HOST:$PORT,tls-creds=ID
> 
> where ID is the object identifier of a set of TLS credentials
> previously created using object_add / -object. There is not
> currently any support for checking the migration client
> certificate names against ACLs. This is pending a conversion
> of the ACL code to QOM.

Does that change the option passed or is that just different
in the way the tls-creds are set up?

> There is no support for dynamically negotiating use of TLS
> between the incoming/outgoing side. Both sides must agree
> on use of TLS out of band and set the URI accordingly. In
> practice it is expected that the administrator will just
> turn on use of TLS on their hosts in the libvirt config
> and then libvirt will instruct QEMU to use TLS for migration.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>

> ---
>  migration/tcp.c | 273 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qemu-options.hx |   7 +-
>  2 files changed, 264 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/tcp.c b/migration/tcp.c
> index ac73977..bef861c 100644
> --- a/migration/tcp.c
> +++ b/migration/tcp.c
> @@ -22,6 +22,8 @@
>  #include "migration/migration.h"
>  #include "migration/qemu-file.h"
>  #include "io/channel-socket.h"
> +#include "io/channel-tls.h"
> +#include "crypto/tlscreds.h"
>  
>  //#define DEBUG_MIGRATION_TCP
>  
> @@ -33,6 +35,22 @@
>      do { } while (0)
>  #endif
>  
> +typedef struct {
> +    MigrationState *s;
> +    QCryptoTLSCreds *tlscreds;
> +    char *hostname;
> +} TCPConnectData;
> +
> +typedef struct {
> +    MigrationState *s;
> +    QCryptoTLSCreds *tlscreds;
> +} TCPListenData;
> +
> +typedef struct {
> +    MigrationState *s;
> +    QIOChannel *ioc;
> +} TCPConnectTLSData;
> +

what makes it TCP specific rather than sharing most of this between transports? 
i.e. should
this work for fd migration ? (rdma is probably not reasonable
giving it's scribbling directly in the other hosts RAM).
Certainly those types dont really look TCP specific.

>  static SocketAddress *tcp_build_address(const char *host_port, Error **errp)
>  {
> @@ -51,21 +69,174 @@ static SocketAddress *tcp_build_address(const char 
> *host_port, Error **errp)
>  }
>  
>  
> +static void tcp_connect_data_free(gpointer opaque)
> +{
> +    TCPConnectData *data = opaque;
> +    if (!data) {
> +        return;
> +    }
> +    g_free(data->hostname);
> +    object_unref(OBJECT(data->tlscreds));
> +    g_free(data);
> +}
> +
> +
> +static void tcp_listen_data_free(gpointer opaque)
> +{
> +    TCPListenData *data = opaque;
> +    if (!data) {
> +        return;
> +    }
> +    object_unref(OBJECT(data->tlscreds));
> +    g_free(data);
> +}
> +
> +
> +static void tcp_connect_tls_data_free(gpointer opaque)
> +{
> +    TCPConnectTLSData *data = opaque;
> +    if (!data) {
> +        return;
> +    }
> +    object_unref(OBJECT(data->ioc));
> +    g_free(data);
> +}
> +
> +
> +static char *tcp_get_opt_str(const char *host_port,
> +                             const char *key)
> +{
> +    const char *offset, *end;
> +
> +    offset = strstr(host_port, key);
> +    if (!offset) {
> +        return NULL;
> +    }
> +
> +    offset += strlen(key);
> +    if (offset[0] != '=') {
> +        return NULL;
> +    }
> +    offset++;
> +    end = strchr(offset, ',');
> +    if (end) {
> +        return g_strndup(offset, end - offset);
> +    } else {
> +        return g_strdup(offset);
> +    }
> +}
> +
> +
> +static QCryptoTLSCreds *tcp_get_tls_creds(const char *host_port,
> +                                          bool is_listen,
> +                                          Error **errp)
> +{
> +    char *credname = NULL;
> +    Object *creds;
> +    QCryptoTLSCreds *ret;
> +
> +    credname = tcp_get_opt_str(host_port, "tls-creds");
> +    if (!credname) {
> +        return NULL;
> +    }

At what point does it get saner just to throw host_port into a qemu_opts 
and let it parse it?

> +    creds = object_resolve_path_component(
> +        object_get_objects_root(), credname);
> +    if (!creds) {
> +        error_setg(errp, "No TLS credentials with id '%s'",
> +                   credname);
> +        goto error;
> +    }
> +    ret = (QCryptoTLSCreds *)object_dynamic_cast(
> +        creds, TYPE_QCRYPTO_TLS_CREDS);
> +    if (!ret) {
> +        error_setg(errp, "Object with id '%s' is not TLS credentials",
> +                   credname);
> +        goto error;
> +    }
> +    if (is_listen) {
> +        if (ret->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
> +            error_setg(errp, "%s",
> +                       "Expected TLS credentials for server endpoint");
> +            goto error;
> +        }
> +    } else {
> +        if (ret->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
> +            error_setg(errp, "%s",
> +                       "Expected TLS credentials for client endpoint");
> +            goto error;
> +        }
> +    }
> +
> +    g_free(credname);
> +    object_ref(OBJECT(ret));
> +    return ret;
> +
> + error:
> +    g_free(credname);
> +    return NULL;
> +}
> +
> +
> +static void tcp_outgoing_migration_tls(Object *src,
> +                                       Error *err,
> +                                       gpointer opaque)
> +{
> +    TCPConnectTLSData *data = opaque;
> +    QIOChannel *ioc = QIO_CHANNEL(src);
> +
> +    if (err) {
> +        DPRINTF("migrate connect TLS error: %s\n", error_get_pretty(err));

error_report ?

> +        data->s->file = NULL;
> +        migrate_fd_error(data->s);
> +    } else {
> +        DPRINTF("migrate connect success\n");

trace_

> +        data->s->file = qemu_fopen_channel_output(ioc);
> +        migrate_fd_connect(data->s);
> +    }
> +}
> +
> +
>  static void tcp_outgoing_migration(Object *src,
>                                     Error *err,
>                                     gpointer opaque)
>  {
> -    MigrationState *s = opaque;
> +    TCPConnectData *data = opaque;
>      QIOChannel *sioc = QIO_CHANNEL(src);
>  
>      if (err) {
>          DPRINTF("migrate connect error: %s\n", error_get_pretty(err));
> -        s->file = NULL;
> -        migrate_fd_error(s);
> +        data->s->file = NULL;
> +        migrate_fd_error(data->s);
>      } else {
> -        DPRINTF("migrate connect success\n");
> -        s->file = qemu_fopen_channel_output(sioc);
> -        migrate_fd_connect(s);
> +        if (data->tlscreds) {
> +            Error *local_err = NULL;
> +            QIOChannelTLS *tioc = qio_channel_tls_new_client(
> +                sioc, data->tlscreds, data->hostname,
> +                &local_err);
> +            if (!tioc) {
> +                DPRINTF("migrate tls setup error: %s\n",
> +                        error_get_pretty(local_err));

error_report ?  More below I think - just make sure that
any errors normally get logged.

> +                error_free(local_err);
> +                data->s->file = NULL;
> +                migrate_fd_error(data->s);
> +            } else {
> +                TCPConnectTLSData *tdata =
> +                    g_new0(TCPConnectTLSData, 1);
> +                DPRINTF("migrate connect tls handshake begin\n");
> +                tdata->s = data->s;
> +                tdata->ioc = sioc;
> +                object_ref(OBJECT(sioc));
> +                qio_channel_tls_handshake(tioc,
> +                                          tcp_outgoing_migration_tls,
> +                                          tdata,
> +                                          tcp_connect_tls_data_free);
> +            }
> +        } else {
> +            DPRINTF("migrate connect success\n");
> +            data->s->file = qemu_fopen_channel_output(sioc);
> +            migrate_fd_connect(data->s);
> +        }
>      }
>      object_unref(src);
>  }
> @@ -77,21 +248,56 @@ void tcp_start_outgoing_migration(MigrationState *s,
>  {
>      SocketAddress *saddr = tcp_build_address(host_port, errp);
>      QIOChannelSocket *sioc;
> +    Error *local_err = NULL;
> +    QCryptoTLSCreds *creds;
> +    TCPConnectData *data;
>  
>      if (!saddr) {
>          return;
>      }
>  
> +    creds = tcp_get_tls_creds(host_port, false, errp);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        qapi_free_SocketAddress(saddr);
> +        return;
> +    }
> +
> +    data = g_new0(TCPConnectData, 1);
> +    data->s = s;
> +    if (creds) {
> +        data->hostname = g_strdup(saddr->u.inet->host);
> +        data->tlscreds = creds;
> +    }
> +
>      sioc = qio_channel_socket_new();
>      qio_channel_socket_connect_async(sioc,
>                                       saddr,
>                                       tcp_outgoing_migration,
> -                                     s,
> -                                     NULL);
> +                                     data,
> +                                     tcp_connect_data_free);
>      qapi_free_SocketAddress(saddr);
>  }
>  
>  
> +static void tcp_incoming_migration_tls(Object *src,
> +                                       Error *err,
> +                                       gpointer opaque)
> +{
> +    QIOChannel *ioc = QIO_CHANNEL(src);
> +
> +    if (err) {
> +        DPRINTF("migrate listen TLS error: %s\n", error_get_pretty(err));
> +        object_unref(OBJECT(ioc));
> +    } else {
> +        DPRINTF("migrate listen success\n");
> +        QEMUFile *f = qemu_fopen_channel_input(ioc);
> +
> +        process_incoming_migration(f);
> +    }
> +}
> +
> +
>  static gboolean tcp_accept_incoming_migration(QIOChannel *ioc,
>                                                GIOCondition condition,
>                                                gpointer opaque)
> @@ -99,6 +305,7 @@ static gboolean tcp_accept_incoming_migration(QIOChannel 
> *ioc,
>      QEMUFile *f;
>      QIOChannelSocket *cioc;
>      Error *err = NULL;
> +    TCPListenData *data = opaque;
>  
>      cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
>                                       &err);
> @@ -108,16 +315,38 @@ static gboolean 
> tcp_accept_incoming_migration(QIOChannel *ioc,
>          goto out;
>      }
>  
> -    DPRINTF("accepted migration\n");
> +    if (data->tlscreds) {
> +        DPRINTF("Starting client TLS\n");
> +        QIOChannelTLS *tioc = qio_channel_tls_new_server(
> +            QIO_CHANNEL(cioc), data->tlscreds,
> +            NULL, /* XXX pass ACL name */
> +            &err);
> +        object_unref(OBJECT(cioc));
> +        if (!tioc) {
> +            DPRINTF("migrate tls setup error: %s\n",
> +                    error_get_pretty(err));
> +            error_free(err);
> +            goto out;
> +        } else {
> +            DPRINTF("migrate connect tls handshake begin\n");
> +            qio_channel_tls_handshake(tioc,
> +                                      tcp_incoming_migration_tls,
> +                                      NULL,
> +                                      NULL);
> +        }
> +    } else {
> +        DPRINTF("accepted migration\n");
>  
> -    f = qemu_fopen_channel_input(QIO_CHANNEL(cioc));
> -    object_unref(OBJECT(cioc));
> +        f = qemu_fopen_channel_input(QIO_CHANNEL(cioc));
> +        object_unref(OBJECT(cioc));
>  
> -    process_incoming_migration(f);
> +        process_incoming_migration(f);
> +    }
>  
>  out:
>      /* Close listening socket as its no longer needed */
>      qio_channel_close(ioc, NULL);
> +    object_unref(OBJECT(ioc));
>      return FALSE;
>  }
>  
> @@ -126,23 +355,39 @@ void tcp_start_incoming_migration(const char 
> *host_port, Error **errp)
>  {
>      SocketAddress *saddr = tcp_build_address(host_port, errp);
>      QIOChannelSocket *listen_ioc;
> +    TCPListenData *data;
> +    Error *local_err = NULL;
> +    QCryptoTLSCreds *creds;
>  
>      if (!saddr) {
>          return;
>      }
>  
> +    creds = tcp_get_tls_creds(host_port, true, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        qapi_free_SocketAddress(saddr);
> +        return;
> +    }
> +
>      listen_ioc = qio_channel_socket_new();
>      if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
>          object_unref(OBJECT(listen_ioc));
>          qapi_free_SocketAddress(saddr);
> +        object_unref(OBJECT(creds));
>          return;
>      }
>  
> +    data = g_new0(TCPListenData, 1);
> +    if (creds) {
> +        data->tlscreds = creds;
> +    }
> +
>      qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
>                            G_IO_IN,
>                            tcp_accept_incoming_migration,
> -                          listen_ioc,
> -                          (GDestroyNotify)object_unref);
> +                          data,
> +                          tcp_listen_data_free);
>  
>      qapi_free_SocketAddress(saddr);
>  }
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 215d00d..3f16cfc 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3364,7 +3364,7 @@ Set TB size.
>  ETEXI
>  
>  DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
> -    "-incoming tcp:[host]:port[,to=maxport][,ipv4][,ipv6]\n" \
> +    "-incoming tcp:[host]:port[,to=maxport][,ipv4][,ipv6][,tls-creds=ID]\n" \
>      "-incoming rdma:host:port[,ipv4][,ipv6]\n" \
>      "-incoming unix:socketpath\n" \
>      "                prepare for incoming migration, listen on\n" \
> @@ -3377,11 +3377,14 @@ DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
>      "                wait for the URI to be specified via 
> migrate_incoming\n",
>      QEMU_ARCH_ALL)
>  STEXI
> address@hidden -incoming 
> tcp:address@hidden:@var{port}[,address@hidden,ipv4][,ipv6]
> address@hidden -incoming 
> tcp:address@hidden:@var{port}[,address@hidden,ipv4][,ipv6][,tls-creds=ID]
>  @itemx -incoming rdma:@var{host}:@var{port}[,ipv4][,ipv6]
>  @findex -incoming
>  Prepare for incoming migration, listen on a given tcp port.
>  
> +If the @var{tls-creds} parameter is specified, it should refer to the ID
> +of a TLS credentials object previously created with @var{-object}.
> +
>  @item -incoming unix:@var{socketpath}
>  Prepare for incoming migration, listen on a given unix socket.
>  
> -- 
> 2.5.0
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK