git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 5/5] credential-store: use a better storage format

From
Jeff King <peff@peff.net>
Date
Sep 14, 2011, 19:21 UTC
Message-ID
<20110914192117.GE28267@sigill.intra.peff.net>
In-Reply-To
<20110914191704.GA23201@sigill.intra.peff.net>

In the name of simplicity, the credential-store helper re-used git's config file format for on-disk storage of credentials. The scheme looked something like this:

  [credential "unique-context"]
	username = foo
	password = bar

This works fine if you have only one credential for each context. But there's no good way to store separate passwords for user1@unique-context versus user2@unique-context. For end users, this meant that:

  1. Trying to access user2@host when you already have a
     cached credential for user1@host would return user2's
     username with user1's password.
  2. The stored result would also mix the credentials.
  3. Rejection would remove all entries for the host,
     regardless of username.

You can hack around it by assuming that order is important, and that:

  [credential "unique-context"]
	username = user1
	password = pass1
	username = user2
	password = pass2

refers to two distinct credentials. But the config code isn't well setup for that. You have to keep track of the last username field you parsed, and assume the next password field after it is associated with it. Which isn't too bad. But removing just one credential (which we need to do for --reject) is impossible. You can say "delete the username field that is user2", but there is no way to say "delete the password field that comes right after the username field that is user2" (it's tempting to find the password for user2 and say "delete the password field that is pass2", but that's not strictly correct either).

Instead, let's define a new, very simple storage format. Each line is a credential containing three tokens: unique, username, and password. Each is separated by a space, and shell-quoted to avoid ambiguity.

There's now an additional test that we can differentiate the credentials correctly (matching the similar test we already have for credential-cache). The test for removing credentials is also enhanced to check that removing one credential leaves an unrelated one stored.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential-store.c     |  119 ++++++++++++++++++++++++++++++++++++++++++------
 t/t0300-credentials.sh |   38 ++++++++++++++-
 2 files changed, 140 insertions(+), 17 deletions(-)
diff --git a/credential-store.c b/credential-store.c
index 8ab8582..aae8e0c 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -2,40 +2,129 @@
 #include "credential.h"
 #include "string-list.h"
 #include "parse-options.h"
+#include "quote.h"
+
+static struct lock_file credential_lock;
+
+static int parse_credential_file(const char *fn,
+				 struct credential *c,
+				 int (*match_cb)(const char *username,
+						 const char *password,
+						 struct credential *c),
+				 int (*other_cb)(const char *buf))
+{
+	FILE *fh;
+	struct strbuf buf = STRBUF_INIT;
+	const char **argv = NULL;
+	int alloc = 0;
+	int retval = 0;
+
+	fh = fopen(fn, "r");
+	if (!fh)
+		return errno == ENOENT ? 0 : -1;
+
+	while (strbuf_getwholeline(&buf, fh, '\n') != EOF) {
+		int nr = 0;
+		char *pristine = xstrdup(buf.buf);
+
+		strbuf_trim(&buf);
+		if (!sq_dequote_to_argv(buf.buf, &argv, &nr, &alloc) &&
+		    nr == 3 &&
+		    !strcmp(c->unique, argv[0]) &&
+		    (!c->username || !strcmp(c->username, argv[1]))) {
+			if (match_cb(argv[1], argv[2], c) < 0) {
+				retval = -1;
+				break;
+			}
+		}
+		else if (other_cb) {
+			if (other_cb(pristine) < 0) {
+				retval = -1;
+				break;
+			}
+		}
+		free(pristine);
+	}
+
+	free(argv);
+	strbuf_release(&buf);
+	fclose(fh);
+	return retval;
+}
+
+
+static int copy_credential(const char *username, const char *password,
+			   struct credential *c)
+{
+	if (!c->username)
+		c->username = xstrdup(username);
+	free(c->password);
+	c->password = xstrdup(password);
+	return 0;
+}
 
 static int lookup_credential(const char *fn, struct credential *c)
 {
-	config_exclusive_filename = fn;
-	credential_from_config(c);
+	if (!c->unique)
+		return 0;
+	parse_credential_file(fn, c, copy_credential, NULL);
 	return c->username && c->password;
 }
 
-static void store_item(const char *fn, const char *unique,
-		       const char *item, const char *value)
+static int skip_match(const char *username, const char *password,
+		      struct credential *c)
 {
-	struct strbuf key = STRBUF_INIT;
+	return 0;
+}
 
-	if (!unique)
-		return;
+static int print_entry(const char *buf)
+{
+	return write_in_full(credential_lock.fd, buf, strlen(buf));
+}
 
-	config_exclusive_filename = fn;
+static int rewrite_credential_file(const char *fn, struct credential *c,
+				   int replace)
+{
 	umask(077);
+	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
+		return -1;
+	if (parse_credential_file(fn, c, skip_match, print_entry) < 0) {
+		rollback_lock_file(&credential_lock);
+		return -1;
+	}
+	if (replace) {
+		struct strbuf buf = STRBUF_INIT;
+		int r;
 
-	strbuf_addf(&key, "credential.%s.%s", unique, item);
-	git_config_set(key.buf, value);
-	strbuf_release(&key);
+		sq_quote_buf(&buf, c->unique);
+		strbuf_addch(&buf, ' ');
+		sq_quote_buf(&buf, c->username);
+		strbuf_addch(&buf, ' ');
+		sq_quote_buf(&buf, c->password);
+		strbuf_addch(&buf, '\n');
+
+		r = write_in_full(credential_lock.fd, buf.buf, buf.len);
+		strbuf_release(&buf);
+		if (r < 0) {
+			rollback_lock_file(&credential_lock);
+			return -1;
+		}
+	}
+	return commit_lock_file(&credential_lock);
 }
 
 static void store_credential(const char *fn, struct credential *c)
 {
-	store_item(fn, c->unique, "username", c->username);
-	store_item(fn, c->unique, "password", c->password);
+	if (!c->unique || !c->username || !c->password)
+		return;
+	rewrite_credential_file(fn, c, 1);
 }
 
 static void remove_credential(const char *fn, struct credential *c)
 {
-	store_item(fn, c->unique, "username", NULL);
-	store_item(fn, c->unique, "password", NULL);
+	if (!c->unique)
+		return;
+	rewrite_credential_file(fn, c, 0);
 }
 
 int main(int argc, const char **argv)
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 41ab8cc..613c3fb 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -314,6 +314,30 @@ test_expect_success 'credential-store requires matching unique token' '
 	EOF
 '
 
+test_expect_success 'credential-store requires matching usernames' '
+	test_when_finished "rm -f .git-credentials" &&
+	check --unique=host store <<-\EOF &&
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username:
+	askpass: Password:
+	EOF
+	test_when_finished "rm -f askpass-password" &&
+	echo other-password >askpass-password &&
+	check --unique=host --username=other store <<-\EOF &&
+	username=other
+	password=other-password
+	--
+	askpass: Password:
+	EOF
+	check --unique=host --username=askpass-username store <<-\EOF
+	username=askpass-username
+	password=askpass-password
+	--
+	EOF
+'
+
 test_expect_success 'credential-store removes rejected credentials' '
 	test_when_finished "rm -f .git-credentials" &&
 	check --unique=host store <<-\EOF &&
@@ -323,16 +347,26 @@ test_expect_success 'credential-store removes rejected credentials' '
 	askpass: Username:
 	askpass: Password:
 	EOF
+	check --unique=host --username=other store <<-\EOF &&
+	username=other
+	password=askpass-password
+	--
+	askpass: Password:
+	EOF
 	check --reject --unique=host --username=askpass-username store <<-\EOF &&
 	--
 	EOF
-	check --unique=host store <<-\EOF
+	check --unique=host --username=askpass-username store <<-\EOF &&
 	username=askpass-username
 	password=askpass-password
 	--
-	askpass: Username:
 	askpass: Password:
 	EOF
+	check --unique=host --username=other store <<-\EOF
+	username=other
+	password=askpass-password
+	--
+	EOF
 '
 
 test_done

-- 
1.7.6.252.ge9c18
Previous: Jeff KingNext: Junio C Hamano
Message 11 of 12 in “credential helper super fun pak”
  1. 0/5 credential helper super fun pakJeff King, Sep 14, 2011
  2. 1/5 docs: minor tweaks to credentials APIJeff King, Sep 14, 2011
  3. 2/5 credential-cache: fix expiration calculation corner casesJeff King, Sep 14, 2011
  4. Thomas RastSep 15, 2011
  5. 1/2 t0300: add missing EOF terminator for <<Thomas Rast, Sep 16, 2011
  6. 2/2 check_expirations: don't copy over same elementThomas Rast, Sep 16, 2011
  7. Jeff KingSep 16, 2011
  8. Junio C HamanoSep 18, 2011
  9. 3/5 t0300: make askpass tests a little more robustJeff King, Sep 14, 2011
  10. 4/5 t0300: make alternate username tests more robustJeff King, Sep 14, 2011
  11. 5/5 credential-store: use a better storage formatJeff King, Sep 14, 2011
  12. Junio C HamanoSep 14, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.