[PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Dmitry Vilkov]

This is fix of bug introduced by 4dbe66464 commit.
The problem is that when username/password combination was not set,
the first HTTP(S) request will fail and user will be asked for
credentials. As a side effect of first HTTP(S) request, libcurl auth
method GSS-Negotiate will be disabled unconditionally. Although, we
haven't tried yet provided credentials for this auth method.

Signed-off-by: Dmitry Vilkov <dmitry.a.vilkov@gmail.com>
---
 http.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 0da9e66..707ea84 100644
--- a/http.c
+++ b/http.c
@@ -951,12 +951,15 @@ static int handle_curl_result(struct slot_results *results)
 		return HTTP_MISSING_TARGET;
 	else if (results->http_code == 401) {
 		if (http_auth.username && http_auth.password) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+			if (http_auth_methods & CURLAUTH_GSSNEGOTIATE) {
+				http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+				return HTTP_REAUTH;
+			}
+#endif
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-#endif
 			return HTTP_REAUTH;
 		}
 	} else {
-- 
2.4.10

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Junio C Hamano]

Dmitry Vilkov <dmitry.a.vilkov@gmail.com> writes:

> This is fix of bug introduced by 4dbe66464 commit.

That would be 4dbe6646 (remote-curl: fall back to Basic auth if
Negotiate fails, 2015-01-08) that appears in v2.3.1 and onward.

> The problem is that when username/password combination was not set,
> the first HTTP(S) request will fail and user will be asked for
> credentials. As a side effect of first HTTP(S) request, libcurl auth
> method GSS-Negotiate will be disabled unconditionally. Although, we
> haven't tried yet provided credentials for this auth method.

Brian, comments?  Here is what you wrote in that commit:

    If Basic and something else are offered, libcurl will never
    attempt to use Basic, even if the other option fails.  Teach the
    HTTP client code to stop trying authentication mechanisms that
    don't use a password (currently Negotiate) after the first
    failure, since if they failed the first time, they will never
    succeed.

Thanks.

> Signed-off-by: Dmitry Vilkov <dmitry.a.vilkov@gmail.com>
> ---
>  http.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/http.c b/http.c
> index 0da9e66..707ea84 100644
> --- a/http.c
> +++ b/http.c
> @@ -951,12 +951,15 @@ static int handle_curl_result(struct slot_results *results)
>  		return HTTP_MISSING_TARGET;
>  	else if (results->http_code == 401) {
>  		if (http_auth.username && http_auth.password) {
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +			if (http_auth_methods & CURLAUTH_GSSNEGOTIATE) {
> +				http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
> +				return HTTP_REAUTH;
> +			}
> +#endif
>  			credential_reject(&http_auth);
>  			return HTTP_NOAUTH;
>  		} else {
> -#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> -			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
> -#endif
>  			return HTTP_REAUTH;
>  		}
>  	} else {

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

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

On Tue, Feb 02, 2016 at 12:37:19PM -0800, Junio C Hamano wrote:
> Dmitry Vilkov <dmitry.a.vilkov@gmail.com> writes:
> 
> > This is fix of bug introduced by 4dbe66464 commit.
> 
> That would be 4dbe6646 (remote-curl: fall back to Basic auth if
> Negotiate fails, 2015-01-08) that appears in v2.3.1 and onward.
> 
> > The problem is that when username/password combination was not set,
> > the first HTTP(S) request will fail and user will be asked for
> > credentials. As a side effect of first HTTP(S) request, libcurl auth
> > method GSS-Negotiate will be disabled unconditionally. Although, we
> > haven't tried yet provided credentials for this auth method.

I'm unclear in what case you'd need to have a username and password
combination with GSS-Negotiate.  Kerberos doesn't use your password,
although you need some indication of a username (valid or not) to get
libcurl to do authentication.

Are you basically using a bare URL (without a username component) and
waiting for git to prompt you for the username, so that it will then
enable authentication?  If so, this patch looks fine for that, although
I'd expand on the commit message.  If not, could you provide an example
of what you're trying to do?

> Brian, comments?  Here is what you wrote in that commit:
> 
>     If Basic and something else are offered, libcurl will never
>     attempt to use Basic, even if the other option fails.  Teach the
>     HTTP client code to stop trying authentication mechanisms that
>     don't use a password (currently Negotiate) after the first
>     failure, since if they failed the first time, they will never
>     succeed.

I think what's happening here is no username is ever provided, so
libcurl never tries authentication in the first place.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Dmitry Vilkov]

