From 29bd63e9907cabc5643aaa3f570b9ff5b2d88268 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Dec 2020 19:55:21 +0000 Subject: [PATCH 1/9] Test nix-instantiate with binary cache store Trying to make sure it work with obscurers stores. --- tests/binary-cache.sh | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/binary-cache.sh b/tests/binary-cache.sh index 92ed36225..8f1c6f14d 100644 --- a/tests/binary-cache.sh +++ b/tests/binary-cache.sh @@ -1,15 +1,20 @@ source common.sh +# We can produce drvs directly into the binary cache clearStore -clearCache +clearCacheCache +nix-instantiate --store "file://$cacheDir" dependencies.nix # Create the binary cache. +clearStore +clearCache outPath=$(nix-build dependencies.nix --no-out-link) nix copy --to file://$cacheDir $outPath -basicTests() { +basicDownloadTests() { + # No uploading tests bcause upload with force HTTP doesn't work. # By default, a binary cache doesn't support "nix-env -qas", but does # support installation. @@ -44,12 +49,12 @@ basicTests() { # Test LocalBinaryCacheStore. -basicTests +basicDownloadTests # Test HttpBinaryCacheStore. export _NIX_FORCE_HTTP=1 -basicTests +basicDownloadTests # Test whether Nix notices if the NAR doesn't match the hash in the NAR info. From 57062179ce36e35715284d2ef570f8cb0b90198d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Dec 2020 16:05:09 +0000 Subject: [PATCH 2/9] Move some PKI stuff from LocalStore to Store --- src/libstore/local-store.cc | 9 --------- src/libstore/local-store.hh | 12 ------------ src/libstore/misc.cc | 9 +++++++++ src/libstore/store-api.hh | 13 +++++++++++++ 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index c52d4b62a..1eb2dec75 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1092,15 +1092,6 @@ void LocalStore::invalidatePath(State & state, const StorePath & path) } -const PublicKeys & LocalStore::getPublicKeys() -{ - auto state(_state.lock()); - if (!state->publicKeys) - state->publicKeys = std::make_unique(getDefaultPublicKeys()); - return *state->publicKeys; -} - - void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) { diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index ae9497b2e..d97645058 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -35,10 +35,6 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig { using LocalFSStoreConfig::LocalFSStoreConfig; - Setting requireSigs{(StoreConfig*) this, - settings.requireSigs, - "require-sigs", "whether store paths should have a trusted signature on import"}; - const std::string name() override { return "Local Store"; } }; @@ -75,8 +71,6 @@ private: minFree but not much below availAfterGC, then there is no point in starting a new GC. */ uint64_t availAfterGC = std::numeric_limits::max(); - - std::unique_ptr publicKeys; }; Sync _state; @@ -94,12 +88,6 @@ public: const Path tempRootsDir; const Path fnTempRoots; -private: - - const PublicKeys & getPublicKeys(); - -public: - // Hack for build-remote.cc. PathSet locksHeld; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index ad4dccef9..0d4190a56 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -282,4 +282,13 @@ StorePaths Store::topoSortPaths(const StorePathSet & paths) } +const PublicKeys & Store::getPublicKeys() +{ + auto cryptoState(_cryptoState.lock()); + if (!cryptoState->publicKeys) + cryptoState->publicKeys = std::make_unique(getDefaultPublicKeys()); + return *cryptoState->publicKeys; +} + + } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 9bcff08eb..e3de6db17 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -189,6 +189,10 @@ struct StoreConfig : public Config const Setting isTrusted{this, false, "trusted", "whether paths from this store can be used as substitutes even when they lack trusted signatures"}; + Setting requireSigs{this, + settings.requireSigs, + "require-sigs", "whether store paths should have a trusted signature on import"}; + Setting priority{this, 0, "priority", "priority of this substituter (lower value means higher priority)"}; Setting wantMassQuery{this, false, "want-mass-query", "whether this substituter can be queried efficiently for path validity"}; @@ -710,11 +714,20 @@ public: return toRealPath(printStorePath(storePath)); } + const PublicKeys & getPublicKeys(); + virtual void createUser(const std::string & userName, uid_t userId) { } protected: + struct CryptoState + { + std::unique_ptr publicKeys; + }; + + Sync _cryptoState; + Stats stats; /* Unsupported methods. */ From 12f7a1f65becfe3b036d0f840ee4a05f2f1f857c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Dec 2020 17:07:28 +0000 Subject: [PATCH 3/9] build-remote no longer requires local store be local --- src/build-remote/build-remote.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 8348d8c91..350bd6cef 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -71,11 +71,15 @@ static int main_build_remote(int argc, char * * argv) initPlugins(); - auto store = openStore().cast(); + auto store = openStore(); /* It would be more appropriate to use $XDG_RUNTIME_DIR, since that gets cleared on reboot, but it wouldn't work on macOS. */ - currentLoad = store->stateDir + "/current-load"; + currentLoad = "/current-load"; + if (auto localStore = store.dynamic_pointer_cast()) + currentLoad = std::string { localStore->stateDir } + currentLoad; + else + currentLoad = settings.nixStateDir + currentLoad; std::shared_ptr sshStore; AutoCloseFD bestSlotLock; @@ -288,8 +292,9 @@ connected: if (!missing.empty()) { Activity act(*logger, lvlTalkative, actUnknown, fmt("copying outputs from '%s'", storeUri)); - for (auto & i : missing) - store->locksHeld.insert(store->printStorePath(i)); /* FIXME: ugly */ + if (auto localStore = store.dynamic_pointer_cast()) + for (auto & i : missing) + localStore->locksHeld.insert(store->printStorePath(i)); /* FIXME: ugly */ copyPaths(ref(sshStore), store, missing, NoRepair, NoCheckSigs, NoSubstitute); } From 450c3500f1e3fb619636c0a29d65300020f99d7d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Dec 2020 17:36:52 +0000 Subject: [PATCH 4/9] Crudely make worker only provide a Store, not LocalStore We downcast in a few places, this will be refactored to be better later. --- src/libstore/build/derivation-goal.cc | 66 ++++++++++++++++++--------- src/libstore/build/worker.cc | 6 ++- src/libstore/build/worker.hh | 9 ++-- 3 files changed, 54 insertions(+), 27 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 47d11dc53..de32f60db 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -848,14 +848,16 @@ void DerivationGoal::buildDone() So instead, check if the disk is (nearly) full now. If so, we don't mark this build as a permanent failure. */ #if HAVE_STATVFS - uint64_t required = 8ULL * 1024 * 1024; // FIXME: make configurable - struct statvfs st; - if (statvfs(worker.store.realStoreDir.c_str(), &st) == 0 && - (uint64_t) st.f_bavail * st.f_bsize < required) - diskFull = true; - if (statvfs(tmpDir.c_str(), &st) == 0 && - (uint64_t) st.f_bavail * st.f_bsize < required) - diskFull = true; + if (auto localStore = dynamic_cast(&worker.store)) { + uint64_t required = 8ULL * 1024 * 1024; // FIXME: make configurable + struct statvfs st; + if (statvfs(localStore->realStoreDir.c_str(), &st) == 0 && + (uint64_t) st.f_bavail * st.f_bsize < required) + diskFull = true; + if (statvfs(tmpDir.c_str(), &st) == 0 && + (uint64_t) st.f_bavail * st.f_bsize < required) + diskFull = true; + } #endif deleteTmpDir(false); @@ -1215,12 +1217,15 @@ void DerivationGoal::startBuilder() useChroot = !(derivationIsImpure(derivationType)) && !noChroot; } - if (worker.store.storeDir != worker.store.realStoreDir) { - #if __linux__ - useChroot = true; - #else - throw Error("building using a diverted store is not supported on this platform"); - #endif + if (auto localStoreP = dynamic_cast(&worker.store)) { + auto & localStore = *localStoreP; + if (localStore.storeDir != localStore.realStoreDir) { + #if __linux__ + useChroot = true; + #else + throw Error("building using a diverted store is not supported on this platform"); + #endif + } } /* Create a temporary directory where the build will take @@ -2182,7 +2187,8 @@ void DerivationGoal::startDaemon() Store::Params params; params["path-info-cache-size"] = "0"; params["store"] = worker.store.storeDir; - params["root"] = worker.store.rootDir; + if (auto localStore = dynamic_cast(&worker.store)) + params["root"] = localStore->rootDir; params["state"] = "/no-such-path"; params["log"] = "/no-such-path"; auto store = make_ref(params, @@ -3246,7 +3252,13 @@ void DerivationGoal::registerOutputs() } } + auto localStoreP = dynamic_cast(&worker.store); + if (!localStoreP) + Unsupported("Can only register outputs with local store"); + auto & localStore = *localStoreP; + if (buildMode == bmCheck) { + if (!worker.store.isValidPath(newInfo.path)) continue; ValidPathInfo oldInfo(*worker.store.queryPathInfo(newInfo.path)); if (newInfo.narHash != oldInfo.narHash) { @@ -3271,8 +3283,8 @@ void DerivationGoal::registerOutputs() /* Since we verified the build, it's now ultimately trusted. */ if (!oldInfo.ultimate) { oldInfo.ultimate = true; - worker.store.signPathInfo(oldInfo); - worker.store.registerValidPaths({{oldInfo.path, oldInfo}}); + localStore.signPathInfo(oldInfo); + localStore.registerValidPaths({{oldInfo.path, oldInfo}}); } continue; @@ -3288,13 +3300,13 @@ void DerivationGoal::registerOutputs() } if (curRound == nrRounds) { - worker.store.optimisePath(actualPath); // FIXME: combine with scanForReferences() + localStore.optimisePath(actualPath); // FIXME: combine with scanForReferences() worker.markContentsGood(newInfo.path); } newInfo.deriver = drvPath; newInfo.ultimate = true; - worker.store.signPathInfo(newInfo); + localStore.signPathInfo(newInfo); finish(newInfo.path); @@ -3302,7 +3314,7 @@ void DerivationGoal::registerOutputs() isn't statically known so that we can safely unlock the path before the next iteration */ if (newInfo.ca) - worker.store.registerValidPaths({{newInfo.path, newInfo}}); + localStore.registerValidPaths({{newInfo.path, newInfo}}); infos.emplace(outputName, std::move(newInfo)); } @@ -3375,11 +3387,16 @@ void DerivationGoal::registerOutputs() paths referenced by each of them. If there are cycles in the outputs, this will fail. */ { + auto localStoreP = dynamic_cast(&worker.store); + if (!localStoreP) + Unsupported("Can only register outputs with local store"); + auto & localStore = *localStoreP; + ValidPathInfos infos2; for (auto & [outputName, newInfo] : infos) { infos2.insert_or_assign(newInfo.path, newInfo); } - worker.store.registerValidPaths(infos2); + localStore.registerValidPaths(infos2); } /* In case of a fixed-output derivation hash mismatch, throw an @@ -3577,7 +3594,12 @@ Path DerivationGoal::openLogFile() auto baseName = std::string(baseNameOf(worker.store.printStorePath(drvPath))); /* Create a log file. */ - Path dir = fmt("%s/%s/%s/", worker.store.logDir, worker.store.drvsLogDir, string(baseName, 0, 2)); + Path logDir; + if (auto localStore = dynamic_cast(&worker.store)) + logDir = localStore->logDir; + else + logDir = settings.nixLogDir; + Path dir = fmt("%s/%s/%s/", logDir, LocalFSStore::drvsLogDir, string(baseName, 0, 2)); createDirs(dir); Path logFileName = fmt("%s/%s%s", dir, string(baseName, 2), diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 6c96a93bd..a9575fb0f 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -8,7 +8,7 @@ namespace nix { -Worker::Worker(LocalStore & store) +Worker::Worker(Store & store) : act(*logger, actRealise) , actDerivations(*logger, actBuilds) , actSubstitutions(*logger, actCopyPaths) @@ -229,7 +229,9 @@ void Worker::run(const Goals & _topGoals) checkInterrupt(); - store.autoGC(false); + // TODO GC interface? + if (auto localStore = dynamic_cast(&store)) + localStore->autoGC(false); /* Call every wake goal (in the ordering established by CompareGoalPtrs). */ diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index bf8cc4586..82e711191 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -2,9 +2,12 @@ #include "types.hh" #include "lock.hh" -#include "local-store.hh" +#include "store-api.hh" #include "goal.hh" +#include +#include + namespace nix { /* Forward definition. */ @@ -102,7 +105,7 @@ public: /* Set if at least one derivation is not deterministic in check mode. */ bool checkMismatch; - LocalStore & store; + Store & store; std::unique_ptr hook; @@ -124,7 +127,7 @@ public: it answers with "decline-permanently", we don't try again. */ bool tryBuildHook = true; - Worker(LocalStore & store); + Worker(Store & store); ~Worker(); /* Make a goal (with caching). */ From 85f2e9e8fa4f7452a05cfffc901d118a7c861d0a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Dec 2020 17:54:57 +0000 Subject: [PATCH 5/9] Expose schedule entrypoints to all stores Remote stores still override so the other end schedules. --- src/libstore/binary-cache-store.hh | 7 ------ .../{local-store-build.cc => entry-points.cc} | 6 ++--- src/libstore/dummy-store.cc | 7 ------ src/libstore/local-store.hh | 9 -------- src/libstore/store-api.cc | 23 ------------------- src/libstore/store-api.hh | 6 ++--- 6 files changed, 6 insertions(+), 52 deletions(-) rename src/libstore/build/{local-store-build.cc => entry-points.cc} (91%) diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 443a53cac..c2163166c 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -108,13 +108,6 @@ public: void narFromPath(const StorePath & path, Sink & sink) override; - BuildResult buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, - BuildMode buildMode) override - { unsupported("buildDerivation"); } - - void ensurePath(const StorePath & path) override - { unsupported("ensurePath"); } - ref getFSAccessor() override; void addSignatures(const StorePath & storePath, const StringSet & sigs) override; diff --git a/src/libstore/build/local-store-build.cc b/src/libstore/build/entry-points.cc similarity index 91% rename from src/libstore/build/local-store-build.cc rename to src/libstore/build/entry-points.cc index c91cda2fd..9f97d40ba 100644 --- a/src/libstore/build/local-store-build.cc +++ b/src/libstore/build/entry-points.cc @@ -5,7 +5,7 @@ namespace nix { -void LocalStore::buildPaths(const std::vector & drvPaths, BuildMode buildMode) +void Store::buildPaths(const std::vector & drvPaths, BuildMode buildMode) { Worker worker(*this); @@ -43,7 +43,7 @@ void LocalStore::buildPaths(const std::vector & drvPaths, } } -BuildResult LocalStore::buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, +BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, BuildMode buildMode) { Worker worker(*this); @@ -63,7 +63,7 @@ BuildResult LocalStore::buildDerivation(const StorePath & drvPath, const BasicDe } -void LocalStore::ensurePath(const StorePath & path) +void Store::ensurePath(const StorePath & path) { /* If the path is already valid, we're done. */ if (isValidPath(path)) return; diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 3c7caf8f2..8f26af685 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -55,13 +55,6 @@ struct DummyStore : public virtual DummyStoreConfig, public virtual Store void narFromPath(const StorePath & path, Sink & sink) override { unsupported("narFromPath"); } - void ensurePath(const StorePath & path) override - { unsupported("ensurePath"); } - - BuildResult buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, - BuildMode buildMode) override - { unsupported("buildDerivation"); } - std::optional queryRealisation(const DrvOutput&) override { unsupported("queryRealisation"); } }; diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index d97645058..aa5de31f0 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -133,15 +133,6 @@ public: StorePath addTextToStore(const string & name, const string & s, const StorePathSet & references, RepairFlag repair) override; - void buildPaths( - const std::vector & paths, - BuildMode buildMode) override; - - BuildResult buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, - BuildMode buildMode) override; - - void ensurePath(const StorePath & path) override; - void addTempRoot(const StorePath & path) override; void addIndirectRoot(const Path & path) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 7aca22bde..f12a564a1 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -747,29 +747,6 @@ const Store::Stats & Store::getStats() } -void Store::buildPaths(const std::vector & paths, BuildMode buildMode) -{ - StorePathSet paths2; - - for (auto & path : paths) { - if (path.path.isDerivation()) { - auto outPaths = queryPartialDerivationOutputMap(path.path); - for (auto & outputName : path.outputs) { - auto currentOutputPathIter = outPaths.find(outputName); - if (currentOutputPathIter == outPaths.end() || - !currentOutputPathIter->second || - !isValidPath(*currentOutputPathIter->second)) - unsupported("buildPaths"); - } - } else - paths2.insert(path.path); - } - - if (queryValidPaths(paths2).size() != paths2.size()) - unsupported("buildPaths"); -} - - void copyStorePath(ref srcStore, ref dstStore, const StorePath & storePath, RepairFlag repair, CheckSigsFlag checkSigs) { diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index e3de6db17..4db980fe9 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -523,17 +523,17 @@ public: explicitly choosing to allow it). */ virtual BuildResult buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, - BuildMode buildMode = bmNormal) = 0; + BuildMode buildMode = bmNormal); /* Ensure that a path is valid. If it is not currently valid, it may be made valid by running a substitute (if defined for the path). */ - virtual void ensurePath(const StorePath & path) = 0; + virtual void ensurePath(const StorePath & path); /* Add a store path as a temporary root of the garbage collector. The root disappears as soon as we exit. */ virtual void addTempRoot(const StorePath & path) - { unsupported("addTempRoot"); } + { warn("not creating temp root, store doesn't support GC"); } /* Add an indirect root, which is merely a symlink to `path' from /nix/var/nix/gcroots/auto/. `path' is supposed From fed123724679de89d3f56a4c01b5c4c96f93e584 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 20 Dec 2020 19:55:21 +0000 Subject: [PATCH 6/9] Test nix-build with non-local-store --store Just a few small things needed fixing! --- src/libstore/build/derivation-goal.cc | 20 +++++++++++++++++--- tests/binary-cache-build-remote.sh | 13 +++++++++++++ tests/local.mk | 4 +++- 3 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 tests/binary-cache-build-remote.sh diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index de32f60db..17f39a86e 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -592,9 +592,17 @@ void DerivationGoal::tryToBuild() PathSet lockFiles; /* FIXME: Should lock something like the drv itself so we don't build same CA drv concurrently */ - for (auto & i : drv->outputsAndOptPaths(worker.store)) - if (i.second.second) - lockFiles.insert(worker.store.Store::toRealPath(*i.second.second)); + if (dynamic_cast(&worker.store)) + /* If we aren't a local store, we might need to use the local store as + a build remote, but that would cause a deadlock. */ + /* FIXME: Make it so we can use ourselves as a build remote even if we + are the local store (separate locking for building vs scheduling? */ + /* FIXME: find some way to lock for scheduling for the other stores so + a forking daemon with --store still won't farm out redundant builds. + */ + for (auto & i : drv->outputsAndOptPaths(worker.store)) + if (i.second.second) + lockFiles.insert(worker.store.Store::toRealPath(*i.second.second)); if (!outputLocks.lockPaths(lockFiles, "", false)) { if (!actLock) @@ -680,6 +688,12 @@ void DerivationGoal::tryLocalBuild() { /* Make sure that we are allowed to start a build. If this derivation prefers to be done locally, do it even if maxBuildJobs is 0. */ + if (!dynamic_cast(&worker.store)) { + throw Error( + "unable to build with a primary store that isn't a local store; " + "either pass a different '--store' or enable remote builds." + "\nhttps://nixos.org/nix/manual/#chap-distributed-builds"); + } unsigned int curBuilds = worker.getNrLocalBuilds(); if (curBuilds >= settings.maxBuildJobs && !(buildLocally && curBuilds == 0)) { worker.waitForBuildSlot(shared_from_this()); diff --git a/tests/binary-cache-build-remote.sh b/tests/binary-cache-build-remote.sh new file mode 100644 index 000000000..ed51164a4 --- /dev/null +++ b/tests/binary-cache-build-remote.sh @@ -0,0 +1,13 @@ +source common.sh + +clearStore +clearCacheCache + +# Fails without remote builders +(! nix-build --store "file://$cacheDir" dependencies.nix) + +# Succeeds with default store as build remote. +nix-build --store "file://$cacheDir" --builders 'auto - - 1 1' -j0 dependencies.nix + +# Succeeds without any build capability because no-op +nix-build --store "file://$cacheDir" -j0 dependencies.nix diff --git a/tests/local.mk b/tests/local.mk index ce94ec80e..aa8b4f9bf 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -9,7 +9,9 @@ nix_tests = \ local-store.sh remote-store.sh export.sh export-graph.sh \ timeout.sh secure-drv-outputs.sh nix-channel.sh \ multiple-outputs.sh import-derivation.sh fetchurl.sh optimise-store.sh \ - binary-cache.sh nix-profile.sh repair.sh dump-db.sh case-hack.sh \ + binary-cache.sh \ + binary-cache-build-remote.sh \ + nix-profile.sh repair.sh dump-db.sh case-hack.sh \ check-reqs.sh pass-as-file.sh tarball.sh restricted.sh \ placeholders.sh nix-shell.sh \ linux-sandbox.sh \ From 7af743470c09b835f910d2e25786c080ccfe52c1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 15 Jan 2021 16:37:41 +0000 Subject: [PATCH 7/9] Make public keys and `requireSigs` local-store specific again Thanks @regnat and @edolstra for catching this and comming up with the solution. They way I had generalized those is wrong, because local settings for non-local stores is confusing default. And due to the nature of C++ inheritance, fixing the defaults is more annoying than it should be. Additionally, I thought we might just drop the check in the substitution logic since `Store::addToStore` is now streaming, but @regnat rightfully pointed out that as it downloads dependencies first, that would still be too late, and also waste effort on possibly unneeded/unwanted dependencies. The simple and correct thing to do is just make a store method for the boolean logic, keeping all the setting and key stuff the way it was before. That new method is both used by `LocalStore::addToStore` and the substitution goal check. Perhaps we might eventually make it fancier, e.g. sending the ValidPathInfo to remote stores for them to validate, but this is good enough for now. --- src/libstore/build/substitution-goal.cc | 4 +--- src/libstore/local-store.cc | 14 ++++++++++++- src/libstore/local-store.hh | 14 +++++++++++++ src/libstore/misc.cc | 9 -------- src/libstore/store-api.hh | 28 +++++++++++++------------ 5 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index d16584f65..f3c9040bc 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -142,9 +142,7 @@ void SubstitutionGoal::tryNext() /* Bail out early if this substituter lacks a valid signature. LocalStore::addToStore() also checks for this, but only after we've downloaded the path. */ - if (worker.store.requireSigs - && !sub->isTrusted - && !info->checkSignatures(worker.store, worker.store.getPublicKeys())) + if (!sub->isTrusted && worker.store.pathInfoIsTrusted(*info)) { logWarning({ .name = "Invalid path signature", diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 4f48522c6..d6d74a0b0 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1098,11 +1098,23 @@ void LocalStore::invalidatePath(State & state, const StorePath & path) } } +const PublicKeys & LocalStore::getPublicKeys() +{ + auto state(_state.lock()); + if (!state->publicKeys) + state->publicKeys = std::make_unique(getDefaultPublicKeys()); + return *state->publicKeys; +} + +bool LocalStore::pathInfoIsTrusted(const ValidPathInfo & info) +{ + return requireSigs && !info.checkSignatures(*this, getPublicKeys()); +} void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) { - if (requireSigs && checkSigs && !info.checkSignatures(*this, getPublicKeys())) + if (checkSigs && pathInfoIsTrusted(info)) throw Error("cannot add path '%s' because it lacks a valid signature", printStorePath(info.path)); addTempRoot(info.path); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 69704d266..9d235ba0a 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -35,6 +35,10 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig { using LocalFSStoreConfig::LocalFSStoreConfig; + Setting requireSigs{(StoreConfig*) this, + settings.requireSigs, + "require-sigs", "whether store paths should have a trusted signature on import"}; + const std::string name() override { return "Local Store"; } }; @@ -71,6 +75,8 @@ private: minFree but not much below availAfterGC, then there is no point in starting a new GC. */ uint64_t availAfterGC = std::numeric_limits::max(); + + std::unique_ptr publicKeys; }; Sync _state; @@ -88,6 +94,12 @@ public: const Path tempRootsDir; const Path fnTempRoots; +private: + + const PublicKeys & getPublicKeys(); + +public: + // Hack for build-remote.cc. PathSet locksHeld; @@ -124,6 +136,8 @@ public: void querySubstitutablePathInfos(const StorePathCAMap & paths, SubstitutablePathInfos & infos) override; + bool pathInfoIsTrusted(const ValidPathInfo &) override; + void addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 0d4190a56..ad4dccef9 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -282,13 +282,4 @@ StorePaths Store::topoSortPaths(const StorePathSet & paths) } -const PublicKeys & Store::getPublicKeys() -{ - auto cryptoState(_cryptoState.lock()); - if (!cryptoState->publicKeys) - cryptoState->publicKeys = std::make_unique(getDefaultPublicKeys()); - return *cryptoState->publicKeys; -} - - } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index e6a14afc3..3221cf249 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -189,10 +189,6 @@ struct StoreConfig : public Config const Setting isTrusted{this, false, "trusted", "whether paths from this store can be used as substitutes even when they lack trusted signatures"}; - Setting requireSigs{this, - settings.requireSigs, - "require-sigs", "whether store paths should have a trusted signature on import"}; - Setting priority{this, 0, "priority", "priority of this substituter (lower value means higher priority)"}; Setting wantMassQuery{this, false, "want-mass-query", "whether this substituter can be queried efficiently for path validity"}; @@ -376,6 +372,21 @@ public: void queryPathInfo(const StorePath & path, Callback> callback) noexcept; + /* Check whether the given valid path info is sufficiently well-formed + (e.g. hash content-address or signature) in order to be included in the + given store. + + These same checks would be performed in addToStore, but this allows an + earlier failure in the case where dependencies need to be added too, but + the addToStore wouldn't fail until those dependencies are added. Also, + we don't really want to add the dependencies listed in a nar info we + don't trust anyyways. + */ + virtual bool pathInfoIsTrusted(const ValidPathInfo &) + { + return true; + } + protected: virtual void queryPathInfoUncached(const StorePath & path, @@ -719,20 +730,11 @@ public: return toRealPath(printStorePath(storePath)); } - const PublicKeys & getPublicKeys(); - virtual void createUser(const std::string & userName, uid_t userId) { } protected: - struct CryptoState - { - std::unique_ptr publicKeys; - }; - - Sync _cryptoState; - Stats stats; /* Unsupported methods. */ From 8c07ed1ddad6595cd679181b0b8d78e09fc6d152 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 22 Jan 2021 15:27:55 +0000 Subject: [PATCH 8/9] Improve documentation and test and requested --- src/libstore/store-api.hh | 6 +++--- tests/binary-cache-build-remote.sh | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 3221cf249..9e98eb8f9 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -372,9 +372,9 @@ public: void queryPathInfo(const StorePath & path, Callback> callback) noexcept; - /* Check whether the given valid path info is sufficiently well-formed - (e.g. hash content-address or signature) in order to be included in the - given store. + /* Check whether the given valid path info is sufficiently attested, by + either being signed by a trusted public key or content-addressed, in + order to be included in the given store. These same checks would be performed in addToStore, but this allows an earlier failure in the case where dependencies need to be added too, but diff --git a/tests/binary-cache-build-remote.sh b/tests/binary-cache-build-remote.sh index ed51164a4..81cd21a4a 100644 --- a/tests/binary-cache-build-remote.sh +++ b/tests/binary-cache-build-remote.sh @@ -7,7 +7,10 @@ clearCacheCache (! nix-build --store "file://$cacheDir" dependencies.nix) # Succeeds with default store as build remote. -nix-build --store "file://$cacheDir" --builders 'auto - - 1 1' -j0 dependencies.nix +outPath=$(nix-build --store "file://$cacheDir" --builders 'auto - - 1 1' -j0 dependencies.nix) + +# Test that the path exactly exists in the destination store. +nix path-info --store "file://$cacheDir" $outPath # Succeeds without any build capability because no-op nix-build --store "file://$cacheDir" -j0 dependencies.nix From 53a709535b42197a9abd3fe46406bb186ad6c751 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 22 Jan 2021 10:21:12 -0500 Subject: [PATCH 9/9] Apply suggestions from code review Thanks! Co-authored-by: Eelco Dolstra --- src/build-remote/build-remote.cc | 6 +++--- src/libstore/build/derivation-goal.cc | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 350bd6cef..68af3e966 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -75,11 +75,11 @@ static int main_build_remote(int argc, char * * argv) /* It would be more appropriate to use $XDG_RUNTIME_DIR, since that gets cleared on reboot, but it wouldn't work on macOS. */ - currentLoad = "/current-load"; + auto currentLoadName = "/current-load"; if (auto localStore = store.dynamic_pointer_cast()) - currentLoad = std::string { localStore->stateDir } + currentLoad; + currentLoad = std::string { localStore->stateDir } + currentLoadName; else - currentLoad = settings.nixStateDir + currentLoad; + currentLoad = settings.nixStateDir + currentLoadName; std::shared_ptr sshStore; AutoCloseFD bestSlotLock; diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 953e241d8..fa8b99118 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -3291,7 +3291,7 @@ void DerivationGoal::registerOutputs() auto localStoreP = dynamic_cast(&worker.store); if (!localStoreP) - Unsupported("Can only register outputs with local store"); + throw Unsupported("can only register outputs with local store, but this is %s", worker.store.getUri()); auto & localStore = *localStoreP; if (buildMode == bmCheck) { @@ -3426,7 +3426,7 @@ void DerivationGoal::registerOutputs() { auto localStoreP = dynamic_cast(&worker.store); if (!localStoreP) - Unsupported("Can only register outputs with local store"); + throw Unsupported("can only register outputs with local store, but this is %s", worker.store.getUri()); auto & localStore = *localStoreP; ValidPathInfos infos2;