Re: [Evolution-hackers] Camel Authentication Changes

[David Woodhouse <dwmw2 infradead org>]

On Sat, 2011-10-15 at 11:18 -0400, Matthew Barnes wrote:
> Just a heads up that I've changed Camel's authentication API to factor
> out the password loop that each and every provider currently replicates
> for themselves.  Turns out they all have the same basic logic flow.
> 
> These changes make password management more consistent and also make
> life easier for providers.  Now when a provider reaches the step where
> it needs to authenticate, it simply calls: 
> 
> 
>   gboolean  camel_session_authenticate_sync  (CamelSession *session,
>                                               CamelService *service,
>                                               const gchar *mechanism,
>                                               GCancellable *cancellable,
>                                               GError **error);

This looks like a good idea, and cleaning up the various ways that the
providers were behaving differently in that authentication loop (and
calling directly out to the GUI) is a good thing.

I looked at those recently when implementing the ->try_empty_password()
SASL methods for automatic NTLM auth, and I take my hat off to you for
finding a way through the mess.

Having said that, I do think we need to adjust this approach to deal
with authentication as a *reactive* thing, not *proactive*. It *used* to
be reactive, but you changed it as a side-effect of your improvements.

The point is that we authenticate ourselves *if* and *when* the server
asks us to. We cannot know in advance whether we're going to need a
password or not.

I tried this code, and the first thing it did was ask me for a password
for my IMAP connection, even though that IMAP account uses a custom
connection command 'ssh $mailhost /usr/libexec/dovecot/imap' so it gets
a *preauthenticated* connection.

(At other times, the custom command might have been something different
like  'ssh $bastionhost openssl s_client -connect $internalserver:993'
and the authentication *would* still have been required. You just can't
know until you connect.)

Even if the server asks for a password, with things like automatic NTLM
authentication you can't know whether the automatic challenge/response
will work. The old loop in the providers would handle that — it would
try first with the empty password, then it would only prompt the user
for a password if it really needed one. I think you've broken that in
master now, because session_authenticate_sync() will just return
immediate success if the SASL ->try_empty_password() method indicates
that we can even *try* to authenticate without a user-provided password.

> The "mechanism" argument refers to the user's preferred SASL mechanism.
> This will usually be taken from "url->authmech"

That's fine at the moment, but the fact that we have to explicitly
choose *one* mechanism in advance ought to be considered a bug. In the
future, we ought to simply try *any* of the methods that the server
offers today, in some order of preference. So we could try Kerberos,
fall back to NTLM, etc. These changes seem to have made that harder to
fix.

>  but it can also be NULL when not using SASL (such as for HTTP-based
> backends).

It looks like it'll always ask for a password for HTTP-based backends,
but those have similar options; they may authenticate with Kerberos, or
with automatic-NTLM, or with cookies... or they *may* actually decide
that they need a password today.

All of the above issues have a single solution — we need to change
things so that the password is requested *only* when we're actually
connected to the server and we have discovered that we actually need it.

I think the best way to handle it is to extend
camel_service_get_password(), which is already called from the providers
in the right place. If we make *it* trigger the password dialog, rather
than doing the dialog in advance, then I think we solve all the issues.

We'll also want to add a 'retrying' argument, for the caller to indicate
that the last password it received was not correct. That'll prompt for
actual user interaction, rather than just handing out the password from
gkr, for example.

> The CamelSession (or subclass) now handles all the looping.  So if the
> function returns TRUE, you're authenticated.  If it returns FALSE, you
> treat it like any other failed operation.  You don't loop.

Aside from the correctness issues, doing it internally can be a lot more
efficient. Imagine you're on a slow connection and each DNS lookup, TCP
connection establishment and SSL negotiation takes about 30 seconds.

Now, do you want to connect a first time and work out that your stored
password is invalid, then tear it all down and make a *new* connection,
only to find that the user fat-fingered the password and you need to try
a *third* time?

Or do you want to keep the same connection open and issue three
consecutive AUTH commands, looping *inside* the IMAP provider?

There are similar considerations when the password changes during a
session. TCP connections may break and need to be recreated at any time,
so you can find that you need a new password at *random* times. You've
handled this in imapx by calling camel_session_authenticate_sync(), but
that doesn't necessarily work well if you are *handling* an existing
request already. Won't it cause one IMAP command to fail for each time
you get your password wrong, if you have to reconnect at run-time?

The provider wants to call out to the session to provide a password, and
when the only way to do that involves the session code calling *back*
into the provider's ->authenticate_sync() method.

In EWS I have a handler for the 'authenticate' signal on the libsoup
object, which is simply expected to provide a password and return. I'd
have to hack that to freeze the request, and then trigger a call to
camel_session_authenticate_sync(). And *then* I'd have to hack the
ews_authenticate_sync() method so that when there's a frozen request
which is waiting for a password, it would resume *that* request instead
of triggering a new dummy request to test authentication.

Either that, or I'd have to let the original request (and everything
else on the queue until the new password arrives) *fail*, and have hacks
to cope with the failure mode and retry the request. I've actually tried
that kind of error-catching hack for ActiveSync, but it was too horrible
and I couldn't make it work sanely; I instead revamped the
authentication handling :)


> Rather than disappearing immediately, the dialog will stay visible until
> authentication is confirmed.  If the wrong password was given, it does
> that jiggle-the-window thing and then refocuses the password entry.

This part is nice. By emitting an 'authenticated' signal after our call
to camel_service_get_password(), we can trigger the disappearance of the
dialog box when we have authenticated correctly. Or we call
camel_service_get_password() again if it fails, of course.

-- 
dwmw2