Merge pull request #15119 from obsidiansystems/canonicalize-pmd-options

libstore: introduce `CanonicalizePathMetadataOptions` for `canonicalisePathMetaData`

src/libstore/include/nix/store/posix-fs-canonicalise.hh

@@ -6,12 +6,47 @@
 
 #include "nix/util/types.hh"
 #include "nix/util/error.hh"
+#include "nix/store/config.hh"
 
 namespace nix {
 
 typedef std::pair<dev_t, ino_t> Inode;
 typedef std::set<Inode> InodesSeen;
 
+struct CanonicalizePathMetadataOptions
+{
+#ifndef _WIN32
+    /**
+     * If uidRange is not empty, this function will throw an error if it
+     * encounters files owned by a user outside of the closed interval
+     * [uidRange->first, uidRange->second].
+     */
+    std::optional<std::pair<uid_t, uid_t>> uidRange;
+#endif
+
+#if NIX_SUPPORT_ACL
+    /**
+     * A list of ACLs that should be ignored when canonicalising.
+     * Normally Nix attempts to remove all ACLs from files and directories
+     * in the Nix store, but some ACLs like `security.selinux` or
+     * `system.nfs4_acl` can't be removed even by root.
+     */
+    const StringSet & ignoredAcls;
+#endif
+};
+
+/**
+ * Makes it easier to cope with conditionally-available fields.
+ *
+ * @todo Switch to a better way, as having a macro is not the nicest.
+ * This will be easier to do after further settings refactors.
+ */
+#if NIX_SUPPORT_ACL
+#  define NIX_WHEN_SUPPORT_ACLS(ARG) .ignoredAcls = ARG,
+#else
+#  define NIX_WHEN_SUPPORT_ACLS(ARG)
+#endif
+
 /**
  * "Fix", or canonicalise, the meta-data of the files in a store path
  * after it has been built.  In particular:
@@ -24,25 +59,10 @@ typedef std::set<Inode> InodesSeen;
  *
  * - the owner and group are set to the Nix user and group, if we're
  *   running as root. (Unix only.)
- *
- * If uidRange is not empty, this function will throw an error if it
- * encounters files owned by a user outside of the closed interval
- * [uidRange->first, uidRange->second].
  */
-void canonicalisePathMetaData(
-    const Path & path,
-#ifndef _WIN32
-    std::optional<std::pair<uid_t, uid_t>> uidRange,
-#endif
-    InodesSeen & inodesSeen);
+void canonicalisePathMetaData(const Path & path, CanonicalizePathMetadataOptions options, InodesSeen & inodesSeen);
 
-void canonicalisePathMetaData(
-    const Path & path
-#ifndef _WIN32
-    ,
-    std::optional<std::pair<uid_t, uid_t>> uidRange = std::nullopt
-#endif
-);
+void canonicalisePathMetaData(const Path & path, CanonicalizePathMetadataOptions options);
 
 void canonicaliseTimestampAndPermissions(const Path & path);
 

src/libstore/local-store.cc

@@ -1083,7 +1083,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairF
 
                 autoGC();
 
-                canonicalisePathMetaData(realPath);
+                canonicalisePathMetaData(realPath, {NIX_WHEN_SUPPORT_ACLS(settings.ignoredAcls)});
 
                 optimisePath(realPath, repair); // FIXME: combine with hashPath()
 
@@ -1243,7 +1243,8 @@ StorePath LocalStore::addToStoreFromDump(
                 narHash = narSink.finish();
             }
 
-            canonicalisePathMetaData(realPath); // FIXME: merge into restorePath
+            canonicalisePathMetaData(
+                realPath, {NIX_WHEN_SUPPORT_ACLS(settings.ignoredAcls)}); // FIXME: merge into restorePath
 
             optimisePath(realPath, repair);
 

src/libstore/posix-fs-canonicalise.cc

@@ -1,10 +1,10 @@
 #include "nix/store/posix-fs-canonicalise.hh"
+#include "nix/store/build-result.hh"
 #include "nix/util/file-system.hh"
 #include "nix/util/signals.hh"
 #include "nix/util/util.hh"
-#include "nix/store/globals.hh"
 #include "nix/store/store-api.hh"
-
+#include "nix/store/globals.hh"
 #include "store-config-private.hh"
 
 #if NIX_SUPPORT_ACL
@@ -41,12 +41,8 @@ void canonicaliseTimestampAndPermissions(const Path & path)
     canonicaliseTimestampAndPermissions(path, lstat(path));
 }
 
