do not archive ACLs that are equivalent to the mode bits (and fix FreeBSD default ACL loss)

src/borg/fuse.py

@@ -74,7 +74,7 @@ def async_wrapper(fn):
 from .storelocking import LockRefresher
 from .helpers.lrucache import LRUCache
 from .item import Item
-from .platform import uid2user, gid2group, acl_text_to_xattr
+from .platform import uid2user, gid2group, acl_text_to_xattr, acl_is_extended
 from .platformflags import is_darwin, is_linux
 from .repository import Repository
 
@@ -683,15 +683,15 @@ def listxattr(self, inode, ctx=None):
         item = self.get_item(inode)
         names = list(item.get("xattrs", {}).keys())
         # expose the archived POSIX ACLs, so e.g. getfacl or tools copying from the mount can read them.
-        names.extend(xattr_name for xattr_name, attr in ACL_XATTRS.items() if attr in item)
+        names.extend(xattr_name for xattr_name, attr in ACL_XATTRS.items() if acl_is_extended(item.get(attr)))
         return names
 
     @async_wrapper
     def getxattr(self, inode, name, ctx=None):
         item = self.get_item(inode)
         if name in ACL_XATTRS:
             acl = item.get(ACL_XATTRS[name])
-            if acl is None:
+            if not acl_is_extended(acl):
                 raise llfuse.FUSEError(ENOATTR)
             try:
                 return acl_text_to_xattr(acl, numeric_ids=self.numeric_ids)

src/borg/hlfuse.py

@@ -31,7 +31,7 @@
 from .storelocking import LockRefresher
 from .helpers.lrucache import LRUCache
 from .item import Item
-from .platform import uid2user, gid2group, acl_text_to_xattr
+from .platform import uid2user, gid2group, acl_text_to_xattr, acl_is_extended
 from .platformflags import is_darwin, is_linux
 from .repository import Repository
 
@@ -593,7 +593,7 @@ def listxattr(self, path):
         item = self.get_inode(node.ino)
         result = [k.decode("utf-8", "surrogateescape") for k in item.get("xattrs", {}).keys()]
         # expose the archived POSIX ACLs, so e.g. getfacl or tools copying from the mount can read them.
-        result.extend(xattr_name for xattr_name, attr in ACL_XATTRS.items() if attr in item)
+        result.extend(xattr_name for xattr_name, attr in ACL_XATTRS.items() if acl_is_extended(item.get(attr)))
         debug_log(f"listxattr -> {result}")
         return result
 
@@ -606,7 +606,7 @@ def getxattr(self, path, name, position=0):
         name_str = name if isinstance(name, str) else name.decode("utf-8", "surrogateescape")
         if name_str in ACL_XATTRS:
             acl = item.get(ACL_XATTRS[name_str])
-            if acl is None:
+            if not acl_is_extended(acl):
                 debug_log("getxattr -> ENOATTR")
                 raise hlfuse.FuseOSError(ENOATTR)
             try:

src/borg/platform/__init__.py

@@ -12,6 +12,7 @@
 from .base import SaveFile, sync_dir, fdatasync, safe_fadvise
 from .base import get_process_id, fqdn, hostname, hostid, swidth
 from .base import acl_text_to_xattr  # overridden below for platforms supporting it
+from .base import acl_is_extended
 
 # work around pyinstaller "forgetting" to include the xattr module
 from . import xattr  # noqa: F401

src/borg/platform/base.py

@@ -95,6 +95,32 @@ def acl_text_to_xattr(acl, numeric_ids=False):
     raise NotImplementedError
 
 
+def acl_is_extended(acl):
+    """
+    Does *acl* (an ACL in the borg item text representation) define anything beyond the
+    traditional permission bits?
+
+    A trivial ACL only has the 3 base entries (owner, owning group, other) and thus is fully
+    equivalent to the mode bits - the kernel does not keep an ACL xattr for such an ACL.
+    borg used to archive a trivial acl_access for directories that only had a default ACL
+    (and an empty acl_default for files/dirs without one), so such ACLs may be present in
+    older archives and must not be offered as ACL xattrs by the FUSE mount.
+    """
+    if not acl:
+        return False
+    for entry in acl.split(b"\n"):
+        if not entry:
+            continue
+        fields = entry.split(b":")
+        if len(fields) < 3:  # unexpected, let the ACL converter deal with it
+            return True
+        if fields[1]:  # a named user/group entry
+            return True
+        if fields[0] not in (b"user", b"group", b"other"):  # e.g. a mask entry
+            return True
+    return False
+
+
 try:
     from os import lchflags  # type: ignore[attr-defined]
 

