Re: [libvirt] [PATCH] phyp: ssh authentication with pub keys fixed

7 Nov 2009

2009/11/7 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
...
Matthias Bolte wrote:
...
2009/11/6 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
...
+    char *pubkey = NULL;
+    char *pvtkey = NULL;
+
+    if (virAsprintf(&pubkey, "%s/.ssh/id_rsa.pub", getenv("HOME")) < 0)
{
+        virReportOOMError(conn);
+        goto err;
+    }
+
+    if (virAsprintf(&pvtkey, "%s/.ssh/id_rsa", getenv("HOME")) < 0) {
+        virReportOOMError(conn);
+        goto err;
+    }
You should use virGetUserDirectory() instead of getenv("HOME"):
   char *userdir = virGetUserDirectory(NULL, geteuid());
   if (userdir == NULL)
       goto err;
Matthias
Here it is.
Thanks again.
[]'s
You included the old and the new version of this patch in the attached file.
...

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index a92046a..f96d2d6 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
[...]
@@ -282,10 +297,8 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
     /* Trying authentication by pubkey */
     while ((rc =
             libssh2_userauth_publickey_fromfile(session, username,
You assign conn->uri->user to username and use it without checking for
NULL. You should either check conn->uri->user for NULL in phypOpen(),
as you do it for conn->uri->server and conn->uri->path, and return
VIR_DRV_OPEN_ERROR if its NULL or request a username via the auth
callback if conn->uri->user is NULL.
...
-                                                "/home/user/"
-                                                ".ssh/id_rsa.pub",
-                                                "/home/user/"
-                                                ".ssh/id_rsa",
+                                                pubkey,
+                                                pvtkey,
                                                 password)) ==
The password (actually the passphrase) is NULL at this point. Is this
really working?
...
LIBSSH2_ERROR_EAGAIN) ;
     if (rc) {
So you fallback to username/password authentication if keyfile
authentication failed (rc != 0). According to the
libssh2_userauth_publickey_fromfile manpage it may return this error
codes:

LIBSSH2_ERROR_ALLOC - An internal memory allocation call failed.
LIBSSH2_ERROR_SOCKET_SEND - Unable to send data on socket.
LIBSSH2_ERROR_SOCKET_TIMEOUT
LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED - The username/public key
combination was invalid.
LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED - The username/public key
combination was invalid, or the signature for the supplied public key
was invalid.

IMHO its not useful to fallback to username/password authentication
for the first three possible errors, only if a keyfile related error
occurs like the last two.

I wonder which error code will be returned if one or both keyfiles
don't exist. Maybe you should check if both keyfiles exist before
calling libssh2_userauth_publickey_fromfile() and fallback to
username/password authentication if one or both are missing.
...
@@ -341,15 +354,22 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
             goto disconnect;
         } else
             goto exit;
+    } else {
+        goto exit;
     }
   disconnect:
     libssh2_session_disconnect(session, "Disconnecting...");
     libssh2_session_free(session);
   err:
+    VIR_FREE(userhome);
+    VIR_FREE(pubkey);
+    VIR_FREE(pvtkey);
     VIR_FREE(password);
     return NULL;
exit:
+    VIR_FREE(userhome);
VIR_FREE(pubkey) is missing here, it's there in the first version of this patch.
...
+    VIR_FREE(pvtkey);
     VIR_FREE(password);
     return session;
 }
Matthias

    