[PATCHv2 0/13] credential helpers

[PATCHv2 0/13] credential helpers

[Jeff King]

This is a re-roll of the credential helper series. Changes are noted
below.

  [01/13]: test-lib: add test_config_global variant
  [02/13]: t5550: fix typo
  [03/13]: introduce credentials API

    - simplified "first word is alnum" rules. You can now use "!f() {"
      to get a shell snippet helper.
    - documentation clarifications based on discussion with Junio
    - fix missing EOF in test script (same as the fixup in pu)
    - minor code style fixups

  [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

    - don't die on unknown action, as suggested by credential helper
      docs. This is to leave room for new actions later.

  [11/13]: strbuf: add strbuf_add*_urlencode
  [12/13]: credentials: add "store" helper

    - don't die on unknown action, as above
    - "store --store=foo" is now spelled "store --file=foo"
    - passwords are now stored in most-recently-added order
    - drop remove/lookup restrictions, per discussion with Junio

    This last one has an interesting side effect. You can now
    do: "git credential-store erase </dev/null" to erase everything
    (since you have provided no restrictions, it matches everything).
    It's not something git would send intentionally, but it seems
    accidentally destructive (e.g., for some reason the main git process
    dies and closes the pipe). Perhaps a "credential on the wire" should
    be re-defined as a set of key/value lines followed by a blank line
    to signal "end"?

  [13/13]: t: add test harness for external credential helpers

    - actually invoke cleanup functions. The lib-credential script
      provides a best-effort cleanup function, since external helpers
      may be writing to system storage outsid of the trash directory.
      But in v1 we didn't actually call it.

-Peff

[PATCHv2 01/13] test-lib: add test_config_global variant

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..160479b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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.rc4.4.g884ec

[PATCHv2 02/13] t5550: fix typo

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5550-http-fetch.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 311a33c..3d6e871 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -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.rc4.4.g884ec

[PATCHv2 03/13] introduce credentials API

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 .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

diff --git a/.gitignore b/.gitignore
index 8572c8c..7d2fefc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -167,6 +167,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /test-chmtime
+/test-credential
 /test-ctype
 /test-date
 /test-delta
diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
new file mode 100644
index 0000000..f624aef
--- /dev/null
+++ b/Documentation/technical/api-credentials.txt
@@ -0,0 +1,241 @@
+credentials API
+===============
+
+The credentials API provides an abstracted way of gathering username and
+password credentials from the user (even though credentials in the wider
+world can take many forms, in this document the word "credential" always
+refers to a username and password pair).
+
+Data Structures
+---------------
+
+`struct credential`::
+
+	This struct represents a single username/password combination
+	along with any associated context. All string fields should be
+	heap-allocated (or NULL if they are not known or not applicable).
+	The meaning of the individual context fields is the same as
+	their counterparts in the helper protocol; see the section below
+	for a description of each field.
++
+The `helpers` member of the struct is a `string_list` of helpers.  Each
+string specifies an external helper which will be run, in order, to
+either acquire or store credentials. See the section on credential
+helpers below.
++
+This struct should always be initialized with `CREDENTIAL_INIT` or
+`credential_init`.
+
+
+Functions
+---------
+
+`credential_init`::
+
+	Initialize a credential structure, setting all fields to empty.
+
+`credential_clear`::
+
+	Free any resources associated with the credential structure,
+	returning it to a pristine initialized state.
+
+`credential_fill`::
+
+	Instruct the credential subsystem to fill the username and
+	password fields of the passed credential struct by first
+	consulting helpers, then asking the user. After this function
+	returns, the username and password fields of the credential are
+	guaranteed to be non-NULL. If an error occurs, the function will
+	die().
+
+`credential_reject`::
+
+	Inform the credential subsystem that the provided credentials
+	have been rejected. This will cause the credential subsystem to
+	notify any helpers of the rejection (which allows them, for
+	example, to purge the invalid credentials from storage).  It
+	will also free() the username and password fields of the
+	credential and set them to NULL (readying the credential for
+	another call to `credential_fill`). Any errors from helpers are
+	ignored.
+
+`credential_approve`::
+
+	Inform the credential subsystem that the provided credentials
+	were successfully used for authentication.  This will cause the
+	credential subsystem to notify any helpers of the approval, so
+	that they may store the result to be used again.  Any errors
+	from helpers are ignored.
+
+Example
+-------
+
+The example below shows how the functions of the credential API could be
+used to login to a fictitious "foo" service on a remote host:
+
+-----------------------------------------------------------------------
+int foo_login(struct foo_connection *f)
+{
+	int status;
+	/*
+	 * Create a credential with some context; we don't yet know the
+	 * username or password.
+	 */
+
+	struct credential c = CREDENTIAL_INIT;
+	c.protocol = xstrdup("foo");
+	c.host = xstrdup(f->hostname);
+
+	/*
+	 * Fill in the username and password fields by contacting
+	 * helpers and/or asking the user. The function will die if it
+	 * fails.
+	 */
+	credential_fill(&c);
+
+	/*
+	 * Otherwise, we have a username and password. Try to use it.
+	 */
+	status = send_foo_login(f, c.username, c.password);
+	switch (status) {
+	case FOO_OK:
+		/* It worked. Store the credential for later use. */
+		credential_accept(&c);
+		break;
+	case FOO_BAD_LOGIN:
+		/* Erase the credential from storage so we don't try it
+		 * again. */
+		credential_reject(&c);
+		break;
+	default:
+		/*
+		 * Some other error occured. We don't know if the
+		 * credential is good or bad, so report nothing to the
+		 * credential subsystem.
+		 */
+	}
+
+	/* Free any associated resources. */
+	credential_clear(&c);
+
+	return status;
+}
+-----------------------------------------------------------------------
+
+
+Credential Helpers
+------------------
+
+Credential helpers are programs executed by git to fetch or save
+credentials from and to long-term storage (where "long-term" is simply
+longer than a single git process; e.g., credentials may be stored
+in-memory for a few minutes, or indefinitely on disk).
+
+Each helper is specified by a single string. The string is transformed
+by git into a command to be executed using these rules:
+
+  1. If the helper string begins with "!", it is considered a shell
+     snippet, and everything after the "!" becomes the command.
+
+  2. Otherwise, if the helper string begins with an absolute path, the
+     verbatim helper string becomes the command.
+
+  3. Otherwise, the string "git credential-" is prepended to the helper
+     string, and the result becomes the command.
+
+The resulting command then has an "operation" argument appended to it
+(see below for details), and the result is executed by the shell.
+
+Here are some example specifications:
+
+----------------------------------------------------
+# run "git credential-foo"
+foo
+
+# same as above, but pass an argument to the helper
+foo --bar=baz
+
+# the arguments are parsed by the shell, so use shell
+# quoting if necessary
+foo --bar="whitespace arg"
+
+# you can also use an absolute path, which will not use the git wrapper
+/path/to/my/helper --with-arguments
+
+# or you can specify your own shell snippet
+!f() { echo "password=`cat $HOME/.secret`"; }; f
+----------------------------------------------------
+
+Generally speaking, rule (3) above is the simplest for users to specify.
+Authors of credential helpers should make an effort to assist their
+users by naming their program "git-credential-$NAME", and putting it in
+the $PATH or $GIT_EXEC_PATH during installation, which will allow a user
+to enable it with `git config credential.helper $NAME`.
+
+When a helper is executed, it will have one "operation" argument
+appended to its command line, which is one of:
+
+`get`::
+
+	Return a matching credential, if any exists.
+
+`store`::
+
+	Store the credential, if applicable to the helper.
+
+`erase`::
+
+	Remove a matching credential, if any, from the helper's storage.
+
+The details of the credential will be provided on the helper's stdin
+stream. The credential is split into a set of named attributes.
+Attributes are provided to the helper, one per line. Each attribute is
+specified by a key-value pair, separated by an `=` (equals) sign,
+followed by a newline. The key may contain any bytes except `=`,
+newline, or NUL. The value may contain any bytes except newline or NUL.
+In both cases, all bytes are treated as-is (i.e., there is no quoting,
+and one cannot transmit a value with newline or NUL in it). The list of
+attributes is terminated by a blank line or end-of-file.
+
+Git will send the following attributes (but may not send all of
+them for a given credential; for example, a `host` attribute makes no
+sense when dealing with a non-network protocol):
+
+`protocol`::
+
+	The protocol over which the credential will be used (e.g.,
+	`https`).
+
+`host`::
+
+	The remote hostname for a network credential.
+
+`path`::
+
+	The path with which the credential will be used. E.g., for
+	accessing a remote https repository, this will be the
+	repository's path on the server.
+
+`username`::
+
+	The credential's username, if we already have one (e.g., from a
+	URL, from the user, or from a previously run helper).
+
+`password`::
+
+	The credential's password, if we are asking it to be stored.
+
+For a `get` operation, the helper should produce a list of attributes
+on stdout in the same format. A helper is free to produce a subset, or
+even no values at all if it has nothing useful to provide. Any provided
+attributes will overwrite those already known about by git.
+
+For a `store` or `erase` operation, the helper's output is ignored.
+If it fails to perform the requested operation, it may complain to
+stderr to inform the user. If it does not support the requested
+operation (e.g., a read-only store), it should silently ignore the
+request.
+
+If a helper receives any other operation, it should silently ignore the
+request. This leaves room for future operations to be added (older
+helpers will just ignore the new requests).
diff --git a/Makefile b/Makefile
index b1c80a6..5ca363b 100644
--- a/Makefile
+++ b/Makefile
@@ -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
@@ -609,6 +611,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
diff --git a/credential.c b/credential.c
new file mode 100644
index 0000000..86397f3
--- /dev/null
+++ b/credential.c
@@ -0,0 +1,234 @@
+#include "cache.h"
+#include "credential.h"
+#include "string-list.h"
+#include "run-command.h"
+
+void credential_init(struct credential *c)
+{
+	memset(c, 0, sizeof(*c));
+	c->helpers.strdup_strings = 1;
+}
+
+void credential_clear(struct credential *c)
+{
+	free(c->protocol);
+	free(c->host);
+	free(c->path);
+	free(c->username);
+	free(c->password);
+	string_list_clear(&c->helpers, 0);
+
+	credential_init(c);
+}
+
+static void credential_describe(struct credential *c, struct strbuf *out)
+{
+	if (!c->protocol)
+		return;
+	strbuf_addf(out, "%s://", c->protocol);
+	if (c->username && *c->username)
+		strbuf_addf(out, "%s@", c->username);
+	if (c->host)
+		strbuf_addstr(out, c->host);
+	if (c->path)
+		strbuf_addf(out, "/%s", c->path);
+}
+
+static char *credential_ask_one(const char *what, struct credential *c)
+{
+	struct strbuf desc = STRBUF_INIT;
+	struct strbuf prompt = STRBUF_INIT;
+	char *r;
+
+	credential_describe(c, &desc);
+	if (desc.len)
+		strbuf_addf(&prompt, "%s for '%s': ", what, desc.buf);
+	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(&desc);
+	strbuf_release(&prompt);
+	return xstrdup(r);
+}
+
+static void credential_getpass(struct credential *c)
+{
+	if (!c->username)
+		c->username = credential_ask_one("Username", c);
+	if (!c->password)
+		c->password = credential_ask_one("Password", c);
+}
+
+int credential_read(struct credential *c, FILE *fp)
+{
+	struct strbuf line = STRBUF_INIT;
+
+	while (strbuf_getline(&line, fp, '\n') != EOF) {
+		char *key = line.buf;
+		char *value = strchr(key, '=');
+
+		if (!line.len)
+			break;
+
+		if (!value) {
+			warning("invalid credential line: %s", key);
+			strbuf_release(&line);
+			return -1;
+		}
+		*value++ = '\0';
+
+		if (!strcmp(key, "username")) {
+			free(c->username);
+			c->username = xstrdup(value);
+		} else if (!strcmp(key, "password")) {
+			free(c->password);
+			c->password = xstrdup(value);
+		} else if (!strcmp(key, "protocol")) {
+			free(c->protocol);
+			c->protocol = xstrdup(value);
+		} else if (!strcmp(key, "host")) {
+			free(c->host);
+			c->host = xstrdup(value);
+		} else if (!strcmp(key, "path")) {
+			free(c->path);
+			c->path = xstrdup(value);
+		}
+		/*
+		 * Ignore other lines; we don't know what they mean, but
+		 * this future-proofs us when later versions of git do
+		 * learn new lines, and the helpers are updated to match.
+		 */
+	}
+
+	strbuf_release(&line);
+	return 0;
+}
+
+static void credential_write_item(FILE *fp, const char *key, const char *value)
+{
+	if (!value)
+		return;
+	fprintf(fp, "%s=%s\n", key, value);
+}
+
+static void credential_write(const struct credential *c, FILE *fp)
+{
+	credential_write_item(fp, "protocol", c->protocol);
+	credential_write_item(fp, "host", c->host);
+	credential_write_item(fp, "path", c->path);
+	credential_write_item(fp, "username", c->username);
+	credential_write_item(fp, "password", c->password);
+}
+
+static int run_credential_helper(struct credential *c,
+				 const char *cmd,
+				 int want_output)
+{
+	struct child_process helper;
+	const char *argv[] = { NULL, NULL };
+	FILE *fp;
+
+	memset(&helper, 0, sizeof(helper));
+	argv[0] = cmd;
+	helper.argv = argv;
+	helper.use_shell = 1;
+	helper.in = -1;
+	if (want_output)
+		helper.out = -1;
+	else
+		helper.no_stdout = 1;
+
+	if (start_command(&helper) < 0)
+		return -1;
+
+	fp = xfdopen(helper.in, "w");
+	credential_write(c, fp);
+	fclose(fp);
+
+	if (want_output) {
+		int r;
+		fp = xfdopen(helper.out, "r");
+		r = credential_read(c, fp);
+		fclose(fp);
+		if (r < 0) {
+			finish_command(&helper);
+			return -1;
+		}
+	}
+
+	if (finish_command(&helper))
+		return -1;
+	return 0;
+}
+
+static int credential_do(struct credential *c, const char *helper,
+			 const char *operation)
+{
+	struct strbuf cmd = STRBUF_INIT;
+	int r;
+
+	if (helper[0] == '!')
+		strbuf_addstr(&cmd, helper + 1);
+	else if (is_absolute_path(helper))
+		strbuf_addstr(&cmd, helper);
+	else
+		strbuf_addf(&cmd, "git credential-%s", helper);
+
+	strbuf_addf(&cmd, " %s", operation);
+	r = run_credential_helper(c, cmd.buf, !strcmp(operation, "get"));
+
+	strbuf_release(&cmd);
+	return r;
+}
+
+void credential_fill(struct credential *c)
+{
+	int i;
+
+	if (c->username && c->password)
+		return;
+
+	for (i = 0; i < c->helpers.nr; i++) {
+		credential_do(c, c->helpers.items[i].string, "get");
+		if (c->username && c->password)
+			return;
+	}
+
+	credential_getpass(c);
+	if (!c->username && !c->password)
+		die("unable to get password from user");
+}
+
+void credential_approve(struct credential *c)
+{
+	int i;
+
+	if (c->approved)
+		return;
+	if (!c->username || !c->password)
+		return;
+
+	for (i = 0; i < c->helpers.nr; i++)
+		credential_do(c, c->helpers.items[i].string, "store");
+	c->approved = 1;
+}
+
+void credential_reject(struct credential *c)
+{
+	int i;
+
+	for (i = 0; i < c->helpers.nr; i++)
+		credential_do(c, c->helpers.items[i].string, "erase");
+
+	free(c->username);
+	c->username = NULL;
+	free(c->password);
+	c->password = NULL;
+	c->approved = 0;
+}
diff --git a/credential.h b/credential.h
new file mode 100644
index 0000000..2ea7d49
--- /dev/null
+++ b/credential.h
@@ -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 */
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
new file mode 100755
index 0000000..54ae1f4
--- /dev/null
+++ b/t/lib-credential.sh
@@ -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
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
new file mode 100755
index 0000000..81a455f
--- /dev/null
+++ b/t/t0300-credentials.sh
@@ -0,0 +1,195 @@
+#!/bin/sh
+
+test_description='basic credential helper tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+test_expect_success 'setup helper scripts' '
+	cat >dump <<-\EOF &&
+	whoami=`echo $0 | sed s/.*git-credential-//`
+	echo >&2 "$whoami: $*"
+	while IFS== read key value; do
+		echo >&2 "$whoami: $key=$value"
+		eval "$key=$value"
+	done
+	EOF
+
+	cat >git-credential-useless <<-\EOF &&
+	#!/bin/sh
+	. ./dump
+	exit 0
+	EOF
+	chmod +x git-credential-useless &&
+
+	cat >git-credential-verbatim <<-\EOF &&
+	#!/bin/sh
+	user=$1; shift
+	pass=$1; shift
+	. ./dump
+	test -z "$user" || echo username=$user
+	test -z "$pass" || echo password=$pass
+	EOF
+	chmod +x git-credential-verbatim &&
+
+	PATH="$PWD:$PATH"
+'
+
+test_expect_success 'credential_fill invokes helper' '
+	check fill "verbatim foo bar" <<-\EOF
+	--
+	username=foo
+	password=bar
+	--
+	verbatim: get
+	EOF
+'
+
+test_expect_success 'credential_fill invokes multiple helpers' '
+	check fill useless "verbatim foo bar" <<-\EOF
+	--
+	username=foo
+	password=bar
+	--
+	useless: get
+	verbatim: get
+	EOF
+'
+
+test_expect_success 'credential_fill stops when we get a full response' '
+	check fill "verbatim one two" "verbatim three four" <<-\EOF
+	--
+	username=one
+	password=two
+	--
+	verbatim: get
+	EOF
+'
+
+test_expect_success 'credential_fill continues through partial response' '
+	check fill "verbatim one \"\"" "verbatim two three" <<-\EOF
+	--
+	username=two
+	password=three
+	--
+	verbatim: get
+	verbatim: get
+	verbatim: username=one
+	EOF
+'
+
+test_expect_success 'credential_fill passes along metadata' '
+	check fill "verbatim one two" <<-\EOF
+	protocol=ftp
+	host=example.com
+	path=foo.git
+	--
+	username=one
+	password=two
+	--
+	verbatim: get
+	verbatim: protocol=ftp
+	verbatim: host=example.com
+	verbatim: path=foo.git
+	EOF
+'
+
+test_expect_success 'credential_approve calls all helpers' '
+	check approve useless "verbatim one two" <<-\EOF
+	username=foo
+	password=bar
+	--
+	--
+	useless: store
+	useless: username=foo
+	useless: password=bar
+	verbatim: store
+	verbatim: username=foo
+	verbatim: password=bar
+	EOF
+'
+
+test_expect_success 'do not bother storing password-less credential' '
+	check approve useless <<-\EOF
+	username=foo
+	--
+	--
+	EOF
+'
+
+
+test_expect_success 'credential_reject calls all helpers' '
+	check reject useless "verbatim one two" <<-\EOF
+	username=foo
+	password=bar
+	--
+	--
+	useless: erase
+	useless: username=foo
+	useless: password=bar
+	verbatim: erase
+	verbatim: username=foo
+	verbatim: password=bar
+	EOF
+'
+
+test_expect_success 'usernames can be preserved' '
+	check fill "verbatim \"\" three" <<-\EOF
+	username=one
+	--
+	username=one
+	password=three
+	--
+	verbatim: get
+	verbatim: username=one
+	EOF
+'
+
+test_expect_success 'usernames can be overridden' '
+	check fill "verbatim two three" <<-\EOF
+	username=one
+	--
+	username=two
+	password=three
+	--
+	verbatim: get
+	verbatim: username=one
+	EOF
+'
+
+test_expect_success 'do not bother completing already-full credential' '
+	check fill "verbatim three four" <<-\EOF
+	username=one
+	password=two
+	--
+	username=one
+	password=two
+	--
+	EOF
+'
+
+# We can't test the basic terminal password prompt here because
+# getpass() tries too hard to find the real terminal. But if our
+# askpass helper is run, we know the internal getpass is working.
+test_expect_success 'empty helper list falls back to internal getpass' '
+	check fill <<-\EOF
+	--
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username:
+	askpass: Password:
+	EOF
+'
+
+test_expect_success 'internal getpass does not ask for known username' '
+	check fill <<-\EOF
+	username=foo
+	--
+	username=foo
+	password=askpass-password
+	--
+	askpass: Password:
+	EOF
+'
+
+test_done
diff --git a/test-credential.c b/test-credential.c
new file mode 100644
index 0000000..dee200e
--- /dev/null
+++ b/test-credential.c
@@ -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.rc4.4.g884ec

