{"thread":{"id":"35111","subject":"[PATCH v2 0/2] HTTP GSS-Negotiate improvements","startedAt":"2013-10-11T22:35:43Z","lastAt":"2013-10-25T20:56:46Z","messageCount":18,"participants":["brian m. carlson","Jonathan Nieder","Junio C Hamano","Shawn Pearce","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"228753","messageId":"1381530945-90590-1-git-send-email-sandals@crustytoothpaste.net","threadId":"35111","inReplyTo":null,"subject":"[PATCH v2 0/2] HTTP GSS-Negotiate improvements","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-11T22:35:43Z","receivedAt":"2013-10-11T22:35:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This patch set adds an option, http.continue, to enable requests for 100\nContinue responses when pushing over HTTP. This is needed for large pushes\nusing GSS-Negotiate authentication.\n\nChanges from v1:\n* Default to disabled.\n\nbrian m. carlson (2):\n http: add option to enable 100 Continue responses\n Update documentation for http.continue option\n\n Documentation/config.txt | 9 +++++++++\n http.c | 6 ++++++\n http.h | 1 +\n remote-curl.c | 7 ++++++-\n 4 files changed, 22 insertions(+), 1 deletion(-)\n\n-- \n1.8.4.rc3\n"},{"id":"228754","messageId":"1381530945-90590-2-git-send-email-sandals@crustytoothpaste.net","threadId":"35111","inReplyTo":"1381530945-90590-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/2] http: add option to enable 100 Continue responses","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-11T22:35:44Z","receivedAt":"2013-10-11T22:35:44Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When using GSS-Negotiate authentication with libcurl, the authentication\nprovided will change every time, and so the probe that git uses to determine if\nauthentication is needed is not sufficient to guarantee that data can be sent.\nIf the data fits entirely in http.postBuffer bytes, the data can be rewound and\nresent if authentication fails; otherwise, a 100 Continue must be requested in\nthis case.\n\nBy default, curl will send an Expect: 100-continue if a certain amount of data\nis to be uploaded, but when using chunked data this is never triggered. Add an\noption http.continue, which defaults to disabled, to control whether this header\nis sent. The addition of an option is necessary because some proxies break\nbadly if sent this header.\n\nSigned-off-by: brian m. carlson \n---\n http.c | 6 ++++++\n http.h | 1 +\n remote-curl.c | 7 ++++++-\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex f3e1439..e4cd255 100644\n--- a/http.c\n+++ b/http.c\n@@ -11,6 +11,7 @@\n int active_requests;\n int http_is_verbose;\n size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n+int http_use_100_continue = 0;\n \n #if LIBCURL_VERSION_NUM >= 0x070a06\n #define LIBCURL_CAN_HANDLE_AUTH_ANY\n@@ -213,6 +214,11 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(\"http.continue\", var)) {\n+\t\thttp_use_100_continue = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(\"http.useragent\", var))\n \t\treturn git_config_string(&user_agent, var, value);\n \ndiff --git a/http.h b/http.h\nindex d77c1b5..e72786e 100644\n--- a/http.h\n+++ b/http.h\n@@ -102,6 +102,7 @@ extern void http_cleanup(void);\n extern int active_requests;\n extern int http_is_verbose;\n extern size_t http_post_buffer;\n+extern int http_use_100_continue;\n \n extern char curl_errorstr[CURL_ERROR_SIZE];\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex b5ebe01..3b5e160 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -470,7 +470,12 @@ static int post_rpc(struct rpc_state *rpc)\n \n \theaders = curl_slist_append(headers, rpc->hdr_content_type);\n \theaders = curl_slist_append(headers, rpc->hdr_accept);\n-\theaders = curl_slist_append(headers, \"Expect:\");\n+\n+\t/* Force it either on or off, since curl will try to decide based on how\n+\t * much data is to be uploaded and we want consistency.\n+\t */\n+\theaders = curl_slist_append(headers, http_use_100_continue ?\n+\t\t\"Expect: 100-continue\" : \"Expect:\");\n \n retry:\n \tslot = get_active_slot();\n-- \n1.8.4.rc3\n"},{"id":"228755","messageId":"1381530945-90590-3-git-send-email-sandals@crustytoothpaste.net","threadId":"35111","inReplyTo":"1381530945-90590-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-11T22:35:45Z","receivedAt":"2013-10-11T22:35:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Explain the reason for and behavior of the new http.continue option, and that it\nis disabled by default. Furthermore, explain that it is required for large\nGSS-Negotiate requests but incompatible with some proxies.\n\nSigned-off-by: brian m. carlson \n---\n Documentation/config.txt | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fca7749..461c9dc 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1516,6 +1516,15 @@ http.postBuffer::\n \tmassive pack file locally. Default is 1 MiB, which is\n \tsufficient for most requests.\n \n+http.continue::\n+\tEnsure that authentication succeeds before sending the pack data when\n+\tPOSTing data using the smart HTTP transport. This is done by\n+\trequesting a 100 Continue response. For requests larger than\n+\t'http.postBuffer', this is required when using GSS-Negotiate\n+\t(Kerberos) authentication over HTTP. However, some proxies do not\n+\thandle the protocol exchange gracefully; for them, this option must be\n+\tdisabled. Defaults to disabled.\n+\n http.lowSpeedLimit, http.lowSpeedTime::\n \tIf the HTTP transfer speed is less than 'http.lowSpeedLimit'\n \tfor longer than 'http.lowSpeedTime' seconds, the transfer is aborted.\n-- \n1.8.4.rc3\n"},{"id":"228756","messageId":"20131011234307.GU9464@google.com","threadId":"35111","inReplyTo":"1381530945-90590-2-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] http: add option to enable 100 Continue responses","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-11T23:43:07Z","receivedAt":"2013-10-11T23:43:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> When using GSS-Negotiate authentication with libcurl, the authentication\n> provided will change every time, and so the probe that git uses to determine if\n> authentication is needed is not sufficient to guarantee that data can be sent.\n> If the data fits entirely in http.postBuffer bytes, the data can be rewound and\n> resent if authentication fails; otherwise, a 100 Continue must be requested in\n> this case.\n>\n> By default, curl will send an Expect: 100-continue if a certain amount of data\n> is to be uploaded, but when using chunked data this is never triggered. Add an\n> option http.continue, which defaults to disabled, to control whether this header\n> is sent. The addition of an option is necessary because some proxies break\n> badly if sent this header.\n\n\"By default\" means \"when allowed to make its own choice\", right? (i.e.,\nthe behavior git never gave libcurl a chance to try :))\n\nMakes sense.\n\n[...]\n> --- a/http.c\n> +++ b/http.c\n> @@ -11,6 +11,7 @@\n> int active_requests;\n> int http_is_verbose;\n> size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n> +int http_use_100_continue = 0;\n\nStyle: git tends to omit the '= 0' implicit for globals, since they are\nalready 0 by default.\n\n[...]\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -470,7 +470,12 @@ static int post_rpc(struct rpc_state *rpc)\n> \n> \theaders = curl_slist_append(headers, rpc->hdr_content_type);\n> \theaders = curl_slist_append(headers, rpc->hdr_accept);\n> -\theaders = curl_slist_append(headers, \"Expect:\");\n> +\n> +\t/* Force it either on or off, since curl will try to decide based on how\n> +\t * much data is to be uploaded and we want consistency.\n> +\t */\n\nStyle:\n\n\t/*\n\t * Multi-line comments in git have the starting and ending comment\n\t * delimiters on their own lines, like this.\n\t */\n\nWill make the fixups mentioned above, squash with documentation, and\napply. Thanks.\n"},{"id":"228757","messageId":"20131011235052.GV9464@google.com","threadId":"35111","inReplyTo":"1381530945-90590-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-11T23:50:52Z","receivedAt":"2013-10-11T23:50:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1516,6 +1516,15 @@ http.postBuffer::\n> \tmassive pack file locally. Default is 1 MiB, which is\n> \tsufficient for most requests.\n> \n> +http.continue::\n> +\tEnsure that authentication succeeds before sending the pack data when\n> +\tPOSTing data using the smart HTTP transport. This is done by\n> +\trequesting a 100 Continue response. For requests larger than\n> +\t'http.postBuffer', this is required when using GSS-Negotiate\n> +\t(Kerberos) authentication over HTTP. However, some proxies do not\n> +\thandle the protocol exchange gracefully; for them, this option must be\n> +\tdisabled. Defaults to disabled.\n\nIt's not only your company's proxy that might mishandle 100-continue\nbut the target server's reverse proxy (or from the point of view of\nthe user, the target server), right?\n\nI think the wording could be clearer about the impact of the setting\n(\"some proxies and reverse proxies\" or something).\n\nPerhaps this should be conditional on the authentication method used,\nso affected people could still contact broken servers without having\ndifferent configuration per server and without having to wait a second\nfor the timeout.\n\nThanks,\nJonathan\n"},{"id":"228760","messageId":"20131012000334.GD79408@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"20131011234307.GU9464@google.com","subject":"Re: [PATCH v2 1/2] http: add option to enable 100 Continue responses","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-12T00:03:34Z","receivedAt":"2013-10-12T00:03:34Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Oct 11, 2013 at 04:43:07PM -0700, Jonathan Nieder wrote:\n> \"By default\" means \"when allowed to make its own choice\", right? (i.e.,\n> the behavior git never gave libcurl a chance to try :))\n> \n> Makes sense.\n\nYes, at least according to Daniel Stenberg. I don't believe it is ever\ntriggered the way that git calls curl with CURLOPT_READFUNCTION, but I\ncan't be certain since I haven't looked at the source code.\n\n> Style: git tends to omit the '= 0' implicit for globals, since they are\n> already 0 by default.\n\nOkay.\n\n> \t/*\n> \t * Multi-line comments in git have the starting and ending comment\n> \t * delimiters on their own lines, like this.\n> \t */\n\nIt wasn't clear from the existing code which way was being used, and the\nCodingGuidelines file didn't seem to cover it. I'll send a patch.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"228761","messageId":"20131012002639.GE79408@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"20131011235052.GV9464@google.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-12T00:26:39Z","receivedAt":"2013-10-12T00:26:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote:\n> brian m. carlson wrote:\n> > +http.continue::\n> > +\tEnsure that authentication succeeds before sending the pack data when\n> > +\tPOSTing data using the smart HTTP transport. This is done by\n> > +\trequesting a 100 Continue response. For requests larger than\n> > +\t'http.postBuffer', this is required when using GSS-Negotiate\n> > +\t(Kerberos) authentication over HTTP. However, some proxies do not\n> > +\thandle the protocol exchange gracefully; for them, this option must be\n> > +\tdisabled. Defaults to disabled.\n> \n> It's not only your company's proxy that might mishandle 100-continue\n> but the target server's reverse proxy (or from the point of view of\n> the user, the target server), right?\n> \n> I think the wording could be clearer about the impact of the setting\n> (\"some proxies and reverse proxies\" or something).\n\nI'm unclear about what systems are actually broken, since I don't deal\nwith any such systems. One of Shawn Pearce's commit messages definitely\ncovered broken proxies, but it could really be anything beyond the\nclient: proxies, reverse proxies, servers, or even rogue intermediaries\n(for non-TLS connections).\n\n> Perhaps this should be conditional on the authentication method used,\n> so affected people could still contact broken servers without having\n> different configuration per server and without having to wait a second\n> for the timeout.\n\ncurl determines what method, but I guess it's safe to assume that the\nauthentication method used for the probe will be the same as the one\nused for the final connection. Unfortunately, all curl will tell us is\nwhat was offered, not what it would have chosen, so if GSSAPI and\nsomething non-Basic are both offered, we'd have to make a guess. (curl\nwill always prefer non-Basic to Basic for the obvious reasons.)\n\nIf you can argue for some sane semantics in what we should do in that\ncase, I'll reroll with that fix and a clearer wording for the docs.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"229193","messageId":"20131018221535.GM865149@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"20131012002639.GE79408@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-18T22:15:35Z","receivedAt":"2013-10-18T22:15:35Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Oct 12, 2013 at 12:26:39AM +0000, brian m. carlson wrote:\n> On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote:\n> > Perhaps this should be conditional on the authentication method used,\n> > so affected people could still contact broken servers without having\n> > different configuration per server and without having to wait a second\n> > for the timeout.\n> \n> curl determines what method, but I guess it's safe to assume that the\n> authentication method used for the probe will be the same as the one\n> used for the final connection. Unfortunately, all curl will tell us is\n> what was offered, not what it would have chosen, so if GSSAPI and\n> something non-Basic are both offered, we'd have to make a guess. (curl\n> will always prefer non-Basic to Basic for the obvious reasons.)\n> \n> If you can argue for some sane semantics in what we should do in that\n> case, I'll reroll with that fix and a clearer wording for the docs.\n\nReading Jonathan Nieder's responses to the first patch, it looks like he\nwas going to apply it, but I haven't seen it make its way into next or\npu. Junio, do you want a reroll, and if so, do you want certain\nsemantics for autodetecting this case, or are you just looking for\ndocumentation fixes?\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"229308","messageId":"xmqqeh7csygy.fsf@gitster.dls.corp.google.com","threadId":"35111","inReplyTo":"20131018221535.GM865149@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-22T23:00:13Z","receivedAt":"2013-10-22T23:00:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On Sat, Oct 12, 2013 at 12:26:39AM +0000, brian m. carlson wrote:\n>> On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote:\n>> > Perhaps this should be conditional on the authentication method used,\n>> > so affected people could still contact broken servers without having\n>> > different configuration per server and without having to wait a second\n>> > for the timeout.\n>> \n>> curl determines what method, but I guess it's safe to assume that the\n>> authentication method used for the probe will be the same as the one\n>> used for the final connection. Unfortunately, all curl will tell us is\n>> what was offered, not what it would have chosen, so if GSSAPI and\n>> something non-Basic are both offered, we'd have to make a guess. (curl\n>> will always prefer non-Basic to Basic for the obvious reasons.)\n>> \n>> If you can argue for some sane semantics in what we should do in that\n>> case, I'll reroll with that fix and a clearer wording for the docs.\n>\n> Reading Jonathan Nieder's responses to the first patch, it looks like he\n> was going to apply it, but I haven't seen it make its way into next or\n> pu. Junio, do you want a reroll, and if so, do you want certain\n> semantics for autodetecting this case, or are you just looking for\n> documentation fixes?\n\nSorry, I wasn't following the topic closely. I can guess what\nJonathan meant by \"fixups mentioned above\", which will be something\nlike the attached patch, but I am not sure what the conclusion of\nthe discussion on 2/2 was.\n\nReading the first part of the description alone gives me an\nimpression that this is only about authentication, but the change\nactually affects (and it should affect) any large-payload exchange,\nnot limited to authentication, no?\n\n +http.continue::\n +\tEnsure that authentication succeeds before sending the pack data when\n +\tPOSTing data using the smart HTTP transport.\n\nI also somehow find \"http.continue\" a strange name for the option.\n\"http.use100Continue\" that can be set to \"yes\" or \"no\" would make\nsense to me, but it is not immediately obvious what \"http.continue\"\nset to \"no\" would mean to regular users, opening ourselves to an\nobvious misinterpretation to misread the variable name as if it\nwould allow resuming a large transfer that failed previously due to\nconnection timeout or something.\n\n-- >8 --\nFrom: \"Brian M. Carlson\" \nDate: Fri, 11 Oct 2013 22:35:44 +0000\nSubject: [PATCH] http: add option to enable 100 Continue responses\n\nWhen using GSS-Negotiate authentication with libcurl, the\nauthentication provided will change every time, and so the probe\nthat git uses to determine if authentication is needed is not\nsufficient to guarantee that data can be sent. If the data fits\nentirely in http.postBuffer bytes, the data can be rewound and\nresent if authentication fails; otherwise, a 100 Continue must be\nrequested in this case.\n\nThe cURL library can send an \"Expect: 100 continue\" if a certain\namount of data is to be uploaded, but when using chunked data, we\ndeliberately and unconditionally disable this behaviour, because\nthere are many proxy servers in the wild that do not handle\n\"100 continue\" correctly.\n\nAdd an option http.continue, which defaults to disabled, to control\nwhether this header is sent; this can be used when the user knows\nthe destination server and all the proxies between the server and\nthe client handle \"100 continue\" correctly.\n\nSigned-off-by: Brian M. Carlson \nSigned-off-by: Junio C Hamano \n---\n http.c | 6 ++++++\n http.h | 1 +\n remote-curl.c | 9 ++++++++-\n 3 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex f3e1439..58651ee 100644\n--- a/http.c\n+++ b/http.c\n@@ -11,6 +11,7 @@\n int active_requests;\n int http_is_verbose;\n size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n+int http_use_100_continue;\n \n #if LIBCURL_VERSION_NUM >= 0x070a06\n #define LIBCURL_CAN_HANDLE_AUTH_ANY\n@@ -213,6 +214,11 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(\"http.continue\", var)) {\n+\t\thttp_use_100_continue = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(\"http.useragent\", var))\n \t\treturn git_config_string(&user_agent, var, value);\n \ndiff --git a/http.h b/http.h\nindex d77c1b5..e72786e 100644\n--- a/http.h\n+++ b/http.h\n@@ -102,6 +102,7 @@ extern void http_cleanup(void);\n extern int active_requests;\n extern int http_is_verbose;\n extern size_t http_post_buffer;\n+extern int http_use_100_continue;\n \n extern char curl_errorstr[CURL_ERROR_SIZE];\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex b5ebe01..ba2b505 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -470,7 +470,14 @@ static int post_rpc(struct rpc_state *rpc)\n \n \theaders = curl_slist_append(headers, rpc->hdr_content_type);\n \theaders = curl_slist_append(headers, rpc->hdr_accept);\n-\theaders = curl_slist_append(headers, \"Expect:\");\n+\n+\t/*\n+\t * Force it either on or off, since curl will try to decide\n+\t * based on how much data is to be uploaded and we want\n+\t * consistency.\n+\t */\n+\theaders = curl_slist_append(headers, http_use_100_continue ?\n+\t\t\"Expect: 100-continue\" : \"Expect:\");\n \n retry:\n \tslot = get_active_slot();\n-- \n1.8.4.1-824-gb03fdb5\n"},{"id":"229309","messageId":"20131022233257.GA9464@google.com","threadId":"35111","inReplyTo":"xmqqeh7csygy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-22T23:32:58Z","receivedAt":"2013-10-22T23:32:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> +http.continue::\n> +\tEnsure that authentication succeeds before sending the pack data when\n> +\tPOSTing data using the smart HTTP transport.\n\nI think we always do that (since v1.7.5-rc0~82^2~1, \"smart-http: Don't\nuse Expect: 100-Continue\", 2011-02-15), in probe_rpc().\n\nThis series seems to be instead about ensuring that authentication\nsucceeds before proceding, within the same connection. The commit\nmessage doesn't mention this, but the symptom being addressed is the\nfollowing:\n\n\t$ git push https://bmc@git.crustytoothpaste.net/git/bmc/test.git development\n\tCounting objects: 37994, done.\n\tDelta compression using up to 4 threads.\n\tCompressing objects: 100% (10683/10683), done.\n\tWriting objects: 100% (37994/37994), 9.15 MiB | 4.45 MiB/s, done.\n\tTotal 37994 (delta 26760), reused 37633 (delta 26467)\n\tUnable to rewind rpc post data - try increasing http.postBuffer\n\tPassword for 'https://bmc@git.crustytoothpaste.net': \n\nAs Brian explains:\n\n\tGSS-Negotiate authentication always requires a rewind with cURL.\n\nWhile reviewing patch 1/2, this workaround seemed like a good idea to\nme --- it lets GSS-Negotiate authentication work without harming\ncurrent users. But after reviewing patch 2/2, it seems that there is\nno good value to set this option to (I don't mean no good *default*\nvalue, but no good value at all). That tells me that either the\ndocumentation needs improvement or this is the wrong knob to make\nconfigurable.\n\nThe problem:\n\n a) If I set \"[http] use100Continue\" to true, then I can use\n GSS-Negotiate authentication without running into the problem of\n not being able to rewind. But when I try to use code.google.com\n it will hang for a second while it waits for the 100-continue.\n\n b) If I set \"[http] use100Continue\" to false, then I can access\n code.google.com without any 1-second delays. But I cannot perform\n large pushes using GSS-Negotiate authentication without increasing\n http.postBuffer.\n\nWouldn't a natural fix be to *always* use \"Expect: 100-continue\" when\nand only when the probe_rpc() revealed a server supporting\nGSS-Negotiate authentication?\n"},{"id":"229311","messageId":"20131023013400.GB9464@google.com","threadId":"35111","inReplyTo":"20131022233257.GA9464@google.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-23T01:34:00Z","receivedAt":"2013-10-23T01:34:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> This series seems to be instead about ensuring that authentication\n> succeeds before proceding, within the same connection.\n\n(I mean within handling of the same request, not the same connection.)\n\nUsing \"Expect: 100-Continue\" would also be an alternative way to\nsupport large pushes that need to follow redirects if we didn't have\npeff's \"http: update base URLs when we see redirects\".\n\nForgive my ignorance: is there a way to do something analagous to that\npatch but for GSS-Negotiate authentication? In other words, after\nusing the first request to figure out what authentication mechanism\nthe server prefers, could git prefer it in remaining requests to avoid\nthe need to rewind?\n\nI don't see any simple way to do that using the libcurl API. If\nchecking if the server accepts GSS-Negotiate authentication and using\nthat to decide whether to 'Expect: 100-Continue' is easier, that would\nbe fine, too.\n\nIf neither is straightforward to do, the configuration seems like a\nfine workaround --- the documentation would just have to explain that\nthis option is meant to be set on a per-host basis in '[http \"\"]'\nsections, to true for hosts you intend to use with GSS-Negotiate auth\nand false for hosts with broken 100-continue support.\n\nThanks,\nJonathan\n"},{"id":"229312","messageId":"20131023030048.GX865149@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"20131023013400.GB9464@google.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-23T03:00:48Z","receivedAt":"2013-10-23T03:00:48Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Oct 22, 2013 at 06:34:00PM -0700, Jonathan Nieder wrote:\n> Forgive my ignorance: is there a way to do something analagous to that\n> patch but for GSS-Negotiate authentication? In other words, after\n> using the first request to figure out what authentication mechanism\n> the server prefers, could git prefer it in remaining requests to avoid\n> the need to rewind?\n\nWe know what authentication mechanisms the server offers, but we don't\nknow what curl will use, other than the fact that it prefers non-Basic\nauthentication (this is documented). So if we see Negotiate only or\nNegotiate and Basic, we know it will try to use Negotiate if possible.\n\nSo essentially the question is, do we believe that Negotiate and other\nauthentication are likely to be used together, and are we willing to\ntake the risk that a user wants to use non-Negotiate on such a server\nand has a broken server or proxy?\n\n> I don't see any simple way to do that using the libcurl API. If\n> checking if the server accepts GSS-Negotiate authentication and using\n> that to decide whether to 'Expect: 100-Continue' is easier, that would\n> be fine, too.\n\nIf that's what the consensus is, that's much, much easier to do. The\nonly problem is that if we have Negotiate and a non-Basic method, such\nas Digest, we might force Expect: 100-continue on when it does not need\nto be used. I think that in practice that's unlikely to be the case, as\nmost people using GSSAPI authentication probably use Kerberos as their\nauthentication server, and Digest hashes the password in a different way\nthat isn't compatible. I'm the only person I'm aware of, other than\nStanford (and potentially MIT), that uses Kerberos auth over HTTP, and\nprobably fewer still use it for git push.\n\nI think that this is probably fine to do, because probably most people\nwho are using Kerberos auth over HTTP are using Apache or nginx, which\nsupport 100-continue, and many of those institutions are universities,\nwhich don't tend to have restrictive (and broken) proxies. It also is\nfine with me because it means that things just work and I don't have to\nworry about setting configuration options.\n\nI'll plan to do a revised patch along these lines later this week unless\nI hear some loud objections before then.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"229313","messageId":"CAJo=hJssxNtJSeLgwG5nON=Y-7HzZPULiwzH+0SXeU8tp3FC-A@mail.gmail.com","threadId":"35111","inReplyTo":"20131023030048.GX865149@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2013-10-23T03:21:48Z","receivedAt":"2013-10-23T03:21:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Oct 22, 2013 at 8:00 PM, brian m. carlson\n wrote:\n> On Tue, Oct 22, 2013 at 06:34:00PM -0700, Jonathan Nieder wrote:\n>> Forgive my ignorance: is there a way to do something analagous to that\n>> patch but for GSS-Negotiate authentication? In other words, after\n>> using the first request to figure out what authentication mechanism\n>> the server prefers, could git prefer it in remaining requests to avoid\n>> the need to rewind?\n>\n> We know what authentication mechanisms the server offers, but we don't\n> know what curl will use, other than the fact that it prefers non-Basic\n> authentication (this is documented). So if we see Negotiate only or\n> Negotiate and Basic, we know it will try to use Negotiate if possible.\n\nYes.\n\n>> I don't see any simple way to do that using the libcurl API. If\n>> checking if the server accepts GSS-Negotiate authentication and using\n>> that to decide whether to 'Expect: 100-Continue' is easier, that would\n>> be fine, too.\n>\n> If that's what the consensus is, that's much, much easier to do. The\n> only problem is that if we have Negotiate and a non-Basic method, such\n> as Digest, we might force Expect: 100-continue on when it does not need\n> to be used.\n\n>From my perspective, it is OK to defaulting to use 100-continue if the\nserver supports Negotiate. If the user is stuck behind a broken proxy\nand can't authenticate, they can't authenticate. They can either set\nthe variable to false, or fix their proxy, or use a different server,\netc.\n"},{"id":"229328","messageId":"xmqq61sornte.fsf@gitster.dls.corp.google.com","threadId":"35111","inReplyTo":"20131022233257.GA9464@google.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T15:47:57Z","receivedAt":"2013-10-23T15:47:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Wouldn't a natural fix be to *always* use \"Expect: 100-continue\" when\n> and only when the probe_rpc() revealed a server supporting\n> GSS-Negotiate authentication?\n\nA stupid question. Is GSS-Negotiate the only special case?\n"},{"id":"229370","messageId":"20131023225314.GY865149@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"xmqq61sornte.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-23T22:53:14Z","receivedAt":"2013-10-23T22:53:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Oct 23, 2013 at 08:47:57AM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> \n> > Wouldn't a natural fix be to *always* use \"Expect: 100-continue\" when\n> > and only when the probe_rpc() revealed a server supporting\n> > GSS-Negotiate authentication?\n> \n> A stupid question. Is GSS-Negotiate the only special case?\n\nI believe so, because it's the only case where we don't have a password.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"229371","messageId":"20131023225632.GZ865149@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"CAJo=hJssxNtJSeLgwG5nON=Y-7HzZPULiwzH+0SXeU8tp3FC-A@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-23T22:56:32Z","receivedAt":"2013-10-23T22:56:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Oct 22, 2013 at 08:21:48PM -0700, Shawn Pearce wrote:\n> From my perspective, it is OK to defaulting to use 100-continue if the\n> server supports Negotiate. If the user is stuck behind a broken proxy\n> and can't authenticate, they can't authenticate. They can either set\n> the variable to false, or fix their proxy, or use a different server,\n> etc.\n\nI think Jonathan's suggestion was to get rid of the variable altogether\nand simply make the code conditional on whether the server is offering\nGSS-Negotiate. I plan to make the use of 100-continue conditional on\nlarge_request as well, so that it only covers the case where it would\notherwise fail. People who have broken proxies or broken servers and\nare using GSS-Negotiate (which, as I said, is probably very few people,\nif any) will simply have to increase the postbuffer size as before.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"229505","messageId":"20131025071706.GA17029@sigill.intra.peff.net","threadId":"35111","inReplyTo":"20131023225632.GZ865149@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-25T07:17:06Z","receivedAt":"2013-10-25T07:17:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 23, 2013 at 10:56:32PM +0000, brian m. carlson wrote:\n\n> On Tue, Oct 22, 2013 at 08:21:48PM -0700, Shawn Pearce wrote:\n> > From my perspective, it is OK to defaulting to use 100-continue if the\n> > server supports Negotiate. If the user is stuck behind a broken proxy\n> > and can't authenticate, they can't authenticate. They can either set\n> > the variable to false, or fix their proxy, or use a different server,\n> > etc.\n> \n> I think Jonathan's suggestion was to get rid of the variable altogether\n> and simply make the code conditional on whether the server is offering\n> GSS-Negotiate. I plan to make the use of 100-continue conditional on\n> large_request as well, so that it only covers the case where it would\n> otherwise fail.\n\nI think that makes sense. Would you also want to suppress the probe\nrequest in that case? It serves the same purpose, but would cause you to\ndo an extra auth for no reason (which potentially means user input,\nwhich is annoying).\n\nAlso, if I recall your original complaint correctly, the \"Expect\"\nhandling was only half of the problem. Wasn't there also an issue where\ngit prompts for a password, even though GSS-Negotiate doesn't use it? Do\nyou have a plan for that?\n\n-Peff\n"},{"id":"229527","messageId":"20131025205628.GA38426@vauxhall.crustytoothpaste.net","threadId":"35111","inReplyTo":"20131025071706.GA17029@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] Update documentation for http.continue option","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-10-25T20:56:46Z","receivedAt":"2013-10-25T20:56:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Oct 25, 2013 at 03:17:06AM -0400, Jeff King wrote:\n> I think that makes sense. Would you also want to suppress the probe\n> request in that case? It serves the same purpose, but would cause you to\n> do an extra auth for no reason (which potentially means user input,\n> which is annoying).\n\nI actually need the probe request to determine what authentication is\navailable, since it isn't guaranteed that the info/refs request requires\nauthentication (in my case, it doesn't).\n\n> Also, if I recall your original complaint correctly, the \"Expect\"\n> handling was only half of the problem. Wasn't there also an issue where\n> git prompts for a password, even though GSS-Negotiate doesn't use it? Do\n> you have a plan for that?\n\nIt only does that if it fails. I do plan to fix that, although\npotentially in a separate patch. It's really very minor, though silly\nto have it prompt when that's not ever going to be useful.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"}]}