[libvirt] [PATCH 0/2] Fix authentication with passwords in libssh2 connection driver
When keyboard interactive authentication is disabled on the remote host the usual way is to request a password and authenticate using tunneled password authentication. Libvirt's libssh2 driver didn't have this fallback and connections to such hosts failed. Peter Krempa (2): util: Add possibility to call virAuthGetPassword without conn object libssh2_transport: Add fallback authentication method for password auth src/rpc/virnetsshsession.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virauth.c | 13 ++++++---- 2 files changed, 72 insertions(+), 5 deletions(-) -- 1.8.0
To make this function callable from code that doesn't have the conn object, call the conn-dependant code only if conn was provided. --- src/util/virauth.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index 6d9935d..738b3c5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -205,7 +205,8 @@ virAuthGetUsername(virConnectPtr conn, } - +/* if this function is called with conn == NULL, the code requesting + * the password from the config file is skipped */ char * virAuthGetPassword(virConnectPtr conn, virConnectAuthPtr auth, @@ -218,10 +219,12 @@ virAuthGetPassword(virConnectPtr conn, char *prompt; char *ret = NULL; - if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) - return NULL; - if (ret != NULL) - return ret; + if (conn) { + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; + } memset(&cred, 0, sizeof(virConnectCredential)); -- 1.8.0
On Tue, Nov 27, 2012 at 03:39:16PM +0100, Peter Krempa wrote:
To make this function callable from code that doesn't have the conn object, call the conn-dependant code only if conn was provided. --- src/util/virauth.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c index 6d9935d..738b3c5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -205,7 +205,8 @@ virAuthGetUsername(virConnectPtr conn, }
- +/* if this function is called with conn == NULL, the code requesting + * the password from the config file is skipped */ char * virAuthGetPassword(virConnectPtr conn, virConnectAuthPtr auth, @@ -218,10 +219,12 @@ virAuthGetPassword(virConnectPtr conn, char *prompt; char *ret = NULL;
- if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) - return NULL; - if (ret != NULL) - return ret; + if (conn) { + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; + }
memset(&cred, 0, sizeof(virConnectCredential));
NACK, I don't think this is right. The libssh2 code that is using this must have a virConnectPtr instance somewhere in its callstack. We should fix the code to pass the connection in where needed, not skip this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Nov 27, 2012 at 02:55:11PM +0000, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 03:39:16PM +0100, Peter Krempa wrote:
To make this function callable from code that doesn't have the conn object, call the conn-dependant code only if conn was provided. --- src/util/virauth.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c index 6d9935d..738b3c5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -205,7 +205,8 @@ virAuthGetUsername(virConnectPtr conn, }
- +/* if this function is called with conn == NULL, the code requesting + * the password from the config file is skipped */ char * virAuthGetPassword(virConnectPtr conn, virConnectAuthPtr auth, @@ -218,10 +219,12 @@ virAuthGetPassword(virConnectPtr conn, char *prompt; char *ret = NULL;
- if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) - return NULL; - if (ret != NULL) - return ret; + if (conn) { + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; + }
memset(&cred, 0, sizeof(virConnectCredential));
NACK, I don't think this is right. The libssh2 code that is using this must have a virConnectPtr instance somewhere in its callstack. We should fix the code to pass the connection in where needed, not skip this.
Or probably better is to change the API signature to take a virURIPtr instead of a virConnectPtr, since the URI is the things that's really wanted here. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/27/12 15:56, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 02:55:11PM +0000, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 03:39:16PM +0100, Peter Krempa wrote:
To make this function callable from code that doesn't have the conn object, call the conn-dependant code only if conn was provided. --- src/util/virauth.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c index 6d9935d..738b3c5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -205,7 +205,8 @@ virAuthGetUsername(virConnectPtr conn, }
- +/* if this function is called with conn == NULL, the code requesting + * the password from the config file is skipped */ char * virAuthGetPassword(virConnectPtr conn, virConnectAuthPtr auth, @@ -218,10 +219,12 @@ virAuthGetPassword(virConnectPtr conn, char *prompt; char *ret = NULL;
- if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) - return NULL; - if (ret != NULL) - return ret; + if (conn) { + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; + }
memset(&cred, 0, sizeof(virConnectCredential));
NACK, I don't think this is right. The libssh2 code that is using this must have a virConnectPtr instance somewhere in its callstack. We
The connection object is available in the doRemoteOpen function in the remote driver and it might be passed further down through the virNetClient and virNetSocket objects until it reaches pure libssh2 transport code.
should fix the code to pass the connection in where needed, not skip this.
This might be needed but right now the credential storage option is not desired. The original keyboard-interactive implementation has to ask the user for various arbitrary challenges that are provided by the server. The virAuth implementation does not allow this. The code provided in patch 2/2 is adding fallback method to do the authentication with passwords when keyboard interactive is not available. When the user will be able to store the passwords in the auth file, this will introduce dual behavior on hosts supporting keyboard interactive auth where the user won't be able to store the password as the challenge is provided by the remote server and virauth does not support getting arbitrary requests from the user. On hosts that don't support this, the user would be able to save passwords. With that change the fallback behavior wouldn't be desired. We might change the default authentication method to be password and leave keyboard-interactive for really special scenarios that wouldn't support storing the credentials. (This at the cost of polluting virNetClient and virNetSocket with the connection object (or the URI) and even more added code).
Or probably better is to change the API signature to take a virURIPtr instead of a virConnectPtr, since the URI is the things that's really wanted here.
Daniel
On Tue, Nov 27, 2012 at 04:38:49PM +0100, Peter Krempa wrote:
On 11/27/12 15:56, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 02:55:11PM +0000, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 03:39:16PM +0100, Peter Krempa wrote:
To make this function callable from code that doesn't have the conn object, call the conn-dependant code only if conn was provided. --- src/util/virauth.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c index 6d9935d..738b3c5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -205,7 +205,8 @@ virAuthGetUsername(virConnectPtr conn, }
- +/* if this function is called with conn == NULL, the code requesting + * the password from the config file is skipped */ char * virAuthGetPassword(virConnectPtr conn, virConnectAuthPtr auth, @@ -218,10 +219,12 @@ virAuthGetPassword(virConnectPtr conn, char *prompt; char *ret = NULL;
- if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) - return NULL; - if (ret != NULL) - return ret; + if (conn) { + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; + }
memset(&cred, 0, sizeof(virConnectCredential));
NACK, I don't think this is right. The libssh2 code that is using this must have a virConnectPtr instance somewhere in its callstack. We
The connection object is available in the doRemoteOpen function in the remote driver and it might be passed further down through the virNetClient and virNetSocket objects until it reaches pure libssh2 transport code.
should fix the code to pass the connection in where needed, not skip this.
This might be needed but right now the credential storage option is not desired. The original keyboard-interactive implementation has to ask the user for various arbitrary challenges that are provided by the server. The virAuth implementation does not allow this.
I'm not sure I agree with you there - the SASL code also has the ability to ask the user for arbitrary challenges, and we support that with the virAuth already. See the method remoteAuthFillFromConfig in remote_driver.c The virNetSSHKbIntCb method could be made to lookup the credentials in the auth config file in a similar manner.
The code provided in patch 2/2 is adding fallback method to do the authentication with passwords when keyboard interactive is not available. When the user will be able to store the passwords in the auth file, this will introduce dual behavior on hosts supporting keyboard interactive auth where the user won't be able to store the password as the challenge is provided by the remote server and virauth does not support getting arbitrary requests from the user. On hosts that don't support this, the user would be able to save passwords.
With that change the fallback behavior wouldn't be desired. We might change the default authentication method to be password and leave keyboard-interactive for really special scenarios that wouldn't support storing the credentials. (This at the cost of polluting virNetClient and virNetSocket with the connection object (or the URI) and even more added code).
Looking at the code, in the keyboard-interactive method, you call a libssh2 API, which in turn calls virNetSSHKbIntCb() to fill up a libvirt virConnectCredentialPtr struct with prompts. In this new patch, you are using the virAuthGetPassword function as a way to fill a virConnectCredentialPtr with prompts. I'd be inclined to say the new patch should just directly call virNetSSHKbIntCb to do this and avoid touching virAuthGetPassword at all. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/27/12 17:39, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 04:38:49PM +0100, Peter Krempa wrote:
On 11/27/12 15:56, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 02:55:11PM +0000, Daniel P. Berrange wrote:
On Tue, Nov 27, 2012 at 03:39:16PM +0100, Peter Krempa wrote:
To make this function callable from code that doesn't have the conn object, call the conn-dependant code only if conn was provided. --- src/util/virauth.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c index 6d9935d..738b3c5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -205,7 +205,8 @@ virAuthGetUsername(virConnectPtr conn, }
- +/* if this function is called with conn == NULL, the code requesting + * the password from the config file is skipped */ char * virAuthGetPassword(virConnectPtr conn, virConnectAuthPtr auth, @@ -218,10 +219,12 @@ virAuthGetPassword(virConnectPtr conn, char *prompt; char *ret = NULL;
- if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) - return NULL; - if (ret != NULL) - return ret; + if (conn) { + if (virAuthGetCredential(conn, servicename, "password", &ret) < 0) + return NULL; + if (ret != NULL) + return ret; + }
memset(&cred, 0, sizeof(virConnectCredential));
NACK, I don't think this is right. The libssh2 code that is using this must have a virConnectPtr instance somewhere in its callstack. We
The connection object is available in the doRemoteOpen function in the remote driver and it might be passed further down through the virNetClient and virNetSocket objects until it reaches pure libssh2 transport code.
should fix the code to pass the connection in where needed, not skip this.
This might be needed but right now the credential storage option is not desired. The original keyboard-interactive implementation has to ask the user for various arbitrary challenges that are provided by the server. The virAuth implementation does not allow this.
I'm not sure I agree with you there - the SASL code also has the ability to ask the user for arbitrary challenges, and we support that with the virAuth already. See the method remoteAuthFillFromConfig in remote_driver.c
The virNetSSHKbIntCb method could be made to lookup the credentials in the auth config file in a similar manner.
The code provided in patch 2/2 is adding fallback method to do the authentication with passwords when keyboard interactive is not available. When the user will be able to store the passwords in the auth file, this will introduce dual behavior on hosts supporting keyboard interactive auth where the user won't be able to store the password as the challenge is provided by the remote server and virauth does not support getting arbitrary requests from the user. On hosts that don't support this, the user would be able to save passwords.
With that change the fallback behavior wouldn't be desired. We might change the default authentication method to be password and leave keyboard-interactive for really special scenarios that wouldn't support storing the credentials. (This at the cost of polluting virNetClient and virNetSocket with the connection object (or the URI) and even more added code).
Looking at the code, in the keyboard-interactive method, you call a libssh2 API, which in turn calls virNetSSHKbIntCb() to fill up a libvirt virConnectCredentialPtr struct with prompts.
In this new patch, you are using the virAuthGetPassword function as a way to fill a virConnectCredentialPtr with prompts. I'd be inclined to say the new patch should just directly call virNetSSHKbIntCb to do this and avoid touching virAuthGetPassword at all.
Well, the authentication callback for the libssh2 API is a bit troublesome to work with. Error reporting is strange and so on. The callback even requires pre-allocating the query strings. If we decide we don't want the ability to use the credential storage configuration I'd rather re-do the functionality of the callback in the password authentication function than use the callback. In fact we probably should do it the other way -- replace the callback with the possibility to read the responses from the config files. (This will need expanding of virauth to support arbitrary queries and responses). But the usability of this would be probably limited and countering the semantics of keyboard-interactive auth. On the other hand, for password authentication it might be desired (and less awkward to implement) to actually use the virauth helpers to read the data from the config file and request them from the user. In that case we would need to actually use the virauth helper for getting the username which might conflict with the username provided in the URI (and I didn't come up with a nice solution for this situation yet ... ) I think that the correct immediate solution would be to use the "fallback" code that is provided in the second patch to be actually the primary one and use keyboard-interactive only when there's demand for that. I'll try to find an elegant solution for this.
Daniel
When keyboard-interactive authentication is disabled on a host libvirt fails when attempting authentication with password. This patch adds code that simulates keyboard interactive authentication using "password" auth method in libssh2. --- src/rpc/virnetsshsession.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 13a0368..9b35b03 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -34,6 +34,7 @@ #include "util.h" #include "virterror_internal.h" #include "virobject.h" +#include "virauth.h" #define VIR_FROM_THIS VIR_FROM_SSH @@ -769,6 +770,66 @@ virNetSSHAuthenticateKeyboardInteractive(virNetSSHSessionPtr sess, return 1; } +/* perform keyboard interactive authentication using fallback password method + * + * Returns: 0 on success + * 1 on authentication failure + * -1 on error + */ +static int +virNetSSHAuthenticateKeyboardFallback(virNetSSHSessionPtr sess, + virNetSSHAuthMethodPtr priv) +{ + char *password = NULL; + char *errmsg; + int ret = -1; + int rc; + + + if (!sess->cred || !sess->cred->cb) { + virReportError(VIR_ERR_SSH, "%s", + _("Can't perform authentication: " + "Authentication callback not provided")); + return -1; + } + + /* Try the authenticating the set amount of times. The server breaks the + * connection if maximum number of bad auth tries is exceeded */ + while (priv->tries < 0 || priv->tries-- > 0) { + if (!(password = virAuthGetPassword(NULL, sess->cred, NULL, + priv->username, sess->hostname))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to retrieve password")); + goto cleanup; + } + + /* tunelled password authentication */ + if ((rc = libssh2_userauth_password(sess->session, + priv->username, + password)) == 0) { + ret = 0; + goto cleanup; + } + + /* error path */ + libssh2_session_last_error(sess->session, &errmsg, NULL, 0); + virReportError(VIR_ERR_AUTH_FAILED, + _("authentication failed: %s"), + errmsg); + + if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED) + goto cleanup; + } + + /* authentication failed */ + ret = 1; + +cleanup: + VIR_FREE(password); + return ret; +} + + /* select auth method and authenticate */ static int virNetSSHAuthenticate(virNetSSHSessionPtr sess) @@ -814,6 +875,9 @@ virNetSSHAuthenticate(virNetSSHSessionPtr sess) case VIR_NET_SSH_AUTH_KEYBOARD_INTERACTIVE: if (strstr(auth_list, "keyboard-interactive")) ret = virNetSSHAuthenticateKeyboardInteractive(sess, auth); + else if (strstr(auth_list, "password")) + ret = virNetSSHAuthenticateKeyboardFallback(sess, auth); + break; case VIR_NET_SSH_AUTH_AGENT: if (strstr(auth_list, "publickey")) -- 1.8.0
participants (2)
-
Daniel P. Berrange -
Peter Krempa