2016-02-03 2:29 GMT+03:00 brian m. carlson <sandals@crustytoothpaste.net>:
> I'm unclear in what case you'd need to have a username and password
> combination with GSS-Negotiate.  Kerberos doesn't use your password,
> although you need some indication of a username (valid or not) to get
> libcurl to do authentication.
>
> Are you basically using a bare URL (without a username component) and
> waiting for git to prompt you for the username, so that it will then
> enable authentication?  If so, this patch looks fine for that, although
> I'd expand on the commit message.  If not, could you provide an example
> of what you're trying to do?

You are right, we are using a bare URL (without a username component).
With username encoded in URL everything works just fine. But it's
generally wrong to pass creds in URL (in my opinion) and security
policy of my employer prohibits doing such thing.
Anyway, as you said libcurl needs valid (not NULL) username/password
to do GSS-Negotiate, so there is nothing wrong if I set empty
username/password combination when git prompts for creds. Even more,
there is no other way to let libcurl to use GSS-Negotiate without
username in URL.

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Junio C Hamano]

Dmitry Vilkov <dmitry.a.vilkov@gmail.com> writes:

> 2016-02-03 2:29 GMT+03:00 brian m. carlson <sandals@crustytoothpaste.net>:
>> I'm unclear in what case you'd need to have a username and password
>> combination with GSS-Negotiate.  Kerberos doesn't use your password,
>> although you need some indication of a username (valid or not) to get
>> libcurl to do authentication.
>>
>> Are you basically using a bare URL (without a username component) and
>> waiting for git to prompt you for the username, so that it will then
>> enable authentication?  If so, this patch looks fine for that, although
>> I'd expand on the commit message.  If not, could you provide an example
>> of what you're trying to do?
>
> You are right, we are using a bare URL (without a username component).
> With username encoded in URL everything works just fine. But it's
> generally wrong to pass creds in URL (in my opinion) and security
> policy of my employer prohibits doing such thing.
>
> Anyway, as you said libcurl needs valid (not NULL) username/password
> to do GSS-Negotiate, so there is nothing wrong if I set empty
> username/password combination when git prompts for creds. 

OK, as Brian said, that use case would need to be in the log
message, at least.  I am curious, though, if you can give just a
random string to username, or at least that must match what the
underlying authentication mechanism uses.

Brian, I can see how this would work in that use case, but I haven't
convinced myself that the change would not affect other existing use
cases that are supported--do you think of any that would negatively
affect the user expeerience?

> Even more,
> there is no other way to let libcurl to use GSS-Negotiate without
> username in URL.

Asking lubcurl expert about that might not be a bad idea; Cc'ed
Daniel Stenberg.

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

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

On Fri, Feb 05, 2016 at 12:18:22PM +0300, Dmitry Vilkov wrote:
> You are right, we are using a bare URL (without a username component).
> With username encoded in URL everything works just fine. But it's
> generally wrong to pass creds in URL (in my opinion) and security
> policy of my employer prohibits doing such thing.
> Anyway, as you said libcurl needs valid (not NULL) username/password
> to do GSS-Negotiate, so there is nothing wrong if I set empty
> username/password combination when git prompts for creds. Even more,
> there is no other way to let libcurl to use GSS-Negotiate without
> username in URL.

You can literally do https://:@git.crustytoothpaste.net/git/repo.git as
the URL, and that will work.  GSS-Negotiate using Kerberos passes the
ticket, which contains the principal name in it, so an actual username
and password is not needed at all.  libcurl just needs something to tell
it to do authentication.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

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

On Fri, Feb 05, 2016 at 09:54:50AM -0800, Junio C Hamano wrote:
> OK, as Brian said, that use case would need to be in the log
> message, at least.  I am curious, though, if you can give just a
> random string to username, or at least that must match what the
> underlying authentication mechanism uses.

You can give any invalid credentials you like.  When using Kerberos, the
provided username and password are ignored, because all the
authentication information is in the ticket, and it's all encrypted.

I'm happy to send a documentation patch for this, as it seems to come up
a lot.

> Brian, I can see how this would work in that use case, but I haven't
> convinced myself that the change would not affect other existing use
> cases that are supported--do you think of any that would negatively
> affect the user expeerience?