[PATCHv2 04/13] credential: add function for parsing url components

[Jeff King]

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

  http://foo:bar@example.com/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.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-credentials.txt |    4 ++
 credential.c                                |   52 +++++++++++++++++++++++++++
 credential.h                                |    1 +
 3 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index f624aef..21ca6a2 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -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
 -------
 
diff --git a/credential.c b/credential.c
index 86397f3..c349b9a 100644
--- a/credential.c
+++ b/credential.c
@@ -2,6 +2,7 @@
 #include "credential.h"
 #include "string-list.h"
 #include "run-command.h"
+#include "url.h"
 
 void credential_init(struct credential *c)
 {
@@ -232,3 +233,54 @@ void credential_reject(struct credential *c)
 	c->password = NULL;
 	c->approved = 0;
 }
+
+void credential_from_url(struct credential *c, const char *url)
+{
+	const char *at, *colon, *cp, *slash, *host, *proto_end;
+
+	credential_clear(c);
+
+	/*
+	 * Match one of:
+	 *   (1) proto://<host>/...
+	 *   (2) proto://<user>@<host>/...
+	 *   (3) proto://<user>:<pass>@<host>/...
+	 */
+	proto_end = strstr(url, "://");
+	if (!proto_end)
+		return;
+	cp = proto_end + 3;
+	at = strchr(cp, '@');
+	colon = strchr(cp, ':');
+	slash = strchrnul(cp, '/');
+
+	if (!at || slash <= at) {
+		/* Case (1) */
+		host = cp;
+	}
+	else if (!colon || at <= colon) {
+		/* Case (2) */
+		c->username = url_decode_mem(cp, at - cp);
+		host = at + 1;
+	} else {
+		/* Case (3) */
+		c->username = url_decode_mem(cp, colon - cp);
+		c->password = url_decode_mem(colon + 1, at - (colon + 1));
+		host = at + 1;
+	}
+
+	if (proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (slash - host > 0)
+		c->host = url_decode_mem(host, slash - host);
+	/* Trim leading and trailing slashes from path */
+	while (*slash == '/')
+		slash++;
+	if (*slash) {
+		char *p;
+		c->path = url_decode(slash);
+		p = c->path + strlen(c->path) - 1;
+		while (p > c->path && *p == '/')
+			*p-- = '\0';
+	}
+}
diff --git a/credential.h b/credential.h
index 2ea7d49..8a6d162 100644
--- a/credential.h
+++ b/credential.h
@@ -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.rc4.4.g884ec

[PATCHv2 05/13] http: use credential API to get passwords

[Jeff King]

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://user@example.com/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".

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                |  113 +++++++++++--------------------------------------
 t/t5550-http-fetch.sh |   38 ++++++++++++-----
 2 files changed, 52 insertions(+), 99 deletions(-)

diff --git a/http.c b/http.c
index e6c7597..917a1ae 100644
--- a/http.c
+++ b/http.c
@@ -3,6 +3,7 @@
 #include "sideband.h"
 #include "run-command.h"
 #include "url.h"
+#include "credential.h"
 
 int data_received;
 int active_requests;
@@ -42,7 +43,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
@@ -53,7 +54,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;
@@ -139,27 +140,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)) {
@@ -232,11 +212,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));
 	}
