From 172a6f2e0dc99dae3f102597bb76a825ffda972f Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Thu, 21 Oct 2021 15:17:43 +0300 Subject: [PATCH 1/2] Fix reading from empty file on encrypted disk. --- src/Disks/DiskEncrypted.cpp | 8 + src/Disks/tests/gtest_disk_encrypted.cpp | 194 +++++++++++++++++++++++ src/IO/ReadBufferFromFileDecorator.cpp | 10 +- src/IO/ReadBufferFromFileDecorator.h | 2 + 4 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 src/Disks/tests/gtest_disk_encrypted.cpp diff --git a/src/Disks/DiskEncrypted.cpp b/src/Disks/DiskEncrypted.cpp index 6a96caed6d5..bbcdb118510 100644 --- a/src/Disks/DiskEncrypted.cpp +++ b/src/Disks/DiskEncrypted.cpp @@ -4,6 +4,8 @@ #include #include #include +#include +#include #include #include @@ -243,6 +245,12 @@ std::unique_ptr DiskEncrypted::readFile( { auto wrapped_path = wrappedPath(path); auto buffer = delegate->readFile(wrapped_path, settings, estimated_size); + if (buffer->eof()) + { + /// File is empty, that's a normal case, see DiskEncrypted::truncateFile(). + /// There is no header so we just return `ReadBufferFromString("")`. + return std::make_unique(std::make_unique(std::string_view{}), wrapped_path); + } auto encryption_settings = current_settings.get(); FileEncryption::Header header = readHeader(*buffer); String key = getKey(path, header, *encryption_settings); diff --git a/src/Disks/tests/gtest_disk_encrypted.cpp b/src/Disks/tests/gtest_disk_encrypted.cpp new file mode 100644 index 00000000000..5d1e6c739db --- /dev/null +++ b/src/Disks/tests/gtest_disk_encrypted.cpp @@ -0,0 +1,194 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace DB; + +constexpr auto kHeaderSize = FileEncryption::Header::kSize; + + +class DiskEncryptedTest : public ::testing::Test +{ +protected: + void SetUp() override + { + /// Make local disk. + temp_dir = std::make_unique(); + temp_dir->createDirectories(); + local_disk = std::make_shared("local_disk", getDirectory(), 0); + } + + void TearDown() override + { + encrypted_disk.reset(); + local_disk.reset(); + } + + void makeEncryptedDisk(FileEncryption::Algorithm algorithm, const String & key) + { + auto settings = std::make_unique(); + settings->wrapped_disk = local_disk; + settings->current_algorithm = algorithm; + settings->keys[0] = key; + settings->current_key_id = 0; + encrypted_disk = std::make_shared("encrypted_disk", std::move(settings)); + } + + String getFileNames() + { + Strings file_names; + encrypted_disk->listFiles("", file_names); + return boost::algorithm::join(file_names, ", "); + } + + String getDirectory() + { + return temp_dir->path() + "/"; + } + + String getFileContents(const String & file_name) + { + auto buf = encrypted_disk->readFile(file_name, {}, 0); + String str; + readStringUntilEOF(str, *buf); + return str; + } + + static void checkBinaryRepresentation(const String & abs_path, size_t size) + { + auto buf = createReadBufferFromFileBase(abs_path, {}, 0); + String str; + readStringUntilEOF(str, *buf); + EXPECT_EQ(str.size(), size); + if (str.size() >= 3) + EXPECT_EQ(str.substr(0, 3), "ENC"); + } + + std::unique_ptr temp_dir; + std::shared_ptr local_disk; + std::shared_ptr encrypted_disk; +}; + + +TEST_F(DiskEncryptedTest, WriteAndRead) +{ + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456"); + + /// No files + EXPECT_EQ(getFileNames(), ""); + + /// Write a file. + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + writeString(std::string_view{"Some text"}, *buf); + } + + /// Now we have one file. + EXPECT_EQ(getFileNames(), "a.txt"); + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 9); + + /// Read the file. + EXPECT_EQ(getFileContents("a.txt"), "Some text"); + checkBinaryRepresentation(getDirectory() + "a.txt", kHeaderSize + 9); + + /// Remove the file. + encrypted_disk->removeFile("a.txt"); + + /// No files again. + EXPECT_EQ(getFileNames(), ""); +} + + +TEST_F(DiskEncryptedTest, Append) +{ + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456"); + + /// Write a file (we use the append mode). + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Append); + writeString(std::string_view{"Some text"}, *buf); + } + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 9); + EXPECT_EQ(getFileContents("a.txt"), "Some text"); + checkBinaryRepresentation(getDirectory() + "a.txt", kHeaderSize + 9); + + /// Append the file. + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Append); + writeString(std::string_view{" Another text"}, *buf); + } + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 22); + EXPECT_EQ(getFileContents("a.txt"), "Some text Another text"); + checkBinaryRepresentation(getDirectory() + "a.txt", kHeaderSize + 22); +} + + +TEST_F(DiskEncryptedTest, Truncate) +{ + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456"); + + /// Write a file (we use the append mode). + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Append); + writeString(std::string_view{"Some text"}, *buf); + } + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 9); + EXPECT_EQ(getFileContents("a.txt"), "Some text"); + checkBinaryRepresentation(getDirectory() + "a.txt", kHeaderSize + 9); + + /// Truncate the file. + encrypted_disk->truncateFile("a.txt", 4); + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 4); + EXPECT_EQ(getFileContents("a.txt"), "Some"); + checkBinaryRepresentation(getDirectory() + "a.txt", kHeaderSize + 4); + + /// Truncate the file to zero size. + encrypted_disk->truncateFile("a.txt", 0); + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 0); + EXPECT_EQ(getFileContents("a.txt"), ""); + checkBinaryRepresentation(getDirectory() + "a.txt", 0); +} + + +TEST_F(DiskEncryptedTest, ZeroFileSize) +{ + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456"); + + /// Write nothing to a file. + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + } + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 0); + EXPECT_EQ(getFileContents("a.txt"), ""); + checkBinaryRepresentation(getDirectory() + "a.txt", 0); + + /// Append the file with nothing. + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Append); + } + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 0); + EXPECT_EQ(getFileContents("a.txt"), ""); + checkBinaryRepresentation(getDirectory() + "a.txt", 0); + + /// Truncate the file to zero size. + encrypted_disk->truncateFile("a.txt", 0); + + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 0); + EXPECT_EQ(getFileContents("a.txt"), ""); + checkBinaryRepresentation(getDirectory() + "a.txt", 0); +} diff --git a/src/IO/ReadBufferFromFileDecorator.cpp b/src/IO/ReadBufferFromFileDecorator.cpp index 5810eccbac7..f4a996fc278 100644 --- a/src/IO/ReadBufferFromFileDecorator.cpp +++ b/src/IO/ReadBufferFromFileDecorator.cpp @@ -5,7 +5,13 @@ namespace DB { ReadBufferFromFileDecorator::ReadBufferFromFileDecorator(std::unique_ptr impl_) - : impl(std::move(impl_)) + : ReadBufferFromFileDecorator(std::move(impl_), "") +{ +} + + +ReadBufferFromFileDecorator::ReadBufferFromFileDecorator(std::unique_ptr impl_, const String & file_name_) + : impl(std::move(impl_)), file_name(file_name_) { swap(*impl); } @@ -13,6 +19,8 @@ ReadBufferFromFileDecorator::ReadBufferFromFileDecorator(std::unique_ptr(impl.get())) return buffer->getFileName(); return std::string(); diff --git a/src/IO/ReadBufferFromFileDecorator.h b/src/IO/ReadBufferFromFileDecorator.h index 1122e02bb20..c83ec669203 100644 --- a/src/IO/ReadBufferFromFileDecorator.h +++ b/src/IO/ReadBufferFromFileDecorator.h @@ -11,6 +11,7 @@ class ReadBufferFromFileDecorator : public ReadBufferFromFileBase { public: explicit ReadBufferFromFileDecorator(std::unique_ptr impl_); + ReadBufferFromFileDecorator(std::unique_ptr impl_, const String & file_name_); std::string getFileName() const override; @@ -22,6 +23,7 @@ public: protected: std::unique_ptr impl; + String file_name; }; } From 26cb62de3287e068eebbfa332c3e61400a98e68f Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sat, 23 Oct 2021 20:58:34 +0300 Subject: [PATCH 2/2] Add more tests for the "encrypted" disk type. --- src/Disks/tests/gtest_disk_encrypted.cpp | 102 ++++++++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/src/Disks/tests/gtest_disk_encrypted.cpp b/src/Disks/tests/gtest_disk_encrypted.cpp index 5d1e6c739db..6f49f467734 100644 --- a/src/Disks/tests/gtest_disk_encrypted.cpp +++ b/src/Disks/tests/gtest_disk_encrypted.cpp @@ -32,13 +32,14 @@ protected: local_disk.reset(); } - void makeEncryptedDisk(FileEncryption::Algorithm algorithm, const String & key) + void makeEncryptedDisk(FileEncryption::Algorithm algorithm, const String & key, const String & path = "") { auto settings = std::make_unique(); settings->wrapped_disk = local_disk; settings->current_algorithm = algorithm; settings->keys[0] = key; settings->current_key_id = 0; + settings->disk_path = path; encrypted_disk = std::make_shared("encrypted_disk", std::move(settings)); } @@ -62,14 +63,22 @@ protected: return str; } - static void checkBinaryRepresentation(const String & abs_path, size_t size) + static String getBinaryRepresentation(const String & abs_path) { auto buf = createReadBufferFromFileBase(abs_path, {}, 0); String str; readStringUntilEOF(str, *buf); + return str; + } + + static void checkBinaryRepresentation(const String & abs_path, size_t size) + { + String str = getBinaryRepresentation(abs_path); EXPECT_EQ(str.size(), size); if (str.size() >= 3) + { EXPECT_EQ(str.substr(0, 3), "ENC"); + } } std::unique_ptr temp_dir; @@ -192,3 +201,92 @@ TEST_F(DiskEncryptedTest, ZeroFileSize) EXPECT_EQ(getFileContents("a.txt"), ""); checkBinaryRepresentation(getDirectory() + "a.txt", 0); } + + +TEST_F(DiskEncryptedTest, AnotherFolder) +{ + /// Encrypted disk will store its files at the path "folder1/folder2/". + local_disk->createDirectories("folder1/folder2"); + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456", "folder1/folder2/"); + + /// Write a file. + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + writeString(std::string_view{"Some text"}, *buf); + } + + /// Now we have one file. + EXPECT_EQ(getFileNames(), "a.txt"); + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 9); + + /// Read the file. + EXPECT_EQ(getFileContents("a.txt"), "Some text"); + checkBinaryRepresentation(getDirectory() + "folder1/folder2/a.txt", kHeaderSize + 9); +} + + +TEST_F(DiskEncryptedTest, RandomIV) +{ + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456"); + + /// Write two files with the same contents. + { + auto buf = encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + writeString(std::string_view{"Some text"}, *buf); + } + { + auto buf = encrypted_disk->writeFile("b.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + writeString(std::string_view{"Some text"}, *buf); + } + + /// Now we have two files. + EXPECT_EQ(encrypted_disk->getFileSize("a.txt"), 9); + EXPECT_EQ(encrypted_disk->getFileSize("b.txt"), 9); + + /// Read the files. + EXPECT_EQ(getFileContents("a.txt"), "Some text"); + EXPECT_EQ(getFileContents("b.txt"), "Some text"); + checkBinaryRepresentation(getDirectory() + "a.txt", kHeaderSize + 9); + checkBinaryRepresentation(getDirectory() + "b.txt", kHeaderSize + 9); + + String bina = getBinaryRepresentation(getDirectory() + "a.txt"); + String binb = getBinaryRepresentation(getDirectory() + "b.txt"); + constexpr size_t iv_offset = 16; + constexpr size_t iv_size = FileEncryption::InitVector::kSize; + EXPECT_EQ(bina.substr(0, iv_offset), binb.substr(0, iv_offset)); /// Part of the header before IV is the same. + EXPECT_NE(bina.substr(iv_offset, iv_size), binb.substr(iv_offset, iv_size)); /// IV differs. + EXPECT_EQ(bina.substr(iv_offset + iv_size, kHeaderSize - iv_offset - iv_size), + binb.substr(iv_offset + iv_size, kHeaderSize - iv_offset - iv_size)); /// Part of the header after IV is the same. + EXPECT_NE(bina.substr(kHeaderSize), binb.substr(kHeaderSize)); /// Encrypted data differs. +} + + +#if 0 +/// TODO: Try to change DiskEncrypted::writeFile() to fix this test. +/// It fails sometimes with quite an unexpected error: +/// libc++abi: terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: +/// filesystem error: in file_size: No such file or directory [/tmp/tmp14608aaaaaa/a.txt] +/// Aborted (core dumped) +/// It happens because for encrypted disks file appending is not atomic (see DiskEncrypted::writeFile()) +/// and a file could be removed after checking its existence but before getting its size. +TEST_F(DiskEncryptedTest, RemoveFileDuringWriting) +{ + makeEncryptedDisk(FileEncryption::Algorithm::AES_128_CTR, "1234567890123456"); + + size_t n = 100000; + std::thread t1{[&] + { + for (size_t i = 0; i != n; ++i) + encrypted_disk->writeFile("a.txt", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Append); + }}; + + std::thread t2{[&] + { + for (size_t i = 0; i != n; ++i) + encrypted_disk->removeFileIfExists("a.txt"); + }}; + + t1.join(); + t2.join(); +} +#endif