I'd have to test how it works with Basic auth as a fallback.  I don't
normally use that on my servers, so I'd have to enable it and try it
out.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Junio C Hamano]

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

> On Fri, Feb 05, 2016 at 12:18:22PM +0300, Dmitry Vilkov wrote:
>> You are right, we are using a bare URL (without a username component).
>> With username encoded in URL everything works just fine. But it's
>> generally wrong to pass creds in URL (in my opinion) and security
>> policy of my employer prohibits doing such thing.
>> Anyway, as you said libcurl needs valid (not NULL) username/password
>> to do GSS-Negotiate, so there is nothing wrong if I set empty
>> username/password combination when git prompts for creds. Even more,
>> there is no other way to let libcurl to use GSS-Negotiate without
>> username in URL.
>
> You can literally do https://:@git.crustytoothpaste.net/git/repo.git as
> the URL, and that will work.  GSS-Negotiate using Kerberos passes the
> ticket, which contains the principal name in it, so an actual username
> and password is not needed at all.  libcurl just needs something to tell
> it to do authentication.

Hmph, so documenting that <emptyname>:<emptypassword>@<repository>
as a supported way might be an ugly-looking solution to the original
problem.  A less ugly-looking solution might be a boolean that can
be set per URL (we already have urlmatch-config infrastructure to
help us do so) to tell us to pass the empty credential to lubCurl,
bypassing the step to ask the user for password that we do not use.

The end-result of either of these solution would strictly be better
than the patch we discussed in that the end user will not have to
interact with the prompt at all, right?

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

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

On Fri, Feb 05, 2016 at 01:02:58PM -0800, Junio C Hamano wrote:
> Hmph, so documenting that <emptyname>:<emptypassword>@<repository>
> as a supported way might be an ugly-looking solution to the original
> problem.  A less ugly-looking solution might be a boolean that can
> be set per URL (we already have urlmatch-config infrastructure to
> help us do so) to tell us to pass the empty credential to lubCurl,
> bypassing the step to ask the user for password that we do not use.
> 
> The end-result of either of these solution would strictly be better
> than the patch we discussed in that the end user will not have to
> interact with the prompt at all, right?

Yes, that's true.  I'll try to come up with a patch this weekend that
implements that (maybe remote.forceAuth = true or somesuch).
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Junio C Hamano]

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

> On Fri, Feb 05, 2016 at 01:02:58PM -0800, Junio C Hamano wrote:
>> Hmph, so documenting that <emptyname>:<emptypassword>@<repository>
>> as a supported way might be an ugly-looking solution to the original
>> problem.  A less ugly-looking solution might be a boolean that can
>> be set per URL (we already have urlmatch-config infrastructure to
>> help us do so) to tell us to pass the empty credential to lubCurl,
>> bypassing the step to ask the user for password that we do not use.
>> 
>> The end-result of either of these solution would strictly be better
>> than the patch we discussed in that the end user will not have to
>> interact with the prompt at all, right?
>
> Yes, that's true.  I'll try to come up with a patch this weekend that
> implements that (maybe remote.forceAuth = true or somesuch).

Thanks.

I think the configuration should live inside http.* namespace, as
there are already things like http[.<url>].sslCert and friends.

I do not have a good suggestion on the name of the leaf-level
variable.  ForceAuth sounds as if you are forcing authentication
even when the other side does not require it, though.

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Daniel Stenberg]

On Fri, 5 Feb 2016, Junio C Hamano wrote:

> OK, as Brian said, that use case would need to be in the log message, at 
> least.  I am curious, though, if you can give just a random string to 
> username, or at least that must match what the underlying authentication 
> mechanism uses.
>
> Brian, I can see how this would work in that use case, but I haven't 
> convinced myself that the change would not affect other existing use cases 
> that are supported--do you think of any that would negatively affect the 
> user expeerience?
>
>> Even more, there is no other way to let libcurl to use GSS-Negotiate 
>> without username in URL.
>
> Asking lubcurl expert about that might not be a bad idea; Cc'ed Daniel 
> Stenberg.

It is correct that libcurl needs a username to trigger the use of HTTP 
authentication - any HTTP authentication - due to how we once designed the 
internals for this - but when using GSS-Negotiate the actually provided user 
name isn't used by libcurl for anything so it could be a fixed string or 
random junk, it doesn't matter as long as a name is provided.