@@ -244,18 +224,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)
@@ -282,7 +258,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);
@@ -324,42 +300,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);
@@ -432,7 +372,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://"))
@@ -481,10 +421,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;
 }
@@ -836,17 +776,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;
 }
 
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 3d6e871..398a2d2 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -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.rc4.4.g884ec

[PATCHv2 06/13] credential: apply helper config

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential.c           |   61 ++++++++++++++++++++++++++++++++++++++++++++++++
 credential.h           |    5 +++-
 t/t0300-credentials.sh |   42 +++++++++++++++++++++++++++++++++
 t/t5550-http-fetch.sh  |   12 +++++++++
 4 files changed, 119 insertions(+), 1 deletions(-)

diff --git a/credential.c b/credential.c
index c349b9a..96be1c2 100644
--- a/credential.c
+++ b/credential.c
@@ -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");
 
diff --git a/credential.h b/credential.h
index 8a6d162..e504272 100644
--- a/credential.h
+++ b/credential.h
@@ -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 */
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 81a455f..e3f61f4 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -192,4 +192,46 @@ test_expect_success 'internal getpass does not ask for known username' '
 	EOF
 '
 
+HELPER="f() {
+		cat >/dev/null
+		echo username=foo
+		echo password=bar
+	}; f"
+test_expect_success 'respect configured credentials' '
+	test_config credential.helper "$HELPER" &&
+	check fill <<-\EOF
+	--
+	username=foo
+	password=bar
+	--
+	EOF
+'
+
+test_expect_success 'match configured credential' '
+	test_config credential.https://example.com.helper "$HELPER" &&
+	check fill <<-\EOF
+	protocol=https
+	host=example.com
+	path=repo.git
+	--
+	username=foo
+	password=bar
+	--
+	EOF
+'
+
+test_expect_success 'do not match configured credential' '
+	test_config credential.https://foo.helper "$HELPER" &&
+	check fill <<-\EOF
+	protocol=https
+	host=bar
+	--
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username for '\''https://bar'\'':
+	askpass: Password for '\''https://askpass-username@bar'\'':
+	EOF
+'
+
 test_done
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 398a2d2..21e2f5d 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -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.rc4.4.g884ec

Re: [PATCHv2 06/13] credential: apply helper config

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index 81a455f..e3f61f4 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -192,4 +192,46 @@ test_expect_success 'internal getpass does not ask for known username' '
>  	EOF
>  '
>  
> +HELPER="f() {
> +		cat >/dev/null
> +		echo username=foo
> +		echo password=bar
> +	}; f"
> +test_expect_success 'respect configured credentials' '
> +	test_config credential.helper "$HELPER" &&
> +	check fill <<-\EOF
> +	--
> +	username=foo
> +	password=bar
> +	--
> +	EOF
> +'

Hmm, why do I get ask-ass-{username,password} from this one?

> +test_expect_success 'match configured credential' '
> +	test_config credential.https://example.com.helper "$HELPER" &&
> +	check fill <<-\EOF
> +	protocol=https
> +	host=example.com
> +	path=repo.git
> +	--
> +	username=foo
> +	password=bar
> +	--
> +	EOF
> +'

And this one, too...

Re: [PATCHv2 06/13] credential: apply helper config

[Jeff King]

On Tue, Dec 06, 2011 at 03:58:35PM -0800, Junio C Hamano wrote:

> > +test_expect_success 'respect configured credentials' '
> > +	test_config credential.helper "$HELPER" &&
> > +	check fill <<-\EOF
> > +	--
> > +	username=foo
> > +	password=bar
> > +	--
> > +	EOF
> > +'
> 
> Hmm, why do I get ask-ass-{username,password} from this one?

Ugh. Because apparently one of my re-roll tweaks from patch 03 regressed
this. Sorry, I should have been more careful about running the full
suite, not just the tests in the commits I tweaked.

Let me investigate and get back to you.

-Peff

Re: [PATCHv2 06/13] credential: apply helper config

[Jeff King]

On Tue, Dec 06, 2011 at 07:45:11PM -0500, Jeff King wrote:

> On Tue, Dec 06, 2011 at 03:58:35PM -0800, Junio C Hamano wrote:
> 
> > > +test_expect_success 'respect configured credentials' '
> > > +	test_config credential.helper "$HELPER" &&
> > > +	check fill <<-\EOF
> > > +	--
> > > +	username=foo
> > > +	password=bar
> > > +	--
> > > +	EOF
> > > +'
> > 
> > Hmm, why do I get ask-ass-{username,password} from this one?
> 
> Ugh. Because apparently one of my re-roll tweaks from patch 03 regressed
> this. Sorry, I should have been more careful about running the full
> suite, not just the tests in the commits I tweaked.
> 
> Let me investigate and get back to you.

Brown paper bag time.

This needs squashed in, due to the changes in patch 03/13:

---
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index e3f61f4..42d0f5b 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -192,7 +192,7 @@ test_expect_success 'internal getpass does not ask for known username' '
 	EOF
 '
 
-HELPER="f() {
+HELPER="!f() {
 		cat >/dev/null
 		echo username=foo
 		echo password=bar
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 21e2f5d..c59908f 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -102,7 +102,7 @@ test_expect_success 'http auth can request both user and pass' '
 '
 
 test_expect_success 'http auth respects credential helper config' '
-	test_config_global credential.helper "f() {
+	test_config_global credential.helper "!f() {
 		cat >/dev/null
 		echo username=user@host
 		echo password=user@host

[PATCHv2 07/13] credential: add credential.*.username

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential.c           |    4 ++++
 t/t0300-credentials.sh |   13 +++++++++++++
 t/t5550-http-fetch.sh  |   16 ++++++++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/credential.c b/credential.c
index 96be1c2..3c17ea1 100644
--- a/credential.c
+++ b/credential.c
@@ -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;
 }
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index e3f61f4..57751a1 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -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://foo@example.com'\'':
+	EOF
+'
+
 test_done
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 21e2f5d..18f4d59 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -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.rc4.4.g884ec

[PATCHv2 08/13] credential: make relevance of http path configurable

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 credential.c           |   14 ++++++++++++++
 credential.h           |    3 ++-
 t/t0300-credentials.sh |   29 +++++++++++++++++++++++++++++
 t/t5550-http-fetch.sh  |    2 +-
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c
index 3c17ea1..a17eafe 100644
--- a/credential.c
+++ b/credential.c
@@ -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)
diff --git a/credential.h b/credential.h
index e504272..96ea41b 100644
--- a/credential.h
+++ b/credential.h
@@ -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;
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 57751a1..89a8531 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -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
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 18f4d59..dd073d1 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -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.rc4.4.g884ec

