From ec9e7cf43f3eb984b9740a259ff88a71c31d703a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 18:24:49 -0500 Subject: [PATCH 01/14] feat(auth): LDAPS handshakes on an engine-built, narrowed TLS context (BACKLOG #2494) On an interpreter with SSLContext.set_ciphersuites (Python 3.15) the approved list drops TLS_AES_128_GCM_SHA256. ldap3 2.9.1 builds its own context and could only narrow TLS 1.2, so assert_ldap3_tls_suites refused and a first deployment on 3.15 would have lost AD sign-in. Option 3 of the item, chosen by the Manager: narrow by another route. assert_ldap3_tls_suites now returns a factory, like the hvac one. It calls create_default_context(SERVER_AUTH, cadata=...) as ldap3 did, sets check_hostname and verify_mode in ldap3's order, then adds the posture of every engine client hop: TLS 1.2 floor, strict X.509 flags, kex pin, narrow_to_approved_suites (TLS 1.2, TLS 1.3, sigalgs), the assertion, and the approved-list hold. The new auth/ldap_tls.NarrowedTls wraps each connection with a fresh context from it and still calls ldap3's own host name check. Its wrap step is copied from ldap3; a test pins ldap3's wrap_socket source hash. The bind passes ldap3 no ciphers= any more. Measured on CPython 3.15.0b3: the bind constructs, offers only the two approved TLS 1.3 suites, and refuses a TLS 1.3 AES-128-only DC that ldap3's own Tls accepts. 3.14 is unchanged on the wire. ADR 0188 amendment, ADR 0180 Amendment E, ADR 0173 and PHI.md corrected. --- CHANGELOG.md | 17 +- docs/ASVS-L2-PHASE0-CHANGES.md | 2 +- docs/PHI.md | 2 +- ...ss-terminating-and-originating-surfaces.md | 4 +- ...on-a-library-that-exposes-no-sslcontext.md | 8 +- ...iphers-on-the-mllp-and-dicom-connectors.md | 89 +++++++- docs/adr/README.md | 2 +- messagefoundry/auth/ldap.py | 63 ++---- messagefoundry/auth/ldap_tls.py | 64 ++++++ messagefoundry/config/tls_policy.py | 199 +++++++---------- scripts/security/crypto_inventory_check.py | 9 +- tests/test_key_lifecycle_coverage.py | 1 + tests/test_ldap_tls.py | 168 ++++++++++++++ tests/test_tls_cipher_assertion_sites.py | 208 +++++------------- tests/test_tls_default_suites.py | 4 + tests/test_tls_handshake_sigalgs.py | 22 +- 16 files changed, 519 insertions(+), 343 deletions(-) create mode 100644 messagefoundry/auth/ldap_tls.py create mode 100644 tests/test_ldap_tls.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 31b6dde9d..629184afe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1343,6 +1343,19 @@ All notable changes to MessageFoundry are documented here. The format follows ([BACKLOG #1141](docs/BACKLOG.md)) ### Security +- **LDAPS to Active Directory would no longer fail closed on Python 3.15, and it narrows TLS 1.3.** + On an interpreter with `SSLContext.set_ciphersuites`, the approved list drops + `TLS_AES_128_GCM_SHA256`. ldap3 builds its own TLS context and could not narrow TLS 1.3, so the + engine refused to build the AD authenticator there. The engine now builds the LDAPS context + itself: a new `ldap3.Tls` subclass, `messagefoundry.auth.ldap_tls.NarrowedTls`, wraps each + connection with a context from `tls_policy.assert_ldap3_tls_suites`, which now returns a factory. + That context loads the CA as ldap3 did and carries the posture every engine-built client hop + has: the approved TLS 1.2 suites, the approved TLS 1.3 suites and the SHA-224-free signature + schemes where the interpreter allows them, the key-exchange pin and a TLS 1.2 floor. ldap3's own + host name check still runs after the handshake. `ciphers=` is no longer passed to ldap3. + Python 3.14 is unchanged on the wire: its TLS 1.3 gap is recorded, not closed. A followed + referral still gets a plain ldap3 context. (`BACKLOG #2494`, owner ruling R3 of + 2026-09-27, ADR 0188 amendment of 2026-09-30) - **On Python 3.15 the engine stops offering SHA-224 TLS signature schemes.** Every context the engine narrows drops `rsa_pkcs1_sha224`, `ecdsa_sha224` and `dsa_sha224` through `SSLContext.set_server_sigalgs`. Read from the OpenSSL source, not yet measured on 3.15, that one @@ -1353,8 +1366,8 @@ All notable changes to MessageFoundry are documented here. The format follows three SHA-224 schemes; `rsa_pss_rsae_*` now comes before `rsa_pss_pss_*`. A build that refuses the ML-DSA names still drops SHA-224, without ML-DSA, and logs a warning once. Python 3.14 is unchanged. A 3.15 - on an OpenSSL older than 3.4 pins nothing and logs a warning once. The LDAPS hop is not reached. - (`BACKLOG #1171`, ASVS 11.4.1, owner ruling 2026-09-29) + on an OpenSSL older than 3.4 pins nothing and logs a warning once. The LDAPS hop was not + reached; since BACKLOG #2494, above, it is. (`BACKLOG #1171`, ASVS 11.4.1, owner ruling 2026-09-29) - **A refused combined sign-in no longer names which factor was wrong in its `auth.login_failed` reason.** Every refused combined sign-in (password and TOTP code in one request) on a local account with TOTP enrolled now writes the same reason, `bad_credentials`, whether the password was diff --git a/docs/ASVS-L2-PHASE0-CHANGES.md b/docs/ASVS-L2-PHASE0-CHANGES.md index 5bbdfcb5c..e8c97e78c 100644 --- a/docs/ASVS-L2-PHASE0-CHANGES.md +++ b/docs/ASVS-L2-PHASE0-CHANGES.md @@ -182,7 +182,7 @@ for, not how it is protected before it gets there. | DIRECT S/MIME (opt-in, [ADR 0085](adr/0085-direct-hisp-smime-connector.md)) | CMS **sign-then-encrypt** in `transports/direct.py` (core `cryptography` `serialization.pkcs7`): PKCS#7 signature over the body with a **SHA-256** digest, the public-key signature algorithm (RSA / ECDSA) following the loaded signing key type (not pinned to RSA), then a PKCS#7 **envelope** to the partner's recipient cert. The envelope content-encryption cipher is **AES-256-CBC, pinned in code**; `_CONTENT_CIPHER` in `transports/direct.py` records why (BACKLOG #1168). The content key is still wrapped with RSAES-PKCS1-v1_5, which the library offers no way to change, so the message is no stronger than the recipient's RSA key | Sender **signing cert** + PEM **private key** (optional `signing_key_password`) and the per-partner **`recipient_cert`**, all operator-supplied files; the recipient cert is trust-verified at construction against an operator `trust_anchor` (one-level direct-issuance check); key/cert mismatch refused. **Usage scope:** the sender signing key signs the CMS body and the partner's `recipient_cert` encrypts the CMS envelope — this material protects the **confidentiality + authenticity of a DIRECT message to one partner in transit**; it is not an at-rest store key and encrypts nothing in the store | **OFF by default** — only when a DIRECT Connection is configured, and its HISP relay host is gated by the **opt-in** `[egress].allowed_direct` allow-list (empty by default = unrestricted; an unlisted host is refused only once the list is populated, or outright when `[security].block_unlisted_outbound` is set). Signing key + recipient certs rotate on the schedule below | | OIDC IdP JWKS verification keys (opt-in, [ADR 0142](adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md)) | **Public** verifying keys fetched from the IdP JWKS: **RS256/PS256** (RSA, ≥ 2048-bit floor) and **ES256/ES384** (EC P-256/P-384) — rebuilt from each JWK by `cryptography` in [`auth/oidc/jwks.py`](../messagefoundry/auth/oidc/jwks.py); the closed `SignatureAlgorithm` enum forecloses `alg:none` and RS256→HS256 confusion. Bounded, TTL-cached (`DEFAULT_JWKS_TTL_SECONDS`), a 512 KiB body cap, a global min-refetch floor (fetch-amplification bound), and a hard refusal of a duplicate `kid`; a key below the floor is skipped/refused, never merely warned. **Usage scope:** these are **public**, non-secret keys used **only** to verify the IdP's id-token signature at console login — they encrypt nothing and can protect no data; the engine holds no private half. | Fetched from the IdP JWKS URI over the CA-pinned no-redirect opener (row below); held process-local in `JwksCache`, never persisted, never logged | Refetched per TTL / on an unknown `kid` within the amplification bound; rolls when the IdP rotates its signing keys | | OIDC IdP TLS trust anchor (`[auth].oidc_tls_ca_cert_file`, opt-in, [ADR 0142](adr/0142-federated-sso-oidc-authorization-code-pkce-relying-party-hybrid-ad-backed.md)) | Pins the CA that must anchor the IdP's TLS server cert on **both** federated-SSO legs (JWKS fetch + token endpoint) — the hardened, no-redirect `ssl` opener in [`auth/oidc_http.py`](../messagefoundry/auth/oidc_http.py) (mirrors `ad_tls_ca_cert_file`); unset ⇒ the OS trust store via `truststore`. No insecure/`verify=False` escape exists — the IdP hop carries an authentication assertion. **Usage scope:** a **trust anchor**, not a key the engine holds — it authenticates the IdP endpoint's TLS identity only; it signs and encrypts nothing and protects no at-rest data. | Operator-supplied CA PEM path (`[auth].oidc_tls_ca_cert_file`) or the OS trust store | Managed by the operator / OS trust store; rotate on IdP CA change | -| AD transport | LDAPS (TLS) with `CERT_REQUIRED` by default; optional internal CA via `ad_tls_ca_cert_file`. On an LDAPS bind the engine reads that file once, when it builds the authenticator, checks it against the optional SHA-256 pin `ad_tls_ca_cert_pin`, and hands ldap3 those same bytes as `ca_certs_data` on every bind (BACKLOG #2034), in [`auth/ldap.py`](../messagefoundry/auth/ldap.py). **Usage scope:** a **trust anchor**, not a key the engine holds. It authenticates only the domain controller's TLS identity on the authenticator's LDAPS connections. Those carry at least the service-account bind, the user lookup and group searches, and a password user's own bind; the password-free lookups that derive roles for Kerberos and OIDC users ride them too. It signs and encrypts nothing and protects no at-rest data. The OIDC IdP TLS trust anchor row above mirrors it and is scoped the same way (BACKLOG #1930). | Operator-supplied CA PEM path (`[auth].ad_tls_ca_cert_file`) or the OS trust store | Managed by the operator / directory / OS trust store; replace it, and its pin, when the directory's issuing CA changes; the change takes a restart, because a reload re-checks the file but does not rebuild the authenticator | +| AD transport | LDAPS (TLS) with `CERT_REQUIRED` by default; optional internal CA via `ad_tls_ca_cert_file`. On an LDAPS bind the engine reads that file once, when it builds the authenticator, checks it against the optional SHA-256 pin `ad_tls_ca_cert_pin`, and loads those same bytes on every bind (BACKLOG #2034), in [`auth/ldap.py`](../messagefoundry/auth/ldap.py). Since BACKLOG #2494 the engine builds that TLS context itself, and [`auth/ldap_tls.py`](../messagefoundry/auth/ldap_tls.py) wraps each LDAPS connection with it. **Usage scope:** a **trust anchor**, not a key the engine holds. It authenticates only the domain controller's TLS identity on the authenticator's LDAPS connections. Those carry at least the service-account bind, the user lookup and group searches, and a password user's own bind; the password-free lookups that derive roles for Kerberos and OIDC users ride them too. It signs and encrypts nothing and protects no at-rest data. The OIDC IdP TLS trust anchor row above mirrors it and is scoped the same way (BACKLOG #1930). | Operator-supplied CA PEM path (`[auth].ad_tls_ca_cert_file`) or the OS trust store | Managed by the operator / directory / OS trust store; replace it, and its pin, when the directory's issuing CA changes; the change takes a restart, because a reload re-checks the file but does not rebuild the authenticator | | SQL Server transport | TLS via ODBC Driver 18 (`Encrypt=yes`, `TrustServerCertificate=no` by default); an optional operator pin through `[store].ssl_root_cert` has its own row, next | Server certificate | Managed by SQL Server / OS trust store | | Store server-certificate trust (`[store].ssl_root_cert`, opt-in, both server-DB backends, BACKLOG #45) | An operator-supplied certificate file the engine consumes to authenticate the store's database server. It applies **only on the secure posture** (`encrypt=true`, `trust_server_certificate=false`) and never turns verification off. Where it is enforced, it replaces the trust source rather than narrowing it, so it can admit a server certificate the OS trust store would reject; that is its purpose for a private CA. **The two backends read it differently, so it means a different thing on each.** **SQL Server:** it is meant to **pin the server's own certificate**. `connection_string` in [`store/sqlserver.py`](../messagefoundry/store/sqlserver.py) emits it as the ODBC Driver 18.1+ `ServerCertificate` keyword, so it is a leaf pin rather than a CA, and it needs Driver 18.1 or newer. Microsoft documents that keyword for strict encryption mode, and says its exact match is done instead of the standard checks (expiry, host name, trust chain). The engine emits `Encrypt=yes`, not `Encrypt=strict`. Whether the driver enforces the pin on that posture has not been measured, so a deploying site should not count on the pin until it has been. **PostgreSQL:** it is a **CA bundle**, loaded by `ssl.create_default_context(cafile=...)` in `_build_ssl` in [`store/postgres.py`](../messagefoundry/store/postgres.py), so the chain **and** the hostname are still verified. `[store].ssl_crl_file` loads onto this branch (BACKLOG #299) and, since BACKLOG #300, onto the PostgreSQL system-trust branch too, where the engine now builds the context itself. On both branches the engine builds a fresh context for every new pool connection, so a replaced bundle applies to each new connection. Connections already open keep the old one until the pool replaces them. Setting it for SQLite, which has no TLS, is refused at load. **Usage scope:** a **trust anchor**, not a key the engine holds. It authenticates only the store database server's TLS identity on the engine-to-store hop. It signs and encrypts nothing and protects no at-rest data; the store DEK is a separate row. | Operator-supplied certificate path (`[store].ssl_root_cert`). A path, not a secret, so it may sit in the config file; its existence is checked at load. Unset means the OS trust store | Managed by the operator. On SQL Server, replace it whenever the database server's certificate is renewed, because the pin is that exact certificate. On PostgreSQL, replace it when the issuing CA changes | | Console → engine TLS (a remote native client — the Qt-free `apiclient`, today the test harness; the PySide6 desktop console is retired) | Verifies the engine API server cert: **OS trust store** by default (`truststore.SSLContext`) or a pinned PEM via `cacert` (`ssl.create_default_context`); opt-in client cert (mTLS) via `load_cert_chain`; on either branch the TLS 1.2 suites are pinned to `_APPROVED_TLS12_SUITES` via `set_ciphers` (BACKLOG #300), so the client offers nothing wider than the engine listener's AEAD default, and since BACKLOG #2042 the TLS 1.3 suites to `_APPROVED_TLS13_SUITES` via `set_ciphersuites` where the interpreter has it (not CPython 3.14); `ssl` in `apiclient/client.py` (CONSOLE-3; extracted from the since-deleted `console/client.py` per ADR 0088) | OS trust store / operator-supplied CA PEM (`cacert`) | Managed by the OS trust store; `cacert` for a self-signed / internal-CA engine | diff --git a/docs/PHI.md b/docs/PHI.md index 5e12e139f..cd44f9821 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -926,7 +926,7 @@ BACKLOG #1171).** By owner ruling of 2026-09-29, every context the engine narrow SHA-224 signature schemes where the interpreter can: `narrow_signature_algorithms` in [config/tls_policy.py](../messagefoundry/config/tls_policy.py). On Python 3.14 it changes nothing, so those contexts still offer and accept SHA-224. On 3.15 they stop, unless the linked OpenSSL is -older than 3.4. The LDAPS hop is not reached. That function's docstring is the one statement of +older than 3.4. The LDAPS hop is reached too, since BACKLOG #2494. That function's docstring is the one statement of what it does, what else it changes, and what it cannot reach. **Outbound destination allowlist `[BUILT]` (WP-11c).** The `[egress]` section diff --git a/docs/adr/0173-tls-peer-revocation-checking-and-ocsp-stapling-across-terminating-and-originating-surfaces.md b/docs/adr/0173-tls-peer-revocation-checking-and-ocsp-stapling-across-terminating-and-originating-surfaces.md index 53c6ced3b..3eaed8e6b 100644 --- a/docs/adr/0173-tls-peer-revocation-checking-and-ocsp-stapling-across-terminating-and-originating-surfaces.md +++ b/docs/adr/0173-tls-peer-revocation-checking-and-ocsp-stapling-across-terminating-and-originating-surfaces.md @@ -863,9 +863,9 @@ tracking item, its row reads *Needs a guard, tracked by* that item. No row names | `transports/dicom.py`, `_client_ssl_context` (the C-STORE SCU) | imaging objects | **Needs a guard** | a verifying PHI hop. Its only hop guard is an `InsecureHopGuard`, taken when TLS is off. An opt-in CRL can already reach it through `[tls].crl_file` on the trust anchor | | `transports/remotefile.py`, `_ftps_ssl_context`, verifying arm | message files, the FTP login | **Needs a guard** | the same shape as the SCU. The file's only hop guard is an `InsecureHopGuard` for anonymous plain FTP | | `transports/remotefile.py`, `_ftps_ssl_context`, `tls_verify=false` arm | message files, the FTP login | **Verify-off** | refused on an enforcing instance even with the escape set | -| `auth/ldap.py`, `ldap3.Tls` in `_server`, `ad_tls_verify=true` | the service-account and user passwords | **Needs a guard** | a verifying credential hop, built outside the connector gate like the OIDC legs. **No CRL setting reaches it**: `ldap3.Tls` holds no `SSLContext`, so `harden_crl_check` has nothing to act on | +| `auth/ldap.py`, `ldap3.Tls` in `_server`, `ad_tls_verify=true` | the service-account and user passwords | **Needs a guard** | a verifying credential hop, built outside the connector gate like the OIDC legs. **No CRL setting reaches it.** This read "`ldap3.Tls` holds no `SSLContext`, so `harden_crl_check` has nothing to act on". Since BACKLOG #2494 the engine builds that context, and it simply loads no CRL (CORRECTED 2026-09-30) | | `auth/ldap.py`, `ad_tls_verify=false` | the same passwords | **Verify-off** | refused on an enforcing instance (#329) | -| `config/tls_policy.py`, `assert_ldap3_tls_suites` | nothing | **Not a PHI or credential hop** | a replica built to read the suite list. It never handshakes | +| `config/tls_policy.py`, `assert_ldap3_tls_suites` | the same passwords | **Not a separate hop** | since BACKLOG #2494 it builds the context the LDAPS row above handshakes on; it was a replica that never handshook (CORRECTED 2026-09-30) | | `pipeline/alert_sinks.py`, `send_plain_email`, through `build_smtp_tls_context` | alert bodies and per-user security-event notices (`pipeline/security_notify.py` sends through it too), the SMTP AUTH credential | **Needs a guard** | a verifying credential hop with no revocation guard, built outside the connector gate | | `pipeline/alert_sinks.py`, `_build_no_redirect_opener`, through `build_asserted_https_handler` (the alert webhook) | alert bodies; a Slack or Teams hook carries its secret in the URL | **Needs a guard** | a verifying credential hop with no revocation guard | | `transports/direct.py`, through `build_smtp_tls_context` | S/MIME-protected bodies, the SMTP AUTH credential | **Owner question** | a shipped comment declines the guard because S/MIME protects the body. That reason covers the body. It does not cover the AUTH credential when a username is set | 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 e4a94598e..7b01a82c8 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) +- **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) - **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` @@ -314,3 +314,9 @@ refused when the client is built, again before each send, and by the connection handshake on narrowed, verifying contexts; a CBC-only proxy, a wrong-name proxy and a proxy the anchor did not issue are each refused; and a urllib3 that ignored the supplied context is caught by the post-handshake check. + +## Amendment E (2026-09-30) -- the LDAPS replica is gone (BACKLOG #2494) + +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. diff --git a/docs/adr/0188-per-connection-tls-ciphers-on-the-mllp-and-dicom-connectors.md b/docs/adr/0188-per-connection-tls-ciphers-on-the-mllp-and-dicom-connectors.md index 5c7540f1f..45fb2cc5d 100644 --- a/docs/adr/0188-per-connection-tls-ciphers-on-the-mllp-and-dicom-connectors.md +++ b/docs/adr/0188-per-connection-tls-ciphers-on-the-mllp-and-dicom-connectors.md @@ -166,7 +166,8 @@ DICOM connector, unchanged here. the database hops). Several of them assert through a library's own context (`build_asserted_https_handler`, `assert_ldap3_tls_suites`) where applying an operator string is a different problem with its own traps — `ldap3` in particular **swallows** an invalid `ciphers=` string, which `assert_ldap3_tls_suites` -already refuses to go near. Retiring the six CBC-SHA2 suites from the inherited default: still gated on +already refuses to go near. (Since the 2026-09-30 amendment below, the engine builds the LDAPS +context itself and passes no `ciphers=` at all.) Retiring the six CBC-SHA2 suites from the inherited default: still gated on the peer census. ## To resolve on acceptance @@ -199,7 +200,7 @@ calls it when `tls_ciphers` is unset, so those seams still read as the three cal | SMTP (EMAIL, DIRECT transport, alert email), syslog forwarder, FTPS, OIDC IdP, Postgres store (pinned-CA and verify-off branches), `verify` smoke | interpreter list | approved list | | Postgres store, default verifying branch | asyncpg's own context | approved list since a BACKLOG #300 follow-up: the engine builds the context with the same `ssl.create_default_context()` call asyncpg made for `ssl=True`, then narrows it, where it used to pass `ssl=True` | | Windows tray `/health` probe (`tray/probe.py`) | `truststore` context | approved list since owner ruling 2026-09-27 (engine PR 1690): the probe holds its own copies of the approved tuples, because it is a separate stdlib-plus-httpx package (ADR 0113), and `tests/test_tls_default_suites.py` checks them against the engine's | -| LDAPS (`ldap3`) and all three Vault clients (`hvac`) | library's list | approved list since owner ruling 2026-09-27. `ldap3.Tls` gets `ciphers=` the approved names, and `assert_ldap3_tls_suites` refuses any other value. The Vault hops handshake on a urllib3 context `assert_hvac_tls_suites` builds and narrows. The accepted risk: an older domain controller that offers none of these suites fails to bind | +| LDAPS (`ldap3`) and all three Vault clients (`hvac`) | library's list | approved list since owner ruling 2026-09-27. **CORRECTED 2026-09-30:** this read "`ldap3.Tls` gets `ciphers=` the approved names, and `assert_ldap3_tls_suites` refuses any other value". Since the 2026-09-30 amendment below, the engine builds the LDAPS context itself and passes ldap3 no `ciphers=`. The Vault hops handshake on a urllib3 context `assert_hvac_tls_suites` builds and narrows. The accepted risk: an older domain controller that offers none of these suites fails to bind | | At least: the SQL Server store, the DATABASE connector and `db_lookup` (ODBC drivers) | library's list | **unchanged**: the library builds the context, as BACKLOG #1170's third category records | A configured `tls_ciphers` or `[api].tls_ciphers` still wins over the default, and still runs the @@ -333,3 +334,87 @@ In `tests/test_tls_default_suites.py`: `apply_operator_tls_ciphers`, which also narrows TLS 1.3. -> `test_every_operator_cipher_branch_narrows_tls13_too`, `test_no_engine_module_applies_a_cipher_string_outside_the_policy_module` + +--- + +## Amendment (2026-09-30): LDAPS is narrowed by an engine `Tls` subclass (BACKLOG #2494, option 3) + +**The LDAPS hop now handshakes on a context the engine builds, so it narrows TLS 1.3 like every +other engine-built hop.** Of the three options BACKLOG #2494 offered, the Manager chose option 3, +"narrow ldap3's TLS 1.3 list by another route". It meets owner ruling R3 of 2026-09-27 (the +library-built contexts are narrowed too) without the AES-128 exception option 2 would need. + +### Why it was needed + +ldap3 2.9.1 builds its context inside `Tls.wrap_socket` and takes no context from outside. Its one +suite lever, `ciphers=`, reaches TLS 1.2 only. On an interpreter with `set_ciphersuites`, the +allow-list drops `TLS_AES_128_GCM_SHA256` (the 2026-09-26 amendment above), but ldap3's context +still offered it, so `assert_ldap3_tls_suites` refused. A first deployment on Python 3.15 would have +lost Active Directory sign-in. The docstring recorded that as a fail-closed by design, pending this +decision. + +### What changed + +- `tls_policy.assert_ldap3_tls_suites` now returns a factory, as `assert_hvac_tls_suites` does. The + factory calls `ssl.create_default_context(Purpose.SERVER_AUTH, cadata=...)`, the call ldap3 + makes, then sets `check_hostname` and `verify_mode` in ldap3's order. It adds the posture every + engine-built client hop carries: a TLS 1.2 floor, strict X.509 flags when verifying, + `harden_kex_groups` and `narrow_to_approved_suites`. The last one sets TLS 1.2 suites, TLS 1.3 + suites and signature schemes wherever the interpreter allows each. Then `harden_cipher_suites` + asserts the context and a shared check holds it to the approved list. It runs once at + construction, then once per connection. +- `auth/ldap_tls.py` adds `NarrowedTls`, a subclass of `ldap3.Tls`. Its `wrap_socket` wraps the + socket with the factory's context and then calls ldap3's own host name check, unchanged. That + wrap step is copied from ldap3, and `tests/test_ldap_tls.py` pins a hash of ldap3's + `wrap_socket` source. Patching ldap3's module global for one call was rejected, because binds + run on worker threads and a module global is shared by all of them. +- The bind no longer passes `ciphers=`. `NarrowedTls` builds the factory itself from `validate` + and `ca_certs_data`, and takes no other ldap3 argument, because the engine's context would not + carry one. The authenticator builds one `NarrowedTls` and hands it to every `ldap3.Server`. + +| Where | TLS 1.2 | TLS 1.3 | Signature schemes | Key-exchange groups | +|---|---|---|---|---| +| LDAPS on CPython 3.14 | 5 approved suites (unchanged) | all three, `TLS_AES_128_GCM_SHA256` included: the recorded gap of the 2026-09-26 amendment | stock list | inherited | +| LDAPS where the interpreter has the setters (3.15) | 5 approved suites | the 2 approved suites | SHA-224 removed, as `narrow_signature_algorithms` reports | pinned, as `harden_kex_groups` reports | + +The suite columns of the 3.15 row were measured on CPython 3.15.0b3 with OpenSSL 3.5.7, locally. +The bind constructs, its context offers exactly the two approved TLS 1.3 suites, and a real TLS 1.3 +handshake against a peer offering only `TLS_AES_128_GCM_SHA256` fails. ldap3's own `Tls` completes +against the same peer, as the control. The last two columns are what each helper reported on that +run, not a handshake reading. + +### What it keeps + +CA loading, verification mode and the absence of SNI are what ldap3 did, for the arguments the +engine passes. ldap3's post-handshake host name check still runs, and still only when verifying. +Each connection gets a fresh context, as before. `tests/test_tls_cipher_assertion_sites.py` now +ties the context each connection wraps with to one the assertion ran on, by identity. The replica +and its equivalence test are gone. + +### What it does not cover + +- **Referrals.** When ldap3 follows a referral, it builds a plain `ldap3.Tls` for the referred + server from a few attributes of this one. That copy carries no `ca_certs_data` and none of the + narrowing. This amendment does not change that path, and neither did the ones before it. +- **Revocation.** The LDAPS context loads no CRL. It is still among the hops the DEPLOYMENT guide + lists as delegated. +- **Client certificates, a pinned protocol version, `ssl_options` and SNI.** The engine passes + none, and `NarrowedTls` accepts none. +- **The other TLS 1.3 residuals.** The 2026-09-26 amendment's 3.14 gap stands on every hop, LDAPS + included, and `tests/test_tls_default_suites.py` keeps its tripwire for the day the interpreter + changes. + +### Acceptance criteria + +- **AC-13** -- WHERE the interpreter has `set_ciphersuites`, THE SYSTEM SHALL build the LDAPS + authenticator without refusing, and its context SHALL offer exactly `APPROVED_TLS13_SUITES` at + TLS 1.3. WHEN it meets a TLS 1.3 peer offering only `TLS_AES_128_GCM_SHA256`, it SHALL fail the + handshake. -> `test_on_315_the_ldaps_bind_constructs_and_offers_no_tls13_aes128`, + `test_on_315_the_ldaps_bind_refuses_a_tls13_aes128_only_dc`, and on 3.14 + `test_on_314_the_ldaps_bind_keeps_the_recorded_tls13_gap` +- **AC-14** -- Every LDAPS connection SHALL wrap with a context the assertion ran on, and ldap3's + host name check SHALL still run when verifying. + -> `test_every_ldaps_connection_wraps_with_a_context_the_assertion_ran_on`, + `test_ldap3s_host_name_check_still_runs_after_the_handshake` +- **AC-15** -- The LDAPS hop is a client hop in `tests/test_tls_default_suites.py`, so AC-7, AC-8 + and AC-10 apply to it. diff --git a/docs/adr/README.md b/docs/adr/README.md index a4eff70d2..eb562d1db 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -211,7 +211,7 @@ what is withheld and what you can request. | [0185](0185-retention-levers-for-the-tamper-evident-audit-log-what-each-deletion-shape-costs-verifiability.md) | **Retention levers for the tamper-evident `audit_log`: what each deletion shape costs verifiability** (BACKLOG #1421) -- an options memo, deliberately choosing nothing. #1421's remaining limb is one ruling: which retention lever exists for a table that must stay tamper-evident. Establishes the mechanism from the code rather than from reasoning about audit logs in general -- `audit_row_hash` (`store/store.py:1009`) digests `[prev_hash, ts, actor, action, channel_id, detail]` plus a conditional `client`, the row `id` is NOT in the payload, and `record_audit` reads the head under the store lock, so chain order is `id` order and nothing else. A verifier exists and is reachable three ways (the `Store` protocol, `messagefoundry audit-verify`/`audit-anchor`, and `[integrity].audit_verify_on_start`, which ships `False`). Eight shapes driven at `c57903c2c` against a throwaway store, both controls firing, with a second full walk beside the shipped one because the shipped verifier returns only the FIRST divergent row. **Three findings are not on `main`:** a break is LOCAL and does not spread (successors chain from the STORED hash, so surviving rows keep their evidentiary value); deleting an INTERIOR row leaves the anchor head byte-identical, so an anchor catches it by COUNT and not by hash; and a tombstone that preserves the stored `row_hash` still breaks the walk at that row, so preserving the link bounds the damage without removing it. **The crux:** a delete-then-reseal verifies clean, which is exactly what an attacker who can write the table would produce, so a re-sealing purge would leave a held off-box anchor as the only working control. Six levers costed against chain, build and operator -- hard delete, tombstone, partitioning, archive-and-reseal, archive-first restore-capable, and a write-time bound (the only one whose cost is not paid in verifiability). Separates two things that are not retention levers: the ADR 0055 group committer (#1421 cost 2) and flipping the #1277 default back off | **Proposed (2026-09-05)** -- awaiting the owner ruling #1421 names; no code, no engine behaviour change, no lever chosen. Severity conditional per CLAUDE.md section 0 -- zero deployments, so nothing is growing today; a first deployment WOULD grow the table unbounded at one row per authenticated read. Carries a Builder recommendation marked as separable from the findings: make the anchor operational first, take the write-time bound as the primary lever, use archive-first with the case C1 contract if the table itself must be bounded, and reject both hard delete and re-seal | | [0186](0186-retire-the-synthetic-data-declaration-every-instance-carries-patient-data.md) | **Retire the synthetic-data declaration -- every instance carries patient data** (BACKLOG #1279) -- `[security].handles_real_patient_data = false` translated to `[ai].data_class = "synthetic"` and turned off **nineteen** start-up gates on one line (keyless at-rest, open-egress + the deny-by-default flip, the `--allow-insecure-bind` clamp, both proxy-attestation gates, MFA-at-exposure, dual-control, both TLS-terminator gates + the 12.1.1 floor probe, PHI retention, the security-notification channel + its deliverability check, the alert SMTP hop, memory-encryption-at-exposure, the API PHI serve hop, the outbound revocation hop). **Removed outright**, with `DataClass`: every instance carries patient data and the PHI gates apply unconditionally. Three findings drove it. (a) It was **not** the loud, audited opt-out the docs claimed -- `security_loosenings()` never named it, so the serve-time loosening warning never fired for the widest relaxation shipped, and the completeness test's exemption for it was both **false in its stated reason and structurally unreachable** (the loop skips non-`bool` defaults; this one defaults `None`). (b) Half the removal had already happened twice -- [ADR 0153](0153-collapse-the-posture-gradient-no-data-label-may-allow-a-cleartext-hop.md) stripped `is_phi` from the widest consumer on an argument that was never cleartext-specific, and [ADR 0148](0148-phi-default-posture-and-an-explicit-security-enforcement-level.md) GIVEN 1 left the label with opt-out as its only job. (c) Every one of the nineteen already had its **own** named, audited, separately-reported switch. Both spellings are now **REFUSED at load** (not ignored -- a config asserting the gates are off while the engine runs them all is a silent contradiction), with a message naming the per-gate replacements. `HopPosture` loses `is_phi`; `derived_posture()` / `require_posture()` return the production **tier** alone; the wire contract drops `data_class` + `synthetic_relaxation`. Retains ADR 0148 GIVEN 2, the production tier, and every per-gate + per-connection switch. **Resolves two ADR 0153 carve-outs by subtraction and makes its recorded follow-up load-bearing:** the API serve hop and the log forwarder restated the `not is_phi` ALLOW arm locally and now have **no** per-cell way to accept a risk under `enforce` (filed, unallocated -- an honest residual). CI's SQL Server load leg and the failover harness take per-gate relaxations instead, both **narrower** than the nineteen the declaration silenced. #26-clean | Accepted (2026-09-09) -- owner ruling; BUILT same day | | [0187](0187-bound-the-subprocess-sandbox-worker-count-a-shared-pool-or-router-phase-only-isolation.md) | **Bound the subprocess sandbox worker count: a shared pool or router-phase-only isolation** (BACKLOG #1458) -- an options memo over the growth [ADR 0087](0087-sandbox-subprocess-isolation.md)'s opt-in `mode="subprocess"` creates: one persistent, unpooled, never-evicted worker tree per traffic-carrying inbound, which is the per-connection-worker resource class [ADR 0052](0052-enterprise-scale-target.md) AC-2 names. Prices four shapes -- a bounded shared pool over both phases; router-phase-only isolation; the two combined (**recommended**); and a cap plus idle eviction -- and records per-connection `[sandbox]` settings as an orthogonal fifth. **Corrects the framing BACKLOG #1458 inherited:** the row's two candidate shapes are not alternatives, because router-phase-only isolation changes what runs in the child and not how many children exist. The load-bearing fact is that every child already runs `load_config` over the WHOLE config dir, so N children hold N identical registries and are interchangeable but for a log label. **Strongest objection carried, not buried:** the kill is the only cancellation primitive, so a pool escalates ADR 0087's accepted *"can deny its own feed"* residual into a cross-lane denial of service. **Carries a measurement** settling BACKLOG #1278's unresolved 22-versus-11 process count -- `Win32_Process` returns distinct pids in parent-child pairs, so the venv launcher's second process is real and ADR 0087's two-per-tree figure is the one to use; the live-engine test #1278 names is still unrun. **Decides nothing:** AC-2 cannot be closed by measurement while ADR 0052 `:108` records its connection-scale harness as non-existent, and the shape is the owner's ruling | **Proposed (2026-09-10)** -- awaiting the owner's shape ruling; no code, no engine behaviour change, no shape chosen. Severity conditional per CLAUDE.md section 0 -- zero deployments, and `mode="off"` ships, so this is what a deploying site that opted in would meet | -| [0188](0188-per-connection-tls-ciphers-on-the-mllp-and-dicom-connectors.md) | **Per-connection `tls_ciphers` on the MLLP and DICOM connectors** -- the strict cipher allow-list `_APPROVED_TLS_SUITES` governed exactly ONE operator setting, `[api].tls_ciphers`, which configures the hop a browser uses to reach the web console. Every PARTNER-facing hop -- MLLP listener and destination, DICOM SCP listener and SCU destination -- had no cipher lever at all: each builds an `SSLContext`, inherits the interpreter's suite list, and `harden_cipher_suites` ASSERTS four properties on it (forward secrecy, encryption, peer authentication, a 128-bit floor) while deliberately not applying the list. So the knob existed for the engine's own console and not for the hops that carry PHI between organisations. Decision: an **opt-in** `tls_ciphers` on `MLLP()` and `DICOM()`, both directions on each, validated by `validate_tls_ciphers` itself -- the same function the `[api].tls_ciphers` field validator calls, allow-list included -- so an operator cannot put a NULL, anonymous, non-forward-secret or under-strength suite on a PHI hop. **Unset is the default and changes nothing**: no `set_ciphers`, the inherited suite list, the six CBC-SHA2 suites still negotiable. One helper, `apply_connection_tls_ciphers`, narrows; the existing `harden_cipher_suites` call stays at each of the four seams on the next line and is NOT folded into it. Folding them was the first draft and the ASVS 12.1.2 call-site guard went red on all four, correctly: that guard reads every context builder for the assertion BY NAME, and it is the only instrument that can see a seam asserting nothing. The seams narrow first and assert last, on the post-`set_ciphers` context; the draft's claim that a drifted order would be UNSAFE is withdrawn in the ADR, since `validate_tls_ciphers` runs on the string independently of the context and is strictly stronger than the assertion. What the trailing assertion adds is a check on the real context shape -- the validator probes a server context, a client context could resolve the same string differently -- and the order is now checked per seam rather than structurally guaranteed. Config surface copies [ADR 0094](0094-granular-expiry-only-tls-relaxation.md): a recognised key of the free-form `settings` mapping, not a typed field, so `connections.toml` reaches it through the same factory with no second schema. **What this deliberately does NOT do:** extend the allow-list to inherited defaults, or retire the six CBC-SHA2 suites. That retention is the ruling `harden_cipher_suites` records -- the list governs what an operator may CONFIGURE, never what a default may contain -- and retiring the six stays gated on a peer census that does not exist. Also rejected: a narrower shipped default (measured, it also ADDS two DSS suites the default did not enable) and a typed model field | **Accepted (2026-09-14)** -- built with the change. **Amended 2026-09-23 (BACKLOG #300): the "unset changes nothing" boundary below is REVERSED.** Unset now narrows to the approved AEAD suites on all four seams, and that list is the default on every context the engine builds; the owner removed the six CBC-SHA2 suites from MLLP and DICOM with no peer census run, and a legacy CBC-only peer is served only by a reviewed change to `_APPROVED_TLS_SUITES`. AC-4 to AC-6 are superseded by the ADR's amendment. **Amended 2026-09-26 (BACKLOG #2042, owner ruling R4):** the three AES-128-GCM TLS 1.2 suites leave the approved list and the default, and TLS 1.3's `TLS_AES_128_GCM_SHA256` goes wherever the interpreter can remove it; on CPython 3.14 it stays, a recorded gap rather than an override. Six ACs, all six now in `tests/test_connection_tls_ciphers.py`, which the first draft cited without shipping. The load-bearing three are the unset-path set: a suite-list equality against the untouched reference construction for each context shape, a spy proving `set_ciphers` is called **not at all**, and the six CBC-SHA2 suites still negotiable. All three run per seam and are MUTATION-PROVEN -- making the unset path narrow reds all twelve cases -- and each is measured against the LOCAL OpenSSL rather than a fixed expectation, because the sub-128-bit and CBC-SHA2 populations are build properties and the two CI builds disagree. Scope: MLLP and DICOM only; the library-context hops (REST/SOAP/FHIR/LDAPS/FTPS/DICOMweb) are out of scope and `ldap3` in particular SWALLOWS an invalid `ciphers=` string. `DICOM()` stays code-first only -- it has no `connections.toml` transport name -- which is a pre-existing property of that connector, unchanged here | +| [0188](0188-per-connection-tls-ciphers-on-the-mllp-and-dicom-connectors.md) | **Per-connection `tls_ciphers` on the MLLP and DICOM connectors** -- the strict cipher allow-list `_APPROVED_TLS_SUITES` governed exactly ONE operator setting, `[api].tls_ciphers`, which configures the hop a browser uses to reach the web console. Every PARTNER-facing hop -- MLLP listener and destination, DICOM SCP listener and SCU destination -- had no cipher lever at all: each builds an `SSLContext`, inherits the interpreter's suite list, and `harden_cipher_suites` ASSERTS four properties on it (forward secrecy, encryption, peer authentication, a 128-bit floor) while deliberately not applying the list. So the knob existed for the engine's own console and not for the hops that carry PHI between organisations. Decision: an **opt-in** `tls_ciphers` on `MLLP()` and `DICOM()`, both directions on each, validated by `validate_tls_ciphers` itself -- the same function the `[api].tls_ciphers` field validator calls, allow-list included -- so an operator cannot put a NULL, anonymous, non-forward-secret or under-strength suite on a PHI hop. **Unset is the default and changes nothing**: no `set_ciphers`, the inherited suite list, the six CBC-SHA2 suites still negotiable. One helper, `apply_connection_tls_ciphers`, narrows; the existing `harden_cipher_suites` call stays at each of the four seams on the next line and is NOT folded into it. Folding them was the first draft and the ASVS 12.1.2 call-site guard went red on all four, correctly: that guard reads every context builder for the assertion BY NAME, and it is the only instrument that can see a seam asserting nothing. The seams narrow first and assert last, on the post-`set_ciphers` context; the draft's claim that a drifted order would be UNSAFE is withdrawn in the ADR, since `validate_tls_ciphers` runs on the string independently of the context and is strictly stronger than the assertion. What the trailing assertion adds is a check on the real context shape -- the validator probes a server context, a client context could resolve the same string differently -- and the order is now checked per seam rather than structurally guaranteed. Config surface copies [ADR 0094](0094-granular-expiry-only-tls-relaxation.md): a recognised key of the free-form `settings` mapping, not a typed field, so `connections.toml` reaches it through the same factory with no second schema. **What this deliberately does NOT do:** extend the allow-list to inherited defaults, or retire the six CBC-SHA2 suites. That retention is the ruling `harden_cipher_suites` records -- the list governs what an operator may CONFIGURE, never what a default may contain -- and retiring the six stays gated on a peer census that does not exist. Also rejected: a narrower shipped default (measured, it also ADDS two DSS suites the default did not enable) and a typed model field | **Accepted (2026-09-14)** -- built with the change. **Amended 2026-09-23 (BACKLOG #300): the "unset changes nothing" boundary below is REVERSED.** Unset now narrows to the approved AEAD suites on all four seams, and that list is the default on every context the engine builds; the owner removed the six CBC-SHA2 suites from MLLP and DICOM with no peer census run, and a legacy CBC-only peer is served only by a reviewed change to `_APPROVED_TLS_SUITES`. AC-4 to AC-6 are superseded by the ADR's amendment. **Amended 2026-09-26 (BACKLOG #2042, owner ruling R4):** the three AES-128-GCM TLS 1.2 suites leave the approved list and the default, and TLS 1.3's `TLS_AES_128_GCM_SHA256` goes wherever the interpreter can remove it; on CPython 3.14 it stays, a recorded gap rather than an override. **Amended 2026-09-30 (BACKLOG #2494, option 3):** LDAPS handshakes on a context the engine builds, through an engine subclass of `ldap3.Tls`, so it narrows TLS 1.3 where the interpreter allows it and no longer refuses to bind on Python 3.15. Six ACs, all six now in `tests/test_connection_tls_ciphers.py`, which the first draft cited without shipping. The load-bearing three are the unset-path set: a suite-list equality against the untouched reference construction for each context shape, a spy proving `set_ciphers` is called **not at all**, and the six CBC-SHA2 suites still negotiable. All three run per seam and are MUTATION-PROVEN -- making the unset path narrow reds all twelve cases -- and each is measured against the LOCAL OpenSSL rather than a fixed expectation, because the sub-128-bit and CBC-SHA2 populations are build properties and the two CI builds disagree. Scope: MLLP and DICOM only; the library-context hops (REST/SOAP/FHIR/LDAPS/FTPS/DICOMweb) are out of scope and `ldap3` in particular SWALLOWS an invalid `ciphers=` string. `DICOM()` stays code-first only -- it has no `connections.toml` transport name -- which is a pre-existing property of that connector, unchanged here | | [0189](0189-a-delivery-tier-log-halt-latch-read-at-the-claim-gate-rather-than-a-gate-at-every-door.md) | **A delivery-tier log-halt latch, read at the claim gate rather than a gate at every door** (BACKLOG #122) -- ADR 0162 makes the engine fail closed when it cannot write its application log, and CLAUDE.md section 1 says every message a connection puts out is counted and logged, so a halted engine must not deliver. The INBOUND tier enforces that with a LATCH -- `_log_halted`, teardown-surviving, read at each internal worker's loop top before the claim. **The DELIVERY tier had no state of its own:** the halt took a lane down by pausing it, which writes into `_outbound_paused`, the OPERATOR-pause set -- and `_teardown_body` CLEARS that set on purpose, and no consumer can tell a halt-pause from an operator pause because they are the same bit. So the rule could only be re-asserted at each DOOR into resuming delivery, and **four were found one at a time, each only after the previous fix shipped**: `start_outbound`, `restart_outbound`, a `stop()`+`start()` teardown, and a reload's `_unpark_outbound_lane`. Each fix was correct; the SHAPE was not -- four gates are a completeness claim nobody can verify, the SDS-3.6 liability. **Decision:** `_delivery_halted`, read at the CLAIM GATE in both claim modes -- `_delivery_worker`'s loop top (per_lane, above the pause gate and before the claim, so no row is left INFLIGHT) and `_dispatch_delivery` (pooled, the first runner-owned code a claimed row reaches, which `reschedule_claimed`s the head and parks the lane). Doors are unbounded; claim modes are two and closed. **DERIVED from `_log_write_stopped` rather than a second flag (SDS-3.5)** -- that fact is already stated once, both halt sites set it, only `_log_recovery_ok` clears it, and `_teardown_body` does not touch it, which is the teardown survival doors 3 and 4 needed. **The four door gates are KEPT as defence in depth** -- they fail fast and PAGE with the reason, which the claim gate cannot. **A `set[str]` of lane names was rejected on measurement, not taste:** `_stop_all_for_log_failure` pauses only the lanes NOT already paused, so a set built from it would omit exactly the engine-parked lane door 4 is about, and no set covers a lane BUILT AFTER the halt. Also rejected: `mark_failed` in the pooled half (spends a retry, eventually writes terminal DEAD on a row never sent -- ADR 0070's machinery-fault distinction), `release_claimed`+STOP (leaves the head past-due so the ~0.25 s sweep re-fires the gate ~4x/s, and `resume_lane` cannot re-arm a STOPPED lane), and filtering the pooled `lane_provider` alone (a partial gate that LOOKS total -- `notify_work` unions it with the known lanes and a producer wake arms the rest). **Operator-visible:** `outbound_status` gains `log_halted` ahead of running/stopping/stopped and `outbound_running` is False while it holds, so a halted lane stops reading as `stopped` -- which told an operator to press start when what the lane wanted was a writable disk -- and a lane an unguarded path brought up stops reading as `running`. `not_deployed`/`failed`/`filtered` still outrank it (per-connection facts fixed on that row), and `outbound_quiesced` still answers off the pause set so purge is unaffected | **Accepted (2026-09-15)** -- built with the change. The acceptance test uses NONE of the four doors: it drives `_start_outbound_unsafe` directly as a stand-in for the fifth door and asserts on BYTES (`list(outdir.iterdir())` over a real File connector draining a real store), with a repaired-disk control arm on the identical rig. **Measured RED on `main` in BOTH claim modes** -- `M1.hl7` reached the output directory while `guard.can_log()` read False throughout. Parametrized over `CLAIM_MODES = ["pooled", "per_lane"]`, both non-vacuous (the two modes reach the claim by different mechanisms and the gate lives in a different function in each) | | [0191](0191-coverage-guided-fuzzing-of-the-tolerant-parsers.md) | **Coverage-guided fuzzing of the tolerant parsers** (BACKLOG #277, Lane 1) -- adopts [Atheris](https://github.com/google/atheris), Google's libFuzzer binding for CPython, against the tolerant HL7 v2, X12 and DICOM parsers behind an ADVISORY ubuntu-only job. Measured at `origin/main` (`e290caef2`) 2026-09-22 with `git grep -lEi 'zap|schemathesis|atheris|boofuzz' -- .github pyproject.toml scripts`, no DAST or fuzzing of this kind was wired in that scope: the needle's **one** hit is a FALSE POSITIVE (`zAp` inside a base64 `integrity` hash in a vendored `package-lock.json`), it returns **zero** case-sensitively, and the `bandit|pip-audit|semgrep` control on the same instrument and scope returns **22** files (21 case-sensitive; 9 under `.github/workflows/`, 75 whole-tree), so the zero is a measurement and not a dead probe. **That probe is BACKLOG #277's OWN, and it proves only that Lane 1's four named instruments were unwired -- NOT that the Secure_Development_Standards section 6.1 *Dynamic* tier was empty, which is what this row and the ADR used to conclude and is FALSE.** DAST is built and shipped: [ADR 0155](0155-dast-dynamic-security-testing-of-the-running-engine.md) is `Accepted (2026-07-31) -- increment 1 built`, with `dast.yml` and `scripts/security/dast_*` on `origin/main`; `git grep -il dast` returns 42 files at the same ref, because DAST arrived under none of the four names in the needle. The two are **different work rather than two passes at one tier row** -- 0155 drives a running service over its authenticated HTTP API, this fuzzes a pure library in-process -- so this change adds no coverage to that row, and whoever re-scores WP-BL3-02 must not read it as newly covering a tier. See ADR 0155's own *Scope boundary* section for what it does and does not reach. **This row and the ADR previously said the control "fired across five files", which reproduces at no scope; the figure came from the dispatching brief and is corrected in place rather than dropped.** **The parsers are the right first slice as a property of the code, not a preference:** `parsing/` is a pure, side-effect-free library whose entire job is untrusted input, so it fuzzes in-process with no server, listener, store or network -- Lane 1's other three instruments (ZAP, Schemathesis, boofuzz) each need a live endpoint and are deliberately NOT wired or scaffolded for. **The contract fuzzed is one already written down:** each codec's error module states that a malformed body raises that codec's `ValueError` subclass, so a target lets every OTHER exception propagate and a propagating exception is libFuzzer's crash. **Targets parse AND THEN read the accessors** -- the eleven HL7 routing properties, the ten X12 ISA properties, the group/segment walks -- which is load-bearing rather than thorough: the one finding to date lives entirely in the accessor tier and fuzzing `parse` alone would have measured nothing. **It found a real defect:** a message carrying an empty segment parses, then every named routing property raises `IndexError`, because `_resolve_builtin` runs `raise_if_blank_segment_scan` OUTSIDE its own `except`, deliberately, to match the legacy python-hl7 path -- so the escaping exception is not a `ValueError` and the documented `except ValueError` dead-letter route does not catch it. Recorded in `KNOWN_FINDINGS`, not fixed: changing which exception the tolerant tier raises is a bug-compatibility semantics decision wider than this harness. **Fixed later under BACKLOG #1594**, which made the peek tolerant of an empty segment and removed the carve-out; see the ADR's 2026-09-26 amendment. **Atheris is confined to ONE file** (`fuzz/fuzz_parsers.py`) because it ships manylinux x86-64 wheels only; `fuzz/targets.py` imports none and runs on every leg, and the `[fuzz]` extra's `sys_platform`/`platform_machine` marker is what keeps it out of the Windows resolution despite `requirements.lock` being exported `--all-extras`. A NEW EXTRA rather than a fourth `ci/locks/*.lock` (the eighth DEP-1 export; `ci/locks/` holds three files), so the blocking DEP-1 gate needed no edit -- but note the extra rides `requirements.lock`, which is exported `--all-extras` and audited by `pip-audit` inside the REQUIRED `dependency-and-secret-scan` context, so a CVE in Atheris can red a required check even though the fuzz job gates nothing; accepted on the same reasoning `security.yml` already states for the toolchain locks, with `--ignore-vuln` as the escape hatch. **Corpus lives OUTSIDE the work tree** (`$TMPDIR`, or `MEFOR_FUZZ_WORK_DIR`) -- a control, not a convenience: every file the fuzzer writes is message-shaped and a corpus grows without bound, so `git add -A` must not reach it, which is stronger than a `.gitignore` pattern. **That held for the default and not for the override until `work_root()` was fenced:** an override was taken unexamined, so a relative one -- including the literal `~/mefor-fuzz` the README handed out, since tilde expansion belongs to the shell and not to the value -- resolved inside the repository and staged fuzzer-minimised message bodies. It now expands `~` itself and REFUSES anything resolving under the repository root, exiting a distinct code the workflow reports as a harness fault rather than a parser finding. Rejected: Hypothesis (a fine tool for the wrong question -- it explores properties an author states and does not steer by coverage; worth adding later for round-trip/idempotence), `python-afl`/pythonfuzz (no cp314 release), a hand-extended adversarial corpus (the BACKLOG #89 shape, which already exists and cannot generate an input nobody thought of -- **#89 is NOT fuzzing and must not be counted as it**), a blocking job, and CI crash-artifact upload | **Accepted (2026-09-22)** -- built with the change. **Advisory three independent ways:** not in `.github/required-contexts.txt` (the required set is untouched -- no count given, per CLAUDE.md section 5), step-level `continue-on-error`, and `parser fuzzing (advisory)` added to `_MUST_NOT_BE_REQUIRED` so promoting it reds `tests/test_required_contexts.py`. No job-level `continue-on-error` -- GitHub reports that as SUCCESS, a job that cannot fail. **The third way was the weak one:** `_MUST_NOT_BE_REQUIRED` is free text never resolved against a real job, so renaming the job's `name:` left it guarding a dead string -- the only one of ten workflow mutations no test caught. `tests/test_security_posture.py` now pins the job's `name:`, its absence of a job-level flag, and its single softened step; the ADR previously said that module already refused the idiom, which was false when written. **Demonstrated attributably**, 20,000 iterations at a fixed seed with a random mutator standing in for libFuzzer (Atheris does not install on the Windows authoring box): `hl7_peek` rediscovered the registered finding after **13** inputs with the carve-out disabled and was **clean over 20,000** with it enabled; `x12_peek` caught a planted bounds-check removal in `_element` after **21** inputs and was **clean over 20,000** once reverted. Both control arms are load-bearing -- without them the catches are unattributable. **Closes the engine half of [ADR 0034](0034-static-analysis-triage-policy-accepted-risk-register.md)'s accepted risk for Scorecard's `Fuzzing` check (WP-BL3-02) and CANNOT close the record half:** the ASVS cell lives in the vault clone, so nothing here changes an ASVS verdict and no prose should say one changed. BACKLOG #277 stays OPEN -- three of Lane 1's four instruments are still unwired | | [0192](0192-server-db-schema-is-provisioned-externally-by-default-the-runtime-login-runs-no-ddl.md) | **Server-DB schema is provisioned externally by default; the runtime login runs no schema DDL** (BACKLOG #305, ASVS 13.2.2) -- `[store].schema_management` takes `auto` or `external`, and `external` is the default on SQL Server and PostgreSQL (SQLite is always `auto`). Under `external` the store open reads the `schema_meta` marker through the existing `_schema_marker_current` and raises `SchemaNotProvisionedError`, naming the fix, when it does not match; it runs no schema DDL and, on SQL Server, no `ALTER DATABASE`. New `messagefoundry store provision-schema` runs the same batch as whoever runs it, with no store key. The #1008 privilege probe counts `db_ddladmin` (SQL Server) and `CREATE` on, or ownership of objects in, the store schema (PostgreSQL) as excess under `external` only, and `auto` on a server DB is a named loosening. Rejected: `auto` by default (13.2.2 stays failed at the default), and the old drop-and-re-grant `db_ddladmin` runbook advice (one login, timing on memory). Accepted cost: a fresh install or schema-moving upgrade fails `serve` until the provisioning step runs | **Accepted (2026-09-23)** -- built with the change. CI server-DB legs opt into `auto` because their suites rebuild tables as the test login; live legs exercise `external` against a scratch database and schema. The ASVS re-score is vault work and is not claimed here | diff --git a/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index 7ce07d724..c7139972c 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -23,7 +23,7 @@ import uuid from dataclasses import dataclass from enum import Enum -from typing import Any, NamedTuple +from typing import TYPE_CHECKING, Any, NamedTuple from messagefoundry.auth.trust_anchors import ad_anchor_spec, verified_anchor_cadata from messagefoundry.config.secretprovider import SecretProvider, resolve_connector_secret @@ -33,11 +33,10 @@ split_kerberos_spn, weakened_tls_escape_permitted, ) -from messagefoundry.config.tls_policy import ( - APPROVED_TLS12_SUITES, - HopPosture, - assert_ldap3_tls_suites, -) +from messagefoundry.config.tls_policy import HopPosture + +if TYPE_CHECKING: + from messagefoundry.auth.ldap_tls import NarrowedTls logger = logging.getLogger(__name__) @@ -447,53 +446,35 @@ def __init__( "production.", INSECURE_TLS_ESCAPE_ENV, ) - # BACKLOG #1317. Assert the suite list this hop will negotiate (ASVS 12.1.2). Verification-off - # is refused/warned above; this is the separate question of whether the traffic is ENCRYPTED and - # the peer AUTHENTICATED at all, which was inherited from the interpreter default and unchecked. - # Measured: the inherited list is clean today, so this raises on no supported configuration -- - # it converts an inherited property into a checked one, the same move harden_cipher_suites - # documents. Done ONCE here rather than in _server() because the answer is fixed by config and - # _server() runs up to three times per login; AuthService builds this eagerly, so a bad suite - # list fails app startup rather than the first bind. + # BACKLOG #1317, #2494. The engine builds this hop's TLS context (ASVS 12.1.2): narrowed to the + # approved suites at TLS 1.2 and, where the interpreter allows it, TLS 1.3, then asserted. + # Verification-off is refused/warned above; this is the separate question of whether the + # traffic is ENCRYPTED and the peer AUTHENTICATED at all. NarrowedTls builds and asserts that + # context once here, so a bad one fails app startup (AuthService builds this eagerly), and + # again for every connection. One Tls serves every Server: ldap3.Tls holds only settings. + # The CA goes in as the bytes checked above, never a path (BACKLOG #2034); None loads the OS + # trust store, as ldap3 did. The accepted risk of owner ruling 2026-09-27 stands: an older + # domain controller that offers none of the approved suites fails to bind. + self._tls: NarrowedTls | None = None if self._ldaps: - assert_ldap3_tls_suites(self._tls_kwargs(), connector=_LDAPS_CONNECTOR) - - def _tls_kwargs(self) -> dict[str, Any]: - """The ``ldap3.Tls`` arguments for this bind — the SINGLE definition, read by both consumers. - - ``__init__`` asserts the suite list these resolve to and ``_server()`` builds the real ``Tls`` - from them, so the two cannot drift onto different shapes. Keep it that way: the assertion runs - against a REBUILT context (``ldap3.Tls`` holds no ``SSLContext`` to check directly), and a - rebuilt context is only evidence about this hop while it is built from the hop's own arguments. + from messagefoundry.auth.ldap_tls import NarrowedTls # lazy: it imports ldap3 - The CA goes in as ``ca_certs_data``, the bytes ``__init__`` checked, and never as - ``ca_certs_file`` (BACKLOG #2034). ``None`` means no CA is configured, and ldap3 then loads - the OS trust store, as it did before. - - ``ciphers`` narrows the TLS 1.2 suites to the approved AEAD list (BACKLOG #300, owner ruling - 2026-09-27), the same list every engine-built context offers. The assertion refuses any other - value, because ldap3 silently swallows a string OpenSSL rejects. The accepted risk, named in - the ruling: an older domain controller that offers none of these suites fails to bind. - """ - return { - "validate": ssl.CERT_REQUIRED if self._s.ad_tls_verify else ssl.CERT_NONE, - "ca_certs_data": self._ca_certs_data, - "ciphers": ":".join(APPROVED_TLS12_SUITES), - } + self._tls = NarrowedTls( + validate=ssl.CERT_REQUIRED if settings.ad_tls_verify else ssl.CERT_NONE, + ca_certs_data=self._ca_certs_data, + connector=_LDAPS_CONNECTOR, + ) def _server(self) -> Any: import ldap3 - tls = None - if self._ldaps: - tls = ldap3.Tls(**self._tls_kwargs()) # ASVS 13.1.3: ldap3's Server.connect_timeout defaults to None (wait forever) and the engine # never sets a process-wide socket default, so this is the ONLY bound on the TCP connect to the # domain controller. Every Server in this module is built here, so threading it here covers the # service-account bind AND the user bind. return ldap3.Server( self._s.ad_server, - tls=tls, + tls=self._tls, get_info=ldap3.NONE, connect_timeout=self._s.ad_connect_timeout, ) diff --git a/messagefoundry/auth/ldap_tls.py b/messagefoundry/auth/ldap_tls.py new file mode 100644 index 000000000..b61ae0aa1 --- /dev/null +++ b/messagefoundry/auth/ldap_tls.py @@ -0,0 +1,64 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Foundation, LLC and contributors +"""The engine's ``ldap3.Tls``: LDAPS wraps its socket with a context the engine built (BACKLOG #2494). + +ldap3 2.9.1 builds its TLS context inside ``Tls.wrap_socket`` and offers no parameter to supply one. +Its only suite lever, ``ciphers=``, reaches TLS 1.2 and not TLS 1.3, so on Python 3.15 the LDAPS hop +could not drop ``TLS_AES_128_GCM_SHA256`` and the engine refused to bind. :class:`NarrowedTls` +replaces the one method that builds the context. The context comes from +:func:`messagefoundry.config.tls_policy.assert_ldap3_tls_suites`, whose docstring says what it holds. + +**What is copied from ldap3, and why.** The wrap step below is ldap3's own, for the arguments the +engine passes: wrap the socket client-side, then, on a handshake with verification on, run ldap3's +host name check. That check is ldap3's function, called, not copied. The alternative was to patch +``ldap3.core.tls.create_default_context`` for the length of one call. That is a module global, and +``AuthService`` runs binds on worker threads, so two concurrent binds could each see the other's +patch. ``tests/test_ldap_tls.py`` pins the source of ldap3's ``wrap_socket``, so an ldap3 that +changes it goes red rather than drifting from this copy. + +Imported lazily by :mod:`messagefoundry.auth.ldap`, like ldap3 itself, so a local-only deployment +never loads it. +""" + +from __future__ import annotations + +import ssl +from typing import Any + +import ldap3 +from ldap3.core.tls import check_hostname + +from messagefoundry.config.tls_policy import assert_ldap3_tls_suites + +__all__ = ["NarrowedTls"] + + +class NarrowedTls(ldap3.Tls): # type: ignore[misc] # ldap3 ships no type information + """An ``ldap3.Tls`` whose ``wrap_socket`` uses a context the engine built and asserted. + + The constructor builds the context factory from ``validate`` and ``ca_certs_data`` and runs it + once, so a bad context fails here. It hands the same two values to ``ldap3.Tls``, so what ldap3 + reads off this object stays true: ``validate`` decides its host name check, and a followed + referral copies it. It takes no other ldap3 argument, because the engine's context would not + carry one. ``ciphers=`` in particular reached TLS 1.2 only. + """ + + def __init__( + self, *, validate: ssl.VerifyMode, ca_certs_data: str | None, connector: str + ) -> None: + self._context_factory = assert_ldap3_tls_suites( + validate=validate, ca_certs_data=ca_certs_data, connector=connector + ) + super().__init__(validate=validate, ca_certs_data=ca_certs_data) + + def wrap_socket(self, connection: Any, do_handshake: bool = False) -> None: + """ldap3's wrap step on a fresh engine context; ldap3 calls this for LDAPS and StartTLS.""" + wrapped = self._context_factory().wrap_socket( + connection.socket, + server_side=False, + do_handshake_on_connect=do_handshake, + server_hostname=self.sni or None, + ) + if do_handshake and self.validate in (ssl.CERT_REQUIRED, ssl.CERT_OPTIONAL): + check_hostname(wrapped, connection.server.host, self.valid_names) + connection.socket = wrapped diff --git a/messagefoundry/config/tls_policy.py b/messagefoundry/config/tls_policy.py index 28fadc4a4..45af9025e 100644 --- a/messagefoundry/config/tls_policy.py +++ b/messagefoundry/config/tls_policy.py @@ -413,9 +413,9 @@ def context_checks_revocation(ctx: ssl.SSLContext | None) -> bool: Reads ``VERIFY_CRL_CHECK_LEAF`` off the context itself rather than trusting that some setting was configured somewhere. That distinction is the whole point: ``[tls].crl_file`` is instance-wide, but - the hops it reaches are not — an ``ldap3.Tls`` or a ``truststore`` context is built by a library that - never sees the policy, so a setting-shaped test would report "revocation is checked" for a handshake - that checks nothing. Asking the object that performs the handshake cannot make that mistake. + the hops it reaches are not — the LDAPS context loads no CRL, and a ``truststore`` context is + built by a library that never sees the policy, so a setting-shaped test would report "revocation + is checked" for a handshake that checks nothing. Asking the object that performs the handshake cannot make that mistake. ``None`` (no context — a hop that is not TLS, or a caller that has none to hand) is ``False``: absent evidence is not evidence of checking. @@ -1168,9 +1168,9 @@ def narrow_signature_algorithms(ctx: ssl.SSLContext) -> bool: so the failure is never blamed on an operator's ``tls_ciphers``. **Reach.** Every engine-built context that narrows its suites, through - :func:`narrow_to_approved_suites` or :func:`apply_operator_tls_ciphers`. At least these are not - reached: the LDAPS hop, whose context ldap3 builds from arguments that carry no signature list, - and the contexts outside this module (``apiclient``, ``tray``, ``tls_probe``). + :func:`narrow_to_approved_suites` or :func:`apply_operator_tls_ciphers`, the LDAPS hop included + since BACKLOG #2494 (:func:`assert_ldap3_tls_suites`). At least these are not reached: the + contexts outside this module (``apiclient``, ``tray``, ``tls_probe``). A ``truststore.SSLContext`` is narrowed through its inner context, as in :func:`narrow_tls13_suites` and for the same reason.""" @@ -1373,120 +1373,85 @@ def urllib_handler_context( return ctx -#: The ``ldap3.Tls`` keyword arguments :func:`assert_ldap3_tls_suites` can faithfully replicate. -#: -#: These are the three the engine passes, and each is measured: ``validate`` becomes ``verify_mode`` -#: (no effect on the suite list, replicated anyway so the probe mirrors ldap3's construction rather -#: than approximating it), ``ca_certs_data`` changes the trust anchors and not one entry of the -#: negotiable suite list, and ``ciphers`` is the suite list itself. Every OTHER ``Tls`` argument is -#: REFUSED rather than ignored. -#: -#: ``ca_certs_file`` left this set in BACKLOG #2034. The bind now hands ldap3 the checked bytes rather -#: than the path, so a path here would mean the bind reads the anchor again, after its check. Refusing -#: it turns that regression into a construction-time error. -#: -#: ``ciphers`` joined in BACKLOG #300 (owner ruling 2026-09-27: narrow the library-built contexts -#: too). It is admitted for ONE value only, the approved TLS 1.2 list joined by ``:``, and it is -#: REQUIRED. The function's docstring says why any other value must still be refused. -_LDAP3_TLS_REPLICABLE_KWARGS = frozenset({"validate", "ca_certs_data", "ciphers"}) - - -def assert_ldap3_tls_suites(tls_kwargs: Mapping[str, object], *, connector: str) -> None: - """Assert the suite list of the context ``ldap3`` will build from these ``Tls`` arguments. - - The LDAPS sibling of :func:`build_asserted_https_handler`, and the one site where that function's - method — hold the library's OWN context and check it — is **not available**. Measured: an - ``ldap3.Tls`` carries zero ``SSLContext`` attributes. It stores the arguments and builds the context - inside ``Tls.wrap_socket`` at connect time, from ``create_default_context(Purpose.SERVER_AUTH, - cafile=...)`` followed by ``check_hostname = False`` and ``verify_mode = validate``; there is no - ``ssl_context=`` parameter to inject one through. So the engine can never hold the object this hop - will use, and the identity check the urllib openers get in - ``tests/test_tls_cipher_assertion_sites.py`` is impossible here. - - This therefore rebuilds ldap3's construction and asserts THAT. It is a weaker guarantee than - identity, and it is only honest because the gap is closed by measurement rather than by argument: - ``test_the_ldaps_replica_matches_the_context_ldap3_actually_builds`` drives ldap3's **real** - ``wrap_socket`` over a socketpair, captures the ``SSLContext`` off the resulting ``SSLSocket``, and - requires its suite list to equal this one's. If ldap3 changes how it builds that context, the - replica stops matching and that test goes red. - - **It REFUSES any ``Tls`` argument it cannot replicate, and that refusal is load-bearing.** An - argument outside :data:`_LDAP3_TLS_REPLICABLE_KWARGS` raises here rather than being replicated - wrongly or passed over in silence. - - **``ciphers`` is REQUIRED, and only one value passes (BACKLOG #300).** The owner ruled on - 2026-09-27 that the library-built contexts are narrowed to the approved list too, and ldap3 takes - a suite list only as this string. Any other value is a trap: ``ldap3/core/tls.py`` wraps - ``set_ciphers`` in ``except ssl.SSLError: pass``, so a string OpenSSL rejects is **swallowed - silently**. Measured, the hop then drops to 3 TLS 1.3 suites with the TLS 1.2 list emptied, and - nothing reports it. So the value must equal :data:`APPROVED_TLS12_SUITES` joined by ``:``, which - carries no ``@`` directive, and the replica applies it with a ``set_ciphers`` that does NOT - swallow. A string OpenSSL rejects fails here, at construction, and never reaches ldap3. A missing - ``ciphers`` is refused too, so the hop cannot fall back to the interpreter's wider list while this - check still passes. - - The replica is then held to the approved list itself, not only to the four properties - :func:`harden_cipher_suites` checks: it must offer at least one TLS 1.2 suite, and every suite - must be in :data:`_APPROVED_TLS_SUITES`. Not the whole list: ``set_ciphers`` drops a name the - OpenSSL build lacks, such as ChaCha20 on a FIPS build, and every engine seam allows that. ldap3 - cannot narrow TLS 1.3, and - on CPython 3.14 nothing can (:func:`narrow_tls13_suites` says why). On an interpreter where the - engine CAN drop ``TLS_AES_128_GCM_SHA256``, the allow-list drops it and this refuses LDAPS, which - fails closed; ``tests/test_tls_default_suites.py`` goes red that day so this is re-derived. - - ``ca_certs_data`` is accepted and deliberately **not loaded**: it changes the trust anchors and not - one entry of the suite list (measured). ``ca_certs_file`` is refused, because the bind loads the - checked bytes and never the path (BACKLOG #2034). +def _narrow_library_context(ctx: ssl.SSLContext, *, connector: str, hop: str) -> None: + """:func:`narrow_to_approved_suites` for a library-built hop, a refusal re-raised as ValueError. - Raises :class:`ValueError` at construction, like every other assertion site. - """ - unreplicable = sorted(set(tls_kwargs) - _LDAP3_TLS_REPLICABLE_KWARGS) - if unreplicable: - raise ValueError( - f"{connector}: cannot assert this hop's TLS suites — ldap3.Tls argument(s) " - f"{', '.join(unreplicable)} change the context ldap3 builds in a way this check does not " - f"replicate, so it would be asserting a context the hop will not use." - ) - validate = tls_kwargs.get("validate") - if not isinstance(validate, int): - raise ValueError( - f"{connector}: cannot assert this hop's TLS suites — no `validate` given, so the peer " - f"verification mode ldap3 will apply is unknown. Refusing rather than guessing it." - ) - approved = ":".join(APPROVED_TLS12_SUITES) - if tls_kwargs.get("ciphers") != approved: - raise ValueError( - f"{connector}: ldap3.Tls must be given `ciphers` equal to the approved TLS 1.2 list " - f"({approved}) and nothing else (BACKLOG #300). A missing value leaves the interpreter's " - f"wider list in place, and ldap3 SWALLOWS a string OpenSSL rejects (except " - f"ssl.SSLError: pass), which silently strips every TLS 1.2 suite." - ) - ctx = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH) - # Order is ldap3's, and it matters: check_hostname must go False BEFORE verify_mode, or setting - # CERT_NONE on a hostname-checking context raises. (ldap3 runs its own hostname check after the - # handshake instead — `check_hostname(...)` at the end of Tls.wrap_socket.) ldap3 applies the - # cipher string last, as here, and without an @SECLEVEL prefix, which keeps the level. - ctx.check_hostname = False - ctx.verify_mode = ssl.VerifyMode(validate) + The ldap3 and hvac factories share it. Each still calls :func:`harden_cipher_suites` itself + after it, so the call-site guards see the assertion at each seam by name.""" try: - ctx.set_ciphers(approved) - except ssl.SSLError as exc: + narrow_to_approved_suites(ctx) # TLS 1.2, TLS 1.3 and sigalgs (BACKLOG #300, #2494) + except (ssl.SSLError, RuntimeError) as exc: raise ValueError( - f"{connector}: this OpenSSL build selects none of the approved TLS 1.2 suites " - f"({approved}), so ldap3 would silently offer no TLS 1.2 suite (BACKLOG #300)." + f"{connector}: this OpenSSL build refuses the approved suite list, so {hop} cannot " + f"be narrowed (BACKLOG #300): {exc}" ) from exc - harden_cipher_suites(ctx, connector=connector) + + +def _hold_to_approved_list(ctx: ssl.SSLContext, *, connector: str, hop: str) -> None: + """Refuse ``ctx`` unless it offers a TLS 1.2 suite and every suite is on the approved list. + + Not the whole list: ``set_ciphers`` drops a name the OpenSSL build lacks, such as ChaCha20 on a + FIPS build, and every engine seam allows that. The library-built hops run this after + :func:`harden_cipher_suites`, which checks four properties and not the list (owner ruling + 2026-09-27, BACKLOG #300).""" ciphers = ctx.get_ciphers() tls12 = [c for c in ciphers if c.get("protocol") != "TLSv1.3"] unlisted = sorted({str(c.get("name", "?")) for c in ciphers} - _APPROVED_TLS_SUITES) if not tls12 or unlisted: raise ValueError( - f"{connector}: the TLS context ldap3 will build is not narrowed to the approved list " - f"(ASVS 12.1.2, BACKLOG #300). TLS 1.2 suites: {len(tls12)}. Unlisted: " - f"{', '.join(unlisted) or 'none'}." + f"{connector}: {hop} is not narrowed to the approved list (ASVS 12.1.2, BACKLOG " + f"#300). TLS 1.2 suites: {len(tls12)}. Unlisted: {', '.join(unlisted) or 'none'}." ) +def assert_ldap3_tls_suites( + *, validate: ssl.VerifyMode, ca_certs_data: str | None, connector: str +) -> Callable[[], ssl.SSLContext]: + """Build and assert the LDAPS hop's TLS context; return the factory that builds it. + + The LDAPS sibling of :func:`assert_hvac_tls_suites`, and the same shape since BACKLOG #2494. + ldap3 2.9.1 builds its own context inside ``Tls.wrap_socket`` and offers no way in, and + ``ciphers=`` was its only suite lever. That lever reaches TLS 1.2 only, so on an interpreter + that can drop ``TLS_AES_128_GCM_SHA256`` (owner ruling R4, :func:`narrow_tls13_suites`) the old + assertion refused LDAPS. A first deployment on Python 3.15 would have lost directory sign-in. + + **So the engine builds the context, and ldap3 only wraps the socket with it.** + ``messagefoundry.auth.ldap_tls.NarrowedTls`` is the engine's ``ldap3.Tls``. Its ``wrap_socket`` + calls the factory this returns, once per connection, as ldap3 built one per connection. The + factory keeps what ldap3 did, in ldap3's order: + + * ``ssl.create_default_context(Purpose.SERVER_AUTH, cadata=...)``, the call ldap3 makes, so CA + loading is the same: the checked bytes when a CA is configured, else the OS trust store. The + bytes, never a path (BACKLOG #2034), so no bind reads the anchor again after its check; + * ``check_hostname = False`` before ``verify_mode = validate``, because ldap3 checks the host + name itself after the handshake, and ``NarrowedTls`` still calls ldap3's own check. + + It then adds the engine posture every other seam carries: a TLS 1.2 floor, strict X.509 flags + when it verifies, the approved key-exchange groups (:func:`harden_kex_groups`), and + :func:`narrow_to_approved_suites`, which sets the TLS 1.2 suites, the TLS 1.3 suites and the + signature schemes wherever the interpreter allows each. Then :func:`harden_cipher_suites` + asserts it, and :func:`_hold_to_approved_list` holds it to the approved list. This runs the + factory once before returning, so a narrowing that stopped working fails at construction. + Raises :class:`ValueError`, like every other assertion site. + """ + + def narrowed_context() -> ssl.SSLContext: + ctx = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH, cadata=ca_certs_data) + ctx.check_hostname = False # ldap3's order: before CERT_NONE, or setting it raises + ctx.verify_mode = validate + ctx.minimum_version = ssl.TLSVersion.TLSv1_2 + if validate != ssl.CERT_NONE: + harden_verify_flags(ctx) + harden_kex_groups(ctx) # pin approved ECDHE groups where supported (ASVS 11.6.2) + _narrow_library_context(ctx, connector=connector, hop="the LDAPS TLS context") + harden_cipher_suites(ctx, connector=connector) + _hold_to_approved_list(ctx, connector=connector, hop="the LDAPS TLS context") + return ctx + + narrowed_context() # fail at construction, not on the first bind + return narrowed_context + + #: The ``hvac.Client`` keyword arguments :func:`assert_hvac_tls_suites` admits UNCONDITIONALLY. #: #: Not one of them reaches the TLS context: ``url`` becomes the request URL, ``token`` an @@ -1631,23 +1596,9 @@ def assert_hvac_tls_suites( def narrowed_context() -> ssl.SSLContext: ctx = create_urllib3_context() - try: - narrow_to_approved_suites(ctx) # approved AEAD default (BACKLOG #300) - except (ssl.SSLError, RuntimeError) as exc: - raise ValueError( - f"{connector}: this OpenSSL build refuses the approved suite list, so the Vault " - f"TLS context cannot be narrowed (BACKLOG #300): {exc}" - ) from exc + _narrow_library_context(ctx, connector=connector, hop="the Vault TLS context") harden_cipher_suites(ctx, connector=connector) - ciphers = ctx.get_ciphers() - tls12 = [c for c in ciphers if c.get("protocol") != "TLSv1.3"] - unlisted = sorted({str(c.get("name", "?")) for c in ciphers} - _APPROVED_TLS_SUITES) - if not tls12 or unlisted: - raise ValueError( - f"{connector}: the Vault TLS context is not narrowed to the approved list (ASVS " - f"12.1.2, BACKLOG #300). TLS 1.2 suites: {len(tls12)}. Unlisted: " - f"{', '.join(unlisted) or 'none'}." - ) + _hold_to_approved_list(ctx, connector=connector, hop="the Vault TLS context") return ctx narrowed_context() # fail at construction, not on the first connection diff --git a/scripts/security/crypto_inventory_check.py b/scripts/security/crypto_inventory_check.py index 9aa00639f..cb8745b29 100644 --- a/scripts/security/crypto_inventory_check.py +++ b/scripts/security/crypto_inventory_check.py @@ -301,6 +301,9 @@ # used for one handshake, and never returned. Not a data path — do not reuse these settings. "messagefoundry/config/tls_probe.py": frozenset({"ssl"}), "messagefoundry/auth/ldap.py": frozenset({"messagefoundry.config.tls_policy", "ssl"}), + # BACKLOG #2494: the engine ldap3.Tls. It wraps each LDAPS socket with a context that + # tls_policy.assert_ldap3_tls_suites built, narrowed and asserted. + "messagefoundry/auth/ldap_tls.py": frozenset({"messagefoundry.config.tls_policy", "ssl"}), # ADR 0142 (OIDC relying party, BACKLOG #274): the federated-SSO layer. # claims.py — hmac.compare_digest for the constant-time nonce comparison; cryptography only for # catching InvalidSignature (the verification itself is transports/signing.py, inventoried). @@ -918,9 +921,13 @@ { "hash:via messagefoundry.auth.trust_anchors", "tls_context:via messagefoundry.auth.trust_anchors", - "tls_context:via messagefoundry.config.tls_policy", } ), + # BACKLOG #2494: the LDAPS context is built by tls_policy.assert_ldap3_tls_suites, and the socket + # is wrapped with it here, in place of ldap3's own wrap_socket. + "messagefoundry/auth/ldap_tls.py": frozenset( + {"tls_context:.wrap_socket()", "tls_context:via messagefoundry.config.tls_policy"} + ), "messagefoundry/auth/oidc/claims.py": frozenset( {"compare:hmac.compare_digest", "sign_verify:via messagefoundry.transports.signing"} ), diff --git a/tests/test_key_lifecycle_coverage.py b/tests/test_key_lifecycle_coverage.py index 684a8e2dc..9f43bfbd4 100644 --- a/tests/test_key_lifecycle_coverage.py +++ b/tests/test_key_lifecycle_coverage.py @@ -194,6 +194,7 @@ "messagefoundry/auth/trust_anchors.py": _KEYLESS, "messagefoundry/auth/webauthn.py": _EPHEMERAL + "; the credentials it verifies are PUBLIC keys", "messagefoundry/auth/ldap.py": _VERIFY_ONLY, + "messagefoundry/auth/ldap_tls.py": _VERIFY_ONLY, "messagefoundry/auth/oidc/claims.py": _VERIFY_ONLY, "messagefoundry/auth/oidc/jwks.py": _VERIFY_ONLY, "messagefoundry/auth/oidc/flow.py": _EPHEMERAL + " (PKCE verifier, state, nonce). The OIDC " diff --git a/tests/test_ldap_tls.py b/tests/test_ldap_tls.py new file mode 100644 index 000000000..41b777cb9 --- /dev/null +++ b/tests/test_ldap_tls.py @@ -0,0 +1,168 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Foundation, LLC and contributors +"""BACKLOG #2494: LDAPS wraps its socket with a context the engine built, and still gets ldap3's checks. + +``messagefoundry.auth.ldap_tls.NarrowedTls`` replaces ldap3's ``Tls.wrap_socket`` so the engine can +narrow TLS 1.3 too. These tests drive the REAL override over a connected socket pair against a TLS +server thread, so each claim is a handshake rather than an attribute read: + +* the anchored CA verifies, a foreign CA is refused, and ldap3's own host name check still runs + after the handshake (and still does not run with verification off, as in ldap3); +* the copy of ldap3's wrap step is pinned to the ldap3 source it was taken from; +* on an interpreter with ``SSLContext.set_ciphersuites`` (CPython 3.15) the hop refuses a TLS 1.3 + peer offering only ``TLS_AES_128_GCM_SHA256``, while ldap3's own ``Tls`` accepts it as the + control, and the bind constructs instead of refusing. On 3.14 the paired test measures the + recorded gap. + +The per-hop CBC, AES-128 and suite-order handshakes live with every other hop, in +``tests/test_tls_default_suites.py``. +""" + +from __future__ import annotations + +import contextlib +import hashlib +import inspect +import socket +import ssl +import threading +from pathlib import Path +from types import SimpleNamespace +from typing import Any, cast + +import ldap3 +import pytest +from ldap3.core.exceptions import LDAPCertificateError + +from messagefoundry.auth import ldap as ldap_auth +from messagefoundry.auth.ldap_tls import NarrowedTls +from messagefoundry.config import tls_policy +from tests.test_tls_cipher_assertion_sites import _ad_settings, _self_signed +from tests.test_tls_default_suites import _Pki, _tls13 +from tests.test_tls_default_suites import pki as pki # noqa: F401 (the fixture, re-exported) + +#: sha256 of ``inspect.getsource(ldap3.Tls.wrap_socket)`` in ldap3 2.9.1, line endings as ``\n``. +_LDAP3_WRAP_SOCKET_SHA256 = "428733b595fea962ca8d0132986fef7aea2df98031bb0efeac450f2506d4c0cd" + +_TLS13_AES128 = tls_policy._TLS13_AES128_SUITE +_has_set_ciphersuites = hasattr(ssl.SSLContext, "set_ciphersuites") + + +def _tls(ca_pem: str, validate: ssl.VerifyMode = ssl.CERT_REQUIRED) -> NarrowedTls: + """The engine's ``Tls`` for a bind anchored at ``ca_pem``, built as ``LdapAuthenticator`` does.""" + return NarrowedTls(validate=validate, ca_certs_data=ca_pem, connector="LDAPS (test)") + + +def _server(pki: _Pki, *, tls13_only_aes128: bool = False) -> ssl.SSLContext: + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + ctx.load_cert_chain(pki.cert, pki.key) + if tls13_only_aes128: + ctx.minimum_version = ssl.TLSVersion.TLSv1_3 + cast(Any, ctx).set_ciphersuites(_TLS13_AES128) # a 3.15 method; typeshed gates it + return ctx + + +def _bind(tls: Any, server: ssl.SSLContext, host: str = "localhost") -> tuple[str, str]: + """Run ``tls.wrap_socket`` with a handshake, as ldap3 does for LDAPS, against ``server`` on the + other end of a socket pair. Returns ``(suite, protocol)``; raises what the client raised.""" + left, right = socket.socketpair() + left.settimeout(10) + right.settimeout(10) + release = threading.Event() + + def serve() -> None: + with contextlib.suppress(ssl.SSLError, OSError): + wrapped = server.wrap_socket(right, server_side=True) + release.wait(10) + wrapped.close() + + thread = threading.Thread(target=serve, daemon=True) + thread.start() + conn = SimpleNamespace(socket=left, server=SimpleNamespace(host=host)) + try: + tls.wrap_socket(conn, do_handshake=True) + cipher = conn.socket.cipher() + assert cipher is not None + return str(cipher[0]), str(cipher[1]) + finally: + release.set() + conn.socket.close() + left.close() + right.close() + thread.join(10) + + +def test_ldap3_wrap_socket_is_the_one_narrowed_tls_copies() -> None: + """``NarrowedTls.wrap_socket`` copies ldap3's wrap step. If ldap3 changes that method, this goes + red: read the new source, carry over what changed, then update the hash.""" + source = inspect.getsource(ldap3.Tls.wrap_socket).replace("\r\n", "\n") + assert hashlib.sha256(source.encode()).hexdigest() == _LDAP3_WRAP_SOCKET_SHA256, ( + f"ldap3 {ldap3.__version__} changed Tls.wrap_socket; re-derive messagefoundry/auth/" + f"ldap_tls.py against it before updating the pinned hash" + ) + + +def test_a_verifying_bind_completes_against_a_dc_the_anchor_signed(pki: _Pki) -> None: + """The positive control for every refusal below, and the suite is an approved one.""" + suite, _protocol = _bind(_tls(Path(pki.ca).read_text()), _server(pki)) + assert suite in tls_policy._APPROVED_TLS_SUITES + + +def test_ldap3s_host_name_check_still_runs_after_the_handshake(pki: _Pki) -> None: + """The engine's context leaves ``check_hostname`` off, as ldap3's does, because ldap3 checks the + name itself after the handshake. The override must still call that check. Same server as the + control above; only the name ldap3 checks against differs.""" + with pytest.raises(LDAPCertificateError): + _bind(_tls(Path(pki.ca).read_text()), _server(pki), host="dc1.example.test") + + +def test_a_dc_outside_the_anchor_is_refused(pki: _Pki, tmp_path: Path) -> None: + """``ca_certs_data`` reaches the engine's context: a CA other than the anchor is refused.""" + other_ca, _key = _self_signed(tmp_path) + with pytest.raises(ssl.SSLCertVerificationError): + _bind(_tls(other_ca.read_text()), _server(pki)) + + +def test_with_verification_off_ldap3_skips_the_host_name_check_as_before(pki: _Pki) -> None: + """ldap3 runs its name check only for ``CERT_REQUIRED`` or ``CERT_OPTIONAL``. The override keeps + that, so ``ad_tls_verify=false`` (refused at startup unless escaped) behaves as it did.""" + suite, _protocol = _bind( + _tls(Path(pki.ca).read_text(), ssl.CERT_NONE), _server(pki), host="dc1.example.test" + ) + assert suite in tls_policy._APPROVED_TLS_SUITES + + +# --- TLS 1.3: the reason for BACKLOG #2494 -------------------------------------------------------- + + +@pytest.mark.skipif(not _has_set_ciphersuites, reason="needs SSLContext.set_ciphersuites (3.15)") +def test_on_315_the_ldaps_bind_constructs_and_offers_no_tls13_aes128() -> None: + """Before #2494 the assertion refused LDAPS here: ldap3's context kept ``TLS_AES_128_GCM_SHA256`` + and the allow-list had dropped it. Now the bind constructs, and its context offers exactly the + approved TLS 1.3 list.""" + tls = ldap_auth.LdapAuthenticator(_ad_settings())._tls + assert tls is not None + ctx = tls._context_factory() + assert _tls13(ctx) == list(tls_policy.APPROVED_TLS13_SUITES) + assert _TLS13_AES128 not in tls_policy._APPROVED_TLS_SUITES + + +@pytest.mark.skipif(not _has_set_ciphersuites, reason="needs SSLContext.set_ciphersuites (3.15)") +def test_on_315_the_ldaps_bind_refuses_a_tls13_aes128_only_dc(pki: _Pki) -> None: + """A real TLS 1.3 handshake against a DC offering only AES-128. The control is ldap3's own + ``Tls`` with the same anchor, which completes: without it the refusal could be a broken peer.""" + control = ldap3.Tls(validate=ssl.CERT_REQUIRED, ca_certs_data=Path(pki.ca).read_text()) + assert _bind(control, _server(pki, tls13_only_aes128=True)) == (_TLS13_AES128, "TLSv1.3") + with pytest.raises(ssl.SSLError): + _bind(_tls(Path(pki.ca).read_text()), _server(pki, tls13_only_aes128=True)) + + +@pytest.mark.skipif(_has_set_ciphersuites, reason="measures the 3.14 gap; 3.15 has its own tests") +def test_on_314_the_ldaps_bind_keeps_the_recorded_tls13_gap() -> None: + """The 3.14 arm. No API can drop ``TLS_AES_128_GCM_SHA256`` here, so the LDAPS context offers + what a stock one does at TLS 1.3, and the allow-list admits it (``narrow_tls13_suites``).""" + tls = ldap_auth.LdapAuthenticator(_ad_settings())._tls + assert tls is not None + ctx = tls._context_factory() + assert _tls13(ctx) == _tls13(ssl.create_default_context()) + assert _TLS13_AES128 in tls_policy._APPROVED_TLS_SUITES diff --git a/tests/test_tls_cipher_assertion_sites.py b/tests/test_tls_cipher_assertion_sites.py index 3b77a4430..097bce28d 100644 --- a/tests/test_tls_cipher_assertion_sites.py +++ b/tests/test_tls_cipher_assertion_sites.py @@ -37,7 +37,7 @@ import ssl import threading import urllib.request -from collections.abc import Iterator, Mapping +from collections.abc import Iterator from pathlib import Path from typing import Any @@ -74,10 +74,6 @@ # which also means the assertions below exercise the same module objects the engine uses. -#: The one ``ciphers`` value ``assert_ldap3_tls_suites`` admits (BACKLOG #300). -_APPROVED_LDAP3_CIPHERS = ":".join(tls_policy.APPROVED_TLS12_SUITES) - - def _cipher_names(ctx: ssl.SSLContext) -> list[str]: return [str(c["name"]) for c in ctx.get_ciphers()] @@ -461,17 +457,11 @@ def test_postgres_default_arm_context_asserts(every_suite_looks_weak: None) -> N # --- the AD LDAPS bind: auth/ldap.py -------------------------------------------------------------- # -# BACKLOG #1317 remainder. The one site in this file where the IDENTITY instrument above cannot be -# used at all. `ldap3.Tls` holds no SSLContext (measured: zero SSLContext attributes on the object) and -# exposes no `ssl_context=` parameter -- it stores the arguments and builds the context inside -# `Tls.wrap_socket` at connect time. So the engine can never hold the object this hop will use, and -# "is this the same object?" has no answer here. -# -# Two measurements stand in for it, and together they cover both halves of the drift risk: -# * CONTEXT half -- `test_the_ldaps_replica_matches_the_context_ldap3_actually_builds` drives ldap3's -# REAL wrap_socket over a socketpair and compares the captured context to the replica. -# * ARGUMENT half -- `test_the_asserted_ldaps_arguments_are_the_ones_the_bind_uses` requires the -# kwargs the assertion ran on to be the kwargs `_server()` hands `ldap3.Tls`. +# BACKLOG #1317 remainder, then #2494. ldap3 2.9.1 builds its TLS context inside `Tls.wrap_socket` and +# takes no `ssl_context=`. Until #2494 the engine could only assert a REPLICA of that context. Since +# #2494 the engine builds the context itself (`tls_policy.assert_ldap3_tls_suites` returns the +# factory) and `auth.ldap_tls.NarrowedTls` wraps each connection with it. So the IDENTITY instrument +# the openers get works here too: the context on the wrapped socket must be one the assertion ran on. def _ad_settings(**overrides: Any) -> AuthSettings: @@ -491,11 +481,12 @@ def _ad_settings(**overrides: Any) -> AuthSettings: def _context_ldap3_builds(tls: Any) -> ssl.SSLContext: - """The ``SSLContext`` ldap3's OWN ``wrap_socket`` builds for ``tls`` — CAPTURED, not reconstructed. + """The ``SSLContext`` ``tls.wrap_socket`` really wraps with: CAPTURED, not reconstructed. - This is what keeps the replica honest. ``wrap_socket`` needs a real socket, so it gets one end of a - ``socketpair`` and ``do_handshake=False``; the context is then readable off the returned - ``SSLSocket``. No peer, no handshake, no network — but ldap3's own construction code really ran. + ``wrap_socket`` needs a real socket, so it gets one end of a ``socketpair`` and + ``do_handshake=False``; the context is then readable off the returned ``SSLSocket``. No peer, no + handshake, no network. For a plain ``ldap3.Tls`` this runs ldap3's own construction; for the + engine's ``NarrowedTls`` it runs the engine's factory, as a real bind would. """ class _Server: @@ -511,7 +502,7 @@ def __init__(self, sock: socket.socket) -> None: try: tls.wrap_socket(conn, do_handshake=False) ctx = conn.socket.context - assert isinstance(ctx, ssl.SSLContext), "ldap3 did not leave an SSLContext on the socket" + assert isinstance(ctx, ssl.SSLContext), "wrap_socket left no SSLContext on the socket" return ctx finally: conn.socket.close() @@ -522,10 +513,8 @@ def __init__(self, sock: socket.socket) -> None: def test_ad_ldaps_bind_asserts(every_suite_looks_weak: None) -> None: """``LdapAuthenticator.__init__`` — the service-account and user binds to Active Directory. - Asserted at construction rather than inside ``_server()``: the suite list is fixed by configuration - and cannot change between calls, ``AuthService`` builds this eagerly at app construction, and - ``_server()`` runs up to three times per login (so a per-call replica would reload the OS trust - store on the login path to re-derive an answer that cannot have changed). + The factory runs once at construction, so a bad context fails app startup: ``AuthService`` + builds this eagerly. It runs again for every connection after that. """ with pytest.raises(ValueError, match="LDAPS bind to AD"): @@ -551,151 +540,73 @@ def test_a_plaintext_ldap_bind_has_no_tls_context_to_assert(every_suite_looks_we @pytest.mark.parametrize("validate", [ssl.CERT_REQUIRED, ssl.CERT_NONE]) -def test_the_ldaps_replica_matches_the_context_ldap3_actually_builds( +def test_every_ldaps_connection_wraps_with_a_context_the_assertion_ran_on( validate: ssl.VerifyMode, tmp_path: Path, asserted_contexts: list[tuple[str, ssl.SSLContext]], ) -> None: - """The replica must resolve to the same suite list as the context ldap3 really builds. + """IDENTITY, the instrument every opener site gets (BACKLOG #2494). - This is the substitute for the identity check every other site in this file gets, and it compares - against ldap3's OWN construction rather than a second reading of its source. The replica is not - re-derived here either — it is taken off the ``asserted_contexts`` spy, so this compares the exact - object the shipped control checked against the exact object the hop will use. - - Both verification modes, because ``validate`` is the one replicated argument that differs between - deployments. A CA is supplied so the ``ca_certs_data`` arm is exercised: the replica - deliberately does not load it, and this is the measurement that says doing so would change nothing - about the suite list. + Each connection must wrap with a FRESH context the assertion ran on, not one built beside it. + Both verification modes, because ``validate`` is the argument that differs between deployments, + and a CA so the ``ca_certs_data`` arm runs. """ + from messagefoundry.auth.ldap_tls import NarrowedTls ca, _key = _self_signed(tmp_path) - kwargs: dict[str, object] = { - "validate": validate, - "ca_certs_data": ca.read_text("ascii"), - "ciphers": _APPROVED_LDAP3_CIPHERS, - } - - tls_policy.assert_ldap3_tls_suites(kwargs, connector="ldaps equivalence probe") - replicas = [ctx for label, ctx in asserted_contexts if label == "ldaps equivalence probe"] - assert len(replicas) == 1, "the assertion did not run exactly once on its own replica" - replica = replicas[0] - - real = _context_ldap3_builds(ldap3.Tls(**kwargs)) - assert [c["name"] for c in real.get_ciphers()] == [c["name"] for c in replica.get_ciphers()], ( - "the replica no longer resolves to the suite list ldap3's own wrap_socket produces, so the " - "AD LDAPS assertion is now checking a context this hop will not use" + tls = NarrowedTls( + validate=validate, ca_certs_data=ca.read_text("ascii"), connector="ldaps identity probe" ) - assert real.verify_mode == replica.verify_mode - assert real.check_hostname == replica.check_hostname - # BACKLOG #300: and the list both resolve to is the approved one, not merely the same one. - assert _tls12_names(real) == list(tls_policy.APPROVED_TLS12_SUITES) + first, second = _context_ldap3_builds(tls), _context_ldap3_builds(tls) + checked = [ctx for label, ctx in asserted_contexts if label == "ldaps identity probe"] + assert len(checked) == 3, "construction plus one per connection" + assert first is checked[1] and second is checked[2] and first is not second + assert first.verify_mode == validate == tls.validate and first.check_hostname is False + assert first.minimum_version == ssl.TLSVersion.TLSv1_2 + assert _tls12_names(first) == list(tls_policy.APPROVED_TLS12_SUITES) -def test_the_asserted_ldaps_arguments_are_the_ones_the_bind_uses( + +def test_every_server_the_bind_builds_carries_the_one_engine_tls( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """What was ASSERTED must be what ``_server()`` hands ``ldap3.Tls`` — the other half of the drift. - - An equivalent context is worthless if the bind is built from different arguments, and the two live - in different methods. ``_tls_kwargs()`` is the single definition both read; this measures that they - really do agree, so a future edit to ``_server()`` alone cannot silently leave the assertion - checking a stale shape. - """ + """``_server()`` runs up to three times per login. Each ``Server`` must carry the engine's + ``NarrowedTls``, anchored at the checked bytes, never a path, and with no ``ciphers``.""" + from messagefoundry.auth.ldap_tls import NarrowedTls ca, _key = _self_signed(tmp_path) # The constructor checks the anchor since BACKLOG #2034; pin its ACL and path verdicts to clean so # the result does not depend on this machine's temp directory. monkeypatch.setattr(trust_anchors, "dacl_is_owner_only", lambda _p: True) monkeypatch.setattr(trust_anchors, "anchor_path_verdict", lambda _p: PathVerdict(ok=True)) - seen: list[Mapping[str, object]] = [] - real = tls_policy.assert_ldap3_tls_suites - - def spy(tls_kwargs: Mapping[str, object], *, connector: str) -> None: - seen.append(dict(tls_kwargs)) - real(tls_kwargs, connector=connector) - - monkeypatch.setattr(ldap_auth, "assert_ldap3_tls_suites", spy) auth = ldap_auth.LdapAuthenticator(_ad_settings(ad_tls_ca_cert_file=str(ca))) - assert len(seen) == 1, "the AD LDAPS bind did not assert its TLS suites exactly once" tls = auth._server().tls - assert seen[0] == { - "validate": tls.validate, - "ca_certs_data": tls.ca_certs_data, - "ciphers": tls.ciphers, - } + assert isinstance(tls, NarrowedTls) and auth._server().tls is tls + assert tls.validate == ssl.CERT_REQUIRED assert tls.ca_certs_data == ca.read_text("ascii") and tls.ca_certs_file is None - assert tls.ciphers == _APPROVED_LDAP3_CIPHERS - - -def test_the_ldaps_assertion_refuses_a_tls_argument_it_cannot_replicate() -> None: - """An unreplicable ``Tls`` argument must REFUSE, not be replicated wrongly or ignored. - - Deliberately without ``every_suite_looks_weak``: this raise has to stand on its own, so a reader - can tell the refusal apart from a suite-list failure. - """ - - with pytest.raises(ValueError, match="sni"): - tls_policy.assert_ldap3_tls_suites( - { - "validate": ssl.CERT_REQUIRED, - "ciphers": _APPROVED_LDAP3_CIPHERS, - "sni": "dc1.example.test", - }, - connector="AD LDAPS bind", - ) - - -@pytest.mark.parametrize( - "ciphers", - [ - None, - "ECDHE-RSA-AES256-GCM-SHA384", - "ECDHE-ECDSA-AES256-SHA384", - "@SECLEVEL=0:" + ":".join(tls_policy.APPROVED_TLS12_SUITES), - ":".join(reversed(tls_policy.APPROVED_TLS12_SUITES)), - "THIS-IS-NOT-A-SUITE", - ], - ids=["missing", "one-suite", "cbc", "seclevel-directive", "reordered", "rejected-by-openssl"], -) -def test_the_ldaps_assertion_admits_only_the_approved_cipher_string(ciphers: str | None) -> None: - """BACKLOG #300: ``ciphers`` is REQUIRED and has exactly one admitted value. - - Missing would leave the interpreter's wider list in place. A narrower or reordered list is not - the approved one. A directive is refused like every ``@`` token (BACKLOG #2106). A string OpenSSL - rejects is the case ldap3 swallows, measured by the test below, so it must never reach ldap3. - """ + assert tls.ciphers is None # the engine's context carries the suites, not ldap3's lever - kwargs: dict[str, object] = {"validate": ssl.CERT_REQUIRED} - if ciphers is not None: - kwargs["ciphers"] = ciphers - with pytest.raises(ValueError, match="`ciphers` equal to the approved"): - tls_policy.assert_ldap3_tls_suites(kwargs, connector="AD LDAPS bind") - -def test_the_ldaps_assertion_holds_the_replica_to_the_approved_list( +def test_the_ldaps_assertion_holds_the_context_to_the_approved_list( monkeypatch: pytest.MonkeyPatch, ) -> None: - """The check after the string compare must fail on its own, or it is decoration. + """The list check after ``harden_cipher_suites`` must fail on its own, or it is decoration. - Widening the tuple the string is compared against makes a CBC string pass that compare. The - replica then offers a suite ``_APPROVED_TLS_SUITES`` does not hold, and only the list check - can see it: the CBC suite is forward-secret, encrypting, authenticated and 256-bit. + Widening the tuple the narrowing applies puts a CBC suite on the context. That suite is + forward-secret, encrypting, authenticated and 256-bit, so only the list check can see it. """ widened = (*tls_policy.APPROVED_TLS12_SUITES, "ECDHE-ECDSA-AES256-SHA384") monkeypatch.setattr(tls_policy, "APPROVED_TLS12_SUITES", widened) with pytest.raises(ValueError, match="not narrowed to the approved list"): tls_policy.assert_ldap3_tls_suites( - {"validate": ssl.CERT_REQUIRED, "ciphers": ":".join(widened)}, - connector="AD LDAPS bind", + validate=ssl.CERT_REQUIRED, ca_certs_data=None, connector="AD LDAPS" ) -def test_the_shipped_ldaps_bind_offers_exactly_the_approved_tls12_list(tmp_path: Path) -> None: - """The POSITIVE control, taken off ldap3's OWN ``wrap_socket`` for the ``Tls`` the shipped bind - builds, and so off the context this hop will really use (BACKLOG #300).""" +def test_the_shipped_ldaps_bind_offers_exactly_the_approved_list(tmp_path: Path) -> None: + """The POSITIVE control, taken off the socket the shipped bind's ``Tls`` really wraps.""" auth = ldap_auth.LdapAuthenticator(_ad_settings()) real = _context_ldap3_builds(auth._server().tls) @@ -703,33 +614,14 @@ def test_the_shipped_ldaps_bind_offers_exactly_the_approved_tls12_list(tmp_path: assert all(n in tls_policy._APPROVED_TLS_SUITES for n in _cipher_names(real)) -def test_the_ldaps_assertion_refuses_when_the_verify_mode_is_unknown() -> None: - """No ``validate`` means the peer-verification mode ldap3 will apply is unknown — refuse it.""" - - with pytest.raises(ValueError, match="no `validate` given"): - tls_policy.assert_ldap3_tls_suites({"ca_certs_data": None}, connector="AD LDAPS bind") - - -def test_the_ldaps_assertion_refuses_a_ca_path() -> None: - """BACKLOG #2034: the bind hands ldap3 the checked bytes, never the path. A ``ca_certs_file`` here - would mean the bind reads the anchor again after its check, so the assertion refuses it.""" - - with pytest.raises(ValueError, match="ca_certs_file"): - tls_policy.assert_ldap3_tls_suites( - {"validate": ssl.CERT_REQUIRED, "ca_certs_file": "ca.pem"}, connector="AD LDAPS bind" - ) - - def test_ldap3_swallows_a_rejected_cipher_string_and_strips_every_tls12_suite() -> None: - """The measurement that makes ``ciphers=`` dangerous, and why only ONE value of it is admitted. + """Why ldap3's own ``ciphers=`` lever was never trustworthy, kept as a measurement. ``ldap3/core/tls.py`` wraps ``set_ciphers`` in ``except ssl.SSLError: pass``. A cipher string OpenSSL rejects therefore vanishes without a log line, and the hop silently loses its ENTIRE TLS - 1.2 suite list while still reporting a configured cipher policy: a control that cannot report its - own failure (SDS-3.7). Since BACKLOG #300 the bind passes ``ciphers=`` anyway, so - ``assert_ldap3_tls_suites`` admits only the approved string and applies it with a ``set_ciphers`` - that raises. If ldap3 ever stops swallowing, this test goes red and that rationale should be - re-derived rather than assumed. + 1.2 suite list while still reporting a configured cipher policy (SDS-3.7). Since BACKLOG #2494 + the engine no longer uses that lever, and ``NarrowedTls`` takes no ``ciphers=``. If ldap3 ever stops + swallowing, this goes red; leaving the lever out still stands, because the lever still cannot reach TLS 1.3. """ baseline = _context_ldap3_builds(ldap3.Tls(validate=ssl.CERT_REQUIRED)) @@ -738,8 +630,8 @@ def test_ldap3_swallows_a_rejected_cipher_string_and_strips_every_tls12_suite() ) assert _tls12_names(baseline), "the baseline offered no TLS 1.2 suites; this proves nothing" assert not _tls12_names(poisoned), ( - "ldap3 no longer strips the TLS 1.2 suites on a rejected cipher string; re-derive why " - "assert_ldap3_tls_suites admits only one `ciphers=` value before relying on that reason" + "ldap3 no longer strips the TLS 1.2 suites on a rejected cipher string; re-derive this " + "test's docstring before relying on that reason" ) diff --git a/tests/test_tls_default_suites.py b/tests/test_tls_default_suites.py index 5fda1ab29..294c05f5b 100644 --- a/tests/test_tls_default_suites.py +++ b/tests/test_tls_default_suites.py @@ -330,6 +330,10 @@ def _trusting(ctx: ssl.SSLContext, pki: _Pki) -> ssl.SSLContext: "DICOM destination": lambda p: _built( dicom._client_ssl_context({"tls": True, "host": "localhost", "tls_ca_file": p.ca}) ), + # The factory LdapAuthenticator builds, which NarrowedTls calls for each connection (#2494). + "LDAPS (AD bind)": lambda p: tls_policy.assert_ldap3_tls_suites( + validate=ssl.CERT_REQUIRED, ca_certs_data=Path(p.ca).read_text(), connector="LDAPS (test)" + )(), } diff --git a/tests/test_tls_handshake_sigalgs.py b/tests/test_tls_handshake_sigalgs.py index d44c80177..8264ab556 100644 --- a/tests/test_tls_handshake_sigalgs.py +++ b/tests/test_tls_handshake_sigalgs.py @@ -404,9 +404,9 @@ def _postgres_verify_off(k: Kit) -> object: def _ldaps(k: Kit) -> object: - """The real caller: ``LdapAuthenticator`` asserts the kwargs from its own ``_tls_kwargs``. The - context measured is the one the gate rebuilds from those kwargs, which is a replica: ldap3 builds - its own inside ``Tls.wrap_socket`` and holds none to compare against (the gate's docstring).""" + """The real caller: ``LdapAuthenticator`` builds its ``NarrowedTls``, which runs the factory + once at construction. Since BACKLOG #2494 that factory builds the context each LDAPS + connection wraps with, so the context measured is the kind the hop uses, not a replica.""" return LdapAuthenticator( AuthSettings( ad_enabled=True, @@ -418,6 +418,11 @@ def _ldaps(k: Kit) -> object: ) +#: The LDAPS site. Its factory builds the context ldap3 wraps each connection with (BACKLOG #2494). +_LDAPS_SITE = ( + "messagefoundry/config/tls_policy.py::assert_ldap3_tls_suites.narrowed_context::connector" +) + #: Every derived site, with how to build it. Each factory calls the REAL builder; the context #: measured is the one that builder handed to ``harden_cipher_suites``, captured by a spy. _SITES: dict[str, Callable[[Kit], object]] = { @@ -437,7 +442,7 @@ def _ldaps(k: Kit) -> object: "messagefoundry/config/tls_policy.py::build_asserted_https_handler::connector": lambda k: ( tls_policy.build_asserted_https_handler(connector="urllib default handler (measurement)") ), - "messagefoundry/config/tls_policy.py::assert_ldap3_tls_suites::connector": _ldaps, + _LDAPS_SITE: _ldaps, "messagefoundry/config/tls_policy.py::assert_hvac_tls_suites.narrowed_context::connector": ( lambda k: tls_policy.assert_hvac_tls_suites({}, connector="Vault (measurement)") ), @@ -487,11 +492,10 @@ def _ldaps(k: Kit) -> object: _VAULT_SITE = ( "messagefoundry/config/tls_policy.py::assert_hvac_tls_suites.narrowed_context::connector" ) -#: Sites whose product exposes no context to tie the capture to: the hvac factory builds a fresh one -#: per connection, and ldap3 builds its own at connect time from the kwargs the gate replicated. -_HOLDS_NO_CONTEXT = frozenset( - {_VAULT_SITE, "messagefoundry/config/tls_policy.py::assert_ldap3_tls_suites::connector"} -) +#: Sites whose product exposes no context to tie the capture to: the hvac and LDAPS factories each +#: build a fresh one per connection. tests/test_tls_cipher_assertion_sites.py ties the LDAPS context +#: a connection wraps with to one the assertion ran on. +_HOLDS_NO_CONTEXT = frozenset({_VAULT_SITE, _LDAPS_SITE}) _VAULT_EXTRA = pytest.mark.skipif( not extra_is_installed(OPTIONAL_EXTRAS["vault"]), reason="the [vault] extra (hvac + requests + urllib3) is not installed in this interpreter", From a44d060778c89a5be594f63da50ca25119521ab1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 18:40:15 -0500 Subject: [PATCH 02/14] test(auth): drive LDAPS through a real ldap3 Connection; name the referral gap (BACKLOG #2494) Review round 1 (code-review subagent, xhigh) found no lost TLS guarantee. Two of its low findings are fixed here. Every handshake test called wrap_socket directly, so a future ldap3 that bypassed the override would stay green. test_a_real_ldap3_connection_uses_the_narrowed_context opens a real ldap3.Connection: a CBC-only DC is refused by NarrowedTls and accepted by ldap3's own Tls (control). Mutation-checked: restoring ldap3's wrap_socket turns it red. The NarrowedTls docstring no longer cites the referral copy as reassurance; it names it as not reached. Proposed PR title: LDAPS narrows TLS 1.3 through an engine ldap3.Tls subclass, so AD sign-in survives Python 3.15 (BACKLOG #2494) Proposed ledger banner: SHIPPED 2026-09-30 on option 3 (Manager's choice): the engine builds the LDAPS context and NarrowedTls wraps each connection with it; measured on CPython 3.15.0b3, the bind constructs and refuses a TLS 1.3 AES-128-only DC. Referral path still unnarrowed (pre-existing). --- messagefoundry/auth/ldap_tls.py | 10 +++-- tests/test_ldap_tls.py | 70 ++++++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/messagefoundry/auth/ldap_tls.py b/messagefoundry/auth/ldap_tls.py index b61ae0aa1..2bf1a1e13 100644 --- a/messagefoundry/auth/ldap_tls.py +++ b/messagefoundry/auth/ldap_tls.py @@ -38,9 +38,13 @@ class NarrowedTls(ldap3.Tls): # type: ignore[misc] # ldap3 ships no type infor The constructor builds the context factory from ``validate`` and ``ca_certs_data`` and runs it once, so a bad context fails here. It hands the same two values to ``ldap3.Tls``, so what ldap3 - reads off this object stays true: ``validate`` decides its host name check, and a followed - referral copies it. It takes no other ldap3 argument, because the engine's context would not - carry one. ``ciphers=`` in particular reached TLS 1.2 only. + reads off this object stays true: ``validate`` decides its host name check. It takes no other + 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. """ def __init__( diff --git a/tests/test_ldap_tls.py b/tests/test_ldap_tls.py index 41b777cb9..2b96e6410 100644 --- a/tests/test_ldap_tls.py +++ b/tests/test_ldap_tls.py @@ -32,13 +32,19 @@ import ldap3 import pytest -from ldap3.core.exceptions import LDAPCertificateError +from ldap3.core.exceptions import LDAPCertificateError, LDAPSocketOpenError from messagefoundry.auth import ldap as ldap_auth from messagefoundry.auth.ldap_tls import NarrowedTls from messagefoundry.config import tls_policy from tests.test_tls_cipher_assertion_sites import _ad_settings, _self_signed -from tests.test_tls_default_suites import _Pki, _tls13 +from tests.test_tls_default_suites import ( + _DEFAULT_SUITE_NAMES, + CBC_ONLY, + _Pki, + _tls12, + _tls13, +) from tests.test_tls_default_suites import pki as pki # noqa: F401 (the fixture, re-exported) #: sha256 of ``inspect.getsource(ldap3.Tls.wrap_socket)`` in ldap3 2.9.1, line endings as ``\n``. @@ -92,6 +98,66 @@ def serve() -> None: thread.join(10) +def _open_ldaps(tls: Any, server: ssl.SSLContext) -> Any: + """Open a real ``ldap3.Connection`` to ``server`` on a loopback port, so the TLS goes through + ldap3's own call site rather than a direct ``wrap_socket`` call. Returns the TLS socket ldap3 + kept; the caller closes it. ldap3 raises its own ``LDAPSocketOpenError`` on a refusal.""" + listener = socket.create_server(("127.0.0.1", 0)) + listener.settimeout(10) + port = listener.getsockname()[1] + + def serve() -> None: + with contextlib.suppress(ssl.SSLError, OSError): + raw, _addr = listener.accept() + raw.settimeout(10) + with server.wrap_socket(raw, server_side=True): + pass + + thread = threading.Thread(target=serve, daemon=True) + thread.start() + try: + conn = ldap3.Connection( + ldap3.Server( + "127.0.0.1", + port=port, + use_ssl=True, + tls=tls, + get_info=ldap3.NONE, + connect_timeout=10, + ), + receive_timeout=10, + ) + conn.open() + return conn.socket + finally: + listener.close() + thread.join(10) + + +@pytest.mark.skipif(CBC_ONLY not in _DEFAULT_SUITE_NAMES, reason="default lacks the CBC suite") +def test_a_real_ldap3_connection_uses_the_narrowed_context(pki: _Pki) -> None: + """Through ``ldap3.Connection.open``, ldap3's real call site. A DC offering only a CBC-SHA2 + suite is refused by the engine's ``Tls`` and accepted by ldap3's own, the control, so the + refusal can only come from the override having run. Verification is off in both, because the + connection is to an IP address the test leaf does not name; the suite is what is measured.""" + cbc_server = _server(pki) + cbc_server.maximum_version = ssl.TLSVersion.TLSv1_2 + cbc_server.set_ciphers(CBC_ONLY) + control = _open_ldaps(ldap3.Tls(validate=ssl.CERT_NONE), cbc_server) + try: + assert control.cipher()[0] == CBC_ONLY + finally: + control.close() + with pytest.raises(LDAPSocketOpenError): + _open_ldaps(_tls(Path(pki.ca).read_text(), ssl.CERT_NONE), cbc_server) + + engine = _open_ldaps(_tls(Path(pki.ca).read_text(), ssl.CERT_NONE), _server(pki)) + try: + assert _tls12(engine.context) == list(tls_policy.APPROVED_TLS12_SUITES) + finally: + engine.close() + + def test_ldap3_wrap_socket_is_the_one_narrowed_tls_copies() -> None: """``NarrowedTls.wrap_socket`` copies ldap3's wrap step. If ldap3 changes that method, this goes red: read the new source, carry over what changed, then update the hash.""" From 12d16712ca61d7a2211de19301286bf6170aa388 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 19:07:53 -0500 Subject: [PATCH 03/14] fix(auth): the AD hop follows no LDAP referral, and refuses one (BACKLOG #2530) ldap3 2.9.1 follows a referral by default: Connection(auto_referrals=True) and Server(allowed_referral_hosts=None), read as [('*', True)]. On a bound connection, strategy/base.py create_referral_connection (751-797) opens a connection to the referred host and binds there with the same user and password, over a plain ldap3.Tls (no pinned CA bytes, no narrowing) or none for ldap://. A first deployment would have sent the service-account password to whatever host one search referral named. Every Connection in auth/ldap.py now sets auto_referrals=False and the Server sets allowed_referral_hosts=[]; each alone stops the follow. Every search goes through _search, and a referral answer to a search or to the user bind raises LdapError naming only the referred host. So a referral is audited as auth.login_error at sign-in, read as unavailable by the reconciler (never revokes), and not counted as a wrong password by the step-up re-bind. Before this, with following off, it would have read as "no entries" or "wrong password". Tests drive a real ldap3 stack against two loopback LDAP servers: the referred one accepts no connection; with both guards reverted it receives the bind password (positive control). Static guards pin the kwargs and the single search path. Docs: ADR 0180 Amendment F, PHI.md section 4, a changelog.d fragment. #2530 is filed on vault PR 2120, not yet on vault main. --- changelog.d/2530.security.md | 10 + docs/PHI.md | 2 +- ...on-a-library-that-exposes-no-sslcontext.md | 20 +- messagefoundry/auth/ldap.py | 82 +++- messagefoundry/auth/ldap_tls.py | 8 +- tests/test_ad_directory_identity.py | 3 + tests/test_ad_user_account_control.py | 1 + tests/test_auth_hardening.py | 2 + tests/test_ldap_referrals.py | 395 ++++++++++++++++++ tests/test_ldap_timeouts.py | 25 +- 10 files changed, 539 insertions(+), 9 deletions(-) create mode 100644 changelog.d/2530.security.md create mode 100644 tests/test_ldap_referrals.py diff --git a/changelog.d/2530.security.md b/changelog.d/2530.security.md new file mode 100644 index 000000000..7f853910e --- /dev/null +++ b/changelog.d/2530.security.md @@ -0,0 +1,10 @@ +- **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 answer + is refused as a directory error naming only the referred host: 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. See [ADR 0180](../docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) + Amendment F. (`BACKLOG #2530`) diff --git a/docs/PHI.md b/docs/PHI.md index cd44f9821..14aa66390 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -873,7 +873,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 7b01a82c8..e14b793b0 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` @@ -320,3 +320,21 @@ 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 answer is now +an `LdapError` that names the referred host 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 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. diff --git a/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index c7139972c..1bb50f9ac 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -14,16 +14,21 @@ ``[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 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 @@ -377,6 +382,68 @@ 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}") + + +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() + for uri in referrals: + try: + host = urlsplit(str(uri)).hostname + except ValueError: + host = None + hosts.add(host if host and _PRINTABLE_HOST.fullmatch(host) else "") + return ", ".join(sorted(hosts)) 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, which a global catalog or a per-domain setting answers. + """ + 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). Point the search base at this domain controller's own domain, or at a " + "global catalog." + ) + _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 answer. Every search in this module goes + through here, so none can read a referral 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 +544,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: @@ -492,6 +562,7 @@ def _service_conn(self) -> Any: authentication=ldap3.SIMPLE, auto_bind=True, receive_timeout=self._s.ad_receive_timeout, + auto_referrals=False, # BACKLOG #2530: see _refuse_referral ) def _equalizing_bind(self, password: str) -> None: @@ -524,6 +595,7 @@ def _equalizing_bind(self, password: str) -> None: password=password, authentication=ldap3.SIMPLE, 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 +631,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 +744,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, @@ -728,12 +804,14 @@ def authenticate( password=password, authentication=ldap3.SIMPLE, 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/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..d5bc4d2ed --- /dev/null +++ b/tests/test_ldap_referrals.py @@ -0,0 +1,395 @@ +# 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 + + +# --- 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``.""" + tree = ast.parse(Path(ldap_module.__file__).read_text(encoding="utf-8")) + callers = [ + func.name + for func in ast.walk(tree) + if isinstance(func, ast.FunctionDef) + for node in ast.walk(func) + 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() + 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..63262d53b 100644 --- a/tests/test_ldap_timeouts.py +++ b/tests/test_ldap_timeouts.py @@ -37,6 +37,7 @@ from __future__ import annotations import ast +import functools from pathlib import Path from typing import Any @@ -99,10 +100,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 +128,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 +138,10 @@ def __exit__(self, *exc: object) -> None: def search(self, **kwargs: Any) -> bool: base = str(kwargs.get("search_base", "")) + 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 +166,9 @@ 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 return bind_ok def unbind(self) -> None: @@ -372,6 +392,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. From eb78899c6e7d8dc776116a79ea79527087bdf39a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 19:17:25 -0500 Subject: [PATCH 04/14] fix(auth): name few referred hosts and scope the refusal to referral results (BACKLOG #2530) Code-review round 1 (code-review subagent, xhigh): no high finding. Repaired here: - the refusal names at most three hosts and counts the rest, and admits an underscore in a host name; - docstrings, ADR 0180 Amendment F and the changelog now say the refusal covers a referral RESULT (resultCode 10). A searchResRef continuation reference is never followed by ldap3 and still reads as no entry; AD adds one to every domain-root search, so refusing it would refuse all; - the refusal's advice no longer assumes a search, and the ADR says a global catalog carries universal-group membership only; - the ldap3 double sets result on every operation, as ldap3 does; - the single-search-path guard covers async and module scope and lets a regular expression's .search through. Not repaired, left for the Manager (see the PR body): the reconciler reads a referral as an outage, so a misconfigured base would hold every pass rather than revoke; the service connection is never unbound (pre-existing); a single Connection factory instead of three sites. Proposed PR title: The AD hop follows no LDAP referral, so the bind password cannot leave the anchored LDAPS hop (BACKLOG #2530) Proposed ledger banner: SHIPPED 2026-09-30: every ldap3 Connection sets auto_referrals=False and the Server allowed_referral_hosts=[]; a referral result refuses as LdapError naming only hosts. Loopback test: referred host gets no connection; with both guards reverted it receives the bind password. Stacked on PR 1869. --- changelog.d/2530.security.md | 4 +-- ...on-a-library-that-exposes-no-sslcontext.md | 12 +++++--- messagefoundry/auth/ldap.py | 24 ++++++++++----- tests/test_ldap_referrals.py | 29 +++++++++++++++---- tests/test_ldap_timeouts.py | 3 ++ 5 files changed, 54 insertions(+), 18 deletions(-) diff --git a/changelog.d/2530.security.md b/changelog.d/2530.security.md index 7f853910e..e05f3d64f 100644 --- a/changelog.d/2530.security.md +++ b/changelog.d/2530.security.md @@ -2,8 +2,8 @@ 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 answer - is refused as a directory error naming only the referred host: sign-in audits it as + `auto_referrals=False`, and its `ldap3.Server` sets `allowed_referral_hosts=[]`. A referral result + is refused as a directory error naming only the referred hosts: 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. See [ADR 0180](../docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) 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 e14b793b0..e4b0310db 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 @@ -332,9 +332,13 @@ 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 answer is now -an `LdapError` that names the referred host 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. +`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. +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/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index 1bb50f9ac..a015450b8 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -384,7 +384,10 @@ def _cn_of(dn: str) -> str | 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}") +_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: @@ -400,7 +403,9 @@ def _referred_hosts(referrals: Iterable[object]) -> str: except ValueError: host = None hosts.add(host if host and _PRINTABLE_HOST.fullmatch(host) else "") - return ", ".join(sorted(hosts)) or "" + named = sorted(hosts)[:_HOSTS_NAMED] + more = len(hosts) - len(named) + return ", ".join(named) + (f" and {more} more" if more else "") or "" def _refuse_referral(conn: Any, operation: str) -> None: @@ -419,7 +424,12 @@ def _refuse_referral(conn: Any, operation: str) -> None: 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, which a global catalog or a per-domain setting answers. + 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 @@ -430,16 +440,16 @@ def _refuse_referral(conn: Any, operation: str) -> None: 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). Point the search base at this domain controller's own domain, or at a " - "global catalog." + "(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." ) _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 answer. Every search in this module goes - through here, so none can read a referral as "no entries"; a test pins that.""" + """``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) diff --git a/tests/test_ldap_referrals.py b/tests/test_ldap_referrals.py index d5bc4d2ed..72d112aaf 100644 --- a/tests/test_ldap_referrals.py +++ b/tests/test_ldap_referrals.py @@ -349,21 +349,40 @@ def test_the_refusal_names_hosts_and_nothing_else_the_directory_sent() -> None: 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 + + # --- 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``.""" + place a ``.search(...)`` call may appear in ``auth/ldap.py``, at any depth, async or not. A + regular expression's ``.search`` is not an LDAP search and is let through.""" tree = ast.parse(Path(ldap_module.__file__).read_text(encoding="utf-8")) + regexes = {"re", "_PRINTABLE_HOST"} + owner = { + id(node): scope.name + for scope in ast.walk(tree) + if isinstance(scope, ast.FunctionDef | ast.AsyncFunctionDef) + for node in ast.walk(scope) + } # the innermost scope wins: ast.walk visits outer functions first, and later keys overwrite callers = [ - func.name - for func in ast.walk(tree) - if isinstance(func, ast.FunctionDef) - for node in ast.walk(func) + 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" + and not (isinstance(node.func.value, ast.Name) and node.func.value.id in regexes) ] assert callers == ["_search"], callers diff --git a/tests/test_ldap_timeouts.py b/tests/test_ldap_timeouts.py index 63262d53b..d45476699 100644 --- a/tests/test_ldap_timeouts.py +++ b/tests/test_ldap_timeouts.py @@ -138,6 +138,8 @@ 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 = [] @@ -169,6 +171,7 @@ def bind(self) -> bool: 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: From 453990c190eab07831b9f4844c947ccc8c37a2e1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 20:08:09 -0500 Subject: [PATCH 05/14] fix(auth): list readable referred hosts first; scope the docs to the binds they cover (BACKLOG #2530) Code-review round 2 (code-review subagent, xhigh): no new correctness defect; six low findings, five repaired here: - an unreadable referral URL no longer takes a named slot ahead of a readable host; it is listed last; - the refusal's global-catalog advice carries the universal-groups caveat and points at ADR 0180 Amendment F; - the changelog says a referral to the service-account bind fails that bind as before (ldap3's auto_bind error), not a host-naming refusal; - the underscore, placeholder-order and empty cases are asserted directly; - the single-search-path guard drops its dead regex exclusion and maps only function bodies. Left: none of substance. Checks: ruff check and format, mypy strict on messagefoundry and tests, pytest over every tests/test_*.py file in seven xdist chunks (all green, before this commit), and the auth/LDAP/TLS/ADR slice after it (1016 passed, 3 skipped). Proposed PR title: The AD hop follows no LDAP referral, so the bind password cannot leave the anchored LDAPS hop (BACKLOG #2530) Proposed ledger banner: SHIPPED 2026-09-30: every ldap3 Connection sets auto_referrals=False and the Server allowed_referral_hosts=[]; a referral result to a search or the user bind refuses as LdapError naming only hosts. Loopback test: referred host gets no connection; with both guards reverted it receives the bind password. Stacked on PR 1869. --- changelog.d/2530.security.md | 3 ++- messagefoundry/auth/ldap.py | 15 +++++++++++---- tests/test_ldap_referrals.py | 16 +++++++++++----- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/changelog.d/2530.security.md b/changelog.d/2530.security.md index e05f3d64f..6b424a48b 100644 --- a/changelog.d/2530.security.md +++ b/changelog.d/2530.security.md @@ -3,7 +3,8 @@ 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 - is refused as a directory error naming only the referred hosts: sign-in audits it as + 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. See [ADR 0180](../docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) diff --git a/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index a015450b8..6a31525bb 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -397,14 +397,20 @@ def _referred_hosts(referrals: Iterable[object]) -> str: 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 - hosts.add(host if host and _PRINTABLE_HOST.fullmatch(host) else "") - named = sorted(hosts)[:_HOSTS_NAMED] - more = len(hosts) - len(named) + 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 "" @@ -441,7 +447,8 @@ def _refuse_referral(conn: Any, operation: str) -> None: 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." + "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) diff --git a/tests/test_ldap_referrals.py b/tests/test_ldap_referrals.py index 72d112aaf..0a8252495 100644 --- a/tests/test_ldap_referrals.py +++ b/tests/test_ldap_referrals.py @@ -344,7 +344,7 @@ def test_the_refusal_names_hosts_and_nothing_else_the_directory_sent() -> None: "user search", ) message = str(refused.value) - assert "referral to , dc2.other.example;" in message + assert "referral to dc2.other.example, ;" in message for leaked in ("DC=other", "bindname", "cn=x", "\x1b", "evil"): assert leaked not in message @@ -359,6 +359,13 @@ def test_the_refusal_names_a_few_hosts_and_counts_the_rest() -> None: 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 -------------------------- @@ -367,14 +374,14 @@ def test_the_refusal_names_a_few_hosts_and_counts_the_rest() -> None: 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`` is not an LDAP search and is let through.""" + 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")) - regexes = {"re", "_PRINTABLE_HOST"} owner = { id(node): scope.name for scope in ast.walk(tree) if isinstance(scope, ast.FunctionDef | ast.AsyncFunctionDef) - for node in ast.walk(scope) + 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), "") @@ -382,7 +389,6 @@ def test_every_search_in_the_ldap_module_goes_through_the_refusing_wrapper() -> if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "search" - and not (isinstance(node.func.value, ast.Name) and node.func.value.id in regexes) ] assert callers == ["_search"], callers From cd9691aaa8cab6c3c95937ffb2c8fd14bb61a77e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:25:01 -0500 Subject: [PATCH 06/14] fix(auth): pass ldap3 an integer receive_timeout so AD connections open on Linux ldap3 sets SO_RCVTIMEO with struct.pack('LL', receive_timeout, 0) on every non-Windows host. [auth].ad_receive_timeout is a float (default 10.0), and struct.pack refuses a float, so every ldap3 socket open raised "struct.error: required argument is not an integer" on Linux before any byte reached the domain controller. Windows hid it because ldap3 converts with int(1000 * t) on that branch. The real-socket referral tests added for BACKLOG #2530 were the first to open an ldap3 socket on the Linux CI leg, and all five went red there. The engine was at fault, not the fake DC. _ldap3_receive_timeout rounds the setting UP to whole seconds: never shorter than configured, never 0 (ldap3's wait-forever). The POSIX branch drops sub-second precision anyway. test_ldap_timeouts now asserts an int equal to ceil(setting) and adds a rounding unit test whose float arm must raise struct.error. --- messagefoundry/auth/ldap.py | 23 ++++++++++++++++++++--- tests/test_ldap_timeouts.py | 33 +++++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index 6a31525bb..589c9e10d 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -21,6 +21,7 @@ from __future__ import annotations import logging +import math import re import ssl import uuid @@ -133,6 +134,22 @@ 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 before a single + byte reached the domain controller: 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: the result is never shorter than the operator configured and never + zero, which ldap3 reads as "wait forever" (ASVS 13.1.3). The POSIX branch drops sub-second + precision anyway (``tv_usec`` is always 0), so ``ceil`` loses nothing that branch could keep. + """ + return math.ceil(seconds) + + def _escape_filter(value: str) -> str: """RFC 4515 escaping for values interpolated into an LDAP search filter.""" out: list[str] = [] @@ -578,7 +595,7 @@ 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 ) @@ -611,7 +628,7 @@ 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: @@ -820,7 +837,7 @@ 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 diff --git a/tests/test_ldap_timeouts.py b/tests/test_ldap_timeouts.py index d45476699..0afda9b50 100644 --- a/tests/test_ldap_timeouts.py +++ b/tests/test_ldap_timeouts.py @@ -38,16 +38,18 @@ 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 +_RECEIVE_TIMEOUT = 9.25 # fractional on purpose: ldap3 needs an int on POSIX (see below) def _ad_settings(**over: Any) -> AuthSettings: @@ -201,14 +203,37 @@ 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 packs it with struct.pack('LL', value, 0) on every + # non-Windows host, and a float raises struct.error there before the socket is used. 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" + ) + struct.pack("LL", value, 0) # ldap3's own POSIX call; raises struct.error on a float # --- 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: From c947b5cc6fb14785ecf196468cc2964876e3d242 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:27:32 -0500 Subject: [PATCH 07/14] docs: say the AD hop refuses referrals, and index ADR 0180 Amendments E and F The Unreleased BACKLOG #2494 CHANGELOG entry still said a followed referral gets a plain ldap3 context. Since BACKLOG #2530 on this branch the engine refuses referrals, so the entry now says so and points at changelog.d/2530.security.md. The ADR index row for 0180 now names Amendments E and F. --- CHANGELOG.md | 5 +++-- docs/adr/README.md | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcea269dd..3161b3ab0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1394,8 +1394,9 @@ All notable changes to MessageFoundry are documented here. The format follows has: the approved TLS 1.2 suites, the approved TLS 1.3 suites and the SHA-224-free signature schemes where the interpreter allows them, the key-exchange pin and a TLS 1.2 floor. ldap3's own host name check still runs after the handshake. `ciphers=` is no longer passed to ldap3. - Python 3.14 is unchanged on the wire: its TLS 1.3 gap is recorded, not closed. A followed - referral still gets a plain ldap3 context. (`BACKLOG #2494`, owner ruling R3 of + Python 3.14 is unchanged on the wire: its TLS 1.3 gap is recorded, not closed. The engine + refuses LDAP referrals, so no referred hop is opened; see `changelog.d/2530.security.md`. + (`BACKLOG #2494`, owner ruling R3 of 2026-09-27, ADR 0188 amendment of 2026-09-30) - **On Python 3.15 the engine stops offering SHA-224 TLS signature schemes.** Every context the engine narrows drops `rsa_pkcs1_sha224`, `ecdsa_sha224` and `dsa_sha224` through diff --git a/docs/adr/README.md b/docs/adr/README.md index a6b3bbd93..4920e44eb 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-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; 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.* | From 02887616e048b82ffbdad774eaefb6fcfd27858a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:36:32 -0500 Subject: [PATCH 08/14] docs(changelog): fragment for the Linux AD receive-timeout fix The fix in cd9691aaa8 had a CHANGELOG.md edit for the referral entry but no fragment of its own for the struct.error defect. Slug name because the defect has no backlog item. Part of the BACKLOG #2530 pull request. --- changelog.d/ldap3-receive-timeout-linux.fixed.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog.d/ldap3-receive-timeout-linux.fixed.md diff --git a/changelog.d/ldap3-receive-timeout-linux.fixed.md b/changelog.d/ldap3-receive-timeout-linux.fixed.md new file mode 100644 index 000000000..8f74380be --- /dev/null +++ b/changelog.d/ldap3-receive-timeout-linux.fixed.md @@ -0,0 +1,7 @@ +- **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` before any + byte reached the domain controller. 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 and never 0, which ldap3 reads as no + timeout. From 41ddf96058aeab18a2a01b6622f17b3d23bfe3b5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:37:57 -0500 Subject: [PATCH 09/14] docs(changelog): name the receive-timeout fragment for its backlog item Renames changelog.d/ldap3-receive-timeout-linux.fixed.md to 2546.fixed.md and cites BACKLOG #2546, the vault item for the Linux struct.error defect (filed on vault PR 2130, not yet on vault main). --- .../{ldap3-receive-timeout-linux.fixed.md => 2546.fixed.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename changelog.d/{ldap3-receive-timeout-linux.fixed.md => 2546.fixed.md} (95%) diff --git a/changelog.d/ldap3-receive-timeout-linux.fixed.md b/changelog.d/2546.fixed.md similarity index 95% rename from changelog.d/ldap3-receive-timeout-linux.fixed.md rename to changelog.d/2546.fixed.md index 8f74380be..1c213dbcd 100644 --- a/changelog.d/ldap3-receive-timeout-linux.fixed.md +++ b/changelog.d/2546.fixed.md @@ -4,4 +4,4 @@ byte reached the domain controller. 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 and never 0, which ldap3 reads as no - timeout. + timeout. (`BACKLOG #2546`) From aa0911445a608b2d11fa50e11fe53e592da9eaa2 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:45:40 -0500 Subject: [PATCH 10/14] fix(auth): cap the AD timeouts at 3600 s and require the receive-timeout wrapper at every Connection A huge finite ad_connect_timeout or ad_receive_timeout overflowed socket.settimeout with OverflowError, which no LdapError handler catches, so sign-in would have failed outside the audited path. The settings validator now refuses anything above 3600 s at config load. The static guard now requires every ldap3.Connection receive_timeout to be a call to _ldap3_receive_timeout, with a control test. The test receive timeout is now 11.25 so its ceiling differs from the default 10. The _ldap3_receive_timeout docstring now says ceil lengthens the timeout on every OS, because ldap3 calls settimeout first. CONFIGURATION.md states the cap and the rounding. Review findings on the BACKLOG #2530 / #2546 pull request. --- docs/CONFIGURATION.md | 4 +-- messagefoundry/auth/ldap.py | 8 +++-- messagefoundry/config/settings.py | 12 +++++++ tests/test_ldap_timeouts.py | 53 +++++++++++++++++++++++++++++-- 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 4f46181d9..d6b5c626d 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 LDAP response read** (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/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index 589c9e10d..2ceb52a1e 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -143,9 +143,11 @@ def _ldap3_receive_timeout(seconds: float) -> int: byte reached the domain controller: 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: the result is never shorter than the operator configured and never - zero, which ldap3 reads as "wait forever" (ASVS 13.1.3). The POSIX branch drops sub-second - precision anyway (``tv_usec`` is always 0), so ``ceil`` loses nothing that branch could keep. + 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) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 150540ff6..c1cab9be4 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.""" @@ -3114,6 +3118,14 @@ def _check_ad_timeout(cls, value: float) -> float: "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)" ) + # A huge finite value overflows socket.settimeout / setsockopt (and ldap3's POSIX + # struct.pack) with OverflowError or struct.error. Those are not ldap3 errors, so they would + # skip the LdapError mapping and the auth.login_error audit on every sign-in. + 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}); a larger value overflows the socket timeout" + ) return value @field_validator("ad_session_recheck_seconds") diff --git a/tests/test_ldap_timeouts.py b/tests/test_ldap_timeouts.py index 0afda9b50..350ca16a4 100644 --- a/tests/test_ldap_timeouts.py +++ b/tests/test_ldap_timeouts.py @@ -49,7 +49,9 @@ from messagefoundry.config.settings import AuthSettings _CONNECT_TIMEOUT = 7.5 # deliberately not the default, so a hardcoded literal cannot pass -_RECEIVE_TIMEOUT = 9.25 # fractional on purpose: ldap3 needs an int on POSIX (see below) +# 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: @@ -215,7 +217,6 @@ def _assert_all_finite(rec: _Recorder) -> None: f"ldap3.Connection #{i} receive_timeout {value!r} is not [auth].ad_receive_timeout " "rounded up to whole seconds" ) - struct.pack("LL", value, 0) # ldap3's own POSIX call; raises struct.error on a float # --- runtime guard ------------------------------------------------------------------------------ @@ -507,6 +508,43 @@ 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 = [ + 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) + ] + 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 _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.""" + raw = ast.parse("self._s.ad_receive_timeout", mode="eval").body + wrapped = ast.parse("_ldap3_receive_timeout(self._s.ad_receive_timeout)", mode="eval").body + assert not _is_receive_timeout_call(raw) + assert _is_receive_timeout_call(wrapped) def test_static_walker_sees_the_bare_import_call_form() -> None: @@ -578,6 +616,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 ---------------------- From e8a539c9cf2773c2d35e9efbf505cdd931495218 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:46:01 -0500 Subject: [PATCH 11/14] docs: move the referral correction into fragments and fix the ADR 0180 index row Reverts the direct CHANGELOG.md edit from c947b5cc6f, because changelog.d/README.md forbids editing CHANGELOG.md in a pull request. The BACKLOG #2530 fragment now says it supersedes the #2494 entry's followed-referral note, in words rather than a fragment path. The #2546 fragment states the 3600 s cap and drops the claim that ldap3 reads 0 as no timeout. The ADR 0180 index row gains Amendment B (BACKLOG #2034) and drops Amendment C's admitted ciphers= value, which Amendment E retired. --- CHANGELOG.md | 5 ++--- changelog.d/2530.security.md | 4 +++- changelog.d/2546.fixed.md | 5 +++-- docs/adr/README.md | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3161b3ab0..dcea269dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1394,9 +1394,8 @@ All notable changes to MessageFoundry are documented here. The format follows has: the approved TLS 1.2 suites, the approved TLS 1.3 suites and the SHA-224-free signature schemes where the interpreter allows them, the key-exchange pin and a TLS 1.2 floor. ldap3's own host name check still runs after the handshake. `ciphers=` is no longer passed to ldap3. - Python 3.14 is unchanged on the wire: its TLS 1.3 gap is recorded, not closed. The engine - refuses LDAP referrals, so no referred hop is opened; see `changelog.d/2530.security.md`. - (`BACKLOG #2494`, owner ruling R3 of + Python 3.14 is unchanged on the wire: its TLS 1.3 gap is recorded, not closed. A followed + referral still gets a plain ldap3 context. (`BACKLOG #2494`, owner ruling R3 of 2026-09-27, ADR 0188 amendment of 2026-09-30) - **On Python 3.15 the engine stops offering SHA-224 TLS signature schemes.** Every context the engine narrows drops `rsa_pkcs1_sha224`, `ecdsa_sha224` and `dsa_sha224` through diff --git a/changelog.d/2530.security.md b/changelog.d/2530.security.md index 6b424a48b..aaf3f5ae4 100644 --- a/changelog.d/2530.security.md +++ b/changelog.d/2530.security.md @@ -7,5 +7,7 @@ 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. See [ADR 0180](../docs/adr/0180-asserting-tls-suites-on-a-library-that-exposes-no-sslcontext.md) + controller's own domain. This supersedes the note in the `BACKLOG #2494` TLS-context entry that a + followed referral still gets a plain ldap3 context: 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 index 1c213dbcd..e3e383ee3 100644 --- a/changelog.d/2546.fixed.md +++ b/changelog.d/2546.fixed.md @@ -3,5 +3,6 @@ that value as an integer, so each AD socket open would have raised `struct.error` before any byte reached the domain controller. 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 and never 0, which ldap3 reads as no - timeout. (`BACKLOG #2546`) + whole seconds, so it is never shorter than configured. Both AD timeouts are now also refused at + config load above 3600 seconds, because a larger value overflows the socket timeout and would + have failed every sign-in outside the audited error path. (`BACKLOG #2546`) diff --git a/docs/adr/README.md b/docs/adr/README.md index 4920e44eb..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; 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 | +| [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.* | From a02b0e64fb01494244fca68b33b276cc710ebc28 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 30 Sep 2026 23:46:46 -0500 Subject: [PATCH 12/14] docs(auth): correct the validator's 0-timeout comment and state the int reason once The validator comment said ldap3 reads 0 as wait-forever for both timeouts; that holds only for connect_timeout, while a 0 receive_timeout makes the socket non-blocking. The test now points at the _ldap3_receive_timeout docstring instead of restating the struct.pack reason. Round-one review findings on the BACKLOG #2530 / #2546 pull request. --- messagefoundry/config/settings.py | 7 ++++--- tests/test_ldap_timeouts.py | 7 +++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index c1cab9be4..22d85edec 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -3110,9 +3110,10 @@ 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 " diff --git a/tests/test_ldap_timeouts.py b/tests/test_ldap_timeouts.py index 350ca16a4..5e249e64b 100644 --- a/tests/test_ldap_timeouts.py +++ b/tests/test_ldap_timeouts.py @@ -205,10 +205,9 @@ 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}" ) - # An INT, not merely a number: ldap3 packs it with struct.pack('LL', value, 0) on every - # non-Windows host, and a float raises struct.error there before the socket is used. 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. + # 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" From f3f86401dc50dea10cf90f016deb4ba603807547 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 1 Oct 2026 00:04:49 -0500 Subject: [PATCH 13/14] fix(auth): correct the AD timeout refusal reasons and make the receive-timeout guard's control drive the guard Round-two review findings. The over-3600 message no longer claims a larger value overflows (overflow starts near 3e6 s; the cap sits far below it). The <=0 message gives the real reasons. The docstring and the BACKLOG #2546 fragment say struct.error fired after the TCP connect, not before any byte. The static guard's filter is now a function its control test runs on a raw and a wrapped snippet, and the walker sanity check requires all three Connection sites. CONFIGURATION.md says the receive timeout bounds each socket receive. The #2530 fragment sentence is split under 25 words. --- changelog.d/2530.security.md | 6 ++--- changelog.d/2546.fixed.md | 10 +++---- docs/CONFIGURATION.md | 2 +- messagefoundry/auth/ldap.py | 4 +-- messagefoundry/config/settings.py | 12 +++++---- tests/test_ldap_timeouts.py | 43 +++++++++++++++++++++---------- 6 files changed, 47 insertions(+), 30 deletions(-) diff --git a/changelog.d/2530.security.md b/changelog.d/2530.security.md index aaf3f5ae4..4f9df3ce9 100644 --- a/changelog.d/2530.security.md +++ b/changelog.d/2530.security.md @@ -7,7 +7,7 @@ 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. This supersedes the note in the `BACKLOG #2494` TLS-context entry that a - followed referral still gets a plain ldap3 context: 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) + 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 index e3e383ee3..025d16ab1 100644 --- a/changelog.d/2546.fixed.md +++ b/changelog.d/2546.fixed.md @@ -1,8 +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` before any - byte reached the domain controller. On a first Linux deployment, AD sign-in would have failed + 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 now also refused at - config load above 3600 seconds, because a larger value overflows the socket timeout and would - have failed every sign-in outside the audited error path. (`BACKLOG #2546`) + 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 d6b5c626d..b51f45691 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -666,7 +666,7 @@ document ([SECURITY-DOCS-POLICY.md](SECURITY-DOCS-POLICY.md)). | `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, 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 LDAP response read** (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_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/messagefoundry/auth/ldap.py b/messagefoundry/auth/ldap.py index 2ceb52a1e..4e99b0788 100644 --- a/messagefoundry/auth/ldap.py +++ b/messagefoundry/auth/ldap.py @@ -139,8 +139,8 @@ def _ldap3_receive_timeout(seconds: float) -> int: 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 before a single - byte reached the domain controller: AD sign-in could not work there at all. Windows hides it, + 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 diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 22d85edec..a9a10b402 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -3117,15 +3117,17 @@ def _check_ad_timeout(cls, value: float) -> float: 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 (and ldap3's POSIX - # struct.pack) with OverflowError or struct.error. Those are not ldap3 errors, so they would - # skip the LdapError mapping and the auth.login_error audit on every sign-in. + # 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}); a larger value overflows the socket timeout" + f"seconds (got {value:g}); the cap keeps the value far below where a socket " + "timeout overflows" ) return value diff --git a/tests/test_ldap_timeouts.py b/tests/test_ldap_timeouts.py index 5e249e64b..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 @@ -479,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), ( @@ -495,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 @@ -510,17 +511,23 @@ def test_every_ldap3_construction_site_passes_a_timeout() -> None: # 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 = _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) ] - 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 _is_receive_timeout_call(node: ast.expr) -> bool: @@ -540,10 +547,18 @@ def _is_receive_timeout_call(node: ast.expr) -> bool: 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.""" - raw = ast.parse("self._s.ad_receive_timeout", mode="eval").body - wrapped = ast.parse("_ldap3_receive_timeout(self._s.ad_receive_timeout)", mode="eval").body - assert not _is_receive_timeout_call(raw) - assert _is_receive_timeout_call(wrapped) + + 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: From 21c948df73f3e6782598baa091a9219854e8541a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 1 Oct 2026 02:51:45 -0500 Subject: [PATCH 14/14] test(auth): guard the ldap3 referral construction-site walk against an empty walk The vacuous-absence lint (tests/test_vacuous_absence_assert_lint.py), which arrived on main, flags test_every_ldap3_construction_site_turns_referrals_off on `wrong`: the `not wrong` check walks `sites`, and the existing per-kind count assertion reads `kinds`, not `sites`, so the lint cannot see it. Assert the walk found at least four sites (three ldap3.Connection and one ldap3.Server) before the absence check. Also merges origin/main in. --- tests/test_ldap_referrals.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_ldap_referrals.py b/tests/test_ldap_referrals.py index 0a8252495..a29d44d8b 100644 --- a/tests/test_ldap_referrals.py +++ b/tests/test_ldap_referrals.py @@ -398,6 +398,8 @@ def test_every_ldap3_construction_site_turns_referrals_off() -> None: ``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