From d172461cd342e48e99ab78470f1f80288f88c7dc Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 26 Jul 2024 14:42:10 -0400 Subject: [PATCH 1/4] fix: missing domains in query for GPOLocalGroups, tighten up an error condition in the connection pool manager --- src/CommonLib/ConnectionPoolManager.cs | 6 ++++-- src/CommonLib/Processors/GPOLocalGroupProcessor.cs | 9 ++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/CommonLib/ConnectionPoolManager.cs b/src/CommonLib/ConnectionPoolManager.cs index f4f76719..f7f015f8 100644 --- a/src/CommonLib/ConnectionPoolManager.cs +++ b/src/CommonLib/ConnectionPoolManager.cs @@ -43,6 +43,9 @@ public void ReleaseConnection(LdapConnectionWrapper connectionWrapper, bool conn public async Task<(bool Success, LdapConnectionWrapper ConnectionWrapper, string Message)> GetLdapConnection( string identifier, bool globalCatalog) { + if (identifier == null) { + return (false, default, "Provided a null identifier for the connection"); + } var resolved = ResolveIdentifier(identifier); if (!_pools.TryGetValue(resolved, out var pool)) { @@ -72,8 +75,7 @@ private string ResolveIdentifier(string identifier) { if (_resolvedIdentifiers.TryGetValue(identifier, out var resolved)) { return resolved; } - - + if (GetDomainSidFromDomainName(identifier, out var sid)) { _log.LogDebug("Resolved identifier {Identifier} to {Resolved}", identifier, sid); _resolvedIdentifiers.TryAdd(identifier, sid); diff --git a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs index eadf1e54..2ed90e7d 100644 --- a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs +++ b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs @@ -54,7 +54,7 @@ public Task ReadGPOLocalGroups(IDirectoryObject entry) { return ReadGPOLocalGroups(links, dn); } - return default; + return Task.FromResult(new ResultingGPOChanges()); } public async Task ReadGPOLocalGroups(string gpLink, string distinguishedName) { @@ -63,13 +63,15 @@ public async Task ReadGPOLocalGroups(string gpLink, string if (gpLink == null) return ret; + var domain = Helpers.DistinguishedNameToDomain(distinguishedName); // First lets check if this OU actually has computers that it contains. If not, then we'll ignore it. // Its cheaper to fetch the affected computers from LDAP first and then process the GPLinks var affectedComputers = new List(); await foreach (var result in _utils.Query(new LdapQueryParameters() { LDAPFilter = new LdapFilter().AddComputersNoMSAs().GetFilter(), Attributes = CommonProperties.ObjectSID, - SearchBase = distinguishedName + SearchBase = distinguishedName, + DomainName = domain })) { if (!result.IsSuccess) { break; @@ -119,7 +121,8 @@ public async Task ReadGPOLocalGroups(string gpLink, string LDAPFilter = new LdapFilter().AddAllObjects().GetFilter(), SearchScope = SearchScope.Base, Attributes = CommonProperties.GPCFileSysPath, - SearchBase = linkDn + SearchBase = linkDn, + DomainName = gpoDomain }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); if (!result.IsSuccess) { From 925f1688daec44b83b1484dd8d0f7a7fe5fa4c85 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 26 Jul 2024 15:01:58 -0400 Subject: [PATCH 2/4] chore: fix broken tests fix: make gpolocalgroup processor a bit smarter --- src/CommonLib/Processors/GPOLocalGroupProcessor.cs | 13 ++++++++++++- test/unit/GPOLocalGroupProcessorTest.cs | 13 +++++-------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs index 2ed90e7d..c579e0fb 100644 --- a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs +++ b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs @@ -63,7 +63,18 @@ public async Task ReadGPOLocalGroups(string gpLink, string if (gpLink == null) return ret; - var domain = Helpers.DistinguishedNameToDomain(distinguishedName); + string domain; + //If our dn is null, use our default domain + if (string.IsNullOrEmpty(distinguishedName)) { + if (!_utils.GetDomain(out var d)) { + return ret; + } + + domain = d.Name; + } else { + domain = Helpers.DistinguishedNameToDomain(distinguishedName); + } + // First lets check if this OU actually has computers that it contains. If not, then we'll ignore it. // Its cheaper to fetch the affected computers from LDAP first and then process the GPLinks var affectedComputers = new List(); diff --git a/test/unit/GPOLocalGroupProcessorTest.cs b/test/unit/GPOLocalGroupProcessorTest.cs index 84643074..4e7ad967 100644 --- a/test/unit/GPOLocalGroupProcessorTest.cs +++ b/test/unit/GPOLocalGroupProcessorTest.cs @@ -150,10 +150,7 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups_Null_Gpcfilesyspath( var result = await processor.ReadGPOLocalGroups(testGPLinkProperty, null); Assert.NotNull(result); - Assert.Single(result.AffectedComputers); - var actual = result.AffectedComputers.First(); - Assert.Equal(Label.Computer, actual.ObjectType); - Assert.Equal("teapot", actual.ObjectIdentifier); + Assert.Empty(result.AffectedComputers); } [Fact] @@ -189,12 +186,12 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups() { "[LDAP:/o=foo/ou=foo Group (ABC123)/cn=foouser (blah)123/dc=somedomain;0;][LDAP:/o=foo/ou=foo Group (ABC123)/cn=foouser (blah)123/dc=someotherdomain;2;]"; var result = await processor.ReadGPOLocalGroups(testGPLinkProperty, null); + var domain = MockableDomain.Construct("TESTLAB.LOCAL"); + mockLDAPUtils.Setup(x => x.GetDomain(out domain)).Returns(true); + mockLDAPUtils.VerifyAll(); Assert.NotNull(result); - Assert.Single(result.AffectedComputers); - var actual = result.AffectedComputers.First(); - Assert.Equal(Label.Computer, actual.ObjectType); - Assert.Equal("teapot", actual.ObjectIdentifier); + Assert.Empty(result.AffectedComputers); } [Fact] From 5bc46d36836cccf83d0cde523ddf833f47dc9f4b Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 26 Jul 2024 15:09:43 -0400 Subject: [PATCH 3/4] chore: fix tests --- test/unit/GPOLocalGroupProcessorTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/GPOLocalGroupProcessorTest.cs b/test/unit/GPOLocalGroupProcessorTest.cs index 4e7ad967..9964f9b6 100644 --- a/test/unit/GPOLocalGroupProcessorTest.cs +++ b/test/unit/GPOLocalGroupProcessorTest.cs @@ -189,7 +189,7 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups() { var domain = MockableDomain.Construct("TESTLAB.LOCAL"); mockLDAPUtils.Setup(x => x.GetDomain(out domain)).Returns(true); - mockLDAPUtils.VerifyAll(); + //mockLDAPUtils.VerifyAll(); Assert.NotNull(result); Assert.Empty(result.AffectedComputers); } From 77615a9aae670e228d73f356c79e3bd69ba39eae Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Fri, 26 Jul 2024 15:50:46 -0400 Subject: [PATCH 4/4] chore: fix tests for real --- test/unit/GPOLocalGroupProcessorTest.cs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/test/unit/GPOLocalGroupProcessorTest.cs b/test/unit/GPOLocalGroupProcessorTest.cs index 9964f9b6..eabc5a81 100644 --- a/test/unit/GPOLocalGroupProcessorTest.cs +++ b/test/unit/GPOLocalGroupProcessorTest.cs @@ -147,13 +147,15 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups_Null_Gpcfilesyspath( var processor = new GPOLocalGroupProcessor(mockLDAPUtils.Object); var testGPLinkProperty = "[LDAP:/o=foo/ou=foo Group (ABC123)/cn=foouser (blah)123/dc=somedomain;0;][LDAP:/o=foo/ou=foo Group (ABC123)/cn=foouser (blah)123/dc=someotherdomain;2;]"; - var result = await processor.ReadGPOLocalGroups(testGPLinkProperty, null); + var result = await processor.ReadGPOLocalGroups(testGPLinkProperty, "DC=Testlab,DC=Local"); - Assert.NotNull(result); - Assert.Empty(result.AffectedComputers); + Assert.Single(result.AffectedComputers); + var actual = result.AffectedComputers.First(); + Assert.Equal(Label.Computer, actual.ObjectType); + Assert.Equal("teapot", actual.ObjectIdentifier); } - [Fact] + [WindowsOnlyFact] public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups() { var mockLDAPUtils = new Mock(MockBehavior.Loose); var gpcFileSysPath = Path.GetTempPath(); @@ -179,19 +181,21 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups() { .Returns(mockComputerResults.ToAsyncEnumerable) .Returns(mockGCPFileSysPathResults.ToAsyncEnumerable) .Returns(Array.Empty>().ToAsyncEnumerable); + var domain = MockableDomain.Construct("TESTLAB.LOCAL"); + mockLDAPUtils.Setup(x => x.GetDomain(out domain)).Returns(true); var processor = new GPOLocalGroupProcessor(mockLDAPUtils.Object); + var testGPLinkProperty = "[LDAP:/o=foo/ou=foo Group (ABC123)/cn=foouser (blah)123/dc=somedomain;0;][LDAP:/o=foo/ou=foo Group (ABC123)/cn=foouser (blah)123/dc=someotherdomain;2;]"; var result = await processor.ReadGPOLocalGroups(testGPLinkProperty, null); - - var domain = MockableDomain.Construct("TESTLAB.LOCAL"); - mockLDAPUtils.Setup(x => x.GetDomain(out domain)).Returns(true); - + //mockLDAPUtils.VerifyAll(); - Assert.NotNull(result); - Assert.Empty(result.AffectedComputers); + Assert.Single(result.AffectedComputers); + var actual = result.AffectedComputers.First(); + Assert.Equal(Label.Computer, actual.ObjectType); + Assert.Equal("teapot", actual.ObjectIdentifier); } [Fact]