[PATCHv2 09/13] docs: end-user documentation for the credential subsystem

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/Makefile           |    1 +
 Documentation/config.txt         |   23 +++++
 Documentation/gitcredentials.txt |  171 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/gitcredentials.txt

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 304b31e..116f175 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -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))
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5a841da..36bcdf2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -832,6 +832,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::
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
new file mode 100644
index 0000000..07f6596
--- /dev/null
+++ b/Documentation/gitcredentials.txt
@@ -0,0 +1,171 @@
+gitcredentials(7)
+=================
+
+NAME
+----
+gitcredentials - providing usernames and passwords to git
+
+SYNOPSIS
+--------
+------------------
+git config credential.https://example.com.username myusername
+git config credential.helper "$helper $options"
+------------------
+
+DESCRIPTION
+-----------
+
+Git will sometimes need credentials from the user in order to perform
+operations; for example, it may need to ask for a username and password
+in order to access a remote repository over HTTP. This manual describes
+the mechanisms git uses to request these credentials, as well as some
+features to avoid inputting these credentials repeatedly.
+
+REQUESTING CREDENTIALS
+----------------------
+
+Without any credential helpers defined, git will try the following
+strategies to ask the user for usernames and passwords:
+
+1. If the `GIT_ASKPASS` environment variable is set, the program
+   specified by the variable is invoked. A suitable prompt is provided
+   to the program on the command line, and the user's input is read
+   from its standard output.
+
+2. Otherwise, if the `core.askpass` configuration variable is set, its
+   value is used as above.
+
+3. Otherwise, if the `SSH_ASKPASS` environment variable is set, its
+   value is used as above.
+
+4. Otherwise, the user is prompted on the terminal.
+
+AVOIDING REPETITION
+-------------------
+
+It can be cumbersome to input the same credentials over and over.  Git
+provides two methods to reduce this annoyance:
+
+1. Static configuration of usernames for a given authentication context.
+
+2. Credential helpers to cache or store passwords, or to interact with
+   a system password wallet or keychain.
+
+The first is simple and appropriate if you do not have secure storage available
+for a password. It is generally configured by adding this to your config:
+
+---------------------------------------
+[credential "https://example.com"]
+	username = me
+---------------------------------------
+
+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
+credential.helper variable.
+
+1. Find a helper.
++
+-------------------------------------------
+$ git help -a | grep credential-
+credential-foo
+-------------------------------------------
+
+2. Read its description.
++
+-------------------------------------------
+$ git help credential-foo
+-------------------------------------------
+
+3. Tell git to use it.
++
+-------------------------------------------
+$ git config --global credential.helper foo
+-------------------------------------------
+
+If there are multiple instances of the `credential.helper` configuration
+variable, each helper will be tried in turn, and may provide a username,
+password, or nothing. Once git has acquired both a username and a
+password, no more helpers will be tried.
+
+
+CREDENTIAL CONTEXTS
+-------------------
+
+Git considers each credential to have a context defined by a URL. This context
+is used to look up context-specific configuration, and is passed to any
+helpers, which may use it as an index into secure storage.
+
+For instance, imagine we are accessing `https://example.com/foo.git`. When git
+looks into a config file to see if a section matches this context, it will
+consider the two a match if the context is a more-specific subset of the
+pattern in the config file. For example, if you have this in your config file:
+
+--------------------------------------
+[credential "https://example.com"]
+	username = foo
+--------------------------------------
+
+then we will match: both protocols are the same, both hosts are the same, and
+the "pattern" URL does not care about the path component at all. However, this
+context would not match:
+
+--------------------------------------
+[credential "https://kernel.org"]
+	username = foo
+--------------------------------------
+
+because the hostnames differ. Nor would it match `foo.example.com`; git
+compares hostnames exactly, without considering whether two hosts are part of
+the same domain. Likewise, a config entry for `http://example.com` would not
+match: git compares the protocols exactly.
+
+
+CONFIGURATION OPTIONS
+---------------------
+
+Options for a credential context can be configured either in
+`credential.\*` (which applies to all credentials), or
+`credential.<url>.\*`, where <url> matches the context as described
+above.
+
+The following options are available in either location:
+
+helper::
+
+	The name of an external credential helper, and any associated options.
+	If the helper name is not an absolute path, then the string `git
+	credential-` is prepended. The resulting string is executed by the
+	shell (so, for example, setting this to `foo --option=bar` will execute
+	`git credential-foo --option=bar` via the shell. See the manual of
+	specific helpers for examples of their use.
+
+username::
+
+	A default username, if one is not provided in the URL.
+
+useHttpPath::
+
+	By default, git does not consider the "path" component of an http URL
+	to be worth matching via external helpers. This means that a credential
+	stored for `https://example.com/foo.git` will also be used for
+	`https://example.com/bar.git`. If you do want to distinguish these
+	cases, set this option to `true`.
+
+
+CUSTOM HELPERS
+--------------
+
+You can write your own custom helpers to interface with any system in
+which you keep credentials. See the documentation for git's
+link:technical/api-credentials.html[credentials API] for details.
+
+GIT
+---
+Part of the linkgit:git[1] suite
-- 
1.7.8.rc4.4.g884ec

[PATCHv2 10/13] credentials: add "cache" helper

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 .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                            |  220 +++++++++++++++++++
 t/t0301-credential-cache.sh                    |   18 ++
 unix-socket.c                                  |   56 +++++
 unix-socket.h                                  |    7 +
 12 files changed, 811 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

diff --git a/.gitignore b/.gitignore
index 7d2fefc..a6b0bd4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -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
diff --git a/Documentation/git-credential-cache--daemon.txt b/Documentation/git-credential-cache--daemon.txt
new file mode 100644
index 0000000..11edc5a
--- /dev/null
+++ b/Documentation/git-credential-cache--daemon.txt
@@ -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
diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
new file mode 100644
index 0000000..f3d09c5
--- /dev/null
+++ b/Documentation/git-credential-cache.txt
@@ -0,0 +1,77 @@
+git-credential-cache(1)
+=======================
+
+NAME
+----
+git-credential-cache - helper to temporarily store passwords in memory
+
+SYNOPSIS
+--------
+-----------------------------
+git config credential.helper 'cache [options]'
+-----------------------------
+
+DESCRIPTION
+-----------
+
+This command caches credentials in memory for use by future git
+programs. The stored credentials never touch the disk, and are forgotten
+after a configurable timeout.  The cache is accessible over a Unix
+domain socket, restricted to the current user by filesystem permissions.
+
+You probably don't want to invoke this command directly; it is meant to
+be used as a credential helper by other parts of git. See
+linkgit:gitcredentials[7] or `EXAMPLES` below.
+
+OPTIONS
+-------
+
+--timeout <seconds>::
+
+	Number of seconds to cache credentials (default: 900).
+
+--socket <path>::
+
+	Use `<path>` to contact a running cache daemon (or start a new
+	cache daemon if one is not started). Defaults to
+	`~/.git-credential-cache/socket`. If your home directory is on a
+	network-mounted filesystem, you may need to change this to a
+	local filesystem.
+
+CONTROLLING THE DAEMON
+----------------------
+
+If you would like the daemon to exit early, forgetting all cached
+credentials before their timeout, you can issue an `exit` action:
+
+--------------------------------------
+git credential-cache exit
+--------------------------------------
+
+EXAMPLES
+--------
+
+The point of this helper is to reduce the number of times you must type
+your username or password. For example:
+
+------------------------------------
+$ git config credential.helper cache
+$ git push http://example.com/repo.git
+Username: <type your username>
+Password: <type your password>
+
+[work for 5 more minutes]
+$ git push http://example.com/repo.git
+[your credentials are used automatically]
+------------------------------------
+
+You can provide options via the credential.helper configuration
+variable (this example drops the cache time to 5 minutes):
+
+-------------------------------------------------------
+$ git config credential.helper 'cache --timeout=300'
+-------------------------------------------------------
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 07f6596..4e3f860 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -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.
diff --git a/Makefile b/Makefile
index 5ca363b..5d41c29 100644
--- a/Makefile
+++ b/Makefile
@@ -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))
 
