[PATCH 0/5] tweak t1304 ACL tests so they work on Solaris

[PATCH 0/5] tweak t1304 ACL tests so they work on Solaris

[Brandon Casey]

The operaton of setfacl and the interaction of mode bits with ACL seem
to differ on Solaris and Linux.  The current t1304 tests fail on Solaris
7 and 10.

The following patch series seems to fix the tests on Solaris 10.  Solaris
7 still fails t1304.3 for some unknown reason.  I used the script below to
do some testing.

On Linux it prints out:

  Initialized empty Git repository in /var/tmp/test_acl2/.git/
  [master (root-commit) 22176cb] test commit
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 foo.txt
  -r--r-----+ 1 casey NRL7240 29 Mar 15 11:47 .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # file: .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx	#effective:r--
  group::---
  mask::r--
  other::---

  -r--r-----+ 1 casey NRL7240 225 Mar 15 11:47 .git/objects/pack/pack-0611d0bcfb48f3db9f0d51334a035289272dbb2c.pack
  # file: .git/objects/pack/pack-0611d0bcfb48f3db9f0d51334a035289272dbb2c.pack
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx	#effective:r--
  group::---
  mask::r--
  other::---


On Solaris 10 it prints out:

  Initialized empty Git repository in /var/tmp/test_acl2/.git/
  [master (root-commit) 5e4c0c6] test commit
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 foo.txt
  -r--------+  1 casey    NRL7240       29 Mar 15 11:49 .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec

  # file: .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:r--
  group::---		#effective:---
  mask:r--
  other:---
  -r--------+  1 casey    NRL7240      225 Mar 15 11:49 .git/objects/pack/pack-fedf57238f710f6b713019fe02fddab7a63702d0.pack

  # file: .git/objects/pack/pack-fedf57238f710f6b713019fe02fddab7a63702d0.pack
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:r--
  group::---		#effective:---
  mask:r--
  other:---


On Solaris 7 it prints out:

  Initialized empty Git repository in /var/tmp/test_acl2/.git/
  [master (root-commit) ca62c6f] test commit
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 foo.txt
  -r--------+  1 casey    NRL7240       29 Mar 15 10:51 .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec

  # file: .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:r--
  group::---		#effective:---
  mask:r--
  other:---
  -r--------+  1 casey    NRL7240      225 Mar 15 10:51 .git/objects/pack/pack-dc814256703ad9c01e6dd1080c7151f664c6382a.pack

  # file: .git/objects/pack/pack-dc814256703ad9c01e6dd1080c7151f664c6382a.pack
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:---
  group::---		#effective:---
  mask:---
  other:---


So, on Solaris 7, the pack file incorrectly has effective permissions of
'---' for the 'display' user.  For some reason the mask ACL was not
inherited from the parent directory.


When the patch series is applied on top of 7aba6185, which introduced these
tests, Linux still correctly fails t1304.3, but t1304.2 now passes.  This is
ok since t1304.2 was only failing because git gave object files more
permissive file permissions than what were requested, not because the ACL's
were clobbered.  Of course, when applied on top of master, all tests pass on
Linux, and now on Solaris 10.


--->8--- test_acl.sh --->8---
#!/bin/sh

alt_user=display

testcase=${1:-'1'}

case "$testcase" in
   1) test_dir=/var/tmp/test_acl1 ;;
   2) test_dir=/var/tmp/test_acl2
      GIT_EXEC_PATH=`pwd`
      PATH=`pwd`:$PATH
      GITPERLLIB=`pwd`/perl/blib/lib
      export GIT_EXEC_PATH PATH GITPERLLIB
      ;;
   *)
      echo 1>&2 "Usage: $0 [1|2]"
      exit 1
      ;;
esac


