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:25:31PM +0000, Daniel P. Berrange wrote:
> 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 :-)

So, having thought about this some more, I think rather than munging
parameters onto the end of the URI, it'll make more sense to use the
'migrate-set-parameters' QMP call ie. to enable use of tls

  migrate-set-parameters tls-creds=tls0

and then to deal with the problem I mention above about not having a
hostname available for fd/exec migration, we can also allow

  migrate-set-parameters tls-hostname=peerhostname

which would set the hostname to be used to validate the x509 certificate.
This would be quite nice for libvirt, since we can carry on using the
fd: migration and establish the connection ourselves, while letting QEMU
do the x509 validation.

This would in turn motivate moving of the TLS IO Channel creation into
a separate file, instead of having it inline in tcp.c. This would in
turn let me address the feedback you had previous about possibility of
unix: and tcp: code being dealt with in the same file to avoid the
code duplication.

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 :|