[BED-6389] Add retry logic to GetDomains to make more resilient to network hiccups #242
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds decorrelated-jitter backoff and retry overloads in Helpers; introduces thread-safe RandomUtils; centralizes LDAP domain exclusion in LdapUtils and updates LdapConnectionPool to use it; adds unit tests for jitter bounds and adjusts LDAP pool tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant H as Helpers
participant R as RandomUtils
rect rgba(230,240,255,0.6)
note over C,H: Retry with decorrelated jitter backoff
C->>H: RetryOnException<T>(action, retryCount, baseDelay?, maxDelay?)
loop attempts up to retryCount
H->>C: invoke action()
alt action throws T
H->>H: BackoffWithDecorrelatedJitter(attempt, baseDelay, maxDelay)
H->>R: request random number
R-->>H: random value
H-->>C: await Task.Delay(jitteredDelay)
else other exception
H-->>C: rethrow
end
end
H-->>C: success or final throw
end
sequenceDiagram
autonumber
participant P as LdapConnectionPool
participant U as LdapUtils
participant AD as Domain APIs
participant H as Helpers
rect rgba(240,255,240,0.6)
note over P,U: Centralized domain exclusion & retry
P->>U: IsExcludedDomain(id)?
alt excluded
U-->>P: true
P-->>P: skip resolution/connection
else not excluded
U-->>P: false
P->>U: GetDomain(...)
U->>H: RetryOnException<ActiveDirectoryObjectNotFoundException, Domain>(...)
H->>AD: Domain.GetDomain(context)
alt success
AD-->>H: Domain
H-->>U: Domain
U-->>P: Domain
P-->>P: proceed
else failure after retries
H-->>U: throw
U->>U: AddExcludedDomain(id)
U-->>P: failure
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
… should exclude domains after retry failure.
…his is where this occurs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CommonLib/LdapConnectionPool.cs (1)
758-762: Dispose leaks global catalog connections.Dispose drains only _connections; _globalCatalogConnection wrappers remain undisposed.
Apply this diff:
public void Dispose() { - while (_connections.TryTake(out var wrapper)) { - wrapper.Connection.Dispose(); - } + while (_connections.TryTake(out var wrapper)) + wrapper.Connection.Dispose(); + while (_globalCatalogConnection.TryTake(out var gcWrapper)) + gcWrapper.Connection.Dispose(); }
🧹 Nitpick comments (10)
src/CommonLib/LdapConnectionPool.cs (2)
813-815: Be cautious with permanent exclusion on a single GetDomain miss.A transient resolver hiccup will blacklist the domain for the job. Consider a TTL‑based exclusion or requiring repeated failures before exclusion.
617-621: Unify backoff with Helpers.BackoffWithDecorrelatedJitter for consistency.This class rolled its own exponential backoff; prefer the new jittered helper to avoid thundering herd.
Apply this diff:
private static TimeSpan GetNextBackoff(int retryCount) { - return TimeSpan.FromSeconds(Math.min( - MinBackoffDelay.TotalSeconds * Math.Pow(BackoffDelayMultiplier, retryCount), - MaxBackoffDelay.TotalSeconds)); + return Helpers.BackoffWithDecorrelatedJitter(retryCount, MinBackoffDelay, MaxBackoffDelay); }test/unit/LdapConnectionPoolTest.cs (1)
11-21: Strengthen the assertion to verify exclusion state.Also assert that the domain is now in the exclusion set to validate the side‑effect.
Apply this change:
var connectAttempt = await connectionPool.TestDomainConnection("excludedDomain.com", false); Assert.False(connectAttempt.Success); Assert.Contains("excluded for connection attempt", connectAttempt.Message); + Assert.True(SharpHoundCommonLib.LdapUtils.IsExcludedDomain("excludedDomain.com"));src/CommonLib/Helpers.cs (2)
323-332: Avoid Math.Pow for 2^attempt; use integer shifts with saturation.Removes floating‑point exponent cost and avoids inf values until clamping.
Apply this diff:
- // Decorrelated Jitter Backoff - see https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ - var temp = Math.Min(maxDelay.Ticks, baseDelay.Ticks * Math.Pow(2, attempt)); + // Decorrelated Jitter Backoff - see https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ + // Compute 2^attempt using integer shifts with saturation. + var exp2 = attempt >= 62 ? double.PositiveInfinity : (double)(1L << attempt); + var temp = Math.Min(maxDelay.Ticks, baseDelay.Ticks * exp2); temp = temp / 2 + RandomUtils.Between(0, temp / 2); var ticksToDelay = Math.Min(maxDelay.Ticks, RandomUtils.Between(baseDelay.Ticks, temp * 3));
340-360: Optional: allow cancellation during backoff waits.Accept a CancellationToken and pass it to Task.Delay to make retries cancel‑friendly.
Would you like me to add overloads:
- RetryOnException(Func, int, TimeSpan? baseDelay = null, TimeSpan? maxDelay = null, ILogger logger = null, CancellationToken ct = default)
- RetryOnException<T,U>(Func, int, TimeSpan? baseDelay = null, TimeSpan? maxDelay = null, ILogger logger = null, CancellationToken ct = default)
and use ct in Task.Delay?src/CommonLib/LdapUtils.cs (5)
512-513: Retry the right exception type and wire logging into retry attempts.ActiveDirectoryObjectNotFoundException is not a transient “network hiccup.” Domain.GetDomain typically throws ActiveDirectoryServerDownException (and other ActiveDirectoryOperationException variants) for connectivity issues. Also pass a logger to capture retry attempts.
Apply this diff at all three call sites:
- domain = Helpers.RetryOnException<ActiveDirectoryObjectNotFoundException, Domain>(() => Domain.GetDomain(context), 2).GetAwaiter().GetResult(); + domain = Helpers.RetryOnException<ActiveDirectoryServerDownException, Domain>( + () => Domain.GetDomain(context), + 2, + logger: _log + ).GetAwaiter().GetResult();- domain = Helpers.RetryOnException<ActiveDirectoryObjectNotFoundException, Domain>(() => Domain.GetDomain(context), 2).GetAwaiter().GetResult(); + domain = Helpers.RetryOnException<ActiveDirectoryServerDownException, Domain>( + () => Domain.GetDomain(context), + 2, + logger: Logging.Logger + ).GetAwaiter().GetResult();- domain = Helpers.RetryOnException<ActiveDirectoryObjectNotFoundException, Domain>(() => Domain.GetDomain(context), 2).GetAwaiter().GetResult(); + domain = Helpers.RetryOnException<ActiveDirectoryServerDownException, Domain>( + () => Domain.GetDomain(context), + 2, + logger: _log + ).GetAwaiter().GetResult();Also applies to: 547-548, 580-581
553-558: Log the exception object, not just the message.You’re discarding stack details; prefer the ILogger overload with Exception.
Apply this diff:
- Logging.Logger.LogDebug("Static GetDomain call failed, adding to exclusion, for domain {DomainName}: {Error}", domainName, - e.Message); + Logging.Logger.LogDebug(e, "Static GetDomain call failed, adding to exclusion, for domain {DomainName}", domainName);
494-516: Consider mirroring exclusion semantics in the instance overload (optional).The TODO notes hint this; adding the same early-exit and null-safe cache key will keep behavior consistent with the static overload.
Apply this diff:
- public bool GetDomain(string domainName, out Domain domain) { - var cacheKey = domainName ?? _nullCacheKey; - if (_domainCache.TryGetValue(cacheKey, out domain)) return true; + public bool GetDomain(string domainName, out Domain domain) { + var cacheKey = domainName ?? _nullCacheKey; + if (_domainCache.TryGetValue(cacheKey, out domain)) return true; + if (IsExcludedDomain(domainName)) { + _log.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName ?? "<null>"); + domain = null; + return false; + } try { ... - _domainCache.TryAdd(cacheKey, domain); + _domainCache.TryAdd(cacheKey, domain); return true; } catch (Exception e) { _log.LogDebug(e, "GetDomain call failed for domain name {Name}", domainName); + // Optional: exclude only if a transient AD connectivity error was observed repeatedly elsewhere. + // AddExcludedDomain(domainName); domain = null; return false; } }
1146-1147: ResetUtils concurrency note.Reinitializing static sets is fine, but ResetUtils isn’t thread-safe. If called while lookups run, state can flip under readers. If used beyond tests, guard invocation or add a brief lock.
30-31: Normalize _domainCache key comparison (optional).Other caches/sets use OrdinalIgnoreCase; _domainCache uses default (case-sensitive), which can lead to duplicate entries per casing.
Apply this diff:
-private static ConcurrentDictionary<string, Domain> _domainCache = new(); +private static ConcurrentDictionary<string, Domain> _domainCache = new(StringComparer.OrdinalIgnoreCase);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/CommonLib/Helpers.cs(3 hunks)src/CommonLib/LdapConnectionPool.cs(4 hunks)src/CommonLib/LdapUtils.cs(7 hunks)src/CommonLib/RandomUtils.cs(1 hunks)test/unit/LdapConnectionPoolTest.cs(1 hunks)test/unit/TimeoutTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-26T16:59:06.972Z
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#217
File: src/CommonLib/LdapConnectionPool.cs:677-677
Timestamp: 2025-06-26T16:59:06.972Z
Learning: In SharpHoundCommonLib's LdapConnectionPool.cs, the team acknowledges that method `GetConnectionForSpecificServerAsync` has misleading naming - it contains "Async" in the name but is actually synchronous and blocks on async code, which they recognize as confusing but may need to live with due to backwards compatibility constraints.
Applied to files:
test/unit/LdapConnectionPoolTest.cssrc/CommonLib/LdapConnectionPool.cs
🧬 Code graph analysis (4)
src/CommonLib/LdapUtils.cs (3)
src/CommonLib/ConcurrentHashSet.cs (5)
ConcurrentHashSet(10-60)ConcurrentHashSet(13-15)ConcurrentHashSet(17-19)Contains(44-46)Add(26-28)src/CommonLib/Helpers.cs (1)
Helpers(18-382)src/CommonLib/Logging/Logging.cs (1)
Logging(7-20)
src/CommonLib/Helpers.cs (2)
src/CommonLib/LdapConnectionPool.cs (1)
TimeSpan(617-621)src/CommonLib/RandomUtils.cs (2)
RandomUtils(6-11)Between(10-10)
test/unit/TimeoutTests.cs (1)
src/CommonLib/Helpers.cs (2)
TimeSpan(323-332)Helpers(18-382)
src/CommonLib/LdapConnectionPool.cs (1)
src/CommonLib/LdapUtils.cs (5)
LdapUtils(28-1424)LdapUtils(79-84)LdapUtils(86-91)IsExcludedDomain(1161-1161)AddExcludedDomain(1162-1162)
🔇 Additional comments (3)
src/CommonLib/LdapConnectionPool.cs (2)
693-696: Centralized exclusion guard looks good.Early‑exit via LdapUtils.IsExcludedDomain prevents wasted work once a domain is excluded.
727-742: GC connection path consistently applies the same exclusion guard.Mirrors the non‑GC flow; good consistency.
test/unit/TimeoutTests.cs (1)
255-270: Good coverage of jitter bounds.Ranges exercise edge cases (reversed bounds, clamped max). Keep an eye on flakiness; 500 trials per case is fine but can be reduced if CI time is a concern.
| LdapUtils.AddExcludedDomain(_identifier); | ||
| } |
There was a problem hiding this comment.
Don’t blanket‑exclude the domain on any exception at the end of CreateNewConnection.
Catching Exception and permanently excluding can mask auth/config issues and reduce resiliency. Restrict exclusion to specific transient/network failures or remove it here and rely on upstream retry/strategy logic.
Apply this change:
- LdapUtils.AddExcludedDomain(_identifier);
+ // Avoid permanent exclusion on generic exceptions; rely on retries/strategies.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LdapUtils.AddExcludedDomain(_identifier); | |
| } | |
| // Avoid permanent exclusion on generic exceptions; rely on retries/strategies. | |
| } |
🤖 Prompt for AI Agents
In src/CommonLib/LdapConnectionPool.cs around lines 852-853, the code currently
catches all Exception and calls LdapUtils.AddExcludedDomain(_identifier), which
can hide auth/configuration errors and permanently blacklist domains; change
this to not blanket-exclude on any exception by replacing the broad catch with
targeted handling: catch only transient/network-related exceptions (e.g.,
LdapException with transient/error codes, SocketException, TimeoutException or
whatever concrete exceptions your LDAP client throws) and call
LdapUtils.AddExcludedDomain(_identifier) only in those catches; for other
exceptions rethrow after logging (or log and let upstream retry/strategy logic
handle it) and avoid swallowing Exception; ensure logging includes the exception
details and update any unit/integration tests to reflect the new behavior.
| public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domain domain) { | ||
| if (_domainCache.TryGetValue(domainName, out domain)) return true; | ||
| if (IsExcludedDomain(domainName)) { | ||
| Logging.Logger.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName); | ||
| return false; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Fix out parameter assignment and null-safe caching/exclusion in static GetDomain.
- Out param domain isn’t assigned on the early-return path, which will not compile.
- Null domainName will currently throw in _domainCache.TryGetValue and in exclusion check.
- Cache key should mirror the instance overload by using _nullCacheKey when domainName is null.
Apply this diff:
- public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domain domain) {
- if (_domainCache.TryGetValue(domainName, out domain)) return true;
- if (IsExcludedDomain(domainName)) {
- Logging.Logger.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName);
- return false;
- }
+ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domain domain) {
+ var cacheKey = domainName ?? _nullCacheKey;
+ if (_domainCache.TryGetValue(cacheKey, out domain)) return true;
+ if (IsExcludedDomain(domainName)) {
+ Logging.Logger.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName ?? "<null>");
+ domain = null;
+ return false;
+ }
...
- if (domain == null) return false;
- _domainCache.TryAdd(domainName, domain);
+ if (domain == null) return false;
+ _domainCache.TryAdd(cacheKey, domain);
return true;Also applies to: 549-550
🤖 Prompt for AI Agents
In src/CommonLib/LdapUtils.cs around lines 526-533 (and also apply same fix at
lines 549-550): create a local cacheKey = domainName ?? _nullCacheKey and use
that for _domainCache.TryGetValue so null won’t throw; assign the out parameter
domain = null before any early returns (e.g., when returning after TryGetValue
miss or when IsExcludedDomain indicates exclusion) and call IsExcludedDomain
with domainName in a null-safe way (or check for null before calling); ensure
the exclusion branch logs and then returns false with domain set to null so the
out param is always assigned.
| if (IsExcludedDomain(domainName)) { | ||
| Logging.Logger.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Nice: early exit on excluded domains.
Once null-safety and out-param assignment are applied, this will prevent thrashing unreachable domains.
To confirm no call sites pass null to the static overload (which would otherwise bypass exclusion semantics), run:
🏁 Script executed:
#!/bin/bash
# Find static calls where the first argument is null
rg -nP --type=cs -C2 '\bLdapUtils\.GetDomain\s*\(\s*null\s*,\s*'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) All occurrences of 'GetDomain(' in C# files:"
rg -n --type=cs '\bGetDomain\s*\(' -S -C2 || true
echo
echo "2) Occurrences of 'LdapUtils.GetDomain(' in C# files:"
rg -nP --type=cs '\bLdapUtils\.GetDomain\s*\(' -S -C2 || true
echo
echo "3) Calls to GetDomain(...) with a literal null as the first argument:"
rg -nP --type=cs '\b(?:LdapUtils\.)?GetDomain\s*\(\s*null\b' -S -C2 || true
echo
echo "4) Show src/CommonLib/LdapUtils.cs around the reported lines (480-560):"
if [ -f src/CommonLib/LdapUtils.cs ]; then
sed -n '480,560p' src/CommonLib/LdapUtils.cs || true
else
echo "src/CommonLib/LdapUtils.cs not found"
fiLength of output: 15200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find references/definitions of _domainCache and _nullCacheKey"
rg -n --type=cs '_domainCache' -S -n -C3 || true
echo
rg -n --type=cs '_nullCacheKey' -S -n -C3 || true
echo
echo "2) Find IsExcludedDomain / AddExcludedDomain"
rg -n --type=cs 'IsExcludedDomain\(' -S -n -C3 || true
echo
rg -n --type=cs 'AddExcludedDomain\(' -S -n -C3 || true
echo
echo "3) Print top of src/CommonLib/LdapUtils.cs (fields / static members)"
if [ -f src/CommonLib/LdapUtils.cs ]; then
sed -n '1,240p' src/CommonLib/LdapUtils.cs || true
else
echo "src/CommonLib/LdapUtils.cs not found"
fi
echo
echo "4) Show the IsExcludedDomain and AddExcludedDomain function bodies (if found)"
files=$(rg -n --type=cs 'IsExcludedDomain\(|AddExcludedDomain\(' -S --hidden --no-line-number --line-number | cut -d: -f1 | sort -u || true)
for f in $files; do
echo "---- $f ----"
rg -n --type=cs 'IsExcludedDomain\(|AddExcludedDomain\(' -n -C6 "$f" || true
sed -n '1,400p' "$f" | sed -n '1,400p' || true
doneLength of output: 65751
Make the static GetDomain null-safe and consistent with the instance overload
Static overload (src/CommonLib/LdapUtils.cs:526–531) calls _domainCache.TryGetValue(domainName, ...) and IsExcludedDomain(domainName) without guarding domainName; passing null will throw (ConcurrentDictionary disallows null keys) and also diverges from the instance GetDomain's use of _nullCacheKey. Normalize null handling (e.g. guard domainName, map null to a consistent null-cache key or delegate to the instance semantics) and ensure the out parameter is assigned on every return path.
Call sites to review: src/CommonLib/LdapConnectionPool.cs:635, 808; src/CommonLib/ConnectionPoolManager.cs:152.
| public static bool IsExcludedDomain(string domain) => _excludedDomains.Contains(domain); | ||
| public static void AddExcludedDomain(string domain) => _excludedDomains.Add(domain); | ||
|
|
There was a problem hiding this comment.
Guard exclusion helpers against null/empty inputs.
Current code will throw ArgumentNullException on null domain. Also avoid polluting the set with empty strings.
Apply this diff:
-public static bool IsExcludedDomain(string domain) => _excludedDomains.Contains(domain);
-public static void AddExcludedDomain(string domain) => _excludedDomains.Add(domain);
+public static bool IsExcludedDomain(string domain) =>
+ !string.IsNullOrWhiteSpace(domain) && _excludedDomains.Contains(domain);
+public static void AddExcludedDomain(string domain) {
+ if (!string.IsNullOrWhiteSpace(domain))
+ _excludedDomains.Add(domain);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static bool IsExcludedDomain(string domain) => _excludedDomains.Contains(domain); | |
| public static void AddExcludedDomain(string domain) => _excludedDomains.Add(domain); | |
| public static bool IsExcludedDomain(string domain) => !string.IsNullOrWhiteSpace(domain) && _excludedDomains.Contains(domain); | |
| public static void AddExcludedDomain(string domain) | |
| { | |
| if (!string.IsNullOrWhiteSpace(domain)) | |
| _excludedDomains.Add(domain); | |
| } |
🤖 Prompt for AI Agents
In src/CommonLib/LdapUtils.cs around lines 1161 to 1163, the helpers don’t guard
against null or empty domain values which causes ArgumentNullException and
allows empty strings into the set; update IsExcludedDomain to return false when
domain is null or empty (optionally trim) and update AddExcludedDomain to no-op
when domain is null or empty (optionally trim and normalize case before adding)
so the set is never polluted and calls are safe.
| private static readonly ThreadLocal<Random> Random = new(() => new Random()); | ||
|
|
||
| public static double NextDouble() => Random.Value.NextDouble(); | ||
| public static double Between(double minValue, double maxValue) => Random.Value.NextDouble() * (maxValue - minValue) + minValue; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Seed ThreadLocal securely and avoid symbol shadowing.
Multiple threads can get identical sequences with new Random(); also the field name Random shadows the type.
Apply this diff:
+using System.Security.Cryptography;
public static class RandomUtils {
- private static readonly ThreadLocal<Random> Random = new(() => new Random());
-
- public static double NextDouble() => Random.Value.NextDouble();
- public static double Between(double minValue, double maxValue) => Random.Value.NextDouble() * (maxValue - minValue) + minValue;
+ private static readonly ThreadLocal<Random> ThreadRandom = new(() => {
+ // Per‑thread, cryptographically seeded RNG to avoid duplicate sequences across threads.
+ Span<byte> buf = stackalloc byte[8];
+ RandomNumberGenerator.Fill(buf);
+ var seed = BitConverter.ToInt32(buf);
+ return new Random(seed);
+ });
+
+ public static double NextDouble() => ThreadRandom.Value.NextDouble();
+ public static double Between(double minValue, double maxValue) {
+ if (maxValue < minValue) (minValue, maxValue) = (maxValue, minValue);
+ return ThreadRandom.Value.NextDouble() * (maxValue - minValue) + minValue;
+ }
}🤖 Prompt for AI Agents
In src/CommonLib/RandomUtils.cs around lines 7 to 11, the ThreadLocal field is
named "Random" (shadowing the System.Random type) and each Random is constructed
with the default constructor which can produce identical sequences across
threads; change the field name to something like s_threadRandom to avoid
shadowing and seed each Random instance using a cryptographically-secure seed
(e.g., obtain an int seed from
System.Security.Cryptography.RandomNumberGenerator or
RandomNumberGenerator.GetInt32) when creating the ThreadLocal Random so each
thread gets a unique, secure seed; keep the API (NextDouble, Between) but have
them use s_threadRandom.Value.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/CommonLib/RandomUtils.cs (1)
6-9: Seed ThreadLocal securely and avoid type shadowing.Default-constructed Random can produce identical sequences across threads; the field name Random also shadows System.Random.
Apply this diff within the selected lines:
- public static class RandomUtils { - private static readonly ThreadLocal<Random> Random = new(() => new Random()); - - public static double NextDouble() => Random.Value.NextDouble(); + public static class RandomUtils { + private static readonly ThreadLocal<Random> s_threadRandom = new(() => + { + Span<byte> seedBytes = stackalloc byte[4]; + RandomNumberGenerator.Fill(seedBytes); + var seed = BitConverter.ToInt32(seedBytes); + return new Random(seed); + }); + + public static double NextDouble() => s_threadRandom.Value.NextDouble();Add this using outside the range (top of file):
using System.Security.Cryptography;
🧹 Nitpick comments (1)
src/CommonLib/RandomUtils.cs (1)
17-17: Between(double): handle inverted bounds.If maxValue < minValue, the current formula yields surprising results. Either normalize or throw.
Apply this diff:
- public static double Between(double minValue, double maxValue) => Random.Value.NextDouble() * (maxValue - minValue) + minValue; + public static double Between(double minValue, double maxValue) + { + if (maxValue < minValue) (minValue, maxValue) = (maxValue, minValue); + return s_threadRandom.Value.NextDouble() * (maxValue - minValue) + minValue; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CommonLib/Helpers.cs(3 hunks)src/CommonLib/RandomUtils.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CommonLib/Helpers.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
src/CommonLib/RandomUtils.cs
Outdated
| public static long NextLong() => LongRandom(long.MinValue, long.MaxValue); | ||
| private static long LongRandom(long min, long max) { | ||
| var buf = new byte[8]; | ||
| Random.Value.NextBytes(buf); | ||
| var longRand = BitConverter.ToInt64(buf, 0); | ||
| return (Math.Abs(longRand % (max - min)) + min); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
NextLong/LongRandom are broken: overflow and modulo bias.
- NextLong(long.MinValue, long.MaxValue) overflows at (max - min).
- Modulo + Math.Abs produces biased distribution and can still overflow on long.MinValue.
Replace NextLong and delete LongRandom:
- public static long NextLong() => LongRandom(long.MinValue, long.MaxValue);
- private static long LongRandom(long min, long max) {
- var buf = new byte[8];
- Random.Value.NextBytes(buf);
- var longRand = BitConverter.ToInt64(buf, 0);
- return (Math.Abs(longRand % (max - min)) + min);
- }
+ public static long NextLong()
+ {
+ // Inclusive min, exclusive max
+ return RandomNumberGenerator.GetInt64(long.MinValue, long.MaxValue);
+ }Note: Requires .NET 6+. If the project targets earlier TFMs, I can provide a rejection‑sampling fallback.
🏁 Script executed:
#!/bin/bash
# Verify target frameworks; expect at least net6.0
fd -t f -e csproj | while read -r f; do
echo "==> $f"
rg -nP 'TargetFrameworks?\s*<|<TargetFramework>' -n -N -H "$f" -n
doneLength of output: 479
Fix NextLong/LongRandom — overflow & modulo‑bias; use rejection sampling fallback (net472)
File: src/CommonLib/RandomUtils.cs (lines 10–16)
- Bug: (max - min) overflows for long.MinValue/long.MaxValue; Math.Abs(longRand % (max - min)) is biased and breaks for long.MinValue.
- Constraint: CommonLib targets net472 (src/CommonLib/SharpHoundCommonLib.csproj) so RandomNumberGenerator.GetInt64 (net6+) is unavailable.
Replace with a safe, TFM-compatible implementation (inclusive min, exclusive max):
// add at class scope
private static readonly System.Security.Cryptography.RandomNumberGenerator _rng = System.Security.Cryptography.RandomNumberGenerator.Create();
// public API
public static long NextLong() => NextLong(long.MinValue, long.MaxValue);
// range-aware, bias-free implementation
private static long NextLong(long minInclusive, long maxExclusive)
{
if (minInclusive >= maxExclusive) throw new ArgumentOutOfRangeException(nameof(minInclusive));
ulong uMin = unchecked((ulong)minInclusive);
ulong uMax = unchecked((ulong)maxExclusive);
ulong range = uMax - uMin; // > 0
var buf = new byte[8];
ulong rand;
// rejection sampling to avoid modulo bias
ulong limit = ulong.MaxValue - (ulong.MaxValue % range);
do
{
_rng.GetBytes(buf);
rand = BitConverter.ToUInt64(buf, 0);
} while (rand >= limit);
return (long)((rand % range) + uMin);
}Either apply the above fallback or upgrade the project to net6+ and use RandomNumberGenerator.GetInt64.
🤖 Prompt for AI Agents
In src/CommonLib/RandomUtils.cs around lines 10–16 replace the current
NextLong/LongRandom with a safe, TFM-compatible rejection-sampling
implementation: add a private static readonly
System.Security.Cryptography.RandomNumberGenerator _rng =
RandomNumberGenerator.Create() at class scope, change public NextLong to call
NextLong(long.MinValue, long.MaxValue) and implement a private NextLong(long
minInclusive, long maxExclusive) that validates min<max, casts min/max to
unchecked ulongs, computes range = uMax - uMin, computes limit = ulong.MaxValue
- (ulong.MaxValue % range), repeatedly fills an 8-byte buffer with _rng.GetBytes
until BitConverter.ToUInt64(buf,0) < limit, then returns (long)((rand % range) +
uMin); this avoids overflow and modulo bias and works on net472.
| return (Math.Abs(longRand % (max - min)) + min); | ||
| } | ||
| public static double Between(double minValue, double maxValue) => Random.Value.NextDouble() * (maxValue - minValue) + minValue; | ||
| public static long Between(long minValue, long maxValue) => LongRandom(minValue, maxValue); |
There was a problem hiding this comment.
Between(long): enforce valid range and remove bias.
Current implementation inherits LongRandom’s overflow/bias issues.
Apply this diff:
- public static long Between(long minValue, long maxValue) => LongRandom(minValue, maxValue);
+ public static long Between(long minValue, long maxValue)
+ {
+ if (maxValue <= minValue)
+ throw new ArgumentOutOfRangeException(nameof(maxValue), "maxValue must be greater than minValue.");
+ return RandomNumberGenerator.GetInt64(minValue, maxValue); // [min, max)
+ }Also consider documenting the inclusive/exclusive semantics ([min, max)) for callers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static long Between(long minValue, long maxValue) => LongRandom(minValue, maxValue); | |
| public static long Between(long minValue, long maxValue) | |
| { | |
| if (maxValue <= minValue) | |
| throw new ArgumentOutOfRangeException(nameof(maxValue), "maxValue must be greater than minValue."); | |
| return RandomNumberGenerator.GetInt64(minValue, maxValue); // [min, max) | |
| } |
🤖 Prompt for AI Agents
In src/CommonLib/RandomUtils.cs around line 18, replace the current one-liner
with a safe implementation that validates the range and avoids bias/overflow:
throw an ArgumentOutOfRangeException if minValue >= maxValue, then use an
unbiased provider such as
System.Security.Cryptography.RandomNumberGenerator.GetInt64(minValue, maxValue)
(or Random.Shared.NextInt64 when targeting a runtime that provides it) to return
a value in [min, max); and add a brief comment documenting the
inclusive/exclusive semantics ([min, max)) for callers.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CommonLib/LdapUtils.cs (1)
526-563: Fix null-safety and cache-key inconsistency in static GetDomain.Three issues prevent this overload from handling null
domainNamecorrectly:
- Line 528:
TryGetValue(domainName, ...)should usecacheKey(defined line 527) to match line 551'sTryAdd(cacheKey, ...). Otherwise, null domains bypass the cache.- Line 529:
IsExcludedDomain(domainName)will throwArgumentNullExceptionifdomainNameis null (ConcurrentHashSet/Dictionary disallow null keys).- Lines 1163–1164: The exclusion helpers don't guard against null/empty, so
AddExcludedDomain(cacheKey)on line 559 will throw ifcacheKeyrepresents a null domain (though_nullCacheKeyis a GUID and won't be null, the helpers should still guard against caller errors elsewhere).Apply this diff:
public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domain domain) { var cacheKey = domainName ?? _nullCacheKey; - if (_domainCache.TryGetValue(domainName, out domain)) return true; + if (_domainCache.TryGetValue(cacheKey, out domain)) return true; if (IsExcludedDomain(domainName)) { Logging.Logger.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName); domain = null; return false; } ... }And guard the exclusion helpers (see separate comment on lines 1163–1164).
🧹 Nitpick comments (2)
src/CommonLib/LdapUtils.cs (2)
494-524: Consider exclusion logic in instance overload for consistency.The static
GetDomain(string, LdapConfig, out Domain)adds failing domains to the exclusion list (line 559), but this instance overload does not. The comment on lines 518–519 notes that exclusion logic may be needed here. If a domain fails repeatedly, the instance method will retry on every invocation without learning from past failures, leading to wasted attempts and timeouts.Consider applying the same exclusion pattern here:
public bool GetDomain(string domainName, out Domain domain) { var cacheKey = domainName ?? _nullCacheKey; if (_domainCache.TryGetValue(cacheKey, out domain)) return true; + if (IsExcludedDomain(domainName)) { + _log.LogDebug("Domain: {DomainName} has been excluded for collection. Skipping", domainName); + domain = null; + return false; + } try { ... domain = Helpers.RetryOnException<ActiveDirectoryObjectNotFoundException, Domain>(() => Domain.GetDomain(context), 2).GetAwaiter().GetResult(); if (domain == null) return false; _domainCache.TryAdd(cacheKey, domain); return true; } catch (Exception e) { - // The Static GetDomain Function ran into an issue requiring to exclude a domain as it would continuously - // try to connect to a domain that it could not connect to. This method may also need the same logic. _log.LogDebug(e, "GetDomain call failed for domain name {Name}", domainName); + AddExcludedDomain(cacheKey); domain = null; return false; } }
572-593: Consider exclusion logic in parameterless overload for consistency.Same concern as the
GetDomain(string, out Domain)instance overload: this method does not exclude failing domains. The comment on lines 587–588 notes that exclusion logic may be needed. For consistency with the static overload, consider adding exclusion checks and updates.public bool GetDomain(out Domain domain) { if (_domainCache.TryGetValue(_nullCacheKey, out domain)) return true; + if (IsExcludedDomain(null)) { + _log.LogDebug("Domain: <null> has been excluded for collection. Skipping"); + domain = null; + return false; + } try { ... domain = Helpers.RetryOnException<ActiveDirectoryObjectNotFoundException, Domain>(() => Domain.GetDomain(context), 2).GetAwaiter().GetResult(); _domainCache.TryAdd(_nullCacheKey, domain); return true; } catch (Exception e) { - // The Static GetDomain Function ran into an issue requiring to exclude a domain as it would continuously - // try to connect to a domain that it could not connect to. This method may also need the same logic. _log.LogDebug(e, "GetDomain call failed for blank domain"); + AddExcludedDomain(_nullCacheKey); domain = null; return false; } }Note: This assumes
IsExcludedDomainandAddExcludedDomainare made null-safe (see comment on lines 1163–1164).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CommonLib/LdapUtils.cs(7 hunks)src/CommonLib/RandomUtils.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CommonLib/RandomUtils.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/CommonLib/LdapUtils.cs (3)
src/CommonLib/ConcurrentHashSet.cs (5)
ConcurrentHashSet(10-60)ConcurrentHashSet(13-15)ConcurrentHashSet(17-19)Contains(44-46)Add(26-28)src/CommonLib/Helpers.cs (1)
Helpers(18-382)src/CommonLib/Logging/Logging.cs (1)
Logging(7-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/CommonLib/LdapUtils.cs (3)
33-35: LGTM: Clean declaration of exclusion set.The case-insensitive comparer matches the design, and the comment clearly explains the intent.
1148-1148: LGTM: Exclusion state is properly reset.Reinitializing
_excludedDomainsinResetUtilsensures a clean slate, consistent with the other static caches.
1163-1164: Guard exclusion helpers against null/empty inputs.Both helpers will throw
ArgumentNullExceptionifdomainis null (ConcurrentHashSet disallows null keys). Also consider rejecting empty strings to avoid polluting the set.Apply this diff:
-public static bool IsExcludedDomain(string domain) => _excludedDomains.Contains(domain); -public static void AddExcludedDomain(string domain) => _excludedDomains.Add(domain); +public static bool IsExcludedDomain(string domain) => + !string.IsNullOrWhiteSpace(domain) && _excludedDomains.Contains(domain); +public static void AddExcludedDomain(string domain) { + if (!string.IsNullOrWhiteSpace(domain)) + _excludedDomains.Add(domain); +}This matches the past review comment and ensures call sites (e.g., line 529) won't throw on null.
2 participants
Description
Motivation and Context
When the static GetDomain call fails in CreateNewConnection, that domain is immediately added to the excluded domains list for the remainder of the job. Maybe this should be a little more forgiving.
https://specterops.atlassian.net/browse/BED-6389
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests