[libvirt] [PATCH] phyp: ssh authentication with pub keys fixed
Eduardo Otubo
otubo at linux.vnet.ibm.com
Tue Nov 10 03:29:14 UTC 2009
Matthias Bolte wrote:
> 2009/11/9 Eduardo Otubo <otubo at linux.vnet.ibm.com>:
>> 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.
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: phyp_sshkeys.patch
Type: text/x-patch
Size: 3143 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091110/7f03a844/attachment-0001.bin>
More information about the libvir-list
mailing list