I tried with the attached patch and i cannot login to my server with GSSAPI, I get an error:
   Failed to open folder.

   The reported error was "GSSAPI authentication failed".

If I revert the patch and rebuild it works. I also see a password prompt originated at the below place, but I get it also at the code without the patch, thus it's not introduced by it (I only mention it here, because I noticed it).

evolution-source-registry process backtrace of the ews thread which invoked the password prompt:

+ Trace 233785

It is odd that this didn't work, since I am using it daily now with no problems and no password prompts.  I set my system up to log in with Kerberos using sssd, so my credentials are already in place at login time.  The corporate environment seems to be Exchange 2010, going by the "about" window in their OWA client.

If there is more detail I can provide, let me know.

Please note that GSSAPI isn't just Kerberos. Testing with gssntlmssp would be useful too, as it often shows up deficiencies in GSSAPI implementations (because if you're only using Kerberos, you never really have to deal with the server sending back a token and you having to *continue* the exchange...)

Daniel, if your patch were instead a *series* of individual self-contained patches it would be easier for Milan to work out which of the changes was actually responsible for the regression in his case... :)

Hmmm... I wonder if I could force SASL to require mutual auth?  That should have the same effect of needing multiple exchanges, which should better test that scenario.  I'll look into that.

The problem appears to be that soup_auth_authenticate() must be called by the "authenticate" signal handler or the following 401 from the server will be deemed a hard failure.  I suppose this is the reason for the ugly "got-body" hack mentioned earlier.  I'll see what I can do to provide a better patch.

(In reply to comment #3)
> Daniel, if your patch were instead a *series* of individual self-contained
> patches it would be easier for Milan to work out which of the changes was
> actually responsible for the regression in his case... :)

Definitely not, I prefer one patch only (it happened to me that I found a bug in the first patch, which was corrected in the third patch - waste of time, you know).

Created attachment 280920 [details] [review]
Next attempt

Here is another patch.

I can see why the HTTP message hack was done, but even after restoring to that, I found situations where it was inadequate.  Some of the backends get their password info through gcr, which returns empty passwords instead of NULL ones.  Empty passwords are enough to make soup attempt its own auth, which destroys the GSSAPI tokens.  So I created a new Soup auth handler for the Negotiate type.  It works similar to the NTLM one, except that it maintains state per message instead of per connection.  This is okay though because my Exchange server requires authentication for every message, and auth is only attempted when the server rejects your message with a 401 anyway.  So theoretically it should also work on a connection basis.

I see that there is now a patch to convince Camel that we're online.  I backed out my own change in that area because of this.  The authtype autodiscovery still needs to remove its check for online though, because even with the current fix it still says you're offline.

The integrated auth is also better for the synchronized soup messages in e-ews-notification.c since they were neglected before, and adding the Negotiate auth type to the session takes care of them automatically.

However, I still don't have access to a multi-roundtrip GSSAPI method so I'll just have to wait and see if this passes muster.

