From 017f7c667aae7a9750efd307c15fe1e0b13e6e02 Mon Sep 17 00:00:00 2001 From: WaiZ0 <33723878+WaiZ0@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:56:47 +0200 Subject: [PATCH 1/2] fix(registry): lowercase target_hosts_by_hostname lookups target_hosts_by_hostname is documented and populated as keyed by lowercased canonical hostname (context.py), and mssql.py already looks it up correctly via target.lower(). Five lookups in registry.py looked it up with the target/probe hostname as-is instead, which is whatever case the host was actually discovered in (e.g. LDAP name- pattern matches return the AD computer's own casing, "MECM.sccm.lab"). Two of those five (get_ntlm_settings, get_mssql_settings) index the dict directly and raise KeyError on a case mismatch, killing the entire RemoteRegistry phase for that target -- including the site- code and component-server reads later in the same generator, which are the low-privilege-readable keys that identify a host as an SCCM site server / SQL server. Confirmed against a live lab where a fixed SMB auth bug let RemoteRegistry connect on every host for the first time, and every single one immediately crashed here. Co-Authored-By: Claude Sonnet 5 --- src/openhound_sccm/collectors/registry.py | 10 +++---- tests/registry_collect_test.py | 35 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/openhound_sccm/collectors/registry.py b/src/openhound_sccm/collectors/registry.py index 180353f..c43e5c5 100644 --- a/src/openhound_sccm/collectors/registry.py +++ b/src/openhound_sccm/collectors/registry.py @@ -465,7 +465,7 @@ def collect_registry(target: str, ctx: "SourceContext") -> Iterable[tuple[str, d # server name only when AD resolution failed (ad_object # is None or lacks a name). logger.info("Found %s, this target is a site server", SCCM_REG_KEYS["component_servers"]) - target_entry = ctx.target_hosts_by_hostname[target] + target_entry = ctx.target_hosts_by_hostname[target.lower()] row = { **(target_entry.ad_object or {}), "source": "RemoteRegistry-ComponentServers", @@ -529,7 +529,7 @@ def collect_registry(target: str, ctx: "SourceContext") -> Iterable[tuple[str, d "database lives elsewhere. Emitting the SQL Server role as assumed " "pending corroboration in preprocess.", target, ) - target_entry = ctx.target_hosts_by_hostname[target] + target_entry = ctx.target_hosts_by_hostname[target.lower()] row = { **(target_entry.ad_object or {}), "source": "RemoteRegistry-MultisiteComponentServers", @@ -600,7 +600,7 @@ def get_current_user(probe: _RegistryProbe, ctx: SourceContext) -> Iterable[tupl current_user_ad_object = ctx.resolve_principal(current_user_sid) if current_user_ad_object: logger.info("Found current user: %s (%s)", current_user_ad_object.get("sam_account_name"), current_user_sid) - target_entry = ctx.target_hosts_by_hostname.get(probe.hostname) + target_entry = ctx.target_hosts_by_hostname.get(probe.hostname.lower()) host_sid = target_entry.ad_object.get("object_sid") if (target_entry and target_entry.ad_object) else None if host_sid is None: # No resolved host AD object — HasSession can't be built for this row downstream; keep the row but log. @@ -663,7 +663,7 @@ def get_ntlm_settings(probe: _RegistryProbe, ctx: SourceContext) -> Iterable[tup disable_loopback_check = disable_loopback_reg == 1 logger.verbose(f"DisableLoopbackCheck is {'enabled' if disable_loopback_check else 'disabled'}") - target_entry = ctx.target_hosts_by_hostname[probe.hostname] + target_entry = ctx.target_hosts_by_hostname[probe.hostname.lower()] row = { **(target_entry.ad_object or {}), "source": "RemoteRegistry-NTLMSettings", @@ -869,7 +869,7 @@ def get_mssql_settings(probe: _RegistryProbe, ctx: SourceContext) -> Iterable[tu probe, instance_names ) - target_entry = ctx.target_hosts_by_hostname[probe.hostname] + target_entry = ctx.target_hosts_by_hostname[probe.hostname.lower()] yield "remoteregistry_mssql_servers", { "source": "RemoteRegistry-MSSQL", diff --git a/tests/registry_collect_test.py b/tests/registry_collect_test.py index bb708a7..2581a43 100644 --- a/tests/registry_collect_test.py +++ b/tests/registry_collect_test.py @@ -188,6 +188,41 @@ def test_none_result_does_not_crash(monkeypatch): assert [row for table, row in rows if table == "remoteregistry_users"] == [] +# --- target_hosts_by_hostname case sensitivity --------------------------------- + +def test_mixed_case_target_does_not_kill_the_registry_phase(monkeypatch): + """Regression: target_hosts_by_hostname is keyed by lowercased hostname + (context.py), but collect_registry is called with whatever case the target + was actually discovered in -- e.g. LDAP name-pattern matches return the AD + computer's own casing ('MECM.sccm.lab'), not lowercase. Every lookup against + that dict must lower() its key first, or get_ntlm_settings raises KeyError + before ever reaching the low-privilege-readable site-code / component-server + keys later in the same generator -- silently losing the whole RemoteRegistry + phase for that host, not just the NTLM-settings row.""" + mixed_case_target = "PS1-PSS.MAYYHEM.COM" + ctx = FakeCtx() + ctx.target_hosts_by_hostname = {TARGET: _Entry(ad_object={"name": "PS1-PSS"})} + + def _fake_probe(*a, **k): + probe = FakeProbe(_site_server(**{COMPONENTS: ["PS1-CCM.MAYYHEM.COM"]}), []) + probe.hostname = mixed_case_target + return probe + + monkeypatch.setattr(registry, "_RegistryProbe", _fake_probe) + + rows = list(registry.collect_registry(mixed_case_target, ctx)) + + # Reached past get_ntlm_settings (no KeyError) and all the way to the + # component-server site-server row -- the exact data this bug was blocking. + site_server_rows = [ + row for table, row in rows + if table == "remoteregistry_computers" + and row["source"] == "RemoteRegistry-ComponentServers" + and row["sccm_site_system_roles"] == ["SMS Site Server@PS1"] + ] + assert len(site_server_rows) == 1 + + # --- Multisite Component Servers ---------------------------------------------- def test_multisite_empty_key_marks_local_site_database(monkeypatch): From fe040f0fa58395e68d182141d2a3ad7e8be6da05 Mon Sep 17 00:00:00 2001 From: Mayyhem Date: Wed, 30 Sep 2026 17:34:52 -0700 Subject: [PATCH 2/2] Preserve host identity across name and case changes, address PRs #8 and #9 --- ARCHITECTURE.md | 10 ++- README.md | 5 ++ src/openhound_sccm/collectors/registry.py | 28 ++++---- src/openhound_sccm/context.py | 14 ++-- tests/per_host_wiring_test.py | 18 +++++ tests/registry_collect_test.py | 80 ++++++++++++++--------- tests/registry_current_user_test.py | 2 +- 7 files changed, 106 insertions(+), 51 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7f00759..f50bed1 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -383,17 +383,20 @@ so it can't stop early. Two more pieces wire discovery into this loop: -- [`SourceContext.register_target`](src/openhound_sccm/context.py#L260-L358) is the single funnel every +- [`SourceContext.register_target`](src/openhound_sccm/context.py#L356-L455) is the single funnel every discovery path calls. It resolves the identifier against AD (best-effort), applies the allow-list (see [§4](#4-targeted-collection-an-include-only-allow-list)), dedups by SID then hostname (with an FQDN upgrade path), merges sources/site-codes onto an existing `TargetEntry`, and — for a genuinely new host — - calls `work_queue.submit(...)` ([context.py:355-356](src/openhound_sccm/context.py#L355-L356)). Both + calls `work_queue.submit(...)` ([context.py:453-454](src/openhound_sccm/context.py#L453-L454)). Both Stage-1 discovery resources (which call it from inside `collectors/*`) and the CLI's `--computers` seeds ([main.py:963-965](src/openhound_sccm/main.py#L963-L965)) go through this *same* funnel, so dedup and filtering are identical. -- [`target_hosts_snapshot`](src/openhound_sccm/context.py#L361-L369) lets phases read the *current* target +- [`target_hosts_snapshot`](src/openhound_sccm/context.py#L459-L471) lets phases read the *current* target set at iteration time, so a host discovered after a phase started is still picked up — the OpenHound equivalent of CMBP's "iterate the updated list." + When a short hostname is upgraded to an FQDN, the old name remains an index alias for a queued worker, + while the snapshot returns the target only once. If a RemoteRegistry worker has no registered target, + it warns and emits the available rows without a host AD SID. One discovery path recurses on its own, one level below `register_target`: [`_expand_group_targets`](src/openhound_sccm/collectors/ldap.py#L618-L661) (ope-e191). When @@ -2084,6 +2087,7 @@ took a separate, manual step outside `openhound collect sccm`. | Date | Change | |---|---| +| 2026-09-30 | **Preserved target identity across hostname upgrades and incomplete RemoteRegistry discovery** (§3). A queued short hostname remains an alias when its target is upgraded to an FQDN; snapshots deduplicate the aliases. Registry lookups use normalized hostnames and emit partial rows with a warning if a target is genuinely absent, instead of failing the phase. Regression tests cover mixed-case names, the queued-name upgrade, and missing targets. | | 2026-08-02 | **A healthy low-privilege run stopped emitting 146 WARNINGs** (§7; con-81c2). Every one of them was the collector working as designed — the same failure mode the 2026-08-01 entry fixed one severity level up, because an operator who learns to ignore the file cannot find the 23 real entries in it. Measured against `out/ab-lowpriv-pe-off/collect_issues_20260801_230714.log` (nine-host mayyhem.com lab, plain domain user, `--run-all`). **(1) Transport tables that were never collected.** `_sccm_expected_miss` gained a third case: a missing `wmi_`/`adminservice_` table whose sibling is *also* absent is DEBUG when no privileged table holds any **rows**, because then the AdminService/WMI phases collected nothing. That was **106 of the 146**. Rows and not mere existence, which the first cut of this got wrong and the lab caught: dlt writes a resource's schema even when it yields zero rows, so a lowpriv run into a *reused* output directory carried `adminservice_client_devices` and `adminservice_site_definitions` at 0 rows, an existence check read that as "a privileged transport ran", and 98 expected misses stayed at WARNING. `--clean` runs hid the bug because a fresh catalog has no privileged tables at all. `_privileged_transport_ran` is pre-existing code shared with the `http_`/`smb_` fallback branch, which had the same blindness, so the fix can legitimately surface `http_`/`smb_` warnings that were previously swallowed — two tests that seeded an *empty* privileged table to mean "a transport ran" now insert a row. Across all 18 lab collections the split is binary (privileged: 13 tables, 13 populated; unprivileged: none populated), so partial privilege — where some classes return rows and others are ACL-denied — has never occurred and is deliberately still a WARNING. It stays a WARNING when some privileged tables did land — the case the old `test_safe_no_sibling_logs_warning` was really protecting. That test asserted the opposite for the no-transport case and was inverted deliberately, and a new test pins the privileged-transport-ran half. **(2) Probe negatives.** A host answering the AdminService probe with 404 is not an SMS Provider, and a host refusing the HTTP probe does not serve that endpoint; both are discovery results, not faults. The 404 drops to VERBOSE because `privileged.py` already logs an INFO conclusion (`is not a reachable AdminService provider; skipping`) immediately after it, so INFO would state the same fact twice per host. The HTTP connect failure drops to INFO rather than VERBOSE because `http.py` has **no** conclusion line — below INFO a host that served nothing would read as a clean collection. 401/403/500 are untouched: a provider that exists and refuses you is a finding. **(3) The RemoteRegistry roll-up lists keys, superseding the capability names introduced 2026-08-01.** `_DENIED_CAPABILITIES` and `_capability_for` are deleted; the message is the distinct denied key paths one per line, counted by distinct key so the number always matches the list (several values can be refused under one key — `_read_mssql_service_state` reads `Start` and `ObjectName` under one service key). The "requires local Administrators, re-run as admin" prose moved to README's low-privilege section, which said it already. Both RemoteRegistry warnings and the three per-read lines also dropped the trailing `on `: `LogContextFilter` prefixes `[target][phase]` on every record a per-host phase emits, so it was always duplication. **(4) impacket's Python 3.14 SyntaxWarning**, from a `return` inside a `finally` at `mssql/version.py:182`, is silenced in `openhound-collector-common` 0.1.4 by a scoped `catch_warnings()` around the `from impacket import ntlm, tds` that triggers the compile — scoped, so a return-in-finally in our own code still reports. `module=` filters do **not** work on compile-time warnings: CPython calls `warn_explicit()` with `module=None` and `warnings.py` derives the module from the file path, not the dotted name. The same release shortened the skipped-transform log from a five-line DuckDB exception dump (spell-check guess + SQL echo + caret) to one line, with the full text kept on a companion DEBUG only when the miss is unexpected. Floor raised to `>=0.1.4`. | | 2026-08-01 | **The `--run-all` summary now names the archive that was written, not one rebuilt from the name we asked for** (§12; con-a4ec, closing the shared-library gap con-8a28 opened). Adopts `openhound-collector-common` **0.1.3** (floor raised `>=0.1.2` → `>=0.1.3`, cap unchanged), whose `run_end_to_end` returns the archive `zip_graph_output` produced on the new `StagePaths.graph_zip` instead of discarding it — `None` when convert emitted no `*.json`. `_log_all_output_locations` loses its `graph_zip_name` parameter and reads the field; the name now travels one way only, from `collect_sccm` into the chain. The reconstruction it replaces (`graph_out / ` plus an existence test) could disagree with reality — a stale archive from an earlier run into the same output directory is the obvious case, and it is exactly the shape `--clean` exists to manage — and only the returned path is evidence. Two tests pin the distinction by putting a *differently named* archive in the graph dir alongside the reported one. Stale floor references in ARCHITECTURE (`>=0.1.0`, and a §12 note saying the 0.1.1 bump was still deferred), README and PUBLISHING corrected to `>=0.1.3` at the same time. | | 2026-08-01 | **The registry arm could not see a named SQL Server instance, and the warning that said so was camouflaged by six false ones** (con-ab59). `get_mssql_settings` probed eight hard-coded `SuperSocketNetLib` paths, every one ending `.MSSQLSERVER` — so a *named* instance could never match. The lab's `ps1-sec` runs the SEC site database as `CONFIGMGRSEC` (SCCM's own site definition: `SQLDatabaseName 'CONFIGMGRSEC\CM_SEC'`, port 1433) and was missed **on a fully privileged run**: eight `not found`s, then "Could not access any MSSQL registry paths". That line was a true positive sitting among six identical ones from hosts with genuinely no SQL, which is why it read as noise — and is why the fix was not to quieten it. Paths are now **derived from SQL Server's own inventory** (`Instance Names\SQL`), whose value *data* is the instance's subkey (`MSSQL16.CONFIGMGRSEC`); the settings path is therefore exact, with no version prefix to guess and no default-instance assumption. Eight hard-coded strings collapse to one derivation plus a single pre-2000 fallback (`SOFTWARE\Microsoft\MSSQLServer\...`, which predates the inventory key and had only a default instance), and the duplicate second read of `Instance Names\SQL` further down the function goes with them — one read now serves both the candidate paths and the reported `instance_names`. Severity was moderate rather than urgent because the separate MSSQL phase covers a *reachable* instance (the same run logs `port 1433 is open` / `EPA testing … via SSPI`); the gap bit a firewalled instance, and `service_start_type` / `service_account_name`, which the live probe does not supply — note `_mssql_service_name` already handled `MSSQL$` correctly and was simply unreachable behind the early return. The warning now fires only on the contradiction worth hearing: the inventory lists instances *and* none of their settings keys can be read. No inventory at all means no SQL Server, the normal case (seven of nine lab hosts, privileged), and logs at verbose. **A test fixture was complicit:** `_mssql_probe` seeded the inventory as `[(instance, instance)]`, putting the instance *name* where the registry stores the *subkey*, so `test_mssql_named_instance_uses_the_dollar_service_name` passed against code that could not reach a named instance — a fake mirroring the code's belief rather than the system's behaviour. Fixture corrected, 8 tests added. Suite 1025 passed / 5 skipped. | diff --git a/README.md b/README.md index b51fde5..02030b7 100644 --- a/README.md +++ b/README.md @@ -702,6 +702,11 @@ readable after the fact without re-running. `collect_issues_.log` hol errors, each with a traceback, and is not created at all by a clean run. `--debug` additionally folds the `dlt`/`ldap3` internals into the full log. +If a RemoteRegistry host is missing from the target index, the issues log warns that its rows will +lack host AD identity. The collector still writes the registry values it can read, but rows without +a host SID cannot support identity-based edges such as `HasSession`. A short hostname that is later +resolved to an FQDN stays indexed under both names, so normal discovery retains that identity. + #### What a low-privilege run looks like Collecting as a plain domain user is a supported, first-class mode — see diff --git a/src/openhound_sccm/collectors/registry.py b/src/openhound_sccm/collectors/registry.py index c43e5c5..d141312 100644 --- a/src/openhound_sccm/collectors/registry.py +++ b/src/openhound_sccm/collectors/registry.py @@ -465,9 +465,10 @@ def collect_registry(target: str, ctx: "SourceContext") -> Iterable[tuple[str, d # server name only when AD resolution failed (ad_object # is None or lacks a name). logger.info("Found %s, this target is a site server", SCCM_REG_KEYS["component_servers"]) - target_entry = ctx.target_hosts_by_hostname[target.lower()] + target_entry = ctx.target_hosts_by_hostname.get(target.lower()) + ad_object = target_entry.ad_object if target_entry else None row = { - **(target_entry.ad_object or {}), + **(ad_object or {}), "source": "RemoteRegistry-ComponentServers", "sccm_infra": True, "sccm_site_system_roles": _roles(["SMS Site Server"], site_code), @@ -529,9 +530,10 @@ def collect_registry(target: str, ctx: "SourceContext") -> Iterable[tuple[str, d "database lives elsewhere. Emitting the SQL Server role as assumed " "pending corroboration in preprocess.", target, ) - target_entry = ctx.target_hosts_by_hostname[target.lower()] + target_entry = ctx.target_hosts_by_hostname.get(target.lower()) + ad_object = target_entry.ad_object if target_entry else None row = { - **(target_entry.ad_object or {}), + **(ad_object or {}), "source": "RemoteRegistry-MultisiteComponentServers", "sccm_infra": True, "sccm_site_system_roles": _roles(["SMS SQL Server", "SMS Site Server"], site_code), @@ -663,16 +665,19 @@ def get_ntlm_settings(probe: _RegistryProbe, ctx: SourceContext) -> Iterable[tup disable_loopback_check = disable_loopback_reg == 1 logger.verbose(f"DisableLoopbackCheck is {'enabled' if disable_loopback_check else 'disabled'}") - target_entry = ctx.target_hosts_by_hostname[probe.hostname.lower()] + target_entry = ctx.target_hosts_by_hostname.get(probe.hostname.lower()) + if target_entry is None: + logger.warning("No registered target for %s; RemoteRegistry rows will lack host AD identity", probe.hostname) + ad_object = target_entry.ad_object if target_entry else None row = { - **(target_entry.ad_object or {}), + **(ad_object or {}), "source": "RemoteRegistry-NTLMSettings", "smb_signing_required": signing_required, "smb_signing_source": signing_source, "restrict_receiving_ntlm_traffic": restrict_receiving_ntlm_traffic, "disable_loopback_check": disable_loopback_check, } - row.setdefault("name", target_entry.ad_object.get("name") if target_entry.ad_object else probe.hostname) + row.setdefault("name", ad_object.get("name") if ad_object else probe.hostname) yield "remoteregistry_computers", row @@ -869,16 +874,17 @@ def get_mssql_settings(probe: _RegistryProbe, ctx: SourceContext) -> Iterable[tu probe, instance_names ) - target_entry = ctx.target_hosts_by_hostname[probe.hostname.lower()] + target_entry = ctx.target_hosts_by_hostname.get(probe.hostname.lower()) + ad_object = target_entry.ad_object if target_entry else None yield "remoteregistry_mssql_servers", { "source": "RemoteRegistry-MSSQL", "force_encryption": force_encryption if force_encryption is not None else None, "extended_protection": extended_protection if extended_protection is not None else None, - "name": target_entry.ad_object.get("name") if target_entry.ad_object else probe.hostname, - "domain_computer_sid": target_entry.ad_object.get("object_sid") if target_entry.ad_object else None, + "name": ad_object.get("name") if ad_object else probe.hostname, + "domain_computer_sid": ad_object.get("object_sid") if ad_object else None, "port": port if port else None, "instance_names": instance_names if instance_names else None, "service_start_type": service_start_type, "service_account_name": service_account_name, - } \ No newline at end of file + } diff --git a/src/openhound_sccm/context.py b/src/openhound_sccm/context.py index 17523cc..39603f7 100644 --- a/src/openhound_sccm/context.py +++ b/src/openhound_sccm/context.py @@ -99,7 +99,7 @@ class SourceContext: # at iteration time so late additions are picked up. # # Two parallel indexes: - # target_hosts_by_hostname — always populated; key = lowercased canonical hostname + # target_hosts_by_hostname — lowercased canonical hostname and prior short-name aliases # _target_hosts_by_sid — only when SID available; key = objectSid string # Both dicts hold references to the same entry dicts, so a mutation via # either index is immediately visible via the other. @@ -413,10 +413,10 @@ def register_target( if existing is not None: logger.verbose(f"Already registered target: {existing.ad_object.get('dns_host_name') if existing.ad_object else existing.hostname}") - # FQDN upgrade: re-key target_hosts_by_hostname + # Keep the queued short name as an alias when upgrading to an FQDN. + # A worker may still receive that name after this entry is upgraded. if "." in canonical_lower and "." not in existing.hostname.lower(): logger.verbose("Upgrading hostname %r -> %r", existing.hostname, canonical) - del self.target_hosts_by_hostname[existing.hostname.lower()] existing.hostname = canonical self.target_hosts_by_hostname[canonical_lower] = existing # Backfill ad_object + SID index if we now have one @@ -457,11 +457,15 @@ def register_target( def target_hosts_snapshot(self) -> list: - """Return the current list of probe targets (TargetEntry objects) as a copy. + """Return each current probe target once, even when it has a short-name alias. Per-host resources iterate this so late-registered hosts are picked up by phases that haven't started yet. Thread-safe against concurrent ``register_target`` mutations. """ with self._ensure_target_lock(): - return list(self.target_hosts_by_hostname.values()) \ No newline at end of file + unique = { + entry.hostname.lower(): entry + for entry in self.target_hosts_by_hostname.values() + } + return list(unique.values()) diff --git a/tests/per_host_wiring_test.py b/tests/per_host_wiring_test.py index c36d7d8..08fbdd7 100644 --- a/tests/per_host_wiring_test.py +++ b/tests/per_host_wiring_test.py @@ -72,6 +72,24 @@ def test_register_existing_target_does_not_resubmit(): assert wq.submitted == ["hostA"] # submitted exactly once +def test_fqdn_upgrade_keeps_queued_short_name_as_alias(): + wq = FakeWorkQueue() + ctx = _ctx(work_queue=wq) + short = ctx.register_target( + "HOST", source="first", ad_object={"name": "HOST", "object_sid": "S-1-2"}, + ) + fqdn = ctx.register_target( + "HOST.example.com", source="second", + ad_object={"name": "HOST", "dns_host_name": "HOST.example.com", "object_sid": "S-1-2"}, + ) + + assert fqdn is short + assert wq.submitted == ["HOST"] + assert ctx.target_hosts_by_hostname[wq.submitted[0].lower()] is short + assert ctx.target_hosts_by_hostname["host.example.com"] is short + assert ctx.target_hosts_snapshot() == [short] + + def test_allow_list_blocks_non_listed_discoveries(): wq = FakeWorkQueue() ctx = _ctx(allowed={"hosta"}, work_queue=wq) diff --git a/tests/registry_collect_test.py b/tests/registry_collect_test.py index 2581a43..57518f5 100644 --- a/tests/registry_collect_test.py +++ b/tests/registry_collect_test.py @@ -20,6 +20,7 @@ TRIGGERS = registry.SCCM_REG_KEYS["triggers"] COMPONENTS = registry.SCCM_REG_KEYS["component_servers"] MULTISITE = registry.SCCM_REG_KEYS["multisite_component_servers"] +CURRENT_USER = registry.SCCM_REG_KEYS["current_user"] class _Entry: @@ -190,37 +191,54 @@ def test_none_result_does_not_crash(monkeypatch): # --- target_hosts_by_hostname case sensitivity --------------------------------- -def test_mixed_case_target_does_not_kill_the_registry_phase(monkeypatch): - """Regression: target_hosts_by_hostname is keyed by lowercased hostname - (context.py), but collect_registry is called with whatever case the target - was actually discovered in -- e.g. LDAP name-pattern matches return the AD - computer's own casing ('MECM.sccm.lab'), not lowercase. Every lookup against - that dict must lower() its key first, or get_ntlm_settings raises KeyError - before ever reaching the low-privilege-readable site-code / component-server - keys later in the same generator -- silently losing the whole RemoteRegistry - phase for that host, not just the NTLM-settings row.""" - mixed_case_target = "PS1-PSS.MAYYHEM.COM" - ctx = FakeCtx() - ctx.target_hosts_by_hostname = {TARGET: _Entry(ad_object={"name": "PS1-PSS"})} - - def _fake_probe(*a, **k): - probe = FakeProbe(_site_server(**{COMPONENTS: ["PS1-CCM.MAYYHEM.COM"]}), []) - probe.hostname = mixed_case_target - return probe - - monkeypatch.setattr(registry, "_RegistryProbe", _fake_probe) - - rows = list(registry.collect_registry(mixed_case_target, ctx)) - - # Reached past get_ntlm_settings (no KeyError) and all the way to the - # component-server site-server row -- the exact data this bug was blocking. - site_server_rows = [ - row for table, row in rows - if table == "remoteregistry_computers" - and row["source"] == "RemoteRegistry-ComponentServers" - and row["sccm_site_system_roles"] == ["SMS Site Server@PS1"] - ] - assert len(site_server_rows) == 1 +def _mixed_case_probe(): + instance_key = "MSSQL13.CONFIGMGRSEC" + probe = FakeProbe( + enum_results=_site_server(**{COMPONENTS: ["PS1-PSS.MAYYHEM.COM"], MULTISITE: []}), + values_by_key={ + CURRENT_USER: [("UserSID", "S-1-USER")], + INSTANCE_NAMES: [("CONFIGMGRSEC", instance_key)], + _supersocket(instance_key): [], + }, + ) + probe.hostname = "PS1-PSS.MAYYHEM.COM" + return probe + + +def test_mixed_case_target_preserves_all_registry_host_rows(monkeypatch): + """All five lookups find the lowercase index key and retain host identity.""" + ctx = FakeCtx(resolve_result={"object_sid": "S-1-USER"}, register_result=None) + ctx.target_hosts_by_hostname = { + TARGET: _Entry(ad_object={"name": "PS1-PSS", "object_sid": "S-1-HOST"}), + } + monkeypatch.setattr(registry, "_RegistryProbe", lambda *a, **k: _mixed_case_probe()) + + rows = list(registry.collect_registry("PS1-PSS.MAYYHEM.COM", ctx)) + + assert ("remoteregistry_sites", {"source": "RemoteRegistry-Triggers", "site_code": "PS1"}) in rows + host_rows = [row for table, row in rows if table == "remoteregistry_computers"] + assert {row["source"] for row in host_rows} == { + "RemoteRegistry-NTLMSettings", "RemoteRegistry-ComponentServers", + "RemoteRegistry-MultisiteComponentServers", + } + assert all(row["object_sid"] == "S-1-HOST" for row in host_rows) + assert next(row for table, row in rows if table == "remoteregistry_users")["host_object_sid"] == "S-1-HOST" + assert next(row for table, row in rows if table == "remoteregistry_mssql_servers")["domain_computer_sid"] == "S-1-HOST" + + +def test_missing_target_emits_partial_rows_and_warns(monkeypatch, caplog): + ctx = FakeCtx(resolve_result={"object_sid": "S-1-USER"}, register_result=None) + ctx.target_hosts_by_hostname = {} + monkeypatch.setattr(registry, "_RegistryProbe", lambda *a, **k: _mixed_case_probe()) + + with caplog.at_level(logging.WARNING, logger="openhound_sccm.collectors.registry"): + rows = list(registry.collect_registry("PS1-PSS.MAYYHEM.COM", ctx)) + + assert any(table == "remoteregistry_sites" for table, _ in rows) + assert any(table == "remoteregistry_computers" for table, _ in rows) + assert next(row for table, row in rows if table == "remoteregistry_users")["host_object_sid"] is None + assert next(row for table, row in rows if table == "remoteregistry_mssql_servers")["domain_computer_sid"] is None + assert sum("No registered target" in record.getMessage() for record in caplog.records) == 1 # --- Multisite Component Servers ---------------------------------------------- diff --git a/tests/registry_current_user_test.py b/tests/registry_current_user_test.py index 938414c..5ab4cb0 100644 --- a/tests/registry_current_user_test.py +++ b/tests/registry_current_user_test.py @@ -4,7 +4,7 @@ class _FakeProbe: - hostname = "host1.lab" + hostname = "HOST1.lab" def read_values(self, _key): return [("UserSID", "S-1-5-21-1-2-3-1106"), ("Session", 1)]