Configurable masking token, perf improvements, refactoring. #4586

michaelcfanning · 2023-12-27T21:44:11Z

This change:

Does some additional refactoring of the secret masking code (in support of a future move to consume the updated masker from a separate library). Mostly this changed entailed having LoggedSecretMasker derive from SecretMasker rather than wrapping an instance of this class.
Added a configurable preferred redaction token to the ReplacementPosition class (now simple renamed to Replacement). For secret values, we retain the existing default redaction string ***. For regexes that have no explicit rule information (an id and friendly name, together which comprise a moniker), we use the string +++. This change will allow users to easily search for logs looking for evidence of automatically redacted secrets. These secrets either should have been protected as secured variables OR there is an upstream leak of the secret in a script/dependency, etc.
For patterns that explicitly declare a rule moniker, the redaction token consists of the opaque id, friendly name and hash of the redacted secret. This data will be persisted to telemetry to allow secure correlation of redacted secrets with evidence of exposure in other locations.
I have added some preliminary telemetry in a naive way. Looking for feedback/refinements of this.
Renamed some items to communicate that they originate with the 1ES scan project, not CredScan.
Expanded unit testing. The updated secret masker namespace now has >90% code coverage.

michaelcfanning · 2023-12-27T21:58:50Z

src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs


 namespace Agent.Sdk.SecretMasking
 {
    /// <summary>
    /// Extended secret masker service, that allows to log origins of secrets
    /// </summary>
-    public class LoggedSecretMasker : ILoggedSecretMasker
+    public class LoggedSecretMasker : SecretMasker, ILoggedSecretMasker


SecretMasker

The logged secret masker (as opposed to the legacy logged secret masker) can extend SecretMasker rather than wrapping it (because no other ILoggedSecretMasker will be wrapped by this class). This change will help us switch over to using a SecretMasker instance that 1ES provides that lives in a different repo (https://github.com/microsoft/security-utilities).

michaelcfanning · 2023-12-28T20:44:25Z

src/Agent.Sdk/SecretMasking/ILoggedSecretMasker.cs

        void AddValue(String value, string origin);
        void AddValueEncoder(ValueEncoder encoder, string origin);
        void SetTrace(ITraceWriter trace);
+        IDictionary<string, string> GetTelemetry();


GetTelemetry

Added this on ILoggedSecretMasker because I thought it made most sense given that we will produce this data for both the legacy and current secret maskers. This interface currently holds all extensions to the original ADO masker interface.

michaelcfanning · 2023-12-28T20:45:15Z

src/Agent.Sdk/SecretMasking/ISecret.cs

@@ -8,5 +8,5 @@ internal interface ISecret
    /// <summary>
    /// Returns one item (start, length) for each match found in the input string.
    /// </summary>
-    IEnumerable<ReplacementPosition> GetPositions(String input);
+    IEnumerable<Replacement> GetReplacements(String input);


Replacement

ReplacementPositions now has additional metadata, for example, the preferred token to use for redaction, so I performed a rename.

michaelcfanning · 2023-12-28T20:54:01Z

public ISecretMasker Clone()

I can't find the use of secret masker clones in the code, does this utilization exist? One difference now between the legacy masker and the current is that the legacy masker doesn't retain elapsed masking time on clone. Is that an issue?

Refers to: src/Agent.Sdk/SecretMasking/LegacyLoggedSecretMasker.cs:134 in a95cc17. [](commit_id = a95cc17, deletion_comment = False)

michaelcfanning · 2023-12-28T20:55:09Z

src/Agent.Sdk/SecretMasking/LegacyLoggedSecretMasker.cs

    }

    Sdk.SecretMasking.ISecretMasker Sdk.SecretMasking.ISecretMasker.Clone()
    {
        return new LegacyLoggedSecretMasker(this._secretMasker.Clone());
    }
+
+    public IDictionary<string, string> GetTelemetry()


GetTelemetry

Note that we only produce the ElapsedMaskingTime data point for the legacy masker, as I think we are only really concerned about relative runtime perf differences between the maskers (i.e., and other telemetry isn't interesting/won't be consumed for the legacy masker).

michaelcfanning · 2023-12-28T21:07:18Z

src/Test/L0/SecretMaskerTests/SecretMaskerL0.cs

@@ -50,7 +50,7 @@ public void IsUserInfoMaskedCorrectly()
            var testSecretMasker = initSecretMasker();

            Assert.Equal(
-               "https://user:***@example.com",
+               "https://user:+++@example.com",


:+++@

here and elsewhere the use of +++ indicates that a value matched a regex and was therefore automatically redacted. we retain *** when redacting an explicitly specified value (e.g., a secured variable).

michaelcfanning · 2023-12-28T21:11:08Z

src/Agent.Sdk/SecretMasking/LegacyLoggedSecretMasker.cs

-        return this._secretMasker.MaskSecrets(input);
+        var stopwatch = Stopwatch.StartNew();
+        string result = this._secretMasker.MaskSecrets(input);
+        Interlocked.Exchange(ref elapsedMaskingTime, stopwatch.ElapsedTicks);


Exchange

I'm not sure what threading is in play in the agent. The masker is thread-safe for the masking operation, so I used Interlocked.Exchange to update the elapsed time property.

michaelcfanning · 2023-12-28T21:22:03Z

src/Agent.Sdk/SecretMasking/SecretMasker.cs

+        }
+
+        // Replace
+        var stringBuilder = new StringBuilder();


StringBuilder

StringBuilder instance construction is pretty expensive, we should consider a thread local field for a dedicated string builder instance (and set its length to 0 in this code path). A future change we can look at, maybe, to test our flighting/performance telemetry-based testing).