src/borg/platform/freebsd.pyx

@@ -137,7 +137,10 @@ cdef _get_acl(p, type, item, attribute, flags, fd=None):
     if text == NULL:
         acl_free(acl)
         raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(p))
-    item[attribute] = text
+    if text[0] != 0:
+        # an empty ACL (e.g. a directory that has no default ACL) means: there is none.
+        # do not archive it - it would only be restored as a no-op.
+        item[attribute] = text
     acl_free(text)
     acl_free(acl)
 
@@ -154,21 +157,35 @@ def acl_get(path, item, st, numeric_ids=False, fd=None):
     flags |= ACL_TEXT_NUMERIC_IDS if numeric_ids else 0
     if isinstance(path, str):
         path = os.fsencode(path)
+    is_dir = stat.S_ISDIR(st.st_mode)
     ret = acl_extended_link_np(path)
     if ret < 0:
         raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
-    if ret == 0:
+    # note: acl_extended_link_np only looks at the access (resp. NFSv4) ACL, so a directory
+    # that only has a default ACL is not "extended" for it - we must not stop looking then.
+    extended = ret == 1
+    if not extended and not is_dir:
         # there is no ACL defining permissions other than those defined by the traditional file permission bits.
         return
-    ret = lpathconf(path, _PC_ACL_NFS4)
-    if ret < 0:
-        raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
-    nfs4_acl = ret == 1
+    if extended:
+        ret = lpathconf(path, _PC_ACL_NFS4)
+        if ret < 0:
+            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+        nfs4_acl = ret == 1
+    else:
+        # a directory with no extended access ACL: only POSIX.1e filesystems have default ACLs.
+        # if this filesystem does not do them (or we can not tell), there is nothing to archive.
+        nfs4_acl = False
+        if lpathconf(path, _PC_ACL_EXTENDED) != 1:
+            return
     if nfs4_acl:
+        # NFSv4 ACLs have no separate default ACL, inheritance is expressed by entry flags.
         _get_acl(path, ACL_TYPE_NFS4, item, 'acl_nfs4', flags, fd=fd)
     else:
-        _get_acl(path, ACL_TYPE_ACCESS, item, 'acl_access', flags, fd=fd)
-        if stat.S_ISDIR(st.st_mode):
+        if extended:
+            _get_acl(path, ACL_TYPE_ACCESS, item, 'acl_access', flags, fd=fd)
+        if is_dir:
+            # only directories can have a default ACL.
             _get_acl(path, ACL_TYPE_DEFAULT, item, 'acl_default', flags, fd=fd)
 
 

src/borg/platform/linux.pyx

@@ -17,6 +17,7 @@ except ImportError:
     SYNC_FILE_RANGE_LOADED = False
 
 from libc cimport errno
+from posix.types cimport mode_t
 
 
 
@@ -48,10 +49,12 @@ cdef extern from "sys/acl.h":
     int acl_set_file(const char *path, int type, acl_t acl)
     int acl_set_fd(int fd, acl_t acl)
     acl_t acl_from_text(const char *buf)
+    int acl_equiv_mode(acl_t acl, mode_t *mode_p)
 
 cdef extern from "acl/libacl.h":
     int acl_extended_file_nofollow(const char *path)
     int acl_extended_fd(int fd)
+    int acl_entries(acl_t acl)
     char *acl_to_any_text(acl_t acl, const char *prefix, char separator, int options)
     int TEXT_NUMERIC_IDS
 
@@ -298,10 +301,16 @@ def acl_get(path, item, st, numeric_ids=False, fd=None):
             access_acl = acl_get_file(path, ACL_TYPE_ACCESS)
         if access_acl == NULL:
             raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
