[libvirt] [PATCH v2] sasl: Fix authentication when using PLAIN mechanism

[Daniel P. Berrange]

On Fri, Nov 22, 2013 at 10:59:20AM +0100, Christophe Fergeau wrote:
> With some authentication mechanism (PLAIN for example), sasl_client_start()
> can return SASL_OK, which translates to virNetSASLSessionClientStart()
> returning VIR_NET_SASL_COMPLETE.
> cyrus-sasl documentation is a bit vague as to what to do in such situation,
> but upstream clarified this a bit in
> http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-sasl&msg=10104
> 
> When we got VIR_NET_SASL_COMPLETE after virNetSASLSessionClientStart() and
> if the remote also tells us that authentication is complete, then we should
> end the authentication procedure rather than forcing a call to
> virNetSASLSessionClientStep(). Without this patch, when trying to use SASL
> PLAIN, I get:
> error :authentication failed : Failed to step SASL negotiation: -1
> (SASL(-1): generic failure: Unable to find a callback: 32775)
> 
> This patch is based on a spice-gtk patch by Dietmar Maurer.
> ---
> Changes since v1:
> - adjust comment before the for(;;) code block
> - add comment before the if (complete && ...) test as this is what is done
>   for these tests in this block.
> 
> 
>  src/remote/remote_driver.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 7181949..38da119 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -4122,9 +4122,16 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv,
>                complete, serverinlen, serverin);
>  
>      /* Loop-the-loop...
> -     * Even if the server has completed, the client must *always* do at least one step
> -     * in this loop to verify the server isn't lying about something. Mutual auth */
> +     * Even if the server has completed, the client must loop until sasl_client_start() or
> +     * sasl_client_step() return SASL_OK to verify the server isn't lying
> +     * about something. Mutual auth
> +     * */
>      for (;;) {
> +        /* Previous server call showed completion & sasl_client_start()
> +         * told us we are locally complete too */
> +        if (complete && err == VIR_NET_SASL_COMPLETE)
> +            break;
> +

We already have this check at the end of the for(;;) loop. We only need
to deal with the first iteration. So I think this code should be placed
prior to the for(;;) loop, rather than inside it.

>      restep:
>          if ((err = virNetSASLSessionClientStep(sasl,
>                                                 serverin,

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