diff --git a/changelog.d/2530.security.md b/changelog.d/2530.security.md new file mode 100644 index 000000000..4f9df3ce9 --- /dev/null +++ b/changelog.d/2530.security.md @@ -0,0 +1,13 @@ +- **The AD hop follows no LDAP referral.** ldap3 follows one by default, and on a bound connection + it binds to the referred host with the same service-account password, over a TLS setup without the + pinned CA or the narrowed suites, or over plain `ldap://`. A first deployment would have sent that + password to whatever host one referral named. Every `ldap3.Connection` the engine builds now sets + `auto_referrals=False`, and its `ldap3.Server` sets `allowed_referral_hosts=[]`. A referral result + to a search or to the user bind is refused as a directory error naming only the referred hosts, + and one to the service-account bind fails that bind as before. Sign-in audits it as + `auth.login_error`, and the session reconciler never revokes on it. A site whose users or groups + span several domains of a forest would need a global catalog, or a search base in the bound + controller's own domain. The `BACKLOG #2494` TLS-context entry says a followed referral still gets + a plain ldap3 context. This supersedes that note: no referral is followed, so no referred hop is + opened. See [ADR 0180](../docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) + Amendment F. (`BACKLOG #2530`) diff --git a/changelog.d/2546.fixed.md b/changelog.d/2546.fixed.md new file mode 100644 index 000000000..025d16ab1 --- /dev/null +++ b/changelog.d/2546.fixed.md @@ -0,0 +1,8 @@ +- **AD sign-in no longer fails on Linux over the receive timeout.** The engine passed + `[auth].ad_receive_timeout`, a float, straight to ldap3. On every non-Windows host ldap3 packs + that value as an integer. So each AD socket open would have raised `struct.error` after the TCP + connect and before the bind was sent. On a first Linux deployment, AD sign-in would have failed + for every user. Windows was not affected. The engine now passes ldap3 the timeout rounded up to + whole seconds, so it is never shorter than configured. Both AD timeouts are also refused at + config load above 3600 seconds. That cap keeps them far below the point where a socket timeout + overflows, which would fail sign-in outside the audited error path. (`BACKLOG #2546`) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 6f6075592..c58fc48b6 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -665,8 +665,8 @@ document ([SECURITY-DOCS-POLICY.md](SECURITY-DOCS-POLICY.md)). | `ad_tls_ca_cert_file` | str | — | trust an internal CA for LDAPS without disabling verification | | `ad_tls_ca_cert_pin` | str | — | optional lowercase-hex SHA-256 pin over the corresponding CA anchor PEM (`ad_tls_ca_cert_file`); a mismatch refuses at load + reload (ASVS 6.7.1); unset = no pin (dormant); set but empty or whitespace refuses at load | | `ad_allow_insecure_ldap` | bool | `false` | explicit opt-in to a non-`ldaps://` bind (trusted-network dev only) | -| `ad_connect_timeout` | float | `10.0` | seconds — bounds the LDAP/LDAPS **TCP connect** on every `ldap3` `Server` the authenticator builds (ASVS 13.1.3). Must be finite and `> 0`; `0`, negative, `inf` and `NaN` are refused at config load. `ldap3`'s own default is `None` (wait forever), so without this an unresponsive DC pinned a thread-pool worker indefinitely | -| `ad_receive_timeout` | float | `10.0` | seconds — bounds **each LDAP response read** (both binds and every search) on every `ldap3` `Connection`. Same finite-positive validation | +| `ad_connect_timeout` | float | `10.0` | seconds — bounds the LDAP/LDAPS **TCP connect** on every `ldap3` `Server` the authenticator builds (ASVS 13.1.3). Must be finite and `> 0`; `0`, negative, `inf` and `NaN` are refused at config load, and so is anything above `3600`. `ldap3`'s own default is `None` (wait forever), so without this an unresponsive DC pinned a thread-pool worker indefinitely | +| `ad_receive_timeout` | float | `10.0` | seconds — bounds **each socket receive** during an LDAP response (both binds and every search) on every `ldap3` `Connection`. Same validation, up to `3600`. The engine rounds it up to whole seconds before handing it to `ldap3`, so `9.25` acts as `10` | | `ad_session_recheck_seconds` | int | `300` | **Directory session reconciliation** ([ADR 0079](adr/0079-kerberos-idp-session-coordination.md) mechanism 2). How often to re-resolve directory principals holding **live** sessions and revoke those AD has disabled or deleted — without it, an AD disable does not take effect until the `[security].max_session_hours` cap (12 h). **`300` (five minutes) is the default** (ADR 0148 GIVEN 1 — the hardened path is the shipped path), floored at **60 s** (a pass costs one LDAP bind per signed-in directory user). `0` disables the loop and is a **loosening** once AD is on — `security_loosenings()` names it. The default is **inert without AD** (`should_reconcile()` also needs an LDAP client), so a non-AD deployment is unaffected; an **explicit** non-zero value without `ad_enabled` is still refused rather than left silently dead. | | `ad_session_recheck_strikes` | int | `2` | Consecutive passes a principal must fail to resolve before its sessions are revoked. *The search matched nothing* cannot tell *deleted* from *moved out of the search base*, so a single ambiguous result must never revoke; a set disabled bit and an unreadable `userAccountControl` strike the same way. A wave of unreadable answers is held instead of revoked, with no setting ([ADR 0195](adr/0195-brake-the-ad-session-reconciler-on-an-undetermined-useraccountcontrol-wave.md)). Range 1–10. | | `ad_session_recheck_max_users` | int | `200` | Per-pass bind budget. Beyond this, remaining users are picked up by later passes (least-recently-probed first), so a large estate degrades to a longer effective interval instead of a bind storm. | diff --git a/docs/PHI.md b/docs/PHI.md index 7f047894a..8baaf3229 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -880,7 +880,7 @@ audit chain or be false. | MLLP inbound/outbound | Plaintext by default; **MLLP-over-TLS (TLS 1.2+, server-cert verify + hostname, opt-in mTLS) when `tls=true`** `[BUILT — WP-13b]`. A non-loopback plaintext MLLP listener is **refused at startup** (exposed-gate, ADR 0002 §0) unless `tls=true` or `serve --allow-insecure-bind`. | — | | File connector | Plaintext `.hl7` on disk/share | Rely on volume/share encryption; SFTP later | | Engine API ↔ console | Loopback HTTP by default; off-loopback requires TLS — **in-process** (`[api].tls_cert_file`, WP-13a) **or upstream** at a trusted reverse proxy (`tls_terminated_upstream` + `trusted_proxies`, WP-15) `[BUILT]`. Upstream, the proxy-to-engine hop is plaintext unless `tls_cert_file` is set; the site secures it, and `serve` requires `plaintext_upstream_hop_acknowledged` (BACKLOG #1179). HSTS engages on `https`; forwarded headers are trusted only from `trusted_proxies`. | — | -| AD / LDAP auth | **LDAPS** with cert verification (`ad_tls_verify`) `[BUILT]` | — | +| AD / LDAP auth | **LDAPS** with cert verification (`ad_tls_verify`) `[BUILT]`. No LDAP referral is followed, so the bind credentials never leave this hop for a referred host; a referral refuses the sign-in (BACKLOG #2530, 2026-09-30). A multi-domain forest would need a global catalog or a search base in the bound controller's own domain. | — | | PostgreSQL / SQL Server backend | TLS-to-DB on by default (`[store].encrypt`), server cert **validated** (`trust_server_certificate=false`) `[BUILT]`. Trust a private/internal DB CA without disabling validation via `[store].ssl_root_cert` file-pin (Postgres CA-bundle, SQL Server ODBC 18.1+ `ServerCertificate` leaf-pin) **or** a Windows machine-store (`LocalMachine\Root`) CA import. | — | **Hard rule:** never bind the API to `0.0.0.0` (or any non-loopback interface) without TLS in front diff --git a/docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md b/docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md index a38109f6a..4a20431a6 100644 --- a/docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md +++ b/docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md @@ -1,6 +1,6 @@ # 0180 — Asserting TLS suites on a library that exposes no SSLContext -- **Status:** Accepted (amended 2026-09-03, extended 2026-09-04 — see Amendment A; amended 2026-09-26 by BACKLOG #2034 — see Amendment B; amended 2026-09-27 by BACKLOG #300 — see Amendment C; amended 2026-09-28 by BACKLOG #300 — see Amendment D; amended 2026-09-30 by BACKLOG #2494 — see Amendment E) +- **Status:** Accepted (amended 2026-09-03, extended 2026-09-04 — see Amendment A; amended 2026-09-26 by BACKLOG #2034 — see Amendment B; amended 2026-09-27 by BACKLOG #300 — see Amendment C; amended 2026-09-28 by BACKLOG #300 — see Amendment D; amended 2026-09-30 by BACKLOG #2494 — see Amendment E; amended 2026-09-30 by BACKLOG #2530 — see Amendment F) - **Date:** 2026-08-28 - **Related:** BACKLOG #1317 · `messagefoundry/config/tls_policy.py` (`harden_cipher_suites`, `build_asserted_https_handler`, `assert_ldap3_tls_suites`, `assert_hvac_tls_suites`) · `messagefoundry/auth/ldap.py` · `messagefoundry/config/secretprovider_vault.py` · `messagefoundry/store/keyprovider_vault.py` · `messagefoundry/store/crypto_transit.py` · `tests/test_tls_cipher_assertion_sites.py` · `.github/workflows/ci.yml` @@ -323,3 +323,25 @@ the post-handshake check. The engine now builds the LDAPS context itself, and an engine subclass of `ldap3.Tls` wraps each connection with it, so the replica and the `ciphers=` string of Amendment C are gone. ADR 0188's amendment of the same date records the change, what it keeps and what it does not cover. + +## Amendment F (2026-09-30) -- the AD hop follows no LDAP referral (BACKLOG #2530) + +Every context above guards one hop: the one to `[auth].ad_server`. ldap3 2.9.1 could leave it. By +default it follows a referral, and on a bound connection it binds to the referred host with the +same user and password (`strategy/base.py`, `create_referral_connection`). It builds a plain +`ldap3.Tls` for that hop from a few attributes, so the hop has no pinned CA bytes, none of the +narrowing, and no TLS at all for an `ldap://` referral. A first deployment would therefore send the +service-account password to whatever host one referral named. + +`messagefoundry/auth/ldap.py` now builds every `Connection` with `auto_referrals=False` and every +`Server` with `allowed_referral_hosts=[]`. Each alone stops the follow, and +`tests/test_ldap_referrals.py` measures both arms against loopback servers. A referral result +(resultCode 10) to a search or to the user bind is now an `LdapError` that names the referred hosts +and nothing else from the URL. Sign-in audits it as `auth.login_error`, and the session reconciler +reads it as unavailable, so it never revokes. A search continuation reference is not a referral +result: ldap3 never follows one, and it still reads as no entry from that subtree. + +A site whose users or groups live in more than one domain of a forest would need a global catalog, +or a search base in the bound controller's own domain, instead of referrals. A global catalog +carries the membership of universal groups only, so roles mapped to another domain's domain-local +or global groups would not resolve through it. diff --git a/docs/adr/README.md b/docs/adr/README.md index a6b3bbd93..8642d6450 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -204,7 +204,7 @@ what is withheld and what you can request. | [0177](0177-rule-3c-governs-a-repository-by-identity-not-by-path-prefix.md) | **Rule 3c governs a repository by identity, not by path prefix** (BACKLOG #1067) -- the worktree gate decided *"is this repository governed"* with an equality-or-slash-prefix test comparing the target's git COMMON DIR against each allowlisted root's WORKING TREE path, so path containment stood in for repository membership. Any repository living under a governed root inherited its governance, including an independent clone sharing nothing with it but its path -- and the refusal then asserted a shared `.git` that such a clone does not have. A refusal which misdescribes what it blocked is what teaches people to route around a gate; the rule-3 comment in the same file records that having happened. Decision: compare against the root's OWN common dir, **equality-or-under**. Every worktree of the primary, sibling or nested under `.claude/worktrees/`, answers the SAME common dir and keeps denying; a vendored clone answers a different one and is released. **Equality-or-UNDER rather than equality alone is the load-bearing half:** a submodule's git dir is `/.git/modules/`, so the obvious identity-only predicate would have flipped submodules from DENY to ALLOW as a silent side effect of fixing the vendored case -- a control weakened by accident, under cover of a fix. Where a root is not a repository's top level there is no identity to compare and the old path test stays unchanged, so a directory that merely contains checkouts cannot start failing open. Rejected: identity alone; repairing only the deny WORDING (that is #1082's subject and the verdict is wrong, not just the sentence); and answering the submodule question here | **Accepted (2026-08-27)** -- built with the change. Four rows, proven red-first: the two vendored rows FAILED on the shipped gate and the two deny controls passed on both sides. Four mutants, each refused scoring unless it changed the file's SHA-256, all killed -- and the two directions red **disjoint** sets (identity-only reds the submodule row alone; reverting to the path prefix reds the two vendored rows alone, overlap 0), so neither arm is standing in for the other. Gate suite 679 passed / 2 skipped at base and after. Severity conditional per CLAUDE.md section 0 -- a FALSE DENY in developer tooling with no product surface, and not reachable until someone vendors a clone under a governed root | | [0178](0178-cp1252-console-safety-for-the-powershell-half-of-scripts.md) | **cp1252 console safety for the PowerShell half of `scripts/`** (BACKLOG #1030) -- the class-wide gate covered `scripts/**/*.py` and the engine, and its own docstring stated why PowerShell was out: the `.ps1` surface "has no equivalent reconfigure". **The ungated surface was the LARGER one** -- measured 54 `.ps1` against 47 `.py`. Control run before any code: the SAME character (U+2192) planted in `scripts/asvs/apply.py` and `scripts/coord/claim.ps1`, one gate, one run -- the offenders list named the `.py` and did NOT contain the `.ps1`; with only the `.ps1` poisoned the suite was fully green. **The PowerShell failure mode is worse and it is measured, not assumed:** through both real hosts with the console pinned to cp1252 every arm returned **rc=0 and none raised** -- the character came back as `?` or as three wrong characters while the script reported success, where Python raises a catchable `UnicodeEncodeError`. **THE EXEMPTION IS RE-DERIVED FROM MEASUREMENT, which is this item hard part.** PowerShell has TWO INDEPENDENT CHANNELS where Python has one: source DECODING (WinPS 5.1 reads a BOM-less file as ANSI; pwsh 7 defaults to UTF-8) and output ENCODING (`[Console]::OutputEncoding`) -- **either alone leaves the character destroyed**, the silently-failing decode direction the row calls unowned. **A prior unlanded attempt (`673474806`, 200 commits behind) is REFUTED by row 2 of that table:** it exempted any file assigning `[Console]::OutputEncoding`, and on WinPS 5.1 a BOM-less hardened file is STILL BROKEN. The predicate is kept but as a HOST-CONDITIONAL claim -- the repo standardises on pwsh 7 (19 `pwsh` references), so the pwsh-7 row governs ALMOST everywhere and there the assignment alone IS sufficient. **The first draft asserted ZERO `powershell.exe` and was WRONG -- a scope error that read as a clean measurement (SDS-3.8):** the grep never looked at the engine, and `messagefoundry/service.py:270` launches `powershell.exe` (Windows PowerShell 5.1, NOT pwsh 7) on `scripts/service/install-service.ps1`, a file inside the gated surface -- where row 2 says the exemption is NOT sufficient. Leaving that as prose would be a compensating control resting on a false premise (SDS-3.7), so it is closed in the PREDICATE: a script with a WinPS 5.1 entry point must ALSO carry a UTF-8 BOM. Changes nothing today (that file carries zero non-cp1252 characters, so it never reaches the exemption) and the caller list is re-derived from `service.py` every run rather than trusted. A blanket BOM rule stays rejected: 0 of 54 files carry one, so it would be a 54-file rewrite riding a zero-diff ratchet. **An instrument lie is recorded because it reads as a clean result:** a naive byte test reports TRUE on the mojibake row, where the ANSI-misread string re-encodes to bytes IDENTICAL to UTF-8 U+2192 -- the bytes round-trip by accident while the string is corrupt. **A side effect the Python remedy does not have:** the assignment mutates the SHARED console -- a child took the code page 1252 to 65001 and it STAYED 65001 after that child exited, while the parent cached view still read 1252 | **Accepted (2026-08-28)** -- built with the change; 9 new tests, 23 to 32, proven red-first. **A RATCHET AT ZERO, NOT A REPAIR:** 0 of 54 `.ps1` carry a non-cp1252 character, so no script changed and no shipped entry point is broken. The zero is a MEASUREMENT -- same detector, same run, **29 distinct codepoints** in `docs/BACKLOG.md` as positive control | | [0179](0179-front-loaded-time-amortised-subtree-re-resolution-in-the-connscale-fd-probe.md) | **Front-loaded, time-amortised subtree re-resolution in the connscale FD probe** (BACKLOG #1357) -- `FdSampler` re-walked the engine's process subtree every `_RESOLVE_EVERY_TICKS = 8` sample ticks, and its own comment reasoned about that number in SECONDS (*"at the runner's poll cadence this re-checks the topology every few seconds"*). **A tick is not the poll interval** -- it is the interval PLUS the probe's own shell-out, measured at 0.21-1.19 s against a 0.25 s interval, so a tick ran 3-5x the unit the constant was reasoned in. The result is arithmetic, not statistical: a real `run_connscale` sweep at the CI cell's cadence (hold 1.5 / poll 0.25) reported `fd_probe_ticks = 2` on **all four** steps against the 8 the gate needed, so **0 of 4 could re-resolve**; a hold ladder crossed over only at ~6 s, and both shipped smoke profiles sit at 3.0. **Where the one walk lands is what makes an arbitrary number look plausible:** the runner builds a fresh sampler per sweep step and `messagefoundry/pipeline/sandbox.py` spawns the sandbox worker child lazily on first dispatch, so the single walk fires at tick 1, mid-ramp, while workers are still appearing -- and `handles_peak` is a plain `max()` with no PID-set predicate, so it is not degraded to a gap, it is a plausible number for the wrong process set. Decision: **two triggers, and drop the tick unit rather than layer over it** -- the first `_FRONTLOAD_WALKS = 4` ticks of a sampler's life each re-walk, then amortise on `_RESOLVE_INTERVAL_S = 15.0` SECONDS. The front-load is bounded by walk COUNT so it starts at the first SAMPLE rather than at construction, and however long the connection ramp takes it still covers the beginning of the measurement window; both counters advance for an ATTEMPT so a failing enumeration cannot re-walk flat out. Both call sites construct the sampler bare, so the default change reaches them with no plumbing. **Changes what `handles_peak` MEANS** -- a reading taken before is not comparable with one taken after. Rejected: lowering the tick count (one tick costs longer than the window, so the unit is wrong at any value); `resolve_every=1` (correct but 1056-1278 ms per walk against 210-370 ms cached would let the probe rather than the profile set the cadence); time-based alone (fixes the unit, never fires inside a 1.5 s hold); and staleness from `cpu_pids` (cheap, and detects DEPARTURES only -- a newly spawned worker can never appear in a read keyed to a stale PID list, and arrival is this defect's case) | **Accepted (2026-08-28)** -- built with the change, proven red-first: the acceptance test drives a PRODUCTION-constructed sampler over the 2-tick budget a step affords against a tree that grows after construction, and reds naming 6 of 7 live descendants missed. Three mutants killed on disjoint tests, each reverted byte-identical by SHA-256. Post-fix on the same rig the CI-cell arm re-resolves inside the window (PIDs 6 to 14, handles 438 to 1016) while the amortisation control made 7 walks over 30 ticks, not 30. **A walking tick costs more, so fewer fit:** end to end the CI cell's `fd_probe_ticks` went 2/2/2/2 to 1/1/1/2, every step still measured, both groups still carry the two readings `fd_count_monotonic` needs -- and where a window affords ONE tick no trigger can see growth, which is a property of `hold_seconds` and is NOT claimed fixed here. **No product axis** -- an instrument, not shipped engine behaviour (CLAUDE.md section 0); the cost is that the resource posture of a multi-process engine cannot be measured, which is what holds #1278. The ~3745-vs-~385 handle figure and every per-worker number derived from it stay WITHDRAWN: the caught PID count varied 2, 3, 8, 50 across consecutive ticks of one run. **Amended 2026-09-09 (BACKLOG #1357) -- this row and the ADR's Decision both read `_RESOLVE_INTERVAL_S = 5.0`, and the tree ships 15.0.** 5.0 was really shipped: commit `227568a94` (2026-08-28) added the ADR and set it, then `51ead1399` raised it to 15.0 the same day under this same item, because 5.0 EQUALLED `_PROBE_TIMEOUT_S` -- `FdSampler._resolve_pids` stamps `_last_walk_at` BEFORE the walk, so a walk that spent its whole budget was due again the instant it returned and the probe re-walked every tick, making the amortisation worth zero on the one path it bounds. That second commit touched `harness/load/connscale/probe.py` and `tests/test_connscale_probe_degradation.py` only, so the record went stale; PR 669 squashed both into `f10867ce4`, landing a Decision reading 5.0 beside a `probe.py` reading 15.0. `_FRONTLOAD_WALKS = 4` is as decided and the mechanism is unchanged -- a wrong number in a record, not a defect in the instrument. The margin is now pinned by `test_the_rewalk_interval_outlives_a_walk_that_spends_its_whole_budget`, so restoring 5.0 turns it red | -| [0180](0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) | **Asserting TLS suites on a library that exposes no SSLContext** (BACKLOG #1317) -- the remainder of #1317, whose row states it exactly: ldap3, hvac and ODBC Driver 18 "choose their own suites and no engine object exists to assert on". Measured with one instrument on two real call sites: the HTTP-family opener reached `harden_cipher_suites` **1** time while `LdapAuthenticator._server()` reached it **0**, and an `ldap3.Tls` was found to carry **zero** `SSLContext` attributes -- it builds the context inside `wrap_socket` at connect time and exposes no `ssl_context=` to inject one through. The suite list that hop resolves to is CLEAN today (17 suites, zero NULL/anonymous/non-forward-secret), so the defect is **inheritance without assertion**, not a negotiable weak suite. Decides the general question: **where a library exposes no context, assert a REBUILT one and pin the rebuild by capturing the library's own** -- a test drives ldap3's real `wrap_socket` over a socketpair and compares -- and **REFUSE any argument the rebuild cannot reproduce**, because `ldap3.Tls(ciphers=...)` is a measured trap: `except ssl.SSLError: pass` swallows a rejected string and silently strips the TLS 1.2 list from 14 suites to 0. Scopes the other two OUT with reasons rather than deferring them: **hvac is unmeasurable here** (hvac/requests/urllib3 all absent, and NO CI leg installs the `[vault]` extra), so a control there would be one no test in the project can execute -- *that hvac scope-out EXPIRED on 2026-09-03: the `test` leg installs `[vault]`, the arm is built, and Amendment A carries the measurement*; **ODBC Driver 18 is out permanently** (`store/sqlserver.py` contains no `ssl` usage at all -- TLS is connection-string keywords terminated in the native driver, so no replica is even possible). | **Accepted (2026-08-28; amended 2026-09-03, extended 2026-09-04 -- Amendment A builds the hvac arm the row above scoped out, and converts that scope-out's own trigger test into a guard; amended 2026-09-27 -- Amendment C (BACKLOG #300) narrows both hops to the approved suites: ldap3 now takes one admitted `ciphers=` value, and the Vault hops handshake on engine-built per-connection contexts rather than a replica; amended 2026-09-28 -- Amendment D (BACKLOG #300) narrows and checks the TLS leg to an https proxy too, and refuses an http Vault behind an https proxy)** -- built with the change; the AD LDAPS bind is the seventh asserted site and the first asserted via a rebuilt context. Red-first: `test_ad_ldaps_bind_asserts` failed DID NOT RAISE before the wiring. Five mutations each red a distinct test and all files restored byte-identical; deleting the call reds two tests while drifting `_server()` off `_tls_kwargs()` reds only the argument test, so the two are not one case wearing two names. 137 passed on the #1317 baseline (129 before, +8 new), 572 on the auth/TLS/settings slice, mypy strict clean with no new errors. Severity conditional per CLAUDE.md section 0 -- nothing is intercepted today; a deploying site WOULD cross an unasserted context on its AD bind | +| [0180](0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) | **Asserting TLS suites on a library that exposes no SSLContext** (BACKLOG #1317) -- the remainder of #1317, whose row states it exactly: ldap3, hvac and ODBC Driver 18 "choose their own suites and no engine object exists to assert on". Measured with one instrument on two real call sites: the HTTP-family opener reached `harden_cipher_suites` **1** time while `LdapAuthenticator._server()` reached it **0**, and an `ldap3.Tls` was found to carry **zero** `SSLContext` attributes -- it builds the context inside `wrap_socket` at connect time and exposes no `ssl_context=` to inject one through. The suite list that hop resolves to is CLEAN today (17 suites, zero NULL/anonymous/non-forward-secret), so the defect is **inheritance without assertion**, not a negotiable weak suite. Decides the general question: **where a library exposes no context, assert a REBUILT one and pin the rebuild by capturing the library's own** -- a test drives ldap3's real `wrap_socket` over a socketpair and compares -- and **REFUSE any argument the rebuild cannot reproduce**, because `ldap3.Tls(ciphers=...)` is a measured trap: `except ssl.SSLError: pass` swallows a rejected string and silently strips the TLS 1.2 list from 14 suites to 0. Scopes the other two OUT with reasons rather than deferring them: **hvac is unmeasurable here** (hvac/requests/urllib3 all absent, and NO CI leg installs the `[vault]` extra), so a control there would be one no test in the project can execute -- *that hvac scope-out EXPIRED on 2026-09-03: the `test` leg installs `[vault]`, the arm is built, and Amendment A carries the measurement*; **ODBC Driver 18 is out permanently** (`store/sqlserver.py` contains no `ssl` usage at all -- TLS is connection-string keywords terminated in the native driver, so no replica is even possible). | **Accepted (2026-08-28; amended 2026-09-03, extended 2026-09-04 -- Amendment A builds the hvac arm the row above scoped out, and converts that scope-out's own trigger test into a guard; amended 2026-09-26 -- Amendment B (BACKLOG #2034) hands the AD bind the checked trust anchor as `ca_certs_data` and refuses `ca_certs_file`; amended 2026-09-27 -- Amendment C (BACKLOG #300) narrows both hops to the approved suites, and the Vault hops handshake on engine-built per-connection contexts rather than a replica; amended 2026-09-28 -- Amendment D (BACKLOG #300) narrows and checks the TLS leg to an https proxy too, and refuses an http Vault behind an https proxy; amended 2026-09-30 -- Amendment E (BACKLOG #2494) retires the LDAPS replica for an engine-built context, and Amendment F (BACKLOG #2530) stops the AD hop following any LDAP referral)** -- built with the change; the AD LDAPS bind is the seventh asserted site and the first asserted via a rebuilt context. Red-first: `test_ad_ldaps_bind_asserts` failed DID NOT RAISE before the wiring. Five mutations each red a distinct test and all files restored byte-identical; deleting the call reds two tests while drifting `_server()` off `_tls_kwargs()` reds only the argument test, so the two are not one case wearing two names. 137 passed on the #1317 baseline (129 before, +8 new), 572 on the auth/TLS/settings slice, mypy strict clean with no new errors. Severity conditional per CLAUDE.md section 0 -- nothing is intercepted today; a deploying site WOULD cross an unasserted context on its AD bind | | [0182](0182-split-the-account-mirror-address-from-the-engine-owned-notification-address.md) | **Split the account mirror address from the engine-owned notification address** (BACKLOG #1139, ASVS 6.3.7) -- one nullable column, `users.email`, was simultaneously the directory's MIRROR (rewritten from the `mail` attribute by `_upsert_ad_user` on every AD/OIDC login) and the TARGET every out-of-band security notice is addressed to. Two things follow from that single fact. **A directory repoint would replace the very address the notice about it has to reach**, which is why #1139's research question -- *"to which address, given the old value is the only one the engine can still reach and the same operation is replacing it?"* -- reads as unanswerable: the premise is wrong, not the answer hard. **And a clear was permanent exclusion**, because `SecurityEventNotifier.notify` opens `if not event.email: return`. Decision: `users.notify_email`, engine-owned, named by NO directory-sync statement -- `update_user_profile`, the one call the AD upsert makes against an existing account, still writes `display_name` and `email` and nothing else. Seeded ONCE at account birth (there is exactly one address at that instant and no reason for the two to differ); thereafter written only by `set_user_notify_email`. **THE DURABILITY RULE IS THAT SETTER'S SIGNATURE, and it is the limb the item says the build owes itself:** `email: str`, not `str | None`, so a clear is unrepresentable at every call site, plus a refusal of the whitespace-only string that would mean the same thing -- repointable but not erasable. Making an address mandatory at creation would NOT have achieved this, since an explicit null still strips it afterwards. **NOT NULL was the other offered route and is rejected**: accounts may still be created with no address (the first-login set-address step is a follow-on), so it would need a placeholder the notifier treats as absent anyway -- a constraint that reads as a guarantee and delivers none. Also rejected: a provenance flag on one column (a second column doing the split's job with less clarity); notifying the directory's NEW address (whoever repointed the attribute is the party it would reach); and any dual-read, shim or deprecation window, per CLAUDE.md section 0. `has_notifiable_admin` and the PHI startup gate move to `notify_email` too -- asking about `email` would be the instrument answering the adjacent question (SDS-3.8). Two pieces of prose the change FALSIFIES are corrected in the commit that falsifies them, the SDS-3.7 shape: the notice body's promise that a cleared address receives nothing further, and the startup gate's docstring claim that a human-set address is overwritten at the next directory sign-in | **Accepted (2026-09-03; amended 2026-09-25, Amendment A: `update_user` moves `notify_email` only on an explicit `notify_email` value, never from the profile `email`, BACKLOG #1139 slice 3)** -- built with the change under an owner ruling of that date; three store backends plus the protocol. Seven new tests proven red-first against `origin/main` source (4 store, 3 service), including a negative control that an admin can still REPOINT, so a no-op setter cannot pass. Postgres and SQL Server run on hosted runners only, so **CI is the authority for those two backends**; the SQLite leg is exercised locally including a real pre-split table with the column dropped. Stacks on PR 759, which landed #1139's write-path limb and is unmerged. Severity conditional per CLAUDE.md section 0 -- **zero deployments**, so no account's notices are misdirected today; a deploying site with AD enabled WOULD inherit both defects | | [0183](0183-provision-the-first-administrator-offline-no-default-account-at-first-run.md) | **Provision the first administrator offline; no default account at first run** (BACKLOG #1136, ASVS 6.3.2) -- the verb asks that default user accounts *"are not present in the application or are disabled"*, and neither arm holds: `_ensure_bootstrap_admin` creates an enabled local account literally named `admin` on an empty table, and the disabled arm is **unexpressible at two altitudes** -- `Store.create_user` carries no `disabled` parameter and all three backends hardcode the column rather than binding it. Decision: **the "not present" arm**, reached by `messagefoundry provision-admin --username ` run at the host before the first `serve`; the seeding path then declines on a non-empty table and no default account is ever created. **The way in is filesystem authority over the store, not an account** -- the same host gate ADR 0171 argues for `admin-unlock`, so it grants nothing a network attacker can reach. **The refusal asks for an ENABLED ADMINISTRATOR, not an empty table**, correcting the researched guard: `_upsert_ad_user` assigns no role, so one completed directory sign-in leaves a roleless row and a non-empty table, and an emptiness guard would refuse in exactly the state where the install has no way in. **No `--password` and no `--password-file`** -- unattended provisioning is refused in terms rather than left as a hatch that lands a standing Administrator credential in argv or on disk. **The credential is claimed at birth** (`users.password_claimed_at`), which removes the half-claimed state a restart could land in and keeps an operator-chosen `admin` out of the WP-3 retirement sweep. The row is created with NO password hash, so every interruption point leaves at most a roleless account that is denied everything by default, and a re-run completes it. Rejected: create-disabled plus a claim step (needs a `disabled` parameter across the protocol and three backends, and a claim ceremony is the thing that can strand a headless restart); create-disabled-then-auto-enable-on-first-login (makes the credential file's permissions the real control while the column records a state nothing enforces -- SDS-3.7); requiring `--email` unconditionally (an operator with no relay types a fake address, and the PHI start gate is already the single authority) | **Accepted (2026-09-05; amended 2026-09-23 -- Amendment A, by owner ruling, brings retiring the auto-create INTO scope as a planned build in waves 0, 1a, 1b, 1c and 2 to 5, and reverses the "shipped default is unchanged" sentence. It recommends no new refusal: the ADR 0167 gate is the only gate that asks the account question, it already refuses a store with no addressed Administrator at the shipped posture, and its message becomes true once no bootstrap account exists. That recommendation holds only if Wave 0 shows the Windows service store ACL that ADR 0163 raised works in both orders. It adds an offline notification-address setter, because an Administrator with no address strands the next start today. Measured blast radius: 255 initialize() call sites in 85 files, and 115 test failures in exactly the 16 files that bind the returned account. Nothing in the amendment is built. The 197-site figure and the out-of-scope sentence later in this cell are the 2026-09-05 record, superseded by the amendment)** -- built with the change; 14 tests. **One planted control**: setting the credential with `must_change_password=True` reds exactly the two tests that cover the claim stamp, with `auth.bootstrap_admin_retired` in the captured log against an account an operator had just provisioned. **ASVS 6.3.2 STAYS PARTIAL** -- retiring `_ensure_bootstrap_admin` and the WP-3 lifecycle is out of scope and is what decides the cell (measured at `initialize()`'s 197 call sites across 64 files, plus `bootstrap_expiry_hours`/`bootstrap_warn_hours`, `_emit_bootstrap_admin`, the `bootstrap_admin_expiring` alert, six documents and four IDE files). Severity conditional per CLAUDE.md section 0 -- **zero deployments** | | [0184](0184-identify-a-federated-login-by-the-idp-namespaced-subject-not-by-the-username-it-claims.md) | **Identify a federated login by the IdP-namespaced subject, not by the username it claims** (BACKLOG #1143, ASVS 6.8.1) -- 6.8.1's own mitigation is to identify a user by the IdP's id plus the user's id inside it. **The engine now STORES that pair and still does not IDENTIFY by it.** `oidc_issuer`/`oidc_subject`, the filtered `ux_users_federated_subject` on all three backends, and the keyed lookup all shipped under #1256; measured 2026-09-05 at `a083cdb89` the unique-index probe that returned 0/0/0 in August returns **1/1/1**, control 10/6/8. What remains is ordering: `authenticate_oidc` resolves the directory principal from the token's username claim, the #1015 continuity guard fetches by that username, `_complete_ad_login` fetches by it AGAIN and that is the read the session is issued for, and the pair is consulted only afterwards as an exclusivity veto over an account already chosen. **Decision: select by `(issuer, subject)` at the head of `_complete_ad_login`, then RE-RESOLVE the directory principal from the bound row's own stored username** -- which removes identifier equality from the identification path, literally the verb's parenthetical. `federated_subject` is already a parameter there and already defaults to `None`, so the simple-bind and Kerberos callers take no new branch; no DDL, no migration. **The half-done version is a privilege-transfer bug and is the likely way to get this wrong:** resolve by the pair to row R but carry on with the `AdPrincipal` resolved from the CLAIMED username U, and `_upsert_ad_user` touches a different row while `roles_for_ad_groups` writes U's directory groups onto R. Rejected: a third `AuthProvider` member (measured -- it either breaks hybrid login at `_complete_ad_login`'s provider guard or is written nowhere; the discriminator belongs on the SESSION); grounding the cell on the continuity pair (that is 10.5.2's verb, and `auth/oidc/claims.py` pins the issuer before any guard runs, so the pair cannot do cross-provider work); rescoring `na` because `oidc_enabled` ships `False` (a disabled feature removes the trigger, not the control); and binding on a directory-held immutable attribute as an AUTHENTICATOR (directory-readable, not secret). **THE FLOOR UNDER EVERY CEREMONY IS ENTAILED, WHICH ADR 0142 A.4 STATED AS AN EITHER/OR:** `set_user_federated_subject` has exactly ONE engine caller, the bind-on-first-presentation site (control: five callers for `set_user_roles`), `api/auth_routes.py` has no federated route and the web console has no federated surface -- **the only way to create a binding today is the one the verb forbids**, and 0142's "refuse unbound accounts" says *until an operator binds them*, which is the surface its other option describes. Also priced: that setter takes `issuer: str, subject: str`, so **an unbind is unrepresentable** and any surface needing a clear is a protocol change on three backends *(stale by 2026-09-23: #1474 had shipped a separate clear method; see Status)* | **Accepted (2026-09-23); slice A built 2026-09-25 (BACKLOG #1143 / #295)** -- a federated login now selects its account by the pair and refuses an unbound one, and `PUT`/`DELETE /users/{user_id}/federated-identity` bind, rebind and unbind; the console leg, the session mechanism field and AC-5 are not in slice A. **Slice B built 2026-09-25:** the console screen `/ui/users/{user_id}/federated-identity` views, links, relinks and unlinks, behind the same action-bound step-up. **Slice C built 2026-09-25:** the admin bind refuses an account with no `directory_object_id` (`directory_object_id_missing`, audited), so AC-5 holds by construction for every binding made since; a site whose directory returns no readable `objectGUID` can bind nobody. Owner ruling given to a Manager seat; the build may start. All four open items are resolved in the ADR: an unbound federated login is **refused** with an operator-readable message; `reconcile_directory_sessions` is **re-keyed on `objectGUID`**, not narrowed by excluding bound rows (measured on acceptance: that re-key had already shipped under #1471/#1532 for rows carrying a directory object id; a NULL-id row still probes by name); **both unbind and rebind** are built (unbind's store and service halves had already shipped under #1474, with no caller); and a session mechanism field is added **together with** the re-auth leg that consumes it ([ADR 0142](0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md) Amendment B). Derived from the owner's reason rather than ruled: no unbind caller should land before the refusal does. Federation ships off, so this hardens a first deployment that turns it on. *Superseded cell text, kept as a record: "**Proposed (2026-09-05)** -- **DO NOT START THE BUILD.** The first-federated-login ceremony is an owner trust decision and is the one thing blocking it; four other questions sit in *To resolve on acceptance*, including whether the username-keyed `reconcile_directory_sessions` loop is fixed by excluding bound rows or by re-keying its probe to `objectGUID`. Research only: no code, no schema, no vault file touched."* **Scope boundary, and it is load-bearing:** this ADR covers the OIDC leg alone and does NOT close the AD `sAMAccountName`-recycle limb. That limb is BACKLOG #1471's, and `api/app.py` `_may_access_upload`, `uploads.py` `UploadedFileMeta`, `tests/test_upload_api.py` and ADR 0136 each cite #1471 for it. *Corrected 2026-09-28 (BACKLOG #2030): this sentence said the four "all name BACKLOG #1143 as the fix". They were re-pointed to #1471 before then, and each has 0 matches for `1143` and 1 for `1471`; the ADR body's own STALE note, measured 2026-09-25, records the same.* | diff --git a/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index c7139972c..4e99b0788 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -14,16 +14,22 @@ ``[auth].ad_receive_timeout`` (each LDAP response read), both defaulting to 10 s (ASVS 13.1.3). ldap3's own defaults are ``None`` on both, i.e. wait forever. ``ldap3``/``spnego`` are imported lazily so a local-only deployment never touches them. + +**No referral is followed, and one is refused** (BACKLOG #2530); :func:`_refuse_referral` says why. """ from __future__ import annotations import logging +import math +import re import ssl import uuid +from collections.abc import Iterable from dataclasses import dataclass from enum import Enum from typing import TYPE_CHECKING, Any, NamedTuple +from urllib.parse import urlsplit from messagefoundry.auth.trust_anchors import ad_anchor_spec, verified_anchor_cadata from messagefoundry.config.secretprovider import SecretProvider, resolve_connector_secret @@ -128,6 +134,24 @@ class _Lookup(NamedTuple): info: dict[str, Any] | None = None +def _ldap3_receive_timeout(seconds: float) -> int: + """``[auth].ad_receive_timeout`` as the whole seconds ldap3 can actually apply on every OS. + + ldap3 sets ``SO_RCVTIMEO`` with ``struct.pack('LL', receive_timeout, 0)`` on every non-Windows + host, and ``struct.pack`` refuses a float. The setting is a float (default ``10.0``), so passing + it straight through made EVERY ldap3 socket open raise ``struct.error`` on Linux after the TCP + connect and before the bind was sent: AD sign-in could not work there at all. Windows hides it, + because ldap3 converts with ``int(1000 * t)`` on that branch. + + Rounds UP, never to nearest, so the result is never shorter than the operator configured. The + cost is a timeout up to one second longer on every OS: ldap3 first calls + ``socket.settimeout(receive_timeout)``, which would have kept sub-second precision. Rounding up + also never yields 0. The settings validator already refuses values <= 0, and 0 would make + ``settimeout`` turn the socket non-blocking, so every read would fail at once. + """ + return math.ceil(seconds) + + def _escape_filter(value: str) -> str: """RFC 4515 escaping for values interpolated into an LDAP search filter.""" out: list[str] = [] @@ -377,6 +401,85 @@ def _cn_of(dn: str) -> str | None: return head[3:] if head[:3].upper() == "CN=" else None +#: A referred host the refusal may name. Anything else is replaced, because the text comes from the +#: directory and is written to the log and the audit row. +_PRINTABLE_HOST = re.compile(r"[A-Za-z0-9._:\[\]-]{1,253}") + +#: How many referred hosts the refusal names before it only counts the rest. +_HOSTS_NAMED = 3 + + +def _referred_hosts(referrals: Iterable[object]) -> str: + """The hosts ``referrals`` point at, for the refusal. Never the whole URL. + + An LDAP URL can carry a DN, a filter and extensions (RFC 4516), and a bindname extension would + name an account. Only the host part says where the directory tried to send the engine. + """ + hosts: set[str] = set() + unreadable = False + for uri in referrals: + try: + host = urlsplit(str(uri)).hostname + except ValueError: + host = None + if host and _PRINTABLE_HOST.fullmatch(host): + hosts.add(host) + else: + unreadable = True + # Readable hosts take the named slots; an unreadable one is only ever listed last. + shown = sorted(hosts) + ([""] if unreadable else []) + named = shown[:_HOSTS_NAMED] + more = len(shown) - len(named) + return ", ".join(named) + (f" and {more} more" if more else "") or "" + + +def _refuse_referral(conn: Any, operation: str) -> None: + """Raise :class:`LdapError` when the directory answered ``conn``'s last operation with a referral. + + BACKLOG #2530. With ``auto_referrals`` on, which is ldap3's default, ldap3 2.9.1 opens a new + connection to the referred host and, on a bound connection, binds there with this connection's + user and password (``strategy/base.py``, ``create_referral_connection``). It builds a plain + ``ldap3.Tls`` for that hop: no pinned CA bytes, no narrowed suites, and none at all for an + ``ldap://`` referral. So one referral would carry the service-account password off the anchored + hop. Every ``Connection`` here is built with ``auto_referrals=False``, so ldap3 hands the + referral back instead, and this turns it into a refusal. + + **A refusal, not a "no match" or a wrong password.** Left alone, a referred search reads as no + entries, which the session reconciler would count toward revoking the account. A referred bind + reads as a rejected password, which the step-up re-bind would count toward the engine lockout. + An :class:`LdapError` is audited as ``auth.login_error`` at sign-in, and read as unavailable by + the reconciler, which never revokes. A referral usually means a search base in another domain of + the forest. + + **Only a referral RESULT (resultCode 10).** A search continuation reference (``searchResRef``) + arrives with resultCode 0, beside the entries. ldap3 never follows one, with or without this + item, so it leaks nothing, and it still reads as no entry from that subtree. AD adds such + references to every search based at a domain root, so refusing them would refuse every search. + """ + from ldap3.core.results import RESULT_REFERRAL # lazy, like every ldap3 import here + + result = conn.result + if not isinstance(result, dict) or result.get("result") != RESULT_REFERRAL: + return + hosts = _referred_hosts(result.get("referrals") or ()) + message = ( + f"AD answered the {operation} with a referral to {hosts}; the engine does not follow " + "referrals, because ldap3 would re-send the bind credentials there without the pinned CA " + "(BACKLOG #2530). This usually means a configured search base lies in another domain of " + "the forest; use a base in this domain controller's own domain, or a global catalog, " + "which carries universal-group membership only (ADR 0180 Amendment F)." + ) + _warn_once(f"referral: {operation}", "%s Reported once per operation.", message) + raise LdapError(message) + + +def _search(conn: Any, operation: str, **kwargs: Any) -> None: + """``conn.search(**kwargs)``, refusing a referral result. Every search in this module goes + through here, so none can read one as "no entries"; a test pins that.""" + conn.search(**kwargs) + _refuse_referral(conn, operation) + + class LdapAuthenticator: """Binds against Active Directory over LDAPS and resolves a user's (nested) group membership.""" @@ -477,6 +580,9 @@ def _server(self) -> Any: tls=self._tls, get_info=ldap3.NONE, connect_timeout=self._s.ad_connect_timeout, + # BACKLOG #2530, see _refuse_referral. ldap3's default, None, admits every host; empty + # admits none, so this holds even for a Connection that omits auto_referrals=False. + allowed_referral_hosts=[], ) def _service_conn(self) -> Any: @@ -491,7 +597,8 @@ def _service_conn(self) -> Any: password=self._bind_password, # resolved once in __init__ (env or [secrets].provider) authentication=ldap3.SIMPLE, auto_bind=True, - receive_timeout=self._s.ad_receive_timeout, + receive_timeout=_ldap3_receive_timeout(self._s.ad_receive_timeout), + auto_referrals=False, # BACKLOG #2530: see _refuse_referral ) def _equalizing_bind(self, password: str) -> None: @@ -523,7 +630,8 @@ def _equalizing_bind(self, password: str) -> None: user=f"CN=mf-nonexistent-timing-equalizer,{self._s.ad_user_search_base}", password=password, authentication=ldap3.SIMPLE, - receive_timeout=self._s.ad_receive_timeout, + receive_timeout=_ldap3_receive_timeout(self._s.ad_receive_timeout), + auto_referrals=False, # BACKLOG #2530: see _refuse_referral ) try: conn.bind() # result deliberately ignored — this branch always fails the login @@ -559,7 +667,9 @@ def _search_user( """ import ldap3 - conn.search( + _search( + conn, + "user search", search_base=self._s.ad_user_search_base, search_filter=search_filter, search_scope=ldap3.SUBTREE, @@ -670,7 +780,9 @@ def _resolve_groups(self, conn: Any, user_dn: str, member_of: list[str]) -> froz if cn: groups.add(cn.lower()) if self._s.ad_use_nested_groups and self._s.ad_group_search_base: - conn.search( + _search( + conn, + "group search", search_base=self._s.ad_group_search_base, search_filter=f"(member:{_MATCHING_RULE_IN_CHAIN}:={_escape_filter(user_dn)})", search_scope=ldap3.SUBTREE, @@ -727,13 +839,15 @@ def authenticate( user=user_dn, password=password, authentication=ldap3.SIMPLE, - receive_timeout=self._s.ad_receive_timeout, + receive_timeout=_ldap3_receive_timeout(self._s.ad_receive_timeout), + auto_referrals=False, # BACKLOG #2530: see _refuse_referral ) # Released on BOTH paths. A rejected password is the common adversarial case, so # returning early without unbinding would leave the connection to GC under exactly # the load that matters (ASVS 13.1.3 — resource release). try: if not user_conn.bind(): + _refuse_referral(user_conn, "user bind") return None finally: user_conn.unbind() diff --git a/messagefoundry/auth/ldap_tls.py b/messagefoundry/auth/ldap_tls.py index 2bf1a1e13..73fa76129 100644 --- a/messagefoundry/auth/ldap_tls.py +++ b/messagefoundry/auth/ldap_tls.py @@ -42,9 +42,11 @@ class NarrowedTls(ldap3.Tls): # type: ignore[misc] # ldap3 ships no type infor ldap3 argument, because the engine's context would not carry one. ``ciphers=`` in particular reached TLS 1.2 only. - **Not reached: a followed referral.** ldap3 builds a plain ``ldap3.Tls`` for the referred server - from a few of these attributes (``strategy/base.py``, ``create_referral_connection``). That copy - carries neither the checked CA bytes nor any of the narrowing. This class does not change it. + **Not reached: a followed referral, which is why the engine follows none.** ldap3 builds a plain + ``ldap3.Tls`` for the referred server from a few of these attributes (``strategy/base.py``, + ``create_referral_connection``). That copy carries neither the checked CA bytes nor any of the + narrowing, and this class does not change it. :mod:`messagefoundry.auth.ldap` turns referral + following off and refuses a referral instead (BACKLOG #2530). """ def __init__( diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 150540ff6..a9a10b402 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -2463,6 +2463,10 @@ def split_kerberos_spn(spn: str) -> tuple[str, str]: #: and ``tests/test_site_context_words.py`` holds the two equal. EXTRA_CONTEXT_WORD_MIN_LENGTH = 3 +#: Upper bound for ``[auth].ad_connect_timeout`` and ``ad_receive_timeout``. An hour is far past any +#: real directory round trip and far below the point where a socket timeout overflows. +_AD_TIMEOUT_MAX_SECONDS = 3600.0 + class AuthSettings(_Section): """Authentication + RBAC knobs. Secrets (the AD bind password) come from env, never the file.""" @@ -3106,13 +3110,24 @@ def _check_totp_skew(cls, value: int) -> int: @field_validator("ad_connect_timeout", "ad_receive_timeout") @classmethod def _check_ad_timeout(cls, value: float) -> float: - # Must stay FINITE and positive (ASVS 13.1.3): ldap3 treats 0/None as "wait forever", which is - # exactly the unbounded wait these settings exist to remove, and inf/NaN are the same hole by - # another spelling. Rejected at config load, not discovered at bind time against a wedged DC. + # Must stay FINITE and positive (ASVS 13.1.3): ldap3 treats a None, or a 0 connect_timeout, as + # "wait forever", which is exactly the unbounded wait these settings exist to remove, and + # inf/NaN are the same hole by another spelling. (A 0 receive_timeout instead makes the socket + # non-blocking, so every read fails at once.) Rejected at config load, not at bind time. if not value > 0 or value == float("inf"): raise ValueError( "ad_connect_timeout / ad_receive_timeout must be a finite number of seconds > 0 " - "(0, a negative value, inf or NaN would restore an unbounded LDAP wait)" + "(inf, NaN or a None connect timeout would mean an unbounded LDAP wait; 0 or a " + "negative value would fail every LDAP read or connect)" + ) + # A huge finite value overflows socket.settimeout / setsockopt (measured from about 3e6 s on + # Windows) with OverflowError or TypeError. Those are not ldap3 errors, so they would skip + # the LdapError mapping and the auth.login_error audit. The cap sits far below that point. + if value > _AD_TIMEOUT_MAX_SECONDS: + raise ValueError( + f"ad_connect_timeout / ad_receive_timeout must be at most {_AD_TIMEOUT_MAX_SECONDS:g} " + f"seconds (got {value:g}); the cap keeps the value far below where a socket " + "timeout overflows" ) return value diff --git a/tests/test_ad_directory_identity.py b/tests/test_ad_directory_identity.py index e42260359..b95edcf0b 100644 --- a/tests/test_ad_directory_identity.py +++ b/tests/test_ad_directory_identity.py @@ -223,6 +223,7 @@ class _FakeConn: def __init__(self, entry: _FakeEntry) -> None: self.entries = [entry] self.kwargs: dict[str, Any] = {} + self.result: dict[str, Any] | None = None # ldap3 sets it per operation; no referral def search(self, **kwargs: Any) -> None: self.kwargs = kwargs @@ -314,6 +315,7 @@ class FakeConnection: def __init__(self, server: Any = None, **kwargs: Any) -> None: self.entries: list[_FakeEntry] = [] self.user = str(kwargs.get("user")) + self.result: dict[str, Any] | None = None # no referral def __enter__(self) -> FakeConnection: return self @@ -1065,6 +1067,7 @@ def __init__(self, entry: _FakeEntry | None) -> None: self._entry = entry self.entries: list[_FakeEntry] = [entry] if entry is not None else [] self.searches: list[dict[str, Any]] = [] + self.result: dict[str, Any] | None = None # no referral def __enter__(self) -> _RecordingConn: return self diff --git a/tests/test_ad_user_account_control.py b/tests/test_ad_user_account_control.py index 59dd92a53..6421f976b 100644 --- a/tests/test_ad_user_account_control.py +++ b/tests/test_ad_user_account_control.py @@ -187,6 +187,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: class FakeConnection: def __init__(self, *args: Any, **kwargs: Any) -> None: self.entries: list[_Entry] = [] + self.result: dict[str, Any] | None = None # no referral def __enter__(self) -> FakeConnection: return self diff --git a/tests/test_auth_hardening.py b/tests/test_auth_hardening.py index e668f68d3..6db031455 100644 --- a/tests/test_auth_hardening.py +++ b/tests/test_auth_hardening.py @@ -481,6 +481,7 @@ def test_nested_group_filter_escapes_user_dn() -> None: class _Conn: entries: list[object] = [] + result: dict[str, object] | None = None # no referral def search(self, **kw: object) -> None: captured.update(kw) @@ -524,6 +525,7 @@ def __getitem__(self, k: str) -> _Attr: class _Conn: def __init__(self, entry: _Entry) -> None: self.entries = [entry] + self.result: dict[str, object] | None = None # no referral def search(self, **kw: object) -> None: pass diff --git a/tests/test_ldap_referrals.py b/tests/test_ldap_referrals.py new file mode 100644 index 000000000..a29d44d8b --- /dev/null +++ b/tests/test_ldap_referrals.py @@ -0,0 +1,422 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Foundation, LLC and contributors +"""BACKLOG #2530: the AD hop follows no LDAP referral, and a referral is a refusal. + +ldap3 2.9.1 follows a referral by default (``Connection(auto_referrals=True)``, and +``Server(allowed_referral_hosts=None)``, which it reads as ``[('*', True)]``). On a bound connection +its ``create_referral_connection`` opens a new connection to the referred host and binds there with +the same user and password, over a plain ``ldap3.Tls`` or none at all. So one referral would carry +the service-account password off the anchored hop. + +The end-to-end tests run a REAL ``ldap3`` stack against two loopback servers that speak just enough +of RFC 4511. The first answers the user search with a referral to the second. The second records +every byte it receives, so "not followed" is a count of connections it accepted, and the +positive-control arm shows the same instrument does see the password when both guards are removed. + +The primary hop here is plain ``ldap://`` (``ad_allow_insecure_ldap``), because the property under +test is whether ldap3 follows, and that does not depend on the primary's TLS. + +PHI-free: synthetic directory names and passwords only. +""" + +from __future__ import annotations + +import ast +import contextlib +import socket +import threading +from collections.abc import Iterator +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import ldap3 +import pytest +from ldap3.core.results import RESULT_REFERRAL + +from messagefoundry.auth import ldap as ldap_module +from messagefoundry.auth.ldap import LdapAuthenticator, LdapError +from messagefoundry.auth.service import AuthService +from messagefoundry.config.settings import AuthSettings +from messagefoundry.store.store import MessageStore +from tests.test_ldap_timeouts import ( + REFERRAL_RESULT, + _ad_settings, + _install_fakes, + _ldap3_construction_sites, +) + +_BIND_PASSWORD = "synthetic-bind-pw" # what _ad_settings binds the service account with +_USER_PASSWORD = "synthetic-user-pw-2530" + +# --- a loopback LDAP server: just enough RFC 4511 BER to bind, search and unbind ------------------ + +_BIND_REQUEST, _SEARCH_REQUEST, _UNBIND_REQUEST = 0x60, 0x63, 0x42 +_BIND_RESPONSE, _SEARCH_DONE = 0x61, 0x65 + + +def _tlv(tag: int, content: bytes) -> bytes: + """One BER tag-length-value, definite length.""" + n = len(content) + if n < 0x80: + return bytes([tag, n]) + content + raw = n.to_bytes((n.bit_length() + 7) // 8, "big") + return bytes([tag, 0x80 | len(raw)]) + raw + content + + +def _ldap_result(tag: int, code: int, referrals: tuple[str, ...] = ()) -> bytes: + """An LDAPResult (RFC 4511 section 4.1.9): code, empty matchedDN and message, the referrals.""" + content = _tlv(0x0A, bytes([code])) + _tlv(0x04, b"") + _tlv(0x04, b"") + if referrals: + content += _tlv(0xA3, b"".join(_tlv(0x04, uri.encode()) for uri in referrals)) + return _tlv(tag, content) + + +def _recv_exact(sock: socket.socket, n: int) -> bytes | None: + data = b"" + while len(data) < n: + chunk = sock.recv(n - len(data)) + if not chunk: + return None + data += chunk + return data + + +def _read_message(sock: socket.socket) -> tuple[bytes, bytes] | None: + """One LDAPMessage off the wire: ``(raw bytes, the SEQUENCE's content)``, or ``None`` at EOF.""" + head = _recv_exact(sock, 2) + if head is None: + return None + length, extra = head[1], b"" + if length & 0x80: + extra = _recv_exact(sock, length & 0x7F) or b"" + length = int.from_bytes(extra, "big") + body = _recv_exact(sock, length) + if body is None: + return None + return head + extra + body, body + + +class _FakeDc: + """A loopback LDAP server. Binds succeed; a search is answered with ``search_referral`` when set, + else with success and no entries. ``received`` holds every byte any client sent it.""" + + def __init__(self, *, search_referral: str | None = None) -> None: + self._referral = search_referral + self._listener = socket.create_server(("127.0.0.1", 0)) + self.port: int = self._listener.getsockname()[1] + self.accepted = 0 + self.received = bytearray() + self._lock = threading.Lock() + threading.Thread(target=self._accept, daemon=True).start() + + def _accept(self) -> None: + while True: + try: + sock, _addr = self._listener.accept() + except OSError: + return # the listener was closed + with self._lock: + self.accepted += 1 + threading.Thread(target=self._session, args=(sock,), daemon=True).start() + + def _session(self, sock: socket.socket) -> None: + with sock, contextlib.suppress(OSError): + sock.settimeout(10) + while (message := _read_message(sock)) is not None: + raw, body = message + with self._lock: + self.received += raw + message_id = body[: 2 + body[1]] # the messageID INTEGER, echoed verbatim + op = body[2 + body[1]] + if op == _BIND_REQUEST: + reply = _ldap_result(_BIND_RESPONSE, 0) + elif op == _SEARCH_REQUEST and self._referral is not None: + reply = _ldap_result(_SEARCH_DONE, RESULT_REFERRAL, (self._referral,)) + elif op == _SEARCH_REQUEST: + reply = _ldap_result(_SEARCH_DONE, 0) + else: # unbind, or anything this server does not speak + return + sock.sendall(_tlv(0x30, message_id + reply)) + + def close(self) -> None: + # shutdown wakes a blocked accept() on Linux, where close() alone does not. + with contextlib.suppress(OSError): + self._listener.shutdown(socket.SHUT_RDWR) + self._listener.close() + + +@pytest.fixture +def dcs() -> Iterator[tuple[_FakeDc, _FakeDc]]: + """``(primary, referred)``: the primary refers every search to ``referred``.""" + referred = _FakeDc() + primary = _FakeDc( + search_referral=f"ldap://127.0.0.1:{referred.port}/OU=Users,DC=other,DC=example?sub" + ) + try: + yield primary, referred + finally: + primary.close() + referred.close() + + +def _settings(primary: _FakeDc, **over: Any) -> AuthSettings: + return _ad_settings( + ad_server=f"ldap://127.0.0.1:{primary.port}", ad_allow_insecure_ldap=True, **over + ) + + +def _revert(monkeypatch: pytest.MonkeyPatch, *, connection: bool, server: bool) -> None: + """Put back ldap3's referral defaults on the engine's own constructions, the mutation arms.""" + real_connection, real_server = ldap3.Connection, ldap3.Server + + class Following(real_connection): # type: ignore[misc, valid-type] + def __init__(self, *args: Any, **kwargs: Any) -> None: + kwargs["auto_referrals"] = True + super().__init__(*args, **kwargs) + + class AnyHost(real_server): # type: ignore[misc, valid-type] + def __init__(self, *args: Any, **kwargs: Any) -> None: + kwargs["allowed_referral_hosts"] = None + super().__init__(*args, **kwargs) + + if connection: + monkeypatch.setattr(ldap3, "Connection", Following) + if server: + monkeypatch.setattr(ldap3, "Server", AnyHost) + + +# --- end to end, through a real ldap3 stack -------------------------------------------------------- + + +def test_a_referred_search_is_refused_and_the_referred_host_never_hears_from_the_engine( + dcs: tuple[_FakeDc, _FakeDc], +) -> None: + """The fix. The primary answers the user search with a referral; the engine raises instead of + following, and the referred server accepts no connection at all.""" + primary, referred = dcs + with pytest.raises(LdapError) as refused: + LdapAuthenticator(_settings(primary)).probe_principal("jsmith") + + assert primary.accepted == 1, "the primary was never asked, so nothing below is measured" + assert referred.accepted == 0, "the engine followed the referral" + message = str(refused.value) + assert "referral" in message and "user search" in message and "127.0.0.1" in message + # The host is named; the rest of the URL, which is the directory's text, is not. + assert "DC=other" not in message + assert _BIND_PASSWORD not in message + + +@pytest.mark.parametrize( + ("connection", "server"), + [(True, False), (False, True)], + ids=["auto_referrals-reverted", "allowed_referral_hosts-reverted"], +) +def test_each_guard_alone_still_stops_the_follow( + dcs: tuple[_FakeDc, _FakeDc], + monkeypatch: pytest.MonkeyPatch, + connection: bool, + server: bool, +) -> None: + """Belt and braces, measured: with either guard put back to ldap3's default, the other still + stops ldap3 from opening a connection to the referred host.""" + primary, referred = dcs + _revert(monkeypatch, connection=connection, server=server) + with pytest.raises(LdapError): + LdapAuthenticator(_settings(primary)).probe_principal("jsmith") + assert referred.accepted == 0 + + +def test_with_both_guards_reverted_ldap3_carries_the_bind_password_to_the_referred_host( + dcs: tuple[_FakeDc, _FakeDc], + monkeypatch: pytest.MonkeyPatch, +) -> None: + """THE POSITIVE CONTROL, and the defect as it was. With ldap3's defaults back on both, ldap3 + connects to the referred host and binds there with the service-account password, and the search + comes back as a quiet "no such account". Without this arm, ``accepted == 0`` above could be a + server that cannot be reached rather than a referral that was not followed.""" + primary, referred = dcs + _revert(monkeypatch, connection=True, server=True) + probe = LdapAuthenticator(_settings(primary)).probe_principal("jsmith") + + assert referred.accepted == 1 + assert _BIND_PASSWORD.encode() in bytes(referred.received), ( + "the referred host did not receive the bind password, so this arm no longer shows the leak " + "the guards exist to stop" + ) + assert probe.answer is ldap_module.DirectoryAnswer.NOT_FOUND + + +async def test_a_referred_kerberos_sign_in_is_audited_as_a_login_error( + dcs: tuple[_FakeDc, _FakeDc], monkeypatch: pytest.MonkeyPatch +) -> None: + """Where an operator meets it: a Windows SSO sign-in whose directory lookup is referred fails as + ``directory unavailable`` and writes ``auth.login_error`` naming the referral. Nothing reaches + the referred host, and neither password is on the audit row.""" + + async def no_sleep(_deadline: float) -> None: + return None + + monkeypatch.setattr("messagefoundry.auth.service._sleep_until", no_sleep) + monkeypatch.setattr("messagefoundry.auth.service.kerberos_principal", lambda _t, _s: "jsmith") + primary, referred = dcs + settings = _settings(primary, kerberos_enabled=True) + store = await MessageStore.open(":memory:") + try: + service = AuthService(store, settings, ldap=LdapAuthenticator(settings)) + await service.initialize() + out = await service.authenticate_kerberos(b"spnego-token") + + assert not out.ok + rows = [str(dict(r)["detail"]) for r in await store.list_audit(action="auth.login_error")] + assert len(rows) == 1 and "referral" in rows[0] and "user search" in rows[0] + assert _BIND_PASSWORD not in rows[0] + assert await store.list_audit(action="auth.login_success") == [] + assert referred.accepted == 0 + finally: + await store.close() + + +# --- the user bind and the group search, through the recording doubles ------------------------------ + + +def test_a_referred_user_bind_is_refused_not_read_as_a_wrong_password( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A referred bind returns ``False`` from ldap3, the same as a rejected password. Read that way, + the step-up re-bind would count it toward the engine lockout. It raises instead, which the + re-bind reads as ``directory_unavailable`` and does not count.""" + _install_fakes(monkeypatch, refer="bind") + with pytest.raises(LdapError, match=r"user bind with a referral to dc9\.other\.example"): + LdapAuthenticator(_ad_settings()).authenticate("alice", _USER_PASSWORD) + + +def test_a_referred_group_search_is_refused_not_read_as_no_groups( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A referred nested-group search would otherwise read as "no nested groups", which drops + roles without a word. It raises on every path that resolves groups.""" + _install_fakes(monkeypatch, refer="group") + with pytest.raises(LdapError, match="group search"): + LdapAuthenticator(_ad_settings()).authenticate("alice", _USER_PASSWORD) + with pytest.raises(LdapError, match="group search"): + LdapAuthenticator(_ad_settings()).probe_principal("alice") + + +def test_the_doubles_control_signs_in_when_nothing_is_referred( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """THE CONTROL for the two refusals above: the same doubles, with no referral, sign in.""" + _install_fakes(monkeypatch) + principal = LdapAuthenticator(_ad_settings()).authenticate("alice", _USER_PASSWORD) + assert principal is not None and principal.username == "alice" + + +# --- the refusal's text ---------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "result", + [None, {"result": 0}, {"result": 32, "referrals": ["ldap://x/"]}], + ids=["no-operation-yet", "success", "no-such-object"], +) +def test_only_a_referral_result_is_refused(result: dict[str, Any] | None) -> None: + ldap_module._refuse_referral(SimpleNamespace(result=result), "user search") + + +def test_the_doubles_answer_with_ldap3s_referral_code() -> None: + """The doubles spell the code as a literal; ``_refuse_referral`` reads ldap3's constant.""" + assert REFERRAL_RESULT["result"] == RESULT_REFERRAL + + +def test_the_refusal_names_hosts_and_nothing_else_the_directory_sent() -> None: + """An LDAP URL can carry a DN, a filter and a bindname extension (RFC 4516). Only the host is + named, and a host that is not a plain host name is replaced rather than logged.""" + referrals = [ + "ldap://dc2.other.example:389/DC=other,DC=example??sub?(cn=x)?bindname=CN%3Dsvc", + "ldap://dc2.other.example/DC=again", + "ldap://evil\x1b[2Jhost/DC=x", + "not a url", + ] + with pytest.raises(LdapError) as refused: + ldap_module._refuse_referral( + SimpleNamespace(result={"result": RESULT_REFERRAL, "referrals": referrals}), + "user search", + ) + message = str(refused.value) + assert "referral to dc2.other.example, ;" in message + for leaked in ("DC=other", "bindname", "cn=x", "\x1b", "evil"): + assert leaked not in message + + +def test_the_refusal_names_a_few_hosts_and_counts_the_rest() -> None: + """A directory decides how many referrals it sends, so the log line and audit row stay short.""" + referrals = [f"ldap://dc{i}.other.example/" for i in range(5)] + ["ldap://dc_9.corp.example/"] + with pytest.raises(LdapError) as refused: + ldap_module._refuse_referral( + SimpleNamespace(result={"result": RESULT_REFERRAL, "referrals": referrals}), "user bind" + ) + message = str(refused.value) + assert "dc0.other.example, dc1.other.example, dc2.other.example and 3 more;" in message + assert "dc3" not in message + # An underscore is legal in an AD host name, and the bad-URL placeholder never takes a slot + # ahead of a readable host. + assert ldap_module._referred_hosts(["ldap://dc_9.corp.example/"]) == "dc_9.corp.example" + assert ldap_module._referred_hosts(["not a url"] * 2 + referrals[:3]) == ( + "dc0.other.example, dc1.other.example, dc2.other.example and 1 more" + ) + assert ldap_module._referred_hosts([]) == "" + + +# --- static: no construction site may bring ldap3's referral defaults back -------------------------- + + +def test_every_search_in_the_ldap_module_goes_through_the_refusing_wrapper() -> None: + """A search called directly would read a referral as "no entries" again. ``_search`` is the one + place a ``.search(...)`` call may appear in ``auth/ldap.py``, at any depth, async or not. A + regular expression's ``.search`` would go red here too; name it in this test if one is added.""" + tree = ast.parse(Path(ldap_module.__file__).read_text(encoding="utf-8")) + owner = { + id(node): scope.name + for scope in ast.walk(tree) + if isinstance(scope, ast.FunctionDef | ast.AsyncFunctionDef) + for statement in scope.body # the body only: defaults and decorators run outside it + for node in ast.walk(statement) + } # the innermost scope wins: ast.walk visits outer functions first, and later keys overwrite + callers = [ + owner.get(id(node), "") + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "search" + ] + assert callers == ["_search"], callers + + +def test_every_ldap3_construction_site_turns_referrals_off() -> None: + """Every ``ldap3.Connection`` in the package passes ``auto_referrals=False`` and every + ``ldap3.Server`` passes ``allowed_referral_hosts=[]``, both as literals. A new construction site + that omits either goes red here even if no runtime test reaches it.""" + sites = _ldap3_construction_sites() + # The absence check below walks `sites`: at least three Connection sites and one Server site. + assert len(sites) >= 4, sites + kinds = [attr for attr, _module, _line, _kw in sites] + assert kinds.count("Server") >= 1 and kinds.count("Connection") >= 3, sites + + def literal(node: ast.expr | None) -> object: + # Only a literal proves the value at every call; a name or an attribute could be anything. + try: + return ast.literal_eval(node) if node is not None else "" + except ValueError: + return "" + + wrong = [ + f"{attr} at {module}:{line}" + for attr, module, line, kwargs in sites + if (attr == "Connection" and literal(kwargs.get("auto_referrals")) is not False) + or (attr == "Server" and literal(kwargs.get("allowed_referral_hosts")) != []) + ] + assert not wrong, ( + "ldap3 construction site(s) that leave referral following on; ldap3 would re-send the " + f"bind credentials to whatever host a referral names (BACKLOG #2530): {wrong}" + ) diff --git a/tests/test_ldap_timeouts.py b/tests/test_ldap_timeouts.py index 6d895e3ed..caccec53f 100644 --- a/tests/test_ldap_timeouts.py +++ b/tests/test_ldap_timeouts.py @@ -24,8 +24,9 @@ module other than ``auth/ldap.py`` (a reconciler path, a future SPNEGO/LDAP helper, a new federation module), which a single-file walk would have missed entirely while advertising full coverage. The static half also rejects an explicitly unbounded literal (``receive_timeout=None`` - or ``=0``), which a keyword-presence check alone would accept; a non-literal value (a settings - attribute) is covered by the runtime half's finiteness assertions. + or ``=0``), which a keyword-presence check alone would accept. It also requires every + ``Connection``'s ``receive_timeout`` to be a ``_ldap3_receive_timeout(...)`` call, so a raw + settings float is refused statically too. *Disclosed limit:* an aliased module import (``import ldap3 as l3``) or a factory that returns a ``Connection`` from elsewhere is not resolved. Those forms do not exist at HEAD and the runtime @@ -37,16 +38,21 @@ from __future__ import annotations import ast +import functools +import math +import struct from pathlib import Path from typing import Any import pytest -from messagefoundry.auth.ldap import AdPrincipal, LdapAuthenticator +from messagefoundry.auth.ldap import AdPrincipal, LdapAuthenticator, _ldap3_receive_timeout from messagefoundry.config.settings import AuthSettings _CONNECT_TIMEOUT = 7.5 # deliberately not the default, so a hardcoded literal cannot pass -_RECEIVE_TIMEOUT = 9.25 +# Deliberately not the default, AFTER rounding: its ceiling is 12, not the default's 10, so a +# hardcoded 10 cannot pass. Fractional on purpose: ldap3 needs an int on POSIX (see below). +_RECEIVE_TIMEOUT = 11.25 def _ad_settings(**over: Any) -> AuthSettings: @@ -99,10 +105,21 @@ def __init__(self) -> None: self.unbinds: list[str | None] = [] -def _install_fakes(monkeypatch: pytest.MonkeyPatch, *, bind_ok: bool = True) -> _Recorder: +#: What ``_install_fakes(refer=...)`` answers a referred operation with (BACKLOG #2530). +REFERRAL_RESULT: dict[str, Any] = { + "result": 10, # RFC 4511 resultCode referral + "referrals": ["ldaps://dc9.other.example:636/DC=other"], +} + + +def _install_fakes( + monkeypatch: pytest.MonkeyPatch, *, bind_ok: bool = True, refer: str = "" +) -> _Recorder: """Install recording ``ldap3`` doubles. ``bind_ok=False`` makes every EXPLICIT bind fail, which is how the valid-user-wrong-password branch is driven (the service-account connection is built - with ``auto_bind=True``, which these doubles do not honour, so it is never an explicit bind).""" + with ``auto_bind=True``, which these doubles do not honour, so it is never an explicit bind). + ``refer`` names the operation, ``"bind"`` (explicit binds) or ``"group"`` (the group search), + that the directory answers with a referral instead.""" import ldap3 rec = _Recorder() @@ -116,6 +133,7 @@ def __init__(self, server: Any = None, **kwargs: Any) -> None: rec.connections.append({"server": server, **kwargs}) self.entries: list[_FakeEntry] = [] self._bind_dn: str | None = kwargs.get("user") + self.result: dict[str, Any] | None = None # no referral def __enter__(self) -> FakeConnection: return self @@ -125,6 +143,12 @@ def __exit__(self, *exc: object) -> None: def search(self, **kwargs: Any) -> bool: base = str(kwargs.get("search_base", "")) + # Every operation sets result afresh, as ldap3 does. + self.result = {"result": 0} + if base.startswith("OU=Groups") and refer == "group": + self.result = REFERRAL_RESULT + self.entries = [] + return False if base.startswith("OU=Groups"): self.entries = [ _FakeEntry( @@ -149,6 +173,10 @@ def search(self, **kwargs: Any) -> bool: def bind(self) -> bool: rec.binds.append(self._bind_dn) + if refer == "bind": + self.result = REFERRAL_RESULT + return False + self.result = {"result": 0 if bind_ok else 49} # 49: invalidCredentials return bind_ok def unbind(self) -> None: @@ -178,14 +206,35 @@ def _assert_all_finite(rec: _Recorder) -> None: assert isinstance(value, int | float) and 0 < float(value) < float("inf"), ( f"ldap3.Connection #{i} receive_timeout is not a finite positive number: {value!r}" ) - assert float(value) == _RECEIVE_TIMEOUT, ( - f"ldap3.Connection #{i} receive_timeout {value!r} is not [auth].ad_receive_timeout" + # An INT, not merely a number; _ldap3_receive_timeout's docstring says why. These fakes never + # open a socket, so only this assertion sees the type in this file; the real-socket arm is + # tests/test_ldap_referrals.py, which went red on Linux CI over it. + assert type(value) is int, ( + f"ldap3.Connection #{i} receive_timeout {value!r} is not an int; ldap3's POSIX branch " + "struct.pack()s it, so a float breaks every AD connection on Linux" + ) + assert value == math.ceil(_RECEIVE_TIMEOUT), ( + f"ldap3.Connection #{i} receive_timeout {value!r} is not [auth].ad_receive_timeout " + "rounded up to whole seconds" ) # --- runtime guard ------------------------------------------------------------------------------ +@pytest.mark.parametrize(("seconds", "expected"), [(10.0, 10), (9.25, 10), (0.001, 1), (3, 3)]) +def test_receive_timeout_is_whole_seconds_rounded_up(seconds: float, expected: int) -> None: + """Never shorter than configured, never 0 (ldap3's "wait forever"), and always an int that + ldap3's POSIX ``struct.pack('LL', ...)`` accepts. The float arm is the control: it is what the + engine used to pass, and it must raise, or this test is not measuring the Linux failure.""" + value = _ldap3_receive_timeout(seconds) + assert type(value) is int and value == expected + struct.pack("LL", value, 0) + if isinstance(seconds, float): + with pytest.raises(struct.error): + struct.pack("LL", seconds, 0) + + def test_authenticate_builds_only_finitely_timed_ldap_objects( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -372,6 +421,7 @@ def test_the_equalizing_bind_is_never_aimed_at_a_principal_the_caller_named() -> _REQUIRED_KWARG = {"Server": "connect_timeout", "Connection": "receive_timeout"} +@functools.cache # one package parse per session; tests/test_ldap_referrals.py walks it too def _ldap3_construction_sites() -> list[tuple[str, str, int, dict[str, ast.expr]]]: """``(ldap3 attribute, module, line number, {keyword: value node})`` for every ldap3 ``Server`` / ``Connection`` construction anywhere in the ``messagefoundry`` package. @@ -430,7 +480,7 @@ def test_every_ldap3_construction_site_passes_a_timeout() -> None: # Sanity: the package walk must still find the three known auth/ldap.py sites, or it has gone # vacuously green (a moved file, a renamed package, a broken walker). kinds = [attr for attr, _module, _line, _kw in sites] - assert kinds.count("Server") >= 1 and kinds.count("Connection") >= 2, ( + assert kinds.count("Server") >= 1 and kinds.count("Connection") >= 3, ( f"AST walk found an unexpected ldap3 construction set: {sites}" ) assert any(module.endswith("auth/ldap.py") for _a, module, _l, _k in sites), ( @@ -446,8 +496,8 @@ def test_every_ldap3_construction_site_passes_a_timeout() -> None: f"controller would block the login thread forever (ASVS 13.1.3): {untimed}" ) # Keyword PRESENCE is not enough: `receive_timeout=None` (or `=0`) satisfies a presence check and - # restores the unbounded wait. Reject a literal None/0 statically; a non-literal (a settings - # attribute) is covered by the finiteness assertions in the runtime fakes above. + # restores the unbounded wait. Reject a literal None/0 statically; the next check refuses a raw + # settings attribute on a Connection. unbounded = [ f"{attr} at {module}:{line} passes {_REQUIRED_KWARG[attr]}={ast.unparse(kwargs[_REQUIRED_KWARG[attr]])}" for attr, module, line, kwargs in sites @@ -458,6 +508,57 @@ def test_every_ldap3_construction_site_passes_a_timeout() -> None: "ldap3 construction site(s) passing an explicitly UNBOUNDED timeout — the keyword is present " f"but means 'wait forever' (ASVS 13.1.3): {unbounded}" ) + # A Connection's receive_timeout must go through _ldap3_receive_timeout. A raw settings float + # passes every check above and still raises struct.error on every non-Windows host, and the + # runtime fakes see only the sites a test happens to drive. + unconverted = _unconverted_receive_timeouts(sites) + assert not unconverted, ( + "ldap3.Connection site(s) passing receive_timeout without _ldap3_receive_timeout(); ldap3 " + f"struct.pack()s it on POSIX, so a float breaks every AD connection on Linux: {unconverted}" + ) + + +def _unconverted_receive_timeouts( + sites: list[tuple[str, str, int, dict[str, ast.expr]]], +) -> list[str]: + return [ + f"Connection at {module}:{line} passes receive_timeout={ast.unparse(value)}" + for attr, module, line, kwargs in sites + if attr == "Connection" + and (value := kwargs.get("receive_timeout")) is not None + and not _is_receive_timeout_call(value) + ] + + +def _is_receive_timeout_call(node: ast.expr) -> bool: + if not isinstance(node, ast.Call): + return False + func = node.func + name = ( + func.id + if isinstance(func, ast.Name) + else func.attr + if isinstance(func, ast.Attribute) + else None + ) + return name == "_ldap3_receive_timeout" + + +def test_the_receive_timeout_guard_discriminates() -> None: + """Control for the guard above: it must refuse the raw settings attribute the engine used to + pass, and accept the wrapped form, or its green on the real tree measures nothing.""" + + def site(arg: str) -> tuple[str, str, int, dict[str, ast.expr]]: + call = ast.parse(f"ldap3.Connection(s, receive_timeout={arg})", mode="eval").body + assert isinstance(call, ast.Call) + return ("Connection", "snippet.py", 1, {kw.arg: kw.value for kw in call.keywords if kw.arg}) + + raw = site("self._s.ad_receive_timeout") + wrapped = site("_ldap3_receive_timeout(self._s.ad_receive_timeout)") + assert _unconverted_receive_timeouts([raw]) == [ + "Connection at snippet.py:1 passes receive_timeout=self._s.ad_receive_timeout" + ] + assert _unconverted_receive_timeouts([wrapped]) == [] def test_static_walker_sees_the_bare_import_call_form() -> None: @@ -529,6 +630,17 @@ def test_ad_timeout_rejects_an_unbounded_value(bad: float) -> None: _ad_settings(ad_receive_timeout=bad) +@pytest.mark.parametrize("field", ["ad_connect_timeout", "ad_receive_timeout"]) +def test_ad_timeout_rejects_a_value_that_overflows_the_socket(field: str) -> None: + """A huge finite value raises OverflowError in socket.settimeout, which no LdapError handler + catches. The bound itself is accepted, so the refusal is the cap and not something else.""" + assert getattr(_ad_settings(**{field: 3600.0}), field) == 3600.0 + with pytest.raises(ValueError, match="at most 3600 seconds"): + _ad_settings(**{field: 3600.5}) + with pytest.raises(ValueError, match="at most 3600 seconds"): + _ad_settings(**{field: 1e300}) + + # --- resource release: the failed-bind path is the common adversarial case ----------------------