credential helpers

Here's the latest re-roll of the credential helpers series. I think this one is probably ready to go to 'next'.

It's rebased on the latest tip of 'master' (applying it to an older commit will get you a minor textual conflict in strbuf.c). It incorporates the erase-safety we discussed, fixes a few commit message typos, and tweaks the test scripts to make testing the external OS X helper a little easier.

[01/13]: test-lib: add test_config_global variant [02/13]: t5550: fix typo
[03/13]: introduce credentials API
[04/13]: credential: add function for parsing url components [05/13]: http: use credential API to get passwords [06/13]: credential: apply helper config
[07/13]: credential: add credential.*.username
[08/13]: credential: make relevance of http path configurable [09/13]: docs: end-user documentation for the credential subsystem [10/13]: credentials: add "cache" helper
[11/13]: strbuf: add strbuf_add*_urlencode
[12/13]: credentials: add "store" helper
[13/13]: t: add test harness for external credential helpers

-Peff

The point of test_config is to simultaneously set a config variable and register its cleanup handler, like:

test_config core.foo bar

However, it stupidly assumes that $1 contained the name of the variable, which means it won't work for:

  test_config --global core.foo bar

We could try to parse the command-line ourselves and figure out which parts need to be fed to test_unconfig. But since this is likely the most common variant, it's much simpler and less error-prone to simply add a new function.

---
 t/test-lib.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

index bdd9513..160479b 100644
@@ -379,6 +379,11 @@ test_config () {
 	git config "$@"
 }
 
+test_config_global () {
+	test_when_finished "test_unconfig --global '$1'" &&
+	git config --global "$@"
+}
+
 # Use test_set_prereq to tell that a particular prerequisite is available.
 # The prerequisite can later be checked for in two ways:
 #

1.7.8.rc2.40.gaf387

This didn't have an impact, because it was just setting up an "expect" file that happened to be identical to the one in the test before it.

---
 t/t5550-http-fetch.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 311a33c..3d6e871 100755
@@ -66,7 +66,7 @@ test_expect_success 'cloning password-protected repository can fail' '
 
 test_expect_success 'http auth can use user/pass in URL' '
 	>askpass-query &&
-	echo wrong >askpass-reponse &&
+	echo wrong >askpass-response &&
 	git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none &&
 	test_cmp askpass-expect-none askpass-query
 '

1.7.8.rc2.40.gaf387

There are a few places in git that need to get a username and password credential from the user; the most notable one is HTTP authentication for smart-http pushing.

Right now the only choices for providing credentials are to put them plaintext into your ~/.netrc, or to have git prompt you (either on the terminal or via an askpass program). The former is not very secure, and the latter is not very convenient.

Unfortunately, there is no "always best" solution for password management. The details will depend on the tradeoff you want between security and convenience, as well as how git can integrate with other security systems (e.g., many operating systems provide a keychain or password wallet for single sign-on).

This patch provides an abstract notion of credentials as a data item, and provides three basic operations:

- fill (i.e., acquire from external storage or from the user)

- approve (mark a credential as "working" for further storage)

- reject (mark a credential as "not working", so it can be removed from storage)

These operations can be backed by external helper processes that interact with system- or user-specific secure storage.

---
 .gitignore                                  |    1 +
 Documentation/technical/api-credentials.txt |  241 +++++++++++++++++++++++++++
 Makefile                                    |    3 +
 credential.c                                |  234 ++++++++++++++++++++++++++
 credential.h                                |   28 +++
 t/lib-credential.sh                         |   33 ++++
 t/t0300-credentials.sh                      |  195 ++++++++++++++++++++++
 test-credential.c                           |   38 +++++
 8 files changed, 773 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/technical/api-credentials.txt
 create mode 100644 credential.c
 create mode 100644 credential.h
 create mode 100755 t/lib-credential.sh
 create mode 100755 t/t0300-credentials.sh
 create mode 100644 test-credential.c

index 8572c8c..7d2fefc 100644
@@ -167,6 +167,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /test-chmtime
+/test-credential
 /test-ctype
 /test-date
 /test-delta

index ed82320..d69f1dd 100644
@@ -431,6 +431,7 @@ PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-credential
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta

@@ -525,6 +526,7 @@ LIB_H += compat/win32/poll.h
 LIB_H += compat/win32/dirent.h
 LIB_H += connected.h
 LIB_H += convert.h
+LIB_H += credential.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h

@@ -611,6 +613,7 @@ LIB_OBJS += connect.o
 LIB_OBJS += connected.o
 LIB_OBJS += convert.o
 LIB_OBJS += copy.o
+LIB_OBJS += credential.o
 LIB_OBJS += csum-file.o
 LIB_OBJS += ctype.o
 LIB_OBJS += date.o

new file mode 100644
index 0000000..2ea7d49
@@ -0,0 +1,28 @@
+#ifndef CREDENTIAL_H
+#define CREDENTIAL_H
+
+#include "string-list.h"
+
+struct credential {
+	struct string_list helpers;
+	unsigned approved:1;
+
+	char *username;
+	char *password;
+	char *protocol;
+	char *host;
+	char *path;
+};
+
+#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }
+
+void credential_init(struct credential *);
+void credential_clear(struct credential *);
+
+void credential_fill(struct credential *);
+void credential_approve(struct credential *);
+void credential_reject(struct credential *);
+
+int credential_read(struct credential *, FILE *);
+
+#endif /* CREDENTIAL_H */

new file mode 100755
index 0000000..54ae1f4
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+# Try a set of credential helpers; the expected stdin,
+# stdout and stderr should be provided on stdin,
+# separated by "--".
+check() {
+	read_chunk >stdin &&
+	read_chunk >expect-stdout &&
+	read_chunk >expect-stderr &&
+	test-credential "$@" <stdin >stdout 2>stderr &&
+	test_cmp expect-stdout stdout &&
+	test_cmp expect-stderr stderr
+}
+
+read_chunk() {
+	while read line; do
+		case "$line" in
+		--) break ;;
+		*) echo "$line" ;;
+		esac
+	done
+}
+
+
+cat >askpass <<\EOF
+#!/bin/sh
+echo >&2 askpass: $*
+what=`echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z`
+echo "askpass-$what"
+EOF
+chmod +x askpass
+GIT_ASKPASS="$PWD/askpass"
+export GIT_ASKPASS

new file mode 100644
index 0000000..dee200e
@@ -0,0 +1,38 @@
+#include "cache.h"
+#include "credential.h"
+#include "string-list.h"
+
+static const char usage_msg[] =
+"test-credential <fill|approve|reject> [helper...]";
+
+int main(int argc, const char **argv)
+{
+	const char *op;
+	struct credential c = CREDENTIAL_INIT;
+	int i;
+
+	op = argv[1];
+	if (!op)
+		usage(usage_msg);
+	for (i = 2; i < argc; i++)
+		string_list_append(&c.helpers, argv[i]);
+
+	if (credential_read(&c, stdin) < 0)
+		die("unable to read credential from stdin");
+
+	if (!strcmp(op, "fill")) {
+		credential_fill(&c);
+		if (c.username)
+			printf("username=%s\n", c.username);
+		if (c.password)
+			printf("password=%s\n", c.password);
+	}
+	else if (!strcmp(op, "approve"))
+		credential_approve(&c);
+	else if (!strcmp(op, "reject"))
+		credential_reject(&c);
+	else
+		usage(usage_msg);
+
+	return 0;
+}

1.7.8.rc2.40.gaf387

A question: does it work also for access via SSH, either without public key set up (i.e. 'keyboard-interactive'), or with encrypted private key without ssh-agent set up?

It would probably require URL i.e. ssh://git.example.com/srv/scm/repo.git or git+ssh://git.example.com/srv/scm/repo.git and not scp-like address i.e. [email protected]:/srv/scm/repo.git, isn't it?

Jakub Narębski

No. ssh handles its own password querying, and contacts the user directly via the terminal. And there's not much point in using a password helper with ssh; if you don't want to type your password, set up a key and use ssh-agent.

It's not a matter of recognizing the URL; it's that we hand off the authentication problem to ssh, which takes care of it entirely itself.

-Peff

All of the components of a credential struct can be found in a URL. For example, the URL:

http://foo:[email protected]/repo.git

contains:

protocol=http
host=example.com
path=repo.git
username=foo
password=bar

We want to be able to turn URLs into broken-down credential structs so that we know two things:

1. Which parts of the username/password we still need

2. What the context of the request is (for prompting or as a key for storing credentials).

This code is based on http_auth_init in http.c, but needed a few modifications in order to get all of the components that the credential object is interested in.

Once the http code is switched over to the credential API, then http_auth_init can just go away.

---
 Documentation/technical/api-credentials.txt |    4 ++
 credential.c                                |   52 +++++++++++++++++++++++++++
 credential.h                                |    1 +
 3 files changed, 57 insertions(+), 0 deletions(-)

index f624aef..21ca6a2 100644
@@ -67,6 +67,10 @@ Functions
 	that they may store the result to be used again.  Any errors
 	from helpers are ignored.
 
+`credential_from_url`::
+
+	Parse a URL into broken-down credential fields.
+
 Example
 -------
 

