From bdd42e06f6387be46a916c0d3cf09edb837e06ae Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 14 Aug 2026 20:16:09 -0700 Subject: [PATCH] Build per-tenant Tantivy write-routing, closing the last ingest write gap search/src/consumer.rs now resolves each record's tenant_id Kafka header through the same IndexRegistry the read side (search/src/registry.rs + enterprise/internal/searchclient) already used, and writes into that tenant's own index instead of always the default one. The periodic Tantivy commit now commits every tenant index that's actually seen a write (IndexRegistry::commit_all), not just the default index. Unlike ClickHouse, this needed no "second binary": Tantivy has no grant system to gate a commercially-licensed credential behind, so IndexRegistry already lived directly in this AGPL-core search binary -- there was never an import-boundary reason to split the write side into an enterprise/ binary the way chwriter/enterprise-ingest was for ClickHouse. Read and write simply share one registry. Because Tantivy is an embedded library, this is genuinely verified in this environment, not just written: registry.rs's commit_all_commits_default_and_every_opened_tenant_index writes into the default index plus two tenant indices, confirms nothing is searchable pre-commit, then confirms all three are post-commit. consumer.rs's tenant_id_from_headers is factored out as a small pure helper (mirroring ingest/consumer.tenantIDFromHeaders) with its own unit tests, plus a guard test against the "tenant_id" header-key literal drifting from the Go side's -- the same guard-test pattern ingest/cmd/ingest already used for its own two Go copies of the constant, now mirrored a third time across the language boundary. One gap is disclosed, not fixed, by this change: unlike chwriter.Registry (an active-tenants-only snapshot built at enterprise-ingest startup, so an unrecognized tenant_id is refused outright) and unlike the read side (gated by searchclient.TenantChecker), this consumer's registry.resolve() call has no active-tenant check at all -- search has no Postgres access to check tenant status against. A still-valid-but-should-be-revoked ingest credential can cause an index directory to be created for a tenant that's no longer active. Narrow blast radius (an orphan, isolated, empty index, not cross-tenant leakage, and only reachable with a real signed credential), but real -- see registry.rs's doc comment on resolve(). Closing it fully would mean giving search some way to learn which tenants are active without an enterprise/ import, which isn't designed yet. This closes the last of Phase 4's ingest write-routing gaps (ClickHouse was closed last commit). The one remaining gap in the whole phase is now the tenant-picker frontend page, deliberately deferred earlier in this phase as out of scope for this environment. --- CLAUDE.md | 39 +++++++---- docs/architecture.md | 40 +++++++---- docs/phase-4-runbook.md | 99 ++++++++++++++++++++------- docs/security/threat-model.md | 124 +++++++++++++++++++++------------- enterprise/README.md | 24 ++++--- search/README.md | 52 ++++++++++---- search/src/consumer.rs | 100 ++++++++++++++++++++++++--- search/src/main.rs | 13 ++-- search/src/registry.rs | 118 +++++++++++++++++++++++++++----- 9 files changed, 458 insertions(+), 151 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index da71abf..f7901d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -249,18 +249,33 @@ ClickHouse user `SELECT` only, which would have made every real per-tenant write fail with a permission error — fixed by granting `SELECT, INSERT` (one credential, both directions; no cross-tenant boundary is crossed by also allowing INSERT within a tenant's own -database). **What's still deferred, clearly**: Tantivy's side. -`search/src/consumer.rs` is a completely independent Redpanda consumer -(not called through `ingest` or `enterprise-ingest` at all) and still -writes every record into the one shared (default) index regardless of -tenant — a different codebase (Rust) and a different process, real, -disclosed, separately-scoped remaining work, not just "the same fix -applied twice." What still keeps this phase from being done: the actual -tenant-picker *page* doesn't exist (`web` has no session/cookie-handling -code at all yet, and `enterprise-auth` has no CORS middleware for a -cross-origin `fetch` with credentials — both real, separately-scoped -frontend gaps), and Tantivy write-routing per the above. Full -accounting: +database). **Tantivy write-routing is now built too** — +`search/src/consumer.rs` (a completely independent Redpanda consumer, +not called through `ingest` or `enterprise-ingest` at all: a different +codebase and process) now resolves each record's `tenant_id` header +through the *same* `IndexRegistry` the read side already used, and +routes the write there instead of always into the default index. Unlike +the ClickHouse side, this needed no "second binary": `IndexRegistry` +already lives in this AGPL-core binary (Tantivy has no grant system to +gate a commercially-licensed credential behind, so there was never an +import-boundary reason to split it out), so read and write share one +registry directly. The periodic Tantivy commit now commits every tenant +index that's seen a write, not just the default one +(`IndexRegistry::commit_all`). **One gap disclosed, not fixed, in this +same change**: unlike `chwriter.Registry` (built from an +active-tenants-only snapshot at startup) and unlike the read side (gated +by `searchclient.TenantChecker`), this consumer's `resolve()` call has +no active-tenant check — this process has no Postgres access to check +against, so a still-valid-but-should-be-revoked ingest credential can +cause an index directory to be created for a tenant that's no longer +active. Narrow blast radius (an orphan, isolated, empty index — not +cross-tenant leakage — and only reachable with a real signed +credential), but real; see `search/src/registry.rs`'s doc comment on +`resolve`. What still keeps this phase from being done: only the +tenant-picker *page* now — `web` has no session/cookie-handling code at +all yet, and `enterprise-auth` has no CORS middleware for a cross-origin +`fetch` with credentials, both real, separately-scoped frontend gaps. +Full accounting: `/docs/security/threat-model.md`; step-by-step verification procedure (not yet run against a live cluster in this environment): `/docs/phase-4-runbook.md`. The rest of this section describes the exit diff --git a/docs/architecture.md b/docs/architecture.md index 96f3cd1..806dd18 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -139,8 +139,8 @@ escape hatch is opaque to any compiler-injected filter. ran in the environment it was built in — Tantivy is an embedded library, so the cross-tenant isolation probe needed no live database or Docker to execute for real, and it passed. -- **Ingest identity and ClickHouse write-routing are now built; Tantivy's - isn't.** `ingest` (AGPL core) gained an optional `TenantResolver` +- **Ingest identity, ClickHouse write-routing, and Tantivy write-routing + are all now built.** `ingest` (AGPL core) gained an optional `TenantResolver` (`ingest/internal/grpcserver`): an agent presents a per-tenant bearer credential (`enterprise-auth -create-ingest-credential-tenant=` mints one, only its hash stored), validated over the network via a new @@ -160,13 +160,28 @@ escape hatch is opaque to any compiler-injected filter. `SELECT` only, which would have made every real per-tenant write fail with a permission error — fixed by granting `SELECT, INSERT` (one credential, both directions; no cross-tenant boundary is crossed by - also allowing INSERT within a tenant's own database). What isn't built - yet: `search`'s independent Redpanda consumer (a different codebase — - Rust — and a different process, not reachable through either `ingest` - or `enterprise-ingest`) still writes every record into the one shared - Tantivy index regardless of tenant. That's real, disclosed, separately - scoped remaining work, not something this change claims to have - closed. + also allowing INSERT within a tenant's own database). `search`'s + independent Redpanda consumer (a different codebase — Rust — and a + different process, not reachable through either `ingest` or + `enterprise-ingest`) is now write-routed too: + `search/src/consumer.rs` resolves each record's `tenant_id` header + through the *same* `IndexRegistry` the read side + (`search/src/registry.rs` + `enterprise/internal/searchclient`) + already used, and writes there instead of always into the default + index. No "second binary" needed here, unlike ClickHouse — Tantivy has + no grant system to gate a commercially-licensed credential behind, so + `IndexRegistry` already lived directly in this AGPL-core binary, and + read/write just share it. One gap is disclosed rather than closed by + this change: unlike `chwriter.Registry` (an active-tenants-only + snapshot built at startup) and unlike the read side (gated by + `searchclient.TenantChecker`), this consumer's `resolve()` call has no + active-tenant check — the process has no Postgres access to check + against — so a still-valid-but-should-be-revoked ingest credential can + cause an index directory to be created for a tenant that's no longer + active. Narrow blast radius (an orphan, isolated, empty index, not + cross-tenant leakage, and only reachable with a real signed + credential), but real; see `search/src/registry.rs`'s doc comment on + `resolve`. - `deploy/operator`'s `Tenant` CRD and `enterprise-api -provision-tenant` are now unified, deliberately lightweight: `-provision-tenant` stays the sole real actor (ClickHouse + `rbacstore`), and now also syncs its @@ -188,9 +203,10 @@ plain `api`), sharing a host-port/network-alias trick so `alerting`/ `web` need no conditional config either way. With both storage engines' connection/index-layer mechanisms built, deployment topology enforced at both the Helm and docker-compose layers, and the two provisioning -mechanisms unified, the largest remaining gap is ingest's per-tenant -*write-routing* (identity is now attached at ingest time; nothing -downstream of Redpanda consumes it yet to isolate the write, see above). +mechanisms unified, and both storage engines' write paths now +per-tenant-routed too (see above), the largest remaining gap in this +phase is the tenant-picker *frontend* page — the backend protocol is +built, but `web` has no session/cookie-handling code yet to call it. ## Licensing boundary diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index b5837bb..06e15c4 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -38,6 +38,15 @@ of what was already run and passed. Two genuine exceptions: "email" -- crewjam's own fake IdP hit this path. Same remaining gap as OIDC: not yet tried against a real external IdP or a running `enterprise-auth` container. +- Tantivy tenant isolation, both directions -- `search/src/registry.rs`'s + cross-tenant read isolation (§9) and, since this pass, + `search/src/consumer.rs`'s per-tenant write-routing (§14) -- verified + live, no disclaimer needed, because Tantivy is an embedded library with + no Docker/broker dependency: real indices, real documents, real + commits, run in this environment. The one thing about it that's still + unverified is not Tantivy itself but the upstream credential/header + plumbing feeding it (ingest's `TenantResolver`, `enterprise-auth`'s + `/internal/authorize-ingest`) against a real running stack. Everything else — `internal/rbacstore`'s CRUD, the auth-enforcement walkthrough, the dashboards tenant-scoping fix, the Helm chart, the @@ -604,7 +613,7 @@ yet would refuse all ingest traffic outright rather than degrading gracefully. See §14 below for what the identity is actually used for on the write path. -## 14. Ingest write-routing (ClickHouse built, Tantivy not yet) +## 14. Ingest write-routing (both storage engines) `enterprise/cmd/enterprise-ingest` (mirrors `enterprise-api`'s "second binary" shape) consumes the `tenant_id` Kafka header §13 attaches and @@ -643,12 +652,50 @@ never actually executed -- "the test exists" is not the same claim as "write-routing is confirmed," same caveat §8 already states for the read-side `chrunner` tests. -**Not built**: Tantivy's side of this. `search/src/consumer.rs` is a -completely independent Redpanda consumer, not called through `ingest` or -`enterprise-ingest` at all, and does not read the `tenant_id` header -- -every record still lands in the one shared (default) Tantivy index -regardless of tenant. See CLAUDE.md and `/docs/security/threat-model.md`'s -"Read this first" for the full disclosure. +**Tantivy's side is built too, and genuinely verified.** +`search/src/consumer.rs` reads the same `tenant_id` Kafka header and +resolves it through `search/src/registry.rs`'s `IndexRegistry` -- the +same registry the read side (§9) already uses -- routing each record's +write into its own tenant's Tantivy index instead of the single default +one. No "second binary" was needed here the way ClickHouse needed +`enterprise-ingest`: Tantivy has no grant system to gate a +commercially-licensed credential behind, so `IndexRegistry` already +lives directly in AGPL-core `search`, and read/write just share it. +Because Tantivy is an embedded library (no Docker/broker needed to +exercise real logic), this actually ran in this environment: + +```sh +cd search +cargo test --quiet +# registry.rs: commit_all_commits_default_and_every_opened_tenant_index +# writes into the default index plus two tenant indices, confirms +# nothing is searchable before commit_all(), then confirms all three ARE +# searchable after -- the write-routing + periodic-commit path +# end-to-end, for real, using the same real-Tantivy-index discipline as +# every other registry.rs/index.rs test. consumer.rs: +# tenant_id_from_headers_* cover the header-extraction helper +# (missing/present/unrelated-header cases) and +# test_tenant_id_header_key_matches_go guards the "tenant_id" literal +# against drifting from ingest/consumer.TenantIDHeaderKey / +# ingest/internal/grpcserver.TenantIDHeaderKey the same way +# ingest/cmd/ingest's own guard test does on the Go side. +``` + +**What's still not built, for either engine**: a live active-tenant +recheck at write time. `chwriter.Registry`'s per-tenant writer map is a +snapshot built once at `enterprise-ingest` startup from +`rbacstore.ListProvisionedDataSources` (active tenants only) -- an +unrecognized `tenant_id` is refused outright, but a tenant deprovisioned +*after* startup keeps writing successfully until the next restart. +`IndexRegistry.resolve()` has no allowlist at all on either the read or +write side -- `search` has no Postgres access to check tenant status +against -- so a still-valid-but-should-be-revoked ingest credential can +cause an orphan Tantivy index directory to be created for a tenant +that's no longer active. Narrow blast radius either way (isolated, not +cross-tenant leakage, reachable only with a real signed credential), but +real and disclosed, not silently accepted -- see +`search/src/registry.rs`'s doc comment on `resolve` and +`/docs/security/threat-model.md`'s "Read this first". **Also not built**: Helm/`docker-compose.yml` do gate *whether* `enterprise-ingest` runs at all (`ingest.requireTenantCredential`, same @@ -688,22 +735,28 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: that split (declarative request vs. imperative provisioning action) is intentional, not the "two disconnected sources of truth" gap this bullet used to describe. -- **Ingest now has a real tenant identity (§13), and ClickHouse - write-routing is built (§14) -- Tantivy write-routing is the one gap - left.** An agent presents a bearer credential (`enterprise-auth - -create-ingest-credential-tenant=`), `ingest/internal/grpcserver. - TenantResolver` validates it (fail-closed) and attaches the resolved - tenant ID to every record as a `tenant_id` Kafka message header. - `enterprise-ingest` reads that header back and routes each record's - ClickHouse write into its own tenant's database (not yet confirmed - against a real ClickHouse in this environment -- see §14). Nothing - reads that header on the Tantivy side yet -- every record still lands - in the one shared Tantivy index no matter what. A newly-provisioned - tenant's ClickHouse database is real, isolated, and actually - populated by write-routed traffic (once confirmed live); its Tantivy - index remains real, isolated at query time, and permanently empty - until Tantivy's write-routing split is built (a real, scoped - follow-up, no longer an undesigned one). +- **Ingest now has a real tenant identity (§13), and both storage + engines' write-routing is built (§14).** An agent presents a bearer + credential (`enterprise-auth -create-ingest-credential-tenant=`), + `ingest/internal/grpcserver.TenantResolver` validates it (fail-closed) + and attaches the resolved tenant ID to every record as a `tenant_id` + Kafka message header. `enterprise-ingest` reads that header back and + routes each record's ClickHouse write into its own tenant's database + (not yet confirmed against a real ClickHouse in this environment -- + see §14). `search/src/consumer.rs` reads the same header and routes + each record into its own tenant's Tantivy index -- genuinely verified + in this environment, unlike the ClickHouse side, since Tantivy needs + no Docker to exercise real logic. Both engines share one remaining, + disclosed gap: neither write path rechecks tenant-active status live + (ClickHouse: a startup-time snapshot, stale until restart; Tantivy: no + allowlist at all, since `search` has no Postgres access) -- narrow + blast radius, not cross-tenant leakage, but real; see §14 and + `/docs/security/threat-model.md`'s "Read this first". A + newly-provisioned tenant's ClickHouse database and Tantivy index are + both now real, isolated, and actually populated by write-routed + traffic (the ClickHouse claim pending live confirmation, the Tantivy + claim already verified) -- what used to be "permanently empty" for + both is no longer true for either. - **Human SSO login now works for both OIDC (§3a) and SAML (§3b)** -- each verified with a real fake IdP (genuine cryptographic signing and verification), not yet a real external IdP or a running diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 77a4ab8..c6fafd3 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -9,15 +9,21 @@ for the full design rationale behind the controls described here. ## Read this first: the single most important open finding -**Updated a third time.** This section originally read "log data +**Updated a fourth time.** This section originally read "log data queried through `POST /query` is not tenant-isolated at all," then "ClickHouse is isolated but Tantivy isn't," then "ingest tags records -with a tenant identity but nothing routes the write." Both ClickHouse -*and* Tantivy connection/index-layer isolation are built on the *read* -path, and ClickHouse write-routing is now built too. What's left is -narrower but still real: **whether a given deployment actually runs the -isolated binaries**, and **Tantivy's write path**, which still has no -per-tenant routing at all. +with a tenant identity but nothing routes the write," then "ClickHouse +write-routing is built but Tantivy's isn't." Both storage engines are +now isolated on both the read and write paths. What's left is narrower: +**whether a given deployment actually runs the isolated binaries** +(deployment-time, not code-level), and two disclosed write-side gaps, +different in kind: ClickHouse's write registry is a startup-time +snapshot of active tenants with no live recheck (a *deprovisioned* +tenant can keep writing successfully until the next `enterprise-ingest` +restart), while Tantivy's write path has no active-tenant allowlist at +all — it opens an index for *any* syntactically-valid `tenant_id` a +record carries, active, deprovisioned, or never real. See below for +both, in full. **ClickHouse (the SQL path) is built.** `enterprise/internal/ tenantprovision` (real `CREATE DATABASE`/`CREATE USER`/`GRANT`) and @@ -70,43 +76,66 @@ provisioned, pointing at the same ClickHouse/Postgres. The Helm chart makes the *default*, chart-managed path correct; it isn't a runtime guard against misconfiguration. -**Ingest now has a real tenant identity, and ClickHouse write-routing is -built — Tantivy write-routing is the one gap left.** `chrunner`/ -`searchclient` prove *read* isolation given tenant-scoped data exists; an -optional `ingest/internal/grpcserver.TenantResolver` closes the "does a -record know which tenant it belongs to" half by validating a per-tenant -bearer credential an agent presents +**Ingest now has a real tenant identity, and both storage engines' +write-routing is built.** `chrunner`/`searchclient` prove *read* +isolation given tenant-scoped data exists; an optional +`ingest/internal/grpcserver.TenantResolver` closes the "does a record +know which tenant it belongs to" half by validating a per-tenant bearer +credential an agent presents (`enterprise-auth -create-ingest-credential-tenant=` mints one; only its SHA-256 hash is ever stored) against a `POST /internal/authorize-ingest` endpoint, and attaching the resolved tenant ID to every record as a `tenant_id` Kafka message header before producing it — fail-closed: once a resolver is configured, a missing or invalid credential refuses the whole batch, never falls back to "no -tenant." `enterprise/cmd/enterprise-ingest` (a second binary, mirroring -`enterprise-api`) now consumes that header: it reuses `ingest/consumer`'s -own flush loop with `enterprise/internal/chwriter.Registry` — a -per-tenant `*clickhousewriter.Writer` registry, built the same way -`chrunner`'s per-tenant connections are — swapped in as the writer, so a -tagged batch's records are grouped by tenant and each group INSERTed -through that tenant's own ClickHouse connection, fail-closed on an -untagged or unprovisioned tenant. Building it surfaced a real gap in an -already-shipped control: `tenantprovision.ProvisionClickHouse`'s grant -was `SELECT`-only (correct for the read-side credential `chrunner` uses, -but `chwriter` reuses the same credential for writes) — every real -per-tenant write would have failed with a permission error until this -was widened to `SELECT, INSERT`. **Not yet confirmed against a real -ClickHouse**, same caveat as the read-side chrunner claim above — the -Docker-free fail-closed tests pass, the live-database tests are written -but skip-gated, see `/docs/phase-4-runbook.md`. What's still missing: -`search`'s independent Redpanda consumer does not read the `tenant_id` -header at all — every ingested record still lands in the one shared -(default) Tantivy index regardless of tenant. A newly-provisioned -tenant's ClickHouse database is now real, isolated, and actually -populated by write-routed agent traffic (once confirmed against a real -cluster); its Tantivy index remains real, isolated, and queryable -through `enterprise-api` — and permanently empty, until Tantivy's own -write-routing split is built, which is now scoped, disclosed remaining -work, not an undesigned gap. +tenant." + +**ClickHouse**: `enterprise/cmd/enterprise-ingest` (a second binary, +mirroring `enterprise-api`) consumes that header: it reuses +`ingest/consumer`'s own flush loop with `enterprise/internal/ +chwriter.Registry` — a per-tenant `*clickhousewriter.Writer` registry, +built the same way `chrunner`'s per-tenant connections are — swapped in +as the writer, so a tagged batch's records are grouped by tenant and +each group INSERTed through that tenant's own ClickHouse connection, +fail-closed on an untagged or unprovisioned tenant. Building it surfaced +a real gap in an already-shipped control: `tenantprovision. +ProvisionClickHouse`'s grant was `SELECT`-only (correct for the +read-side credential `chrunner` uses, but `chwriter` reuses the same +credential for writes) — every real per-tenant write would have failed +with a permission error until this was widened to `SELECT, INSERT`. +**Not yet confirmed against a real ClickHouse**, same caveat as the +read-side chrunner claim above — the Docker-free fail-closed tests pass, +the live-database tests are written but skip-gated, see +`/docs/phase-4-runbook.md`. + +**Tantivy**: `search/src/consumer.rs` now resolves each record's +`tenant_id` header through the *same* `IndexRegistry` the read side +already used (`search/src/registry.rs`), and writes there instead of +always into the default index — genuinely verified in this environment, +same as the read-side Tantivy claim above, since Tantivy is an embedded +library with no Docker dependency. No "second binary" was needed here, +unlike ClickHouse: Tantivy has no grant system to gate a +commercially-licensed credential behind, so `IndexRegistry` already +lived directly in this AGPL-core `search` binary, and read/write simply +share it. + +**Both engines share one open question** — deployment topology, covered +above — and Tantivy specifically has one gap ClickHouse's design doesn't: +`chwriter.Registry`'s map is built once at startup from an +active-tenants-only query, so an unrecognized `tenant_id` is refused +outright; `IndexRegistry.resolve()` (used for both read and write) has +no equivalent allowlist at all, because `search` has no Postgres access +to check tenant status against — a syntactically-valid `tenant_id` on a +still-valid-but-should-be-revoked ingest credential can cause an orphan +index directory to be created for a tenant that's no longer active. +Narrow blast radius (isolated, empty except for that traffic, not +cross-tenant leakage, and reachable only with a real signed credential), +but real, and not closed by this change; see +`search/src/registry.rs`'s doc comment on `resolve`. A newly-provisioned +tenant's ClickHouse database and Tantivy index are both now real, +isolated, and actually populated by write-routed agent traffic (the +ClickHouse claim pending live confirmation, the Tantivy claim already +verified). ## System overview @@ -146,13 +175,14 @@ sentryctl ──▶ api, alerting (Bearer token when SENTRYCTL_TOKEN is set) Ingest path (agent → Redpanda → ingest → ClickHouse, and Redpanda → search → Tantivy): `ingest` resolves and tags each record with a real tenant ID (see "Read this first" above). `enterprise/cmd/enterprise-ingest` -now consumes that tag and routes ClickHouse writes to each tenant's own -database. Tantivy's independent Redpanda consumer (`search/src/ -consumer.rs`) still does not consume the tag at all — every record still -lands in the one shared Tantivy index regardless of tenant. That -narrower gap is still out of scope for what's built so far and is not -separately designed in `/docs/phase-4-isolation-design.md`; named here as -a gap that design doc doesn't yet cover, not just an implementation gap. +consumes that tag and routes ClickHouse writes to each tenant's own +database. `search/src/consumer.rs` (Tantivy's independent Redpanda +consumer) consumes the same tag and routes each record into its own +tenant's index. Both write paths' one remaining gap — no live +active-tenant recheck (ClickHouse: a startup-time snapshot; Tantivy: no +allowlist at all) — is named in "Read this first" above, not separately +designed in `/docs/phase-4-isolation-design.md`; named here as a gap that +design doc doesn't yet cover, not just an implementation gap. ## Module boundary (trust boundary #1) @@ -454,8 +484,8 @@ terms: | Tantivy per-tenant index routing (`search/src/registry.rs`) | **Enforced, verified live** — real Tantivy indices, real cross-tenant probe, all passing | | Tantivy tenant_id resolution (`enterprise/internal/searchclient`) | **Enforced, verified live** — real gRPC wire-level test | | Ingest tenant *identity* (credential validation, tagging) | **Built and tested** — fail-closed `TenantResolver`, `tenant_id` Kafka header attached per record | -| Ingest tenant *write-routing*, ClickHouse | **Built, not yet confirmed against a real ClickHouse** — `enterprise-ingest`/`chwriter.Registry` route each tagged batch to its tenant's own database, fail-closed on an untagged/unprovisioned tenant; Docker-free tests pass, live-database tests are skip-gated | -| Ingest tenant *write-routing*, Tantivy | **Not implemented, now scoped** — every record still lands in the single shared Tantivy index regardless of tenant; `search/src/consumer.rs` consuming the tenant_id header to route the write is real, disclosed remaining work | +| Ingest tenant *write-routing*, ClickHouse | **Built, not yet confirmed against a real ClickHouse** — `enterprise-ingest`/`chwriter.Registry` route each tagged batch to its tenant's own database, fail-closed on an untagged/unprovisioned tenant; Docker-free tests pass, live-database tests are skip-gated. Startup-time active-tenant snapshot, no live recheck — a deprovisioned tenant can keep writing until the next restart | +| Ingest tenant *write-routing*, Tantivy | **Built and genuinely verified** — `search/src/consumer.rs` routes each record into its own tenant's index via `IndexRegistry`, same registry the (already-verified) read side uses; no Docker needed, real tests pass. No active-tenant allowlist at all on write (`search` has no Postgres access) — a syntactically-valid `tenant_id` on a still-valid credential can create an orphan index for a no-longer-active tenant; narrow, disclosed, not cross-tenant leakage | | Deployment actually routing traffic to `enterprise-api` (Helm) | **Enforced** — `api`/`enterprise-api` are mutually exclusive, same flag as RBAC/audit/SSO | | Deployment actually routing traffic to `enterprise-api` (docker-compose) | **Enforced** — `api`/`enterprise-api` are mutually exclusive via `COMPOSE_PROFILES`, same flag choice as Helm's `enterprise.enabled`; verified via `docker compose config`, not an actual `docker compose up` in this environment | | Human SSO login — OIDC | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | diff --git a/enterprise/README.md b/enterprise/README.md index e5d10b5..c911c17 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -182,14 +182,13 @@ silently left out: a cross-origin `fetch` with credentials from `web`'s origin needs it), neither of which is verifiable in this environment without a live backend and a browser session to exercise. -- **Ingest write-routing, for Tantivy** -- `search/src/consumer.rs` (a - completely independent Redpanda consumer, not called through `ingest` - or `enterprise-ingest` at all) still writes every record into the one - shared (default) Tantivy index, regardless of tenant. The ClickHouse - half is now built (see "Ingest write-routing" below); Tantivy's is - real, disclosed, separate follow-up work -- a different codebase - (Rust) and a different consumer process, not just "the same fix - applied twice." + +Ingest write-routing (both ClickHouse and Tantivy) is no longer on this +list -- see "Ingest write-routing (ClickHouse)" below and +`/search/README.md`'s "Per-tenant indices" section for Tantivy, which +needed no code in this module at all: `search`'s `IndexRegistry` already +lived in AGPL core, so its write side didn't need an `enterprise/` +counterpart the way ClickHouse's did. Deployment-topology routing (does traffic actually reach `enterprise-api` instead of `api`) is no longer deferred -- both `deploy/helm/sentry` and @@ -307,8 +306,13 @@ ingest must never import enterprise/) is the client side of `internal/ authhandler`'s new `POST /internal/authorize-ingest` -- see "Ingest tenant identity" above. -Future additions: per-tenant write-routing for ingest (ClickHouse and -Tantivy both) -- see "Ingest tenant identity" above. +Per-tenant write-routing for ingest is built for both ClickHouse (this +module's `cmd/enterprise-ingest` + `internal/chwriter`, see "Ingest +write-routing (ClickHouse)" above) and Tantivy (`search/src/consumer.rs` ++ `search/src/registry.rs`, entirely in AGPL core -- see +`/search/README.md`'s "Per-tenant indices" section, since Tantivy's lack +of a grant system meant there was never an import-boundary reason to put +any of it here). ## Why OIDC and SAML aren't hand-rolled diff --git a/search/README.md b/search/README.md index 09a92ca..a7630fa 100644 --- a/search/README.md +++ b/search/README.md @@ -38,7 +38,7 @@ ClickHouse's `logs.record_id` column (see only ever returns IDs, never row data — it stays a pure text index, not a second copy of the row. -## Per-tenant indices (Phase 4) — read-side only +## Per-tenant indices (Phase 4) — read and write `SearchRequest.tenant_id` (empty by default) selects which index `src/registry.rs`'s `IndexRegistry` searches: empty resolves to the @@ -50,13 +50,31 @@ convention). `tenant_id` is set only by a trusted server-side caller (`enterprise/internal/searchclient`, from the authenticated request identity) — never a value a browser/client controls. -**This is read-side isolation only.** `consumer.rs`'s Redpanda consumer -— the only thing that ever *writes* into an index — still only ever -writes into the single default index, because `ingest`/the log-record -schema itself carries no tenant concept yet. A newly-opened tenant index -starts, and stays, empty until something upstream of this service -becomes tenant-aware on the write side too — a real, disclosed gap, not -an oversight; see `/docs/security/threat-model.md`. +`consumer.rs`'s Redpanda consumer resolves the *same* registry now, +keyed by each Kafka message's `tenant_id` header — attached server-side +by `ingest/internal/grpcserver` after validating an agent's per-tenant +credential, mirroring exactly how `enterprise/cmd/enterprise-ingest` +routes the ClickHouse write side (see `/enterprise/README.md`'s "Ingest +write-routing" section). No import boundary to work around here, unlike +Go: `IndexRegistry` already lives in this AGPL-core binary, so both the +read and write paths share one registry directly, with no "second +binary" needed. The periodic Tantivy commit (`COMMIT_INTERVAL_MS`) now +commits every tenant index that's actually seen a write, plus the +default index, via `IndexRegistry::commit_all`, not just one index. + +**One residual gap, disclosed rather than fixed here**: unlike the read +side (gated by `enterprise/internal/searchclient`'s `TenantChecker`, +which refuses to search a tenant that isn't `active` in `rbacstore`) and +unlike ClickHouse's write side (`chwriter.Registry`, built from an +active-tenants-only snapshot at startup, so an unrecognized tenant has +no writer at all), this consumer's `registry.resolve()` call has no +active-tenant gate — this process has no Postgres access to check +against, the same reason `IndexRegistry` couldn't do the mid-provisioning +check itself before `TenantChecker` was added for the read side. A +still-valid (not yet revoked) ingest credential for a tenant that's no +longer active can cause an index directory to be created for it here. +See `src/registry.rs`'s doc comment on `resolve` for the full writeup, +including why closing it fully isn't scoped yet. ## Offset tracking: why this isn't a Kafka consumer group @@ -116,12 +134,20 @@ before-commit/after-commit visibility boundary. `registry.rs`'s tests are the same shape and cover the actual adversarial claim: real per- tenant indices, real documents, and a search scoped to one tenant returning zero results for another tenant's matching document -(`tenant_index_is_isolated_from_default_and_other_tenants`) — all of -this, unlike almost everything else in Phase 4, was genuinely run in -the environment this was built in, not just written. `consumer.rs` (the -rskafka wiring) is not unit-tested — that needs a real Redpanda, same +(`tenant_index_is_isolated_from_default_and_other_tenants`), plus +`commit_all_commits_default_and_every_opened_tenant_index` proving the +write-routing commit path actually makes every tenant's buffered writes +searchable, not just the default index's — all of this, unlike almost +everything else in Phase 4, was genuinely run in the environment this +was built in, not just written. `consumer.rs`'s Kafka/rskafka wiring +itself is still not unit-tested — that needs a real Redpanda, same category of gap as `/ingest`'s `kafka.Reader`/`kafka.Writer` wiring, and -is exercised by the docker-compose end-to-end flow instead. +is exercised by the docker-compose end-to-end flow instead — but the +pure `tenant_id_from_headers` header-extraction helper it uses is +factored out and unit-tested the same way `ingest/consumer`'s Go +equivalent is, including a guard test +(`test_tenant_id_header_key_matches_go`) against the header-key literal +drifting from the Go side's. ```sh # from the repo root, not search/ diff --git a/search/src/consumer.rs b/search/src/consumer.rs index 8c45599..174b2c8 100644 --- a/search/src/consumer.rs +++ b/search/src/consumer.rs @@ -2,13 +2,25 @@ use anyhow::{Context, Result}; use prost::Message; use rskafka::client::partition::UnknownTopicHandling; use rskafka::client::ClientBuilder; +use std::collections::BTreeMap; use std::sync::Arc; use tokio::sync::Mutex; use crate::config::Config; -use crate::index::SearchIndex; use crate::logsv1; use crate::offsets::OffsetStore; +use crate::registry::IndexRegistry; + +/// Kafka message header a resolved tenant ID rides in, attached by +/// ingest's gRPC front end. Mirrors `ingest/internal/grpcserver. +/// TenantIDHeaderKey` / `ingest/consumer.TenantIDHeaderKey` -- those two +/// Go packages duplicate the same literal rather than importing across a +/// producer/consumer boundary (see their doc comments), and this Rust +/// consumer is a third independent reader of the same header, so it +/// duplicates the literal too. `TestTenantIDHeaderKeyMatchesGo` below +/// guards against drift the same way `ingest/cmd/ingest`'s +/// `TestTenantIDHeaderKeyConstantsMatch` does on the Go side. +const TENANT_ID_HEADER_KEY: &str = "tenant_id"; /// Reads the same `sentry.logs.raw` topic ingest's ClickHouse-writer /// consumer reads, as an independent consumer group in spirit (its own @@ -18,7 +30,7 @@ use crate::offsets::OffsetStore; /// discovered dynamically, since it has to match what /// /transport/provision-topics.sh actually created anyway (documented /// cross-component contract, same as the topic name already is). -pub async fn run(cfg: Arc, index: Arc, partition_count: i32) -> Result<()> { +pub async fn run(cfg: Arc, registry: Arc, partition_count: i32) -> Result<()> { let client = ClientBuilder::new(cfg.redpanda_brokers.clone()) .build() .await @@ -32,14 +44,16 @@ pub async fn run(cfg: Arc, index: Arc, partition_count: i32 // Periodic Tantivy commit, batched for throughput the same way // ingest's ClickHouse writer batches inserts rather than inserting - // per-record. + // per-record. Commits every tenant index a write has actually been + // routed to (plus the default index), not just one -- see + // registry.rs's commit_all doc comment. let commit_interval = cfg.commit_interval; - let index_for_commit = Arc::clone(&index); + let registry_for_commit = Arc::clone(®istry); tokio::spawn(async move { let mut ticker = tokio::time::interval(commit_interval); loop { ticker.tick().await; - if let Err(e) = index_for_commit.commit().await { + if let Err(e) = registry_for_commit.commit_all().await { tracing::error!(error = %e, "periodic tantivy commit failed"); } } @@ -49,11 +63,11 @@ pub async fn run(cfg: Arc, index: Arc, partition_count: i32 for partition in 0..partition_count { let start_offset = offsets.lock().await.get(partition); let client = Arc::clone(&client); - let index = Arc::clone(&index); + let registry = Arc::clone(®istry); let offsets = Arc::clone(&offsets); let topic = cfg.redpanda_topic.clone(); handles.push(tokio::spawn(async move { - consume_partition(client, topic, partition, start_offset, index, offsets).await + consume_partition(client, topic, partition, start_offset, registry, offsets).await })); } @@ -65,13 +79,26 @@ pub async fn run(cfg: Arc, index: Arc, partition_count: i32 Ok(()) } +/// Extracts the `tenant_id` header's value, or "" if absent -- an empty +/// string is exactly what `IndexRegistry::resolve` treats as "route to +/// the default index," so an untagged record (every Phase 0-3 message, +/// and any Phase 4 message from a deployment that never turned on +/// `ingest`'s `TenantResolver`) keeps landing in the same shared index +/// it always has. Mirrors `ingest/consumer.tenantIDFromHeaders` exactly. +fn tenant_id_from_headers(headers: &BTreeMap>) -> String { + headers + .get(TENANT_ID_HEADER_KEY) + .map(|v| String::from_utf8_lossy(v).into_owned()) + .unwrap_or_default() +} + #[allow(clippy::too_many_arguments)] async fn consume_partition( client: Arc, topic: String, partition: i32, start_offset: i64, - index: Arc, + registry: Arc, offsets: Arc>, ) -> Result<()> { let partition_client = client @@ -115,8 +142,25 @@ async fn consume_partition( continue; } + let tenant_id = tenant_id_from_headers(&record_and_offset.record.headers); + let index = match registry.resolve(&tenant_id).await { + Ok(index) => index, + Err(e) => { + // Shouldn't normally happen -- ingest/grpcserver only + // ever attaches a tenant_id it validated against a + // real credential -- but an unsafe/malformed + // tenant_id is a hard skip, never a silent fall-back + // to the default or any other tenant's index. See + // registry.rs's doc comment for the one residual gap + // this consumer doesn't close (no active-tenant + // check, since this process has no Postgres access). + tracing::error!(error = %e, record_id = %rec.record_id, tenant_id, "skipping record: failed to resolve tenant index"); + continue; + } + }; + if let Err(e) = index.upsert(&rec.record_id, &rec.message).await { - tracing::error!(error = %e, record_id = %rec.record_id, "failed to index record"); + tracing::error!(error = %e, record_id = %rec.record_id, tenant_id, "failed to index record"); } } @@ -132,3 +176,41 @@ async fn consume_partition( } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn tenant_id_from_headers_returns_empty_string_when_absent() { + let headers = BTreeMap::new(); + assert_eq!(tenant_id_from_headers(&headers), ""); + } + + #[test] + fn tenant_id_from_headers_extracts_the_tenant_id_header() { + let mut headers = BTreeMap::new(); + headers.insert(TENANT_ID_HEADER_KEY.to_string(), b"acme".to_vec()); + assert_eq!(tenant_id_from_headers(&headers), "acme"); + } + + #[test] + fn tenant_id_from_headers_ignores_unrelated_headers() { + let mut headers = BTreeMap::new(); + headers.insert("some-other-header".to_string(), b"acme".to_vec()); + assert_eq!(tenant_id_from_headers(&headers), ""); + } + + /// Guards against the exact literal drift + /// `ingest/cmd/ingest`'s `TestTenantIDHeaderKeyConstantsMatch` guards + /// against on the Go side -- three independent readers/writers of the + /// same Kafka header (`ingest/internal/grpcserver` producing, + /// `ingest/consumer` and this file both consuming) duplicate the same + /// literal by design rather than sharing an import across a + /// producer/consumer or language boundary, so nothing but a test + /// catches them drifting apart. + #[test] + fn test_tenant_id_header_key_matches_go() { + assert_eq!(TENANT_ID_HEADER_KEY, "tenant_id"); + } +} diff --git a/search/src/main.rs b/search/src/main.rs index 3667889..a3ea7c9 100644 --- a/search/src/main.rs +++ b/search/src/main.rs @@ -35,11 +35,10 @@ async fn main() -> Result<()> { let index = Arc::new( SearchIndex::open_or_create(&cfg.index_path).context("opening tantivy index")?, ); - // Per-tenant indices (Phase 4) are resolved on demand by - // IndexRegistry, opened under cfg.tenants_index_path -- see - // registry.rs's doc comment for what this does and doesn't isolate - // yet (read-side only; the consumer below still only ever writes - // into the single default `index` above). + // Per-tenant indices are resolved on demand by IndexRegistry, opened + // under cfg.tenants_index_path -- see registry.rs's doc comment for + // what this isolates (both read and write now) and the one residual + // gap it doesn't close. let registry = Arc::new(IndexRegistry::new(Arc::clone(&index), cfg.tenants_index_path.clone())); let partition_count: i32 = std::env::var("REDPANDA_TOPIC_PARTITIONS") @@ -48,9 +47,9 @@ async fn main() -> Result<()> { .unwrap_or(DEFAULT_PARTITION_COUNT); let consumer_cfg = Arc::clone(&cfg); - let consumer_index = Arc::clone(&index); + let consumer_registry = Arc::clone(®istry); let consumer_handle = tokio::spawn(async move { - if let Err(e) = consumer::run(consumer_cfg, consumer_index, partition_count).await { + if let Err(e) = consumer::run(consumer_cfg, consumer_registry, partition_count).await { tracing::error!(error = %e, "redpanda consumer exited with error"); } }); diff --git a/search/src/registry.rs b/search/src/registry.rs index 859aa10..dad3437 100644 --- a/search/src/registry.rs +++ b/search/src/registry.rs @@ -6,26 +6,45 @@ use tokio::sync::RwLock; use crate::index::SearchIndex; -/// Resolves a `tenant_id` (from `SearchRequest.tenant_id`, per -/// search.proto's doc comment: set only by a trusted server-side caller -/// -- `enterprise/internal/searchclient`, from the authenticated request -/// identity, never a value a browser/client controls directly) to its -/// own `SearchIndex`, opening one on demand under `/ -/// ` the first time it's requested. +/// Resolves a `tenant_id` to its own `SearchIndex`, opening one on +/// demand under `/` the first time it's +/// requested. Used on both sides now: the read side +/// (`SearchRequest.tenant_id`, per search.proto's doc comment -- set +/// only by a trusted server-side caller, `enterprise/internal/ +/// searchclient`, from the authenticated request identity, never a +/// value a browser/client controls directly) and the write side +/// (`consumer.rs`, from a Kafka message's `tenant_id` header, attached +/// server-side by `ingest/internal/grpcserver` after validating an +/// agent's per-tenant credential -- never a value the agent's message +/// body controls directly either). /// /// An empty `tenant_id` resolves to `default_index` -- the single index -/// path every Phase 0-3 deployment already uses. This is intentionally -/// where the per-tenant story stops today: `consumer.rs`'s Redpanda -/// consumer (the only thing that ever *writes* into an index) only ever -/// writes into `default_index`, because `ingest`/the log-record schema -/// itself carries no tenant concept yet -- see -/// /docs/security/threat-model.md's "ingest path... carries no tenant -/// concept" caveat. A tenant's own index therefore starts, and stays, -/// empty until something upstream of this service becomes tenant-aware -/// on the write side too. What this registry proves is that *read* -/// isolation is real once there's tenant-scoped data to isolate -- -/// exactly the same scope boundary enterprise/internal/chrunner drew for -/// ClickHouse (see that package's doc comment). +/// path every Phase 0-3 deployment, and every untagged record, still +/// uses. +/// +/// **Known residual gap, disclosed rather than silently accepted**: +/// unlike the read side (gated by `enterprise/internal/searchclient`'s +/// `TenantChecker`, which refuses to even issue a search for a tenant +/// that isn't `active` in `rbacstore`) and unlike ClickHouse's write +/// side (`enterprise/internal/chwriter.Registry`, built once at startup +/// from `rbacstore.ListProvisionedDataSources` -- `active` tenants +/// only, so an unrecognized `tenant_id` has no writer and the whole +/// batch is refused), this registry's `resolve` has no equivalent gate +/// on the write path: `consumer.rs` calls it directly, with no Postgres +/// access to check tenant status against, the same reason this +/// module's doc comment used to give for the old read-side gap +/// `TenantChecker` was built to close. A syntactically-valid `tenant_id` +/// on an ingest credential that's still valid but should have been +/// revoked (deprovisioning does not yet revoke `ingest_credentials` +/// rows -- see `/CLAUDE.md`'s Phase 4 non-goals) can therefore cause an +/// index directory to be silently created here for a tenant that isn't +/// really active. The blast radius is narrow -- an orphan, isolated, +/// empty-except-for-that-tenant's-own-traffic index directory, not +/// cross-tenant data exposure, and only reachable with a real signed +/// ingest credential, not by an arbitrary caller -- but it is real, not +/// hypothetical. Closing it fully would mean giving `search` (AGPL +/// core, no `enterprise/` import allowed) some way to learn which +/// tenants are actually active; not designed yet. pub struct IndexRegistry { default_index: Arc, tenants_root: PathBuf, @@ -73,6 +92,36 @@ impl IndexRegistry { .or_insert_with(|| Arc::new(opened)); Ok(Arc::clone(idx)) } + + /// Commits `default_index` plus every tenant index opened so far -- + /// the periodic-commit ticker in consumer.rs calls this instead of + /// committing a single index, now that a batch of records can span + /// several tenants' indices. An index that was never opened (no + /// write ever routed to it) is never touched, matching `resolve`'s + /// own on-demand-open behavior -- nothing to commit for a tenant + /// with no traffic yet. One tenant's commit failing does not stop + /// the others from being attempted -- a single broken index + /// shouldn't stall every other tenant's documents from becoming + /// searchable. Returns the last error encountered, if any, after + /// every index has been tried. + pub async fn commit_all(&self) -> Result<()> { + let mut last_err = self.default_index.commit().await.context("committing default index").err(); + let tenants = self.tenants.read().await; + for (tenant_id, idx) in tenants.iter() { + if let Err(e) = idx + .commit() + .await + .with_context(|| format!("committing index for tenant {tenant_id:?}")) + { + tracing::error!(error = %e, tenant_id, "failed to commit tenant tantivy index"); + last_err = Some(e); + } + } + match last_err { + Some(e) => Err(e), + None => Ok(()), + } + } } /// Mirrors enterprise/internal/tenantprovision's tenantIdentifierPattern @@ -182,4 +231,37 @@ mod tests { assert!(Arc::ptr_eq(&results[0], r), "expected every concurrent resolve to return the same Arc"); } } + + #[tokio::test] + async fn commit_all_commits_default_and_every_opened_tenant_index() { + let (registry, default_index, _dir) = new_test_registry(); + + default_index.upsert("default-1", "hello").await.unwrap(); + let acme_idx = registry.resolve("acme").await.unwrap(); + acme_idx.upsert("acme-1", "hello").await.unwrap(); + let globex_idx = registry.resolve("globex").await.unwrap(); + globex_idx.upsert("globex-1", "hello").await.unwrap(); + + // Nothing committed yet -- none of the three should be + // searchable, proving this test would actually catch commit_all + // silently skipping an index rather than passing vacuously. + assert!(default_index.search("hello", 10).unwrap().is_empty()); + assert!(acme_idx.search("hello", 10).unwrap().is_empty()); + assert!(globex_idx.search("hello", 10).unwrap().is_empty()); + + registry.commit_all().await.unwrap(); + + assert_eq!(default_index.search("hello", 10).unwrap(), vec!["default-1"]); + assert_eq!(acme_idx.search("hello", 10).unwrap(), vec!["acme-1"]); + assert_eq!(globex_idx.search("hello", 10).unwrap(), vec!["globex-1"]); + } + + #[tokio::test] + async fn commit_all_is_a_noop_for_tenants_never_resolved() { + // A tenant with no traffic yet has no directory created at all -- + // commit_all must not try to open/commit anything for it. + let (registry, _default, dir) = new_test_registry(); + registry.commit_all().await.unwrap(); + assert!(!dir.path().join("tenants").join("never-seen").exists()); + } }