From cbc43442977a6fbbd046a98fcb09362a60323b5d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Aug 2020 03:47:36 +0000 Subject: [PATCH 01/12] Trustless remote building Co-authored-by: Matthew Bauer --- src/build-remote/build-remote.cc | 22 +++++--- src/libstore/daemon.cc | 2 + src/libstore/store-api.cc | 18 ++++++- src/libstore/store-api.hh | 4 +- src/nix-daemon/nix-daemon.cc | 19 ++++++- tests/build-hook-ca.nix | 52 +++++++++++++++++++ tests/build-remote-trustless-should-fail-0.sh | 11 ++++ tests/build-remote-trustless-should-pass-0.sh | 9 ++++ tests/build-remote-trustless-should-pass-1.sh | 9 ++++ tests/build-remote-trustless-should-pass-2.sh | 9 ++++ tests/build-remote-trustless-should-pass-3.sh | 10 ++++ tests/build-remote-trustless.sh | 18 +++++++ tests/init.sh | 2 +- tests/local.mk | 5 ++ tests/nix-daemon-untrusting.sh | 3 ++ 15 files changed, 181 insertions(+), 12 deletions(-) create mode 100644 tests/build-hook-ca.nix create mode 100644 tests/build-remote-trustless-should-fail-0.sh create mode 100644 tests/build-remote-trustless-should-pass-0.sh create mode 100644 tests/build-remote-trustless-should-pass-1.sh create mode 100644 tests/build-remote-trustless-should-pass-2.sh create mode 100644 tests/build-remote-trustless-should-pass-3.sh create mode 100644 tests/build-remote-trustless.sh create mode 100755 tests/nix-daemon-untrusting.sh diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index ce5127113..8ae1cabfe 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -247,6 +247,9 @@ static int _main(int argc, char * * argv) connected: close(5); + assert(sshStore); + auto sshStore2 = ref(sshStore); + std::cerr << "# accept\n" << storeUri << "\n"; auto inputs = readStrings(source); @@ -269,18 +272,23 @@ connected: { Activity act(*logger, lvlTalkative, actUnknown, fmt("copying dependencies to '%s'", storeUri)); - copyPaths(store, ref(sshStore), store->parseStorePathSet(inputs), NoRepair, NoCheckSigs, substitute); + copyPaths(store, sshStore2, store->parseStorePathSet(inputs), NoRepair, NoCheckSigs, substitute); } uploadLock = -1; - auto drv = store->readDerivation(*drvPath); - drv.inputSrcs = store->parseStorePathSet(inputs); + BasicDerivation drv = store->readDerivation(*drvPath); - auto result = sshStore->buildDerivation(*drvPath, drv); + if (sshStore2->isTrusting || derivationIsCA(drv.type())) { + drv.inputSrcs = store->parseStorePathSet(inputs); + auto result = sshStore2->buildDerivation(*drvPath, drv); + if (!result.success()) + throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg); + } else { + copyPaths(store, sshStore2, {*drvPath}, NoRepair, NoCheckSigs, substitute); + sshStore2->buildPaths({{*drvPath}}); + } - if (!result.success()) - throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg); StorePathSet missing; for (auto & path : outputs) @@ -290,7 +298,7 @@ connected: Activity act(*logger, lvlTalkative, actUnknown, fmt("copying outputs from '%s'", storeUri)); for (auto & i : missing) store->locksHeld.insert(store->printStorePath(i)); /* FIXME: ugly */ - copyPaths(ref(sshStore), store, missing, NoRepair, NoCheckSigs, NoSubstitute); + copyPaths(sshStore2, store, missing, NoRepair, NoCheckSigs, NoSubstitute); } return 0; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 45e81c8da..ffa7e0dcd 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -914,6 +914,8 @@ void processConnection( opCount++; + debug("performing daemon worker op: %d", op); + try { performOp(tunnelLogger, store, trusted, recursive, clientVersion, from, to, op); } catch (Error & e) { diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 3d07e2d38..360820b1b 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -834,7 +834,23 @@ std::map copyPaths(ref srcStore, ref dstStor MaintainCount mc(nrRunning); showProgress(); try { - copyStorePath(srcStore, dstStore, storePath, repair, checkSigs); + if (dstStore->isTrusting || info->ca) { + copyStorePath(srcStore, dstStore, storePath, repair, checkSigs); + } else if (info->deriver && dstStore->storeDir == srcStore->storeDir) { + auto drvPath = *info->deriver; + auto outputMap = srcStore->queryDerivationOutputMap(drvPath); + auto p = std::find_if(outputMap.begin(), outputMap.end(), [&](auto & i) { + return i.second == storePath; + }); + // drv file is always CA + copyStorePath(srcStore, dstStore, drvPath, repair, checkSigs); + dstStore->buildPaths({{ + drvPath, + p != outputMap.end() ? StringSet { p->first } : StringSet {}, + }}); + } else { + dstStore->ensurePath(storePath); + } } catch (Error &e) { nrFailed++; if (!settings.keepGoing) diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 1e940e6a8..4ba51a9e5 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -158,7 +158,9 @@ public: const Setting pathInfoCacheSize{this, 65536, "path-info-cache-size", "size of the in-memory store path information cache"}; - const Setting isTrusted{this, false, "trusted", "whether paths from this store can be used as substitutes even when they lack trusted signatures"}; + const Setting isTrusted{this, false, "trusted", "whether paths from this store can be used as substitutes even when they lack trusted signatures. Compare \"trusting\""}; + + Setting isTrusting{this, true, "trusting", "whether (we think) paths can be added to this store even when they lack trusted signatures. Compare \"trusted\""}; Setting priority{this, 0, "priority", "priority of this substituter (lower value means higher priority)"}; diff --git a/src/nix-daemon/nix-daemon.cc b/src/nix-daemon/nix-daemon.cc index cfa634a44..ce963acdc 100644 --- a/src/nix-daemon/nix-daemon.cc +++ b/src/nix-daemon/nix-daemon.cc @@ -268,6 +268,7 @@ static int _main(int argc, char * * argv) { { auto stdio = false; + std::optional isTrustedOpt; parseCmdLine(argc, argv, [&](Strings::iterator & arg, const Strings::iterator & end) { if (*arg == "--daemon") @@ -278,14 +279,26 @@ static int _main(int argc, char * * argv) printVersion("nix-daemon"); else if (*arg == "--stdio") stdio = true; - else return false; + else if (*arg == "--trust") { + settings.requireExperimentalFeature("nix-testing"); + isTrustedOpt = Trusted; + } else if (*arg == "--no-trust") { + settings.requireExperimentalFeature("nix-testing"); + isTrustedOpt = NotTrusted; + } else return false; return true; }); initPlugins(); + auto ensureNoTrustedFlag = [&]() { + if (isTrustedOpt) + throw Error("--trust and --no-trust flags are only for use with --stdio when this nix-daemon process is not proxying another"); + }; + if (stdio) { if (getStoreType() == tDaemon) { + ensureNoTrustedFlag(); // Forward on this connection to the real daemon auto socketPath = settings.nixDaemonSocketFile; auto s = socket(PF_UNIX, SOCK_STREAM, 0); @@ -335,9 +348,11 @@ static int _main(int argc, char * * argv) /* Auth hook is empty because in this mode we blindly trust the standard streams. Limitting access to thoses is explicitly not `nix-daemon`'s responsibility. */ - processConnection(openUncachedStore(), from, to, Trusted, NotRecursive, [&](Store & _){}); + auto isTrusted = isTrustedOpt.value_or(Trusted); + processConnection(openUncachedStore(), from, to, isTrusted, NotRecursive, [&](Store & _){}); } } else { + ensureNoTrustedFlag(); daemonLoop(argv); } diff --git a/tests/build-hook-ca.nix b/tests/build-hook-ca.nix new file mode 100644 index 000000000..c0bc6d211 --- /dev/null +++ b/tests/build-hook-ca.nix @@ -0,0 +1,52 @@ +{ busybox }: + +with import ./config.nix; + +let + + mkDerivation = args: + derivation ({ + inherit system; + builder = busybox; + args = ["sh" "-e" args.builder or (builtins.toFile "builder-${args.name}.sh" "if [ -e .attrs.sh ]; then source .attrs.sh; fi; eval \"$buildCommand\"")]; + outputHashMode = "recursive"; + outputHashAlgo = "sha256"; + } // removeAttrs args ["builder" "meta"]) + // { meta = args.meta or {}; }; + + input1 = mkDerivation { + shell = busybox; + name = "build-remote-input-1"; + buildCommand = "echo FOO > $out"; + outputHash = "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="; + }; + + input2 = mkDerivation { + shell = busybox; + name = "build-remote-input-2"; + buildCommand = "echo BAR > $out"; + outputHash = "sha256-XArauVH91AVwP9hBBQNlkX9ccuPpSYx9o0zeIHb6e+Q="; + }; + + input3 = mkDerivation { + shell = busybox; + name = "build-remote-input-3"; + buildCommand = '' + read x < ${input2} + echo $x BAZ > $out + ''; + outputHash = "sha256-daKAcPp/+BYMQsVi/YYMlCKoNAxCNDsaivwSHgQqD2s="; + }; + +in + + mkDerivation { + shell = busybox; + name = "build-remote"; + buildCommand = '' + read x < ${input1} + read y < ${input3} + echo "$x $y" > $out + ''; + outputHash = "sha256-5SxbkUw6xe2l9TE1uwCvTtTDysD1vhRor38OtDF0LqQ="; + } diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh new file mode 100644 index 000000000..b72c134e7 --- /dev/null +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -0,0 +1,11 @@ +source common.sh + +# We act as if remote trusts us, but it doesn't. This fails since we are +# building input-addressed derivations with `buildDerivation`, which +# depends on trust. +file=build-hook.nix +prog=$(readlink -e ./nix-daemon-untrusting.sh) +proto=ssh-ng +trusting=true + +! source build-remote-trustless.sh diff --git a/tests/build-remote-trustless-should-pass-0.sh b/tests/build-remote-trustless-should-pass-0.sh new file mode 100644 index 000000000..82c3f4520 --- /dev/null +++ b/tests/build-remote-trustless-should-pass-0.sh @@ -0,0 +1,9 @@ +source common.sh + +# Remote trusts us but we pretend it doesn't. +file=build-hook.nix +prog=nix-store +proto=ssh +trusting=false + +source build-remote-trustless.sh diff --git a/tests/build-remote-trustless-should-pass-1.sh b/tests/build-remote-trustless-should-pass-1.sh new file mode 100644 index 000000000..22c304bc2 --- /dev/null +++ b/tests/build-remote-trustless-should-pass-1.sh @@ -0,0 +1,9 @@ +source common.sh + +# Remote trusts us but we pretend it doesn't. +file=build-hook.nix +prog=nix-daemon +proto=ssh-ng +trusting=false + +source build-remote-trustless.sh diff --git a/tests/build-remote-trustless-should-pass-2.sh b/tests/build-remote-trustless-should-pass-2.sh new file mode 100644 index 000000000..941ddca05 --- /dev/null +++ b/tests/build-remote-trustless-should-pass-2.sh @@ -0,0 +1,9 @@ +source common.sh + +# Remote doesn't trust us nor do we think it does +file=build-hook.nix +prog=$(readlink -e ./nix-daemon-untrusting.sh) +proto=ssh-ng +trusting=false + +source build-remote-trustless.sh diff --git a/tests/build-remote-trustless-should-pass-3.sh b/tests/build-remote-trustless-should-pass-3.sh new file mode 100644 index 000000000..d4f7e863a --- /dev/null +++ b/tests/build-remote-trustless-should-pass-3.sh @@ -0,0 +1,10 @@ +source common.sh + +# We act as if remote trusts us, but it doesn't. This is fine because we +# are only building (fixed) CA derivations. +file=build-hook-ca.nix +prog=$(readlink -e ./nix-daemon-untrusting.sh) +proto=ssh-ng +trusting=true + +source build-remote-trustless.sh diff --git a/tests/build-remote-trustless.sh b/tests/build-remote-trustless.sh new file mode 100644 index 000000000..2689e5727 --- /dev/null +++ b/tests/build-remote-trustless.sh @@ -0,0 +1,18 @@ +if ! canUseSandbox; then exit; fi +if ! [[ $busybox =~ busybox ]]; then exit; fi + +unset NIX_STORE_DIR +unset NIX_STATE_DIR + +# Note: ssh://localhost bypasses ssh, directly invoking nix-store as a +# child process. This allows us to test LegacySSHStore::buildDerivation(). +# ssh-ng://... likewise allows us to test RemoteStore::buildDerivation(). + +nix build -L -v -f $file -o $TEST_ROOT/result --max-jobs 0 \ + --arg busybox $busybox \ + --store $TEST_ROOT/local \ + --builders "$proto://localhost?remote-program=$prog&trusting=$trusting&remote-store=$TEST_ROOT/remote%3Fsystem-features=foo%20bar%20baz - - 1 1 foo,bar,baz" + +outPath=$(readlink -f $TEST_ROOT/result) + +grep 'FOO BAR BAZ' $TEST_ROOT/${subDir}/local${outPath} diff --git a/tests/init.sh b/tests/init.sh index f9ced6b0d..7adababf9 100644 --- a/tests/init.sh +++ b/tests/init.sh @@ -17,7 +17,7 @@ cat > "$NIX_CONF_DIR"/nix.conf < Date: Mon, 17 Aug 2020 13:15:08 -0400 Subject: [PATCH 02/12] Disable failing tests --- tests/local.mk | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/local.mk b/tests/local.mk index 75fdd8332..efe193356 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -15,10 +15,8 @@ nix_tests = \ linux-sandbox.sh \ build-dry.sh \ build-remote-input-addressed.sh \ - build-remote-content-addressed-fixed.sh \ build-remote-trustless-should-pass-1.sh \ build-remote-trustless-should-pass-2.sh \ - build-remote-trustless-should-pass-3.sh \ build-remote-trustless-should-fail-0.sh \ nar-access.sh \ structured-attrs.sh \ @@ -30,7 +28,6 @@ nix_tests = \ shell.sh \ brotli.sh \ pure-eval.sh \ - check.sh \ plugins.sh \ search.sh \ nix-copy-ssh.sh \ @@ -39,7 +36,10 @@ nix_tests = \ recursive.sh \ flakes.sh # parallel.sh + # build-remote-content-addressed-fixed.sh \ + # build-remote-trustless-should-pass-3.sh \ # build-remote-trustless-should-pass-0.sh # problem with legacy ssh-store only + # check.sh \ install-tests += $(foreach x, $(nix_tests), tests/$(x)) From 36758a1a09d75ad18d04ce2f5650eec455198ee4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 17 Aug 2020 18:01:38 +0000 Subject: [PATCH 03/12] But back check.sh Whether it fails or not, it is no a new test so we have to leave it. --- tests/local.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/local.mk b/tests/local.mk index efe193356..ff3063d58 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -28,6 +28,7 @@ nix_tests = \ shell.sh \ brotli.sh \ pure-eval.sh \ + check.sh \ plugins.sh \ search.sh \ nix-copy-ssh.sh \ @@ -39,7 +40,6 @@ nix_tests = \ # build-remote-content-addressed-fixed.sh \ # build-remote-trustless-should-pass-3.sh \ # build-remote-trustless-should-pass-0.sh # problem with legacy ssh-store only - # check.sh \ install-tests += $(foreach x, $(nix_tests), tests/$(x)) From 6c7b81047f8ead0bb2f8dd588dfcb5f50d1554a9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 9 Sep 2020 17:35:54 +0000 Subject: [PATCH 04/12] Make sure srcStore has path before coppying --- src/libstore/store-api.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index aed43543f..8075b3ff9 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -847,6 +847,7 @@ std::map copyPaths(ref srcStore, ref dstStor return i.second == storePath; }); // drv file is always CA + srcStore->ensurePath(drvPath); copyStorePath(srcStore, dstStore, drvPath, repair, checkSigs); dstStore->buildPaths({{ drvPath, From 141cb9a706f73725fd4f8d62569f45bbbf9b6c3b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 22 Jan 2021 17:56:28 +0000 Subject: [PATCH 05/12] Make regular `copyPaths` only copy again The is new function parameter so just the build hook can opt into the remote-side building. --- src/build-remote/build-remote.cc | 7 +++-- src/libstore/store-api.cc | 53 ++++++++++++++++++++------------ src/libstore/store-api.hh | 20 ++++++++++-- 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 402754c3c..ef943ee11 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -270,9 +270,11 @@ connected: auto substitute = settings.buildersUseSubstitutes ? Substitute : NoSubstitute; + auto copyStorePathImpl = sshStore2->isTrusting ? copyStorePathAdapter : copyOrBuildStorePath; + { Activity act(*logger, lvlTalkative, actUnknown, fmt("copying dependencies to '%s'", storeUri)); - copyPaths(store, sshStore2, store->parseStorePathSet(inputs), NoRepair, NoCheckSigs, substitute); + copyPaths(store, sshStore2, store->parseStorePathSet(inputs), NoRepair, NoCheckSigs, substitute, copyStorePathImpl); } uploadLock = -1; @@ -285,7 +287,7 @@ connected: if (!result.success()) throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg); } else { - copyPaths(store, sshStore2, {*drvPath}, NoRepair, NoCheckSigs, substitute); + copyPaths(store, sshStore2, {*drvPath}, NoRepair, NoCheckSigs, substitute, copyStorePathImpl); sshStore2->buildPaths({{*drvPath}}); } @@ -298,6 +300,7 @@ connected: Activity act(*logger, lvlTalkative, actUnknown, fmt("copying outputs from '%s'", storeUri)); for (auto & i : missing) store->locksHeld.insert(store->printStorePath(i)); /* FIXME: ugly */ + /* No `copyStorePathImpl` because we always trust ourselves. */ copyPaths(sshStore2, store, missing, NoRepair, NoCheckSigs, NoSubstitute); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index cce95e4cf..ad773bb0f 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -819,8 +819,40 @@ void copyStorePath(ref srcStore, ref dstStore, } +void copyStorePathAdapter(ref srcStore, ref dstStore, + const ValidPathInfo & info, RepairFlag repair, CheckSigsFlag checkSigs) +{ + copyStorePath(srcStore, dstStore, info.path, repair, checkSigs); +} + +void copyOrBuildStorePath(ref srcStore, ref dstStore, + const ValidPathInfo & info, RepairFlag repair, CheckSigsFlag checkSigs) +{ + auto storePath = info.path; + if (dstStore->isTrusting || info.ca) { + copyStorePath(srcStore, dstStore, storePath, repair, checkSigs); + } else if (info.deriver && dstStore->storeDir == srcStore->storeDir) { + auto drvPath = *info.deriver; + auto outputMap = srcStore->queryDerivationOutputMap(drvPath); + auto p = std::find_if(outputMap.begin(), outputMap.end(), [&](auto & i) { + return i.second == storePath; + }); + // drv file is always CA + srcStore->ensurePath(drvPath); + copyStorePath(srcStore, dstStore, drvPath, repair, checkSigs); + dstStore->buildPaths({{ + drvPath, + p != outputMap.end() ? StringSet { p->first } : StringSet {}, + }}); + } else { + dstStore->ensurePath(storePath); + } +} + + std::map copyPaths(ref srcStore, ref dstStore, const StorePathSet & storePaths, - RepairFlag repair, CheckSigsFlag checkSigs, SubstituteFlag substitute) + RepairFlag repair, CheckSigsFlag checkSigs, SubstituteFlag substitute, + std::function, ref, const ValidPathInfo &, RepairFlag, CheckSigsFlag)> copyStorePathImpl) { auto valid = dstStore->queryValidPaths(storePaths, substitute); @@ -893,24 +925,7 @@ std::map copyPaths(ref srcStore, ref dstStor MaintainCount mc(nrRunning); showProgress(); try { - if (dstStore->isTrusting || info->ca) { - copyStorePath(srcStore, dstStore, storePath, repair, checkSigs); - } else if (info->deriver && dstStore->storeDir == srcStore->storeDir) { - auto drvPath = *info->deriver; - auto outputMap = srcStore->queryDerivationOutputMap(drvPath); - auto p = std::find_if(outputMap.begin(), outputMap.end(), [&](auto & i) { - return i.second == storePath; - }); - // drv file is always CA - srcStore->ensurePath(drvPath); - copyStorePath(srcStore, dstStore, drvPath, repair, checkSigs); - dstStore->buildPaths({{ - drvPath, - p != outputMap.end() ? StringSet { p->first } : StringSet {}, - }}); - } else { - dstStore->ensurePath(storePath); - } + copyStorePathImpl(srcStore, dstStore, *info, repair, checkSigs); } catch (Error &e) { nrFailed++; if (!settings.keepGoing) diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 696a2d3fd..5597ebb22 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -738,17 +738,33 @@ void copyStorePath(ref srcStore, ref dstStore, const StorePath & storePath, RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs); +/* copyStorePath wrapped to be used with `copyPaths`. */ +void copyStorePathAdapter(ref srcStore, ref dstStore, + const ValidPathInfo & info, RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs); + +/* The more liberal alternative to `copyStorePathAdapter`, useful for remote + stores that do not trust us. */ +void copyOrBuildStorePath(ref srcStore, ref dstStore, + const ValidPathInfo & info, RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs); + /* Copy store paths from one store to another. The paths may be copied in parallel. They are copied in a topologically sorted order (i.e. if A is a reference of B, then A is copied before B), but the set of store paths is not automatically closed; use copyClosure() for that. Returns a map of what each path was copied to the dstStore - as. */ + as. + + The `copyStorePathImpl` parameter allows doing something other than just + copying. For example, this is used with the build hook to allow the other + side to build dependencies we don't have permission to copy. This behavior + isn't just the default that way `nix copy` etc. still can be relied upon to + not build anything. */ std::map copyPaths(ref srcStore, ref dstStore, const StorePathSet & storePaths, RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs, - SubstituteFlag substitute = NoSubstitute); + SubstituteFlag substitute = NoSubstitute, + std::function, ref, const ValidPathInfo &, RepairFlag, CheckSigsFlag)> copyStorePathImpl = copyStorePathAdapter); /* Copy the closure of the specified paths from one store to another. */ From 2dd11f07808c8b86513260b85f35a70cbe9b1249 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 27 Feb 2021 05:33:47 +0000 Subject: [PATCH 06/12] Reenable previously failing trustless remote builder tests --- tests/local.mk | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/local.mk b/tests/local.mk index c8aed0390..2cb65867c 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -19,8 +19,10 @@ nix_tests = \ build-remote-input-addressed.sh \ build-remote-content-addressed-fixed.sh \ build-remote-content-addressed-floating.sh \ + build-remote-trustless-should-pass-0.sh \ build-remote-trustless-should-pass-1.sh \ build-remote-trustless-should-pass-2.sh \ + build-remote-trustless-should-pass-3.sh \ build-remote-trustless-should-fail-0.sh \ ssh-relay.sh \ nar-access.sh \ @@ -47,9 +49,6 @@ nix_tests = \ build.sh \ compute-levels.sh # parallel.sh - # build-remote-content-addressed-fixed.sh # problem with fixed output derivations - # build-remote-trustless-should-pass-0.sh # problem with legacy ssh-store only - # build-remote-trustless-should-pass-3.sh # problem with fixed output derivations install-tests += $(foreach x, $(nix_tests), tests/$(x)) From 6e1e15ffec69de8a3954100389f1d41494f8da8d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 7 Apr 2023 11:13:23 -0400 Subject: [PATCH 07/12] Fix it! --- src/build-remote/build-remote.cc | 19 +++++++++------ tests/build-remote-trustless-after.sh | 2 ++ tests/build-remote-trustless-should-fail-0.sh | 24 +++++++++++++++---- tests/build-remote-trustless-should-pass-0.sh | 4 ++-- tests/build-remote-trustless-should-pass-1.sh | 4 ++-- tests/build-remote-trustless-should-pass-2.sh | 4 ++-- tests/build-remote-trustless-should-pass-3.sh | 6 ++--- tests/build-remote-trustless.sh | 14 +++++------ 8 files changed, 48 insertions(+), 29 deletions(-) create mode 100644 tests/build-remote-trustless-after.sh diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 649ad3e7c..3d4dbc3d6 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -290,14 +290,19 @@ connected: auto drv = store->readDerivation(*drvPath); std::optional optResult; - if (sshStore->isTrustedClient() || drv.type().isCA()) { - // Hijack the inputs paths of the derivation to include all the paths - // that come from the `inputDrvs` set. - // We don’t do that for the derivations whose `inputDrvs` is empty - // because + // If we don't know whether we are trusted (e.g. `ssh://` + // stores), we assume we are. This is neccessary for backwards + // compat. + if (std::optional trust = sshStore->isTrustedClient(); (!trust || *trust) || drv.type().isCA()) { + // Hijack the inputs paths of the derivation to include all + // the paths that come from the `inputDrvs` set. We don’t do + // that for the derivations whose `inputDrvs` is empty + // because: + // // 1. It’s not needed - // 2. Changing the `inputSrcs` set changes the associated output ids, - // which break CA derivations + // + // 2. Changing the `inputSrcs` set changes the associated + // output ids, which break CA derivations if (!drv.inputDrvs.empty()) drv.inputSrcs = store->parseStorePathSet(inputs); optResult = sshStore->buildDerivation(*drvPath, (const BasicDerivation &) drv); diff --git a/tests/build-remote-trustless-after.sh b/tests/build-remote-trustless-after.sh new file mode 100644 index 000000000..19f59e6ae --- /dev/null +++ b/tests/build-remote-trustless-after.sh @@ -0,0 +1,2 @@ +outPath=$(readlink -f $TEST_ROOT/result) +grep 'FOO BAR BAZ' ${remoteDir}/${outPath} diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh index b72c134e7..b5cedb544 100644 --- a/tests/build-remote-trustless-should-fail-0.sh +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -1,11 +1,25 @@ source common.sh -# We act as if remote trusts us, but it doesn't. This fails since we are -# building input-addressed derivations with `buildDerivation`, which -# depends on trust. +[[ $busybox =~ busybox ]] || skipTest "no busybox" + +unset NIX_STORE_DIR +unset NIX_STATE_DIR + +# We first build a dependency of the derivation we eventually want to +# build. +nix-build build-hook.nix -A passthru.input2 \ + -o "$TEST_ROOT/input2" \ + --arg busybox "$busybox" \ + --store "$TEST_ROOT/local" \ + --option system-features bar + +# Now when we go to build that downstream derivation, Nix will try to +# copy our already-build `input2` to the remote store. That store object +# is input-addressed, so this will fail. + file=build-hook.nix prog=$(readlink -e ./nix-daemon-untrusting.sh) proto=ssh-ng -trusting=true -! source build-remote-trustless.sh +expectStderr 1 source build-remote-trustless.sh \ + | grepQuiet "cannot add path '[^ ]*' because it lacks a signature by a trusted key" diff --git a/tests/build-remote-trustless-should-pass-0.sh b/tests/build-remote-trustless-should-pass-0.sh index 82c3f4520..2a7ebd8c6 100644 --- a/tests/build-remote-trustless-should-pass-0.sh +++ b/tests/build-remote-trustless-should-pass-0.sh @@ -1,9 +1,9 @@ source common.sh -# Remote trusts us but we pretend it doesn't. +# Remote trusts us file=build-hook.nix prog=nix-store proto=ssh -trusting=false source build-remote-trustless.sh +source build-remote-trustless-after.sh diff --git a/tests/build-remote-trustless-should-pass-1.sh b/tests/build-remote-trustless-should-pass-1.sh index 22c304bc2..516bdf092 100644 --- a/tests/build-remote-trustless-should-pass-1.sh +++ b/tests/build-remote-trustless-should-pass-1.sh @@ -1,9 +1,9 @@ source common.sh -# Remote trusts us but we pretend it doesn't. +# Remote trusts us file=build-hook.nix prog=nix-daemon proto=ssh-ng -trusting=false source build-remote-trustless.sh +source build-remote-trustless-after.sh diff --git a/tests/build-remote-trustless-should-pass-2.sh b/tests/build-remote-trustless-should-pass-2.sh index 941ddca05..6383f5489 100644 --- a/tests/build-remote-trustless-should-pass-2.sh +++ b/tests/build-remote-trustless-should-pass-2.sh @@ -1,9 +1,9 @@ source common.sh -# Remote doesn't trust us nor do we think it does +# Remote doesn't trust us file=build-hook.nix prog=$(readlink -e ./nix-daemon-untrusting.sh) proto=ssh-ng -trusting=false source build-remote-trustless.sh +source build-remote-trustless-after.sh diff --git a/tests/build-remote-trustless-should-pass-3.sh b/tests/build-remote-trustless-should-pass-3.sh index 0c4e191fc..c3ec359fb 100644 --- a/tests/build-remote-trustless-should-pass-3.sh +++ b/tests/build-remote-trustless-should-pass-3.sh @@ -1,10 +1,10 @@ source common.sh -# We act as if remote trusts us, but it doesn't. This is fine because we -# are only building (fixed) CA derivations. +# Remote doesn't trusts us, but this is fine because we are only +# building (fixed) CA derivations. file=build-hook-ca-fixed.nix prog=$(readlink -e ./nix-daemon-untrusting.sh) proto=ssh-ng -trusting=true source build-remote-trustless.sh +source build-remote-trustless-after.sh diff --git a/tests/build-remote-trustless.sh b/tests/build-remote-trustless.sh index 8260c4dcd..9df44e0c5 100644 --- a/tests/build-remote-trustless.sh +++ b/tests/build-remote-trustless.sh @@ -1,16 +1,14 @@ -if ! canUseSandbox; then exit; fi -if ! [[ $busybox =~ busybox ]]; then exit; fi +requireSandboxSupport +[[ $busybox =~ busybox ]] || skipTest "no busybox" unset NIX_STORE_DIR unset NIX_STATE_DIR +remoteDir=$TEST_ROOT/remote + # Note: ssh{-ng}://localhost bypasses ssh. See tests/build-remote.sh for # more details. -nix build -L -v -f $file -o $TEST_ROOT/result --max-jobs 0 \ +nix-build $file -o $TEST_ROOT/result --max-jobs 0 \ --arg busybox $busybox \ --store $TEST_ROOT/local \ - --builders "$proto://localhost?remote-program=$prog&trusting=$trusting&remote-store=$TEST_ROOT/remote%3Fsystem-features=foo%20bar%20baz - - 1 1 foo,bar,baz" - -outPath=$(readlink -f $TEST_ROOT/result) - -grep 'FOO BAR BAZ' $TEST_ROOT/${subDir}/local${outPath} + --builders "$proto://localhost?remote-program=$prog&remote-store=${remoteDir}%3Fsystem-features=foo%20bar%20baz - - 1 1 foo,bar,baz" From e95db8f2b9aebbb4079805cb7ecfc751af41e0b4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 17 Apr 2023 09:35:43 -0400 Subject: [PATCH 08/12] `nix-testing` -> `daemon-trust-override` And only enable in the tests that need it. This makes it less of a sledgehammer. --- src/libutil/experimental-features.cc | 11 ++++++----- src/libutil/experimental-features.hh | 2 +- src/nix/daemon.cc | 6 +++--- tests/build-remote-trustless-should-fail-0.sh | 4 ++++ tests/build-remote-trustless-should-pass-2.sh | 4 ++++ tests/build-remote-trustless-should-pass-3.sh | 4 ++++ tests/init.sh | 2 +- 7 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index be5a2c088..bd1899662 100644 --- a/src/libutil/experimental-features.cc +++ b/src/libutil/experimental-features.cc @@ -190,12 +190,13 @@ constexpr std::array xpFeatureDetails = {{ )", }, { - .tag = Xp::NixTesting, - .name = "nix-testing", + .tag = Xp::DaemonTrustOverride, + .name = "daemon-trust-override", .description = R"( - A "permanent" experimental feature for extra features we just need - for testing. Not actually an "experiment" in the sense of being - prospective functionality for regular users. + Allow forcing trusting or not trusting clients with + `nix-daemon`. This is useful for testing, but possibly also + useful for various experiments with `nix-daemon --stdio` + networking. )", }, }}; diff --git a/src/libutil/experimental-features.hh b/src/libutil/experimental-features.hh index c41f73fa0..3c00bc4e5 100644 --- a/src/libutil/experimental-features.hh +++ b/src/libutil/experimental-features.hh @@ -28,7 +28,7 @@ enum struct ExperimentalFeature AutoAllocateUids, Cgroups, DiscardReferences, - NixTesting, + DaemonTrustOverride, }; /** diff --git a/src/nix/daemon.cc b/src/nix/daemon.cc index 4be93bb1c..35e8a5f87 100644 --- a/src/nix/daemon.cc +++ b/src/nix/daemon.cc @@ -472,13 +472,13 @@ static int main_nix_daemon(int argc, char * * argv) else if (*arg == "--stdio") stdio = true; else if (*arg == "--force-trusted") { - experimentalFeatureSettings.require(Xp::NixTesting); + experimentalFeatureSettings.require(Xp::DaemonTrustOverride); isTrustedOpt = Trusted; } else if (*arg == "--force-untrusted") { - experimentalFeatureSettings.require(Xp::NixTesting); + experimentalFeatureSettings.require(Xp::DaemonTrustOverride); isTrustedOpt = NotTrusted; } else if (*arg == "--default-trust") { - experimentalFeatureSettings.require(Xp::NixTesting); + experimentalFeatureSettings.require(Xp::DaemonTrustOverride); isTrustedOpt = std::nullopt; } else return false; return true; diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh index b5cedb544..fad1def59 100644 --- a/tests/build-remote-trustless-should-fail-0.sh +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -1,5 +1,9 @@ source common.sh +enableFeatures "daemon-trust-override" + +restartDaemon + [[ $busybox =~ busybox ]] || skipTest "no busybox" unset NIX_STORE_DIR diff --git a/tests/build-remote-trustless-should-pass-2.sh b/tests/build-remote-trustless-should-pass-2.sh index 6383f5489..b769a88f0 100644 --- a/tests/build-remote-trustless-should-pass-2.sh +++ b/tests/build-remote-trustless-should-pass-2.sh @@ -1,5 +1,9 @@ source common.sh +enableFeatures "daemon-trust-override" + +restartDaemon + # Remote doesn't trust us file=build-hook.nix prog=$(readlink -e ./nix-daemon-untrusting.sh) diff --git a/tests/build-remote-trustless-should-pass-3.sh b/tests/build-remote-trustless-should-pass-3.sh index c3ec359fb..40f81da5a 100644 --- a/tests/build-remote-trustless-should-pass-3.sh +++ b/tests/build-remote-trustless-should-pass-3.sh @@ -1,5 +1,9 @@ source common.sh +enableFeatures "daemon-trust-override" + +restartDaemon + # Remote doesn't trusts us, but this is fine because we are only # building (fixed) CA derivations. file=build-hook-ca-fixed.nix diff --git a/tests/init.sh b/tests/init.sh index 2c4f4a2f3..c420e8c9f 100755 --- a/tests/init.sh +++ b/tests/init.sh @@ -20,7 +20,7 @@ cat > "$NIX_CONF_DIR"/nix.conf < Date: Mon, 17 Apr 2023 09:56:32 -0400 Subject: [PATCH 09/12] Improve the build remote comment. --- src/build-remote/build-remote.cc | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 3d4dbc3d6..b0bc8a9ff 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -290,9 +290,30 @@ connected: auto drv = store->readDerivation(*drvPath); std::optional optResult; + + // Let's break this down + // + // ### Trust part + // + // ``` + // std::optional trust = sshStore->isTrustedClient(); (!trust || *trust) + // ``` + // // If we don't know whether we are trusted (e.g. `ssh://` - // stores), we assume we are. This is neccessary for backwards + // stores), we assume we are. This is necessary for backwards // compat. + // + // ### Content-addressing part + // + // ``` + // ...trustCond... || drv.type().isCA() + // ``` + // + // See the very large comment in `case wopBuildDerivation:` in + // `src/libstore/daemon.cc` that explains the trust model here. + // + // This condition mirrors that: that code enforces the "rules"; + // we do the best we can given those "rules". if (std::optional trust = sshStore->isTrustedClient(); (!trust || *trust) || drv.type().isCA()) { // Hijack the inputs paths of the derivation to include all // the paths that come from the `inputDrvs` set. We don’t do From 23ee2d79a90ace2fe012f32c37121f50afc35223 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 17 Apr 2023 09:59:14 -0400 Subject: [PATCH 10/12] Use `buildPathsWithResults` in build-remote.cc trustless path It handles failures more correctly; I am glad we have it now! --- src/build-remote/build-remote.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index b0bc8a9ff..403f0fb17 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -332,7 +332,10 @@ connected: throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg); } else { copyPaths(*store, *sshStore, StorePathSet {*drvPath}, NoRepair, NoCheckSigs, substitute); - sshStore->buildPaths({ DerivedPath::Built { *drvPath, OutputsSpec::All {} } }); + auto res = sshStore->buildPathsWithResults({ DerivedPath::Built { *drvPath, OutputsSpec::All {} } }); + // One path to build should mean one result back + assert(res.size() == 1); + optResult = std::move(res[0]); } From cd0d8e0bd50d7259057ac138a7227907f88b91d5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 8 May 2023 09:57:05 -0400 Subject: [PATCH 11/12] Apply suggestions from code review Co-authored-by: Valentin Gagarin --- src/build-remote/build-remote.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 1467761c9..7366a7c27 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -312,9 +312,12 @@ connected: // See the very large comment in `case wopBuildDerivation:` in // `src/libstore/daemon.cc` that explains the trust model here. // - // This condition mirrors that: that code enforces the "rules"; + // This condition mirrors that: that code enforces the "rules" outlined there; // we do the best we can given those "rules". - if (std::optional trust = sshStore->isTrustedClient(); (!trust || *trust) || drv.type().isCA()) { + std::optional trusted = sshStore->isTrustedClient(); + // for backward compatibility (use existing comments here) + bool trustedOrLegacy = !trusted || *trusted; + if (trustedOrLegacy || drv.type().isCA()) { // Hijack the inputs paths of the derivation to include all // the paths that come from the `inputDrvs` set. We don’t do // that for the derivations whose `inputDrvs` is empty @@ -331,9 +334,9 @@ connected: if (!result.success()) throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg); } else { - copyPaths(*store, *sshStore, StorePathSet {*drvPath}, NoRepair, NoCheckSigs, substitute); + copyClosure(*store, *sshStore, StorePathSet {*drvPath}, NoRepair, NoCheckSigs, substitute); auto res = sshStore->buildPathsWithResults({ DerivedPath::Built { *drvPath, OutputsSpec::All {} } }); - // One path to build should mean one result back + // One path to build should produce exactly one build result assert(res.size() == 1); optResult = std::move(res[0]); } From df53a7d26868d7ce432e34e5d0c397886f7772f8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 8 May 2023 10:08:01 -0400 Subject: [PATCH 12/12] Split comment, match with each variable --- src/build-remote/build-remote.cc | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 7366a7c27..323e04fdb 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -291,32 +291,19 @@ connected: std::optional optResult; - // Let's break this down - // - // ### Trust part - // - // ``` - // std::optional trust = sshStore->isTrustedClient(); (!trust || *trust) - // ``` - // // If we don't know whether we are trusted (e.g. `ssh://` // stores), we assume we are. This is necessary for backwards // compat. - // - // ### Content-addressing part - // - // ``` - // ...trustCond... || drv.type().isCA() - // ``` - // + bool trustedOrLegacy = ({ + std::optional trusted = sshStore->isTrustedClient(); + !trusted || *trusted; + }); + // See the very large comment in `case wopBuildDerivation:` in // `src/libstore/daemon.cc` that explains the trust model here. // // This condition mirrors that: that code enforces the "rules" outlined there; // we do the best we can given those "rules". - std::optional trusted = sshStore->isTrustedClient(); - // for backward compatibility (use existing comments here) - bool trustedOrLegacy = !trusted || *trusted; if (trustedOrLegacy || drv.type().isCA()) { // Hijack the inputs paths of the derivation to include all // the paths that come from the `inputDrvs` set. We don’t do