index 2ea7d49..8a6d162 100644
@@ -24,5 +24,6 @@ struct credential {
 void credential_reject(struct credential *);
 
 int credential_read(struct credential *, FILE *);
+void credential_from_url(struct credential *, const char *url);
 
 #endif /* CREDENTIAL_H */

1.7.8.rc2.40.gaf387

This patch converts the http code to use the new credential API, both for http authentication as well as for getting certificate passwords.

Most of the code change is simply variable naming (the passwords are now contained inside the credential struct) or deletion of obsolete code (the credential code handles URL parsing and prompting for us).

The behavior should be the same, with one exception: the credential code will prompt with a description based on the credential components. Therefore, the old prompt of:

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

now looks like:

Username for 'https://example.com/repo.git':
Password for 'https://[email protected]/repo.git':

Note that we include more information in each line, specifically:

1. We now include the protocol. While more noisy, this is an important part of knowing what you are accessing (especially if you care about http vs https).

2. We include the username in the password prompt. This is not a big deal when you have just been prompted for it, but the username may also come from the remote's URL (and after future patches, from configuration or credential helpers). In that case, it's a nice reminder of the user for which you're giving the password.

3. We include the path component of the URL. In many cases, the user won't care about this and it's simply noise (i.e., they'll use the same credential for a whole site). However, that is part of a larger question, which is whether path components should be part of credential context, both for prompting and for lookup by storage helpers. That issue will be addressed as a whole in a future patch.

Similarly, for unlocking certificates, we used to say:

Certificate Password for 'example.com':

and we now say:

Password for 'cert:///path/to/certificate':

Showing the path to the client certificate makes more sense, as that is what you are unlocking, not "example.com".

---
 http.c                |  113 +++++++++++--------------------------------------
 t/t5550-http-fetch.sh |   38 ++++++++++++-----
 2 files changed, 52 insertions(+), 99 deletions(-)

index 44fcc4d..8e72664 100644
@@ -3,6 +3,7 @@
 #include "sideband.h"
 #include "run-command.h"
 #include "url.h"
+#include "credential.h"
 
 int active_requests;
 int http_is_verbose;

@@ -41,7 +42,7 @@
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
-static char *user_name, *user_pass, *description;
+static struct credential http_auth = CREDENTIAL_INIT;
 static const char *user_agent;
 
 #if LIBCURL_VERSION_NUM >= 0x071700

@@ -52,7 +53,7 @@
 #define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
 #endif
 
-static char *ssl_cert_password;
+static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 
 static struct curl_slist *pragma_header;

@@ -136,27 +137,6 @@ static void process_curl_messages(void)
 }
 #endif
 
-static char *git_getpass_with_description(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);
-	/*
-	 * NEEDSWORK: 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)) {

@@ -229,11 +209,11 @@ static int http_options(const char *var, const char *value, void *cb)
 
 static void init_curl_http_auth(CURL *result)
 {
-	if (user_name) {
+	if (http_auth.username) {
 		struct strbuf up = STRBUF_INIT;
-		if (!user_pass)
-			user_pass = xstrdup(git_getpass_with_description("Password", description));
-		strbuf_addf(&up, "%s:%s", user_name, user_pass);
+		credential_fill(&http_auth);
+		strbuf_addf(&up, "%s:%s",
+			    http_auth.username, http_auth.password);
 		curl_easy_setopt(result, CURLOPT_USERPWD,
 				 strbuf_detach(&up, NULL));
 	}

@@ -241,18 +221,14 @@ static void init_curl_http_auth(CURL *result)
 
 static int has_cert_password(void)
 {
-	if (ssl_cert_password != NULL)
-		return 1;
 	if (ssl_cert == NULL || ssl_cert_password_required != 1)
 		return 0;
-	/* Only prompt the user once. */
-	ssl_cert_password_required = -1;
-	ssl_cert_password = git_getpass_with_description("Certificate Password", description);
-	if (ssl_cert_password != NULL) {
-		ssl_cert_password = xstrdup(ssl_cert_password);
-		return 1;
-	} else
-		return 0;
+	if (!cert_auth.password) {
+		cert_auth.protocol = xstrdup("cert");
+		cert_auth.path = xstrdup(ssl_cert);
+		credential_fill(&cert_auth);
+	}
+	return 1;
 }
 
 static CURL *get_curl_handle(void)

@@ -279,7 +255,7 @@ static int has_cert_password(void)
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-		curl_easy_setopt(result, CURLOPT_KEYPASSWD, ssl_cert_password);
+		curl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password);
 #if LIBCURL_VERSION_NUM >= 0x070903
 	if (ssl_key != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);

@@ -321,42 +297,6 @@ static int has_cert_password(void)
 	return result;
 }
 
-static void http_auth_init(const char *url)
-{
-	const char *at, *colon, *cp, *slash, *host;
-
-	cp = strstr(url, "://");
-	if (!cp)
-		return;
-
-	/*
-	 * Ok, the URL looks like "proto://something".  Which one?
-	 * "proto://<user>:<pass>@<host>/...",
-	 * "proto://<user>@<host>/...", or just
-	 * "proto://<host>/..."?
-	 */
-	cp += 3;
-	at = strchr(cp, '@');
-	colon = strchr(cp, ':');
-	slash = strchrnul(cp, '/');
-	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)
 {
 	const char *val = getenv(envname);

@@ -429,7 +369,7 @@ void http_init(struct remote *remote, const char *url)
 		curl_ftp_no_epsv = 1;
 
 	if (url) {
-		http_auth_init(url);
+		credential_from_url(&http_auth, url);
 		if (!ssl_cert_password_required &&
 		    getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
 		    !prefixcmp(url, "https://"))

@@ -478,10 +418,10 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
-	if (ssl_cert_password != NULL) {
-		memset(ssl_cert_password, 0, strlen(ssl_cert_password));
-		free(ssl_cert_password);
-		ssl_cert_password = NULL;
+	if (cert_auth.password != NULL) {
+		memset(cert_auth.password, 0, strlen(cert_auth.password));
+		free(cert_auth.password);
+		cert_auth.password = NULL;
 	}
 	ssl_cert_password_required = 0;
 }

@@ -837,17 +777,11 @@ static int http_request(const char *url, void *result, int target, int options)
 		else if (missing_target(&results))
 			ret = HTTP_MISSING_TARGET;
 		else if (results.http_code == 401) {
-			if (user_name && user_pass) {
+			if (http_auth.username && http_auth.password) {
+				credential_reject(&http_auth);
 				ret = HTTP_NOAUTH;
 			} else {
-				/*
-				 * git_getpass is needed here because its very likely stdin/stdout are
-				 * pipes to our parent process.  So we instead need to use /dev/tty,
-				 * but that is non-portable.  Using git_getpass() can at least be stubbed
-				 * on other platforms with a different implementation if/when necessary.
-				 */
-				if (!user_name)
-					user_name = xstrdup(git_getpass_with_description("Username", description));
+				credential_fill(&http_auth);
 				init_curl_http_auth(slot->curl);
 				ret = HTTP_REAUTH;
 			}

@@ -866,6 +800,9 @@ static int http_request(const char *url, void *result, int target, int options)
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 
+	if (ret == HTTP_OK)
+		credential_approve(&http_auth);
+
 	return ret;
 }
 

index 3d6e871..398a2d2 100755
@@ -49,40 +49,56 @@ test_expect_success 'setup askpass helpers' '
 	EOF
 	chmod +x askpass &&
 	GIT_ASKPASS="$PWD/askpass" &&
-	export GIT_ASKPASS &&
-	>askpass-expect-none &&
-	echo "askpass: Password for '\''$HTTPD_DEST'\'': " >askpass-expect-pass &&
-	{ echo "askpass: Username for '\''$HTTPD_DEST'\'': " &&
-	  cat askpass-expect-pass
-	} >askpass-expect-both
-'
+	export GIT_ASKPASS
+'
+
+expect_askpass() {
+	dest=$HTTPD_DEST/auth/repo.git
+	{
+		case "$1" in
+		none)
+			;;
+		pass)
+			echo "askpass: Password for 'http://$2@$dest': "
+			;;
+		both)
+			echo "askpass: Username for 'http://$dest': "
+			echo "askpass: Password for 'http://$2@$dest': "
+			;;
+		*)
+			false
+			;;
+		esac
+	} >askpass-expect &&
+	test_cmp askpass-expect askpass-query
+}
 
 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
+	expect_askpass both wrong
 '
 
 test_expect_success 'http auth can use user/pass in URL' '
 	>askpass-query &&
 	echo wrong >askpass-response &&
 	git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none &&
-	test_cmp askpass-expect-none askpass-query
+	expect_askpass none
 '
 
 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
+	expect_askpass pass user@host
 '
 
 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
+	expect_askpass both user@host
 '
 
 test_expect_success 'fetch changes via http' '

1.7.8.rc2.40.gaf387

The functionality for credential storage helpers is already there; we just need to give the users a way to turn it on. This patch provides a "credential.helper" configuration variable which allows the user to provide one or more helper strings.

Rather than simply matching credential.helper, we will also compare URLs in subsection headings to the current context. This means you can apply configuration to a subset of credentials. For example:

[credential "https://example.com"]
helper = foo

would match a request for "https://example.com/foo.git", but not one for "https://kernel.org/foo.git".

This is overkill for the "helper" variable, since users are unlikely to want different helpers for different sites (and since helpers run arbitrary code, they could do the matching themselves anyway).

However, future patches will add new config variables where this extra feature will be more useful.

---
 credential.c           |   61 ++++++++++++++++++++++++++++++++++++++++++++++++
 credential.h           |    5 +++-
 t/t0300-credentials.sh |   42 +++++++++++++++++++++++++++++++++
 t/t5550-http-fetch.sh  |   12 +++++++++
 4 files changed, 119 insertions(+), 1 deletions(-)

index c349b9a..96be1c2 100644
@@ -22,6 +22,61 @@ void credential_clear(struct credential *c)
 	credential_init(c);
 }
 
