threads / discuss / 28567

Git ksshaskpass to play nice with https and kwallet

Subject: Git ksshaskpass to play nice with https and kwallet

## tl;dr

30 messages between Oct 4, 2011 and Oct 14, 2011.

replies: 29people: 4as markdown or json

Michael J Gruber· Oct 4, 2011, 10:19 UTC · lore

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
Jeff King· Oct 4, 2011, 10:50 UTC · re: Michael J Gruber · lore

Re: Git ksshaskpass to play nice with https and kwallet

On Tue, Oct 04, 2011 at 12:19:59PM +0200, Michael J Gruber wrote:
Show 12 quoted lines
> 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
Michael J Gruber· Oct 4, 2011, 11:27 UTC · re: Jeff King · lore

Re: Git ksshaskpass to play nice with https and kwallet

Jeff King venit, vidit, dixit 04.10.2011 12:50:
Show 18 quoted lines
> 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 ;)
Show 11 quoted lines
> 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

Jeff King· Oct 4, 2011, 11:37 UTC · re: Michael J Gruber · lore

Re: Git ksshaskpass to play nice with https and kwallet

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?

Show 5 quoted lines
> 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
Michael J Gruber· Oct 4, 2011, 12:12 UTC · re: Jeff King · lore

Re: Git ksshaskpass to play nice with https and kwallet

Jeff King venit, vidit, dixit 04.10.2011 13:37:
Show 29 quoted lines
> 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.

Show 5 quoted lines
> 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.
Show 8 quoted lines
> 
>   $ 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
Jeff King· Oct 4, 2011, 12:43 UTC · re: Michael J Gruber · lore

Re: Git ksshaskpass to play nice with https and kwallet

On Tue, Oct 04, 2011 at 02:12:02PM +0200, Michael J Gruber wrote:
Show 8 quoted lines
> > 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.

Show 12 quoted lines
> >   $ 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
Michael J Gruber· Oct 4, 2011, 18:49 UTC · re: Jeff King · lore

Re: Git ksshaskpass to play nice with https and kwallet

Jeff King venit, vidit, dixit 04.10.2011 14:43:
Show 15 quoted lines
> 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
Jeff King· Oct 5, 2011, 17:55 UTC · re: Michael J Gruber · lore

Re: Git ksshaskpass to play nice with https and kwallet

On Tue, Oct 04, 2011 at 08:49:55PM +0200, Michael J Gruber wrote:
Show 8 quoted lines
> 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
Jeff King· Oct 5, 2011, 18:01 UTC · re: Jeff King · lore

Re: Git ksshaskpass to play nice with https and kwallet

On Wed, Oct 05, 2011 at 01:55:36PM -0400, Jeff King wrote:
Show 23 quoted lines
> 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
Michael J Gruber· Oct 6, 2011, 06:33 UTC · re: Jeff King · lore

Re: Git ksshaskpass to play nice with https and kwallet

Jeff King venit, vidit, dixit 05.10.2011 20:01:
Show 45 quoted lines
> 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.
Show 10 quoted lines
> 
>   $ 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
Michael J Gruber· Oct 6, 2011, 13:15 UTC · re: Michael J Gruber · lore

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

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
Jeff King· Oct 6, 2011, 13:25 UTC · re: Michael J Gruber · lore

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

On Thu, Oct 06, 2011 at 03:15:59PM +0200, Michael J Gruber wrote:
Show 18 quoted lines
> 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
Jeff King· Oct 6, 2011, 13:37 UTC · re: Jeff King · lore

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

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
Michael J Gruber· Oct 12, 2011, 20:51 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 06.10.2011 15:37:
Show 9 quoted lines
> 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?

Show 129 quoted lines
> 
> -- >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)
Jeff King· Oct 12, 2011, 21:43 UTC · re: Michael J Gruber · lore

[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>
---
On Wed, Oct 12, 2011 at 10:51:20PM +0200, Michael J Gruber wrote:
Show 7 quoted lines
> > 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
Jeff King· Oct 12, 2011, 21:46 UTC · re: Jeff King · lore

Re: [PATCH] http_init: accept separate URL parameter

On Wed, Oct 12, 2011 at 05:43:16PM -0400, Jeff King wrote:
Show 22 quoted lines
> 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
Junio C Hamano· Oct 12, 2011, 22:38 UTC · re: Jeff King · lore

Re: [PATCH] http_init: accept separate URL parameter

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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?

Jeff King· Oct 12, 2011, 22:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] http_init: accept separate URL parameter

On Wed, Oct 12, 2011 at 03:38:27PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> 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
Michael J Gruber· Oct 13, 2011, 07:26 UTC · re: Jeff King · lore

Re: [PATCH] http_init: accept separate URL parameter

Jeff King venit, vidit, dixit 13.10.2011 00:46:
Show 28 quoted lines
> 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
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

[PATCH 0/6] http-auth-early

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
Jeff King· Oct 14, 2011, 13:19 UTC · re: Michael J Gruber · lore

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

On Fri, Oct 14, 2011 at 09:40:34AM +0200, Michael J Gruber wrote:
Show 11 quoted lines
> 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. :)

Show 9 quoted lines
> 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
Michael J Gruber· Oct 14, 2011, 13:24 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 14.10.2011 15:19:
Show 16 quoted lines
> 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 :)
Show 14 quoted lines
>> 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
Junio C Hamano· Oct 14, 2011, 18:59 UTC · re: Jeff King · lore

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

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.
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

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

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
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

[PATCH 2/6] improve httpd auth tests

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
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

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

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
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

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

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
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

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

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
Michael J Gruber· Oct 14, 2011, 07:40 UTC · re: Michael J Gruber · lore

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

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
Tay Ray Chuan· Oct 13, 2011, 02:06 UTC · re: Jeff King · lore

Re: [PATCH] http_init: accept separate URL parameter

On Thu, Oct 13, 2011 at 5:43 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> 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

← back to recent threads