Git ksshaskpass to play nice with https and kwallet

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

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

Jeff King venit, vidit, dixit 04.10.2011 12:50:

Don't worry ;)

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

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?

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 '[email protected]':

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 '[email protected]':

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

Jeff King venit, vidit, dixit 04.10.2011 13:37:

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.

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

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

https://[email protected]

with last week's next, at least.

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 '[email protected]' the key for the password, whether the username comes from the wallet or from the config. (Again, why not from the URL?)

Michael

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.

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://[email protected]/foo.git

but, if it's something like:

  $ git clone https://[email protected]/foo.git
  [months pass]

  $ git push
  Password for '[email protected]':

then it's a nice reminder.

-Peff

Jeff King venit, vidit, dixit 04.10.2011 14:43:

We seem to mean something different:

git config --get remote.bitbucket.pushurl
https://[email protected]/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

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://[email protected]/peff/git.git
  Password for 'github.com':

Using v1.7.7 produces similar results.

-Peff

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://[email protected]/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://[email protected]/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://[email protected]/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

Jeff King venit, vidit, dixit 05.10.2011 20:01:

Yep, I have a pushurl in config.

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

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

git-remote-https foo https://[email protected]

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.

---
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(-)

index b8cf45a..1fa396a 100644
@@ -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

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.

---
 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(-)

index 3af4c71..e341872 100644
@@ -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;

index 376331a..215b640 100644
@@ -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;

index b2ae8de..d9f9938 100644
@@ -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;
 	}
 

index 0bf8592..3c332a9 100644
@@ -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;

index 69831e9..33d3d8c 100644
@@ -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

Jeff King venit, vidit, dixit 06.10.2011 15:37:

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?

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.

---

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://[email protected]/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(-)

index 3af4c71..e341872 100644
@@ -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;

index 6e8f6d0..ecbfae5 100644
@@ -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;

index d6b2d78..65d3aa7 100644
@@ -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;
 	}
 

index 0bf8592..3c332a9 100644
@@ -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;

index 6c24ab1..d4d0910 100644
@@ -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

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

-Peff

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

Jeff King venit, vidit, dixit 13.10.2011 00:46:

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

From: Jeff King <[email protected]>

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.

---
 http.c |   27 ++++-----------------------
 url.c  |   26 ++++++++++++++++++--------
 url.h  |    1 +
 3 files changed, 23 insertions(+), 31 deletions(-)

index a1ea3db..c93716c 100644
@@ -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));
 	}
 }
 

index 3e06fd3..389d9da 100644
@@ -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)

index 7100e32..abdaf6f 100644
@@ -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

From: Jeff King <[email protected]>

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.

---
 t/lib-httpd.sh        |   10 +++++---
 t/t5550-http-fetch.sh |   51 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 8 deletions(-)

index b8996a3..f7dc078 100644
@@ -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%[email protected]:$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%[email protected]:$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

1.7.7.338.g0156b

From: Jeff King <[email protected]>

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.

---
 remote-curl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index faaeda4..6c24ab1 100644
@@ -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

From: Jeff King <[email protected]>

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.

---
 http.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

index c93716c..89e3cf4 100644
@@ -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

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 <[email protected]>

---
 http.c                |   41 +++++++++++++++++++++++++++++++++--------
 t/t5550-http-fetch.sh |    4 ++--
 2 files changed, 35 insertions(+), 10 deletions(-)

index 89e3cf4..149e116 100644
@@ -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;
 			}

index ed4db09..d1ab4d0 100755
@@ -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

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

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. :)

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

Jeff King venit, vidit, dixit 14.10.2011 15:19:

Good morning :)

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

Thanks both; will queue with that updated function name.

From: Jeff King <[email protected]>

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.

---
 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(-)

index 3af4c71..e341872 100644
@@ -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;

index 6e8f6d0..ecbfae5 100644
@@ -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;

index 149e116..b181c8a 100644
@@ -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;
 	}
 

index 0bf8592..3c332a9 100644
@@ -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;

index 6c24ab1..d4d0910 100644
@@ -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

This is excellent.

Cheers,
Ray Chuan