+int credential_match(const struct credential *want,
+		     const struct credential *have)
+{
+#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
+	return CHECK(protocol) &&
+	       CHECK(host) &&
+	       CHECK(path) &&
+	       CHECK(username);
+#undef CHECK
+}
+
+static int credential_config_callback(const char *var, const char *value,
+				      void *data)
+{
+	struct credential *c = data;
+	const char *key, *dot;
+
+	key = skip_prefix(var, "credential.");
+	if (!key)
+		return 0;
+
+	if (!value)
+		return config_error_nonbool(var);
+
+	dot = strrchr(key, '.');
+	if (dot) {
+		struct credential want = CREDENTIAL_INIT;
+		char *url = xmemdupz(key, dot - key);
+		int matched;
+
+		credential_from_url(&want, url);
+		matched = credential_match(&want, c);
+
+		credential_clear(&want);
+		free(url);
+
+		if (!matched)
+			return 0;
+		key = dot + 1;
+	}
+
+	if (!strcmp(key, "helper"))
+		string_list_append(&c->helpers, value);
+
+	return 0;
+}
+
+static void credential_apply_config(struct credential *c)
+{
+	if (c->configured)
+		return;
+	git_config(credential_config_callback, c);
+	c->configured = 1;
+}
+
 static void credential_describe(struct credential *c, struct strbuf *out)
 {
 	if (!c->protocol)

@@ -195,6 +250,8 @@ void credential_fill(struct credential *c)
 	if (c->username && c->password)
 		return;
 
+	credential_apply_config(c);
+
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
 		if (c->username && c->password)

@@ -215,6 +272,8 @@ void credential_approve(struct credential *c)
 	if (!c->username || !c->password)
 		return;
 
+	credential_apply_config(c);
+
 	for (i = 0; i < c->helpers.nr; i++)
 		credential_do(c, c->helpers.items[i].string, "store");
 	c->approved = 1;

@@ -224,6 +283,8 @@ void credential_reject(struct credential *c)
 {
 	int i;
 
+	credential_apply_config(c);
+
 	for (i = 0; i < c->helpers.nr; i++)
 		credential_do(c, c->helpers.items[i].string, "erase");
 

index 8a6d162..e504272 100644
@@ -5,7 +5,8 @@
 
 struct credential {
 	struct string_list helpers;
-	unsigned approved:1;
+	unsigned approved:1,
+		 configured:1;
 
 	char *username;
 	char *password;

@@ -25,5 +26,7 @@ struct credential {
 
 int credential_read(struct credential *, FILE *);
 void credential_from_url(struct credential *, const char *url);
+int credential_match(const struct credential *have,
+		     const struct credential *want);
 
 #endif /* CREDENTIAL_H */

index 398a2d2..c59908f 100755
@@ -101,6 +101,18 @@ test_expect_success 'http auth can request both user and pass' '
 	expect_askpass both user@host
 '
 
+test_expect_success 'http auth respects credential helper config' '
+	test_config_global credential.helper "!f() {
+		cat >/dev/null
+		echo username=user@host
+		echo password=user@host
+	}; f" &&
+	>askpass-query &&
+	echo wrong >askpass-response &&
+	git clone "$HTTPD_URL/auth/repo.git" clone-auth-helper &&
+	expect_askpass none
+'
+
 test_expect_success 'fetch changes via http' '
 	echo content >>file &&
 	git commit -a -m two &&

1.7.8.rc2.40.gaf387

Credential helpers can help users avoid having to type their username and password over and over. However, some users may not want a helper for their password, or they may be running a helper which caches for a short time. In this case, it is convenient to provide the non-secret username portion of their credential via config.

---
 credential.c           |    4 ++++
 t/t0300-credentials.sh |   13 +++++++++++++
 t/t5550-http-fetch.sh  |   16 ++++++++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

index 96be1c2..3c17ea1 100644
@@ -65,6 +65,10 @@ static int credential_config_callback(const char *var, const char *value,
 
 	if (!strcmp(key, "helper"))
 		string_list_append(&c->helpers, value);
+	else if (!strcmp(key, "username")) {
+		if (!c->username)
+			c->username = xstrdup(value);
+	}
 
 	return 0;
 }

index 42d0f5b..53e94bc 100755
@@ -234,4 +234,17 @@ test_expect_success 'do not match configured credential' '
 	EOF
 '
 
+test_expect_success 'pull username from config' '
+	test_config credential.https://example.com.username foo &&
+	check fill <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	username=foo
+	password=askpass-password
+	--
+	askpass: Password for '\''https://[email protected]'\'':
+	EOF
+'
+
 test_done

index c59908f..3262f90 100755
@@ -113,6 +113,22 @@ test_expect_success 'http auth respects credential helper config' '
 	expect_askpass none
 '
 
+test_expect_success 'http auth can get username from config' '
+	test_config_global "credential.$HTTPD_URL.username" user@host &&
+	>askpass-query &&
+	echo user@host >askpass-response &&
+	git clone "$HTTPD_URL/auth/repo.git" clone-auth-user &&
+	expect_askpass pass user@host
+'
+
+test_expect_success 'configured username does not override URL' '
+	test_config_global "credential.$HTTPD_URL.username" wrong &&
+	>askpass-query &&
+	echo user@host >askpass-response &&
+	git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-user2 &&
+	expect_askpass pass user@host
+'
+
 test_expect_success 'fetch changes via http' '
 	echo content >>file &&
 	git commit -a -m two &&

1.7.8.rc2.40.gaf387

When parsing a URL into a credential struct, we carefully record each part of the URL, including the path on the remote host, and use the result as part of the credential context.

This had two practical implications:

1. Credential helpers which store a credential for later access are likely to use the "path" portion as part of the storage key. That means that a request to

https://example.com/foo.git

would not use the same credential that was stored in an earlier request for:

https://example.com/bar.git

2. The prompt shown to the user includes all relevant context, including the path.

In most cases, however, users will have a single password per host. The behavior in (1) will be inconvenient, and the prompt in (2) will be overly long.

This patch introduces a config option to toggle the relevance of http paths. When turned on, we use the path as before. When turned off, we drop the path component from the
context: helpers don't see it, and it does not appear in the prompt.

This is nothing you couldn't do with a clever credential helper at the start of your stack, like:

[credential "http://"]

	helper = "!f() { grep -v ^path= ; }; f"
	helper = your_real_helper

But doing this:

[credential]
useHttpPath = false

is way easier and more readable. Furthermore, since most users will want the "off" behavior, that is the new default. Users who want it "on" can set the variable (either for all credentials, or just for a subset using credential.*.useHttpPath).

---
 credential.c           |   14 ++++++++++++++
 credential.h           |    3 ++-
 t/t0300-credentials.sh |   29 +++++++++++++++++++++++++++++
 t/t5550-http-fetch.sh  |    2 +-
 4 files changed, 46 insertions(+), 2 deletions(-)

index 3c17ea1..a17eafe 100644
@@ -69,16 +69,30 @@ static int credential_config_callback(const char *var, const char *value,
 		if (!c->username)
 			c->username = xstrdup(value);
 	}
+	else if (!strcmp(key, "usehttppath"))
+		c->use_http_path = git_config_bool(var, value);
 
 	return 0;
 }
 
+static int proto_is_http(const char *s)
+{
+	if (!s)
+		return 0;
+	return !strcmp(s, "https") || !strcmp(s, "http");
+}
+
 static void credential_apply_config(struct credential *c)
 {
 	if (c->configured)
 		return;
 	git_config(credential_config_callback, c);
 	c->configured = 1;
+
+	if (!c->use_http_path && proto_is_http(c->protocol)) {
+		free(c->path);
+		c->path = NULL;
+	}
 }
 
 static void credential_describe(struct credential *c, struct strbuf *out)

index e504272..96ea41b 100644
@@ -6,7 +6,8 @@
 struct credential {
 	struct string_list helpers;
 	unsigned approved:1,
-		 configured:1;
+		 configured:1,
+		 use_http_path:1;
 
 	char *username;
 	char *password;

index 53e94bc..885af8f 100755
@@ -247,4 +247,33 @@ test_expect_success 'pull username from config' '
 	EOF
 '
 
+test_expect_success 'http paths can be part of context' '
+	check fill "verbatim foo bar" <<-\EOF &&
+	protocol=https
+	host=example.com
+	path=foo.git
+	--
+	username=foo
+	password=bar
+	--
+	verbatim: get
+	verbatim: protocol=https
+	verbatim: host=example.com
+	EOF
+	test_config credential.https://example.com.useHttpPath true &&
+	check fill "verbatim foo bar" <<-\EOF
+	protocol=https
+	host=example.com
+	path=foo.git
+	--
+	username=foo
+	password=bar
+	--
+	verbatim: get
+	verbatim: protocol=https
+	verbatim: host=example.com
+	verbatim: path=foo.git
+	EOF
+'
+
 test_done

index 3262f90..95a133d 100755
@@ -53,7 +53,7 @@ test_expect_success 'setup askpass helpers' '
 '
 
 expect_askpass() {
-	dest=$HTTPD_DEST/auth/repo.git
+	dest=$HTTPD_DEST
 	{
 		case "$1" in
 		none)

1.7.8.rc2.40.gaf387

Jeff King <[email protected]> writes:

[...]

Shouldn't this be 'usePath' or 'usePathComponent' or 'useRepositoryPath', etc.? Because if^W when remote helper for Subversion is complete, you could have svn://svnserve.example.com/path/to/repo as an URL... which would be not HTTP(S).

Jakub Narębski

It must be per-protocol, because paths will be relevant for some protocols (e.g., unlocking certificates in the local filesystem). So if anything, it would be "useNetworkPath" or something, if we wanted to unify svn and http. But it would also be OK to have a separate flag for http and svn, which is more flexible (and most users aren't going to set it at all, so they won't notice).

-Peff

The credential API and helper format is already defined in technical/api-credentials.txt. This presents the end-user view.

---
 Documentation/Makefile           |    1 +
 Documentation/config.txt         |   23 +++++
 Documentation/gitcredentials.txt |  171 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/gitcredentials.txt

index 304b31e..116f175 100644
@@ -7,6 +7,7 @@ MAN5_TXT=gitattributes.txt gitignore.txt gitmodules.txt githooks.txt \
 MAN7_TXT=gitcli.txt gittutorial.txt gittutorial-2.txt \
 	gitcvs-migration.txt gitcore-tutorial.txt gitglossary.txt \
 	gitdiffcore.txt gitnamespaces.txt gitrevisions.txt gitworkflows.txt
+MAN7_TXT += gitcredentials.txt
 
 MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
 MAN_XML=$(patsubst %.txt,%.xml,$(MAN_TXT))

index 8a7d2d4..c6630c7 100644
@@ -834,6 +834,29 @@ commit.template::
 	"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
 	specified user's home directory.
 
+credential.helper::
+	Specify an external helper to be called when a username or
+	password credential is needed; the helper may consult external
+	storage to avoid prompting the user for the credentials. See
+	linkgit:gitcredentials[7] for details.
+
+credential.useHttpPath::
+	When acquiring credentials, consider the "path" component of an http
+	or https URL to be important. Defaults to false. See
+	linkgit:gitcredentials[7] for more information.
+
+credential.username::
+	If no username is set for a network authentication, use this username
+	by default. See credential.<context>.* below, and
+	linkgit:gitcredentials[7].
+
+credential.<url>.*::
+	Any of the credential.* options above can be applied selectively to
+	some credentials. For example "credential.https://example.com.username"
+	would set the default username only for https connections to
+	example.com. See linkgit:gitcredentials[7] for details on how URLs are
+	matched.
+
 include::diff-config.txt[]
 
 difftool.<tool>.path::

1.7.8.rc2.40.gaf387

If you access repositories over smart-http using http authentication, then it can be annoying to have git ask you for your password repeatedly. We cache credentials in memory, of course, but git is composed of many small programs. Having to input your password for each one can be frustrating.

This patch introduces a credential helper that will cache passwords in memory for a short period of time.

---
Since the previous round, the tests have been tweaked to remove the use of the "ftp" protocol. While this worked just fine for our cache and store helpers, git doesn't actually generate protocol lines like that, and some helpers may not understand it (e.g., the OS X helper has to convert the protocol strings into numeric constants, and doesn't bother converting things that git doesn't generate). And we use the same battery of tests for the internal helpers as for external ones.

 .gitignore                                     |    2 +
 Documentation/git-credential-cache--daemon.txt |   26 +++
 Documentation/git-credential-cache.txt         |   77 +++++++
 Documentation/gitcredentials.txt               |   17 +-
 Makefile                                       |    3 +
 credential-cache--daemon.c                     |  269 ++++++++++++++++++++++++
 credential-cache.c                             |  120 +++++++++++
 git-compat-util.h                              |    1 +
 t/lib-credential.sh                            |  221 +++++++++++++++++++
 t/t0301-credential-cache.sh                    |   18 ++
 unix-socket.c                                  |   56 +++++
 unix-socket.h                                  |    7 +
 12 files changed, 812 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/git-credential-cache--daemon.txt
 create mode 100644 Documentation/git-credential-cache.txt
 create mode 100644 credential-cache--daemon.c
 create mode 100644 credential-cache.c
 create mode 100755 t/t0301-credential-cache.sh
 create mode 100644 unix-socket.c
 create mode 100644 unix-socket.h

index 7d2fefc..a6b0bd4 100644
@@ -30,6 +30,8 @@
 /git-commit-tree
 /git-config
 /git-count-objects
+/git-credential-cache
+/git-credential-cache--daemon
 /git-cvsexportcommit
 /git-cvsimport
 /git-cvsserver

new file mode 100644
index 0000000..11edc5a
@@ -0,0 +1,26 @@
+git-credential-cache--daemon(1)
+===============================
+
+NAME
+----
+git-credential-cache--daemon - temporarily store user credentials in memory
+
+SYNOPSIS
+--------
+[verse]
+git credential-cache--daemon <socket>
+
+DESCRIPTION
+-----------
+
+NOTE: You probably don't want to invoke this command yourself; it is
+started automatically when you use linkgit:git-credential-cache[1].
+
+This command listens on the Unix domain socket specified by `<socket>`
+for `git-credential-cache` clients. Clients may store and retrieve
+credentials. Each credential is held for a timeout specified by the
+client; once no credentials are held, the daemon exits.
+
+GIT
+---
+Part of the linkgit:git[1] suite

index 07f6596..4e3f860 100644
@@ -63,11 +63,18 @@ Credential helpers, on the other hand, are external programs from which git can
 request both usernames and passwords; they typically interface with secure
 storage provided by the OS or other programs.
 
-To use a helper, you must first select one to use.  Git does not yet
-include any credential helpers, but you may have third-party helpers
-installed; search for `credential-*` in the output of `git help -a`, and
-consult the documentation of individual helpers.  Once you have selected
-a helper, you can tell git to use it by putting its name into the
+To use a helper, you must first select one to use. Git currently
+includes the following helpers:
+
+cache::
+
+	Cache credentials in memory for a short period of time. See
+	linkgit:git-credential-cache[1] for details.
+
+You may also have third-party helpers installed; search for
+`credential-*` in the output of `git help -a`, and consult the
+documentation of individual helpers.  Once you have selected a helper,
+you can tell git to use it by putting its name into the
 credential.helper variable.
 
 1. Find a helper.

index d69f1dd..7ad2fe3 100644
@@ -427,6 +427,8 @@ PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
+PROGRAM_OBJS += credential-cache.o
+PROGRAM_OBJS += credential-cache--daemon.o
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 

@@ -702,6 +704,7 @@ LIB_OBJS += transport-helper.o
 LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
+LIB_OBJS += unix-socket.o
 LIB_OBJS += unpack-trees.o
 LIB_OBJS += url.o
 LIB_OBJS += usage.o

index 77062ed..8f3972c 100644
@@ -135,6 +135,7 @@
 #include <arpa/inet.h>
 #include <netdb.h>
 #include <pwd.h>
+#include <sys/un.h>
 #ifndef NO_INTTYPES_H
 #include <inttypes.h>
 #else

new file mode 100755
index 0000000..3a65f99
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='credential-cache tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+# don't leave a stale daemon running
+trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
+
+helper_test cache
+helper_test_timeout cache --timeout=1
+
+# we can't rely on our "trap" above working after test_done,
+# as test_done will delete the trash directory containing
+# our socket, leaving us with no way to access the daemon.
+git credential-cache exit
+
+test_done

new file mode 100644
index 0000000..e271aee
@@ -0,0 +1,7 @@
+#ifndef UNIX_SOCKET_H
+#define UNIX_SOCKET_H
+
+int unix_stream_connect(const char *path);
+int unix_stream_listen(const char *path);
+
+#endif /* UNIX_SOCKET_H */

1.7.8.rc2.40.gaf387

Hi,

[...]

This test is failing for me:

expecting success:
check fill $HELPER <<-\EOF
protocol=https
host=example.com
--
username=askpass-username password=askpass-password
--
askpass: Username for 'https://example.com':
askpass: Password for 'https://[email protected]':
EOF

--- expect-stderr 2012-01-10 00:07:02.398365248 +0000
+++ stderr 2012-01-10 00:07:02.418364996 +0000
@@ -1,2 +1,3 @@
+fatal: socket path is too long to fit in sockaddr
askpass: Username for 'https://example.com':
askpass: Password for 'https://[email protected]':
not ok - 1 helper (cache) has no existing data

I didn't notice until recently because typically the cwd is short. The sun_path[] array on this machine is 108 bytes; POSIX informs me that some platforms make it as small as 96 bytes and there's no guaranteed lower limit. The machines[*] triggering it were running tests in a chroot, hence the long path.

So you should be able to reproduce this with

longpath=foo/bar; # > 6 chars longpath=$longpath/$longpath/$longpath/$longpath; # > 24 longpath=$longpath/$longpath/$longpath/$longpath; # > 96 longpath=/tmp/$longpath/$longpath
mkdir -p $longpath
sh t0301-credential-cache.sh --root=$longpath

Ideas?
---
[*] https://buildd.debian.org/status/logs.php?pkg=git&ver=1%3A1.7.9~rc0-1

 credential-cache.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

index dc98372e..fd9304e6 100644
@@ -82,7 +82,7 @@ static void do_cache(const char *socket, const char *action, int timeout,
 
 int main(int argc, const char **argv)
 {
-	char *socket_path = NULL;
+	const char *socket_path = NULL;
 	int timeout = 900;
 	const char *op;
 	const char * const usage[] = {

@@ -102,10 +102,18 @@ int main(int argc, const char **argv)
 		usage_with_options(usage, options);
 	op = argv[0];
 
-	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
-	if (!socket_path)
-		die("unable to find a suitable socket path; use --socket");
+	if (!socket_path) {
+		char *home = expand_user_path("~");
+		if (!home)
+			die("unable to find a suitable socket path; use --socket");
+
+		if (!chdir(home))
+			socket_path = ".git-credential-cache/socket";
+		else if (errno == ENOENT && !strcmp(op, "exit"))
+			return 0;
+		else
+			die("cannot enter home directory");
+	}
 
 	if (!strcmp(op, "exit"))
 		do_cache(socket_path, op, timeout, 0);

1.7.8.2

Ugh. Some days I really love working on Unix systems. And then some days you run across junk like this.

Presumably Linux has kept to 108 to avoid breaking older programs. So it's not as if we can assume it will be changed soon, or just write this off as a limitation for old crappy systems.

Googling around, I've seen some indication that you can simply over-allocate the sockaddr_un and pass a longer length to connect. However that just seems to yield EINVAL on Linux (and even if it did work on Linux, I'd be surprised if it worked everywhere).

Which really leaves us with only one option: chdir and bind to a relative path, as you did below.

---
 unix-socket.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 69 insertions(+), 5 deletions(-)

index 84b1509..664782a 100644
@@ -9,27 +9,86 @@ static int unix_stream_socket(void)
 	return fd;
 }
 
-static void unix_sockaddr_init(struct sockaddr_un *sa, const char *path)
+static int chdir_len(const char *orig, int len)
+{
+	char *path = xmemdupz(orig, len);
+	int r = chdir(path);
+	free(path);
+	return r;
+}
+
+struct unix_sockaddr_context {
+	char orig_dir[PATH_MAX];
+};
+
+static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
+{
+	if (!ctx->orig_dir[0])
+		return;
+	/*
+	 * If we fail, we can't just return an error, since we have
+	 * moved the cwd of the whole process, which could confuse calling
+	 * code.  We are better off to just die.
+	 */
+	if (chdir(ctx->orig_dir) < 0)
+		die("unable to restore original working directory");
+}
+
+static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
+			      struct unix_sockaddr_context *ctx)
 {
 	int size = strlen(path) + 1;
-	if (size > sizeof(sa->sun_path))
-		die("socket path is too long to fit in sockaddr");
+
+	ctx->orig_dir[0] = '\0';
+	if (size > sizeof(sa->sun_path)) {
+		const char *slash = find_last_dir_sep(path);
+		const char *dir;
+		int dir_len;
+
+		if (!slash) {
+			errno = ENAMETOOLONG;
+			return -1;
+		}
+
+		dir = path;
+		dir_len = slash - path;
+
+		path = slash + 1;
+		size = strlen(path) + 1;
+		if (size > sizeof(sa->sun_path)) {
+			errno = ENAMETOOLONG;
+			return -1;
+		}
+
+		if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {
+			errno = ENAMETOOLONG;
+			return -1;
+		}
+		if (chdir_len(dir, dir_len) < 0)
+			return -1;
+	}
+
 	memset(sa, 0, sizeof(*sa));
 	sa->sun_family = AF_UNIX;
 	memcpy(sa->sun_path, path, size);
+	return 0;
 }
 
 int unix_stream_connect(const char *path)
 {
 	int fd;
 	struct sockaddr_un sa;
+	struct unix_sockaddr_context ctx;
 
-	unix_sockaddr_init(&sa, path);
+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+		return -1;
 	fd = unix_stream_socket();
 	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		unix_sockaddr_cleanup(&ctx);
 		close(fd);
 		return -1;
 	}
+	unix_sockaddr_cleanup(&ctx);
 	return fd;
 }
 

@@ -37,20 +96,25 @@ int unix_stream_listen(const char *path)
 {
 	int fd;
 	struct sockaddr_un sa;
+	struct unix_sockaddr_context ctx;
 
-	unix_sockaddr_init(&sa, path);
+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+		return -1;
 	fd = unix_stream_socket();
 
 	unlink(path);
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		unix_sockaddr_cleanup(&ctx);
 		close(fd);
 		return -1;
 	}
 
 	if (listen(fd, 5) < 0) {
+		unix_sockaddr_cleanup(&ctx);
 		close(fd);
 		return -1;
 	}
 
+	unix_sockaddr_cleanup(&ctx);
 	return fd;
 }

1.7.9.rc0.33.g15ced5

And I think this should go on top. You were lucky enough that I used a die() in the original code for your condition (because I thought it was a "this could never happen, right?" condition). Had it simply returned an error, the cache-daemon would have silently failed to do anything, which would have been much more confusing for you. :)

We probably should have done this as part of Clemens' 98c2924, but I didn't think of it then (but the same reasoning applies to both patches).

-- >8 --
Subject: [PATCH] credential-cache: report more daemon connection errors

Originally, this code remained relatively silent when we failed to connect to the cache. The idea was that it was simply a cache, and we didn't want to bother the user with temporary failures (the worst case is that we would simply ask their password again).

However, if you have a configuration failure or other problem, it is helpful for the daemon to report those problems. Git will happily ignore the failed error code, but the extra information to stderr can help the user diagnose the problem.

---
 credential-cache.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

index b15a9a7..1933018 100644
@@ -71,10 +71,14 @@ static void do_cache(const char *socket, const char *action, int timeout,
 			die_errno("unable to relay credential");
 	}
 
-	if (send_request(socket, &buf) < 0 && (flags & FLAG_SPAWN)) {
-		spawn_daemon(socket);
-		if (send_request(socket, &buf) < 0)
+	if (send_request(socket, &buf) < 0) {
+		if (errno != ENOENT)
 			die_errno("unable to connect to cache daemon");
+		if (flags & FLAG_SPAWN) {
+			spawn_daemon(socket);
+			if (send_request(socket, &buf) < 0)
+				die_errno("unable to connect to cache daemon");
+		}
 	}
 	strbuf_release(&buf);
 }

1.7.9.rc0.33.g15ced5

Thanks, these look sane.

Will queue both to 'next' aiming for 'master' before 1.7.9-rc1.

This actually has a minor regression, fixed below.

-- >8 --
Subject: [PATCH] credential-cache: ignore "connection refused" errors

The credential-cache helper will try to connect to its daemon over a unix socket. Originally, a failure to do so was silently ignored, and we would either give up (if performing a "get" or "erase" operation), or spawn a new daemon (for a "store" operation).

But since 8ec6c8d, we try to report more errors. We detect a missing daemon by checking for ENOENT on our connection attempt. If the daemon is missing, we continue as before (giving up or spawning a new daemon). For any other error, we die and report the problem.

However, checking for ENOENT is not sufficient for a missing daemon. We might also get ECONNREFUSED if a dead daemon process left a stale socket. This generally shouldn't happen, as the daemon cleans up after itself, but the daemon may not always be given a chance to do so (e.g., power loss, "kill -9").

The resulting state is annoying not just because the helper outputs an extra useless message, but because it actually blocks the helper from spawning a new daemon to replace the stale socket.

Fix it by checking for ECONNREFUSED.

---
If we really want to go belt-and-suspenders, the logic should perhaps be changed to:

  if (send_request(socket, &buf < 0) {
          /* if we're starting a new one, who cares why it didn't work */
          if (flags & FLAG_SPAWN) {
                  spawn_daemon(socket);
                  if (send_request(socket, &buf) < 0)
                          die_errno("unable to connect to spawned daemon");
          }
          /* otherwise, report any non-minor errors */
          else if(errno != ENOENT && errno != ECONNREFUSED)
                  die_errno("unable to connect to cache daemon");
          /* otherwise we are just missing the daemon, and we can ignore */
  }

but that implies there is some condition besides ENOENT and ECONNREFUSED where actually starting a new daemon (which will try to unlink whatever is there now!) would be a good idea. I'd rather be conservative and see if anybody reports a real-world case.

 credential-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 1933018..9a03792 100644
@@ -72,7 +72,7 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	}
 
 	if (send_request(socket, &buf) < 0) {
-		if (errno != ENOENT)
+		if (errno != ENOENT && errno != ECONNREFUSED)
 			die_errno("unable to connect to cache daemon");
 		if (flags & FLAG_SPAWN) {
 			spawn_daemon(socket);

1.7.9.rc0.33.gd3c17

Thanks, really appreciated.

A part of me worries that this assumption (3), and the additional assumption that nothing running concurrently cares about the cwd, might come back to bite us when the future (2) arrives. But I don't see a better approach.

Follow-on ideas: on platforms that support it, it would be nice to use

	ctx->orig_dirfd = open(".", O_RDONLY);
	if (ctx->orig_dirfd < 0)
		... error handling ...
	...
	if (ctx->orig_dirfd >= 0) {
		if (fchdir(ctx->orig_dirfd))
			die_errno("unable to restore original working directory");
		close(ctx->orig_dirfd);
	}

[...]

[...]

		if (chdir_len(dir, slash - dir))
			return -1;

The problem is that when future (2) arrives, a credential-cache specific hack won't be helping it at all. :)

To be honest, I don't really expect a lot of future unix-socket users. It's not portable, and most of our IPC happens over pipes. The design of the cache daemon is very specific in requiring a true many-clients-to-one-server model, but also caring deeply about access controls (making TCP sockets less good[1]).

[1] One could in theory do a loopback TCP socket and provide a random token read from an access-controlled file. But that ends up a lot more complicated and introduces new attack surfaces. Which is a bad thing for security-sensitive code like this.

Yeah, I started by using fchdir, but noticed that we don't use it anywhere else and didn't want to create a portability problem. It does fix the "somebody deleted your cwd while you were gone from it" problem. But if you have no cwd at all, the open call will still fail.

Still, it would be slightly more robust. I wonder how portable fchdir is in practice (I guess we could always fall back to the getcwd code path). Do you want to prepare a patch on top?

Ah, yeah. Originally I had written it so that "slash" didn't survive untouched to there, but in the current code, that would work.

Junio, if you haven't merged it to next yet, it might be worth squashing in the patch below. Otherwise I wouldn't worry about it.

Thanks for the review (and for, as usual, a well-written bug report).

---

index 91ed9dc..e8f19c6 100644
@@ -43,7 +43,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;
-		int dir_len;
 
 		if (!slash) {
 			errno = ENAMETOOLONG;

@@ -51,8 +50,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 		}
 
 		dir = path;
-		dir_len = slash - path;
-
 		path = slash + 1;
 		size = strlen(path) + 1;
 		if (size > sizeof(sa->sun_path)) {

@@ -64,7 +61,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 			errno = ENAMETOOLONG;
 			return -1;
 		}
-		if (chdir_len(dir, dir_len) < 0)
+		if (chdir_len(dir, slash - dir) < 0)
 			return -1;
 	}
 

I've been wanting to get around to doing something similar for setup.c for a while. I'm happy enough to forget about it for now. ;-)

Thanks again for the fix. Here's another quick nit.

-- >8 --
Subject: unix-socket: do not let close() or chdir() clobber errno during cleanup

unix_stream_connect and unix_stream_listen return -1 on error, with errno set by the failing underlying call to allow the caller to write a useful diagnosis.

Unfortunately the error path involves a few system calls itself, such as close(), that can themselves touch errno.

This is not as worrisome as it might sound. If close() fails, this just means substituting one meaningful error message for another, which is perfectly fine. However, when the call _succeeds_, it is allowed to (and sometimes might) clobber errno along the way with some undefined value, so it is good higiene to save errno and restore it immediately before returning to the caller. Do so.

---
 unix-socket.c |   39 ++++++++++++++++++++++-----------------
 1 files changed, 22 insertions(+), 17 deletions(-)

index 7d8bec61..01f119f9 100644
@@ -73,25 +73,29 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 
 int unix_stream_connect(const char *path)
 {
-	int fd;
+	int fd, saved_errno;
 	struct sockaddr_un sa;
 	struct unix_sockaddr_context ctx;
 
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;
 	fd = unix_stream_socket();
-	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
-		unix_sockaddr_cleanup(&ctx);
-		close(fd);
-		return -1;
-	}
+	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
+		goto fail;
 	unix_sockaddr_cleanup(&ctx);
 	return fd;
+
+fail:
+	saved_errno = errno;
+	unix_sockaddr_cleanup(&ctx);
+	close(fd);
+	errno = saved_errno;
+	return -1;
 }
 
 int unix_stream_listen(const char *path)
 {
-	int fd;
+	int fd, saved_errno;
 	struct sockaddr_un sa;
 	struct unix_sockaddr_context ctx;
 

@@ -100,18 +104,19 @@ int unix_stream_listen(const char *path)
 	fd = unix_stream_socket();
 
 	unlink(path);
-	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
-		unix_sockaddr_cleanup(&ctx);
-		close(fd);
-		return -1;
-	}
+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
+		goto fail;
 
