Re: [REGRESSION 2.10.2] problematic "empty auth" changes

[David Turner <novalis@novalis.org>]

On Thu, 2016-12-08 at 15:47 +0100, Johannes Schindelin wrote:
> Hi Dave,
> 
> I got a couple of bug reports that claim that 2.10.2 regressed on using
> network credentials. That is, users regularly hit Enter twice when being
> asked for user name and password while fetching via https://, and cURL
> automatically used to fall back to using the login credentials (i.e.
> authenticating via the Domain controller).
> 
> Turns out those claims are correct: hitting Enter twice (or using URLs
> with empty user name/password such as https://:tfs:8080/) work in 2.10.1
> and yield "Authentication failed" in 2.10.2.
> 
> I tracked this down to 5275c3081c (http: http.emptyauth should allow empty
> (not just NULL) usernames, 2016-10-04) which all of a sudden disallowed
> empty user names (and now only handles things correctly when
> http.emptyAuth is set to true specifically).
> 
> This smells like a real bad regression to me, certainly given the time I
> had to spend to figure this out (starting from not exactly helpful bug
> reports, due to being very specific to their setups being private).
> 
> I am *really* tempted to change the default of http.emptyAuth to true, *at
> least* for Windows (where it is quite common to use your login credentials
> to authenticate to corporate servers).
> 
> Before I do anything rash, though: Do you see any downside to that?

I know of no reason that shouldn't work.  Indeed, it's what we use do
internally.  So far, nobody has reported problems.  That said, we have
exactly three sets of git servers that most users talk to (two different
internal; and occasionally github.com for external stuff).  So our
coverage is not very broad.

If you're going to do it, tho, don't just do it for Windows users -- do
it for everyone.  Plenty of Unix clients connect to Windows-based auth
systems.

That said, I could imagine that there are cases where it would cause
failures for a different set of users.  I don't know of any off the top
of my head, but this is not my area of expertise.

We could move closer to the old behavior with something like:

        if (!http_auth.username || !*http_auth.username) {
                if (curl_empty_auth)
                        curl_easy_setopt(result, CURLOPT_USERPWD, ":");
                if (!http_auth.username)
                        return;
        }

This is very ad-hoc, in that I have not examined exactly when
http_auth.username would be null vs empty; it merely attempts to get as
close as possible to the old behavior.

[Side note: I was curious if 26a7b23429 would have been a better fix for
our problem.  It appears that the answer is no; using that patch but
minus my 5275c3081c does not work for us.]