823f5d48d1421bf5f1e4aaed5c255b65b257c481
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 jcoffey-dev
|
823f5d48d1 |
Unify the Tenant CRD with enterprise-api -provision-tenant (lightweight)
Closes a gap named across CLAUDE.md/docs/architecture.md/deploy/README.md
since early Phase 4: the operator's Tenant CRD and -provision-tenant
were two disconnected mechanisms. The operator's reconciler generated a
K8s Secret with a locally-generated random password that authenticated
against nothing (nothing ever called ClickHouse to create a matching
user), and unconditionally claimed status.phase=Active the moment a
Tenant object existed -- actively misleading, not just incomplete.
Two unification shapes were considered (surfaced to the user via
AskUserQuestion, given the real difference in blast radius): the
operator's reconcile loop becoming a second real actor (new Postgres +
ClickHouse admin credentials flowing into the K8s controller, plus real
reconcile-loop idempotency/retry design for an inherently one-shot
external side effect), or keeping -provision-tenant as the sole real
actor and having it also sync its result into the CRD. Went with the
lighter option.
enterprise/internal/tenantcrd (new): a Syncer using the K8s dynamic
client (unstructured.Unstructured + a GroupVersionResource, not
deploy/operator's typed Tenant struct -- avoids a cross-module Go
dependency between two independently-versioned modules for one type).
Upserts the Tenant object, creates/updates a Secret with the *real*
ClickHouse credentials owned by that Tenant via an OwnerReference, then
patches status.{clickHouseDatabaseName,clickHouseSecretRef,
tantivyIndexPath}. Idempotent and safe to retry: never rotates a
credential across a re-sync, never overwrites a pre-existing
spec.displayName a human/GitOps process set.
cmd/enterprise-api/main.go's runProvisionTenant calls Sync when
TENANT_CRD_NAMESPACE is set (empty = no-op, same shape as every other
optional dependency in this codebase). Its "already active" refusal is
now split: ClickHouse re-provisioning is still refused (rotating a live
credential would break every open connection for no benefit), but CR
sync alone is now retryable using the credentials already on file in
rbacstore -- needed for retrying a previously-failed sync, or
backfilling CR sync for a tenant provisioned before this existed.
deploy/operator's reconciler rewritten to match: it never claims
PhaseActive on its own initiative anymore, only once
status.ClickHouseDatabaseName is non-empty (the field -provision-tenant,
and only -provision-tenant, sets). Phase is now a pure function of
{spec.suspended, status.ClickHouseDatabaseName != ""} recomputed every
reconcile, not toggled in place -- fixes a related bug the old code
would have hit once suspension was involved: un-suspending an
already-provisioned tenant needs to return straight to Active, which
isn't derivable from "last observed phase was Suspended" alone. The
reconciler no longer creates or manages any Secret, dropped its
`secrets` RBAC grant entirely, and gained zero new dependencies.
Helm chart: enterprise-api gets its own ServiceAccount/Role/RoleBinding
(get/list/create tenants, get/update/patch tenants/status, get/create/
update secrets -- least-privilege, scoped to the release namespace, not
a ClusterRole) and a TENANT_CRD_NAMESPACE env var, both gated on
tenantOperator.enabled. tenant-operator's ClusterRole loses the
secrets grant it no longer needs.
Verified in this environment: enterprise/internal/tenantcrd's tests run
against k8s.io/client-go's fake dynamic + typed clientsets (real client
library, fake transport, no cluster needed); deploy/operator's rewritten
tenant_controller_test.go runs against controller-runtime's fake
client, including new regression tests for the "must not claim Active
without confirmation" and "un-suspending returns to Active, not
Provisioning" properties; helm template + parsing the rendered YAML
confirms the RBAC split renders exactly as designed under both
tenantOperator.enabled=true/false. Not verified: an actual
-provision-tenant run against a real cluster with the operator watching
(no live cluster in this environment, same disclosed limitation as the
rest of /deploy). Docs updated in lockstep: CLAUDE.md, docs/architecture.md,
deploy/README.md, deploy/helm/sentry/README.md (including a corrected
"Trying the two-tenant example" walkthrough), phase-4-runbook.md (new
§11), enterprise/README.md. Also fixed two unrelated stale claims found
along the way: docs/architecture.md still said docker-compose.yml ran
plain api unconditionally (fixed in an earlier commit, doc not updated
then), and enterprise-api's own main.go doc comment still said Helm/
docker-compose wiring wasn't built yet.
|
||
|
jcoffey-dev
|
2e698f5623 |
Close the last tenant-isolation adversarial probe (mid-provisioning tenants)
Phase 4 task 8's verification plan named four adversarial probes; three were closed earlier this phase, the fourth (an evaluator tick, or any other caller, hitting a tenant that exists but hasn't reached the active+credentialed gate yet -- must be refused, not served) was still an explicitly-skipped stub in api/queryapi/tenant_isolation_gap_test.go. Investigating it found the two storage engines needed genuinely different treatment: - ClickHouse (enterprise/internal/chrunner) already had this property structurally, for free: Registry is built once at startup from rbacstore.ListProvisionedDataSources, which already filters to active+credentialed tenants only, so a mid-provisioning tenant is simply absent from the connection map. New test TestRegistryRefusesMidProvisioningTenant proves this without Docker -- an empty DataSource list never dials ClickHouse, so this genuinely runs in this environment, unlike every other test in that file. - Tantivy (search/src/registry.rs's IndexRegistry) was a real, different gap, not just an unverified assumption: it opens-or-creates an index for any syntactically-valid tenant_id on first request, because it's a separate process with no Postgres access and structurally can't know which tenants are actually provisioned. A query against a mid-provisioning tenant would have silently succeeded with zero results from a freshly-created empty index -- "ambient success" indistinguishable from "no matching logs," exactly the failure mode this item was worried about. Fixed the Tantivy gap with a new enterprise/internal/searchclient. TenantChecker interface (backed by a new rbacstore.TenantIsActive, implemented structurally, no new import edge needed), consulted before every gRPC call: Client.Search now refuses a non-active tenant before it ever reaches `search`. Dial's signature gained a required TenantChecker parameter; enterprise-api's main.go passes its existing rbacstore.Store (already satisfies the interface). Verified Docker-free via searchclient's existing real-in-process-gRPC-server test harness (TestSearchRefusesMidProvisioningTenant, plus TestSearchPropagatesTenantCheckerError for the fail-closed-on-error case) -- both genuinely run in this environment, same bar as the rest of the Tantivy isolation work. rbacstore.TenantIsActive itself has two new skip-gated live-Postgres tests (TestTenantIsActive, TestTenantIsActiveNonexistentTenant) -- disclosed as not run against a live database here, same gap as the rest of this phase's Postgres-backed pieces. api/queryapi/tenant_isolation_gap_test.go rewritten from a checklist with one skipped stub to a full accounting of all four now-closed probes. Docs updated in lockstep: CLAUDE.md, threat-model.md, phase-4-isolation-design.md (implementation note added after its original sign-off), phase-4-runbook.md (§9), enterprise/README.md. |
||
|
jcoffey-dev
|
243f4dc2ab |
Enforce per-resource dashboard grants (RBAC matrix's own/granted qualifier)
api/dashboards' handler previously enforced only tenant-baseline role
(RoleEditor+), so any Editor could edit/delete any dashboard in their
tenant -- the matrix's "(own/granted)" qualifier was explicitly named
as unbuilt in this handler's own doc comment. This closes that gap.
New core interface api/dashboards.PermissionStore (nil-safe, same "not
wired == no-op" shape as authz.Authorizer) resolves a per-resource
dashboard_permissions grant. canEditDashboard now requires the
identity be Admin/Owner, the dashboard's creator, or hold a grant of at
least Editor; canManageGrants is deliberately stricter (creator or
Admin/Owner only, never grant-derived access) so a user who can edit a
dashboard only because of a grant can't extend or re-grant that access
to themselves or others. Wired handlers: PUT/DELETE
/dashboards/{id}/permissions/{userId}, GET .../permissions.
Two real bugs found and fixed while wiring this up, before any of it
touched a live database:
- handleCreate/handleImport never stamped created_by from the
authenticated identity, so every dashboard was owned by "anonymous"
regardless of who made it -- the ownership check would have been
meaningless. Also fixed: ImportDashboard trusted the exported JSON's
created_by verbatim, so re-importing someone else's export would
leave the actual importer unable to edit their own copy.
- metadata/migrations/0024_create_dashboard_permissions.sql's CHECK
constraint diverged from /docs/phase-4-rbac-design.md's schema
(allowed role='admin', nullable granted_by). Reconciled via
0033_restrict_dashboard_permissions_role.sql: Admin/Owner already
have tenant-wide access so a resource-level "admin" grant is
meaningless, and every real grant now always has an attributable
granter.
enterprise/internal/rbacstore gets the storage side: raw CRUD
(dashboard_permissions.go) plus DashboardPermissions
(dashboards_adapter.go), an adapter implementing
api/dashboards.PermissionStore -- same pattern as audit.QueryAPILogger
over queryapi.AuditLogger. Wired into enterprise/cmd/enterprise-api
only; plain api/cmd/api passes nil (ownership/Admin checks still work
via the nil-permissions fallback, just without the "granted" bonus).
Verified: the full own/granted/admin/creator matrix, including the
granted-editor-cannot-manage-grants regression, passes against a fake
PermissionStore (api/dashboards/handler_test.go, all existing tests
also still pass unmodified in behavior). Real integration tests exist
in enterprise/internal/rbacstore/rbacstore_test.go (skip-gated on
RBACSTORE_TEST_POSTGRES_ADDR, same convention as every other
Postgres-backed piece this phase) but have not run against a live
database in this environment -- disclosed in threat-model.md,
phase-4-runbook.md, and enterprise/README.md alongside every other
piece carrying the same gap. Also fixed a stale path in
phase-4-runbook.md's dashboards-tenant-scoping section
(./internal/dashboards/... -> ./dashboards/..., stale since that
package moved out of api/internal/ earlier in this phase).
|
||
|
jcoffey-dev
|
ba2276aa1a |
Phase 4: real Tantivy per-tenant isolation (search/src/registry.rs, enterprise/internal/searchclient)
Closes the last named "isolation mechanism" gap: search.proto gains a tenant_id field on SearchRequest; search/src/registry.rs's IndexRegistry resolves it to an on-demand-opened, per-tenant Tantivy index (empty tenant_id keeps today's single default index, so this is purely additive); enterprise/internal/searchclient sets that field from the authenticated request identity in ctx, mirroring chrunner's exact fail-closed "never a parameter" shape. Wired into enterprise-api in place of the shared api/searchclient. Unlike the ClickHouse pieces from the previous two commits, this one is genuinely verified end to end in this environment: Tantivy is an embedded library, not a networked service, so both the Rust index registry (cargo test, cargo clippy --all-targets -- -D warnings, both clean) and the Go client (a real in-process gRPC server) could actually run. registry.rs's tenant_index_is_isolated_from_default_and_other_tenants seeds three real indices with the same term and confirms a tenant-scoped search returns only that tenant's document -- item 3 of the isolation design doc's verification plan, closed for real, not just written. With both ClickHouse and Tantivy isolation now built, the single largest remaining gap is no longer a missing mechanism: it's that nothing forces or flags whether a deployment actually runs enterprise-api instead of plain api, and that ingest itself has no tenant concept for either storage engine (every record still lands in the one shared database/ index no matter what -- undesigned, not just unbuilt). Updated the threat model, architecture doc, CLAUDE.md, and both READMEs accordingly. |
||
|
jcoffey-dev
|
1d57e697b1 |
Phase 4: real per-tenant ClickHouse isolation via a new enterprise-api binary
Closes the threat model's headline finding for the SQL query path:
enterprise/internal/tenantprovision does real CREATE DATABASE/USER/GRANT
against ClickHouse, and enterprise/internal/chrunner is a per-tenant
connection registry implementing api's SQLRunner interface, resolving
the tenant from the authenticated request identity -- never a
caller-suppliable parameter. Both are wired into a new binary,
enterprise/cmd/enterprise-api, alongside the unchanged single-tenant
api/cmd/api, since AGPL core can never import enterprise/ and Go's own
internal/ package visibility rules meant enterprise/ couldn't implement
core's SQLRunner interface without importing the package that defines
it. That required moving api/internal/{authz,queryapi,dashboards,
querylang/executor,searchclient,httpserver} out of internal/ -- the
minimal set enterprise-api needs to import; querylang's compiler
internals (planner/lexer/parser/ast/ir) and api's own config stay
internal, since nothing outside api needs them directly.
Also finally wires enterprise/internal/audit into queryapi.AuditLogger
(nil since Phase 4 task 4) via a new adapter, and adds live-ClickHouse
integration tests for two of the four adversarial probes named in
docs/phase-4-isolation-design.md's verification plan.
Corrected several overclaims in the docs while writing this up: an
earlier claim that rbacstore's CRUD was "verified against a live
Postgres" was never actually true in this environment (only
internal/audit was, earlier in this phase, before Docker access was
lost) -- threat-model.md, phase-4-runbook.md, CLAUDE.md, and
enterprise/README.md all now distinguish "a real integration test
exists" from "this was confirmed against a live database."
Still not built: Tantivy/free-text tenant isolation
(enterprise/internal/searchclient), and any deployment-topology
mechanism that actually routes traffic to enterprise-api instead of
plain api -- both binaries exist side by side today with nothing
enforcing or flagging which one a deployment runs.
|