From c174456ed30baef48d69116fa3a29a42293f6a95 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Tue, 12 Jan 2021 18:03:22 +0100 Subject: [PATCH 1/4] ConfigObjectUtility::GetObjectConfigPath(): hash names of not already existing objects ... to avoid too long file names. refs #8022 --- lib/remote/configobjectutility.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 1363dfb06..4c36a63c8 100644 --- a/lib/remote/configobjectutility.cpp +++ b/lib/remote/configobjectutility.cpp @@ -8,11 +8,13 @@ #include "base/configwriter.hpp" #include "base/exception.hpp" #include "base/dependencygraph.hpp" +#include "base/tlsutility.hpp" #include "base/utility.hpp" #include #include #include #include +#include using namespace icinga; @@ -35,8 +37,13 @@ String ConfigObjectUtility::GetObjectConfigPath(const Type::Ptr& type, const Str /* This may throw an exception the caller above must handle. */ String prefix = GetConfigDir(); - return prefix + "/conf.d/" + typeDir + - "/" + EscapeName(fullName) + ".conf"; + auto old (prefix + "/conf.d/" + typeDir + "/" + EscapeName(fullName) + ".conf"); + + if (fullName.GetLength() <= 80u + 3u /* "..." */ + 40u /* hex SHA1 */ || Utility::PathExists(old)) { + return std::move(old); + } + + return prefix + "/conf.d/" + typeDir + "/" + fullName.SubStr(0, 80) + "..." + SHA1(fullName) + ".conf"; } void ConfigObjectUtility::RepairPackage(const String& package) From fc0019b27124f31c2c652366d8579acda4941a35 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 17 Jun 2021 08:49:54 +0200 Subject: [PATCH 2/4] Utility: add a function to truncate strings while avoiding collisions --- lib/base/utility.hpp | 36 ++++++++++++++++++++++++++++++++++++ test/CMakeLists.txt | 1 + test/base-utility.cpp | 24 ++++++++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/lib/base/utility.hpp b/lib/base/utility.hpp index 4505dc918..a709d6313 100644 --- a/lib/base/utility.hpp +++ b/lib/base/utility.hpp @@ -7,7 +7,9 @@ #include "base/string.hpp" #include "base/array.hpp" #include "base/threadpool.hpp" +#include "base/tlsutility.hpp" #include +#include #include #include @@ -144,6 +146,40 @@ public: static void IncrementTime(double); #endif /* I2_DEBUG */ + /** + * TruncateUsingHash truncates a given string to an allowed maximum length while avoiding collisions in the output + * using a hash function (SHA1). + * + * For inputs shorter than the maximum output length, the output will be the same as the input. If the input has at + * least the maximum output length, it is hashed used SHA1 and the output has the format "A...B" where A is a prefix + * of the input and B is the hex-encoded SHA1 hash of the input. The length of A is chosen so that the result has + * the maximum allowed output length. + * + * @tparam maxLength Maximum length of the output string (must be at least 44) + * @param in String to truncate + * @return A truncated string derived from in of at most length maxLength + */ + template + static String TruncateUsingHash(const String &in) { + /* + * Note: be careful when changing this function as it is used to derive file names that should not change + * between versions or would need special handling if they do (/var/lib/icinga2/api/packages/_api). + */ + + const size_t sha1HexLength = SHA_DIGEST_LENGTH*2; + static_assert(maxLength >= 1 + 3 + sha1HexLength, + "maxLength must be at least 44 to hold one character, '...', and a hex-encoded SHA1 hash"); + + /* If the input is shorter than the limit, no truncation is needed */ + if (in.GetLength() < maxLength) { + return in; + } + + const char *trunc = "..."; + + return in.SubStr(0, maxLength - sha1HexLength - strlen(trunc)) + trunc + SHA1(in); + } + private: Utility(); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index bfcd83ffd..2ca656dac 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -113,6 +113,7 @@ add_boost_test(base base_utility/comparepasswords_works base_utility/comparepasswords_issafe base_utility/validateutf8 + base_utility/TruncateUsingHash base_value/scalar base_value/convert base_value/format diff --git a/test/base-utility.cpp b/test/base-utility.cpp index e25179229..0b200692b 100644 --- a/test/base-utility.cpp +++ b/test/base-utility.cpp @@ -75,4 +75,28 @@ BOOST_AUTO_TEST_CASE(validateutf8) BOOST_CHECK(Utility::ValidateUTF8("\xC3\xA4") == "\xC3\xA4"); } +BOOST_AUTO_TEST_CASE(TruncateUsingHash) +{ + /* + * Note: be careful when changing the output of TruncateUsingHash as it is used to derive file names that should not + * change between versions or would need special handling if they do (/var/lib/icinga2/api/packages/_api). + */ + + /* minimum allowed value for maxLength template parameter */ + BOOST_CHECK_EQUAL(Utility::TruncateUsingHash<44>(std::string(64, 'a')), + "a...0098ba824b5c16427bd7a1122a5a442a25ec644d"); + + BOOST_CHECK_EQUAL(Utility::TruncateUsingHash<80>(std::string(100, 'a')), + std::string(37, 'a') + "...7f9000257a4918d7072655ea468540cdcbd42e0c"); + + /* short enough values should not be truncated */ + BOOST_CHECK_EQUAL(Utility::TruncateUsingHash<80>(""), ""); + BOOST_CHECK_EQUAL(Utility::TruncateUsingHash<80>(std::string(60, 'a')), std::string(60, 'a')); + BOOST_CHECK_EQUAL(Utility::TruncateUsingHash<80>(std::string(79, 'a')), std::string(79, 'a')); + + /* inputs of maxLength are hashed to avoid collisions */ + BOOST_CHECK_EQUAL(Utility::TruncateUsingHash<80>(std::string(80, 'a')), + std::string(37, 'a') + "...86f33652fcffd7fa1443e246dd34fe5d00e25ffd"); +} + BOOST_AUTO_TEST_SUITE_END() From 3db48de0e62311838c443d00e61d2330c3a8f4e5 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 17 Jun 2021 09:02:58 +0200 Subject: [PATCH 3/4] GetObjectPath: ensure use of escaped name in all cases and use TruncateUsingHash() 68a0079c26686363b6202a8abd2712d2bf96d9f2 introduced two problems that are fixed with this commit: 1. The new truncated/hashed name did not use EscapeName() 2. There was a possible collision of names when creating objects with a full name of format "[80 characters]...[40 hex digits]" (i.e. the same as the truncated/hashed variant but short enough that it isn't hashed) --- lib/remote/configobjectutility.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 4c36a63c8..131f7313f 100644 --- a/lib/remote/configobjectutility.cpp +++ b/lib/remote/configobjectutility.cpp @@ -35,15 +35,17 @@ String ConfigObjectUtility::GetObjectConfigPath(const Type::Ptr& type, const Str boost::algorithm::to_lower(typeDir); /* This may throw an exception the caller above must handle. */ - String prefix = GetConfigDir(); + String prefix = GetConfigDir() + "/conf.d/" + type->GetPluralName().ToLower() + "/"; - auto old (prefix + "/conf.d/" + typeDir + "/" + EscapeName(fullName) + ".conf"); + String escapedName = EscapeName(fullName); - if (fullName.GetLength() <= 80u + 3u /* "..." */ + 40u /* hex SHA1 */ || Utility::PathExists(old)) { + String old = prefix + escapedName + ".conf"; + if (Utility::PathExists(old)) { return std::move(old); } - return prefix + "/conf.d/" + typeDir + "/" + fullName.SubStr(0, 80) + "..." + SHA1(fullName) + ".conf"; + /* Maximum length 80 bytes object name + 3 bytes "..." + 40 bytes SHA1 (hex-encoded) */ + return prefix + Utility::TruncateUsingHash<80+3+40>(escapedName) + ".conf"; } void ConfigObjectUtility::RepairPackage(const String& package) From 2dc5c9e47b21a2f550b77f8fd65148ce345f4a75 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 17 Jun 2021 10:52:25 +0200 Subject: [PATCH 4/4] GetObjectConfigPath: only truncate and hash comment and downtime filenames This partially reverts 68a0079c26686363b6202a8abd2712d2bf96d9f2 and keeps the fix only for comment and downtime objects for now. For reasoning, please see the comment in the code. --- lib/remote/configobjectutility.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 131f7313f..7dc71ba41 100644 --- a/lib/remote/configobjectutility.cpp +++ b/lib/remote/configobjectutility.cpp @@ -39,9 +39,21 @@ String ConfigObjectUtility::GetObjectConfigPath(const Type::Ptr& type, const Str String escapedName = EscapeName(fullName); - String old = prefix + escapedName + ".conf"; - if (Utility::PathExists(old)) { - return std::move(old); + String longPath = prefix + escapedName + ".conf"; + + /* + * The long path may cause trouble due to exceeding the allowed filename length of the filesystem. Therefore, the + * preferred solution would be to use the truncated and hashed version as returned at the end of this function. + * However, for compatibility reasons, we have to keep the old long version in some cases. Notably, this could lead + * to the creation of objects that can't be synced to child nodes if they are running an older version. Thus, for + * now, the fix is only enabled for comments and downtimes, as these are the object types for which the issue is + * most likely triggered but can't be worked around easily (you'd have to rename the host and/or service in order to + * be able to schedule a downtime or add an acknowledgement, which is not feasible) and the impact of not syncing + * these objects through the whole cluster is limited. For other object types, we currently prefer to fail the + * creation early so that configuration inconsistencies throughout the cluster are avoided. + */ + if ((type->GetName() != "Comment" && type->GetName() != "Downtime") || Utility::PathExists(longPath)) { + return std::move(longPath); } /* Maximum length 80 bytes object name + 3 bytes "..." + 40 bytes SHA1 (hex-encoded) */