-- 

  / daniel.haxx.se

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Dmitry Vilkov]

2016-02-06 0:52 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> On Fri, Feb 05, 2016 at 01:02:58PM -0800, Junio C Hamano wrote:
>>> Hmph, so documenting that <emptyname>:<emptypassword>@<repository>
>>> as a supported way might be an ugly-looking solution to the original
>>> problem.  A less ugly-looking solution might be a boolean that can
>>> be set per URL (we already have urlmatch-config infrastructure to
>>> help us do so) to tell us to pass the empty credential to lubCurl,
>>> bypassing the step to ask the user for password that we do not use.
>>>
>>> The end-result of either of these solution would strictly be better
>>> than the patch we discussed in that the end user will not have to
>>> interact with the prompt at all, right?
>>
>> Yes, that's true.  I'll try to come up with a patch this weekend that
>> implements that (maybe remote.forceAuth = true or somesuch).
>
> Thanks.
>
> I think the configuration should live inside http.* namespace, as
> there are already things like http[.<url>].sslCert and friends.
>
> I do not have a good suggestion on the name of the leaf-level
> variable.  ForceAuth sounds as if you are forcing authentication
> even when the other side does not require it, though.

That would be great! Definitely it will be much better solution than
patch I've proposed.

[PATCH] http: add option to try authentication without username

[brian m. carlson]

