[REGRESSION 2.10.2] problematic "empty auth" changes
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
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
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
Johannes Schindelin 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
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 signature.asc Description: PGP signature
Re: [REGRESSION 2.10.2] problematic "empty auth" changes
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
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 signature.asc Description: PGP signature