feat: Add a CheckActiveDirectory module for AD/identity health - #1399
Open
mickem wants to merge 5 commits into
Open
feat: Add a CheckActiveDirectory module for AD/identity health#1399mickem wants to merge 5 commits into
mickem wants to merge 5 commits into
Conversation
Add a new Windows-only CheckActiveDirectory module covering the domain health signals NSClient++ had no answer for, plus a scenario guide that extends coverage to NTDS/ADCS/ADFS via existing check_pdh counters. New commands: - check_ad_replication: inbound replication links on a domain controller via DsBind/DsReplicaGetInfo (last success/attempt as date keywords, consecutive_failures as perfdata). WARN on the first failed sync, CRIT after five in a row or 24h without success. Non-DCs get a fleet-safe UNKNOWN contract; single-DC domains an explanatory OK empty state. - check_secure_channel: verify the machine-account secure channel via netlogon TC_VERIFY (the Test-ComputerSecureChannel operation), with verify=false for a passive TC_QUERY. Workgroup machines return the documented not-joined UNKNOWN. - check_kdc: send a real unauthenticated Kerberos AS-REQ over TCP 88 and classify the answer; KDC_ERR_PREAUTH_REQUIRED is the healthy response. This catches a KDC that accepts connections but no longer issues tickets, which a port check cannot. KDC/realm are discovered from the domain join or given explicitly, with round-trip perfdata. The AS-REQ encoder/KRB-ERROR classifier is pure (kdc_probe.cpp) and covered by gtest against an independent DER reader. The integration suite drives all three commands over the client-query path and pins the Kerberos exchange deterministically against a canned fake KDC, including the valued-boolean (verify=false) REST parsing contract. Only system import libraries are added (ntdsapi, netapi32, winsock), so the installer grows by just the module DLL. The MSI, plugin feature-hint map and reference docs samples are wired up, and a new Active Directory & Identity scenario documents the module together with curated NTDS, Certification Authority and AD FS counter checks over check_pdh. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Michael Medin <michael@medin.name>
When a check's warning or critical expression carried no bound for a perf'd variable (e.g. check_kdc's default critical `responding = 0` alongside `time > 1000`), the missing threshold was rendered as a literal 0 in the emitted perfdata: 'dc01'=2ms;1000;0. Perfdata consumers (PNP4Nagios, Grafana, Icinga) read crit=0 as "critical when above 0", painting every healthy sample as breaching critical. The perf_value struct, the protobuf layer and the Nagios text renderer already model absent bounds; the value was lost at one seam, where the variable nodes defaulted missing warn/crit nodes to 0 before handing them to the performance generators. Thread boost::optional through that seam so an absent bound stays absent all the way out. Genuine 0 bounds (e.g. `warning=consecutive_failures > 0`) still render as 0. Checks whose thresholds only bound some of their perf'd keywords now emit shorter perf entries; captured sample docs will be refreshed as they are re-captured. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Michael Medin <michael@medin.name>
Code review of the new module surfaced a set of misclassification and timeout bugs; all checks keep their documented contracts but stop lying about failures: check_ad_replication - The benign not-a-DC contract now only applies without server=: an explicitly named DC that cannot be bound is a dead or unreachable domain controller and is reported as a plain UNKNOWN failure instead of the "safe to ignore fleet-wide" message. - DsBindW has no timeout of its own and blocks ~21s+ against a black-holed host; remote targets are now pre-flighted with a bounded TCP connect to the RPC endpoint mapper (port 135, 5s) so an unreachable DC fails fast instead of blowing the transport timeout. check_secure_channel - The domain-join defaulting queried the local machine even when server=<remote> was given; NetGetJoinInformation now targets the named computer, and the not-joined/failure messages name it. - Netlogon call failures (service stopped, access denied, RPC failure) were folded into the same healthy=0 row as a genuinely broken channel, paging a trust-relationship-broken alert for a healthy one. They now return UNKNOWN with the call error, matching the sibling checks' data-source-failure contract; CRITICAL is reserved for netlog2_tc_connection_status. check_kdc - KDCs were probed sequentially, each with its own timeout, so a site outage with several dead KDCs exceeded the transport command timeout and produced no result at all. All probes now share one io_context and one deadline: worst case is one timeout, not one per KDC. - Round-trip time was measured from before DNS resolution, billing a slow resolver to the KDC and tripping the `time > 1000` warning; it is now measured from connect, and stamped at completion so a slow KDC cannot inflate a fast one's reading. - timeout= was in seconds while every sibling network probe (check_tcp, check_dns, check_http, check_ntp_offset) is in milliseconds, and timeout*1000 could overflow int. It is now milliseconds (default 5000) with no multiplication. - The DER writer silently truncated element lengths above 0xFFFF, corrupting the request for absurdly long realms; it now emits however many long-form length octets the size needs (unit-tested through the independent reader). Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Michael Medin <michael@medin.name>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new Windows-only CheckActiveDirectory module and documentation to extend NSClient++ coverage for AD replication, secure channel, and Kerberos KDC health, including installer wiring and end-to-end tests.
Changes:
- Introduces
CheckActiveDirectorymodule withcheck_ad_replication,check_secure_channel, andcheck_kdccommands (plus pure Kerberos probe logic and replication helpers). - Adds unit + integration test coverage (gtest for probe/filter logic; Jest suite driving
nscp clientend-to-end). - Wires module into installer/feature hints and adds a new “Active Directory & Identity” monitoring scenario with command samples.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/checkactivedirectory-commands.test.ts | Adds Windows-only end-to-end command tests including a fake TCP KDC. |
| service/plugins/plugin_manager.cpp | Registers module name for installer feature hint mapping. |
| modules/CheckActiveDirectory/module.json | Declares new module metadata and exposed commands. |
| modules/CheckActiveDirectory/module.cmake | Gates module build to Windows. |
| modules/CheckActiveDirectory/kdc_probe.hpp | Declares pure AS-REQ builder and KRB-ERROR classifier API. |
| modules/CheckActiveDirectory/kdc_probe.cpp | Implements minimal DER writer + KDC response classifier used by check_kdc. |
| modules/CheckActiveDirectory/check_secure_channel.hpp | Declares check_secure_channel command entrypoint. |
| modules/CheckActiveDirectory/check_secure_channel.cpp | Implements netlogon secure-channel query/verify check and filter integration. |
| modules/CheckActiveDirectory/check_kdc.hpp | Declares check_kdc command entrypoint. |
| modules/CheckActiveDirectory/check_kdc.cpp | Implements concurrent TCP AS-REQ probes with perfdata latency and response classification. |
| modules/CheckActiveDirectory/check_ad_replication.hpp | Declares check_ad_replication command entrypoint. |
| modules/CheckActiveDirectory/check_ad_replication.cpp | Implements replication neighbor collection + thresholding/filter output. |
| modules/CheckActiveDirectory/check_activedirectory_test.cpp | Adds gtest coverage for AS-REQ encoding, response classification, and replication DN parsing helpers. |
| modules/CheckActiveDirectory/ad_replication_source_win.cpp | Implements Windows DS replication-neighbor fetch via DsReplicaGetInfo. |
| modules/CheckActiveDirectory/ad_replication_source.hpp | Declares replication neighbor fetch interface. |
| modules/CheckActiveDirectory/ad_replication_filter.hpp | Defines replication filter object + handler and DN parsing helper. |
| modules/CheckActiveDirectory/ad_replication_filter.cpp | Implements DN parsing helper and filter registry variables/perf config. |
| modules/CheckActiveDirectory/CheckActiveDirectory.h | Declares the module plugin class and command handlers. |
| modules/CheckActiveDirectory/CheckActiveDirectory.cpp | Implements plugin command forwarding to command modules. |
| modules/CheckActiveDirectory/CMakeLists.txt | Adds module build/link config plus unit test target. |
| installers/installer-NSCP/Product.wxs | Ships CheckActiveDirectory.dll in the MSI. |
| include/parsers/where/variable.hpp | Updates perfdata generation to keep warn/crit fields empty when no bounds exist. |
| docs/samples/CheckActiveDirectory_check_secure_channel_samples.md | Adds usage/output samples for check_secure_channel. |
| docs/samples/CheckActiveDirectory_check_secure_channel_desc.md | Adds command reference description for check_secure_channel. |
| docs/samples/CheckActiveDirectory_check_kdc_samples.md | Adds usage/output samples for check_kdc. |
| docs/samples/CheckActiveDirectory_check_kdc_desc.md | Adds command reference description for check_kdc. |
| docs/samples/CheckActiveDirectory_check_ad_replication_samples.md | Adds usage/output samples for check_ad_replication. |
| docs/samples/CheckActiveDirectory_check_ad_replication_desc.md | Adds command reference description for check_ad_replication. |
| docs/mkdocs.yml | Adds new “Active Directory & Identity” scenario page to nav. |
| docs/docs/scenarios/index.md | Links new scenario from scenarios index table. |
| docs/docs/scenarios/active-directory.md | Adds full scenario guide for AD/identity monitoring (module + PDH counters). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The KRB-ERROR walk validated a DER length with `pos + len <= size`. That sum wraps on a 32-bit build, so a crafted 4-octet length (0xfffffff2) passed the check and then moved the walk offset *backwards* - an unterminated parse loop that pins a worker thread forever. The response comes from whatever host:port the check was pointed at, so any caller able to run check_kdc could wedge the agent, and the x86 packages are the exposed ones. The reader is now a bounds-checked cursor: lengths are validated against the bytes remaining, TLVs hand out sub-cursors instead of raw offsets, and the integer accumulator is unsigned (shifting a negative value left is undefined before C++20). Nothing indexes the buffer by hand any more. The independent reader in the test grew the same rule, which turned up a test vector whose declared lengths were a byte short - the old lenient parser had accepted it. The win32 handles follow: ds_binding for DsBind/DsUnBind, ds_replica_info for the type-matched DsReplicaFreeInfo, and net_api_ptr for the NetApiBufferFree family, so every early return releases what it took. Raw arrays and `new` gave way to std::array and make_shared. Also fixed: - FormatMessage terminates its text with CRLF, and the checks embedded that mid-sentence: the documented check_kdc failure literally rendered across two lines, and NRPE/NSCA keep only the first. Trimmed at every site. - A stopped NTDS on a real domain controller reported "Not a domain controller" - the exact outage the check exists for, filed as benign. The machine role now decides that, not the bind failure, which cannot tell a member server from a DC with a dead directory service. - check_kdc emitted time=-1 perfdata for a host that never resolved, planting a negative latency in the series. `time` is optional now: no exchange, no sample. - check_ad_replication took its 5s reachability deadline from a literal; timeout= now exposes it the way check_kdc does. - A discovered realm is uppercased as ASCII (std::toupper is locale dependent and mangles UTF-8); an explicit realm= is passed through as typed, since Kerberos realms are case sensitive. - Guarded the discovery paths that could otherwise render "all 0 KDC(s) are responding" or query netlogon for an empty domain name. Review feedback: capped realm= at 255 characters so a caller cannot size the payload the agent writes at an arbitrary host, corrected the module.cmake skip reason capitalisation, and stripped the BOM from the check_kdc samples. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Michael Medin <michael@medin.name>
The non-const std::wstring::data() overload is C++17, so the oldest supported MSVC toolset compiled the trusted-domain buffer as const wchar_t* and rejected the LPWSTR initialisation, breaking all three Windows builds. Take the buffer by &[0] instead, which yields a mutable pointer under every supported standard. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Michael Medin <michael@medin.name>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add a new Windows-only CheckActiveDirectory module covering the domain health signals NSClient++ had no answer for, plus a scenario guide that extends coverage to NTDS/ADCS/ADFS via existing check_pdh counters.
New commands:
The AS-REQ encoder/KRB-ERROR classifier is pure (kdc_probe.cpp) and covered by gtest against an independent DER reader. The integration suite drives all three commands over the client-query path and pins the Kerberos exchange deterministically against a canned fake KDC, including the valued-boolean (verify=false) REST parsing contract.
Only system import libraries are added (ntdsapi, netapi32, winsock), so the installer grows by just the module DLL. The MSI, plugin feature-hint map and reference docs samples are wired up, and a new Active Directory & Identity scenario documents the module together with curated NTDS, Certification Authority and AD FS counter checks over check_pdh.
Assisted-by: Claude Code:claude-fable-5