From da66d0329831403d0aeba468353ba1894792d008 Mon Sep 17 00:00:00 2001 From: anemeth Date: Wed, 26 Aug 2026 11:56:37 -0700 Subject: [PATCH 1/3] fix: Make ACLProcessor guid cache singleton and thread-safe --- src/CommonLib/Processors/ACLProcessor.cs | 38 +++++++++++++++------ test/unit/ACLProcessorTest.cs | 43 ++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 53e346ab..9fa038ca 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -13,16 +13,20 @@ using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading; namespace SharpHoundCommonLib.Processors { public class ACLProcessor { private static readonly Dictionary BaseGuids; - private readonly ConcurrentDictionary _guidMap = new(); + /// This is a shared cache of GUID mappings for each ILdapUtils instance. It allows multiple ACLProcessor instances to share the same GUID cache for a given ILdapUtils instance. + /// This resolves an issue from back when the guid cache was static and shared across all ILdapUtils instances, which was causing issues when domains were being processed in parallel for tests: https://github.com/SpecterOps/SharpHoundCommon/pull/169 + /// Learn about Conditional Weak Tables https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2?view=netframework-4.7.2#examples + /// But the short version is that this allows us to have a shared cache for each ILdapUtils instance, but when the ILdapUtils instance is garbage collected, the cache will be garbage collected as well. + private static readonly ConditionalWeakTable SharedGuidCaches = new(); private readonly ILogger _log; private readonly ILdapUtils _utils; - private readonly ConcurrentHashSet _builtDomainCaches = new(StringComparer.OrdinalIgnoreCase); private readonly ConcurrentDictionary _exchangeTrusteeSidCache = new(StringComparer.OrdinalIgnoreCase); - private readonly object _lock = new(); // These Exchange principals commonly carry product-added deny ACEs that we intentionally suppress. private static readonly HashSet ExchangeTrusteeNames = new(StringComparer.OrdinalIgnoreCase) { "Exchange Windows Permissions", @@ -30,6 +34,17 @@ public class ACLProcessor { "Exchange Servers", "Organization Management" }; + private readonly GuidCacheState _guidCache; + + private sealed class GuidCacheState { + // This is a mapping of GUIDs to their corresponding names for LDAP rights. + // The collection represents the response from the LDAP query Task kept in BuildTasks. + public readonly ConcurrentDictionary GuidMap = new(); + // This is a mapping of domains to their corresponding build tasks for the GUID cache. + // The Lazy ensures that the build task is only executed once per domain, even if multiple threads attempt to build the cache for the same domain simultaneously. + public readonly ConcurrentDictionary> BuildTasks = + new(StringComparer.OrdinalIgnoreCase); + } static ACLProcessor() { //Create a dictionary with the base GUIDs of each object type @@ -54,6 +69,7 @@ static ACLProcessor() { public ACLProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; + _guidCache = SharedGuidCaches.GetValue(utils, _ => new GuidCacheState()); _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); } @@ -120,14 +136,14 @@ public override string ToString() { /// LAPS /// private async Task BuildGuidCache(string domain) { - lock (_lock) { - if (_builtDomainCaches.Contains(domain)) { - return; - } + var buildTask = _guidCache.BuildTasks.GetOrAdd(domain, + // The ExecutionAndPublication mode ensures that only one thread can execute the factory method at a time, and all other threads will wait for the result of that execution. This prevents multiple threads from building the cache simultaneously for the same domain. + _ => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); - _builtDomainCaches.Add(domain); - } + await buildTask.Value; + } + private async Task BuildGuidCacheCore(string domain) { _log.LogInformation("Building GUID Cache for {Domain}", domain); await foreach (var result in _utils.PagedQuery(new LdapQueryParameters { DomainName = domain, @@ -155,7 +171,7 @@ private async Task BuildGuidCache(string domain) { if (name is LDAPProperties.LAPSPlaintextPassword or LDAPProperties.LAPSEncryptedPassword or LDAPProperties.LegacyLAPSPassword) { _log.LogInformation("Found GUID for ACL Right {Name}: {Guid} in domain {Domain}", name, guid, domain); - _guidMap.TryAdd(guid, name); + _guidCache.GuidMap.TryAdd(guid, name); } } else { _log.LogDebug("Error while building GUID cache for {Domain}: {Message}", domain, result.Error); @@ -770,7 +786,7 @@ await CountCustomDenyAce(ace, customDenyAceAccumulator, objectDomain, objectType IsPermissionForOwnerRightsSid = isPermissionForOwnerRightsSid, IsInheritedPermissionForOwnerRightsSid = isInheritedPermissionForOwnerRightsSid, }; - else if (_guidMap.TryGetValue(aceType, out var lapsAttribute)) { + else if (_guidCache.GuidMap.TryGetValue(aceType, out var lapsAttribute)) { // Compare the retrieved attribute name against LDAPProperties values if (lapsAttribute == LDAPProperties.LegacyLAPSPassword || lapsAttribute == LDAPProperties.LAPSPlaintextPassword || diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index 8a3a2b5b..479d6aeb 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -58,6 +58,49 @@ public void SanityCheck() { Assert.True(true); } + [Fact] + public async Task ACLProcessor_BuildGuidCache_AcrossInstances_QueriesOncePerDomain() { + var mockLdapUtils = new Mock(); + mockLdapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(Array.Empty>().ToAsyncEnumerable); + var domain = $"{Guid.NewGuid():N}.TEST"; + var processors = Enumerable.Range(0, 50) + .Select(_ => new ACLProcessor(mockLdapUtils.Object)) + .ToArray(); + + await Task.WhenAll(processors.Select(processor => + processor.ProcessACL(null, domain, Label.Computer, false).ToArrayAsync())); + + mockLdapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Once); + } + + [Fact] + public async Task ACLProcessor_BuildGuidCache_AcrossLdapUtils_QueriesOncePerUtility() { + var firstLdapUtils = new Mock(); + var secondLdapUtils = new Mock(); + foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { + ldapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(Array.Empty>().ToAsyncEnumerable); + } + + var domain = $"{Guid.NewGuid():N}.TEST"; + await Task.WhenAll( + new ACLProcessor(firstLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(), + new ACLProcessor(secondLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); + + foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { + ldapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Once); + } + } + [Fact] public void ACLProcessor_IsACLProtected_NullNTSD_ReturnsFalse() { var processor = new ACLProcessor(new MockLdapUtils()); From 9a881c498b8eb9295983ae3fda0712339f27112c Mon Sep 17 00:00:00 2001 From: anemeth Date: Thu, 27 Aug 2026 12:46:42 -0700 Subject: [PATCH 2/3] feat: Apply new ProcessorContext pattern to ACLProcessor for shared state and state lifetime management --- src/CommonLib/Processors/ACLProcessor.cs | 99 ++++++++++++++++++------ test/unit/ACLProcessorTest.cs | 58 +++++++++----- 2 files changed, 115 insertions(+), 42 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 9fa038ca..86d16380 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -13,17 +13,43 @@ using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; using System.Linq; -using System.Runtime.CompilerServices; using System.Threading; namespace SharpHoundCommonLib.Processors { + /// + /// Owns state shared by processor instances and gives that state an explicit lifetime. + /// + public sealed class ACLProcessorContext : IDisposable { + private readonly ACLProcessor.GuidCache _aclGuidCache = new(); + private int _disposed; + + /// + /// Creates an that shares its GUID cache with other + /// ACL processors created by this context. + /// + public ACLProcessor CreateACLProcessor(ILdapUtils utils, ILogger log = null) { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(ACLProcessorContext)); + } + + return new ACLProcessor(utils, _aclGuidCache, log); + } + + /// + /// Clears the shared processor state. Processors created by this context must not + /// be used after the context is disposed. + /// + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _aclGuidCache.Dispose(); + } + } + public class ACLProcessor { private static readonly Dictionary BaseGuids; - /// This is a shared cache of GUID mappings for each ILdapUtils instance. It allows multiple ACLProcessor instances to share the same GUID cache for a given ILdapUtils instance. - /// This resolves an issue from back when the guid cache was static and shared across all ILdapUtils instances, which was causing issues when domains were being processed in parallel for tests: https://github.com/SpecterOps/SharpHoundCommon/pull/169 - /// Learn about Conditional Weak Tables https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2?view=netframework-4.7.2#examples - /// But the short version is that this allows us to have a shared cache for each ILdapUtils instance, but when the ILdapUtils instance is garbage collected, the cache will be garbage collected as well. - private static readonly ConditionalWeakTable SharedGuidCaches = new(); private readonly ILogger _log; private readonly ILdapUtils _utils; private readonly ConcurrentDictionary _exchangeTrusteeSidCache = new(StringComparer.OrdinalIgnoreCase); @@ -34,16 +60,43 @@ public class ACLProcessor { "Exchange Servers", "Organization Management" }; - private readonly GuidCacheState _guidCache; - - private sealed class GuidCacheState { - // This is a mapping of GUIDs to their corresponding names for LDAP rights. - // The collection represents the response from the LDAP query Task kept in BuildTasks. - public readonly ConcurrentDictionary GuidMap = new(); - // This is a mapping of domains to their corresponding build tasks for the GUID cache. - // The Lazy ensures that the build task is only executed once per domain, even if multiple threads attempt to build the cache for the same domain simultaneously. - public readonly ConcurrentDictionary> BuildTasks = + private readonly GuidCache _guidCache; + + internal sealed class GuidCache : IDisposable { + private readonly ConcurrentDictionary _guidMap = new(); + private readonly ConcurrentDictionary> _buildTasks = new(StringComparer.OrdinalIgnoreCase); + private int _disposed; + + public Lazy GetOrAddBuildTask(string domain, Func> buildTaskFactory) { + ThrowIfDisposed(); + return _buildTasks.GetOrAdd(domain, _ => buildTaskFactory()); + } + + public void AddGuid(string guid, string name) { + ThrowIfDisposed(); + _guidMap.TryAdd(guid, name); + } + + public bool TryGetGuid(string guid, out string name) { + ThrowIfDisposed(); + return _guidMap.TryGetValue(guid, out name); + } + + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _buildTasks.Clear(); + _guidMap.Clear(); + } + + private void ThrowIfDisposed() { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(ACLProcessorContext)); + } + } } static ACLProcessor() { @@ -66,10 +119,12 @@ static ACLProcessor() { }; } - public ACLProcessor(ILdapUtils utils, ILogger log = null) - { + public ACLProcessor(ILdapUtils utils, ILogger log = null) : this(utils, new GuidCache(), log) { + } + + internal ACLProcessor(ILdapUtils utils, GuidCache guidCache, ILogger log = null) { _utils = utils; - _guidCache = SharedGuidCaches.GetValue(utils, _ => new GuidCacheState()); + _guidCache = guidCache; _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); } @@ -136,9 +191,9 @@ public override string ToString() { /// LAPS /// private async Task BuildGuidCache(string domain) { - var buildTask = _guidCache.BuildTasks.GetOrAdd(domain, + var buildTask = _guidCache.GetOrAddBuildTask(domain, // The ExecutionAndPublication mode ensures that only one thread can execute the factory method at a time, and all other threads will wait for the result of that execution. This prevents multiple threads from building the cache simultaneously for the same domain. - _ => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); + () => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); await buildTask.Value; } @@ -171,7 +226,7 @@ private async Task BuildGuidCacheCore(string domain) { if (name is LDAPProperties.LAPSPlaintextPassword or LDAPProperties.LAPSEncryptedPassword or LDAPProperties.LegacyLAPSPassword) { _log.LogInformation("Found GUID for ACL Right {Name}: {Guid} in domain {Domain}", name, guid, domain); - _guidCache.GuidMap.TryAdd(guid, name); + _guidCache.AddGuid(guid, name); } } else { _log.LogDebug("Error while building GUID cache for {Domain}: {Message}", domain, result.Error); @@ -786,7 +841,7 @@ await CountCustomDenyAce(ace, customDenyAceAccumulator, objectDomain, objectType IsPermissionForOwnerRightsSid = isPermissionForOwnerRightsSid, IsInheritedPermissionForOwnerRightsSid = isInheritedPermissionForOwnerRightsSid, }; - else if (_guidCache.GuidMap.TryGetValue(aceType, out var lapsAttribute)) { + else if (_guidCache.TryGetGuid(aceType, out var lapsAttribute)) { // Compare the retrieved attribute name against LDAPProperties values if (lapsAttribute == LDAPProperties.LegacyLAPSPassword || lapsAttribute == LDAPProperties.LAPSPlaintextPassword || diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index 479d6aeb..620533c9 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -59,14 +59,15 @@ public void SanityCheck() { } [Fact] - public async Task ACLProcessor_BuildGuidCache_AcrossInstances_QueriesOncePerDomain() { + public async Task ProcessorContext_ACLProcessors_QueryOncePerDomain() { var mockLdapUtils = new Mock(); mockLdapUtils .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) .Returns(Array.Empty>().ToAsyncEnumerable); var domain = $"{Guid.NewGuid():N}.TEST"; + using var context = new ACLProcessorContext(); var processors = Enumerable.Range(0, 50) - .Select(_ => new ACLProcessor(mockLdapUtils.Object)) + .Select(_ => context.CreateACLProcessor(mockLdapUtils.Object)) .ToArray(); await Task.WhenAll(processors.Select(processor => @@ -79,26 +80,43 @@ await Task.WhenAll(processors.Select(processor => } [Fact] - public async Task ACLProcessor_BuildGuidCache_AcrossLdapUtils_QueriesOncePerUtility() { - var firstLdapUtils = new Mock(); - var secondLdapUtils = new Mock(); - foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { - ldapUtils - .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) - .Returns(Array.Empty>().ToAsyncEnumerable); - } - + public async Task ProcessorContext_ACLProcessors_DoNotShareCacheAcrossContexts() { + var mockLdapUtils = new Mock(); + mockLdapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(Array.Empty>().ToAsyncEnumerable); var domain = $"{Guid.NewGuid():N}.TEST"; + using var firstContext = new ACLProcessorContext(); + using var secondContext = new ACLProcessorContext(); + await Task.WhenAll( - new ACLProcessor(firstLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(), - new ACLProcessor(secondLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); - - foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { - ldapUtils.Verify( - x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), - It.IsAny()), - Times.Once); - } + firstContext.CreateACLProcessor(mockLdapUtils.Object) + .ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(), + secondContext.CreateACLProcessor(mockLdapUtils.Object) + .ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); + + mockLdapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Exactly(2)); + } + + [Fact] + public void ProcessorContext_CreateACLProcessor_AfterDispose_Throws() { + var context = new ACLProcessorContext(); + context.Dispose(); + + Assert.Throws(() => context.CreateACLProcessor(new MockLdapUtils())); + } + + [Fact] + public async Task ProcessorContext_ACLProcessor_AfterDispose_Throws() { + var context = new ACLProcessorContext(); + var processor = context.CreateACLProcessor(new MockLdapUtils()); + context.Dispose(); + + await Assert.ThrowsAsync(() => + processor.ProcessACL(null, "TEST.LOCAL", Label.Computer, false).ToArrayAsync()); } [Fact] From 45e4a3b6433bd096bd327b42e8502f2cc9883db6 Mon Sep 17 00:00:00 2001 From: anemeth Date: Wed, 2 Sep 2026 15:26:26 -0700 Subject: [PATCH 3/3] fix: Allow faulted guid schema queries to retry --- src/CommonLib/Processors/ACLProcessor.cs | 16 +++++++++++++- test/unit/ACLProcessorTest.cs | 28 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 86d16380..ed980367 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -73,6 +73,14 @@ public Lazy GetOrAddBuildTask(string domain, Func> buildTaskFac return _buildTasks.GetOrAdd(domain, _ => buildTaskFactory()); } + public bool RemoveBuildTask(string domain, Lazy buildTask) { + // Remove only this instance so a delayed fault cannot remove a newer retry task. + // _buildTasks.TryRemove does not guarantee that the value is the same as the one being removed, so we need to cast to ICollection and use Remove instead. + // This lets us conditionally remove the task only if it is the same instance as the one we expect. + return ((ICollection>>)_buildTasks) + .Remove(new KeyValuePair>(domain, buildTask)); + } + public void AddGuid(string guid, string name) { ThrowIfDisposed(); _guidMap.TryAdd(guid, name); @@ -195,7 +203,13 @@ private async Task BuildGuidCache(string domain) { // The ExecutionAndPublication mode ensures that only one thread can execute the factory method at a time, and all other threads will wait for the result of that execution. This prevents multiple threads from building the cache simultaneously for the same domain. () => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); - await buildTask.Value; + try { + await buildTask.Value; + } + catch { + _guidCache.RemoveBuildTask(domain, buildTask); + throw; + } } private async Task BuildGuidCacheCore(string domain) { diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index 620533c9..0cb9ddbb 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -79,6 +79,34 @@ await Task.WhenAll(processors.Select(processor => Times.Once); } + [Fact] + public async Task ProcessorContext_ACLProcessors_RetriesGuidCacheBuildAfterFailure() { + var mockLdapUtils = new Mock(); + var queryAttempts = 0; + mockLdapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(() => { + if (Interlocked.Increment(ref queryAttempts) == 1) { + throw new InvalidOperationException("Expected test failure"); + } + + return Array.Empty>().ToAsyncEnumerable(); + }); + var domain = $"{Guid.NewGuid():N}.TEST"; + using var context = new ACLProcessorContext(); + var processor = context.CreateACLProcessor(mockLdapUtils.Object); + + await Assert.ThrowsAsync(() => + processor.ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); + + await processor.ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(); + + mockLdapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Exactly(2)); + } + [Fact] public async Task ProcessorContext_ACLProcessors_DoNotShareCacheAcrossContexts() { var mockLdapUtils = new Mock();