Re: [PATCH 2/3] http: attempt Negotiate auth in http.emptyAuth=auto mode

[Junio C Hamano <gitster@pobox.com>]

"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> When a server advertises Negotiate (SPNEGO) authentication, the
> "auto" mode of http.emptyAuth should detect this as an "exotic"
> method and proactively send empty credentials, allowing libcurl to
> use the system Kerberos ticket without prompting the user.
>
> However, two features interact to prevent this from working:
>
> The Negotiate-stripping logic, introduced in 4dbe66464b
> (remote-curl: fall back to Basic auth if Negotiate fails,
> 2015-01-08), removes CURLAUTH_GSSNEGOTIATE from the allowed
> methods on the first 401 response. The empty-auth auto-detection,
> introduced in 40a18fc77c (http: add an "auto" mode for
> http.emptyauth, 2017-02-25), then checks the remaining methods
> for anything "exotic" -- but Negotiate has already been removed,
> so auto mode never activates for servers whose only non-Basic/Digest
> method is Negotiate (e.g., Apache with mod_auth_kerb offering
> Basic + Negotiate).

Well explained.

> Fix this by delaying the Negotiate stripping in auto mode: on the
> first 401, keep Negotiate in the allowed methods so that auto mode
> can detect it and retry with empty credentials. If that attempt
> fails (no valid Kerberos ticket), strip Negotiate on the second 401
> and fall through to credential_fill() as usual.

OK, succeeding after two attempts is much better than failing after
only one attempt.

> To support this, also teach http_reauth_prepare() to skip
> credential_fill() when empty auth is about to be attempted, since
> filling real credentials would bypass the empty-auth mechanism.

And that is why the previous step shines.  Very neat.

> The true and false modes are unchanged: true sends empty credentials
> on the very first request (before any 401), and false never sends
> them.

OK.  This is a tangent, but "git config --help" on "http.emptyAuth"
is horrible.  It does not say what the allowed values are, so I had
to first write "There are million other things in the system that
this patch does not modify, so what's the point of singling out
these two settings and saying that this patch does not change
them?", before realizing that 'auto' the patch (and the explanation
of the "empty-autho auto-detction" above) is about the third
possiblity of the same variable and take it back.

> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
>  http.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index f208e0ad82..1c7ea32ef2 100644
> --- a/http.c
> +++ b/http.c
> @@ -138,6 +138,7 @@ static unsigned long empty_auth_useless =
>  	CURLAUTH_BASIC
>  	| CURLAUTH_DIGEST_IE
>  	| CURLAUTH_DIGEST;
> +static int empty_auth_try_negotiate;
>  
>  static struct curl_slist *pragma_header;
>  static struct string_list extra_http_headers = STRING_LIST_INIT_DUP;

I guess the existing code already assumes that we connect to a
single destination, run a single "session", and then die, so it is
in line with the existing design to have a file-scope global keep
track of our "state".  In the longer run we may want to move these
things to a struct so that we can run multiple sessions without
having to kill ourselves and restart, but that is totally outside
the topic of these patches to fix the negotiate auth.

> @@ -667,6 +668,17 @@ static void init_curl_http_auth(CURL *result)
>  
>  void http_reauth_prepare(int all_capabilities)
>  {
> +	/*
> +	 * If we deferred stripping Negotiate to give empty auth a
> +	 * chance (auto mode), skip credential_fill on this retry so
> +	 * that init_curl_http_auth() sends empty credentials and
> +	 * libcurl can attempt Negotiate with the system ticket cache.
> +	 */
> +	if (empty_auth_try_negotiate &&
> +	    !http_auth.password && !http_auth.credential &&
> +	    (http_auth_methods & CURLAUTH_GSSNEGOTIATE))
> +		return;
> +
>  	credential_fill(the_repository, &http_auth, all_capabilities);
>  }
>  
> @@ -1895,7 +1907,18 @@ static int handle_curl_result(struct slot_results *results)
>  				http_proactive_auth = PROACTIVE_AUTH_NONE;
>  			return HTTP_NOAUTH;
>  		} else {
> -			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
> +			if (curl_empty_auth == -1 &&
> +			    !empty_auth_try_negotiate &&
> +			    (results->auth_avail & CURLAUTH_GSSNEGOTIATE)) {
> +				/*
> +				 * In auto mode, give Negotiate a chance via
> +				 * empty auth before stripping it. If it fails,
> +				 * we will strip it on the next 401.
> +				 */
> +				empty_auth_try_negotiate = 1;
> +			} else {
> +				http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
> +			}
>  			if (results->auth_avail) {
>  				http_auth_methods &= results->auth_avail;
>  				http_auth_methods_restricted = 1;