fix(http): stop trusting ambient system/environment proxy config

.tickets/_TICKETS-BY-STATUS.md

@@ -5,7 +5,7 @@ Regenerate after any status change rather than editing by hand.
 `.gitattributes` marks this file `merge=ours`, which needs a one-time
 `git config merge.ours.driver true` per clone.
 
-**148 tickets** — 6 in progress · 29 open · 113 closed
+**149 tickets** — 6 in progress · 29 open · 114 closed
 
 ## In progress (6)
 
@@ -52,13 +52,14 @@ Regenerate after any status change rather than editing by hand.
 | [`Ope-exvi`](Ope-exvi.md) | 3 | feature | Findings / Remediations | sccm, findings, reporting |
 | [`ope-4ba1`](ope-4ba1.md) | 4 | task | Make shared AdClient credential-summary warning flag-name-agnostic (do not name collector-specific CLI flags) | shared-lib, openhound-collector-common, ldap, logging, auth |
 
-## Closed (113)
+## Closed (114)
 
 | Ticket | P | Type | Title | Tags |
 |---|---|---|---|---|
 | [`con-3354`](con-3354.md) | 1 | bug | Site-code conflict discards a real secondary site (one scalar site_code per target) | sccm, context, site-discovery, data-loss |
 | [`con-401c`](con-401c.md) | 1 | bug | Collector console log output does not reach redirected stdout | logging, diagnostics, cli |
 | [`con-7296`](con-7296.md) | 1 | task | Emit MSSQL nodes/edges as source_kind=MSSQL payload + zip graph output on --run-all | sccm, mssql, convert, packaging |
+| [`con-ba1f`](con-ba1f.md) | 1 | bug | HTTP client trusted the ambient system/environment proxy | real-env, http, proxy |
 | [`con-c522`](con-c522.md) | 1 | task | Clear 14 Dependabot alerts via lock-only dependency bump | security, dependencies |
 | [`ope-0495`](ope-0495.md) | 1 | task | SCCM collector vs live CMBP unit-test parity gaps (2026-07-14 comparison) | — |
 | [`ope-1201`](ope-1201.md) | 1 | bug | node_mssql_server drops registry-only SQL servers (coalesce port VARCHAR vs INTEGER_LITERAL) | sccm, preproc, mssql, dlt |

.tickets/con-ba1f.md

@@ -0,0 +1,41 @@
+---
+id: con-ba1f
+status: closed
+deps: []
+links: []
+created: 2026-09-08T00:00:00Z
+type: bug
+priority: 1
+tags: [real-env, http, proxy]
+---
+
+# HTTP client trusted the ambient system/environment proxy
+
+Found during a real-environment (non-lab) assessment: every AdminService/HTTP request,
+including same-LAN, same-domain targets, got a `ProxyError` connect-timeout. All of them
+tried to route through the operator's corporate web proxy (configured for general
+internet access), which has no route to internal hosts.
+
+Root cause: `HttpClient.__init__` (`clients/http.py`) creates `requests.Session()` with
+no override, and `requests` trusts ambient proxy config by default — env vars, or on
+Windows the registry-configured system proxy via `urllib.request.getproxies_registry()`.
+
+This collector already has its own explicit, intentional pivoting mechanism, `-x`/`--proxy`
+(a SOCKS5 tunnel installed at the socket layer, per `main.py`). Silently trusting a random
+ambient corporate proxy was never intended, and broke a real engagement until worked
+around with a manually-set `NO_PROXY` environment variable.
+
+## Fix
+
+`self._session.trust_env = False` right after session construction. Confirmed safe
+against `--proxy`: the SOCKS5 mechanism patches `socket.socket`/`socket.create_connection`/
+`socket.getaddrinfo` process-wide and never touches `requests`' own proxy resolution, so
+the two don't interact.
+
+## Notes
+
+**2026-09-08T00:00:00Z**
+
+Fixed and tested. New test `test_session_does_not_trust_ambient_proxy_env` in
+`tests/http_client_test.py`. README's "Proxying / pivoting" section updated to state
+ambient proxies are never used automatically.

ARCHITECTURE.md

@@ -2084,6 +2084,7 @@ took a separate, manual step outside `openhound collect sccm`.
 
 | Date | Change |
 |---|---|
