fix: Harden the CheckActiveDirectory checks against review findings

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>

Author: mickem

docs/samples/CheckActiveDirectory_check_ad_replication_desc.md

@@ -32,7 +32,13 @@ synced trips the 24-hour rule by design.
 Options: `server=<dc>` checks another domain controller (default: the local
 machine — replication state is per-DC, so run the check on every DC).
 
-**Not-a-DC contract:** on a host that does not run the directory service the
-check returns **UNKNOWN** with a "Not a domain controller" message rather than
-a hard error, so it is safe to deploy fleet-wide. A single-DC domain (no
-replication partners) returns **OK** with an explanatory empty-state message.
+**Not-a-DC contract:** run without `server=` on a host that does not run the
+directory service, the check returns **UNKNOWN** with a "Not a domain
+controller" message rather than a hard error, so it is safe to deploy
+fleet-wide. The contract only applies locally: a `server=` target that cannot
+be bound is reported as a plain failure — an explicitly named DC being
+unreachable (powered off, firewalled, NTDS stopped) is an outage, not an
+ignorable non-DC. Remote targets are first probed on TCP port 135 (the RPC
+endpoint mapper) with a 5 second deadline so a black-holed host fails fast
+instead of hanging the check. A single-DC domain (no replication partners)
+returns **OK** with an explanatory empty-state message.

docs/samples/CheckActiveDirectory_check_kdc_desc.md

@@ -29,5 +29,6 @@ Defaults: **WARNING** when `time > 1000`, **CRITICAL** when `responding = 0`.
 Options: `server=<host>` (repeatable) picks the KDC(s) to probe and
 `realm=<REALM>` the realm; both default to what the domain join discovers
 (`DsGetDcName`). On a machine that is not domain-joined, `server=` and
-`realm=` are required and the check says so with **UNKNOWN**. `timeout=<s>`
-(default 5) bounds each probe.
+`realm=` are required and the check says so with **UNKNOWN**. `timeout=<ms>`
+(default 5000) bounds the probes; all KDCs are probed concurrently, so it also
+bounds the whole check even when several KDCs are unreachable.

docs/samples/CheckActiveDirectory_check_secure_channel_desc.md

@@ -24,13 +24,18 @@ Keywords (a single row):
 Defaults: **CRITICAL** when `healthy = 0`; no warning threshold.
 
 Options: `domain=<name>` checks the channel to a specific trusted domain
-(default: the domain this machine is joined to); `server=<host>` queries
-another computer's netlogon service.
+(default: the domain the checked machine is joined to); `server=<host>`
+queries another computer's netlogon service (its join state is then also read
+from that computer when `domain=` is not given).
 
 **Not-joined contract:** on a workgroup or standalone machine the check
 returns **UNKNOWN** ("not joined to a domain") rather than a hard error, so it
 is safe to deploy fleet-wide.
 
-Note: verifying the channel requires administrator rights on the target, which
-the NSClient++ service (LocalSystem) has; running the check as an unprivileged
-user may yield access-denied instead.
+**CRITICAL means a broken channel, nothing else:** when the netlogon query
+itself fails — the service is stopped or restarting, the caller lacks
+administrator rights, or the RPC connection to `server=` fails — the check
+returns **UNKNOWN** with the failure message instead of scoring the channel as
+broken. Verifying the channel requires administrator rights on the target,
+which the NSClient++ service (LocalSystem) has; running the check as an
+unprivileged user yields that UNKNOWN.

modules/CheckActiveDirectory/ad_replication_source_win.cpp

@@ -3,13 +3,22 @@
 
 #include "ad_replication_source.hpp"
 
+// boost/asio must precede Windows.h so winsock2.h is included first.
+#include <boost/asio.hpp>
+
 // Windows.h must precede ntdsapi.h; the capital W keeps clang-format's
-// case-sensitive include sort from breaking that order.
+// case-sensitive include sort from breaking that order. asio defines
+// WIN32_LEAN_AND_MEAN, which drops rpc.h from Windows.h, so pull it in
+// explicitly: ntdsapi.h needs RPC_AUTH_IDENTITY_HANDLE.
 #include <Windows.h>
+#include <rpc.h>
+
 #include <ntdsapi.h>
 
+#include <chrono>
 #include <error/error.hpp>
 #include <str/utf8.hpp>