@@ -699,6 +701,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
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
new file mode 100644
index 0000000..390f194
--- /dev/null
+++ b/credential-cache--daemon.c
@@ -0,0 +1,269 @@
+#include "cache.h"
+#include "credential.h"
+#include "unix-socket.h"
+#include "sigchain.h"
+
+static const char *socket_path;
+
+static void cleanup_socket(void)
+{
+	if (socket_path)
+		unlink(socket_path);
+}
+
+static void cleanup_socket_on_signal(int sig)
+{
+	cleanup_socket();
+	sigchain_pop(sig);
+	raise(sig);
+}
+
+struct credential_cache_entry {
+	struct credential item;
+	unsigned long expiration;
+};
+static struct credential_cache_entry *entries;
+static int entries_nr;
+static int entries_alloc;
+
+static void cache_credential(struct credential *c, int timeout)
+{
+	struct credential_cache_entry *e;
+
+	ALLOC_GROW(entries, entries_nr + 1, entries_alloc);
+	e = &entries[entries_nr++];
+
+	/* take ownership of pointers */
+	memcpy(&e->item, c, sizeof(*c));
+	memset(c, 0, sizeof(*c));
+	e->expiration = time(NULL) + timeout;
+}
+
+static struct credential_cache_entry *lookup_credential(const struct credential *c)
+{
+	int i;
+	for (i = 0; i < entries_nr; i++) {
+		struct credential *e = &entries[i].item;
+		if (credential_match(c, e))
+			return &entries[i];
+	}
+	return NULL;
+}
+
+static void remove_credential(const struct credential *c)
+{
+	struct credential_cache_entry *e;
+
+	e = lookup_credential(c);
+	if (e)
+		e->expiration = 0;
+}
+
+static int check_expirations(void)
+{
+	static unsigned long wait_for_entry_until;
+	int i = 0;
+	unsigned long now = time(NULL);
+	unsigned long next = (unsigned long)-1;
+
+	/*
+	 * Initially give the client 30 seconds to actually contact us
+	 * and store a credential before we decide there's no point in
+	 * keeping the daemon around.
+	 */
+	if (!wait_for_entry_until)
+		wait_for_entry_until = now + 30;
+
+	while (i < entries_nr) {
+		if (entries[i].expiration <= now) {
+			entries_nr--;
+			credential_clear(&entries[i].item);
+			if (i != entries_nr)
+				memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));
+			/*
+			 * Stick around 30 seconds in case a new credential
+			 * shows up (e.g., because we just removed a failed
+			 * one, and we will soon get the correct one).
+			 */
+			wait_for_entry_until = now + 30;
+		}
+		else {
+			if (entries[i].expiration < next)
+				next = entries[i].expiration;
+			i++;
+		}
+	}
+
+	if (!entries_nr) {
+		if (wait_for_entry_until <= now)
+			return 0;
+		next = wait_for_entry_until;
+	}
+
+	return next - now;
+}
+
+static int read_request(FILE *fh, struct credential *c,
+			struct strbuf *action, int *timeout) {
+	static struct strbuf item = STRBUF_INIT;
+	const char *p;
+
+	strbuf_getline(&item, fh, '\n');
+	p = skip_prefix(item.buf, "action=");
+	if (!p)
+		return error("client sent bogus action line: %s", item.buf);
+	strbuf_addstr(action, p);
+
+	strbuf_getline(&item, fh, '\n');
+	p = skip_prefix(item.buf, "timeout=");
+	if (!p)
+		return error("client sent bogus timeout line: %s", item.buf);
+	*timeout = atoi(p);
+
+	if (credential_read(c, fh) < 0)
+		return -1;
+	return 0;
+}
+
+static void serve_one_client(FILE *in, FILE *out)
+{
+	struct credential c = CREDENTIAL_INIT;
+	struct strbuf action = STRBUF_INIT;
+	int timeout = -1;
+
+	if (read_request(in, &c, &action, &timeout) < 0)
+		/* ignore error */ ;
+	else if (!strcmp(action.buf, "get")) {
+		struct credential_cache_entry *e = lookup_credential(&c);
+		if (e) {
+			fprintf(out, "username=%s\n", e->item.username);
+			fprintf(out, "password=%s\n", e->item.password);
+		}
+	}
+	else if (!strcmp(action.buf, "exit"))
+		exit(0);
+	else if (!strcmp(action.buf, "erase"))
+		remove_credential(&c);
+	else if (!strcmp(action.buf, "store")) {
+		if (timeout < 0)
+			warning("cache client didn't specify a timeout");
+		else if (!c.username || !c.password)
+			warning("cache client gave us a partial credential");
+		else {
+			remove_credential(&c);
+			cache_credential(&c, timeout);
+		}
+	}
+	else
+		warning("cache client sent unknown action: %s", action.buf);
+
+	credential_clear(&c);
+	strbuf_release(&action);
+}
+
+static int serve_cache_loop(int fd)
+{
+	struct pollfd pfd;
+	unsigned long wakeup;
+
+	wakeup = check_expirations();
+	if (!wakeup)
+		return 0;
+
+	pfd.fd = fd;
+	pfd.events = POLLIN;
+	if (poll(&pfd, 1, 1000 * wakeup) < 0) {
+		if (errno != EINTR)
+			die_errno("poll failed");
+		return 1;
+	}
+
+	if (pfd.revents & POLLIN) {
+		int client, client2;
+		FILE *in, *out;
+
+		client = accept(fd, NULL, NULL);
+		if (client < 0) {
+			warning("accept failed: %s", strerror(errno));
+			return 1;
+		}
+		client2 = dup(client);
+		if (client2 < 0) {
+			warning("dup failed: %s", strerror(errno));
+			close(client);
+			return 1;
+		}
+
+		in = xfdopen(client, "r");
+		out = xfdopen(client2, "w");
+		serve_one_client(in, out);
+		fclose(in);
+		fclose(out);
+	}
+	return 1;
+}
+
+static void serve_cache(const char *socket_path)
+{
+	int fd;
+
+	fd = unix_stream_listen(socket_path);
+	if (fd < 0)
+		die_errno("unable to bind to '%s'", socket_path);
+
+	printf("ok\n");
+	fclose(stdout);
+
+	while (serve_cache_loop(fd))
+		; /* nothing */
+
+	close(fd);
+	unlink(socket_path);
+}
+
+static const char permissions_advice[] =
+"The permissions on your socket directory are too loose; other\n"
+"users may be able to read your cached credentials. Consider running:\n"
+"\n"
+"	chmod 0700 %s";
+static void check_socket_directory(const char *path)
+{
+	struct stat st;
+	char *path_copy = xstrdup(path);
+	char *dir = dirname(path_copy);
+
+	if (!stat(dir, &st)) {
+		if (st.st_mode & 077)
+			die(permissions_advice, dir);
+		free(path_copy);
+		return;
+	}
+
+	/*
+	 * We must be sure to create the directory with the correct mode,
+	 * not just chmod it after the fact; otherwise, there is a race
+	 * condition in which somebody can chdir to it, sleep, then try to open
+	 * our protected socket.
+	 */
+	if (safe_create_leading_directories_const(dir) < 0)
+		die_errno("unable to create directories for '%s'", dir);
+	if (mkdir(dir, 0700) < 0)
+		die_errno("unable to mkdir '%s'", dir);
+	free(path_copy);
+}
+
+int main(int argc, const char **argv)
+{
+	socket_path = argv[1];
+
+	if (!socket_path)
+		die("usage: git-credential-cache--daemon <socket_path>");
+	check_socket_directory(socket_path);
+
+	atexit(cleanup_socket);
+	sigchain_push_common(cleanup_socket_on_signal);
+
+	serve_cache(socket_path);
+
+	return 0;
+}
diff --git a/credential-cache.c b/credential-cache.c
new file mode 100644
index 0000000..dc98372
--- /dev/null
+++ b/credential-cache.c
@@ -0,0 +1,120 @@
+#include "cache.h"
+#include "credential.h"
+#include "string-list.h"
+#include "parse-options.h"
+#include "unix-socket.h"
+#include "run-command.h"
+
+#define FLAG_SPAWN 0x1
+#define FLAG_RELAY 0x2
+
+static int send_request(const char *socket, const struct strbuf *out)
+{
+	int got_data = 0;
+	int fd = unix_stream_connect(socket);
+
+	if (fd < 0)
+		return -1;
+
+	if (write_in_full(fd, out->buf, out->len) < 0)
+		die_errno("unable to write to cache daemon");
+	shutdown(fd, SHUT_WR);
+
+	while (1) {
+		char in[1024];
+		int r;
+
+		r = read_in_full(fd, in, sizeof(in));
+		if (r == 0)
+			break;
+		if (r < 0)
+			die_errno("read error from cache daemon");
+		write_or_die(1, in, r);
+		got_data = 1;
+	}
+	return got_data;
+}
+
+static void spawn_daemon(const char *socket)
+{
+	struct child_process daemon;
+	const char *argv[] = { NULL, NULL, NULL };
+	char buf[128];
+	int r;
+
+	memset(&daemon, 0, sizeof(daemon));
+	argv[0] = "git-credential-cache--daemon";
+	argv[1] = socket;
+	daemon.argv = argv;
+	daemon.no_stdin = 1;
+	daemon.out = -1;
+
+	if (start_command(&daemon))
+		die_errno("unable to start cache daemon");
+	r = read_in_full(daemon.out, buf, sizeof(buf));
+	if (r < 0)
+		die_errno("unable to read result code from cache daemon");
+	if (r != 3 || memcmp(buf, "ok\n", 3))
+		die("cache daemon did not start: %.*s", r, buf);
+	close(daemon.out);
+}
+
+static void do_cache(const char *socket, const char *action, int timeout,
+		     int flags)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "action=%s\n", action);
+	strbuf_addf(&buf, "timeout=%d\n", timeout);
+	if (flags & FLAG_RELAY) {
+		if (strbuf_read(&buf, 0, 0) < 0)
+			die_errno("unable to relay credential");
+	}
+
+	if (!send_request(socket, &buf))
+		return;
+	if (flags & FLAG_SPAWN) {
+		spawn_daemon(socket);
+		send_request(socket, &buf);
+	}
+	strbuf_release(&buf);
+}
+
+int main(int argc, const char **argv)
+{
+	char *socket_path = NULL;
+	int timeout = 900;
+	const char *op;
+	const char * const usage[] = {
+		"git credential-cache [options] <action>",
+		NULL
+	};
+	struct option options[] = {
+		OPT_INTEGER(0, "timeout", &timeout,
+			    "number of seconds to cache credentials"),
+		OPT_STRING(0, "socket", &socket_path, "path",
+			   "path of cache-daemon socket"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, NULL, options, usage, 0);
+	if (!argc)
+		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 (!strcmp(op, "exit"))
+		do_cache(socket_path, op, timeout, 0);
+	else if (!strcmp(op, "get") || !strcmp(op, "erase"))
+		do_cache(socket_path, op, timeout, FLAG_RELAY);
+	else if (!strcmp(op, "store"))
+		do_cache(socket_path, op, timeout, FLAG_RELAY|FLAG_SPAWN);
+	else
+		; /* ignore unknown operation */
+
+	return 0;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 8b4dd5c..5c238bd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -130,6 +130,7 @@
 #include <arpa/inet.h>
 #include <netdb.h>
 #include <pwd.h>
+#include <sys/un.h>
 #ifndef NO_INTTYPES_H
 #include <inttypes.h>
 #else
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 54ae1f4..fc34447 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -21,6 +21,226 @@ read_chunk() {
 	done
 }
 
+# Clear any residual data from previous tests. We only
+# need this when testing third-party helpers which read and
+# write outside of our trash-directory sandbox.
+#
+# Don't bother checking for success here, as it is
+# outside the scope of tests and represents a best effort to
+# clean up after ourselves.
+helper_test_clean() {
+	reject $1 https example.com store-user
+	reject $1 https example.com user1
+	reject $1 https example.com user2
+	reject $1 ftp other.tld user
+	reject $1 https timeout.tld user
+}
+
+reject() {
+	(
+		echo protocol=$2
+		echo host=$3
+		echo username=$4
+	) | test-credential reject $1
+}
+
+helper_test() {
+	HELPER=$1
+
+	test_expect_success "helper ($HELPER) has no existing data" '
+		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://askpass-username@example.com'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) stores password" '
+		check approve $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=store-user
+		password=store-pass
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) can retrieve password" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		--
+		username=store-user
+		password=store-pass
+		--
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) requires matching protocol" '
+		check fill $HELPER <<-\EOF
+		protocol=http
+		host=example.com
+		--
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''http://example.com'\'':
+		askpass: Password for '\''http://askpass-username@example.com'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) requires matching host" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=other.tld
+		--
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://other.tld'\'':
+		askpass: Password for '\''https://askpass-username@other.tld'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) requires matching username" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=other
+		--
+		username=other
+		password=askpass-password
+		--
+		askpass: Password for '\''https://other@example.com'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) requires matching path" '
+		check approve $HELPER <<-\EOF &&
+		protocol=ftp
+		host=other.tld
+		path=foo.git
+		username=user
+		password=pass
+		EOF
+		check fill $HELPER <<-\EOF
+		protocol=ftp
+		host=other.tld
+		path=bar.git
+		--
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''ftp://other.tld/bar.git'\'':
+		askpass: Password for '\''ftp://askpass-username@other.tld/bar.git'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) can forget host" '
+		check reject $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		EOF
+		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://askpass-username@example.com'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) can store multiple users" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user1
+		password=pass1
+		EOF
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user2
+		password=pass2
+		EOF
+		check fill $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user1
+		--
+		username=user1
+		password=pass1
+		EOF
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user2
+		--
+		username=user2
+		password=pass2
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) can forget user" '
+		check reject $HELPER <<-\EOF &&
+		protocol=https
+		host=example.com
+		username=user1
+		EOF
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user1
+		--
+		username=user1
+		password=askpass-password
+		--
+		askpass: Password for '\''https://user1@example.com'\'':
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) remembers other user" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user2
+		--
+		username=user2
+		password=pass2
+		EOF
+	'
+}
+
+helper_test_timeout() {
+	HELPER="$*"
+
+	test_expect_success "helper ($HELPER) times out" '
+		check approve "$HELPER" <<-\EOF &&
+		protocol=https
+		host=timeout.tld
+		username=user
+		password=pass
+		EOF
+		sleep 2 &&
+		check fill "$HELPER" <<-\EOF
+		protocol=https
+		host=timeout.tld
+		--
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://timeout.tld'\'':
+		askpass: Password for '\''https://askpass-username@timeout.tld'\'':
+		EOF
+	'
+}
 
 cat >askpass <<\EOF
 #!/bin/sh
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
new file mode 100755
index 0000000..3a65f99
--- /dev/null
+++ b/t/t0301-credential-cache.sh
@@ -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
diff --git a/unix-socket.c b/unix-socket.c
new file mode 100644
index 0000000..84b1509
--- /dev/null
+++ b/unix-socket.c
@@ -0,0 +1,56 @@
+#include "cache.h"
+#include "unix-socket.h"
+
+static int unix_stream_socket(void)
+{
+	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (fd < 0)
+		die_errno("unable to create socket");
+	return fd;
+}
+
+static void unix_sockaddr_init(struct sockaddr_un *sa, const char *path)
+{
+	int size = strlen(path) + 1;
+	if (size > sizeof(sa->sun_path))
+		die("socket path is too long to fit in sockaddr");
+	memset(sa, 0, sizeof(*sa));
+	sa->sun_family = AF_UNIX;
+	memcpy(sa->sun_path, path, size);
+}
+
+int unix_stream_connect(const char *path)
+{
+	int fd;
+	struct sockaddr_un sa;
+
+	unix_sockaddr_init(&sa, path);
+	fd = unix_stream_socket();
+	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		close(fd);
+		return -1;
+	}
+	return fd;
+}
+
+int unix_stream_listen(const char *path)
+{
+	int fd;
+	struct sockaddr_un sa;
+
+	unix_sockaddr_init(&sa, path);
+	fd = unix_stream_socket();
+
+	unlink(path);
+	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		close(fd);
+		return -1;
+	}
+
+	if (listen(fd, 5) < 0) {
+		close(fd);
+		return -1;
+	}
+
+	return fd;
+}
diff --git a/unix-socket.h b/unix-socket.h
new file mode 100644
index 0000000..e271aee
--- /dev/null
+++ b/unix-socket.h
@@ -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.rc4.4.g884ec

