[PATCH v2 0/2] HTTP GSS-Negotiate improvements

[PATCH v2 0/2] HTTP GSS-Negotiate improvements

[brian m. carlson]

This patch set adds an option, http.continue, to enable requests for 100
Continue responses when pushing over HTTP.  This is needed for large pushes
using GSS-Negotiate authentication.

Changes from v1:
* Default to disabled.

brian m. carlson (2):
  http: add option to enable 100 Continue responses
  Update documentation for http.continue option

 Documentation/config.txt | 9 +++++++++
 http.c                   | 6 ++++++
 http.h                   | 1 +
 remote-curl.c            | 7 ++++++-
 4 files changed, 22 insertions(+), 1 deletion(-)

-- 
1.8.4.rc3

[PATCH v2 1/2] http: add option to enable 100 Continue responses

[brian m. carlson]

When using GSS-Negotiate authentication with libcurl, the authentication
provided will change every time, and so the probe that git uses to determine if
authentication is needed is not sufficient to guarantee that data can be sent.
If the data fits entirely in http.postBuffer bytes, the data can be rewound and
resent if authentication fails; otherwise, a 100 Continue must be requested in
this case.

By default, curl will send an Expect: 100-continue if a certain amount of data
is to be uploaded, but when using chunked data this is never triggered.  Add an
option http.continue, which defaults to disabled, to control whether this header
is sent.  The addition of an option is necessary because some proxies break
badly if sent this header.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 http.c        | 6 ++++++
 http.h        | 1 +
 remote-curl.c | 7 ++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index f3e1439..e4cd255 100644
--- a/http.c
+++ b/http.c
@@ -11,6 +11,7 @@
 int active_requests;
 int http_is_verbose;
 size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
+int http_use_100_continue = 0;
 
 #if LIBCURL_VERSION_NUM >= 0x070a06
 #define LIBCURL_CAN_HANDLE_AUTH_ANY
@@ -213,6 +214,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.continue", var)) {
+		http_use_100_continue = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp("http.useragent", var))
 		return git_config_string(&user_agent, var, value);
 
diff --git a/http.h b/http.h
index d77c1b5..e72786e 100644
--- a/http.h
+++ b/http.h
@@ -102,6 +102,7 @@ extern void http_cleanup(void);
 extern int active_requests;
 extern int http_is_verbose;
 extern size_t http_post_buffer;
+extern int http_use_100_continue;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
 
diff --git a/remote-curl.c b/remote-curl.c
index b5ebe01..3b5e160 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -470,7 +470,12 @@ static int post_rpc(struct rpc_state *rpc)
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, "Expect:");
+
+	/* Force it either on or off, since curl will try to decide based on how
+	 * much data is to be uploaded and we want consistency.
+	 */
+	headers = curl_slist_append(headers, http_use_100_continue ?
+		"Expect: 100-continue" : "Expect:");
 
 retry:
 	slot = get_active_slot();
-- 
1.8.4.rc3

Re: [PATCH v2 1/2] http: add option to enable 100 Continue responses

[Jonathan Nieder]

brian m. carlson wrote:

> When using GSS-Negotiate authentication with libcurl, the authentication
> provided will change every time, and so the probe that git uses to determine if
> authentication is needed is not sufficient to guarantee that data can be sent.
> If the data fits entirely in http.postBuffer bytes, the data can be rewound and
> resent if authentication fails; otherwise, a 100 Continue must be requested in
> this case.
>
> By default, curl will send an Expect: 100-continue if a certain amount of data
> is to be uploaded, but when using chunked data this is never triggered.  Add an
> option http.continue, which defaults to disabled, to control whether this header
> is sent.  The addition of an option is necessary because some proxies break
> badly if sent this header.

"By default" means "when allowed to make its own choice", right?  (i.e.,
the behavior git never gave libcurl a chance to try :))

Makes sense.

[...]
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,7 @@
>  int active_requests;
>  int http_is_verbose;
>  size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
> +int http_use_100_continue = 0;

Style: git tends to omit the '= 0' implicit for globals, since they are
already 0 by default.

