[libvirt] [PATCH]: Fix qemu+tls negotiation
All, While doing testing on the migration stuff, I noticed that a connection string using tls (in my case, qemu+tls://host/system) was hanging up trying to connect. I traced this down to a bug in the newer qemud negotiation implementation. What is happening is that we are forgetting to clear client->handshake to 0 after successfully doing a remoteAccessCheck(); this means we were never putting the '\1' byte on the transmit queue to be sent to the client, so the client was essentially waiting forever for the server to respond. Fix this by clearing the handshake once we've successfully done the handshake. Signed-off-by: Chris Lalancette <clalance@redhat.com>
On Tue, Mar 03, 2009 at 09:05:52AM +0100, Chris Lalancette wrote:
All, While doing testing on the migration stuff, I noticed that a connection string using tls (in my case, qemu+tls://host/system) was hanging up trying to connect. I traced this down to a bug in the newer qemud negotiation implementation. What is happening is that we are forgetting to clear client->handshake to 0 after successfully doing a remoteAccessCheck(); this means we were never putting the '\1' byte on the transmit queue to be sent to the client, so the client was essentially waiting forever for the server to respond. Fix this by clearing the handshake once we've successfully done the handshake.
Okay, that looks logical, nowhere in the code the handshake bit was set back to 0. I applied it myself since I expect a release in the next hours, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Mar 03, 2009 at 09:05:52AM +0100, Chris Lalancette wrote:
All, While doing testing on the migration stuff, I noticed that a connection string using tls (in my case, qemu+tls://host/system) was hanging up trying to connect. I traced this down to a bug in the newer qemud negotiation implementation. What is happening is that we are forgetting to clear client->handshake to 0 after successfully doing a remoteAccessCheck(); this means we were never putting the '\1' byte on the transmit queue to be sent to the client, so the client was essentially waiting forever for the server to respond. Fix this by clearing the handshake once we've successfully done the handshake.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
diff --git a/qemud/qemud.c b/qemud/qemud.c index e852841..fd315fc 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1339,6 +1339,8 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket /* Begin the TLS handshake. */ ret = gnutls_handshake (client->tlssession); if (ret == 0) { + client->handshake = 0; + /* Unlikely, but ... Next step is to check the certificate. */ if (remoteCheckAccess (client) == -1) goto cleanup;
This chunk is not required, because we have just VIR_ALLOC(client) and thus its memory is guarenteed all zero.
@@ -1930,6 +1932,8 @@ qemudDispatchClientHandshake(struct qemud_server *server, /* Continue the handshake. */ ret = gnutls_handshake (client->tlssession); if (ret == 0) { + client->handshake = 0; + /* Finished. Next step is to check the certificate. */ if (remoteCheckAccess (client) == -1) qemudDispatchClientFailure(client);
This bit must have been lost in the recent refactoring i did Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
diff --git a/qemud/qemud.c b/qemud/qemud.c index e852841..fd315fc 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1339,6 +1339,8 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket /* Begin the TLS handshake. */ ret = gnutls_handshake (client->tlssession); if (ret == 0) { + client->handshake = 0; + /* Unlikely, but ... Next step is to check the certificate. */ if (remoteCheckAccess (client) == -1) goto cleanup;
This chunk is not required, because we have just VIR_ALLOC(client) and thus its memory is guarenteed all zero.
Yeah, good point, it's not strictly necessary, but I guess harmless. Since DV committed it already, we can clean it up later.
@@ -1930,6 +1932,8 @@ qemudDispatchClientHandshake(struct qemud_server *server, /* Continue the handshake. */ ret = gnutls_handshake (client->tlssession); if (ret == 0) { + client->handshake = 0; + /* Finished. Next step is to check the certificate. */ if (remoteCheckAccess (client) == -1) qemudDispatchClientFailure(client);
This bit must have been lost in the recent refactoring i did
Right, this is the really important bit, and the one that makes it work for me. -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard