Re: [PATCH] http(s): automatically try NTLM authentication first

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Wed, 22 Feb 2017, Jeff King wrote:

> On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:
> 
> > David Turner <David.Turner@twosigma.com> writes:
> > 
> > > Always, no.  For failed authentication (or authorization),
> > > apparently, yes.  I tested this by  setting the variable to false
> > > and then true, and trying to Push to a github repository which I
> > > didn't have write access to, with both an empty username
> > > (https://@:github.com/...) and no username (http://github.com/...).
> > > I ran this under GIT_CURL_VERBOSE=1 and I saw two 401 responses in
> > > the "http.emptyauth=true" case and one in the false case.  I also
> > > tried with a repo that I did have access to (first configuring the
> > > necessary tokens for HTTPS push access), and saw two 401 responses
> > > in *both* cases.  
> > 
> > Thanks; that matches my observation.  I do not think we care about
> > an extra roundtrip for the failure case, but as long as we do not
> > increase the number of roundtrip in the normal case, we can declare
> > that this is an improvement.  I am not quite sure where that extra
> > 401 comes from in the normal case, and that might be an indication
> > that we already are doing something wrong, though.
> 
> This patch drops the useless probe request:
> 
> diff --git a/http.c b/http.c
> index 943e630ea..7b4c2db86 100644
> --- a/http.c
> +++ b/http.c
> @@ -1663,6 +1663,9 @@ static int http_request(const char *url,
>  		curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
>  				options->effective_url);
>  
> +	if (results.auth_avail == CURLAUTH_BASIC)
> +		http_auth_methods = CURLAUTH_BASIC;
> +
>  	curl_slist_free_all(headers);
>  	strbuf_release(&buf);
>  
> 
> but setting http.emptyauth adds back in the useless request. I think
> that could be fixed by skipping the empty-auth thing when
> http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps
> other methods need it to, so maybe skip it if _just_ BASIC is set).
> 
> I suspect the patch above could probably be generalized as:
> 
>   /* cut out methods we know the server doesn't support */
>   http_auth_methods &= results.auth_avail;
> 
> and let curl figure it out from there.

Maybe this patch (or a variation thereof) would also be able to fix this
problem with the patch:

	https://github.com/git-for-windows/git/issues/1034

Short version: for certain servers (that do *not* advertise Negotiate),
setting emptyauth to true will result in a failed fetch, without letting
the user type in their credentials.

Ciao,
Johannes