Harden enterprise SAML & SCIM GA path (rebased re-port of #92)#161
Open
Bobcatsfan33 wants to merge 10 commits into
Open
Harden enterprise SAML & SCIM GA path (rebased re-port of #92)#161Bobcatsfan33 wants to merge 10 commits into
Bobcatsfan33 wants to merge 10 commits into
Conversation
SAML replay/relay-state hardening, durable SCIM storage with role mapping, migrations, middleware bearer-key auth, deps (defusedxml, python3-saml).
…AML/SCIM schemas
…scim_patch + group role mapping
…r (routes moved from api.py)
…st, idp-initiated off) + tests
…ole sync) + GA docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a clean re-port of #92 (
codex/enterprise-idp-ga) onto the refactoredmain. It supersedes #92, which was stale againstmain(routes were decomposed out ofapi.pyintoapi_routers/*, andmainindependently added SCIM filter/patch + role-mapping and a hardened preflight). The original #92 branch no longer applies cleanly; this branch carries its security behavior forward on top ofmain's current architecture.Do not merge without human review — this is security-critical auth code (SAML SSO + SCIM provisioning).
What was ported from #92
modules/auth/saml.py) — durable AuthnRequest state + relay-state tables, RelayState host allow-list validation (open-redirect defense), single-useInResponseToconsumption, assertion replay protection (saml_assertion_replaytable), Destination/Recipient checks against the ACS URL, IdP-initiated disabled by default, andin_response_to/assertion_idexposed on the parsed assertion.api_routers/enterprise.py(notapi.py). ParsesRelayState, passes it toparse_assertion, emitsAUTH_SUCCESS/AUTH_FAILUREaudit events, and returnsin_response_to.modules/auth/scim.py) — replaced the in-memory dict store with durable storage (scim_users/scim_groupsviamodules.storage.pg_connection), tenant-scoped uniqueness, ETag-style weak versions, and audit events for create/update/patch/delete.modules.auth.saml+modules.auth.sciminit_db, add a202605230001_enterprise_idp_gamigration (kept alongsidemain'sapi_key_rolesmigration).Authorization: Bearer <key>(Okta/Entra/OneLogin SCIM clients) before falling through to JWT.defusedxml==0.7.1andpython3-saml==1.16.0(keptmain's newer pins:python-jose 3.5.0,cryptography 48.0.1, etc. — no downgrades).python3-saml.python3-saml+defusedxmlruntime deps present, IdP-initiated off by default).scripts/idp_ga_validation.py,tests/test_saml_scim.pyadditions,tests/test_preflight_prod.pyadditions, newtests/test_tenant_middleware.py, and docs.How
main's architecture was preservedapi_routers/enterprise.py(routes moved out ofapi.py).scim_filter.pyandscim_patch.pyare untouched and still drive filtering + PatchOp — durable storage was integrated under them.main's SCIM group→role mapping (TOKENDNA_SCIM_GROUP_ROLE_MAP_JSON) was re-implemented on durable storage: a newmanual_roles_jsoncolumn persists manually-assigned roles, and_sync_roles()recomputes effective roles = manual ∪ group-derived on every user/group write.main'stest_scim_group_membership_syncs_rolespasses unchanged.main's newer dependency pins and hardened preflight (OIDC tenant-claim, compliance-profile, DoD/FedRAMP gates).Test results (Python 3.10 sandbox)
ruff check .— All checks passed.pytest tests/test_saml_scim.py tests/test_preflight_prod.py tests/test_tenant_middleware.py -q— all pass (28 + 14 = 42 passed).tests/test_v1_endpoints.pycases that fail identically on cleanorigin/mainunder the same-kfilter (test-ordering/env-leakage, unrelated to this PR; they pass when run in isolation).Note: the sandbox is Python 3.10, so
requirements.txt(which pinsnumpy==2.4.3, etc. requiring 3.11+) could not be installed verbatim; compatible versions were used for the local run. CI on the target Python version should install the pinned set.CodeQL hardening
The exact alert from #92 could not be fetched, so the new/changed code was proactively audited for the common CodeQL Python queries:
py/xxe,py/xml-bomb) — SAML response XML is parsed only withdefusedxml. The original Harden enterprise SAML and SCIM GA path #92 fell back to the stdlibxml.etreeparser whendefusedxmlwas missing (an unsafe sink on attacker-controlled XML); that fallback was removed — ifdefusedxmlis unavailable the code refuses to parse and logs an error.defusedxmlis now a hard runtime dep and a preflight check.py/clear-text-logging-sensitive-data) — verified the rawSAMLResponse/assertion body and bearer credentials are never logged. Audit events record only identifiers (name_id, issuer, session_index, assertion_id) as is standard for auth audit trails.?placeholders throughAdaptedCursor; no f-string/%-format SQL was introduced._sync_roles(), the newmanual_roles_jsoncolumn, and howpatch_userfeedsreplace_user. Known behavioral quirk (preserved frommain, not introduced here): becausescim_patch.apply_patchechoes the full resource, a PATCH that doesn't touchrolesstill rebases the user's current effective roles into their manual roles. This exactly reproducesmain's prior in-memory behavior (verified by runningmain's module directly), but a reviewer may want to decide whether GA should instead keep manual/derived strictly separate across PATCH._sync_rolesperformance/consistency. It recomputes all users in a tenant on each user/group write and does not bumpversion/updated_atfor role-only changes (matchesmain). Fine for provisioning volumes; confirm acceptable at scale and under Postgres (the sandbox verified SQLite only — Postgres path is code-review-only here).store.lookup_by_key(bearer.credentials)in try/except so a genuine JWT bearer falls through cleanly instead of 500-ing when the key probe errors (this was required to keeptest_tenant_auth_rbac.pygreen). Confirm swallowing lookup errors here is acceptable.manual_roles_jsonmigration.scim_usersis a brand-new durable table (was in-memory), so there is no legacy data to migrate;init_dbcreates the full schema fresh. Confirm no environment already created ascim_userstable without this column._extract_response_statereturns{}ifdefusedxmlis missing, which would break replay/request bookkeeping. This is intended (fail-closed) and guarded by a preflight check, but confirm the production image always shipsdefusedxml.not_before/not_afterare taken from the unauthenticatedConditionsextraction for bookkeeping afterpython3-samlsignature validation — confirm this matches your trust model (crypto trust stays withpython3-saml).docs/ops/saml-scim.md) were rewritten to GA wording and re-merged withmain's OIDC/SCIM role-map sections; please sanity-check the runbook.python3-saml(xmlsec) was not exercised in the sandbox (dependency needs system libs).scripts/idp_ga_validation.pyis the intended live harness.Minor: the
Dockerfile# ── Stage 1 ──comment separator differs from the source branch by a single box-drawing dash (cosmetic comment only; no build/CI/security impact).