Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP Connection Logic Rewrite #127

Merged
merged 14 commits into from
May 30, 2024
Merged

LDAP Connection Logic Rewrite #127

merged 14 commits into from
May 30, 2024

Conversation

rvazarkar
Copy link
Contributor

Description

Completely alters the way LDAP connections are made to be more stable

Motivation and Context

LDAP connections are currently brittle and prone to breaking. Additionally, the GetDomain call is very flaky and can cause failures where it shouldn't. With this change, we use the following workflow:

/// Step 1: If domain passed in is non-null, skip this step
        // - Call GetDomain with a null domain to get the user's current domain
        // Step 2: Take domain passed in to the function or resolved from step 1
        // - Try an ldap connection on SSL
        // - If ServerUnavailable - Try an ldap connection on non-SSL
        //     Step 3: Pass the domain to GetDomain to resolve to a better name (potentially)
        //     - If we get a better name, repeat step 2 with the new name
        //     Step 4:
        // - Use GetDomain to get a domain object along with a list of domain controllers
        // - Try the primary domain controller on both ssl/non-ssl
        // - Loop over domain controllers and try each on ssl/non-ssl

This allows several fallback steps when negotiation ldap connections, which should significantly improve the ability of SharpHound to successfully connect

How Has This Been Tested?

Local tests pass + testing in local environment. Additional tests incoming

Screenshots (if appropriate):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

Copy link
Contributor

@zinic zinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got a few things that may or may not be issues and some thoughts.

src/CommonLib/LDAPConfig.cs Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Outdated Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Outdated Show resolved Hide resolved
src/CommonLib/LDAPUtils.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@zinic zinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes this round look good

Copy link
Contributor

@juggernot325 juggernot325 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have limited C# knowledge, but the LDAP connection and error handling logic looks sound.

@rvazarkar rvazarkar merged commit cc7b6a2 into v3 May 30, 2024
3 checks passed
@rvazarkar rvazarkar deleted the ldap_connection_rewrite branch May 30, 2024 16:20
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants