Improve NTLM Collection Logging BED-7374

RPCTest/RPCTest.csproj

@@ -0,0 +1,23 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+    <PropertyGroup>
+        <TargetFramework>net8.0</TargetFramework>
+        <LangVersion>latest</LangVersion>
+        <ImplicitUsings>enable</ImplicitUsings>
+        <Nullable>enable</Nullable>
+    </PropertyGroup>
+
+    <ItemGroup>
+        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.7.1" />
+        <PackageReference Include="xunit" Version="2.4.1" />
+        <PackageReference Include="xunit.runner.visualstudio" Version="2.4.1">
+            <PrivateAssets>all</PrivateAssets>
+            <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+        </PackageReference>
+    </ItemGroup>
+
+    <ItemGroup>
+        <ProjectReference Include="..\src\SharpHoundRPC\SharpHoundRPC.csproj" />
+    </ItemGroup>
+
+</Project>

RPCTest/Registry/StrategyExecutorTests.cs

@@ -0,0 +1,130 @@
+using SharpHoundRPC.Registry;
+using Xunit;
+
+namespace RPCTest.Registry;
+
+public class StrategyExecutorTests {
+    private readonly StrategyExecutor _strategyExecutor = new();
+
+    [Fact]
+    public async Task CollectAsync_NoStrategies_ReturnsFailure_WithNoAttempts() {
+        // Act
+        var result = await _strategyExecutor.CollectAsync<RegistryQueryResult, RegistryQuery>("target machine", [], []);
+
+        // Assert
+        Assert.NotNull(result);
+        Assert.False(result.WasSuccessful);
+        Assert.Empty(result.FailureAttempts!);
+        Assert.Null(result.Results);
+        Assert.Null(result.SuccessfulStrategy);
+    }
+
+    [Fact]
+    public async Task CollectAsync_CanExecuteIsFalse_ReturnsFailure_WithAttempt() {
+        //Arrange
+        var strategy = new FakeCollectionStrategy(false, "well now I am not doing it");
+
+        // Act
+        var result = await _strategyExecutor.CollectAsync("target machine", [], [strategy]);
+
+        // Assert
+        Assert.False(result.WasSuccessful);
+        Assert.Null(result.Results);
+        Assert.Null(result.SuccessfulStrategy);
+
+        var attempt = result.FailureAttempts?.Single();
+        Assert.Equal("well now I am not doing it", attempt?.FailureReason);
+        Assert.Equal(strategy.GetType(), attempt?.StrategyType);
+    }
+
+    [Theory]
+    [MemberData(nameof(StrategyExceptions))]
+    public async Task CollectAsync_ThrowsException_ReturnsFailure_WithExceptionMessage(Exception strategyException) {
+        //Arrange
+        var strategy = new FakeCollectionStrategy(
+            canExecute: true,
+            exception: strategyException
+        );
+
+        // Act
+        var result = await _strategyExecutor.CollectAsync("target machine", [], [strategy]);
+
+        // Assert
+        Assert.False(result.WasSuccessful);
+        Assert.Null(result.Results);
+        Assert.Null(result.SuccessfulStrategy);
+
+        var attempt = result.FailureAttempts?.Single();
+        Assert.Contains($"Collector failed: {strategyException.Message}.", attempt!.FailureReason!); 
+
+        if (strategyException.InnerException is not null) 
+            Assert.Contains($"\nInner Exception: {strategyException.InnerException}", attempt!.FailureReason!); 
+        else 
+            Assert.DoesNotContain("Inner Exception:", attempt!.FailureReason!);
+    }
+
+    public static IEnumerable<object[]> StrategyExceptions =>
+    [
+        [new Exception("Outer Exception")],
+        [new Exception("Outer Exception", new Exception("Inner Exception"))]
+    ];
+
+    [Fact]
+    public async Task CollectAsync_FirstStrategySuccessful_ReturnsSuccess_WithNoAttempts() {
+        //Arrange
+        var strategyResult = new RegistryQueryResult(@"SYSTEM\CurrentControlSet\Control\Lsa\MSV1_0", "NtlmMinClientSec", 1, null, true);
+        var strategy = new FakeCollectionStrategy(
+            true,
+            results: [strategyResult]
+        );
+
+        // Act
+        var result = await _strategyExecutor.CollectAsync("target machine", [], [strategy]);
+
+        // Assert
+        Assert.True(result.WasSuccessful);
+        Assert.Empty(result.FailureAttempts!);
+        Assert.Equal(strategy.GetType(), result.SuccessfulStrategy);
+        Assert.Equal(strategyResult, result.Results?.Single());
+    }
+
+    [Fact]
+    public async Task CollectAsync_SecondStrategySuccessful_ReturnsSuccess_WithAttempt() {
+        //Arrange
+        var failedStrategy = new FakeCollectionStrategy(false, "well now I am not doing it");
+        var strategyResult = new RegistryQueryResult(@"SYSTEM\CurrentControlSet\Control\Lsa\MSV1_0", "NtlmMinClientSec", 1, null, true);
+        var successfulStrategy = new FakeCollectionStrategy(
+            true,
+            results: [strategyResult]
+        );
+
+        // Act
+        var result = await _strategyExecutor.CollectAsync("target machine", [], [failedStrategy, successfulStrategy]);
+
+        // Assert
+        Assert.True(result.WasSuccessful);
+        Assert.Single(result.FailureAttempts!);
+        Assert.Equal(successfulStrategy.GetType(), result.SuccessfulStrategy);
+        Assert.Equal(strategyResult, result.Results?.Single());
+    }
+}
+
+internal sealed class FakeCollectionStrategy(
+    bool canExecute,
+    string failureReason = "",
+    Exception? exception = null,
+    IEnumerable<RegistryQueryResult>? results = null
+) : ICollectionStrategy<RegistryQueryResult, RegistryQuery> 
+{
+    public Task<(bool, string)> CanExecute(string target)
+        => Task.FromResult((canExecute, failureReason));
+
+    public Task<IEnumerable<RegistryQueryResult>> ExecuteAsync(
+        string target,
+        IEnumerable<RegistryQuery> queries) {
+        if (exception is not null)
+            throw exception;
+
+        return Task.FromResult(results ?? []);
+    }
+}

