# [PATCH 0/4] Allow Git repositories to be shared using POSIX ACLs 18 messages from 2010-02-19 to 2010-02-22. Participants: Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/22726 ## Matthieu Moy, 2010-02-19 16:33 Subject: [PATCH 0/4] Allow Git repositories to be shared using POSIX ACLs Message-ID: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266597207-32036-1-git-send-email-Matthieu.Moy%40imag.fr ``` I talked about the problem earlier: http://thread.gmane.org/gmane.comp.version-control.git/136720 In short: if a user wants to share a git repository using POSIX filesystem ACLs, it seems everything works except pack file creation. The problem is that we create the file with a mode 0600, i.e. no permission for group, but the semantics of ACL is that this group permission is used as ACL umask when the file has ACLs (don't ask me why, I never understood the rational, but that's how it works). Instead, if we let the umask do its job at the time of file creation, then the pack file is created with the correct permission bits and ACLs, and we don't need to chmod it after the fact. This is what this patch serie does. The test-case is really weak: ideally, we should have a real test with several users interacting. But that's hardly scriptable in a portable way, so the only testcase I added relies on getfacl and "ls -l" to find out if the result is correct. However, I did some manual testing, and as far as I can say, Git works very well with ACLs with this patch. ``` ## Matthieu Moy, 2010-02-19 16:33 Subject: [PATCH 1/4] Add a testcase for ACL with restrictive umask. Message-ID: <1266597207-32036-2-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266597207-32036-2-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` Right now, Git creates unreadable pack files on non-shared repositories when the user has a umask of 077, even when the default ACLs for the directory would give read/write access to a specific user. Signed-off-by: Matthieu Moy --- t/t1304-default-acl.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 49 insertions(+), 0 deletions(-) create mode 100755 t/t1304-default-acl.sh diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh new file mode 100755 index 0000000..4ee44a1 --- /dev/null +++ b/t/t1304-default-acl.sh @@ -0,0 +1,49 @@ +#!/bin/sh +# +# Copyright (c) 2010 Matthieu Moy +# + +test_description='Test repository with default ACL' + +. ./test-lib.sh + +# We need an arbitrary other user give permission to using ACLs. root +# is a good candidate: exists on all unices, and it has permission +# anyway, so we don't create a security hole running the testsuite. + +if ! setfacl -Rm u:root:rwx .; then + say "Skipping ACL tests: unable to use setfacl" + test_done +fi + +setfacl -Rm d:u:"$LOGNAME":rwx . +setfacl -Rm d:u:root:rwx . + +touch file.txt +git add file.txt +git commit -m "init" + +modebits () { + ls -l "$1" | sed -e 's|^\(..........\).*|\1|' +} + +test_expect_failure 'git gc does not break ACLs with restrictive umask' ' + umask 077 && + git gc && + actual=$(modebits .git/objects/pack/*.pack) && + case "$actual" in + -r--r-----*) + : happy + ;; + *) + false + ;; + esac && + getfacl .git/objects/pack/*.pack > actual && + grep -q "user:root:rwx" actual && + grep -q "user:${LOGNAME}:rwx" actual && + grep -q "mask::r--" actual && + grep -q "group::---" actual +' + +test_done -- 1.7.0.rc2.4.gc602c4 ``` ## Matthieu Moy, 2010-02-19 16:33 Subject: [PATCH 2/4] Move gitmkstemps to path.c Message-ID: <1266597207-32036-3-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266597207-32036-3-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` This function used to be only a compatibility function, but we're going to extend it and actually use it, so make it part of Git. Signed-off-by: Matthieu Moy --- Makefile | 1 - compat/mkstemps.c | 70 ----------------------------------------------------- path.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 71 deletions(-) delete mode 100644 compat/mkstemps.c diff --git a/Makefile b/Makefile index 7bf2fca..4387d42 100644 --- a/Makefile +++ b/Makefile @@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP endif ifdef NO_MKSTEMPS COMPAT_CFLAGS += -DNO_MKSTEMPS - COMPAT_OBJS += compat/mkstemps.o endif ifdef NO_UNSETENV COMPAT_CFLAGS += -DNO_UNSETENV diff --git a/compat/mkstemps.c b/compat/mkstemps.c deleted file mode 100644 index 14179c8..0000000 --- a/compat/mkstemps.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "../git-compat-util.h" - -/* Adapted from libiberty's mkstemp.c. */ - -#undef TMP_MAX -#define TMP_MAX 16384 - -int gitmkstemps(char *pattern, int suffix_len) -{ - static const char letters[] = - "abcdefghijklmnopqrstuvwxyz" - "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "0123456789"; - static const int num_letters = 62; - uint64_t value; - struct timeval tv; - char *template; - size_t len; - int fd, count; - - len = strlen(pattern); - - if (len < 6 + suffix_len) { - errno = EINVAL; - return -1; - } - - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { - errno = EINVAL; - return -1; - } - - /* - * Replace pattern's XXXXXX characters with randomness. - * Try TMP_MAX different filenames. - */ - gettimeofday(&tv, NULL); - value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); - template = &pattern[len - 6 - suffix_len]; - for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; - /* Fill in the random bits. */ - template[0] = letters[v % num_letters]; v /= num_letters; - template[1] = letters[v % num_letters]; v /= num_letters; - template[2] = letters[v % num_letters]; v /= num_letters; - template[3] = letters[v % num_letters]; v /= num_letters; - template[4] = letters[v % num_letters]; v /= num_letters; - template[5] = letters[v % num_letters]; v /= num_letters; - - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); - if (fd > 0) - return fd; - /* - * Fatal error (EPERM, ENOSPC etc). - * It doesn't make sense to loop. - */ - if (errno != EEXIST) - break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; - } - /* We return the null string if we can't find a unique file name. */ - pattern[0] = '\0'; - errno = EINVAL; - return -1; -} diff --git a/path.c b/path.c index d1fccbd..60f5b78 100644 --- a/path.c +++ b/path.c @@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) return mkstemps(path, suffix_len); } +/* Adapted from libiberty's mkstemp.c. */ + +#undef TMP_MAX +#define TMP_MAX 16384 + +int gitmkstemps(char *pattern, int suffix_len) +{ + static const char letters[] = + "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789"; + static const int num_letters = 62; + uint64_t value; + struct timeval tv; + char *template; + size_t len; + int fd, count; + + len = strlen(pattern); + + if (len < 6 + suffix_len) { + errno = EINVAL; + return -1; + } + + if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { + errno = EINVAL; + return -1; + } + + /* + * Replace pattern's XXXXXX characters with randomness. + * Try TMP_MAX different filenames. + */ + gettimeofday(&tv, NULL); + value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); + template = &pattern[len - 6 - suffix_len]; + for (count = 0; count < TMP_MAX; ++count) { + uint64_t v = value; + /* Fill in the random bits. */ + template[0] = letters[v % num_letters]; v /= num_letters; + template[1] = letters[v % num_letters]; v /= num_letters; + template[2] = letters[v % num_letters]; v /= num_letters; + template[3] = letters[v % num_letters]; v /= num_letters; + template[4] = letters[v % num_letters]; v /= num_letters; + template[5] = letters[v % num_letters]; v /= num_letters; + + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + if (fd > 0) + return fd; + /* + * Fatal error (EPERM, ENOSPC etc). + * It doesn't make sense to loop. + */ + if (errno != EEXIST) + break; + /* + * This is a random value. It is only necessary that + * the next TMP_MAX values generated by adding 7777 to + * VALUE are different with (module 2^32). + */ + value += 7777; + } + /* We return the null string if we can't find a unique file name. */ + pattern[0] = '\0'; + errno = EINVAL; + return -1; +} + int validate_headref(const char *path) { struct stat st; -- 1.7.0.rc2.4.gc602c4 ``` ## Matthieu Moy, 2010-02-19 16:33 Subject: [PATCH 3/4] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. Message-ID: <1266597207-32036-4-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266597207-32036-4-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` gitmkstemps emulates the behavior of mkstemps, which is usually used to create files in a shared directory like /tmp/, hence, it creates files with permission 0600. But we use it also to create pack files, which do not need this protection. Signed-off-by: Matthieu Moy --- cache.h | 4 ++++ path.c | 16 ++++++++++++++-- wrapper.c | 10 ++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cache.h b/cache.h index d478eff..0319637 100644 --- a/cache.h +++ b/cache.h @@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template); int git_mkstemps(char *path, size_t n, const char *template, int suffix_len); +/* set default permissions by passing mode arguments to open(2) */ +int git_mkstemps_mode(char *pattern, int suffix_len, int mode); +int git_mkstemp_mode(char *pattern, int mode); + /* * NOTE NOTE NOTE!! * diff --git a/path.c b/path.c index 60f5b78..005b836 100644 --- a/path.c +++ b/path.c @@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) #undef TMP_MAX #define TMP_MAX 16384 -int gitmkstemps(char *pattern, int suffix_len) +int git_mkstemps_mode(char *pattern, int suffix_len, int mode) { static const char letters[] = "abcdefghijklmnopqrstuvwxyz" @@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len) template[4] = letters[v % num_letters]; v /= num_letters; template[5] = letters[v % num_letters]; v /= num_letters; - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); if (fd > 0) return fd; /* @@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len) return -1; } +int git_mkstemp_mode(char *pattern, int mode) +{ + /* mkstemp is just mkstemps with no suffix */ + return git_mkstemps_mode(pattern, 0, mode); +} + +int gitmkstemps(char *pattern, int suffix_len) +{ + return git_mkstemps_mode(pattern, suffix_len, 0600); +} + int validate_headref(const char *path) { struct stat st; @@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p) } } } + diff --git a/wrapper.c b/wrapper.c index 0e3e20a..673762f 100644 --- a/wrapper.c +++ b/wrapper.c @@ -204,6 +204,16 @@ int xmkstemp(char *template) return fd; } +int xmkstemp_mode(char *template, int mode) +{ + int fd; + + fd = git_mkstemp_mode(template, mode); + if (fd < 0) + die_errno("Unable to create temporary file"); + return fd; +} + /* * zlib wrappers to make sure we don't silently miss errors * at init time. -- 1.7.0.rc2.4.gc602c4 ``` ## Matthieu Moy, 2010-02-19 16:33 Subject: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: <1266597207-32036-5-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266597207-32036-5-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` We used to create 0600 files, and then use chmod to set the group and other permission bits to the umask. This usually has the same effect as a normal file creation with a umask. But in the presence of ACLs, the group permission plays the role of the ACL mask: the "g" bits of newly created files are chosen according to default ACL mask of the directory, not according to the umask, and doing a chmod() on these "g" bits affect the ACL's mask instead of actual group permission. In other words, creating files with 0600 and then doing a chmod to the umask creates files which are unreadable by users allowed in the default ACL. To create the files without breaking ACLs, we let the umask do it's job at the file's creation time, and get rid of the later chmod. Signed-off-by: Matthieu Moy --- builtin-pack-objects.c | 18 ++---------------- t/t1304-default-acl.sh | 2 +- wrapper.c | 10 +++++++--- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index e1d3adf..539e75d 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -464,9 +464,6 @@ static int write_one(struct sha1file *f, return 1; } -/* forward declaration for write_pack_file */ -static int adjust_perm(const char *path, mode_t mode); - static void write_pack_file(void) { uint32_t i = 0, j; @@ -523,21 +520,17 @@ static void write_pack_file(void) } if (!pack_to_stdout) { - mode_t mode = umask(0); struct stat st; const char *idx_tmp_name; char tmpname[PATH_MAX]; - umask(mode); - mode = 0444 & ~mode; - idx_tmp_name = write_idx_file(NULL, written_list, nr_written, sha1); snprintf(tmpname, sizeof(tmpname), "%s-%s.pack", base_name, sha1_to_hex(sha1)); free_pack_by_name(tmpname); - if (adjust_perm(pack_tmp_name, mode)) + if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); if (rename(pack_tmp_name, tmpname)) die_errno("unable to rename temporary pack file"); @@ -565,7 +558,7 @@ static void write_pack_file(void) snprintf(tmpname, sizeof(tmpname), "%s-%s.idx", base_name, sha1_to_hex(sha1)); - if (adjust_perm(idx_tmp_name, mode)) + if (adjust_shared_perm(idx_tmp_name)) die_errno("unable to make temporary index file readable"); if (rename(idx_tmp_name, tmpname)) die_errno("unable to rename temporary index file"); @@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av) loosen_unused_packed_objects(&revs); } -static int adjust_perm(const char *path, mode_t mode) -{ - if (chmod(path, mode)) - return -1; - return adjust_shared_perm(path); -} - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 4ee44a1..b8e9c34 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -27,7 +27,7 @@ modebits () { ls -l "$1" | sed -e 's|^\(..........\).*|\1|' } -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' +test_expect_success 'git gc does not break ACLs with restrictive umask' ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && diff --git a/wrapper.c b/wrapper.c index 673762f..b2c784d 100644 --- a/wrapper.c +++ b/wrapper.c @@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush) int odb_mkstemp(char *template, size_t limit, const char *pattern) { int fd; - + /* + * we let the umask do it's job, don't try to be more + * restrictive. + */ + int mode = 0666; snprintf(template, limit, "%s/%s", get_object_directory(), pattern); - fd = mkstemp(template); + fd = git_mkstemp_mode(template, mode); if (0 <= fd) return fd; @@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern) snprintf(template, limit, "%s/%s", get_object_directory(), pattern); safe_create_leading_directories(template); - return xmkstemp(template); + return xmkstemp_mode(template, mode); } int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) -- 1.7.0.rc2.4.gc602c4 ``` ## Junio C Hamano, 2010-02-19 17:52 Subject: Re: [PATCH 0/4] Allow Git repositories to be shared using POSIX ACLs Message-ID: <7vsk8xjep1.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsk8xjep1.fsf%40alter.siamese.dyndns.org In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` Matthieu Moy writes: > In short: if a user wants to share a git repository using POSIX > filesystem ACLs, it seems everything works except pack file creation. > The problem is that we create the file with a mode 0600, i.e. no > permission for group,... Modulo that it is more like "we let mkstemp() to choose whatever file mode", this is a good analysis. I think some versions of glibc used to have mkstemp() that creates 0644 or 0664, meaning that the modes left by various implementations of mkstemp() are different from each other, and also different from what we want. With aef5aed (pack-objects: quickfix for permission modes., 2007-04-22) and b6b32cc (Fix 'quickfix' on pack-objects., 2007-04-22), were attempts to work around that issue. By not using mkstemp() from the platform but having our own would allow us to not even worry about this issue (this can be seen by the removal of a call to umask() in [PATCH 4/4]). I see your patches as a "bite-the-bullet and do the right thing" solution. > The test-case is really weak: ideally, we should have a real test with > several users interacting. But that's hardly scriptable in a portable > way, so the only testcase I added relies on getfacl and "ls -l" to find > out if the result is correct. However, I did some manual testing, and > as far as I can say, Git works very well with ACLs with this patch. Thanks. ``` ## Junio C Hamano, 2010-02-20 20:01 Subject: Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: <7vy6inekxq.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6inekxq.fsf%40alter.siamese.dyndns.org In-Reply-To: <1266597207-32036-5-git-send-email-Matthieu.Moy@imag.fr> ``` Matthieu Moy writes: > diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh > index 4ee44a1..b8e9c34 100755 > --- a/t/t1304-default-acl.sh > +++ b/t/t1304-default-acl.sh > @@ -27,7 +27,7 @@ modebits () { > ls -l "$1" | sed -e 's|^\(..........\).*|\1|' > } > > -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' > +test_expect_success 'git gc does not break ACLs with restrictive umask' ' > umask 077 && > git gc && > actual=$(modebits .git/objects/pack/*.pack) && This does not seem to pass for me; I get 0444 instead of 0440 as you expect. ``` ## Matthieu Moy, 2010-02-22 07:55 Subject: Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: URL: https://gitlist.dev/e/vpqfx4tln64.fsf%40bauges.imag.fr In-Reply-To: <7vy6inekxq.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Matthieu Moy writes: > >> diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh >> index 4ee44a1..b8e9c34 100755 >> --- a/t/t1304-default-acl.sh >> +++ b/t/t1304-default-acl.sh >> @@ -27,7 +27,7 @@ modebits () { >> ls -l "$1" | sed -e 's|^\(..........\).*|\1|' >> } >> >> -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' >> +test_expect_success 'git gc does not break ACLs with restrictive umask' ' >> umask 077 && >> git gc && >> actual=$(modebits .git/objects/pack/*.pack) && > > This does not seem to pass for me; I get 0444 instead of 0440 as you expect. That is strange, it does pass (or it's skipped if I don't have setfacl) for me. What system are you using? Do you have ACLs enabled on the filesystem where you run the tests? What permission do you get for packs with Git-without-my-patch (I used to get 0400)? Thanks, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2010-02-22 20:33 Subject: Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: <7veikdau4i.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veikdau4i.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Matthieu Moy writes: > That is strange, it does pass (or it's skipped if I don't have > setfacl) for me. > > What system are you using? Do you have ACLs enabled on the filesystem > where you run the tests? What permission do you get for packs with > Git-without-my-patch (I used to get 0400)? On my primary box (happens to be Debian 5 but that does not matter) I do not even have setfacl and the tests are properly skipped. On another box with FC11 (git.git directory is on an ext4 partition), here is what "sh -x t1304-default.acl.sh -i -v" gives me when run in 'pu'. + test_description='Test repository with default ACL' + . ./test-lib.sh ++ case "$GIT_TEST_TEE_STARTED, $* " in ++ ORIGINAL_TERM=screen ++ LANG=C ++ LC_ALL=C ++ PAGER=cat ++ TZ=UTC ++ TERM=dumb ++ export LANG LC_ALL PAGER TERM TZ ++ EDITOR=: ++ unset VISUAL ++ unset GIT_EDITOR ++ unset AUTHOR_DATE ++ unset AUTHOR_EMAIL ++ unset AUTHOR_NAME ++ unset COMMIT_AUTHOR_EMAIL ++ unset COMMIT_AUTHOR_NAME ++ unset EMAIL ++ unset GIT_ALTERNATE_OBJECT_DIRECTORIES ++ unset GIT_AUTHOR_DATE ++ GIT_AUTHOR_EMAIL=author@example.com ++ GIT_AUTHOR_NAME='A U Thor' ++ unset GIT_COMMITTER_DATE ++ GIT_COMMITTER_EMAIL=committer@example.com ++ GIT_COMMITTER_NAME='C O Mitter' ++ unset GIT_DIFF_OPTS ++ unset GIT_DIR ++ unset GIT_WORK_TREE ++ unset GIT_EXTERNAL_DIFF ++ unset GIT_INDEX_FILE ++ unset GIT_OBJECT_DIRECTORY ++ unset GIT_CEILING_DIRECTORIES ++ unset SHA1_FILE_DIRECTORIES ++ unset SHA1_FILE_DIRECTORY ++ GIT_MERGE_VERBOSITY=5 ++ export GIT_MERGE_VERBOSITY ++ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME ++ export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME ++ export EDITOR ++ GIT_TEST_CMP='diff -u' ++ unset CDPATH ++ case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in +++ echo +++ tr '[A-Z]' '[a-z]' ++ _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' ++ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' ++ '[' xscreen '!=' xdumb ']' ++ TERM=screen ++ export TERM ++ '[' -t 1 ']' ++ test 2 -ne 0 ++ case "$1" in ++ immediate=t ++ shift ++ test 1 -ne 0 ++ case "$1" in ++ verbose=t ++ shift ++ test 0 -ne 0 ++ test -n '' ++ test 'Test repository with default ACL' '!=' '' ++ test '' = t ++ exec ++ test t = t ++ exec ++ test_failure=0 ++ test_count=0 ++ test_fixed=0 ++ test_broken=0 ++ test_success=0 ++ GIT_EXIT_OK= ++ trap die EXIT ++ satisfied=' ' +++ pwd ++ TEST_DIRECTORY=/scratch/buildfarm/pu/t ++ test -n '' ++ test -n '' ++ git_bin_dir=/scratch/buildfarm/pu/t/../bin-wrappers ++ test -x /scratch/buildfarm/pu/t/../bin-wrappers/git ++ PATH=/scratch/buildfarm/pu/t/../bin-wrappers:/home/junio/g/Fedora-11-i686/git-active/bin:/home/junio/bin/common:/sbin:/usr/sbin:/usr/kerberos/bin:/usr/bin:/bin:/usr/local/sbin ++ GIT_EXEC_PATH=/scratch/buildfarm/pu/t/.. ++ test -n '' +++ pwd ++ GIT_TEMPLATE_DIR=/scratch/buildfarm/pu/t/../templates/blt ++ unset GIT_CONFIG ++ GIT_CONFIG_NOSYSTEM=1 ++ GIT_CONFIG_NOGLOBAL=1 ++ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL ++ . ../GIT-BUILD-OPTIONS +++ SHELL_PATH=/bin/sh +++ PERL_PATH=/usr/bin/perl +++ TAR=tar +++ NO_CURL= +++ NO_PERL= +++ NO_PYTHON= +++ pwd +++ pwd ++ GITPERLLIB=/scratch/buildfarm/pu/t/../perl/blib/lib:/scratch/buildfarm/pu/t/../perl/blib/arch/auto/Git ++ export GITPERLLIB ++ test -d ../templates/blt ++ test -z '' ++ test -z '' +++ pwd ++ GITPYTHONLIB=/scratch/buildfarm/pu/t/../git_remote_helpers/build/lib ++ export GITPYTHONLIB ++ test -d ../git_remote_helpers/build ++ test -x ../test-chmtime +++ basename t1304-default-acl.sh .sh ++ test='trash directory.t1304-default-acl' ++ test -n '' ++ case "$test" in ++ TRASH_DIRECTORY='/scratch/buildfarm/pu/t/trash directory.t1304-default-acl' ++ test '!' -z '' ++ remove_trash='/scratch/buildfarm/pu/t/trash directory.t1304-default-acl' ++ rm -fr 'trash directory.t1304-default-acl' ++ test_create_repo 'trash directory.t1304-default-acl' ++ test 1 = 1 +++ pwd ++ owd=/scratch/buildfarm/pu/t ++ repo='trash directory.t1304-default-acl' ++ mkdir -p 'trash directory.t1304-default-acl' ++ cd 'trash directory.t1304-default-acl' ++ /scratch/buildfarm/pu/t/../git-init --template=/scratch/buildfarm/pu/t/../templates/blt/ Initialized empty Git repository in /scratch/buildfarm/pu/t/trash directory.t1304-default-acl/.git/ ++ mv .git/hooks .git/hooks-disabled ++ cd /scratch/buildfarm/pu/t ++ cd -P 'trash directory.t1304-default-acl' ++ this_test=t1304-default-acl.sh ++ this_test=t1304 ++ case $(uname -s) in +++ uname -s ++ test_set_prereq POSIXPERM ++ satisfied=' POSIXPERM ' ++ test_set_prereq BSLASHPSPEC ++ satisfied=' POSIXPERM BSLASHPSPEC ' ++ test_set_prereq EXECKEEPSPID ++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID ' ++ test -z '' ++ test_set_prereq PERL ++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL ' ++ test -z '' ++ test_set_prereq PYTHON ++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON ' ++ ln -s x y ++ test -h y ++ test_set_prereq SYMLINKS ++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON SYMLINKS ' ++ rm -f y + setfacl -Rm u:root:rwx . + setfacl -Rm d:u:junio:rwx . + setfacl -Rm d:u:root:rwx . + touch file.txt + git add file.txt + git commit -m init [master (root-commit) 956f7d1] init Author: A U Thor 0 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 file.txt + test_expect_success 'git gc does not break ACLs with restrictive umask' ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + test 2 = 3 + prereq= + test 2 = 2 + test_skip 'git gc does not break ACLs with restrictive umask' ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + test_count=1 + to_skip= + test -z '' + test -n '' + case "$to_skip" in + false + say 'expecting success: umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + say_color info 'expecting success: umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + test -z info + shift + echo '* expecting success: umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' * expecting success: umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual + test_run_ ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + eval ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' ++ umask 077 ++ git gc +++ modebits .git/objects/pack/pack-9a3c3444a7e8660480c6ea70aa526e8299f7cfd6.pack +++ ls -l .git/objects/pack/pack-9a3c3444a7e8660480c6ea70aa526e8299f7cfd6.pack +++ sed -e 's|^\(..........\).*|\1|' ++ actual=-r--r--r-- ++ case "$actual" in ++ echo 'expected 440, got -r--r--r--' expected 440, got -r--r--r-- ++ false + eval_ret=1 + return 0 + '[' 0 = 0 -a 1 = 0 ']' + test_failure_ 'git gc does not break ACLs with restrictive umask' ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + test_failure=1 + say_color error 'FAIL 1: git gc does not break ACLs with restrictive umask' + test -z error + shift + echo '* FAIL 1: git gc does not break ACLs with restrictive umask' * FAIL 1: git gc does not break ACLs with restrictive umask + shift + echo ' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual ' + sed -e 's/^/ /' umask 077 && git gc && actual=$(modebits .git/objects/pack/*.pack) && case "$actual" in -r--r-----*) : happy ;; *) echo "expected 440, got $actual"; false ;; esac && getfacl .git/objects/pack/*.pack > actual && grep -q "user:root:rwx" actual && grep -q "user:${LOGNAME}:rwx" actual && grep -q "mask::r--" actual && grep -q "group::---" actual + test t = '' + GIT_EXIT_OK=t + exit 1 + die + code=1 + test -n t + exit 1 ``` ## Junio C Hamano, 2010-02-22 20:36 Subject: Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: <7vsk8t9feq.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsk8t9feq.fsf%40alter.siamese.dyndns.org In-Reply-To: <7veikdau4i.fsf@alter.siamese.dyndns.org> ``` By the way, here is what the same command gets when run on FreeBSD 8.0 + test_description='Test repository with default ACL' + . ./test-lib.sh + ORIGINAL_TERM=screen + LANG=C + LC_ALL=C + PAGER=cat + TZ=UTC + TERM=dumb + export LANG LC_ALL PAGER TERM TZ + EDITOR=: + unset VISUAL + unset GIT_EDITOR + unset AUTHOR_DATE + unset AUTHOR_EMAIL + unset AUTHOR_NAME + unset COMMIT_AUTHOR_EMAIL + unset COMMIT_AUTHOR_NAME + unset EMAIL + unset GIT_ALTERNATE_OBJECT_DIRECTORIES + unset GIT_AUTHOR_DATE + GIT_AUTHOR_EMAIL=author@example.com + GIT_AUTHOR_NAME='A U Thor' + unset GIT_COMMITTER_DATE + GIT_COMMITTER_EMAIL=committer@example.com + GIT_COMMITTER_NAME='C O Mitter' + unset GIT_DIFF_OPTS + unset GIT_DIR + unset GIT_WORK_TREE + unset GIT_EXTERNAL_DIFF + unset GIT_INDEX_FILE + unset GIT_OBJECT_DIRECTORY + unset GIT_CEILING_DIRECTORIES + unset SHA1_FILE_DIRECTORIES + unset SHA1_FILE_DIRECTORY + GIT_MERGE_VERBOSITY=5 + export GIT_MERGE_VERBOSITY + export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME + export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME + export EDITOR + GIT_TEST_CMP='diff -u' + unset CDPATH + echo + tr '[A-Z]' '[a-z]' + _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' + _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' + [ xscreen != xdumb ] + TERM=screen + export TERM + [ -t 1 ] + test 2 -ne 0 + immediate=t + shift + test 1 -ne 0 + verbose=t + shift + test 0 -ne 0 + test -n '' + test 'Test repository with default ACL' != '' + test '' = t + exec + test t = t + exec + test_failure=0 + test_count=0 + test_fixed=0 + test_broken=0 + test_success=0 + GIT_EXIT_OK='' + trap die EXIT + satisfied=' ' + pwd + TEST_DIRECTORY=/usr/home/junio/buildfarm/pu/t + test -n '' + test -n '' + git_bin_dir=/usr/home/junio/buildfarm/pu/t/../bin-wrappers + test -x /usr/home/junio/buildfarm/pu/t/../bin-wrappers/git + PATH=/usr/home/junio/buildfarm/pu/t/../bin-wrappers:/home/junio/g/FreeBSD-8.0-RELEASE-i386/git-active/bin:/home/junio/bin/common:/usr/local/bin:/sbin:/usr/sbin:/usr/bin:/bin + GIT_EXEC_PATH=/usr/home/junio/buildfarm/pu/t/.. + test -n '' + pwd + GIT_TEMPLATE_DIR=/usr/home/junio/buildfarm/pu/t/../templates/blt + unset GIT_CONFIG + GIT_CONFIG_NOSYSTEM=1 + GIT_CONFIG_NOGLOBAL=1 + export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL + . ../GIT-BUILD-OPTIONS + SHELL_PATH=/bin/sh + PERL_PATH=/usr/bin/perl + TAR=tar + NO_CURL='' + NO_PERL='' + NO_PYTHON='' + pwd + pwd + GITPERLLIB=/usr/home/junio/buildfarm/pu/t/../perl/blib/lib:/usr/home/junio/buildfarm/pu/t/../perl/blib/arch/auto/Git + export GITPERLLIB + test -d ../templates/blt + test -z '' + test -z '' + pwd + GITPYTHONLIB=/usr/home/junio/buildfarm/pu/t/../git_remote_helpers/build/lib + export GITPYTHONLIB + test -d ../git_remote_helpers/build + test -x ../test-chmtime + basename t1304-default-acl.sh .sh + test='trash directory.t1304-default-acl' + test -n '' + TRASH_DIRECTORY='/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl' + test ! -z '' + remove_trash='/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl' + rm -fr 'trash directory.t1304-default-acl' + test_create_repo 'trash directory.t1304-default-acl' + test 1 = 1 + pwd + owd=/usr/home/junio/buildfarm/pu/t + repo='trash directory.t1304-default-acl' + mkdir -p 'trash directory.t1304-default-acl' + cd 'trash directory.t1304-default-acl' + /usr/home/junio/buildfarm/pu/t/../git-init --template=/usr/home/junio/buildfarm/pu/t/../templates/blt/ Initialized empty Git repository in /usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl/.git/ + mv .git/hooks .git/hooks-disabled + cd /usr/home/junio/buildfarm/pu/t + cd -P 'trash directory.t1304-default-acl' + this_test=t1304-default-acl.sh + this_test=t1304 + uname -s + test_set_prereq POSIXPERM + satisfied=' POSIXPERM ' + test_set_prereq BSLASHPSPEC + satisfied=' POSIXPERM BSLASHPSPEC ' + test_set_prereq EXECKEEPSPID + satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID ' + test -z '' + test_set_prereq PERL + satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL ' + test -z '' + test_set_prereq PYTHON + satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON ' + ln -s x y + test -h y + test_set_prereq SYMLINKS + satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON SYMLINKS ' + rm -f y + setfacl -Rm u:root:rwx . setfacl: illegal option -- R usage: setfacl [-bdhkn] [-m entries] [-M file] [-x entries] [-X file] [file ...] + say 'Skipping ACL tests: unable to use setfacl' + say_color info 'Skipping ACL tests: unable to use setfacl' + test -z info + shift + echo '* Skipping ACL tests: unable to use setfacl' * Skipping ACL tests: unable to use setfacl + test_done + GIT_EXIT_OK=t + test_results_dir=/usr/home/junio/buildfarm/pu/t/test-results + mkdir -p /usr/home/junio/buildfarm/pu/t/test-results + test_results_path=/usr/home/junio/buildfarm/pu/t/test-results/t1304-default-acl-56364 + echo 'total 0' + echo 'success 0' + echo 'fixed 0' + echo 'broken 0' + echo 'failed 0' + echo '' + test 0 != 0 + test 0 != 0 + msg='0 test(s)' + say_color pass 'passed all 0 test(s)' + test -z pass + shift + echo '* passed all 0 test(s)' * passed all 0 test(s) + test -d '/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl' + dirname '/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl' + cd /usr/home/junio/buildfarm/pu/t + basename '/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl' + rm -rf 'trash directory.t1304-default-acl' + exit 0 + die + code=0 + test -n t + exit 0 ``` ## Matthieu Moy, 2010-02-22 22:11 Subject: Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: URL: https://gitlist.dev/e/vpqaav0or98.fsf%40bauges.imag.fr In-Reply-To: <7veikdau4i.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > On another box with FC11 (git.git directory is on an ext4 partition), here > is what "sh -x t1304-default.acl.sh -i -v" gives me when run in > 'pu'. Actually, it wasn't such a subtle configuration. You have umask=022 on your machine, and I had umask=077 on the one I did the test. My test script was setting umask after . test-lib.sh, it should have done it before (so that the created repository is created with restrictive umask right away). While I was there, I also added a testcase for object files. > By the way, here is what the same command gets when run on FreeBSD 8.0 > [...] > + setfacl -Rm u:root:rwx . > setfacl: illegal option -- R Thanks. I fixed the script not to use -R. But that's without any warranty, I don't have a FreeBSD machine for testing ... New serie comming soon, which should adress all your comments. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 0/6 v3] Allow Git repositories to be shared using POSIX ACLs Message-ID: <1266877936-13537-1-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-1-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` This version should include fixes for all of Junio's comment: [PATCH 1/6] Add a testcase for ACL with restrictive umask. => This should pass with any umask, and includes a fix for FreeBSD (but I'm not sure it's sufficient) [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. => better log message suggested by Junio. [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL on exit. => I actually got rid of this return EINVAL. ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 1/6] Add a testcase for ACL with restrictive umask. Message-ID: <1266877936-13537-2-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-2-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` Right now, Git creates unreadable pack files on non-shared repositories when the user has a umask of 077, even when the default ACLs for the directory would give read/write access to a specific user. Loose object files are created world-readable, which doesn't break ACLs, but isn't necessarily desirable. Signed-off-by: Matthieu Moy --- t/t1304-default-acl.sh | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 67 insertions(+), 0 deletions(-) create mode 100755 t/t1304-default-acl.sh diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh new file mode 100755 index 0000000..07dd6af --- /dev/null +++ b/t/t1304-default-acl.sh @@ -0,0 +1,67 @@ +#!/bin/sh +# +# Copyright (c) 2010 Matthieu Moy +# + +test_description='Test repository with default ACL' + +# Create the test repo with restrictive umask +# => this must come before . ./test-lib.sh +umask 077 + +. ./test-lib.sh + +# We need an arbitrary other user give permission to using ACLs. root +# is a good candidate: exists on all unices, and it has permission +# anyway, so we don't create a security hole running the testsuite. + +if ! setfacl -m u:root:rwx .; then + say "Skipping ACL tests: unable to use setfacl" + test_done +fi + +modebits () { + ls -l "$1" | sed -e 's|^\(..........\).*|\1|' +} + +check_perms_and_acl () { + actual=$(modebits "$1") && + case "$actual" in + -r--r-----*) + : happy + ;; + *) + echo "Got permission '$actual', expected '-r--r-----'" + false + ;; + esac && + getfacl "$1" > actual && + grep -q "user:root:rwx" actual && + grep -q "user:${LOGNAME}:rwx" actual && + grep -q "mask::r--" actual && + grep -q "group::---" actual || false +} + +dirs_to_set="./ .git/ .git/objects/ .git/objects/pack/" + +test_expect_success 'Setup test repo' ' + setfacl -m u:root:rwx $dirs_to_set && + setfacl -d -m u:"$LOGNAME":rwx $dirs_to_set && + setfacl -d -m u:root:rwx $dirs_to_set && + + touch file.txt && + git add file.txt && + git commit -m "init" +' + +test_expect_failure 'Objects creation does not break ACLs with restrictive umask' ' + # SHA1 for empty blob + check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 +' + +test_expect_failure 'git gc does not break ACLs with restrictive umask' ' + git gc && + check_perms_and_acl .git/objects/pack/*.pack +' + +test_done -- 1.7.0.54.gb6a04.dirty ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 2/6] Move gitmkstemps to path.c Message-ID: <1266877936-13537-3-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-3-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` This function used to be only a compatibility function, but we're going to extend it and actually use it, so make it part of Git. Signed-off-by: Matthieu Moy --- Makefile | 1 - compat/mkstemps.c | 70 ----------------------------------------------------- path.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 71 deletions(-) delete mode 100644 compat/mkstemps.c diff --git a/Makefile b/Makefile index 7bf2fca..4387d42 100644 --- a/Makefile +++ b/Makefile @@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP endif ifdef NO_MKSTEMPS COMPAT_CFLAGS += -DNO_MKSTEMPS - COMPAT_OBJS += compat/mkstemps.o endif ifdef NO_UNSETENV COMPAT_CFLAGS += -DNO_UNSETENV diff --git a/compat/mkstemps.c b/compat/mkstemps.c deleted file mode 100644 index 14179c8..0000000 --- a/compat/mkstemps.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "../git-compat-util.h" - -/* Adapted from libiberty's mkstemp.c. */ - -#undef TMP_MAX -#define TMP_MAX 16384 - -int gitmkstemps(char *pattern, int suffix_len) -{ - static const char letters[] = - "abcdefghijklmnopqrstuvwxyz" - "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "0123456789"; - static const int num_letters = 62; - uint64_t value; - struct timeval tv; - char *template; - size_t len; - int fd, count; - - len = strlen(pattern); - - if (len < 6 + suffix_len) { - errno = EINVAL; - return -1; - } - - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { - errno = EINVAL; - return -1; - } - - /* - * Replace pattern's XXXXXX characters with randomness. - * Try TMP_MAX different filenames. - */ - gettimeofday(&tv, NULL); - value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); - template = &pattern[len - 6 - suffix_len]; - for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; - /* Fill in the random bits. */ - template[0] = letters[v % num_letters]; v /= num_letters; - template[1] = letters[v % num_letters]; v /= num_letters; - template[2] = letters[v % num_letters]; v /= num_letters; - template[3] = letters[v % num_letters]; v /= num_letters; - template[4] = letters[v % num_letters]; v /= num_letters; - template[5] = letters[v % num_letters]; v /= num_letters; - - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); - if (fd > 0) - return fd; - /* - * Fatal error (EPERM, ENOSPC etc). - * It doesn't make sense to loop. - */ - if (errno != EEXIST) - break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; - } - /* We return the null string if we can't find a unique file name. */ - pattern[0] = '\0'; - errno = EINVAL; - return -1; -} diff --git a/path.c b/path.c index 79aa104..ab2e368 100644 --- a/path.c +++ b/path.c @@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) return mkstemps(path, suffix_len); } +/* Adapted from libiberty's mkstemp.c. */ + +#undef TMP_MAX +#define TMP_MAX 16384 + +int gitmkstemps(char *pattern, int suffix_len) +{ + static const char letters[] = + "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789"; + static const int num_letters = 62; + uint64_t value; + struct timeval tv; + char *template; + size_t len; + int fd, count; + + len = strlen(pattern); + + if (len < 6 + suffix_len) { + errno = EINVAL; + return -1; + } + + if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { + errno = EINVAL; + return -1; + } + + /* + * Replace pattern's XXXXXX characters with randomness. + * Try TMP_MAX different filenames. + */ + gettimeofday(&tv, NULL); + value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); + template = &pattern[len - 6 - suffix_len]; + for (count = 0; count < TMP_MAX; ++count) { + uint64_t v = value; + /* Fill in the random bits. */ + template[0] = letters[v % num_letters]; v /= num_letters; + template[1] = letters[v % num_letters]; v /= num_letters; + template[2] = letters[v % num_letters]; v /= num_letters; + template[3] = letters[v % num_letters]; v /= num_letters; + template[4] = letters[v % num_letters]; v /= num_letters; + template[5] = letters[v % num_letters]; v /= num_letters; + + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + if (fd > 0) + return fd; + /* + * Fatal error (EPERM, ENOSPC etc). + * It doesn't make sense to loop. + */ + if (errno != EEXIST) + break; + /* + * This is a random value. It is only necessary that + * the next TMP_MAX values generated by adding 7777 to + * VALUE are different with (module 2^32). + */ + value += 7777; + } + /* We return the null string if we can't find a unique file name. */ + pattern[0] = '\0'; + errno = EINVAL; + return -1; +} + int validate_headref(const char *path) { struct stat st; -- 1.7.0.54.gb6a04.dirty ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. Message-ID: <1266877936-13537-4-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-4-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` gitmkstemps emulates the behavior of mkstemps, which is usually used to create files in a shared directory like /tmp/, hence, it creates files with permission 0600. Add git_mkstemps_mode() that allows us to specify the desired mode, and make git_mkstemps() a wrapper that always uses 0600 to call it. Later we will use git_mkstemps_mode() when creating pack files. Signed-off-by: Matthieu Moy --- cache.h | 4 ++++ path.c | 16 ++++++++++++++-- wrapper.c | 10 ++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cache.h b/cache.h index d478eff..0319637 100644 --- a/cache.h +++ b/cache.h @@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template); int git_mkstemps(char *path, size_t n, const char *template, int suffix_len); +/* set default permissions by passing mode arguments to open(2) */ +int git_mkstemps_mode(char *pattern, int suffix_len, int mode); +int git_mkstemp_mode(char *pattern, int mode); + /* * NOTE NOTE NOTE!! * diff --git a/path.c b/path.c index ab2e368..925039a 100644 --- a/path.c +++ b/path.c @@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) #undef TMP_MAX #define TMP_MAX 16384 -int gitmkstemps(char *pattern, int suffix_len) +int git_mkstemps_mode(char *pattern, int suffix_len, int mode) { static const char letters[] = "abcdefghijklmnopqrstuvwxyz" @@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len) template[4] = letters[v % num_letters]; v /= num_letters; template[5] = letters[v % num_letters]; v /= num_letters; - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); if (fd > 0) return fd; /* @@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len) return -1; } +int git_mkstemp_mode(char *pattern, int mode) +{ + /* mkstemp is just mkstemps with no suffix */ + return git_mkstemps_mode(pattern, 0, mode); +} + +int gitmkstemps(char *pattern, int suffix_len) +{ + return git_mkstemps_mode(pattern, suffix_len, 0600); +} + int validate_headref(const char *path) { struct stat st; @@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p) } } } + diff --git a/wrapper.c b/wrapper.c index 0e3e20a..673762f 100644 --- a/wrapper.c +++ b/wrapper.c @@ -204,6 +204,16 @@ int xmkstemp(char *template) return fd; } +int xmkstemp_mode(char *template, int mode) +{ + int fd; + + fd = git_mkstemp_mode(template, mode); + if (fd < 0) + die_errno("Unable to create temporary file"); + return fd; +} + /* * zlib wrappers to make sure we don't silently miss errors * at init time. -- 1.7.0.54.gb6a04.dirty ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 4/6] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. Message-ID: <1266877936-13537-5-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-5-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` We used to create 0600 files, and then use chmod to set the group and other permission bits to the umask. This usually has the same effect as a normal file creation with a umask. But in the presence of ACLs, the group permission plays the role of the ACL mask: the "g" bits of newly created files are chosen according to default ACL mask of the directory, not according to the umask, and doing a chmod() on these "g" bits affect the ACL's mask instead of actual group permission. In other words, creating files with 0600 and then doing a chmod to the umask creates files which are unreadable by users allowed in the default ACL. To create the files without breaking ACLs, we let the umask do it's job at the file's creation time, and get rid of the later chmod. Signed-off-by: Matthieu Moy --- builtin-pack-objects.c | 18 ++---------------- t/t1304-default-acl.sh | 2 +- wrapper.c | 10 +++++++--- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index e1d3adf..539e75d 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -464,9 +464,6 @@ static int write_one(struct sha1file *f, return 1; } -/* forward declaration for write_pack_file */ -static int adjust_perm(const char *path, mode_t mode); - static void write_pack_file(void) { uint32_t i = 0, j; @@ -523,21 +520,17 @@ static void write_pack_file(void) } if (!pack_to_stdout) { - mode_t mode = umask(0); struct stat st; const char *idx_tmp_name; char tmpname[PATH_MAX]; - umask(mode); - mode = 0444 & ~mode; - idx_tmp_name = write_idx_file(NULL, written_list, nr_written, sha1); snprintf(tmpname, sizeof(tmpname), "%s-%s.pack", base_name, sha1_to_hex(sha1)); free_pack_by_name(tmpname); - if (adjust_perm(pack_tmp_name, mode)) + if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); if (rename(pack_tmp_name, tmpname)) die_errno("unable to rename temporary pack file"); @@ -565,7 +558,7 @@ static void write_pack_file(void) snprintf(tmpname, sizeof(tmpname), "%s-%s.idx", base_name, sha1_to_hex(sha1)); - if (adjust_perm(idx_tmp_name, mode)) + if (adjust_shared_perm(idx_tmp_name)) die_errno("unable to make temporary index file readable"); if (rename(idx_tmp_name, tmpname)) die_errno("unable to rename temporary index file"); @@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av) loosen_unused_packed_objects(&revs); } -static int adjust_perm(const char *path, mode_t mode) -{ - if (chmod(path, mode)) - return -1; - return adjust_shared_perm(path); -} - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 07dd6af..8472dbb 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -59,7 +59,7 @@ test_expect_failure 'Objects creation does not break ACLs with restrictive umask check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 ' -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' +test_expect_success 'git gc does not break ACLs with restrictive umask' ' git gc && check_perms_and_acl .git/objects/pack/*.pack ' diff --git a/wrapper.c b/wrapper.c index 673762f..9c71b21 100644 --- a/wrapper.c +++ b/wrapper.c @@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush) int odb_mkstemp(char *template, size_t limit, const char *pattern) { int fd; - + /* + * we let the umask do its job, don't try to be more + * restrictive except to remove write permission. + */ + int mode = 0444; snprintf(template, limit, "%s/%s", get_object_directory(), pattern); - fd = mkstemp(template); + fd = git_mkstemp_mode(template, mode); if (0 <= fd) return fd; @@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern) snprintf(template, limit, "%s/%s", get_object_directory(), pattern); safe_create_leading_directories(template); - return xmkstemp(template); + return xmkstemp_mode(template, mode); } int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) -- 1.7.0.54.gb6a04.dirty ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL on exit. Message-ID: <1266877936-13537-6-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-6-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` When reaching the end of git_mkstemps_mode, at least one call to open() has been done, and errno has been set accordingly. Setting errno is therefore not necessary, and actually harmfull since callers can't distinguish e.g. permanent failure from ENOENT, which can just mean that we need to create the containing directory. Signed-off-by: Matthieu Moy --- path.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/path.c b/path.c index 925039a..6f44921 100644 --- a/path.c +++ b/path.c @@ -222,7 +222,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) } /* We return the null string if we can't find a unique file name. */ pattern[0] = '\0'; - errno = EINVAL; return -1; } -- 1.7.0.54.gb6a04.dirty ``` ## Matthieu Moy, 2010-02-22 22:32 Subject: [PATCH 6/6] Use git_mkstemp_mode instead of plain mkstemp to create object files Message-ID: <1266877936-13537-7-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1266877936-13537-7-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: <1266597207-32036-1-git-send-email-Matthieu.Moy@imag.fr> ``` We used to unnecessarily give the read permission to group and others, regardless of the umask, which isn't serious because the objects are still protected by their containing directory, but isn't necessary either. Signed-off-by: Matthieu Moy --- sha1_file.c | 6 +++--- t/t1304-default-acl.sh | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 657825e..3316f28 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2206,7 +2206,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename) } out: - if (set_shared_perm(filename, (S_IFREG|0444))) + if (adjust_shared_perm(filename)) return error("unable to set permission to '%s'", filename); return 0; } @@ -2262,7 +2262,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) } memcpy(buffer, filename, dirlen); strcpy(buffer + dirlen, "tmp_obj_XXXXXX"); - fd = mkstemp(buffer); + fd = git_mkstemp_mode(buffer, 0444); if (fd < 0 && dirlen && errno == ENOENT) { /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); @@ -2272,7 +2272,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Try again */ strcpy(buffer + dirlen - 1, "/tmp_obj_XXXXXX"); - fd = mkstemp(buffer); + fd = git_mkstemp_mode(buffer, 0444); } return fd; } diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 8472dbb..cc30be4 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -54,7 +54,7 @@ test_expect_success 'Setup test repo' ' git commit -m "init" ' -test_expect_failure 'Objects creation does not break ACLs with restrictive umask' ' +test_expect_success 'Objects creation does not break ACLs with restrictive umask' ' # SHA1 for empty blob check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 ' -- 1.7.0.54.gb6a04.dirty ```