[PATCHv2 11/13] strbuf: add strbuf_add*_urlencode

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.c |   37 +++++++++++++++++++++++++++++++++++++
 strbuf.h |    5 +++++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 3ad2cc0..60e5e59 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -397,3 +397,40 @@ int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 
 	return len;
 }
+
+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);
+}
diff --git a/strbuf.h b/strbuf.h
index 46a33f8..cecd48c 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -115,4 +115,9 @@ struct strbuf_expand_dict_entry {
 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.rc4.4.g884ec

[PATCHv2 12/13] credentials: add "store" helper

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore                             |    1 +
 Documentation/git-credential-store.txt |   75 ++++++++++++++++
 Documentation/gitcredentials.txt       |    5 +
 Makefile                               |    1 +
 credential-store.c                     |  148 ++++++++++++++++++++++++++++++++
 t/t0302-credential-store.sh            |    9 ++
 6 files changed, 239 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

diff --git a/.gitignore b/.gitignore
index a6b0bd4..2b7a3f9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -32,6 +32,7 @@
 /git-count-objects
 /git-credential-cache
 /git-credential-cache--daemon
+/git-credential-store
 /git-cvsexportcommit
 /git-cvsimport
 /git-cvsserver
diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
new file mode 100644
index 0000000..3109346
--- /dev/null
+++ b/Documentation/git-credential-store.txt
@@ -0,0 +1,75 @@
+git-credential-store(1)
+=======================
+
+NAME
+----
+git-credential-store - helper to store credentials on disk
+
+SYNOPSIS
+--------
+-------------------
+git config credential.helper 'store [options]'
+-------------------
+
+DESCRIPTION
+-----------
+
+NOTE: Using this helper will store your passwords unencrypted on disk,
+protected only by filesystem permissions. If this is not an acceptable
+security tradeoff, try linkgit:git-credential-cache[1], or find a helper
+that integrates with secure storage provided by your operating system.
+
+This command stores credentials indefinitely on disk for use by future
+git programs.
+
+You probably don't want to invoke this command directly; it is meant to
+be used as a credential helper by other parts of git. See
+linkgit:gitcredentials[7] or `EXAMPLES` below.
+
+OPTIONS
+-------
+
+--store=<path>::
+
+	Use `<path>` to store credentials. The file will have its
+	filesystem permissions set to prevent other users on the system
+	from reading it, but will not be encrypted or otherwise
+	protected. Defaults to `~/.git-credentials`.
+
+EXAMPLES
+--------
+
+The point of this helper is to reduce the number of times you must type
+your username or password. For example:
+
+------------------------------------------
+$ git config credential.helper store
+$ git push http://example.com/repo.git
+Username: <type your username>
+Password: <type your password>
+
+[several days later]
+$ git push http://example.com/repo.git
+[your credentials are used automatically]
+------------------------------------------
+
+STORAGE FORMAT
+--------------
+
+The `.git-credentials` file is stored in plaintext. Each credential is
+stored on its own line as a URL like:
+
+------------------------------
+https://user:pass@example.com
+------------------------------
+
+When git needs authentication for a particular URL context,
+credential-store will consider that context a pattern to match against
+each entry in the credentials file.  If the protocol, hostname, and
+username (if we already have one) match, then the password is returned
+to git. See the discussion of configuration in linkgit:gitcredentials[7]
+for more information.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 4e3f860..066f825 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -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,
diff --git a/Makefile b/Makefile
index 5d41c29..2537128 100644
--- a/Makefile
+++ b/Makefile
@@ -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))
 
diff --git a/credential-store.c b/credential-store.c
new file mode 100644
index 0000000..ed58768
--- /dev/null
+++ b/credential-store.c
@@ -0,0 +1,148 @@
+#include "cache.h"
+#include "credential.h"
+#include "string-list.h"
+#include "parse-options.h"
+
+static struct lock_file credential_lock;
+
+static void parse_credential_file(const char *fn,
+				  struct credential *c,
+				  void (*match_cb)(struct credential *),
+				  void (*other_cb)(struct strbuf *))
+{
+	FILE *fh;
+	struct strbuf line = STRBUF_INIT;
+	struct credential entry = CREDENTIAL_INIT;
+
+	fh = fopen(fn, "r");
+	if (!fh) {
+		if (errno != ENOENT)
+			die_errno("unable to open %s", fn);
+		return;
+	}
+
+	while (strbuf_getline(&line, fh, '\n') != EOF) {
+		credential_from_url(&entry, line.buf);
+		if (entry.username && entry.password &&
+		    credential_match(c, &entry)) {
+			if (match_cb) {
+				match_cb(&entry);
+				break;
+			}
+		}
+		else if (other_cb)
+			other_cb(&line);
+	}
+
+	credential_clear(&entry);
+	strbuf_release(&line);
+	fclose(fh);
+}
+
+static void print_entry(struct credential *c)
+{
+	printf("username=%s\n", c->username);
+	printf("password=%s\n", c->password);
+}
+
+static void print_line(struct strbuf *buf)
+{
+	strbuf_addch(buf, '\n');
+	write_or_die(credential_lock.fd, buf->buf, buf->len);
+}
+
+static void rewrite_credential_file(const char *fn, struct credential *c,
+				    struct strbuf *extra)
+{
+	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
+		die_errno("unable to get credential storage lock");
+	if (extra)
+		print_line(extra);
+	parse_credential_file(fn, c, NULL, print_line);
+	if (commit_lock_file(&credential_lock) < 0)
+		die_errno("unable to commit credential store");
+}
+
+static void store_credential(const char *fn, struct credential *c)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	/*
+	 * Sanity check that what we are storing is actually sensible.
+	 * In particular, we can't make a URL without a protocol field.
+	 * Without either a host or pathname (depending on the scheme),
+	 * we have no primary key. And without a username and password,
+	 * we are not actually storing a credential.
+	 */
+	if (!c->protocol || !(c->host || c->path) ||
+	    !c->username || !c->password)
+		return;
+
+	strbuf_addf(&buf, "%s://", c->protocol);
+	strbuf_addstr_urlencode(&buf, c->username, 1);
+	strbuf_addch(&buf, ':');
+	strbuf_addstr_urlencode(&buf, c->password, 1);
+	strbuf_addch(&buf, '@');
+	if (c->host)
+		strbuf_addstr_urlencode(&buf, c->host, 1);
+	if (c->path) {
+		strbuf_addch(&buf, '/');
+		strbuf_addstr_urlencode(&buf, c->path, 0);
+	}
+
+	rewrite_credential_file(fn, c, &buf);
+	strbuf_release(&buf);
+}
+
+static void remove_credential(const char *fn, struct credential *c)
+{
+	rewrite_credential_file(fn, c, NULL);
+}
+
+static int lookup_credential(const char *fn, struct credential *c)
+{
+	parse_credential_file(fn, c, print_entry, NULL);
+	return c->username && c->password;
+}
+
+int main(int argc, const char **argv)
+{
+	const char * const usage[] = {
+		"git credential-store [options] <action>",
+		NULL
+	};
+	const char *op;
+	struct credential c = CREDENTIAL_INIT;
+	char *file = NULL;
+	struct option options[] = {
+		OPT_STRING_LIST(0, "file", &file, "path",
+				"fetch and store credentials in <path>"),
+		OPT_END()
+	};
+
+	umask(077);
+
+	argc = parse_options(argc, argv, NULL, options, usage, 0);
+	if (argc != 1)
+		usage_with_options(usage, options);
+	op = argv[0];
+
+	if (!file)
+		file = expand_user_path("~/.git-credentials");
+	if (!file)
+		die("unable to set up default path; use --file");
+
+	if (credential_read(&c, stdin) < 0)
+		die("unable to read credential");
+
+	if (!strcmp(op, "get"))
+		lookup_credential(file, &c);
+	else if (!strcmp(op, "erase"))
+		remove_credential(file, &c);
+	else if (!strcmp(op, "store"))
+		store_credential(file, &c);
+	else
+		; /* Ignore unknown operation. */
+
+	return 0;
+}
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
new file mode 100755
index 0000000..f61b40c
--- /dev/null
+++ b/t/t0302-credential-store.sh
@@ -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.rc4.4.g884ec

