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.
This commit is contained in:
John Ericson 2021-01-15 16:37:41 +00:00
parent 0027b05a15
commit 7af743470c
5 changed files with 43 additions and 26 deletions

View file

@ -142,9 +142,7 @@ void SubstitutionGoal::tryNext()
/* Bail out early if this substituter lacks a valid /* Bail out early if this substituter lacks a valid
signature. LocalStore::addToStore() also checks for this, but signature. LocalStore::addToStore() also checks for this, but
only after we've downloaded the path. */ only after we've downloaded the path. */
if (worker.store.requireSigs if (!sub->isTrusted && worker.store.pathInfoIsTrusted(*info))
&& !sub->isTrusted
&& !info->checkSignatures(worker.store, worker.store.getPublicKeys()))
{ {
logWarning({ logWarning({
.name = "Invalid path signature", .name = "Invalid path signature",

View file

@ -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, void LocalStore::addToStore(const ValidPathInfo & info, Source & source,
RepairFlag repair, CheckSigsFlag checkSigs) 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)); throw Error("cannot add path '%s' because it lacks a valid signature", printStorePath(info.path));
addTempRoot(info.path); addTempRoot(info.path);

View file

@ -35,6 +35,10 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig
{ {
using LocalFSStoreConfig::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"; } const std::string name() override { return "Local Store"; }
}; };
@ -71,6 +75,8 @@ private:
minFree but not much below availAfterGC, then there is no minFree but not much below availAfterGC, then there is no
point in starting a new GC. */ point in starting a new GC. */
uint64_t availAfterGC = std::numeric_limits<uint64_t>::max(); uint64_t availAfterGC = std::numeric_limits<uint64_t>::max();
std::unique_ptr<PublicKeys> publicKeys;
}; };
Sync<State> _state; Sync<State> _state;
@ -88,6 +94,12 @@ public:
const Path tempRootsDir; const Path tempRootsDir;
const Path fnTempRoots; const Path fnTempRoots;
private:
const PublicKeys & getPublicKeys();
public:
// Hack for build-remote.cc. // Hack for build-remote.cc.
PathSet locksHeld; PathSet locksHeld;
@ -124,6 +136,8 @@ public:
void querySubstitutablePathInfos(const StorePathCAMap & paths, void querySubstitutablePathInfos(const StorePathCAMap & paths,
SubstitutablePathInfos & infos) override; SubstitutablePathInfos & infos) override;
bool pathInfoIsTrusted(const ValidPathInfo &) override;
void addToStore(const ValidPathInfo & info, Source & source, void addToStore(const ValidPathInfo & info, Source & source,
RepairFlag repair, CheckSigsFlag checkSigs) override; RepairFlag repair, CheckSigsFlag checkSigs) override;

View file

@ -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;
}
} }

View file

@ -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"}; 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<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"}; 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, void queryPathInfo(const StorePath & path,
Callback<ref<const ValidPathInfo>> callback) noexcept; 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: protected:
virtual void queryPathInfoUncached(const StorePath & path, virtual void queryPathInfoUncached(const StorePath & path,
@ -719,20 +730,11 @@ public:
return toRealPath(printStorePath(storePath)); return toRealPath(printStorePath(storePath));
} }
const PublicKeys & getPublicKeys();
virtual void createUser(const std::string & userName, uid_t userId) virtual void createUser(const std::string & userName, uid_t userId)
{ } { }
protected: protected:
struct CryptoState
{
std::unique_ptr<PublicKeys> publicKeys;
};
Sync<CryptoState> _cryptoState;
Stats stats; Stats stats;
/* Unsupported methods. */ /* Unsupported methods. */