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

[Eduardo Otubo]

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.

> 
>>             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

-- 
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: 3096 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091109/181c0b53/attachment-0001.bin>