From 2e698f562304ca2c28393a0c4843f3f792d3c8be Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 14 Aug 2026 08:07:52 -0700 Subject: [PATCH] Close the last tenant-isolation adversarial probe (mid-provisioning tenants) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 4 task 8's verification plan named four adversarial probes; three were closed earlier this phase, the fourth (an evaluator tick, or any other caller, hitting a tenant that exists but hasn't reached the active+credentialed gate yet -- must be refused, not served) was still an explicitly-skipped stub in api/queryapi/tenant_isolation_gap_test.go. Investigating it found the two storage engines needed genuinely different treatment: - ClickHouse (enterprise/internal/chrunner) already had this property structurally, for free: Registry is built once at startup from rbacstore.ListProvisionedDataSources, which already filters to active+credentialed tenants only, so a mid-provisioning tenant is simply absent from the connection map. New test TestRegistryRefusesMidProvisioningTenant proves this without Docker -- an empty DataSource list never dials ClickHouse, so this genuinely runs in this environment, unlike every other test in that file. - Tantivy (search/src/registry.rs's IndexRegistry) was a real, different gap, not just an unverified assumption: it opens-or-creates an index for any syntactically-valid tenant_id on first request, because it's a separate process with no Postgres access and structurally can't know which tenants are actually provisioned. A query against a mid-provisioning tenant would have silently succeeded with zero results from a freshly-created empty index -- "ambient success" indistinguishable from "no matching logs," exactly the failure mode this item was worried about. Fixed the Tantivy gap with a new enterprise/internal/searchclient. TenantChecker interface (backed by a new rbacstore.TenantIsActive, implemented structurally, no new import edge needed), consulted before every gRPC call: Client.Search now refuses a non-active tenant before it ever reaches `search`. Dial's signature gained a required TenantChecker parameter; enterprise-api's main.go passes its existing rbacstore.Store (already satisfies the interface). Verified Docker-free via searchclient's existing real-in-process-gRPC-server test harness (TestSearchRefusesMidProvisioningTenant, plus TestSearchPropagatesTenantCheckerError for the fail-closed-on-error case) -- both genuinely run in this environment, same bar as the rest of the Tantivy isolation work. rbacstore.TenantIsActive itself has two new skip-gated live-Postgres tests (TestTenantIsActive, TestTenantIsActiveNonexistentTenant) -- disclosed as not run against a live database here, same gap as the rest of this phase's Postgres-backed pieces. api/queryapi/tenant_isolation_gap_test.go rewritten from a checklist with one skipped stub to a full accounting of all four now-closed probes. Docs updated in lockstep: CLAUDE.md, threat-model.md, phase-4-isolation-design.md (implementation note added after its original sign-off), phase-4-runbook.md (§9), enterprise/README.md. --- CLAUDE.md | 18 ++++- api/queryapi/tenant_isolation_gap_test.go | 63 +++++++++++------ docs/phase-4-isolation-design.md | 22 ++++++ docs/phase-4-runbook.md | 42 ++++++++--- docs/security/threat-model.md | 2 +- enterprise/README.md | 8 ++- enterprise/cmd/enterprise-api/main.go | 2 +- enterprise/internal/chrunner/chrunner_test.go | 35 ++++++++++ enterprise/internal/rbacstore/rbacstore.go | 20 ++++++ .../internal/rbacstore/rbacstore_test.go | 45 ++++++++++++ .../internal/searchclient/searchclient.go | 69 +++++++++++++++--- .../searchclient/searchclient_test.go | 70 +++++++++++++++++-- 12 files changed, 345 insertions(+), 51 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 77a10d3..5bba796 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -173,7 +173,23 @@ built too **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. The deployment- +actually ran in this environment, no Docker needed. That same +Docker-free advantage is what caught a real bug while closing the last +of Phase 4 task 8's four adversarial probes (a mid-provisioning tenant +must be refused, not served): `search/src/registry.rs`'s `IndexRegistry` +opened-or-created an index for any syntactically-valid `tenant_id`, +meaning a query against a tenant that exists in `rbacstore` but isn't +active yet would have silently returned zero results from a +freshly-created empty index instead of being refused -- +`chrunner`'s ClickHouse routing had the equivalent guarantee for free +(a mid-provisioning tenant simply isn't in its startup-built connection +map) but Tantivy, a separate process with no Postgres access, had no +way to know. Fixed with a new `enterprise/internal/searchclient. +TenantChecker` (backed by `rbacstore.TenantIsActive`); both halves of +the fix verified Docker-free (`chrunner_test.go`'s and +`searchclient_test.go`'s `TestSearchRefusesMidProvisioningTenant`-shaped +tests) — see `api/queryapi/tenant_isolation_gap_test.go` for the full +accounting of all four probes, now all closed. The deployment- topology gap that briefly was the largest one is now closed for Helm: `deploy/helm/sentry/templates/api.yaml`/`enterprise-api.yaml` are mutually exclusive on the same `enterprise.enabled` flag that turns on diff --git a/api/queryapi/tenant_isolation_gap_test.go b/api/queryapi/tenant_isolation_gap_test.go index 3e40b2f..249f18a 100644 --- a/api/queryapi/tenant_isolation_gap_test.go +++ b/api/queryapi/tenant_isolation_gap_test.go @@ -3,7 +3,7 @@ // "Verification plan for this design specifically" section names for // Phase 4 task 8 have a permanent, grep-able home in the test tree. // -// Three of the four are no longer blocked: +// All four are now closed: // // - Item 1 (fully-qualified cross-tenant raw SQL): // enterprise/internal/tenantprovision/tenantprovision_test.go's @@ -20,25 +20,46 @@ // 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 (an evaluator tick mid-provisioning must be refused, not +// served): closed on both storage engines, though the two needed +// genuinely different fixes once actually investigated. +// enterprise/internal/chrunner's fail-closed behavior turned out to +// already be structural -- a tenant not yet active+credentialed is +// simply absent from the immutable connection map enterprise-api's +// main.go builds at startup from +// rbacstore.ListProvisionedDataSources (itself covered by +// rbacstore_test.go's +// TestListProvisionedDataSourcesExcludesUnprovisionedAndInactive), +// so "mid-provisioning" and "entirely unknown tenant" collapse to +// the identical RunSQL map-lookup-miss path -- +// enterprise/internal/chrunner/chrunner_test.go's +// TestRegistryRefusesMidProvisioningTenant proves this without +// needing Docker (an empty DataSource list never dials ClickHouse), +// complementing TestRegistryRefusesUnknownTenant's live-ClickHouse +// version of the same property. // -// 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. +// Tantivy was a real, different gap, not just an unverified +// assumption: search/src/registry.rs's IndexRegistry opens-or- +// creates an index for *any* syntactically-valid tenant_id on first +// request, because it's a separate process with no Postgres access +// and structurally can't know which tenants are actually +// provisioned. Without a check upstream, a query against a +// mid-provisioning tenant would have silently succeeded with zero +// results from a freshly-created empty index -- "ambient success" +// indistinguishable from "no matching logs," exactly the failure +// mode this item worried about. Fixed by adding +// enterprise/internal/searchclient.TenantChecker (backed by +// rbacstore.TenantIsActive): Client.Search now refuses before the +// gRPC call ever goes out if the tenant isn't active. Covered by +// enterprise/internal/searchclient/searchclient_test.go's +// TestSearchRefusesMidProvisioningTenant (Docker-free, real +// in-process gRPC server) and rbacstore_test.go's +// TestTenantIsActive/TestTenantIsActiveNonexistentTenant +// (live-Postgres, skip-gated). +// +// Scope boundary all four 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_EvaluatorTickMidProvisioningIsRefusedNotServed(t *testing.T) { - t.Skip("BLOCKED on enterprise/internal/tenantprovision's ordered " + - "provisioning state machine (CREATE USER -> GRANT -> mark active): " + - "needs a tenant row that exists but hasn't reached the active gate " + - "yet, and a simulated /alerting evaluator tick against it, to " + - "confirm every tenant-resolution path actually checks tenant " + - "status server-side rather than inferring readiness from ambient " + - "connection success. See /docs/phase-4-isolation-design.md's " + - "verification plan, item 4, and its provisioning-gate requirement.") -} diff --git a/docs/phase-4-isolation-design.md b/docs/phase-4-isolation-design.md index 38267a4..7b0da2d 100644 --- a/docs/phase-4-isolation-design.md +++ b/docs/phase-4-isolation-design.md @@ -315,3 +315,25 @@ suite must include, against the live stack: - A simulated evaluator tick firing mid-provisioning (tenant row exists, grants not yet confirmed) to confirm it's refused, not silently served against a partially-provisioned or default-profile connection. + + **Implementation note (Phase 4 task 8, added after this design was + signed off):** closed for both storage engines, via + `api/queryapi/tenant_isolation_gap_test.go`'s pointers. ClickHouse's + refusal turned out to be structural, not something that needed new + code: `chrunner.Registry` is built once at startup from + `rbacstore.ListProvisionedDataSources`, which already excludes + anything short of active+credentialed, so a mid-provisioning tenant is + simply absent from the connection map — proven Docker-free + (`chrunner_test.go`'s `TestRegistryRefusesMidProvisioningTenant`, + since an empty `DataSource` list never dials ClickHouse). Tantivy was + a real, different gap: `search/src/registry.rs`'s `IndexRegistry` + opens-or-creates an index for *any* syntactically-valid `tenant_id` on + first request, because it's a separate process with no Postgres + access and structurally cannot know which tenants are provisioned -- + without a check upstream, a mid-provisioning tenant's query would have + silently succeeded with zero results from a freshly-created empty + index. Fixed by adding `enterprise/internal/searchclient. + TenantChecker` (backed by a new `rbacstore.TenantIsActive`): `Client. + Search` now refuses before the gRPC call goes out if the tenant isn't + active, verified Docker-free via a real in-process gRPC server + (`searchclient_test.go`'s `TestSearchRefusesMidProvisioningTenant`). diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index 5ec8093..d9516c4 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -386,14 +386,27 @@ cargo test 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. +# correctly, that a request with no/invalid tenant identity is refused, +# and (since TenantChecker was added to close verification-plan item 4) +# that a tenant which exists but isn't active yet -- e.g. right after +# enterprise-auth -create-tenant but before enterprise-api +# -provision-tenant -- is refused too, not silently searched against a +# freshly-created empty index. + +go test ./internal/chrunner/... -run MidProvisioning -v +# the ClickHouse half of the same item-4 probe -- also Docker-free, +# since an empty DataSource list never dials out. ``` 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. +Docker access, alongside `enterprise/internal/loginhandler`'s OIDC/SAML +tests (§3a/§3b) — both are unusually strong evidence precisely because +they needed no infrastructure this environment lacked. Writing the +`TenantChecker` test here is also what found the Tantivy mid- +provisioning gap in the first place, not just what closed it after the +fact -- see `api/queryapi/tenant_isolation_gap_test.go`'s item 4 for the +full story. ## 10. Confirm the Helm chart actually enforces the binary swap @@ -466,11 +479,22 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: (`enterprise/internal/rbacstore/rbacstore_test.go`) haven't run against a live database in this environment, same gap as the rest of this phase's Postgres-backed pieces. -- Three of the four adversarial ClickHouse/Tantivy probes named in - `/docs/phase-4-isolation-design.md`'s verification plan are closed - (§8, §9); the last (mid-provisioning-race handling) is still stubbed - as an explicitly-skipped test in - `api/queryapi/tenant_isolation_gap_test.go`. +- All four adversarial ClickHouse/Tantivy probes named in + `/docs/phase-4-isolation-design.md`'s verification plan are now closed + -- see `api/queryapi/tenant_isolation_gap_test.go` for the full + accounting. The fourth (mid-provisioning-race handling) turned out to + need a real code fix on the Tantivy side, not just a test: `search/ + src/registry.rs`'s `IndexRegistry` opened-or-created an index for any + syntactically-valid `tenant_id`, so a mid-provisioning tenant's query + would have silently succeeded with zero results instead of being + refused. Fixed via `enterprise/internal/searchclient.TenantChecker` + (backed by `rbacstore.TenantIsActive`). Both the ClickHouse and + Tantivy halves of this probe run genuinely, without Docker, in this + environment (`chrunner_test.go`'s + `TestRegistryRefusesMidProvisioningTenant`, `searchclient_test.go`'s + `TestSearchRefusesMidProvisioningTenant`) -- `rbacstore.TenantIsActive` + itself has skip-gated live-Postgres tests that haven't run here, same + disclosed gap as the rest of this phase's Postgres-backed pieces. ## Tearing down diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 5caa5fe..b7d9329 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -412,5 +412,5 @@ terms: | Query audit logging (routine queries) | **Enforced**, fail-open, and now wired to a real writer via `enterprise-api` (`audit.QueryAPILogger`) | | Audit log tamper detection (hash chain) | **Enforced**, verified live | | Audit log tamper prevention (external anchoring) | **Design only** — `FileSink` is a dev stand-in | -| Mid-provisioning-race handling (evaluator ticks against a not-yet-active tenant) | **Unverified** — see `api/queryapi/tenant_isolation_gap_test.go` | +| Mid-provisioning-race handling (evaluator ticks against a not-yet-active tenant) | **Closed on both storage engines** — see `api/queryapi/tenant_isolation_gap_test.go`; ClickHouse verified Docker-free (structural, not just tested), Tantivy fixed and verified Docker-free after finding it was a real gap, not just an unverified assumption | | Protection against a privileged DB administrator | **Explicit non-goal** | diff --git a/enterprise/README.md b/enterprise/README.md index 81d9a7a..872e5a2 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -76,7 +76,13 @@ section for exactly what "not yet run" means here and why. Don't read 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. + needed for either. `Client` also carries a `TenantChecker` (backed by + `rbacstore.TenantIsActive`) since `search/src/registry.rs`'s + `IndexRegistry` opens-or-creates an index for any syntactically-valid + `tenant_id` -- a real gap found while closing + `/docs/phase-4-isolation-design.md`'s verification-plan item 4: a + mid-provisioning tenant would otherwise get a silently-empty search + result instead of a refusal. Verified the same Docker-free way. - `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 diff --git a/enterprise/cmd/enterprise-api/main.go b/enterprise/cmd/enterprise-api/main.go index 099eecf..21d3b4d 100644 --- a/enterprise/cmd/enterprise-api/main.go +++ b/enterprise/cmd/enterprise-api/main.go @@ -125,7 +125,7 @@ func main() { } defer registry.Close() - search, err := searchclient.Dial(cfg.SearchGRPCAddr) + search, err := searchclient.Dial(cfg.SearchGRPCAddr, rbac) if err != nil { logger.Error("dialing search service", "error", err) os.Exit(1) diff --git a/enterprise/internal/chrunner/chrunner_test.go b/enterprise/internal/chrunner/chrunner_test.go index 9eda0a4..d57603d 100644 --- a/enterprise/internal/chrunner/chrunner_test.go +++ b/enterprise/internal/chrunner/chrunner_test.go @@ -183,3 +183,38 @@ func TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL(t *testing.T) { t.Fatal("tenant A's request was able to read tenant B's database by fully-qualified name -- isolation is broken") } } + +// TestRegistryRefusesMidProvisioningTenant is Phase 4 task 8's item 4 +// adversarial probe (see /docs/phase-4-isolation-design.md's +// verification plan and api/queryapi/tenant_isolation_gap_test.go): +// a tenant row that exists in rbacstore but hasn't reached the +// active+credentialed gate yet must be refused, not served via some +// ambient connection. Unlike every other test in this file, this one +// needs no live ClickHouse at all -- New never dials out for an empty +// DataSource list, so an empty Registry (as if every tenant in +// `tenants` were still mid-provisioning) is exactly what +// enterprise-api's main.go would build from +// rbacstore.ListProvisionedDataSources before any tenant clears that +// filter. "Mid-provisioning" and "entirely unknown" collapse to the +// identical code path here by construction: Registry has no concept of +// "a tenant row exists," only of "a runner is in my map" -- the real +// gate is ListProvisionedDataSources's SQL WHERE clause, already +// covered by rbacstore_test.go's +// TestListProvisionedDataSourcesExcludesUnprovisionedAndInactive. This +// test is the Docker-free proof that RunSQL's refusal actually holds on +// the empty-map end of that gate, complementing +// TestRegistryRefusesUnknownTenant's live-ClickHouse proof on the +// populated end. +func TestRegistryRefusesMidProvisioningTenant(t *testing.T) { + ctx := context.Background() + reg, err := New(ctx, "unused:9000", nil) + if err != nil { + t.Fatalf("New: %v", err) + } + defer reg.Close() + + reqCtx := authz.WithIdentity(ctx, authz.Identity{TenantID: "mid-provisioning-tenant", Role: authz.RoleViewer}) + if _, err := reg.RunSQL(reqCtx, "SELECT 1"); err == nil { + t.Fatal("expected RunSQL to refuse a tenant that hasn't reached the active+credentialed gate, not silently serve it") + } +} diff --git a/enterprise/internal/rbacstore/rbacstore.go b/enterprise/internal/rbacstore/rbacstore.go index f703fe3..4e70fd3 100644 --- a/enterprise/internal/rbacstore/rbacstore.go +++ b/enterprise/internal/rbacstore/rbacstore.go @@ -178,6 +178,26 @@ func (s *Store) GetTenant(ctx context.Context, id string) (*Tenant, error) { return &t, nil } +// TenantIsActive answers a narrow, frequently-asked question -- it +// implements enterprise/internal/searchclient.TenantChecker +// structurally (no import needed in that direction; see that package's +// doc comment for why Tantivy's per-tenant index resolution needs this +// check where chrunner's ClickHouse routing gets the equivalent +// guarantee for free from its immutable startup-built connection map). +// Backed by GetTenant, never cached -- SetTenantStatus's doc comment +// already establishes "re-check server-side, never assume 'active'" as +// this package's convention. +func (s *Store) TenantIsActive(ctx context.Context, tenantID string) (bool, error) { + t, err := s.GetTenant(ctx, tenantID) + if err != nil { + if errors.Is(err, ErrNotFound) { + return false, nil + } + return false, err + } + return t.Status == "active", nil +} + // SetTenantStatus is the only way a tenant's status column changes -- // every tenant-resolution path elsewhere must re-check this via // GetTenant, never cache/assume 'active', per diff --git a/enterprise/internal/rbacstore/rbacstore_test.go b/enterprise/internal/rbacstore/rbacstore_test.go index 607977a..b1872c8 100644 --- a/enterprise/internal/rbacstore/rbacstore_test.go +++ b/enterprise/internal/rbacstore/rbacstore_test.go @@ -607,3 +607,48 @@ func TestGetUserByEmailNotFound(t *testing.T) { t.Fatalf("GetUserByEmail error = %v, want ErrNotFound", err) } } + +// TestTenantIsActive is the rbacstore-side half of Phase 4 task 8's +// item 4 adversarial probe -- enterprise/internal/searchclient's +// TestSearchRefusesMidProvisioningTenant proves Search refuses when +// TenantChecker.TenantIsActive returns false; this proves the real +// implementation actually returns false for a mid-provisioning tenant +// and only ever returns true once SetTenantStatus marks it active. +func TestTenantIsActive(t *testing.T) { + s := testStore(t) + ctx := context.Background() + tenantID := "test-tenant-" + uniqueSuffix() + + if _, err := s.CreateTenant(ctx, tenantID, "Test Tenant"); err != nil { + t.Fatalf("CreateTenant: %v", err) + } + active, err := s.TenantIsActive(ctx, tenantID) + if err != nil { + t.Fatalf("TenantIsActive (provisioning): %v", err) + } + if active { + t.Fatal("a freshly-created tenant (status 'provisioning') must not be active") + } + + if err := s.SetTenantStatus(ctx, tenantID, "active"); err != nil { + t.Fatalf("SetTenantStatus: %v", err) + } + active, err = s.TenantIsActive(ctx, tenantID) + if err != nil { + t.Fatalf("TenantIsActive (active): %v", err) + } + if !active { + t.Fatal("expected the tenant to be active after SetTenantStatus") + } +} + +func TestTenantIsActiveNonexistentTenant(t *testing.T) { + s := testStore(t) + active, err := s.TenantIsActive(context.Background(), "does-not-exist-"+uniqueSuffix()) + if err != nil { + t.Fatalf("TenantIsActive: %v, want a plain (false, nil) for a nonexistent tenant, not an error", err) + } + if active { + t.Fatal("a nonexistent tenant must not be reported active") + } +} diff --git a/enterprise/internal/searchclient/searchclient.go b/enterprise/internal/searchclient/searchclient.go index acd676f..2721c10 100644 --- a/enterprise/internal/searchclient/searchclient.go +++ b/enterprise/internal/searchclient/searchclient.go @@ -13,6 +13,25 @@ // 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. +// +// One real divergence from chrunner, found while closing +// /docs/phase-4-isolation-design.md's verification-plan item 4 (a +// mid-provisioning tenant must be refused, not served): search/src/ +// registry.rs's IndexRegistry opens-or-creates an index for *any* +// syntactically-valid tenant_id on first request -- it has no concept of +// "is this tenant actually provisioned," because it's a separate +// process with no Postgres access, so it structurally can't know. +// chrunner gets its fail-closed property for free (a tenant not yet +// active+credentialed is simply absent from the immutable map +// enterprise-api's main.go builds at startup from +// rbacstore.ListProvisionedDataSources) -- Tantivy has no equivalent +// startup-time gate, so without a check *here*, a query against a +// mid-provisioning (or entirely made-up) tenant would silently succeed +// with zero results from a freshly-created empty index, rather than +// refusing -- "ambient success" masquerading as "no matching logs," +// exactly the failure mode the verification plan named. TenantChecker +// closes that: Search now refuses before the gRPC call ever goes out if +// the tenant isn't active in rbacstore. package searchclient import ( @@ -26,20 +45,38 @@ import ( searchv1 "github.com/sentry/sentry/proto/sentry/search/v1" ) +// TenantChecker answers "is this tenant allowed to search at all" -- +// backed by rbacstore.Store.TenantIsActive in production (a narrow +// interface, not *rbacstore.Store directly, so this package doesn't +// need rbacstore's full surface and tests can fake it without a live +// Postgres). Never cached: SetTenantStatus's doc comment already +// established "every tenant-resolution path elsewhere must re-check +// this via GetTenant, never cache/assume 'active'" for chrunner-style +// resolution, and the same reasoning applies here. +type TenantChecker interface { + TenantIsActive(ctx context.Context, tenantID string) (bool, error) +} + type Client struct { - grpc searchv1.SearchServiceClient - conn *grpc.ClientConn + grpc searchv1.SearchServiceClient + conn *grpc.ClientConn + tenants TenantChecker } // 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) { +// internal-service-to-service trust boundary) aside from the added +// TenantChecker -- the only difference from that package is what Search +// does with the resolved tenant. tenants is required, not optional: +// every production caller of this package is enterprise-api, which +// always has a live rbacstore.Store to pass (unlike, say, +// authz.Authorizer, there is no legitimate deployment shape where this +// package runs without one). +func Dial(addr string, tenants TenantChecker) (*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 + return &Client{grpc: searchv1.NewSearchServiceClient(conn), conn: conn, tenants: tenants}, nil } func (c *Client) Close() error { @@ -48,11 +85,14 @@ func (c *Client) Close() error { // 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. +// identity, an identity with no tenant (RoleService, or a misconfigured +// authorizer), or a tenant that isn't active in rbacstore all refuse the +// call rather than reaching `search`, which would otherwise silently +// open-or-create a fresh empty index for a tenant that was never +// actually provisioned (see this file's package doc comment). This +// mirrors chrunner.Registry.RunSQL's exact fail-closed shape, just with +// an explicit check where chrunner gets the same guarantee for free from +// its immutable connection map. func (c *Client) Search(ctx context.Context, query string, limit uint32) ([]string, error) { identity, ok := authz.IdentityFromContext(ctx) if !ok { @@ -61,6 +101,13 @@ func (c *Client) Search(ctx context.Context, query string, limit uint32) ([]stri if identity.TenantID == "" { return nil, fmt.Errorf("searchclient: authenticated identity %q has no tenant, refusing to search", identity.Role) } + active, err := c.tenants.TenantIsActive(ctx, identity.TenantID) + if err != nil { + return nil, fmt.Errorf("searchclient: checking tenant %q status: %w", identity.TenantID, err) + } + if !active { + return nil, fmt.Errorf("searchclient: tenant %q is not active, refusing to search", identity.TenantID) + } resp, err := c.grpc.Search(ctx, &searchv1.SearchRequest{Query: query, Limit: limit, TenantId: identity.TenantID}) if err != nil { diff --git a/enterprise/internal/searchclient/searchclient_test.go b/enterprise/internal/searchclient/searchclient_test.go index b84c8c7..c9262cd 100644 --- a/enterprise/internal/searchclient/searchclient_test.go +++ b/enterprise/internal/searchclient/searchclient_test.go @@ -8,6 +8,7 @@ package searchclient import ( "context" + "fmt" "net" "testing" @@ -28,7 +29,23 @@ func (f *fakeSearchServer) Search(_ context.Context, req *searchv1.SearchRequest return &searchv1.SearchResponse{RecordIds: f.recordIDs}, nil } -func newTestServer(t *testing.T) (*Client, *fakeSearchServer) { +// fakeTenantChecker is an in-memory TenantChecker -- active lists which +// tenant IDs count as active, everything else answers false (not an +// error), matching rbacstore.TenantIsActive's shape (a nonexistent +// tenant is "not active," not a lookup failure). +type fakeTenantChecker struct { + active map[string]bool + err error +} + +func (f *fakeTenantChecker) TenantIsActive(_ context.Context, tenantID string) (bool, error) { + if f.err != nil { + return false, f.err + } + return f.active[tenantID], nil +} + +func newTestServer(t *testing.T, tenants TenantChecker) (*Client, *fakeSearchServer) { t.Helper() lis, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { @@ -40,7 +57,7 @@ func newTestServer(t *testing.T) (*Client, *fakeSearchServer) { go func() { _ = srv.Serve(lis) }() t.Cleanup(srv.Stop) - client, err := Dial(lis.Addr().String()) + client, err := Dial(lis.Addr().String(), tenants) if err != nil { t.Fatalf("Dial: %v", err) } @@ -49,7 +66,7 @@ func newTestServer(t *testing.T) (*Client, *fakeSearchServer) { } func TestSearchForwardsTenantIDFromContext(t *testing.T) { - client, fake := newTestServer(t) + client, fake := newTestServer(t, &fakeTenantChecker{active: map[string]bool{"acme": true}}) ctx := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer}) ids, err := client.Search(ctx, "error", 10) @@ -68,7 +85,7 @@ func TestSearchForwardsTenantIDFromContext(t *testing.T) { } func TestSearchRefusesWithNoIdentity(t *testing.T) { - client, fake := newTestServer(t) + client, fake := newTestServer(t, &fakeTenantChecker{active: map[string]bool{"acme": true}}) if _, err := client.Search(context.Background(), "error", 10); err == nil { t.Fatal("expected Search to refuse a request with no authenticated identity in context") @@ -79,7 +96,7 @@ func TestSearchRefusesWithNoIdentity(t *testing.T) { } func TestSearchRefusesIdentityWithNoTenant(t *testing.T) { - client, fake := newTestServer(t) + client, fake := newTestServer(t, &fakeTenantChecker{active: map[string]bool{"acme": true}}) // 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. @@ -94,7 +111,7 @@ func TestSearchRefusesIdentityWithNoTenant(t *testing.T) { } func TestSearchDifferentTenantsSendDifferentTenantIDs(t *testing.T) { - client, fake := newTestServer(t) + client, fake := newTestServer(t, &fakeTenantChecker{active: map[string]bool{"acme": true, "globex": true}}) ctxA := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer}) if _, err := client.Search(ctxA, "q", 5); err != nil { @@ -112,3 +129,44 @@ func TestSearchDifferentTenantsSendDifferentTenantIDs(t *testing.T) { t.Fatalf("TenantId = %q, want globex", fake.lastRequest.TenantId) } } + +// TestSearchRefusesMidProvisioningTenant is Phase 4 task 8's item 4 +// adversarial probe (see /docs/phase-4-isolation-design.md's +// verification plan and api/queryapi/tenant_isolation_gap_test.go), +// closed here without needing Docker or a live Postgres: a tenant that +// exists (an authenticated identity can carry its ID -- e.g. right +// after enterprise-auth -create-tenant / -grant-membership-* but before +// enterprise-api -provision-tenant runs) but isn't active yet must be +// refused, not silently served a fresh empty index. This is exactly the +// gap search/src/registry.rs's IndexRegistry can't close on its own +// (see this package's doc comment) -- proving it here, at the one layer +// that actually has rbacstore access, is what makes the guarantee real. +func TestSearchRefusesMidProvisioningTenant(t *testing.T) { + client, fake := newTestServer(t, &fakeTenantChecker{active: map[string]bool{"acme": true}}) + // "pending" is deliberately absent from the active set -- simulates + // a tenant row that exists in rbacstore (provisioning, or even + // active-but-not-yet-credentialed) without needing a real tenants + // table to prove the point: fakeTenantChecker.active only ever + // answers "true" for tenants explicitly marked active, exactly like + // rbacstore.TenantIsActive does for a real 'provisioning' row. + ctx := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "pending", Role: authz.RoleViewer}) + + if _, err := client.Search(ctx, "error", 10); err == nil { + t.Fatal("expected Search to refuse a tenant that isn't active yet, not silently search an empty index for it") + } + if fake.lastRequest != nil { + t.Fatal("expected the gRPC call to never reach the server for a non-active tenant -- the whole point is refusing before search/src/registry.rs ever gets a chance to open-or-create an index for it") + } +} + +func TestSearchPropagatesTenantCheckerError(t *testing.T) { + client, fake := newTestServer(t, &fakeTenantChecker{err: fmt.Errorf("boom")}) + ctx := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer}) + + if _, err := client.Search(ctx, "error", 10); err == nil { + t.Fatal("expected Search to fail closed when the tenant status check itself errors, not treat an error as \"not active but otherwise fine\"") + } + if fake.lastRequest != nil { + t.Fatal("expected the gRPC call to never reach the server when the tenant check errored") + } +}