[Bug 27622] Pluggable SASL authentication

[bugzilla-daemon at freedesktop.org]

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

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://git.collabora.co.uk/
                   |                            |?p=user/eitan/wocky.git;a=s
                   |                            |hortlog;h=refs/heads/sasl
         AssignedTo|telepathy-bugs at lists.freede |eitan.isaacson at collabora.co
                   |sktop.org                   |.uk

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2010-04-13 17:14:26 PDT ---
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.

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

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...).

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.

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.

== 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);
    }

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?

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