michaelcfanning · 2023-12-28T21:24:14Z

src/Agent.Sdk/SecretMasking/RegexSecret.cs

            {
-                startIndex = match.Index + 1;
-                yield return new ReplacementPosition(match.Index, match.Length);
+                if (input.IndexOf(sniffLiteral, StringComparison.Ordinal) != -1)


IndexOf

A simple IndexOf check is 15x+ faster than something like RE2 in a typical perf test. And so using it as a first pass filter to decide whether to run a follow-on regex is likely to yield performance gains. These are typically 4o% or more in other scan scenarios 1ES owns that use the technique.

…for deleted agent knob.

michaelcfanning · 2023-12-28T23:47:54Z

@merlynomsft

…scanner).

michaelcfanning · 2024-01-02T16:20:19Z

src/Agent.Worker/JobRunner.cs

@@ -347,6 +348,9 @@ public async Task<TaskResult> RunAsync(Pipelines.AgentJobRequestMessage message,
                finally
                {
                    Trace.Info("Finalize job.");
+
+                    PublishSecretsMaskingTelemetry(jobContext);


PublishSecretsMaskingTelemetry

Is this a good place to put the secret masking telemetry? Is this when the effective lifetime of the masker is over? What are the lifetime semantics of the host context relative to this code?

Looks good to me, considering we upload logs just after (UploadDiagnosticLogsAsync)

michaelcfanning added 4 commits November 10, 2023 09:04

Add ability to configure the masking token.

ec1a87d

Masking update.

de5824a

Ongoing refactoring. Allow alternate masking token.

8de1ab2

Merge from master.

d4b65df

michaelcfanning changed the title ~~Configurable masking token~~ Configurable masking token, perf improvements, refactoring. Dec 27, 2023

michaelcfanning commented Dec 27, 2023

View reviewed changes

Significantly improve test coverage.

a95cc17

michaelcfanning added enhancement Area: Agent labels Dec 28, 2023

michaelcfanning commented Dec 28, 2023

View reviewed changes

Refinements based on PR review.

a24381f

michaelcfanning commented Dec 28, 2023

View reviewed changes

Add monikers to rule patterns. Remove unused environment var setting …

ba5aa24

…for deleted agent knob.

michaelcfanning marked this pull request as ready for review December 28, 2023 23:47

michaelcfanning requested review from a team as code owners December 28, 2023 23:47

michaelcfanning added 3 commits December 28, 2023 16:18

Add full telemetry to tests (which previously did not invoke the new …

8a772aa

…scanner).

Fix test typos.

f9e3a63

Fix two broken tests.

d911e20

michaelcfanning commented Jan 2, 2024

View reviewed changes

Remove erroneous line of code.

1a87083

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable masking token, perf improvements, refactoring. #4586

Configurable masking token, perf improvements, refactoring. #4586

michaelcfanning commented Dec 27, 2023 •

edited

Loading

michaelcfanning Dec 27, 2023

michaelcfanning Dec 28, 2023

michaelcfanning Dec 28, 2023

michaelcfanning commented Dec 28, 2023

michaelcfanning Dec 28, 2023

michaelcfanning Dec 28, 2023

michaelcfanning Dec 28, 2023

michaelcfanning Dec 28, 2023

michaelcfanning Dec 28, 2023

michaelcfanning commented Dec 28, 2023

michaelcfanning Jan 2, 2024

merlynomsft Jan 11, 2024

Configurable masking token, perf improvements, refactoring. #4586

Are you sure you want to change the base?

Configurable masking token, perf improvements, refactoring. #4586

Conversation

michaelcfanning commented Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Dec 28, 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

michaelcfanning commented Dec 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Dec 27, 2023 •

edited

Loading