umask 077 &&
  rm -rf "$test_dir" && mkdir "$test_dir" && cd "$test_dir" &&
  setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx . &&
  setfacl -m d:u:$alt_user:rwx . &&
  setfacl -m u:$alt_user:rwx,m:rwx . &&
  git init &&
  echo 'this is a test' > foo.txt &&
  git add foo.txt &&
  git commit -m 'test commit' &&
  ls -l .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec &&
  getfacl .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec &&
  git gc &&
  ls -l .git/objects/pack/*.pack &&
  getfacl .git/objects/pack/*.pack
--->8---


Brandon Casey (5):
  t/t1304: avoid -d option to setfacl
  t/t1304: set the Default ACL base entries
  t/t1304: use 'test -r' to test readability rather than looking at
    mode bits
  t/t1304: set the mask ACL that is checked in check_perms_and_acl
  t/t1304: make a second colon optional in the mask ACL check

 t/t1304-default-acl.sh |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

[PATCH 1/5] t/t1304: avoid -d option to setfacl

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Some platforms (Solaris) have a setfacl whose -d switch works differently
than the one on Linux.  On Linux, it causes all operations to be applied
to the Default ACL.  There is a notation for operating on the Default ACL:

   [d[efault]:] [u[ser]:]uid [:perms]

so use it instead of the -d switch.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index cc30be4..415a2dd 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -46,8 +46,8 @@ 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 &&
+	setfacl -m d:u:"$LOGNAME":rwx  $dirs_to_set &&
+	setfacl -m d:u:root:rwx        $dirs_to_set &&
 
 	touch file.txt &&
 	git add file.txt &&
-- 
1.6.6.2

[PATCH 2/5] t/t1304: set the Default ACL base entries

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

According to the Linux setfacl man page, in order for an ACL to be valid,
the following rules must be satisfied:

   * Whenever an ACL contains any Default ACL entries, the three Default
     ACL base entries (default owner, default group, and default others)
     must also exist.

   * Whenever a Default ACL contains named user entries or named group
     objects, it must also contain a default effective rights mask.

Some implementations of setfacl (Linux) do this automatically when
necessary, some (Solaris) do not.  Solaris's setfacl croaks when trying to
create a default user ACL if the above rules are not satisfied.  So, create
them before modifying the default user ACL's.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 415a2dd..3a1532b 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -45,6 +45,7 @@ check_perms_and_acl () {
 dirs_to_set="./ .git/ .git/objects/ .git/objects/pack/"
 
 test_expect_success 'Setup test repo' '
+	setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx $dirs_to_set &&
 	setfacl -m u:root:rwx          $dirs_to_set &&
 	setfacl -m d:u:"$LOGNAME":rwx  $dirs_to_set &&
 	setfacl -m d:u:root:rwx        $dirs_to_set &&
-- 
1.6.6.2

[PATCH 3/5] t/t1304: use 'test -r' to test readability rather than looking at mode bits

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

This test was using the group read permission bit as an indicator of the
default ACL mask.  This behavior is valid on Linux but not on other
platforms like Solaris.  So, rather than looking at mode bits, just test
readability for the user.  This, along with the checks for the existence
of the ACL's that were set on the parent directories, should be enough.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 3a1532b..52246d7 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -20,21 +20,8 @@ if ! setfacl -m u:root:rwx .; then
     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 &&
+	test -r "$1" &&
 	getfacl "$1" > actual &&
 	grep -q "user:root:rwx" actual &&
 	grep -q "user:${LOGNAME}:rwx" actual &&
-- 
1.6.6.2

[PATCH 4/5] t/t1304: set the mask ACL that is checked in check_perms_and_acl

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>


Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 52246d7..85351ae 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -33,6 +33,7 @@ dirs_to_set="./ .git/ .git/objects/ .git/objects/pack/"
 
 test_expect_success 'Setup test repo' '
 	setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx $dirs_to_set &&
+	setfacl -m m:rwx               $dirs_to_set &&
 	setfacl -m u:root:rwx          $dirs_to_set &&
 	setfacl -m d:u:"$LOGNAME":rwx  $dirs_to_set &&
 	setfacl -m d:u:root:rwx        $dirs_to_set &&
-- 
1.6.6.2

[PATCH 5/5] t/t1304: make a second colon optional in the mask ACL check

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Solaris only uses one colon in the listing of the ACL mask, Linux uses two,
so substitute egrep for grep and make the second colon optional.

The -q option for Solaris 7's /usr/xpg4/bin/egrep does not appear to be
implemented, so redirect output to /dev/null.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 85351ae..055ad00 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -25,7 +25,7 @@ check_perms_and_acl () {
 	getfacl "$1" > actual &&
 	grep -q "user:root:rwx" actual &&
 	grep -q "user:${LOGNAME}:rwx" actual &&
-	grep -q "mask::r--" actual &&
+	egrep "mask::?r--" actual > /dev/null 2>&1 &&
 	grep -q "group::---" actual || false
 }
 
-- 
1.6.6.2

Re: [PATCH 3/5] t/t1304: use 'test -r' to test readability rather than looking at mode bits

[Matthieu Moy]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> -	-r--r-----*)
> -		: happy
> -		;;

> +	test -r "$1" &&

Maybe it makes sense to ask for -r--?-----* instead, to make sure we
don't give the read bit to anyone. But that's not really important
since we're protected by the directory's permission anyway.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 4/5] t/t1304: set the mask ACL that is checked in check_perms_and_acl

[Matthieu Moy]

Brandon Casey <casey@nrlssc.navy.mil> writes:

>  test_expect_success 'Setup test repo' '
>  	setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx $dirs_to_set &&
> +	setfacl -m m:rwx               $dirs_to_set &&

The patch sounds right, but I don't understand the commit message. You
set m:rwx, and check_perms_and_acl expects mask::r--, so it's not
exactly what check_perms_and_acl checks.

My understanding is that you set the mask here to enforce the validity
of the ACL, but then you may want to just squash this into [PATCH 2/5].

But again, that's bikeshedding, the patch serie looks right to me.
Sorry for not making the test right myself ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/5] t/t1304: use 'test -r' to test readability rather than looking at mode bits

[Brandon Casey]

On 03/15/2010 12:33 PM, Matthieu Moy wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> -	-r--r-----*)
>> -		: happy
>> -		;;
> 
>> +	test -r "$1" &&
> 
> Maybe it makes sense to ask for -r--?-----* instead, to make sure we
> don't give the read bit to anyone. But that's not really important
> since we're protected by the directory's permission anyway.

I'm a little hesitant to add a test on the output from 'ls', since
Solaris and Linux produce different output in the presence of ACL's.

-brandon

Re: [PATCH 4/5] t/t1304: set the mask ACL that is checked in check_perms_and_acl

[Brandon Casey]

On 03/15/2010 12:37 PM, Matthieu Moy wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>>  test_expect_success 'Setup test repo' '
>>  	setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx $dirs_to_set &&
>> +	setfacl -m m:rwx               $dirs_to_set &&
> 
> The patch sounds right, but I don't understand the commit message. You
> set m:rwx, and check_perms_and_acl expects mask::r--, so it's not
> exactly what check_perms_and_acl checks.

Ah, yeah, it does sound like I'm saying that check_perms_and_acl is
checking for the particular mask that I'm setting.  I really meant
it to read more like: since check_perms_and_acl is checking the
'mask ACL', it should be set appropriately.

> My understanding is that you set the mask here to enforce the validity
> of the ACL, but then you may want to just squash this into [PATCH 2/5].

I think the ACL is valid according to the rules stated in the Linux man
page, but depending on the previously existing mask ACL on the directories,
the other ACL's that were set may or may not have any effect.  I think on
Linux, the setfacl command updates the effective rights mask when other
ACL entries are modified.  I don't think this happens on Solaris.

If I do this:

  $ cd /var/tmp &&
    mkdir test &&
    setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx test &&
    setfacl -m d:u:guest:rwx test &&
    setfacl -m u:guest:rwx test &&
    getfacl test

On Solaris I get:

  # file: test
  # owner: XXX
  # group: XXX
  user::rwx
  user:guest:rwx                #effective:---
  group::---              #effective:---
  mask:---
  other:---
  default:user::rwx
  default:user:guest:rwx
  default:group::---
  default:mask:rwx
  default:other:---

and on Linux I get:

# file: test
# owner: XXX
# group: XXX
user::rwx
user:guest:rwx
group::---
mask::rwx
other::---
default:user::rwx
default:user:guest:rwx
default:group::---
default:mask::rwx
default:other::---


Notice how the mask entry is different.  On Solaris you get --- and user
'guest' effectively has no permissions, while on Linux it has full rwx.
So for the test we should set the mask explicitly.

-brandon

[PATCH 4/5 v2] t/t1304: set the ACL effective rights mask

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Some implementations of setfacl do not recalculate the effective rights
mask when the ACL is modified.  So, set the effective rights mask
explicitly to ensure that the ACL's that are set on the directories will
have effect.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


Here's a new patch with an improved commit message (hopefully).

-brandon


 t/t1304-default-acl.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 52246d7..85351ae 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -33,6 +33,7 @@ dirs_to_set="./ .git/ .git/objects/ .git/objects/pack/"
 
 test_expect_success 'Setup test repo' '
 	setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx $dirs_to_set &&
+	setfacl -m m:rwx               $dirs_to_set &&
 	setfacl -m u:root:rwx          $dirs_to_set &&
 	setfacl -m d:u:"$LOGNAME":rwx  $dirs_to_set &&
 	setfacl -m d:u:root:rwx        $dirs_to_set &&
-- 
1.6.6.2

Re: [PATCH 4/5 v2] t/t1304: set the ACL effective rights mask

[Matthieu Moy]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Here's a new patch with an improved commit message (hopefully).

Yes, I do like it better.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/