[PATCH v5 1/3] git-credential-store: support multiple credential files

Subsystems: the rest

STALE3737d

5 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH v5 1/3] git-credential-store: support multiple credential files

From: Paul Tan <hidden>
Date: 2016-06-15 23:04:16

Previously, git-credential-store only supported storing credentials in a
single file: ~/.git-credentials. In order to support the XDG base
directory specification[1], git-credential-store needs to be able to
lookup and erase credentials from multiple files, as well as to pick the
appropriate file to write to so that the credentials can be found on
subsequent lookups.

[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html

Note that some credential storage files may not be owned, readable or
writable by the user, as they may be system-wide files that are meant to
apply to every user.

Instead of a single file path, lookup_credential(), remove_credential()
and store_credential() now take a precedence-ordered string_list of
file paths. lookup_credential() expects both user-specific and
system-wide credential files to be provided to support the use case of
system administrators setting default credentials for users.
remove_credential() and store_credential() expect only the user-specific
credential files to be provided as usually the only config files that
users are allowed to edit are their own user-specific ones.

lookup_credential() will read these (user-specific and system-wide) file
paths in order until it finds the 1st matching credential and print it.
As some files may be private and thus unreadable, any file which cannot
be read will be ignored silently.

remove_credential() will erase credentials from all (user-specific)
files in the list.  This is because if credentials are only erased from
the file with the highest precedence, a matching credential may still be
found in a file further down the list. (Note that due to the lockfile
code, this requires the directory to be writable, which should be so for
user-specific config files)

store_credential() will write the credentials to the first existing
(user-specific) file in the list. If none of the files in the list
exist, store_credential() will write to the filename specified by the
first item of the filename list. For backwards compatibility, this
filename should be "~/.git-credentials".

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Paul Tan <redacted>
---

The previous version can be found at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/265683

The changes are as follows:

* Fix comment formatting error. Thanks Matthieu.

 credential-store.c | 81 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 25 deletions(-)
diff --git a/credential-store.c b/credential-store.c
index 925d3f4..d700a93 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,7 +6,7 @@
 
 static struct lock_file credential_lock;
 
-static void parse_credential_file(const char *fn,
+static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
@@ -14,18 +14,20 @@ static void parse_credential_file(const char *fn,
 	FILE *fh;
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
+	int found_credential = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
-		if (errno != ENOENT)
+		if (errno != ENOENT && errno != EACCES)
 			die_errno("unable to open %s", fn);
-		return;
+		return found_credential;
 	}
 
 	while (strbuf_getline(&line, fh, '\n') != EOF) {
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
+			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
@@ -38,6 +40,7 @@ static void parse_credential_file(const char *fn,
 	credential_clear(&entry);
 	strbuf_release(&line);
 	fclose(fh);
+	return found_credential;
 }
 
 static void print_entry(struct credential *c)
@@ -64,21 +67,10 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to commit credential store");
 }
 
-static void store_credential(const char *fn, struct credential *c)
+static void store_credential_file(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, ':');
@@ -95,8 +87,37 @@ static void store_credential(const char *fn, struct credential *c)
 	strbuf_release(&buf);
 }
 
-static void remove_credential(const char *fn, struct credential *c)
+static void store_credential(const struct string_list *fns, struct credential *c)
 {
+	struct string_list_item *fn;
+
+	/*
+	 * 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;
+
+	for_each_string_list_item(fn, fns)
+		if (!access(fn->string, F_OK)) {
+			store_credential_file(fn->string, c);
+			return;
+		}
+	/*
+	 * Write credential to the filename specified by fns->items[0], thus
+	 * creating it
+	 */
+	if (fns->nr)
+		store_credential_file(fns->items[0].string, c);
+}
+
+static void remove_credential(const struct string_list *fns, struct credential *c)
+{
+	struct string_list_item *fn;
+
 	/*
 	 * Sanity check that we actually have something to match
 	 * against. The input we get is a restrictive pattern,
@@ -105,14 +126,20 @@ static void remove_credential(const char *fn, struct credential *c)
 	 * 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);
+	if (!c->protocol && !c->host && !c->path && !c->username)
+		return;
+	for_each_string_list_item(fn, fns)
+		if (!access(fn->string, F_OK))
+			rewrite_credential_file(fn->string, c, NULL);
 }
 
-static int lookup_credential(const char *fn, struct credential *c)
+static void lookup_credential(const struct string_list *fns, struct credential *c)
 {
-	parse_credential_file(fn, c, print_entry, NULL);
-	return c->username && c->password;
+	struct string_list_item *fn;
+
+	for_each_string_list_item(fn, fns)
+		if (parse_credential_file(fn->string, c, print_entry, NULL))
+			return; /* Found credential */
 }
 
 int main(int argc, char **argv)
@@ -123,6 +150,7 @@ int main(int argc, char **argv)
 	};
 	const char *op;
 	struct credential c = CREDENTIAL_INIT;