-	if (listen(fd, 5) < 0) {
-		unix_sockaddr_cleanup(&ctx);
-		close(fd);
-		return -1;
-	}
+	if (listen(fd, 5) < 0)
+		goto fail;
 
 	unix_sockaddr_cleanup(&ctx);
 	return fd;
+
+fail:
+	saved_errno = errno;
+	unix_sockaddr_cleanup(&ctx);
+	close(fd);
+	errno = saved_errno;
+	return -1;
 }

1.7.8.3

My favorite form for nits to come in: a patch.

Looks good to me. Thanks.

-Peff

This just follows the rfc3986 rules for percent-encoding url data into a strbuf.

---
 strbuf.c |   37 +++++++++++++++++++++++++++++++++++++
 strbuf.h |    5 +++++
 2 files changed, 42 insertions(+), 0 deletions(-)

index a849705..ff0b96b 100644
@@ -411,3 +411,40 @@ void strbuf_add_lines(struct strbuf *out, const char *prefix,
 	}
 	strbuf_complete_line(out);
 }
+
+static int is_rfc3986_reserved(char ch)
+{
+	switch (ch) {
+		case '!': case '*': case '\'': case '(': case ')': case ';':
+		case ':': case '@': case '&': case '=': case '+': case '$':
+		case ',': case '/': case '?': case '#': case '[': case ']':
+			return 1;
+	}
+	return 0;
+}
+
+static int is_rfc3986_unreserved(char ch)
+{
+	return isalnum(ch) ||
+		ch == '-' || ch == '_' || ch == '.' || ch == '~';
+}
+
+void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
+			  int reserved)
+{
+	strbuf_grow(sb, len);
+	while (len--) {
+		char ch = *s++;
+		if (is_rfc3986_unreserved(ch) ||
+		    (!reserved && is_rfc3986_reserved(ch)))
+			strbuf_addch(sb, ch);
+		else
+			strbuf_addf(sb, "%%%02x", ch);
+	}
+}
+
+void strbuf_addstr_urlencode(struct strbuf *sb, const char *s,
+			     int reserved)
+{
+	strbuf_add_urlencode(sb, s, strlen(s), reserved);
+}

