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 180353f..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] + 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] + 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), @@ -600,7 +602,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,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] + 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] + 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 bb708a7..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: @@ -188,6 +189,58 @@ 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 _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 ---------------------------------------------- def test_multisite_empty_key_marks_local_site_database(monkeypatch): 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)]