[PATCH 1/2] compare-trees: add --features and disallow args with it and -h

[Rob Browning]

This will be used to guard tests that are dependent on particular
features (e.g. acls).

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---
dev/compare-trees | 65 ++++++++++++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 20 deletions(-)

diff --git a/dev/compare-trees b/dev/compare-trees
index 2d2ffa8f1..118b1215d 100755
--- a/dev/compare-trees
+++ b/dev/compare-trees
@@ -7,7 +7,10 @@ set -euo pipefail

usage() {
cat <<EOF
-Usage: compare-trees [-h] [-c] [-x] SOURCE DEST
+Usage:
+ compare-trees [-c] [-x] [--] SOURCE DEST
+ compare-trees --features
+ compare-trees -h
OPTIONS:
-h
Display help
@@ -18,50 +21,60 @@ OPTIONS:
--times
--no-times
Check or don't check timestamps (checking is the default)
+ --features
+ --no-features
+ Show or don't show enabled features (can be combined with other options)
+ --
+ Don't treat following arguments as options (e.g. compare-trees -- -c dest)
EOF
}

+show_features=''
verify_content=" --checksum"
verify_times=' --times'

-while test $# -gt 0; do
- case "$1" in
- -h) usage; exit 0;;
- -c) verify_content=" --checksum"; shift;;
- -x) verify_content=""; shift;;
- --times) verify_times=' --times'; shift;;
- --no-times) verify_times=''; shift;;
- -*) usage 1>&2; exit 2;;
- [^-]*) break;;
- esac
-done
+case "$#/${1:-}" in
+ 1/-h) usage; exit 0;;
+ 1/--features) show_features=yes; shift;;
+ *)
+ while test $# -gt 0; do
+ case "$1" in
+ -h|--features) usage 1>&2; exit 2;;
+ -c) verify_content=" --checksum"; shift;;
+ -x) verify_content=""; shift;;
+ --times) verify_times=' --times'; shift;;
+ --no-times) verify_times=''; shift;;
+ --) shift; break;;
+ [^-]*) break;;
+ esac
+ done
+esac

-if ! test $# -eq 2
-then
+if test $# -ne 2 -a -z "$show_features"; then
usage 1>&2
exit 2
fi

-src="$1"
-dest="$2"
-
-tmpfile="$(mktemp /tmp/bup-test-XXXXXXX)" || exit $?
-trap "rm -rf '$tmpfile'" EXIT || exit $?
+src="${1:-}"
+dest="${2:-}"

rsync_opts="-rlpgoD" # --archive, without --times
rsync_opts="$rsync_opts -niH --delete"
rsync_opts="$rsync_opts$verify_content"
rsync_opts="$rsync_opts$verify_times"

+comparing_acls=''
rsync_version=$(rsync --version)
if [[ ! "$rsync_version" =~ "ACLs" ]] || [[ "$rsync_version" =~ "no ACLs" ]]; then
echo "Not comparing ACLs (not supported by available rsync)" 1>&2
else
- case $OSTYPE in
+ case "$OSTYPE" in
+ # FIXME: darwin and netbsd now, at least...
cygwin|darwin|netbsd)
echo "Not comparing ACLs (not yet supported on $OSTYPE)" 1>&2
;;
*)
+ comparing_acls=yes
rsync_opts="$rsync_opts -A"
;;
esac
@@ -74,6 +87,18 @@ else
xattrs_available=yes
fi

+if test "$show_features"; then
+ echo "POSIX ACLs: ${comparing_acls:-no}"
+ echo "Extended attributes (xattrs): ${xattrs_available:-no}"
+fi
+
+if test "$show_features"; then
+ exit 0
+fi
+
+tmpfile="$(mktemp /tmp/bup-test-XXXXXXX)" || exit $?
+trap "rm -rf '$tmpfile'" EXIT || exit $?
+
# Even in dry-run mode, rsync may fail if -X is specified and the
# filesystems don't support xattrs.

--
2.39.2

[Rob Browning]

