(maint) Fix permissions hacks

Applying permissions on Windows and using fs::permissions on Solaris
were both hacked around due to a faulty assumption that they were broken
on those platforms. The root cause of both issues was a static
initialization ordering issue.

Fix it by using a macro to alias the variable name, rather than
assigning from a global variable in another object. Unwind Windows and
Solaris hacks.

lib/inc/pxp-agent/configuration.hpp

@@ -24,11 +24,9 @@ namespace PXPAgent {
 
 extern const std::string DEFAULT_SPOOL_DIR;     // used by unit tests
 
-#ifndef _WIN32
-// Not used on Windows. We instead rely on inherited directory ACLs.
+// Note that on Windows we rely on inherited directory ACLs.
 extern const boost::filesystem::perms NIX_FILE_PERMS;
 extern const boost::filesystem::perms NIX_DIR_PERMS;
-#endif
 
 //
 // Types

lib/src/configuration.cc

@@ -104,10 +104,8 @@ static const std::string DEFAULT_SPOOL_DIR_PURGE_TTL { "14d" };
 
 static const std::string AGENT_CLIENT_TYPE { "agent" };
 
-#ifndef _WIN32
 const fs::perms NIX_FILE_PERMS { fs::owner_read | fs::owner_write | fs::group_read };
 const fs::perms NIX_DIR_PERMS  { NIX_FILE_PERMS | fs::owner_exe | fs::group_exe };
-#endif
 
 //
 // Public interface
@@ -228,9 +226,7 @@ std::string Configuration::setupLogging()
         // up logging before calling validateAndNormalizeConfiguration
         validateLogDirPath(logfile_);
         logfile_fstream_.open(logfile_.c_str(), std::ios_base::app);
-#ifndef _WIN32
         fs::permissions(logfile_, NIX_FILE_PERMS);
-#endif
 
         log_stream = &logfile_fstream_;
     } else {
@@ -243,9 +239,7 @@ std::string Configuration::setupLogging()
         pcp_access_fstream_ptr_.reset(
             new boost::nowide::ofstream(pcp_access_logfile_.c_str(),
                                         std::ios_base::app));
-#ifndef _WIN32
         fs::permissions(pcp_access_logfile_, NIX_FILE_PERMS);
-#endif
     }
 
 #ifndef _WIN32
@@ -936,15 +930,13 @@ void Configuration::validateAndNormalizeOtherSettings()
 
     fs::path task_cache_dir_path { lth_file::tilde_expand(task_cache_dir) };
     check_and_create_dir(task_cache_dir_path, "task-cache-dir", true);
-#ifndef _WIN32
     try {
         fs::permissions(task_cache_dir_path, NIX_DIR_PERMS);
     } catch (const fs::filesystem_error& e) {
             throw Configuration::Error {
                 lth_loc::format("Failed to make the task-cache-dir '{1}' user/group readable and executable, and user writable during configuration validation: {2}",
                             task_cache_dir_path.string(), e.what()) };
     }
-#endif
 
     HW::SetFlag<std::string>("task-cache-dir", task_cache_dir_path.string());
 

lib/src/external_module.cc

@@ -362,12 +362,8 @@ ActionResponse ExternalModule::callNonBlockingAction(const ActionRequest& reques
         std::map<std::string, std::string>(),  // environment
         [results_dir_path](size_t pid) {
             auto pid_file = (results_dir_path / "pid").string();
-#ifdef _WIN32
-            lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file);
-#else
             lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file,
                                            NIX_FILE_PERMS, std::ios::binary);
