ostree commit: Add option --use-bare-user-xattrs

For our build-system we have a [patched version of `fakeroot`][1] which
understands the `user.ostreemeta` xattr.  It's intended for use with
`ostree checkout --user-mode --require-hardlinks`.  When programs are run
with `LD_PRELOAD=libfakeroot...` they see the ownership and permissions
that ostree understands, rather than the real ones.

Our build system mostly consists of calls to:

    ostree checkout --user-mode --require-hardlinks $COMMIT root
    LD_PRELOAD=fakeroot <make some changes>
    ostree commit --tree=dir=root --link-checkout-speedup --devino-canonical

in the past this seemed to work fine but at some point I started losing the
setuid bit on `sudo`.  I suspect the previous behaviour was a bug, possibly
fixed in ostreedev#1170 (8fe4536).  This commit makes the `ostreemeta` preserving
behaviour explicit.

If the user.ostreemeta xattr doesn't exist the file is given the sensible
default uid/gid of 0:0.

`--use-bare-user-xattrs` will currently be ignored when loading a commit
from a tarball.  This can be fixed in the future if there's any demand for
it.

[1]: https://github.com/stb-tester/fakeroot/tree/ostree

ci/installdeps.sh

@@ -18,7 +18,7 @@ pkg_upgrade
 pkg_install_buildroot
 pkg_builddep ostree
 pkg_install sudo which attr fuse strace \
-    libubsan libasan libtsan PyYAML redhat-rpm-config \
+    libubsan libasan libtsan PyYAML pyxattr redhat-rpm-config \
     elfutils
 if test -n "${CI_PKGS:-}"; then
     pkg_install ${CI_PKGS}

ci/travis-install.sh

@@ -113,6 +113,7 @@ case "$ci_distro" in
             libsoup2.4-dev \
             libcurl4-openssl-dev \
             procps \
+            python-xattr \
             zlib1g-dev \
             python3-yaml \
             ${ci_pkgs:-} \

src/libostree/ostree-repo-commit.c

@@ -3207,6 +3207,69 @@ create_tree_variant_from_hashes (GHashTable            *file_checksums,
   return g_variant_ref_sink (serialized_tree);
 }
 
+/* v is a GVariant of type "a(ayay)" representing a list of xattrs */
+static GBytes*
+variant_xattr_lookup(GVariant *v, const gchar *key)
+{
+  GVariantIter iter;
+  g_variant_iter_init (&iter, v);
+  while (TRUE)
+    {
+      gchar *k;
+      g_autoptr(GVariant) v = NULL;
+
+      if (!g_variant_iter_next (&iter, "(^&ay@ay)", &k, &v))
+        return NULL;
+      if (g_strcmp0 (k, key) == 0)
+        return g_variant_get_data_as_bytes (v);
+    }
+}
+
+static gboolean
+load_bare_user_xattrs (GVariant                 *original_xattrs,
+                       GFileInfo                *file_info,
+                       GFileInfo               **out_modified_info,
+                       GVariant                **out_xattrs,
+                       GCancellable             *cancellable,
+                       GError                  **error)
+{
+  struct stat current_stat;
+  _ostree_gfileinfo_to_stbuf (file_info, &current_stat);
+
+  struct stat new_stat;
+  g_autoptr(GVariant) ret_xattrs = NULL;
+
+  g_autoptr(GBytes) bytes = variant_xattr_lookup (original_xattrs, "user.ostreemeta");
+  if (bytes == NULL)
+    {
+      new_stat = current_stat;
+      new_stat.st_uid = 0;
+      new_stat.st_gid = 0;
+    }
+  else
+    ret_xattrs = _ostree_filemeta_to_stat (&new_stat, bytes);
+
+  new_stat.st_mode = (current_stat.st_mode & S_IFMT) | (new_stat.st_mode & 07777);
+
+  if (new_stat.st_uid != current_stat.st_uid ||
+      new_stat.st_gid != current_stat.st_gid ||
+      new_stat.st_mode != current_stat.st_mode)
+    {
+      g_autoptr(GFileInfo) modified_info = g_file_info_dup (file_info);
+
+      g_file_info_set_attribute_uint32 (modified_info, "unix::uid", new_stat.st_uid);
+      g_file_info_set_attribute_uint32 (modified_info, "unix::gid", new_stat.st_gid);
+      g_file_info_set_attribute_uint32 (modified_info, "unix::mode", new_stat.st_mode);
+
+      g_set_object (out_modified_info, modified_info);
+    }
+  else
+    g_set_object (out_modified_info, g_object_ref (file_info));
+
+  *out_xattrs = g_steal_pointer (&ret_xattrs);
+  return TRUE;
+}
+
 /* If any filtering is set up, perform it, and return modified file info in
  * @out_modified_info. Note that if no filtering is applied, @out_modified_info
  * will simply be another reference (with incremented refcount) to @file_info.
@@ -3225,7 +3288,7 @@ _ostree_repo_commit_modifier_apply (OstreeRepo               *self,
       (modifier->filter == NULL &&
        (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS) == 0))
     {
-      *out_modified_info = g_object_ref (file_info);
+      g_set_object (out_modified_info, file_info);
       return OSTREE_REPO_COMMIT_FILTER_ALLOW;
     }
 
@@ -3257,7 +3320,7 @@ _ostree_repo_commit_modifier_apply (OstreeRepo               *self,
       g_file_info_set_attribute_uint32 (modified_info, "unix::gid", 0);
     }
 
-  *out_modified_info = modified_info;
+  g_set_object (out_modified_info, modified_info);
 
   return result;
 }
@@ -3293,6 +3356,7 @@ get_final_xattrs (OstreeRepo                       *self,
                   int                               dfd,
                   const char                       *dfd_subpath,
                   GVariant                         *source_xattrs,
+                  GFileInfo                       **out_fileinfo,
                   GVariant                        **out_xattrs,
                   gboolean                         *out_modified,
                   GCancellable                     *cancellable,
@@ -3303,10 +3367,13 @@ get_final_xattrs (OstreeRepo                       *self,
   const gboolean skip_xattrs = (modifier &&
       modifier->flags & (OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS |
                          OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS)) > 0;
+  const gboolean use_bare_user_xattrs = (modifier &&
+      modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_USE_BARE_USER_XATTRS);
 
   /* fetch on-disk xattrs if needed & not disabled */
   g_autoptr(GVariant) original_xattrs = NULL;
