[GSOC] Microproject "Move ~/.git-credential-cache to ~/.config/git"

I'm Hui Yiqun, a master degress candidate of Tsinghua University. I'd
like to participate GSOC 2016 as an developer of git.

I have basic knowledge about several programming languages (namely C,
python, go and etc.), compilers and cmake which may help my
development.

My github page is at [here](https://github.com/huiyiqun/). There are
some tiny programs.

I took part in a CDN project last year and learnt much about TCP/IP,
web service and, most importantly, cooperation. However, I really want
to learn how members of an opensource project work together.

I have covered most of the available materials, such as list of ideas
and micro-projects, `README.md`, `INSTALL` and
`Documentation/CodingGuidelines`. So I decide to start my contribution
with the microproject "Move ~/.git-credential-cache to ~/.config/git"
found [here](http://git.github.io/SoC-2016-Microprojects/), which
seems easy for me.

I greped the source code and found out that there are two places where
"git-credential-cache" are hard-coded:

1. credential-cache.c
2. contrib/persistent-https/socket.go

At first sight, there are following tasks to do:

1. implement a function `xdg_cache_home` similar to `xdg_config_home`
in `path.c`.
2. replace the hard-coded path with an call to `xdg_cache_home`.

I'm still confused about following:

1. should `~/.git-credential-cache` been moved to
`~/.cache/git/credential`(as the descreption of the micropject says)
or `~/.config/git/credential`(as the title of the microproject says)?
2. If `~/.cache/git/credential` is the desired target, there seems
nothing to do with `XDG_CONFIG_HOME`.
3. Does "without breaking compatibility with the old behavior." mean
that I should still try to connect to the unix socket placed at the
old place? If yes, which order is prefered?

Thanks for your patience. I hope that my English didn't affect the
communication.

this function does the following:

1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
step, otherwise `/tmp/git-$uid` is taken.
2. ensure that above directory does exist. what's more, it must has correct
permission and ownership.
3. a newly allocated string consisting of the path of above directory and
$filename is returned.

Under following situation, NULL will be returned:
+ the directory mentioned in step 1 exists but have wrong permission or
ownership.
+ the directory or its parent cannot be created.

Notice:

+ the caller is responsible for deallocating the returned string.

Signed-off-by: Hui Yiqun <redacted>
---
 cache.h | 23 +++++++++++++++++++++++
 path.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/cache.h b/cache.h
index b829410..e640a54 100644
--- a/cache.h
+++ b/cache.h

@@ -999,6 +999,29 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * this function does the following:
+ *
+ * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
+ * step, otherwise `/tmp/git-$uid` is taken.
+ * 2. ensure that above directory does exist. what's more, it must has correct
+ * permission and ownership.
+ * 3. a newly allocated string consisting of the path of above directory and
+ * $filename is returned.
+ *
+ * Under following situation, NULL will be returned:
+ *
+ * + the directory mentioned in step 1 exists but have wrong permission or
+ * ownership.
+ * + the directory or its parent cannot be created.
+ *
+ * Notice:
+ *
+ * + the caller is responsible for deallocating the returned string.
+ *
+ */
+extern char *xdg_runtime_dir(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2

diff --git a/path.c b/path.c
index 8b7e168..2deecb3 100644
--- a/path.c
+++ b/path.c

@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "dir.h"
+#include "git-compat-util.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {

@@ -1193,6 +1194,64 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+char *xdg_runtime_dir(const char *filename)
+{
+	char *runtime_dir, *git_runtime_dir;
+	struct stat st;
+	uid_t uid = getuid();
+
+	assert(filename);
+	runtime_dir = getenv("XDG_RUNTIME_DIR");
+	if (runtime_dir && *runtime_dir)
+		git_runtime_dir = mkpathdup("%s/git/", runtime_dir);
+	else
+		git_runtime_dir = mkpathdup("/tmp/git-%d", uid);
+
+	if (!lstat(git_runtime_dir, &st)) {
+		/*
+		 * As described in XDG base dir spec[1], the subdirectory
+		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+		 * the user, and its unix access mode MUST be 0700.
+		 *
+		 * Calling chmod or chown silently may cause security
+		 * problem if somebody chdir to it, sleep, and then, try
+		 * to open our protected runtime cache or socket.
+		 * So we just put warning and left it to user to solve.
+		 *
+		 * [1]https://specifications.freedesktop.org/basedir-spec/
+		 * basedir-spec-latest.html
+		 */
+		if ((st.st_mode & 0777) != S_IRWXU) {
+			fprintf(stderr,
+					"permission of runtime directory '%s' "
+					"MUST be 0700 instead of 0%o\n",
+					git_runtime_dir, (st.st_mode & 0777));
+			return NULL;
+		} else if (st.st_uid != uid) {
+			fprintf(stderr,
+					"owner of runtime directory '%s' "
+					"MUST be %d instead of %d\n",
+					git_runtime_dir, uid, st.st_uid);
+			return NULL;
+		}
+		/* TODO: check whether git_runtime_dir is an directory */
+	} else {
+		if (safe_create_leading_directories_const(git_runtime_dir) < 0) {
+			fprintf(stderr,
+					"unable to create directories for '%s'\n",
+					git_runtime_dir);
+			return NULL;
+		}
+		if (mkdir(git_runtime_dir, 0700) < 0) {
+			fprintf(stderr,
+					"unable to mkdir '%s'\n", git_runtime_dir);
+			return NULL;
+		}
+	}
+	free(git_runtime_dir);
+	return mkpathdup("%s/%s", git_runtime_dir, filename);
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")

-- 
2.7.2

move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")

Signed-off-by: Hui Yiqun <redacted>
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..40d838b 100644
--- a/credential-cache.c
+++ b/credential-cache.c

@@ -105,7 +105,7 @@ int main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = xdg_runtime_dir("credential-cache.sock");
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 

-- 
2.7.2

t0301 now tests git-credential-cache support for XDG user-specific
runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:

* if $XDG_RUNTIME_DIR exists, use socket at
  `$XDG_RUNTIME_DIR/git/credential-cache.sock`.

* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.

Signed-off-by: Hui Yiqun <redacted>
---
 t/t0301-credential-cache.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c8411..0718bb0 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh

@@ -12,7 +12,32 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+test_expect_success 'set $XDG_RUNTIME_DIR' '
+	XDG_RUNTIME_DIR=$HOME/xdg_runtime/
+'
+
+helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '
+	test_path_is_missing "/tmp/git-$(id -u)/git/credential-cache.sock" &&
+	test -S "$XDG_RUNTIME_DIR/git/credential-cache.sock"
+'
+
+test_expect_success 'force git-credential-cache to exit so that socket disappear' '
+	git credential-cache exit &&
+	test_path_is_missing "$XDG_RUNTIME_DIR/git/credential-cache.sock" &&
+	unset XDG_RUNTIME_DIR
+'
+
 helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is not set, `/tmp/git-$(id -u) is used' '
+	test -S "/tmp/git-$(id -u)/credential-cache.sock"
+'
+
+# TODO: if $XDG_RUNTIME_DIR/git/ exists, but has wrong permission and ownership,
+# `helper_test cache` must fail.
+
 helper_test_timeout cache --timeout=1
 
 # we can't rely on our "trap" above working after test_done,

-- 
2.7.2

On Wed, Mar 16, 2016 at 06:07:43PM +0800, Hui Yiqun wrote:

+	if (runtime_dir && *runtime_dir)
+		git_runtime_dir = mkpathdup("%s/git/", runtime_dir);
+	else
+		git_runtime_dir = mkpathdup("/tmp/git-%d", uid);

Here we allocate the string, but later we may return NULL on error,
leaking the allocated memory.

+	if (!lstat(git_runtime_dir, &st)) {
+		/*
+		 * As described in XDG base dir spec[1], the subdirectory
+		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+		 * the user, and its unix access mode MUST be 0700.
+		 *
+		 * Calling chmod or chown silently may cause security
+		 * problem if somebody chdir to it, sleep, and then, try
+		 * to open our protected runtime cache or socket.
+		 * So we just put warning and left it to user to solve.
+		 *
+		 * [1]https://specifications.freedesktop.org/basedir-spec/
+		 * basedir-spec-latest.html
+		 */

OK. I think these checks should be sufficient to deal with the /tmp race
I mentioned elsewhere in the thread (assuming that an attacker cannot
flip the uid back and forth in the same way, but that should be true on
Unix systems).

+		if ((st.st_mode & 0777) != S_IRWXU) {
+			fprintf(stderr,
+					"permission of runtime directory '%s' "
+					"MUST be 0700 instead of 0%o\n",
+					git_runtime_dir, (st.st_mode & 0777));
+			return NULL;
+		} else if (st.st_uid != uid) {
+			fprintf(stderr,
+					"owner of runtime directory '%s' "
+					"MUST be %d instead of %d\n",
+					git_runtime_dir, uid, st.st_uid);
+			return NULL;
+		}

Should these be using warning(), rather than a raw fprintf?

+	} else {
+		if (safe_create_leading_directories_const(git_runtime_dir) < 0) {
+			fprintf(stderr,
+					"unable to create directories for '%s'\n",
+					git_runtime_dir);
+			return NULL;
+		}
+		if (mkdir(git_runtime_dir, 0700) < 0) {
+			fprintf(stderr,
+					"unable to mkdir '%s'\n", git_runtime_dir);
+			return NULL;
+		}
+	}

And this retains the un-racy mkdir(). Good.

+	free(git_runtime_dir);
+	return mkpathdup("%s/%s", git_runtime_dir, filename);

This mkpathdup accesses the string we just freed?

It might be easier to just use a strbuf here, and then you can append to
it at the end.

-Peff

On Wed, Mar 16, 2016 at 06:07:45PM +0800, Hui Yiqun wrote:

Doesn't this need to export the variable so that credential-cache can
see it?

+
+helper_test cache
+

This runs the full suite of tests twice (once here, and once for the
original helper_test invocation you left below). Shouldn't we just do it
once (making sure that $XDG_RUNTIME_DIR is respected)?

+test_expect_success 'force git-credential-cache to exit so that socket disappear' '
+	git credential-cache exit &&
+	test_path_is_missing "$XDG_RUNTIME_DIR/git/credential-cache.sock" &&
+	unset XDG_RUNTIME_DIR
+'

I wondered if this might be racy. credential-cache tells the daemon
"exit", then waits for a response or EOF. The daemon sees "exit" and
calls exit(0) immediately. We clean up the socket in an atexit()
handler. So I think we are OK (the pipe will get closed when the process
exits, and the atexit handler must have run by then).

But that definitely was not designed, and is just how it happens to
work. I'm not sure if it's worth commenting on that (here, or perhaps in
the daemon code).

-Peff

2016-03-17 1:06 GMT+08:00 Jeff King [off-list ref]:

On Wed, Mar 16, 2016 at 06:07:43PM +0800, Hui Yiqun wrote:

quoted

+     if (runtime_dir && *runtime_dir)
+             git_runtime_dir = mkpathdup("%s/git/", runtime_dir);
+     else
+             git_runtime_dir = mkpathdup("/tmp/git-%d", uid);

Here we allocate the string, but later we may return NULL on error,
leaking the allocated memory.

Yes, do you think goto is a good solution for clearup?

quoted

+     if (!lstat(git_runtime_dir, &st)) {
+             /*
+              * As described in XDG base dir spec[1], the subdirectory
+              * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+              * the user, and its unix access mode MUST be 0700.
+              *
+              * Calling chmod or chown silently may cause security
+              * problem if somebody chdir to it, sleep, and then, try
+              * to open our protected runtime cache or socket.
+              * So we just put warning and left it to user to solve.
+              *
+              * [1]https://specifications.freedesktop.org/basedir-spec/
+              * basedir-spec-latest.html
+              */

OK. I think these checks should be sufficient to deal with the /tmp race
I mentioned elsewhere in the thread (assuming that an attacker cannot
flip the uid back and forth in the same way, but that should be true on
Unix systems).

quoted

+             if ((st.st_mode & 0777) != S_IRWXU) {
+                     fprintf(stderr,
+                                     "permission of runtime directory '%s' "
+                                     "MUST be 0700 instead of 0%o\n",
+                                     git_runtime_dir, (st.st_mode & 0777));
+                     return NULL;
+             } else if (st.st_uid != uid) {
+                     fprintf(stderr,
+                                     "owner of runtime directory '%s' "
+                                     "MUST be %d instead of %d\n",
+                                     git_runtime_dir, uid, st.st_uid);
+                     return NULL;
+             }

Should these be using warning(), rather than a raw fprintf?

Well, I will replace it.

During the greping. I found that I should also wrap my warning strings
with _() for i18n.

quoted

+     } else {
+             if (safe_create_leading_directories_const(git_runtime_dir) < 0) {
+                     fprintf(stderr,
+                                     "unable to create directories for '%s'\n",
+                                     git_runtime_dir);
+                     return NULL;
+             }
+             if (mkdir(git_runtime_dir, 0700) < 0) {
+                     fprintf(stderr,
+                                     "unable to mkdir '%s'\n", git_runtime_dir);
+                     return NULL;
+             }
+     }

And this retains the un-racy mkdir(). Good.

quoted

+     free(git_runtime_dir);
+     return mkpathdup("%s/%s", git_runtime_dir, filename);

This mkpathdup accesses the string we just freed?

It might be easier to just use a strbuf here, and then you can append to
it at the end.

I think so. Thanks.

-Peff

2016-03-16 18:07 GMT+08:00 Hui Yiqun [off-list ref]:

I'm sure but if user set up git-credential-cache with following command:

    git config --global credential.helper "cache --socket
~/.git-credential-cache/socket"

will the ~ be expanded?

2016-03-17 1:15 GMT+08:00 Jeff King [off-list ref]:

On Wed, Mar 16, 2016 at 06:07:45PM +0800, Hui Yiqun wrote:

quoted

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c8411..0718bb0 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh

@@ -12,7 +12,32 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT

+test_expect_success 'set $XDG_RUNTIME_DIR' '
+     XDG_RUNTIME_DIR=$HOME/xdg_runtime/
+'

Doesn't this need to export the variable so that credential-cache can
see it?

I'm not sure, but it seems that a little clean up code added before send-email
make the test fail. At that time, I run test without building. I've
send PATCH v2
which runs well on my computer. However, $XDG_RUNTIME_DIR is still not
exported, but that just works.

I will try to dig deeper into the bash script to see why.

quoted

+
+helper_test cache
+

This runs the full suite of tests twice (once here, and once for the
original helper_test invocation you left below). Shouldn't we just do it
once (making sure that $XDG_RUNTIME_DIR is respected)?

I'd like to test the behavior of git-credential-cache when
$XDG_RUNTIME_DIR is unset.

In `t/t0302-credential-store.sh`, helper_test is also run multiple
times. That's why I
do so.

quoted

+test_expect_success 'force git-credential-cache to exit so that socket disappear' '
+     git credential-cache exit &&
+     test_path_is_missing "$XDG_RUNTIME_DIR/git/credential-cache.sock" &&
+     unset XDG_RUNTIME_DIR
+'

I wondered if this might be racy. credential-cache tells the daemon
"exit", then waits for a response or EOF. The daemon sees "exit" and
calls exit(0) immediately. We clean up the socket in an atexit()
handler. So I think we are OK (the pipe will get closed when the process
exits, and the atexit handler must have run by then).

But that definitely was not designed, and is just how it happens to
work. I'm not sure if it's worth commenting on that (here, or perhaps in
the daemon code).

I'm still confused.

What do you mean by "pipe"? should it be "socket" instead?

What is not designed? cleanup being done, my tests passing or the
synchronization?

-Pef