Git ksshaskpass to play nice with https and kwallet

Git ksshaskpass to play nice with https and kwallet

[Michael J Gruber]

Checking out bitbucket and https, I came across a stupid issue between
the default KDE setup (SSH_ASKPASS=/usr/bin/ksshakspass and KDE wallet)
and the way we call the askpass helper, which makes the combo not work
out of the box.

For those interested in getting it to work (or learning why it does not
work out of the box), you can read the details formatted on my sparse
blog (brain dump)

http://grubix.blogspot.com/2011/10/git-ksshaskpass-to-play-nice-with-https.html

or below if you don't feel like clicking through.

Cheers
Michael

By default (with GIT_ASKPASS not set etc.), Git invokes $SSH_ASKPASS
when it needs to ask you for credential info. In a KDE environment,
SSH_ASKPASS is set to /usr/bin/ksshaskpass. So far so good.

But Git calls the askpass helper with a command line like
/usr/bin/ksshaskpass Username for 'bitbucket.org':
and once again with
/usr/bin/ksshaskpass Password for 'bitbucket.org':
So far so good.

But when asked to store the credentials in the KDE wallet, ksshaskpass
tries (too) hard to guess a good key from that line. And for both
invocations, it comes up with the same key (the URL), so that when the
password info is needed, the username info from the wallet is returned.
Authentication fails.
Far from good.

If you still want to use the KDE helpers, you can save the little snippet
#!/bin/bash
set $*
exec ksshaskpass $1\@$3
into ~/bin/git-ksshaskpass (and make it executable) and set

git config --global core.askpass ~/bin/git-ksshaskpass
Gitcha!

NB:

    tested and works with https
    not tested with other methods (use ssh-agent)
    new credential helper interface coming in Git after 1.7.7

Re: Git ksshaskpass to play nice with https and kwallet

[Jeff King]

On Tue, Oct 04, 2011 at 12:19:59PM +0200, Michael J Gruber wrote:

> But Git calls the askpass helper with a command line like
> /usr/bin/ksshaskpass Username for 'bitbucket.org':
> and once again with
> /usr/bin/ksshaskpass Password for 'bitbucket.org':
> So far so good.
> 
> But when asked to store the credentials in the KDE wallet, ksshaskpass
> tries (too) hard to guess a good key from that line. And for both
> invocations, it comes up with the same key (the URL), so that when the
> password info is needed, the username info from the wallet is returned.
> Authentication fails.
> Far from good.

Neat. I didn't know ksshaskpass would do that. I wondered for a minute
if all of the credential helper stuff could have gone through the
askpass interface. But I don't think so.

One problem is that the askpass interface only lets us ask for one thing
at a time. So even with your clever hack, it will end up storing two
separate keys: Username@host and Password@host. But it has no idea
they're connected. So if you store "user1 / pass1", then try to push to
"user2@host", we would silently use the password for user1.

On top of that, there isn't much contextual information. I guess they
assumed the guessing would be used for "ssh". But it means that a stored
ssh password could potentially be used for git, and vice versa. I guess
you could get around that by making the host field longer and more
descriptive (i.e., a full url).

-Peff

Re: Git ksshaskpass to play nice with https and kwallet

[Michael J Gruber]

Jeff King venit, vidit, dixit 04.10.2011 12:50:
> On Tue, Oct 04, 2011 at 12:19:59PM +0200, Michael J Gruber wrote:
> 
>> But Git calls the askpass helper with a command line like
>> /usr/bin/ksshaskpass Username for 'bitbucket.org':
>> and once again with
>> /usr/bin/ksshaskpass Password for 'bitbucket.org':
>> So far so good.
>>
>> But when asked to store the credentials in the KDE wallet, ksshaskpass
>> tries (too) hard to guess a good key from that line. And for both
>> invocations, it comes up with the same key (the URL), so that when the
>> password info is needed, the username info from the wallet is returned.
>> Authentication fails.
>> Far from good.
> 
> Neat. I didn't know ksshaskpass would do that. I wondered for a minute
> if all of the credential helper stuff could have gone through the
> askpass interface. But I don't think so.

Don't worry ;)

> One problem is that the askpass interface only lets us ask for one thing
> at a time. So even with your clever hack, it will end up storing two
> separate keys: Username@host and Password@host. But it has no idea
> they're connected. So if you store "user1 / pass1", then try to push to
> "user2@host", we would silently use the password for user1.
> 
> On top of that, there isn't much contextual information. I guess they
> assumed the guessing would be used for "ssh". But it means that a stored
> ssh password could potentially be used for git, and vice versa. I guess
> you could get around that by making the host field longer and more
> descriptive (i.e., a full url).

I think it's really meant for ssh keys only, where the keyid identifies
the key uniquely.

Still, ksshaskpass's trying to guess a unique key from the prompt text
seems quite hackish to me. But many people will have a Git without
credential-helpers, and a KDE default setup, so hope my post helps
someone besides myself.

Note that git-credentials-askpass would have a fair chance of doing
better: credential_askpass() knows the username and could pass it to
credential_ask_one(), e.g. by amending the description field, or setting
the first field to "Password for user %(user)". Do you think that would
be worth deviating from the default behavior (i.e. compared to no helper)?

Cheers,
Michael

Re: Git ksshaskpass to play nice with https and kwallet

[Jeff King]

On Tue, Oct 04, 2011 at 01:27:57PM +0200, Michael J Gruber wrote:

> Still, ksshaskpass's trying to guess a unique key from the prompt text
> seems quite hackish to me. But many people will have a Git without
> credential-helpers, and a KDE default setup, so hope my post helps
> someone besides myself.

Hmm. I don't think that pre-credential-helper git actually puts the
hostname in the prompt, though. It just says "Username:". So your trick
wouldn't work then, would it?

> Note that git-credentials-askpass would have a fair chance of doing
> better: credential_askpass() knows the username and could pass it to
> credential_ask_one(), e.g. by amending the description field, or setting
> the first field to "Password for user %(user)". Do you think that would
> be worth deviating from the default behavior (i.e. compared to no helper)?

I think that git should do that by default. v1.7.7 (and earlier) does:

  $ git push https://example.com/foo.git
  Username:
  Password:

With my patches in 'next', it does:

  $ git push https://example.com/foo.git
  Username for 'example.com':
  Password for 'example.com':

But it would probably be better to say:

  $ git push https://example.com/foo.git
  Username for 'example.com':
  Password for 'user@example.com':

The latter is especially useful if you have put a username in your
~/.gitconfig, in which case you get:

  $ git push https://example.com/foo.git
  Password for 'user@example.com':

which is a nice reminder. And it would happen to work with your askpass
magic (I also wonder if it should mention the protocol and the repo, but
most of the time that isn't relevant, and it does make the prompt harder
to read).

-Peff

Re: Git ksshaskpass to play nice with https and kwallet

[Michael J Gruber]

Jeff King venit, vidit, dixit 04.10.2011 13:37:
> On Tue, Oct 04, 2011 at 01:27:57PM +0200, Michael J Gruber wrote:
> 
>> Still, ksshaskpass's trying to guess a unique key from the prompt text
>> seems quite hackish to me. But many people will have a Git without
>> credential-helpers, and a KDE default setup, so hope my post helps
>> someone besides myself.
> 
> Hmm. I don't think that pre-credential-helper git actually puts the
> hostname in the prompt, though. It just says "Username:". So your trick
> wouldn't work then, would it?
> 
>> Note that git-credentials-askpass would have a fair chance of doing
>> better: credential_askpass() knows the username and could pass it to
>> credential_ask_one(), e.g. by amending the description field, or setting
>> the first field to "Password for user %(user)". Do you think that would
>> be worth deviating from the default behavior (i.e. compared to no helper)?
> 
> I think that git should do that by default. v1.7.7 (and earlier) does:
> 
>   $ git push https://example.com/foo.git
>   Username:
>   Password:
> 
> With my patches in 'next', it does:
> 
>   $ git push https://example.com/foo.git
>   Username for 'example.com':
>   Password for 'example.com':
>

