From 4a7f9b7b02a52c73fc2fa31b36dde0e8e60fb64c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Apr 2023 22:17:12 -0400 Subject: [PATCH] Split out `VisibleStore` and `ReferrersStore` from `Store` More progress on issue #5729 `queryAllValidPaths` and `queryReferrers` were two methods that failing `unsupported` default implementation. This is not type safe. Now, those methods are in their own classes, and there is no more `unsupported` default instances of them. (I thought we could get away with one class for both, but sadly that is not quite the case: `LocalBinaryCacheStore` and `S3BinaryCacheStore` implement the former but the latter.) --- src/libcmd/command.cc | 5 ++- src/libstore/build/local-derivation-goal.cc | 14 +++++-- src/libstore/daemon.cc | 11 ++++-- src/libstore/local-binary-cache-store.cc | 3 +- src/libstore/local-store.hh | 3 +- src/libstore/misc.cc | 11 ++++-- src/libstore/referrers-store.hh | 38 +++++++++++++++++++ src/libstore/remote-store.hh | 5 ++- src/libstore/s3-binary-cache-store.cc | 3 +- src/libstore/store-api.hh | 18 --------- src/libstore/visible-store.hh | 42 +++++++++++++++++++++ src/nix-store/nix-store.cc | 7 +++- 12 files changed, 123 insertions(+), 37 deletions(-) create mode 100644 src/libstore/referrers-store.hh create mode 100644 src/libstore/visible-store.hh diff --git a/src/libcmd/command.cc b/src/libcmd/command.cc index bedf11e2c280..9860a590cf05 100644 --- a/src/libcmd/command.cc +++ b/src/libcmd/command.cc @@ -1,6 +1,8 @@ #include "command.hh" #include "store-api.hh" +#include "store-cast.hh" #include "local-fs-store.hh" +#include "visible-store.hh" #include "derivations.hh" #include "nixexpr.hh" #include "profiles.hh" @@ -169,10 +171,11 @@ void BuiltPathsCommand::run(ref store, Installables && installables) { BuiltPaths paths; if (all) { + auto & visibleStore = require(*store); if (installables.size()) throw UsageError("'--all' does not expect arguments"); // XXX: Only uses opaque paths, ignores all the realisations - for (auto & p : store->queryAllValidPaths()) + for (auto & p : visibleStore.queryAllValidPaths()) paths.push_back(BuiltPath::Opaque{p}); } else { paths = Installable::toBuiltPaths(getEvalStore(), store, realiseMode, operateOn, installables); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 58d6901d3144..81f97a8d604f 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1194,10 +1194,16 @@ struct RestrictedStoreConfig : virtual LocalFSStoreConfig const std::string name() { return "Restricted Store"; } }; -/* A wrapper around LocalStore 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 LocalFSStore, public virtual GcStore +/** + * A wrapper around `LocalStore` 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 LocalFSStore + , public virtual GcStore + , public virtual ReferrersStore { ref next; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 63898f8dc81f..332863592f2d 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -6,6 +6,7 @@ #include "store-cast.hh" #include "gc-store.hh" #include "log-store.hh" +#include "referrers-store.hh" #include "path-with-outputs.hh" #include "finally.hh" #include "archive.hh" @@ -343,9 +344,10 @@ static void performOp(TunnelLogger * logger, ref store, if (op == wopQueryReferences) for (auto & i : store->queryPathInfo(path)->references) paths.insert(i); - else if (op == wopQueryReferrers) - store->queryReferrers(path, paths); - else if (op == wopQueryValidDerivers) + else if (op == wopQueryReferrers) { + auto & referrersStore = require(*store); + referrersStore.queryReferrers(path, paths); + } else if (op == wopQueryValidDerivers) paths = store->queryValidDerivers(path); else paths = store->queryDerivationOutputs(path); logger->stopWork(); @@ -803,7 +805,8 @@ static void performOp(TunnelLogger * logger, ref store, case wopQueryAllValidPaths: { logger->startWork(); - auto paths = store->queryAllValidPaths(); + auto & visibleStore = require(*store); + auto paths = visibleStore.queryAllValidPaths(); logger->stopWork(); worker_proto::write(*store, to, paths); break; diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 5481dd762e27..d6c6c4302aab 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -1,4 +1,5 @@ #include "binary-cache-store.hh" +#include "visible-store.hh" #include "globals.hh" #include "nar-info-disk-cache.hh" @@ -20,7 +21,7 @@ struct LocalBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig } }; -class LocalBinaryCacheStore : public virtual LocalBinaryCacheStoreConfig, public virtual BinaryCacheStore +class LocalBinaryCacheStore : public virtual LocalBinaryCacheStoreConfig, public virtual BinaryCacheStore, public virtual VisibleStore { private: diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 55add18dd9f8..04d1ea79ece0 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -7,6 +7,7 @@ #include "store-api.hh" #include "local-fs-store.hh" #include "gc-store.hh" +#include "referrers-store.hh" #include "sync.hh" #include "util.hh" @@ -51,7 +52,7 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig std::string doc() override; }; -class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore, public virtual GcStore +class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore, public virtual GcStore, public virtual ReferrersStore { private: diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 89148d4152c4..d0e813ea2b08 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -3,6 +3,8 @@ #include "globals.hh" #include "local-store.hh" #include "store-api.hh" +#include "referrers-store.hh" +#include "store-cast.hh" #include "thread-pool.hh" #include "topo-sort.hh" #include "callback.hh" @@ -15,18 +17,19 @@ void Store::computeFSClosure(const StorePathSet & startPaths, StorePathSet & paths_, bool flipDirection, bool includeOutputs, bool includeDerivers) { std::function(const StorePath & path, std::future> &)> queryDeps; - if (flipDirection) + if (flipDirection) { + auto & referrersStore = require(*this); queryDeps = [&](const StorePath& path, std::future> & fut) { StorePathSet res; StorePathSet referrers; - queryReferrers(path, referrers); + referrersStore.queryReferrers(path, referrers); for (auto& ref : referrers) if (ref != path) res.insert(ref); if (includeOutputs) - for (auto& i : queryValidDerivers(path)) + for (auto& i : referrersStore.queryValidDerivers(path)) res.insert(i); if (includeDerivers && path.isDerivation()) @@ -35,7 +38,7 @@ void Store::computeFSClosure(const StorePathSet & startPaths, res.insert(*maybeOutPath); return res; }; - else + } else queryDeps = [&](const StorePath& path, std::future> & fut) { StorePathSet res; diff --git a/src/libstore/referrers-store.hh b/src/libstore/referrers-store.hh new file mode 100644 index 000000000000..a62f24e3418b --- /dev/null +++ b/src/libstore/referrers-store.hh @@ -0,0 +1,38 @@ +#pragma once +///@file + +#include "visible-store.hh" + +namespace nix { + +/** + * A store that allows querying referrers. + * + * The referrers relation is the dual of the references relation, the + * latter being the "regular" one we are usually interested in. + * + * This is no inherent reason why this should be a subclass of + * `VisibleStore`; it just so happens that every extent store object we + * have to day that implements `queryReferrers()` also implements + * `queryAllValidPaths()`. If that ceases to be the case, we can revisit + * this; until this having this interface inheritance means fewer + * interface combinations to think about. + */ +struct ReferrersStore : virtual VisibleStore +{ + inline static std::string operationName = "Query referrers"; + + /** + * Queries the set of incoming FS references for a store path. + * The result is not cleared. + * + * @param path The path of the store object we care about incoming + * references to. + * + * @param [out] referrers The set in which to collect the referrers + * of `path`. + */ + virtual void queryReferrers(const StorePath & path, StorePathSet & referrers) = 0; +}; + +} diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 1c45f543e77c..5dcc23c08b86 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -7,6 +7,7 @@ #include "store-api.hh" #include "gc-store.hh" #include "log-store.hh" +#include "referrers-store.hh" namespace nix { @@ -39,7 +40,9 @@ struct RemoteStoreConfig : virtual StoreConfig class RemoteStore : public virtual RemoteStoreConfig, public virtual Store, public virtual GcStore, - public virtual LogStore + public virtual LogStore, + public virtual ReferrersStore + { public: diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index d2fc6abafe46..0696bb644aa7 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -2,6 +2,7 @@ #include "s3.hh" #include "s3-binary-cache-store.hh" +#include "visible-store.hh" #include "nar-info.hh" #include "nar-info-disk-cache.hh" #include "globals.hh" @@ -260,7 +261,7 @@ struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig } }; -struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual S3BinaryCacheStore +struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual S3BinaryCacheStore, public VisibleStore { std::string bucketName; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 3cb48bff5b48..ed30195a2c5c 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -336,17 +336,6 @@ public: virtual StorePathSet queryValidPaths(const StorePathSet & paths, SubstituteFlag maybeSubstitute = NoSubstitute); - /** - * Query the set of all valid paths. Note that for some store - * backends, the name part of store paths may be replaced by 'x' - * (i.e. you'll get /nix/store/-x rather than - * /nix/store/-). Use queryPathInfo() to obtain the - * full store path. FIXME: should return a set of - * std::variant to get rid of this hack. - */ - virtual StorePathSet queryAllValidPaths() - { unsupported("queryAllValidPaths"); } - constexpr static const char * MissingName = "x"; /** @@ -403,13 +392,6 @@ protected: public: - /** - * Queries the set of incoming FS references for a store path. - * The result is not cleared. - */ - virtual void queryReferrers(const StorePath & path, StorePathSet & referrers) - { unsupported("queryReferrers"); } - /** * @return all currently valid derivations that have `path` as an * output. diff --git a/src/libstore/visible-store.hh b/src/libstore/visible-store.hh new file mode 100644 index 000000000000..0d602a0c38af --- /dev/null +++ b/src/libstore/visible-store.hh @@ -0,0 +1,42 @@ +#pragma once +///@file + +#include "store-api.hh" + +namespace nix { + +/** + * A Store that exposes all store objects. + * + * ### Privacy and Security + * + * For the base `Store` class, we aim for `StorePath`s to act as + * capabilities: only store objects which are reachable from the store + * objects the user has (i.e. those directly-referenced objects and + * their reference closure). + * + * A `VisibleStore` breaks this by exposing these methods that allow + * discovering other store objects, outside the "reachable set" as + * defined above. This is necessary to implement certain operations, but + * care must taken exposing this functionality to the user as it makes + * e.g. secret management and other security properties trickier to get + * right. + */ +struct VisibleStore : virtual Store +{ + inline static std::string operationName = "Query all valid paths"; + + /** + * Query the set of all valid paths. Note that for some store + * backends, the name part of store paths may be replaced by 'x' + * (i.e. you'll get `/nix/store/-x` rather than + * `/nix/store/-`). Use `queryPathInfo()` to obtain the + * full store path. + * + * \todo should return a set of `std::variant` + * to get rid of this hack. + */ + virtual StorePathSet queryAllValidPaths() = 0; +}; + +} diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 7035e6a7b1ff..a297c4eadac4 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -6,6 +6,7 @@ #include "store-cast.hh" #include "gc-store.hh" #include "log-store.hh" +#include "referrers-store.hh" #include "local-store.hh" #include "monitor-fd.hh" #include "serve-protocol.hh" @@ -335,6 +336,7 @@ static void opQuery(Strings opFlags, Strings opArgs) case qReferences: case qReferrers: case qReferrersClosure: { + auto & referrersStore = require(*store); StorePathSet paths; for (auto & i : opArgs) { auto ps = maybeUseOutputs(store->followLinksToStorePath(i), useOutput, forceRealise); @@ -346,7 +348,7 @@ static void opQuery(Strings opFlags, Strings opArgs) } else if (query == qReferrers) { StorePathSet tmp; - store->queryReferrers(j, tmp); + referrersStore.queryReferrers(j, tmp); for (auto & i : tmp) paths.insert(i); } @@ -495,12 +497,13 @@ static void opReadLog(Strings opFlags, Strings opArgs) static void opDumpDB(Strings opFlags, Strings opArgs) { + auto & visibleStore = require(*store); if (!opFlags.empty()) throw UsageError("unknown flag"); if (!opArgs.empty()) { for (auto & i : opArgs) cout << store->makeValidityRegistration({store->followLinksToStorePath(i)}, true, true); } else { - for (auto & i : store->queryAllValidPaths()) + for (auto & i : visibleStore.queryAllValidPaths()) cout << store->makeValidityRegistration({i}, true, true); } }