While toying with some broader changes to our acl support, I noticed
that the current apply_acl function unintentionally restores any acls
of ACL_TYPE_DEFAULT as ACL_TYPE_ACCESS instead. Aside from the
correctness question, this could leave paths with incorrect
permissions.

My current assessment is that if none of the restored paths have
anything other than "normal" unix permissions (i.e. user, group,
other), if for example, no special acl permissions have been set via
something like "setfacl -m u:some-other-user:rw ...", then this bug
shouldn't affect restores because the acl_extended_file() check in
read_acl would have prevented any acls from being added during the
original save (since the acls are "trivial", i.e. fully represented by
the stat mode).

Since this issue seems like it could be a potentially significant
problem in some cases, perhaps even a security risk, I'm proposing
these changes for both the main branch, and a new 0.33.x branch.

Thanks

Rob Browning (2):
compare-trees: add --features and disallow args with it and -h
Restore posix1e default acls as default, not access; improve tests

dev/compare-trees | 65 ++++++++++++++++--------
lib/bup/_helpers.c | 9 ++--
test/ext/test-meta | 9 ----
test/ext/test-meta-acls | 107 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 157 insertions(+), 33 deletions(-)
create mode 100755 test/ext/test-meta-acls

--
2.39.2

[Rob Browning]

Previously bup would restore default acls using ACL_TYPE_ACCESS rather
than ACL_TYPE_DEFAULT. Since that was done after restoring any access
acl, the default acl would presumably supersede the access acl, and no
default acl would be set.

Adjust apply_acl to use the correct type, and add test-meta-acls to
test more cases, including that one. Arrange for the new tests to run
whenever the conditions are right -- setfacl and getfacl are
available, and the filesystem and rsync appear to support acls. The
previous acl tests would only run when root.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

lib/bup/_helpers.c | 9 ++--
test/ext/test-meta | 9 ----
test/ext/test-meta-acls | 107 ++++++++++++++++++++++++++++++++++++++++

3 files changed, 112 insertions(+), 13 deletions(-)
create mode 100755 test/ext/test-meta-acls

diff --git a/lib/bup/_helpers.c b/lib/bup/_helpers.c
index 7a6a6e412..507b5842e 100644
--- a/lib/bup/_helpers.c
+++ b/lib/bup/_helpers.c
@@ -2015,7 +2015,8 @@ out:
return ret;
}

-static int bup_apply_acl_string(const char *name, const char *s)
+static int
+bup_apply_acl_string(const char *name, acl_type_t type, const char *s)
{
acl_t acl = acl_from_text(s);
int ret = 0;
@@ -2025,7 +2026,7 @@ static int bup_apply_acl_string(const char *name, const char *s)
return -1;
}

- if (acl_set_file(name, ACL_TYPE_ACCESS, acl)) {
+ if (acl_set_file(name, type, acl)) {
PyErr_SetFromErrno(PyExc_IOError);
ret = -1;
}
@@ -2042,10 +2043,10 @@ static PyObject *bup_apply_acl(PyObject *self, PyObject *args)
if (!PyArg_ParseTuple(args, cstr_argf cstr_argf "|" cstr_argf, &name, &acl, &def))
return NULL;

- if (bup_apply_acl_string(name, acl))
+ if (bup_apply_acl_string(name, ACL_TYPE_ACCESS, acl))
return NULL;

- if (def && bup_apply_acl_string(name, def))
+ if (def && bup_apply_acl_string(name, ACL_TYPE_DEFAULT, def))
return NULL;

Py_RETURN_NONE;
diff --git a/test/ext/test-meta b/test/ext/test-meta
index 4d6b86aae..058ba8007 100755
--- a/test/ext/test-meta
+++ b/test/ext/test-meta
@@ -749,15 +749,6 @@ if [ "$root_status" = root ]; then
'import sys; exit(not len(sys.stdin.readlines()) == 2)'
) || exit $?

