Repository navigation
Focused GetDomain Replacement #317
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
Open
rvazarkar
wants to merge
18
commits into
ldap_beta_fixes
Choose a base branch
from
get_domain_v3
base: ldap_beta_fixes
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
644af63
chore: add new classes to scaffold
rvazarkar 815cf75
chore: add LdapConnectionFactory
rvazarkar 882b744
chore: add domain resolver class and fix a bug in culture comparison
rvazarkar d746c61
feat: add UserDomain parameter to allow hinting in NetOnly situations
rvazarkar 3e43a99
feat: add additional metadata to resolved ldap data
rvazarkar 7d2ab27
feat: add trust enrichment data to domain info
rvazarkar 3da65fb
feat: add legacy domain resolver to use old GetDomain functionality i…
rvazarkar 0f2e857
refactor(ldap)!: switch domain resolution to controlled LDAP
rvazarkar aeccc97
chore: add some more tests
rvazarkar ec56aca
fix: incorrect auth retries when it is a definitive failure
rvazarkar 4768819
chore: add agents and coding_standards
rvazarkar 1ebf467
chore: capitalize files for agents
rvazarkar b1171eb
feat: include RODC in controller discovery
rvazarkar d46c1f4
fix(ldap): retry failed metadata reads while preserving cached identity
rvazarkar c1c8869
fix: bug in trust classification
rvazarkar be8b44f
fix: authentication rejection should short circuit all other LDAP que…
rvazarkar cd6bbf0
fix: accept single label DNS names
rvazarkar fc42c38
chore: use toupperinvariant for an edge case
rvazarkar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # Repository guide for coding agents | ||
|
|
||
| Read `coding_standards.md` and `CONTRIBUTING.md` before changing code. Treat this file as a practical map of the repository, not as a substitute for inspecting the affected component. | ||
|
|
||
| ## Repository map | ||
|
|
||
| - `src/CommonLib/`: `SharpHoundCommonLib`, the higher-level library for LDAP resolution, caching, processors, metrics, and collection support. | ||
| - `src/SharpHoundRPC/`: `SharpHoundRPC`, the lower-level Windows RPC, native interop, handle, and registry library. CommonLib references this project. | ||
| - `test/unit/`: xUnit tests for CommonLib, including mocks and facades. | ||
| - `RPCTest/`: xUnit tests for SharpHoundRPC. | ||
| - `docfx/`: documentation project and generated coverage output. | ||
| - `.github/workflows/build-and-test.yml`: authoritative CI build and test sequence. | ||
|
|
||
| The two shipping projects target `net472` via `Directory.Build.props`; both test projects target `net8.0`. The root README's older prerequisite text does not override the project files. Windows is the CI platform and the code uses Windows and Active Directory APIs. | ||
|
|
||
| ## Working on a change | ||
|
|
||
| 1. Inspect the affected project, nearby implementation and tests, and any relevant public contract before editing. | ||
| 2. Keep changes scoped. Follow the existing style in each file; do not reformat unrelated code or change target frameworks, dependencies, package metadata, or generated files without a task reason. | ||
| 3. Put high-level behavior in CommonLib and native/RPC details in SharpHoundRPC. Preserve existing result, error, cancellation, and handle ownership behavior unless the task calls for changing it. | ||
| 4. Add focused tests for behavior changes using local mocks or facades. Do not require a live domain, credentials, or external hosts for routine tests. | ||
| 5. Run the relevant test project, then the CI sequence when shared behavior or build configuration changes. Report commands run, failures, and any environment limitation accurately. | ||
| 6. Update relevant README or API documentation when a consumer-facing contract changes. | ||
|
|
||
| ## Commands | ||
|
|
||
| Run from the repository root on Windows with the .NET 8 SDK: | ||
|
|
||
| ```powershell | ||
| dotnet restore | ||
| dotnet build --no-restore | ||
| dotnet test --no-build | ||
| ``` | ||
|
|
||
| For a focused check, use `dotnet test test/unit/CommonLibTest.csproj` or `dotnet test RPCTest/RPCTest.csproj`. `dotnet test` produces coverage files under `docfx/coverage/`. | ||
|
|
||
| ## Workspace care | ||
|
|
||
| - Inspect `git status` before and after changes. Preserve user edits and untracked files. | ||
| - Do not commit generated coverage, `bin/`, or `obj/` output. | ||
| - Do not put secrets, credentials, or sensitive collected directory data into code, tests, logs, or documentation. | ||
| - If the requested change needs a real AD environment or Windows-only behavior that cannot be exercised locally, use the available unit tests and state the remaining validation gap. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Coding standards | ||
|
|
||
| These standards apply to new work in SharpHoundCommon. Keep changes focused and follow the style of the file being edited; the repository does not have a single enforced formatter. | ||
|
|
||
| ## Project boundaries and compatibility | ||
|
|
||
| - `src/CommonLib` contains higher-level LDAP, cache, processor, and collection behavior. `src/SharpHoundRPC` contains lower-level Windows RPC, native interop, handle, and registry code. Keep the dependency direction from CommonLib to RPC. | ||
| - Both shipping libraries target **.NET Framework 4.7.2** through `Directory.Build.props`. Do not use an API or language feature in library code unless it builds for that target and its configured compiler. The `net8.0` test projects can use newer language features; their syntax is not a compatibility guide for library code. | ||
| - Preserve public signatures, serialized output shapes, result and error meanings, and package behavior unless a change deliberately updates that contract. Add a regression test for a behavior change. | ||
| - Keep Windows and Active Directory specifics behind the existing interfaces and wrappers so behavior can be tested without a live domain. | ||
|
|
||
| ## C# style | ||
|
|
||
| - Use four spaces for indentation. Match the surrounding file's namespace and brace layout; both end-of-line and next-line braces exist in this repository. Avoid formatting unrelated code. | ||
| - Use `PascalCase` for types, public members, and constants; `camelCase` for parameters and locals; and `_camelCase` for private fields. Use names that reflect the AD, LDAP, RPC, or registry concept involved. | ||
| - Prefer small methods with explicit inputs and outcomes. Reuse existing interfaces, result types, and helpers instead of introducing a parallel abstraction for the same operation. | ||
| - Use `async`/`await` for asynchronous I/O. Propagate cancellation where an API accepts a `CancellationToken`; do not hide cancellation as an ordinary failure. | ||
| - Use structured `ILogger` messages with named placeholders. Do not log credentials, tokens, private keys, or raw sensitive directory data. | ||
| - In RPC and interop code, make ownership clear. Dispose native handles, buffers, and other disposable resources on success and failure paths; keep conversions and lifetime boundaries close together. | ||
| - Add XML documentation when a public API's purpose, parameters, error behavior, or ownership is not clear from its name. Update package READMEs for consumer-facing changes. | ||
|
|
||
| ## Tests | ||
|
|
||
| - Add or update focused xUnit tests in `test/unit` for CommonLib changes and `RPCTest` for RPC changes. Put tests near the existing tests for the affected component. | ||
| - Test observable behavior and important failure paths, including null or missing LDAP values, RPC status failures, cancellation, and resource cleanup when relevant. Use the existing mocks and facades for directory, network, and native boundaries. | ||
| - Keep routine tests deterministic and independent of a live AD domain or remote host. Avoid timing-sensitive assertions and shared mutable state when practical. | ||
| - Use a descriptive test name consistent with neighboring tests; `[Theory]` is useful for related input cases. Do not add tests that only repeat implementation details. | ||
|
|
||
| ## Validation and review | ||
|
|
||
| CI runs on Windows with the .NET 8 SDK. From the repository root, its core sequence is: | ||
|
|
||
| ```powershell | ||
| dotnet restore | ||
| dotnet build --no-restore | ||
| dotnet test --no-build | ||
| ``` | ||
|
|
||
| Run the relevant test project during development, then the full sequence for changes that affect shared code or project configuration. `dotnet test` also generates coverage under `docfx/coverage/` as described in `CONTRIBUTING.md`. | ||
|
|
||
| Before review, check for unintended public API changes, compatibility with `net472`, resource leaks, sensitive logging, and unrelated formatting changes. Explain behavior changes and test evidence in the pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| using System; | ||
| using System.DirectoryServices.Protocols; | ||
| using System.Net; | ||
|
|
||
| namespace SharpHoundCommonLib { | ||
| internal static class LdapConnectionFactory { | ||
| // Creates an unbound connection. The caller owns binding, retries, and disposal. | ||
| internal static LdapConnection Create(LdapConfig config, string target, bool ssl, | ||
| bool globalCatalog = false, bool pinServer = false) { | ||
| var port = globalCatalog ? config.GetGCPort(ssl) : config.GetPort(ssl); | ||
| var identifier = new LdapDirectoryIdentifier(target, port, pinServer, false); | ||
| var connection = new LdapConnection(identifier); | ||
| try { | ||
| connection.Timeout = TimeSpan.FromMinutes(5); | ||
| connection.SessionOptions.ProtocolVersion = 3; | ||
| // Referral chasing does not work with paged searches. | ||
| connection.SessionOptions.ReferralChasing = ReferralChasingOptions.None; | ||
| if (pinServer) connection.SessionOptions.AutoReconnect = false; | ||
| if (ssl) connection.SessionOptions.SecureSocketLayer = true; | ||
|
|
||
| var signing = !config.DisableSigning && !ssl; | ||
| connection.SessionOptions.Signing = signing; | ||
| connection.SessionOptions.Sealing = signing; | ||
|
|
||
| if (config.DisableCertVerification) | ||
| connection.SessionOptions.VerifyServerCertificate = (_, _) => true; | ||
|
|
||
| if (config.Username != null) | ||
| connection.Credential = new NetworkCredential(config.Username, config.Password); | ||
|
|
||
| connection.AuthType = config.AuthType; | ||
| return connection; | ||
| } | ||
| catch { | ||
| connection.Dispose(); | ||
| throw; | ||
| } | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: SpecterOps/SharpHoundCommon
Length of output: 8365
🌐 Web query:
GitHub Copilot coding agent AGENTS.md filename discovery official documentation💡 Result:
🤖 get_repo_knowledge executed:
get_repo_knowledge SpecterOps/SharpHoundCommon /tmp/coderabbit-repo-knowledge/specterops-sharphoundcommon-a007128bLength of output: 632
Rename the guide to
AGENTS.mdfor GitHub Copilot discovery.GitHub documents
AGENTS.mdas the repository instruction filename. This file is namedagents.md, so GitHub Copilot coding agent may not discover it. If another loader is intended, document that loader and its filename.Suggested rename
diff --git a/agents.md b/AGENTS.md similarity index 100% rename from agents.md rename to AGENTS.md🤖 Prompt for AI Agents