Re: [PATCHv2 12/13] credentials: add "store" helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +static void store_credential(const char *fn, struct credential *c)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	/*
> +	 * Sanity check that what we are storing is actually sensible.
> +	 * In particular, we can't make a URL without a protocol field.
> +	 * Without either a host or pathname (depending on the scheme),
> +	 * we have no primary key. And without a username and password,
> +	 * we are not actually storing a credential.
> +	 */
> +	if (!c->protocol || !(c->host || c->path) ||
> +	    !c->username || !c->password)
> +		return;

Very nicely explained. I wish all our patches had comments like this to
explain tricky bit that looks as if the choice was arbitrarily made but
in fact the logic was carefully constructed.

Thanks.

Re: [PATCHv2 12/13] credentials: add "store" helper

[Jeff King]

On Tue, Dec 06, 2011 at 01:23:05AM -0500, Jeff King wrote:

> +int main(int argc, const char **argv)
> +{
> +	const char * const usage[] = {
> +		"git credential-store [options] <action>",
> +		NULL
> +	};
> +	const char *op;
> +	struct credential c = CREDENTIAL_INIT;
> +	char *file = NULL;
> +	struct option options[] = {
> +		OPT_STRING_LIST(0, "file", &file, "path",
> +				"fetch and store credentials in <path>"),
> +		OPT_END()
> +	};

Eek, this should be OPT_STRING, of course. I'll fix it in my next
re-roll, but I wanted to mention it in case anybody is reviewing v2.

-Peff

[PATCHv2 13/13] t: add test harness for external credential helpers

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0303-credential-external.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100755 t/t0303-credential-external.sh

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
new file mode 100755
index 0000000..79b046f
--- /dev/null
+++ b/t/t0303-credential-external.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='external credential helper tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
+	say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
+else
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+#	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
+fi
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
+	say "# skipping external helper timeout tests"
+else
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+fi
+
+test_done
-- 
1.7.8.rc4.4.g884ec

Re: [PATCHv2 13/13] t: add test harness for external credential helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> new file mode 100755
> index 0000000..79b046f
> --- /dev/null
> +++ b/t/t0303-credential-external.sh
> @@ -0,0 +1,23 @@
> ...
> +else
> +	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> +	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> +#	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> +fi

Huh? Leftover debugging cruft?

Re: [PATCHv2 13/13] t: add test harness for external credential helpers

[Jeff King]

On Tue, Dec 06, 2011 at 01:51:43PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> > new file mode 100755
> > index 0000000..79b046f
> > --- /dev/null
> > +++ b/t/t0303-credential-external.sh
> > @@ -0,0 +1,23 @@
> > ...
> > +else
> > +	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> > +	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> > +#	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> > +fi
> 
> Huh? Leftover debugging cruft?

Oops, yes. It should be:

  clean
  do_the_test
  clean

The first clean is "remove any cruft accidentally leftover from a
previous run, so we can do our test". The second clean is "be a good
citizen and get rid of cruft we just accumulated".

As part of my testing, I commented out the second clean momentarily so
that I could verify that the cruft was left without the clean, but gone
with the clean. And then I accidentally committed with the "#" left in.

Here's the correct version, with some extra comments about the
clean-test-clean cycle:

-- >8 --
Subject: [PATCHv2 13/13] t: add test harness for external credential helpers

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

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0303-credential-external.sh |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100755 t/t0303-credential-external.sh

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
new file mode 100755
index 0000000..092dd3c
--- /dev/null
+++ b/t/t0303-credential-external.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='external credential helper tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential.sh
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
+	say "# skipping external helper tests (set GIT_TEST_CREDENTIAL_HELPER)"
+else
+	# 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"
+
+	helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+
+	# then 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"
+fi
+
+if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
+	say "# skipping external helper timeout tests"
+else
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+	helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+fi
+
+test_done
-- 
1.7.8.rc2.8.gf076c

Re: [PATCHv2 0/13] credential helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>     ... You can now
>     do: "git credential-store erase </dev/null" to erase everything
>     (since you have provided no restrictions, it matches everything).

That "justification" does not sound so true to me but perhaps that is
because it is unclear what "erase" means and what it means to give the
operation parameters.

When I see "erase $foo", I would find it natural if $foo meant "if there
is something that matches $foo, then please remove it, but keep everything
else intact", and not the other way around "Match the existing entries
against a pattern (or a set of matching patterns) I am giving you, and
drop all the rest". So if I happen to give you an empty set, I would
expect nothing is removed.

Perhaps the root cause of the issue is that you are treating the input as
"restriction" instead of something that produces "positive matches"?

Confused.

Re: [PATCHv2 0/13] credential helpers

[Jeff King]

On Tue, Dec 06, 2011 at 01:40:17PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >     ... You can now
> >     do: "git credential-store erase </dev/null" to erase everything
> >     (since you have provided no restrictions, it matches everything).
> 
> That "justification" does not sound so true to me but perhaps that is
> because it is unclear what "erase" means and what it means to give the
> operation parameters.

It's not meant to be a justification, but rather an explanation. I think
the behavior is probably too dangerous to leave.

> When I see "erase $foo", I would find it natural if $foo meant "if there
> is something that matches $foo, then please remove it, but keep everything
> else intact", and not the other way around "Match the existing entries
> against a pattern (or a set of matching patterns) I am giving you, and
> drop all the rest". So if I happen to give you an empty set, I would
> expect nothing is removed.

It does do the first thing you mentioned (you provide one pattern $foo,
and we match the pattern you have given). It's just that the pattern you
have specified is "everything". The problem is not in the matching, but
in the pattern specification language.

This pattern:

  protocol=https
  host=github.com

means "match everything that uses https _and_ has a host of github.com".
The username and path fields are not present, which implicitly means
"don't care about them".

Similarly, this pattern:

  protocol=https

means "match everything that uses https". Everything else is not
specified, and therefore we allow anything.

Then what does the empty pattern do? It cares about nothing, and
therefore matches everything.

By itself, I don't think that is a problem. It's something you might
want to specify, and it's logically consistent with the way the patterns
are matched.  What is dangerous, though, is that failing to provide
input is byte-wise identical to the empty pattern. And that's why I say
it's a pattern specification problem.

A rough BNF for the pattern format is something like:

  pattern = *line
  line = key "=" value
  key = *<any byte except NUL, "=", or "\n">
  value = *<any byte except NUL or "\n">

Because the pattern takes 0 or more lines and no terminator, we can't
distinguish between empty or truncated input and the empty pattern. So
one solution would be:

  pattern = *line "\n"

i.e., require a blank line terminator.

Does that explain the issue better?

-Peff

Re: [PATCHv2 0/13] credential helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Because the pattern takes 0 or more lines and no terminator, we can't
> distinguish between empty or truncated input and the empty pattern.

I agree that such a positive "Ok here is the end of specification" marker
is a good idea, even if we do not worry about "an empty set".

When the requestor wants to specify the credentials with host and user,
but the wire is cut after host is communicated but before user is, we do
want to notice the communication error, instead of silently erasing all
the credentials on the host regardless of the user.

Thanks.

Re: [PATCHv2 0/13] credential helpers

[Jeff King]

On Thu, Dec 08, 2011 at 01:34:29PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Because the pattern takes 0 or more lines and no terminator, we can't
> > distinguish between empty or truncated input and the empty pattern.
> 
> I agree that such a positive "Ok here is the end of specification" marker
> is a good idea, even if we do not worry about "an empty set".
> 
> When the requestor wants to specify the credentials with host and user,
> but the wire is cut after host is communicated but before user is, we do
> want to notice the communication error, instead of silently erasing all
> the credentials on the host regardless of the user.

OK, I've tweaked the series to require an end-of-credential marker (a
blank line) both in input and output.

In addition, I've changed the code that runs helpers to make reading
from the helpers an all-or-nothing thing (instead of incrementally
ovewriting our credential as we read from it). Before, if a helper
exited with error, we would happily use its partial result. Instead, we
now read its response into a holding area, and only copy it into our
credential when we get a successful exit code. This lets us detect
truncation when reading from the helper, too.

It works, and it detects truncated output both ways properly (I know
because I had to update every test, since the old output was missing the
end-of-credential marker).

It makes me a little sad, because the original format (relying on EOF)
was so Unix-y. You could make a helper like this:

  echo password=`gpg -qd ~/.secret.gpg`

and now you must remember to tack an extra "echo" at the end. Not a big
deal, but it somehow just feels less elegant to my gut.  OTOH, classic
Unix constructs have always been a nightmare for robustness and error
checking[1], so this is certainly nothing new.

The diff from this tip to the old tip is below to give you a sense of
the magnitude of the change (the individual changes are squashed into
their respective patches for the next re-roll, of course). I'll hold off
on posting the whole series to see if we get any more comments.

-Peff

[1] I mean things like:

      grep foo bar | sed 's/some/transformation/'

    where we totally ignore errors from grep, and where a truncated
    output on the pipe would just subtly generate wrong answers.
---

 Documentation/technical/api-credentials.txt |    2 +-
 credential-cache--daemon.c                  |    1 +
 credential-cache.c                          |    2 +
 credential-store.c                          |    1 +
 credential.c                                |   39 +++++++++++++++++++++++---
 t/lib-credential.sh                         |    1 +
 t/t0300-credentials.sh                      |    3 ++
 t/t5550-http-fetch.sh                       |    1 +
 8 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index 21ca6a2..0aac899 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -199,7 +199,7 @@ followed by a newline. The key may contain any bytes except `=`,
 newline, or NUL. The value may contain any bytes except newline or NUL.
 In both cases, all bytes are treated as-is (i.e., there is no quoting,
 and one cannot transmit a value with newline or NUL in it). The list of