Sheesh. I'm too used to using next(+) to even think of that! You're
completely right: My trick only works with next's additions.

> But it would probably be better to say:
> 
>   $ git push https://example.com/foo.git
>   Username for 'example.com':
>   Password for 'user@example.com':

Yes, exactly. credential_askpass() knows what it needs for that.

> The latter is especially useful if you have put a username in your
> ~/.gitconfig, in which case you get:

I'm actually wondering why git can't infer the user from

https://user@host.com

with last week's next, at least.
> 
>   $ git push https://example.com/foo.git
>   Password for 'user@example.com':
> 
> which is a nice reminder. And it would happen to work with your askpass
> magic (I also wonder if it should mention the protocol and the repo, but
> most of the time that isn't relevant, and it does make the prompt harder
> to read).

With the above, I can probably do without any magic: 'example.com' would
be the wallet key for the username (if I let the wallet store it) and
'user@example.com' the key for the password, whether the username comes
from the wallet or from the config. (Again, why not from the URL?)

Michael

Re: Git ksshaskpass to play nice with https and kwallet

[Jeff King]

On Tue, Oct 04, 2011 at 02:12:02PM +0200, Michael J Gruber wrote:

> > The latter is especially useful if you have put a username in your
> > ~/.gitconfig, in which case you get:
> 
> I'm actually wondering why git can't infer the user from
> 
> https://user@host.com
> 
> with last week's next, at least.

It can, and it has for some time. Part of the configurable-username
thing was that it would be way nicer to just use a user-agnostic URL,
because it means it's easier to share with other people.

> >   $ git push https://example.com/foo.git
> >   Password for 'user@example.com':
> > 
> > which is a nice reminder. And it would happen to work with your askpass
> > magic (I also wonder if it should mention the protocol and the repo, but
> > most of the time that isn't relevant, and it does make the prompt harder
> > to read).
> 
> With the above, I can probably do without any magic: 'example.com' would
> be the wallet key for the username (if I let the wallet store it) and
> 'user@example.com' the key for the password, whether the username comes
> from the wallet or from the config. (Again, why not from the URL?)

Yeah, sorry, I should have said "ksshaskpass's magic". :)

And yes, it can come from the URL. Mentioning the user in the password
prompt is not as useful a reminder if it comes from:

  $ git push https://user@example.com/foo.git

but, if it's something like:

  $ git clone https://user@example.com/foo.git
  [months pass]

  $ git push
  Password for 'user@example.com':

then it's a nice reminder.

-Peff

Re: Git ksshaskpass to play nice with https and kwallet

[Michael J Gruber]

Jeff King venit, vidit, dixit 04.10.2011 14:43:
> On Tue, Oct 04, 2011 at 02:12:02PM +0200, Michael J Gruber wrote:
> 
>>> The latter is especially useful if you have put a username in your
>>> ~/.gitconfig, in which case you get:
>>
>> I'm actually wondering why git can't infer the user from
>>
>> https://user@host.com
>>
>> with last week's next, at least.
> 
> It can, and it has for some time. Part of the configurable-username
> thing was that it would be way nicer to just use a user-agnostic URL,
> because it means it's easier to share with other people.
> 

We seem to mean something different:

git config --get remote.bitbucket.pushurl
https://grubix@bitbucket.org/grubix/git.git
SSH_ASKPASS= git push -n bitbucket
Username for 'bitbucket.org':

I mean that git should not need to ask for the username here.

Michael

Re: Git ksshaskpass to play nice with https and kwallet

[Jeff King]

On Tue, Oct 04, 2011 at 08:49:55PM +0200, Michael J Gruber wrote:

> We seem to mean something different:
> 
> git config --get remote.bitbucket.pushurl
> https://grubix@bitbucket.org/grubix/git.git
> SSH_ASKPASS= git push -n bitbucket
> Username for 'bitbucket.org':
> 
> I mean that git should not need to ask for the username here.

No, we are in agreement about the intended behavior. I think you are
seeing a bug. What version of git produced it?

With my http-auth series, I get:

  $ git push https://github.com/peff/git.git
  Username for 'github.com':

  $ git push https://peff@github.com/peff/git.git
  Password for 'github.com':

Using v1.7.7 produces similar results.

-Peff

Re: Git ksshaskpass to play nice with https and kwallet

[Jeff King]

On Wed, Oct 05, 2011 at 01:55:36PM -0400, Jeff King wrote:

> On Tue, Oct 04, 2011 at 08:49:55PM +0200, Michael J Gruber wrote:
> 
> > We seem to mean something different:
> > 
> > git config --get remote.bitbucket.pushurl
> > https://grubix@bitbucket.org/grubix/git.git
> > SSH_ASKPASS= git push -n bitbucket
> > Username for 'bitbucket.org':
> > 
> > I mean that git should not need to ask for the username here.
> 
> No, we are in agreement about the intended behavior. I think you are
> seeing a bug. What version of git produced it?
> 
> With my http-auth series, I get:
> 
>   $ git push https://github.com/peff/git.git
>   Username for 'github.com':
> 
>   $ git push https://peff@github.com/peff/git.git
>   Password for 'github.com':
> 
> Using v1.7.7 produces similar results.

Hrm. I do get this, with the same version of git:

  $ git config remote.foo.url https://github.com/peff/git.git
  $ git push foo
  Username for 'github.com':

  $ git config remote.foo.url https://peff@github.com/peff/git.git
  $ git push foo
  Password for 'github.com':

So far so good. Now how about this:

  $ git config remote.foo.url https://github.com/peff/git.git
  $ git config remote.foo.pushurl https://peff@github.com/peff/git.git
  $ git push foo
  Username for 'github.com':

So I think the problem is with pushurl, not with the auth code. Oddly,
though, running GIT_TRACE reveals:

  $ GIT_TRACE=1 git push foo
  trace: built-in: git 'push' 'foo'
  trace: run_command: 'git-remote-https' 'foo' 'https://peff@github.com/peff/git.git'

which is the right URL. So it's almost as if we are throwing away the
passed URL in favor of the configuration, and then looking up the
configuration wrong. I'm about to go get on a plane, so I don't have
more time to look at it now, but I suspect it's something simple and
stupid.

-Peff

Re: Git ksshaskpass to play nice with https and kwallet

[Michael J Gruber]

Jeff King venit, vidit, dixit 05.10.2011 20:01:
> On Wed, Oct 05, 2011 at 01:55:36PM -0400, Jeff King wrote:
> 
>> On Tue, Oct 04, 2011 at 08:49:55PM +0200, Michael J Gruber wrote:
>>
>>> We seem to mean something different:
>>>
>>> git config --get remote.bitbucket.pushurl
>>> https://grubix@bitbucket.org/grubix/git.git
>>> SSH_ASKPASS= git push -n bitbucket
>>> Username for 'bitbucket.org':
>>>
>>> I mean that git should not need to ask for the username here.
>>
>> No, we are in agreement about the intended behavior. I think you are
>> seeing a bug. What version of git produced it?
>>
>> With my http-auth series, I get:
>>
>>   $ git push https://github.com/peff/git.git
>>   Username for 'github.com':
>>
>>   $ git push https://peff@github.com/peff/git.git
>>   Password for 'github.com':
>>
>> Using v1.7.7 produces similar results.
> 
> Hrm. I do get this, with the same version of git:
> 
>   $ git config remote.foo.url https://github.com/peff/git.git
>   $ git push foo
>   Username for 'github.com':
> 
>   $ git config remote.foo.url https://peff@github.com/peff/git.git
>   $ git push foo
>   Password for 'github.com':
> 
> So far so good. Now how about this:
> 
>   $ git config remote.foo.url https://github.com/peff/git.git
>   $ git config remote.foo.pushurl https://peff@github.com/peff/git.git
>   $ git push foo
>   Username for 'github.com':
> 
> So I think the problem is with pushurl, not with the auth code. Oddly,
> though, running GIT_TRACE reveals:

