[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



2009/11/10 Eduardo Otubo <otubo linux vnet ibm com>:
> Eduardo Otubo wrote:
>>
>> Matthias Bolte wrote:
>>>
>>> 2009/11/9 Eduardo Otubo <otubo 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 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
>

Okay, I've applied the difference between this patch and the last patch.

Matthias


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]