From 3aac4dea6796ca6142dd9c368f666b5d368307ec Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 13:52:10 +0100 Subject: [PATCH 1/7] [partition] Remove logging-of-a-pointer during device detection --- src/modules/partition/core/PartitionCoreModule.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/modules/partition/core/PartitionCoreModule.cpp b/src/modules/partition/core/PartitionCoreModule.cpp index 69a6db535..109e5182e 100644 --- a/src/modules/partition/core/PartitionCoreModule.cpp +++ b/src/modules/partition/core/PartitionCoreModule.cpp @@ -257,7 +257,6 @@ PartitionCoreModule::doInit() cDebug() << Logger::SubEntry << "node\tcapacity\tname\tprettyName"; for ( auto device : devices ) { - cDebug() << Logger::SubEntry << Logger::Pointer( device ); if ( device ) { // Gives ownership of the Device* to the DeviceInfo object From 4db4e983e3cf6049279538d8b2b47db1382bd297 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 14:04:00 +0100 Subject: [PATCH 2/7] [partition] Don't format tables of attributes in source --- .../partition/jobs/FillGlobalStorageJob.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/modules/partition/jobs/FillGlobalStorageJob.cpp b/src/modules/partition/jobs/FillGlobalStorageJob.cpp index 40e67d620..3af70ed05 100644 --- a/src/modules/partition/jobs/FillGlobalStorageJob.cpp +++ b/src/modules/partition/jobs/FillGlobalStorageJob.cpp @@ -105,13 +105,18 @@ mapForPartition( Partition* partition, const QString& uuid ) // so indent a bit Logger::CDebug deb; using TR = Logger::DebugRow< const char* const, const QString& >; + // clang-format off deb << Logger::SubEntry << "mapping for" << partition->partitionPath() << partition->deviceNode() - << TR( "partlabel", map[ "partlabel" ].toString() ) << TR( "partuuid", map[ "partuuid" ].toString() ) - << TR( "parttype", map[ "parttype" ].toString() ) << TR( "partattrs", map[ "partattrs" ].toString() ) - << TR( "mountPoint:", PartitionInfo::mountPoint( partition ) ) << TR( "fs:", map[ "fs" ].toString() ) - << TR( "fsName", map[ "fsName" ].toString() ) << TR( "uuid", uuid ) + << TR( "partlabel", map[ "partlabel" ].toString() ) + << TR( "partuuid", map[ "partuuid" ].toString() ) + << TR( "parttype", map[ "parttype" ].toString() ) + << TR( "partattrs", map[ "partattrs" ].toString() ) + << TR( "mountPoint:", PartitionInfo::mountPoint( partition ) ) + << TR( "fs:", map[ "fs" ].toString() ) + << TR( "fsName", map[ "fsName" ].toString() ) + << TR( "uuid", uuid ) << TR( "claimed", map[ "claimed" ].toString() ); - + // clang-format on if ( partition->roles().has( PartitionRole::Luks ) ) { const FileSystem& fsRef = partition->fileSystem(); From 7cc84b89be25c1a4c112666f9d3c9ebe9c66464d Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 14:15:00 +0100 Subject: [PATCH 3/7] [partition] Clarify the meaning of the various UUIDs in debug-output --- src/modules/partition/jobs/FillGlobalStorageJob.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/partition/jobs/FillGlobalStorageJob.cpp b/src/modules/partition/jobs/FillGlobalStorageJob.cpp index 3af70ed05..d39e7d858 100644 --- a/src/modules/partition/jobs/FillGlobalStorageJob.cpp +++ b/src/modules/partition/jobs/FillGlobalStorageJob.cpp @@ -108,13 +108,13 @@ mapForPartition( Partition* partition, const QString& uuid ) // clang-format off deb << Logger::SubEntry << "mapping for" << partition->partitionPath() << partition->deviceNode() << TR( "partlabel", map[ "partlabel" ].toString() ) - << TR( "partuuid", map[ "partuuid" ].toString() ) + << TR( "partition-uuid (partuuid)", map[ "partuuid" ].toString() ) << TR( "parttype", map[ "parttype" ].toString() ) << TR( "partattrs", map[ "partattrs" ].toString() ) << TR( "mountPoint:", PartitionInfo::mountPoint( partition ) ) << TR( "fs:", map[ "fs" ].toString() ) << TR( "fsName", map[ "fsName" ].toString() ) - << TR( "uuid", uuid ) + << TR( "filesystem-uuid (uuid)", uuid ) << TR( "claimed", map[ "claimed" ].toString() ); // clang-format on if ( partition->roles().has( PartitionRole::Luks ) ) From 7b3c4db8f00495bc58a173a11f67329ea42d6ec0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 14:21:46 +0100 Subject: [PATCH 4/7] [libcalamares] Redacted -> RedactedCommand - For logging (shell) commands where a password might become visible, use RedactedCommand. Rename it to allow for other kinds of redaction, too. --- src/libcalamares/utils/Logger.cpp | 2 +- src/libcalamares/utils/Logger.h | 6 +++--- src/libcalamares/utils/Runner.cpp | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libcalamares/utils/Logger.cpp b/src/libcalamares/utils/Logger.cpp index 79ae873db..01c3e1539 100644 --- a/src/libcalamares/utils/Logger.cpp +++ b/src/libcalamares/utils/Logger.cpp @@ -229,7 +229,7 @@ toString( const QVariant& v ) } QDebug& -operator<<( QDebug& s, const Redacted& l ) +operator<<( QDebug& s, const RedactedCommand& l ) { // Special case logging: don't log the (encrypted) password. if ( l.list.contains( "usermod" ) ) diff --git a/src/libcalamares/utils/Logger.h b/src/libcalamares/utils/Logger.h index bf6b99d00..9f0aeffea 100644 --- a/src/libcalamares/utils/Logger.h +++ b/src/libcalamares/utils/Logger.h @@ -214,9 +214,9 @@ public: * since the log may get posted to bug reports, or stored in * the target system. */ -struct Redacted +struct RedactedCommand { - Redacted( const QStringList& l ) + RedactedCommand( const QStringList& l ) : list( l ) { } @@ -224,7 +224,7 @@ struct Redacted const QStringList& list; }; -QDebug& operator<<( QDebug& s, const Redacted& l ); +QDebug& operator<<( QDebug& s, const RedactedCommand& l ); /** * @brief Formatted logging of a pointer diff --git a/src/libcalamares/utils/Runner.cpp b/src/libcalamares/utils/Runner.cpp index e138b1c68..c7146c2d7 100644 --- a/src/libcalamares/utils/Runner.cpp +++ b/src/libcalamares/utils/Runner.cpp @@ -163,7 +163,7 @@ Calamares::Utils::Runner::run() } ); } - cDebug() << Logger::SubEntry << "Running" << Logger::Redacted( m_command ); + cDebug() << Logger::SubEntry << "Running" << Logger::RedactedCommand( m_command ); process.start(); if ( !process.waitForStarted() ) { @@ -225,13 +225,13 @@ Calamares::Utils::Runner::run() { if ( !output.isEmpty() ) { - cDebug() << Logger::SubEntry << "Target cmd:" << Logger::Redacted( m_command ) << "Exit code:" << r + cDebug() << Logger::SubEntry << "Target cmd:" << Logger::RedactedCommand( m_command ) << "Exit code:" << r << "output:\n" << Logger::NoQuote << output; } else { - cDebug() << Logger::SubEntry << "Target cmd:" << Logger::Redacted( m_command ) << "Exit code:" << r + cDebug() << Logger::SubEntry << "Target cmd:" << Logger::RedactedCommand( m_command ) << "Exit code:" << r << "(no output)"; } } From 152b3c333be53cd99a3b19214bc9bf4e45b75706 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 14:47:13 +0100 Subject: [PATCH 5/7] [libcalamares] Introduce redaction-of-names class for logging - redacted names are stable inside of one run of Calamares - random, private displays of a given string for a context SEE #1593 --- src/libcalamares/utils/Logger.cpp | 32 +++++++++++++++++++++++++++++++ src/libcalamares/utils/Logger.h | 17 ++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/libcalamares/utils/Logger.cpp b/src/libcalamares/utils/Logger.cpp index 01c3e1539..2b952cc56 100644 --- a/src/libcalamares/utils/Logger.cpp +++ b/src/libcalamares/utils/Logger.cpp @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include #include @@ -252,4 +254,34 @@ operator<<( QDebug& s, const RedactedCommand& l ) return s; } +/** @brief Returns a stable-but-private hash of @p context and @p s + * + * Identical strings with the same context will be hashed the same, + * so that they can be logged and still recognized as the-same. + */ +static uint insertRedactedName( const QString& context, const QString& s ) +{ + static uint salt = QRandomGenerator::global()->generate(); // Just once + + uint val = qHash(context, salt); + return qHash(s, val); +} + +RedactedName::RedactedName( const QString& context, const QString& s ) + : m_id( insertRedactedName(context, s) ), + m_context(context) +{ +} + +RedactedName::RedactedName(const char *context, const QString& s ) + : RedactedName( QString::fromLatin1(context), s ) +{ +} + +QDebug& +operator<< ( QDebug& s, const RedactedName& n ) +{ + return s << NoQuote << n.m_context << '$' << n.m_id << Quote; +} + } // namespace Logger diff --git a/src/libcalamares/utils/Logger.h b/src/libcalamares/utils/Logger.h index 9f0aeffea..70b29b98e 100644 --- a/src/libcalamares/utils/Logger.h +++ b/src/libcalamares/utils/Logger.h @@ -226,6 +226,23 @@ struct RedactedCommand QDebug& operator<<( QDebug& s, const RedactedCommand& l ); +/** @brief When logging "private" identifiers, keep them consistent but private + * + * Send a string to a logger in such a way that each time it is logged, + * it logs the same way, but without revealing the actual contents. + * This can be applied to user names, UUIDs, etc. + */ +struct RedactedName +{ + RedactedName( const char* context, const QString& s ); + RedactedName( const QString& context, const QString& s ); + + const uint m_id; + const QString m_context; +}; + +QDebug& operator<<( QDebug& s, const RedactedName& n ); + /** * @brief Formatted logging of a pointer * From 5a4e2b73ab52cfb34f9168bc3a7c0dcede4b8334 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 14:54:09 +0100 Subject: [PATCH 6/7] [libcalamares][partition] Give RedactedName a convert-to-QString - use hex-trailer - while here, convert DebugRow to use a copy rather than a reference, to avoid dangling references when applied to temporaries - convert *partition* module to use the RedactedNames --- src/libcalamares/utils/Logger.cpp | 7 +++---- src/libcalamares/utils/Logger.h | 12 +++++++++--- src/modules/partition/core/PartitionCoreModule.cpp | 7 +++++-- src/modules/partition/jobs/FillGlobalStorageJob.cpp | 6 +++--- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/libcalamares/utils/Logger.cpp b/src/libcalamares/utils/Logger.cpp index 2b952cc56..d35d6891b 100644 --- a/src/libcalamares/utils/Logger.cpp +++ b/src/libcalamares/utils/Logger.cpp @@ -274,14 +274,13 @@ RedactedName::RedactedName( const QString& context, const QString& s ) } RedactedName::RedactedName(const char *context, const QString& s ) - : RedactedName( QString::fromLatin1(context), s ) + : RedactedName( QString::fromLatin1( context ), s ) { } -QDebug& -operator<< ( QDebug& s, const RedactedName& n ) +RedactedName::operator QString() const { - return s << NoQuote << n.m_context << '$' << n.m_id << Quote; + return QString( m_context + '$' + QString::number( m_id, 16 ) ); } } // namespace Logger diff --git a/src/libcalamares/utils/Logger.h b/src/libcalamares/utils/Logger.h index 70b29b98e..0d7d5c870 100644 --- a/src/libcalamares/utils/Logger.h +++ b/src/libcalamares/utils/Logger.h @@ -145,8 +145,8 @@ public: { } - const T& first; - const U& second; + const T first; + const U second; }; /** @@ -237,11 +237,17 @@ struct RedactedName RedactedName( const char* context, const QString& s ); RedactedName( const QString& context, const QString& s ); + operator QString() const; + +private: const uint m_id; const QString m_context; }; -QDebug& operator<<( QDebug& s, const RedactedName& n ); +inline QDebug& operator<<( QDebug& s, const RedactedName& n ) +{ + return s << NoQuote << QString( n ) << Quote; +} /** * @brief Formatted logging of a pointer diff --git a/src/modules/partition/core/PartitionCoreModule.cpp b/src/modules/partition/core/PartitionCoreModule.cpp index 109e5182e..2475937d2 100644 --- a/src/modules/partition/core/PartitionCoreModule.cpp +++ b/src/modules/partition/core/PartitionCoreModule.cpp @@ -262,8 +262,11 @@ PartitionCoreModule::doInit() // Gives ownership of the Device* to the DeviceInfo object auto deviceInfo = new DeviceInfo( device ); m_deviceInfos << deviceInfo; - cDebug() << Logger::SubEntry << device->deviceNode() << device->capacity() << device->name() - << device->prettyName(); + cDebug() << Logger::SubEntry + << device->deviceNode() + << device->capacity() + << Logger::RedactedName( "DevName", device->name() ) + << Logger::RedactedName( "DevNamePretty", device->prettyName() ); } else { diff --git a/src/modules/partition/jobs/FillGlobalStorageJob.cpp b/src/modules/partition/jobs/FillGlobalStorageJob.cpp index d39e7d858..5be353113 100644 --- a/src/modules/partition/jobs/FillGlobalStorageJob.cpp +++ b/src/modules/partition/jobs/FillGlobalStorageJob.cpp @@ -104,17 +104,17 @@ mapForPartition( Partition* partition, const QString& uuid ) // Debugging for inside the loop in createPartitionList(), // so indent a bit Logger::CDebug deb; - using TR = Logger::DebugRow< const char* const, const QString& >; + using TR = Logger::DebugRow< const char* const, const QString >; // clang-format off deb << Logger::SubEntry << "mapping for" << partition->partitionPath() << partition->deviceNode() << TR( "partlabel", map[ "partlabel" ].toString() ) - << TR( "partition-uuid (partuuid)", map[ "partuuid" ].toString() ) + << TR( "partition-uuid (partuuid)", Logger::RedactedName( "PartUUID", map[ "partuuid" ].toString() ) ) << TR( "parttype", map[ "parttype" ].toString() ) << TR( "partattrs", map[ "partattrs" ].toString() ) << TR( "mountPoint:", PartitionInfo::mountPoint( partition ) ) << TR( "fs:", map[ "fs" ].toString() ) << TR( "fsName", map[ "fsName" ].toString() ) - << TR( "filesystem-uuid (uuid)", uuid ) + << TR( "filesystem-uuid (uuid)", Logger::RedactedName( "FSUUID", uuid ) ) << TR( "claimed", map[ "claimed" ].toString() ); // clang-format on if ( partition->roles().has( PartitionRole::Luks ) ) From efe84bc6c0f1ee8f81a10617a5d55dd43f87422e Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Tue, 16 Nov 2021 15:31:35 +0100 Subject: [PATCH 7/7] [partition] Don't log private names - log device node (/dev/sdb) instead of its name - don't log job's prettyName() because that's translated, and also contains user-visible private names (introducing a non-translated, nicely redacted version of prettyName() seems like too much effort for something that can be reconstructed from bits earlier in the log) --- src/modules/partition/core/PartitionCoreModule.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/partition/core/PartitionCoreModule.cpp b/src/modules/partition/core/PartitionCoreModule.cpp index 2475937d2..16e5a7ea1 100644 --- a/src/modules/partition/core/PartitionCoreModule.cpp +++ b/src/modules/partition/core/PartitionCoreModule.cpp @@ -709,10 +709,10 @@ PartitionCoreModule::dumpQueue() const cDebug() << "# Queue:"; for ( auto info : m_deviceInfos ) { - cDebug() << Logger::SubEntry << "## Device:" << info->device->name(); + cDebug() << Logger::SubEntry << "## Device:" << info->device->deviceNode(); for ( const auto& job : info->jobs() ) { - cDebug() << Logger::SubEntry << "-" << job->prettyName(); + cDebug() << Logger::SubEntry << "-" << job->metaObject()->className(); } } }