{"thread":{"id":"45195","subject":"[PATCH] http(s): automatically try NTLM authentication first","startedAt":"2017-02-22T17:40:07Z","lastAt":"2017-02-28T10:21:43Z","messageCount":38,"participants":["David Turner","Junio C Hamano","Jeff King","brian m. carlson","Mantas Mikulėnas","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312322","messageId":"20170222173936.25016-1-dturner@twosigma.com","threadId":"45195","inReplyTo":null,"subject":"[PATCH] http(s): automatically try NTLM authentication first","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-02-22T17:39:36Z","receivedAt":"2017-02-22T17:40:07Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: Johannes Schindelin \n\nIt is common in corporate setups to have permissions managed via a\ndomain account. That means that the user does not really have to log in\nwhen accessing a central repository via https://, but that the login\ncredentials are used to authenticate with that repository.\n\nThe common way to do that used to require empty credentials, i.e. hitting\nEnter twice when being asked for user name and password, or by using the\nvery funny notation https://:@server/repository\n\nA recent commit (5275c3081c (http: http.emptyauth should allow empty (not\njust NULL) usernames, 2016-10-04)) broke that usage, though, all of a\nsudden requiring users to set http.emptyAuth = true.\n\nWhich brings us to the bigger question why http.emptyAuth defaults to\nfalse, to begin with.\n\nIt would be one thing if cURL would not let the user specify credentials\ninteractively after attempting NTLM authentication (i.e. login\ncredentials), but that is not the case.\n\nIt would be another thing if attempting NTLM authentication was not\nusually what users need to do when trying to authenticate via https://.\nBut that is also not the case.\n\nSo let's just go ahead and change the default, and unbreak the NTLM\nauthentication. As a bonus, this also makes the \"you need to hit Enter\ntwice\" (which is hard to explain: why enter empty credentials when you\nwant to authenticate with your login credentials?) and the \":@\" hack\n(which is also pretty, pretty hard to explain to users) obsolete.\n\nThis fixes https://github.com/git-for-windows/git/issues/987\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: David Turner \n---\nThis has been in git for Windows for a few months (without the\nconfig.txt change). We've also been using it internally. So I think\nit's time to merge back to upstream git.\n\n Documentation/config.txt | 3 ++-\n http.c | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fc5a28a320..b0da64ed33 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1742,7 +1742,8 @@ http.emptyAuth::\n \tAttempt authentication without seeking a username or password. This\n \tcan be used to attempt GSS-Negotiate authentication without specifying\n \ta username in the URL, as libcurl normally requires a username for\n-\tauthentication.\n+\tauthentication. Default is true, since if this fails, git will fall\n+\tback to asking the user for their username/password.\n \n http.delegation::\n \tControl GSSAPI credential delegation. The delegation is disabled\ndiff --git a/http.c b/http.c\nindex 90a1c0f113..943e630ea6 100644\n--- a/http.c\n+++ b/http.c\n@@ -109,7 +109,7 @@ static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n-static int curl_empty_auth;\n+static int curl_empty_auth = 1;\n \n enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;\n \n-- \n2.11.GIT\n\n"},{"id":"312335","messageId":"xmqqpoiaasgj.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170222173936.25016-1-dturner@twosigma.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-22T20:19:56Z","receivedAt":"2017-02-22T20:22:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> From: Johannes Schindelin \n>\n> It is common in corporate setups to have permissions managed via a\n> domain account. That means that the user does not really have to log in\n> when accessing a central repository via https://, but that the login\n> credentials are used to authenticate with that repository.\n>\n> The common way to do that used to require empty credentials, i.e. hitting\n> Enter twice when being asked for user name and password, or by using the\n> very funny notation https://:@server/repository\n>\n> A recent commit (5275c3081c (http: http.emptyauth should allow empty (not\n> just NULL) usernames, 2016-10-04)) broke that usage, though, all of a\n> sudden requiring users to set http.emptyAuth = true.\n>\n> Which brings us to the bigger question why http.emptyAuth defaults to\n> false, to begin with.\n\nThis is a valid question, and and I do not see it explicitly asked\nin the thread:\n\nhttps://public-inbox.org/git/CAPig+cSphEu3iRJrkdBA+BRhi9HnopLJnKOHVuGhUqavtV1RXg@mail.gmail.com/#t\n\neven though there is a hint of it already there.\n\n> It would be one thing if cURL would not let the user specify credentials\n> interactively after attempting NTLM authentication (i.e. login\n> credentials), but that is not the case.\n>\n> It would be another thing if attempting NTLM authentication was not\n> usually what users need to do when trying to authenticate via https://.\n> But that is also not the case.\n\nSome other possible worries we may have had I can think of are:\n\n - With this enabled unconditionally, would we leak some information?\n\n - With this enabled unconditionally, would we always incur an extra\n roundtrip for people who are not running NTLM at all?\n\nI do not think the former is the case, but what would I know (adding a\nfew people involved in the original thread to CC: ;-)\n\n> Documentation/config.txt | 3 ++-\n> http.c | 2 +-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index fc5a28a320..b0da64ed33 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1742,7 +1742,8 @@ http.emptyAuth::\n> \tAttempt authentication without seeking a username or password. This\n> \tcan be used to attempt GSS-Negotiate authentication without specifying\n> \ta username in the URL, as libcurl normally requires a username for\n> -\tauthentication.\n> +\tauthentication. Default is true, since if this fails, git will fall\n> +\tback to asking the user for their username/password.\n> \n> http.delegation::\n> \tControl GSSAPI credential delegation. The delegation is disabled\n> diff --git a/http.c b/http.c\n> index 90a1c0f113..943e630ea6 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -109,7 +109,7 @@ static int curl_save_cookies;\n> struct credential http_auth = CREDENTIAL_INIT;\n> static int http_proactive_auth;\n> static const char *user_agent;\n> -static int curl_empty_auth;\n> +static int curl_empty_auth = 1;\n> \n> enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;\n"},{"id":"312341","messageId":"97ab9a812f7b46d7b10d4d06f73259d8@exmbdft7.ad.twosigma.com","threadId":"45195","inReplyTo":"xmqqpoiaasgj.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH] http(s): automatically try NTLM authentication first","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-22T21:04:14Z","receivedAt":"2017-02-22T21:06:06Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C\n> Hamano\n> Sent: Wednesday, February 22, 2017 3:20 PM\n> To: David Turner \n> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; Johannes Schindelin\n> ; Eric Sunshine\n> ; Jeff King \n> Subject: Re: [PATCH] http(s): automatically try NTLM authentication first\n> \n> David Turner writes:\n> \n> > From: Johannes Schindelin \n> >\n> > It is common in corporate setups to have permissions managed via a\n> > domain account. That means that the user does not really have to log\n> > in when accessing a central repository via https://, but that the\n> > login credentials are used to authenticate with that repository.\n> >\n> > The common way to do that used to require empty credentials, i.e.\n> > hitting Enter twice when being asked for user name and password, or by\n> > using the very funny notation https://:@server/repository\n> >\n> > A recent commit (5275c3081c (http: http.emptyauth should allow empty\n> > (not just NULL) usernames, 2016-10-04)) broke that usage, though, all\n> > of a sudden requiring users to set http.emptyAuth = true.\n> >\n> > Which brings us to the bigger question why http.emptyAuth defaults to\n> > false, to begin with.\n> \n> This is a valid question, and and I do not see it explicitly asked in the thread:\n> \n> https://public-\n> inbox.org/git/CAPig+cSphEu3iRJrkdBA+BRhi9HnopLJnKOHVuGhUqavtV1RXg\n> @mail.gmail.com/#t\n> \n> even though there is a hint of it already there.\n> \n> > It would be one thing if cURL would not let the user specify\n> > credentials interactively after attempting NTLM authentication (i.e.\n> > login credentials), but that is not the case.\n> >\n> > It would be another thing if attempting NTLM authentication was not\n> > usually what users need to do when trying to authenticate via https://.\n> > But that is also not the case.\n> \n> Some other possible worries we may have had I can think of are:\n> \n> - With this enabled unconditionally, would we leak some information?\n\nI think \"NTLM\" is actually a misnomer here (I just copied Johannes's \ncommit message). The mechanism is actually SPNEGO, if I understand this \ncorrectly. It seems to me that this is probably secure, since it is apparently\nwidely implemented in browsers.\n\n> - With this enabled unconditionally, would we always incur an extra\n> roundtrip for people who are not running NTLM at all?\n>\n> I do not think the former is the case, but what would I know (adding a few\n> people involved in the original thread to CC: ;-)\n\nAlways, no. For failed authentication (or authorization), apparently, yes. \nI tested this by setting the variable to false and then true, and trying to \nPush to a github repository which I didn't have write access to, with \nboth an empty username (https://@:github.com/...) and no username \n(http://github.com/...). I ran this under GIT_CURL_VERBOSE=1 and\nI saw two 401 responses in the \"http.emptyauth=true\" case and one\nin the false case. I also tried with a repo that I did have access to (first\nconfiguring the necessary tokens for HTTPS push access), and saw two\n401 responses in *both* cases. \n\n"},{"id":"312343","messageId":"20170222210636.k2ps3qhhpiyyv6cp@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqqpoiaasgj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T21:06:36Z","receivedAt":"2017-02-22T21:13:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2017 at 12:19:56PM -0800, Junio C Hamano wrote:\n\n> > It would be one thing if cURL would not let the user specify credentials\n> > interactively after attempting NTLM authentication (i.e. login\n> > credentials), but that is not the case.\n> >\n> > It would be another thing if attempting NTLM authentication was not\n> > usually what users need to do when trying to authenticate via https://.\n> > But that is also not the case.\n> \n> Some other possible worries we may have had I can think of are:\n> \n> - With this enabled unconditionally, would we leak some information?\n> \n> - With this enabled unconditionally, would we always incur an extra\n> roundtrip for people who are not running NTLM at all?\n> \n> I do not think the former is the case, but what would I know (adding a\n> few people involved in the original thread to CC: ;-)\n\nI don't think it incurs an extra round-trip now, because of the way\nlibcurl works. Though I think it _does_ make it harder for curl to later\noptimize out that extra round-trip.\n\nThe easiest way to see the difference is to run something like:\n\n GIT_CURL_VERBOSE=1 \\\n git ls-remote https://example.com/repo-which-needs-auth.git 2>trace\n egrep '^>|^< HTTP|Authorization' trace\n\nBefore this patch, I get (this is against github.com, which only does\nBasic auth):\n\n > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n < HTTP/1.1 401 Authorization Required\n > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n < HTTP/1.1 401 Authorization Required\n > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Authorization: Basic \n < HTTP/1.1 200 OK\n\nAnd after I get:\n\n > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n < HTTP/1.1 401 Authorization Required\n > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Authorization: Basic Og==\n < HTTP/1.1 401 Authorization Required\n > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Authorization: Basic \n < HTTP/1.1 200 OK\n\nIn the current trace, you can see that libcurl insists on making a\nsecond auth-less request after we've fed it credentials. I'm not sure\nhow to get rid of this useless extra round-trip, but it would be nice to\ndo so (IIRC, it is a probe request to find out the list of auth types\nthat the server supports, which are not remembered from the previous\nrequest).\n\nWith http.emptyauth, the second round-trip _isn't_ useless. It's trying\nto send the empty credential.\n\nSo while curl isn't currently optimizing out the second call, I think\nhttp.emptyauth makes it harder to do the right thing. That's probably\nfixable if the logic ends up more like:\n\n - curl reports a 401 to us; actually look at the list of auth methods.\n\n - if there was gss-negotiate, then kick in the empty-auth magic\n automatically.\n\n - if empty-auth failed (or if we decided not to try it), ask for a\n credential and retry the request. Either way, tell curl that we want\n to use \"Basic\" so it doesn't have to do the probe request (and\n obviously if the server did not support Basic, then fail\n immediately).\n\nI think that would keep it to 2 round-trips for the normal \"Basic\" case,\nas well as for the GSSNegotiate case. It would be 3 requests when the\nserver offers GSSNegotiate but you can't use it (but you could set\nhttp.emptyauth=false to optimize that out).\n\nThat's all theoretical, though. I might not even be right about the\nreason for the second request, and I certainly haven't written any code\n(nor do I have a GSSNegotiate setup to test against).\n\n-Peff\n"},{"id":"312344","messageId":"xmqq8toyapu6.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"97ab9a812f7b46d7b10d4d06f73259d8@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-22T21:16:33Z","receivedAt":"2017-02-22T21:16:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> Always, no. For failed authentication (or authorization), apparently, yes. \n> I tested this by setting the variable to false and then true, and trying to \n> Push to a github repository which I didn't have write access to, with \n> both an empty username (https://@:github.com/...) and no username \n> (http://github.com/...). I ran this under GIT_CURL_VERBOSE=1 and\n> I saw two 401 responses in the \"http.emptyauth=true\" case and one\n> in the false case. I also tried with a repo that I did have access to (first\n> configuring the necessary tokens for HTTPS push access), and saw two\n> 401 responses in *both* cases. \n\nThanks; that matches my observation. I do not think we care about\nan extra roundtrip for the failure case, but as long as we do not\nincrease the number of roundtrip in the normal case, we can declare\nthat this is an improvement. I am not quite sure where that extra\n401 comes from in the normal case, and that might be an indication\nthat we already are doing something wrong, though.\n\n\n\n"},{"id":"312345","messageId":"xmqq4lzlc408.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170222210636.k2ps3qhhpiyyv6cp@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-22T21:25:11Z","receivedAt":"2017-02-22T21:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't think it incurs an extra round-trip now, because of the way\n> libcurl works. Though I think it _does_ make it harder for curl to later\n> optimize out that extra round-trip.\n> ...\n> In the current trace, you can see that libcurl insists on making a\n> second auth-less request after we've fed it credentials. I'm not sure\n> how to get rid of this useless extra round-trip, but it would be nice to\n> do so (IIRC, it is a probe request to find out the list of auth types\n> that the server supports, which are not remembered from the previous\n> request).\n> ...\n> With http.emptyauth, the second round-trip _isn't_ useless. It's trying\n> to send the empty credential.\n> \n> So while curl isn't currently optimizing out the second call, I think\n> http.emptyauth makes it harder to do the right thing.\n> ...\n> I think that would keep it to 2 round-trips for the normal \"Basic\" case,\n> as well as for the GSSNegotiate case. It would be 3 requests when the\n> server offers GSSNegotiate but you can't use it (but you could set\n> http.emptyauth=false to optimize that out).\n\nThanks for your thoughts. I'd think that we should take this change\nand leave the optimization for later, then. It's not like the\nchange of the default is making the normal situation any worse, it\nseems.\n\n\n"},{"id":"312346","messageId":"20170222213410.iak43asq775tzr42@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqq8toyapu6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T21:34:10Z","receivedAt":"2017-02-22T21:34:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:\n\n> David Turner writes:\n> \n> > Always, no. For failed authentication (or authorization), apparently, yes. \n> > I tested this by setting the variable to false and then true, and trying to \n> > Push to a github repository which I didn't have write access to, with \n> > both an empty username (https://@:github.com/...) and no username \n> > (http://github.com/...). I ran this under GIT_CURL_VERBOSE=1 and\n> > I saw two 401 responses in the \"http.emptyauth=true\" case and one\n> > in the false case. I also tried with a repo that I did have access to (first\n> > configuring the necessary tokens for HTTPS push access), and saw two\n> > 401 responses in *both* cases. \n> \n> Thanks; that matches my observation. I do not think we care about\n> an extra roundtrip for the failure case, but as long as we do not\n> increase the number of roundtrip in the normal case, we can declare\n> that this is an improvement. I am not quite sure where that extra\n> 401 comes from in the normal case, and that might be an indication\n> that we already are doing something wrong, though.\n\nThis patch drops the useless probe request:\n\ndiff --git a/http.c b/http.c\nindex 943e630ea..7b4c2db86 100644\n--- a/http.c\n+++ b/http.c\n@@ -1663,6 +1663,9 @@ static int http_request(const char *url,\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,\n \t\t\t\toptions->effective_url);\n \n+\tif (results.auth_avail == CURLAUTH_BASIC)\n+\t\thttp_auth_methods = CURLAUTH_BASIC;\n+\n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n \n\nbut setting http.emptyauth adds back in the useless request. I think\nthat could be fixed by skipping the empty-auth thing when\nhttp_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps\nother methods need it to, so maybe skip it if _just_ BASIC is set).\n\nI suspect the patch above could probably be generalized as:\n\n /* cut out methods we know the server doesn't support */\n http_auth_methods &= results.auth_avail;\n\nand let curl figure it out from there.\n\n-Peff\n"},{"id":"312347","messageId":"20170222213542.opunuepfmj557zyr@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqq4lzlc408.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T21:35:43Z","receivedAt":"2017-02-22T21:36:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I don't think it incurs an extra round-trip now, because of the way\n> > libcurl works. Though I think it _does_ make it harder for curl to later\n> > optimize out that extra round-trip.\n> > ...\n> > In the current trace, you can see that libcurl insists on making a\n> > second auth-less request after we've fed it credentials. I'm not sure\n> > how to get rid of this useless extra round-trip, but it would be nice to\n> > do so (IIRC, it is a probe request to find out the list of auth types\n> > that the server supports, which are not remembered from the previous\n> > request).\n> > ...\n> > With http.emptyauth, the second round-trip _isn't_ useless. It's trying\n> > to send the empty credential.\n> > \n> > So while curl isn't currently optimizing out the second call, I think\n> > http.emptyauth makes it harder to do the right thing.\n> > ...\n> > I think that would keep it to 2 round-trips for the normal \"Basic\" case,\n> > as well as for the GSSNegotiate case. It would be 3 requests when the\n> > server offers GSSNegotiate but you can't use it (but you could set\n> > http.emptyauth=false to optimize that out).\n> \n> Thanks for your thoughts. I'd think that we should take this change\n> and leave the optimization for later, then. It's not like the\n> change of the default is making the normal situation any worse, it\n> seems.\n\nI'm not excited that it will start making known bogus-username requests\nby default to servers which do not even support Negotiate. I guess that\nis really the server-operators problem, but it feels pretty hacky.\n\n-Peff\n"},{"id":"312348","messageId":"xmqqwpchanxz.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170222213542.opunuepfmj557zyr@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-22T21:57:28Z","receivedAt":"2017-02-22T22:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:\n>> \n>> Thanks for your thoughts. I'd think that we should take this change\n>> and leave the optimization for later, then. It's not like the\n>> change of the default is making the normal situation any worse, it\n>> seems.\n>\n> I'm not excited that it will start making known bogus-username requests\n> by default to servers which do not even support Negotiate. I guess that\n> is really the server-operators problem, but it feels pretty hacky.\n\nI guess that's another valid concern. The servers used to be able\nto say \"Ah, this repository needs auth and this request does not, so\nreject it without asking the auth-db\". Now it must say \"Ah, this\nrepository needs auth and this request does have one, but it is\nempty so let's not even bother the auth-db\" in order to reject a\nuseless \"empty-auth\" request with the same efficiency.\n\nAfter the first request without auth (that fails), do we learn\nanything useful from the server side (like \"it knows Negotiate\")\nthat we can use to flip the \"empty-auth\" bit to give a better\ndefault to people from both worlds, I wonder...?\n"},{"id":"312349","messageId":"20170222215833.d7htyo32ptfse5l4@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqqwpchanxz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T21:58:33Z","receivedAt":"2017-02-22T22:28:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:\n> >> \n> >> Thanks for your thoughts. I'd think that we should take this change\n> >> and leave the optimization for later, then. It's not like the\n> >> change of the default is making the normal situation any worse, it\n> >> seems.\n> >\n> > I'm not excited that it will start making known bogus-username requests\n> > by default to servers which do not even support Negotiate. I guess that\n> > is really the server-operators problem, but it feels pretty hacky.\n> \n> I guess that's another valid concern. The servers used to be able\n> to say \"Ah, this repository needs auth and this request does not, so\n> reject it without asking the auth-db\". Now it must say \"Ah, this\n> repository needs auth and this request does have one, but it is\n> empty so let's not even bother the auth-db\" in order to reject a\n> useless \"empty-auth\" request with the same efficiency.\n> \n> After the first request without auth (that fails), do we learn\n> anything useful from the server side (like \"it knows Negotiate\")\n> that we can use to flip the \"empty-auth\" bit to give a better\n> default to people from both worlds, I wonder...?\n\nYes, that's exactly what I was trying to say in my first message.\n\n-Peff\n"},{"id":"312350","messageId":"xmqqshn5am74.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170222215833.d7htyo32ptfse5l4@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-22T22:35:11Z","receivedAt":"2017-02-22T22:38:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:\n>> >> \n>> >> Thanks for your thoughts. I'd think that we should take this change\n>> >> and leave the optimization for later, then. It's not like the\n>> >> change of the default is making the normal situation any worse, it\n>> >> seems.\n>> >\n>> > I'm not excited that it will start making known bogus-username requests\n>> > by default to servers which do not even support Negotiate. I guess that\n>> > is really the server-operators problem, but it feels pretty hacky.\n>> \n>> I guess that's another valid concern. The servers used to be able\n>> to say \"Ah, this repository needs auth and this request does not, so\n>> reject it without asking the auth-db\". Now it must say \"Ah, this\n>> repository needs auth and this request does have one, but it is\n>> empty so let's not even bother the auth-db\" in order to reject a\n>> useless \"empty-auth\" request with the same efficiency.\n>> \n>> After the first request without auth (that fails), do we learn\n>> anything useful from the server side (like \"it knows Negotiate\")\n>> that we can use to flip the \"empty-auth\" bit to give a better\n>> default to people from both worlds, I wonder...?\n>\n> Yes, that's exactly what I was trying to say in my first message.\n\nI see. I am still inclined to take this as-is for now to cook in\n'next', though. \n\nA solution along your line would help Negotiate users OOB experience\nwithout hurting the servers that do not offer Negotiate, but until\nthat materializes, users can set the lazier http.emptyAuth on\n(without selectively setting http..emptyAuth off for sites\nwithout Negotiate) and hurt the servers by throwing an empty auth\nanyway regardless of the default, so the flipping of the default is\nnot fundamentally adding more harm in that sense.\n"},{"id":"312353","messageId":"20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqqshn5am74.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T23:33:33Z","receivedAt":"2017-02-22T23:33:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2017 at 02:35:11PM -0800, Junio C Hamano wrote:\n\n> A solution along your line would help Negotiate users OOB experience\n> without hurting the servers that do not offer Negotiate, but until\n> that materializes, users can set the lazier http.emptyAuth on\n> (without selectively setting http..emptyAuth off for sites\n> without Negotiate) and hurt the servers by throwing an empty auth\n> anyway regardless of the default, so the flipping of the default is\n> not fundamentally adding more harm in that sense.\n\nI was hoping to materialize it today. :)\n\nHere's what I came up with. I have a lot of questions about the second\npatch which I'll outline there. But I think it may be a good start.\n\n [1/2]: http: restrict auth methods to what the server advertises\n [2/2]: http: add an \"auto\" mode for http.emptyauth\n\n http.c | 38 +++++++++++++++++++++++++++++++++++---\n 1 file changed, 35 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"312354","messageId":"20170222233419.q3fxqmrscosumbjm@genre.crustytoothpaste.net","threadId":"45195","inReplyTo":"97ab9a812f7b46d7b10d4d06f73259d8@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-02-22T23:34:19Z","receivedAt":"2017-02-22T23:35:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Feb 22, 2017 at 09:04:14PM +0000, David Turner wrote:\n> > -----Original Message-----\n> > From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C\n> > Hamano\n> > Sent: Wednesday, February 22, 2017 3:20 PM\n> > To: David Turner \n> > Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; Johannes Schindelin\n> > ; Eric Sunshine\n> > ; Jeff King \n> > Subject: Re: [PATCH] http(s): automatically try NTLM authentication first\n> > \n> > \n> > Some other possible worries we may have had I can think of are:\n> > \n> > - With this enabled unconditionally, would we leak some information?\n> \n> I think \"NTLM\" is actually a misnomer here (I just copied Johannes's \n> commit message). The mechanism is actually SPNEGO, if I understand this \n> correctly. It seems to me that this is probably secure, since it is apparently\n> widely implemented in browsers.\n\nThis is SPNEGO. It will work with NTLM as well as Kerberos.\n\nBrowsers usually disable this feature by default, as it basically will\nattempt to authenticate to any site that sends a 401. For Kerberos\nagainst a malicious site, the user will either not have a valid ticket\nfor that domain, or the user's Kerberos server will refuse to provide a\nticket to pass to the server, so there's no security risk involved.\n\nI'm unclear how SPNEGO works with NTLM, so I can't speak for the\nsecurity of it. From what I understand of NTLM and from RFC 4559, it\nconsists of a shared secret. I'm unsure what security measures are in\nplace to not send that to an untrusted server.\n\nAs far as Kerberos, this is a desirable feature to have enabled, with\nlittle downside. I just don't know about the security of the NTLM part,\nand I don't think we should take this patch unless we're sure we know\nthe consequences of it.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"312357","messageId":"20170222234059.iajn2zuwzkzjxit2@sigill.intra.peff.net","threadId":"45195","inReplyTo":"20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net","subject":"[PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T23:40:59Z","receivedAt":"2017-02-22T23:42:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This variable needs to be specified to make some types of\nnon-basic authentication work, but ideally this would just\nwork out of the box for everyone.\n\nHowever, simply setting it to \"1\" by default introduces an\nextra round-trip for cases where it _isn't_ useful. We end\nup sending a bogus empty credential that the server rejects.\n\nInstead, let's introduce an automatic mode, that works like\nthis:\n\n 1. We won't try to send the bogus credential on the first\n request. We'll wait to get an HTTP 401, as usual.\n\n 2. After seeing an HTTP 401, the empty-auth hack will kick\n in only when we know there is an auth method beyond\n \"Basic\" to be tried.\n\nThat should make it work out of the box, without incurring\nany extra round-trips for people hitting Basic-only servers.\n\nThis _does_ incur an extra round-trip if you really want to\nuse \"Basic\" but your server advertises other methods (the\nemptyauth hack will kick in but fail, and then Git will\nactually ask for a password).\n\nThe auto mode may incur an extra round-trip over setting\nhttp.emptyauth=true, because part of the emptyauth hack is\nto feed this blank password to curl even before we've made a\nsingle request.\n\nSigned-off-by: Jeff King \n---\nMy open questions are:\n\n - I don't have anything but a Basic server to test against. So it's\n entirely possible that this doesn't actually work in the NTLM case.\n\n - what does a request log look like for somebody actually using NTLM?\n It's possible if the initial request sends a restrict auth_avail,\n that curl could get away without the extra probe request, and we'd\n end up with the same number of requests for \"auto\" mode versus\n http.emptyauth=true.\n\n - the whole \"don't use this on the initial request\" flag feels really\n hacky. It's a side effect of how emptyauth tries to kick in even\n before we have sent any requests. Probably it should have been\n handled in the 401 code path originally, but I'm hesitant to change\n it now. I suspect it is eliminating a round-trip in practice when it\n is enabled.\n\n - I didn't test a server that advertises Basic and something else, but\n really only takes Basic. So I'm just assuming that it incurs the\n extra round-trip (actually probably two, one for curl's method\n probe).\n\n - When your curl is too old to do CURLAUTH_ANY, I just left the\n default to disable emptyauth. But it could easily be \"1\" if people\n care.\n\n http.c | 38 ++++++++++++++++++++++++++++++++++----\n 1 file changed, 34 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a05609766..ea70ec1ee 100644\n--- a/http.c\n+++ b/http.c\n@@ -109,7 +109,7 @@ static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n-static int curl_empty_auth;\n+static int curl_empty_auth = -1;\n \n enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;\n \n@@ -125,6 +125,7 @@ static struct credential cert_auth = CREDENTIAL_INIT;\n static int ssl_cert_password_required;\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n static unsigned long http_auth_methods = CURLAUTH_ANY;\n+static int http_auth_methods_restricted;\n #endif\n \n static struct curl_slist *pragma_header;\n@@ -382,10 +383,37 @@ static int http_options(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static int curl_empty_auth_enabled(void)\n+{\n+\tif (curl_empty_auth < 0) {\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t\t/*\n+\t\t * In the automatic case, kick in the empty-auth\n+\t\t * hack as long as we would potentially try some\n+\t\t * method more exotic than \"Basic\".\n+\t\t *\n+\t\t * But only do so when this is _not_ our initial\n+\t\t * request, as we would not then yet know what\n+\t\t * methods are available.\n+\t\t */\n+\t\treturn http_auth_methods_restricted &&\n+\t\t http_auth_methods != CURLAUTH_BASIC;\n+#else\n+\t\t/*\n+\t\t * Our libcurl is too old to do AUTH_ANY in the first place;\n+\t\t * just default to turning the feature off.\n+\t\t */\n+\t\treturn 0;\n+#endif\n+\t}\n+\n+\treturn curl_empty_auth;\n+}\n+\n static void init_curl_http_auth(CURL *result)\n {\n \tif (!http_auth.username || !*http_auth.username) {\n-\t\tif (curl_empty_auth)\n+\t\tif (curl_empty_auth_enabled())\n \t\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, \":\");\n \t\treturn;\n \t}\n@@ -1079,7 +1107,7 @@ struct active_request_slot *get_active_slot(void)\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);\n #endif\n-\tif (http_auth.password || curl_empty_auth)\n+\tif (http_auth.password || curl_empty_auth_enabled())\n \t\tinit_curl_http_auth(slot->curl);\n \n \treturn slot;\n@@ -1347,8 +1375,10 @@ static int handle_curl_result(struct slot_results *results)\n \t\t} else {\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n-\t\t\tif (results->auth_avail)\n+\t\t\tif (results->auth_avail) {\n \t\t\t\thttp_auth_methods &= results->auth_avail;\n+\t\t\t\thttp_auth_methods_restricted = 1;\n+\t\t\t}\n #endif\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n-- \n2.12.0.rc2.597.g959f68882\n"},{"id":"312358","messageId":"20170222233436.l5c3ya53cl7y5x7z@sigill.intra.peff.net","threadId":"45195","inReplyTo":"20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net","subject":"[PATCH 1/2] http: restrict auth methods to what the server advertises","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T23:34:37Z","receivedAt":"2017-02-22T23:42:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"By default, we tell curl to use CURLAUTH_ANY, which does not\nlimit its set of auth methods. However, this results in an\nextra round-trip to the server when authentication is\nrequired. After we've fed the credential to curl, it wants\nto probe the server to find its list of available methods\nbefore sending an Authorization header.\n\nWe can shortcut this by limiting our http_auth_methods by\nwhat the server told us it supports. In some cases (such as\nwhen the server only supports Basic), that lets curl skip\nthe extra probe request.\n\nThe end result should look the same to the user, but you can\nuse GIT_TRACE_CURL to verify the sequence of requests:\n\n GIT_TRACE_CURL=1 \\\n git ls-remote https://example.com/repo.git \\\n 2>&1 >/dev/null |\n egrep '(Send|Recv) header: (GET|HTTP|Auth)'\n\nBefore this patch, hitting a Basic-only server like\ngithub.com results in:\n\n Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Recv header: HTTP/1.1 401 Authorization Required\n Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Recv header: HTTP/1.1 401 Authorization Required\n Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Send header: Authorization: Basic \n Recv header: HTTP/1.1 200 OK\n\nAnd after:\n\n Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Recv header: HTTP/1.1 401 Authorization Required\n Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1\n Send header: Authorization: Basic \n Recv header: HTTP/1.1 200 OK\n\nThe possible downsides are:\n\n - This only helps for a Basic-only server; for a server\n with multiple auth options, curl may still send a probe\n request to see which ones are available (IOW, there's no\n way to say \"don't probe, I already know what the server\n will say\").\n\n - The http_auth_methods variable is global, so this will\n apply to all further requests. That's acceptable for\n Git's usage of curl, though, which also treats the\n credentials as global. I.e., in any given program\n invocation we hit only one conceptual server (we may be\n redirected at the outset, but in that case that's whose\n auth_avail field we'd see).\n\nSigned-off-by: Jeff King \n---\n http.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 90a1c0f11..a05609766 100644\n--- a/http.c\n+++ b/http.c\n@@ -1347,6 +1347,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\t} else {\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n+\t\t\tif (results->auth_avail)\n+\t\t\t\thttp_auth_methods &= results->auth_avail;\n #endif\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n-- \n2.12.0.rc2.597.g959f68882\n\n"},{"id":"312360","messageId":"20170222234246.wjp3567vesdusiaf@sigill.intra.peff.net","threadId":"45195","inReplyTo":"20170222233419.q3fxqmrscosumbjm@genre.crustytoothpaste.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-22T23:42:46Z","receivedAt":"2017-02-22T23:42:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2017 at 11:34:19PM +0000, brian m. carlson wrote:\n\n> Browsers usually disable this feature by default, as it basically will\n> attempt to authenticate to any site that sends a 401. For Kerberos\n> against a malicious site, the user will either not have a valid ticket\n> for that domain, or the user's Kerberos server will refuse to provide a\n> ticket to pass to the server, so there's no security risk involved.\n> \n> I'm unclear how SPNEGO works with NTLM, so I can't speak for the\n> security of it. From what I understand of NTLM and from RFC 4559, it\n> consists of a shared secret. I'm unsure what security measures are in\n> place to not send that to an untrusted server.\n> \n> As far as Kerberos, this is a desirable feature to have enabled, with\n> little downside. I just don't know about the security of the NTLM part,\n> and I don't think we should take this patch unless we're sure we know\n> the consequences of it.\n\nHmm. That would be a problem with my proposed patch 2 then, too, if only\nbecause it turns the feature on by default in more places.\n\nIf it _is_ dangerous to turn on all the time, I'd think we should\nconsider warning people in the http.emptyauth documentation.\n\n-Peff\n"},{"id":"312365","messageId":"b152fad7e79046c5aa6cac9e21066c1c@exmbdft7.ad.twosigma.com","threadId":"45195","inReplyTo":"20170222233419.q3fxqmrscosumbjm@genre.crustytoothpaste.net","subject":"RE: [PATCH] http(s): automatically try NTLM authentication first","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-23T01:03:39Z","receivedAt":"2017-02-23T01:05:17Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: brian m. carlson [mailto:sandals@crustytoothpaste.net]\n> \n> This is SPNEGO. It will work with NTLM as well as Kerberos.\n> \n> Browsers usually disable this feature by default, as it basically will attempt to\n> authenticate to any site that sends a 401. For Kerberos against a malicious\n> site, the user will either not have a valid ticket for that domain, or the user's\n> Kerberos server will refuse to provide a ticket to pass to the server, so\n> there's no security risk involved.\n> \n> I'm unclear how SPNEGO works with NTLM, so I can't speak for the security\n> of it. From what I understand of NTLM and from RFC 4559, it consists of a\n> shared secret. I'm unsure what security measures are in place to not send\n> that to an untrusted server.\n> \n> As far as Kerberos, this is a desirable feature to have enabled, with little\n> downside. I just don't know about the security of the NTLM part, and I don't\n> think we should take this patch unless we're sure we know the\n> consequences of it.\n\nNTLM on its own is bad:\n\nhttps://msdn.microsoft.com/en-us/library/windows/desktop/aa378749(v=vs.85).aspx\nsays:\n\n\"\n1. (Interactive authentication only) A user accesses a client computer and \nprovides a domain name, user name, and password. The client computes a \ncryptographic hash of the password and discards the actual password.\n2. The client sends the user name to the server (in plaintext).\n3. The server generates a 16-byte random number, called a challenge or \nnonce, and sends it to the client.\n4. The client encrypts this challenge with the hash of the user's password \nand returns the result to the server. This is called the response.\n...\"\n\nWait, what? If I'm a malicious server, I can get access to an offline oracle\nfor whether I've correctly guessed the user's password? That doesn't \nsound secure at all! Skimming the SPNEGO RFCs, there appears to be no\nmitigation for this. \n\nSo, I guess, this patch might be considered a security risk. But on the \nother hand, even *without* this patch, and without http.allowempty at \nall, I think a config which simply uses a https:// url without the magic :@\nwould try SPNEGO. As I understand it, the http.allowempty config just \nmakes the traditional :@ urls work. \n\nActually, though, I am not sure this is as bad as it seems, because gssapi\nmight protect us. When I locally tried a fake server, git (libcurl) refused to \nsend my Kerberos credentials because \"Server not found in Kerberos \ndatabase\". I don't have a machine set up with NTLM authentication \n(because, apparently, that would be insane), so I don't know how to \nconfirm that gssapi would operate off of a whitelist for NTLM as well. \n"},{"id":"312366","messageId":"b5778a7988ad4dfa9adfc8d312432189@exmbdft7.ad.twosigma.com","threadId":"45195","inReplyTo":"20170222234059.iajn2zuwzkzjxit2@sigill.intra.peff.net","subject":"RE: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-23T01:16:33Z","receivedAt":"2017-02-23T01:26:36Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"I don't know enough about how libcurl handles authentication to know whether \nthese patches are a good idea, but I have a minor comment anyway.\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> +static int curl_empty_auth_enabled(void) {\n> +\tif (curl_empty_auth < 0) {\n> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\t\t/*\n> +\t\t * In the automatic case, kick in the empty-auth\n> +\t\t * hack as long as we would potentially try some\n> +\t\t * method more exotic than \"Basic\".\n> +\t\t *\n> +\t\t * But only do so when this is _not_ our initial\n> +\t\t * request, as we would not then yet know what\n> +\t\t * methods are available.\n> +\t\t */\n\nEliminate double-negative:\n\n\"But only do this when this is our second or subsequent request, \nas by then we know what methods are available.\"\n\n"},{"id":"312368","messageId":"20170223013746.lturqad7lnehedb4@sigill.intra.peff.net","threadId":"45195","inReplyTo":"b5778a7988ad4dfa9adfc8d312432189@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-23T01:37:46Z","receivedAt":"2017-02-23T01:38:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:\n\n> I don't know enough about how libcurl handles authentication to know whether \n> these patches are a good idea, but I have a minor comment anyway.\n\nAs somebody who is using non-Basic auth, can you apply these patches and\nshow us the output of:\n\n GIT_TRACE_CURL=1 \\\n git ls-remote https://your-server 2>&1 >/dev/null |\n egrep '(Send|Recv) header: (GET|HTTP|Auth)'\n\n(without http.emptyauth turned on, obviously).\n\n> > +\t\t * But only do so when this is _not_ our initial\n> > +\t\t * request, as we would not then yet know what\n> > +\t\t * methods are available.\n> > +\t\t */\n> \n> Eliminate double-negative:\n> \n> \"But only do this when this is our second or subsequent request, \n> as by then we know what methods are available.\"\n\nYeah, that is clearer.\n\n-Peff\n"},{"id":"312369","messageId":"xmqq60k1abzi.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170222234246.wjp3567vesdusiaf@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T02:15:45Z","receivedAt":"2017-02-23T02:15:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Feb 22, 2017 at 11:34:19PM +0000, brian m. carlson wrote:\n>\n>> Browsers usually disable this feature by default, as it basically will\n>> attempt to authenticate to any site that sends a 401. For Kerberos\n>> against a malicious site, the user will either not have a valid ticket\n>> for that domain, or the user's Kerberos server will refuse to provide a\n>> ticket to pass to the server, so there's no security risk involved.\n>> \n>> I'm unclear how SPNEGO works with NTLM, so I can't speak for the\n>> security of it. From what I understand of NTLM and from RFC 4559, it\n>> consists of a shared secret. I'm unsure what security measures are in\n>> place to not send that to an untrusted server.\n>> \n>> As far as Kerberos, this is a desirable feature to have enabled, with\n>> little downside. I just don't know about the security of the NTLM part,\n>> and I don't think we should take this patch unless we're sure we know\n>> the consequences of it.\n>\n> Hmm. That would be a problem with my proposed patch 2 then, too, if only\n> because it turns the feature on by default in more places.\n>\n> If it _is_ dangerous to turn on all the time, I'd think we should\n> consider warning people in the http.emptyauth documentation.\n\nYeah, http..emptyAuth that knows where it is going may be a lot\nsafer but a blanket http.emptyAuth does sound bad.\n"},{"id":"312370","messageId":"20170223041919.xwdux5rxpojvms7k@genre.crustytoothpaste.net","threadId":"45195","inReplyTo":"b152fad7e79046c5aa6cac9e21066c1c@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-02-23T04:19:19Z","receivedAt":"2017-02-23T04:20:26Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Feb 23, 2017 at 01:03:39AM +0000, David Turner wrote:\n> So, I guess, this patch might be considered a security risk. But on the \n> other hand, even *without* this patch, and without http.allowempty at \n> all, I think a config which simply uses a https:// url without the magic :@\n> would try SPNEGO. As I understand it, the http.allowempty config just \n> makes the traditional :@ urls work. \n\nNo, it's a bit different. libcurl won't try to authenticate to a server\nunless it has a username (and possibly password). With the curl command\nline client, you use a dummy value or -u: to force it to do auth anyway\n(because you want, say, GSSAPI). http.emptyAuth just sets that option\nto “:” so libcurl will auth:\n\n\t\tif (curl_empty_auth)\n\t\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, \":\");\n\nI just use a dummy username for my URLs, but you can write :@ or any\nother permutation to get it to work without emptyAuth. As a\nconsequence, you have to opt-in to that on a per-URL (or per-domain)\nbasis, which is a bit more secure.\n\n> Actually, though, I am not sure this is as bad as it seems, because gssapi\n> might protect us. When I locally tried a fake server, git (libcurl) refused to \n> send my Kerberos credentials because \"Server not found in Kerberos \n> database\". I don't have a machine set up with NTLM authentication \n> (because, apparently, that would be insane), so I don't know how to \n> confirm that gssapi would operate off of a whitelist for NTLM as well. \n\nYup. That's pretty much what I thought would happen, since the Kerberos\nserver has no HTTP/malicious.evil.tld@YOURREALM.TLD service ticket.\nAgain, I don't know how NTLM does things, or if it's wrapped in a\nsuitable ticket format somehow.\n\nLast I base64-decoded an NTLM SPNEGO response, it did not contain the\nOID required by GSSAPI as a prefix; it instead contained an “NTLMSSP”\nheader, which isn't a valid OID. I didn't delve much further, since I\nwas pretty sure I didn't want to know more.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"312386","messageId":"d5375d24-cfdd-2348-84ae-b878c4aaa369@gmail.com","threadId":"45195","inReplyTo":"b152fad7e79046c5aa6cac9e21066c1c@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Mantas Mikulėnas","fromEmail":"grawity@gmail.com","sentAt":"2017-02-23T09:13:12Z","receivedAt":"2017-02-23T09:13:28Z","isPatch":true,"sender":{"key":"grawity@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31021?v=4"},"body":"On 2017-02-23 03:03, David Turner wrote:\n> Actually, though, I am not sure this is as bad as it seems, because gssapi\n> might protect us. When I locally tried a fake server, git (libcurl) refused to \n> send my Kerberos credentials because \"Server not found in Kerberos \n> database\". I don't have a machine set up with NTLM authentication \n> (because, apparently, that would be insane), so I don't know how to \n> confirm that gssapi would operate off of a whitelist for NTLM as well. \n\nNTLM and Kerberos work very differently in that regard.\n\nKerberos is ticket-based so the client *first* has to obtain a ticket\nfrom the domain's KDC, so a malicious server at minimum needs to know\nwhat principal name to provide (i.e. which real server to try\nimpersonating). And even if it does that, the ticket doesn't contain\ncrackable hashes, just data encrypted with a key known only to the KDC\nand the real server. So the whitelist is only for privacy and/or\nperformance reasons, I guess?\n\nNTLM is challenge/response without any third party, and yes, it requires\nthe application to implement its own whitelisting to avoid the security\nproblems.\n\n-- \nMantas Mikulėnas \n"},{"id":"312394","messageId":"092a87cf9aa94d53aebf42facb75b985@exmbdft7.ad.twosigma.com","threadId":"45195","inReplyTo":"20170223013746.lturqad7lnehedb4@sigill.intra.peff.net","subject":"RE: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-23T16:31:13Z","receivedAt":"2017-02-23T16:41:39Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Wednesday, February 22, 2017 8:38 PM\n> To: David Turner \n> Cc: Junio C Hamano ; git@vger.kernel.org;\n> sandals@crustytoothpaste.net; Johannes Schindelin\n> ; Eric Sunshine \n> Subject: Re: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth\n> \n> On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:\n> \n> > I don't know enough about how libcurl handles authentication to know\n> > whether these patches are a good idea, but I have a minor comment\n> anyway.\n> \n> As somebody who is using non-Basic auth, can you apply these patches and\n> show us the output of:\n> \n> GIT_TRACE_CURL=1 \\\n> git ls-remote https://your-server 2>&1 >/dev/null |\n> egrep '(Send|Recv) header: (GET|HTTP|Auth)'\n> \n> (without http.emptyauth turned on, obviously).\n\nThe results appear to be identical with and without\nthe patch. With http.emptyauth turned off,\n16:27:28.208924 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n16:27:28.212872 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required\nUsername for 'http://git': [I just pressed enter]\nPassword for 'http://git': [ditto]\n16:27:29.928872 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n16:27:29.929787 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required\n\n(if someone else wants to replicate this, delete >/dev/null bit \nfrom Jeff's shell snippet)\n\n\n"},{"id":"312397","messageId":"alpine.DEB.2.20.1702231806340.3767@virtualbox","threadId":"45195","inReplyTo":"20170222213410.iak43asq775tzr42@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-23T17:08:49Z","receivedAt":"2017-02-23T17:10:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 22 Feb 2017, Jeff King wrote:\n\n> On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:\n> \n> > David Turner writes:\n> > \n> > > Always, no. For failed authentication (or authorization),\n> > > apparently, yes. I tested this by setting the variable to false\n> > > and then true, and trying to Push to a github repository which I\n> > > didn't have write access to, with both an empty username\n> > > (https://@:github.com/...) and no username (http://github.com/...).\n> > > I ran this under GIT_CURL_VERBOSE=1 and I saw two 401 responses in\n> > > the \"http.emptyauth=true\" case and one in the false case. I also\n> > > tried with a repo that I did have access to (first configuring the\n> > > necessary tokens for HTTPS push access), and saw two 401 responses\n> > > in *both* cases. \n> > \n> > Thanks; that matches my observation. I do not think we care about\n> > an extra roundtrip for the failure case, but as long as we do not\n> > increase the number of roundtrip in the normal case, we can declare\n> > that this is an improvement. I am not quite sure where that extra\n> > 401 comes from in the normal case, and that might be an indication\n> > that we already are doing something wrong, though.\n> \n> This patch drops the useless probe request:\n> \n> diff --git a/http.c b/http.c\n> index 943e630ea..7b4c2db86 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1663,6 +1663,9 @@ static int http_request(const char *url,\n> \t\tcurlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,\n> \t\t\t\toptions->effective_url);\n> \n> +\tif (results.auth_avail == CURLAUTH_BASIC)\n> +\t\thttp_auth_methods = CURLAUTH_BASIC;\n> +\n> \tcurl_slist_free_all(headers);\n> \tstrbuf_release(&buf);\n> \n> \n> but setting http.emptyauth adds back in the useless request. I think\n> that could be fixed by skipping the empty-auth thing when\n> http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps\n> other methods need it to, so maybe skip it if _just_ BASIC is set).\n> \n> I suspect the patch above could probably be generalized as:\n> \n> /* cut out methods we know the server doesn't support */\n> http_auth_methods &= results.auth_avail;\n> \n> and let curl figure it out from there.\n\nMaybe this patch (or a variation thereof) would also be able to fix this\nproblem with the patch:\n\n\thttps://github.com/git-for-windows/git/issues/1034\n\nShort version: for certain servers (that do *not* advertise Negotiate),\nsetting emptyauth to true will result in a failed fetch, without letting\nthe user type in their credentials.\n\nCiao,\nJohannes\n"},{"id":"312415","messageId":"xmqqbmts9177.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"alpine.DEB.2.20.1702231806340.3767@virtualbox","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T19:06:20Z","receivedAt":"2017-02-23T19:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Wed, 22 Feb 2017, Jeff King wrote:\n>> This patch drops the useless probe request:\n> ...\n>> but setting http.emptyauth adds back in the useless request. I think\n>> that could be fixed by skipping the empty-auth thing when\n>> http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps\n>> other methods need it to, so maybe skip it if _just_ BASIC is set).\n>> \n>> I suspect the patch above could probably be generalized as:\n>> \n>> /* cut out methods we know the server doesn't support */\n>> http_auth_methods &= results.auth_avail;\n>> \n>> and let curl figure it out from there.\n>\n> Maybe this patch (or a variation thereof) would also be able to fix this\n> problem with the patch:\n>\n> \thttps://github.com/git-for-windows/git/issues/1034\n>\n> Short version: for certain servers (that do *not* advertise Negotiate),\n> setting emptyauth to true will result in a failed fetch, without letting\n> the user type in their credentials.\n\nThe issue described in that page looks rather serious.\n\nI believe that a \"variation\" has become the first part of a\ntwo-patch series that appear in the downthread from here. Perhaps\nyou can ask them to test it out (or even better if you have a setup\nyou can easily test against yourself)?\n"},{"id":"312418","messageId":"xmqq1suo90za.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170222234246.wjp3567vesdusiaf@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T19:11:05Z","receivedAt":"2017-02-23T19:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Feb 22, 2017 at 11:34:19PM +0000, brian m. carlson wrote:\n>\n>> Browsers usually disable this feature by default, as it basically will\n>> attempt to authenticate to any site that sends a 401. For Kerberos\n>> against a malicious site, the user will either not have a valid ticket\n>> for that domain, or the user's Kerberos server will refuse to provide a\n>> ticket to pass to the server, so there's no security risk involved.\n>> \n>> I'm unclear how SPNEGO works with NTLM, so I can't speak for the\n>> security of it. From what I understand of NTLM and from RFC 4559, it\n>> consists of a shared secret. I'm unsure what security measures are in\n>> place to not send that to an untrusted server.\n>> \n>> As far as Kerberos, this is a desirable feature to have enabled, with\n>> little downside. I just don't know about the security of the NTLM part,\n>> and I don't think we should take this patch unless we're sure we know\n>> the consequences of it.\n>\n> Hmm. That would be a problem with my proposed patch 2 then, too, if only\n> because it turns the feature on by default in more places.\n>\n> If it _is_ dangerous to turn on all the time, I'd think we should\n> consider warning people in the http.emptyauth documentation.\n\nI presume that we have finished discussing the security\nramification, and if I am not mistaken the conclusion was that it\ncould leak information if we turned on emptyAuth unconditionally\nwhen talking to a wrong server, and that the documentation needs an\nupdate to recommend those who use emptyAuth because they want to\ntalk to Negotiate servers to use the http..emptyAuth form,\nlimited to such servers, not a more generic http.emptyAuth, to avoid\ninformation leakage?\n\nIf that is the case, let's take your 1/2 in the near-by thread\nwithout 2/2 (auto-enable emptyAuth) for now, as Dscho seems to have\na case that may be helped by it.\n"},{"id":"312423","messageId":"20170223193524.e7jsik4nwchjaeoe@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqq1suo90za.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-23T19:35:24Z","receivedAt":"2017-02-23T19:42:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 11:11:05AM -0800, Junio C Hamano wrote:\n\n> >> As far as Kerberos, this is a desirable feature to have enabled, with\n> >> little downside. I just don't know about the security of the NTLM part,\n> >> and I don't think we should take this patch unless we're sure we know\n> >> the consequences of it.\n> >\n> > Hmm. That would be a problem with my proposed patch 2 then, too, if only\n> > because it turns the feature on by default in more places.\n> >\n> > If it _is_ dangerous to turn on all the time, I'd think we should\n> > consider warning people in the http.emptyauth documentation.\n> \n> I presume that we have finished discussing the security\n> ramification, and if I am not mistaken the conclusion was that it\n> could leak information if we turned on emptyAuth unconditionally\n> when talking to a wrong server, and that the documentation needs an\n> update to recommend those who use emptyAuth because they want to\n> talk to Negotiate servers to use the http..emptyAuth form,\n> limited to such servers, not a more generic http.emptyAuth, to avoid\n> information leakage?\n\nI don't know enough to evaluate the claims of emptyAuth being dangerous\nor not (nor do I use it myself or admin a server whose users need it).\nSo I will let interested parties hash out whether it is a good idea or\nnot, and I'm happy to drop my 2/2 for now.\n\nIf we are to make it more widely available, I would prefer something\nmore like my 2/2 than always turning on http.emptyAuth, if only because\nit reduces the cost to people not using the feature. I'm happy to work\nmore on the patch if we decide to go that route.\n\n> If that is the case, let's take your 1/2 in the near-by thread\n> without 2/2 (auto-enable emptyAuth) for now, as Dscho seems to have\n> a case that may be helped by it.\n\nYes, I think 1/2 stands on its own. Whether it helps Dscho's case or\nnot, it eliminates an HTTP round-trip for Basic-only servers, which I\nthink is worth it.\n\n-Peff\n"},{"id":"312424","messageId":"20170223194237.eckkpiqv7inuz7un@sigill.intra.peff.net","threadId":"45195","inReplyTo":"alpine.DEB.2.20.1702231806340.3767@virtualbox","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-23T19:42:37Z","receivedAt":"2017-02-23T19:42:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 06:08:49PM +0100, Johannes Schindelin wrote:\n\n> > I suspect the patch above could probably be generalized as:\n> > \n> > /* cut out methods we know the server doesn't support */\n> > http_auth_methods &= results.auth_avail;\n> > \n> > and let curl figure it out from there.\n> \n> Maybe this patch (or a variation thereof) would also be able to fix this\n> problem with the patch:\n> \n> \thttps://github.com/git-for-windows/git/issues/1034\n> \n> Short version: for certain servers (that do *not* advertise Negotiate),\n> setting emptyauth to true will result in a failed fetch, without letting\n> the user type in their credentials.\n\nI suspect it isn't enough to help without 2/2. This will tell curl that\nthe server does not do Negotiate, so it will skip the probe request. But\nGit will still feed curl the bogus empty credential.\n\nThat's what 2/2 tries to fix: only kick in the emptyAuth hack when there\nis something besides Basic[1] to try. The way it is written adds an\nextra \"auto\" mode to emptyAuth, as I wanted to leave \"emptyauth=true\" as\na workaround in case the \"auto\" behavior does not work. And then I\nturned on \"auto\" by default, since that was what the discussion was\nshooting for.\n\nBut if we are worried about turning on emptyAuth everywhere, the auto\nbehavior could be tied to emptyauth=true (and have something like\n\"emptyauth=always\" to _really_ force it). I don't have an opinion there.\nIt sounds like emptyauth has been enabled by default on Windows for a\nwhile. It's not clear to me if that's a security problem or not.\n\n-Peff\n"},{"id":"312425","messageId":"20170223194418.eqi5ynhyhrcybiok@sigill.intra.peff.net","threadId":"45195","inReplyTo":"092a87cf9aa94d53aebf42facb75b985@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-23T19:44:19Z","receivedAt":"2017-02-23T19:44:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 04:31:13PM +0000, David Turner wrote:\n\n> > As somebody who is using non-Basic auth, can you apply these patches and\n> > show us the output of:\n> > \n> > GIT_TRACE_CURL=1 \\\n> > git ls-remote https://your-server 2>&1 >/dev/null |\n> > egrep '(Send|Recv) header: (GET|HTTP|Auth)'\n> > \n> > (without http.emptyauth turned on, obviously).\n> \n> The results appear to be identical with and without\n> the patch. With http.emptyauth turned off,\n> 16:27:28.208924 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n> 16:27:28.212872 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required\n> Username for 'http://git': [I just pressed enter]\n> Password for 'http://git': [ditto]\n> 16:27:29.928872 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n> 16:27:29.929787 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required\n\nJust to be sure: did you remove http.emptyauth config completely from\nyour config files, or did you turn it to \"false\"? Because the new\nbehavior only kicks in when it isn't configured at all (probably we\nshould respect \"auto\" as a user-provided name).\n\n> (if someone else wants to replicate this, delete >/dev/null bit \n> from Jeff's shell snippet)\n\nHrm, you shouldn't need to. The stderr redirection comes first, so it\nshould become the new stdout.\n\n-Peff\n"},{"id":"312430","messageId":"363ee9e9f043443e8ad096e2c2d8bd77@exmbdft7.ad.twosigma.com","threadId":"45195","inReplyTo":"20170223194418.eqi5ynhyhrcybiok@sigill.intra.peff.net","subject":"RE: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-02-23T20:05:08Z","receivedAt":"2017-02-23T20:05:17Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Thursday, February 23, 2017 2:44 PM\n> To: David Turner \n> Cc: Junio C Hamano ; git@vger.kernel.org;\n> sandals@crustytoothpaste.net; Johannes Schindelin\n> ; Eric Sunshine \n> Subject: Re: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth\n> \n> On Thu, Feb 23, 2017 at 04:31:13PM +0000, David Turner wrote:\n> \n> > > As somebody who is using non-Basic auth, can you apply these patches\n> > > and show us the output of:\n> > >\n> > > GIT_TRACE_CURL=1 \\\n> > > git ls-remote https://your-server 2>&1 >/dev/null |\n> > > egrep '(Send|Recv) header: (GET|HTTP|Auth)'\n> > >\n> > > (without http.emptyauth turned on, obviously).\n> >\n> > The results appear to be identical with and without the patch. With\n> > http.emptyauth turned off,\n> > 16:27:28.208924 http.c:524 => Send header: GET\n> /info/refs?service=git-upload-pack HTTP/1.1\n> > 16:27:28.212872 http.c:524 <= Recv header: HTTP/1.1 401\n> Authorization Required\n> > Username for 'http://git': [I just pressed enter] Password for\n> > 'http://git': [ditto]\n> > 16:27:29.928872 http.c:524 => Send header: GET\n> /info/refs?service=git-upload-pack HTTP/1.1\n> > 16:27:29.929787 http.c:524 <= Recv header: HTTP/1.1 401\n> Authorization Required\n> \n> Just to be sure: did you remove http.emptyauth config completely from your\n> config files, or did you turn it to \"false\"? Because the new behavior only kicks\n> in when it isn't configured at all (probably we should respect \"auto\" as a user-\n> provided name).\n\nI turned it to false. With it completely removed, I get this, both times:\n\n20:03:49.896797 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n20:03:49.900776 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required\n20:03:49.900929 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n20:03:49.904754 http.c:524 <= Recv header: HTTP/1.1 401 Authorization Required\n20:03:49.906649 http.c:524 => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1\n20:03:49.906654 http.c:524 => Send header: Authorization: Negotiate \n20:03:49.956753 http.c:524 <= Recv header: HTTP/1.1 200 OK - $gitservername\n\n> > (if someone else wants to replicate this, delete >/dev/null bit from\n> > Jeff's shell snippet)\n> \n> Hrm, you shouldn't need to. The stderr redirection comes first, so it should\n> become the new stdout.\n\nWeird. It didn't appear work earlier, but I must have screwed something up.\nAnd I learned something about shell redirection.\n"},{"id":"312433","messageId":"xmqqlgsw7iey.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170223194237.eckkpiqv7inuz7un@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-23T20:37:25Z","receivedAt":"2017-02-23T20:37:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I suspect it isn't enough to help without 2/2. This will tell curl that\n> the server does not do Negotiate, so it will skip the probe request. But\n> Git will still feed curl the bogus empty credential.\n>\n> That's what 2/2 tries to fix: only kick in the emptyAuth hack when there\n> is something besides Basic[1] to try. The way it is written adds an\n\nIn your [1] you wanted to mention that Digest would have the same\nproperty as Basic, or something like that?\n\n> extra \"auto\" mode to emptyAuth, as I wanted to leave \"emptyauth=true\" as\n> a workaround in case the \"auto\" behavior does not work. And then I\n> turned on \"auto\" by default, since that was what the discussion was\n> shooting for.\n>\n> But if we are worried about turning on emptyAuth everywhere, the auto\n> behavior could be tied to emptyauth=true (and have something like\n> \"emptyauth=always\" to _really_ force it). I don't have an opinion there.\n\nI do not have a strong opinion, either, but it sounds like that even\nthe \"disable emptyAuth hack if the server is Basic only\" variant\nwould be much better than setting emptyAuth on by default. At least\nthe user whose issue was reported in Dscho's message would be fixed\nby such a variant, I would think (i.e. talking to a server with no\nNegotiate and emptyAuth set to true results in no attempt to give\nthe user a chance to tell who s/he is --- your 2/2 will turn\nemptyAuth off in that case).\n\n\n"},{"id":"312435","messageId":"20170223204804.syj6tgjdrgmqdzna@sigill.intra.peff.net","threadId":"45195","inReplyTo":"xmqqlgsw7iey.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-23T20:48:04Z","receivedAt":"2017-02-23T20:48:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2017 at 12:37:25PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I suspect it isn't enough to help without 2/2. This will tell curl that\n> > the server does not do Negotiate, so it will skip the probe request. But\n> > Git will still feed curl the bogus empty credential.\n> >\n> > That's what 2/2 tries to fix: only kick in the emptyAuth hack when there\n> > is something besides Basic[1] to try. The way it is written adds an\n> \n> In your [1] you wanted to mention that Digest would have the same\n> property as Basic, or something like that?\n\nOops, yeah. What I was going to say is that we may want a list of auth\ntypes where we _do_ want the hack on, rather than ones where we know it\ndoes not work. People are more likely to notice when the list is wrong,\nthen.\n\n> > But if we are worried about turning on emptyAuth everywhere, the auto\n> > behavior could be tied to emptyauth=true (and have something like\n> > \"emptyauth=always\" to _really_ force it). I don't have an opinion there.\n> \n> I do not have a strong opinion, either, but it sounds like that even\n> the \"disable emptyAuth hack if the server is Basic only\" variant\n> would be much better than setting emptyAuth on by default. At least\n> the user whose issue was reported in Dscho's message would be fixed\n> by such a variant, I would think (i.e. talking to a server with no\n> Negotiate and emptyAuth set to true results in no attempt to give\n> the user a chance to tell who s/he is --- your 2/2 will turn\n> emptyAuth off in that case).\n\nYes, I agree that the \"auto\" behavior is better than defaulting to\n\"true\". I am speaking from the perspective of git.git, which is\ncurrently defaulting to \"false\". It is not clear to me if \"auto\" is\nbetter than \"false\" because of the security implications.\n\nFor Git for Windows, it seems like the auto behavior would be a strict\nimprovement over the \"true\" default they've been shipping.\n\n-Peff\n"},{"id":"312632","messageId":"alpine.DEB.2.20.1702251243390.3767@virtualbox","threadId":"45195","inReplyTo":"20170223013746.lturqad7lnehedb4@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-25T11:48:54Z","receivedAt":"2017-02-25T11:51:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 22 Feb 2017, Jeff King wrote:\n\n> [two beautiful patches]\n\nI applied them and verified that the reported issue is fixed. Thank you!\n\nHopefully you do not mind that I cherry-picked them in preparation for\nGit for Windows v2.12.0?\n\nI added a small fixup (https://github.com/dscho/git/commit/44ae0bcae5):\n\n-- snip --\nSubject: [PATCH] fixup! http: add an \"auto\" mode for http.emptyauth\n\nNote: we keep a \"black list\" of authentication methods for which we do\nnot want to enable http.emptyAuth automatically. A white list would be\nnicer, but less robust, as we want to support linking to several cURL\nversions and the list of authentication methods (as well as their names)\nchanged over time.\n\n[jes: actually added the \"auto\" handling, excluded Digest, too]\n\nThis fixes https://github.com/git-for-windows/git/issues/1034\n\nSigned-off-by: Johannes Schindelin \n---\n http.c | 55 +++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 33 insertions(+), 22 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex f8eb0f23d6c..fb94c444c80 100644\n--- a/http.c\n+++ b/http.c\n@@ -334,7 +334,10 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&user_agent, var, value);\n \n \tif (!strcmp(\"http.emptyauth\", var)) {\n-\t\tcurl_empty_auth = git_config_bool(var, value);\n+\t\tif (value && !strcmp(\"auto\", value))\n+\t\t\tcurl_empty_auth = -1;\n+\t\telse\n+\t\t\tcurl_empty_auth = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -385,29 +388,37 @@ static int http_options(const char *var, const char *value, void *cb)\n \n static int curl_empty_auth_enabled(void)\n {\n-\tif (curl_empty_auth < 0) {\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\t\t/*\n-\t\t * In the automatic case, kick in the empty-auth\n-\t\t * hack as long as we would potentially try some\n-\t\t * method more exotic than \"Basic\".\n-\t\t *\n-\t\t * But only do so when this is _not_ our initial\n-\t\t * request, as we would not then yet know what\n-\t\t * methods are available.\n-\t\t */\n-\t\treturn http_auth_methods_restricted &&\n-\t\t http_auth_methods != CURLAUTH_BASIC;\n+\tif (curl_empty_auth >= 0)\n+\t\treturn curl_empty_auth;\n+\n+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t/*\n+\t * Our libcurl is too old to do AUTH_ANY in the first place;\n+\t * just default to turning the feature off.\n+\t */\n #else\n-\t\t/*\n-\t\t * Our libcurl is too old to do AUTH_ANY in the first place;\n-\t\t * just default to turning the feature off.\n-\t\t */\n-\t\treturn 0;\n+\t/*\n+\t * In the automatic case, kick in the empty-auth\n+\t * hack as long as we would potentially try some\n+\t * method more exotic than \"Basic\".\n+\t *\n+\t * But only do this when this is our second or\n+\t * subsequent * request, as by then we know what\n+\t * methods are available.\n+\t */\n+\tif (http_auth_methods_restricted)\n+\t\tswitch (http_auth_methods) {\n+\t\tcase CURLAUTH_BASIC:\n+\t\tcase CURLAUTH_DIGEST:\n+#ifdef CURLAUTH_DIGEST_IE\n+\t\tcase CURLAUTH_DIGEST_IE:\n #endif\n-\t}\n-\n-\treturn curl_empty_auth;\n+\t\t\treturn 0;\n+\t\tdefault:\n+\t\t\treturn 1;\n+\t\t}\n+#endif\n+\treturn 0;\n }\n \n static void init_curl_http_auth(CURL *result)\n-- snap --\n\nAs you can see, I actually implemented the handling for\nhttp.emptyauth=auto, and I was more comfortable with handling the \"easy\"\ncases first in the curl_empty_auth_enabled function.\n\nI also took Dave's suggestion:\n\n> On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:\n> \n> > > +\t\t * But only do so when this is _not_ our initial\n> > > +\t\t * request, as we would not then yet know what\n> > > +\t\t * methods are available.\n> > > +\t\t */\n> > \n> > Eliminate double-negative:\n> > \n> > \"But only do this when this is our second or subsequent request, \n> > as by then we know what methods are available.\"\n> \n> Yeah, that is clearer.\n\nThank you all!\n\nNow, how to get this into upstream Git, too? Jeff, do you want to submit a\nv2? In that case, would you please consider the fixup! I mentioned above?\nOtherwise I'd be happy to take it from here.\n\nCiao,\nDscho\n"},{"id":"312633","messageId":"alpine.DEB.2.20.1702251250320.3767@virtualbox","threadId":"45195","inReplyTo":"20170223204804.syj6tgjdrgmqdzna@sigill.intra.peff.net","subject":"Re: [PATCH] http(s): automatically try NTLM authentication first","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-25T11:51:17Z","receivedAt":"2017-02-25T11:53:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 23 Feb 2017, Jeff King wrote:\n\n> For Git for Windows, [PATCH 2/2] seems like the auto behavior would be a\n> strict improvement over the \"true\" default they've been shipping.\n\nAbsolutely. Thank you for your tremendous help!\n\nCiao,\nDscho\n"},{"id":"312645","messageId":"20170225191831.dkjasyv3tmkwutre@sigill.intra.peff.net","threadId":"45195","inReplyTo":"20170225191506.4it7pdsi6ijanfft@sigill.intra.peff.net","subject":"[PATCH] http: add an \"auto\" mode for http.emptyauth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-25T19:18:31Z","receivedAt":"2017-02-25T19:19:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This variable needs to be specified to make some types of\nnon-basic authentication work, but ideally this would just\nwork out of the box for everyone.\n\nHowever, simply setting it to \"1\" by default introduces an\nextra round-trip for cases where it _isn't_ useful. We end\nup sending a bogus empty credential that the server rejects.\n\nInstead, let's introduce an automatic mode, that works like\nthis:\n\n 1. We won't try to send the bogus credential on the first\n request. We'll wait to get an HTTP 401, as usual.\n\n 2. After seeing an HTTP 401, the empty-auth hack will kick\n in only when we know there is an auth method available\n that might make use of it (i.e., something besides\n \"Basic\" or \"Digest\").\n\nThat should make it work out of the box, without incurring\nany extra round-trips for people hitting Basic-only servers.\n\nThis _does_ incur an extra round-trip if you really want to\nuse \"Basic\" but your server advertises other methods (the\nemptyauth hack will kick in but fail, and then Git will\nactually ask for a password).\n\nThe auto mode may incur an extra round-trip over setting\nhttp.emptyauth=true, because part of the emptyauth hack is\nto feed this blank password to curl even before we've made a\nsingle request.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\nAnd here's the full patch. It is meant to go on top of the\nalready-queued 1/2, though I suspect it could apply separately.\n\nTest reports welcome from people who actually have NTLM or Kerberos\nservers. The changes from the previous are fairly minimal, but this kind\nof bit-mangling is exactly the kind of thing where I tend to\naccidentally invert the logic. ;)\n\n http.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a05609766..dd637d031 100644\n--- a/http.c\n+++ b/http.c\n@@ -109,7 +109,7 @@ static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n-static int curl_empty_auth;\n+static int curl_empty_auth = -1;\n \n enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;\n \n@@ -125,6 +125,14 @@ static struct credential cert_auth = CREDENTIAL_INIT;\n static int ssl_cert_password_required;\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n static unsigned long http_auth_methods = CURLAUTH_ANY;\n+static int http_auth_methods_restricted;\n+/* Modes for which empty_auth cannot actually help us. */\n+static unsigned long empty_auth_useless =\n+\tCURLAUTH_BASIC\n+#ifdef CURLAUTH_DIGEST_IE\n+\t| CURLAUTH_DIGEST_IE\n+#endif\n+\t| CURLAUTH_DIGEST;\n #endif\n \n static struct curl_slist *pragma_header;\n@@ -333,7 +341,10 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&user_agent, var, value);\n \n \tif (!strcmp(\"http.emptyauth\", var)) {\n-\t\tcurl_empty_auth = git_config_bool(var, value);\n+\t\tif (value && !strcmp(\"auto\", value))\n+\t\t\tcurl_empty_auth = -1;\n+\t\telse\n+\t\t\tcurl_empty_auth = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -382,10 +393,37 @@ static int http_options(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static int curl_empty_auth_enabled(void)\n+{\n+\tif (curl_empty_auth >= 0)\n+\t\treturn curl_empty_auth;\n+\n+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t/*\n+\t * Our libcurl is too old to do AUTH_ANY in the first place;\n+\t * just default to turning the feature off.\n+\t */\n+#else\n+\t/*\n+\t * In the automatic case, kick in the empty-auth\n+\t * hack as long as we would potentially try some\n+\t * method more exotic than \"Basic\" or \"Digest\".\n+\t *\n+\t * But only do this when this is our second or\n+\t * subsequent * request, as by then we know what\n+\t * methods are available.\n+\t */\n+\tif (http_auth_methods_restricted &&\n+\t (http_auth_methods & ~empty_auth_useless))\n+\t\treturn 1;\n+#endif\n+\treturn 0;\n+}\n+\n static void init_curl_http_auth(CURL *result)\n {\n \tif (!http_auth.username || !*http_auth.username) {\n-\t\tif (curl_empty_auth)\n+\t\tif (curl_empty_auth_enabled())\n \t\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, \":\");\n \t\treturn;\n \t}\n@@ -1079,7 +1117,7 @@ struct active_request_slot *get_active_slot(void)\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);\n #endif\n-\tif (http_auth.password || curl_empty_auth)\n+\tif (http_auth.password || curl_empty_auth_enabled())\n \t\tinit_curl_http_auth(slot->curl);\n \n \treturn slot;\n@@ -1347,8 +1385,10 @@ static int handle_curl_result(struct slot_results *results)\n \t\t} else {\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n \t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n-\t\t\tif (results->auth_avail)\n+\t\t\tif (results->auth_avail) {\n \t\t\t\thttp_auth_methods &= results->auth_avail;\n+\t\t\t\thttp_auth_methods_restricted = 1;\n+\t\t\t}\n #endif\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n-- \n2.12.0.616.g5f622f3b1\n\n"},{"id":"312646","messageId":"20170225191506.4it7pdsi6ijanfft@sigill.intra.peff.net","threadId":"45195","inReplyTo":"alpine.DEB.2.20.1702251243390.3767@virtualbox","subject":"Re: [PATCH 2/2] http: add an \"auto\" mode for http.emptyauth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-25T19:15:06Z","receivedAt":"2017-02-25T19:22:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2017 at 12:48:54PM +0100, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Wed, 22 Feb 2017, Jeff King wrote:\n> \n> > [two beautiful patches]\n> \n> I applied them and verified that the reported issue is fixed. Thank you!\n> \n> Hopefully you do not mind that I cherry-picked them in preparation for\n> Git for Windows v2.12.0?\n\nNo, I don't mind. I'm happy that more people with a non-Basic setup are\nverifying that they work. :)\n\nOf the changes:\n\n> diff --git a/http.c b/http.c\n> index f8eb0f23d6c..fb94c444c80 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -334,7 +334,10 @@ static int http_options(const char *var, const char *value, void *cb)\n> \t\treturn git_config_string(&user_agent, var, value);\n> \n> \tif (!strcmp(\"http.emptyauth\", var)) {\n> -\t\tcurl_empty_auth = git_config_bool(var, value);\n> +\t\tif (value && !strcmp(\"auto\", value))\n> +\t\t\tcurl_empty_auth = -1;\n> +\t\telse\n> +\t\t\tcurl_empty_auth = git_config_bool(var, value);\n> \t\treturn 0;\n> \t}\n\nObviously good, I should have included this in the original.\n\n> +#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\t/*\n> +\t * Our libcurl is too old to do AUTH_ANY in the first place;\n> +\t * just default to turning the feature off.\n> +\t */\n> #else\n> -\t\t/*\n> -\t\t * Our libcurl is too old to do AUTH_ANY in the first place;\n> -\t\t * just default to turning the feature off.\n> -\t\t */\n\nThe ifdef reordering here is good.\n\n> +\t/*\n> +\t * In the automatic case, kick in the empty-auth\n> +\t * hack as long as we would potentially try some\n> +\t * method more exotic than \"Basic\".\n> +\t *\n> +\t * But only do this when this is our second or\n> +\t * subsequent * request, as by then we know what\n> +\t * methods are available.\n> +\t */\n> +\tif (http_auth_methods_restricted)\n> +\t\tswitch (http_auth_methods) {\n> +\t\tcase CURLAUTH_BASIC:\n> +\t\tcase CURLAUTH_DIGEST:\n> +#ifdef CURLAUTH_DIGEST_IE\n> +\t\tcase CURLAUTH_DIGEST_IE:\n> #endif\n> [...]\n> +\t\t\treturn 0;\n> +\t\tdefault:\n> +\t\t\treturn 1;\n> +\t\t}\n\nThis is an improvement over my basic-only, but I think you actually want\nto bitmask here. A server which advertises only BASIC|DIGEST should not\ndo empty-auth, but wouldn't match your switch statement.\n\nPatch below.\n\n> Now, how to get this into upstream Git, too? Jeff, do you want to submit a\n> v2? In that case, would you please consider the fixup! I mentioned above?\n> Otherwise I'd be happy to take it from here.\n\nI don't mind doing a v2. I'm unsure of whether we want to default to\n\"auto\" or not upstream. It seems from your releases that you think it is\nsafe enough to do in Windows. And I guess nobody outside of that is\nreally doing NTLM. So it's OK, I guess?\n\n I don't have enough information to make an intelligent opinion,\nso I'm happy to defer.\n\nI'll send my v2 in a minute. Here's the interdiff/fixup if you need to\napply it separately:\n\ndiff --git a/http.c b/http.c\nindex 523c43cf9..dd637d031 100644\n--- a/http.c\n+++ b/http.c\n@@ -126,6 +126,13 @@ static int ssl_cert_password_required;\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n static unsigned long http_auth_methods = CURLAUTH_ANY;\n static int http_auth_methods_restricted;\n+/* Modes for which empty_auth cannot actually help us. */\n+static unsigned long empty_auth_useless =\n+\tCURLAUTH_BASIC\n+#ifdef CURLAUTH_DIGEST_IE\n+\t| CURLAUTH_DIGEST_IE\n+#endif\n+\t| CURLAUTH_DIGEST;\n #endif\n \n static struct curl_slist *pragma_header;\n@@ -400,23 +407,15 @@ static int curl_empty_auth_enabled(void)\n \t/*\n \t * In the automatic case, kick in the empty-auth\n \t * hack as long as we would potentially try some\n-\t * method more exotic than \"Basic\".\n+\t * method more exotic than \"Basic\" or \"Digest\".\n \t *\n \t * But only do this when this is our second or\n \t * subsequent * request, as by then we know what\n \t * methods are available.\n \t */\n-\tif (http_auth_methods_restricted)\n-\t\tswitch (http_auth_methods) {\n-\t\tcase CURLAUTH_BASIC:\n-\t\tcase CURLAUTH_DIGEST:\n-#ifdef CURLAUTH_DIGEST_IE\n-\t\tcase CURLAUTH_DIGEST_IE:\n-#endif\n-\t\t\treturn 0;\n-\t\tdefault:\n-\t\t\treturn 1;\n-\t\t}\n+\tif (http_auth_methods_restricted &&\n+\t (http_auth_methods & ~empty_auth_useless))\n+\t\treturn 1;\n #endif\n \treturn 0;\n }\n"},{"id":"312759","messageId":"xmqqpoi3xz1i.fsf@gitster.mtv.corp.google.com","threadId":"45195","inReplyTo":"20170225191831.dkjasyv3tmkwutre@sigill.intra.peff.net","subject":"Re: [PATCH] http: add an \"auto\" mode for http.emptyauth","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T18:35:05Z","receivedAt":"2017-02-27T19:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The auto mode may incur an extra round-trip over setting\n> http.emptyauth=true, because part of the emptyauth hack is\n> to feed this blank password to curl even before we've made a\n> single request.\n\nIOW, people who care about an extra round-trip have this workaround,\nwhich is good.\n\nThis, along with the possible security implications, may want to be\nadded to the documentation but that is outside the topic of this\nchange, and I think we would want to see such an update come from\nthose who actually use NTLM (or Kerberos, but they know they have\nminimum security implications).\n\n> +#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\t/*\n> +\t * Our libcurl is too old to do AUTH_ANY in the first place;\n> +\t * just default to turning the feature off.\n> +\t */\n> +#else\n> +\t/*\n> +\t * In the automatic case, kick in the empty-auth\n> +\t * hack as long as we would potentially try some\n> +\t * method more exotic than \"Basic\" or \"Digest\".\n> +\t *\n> +\t * But only do this when this is our second or\n> +\t * subsequent * request, as by then we know what\n\nI'll drop the '*' that you left while line-wrapping ;-)\n\n> +\t * methods are available.\n> +\t */\n\nThanks. This looks good.\n"},{"id":"312827","messageId":"alpine.DEB.2.20.1702281116360.3767@virtualbox","threadId":"45195","inReplyTo":"xmqqpoi3xz1i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http: add an \"auto\" mode for http.emptyauth","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-28T10:18:28Z","receivedAt":"2017-02-28T10:21:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 27 Feb 2017, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The auto mode may incur an extra round-trip over setting\n> > http.emptyauth=true, because part of the emptyauth hack is to feed\n> > this blank password to curl even before we've made a single request.\n> \n> IOW, people who care about an extra round-trip have this workaround,\n> which is good.\n> \n> This, along with the possible security implications, may want to be\n> added to the documentation but that is outside the topic of this change,\n> and I think we would want to see such an update come from those who\n> actually use NTLM (or Kerberos, but they know they have minimum security\n> implications).\n> \n> > +#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY +\t/* +\t * Our libcurl is\n> > too old to do AUTH_ANY in the first place; +\t * just default to\n> > turning the feature off. +\t */ +#else +\t/* +\t * In the\n> > automatic case, kick in the empty-auth +\t * hack as long as we\n> > would potentially try some +\t * method more exotic than \"Basic\"\n> > or \"Digest\". +\t * +\t * But only do this when this is our\n> > second or +\t * subsequent * request, as by then we know what\n> \n> I'll drop the '*' that you left while line-wrapping ;-)\n> \n> > +\t * methods are available. +\t */\n> \n> Thanks. This looks good.\n\nI replaced the previous version in Git for Windows' `master` branch with\nthe one in `pu`.\n\nThanks,\nJohannes\n"}]}