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

Matthias Bolte wrote: > > 2009/11/9 Eduardo Otubo <otubo(a)linux.vnet.ibm.com&gt;: >> >> 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.