[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -470,7 +470,12 @@ static int post_rpc(struct rpc_state *rpc)
>  
>  	headers = curl_slist_append(headers, rpc->hdr_content_type);
>  	headers = curl_slist_append(headers, rpc->hdr_accept);
> -	headers = curl_slist_append(headers, "Expect:");
> +
> +	/* Force it either on or off, since curl will try to decide based on how
> +	 * much data is to be uploaded and we want consistency.
> +	 */

Style:

	/*
	 * Multi-line comments in git have the starting and ending comment
	 * delimiters on their own lines, like this.
	 */

Will make the fixups mentioned above, squash with documentation, and
apply.  Thanks.

Re: [PATCH v2 1/2] http: add option to enable 100 Continue responses

[brian m. carlson]

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

On Fri, Oct 11, 2013 at 04:43:07PM -0700, Jonathan Nieder wrote:
> "By default" means "when allowed to make its own choice", right?  (i.e.,
> the behavior git never gave libcurl a chance to try :))
> 
> Makes sense.

Yes, at least according to Daniel Stenberg.  I don't believe it is ever
triggered the way that git calls curl with CURLOPT_READFUNCTION, but I
can't be certain since I haven't looked at the source code.

> Style: git tends to omit the '= 0' implicit for globals, since they are
> already 0 by default.

Okay.

> 	/*
> 	 * Multi-line comments in git have the starting and ending comment
> 	 * delimiters on their own lines, like this.
> 	 */

It wasn't clear from the existing code which way was being used, and the
CodingGuidelines file didn't seem to cover it.  I'll send a patch.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

Explain the reason for and behavior of the new http.continue option, and that it
is disabled by default.  Furthermore, explain that it is required for large
GSS-Negotiate requests but incompatible with some proxies.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/config.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fca7749..461c9dc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1516,6 +1516,15 @@ http.postBuffer::
 	massive pack file locally.  Default is 1 MiB, which is
 	sufficient for most requests.
 
+http.continue::
+	Ensure that authentication succeeds before sending the pack data when
+	POSTing data using the smart HTTP transport.  This is done by
+	requesting a 100 Continue response.  For requests larger than
+	'http.postBuffer', this is required when using GSS-Negotiate
+	(Kerberos) authentication over HTTP.  However, some proxies do not
+	handle the protocol exchange gracefully; for them, this option must be
+	disabled.  Defaults to disabled.
+
 http.lowSpeedLimit, http.lowSpeedTime::
 	If the HTTP transfer speed is less than 'http.lowSpeedLimit'
 	for longer than 'http.lowSpeedTime' seconds, the transfer is aborted.
-- 
1.8.4.rc3

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Jonathan Nieder]

brian m. carlson wrote:

> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1516,6 +1516,15 @@ http.postBuffer::
>  	massive pack file locally.  Default is 1 MiB, which is
>  	sufficient for most requests.
>  
> +http.continue::
> +	Ensure that authentication succeeds before sending the pack data when
> +	POSTing data using the smart HTTP transport.  This is done by
> +	requesting a 100 Continue response.  For requests larger than
> +	'http.postBuffer', this is required when using GSS-Negotiate
> +	(Kerberos) authentication over HTTP.  However, some proxies do not
> +	handle the protocol exchange gracefully; for them, this option must be
> +	disabled.  Defaults to disabled.

It's not only your company's proxy that might mishandle 100-continue
but the target server's reverse proxy (or from the point of view of
the user, the target server), right?

I think the wording could be clearer about the impact of the setting
("some proxies and reverse proxies" or something).

Perhaps this should be conditional on the authentication method used,
so affected people could still contact broken servers without having
different configuration per server and without having to wait a second
for the timeout.

Thanks,
Jonathan

Re: [PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

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

On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote:
> brian m. carlson wrote:
> > +http.continue::
> > +	Ensure that authentication succeeds before sending the pack data when
> > +	POSTing data using the smart HTTP transport.  This is done by
> > +	requesting a 100 Continue response.  For requests larger than
> > +	'http.postBuffer', this is required when using GSS-Negotiate
> > +	(Kerberos) authentication over HTTP.  However, some proxies do not
> > +	handle the protocol exchange gracefully; for them, this option must be
> > +	disabled.  Defaults to disabled.
> 
> It's not only your company's proxy that might mishandle 100-continue
> but the target server's reverse proxy (or from the point of view of
> the user, the target server), right?
> 
> I think the wording could be clearer about the impact of the setting
> ("some proxies and reverse proxies" or something).