-        access_text = acl_to_any_text(access_acl, NULL, '\n', TEXT_NUMERIC_IDS)
-        if access_text == NULL:
-            raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
-        item['acl_access'] = converter(access_text)
+        # a directory having only a default ACL also counts as "extended" (see above), but its
+        # access ACL then just mirrors the traditional permission bits and thus carries no extra
+        # information. the kernel does not keep a system.posix_acl_access xattr in that case
+        # either, so do not archive such an access ACL. acl_equiv_mode returns 0 if the ACL is
+        # equivalent to the mode bits, 1 if it is not and -1 on error (then we rather store it).
+        if acl_equiv_mode(access_acl, NULL) != 0:
+            access_text = acl_to_any_text(access_acl, NULL, '\n', TEXT_NUMERIC_IDS)
+            if access_text == NULL:
+                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+            item['acl_access'] = converter(access_text)
     finally:
         acl_free(access_text)
         acl_free(access_acl)
@@ -311,10 +320,13 @@ def acl_get(path, item, st, numeric_ids=False, fd=None):
             default_acl = acl_get_file(path, ACL_TYPE_DEFAULT)
             if default_acl == NULL:
                 raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
-            default_text = acl_to_any_text(default_acl, NULL, '\n', TEXT_NUMERIC_IDS)
-            if default_text == NULL:
-                raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
-            item['acl_default'] = converter(default_text)
+            # a directory without a default ACL gives an empty ACL here - nothing to archive
+            # (same as above: the kernel has no system.posix_acl_default xattr for it).
+            if acl_entries(default_acl) > 0:
+                default_text = acl_to_any_text(default_acl, NULL, '\n', TEXT_NUMERIC_IDS)
+                if default_text == NULL:
+                    raise OSError(errno.errno, os.strerror(errno.errno), os.fsdecode(path))
+                item['acl_default'] = converter(default_text)
         finally:
             acl_free(default_text)
             acl_free(default_acl)

src/borg/testsuite/archiver/mount_cmds_test.py

@@ -189,16 +189,28 @@ def test_fuse_acls(archivers, request):
     dir_path = os.path.join(archiver.input_path, "dir1")
     os.mkdir(dir_path)
     platform.acl_set(dir_path, {"acl_access": access_acl, "acl_default": default_acl})
+    dir2_path = os.path.join(archiver.input_path, "dir2")
+    os.mkdir(dir2_path)
+    platform.acl_set(dir2_path, {"acl_default": default_acl})  # only a default ACL, no access ACL
     cmd(archiver, "create", "archive", "input")
     mountpoint = os.path.join(archiver.tmpdir, "mountpoint")
     with fuse_mount(archiver, mountpoint, "-a", "archive"):
         mounted_file = os.path.join(mountpoint, "archive", "input", "file1")
         mounted_dir = os.path.join(mountpoint, "archive", "input", "dir1")
+        mounted_dir2 = os.path.join(mountpoint, "archive", "input", "dir2")
         # the ACL xattrs must be listed:
         assert "system.posix_acl_access" in os.listxattr(mounted_file)
         assert "system.posix_acl_default" not in os.listxattr(mounted_file)
         assert "system.posix_acl_access" in os.listxattr(mounted_dir)
         assert "system.posix_acl_default" in os.listxattr(mounted_dir)
+        # a directory that only has a default ACL must not offer an access ACL xattr
+        # (its access ACL is just the mode bits, the source fs does not have that xattr either):
+        assert "system.posix_acl_access" not in os.listxattr(dir2_path)
+        assert "system.posix_acl_access" not in os.listxattr(mounted_dir2)
+        assert "system.posix_acl_default" in os.listxattr(mounted_dir2)
+        with pytest.raises(OSError) as exc_info:
+            os.getxattr(mounted_dir2, "system.posix_acl_access")
+        assert exc_info.value.errno in (errno.ENODATA, errno.ENOTSUP)
         # the binary xattr values must be identical to what the kernel provides for the source fs objects:
         try:
             mounted_file_acl = os.getxattr(mounted_file, "system.posix_acl_access")
@@ -212,6 +224,9 @@ def test_fuse_acls(archivers, request):
         assert mounted_file_acl == os.getxattr(file_path, "system.posix_acl_access")
         for name in ("system.posix_acl_access", "system.posix_acl_default"):
             assert os.getxattr(mounted_dir, name) == os.getxattr(dir_path, name)
+        assert os.getxattr(mounted_dir2, "system.posix_acl_default") == os.getxattr(
+            dir2_path, "system.posix_acl_default"
+        )
         # borg's own ACL code (going through libacl, like getfacl or tools copying
         # from the mount would) must see the same ACLs through the mount:
         item_src, item_mnt = {}, {}
