Obscuring secrets in Log Process Details (#472)

* Obscuring secrets in SysbenchClientExecutor

* Obscuring full command.

* Updating version.

* obscuring process arguments in log process details.

* Moving SensitiveData.ObscureSecrets to LogProcessDetails in telemetry.

* Obscuring secrets in stack trace and error message.

* Fixes.

* Fixes.

* Adding a unit test for error logging.

* Fixing unit tests.

* Adding more to unit test.

---------

Co-authored-by: saibulusu <[email protected]>

Author: saibulusu

VERSION

@@ -1 +1 @@
-1.16.39
+1.16.39

src/VirtualClient/VirtualClient.Common/Telemetry/EventContextExtensions.cs

@@ -42,7 +42,7 @@ public static EventContext AddError(this EventContext context, Exception error,
                     errors.Add(new
                     {
                         errorType = currentError.GetType().FullName,
-                        errorMessage = currentError.Message
+                        errorMessage = SensitiveData.ObscureSecrets(currentError.Message)
                     });
 
                     currentError = currentError.InnerException;
@@ -65,11 +65,11 @@ public static EventContext AddError(this EventContext context, Exception error,
                 {
                     if (error.StackTrace.Length > maxCallStackLength)
                     {
-                        context.Properties[EventContextExtensions.ErrorCallstackProperty] = error.StackTrace.Substring(0, maxCallStackLength);
+                        context.Properties[EventContextExtensions.ErrorCallstackProperty] = SensitiveData.ObscureSecrets(error.StackTrace.Substring(0, maxCallStackLength));
                     }
                     else
                     {
-                        context.Properties[EventContextExtensions.ErrorCallstackProperty] = error.StackTrace;
+                        context.Properties[EventContextExtensions.ErrorCallstackProperty] = SensitiveData.ObscureSecrets(error.StackTrace);
                     }
                 }
             }

src/VirtualClient/VirtualClient.Contracts.UnitTests/VirtualClientLoggingExtensionsTests.cs

@@ -17,6 +17,8 @@ namespace VirtualClient.Contracts
     using Newtonsoft.Json;
     using Newtonsoft.Json.Linq;
     using NUnit.Framework;
+    using Polly;
+    using VirtualClient.Common;
     using VirtualClient.Common.Contracts;
     using VirtualClient.Common.Extensions;
     using VirtualClient.Common.Telemetry;
@@ -991,7 +993,7 @@ public async Task LogProcessDetailsAsyncExtensionEmitsTheExpectedProcessInformat
                 StandardOutput = expectedStandardOutput != null ? new Common.ConcurrentBuffer(new StringBuilder(expectedStandardOutput)) : null,
                 StandardError = expectedStandardError != null ? new Common.ConcurrentBuffer(new StringBuilder(expectedStandardError)) : null
             };
-
+            
             string expectedResults = "Any results output by the process.";
             bool expectedProcessDetailsCaptured = false;
             bool expectedProcessResultsCaptured = false;
@@ -1000,7 +1002,7 @@ public async Task LogProcessDetailsAsyncExtensionEmitsTheExpectedProcessInformat
             {
                 Assert.AreEqual(LogLevel.Information, level, $"Log level not matched");
                 Assert.IsInstanceOf<EventContext>(state);
-                
+
                 if (eventInfo.Name == $"{nameof(TestExecutor)}.ProcessDetails")
                 {
                     Assert.IsTrue((state as EventContext).Properties.TryGetValue("process", out object processContext));
@@ -1046,6 +1048,73 @@ public async Task LogProcessDetailsAsyncExtensionEmitsTheExpectedProcessInformat
             Assert.IsTrue(expectedProcessResultsCaptured);
         }
 
+        [Test]
+        [TestCase(0, "run password=secret123", null)]
+        [TestCase(1, "run password=secret123", "run password=secret123")]
+        public async Task LogProcessDetailsAsyncObscuresSecrets(int exitCode, string standardOutput, string standardError)
+        {
+            InMemoryProcess process = new InMemoryProcess
+            {
+                ExitCode = exitCode,
+                StartInfo = new ProcessStartInfo
+                {
+                    FileName = "run",
+                    Arguments = "password=secret123"
+                },
+                StandardOutput = new ConcurrentBuffer(new StringBuilder(standardOutput)),
+                StandardError = new ConcurrentBuffer(new StringBuilder(standardError))
+            };
+
+            this.mockFixture.Logger.OnLog = (level, eventInfo, state, error) =>
+            {
+                if (eventInfo.Name == $"{nameof(TestExecutor)}.ProcessDetails")
+                {
+                    (state as EventContext).Properties.TryGetValue("process", out object processContext);
+                    string actualProcessInfo = processContext.ToJson();
+
+                    Assert.IsFalse(actualProcessInfo.ToString().Contains("secret123"));
+                }
+            };
+
+            TestExecutor component = new TestExecutor(this.mockFixture);
+            await component.LogProcessDetailsAsync(process, new EventContext(Guid.NewGuid()), results: new List<string> { }, logToTelemetry: true)
+                .ConfigureAwait(false);
+        }
+
+        [Test]
+        public void LogErrorMessageObscuresSecrets()
+        {
+            Exception expectedError = null;
+            try
+            {
+                // To ensure a call stack is included.
+                throw new Exception("An error occurred, password=secret123");
+            }
+            catch (Exception exc)
+            {
+                expectedError = exc;
+            }
+
+            this.mockLogger.Object.LogErrorMessage(expectedError, this.mockEventContext);
+
+            this.mockLogger
+                .Setup(logger => logger.Log(LogLevel.Error, It.IsAny<EventId>(), It.IsAny<EventContext>(), null, null))
+                .Callback<LogLevel, EventId, EventContext, Exception, Func<EventContext, Exception, string>>((level, eventId, state, exc, formatter) =>
+                {
+                    Assert.IsNotNull(state);
+                    Assert.IsTrue(state.Properties.ContainsKey("error"));
+                    Assert.IsTrue(state.Properties.ContainsKey("errorCallstack"));
+
+                    List<object> errorEntries = state.Properties["error"] as List<object>;
+                    Assert.IsNotNull(errorEntries);
+                    Assert.IsTrue(errorEntries.Count == 1);
+
+                    Assert.IsFalse(JsonConvert.SerializeObject(errorEntries.First()).Contains("secret123"));
+                });
+
+            this.mockLogger.Object.LogErrorMessage(expectedError, this.mockEventContext);
+        }
+
         [Test]
         [TestCase(0, null, null, null, null, null)]
         [TestCase(0, "", "", "", "", "")]

src/VirtualClient/VirtualClient.Contracts/VirtualClientLoggingExtensions.cs

@@ -369,6 +369,8 @@ public static void LogErrorMessage(this ILogger logger, string errorMessage, Exc
             eventContext.ThrowIfNull(nameof(eventContext));
             errorMessage.ThrowIfNullOrWhiteSpace(nameof(errorMessage));
 
+            errorMessage = SensitiveData.ObscureSecrets(errorMessage);
+
             if (error != null)
             {
                 EventContext errorContext = eventContext.Clone();

src/VirtualClient/VirtualClient.Core/VirtualClientLoggingExtensions.cs

@@ -174,6 +174,11 @@ internal static void LogProcessDetails(this ILogger logger, ProcessDetails proce
 
             try
             {
+                // Obscure sensitive data in the command line
+                processDetails.CommandLine = SensitiveData.ObscureSecrets(processDetails.CommandLine);
+                processDetails.StandardOutput = SensitiveData.ObscureSecrets(processDetails.StandardOutput);
+                processDetails.StandardError = SensitiveData.ObscureSecrets(processDetails.StandardError);
+
                 // Examples:
                 // --------------
                 // GeekbenchExecutor.ProcessDetails
@@ -229,6 +234,11 @@ internal static void LogProcessDetails(this ILogger logger, IProcessProxy proces
 
             try
             {
+                // Obscure sensitive data in the command line
+                process.ProcessDetails.CommandLine = SensitiveData.ObscureSecrets(process.ProcessDetails.CommandLine);
+                process.ProcessDetails.StandardOutput = SensitiveData.ObscureSecrets(process.ProcessDetails.StandardOutput);
+                process.ProcessDetails.StandardError = SensitiveData.ObscureSecrets(process.ProcessDetails.StandardError);
+
                 // Examples:
                 // --------------
                 // GeekbenchExecutor.ProcessDetails