SharpHoundCommon.sln

@@ -9,6 +9,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Docfx", "docfx\Docfx.csproj
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SharpHoundRPC", "src\SharpHoundRPC\SharpHoundRPC.csproj", "{4F06116D-88A7-4601-AB28-B48F2857D458}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "RPCTest", "RPCTest\RPCTest.csproj", "{F1E6714E-72B3-4295-9940-0EAA69695201}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -31,5 +33,9 @@ Global
 		{4F06116D-88A7-4601-AB28-B48F2857D458}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{4F06116D-88A7-4601-AB28-B48F2857D458}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{4F06116D-88A7-4601-AB28-B48F2857D458}.Release|Any CPU.Build.0 = Release|Any CPU
+		{F1E6714E-72B3-4295-9940-0EAA69695201}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{F1E6714E-72B3-4295-9940-0EAA69695201}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{F1E6714E-72B3-4295-9940-0EAA69695201}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{F1E6714E-72B3-4295-9940-0EAA69695201}.Release|Any CPU.Build.0 = Release|Any CPU
 	EndGlobalSection
 EndGlobal

src/CommonLib/Processors/ComputerAvailability.cs

@@ -95,11 +95,19 @@ await SendComputerStatus(new CSVComputerStatus {
                 };
             }
 
-            if (_skipPortScan)
+            if (_skipPortScan) {
+                await SendComputerStatus(new CSVComputerStatus {
+                    Status = CSVComputerStatus.StatusSuccess,
+                    Task = "ComputerAvailability",
+                    ComputerName = computerName,
+                    ObjectId = objectId,
+                });
+
                 return new ComputerStatus {
                     Connectable = true,
                     Error = null
                 };
+            }
 
             if (!await _scanner.CheckPort(computerName)) {
                 _log.LogTrace("{ComputerName} is not available because port 445 is unavailable", computerName);

src/CommonLib/Processors/RegistryProcessor.cs

@@ -6,20 +6,24 @@
 using System;
 using System.Linq;
 using System.Threading.Tasks;
-using static SharpHoundCommonLib.Helpers;
 
 namespace SharpHoundCommonLib.Processors;
 
 public class RegistryProcessor {
+    public delegate Task ComputerStatusDelegate(CSVComputerStatus status);
+
     private readonly ILogger _log;
     private readonly IPortScanner _portScanner;
+    private readonly IStrategyExecutor _registryCollector; 
+    private readonly AdaptiveTimeout _registryAdaptiveTimeout = new(maxTimeout:TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ReadRegistrySettings)));
     private readonly ICollectionStrategy<RegistryQueryResult, RegistryQuery>[] _strategies;
     private readonly RegistryQuery[] _queries;