- WVSTART 'meta - POSIX.1e ACLs (as root)'
- WVPASS force-delete "$testfs"/src
- WVPASS mkdir "$testfs"/src
- WVPASS touch "$testfs"/src/foo
- WVPASS mkdir "$testfs"/src/bar
- WVPASS setfacl -m u:root:r "$testfs"/src/foo
- WVPASS setfacl -m u:root:r "$testfs"/src/bar
- (WVPASS cd "$testfs"; WVPASS test-src-create-extract) || exit $?
-
# Test restoration to a limited filesystem (vfat).
(
WVPASS bup meta --create --recurse --file "$testfs"/src.meta \
diff --git a/test/ext/test-meta-acls b/test/ext/test-meta-acls
new file mode 100755
index 000000000..b17c10f09
--- /dev/null
+++ b/test/ext/test-meta-acls
@@ -0,0 +1,107 @@
+#!/usr/bin/env bash
+. wvtest.sh
+. wvtest-bup.sh
+. dev/lib.sh
+
+set -o pipefail
+
+if ! command -v getfacl > /dev/null || ! command -v setfacl > /dev/null; then
+ WVSKIP "No getfacl and setfacl; skipping test-meta-acls"

+ exit 0
+fi
+

+top="$(WVPASS pwd)" || exit $?
+
+bup() { "$top/bup" "$@"; }
+compare-trees() { "$top/dev/compare-trees" "$@"; }
+id-other-than() { "$top/dev/id-other-than" "$@"; }
+
+if ! bup features | grep -qi 'posix acls: yes'; then
+ WVSKIP "bup features missing POSIX ACLs; skipping test-meta-acls"

+ exit 0
+fi
+

+if ! compare-trees --features | grep -qi 'posix acls: yes'; then
+ WVSKIP "compare-trees --features missing POSIX ACLs; skipping test-meta-acls"

+ exit 0
+fi
+

+tmpdir="$(WVPASS wvmktempdir)" || exit $?
+export BUP_DIR="$tmpdir/bup"
+
+uid=$(WVPASS id -un) || exit $?
+other_uinfo="$(id-other-than --user "$uid")" || exit $?
+other_user="${other_uinfo%%:*}"
+other_uid="${other_uinfo##*:}"
+
+gid=$(WVPASS id -gn) || exit $?
+other_ginfo="$(id-other-than --group "$gid")" || exit $?
+other_group="${other_ginfo%%:*}"
+other_gid="${other_ginfo##*:}"
+
+WVPASS cd "$tmpdir"
+
+WVPASS mkdir src
+WVPASS touch src/u-r
+
+if ! setfacl -m "u:$other_user:r" src/u-r; then
+ WVSKIP "setfacl $top/testfile failed; skipping test-meta-acls"

+ exit 0
+fi
+

+WVSTART "Basic ACL support"
+
+# file ACL_USER access acl(5)
+for perm in r rw rwx; do
+ WVPASS touch src/u-"$perm"
+ WVPASS setfacl -m "u:$other_user:$perm" src/u-"$perm"
+done
+# file ACL_GROUP access acl(5)
+for perm in r rw rwx; do
+ WVPASS touch src/g-"$perm"
+ WVPASS setfacl -m "g:$other_group:$perm" src/g-"$perm"
+done
+
+# directory ACL_USER access acl(5)
+for perm in r rw rwx; do
+ WVPASS mkdir src/d-u-"$perm"
+ WVPASS setfacl -m "u:$other_user:$perm" src/d-u-"$perm"
+done
+# directory ACL_GROUP access acl(5)
+for perm in r rw rwx; do
+ WVPASS mkdir src/d-g-"$perm"
+ WVPASS setfacl -m "g:$other_group:$perm" src/d-g-"$perm"
+done
+
+# directory ACL_USER default acl(5)
+for perm in r rw rwx; do
+ WVPASS mkdir src/d-def-u-"$perm"
+ WVPASS setfacl -d -m "u:$other_user:$perm" src/d-def-u-"$perm"
+done
+# directory ACL_GROUP default acl(5)
+for perm in r rw rwx; do
+ WVPASS mkdir src/d-def-g-"$perm"
+ WVPASS setfacl -d -m "g:$other_group:$perm" src/d-def-g-"$perm"
+done
+
+# directory ACL_USER access and default acl(5)
+for perm in r rw rwx; do
+ WVPASS mkdir src/d-both-u-"$perm"
+ WVPASS setfacl -m "u:$other_user:$perm" src/d-both-u-"$perm"
+ WVPASS setfacl -d -m "u:$other_user:$perm" src/d-both-u-"$perm"
+done
+# directory ACL_GROUP access and default acl(5)
+for perm in r rw rwx; do
+ WVPASS mkdir src/d-both-g-"$perm"
+ WVPASS setfacl -m "g:$other_group:$perm" src/d-both-g-"$perm"
+ WVPASS setfacl -d -m "g:$other_group:$perm" src/d-both-g-"$perm"
+done
+
+WVPASS bup init
+WVPASS bup index -u src
+WVPASS bup save --strip -n acls src
+WVPASS bup restore -C dest acls/latest/.
+WVPASS compare-trees src/ dest/
+
+cd "$top"
+WVPASS rm -rf "$tmpdir"
--
2.39.2

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> --- a/test/ext/test-meta
> +++ b/test/ext/test-meta
> @@ -749,15 +749,6 @@ if [ "$root_status" = root ]; then
> 'import sys; exit(not len(sys.stdin.readlines()) == 2)'
> ) || exit $?
>
> - WVSTART 'meta - POSIX.1e ACLs (as root)'
> - WVPASS force-delete "$testfs"/src
> - WVPASS mkdir "$testfs"/src
> - WVPASS touch "$testfs"/src/foo
> - WVPASS mkdir "$testfs"/src/bar
> - WVPASS setfacl -m u:root:r "$testfs"/src/foo
> - WVPASS setfacl -m u:root:r "$testfs"/src/bar
> - (WVPASS cd "$testfs"; WVPASS test-src-create-extract) || exit $?
> -
> # Test restoration to a limited filesystem (vfat).
> (
> WVPASS bup meta --create --recurse --file "$testfs"/src.meta \

Actually, I think I should leave these in the final version, i.e. can't
hurt, might help. They run as root using a filesystem we created that
should have acl support even if the source tree filesystem doesn't.

--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> --- a/dev/compare-trees
> +++ b/dev/compare-trees

[...]

> @@ -18,50 +21,60 @@ OPTIONS:
> --times
> --no-times
> Check or don't check timestamps (checking is the default)
> + --features
> + --no-features
> + Show or don't show enabled features (can be combined with other options)
> + --
> + Don't treat following arguments as options (e.g. compare-trees -- -c dest)
> EOF
> }

