Merge pull request #8835 from Icinga/bugfix/api-filename-truncation

Fix/restrict truncation of filenames for API-created objects

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 <boost/thread/tss.hpp>
+#include <openssl/sha.h>
 #include <functional>
 #include <typeinfo>
 #include <vector>
@@ -146,6 +148,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<size_t maxLength>
+	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();
 

lib/remote/configobjectutility.cpp

@@ -35,15 +35,29 @@ 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 longPath = prefix + escapedName + ".conf";
-		return std::move(old);
+
+	/*
+	 * 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);
 	}
 
-	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)

test/CMakeLists.txt

@@ -118,6 +118,7 @@ add_boost_test(base
     base_utility/comparepasswords_issafe
     base_utility/validateutf8
     base_utility/EscapeCreateProcessArg
+    base_utility/TruncateUsingHash
     base_value/scalar
     base_value/convert
     base_value/format

test/base-utility.cpp

@@ -111,4 +111,28 @@ BOOST_AUTO_TEST_CASE(EscapeCreateProcessArg)
 #endif /* _WIN32 */
 }
 
+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()