index 08fc13d..fbf059f 100644
@@ -123,4 +123,9 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 extern int strbuf_branchname(struct strbuf *sb, const char *name);
 extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
+extern void strbuf_add_urlencode(struct strbuf *, const char *, size_t,
+				 int reserved);
+extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
+				    int reserved);
+
 #endif /* STRBUF_H */

1.7.8.rc2.40.gaf387

It indicates whether one should encode rfc3986 reserved characters. You want to use it when encoding the host, username, and password portions of a URL (i.e., before the "/"), but not the path (since you don't want to encode all of the slashes). If you were breaking down the path more (e.g., into a "query" and "fragment" portion), you would care about more specific quoting there, but we don't; we treat everything after the slash as an opaque blob of path.

Patch to the strbuf api documentation is below. I think it should be squashed into patch 12/13.

It is aligned correctly. When you ident by tabs, the "+" of the diff gets soaked in the first tabstop, so it is off-by-one with respect to non-tabbed input (it is off even more in the quoted section above, because "> > +" gets soaked into the first tabstop). You can see your version above also is misaligned when I quote it. :)

If you apply the diff, the result is as you expected.

-Peff

---

index afe2759..e1ab6c5 100644
@@ -270,3 +270,14 @@ same behaviour as well.
 	third argument can be used to set the environment which the editor is
 	run in. If the buffer is NULL the editor is launched as usual but the
 	file's contents are not read into the buffer upon completion.
