[REGRESSION 2.10.2] problematic "empty auth" changes

[REGRESSION 2.10.2] problematic "empty auth" changes

[Johannes Schindelin]

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?

Ciao,
Dscho

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

[David Turner]

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.]

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

[Johannes Schindelin]

Hi David,

On Thu, 8 Dec 2016, David Turner wrote:

> On Thu, 2016-12-08 at 15:47 +0100, Johannes Schindelin wrote:
> 
> > 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.

Good.

> 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.

Okay. I think I will extend that coverage rather boldly, then ;-)

> 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.

Makes sense.

> That said, I could imagine that there are cases where it would cause
> failures for a different set of users.

Let's see. At the moment, my main concern is that Git for Windows is
broken for corporate users (i.e. users who rely on the implicit
login authentication provided through their domain accounts). I cannot
imagine that defaulting http.emptyAuth=true could cause any worse
breakage.

It would be different, of course, if http.emptyAuth would *not* allow the
user to type their credentials when accessing something like
https://github.com/dscho/shhh-secret-repository, *only* trying the login
credentials. But that is not the case, with http.emptyAuth=true, login
credentials are attempted first, and when they fail, the user is still
asked interactively for their credentials.

All I can see is that this would be *an improvement*: corporate users
trying to access a Git repository that requires their login credentials
would now not even need to enter empty user name/password.

This alone would be already a good reason to change the default, IMHO.

So here is my plan:

- change the default of http.emptyAuth to true in the next Git for Windows
  version

- publish a prerelease for early adopters to test

- contribute this patch here on the Git mailing list, in the hope that it
  will make it into the next major version

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It would be different, of course, if http.emptyAuth would *not* allow the
> user to type their credentials when accessing something like
> https://github.com/dscho/shhh-secret-repository, *only* trying the login
> credentials. But that is not the case, with http.emptyAuth=true, login
> credentials are attempted first, and when they fail, the user is still
> asked interactively for their credentials.
>
> All I can see is that this would be *an improvement*: corporate users
> trying to access a Git repository that requires their login credentials
> would now not even need to enter empty user name/password.

Yup, my thought process after seeing your first message to David
exactly mirrored the above two paragraphs.  It sounds like you two
have a good plan ;-)

Thanks.

> This alone would be already a good reason to change the default, IMHO.
>
> So here is my plan:
>
> - change the default of http.emptyAuth to true in the next Git for Windows
>   version
>
> - publish a prerelease for early adopters to test
>
> - contribute this patch here on the Git mailing list, in the hope that it
>   will make it into the next major version
>
> Ciao,
> Dscho

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Thu, Dec 08, 2016 at 04:12:32PM -0500, David Turner wrote:
> 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.

Let me echo this.  This would make Kerberos (and probably other forms of
SPNEGO) work out of the box, which would reduce a lot of confusion that
people have.

I can confirm enabling http.emptyAuth works properly with Kerberos,
including with fallback to Basic, so I see no reason why we shouldn't do
it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Johannes Schindelin]

Hi Brian,

On Fri, 9 Dec 2016, brian m. carlson wrote:

> On Thu, Dec 08, 2016 at 04:12:32PM -0500, David Turner wrote:
> > 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.
> 
> Let me echo this.  This would make Kerberos (and probably other forms of
> SPNEGO) work out of the box, which would reduce a lot of confusion that
> people have.
> 
> I can confirm enabling http.emptyAuth works properly with Kerberos,
> including with fallback to Basic, so I see no reason why we shouldn't do
> it.

One of my colleagues offered a legitimate concern: it potentially adds
another round-trip.

Do you happen to know whether regular HTTPS negotiation will have an extra
round-trip if Kerberos is attempted, but we have to fall back to
interactively prompt for (or use stored) credentials?

Ciao,
Johannes

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

On Sat, Dec 10, 2016 at 03:52:39PM +0100, Johannes Schindelin wrote:
> One of my colleagues offered a legitimate concern: it potentially adds
> another round-trip.
> 
> Do you happen to know whether regular HTTPS negotiation will have an extra
> round-trip if Kerberos is attempted, but we have to fall back to
> interactively prompt for (or use stored) credentials?

With Kerberos (using tickets), you have 7 request/response pairs, and an
additional round trip for the 100 Continue if your push is larger than
http.postBuffer.  You only have 6 for Basic using Kerberos.

However, libcurl is generally going to be able to figure out whether
your Kerberos credentials can be used, so when it falls back to Basic,
it does so because it knows you have nothing to use with Negotiate (e.g.
you have no ticket), and therefore it doesn't even try.  I suppose if I
tried to push to a server that offered Negotiate and Basic, but didn't
accept my Kerberos credentials, it might fall back in such a way,
though.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]