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.

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<PublicKeys>(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<bool> 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<uint64_t>::max();
+
+        std::unique_ptr<PublicKeys> publicKeys;
     };
 
     Sync<State> _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<PublicKeys>(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<bool> isTrusted{this, false, "trusted", "whether paths from this store can be used as substitutes even when they lack trusted signatures"};
 
-    Setting<bool> requireSigs{this,
-        settings.requireSigs,
-        "require-sigs", "whether store paths should have a trusted signature on import"};
-
     Setting<int> priority{this, 0, "priority", "priority of this substituter (lower value means higher priority)"};
 
     Setting<bool> 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<ref<const ValidPathInfo>> 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> publicKeys;
-    };
-
-    Sync<CryptoState> _cryptoState;
-
     Stats stats;
 
     /* Unsupported methods. */