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

[Daniel P. Berrange]

On Fri, Feb 12, 2016 at 05:09:52PM +0000, Dr. David Alan Gilbert wrote:
> * 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?

It will mean another new paramter will be added. For example
the above command will become something like this:

   tcp:$HOST:$PORT,tls-creds=ID,auth=ID

where 'auth=ID' provides the ID of an object implementing
the authentication/authorization check

> > +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.

TLS as a protocol doesn't have any limitations, but as part of having
the client validate the x509 certificates, it needs to have a hostname
or IP address to validate against the certificate. This is available
for TCP and RDMA, but there's no user provided hostname for unix,
exec, and fd migration protocols.

We could extend the syntax for each of those, so that the user could
provide a hostname, and that would then allow us to wire up TLS for
that backend. If we did that, then it would make sense to have the
TLS setup in a separate migration/tls.c file, that we could activate
over all channels.

> > +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?

Possibly quite soon :-)

> > +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 ?

Is it desirable for code triggered from the QMP/HMP monitor to be
doing that. I thought we were not supposed todo that in general.
Normally we could propagate an error back to the monitor, but
since migrate happens in the background, we've no way todo that.
We really need to be able to feed the errors back to the client
app over QMP somehow, but there's no mechanism for that right
now :-(

Perhaps 'info migrate' should have a field to report the actual
error message when status == failed ?  Or we could define an
async event that we emit to the client with the actual error.
Either would make life nicer for libvirt, which can't report
any good errors - this is the main reason libvirt currently
does all TCP connection setup itself and uses fd:, and not
tcp:.  Libvirt will need to use tcp: again though, to get
TLS working, unless we add a 'hostname' field to the 'fd:'
protocol....

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

Yep, I mentioned in a repl to a previous review, that I
intend to do a single cleanup patch at the end which
adds some consistent trace points across all the backend
drivers for migrate.

> > +        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.

Same issue as above.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|