From 826877cabf9374e0acd5408c6975ee332b1cccc8 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 8 Mar 2021 11:56:33 +0100 Subject: [PATCH 1/4] Add some logic for signing realisations Not exposed anywhere, but built realisations are now signed (and this should be forwarded when copy-ing them around) --- src/libstore/build/local-derivation-goal.cc | 12 ++++-- src/libstore/ca-specific-schema.sql | 1 + src/libstore/local-store.cc | 29 ++++++++++--- src/libstore/local-store.hh | 4 +- src/libstore/realisation.cc | 46 ++++++++++++++++++++- src/libstore/realisation.hh | 8 ++++ src/libstore/store-api.hh | 5 +++ 7 files changed, 93 insertions(+), 12 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 9c2f1dda6..048135ccf 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2615,10 +2615,14 @@ void LocalDerivationGoal::registerOutputs() but it's fine to do in all cases. */ if (settings.isExperimentalFeatureEnabled("ca-derivations")) { - for (auto& [outputName, newInfo] : infos) - worker.store.registerDrvOutput(Realisation{ - .id = DrvOutput{initialOutputs.at(outputName).outputHash, outputName}, - .outPath = newInfo.path}); + for (auto& [outputName, newInfo] : infos) { + auto thisRealisation = Realisation{ + .id = DrvOutput{initialOutputs.at(outputName).outputHash, + outputName}, + .outPath = newInfo.path}; + getLocalStore().signRealisation(thisRealisation); + worker.store.registerDrvOutput(thisRealisation); + } } } diff --git a/src/libstore/ca-specific-schema.sql b/src/libstore/ca-specific-schema.sql index 93c442826..20ee046a1 100644 --- a/src/libstore/ca-specific-schema.sql +++ b/src/libstore/ca-specific-schema.sql @@ -6,6 +6,7 @@ create table if not exists Realisations ( drvPath text not null, outputName text not null, -- symbolic output id, usually "out" outputPath integer not null, + signatures text, -- space-separated list primary key (drvPath, outputName), foreign key (outputPath) references ValidPaths(id) on delete cascade ); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 90fb4a4bd..6bc963f27 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -310,13 +310,13 @@ LocalStore::LocalStore(const Params & params) if (settings.isExperimentalFeatureEnabled("ca-derivations")) { state->stmts->RegisterRealisedOutput.create(state->db, R"( - insert or replace into Realisations (drvPath, outputName, outputPath) - values (?, ?, (select id from ValidPaths where path = ?)) + insert or replace into Realisations (drvPath, outputName, outputPath, signatures) + values (?, ?, (select id from ValidPaths where path = ?), ?) ; )"); state->stmts->QueryRealisedOutput.create(state->db, R"( - select Output.path from Realisations + select Output.path, Realisations.signatures from Realisations inner join ValidPaths as Output on Output.id = Realisations.outputPath where drvPath = ? and outputName = ? ; @@ -662,6 +662,7 @@ void LocalStore::registerDrvOutput(const Realisation & info) (info.id.strHash()) (info.id.outputName) (printStorePath(info.outPath)) + (concatStringsSep(" ", info.signatures)) .exec(); }); } @@ -1107,6 +1108,11 @@ bool LocalStore::pathInfoIsTrusted(const ValidPathInfo & info) return requireSigs && !info.checkSignatures(*this, getPublicKeys()); } +bool LocalStore::realisationIsUntrusted(const Realisation & realisation) +{ + return requireSigs && !realisation.checkSignatures(getPublicKeys()); +} + void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) { @@ -1612,6 +1618,18 @@ void LocalStore::addSignatures(const StorePath & storePath, const StringSet & si } +void LocalStore::signRealisation(Realisation & realisation) +{ + // FIXME: keep secret keys in memory. + + auto secretKeyFiles = settings.secretKeyFiles; + + for (auto & secretKeyFile : secretKeyFiles.get()) { + SecretKey secretKey(readFile(secretKeyFile)); + realisation.sign(secretKey); + } +} + void LocalStore::signPathInfo(ValidPathInfo & info) { // FIXME: keep secret keys in memory. @@ -1649,8 +1667,9 @@ std::optional LocalStore::queryRealisation( if (!use.next()) return std::nullopt; auto outputPath = parseStorePath(use.getStr(0)); - return Ret{ - Realisation{.id = id, .outPath = outputPath}}; + auto signatures = tokenizeString(use.getStr(1)); + return Ret{Realisation{ + .id = id, .outPath = outputPath, .signatures = signatures}}; }); } } // namespace nix diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index fc67f215a..d54609f01 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -137,6 +137,7 @@ public: SubstitutablePathInfos & infos) override; bool pathInfoIsTrusted(const ValidPathInfo &) override; + bool realisationIsUntrusted(const Realisation & ) override; void addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override; @@ -272,9 +273,10 @@ private: bool isValidPath_(State & state, const StorePath & path); void queryReferrers(State & state, const StorePath & path, StorePathSet & referrers); - /* Add signatures to a ValidPathInfo using the secret keys + /* 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 &); Path getRealStoreDir() override { return realStoreDir; } diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index cd74af4ee..638065547 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -25,27 +25,69 @@ nlohmann::json Realisation::toJSON() const { return nlohmann::json{ {"id", id.to_string()}, {"outPath", outPath.to_string()}, + {"signatures", signatures}, }; } Realisation Realisation::fromJSON( const nlohmann::json& json, const std::string& whence) { - auto getField = [&](std::string fieldName) -> std::string { + auto getOptionalField = [&](std::string fieldName) -> std::optional { auto fieldIterator = json.find(fieldName); if (fieldIterator == json.end()) + return std::nullopt; + return *fieldIterator; + }; + auto getField = [&](std::string fieldName) -> std::string { + if (auto field = getOptionalField(fieldName)) + return *field; + else throw Error( "Drv output info file '%1%' is corrupt, missing field %2%", whence, fieldName); - return *fieldIterator; }; + StringSet signatures; + if (auto signaturesIterator = json.find("signatures"); signaturesIterator != json.end()) + signatures.insert(signaturesIterator->begin(), signaturesIterator->end()); + return Realisation{ .id = DrvOutput::parse(getField("id")), .outPath = StorePath(getField("outPath")), + .signatures = signatures, }; } +std::string Realisation::fingerprint() const +{ + auto serialized = toJSON(); + serialized.erase("signatures"); + return serialized.dump(); +} + +void Realisation::sign(const SecretKey & secretKey) +{ + signatures.insert(secretKey.signDetached(fingerprint())); +} + +bool Realisation::checkSignature(const PublicKeys & publicKeys, const std::string & sig) const +{ + return verifyDetached(fingerprint(), sig, publicKeys); +} + +size_t Realisation::checkSignatures(const PublicKeys & publicKeys) const +{ + // FIXME: Maybe we should return `maxSigs` if the realisation corresponds to + // an input-addressed one − because in that case the drv is enough to check + // it − but we can't know that here. + + size_t good = 0; + for (auto & sig : signatures) + if (checkSignature(publicKeys, sig)) + good++; + return good; +} + StorePath RealisedPath::path() const { return std::visit([](auto && arg) { return arg.getPath(); }, raw); } diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index fc92d3c17..f5049c9e9 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -3,6 +3,7 @@ #include "path.hh" #include #include "comparator.hh" +#include "crypto.hh" namespace nix { @@ -25,9 +26,16 @@ struct Realisation { DrvOutput id; StorePath outPath; + StringSet signatures; + nlohmann::json toJSON() const; static Realisation fromJSON(const nlohmann::json& json, const std::string& whence); + std::string fingerprint() const; + void sign(const SecretKey &); + bool checkSignature(const PublicKeys & publicKeys, const std::string & sig) const; + size_t checkSignatures(const PublicKeys & publicKeys) const; + StorePath getPath() const { return outPath; } GENERATE_CMP(Realisation, me->id, me->outPath); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 71a28eeb8..0cd56d34e 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -389,6 +389,11 @@ public: return true; } + virtual bool realisationIsUntrusted(const Realisation & ) + { + return true; + } + protected: virtual void queryPathInfoUncached(const StorePath & path, From 3e6017f911127555cfbed71fe4a4df8f70d08bbb Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 8 Mar 2021 15:07:33 +0100 Subject: [PATCH 2/4] pathInfoIsTrusted -> pathInfoIsUntrusted I guess the rationale behind the old name wath that `pathInfoIsTrusted(info)` returns `true` iff we would need to `blindly` trust the path (because it has no valid signature and `requireSigs` is set), but I find it to be a really confusing footgun because it's quite natural to give it the opposite meaning. --- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/local-store.cc | 4 ++-- src/libstore/local-store.hh | 2 +- src/libstore/store-api.hh | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 5d88b8758..7b1ac126e 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -142,7 +142,7 @@ void PathSubstitutionGoal::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 (!sub->isTrusted && worker.store.pathInfoIsTrusted(*info)) + if (!sub->isTrusted && worker.store.pathInfoIsUntrusted(*info)) { warn("substituter '%s' does not have a valid signature for path '%s'", sub->getUri(), worker.store.printStorePath(storePath)); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 6bc963f27..950a9f74e 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1103,7 +1103,7 @@ const PublicKeys & LocalStore::getPublicKeys() return *state->publicKeys; } -bool LocalStore::pathInfoIsTrusted(const ValidPathInfo & info) +bool LocalStore::pathInfoIsUntrusted(const ValidPathInfo & info) { return requireSigs && !info.checkSignatures(*this, getPublicKeys()); } @@ -1116,7 +1116,7 @@ bool LocalStore::realisationIsUntrusted(const Realisation & realisation) void LocalStore::addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) { - if (checkSigs && pathInfoIsTrusted(info)) + if (checkSigs && pathInfoIsUntrusted(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 d54609f01..c311d295a 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -136,7 +136,7 @@ public: void querySubstitutablePathInfos(const StorePathCAMap & paths, SubstitutablePathInfos & infos) override; - bool pathInfoIsTrusted(const ValidPathInfo &) override; + bool pathInfoIsUntrusted(const ValidPathInfo &) override; bool realisationIsUntrusted(const Realisation & ) override; void addToStore(const ValidPathInfo & info, Source & source, diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 0cd56d34e..b90aeaa4c 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -384,7 +384,7 @@ public: we don't really want to add the dependencies listed in a nar info we don't trust anyyways. */ - virtual bool pathInfoIsTrusted(const ValidPathInfo &) + virtual bool pathInfoIsUntrusted(const ValidPathInfo &) { return true; } From 54ced9072b94515a756e1e8e76c92a42f0ccf366 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 8 Mar 2021 16:43:11 +0100 Subject: [PATCH 3/4] Check the signatures when copying store paths around Broken atm --- src/libstore/local-store.cc | 8 ++++++++ src/libstore/local-store.hh | 1 + src/libstore/store-api.cc | 2 +- src/libstore/store-api.hh | 2 ++ tests/ca/signatures.sh | 39 +++++++++++++++++++++++++++++++++++++ tests/local.mk | 3 ++- 6 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 tests/ca/signatures.sh diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 950a9f74e..83daa7506 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -652,6 +652,14 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat } } +void LocalStore::registerDrvOutput(const Realisation & info, CheckSigsFlag checkSigs) +{ + settings.requireExperimentalFeature("ca-derivations"); + if (checkSigs == NoCheckSigs || !realisationIsUntrusted(info)) + registerDrvOutput(info); + else + throw Error("cannot register realisation '%s' because it lacks a valid signature", info.outPath.to_string()); +} void LocalStore::registerDrvOutput(const Realisation & info) { diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index c311d295a..26e034a82 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -203,6 +203,7 @@ public: /* Register the store path 'output' as the output named 'outputName' of derivation 'deriver'. */ void registerDrvOutput(const Realisation & info) override; + void registerDrvOutput(const Realisation & info, CheckSigsFlag checkSigs) override; void cacheDrvOutputMapping(State & state, const uint64_t deriver, const string & outputName, const StorePath & output); std::optional queryRealisation(const DrvOutput&) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 77c310988..5e321cedf 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -798,7 +798,7 @@ std::map copyPaths(ref srcStore, ref dstStor auto pathsMap = copyPaths(srcStore, dstStore, storePaths, repair, checkSigs, substitute); try { for (auto & realisation : realisations) { - dstStore->registerDrvOutput(realisation); + dstStore->registerDrvOutput(realisation, checkSigs); } } catch (MissingExperimentalFeature & e) { // Don't fail if the remote doesn't support CA derivations is it might diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index b90aeaa4c..5d19e8949 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -485,6 +485,8 @@ public: */ virtual void registerDrvOutput(const Realisation & output) { unsupported("registerDrvOutput"); } + virtual void registerDrvOutput(const Realisation & output, CheckSigsFlag checkSigs) + { return registerDrvOutput(output); } /* Write a NAR dump of a store path. */ virtual void narFromPath(const StorePath & path, Sink & sink) = 0; diff --git a/tests/ca/signatures.sh b/tests/ca/signatures.sh new file mode 100644 index 000000000..4b4e468f7 --- /dev/null +++ b/tests/ca/signatures.sh @@ -0,0 +1,39 @@ +source common.sh + +# Globally enable the ca derivations experimental flag +sed -i 's/experimental-features = .*/& ca-derivations ca-references/' "$NIX_CONF_DIR/nix.conf" + +clearStore +clearCache + +nix-store --generate-binary-cache-key cache1.example.org $TEST_ROOT/sk1 $TEST_ROOT/pk1 +pk1=$(cat $TEST_ROOT/pk1) + +export REMOTE_STORE_DIR="$TEST_ROOT/remote_store" +export REMOTE_STORE="file://$REMOTE_STORE_DIR" + +ensureCorrectlyCopied () { + attrPath="$1" + nix build --store "$REMOTE_STORE" --file ./content-addressed.nix "$attrPath" +} + +testOneCopy () { + clearStore + rm -rf "$REMOTE_STORE_DIR" + + attrPath="$1" + nix copy --to $REMOTE_STORE "$attrPath" --file ./content-addressed.nix \ + --secret-key-files "$TEST_ROOT/sk1" + + ensureCorrectlyCopied "$attrPath" + + # Ensure that we can copy back what we put in the store + clearStore + nix copy --from $REMOTE_STORE \ + --file ./content-addressed.nix "$attrPath" \ + --trusted-public-keys $pk1 +} + +for attrPath in rootCA dependentCA transitivelyDependentCA dependentNonCA dependentFixedOutput; do + testOneCopy "$attrPath" +done diff --git a/tests/local.mk b/tests/local.mk index e17555051..9a227bec5 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -41,8 +41,9 @@ nix_tests = \ build.sh \ compute-levels.sh \ ca/build.sh \ - ca/nix-copy.sh \ ca/substitute.sh + ca/signatures.sh \ + ca/nix-copy.sh # parallel.sh install-tests += $(foreach x, $(nix_tests), tests/$(x)) From 703c98c6cb922ff9d8cd8cb2c1104e0d3b15b803 Mon Sep 17 00:00:00 2001 From: regnat Date: Mon, 8 Mar 2021 17:32:20 +0100 Subject: [PATCH 4/4] Properly sign the unresolved drvs Don't let them inherit the signature from the parent one (because it makes no sense to do so), but re-sign them after they have been built --- src/libstore/build/derivation-goal.cc | 2 ++ src/libstore/build/derivation-goal.hh | 3 +++ src/libstore/build/local-derivation-goal.cc | 7 ++++++- src/libstore/build/local-derivation-goal.hh | 2 ++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 7dcd2a6eb..d624e58b9 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -925,6 +925,8 @@ void DerivationGoal::resolvedFinished() { if (realisation) { auto newRealisation = *realisation; newRealisation.id = DrvOutput{initialOutputs.at(wantedOutput).outputHash, wantedOutput}; + newRealisation.signatures.clear(); + signRealisation(newRealisation); worker.store.registerDrvOutput(newRealisation); } else { // If we don't have a realisation, then it must mean that something diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index c85bcd84f..704b77caf 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -180,6 +180,9 @@ struct DerivationGoal : public Goal /* Open a log file and a pipe to it. */ Path openLogFile(); + /* Sign the newly built realisation if the store allows it */ + virtual void signRealisation(Realisation&) {} + /* Close the log file. */ void closeLogFile(); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 048135ccf..2966bb565 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2620,12 +2620,17 @@ void LocalDerivationGoal::registerOutputs() .id = DrvOutput{initialOutputs.at(outputName).outputHash, outputName}, .outPath = newInfo.path}; - getLocalStore().signRealisation(thisRealisation); + signRealisation(thisRealisation); worker.store.registerDrvOutput(thisRealisation); } } } +void LocalDerivationGoal::signRealisation(Realisation & realisation) +{ + getLocalStore().signRealisation(realisation); +} + void LocalDerivationGoal::checkOutputs(const std::map & outputs) { diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 4bbf27a1b..47b818a8b 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -161,6 +161,8 @@ struct LocalDerivationGoal : public DerivationGoal as valid. */ void registerOutputs() override; + void signRealisation(Realisation &) override; + /* Check that an output meets the requirements specified by the 'outputChecks' attribute (or the legacy '{allowed,disallowed}{References,Requisites}' attributes). */