I'm unclear about what systems are actually broken, since I don't deal
with any such systems.  One of Shawn Pearce's commit messages definitely
covered broken proxies, but it could really be anything beyond the
client: proxies, reverse proxies, servers, or even rogue intermediaries
(for non-TLS connections).

> Perhaps this should be conditional on the authentication method used,
> so affected people could still contact broken servers without having
> different configuration per server and without having to wait a second
> for the timeout.

curl determines what method, but I guess it's safe to assume that the
authentication method used for the probe will be the same as the one
used for the final connection.  Unfortunately, all curl will tell us is
what was offered, not what it would have chosen, so if GSSAPI and
something non-Basic are both offered, we'd have to make a guess.  (curl
will always prefer non-Basic to Basic for the obvious reasons.)

If you can argue for some sane semantics in what we should do in that
case, I'll reroll with that fix and a clearer wording for the docs.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

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

On Sat, Oct 12, 2013 at 12:26:39AM +0000, brian m. carlson wrote:
> On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote:
> > Perhaps this should be conditional on the authentication method used,
> > so affected people could still contact broken servers without having
> > different configuration per server and without having to wait a second
> > for the timeout.
> 
> curl determines what method, but I guess it's safe to assume that the
> authentication method used for the probe will be the same as the one
> used for the final connection.  Unfortunately, all curl will tell us is
> what was offered, not what it would have chosen, so if GSSAPI and
> something non-Basic are both offered, we'd have to make a guess.  (curl
> will always prefer non-Basic to Basic for the obvious reasons.)
> 
> If you can argue for some sane semantics in what we should do in that
> case, I'll reroll with that fix and a clearer wording for the docs.

Reading Jonathan Nieder's responses to the first patch, it looks like he
was going to apply it, but I haven't seen it make its way into next or
pu.  Junio, do you want a reroll, and if so, do you want certain
semantics for autodetecting this case, or are you just looking for
documentation fixes?

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On Sat, Oct 12, 2013 at 12:26:39AM +0000, brian m. carlson wrote:
>> On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote:
>> > Perhaps this should be conditional on the authentication method used,
>> > so affected people could still contact broken servers without having
>> > different configuration per server and without having to wait a second
>> > for the timeout.
>> 
>> curl determines what method, but I guess it's safe to assume that the
>> authentication method used for the probe will be the same as the one
>> used for the final connection.  Unfortunately, all curl will tell us is
>> what was offered, not what it would have chosen, so if GSSAPI and
>> something non-Basic are both offered, we'd have to make a guess.  (curl
>> will always prefer non-Basic to Basic for the obvious reasons.)
>> 
>> If you can argue for some sane semantics in what we should do in that
>> case, I'll reroll with that fix and a clearer wording for the docs.
>
> Reading Jonathan Nieder's responses to the first patch, it looks like he
> was going to apply it, but I haven't seen it make its way into next or
> pu.  Junio, do you want a reroll, and if so, do you want certain
> semantics for autodetecting this case, or are you just looking for
> documentation fixes?

Sorry, I wasn't following the topic closely.  I can guess what
Jonathan meant by "fixups mentioned above", which will be something
like the attached patch, but I am not sure what the conclusion of
the discussion on 2/2 was.

Reading the first part of the description alone gives me an
impression that this is only about authentication, but the change
actually affects (and it should affect) any large-payload exchange,
not limited to authentication, no?

    +http.continue::
    +	Ensure that authentication succeeds before sending the pack data when
    +	POSTing data using the smart HTTP transport.

I also somehow find "http.continue" a strange name for the option.
"http.use100Continue" that can be set to "yes" or "no" would make
sense to me, but it is not immediately obvious what "http.continue"
set to "no" would mean to regular users, opening ourselves to an
obvious misinterpretation to misread the variable name as if it
would allow resuming a large transfer that failed previously due to
connection timeout or something.

-- >8 --
From: "Brian M. Carlson" <sandals@crustytoothpaste.net>
Date: Fri, 11 Oct 2013 22:35:44 +0000
Subject: [PATCH] http: add option to enable 100 Continue responses

When using GSS-Negotiate authentication with libcurl, the
authentication provided will change every time, and so the probe
that git uses to determine if authentication is needed is not
sufficient to guarantee that data can be sent.  If the data fits
entirely in http.postBuffer bytes, the data can be rewound and
resent if authentication fails; otherwise, a 100 Continue must be
requested in this case.

The cURL library can send an "Expect: 100 continue" if a certain
amount of data is to be uploaded, but when using chunked data, we
deliberately and unconditionally disable this behaviour, because
there are many proxy servers in the wild that do not handle
"100 continue" correctly.

Add an option http.continue, which defaults to disabled, to control
whether this header is sent; this can be used when the user knows
the destination server and all the proxies between the server and
the client handle "100 continue" correctly.

Signed-off-by: Brian M. Carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c        | 6 ++++++
 http.h        | 1 +
 remote-curl.c | 9 ++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index f3e1439..58651ee 100644
--- a/http.c
+++ b/http.c
@@ -11,6 +11,7 @@
 int active_requests;
 int http_is_verbose;
 size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
+int http_use_100_continue;
 
 #if LIBCURL_VERSION_NUM >= 0x070a06
 #define LIBCURL_CAN_HANDLE_AUTH_ANY
@@ -213,6 +214,11 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.continue", var)) {
+		http_use_100_continue = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp("http.useragent", var))
 		return git_config_string(&user_agent, var, value);
 
diff --git a/http.h b/http.h
index d77c1b5..e72786e 100644
--- a/http.h
+++ b/http.h
@@ -102,6 +102,7 @@ extern void http_cleanup(void);
 extern int active_requests;
 extern int http_is_verbose;
 extern size_t http_post_buffer;
+extern int http_use_100_continue;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
 
diff --git a/remote-curl.c b/remote-curl.c
index b5ebe01..ba2b505 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -470,7 +470,14 @@ static int post_rpc(struct rpc_state *rpc)
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, "Expect:");
+
+	/*
+	 * Force it either on or off, since curl will try to decide
+	 * based on how much data is to be uploaded and we want
+	 * consistency.
+	 */
+	headers = curl_slist_append(headers, http_use_100_continue ?
+		"Expect: 100-continue" : "Expect:");
 
 retry:
 	slot = get_active_slot();
-- 
1.8.4.1-824-gb03fdb5

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Jonathan Nieder]

Junio C Hamano wrote:

>     +http.continue::
>     +	Ensure that authentication succeeds before sending the pack data when
>     +	POSTing data using the smart HTTP transport.

I think we always do that (since v1.7.5-rc0~82^2~1, "smart-http: Don't
use Expect: 100-Continue", 2011-02-15), in probe_rpc().

This series seems to be instead about ensuring that authentication
succeeds before proceding, within the same connection.  The commit
message doesn't mention this, but the symptom being addressed is the
following:

	$ git push https://bmc@git.crustytoothpaste.net/git/bmc/test.git development
	Counting objects: 37994, done.
	Delta compression using up to 4 threads.
	Compressing objects: 100% (10683/10683), done.
	Writing objects: 100% (37994/37994), 9.15 MiB | 4.45 MiB/s, done.
	Total 37994 (delta 26760), reused 37633 (delta 26467)
	Unable to rewind rpc post data - try increasing http.postBuffer
	Password for 'https://bmc@git.crustytoothpaste.net': 

As Brian explains:

	GSS-Negotiate authentication always requires a rewind with cURL.

While reviewing patch 1/2, this workaround seemed like a good idea to
me --- it lets GSS-Negotiate authentication work without harming
current users.  But after reviewing patch 2/2, it seems that there is
no good value to set this option to (I don't mean no good *default*
value, but no good value at all).  That tells me that either the
documentation needs improvement or this is the wrong knob to make
configurable.

The problem:

 a) If I set "[http] use100Continue" to true, then I can use
    GSS-Negotiate authentication without running into the problem of
    not being able to rewind.  But when I try to use code.google.com
    it will hang for a second while it waits for the 100-continue.

 b) If I set "[http] use100Continue" to false, then I can access
    code.google.com without any 1-second delays.  But I cannot perform
    large pushes using GSS-Negotiate authentication without increasing
    http.postBuffer.