+	struct string_list fns = STRING_LIST_INIT_DUP;
 	char *file = NULL;
 	struct option options[] = {
 		OPT_STRING(0, "file", &file, "path",
@@ -139,20 +167,23 @@ int main(int argc, char **argv)
 
 	if (!file)
 		file = expand_user_path("~/.git-credentials");
-	if (!file)
+	if (file)
+		string_list_append(&fns, file);
+	else
 		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);
+		lookup_credential(&fns, &c);
 	else if (!strcmp(op, "erase"))
-		remove_credential(file, &c);
+		remove_credential(&fns, &c);
 	else if (!strcmp(op, "store"))
-		store_credential(file, &c);
+		store_credential(&fns, &c);
 	else
 		; /* Ignore unknown operation. */
 
+	string_list_clear(&fns, 0);
 	return 0;
 }
-- 
2.1.4

[PATCH v5 2/3] git-credential-store: support XDG_CONFIG_HOME

From: Paul Tan <hidden>
Date: 2016-06-15 23:04:16

Add $XDG_CONFIG_HOME/git/credentials to the default credential search
path of git-credential-store. This allows git-credential-store to
support user-specific configuration files in accordance with the XDG
base directory specification[1].

[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html

~/.git-credentials has a higher precedence than
$XDG_CONFIG_HOME/git/credentials when looking up credentials.  This
means that if any duplicate matching credentials are found in the xdg
file (due to ~/.git-credentials being updated by old versions of git or
outdated tools), they will not be used at all. This is to give the user
some leeway in switching to old versions of git while keeping the xdg
directory. This is consistent with the behavior of git-config.

However, the higher precedence of ~/.git-credentials means that as long
as ~/.git-credentials exist, all credentials will be written to the
~/.git-credentials file even if the user has an xdg file as having a
~/.git-credentials file indicates that the user wants to preserve
backwards-compatibility. This is also consistent with the behavior of
git-config.

To make this precedence explicit in docs/git-credential-store, add a new
section FILES that lists out the credential file paths in their order of
precedence, and explain how the ordering affects the lookup, storage and
erase operations.

Also, update the documentation for --file to briefly explain the
operations on multiple files if the --file option is not provided.

Since the xdg file will not be used unless it actually exists, to
prevent the situation where some credentials are present in the xdg file
while some are present in the home file, users are recommended to not
create the xdg file if they require compatibility with old versions of
git or outdated tools. Note, though, that "erase" can be used to
explicitly erase matching credentials from all files.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Jeff King [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Paul Tan <redacted>
---

This patch is a combination of 2 patches [1][2].

[1] http://thread.gmane.org/gmane.comp.version-control.git/265683/focus=265686

[2] http://thread.gmane.org/gmane.comp.version-control.git/265683/focus=265685

The changes are as follows:

* s/per default/by default/ in the documentation. Thanks Matthieu.

 Documentation/git-credential-store.txt | 35 ++++++++++++++++++++++++++++++++--
 credential-store.c                     | 13 +++++++++----
 2 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index bc97071..e3c8f27 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -31,10 +31,41 @@ OPTIONS
 
 --file=<path>::
 
-	Use `<path>` to store credentials. The file will have its
+	Use `<path>` to lookup and 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`.
+	protected. If not specified, credentials will be searched for from
+	`~/.git-credentials` and `$XDG_CONFIG_HOME/git/credentials`, and
+	credentials will be written to `~/.git-credentials` if it exists, or
+	`$XDG_CONFIG_HOME/git/credentials` if it exists and the former does
+	not. See also <<FILES>>.
+
+[[FILES]]
+FILES
+-----
+
+If not set explicitly with '--file', there are two files where
+git-credential-store will search for credentials in order of precedence:
+
+~/.git-credentials::
+	User-specific credentials file.
+
+$XDG_CONFIG_HOME/git/credentials::
+	Second user-specific credentials file. If '$XDG_CONFIG_HOME' is not set
+	or empty, `$HOME/.config/git/credentials` will be used. Any credentials
+	stored in this file will not be used if `~/.git-credentials` has a
+	matching credential as well. It is a good idea not to create this file
+	if you sometimes use older versions of Git that do not support it.
+
+For credential lookups, the files are read in the order given above, with the
+first matching credential found taking precedence over credentials found in
+files further down the list.
+
+Credential storage will by default write to the first existing file in the
+list. If none of these files exist, `~/.git-credentials` will be created and
+written to.
+
+When erasing credentials, matching credentials will be erased from all files.
 
 EXAMPLES
 --------
diff --git a/credential-store.c b/credential-store.c
index d700a93..8b22251 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -165,11 +165,16 @@ int main(int argc, char **argv)
 		usage_with_options(usage, options);
 	op = argv[0];
 
-	if (!file)
-		file = expand_user_path("~/.git-credentials");
-	if (file)
+	if (file) {
 		string_list_append(&fns, file);
-	else
+	} else {
+		if ((file = expand_user_path("~/.git-credentials")))
+			string_list_append_nodup(&fns, file);
+		home_config_paths(NULL, &file, "credentials");
+		if (file)
+			string_list_append_nodup(&fns, file);
+	}
+	if (!fns.nr)
 		die("unable to set up default path; use --file");
 
 	if (credential_read(&c, stdin) < 0)
-- 
2.1.4

[PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME

From: Paul Tan <hidden>
Date: 2016-06-15 23:04:16

t0302 now tests git-credential-store's support for the XDG user-specific
configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:

* Ensure that the XDG file is strictly opt-in. It should not be created
  by git at all times if it does not exist.

* Conversely, if the XDG file exists, ~/.git-credentials should
  not be created at all times.

* If both the XDG file and ~/.git-credentials exists, then both files
  should be used for credential lookups. However, credentials should
  only be written to ~/.git-credentials.

* Credentials must be erased from both files.

* $XDG_CONFIG_HOME can be a custom directory set by the user as per the
  XDG base directory specification. Test that git-credential-store
  respects that, but defaults to "~/.config/git/credentials" if it does
  not exist or is empty.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Paul Tan <redacted>
---

The previous version can be found at [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/265683/focus=265684

The changes are as follows:

* Fix broken && chains

* Rewrite test descriptions to be shorter

* Code formatting

* Fixed test "get: return credentials from home file if xdg files are
unreadable". It should actually be the reverse to follow the actual
precedence order.

Thanks Eric and Matthieu for the code review.

Matthieu and Eric: I know I said I will try to re-order the patches to
put the tests before the implementation, but after thinking and trying
to rewrite the commit messages I realised it seems really weird to me.
In this patch series, the implementation is split across the first two
patches. The first patch should use the old tests, and ideally, the new
tests should be squashed with the second patch because it seems more
logical to me to implement the tests at the same time as the new
feature. However, since the tests patch is very long, to make it easier
to review it is split into a separate patch which is applied after the
implementation patches.

 t/t0302-credential-store.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..4e1f8ec 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,4 +6,118 @@ test_description='credential-store tests'
 
 helper_test store
 
+test_expect_success 'when xdg file does not exist, xdg file not created' '
+	test_path_is_missing "$HOME/.config/git/credentials" &&
+	test -s "$HOME/.git-credentials"
+'
+
+test_expect_success 'setup xdg file' '
+	rm -f "$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	>"$HOME/.config/git/credentials"
+'
+
+helper_test store
+
+test_expect_success 'when xdg file exists, home file not created' '
+	test -s "$HOME/.config/git/credentials" &&
+	test_path_is_missing "$HOME/.git-credentials"
+'
+
+test_expect_success 'setup custom xdg file' '
+	rm -f "$HOME/.git-credentials" &&
+	rm -f "$HOME/.config/git/credentials" &&
+	mkdir -p "$HOME/xdg/git" &&
+	>"$HOME/xdg/git/credentials"
+'
+
+XDG_CONFIG_HOME="$HOME/xdg"
+export XDG_CONFIG_HOME
+helper_test store
+unset XDG_CONFIG_HOME
+
+test_expect_success 'if custom xdg file exists, home and xdg files not created' '
+	test_when_finished "rm -f $HOME/xdg/git/credentials" &&
+	test -s "$HOME/xdg/git/credentials" &&
+	test_path_is_missing "$HOME/.git-credentials" &&
+	test_path_is_missing "$HOME/.config/git/credentials"
+'
+
+test_expect_success 'get: use home file if both home and xdg files have matches' '
+	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=home-user
+	password=home-pass
+	--
+	EOF
+'
+
+test_expect_success 'get: use xdg file if home file has no matches' '
+	>"$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=xdg-user
+	password=xdg-pass
+	--
+	EOF
+'
+
+test_expect_success 'get: use xdg file if home file is unreadable' '
+	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+	chmod -r "$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=xdg-user
+	password=xdg-pass
+	--
+	EOF
+'
+
+test_expect_success 'store: if both xdg and home files exist, only store in home file' '
+	>"$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	>"$HOME/.config/git/credentials" &&
+	check approve store <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	echo "https://store-user:store-pass@example.com" >expected &&
+	test_cmp expected "$HOME/.git-credentials" &&
+	test_must_be_empty "$HOME/.config/git/credentials"
+'
+
+
+test_expect_success 'erase: erase matching credentials from both xdg and home files' '
+	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+	mkdir -p "$HOME/.config/git" &&
+	echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+	check reject store <<-\EOF &&
+	protocol=https
+	host=example.com
+	EOF
+	test_must_be_empty "$HOME/.git-credentials" &&
+	test_must_be_empty "$HOME/.config/git/credentials"
+'
+
 test_done
-- 
2.1.4

Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:04:17

On Tue, Mar 24, 2015 at 1:20 AM, Paul Tan [off-list ref] wrote:
quoted hunk
t0302 now tests git-credential-store's support for the XDG user-specific
configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:

* Ensure that the XDG file is strictly opt-in. It should not be created
  by git at all times if it does not exist.

* Conversely, if the XDG file exists, ~/.git-credentials should
  not be created at all times.

* If both the XDG file and ~/.git-credentials exists, then both files
  should be used for credential lookups. However, credentials should
  only be written to ~/.git-credentials.

* Credentials must be erased from both files.

* $XDG_CONFIG_HOME can be a custom directory set by the user as per the
  XDG base directory specification. Test that git-credential-store
  respects that, but defaults to "~/.config/git/credentials" if it does
  not exist or is empty.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..4e1f8ec 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,4 +6,118 @@ test_description='credential-store tests'

 helper_test store

+test_expect_success 'get: use xdg file if home file is unreadable' '
I meant to mention this earlier. Does this test need to be protected
by the POSIXPERM prerequisite since it's using chmod?

    test_expect_success POSIXPERM 'get: ... unreadable' '

Otherwise, the test will likely fail on Windows.
+       echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+       chmod -r "$HOME/.git-credentials" &&
+       mkdir -p "$HOME/.config/git" &&
+       echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+       check fill store <<-\EOF
+       protocol=https
+       host=example.com
+       --
+       protocol=https
+       host=example.com
+       username=xdg-user
+       password=xdg-pass
+       --
+       EOF
+'

Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME

From: Johannes Sixt <hidden>
Date: 2016-06-15 23:04:17

Am 25.03.2015 um 07:42 schrieb Eric Sunshine:
On Tue, Mar 24, 2015 at 1:20 AM, Paul Tan [off-list ref] wrote:
quoted
t0302 now tests git-credential-store's support for the XDG user-specific
configuration file $XDG_CONFIG_HOME/git/credentials. Specifically:

* Ensure that the XDG file is strictly opt-in. It should not be created
   by git at all times if it does not exist.

* Conversely, if the XDG file exists, ~/.git-credentials should
   not be created at all times.

* If both the XDG file and ~/.git-credentials exists, then both files
   should be used for credential lookups. However, credentials should
   only be written to ~/.git-credentials.

* Credentials must be erased from both files.

* $XDG_CONFIG_HOME can be a custom directory set by the user as per the
   XDG base directory specification. Test that git-credential-store
   respects that, but defaults to "~/.config/git/credentials" if it does
   not exist or is empty.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index f61b40c..4e1f8ec 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,4 +6,118 @@ test_description='credential-store tests'

  helper_test store

+test_expect_success 'get: use xdg file if home file is unreadable' '
I meant to mention this earlier. Does this test need to be protected
by the POSIXPERM prerequisite since it's using chmod?

     test_expect_success POSIXPERM 'get: ... unreadable' '

Otherwise, the test will likely fail on Windows.
Well spotted! The test indeed fails on Windows. POSIXPERM is required.
quoted
+       echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
+       chmod -r "$HOME/.git-credentials" &&
+       mkdir -p "$HOME/.config/git" &&
+       echo "https://xdg-user:xdg-pass@example.com" >"$HOME/.config/git/credentials" &&
+       check fill store <<-\EOF
+       protocol=https
+       host=example.com
+       --
+       protocol=https
+       host=example.com
+       username=xdg-user
+       password=xdg-pass
+       --
+       EOF
+'
-- Hannes
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help