Merge pull request #8370 from hercules-ci/fetchClosure-input-addressed

`fetchClosure`: input addressed and pure

8 files changed, 292 insertions, 67 deletions

diff --git a/doc/manual/src/language/builtins-prefix.md b/doc/manual/src/language/builtins-prefix.md
index 35e3dccc3..7b2321466 100644
--- a/doc/manual/src/language/builtins-prefix.md
+++ b/doc/manual/src/language/builtins-prefix.md
@@ -3,7 +3,7 @@
 This section lists the functions built into the Nix language evaluator.
 All built-in functions are available through the global [`builtins`](./builtin-constants.md#builtins-builtins) constant.
 
-For convenience, some built-ins are can be accessed directly:
+For convenience, some built-ins can be accessed directly:
 
 - [`derivation`](#builtins-derivation)
 - [`import`](#builtins-import)
diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md
index 8479b166a..139d07188 100644
--- a/doc/manual/src/release-notes/rl-next.md
+++ b/doc/manual/src/release-notes/rl-next.md
@@ -2,5 +2,7 @@
 
 - [`nix-channel`](../command-ref/nix-channel.md) now supports a `--list-generations` subcommand
 
+* The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure evaluation mode](../command-ref/conf-file.md#conf-pure-eval), as those are not impure.
+
 - Nix now allows unprivileged/[`allowed-users`](../command-ref/conf-file.md#conf-allowed-users) to sign paths.
   Previously, only [`trusted-users`](../command-ref/conf-file.md#conf-trusted-users) users could sign paths.
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 5dfad470a..f4bdf8fc6 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -1502,6 +1502,8 @@ static RegisterPrimOp primop_storePath({
       in a new path (e.g. `/nix/store/ld01dnzc…-source-source`).
 
       Not available in [pure evaluation mode](@docroot@/command-ref/conf-file.md#conf-pure-eval).
+
+      See also [`builtins.fetchClosure`](#builtins-fetchClosure).
     )",
     .fun = prim_storePath,
 });
diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index bae849f61..7fe8203f4 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -5,37 +5,150 @@
 
 namespace nix {
 
+/**
+ * Handler for the content addressed case.
+ *
+ * @param state Evaluator state and store to write to.
+ * @param fromStore Store containing the path to rewrite.
+ * @param fromPath Source path to be rewritten.
+ * @param toPathMaybe Path to write the rewritten path to. If empty, the error shows the actual path.
+ * @param v Return `Value`
+ */
+static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, const std::optional<StorePath> & toPathMaybe, Value &v) {
+
+    // establish toPath or throw
+
+    if (!toPathMaybe || !state.store->isValidPath(*toPathMaybe)) {
+        auto rewrittenPath = makeContentAddressed(fromStore, *state.store, fromPath);
+        if (toPathMaybe && *toPathMaybe != rewrittenPath)
+            throw Error({
+                .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected",
+                    state.store->printStorePath(fromPath),
+                    state.store->printStorePath(rewrittenPath),
+                    state.store->printStorePath(*toPathMaybe)),
+                .errPos = state.positions[pos]
+            });
+        if (!toPathMaybe)
+            throw Error({
+                .msg = hintfmt(
+                    "rewriting '%s' to content-addressed form yielded '%s'\n"
+                    "Use this value for the 'toPath' attribute passed to 'fetchClosure'",
+                    state.store->printStorePath(fromPath),
+                    state.store->printStorePath(rewrittenPath)),
+                .errPos = state.positions[pos]
+            });
+    }
+
+    auto toPath = *toPathMaybe;
+
+    // check and return
+
+    auto resultInfo = state.store->queryPathInfo(toPath);
+
+    if (!resultInfo->isContentAddressed(*state.store)) {
+        // We don't perform the rewriting when outPath already exists, as an optimisation.
+        // However, we can quickly detect a mistake if the toPath is input addressed.
+        throw Error({
+            .msg = hintfmt(
+                "The 'toPath' value '%s' is input-addressed, so it can't possibly be the result of rewriting to a content-addressed path.\n\n"
+                "Set 'toPath' to an empty string to make Nix report the correct content-addressed path.",
+                state.store->printStorePath(toPath)),
+            .errPos = state.positions[pos]
+        });
+    }
+
+    state.mkStorePathString(toPath, v);
+}
+
+/**
+ * Fetch the closure and make sure it's content addressed.
+ */
+static void runFetchClosureWithContentAddressedPath(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, Value & v) {
+
+    if (!state.store->isValidPath(fromPath))
+        copyClosure(fromStore, *state.store, RealisedPath::Set { fromPath });
+
+    auto info = state.store->queryPathInfo(fromPath);
+
+    if (!info->isContentAddressed(*state.store)) {
+        throw Error({
+            .msg = hintfmt(
+                "The 'fromPath' value '%s' is input-addressed, but 'inputAddressed' is set to 'false' (default).\n\n"
+                "If you do intend to fetch an input-addressed store path, add\n\n"
+                "    inputAddressed = true;\n\n"
+                "to the 'fetchClosure' arguments.\n\n"
+                "Note that to ensure authenticity input-addressed store paths, users must configure a trusted binary cache public key on their systems. This is not needed for content-addressed paths.",
+                state.store->printStorePath(fromPath)),
+            .errPos = state.positions[pos]
+        });
+    }
+
+    state.mkStorePathString(fromPath, v);
+}
+
+/**
+ * Fetch the closure and make sure it's input addressed.
+ */
+static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, Value & v) {
+
+    if (!state.store->isValidPath(fromPath))
+        copyClosure(fromStore, *state.store, RealisedPath::Set { fromPath });
+
+    auto info = state.store->queryPathInfo(fromPath);
+
+    if (info->isContentAddressed(*state.store)) {
+        throw Error({
+            .msg = hintfmt(
+                "The store object referred to by 'fromPath' at '%s' is not input-addressed, but 'inputAddressed' is set to 'true'.\n\n"
+                "Remove the 'inputAddressed' attribute (it defaults to 'false') to expect 'fromPath' to be content-addressed",
+                state.store->printStorePath(fromPath)),
+            .errPos = state.positions[pos]
+        });
+    }
+
+    state.mkStorePathString(fromPath, v);
+}
+
+typedef std::optional<StorePath> StorePathOrGap;
+
 static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.fetchClosure");
 
     std::optional<std::string> fromStoreUrl;
     std::optional<StorePath> fromPath;
-    bool toCA = false;
-    std::optional<StorePath> toPath;
+    std::optional<StorePathOrGap> toPath;
+    std::optional<bool> inputAddressedMaybe;
 
     for (auto & attr : *args[0]->attrs) {
         const auto & attrName = state.symbols[attr.name];
+        auto attrHint = [&]() -> std::string {
+            return "while evaluating the '" + attrName + "' attribute passed to builtins.fetchClosure";
+        };
 
         if (attrName == "fromPath") {
             NixStringContext context;
-            fromPath = state.coerceToStorePath(attr.pos, *attr.value, context,
-                    "while evaluating the 'fromPath' attribute passed to builtins.fetchClosure");
+            fromPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint());
         }
 
         else if (attrName == "toPath") {
             state.forceValue(*attr.value, attr.pos);
-            toCA = true;
-            if (attr.value->type() != nString || attr.value->string.s != std::string("")) {
+            bool isEmptyString = attr.value->type() == nString && attr.value->string.s == std::string("");
+            if (isEmptyString) {
+                toPath = StorePathOrGap {};
+            }
+            else {
                 NixStringContext context;
-                toPath = state.coerceToStorePath(attr.pos, *attr.value, context,
-                        "while evaluating the 'toPath' attribute passed to builtins.fetchClosure");
+                toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint());
             }
         }
 
         else if (attrName == "fromStore")
             fromStoreUrl = state.forceStringNoCtx(*attr.value, attr.pos,
-                    "while evaluating the 'fromStore' attribute passed to builtins.fetchClosure");
+                    attrHint());
+
+        else if (attrName == "inputAddressed")
+            inputAddressedMaybe = state.forceBool(*attr.value, attr.pos, attrHint());
 
         else
             throw Error({
@@ -50,6 +163,18 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
             .errPos = state.positions[pos]
         });
 
+    bool inputAddressed = inputAddressedMaybe.value_or(false);
+
+    if (inputAddressed) {
+        if (toPath)
+            throw Error({
+                .msg = hintfmt("attribute '%s' is set to true, but '%s' is also set. Please remove one of them",
+                    "inputAddressed",
+                    "toPath"),
+                .errPos = state.positions[pos]
+            });
+    }
+
     if (!fromStoreUrl)
         throw Error({
             .msg = hintfmt("attribute '%s' is missing in call to 'fetchClosure'", "fromStore"),
@@ -74,55 +199,40 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     auto fromStore = openStore(parsedURL.to_string());
 
-    if (toCA) {
-        if (!toPath || !state.store->isValidPath(*toPath)) {
-            auto remappings = makeContentAddressed(*fromStore, *state.store, { *fromPath });
-            auto i = remappings.find(*fromPath);
-            assert(i != remappings.end());
-            if (toPath && *toPath != i->second)
-                throw Error({
-                    .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected",
-                        state.store->printStorePath(*fromPath),
-                        state.store->printStorePath(i->second),
-                        state.store->printStorePath(*toPath)),
-                    .errPos = state.positions[pos]
-                });
-            if (!toPath)
-                throw Error({
-                    .msg = hintfmt(
-                        "rewriting '%s' to content-addressed form yielded '%s'; "
-                        "please set this in the 'toPath' attribute passed to 'fetchClosure'",
-                        state.store->printStorePath(*fromPath),
-                        state.store->printStorePath(i->second)),
-                    .errPos = state.positions[pos]
-                });
-        }
-    } else {
-        if (!state.store->isValidPath(*fromPath))
-            copyClosure(*fromStore, *state.store, RealisedPath::Set { *fromPath });
-        toPath = fromPath;
-    }
-
-    /* In pure mode, require a CA path. */
-    if (evalSettings.pureEval) {
-        auto info = state.store->queryPathInfo(*toPath);
-        if (!info->isContentAddressed(*state.store))
-            throw Error({
-                .msg = hintfmt("in pure mode, 'fetchClosure' requires a content-addressed path, which '%s' isn't",
-                    state.store->printStorePath(*toPath)),
-                .errPos = state.positions[pos]
-            });
-    }
-
-    state.mkStorePathString(*toPath, v);
+    if (toPath)
+        runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, *toPath, v);
+    else if (inputAddressed)
+        runFetchClosureWithInputAddressedPath(state, pos, *fromStore, *fromPath, v);
+    else
+        runFetchClosureWithContentAddressedPath(state, pos, *fromStore, *fromPath, v);
 }
 
 static RegisterPrimOp primop_fetchClosure({
     .name = "__fetchClosure",
     .args = {"args"},
     .doc = R"(
-      Fetch a Nix store closure from a binary cache, rewriting it into
-      content-addressed form. For example,
+      Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache, and return the store path as a string with context.
+
+      This function can be invoked in three ways, that we will discuss in order of preference.
+
+      **Fetch a content-addressed store path**
+
+      Example:
+
+      ```nix
+      builtins.fetchClosure {
+        fromStore = "https://cache.nixos.org";
+        fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
+      }
+      ```
+
+      This is the simplest invocation, and it does not require the user of the expression to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity.
+
+      If your store path is [input addressed](@docroot@/glossary.md#gloss-input-addressed-store-object) instead of content addressed, consider the other two invocations.
+
+      **Fetch any store path and rewrite it to a fully content-addressed store path**
+
+      Example:
 
       ```nix
       builtins.fetchClosure {
@@ -132,28 +242,42 @@ static RegisterPrimOp primop_fetchClosure({
       }
       ```
 
-      fetches `/nix/store/r2jd...` from the specified binary cache,
+      This example fetches `/nix/store/r2jd...` from the specified binary cache,
       and rewrites it into the content-addressed store path
       `/nix/store/ldbh...`.
 
-      If `fromPath` is already content-addressed, or if you are
-      allowing impure evaluation (`--impure`), then `toPath` may be
-      omitted.
+      Like the previous example, no extra configuration or privileges are required.
 
       To find out the correct value for `toPath` given a `fromPath`,
-      you can use `nix store make-content-addressed`:
+      use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md):
 
       ```console
       # nix store make-content-addressed --from https://cache.nixos.org /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1
       rewrote '/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1' to '/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1'
       ```
 
-      This function is similar to `builtins.storePath` in that it
-      allows you to use a previously built store path in a Nix
-      expression. However, it is more reproducible because it requires
-      specifying a binary cache from which the path can be fetched.
-      Also, requiring a content-addressed final store path avoids the
-      need for users to configure binary cache public keys.
+      Alternatively, set `toPath = ""` and find the correct `toPath` in the error message.
+
+      **Fetch an input-addressed store path as is**
+
+      Example:
+
+      ```nix
+      builtins.fetchClosure {
+        fromStore = "https://cache.nixos.org";
+        fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
+        inputAddressed = true;
+      }
+      ```
+
+      It is possible to fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) and return it as is.
+      However, this is the least preferred way of invoking `fetchClosure`, because it requires that the input-addressed paths are trusted by the Nix configuration.
+
+      **`builtins.storePath`**
+
+      `fetchClosure` is similar to [`builtins.storePath`](#builtins-storePath) in that it allows you to use a previously built store path in a Nix expression.
+      However, `fetchClosure` is more reproducible because it specifies a binary cache from which the path can be fetched.
+      Also, using content-addressed store paths does not require users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity.
     )",
     .fun = prim_fetchClosure,
     .experimentalFeature = Xp::FetchClosure,
diff --git a/src/libstore/make-content-addressed.cc b/src/libstore/make-content-addressed.cc
index 53fe04704..626a22480 100644
--- a/src/libstore/make-content-addressed.cc
+++ b/src/libstore/make-content-addressed.cc
@@ -80,4 +80,15 @@ std::map<StorePath, StorePath> makeContentAddressed(
     return remappings;
 }
 
+StorePath makeContentAddressed(
+    Store & srcStore,
+    Store & dstStore,
+    const StorePath & fromPath)
+{
+    auto remappings = makeContentAddressed(srcStore, dstStore, StorePathSet { fromPath });
+    auto i = remappings.find(fromPath);
+    assert(i != remappings.end());
+    return i->second;
+}
+
 }
diff --git a/src/libstore/make-content-addressed.hh b/src/libstore/make-content-addressed.hh
index 2ce6ec7bc..60bb2b477 100644
--- a/src/libstore/make-content-addressed.hh
+++ b/src/libstore/make-content-addressed.hh
@@ -5,9 +5,20 @@
 
 namespace nix {
 
+/** Rewrite a closure of store paths to be completely content addressed.
+ */
 std::map<StorePath, StorePath> makeContentAddressed(
     Store & srcStore,
     Store & dstStore,
-    const StorePathSet & storePaths);
+    const StorePathSet & rootPaths);
+
+/** Rewrite a closure of a store path to be completely content addressed.
+ *
+ * This is a convenience function for the case where you only have one root path.
+ */
+StorePath makeContentAddressed(
+    Store & srcStore,
+    Store & dstStore,
+    const StorePath & rootPath);
 
 }
diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh
index 21650eb06..a02d1ce7a 100644
--- a/tests/fetchClosure.sh
+++ b/tests/fetchClosure.sh
@@ -33,20 +33,43 @@ clearStore
 [ ! -e $nonCaPath ]
 [ -e $caPath ]
 
+clearStore
+
+# The daemon will reject input addressed paths unless configured to trust the
+# cache key or the user. This behavior should be covered by another test, so we
+# skip this part when using the daemon.
 if [[ "$NIX_REMOTE" != "daemon" ]]; then
 
-    # In impure mode, we can use non-CA paths.
-    [[ $(nix eval --raw --no-require-sigs --impure --expr "
+    # If we want to return a non-CA path, we have to be explicit about it.
+    expectStderr 1 nix eval --raw --no-require-sigs --expr "
+      builtins.fetchClosure {
+        fromStore = \"file://$cacheDir\";
+        fromPath = $nonCaPath;
+      }
+    " | grepQuiet -E "The .fromPath. value .* is input-addressed, but .inputAddressed. is set to .false."
+
+    # TODO: Should the closure be rejected, despite single user mode?
+    # [ ! -e $nonCaPath ]
+
+    [ ! -e $caPath ]
+
+    # We can use non-CA paths when we ask explicitly.
+    [[ $(nix eval --raw --no-require-sigs --expr "
       builtins.fetchClosure {
         fromStore = \"file://$cacheDir\";
         fromPath = $nonCaPath;
+        inputAddressed = true;
       }
     ") = $nonCaPath ]]
 
     [ -e $nonCaPath ]
+    [ ! -e $caPath ]
+
 
 fi
 
+[ ! -e $caPath ]
+
 # 'toPath' set to empty string should fail but print the expected path.
 expectStderr 1 nix eval -v --json --expr "
   builtins.fetchClosure {
@@ -59,6 +82,10 @@ expectStderr 1 nix eval -v --json --expr "
 # If fromPath is CA, then toPath isn't needed.
 nix copy --to file://$cacheDir $caPath
 
+clearStore
+
+[ ! -e $caPath ]
+
 [[ $(nix eval -v --raw --expr "
   builtins.fetchClosure {
     fromStore = \"file://$cacheDir\";
@@ -66,6 +93,8 @@ nix copy --to file://$cacheDir $caPath
   }
 ") = $caPath ]]
 
+[ -e $caPath ]
+
 # Check that URL query parameters aren't allowed.
 clearStore
 narCache=$TEST_ROOT/nar-cache
@@ -77,3 +106,45 @@ rm -rf $narCache
   }
 ")
 (! [ -e $narCache ])
+
+# If toPath is specified but wrong, we check it (only) when the path is missing.
+clearStore
+
+badPath=$(echo $caPath | sed -e 's!/store/................................-!/store/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-!')
+
+[ ! -e $badPath ]
+
+expectStderr 1 nix eval -v --raw --expr "
+  builtins.fetchClosure {
+    fromStore = \"file://$cacheDir\";
+    fromPath = $nonCaPath;
+    toPath = $badPath;
+  }
+" | grep "error: rewriting.*$nonCaPath.*yielded.*$caPath.*while.*$badPath.*was expected"
+
+[ ! -e $badPath ]
+
+# We only check it when missing, as a performance optimization similar to what we do for fixed output derivations. So if it's already there, we don't check it.
+# It would be nice for this to fail, but checking it would be too(?) slow.
+[ -e $caPath ]
+
+[[ $(nix eval -v --raw --expr "
+  builtins.fetchClosure {
+    fromStore = \"file://$cacheDir\";
+    fromPath = $badPath;
+    toPath = $caPath;
+  }
+") = $caPath ]]
+
+
+# However, if the output address is unexpected, we can report it
+
+
+expectStderr 1 nix eval -v --raw --expr "
+  builtins.fetchClosure {
+    fromStore = \"file://$cacheDir\";
+    fromPath = $caPath;
+    inputAddressed = true;
+  }
+" | grepQuiet 'error.*The store object referred to by.*fromPath.* at .* is not input-addressed, but .*inputAddressed.* is set to .*true.*'
+
diff --git a/tests/signing.sh b/tests/signing.sh
index 9b673c609..942b51630 100644
--- a/tests/signing.sh
+++ b/tests/signing.sh
@@ -84,6 +84,10 @@ info=$(nix path-info --store file://$cacheDir --json $outPath2)
 # Copying to a diverted store should fail due to a lack of signatures by trusted keys.
 chmod -R u+w $TEST_ROOT/store0 || true
 rm -rf $TEST_ROOT/store0
+
+# Fails or very flaky only on GHA + macOS:
+#     expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key'
+# but this works:
 (! nix copy --to $TEST_ROOT/store0 $outPath)
 
 # But succeed if we supply the public keys.