From de0ffda2524361f38e95a6becc1aba4372c73c66 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 2 Feb 2024 13:17:58 -0500 Subject: [PATCH 01/11] fix: fix ldap backoff retry logic to actually create new connections, and clean up edge cases --- src/CommonLib/Enums/LdapErrorCodes.cs | 3 +- src/CommonLib/LDAPUtils.cs | 181 +++++++++----------------- 2 files changed, 65 insertions(+), 119 deletions(-) diff --git a/src/CommonLib/Enums/LdapErrorCodes.cs b/src/CommonLib/Enums/LdapErrorCodes.cs index 31e61890..cfbba682 100644 --- a/src/CommonLib/Enums/LdapErrorCodes.cs +++ b/src/CommonLib/Enums/LdapErrorCodes.cs @@ -4,6 +4,7 @@ public enum LdapErrorCodes : int { Success = 0, Busy = 51, - ServerDown = 81 + ServerDown = 81, + LocalError = 82 } } \ No newline at end of file diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index 80ccde11..5c76a8c1 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -5,6 +5,7 @@ using System.DirectoryServices; using System.DirectoryServices.ActiveDirectory; using System.DirectoryServices.Protocols; +using System.IO.Compression; using System.Linq; using System.Net; using System.Net.Sockets; @@ -18,9 +19,7 @@ using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; using SharpHoundCommonLib.Processors; -using SharpHoundRPC; using SharpHoundRPC.NetAPINative; -using SharpHoundRPC.Wrappers; using Domain = System.DirectoryServices.ActiveDirectory.Domain; using SearchScope = System.DirectoryServices.Protocols.SearchScope; using SecurityMasks = System.DirectoryServices.Protocols.SecurityMasks; @@ -53,8 +52,8 @@ private static readonly ConcurrentDictionary private readonly ConcurrentDictionary _domainCache = new(); private readonly ConcurrentDictionary _domainControllerCache = new(); private static readonly TimeSpan MinBackoffDelay = TimeSpan.FromSeconds(2); - private static readonly TimeSpan MaxBackoffDelay = TimeSpan.FromSeconds(10); - private static readonly TimeSpan BackoffDelayMultiplier = TimeSpan.FromSeconds(2); + private static readonly TimeSpan MaxBackoffDelay = TimeSpan.FromSeconds(20); + private static readonly int BackoffDelayMultiplier = 2; private const int MaxRetries = 3; private readonly ConcurrentDictionary _globalCatalogConnections = new(); @@ -66,6 +65,7 @@ private static readonly ConcurrentDictionary private readonly ConcurrentDictionary _netbiosCache = new(); private readonly PortScanner _portScanner; private LDAPConfig _ldapConfig = new(); + private readonly SemaphoreSlim _semaphoreSlim = new(1, 1); /// /// Creates a new instance of LDAP Utils with defaults @@ -517,8 +517,7 @@ public IEnumerable DoRangedRetrieval(string distinguishedName, string at //Allow three retries with a backoff on each one if we get a "Server is Busy" error retryCount++; Thread.Sleep(backoffDelay); - backoffDelay = TimeSpan.FromSeconds(Math.Min( - backoffDelay.TotalSeconds * BackoffDelayMultiplier.TotalSeconds, MaxBackoffDelay.TotalSeconds)); + backoffDelay = GetNextBackoff(retryCount); continue; } catch (Exception e) @@ -874,38 +873,65 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope }catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.ServerDown && retryCount < MaxRetries) { + var isSemaphoreHeld = _semaphoreSlim.CurrentCount == 0; retryCount++; - Thread.Sleep(backoffDelay); - backoffDelay = TimeSpan.FromSeconds(Math.Min( - backoffDelay.TotalSeconds * BackoffDelayMultiplier.TotalSeconds, MaxBackoffDelay.TotalSeconds)); - conn = CreateNewConnection(domainName, globalCatalog, skipCache); - if (conn == null) + + _semaphoreSlim.Wait(cancellationToken); + try { - _log.LogError("Unable to create replacement ldap connection for ServerDown exception. Breaking loop"); - yield break; + if (!isSemaphoreHeld) + { + Thread.Sleep(backoffDelay); + backoffDelay = GetNextBackoff(retryCount); + conn = CreateNewConnection(domainName, globalCatalog, true); + if (conn == null) + { + _log.LogError( + "Unable to create replacement ldap connection for ServerDown exception. Breaking loop"); + yield break; + } + + _log.LogInformation("Created new LDAP connection after receiving ServerDown from server"); + } + else + { + backoffDelay = GetNextBackoff(retryCount); + conn = CreateNewConnection(domainName, globalCatalog); + } + } + finally + { + _semaphoreSlim.Release(); } - - _log.LogInformation("Created new LDAP connection after receiving ServerDown from server"); continue; }catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.Busy && retryCount < MaxRetries) { retryCount++; - Thread.Sleep(backoffDelay); - backoffDelay = TimeSpan.FromSeconds(Math.Min( - backoffDelay.TotalSeconds * BackoffDelayMultiplier.TotalSeconds, MaxBackoffDelay.TotalSeconds)); + backoffDelay = GetNextBackoff(retryCount); continue; } catch (LdapException le) { - if (le.ErrorCode != 82) + if (le.ErrorCode != (int)LdapErrorCodes.LocalError) + { if (throwException) + { throw new LDAPQueryException( - $"LDAP Exception in Loop: {le.ErrorCode}. {le.ServerErrorMessage}. {le.Message}. Filter: {ldapFilter}. Domain: {domainName}.", + $"LDAP Exception in Loop: {le.ErrorCode}. {le.ServerErrorMessage}. {le.Message}. Filter: {ldapFilter}. Domain: {domainName}", le); - else - _log.LogWarning(le, - "LDAP Exception in Loop: {ErrorCode}. {ServerErrorMessage}. {Message}. Filter: {Filter}. Domain: {Domain}", - le.ErrorCode, le.ServerErrorMessage, le.Message, ldapFilter, domainName); + } + + _log.LogWarning(le, + "LDAP Exception in Loop: {ErrorCode}. {ServerErrorMessage}. {Message}. Filter: {Filter}. Domain: {Domain}", + le.ErrorCode, le.ServerErrorMessage, le.Message, ldapFilter, domainName); + } + if (le.ErrorCode == (int)LdapErrorCodes.ServerDown) + { + throw new LDAPQueryException( + $"LDAP Exception in Loop: {le.ErrorCode}. {le.ServerErrorMessage}. {le.Message}. Filter: {ldapFilter}. Domain: {domainName}", + le); + } + yield break; } catch (Exception e) @@ -979,99 +1005,15 @@ public virtual IEnumerable QueryLDAP(string ldapFilter, Sear string[] props, string domainName = null, bool includeAcl = false, bool showDeleted = false, string adsPath = null, bool globalCatalog = false, bool skipCache = false, bool throwException = false) { - var queryParams = SetupLDAPQueryFilter( - ldapFilter, scope, props, includeAcl, domainName, includeAcl, adsPath, globalCatalog, skipCache); - - if (queryParams.Exception != null) - { - if (throwException) throw queryParams.Exception; - - _log.LogWarning(queryParams.Exception, "Failed to setup LDAP Query Filter"); - yield break; - } - var conn = queryParams.Connection; - var request = queryParams.SearchRequest; - var pageControl = queryParams.PageControl; - - PageResultResponseControl pageResponse = null; - - var backoffDelay = MinBackoffDelay; - var retryCount = 0; - - while (true) - { - SearchResponse response; - - try - { - _log.LogTrace("Sending LDAP request for {Filter}", ldapFilter); - response = (SearchResponse)conn.SendRequest(request); - if (response != null) - pageResponse = (PageResultResponseControl)response.Controls - .Where(x => x is PageResultResponseControl).DefaultIfEmpty(null).FirstOrDefault(); - } - catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.Busy && retryCount < MaxRetries) - { - retryCount++; - Thread.Sleep(backoffDelay); - backoffDelay = TimeSpan.FromSeconds(Math.Min( - backoffDelay.TotalSeconds * BackoffDelayMultiplier.TotalSeconds, MaxBackoffDelay.TotalSeconds)); - continue; - } - catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.ServerDown && - retryCount < MaxRetries) - { - retryCount++; - Thread.Sleep(backoffDelay); - backoffDelay = TimeSpan.FromSeconds(Math.Min( - backoffDelay.TotalSeconds * BackoffDelayMultiplier.TotalSeconds, MaxBackoffDelay.TotalSeconds)); - conn = CreateNewConnection(domainName, globalCatalog, skipCache); - if (conn == null) - { - _log.LogError("Unable to create replacement ldap connection for ServerDown exception. Breaking loop"); - yield break; - } - - _log.LogInformation("Created new LDAP connection after receiving ServerDown from server"); - continue; - } - catch (LdapException le) - { - if (le.ErrorCode != 82) - if (throwException) - throw new LDAPQueryException( - $"LDAP Exception in Loop: {le.ErrorCode}. {le.ServerErrorMessage}. {le.Message}. Filter: {ldapFilter}. Domain: {domainName}", - le); - else - _log.LogWarning(le, - "LDAP Exception in Loop: {ErrorCode}. {ServerErrorMessage}. {Message}. Filter: {Filter}. Domain: {Domain}", - le.ErrorCode, le.ServerErrorMessage, le.Message, ldapFilter, domainName); - yield break; - } - catch (Exception e) - { - if (throwException) - throw new LDAPQueryException( - $"Exception in LDAP loop for {ldapFilter} and {domainName ?? "Default Domain"}", e); - - _log.LogWarning(e, "Exception in LDAP loop for {Filter} and {Domain}", ldapFilter, - domainName ?? "Default Domain"); - yield break; - } - - if (response == null || pageResponse == null) continue; - - if (response.Entries == null) - yield break; - - foreach (SearchResultEntry entry in response.Entries) - yield return new SearchResultEntryWrapper(entry, this); - - if (pageResponse.Cookie.Length == 0 || response.Entries.Count == 0) - yield break; + return QueryLDAP(ldapFilter, scope, props, new CancellationToken(), domainName, includeAcl, showDeleted, + adsPath, globalCatalog, skipCache, throwException); + } - pageControl.Cookie = pageResponse.Cookie; - } + private static TimeSpan GetNextBackoff(int retryCount) + { + return TimeSpan.FromSeconds(Math.Min( + BackoffDelayMultiplier * (retryCount + 1) * retryCount, + MaxBackoffDelay.TotalSeconds)); } /// @@ -1560,8 +1502,11 @@ private async Task CreateLDAPConnection(string domainName = null connection.AuthType = authType; - if (!skipCache) - _ldapConnections.TryAdd(targetServer, connection); + _ldapConnections.AddOrUpdate(targetServer, connection, (s, ldapConnection) => + { + ldapConnection.Dispose(); + return connection; + }); return connection; } From bba535d777dd9e3a30c84787020e5fba1d9c9dab Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 2 Feb 2024 14:10:09 -0500 Subject: [PATCH 02/11] chore: add some comments --- src/CommonLib/LDAPUtils.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index 5c76a8c1..594e9de9 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -873,7 +873,15 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope }catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.ServerDown && retryCount < MaxRetries) { + /*A ServerDown exception indicates that our connection is no longer valid for one of many reasons. + However, this function is generally called by multiple threads, so we need to be careful in recreating + the connection. Using a semaphore, we can ensure that only one thread is actually recreating the connection + while the other threads that hit the ServerDown exception simply wait. The initial caller will hold the semaphore + and do a backoff delay before trying to make a new connection which will replace the existing connection in the + _ldapConnections cache. Other threads will retrieve the new connection from the cache instead of making a new one + This minimizes overhead of new connections while still fixing our core problem*/ var isSemaphoreHeld = _semaphoreSlim.CurrentCount == 0; + //Always increment retry count retryCount++; _semaphoreSlim.Wait(cancellationToken); @@ -881,8 +889,11 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope { if (!isSemaphoreHeld) { + //If no one is holding this semaphore, we're the first entrant into this logic, so its our responsibility + //to make the new LDAP connection Thread.Sleep(backoffDelay); backoffDelay = GetNextBackoff(retryCount); + //Explicitly skip the cache so we don't get the same connection back conn = CreateNewConnection(domainName, globalCatalog, true); if (conn == null) { @@ -895,12 +906,14 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope } else { + //If the semaphore is already held, we're just waiting until we get the semaphore, at which point a new connection should be available backoffDelay = GetNextBackoff(retryCount); conn = CreateNewConnection(domainName, globalCatalog); } } finally { + //Always release _semaphoreSlim.Release(); } continue; From 514a27cdf12e2fc2ed8bb0aa0a572feb43e725b9 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 2 Feb 2024 14:53:27 -0500 Subject: [PATCH 03/11] chore: DRY --- src/CommonLib/LDAPUtils.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index 594e9de9..c0e2ace3 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -892,7 +892,6 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope //If no one is holding this semaphore, we're the first entrant into this logic, so its our responsibility //to make the new LDAP connection Thread.Sleep(backoffDelay); - backoffDelay = GetNextBackoff(retryCount); //Explicitly skip the cache so we don't get the same connection back conn = CreateNewConnection(domainName, globalCatalog, true); if (conn == null) @@ -907,12 +906,12 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope else { //If the semaphore is already held, we're just waiting until we get the semaphore, at which point a new connection should be available - backoffDelay = GetNextBackoff(retryCount); conn = CreateNewConnection(domainName, globalCatalog); } } finally { + backoffDelay = GetNextBackoff(retryCount); //Always release _semaphoreSlim.Release(); } From e12785f91bb0645264eeb0a9144b4773c64eea8e Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 2 Feb 2024 15:06:43 -0500 Subject: [PATCH 04/11] chore: more comment --- src/CommonLib/LDAPUtils.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index c0e2ace3..cd9851ca 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -879,7 +879,9 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope while the other threads that hit the ServerDown exception simply wait. The initial caller will hold the semaphore and do a backoff delay before trying to make a new connection which will replace the existing connection in the _ldapConnections cache. Other threads will retrieve the new connection from the cache instead of making a new one - This minimizes overhead of new connections while still fixing our core problem*/ + This minimizes overhead of new connections while still fixing our core problem. + + A CurrentCount of 0 indicates the semaphore is already held because c# semaphores are weird and backwards*/ var isSemaphoreHeld = _semaphoreSlim.CurrentCount == 0; //Always increment retry count retryCount++; From 81498cf358a9d73689dfef2db165f5d04dc9233c Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 10:25:41 -0500 Subject: [PATCH 05/11] chore: simplify backoff delay --- src/CommonLib/LDAPUtils.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index cd9851ca..eec9b3c3 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -1026,7 +1026,7 @@ public virtual IEnumerable QueryLDAP(string ldapFilter, Sear private static TimeSpan GetNextBackoff(int retryCount) { return TimeSpan.FromSeconds(Math.Min( - BackoffDelayMultiplier * (retryCount + 1) * retryCount, + MinBackoffDelay.TotalSeconds * Math.Pow(BackoffDelayMultiplier, retryCount), MaxBackoffDelay.TotalSeconds)); } From 1e0614285895e2eb5b50bd17c4d2e2122d30da08 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 10:55:09 -0500 Subject: [PATCH 06/11] fix: replace semaphore with manual reset event --- src/CommonLib/LDAPUtils.cs | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index eec9b3c3..3242325a 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -53,7 +53,7 @@ private static readonly ConcurrentDictionary private readonly ConcurrentDictionary _domainControllerCache = new(); private static readonly TimeSpan MinBackoffDelay = TimeSpan.FromSeconds(2); private static readonly TimeSpan MaxBackoffDelay = TimeSpan.FromSeconds(20); - private static readonly int BackoffDelayMultiplier = 2; + private const int BackoffDelayMultiplier = 2; private const int MaxRetries = 3; private readonly ConcurrentDictionary _globalCatalogConnections = new(); @@ -65,7 +65,8 @@ private static readonly ConcurrentDictionary private readonly ConcurrentDictionary _netbiosCache = new(); private readonly PortScanner _portScanner; private LDAPConfig _ldapConfig = new(); - private readonly SemaphoreSlim _semaphoreSlim = new(1, 1); + private readonly ManualResetEvent _manualResetEvent = new(false); + /// /// Creates a new instance of LDAP Utils with defaults @@ -879,17 +880,16 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope while the other threads that hit the ServerDown exception simply wait. The initial caller will hold the semaphore and do a backoff delay before trying to make a new connection which will replace the existing connection in the _ldapConnections cache. Other threads will retrieve the new connection from the cache instead of making a new one - This minimizes overhead of new connections while still fixing our core problem. + This minimizes overhead of new connections while still fixing our core problem.*/ + - A CurrentCount of 0 indicates the semaphore is already held because c# semaphores are weird and backwards*/ - var isSemaphoreHeld = _semaphoreSlim.CurrentCount == 0; //Always increment retry count retryCount++; - - _semaphoreSlim.Wait(cancellationToken); - try + + //If we are the holders of the reset event, we need to do logic to reset the connection + if (_manualResetEvent.Reset()) { - if (!isSemaphoreHeld) + try { //If no one is holding this semaphore, we're the first entrant into this logic, so its our responsibility //to make the new LDAP connection @@ -905,18 +905,19 @@ A CurrentCount of 0 indicates the semaphore is already held because c# semaphore _log.LogInformation("Created new LDAP connection after receiving ServerDown from server"); } - else + finally { - //If the semaphore is already held, we're just waiting until we get the semaphore, at which point a new connection should be available - conn = CreateNewConnection(domainName, globalCatalog); + _manualResetEvent.Set(); } } - finally + else { - backoffDelay = GetNextBackoff(retryCount); - //Always release - _semaphoreSlim.Release(); + //If someone else is holding the reset event, we want to just wait and then pull the newly created connection out of the cache + _manualResetEvent.WaitOne(); + conn = CreateNewConnection(domainName, globalCatalog); } + + backoffDelay = GetNextBackoff(retryCount); continue; }catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.Busy && retryCount < MaxRetries) { retryCount++; From 765bf91d85182ba145b10cc2999cb810c3729734 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 10:56:17 -0500 Subject: [PATCH 07/11] chore: bad import --- src/CommonLib/LDAPUtils.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index 3242325a..9576e783 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -5,7 +5,6 @@ using System.DirectoryServices; using System.DirectoryServices.ActiveDirectory; using System.DirectoryServices.Protocols; -using System.IO.Compression; using System.Linq; using System.Net; using System.Net.Sockets; From dee2f881aa2d080a3e649c1b8b35fb8a73ddf714 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 10:57:14 -0500 Subject: [PATCH 08/11] chore: rename var for readability --- src/CommonLib/LDAPUtils.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index 9576e783..bf88e674 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -64,7 +64,7 @@ private static readonly ConcurrentDictionary private readonly ConcurrentDictionary _netbiosCache = new(); private readonly PortScanner _portScanner; private LDAPConfig _ldapConfig = new(); - private readonly ManualResetEvent _manualResetEvent = new(false); + private readonly ManualResetEvent _connectionResetEvent = new(false); /// @@ -886,7 +886,7 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope retryCount++; //If we are the holders of the reset event, we need to do logic to reset the connection - if (_manualResetEvent.Reset()) + if (_connectionResetEvent.Reset()) { try { @@ -906,13 +906,13 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope } finally { - _manualResetEvent.Set(); + _connectionResetEvent.Set(); } } else { //If someone else is holding the reset event, we want to just wait and then pull the newly created connection out of the cache - _manualResetEvent.WaitOne(); + _connectionResetEvent.WaitOne(); conn = CreateNewConnection(domainName, globalCatalog); } From 7d7e669f61083f43f65306e3d56eef676c4abe8e Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 12:07:45 -0500 Subject: [PATCH 09/11] chore: delete old comment --- src/CommonLib/LDAPUtils.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index bf88e674..a547244c 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -890,8 +890,6 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope { try { - //If no one is holding this semaphore, we're the first entrant into this logic, so its our responsibility - //to make the new LDAP connection Thread.Sleep(backoffDelay); //Explicitly skip the cache so we don't get the same connection back conn = CreateNewConnection(domainName, globalCatalog, true); From efa6a28b1d8a7eaa1b5d2304445ddb5d51136bdd Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 13:27:01 -0500 Subject: [PATCH 10/11] fix: fix retry logic again --- src/CommonLib/LDAPUtils.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index a547244c..febcdc8f 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -65,6 +65,7 @@ private static readonly ConcurrentDictionary private readonly PortScanner _portScanner; private LDAPConfig _ldapConfig = new(); private readonly ManualResetEvent _connectionResetEvent = new(false); + private readonly object _lockObj = new(); /// @@ -884,12 +885,15 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope //Always increment retry count retryCount++; - - //If we are the holders of the reset event, we need to do logic to reset the connection - if (_connectionResetEvent.Reset()) + + //Attempt to acquire a lock + if (Monitor.TryEnter(_lockObj)) { + //If we've acquired the lock, we want to immediately signal our reset event so everyone else waits + _connectionResetEvent.Reset(); try { + //Sleep for our backoff Thread.Sleep(backoffDelay); //Explicitly skip the cache so we don't get the same connection back conn = CreateNewConnection(domainName, globalCatalog, true); @@ -904,12 +908,15 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope } finally { + //Reset our event + release the lock _connectionResetEvent.Set(); + Monitor.Exit(_lockObj); } } else { //If someone else is holding the reset event, we want to just wait and then pull the newly created connection out of the cache + //This event will be released after the first entrant thread is done making a new connection _connectionResetEvent.WaitOne(); conn = CreateNewConnection(domainName, globalCatalog); } From 42f08054648a556c5af7579a883e7cdf83c1acbc Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 6 Feb 2024 13:34:45 -0500 Subject: [PATCH 11/11] chore: run formatter, add a small sleep to prevent race --- src/CommonLib/LDAPUtils.cs | 79 +++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 27 deletions(-) diff --git a/src/CommonLib/LDAPUtils.cs b/src/CommonLib/LDAPUtils.cs index febcdc8f..29ba263d 100644 --- a/src/CommonLib/LDAPUtils.cs +++ b/src/CommonLib/LDAPUtils.cs @@ -66,7 +66,7 @@ private static readonly ConcurrentDictionary private LDAPConfig _ldapConfig = new(); private readonly ManualResetEvent _connectionResetEvent = new(false); private readonly object _lockObj = new(); - + /// /// Creates a new instance of LDAP Utils with defaults @@ -104,11 +104,13 @@ public void SetLDAPConfig(LDAPConfig config) { kv.Value.Dispose(); } + _globalCatalogConnections.Clear(); foreach (var kv in _ldapConnections) { kv.Value.Dispose(); } + _ldapConnections.Clear(); } @@ -232,28 +234,35 @@ public TypedPrincipal ResolveIDAndType(string id, string fallbackDomain) return new TypedPrincipal(id, type); } - public TypedPrincipal ResolveCertTemplateByProperty(string propValue, string propertyName, string containerDN, string domainName) + public TypedPrincipal ResolveCertTemplateByProperty(string propValue, string propertyName, string containerDN, + string domainName) { var filter = new LDAPFilter().AddCertificateTemplates().AddFilter(propertyName + "=" + propValue, true); var res = QueryLDAP(filter.GetFilter(), SearchScope.OneLevel, - CommonProperties.TypeResolutionProps, adsPath: containerDN, domainName: domainName); + CommonProperties.TypeResolutionProps, adsPath: containerDN, domainName: domainName); if (res == null) { - _log.LogWarning("Could not find certificate template with '{propertyName}:{propValue}' under {containerDN}; null result", propertyName, propValue, containerDN); + _log.LogWarning( + "Could not find certificate template with '{propertyName}:{propValue}' under {containerDN}; null result", + propertyName, propValue, containerDN); return null; } List resList = new List(res); if (resList.Count == 0) { - _log.LogWarning("Could not find certificate template with '{propertyName}:{propValue}' under {containerDN}; empty list", propertyName, propValue, containerDN); + _log.LogWarning( + "Could not find certificate template with '{propertyName}:{propValue}' under {containerDN}; empty list", + propertyName, propValue, containerDN); return null; } if (resList.Count > 1) { - _log.LogWarning("Found more than one certificate template with '{propertyName}:{propValue}' under {containerDN}", propertyName, propValue, containerDN); + _log.LogWarning( + "Found more than one certificate template with '{propertyName}:{propValue}' under {containerDN}", + propertyName, propValue, containerDN); return null; } @@ -523,7 +532,8 @@ public IEnumerable DoRangedRetrieval(string distinguishedName, string at } catch (Exception e) { - _log.LogError(e, "Error doing ranged retrieval for {Attribute} on {Dn}", attributeName, distinguishedName); + _log.LogError(e, "Error doing ranged retrieval for {Attribute} on {Dn}", attributeName, + distinguishedName); yield break; } @@ -847,7 +857,8 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope if (queryParams.Exception != null) { _log.LogWarning("Failed to setup LDAP Query Filter: {Message}", queryParams.Exception.Message); - if (throwException) throw new LDAPQueryException("Failed to setup LDAP Query Filter", queryParams.Exception); + if (throwException) + throw new LDAPQueryException("Failed to setup LDAP Query Filter", queryParams.Exception); yield break; } @@ -871,21 +882,21 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope if (response != null) pageResponse = (PageResultResponseControl)response.Controls .Where(x => x is PageResultResponseControl).DefaultIfEmpty(null).FirstOrDefault(); - }catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.ServerDown && - retryCount < MaxRetries) + } + catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.ServerDown && + retryCount < MaxRetries) { /*A ServerDown exception indicates that our connection is no longer valid for one of many reasons. However, this function is generally called by multiple threads, so we need to be careful in recreating the connection. Using a semaphore, we can ensure that only one thread is actually recreating the connection while the other threads that hit the ServerDown exception simply wait. The initial caller will hold the semaphore - and do a backoff delay before trying to make a new connection which will replace the existing connection in the + and do a backoff delay before trying to make a new connection which will replace the existing connection in the _ldapConnections cache. Other threads will retrieve the new connection from the cache instead of making a new one This minimizes overhead of new connections while still fixing our core problem.*/ - //Always increment retry count retryCount++; - + //Attempt to acquire a lock if (Monitor.TryEnter(_lockObj)) { @@ -917,13 +928,17 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope { //If someone else is holding the reset event, we want to just wait and then pull the newly created connection out of the cache //This event will be released after the first entrant thread is done making a new connection + //The thread.sleep is to prevent a potential, very unlikely race + Thread.Sleep(50); _connectionResetEvent.WaitOne(); conn = CreateNewConnection(domainName, globalCatalog); } - + backoffDelay = GetNextBackoff(retryCount); continue; - }catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.Busy && retryCount < MaxRetries) { + } + catch (LdapException le) when (le.ErrorCode == (int)LdapErrorCodes.Busy && retryCount < MaxRetries) + { retryCount++; backoffDelay = GetNextBackoff(retryCount); continue; @@ -950,7 +965,7 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope $"LDAP Exception in Loop: {le.ErrorCode}. {le.ServerErrorMessage}. {le.Message}. Filter: {ldapFilter}. Domain: {domainName}", le); } - + yield break; } catch (Exception e) @@ -984,7 +999,8 @@ public IEnumerable QueryLDAP(string ldapFilter, SearchScope } } - private LdapConnection CreateNewConnection(string domainName = null, bool globalCatalog = false, bool skipCache = false) + private LdapConnection CreateNewConnection(string domainName = null, bool globalCatalog = false, + bool skipCache = false) { var task = globalCatalog ? Task.Run(() => CreateGlobalCatalogConnection(domainName, _ldapConfig.AuthType)) @@ -1394,7 +1410,8 @@ private SearchRequest CreateSearchRequest(string filter, SearchScope scope, stri var domain = GetDomain(domainName)?.Name ?? domainName; if (domain == null) - throw new LDAPQueryException($"Unable to create search request: GetDomain call failed for {domainName}"); + throw new LDAPQueryException( + $"Unable to create search request: GetDomain call failed for {domainName}"); var adPath = adsPath?.Replace("LDAP://", "") ?? $"DC={domain.Replace(".", ",DC=")}"; @@ -1425,7 +1442,9 @@ private async Task CreateGlobalCatalogConnection(string domainNa var domain = GetDomain(domainName); if (domain == null) { - _log.LogDebug("Unable to create global catalog connection for domain {DomainName}: GetDomain failed", domainName); + _log.LogDebug( + "Unable to create global catalog connection for domain {DomainName}: GetDomain failed", + domainName); throw new LDAPQueryException($"GetDomain call failed for {domainName}"); } @@ -1479,8 +1498,10 @@ private async Task CreateLDAPConnection(string domainName = null var domain = GetDomain(domainName); if (domain == null) { - _log.LogDebug("Unable to create ldap connection for domain {DomainName}: GetDomain failed", domainName); - throw new LDAPQueryException($"Error creating LDAP connection: GetDomain call failed for {domainName}"); + _log.LogDebug("Unable to create ldap connection for domain {DomainName}: GetDomain failed", + domainName); + throw new LDAPQueryException( + $"Error creating LDAP connection: GetDomain call failed for {domainName}"); } if (!_domainControllerCache.TryGetValue(domain.Name, out targetServer)) @@ -1623,7 +1644,8 @@ public int GetDomainRangeSize(string domainName = null, int defaultRangeSize = 7 //Default to a page size of 750 for safety if (domainPath == null) { - _log.LogDebug("Unable to resolve domain {Domain} to distinguishedname to get page size", domainName ?? "current domain"); + _log.LogDebug("Unable to resolve domain {Domain} to distinguishedname to get page size", + domainName ?? "current domain"); return defaultRangeSize; } @@ -1635,7 +1657,8 @@ public int GetDomainRangeSize(string domainName = null, int defaultRangeSize = 7 var configPath = CommonPaths.CreateDNPath(CommonPaths.QueryPolicyPath, domainPath); var enumerable = QueryLDAP("(objectclass=*)", SearchScope.Base, null, adsPath: configPath); var config = enumerable.DefaultIfEmpty(null).FirstOrDefault(); - var pageSize = config?.GetArrayProperty(LDAPProperties.LdapAdminLimits).FirstOrDefault(x => x.StartsWith("MaxPageSize", StringComparison.OrdinalIgnoreCase)); + var pageSize = config?.GetArrayProperty(LDAPProperties.LdapAdminLimits) + .FirstOrDefault(x => x.StartsWith("MaxPageSize", StringComparison.OrdinalIgnoreCase)); if (pageSize == null) { _log.LogDebug("No LDAPAdminLimits object found for {Domain}", domainName); @@ -1646,7 +1669,8 @@ public int GetDomainRangeSize(string domainName = null, int defaultRangeSize = 7 if (int.TryParse(pageSize.Split('=').Last(), out parsedPageSize)) { _ldapRangeSizeCache.TryAdd(domainPath.ToUpper(), parsedPageSize); - _log.LogInformation("Found page size {PageSize} for {Domain}", parsedPageSize, domainName ?? "current domain"); + _log.LogInformation("Found page size {PageSize} for {Domain}", parsedPageSize, + domainName ?? "current domain"); return parsedPageSize; } @@ -1700,12 +1724,13 @@ public string GetSchemaPath(string domainName) public bool IsDomainController(string computerObjectId, string domainName) { - var filter = new LDAPFilter().AddFilter(LDAPProperties.ObjectSID + "=" + computerObjectId, true).AddFilter(CommonFilters.DomainControllers, true); + var filter = new LDAPFilter().AddFilter(LDAPProperties.ObjectSID + "=" + computerObjectId, true) + .AddFilter(CommonFilters.DomainControllers, true); var res = QueryLDAP(filter.GetFilter(), SearchScope.Subtree, - CommonProperties.ObjectID, domainName: domainName); + CommonProperties.ObjectID, domainName: domainName); if (res.Count() > 0) return true; return false; } } -} +} \ No newline at end of file