-#endif
         },          // pid callback
         0,          // timeout
         { lth_exec::execution_options::thread_safe,

lib/src/modules/task.cc

@@ -20,10 +20,6 @@
 
 #include <tuple>
 
-#ifdef __sun
-#include <sys/stat.h>
-#endif
-
 namespace PXPAgent {
 namespace Modules {
 
@@ -164,9 +160,8 @@ static Module::ProcessingError toModuleProcessingError(const fs::filesystem_erro
     }
 }
 
-#ifndef _WIN32
-static const fs::perms NIX_TASK_FILE_PERMS = NIX_DIR_PERMS;
-#endif
+// A define is used here because creating a local variable is subject to static initialization ordering.
+#define NIX_TASK_FILE_PERMS NIX_DIR_PERMS
 
 // Creates the <task_cache_dir>/<sha256> directory for a single
 // task, ensuring that its permissions are readable by
@@ -178,9 +173,7 @@ static fs::path createCacheDir(const fs::path& task_cache_dir, const std::string
     auto cache_dir = task_cache_dir / sha256;
     fs::create_directory(cache_dir);
     fs::last_write_time(cache_dir, time(nullptr));
-#ifndef _WIN32
     fs::permissions(cache_dir, NIX_DIR_PERMS);
-#endif
     return cache_dir;
 }
 
@@ -261,19 +254,7 @@ static std::tuple<bool, std::string> downloadTaskFile(const std::vector<std::str
         // timeout from connection after one minute, can configure
         req.connection_timeout(60000);
         try {
-#ifdef __sun
-            client.download_file(req, file_path.string(), NIX_TASK_FILE_PERMS);
-            // fs::permissions removes all permissions on Solaris. Use chmod directly to fix them as a workaround.
-            auto err = chmod(file_path.c_str(), S_IRWXU | S_IRGRP | S_IXGRP);
-            if (err) {
-                throw fs::filesystem_error("Failed to update mode of + " + file_path.string(),
-                    boost::system::error_code(errno, boost::system::system_category()));
-            }
-#elif !defined(_WIN32)
             client.download_file(req, file_path.string(), NIX_TASK_FILE_PERMS);
-#else
-            client.download_file(req, file_path.string());
-#endif
         } catch (lth_curl::http_file_download_exception& e) {
             // Server-side error, do nothing here -- we want to try the next master-uri.
             LOG_WARNING("Downloading the task file from the master-uri '{1}' failed. Reason: {2}", master_uri, e.what());
@@ -313,16 +294,7 @@ static fs::path updateTaskFile(const std::vector<std::string>& master_uris,
     auto filepath = cache_dir / filename;
 
     if (fs::exists(filepath) && sha256 == calculateSha256(filepath.string())) {
-#ifdef __sun
-        // fs::permissions removes all permissions on Solaris. Use chmod directly as a workaround.
-        auto err = chmod(filepath.c_str(), S_IRWXU | S_IRGRP | S_IXGRP);
-        if (err) {
-            throw fs::filesystem_error("Failed to update mode of + " + filepath.string(),
-                boost::system::error_code(errno, boost::system::system_category()));
-        }
-#elif !defined(_WIN32)
         fs::permissions(filepath, NIX_TASK_FILE_PERMS);
-#endif
         return filepath;
     }
 
@@ -416,12 +388,8 @@ void Task::callNonBlockingAction(
         environment,
         [results_dir](size_t pid) {
             auto pid_file = (results_dir / "pid").string();
-#ifdef _WIN32
-            lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file);
-#else
             lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file,
                                            NIX_FILE_PERMS, std::ios::binary);
-#endif
         },  // pid callback
         0,  // timeout
         leatherman::util::option_set<lth_exec::execution_options> {

lib/src/results_storage.cc

@@ -42,11 +42,7 @@ bool ResultsStorage::find(const std::string& transaction_id)
 
 static void writeMetadata(const std::string& txt, const std::string& file_path) {
     try {
-#ifdef _WIN32
-        lth_file::atomic_write_to_file(txt, file_path);
-#else
         lth_file::atomic_write_to_file(txt, file_path, NIX_FILE_PERMS, std::ios::binary);
-#endif
     } catch (const std::exception& e) {
         throw ResultsStorage::Error {
             lth_loc::format("failed to write metadata: {1}", e.what()) };
@@ -63,9 +59,7 @@ void ResultsStorage::initializeMetadataFile(const std::string& transaction_id,
                   transaction_id, results_path.string());
         try {
             fs::create_directories(results_path);
-#ifndef _WIN32
             fs::permissions(results_path, NIX_DIR_PERMS);
-#endif
         } catch (const fs::filesystem_error& e) {
             throw ResultsStorage::Error {
                 lth_loc::format("failed to create results directory '{1}'",