Yep, I have a pushurl in config.

> 
>   $ GIT_TRACE=1 git push foo
>   trace: built-in: git 'push' 'foo'
>   trace: run_command: 'git-remote-https' 'foo' 'https://peff@github.com/peff/git.git'
> 
> which is the right URL. So it's almost as if we are throwing away the
> passed URL in favor of the configuration, and then looking up the
> configuration wrong. I'm about to go get on a plane, so I don't have
> more time to look at it now, but I suspect it's something simple and
> stupid.

Thanks for confirming what it should be like. I never expected pushurl
and url to behave differently (I had used GIT_TRACE myself).

Good flight :)

Michael

[RFC/PATCH] remote-curl: Obey passed URL

[Michael J Gruber]

When the curl remote helper is called, e.g., as

git-remote-https foo https://john@doe.com

it looks up remote.foo.url rather than taking the provided url, at least
as far as http_init() is concerned (authentication). (It does use the
provided url at later stages.)

The problem is that for pushing, "git push" looks up the pushurl, which
may be different, and passes that down to the remote helper, so that the
remote helper should take that when provided.

Note that at the init stage, the remote helper does not know what kind of
transcation is going to be requested, but the caller does.

Change it so that the remote helper obeys the passed url rather than
trying to look it up.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
While this passes all tests and fixes the described problem, I don't
know about any side effects. Also, I always feel a bit insecure about
simply dropping pointers to allocated memory (remote->url[0]). Should
I free() it? Neither remote-helpers nor memory management are exactly
homeground for me...
---
 remote-curl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index b8cf45a..1fa396a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -846,6 +846,7 @@ int main(int argc, const char **argv)
 
 	if (argc > 2) {
 		end_url_with_slash(&buf, argv[2]);
+		remote->url[0] = xstrdup(argv[2]);
 	} else {
 		end_url_with_slash(&buf, remote->url[0]);
 	}
-- 
1.7.7.rc2.451.g15e150

Re: [RFC/PATCH] remote-curl: Obey passed URL

[Jeff King]

On Thu, Oct 06, 2011 at 03:15:59PM +0200, Michael J Gruber wrote:

> When the curl remote helper is called, e.g., as
> 
> git-remote-https foo https://john@doe.com
> 
> it looks up remote.foo.url rather than taking the provided url, at least
> as far as http_init() is concerned (authentication). (It does use the
> provided url at later stages.)
> [...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -846,6 +846,7 @@ int main(int argc, const char **argv)
>  
>  	if (argc > 2) {
>  		end_url_with_slash(&buf, argv[2]);
> +		remote->url[0] = xstrdup(argv[2]);
>  	} else {
>  		end_url_with_slash(&buf, remote->url[0]);
>  	}

Your analysis is correct, but tweaking the remote object seems kind
of ugly. I think a nicer solution would be to pass the URL in
separately to http_init. Of the three existing callers:

  1. http-fetch.c just passes a NULL remote. Which means we don't even
     look at the URL at all for grabbing the auth information. Passing
     the URL would be an improvement.

  2. http-push.c creates a fake remote just to stick the URL in. That
     ugly code could just go away.

  3. remote-curl.c needs to pass in the alternate URL anyway, as you
     described.

So it seems like all callsites would benefit.

That being said, I wonder if http_init is the right place to pull the
auth information out. It's where we've always done it, and it makes
sense if you are hitting one base URL. But what happens if we get a
redirect to some other site? Shouldn't we be deciding at the time of
making the request what the context of the http request is?

And then http_init can stop caring entirely what the URL is.

-Peff

Re: [RFC/PATCH] remote-curl: Obey passed URL

[Jeff King]

On Thu, Oct 06, 2011 at 09:25:00AM -0400, Jeff King wrote:

> Your analysis is correct, but tweaking the remote object seems kind
> of ugly. I think a nicer solution would be to pass the URL in
> separately to http_init. Of the three existing callers:

Here's what that patch looks like. It's definitely an improvement and
fixes a real bug, so it may be worth applying. But I'm still going to
look into pushing the url examination closer to the point of use.

-- >8 --
Subject: [PATCH] http_init: accept separate URL parameter

The http_init function takes a "struct remote". Part of its
initialization procedure is to look at the remote's url and
grab some auth-related parameters. However, using the url
included in the remote is:

  - wrong; the remote-curl helper may have a separate,
    unrelated URL (e.g., from remote.*.pushurl). Looking at
    the remote's configured url is incorrect.

  - incomplete; http-fetch doesn't have a remote, so passes
    NULL. So http_init never gets to see the URL we are
    actually going to use.

  - cumbersome; http-push has a similar problem to
    http-fetch, but actually builds a fake remote just to
    pass in the URL.

Instead, let's just add a separate URL parameter to
http_init, and all three callsites can pass in the
appropriate information.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-fetch.c  |    2 +-
 http-push.c   |   10 +---------
 http.c        |    8 ++++----
 http.h        |    2 +-
 remote-curl.c |    2 +-
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/http-fetch.c b/http-fetch.c
index 3af4c71..e341872 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -63,7 +63,7 @@ int main(int argc, const char **argv)
 
 	git_config(git_default_config, NULL);
 
-	http_init(NULL);
+	http_init(NULL, url);
 	walker = get_http_walker(url);
 	walker->get_tree = get_tree;
 	walker->get_history = get_history;
diff --git a/http-push.c b/http-push.c
index 376331a..215b640 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1747,7 +1747,6 @@ int main(int argc, char **argv)
 	int i;
 	int new_refs;
 	struct ref *ref, *local_refs;
-	struct remote *remote;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -1821,14 +1820,7 @@ int main(int argc, char **argv)
 
 	memset(remote_dir_exists, -1, 256);
 
-	/*
-	 * Create a minimum remote by hand to give to http_init(),
-	 * primarily to allow it to look at the URL.
-	 */
-	remote = xcalloc(sizeof(*remote), 1);
-	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
-	remote->url[remote->url_nr++] = repo->url;
-	http_init(remote);
+	http_init(NULL, repo->url);
 
 #ifdef USE_CURL_MULTI
 	is_running_queue = 0;
diff --git a/http.c b/http.c
index b2ae8de..d9f9938 100644
--- a/http.c
+++ b/http.c
@@ -357,7 +357,7 @@ static void set_from_env(const char **var, const char *envname)
 		*var = val;
 }
 