Vestigial, should just be:

--features
Show enabled features

[Greg Troxel]

I'm somewhat unclear on ACLs and don't have any useful comments on this
patch. But I recently had to understand more because of unison and I
would like to step way back and talk about the big picture.

ugo mode bits are of quite standard and interoperable. So when you
create a backup on one operating system and filesystem and restore it on
a different operating system or filesystem, you get back files with the
same semantics.

It seems obvious that acls should have the same property, even if the
details are unclear. Is that agreed?

With ACLs, there is "posix draft ACLs" which are not really a standard
but are a standard. There is also "NFSv4 ACLs".

AIUI, POSIX specifies system calls and user programs (e.g. getfacl), and
on BSD (and Solaris?) systems that support ACLs, those are present.

On GNU/Linux, the situation seems messier, and I see two realities
described:

A) userland support isn't there and ACLs are done by messing with system
extended attributes

B) userland support is fine but it isn't installed on some systems

My current read is "B, but many people can't cope with the concept of
needing things installed so a lot of A happens".

I wonder if in bup which approach we are taking and if backups made on
GNU/Linux can be restored on say FreeBSD/zfs (assuming that supports
ACLs which I think it does) and vice versa, with ACLs intact.

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> It seems obvious that acls should have the same property, even if the
> details are unclear. Is that agreed?

