[#4643] Redact password in parse() error logs

Log redactedAccessString() on DatabaseConnection::parse()
failure so cleartext passwords are not written to ERROR logs.

Closes #4643

Co-authored-by: Cursor <cursoragent@cursor.com>

diff --git a/changelog_unreleased/4643-parse-logs-cleartext-database-access-string-on-failure b/changelog_unreleased/4643-parse-logs-cleartext-database-access-string-on-failure
new file mode 100644 (file)
index 0000000..9e4307e
--- /dev/null
+++ b/changelog_unreleased/4643-parse-logs-cleartext-database-access-string-on-failure
@@ -0,0 +1,5 @@
+[bug]          wlodek
+       DatabaseConnection::parse() now logs a redacted
+       access string on failure, avoiding cleartext
+       passwords in ERROR logs.
+       (Gitlab #4643)

diff --git a/src/lib/database/database_connection.cc b/src/lib/database/database_connection.cc
index d0d87ea7b4e00f02d6fd030d4fb244e6764ecace..2e01786ca2a22a309277926fcaa0e7194fec64a5 100644 (file)
--- a/src/lib/database/database_connection.cc
+++ b/src/lib/database/database_connection.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2025 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2026 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -61,9 +61,11 @@ DatabaseConnection::parse(const std::string& dbaccess) {
                 // at the position of ending apostrophe.
                 auto password = dba.substr(password_pos + password_prefix.length(),
                                            password_end_pos - password_pos - password_prefix.length());
+                // Store the password before credential checks so a failure can still
+                // be logged via redactedAccessString() without exposing the secret.
+                mapped_tokens.insert(make_pair("password", password));
                 // Refuse default passwords.
                 DefaultCredentials::check(password);
-                mapped_tokens.insert(make_pair("password", password));
 
                 // We need to erase the password from the access string because the generic
                 // algorithm parsing other parameters requires that there are no whitespaces
@@ -95,8 +97,9 @@ DatabaseConnection::parse(const std::string& dbaccess) {
                 }
             }
         } catch (const std::exception& ex) {
-            // We'd obscure the password if we could parse the access string.
-            DB_LOG_ERROR(DB_INVALID_ACCESS).arg(dbaccess);
+            // Never log the raw access string: it may contain a cleartext password.
+            // redactedAccessString() replaces any parsed password with asterisks.
+            DB_LOG_ERROR(DB_INVALID_ACCESS).arg(redactedAccessString(mapped_tokens));
             throw;
         }
     }

diff --git a/src/lib/database/db_messages.mes b/src/lib/database/db_messages.mes
index 5b103526ded914b24a3def28e7186479c811360a..45703954c72d063074118e83f3a4ed3b12c51138 100644 (file)
--- a/src/lib/database/db_messages.mes
+++ b/src/lib/database/db_messages.mes
@@ -12,9 +12,9 @@ be used in config-database[s].
 
 % DATABASE_INVALID_ACCESS invalid database access string: %1
 This is logged when an attempt has been made to parse a database access string
-and the attempt ended in error.  The access string in question - which
-should be of the form 'keyword=value keyword=value...' is included in
-the message.
+and the attempt ended in error.  A redacted form of the access string - which
+should be of the form 'keyword=value keyword=value...' with any password
+replaced by asterisks - is included in the message.
 
 % DATABASE_MYSQL_COMMIT committing to MySQL database
 The code has issued a commit call.  All outstanding transactions will be

diff --git a/src/lib/database/tests/database_connection_unittest.cc b/src/lib/database/tests/database_connection_unittest.cc
index aac6cce436b3de7cefcb932a758f9a2e6ba30484..5e46d5fb5dcdb34b954a24a6fc249acd43485ec5 100644 (file)
--- a/src/lib/database/tests/database_connection_unittest.cc
+++ b/src/lib/database/tests/database_connection_unittest.cc
@@ -11,12 +11,14 @@
 #include <database/database_connection.h>
 #include <database/dbaccess_parser.h>
 #include <exceptions/exceptions.h>
+#include <testutils/log_utils.h>
 #include <gtest/gtest.h>
 
 #include <functional>
 
 using namespace isc::data;
 using namespace isc::db;
+using namespace isc::dhcp::test;
 using namespace isc::util;
 namespace ph = std::placeholders;
 
@@ -433,6 +435,35 @@ TEST(DatabaseConnectionTest, parseQuotedDefaultPassword) {
     EXPECT_THROW(DatabaseConnection::parse(bad), DefaultCredential);
 }
 
+/// @brief Test fixture capturing logs emitted by DatabaseConnection::parse.
+class DatabaseConnectionParseLogTest : public LogContentTest {
+};
+
+// Verifies that parse() failure for a quoted default password logs a redacted
+// access string and never writes the cleartext password.
+TEST_F(DatabaseConnectionParseLogTest, parseErrorDoesNotLogCleartextPassword) {
+    std::string bad = "user=me password='1234' name=kea type=mysql";
+    EXPECT_THROW(DatabaseConnection::parse(bad), DefaultCredential);
+
+    EXPECT_EQ(1U, countFile("DATABASE_INVALID_ACCESS"));
+    EXPECT_EQ(1U, countFile("password=*****"));
+    EXPECT_EQ(0U, countFile("password='1234'"));
+    EXPECT_EQ(0U, countFile("password=1234"));
+    EXPECT_EQ(0U, countFile("'1234'"));
+}
+
+// Verifies that a mid-parse failure after an unquoted password was accepted
+// still redacts the password in the error log.
+TEST_F(DatabaseConnectionParseLogTest, parseInvalidTokenDoesNotLogCleartextPassword) {
+    std::string bad = "user=me password=s3cret name=kea badtoken";
+    EXPECT_THROW(DatabaseConnection::parse(bad), isc::InvalidParameter);
+
+    EXPECT_EQ(1U, countFile("DATABASE_INVALID_ACCESS"));
+    EXPECT_EQ(1U, countFile("password=*****"));
+    EXPECT_EQ(0U, countFile("password=s3cret"));
+    EXPECT_EQ(0U, countFile("s3cret"));
+}
+
 /// @brief redactedAccessString test
 ///
 /// Checks that the redacted configuration string includes the password only