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

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

Eduardo Otubo wrote: > > 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. > > > ------------------------------------------------------------------------ > > -- > Libvir-list mailing list > Libvir-list(a)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