-void http_init(struct remote *remote)
+void http_init(struct remote *remote, const char *url)
 {
 	char *low_speed_limit;
 	char *low_speed_time;
@@ -421,11 +421,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
 
-	if (remote && remote->url && remote->url[0]) {
-		http_auth_init(remote->url[0]);
+	if (url) {
+		http_auth_init(url);
 		if (!ssl_cert_password_required &&
 		    getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
-		    !prefixcmp(remote->url[0], "https://"))
+		    !prefixcmp(url, "https://"))
 			ssl_cert_password_required = 1;
 	}
 
diff --git a/http.h b/http.h
index 0bf8592..3c332a9 100644
--- a/http.h
+++ b/http.h
@@ -86,7 +86,7 @@ struct buffer {
 extern void step_active_slots(void);
 #endif
 
-extern void http_init(struct remote *remote);
+extern void http_init(struct remote *remote, const char *url);
 extern void http_cleanup(void);
 
 extern int data_received;
diff --git a/remote-curl.c b/remote-curl.c
index 69831e9..33d3d8c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -852,7 +852,7 @@ int main(int argc, const char **argv)
 
 	url = strbuf_detach(&buf, NULL);
 
-	http_init(remote);
+	http_init(remote, url);
 
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF)
-- 
1.7.7.37.g0e376

Re: [RFC/PATCH] remote-curl: Obey passed URL

[Michael J Gruber]

Jeff King venit, vidit, dixit 06.10.2011 15:37:
> On Thu, Oct 06, 2011 at 09:25:00AM -0400, Jeff King wrote:
> 
>> Your analysis is correct, but tweaking the remote object seems kind
>> of ugly. I think a nicer solution would be to pass the URL in
>> separately to http_init. Of the three existing callers:
> 
> Here's what that patch looks like. It's definitely an improvement and
> fixes a real bug, so it may be worth applying. But I'm still going to
> look into pushing the url examination closer to the point of use.

It definitely is an improvement. I've been running happily with this
(and without my askpass helper/workaround). Are you going forward with
this one?

> 
> -- >8 --
> Subject: [PATCH] http_init: accept separate URL parameter
> 
> The http_init function takes a "struct remote". Part of its
> initialization procedure is to look at the remote's url and
> grab some auth-related parameters. However, using the url
> included in the remote is:
> 
>   - wrong; the remote-curl helper may have a separate,
>     unrelated URL (e.g., from remote.*.pushurl). Looking at
>     the remote's configured url is incorrect.
> 
>   - incomplete; http-fetch doesn't have a remote, so passes
>     NULL. So http_init never gets to see the URL we are
>     actually going to use.
> 
>   - cumbersome; http-push has a similar problem to
>     http-fetch, but actually builds a fake remote just to
>     pass in the URL.
> 
> Instead, let's just add a separate URL parameter to
> http_init, and all three callsites can pass in the
> appropriate information.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  http-fetch.c  |    2 +-
>  http-push.c   |   10 +---------
>  http.c        |    8 ++++----
>  http.h        |    2 +-
>  remote-curl.c |    2 +-
>  5 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/http-fetch.c b/http-fetch.c
> index 3af4c71..e341872 100644
> --- a/http-fetch.c
> +++ b/http-fetch.c
> @@ -63,7 +63,7 @@ int main(int argc, const char **argv)
>  
>  	git_config(git_default_config, NULL);
>  
> -	http_init(NULL);
> +	http_init(NULL, url);
>  	walker = get_http_walker(url);
>  	walker->get_tree = get_tree;
>  	walker->get_history = get_history;
> diff --git a/http-push.c b/http-push.c
> index 376331a..215b640 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1747,7 +1747,6 @@ int main(int argc, char **argv)
>  	int i;
>  	int new_refs;
>  	struct ref *ref, *local_refs;
> -	struct remote *remote;
>  
>  	git_extract_argv0_path(argv[0]);
>  
> @@ -1821,14 +1820,7 @@ int main(int argc, char **argv)
>  
>  	memset(remote_dir_exists, -1, 256);
>  
> -	/*
> -	 * Create a minimum remote by hand to give to http_init(),
> -	 * primarily to allow it to look at the URL.
> -	 */
> -	remote = xcalloc(sizeof(*remote), 1);
> -	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
> -	remote->url[remote->url_nr++] = repo->url;
> -	http_init(remote);
> +	http_init(NULL, repo->url);
>  
>  #ifdef USE_CURL_MULTI
>  	is_running_queue = 0;
> diff --git a/http.c b/http.c
> index b2ae8de..d9f9938 100644
> --- a/http.c
> +++ b/http.c
> @@ -357,7 +357,7 @@ static void set_from_env(const char **var, const char *envname)
>  		*var = val;
>  }
>  
> -void http_init(struct remote *remote)
> +void http_init(struct remote *remote, const char *url)
>  {
>  	char *low_speed_limit;
>  	char *low_speed_time;
> @@ -421,11 +421,11 @@ void http_init(struct remote *remote)
>  	if (getenv("GIT_CURL_FTP_NO_EPSV"))
>  		curl_ftp_no_epsv = 1;
>  
> -	if (remote && remote->url && remote->url[0]) {
> -		http_auth_init(remote->url[0]);
> +	if (url) {
> +		http_auth_init(url);
>  		if (!ssl_cert_password_required &&
>  		    getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
> -		    !prefixcmp(remote->url[0], "https://"))
> +		    !prefixcmp(url, "https://"))
>  			ssl_cert_password_required = 1;
>  	}
>  
> diff --git a/http.h b/http.h
> index 0bf8592..3c332a9 100644
> --- a/http.h
> +++ b/http.h
> @@ -86,7 +86,7 @@ struct buffer {
>  extern void step_active_slots(void);
>  #endif
>  
> -extern void http_init(struct remote *remote);
> +extern void http_init(struct remote *remote, const char *url);
>  extern void http_cleanup(void);
>  
>  extern int data_received;
> diff --git a/remote-curl.c b/remote-curl.c
> index 69831e9..33d3d8c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -852,7 +852,7 @@ int main(int argc, const char **argv)
>  
>  	url = strbuf_detach(&buf, NULL);
>  
> -	http_init(remote);
> +	http_init(remote, url);
>  
>  	do {
>  		if (strbuf_getline(&buf, stdin, '\n') == EOF)

[PATCH] http_init: accept separate URL parameter

[Jeff King]

The http_init function takes a "struct remote". Part of its
initialization procedure is to look at the remote's url and
grab some auth-related parameters. However, using the url
included in the remote is:

  - wrong; the remote-curl helper may have a separate,
    unrelated URL (e.g., from remote.*.pushurl). Looking at
    the remote's configured url is incorrect.

  - incomplete; http-fetch doesn't have a remote, so passes
    NULL. So http_init never gets to see the URL we are
    actually going to use.

  - cumbersome; http-push has a similar problem to
    http-fetch, but actually builds a fake remote just to
    pass in the URL.

Instead, let's just add a separate URL parameter to
http_init, and all three callsites can pass in the
appropriate information.

Signed-off-by: Jeff King <peff@peff.net>
---
On Wed, Oct 12, 2011 at 10:51:20PM +0200, Michael J Gruber wrote:

> > Here's what that patch looks like. It's definitely an improvement and
> > fixes a real bug, so it may be worth applying. But I'm still going to
> > look into pushing the url examination closer to the point of use.
> 
> It definitely is an improvement. I've been running happily with this
> (and without my askpass helper/workaround). Are you going forward with
> this one?

I think we should go ahead with this one. I gave some thought to
tweaking the http code to figure out authentication closer to the point
of use, so we could be adaptive to things like redirects. But it's quite
an invasive change, since we now have to start possibly keeping a string
of credentials, each mapped from their context.

But more importantly, it changes the user-visible behavior. If I do
something like:

  git fetch https://user@git.foo.com/repo.git

and give it a password, and then it redirects me to "git2.foo.com" or
something, then right now we will retry the same credential. I'm not
sure if people rely on that or not.

Arguably, it's wrong to do so in the general case. If I redirect to
"git.someotherdomain.com", you probably _do_ want to re-ask the
credential. So maybe it should be changed, and there should be some
magic with comparing the old and new contexts. I dunno.

At any rate, this is certainly an improvement in the meantime. If the
url parameter to http_init eventually goes away, it is easy enough to do
on top of this.

 http-fetch.c  |    2 +-
 http-push.c   |   10 +---------
 http.c        |    8 ++++----
 http.h        |    2 +-
 remote-curl.c |    2 +-
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/http-fetch.c b/http-fetch.c
index 3af4c71..e341872 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -63,7 +63,7 @@ int main(int argc, const char **argv)
 
 	git_config(git_default_config, NULL);
 
-	http_init(NULL);
+	http_init(NULL, url);
 	walker = get_http_walker(url);
 	walker->get_tree = get_tree;
 	walker->get_history = get_history;
diff --git a/http-push.c b/http-push.c
index 6e8f6d0..ecbfae5 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1747,7 +1747,6 @@ int main(int argc, char **argv)
 	int i;
 	int new_refs;
 	struct ref *ref, *local_refs;
-	struct remote *remote;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -1821,14 +1820,7 @@ int main(int argc, char **argv)
 
 	memset(remote_dir_exists, -1, 256);
 
-	/*
-	 * Create a minimum remote by hand to give to http_init(),
-	 * primarily to allow it to look at the URL.
-	 */
-	remote = xcalloc(sizeof(*remote), 1);
-	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
-	remote->url[remote->url_nr++] = repo->url;
-	http_init(remote);
+	http_init(NULL, repo->url);
 
 #ifdef USE_CURL_MULTI
 	is_running_queue = 0;
diff --git a/http.c b/http.c
index d6b2d78..65d3aa7 100644
--- a/http.c
+++ b/http.c
@@ -356,7 +356,7 @@ static void set_from_env(const char **var, const char *envname)
 		*var = val;
 }
 
-void http_init(struct remote *remote)
+void http_init(struct remote *remote, const char *url)
 {
 	char *low_speed_limit;
 	char *low_speed_time;
@@ -420,11 +420,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
 
-	if (remote && remote->url && remote->url[0]) {
-		http_auth_init(remote->url[0]);
+	if (url) {
+		http_auth_init(url);
 		if (!ssl_cert_password_required &&
 		    getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
-		    !prefixcmp(remote->url[0], "https://"))
+		    !prefixcmp(url, "https://"))
 			ssl_cert_password_required = 1;
 	}
 
diff --git a/http.h b/http.h
index 0bf8592..3c332a9 100644
--- a/http.h
+++ b/http.h
@@ -86,7 +86,7 @@ struct buffer {
 extern void step_active_slots(void);
 #endif
 
-extern void http_init(struct remote *remote);
+extern void http_init(struct remote *remote, const char *url);
 extern void http_cleanup(void);
 
 extern int data_received;
diff --git a/remote-curl.c b/remote-curl.c
index 6c24ab1..d4d0910 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -850,7 +850,7 @@ int main(int argc, const char **argv)
 
 	url = strbuf_detach(&buf, NULL);
 
-	http_init(remote);
+	http_init(remote, url);
 
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF)
-- 
1.7.7.rc2.21.gb9948

Re: [PATCH] http_init: accept separate URL parameter

[Jeff King]

On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:

> The http_init function takes a "struct remote". Part of its
> initialization procedure is to look at the remote's url and
> grab some auth-related parameters. However, using the url
> included in the remote is:
> 
>   - wrong; the remote-curl helper may have a separate,
>     unrelated URL (e.g., from remote.*.pushurl). Looking at
>     the remote's configured url is incorrect.
> 
>   - incomplete; http-fetch doesn't have a remote, so passes
>     NULL. So http_init never gets to see the URL we are
>     actually going to use.
> 
>   - cumbersome; http-push has a similar problem to
>     http-fetch, but actually builds a fake remote just to
>     pass in the URL.
> 
> Instead, let's just add a separate URL parameter to
> http_init, and all three callsites can pass in the
> appropriate information.
> 
> Signed-off-by: Jeff King <peff@peff.net>

Sorry, I forgot to mention: this is meant to go on top of the
http-auth-keyring topic.

-Peff

Re: [PATCH] http_init: accept separate URL parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
> ...
>> Instead, let's just add a separate URL parameter to
>> http_init, and all three callsites can pass in the
>> appropriate information.
>> 
>> Signed-off-by: Jeff King <peff@peff.net>
>
> Sorry, I forgot to mention: this is meant to go on top of the
> http-auth-keyring topic.

Hmm, of course the patch was written to help http-auth-keyring topic, but
wouldn't this be an improvement that is general enough?  I.e. it could
even go to the bottom of the topic, no?

Re: [PATCH] http_init: accept separate URL parameter

[Jeff King]

On Wed, Oct 12, 2011 at 03:38:27PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
> > ...
> >> Instead, let's just add a separate URL parameter to
> >> http_init, and all three callsites can pass in the
> >> appropriate information.
> >> 
> >> Signed-off-by: Jeff King <peff@peff.net>
> >
> > Sorry, I forgot to mention: this is meant to go on top of the
> > http-auth-keyring topic.
> 
> Hmm, of course the patch was written to help http-auth-keyring topic, but
> wouldn't this be an improvement that is general enough?  I.e. it could
> even go to the bottom of the topic, no?

Yes, it could, and probably should. I suspect it might need some
rebasing to do that.

I'm going to float some other possible designs for the topic as soon as
I put enough polish on them. So I'll try to move this down when I
re-roll.  In the meantime, if you want to throw it on top, great. If you
want to ignore it until then, no problem. :)

-Peff

Re: [PATCH] http_init: accept separate URL parameter

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.10.2011 00:46:
> On Wed, Oct 12, 2011 at 03:38:27PM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
>>> ...
>>>> Instead, let's just add a separate URL parameter to
>>>> http_init, and all three callsites can pass in the
>>>> appropriate information.
>>>>
>>>> Signed-off-by: Jeff King <peff@peff.net>
>>>
>>> Sorry, I forgot to mention: this is meant to go on top of the
>>> http-auth-keyring topic.
>>
>> Hmm, of course the patch was written to help http-auth-keyring topic, but
>> wouldn't this be an improvement that is general enough?  I.e. it could
>> even go to the bottom of the topic, no?
> 
> Yes, it could, and probably should. I suspect it might need some
> rebasing to do that.
> 
> I'm going to float some other possible designs for the topic as soon as
> I put enough polish on them. So I'll try to move this down when I
> re-roll.  In the meantime, if you want to throw it on top, great. If you
> want to ignore it until then, no problem. :)
> 
> -Peff

Thanks, Jeff.

To clarify:

Without http-auth-keyring, this helps in the sense that git reads the
username from a user@host URL and asks for the password only. When using
GIT_ASKPASS or such, the askpass helper is called with "Password:" only.

With (parts of) http-auth-keyring, the askpass helper is called with
"Password for:user@host", which helps the user identify the request, and
which helps helpers such as ksshaskpass to store the password with a
meaningful key in a wallet.

I'm not sure whether it's feasible/worth taking these bits of
http-auth-keyring (improved prompt) out and apply them early. That is,
I'm sure it's worth it (it would alleviate the need for credential
helpers for some users at least), I haven't looked at feasibility ;)

Michael

[PATCH 0/6] http-auth-early

[Michael J Gruber]

Here are the early parts of Jeff's http-auth-keyring series.
It contains only parts which are not using the credential API (which
is still under discussion), so that this can go in (and help users)
and alleviates the pressure on the credential discussion:

Early bits with cleanups to http.c.
Cherry-picked bit for improved prompts ("Username for ..." etc.)
Cherry-pickes bit for using configured pushurls.

I tried to pick/resolve in a way which should help rebasing Jeff's series
on top of this.

Jeff King (5):
  url: decode buffers that are not NUL-terminated
  improve httpd auth tests
  remote-curl: don't retry auth failures with dumb protocol
  http: retry authentication failures for all http requests
  http_init: accept separate URL parameter

Michael J Gruber (1):
  http: use hostname in credential description

 http-fetch.c          |    2 +-
 http-push.c           |   10 +-----
 http.c                |   91 +++++++++++++++++++++++++++---------------------
 http.h                |    2 +-
 remote-curl.c         |    4 +-
 t/lib-httpd.sh        |   10 +++--
 t/t5550-http-fetch.sh |   51 +++++++++++++++++++++++++--
 url.c                 |   26 ++++++++++----
 url.h                 |    1 +
 9 files changed, 128 insertions(+), 69 deletions(-)

-- 
1.7.7.338.g0156b

[PATCH 1/6] url: decode buffers that are not NUL-terminated

[Michael J Gruber]

From: Jeff King <peff@peff.net>

The url_decode function needs only minor tweaks to handle
arbitrary buffers. Let's do those tweaks, which cleans up an
unreadable mess of temporary strings in http.c.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |   27 ++++-----------------------
 url.c  |   26 ++++++++++++++++++--------
 url.h  |    1 +
 3 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/http.c b/http.c
index a1ea3db..c93716c 100644
--- a/http.c
+++ b/http.c
@@ -307,8 +307,7 @@ static CURL *get_curl_handle(void)
 
 static void http_auth_init(const char *url)
 {
-	char *at, *colon, *cp, *slash, *decoded;
-	int len;
+	char *at, *colon, *cp, *slash;
 
 	cp = strstr(url, "://");
 	if (!cp)
@@ -328,29 +327,11 @@ static void http_auth_init(const char *url)
 		return; /* No credentials */
 	if (!colon || at <= colon) {
 		/* Only username */
-		len = at - cp;
-		user_name = xmalloc(len + 1);
-		memcpy(user_name, cp, len);
-		user_name[len] = '\0';
-		decoded = url_decode(user_name);
-		free(user_name);
-		user_name = decoded;
+		user_name = url_decode_mem(cp, at - cp);
 		user_pass = NULL;
 	} else {
-		len = colon - cp;
-		user_name = xmalloc(len + 1);
-		memcpy(user_name, cp, len);
-		user_name[len] = '\0';
-		decoded = url_decode(user_name);
-		free(user_name);
-		user_name = decoded;
-		len = at - (colon + 1);
-		user_pass = xmalloc(len + 1);
-		memcpy(user_pass, colon + 1, len);
-		user_pass[len] = '\0';
-		decoded = url_decode(user_pass);
-		free(user_pass);
-		user_pass = decoded;
+		user_name = url_decode_mem(cp, colon - cp);
+		user_pass = url_decode_mem(colon + 1, at - (colon + 1));
 	}
 }
 
diff --git a/url.c b/url.c
index 3e06fd3..389d9da 100644
--- a/url.c
+++ b/url.c
@@ -68,18 +68,20 @@ static int url_decode_char(const char *q)
 	return val;
 }
 
