2009/11/9 Eduardo Otubo <otubo@xxxxxxxxxxxxxxxxxx>:
> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list