Skip to content

Commit 8ad304b

Browse files
definitelynotagoblinmykeeliumktstrader
authored
[BED-5971] Let's add timeouts to blocking ops (#217)
* chore: Update CheckPort to use ExecuteWithTimeout * feat: getting started putting more ExecuteWithTimeouts around * Correct ExecuteWithTimeout for simple Task functions * fix: added timeout wrapper to ScanHttpEndpoint in ScanAsync to cover calls to PerformNtlmAuthenticationAsync * feat: SHRegistry timeout and abstraction updates * feat: SmbScanner.SendAndRecieveData timeout * chore: move ExecuteWithTimeout functions to new class * chore: Fix DCRegistryProcessor, add more tests for ExecuteWithTimeout * chore: But this time actually add more tests for ExecuteWithTimeout * feat: Add new parent token cancelled result to ExecuteWithTimeout, appropriate unit tests * feat: add timeout to LdapUtils.GetDomain, lots of async plumbing * feat: add timeout for LdapConnection.SendRequest, more async plumbing * chore: polish up OpenSamServer; polish up LocalGroupProcessorTest * feat: add timeout wrapper to HttpNtlmAuthenticationService.EnsureRequiresAuth * chore: reverting wrapper under ScanAsync since we are adding timeout wrapper layers lower * fix: Return public functions to previous signatures pt 1: Tasks * chore: Marking External Calls * fix: Return public functions to previous signatures pt 2: out vars * feat: async LdapConnection.SendRequest implementation * chore: added more unit test cases for throwing TimeoutException * Make async tests without await sync * Build pipeline should only run on pull_request, not on push * chore: it can't be these tests causing Actions to hang, right? * fix: Test max parallel threads setting on xunit config * fix: No timeout around GetDomain, work to make RequestNETBIOSNameFromComputer more async * chore: Marking External Calls * chore: remove xunit.runner.json config; add warning to sync ExecuteWithTimeout methods * fix: a few new unit tests are throwing unexpected exceptions, let's address those in a bit * fix: removing LongRunning sync task hint to see how it affects xUnit parallel tests on Github runner * chore: testing LongRunning sync methods with non-parallel Timeout tests * chore: removing LongRunning for the time being * chore: remove unnecessary async from ConnectionPoolManager Query methods * Pass timeout token to PerformNtlmAuthenticationAsync * chore: Passing down more timeout tokens * fix: return OpenSamServer to previous contract * chore: Adding Names to Return Tuple for RequestNETBIOSNameFromComputerAsync * chore: remove an abstraction which probably causes more context switching than real value * chore: Move Success Log in PortScanner.CheckPort * chore: addressing coderabbit comments * fix: dang it is those same to no-host exceptions :( * chore: exposing async versions of public methods that we can use internally to follow the Task pattern * fix: try/catch _utils.GetDomain * Update test/unit/PortScannerTest.cs This seems reasonable let's give it a shot. I'm curious how CheckPort will respond to the non-routable IP Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Revert "Update test/unit/PortScannerTest.cs " This reverts commit b41bd62. --------- Co-authored-by: Michael Cuomo <[email protected]> Co-authored-by: Katie Strader <[email protected]>
1 parent 5f4762a commit 8ad304b

29 files changed

+1173
-729
lines changed

‎.github/workflows/build.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
name: Build
22

3-
on: [push, pull_request]
3+
on: pull_request
44

55
jobs:
66
build:

‎src/CommonLib/ConnectionPoolManager.cs‎

Lines changed: 34 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Collections.Concurrent;
33
using System.Collections.Generic;
44
using System.DirectoryServices;
5+
using System.Runtime.CompilerServices;
56
using System.Security.Principal;
67
using System.Threading;
78
using System.Threading.Tasks;
@@ -28,7 +29,8 @@ public IAsyncEnumerable<Result<string>> RangedRetrieval(string distinguishedName
2829
string attributeName, CancellationToken cancellationToken = new()) {
2930
var domain = Helpers.DistinguishedNameToDomain(distinguishedName);
3031

31-
if (!GetPool(domain, out var pool)) {
32+
var (getPoolSuccess, pool) = GetPool(domain);
33+
if (!getPoolSuccess) {
3234
return new List<Result<string>> {Result<string>.Fail("Failed to resolve a connection pool")}.ToAsyncEnumerable();
3335
}
3436

@@ -37,7 +39,8 @@ public IAsyncEnumerable<Result<string>> RangedRetrieval(string distinguishedName
3739

3840
public IAsyncEnumerable<LdapResult<IDirectoryObject>> PagedQuery(LdapQueryParameters queryParameters,
3941
CancellationToken cancellationToken = new()) {
40-
if (!GetPool(queryParameters.DomainName, out var pool)) {
42+
var (getPoolSuccess, pool) = GetPool(queryParameters.DomainName);
43+
if (!getPoolSuccess) {
4144
return new List<LdapResult<IDirectoryObject>> {LdapResult<IDirectoryObject>.Fail("Failed to resolve a connection pool", queryParameters)}.ToAsyncEnumerable();
4245
}
4346

@@ -46,7 +49,8 @@ public IAsyncEnumerable<LdapResult<IDirectoryObject>> PagedQuery(LdapQueryParame
4649

4750
public IAsyncEnumerable<LdapResult<IDirectoryObject>> Query(LdapQueryParameters queryParameters,
4851
CancellationToken cancellationToken = new()) {
49-
if (!GetPool(queryParameters.DomainName, out var pool)) {
52+
var (getPoolSuccess, pool) = GetPool(queryParameters.DomainName);
53+
if (!getPoolSuccess) {
5054
return new List<LdapResult<IDirectoryObject>> {LdapResult<IDirectoryObject>.Fail("Failed to resolve a connection pool", queryParameters)}.ToAsyncEnumerable();
5155
}
5256

@@ -73,24 +77,24 @@ public void ReleaseConnection(LdapConnectionWrapper connectionWrapper, bool conn
7377
return (success, message);
7478
}
7579

76-
private bool GetPool(string identifier, out LdapConnectionPool pool) {
80+
private (bool, LdapConnectionPool) GetPool(string identifier) {
7781
if (string.IsNullOrWhiteSpace(identifier)) {
78-
pool = default;
79-
return false;
82+
return (false, default);
8083
}
8184

8285
var resolved = ResolveIdentifier(identifier);
83-
if (!_pools.TryGetValue(resolved, out pool)) {
86+
if (!_pools.TryGetValue(resolved, out var pool)) {
8487
pool = new LdapConnectionPool(identifier, resolved, _ldapConfig, scanner: _portScanner);
8588
_pools.TryAdd(resolved, pool);
8689
}
8790

88-
return true;
91+
return (true, pool);
8992
}
9093

9194
public async Task<(bool Success, LdapConnectionWrapper ConnectionWrapper, string Message)> GetLdapConnection(
9295
string identifier, bool globalCatalog) {
93-
if (!GetPool(identifier, out var pool)) {
96+
var (getPoolSuccess, pool) = GetPool(identifier);
97+
if (!getPoolSuccess) {
9498
return (false, default, $"Unable to resolve a pool for {identifier}");
9599
}
96100

@@ -103,36 +107,42 @@ private bool GetPool(string identifier, out LdapConnectionPool pool) {
103107

104108
public (bool Success, LdapConnectionWrapper connectionWrapper, string Message) GetLdapConnectionForServer(
105109
string identifier, string server, bool globalCatalog) {
106-
if (!GetPool(identifier, out var pool)) {
110+
111+
return GetLdapConnectionForServerAsync(identifier, server, globalCatalog).GetAwaiter().GetResult();
112+
}
113+
114+
public async Task<(bool Success, LdapConnectionWrapper connectionWrapper, string Message)> GetLdapConnectionForServerAsync(
115+
string identifier, string server, bool globalCatalog) {
116+
var (getPoolSuccess, pool) = GetPool(identifier);
117+
if (!getPoolSuccess) {
107118
return (false, default, $"Unable to resolve a pool for {identifier}");
108119
}
109120

110-
return pool.GetConnectionForSpecificServerAsync(server, globalCatalog);
121+
return await pool.GetConnectionForSpecificServerActuallyAsync(server, globalCatalog);
111122
}
112123

113124
private string ResolveIdentifier(string identifier) {
114125
if (_resolvedIdentifiers.TryGetValue(identifier, out var resolved)) {
115126
return resolved;
116127
}
117-
118-
if (GetDomainSidFromDomainName(identifier, out var sid)) {
128+
129+
if (GetDomainSidFromDomainName(identifier) is (true, var sid)) {
119130
_log.LogDebug("Resolved identifier {Identifier} to {Resolved}", identifier, sid);
120131
_resolvedIdentifiers.TryAdd(identifier, sid);
121132
return sid;
122133
}
123-
134+
124135
return identifier;
125136
}
126137

127-
private bool GetDomainSidFromDomainName(string domainName, out string domainSid) {
128-
if (Cache.GetDomainSidMapping(domainName, out domainSid)) return true;
138+
private (bool, string) GetDomainSidFromDomainName(string domainName) {
139+
if (Cache.GetDomainSidMapping(domainName, out var domainSid)) return (true, domainSid);
129140

130141
try {
131142
var entry = new DirectoryEntry($"LDAP://{domainName}").ToDirectoryObject();
132143
if (entry.TryGetSecurityIdentifier(out var sid)) {
133144
Cache.AddDomainSidMapping(domainName, sid);
134-
domainSid = sid;
135-
return true;
145+
return (true, sid);
136146
}
137147
}
138148
catch {
@@ -141,9 +151,10 @@ private bool GetDomainSidFromDomainName(string domainName, out string domainSid)
141151

142152
if (LdapUtils.GetDomain(domainName, _ldapConfig, out var domainObject))
143153
try {
154+
// TODO: MC - Confirm GetDirectoryEntry is not a Blocking External Call
144155
if (domainObject.GetDirectoryEntry().ToDirectoryObject().TryGetSecurityIdentifier(out domainSid)) {
145156
Cache.AddDomainSidMapping(domainName, domainSid);
146-
return true;
157+
return (true, domainSid);
147158
}
148159
}
149160
catch {
@@ -153,16 +164,18 @@ private bool GetDomainSidFromDomainName(string domainName, out string domainSid)
153164
foreach (var name in _translateNames)
154165
try {
155166
var account = new NTAccount(domainName, name);
167+
// Blocking External Call
168+
// Calls Win32.LsaOpenPolicy and either Win32NativeLsaLookupNames2 or Win32Native.LsaLookupNames
156169
var sid = (SecurityIdentifier)account.Translate(typeof(SecurityIdentifier));
157170
domainSid = sid.AccountDomainSid.ToString();
158171
Cache.AddDomainSidMapping(domainName, domainSid);
159-
return true;
172+
return (true, domainSid);
160173
}
161174
catch {
162175
//We expect this to fail if the username doesn't exist in the domain
163176
}
164177

165-
return false;
178+
return (false, null);
166179
}
167180

168181
public void Dispose() {

‎src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@ public bool TryGetDistinguishedName(out string value) {
2020

2121
private bool CheckCache(string propertyName) {
2222
try {
23+
// Blocking External Call
2324
if (!_entry.Properties.Contains(propertyName))
2425
_entry.RefreshCache(new[] { propertyName });
25-
26+
27+
// Blocking External Call
2628
return _entry.Properties.Contains(propertyName);
2729
}
2830
catch {
@@ -35,7 +37,8 @@ public bool TryGetProperty(string propertyName, out string value) {
3537
if (!CheckCache(propertyName)) {
3638
return false;
3739
}
38-
40+
41+
// Blocking External Call
3942
var s = _entry.Properties[propertyName].Value;
4043
value = s switch {
4144
string st => st,
@@ -51,7 +54,8 @@ public bool TryGetByteProperty(string propertyName, out byte[] value) {
5154
if (!CheckCache(propertyName)) {
5255
return false;
5356
}
54-
57+
58+
// Blocking External Call
5559
var prop = _entry.Properties[propertyName].Value;
5660
if (prop is not byte[] b) return false;
5761
value = b;
@@ -65,6 +69,7 @@ public bool TryGetArrayProperty(string propertyName, out string[] value) {
6569
}
6670

6771
var dest = new List<string>();
72+
// Blocking External Call
6873
foreach (var val in _entry.Properties[propertyName]) {
6974
if (val is string s) {
7075
dest.Add(s);
@@ -81,6 +86,7 @@ public bool TryGetByteArrayProperty(string propertyName, out byte[][] value) {
8186
return false;
8287
}
8388

89+
// Blocking External Call
8490
var raw = _entry.Properties[propertyName].Value;
8591
if (raw is not byte[][] b) {
8692
return false;
@@ -132,6 +138,7 @@ public bool TryGetSecurityIdentifier(out string securityIdentifier) {
132138
return false;
133139
}
134140

141+
// Blocking External Call
135142
var raw = _entry.Properties[LDAPProperties.ObjectSID][0];
136143
try {
137144
securityIdentifier = raw switch {
@@ -163,25 +170,29 @@ public bool TryGetGuid(out string guid) {
163170

164171
public string GetProperty(string propertyName) {
165172
CheckCache(propertyName);
173+
// Blocking External Call
166174
return _entry.Properties[propertyName].Value as string;
167175
}
168176

169177
public byte[] GetByteProperty(string propertyName) {
170178
CheckCache(propertyName);
179+
// Blocking External Call
171180
return _entry.Properties[propertyName].Value as byte[];
172181
}
173182

174183
public int PropertyCount(string propertyName) {
175184
if (!CheckCache(propertyName)) {
176185
return 0;
177186
}
178-
187+
188+
// Blocking External Call
179189
var prop = _entry.Properties[propertyName];
180190
return prop.Count;
181191

182192
}
183193

184194
public IEnumerable<string> PropertyNames() {
195+
// Blocking External Call
185196
foreach (var property in _entry.Properties.PropertyNames)
186197
yield return property.ToString().ToLower();
187198
}

0 commit comments

Comments
 (0)