-static void canonicalisePathMetaData_(
-    const Path & path,
-#ifndef _WIN32
-    std::optional<std::pair<uid_t, uid_t>> uidRange,
-#endif
-    InodesSeen & inodesSeen)
+static void
+canonicalisePathMetaData_(const Path & path, CanonicalizePathMetadataOptions options, InodesSeen & inodesSeen)
 {
     checkInterrupt();
 
@@ -80,7 +76,7 @@ static void canonicalisePathMetaData_(
             throw SysError("querying extended attributes of '%s'", path);
 
         for (auto & eaName : tokenizeString<Strings>(std::string(eaBuf.data(), eaSize), std::string("\000", 1))) {
-            if (settings.ignoredAcls.get().count(eaName))
+            if (options.ignoredAcls.count(eaName))
                 continue;
             if (lremovexattr(path.c_str(), eaName.c_str()) == -1)
                 throw SysError("removing extended attribute '%s' from '%s'", eaName, path);
@@ -95,7 +91,7 @@ static void canonicalisePathMetaData_(
        However, ignore files that we chown'ed ourselves previously to
        ensure that we don't fail on hard links within the same build
        (i.e. "touch $out/foo; ln $out/foo $out/bar"). */
-    if (uidRange && (st.st_uid < uidRange->first || st.st_uid > uidRange->second)) {
+    if (options.uidRange && (st.st_uid < options.uidRange->first || st.st_uid > options.uidRange->second)) {
         if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino)))
             throw BuildError(BuildResult::Failure::OutputRejected, "invalid ownership on file '%1%'", path);
         mode_t mode = st.st_mode & ~S_IFMT;
@@ -131,29 +127,14 @@ static void canonicalisePathMetaData_(
     if (S_ISDIR(st.st_mode)) {
         for (auto & i : DirectoryIterator{path}) {
             checkInterrupt();
-            canonicalisePathMetaData_(
-                i.path().string(),
-#ifndef _WIN32
-                uidRange,
-#endif
-                inodesSeen);
+            canonicalisePathMetaData_(i.path().string(), options, inodesSeen);
         }
     }
 }
 
-void canonicalisePathMetaData(
-    const Path & path,
-#ifndef _WIN32
-    std::optional<std::pair<uid_t, uid_t>> uidRange,
-#endif
-    InodesSeen & inodesSeen)
+void canonicalisePathMetaData(const Path & path, CanonicalizePathMetadataOptions options, InodesSeen & inodesSeen)
 {
-    canonicalisePathMetaData_(
-        path,
-#ifndef _WIN32
-        uidRange,
-#endif
-        inodesSeen);
+    canonicalisePathMetaData_(path, options, inodesSeen);
 
 #ifndef _WIN32
     /* On platforms that don't have lchown(), the top-level path can't
@@ -167,21 +148,10 @@ void canonicalisePathMetaData(
 #endif
 }
 
-void canonicalisePathMetaData(
-    const Path & path
-#ifndef _WIN32
-    ,
-    std::optional<std::pair<uid_t, uid_t>> uidRange
-#endif
-)
+void canonicalisePathMetaData(const Path & path, CanonicalizePathMetadataOptions options)
 {
     InodesSeen inodesSeen;
-    canonicalisePathMetaData_(
-        path,
-#ifndef _WIN32
-        uidRange,
-#endif
-        inodesSeen);
+    canonicalisePathMetaData_(path, options, inodesSeen);
 }
 
 } // namespace nix

src/libstore/unix/build/derivation-builder.cc

@@ -1459,7 +1459,13 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
            rewriting doesn't contain a hard link to /etc/shadow or
            something like that. */
         canonicalisePathMetaData(
-            actualPath, buildUser ? std::optional(buildUser->getUIDRange()) : std::nullopt, inodesSeen);
+            actualPath,
+            {
+#ifndef _WIN32
+                .uidRange = buildUser ? std::optional(buildUser->getUIDRange()) : std::nullopt,
+#endif
+                NIX_WHEN_SUPPORT_ACLS(settings.ignoredAcls)},
+            inodesSeen);
 
         bool discardReferences = false;
         if (auto udr = get(drvOptions.unsafeDiscardReferences, outputName)) {
@@ -1581,7 +1587,15 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
 
                 /* FIXME: set proper permissions in restorePath() so
                    we don't have to do another traversal. */
-                canonicalisePathMetaData(actualPath, {}, inodesSeen);
+                canonicalisePathMetaData(
+                    actualPath,
+                    {
+#ifndef _WIN32
+                        // builder UIDs are already dealt with
+                        .uidRange = std::nullopt,
+#endif
+                        NIX_WHEN_SUPPORT_ACLS(settings.ignoredAcls)},
+                    inodesSeen);
             }
         };
 
@@ -1738,7 +1752,15 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
 
         /* FIXME: set proper permissions in restorePath() so
             we don't have to do another traversal. */
-        canonicalisePathMetaData(actualPath, {}, inodesSeen);
+        canonicalisePathMetaData(
+            actualPath,
+            {
+#ifndef _WIN32
+                // builder UIDs are already dealt with
+                .uidRange = std::nullopt,
+#endif
+                NIX_WHEN_SUPPORT_ACLS(settings.ignoredAcls)},
+            inodesSeen);
 
         /* Calculate where we'll move the output files. In the checking case we
            will leave leave them where they are, for now, rather than move to

src/nix/nix-store/nix-store.cc

@@ -6,6 +6,7 @@
 #include "nix/store/store-cast.hh"
 #include "nix/store/local-fs-store.hh"
 #include "nix/store/log-store.hh"
+#include "nix/store/local-store.hh"
 #include "nix/store/serve-protocol.hh"
 #include "nix/store/serve-protocol-connection.hh"
 #include "nix/main/shared.hh"
@@ -16,13 +17,12 @@
 #include "nix/store/path-with-outputs.hh"
 #include "nix/store/export-import.hh"
 #include "nix/util/strings.hh"
+#include "nix/store/posix-fs-canonicalise.hh"
 
 #include "man-pages.hh"
 
 #ifndef _WIN32 // TODO implement on Windows or provide allowed-to-noop interface
-#  include "nix/store/local-store.hh"
 #  include "nix/util/monitor-fd.hh"
-#  include "nix/store/posix-fs-canonicalise.hh"
 #endif
 
 #include <iostream>
@@ -49,7 +49,6 @@ static int rootNr = 0;
 static bool noOutput = false;
 static std::shared_ptr<Store> store;
 
-#ifndef _WIN32 // TODO reenable on Windows once we have `LocalStore` there
 ref<LocalStore> ensureLocalStore()
 {
     auto store2 = std::dynamic_pointer_cast<LocalStore>(store);
@@ -57,7 +56,6 @@ ref<LocalStore> ensureLocalStore()
         throw Error("you don't have sufficient rights to use this command");
     return ref<LocalStore>(store2);
 }
-#endif
 
 static StorePath useDeriver(const StorePath & path)
 {
@@ -590,11 +588,8 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise)
         if (!store->isValidPath(info->path) || reregister) {
             /* !!! races */
             if (canonicalise)
-#ifdef _WIN32 // TODO implement on Windows
-                throw UnimplementedError("file attribute canonicalisation Is not implemented on Windows");
-#else
-                canonicalisePathMetaData(store->printStorePath(info->path), {});
-#endif
+                canonicalisePathMetaData(
+                    store->printStorePath(info->path), {NIX_WHEN_SUPPORT_ACLS(settings.ignoredAcls)});
             if (!hashGiven) {
                 HashResult hash = hashPath(
                     {store->requireStoreObjectAccessor(info->path, /*requireValidPath=*/false)},
@@ -607,9 +602,7 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise)
         }
     }
 
-#ifndef _WIN32 // TODO reenable on Windows once we have `LocalStore` there
     ensureLocalStore()->registerValidPaths(infos);
-#endif
 }
 
 static void opLoadDB(Strings opFlags, Strings opArgs)