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

[Matthias Bolte]

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