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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list