-static char *url_decode_internal(const char **query, const char *stop_at,
-				 struct strbuf *out, int decode_plus)
+static char *url_decode_internal(const char **query, int len,
+				 const char *stop_at, struct strbuf *out,
+				 int decode_plus)
 {
 	const char *q = *query;
 
-	do {
+	while (len) {
 		unsigned char c = *q;
 
 		if (!c)
 			break;
 		if (stop_at && strchr(stop_at, c)) {
 			q++;
+			len--;
 			break;
 		}
 
@@ -88,6 +90,7 @@ static char *url_decode_internal(const char **query, const char *stop_at,
 			if (0 <= val) {
 				strbuf_addch(out, val);
 				q += 3;
+				len -= 3;
 				continue;
 			}
 		}
@@ -97,34 +100,41 @@ static char *url_decode_internal(const char **query, const char *stop_at,
 		else
 			strbuf_addch(out, c);
 		q++;
-	} while (1);
+		len--;
+	}
 	*query = q;
 	return strbuf_detach(out, NULL);
 }
 
 char *url_decode(const char *url)
 {
+	return url_decode_mem(url, strlen(url));
+}
+
+char *url_decode_mem(const char *url, int len)
+{
 	struct strbuf out = STRBUF_INIT;
-	const char *colon = strchr(url, ':');
+	const char *colon = memchr(url, ':', len);
 
 	/* Skip protocol part if present */
 	if (colon && url < colon) {
 		strbuf_add(&out, url, colon - url);
+		len -= colon - url;
 		url = colon;
 	}
-	return url_decode_internal(&url, NULL, &out, 0);
+	return url_decode_internal(&url, len, NULL, &out, 0);
 }
 
 char *url_decode_parameter_name(const char **query)
 {
 	struct strbuf out = STRBUF_INIT;
-	return url_decode_internal(query, "&=", &out, 1);
+	return url_decode_internal(query, -1, "&=", &out, 1);
 }
 
 char *url_decode_parameter_value(const char **query)
 {
 	struct strbuf out = STRBUF_INIT;
-	return url_decode_internal(query, "&", &out, 1);
+	return url_decode_internal(query, -1, "&", &out, 1);
 }
 
 void end_url_with_slash(struct strbuf *buf, const char *url)
diff --git a/url.h b/url.h
index 7100e32..abdaf6f 100644
--- a/url.h
+++ b/url.h
@@ -4,6 +4,7 @@
 extern int is_url(const char *url);
 extern int is_urlschemechar(int first_flag, int ch);
 extern char *url_decode(const char *url);
+extern char *url_decode_mem(const char *url, int len);
 extern char *url_decode_parameter_name(const char **query);
 extern char *url_decode_parameter_value(const char **query);
 
-- 
1.7.7.338.g0156b

[PATCH 2/6] improve httpd auth tests

[Michael J Gruber]

From: Jeff King <peff@peff.net>

These just checked that we could clone a repository when the
username and password were given in the URL; we should also
check that git will prompt when no or partial credentials
are given.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-httpd.sh        |   10 +++++---
 t/t5550-http-fetch.sh |   51 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b8996a3..f7dc078 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -81,8 +81,7 @@ prepare_httpd() {
 
 	if test -n "$LIB_HTTPD_SSL"
 	then
-		HTTPD_URL=https://127.0.0.1:$LIB_HTTPD_PORT
-		AUTH_HTTPD_URL=https://user%40host:user%40host@127.0.0.1:$LIB_HTTPD_PORT
+		HTTPD_PROTO=https
 
 		RANDFILE_PATH="$HTTPD_ROOT_PATH"/.rnd openssl req \
 			-config "$TEST_PATH/ssl.cnf" \
@@ -93,9 +92,12 @@ prepare_httpd() {
 		export GIT_SSL_NO_VERIFY
 		HTTPD_PARA="$HTTPD_PARA -DSSL"
 	else
-		HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
-		AUTH_HTTPD_URL=http://user%40host:user%40host@127.0.0.1:$LIB_HTTPD_PORT
+		HTTPD_PROTO=http
 	fi
+	HTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT
+	HTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST
+	HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST
+	HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST
 
 	if test -n "$LIB_HTTPD_DAV" -o -n "$LIB_HTTPD_SVN"
 	then
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index a1883ca..ed4db09 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -35,11 +35,54 @@ test_expect_success 'clone http repository' '
 	test_cmp file clone/file
 '
 
-test_expect_success 'clone http repository with authentication' '
+test_expect_success 'create password-protected repository' '
 	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/auth/" &&
-	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" "$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git" &&
-	git clone $AUTH_HTTPD_URL/auth/repo.git clone-auth &&
-	test_cmp file clone-auth/file
+	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git"
+'
+
+test_expect_success 'setup askpass helpers' '
+	cat >askpass <<-EOF &&
+	#!/bin/sh
+	echo >>"$PWD/askpass-query" "askpass: \$*" &&
+	cat "$PWD/askpass-response"
+	EOF
+	chmod +x askpass &&
+	GIT_ASKPASS="$PWD/askpass" &&
+	export GIT_ASKPASS &&
+	>askpass-expect-none &&
+	echo "askpass: Password: " >askpass-expect-pass &&
+	{ echo "askpass: Username: " &&
+	  cat askpass-expect-pass
+	} >askpass-expect-both
+'
+
+test_expect_success 'cloning password-protected repository can fail' '
+	>askpass-query &&
+	echo wrong >askpass-response &&
+	test_must_fail git clone "$HTTPD_URL/auth/repo.git" clone-auth-fail &&
+	test_cmp askpass-expect-both askpass-query
+'
+
+test_expect_success 'http auth can use user/pass in URL' '
+	>askpass-query &&
+	echo wrong >askpass-reponse &&
+	git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none &&
+	test_cmp askpass-expect-none askpass-query
+'
+
+test_expect_success 'http auth can use just user in URL' '
+	>askpass-query &&
+	echo user@host >askpass-response &&
+	git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass &&
+	test_cmp askpass-expect-pass askpass-query
+'
+
+test_expect_success 'http auth can request both user and pass' '
+	>askpass-query &&
+	echo user@host >askpass-response &&
+	git clone "$HTTPD_URL/auth/repo.git" clone-auth-both &&
+	test_cmp askpass-expect-both askpass-query
 '
 
 test_expect_success 'fetch changes via http' '
-- 
1.7.7.338.g0156b

[PATCH 3/6] remote-curl: don't retry auth failures with dumb protocol

[Michael J Gruber]

From: Jeff King <peff@peff.net>

When fetching an http URL, we first try fetching info/refs
with an extra "service" parameter. This will work for a
smart-http server, or a dumb server which ignores extra
parameters when fetching files. If that fails, we retry
without the extra parameter to remain compatible with dumb
servers which didn't like our first request.

If the server returned a "401 Unauthorized", indicating that
the credentials we provided were not good, there is not much
point in retrying. With the current code, we just waste an
extra round trip to the HTTP server before failing.

But as the http code becomes smarter about throwing away
rejected credentials and re-prompting the user for new ones
(which it will later in this series), this will become more
confusing. At some point we will stop asking for credentials
to retry smart http, and will be asking for credentials to
retry dumb http. So now we're not only wasting an extra HTTP
round trip for something that is unlikely to work, but we're
making the user re-type their password for it.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote-curl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index faaeda4..6c24ab1 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -115,7 +115,7 @@ static struct discovery* discover_refs(const char *service)
 	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
 
 	/* try again with "plain" url (no ? or & appended) */
-	if (http_ret != HTTP_OK) {
+	if (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {
 		free(refs_url);
 		strbuf_reset(&buffer);
 
-- 
1.7.7.338.g0156b

[PATCH 4/6] http: retry authentication failures for all http requests

[Michael J Gruber]

From: Jeff King <peff@peff.net>

Commit 42653c0 (Prompt for a username when an HTTP request
401s, 2010-04-01) changed http_get_strbuf to prompt for
credentials when we receive a 401, but didn't touch
http_get_file. The latter is called only for dumb http;
while it's usually the case that people don't use
authentication on top of dumb http, there is no reason not
to allow both types of requests to use this feature.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index c93716c..89e3cf4 100644
--- a/http.c
+++ b/http.c
@@ -846,13 +846,18 @@ static int http_request(const char *url, void *result, int target, int options)
 	return ret;
 }
 
+static int http_request_reauth(const char *url, void *result, int target,
+			       int options)
+{
+	int ret = http_request(url, result, target, options);
+	if (ret != HTTP_REAUTH)
+		return ret;
+	return http_request(url, result, target, options);
+}
+
 int http_get_strbuf(const char *url, struct strbuf *result, int options)
 {
-	int http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options);
-	if (http_ret == HTTP_REAUTH) {
-		http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options);
-	}
-	return http_ret;
+	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
 }
 
 /*
@@ -875,7 +880,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 		goto cleanup;
 	}
 
-	ret = http_request(url, result, HTTP_REQUEST_FILE, options);
+	ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
 	fclose(result);
 
 	if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))
-- 
1.7.7.338.g0156b

[PATCH 5/6] http: use hostname in credential description

[Michael J Gruber]

Until now, a request for an http password looked like:

  Username:
  Password:

Now it will look like:

  Username for 'example.com':
  Password for 'example.com':

Picked-from: Jeff King <peff@peff.net>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 http.c                |   41 +++++++++++++++++++++++++++++++++--------
 t/t5550-http-fetch.sh |    4 ++--
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/http.c b/http.c
index 89e3cf4..149e116 100644
--- a/http.c
+++ b/http.c
@@ -42,7 +42,7 @@ static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
-static char *user_name, *user_pass;
+static char *user_name, *user_pass, *description;
 static const char *user_agent;
 
 #if LIBCURL_VERSION_NUM >= 0x071700
@@ -139,6 +139,25 @@ static void process_curl_messages(void)
 }
 #endif
 
+static char *git_getpass_one(const char *what, const char *desc)
+{
+	struct strbuf prompt = STRBUF_INIT;
+	char *r;
+
+	if (desc)
+		strbuf_addf(&prompt, "%s for '%s': ", what, desc);
+	else
+		strbuf_addf(&prompt, "%s: ", what);
+	/* FIXME: for usernames, we should do something less magical that
+	* actually echoes the characters. However, we need to read from
+	* /dev/tty and not stdio, which is not portable (but getpass will do
+	* it for us). http.c uses the same workaround. */
+	r = git_getpass(prompt.buf);
+
+	strbuf_release(&prompt);
+	return xstrdup(r);
+}
+
 static int http_options(const char *var, const char *value, void *cb)
 {
 	if (!strcmp("http.sslverify", var)) {
@@ -214,7 +233,7 @@ static void init_curl_http_auth(CURL *result)
 	if (user_name) {
 		struct strbuf up = STRBUF_INIT;
 		if (!user_pass)
-			user_pass = xstrdup(git_getpass("Password: "));
+			user_pass = xstrdup(git_getpass_one("Password", description));
 		strbuf_addf(&up, "%s:%s", user_name, user_pass);
 		curl_easy_setopt(result, CURLOPT_USERPWD,
 				 strbuf_detach(&up, NULL));
@@ -229,7 +248,7 @@ static int has_cert_password(void)
 		return 0;
 	/* Only prompt the user once. */
 	ssl_cert_password_required = -1;
-	ssl_cert_password = git_getpass("Certificate Password: ");
+	ssl_cert_password = git_getpass_one("Certificate Password", description);
 	if (ssl_cert_password != NULL) {
 		ssl_cert_password = xstrdup(ssl_cert_password);
 		return 1;
@@ -307,7 +326,7 @@ static CURL *get_curl_handle(void)
 
 static void http_auth_init(const char *url)
 {
-	char *at, *colon, *cp, *slash;
+	const char *at, *colon, *cp, *slash, *host;
 
 	cp = strstr(url, "://");
 	if (!cp)
@@ -323,16 +342,22 @@ static void http_auth_init(const char *url)
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');
-	if (!at || slash <= at)
-		return; /* No credentials */
-	if (!colon || at <= colon) {
+	if (!at || slash <= at) {
+		/* No credentials, but we may have to ask for some later */
+		host = cp;
+	}
+	else if (!colon || at <= colon) {
 		/* Only username */
 		user_name = url_decode_mem(cp, at - cp);
 		user_pass = NULL;
+		host = at + 1;
 	} else {
 		user_name = url_decode_mem(cp, colon - cp);
 		user_pass = url_decode_mem(colon + 1, at - (colon + 1));
+		host = at + 1;
 	}
+
+	description = url_decode_mem(host, slash - host);
 }
 
 static void set_from_env(const char **var, const char *envname)
@@ -828,7 +853,7 @@ static int http_request(const char *url, void *result, int target, int options)
 				 * but that is non-portable.  Using git_getpass() can at least be stubbed
 				 * on other platforms with a different implementation if/when necessary.
 				 */
-				user_name = xstrdup(git_getpass("Username: "));
+				user_name = xstrdup(git_getpass_one("Username", description));
 				init_curl_http_auth(slot->curl);
 				ret = HTTP_REAUTH;
 			}
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index ed4db09..d1ab4d0 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -51,8 +51,8 @@ test_expect_success 'setup askpass helpers' '
 	GIT_ASKPASS="$PWD/askpass" &&
 	export GIT_ASKPASS &&
 	>askpass-expect-none &&
-	echo "askpass: Password: " >askpass-expect-pass &&
-	{ echo "askpass: Username: " &&
+	echo "askpass: Password for '\''$HTTPD_DEST'\'': " >askpass-expect-pass &&
+	{ echo "askpass: Username for '\''$HTTPD_DEST'\'': " &&
 	  cat askpass-expect-pass
 	} >askpass-expect-both
 '
-- 
1.7.7.338.g0156b

[PATCH 6/6] http_init: accept separate URL parameter

[Michael J Gruber]

From: Jeff King <peff@peff.net>

The http_init function takes a "struct remote". Part of its
initialization procedure is to look at the remote's url and
grab some auth-related parameters. However, using the url
included in the remote is:

  - wrong; the remote-curl helper may have a separate,
    unrelated URL (e.g., from remote.*.pushurl). Looking at
    the remote's configured url is incorrect.

  - incomplete; http-fetch doesn't have a remote, so passes
    NULL. So http_init never gets to see the URL we are
    actually going to use.

  - cumbersome; http-push has a similar problem to
    http-fetch, but actually builds a fake remote just to
    pass in the URL.

Instead, let's just add a separate URL parameter to
http_init, and all three callsites can pass in the
appropriate information.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http-fetch.c  |    2 +-
 http-push.c   |   10 +---------
 http.c        |    8 ++++----
 http.h        |    2 +-
 remote-curl.c |    2 +-
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/http-fetch.c b/http-fetch.c
index 3af4c71..e341872 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -63,7 +63,7 @@ int main(int argc, const char **argv)
 
 	git_config(git_default_config, NULL);
 
-	http_init(NULL);
+	http_init(NULL, url);
 	walker = get_http_walker(url);
 	walker->get_tree = get_tree;
 	walker->get_history = get_history;
diff --git a/http-push.c b/http-push.c
index 6e8f6d0..ecbfae5 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1747,7 +1747,6 @@ int main(int argc, char **argv)
 	int i;
 	int new_refs;
 	struct ref *ref, *local_refs;
-	struct remote *remote;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -1821,14 +1820,7 @@ int main(int argc, char **argv)
 
 	memset(remote_dir_exists, -1, 256);
 
-	/*
-	 * Create a minimum remote by hand to give to http_init(),
-	 * primarily to allow it to look at the URL.
-	 */
-	remote = xcalloc(sizeof(*remote), 1);
-	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
-	remote->url[remote->url_nr++] = repo->url;
-	http_init(remote);
+	http_init(NULL, repo->url);
 
 #ifdef USE_CURL_MULTI
 	is_running_queue = 0;
diff --git a/http.c b/http.c
index 149e116..b181c8a 100644
--- a/http.c
+++ b/http.c
@@ -367,7 +367,7 @@ static void set_from_env(const char **var, const char *envname)
 		*var = val;
 }
 
-void http_init(struct remote *remote)
+void http_init(struct remote *remote, const char *url)
 {
 	char *low_speed_limit;
 	char *low_speed_time;
@@ -431,11 +431,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
 
-	if (remote && remote->url && remote->url[0]) {
-		http_auth_init(remote->url[0]);
+	if (url) {
+		http_auth_init(url);
 		if (!ssl_cert_password_required &&
 		    getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
-		    !prefixcmp(remote->url[0], "https://"))
+		    !prefixcmp(url, "https://"))
 			ssl_cert_password_required = 1;
 	}
 
diff --git a/http.h b/http.h
index 0bf8592..3c332a9 100644
--- a/http.h
+++ b/http.h
@@ -86,7 +86,7 @@ extern void add_fill_function(void *data, int (*fill)(void *));
 extern void step_active_slots(void);
 #endif
 
-extern void http_init(struct remote *remote);
+extern void http_init(struct remote *remote, const char *url);
 extern void http_cleanup(void);
 
 extern int data_received;
diff --git a/remote-curl.c b/remote-curl.c
index 6c24ab1..d4d0910 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -850,7 +850,7 @@ int main(int argc, const char **argv)
 
 	url = strbuf_detach(&buf, NULL);
 
-	http_init(remote);
+	http_init(remote, url);
 
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF)
-- 
1.7.7.338.g0156b

Re: [PATCH 0/6] http-auth-early

[Jeff King]

On Fri, Oct 14, 2011 at 09:40:34AM +0200, Michael J Gruber wrote:

> Here are the early parts of Jeff's http-auth-keyring series.
> It contains only parts which are not using the credential API (which
> is still under discussion), so that this can go in (and help users)
> and alleviates the pressure on the credential discussion:
> 
> Early bits with cleanups to http.c.
> Cherry-picked bit for improved prompts ("Username for ..." etc.)
> Cherry-pickes bit for using configured pushurls.
> 
> I tried to pick/resolve in a way which should help rebasing Jeff's series
> on top of this.

Thanks for working on this. One of my intended tasks for today is to
rebase my series, so it is nice to wake up to half of the work done. :)

> Jeff King (5):
>   url: decode buffers that are not NUL-terminated
>   improve httpd auth tests
>   remote-curl: don't retry auth failures with dumb protocol
>   http: retry authentication failures for all http requests
>   http_init: accept separate URL parameter
> 
> Michael J Gruber (1):
>   http: use hostname in credential description

Your changes all look right. The naming of git_getpass_one in the
cherry-picked commit is a little odd without the rest of the series as
context. I would maybe have called it "git_getpass_with_description" or
something.

-Peff

Re: [PATCH 0/6] http-auth-early

[Michael J Gruber]

Jeff King venit, vidit, dixit 14.10.2011 15:19:
> On Fri, Oct 14, 2011 at 09:40:34AM +0200, Michael J Gruber wrote:
> 
>> Here are the early parts of Jeff's http-auth-keyring series.
>> It contains only parts which are not using the credential API (which
>> is still under discussion), so that this can go in (and help users)
>> and alleviates the pressure on the credential discussion:
>>
>> Early bits with cleanups to http.c.
>> Cherry-picked bit for improved prompts ("Username for ..." etc.)
>> Cherry-pickes bit for using configured pushurls.
>>
>> I tried to pick/resolve in a way which should help rebasing Jeff's series
>> on top of this.
> 
> Thanks for working on this. One of my intended tasks for today is to
> rebase my series, so it is nice to wake up to half of the work done. :)

Good morning :)

>> Jeff King (5):
>>   url: decode buffers that are not NUL-terminated
>>   improve httpd auth tests
>>   remote-curl: don't retry auth failures with dumb protocol
>>   http: retry authentication failures for all http requests
>>   http_init: accept separate URL parameter
>>
>> Michael J Gruber (1):
>>   http: use hostname in credential description
> 
> Your changes all look right. The naming of git_getpass_one in the
> cherry-picked commit is a little odd without the rest of the series as
> context. I would maybe have called it "git_getpass_with_description" or
> something.

git_getpass_my_life_will_be_short_and_ended_by_credentials

I don't care. In fact, I wasn't sure whether I should I even change the
author on this one. It's not a straight resolution and does involve
choices, but the meat is from your series.

Michael

Re: [PATCH 0/6] http-auth-early

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Your changes all look right. The naming of git_getpass_one in the
> cherry-picked commit is a little odd without the rest of the series as
> context. I would maybe have called it "git_getpass_with_description" or
> something.

Thanks both; will queue with that updated function name.

Re: [PATCH] http_init: accept separate URL parameter

[Tay Ray Chuan]

On Thu, Oct 13, 2011 at 5:43 AM, Jeff King <peff@peff.net> wrote:
> The http_init function takes a "struct remote". Part of its
> initialization procedure is to look at the remote's url and
> grab some auth-related parameters. However, using the url
> included in the remote is:
>
>  - wrong; the remote-curl helper may have a separate,
>    unrelated URL (e.g., from remote.*.pushurl). Looking at
>    the remote's configured url is incorrect.
>
>  - incomplete; http-fetch doesn't have a remote, so passes
>    NULL. So http_init never gets to see the URL we are
>    actually going to use.
>
>  - cumbersome; http-push has a similar problem to
>    http-fetch, but actually builds a fake remote just to
>    pass in the URL.
>
> Instead, let's just add a separate URL parameter to
> http_init, and all three callsites can pass in the
> appropriate information.
>
> Signed-off-by: Jeff King <peff@peff.net>

This is excellent.

  Acked-by: Tay Ray Chuan <rctay89@gmail.com>

-- 
Cheers,
Ray Chuan