Performing GSS-Negotiate authentication using Kerberos does not require
specifying a username or password, since that information is already
included in the ticket itself.  However, libcurl refuses to perform
authentication if it has not been provided with a username and password.
Add an option, http.emptyAuth, that provides libcurl with an empty
username and password to make it attempt authentication anyway.
---
I'm not super excited about this name, but I couldn't think of a better
one.  Suggestions welcome.

 Documentation/config.txt |  6 ++++++
 http.c                   | 13 +++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27f02be3..f11de77e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1648,6 +1648,12 @@ http.proxyAuthMethod::
 * `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
 --
 
+http.emptyAuth::
+	Attempt authentication without seeking a username or password.  This
+	can be used to attempt GSS-Negotiate authentication without specifying
+	a username in the URL, as libcurl normally requires a username for
+	authentication.
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
diff --git a/http.c b/http.c
index dfc53c1e..a12a804b 100644
--- a/http.c
+++ b/http.c
@@ -87,6 +87,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
+static int curl_empty_auth;
 
 #if LIBCURL_VERSION_NUM >= 0x071700
 /* Use CURLOPT_KEYPASSWD as is */
@@ -299,14 +300,22 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.useragent", var))
 		return git_config_string(&user_agent, var, value);
 
+	if (!strcmp("http.emptyauth", var)) {
+		curl_empty_auth = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
 
 static void init_curl_http_auth(CURL *result)
 {
-	if (!http_auth.username)
+	if (!http_auth.username) {
+		if (curl_empty_auth)
+			curl_easy_setopt(result, CURLOPT_USERPWD, ":");
 		return;
+	}
 
 	credential_fill(&http_auth);
 
@@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-	if (http_auth.password)
+	if (http_auth.password || curl_empty_auth)
 		init_curl_http_auth(slot->curl);
 
 	return slot;

Re: [PATCH] http: add option to try authentication without username

[Eric Sunshine]

On Mon, Feb 15, 2016 at 1:44 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Performing GSS-Negotiate authentication using Kerberos does not require
> specifying a username or password, since that information is already
> included in the ticket itself.  However, libcurl refuses to perform
> authentication if it has not been provided with a username and password.
> Add an option, http.emptyAuth, that provides libcurl with an empty
> username and password to make it attempt authentication anyway.

I'm not familiar with this code, so let me know if my comments (below)
are off the mark...

> ---
> diff --git a/http.c b/http.c
> +++ b/http.c
> @@ -299,14 +300,22 @@ static int http_options(const char *var, const char *value, void *cb)
>  static void init_curl_http_auth(CURL *result)
>  {
> -       if (!http_auth.username)
> +       if (!http_auth.username) {
> +               if (curl_empty_auth)
> +                       curl_easy_setopt(result, CURLOPT_USERPWD, ":");

Does this need to take LIBCURL_VERSION_NUM into account? Other code
which uses CURLOPT_USERPWD only does so for certain versions of
libcurl, otherwise CURLOPT_USERNAME and CURLOPT_PASSWORD is used.

>                 return;
> +       }
>
>         credential_fill(&http_auth);
>
> @@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
>  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
>         curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
>  #endif
> -       if (http_auth.password)
> +       if (http_auth.password || curl_empty_auth)
>                 init_curl_http_auth(slot->curl);
>
>         return slot;

Rather than sprinkling curl_empty_auth special cases here and there,
would it be possible to simply set http_auth.username and
http_auth.password to empty strings early on if they are not already
set and curl_empty_auth is true, and then let the:

    strbuf_addf(&up, "%s:%s",
        http_auth.username, http_auth.password);

in init_curl_http_auth() handle them in the normal fashion, with the
end result being the same ":" set explicitly by this patch?

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

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

On Mon, Feb 15, 2016 at 03:19:25PM -0500, Eric Sunshine wrote:
> On Mon, Feb 15, 2016 at 1:44 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > Performing GSS-Negotiate authentication using Kerberos does not require
> > specifying a username or password, since that information is already
> > included in the ticket itself.  However, libcurl refuses to perform
> > authentication if it has not been provided with a username and password.
> > Add an option, http.emptyAuth, that provides libcurl with an empty
> > username and password to make it attempt authentication anyway.
> 
> I'm not familiar with this code, so let me know if my comments (below)
> are off the mark...
> 
> > ---
> > diff --git a/http.c b/http.c
> > +++ b/http.c
> > @@ -299,14 +300,22 @@ static int http_options(const char *var, const char *value, void *cb)
> >  static void init_curl_http_auth(CURL *result)
> >  {
> > -       if (!http_auth.username)
> > +       if (!http_auth.username) {
> > +               if (curl_empty_auth)
> > +                       curl_easy_setopt(result, CURLOPT_USERPWD, ":");
> 
> Does this need to take LIBCURL_VERSION_NUM into account? Other code
> which uses CURLOPT_USERPWD only does so for certain versions of
> libcurl, otherwise CURLOPT_USERNAME and CURLOPT_PASSWORD is used.

This is the oldest version, which means it's the most compatible.  Since
we don't need to consider odd usernames, it seemed silly to have both
cases present in the code.  The benefit of using the pair of options is
that we don't leak memory in the non-empty auth case.

> >                 return;
> > +       }
> >
> >         credential_fill(&http_auth);
> >
> > @@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
> >  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> >         curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
> >  #endif
> > -       if (http_auth.password)
> > +       if (http_auth.password || curl_empty_auth)
> >                 init_curl_http_auth(slot->curl);
> >
> >         return slot;
> 
> Rather than sprinkling curl_empty_auth special cases here and there,
> would it be possible to simply set http_auth.username and
> http_auth.password to empty strings early on if they are not already
> set and curl_empty_auth is true, and then let the:
> 
>     strbuf_addf(&up, "%s:%s",
>         http_auth.username, http_auth.password);
> 
> in init_curl_http_auth() handle them in the normal fashion, with the
> end result being the same ":" set explicitly by this patch?

That would work.  I was concerned about the credential_fill call
actually prompting the user, but it appears that it doesn't do that if
the password already exists.  I don't know if we want to rely on that
functionality, though.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] http: add option to try authentication without username

[Jeff King]

On Mon, Feb 15, 2016 at 08:29:37PM +0000, brian m. carlson wrote:

> > Rather than sprinkling curl_empty_auth special cases here and there,
> > would it be possible to simply set http_auth.username and
> > http_auth.password to empty strings early on if they are not already
> > set and curl_empty_auth is true, and then let the:
> > 
> >     strbuf_addf(&up, "%s:%s",
> >         http_auth.username, http_auth.password);
> > 
> > in init_curl_http_auth() handle them in the normal fashion, with the
> > end result being the same ":" set explicitly by this patch?
> 
> That would work.  I was concerned about the credential_fill call
> actually prompting the user, but it appears that it doesn't do that if
> the password already exists.  I don't know if we want to rely on that
> functionality, though.

Yeah, credential_fill() will treat that as a noop, as it is no different
than getting "https://user:pass@example.com" in the URL in the first
place. But it will _also_ send the result to credential_approve() and
credential_reject(), which you probably don't want (because you do not
want to store these useless dummy credentials in your keystore).

So I think this hack should remain purely at the curl level, and never
touch the credential struct at all.

Which is a shame, because I think Eric's suggestion is otherwise much
more readable. :)

-Peff

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

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

On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> On Mon, Feb 15, 2016 at 08:29:37PM +0000, brian m. carlson wrote:
> > That would work.  I was concerned about the credential_fill call
> > actually prompting the user, but it appears that it doesn't do that if
> > the password already exists.  I don't know if we want to rely on that
> > functionality, though.
> 
> Yeah, credential_fill() will treat that as a noop, as it is no different
> than getting "https://user:pass@example.com" in the URL in the first
> place. But it will _also_ send the result to credential_approve() and
> credential_reject(), which you probably don't want (because you do not
> want to store these useless dummy credentials in your keystore).

Correct.

> So I think this hack should remain purely at the curl level, and never
> touch the credential struct at all.
> 
> Which is a shame, because I think Eric's suggestion is otherwise much
> more readable. :)

Yes, I agree.  That would have been a much nicer and smaller change.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] http: add option to try authentication without username

[Junio C Hamano]

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

> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> ...
>> So I think this hack should remain purely at the curl level, and never
>> touch the credential struct at all.
>> 
>> Which is a shame, because I think Eric's suggestion is otherwise much
>> more readable. :)
>
> Yes, I agree.  That would have been a much nicer and smaller change.

Alright, reading all reviews and taking them into account, the
original, when a Sign-off is added, would be acceptable, it seems.

Thanks.

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

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

On Mon, Feb 15, 2016 at 01:39:30PM -0800, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> > ...
> >> So I think this hack should remain purely at the curl level, and never
> >> touch the credential struct at all.
> >> 
> >> Which is a shame, because I think Eric's suggestion is otherwise much
> >> more readable. :)
> >
> > Yes, I agree.  That would have been a much nicer and smaller change.
> 
> Alright, reading all reviews and taking them into account, the
> original, when a Sign-off is added, would be acceptable, it seems.

Sorry about that.  Please add my

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

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

Re: [PATCH] http: add option to try authentication without username

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
>>> So I think this hack should remain purely at the curl level, and never
>>> touch the credential struct at all.
>>>
>>> Which is a shame, because I think Eric's suggestion is otherwise much
>>> more readable. :)
>>
>> Yes, I agree.  That would have been a much nicer and smaller change.
>
> Alright, reading all reviews and taking them into account, the
> original, when a Sign-off is added, would be acceptable, it seems.

One final question: Keeping in mind my lack of familiarity with this
particular use-case, would it be possible to infer the need to employ
this curl-specific workaround rather than making users tweak a config
setting? Or would that be a security risk or an otherwise stupid idea?

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

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

On Mon, Feb 15, 2016 at 04:46:43PM -0500, Eric Sunshine wrote:
> On Mon, Feb 15, 2016 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> >>> So I think this hack should remain purely at the curl level, and never
> >>> touch the credential struct at all.
> >>>
> >>> Which is a shame, because I think Eric's suggestion is otherwise much
> >>> more readable. :)
> >>
> >> Yes, I agree.  That would have been a much nicer and smaller change.
> >
> > Alright, reading all reviews and taking them into account, the
> > original, when a Sign-off is added, would be acceptable, it seems.
> 
> One final question: Keeping in mind my lack of familiarity with this
> particular use-case, would it be possible to infer the need to employ
> this curl-specific workaround rather than making users tweak a config
> setting? Or would that be a security risk or an otherwise stupid idea?

It's not very easy to infer whether it's needed.  We'd need to know what
types of authentication are offered, and somehow we'd have to intuit
proper behavior when both GSS-Negotiate and Basic are enabled.  Some
sites do that because you can use Basic against the Kerberos database.
One user might legitimately want to always use Basic (e.g. with a
password manager) and another might always want to use Negotiate.
Setting this option is one way to ensure the latter.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Dmitry Vilkov]

Hi guys! Any luck with fixing this issue?

I would like to draw your attention to the fact that Git starting from
version 2.3.1 is unusable with servers that support GSS-Negotiation
(e.g. Microsoft TFS).

Sorry, english is not my native language and probably I was not clear
enough when described the problem at first time.

So, let me try again. Algorithm of fetching data through HTTP(S)
protocol is as follows:
1. Try to fetch data without any credentials.
2. If first step failed
  2.1. Disable GSS-Negotiation algorithm (this is the side effect
which is fixed by my patch).
  2.2. Read credentials from config files.
  2.3. If there is no credentials then ask user for it.
  2.4. Go to step one, but now try to fetch data with found/provided
credentials.

As you can see there is no other way to connect to server with
GSS-Negotiation auth than use URLs like
"https://:@my-gss-git-server.com" (because there is step zero where we
are parsing URL which can contain credentials that we can use in step
one).

Maybe you could accept my patch, so users would use
"credential.helper=store" to avoid using ":@" in remote URL? At least
for now, while there is no good solution to this issue? It would be
very helpful because now we have to have our own version of patched
Git :(

Thanks in advance.


2016-02-08 12:11 GMT+03:00 Dmitry Vilkov <dmitry.a.vilkov@gmail.com>:
> 2016-02-06 0:52 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>>> On Fri, Feb 05, 2016 at 01:02:58PM -0800, Junio C Hamano wrote:
>>>> Hmph, so documenting that <emptyname>:<emptypassword>@<repository>
>>>> as a supported way might be an ugly-looking solution to the original
>>>> problem.  A less ugly-looking solution might be a boolean that can
>>>> be set per URL (we already have urlmatch-config infrastructure to
>>>> help us do so) to tell us to pass the empty credential to lubCurl,
>>>> bypassing the step to ask the user for password that we do not use.
>>>>
>>>> The end-result of either of these solution would strictly be better
>>>> than the patch we discussed in that the end user will not have to
>>>> interact with the prompt at all, right?
>>>
>>> Yes, that's true.  I'll try to come up with a patch this weekend that
>>> implements that (maybe remote.forceAuth = true or somesuch).
>>
>> Thanks.
>>
>> I think the configuration should live inside http.* namespace, as
>> there are already things like http[.<url>].sslCert and friends.
>>
>> I do not have a good suggestion on the name of the leaf-level
>> variable.  ForceAuth sounds as if you are forcing authentication
>> even when the other side does not require it, though.
>
> That would be great! Definitely it will be much better solution than
> patch I've proposed.

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

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

On Sat, Feb 20, 2016 at 05:35:19PM +0300, Dmitry Vilkov wrote:
> Maybe you could accept my patch, so users would use
> "credential.helper=store" to avoid using ":@" in remote URL? At least
> for now, while there is no good solution to this issue? It would be
> very helpful because now we have to have our own version of patched
> Git :(

I sent in a patch (and I believe I CC'd you) that adds an option
http.emptyAuth that can be used in this case.  It should make its way to
a future release.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

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

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Junio C Hamano]

Dmitry Vilkov <dmitry.a.vilkov@gmail.com> writes:

> Hi guys! Any luck with fixing this issue?

I think Brian suggested an alternative approach, to which you earler
responded

>> That would be great! Definitely it will be much better solution than
>> patch I've proposed.

The patch has been queued as 121061f6 (http: add option to try
authentication without username, 2016-02-15); perhaps you can help
by trying it out in your installation before it hits a released
version of Git?  That way, if the patch does not fix your problem,
or it introduces an unexpected side effect, we would notice before
we include it in a future release.

Thanks.

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Dmitry Vilkov]

Hi, guys!

> I sent in a patch (and I believe I CC'd you) that adds an option
> http.emptyAuth that can be used in this case.  It should make its way to
> a future release.

Somehow I've missed your letter...

> The patch has been queued as 121061f6 (http: add option to try
> authentication without username, 2016-02-15); perhaps you can help
> by trying it out in your installation before it hits a released
> version of Git?  That way, if the patch does not fix your problem,
> or it introduces an unexpected side effect, we would notice before
> we include it in a future release.

I've cherry-picked commit 121061f6 over version 2.4.10 and 2.7.1.
In both cases it works exactly as expected.

Please, let me know if I can help with something else regarding this issue.

2016-02-21 0:38 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Dmitry Vilkov <dmitry.a.vilkov@gmail.com> writes:
>
>> Hi guys! Any luck with fixing this issue?
>
> I think Brian suggested an alternative approach, to which you earler
> responded
>
>>> That would be great! Definitely it will be much better solution than
>>> patch I've proposed.
>
> The patch has been queued as 121061f6 (http: add option to try
> authentication without username, 2016-02-15); perhaps you can help
> by trying it out in your installation before it hits a released
> version of Git?  That way, if the patch does not fix your problem,
> or it introduces an unexpected side effect, we would notice before
> we include it in a future release.
>
> Thanks.
>