Logfile open mode and permission plus location configurability. (#556)

* Unify logfile names and create with more secure open options and perms. Fixes #555 (Tribute to Colin McRae) * Logfile location configurability. Fixes #555
MisterTea · Jan 18, 2023 · 92c4c6a · 92c4c6a
1 parent 7289e04
commit 92c4c6a
Show file tree

Hide file tree

Showing 10 changed files with 127 additions and 94 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -53,6 +53,10 @@ jobs:
       - run:
           name: Kill server
           command: sudo pkill etserver
+      - run:
+          name: Debug info/logs if failed
+          when: always
+          command: ls -la /tmp/et*; sudo awk 'FNR==1 {print "XXXXXX", FILENAME, "XXXXXX"}{print}' /tmp/et*.log
 
   connect_with_jumphost:
     machine:
@@ -84,6 +88,10 @@ jobs:
       - run:
           name: Kill servers
           command: sudo pkill etserver
+      - run:
+          name: Debug info/logs if failed
+          when: always
+          command: ls -la /tmp/et*; sudo awk 'FNR==1 {print "XXXXXX", FILENAME, "XXXXXX"}{print}' /tmp/et*.log
 
 workflows:
   version: 2

diff --git a/etc/et.cfg b/etc/et.cfg
@@ -10,3 +10,4 @@ verbose = 0
 silent = 0
 logsize = 20971520
 telemetry = true
+logdirectory = /tmp
diff --git a/src/base/LogHandler.cpp b/src/base/LogHandler.cpp
@@ -4,8 +4,9 @@ INITIALIZE_EASYLOGGINGPP
 
 namespace et {
 el::Configurations LogHandler::setupLogHandler(int *argc, char ***argv) {
-  // easylogging parse verbose arguments, see [Application Arguments]
+  // easylogging parses verbose arguments, see [Application Arguments]
   // in https://github.com/muflihun/easyloggingpp/blob/master/README.md
+  // but it is non-intuitive so we explicitly set verbosity based on cxxopts
   START_EASYLOGGINGPP(*argc, *argv);
 
   // Easylogging configurations
@@ -23,13 +24,43 @@ el::Configurations LogHandler::setupLogHandler(int *argc, char ***argv) {
   return defaultConf;
 }
 
-void LogHandler::setupLogFile(el::Configurations *defaultConf, string filename,
-                              string maxlogsize) {
+void LogHandler::setupLogFiles(el::Configurations *defaultConf,
+                               const string &path, const string &filenamePrefix,
+                               bool logToStdout, bool redirectStderrToFile,
+                               bool appendPid, string maxlogsize) {
+  time_t rawtime;
+  struct tm *timeinfo;
+  char buffer[80];
+  time(&rawtime);
+  timeinfo = localtime(&rawtime);
+  strftime(buffer, sizeof(buffer), "%Y-%m-%d_%H-%M-%S", timeinfo);
+  string current_time(buffer);
+  string logFilename = filenamePrefix + "-" + current_time;
+  string stderrFilename = filenamePrefix + "-stderr-" + current_time;
+  if (appendPid) {
+    string pid = std::to_string(getpid());
+    logFilename.append("_" + pid);
+    stderrFilename.append("_" + pid);
+  }
+  logFilename.append(".log");
+  stderrFilename.append(".log");
+  string fullFname = createLogFile(path, logFilename);
+
   // Enable strict log file size check
   el::Loggers::addFlag(el::LoggingFlag::StrictLogFileSizeCheck);
-  defaultConf->setGlobally(el::ConfigurationType::Filename, filename);
+  defaultConf->setGlobally(el::ConfigurationType::Filename, fullFname);
   defaultConf->setGlobally(el::ConfigurationType::ToFile, "true");
   defaultConf->setGlobally(el::ConfigurationType::MaxLogFileSize, maxlogsize);
+
+  if (logToStdout) {
+    defaultConf->setGlobally(el::ConfigurationType::ToStandardOutput, "true");
+  } else {
+    defaultConf->setGlobally(el::ConfigurationType::ToStandardOutput, "false");
+  }
+
+  if (redirectStderrToFile) {
+    stderrToFile(path, stderrFilename);
+  }
 }
 
 void LogHandler::rolloutHandler(const char *filename, std::size_t size) {
@@ -38,27 +69,6 @@ void LogHandler::rolloutHandler(const char *filename, std::size_t size) {
   remove(filename);
 }
 
-string LogHandler::stderrToFile(const string &pathPrefix) {
-  time_t rawtime;
-  struct tm *timeinfo;
-  char buffer[80];
-  time(&rawtime);
-  timeinfo = localtime(&rawtime);
-  strftime(buffer, sizeof(buffer), "%Y-%m-%d_%I-%M", timeinfo);
-  string current_time(buffer);
-  string stderrFilename = pathPrefix + "_stderr_" + current_time;
-  FILE *stderr_stream = freopen(stderrFilename.c_str(), "w", stderr);
-  fs::permissions(
-      stderrFilename,
-      fs::perms::owner_read | fs::perms::owner_write | fs::perms::group_read,
-      fs::perm_options::replace);
-  if (!stderr_stream) {
-    STFATAL << "Invalid filename " << stderrFilename;
-  }
-  setvbuf(stderr_stream, NULL, _IOLBF, BUFSIZ);  // set to line buffering
-  return stderrFilename;
-}
-
 void LogHandler::setupStdoutLogger() {
   el::Logger *stdoutLogger = el::Loggers::getLogger("stdout");
   // Easylogging configurations
@@ -70,4 +80,33 @@ void LogHandler::setupStdoutLogger() {
   stdoutConf.setGlobally(el::ConfigurationType::ToFile, "false");
   el::Loggers::reconfigureLogger(stdoutLogger, stdoutConf);
 }
+
+string LogHandler::createLogFile(const string &path, const string &filename) {
+  string fullFname = path + "/" + filename;
+  try {
+    fs::create_directories(path);
+  } catch (const fs::filesystem_error &fse) {
+    CLOG(ERROR, "stdout") << "Cannot create logfile directory: " << fse.what()
+                          << endl;
+    exit(1);
+  }
+#ifdef WIN32
+  // O_NOFOLLOW does not exist on windows
+  FATAL_FAIL(::open(fullFname.c_str(), O_EXCL | O_CREAT, 0600));
+#else
+  FATAL_FAIL(::open(fullFname.c_str(), O_NOFOLLOW | O_EXCL | O_CREAT, 0600));
+#endif
+  return fullFname;
+}
+
+void LogHandler::stderrToFile(const string &path,
+                              const string &stderrFilename) {
+  string fullFname = createLogFile(path, stderrFilename);
+  FILE *stderr_stream = freopen(fullFname.c_str(), "w", stderr);
+  if (!stderr_stream) {
+    STFATAL << "Invalid filename " << stderrFilename;
+  }
+  setvbuf(stderr_stream, NULL, _IOLBF, BUFSIZ);  // set to line buffering
+}
+
 }  // namespace et
diff --git a/src/base/LogHandler.hpp b/src/base/LogHandler.hpp
@@ -7,11 +7,18 @@ namespace et {
 class LogHandler {
  public:
   static el::Configurations setupLogHandler(int *argc, char ***argv);
-  static void setupLogFile(el::Configurations *defaultConf, string filename,
-                           string maxlogsize = "20971520");
+  static void setupLogFiles(el::Configurations *defaultConf, const string &path,
+                            const string &filenamePrefix,
+                            bool logToStdout = false,
+                            bool redirectStderrToFile = false,
+                            bool appendPid = false,
+                            string maxlogsize = "20971520");
   static void rolloutHandler(const char *filename, std::size_t size);
-  static string stderrToFile(const string &pathPrefix);
   static void setupStdoutLogger();
+
+ private:
+  static void stderrToFile(const string &path, const string &stderrFilename);
+  static string createLogFile(const string &path, const string &filename);
 };
 }  // namespace et
 #endif  // __ET_LOG_HANDLER__
diff --git a/src/htm/HtmClientMain.cpp b/src/htm/HtmClientMain.cpp
@@ -61,14 +61,9 @@ int main(int argc, char** argv) {
 
   // Setup easylogging configurations
   el::Configurations defaultConf = LogHandler::setupLogHandler(&argc, &argv);
-  defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "false");
   el::Loggers::setVerboseLevel(3);
-  // default max log file size is 20MB for etserver
-  string maxlogsize = "20971520";
-  LogHandler::setupLogFile(&defaultConf, GetTempDirectory() + "htm.log",
-                           maxlogsize);
-  // Redirect std streams to a file
-  LogHandler::stderrToFile(GetTempDirectory() + "htm");
+  LogHandler::setupLogFiles(&defaultConf, GetTempDirectory(), "htm", false,
+                            true);
 
   // Reconfigure default logger to apply settings above
   el::Loggers::reconfigureLogger("default", defaultConf);

diff --git a/src/htm/HtmServerMain.cpp b/src/htm/HtmServerMain.cpp
@@ -13,14 +13,9 @@ int main(int argc, char **argv) {
   // Setup easylogging configurations
   el::Configurations defaultConf =
       et::LogHandler::setupLogHandler(&argc, &argv);
-  defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "false");
   el::Loggers::setVerboseLevel(3);
-  // default max log file size is 20MB for etserver
-  string maxlogsize = "20971520";
-  LogHandler::setupLogFile(&defaultConf, GetTempDirectory() + "htmd.log",
-                           maxlogsize);
-  // Redirect std streams to a file
-  LogHandler::stderrToFile(GetTempDirectory() + "htmd");
+  LogHandler::setupLogFiles(&defaultConf, GetTempDirectory(), "htmd", false,
+                            true);
 
   // Reconfigure default logger to apply settings above
   el::Loggers::reconfigureLogger("default", defaultConf);

diff --git a/src/terminal/TerminalClientMain.cpp b/src/terminal/TerminalClientMain.cpp
@@ -88,11 +88,13 @@ int main(int argc, char** argv) {
         ("v,verbose", "Enable verbose logging",
          cxxopts::value<int>()->default_value("0"))  //
         ("k,keepalive", "Client keepalive duration in seconds",
-         cxxopts::value<int>())                              //
-        ("logtostdout", "Write log to stdout")               //
-        ("silent", "Disable logging")                        //
-        ("N,no-terminal", "Do not create a terminal")        //
-        ("f,forward-ssh-agent", "Forward ssh-agent socket")  //
+         cxxopts::value<int>())  //
+        ("l,logdir", "Base directory for log files.",
+         cxxopts::value<std::string>()->default_value(tmpDir))  //
+        ("logtostdout", "Write log to stdout")                  //
+        ("silent", "Disable logging")                           //
+        ("N,no-terminal", "Do not create a terminal")           //
+        ("f,forward-ssh-agent", "Forward ssh-agent socket")     //
         ("ssh-socket", "The ssh-agent socket to forward",
          cxxopts::value<std::string>())  //
         ("telemetry",
@@ -119,21 +121,14 @@ int main(int argc, char** argv) {
 
     el::Loggers::setVerboseLevel(result["verbose"].as<int>());
 
-    if (result.count("logtostdout")) {
-      defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "true");
-    } else {
-      defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "false");
-      // Redirect std streams to a file
-      LogHandler::stderrToFile((tmpDir + "/etclient"));
-    }
-
     // silent Flag, since etclient doesn't read /etc/et.cfg file
     if (result.count("silent")) {
       defaultConf.setGlobally(el::ConfigurationType::Enabled, "false");
     }
 
-    LogHandler::setupLogFile(
-        &defaultConf, (tmpDir + "/etclient-%datetime{%Y-%M-%d_%H_%m_%s}.log"));
+    LogHandler::setupLogFiles(&defaultConf, result["logdir"].as<string>(),
+                              "etclient", result.count("logtostdout"),
+                              !result.count("logtostdout"));
 
     el::Loggers::reconfigureLogger("default", defaultConf);
     // set thread name

diff --git a/src/terminal/TerminalMain.cpp b/src/terminal/TerminalMain.cpp
@@ -48,7 +48,9 @@ int main(int argc, char** argv) {
         // Not used by etterminal but easylogging uses this flag under the hood
         ("v,verbose", "Enable verbose logging",
          cxxopts::value<int>()->default_value("0"))  //
-        ("logtostdout", "Write log to stdout")       //
+        ("l,logdir", "Base directory for log files.",
+         cxxopts::value<std::string>()->default_value(GetTempDirectory()))  //
+        ("logtostdout", "Write log to stdout")                              //
         ("serverfifo",
          "If set, connects to the etserver instance listening on the matching "
          "fifo name",                                       //
@@ -63,15 +65,6 @@ int main(int argc, char** argv) {
 
     el::Loggers::setVerboseLevel(result["verbose"].as<int>());
 
-    if (result.count("logtostdout")) {
-      defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "true");
-    } else {
-      defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "false");
-    }
-
-    // default max log file size is 20MB for etserver
-    string maxlogsize = "20971520";
-
     GOOGLE_PROTOBUF_VERIFY_VERSION;
     srand(1);
 
@@ -147,10 +140,9 @@ int main(int argc, char** argv) {
     string username = string(ssh_get_local_username());
     if (result.count("jump")) {
       // etserver with --jump cannot write to the default log file(root)
-      LogHandler::setupLogFile(
-          &defaultConf,
-          GetTempDirectory() + "etjump-" + username + "-" + id + ".log",
-          maxlogsize);
+      LogHandler::setupLogFiles(&defaultConf, result["logdir"].as<string>(),
+                                ("etjump-" + username + "-" + id),
+                                result.count("logtostdout"), false);
       // Reconfigure default logger to apply settings above
       el::Loggers::reconfigureLogger("default", defaultConf);
       // set thread name
@@ -177,10 +169,10 @@ int main(int argc, char** argv) {
     }
 
     // etserver with --idpasskey cannot write to the default log file(root)
-    LogHandler::setupLogFile(
-        &defaultConf,
-        GetTempDirectory() + "etterminal-" + username + "-" + id + ".log",
-        maxlogsize);
+    LogHandler::setupLogFiles(&defaultConf, result["logdir"].as<string>(),
+                              ("etterminal-" + username + "-" + id),
+                              result.count("logtostdout"), false);
+
     // Reconfigure default logger to apply settings above
     el::Loggers::reconfigureLogger("default", defaultConf);
     // set thread name

diff --git a/src/terminal/TerminalServerMain.cpp b/src/terminal/TerminalServerMain.cpp
@@ -37,7 +37,9 @@ int main(int argc, char **argv) {
         ("daemon", "Daemonize the server")             //
         ("cfgfile", "Location of the config file",
          cxxopts::value<std::string>()->default_value(""))  //
-        ("logtostdout", "log to stdout")                    //
+        ("l,logdir", "Base directory for log files.",
+         cxxopts::value<std::string>())   //
+        ("logtostdout", "log to stdout")  //
         ("pidfile", "Location of the pid file",
          cxxopts::value<std::string>()->default_value(
              "/var/run/etserver.pid"))  //
@@ -70,14 +72,6 @@ int main(int argc, char **argv) {
       }
     }
 
-    if (result.count("logtostdout")) {
-      defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "true");
-    } else {
-      defaultConf.setGlobally(el::ConfigurationType::ToStandardOutput, "false");
-      // Redirect std streams to a file
-      LogHandler::stderrToFile(GetTempDirectory() + "etserver");
-    }
-
     ServerFifoPath serverFifo;
 
     // default max log file size is 20MB for etserver
@@ -86,6 +80,7 @@ int main(int argc, char **argv) {
     int port = 0;
     string bindIp = "";
     bool telemetry = false;
+    string logDirectory = GetTempDirectory();
     if (result.count("cfgfile")) {
       // Load the config file
       CSimpleIniA ini(true, false, false);
@@ -128,13 +123,19 @@ int main(int argc, char **argv) {
         if (silent && atoi(silent) != 0) {
           defaultConf.setGlobally(el::ConfigurationType::Enabled, "false");
         }
+
         // read log file size limit
         const char *logsize = ini.GetValue("Debug", "logsize", NULL);
         if (logsize && atoi(logsize) != 0) {
           // make sure maxlogsize is a string of int value
           maxlogsize = string(logsize);
         }
 
+        // log file directory (TODO path validation and trailing slash cleanup)
+        const char *logdir = ini.GetValue("Debug", "logdirectory", NULL);
+        if (logdir) {
+          logDirectory = string(logdir);
+        }
       } else {
         STFATAL << "Invalid config file: " << cfgfilename;
       }
@@ -157,6 +158,10 @@ int main(int argc, char **argv) {
       telemetry = result["telemetry"].as<bool>();
     }
 
+    if (result.count("logdir")) {
+      logDirectory = result["logdir"].as<string>();
+    }
+
     GOOGLE_PROTOBUF_VERIFY_VERSION;
     srand(1);
 
@@ -165,9 +170,9 @@ int main(int argc, char **argv) {
     }
 
     // Set log file for etserver process here.
-    LogHandler::setupLogFile(&defaultConf,
-                             GetTempDirectory() + "etserver-%datetime.log",
-                             maxlogsize);
+    LogHandler::setupLogFiles(
+        &defaultConf, logDirectory, "etserver", result.count("logtostdout"),
+        !result.count("logtostdout"), true /* appendPid */, maxlogsize);
     // Reconfigure default logger to apply settings above
     el::Loggers::reconfigureLogger("default", defaultConf);
     // set thread name