-attributes is terminated by a blank line or end-of-file.
+attributes is terminated by a blank line.
 
 Git will send the following attributes (but may not send all of
 them for a given credential; for example, a `host` attribute makes no
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..38403645 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -138,6 +138,7 @@ static void serve_one_client(FILE *in, FILE *out)
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
+			fprintf(out, "\n");
 		}
 	}
 	else if (!strcmp(action.buf, "exit"))
diff --git a/credential-cache.c b/credential-cache.c
index dc98372..5b8d8c9 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -70,6 +70,8 @@ static void do_cache(const char *socket, const char *action, int timeout,
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die_errno("unable to relay credential");
 	}
+	else
+		strbuf_addch(&buf, '\n');
 
 	if (!send_request(socket, &buf))
 		return;
diff --git a/credential-store.c b/credential-store.c
index ed58768..00e38f0 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -43,6 +43,7 @@ static void print_entry(struct credential *c)
 {
 	printf("username=%s\n", c->username);
 	printf("password=%s\n", c->password);
+	printf("\n");
 }
 
 static void print_line(struct strbuf *buf)
diff --git a/credential.c b/credential.c
index a17eafe..6d2a37d 100644
--- a/credential.c
+++ b/credential.c
@@ -147,8 +147,10 @@ int credential_read(struct credential *c, FILE *fp)
 		char *key = line.buf;
 		char *value = strchr(key, '=');
 
-		if (!line.len)
-			break;
+		if (!line.len) {
+			strbuf_release(&line);
+			return 0;
+		}
 
 		if (!value) {
 			warning("invalid credential line: %s", key);
@@ -181,7 +183,7 @@ int credential_read(struct credential *c, FILE *fp)
 	}
 
 	strbuf_release(&line);
-	return 0;
+	return -1;
 }
 
 static void credential_write_item(FILE *fp, const char *key, const char *value)
@@ -198,6 +200,26 @@ static void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path);
 	credential_write_item(fp, "username", c->username);
 	credential_write_item(fp, "password", c->password);
+	putc('\n', fp);
+}
+
+static void credential_merge_one(char **dst, char **src)
+{
+	if (!*src)
+		return;
+	free(*dst);
+	*dst = *src;
+	*src = NULL;
+}
+
+static void credential_merge(struct credential *dst,
+			     struct credential *src)
+{
+	credential_merge_one(&dst->protocol, &src->protocol);
+	credential_merge_one(&dst->host, &src->host);
+	credential_merge_one(&dst->path, &src->path);
+	credential_merge_one(&dst->username, &src->username);
+	credential_merge_one(&dst->password, &src->password);
 }
 
 static int run_credential_helper(struct credential *c,
@@ -206,6 +228,7 @@ static int run_credential_helper(struct credential *c,
 {
 	struct child_process helper;
 	const char *argv[] = { NULL, NULL };
+	struct credential response = CREDENTIAL_INIT;
 	FILE *fp;
 
 	memset(&helper, 0, sizeof(helper));
@@ -227,17 +250,23 @@ static int run_credential_helper(struct credential *c,
 
 	if (want_output) {
 		int r;
+
 		fp = xfdopen(helper.out, "r");
-		r = credential_read(c, fp);
+		r = credential_read(&response, fp);
 		fclose(fp);
 		if (r < 0) {
+			credential_clear(&response);
 			finish_command(&helper);
 			return -1;
 		}
 	}
 
-	if (finish_command(&helper))
+	if (finish_command(&helper)) {
+		credential_clear(&response);
 		return -1;
+	}
+
+	credential_merge(c, &response);
 	return 0;
 }
 
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index fc34447..c0de4e9 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -5,6 +5,7 @@
 # separated by "--".
 check() {
 	read_chunk >stdin &&
+	echo >>stdin &&
 	read_chunk >expect-stdout &&
 	read_chunk >expect-stderr &&
 	test-credential "$@" <stdin >stdout 2>stderr &&
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..f0e77dc 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -9,6 +9,7 @@ test_expect_success 'setup helper scripts' '
 	whoami=`echo $0 | sed s/.*git-credential-//`
 	echo >&2 "$whoami: $*"
 	while IFS== read key value; do
+		test -z "$key" && break
 		echo >&2 "$whoami: $key=$value"
 		eval "$key=$value"
 	done
@@ -28,6 +29,7 @@ test_expect_success 'setup helper scripts' '
 	. ./dump
 	test -z "$user" || echo username=$user
 	test -z "$pass" || echo password=$pass
+	echo
 	EOF
 	chmod +x git-credential-verbatim &&
 
@@ -196,6 +198,7 @@ HELPER="!f() {
 		cat >/dev/null
 		echo username=foo
 		echo password=bar
+		echo
 	}; f"
 test_expect_success 'respect configured credentials' '
 	test_config credential.helper "$HELPER" &&
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 95a133d..b817c69 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -106,6 +106,7 @@ test_expect_success 'http auth respects credential helper config' '
 		cat >/dev/null
 		echo username=user@host
 		echo password=user@host
+		echo
 	}; f" &&
 	>askpass-query &&
 	echo wrong >askpass-response &&

Re: [PATCHv2 0/13] credential helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It works, and it detects truncated output both ways properly (I know
> because I had to update every test, since the old output was missing the
> end-of-credential marker).
>
> It makes me a little sad, because the original format (relying on EOF)
> was so Unix-y.

It saddens me, too.  A reasonable middle ground would be to stop treating
an empty input as "no restriction" but "never matches".

I suspect that it is far more likely for a helper to fail (due to
configuration errors, for example) before it produces any output than
after it gives some but not all output lines.

Re: [PATCHv2 0/13] credential helpers

[Jeff King]

On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It works, and it detects truncated output both ways properly (I know
> > because I had to update every test, since the old output was missing the
> > end-of-credential marker).
> >
> > It makes me a little sad, because the original format (relying on EOF)
> > was so Unix-y.
> 
> It saddens me, too.  A reasonable middle ground would be to stop treating
> an empty input as "no restriction" but "never matches".
> 
> I suspect that it is far more likely for a helper to fail (due to
> configuration errors, for example) before it produces any output than
> after it gives some but not all output lines.

Yeah, I think that's a reasonable compromise. Instead of the patch I
posted earlier, how about this:

diff --git a/credential-store.c b/credential-store.c
index a2c2cd0..26f7589 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
 
 static void remove_credential(const char *fn, struct credential *c)
 {
-	rewrite_credential_file(fn, c, NULL);
+	/*
+	 * Sanity check that we actually have something to match
+	 * against. The input we get is a restrictive pattern,
+	 * so technically a blank credential means "erase everything".
+	 * But it is too easy to accidentally send this, since it is equivalent
+	 * to empty input. So explicitly disallow it, and require that the
+	 * pattern have some actual content to match.
+	 */
+	if (c->protocol || c->host || c->path || c->username)
+		rewrite_credential_file(fn, c, NULL);
 }
 
 static int lookup_credential(const char *fn, struct credential *c)


We _could_ modify credential_match() to automatically reject such a
pattern at that level, but it does actually have a use on the lookup
side. In config, a context like "https://example.com/foo.git" would
match each of:

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

The final one is an just an extension of the others to the empty pattern
(you could also spell it [credential ""], and it would have the same
effect).

So the "empty pattern" does actually have a use, from the end-users's
point of view. It's just that with removal, it's a little more dangerous
and a little less likely to be useful (as compared to lookup).

-Peff

Re: [PATCHv2 0/13] credential helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I think that's a reasonable compromise. Instead of the patch I
> posted earlier, how about this:
>
> diff --git a/credential-store.c b/credential-store.c
> index a2c2cd0..26f7589 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
>  
>  static void remove_credential(const char *fn, struct credential *c)
>  {
> -	rewrite_credential_file(fn, c, NULL);
> +	/*
> +	 * Sanity check that we actually have something to match
> +	 * against. The input we get is a restrictive pattern,
> +	 * so technically a blank credential means "erase everything".
> +	 * But it is too easy to accidentally send this, since it is equivalent
> +	 * to empty input. So explicitly disallow it, and require that the
> +	 * pattern have some actual content to match.
> +	 */
> +	if (c->protocol || c->host || c->path || c->username)
> +		rewrite_credential_file(fn, c, NULL);
>  }

Looks very sane.

> We _could_ modify credential_match() to automatically reject such a
> pattern at that level,...

I do not think that is such a good idea to modify "match()" function
either, as I agree match with empty has its uses, but that does not stop
"rewrite_credential_file()" from being safe by default, no? After all, the
one that makes the decision to drop things that match the pattern is that
function (it chooses to give NULL to match_cb).

> So the "empty pattern" does actually have a use, from the end-users's
> point of view. It's just that with removal, it's a little more dangerous
> and a little less likely to be useful (as compared to lookup).
>
> -Peff

Re: [PATCHv2 0/13] credential helpers

[Jeff King]

On Fri, Dec 09, 2011 at 03:34:08PM -0800, Junio C Hamano wrote:

> > We _could_ modify credential_match() to automatically reject such a
> > pattern at that level,...
> 
> I do not think that is such a good idea to modify "match()" function
> either, as I agree match with empty has its uses, but that does not stop
> "rewrite_credential_file()" from being safe by default, no? After all, the
> one that makes the decision to drop things that match the pattern is that
> function (it chooses to give NULL to match_cb).

Yeah, you could move it down to that level, but there isn't much point.
rewrite_credential_file is unique to credential-store, and the only two
callers are store_credential (which has its own, stricter rules already)
and remove_credential, which we are modifying here.

Note that I didn't bother with the same safety valve for
credential-cache. It is, after all, a cache that will go away eventually
anyway, so safety is less interesting.

Third-party helpers will have to do their own checks anyway, as in
general I don't plan on them linking directly against git code.

Speaking of which, I hackishly ported Jay's osxkeychain helper to the
new format last night. I'll try to clean that up and post it tonight.

-Peff

Re: [PATCHv2 0/13] credential helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Speaking of which, I hackishly ported Jay's osxkeychain helper to the
> new format last night. I'll try to clean that up and post it tonight.

;-).