While I suspect there's a good bit of complexity in the details, broadly
speaking, bup's intended to try to capture all the relevant information
at save time, and then restore that on a best effort basis during
restore (given the target platform and filesystem capabilities).

So ideally, bup should capture posix1e acls when they're available, and
attempt to restore them when requested.

Though also ideally, in the longer run bup should probably have better
control over the restore process, perhaps providing some way to make
selections about how the restore process should handle various
categories. For example, you might want to skip acls if you know you're
restoring to a vfat filesystem, instead of registering a flood of
deferred errors...

> With ACLs, there is "posix draft ACLs" which are not really a standard
> but are a standard. There is also "NFSv4 ACLs".

Right, and right now bup has a reserved metadata tag for nfsv4 acls and
a "todo", and that's it, i.e. no support yet. My general impression is
that they're related, but different...

> AIUI, POSIX specifies system calls and user programs (e.g. getfacl), and
> on BSD (and Solaris?) systems that support ACLs, those are present.

Not sure whether or not setfacl and getfacl are in the standard --
though I haven't read the whole thing (and the proposal copy I found
doesn't support searching).

It looked like at least FreeBSD and MacOS provide the core functions via
libc, while, as you say, Linux requires -lacl.

It's also the case that bup only supports Linux right now because the C
code requires acl_extended_file(), which is Linux specific (also
indicates that only Linux saves/restores can be affected by the bug).

> A) userland support isn't there and ACLs are done by messing with system
> extended attributes

Do you mean posix1e acls, or just that you may be able to do
somewhat-related things with xattrs?

> I wonder if in bup which approach we are taking and if backups made on
> GNU/Linux can be restored on say FreeBSD/zfs (assuming that supports
> ACLs which I think it does) and vice versa, with ACLs intact.

Eventually, I think so -- we currently handle things via the posix1e
"short" text form, which is well specified, though (maybe) a bit
ambiguous, and is restored via the standard's acl_from_text().

i.e. I was slightly annoyed by the standard's use of colon-delimited
"strings" for user and group names, given all the potential issues
there. For example, from the standard:

(23.3.2): If an implementation allows the colon character ":" to be
present in an ACL entry qualifier, then that implementation shall
provide a method for distinguishing between a colon character as a
field separator in an ACL entry definition and a colon character as a
component of the ACL entry

...and so locally, I've reworked our code to handle things ourselves by
creating the numeric-id-only short form manually, ignoring
acl_to_text(), which doesn't let you specify, and not relying on the
Linux-specific acl_to_any_text(). Then we separately capture the
relevant id -> name mappings[1].

That should be a fairly comprehensive fix, and should also allow us to
support other platforms, but it's a good bit more code, and so assuming
I decide to finish it up and propose it, we'll have to decide whether
it's desirable.

[1] I also made acl_extended_file optional, and added support for
acl_is_trivial_np() (similar) when we find it.

Thanks

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> While toying with some broader changes to our acl support, I noticed
> that the current apply_acl function unintentionally restores any acls
> of ACL_TYPE_DEFAULT as ACL_TYPE_ACCESS instead. Aside from the
> correctness question, this could leave paths with incorrect
> permissions.
>
> My current assessment is that if none of the restored paths have
> anything other than "normal" unix permissions (i.e. user, group,
> other), if for example, no special acl permissions have been set via
> something like "setfacl -m u:some-other-user:rw ...", then this bug
> shouldn't affect restores because the acl_extended_file() check in
> read_acl would have prevented any acls from being added during the
> original save (since the acls are "trivial", i.e. fully represented by
> the stat mode).
>
> Since this issue seems like it could be a potentially significant
> problem in some cases, perhaps even a security risk, I'm proposing
> these changes for both the main branch, and a new 0.33.x branch.
>
> Thanks
>
> Rob Browning (2):
> compare-trees: add --features and disallow args with it and -h
> Restore posix1e default acls as default, not access; improve tests

Pushed this to 0.32.x, 0.33.x, and main.