[PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[David Turner]

When using kerberos authentication, one URL pattern which is
allowed is http://@gitserver.example.com.  This leads to a username
of zero-length, rather than a NULL username.  But the two cases
should be treated the same by http.emptyauth.

Signed-off-by: David Turner <dturner@twosigma.com>
---
 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 82ed542..bd0dba2 100644
--- a/http.c
+++ b/http.c
@@ -351,7 +351,7 @@ 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 || !*http_auth.username) {
 		if (curl_empty_auth)
 			curl_easy_setopt(result, CURLOPT_USERPWD, ":");
 		return;
-- 
2.8.0.rc4.22.g8ae061a

Re: [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[Jeff King]

On Mon, Oct 03, 2016 at 01:19:28PM -0400, David Turner wrote:

> When using kerberos authentication, one URL pattern which is
> allowed is http://@gitserver.example.com.  This leads to a username
> of zero-length, rather than a NULL username.  But the two cases
> should be treated the same by http.emptyauth.
> 
> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
>  http.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/http.c b/http.c
> index 82ed542..bd0dba2 100644
> --- a/http.c
> +++ b/http.c
> @@ -351,7 +351,7 @@ 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 || !*http_auth.username) {

Hmm. This fixes this caller, but what about other users of the
credential struct? I wonder if the correct fix is in
credential_from_url(), which should avoid writing an empty
field.

OTOH, I can imagine that "http://user:@example.com" would be a way to
say "I have a username and the password is blank" without getting
prompted.  Which makes me wonder if it is useful to say "my username is
blank" in the same way.

I dunno. The code path you are changing _only_ affects anything
if the http.emptyauth config is set. But I guess I just don't understand
why you would say "http://@gitserver" in the first place. Is that a
common thing?

-Peff

RE: [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[David Turner]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, October 03, 2016 5:01 PM
> To: David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net
> Subject: Re: [PATCH] http: http.emptyauth should allow empty (not just
> NULL) usernames
> 
> On Mon, Oct 03, 2016 at 01:19:28PM -0400, David Turner wrote:
> 
> > When using kerberos authentication, one URL pattern which is allowed
> > is http://@gitserver.example.com.  This leads to a username of
> > zero-length, rather than a NULL username.  But the two cases should be
> > treated the same by http.emptyauth.
> >
> > Signed-off-by: David Turner <dturner@twosigma.com>
> > ---
> >  http.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/http.c b/http.c
> > index 82ed542..bd0dba2 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -351,7 +351,7 @@ 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 || !*http_auth.username) {
> 
> Hmm. This fixes this caller, but what about other users of the credential
> struct? I wonder if the correct fix is in credential_from_url(), which
> should avoid writing an empty field.
> 
> OTOH, I can imagine that "http://user:@example.com" would be a way to say
> "I have a username and the password is blank" without getting prompted.
> Which makes me wonder if it is useful to say "my username is blank" in the
> same way.

Yes, that was my thought process.

> I dunno. The code path you are changing _only_ affects anything if the
> http.emptyauth config is set. But I guess I just don't understand why you
> would say "http://@gitserver" in the first place. Is that a common thing?
> 
> -Peff

I have no idea if it is common.  I know that we do it.

It used to be that git 2.8/libcurl would handle @gitserver as if the username were blank, but then we upgraded our company's libcurl and it broke (git started prompting for a password). I do not know what the previous version of libcurl was.

The reason we have a required-to-be-blank username/password is apparently Kerberos (or something about our particular Kerberos configuration), which I treat as inscrutable black magic.

Re: [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[Jeff King]

On Mon, Oct 03, 2016 at 09:54:19PM +0000, David Turner wrote:

> > I dunno. The code path you are changing _only_ affects anything if the
> > http.emptyauth config is set. But I guess I just don't understand why you
> > would say "http://@gitserver" in the first place. Is that a common thing?
> 
> I have no idea if it is common.  I know that we do it.

I guess my question is: _why_ do you do it? Or more specifically, does
http://gitserver.example.com" with http.emptyauth not work, and why?

From your response, I _think_ the answer is "no, it doesn't, and I have
no clue why".

So I dunno. It is annoying not to know what is actually going on, but
I'm OK with it if we don't think there's a high chance of regressing any
other workflows (which I guess not, because http.emptyauth seems to be a
Kerberos-specific hack in the first place).

-Peff

RE: [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[David Turner]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, October 03, 2016 5:59 PM
> To: David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net
> Subject: Re: [PATCH] http: http.emptyauth should allow empty (not just
> NULL) usernames
> 
> On Mon, Oct 03, 2016 at 09:54:19PM +0000, David Turner wrote:
> 
> > > I dunno. The code path you are changing _only_ affects anything if
> > > the http.emptyauth config is set. But I guess I just don't
> > > understand why you would say "http://@gitserver" in the first place.
> Is that a common thing?
> >
> > I have no idea if it is common.  I know that we do it.
> 
> I guess my question is: _why_ do you do it? Or more specifically, does
> http://gitserver.example.com" with http.emptyauth not work, and why?
> 
> From your response, I _think_ the answer is "no, it doesn't, and I have no
> clue why".

That was true historically. 

I just tried our old version of git 2.8 (that is, before this patch, and before the libcurl upgrade), and http://gitserver.example.com *does* seem to work with http.emptyauth (and does not work without).  However, http://@gitserver.example.com does *not* work with http.emptyauth, and *does* work without.

After the libcurl upgrade, but before this patch, http://@gitserver.example.com does *not* work with http.emptyauth, while http://gitserver.example.com does.

And finally, after the upgrade and with this patch, both urls work.

> So I dunno. It is annoying not to know what is actually going on, but I'm
> OK with it if we don't think there's a high chance of regressing any other
> workflows (which I guess not, because http.emptyauth seems to be a
> Kerberos-specific hack in the first place).

Yes, I think this is all Kerberos-only.

Re: [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[Junio C Hamano]

David Turner <David.Turner@twosigma.com> writes:

>> > > I dunno. The code path you are changing _only_ affects anything if
>> > > the http.emptyauth config is set. But I guess I just don't
>> > > understand why you would say "http://@gitserver" in the first place.
>> Is that a common thing?
>> >
>> > I have no idea if it is common.  I know that we do it.
>> 
>> I guess my question is: _why_ do you do it? Or more specifically, does
>> http://gitserver.example.com" with http.emptyauth not work, and why?
>> 
>> From your response, I _think_ the answer is "no, it doesn't, and I have no
>> clue why".
>
> That was true historically. 
>
> I just tried our old version of git 2.8 (that is, before this patch, and before the libcurl upgrade), and http://gitserver.example.com *does* seem to work with http.emptyauth (and does not work without).  However, http://@gitserver.example.com does *not* work with http.emptyauth, and *does* work without.
>
> After the libcurl upgrade, but before this patch, http://@gitserver.example.com does *not* work with http.emptyauth, while http://gitserver.example.com does.
>
> And finally, after the upgrade and with this patch, both urls work.
>
>> So I dunno. It is annoying not to know what is actually going on, but I'm
>> OK with it if we don't think there's a high chance of regressing any other
>> workflows (which I guess not, because http.emptyauth seems to be a
>> Kerberos-specific hack in the first place).
>
> Yes, I think this is all Kerberos-only.

Now, perhaps with these back-and-forth, hopefully you have enough
material to update the proposed log message to clarify so that next
Peff won't have to ask "would it be common?  why would you do so?"

Thanks.

Re: [PATCH] http: http.emptyauth should allow empty (not just NULL) usernames

[brian m. carlson]

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

On Mon, Oct 03, 2016 at 09:54:19PM +0000, David Turner wrote:
> 
> > I dunno. The code path you are changing _only_ affects anything if the
> > http.emptyauth config is set. But I guess I just don't understand why you
> > would say "http://@gitserver" in the first place. Is that a common thing?
> > 
> > -Peff
> 
> I have no idea if it is common.  I know that we do it.

I've never seen this.  RFC 3986 does seem to allow it:

  authority   = [ userinfo "@" ] host [ ":" port ]
  userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )

I normally write it like one of these:

  https://bmc@git.crustytoothpaste.net/
  https://:@git.crustytoothpaste.net/

Of course, the username is ignored in the first one, but it serves a
documentary purpose for me.

> The reason we have a required-to-be-blank username/password is
> apparently Kerberos (or something about our particular Kerberos
> configuration), which I treat as inscrutable black magic.

The issue with git is usually that it uses libcurl, which won't do
authentication unless it has a username or password, even if those are
empty or ignored.  http.emptyAuth was designed for this case.

With Kerberos (at least in my experience), the username doesn't actually
get sent, since you send only ticket-related information over the
channel, and that has your principal name embedded.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

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