Improve sensitive data detection when logging records #4658

xakep139 · 2023-11-02T17:58:23Z

This PR fixes an issue when our [LoggerMessage] source-gen was allowing this syntax without any warnings or errors:

record MyType
{
    [PrivateData]
    public required string Password { get; init; }
}

[LoggerMessage(LogLevel.Debug, "Command is {command}")]
static void LogCommand(ILogger logger, MyType command);

Now we'll emit an error if this happens.
Additionally, this PR fixes usages of [LogProperties] when it's applied on an argument of record type that has implicitly declared property via primary constructor and this property is annotated with a data classification attribute, e.g.:

record RecordWithSensitiveMembers(
    [PrivateData] string ArgFromPrimaryCtor); // <=== we need to redact the corresponding property

[LoggerMessage(LogLevel.Information)]
static partial void LogRecordMethod(
    ILogger logger,
    IRedactorProvider redactorProvider,
    [LogProperties] RecordWithSensitiveMembers data);

Ideally, this should be annotated as [property: PrivateData] string ArgFromPrimaryCtor, but usually folks don't do that (and some might not even know about that syntax).
Now we'll cover such cases and redact RecordWithSensitiveMembers.ArgFromPrimaryCtor in generated code.

Microsoft Reviewers: Open in CodeFlow

…ection

xakep139 · 2023-11-03T15:06:05Z

.../Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs

        {
-            if (i.Name == ResourceUtilizationInstruments.CpuUtilization
-            || i.Name == ResourceUtilizationInstruments.MemoryUtilization)
+            if (!ReferenceEquals(instrument.Meter.Scope, meterScope))


This fixes a flaky test: https://dev.azure.com/dnceng-public/public/_build/results?buildId=458782&view=ms.vss-test-web.build-test-results-tab&runId=10343638&resultId=100183&paneView=debug

xakep139 · 2023-11-03T15:06:32Z

...es/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Windows/WindowsCountersTests.cs

@@ -40,7 +40,10 @@ public void WindowsCounters_Registers_Instruments()
        {
            InstrumentPublished = (instrument, listener) =>
            {
-                listener.EnableMeasurementEvents(instrument);
+                if (ReferenceEquals(meter, instrument.Meter))


This also fixes a flaky test: https://dev.azure.com/dnceng-public/public/_build/results?buildId=458782&view=ms.vss-test-web.build-test-results-tab&runId=10343638&resultId=100183&paneView=debug

xakep139 · 2023-11-03T15:12:18Z

src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs

@@ -231,4 +231,10 @@ internal sealed class DiagDescriptors : DiagDescriptorsBase
        title: Resources.InvalidAttributeUsageTitle,
        messageFormat: Resources.InvalidAttributeUsageMessage,
        category: Category);
+
+    public static DiagnosticDescriptor RecordTypeSensitiveArgumentIsInTemplate { get; } = Make(


@geeknoid @davidfowl should this be an error or rather warning?

src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx

dotnet-comment-bot · 2023-11-03T15:28:53Z

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project	Expected	Actual
Microsoft.AspNetCore.Diagnostics.Middleware	99	100
Microsoft.Extensions.Telemetry	92	93

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=459450&view=codecoverage-tab

geeknoid · 2023-11-03T16:36:32Z

test/Generators/Microsoft.Gen.Logging/TestClasses/SensitiveRecordExtensions.cs

+
+        public record class BaseRecord
+        {
+            [PrivateData] public const string ConstFieldBase = Sensitive;


What does it mean to apply this attribute to a const? Seems like we might need an analyzer to flag this as not desirable?

I agree it doesn't make sense, but I wanted to list all possible combinations (even if they don't make sense) to ensure that record's default ToString() implementation doesn't emit these values. I will file an issue for the analyzer shortly.

Created #4669

geeknoid · 2023-11-03T16:41:53Z

src/Generators/Microsoft.Gen.Logging/Parsing/Parser.LogProperties.cs

@@ -20,12 +20,13 @@ internal partial class Parser
 {
    private static readonly HashSet<TypeKind> _allowedTypeKinds = [TypeKind.Class, TypeKind.Struct, TypeKind.Interface];

-    private bool ProcessLogPropertiesForParameter(
+    internal bool ProcessLogPropertiesForParameter(


Is this internal for testing?

geeknoid · 2023-11-03T16:55:07Z

src/Generators/Microsoft.Gen.Logging/Parsing/Parser.Records.cs

+
+namespace Microsoft.Gen.Logging.Parsing
+{
+    internal partial class Parser


Is this internal for testing? Otherwise it can be private.

Parser class was internal even before my changes

Co-authored-by: Darius Letterman <dcletterman@gmail.com>

docs/list-of-diagnostics.md

xakep139 added 2 commits October 20, 2023 15:08

Added a test to showcase the bug

a822090

added another test case to be fixed

7b17ca9

xakep139 added the area-telemetry label Nov 2, 2023

ghost assigned xakep139 Nov 2, 2023

xakep139 added 3 commits November 2, 2023 19:20

Merge branch 'release/8.0' into xakep139/sensitive-record-logging-det…

10cb975

…ection

add tests

6936821

complete code porting

4836f88

xakep139 marked this pull request as ready for review November 3, 2023 14:47

xakep139 requested review from geeknoid, tekian, dariusclay, omsoftdev and iliar-turdushev November 3, 2023 14:47

fixes

10aaa0f

xakep139 commented Nov 3, 2023

View reviewed changes

dariusclay approved these changes Nov 3, 2023

View reviewed changes

src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx Outdated Show resolved Hide resolved

tekian approved these changes Nov 3, 2023

View reviewed changes

geeknoid reviewed Nov 3, 2023

View reviewed changes

xakep139 and others added 2 commits November 4, 2023 16:30

more test cases

d0f04f9

Update src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx

3ab69a9

Co-authored-by: Darius Letterman <dcletterman@gmail.com>

geeknoid approved these changes Nov 4, 2023

View reviewed changes

geeknoid merged commit f8ddd93 into release/8.0 Nov 4, 2023
6 checks passed

geeknoid deleted the xakep139/sensitive-record-logging-detection branch November 4, 2023 17:52

joperezr mentioned this pull request Nov 6, 2023

Sensitive records aren't redacted when used in IEnumerable<T> in a logging method #4657

Closed

RussKie reviewed Nov 9, 2023

View reviewed changes

docs/list-of-diagnostics.md Show resolved Hide resolved

github-actions bot locked and limited conversation to collaborators Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve sensitive data detection when logging records #4658

Improve sensitive data detection when logging records #4658

xakep139 commented Nov 2, 2023 •

edited

Loading

xakep139 Nov 3, 2023

xakep139 Nov 3, 2023

xakep139 Nov 3, 2023 •

edited

Loading

dotnet-comment-bot commented Nov 3, 2023

geeknoid Nov 3, 2023

xakep139 Nov 4, 2023

xakep139 Nov 4, 2023

geeknoid Nov 3, 2023

xakep139 Nov 4, 2023

geeknoid Nov 3, 2023

xakep139 Nov 4, 2023

Improve sensitive data detection when logging records #4658

Improve sensitive data detection when logging records #4658

Conversation

xakep139 commented Nov 2, 2023 • edited Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xakep139 Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

dotnet-comment-bot commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xakep139 commented Nov 2, 2023 •

edited

Loading

xakep139 Nov 3, 2023 •

edited

Loading