+#include <str/xtos.hpp>
 #include <vector>
 
 namespace ad_replication_source {
@@ -25,6 +34,40 @@ std::string local_dns_hostname() {
   return utf8::cvt<std::string>(std::wstring(buf.data()));
 }
 
+// DsBindW itself has no timeout: against a black-holed host it blocks for the
+// full TCP retransmit window (~21s+, possibly per RPC endpoint), blowing the
+// transport's command timeout. Before binding to an explicitly named remote
+// server, require its RPC endpoint mapper (port 135) to answer a TCP connect
+// within a bounded deadline so an unreachable DC fails fast instead.
+bool can_reach_rpc(const std::string &host, int timeout_ms, std::string &error) {
+  namespace asio = boost::asio;
+  using boost::asio::ip::tcp;
+
+  asio::io_context io;
+  tcp::resolver resolver(io);
+  tcp::socket socket(io);
+  bool connected = false;
+
+  resolver.async_resolve(host, "135", [&](const boost::system::error_code &ec, tcp::resolver::results_type results) {
+    if (ec) {
+      error = "resolve failed: " + ec.message();
+      return;
+    }
+    asio::async_connect(socket, results, [&](const boost::system::error_code &ec, const tcp::endpoint &) {
+      if (ec)
+        error = "connect failed: " + ec.message();
+      else
+        connected = true;
+    });
+  });
+
+  io.run_for(std::chrono::milliseconds(timeout_ms));
+  if (!connected && error.empty()) error = "no answer within " + str::xtos(timeout_ms) + "ms";
+  boost::system::error_code ignored;
+  socket.close(ignored);
+  return connected;
+}
+
 long long filetime_to_epoch(const FILETIME &ft) {
   const unsigned long long ticks = (static_cast<unsigned long long>(ft.dwHighDateTime) << 32) | ft.dwLowDateTime;
   if (ticks == 0) return 0;
@@ -47,13 +90,22 @@ bool fetch(const std::string &server, std::vector<ad_replication_filter::filter_
   }
   const std::wstring target_w = utf8::cvt<std::wstring>(target);
 
+  if (!server.empty()) {
+    std::string reach_error;
+    if (!can_reach_rpc(server, 5000, reach_error)) {
+      error = "Cannot reach the RPC endpoint mapper on " + server + " (port 135): " + reach_error;
+      return false;
+    }
+  }
+
   HANDLE hds = nullptr;
   DWORD rc = DsBindW(target_w.c_str(), nullptr, &hds);
   if (rc != ERROR_SUCCESS) {
-    // The directory service not being reachable on the target is what a bind
-    // failure means in practice: a non-DC (or a stopped NTDS). Report it as
-    // the not-a-DC contract rather than a replication failure.
-    not_a_dc = true;
+    // Only the local host gets the benign not-a-DC contract: deployed
+    // fleet-wide, a local bind failure means a non-DC (or a stopped NTDS).
+    // An explicitly named server failing to bind is a dead or unreachable
+    // domain controller — the very outage the check exists to surface.
+    not_a_dc = server.empty();
     error = "Failed to bind to the directory service on " + target + ": " + error::lookup::last_error(rc);
     return false;
   }

modules/CheckActiveDirectory/check_activedirectory_test.cpp

@@ -184,6 +184,25 @@ TEST_F(AsReqTest, EncodesTillNonceAndEtypes) {
   EXPECT_EQ(23, read_int(etype_list[2]));  // rc4-hmac
 }
 
+TEST(AsReq, EncodesElementsLargerThan64k) {
+  // A realm this large pushes the req-body (where the realm appears twice)
+  // past 0xFFFF, forcing 3-byte long-form DER lengths; an encoder that
+  // truncates to 2 length octets would emit a self-inconsistent blob this
+  // independent reader could not walk.
+  const std::string realm(70000, 'R');
+  const bytes req = kdc_probe::build_as_req(realm, "c", 1UL);
+  const std::vector<tlv> outer = parse_tlvs(req);
+  ASSERT_EQ(1u, outer.size());
+  ASSERT_EQ(0x6a, outer[0].tag);
+  const std::vector<tlv> fields = parse_tlvs(parse_tlvs(outer[0].content)[0].content);
+  const tlv *body = find_tag(fields, 0xa4);
+  ASSERT_NE(nullptr, body);
+  const std::vector<tlv> body_fields = parse_tlvs(parse_tlvs(body->content)[0].content);
+  const tlv *realm_field = find_tag(body_fields, 0xa2);
+  ASSERT_NE(nullptr, realm_field);
+  EXPECT_EQ(realm, read_string(parse_tlvs(realm_field->content)[0]));
+}
+
 TEST(AsReq, NonceWithHighBitGetsLeadingZeroByte) {
   // 0x80000000 would read as negative without a leading 0x00 pad octet.
   const bytes req = kdc_probe::build_as_req("R", "c", 0x80000000UL);

modules/CheckActiveDirectory/check_kdc.cpp

@@ -15,6 +15,7 @@
 #include <algorithm>
 #include <cctype>
 #include <chrono>
+#include <deque>
 #include <error/error.hpp>
 #include <memory>
 #include <nscapi/nscapi_program_options.hpp>
@@ -91,14 +92,45 @@ struct probe_outcome {
   probe_outcome() : exchanged(false), time_ms(-1) {}
 };
 
+// The in-flight state of one KDC probe on the shared io_context.
+struct probe_state {
+  explicit probe_state(boost::asio::io_context &io) : resolver(io), socket(io), done(false), timed(false) { header[0] = header[1] = header[2] = header[3] = 0; }
+
+  boost::asio::ip::tcp::resolver resolver;
+  boost::asio::ip::tcp::socket socket;
+  unsigned char header[4];
+  bool done;
+  // Round-trip time is measured from when the resolver answered (timed set),
+  // so a slow DNS server is not billed to the KDC.
+  bool timed;
+  std::chrono::steady_clock::time_point exchange_start;
+  probe_outcome out;
+
+  void start_exchange() {
+    exchange_start = std::chrono::steady_clock::now();
+    timed = true;
+  }
+  // Terminal handlers stamp the time here; measuring after the event loop
+  // exits would bill a fast KDC for the full deadline whenever a slow one
+  // keeps the loop running.
+  void finish(const std::string &error) {
+    out.error = error;
+    if (timed) out.time_ms = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - exchange_start).count();
+    done = true;
+  }
+};
+
 // One framed request/response exchange (RFC 4120 7.2.2: 4-byte big-endian
-// length prefix) against host:port with an overall deadline.
-probe_outcome exchange_with_kdc(const std::string &host, int port, int timeout_ms, const kdc_probe::bytes &request) {
+// length prefix) per host against host:port. All hosts are probed concurrently
+// on one io_context with a single deadline, so the worst case costs one
+// timeout rather than one per unreachable KDC. A DNS lookup the OS never
+// answers can still hold the io_context destructor past the deadline (asio
+// runs getaddrinfo on a worker thread it joins on shutdown), but at most once
+// for the whole batch.
+std::vector<probe_outcome> exchange_with_kdcs(const std::vector<std::string> &hosts, int port, int timeout_ms, const kdc_probe::bytes &request) {
   namespace asio = boost::asio;
   using boost::asio::ip::tcp;
 
-  probe_outcome out;
-
   kdc_probe::bytes framed;
   framed.reserve(request.size() + 4);
   framed.push_back(static_cast<unsigned char>((request.size() >> 24) & 0xff));
@@ -108,68 +140,67 @@ probe_outcome exchange_with_kdc(const std::string &host, int port, int timeout_m
   framed.insert(framed.end(), request.begin(), request.end());
 
   asio::io_context io;
-  tcp::resolver resolver(io);
-  tcp::socket socket(io);
-  unsigned char header[4] = {0, 0, 0, 0};
-  bool done = false;
-  std::string error;
+  // deque: the completion handlers hold references into the container.
+  std::deque<probe_state> states;
 
-  const auto start = std::chrono::steady_clock::now();
-  resolver.async_resolve(host, str::xtos(port), [&](const boost::system::error_code &ec, tcp::resolver::results_type results) {
-    if (ec) {
-      error = "resolve failed: " + ec.message();
-      done = true;
-      return;
-    }
-    asio::async_connect(socket, results, [&](const boost::system::error_code &ec, const tcp::endpoint &) {
+  for (const std::string &host : hosts) {
+    states.emplace_back(io);
+    probe_state &st = states.back();
+    st.resolver.async_resolve(host, str::xtos(port), [&st, &framed](const boost::system::error_code &ec, tcp::resolver::results_type results) {
       if (ec) {
-        error = "connect failed: " + ec.message();
-        done = true;
+        st.finish("resolve failed: " + ec.message());
         return;
       }
-      asio::async_write(socket, asio::buffer(framed), [&](const boost::system::error_code &ec, std::size_t) {
+      st.start_exchange();
+      asio::async_connect(st.socket, results, [&st, &framed](const boost::system::error_code &ec, const tcp::endpoint &) {
         if (ec) {
-          error = "send failed: " + ec.message();
-          done = true;
+          st.finish("connect failed: " + ec.message());
           return;
         }
-        asio::async_read(socket, asio::buffer(header), [&](const boost::system::error_code &ec, std::size_t) {
+        asio::async_write(st.socket, asio::buffer(framed), [&st](const boost::system::error_code &ec, std::size_t) {
           if (ec) {
-            error = "read failed: " + ec.message();
-            done = true;
-            return;
-          }
-          const std::size_t len = (static_cast<std::size_t>(header[0]) << 24) | (static_cast<std::size_t>(header[1]) << 16) |
-                                  (static_cast<std::size_t>(header[2]) << 8) | static_cast<std::size_t>(header[3]);
-          if (len == 0 || len > 512 * 1024) {
-            error = "invalid response length";
-            done = true;
+            st.finish("send failed: " + ec.message());
             return;
           }
-          out.response.resize(len);
-          asio::async_read(socket, asio::buffer(out.response), [&](const boost::system::error_code &ec, std::size_t) {
+          asio::async_read(st.socket, asio::buffer(st.header), [&st](const boost::system::error_code &ec, std::size_t) {
             if (ec) {
-              error = "read failed: " + ec.message();
-              out.response.clear();
-              done = true;
+              st.finish("read failed: " + ec.message());
+              return;
+            }
+            const std::size_t len = (static_cast<std::size_t>(st.header[0]) << 24) | (static_cast<std::size_t>(st.header[1]) << 16) |
+                                    (static_cast<std::size_t>(st.header[2]) << 8) | static_cast<std::size_t>(st.header[3]);
+            if (len == 0 || len > 512 * 1024) {
+              st.finish("invalid response length");
               return;
             }
-            out.exchanged = true;
-            done = true;
+            st.out.response.resize(len);
+            asio::async_read(st.socket, asio::buffer(st.out.response), [&st](const boost::system::error_code &ec, std::size_t) {
+              if (ec) {
+                st.out.response.clear();
+                st.finish("read failed: " + ec.message());
+                return;
+              }
+              st.out.exchanged = true;
+              st.finish("");
+            });
           });
         });
       });
     });
-  });
+  }
 
   io.run_for(std::chrono::milliseconds(timeout_ms));
-  out.time_ms = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - start).count();
-  if (!done) {
-    error = "timeout after " + str::xtos(timeout_ms) + "ms";
-    boost::system::error_code ignored;
-    socket.close(ignored);
+
+  std::vector<probe_outcome> out;
+  for (probe_state &st : states) {
+    if (!st.done) {
+      boost::system::error_code ignored;
+      st.socket.close(ignored);
+      st.resolver.cancel();
+      st.finish("timeout after " + str::xtos(timeout_ms) + "ms");
+    }
+    out.push_back(st.out);
   }
-  out.error = error;
   return out;
 }
 
@@ -187,7 +218,7 @@ void check(const PB::Commands::QueryRequestMessage::Request &request, PB::Comman
   std::vector<std::string> servers;
   std::string realm;
   int port = 88;
-  int timeout = 5;
+  int timeout_ms = 5000;
 
   kdc_filter::filter filter;
   filter_helper.add_options("time > 1000", "responding = 0", "", filter.get_filter_syntax(), "ignored");
@@ -200,7 +231,7 @@ void check(const PB::Commands::QueryRequestMessage::Request &request, PB::Comman
     ("server", po::value<std::vector<std::string>>(&servers), "KDC host to probe; can be given multiple times (default: the KDC located via the domain join).")
     ("realm", po::value<std::string>(&realm), "Kerberos realm to request a ticket for (default: the joined domain; required when not domain-joined).")
     ("port", po::value<int>(&port)->default_value(88), "TCP port to probe.")
-    ("timeout", po::value<int>(&timeout)->default_value(5), "Seconds to wait for each KDC before considering it down.")
+    ("timeout", po::value<int>(&timeout_ms)->default_value(5000), "Timeout in milliseconds. All KDCs are probed concurrently, so this also bounds the whole check.")
     ;
   // clang-format on
 
@@ -228,13 +259,15 @@ void check(const PB::Commands::QueryRequestMessage::Request &request, PB::Comman
 
   const kdc_probe::bytes as_req = kdc_probe::build_as_req(realm, "nscp-probe", 12345678UL);
 
+  const std::vector<probe_outcome> outcomes = exchange_with_kdcs(servers, port, timeout_ms, as_req);
+
   parsers::where::constants::reset();
-  for (const std::string &server : servers) {
+  for (std::size_t i = 0; i < servers.size(); ++i) {
     kdc_filter::filter_obj_ptr obj(new kdc_filter::filter_obj());
-    obj->kdc = server;
+    obj->kdc = servers[i];
     obj->realm = realm;
     obj->port = port;
-    const probe_outcome outcome = exchange_with_kdc(server, port, timeout * 1000, as_req);
+    const probe_outcome &outcome = outcomes[i];
     obj->time = outcome.time_ms;
     if (outcome.exchanged) {
       const kdc_probe::classification c = kdc_probe::classify_response(outcome.response);