+| 2026-09-03 | **The HTTP client trusted the ambient system/environment proxy, silently swallowing every AdminService/HTTP request on a box with a corporate proxy configured** (con-ba1f). Reported symptom: a real (non-lab) engagement got a `ProxyError` connect-timeout on every single AdminService/HTTP target, including same-LAN, same-domain hosts — the requests all tried to route through the operator's corporate web proxy (configured for general internet access), which has no route to internal hosts. Root cause: `HttpClient.__init__` (`clients/http.py`) created `requests.Session()` with no override, and `requests` trusts ambient proxy config by default — env vars, or on Windows the registry-configured system proxy via `urllib.request.getproxies_registry()`. This collector already has its own explicit, intentional pivoting mechanism, `-x`/`--proxy` (a SOCKS5 tunnel installed at the socket layer by `openhound_collector_common.proxy.patch`, per `main.py`) — silently trusting a random ambient corporate proxy was never intended and broke a real engagement until worked around with a manually-set `NO_PROXY` env var. Fix: `self._session.trust_env = False` right after session construction. Confirmed safe against `--proxy`: the SOCKS5 mechanism patches `socket.socket`/`socket.create_connection`/`socket.getaddrinfo` process-wide and never touches `requests`' own proxy resolution, so the two don't interact. New test `test_session_does_not_trust_ambient_proxy_env` in `tests/http_client_test.py`; README's "Proxying / pivoting" section updated to state ambient proxies are never used automatically. |
 | 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 <host>`: `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 / <requested name>` 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$<name>` 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. |

README.md

@@ -817,6 +817,13 @@ the KDC, and that traffic never touches our sockets. To use a logged-in identity
 through the pivot, export its Kerberos ticket and pass `--ticket`, or set up
 OS-level transparent proxying (tun2socks / Proxifier) on the outside box.
 
+**Ambient system/environment proxies are never used automatically.** `--proxy`
+above is the only way to route this collector's traffic through a proxy. The
+HTTP client explicitly disables `requests`' default trust of `HTTP_PROXY`/
+`HTTPS_PROXY` env vars and (on Windows) the registry-configured system proxy —
+a box configured with a corporate web proxy for general internet access will
+still reach internal AdminService/HTTP targets directly, not through that proxy.
+
 ---
 
 # Automating the Upload

src/openhound_sccm/clients/http.py

@@ -124,6 +124,15 @@ def __init__(
         self._kerberos_negotiator: Any = None
 
         self._session = requests.Session()
+        # Never trust ambient proxy config (env vars, or on Windows the
+        # registry-configured system proxy) -- this collector's own pivoting
+        # mechanism is the explicit --proxy SOCKS5 flag, which tunnels at the
+        # socket layer (openhound_collector_common.proxy.patch) and is
+        # unaffected by this setting. Left at the default, a corporate web
+        # proxy configured for general internet access silently swallowed
+        # every AdminService/HTTP request -- including same-LAN, same-domain
+        # targets -- with a ProxyError timeout, on a real engagement.
+        self._session.trust_env = False
         self._session.verify = verify_ssl
         self._session.headers.update({"Accept": "application/json"})
 

tests/http_client_test.py

@@ -49,6 +49,20 @@ def _client(session, auth=AuthMode.NONE, **kw):
     return c
 
 
+def test_session_does_not_trust_ambient_proxy_env(monkeypatch):
+    # A real engagement had every AdminService/HTTP request -- including
+    # same-LAN, same-domain targets -- silently routed through a corporate
+    # web proxy and timing out, because requests.Session() trusts ambient
+    # proxy config (env vars, or on Windows the registry system proxy) by
+    # default. This collector's own pivoting mechanism is the explicit
+    # --proxy SOCKS5 flag (socket-layer, see openhound_collector_common.proxy),
+    # so ambient proxy trust must stay off regardless of what's set here.
+    monkeypatch.setenv("HTTPS_PROXY", "http://corporate-proxy.example.com:8080")
+    monkeypatch.setenv("HTTP_PROXY", "http://corporate-proxy.example.com:8080")
+    c = HttpClient(base_url="https://mp.mayyhem.com", auth=AuthMode.NONE, domain="mayyhem.com")
+    assert c._session.trust_env is False
+
+
 def test_none_mode_never_sends_authorization():
     sess = mock.Mock()
     sess.get.return_value = _FakeResp(401, b"deny")