+
+`strbuf_add_urlencode`::
+
+	Copy data to the end of the buffer, percent-encoding it as per
+	rfc3986. If the reserved flag is non-zero, then characters in
+	the rfc3986 reserved list are percent-encoded; otherwise, they
+	are copied literally. Characters in the rfc3986 unreserved list
+	are always copied literally. All other characters are
+	percent-encoded. Typically, one would use the reserved flag for
+	the host and user-info sections of a URL, but leave special
+	characters in the path untouched.

This is like "cache", except that we actually put the credentials on disk. This can be terribly insecure, of course, but we do what we can to protect them by filesystem permissions, and we warn the user in the documentation.

This is not unlike using .netrc to store entries, but it's a little more user-friendly. Instead of putting credentials in place ahead of time, we transparently store them after prompting the user for them once.

---
This version has the "don't erase everything" protection we discussed.

 .gitignore                             |    1 +
 Documentation/git-credential-store.txt |   75 +++++++++++++++
 Documentation/gitcredentials.txt       |    5 +
 Makefile                               |    1 +
 credential-store.c                     |  157 ++++++++++++++++++++++++++++++++
 t/t0302-credential-store.sh            |    9 ++
 6 files changed, 248 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-credential-store.txt
 create mode 100644 credential-store.c
 create mode 100755 t/t0302-credential-store.sh

index a6b0bd4..2b7a3f9 100644
@@ -32,6 +32,7 @@
 /git-count-objects
 /git-credential-cache
 /git-credential-cache--daemon
+/git-credential-store
 /git-cvsexportcommit
 /git-cvsimport
 /git-cvsserver

index 4e3f860..066f825 100644
@@ -71,6 +71,11 @@ cache::
 	Cache credentials in memory for a short period of time. See
 	linkgit:git-credential-cache[1] for details.
 
+store::
+
+	Store credentials indefinitely on disk. See
+	linkgit:git-credential-store[1] for details.
+
 You may also have third-party helpers installed; search for
 `credential-*` in the output of `git help -a`, and consult the
 documentation of individual helpers.  Once you have selected a helper,

index 7ad2fe3..b144f1a 100644
@@ -429,6 +429,7 @@ PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += credential-cache.o
 PROGRAM_OBJS += credential-cache--daemon.o
+PROGRAM_OBJS += credential-store.o
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 

new file mode 100755
index 0000000..f61b40c
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+test_description='credential-store tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+helper_test store
+
+test_done

1.7.8.rc2.40.gaf387

We already have tests for the internal helpers, but it's nice to give authors of external tools an easy way to sanity-check their helpers.

If you have written the "git-credential-foo" helper, you can do so with:

  GIT_TEST_CREDENTIAL_HELPER=foo \
  make t0303-credential-external.sh

This assumes that your helper is capable of both storing and retrieving credentials (some helpers may be read-only, and they will fail these tests).

If your helper supports time-based expiration with a configurable timeout, you can test that feature like this:

  GIT_TEST_CREDENTIAL_HELPER_TIMEOUT="foo --timeout=1" \
  make t0303-credential-external.sh

---
This version adds GIT_TEST_CREDENTIAL_HELPER_SETUP, which is an unfortunate hack needed for the OS X helper (I'll post that patch in a few minutes).

 t/t0303-credential-external.sh |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)
 create mode 100755 t/t0303-credential-external.sh

new file mode 100755
index 0000000..267f4c8
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='external credential helper tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+pre_test() {
+	test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
+	eval "$GIT_TEST_CREDENTIAL_HELPER_SETUP"
+
+	# clean before the test in case there is cruft left
+	# over from a previous run that would impact results
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+}
+
+post_test() {
+	# clean afterwards so that we are good citizens
+	# and don't leave cruft in the helper's storage, which
+	# might be long-term system storage
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+}
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
+	say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
+else
+	pre_test
+	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+	post_test
+fi
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
+	say "# skipping external helper timeout tests"
+else
+	pre_test
+	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+	post_test
+fi
+
+test_done

1.7.8.rc2.40.gaf387

And here's the prompt/terminal refactoring to go on top of it. This isn't as well reviewed as the main credential series, but I think it's reasonably solid.

Since the last round, I fixed the bone-head NO_DEV_TTY/HAVE_DEV_TTY mixup that Jakub pointed out. And I split the "Linux has /dev/tty" patch out so I could test to make sure it isn't accidentally turned on until we actually enable it.

I also tested it on OS X, and it works fine there.

[1/9]: imap-send: avoid buffer overflow
[2/9]: imap-send: don't check return value of git_getpass [3/9]: move git_getpass to its own source file [4/9]: refactor git_getpass into generic prompt function [5/9]: add generic terminal prompt function [6/9]: prompt: use git_terminal_prompt
[7/9]: credential: use git_prompt instead of git_getpass [8/9]: Makefile: linux has /dev/tty
[9/9]: Makefile: OS X has /dev/tty

-Peff

We format the password prompt in an 80-character static buffer. It contains the remote host and username, so it's unlikely to overflow (or be exploitable by a remote attacker), but there's no reason not to be careful and use a strbuf.

---
 imap-send.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

