[libvirt] [PATCH] phyp: ssh authentication with pub keys fixed
-- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
2009/11/6 Eduardo Otubo <otubo@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
Matthias Bolte wrote:
2009/11/6 Eduardo Otubo <otubo@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 -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
2009/11/7 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
Matthias Bolte wrote:
2009/11/6 Eduardo Otubo <otubo@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
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@linux.vnet.ibm.com
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
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. -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@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 -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
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
2009/11/10 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
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.
It is :-) There is one small issue left. GCC doesn't like the variable declarations after the keyboard_interactive, so I moved them to the beginning of the function. ACK, committed and pushed. Thanks. Matthias
participants (2)
-
Eduardo Otubo -
Matthias Bolte