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

[Matthias Bolte]

2009/11/7 Eduardo Otubo <otubo at linux.vnet.ibm.com>:
> Matthias Bolte wrote:
>>
>> 2009/11/6 Eduardo Otubo <otubo at linux.vnet.ibm.com>:
>>>
>>> +    char *pubkey = NULL;
>>> +    char *pvtkey = NULL;
>>> +
>>> +    if (virAsprintf(&pubkey, "%s/.ssh/id_rsa.pub", getenv("HOME")) < 0)
>>> {
>>> +        virReportOOMError(conn);
>>> +        goto err;
>>> +    }
>>> +
>>> +    if (virAsprintf(&pvtkey, "%s/.ssh/id_rsa", getenv("HOME")) < 0) {
>>> +        virReportOOMError(conn);
>>> +        goto err;
>>> +    }
>>
>> You should use virGetUserDirectory() instead of getenv("HOME"):
>>
>>    char *userdir = virGetUserDirectory(NULL, geteuid());
>>
>>    if (userdir == NULL)
>>        goto err;
>>
>> Matthias
>
> Here it is.
> Thanks again.
>
> []'s

You included the old and the new version of this patch in the attached file.

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

> -                                                "/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?

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

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.

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.

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

> +    VIR_FREE(pvtkey);
>      VIR_FREE(password);
>      return session;
>  }

Matthias