-    private readonly AdaptiveTimeout _registryAdaptiveTimeout = new(maxTimeout:TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ReadRegistrySettings)));
 
-    public RegistryProcessor(ILogger log, string domain) {
+    public RegistryProcessor(ILogger log, IStrategyExecutor registryCollector, string domain) {
         _log = log ?? Logging.LogProvider.CreateLogger("RegistryProcessor");
         _portScanner = new PortScanner();
+        _registryCollector = registryCollector;
+
         _strategies = [
             // Higher priority at the top of the list
             new DotNetWmiRegistryStrategy(_portScanner, domain),
@@ -50,12 +54,13 @@ public RegistryProcessor(ILogger log, string domain) {
         ];
     }
 
+    public event ComputerStatusDelegate ComputerStatusEvent;
+
     public async Task<APIResult<RegistryData>> ReadRegistrySettings(string targetMachine) {
         var output = new RegistryData();
 
         try {
-            var registryCollector = new StrategyExecutor();
-            var result = await _registryAdaptiveTimeout.ExecuteWithTimeout(async (_) => await registryCollector
+            var result = await _registryAdaptiveTimeout.ExecuteWithTimeout(async (_) => await _registryCollector
                 .CollectAsync(targetMachine, _queries, _strategies)
                 .ConfigureAwait(false));
 
@@ -65,6 +70,32 @@ public async Task<APIResult<RegistryData>> ReadRegistrySettings(string targetMac
 
             var collectedData = result.Value;
 
+            foreach (var attempt in collectedData.FailureAttempts ?? []) {
+                _log.LogTrace("ReadRegistry failed on {ComputerName} using {Strategy}: {Error}", targetMachine, attempt.StrategyType.Name, attempt.FailureReason);
+                await SendComputerStatus(new CSVComputerStatus
+                {
+                    Task = $"{nameof(ReadRegistrySettings)} - {attempt.StrategyType.Name}",
+                    ComputerName = targetMachine,
+                    Status = attempt.FailureReason
+                });
+            }
+
+            if (!collectedData.WasSuccessful) {
+                var msg = collectedData.FailureAttempts is null 
+                    ? "Failed to read registry settings"
+                    : string.Join("\n",
+                        collectedData.FailureAttempts.Select(a => $"{a.StrategyType.Name}: {a.FailureReason ?? ""}"));
+
+                return APIResult<RegistryData>.Failure(msg);
+            }
+
+            await SendComputerStatus(new CSVComputerStatus
+            {
+                Task = $"{nameof(ReadRegistrySettings)} - {collectedData.SuccessfulStrategy?.Name ?? ""}",
+                ComputerName = targetMachine,
+                Status = CSVComputerStatus.StatusSuccess
+            });
+
             foreach (var key in collectedData.Results ?? []) {
                 if (!key.ValueExists)
                     continue;
@@ -101,13 +132,6 @@ public async Task<APIResult<RegistryData>> ReadRegistrySettings(string targetMac
                 }
             }
 
-            // If all strategies failed, need to report errors.
-            if (collectedData.FailureAttempts.Count() == _strategies.Length) {
-                string msg = string.Join("\n",
-                    collectedData.FailureAttempts.Select(a => $"{a.StrategyType.Name}: {a.FailureReason ?? ""}"));
-                return APIResult<RegistryData>.Failure(msg);
-            }
-
             return APIResult<RegistryData>.Success(output);
         } catch (Exception ex) {
             _log.LogError(
@@ -118,4 +142,8 @@ public async Task<APIResult<RegistryData>> ReadRegistrySettings(string targetMac
             return APIResult<RegistryData>.Failure(ex.ToString());
         }
     }
+
+    private async Task SendComputerStatus(CSVComputerStatus status) {
+        if (ComputerStatusEvent is not null) await ComputerStatusEvent.Invoke(status);
+    }
 }

src/SharpHoundRPC/Registry/DotNetWmiRegistryStrategy.cs

@@ -26,7 +26,6 @@ public class DotNetWmiRegistryStrategy : ICollectionStrategy<RegistryQueryResult
         /// </summary>
         public bool UseKerberos { get; set; } = true;
 
-
         /// <summary>
         /// Creates a new WMI registry strategy
         /// </summary>
@@ -38,6 +37,9 @@ public DotNetWmiRegistryStrategy(IPortScanner scanner, string domain) {
         }
 
         public async Task<(bool, string)> CanExecute(string targetMachine) {
+            if (string.IsNullOrEmpty(targetMachine)) {
+                throw new ArgumentException("Target machine cannot be null or empty", nameof(targetMachine));
+            }
             try {
                 var isOpen = await _portScanner.CheckPort(targetMachine, EpMapperPort, throwError: true);
                 return (isOpen, string.Empty);

src/SharpHoundRPC/Registry/NativeUtils.cs

@@ -1,10 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
-using System.Linq;
 using System.Runtime.InteropServices;
 using System.Text;
-using System.Threading.Tasks;
 using static SharpHoundRPC.NetAPINative.NetAPIEnums;
 
 namespace SharpHoundRPC.Registry {

src/SharpHoundRPC/Registry/RemoteRegistryStrategy.cs

@@ -1,7 +1,7 @@
 #nullable enable
 namespace SharpHoundRPC.Registry {
     using Microsoft.Win32;
-    using SharpHoundRPC.PortScanner;
+    using PortScanner;
     using System;
     using System.Collections.Generic;
     using System.Linq;
@@ -38,7 +38,6 @@ public async Task<IEnumerable<RegistryQueryResult>> ExecuteAsync(
             if (queries == null || !queries.Any())
                 throw new ArgumentException("Queries cannot be null or empty", nameof(queries));
 
-
             return await Task.Run(() => {
                 var results = new List<RegistryQueryResult>();
 

src/SharpHoundRPC/Registry/StrategyExecutor.cs

@@ -4,8 +4,15 @@ namespace SharpHoundRPC.Registry {
     using System.Collections.Generic;
     using System.Threading.Tasks;
 
-
-    public class StrategyExecutor {
+    public interface IStrategyExecutor
+    {
+        Task<StrategyExecutorResult<T>> CollectAsync<T, TQuery>(
+            string targetMachine,
+            IEnumerable<TQuery> queries,
+            IEnumerable<ICollectionStrategy<T, TQuery>> strategies);
+    }
+
+    public class StrategyExecutor : IStrategyExecutor {
         public async Task<StrategyExecutorResult<T>> CollectAsync<T, TQuery>(
             string targetMachine,
             IEnumerable<TQuery> queries,
@@ -25,24 +32,26 @@ public async Task<StrategyExecutorResult<T>> CollectAsync<T, TQuery>(
                 try {
                     var results = await strategy.ExecuteAsync(targetMachine, queries).ConfigureAwait(false);
 
-                    attempt.WasSuccessful = true;
-                    attempt.Results = results;
-
                     return new StrategyExecutorResult<T> {
                         Results = results,
                         FailureAttempts = attempts,
-                        WasSuccessful = true
+                        WasSuccessful = true,
+                        SuccessfulStrategy =  strategy.GetType()
                     };
                 } catch (Exception ex) {
-                    attempt.FailureReason = $"Collector failed: {ex.Message}.\nInner Exception: {ex.InnerException}";
+                    var innerException = ex.InnerException != null
+                        ? $"\nInner Exception: {ex.InnerException}"
+                        : string.Empty;
+
+                    attempt.FailureReason = $"Collector failed: {ex.Message}.{innerException}";
                 }
 
                 attempts.Add(attempt);
             }
 
             return new StrategyExecutorResult<T> {
-                Results = null,
-                FailureAttempts = attempts
+                FailureAttempts = attempts,
+                WasSuccessful = false,
             };
         }
     }