Wouldn't a natural fix be to *always* use "Expect: 100-continue" when
and only when the probe_rpc() revealed a server supporting
GSS-Negotiate authentication?

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Jonathan Nieder]

Jonathan Nieder wrote:

> This series seems to be instead about ensuring that authentication
> succeeds before proceding, within the same connection.

(I mean within handling of the same request, not the same connection.)

Using "Expect: 100-Continue" would also be an alternative way to
support large pushes that need to follow redirects if we didn't have
peff's "http: update base URLs when we see redirects".

Forgive my ignorance: is there a way to do something analagous to that
patch but for GSS-Negotiate authentication?  In other words, after
using the first request to figure out what authentication mechanism
the server prefers, could git prefer it in remaining requests to avoid
the need to rewind?

I don't see any simple way to do that using the libcurl API.  If
checking if the server accepts GSS-Negotiate authentication and using
that to decide whether to 'Expect: 100-Continue' is easier, that would
be fine, too.

If neither is straightforward to do, the configuration seems like a
fine workaround --- the documentation would just have to explain that
this option is meant to be set on a per-host basis in '[http "<url>"]'
sections, to true for hosts you intend to use with GSS-Negotiate auth
and false for hosts with broken 100-continue support.

Thanks,
Jonathan

Re: [PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

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

On Tue, Oct 22, 2013 at 06:34:00PM -0700, Jonathan Nieder wrote:
> Forgive my ignorance: is there a way to do something analagous to that
> patch but for GSS-Negotiate authentication?  In other words, after
> using the first request to figure out what authentication mechanism
> the server prefers, could git prefer it in remaining requests to avoid
> the need to rewind?

We know what authentication mechanisms the server offers, but we don't
know what curl will use, other than the fact that it prefers non-Basic
authentication (this is documented).  So if we see Negotiate only or
Negotiate and Basic, we know it will try to use Negotiate if possible.

So essentially the question is, do we believe that Negotiate and other
authentication are likely to be used together, and are we willing to
take the risk that a user wants to use non-Negotiate on such a server
and has a broken server or proxy?

> I don't see any simple way to do that using the libcurl API.  If
> checking if the server accepts GSS-Negotiate authentication and using
> that to decide whether to 'Expect: 100-Continue' is easier, that would
> be fine, too.

If that's what the consensus is, that's much, much easier to do.  The
only problem is that if we have Negotiate and a non-Basic method, such
as Digest, we might force Expect: 100-continue on when it does not need
to be used.  I think that in practice that's unlikely to be the case, as
most people using GSSAPI authentication probably use Kerberos as their
authentication server, and Digest hashes the password in a different way
that isn't compatible.  I'm the only person I'm aware of, other than
Stanford (and potentially MIT), that uses Kerberos auth over HTTP, and
probably fewer still use it for git push.

I think that this is probably fine to do, because probably most people
who are using Kerberos auth over HTTP are using Apache or nginx, which
support 100-continue, and many of those institutions are universities,
which don't tend to have restrictive (and broken) proxies.  It also is
fine with me because it means that things just work and I don't have to
worry about setting configuration options.

I'll plan to do a revised patch along these lines later this week unless
I hear some loud objections before then.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Shawn Pearce]

On Tue, Oct 22, 2013 at 8:00 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Tue, Oct 22, 2013 at 06:34:00PM -0700, Jonathan Nieder wrote:
>> Forgive my ignorance: is there a way to do something analagous to that
>> patch but for GSS-Negotiate authentication?  In other words, after
>> using the first request to figure out what authentication mechanism
>> the server prefers, could git prefer it in remaining requests to avoid
>> the need to rewind?
>
> We know what authentication mechanisms the server offers, but we don't
> know what curl will use, other than the fact that it prefers non-Basic
> authentication (this is documented).  So if we see Negotiate only or
> Negotiate and Basic, we know it will try to use Negotiate if possible.

