PR #1668: bucache: save and restore xattrs, ACLs

bin/ch-completion.bash

@@ -112,9 +112,9 @@ _image_build_opts="-b --bind --build-arg -f --file --force
 
 _image_common_opts="-a --arch --always-download --auth --cache
                     --cache-large --dependencies -h --help
-                    --no-cache --no-lock --profile --rebuild
-                    --password-many -s --storage --tls-no-verify
-                    -v --verbose --version"
+                    --no-cache --no-lock --no-xattrs --profile
+                    --rebuild --password-many -s --storage
+                    --tls-no-verify -v --verbose --version"
 
 _image_subcommands="build build-cache delete gestalt
                     import list pull push reset undelete"

bin/ch-convert

@@ -50,7 +50,8 @@ cv_chimage_dir () {
     # (2) it allows a progress meter (issue #1332). A very quick, very dirty,
     # completely half-assed performance test suggests that's about the same.
     dir_make "$2"
-    tar_ "$img" | tar xf - -pC "$2"
+    # shellcheck disable=SC2086
+    tar_ "$img" | tar xf - $tar_xattr_args -pC "$2"
     dir_fixup "$2"
 }
 
@@ -111,7 +112,8 @@ cv_dir_squash () {
     mount_points_ensure "$1" "$pflist"
     # Exclude build cache metadata. 64kiB block size based on Shane's
     # experiments.
-    mksquashfs "$1" "$2" -b 65536 -noappend -all-root -pf "$pflist" \
+    # shellcheck disable=SC2086
+    mksquashfs "$1" "$2" $squash_xattr_arg -b 65536 -noappend -all-root -pf "$pflist" \
                          -e "$1"/.git -e "$1"/.gitignore -e "$1"/ch/git.pickle
     # Zero the archive's internal modification time at bytes 8–11, 0-indexed
     # [1]. Newer SquashFS-Tools ≥4.3 have option “-fstime 0” to do this, but
@@ -313,11 +315,11 @@ cv_tar_dir () {
     ex2="/${ex1}"
     ex3="./${ex1}"
     VERBOSE "exclude patterns: ${ex1} ${ex2} ${ex3}"
-    #shellcheck disable=SC2094
+    #shellcheck disable=SC2094,SC2086
       pv_ -s "$(stat -c%s "$1")" < "$1" \
     | tar x"$(tar_decompress_arg "$1")" -pC "$2" -f - \
           --xform 's|^\./||x' --strip-components=$strip_ct \
-          --anchored --no-wildcards-match-slash \
+          --anchored --no-wildcards-match-slash $tar_xattr_args \
           --exclude="$ex1" --exclude="$ex2" --exclude="$ex3"
     dir_fixup "$2"
 }
@@ -422,7 +424,8 @@ dockman_to_tar () {
             --format='{{range .Config.Env}}{{println .}}{{end}}' > "$tmpenv"
     # The tar flavor Docker gives us does not support UIDs or GIDs greater
     # than 2**21, so use 0/0 rather than what’s on the filesystem. See #1573.
-    tar rf "$tmptar" -b1 -P --owner=0 --group=0 \
+    # shellcheck disable=SC2086
+    tar rf "$tmptar" -b1 -P --owner=0 --group=0 $tar_xattr_args \
         --xform="s|${tmpenv}|ch/environment|" "$tmpenv"
     INFO 'compressing ...'
     pv_ < "$tmptar" | gzip_ -6 > "$3"
@@ -707,7 +710,9 @@ path_noclobber () {
 # Tar $1 and emit the result on stdout, excluding build cache metadata.
 # Produce a tarbomb because Docker requires tarbombs.
 tar_ () {
-    ( cd "$1" && tar cf - --exclude=./.git \
+    # shellcheck disable=SC2086
+    ( cd "$1" && tar cf - $tar_xattr_args \
+                          --exclude=./.git \
                           --exclude=./.gitignore \
                           --exclude=./ch/git.pickle . ) | pv_
 }
@@ -832,6 +837,9 @@ while true; do
             --no-clobber)
                 no_clobber=yes
                 ;;
+            --no-xattrs)
+                no_xattrs=yes
+                ;;
             -o|--out-fmt)
                 shift
                 out_fmt=$1
@@ -857,6 +865,13 @@ done
 if [ "$#" -ne 2 ]; then
     usage
 fi
+if [ -n "$no_xattrs" ]; then
+    tar_xattr_args=
+    squash_xattr_arg=
+else
+    tar_xattr_args='--xattrs-include=user.* --xattrs-include=system.*'
+    squash_xattr_arg=-xattrs
+fi
 in_desc=$1
 out_desc=$2
 VERBOSE "verbose level: ${verbose}"

bin/ch-image.py.in

@@ -122,6 +122,9 @@ def main():
            [["--no-lock"],
             { "action": "store_true",
               "help": "allow concurrent storage directory access (risky!)" }],
+           [["--no-xattrs"],
+            { "action": "store_true",
+              "help": "build cache ignores xattrs and ACLs"}],
            [["--password-many"],
             { "action": "store_true",
               "help": "re-prompt each time a registry password is needed" }],

bin/ch-run-oci.py.in

@@ -94,6 +94,7 @@ def args_parse():
    args_.profile = False
    args_.storage = None
    args_.tls_no_verify = False
+   args_.no_xattrs = False
    ch.init(args_)
 
    if len(sys.argv) < 2:

doc/ch-convert.rst

@@ -49,6 +49,9 @@ producing the final format actually needed.
   :code:`--no-clobber`
     Error if :code:`OUT` already exists, rather than replacing it.
 
+  :code:`--no-xattrs`
+    Ignore xattrs and ACLs when converting.
+
   :code:`-o`, :code:`--out-fmt FMT`
     Output image format is :code:`FMT`; inferred if omitted.
 

doc/ch-image.rst

@@ -85,6 +85,9 @@ Common options placed before or after the sub-command:
     :code:`ch-image` instances as you want against the same storage directory,
     which risks corruption but may be OK for some workloads.
 
+  :code:`--no-xattrs`
+    Ignore xattrs and ACLs.
+
   :code:`--profile`
     Dump profile to files :code:`/tmp/chofile.p` (:code:`cProfile` dump
     format) and :code:`/tmp/chofile.txt` (text summary). You can convert the
@@ -295,6 +298,12 @@ repositories within the image should work. **Important exception**: No files
 named :code:`.git*` or other Git metadata are permitted in the image’s root
 directory.
 
+`Extended attributes <https://man7.org/linux/man-pages/man7/xattr.7.html>`_ (xattrs)
+belonging to unprivileged namespaces (e.g. :code:`user`) are also saved and
+restored by the cache by default. Notably, extended attributes in privileged
+namespaces (e.g. :code:`trusted`) cannot be read by :code:`ch-image` and will be
+lost without warning.
+
 The cache has three modes, *enabled*, *disabled*, and a hybrid mode called
 *rebuild* where the cache is fully enabled for :code:`FROM` instructions, but
 all other operations re-execute and re-cache their results. The purpose of

lib/build_cache.py

@@ -241,6 +241,13 @@ class File_Metadata:
    #   hardlink_to ... If non-None, file is a hard link to this other file,
    #                   stored as a Path object relative to the image root.
    #
+   #   xattrs ........ Extended attributes of the file, stored as a dictionary.
+   #                   The keys take the form “namespace.name”, where
+   #                   “namespace” is the namespace the xattr belongs to (e.g.
+   #                   “user” or “system”) and “name” is the actual name of the
+   #                   xattr. The value in the dictionary is the value assigned
+   #                   to the xattr.
+   #
    # Attributes not stored (recomputed on unpickle):
    #
    #   image_root .... Absolute path to the image directory to which path is
@@ -267,6 +274,10 @@ def __init__(self, image_root, path):
       self.dont_restore = False
       self.hardlink_to = None
       self.large_name = None
+      self.xattrs = dict()
+      if ch.save_xattrs:
+         for xattr in os.listxattr(self.path_abs, follow_symlinks=False):
+            self.xattrs[xattr] = os.getxattr(self.path_abs, xattr, follow_symlinks=False)
 
    def __getstate__(self):
       return { a:v for (a,v) in self.__dict__.items()
@@ -470,6 +481,10 @@ def git_restore(self, quick):
          ch.ossafe(os.mkfifo, "can’t make FIFO: %s" % self.path, self.path_abs)
       elif (self.path.git_incompatible_p):
          self.path_abs.git_escaped.rename_(self.path_abs)
+      if ch.save_xattrs:
+         for (xattr, val) in self.xattrs.items():
+            ch.ossafe(os.setxattr, "unable to restore xattr: %s" % xattr,
+                      self.path_abs, xattr, val, follow_symlinks=False)
       # Recurse children.
       if (len(self.children) > 0):
          for child in self.children.values():
@@ -546,11 +561,13 @@ def str_for_log(self):
 
    def unpickle_fix(self, image_root, path):
       "Does no I/O."
-      # old: large_name, size: no such attribute
+      # old: large_name, size, xattrs: no such attribute
       if (not (hasattr(self, "large_name"))):
          self.large_name = None
       if (not (hasattr(self, "size"))):
          self.size = -1
+      if (not (hasattr(self, "xattrs"))):
+         self.xattrs = dict()
       # old: hardlink_to: stored as string
       if (isinstance(self.hardlink_to, str)):
          self.hardlink_to = fs.Path(self.hardlink_to)

lib/charliecloud.py

@@ -602,9 +602,10 @@ def exit(code):
 
 def init(cli):
    # logging
-   global log_festoon, log_fp, trace_fatal, verbose
+   global log_festoon, log_fp, trace_fatal, verbose, save_xattrs
    assert (0 <= cli.verbose <= 3)
    verbose = cli.verbose
+   save_xattrs = (not cli.no_xattrs)
    trace_fatal = (cli.debug or bool(os.environ.get("CH_IMAGE_DEBUG", False)))
    if ("CH_LOG_FESTOON" in os.environ):
       log_festoon = True

test/build/55_cache.bats

@@ -1399,3 +1399,33 @@ EOF
     [[ $output = *'deleting, see issue #1351: var/lib/rpm/__db.001'* ]]
     [[ ! -e $CH_IMAGE_STORAGE/img/tmpimg/var/lib/rpm/__db.001 ]]
 }
+
+@test "${tag}: restore ACLs, xattrs" {  # issue #1287
+    # Build an image, then re-build from cache to test xattr/ACL cache
+    # functionality.
+    TMP_CX=$BATS_TMPDIR
+    TMP_DF=$BATS_TMPDIR/weirdal.df
+    cat <<'EOF' > "$TMP_DF"
+FROM alpine:3.17
+RUN apk add attr
+RUN apk add acl
+RUN touch /home/foo
+RUN setfattr -n user.foo -v bar /home/foo
+RUN setfacl -m u:root:r /home/foo
+EOF
+    ch-image build-cache --reset
+    ch-image build -t tmpimg -f "$TMP_DF" "$TMP_CX"
+    ch-image delete tmpimg
+    ch-image build -t tmpimg -f "$TMP_DF" "$TMP_CX"
+    run ch-run tmpimg -- getfattr home/foo
+    # don’t check for ACL xattr bc it’s more straightforward to use getfacl(1).
+    echo "$output"
+    [[ $status -eq 0 ]]
+    [[ $output = *'# file: home/foo'* ]]
+    [[ $output = *'user.foo'* ]]
+
+    run ch-run tmpimg -- getfacl home/foo
+    echo "$output"
+    [[ $status -eq 0 ]]
+    [[ $output = *"user:$USER:r--"* ]]
+}

test/make-auto.d/build_custom.bats.in

@@ -18,7 +18,21 @@ source common.bash  # for ShellCheck; removed by ch-test
             tarball=${tarballs[0]}
             # Convert to SquashFS if needed.
             if [[ $CH_TEST_PACK_FMT = squash* ]]; then
-                ch-convert "$tarball" "${tarball/tar.?z/sqfs}"
+                # With the centos7xz image, we run into permission errors if we
+                # try to use the tar “--xattrs-include” option. Using strace(1),
+                # we determined that with the xattrs option specified, tar first
+                # calls mknodat(2) to create a file with permissions 000, then
+                # openat(2) on the same file, which fails with EACCESS. Without
+                # the xattrs option, the file is created by a call to openat(2)
+                # with the O_CREAT flag (rather than mknodat(2)), so the
+                # permission error is avoided. (See
+                # https://savannah.gnu.org/support/index.php?110903).
+                if [[ $tarball = *centos7xz* ]]; then
+                    xattrs_arg=--no-xattrs
+                else
+                    xattrs_arg=
+                fi
+                ch-convert $xattrs_arg "$tarball" "${tarball/tar.?z/sqfs}"
                 rm "$tarball"
             fi
         elif [[ -d $out ]]; then                             # directory

test/run/ch-convert.bats

@@ -493,6 +493,70 @@ test_from () {
     [[ $(stat -c %a "${out}/maxperms_file") = 777 ]]
 }
 
+@test 'ch-convert: b0rked xattrs' {
+    # b0rked: (adj) broken, messed up
+    #
+    # In this test, we create a tarball with “unusual” xattrs that we don’t want
+    # to restore (i.e. a borked tarball), and try to convert it into a ch-image.
+    [[ -n $CH_TEST_SUDO ]] || skip 'sudo required'
+
+    cd "$BATS_TMPDIR"
+
+    borked_img="borked_image"
+    borked_file="${borked_img}/home/foo"
+    borked_tar="borked.tgz"
+    borked_out="borked_dir"
+
+    rm -rf "$borked_img" "$borked_tar" "$borked_out"
+
+    ch-image build -t tmpimg - <<'EOF'
+FROM alpine:3.17
+RUN touch /home/foo
+EOF
+
+    # convert image to dir and actually bork it
+    ch-convert -i ch-image -o dir tmpimg "$borked_img"
+    setfattr -n user.foo -v bar "$borked_file"
+    sudo setfattr -n security.foo -v bar "$borked_file"
+    sudo setfattr -n trusted.foo -v bar "$borked_file"
+    setfacl -m "u:$USER:r" "$borked_file"
+
+    # confirm it worked
+    run sudo getfattr -dm - -- "$borked_file"
+    echo "$output"
+    [[ $status -eq 0 ]]
+    [[ $output = *"# file: $borked_file"* ]]
+    [[ $output = *'security.foo="bar"'* ]]
+    [[ $output = *'trusted.foo="bar"'* ]]
+    [[ $output = *'user.foo="bar"'* ]]
+
+    run getfacl "$borked_file"
+    echo "$output"
+    [[ $status -eq 0 ]]
+    [[ $output = *"user:$USER:r--"* ]]
+
+    # tar it up
+    sudo tar --xattrs-include='user.*' \
+             --xattrs-include='system.*' \
+             --xattrs-include='security.*' \
+             --xattrs-include='trusted.*' \
+             -czvf "$borked_tar" "$borked_img"
+
+    ch-convert -i tar -o dir "$borked_tar" "$borked_out"
+
+    run sudo getfattr -dm - -- "$borked_out/home/foo"
+    echo "$output"
+    [[ $status -eq 0 ]]
+    [[ $output != *'security.foo="bar"'* ]]
+    [[ $output != *'trusted.foo="bar"'* ]]
+    [[ $output = *'user.foo="bar"'* ]]
+
+    run getfacl "$borked_out/home/foo"
+    echo "$output"
+    [[ $status -eq 0 ]]
+    [[ $output = *"user:$USER:r--"* ]]
+}
+
 
 @test 'ch-convert: dir -> ch-image -> X' {
     test_from ch-image