[libvirt] [PATCH] sasl: Fix authentication when using PLAIN mechanism
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. --- src/remote/remote_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7181949..e15eeaf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4125,6 +4125,9 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv, * 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 */ for (;;) { + if (complete && err == VIR_NET_SASL_COMPLETE) + break; + restep: if ((err = virNetSASLSessionClientStep(sasl, serverin, -- 1.8.4.2
On 11/21/2013 10:56 AM, 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. --- src/remote/remote_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7181949..e15eeaf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4125,6 +4125,9 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv, * 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 */
This code comment is now stale. Can you touch it up?
for (;;) { + if (complete && err == VIR_NET_SASL_COMPLETE) + break; + restep:
Side note: Previously, the code could have used 'continue' instead of 'goto restep'; but now the goto is necessary. Based on the linked message, the patch makes sense, but I'd still wait for a review from Dan as someone more familiar with SASL code. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Nov 21, 2013 at 04:45:32PM -0700, Eric Blake wrote:
On 11/21/2013 10:56 AM, 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. --- src/remote/remote_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7181949..e15eeaf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4125,6 +4125,9 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv, * 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 */
This code comment is now stale. Can you touch it up?
I've changed it to: /* Loop-the-loop... * 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 * */ Christophe
On Fri, Nov 22, 2013 at 10:54:01AM +0100, Christophe Fergeau wrote:
On Thu, Nov 21, 2013 at 04:45:32PM -0700, Eric Blake wrote:
On 11/21/2013 10:56 AM, 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. --- src/remote/remote_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7181949..e15eeaf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4125,6 +4125,9 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv, * 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 */
This code comment is now stale. Can you touch it up?
I've changed it to: /* Loop-the-loop... * 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 * */
and I've also added a comment right before the added code chunk as the other identical tests in this code block are doing that. I'll send a v2. Christophe
participants (2)
-
Christophe Fergeau -
Eric Blake