[Bug 27622] Pluggable SASL authentication

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27622

--- Comment #5 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-04-15 06:59:17 PDT ---
I finally pushed my changes, after 18 hours of flight and no connectivity.

(In reply to comment #1)
> I've reviewed Eitan's branch up to 51d581f. I've got a few patches on top of it
> at
> http://git.collabora.co.uk/?p=user/wjt/wocky.git;a=shortlog;h=refs/heads/sasl.
> They're just trivia; I like the shape of the API and the majority of the
> implementation.
> 
> == Trivia ==
> 
> 241bd34 messes with the license header in wocky/wocky-sasl-auth.c.
> 

Fixed (b81e992)

> c28f475 adds a NULL check before calling g_free(), which is unnecessary: g_free
> (NULL); is a no-op.
> 

Fixed (744871f)

> Both wocky_sasl_auth_dispose() and wocky_sasl_auth_finalize() free
> priv->selected_mechanism. Only the latter should do it. (Arguably, the freeing
> priv->mechanisms should be in _finalize() rather than _dispose() since it's
> freeing memory not releasing references, but...).
> 

Fixed (24f3b6d)

> wocky_sasl_auth_choose_handler(): the coding style checker ought to have
> complained that there should be a space after the cast in:
> 
> +          (plain_mech = g_slist_find_custom (
> +              priv->mechanisms, "PLAIN", (GCompareFunc)strcmp)) != NULL)
> 
> But this whole loop could be replaced by a call to g_slist_remove_all()... oh
> no it couldn't because we need to call strcmp. Grr anaemic collections.
> 

Fixed (b280fe1)

> This is just nit-picking, but in wocky_sasl_auth_authenticate_async():
> 
>   /* add built-in handlers */
> 
>   builtin_handler = WOCKY_SASL_HANDLER (wocky_sasl_digest_md5_new (
>           priv->server, priv->username, priv->password));
> 
>   wocky_sasl_auth_add_handler (sasl, builtin_handler);
> 
>   g_object_unref (builtin_handler);
> 
>   builtin_handler = WOCKY_SASL_HANDLER (wocky_sasl_plain_new (
>           priv->username, priv->password));
> 
>   wocky_sasl_auth_add_handler (sasl, builtin_handler);
> 
>   g_object_unref (builtin_handler);
> 
>   priv->state = WOCKY_SASL_AUTH_STATE_NEGOTIATING_MECHANISM;
> 
>   mech_node = wocky_xmpp_node_get_child_ns (features->node, "mechanisms",
>       WOCKY_XMPP_NS_SASL_AUTH);
> 
>   priv->mechanisms = wocky_sasl_auth_mechanisms_to_list (mech_node);
> 
> That's a whole lot of single statement paragraphs! I've got a patch in my
> branch that makes it (I think) easier to read.

Thanks for doing that!

> 
> == Remarks ==
> 
> Wow, I've never seen g_object_disconnect() before. The signal spec arguments
> for that
> function are ... odd. :)
> 
> Congratulations on removing an actual spaghetti-code goto at the end of
> sasl_auth_stanza_received() in ecacc9a! :)
> 
> == sasl_auth_got_failure: ==
> 
>   if (g_strcmp0 ("aborted", reason->name) == 0)
>     {
>       g_simple_async_result_propagate_error (priv->result, error);
>     }
>   else
>     {
>       g_set_error (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE,
>           "Authentication failed: %s",
>           reason == NULL ? "Unknown reason" : reason->name);
>     }
> 
> Firstly, reason may be NULL by the code above, in which case we'll crash when
> calling g_strcmp0(). Secondly, why does the name of the node returned by the
> server affect whether we should take the error from priv->result or build a new
> one? Finally, the one caller of sasl_auth_got_failure() immediatelly calls
> auth_failed() which just shoves the error back into priv->result! :)
> 
> Reading the code more closely, I think what's happening is: when we abort after
> choosing a mechanism, we wait until the server acks our aborting before we
> throw an error, and in the meantime stash the reason the handler gave for
> aborting the exchange on priv->result (since it's a convenient place to keep it
> kicking around). Then when we get an ack from the server, we pull the reason
> out again.
> 
> But this isn't safe:
> 
> • If we abort, but the server returns a different reason for failure, then
>   we'll call g_simple_async_result_set_error (priv->result, ...) twice.
>   Actually, that turns out to be okay. I'm a bit surprised!
> • If we don't abort, but the server erroneously claims that we did, then
>   g_simple_async_result_propagate_error() will not in fact propagate an error
>   in sasl_auth_got_failure() and then the 
>       g_assert (error != NULL);
>   in sasl_auth_stanza_received() will fail and the world will end.
> 
> I think it might be clearer to have gchar *abort_message in priv and do
> something like:
> 
>   if (priv->abort_message != NULL)
>     {
>       g_set_error_literal (error, WOCKY_SASL_AUTH_ERROR,
>           WOCKY_SASL_AUTH_ERROR_FAILURE, priv->abort_message);
>     }
>   else
>     {
>       g_set_error (error, WOCKY_SASL_AUTH_ERROR, WOCKY_SASL_AUTH_ERROR_FAILURE,
>           "Authentication failed: %s",
>           reason == NULL ? "Unknown reason" : reason->name);
>     }
> 

Good idea. Instead of a string, I used a GError, so a user could specify a code
in addition to a message (2b05d82).

> Relatedly, there's a TODO in this function that I've fixed in my branch of your
> branch!
> 
> == wocky_sasl_auth_choose_handler: ==
> 
>   priv->handler = priv->handlers->data;
> 
>   wocky_sasl_handler_handle_mechanisms_advertised (priv->handlers->data,
>       priv->mechanisms);
> 
> On a first reading, I thought that this meant that only the first handler would
> be tried. But of course that's not the case: on_handler_selected_mechanism()
> tries the next one if the first gives up, etc. Maybe extract part of
> on_handler_selected_mechanism() into a try_next_handler() function which
> wocky_sasl_auth_choose_handler() calls, and/or sling in a "kick the first
> handler to start the chain" comment or something?

I think I made it more readable (695ce71).

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.