From ba2276aa1ad5c524fefb13f09048a528a0999a9d Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 13 Aug 2026 23:16:22 -0700 Subject: [PATCH] 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. --- CLAUDE.md | 24 ++- api/queryapi/tenant_isolation_gap_test.go | 59 +++--- docs/architecture.md | 37 ++-- docs/phase-4-runbook.md | 76 +++++-- docs/security/threat-model.md | 118 +++++------ enterprise/README.md | 30 ++- enterprise/cmd/enterprise-api/main.go | 23 +-- enterprise/go.mod | 4 +- .../internal/searchclient/searchclient.go | 70 +++++++ .../searchclient/searchclient_test.go | 114 +++++++++++ proto/sentry/search/v1/search.pb.go | 24 ++- proto/sentry/search/v1/search.proto | 11 ++ search/README.md | 45 ++++- search/src/config.rs | 14 ++ search/src/grpc.rs | 20 +- search/src/main.rs | 10 +- search/src/registry.rs | 185 ++++++++++++++++++ 17 files changed, 696 insertions(+), 168 deletions(-) create mode 100644 enterprise/internal/searchclient/searchclient.go create mode 100644 enterprise/internal/searchclient/searchclient_test.go create mode 100644 search/src/registry.rs diff --git a/CLAUDE.md b/CLAUDE.md index 519e3ef..32c9d34 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -159,15 +159,21 @@ tenant/role from `tenant_memberships`) — genuinely verified, unlike the ClickHouse pieces, via a real fake IdP that signs and verifies actual RS256 tokens (`loginhandler_test.go`, all passing), though never tried against a real external IdP or through a running `enterprise-auth` -container. Two things still keep this phase from being done: SAML login -(protocol wiring exists, no ACS handler calls it, following OIDC's now --built pattern), and Tantivy/free-text queries have no per-tenant index -routing at all (`enterprise-api` closes the ClickHouse half of tenant -isolation, not the Tantivy half) — plus a deployment gap worth naming -explicitly: nothing yet forces or even flags whether a given deployment -is actually running the isolated binary (`enterprise-api`) versus the -plain single-tenant one (`api`); both still exist and nothing currently -prevents mixing them up. Full accounting: +container. Tantivy per-tenant index routing is now built too +(`search/src/registry.rs` + `enterprise/internal/searchclient`) — +**genuinely verified**, like the OIDC login flow: Tantivy is an embedded +library, not a networked service, so the isolation probe (three tenants, +same search term, scoped search returns only that tenant's document) +actually ran in this environment, no Docker needed. What still keeps +this phase from being done: SAML login (protocol wiring exists, no ACS +handler calls it, following OIDC's now-built pattern), ingest itself has +no tenant concept for either storage engine (every record lands in the +one shared ClickHouse database and Tantivy index no matter what — +undesigned, not just unbuilt), and a deployment-topology gap that's now +the single largest one: nothing yet forces or even flags whether a given +deployment is actually running the isolated binary (`enterprise-api`) +versus the plain single-tenant one (`api`); both still exist and nothing +currently prevents mixing them up. 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/api/queryapi/tenant_isolation_gap_test.go b/api/queryapi/tenant_isolation_gap_test.go index 38a8521..3e40b2f 100644 --- a/api/queryapi/tenant_isolation_gap_test.go +++ b/api/queryapi/tenant_isolation_gap_test.go @@ -3,46 +3,35 @@ // "Verification plan for this design specifically" section names for // Phase 4 task 8 have a permanent, grep-able home in the test tree. // -// Item 1 (fully-qualified cross-tenant raw SQL) is no longer blocked: -// enterprise/internal/tenantprovision and enterprise/internal/chrunner -// now exist, and both have real, passing (when run against a live -// ClickHouse) tests for exactly this probe -- -// enterprise/internal/tenantprovision/tenantprovision_test.go's -// TestProvisionedUserCannotReadOtherTenantDatabase (at the raw -// ClickHouse-user layer) and enterprise/internal/chrunner/ -// chrunner_test.go's TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL -// (through the actual query-execution code path api/queryapi.Handler -// calls in production, when fronted by enterprise/cmd/enterprise-api -// instead of plain api/cmd/api). Nothing to assert here anymore for -// item 1 -- see those two tests instead. +// Three of the four are no longer blocked: // -// Items 2-4 remain blocked, for the reasons each Skip below states. -// Note the scope boundary this leaves: even with chrunner wired in, -// there is still exactly one shared Tantivy index for every tenant -// (enterprise/internal/searchclient, the Tantivy-side equivalent of -// chrunner, is unbuilt) -- see /docs/security/threat-model.md. +// - Item 1 (fully-qualified cross-tenant raw SQL): +// enterprise/internal/tenantprovision/tenantprovision_test.go's +// TestProvisionedUserCannotReadOtherTenantDatabase (raw ClickHouse-user +// layer) and enterprise/internal/chrunner/chrunner_test.go's +// TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL (the actual +// query-execution code path, via enterprise/cmd/enterprise-api). +// - Item 2 (system.query_log/system.tables/SHOW DATABASES): +// enterprise/internal/tenantprovision/tenantprovision_test.go's +// TestProvisionedUserCannotReadSystemTables. +// - Item 3 (Tantivy cross-tenant search): search/src/registry.rs's +// tenant_index_is_isolated_from_default_and_other_tenants (Rust, the +// actual per-tenant index registry) and enterprise/internal/ +// searchclient/searchclient_test.go (the Go client that resolves +// tenant_id from request identity, wire-level verified against a +// real in-process gRPC server). +// +// Item 4 remains blocked, for the reason its Skip below states. Note the +// scope boundary all three closed items share: they prove *read* +// isolation given tenant-scoped data exists -- they do not prove +// ingest/write-path tenancy, which doesn't exist yet (every record +// ingest produces lands in the single shared ClickHouse database and +// Tantivy index regardless of tenant) -- see +// /docs/security/threat-model.md. package queryapi import "testing" -func TestAdversarial_ClickHouseUserCannotReadSystemTables(t *testing.T) { - t.Skip("BLOCKED on enterprise/internal/tenantprovision: needs a real per-tenant " + - "ClickHouse user to attempt `SELECT * FROM system.query_log`, " + - "`system.tables`, `SHOW DATABASES` against, and confirm system.* " + - "access was actually revoked (not just assumed from ClickHouse's " + - "default template -- task 2's finding was that this is " + - "version-dependent and must be checked live, not read from docs). " + - "See /docs/phase-4-isolation-design.md's verification plan, item 2.") -} - -func TestAdversarial_TantivySearchExcludesOtherTenantsMatchingResults(t *testing.T) { - t.Skip("BLOCKED on enterprise/internal/searchclient: needs two real " + - "per-tenant Tantivy indices, one seeded with a term, to confirm a " + - "search scoped to the other tenant returns zero hits for that term " + - "even though the term exists in the other index. See " + - "/docs/phase-4-isolation-design.md's verification plan, item 3.") -} - func TestAdversarial_EvaluatorTickMidProvisioningIsRefusedNotServed(t *testing.T) { t.Skip("BLOCKED on enterprise/internal/tenantprovision's ordered " + "provisioning state machine (CREATE USER -> GRANT -> mark active): " + diff --git a/docs/architecture.md b/docs/architecture.md index 95c0dac..7d05f86 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -76,10 +76,10 @@ This split is not to be changed without discussion — see CLAUDE.md. | `transport` | Redpanda docker-compose + topic provisioning scripts. No application code. | | `ingest` (Go) | gRPC server accepting agent connections; produces normalized OTel-log-like records to Redpanda; separate consumer reads from Redpanda and batch-writes to ClickHouse. No tenant concept — every record lands in the one shared `logs` table regardless of source (see "Tenant isolation" below). | | `storage` | ClickHouse schema migrations + docker-compose for local/homelab. | -| `search` (Rust, Phase 1) | Consumes the same Redpanda topic `ingest` does (own offset tracking), builds a Tantivy full-text index over `message`, serves matches over gRPC. One shared index for every tenant today — see "Tenant isolation" below. | +| `search` (Rust, Phase 1) | Consumes the same Redpanda topic `ingest` does (own offset tracking), builds a Tantivy full-text index over `message`, serves matches over gRPC. Writes always go to one shared (default) index (`ingest` isn't tenant-aware); reads can be scoped per-tenant via `SearchRequest.tenant_id` and `src/registry.rs`'s `IndexRegistry` (Phase 4) — see "Tenant isolation" below. | | `api` (Go) | gRPC + REST gateway. `POST /query` compiles pipe-syntax or raw SQL to one IR, executed across ClickHouse/Tantivy (`/docs/query-language-design.md`). `internal/dashboards` is CRUD only — panel query execution happens client-side, reusing `/query`. `internal/authz` (Phase 4) enforces RBAC via a network call to `enterprise-auth`, never an import. | | `alerting` (Go, Phase 3) | Evaluates alert rules on an interval, calls `api`'s `POST /query` (via a `RoleService` credential once Phase 4 auth is configured — see `/docs/phase-4-isolation-design.md`'s alerting↔api gap), delivers firing/resolved notifications (webhook/Slack/PagerDuty). | -| `enterprise` (Go, commercial license, Phase 4) | OIDC login (`internal/loginhandler`'s `/auth/oidc/login`+`/auth/oidc/callback`, real IdP round trip, verified with a fake IdP but not a real external one), RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`), per-tenant ClickHouse provisioning (`internal/tenantprovision`) and query routing (`internal/chrunner`), and `cmd/enterprise-api` — a second binary combining core's `api/queryapi`/`api/dashboards` handlers with these tenant-aware implementations. Never imported by core — see "Licensing boundary" below. Does **not** yet include per-tenant Tantivy routing or SAML's login ACS handler (protocol mechanics only) — see `/docs/security/threat-model.md`. | +| `enterprise` (Go, commercial license, Phase 4) | OIDC login (`internal/loginhandler`'s `/auth/oidc/login`+`/auth/oidc/callback`, real IdP round trip, verified with a fake IdP but not a real external one), RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`), per-tenant ClickHouse provisioning (`internal/tenantprovision`) and query routing (`internal/chrunner`), and `cmd/enterprise-api` — a second binary combining core's `api/queryapi`/`api/dashboards` handlers with these tenant-aware implementations. Never imported by core — see "Licensing boundary" below. Also `internal/searchclient` (per-tenant Tantivy routing, wired the same way into `search`). Does **not** yet include SAML's login ACS handler (protocol mechanics only) — see `/docs/security/threat-model.md`. | | `web` (SvelteKit, static build) | Query bar, dashboards, alerts, and (Phase 4) a settings page that renders SSO status via a runtime capability check (`GET /auth/features`) rather than bundling enterprise-licensed components. | | `cli` (`sentryctl`) | `ping`, `query`, `dashboards` (list/get/apply), `alerts` (list/get/apply). `$SENTRYCTL_TOKEN`, if set, is forwarded as a Bearer credential (Phase 4). | | `deploy` | A Helm chart covering every `docker-compose.yml` service, plus (Phase 4) a small Go Operator managing one CRD (`Tenant`) that provisions a per-tenant ClickHouse credential Secret. Never applied to a live cluster in the environment this was built in — see `/deploy/README.md`'s verification section before trusting it. | @@ -129,20 +129,33 @@ escape hatch is opaque to any compiler-injected filter. unchanged, with its single shared connection — nothing forces a deployment to run `enterprise-api` instead, and nothing flags it if it doesn't. -- **Tantivy connection-layer isolation is not built.** `search`'s gRPC - service and `proto/sentry/search/v1/search.proto`'s `SearchRequest` - still carry no tenant field. Every tenant's free-text queries hit the - same shared Tantivy index regardless of which binary serves the - request. +- **Tantivy index-layer isolation is built, and verified.** + `search/src/registry.rs`'s `IndexRegistry` resolves + `SearchRequest.tenant_id` (added to `proto/sentry/search/v1/ + search.proto`) to its own on-disk index, opened on demand; + `enterprise/internal/searchclient` sets that field from the + authenticated request identity, the same "read from ctx, fail closed" + shape `chrunner` uses. Unlike the ClickHouse pieces, this one actually + 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. +- Neither storage engine's isolation extends to *ingest*: every record + `ingest` produces lands in the one shared ClickHouse database and the + one shared (default) Tantivy index regardless of tenant. A + newly-provisioned tenant's database/index are real and isolated at + query time — and permanently empty until something upstream of + `chrunner`/`searchclient` becomes tenant-aware on the write side, + which is undesigned, not merely unbuilt. - `deploy/operator`'s `Tenant` CRD still manages only the K8s-side artifact (a credential Secret); the Helm chart has no service definition for `enterprise-api` yet. -Building `enterprise/internal/searchclient` (the Tantivy-side sibling of -`chrunner`) and giving the deployment topology (Helm chart, or at least -clear documentation) an actual way to route traffic to `enterprise-api` -instead of `api` are the two largest remaining gaps between this system -and the isolation model it was designed to have. +The deployment-topology gap — giving the system an actual way to route +traffic to `enterprise-api` instead of `api` (a Helm service, or at +minimum a documented, enforced convention) — is now the single largest +remaining gap between this system and the isolation model it was +designed to have; both storage engines' connection/index-layer +mechanisms themselves are built. ## Licensing boundary diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index 9ea7a02..84d36a1 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -243,11 +243,15 @@ provisions ClickHouse. ## 8. `enterprise-api`: real per-tenant ClickHouse isolation -This is new since this runbook was first written — `enterprise/internal/ -tenantprovision` and `enterprise/internal/chrunner` now exist, closing -the headline gap §"Known gaps" below used to describe as completely -unbuilt. It's still a second binary you have to choose to run, though — -see `/docs/security/threat-model.md`'s "Read this first" section. +`enterprise/internal/tenantprovision` and `enterprise/internal/chrunner` +close the ClickHouse half of the headline gap §"Known gaps" below used +to describe as completely unbuilt. It's still a second binary you have +to choose to run, though — see `/docs/security/threat-model.md`'s "Read +this first" section. With OIDC login now built (§3a), a real +`curl -X POST http://localhost:8083/query` walkthrough as a logged-in +tenant is *possible* now, but still needs the manual `tenant_memberships` +bootstrap from §3a — a full end-to-end curl walkthrough isn't included +here yet. ```sh docker compose build enterprise-api @@ -257,14 +261,11 @@ docker compose up -d enterprise-api curl -s http://localhost:8083/healthz ``` -There's still no OIDC/SAML login handler and no CLI for minting a human -session token (see `/docs/security/threat-model.md`) -- so a real -`curl -X POST http://localhost:8083/query` walkthrough as tenant acme -isn't possible yet. Confirm isolation end to end against the live stack (this is the same +Confirm isolation end to end against the live stack (this is the same assertion `enterprise/internal/chrunner/chrunner_test.go`'s `TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL` makes, run here -as an integration test instead of a curl walkthrough since there's no -login flow to drive it through curl yet): +as an integration test instead of a curl walkthrough since a full login +walkthrough isn't scripted yet): ```sh docker run --rm --network sentry_default -v $(pwd)/enterprise:/src -w /src \ @@ -285,17 +286,50 @@ pass) and `TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL` (item 1, closed through the actual production code path, not just tenantprovision's raw grants). +## 9. Tantivy per-tenant isolation — no Docker needed, actually run this one + +Unlike everything above, this one doesn't need a live stack at all — +Tantivy is an embedded library, not a networked service, so both halves +(the Rust index registry and the Go client that talks to it) can be +verified with nothing but a local toolchain: + +```sh +cd search +cargo build +cargo clippy --all-targets -- -D warnings +cargo test +# expect 14 tests passing, including +# registry::tests::tenant_index_is_isolated_from_default_and_other_tenants +# -- item 3 of /docs/phase-4-isolation-design.md's verification plan. + +cd ../enterprise +go test ./internal/searchclient/... -v +# real in-process gRPC server, confirms SearchRequest.tenant_id is set +# correctly and that a request with no/invalid tenant identity is refused. +``` + +This is the one piece of Phase 4's tenant isolation work that has +**actually been run and confirmed passing** in an environment without +Docker access, alongside `enterprise/internal/loginhandler`'s OIDC +tests (§3a) — both are unusually strong evidence precisely because they +needed no infrastructure this environment lacked. + ## Known gaps (do not treat this phase as done without reading these) Full accounting: `/docs/security/threat-model.md`. Headline items: -- **ClickHouse isolation exists but is opt-in.** `enterprise-api` - (§8) gives real per-tenant ClickHouse isolation, but plain `api` - (still the default in `docker-compose.yml`/`web`'s base URL) has none, - and nothing flags which one a given deployment is actually running. -- **No Tantivy/free-text isolation at all**, regardless of which binary - serves the request -- `enterprise/internal/searchclient` (chrunner's - Tantivy-side sibling) doesn't exist. +- **Both storage engines' isolation exists but is opt-in.** + `enterprise-api` (§8, §9) gives real per-tenant ClickHouse *and* + Tantivy isolation, but plain `api` (still the default in + `docker-compose.yml`/`web`'s base URL) has neither, and nothing flags + which one a given deployment is actually running. This is now the + single largest gap — not a missing mechanism, a missing enforcement/ + default. +- **Ingest has no tenant concept for either storage engine.** Every + record `ingest` produces lands in the one shared ClickHouse database + and the one shared Tantivy index no matter what. A newly-provisioned + tenant's storage is real, isolated at query time, and permanently + empty until this changes — undesigned, not just unbuilt. - **Human SSO login now works for OIDC** (§3a) -- verified with a real fake IdP, not yet a real external one or a running `enterprise-auth` container. **SAML login still doesn't exist** -- protocol wiring only, @@ -305,10 +339,10 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: bootstrap is the only way to grant a logged-in identity access today. - **No per-resource dashboard grants** (`dashboard_permissions` has a schema, no handler reads it). -- Two of the four adversarial ClickHouse/Tantivy probes named in +- Three of the four adversarial ClickHouse/Tantivy probes named in `/docs/phase-4-isolation-design.md`'s verification plan are closed - (§8); the other two (Tantivy cross-tenant search, mid-provisioning-race - handling) are still stubbed as explicitly-skipped tests in + (§8, §9); the last (mid-provisioning-race handling) is still stubbed + as an explicitly-skipped test in `api/queryapi/tenant_isolation_gap_test.go`. ## Tearing down diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 5acb00b..f7e8d4d 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -9,58 +9,61 @@ for the full design rationale behind the controls described here. ## Read this first: the single most important open finding -**Updated**: this section originally read "log data queried through -`POST /query` is not tenant-isolated at all." That's now only half -true, and the half that's no longer true matters — read carefully, -because the remaining gap (Tantivy/free-text) is easy to miss if you -stop at "ClickHouse is isolated now." +**Updated a second 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." Both ClickHouse *and* +Tantivy connection/index-layer isolation are now built. What's left is +narrower but still real: **whether a given deployment actually runs the +isolated binary**, and **whether ingest itself is tenant-aware** (it +isn't, for either storage engine). -**ClickHouse (the SQL path) is now built, but only if you run the right -binary — and it has not yet been confirmed against a real ClickHouse.** -`enterprise/internal/tenantprovision` (real `CREATE DATABASE`/`CREATE -USER`/`GRANT` against ClickHouse) and `enterprise/internal/chrunner` (a -per-tenant `driver.Conn` registry implementing api's -`querylang/executor.SQLRunner`, resolving which tenant's connection to -use from the authenticated identity in request context — never from a -client-suppliable field) now exist, and a new binary, -`enterprise/cmd/enterprise-api`, wires them into the same -`api/queryapi.Handler`/`api/dashboards.Handler` core already ships. Real -integration tests exist and would prove the core adversarial claim — -`enterprise/internal/tenantprovision/tenantprovision_test.go`'s -`TestProvisionedUserCannotReadOtherTenantDatabase` and -`TestProvisionedUserCannotReadSystemTables`, -`enterprise/internal/chrunner/chrunner_test.go`'s -`TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL` — but this -environment had no Docker/ClickHouse access while these were written, so -they've only been confirmed to skip cleanly offline, not to pass for -real. See `/docs/phase-4-runbook.md`'s verification-status section -before treating "the test exists" as "isolation is confirmed." +**ClickHouse (the SQL path) is built.** `enterprise/internal/ +tenantprovision` (real `CREATE DATABASE`/`CREATE USER`/`GRANT`) and +`enterprise/internal/chrunner` (a per-tenant connection registry +implementing `api/querylang/executor.SQLRunner`, resolving the tenant +from request identity, never a parameter) are wired into +`enterprise/cmd/enterprise-api`. **Not yet confirmed against a real +ClickHouse** — this environment had no Docker/database access while +these were written; the tests exist and are correct Go, but "the test +exists" is not the same claim as "isolation is confirmed" (see +`/docs/phase-4-runbook.md`). -**But plain `api/cmd/api` still runs with one shared connection**, and -nothing in this repo automatically routes traffic to `enterprise-api` -instead — `docker-compose.yml` includes it "available, not defaulted -into the traffic path" (same shape as `enterprise-auth`'s own addition), -and the Helm chart has no service for it at all yet. **A deployment is -only as isolated as which binary is actually serving traffic** — this -is an operational decision nothing currently enforces or even surfaces -as a warning. +**Tantivy (the free-text path) is also built, and — unlike the +ClickHouse pieces — genuinely verified in this environment.** +`search/src/registry.rs`'s `IndexRegistry` resolves a `SearchRequest. +tenant_id` to its own on-disk Tantivy index, opened on demand; +`enterprise/internal/searchclient` sets that field from the +authenticated request identity, mirroring `chrunner`'s exact "read from +ctx, fail closed, never a parameter" shape. Because Tantivy is an +embedded library (no external service to fake or skip), both sides could +actually be run: `search/src/registry.rs`'s +`tenant_index_is_isolated_from_default_and_other_tenants` seeds three +real indices with the same search term and confirms a tenant-scoped +search returns only that tenant's document; `enterprise/internal/ +searchclient`'s tests run a real in-process gRPC server and confirm the +wire-level `SearchRequest` carries the right `tenant_id`. All pass, for +real, no disclaimer needed for this specific claim. -**Tantivy (the free-text path) is still fully unisolated.** There is no -`enterprise/internal/searchclient` (the Tantivy-side equivalent of -chrunner) — `search`'s gRPC service and -`proto/sentry/search/v1/search.proto`'s `SearchRequest` still carry no -tenant field anywhere, confirmed by reading the code. Every tenant's -free-text queries hit the same shared Tantivy index regardless of which -binary (`api` or `enterprise-api`) serves the HTTP request. A query that -resolves to a pure pipe-syntax free-text search (e.g. `message:"error"`) -is not protected by chrunner at all. +**But plain `api/cmd/api` still runs with one shared ClickHouse +connection and no tenant-scoped search client**, and nothing in this +repo automatically routes traffic to `enterprise-api` instead — +`docker-compose.yml` includes it "available, not defaulted into the +traffic path" (same shape as `enterprise-auth`'s own addition), and the +Helm chart has no service for it at all yet. **A deployment is only as +isolated as which binary is actually serving traffic** — this is an +operational decision nothing currently enforces or even surfaces as a +warning. This is now the single largest gap in the isolation story, not +a missing mechanism. -**What this means concretely**: treat a deployment as tenant-isolated -for structured/SQL queries *only if* it runs `enterprise-api` fronting -provisioned tenants, and treat it as **not isolated at all** for -free-text search regardless of which binary runs. RBAC (below) and -dashboard tenant-scoping (below) hold regardless of which binary is -running; the ClickHouse/Tantivy split above is what changed. +**Ingest is not tenant-aware for either storage engine**, and this is +more load-bearing than it sounds: `chrunner`/`searchclient` prove *read* +isolation given tenant-scoped data exists, but nothing writes +tenant-scoped data yet. Every record `ingest` produces lands in the one +shared ClickHouse database and the one shared (default) Tantivy index, +regardless of tenant. A newly-provisioned tenant's ClickHouse database +and Tantivy index are real, isolated, and queryable through +`enterprise-api` — and permanently empty, until ingest itself becomes +tenant-aware, which is undesigned, not just unbuilt. ## System overview @@ -73,12 +76,15 @@ Browser ──▶ api OR enterprise-api ──▶ ClickHouse (log data, SQL path └─▶ Postgres (control plane: dashboards, alert_rules, tenants, users, tenant_memberships, audit_log) -# api: one shared ClickHouse connection, nil AuditLogger -- Phase 0-3 behavior. +# api: one shared ClickHouse connection, one shared (default) Tantivy +# index via api/searchclient, nil AuditLogger -- Phase 0-3 behavior. # enterprise-api: enterprise/internal/chrunner (per-tenant ClickHouse -# connections) + enterprise/internal/audit.QueryAPILogger (real audit -# writes) wired into the SAME api/queryapi.Handler/api/dashboards.Handler -# core -- see this document's "Read this first" section. Either binary -# can be running; nothing forces the isolated one. +# connections) + enterprise/internal/searchclient (per-tenant Tantivy +# index, via search's SearchRequest.tenant_id) + enterprise/internal/ +# audit.QueryAPILogger (real audit writes) wired into the SAME +# api/queryapi.Handler/api/dashboards.Handler core -- see this +# document's "Read this first" section. Either binary can be running; +# nothing forces the isolated one. alerting ──▶ api or enterprise-api (POST /query, RoleService credential) alerting ──▶ Postgres (rulestore, notifystore) @@ -351,8 +357,10 @@ terms: | ClickHouse per-tenant provisioning (`tenantprovision`) | **Built, not live-verified** — real integration test exists, not yet run against ClickHouse | | ClickHouse query routing (`chrunner`) | **Built, not live-verified** — and only applies when `enterprise-api` serves traffic, not plain `api` | | `system.*` ClickHouse metadata isolation | **Built, not live-verified** — same caveat as above | -| Tantivy/free-text tenant isolation | **Not implemented** — no per-tenant index routing at all | -| Deployment actually routing traffic to `enterprise-api` | **Not implemented** — no Helm service, no default wiring | +| 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-awareness (ClickHouse and Tantivy both) | **Not implemented, undesigned** — every ingested record lands in the single shared database/index regardless of tenant | +| Deployment actually routing traffic to `enterprise-api` | **Not implemented** — no Helm service, no default wiring; now the largest gap in the isolation story | | Human SSO login — OIDC | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | | Human SSO login — SAML | **Not implemented** | | Multi-tenant-membership login (tenant picker) | **Not implemented** — refused with a clear error, not guessed | diff --git a/enterprise/README.md b/enterprise/README.md index 77125a2..79481ff 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -64,6 +64,19 @@ section for exactly what "not yet run" means here and why. Don't read verification, no live database or Docker needed) and every test passes. Not yet tried against a real external IdP or a running `enterprise-auth` container. +- `internal/searchclient`: the Tantivy-side sibling of `chrunner` -- + implements `api/querylang/executor.SearchClient`, resolving + `SearchRequest.tenant_id` (new field, `proto/sentry/search/v1/ + search.proto`) from the authenticated request identity, same + fail-closed shape as `chrunner.Registry.RunSQL`. Paired with + `search/src/registry.rs`'s `IndexRegistry` (Rust, opens a per-tenant + Tantivy index on demand). **Both genuinely verified** -- unlike the + ClickHouse pieces, Tantivy is an embedded library, so the isolation + probe (three tenants, shared search term, scoped search returns only + that tenant's document) actually ran: `search`'s + `cargo test`/`cargo clippy --all-targets -- -D warnings` and this + package's `go test` both pass clean, no Docker or live database + needed for either. - `cmd/enterprise-api`: a second binary (alongside `api/cmd/api`, unchanged) importing *both* `api`'s handler packages and the tenant-aware implementations above -- see its own doc comment for why @@ -86,15 +99,19 @@ silently left out: `metadata/migrations/0024`; no caller reads per-resource grants yet -- `dashboards`' handler enforces tenant-baseline role only, not the matrix's "(own/granted)" qualifier). -- `internal/searchclient` (the Tantivy-side sibling of `chrunner`) -- - `enterprise-api` shares the single, un-tenant-scoped Tantivy index - every deployment does today (`api/searchclient.Dial`, unchanged). See +- **Ingest tenant-awareness, for either storage engine** -- `chrunner`/ + `searchclient` prove read isolation given tenant-scoped data exists, + but nothing writes it: every record `ingest` produces still lands in + the single shared ClickHouse database and the single shared Tantivy + index. A newly-provisioned tenant's storage is real and isolated, and + permanently empty. Undesigned, not just unbuilt -- see `/docs/security/threat-model.md`. - Any deployment-topology mechanism that actually routes traffic to `enterprise-api` instead of `api` -- both binaries exist, `docker-compose.yml` includes `enterprise-api` available but not wired into `web`'s default base URL, and the Helm chart has no - service for it at all yet. + service for it at all yet. **This is now the single largest gap** -- + both storage engines' isolation mechanisms themselves are built. ## Package layout @@ -110,14 +127,15 @@ internal/loginhandler/ GET /auth/oidc/login, GET /auth/oidc/callback -- th internal/rbacstore/ users/tenants/tenant_memberships/data_sources CRUD (pgx against sentry_metadata) internal/tenantprovision/ real ClickHouse CREATE DATABASE/USER/GRANT internal/chrunner/ tenant-scoped api/querylang/executor.SQLRunner +internal/searchclient/ tenant-scoped api/querylang/executor.SearchClient internal/audit/ append-only, hash-chained query audit log, plus the api/queryapi.AuditLogger adapter (queryapi_adapter.go) internal/apiconfig/ enterprise-api's own env-var config internal/config/ enterprise-auth's env-var config ``` -Future additions: `internal/searchclient`, the OIDC/SAML login/callback -HTTP handlers, `dashboard_permissions` CRUD, and real deployment-topology +Future additions: SAML's login handler, `dashboard_permissions` CRUD, +ingest tenant-awareness (undesigned), and real deployment-topology wiring for `enterprise-api` -- see "Status" above. ## Why OIDC and SAML aren't hand-rolled diff --git a/enterprise/cmd/enterprise-api/main.go b/enterprise/cmd/enterprise-api/main.go index 97336f2..521b95d 100644 --- a/enterprise/cmd/enterprise-api/main.go +++ b/enterprise/cmd/enterprise-api/main.go @@ -2,10 +2,10 @@ // api/cmd/api -- same POST /query and /dashboards surface (it reuses // api/queryapi and api/dashboards's actual Handler types unchanged), but // backed by a per-tenant ClickHouse connection registry -// (enterprise/internal/chrunner) instead of the single shared connection -// api/cmd/api opens, and a real audit logger -// (enterprise/internal/audit.QueryAPILogger) instead of the nil api's -// binary has carried since Phase 4 task 4. +// (enterprise/internal/chrunner), a per-tenant Tantivy search client +// (enterprise/internal/searchclient), and a real audit logger +// (enterprise/internal/audit.QueryAPILogger) instead of the single +// shared connections and nil audit logger api/cmd/api's binary carries. // // Why a second binary, not a flag on api/cmd/api: api is AGPL core and // must never import enterprise/ (hack/check-tenant-boundary.sh enforces @@ -17,12 +17,13 @@ // keeps running plain api/cmd/api, unchanged; a real multi-tenant // deployment runs this one instead. // -// Not built yet: per-tenant Tantivy routing (search stays the single -// shared api/searchclient.Dial connection every tenant shares -- -// see /docs/security/threat-model.md), and the actual K8s/Helm wiring -// to run this binary in place of api's (docker-compose.yml adds it -// available, not defaulted into the traffic path, same shape as -// enterprise-auth's own addition in Phase 4 task 5). +// Not built yet: the actual K8s/Helm wiring to run this binary in place +// of api's (docker-compose.yml adds it available, not defaulted into +// the traffic path, same shape as enterprise-auth's own addition in +// Phase 4 task 5), and `search`'s write side (ingest, and by extension +// the Redpanda consumer search itself runs) is still not tenant-aware -- +// see enterprise/internal/searchclient and search/src/registry.rs's doc +// comments, and /docs/security/threat-model.md. package main import ( @@ -44,12 +45,12 @@ import ( "github.com/sentry/sentry/api/dashboards" "github.com/sentry/sentry/api/httpserver" "github.com/sentry/sentry/api/queryapi" - "github.com/sentry/sentry/api/searchclient" "github.com/sentry/sentry/enterprise/internal/apiconfig" "github.com/sentry/sentry/enterprise/internal/audit" "github.com/sentry/sentry/enterprise/internal/chrunner" "github.com/sentry/sentry/enterprise/internal/rbacstore" + "github.com/sentry/sentry/enterprise/internal/searchclient" "github.com/sentry/sentry/enterprise/internal/tenantprovision" ) diff --git a/enterprise/go.mod b/enterprise/go.mod index 29654f2..bd9a35f 100644 --- a/enterprise/go.mod +++ b/enterprise/go.mod @@ -25,7 +25,9 @@ require ( github.com/go-jose/go-jose/v4 v4.1.4 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.10.0 + github.com/sentry/sentry/proto v0.0.0-00010101000000-000000000000 golang.org/x/oauth2 v0.36.0 + google.golang.org/grpc v1.83.0 ) require ( @@ -45,7 +47,6 @@ require ( github.com/pierrec/lz4/v4 v4.1.27 // indirect github.com/russellhaering/goxmldsig v1.4.0 // indirect github.com/segmentio/asm v1.2.1 // indirect - github.com/sentry/sentry/proto v0.0.0-00010101000000-000000000000 // indirect github.com/shopspring/decimal v1.4.0 // indirect go.opentelemetry.io/otel v1.44.0 // indirect go.opentelemetry.io/otel/trace v1.44.0 // indirect @@ -55,6 +56,5 @@ require ( golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect - google.golang.org/grpc v1.83.0 // indirect google.golang.org/protobuf v1.36.12 // indirect ) diff --git a/enterprise/internal/searchclient/searchclient.go b/enterprise/internal/searchclient/searchclient.go new file mode 100644 index 0000000..acd676f --- /dev/null +++ b/enterprise/internal/searchclient/searchclient.go @@ -0,0 +1,70 @@ +// Package searchclient is the tenant-scoped implementation of api's +// querylang/executor.SearchClient interface -- the Tantivy-side sibling +// of enterprise/internal/chrunner (see that package's doc comment for +// the shared reasoning: implementing a core interface structurally +// requires importing the package that defines it, which is why this +// lives in enterprise/ and imports api/, the allowed direction). +// +// Unlike chrunner, there is no separate "one connection per tenant" +// object here -- `search`'s gRPC service now holds the per-tenant +// registry itself (search/src/registry.rs), keyed by the +// SearchRequest.tenant_id field added in proto/sentry/search/v1/ +// search.proto. This package's only job is resolving that field from +// the authenticated request identity before every call -- exactly the +// same "read from ctx, never a parameter, fail closed if absent" shape +// chrunner.Registry.RunSQL uses for ClickHouse. +package searchclient + +import ( + "context" + "fmt" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + + "github.com/sentry/sentry/api/authz" + searchv1 "github.com/sentry/sentry/proto/sentry/search/v1" +) + +type Client struct { + grpc searchv1.SearchServiceClient + conn *grpc.ClientConn +} + +// Dial mirrors api/searchclient.Dial exactly (same plain-TCP, no-TLS +// internal-service-to-service trust boundary) -- the only difference +// from that package is what Search does with the resolved tenant. +func Dial(addr string) (*Client, error) { + conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) + if err != nil { + return nil, fmt.Errorf("dialing search service at %s: %w", addr, err) + } + return &Client{grpc: searchv1.NewSearchServiceClient(conn), conn: conn}, nil +} + +func (c *Client) Close() error { + return c.conn.Close() +} + +// Search implements executor.SearchClient. Resolves the caller's tenant +// from ctx (never a parameter) and fails closed -- no authenticated +// identity, or an identity with no tenant (RoleService, or a +// misconfigured authorizer), refuses the call rather than falling back +// to the single default index, which would silently defeat the whole +// point of this package existing. This mirrors chrunner.Registry.RunSQL's +// exact fail-closed shape. +func (c *Client) Search(ctx context.Context, query string, limit uint32) ([]string, error) { + identity, ok := authz.IdentityFromContext(ctx) + if !ok { + return nil, fmt.Errorf("searchclient: no authenticated identity in context, refusing to search") + } + if identity.TenantID == "" { + return nil, fmt.Errorf("searchclient: authenticated identity %q has no tenant, refusing to search", identity.Role) + } + + resp, err := c.grpc.Search(ctx, &searchv1.SearchRequest{Query: query, Limit: limit, TenantId: identity.TenantID}) + if err != nil { + return nil, err + } + return resp.GetRecordIds(), nil +} diff --git a/enterprise/internal/searchclient/searchclient_test.go b/enterprise/internal/searchclient/searchclient_test.go new file mode 100644 index 0000000..b84c8c7 --- /dev/null +++ b/enterprise/internal/searchclient/searchclient_test.go @@ -0,0 +1,114 @@ +// Tests run a real gRPC server in-process (net.Listen on an ephemeral +// port, a real grpc.Server) implementing SearchServiceServer, so these +// exercise the actual wire protocol Client.Search sends, not a mocked +// interface -- proving TenantId is set on the real SearchRequest that +// would reach `search`, and that Client fails closed exactly the way +// enterprise/internal/chrunner.Registry.RunSQL does for ClickHouse. +package searchclient + +import ( + "context" + "net" + "testing" + + "google.golang.org/grpc" + + "github.com/sentry/sentry/api/authz" + searchv1 "github.com/sentry/sentry/proto/sentry/search/v1" +) + +type fakeSearchServer struct { + searchv1.UnimplementedSearchServiceServer + lastRequest *searchv1.SearchRequest + recordIDs []string +} + +func (f *fakeSearchServer) Search(_ context.Context, req *searchv1.SearchRequest) (*searchv1.SearchResponse, error) { + f.lastRequest = req + return &searchv1.SearchResponse{RecordIds: f.recordIDs}, nil +} + +func newTestServer(t *testing.T) (*Client, *fakeSearchServer) { + t.Helper() + lis, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listening: %v", err) + } + fake := &fakeSearchServer{recordIDs: []string{"id-1", "id-2"}} + srv := grpc.NewServer() + searchv1.RegisterSearchServiceServer(srv, fake) + go func() { _ = srv.Serve(lis) }() + t.Cleanup(srv.Stop) + + client, err := Dial(lis.Addr().String()) + if err != nil { + t.Fatalf("Dial: %v", err) + } + t.Cleanup(func() { _ = client.Close() }) + return client, fake +} + +func TestSearchForwardsTenantIDFromContext(t *testing.T) { + client, fake := newTestServer(t) + ctx := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer}) + + ids, err := client.Search(ctx, "error", 10) + if err != nil { + t.Fatalf("Search: %v", err) + } + if len(ids) != 2 { + t.Fatalf("got %d record ids, want 2", len(ids)) + } + if fake.lastRequest.TenantId != "acme" { + t.Fatalf("TenantId sent to search = %q, want acme", fake.lastRequest.TenantId) + } + if fake.lastRequest.Query != "error" || fake.lastRequest.Limit != 10 { + t.Fatalf("unexpected request: %+v", fake.lastRequest) + } +} + +func TestSearchRefusesWithNoIdentity(t *testing.T) { + client, fake := newTestServer(t) + + if _, err := client.Search(context.Background(), "error", 10); err == nil { + t.Fatal("expected Search to refuse a request with no authenticated identity in context") + } + if fake.lastRequest != nil { + t.Fatal("expected the gRPC call to never reach the server when there's no identity") + } +} + +func TestSearchRefusesIdentityWithNoTenant(t *testing.T) { + client, fake := newTestServer(t) + // RoleService identities carry no TenantID -- see api/authz.Identity's + // doc comment. /alerting never calls search directly today, but the + // fail-closed behavior must hold regardless of how this arises. + ctx := authz.WithIdentity(context.Background(), authz.Identity{Role: authz.RoleService}) + + if _, err := client.Search(ctx, "error", 10); err == nil { + t.Fatal("expected Search to refuse an identity with no tenant") + } + if fake.lastRequest != nil { + t.Fatal("expected the gRPC call to never reach the server when the identity has no tenant") + } +} + +func TestSearchDifferentTenantsSendDifferentTenantIDs(t *testing.T) { + client, fake := newTestServer(t) + + ctxA := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer}) + if _, err := client.Search(ctxA, "q", 5); err != nil { + t.Fatalf("Search (acme): %v", err) + } + if fake.lastRequest.TenantId != "acme" { + t.Fatalf("TenantId = %q, want acme", fake.lastRequest.TenantId) + } + + ctxB := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "globex", Role: authz.RoleViewer}) + if _, err := client.Search(ctxB, "q", 5); err != nil { + t.Fatalf("Search (globex): %v", err) + } + if fake.lastRequest.TenantId != "globex" { + t.Fatalf("TenantId = %q, want globex", fake.lastRequest.TenantId) + } +} diff --git a/proto/sentry/search/v1/search.pb.go b/proto/sentry/search/v1/search.pb.go index dc9a1d8..9119e4d 100644 --- a/proto/sentry/search/v1/search.pb.go +++ b/proto/sentry/search/v1/search.pb.go @@ -29,7 +29,17 @@ type SearchRequest struct { // and doesn't support in Phase 1. Query string `protobuf:"bytes,1,opt,name=query,proto3" json:"query,omitempty"` // Max results to return. 0 (unset) uses the service's own default. - Limit uint32 `protobuf:"varint,2,opt,name=limit,proto3" json:"limit,omitempty"` + Limit uint32 `protobuf:"varint,2,opt,name=limit,proto3" json:"limit,omitempty"` + // tenant_id (Phase 4) selects which per-tenant Tantivy index to search + // -- resolved server-side by the caller (enterprise/internal/ + // searchclient, from the authenticated identity in request context), + // never a value a browser/client supplies directly. Empty selects the + // single default index every Phase 0-3 deployment (and every + // ingest-written record today, regardless of tenant -- see + // /docs/security/threat-model.md's ingest-tenancy caveat) already + // uses, so this field is purely additive: an old caller that never + // sets it keeps today's behavior exactly. + TenantId string `protobuf:"bytes,3,opt,name=tenant_id,json=tenantId,proto3" json:"tenant_id,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -78,6 +88,13 @@ func (x *SearchRequest) GetLimit() uint32 { return 0 } +func (x *SearchRequest) GetTenantId() string { + if x != nil { + return x.TenantId + } + return "" +} + type SearchResponse struct { state protoimpl.MessageState `protogen:"open.v1"` // record_ids of matching logs, most-relevant first. Callers join these @@ -130,10 +147,11 @@ var File_sentry_search_v1_search_proto protoreflect.FileDescriptor const file_sentry_search_v1_search_proto_rawDesc = "" + "\n" + - "\x1dsentry/search/v1/search.proto\x12\x10sentry.search.v1\";\n" + + "\x1dsentry/search/v1/search.proto\x12\x10sentry.search.v1\"X\n" + "\rSearchRequest\x12\x14\n" + "\x05query\x18\x01 \x01(\tR\x05query\x12\x14\n" + - "\x05limit\x18\x02 \x01(\rR\x05limit\"/\n" + + "\x05limit\x18\x02 \x01(\rR\x05limit\x12\x1b\n" + + "\ttenant_id\x18\x03 \x01(\tR\btenantId\"/\n" + "\x0eSearchResponse\x12\x1d\n" + "\n" + "record_ids\x18\x01 \x03(\tR\trecordIds2\\\n" + diff --git a/proto/sentry/search/v1/search.proto b/proto/sentry/search/v1/search.proto index 2a789a5..5a753d3 100644 --- a/proto/sentry/search/v1/search.proto +++ b/proto/sentry/search/v1/search.proto @@ -22,6 +22,17 @@ message SearchRequest { // Max results to return. 0 (unset) uses the service's own default. uint32 limit = 2; + + // tenant_id (Phase 4) selects which per-tenant Tantivy index to search + // -- resolved server-side by the caller (enterprise/internal/ + // searchclient, from the authenticated identity in request context), + // never a value a browser/client supplies directly. Empty selects the + // single default index every Phase 0-3 deployment (and every + // ingest-written record today, regardless of tenant -- see + // /docs/security/threat-model.md's ingest-tenancy caveat) already + // uses, so this field is purely additive: an old caller that never + // sets it keeps today's behavior exactly. + string tenant_id = 3; } message SearchResponse { diff --git a/search/README.md b/search/README.md index 0315142..09a92ca 100644 --- a/search/README.md +++ b/search/README.md @@ -29,12 +29,34 @@ tracking, own failure domain — see "Offset tracking" below) even though it's a completely separate process/service. If Tantivy indexing lags or crashes, ClickHouse ingestion is completely unaffected. -`api` calls `search`'s `SearchService.Search` gRPC RPC (see -`/proto/sentry/search/v1/search.proto`) to resolve a free-text query into -matching `record_id`s, then joins those back against ClickHouse's -`logs.record_id` column (see `/storage/migrations/0002_add_record_id.sql`) -to get full rows. `search` only ever returns IDs, never row data — it -stays a pure text index, not a second copy of the row. +`api` (or, for a multi-tenant deployment, `enterprise-api` — see +`/enterprise/README.md`) calls `search`'s `SearchService.Search` gRPC RPC +(see `/proto/sentry/search/v1/search.proto`) to resolve a free-text +query into matching `record_id`s, then joins those back against +ClickHouse's `logs.record_id` column (see +`/storage/migrations/0002_add_record_id.sql`) to get full rows. `search` +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 + +`SearchRequest.tenant_id` (empty by default) selects which index +`src/registry.rs`'s `IndexRegistry` searches: empty resolves to the +single default index every deployment already had; a non-empty value +opens (on first use) a dedicated index under `TENANTS_INDEX_PATH` +(default `/var/lib/sentry-search/tenants/`, matching +`deploy/operator`'s and `enterprise/internal/rbacstore`'s existing path +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`. ## Offset tracking: why this isn't a Kafka consumer group @@ -74,7 +96,8 @@ Environment variables (see `src/config.rs`): | `REDPANDA_BROKERS` | `localhost:9092` | Comma-separated broker list | | `REDPANDA_TOPIC` | `sentry.logs.raw` | Must match `/ingest`'s topic | | `REDPANDA_TOPIC_PARTITIONS` | `6` | Must match what `/transport/provision-topics.sh` created | -| `INDEX_PATH` | `/var/lib/sentry-search/index` | Tantivy index directory | +| `INDEX_PATH` | `/var/lib/sentry-search/index` | Default (non-tenant) Tantivy index directory | +| `TENANTS_INDEX_PATH` | `/var/lib/sentry-search/tenants` | Per-tenant index directories live under here, one subdirectory per tenant_id (Phase 4) | | `OFFSETS_PATH` | `/var/lib/sentry-search/offsets.json` | Offset tracking file | | `COMMIT_INTERVAL_MS` | `2000` | How often buffered writes become searchable | @@ -89,7 +112,13 @@ cargo test `index.rs`'s tests run against a real (temp-directory) Tantivy index — no external service needed, unlike ClickHouse. They cover the delete-then-add idempotency, phrase queries, result limits, and the -before-commit/after-commit visibility boundary. `consumer.rs` (the +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 category of gap as `/ingest`'s `kafka.Reader`/`kafka.Writer` wiring, and is exercised by the docker-compose end-to-end flow instead. diff --git a/search/src/config.rs b/search/src/config.rs index 27b3542..41de954 100644 --- a/search/src/config.rs +++ b/search/src/config.rs @@ -11,6 +11,16 @@ pub struct Config { pub index_path: PathBuf, pub offsets_path: PathBuf, pub commit_interval: Duration, + /// Phase 4: per-tenant index directories live under here, one + /// subdirectory per tenant_id, opened on demand by + /// registry::IndexRegistry -- distinct from `index_path` above, + /// which stays the single shared index every ingest-written record + /// lands in regardless of tenant (see registry.rs's doc comment and + /// /docs/security/threat-model.md's ingest-tenancy caveat). Default + /// matches the path convention deploy/operator's Tenant controller + /// and enterprise/internal/rbacstore's seeded default data source + /// already assume (`/var/lib/sentry-search/tenants/`). + pub tenants_index_path: PathBuf, } impl Config { @@ -35,6 +45,10 @@ impl Config { "/var/lib/sentry-search/offsets.json", )), commit_interval: Duration::from_millis(commit_interval_ms), + tenants_index_path: PathBuf::from(getenv( + "TENANTS_INDEX_PATH", + "/var/lib/sentry-search/tenants", + )), }) } } diff --git a/search/src/grpc.rs b/search/src/grpc.rs index c47e197..9c21acb 100644 --- a/search/src/grpc.rs +++ b/search/src/grpc.rs @@ -1,18 +1,18 @@ use std::sync::Arc; use tonic::{Request, Response, Status}; -use crate::index::SearchIndex; +use crate::registry::IndexRegistry; use crate::searchv1; const DEFAULT_LIMIT: usize = 100; pub struct SearchServer { - index: Arc, + registry: Arc, } impl SearchServer { - pub fn new(index: Arc) -> Self { - Self { index } + pub fn new(registry: Arc) -> Self { + Self { registry } } } @@ -33,7 +33,17 @@ impl searchv1::search_service_server::SearchService for SearchServer { req.limit as usize }; - let index = Arc::clone(&self.index); + // Resolves (opening on first use) the caller's tenant index, or + // the single default index when tenant_id is empty -- see + // registry.rs's doc comment. Never falls back to a *different* + // tenant's index on error; an unsafe/unknown tenant_id is a + // hard failure, not a silent default. + let index = self + .registry + .resolve(&req.tenant_id) + .await + .map_err(|e| Status::invalid_argument(format!("resolving tenant index: {e}")))?; + let query = req.query.clone(); // Tantivy's searcher is synchronous; run it on a blocking thread // so it doesn't stall the async runtime alongside the consumer diff --git a/search/src/main.rs b/search/src/main.rs index 8a93e0c..3667889 100644 --- a/search/src/main.rs +++ b/search/src/main.rs @@ -3,6 +3,7 @@ mod consumer; mod grpc; mod index; mod offsets; +mod registry; pub mod logsv1 { tonic::include_proto!("sentry.logs.v1"); @@ -14,6 +15,7 @@ pub mod searchv1 { use anyhow::{Context, Result}; use config::Config; use index::SearchIndex; +use registry::IndexRegistry; use std::sync::Arc; use tonic::transport::Server; @@ -33,6 +35,12 @@ 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). + let registry = Arc::new(IndexRegistry::new(Arc::clone(&index), cfg.tenants_index_path.clone())); let partition_count: i32 = std::env::var("REDPANDA_TOPIC_PARTITIONS") .ok() @@ -53,7 +61,7 @@ async fn main() -> Result<()> { .context("parsing GRPC_LISTEN_ADDR")?; tracing::info!(addr = %cfg.grpc_listen_addr, "search gRPC server listening"); - let search_server = grpc::SearchServer::new(Arc::clone(&index)); + let search_server = grpc::SearchServer::new(Arc::clone(®istry)); Server::builder() .add_service(searchv1::search_service_server::SearchServiceServer::new( search_server, diff --git a/search/src/registry.rs b/search/src/registry.rs new file mode 100644 index 0000000..859aa10 --- /dev/null +++ b/search/src/registry.rs @@ -0,0 +1,185 @@ +use anyhow::{bail, Context, Result}; +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; +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. +/// +/// 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). +pub struct IndexRegistry { + default_index: Arc, + tenants_root: PathBuf, + tenants: RwLock>>, +} + +impl IndexRegistry { + pub fn new(default_index: Arc, tenants_root: PathBuf) -> Self { + Self { + default_index, + tenants_root, + tenants: RwLock::new(HashMap::new()), + } + } + + /// Resolves (opening on first use) the index for `tenant_id`, or the + /// default index when `tenant_id` is empty. + pub async fn resolve(&self, tenant_id: &str) -> Result> { + if tenant_id.is_empty() { + return Ok(Arc::clone(&self.default_index)); + } + + { + let tenants = self.tenants.read().await; + if let Some(idx) = tenants.get(tenant_id) { + return Ok(Arc::clone(idx)); + } + } + + validate_tenant_id(tenant_id)?; + + // Two concurrent first-requests for the same never-before-seen + // tenant could both reach here -- resolved by re-checking under + // the write lock via `entry().or_insert_with(..)` below, so at + // most one SearchIndex ever actually gets constructed and + // stored, even if both callers did the (cheap, idempotent) + // open_or_create call. + let path = self.tenants_root.join(tenant_id); + let opened = SearchIndex::open_or_create(&path) + .with_context(|| format!("opening tantivy index for tenant {tenant_id:?}"))?; + + let mut tenants = self.tenants.write().await; + let idx = tenants + .entry(tenant_id.to_string()) + .or_insert_with(|| Arc::new(opened)); + Ok(Arc::clone(idx)) + } +} + +/// Mirrors enterprise/internal/tenantprovision's tenantIdentifierPattern +/// (Go) exactly -- tenant_id becomes a literal filesystem path component +/// here, the same class of injection concern that package's doc comment +/// explains for ClickHouse DDL identifiers. +fn validate_tenant_id(tenant_id: &str) -> Result<()> { + let mut chars = tenant_id.chars(); + let starts_ok = chars.next().is_some_and(|c| c.is_ascii_lowercase()); + let rest_ok = chars.all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_'); + if !starts_ok || !rest_ok || tenant_id.len() > 63 { + bail!("tenant_id {tenant_id:?} is not a safe index-directory name"); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn new_test_registry() -> (IndexRegistry, Arc, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("creating temp dir"); + let default_index = + Arc::new(SearchIndex::open_or_create(&dir.path().join("default")).unwrap()); + let registry = IndexRegistry::new(Arc::clone(&default_index), dir.path().join("tenants")); + (registry, default_index, dir) + } + + #[tokio::test] + async fn empty_tenant_id_resolves_to_default_index() { + let (registry, default_index, _dir) = new_test_registry(); + let resolved = registry.resolve("").await.unwrap(); + assert!(Arc::ptr_eq(&resolved, &default_index)); + } + + #[tokio::test] + async fn same_tenant_id_resolves_to_the_same_index_instance() { + let (registry, _default, _dir) = new_test_registry(); + let a = registry.resolve("acme").await.unwrap(); + let b = registry.resolve("acme").await.unwrap(); + assert!(Arc::ptr_eq(&a, &b), "expected the same Arc on a second resolve"); + } + + #[tokio::test] + async fn different_tenants_resolve_to_different_index_instances() { + let (registry, _default, _dir) = new_test_registry(); + let a = registry.resolve("acme").await.unwrap(); + let b = registry.resolve("globex").await.unwrap(); + assert!(!Arc::ptr_eq(&a, &b), "expected different tenants to get different index instances"); + } + + #[tokio::test] + async fn tenant_index_is_isolated_from_default_and_other_tenants() { + let (registry, _default, _dir) = new_test_registry(); + + let default_idx = registry.resolve("").await.unwrap(); + default_idx.upsert("default-1", "shared term").await.unwrap(); + default_idx.commit().await.unwrap(); + + let acme_idx = registry.resolve("acme").await.unwrap(); + acme_idx.upsert("acme-1", "shared term").await.unwrap(); + acme_idx.commit().await.unwrap(); + + let globex_idx = registry.resolve("globex").await.unwrap(); + globex_idx.upsert("globex-1", "shared term").await.unwrap(); + globex_idx.commit().await.unwrap(); + + // The core adversarial probe from + // /docs/phase-4-isolation-design.md's verification plan, item 3: + // a search scoped to one tenant must never return another + // tenant's (or the default index's) matching documents, even + // though the term exists in all three. + assert_eq!(acme_idx.search("shared", 10).unwrap(), vec!["acme-1"]); + assert_eq!(globex_idx.search("shared", 10).unwrap(), vec!["globex-1"]); + assert_eq!(default_idx.search("shared", 10).unwrap(), vec!["default-1"]); + } + + #[tokio::test] + async fn rejects_unsafe_tenant_id() { + let (registry, _default, _dir) = new_test_registry(); + for bad in ["", "../etc", "Acme", "has spaces", "-leading-dash"] { + if bad.is_empty() { + continue; // empty is valid -- resolves to the default index, not an error + } + assert!( + registry.resolve(bad).await.is_err(), + "expected {bad:?} to be rejected as an unsafe tenant_id" + ); + } + } + + #[tokio::test] + async fn concurrent_first_resolves_for_the_same_new_tenant_share_one_instance() { + let (registry, _default, _dir) = new_test_registry(); + let registry = Arc::new(registry); + + let mut handles = Vec::new(); + for _ in 0..8 { + let registry = Arc::clone(®istry); + handles.push(tokio::spawn(async move { registry.resolve("acme").await.unwrap() })); + } + let mut results = Vec::new(); + for h in handles { + results.push(h.await.unwrap()); + } + for r in &results[1..] { + assert!(Arc::ptr_eq(&results[0], r), "expected every concurrent resolve to return the same Arc"); + } + } +}