index e1ad1a4..4c1e897 100644
@@ -1209,9 +1209,10 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 			goto bail;
 		}
 		if (!srvc->pass) {
-			char prompt[80];
-			sprintf(prompt, "Password (%s@%s): ", srvc->user, srvc->host);
-			arg = git_getpass(prompt);
+			struct strbuf prompt = STRBUF_INIT;
+			strbuf_addf(&prompt, "Password (%s@%s): ", srvc->user, srvc->host);
+			arg = git_getpass(prompt.buf);
+			strbuf_release(&prompt);
 			if (!arg) {
 				perror("getpass");
 				exit(1);

1.7.8.rc2.40.gaf387

git_getpass will always die() if we weren't able to get input, so there's no point looking for NULL.

---
 imap-send.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

index 4c1e897..227253e 100644
@@ -1213,10 +1213,6 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 			strbuf_addf(&prompt, "Password (%s@%s): ", srvc->user, srvc->host);
 			arg = git_getpass(prompt.buf);
 			strbuf_release(&prompt);
-			if (!arg) {
-				perror("getpass");
-				exit(1);
-			}
 			if (!*arg) {
 				fprintf(stderr, "Skipping account %s@%s, no password\n", srvc->user, srvc->host);
 				goto bail;

1.7.8.rc2.40.gaf387

This is currently in connect.c, but really has nothing to do with the git protocol itself. Let's make a new source file all about prompting the user, which will make it cleaner to refactor.

---
 Makefile     |    2 ++
 cache.h      |    1 -
 connect.c    |   44 --------------------------------------------
 credential.c |    1 +
 imap-send.c  |    1 +
 prompt.c     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 prompt.h     |    6 ++++++
 7 files changed, 58 insertions(+), 45 deletions(-)
 create mode 100644 prompt.c
 create mode 100644 prompt.h

index b144f1a..5201bea 100644
@@ -565,6 +565,7 @@ LIB_H += parse-options.h
 LIB_H += patch-ids.h
 LIB_H += pkt-line.h
 LIB_H += progress.h
+LIB_H += prompt.h
 LIB_H += quote.h
 LIB_H += reflog-walk.h
 LIB_H += refs.h

@@ -672,6 +673,7 @@ LIB_OBJS += pkt-line.o
 LIB_OBJS += preload-index.o
 LIB_OBJS += pretty.o
 LIB_OBJS += progress.o
+LIB_OBJS += prompt.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o

index 8c98d05..bb26c2f 100644
@@ -1025,7 +1025,6 @@ struct ref {
 extern struct ref *find_ref_by_name(const struct ref *list, const char *name);
 
 #define CONNECT_VERBOSE       (1u << 0)
-extern char *git_getpass(const char *prompt);
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
 extern int git_connection_is_socket(struct child_process *conn);

index a17eafe..fbb7231 100644
@@ -3,6 +3,7 @@
 #include "string-list.h"
 #include "run-command.h"
 #include "url.h"
+#include "prompt.h"
 
 void credential_init(struct credential *c)
 {

index 227253e..43588e8 100644
@@ -25,6 +25,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "run-command.h"
+#include "prompt.h"
 #ifdef NO_OPENSSL
 typedef void *SSL;
 #else

new file mode 100644
index 0000000..0fd7bd9
@@ -0,0 +1,6 @@
+#ifndef PROMPT_H
+#define PROMPT_H
+
+char *git_getpass(const char *prompt);
+
+#endif /* PROMPT_H */

1.7.8.rc2.40.gaf387

This will allow callers to specify more options (e.g., leaving echo on). The original git_getpass becomes a slim wrapper around the new function.

---
 prompt.c |   46 ++++++++++++++++++++++++++++++----------------
 prompt.h |    3 +++
 2 files changed, 33 insertions(+), 16 deletions(-)

index 42a1c9f..2002644 100644
@@ -3,26 +3,13 @@
 #include "strbuf.h"
 #include "prompt.h"
 
-char *git_getpass(const char *prompt)
+static char *do_askpass(const char *cmd, const char *prompt)
 {
-	const char *askpass;
 	struct child_process pass;
 	const char *args[3];
 	static struct strbuf buffer = STRBUF_INIT;
 
-	askpass = getenv("GIT_ASKPASS");
-	if (!askpass)
-		askpass = askpass_program;
-	if (!askpass)
-		askpass = getenv("SSH_ASKPASS");
-	if (!askpass || !(*askpass)) {
-		char *result = getpass(prompt);
-		if (!result)
-			die_errno("Could not read password");
-		return result;
-	}
-
-	args[0] = askpass;
+	args[0] = cmd;
 	args[1]	= prompt;
 	args[2] = NULL;
 

@@ -35,7 +22,7 @@
 
 	strbuf_reset(&buffer);
 	if (strbuf_read(&buffer, pass.out, 20) < 0)
-		die("failed to read password from %s\n", askpass);
+		die("failed to get '%s' from %s\n", prompt, cmd);
 
 	close(pass.out);
 

@@ -46,3 +33,30 @@
 
 	return buffer.buf;
 }
+
+char *git_prompt(const char *prompt, int flags)
+{
+	char *r;
+
+	if (flags & PROMPT_ASKPASS) {
+		const char *askpass;
+
+		askpass = getenv("GIT_ASKPASS");
+		if (!askpass)
+			askpass = askpass_program;
+		if (!askpass)
+			askpass = getenv("SSH_ASKPASS");
+		if (askpass && *askpass)
+			return do_askpass(askpass, prompt);
+	}
+
+	r = getpass(prompt);
+	if (!r)
+		die_errno("could not read '%s'", prompt);
+	return r;
+}
+
+char *git_getpass(const char *prompt)
+{
+	return git_prompt(prompt, PROMPT_ASKPASS);
+}

index 0fd7bd9..9ab85a7 100644
@@ -1,6 +1,9 @@
 #ifndef PROMPT_H
 #define PROMPT_H
 
+#define PROMPT_ASKPASS (1<<0)
+
+char *git_prompt(const char *prompt, int flags);
 char *git_getpass(const char *prompt);
 
 #endif /* PROMPT_H */

1.7.8.rc2.40.gaf387

When we need to prompt the user for input interactively, we want to access their terminal directly. We can't rely on stdio because it may be connected to pipes or files, rather than the terminal. Instead, we use "getpass()", because it abstracts the idea of prompting and reading from the terminal. However, it has some problems:

1. It never echoes the typed characters, which makes it OK for passwords but annoying for other input (like usernames).

2. Some implementations of getpass() have an extremely small input buffer (e.g., Solaris 8 is reported to support only 8 characters).

3. Some implementations of getpass() will fall back to reading from stdin (e.g., glibc). We explicitly don't want this, because our stdin may be connected to a pipe speaking a particular protocol, and reading will disrupt the protocol flow (e.g., the remote-curl helper).

4. Some implementations of getpass() turn off signals, so that hitting "^C" on the terminal does not break out of the password prompt. This can be a mild annoyance.

Instead, let's provide an abstract "git_terminal_prompt" function that addresses these concerns. This patch includes an implementation based on /dev/tty, enabled by setting HAVE_DEV_TTY. The fallback is to use getpass() as before.

---
 Makefile          |    9 ++++++
 compat/terminal.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/terminal.h |    6 ++++
 3 files changed, 96 insertions(+), 0 deletions(-)
 create mode 100644 compat/terminal.c
 create mode 100644 compat/terminal.h

index 5201bea..4ef6ba5 100644
@@ -227,6 +227,9 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
+# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
+# user.
+#
 # Define GETTEXT_POISON if you are debugging the choice of strings marked
 # for translation.  In a GETTEXT_POISON build, you can turn all strings marked
 # for translation into gibberish by setting the GIT_GETTEXT_POISON variable

@@ -523,6 +526,7 @@ LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
 LIB_H += compat/obstack.h
+LIB_H += compat/terminal.h
 LIB_H += compat/win32/pthread.h
 LIB_H += compat/win32/syslog.h
 LIB_H += compat/win32/poll.h

@@ -612,6 +616,7 @@ LIB_OBJS += color.o
 LIB_OBJS += combine-diff.o
 LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
+LIB_OBJS += compat/terminal.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o

@@ -1642,6 +1647,10 @@ ifdef HAVE_PATHS_H
 	BASIC_CFLAGS += -DHAVE_PATHS_H
 endif
 
+ifdef HAVE_DEV_TTY
+	BASIC_CFLAGS += -DHAVE_DEV_TTY
+endif
+
 ifdef DIR_HAS_BSD_GROUP_SEMANTICS
 	COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
 endif

new file mode 100644
index 0000000..97db7cd
@@ -0,0 +1,6 @@
+#ifndef COMPAT_TERMINAL_H
+#define COMPAT_TERMINAL_H
+
+char *git_terminal_prompt(const char *prompt, int echo);
+
+#endif /* COMPAT_TERMINAL_H */

1.7.8.rc2.40.gaf387

[email protected] wrote on Sat, 10 Dec 2011 05:41 -0500:

		-- Pete

Right. But note that it is protected by term_fd being set.

Yes, but just below:

We set up term_fd, and then:

On error, disable it again.

It could, but it's a no-op as-is, as term_fd will be -1.

I agree it might be a little more obvious to put it there (I think what happened is my initial revision did not look at "echo" ever again, and then that conditional was added later when I realized that the "!echo" case needed us to print the newline manually).

Because I used sigchain_push_common, which has no pop_common analog. But it's OK, because calling restore_term sets term_fd to -1, making further calls a no-op. So leaving the handler in place is fine.

Another option would be to add sigchain_pop_common, which pops the same signals from push_common.

-Peff

[email protected] wrote on Thu, 15 Dec 2011 08:39 -0500:

Thanks for the detailed explanation. It was indeed the lack of symmetry that set me off. I missed the term_fd check for both.

		-- Pete

Our custom implementation of git_terminal_prompt has many advantages over regular getpass(), as described in the prior commit.

This also lets us implement a PROMPT_ECHO flag for callers who want it.

---
 prompt.c |    3 ++-
 prompt.h |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

index 2002644..72ab9de 100644
@@ -2,6 +2,7 @@
 #include "run-command.h"
 #include "strbuf.h"
 #include "prompt.h"
+#include "compat/terminal.h"
 
 static char *do_askpass(const char *cmd, const char *prompt)
 {

@@ -50,7 +51,7 @@
 			return do_askpass(askpass, prompt);
 	}
 
-	r = getpass(prompt);
+	r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
 	if (!r)
 		die_errno("could not read '%s'", prompt);
 	return r;

index 9ab85a7..04f321a 100644
@@ -2,6 +2,7 @@
 #define PROMPT_H
 
 #define PROMPT_ASKPASS (1<<0)
+#define PROMPT_ECHO    (1<<1)
 
 char *git_prompt(const char *prompt, int flags);
 char *git_getpass(const char *prompt);

1.7.8.rc2.40.gaf387

We use git_getpass to retrieve the username and password from the terminal. However, git_getpass will not echo the username as the user types. We can fix this by using the more generic git_prompt, which underlies git_getpass but lets us specify an "echo" option.

---
 credential.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

index fbb7231..62d1c56 100644
@@ -109,7 +109,8 @@ static void credential_describe(struct credential *c, struct strbuf *out)
 		strbuf_addf(out, "/%s", c->path);
 }
 
-static char *credential_ask_one(const char *what, struct credential *c)
+static char *credential_ask_one(const char *what, struct credential *c,
+				int flags)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct strbuf prompt = STRBUF_INIT;

@@ -121,11 +122,7 @@ static void credential_describe(struct credential *c, struct strbuf *out)
 	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);
+	r = git_prompt(prompt.buf, flags);
 
 	strbuf_release(&desc);
 	strbuf_release(&prompt);

@@ -135,9 +132,11 @@ static void credential_describe(struct credential *c, struct strbuf *out)
 static void credential_getpass(struct credential *c)
 {
 	if (!c->username)
-		c->username = credential_ask_one("Username", c);
+		c->username = credential_ask_one("Username", c,
+						 PROMPT_ASKPASS|PROMPT_ECHO);
 	if (!c->password)
-		c->password = credential_ask_one("Password", c);
+		c->password = credential_ask_one("Password", c,
+						 PROMPT_ASKPASS);
 }
 
 int credential_read(struct credential *c, FILE *fp)

1.7.8.rc2.40.gaf387

Therefore we can turn on our custom prompt function instead of relying on getpass.

---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 4ef6ba5..198fc9b 100644
@@ -841,6 +841,7 @@ ifeq ($(uname_S),Linux)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
 	HAVE_PATHS_H = YesPlease
+	HAVE_DEV_TTY = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	NO_STRLCPY = YesPlease

1.7.8.rc2.40.gaf387

We can use our enhanced getpass(). Tested by me.

---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 198fc9b..1b3f5f0 100644
@@ -902,6 +902,7 @@ ifeq ($(uname_S),Darwin)
 	endif
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
+	HAVE_DEV_TTY = YesPlease
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease

1.7.8.rc2.40.gaf387

---
These two patches go on top of jk/credentials.

 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

index 2c506b3..5b7f6a8 100644
@@ -1139,6 +1139,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_DEV_TTY = YesPlease
 	NO_POSIX_GOODIES = UnfortunatelyYes
 	NATIVE_CRLF = YesPlease
 

@@ -1232,6 +1233,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	ETAGS_TARGET = ETAGS
 	NO_INET_PTON = YesPlease
 	NO_INET_NTOP = YesPlease
+	NO_DEV_TTY = YesPlease
 	NO_POSIX_GOODIES = UnfortunatelyYes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"

1.7.8.216.g2e426

Am 12.12.2011 22:18, schrieb Jeff King:

I see. Obviously, I've tested an earlier iteration. This patch can be dropped then.

-- Hannes

Introduce a configuration option NO_UNIX_SOCKETS to exclude code that depends on Unix sockets and use it in MSVC and MinGW builds.

Notice that unix-socket.h was missing from LIB_H before; fix that, too.

---
All or most of the tests introduced by the series fail on Windows. What is the preferred way to exclude them? Mark the all with a prerequisite? Or exit early at the beginning of the test file?

 Makefile |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

index 5b7f6a8..77c85db 100644
@@ -143,6 +143,8 @@ all::
 #
 # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().
 #
+# Define NO_UNIX_SOCKETS if your system does not offer unix sockets.
+#
 # Define NO_SOCKADDR_STORAGE if your platform does not have struct
 # sockaddr_storage.
 #

@@ -430,8 +432,6 @@ PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
-PROGRAM_OBJS += credential-cache.o
-PROGRAM_OBJS += credential-cache--daemon.o
 PROGRAM_OBJS += credential-store.o
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))

@@ -709,7 +709,6 @@ LIB_OBJS += transport-helper.o
 LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
-LIB_OBJS += unix-socket.o
 LIB_OBJS += unpack-trees.o
 LIB_OBJS += url.o
 LIB_OBJS += usage.o

@@ -1112,6 +1111,7 @@ ifeq ($(uname_S),Windows)
 	NO_SYS_POLL_H = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
+	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_STRCASESTR = YesPlease

@@ -1206,6 +1206,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_LIBGEN_H = YesPlease
 	NO_SYS_POLL_H = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
+	NO_UNIX_SOCKETS = YesPlease
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_STRCASESTR = YesPlease

@@ -1584,6 +1585,12 @@ ifdef NO_INET_PTON
 	LIB_OBJS += compat/inet_pton.o
 	BASIC_CFLAGS += -DNO_INET_PTON
 endif
+ifndef NO_UNIX_SOCKETS
+	LIB_OBJS += unix-socket.o
+	LIB_H += unix-socket.h
+	PROGRAM_OBJS += credential-cache.o
+	PROGRAM_OBJS += credential-cache--daemon.o
+endif
 
 ifdef NO_ICONV
 	BASIC_CFLAGS += -DNO_ICONV

1.7.8.216.g2e426

Makes sense, thanks.

Oops, thanks.

The cache daemon is the only thing that uses sockets. I would suggest just exiting early from its script (patch below).

The rest of the tests should pass under Windows. If they don't, then they should be fixed (I'm happy to work on that, with the limitation that I don't actually have a Windows box, and if at all possible I'd like to keep it that way).

