feat(gitops): sealed broker-realm secrets + CI guard + canonical insight client scope (Phase 1, boxes 2–3) - #2209
Conversation
…t client scope Boxes 2+3 of constructorfabric#2195 (ADR-0003 broker realms as code): - environments/local/keycloak/realms/insight-broker.yaml — the canonical realm shape environments copy: the `insight` client scope allow-lists exactly `email` + single-string `tenant_id` (plus protocol-level `sub`), and the insight-authenticator client runs with fullScopeAllowed off and exactly [basic, insight] assigned, so upstream-IdP claims die at the broker unless a mapper re-emits them. tenant_id mapper shape matches the compose realm generator. Inert until an env sets keycloakConfig.enabled (Phase 4). - insight-keycloak-config sealed-secret shape template + inventory entry (off by default): one Secret carries the config-cli login and every env-var placeholder the realm YAML references. - scripts/ci/realm_secret_guard.py + ci.yml job + pre-commit hook: credential fields in realm YAML must hold env placeholders (client secret, IdP clientSecret, passwords, bindCredential, credentials values); also rejects placeholder syntax in YAML comments, which fails the config-cli import at deploy time (Phase-0 finding). Unparseable YAML is an error, never a pass. Verified live on a scratch cluster: realm applied by the hook Job; example ID/access tokens for a probe user carry exactly sub+email+tenant_id and a stray user attribute does not leak. Guard exercised on pass, literal-secret, comment-placeholder, and unparseable-YAML inputs. Refs constructorfabric#2195 Part of constructorfabric#2193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds a canonical Keycloak broker realm, sealed-secret configuration, and environment wiring. It adds a Python guard that rejects literal credentials and invalid placeholders, then runs the guard in CI and pre-commit. ChangesKeycloak realm guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant PyYAML
participant realm_secret_guard
participant RealmYAML
PullRequest->>GitHubActions: Change realm YAML
GitHubActions->>PyYAML: Install parser
GitHubActions->>realm_secret_guard: Run validation
realm_secret_guard->>RealmYAML: Read and inspect
RealmYAML-->>realm_secret_guard: YAML content
realm_secret_guard-->>GitHubActions: Findings and exit status
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/gitops/environments/local/keycloak/realms/insight-broker.yaml`:
- Around line 37-46: The email mapper currently reads from the user attribute
map instead of Keycloak’s built-in UserModel.email property. Update the mapper
named “email” to use oidc-usermodel-property-mapper while preserving its
existing claim name, token inclusion, and email configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dbbdc00-1173-4fb8-82b0-10b9bfbb6c36
📒 Files selected for processing (7)
.github/workflows/ci.yml.pre-commit-config.yamldeploy/gitops/README.mddeploy/gitops/environments/local/inventory.yaml.templatedeploy/gitops/environments/local/keycloak/realms/insight-broker.yamldeploy/gitops/environments/local/sealed-secrets/insight/insight-keycloak-config-sealedsecret.yaml.templatescripts/ci/realm_secret_guard.py
…m other existing Secrets Realm placeholders should reference already-sealed values (e.g. the authenticator client secret both the realm and the authenticator config must agree on) instead of sealing the same value twice. envFrom cannot do this for keys that are not valid env-var names (it drops them silently — hyphenated keys like oidc-client-secret), so extraEnv passes verbatim env entries with secretKeyRef mappings into the config-cli container. Refs constructorfabric#2195 Part of constructorfabric#2193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Per review: the repository's standard secret scanning covers realm YAML like any other file, and realm content only ever references secrets via env placeholders resolved from sealed Secrets (keycloakConfig.extraEnv for already-sealed values). The two authoring rules the script enforced stay documented in the gitops README — including the config-cli comment-substitution gotcha, which fails loudly at deploy time anyway. Refs constructorfabric#2195 Part of constructorfabric#2193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
email is a built-in UserModel property; the property mapper is what the stock `email` client scope uses. The attribute mapper worked on 26.4 (properties are unified into the attribute view there) but the property mapper is the canonical, version-robust shape. Live claim-set probe re-run: identical output (sub+email+tenant_id, stray attribute does not leak). Addresses the CodeRabbit finding on constructorfabric#2209. Refs constructorfabric#2195 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…im comments Realm IdP mappers pin the tenant per environment from the existing global.tenantDefaultId single source of truth (hardcoded-attribute-idp- mapper, never an upstream claim — changing the IdP must not change the tenant). Comments across the broker-realm files cut to short, exact notes. Refs constructorfabric#2195 Part of constructorfabric#2193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The broker Keycloak deploys with the stack instead of being an external prerequisite (amends ADR-0002 Option A; ADR-0003 already allowed the subchart mechanism). insight-keycloak gains mode=production: `start`, MariaDB via the existing L2 (no second DBMS, per the Phase-4 DB note), bootstrap admin from a Secret, health probes on the management port, no realm import — realm content stays config-cli-as-code (constructorfabric#2204/constructorfabric#2209). mariadb-init-svcdbs provisions the keycloak database. Verified on a scratch cluster: boots against MariaDB (89 KC tables), canonical realm applied via the hook Job, discovery 200, realm survives pod restart. Refs constructorfabric#2193 constructorfabric#2198 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
What
The remaining two Phase-1 boxes of the broker rollout, delivered together (they are one coherent unit: the realm YAML, the Secret it references, and the guard that polices it):
Box 2 — secrets via SealedSecrets + CI guard
insight-keycloak-configsealed-secret shape template (environments/local/sealed-secrets/insight/…yaml.template) + inventorysecrets.servicesentry (off by default). One Secret feeds the config-cli hook Job (keycloakConfig.existingSecret, feat(gitops): keycloak-config-cli sync Job — broker realms as code (Phase 1, box 1) #2204): theKEYCLOAK_USER/KEYCLOAK_PASSWORDlogin plus every$(env:VAR)placeholder the realm YAML references.keycloakConfig.extraEnv(added here) lets placeholders resolve from other already-sealed Secrets via explicitsecretKeyRefmappings, so a value shared with another consumer (e.g. the authenticator's client secret) has exactly one sealed source of truth and is never sealed twice — also the only way to consume Secret keys that are not valid env-var names, whichenvFromdrops silently. A bespoke realm-YAML lint was tried and dropped in review: the repository's standard secret scanning covers these files, and the authoring rules (placeholders only; no placeholder syntax in YAML comments — config-cli substitution scans comments and fails the import loudly at deploy time) are documented in the gitops README.Box 3 — canonical
insightclient scopeenvironments/local/keycloak/realms/insight-broker.yaml— the canonical realm shape environments copy when onboarding: aclientScopesentry namedinsightemitting exactlyemailand the single-stringtenant_id(DD-AUTH-04), and theinsight-authenticatorconfidential client withfullScopeAllowed: falseand exactly[basic, insight]assigned — the allow-list ADR-0003 mandates, so upstream-IdP claims cannot pass through unless a mapper re-emits them. (basicis the Keycloak 24+ built-in carrying the protocol-requiredsub/session claims; it emits no identity claims.) Thetenant_idmapper shape matches the compose realm generator. The file is inert until an environment flipskeycloakConfig.enabled(Phase 4).Verification
sub,email,tenant_idplus protocol claims — the stray attribute does not leak, and noprofile-scope claims appear.helm lintunaffected (no chart changes); pre-commit clean.One finding for Phase 3 (#2196)
Keycloak 24+ declarative user profile drops unmanaged user attributes written via the admin REST API by default (
unmanagedAttributePolicyunset) — a probe user created that way silently losttenant_iduntil the scratch realm allowed unmanaged attributes. The production path is unaffected (identity-provider mappers stamp the attribute at first broker login, as the Phase-0 PoC proved end-to-end), but Phase 3 should decide whether realms declaretenant_idin the user profile explicitly rather than relying on that difference.Refs #2195
Part of #2193
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes