[libvirt] [PATCH 1/2] util: Add possibility to call virAuthGetPassword without conn object

[Peter Krempa]

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
>