From d92d4f85a5c8a2a2385c084500a8b6bd54b54e6c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 24 Jun 2020 21:22:13 +0000 Subject: [PATCH 1/8] Move ValidPathInfo to its own header --- src/libstore/nar-info.cc | 1 + src/libstore/nar-info.hh | 2 +- src/libstore/path-info.hh | 99 +++++++++++++++++++++++++++++++++++++++ src/libstore/store-api.hh | 78 +----------------------------- 4 files changed, 102 insertions(+), 78 deletions(-) create mode 100644 src/libstore/path-info.hh diff --git a/src/libstore/nar-info.cc b/src/libstore/nar-info.cc index 04550ed97..ef04bc859 100644 --- a/src/libstore/nar-info.cc +++ b/src/libstore/nar-info.cc @@ -1,5 +1,6 @@ #include "globals.hh" #include "nar-info.hh" +#include "store-api.hh" namespace nix { diff --git a/src/libstore/nar-info.hh b/src/libstore/nar-info.hh index 373c33427..fa38ccd78 100644 --- a/src/libstore/nar-info.hh +++ b/src/libstore/nar-info.hh @@ -2,7 +2,7 @@ #include "types.hh" #include "hash.hh" -#include "store-api.hh" +#include "path-info.hh" namespace nix { diff --git a/src/libstore/path-info.hh b/src/libstore/path-info.hh new file mode 100644 index 000000000..f5dee00a6 --- /dev/null +++ b/src/libstore/path-info.hh @@ -0,0 +1,99 @@ +#pragma once + +// TODO many of thes eare not needed. +#include "path.hh" +#include "hash.hh" +#include "content-address.hh" +#include "serialise.hh" +#include "crypto.hh" +#include "lru-cache.hh" +#include "sync.hh" +#include "globals.hh" +#include "config.hh" + +#include +#include +#include + +namespace nix { + + +class Store; + +struct ValidPathInfo +{ + StorePath path; + std::optional deriver; + Hash narHash; + StorePathSet references; + time_t registrationTime = 0; + uint64_t narSize = 0; // 0 = unknown + uint64_t id; // internal use only + + /* Whether the path is ultimately trusted, that is, it's a + derivation output that was built locally. */ + bool ultimate = false; + + StringSet sigs; // note: not necessarily verified + + /* If non-empty, an assertion that the path is content-addressed, + i.e., that the store path is computed from a cryptographic hash + of the contents of the path, plus some other bits of data like + the "name" part of the path. Such a path doesn't need + signatures, since we don't have to trust anybody's claim that + the path is the output of a particular derivation. (In the + extensional store model, we have to trust that the *contents* + of an output path of a derivation were actually produced by + that derivation. In the intensional model, we have to trust + that a particular output path was produced by a derivation; the + path then implies the contents.) + + Ideally, the content-addressability assertion would just be a Boolean, + and the store path would be computed from the name component, ‘narHash’ + and ‘references’. However, we support many types of content addresses. + */ + std::optional ca; + + bool operator == (const ValidPathInfo & i) const + { + return + path == i.path + && narHash == i.narHash + && references == i.references; + } + + /* Return a fingerprint of the store path to be used in binary + cache signatures. It contains the store path, the base-32 + SHA-256 hash of the NAR serialisation of the path, the size of + the NAR, and the sorted references. The size field is strictly + speaking superfluous, but might prevent endless/excessive data + attacks. */ + std::string fingerprint(const Store & store) const; + + void sign(const Store & store, const SecretKey & secretKey); + + /* Return true iff the path is verifiably content-addressed. */ + bool isContentAddressed(const Store & store) const; + + static const size_t maxSigs = std::numeric_limits::max(); + + /* Return the number of signatures on this .narinfo that were + produced by one of the specified keys, or maxSigs if the path + is content-addressed. */ + size_t checkSignatures(const Store & store, const PublicKeys & publicKeys) const; + + /* Verify a single signature. */ + bool checkSignature(const Store & store, const PublicKeys & publicKeys, const std::string & sig) const; + + Strings shortRefs() const; + + ValidPathInfo(const ValidPathInfo & other) = default; + + ValidPathInfo(StorePath && path) : path(std::move(path)) { }; + ValidPathInfo(const StorePath & path) : path(path) { }; + + virtual ~ValidPathInfo() { } +}; + +typedef list ValidPathInfos; +} diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 25d78c297..00b9c385c 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -10,6 +10,7 @@ #include "globals.hh" #include "config.hh" #include "derivations.hh" +#include "path-info.hh" #include #include @@ -111,83 +112,6 @@ struct SubstitutablePathInfo typedef std::map SubstitutablePathInfos; -struct ValidPathInfo -{ - StorePath path; - std::optional deriver; - Hash narHash; - StorePathSet references; - time_t registrationTime = 0; - uint64_t narSize = 0; // 0 = unknown - uint64_t id; // internal use only - - /* Whether the path is ultimately trusted, that is, it's a - derivation output that was built locally. */ - bool ultimate = false; - - StringSet sigs; // note: not necessarily verified - - /* If non-empty, an assertion that the path is content-addressed, - i.e., that the store path is computed from a cryptographic hash - of the contents of the path, plus some other bits of data like - the "name" part of the path. Such a path doesn't need - signatures, since we don't have to trust anybody's claim that - the path is the output of a particular derivation. (In the - extensional store model, we have to trust that the *contents* - of an output path of a derivation were actually produced by - that derivation. In the intensional model, we have to trust - that a particular output path was produced by a derivation; the - path then implies the contents.) - - Ideally, the content-addressability assertion would just be a Boolean, - and the store path would be computed from the name component, ‘narHash’ - and ‘references’. However, we support many types of content addresses. - */ - std::optional ca; - - bool operator == (const ValidPathInfo & i) const - { - return - path == i.path - && narHash == i.narHash - && references == i.references; - } - - /* Return a fingerprint of the store path to be used in binary - cache signatures. It contains the store path, the base-32 - SHA-256 hash of the NAR serialisation of the path, the size of - the NAR, and the sorted references. The size field is strictly - speaking superfluous, but might prevent endless/excessive data - attacks. */ - std::string fingerprint(const Store & store) const; - - void sign(const Store & store, const SecretKey & secretKey); - - /* Return true iff the path is verifiably content-addressed. */ - bool isContentAddressed(const Store & store) const; - - static const size_t maxSigs = std::numeric_limits::max(); - - /* Return the number of signatures on this .narinfo that were - produced by one of the specified keys, or maxSigs if the path - is content-addressed. */ - size_t checkSignatures(const Store & store, const PublicKeys & publicKeys) const; - - /* Verify a single signature. */ - bool checkSignature(const Store & store, const PublicKeys & publicKeys, const std::string & sig) const; - - Strings shortRefs() const; - - ValidPathInfo(const ValidPathInfo & other) = default; - - ValidPathInfo(StorePath && path) : path(std::move(path)) { }; - ValidPathInfo(const StorePath & path) : path(path) { }; - - virtual ~ValidPathInfo() { } -}; - -typedef list ValidPathInfos; - enum BuildMode { bmNormal, bmRepair, bmCheck }; From 54281f3ac130c21407e5ed4326d1d57626d6c19b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 3 Aug 2020 04:13:45 +0000 Subject: [PATCH 2/8] `addToStore` in terms of `addToStoreFromDump` is not local-store-specific --- src/libstore/local-store.cc | 14 -------------- src/libstore/local-store.hh | 10 +--------- src/libstore/store-api.cc | 14 ++++++++++++++ src/libstore/store-api.hh | 6 +++++- 4 files changed, 20 insertions(+), 24 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f6ce8877e..f908f7d67 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1038,20 +1038,6 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, } -StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, - FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) -{ - Path srcPath(absPath(_srcPath)); - auto source = sinkToSource([&](Sink & sink) { - if (method == FileIngestionMethod::Recursive) - dumpPath(srcPath, sink, filter); - else - readFile(srcPath, sink); - }); - return addToStoreFromDump(*source, name, method, hashAlgo, repair); -} - - StorePath LocalStore::addToStoreFromDump(Source & source0, const string & name, FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 5a87ec2f8..31e6587ac 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -145,16 +145,8 @@ public: void addToStore(const ValidPathInfo & info, Source & source, RepairFlag repair, CheckSigsFlag checkSigs) override; - StorePath addToStore(const string & name, const Path & srcPath, - FileIngestionMethod method, HashType hashAlgo, - PathFilter & filter, RepairFlag repair) override; - - /* Like addToStore(), but the contents of the path are contained - in `dump', which is either a NAR serialisation (if recursive == - true) or simply the contents of a regular file (if recursive == - false). */ StorePath addToStoreFromDump(Source & dump, const string & name, - FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override; + FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) override; StorePath addTextToStore(const string & name, const string & s, const StorePathSet & references, RepairFlag repair) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 33f931442..fb9e30597 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -239,6 +239,20 @@ StorePath Store::computeStorePathForText(const string & name, const string & s, } +StorePath Store::addToStore(const string & name, const Path & _srcPath, + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) +{ + Path srcPath(absPath(_srcPath)); + auto source = sinkToSource([&](Sink & sink) { + if (method == FileIngestionMethod::Recursive) + dumpPath(srcPath, sink, filter); + else + readFile(srcPath, sink); + }); + return addToStoreFromDump(*source, name, method, hashAlgo, repair); +} + + /* The aim of this function is to compute in one pass the correct ValidPathInfo for the files that we are trying to add to the store. To accomplish that in one diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 49da19496..e05a19975 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -456,7 +456,7 @@ public: libutil/archive.hh). */ virtual StorePath addToStore(const string & name, const Path & srcPath, FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, - PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair) = 0; + PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair); /* Copy the contents of a path to the store and register the validity the resulting path, using a constant amount of @@ -465,6 +465,10 @@ public: FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, std::optional expectedCAHash = {}); + /* Like addToStore(), but the contents of the path are contained + in `dump', which is either a NAR serialisation (if recursive == + true) or simply the contents of a regular file (if recursive == + false). */ // FIXME: remove? virtual StorePath addToStoreFromDump(Source & dump, const string & name, FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) From 24e07c428f21f28df2a41a7a9851d5867f34753a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 3 Aug 2020 18:33:39 +0200 Subject: [PATCH 3/8] Delete compressed NARs Fixes #3891. --- src/libstore/binary-cache-store.cc | 2 ++ src/libutil/util.cc | 1 + 2 files changed, 3 insertions(+) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 30150eeba..3d3195e03 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -153,6 +153,8 @@ void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource auto [fdTemp, fnTemp] = createTempFile(); + AutoDelete autoDelete(fnTemp); + auto now1 = std::chrono::steady_clock::now(); /* Read the NAR simultaneously into a CompressionSink+FileSink (to diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 97d278581..8bc60ec2d 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -494,6 +494,7 @@ std::pair createTempFile(const Path & prefix) { Path tmpl(getEnv("TMPDIR").value_or("/tmp") + "/" + prefix + ".XXXXXX"); // Strictly speaking, this is UB, but who cares... + // FIXME: use O_TMPFILE. AutoCloseFD fd(mkstemp((char *) tmpl.c_str())); if (!fd) throw SysError("creating temporary file '%s'", tmpl); From fe7e57a80d1f698565aaa13596de3fa13dbafe1f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 4 Aug 2020 03:46:28 +0000 Subject: [PATCH 4/8] tab -> space --- src/nix/installables.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix/installables.cc b/src/nix/installables.cc index babb0e5fe..59b52ce95 100644 --- a/src/nix/installables.cc +++ b/src/nix/installables.cc @@ -437,7 +437,7 @@ ref openEvalCache( std::shared_ptr lockedFlake, bool useEvalCache) { - auto fingerprint = lockedFlake->getFingerprint(); + auto fingerprint = lockedFlake->getFingerprint(); return make_ref( useEvalCache && evalSettings.pureEval ? std::optional { std::cref(fingerprint) } From 327b1bf378ebdfe7acc6357b651470b45cb44472 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 4 Aug 2020 14:50:43 +0200 Subject: [PATCH 5/8] BinaryCacheStore: Explicitly flush file sink The file sink is also flushed in its destructor, but we ignore any exceptions in the destructor. Issue #3886. --- src/libstore/binary-cache-store.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 3d3195e03..7d103e0cc 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -169,6 +169,7 @@ void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource TeeSource teeSource(narSource, *compressionSink); narAccessor = makeNarAccessor(teeSource); compressionSink->finish(); + fileSink.flush(); } auto now2 = std::chrono::steady_clock::now(); From dfe66420e7e5395acfa8068fafcadc3a38a56184 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 4 Aug 2020 15:56:10 +0200 Subject: [PATCH 6/8] Revert "Remove putBytes" This reverts commit b8eea7e81af53905be7845dffc6d0a83ea8edc97. --- src/libstore/s3-binary-cache-store.cc | 7 +++++-- src/libstore/s3-binary-cache-store.hh | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 67935f3ba..1b7dff085 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -343,10 +343,13 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore std::chrono::duration_cast(now2 - now1) .count(); - printInfo("uploaded 's3://%s/%s' in %d ms", - bucketName, path, duration); + auto size = istream->tellg(); + + printInfo("uploaded 's3://%s/%s' (%d bytes) in %d ms", + bucketName, path, size, duration); stats.putTimeMs += duration; + stats.putBytes += size; stats.put++; } diff --git a/src/libstore/s3-binary-cache-store.hh b/src/libstore/s3-binary-cache-store.hh index b2b75d498..4d43fe4d2 100644 --- a/src/libstore/s3-binary-cache-store.hh +++ b/src/libstore/s3-binary-cache-store.hh @@ -19,6 +19,7 @@ public: struct Stats { std::atomic put{0}; + std::atomic putBytes{0}; std::atomic putTimeMs{0}; std::atomic get{0}; std::atomic getBytes{0}; From 4e7f1c7f11fb22d7954b2dacbe87c61548fab82d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 4 Aug 2020 16:00:59 +0200 Subject: [PATCH 7/8] S3BinaryCacheStore: Fix size determination --- src/libstore/binary-cache-store.cc | 2 +- src/libstore/s3-binary-cache-store.cc | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 7d103e0cc..9682db730 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -283,7 +283,7 @@ void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource if (repair || !fileExists(narInfo->url)) { stats.narWrite++; upsertFile(narInfo->url, - std::make_shared(fnTemp, std::ios_base::in), + std::make_shared(fnTemp, std::ios_base::in | std::ios_base::binary), "application/x-nix-nar"); } else stats.narWriteAverted++; diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 1b7dff085..a0a446bd3 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -266,6 +266,10 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore const std::string & mimeType, const std::string & contentEncoding) { + istream->seekg(0, istream->end); + auto size = istream->tellg(); + istream->seekg(0, istream->beg); + auto maxThreads = std::thread::hardware_concurrency(); static std::shared_ptr @@ -343,13 +347,11 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore std::chrono::duration_cast(now2 - now1) .count(); - auto size = istream->tellg(); - printInfo("uploaded 's3://%s/%s' (%d bytes) in %d ms", bucketName, path, size, duration); stats.putTimeMs += duration; - stats.putBytes += size; + stats.putBytes += std::max(size, (decltype(size)) 0); stats.put++; } From 6d9ccde20d783a660aca3cab0e6e5c20e8444641 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 4 Aug 2020 01:09:52 +0000 Subject: [PATCH 8/8] Make JSON equality tests agnostic to ordering It is in fact more sorted than before, but I don't think we want to guarantee anything about the ordering. --- tests/binary-cache.sh | 8 ++++++-- tests/nar-access.sh | 24 ++++++++++++++++++------ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/tests/binary-cache.sh b/tests/binary-cache.sh index 40f1a4f76..b05a7d167 100644 --- a/tests/binary-cache.sh +++ b/tests/binary-cache.sh @@ -218,7 +218,9 @@ outPath=$(nix-build --no-out-link -E ' nix copy --to file://$cacheDir?write-nar-listing=1 $outPath -[[ $(cat $cacheDir/$(basename $outPath).ls) = '{"version":1,"root":{"type":"directory","entries":{"bar":{"type":"regular","size":4,"narOffset":232},"link":{"type":"symlink","target":"xyzzy"}}}}' ]] +diff -u \ + <(jq -S < $cacheDir/$(basename $outPath).ls) \ + <(echo '{"version":1,"root":{"type":"directory","entries":{"bar":{"type":"regular","size":4,"narOffset":232},"link":{"type":"symlink","target":"xyzzy"}}}}' | jq -S) # Test debug info index generation. @@ -234,4 +236,6 @@ outPath=$(nix-build --no-out-link -E ' nix copy --to "file://$cacheDir?index-debug-info=1&compression=none" $outPath -[[ $(cat $cacheDir/debuginfo/02623eda209c26a59b1a8638ff7752f6b945c26b.debug) = '{"archive":"../nar/100vxs724qr46phz8m24iswmg9p3785hsyagz0kchf6q6gf06sw6.nar","member":"lib/debug/.build-id/02/623eda209c26a59b1a8638ff7752f6b945c26b.debug"}' ]] +diff -u \ + <(cat $cacheDir/debuginfo/02623eda209c26a59b1a8638ff7752f6b945c26b.debug | jq -S) \ + <(echo '{"archive":"../nar/100vxs724qr46phz8m24iswmg9p3785hsyagz0kchf6q6gf06sw6.nar","member":"lib/debug/.build-id/02/623eda209c26a59b1a8638ff7752f6b945c26b.debug"}' | jq -S) diff --git a/tests/nar-access.sh b/tests/nar-access.sh index 553d6ca89..88b997ca6 100644 --- a/tests/nar-access.sh +++ b/tests/nar-access.sh @@ -26,12 +26,24 @@ nix cat-store $storePath/foo/baz > baz.cat-nar diff -u baz.cat-nar $storePath/foo/baz # Test --json. -[[ $(nix ls-nar --json $narFile /) = '{"type":"directory","entries":{"foo":{},"foo-x":{},"qux":{},"zyx":{}}}' ]] -[[ $(nix ls-nar --json -R $narFile /foo) = '{"type":"directory","entries":{"bar":{"type":"regular","size":0,"narOffset":368},"baz":{"type":"regular","size":0,"narOffset":552},"data":{"type":"regular","size":58,"narOffset":736}}}' ]] -[[ $(nix ls-nar --json -R $narFile /foo/bar) = '{"type":"regular","size":0,"narOffset":368}' ]] -[[ $(nix ls-store --json $storePath) = '{"type":"directory","entries":{"foo":{},"foo-x":{},"qux":{},"zyx":{}}}' ]] -[[ $(nix ls-store --json -R $storePath/foo) = '{"type":"directory","entries":{"bar":{"type":"regular","size":0},"baz":{"type":"regular","size":0},"data":{"type":"regular","size":58}}}' ]] -[[ $(nix ls-store --json -R $storePath/foo/bar) = '{"type":"regular","size":0}' ]] +diff -u \ + <(nix ls-nar --json $narFile / | jq -S) \ + <(echo '{"type":"directory","entries":{"foo":{},"foo-x":{},"qux":{},"zyx":{}}}' | jq -S) +diff -u \ + <(nix ls-nar --json -R $narFile /foo | jq -S) \ + <(echo '{"type":"directory","entries":{"bar":{"type":"regular","size":0,"narOffset":368},"baz":{"type":"regular","size":0,"narOffset":552},"data":{"type":"regular","size":58,"narOffset":736}}}' | jq -S) +diff -u \ + <(nix ls-nar --json -R $narFile /foo/bar | jq -S) \ + <(echo '{"type":"regular","size":0,"narOffset":368}' | jq -S) +diff -u \ + <(nix ls-store --json $storePath | jq -S) \ + <(echo '{"type":"directory","entries":{"foo":{},"foo-x":{},"qux":{},"zyx":{}}}' | jq -S) +diff -u \ + <(nix ls-store --json -R $storePath/foo | jq -S) \ + <(echo '{"type":"directory","entries":{"bar":{"type":"regular","size":0},"baz":{"type":"regular","size":0},"data":{"type":"regular","size":58}}}' | jq -S) +diff -u \ + <(nix ls-store --json -R $storePath/foo/bar| jq -S) \ + <(echo '{"type":"regular","size":0}' | jq -S) # Test missing files. nix ls-store --json -R $storePath/xyzzy 2>&1 | grep 'does not exist in NAR'