feat: integrate Keycloak 26 commercial hardening - #42
Merged
Conversation
…irst RP Real-world bring-up on Keycloak 26.3.2 surfaced four import/runtime failures in the committed realm, each reproduced and fixed: - RealmRepresentation rejects unknown fields, so the $-prefixed annotation keys aborted --import-realm and crash-looped the container. Annotations moved to deploy/keycloak/README.md; the validator now fails on any $ key. - SAML IdP URLs are URL-validated at import: the bare __set_from_kv__ placeholder aborted the import. Placeholders are now URL-shaped (https://set-from-kv.invalid/__set_from_kv__) and still patched from KV. - An ENABLED committed LDAP source with placeholder DNs breaks every realm user operation (Invalid DN). The committed source now ships disabled and kcadm-bootstrap.sh enables it only after patching real values from KV. - The default Infinispan jdbc-ping stack crash-loops single-node compose restarts (each aborted boot leaves a stale jgroups_ping coordinator row the next boot fatally tries to join). The standalone compose now defaults to KC_CACHE=local via IDP_CACHE_MODE (clustered deployments set ispn). Imported realms also lack the standard client scopes, and without `basic` Keycloak 26 lightweight access tokens omit `sub`, breaking any RP that authenticates by subject. The realm now commits basic/profile/email scopes as realm defaults, adds an audience mapper to the RP template, and registers naruon-web as the first concrete RP (public PKCE S256 client with the sub/aud/role/org/workspace claims naruon's session contract requires). Verified: scripts/validate_realm.py passes; the realm imports cleanly into quay.io/keycloak/keycloak:26.3.2 with no sanitization; a live naruon authorization-code login (PKCE, RS256, JWKS) established a backend session end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…time API
Employer-specific federation (the hssmartdev ADFS SAML IdP, corporate LDAP)
was committed into realm-cwl.json with KV placeholders. That hardcodes one
employer into the ecosystem IdP's code and also breaks bring-up mechanically:
SAML IdP URLs are validated at import (placeholders abort it) and an enabled
LDAP source with placeholder DNs fails every realm user operation
(Invalid DN: __set_from_kv__).
External IdPs are deployment data, so they now live behind a runtime admin
API on the account-unification service:
- GET/PUT/DELETE /federation/identity-providers[/{alias}] and
POST /federation/identity-providers:apply. Desired state persists in the
KV/DB config store (source of truth) and is converged into Keycloak via
the Admin REST API, so a realm rebuild re-converges with one apply call.
- AdminApi gains identity-provider CRUD (HttpAdminApi + mock); KvStore gains
delete() on the protocol and both backends.
- realm-cwl.json commits no identityProviders and no user-storage federation;
scripts/validate_realm.py now fails closed on committed federation instead
of requiring it, and kcadm-bootstrap.sh drops the ADFS/LDAP patch steps and
additionally grants manage-identity-providers to the service account.
- deploy/templates/ remain as ready-made payload references for the API.
Verified: scripts/validate_realm.py passes; the account-unification suite
passes (58 tests) including new federation registry tests that register the
employer ADFS as runtime data; the realm still imports cleanly and a live
naruon OIDC login flow reaches the passwordless form after removing the
hardcoded federation from a running realm.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Semgrep (dynamic-urllib-use-detected) flags urllib.request.urlopen because it accepts file:// and other schemes. The probe only ever talks to the local listener, so it now validates the scheme and issues the request over an explicit plain-HTTP http.client connection; non-http probe URLs are refused before any connection is opened, with a regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ret handling Strix flagged the account-unification service (which holds realm-management privileges) as reachable unauthenticated, plus path-traversal and secret-in-argv issues. Remediated without suppression: - Operator bearer auth (app/auth.py) now gates every privileged router — merge, identity reads, SCIM provisioning/deactivation, and the federation registry — via a shared operator token from the KV/DB config store (constant-time compare, fails closed when unconfigured). /healthz stays open for probes. (VULN-0001 merge, VULN-0002 federation, VULN-0003 SCIM, VULN-0007 reads) - Path-segment identifier validation (app/identifiers.py) rejects '/', '\\', '.', '..', percent-encoding, and control characters. Applied at the API/SCIM boundary (400) and as a centralized chokepoint inside the Admin REST client before any URL is built, so a user_id like '../users/victim' or '%2e%2e' cannot escape the intended resource. (VULN-0006) - kcadm bootstrap no longer passes the admin password on argv: it fetches a short-lived admin token via curl with the password sourced from a 0600 temp file (--data-urlencode "@file"), then configures kcadm with that bearer token. (VULN-0005) - The Helm chart supports and documents an immutable image digest for the privileged account-unification image, rendered as tag@sha256 when set. (VULN-0004) New tests: operator-auth gating (missing/wrong/valid token, fail-closed, /healthz open), path-segment validation + Admin-client traversal rejection via MockTransport. Full suite passes (73 tests); ruff clean; interrogate 97.4%; validate_realm.py OK. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ot-review dispatch allowlist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The login page offered no way to create an account, so a fresh deployment could onboard nobody without operator kcadm surgery. Registration is now allowed with the email address as the account identity; the throwaway registration password never becomes a usable credential because the browser flow has no password authenticator and the default webauthn-register-passwordless required action enrolls a passkey in the first session. verifyEmail stays false while the realm has no smtpServer — the validator now enforces both pairings fail-closed (email-first + default passkey enrollment when registration is on; verifyEmail only with SMTP), each proven by mutation tests. Evidence: scripts/validate_realm.py OK on the shipped realm and rejects all three mutations; account-unification pytest 77 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Product frontends now own the signup UX: POST /registration/accounts on the account-unification service (own bearer token, distinct from the operator credential) creates the Keycloak account via the Admin API with an initial password and the webauthn-register-passwordless required action. The IdP-hosted registration form goes back off (registrationAllowed:false). The browser flow gains a browser-passwordless-credentials subflow where the passkey and the credential form are ALTERNATIVE siblings: the form is offered only while the account has no passkey, and the in-process password janitor (plus /registration/password-janitor:run) revokes the bootstrap password after enrollment, keeping the steady state passwordless. The validator enforces this exact bootstrap shape and still bans every other credential-form authenticator. Fresh-bring-up fixes found while wiring this live: the audit sink wrote into the read-only /bootstrap mount (now a separate writable path with a Dockerfile-owned directory), and kcadm-bootstrap.sh granted realm-management roles without scope mappings or a client-role protocol mapper, so the fullScopeAllowed:false service-account token never carried them and every Admin call failed 403. Evidence: validate_realm.py OK plus REQUIRED-password-form and username-password-form mutations rejected; service pytest 90 passed; live compose bring-up serves /healthz ok and POST /registration/accounts returns 201 with the account id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HttpAdminApi cached the service-account token for the connection lifetime, so after the token lifespan every Admin REST call failed 401 until a process restart — first observed as registration 500s minutes after bring-up. Each verb now routes through _send_with_reauth, which refreshes the token exactly once on a 401 and then fails honestly. Regression test drives a MockTransport that rejects the stale token and asserts the retry carries a freshly issued one. Evidence: service pytest 91 passed; live compose registration returns 201 again after the previous token aged out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lib finding The central SAST Semgrep gate fails on the base branch because p/default's python.lang.security.audit.dynamic-urllib-use-detected flags app/healthcheck.py: urlopen() receives a non-literal url, which urllib would happily open as a file:// path. This Medium finding blocks every open keyverse PR, since each PR scans a tree that still contains this file. Harden the probe by rejecting any URL whose scheme is not http/https before opening it, so a stray value can never coerce urlopen into a file:// read or another protocol handler. The residual audit finding on the (still non-literal) urlopen call is suppressed narrowly with an inline `# nosemgrep: dynamic-urllib-use-detected`, justified by the scheme allow-list and the fact that the container self-probe URL is not attacker-controlled. Add a regression test for the rejected-scheme path. Verified locally: semgrep marks the finding suppressed (gate passes), ruff is clean, and the healthcheck tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
…uplicate
A merged-away duplicate is tombstoned (disabled + a `merged_into_user_id`
pointer to the survivor) so it can never authenticate again, per
docs/merge-unification-flow.md and the CLAUDE.md merge invariant. But the SCIM
shim's `PUT /scim/v2/Users/{id}` (`replace_user`) only did a 404 existence
check and then translated the resource (whose `active` defaults to true) into a
replace — with no tombstone guard.
SCIM PUT is the *only* reactivation vector: `create` guards uniqueness and
`patch`/`delete` only ever disable. So a routine upstream HR/IGA full-sync PUT
that still lists the decommissioned person silently re-enabled the tombstoned
account (restoring its untouched passkey/WebAuthn login), and against a live
Keycloak the PUT would also overwrite the user representation, wiping the
`merged_into_user_id` pointer that resolves stale references to the survivor.
SCIM endpoints carry no app-level authz (trust terminates at the WAF edge), so
the trigger is unprivileged.
Fix: in `replace_user`, refuse with SCIM 409 when the target carries the
tombstone attribute, keeping a merged duplicate immutable via SCIM. Adds
`get_user_attribute` to the `AdminApi` protocol, the HTTP client, and the mock
so the guard reads the pointer uniformly. Regression test asserts the PUT is
refused and the duplicate stays disabled with its survivor pointer intact
(verified red→green: without the guard the test fails as the account is
re-enabled). Full suite 54 passed; interrogate 100%.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
…fied email
`UnificationService.merge_accounts` nested the unverified-email refusal inside
`if not decision.matched:`. Because `decide_match(..., explicit_link=True)`
returns `matched=True, reason=EXPLICIT_LINK`, an operator "explicit link" merge
skipped the guard entirely and merged + tombstoned two accounts whose only
shared tie was an UNVERIFIED email — the account-takeover vector the hard rule
exists to block (an attacker registers a duplicate holding the victim's
unverified email, then one explicit_link=True merge folds it into the victim).
This violates three contract sources:
- `app/models.py` MergeRequest.explicit_link docstring: "Even so, the service
refuses if the only tie is an UNVERIFIED email."
- `docs/merge-unification-flow.md`: "reject if only tie is unverified email ->
422 UnverifiedEmailMerge" is an unconditional step after decide_match.
- `CLAUDE.md`: "Never link or merge accounts on an unverified email."
Fix: hoist the unverified-email guard out of the not-matched branch and run it
for every decision reason except a genuine tie (EXACT_IDP_SUBJECT / VERIFIED_
EMAIL), so explicit-link and no-match are both covered. Legitimate merges are
preserved: an explicit link with different/absent emails still merges, and
verified-email / exact-(idp,subject) matches are exempt.
TDD: added test_refuse_explicit_merge_on_shared_unverified_email — confirmed it
fails on the pre-fix code ("DID NOT RAISE UnverifiedEmailMergeError", duplicate
gets tombstoned) and passes after. No existing test changes.
Verified (CI parity, py3.12, services/account_unification): ruff clean,
interrogate 100% (>=80 gate), pytest all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
…n unverified email" This reverts commit e95e87b.
…e handlers The healthcheck validated only the *initial* URL scheme, then used the default `urllib` opener -- which follows redirects and carries an `FTPHandler`. A `http:// -> ftp://` (or `file://`) redirect from the probed endpoint would have been followed by another protocol handler (the code comment even wrongly claimed it could not be). CodeRabbit flagged it (CWE-918 SSRF, Major). Fix: route the probe through a purpose-built opener that (1) carries only HTTP/HTTPS handlers -- no ftp/file/data handler exists to open such a target -- and (2) uses `_HttpOnlyRedirectHandler`, which drops any redirect whose `Location` scheme is not in the http/https allow-list. Both are belt-and- suspenders; either alone fails the ftp redirect closed. `main` now opens via the patchable `_open_health_url` seam. Added `test_healthcheck_opener_drops_non_http_redirect_target` (ftp target dropped, same-scheme redirect kept, no ftp/file/data handler on the opener); the three existing tests re-point to the new seam. Verified (CI parity, services/account_unification): pytest all pass, ruff clean, interrogate 99.4% (>=80 gate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
Contributor
Author
|
@coderabbitai review Please review the exact current head. The previous actionable and nitpick findings were re-evaluated and addressed, including the password-free action-email enrollment redesign, storage-lock-free federation I/O, fail-closed redaction, protocol-native SCIM errors, durable deployment state, and deterministic production-lock race regression. |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 36 minutes. |
Contributor
Author
|
@coderabbitai review Please review the exact current head |
|
✅ Action performedReview finished.
|
Contributor
Author
|
Current-head remediation status (
Remaining protected gates are exact-head CI/CodeQL/Semgrep/Security execution and an independent approving review. They are not bypassed. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
3 participants
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
Consolidates the overlapping Keycloak 26 bring-up, SCIM/merge serialization, SQLite runtime safety, deployment durability, protected hourly PR stewardship, and buyer-visible product hardening work into one integration branch.
Buyer-visible product gaps closed
naruon-webPKCE relying party;VERIFY_EMAILandwebauthn-register-passwordless;applied_to_keycloak=falsewhen desired state is stored but Keycloak convergence fails;application/scim+json;CHANGELOG.md, architecture/design documentation, implementation plans, and regression coverage for the new invariants.Integration policy
Verification required on the exact final head
uv sync --locked --extra devuv run ruff check app tests toolsuv run interrogate .at 100%uv run pytest -qat 100% production statement and branch coverageSupersedes #18, #39, #40, and #41.