Close the last tenant-isolation adversarial probe (mid-provisioning tenants)
Phase 4 task 8's verification plan named four adversarial probes; three were closed earlier this phase, the fourth (an evaluator tick, or any other caller, hitting a tenant that exists but hasn't reached the active+credentialed gate yet -- must be refused, not served) was still an explicitly-skipped stub in api/queryapi/tenant_isolation_gap_test.go. Investigating it found the two storage engines needed genuinely different treatment: - ClickHouse (enterprise/internal/chrunner) already had this property structurally, for free: Registry is built once at startup from rbacstore.ListProvisionedDataSources, which already filters to active+credentialed tenants only, so a mid-provisioning tenant is simply absent from the connection map. New test TestRegistryRefusesMidProvisioningTenant proves this without Docker -- an empty DataSource list never dials ClickHouse, so this genuinely runs in this environment, unlike every other test in that file. - Tantivy (search/src/registry.rs's IndexRegistry) was a real, different gap, not just an unverified assumption: it opens-or-creates an index for any syntactically-valid tenant_id on first request, because it's a separate process with no Postgres access and structurally can't know which tenants are actually provisioned. A query against a mid-provisioning tenant would have silently succeeded with zero results from a freshly-created empty index -- "ambient success" indistinguishable from "no matching logs," exactly the failure mode this item was worried about. Fixed the Tantivy gap with a new enterprise/internal/searchclient. TenantChecker interface (backed by a new rbacstore.TenantIsActive, implemented structurally, no new import edge needed), consulted before every gRPC call: Client.Search now refuses a non-active tenant before it ever reaches `search`. Dial's signature gained a required TenantChecker parameter; enterprise-api's main.go passes its existing rbacstore.Store (already satisfies the interface). Verified Docker-free via searchclient's existing real-in-process-gRPC-server test harness (TestSearchRefusesMidProvisioningTenant, plus TestSearchPropagatesTenantCheckerError for the fail-closed-on-error case) -- both genuinely run in this environment, same bar as the rest of the Tantivy isolation work. rbacstore.TenantIsActive itself has two new skip-gated live-Postgres tests (TestTenantIsActive, TestTenantIsActiveNonexistentTenant) -- disclosed as not run against a live database here, same gap as the rest of this phase's Postgres-backed pieces. api/queryapi/tenant_isolation_gap_test.go rewritten from a checklist with one skipped stub to a full accounting of all four now-closed probes. Docs updated in lockstep: CLAUDE.md, threat-model.md, phase-4-isolation-design.md (implementation note added after its original sign-off), phase-4-runbook.md (§9), enterprise/README.md.
This commit is contained in:
@@ -173,7 +173,23 @@ built too
|
|||||||
**genuinely verified**, like the OIDC login flow: Tantivy is an embedded
|
**genuinely verified**, like the OIDC login flow: Tantivy is an embedded
|
||||||
library, not a networked service, so the isolation probe (three tenants,
|
library, not a networked service, so the isolation probe (three tenants,
|
||||||
same search term, scoped search returns only that tenant's document)
|
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:
|
topology gap that briefly was the largest one is now closed for Helm:
|
||||||
`deploy/helm/sentry/templates/api.yaml`/`enterprise-api.yaml` are
|
`deploy/helm/sentry/templates/api.yaml`/`enterprise-api.yaml` are
|
||||||
mutually exclusive on the same `enterprise.enabled` flag that turns on
|
mutually exclusive on the same `enterprise.enabled` flag that turns on
|
||||||
|
|||||||
@@ -3,7 +3,7 @@
|
|||||||
// "Verification plan for this design specifically" section names for
|
// "Verification plan for this design specifically" section names for
|
||||||
// Phase 4 task 8 have a permanent, grep-able home in the test tree.
|
// 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):
|
// - Item 1 (fully-qualified cross-tenant raw SQL):
|
||||||
// enterprise/internal/tenantprovision/tenantprovision_test.go's
|
// enterprise/internal/tenantprovision/tenantprovision_test.go's
|
||||||
@@ -20,25 +20,46 @@
|
|||||||
// searchclient/searchclient_test.go (the Go client that resolves
|
// searchclient/searchclient_test.go (the Go client that resolves
|
||||||
// tenant_id from request identity, wire-level verified against a
|
// tenant_id from request identity, wire-level verified against a
|
||||||
// real in-process gRPC server).
|
// 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
|
// Tantivy was a real, different gap, not just an unverified
|
||||||
// scope boundary all three closed items share: they prove *read*
|
// assumption: search/src/registry.rs's IndexRegistry opens-or-
|
||||||
// isolation given tenant-scoped data exists -- they do not prove
|
// creates an index for *any* syntactically-valid tenant_id on first
|
||||||
// ingest/write-path tenancy, which doesn't exist yet (every record
|
// request, because it's a separate process with no Postgres access
|
||||||
// ingest produces lands in the single shared ClickHouse database and
|
// and structurally can't know which tenants are actually
|
||||||
// Tantivy index regardless of tenant) -- see
|
// provisioned. Without a check upstream, a query against a
|
||||||
// /docs/security/threat-model.md.
|
// 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
|
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.")
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -315,3 +315,25 @@ suite must include, against the live stack:
|
|||||||
- A simulated evaluator tick firing mid-provisioning (tenant row exists,
|
- A simulated evaluator tick firing mid-provisioning (tenant row exists,
|
||||||
grants not yet confirmed) to confirm it's refused, not silently served
|
grants not yet confirmed) to confirm it's refused, not silently served
|
||||||
against a partially-provisioned or default-profile connection.
|
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`).
|
||||||
|
|||||||
+33
-9
@@ -386,14 +386,27 @@ cargo test
|
|||||||
cd ../enterprise
|
cd ../enterprise
|
||||||
go test ./internal/searchclient/... -v
|
go test ./internal/searchclient/... -v
|
||||||
# real in-process gRPC server, confirms SearchRequest.tenant_id is set
|
# 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
|
This is the one piece of Phase 4's tenant isolation work that has
|
||||||
**actually been run and confirmed passing** in an environment without
|
**actually been run and confirmed passing** in an environment without
|
||||||
Docker access, alongside `enterprise/internal/loginhandler`'s OIDC
|
Docker access, alongside `enterprise/internal/loginhandler`'s OIDC/SAML
|
||||||
tests (§3a) — both are unusually strong evidence precisely because they
|
tests (§3a/§3b) — both are unusually strong evidence precisely because
|
||||||
needed no infrastructure this environment lacked.
|
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
|
## 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
|
(`enterprise/internal/rbacstore/rbacstore_test.go`) haven't run
|
||||||
against a live database in this environment, same gap as the rest of
|
against a live database in this environment, same gap as the rest of
|
||||||
this phase's Postgres-backed pieces.
|
this phase's Postgres-backed pieces.
|
||||||
- Three of the four adversarial ClickHouse/Tantivy probes named in
|
- All four adversarial ClickHouse/Tantivy probes named in
|
||||||
`/docs/phase-4-isolation-design.md`'s verification plan are closed
|
`/docs/phase-4-isolation-design.md`'s verification plan are now closed
|
||||||
(§8, §9); the last (mid-provisioning-race handling) is still stubbed
|
-- see `api/queryapi/tenant_isolation_gap_test.go` for the full
|
||||||
as an explicitly-skipped test in
|
accounting. The fourth (mid-provisioning-race handling) turned out to
|
||||||
`api/queryapi/tenant_isolation_gap_test.go`.
|
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
|
## Tearing down
|
||||||
|
|
||||||
|
|||||||
@@ -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`) |
|
| 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 detection (hash chain) | **Enforced**, verified live |
|
||||||
| Audit log tamper prevention (external anchoring) | **Design only** — `FileSink` is a dev stand-in |
|
| 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** |
|
| Protection against a privileged DB administrator | **Explicit non-goal** |
|
||||||
|
|||||||
@@ -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
|
that tenant's document) actually ran: `search`'s
|
||||||
`cargo test`/`cargo clippy --all-targets -- -D warnings` and this
|
`cargo test`/`cargo clippy --all-targets -- -D warnings` and this
|
||||||
package's `go test` both pass clean, no Docker or live database
|
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`,
|
- `cmd/enterprise-api`: a second binary (alongside `api/cmd/api`,
|
||||||
unchanged) importing *both* `api`'s handler packages and the
|
unchanged) importing *both* `api`'s handler packages and the
|
||||||
tenant-aware implementations above -- see its own doc comment for why
|
tenant-aware implementations above -- see its own doc comment for why
|
||||||
|
|||||||
@@ -125,7 +125,7 @@ func main() {
|
|||||||
}
|
}
|
||||||
defer registry.Close()
|
defer registry.Close()
|
||||||
|
|
||||||
search, err := searchclient.Dial(cfg.SearchGRPCAddr)
|
search, err := searchclient.Dial(cfg.SearchGRPCAddr, rbac)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
logger.Error("dialing search service", "error", err)
|
logger.Error("dialing search service", "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
|
|||||||
@@ -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")
|
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")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -178,6 +178,26 @@ func (s *Store) GetTenant(ctx context.Context, id string) (*Tenant, error) {
|
|||||||
return &t, nil
|
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 --
|
// SetTenantStatus is the only way a tenant's status column changes --
|
||||||
// every tenant-resolution path elsewhere must re-check this via
|
// every tenant-resolution path elsewhere must re-check this via
|
||||||
// GetTenant, never cache/assume 'active', per
|
// GetTenant, never cache/assume 'active', per
|
||||||
|
|||||||
@@ -607,3 +607,48 @@ func TestGetUserByEmailNotFound(t *testing.T) {
|
|||||||
t.Fatalf("GetUserByEmail error = %v, want ErrNotFound", err)
|
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")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -13,6 +13,25 @@
|
|||||||
// the authenticated request identity before every call -- exactly the
|
// the authenticated request identity before every call -- exactly the
|
||||||
// same "read from ctx, never a parameter, fail closed if absent" shape
|
// same "read from ctx, never a parameter, fail closed if absent" shape
|
||||||
// chrunner.Registry.RunSQL uses for ClickHouse.
|
// 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
|
package searchclient
|
||||||
|
|
||||||
import (
|
import (
|
||||||
@@ -26,20 +45,38 @@ import (
|
|||||||
searchv1 "github.com/sentry/sentry/proto/sentry/search/v1"
|
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 {
|
type Client struct {
|
||||||
grpc searchv1.SearchServiceClient
|
grpc searchv1.SearchServiceClient
|
||||||
conn *grpc.ClientConn
|
conn *grpc.ClientConn
|
||||||
|
tenants TenantChecker
|
||||||
}
|
}
|
||||||
|
|
||||||
// Dial mirrors api/searchclient.Dial exactly (same plain-TCP, no-TLS
|
// Dial mirrors api/searchclient.Dial exactly (same plain-TCP, no-TLS
|
||||||
// internal-service-to-service trust boundary) -- the only difference
|
// internal-service-to-service trust boundary) aside from the added
|
||||||
// from that package is what Search does with the resolved tenant.
|
// TenantChecker -- the only difference from that package is what Search
|
||||||
func Dial(addr string) (*Client, error) {
|
// 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()))
|
conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials()))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("dialing search service at %s: %w", addr, err)
|
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 {
|
func (c *Client) Close() error {
|
||||||
@@ -48,11 +85,14 @@ func (c *Client) Close() error {
|
|||||||
|
|
||||||
// Search implements executor.SearchClient. Resolves the caller's tenant
|
// Search implements executor.SearchClient. Resolves the caller's tenant
|
||||||
// from ctx (never a parameter) and fails closed -- no authenticated
|
// from ctx (never a parameter) and fails closed -- no authenticated
|
||||||
// identity, or an identity with no tenant (RoleService, or a
|
// identity, an identity with no tenant (RoleService, or a misconfigured
|
||||||
// misconfigured authorizer), refuses the call rather than falling back
|
// authorizer), or a tenant that isn't active in rbacstore all refuse the
|
||||||
// to the single default index, which would silently defeat the whole
|
// call rather than reaching `search`, which would otherwise silently
|
||||||
// point of this package existing. This mirrors chrunner.Registry.RunSQL's
|
// open-or-create a fresh empty index for a tenant that was never
|
||||||
// exact fail-closed shape.
|
// 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) {
|
func (c *Client) Search(ctx context.Context, query string, limit uint32) ([]string, error) {
|
||||||
identity, ok := authz.IdentityFromContext(ctx)
|
identity, ok := authz.IdentityFromContext(ctx)
|
||||||
if !ok {
|
if !ok {
|
||||||
@@ -61,6 +101,13 @@ func (c *Client) Search(ctx context.Context, query string, limit uint32) ([]stri
|
|||||||
if identity.TenantID == "" {
|
if identity.TenantID == "" {
|
||||||
return nil, fmt.Errorf("searchclient: authenticated identity %q has no tenant, refusing to search", identity.Role)
|
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})
|
resp, err := c.grpc.Search(ctx, &searchv1.SearchRequest{Query: query, Limit: limit, TenantId: identity.TenantID})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ package searchclient
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"fmt"
|
||||||
"net"
|
"net"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
@@ -28,7 +29,23 @@ func (f *fakeSearchServer) Search(_ context.Context, req *searchv1.SearchRequest
|
|||||||
return &searchv1.SearchResponse{RecordIds: f.recordIDs}, nil
|
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()
|
t.Helper()
|
||||||
lis, err := net.Listen("tcp", "127.0.0.1:0")
|
lis, err := net.Listen("tcp", "127.0.0.1:0")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -40,7 +57,7 @@ func newTestServer(t *testing.T) (*Client, *fakeSearchServer) {
|
|||||||
go func() { _ = srv.Serve(lis) }()
|
go func() { _ = srv.Serve(lis) }()
|
||||||
t.Cleanup(srv.Stop)
|
t.Cleanup(srv.Stop)
|
||||||
|
|
||||||
client, err := Dial(lis.Addr().String())
|
client, err := Dial(lis.Addr().String(), tenants)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Dial: %v", err)
|
t.Fatalf("Dial: %v", err)
|
||||||
}
|
}
|
||||||
@@ -49,7 +66,7 @@ func newTestServer(t *testing.T) (*Client, *fakeSearchServer) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestSearchForwardsTenantIDFromContext(t *testing.T) {
|
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})
|
ctx := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer})
|
||||||
|
|
||||||
ids, err := client.Search(ctx, "error", 10)
|
ids, err := client.Search(ctx, "error", 10)
|
||||||
@@ -68,7 +85,7 @@ func TestSearchForwardsTenantIDFromContext(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestSearchRefusesWithNoIdentity(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 {
|
if _, err := client.Search(context.Background(), "error", 10); err == nil {
|
||||||
t.Fatal("expected Search to refuse a request with no authenticated identity in context")
|
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) {
|
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
|
// RoleService identities carry no TenantID -- see api/authz.Identity's
|
||||||
// doc comment. /alerting never calls search directly today, but the
|
// doc comment. /alerting never calls search directly today, but the
|
||||||
// fail-closed behavior must hold regardless of how this arises.
|
// fail-closed behavior must hold regardless of how this arises.
|
||||||
@@ -94,7 +111,7 @@ func TestSearchRefusesIdentityWithNoTenant(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestSearchDifferentTenantsSendDifferentTenantIDs(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})
|
ctxA := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", Role: authz.RoleViewer})
|
||||||
if _, err := client.Search(ctxA, "q", 5); err != nil {
|
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)
|
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")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user