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

2009/11/9 Eduardo Otubo <otubo(a)linux.vnet.ibm.com&gt;: > Matthias Bolte wrote: >>> 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. > Ok. > >>> - "/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? > Talking with libssh2 guys, this feature is not exactly working well, they > said that it is possible to pass a random passphrase (or even NULL) that it > will authenticate using pub and pvt keys. So, I assumed this as a hardcoded > NULL just until they fix this function. Hm, okay. May be you should add a comment about this. >>> 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. > Appearently, going further the man pages and tracing all the function return > points, I figured out that this function may also return > LIBSSH2_ERROR_SOCKET_NONE or LIBSSH2_ERROR_NONE for many reasons. As far as > I understand, LIBSSH2_ERROR_NONE is for a succesful pubkey authentication, > and LIBSSH2_ERROR_SOCKET_NONE is for a non succesful. Adjusted all values > for this if construction. > >> 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. > In this case I explicit check for errors (LIBSSH2_ERROR_ALLOC, > LIBSSH2_ERROR_SOCKET_SEND and LIBSSH2_ERROR_SOCKET_TIMEOUT) before fallback. > >> 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. > Ok. I am stating files now. > >>> @@ -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. > Ok. > > > Thanks again :) > []'s > diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c > index a92046a..94581b2 100644 > --- a/src/phyp/phyp_driver.c > +++ b/src/phyp/phyp_driver.c [...] > @@ -280,15 +302,19 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, > } > > /* Trying authentication by pubkey */ > + if (stat(pvtkey, &pvt_stat) || stat(pubkey, &pub_stat)) You could have used access(pvtkey, R_OK) instead, but stat() is okay. Don't you want to try username/password authentication in case of missing keyfiles? Instead you goto err. > + goto err; > + > while ((rc = > libssh2_userauth_publickey_fromfile(session, username, > - "/home/user/" > - ".ssh/id_rsa.pub", > - "/home/user/" > - ".ssh/id_rsa", > - password)) == > + pubkey, > + pvtkey, > + NULL)) == > LIBSSH2_ERROR_EAGAIN) ; > - if (rc) { > + > + if (rc == LIBSSH2_ERROR_NONE Didn't you say LIBSSH2_ERROR_NONE would be returned in case of successful authentication? I think you wanted to check for LIBSSH2_ERROR_SOCKET_NONE here, didn't you? > + || rc == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED > + || rc == LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) { > int i; > int hasPassphrase = 0; > Keep it up, you'll get this patch right :-) Matthias