From: Wlodek Wencel Date: Thu, 23 Jul 2026 11:43:08 +0000 (+0200) Subject: [#4643] Redact password in parse() error logs X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=33daa8b9d0d614bd1b7ee483b7bedda64eec7664;p=thirdparty%2Fkea.git [#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 --- 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 index 0000000000..9e4307edc5 --- /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 d0d87ea7b4..2e01786ca2 100644 --- 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 5b103526de..45703954c7 100644 --- 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 aac6cce436..5e46d5fb5d 100644 --- a/src/lib/database/tests/database_connection_unittest.cc +++ b/src/lib/database/tests/database_connection_unittest.cc @@ -11,12 +11,14 @@ #include #include #include +#include #include #include 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