[libvirt] [PATCH] phyp: ssh authentication with pub keys fixed
Eduardo Otubo
otubo at linux.vnet.ibm.com
Tue Nov 10 12:32:25 UTC 2009
Eduardo Otubo wrote:
> 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.
>
>
> ------------------------------------------------------------------------
>
> --
> Libvir-list mailing list
> Libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Actually, I can't do that construction. Using the label and the goto
that way. I just put the label right above the if statement and set the
rc to fallback to keyboard interactive (in case of files not found).
[]'s
--
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: 3195 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091110/60fc0abf/attachment-0001.bin>
More information about the libvir-list
mailing list