-  if (!skip_xattrs && !self->disable_xattrs)
+  g_autoptr(GVariant) ret_xattrs = NULL;
+  if ((!skip_xattrs && !self->disable_xattrs) || use_bare_user_xattrs)
     {
       if (source_xattrs)
         original_xattrs = g_variant_ref (source_xattrs);
@@ -3337,18 +3404,34 @@ get_final_xattrs (OstreeRepo                       *self,
         }
 
       g_assert (original_xattrs);
+      ret_xattrs = g_variant_ref (original_xattrs);
     }
 
-  g_autoptr(GVariant) ret_xattrs = NULL;
-  if (modifier && modifier->xattr_callback)
+  g_autoptr(GFileInfo) ret_fileinfo = NULL;
+  if (use_bare_user_xattrs)
     {
-      ret_xattrs = modifier->xattr_callback (self, relpath, file_info,
-                                             modifier->xattr_user_data);
+      if (!load_bare_user_xattrs (ret_xattrs, file_info,
+                                  &ret_fileinfo, &ret_xattrs,
+                                  cancellable, error))
+        return FALSE;
     }
+  else
+    ret_fileinfo = g_object_ref (file_info);
 
-  /* if callback returned NULL or didn't exist, default to on-disk state */
-  if (!ret_xattrs && original_xattrs)
-    ret_xattrs = g_variant_ref (original_xattrs);
+  if (modifier && modifier->xattr_callback)
+    {
+      // The file_info passed to the xattr_callback hasn't been updated with the
+      // new xattrs from load_bare_user_xattrs.  The exising callbacks don't
+      // read xattrs from file_info anyway, so this is fine.
+      GVariant *cb_xattrs = modifier->xattr_callback (
+          self, relpath, ret_fileinfo, modifier->xattr_user_data);
+      if (cb_xattrs)
+        {
+          /* if callback returned NULL default to on-disk state */
+          g_variant_unref (ret_xattrs);
+          ret_xattrs = cb_xattrs;
+        }
+    }
 
   if (modifier && modifier->sepolicy)
     {
@@ -3392,11 +3475,14 @@ get_final_xattrs (OstreeRepo                       *self,
         }
     }
 
-  if (original_xattrs && ret_xattrs && g_variant_equal (original_xattrs, ret_xattrs))
+  if (original_xattrs && ret_xattrs && g_variant_equal (original_xattrs, ret_xattrs) &&
+      _ostree_gfileinfo_equal(ret_fileinfo, file_info))
     modified = FALSE;
 
   if (out_xattrs)
     *out_xattrs = g_steal_pointer (&ret_xattrs);
+  if (out_fileinfo)
+    g_set_object (out_fileinfo, g_steal_pointer (&ret_fileinfo));
   if (out_modified)
     *out_modified = modified;
   return TRUE;
