[#4086] Redact schema init parameters

new file:   changelog_unreleased/4086-password-leaked-to-logs

/src/lib/asiolink/process_spawn.*
     ProcessSpawnImpl::getCommandLine() - modified to accept
     ProcessSpawn::getCommandLine() - modified to accept
     a list of arguments whose values should be redacted

/src/lib/asiolink/tests/process_spawn_unittest.cc
    TEST_F(ProcessSpawnTest, getCommandLineRedacted) - new test

/src/lib/mysql/mysql_connection.cc
    MySqlConnection::initializeSchema() - add list of args to redact to
    getCommandLine() call

/src/lib/pgsql/pgsql_connection.cc
    PgSqlConnection::initializeSchema() - add list of args to redact to
    getCommandLine() call

diff --git a/changelog_unreleased/4086-password-leaked-to-logs b/changelog_unreleased/4086-password-leaked-to-logs
new file mode 100644 (file)
index 0000000..386a631
--- /dev/null
+++ b/changelog_unreleased/4086-password-leaked-to-logs
@@ -0,0 +1,5 @@
+[sec]          tmark
+       Removed logging of database password as clear
+       text when kea-dhcp4 or kea-dhcp6 initialize
+       the schema.
+       (Gitlab #4086)

diff --git a/src/lib/asiolink/process_spawn.cc b/src/lib/asiolink/process_spawn.cc
index 1d91075cef065c580457d0c9cc3ff0f1437616ad..5bc868d01eafdee091827e7990e2c1c1425da007 100644 (file)
--- a/src/lib/asiolink/process_spawn.cc
+++ b/src/lib/asiolink/process_spawn.cc
@@ -93,7 +93,8 @@ public:
     ~ProcessSpawnImpl();
 
     /// @brief Returns full command line, including arguments, for the process.
-    std::string getCommandLine() const;
+    /// @param redact_args list of arguments to redact.
+    std::string getCommandLine(std::unordered_set<std::string> redact_args = {}) const;
 
     /// @brief Spawn the new process.
     ///
@@ -316,15 +317,26 @@ ProcessSpawnImpl::~ProcessSpawnImpl() {
 }
 
 std::string
-ProcessSpawnImpl::getCommandLine() const {
+ProcessSpawnImpl::getCommandLine(std::unordered_set<std::string> redact_args /* = {} */) const {
     std::ostringstream s;
     s << executable_;
     // Start with index 1, because the first argument duplicates the
     // path to the executable. Note, that even if there are no parameters
     // the minimum size of the table is 2.
     int i = 1;
+    bool redact_next = false;
     while (args_[i] != NULL) {
-        s << " " << args_[i];
+        if (redact_next) {
+            s << " " << "*****";
+            redact_next = false;
+        } else {
+            if (redact_args.contains(args_[i])) {
+                redact_next = true;
+            }
+
+            s << " " << args_[i];
+        }
+
         ++i;
     }
     return (s.str());
@@ -489,8 +501,8 @@ ProcessSpawn::ProcessSpawn(const SpawnMode mode,
 }
 
 std::string
-ProcessSpawn::getCommandLine() const {
-    return (impl_->getCommandLine());
+ProcessSpawn::getCommandLine(std::unordered_set<std::string> redact_args /* = {} */) const {
+    return (impl_->getCommandLine(redact_args));
 }
 
 pid_t

diff --git a/src/lib/asiolink/process_spawn.h b/src/lib/asiolink/process_spawn.h
index 394dc95728d1b013119ceca0b5561c487bde94df..aabdaad09d3de70bc534f0f26c3c3aae99efecf1 100644 (file)
--- a/src/lib/asiolink/process_spawn.h
+++ b/src/lib/asiolink/process_spawn.h
@@ -15,6 +15,8 @@
 #include <vector>
 #include <boost/shared_ptr.hpp>
 
+#include <unordered_set>
+
 namespace isc {
 namespace asiolink {
 
@@ -85,7 +87,9 @@ public:
     ~ProcessSpawn() = default;
 
     /// @brief Returns full command line, including arguments, for the process.
-    std::string getCommandLine() const;
+    ///
+    /// @param redact_args list of arguments to redact
+    std::string getCommandLine(std::unordered_set<std::string> redact_args = {}) const;
 
     /// @brief Spawn the new process.
     ///

diff --git a/src/lib/asiolink/tests/process_spawn_unittest.cc b/src/lib/asiolink/tests/process_spawn_unittest.cc
index 6ea7e696278e23f31ffcadd01f057359257ea9a7..10a898c21b654d536bf83166dcc6f73e106a6a02 100644 (file)
--- a/src/lib/asiolink/tests/process_spawn_unittest.cc
+++ b/src/lib/asiolink/tests/process_spawn_unittest.cc
@@ -579,4 +579,30 @@ TEST_F(ProcessSpawnTest, inheritEnvWithParentVarSync) {
     EXPECT_EQ(34, process.getExitStatus(pid));
 }
 
+// This test verifies that the full command line for the process is
+// returned with specific arguments redacted.
+TEST_F(ProcessSpawnTest, getCommandLineRedacted) {
+    {
+        // Case 1: arguments present.
+        ProcessArgs args;
+        args.push_back("db-init");
+        args.push_back("mysql");
+        args.push_back("--host");
+        args.push_back("example.com");
+        args.push_back("--user");
+        args.push_back("someone");
+        args.push_back("--password");
+        args.push_back("sesame");
+        args.push_back("--other");
+        args.push_back("stuff");
+        args.push_back("foo");
+
+        ProcessSpawn process(ProcessSpawn::ASYNC, TEST_SCRIPT_SH, args);
+        std::string expected = TEST_SCRIPT_SH;
+        expected += " db-init mysql --host example.com --user ***** --password ***** --other stuff foo";
+        std::unordered_set<std::string> redact_args = { "--user", "--password", "--not-there" };
+        EXPECT_EQ(expected, process.getCommandLine(redact_args));
+    }
+}
+
 } // end of anonymous namespace

diff --git a/src/lib/mysql/mysql_connection.cc b/src/lib/mysql/mysql_connection.cc
index ba7006d64d88827e0ed292a61d6fd0eed4e0505d..8dc0c315a3f133257084988f021d38f8d966d0a1 100644 (file)
--- a/src/lib/mysql/mysql_connection.cc
+++ b/src/lib/mysql/mysql_connection.cc
@@ -477,7 +477,8 @@ MySqlConnection::initializeSchema(const ParameterMap& parameters) {
     // Run.
     ProcessSpawn kea_admin(ProcessSpawn::SYNC, KEA_ADMIN_, kea_admin_parameters, vars,
                            /* inherit_env = */ true);
-    DB_LOG_INFO(MYSQL_INITIALIZE_SCHEMA).arg(kea_admin.getCommandLine());
+    DB_LOG_INFO(MYSQL_INITIALIZE_SCHEMA)
+        .arg(kea_admin.getCommandLine(std::unordered_set<std::string>{"--user", "--password"}));
     pid_t const pid(kea_admin.spawn());
     if (kea_admin.isRunning(pid)) {
         isc_throw(SchemaInitializationFailed, "kea-admin still running");

diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc
index 65284f12c34659aab43b42b4116c956fa63f8970..cf8d82410e0261ce6471935a4ca24e12685ace38 100644 (file)
--- a/src/lib/pgsql/pgsql_connection.cc
+++ b/src/lib/pgsql/pgsql_connection.cc
@@ -233,7 +233,8 @@ PgSqlConnection::initializeSchema(const ParameterMap& parameters) {
     // Run.
     ProcessSpawn kea_admin(ProcessSpawn::SYNC, KEA_ADMIN_, kea_admin_parameters, vars,
                            /* inherit_env = */ true);
-    DB_LOG_INFO(PGSQL_INITIALIZE_SCHEMA).arg(kea_admin.getCommandLine());
+    DB_LOG_INFO(PGSQL_INITIALIZE_SCHEMA)
+        .arg(kea_admin.getCommandLine(std::unordered_set<std::string>{"--user", "--password"}));
     pid_t const pid(kea_admin.spawn());
     if (kea_admin.isRunning(pid)) {
         isc_throw(SchemaInitializationFailed, "kea-admin still running");