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

[brian m. carlson]

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  wrote:
> > "brian m. carlson"  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


signature.asc
Description: PGP signature

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

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:39 PM, Junio C Hamano  wrote:
> "brian m. carlson"  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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[brian m. carlson]

On Mon, Feb 15, 2016 at 01:39:30PM -0800, Junio C Hamano wrote:
> "brian m. carlson"  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 
-- 
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


signature.asc
Description: PGP signature

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

[Junio C Hamano]

"brian m. carlson"  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[brian m. carlson]

On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> On Mon, Feb 15, 2016 at 08:29:37PM +, 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:p...@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


signature.asc
Description: PGP signature

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

[Jeff King]

On Mon, Feb 15, 2016 at 08:29:37PM +, 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(, "%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:p...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[brian m. carlson]

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
>  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(_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(, "%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


signature.asc
Description: PGP signature

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

[Eric Sunshine]

On Mon, Feb 15, 2016 at 1:44 PM, brian m. carlson
 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(_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(, "%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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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(_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(_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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html