[libvirt] [PATCH v3] 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. --- Change since v2: - move the added test out of the for(;;) loop src/remote/remote_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index df7558b..f9fd915 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4121,10 +4121,18 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv, VIR_DEBUG("Client step result complete: %d. Data %zu bytes %p", complete, serverinlen, serverin); + /* Previous server call showed completion & sasl_client_start() told us + * we are locally complete too */ + if (complete && err == VIR_NET_SASL_COMPLETE) + goto done; + /* 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 (;;) { + restep: if ((err = virNetSASLSessionClientStep(sasl, serverin, @@ -4195,6 +4203,7 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv, priv->is_secure = 1; } +done: VIR_DEBUG("SASL authentication complete"); virNetClientSetSASLSession(priv->client, sasl); ret = 0; -- 1.8.4.2
On 11/22/2013 10:26 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. --- Change since v2: - move the added test out of the for(;;) loop
/* 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 (;;) { +
This blank line seems spurious
restep:
now that you aren't modifying the head of the loop, you could follow my earlier suggestion of dropping the 'restep' label and replacing 'goto restep' with 'continue'. But that's trivial, so I don't care either way, and don't need to see a v4 if you choose to change before pushing. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Nov 22, 2013 at 12:58:27PM -0700, Eric Blake wrote:
On 11/22/2013 10:26 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. --- Change since v2: - move the added test out of the for(;;) loop
/* 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 (;;) { +
This blank line seems spurious
restep:
now that you aren't modifying the head of the loop, you could follow my earlier suggestion of dropping the 'restep' label and replacing 'goto restep' with 'continue'. But that's trivial, so I don't care either way, and don't need to see a v4 if you choose to change before pushing.
ACK.
I've pushed this after removing the blank line, and I've pushed an additional patch replacing restep with continue; Christophe
participants (2)
-
Christophe Fergeau -
Eric Blake