Yes.

>> I don't see any simple way to do that using the libcurl API.  If
>> checking if the server accepts GSS-Negotiate authentication and using
>> that to decide whether to 'Expect: 100-Continue' is easier, that would
>> be fine, too.
>
> If that's what the consensus is, that's much, much easier to do.  The
> only problem is that if we have Negotiate and a non-Basic method, such
> as Digest, we might force Expect: 100-continue on when it does not need
> to be used.

>From my perspective, it is OK to defaulting to use 100-continue if the
server supports Negotiate. If the user is stuck behind a broken proxy
and can't authenticate, they can't authenticate. They can either set
the variable to false, or fix their proxy, or use a different server,
etc.

Re: [PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

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

On Tue, Oct 22, 2013 at 08:21:48PM -0700, Shawn Pearce wrote:
> From my perspective, it is OK to defaulting to use 100-continue if the
> server supports Negotiate. If the user is stuck behind a broken proxy
> and can't authenticate, they can't authenticate. They can either set
> the variable to false, or fix their proxy, or use a different server,
> etc.

I think Jonathan's suggestion was to get rid of the variable altogether
and simply make the code conditional on whether the server is offering
GSS-Negotiate.  I plan to make the use of 100-continue conditional on
large_request as well, so that it only covers the case where it would
otherwise fail.  People who have broken proxies or broken servers and
are using GSS-Negotiate (which, as I said, is probably very few people,
if any) will simply have to increase the postbuffer size as before.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Jeff King]

On Wed, Oct 23, 2013 at 10:56:32PM +0000, brian m. carlson wrote:

> On Tue, Oct 22, 2013 at 08:21:48PM -0700, Shawn Pearce wrote:
> > From my perspective, it is OK to defaulting to use 100-continue if the
> > server supports Negotiate. If the user is stuck behind a broken proxy
> > and can't authenticate, they can't authenticate. They can either set
> > the variable to false, or fix their proxy, or use a different server,
> > etc.
> 
> I think Jonathan's suggestion was to get rid of the variable altogether
> and simply make the code conditional on whether the server is offering
> GSS-Negotiate.  I plan to make the use of 100-continue conditional on
> large_request as well, so that it only covers the case where it would
> otherwise fail.

I think that makes sense. Would you also want to suppress the probe
request in that case? It serves the same purpose, but would cause you to
do an extra auth for no reason (which potentially means user input,
which is annoying).

Also, if I recall your original complaint correctly, the "Expect"
handling was only half of the problem. Wasn't there also an issue where
git prompts for a password, even though GSS-Negotiate doesn't use it? Do
you have a plan for that?

-Peff

Re: [PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

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

On Fri, Oct 25, 2013 at 03:17:06AM -0400, Jeff King wrote:
> I think that makes sense. Would you also want to suppress the probe
> request in that case? It serves the same purpose, but would cause you to
> do an extra auth for no reason (which potentially means user input,
> which is annoying).

I actually need the probe request to determine what authentication is
available, since it isn't guaranteed that the info/refs request requires
authentication (in my case, it doesn't).

> Also, if I recall your original complaint correctly, the "Expect"
> handling was only half of the problem. Wasn't there also an issue where
> git prompts for a password, even though GSS-Negotiate doesn't use it? Do
> you have a plan for that?

It only does that if it fails.  I do plan to fix that, although
potentially in a separate patch.  It's really very minor, though silly
to have it prompt when that's not ever going to be useful.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] Update documentation for http.continue option

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Wouldn't a natural fix be to *always* use "Expect: 100-continue" when
> and only when the probe_rpc() revealed a server supporting
> GSS-Negotiate authentication?

A stupid question. Is GSS-Negotiate the only special case?

Re: [PATCH v2 2/2] Update documentation for http.continue option

[brian m. carlson]

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

On Wed, Oct 23, 2013 at 08:47:57AM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Wouldn't a natural fix be to *always* use "Expect: 100-continue" when
> > and only when the probe_rpc() revealed a server supporting
> > GSS-Negotiate authentication?
> 
> A stupid question. Is GSS-Negotiate the only special case?

I believe so, because it's the only case where we don't have a password.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]