@@ -3612,7 +3698,6 @@ write_content_to_mtree_internal (OstreeRepo                  *self,
   g_autoptr(GFileInfo) modified_info = NULL;
   OstreeRepoCommitFilterResult filter_result =
     _ostree_repo_commit_modifier_apply (self, modifier, child_relpath, child_info, &modified_info);
-  const gboolean child_info_was_modified = !_ostree_gfileinfo_equal (child_info, modified_info);
 
   if (filter_result != OSTREE_REPO_COMMIT_FILTER_ALLOW)
     {
@@ -3660,9 +3745,9 @@ write_content_to_mtree_internal (OstreeRepo                  *self,
   gboolean xattrs_were_modified;
   if (dir_enum != NULL)
     {
-      if (!get_final_xattrs (self, modifier, child_relpath, child_info, child,
-                             -1, name, source_xattrs, &xattrs, &xattrs_were_modified,
-                             cancellable, error))
+      if (!get_final_xattrs (self, modifier, child_relpath, modified_info, child,
+                             -1, name, source_xattrs, &modified_info, &xattrs,
+                             &xattrs_were_modified, cancellable, error))
         return FALSE;
     }
   else
@@ -3672,15 +3757,17 @@ write_content_to_mtree_internal (OstreeRepo                  *self,
        */
       int xattr_fd_arg = (file_input_fd != -1) ? file_input_fd : dfd_iter->fd;
       const char *xattr_path_arg = (file_input_fd != -1) ? NULL : name;
-      if (!get_final_xattrs (self, modifier, child_relpath, child_info, child,
+      if (!get_final_xattrs (self, modifier, child_relpath, modified_info, child,
                              xattr_fd_arg, xattr_path_arg, source_xattrs,
-                             &xattrs, &xattrs_were_modified,
+                             &modified_info, &xattrs, &xattrs_were_modified,
                              cancellable, error))
         return FALSE;
     }
 
   /* Used below to see whether we can do a fast path commit */
-  const gboolean modified_file_meta = child_info_was_modified || xattrs_were_modified;
+  const gboolean modified_file_meta =
+      !_ostree_gfileinfo_equal (child_info, modified_info) ||
+      xattrs_were_modified;
 
   /* A big prerequisite list of conditions for whether or not we can
    * "adopt", i.e. just checksum and rename() into place
@@ -3818,6 +3905,7 @@ write_directory_to_mtree_internal (OstreeRepo                  *self,
     }
   else
     {
+      /* This is a real file */
       g_autoptr(GVariant) xattrs = NULL;
 
       g_autoptr(GFileInfo) child_info =
@@ -3836,8 +3924,8 @@ write_directory_to_mtree_internal (OstreeRepo                  *self,
 
       if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW)
         {
-          if (!get_final_xattrs (self, modifier, relpath, child_info, dir, -1, NULL,
-                                 NULL, &xattrs, NULL, cancellable, error))
+          if (!get_final_xattrs (self, modifier, relpath, modified_info, dir, -1, NULL,
+                                 NULL, &modified_info, &xattrs, NULL, cancellable, error))
             return FALSE;
 
           g_autofree guchar *child_file_csum = NULL;
@@ -3935,7 +4023,8 @@ write_dfd_iter_to_mtree_internal (OstreeRepo                  *self,
   if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW)
     {
       if (!get_final_xattrs (self, modifier, relpath, modified_info, NULL, src_dfd_iter->fd,
-                             NULL, NULL, &xattrs, NULL, cancellable, error))
+                             NULL, NULL, &modified_info, &xattrs,
+                             NULL, cancellable, error))
         return FALSE;
 
       if (!_ostree_repo_write_directory_meta (self, modified_info, xattrs, &child_file_csum,

src/libostree/ostree-repo.h

@@ -649,6 +649,7 @@ typedef OstreeRepoCommitFilterResult (*OstreeRepoCommitFilter) (OstreeRepo    *r
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_ERROR_ON_UNLABELED: Emit an error if configured SELinux policy does not provide a label
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME: Delete added files/directories after commit; Since: 2017.13
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL: If a devino cache hit is found, skip modifier filters (non-directories only); Since: 2017.14
+ * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_USE_BARE_USER_XATTRS: Get uid, gid, mode and xattrs from the user.ostreemeta xattr; Since: XXX
  */
 typedef enum {
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_NONE = 0,
@@ -658,6 +659,7 @@ typedef enum {
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_ERROR_ON_UNLABELED = (1 << 3),
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME = (1 << 4),
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL = (1 << 5),
+  OSTREE_REPO_COMMIT_MODIFIER_FLAGS_USE_BARE_USER_XATTRS = (1 << 6),
 } OstreeRepoCommitModifierFlags;
 
 /**

src/ostree/ot-builtin-commit.c

@@ -53,6 +53,7 @@ static gboolean opt_skip_if_unchanged;
 static gboolean opt_tar_autocreate_parents;
 static char *opt_tar_pathname_filter;
 static gboolean opt_no_xattrs;
+static gboolean opt_use_bare_user_xattrs;
 static char *opt_selinux_policy;
 static gboolean opt_selinux_policy_from_base;
 static gboolean opt_canonical_permissions;
@@ -114,6 +115,7 @@ static GOptionEntry options[] = {
   { "canonical-permissions", 0, 0, G_OPTION_ARG_NONE, &opt_canonical_permissions, "Canonicalize permissions in the same way bare-user does for hardlinked files", NULL },
   { "mode-ro-executables", 0, 0, G_OPTION_ARG_NONE, &opt_ro_executables, "Ensure executable files are not writable", NULL },
   { "no-xattrs", 0, 0, G_OPTION_ARG_NONE, &opt_no_xattrs, "Do not import extended attributes", NULL },
+  { "use-bare-user-xattrs", 0, 0, G_OPTION_ARG_NONE, &opt_use_bare_user_xattrs, "Set uid, gid, mode and xattrs according to the user.ostreemeta xattr on files", NULL },
   { "selinux-policy", 0, 0, G_OPTION_ARG_FILENAME, &opt_selinux_policy, "Set SELinux labels based on policy in root filesystem PATH (may be /)", "PATH" },
   { "selinux-policy-from-base", 'P', 0, G_OPTION_ARG_NONE, &opt_selinux_policy_from_base, "Set SELinux labels based on first --tree argument", NULL },
   { "link-checkout-speedup", 0, 0, G_OPTION_ARG_NONE, &opt_link_checkout_speedup, "Optimize for commits of trees composed of hardlinks into the repository", NULL },
@@ -555,6 +557,8 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio
 
   if (opt_no_xattrs)
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS;
+  if (opt_use_bare_user_xattrs)
+    flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_USE_BARE_USER_XATTRS;
   if (opt_consume)
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME;
   if (opt_devino_canonical)
@@ -580,6 +584,7 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio
       || opt_statoverride_file != NULL
       || opt_skiplist_file != NULL
       || opt_no_xattrs
+      || opt_use_bare_user_xattrs
       || opt_ro_executables
       || opt_selinux_policy
       || opt_selinux_policy_from_base)

tests/test-basic-user.sh

@@ -28,7 +28,7 @@ skip_without_user_xattrs
 mode="bare-user"
 setup_test_repository "$mode"
 
-extra_basic_tests=6
+extra_basic_tests=7
 . $(dirname $0)/basic-test.sh
 
 # Reset things so we don't inherit a lot of state from earlier tests
@@ -125,3 +125,46 @@ assert_file_has_content out.txt "Content Cache Hits: [1-9][0-9]*"
 
 $OSTREE refs --delete test2-{linkcheckout,devino}-test
 echo "ok commit with -I"
+
+mkdir -p xattrroot/bin xattrroot/home/ostree
+touch xattrroot/bin/sudo xattrroot/bin/bash
+
+cat >setx.py <<'EOF'
+#/usr/bin/python
+import os
+import struct
+import sys
+
+import xattr
+
+
+def setx(filename, uid, gid, mode):
+    type_bits = os.stat(filename).st_mode & 0o170000
+    return xattr.setxattr(filename, "user.ostreemeta",
+                          struct.pack('>III', uid, gid, mode | type_bits))
+
+setx("xattrroot", 0, 0, 0o0755)
+setx("xattrroot/bin", 0, 0, 0o0755)
+setx("xattrroot/bin/bash", 0, 0, 0o0755)
+setx("xattrroot/bin/sudo", 0, 0, 0o4755)
+setx("xattrroot/home", 0, 0, 0o0755)
+setx("xattrroot/home/ostree", 1001, 80, 0o0700)
+EOF
+python setx.py
+
+$OSTREE commit -b xattrtest --tree=dir=xattrroot \
+               --link-checkout-speedup --consume \
+               --use-bare-user-xattrs
+$OSTREE ls -R xattrtest >out
+cat >expected <<EOF
+d00755 0 0      0 /
+d00755 0 0      0 /bin
+-00755 0 0      0 /bin/bash
+-04755 0 0      0 /bin/sudo
+d00755 0 0      0 /home
+d00700 1001 80      0 /home/ostree
+EOF
+
+diff -u expected out || fatal "Tree contents incorrect"
+
+echo "ok commit with --use-bare-user-xattrs"