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

David Turner [off-list ref] 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.

On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:

David Turner [off-list ref] writes:

quoted

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.

-Peff

Hi Peff,

On Wed, 22 Feb 2017, Jeff King wrote:

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

On Thu, Feb 23, 2017 at 06:08:49PM +0100, Johannes Schindelin wrote:

quoted

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.

I suspect it isn't enough to help without 2/2. This will tell curl that
the server does not do Negotiate, so it will skip the probe request. But
Git will still feed curl the bogus empty credential.

That's what 2/2 tries to fix: only kick in the emptyAuth hack when there
is something besides Basic[1] to try. The way it is written adds an
extra "auto" mode to emptyAuth, as I wanted to leave "emptyauth=true" as
a workaround in case the "auto" behavior does not work. And then I
turned on "auto" by default, since that was what the discussion was
shooting for.

But if we are worried about turning on emptyAuth everywhere, the auto
behavior could be tied to emptyauth=true (and have something like
"emptyauth=always" to _really_ force it). I don't have an opinion there.
It sounds like emptyauth has been enabled by default on Windows for a
while. It's not clear to me if that's a security problem or not.

-Peff