From 2fcf2d0d20f7e67a80c2cb5b2d0a437faed6efb7 Mon Sep 17 00:00:00 2001 From: Griffin Smith Date: Thu, 23 Jul 2020 17:18:00 -0400 Subject: refactor(3p/nix): Remove custom base64 implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the custom, rather questionable base64 implementation with absl::Base64{Une,E}scape. To make sure that the custom implementation was doing the same thing I've also added a test covering nix::Hash::to_string, which was one function that used it - the test passed prior to the replacement, and continued to pass afterwards. The previous base64Decode function threw an exception on failure - to avoid going too far down the rabbit hole I've replicated that functionality at all call sites, but this should be replaced with more sensible error handling such as StatusOr eventually. Also, before this change: ❯ nix eval -f . users.tazjin.emacs.outPath "/nix/store/g6ri2q8nra96ix20bcsc734r1yyaylb1-tazjins-emacs" And after: ❯ ./result/bin/nix eval -f . users.tazjin.emacs.outPath "/nix/store/g6ri2q8nra96ix20bcsc734r1yyaylb1-tazjins-emacs" Change-Id: Id292ffbb82fe808f3f1b34670afbe7b8c13ad615 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1385 Reviewed-by: tazjin Tested-by: BuildkiteCI --- third_party/nix/src/libstore/crypto.cc | 15 +++++-- third_party/nix/src/libutil/hash.cc | 11 ++++- third_party/nix/src/libutil/util.cc | 67 ------------------------------ third_party/nix/src/libutil/util.hh | 4 -- third_party/nix/src/nix-store/nix-store.cc | 5 ++- third_party/nix/src/tests/CMakeLists.txt | 8 ++++ third_party/nix/src/tests/hash_test.cc | 24 +++++++++++ 7 files changed, 56 insertions(+), 78 deletions(-) create mode 100644 third_party/nix/src/tests/hash_test.cc (limited to 'third_party/nix/src') diff --git a/third_party/nix/src/libstore/crypto.cc b/third_party/nix/src/libstore/crypto.cc index 62b3c05ff95f..bec0b08c67c1 100644 --- a/third_party/nix/src/libstore/crypto.cc +++ b/third_party/nix/src/libstore/crypto.cc @@ -1,5 +1,7 @@ #include "libstore/crypto.hh" +#include + #include "libstore/globals.hh" #include "libutil/util.hh" @@ -27,7 +29,10 @@ Key::Key(const std::string& s) { throw Error("secret key is corrupt"); } - key = base64Decode(key); + if (!absl::Base64Unescape(key, &key)) { + // TODO(grfn): replace this with StatusOr + throw Error("Invalid Base64"); + } } SecretKey::SecretKey(const std::string& s) : Key(s) { @@ -52,7 +57,7 @@ std::string SecretKey::signDetached(const std::string& data) const { unsigned long long sigLen; crypto_sign_detached(sig, &sigLen, (unsigned char*)data.data(), data.size(), (unsigned char*)key.data()); - return name + ":" + base64Encode(std::string((char*)sig, sigLen)); + return name + ":" + absl::Base64Escape(std::string((char*)sig, sigLen)); #else noSodium(); #endif @@ -86,7 +91,11 @@ bool verifyDetached(const std::string& data, const std::string& sig, return false; } - auto sig2 = base64Decode(ss.second); + std::string sig2; + if (!absl::Base64Unescape(ss.second, &sig2)) { + // TODO(grfn): replace this with StatusOr + throw Error("Invalid Base64"); + } if (sig2.size() != crypto_sign_BYTES) { throw Error("signature is not valid"); } diff --git a/third_party/nix/src/libutil/hash.cc b/third_party/nix/src/libutil/hash.cc index 5c2ddc4bda8e..0d80972cceda 100644 --- a/third_party/nix/src/libutil/hash.cc +++ b/third_party/nix/src/libutil/hash.cc @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -117,7 +118,9 @@ std::string Hash::to_string(Base base, bool includeType) const { break; case Base64: case SRI: - s += base64Encode(std::string((const char*)hash, hashSize)); + std::string b64; + absl::Base64Escape(std::string((const char*)hash, hashSize), &b64); + s += b64; break; } return s; @@ -201,7 +204,11 @@ Hash::Hash(const std::string& s, HashType type) : type(type) { } else if (isSRI || size == base64Len()) { - auto d = base64Decode(std::string(s, pos)); + std::string d; + if (!absl::Base64Unescape(std::string(s, pos), &d)) { + // TODO(grfn): replace this with StatusOr + throw Error("Invalid Base64"); + } if (d.size() != hashSize) { throw BadHash("invalid %s hash '%s'", isSRI ? "SRI" : "base-64", s); } diff --git a/third_party/nix/src/libutil/util.cc b/third_party/nix/src/libutil/util.cc index b97624e58dbb..f69c341c2c59 100644 --- a/third_party/nix/src/libutil/util.cc +++ b/third_party/nix/src/libutil/util.cc @@ -1321,73 +1321,6 @@ std::string filterANSIEscapes(const std::string& s, bool filterAll, return t; } -static char base64Chars[] = - "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; - -std::string base64Encode(const std::string& s) { - std::string res; - int data = 0; - int nbits = 0; - - for (char c : s) { - data = data << 8 | (unsigned char)c; - nbits += 8; - while (nbits >= 6) { - nbits -= 6; - res.push_back(base64Chars[data >> nbits & 0x3f]); - } - } - - if (nbits != 0) { - res.push_back(base64Chars[data << (6 - nbits) & 0x3f]); - } - while ((res.size() % 4) != 0u) { - res.push_back('='); - } - - return res; -} - -std::string base64Decode(const std::string& s) { - bool init = false; - char decode[256]; - if (!init) { - // FIXME: not thread-safe. - memset(decode, -1, sizeof(decode)); - for (int i = 0; i < 64; i++) { - decode[(int)base64Chars[i]] = i; - } - init = true; - } - - std::string res; - unsigned int d = 0; - unsigned int bits = 0; - - for (char c : s) { - if (c == '=') { - break; - } - if (c == '\n') { - continue; - } - - char digit = decode[(unsigned char)c]; - if (digit == -1) { - throw Error("invalid character in Base64 string"); - } - - bits += 6; - d = d << 6 | digit; - if (bits >= 8) { - res.push_back(d >> (bits - 8) & 0xff); - bits -= 8; - } - } - - return res; -} - void callFailure(const std::function& failure, const std::exception_ptr& exc) { try { diff --git a/third_party/nix/src/libutil/util.hh b/third_party/nix/src/libutil/util.hh index 0fa15f745c86..d9fc1a27b134 100644 --- a/third_party/nix/src/libutil/util.hh +++ b/third_party/nix/src/libutil/util.hh @@ -358,10 +358,6 @@ std::string filterANSIEscapes( const std::string& s, bool filterAll = false, unsigned int width = std::numeric_limits::max()); -/* Base64 encoding/decoding. */ -std::string base64Encode(const std::string& s); -std::string base64Decode(const std::string& s); - /* Get a value for the specified key from an associate container, or a default value if the key doesn't exist. */ template diff --git a/third_party/nix/src/nix-store/nix-store.cc b/third_party/nix/src/nix-store/nix-store.cc index 7729945b1307..1f1e221fc39c 100644 --- a/third_party/nix/src/nix-store/nix-store.cc +++ b/third_party/nix/src/nix-store/nix-store.cc @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -1170,11 +1171,11 @@ static void opGenerateBinaryCacheKey(Strings opFlags, Strings opArgs) { } writeFile(publicKeyFile, keyName + ":" + - base64Encode(std::string( + absl::Base64Escape(std::string( (char*)pk, crypto_sign_PUBLICKEYBYTES))); umask(0077); writeFile(secretKeyFile, keyName + ":" + - base64Encode(std::string( + absl::Base64Escape(std::string( (char*)sk, crypto_sign_SECRETKEYBYTES))); #else throw Error( diff --git a/third_party/nix/src/tests/CMakeLists.txt b/third_party/nix/src/tests/CMakeLists.txt index b21b194630c1..724a7a95b519 100644 --- a/third_party/nix/src/tests/CMakeLists.txt +++ b/third_party/nix/src/tests/CMakeLists.txt @@ -11,6 +11,14 @@ target_link_libraries(attr-set gtest_discover_tests(attr-set) +add_executable(hash_test hash_test.cc) +target_link_libraries(hash_test + nixutil + GTest::gtest_main +) + +gtest_discover_tests(hash_test) + add_executable(value-to-json value-to-json.cc) target_link_libraries(value-to-json nixexpr diff --git a/third_party/nix/src/tests/hash_test.cc b/third_party/nix/src/tests/hash_test.cc new file mode 100644 index 000000000000..c56fa937574b --- /dev/null +++ b/third_party/nix/src/tests/hash_test.cc @@ -0,0 +1,24 @@ +#include "libutil/hash.hh" + +#include + +class HashTest : public ::testing::Test {}; + +namespace nix { + +TEST(HASH_TEST, SHA256) { + auto hash = hashString(HashType::htSHA256, "foo"); + ASSERT_EQ(hash.base64Len(), 44); + ASSERT_EQ(hash.base32Len(), 52); + ASSERT_EQ(hash.base16Len(), 64); + + ASSERT_EQ(hash.to_string(Base16), + "sha256:" + "2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae"); + ASSERT_EQ(hash.to_string(Base32), + "sha256:1bp7cri8hplaz6hbz0v4f0nl44rl84q1sg25kgwqzipzd1mv89ic"); + ASSERT_EQ(hash.to_string(Base64), + "sha256:LCa0a2j/xo/5m0U8HTBBNBNCLXBkg7+g+YpeiGJm564="); +} + +} // namespace nix -- cgit 1.4.1