In theory we should also disable the documentation for credential-cache. But that means surgery not only to Documentation/Makefile, but figuring out how to pass the flags down to the actual asciidoc process (since gitcredentials(7) mentions it in the text). Certainly possible, but I don't know if it's worth the effort or not.

---

index e3ee884..6e7e190 100644
@@ -2220,6 +2220,7 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
 	@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@
 endif
 	@echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@
+	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK

index 3a65f99..82c8411 100755
@@ -4,6 +4,11 @@ test_description='credential-cache tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
+test -z "$NO_UNIX_SOCKETS" || {
+	skip_all='skipping credential-cache tests, unix sockets not available'
+	test_done
+}
+
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 

I do not think it matters that much. We've been shipping documentation for stuff like remote archiver and daemon without conditional compilation, no?

I'll queue a single patch that is a squash between 2/2 and Peff's test updates between "credentials: add "cache" helper" and "strbuf: add strbuf_add*_urlencode" in the series.

Thanks.

True. Let's not worry about it, then.

That's perfect. Thanks.

-Peff

Am 13.12.2011 01:45, schrieb Junio C Hamano:

Thanks. The resulting series builds fine on Windows and passes/skips the new tests in the expected manner.

-- Hannes

Thanks. Let's advance the topic forward then.

And here is that helper.

-- >8 --
Subject: [PATCH] contrib: add credential helper for OS X Keychain

With this installed in your $PATH, you can store git-over-http passwords in your keychain by doing:

git config credential.helper osxkeychain

The code is based in large part on the work of Jay Soffian, who wrote the helper originally for the initial, unpublished version of the credential helper protocol.

This version will pass t0303 if you do:

  GIT_TEST_CREDENTIAL_HELPER=osxkeychain \
  GIT_TEST_CREDENTIAL_HELPER_SETUP="export HOME=$HOME" \
  ./t0303-credential-external.sh

The "HOME" setup is unfortunately necessary. The test scripts set HOME to the trash directory, but this causes the keychain API to complain.

---
I tried clever workarounds for the HOME thing, like running "security create-keychain" in the trash directory. But if I try to create the "login" keychain, it says it's already there! So it's like we're in a weird limbo where we both have and do not have access to the keychains.

I'm not too concerned with the hackishness of my solution, though; this isn't even a part of the regular test scripts, but just a thing you can do manually to test the helper from contrib.

Many thanks to Jay for the initial version; even though this version is quite chopped-up, having somebody else provide a nice example of how to use the SecKeychain functions made it much easier. I tried to trim the code down to the bare essentials to glue our input into the SecKeychain API, and to show good practices for other helpers to follow.

I dropped the python version, as it is redundant and I didn't feel like porting it to the new interface. It could be added on top if somebody feels like converting it as an exercise. :)

 contrib/credential/osxkeychain/.gitignore          |    1 +
 contrib/credential/osxkeychain/Makefile            |   14 ++
 .../osxkeychain/git-credential-osxkeychain.c       |  173 ++++++++++++++++++++
 3 files changed, 188 insertions(+), 0 deletions(-)
 create mode 100644 contrib/credential/osxkeychain/.gitignore
 create mode 100644 contrib/credential/osxkeychain/Makefile
 create mode 100644 contrib/credential/osxkeychain/git-credential-osxkeychain.c

new file mode 100644
index 0000000..6c5b702
@@ -0,0 +1 @@
+git-credential-osxkeychain

new file mode 100644
index 0000000..75c07f8
@@ -0,0 +1,14 @@
+all:: git-credential-osxkeychain
+
+CC = gcc
+RM = rm -f
+CFLAGS = -g -Wall
+
+git-credential-osxkeychain: git-credential-osxkeychain.o
+	$(CC) -o $@ $< -Wl,-framework -Wl,Security
+
+git-credential-osxkeychain.o: git-credential-osxkeychain.c
+	$(CC) -c $(CFLAGS) $<
+
+clean:
+	$(RM) git-credential-osxkeychain git-credential-osxkeychain.o

1.7.8.rc2.40.gaf387