From 29be7958e47d96c813254a5341ff4da74c615ded Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 19 Nov 2023 10:28:52 -0500 Subject: [PATCH] Make building only require a `LocalFSStore` This is the crucial first step to allowing client-side building --- i.e. building locally but using some form of RPC to manipulate metadata rather than interact with a concrete form of persisting metadata like the SQLite database. To facilitate this, a few methods were hastily moved from `LocalStore` to `LocalFSStore`. We should not just accept that as good enough to merge, but carefully consider the trust/security implications of this, and the proper division of labor between the less-trusted "builder" and the more-trusted "store". --- src/libstore/build/derivation-goal.cc | 4 +-- src/libstore/build/local-derivation-goal.cc | 24 +++++++-------- src/libstore/build/local-derivation-goal.hh | 4 +-- src/libstore/build/worker.cc | 6 ++-- src/libstore/local-fs-store.hh | 34 +++++++++++++++++++++ src/libstore/local-store.hh | 16 +++------- 6 files changed, 58 insertions(+), 30 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 81eef7c4748..fb9b8850ac4 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -668,7 +668,7 @@ void DerivationGoal::tryToBuild() PathSet lockFiles; /* FIXME: Should lock something like the drv itself so we don't build same CA drv concurrently */ - if (dynamic_cast(&worker.store)) { + 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 @@ -1240,7 +1240,7 @@ Path DerivationGoal::openLogFile() /* Create a log file. */ Path logDir; - if (auto localStore = dynamic_cast(&worker.store)) + if (auto localStore = dynamic_cast(&worker.store)) logDir = localStore->logDir; else logDir = settings.nixLogDir; diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2bb58df55ff..4e219c602f3 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -121,9 +121,9 @@ inline bool LocalDerivationGoal::needsHashRewrite() } -LocalStore & LocalDerivationGoal::getLocalStore() +LocalFSStore & LocalDerivationGoal::getLocalFSStore() { - auto p = dynamic_cast(&worker.store); + auto p = dynamic_cast(&worker.store); assert(p); return *p; } @@ -204,7 +204,7 @@ void LocalDerivationGoal::tryLocalBuild() useChroot = derivationType->isSandboxed() && !noChroot; } - auto & localStore = getLocalStore(); + auto & localStore = getLocalFSStore(); if (localStore.storeDir != localStore.realStoreDir.get()) { #if __linux__ useChroot = true; @@ -343,7 +343,7 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() so, we don't mark this build as a permanent failure. */ #if HAVE_STATVFS { - auto & localStore = getLocalStore(); + auto & localStore = getLocalFSStore(); uint64_t required = 8ULL * 1024 * 1024; // FIXME: make configurable struct statvfs st; if (statvfs(localStore.realStoreDir.get().c_str(), &st) == 0 && @@ -1223,16 +1223,16 @@ struct RestrictedStoreConfig : virtual LocalFSStoreConfig const std::string name() { return "Restricted Store"; } }; -/* A wrapper around LocalStore that only allows building/querying of +/* A wrapper around LocalFSStore that only allows building/querying of paths that are in the input closures of the build or were added via recursive Nix calls. */ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual IndirectRootStore, public virtual GcStore { - ref next; + ref next; LocalDerivationGoal & goal; - RestrictedStore(const Params & params, ref next, LocalDerivationGoal & goal) + RestrictedStore(const Params & params, ref next, LocalDerivationGoal & goal) : StoreConfig(params) , LocalFSStoreConfig(params) , RestrictedStoreConfig(params) @@ -1459,12 +1459,12 @@ void LocalDerivationGoal::startDaemon() Store::Params params; params["path-info-cache-size"] = "0"; params["store"] = worker.store.storeDir; - if (auto & optRoot = getLocalStore().rootDir.get()) + if (auto & optRoot = getLocalFSStore().rootDir.get()) params["root"] = *optRoot; params["state"] = "/no-such-path"; params["log"] = "/no-such-path"; auto store = make_ref(params, - ref(std::dynamic_pointer_cast(worker.store.shared_from_this())), + ref(std::dynamic_pointer_cast(worker.store.shared_from_this())), *this); addedPaths.clear(); @@ -2632,7 +2632,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() } } - auto & localStore = getLocalStore(); + auto & localStore = getLocalFSStore(); if (buildMode == bmCheck) { @@ -2709,7 +2709,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() paths referenced by each of them. If there are cycles in the outputs, this will fail. */ { - auto & localStore = getLocalStore(); + auto & localStore = getLocalFSStore(); ValidPathInfos infos2; for (auto & [outputName, newInfo] : infos) { @@ -2754,7 +2754,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() void LocalDerivationGoal::signRealisation(Realisation & realisation) { - getLocalStore().signRealisation(realisation); + getLocalFSStore().signRealisation(realisation); } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 88152a645cf..b4687dccbc3 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -2,14 +2,14 @@ ///@file #include "derivation-goal.hh" -#include "local-store.hh" +#include "local-fs-store.hh" #include "processes.hh" namespace nix { struct LocalDerivationGoal : public DerivationGoal { - LocalStore & getLocalStore(); + LocalFSStore & getLocalFSStore(); /** * User selected for running the builder. diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 01914e2d613..10c5c721548 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -3,6 +3,8 @@ #include "substitution-goal.hh" #include "drv-output-substitution-goal.hh" #include "local-derivation-goal.hh" +// Just for `autoGC`, which should have own interface, don't add more usage! +#include "local-store.hh" #include "hook-instance.hh" #include "signals.hh" @@ -64,7 +66,7 @@ std::shared_ptr Worker::makeDerivationGoal(const StorePath & drv const OutputsSpec & wantedOutputs, BuildMode buildMode) { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { - return !dynamic_cast(&store) + return !dynamic_cast(&store) ? std::make_shared(drvPath, wantedOutputs, *this, buildMode) : std::make_shared(drvPath, wantedOutputs, *this, buildMode); }); @@ -75,7 +77,7 @@ std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath const BasicDerivation & drv, const OutputsSpec & wantedOutputs, BuildMode buildMode) { return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { - return !dynamic_cast(&store) + return !dynamic_cast(&store) ? std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode) : std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode); }); diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh index bf855b67ece..3ec862e3892 100644 --- a/src/libstore/local-fs-store.hh +++ b/src/libstore/local-fs-store.hh @@ -71,6 +71,40 @@ public: std::optional getBuildLogExact(const StorePath & path) override; + /** + * @todo this was moved to `LocalFSStore` from `LocalStore` because + * building needed it. Instead of just blindly moving it, we should + * should consider the division of labor and trust between the + * builder and the store. + */ + virtual void registerValidPaths(const ValidPathInfos & infos) + { unsupported("registerValidPaths"); } + + /** + * Optimise a single store path. Optionally, test the encountered + * symlinks for corruption. + * + * @todo this was moved to `LocalFSStore` from `LocalStore` because + * building needed it. Instead of just blindly moving it, we should + * should consider the division of labor and trust between the + * builder and the store. + */ + virtual void optimisePath(const Path & path, RepairFlag repair) + { unsupported("optimisePath"); } + + /** + * Add signatures to a ValidPathInfo or Realisation using the secret keys + * specified by the ‘secret-key-files’ option. + * + * @todo this was moved to `LocalFSStore` from `LocalStore` because + * building needed it. Instead of just blindly moving it, we should + * should consider the division of labor and trust between the + * builder and the store. + */ + virtual void signPathInfo(ValidPathInfo & info) + { unsupported("signPathInfo"); } + virtual void signRealisation(Realisation &) + { unsupported("signRealisation"); } }; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 8f0ffd2a2be..2dd1b0f0418 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -237,11 +237,7 @@ public: void optimiseStore() override; - /** - * Optimise a single store path. Optionally, test the encountered - * symlinks for corruption. - */ - void optimisePath(const Path & path, RepairFlag repair); + void optimisePath(const Path & path, RepairFlag repair) override; bool verifyStore(bool checkContents, RepairFlag repair) override; @@ -255,7 +251,7 @@ public: */ void registerValidPath(const ValidPathInfo & info); - void registerValidPaths(const ValidPathInfos & infos); + void registerValidPaths(const ValidPathInfos & infos) override; unsigned int getProtocol() override; @@ -343,12 +339,8 @@ private: bool isValidPath_(State & state, const StorePath & path); void queryReferrers(State & state, const StorePath & path, StorePathSet & referrers); - /** - * Add signatures to a ValidPathInfo or Realisation using the secret keys - * specified by the ‘secret-key-files’ option. - */ - void signPathInfo(ValidPathInfo & info); - void signRealisation(Realisation &); + void signPathInfo(ValidPathInfo & info) override; + void signRealisation(Realisation &) override; // XXX: Make a generic `Store` method ContentAddress hashCAPath(