mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
main
23 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
f4fd7e1f67 |
fix(security): classify outbound addresses by what they reach (GHSA-wm4f-79pw-pfr9) (#1003)
* fix(security): classify outbound addresses by what they reach (GHSA-wm4f-79pw-pfr9)
Five guards screened outbound URLs and each hand-rolled its own address
normalization and policy tests, so each had a different hole. An IPv6
transition address carries an IPv4 destination in its low bits and
`ipaddress` classifies the wrapper, not the destination: 64:ff9b::a9fe:a9fe
reports is_global because 64:ff9b::/96 is global unicast, while a NAT64
gateway routes it to the cloud metadata endpoint. CGNAT (100.64.0.0/10) is
neither is_private nor is_global, so a denylist built on is_private missed
it with no gateway involved at all.
Add turnstone/core/ip_classify.py as the single classifier. One function
returns exactly one policy lane — PUBLIC, PRIVATE (operator-approvable) or
NEVER — and every guard branches on the lane rather than re-deriving it.
Two overlapping booleans would make a verdict depend on which one a caller
tested first; several addresses are simultaneously globally routable and
metadata-reaching.
- Decode transition addresses per RFC 6052 §2.2 (NAT64 well-known and
local-use prefixes, 6to4, Teredo, IPv4-mapped, IPv4-compatible) and judge
them by the IPv4 they reach. The local-use prefix does not say which
layout its gateway uses, so every length it can carry is decoded and the
worst result classified.
- Share hostname resolution too. The five copies had already drifted on
which failures they caught, and getaddrinfo raises UnicodeError — not an
OSError — from the IDNA encoder.
- Resolution failure is a refusal, not a pass: the fetch resolves again, so
an authority answering the guard with SERVFAIL and the fetch with an
internal address would otherwise switch the guard off for that hop.
- Screen every redirect hop in every mode. allow_private_origin widens which
lanes are acceptable rather than turning screening off, and the permission
is revoked after any hop that is not wholly private.
- Cleartext http is allowed only for a hostname that RESOLVES to loopback.
*.localhost is ordinary DNS, and trusting the name put an OIDC token
exchange on the wire in the clear.
- Screen doctor and console-probe URLs through the classifier. Both used a
host.startswith("169.254.") string test that never resolved, so any DNS
name pointing at the metadata service passed and its body was returned to
the model.
- Add known vendor metadata prefixes the stdlib does not flag, and place
deprecated IPv6 site-local outside the public lane.
The operator's private-network opt-in still admits the whole home lab,
including IPv6 loopback, CGNAT and split-horizon hosts. Metadata,
link-local, multicast, unspecified and reserved addresses stay refused
regardless of the opt-in, including as a redirect target from an approved
private origin — the settings help and docs now say so.
Reported by @tonghuaroot.
* fix(security): close Azure/Oracle metadata gap and restore dual-stack origins
Review follow-ups on the address-classification rework.
Azure's host-agent endpoint (168.63.129.16) and Oracle Cloud's metadata
endpoint (192.0.0.192) sit in ordinary unicast space, so the stdlib reported
them as globally routable and both classified PUBLIC — reachable with no
opt-in at all, a worse position than the RFC 1918 host beside them, and
directly contradicting the "metadata stays refused even with the opt-in"
guarantee the settings help and docs now advertise. Both join the shared
vendor list.
Revoking the private-hop permission on the ORIGIN hop broke the case
`_screen_tool_url` deliberately admits: a dual-stack or split-horizon
home-lab host answering with both a LAN and a public record was approved,
then refused on its own `302 /login` — one hop was all it ever got. Track
the approved HOST instead, so redirects that stay on it remain covered while
a redirect to any other private host is still refused once the chain is no
longer wholly private.
Also:
- Try several registry candidates for the collector-scope probe instead of
abandoning it when the first is unresolvable, which also stopped a healthy
registry from logging as malformed.
- Bound the probe's name resolution with an explicit timeout matching the
2s the httpx connect deadline used to provide; it runs before the console
lifespan yields and getaddrinfo has no timeout of its own.
- Route doctor and the console probe through `web.screen_url` rather than
keeping a third and fourth copy of parse/resolve/classify/fold, which had
already diverged on default port and empty-hostname wording. An empty
hostname no longer reports as a cloud-metadata refusal.
- Give `screen_url` a scheme-aware default port.
- Stop doubling the word "hostname" in the OAuth resolution refusal.
- Correct the `_screen_tool_url` docstring: it described `private_origin` as
requiring every record to be private, which the mixed-record decision
reversed, and `private_block` as a property of a refusal when it reports
the lane on the success path too.
- Make the preview tests' screening stub opt-in rather than autouse — as a
module-wide fixture it also stubbed the tests whose subject IS the screen,
so one of them would have passed even if screening refused everything.
Verified the module now passes with all name resolution blocked.
* fix(security): refuse mixed-record private origins instead of exempting them
The previous commit let an approved private origin redirect to itself by
exempting its hostname from the chain-wide revocation. That exemption was
wrong three ways: it was captured once and never cleared, so a public hop
could steer the fetcher back into the approved host at a path of its
choosing — reopening the private -> public -> private bypass; it was
re-entrant across same-host redirects with fresh DNS each time, so a
self-redirecting host could walk arbitrary internal addresses; and it
matched on bare hostname, so it spanned every port on the approved box.
All three were reproduced against the parent commit, which refuses them.
Delete the exemption rather than repair it. The case it existed for — a
dual-stack host answering with both a LAN and a public record — is now
refused where it is actually decidable, in `_screen_tool_url`, with the
remedy in the message: point the tool at the LAN address directly. A
granted chain therefore always starts wholly private, so the fetch guard
needs no notion of an approved host and stays one unconditional rule.
That the accommodation could not be expressed safely in the guard is the
signal: the connection may land on either record, so approving such a host
never described where the fetch would go.
Also from the same review:
- Walk the whole service registry for a collector-scope probe candidate
instead of the first three, and split the outcome into three log lines,
so entries that are merely unreachable stop raising the malformed-registry
alarm and skipping the boot check cluster-wide.
- Stop the candidate walk on a resolver timeout. `asyncio.timeout` bounds
the await, not the work, so continuing left one parked thread per timed-out
candidate on the shared executor.
- Move the metadata-hostname denylist into `ip_classify` and enforce it in
`screen_url`, so doctor and the console probe inherit it instead of each
keeping a copy.
- Drop the scheme-aware default port: a numeric service does not change
which addresses resolution returns, and classification reads only those.
`parsed.port` is still touched so an out-of-range value refuses.
- Correct the vendor-metadata comment, which generalized a claim true of
Azure's and Oracle's addresses to Alibaba's CGNAT one.
- Rename a test class that was still named for the rule it no longer tests.
|
||
|
Patrick Buckley
|
9391509e85 |
fix(oidc): trust Entra's graph.microsoft.com userinfo out of the box
Microsoft Entra's discovery document advertises userinfo_endpoint on graph.microsoft.com — a host distinct from the login.microsoftonline.com issuer — so discover_oidc's cross-host guard rejected it and disabled OIDC unless the operator set trusted_endpoint_hosts. Add login.microsoftonline.com to the built-in KNOWN_TRUSTED_OAUTH_ENDPOINT_HOSTS allow-list (mirroring the Google entry) so Azure AD OIDC works with no extra configuration. Surfaced by the live obo integration test. |
||
|
Patrick Buckley
|
ec079f0df3 |
fix(oidc/console): unblock obo edits when OIDC off; latch config-invalid rediscovery
Round-8 review follow-up — two correctness follow-ons from the round-7 rediscovery/console-gate fixes, plus two cleanups. - The console obo write gate ran the OIDC-deployment checks on EVERY update, so once OIDC was operator-disabled any edit of an existing oauth_obo server — including the natural remedy of setting enabled=false — was rejected 400, leaving DELETE as the only way out. The deployment-level checks (encryption key, OIDC enabled/configured, capture opt-in, valid grant profile) now run only when a write is a NEW obo enablement (create or flip INTO obo); a same-type edit keeps only the per-server validity checks (audience required, entra-scope reject), so an operator can always disable or edit an existing obo server. - Probing rediscovery with enabled forced True carried the retryable boot flag into discover_oidc, whose config-error branches returned enabled= False without clearing it, so a config-invalid IdP (an endpoint failing SSRF/same-origin validation) re-probed every 60s forever. The config- error branches now latch discovery_retryable=False (terminal), and maybe_rediscover installs that terminal config so the node stops probing; the transient fetch/degraded branches keep retrying. Cleanups: fold the obo missing-expires_in fallback into _expires_at_from_response via a default_ttl_seconds param (one owner of the stored-expiry format), and drop the redundant audience-change inequality already guaranteed by the no-op normalization (matching the sibling scopes_changing). |
||
|
Patrick Buckley
|
af56170be6 |
fix(oidc/mcp): make runtime OIDC rediscovery actually work; preserve oauth_user paths
Round-7 review follow-up. - The runtime OIDC re-discovery feature was dead code: discover_oidc PRESERVES the input config's `enabled` flag on success (only load_oidc_config ever sets it True), and maybe_rediscover_oidc always probed from the disabled boot config, so a successful rediscovery still returned enabled=False and the config swap was unreachable — the whole boot-outage auto-heal never worked. It now probes with enabled forced on so the flag is a reliable success signal. The unit test that "covered" this was mocking discover_oidc to return enabled=True, masking the bug; it now drives the real discover_oidc through a mocked HTTP discovery GET. - The console never runs runtime rediscovery, so a transient discovery failure at console boot made every oauth_obo server un-editable and un-disable-able. The write gate now accepts a discovery_retryable config (OIDC configured, discovery transiently down) and rejects only a genuinely absent OIDC. - The first rediscovery probe was suppressed for ~60s after host boot because the "last probe" timestamp defaulted to 0.0; it now uses a None sentinel for "never probed". - Two behavior-preservation fixes for the pre-existing oauth_user path: the shared hardened token-POST no longer escalates oauth_user oversized error bodies (that status-based classification is opt-in for the obo legs only), and the token_revoked audit fires unconditionally for oauth_user again (a refresh failure means a real grant died) while staying delete-gated for obo to avoid revocation rows for tokens that never existed. Cleanups: drop a throwaway set allocation in the pool-emptiness check, compute the create handler's cleaned OAuth text once, remove a dead no-op pop with a false comment, and simplify the cleared-map prune to two non-overlapping passes. |
||
|
Patrick Buckley
|
32c76499fa |
fix(mcp): harden obo mint path and admin lifecycle after review
Mint engine: guard the credential-rotation persist so a storage blip cannot escape the classified-result contract mid-mint (and cannot brick the user's other obo servers on strict-rotation IdPs); stop borrowing the login flow's httpx client across event loops — mints use a transient per-request client (obo_http_client remains as a test seam); retry OIDC discovery at runtime (cooldown-gated, single-flight) so a node that booted during an IdP outage can mint again without a restart; key the under-lock force-refresh reuse gate on created, which delete+create makes the mint time (obo rows never set last_refreshed, so the copied oauth_user gate never fired and serialized waiters each re-redeemed). Cross-node consent badges: the cleared-pairs set becomes a TTL map with bounded growth, so a badge written by another node after this node's last clear self-heals within one TTL window instead of surviving until a restart. Admin lifecycle: purge the mint cache when oauth_scopes changes on an obo row (an rfc8693 privilege reduction now applies immediately, like audience changes); normalize no-op scope/audience re-sends out of updates — the admin form re-submits pre-filled fields on every save, which both re-triggered purges and made entra-profile rows with legacy scopes un-editable; make flip-into-obo scope handling grant-profile aware (entra clears the carry-over, rfc8693 honors the request); clear obo-era audience/scopes when flipping back to oauth_user (the IdP-side app identifier is not a resource indicator); mirror the same column policy in the create handler. Revocation honesty: hide obo mint-cache rows from the user connections list and refuse the per-server disconnect with 409 — deleting the row returned 204, audited token_revoked, and then session-start priming silently re-minted from the surviving captured credential. Console form: keep the audience-from-URL autofill off for sign-in passthrough (the audience there is an IdP application identifier, and the prefilled URL passed every validation layer then failed every mint); clear the autofill artifact when switching modes; omit unchanged scopes from submissions. Dispatchers: route tool/resource/prompt through one shared lookup-error mapping and an auth-model-aware 401-exhausted detail (obo users are no longer pointed at a consent flow that does not exist). The consent-url audit count drops 13 → 7: the three per-dispatcher mapping copies collapsed into _pool_lookup_error. Priming: skip all obo servers for users with no captured credential via one existence SELECT (previously three reads per server per session). Also: USER_SCOPED_AUTH_TYPES now lives in storage._protocol so the backend SQL predicates share the application layer's set; docs describe the actual purge-on-transition behavior (the orphan-and-reactivate claims were wrong); the entra e2e setup script no longer aborts silently under set -e with suppressed stderr. |
||
|
Patrick Buckley
|
0732f9a7d2 |
feat(mcp): capture the IdP refresh token at OIDC login (opt-in)
[oidc] capture_user_credential (default off; env TURNSTONE_OIDC_CAPTURE_USER_CREDENTIAL) persists the user's IdP refresh token - encrypted with the MCP token envelope - as the single credential oauth_obo servers will redeem on demand. - enabling the knob appends offline_access to the login scopes (idempotent when the operator already lists it) - capture runs after user provisioning and is best-effort: a capture failure logs loudly but never blocks login; the mint path surfaces a missing credential on the reconnect rail - startup hard-fails (SystemExit) when capture is enabled without a [security] token encryption key, same as the oauth_user enforcement Refs #551. |
||
|
Patrick Buckley
|
4350248d8f |
fix(oidc): carry the opt-in hint on discovered-endpoint rejections
The discovered-endpoint wrapper converted every OAuthSSRFError to a bare OIDCError, so a private-resolving endpoint or trusted host got the non-public message without the allow_private_network remediation even though the same knob fixes it. Hoist the hint into a module constant and append it in both wrappers. Also name "unspecified" in the refused-even-with-opt-in message so 0.0.0.0/:: rejections read unambiguously. |
||
|
Patrick Buckley
|
9c74673fd4 |
feat(oidc): allow_private_network opt-in for self-hosted IdPs
The SSRF guard on OIDC endpoint URLs hard-refused any hostname resolving to a non-public address, which made it impossible to use a self-hosted IdP (Keycloak, Authentik, Dex) on an internal network — even though the login-flow issuer is operator-configured, i.e. trusted input. Add [oidc] allow_private_network in config.toml (or TURNSTONE_OIDC_ALLOW_PRIVATE_NETWORK), default off. When set, the issuer and its discovered endpoints may resolve to private-range, unique-local, CGNAT, and loopback addresses. Link-local, multicast, unspecified, and reserved ranges stay refused regardless — cloud metadata services live on link-local and no legitimate IdP does. The HTTPS requirement and same-origin endpoint checks are unchanged. The private-address refusal now raises OAuthSSRFPrivateAddressError, and the OIDC wrapper appends the remediation hint to the error message so the failure is self-service. mcp_oauth call sites — where endpoint URLs come from untrusted remote-server metadata — do not get the knob and keep the strict public-address rule. |
||
|
metaclassing
|
deff44bcea |
Addendum to Entra ID's... proclivities (#772)
* OIDC entra capture * copilot being nitpicky --------- Co-authored-by: pow3rtool <root@pow3rtools> |
||
|
Patrick Buckley
|
b2153d907f |
fix(oidc): close transient client on disable paths + correct docstring
PR #476 review feedback (Copilot, oidc.py:584,616): 1. initialize_oidc_state's docstring claimed "on any failure enabled is False" but the JWKS-prefetch failure branch intentionally keeps enabled=True so the callback's lazy-fetch retry can recover from a transient IdP issue at startup. Docstring rewritten to spell out the three post-conditions: disable, JWKS-failure-keeps-enabled, success. 2. The long-lived httpx.AsyncClient was created up front, then three disable branches (discovery exception, discovery-returned- disabled, missing redirect_base) returned without closing it, leaving sockets held until shutdown. Restructured: discovery now uses a transient AsyncClient inside a context manager (closed at exit). The long-lived client is only created after the disable checks pass. The JWKS-failure branch still legitimately keeps the client open because the lazy-retry path needs it. The pre-existing single-client-passthrough test was replaced with three more specific tests: long-lived client only goes to fetch_jwks (not discover_oidc); discovery-exception path leaves http_client=None; missing-redirect_base path leaves http_client=None. |
||
|
Patrick Buckley
|
5d4a50d2cd |
chore(oidc): consolidate test OIDCConfig helper + fix exceptions banner (cumulative q-4, q-5)
q-4: tests/test_oidc.py's _make_config and tests/test_oidc_handlers.py's _make_oidc_config built the same OIDCConfig with sensible defaults but had drifted — only the handlers helper set redirect_base. After b3 made redirect_base operationally required, every test_oidc.py test that exercised redirect_base had to override it explicitly. A future test could omit redirect_base and silently exercise the wrong production path. Moves make_oidc_test_config to tests/conftest.py with the more complete handler-version defaults (including redirect_base). Both test files import it under their existing local alias (_make_config / _make_oidc_config) so the 60+ call sites in test_oidc.py and the handler tests don't have to change. q-5: section banner '# Exception' (singular) at oidc.py:79 became inconsistent after b5 (callback robustness) added OIDCKeyNotFoundError. Renamed to '# Exceptions'. |
||
|
Patrick Buckley
|
1c41212f15 |
fix(oidc): self-heal stranded user when role mapping fails post-create (cumulative bug-1)
If apply_role_mapping raised after create_oidc_user committed (transient storage failure, race with role deletion, etc.), provision_oidc_user's inline safety-net was skipped — and on retry the existing-identity branch never reached the safety-net code, leaving the user permanently stranded with zero roles. Extracts _ensure_default_role(storage, user_id, desired_role_ids=None) helper. Calls it on BOTH the new-user and existing-identity paths so a user stranded by a transient failure recovers on next login. desired_role_ids is a hint that lets the helper skip list_user_roles when claim-driven mapping populated at least one role; the new-user path was already paying that query, the existing-identity path now pays it only when claim mapping returned an empty desired set. Documents the admin-strip behavior in the helper docstring: stripping all roles from an OIDC user no longer locks them out, since the next login will re-grant builtin-viewer (assigned_by='oidc-default'). The documented way to deny an OIDC user is to unlink their OIDC identity via the admin endpoint, not to strip roles. The pre-fix behavior (stripped user actually locked out) was the bug. The 'oidc-default' vs 'oidc' assigned_by distinction is preserved: apply_role_mapping's revocation lane only touches 'oidc' rows, so the safety-net role survives every subsequent login regardless of claims. Six new tests cover both paths, the hint short-circuit, the list_user_roles fallback, the missing-builtin-viewer no-op, and the self-heal regression case for already-stranded users. |
||
|
Patrick Buckley
|
5c11ab985f |
test(oidc): close coverage gaps + tighten fetch_jwks shape check (q-5, q-8)
q-5: _derive_username's UUID-retry tier (oidc.py:923-933) was untested.
After perf-6 collapsed tier-2 to a single find_existing_usernames call,
the only remaining tail was the 3-attempt UUID-retry loop and the final
raise. New TestDeriveUsername class covers:
- falls into UUID retry when all 10 suffix candidates are taken
- UUID retry succeeds on the second attempt after one collision
- UUID retry exhausted -> raises OIDCError
q-8: filled the unit-level coverage holes the multi-stage review flagged:
- test_validate_id_token_retry_after_kid_rotation — direct unit test of
the OIDCKeyNotFoundError path with real RS256 keys + JWKS rotation
(previously only exercised end-to-end through the handler).
- test_callback_uses_pending_audience_not_handler_audience — pins down
the bug-3 fix by decoding the issued JWT cookie and asserting aud
matches the audience stored at /authorize time, not the handler param.
- test_apply_role_mapping_int_claim / _dict_claim — exercises the
else: values = [str(claim_value)] branch for non-string non-list
claim shapes.
- TestFetchJWKS — non-200 status, non-dict body, dict-missing-keys,
keys-not-list, transport network error.
- TestExchangeCode network/4xx/5xx error tests (the non-dict-body case
already shipped in batch 5).
Also a small production hardening that fell out of writing the
TestFetchJWKS::test_fetch_jwks_non_dict_body_raises test: fetch_jwks now
guards isinstance(result, dict) before result.get("keys"), matching the
shape-check pattern that discover_oidc and exchange_code already use.
A list/null body now surfaces as OIDCError("...not a JSON object") rather
than AttributeError leaking up to the lifespan.
|
||
|
Patrick Buckley
|
bae4adca12 |
refactor(oidc): quality cleanup (bug-3, q-1/3/4/6/7/9/10/11/12/13)
Eleven small maintenance fixes; no behavior change beyond bug-3.
bug-3: pending.get('audience', audience) couldn't fall back because
pop_oidc_pending_state always returns a dict with the audience key
set verbatim from a non-null TEXT column. Replaced with
pending.get('audience') or audience to cover the empty-string case
defensively. Comment explains the security rationale.
q-1: extract _env_or_cfg_str / _env_or_cfg_bool helpers in oidc.py;
load_oidc_config's six near-identical env-or-config blocks collapse
to one-liners. role_map / trusted_endpoint_hosts / redirect_base
retain bespoke parsing.
q-3: discover_oidc narrows except (httpx.HTTPError, ValueError, KeyError)
with exc_info=True.
q-4: OIDC_STATE_TTL_SECONDS = 300 constant in oidc.py; auth.py imports
and passes it explicitly. Storage signatures keep the literal default
(storage layer doesn't know OIDC TTL semantics).
q-6: hoist runtime imports (OIDCError, OIDCKeyNotFoundError, exchange_code,
fetch_jwks, provision_oidc_user, validate_id_token, build_authorize_url,
generate_pkce_verifier) to module scope in auth.py. The genuine cycle
is only oidc._derive_username -> auth.is_valid_username, kept
function-scoped. test_oidc_handlers.py mock targets repointed to
turnstone.core.auth.X to match the new binding.
q-7: comment + docs explain the 'oidc' vs 'oidc-default' assigned_by
marker distinction.
q-9: OIDCIdentity / OIDCPendingState TypedDicts in storage protocol.
Implementations construct via TypedDict syntax so mypy structurally
verifies all required fields.
q-10: fetch_jwks narrows except (httpx.HTTPError, ValueError); docstring
matches.
q-11: rename generate_pkce_pair -> generate_pkce_verifier; return only
the verifier (build_authorize_url already recomputes the challenge).
q-12: extract _buildOidcRow helper in admin.js so future field additions
go in one place.
q-13: OIDCConfig docstring lists startup-config vs discovery-derived
field groups.
|
||
|
Patrick Buckley
|
39a647f39c |
perf(oidc): batch perf hardening (perf-1..8)
Eight independent perf wins on the OIDC hot path: perf-1: list_users() full-scan setup-gate replaced with new count_users() on both authorize and callback. Saves a full users-table fetch per login. perf-2: handle_oidc_callback's sync DB chain wrapped in asyncio.to_thread for cleanup, pop_oidc_pending_state, count_users, and provision_oidc_user. handle_oidc_authorize gets the same treatment for count_users and create_oidc_pending_state. Event loop no longer blocks for the full callback duration on Postgres deployments. perf-3: apply_role_mapping N+1 collapsed via new replace_oidc_roles storage method. One transaction handles the diff + insert + delete instead of 2N+1 commits per login. Returns (added, removed) so the caller can still emit per-role audit logs. The diff respects the documented invariant "manually-assigned roles are never touched" — desired_role_ids is filtered against rows where assigned_by != 'oidc' before computing added/removed. This prevents a PK conflict (Postgres lockout) or silent OR-IGNORE no-op (SQLite lying return) when admin-ui or oidc-default already holds the same role_id. perf-4: provision_oidc_user no longer re-queries list_user_roles after apply_role_mapping. The new-user builtin-viewer fallback is gated on desired_role_ids being empty, which is information apply_role_mapping already returned. perf-5: JWKS refetch dedup via asyncio.Lock on app.state. Both lazy-fetch (cold-start recovery) and rotation paths share the same lock with a double-check pattern: re-resolve kid against the current cache before issuing a new GET. N concurrent callbacks during rotation now produce at most 1 fetch. perf-6: _derive_username's 9-suffix loop collapsed via new find_existing_usernames(candidates) -> set query. Worst case drops from 13 sequential queries to 1 + up-to-3 UUID-retry queries. perf-7: cleanup_expired_oidc_states gated to once-per-60s per process via app.state.oidc_last_cleanup_monotonic. The pop already deletes the consumed row; the bulk cleanup is only relevant for abandoned authorize flows, so frequency was overkill. perf-8: Long-lived httpx.AsyncClient stashed on app.state.oidc_http_client by initialize_oidc_state. discover_oidc/fetch_jwks/exchange_code accept an optional client= kwarg; when set, skip the per-call AsyncClient context-manager. New close_oidc_state lifespan teardown closes it. Tests pass client=None to keep the transient-client legacy path. New storage methods (sqlite + postgresql): - count_users() -> int - find_existing_usernames(candidates) -> set[str] - replace_oidc_roles(user_id, desired) -> (added, removed) |
||
|
Patrick Buckley
|
0af3adae1d |
fix(oidc): callback robustness — typed exceptions, shape checks, log sanitize, JS race (bug-4, bug-5, bug-6, sec-4)
Four small hardening fixes on the OIDC callback hot path: bug-4: JWKS rotation retry was matching the substring 'not found in JWKS' inside an OIDCError message. A future rephrasing would silently break key rotation. Adds OIDCKeyNotFoundError(OIDCError); validate_id_token raises the subclass at the kid-not-found site; handle_oidc_callback catches it explicitly. Other 'not found' errors in validate_id_token remain as plain OIDCError. bug-5: tokens['id_token'] raised KeyError if the IdP returned 200 without id_token. exchange_code now rejects non-dict response bodies; the callback validates id_token shape (must be non-empty str) before passing to validate_id_token. Both raise OIDCError, surfaced as the standard 'Authentication failed' redirect. bug-6: shared_static/auth.js — the OIDC error display raced showLogin's /v1/api/auth/status fetch via a 300ms setTimeout. showLogin now takes an optional oidcError parameter and paints it after _switchMode clears the error, in both the success and catch branches of the fetch. sec-4: oidc.py exchange_code's non-200 OIDCError interpolated up to 500 bytes of attacker-controlled IdP body, which then went to log.warning via 'OIDC callback failed: %s'. CRLF in resp.text could forge log lines. New _sanitize_log_text helper escapes control chars via unicode_escape and caps at the rendered length. |
||
|
Patrick Buckley
|
11618bb1d7 |
fix(oidc): atomic user + identity provisioning to prevent orphan rows (bug-1)
provision_oidc_user previously called create_user (INSERT OR IGNORE on SQLite — silent no-op on UNIQUE conflict), then create_oidc_identity (also INSERT OR IGNORE), then apply_role_mapping which writes user_role rows for the supposedly-new user_id. On a username TOCTOU race or concurrent (issuer, sub) double-create, both inserts no-opped but user_role rows were already written — leaving orphan rows pointing at a user_id that doesn't exist. PostgreSQL's create_user raised IntegrityError instead of silently no-opping so it produced a misleading 'Authentication failed' error without orphans, but the user-facing UX was equally poor. Adds StorageConflictError to the storage protocol and create_oidc_user that does both inserts in one transaction. Username collision and (issuer, subject) collision both raise StorageConflictError, mapped to OIDCError by provision_oidc_user. Crucially the new code does not silently bind a colliding-username new identity to the existing user — that would be an account-takeover vector. It raises. SQLite uses BEGIN IMMEDIATE inside the try block so lock-contention errors surface as StorageConflictError instead of leaking the raw sqlalchemy OperationalError. PostgreSQL relies on SQLAlchemy 2.x begin-on-demand semantics; the explicit conn.commit()/rollback() in the catch block is the only materialization path. Discrimination on PG uses exc.orig.diag.constraint_name with message-substring fallback. |
||
|
Patrick Buckley
|
52aba17740 |
fix(oidc): require TURNSTONE_OIDC_REDIRECT_BASE; drop Host-header fallback (sec-2)
_build_oidc_redirect_uri previously fell back to the request Host
header when redirect_base was unset. With a permissive reverse proxy
or direct backend access, a spoofed Host minted an authorize URL
pointing to attacker-controlled host — combined with a permissive
IdP redirect_uri allowlist this enables auth-code interception.
There is no production scenario where a Host-derived redirect_uri is
correct, so this fails closed:
- initialize_oidc_state checks redirect_base after discovery succeeds
and disables OIDC (with an explicit error log naming the env var)
if it's empty. Runs before fetch_jwks so a misconfigured deploy
doesn't make a wasted JWKS call.
- _build_oidc_redirect_uri simplifies to f"{redirect_base}/v1/api/auth/oidc/callback".
request parameter dropped; both call sites (handle_oidc_authorize,
handle_oidc_callback) updated.
- docs/oidc.md promotes TURNSTONE_OIDC_REDIRECT_BASE from "Recommended"
to "Required" with the security rationale.
|
||
|
Patrick Buckley
|
6f9e140a41 |
refactor(oidc): unify server+console lifespan via initialize_oidc_state (q-2, bug-2)
The OIDC discovery + JWKS prefetch block was duplicated byte-for-byte between turnstone/server.py and turnstone/console/server.py. The bare except branch in that block also left app.state.oidc_config unchanged on unexpected exceptions — leaving the runtime with enabled=True and empty endpoints, producing malformed authorize URLs. Extracts initialize_oidc_state(app_state) into turnstone/core/oidc.py which guarantees a coherent post-condition on every code path: - discovery exception -> oidc_config replaced with enabled=False, jwks_data=None - discovery returns enabled=False -> jwks_data=None - JWKS prefetch fails -> jwks_data=None but enabled=True preserved (the callback's lazy-fetch retry path remains the recovery) - success -> oidc_config + jwks_data both populated Also hardens discover_oidc against non-dict discovery responses (list/null/string/int) — previously these raised AttributeError out of doc.get and propagated past the lifespan's bare except. server.py and console/server.py lifespan blocks collapse to a single await initialize_oidc_state(app.state) call. |
||
|
Patrick Buckley
|
0df7dc026b |
fix(oidc): SSRF + plaintext credential exfil via discovery doc (sec-1, sec-3)
OIDC discovery-document endpoints (token_endpoint, jwks_uri, userinfo_endpoint) were stored verbatim in OIDCConfig and later passed to httpx without revalidation. Only the issuer URL was checked. A hostile or compromised IdP could return token_endpoint pointing to an internal IP (169.254.169.254, 10.0.0.0/8, etc.) and Turnstone would POST the client_secret there. Extracts the existing scheme/userinfo/SSRF check into _validate_url_no_ssrf, adds validate_discovered_endpoint that runs the same checks plus an issuer-binding check, and wires it into discover_oidc for authorization_endpoint, token_endpoint, jwks_uri, and userinfo_endpoint (when present). Issuer binding accepts: - Same (scheme, hostname, effective port) as the issuer. - A hostname in _KNOWN_TRUSTED_ENDPOINT_HOSTS for the issuer (Google's multi-origin discovery is in the allow-map by default). - A hostname in OIDCConfig.trusted_endpoint_hosts, settable via TURNSTONE_OIDC_TRUSTED_ENDPOINT_HOSTS env var or config.toml, for IdPs not in the static map. Effective port comparison treats https://host and https://host:443 as the same origin (urllib.parse.urlparse leaves the explicit form's port as 443 and the implicit form's as None). 24 new tests cover the validator, the Google known-hosts path, the operator allow-list, default-port equivalence, foreign-host rejection, private-IP rejection, embedded credentials, and DNS rotation between issuer check and endpoint use. |
||
|
Patrick Buckley
|
1f47ca62de |
fix: validate OIDC issuer URLs against SSRF before discovery fetch (#140)
* fix: validate OIDC issuer URLs against SSRF before discovery fetch Add validate_issuer_url() that rejects private/loopback/link-local IPs, non-HTTPS (except localhost for dev), embedded credentials, and unresolvable hostnames. Called before the HTTP fetch in discover_oidc() so the request is never made for invalid URLs. 17 new tests. * fix: address review — use is_global, redact userinfo, catch ValueError Use `not addr.is_global` instead of individual range checks to cover all non-routable addresses (CGNAT, unspecified, multicast). Redact credentials from error messages to prevent log leakage. Catch ValueError from ip_address() for zone-indexed IPv6 addresses. |
||
|
Patrick Buckley
|
e603a6a7d1 |
fix(oidc): pin redirect URI via TURNSTONE_OIDC_REDIRECT_BASE env var (#74)
* fix(oidc): pin redirect URI via TURNSTONE_OIDC_REDIRECT_BASE env var OIDC redirect_uri was derived from the request Host header, which is unreliable behind reverse proxies. Add TURNSTONE_OIDC_REDIRECT_BASE (env var / config.toml) to pin the externally-reachable origin. Extract _build_oidc_redirect_uri() helper to deduplicate the authorize and callback handlers. Validate redirect_base at load time (must be scheme://host[:port], rejects paths/query strings/invalid schemes). * fix(oidc): reject redirect_base with missing hostname Addresses Copilot review: values like `https://` or `https://:443` passed validation but would produce invalid redirect URIs. * fix(oidc): reject redirect_base with userinfo or invalid port Addresses Copilot round 2: urlparse silently accepts user:pass@host and non-numeric ports. Now explicitly rejects both. |
||
|
Patrick Buckley
|
20df7b3034 |
feat: OIDC SSO authentication with PKCE, auto-provisioning, and role … (#71)
* feat: OIDC SSO authentication with PKCE, auto-provisioning, and role mapping Add OpenID Connect as a fourth authentication method, enabling single sign-on via any OIDC provider (Okta, Azure AD, Google, Keycloak). Opt-in via env vars (TURNSTONE_OIDC_ISSUER, CLIENT_ID, CLIENT_SECRET). Security: - Authorization Code Flow with PKCE (S256) - State/nonce parameters with database-backed pending store (multi-node safe) - JWKS signature validation with async fetch + key rotation retry - Algorithm allowlist from JWKS key (not token header) prevents confusion - Identity matching exclusively by (issuer, sub) — prevents account takeover - password_enabled=false enforced server-side, not just UI - Rate limiting on both authorize and callback endpoints - OIDC users get "!oidc" password sentinel (bcrypt rejects naturally) - ID token validated for iss, aud, exp, nonce Features: - Auto-provisioning with username deduplication on first login - Claim-based role mapping with IdP demotion propagation (revokes stale roles) - "Continue with [Provider]" SSO button on login page - OIDC-only mode hides password form - Setup wizard required before OIDC login (admin bootstrap) Storage: migration 018 (oidc_identities + oidc_pending_states tables), 8 new protocol methods on both SQLite and PostgreSQL backends. 66 new tests (2273 total). * fix: address PR #71 review feedback (18 items) Bugs fixed: - OIDC success redirect now fetches permissions via new /auth/whoami endpoint before completing login (fixes permission-gating in UI) - Remove double decodeURIComponent on oidc_error (URLSearchParams already decodes; extra call throws on stray %) - Authorize rate limiter returns redirect instead of JSON 429 (endpoint reached via browser navigation, not fetch) - Lazy JWKS fetch in callback when startup discovery failed (IdP recovery without restart) - Startup exception handlers now log with exc_info=True - PostgreSQL pop_oidc_pending_state uses DELETE...RETURNING for true atomicity (eliminates TOCTOU) Behavior: - New OIDC users without role mapping get builtin-viewer by default (assigned_by="oidc-default", not revoked by role sync) Documentation fixes: - Role mapping: sync semantics (add + revoke stale), not "additive only" - PASSWORD_ENABLED=false blocks ALL password logins including admin - Algorithm: asymmetric allowlist, not per-key derivation - PlantUML diagram updated for role revocation API spec fixes: - Removed error_codes=[302] from callback (302 is success redirect) - Added /auth/whoami to both server + console specs - Regenerated TypeScript SDK OpenAPI snapshots (23 + 51 paths) * fix: address PR #71 round 2 review feedback (10 items) Rate limiting: - Authorize endpoint now calls record() after check() so the rate limiter actually counts attempts (was a no-op before) OIDC resilience: - Split startup try/except: discovery failure disables OIDC, JWKS prefetch failure leaves OIDC enabled for lazy retry on first login - JWKS unavailable message changed to "temporarily unavailable" (was misleadingly "not configured") - create_oidc_pending_state raises on collision instead of OR IGNORE (prevents silent insert drop on state collision) - SQLite pop_oidc_pending_state uses BEGIN IMMEDIATE for write lock (eliminates TOCTOU race) Frontend: - OIDC error display deferred 300ms so showLogin()'s async status fetch doesn't clear it via _switchMode → _clearError API spec: - OIDC authorize/callback endpoints now declare response_code=302 - Added AuthWhoamiResponse Pydantic model for /auth/whoami - Regenerated TypeScript SDK OpenAPI snapshots Documentation: - Diagram: JWKS "cached at startup, refreshed on-demand" (was "hourly") - Added TODO(tech-debt) comments on Host header redirect_uri sites |