(In reply to comment #7)
> However, I still don't have access to a multi-roundtrip GSSAPI method so I'll
> just have to wait and see if this passes muster.

This isn't hard to set up, assuming your server supports NTLM authentication within Negotiate/SPNEGO. Which they all do, don't they?

If you are using Fedora, it's a simple as:

 sudo yum install gssntlmssp
 # This will be fixed soon now that krb5 supports /etc/gss/mech.d/*.conf
 sudo mv /etc/gss/mech.ntlmssp /etc/gss/mech
 export NTLM_USER_FILE=~/.ntlmpass
 echo $DOMAIN:$USER:$PASSWORD > $NTLM_USER_FILE
 # Make sure we *use* it
 kdestroy
 EWS_DEBUG=2 evolution

If you have to install gss-ntlmssp by hand it's only slightly less simple...

At work they tend to disable NTLM (especially if it has LanMan) for security reasons, and they definitely frown on having a plaintext password file.  I do have access to an outside Exchange server though.

It doesn't seem to be as simple as the example above.  I know the system is at least aware of the ntlmssp module now since I traced its conf file loader.  It created a mechanism descriptor for ntlmssp, but never loaded the library.  So I always get the error 'Matching credentials not found'.

I hacked camel-sasl-gssapi.c to explicitly use the gssntlmssp mech, though, and Evolution still logged me in.  No Kerberos credentials to give a false positive, and the base64 tokens definitely showed a different pattern than I normally see.  Also, this time the server had no final challenge on its 200 message like KRB5 does.

So it does seem to be working for me.

Disabling NTLM is hard because Kerberos is so brittle and often fails due to SPN/DNS/etc. mismatches. So in most environments it's really important that clients can fall back to using NTLMSSP.

The plain-text password file is just a testing hack; we have patches for GSS-NTLMSSP which make it talk to Samba/winbind and use credentials from there, just as camal-sasl-ntlm.c uses Samba's ntlm_auth helper for the same thing.

The real issue, as you say, is that you needed to hack camel-sasl-gssapi.c to use the right mechanism. I see it's currently using GSS_C_OID_KRBV5_DES explicitly; that is *wrong* for HTTP "Negotiate"; it should be using SPNEGO.

And then the fallback from KRB5 to NTLMSSP would have worked just fine (and other mechanisms like IAKERB would also be available).

Created attachment 281089 [details] [review]
Add GSS-SPNEGO SASL mech support to EDS

I don't actually have an evo build environment for 3.12 yet; I'm still on Fedora 20 with 3.10. Please could I trouble you to test the following EDS patch, along with the corresponding one-liner to make evolution-ews use 'GSS-SPNEGO' instead of 'GSSAPI'? Thanks.

Oh, forget that. We *have* the service name. We can just make the existing GSSAPI mech automatically use SPNEGO when it sees that the service name is "HTTP". No need for the GSS-SPNEGO mechanism (which was never really anything but a de facto standard anyway). I'll give you a much simpler patch...

Created attachment 281090 [details] [review]
Simpler patch to use SPNEGO for HTTP

This is much simpler, and requires no corresponding change on the evolution-ews side. It should just silently Do The Right Thing™ for GSSAPI HTTP auth in all cases.

Fortunately GSS_C_OID_KRBV5_DES doesn't exist in MIT's distribution, so the #ifdef that redefines it as GSS_C_NO_OID kicks in.  Unfortunately, MIT uses this to return the first mechanism in the list and doesn't even bother to check whether it matches the credentials, or if none supplied, whether it can find default creds.  So no matter what, if you don't use Kerberos you're out of luck with that constant.

I did briefly attempt SPNEGO also, but it ran into an error that I couldn't get a proper message for.  If we want to make SPNEGO the protocol for HTTP (and since the auth mechanism is called Negotiate, that would seem to imply that you should), we also need to get better error messages from SPNEGO.  At the moment it calls error_message() which is a Kerberos et utility library call and doesn't even seem to be documented.  It needs to use gss_display_status() with the proper OID and minor status code to return an intelligible response when nego fails.  Otherwise you just get "Unknown code FF 35" or something like that.

It also appears that the patch forgot the most crucial component--using priv->mech in the gss_init_sec_context call.

Here are some new patches to address those problems as well as to clean up some lint from developing the patch.

Created attachment 281159 [details] [review]
Additional changes to camel-sasl-gssapi

Created attachment 281160 [details] [review]
Cleaned up patch

Review of attachment 281159 [details] [review]:

Looks good to me; thanks. Are you able to commit it?

Probably not.  I'm a 1-time contributor and probably don't have commit access.

I can commit the EDS part, although I'll let Milan have the final say on the EWS part. Please could you make it two separate patches though. First fix up the error handling, and then the actual changing of the mechanism has to be in a separate part. That's just good engineering practice; you want small individual changes which are easy to bisect if they are later found to cause problems.

On the error handling, I'd prefer to simply change the gssapi_set_exception() function to take the mechanism as an argument, rather than changing all its callers to call it only for non-GSS_S_FAILURE errors, and to call something else for GSS_S_FAILURE. 

Just have the callers call gssapi_set_exception() with the extra 'mech' argument, and let it do the right thing. (And I'm not entirely sure why we only use the mechanism's error code for GSS_S_FAILURE anyway.)

To make sure we're on the same page, you mean split the EDS-specific part into two patches?  Because the EWS part and the EDS part are already two separate patches.

The reason for the distinction in error code display is that Camel's gssapi exception translator doesn't do anything with the minor code on the well-known major errors.  It only asks for a translation of the minor codes when the error is GSS_S_FAILURE.  This does seem consistent with the specs:

   GSS-API routines return a minor_status parameter, which is used to
   indicate specialized errors from the underlying security mechanism.
   This parameter may contain a single mechanism-specific error,
   indicated by a OM_uint32 value.

   The minor_status parameter will always be set by a GSS-API routine,
   even if it returns a calling error or one of the generic API errors
   indicated above as fatal, although most other output parameters may
   remain unset in such cases...

and

   In addition to the explicit major status codes documented here, the
   code GSS_S_FAILURE may be returned by any routine, indicating an
   implementation-specific or mechanism-specific error condition,
   further details of which are reported via the minor_status parameter.

So it falls short of saying that minor_status is only useful if GSS_S_FAILURE is the major, but it strongly implies that.

However, the GSSAPI module could simply call gss_display_status in all cases and forget storing its own versions of those error strings.

What's your preference?

One more thought:  Since SPNEGO offers all mechanisms for which credentials are available, perhaps the '#if HAVE_KRB5' mechanism needs to be modified so that SPNEGO can be made available even if Kerberos is not.

(In reply to comment #20)
> To make sure we're on the same page, you mean split the EDS-specific part into
> two patches?

Yes. Thanks.
> The reason for the distinction in error code display is that Camel's gssapi
> exception translator doesn't do anything with the minor code on the well-known
> major errors.  It only asks for a translation of the minor codes when the error
> is GSS_S_FAILURE.  This does seem consistent with the specs:
> 
>    GSS-API routines return a minor_status parameter, which is used to
>    indicate specialized errors from the underlying security mechanism.
>    This parameter may contain a single mechanism-specific error,
>    indicated by a OM_uint32 value.
> 
>    The minor_status parameter will always be set by a GSS-API routine,
>    even if it returns a calling error or one of the generic API errors
>    indicated above as fatal, although most other output parameters may
>    remain unset in such cases...

That certainly doesn't say that it's only for GSS_S_FAILURE. That says it'll always be set.

> and
> 
>    In addition to the explicit major status codes documented here, the
>    code GSS_S_FAILURE may be returned by any routine, indicating an
>    implementation-specific or mechanism-specific error condition,
>    further details of which are reported via the minor_status parameter.

And that doesn't actually say that it isn't set when the major error *isn't* GSS_S_FAILURE, although I see why you'd make that inference. But poking through krb5 source code I certainly do see instances where a mechanism-specific error is given in addition to a major error code that isn't GSS_S_FAILURE.

> So it falls short of saying that minor_status is only useful if GSS_S_FAILURE
> is the major, but it strongly implies that.
> 
> However, the GSSAPI module could simply call gss_display_status in all cases
> and forget storing its own versions of those error strings.

The problem there is translations though, right?
 
> What's your preference?

Leave it as-is for now, displaying the minor code only for GSS_S_FAILURE. Let's just do what you implemented — use gss_display_status() for unknown mechanisms. But with a single function gssapi_set_exception() instead of adding special knowledge to the call sites. And if we *later* change it to use the minor code in all cases, then that's something that can happen just once inside the gssapi_set_exception() function.

(In reply to comment #21)
> One more thought:  Since SPNEGO offers all mechanisms for which credentials are
> available, perhaps the '#if HAVE_KRB5' mechanism needs to be modified so that
> SPNEGO can be made available even if Kerberos is not.

TBH, I don't really understand why any of that krb5-specific stuff is needed. If I have a GSSAPI which supports *only* SPNEGO and the NTMLSSP mechanism, and no hint of Kerberos support anywhere, that *ought* to work. There seems to be a whole lot more implementation-dependencies in this code than we need. But let's just stop pulling on this thread for now, or we'll end up rewriting the whole damn thing... :)

Review of attachment 281090 [details] [review]:

I do not understand what this is good for. The newly added priv->mech is only written to, not read at all, thus useless.

Review of attachment 281159 [details] [review]:

I also see couple coding style issues, some brought from the previous patch, some new, but this is a small patch, thus I can change it before committing.

It seems to produce odd error messages. Namely, I do not have a ticket for my EWS account, but I have a ticker for another machine. The reported error without this patch reads:
   "No response: Server not found in Kerberos database".
while with this patch it says a confusing message:
   "No response: The consistency checks performed on the input_token failed."
Even worse case is:
   "No response: (null)"
which I get when I try to authenticate after I called 'kdestroy' in a console.

::: evolution-data-server-3.12.4/camel/camel-sasl-gssapi.c.gssapi-http-nego
@@ +213,3 @@
+			message_part = g_strdup_printf (
+				"Could not retrieve status code for %ud: "
+				"%ud %ud", minor, tmajor, tminor);

As long as this string can make it into UI it should be localized and a translator comment added to explain what the numbers are.

I still didn't try the "Cleaned up patch", the EDS change may work on its own, right? I also guess you are going to update it, according to the above discussion.

I've fixed up the (null) and the confusing message, and committed the EDS part to EDS master:

https://git.gnome.org/browse/evolution-data-server/commit/?id=a523ac27f
https://git.gnome.org/browse/evolution-data-server/commit/?id=bd8434342

and finally (because I'm stupid and missed the bit about marking for translation until I'd pushed it; sorry) 

https://git.gnome.org/browse/evolution-data-server/commit/?id=f98caca8e

This wants committing to the 3.12 branch too, assuming we're allowed to add the new translated string. Which we might as well; none of the *other* strings we get from error_message() were being translated, if my 'LANG=fr_FR' test run is indicative.

(In reply to comment #25)
> I still didn't try the "Cleaned up patch", the EDS change may work on its own,
> right? I also guess you are going to update it, according to the above
> discussion.

Yes, it should work without the EDS change.  Though apparently there is still more work to do.  I agree that you should never see "(null)" as an error message.  I'll have to figure out what happened--I never saw it here, even when I did kdestroy.

I assumed that the message about a corrupt input_token was actually returned from the server upon receipt of wrong data, but I should take a closer look.

(In reply to comment #22)
> TBH, I don't really understand why any of that krb5-specific stuff is needed.
> If I have a GSSAPI which supports *only* SPNEGO and the NTMLSSP mechanism, and
> no hint of Kerberos support anywhere, that *ought* to work. There seems to be a
> whole lot more implementation-dependencies in this code than we need. But let's
> just stop pulling on this thread for now, or we'll end up rewriting the whole
> damn thing... :)

Yeah you're probably right :)  Anyway, I can at least answer your question:

Historically, the reason is that GSSAPI's only initial adopter was Kerberos and Kerberos was almost always used through GSSAPI, so the two became synonymous for a while.  Globus GSI, NTLM, and SPNEGO came later.  Even worse (and probably why nobody else bothered for so long), GSSAPI did not support mechanism plugins, nor did the GSSAPI spec provide for any way to add them.  You just programmed for GSSAPI then linked in the specific mechanism you wanted, ie gssapi_krb5.

Eventually mechglue, a GSSAPI framework with no mechanism implementations, appeared.  Only in the recent few KRB5 releases did MIT become pluggable.  It appears that Heimdal did somewhere in there too.

Anyway, short history lesson aside, the proper check these days should mainly see if GSSAPI is available.  The major brands of Kerberos also need to be accounted for so that the KRB5-specific return codes can be properly checked and credentials renewed though.


Now, on to figure out why EWS is weird.

Oh, one more thing:  While reviewing the MIT GSSAPI gss_display_status code, I realized that it returns a series of sentences, some of which end in a period, but none of which include ending spaces or newlines.  The mech code may be okay with that if it only returns one message anyway, but it may be best to put a newline between messages while concatenating them.

The (null) went away when I stopped it calling g_strconcat(NULL, ...), btw.

I've now also tested your 'cleaned up patch' from comment 16. It appears to make no visible difference here to my active account.

Looking at it though, this function looks wrong:

+/* Return TRUE if using GSSAPI.  */
+gboolean
+e_ews_connection_utils_get_without_password (CamelEwsSettings *ews_settings)
+{
+	gboolean result = FALSE;
+	gchar *auth_mech = NULL;
+
+	g_object_get (G_OBJECT (ews_settings), "auth-mechanism", &auth_mech, NULL);
+	if (g_strcmp0 (auth_mech, "GSSAPI") == 0)
+		result = TRUE;
+
+	g_free (auth_mech);
 
+	return result;
+}

That isn't going to do the right thing for automatic NTLM auth, is it? Rather than hacking up a special case, surely you actually want to query the SASL mech for its capabilities?

See bug 703181 for previous discussion about passwordless authentication.

(In reply to comment #30)
> I've now also tested your 'cleaned up patch' from comment 16. It appears to
> make no visible difference here to my active account.

Is that in a good or a bad way?  :)


> Looking at it though, this function looks wrong:
> ...
> That isn't going to do the right thing for automatic NTLM auth, is it? Rather
> than hacking up a special case, surely you actually want to query the SASL mech
> for its capabilities?

Well, the auth before would always ask for a password for NTLM.  I went on the assumption that that was desired, but apparently that is not the case.  So I guess that means that only with basic auth do we want to hit the user up front with a password request.

Would something fail if all instances of get_without_password always returned TRUE regardless of auth type?

Created attachment 281353 [details] [review]
Added logic to detect failed Negotiate attempt

The problem with bad input_token messages is caused when the server resets authentication by providing WWW-Authenticate: Negotiate with no args after the exchange has already begun.  I expected a better indicator of failure such as a 400 status instead of a 401.

Oh well, I added another check in soup-auth-negotiate against the actual observed response.  Maybe now you'll get the expected responses?

I also observed that the SPNEGO mechanism leaves the used_mech as GSS_C_NO_OID when it can't find any usable mechanisms.  This causes the gssapi_set_exception call to use the Kerberos message translator (GSS_C_OID_KRBV5_DES is undefined for MIT Kerberos and GSS_C_NO_OID is substituted), and the result is "Unknown status code FF xx" where I can't remember what xx is.  Perhaps we should do the gss_display_status method regardless of mechanism when major is GSS_S_FAILURE.  That should also return proper Kerberos messages too.

(In reply to comment #32)
> (In reply to comment #30)
> > I've now also tested your 'cleaned up patch' from comment 16. It appears to
> > make no visible difference here to my active account.
> 
> Is that in a good or a bad way?  :)

Not sure. It's good that it didn't break anything, certainly. I should try removing my password from gnome-keyring and check I don't get asked for it.

> Well, the auth before would always ask for a password for NTLM.  I went on the
> assumption that that was desired, but apparently that is not the case. 

Indeed, that's definitely a bug. Milan's proposed patch for it in https://bugzilla.gnome.org/show_bug.cgi?id=703181#c26 is actually remarkably similar to yours, isn't it? Perhaps you could just lift his ews_connect_check_ntlm_available() function?

> Would something fail if all instances of get_without_password always returned
> TRUE regardless of auth type?

No, nothing should fail if we do that; it'll just be slightly suboptimal because it'll make requests that fail, and then immediately try again *with* the password. Because we don't handle it the same way that libsoup does, with an 'authenticate' signal while the request is actually in-flight; instead we fail and then have to resubmit it from scratch.

See https://bugzilla.gnome.org/show_bug.cgi?id=703181#c13 where the get_without_password() function was introduced:
> The authentication_session_execute_sync() asks for the property and if the
> password is not required, then it tries to authenticate with an empty
> password (NULL is not allowed, and I didn't want to change this constraint).
> If it fails for whatever reason (except of cancelled), then the user is
> asked for the password and the authentication continues as before. Quite
> like Camel does it.

It's supposed to return TRUE when it thinks it *might* be able to authenticate without a password. It's basically an optimisation; returning TRUE every time (or just ripping out the method from every back end and never calling it, and just behaving as if it were TRUE) would *work*, but be suboptimal.

(In reply to comment #34)
> Perhaps we should do the gss_display_status method regardless of mechanism 
> when major is GSS_S_FAILURE. That should also return proper Kerberos messages 
> too.

Yes. Testing appears to show that the messages from error_message() aren't translated *anyway*, so we lose nothing by just using gss_display_status() everywhere.

(In reply to comment #26)
> I've fixed up the (null) and the confusing message, and committed the EDS part
> to EDS master:

I'd prefer to do reviews first, remember, your pet is evolution-ews, not evolution-data-server. More importantly, it behaves differently here than on your machines, thus the review and test of the changes make sense.

> This wants committing to the 3.12 branch too, assuming we're allowed to add the
> new translated string.

No, you are not allowed that, because it introduces untranslated strings. Ask for an approval first. See [1] for more info.

[1] https://wiki.gnome.org/ReleasePlanning/RequestingFreezeBreaks

Review of attachment 281353 [details] [review]:

One concern about soup-auth-negotiate - the naming is not good, because it suggests that the code comes from libsoup, not from evolution-ews. If you look into the code, then you'll see that evolution-ews has similar soup classes, they are named with an 'e' prefix, like e-soup-message and e-soup-response. Do the same, please.

Below are my comments during the patch read review. Most of it is about coding style. The testing shows it working, I can connect to the server.

The error when trying to connect with an empty kerberos cache is returned as:
> HTTP/1.1 400 SPNEGO cannot find mechanisms to negotiate
where the text after "400" gets into UI. It's expected that it comes to UI, but the error itself is confusing and doesn't give me any clue what I can do to correct it, while the previously returned "No Kerberos credentials available" is self explanatory (or easier to be understood for me). I see it's returned by the camel_sasl_gssapi, thus I guess you cannot do much about it, right?

::: evolution-ews-3.12.4/src/camel/camel-ews-store.c.gssapi-fix
@@ -1894,3 @@
-			_("You must be working online to complete this operation"));
-		return NULL;
-	}

I still do not feel like this being correct. The test in the background is to make sure the server address can be reached, not whether you can actually login to it with whatever mechanism.

::: evolution-ews-3.12.4/src/server/camel-ews-settings.c.gssapi-fix
@@ +978,3 @@
+	if (soup_uri != NULL) {
+		const gchar *host = soup_uri_get_host (soup_uri);
+		int          port = soup_uri_get_port (soup_uri);

s/int/gint/

The code uses GLib types exclusively.

::: evolution-ews-3.12.4/src/server/e-ews-connection.c.gssapi-fix
@@ +79,3 @@
+ews_dump_raw_soup_request (SoupMessage *msg);
+static void
+ews_dump_raw_soup_response (SoupMessage *msg);

no need for new lines here, right?

@@ +2324,3 @@
 	g_free (cnc->priv->password);
+	if (!password)
+		password = "";

why is this here?

@@ +2472,3 @@
+
+	log_level = e_ews_debug_get_log_level ();
+	if (log_level >= 1 && log_level < 3) {

please use log_level == 1, as it used to be before your change. This way the headers and body is printed twice when you use EWS_DEBUG=2.

@@ +2477,3 @@
+			soup_buffer_free (
+					soup_message_body_flatten (
+						SOUP_MESSAGE (msg)->request_body));

This whole 'if' is quite hard to read for me, please do not do the unnecessary new lines there, make it just a two-liner.

@@ +2502,3 @@
+			soup_buffer_free (
+					soup_message_body_flatten (
+						SOUP_MESSAGE (msg)->response_body));

similar as above, hard to read 'if' statement.

@@ +2686,3 @@
+	{
+		return;
+	}

not needed {}

::: evolution-ews-3.12.4/src/server/e-ews-connection-utils.c.gssapi-fix
@@ +49,3 @@
+ * message constructed, then through a callback that it installs, intercepts 
+ * the server reply, parses the WWW-Authenticate reply, replaces the
+ * Authorization header in the request, and requeues the request.

This comment doesn't make any sense here.

::: evolution-ews-3.12.4/src/server/soup-auth-negotiate.c.gssapi-fix
@@ +53,3 @@
+	char *challenge;
+	int  auth_started;
+	int  challenge_available;

s/char/gchar/
s/int/gint/

@@ +56,3 @@
+} SoupMessageState;
+
+static GHashTable *msgs_table;

Instead of using global memory (and eventually face thread unsafety of the GHashTable), what about storing the state on the SoupMessage itself? It's a GObject, thus you can store user data on it.

@@ +139,3 @@
+	SoupAuth *auth = SOUP_AUTH (user_data);
+	if (msg->status_code == 200)
+	{

coding style: the '{' should be at the end of the previous line

@@ +177,3 @@
+ *
+ * Since: 2.34
+ */

This comment doesn't look correct.

@@ +274,3 @@
+static gboolean
+soup_auth_negotiate_is_ready (SoupAuth *auth,
+			      SoupMessage *msg)

This is the correct way to indent function arguments. Please do it this way all around this source file. (You have sometimes multiple arguments on one line and/or the second line indentation is misplaced.)

@@ +343,3 @@
+/*
+	SoupAuthNegotiatePrivate *priv = SOUP_AUTH_NEGOTIATE_GET_PRIVATE (auth);
+*/

This looks odd.

@@ +350,3 @@
+soup_auth_negotiate_get_authorization (SoupAuth *auth, SoupMessage *msg)
+{
+	char *token;

s/char/gchar/

::: evolution-ews-3.12.4/src/server/soup-auth-negotiate.h.gssapi-fix
@@ +1,3 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
+/*
+ * Copyright (C) 2000-2003, Ximian, Inc.

Add a proper license comment here

@@ +5,3 @@
+
+#ifndef SOUP_AUTH_NEGOTIATE_H
+#define SOUP_AUTH_NEGOTIATE_H 1

It's uncommon to define it to '1', it's simply defined

@@ +10,3 @@
+
+#define SOUP_TYPE_AUTH_NEGOTIATE \
+	(soup_auth_negotiate_get_type ())

no need to break this into more lines while keeping the rest (much longer) on one line

(In reply to comment #35)
> Indeed, that's definitely a bug. Milan's proposed patch for it in
> https://bugzilla.gnome.org/show_bug.cgi?id=703181#c26 is actually remarkably
> similar to yours, isn't it? Perhaps you could just lift his
> ews_connect_check_ntlm_available() function?

Nope, you cannot. It requires a change in libsoup, which is a problem. I just asked Dan for an update on the state of the libsoup fix in that bug report.

> It's supposed to return TRUE when it thinks it *might* be able to authenticate
> without a password. It's basically an optimisation; returning TRUE every time
> (or just ripping out the method from every back end and never calling it, and
> just behaving as if it were TRUE) would *work*, but be suboptimal.

I do not agree with this. Please do not do that.

(In reply to comment #37)
> > This wants committing to the 3.12 branch too, assuming we're allowed to add the
> > new translated string.
> 
> No, you are not allowed that, because it introduces untranslated strings. Ask
> for an approval first. See [1] for more info.
> 
> [1] https://wiki.gnome.org/ReleasePlanning/RequestingFreezeBreaks

As I said, these strings were already untranslated. If we *succeed* in converting the minor code into a string, it's an untranslated string. The world is not going to end if the error message we get when we *fail* to convert the minor code into a string... is also untranslated. Which AFAICT it was already when we were using error_message() too.

But yes, I'm aware there's a process to be followed. I should have said "once we've been given approval to add the string", not "assuming we're allowed...". I was being lazy with the way I phrased that. Apologies if my meaning was unclear.

(In reply to comment #39)
> > It's supposed to return TRUE when it thinks it *might* be able to authenticate
> > without a password. It's basically an optimisation; returning TRUE every time
> > (or just ripping out the method from every back end and never calling it, and
> > just behaving as if it were TRUE) would *work*, but be suboptimal.
> 
> I do not agree with this. Please do not do that.

Would you agree with this version:

"If the password is not required, then it tries to authenticate with an empty password. If it fails for whatever reason, then the user is asked for the password and the authentication continues as before."

Better? Because that one is your *own* words, from when you introduced the get_without_password() methods.

But yes, all statements about how software works should be predicated with "except if there are bugs...". So perhaps where I said "would work", I should have said "should work". Then you would agree with what I said, yes?

I hadn't realised we still had that libsoup bug; I thought it was fixed long ago. I'll switch back to NTLM auth and retest with/without that libsoup patch. Thanks for noticing that it was unresolved.

Created attachment 281396 [details] [review]
Fix NTLM support

This is an incremental patch which fixes the NTLM issues too.

Due to libsoup bugs NTLM was broken not only with single-sign-on, but even if you just mistyped your password once. If you did, then your password would *never* be accepted because libsoup was in a broken state. You'd just keep getting asked for it over and over again, even when you got it right.

This fixes that along with the SSO issues.

OK, now I'm back to trying to split the original patch into individually reviewable and committable pieces. Here's a start, incorporating the NTLM fixes and the get_without_password() bits:

http://git.infradead.org/users/dwmw2/evolution-ews.git/shortlog/refs/heads/evolution-ews-3-12

The last commit is still a mess, but at least it only touches 8 files now not 16; the first commits are probably almost ready to be pushed (to master, although I'm testing this on 3.12)

The logging changes at least need to be split out into a patch with a clear commit comment. And I suspect you should be moving the GSSAPI code out into its own file in one patch, then *changing* it in another. The use of the 'authenticate' signal in particular needs careful though; I really don't like it at the moment. See the hacks I've had to put in place to stop it from breaking NTLM and Basic auth.

(In reply to comment #38)
> Review of attachment 281353 [details] [review]:

> Below are my comments during the patch read review. Most of it is about coding
> style. The testing shows it working, I can connect to the server.
> 
> The error when trying to connect with an empty kerberos cache is returned as:
> > HTTP/1.1 400 SPNEGO cannot find mechanisms to negotiate
> where the text after "400" gets into UI. It's expected that it comes to UI, but
> the error itself is confusing and doesn't give me any clue what I can do to
> correct it, while the previously returned "No Kerberos credentials available"
> is self explanatory (or easier to be understood for me). I see it's returned by
> the camel_sasl_gssapi, thus I guess you cannot do much about it, right?

Unfortunately, yes.  Perhaps if the call were preceded somewhere by a call to gss_acquire_cred() it could shortcut the whole try-and-fail process and return a more informative message about not finding any credentials.


> ::: evolution-ews-3.12.4/src/camel/camel-ews-store.c.gssapi-fix
> @@ -1894,3 @@
> -            _("You must be working online to complete this operation"));
> -        return NULL;
> -    }
> 
> I still do not feel like this being correct. The test in the background is to
> make sure the server address can be reached, not whether you can actually login
> to it with whatever mechanism.

At present, it is impossible (at least in my tests) to use the "Detect authentication mechanisms" button while the above lines are in--it always says you're offline in this area.  The other instances that check if the host is reachable seem to be fine though.  Maybe there's another bug that needs to be addressed?


> @@ +2324,3 @@
>      g_free (cnc->priv->password);
> +    if (!password)
> +        password = "";
> 
> why is this here?

When a backend queries gcr for a password, it will get an empty string instead of NULL, so there is a consistency issue.  Also, libsoup must have soup_auth_authenticate() called with non-NULL username and password in order for it to attempt any authentication at all regardless of mechanism.  Without that call, it just drops the 401 on the client.  So the above code assures that soup_auth_authenticate gets called when the authenticate signal is fired.


> ::: evolution-ews-3.12.4/src/server/e-ews-connection-utils.c.gssapi-fix
> @@ +49,3 @@
> + * message constructed, then through a callback that it installs, intercepts 
> + * the server reply, parses the WWW-Authenticate reply, replaces the
> + * Authorization header in the request, and requeues the request.
> 
> This comment doesn't make any sense here.

True.  With GSSAPI moved into a Soup authentication module, all mention of message hacking should be removed.  So that entire comment (which was added to explain the former hack) should be removed along with the #defines above it.


> @@ +56,3 @@
> +} SoupMessageState;
> +
> +static GHashTable *msgs_table;
> 
> Instead of using global memory (and eventually face thread unsafety of the
> GHashTable), what about storing the state on the SoupMessage itself? It's a
> GObject, thus you can store user data on it.

I debated on whether or not to continue that tradition from the old code, but after seeing that this is the method used by libsoup in soup-connection-auth.c, I decided it was better than the data attach method.  In addition, it needs to handle the "finished" message anyway to process the final token (if given) from the server on its 200 message.

But I won't argue against a return to that method, as long as the final token processing remains.


> @@ +177,3 @@
> + *
> + * Since: 2.34
> + */
> 
> This comment doesn't look correct.

It's not.  I used libsoup's soup-auth-basic.* as a template.  That does appear to be the only remaining non sequitur part though.  In addition to removing the 2.34 part, the rest of the comment could be relocated to the preamble comment.  Where it is right now looks to be more of an "oh by the way..." comment.


> @@ +274,3 @@
> +static gboolean
> +soup_auth_negotiate_is_ready (SoupAuth *auth,
> +                  SoupMessage *msg)
> 
> This is the correct way to indent function arguments. Please do it this way all
> around this source file. (You have sometimes multiple arguments on one line
> and/or the second line indentation is misplaced.)

More carryover from libsoup.


> 
> @@ +343,3 @@
> +/*
> +    SoupAuthNegotiatePrivate *priv = SOUP_AUTH_NEGOTIATE_GET_PRIVATE (auth);
> +*/
> 
> This looks odd.

In the final product, those lines can be omitted.  I only left it in in case I needed to actually do something with it in that function.  But the compiler burped about unused variables when it wasn't commented out.


> @@ +5,3 @@
> +
> +#ifndef SOUP_AUTH_NEGOTIATE_H
> +#define SOUP_AUTH_NEGOTIATE_H 1
> 
> It's uncommon to define it to '1', it's simply defined

The soup-auth-negotiate.h file was copied from libsoup's soup-auth-basic and then modified.  But no objection to this change.

Created attachment 281438 [details] [review]
Patch to delay SASL creation and "auth started" flag until a challenge is ready

I discovered another problem that surfaced after the error message changes.  Apparently libsoup can call soup_auth_xxx_is_ready() at any time.  After that last fix, ironically, calling soup_auth_negotiate_is_ready() before it's ready will cause auth to fail.

Oddly enough, Soup doesn't use this pattern on the first message, so it works then.  On future messages that need authentication, though, it fails.

Here is a patch to the last EDS patch which hopefully addresses this problem.  I'm testing it now, but thought I'd post it to delay any commits that might otherwise happen.

(In reply to comment #44)
> > > HTTP/1.1 400 SPNEGO cannot find mechanisms to negotiate

> Unfortunately, yes.  Perhaps if the call were preceded somewhere by a call to
> gss_acquire_cred() it could shortcut the whole try-and-fail process and return
> a more informative message about not finding any credentials.

Well... I note that if GSSAPI auth fails it does end up asking me for a *password*. Which is either:
 1. Bloody stupid, and we have to make it stop, or
 2. Quite sensible, and we use gss_acquire_cred_with_password() to get some.

We need to choose, and do one or the other :)

I favour the latter, which ties in quite nicely with what you said about acquiring creds first and then using them instead of passing GSS_C_NO_CREDENTIAL to gss_init_sec_context().

> At present, it is impossible (at least in my tests) to use the "Detect
> authentication mechanisms" button while the above lines are in--it always says
> you're offline in this area.  The other instances that check if the host is
> reachable seem to be fine though.  Maybe there's another bug that needs to be
> addressed?

This isn't hard. We attempt to connect to port 443 (or whatever) of the server. If a simple test request elicits a list of WWW-Authenticate: headers, then we know what the server supports (yay). And if we fail to connect, we have to handle that gracefully *anyway*, even if it's unexpected because we think we're "online".

We don't really need a *separate* test for connectivity, do we? As long as the error handling is tested for the failed-to-connect case, and as long as this is cleanly put into a separate commit with its own clear commit message (which I've done in the tree I posted above although the message was just a cut/paste), this ought to be fine.

> 
> > @@ +2324,3 @@
> >      g_free (cnc->priv->password);
> > +    if (!password)
> > +        password = "";
> > 
> > why is this here?
> 
> When a backend queries gcr for a password, it will get an empty string instead
> of NULL, so there is a consistency issue.  Also, libsoup must have
> soup_auth_authenticate() called with non-NULL username and password in order
> for it to attempt any authentication at all regardless of mechanism.

Hm, that isn't really true — libsoup only emits the authenticate signal (and requires soup_auth_authenticate() to be called) if it actually needs a password. Where it can authenticate automatically (i.e. with ntlm_auth) it doesn't do that at all. And I bet if you look at the pending patches for GSSAPI support in libsoup itself (bug 587145), they won't need soup_auth_authenticate() to be called either. We shouldn't either.

> that call, it just drops the 401 on the client.  So the above code assures that
> soup_auth_authenticate gets called when the authenticate signal is fired.

... and it also breaks NTLM auth by feeding it "" as a password when we shouldn't be calling soup_auth_authenticate() at all.

(In reply to comment #46)
> (In reply to comment #44)
> > At present, it is impossible (at least in my tests) to use the "Detect
> > authentication mechanisms" button while the above lines are in--it always says
> > you're offline in this area.
> 
> We don't really need a *separate* test for connectivity, do we? As long as the
> error handling is tested for the failed-to-connect case, and as long as this is
> cleanly put into a separate commit with its own clear commit message (which
> I've done in the tree I posted above although the message was just a
> cut/paste), this ought to be fine.

Exactly.  It will only attempt to connect for authentication detection when the user is setting up an account and presses the button.  So it is safe to attempt the connection without attempting to detect whether online or not, and return the result to the user, which appears at the bottom of the connection config window.  For normal ops with an existing account, of course, it should watch the network and only attempt to connect when online.

This test became part of the bug because before GSSAPI, there was only one connection method, NTLM.  So the button to detect methods didn't even exist before.  It was added to support the addition of GSSAPI, but it failed for me when I tried to set up an account and query the server.



> > When a backend queries gcr for a password, it will get an empty string instead
> > of NULL, so there is a consistency issue.  Also, libsoup must have
> > soup_auth_authenticate() called with non-NULL username and password in order
> > for it to attempt any authentication at all regardless of mechanism.
> 
> Hm, that isn't really true — libsoup only emits the authenticate signal (and
> requires soup_auth_authenticate() to be called) if it actually needs a
> password. Where it can authenticate automatically (i.e. with ntlm_auth) it
> doesn't do that at all. And I bet if you look at the pending patches for GSSAPI
> support in libsoup itself (bug 587145), they won't need
> soup_auth_authenticate() to be called either. We shouldn't either.

Okay, I went back to the libsoup source to study its strategy.  It looks like the purpose of soup_auth_xxx_is_authenticated() is to see if it believes the authentication exchange can eventually succeed without user input, not whether it believes authentication is necessary.  Confusing nomenclature, but I believe I have now properly grokked the documentation for this function.

So the function soup_auth_negotiate_is_authenticated() needs to return TRUE until an authentication exchange fails.  This will prevent the authenticate signal until you really do need to ask for credentials.

So they added a gss_acquire_cred_with_password() call!?  About time!  Do you know where it's documented?  Anyway, this sounds like something that should ultimately be done in the Camel SASL layer, but a TODO could be added until the API exists.

(In reply to comment #47)
> So they added a gss_acquire_cred_with_password() call!?  About time!  Do you
> know where it's documented?  Anyway, this sounds like something that should
> ultimately be done in the Camel SASL layer, but a TODO could be added until the
> API exists.

MIT krb5 has had it since 2010 when IAKERB was added. Not sure about documentation. You could perhaps do worse than look at the code in pidgin-sipe which uses it. But I strongly suspect that's one for another day.

Btw, to avoid a complaint about g_object_unref(NULL) I needed to add this:

--- a/src/server/soup-auth-negotiate.c
+++ b/src/server/soup-auth-negotiate.c
@@ -124,7 +124,8 @@ soup_auth_negotiate_delete_context (SoupMessage *msg, gpoint
                msg, G_CALLBACK (soup_auth_negotiate_message_finished), 
                user_data);
 
-       g_object_unref (state->gssapi_sasl);
+       if (state->auth_started)
+               g_object_unref (state->gssapi_sasl);
        g_free (state->token);
        g_free (state->challenge);
        g_slice_free (SoupMessageState, state);

I've also attempted to address some of Milan's feedback, merged your patch from comment 45, and pushed it out to my git tree.

That last patch needs to be split into at least two; one changing the logging, and then refactoring the GSSAPI authenticator. And that 'password=""' hack needs.... something. But not tonig^W this morning, at least not by me.

Yeah I decided to refactor e-soup-auth-negotiate (as it's now called) using the newfound info.  But I discovered that it's no closer to a solution.  And here's why:

camel_sasl_gssapi requires either a CamelService or a call to camel_sasl_gssapi_override_host_and_user to set the user and hostnames.  If it wants password info, that must come exclusively from a CamelService.  libsoup's authenticator, of course, has no idea what that is.  And its only source of user info is to return FALSE for soup_auth_negotiate_is_authenticated() which will trigger the authenticate signal.  Then if password is NULL (which it will be without the password hack), it will receive no help from EWS and it will die.


It may be necessary to use gssapi directly, which doubtless puts this even closer to the proposed libsoup patch.  Unfortunately, I see that it hasn't had any apparent movement for about a year now.  Do you know what the current show-stopper is with that?  They discuss the need for an authentication manager, a connection-based auth mech, and making NTLM a subclass of the connection based auth mech, all of which is done as far as I can see.

The remaining issues I see are 1) error reporting when the final 200 challenge fails, and 2) dependencies.

For point 1, would changing the message status to 500 with the error message be out of the question?  A failure in the final token can be taken as server failed to authenticate, so that would indeed be a server failure.

On point 2, perhaps you should allow the builder to choose one of the major GSSAPI implementations (MIT or Heimdal) or offer the option to provide the necessary include/link flags to use a 3rd-party GSSAPI implementation such as mechglue in a generic way.  The OID for SPNEGO can be included in the source so that it is not necessary to #include strange versions of gssapi.h.  And with gssntlmssp, it is not even necessary to maintain a KDC for testing purposes.

(Are you available on IRC; I wonder if some of this discussion would be better there?)

(In reply to comment #49)
> camel_sasl_gssapi requires either a CamelService or a call to
> camel_sasl_gssapi_override_host_and_user to set the user and hostnames.  If it
> wants password info, that must come exclusively from a CamelService.  libsoup's
> authenticator, of course, has no idea what that is.  And its only source of
> user info is to return FALSE for soup_auth_negotiate_is_authenticated() which
> will trigger the authenticate signal.  Then if password is NULL (which it will
> be without the password hack), it will receive no help from EWS and it will
> die.

Well.... I don't think Milan or I were really *forbidding* you from using the 'authenticate' signal and soup_auth_authenticate for this. But:

 - Be aware that it's not how a 'real' libsoup implementation would work, and
   make it clear in the (commit and code) comments that it's a hack.

 - Don't do that horrid 'if (!password) password = ""' thing in 
   ews_connection_set_password() which breaks NTLM (and maybe Basic) auth. 
   Surely you can isolate this to ews_connection_authenticate() and just call
   soup_auth_authenticate() with a locally-sourced copy of the string "" if
   the scheme is 'Negotiate'?

In fact, all you really get from the authenticate callback is the username, right? And in our case the username doesn't even get *used*, does it. I see in camel-sasl-gssapi that after GSSAPI_STATE_COMPLETE it *then* wraps the username and some other stuff with gss_wrap() to produce an extra token. I don't recognise that behaviour at all; I guess it's part of the SASL spec and not used for HTTP Negotiate anyway?

You already have the host out of the message URI. Just use "" for the username and surely you're fine?

> It may be necessary to use gssapi directly, which doubtless puts this even
> closer to the proposed libsoup patch.  Unfortunately, I see that it hasn't had
> any apparent movement for about a year now.  Do you know what the current
> show-stopper is with that?  They discuss the need for an authentication
> manager, a connection-based auth mech, and making NTLM a subclass of the
> connection based auth mech, all of which is done as far as I can see.

Indeed, I think it's fairly much all there. It mostly just needs someone to actually put it together and make sure Dan is happy with it. I wasn't really intending to make you do that, but it'd be great if you want to. But I *do* want to make sure we are aware that a libsoup implementation is (hopefully) coming soon, and have a strategy for using it if/when it does — or at least not *conflicting* with it.

So for example: If we do have a hack in ews_connection_authenticate() to do something special for our implementation, perhaps it should be checking E_SOUP_IS_AUTH_NEGOTIATE(auth) instead of just !strcmp(scheme, "Negotiate").

> The remaining issues I see are 1) error reporting when the final 200 challenge
> fails, and 2) dependencies. 
> For point 1, would changing the message status to 500 with the error message be
> out of the question?  A failure in the final token can be taken as server
> failed to authenticate, so that would indeed be a server failure.

We connect to the server over HTTPS, generally. And we check the certificate. (Tell me we do. Otherwise I'm going to curl up in the corner and cry). So arguably the server has already authenticated themselves to us. I don't see why we even need to set the GSS_C_MUTUAL_FLAG flag in our negotiation. And there are servers which don't *bother* to return WWW-Authenticate: or WWW-Authenticate-Info: with the final 200 even if there's a token to send. Because they're trying to authenticate *us*, and once they've done so there's no point in continuing.

I would be inclined not to even bother processing the final token, to be honest. Or am I missing something?

> On point 2, perhaps you should allow the builder to choose one of the major
> GSSAPI implementations (MIT or Heimdal) or offer the option to provide the
> necessary include/link flags to use a 3rd-party GSSAPI implementation such as
> mechglue in a generic way.  The OID for SPNEGO can be included in the source so
> that it is not necessary to #include strange versions of gssapi.h.  And with
> gssntlmssp, it is not even necessary to maintain a KDC for testing purposes.

Oh, absolutely. The existing camel-sasl-gssapi is a horrid mess of implementation details and I really don't understand why. We ought to be able to use only standard GSSAPI stuff and nothing KRB5-specific. I don't even see why we care about Heimdal vs. MIT.

(The reason I've context-switched back into GSSAPI stuff is because I've recently added Negotiate support to the OpenConnect VPN client. That now has autoconf magic to detect GSSAPI, and implementation-agnostic Negotiate support which has been tested on Linux, various *BSDs, Solaris and OSX without caring at all about *which* GSSAPI implementation it's using. This shouldn't be hard.)

This appears to work, and we don't need to emit the authenticate signal at all.

--- a/src/server/soup-auth-negotiate.c
+++ b/src/server/soup-auth-negotiate.c
@@ -285,7 +285,7 @@ soup_auth_negotiate_is_ready (SoupAuth *auth,
                        host = soup_uri_get_host (soup_uri);
 
                        camel_sasl_gssapi_override_host_and_user (
-                               CAMEL_SASL_GSSAPI (gssapi_sasl), host, priv->use
+                               CAMEL_SASL_GSSAPI (gssapi_sasl), host, "");
 
                        state->gssapi_sasl = gssapi_sasl;
 
@@ -325,6 +325,6 @@ soup_auth_negotiate_is_authenticated (SoupAuth *auth)
 /*
        SoupAuthNegotiatePrivate *priv = SOUP_AUTH_NEGOTIATE_GET_PRIVATE (auth);
 */
-       return FALSE;
+       return TRUE;
 }
 

If I 'kdestroy' and also disable the automatic NTLMSSP, I get prompted for a password as I did before the above change. Which isn't stunningly useful, as discussed, but at least I haven't broken the error case with the above.

Is there a reason we can't do it this way?

We can drop the bit in camel-ews-settings.c to set the host and port too, can't we? That was left over from either your attempts to fix the 'online' check, or some other issue with the SASL mech which doesn't still seem to be relevant.

And ews_connection_constructor also shouldn't be using e_ews_connection_utils_get_without_password() as the trigger for adding our Negotiate authenticator feature to the session. Check the auth-mechanism instead. And don't forget to unref the ews_settings when done with them.

I've split out the logging changes and fixed them up somewhat, and now I think we have something that's vaguely presentable as a series of individual and reviewable (and *bisectable*) changes. As before, it's at http://git.infradead.org/users/dwmw2/evolution-ews.git/shortlog/refs/heads/evolution-ews-3-12

It could certainly do with better commit messages in some places, and in particular I'm looking for *reasoning*. Why did we add the cookie jar, for example? Is that just because it seemed like a good idea, or does it actually *fix* something? If it's really necessary, do we need *persistent* cookie storage? And if it just seemed like a good idea, then presumably we won't even contemplate actually committing to the 3.12 branch. And the message restart 'fix' ... does it *matter*?

It would also be good to know precisely what practical issues are fixed, if any, by what's now the final patch that adds ESoupAuthNegotiate. Would we want to backport that to 3.12? Unless there's a *real* fix in there, I'd be inclined not to.

At this point I suspect that only the first three commits (i.e. the bottom of the list below), which fix the get_without_password() handling, are really needed in 3.12. Repeatedly prompting for an unneeded password is a horrid UI bu, and remember that even in the non-SSO case if we're using NTLM auth and the user mistypes the password then got it right on the second attempt, *that* was broken too and would just continue to fail to authenticate.

Here's what I have...

commit abf6ea7a80f6c4a8aff4fef5b4a5785c46d38f3d
Author: Daniel Sands <dnsands@sandia.gov>
Date:   Wed Jul 23 12:14:09 2014 +0100

    Clean up Negotiate auth implementation to be a SoupAuth subclass
    
    It would still be better if libsoup did this for us (cf. bug 587145) but
    this is at least slightly cleaner than what we had before. And hopefully
    leaves us in a better position to migrate to a 'real' solution provided
    by libsoup... if that ever actually materialises.

 src/server/Makefile.am              |   2 +
 src/server/e-ews-connection-utils.c | 160 -----------------
 src/server/e-ews-connection-utils.h |   4 -
 src/server/e-ews-connection.c       |  33 +++-
 src/server/e-ews-notification.c     |  14 --
 src/server/e-soup-auth-negotiate.c  | 337 ++++++++++++++++++++++++++++++++++++
 src/server/e-soup-auth-negotiate.h  |  26 +++
 7 files changed, 389 insertions(+), 187 deletions(-)

commit d24edc264e55c275b9fbb7cc03cff885898d8c93
Author: Daniel Sands <dnsands@sandia.gov>
Date:   Wed Jul 23 12:11:33 2014 +0100

    Clean up logging slightly and make it more consistent
    
    [dwmw2: Some further cleanups and reduce duplication]

 src/server/e-ews-connection.c | 97 +++++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 32 deletions(-)

commit 849f7896b308c26b20e0c5da6ea0e0771ccb7183
Author: Daniel Sands <dnsands@sandia.gov>
Date:   Wed Jul 23 12:09:01 2014 +0100

    Add SOUP_FEATURE_COOKIE_JAR to our SoupSession
    
    [dwmw2: Not sure if the lack of cookie handling has ever actually caused
            a problem, but this was hidden in a huge patch doing a bunch of
            other things, and I'm *assuming* there was a reason for it...? ]

 src/server/e-ews-connection.c | 3 +++
 1 file changed, 3 insertions(+)

commit 74aa11bcd246b83bca7233110ecde59e2b1d28d7
Author: David Woodhouse <David.Woodhouse@intel.com>
Date:   Tue Jul 22 17:05:55 2014 +0100

    Bug 732850 - Not all message restarts are due to a redirect
    
    Not all reposts are due to redirects, so the post_restarted callback
    should check the message status before assuming so.  In this case,
    reposts happen because of GSSAPI negotiation.

 src/server/e-ews-connection.c | 4 ++++
 1 file changed, 4 insertions(+)

commit e425bd212f219c6e99e48777df222316d0f3239d
Author: Daniel Sands <dnsands@sandia.gov>
Date:   Tue Jul 22 16:56:38 2014 +0100

    Bug 732850 - Remove reachability check from ews_store_query_auth_types_sync()
    
    The auth method discovery code first checks whether the "host" is reachable.
    This will always fail because it only sets its Host URL for the service, and
    the camel code relies on the host and port itself being set before it can
    answer that question.  Even after setting the host and port, it is necessary to
    give the underlying system time to query the network service.  So this check
    should not be done at all.

 src/camel/camel-ews-store.c | 8 --------
 1 file changed, 8 deletions(-)

commit b0ada60dbcaca0d7511115e2d7e030316b27a663
Author: David Woodhouse <David.Woodhouse@intel.com>
Date:   Tue Jul 22 16:42:02 2014 +0100

    Bug 703181 - NTLM authentication doesn't (always) need a password
    
    Now that we have the get_without_password() methods, and now that we know
    how to work around the libsoup bugs with recovering from failed NTLM
    authentication, make it do the right thing when Samba's ntlm_auth is
    available too.
    
    The ews_connect_check_ntlm_available() function is lifted directly from
    Milan's patch at https://bugzilla.gnome.org/703181#c26

 src/server/e-ews-connection-utils.c | 81 +++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

commit ac5b4e8b4dac0da98524c4c12b7258943e684f8e
Author: Daniel Sands <dnsands@sandia.gov>
Date:   Tue Jul 22 16:38:46 2014 +0100

    Bug 732850 - GSSAPI authentication doesn't need a password
    
    Add get_without_password() method for EWS sources, and make it return
    TRUE for GSSAPI authentication.

 src/addressbook/e-book-backend-ews.c                 | 15 +++++++++++++++
 src/calendar/e-cal-backend-ews.c                     | 15 +++++++++++++++
 src/configuration/e-ews-config-utils.c               | 10 ++++++++++
 src/configuration/e-mail-config-ews-autodiscover.c   | 19 +++++++++++++++++++
 src/configuration/e-mail-config-ews-delegates-page.c | 17 +++++++++++++++++
 src/configuration/e-mail-config-ews-oal-combo-box.c  | 20 ++++++++++++++++++++
 src/configuration/e-mail-config-ews-ooo-page.c       | 17 +++++++++++++++++
 src/server/e-ews-connection-utils.c                  | 14 ++++++++++++++
 src/server/e-ews-connection-utils.h                  |  3 +++
 src/server/e-ews-connection.c                        | 15 +++++++++++++++
 10 files changed, 145 insertions(+)

commit 2bcd73e3998a5250c61eb2becd44587381fa6445
Author: David Woodhouse <David.Woodhouse@intel.com>
Date:   Tue Jul 22 16:13:38 2014 +0100

    Work around libsoup bug 703181 in recovering from NTLM auth failures.
    
    Left to its own devices, libsoup will end up with NTLM authentication
    permanently in SOUP_NTLM_FAILED state, even if we give it a new password.
    
    Even without winbind/ntlm_auth in the picture, this was a problem. If a
    user mistyped their password once, it wouldn't be accepted on the second
    attempt even if they did get it right.
    
    With automatic NTLM authentication being attempted, this failed state was
    happening even more frequently. Hence the reversion of commit addcca8 when
    it was noticed (see bug 703181 comments 20-21).
    
    Proper fixes are going into libsoup, of course, but this serves to work
    around it for now by cancelling the message when we can't authenticate.

 src/server/e-ews-connection.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

I've committed the get_without_password() part, for GSSAPI and NTLM. As well as a workaround for the libsoup bug 703181.

https://git.gnome.org/browse/evolution-ews/commit/?id=ba4f47aa (workaround)
https://git.gnome.org/browse/evolution-ews/commit/?id=78afd10a (GSSAPI)
https://git.gnome.org/browse/evolution-ews/commit/?id=ec6ffbb1 (NTLM)

And for 3.12:

https://git.gnome.org/browse/evolution-ews/commit/?h=evolution-ews-3-12&id=c75a7a81
https://git.gnome.org/browse/evolution-ews/commit/?h=evolution-ews-3-12&id=4097e977
https://git.gnome.org/browse/evolution-ews/commit/?h=evolution-ews-3-12&id=8bfd63d5

The remaining parts of your patch are in my git repository shown above, and hopefully we're not far off being able to commit those to the master branch too. I think they're mostly waiting on improved commit messages with justifications, and perhaps a response to Milan's observation about the static GHashTable for message state?

I suspect you might get away with the potential threading issues with that as long as you ensur you call e_soup_auth_negotiate_message_finished() from the soup thread instead of directly on the "finished" signal occurring?

(In reply to comment #53)
> And ews_connection_constructor also shouldn't be using
> e_ews_connection_utils_get_without_password() as the trigger for adding our
> Negotiate authenticator feature to the session. Check the auth-mechanism
> instead. And don't forget to unref the ews_settings when done with them.

Well, I guess not by that name.  The get_without_password() call does check the auth-mechanism and returns true or false based on the result.  So if it were named as a common function for desired behavior rather than an extension to the Authenticator mechanism, it would be the correct common point by which all "do we need to ask for a password?" questions are answered.


> It could certainly do with better commit messages in some places, and in
> particular I'm looking for *reasoning*. Why did we add the cookie jar, for
> example? Is that just because it seemed like a good idea, or does it actually
> *fix* something?

Sorry, that was a test to see if maintaining the server's cookies would convince it to offer persistent authentication for the connection.  It failed.  However, the cookie that the server gives helps to maintain server affinity (in a farm setup, if I understand what they mean) so it's not a useless addition.  For its purpose, I don't believe it needs to be persistent.  It just turns out that the cookie jar doesn't help this bug any.


> And the message restart
> 'fix' ... does it *matter*?

I don't think anything was broken by skipping the check, but the reposts for authentication were causing a warning message about the libsoup redirect bug to appear on stdout.


> It would also be good to know precisely what practical issues are fixed, if
> any, by what's now the final patch that adds ESoupAuthNegotiate. Would we want
> to backport that to 3.12? Unless there's a *real* fix in there, I'd be inclined
> not to.

Yes, but it has more to do with the gcr problem.  The backends such as address book and calendar ask for a password, and gcr provides the dreaded empty string instead of NULL.  So the ews_connection_authenticate() calls soup_auth_authenticat() when the authenticate signal is triggered.  At that point, libsoup overrides all of the former hack's attempts at Negotiate authentication since it now has data for its own authenticator.  So unless gcr were fixed to return a NULL or the backends interpreted an empty password as no password, the message hacking in 3.12 will continue to fail for the backends.

9(In reply to comment #55)
> (In reply to comment #53)
> > And ews_connection_constructor also shouldn't be using
> > e_ews_connection_utils_get_without_password() as the trigger for adding our
> > Negotiate authenticator feature to the session. Check the auth-mechanism
> > instead. And don't forget to unref the ews_settings when done with them.
> 
> Well, I guess not by that name.  The get_without_password() call does check the
> auth-mechanism and returns true or false based on the result.

It's not just about the name. I already fixed the get_without_password() method to  return TRUE is cases where GSSAPI is not being used. It isn't the right condition for "shall we install the libsoup GSSAPI authenticator".
> 
> > It would also be good to know precisely what practical issues are fixed, if
> > any, by what's now the final patch that adds ESoupAuthNegotiate. Would we want
> > to backport that to 3.12? Unless there's a *real* fix in there, I'd be inclined
> > not to.
> 
> Yes, but it has more to do with the gcr problem.  The backends such as address
> book and calendar ask for a password, and gcr provides the dreaded empty string
> instead of NULL.  So the ews_connection_authenticate() calls
> soup_auth_authenticat() when the authenticate signal is triggered.  At that
> point, libsoup overrides all of the former hack's attempts at Negotiate
> authentication since it now has data for its own authenticator.  So unless gcr
> were fixed to return a NULL or the backends interpreted an empty password as no
> password, the message hacking in 3.12 will continue to fail for the backends.

OK. So to reproduce that I'd have to get GSSAPI auth to *fail* on the "without password" attempt first, then enter a password when it pointlessly asks for one, and then I'll see it hand a password to libsoup's NTLM authenticator through soup_auth_authenticate()? If we were being ultra-conservative about change in 3.12 couldn't we fix that with a simple check in ews_connection_authenticate() and just make it refuse to call soup_auth_authenticate() when it's supposed to be using GSSAPI?

(In reply to comment #50)
> (Are you available on IRC; I wonder if some of this discussion would be better
> there?)

No, but feel free to email me directly.


> Well.... I don't think Milan or I were really *forbidding* you from using the
> 'authenticate' signal and soup_auth_authenticate for this. But:
> 
>  - Don't do that horrid 'if (!password) password = ""' thing in 
>    ews_connection_set_password() which breaks NTLM (and maybe Basic) auth. 
>    Surely you can isolate this to ews_connection_authenticate() and just call
>    soup_auth_authenticate() with a locally-sourced copy of the string "" if
>    the scheme is 'Negotiate'?

Well, the point is that with a NULL password, I don't even get a username back.  But I'm just wondering though, we were talking about using gss_acquire_cred() anyway.  If that were added, then those credentials could be queried for principal info, and then camel-sasl-gssapi would not strictly require you to supply a username.  And of course if no credentials were available, it could set the error message to indicate that it couldn't find any instead of leaving you with a cryptic message about SPNEGO mechanisms.  So that idea seems better yet.


> In fact, all you really get from the authenticate callback is the username,
> right? And in our case the username doesn't even get *used*, does it.

When you use gssapi, you construct a target principal as the string "user@host" which gssapi uses to get appropriate credentials.  So yes the username is important, and it is surprising that it worked for you without it.  Did you try with Kerberos?


> I see in
> camel-sasl-gssapi that after GSSAPI_STATE_COMPLETE it *then* wraps the username
> and some other stuff with gss_wrap() to produce an extra token. I don't
> recognise that behaviour at all; I guess it's part of the SASL spec and not
> used for HTTP Negotiate anyway?

Correct, that's SASL behavior and not something we care about.  And uggh... reviewing the SASL GSSAPI RFC, it appears they have also made GSSAPI synonymous with Kerberos.  Anyway, I think camel-sasl-gssapi had been chosen by the previous implementation more for its utility than its appropriateness.


> Indeed, I think it's fairly much all there. It mostly just needs someone to
> actually put it together and make sure Dan is happy with it. I wasn't really
> intending to make you do that, but it'd be great if you want to. But I *do*
> want to make sure we are aware that a libsoup implementation is (hopefully)
> coming soon, and have a strategy for using it if/when it does — or at least not
> *conflicting* with it.

Since ESoupAuthNegotiate is a substitute authentication mechanism for Soup, it definitely helps in that respect ;)  And yes if Guido would like help, I'd be willing to provide it.


> So for example: If we do have a hack in ews_connection_authenticate() to do
> something special for our implementation, perhaps it should be checking
> E_SOUP_IS_AUTH_NEGOTIATE(auth) instead of just !strcmp(scheme, "Negotiate").

That works, but it sounds like there may not need to be any special processing in the _authenticate function, which would be even better.


> We connect to the server over HTTPS, generally. And we check the certificate.
> (Tell me we do. Otherwise I'm going to curl up in the corner and cry). So
> arguably the server has already authenticated themselves to us. I don't see why
> we even need to set the GSS_C_MUTUAL_FLAG flag in our negotiation. And there
> are servers which don't *bother* to return WWW-Authenticate: or
> WWW-Authenticate-Info: with the final 200 even if there's a token to send.
> Because they're trying to authenticate *us*, and once they've done so there's
> no point in continuing.

Yes, and the NTLM mechanism also returns no final message, so the code doesn't require it to exist.  But it is good practice to use all available tokens in case, for example, the server certificate were compromised by a Heartbleed type bug.  If using Kerberos and they didn't also compromise the KDC, you'd at least have a last line of defense.


> > On point 2, perhaps you should allow the builder to choose one of the major
> > GSSAPI implementations (MIT or Heimdal) or offer the option to provide the
> > necessary include/link flags to use a 3rd-party GSSAPI implementation such as
> > mechglue in a generic way.  The OID for SPNEGO can be included in the source so
> > that it is not necessary to #include strange versions of gssapi.h.  And with
> > gssntlmssp, it is not even necessary to maintain a KDC for testing purposes.
> 
> Oh, absolutely. The existing camel-sasl-gssapi is a horrid mess of
> implementation details and I really don't understand why. We ought to be able
> to use only standard GSSAPI stuff and nothing KRB5-specific. I don't even see
> why we care about Heimdal vs. MIT.

Other than which -l's you need at link time, or if you want to handle special cases such as expired Kerberos creds, there really isn't a reason to care about which implementation you're using.  Though in this case, point 2 was about libsoup's negotiate addition (but camel-sasl-gssapi has similar issues).

> 
> (The reason I've context-switched back into GSSAPI stuff is because I've
> recently added Negotiate support to the OpenConnect VPN client. That now has
> autoconf magic to detect GSSAPI, and implementation-agnostic Negotiate support
> which has been tested on Linux, various *BSDs, Solaris and OSX without caring
> at all about *which* GSSAPI implementation it's using. This shouldn't be hard.)

(In reply to comment #56)
> 9(In reply to comment #55)
> > Yes, but it has more to do with the gcr problem.  The backends such as address
> > book and calendar ask for a password, and gcr provides the dreaded empty string
> > instead of NULL.  So the ews_connection_authenticate() calls
> > soup_auth_authenticat() when the authenticate signal is triggered.  At that
> > point, libsoup overrides all of the former hack's attempts at Negotiate
> > authentication since it now has data for its own authenticator.  So unless gcr
> > were fixed to return a NULL or the backends interpreted an empty password as no
> > password, the message hacking in 3.12 will continue to fail for the backends.
> 
> OK. So to reproduce that I'd have to get GSSAPI auth to *fail* on the "without
> password" attempt first, then enter a password when it pointlessly asks for
> one, and then I'll see it hand a password to libsoup's NTLM authenticator
> through soup_auth_authenticate()? If we were being ultra-conservative about
> change in 3.12 couldn't we fix that with a simple check in
> ews_connection_authenticate() and just make it refuse to call
> soup_auth_authenticate() when it's supposed to be using GSSAPI?

No, you don't even have to fail.  GCR provides all the settings necessary for the backend to make a connection, not just the password.  But as part of initial setup it calls ews_connection_set_password() to install the password it got (empty but not null).  Since this password is now available, the authenticate handler gleefully provides it to libsoup, which breaks down your negotiations.

Refusing to call soup_auth_authenticate()... well, I was still in the mindset of my first patch where that caused multiple-roundtrip negotiation to fail.  But then that's because I removed the message-body signal hack.  So maybe that would work with current 3.12 changes.  Curse you for coming up with such a simple solution ;)

(In reply to comment #57)
> Well, the point is that with a NULL password, I don't even get a username back.

You already don't need a username. I already ripped that all out. See my latest version of the patch.


> When you use gssapi, you construct a target principal as the string "user@host"
> which gssapi uses to get appropriate credentials.

No, the target principal is "service@host". In this case, it's HTTP@.

You don't need the username, and in fact I've already ripped it all out. See my latest version of the patch.

Yes, I've been trying Kerberos, NTLMSSP within SPNEGO, automatic NTLM auth *and* manual NTLM auth. And various combinations of them.

> Correct, that's SASL behavior and not something we care about.  And uggh...
> reviewing the SASL GSSAPI RFC, it appears they have also made GSSAPI synonymous
> with Kerberos.  Anyway, I think camel-sasl-gssapi had been chosen by the
> previous implementation more for its utility than its appropriateness.

I'm up for ripping it out and using gss_init_sec_context() directly... it's not exactly hard. And makes our code more likely to be acceptable for inclusion directly in libsoup.

Btw, if we *really* wanted to use SoupConnectionAuth couldn't we just make it exported upstream and then use a local copy of the header when it isn't found? If it changes in future, it's already exported and we use the 'current' copy. If we're using a version of libsoup which doesn't export it, our local header is correct.

(In reply to comment #57)
> (In reply to comment #50)
> When you use gssapi, you construct a target principal as the string "user@host"
> which gssapi uses to get appropriate credentials.  So yes the username is
> important, and it is surprising that it worked for you without it.  Did you try
> with Kerberos?

Bzzzt, wrong.  That's the problem with reciting from memory.  No, it's service@host and the username isn't considered.  So yes, gssapi has no reason whatsoever to cause the authenticate signal.

(In reply to comment #59)
> Btw, if we *really* wanted to use SoupConnectionAuth couldn't we just make it
> exported upstream and then use a local copy of the header when it isn't found?
> If it changes in future, it's already exported and we use the 'current' copy.
> If we're using a version of libsoup which doesn't export it, our local header
> is correct.

I've thought about petitioning the libsoup people to export it since it could potentially be useful for someone's custom auth.  (like this one)

But I was also vaguely aware of the effort to put gssapi into libsoup so requesting they export it so I can create gssapi support seemed a bit counterproductive :)

(In reply to comment #58)
> No, you don't even have to fail.  GCR provides all the settings necessary for
> the backend to make a connection, not just the password.  But as part of
> initial setup it calls ews_connection_set_password() to install the password it
> got (empty but not null).  Since this password is now available, the
> authenticate handler gleefully provides it to libsoup, which breaks down your
> negotiations.

Aha, I've worked out why I couldn't reproduce it. My server only supports Negotiate and NTLM auth, not Basic. And since we don't *enable* NTLM if we're using GSSAPI, libsoup was never going to try authenticating for itself.

If I do tell libsoup to enable NTLM (even when we're really using GSSAPI), then it does behave as you describe. And in fact, even refusing to give it a password isn't sufficient to make NTLM stop trying to play for itself; it wants to send its own initial request packet even without credentials.

However, Basic auth is simpler. If I do fake Basic auth, it does seem that refusing to call soup_auth_authenticate() is sufficient.

The slightly cleaner option is to use soup_session_remove_feature_by_type() to disable Basic auth. But that can't be done in e_ews_connection_init() for the reasons you discovered; wed need to add a constructor. If this is supposed to be the less intrusive approach, then just refusing to provide a password is perhaps the easier option.

That isn't to say that your code isn't a very worthwhile improvement... but it might not be needed in the stable 3.12 branch, that's all :)

Hmmm, this is just a puzzler and not an action item.  But if setting an empty password makes NTLM fail, how do the calendar and address book (which set empty passwords if the system has none to offer) still work?

Is it possible that your keyring has stored passwords for those services?

Ok, I did this the clean way, adding a constructor and taking the opportunity to fix up the deprecated way were enabling NTLM auth too.

https://git.gnome.org/browse/evolution-ews/commit/?id=b6ff74a49

We do still want your better solution, but this is the minimal version which is definitely safe for 3.12 ... although TBH my vote would be for putting your new code into 3.12 too.

(In reply to comment #63)
> Hmmm, this is just a puzzler and not an action item.  But if setting an empty
> password makes NTLM fail, how do the calendar and address book (which set empty
> passwords if the system has none to offer) still work?
> 
> Is it possible that your keyring has stored passwords for those services?

So it doesn't so much "make it fail", as "when it's failing, it makes it fail *wrongly*". It goes into an infinite loop retrying the dummy request that try_password_sync() submits, with the password "", instead of letting it *fail* and return, thus prompting the user for a(nother) password.

Ah.  But the address book and calendar are also used by the Gnome desktop, so you should be seeing those prompts as soon as you log in if you don't have a stored (correct) password, assuming no ntlm_helper.  At least I did.  And if you just cancel it, then every time you bring up the activities menu and type something into the search box you'll be prompted yet again.


Also I decided to look up the cookie issue again and discovered that it may be highly important for Exchange Online accounts.  Should I go ahead and create a new bug for cookie support?

(In reply to comment #65)

> So it doesn't so much "make it fail", as "when it's failing, it makes it fail
> *wrongly*". It goes into an infinite loop retrying the dummy request that
> try_password_sync() submits, with the password "", instead of letting it *fail*
> and return, thus prompting the user for a(nother) password.

Yes, the logic for the retrying flag seems a bit odd.  Is there a reason it sets the password to NULL, then goes ahead with the quest to retrieve the password, compare it to NULL, and then skip the soup_auth_authenticate() call?

It sounds like the ideal for a rework would be for the authenticate handler itself to prompt for the new password.  But that's one of those "if it ain't broke" things.

No need for a new bug for cookies; just give me an explanation of why it's important, which can live in the commit comment, please.

The 'retrying' thing in ews_connection_authenticate() is handling the difference in API between libsoup and ESources. When libsoup emits an authenticate signal with the 'retrying' flag, indicating that the password we currently have is not good enough, we want to do *nothing* apart from clear our current idea of what the password is. The request will fail, and that'll be handled appropriately by our caller. (Well, maybe. ISTR the ESource idea of how passwords work is a lot less capable than libsoup's w.r.t. passwords changing while the software is running, etc.)

That if (retrying) bit could have just returned after clearing the password; the function *wasn't* going to do anything else. Until I added that cancel_message for NTLM in the !password case.

Created attachment 281540 [details] [review]
Use GSSAPI directly instead of SASL

FWIW here's what it would look like if we were to use GSSAPI directly instead of CamelSasl. It's an incremental patch on top of your refactoring.

Might be worth doing...

 src/server/e-soup-auth-negotiate.c | 166 +++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 81 deletions(-)

Okay.  Well here are a couple of web references about the exchange cookie.  First, an explanation of what it is:

http://blogs.msdn.com/b/exchangedev/archive/2011/07/20/client-access-server-affinity-and-network-load-balancing-considerations-for-programmatic-access-to-exchange-online.aspx

Second, an explanation of why it might be important to Evolution-EWS, at least for access to Exchange Online:

https://community.office365.com/en-us/f/148/t/44178.aspx

and:

http://gsexdev.blogspot.com/2012/01/401-erors-in-exchange-online-using-ews.html


Also, taking a closer look at the Exchange cookie, it does appear that persistent storage is better since Exchange tends to give the cookie a year's lifetime.

Review of attachment 281540 [details] [review]:

I like it mostly.  The gss_display_status() calls each need to be in their own loops though.  Both individually use the msg_ctx value for their internal message mechanisms, so when one has more than one message it will confuse the other.  Then of course there's that pesky issue that GSSAPI has not internationalized yet.

Also, now that I know we don't need the authenticate signal, I refactored the code so that e_soup_auth_negotiate_update() also does the gss part, and e_soup_auth_negotiate_is_ready() only returns whether a token is ready.  It may be about the same amount of code, but it looks a lot cleaner and fits the spirit of the soup_auth_xxx functions much better.

(In reply to comment #71)
> Review of attachment 281540 [details] [review]:
> 
> I like it mostly.  The gss_display_status() calls each need to be in their own
> loops though.

Hm, true. I'll fix that (and in OpenConnect where I lifted the code from). Thanks.

> Also, now that I know we don't need the authenticate signal, I refactored the
> code so that e_soup_auth_negotiate_update() also does the gss part, and
> e_soup_auth_negotiate_is_ready() only returns whether a token is ready.

Nice. I wondered about that. I also tried making e_soup_auth_negotiate_update() recreate the token from the 'auth_params' hash table, the same as soup_connection_auth_update() does. But we call this function directly on message completion without the auth_params hash table, which made that non-trivial.

Let's ignore the direct-to-GSSAPI thing for now; it's *definitely* too intrusive for a 3.12 backport.

do you want to let me have a patch against http://git.infradead.org/users/dwmw2/evolution-ews.git/shortlog/refs/heads/evolution-ews-3-12 which moves the SASL bit into e_soup_auth_negotiate_update() as you described?

Review of attachment 281540 [details] [review]:

::: src/server/e-soup-auth-negotiate.c
@@ +27,3 @@
 #include "e-soup-auth-negotiate.h"
 
+#include <gssapi/gssapi.h>

This breaks portability. One of the reasons to use CamelSaslGssapi was to avoid dependency issues and to reuse already written and functional code. All bad things are solved in eds already, together with the complicated krb5 detection.

I'm sorry, but I'm quite confused with this bug report. Some parts are already committed, some are in a private repository and some unreviewed patches are still here. It's a mess - with 73+ comments even bigger. At least for me.

I tested current git master (at commit 73914e5) and it looks fine. It still gives meaningless error messages for SPNEGO, but I understand it's what the library returns. Maybe having our own messages for particular error codes would help, like giving user-friendly text for the code, with a library-returned message in braces, i.e.
  "Credentials for the server not found (SPNEGO cannot find mechanisms to
   negotiate)"
or
  "Credentials for the server not found (The consistency checks performed
   on the input_token failed)"

The 3.12 branch (at commit 8bfd63d) works slightly differently with respect of error message (it still gives Kerberos errors), but that's expected and fine.

The important thing is that I can connect to my Exchange server with both NTLM (being asked for a password, as expected) and Kerberos authentication.

Nonetheless, my Exchange server seems to be too simple (or common). I *cannot* connect to an Office365.com account with git master, but can with 3.12 branch. Reverting the last two commits, to commit ec6ffbb, makes it work again.

If you are wondering, then the server returns as its first response:
< HTTP/1.1 401 Unauthorized
< Soup-Debug-Timestamp: 1406186634
< Soup-Debug: ESoapMessage 1 (0x603c450)
< Server: Microsoft-IIS/8.0
< request-id: 4bb34a7e-39bb-49b4-9e1e-aeb820cd0674
< X-Powered-By: ASP.NET
< X-FEServer: CY1PR09CA0033
< WWW-Authenticate: Basic Realm=""
< Date: Thu, 24 Jul 2014 07:23:39 GMT
< Content-Length: 0

(In reply to comment #74)
> I'm sorry, but I'm quite confused with this bug report. Some parts are already
> committed, some are in a private repository and some unreviewed patches are
> still here.

The private repository is to help keep track — that represents the current state of what we're proposing. And I'm committing things to the real repository as they become clear, are reviewed and tested...ish :)

> I tested current git master (at commit 73914e5) and it looks fine. It still
> gives meaningless error messages for SPNEGO, but I understand it's what the
> library returns. Maybe having our own messages for particular error codes
> would help, 

That's a good idea, if we can be sure we can handle the minor codes properly. Aside from being implementation-specific, I think they get mapped into a single flat number space too, so it's hard to match them and we might have to resort to actual *string* matching against the strings that are returned. I'll investigate.

Actually, MIT krb5 does have the infrastructure for translations and ought to be translating them... it's just that they don't have any translations for any languages :).  If you install some, it might actually work.

> The 3.12 branch (at commit 8bfd63d) works slightly differently with respect of
> error message (it still gives Kerberos errors), but that's expected and fine.
> The important thing is that I can connect to my Exchange server with both NTLM
> (being asked for a password, as expected) and Kerberos authentication.

Great, thanks for the test.

> Nonetheless, my Exchange server seems to be too simple (or common). I *cannot*
> connect to an Office365.com account with git master, but can with 3.12 branch.
> Reverting the last two commits, to commit ec6ffbb, makes it work again.

Oh, I think I see that. Sorry, I think that's because I'm comparing against "BASIC" not "Basic". I could use g_ascii_strcasecmp()... except that this is inexplicably NULL for NTLM instead of "NTLM", and there's no g_ascii_strcasecmp0(). Is there a reason for that, and should we 'fix' it?

Created attachment 281567 [details] [review]
Fix the auth-mechanism string compares once and for all

Argh, my "fix" to make it check for "BASIC" instead of "Basic" wasn't right either. It should be checking for "PLAIN".

But really, let's not have this special knowledge and hard-coded strings littered all over the code. This fixes it properly by introducing a camel_ews_settings_get_auth_mechanism() helper function which encodes all that knowledge, and other places can just use that.

As an added bonus, we get an enum value so that where it's appropriate, we can get complaints about unhandled values in switch statements if a new auth type gets added in the future.

Review of attachment 281567 [details] [review]:

I'm not against the helper function, it makes sense and helps too, thus I'm for it. Thanks. I have nothing functional from the patch review, only couple minor coding style "issues".

Still, I cannot connect with this patch either. As I mentioned on IRC, my preferences says NTLM, while the server advertises only Basic authentication method.

::: src/server/camel-ews-settings.h
@@ +62,3 @@
+} EwsAuthType;
+
+

coding-style: two empty new lines

::: src/server/e-ews-connection.c
@@ +663,2 @@
+		if (camel_ews_settings_get_auth_mechanism (ews_settings) ==
+		    EWS_AUTH_TYPE_GSSAPI)

coding style: I do not like these wrappings, it breaks grepping too, but more importantly, my monitor can cope with more than 80 letters per line.

::: src/server/e-ews-notification.c
@@ +362,3 @@
 
+	if (camel_ews_settings_get_auth_mechanism (ews_settings) ==
+	    EWS_AUTH_TYPE_GSSAPI)

coding style: unnecessary line wrap

OK, I've now pushed all of Daniel's changes to master, and much of it to the evolution-3-12 branch too.

For 3.12 I've so far held off on the main patch moving Negotiate auth to a SoupAuth subclass, but I think it does want to go in too.

I've just been testing 3.12 with the old code, and it's broken in a few places. It seems that every caller needs to manually install the GSSAPI hacks on their own *message*. That's done in e_ews_notification_subscribe_folder_sync() for example, but *missing* in a couple of other places (unsubscribe, get_events), so *those* requests currently fail to authenticate in 3.12 with GSSAPI. I haven't looked further afield (maybe those are the only two bugs anywhere?)

We could ponder fixing it manually or moving the setup *into* e_ews_message_new_with_header() or something, but really I think Daniel's answer is the best, and then it'll be consistent with what we have going forward. So I'm inclined to suggest that we just do that.

Thoughts?

I prefer to have consistent code changes in 3.12 and git master. It's easier for everything, including code maintenance. Why do you not want to push the same changes into 3.12 too? The only obstacle, in 3.12, are newly added (translatable) strings, while I guess there are none with respect of evolution-ews? There had been some in evolution-data-server.

(In reply to comment #79)
> I prefer to have consistent code changes in 3.12 and git master. It's easier
> for everything, including code maintenance

Absolutely. That is my preference too. I just wanted to give you the opportunity to object before I did it!

> The only obstacle, in 3.12, are newly added
> (translatable) strings, while I guess there are none with respect of
> evolution-ews? There had been some in evolution-data-server.

Right. There are no new translatable strings on the EWS side.

The EDS change is separate and orthogonal — that fixes the SASL mech to use SPNEGO instead of bare KRB5 for HTTP, which means that GSS-NTLMSSP can work.

We'll want that in 3.12 too, but we need to follow the process. (Alternatively, we could cheat. When the message fetching fails, we could just call error_message() with the unknown minor code. That always *did* return an untranslated string "Unknown error", but it was hidden from the bureaucrats :)

The only reason I haven't actually *pushed* the EWS part to 3.12 immediately on seeing your above response is because I need to work out how to handle the addition of the new camel_ews_settings_get_auth_mechanism() function. Can I just mark it as 'Since: 3.12.5' instead of 'Since: 3.14'?

(In reply to comment #80)
> The only reason I haven't actually *pushed* the EWS part to 3.12 immediately on
> seeing your above response is because I need to work out how to handle the
> addition of the new camel_ews_settings_get_auth_mechanism() function. Can I
> just mark it as 'Since: 3.12.5' instead of 'Since: 3.14'?

Oops, I'm sorry, this slipped around me unnoticed. I do this too, just mark the version it was added in. It is kind of useless, because the evolution-ews is not used by anything (as a library), or might not be used, thus it's API free (the ABI constraints still apply), but having there a version when it was added looks correct and useful.

I'd like to get rid of this bug report, namely its patches, to get it out of my radar. Can we do it now?

3.12 is fairly dead now, and I think everything went into HEAD. Let's close this now.

Thanks for all the work on this, Daniel!