From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32994) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUHTO-0000o8-Qw for qemu-devel@nongnu.org; Fri, 12 Feb 2016 12:25:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUHTL-0002xu-2J for qemu-devel@nongnu.org; Fri, 12 Feb 2016 12:25:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38704) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUHTK-0002xl-R1 for qemu-devel@nongnu.org; Fri, 12 Feb 2016 12:25:34 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 5FDCEC0C236C for ; Fri, 12 Feb 2016 17:25:34 +0000 (UTC) Date: Fri, 12 Feb 2016 17:25:31 +0000 From: "Daniel P. Berrange" Message-ID: <20160212172531.GP24725@redhat.com> References: <1452599056-27357-1-git-send-email-berrange@redhat.com> <1452599056-27357-21-git-send-email-berrange@redhat.com> <20160212170951.GG2411@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160212170951.GG2411@work-vm> Subject: Re: [Qemu-devel] [PATCH v1 20/22] migration: support TLS encryption with TCP migration backend Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Amit Shah , qemu-devel@nongnu.org, Juan Quintela On Fri, Feb 12, 2016 at 05:09:52PM +0000, Dr. David Alan Gilbert wrote: > * Daniel P. Berrange (berrange@redhat.com) 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 :|