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

Matthias Bolte wrote: > 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 Hope this is the last patch I send on this topic! :) All fixed. ------------------------------------------------------------------------ -- Libvir-list mailing list Libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list