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

            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

        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

    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 <[email protected]>

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 dariusclay, geeknoid, iliar-turdushev, omsoftdev and tekian 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 <[email protected]>

geeknoid approved these changes Nov 4, 2023

View reviewed changes

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

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

Improve sensitive data detection when logging records #4658

Improve sensitive data detection when logging records #4658

Uh oh!

Conversation

xakep139 commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xakep139 Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dotnet-comment-bot commented Nov 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

xakep139 commented Nov 2, 2023 •

edited

Loading

xakep139 Nov 3, 2023 •

edited

Loading