@@ -223,6 +238,12 @@ def test_fuse_acls(archivers, request):
         platform.acl_get(mounted_dir, item_mnt, os.stat(mounted_dir))
         assert item_src["acl_access"] == item_mnt["acl_access"]
         assert item_src["acl_default"] == item_mnt["acl_default"]
+        item_src, item_mnt = {}, {}
+        platform.acl_get(dir2_path, item_src, os.stat(dir2_path))
+        platform.acl_get(mounted_dir2, item_mnt, os.stat(mounted_dir2))
+        assert "acl_access" not in item_src
+        assert "acl_access" not in item_mnt
+        assert item_src["acl_default"] == item_mnt["acl_default"]
     # also check with --numeric-ids:
     with fuse_mount(archiver, mountpoint, "-a", "archive", "--numeric-ids"):
         mounted_file = os.path.join(mountpoint, "archive", "input", "file1")

src/borg/testsuite/platform/freebsd_test.py

@@ -93,4 +93,26 @@ def test_default_acl():
         assert get_acl(tmpdir)["acl_default"] == DEFAULT_ACL
 
 
+@skipif_acls_not_working
+def test_default_acl_only():
+    # a directory can have a default ACL while its access ACL is just the traditional permission
+    # bits. acl_extended_link_np() only considers the access ACL, so borg must not rely on it
+    # alone for directories - otherwise the default ACL would not be archived at all.
+    with tempfile.TemporaryDirectory() as tmpdir:
+        set_acl(tmpdir, default=DEFAULT_ACL)
+        item = get_acl(tmpdir)
+        assert "acl_access" not in item
+        assert item["acl_default"] == DEFAULT_ACL
+
+
+@skipif_acls_not_working
+def test_access_acl_only_no_empty_default():
+    # a directory without a default ACL must not get an empty acl_default archived.
+    with tempfile.TemporaryDirectory() as tmpdir:
+        set_acl(tmpdir, access=ACCESS_ACL)
+        item = get_acl(tmpdir)
+        assert item["acl_access"] == ACCESS_ACL
+        assert "acl_default" not in item
+
+
 # nfs4 acls testing not implemented.

src/borg/testsuite/platform/linux_test.py

@@ -82,6 +82,41 @@ def test_default_acl():
         assert get_acl(tmpdir)["acl_default"] == DEFAULT_ACL
 
 
+@skipif_acls_not_working
+def test_default_acl_only():
+    # a directory can have a default ACL while its access ACL is just the traditional permission
+    # bits. such an access ACL must not be archived - it has no xattr on the source fs either.
+    with tempfile.TemporaryDirectory() as tmpdir:
+        set_acl(tmpdir, default=DEFAULT_ACL)
+        item = get_acl(tmpdir)
+        assert "acl_access" not in item
+        assert item["acl_default"] == DEFAULT_ACL
+
+
+@skipif_acls_not_working
+def test_access_acl_only_no_empty_default():
+    # a directory without a default ACL must not get an empty acl_default archived.
+    with tempfile.TemporaryDirectory() as tmpdir:
+        set_acl(tmpdir, access=ACCESS_ACL)
+        item = get_acl(tmpdir)
+        assert item["acl_access"] == ACCESS_ACL
+        assert "acl_default" not in item
+
+
+def test_acl_is_extended():
+    from ...platform import acl_is_extended
+
+    assert not acl_is_extended(None)
+    assert not acl_is_extended(b"")
+    # trivial ACLs (only the base entries) are equivalent to the mode bits:
+    assert not acl_is_extended(b"user::rw-\ngroup::r--\nother::r--")
+    assert not acl_is_extended(b"user::rw-\ngroup::r--\nother::r--\n")
+    # anything else is a real ACL:
+    assert acl_is_extended(b"user::rw-\ngroup::r--\nmask::rw-\nother::r--")
+    assert acl_is_extended(b"user::rw-\nuser:root:rw-:0\ngroup::r--\nother::r--")
+    assert acl_is_extended(b"user::rw-\ngroup::r--\ngroup:8888:rw-:8888\nother::r--")
+
+
 @skipif_acls_not_working
 @skipif_no_ubel_user
 def test_non_ascii_acl():