diff --git a/CLAUDE.md b/CLAUDE.md index a0c13d3..bc22479 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -140,18 +140,29 @@ Non-goals for this phase (same discipline as every phase so far): ## What "done" looks like for Phase 4 -**Status: in progress, not shipped.** Through task 8: RBAC enforcement -(`api/internal/authz`), the `alerting`↔`api` service-identity credential, -tenant-scoped dashboards, and append-only audit logging are built and -tested (including live-Postgres verification for audit logging and -rbacstore). The two items this phase's exit criteria below actually -hinge on are **not** built: SSO login (OIDC/SAML protocol wiring exists; -no HTTP login handler calls it) and — the highest-risk one — tenant -isolation for log data itself (every tenant's `/query` still executes -against one shared ClickHouse connection and Tantivy index; RBAC -controls who can query, not what a query can see). Full accounting: -`/docs/security/threat-model.md`; step-by-step verification procedure -(not yet run against a live cluster in this environment): +**Status: in progress, not shipped.** RBAC enforcement (`api/authz`), the +`alerting`↔`api` service-identity credential, tenant-scoped dashboards, +append-only audit logging, and — since the second pass on this phase — +real per-tenant ClickHouse provisioning and query routing +(`enterprise/internal/tenantprovision`, `enterprise/internal/chrunner`, +wired into a new `enterprise/cmd/enterprise-api` binary alongside plain +`api/cmd/api`) are all built and tested — real integration tests exist +for the ClickHouse pieces, but this environment lost Docker/database +access partway through the phase, so only the audit-logging guarantees +were actually confirmed against a live database; the rest is untested +beyond "compiles, and skips cleanly when no live database is +configured" (see `/docs/phase-4-runbook.md`'s verification-status +section). Two things still keep this phase from being done: SSO login +(OIDC/SAML protocol wiring exists, no +HTTP login handler calls it), 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: `/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 bar this phase is aiming at, not a completed state. diff --git a/api/internal/authz/authz.go b/api/authz/authz.go similarity index 85% rename from api/internal/authz/authz.go rename to api/authz/authz.go index f762a23..ae00525 100644 --- a/api/internal/authz/authz.go +++ b/api/authz/authz.go @@ -75,6 +75,13 @@ func IdentityFromContext(ctx context.Context) (Identity, bool) { return id, ok } -func withIdentity(ctx context.Context, id Identity) context.Context { +// WithIdentity attaches an already-resolved Identity to ctx -- exported +// (not just middleware.go's internal use) so packages that construct +// their own request context outside an HTTP handler -- e.g. enterprise/ +// internal/chrunner's tests, or a future non-HTTP caller -- can put a +// real Identity in context the same way RequireRole/RequireRoleOrService +// do, rather than reaching for an unexported field via reflection or +// duplicating this one-line function. +func WithIdentity(ctx context.Context, id Identity) context.Context { return context.WithValue(ctx, identityContextKey{}, id) } diff --git a/api/internal/authz/authz_test.go b/api/authz/authz_test.go similarity index 100% rename from api/internal/authz/authz_test.go rename to api/authz/authz_test.go diff --git a/api/internal/authz/httpauthz.go b/api/authz/httpauthz.go similarity index 100% rename from api/internal/authz/httpauthz.go rename to api/authz/httpauthz.go diff --git a/api/internal/authz/httpauthz_test.go b/api/authz/httpauthz_test.go similarity index 100% rename from api/internal/authz/httpauthz_test.go rename to api/authz/httpauthz_test.go diff --git a/api/internal/authz/middleware.go b/api/authz/middleware.go similarity index 94% rename from api/internal/authz/middleware.go rename to api/authz/middleware.go index 4214c37..e9ce23e 100644 --- a/api/internal/authz/middleware.go +++ b/api/authz/middleware.go @@ -42,7 +42,7 @@ func RequireRole(authorizer Authorizer, minRole Role, next http.HandlerFunc) htt writeForbidden(w) return } - next(w, r.WithContext(withIdentity(r.Context(), identity))) + next(w, r.WithContext(WithIdentity(r.Context(), identity))) } } @@ -67,6 +67,6 @@ func RequireRoleOrService(authorizer Authorizer, minRole Role, next http.Handler writeForbidden(w) return } - next(w, r.WithContext(withIdentity(r.Context(), identity))) + next(w, r.WithContext(WithIdentity(r.Context(), identity))) } } diff --git a/api/internal/authz/middleware_test.go b/api/authz/middleware_test.go similarity index 100% rename from api/internal/authz/middleware_test.go rename to api/authz/middleware_test.go diff --git a/api/cmd/api/main.go b/api/cmd/api/main.go index d5ddd19..e4add2f 100644 --- a/api/cmd/api/main.go +++ b/api/cmd/api/main.go @@ -1,7 +1,7 @@ // Command api is Sentry's query API: a single POST /query endpoint // accepting either the pipe syntax or raw SQL, compiled and routed // across ClickHouse and search by internal/querylang. See -// internal/queryapi and /docs/query-language-design.md for why this is +// queryapi and /docs/query-language-design.md for why this is // plain REST rather than the pinned gRPC+gateway pattern. package main @@ -19,13 +19,13 @@ import ( "github.com/ClickHouse/clickhouse-go/v2" "github.com/jackc/pgx/v5/pgxpool" - "github.com/sentry/sentry/api/internal/authz" + "github.com/sentry/sentry/api/authz" + "github.com/sentry/sentry/api/dashboards" + "github.com/sentry/sentry/api/httpserver" "github.com/sentry/sentry/api/internal/config" - "github.com/sentry/sentry/api/internal/dashboards" - "github.com/sentry/sentry/api/internal/httpserver" - "github.com/sentry/sentry/api/internal/queryapi" - "github.com/sentry/sentry/api/internal/querylang/executor" - "github.com/sentry/sentry/api/internal/searchclient" + "github.com/sentry/sentry/api/queryapi" + "github.com/sentry/sentry/api/querylang/executor" + "github.com/sentry/sentry/api/searchclient" ) func main() { @@ -104,7 +104,7 @@ func main() { dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool), authorizer) // One shared mux, CORS applied once around the whole thing -- see - // internal/httpserver's doc comment for why this changed from each + // httpserver's doc comment for why this changed from each // handler wrapping itself individually. mux := http.NewServeMux() queryHandler.RegisterRoutes(mux) diff --git a/api/internal/dashboards/handler.go b/api/dashboards/handler.go similarity index 99% rename from api/internal/dashboards/handler.go rename to api/dashboards/handler.go index 1636dc7..f8996dd 100644 --- a/api/internal/dashboards/handler.go +++ b/api/dashboards/handler.go @@ -7,7 +7,7 @@ import ( "log/slog" "net/http" - "github.com/sentry/sentry/api/internal/authz" + "github.com/sentry/sentry/api/authz" ) // store is the narrow interface Handler depends on -- *Store (store.go) diff --git a/api/internal/dashboards/handler_test.go b/api/dashboards/handler_test.go similarity index 99% rename from api/internal/dashboards/handler_test.go rename to api/dashboards/handler_test.go index b7eb67b..1871e1b 100644 --- a/api/internal/dashboards/handler_test.go +++ b/api/dashboards/handler_test.go @@ -11,7 +11,7 @@ import ( "strings" "testing" - "github.com/sentry/sentry/api/internal/authz" + "github.com/sentry/sentry/api/authz" ) // fakeStore enforces tenant scoping the same way store.go's real @@ -431,7 +431,7 @@ func TestCreateDashboardStoreErrorReturns500(t *testing.T) { // TestServiceIdentityCannotAccessDashboards is the other half of the // service-identity boundary (the /query half is -// api/internal/queryapi's own tests) -- api/internal/authz's own tests +// api/queryapi's own tests) -- api/authz's own tests // already prove RequireRole rejects RoleService in isolation // (TestRequireRolePlainDoesNotAllowService); this proves it holds // through the real dashboards handler, wired the way it's actually diff --git a/api/internal/dashboards/store.go b/api/dashboards/store.go similarity index 100% rename from api/internal/dashboards/store.go rename to api/dashboards/store.go diff --git a/api/internal/dashboards/store_integration_test.go b/api/dashboards/store_integration_test.go similarity index 98% rename from api/internal/dashboards/store_integration_test.go rename to api/dashboards/store_integration_test.go index 44d551e..eeb7269 100644 --- a/api/internal/dashboards/store_integration_test.go +++ b/api/dashboards/store_integration_test.go @@ -12,7 +12,7 @@ // docker run --rm --network sentry_default -v $(pwd)/../../..:/src -w /src/api \ // -e DASHBOARDS_TEST_POSTGRES_ADDR=metadata-postgres:5432 \ // -e DASHBOARDS_TEST_POSTGRES_PASSWORD=sentry-dev-only \ -// golang:1.25-alpine go test ./internal/dashboards/... -run Integration -v +// golang:1.25-alpine go test ./dashboards/... -run Integration -v package dashboards import ( diff --git a/api/internal/dashboards/types.go b/api/dashboards/types.go similarity index 100% rename from api/internal/dashboards/types.go rename to api/dashboards/types.go diff --git a/api/internal/httpserver/cors.go b/api/httpserver/cors.go similarity index 86% rename from api/internal/httpserver/cors.go rename to api/httpserver/cors.go index ac553f1..9d409bf 100644 --- a/api/internal/httpserver/cors.go +++ b/api/httpserver/cors.go @@ -1,6 +1,6 @@ // Package httpserver holds cross-handler HTTP concerns for /api. Phase 3 -// introduced a second handler package (internal/dashboards) alongside -// internal/queryapi, so CORS moved out of individual handlers into one +// introduced a second handler package (dashboards) alongside +// queryapi, so CORS moved out of individual handlers into one // wrap applied around the fully-assembled mux in cmd/api/main.go, rather // than each handler package wrapping itself. package httpserver diff --git a/api/internal/httpserver/cors_test.go b/api/httpserver/cors_test.go similarity index 100% rename from api/internal/httpserver/cors_test.go rename to api/httpserver/cors_test.go diff --git a/api/internal/querylang/ast/ast.go b/api/internal/querylang/ast/ast.go index f4f3e77..6134546 100644 --- a/api/internal/querylang/ast/ast.go +++ b/api/internal/querylang/ast/ast.go @@ -74,7 +74,7 @@ func (FreeText) isTerm() {} type TimeExpr struct { Absolute string IsRelative bool - RelativeSign int // -1 or +1 + RelativeSign int // -1 or +1 RelativeN int RelativeUnit string // "s" | "m" | "h" | "d" | "w" } diff --git a/api/internal/querylang/lexer/lexer.go b/api/internal/querylang/lexer/lexer.go index 0e09fb4..b9c9a73 100644 --- a/api/internal/querylang/lexer/lexer.go +++ b/api/internal/querylang/lexer/lexer.go @@ -12,23 +12,23 @@ type Kind int const ( EOF Kind = iota Illegal - Ident // bare words: field names, keywords, unquoted values/free-text terms - String // quoted string: "..." - Number // 123, 1.5 - Pipe // | - Eq // = - Neq // != - Gt // > - Gte // >= - Lt // < - Lte // <= - Colon // : - Comma // , - LParen // ( - RParen // ) - Minus // - - Plus // + - Star // * (only meaningful inside count(*), same as SQL) + Ident // bare words: field names, keywords, unquoted values/free-text terms + String // quoted string: "..." + Number // 123, 1.5 + Pipe // | + Eq // = + Neq // != + Gt // > + Gte // >= + Lt // < + Lte // <= + Colon // : + Comma // , + LParen // ( + RParen // ) + Minus // - + Plus // + + Star // * (only meaningful inside count(*), same as SQL) ) type Token struct { diff --git a/api/internal/querylang/planner/planner.go b/api/internal/querylang/planner/planner.go index 1888c4c..291c702 100644 --- a/api/internal/querylang/planner/planner.go +++ b/api/internal/querylang/planner/planner.go @@ -18,7 +18,7 @@ import ( type Language string const ( - Auto Language = "" // detect from the query text (default) + Auto Language = "" // detect from the query text (default) SQL Language = "sql" SPL Language = "spl" // the pipe syntax; named to match the query-language-reference doc ) diff --git a/api/internal/queryapi/handler.go b/api/queryapi/handler.go similarity index 97% rename from api/internal/queryapi/handler.go rename to api/queryapi/handler.go index cab73af..670392f 100644 --- a/api/internal/queryapi/handler.go +++ b/api/queryapi/handler.go @@ -19,9 +19,9 @@ import ( "strings" "time" - "github.com/sentry/sentry/api/internal/authz" - "github.com/sentry/sentry/api/internal/querylang/executor" + "github.com/sentry/sentry/api/authz" "github.com/sentry/sentry/api/internal/querylang/planner" + "github.com/sentry/sentry/api/querylang/executor" ) // AuditLogger is core's extension point for query audit logging -- @@ -74,7 +74,7 @@ func NewHandler(logger *slog.Logger, sqlRunner executor.SQLRunner, search execut } // RegisterRoutes adds this handler's routes onto a shared mux. Phase 3 -// introduced a second handler package (internal/dashboards), so CORS is +// introduced a second handler package (dashboards), so CORS is // now applied once, by main.go, around the fully-assembled mux rather // than by each handler wrapping itself individually -- see // httpserver.WithCORS. diff --git a/api/internal/queryapi/handler_test.go b/api/queryapi/handler_test.go similarity index 99% rename from api/internal/queryapi/handler_test.go rename to api/queryapi/handler_test.go index dbd2a2d..6d63f64 100644 --- a/api/internal/queryapi/handler_test.go +++ b/api/queryapi/handler_test.go @@ -12,8 +12,8 @@ import ( "testing" "time" - "github.com/sentry/sentry/api/internal/authz" - "github.com/sentry/sentry/api/internal/querylang/executor" + "github.com/sentry/sentry/api/authz" + "github.com/sentry/sentry/api/querylang/executor" ) type fakeSQLRunner struct { diff --git a/api/internal/queryapi/tenant_isolation_gap_test.go b/api/queryapi/tenant_isolation_gap_test.go similarity index 51% rename from api/internal/queryapi/tenant_isolation_gap_test.go rename to api/queryapi/tenant_isolation_gap_test.go index 2c9e214..38a8521 100644 --- a/api/internal/queryapi/tenant_isolation_gap_test.go +++ b/api/queryapi/tenant_isolation_gap_test.go @@ -1,38 +1,30 @@ -// This file is a checklist, not a passing test suite -- it exists so -// the four adversarial probes /docs/phase-4-isolation-design.md's +// This file is a checklist, not a fully passing test suite -- it exists +// so the four adversarial probes /docs/phase-4-isolation-design.md's // "Verification plan for this design specifically" section names for -// Phase 4 task 8 have a permanent, grep-able home in the test tree, -// even though none of them can run for real yet. +// Phase 4 task 8 have a permanent, grep-able home in the test tree. // -// Why they can't run: every one of these probes needs a *per-tenant* -// ClickHouse user/database or Tantivy index to attack -- and none -// exist. api/internal/querylang/executor.SQLRunner/SearchClient (the -// only two interfaces api/internal/queryapi.Handler talks to) carry no -// tenant field at all, confirmed by reading both interfaces; neither -// does proto/sentry/search/v1/search.proto's SearchRequest. See -// /docs/security/threat-model.md's "Read this first" section for the -// full writeup -- there is currently exactly one shared ClickHouse -// connection and one shared Tantivy index for every tenant, so "does -// tenant A's connection leak tenant B's data" has no meaningful -// operational answer yet: there's only one connection. +// 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. // -// Each Skip below names precisely what has to exist before that test -// can be written for real (enterprise/internal/tenantprovision, -// enterprise/internal/chrunner, enterprise/internal/searchclient -- all -// still unbuilt, per the Phase 4 task 5 summary). Turning a Skip here -// into a real assertion is the acceptance criterion for those packages, -// not a nice-to-have follow-up. +// 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. package queryapi import "testing" -func TestAdversarial_ClickHouseUserCannotReadOtherTenantDatabaseByFullyQualifiedName(t *testing.T) { - t.Skip("BLOCKED on enterprise/internal/tenantprovision + enterprise/internal/chrunner: " + - "needs two real per-tenant ClickHouse users/databases to attempt " + - "`SELECT * FROM other_tenant_db.logs` against. See " + - "/docs/phase-4-isolation-design.md's verification plan, item 1.") -} - 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`, " + diff --git a/api/internal/querylang/executor/chrunner.go b/api/querylang/executor/chrunner.go similarity index 95% rename from api/internal/querylang/executor/chrunner.go rename to api/querylang/executor/chrunner.go index 83bed19..11e04ee 100644 --- a/api/internal/querylang/executor/chrunner.go +++ b/api/querylang/executor/chrunner.go @@ -12,7 +12,7 @@ import ( // ClickHouse and shapes the result into JSON-friendly columns/rows, // discovering the result's column set at query time via reflection since // the query itself is arbitrary. Ported from Phase 0/1's -// api/internal/queryapi.Executor, which this replaces (see task 4) -- +// api/queryapi.Executor, which this replaces (see task 4) -- // same logic, moved here since it's the query-execution layer's // plumbing, not specific to the old placeholder /query handler. type ChRunner struct { diff --git a/api/internal/querylang/executor/executor.go b/api/querylang/executor/executor.go similarity index 100% rename from api/internal/querylang/executor/executor.go rename to api/querylang/executor/executor.go diff --git a/api/internal/querylang/executor/executor_test.go b/api/querylang/executor/executor_test.go similarity index 100% rename from api/internal/querylang/executor/executor_test.go rename to api/querylang/executor/executor_test.go diff --git a/api/internal/querylang/executor/sql.go b/api/querylang/executor/sql.go similarity index 100% rename from api/internal/querylang/executor/sql.go rename to api/querylang/executor/sql.go diff --git a/api/internal/searchclient/client.go b/api/searchclient/client.go similarity index 100% rename from api/internal/searchclient/client.go rename to api/searchclient/client.go diff --git a/deploy/helm/sentry/templates/api.yaml b/deploy/helm/sentry/templates/api.yaml index f3262d5..ddabb85 100644 --- a/deploy/helm/sentry/templates/api.yaml +++ b/deploy/helm/sentry/templates/api.yaml @@ -46,7 +46,7 @@ spec: key: password {{- if .Values.enterprise.enabled }} # Turns on authz.RequireRole*/RequireRoleOrService enforcement - # on /query and /dashboards -- see api/internal/authz and + # on /query and /dashboards -- see api/authz and # /docs/phase-4-rbac-design.md. Off (unset) when # enterprise.enabled is false, matching every nil-authorizer # no-op default in this codebase. diff --git a/docker-compose.yml b/docker-compose.yml index f7d4163..98bede7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -264,6 +264,46 @@ services: timeout: 5s retries: 30 + # Multi-tenant-aware alternative to `api` (Phase 4) -- see + # enterprise/cmd/enterprise-api/main.go's doc comment for why this is + # a second binary rather than a flag on `api`. NOT part of the default + # traffic path: `web`'s VITE_API_BASE_URL still points at `api` + # (localhost:8080), and nothing here provisions any tenants (see that + # binary's -provision-tenant flag) -- included so it can be + # built/run/curled directly, same "available, not defaulted in" shape + # as enterprise-auth above. CLICKHOUSE_ADMIN_USERNAME/PASSWORD reuse + # the same admin credential `clickhouse-migrate` uses, since + # tenantprovision needs access_management, not a tenant-scoped grant. + enterprise-api: + build: + context: . + dockerfile: enterprise/cmd/enterprise-api/Dockerfile + container_name: sentry-enterprise-api + depends_on: + clickhouse-migrate: + condition: service_completed_successfully + metadata-migrate: + condition: service_completed_successfully + ports: + - "8083:8083" + environment: + CLICKHOUSE_ADDR: "clickhouse:9000" + CLICKHOUSE_ADMIN_USERNAME: "default" + CLICKHOUSE_ADMIN_PASSWORD: "sentry-dev-only" + SEARCH_GRPC_ADDR: "search:50052" + POSTGRES_ADDR: "metadata-postgres:5432" + POSTGRES_DATABASE: "sentry_metadata" + POSTGRES_USERNAME: "sentry" + POSTGRES_PASSWORD: "sentry-dev-only" + AUDIT_WRITER_USERNAME: "audit_writer" + AUDIT_WRITER_PASSWORD: "audit-writer-dev-only" + ENTERPRISE_AUTH_URL: "http://enterprise-auth:8082" + healthcheck: + test: ["CMD", "/enterprise-api", "-healthcheck"] + interval: 5s + timeout: 5s + retries: 30 + web: build: context: web diff --git a/docs/architecture.md b/docs/architecture.md index 8e80f2a..c6b5af9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -79,7 +79,7 @@ This split is not to be changed without discussion — see CLAUDE.md. | `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. | | `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) | SSO (OIDC/SAML protocol mechanics), RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), and `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`). Never imported by core — see "Licensing boundary" below. Does **not** yet include per-tenant ClickHouse/Tantivy connection routing or the OIDC/SAML login HTTP handlers — see `/docs/security/threat-model.md`. | +| `enterprise` (Go, commercial license, Phase 4) | SSO (OIDC/SAML protocol mechanics), 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 the OIDC/SAML login HTTP handlers — 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. | @@ -101,32 +101,48 @@ through a tenant-scoped connection the database's own access control enforces — not at the query-compiler layer, since Phase 2's raw-SQL escape hatch is opaque to any compiler-injected filter. -**As built, through Phase 4 task 8:** +**As built, currently:** -- Role-based access control (`api/internal/authz`) is live on `/query` +- Role-based access control (`api/authz`) is live on `/query` and `/dashboards`, resolved via `enterprise-auth` over HTTP. - Control-plane tenant scoping is live for dashboards - (`api/internal/dashboards`'s store filters every query by the + (`api/dashboards`'s store filters every query by the authenticated identity's tenant, never a client-supplied field). - The `alerting`↔`api` service-identity gap (task 2's finding) is closed: a `RoleService` credential, distinct from every human role. -- **The connection-layer isolation itself — the actual design above — - is not built.** `api/internal/querylang/executor.SQLRunner`/ - `SearchClient` and `search`'s gRPC service carry no tenant field - anywhere. There is one shared ClickHouse connection and one shared - Tantivy index for every tenant. RBAC controls *who* can run a query; - nothing yet controls *what data* that query can see. -- `deploy/operator`'s `Tenant` CRD manages only the K8s-side artifact (a - credential Secret) — it doesn't call ClickHouse or provision anything - ClickHouse-side. `enterprise/internal/tenantprovision` (the piece that - would) is unbuilt. +- **ClickHouse connection-layer isolation is built**, but lives in a + second binary: `enterprise/internal/tenantprovision` (real `CREATE + DATABASE`/`CREATE USER`/`GRANT` against ClickHouse) and + `enterprise/internal/chrunner` (a per-tenant connection registry + implementing `api/querylang/executor.SQLRunner`, resolving the + right tenant's connection from the authenticated identity in request + context) are wired into `enterprise/cmd/enterprise-api` — a binary + that imports both `api`'s handler packages and enterprise's + tenant-aware implementations (the allowed `enterprise → api` import + direction; core still never imports `enterprise/`). Real integration + tests assert a tenant cannot read another tenant's database by + fully-qualified name, and that `system.query_log`/`system.tables`/ + `SHOW DATABASES` don't leak across tenants either — written but not + yet run against a live ClickHouse in this environment, see + `/docs/security/threat-model.md` and `/docs/phase-4-runbook.md`'s + verification-status sections. Plain `api/cmd/api` still exists, + 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. +- `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/chrunner` + `internal/searchclient` (the -tenant-scoped implementations of the two interfaces above) and wiring -them into `api/internal/queryapi.Handler` in place of the single shared -connection `api/cmd/api/main.go` opens today is the single largest -remaining gap between this system and the isolation model it was -designed to have. +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. ## Licensing boundary @@ -137,7 +153,7 @@ CI by `hack/check-tenant-boundary.sh`, which greps every build for the import edge. Where core needs a decision only `enterprise/` can make (is this request authorized, what SSO is configured), it calls `enterprise-auth` over plain HTTP instead -(`api/internal/authz.HTTPAuthorizer`, `web`'s `GET /auth/features`) — +(`api/authz.HTTPAuthorizer`, `web`'s `GET /auth/features`) — the same "network boundary, not import boundary" shape `/alerting`↔`api` already used before `enterprise/` existed. diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index b6da9da..71a4e5c 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -8,28 +8,33 @@ logging, and a Kubernetes deployment path. Read those first. Every prior phase's runbook documents claims **checked against the live stack**, not asserted. This one is different, and says so plainly rather -than papering over it: **this session had no working Docker daemon -access and no reachable Kubernetes cluster**, so most of what follows is -a *procedure to run*, not a report of what was already run and passed. -Two exceptions, genuinely verified live against a real Postgres during -earlier Phase 4 tasks (see their own doc comments for the exact `docker -run` invocations): +than papering over it: for the great majority of this phase's work, +**there was no working Docker daemon access and no reachable Kubernetes +cluster**, so most of what follows is a *procedure to run*, not a report +of what was already run and passed. One genuine exception, verified live +against a real Postgres earlier in this phase's work (see its own doc +comments for the exact `docker run` invocations, and note this was +before the environment lost Docker access, not a claim about this +runbook's own session): - `enterprise/internal/audit`'s hash-chain, tamper-detection, and concurrent-write guarantees (task 4). -- `enterprise/internal/rbacstore`'s CRUD, run against a live Postgres - the same way. -Everything else below — the auth-enforcement walkthrough, the dashboards -tenant-scoping fix, the Helm chart, the tenant-operator — has unit/fake- -client/`helm template` coverage (all passing, see each component's own -`go test`/`helm lint` output) but has **not** been exercised against a -real running stack in this session. If you're reading this to decide -whether Phase 4 is production-ready: it isn't yet, independent of this -gap — see `/docs/security/threat-model.md`'s headline finding (log-data -query isolation isn't built). This runbook exists so the first person -with real Docker/K8s access can actually close the loop, not to claim -that already happened. +Everything else — `internal/rbacstore`'s CRUD, the auth-enforcement +walkthrough, the dashboards tenant-scoping fix, the Helm chart, the +tenant-operator, and (newest) `internal/tenantprovision`/ +`internal/chrunner`'s live-ClickHouse tests — has unit/fake-client/ +`helm template` coverage (all passing, see each component's own `go +test`/`helm lint` output, including every `Skip*`-gated integration test +confirmed to skip cleanly offline) but has **not** been exercised +against a real running stack. Be specific when citing this runbook: "the +tests exist and pass structurally" is a true, verified claim; "isolation +was confirmed against real ClickHouse" is not, yet. If you're reading +this to decide whether Phase 4 is production-ready: it isn't yet, +independent of this gap — see `/docs/security/threat-model.md`'s +headline finding. This runbook exists so the first person with real +Docker/K8s access can actually close the loop, not to claim that already +happened. ## 1. Bring up the stack @@ -172,28 +177,74 @@ kubectl get secret sentry-tenant-acme-clickhouse -o yaml Expect `kubectl get tenants` to show `acme` reach `status.phase: Active` and the Secret to contain a generated `username`/`password`/`database`. This proves the K8s-side half of a real two-tenant deployment — it does -**not** prove either tenant has a working ClickHouse database, since -`enterprise/internal/tenantprovision` (the piece that would create one) -isn't built. See `/deploy/README.md` and -`/docs/security/threat-model.md`. +**not** provision a working ClickHouse database itself (the Operator +manages the K8s Secret only); §8 below is the piece that actually +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. + +```sh +docker compose build enterprise-api +docker compose run --rm enterprise-api -provision-tenant=acme -display-name="Acme Corp" +docker compose run --rm enterprise-api -provision-tenant=globex -display-name="Globex Corporation" +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 +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): + +```sh +docker run --rm --network sentry_default -v $(pwd)/enterprise:/src -w /src \ + -e CHRUNNER_TEST_CLICKHOUSE_ADDR=clickhouse:9000 \ + -e CHRUNNER_TEST_CLICKHOUSE_PASSWORD=sentry-dev-only \ + golang:1.25-alpine go test ./internal/chrunner/... -v + +docker run --rm --network sentry_default -v $(pwd)/enterprise:/src -w /src \ + -e TENANTPROVISION_TEST_CLICKHOUSE_ADDR=clickhouse:9000 \ + -e TENANTPROVISION_TEST_CLICKHOUSE_PASSWORD=sentry-dev-only \ + golang:1.25-alpine go test ./internal/tenantprovision/... -v +``` + +Expect all tests to pass, including +`TestProvisionedUserCannotReadSystemTables` (item 2 of +`/docs/phase-4-isolation-design.md`'s verification plan, closed this +pass) and `TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL` (item 1, +closed through the actual production code path, not just +tenantprovision's raw grants). ## Known gaps (do not treat this phase as done without reading these) Full accounting: `/docs/security/threat-model.md`. Headline items: -- **No tenant isolation on log data.** `POST /query` executes against - one shared ClickHouse connection and one shared Tantivy index for - every tenant, regardless of RBAC. This is Phase 4's originally-stated - highest-risk item and it is not resolved. +- **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. - **No human SSO login.** OIDC/SAML protocol wiring exists; the HTTP login/callback handlers that would use it don't. - **No per-resource dashboard grants** (`dashboard_permissions` has a schema, no handler reads it). -- Four adversarial ClickHouse/Tantivy probes named in - `/docs/phase-4-isolation-design.md`'s verification plan are stubbed as - explicitly-skipped tests in `api/internal/queryapi/ - tenant_isolation_gap_test.go`, blocked on the tenant-scoped connection - work above. +- Two 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 + `api/queryapi/tenant_isolation_gap_test.go`. ## Tearing down @@ -214,13 +265,13 @@ be too. set.** Check `api/cmd/api/main.go` actually left `authorizer` nil when `cfg.EnterpriseAuthURL == ""` — a nil `Authorizer` must be a no-op -(`api/internal/authz.RequireRole`'s doc comment). If this regresses, it +(`api/authz.RequireRole`'s doc comment). If this regresses, it breaks every existing Phase 0-3 deployment silently. **A dashboard created by one tenant is visible to another.** This is the exact bug found and fixed in task 7 — see `/docs/security/threat-model.md`'s "application-layer tenant scoping" -section and `api/internal/dashboards/handler_test.go`'s +section and `api/dashboards/handler_test.go`'s `TestCrossTenant*` tests. If this regresses, `Handler.tenantID` or `store.go`'s `WHERE tenant_id = ...` filters have been bypassed somewhere — check every store method still takes and uses a `tenantID` diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 95987e5..71dc7b3 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -9,28 +9,58 @@ for the full design rationale behind the controls described here. ## Read this first: the single most important open finding -**Log data queried through `POST /query` is not tenant-isolated today.** -Every authenticated tenant's ad hoc queries and dashboard panel queries -execute against the same shared ClickHouse connection and the same -shared Tantivy index — there is no per-tenant database, user, or index -routing anywhere in the query execution path -(`api/internal/querylang/executor.SQLRunner`/`SearchClient`, `search`'s -gRPC service, `proto/sentry/search/v1/search.proto`). Confirmed by -reading the actual code, not assumed: neither interface, nor the -`search` proto, carries a tenant field anywhere. +**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." -This is exactly the mechanism `/docs/phase-4-isolation-design.md` -specifies as the core deliverable of tenant isolation (one dedicated -ClickHouse database/user and one dedicated Tantivy index directory per -tenant) — it is **designed but not built**. What *is* built and live: -role-based access control (below) and tenant-scoped control-plane data -(dashboards, below). Until `enterprise/internal/chrunner` and -`enterprise/internal/searchclient` exist and are wired into -`api/internal/queryapi.Handler` in place of the single shared connection -`api/cmd/api/main.go` opens today, **treat any deployment of this system -as single-tenant only**, regardless of how many `Tenant` CRs or -`tenant_memberships` rows exist. RBAC controls who can run a query; they -do not control what data that query can see. +**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." + +**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 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. + +**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. ## System overview @@ -38,12 +68,19 @@ do not control what data that query can see. Browser ──▶ web (SvelteKit, static) │ ▼ -Browser ──▶ api ──▶ ClickHouse (log data, SQL path) - │ └─▶ search (gRPC) ──▶ Tantivy (log data, full-text path) +Browser ──▶ api OR enterprise-api ──▶ ClickHouse (log data, SQL path) + │ └─▶ search (gRPC) ──▶ Tantivy (log data, full-text path) └─▶ Postgres (control plane: dashboards, alert_rules, tenants, users, tenant_memberships, audit_log) -alerting ──▶ api (POST /query, RoleService credential) +# api: one shared ClickHouse connection, 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. + +alerting ──▶ api or enterprise-api (POST /query, RoleService credential) alerting ──▶ Postgres (rulestore, notifystore) api/alerting ──▶ enterprise-auth (POST /internal/authorize, HTTP only — @@ -67,9 +104,9 @@ doesn't yet cover, not just an implementation gap. session issuance) is never imported by AGPL core (`/api`, `/alerting`, `/web`, `/cli`) — enforced in CI by `hack/check-tenant-boundary.sh`, which greps for the import edge on every build. Core calls -`enterprise-auth` over plain HTTP (`api/internal/authz.HTTPAuthorizer`), +`enterprise-auth` over plain HTTP (`api/authz.HTTPAuthorizer`), forwarding only the `Cookie`/`Authorization` headers, never the full -request (`api/internal/authz/httpauthz_test.go` asserts this — an +request (`api/authz/httpauthz_test.go` asserts this — an unrelated header like `X-Forwarded-For` is never forwarded). This means core's authorization decision is only as trustworthy as the network path to `enterprise-auth` — see "Deployment/network assumptions" below. @@ -95,9 +132,9 @@ a network-reachable endpoint) and configured via `API_SERVICE_TOKEN`. `enterprise/internal/session.Manager` issues and validates this token; `enterprise/internal/authhandler`'s `POST /internal/authorize` resolves it. `RoleService` is a distinct, non-comparable lane on the `Role` type -(`api/internal/authz.Role.Satisfies`) — a service credential can never +(`api/authz.Role.Satisfies`) — a service credential can never satisfy a human-role check and vice versa, verified by exhaustive -table-driven tests (`api/internal/authz/authz_test.go`). +table-driven tests (`api/authz/authz_test.go`). **Session/token integrity.** Tokens are HS256-signed JWTs with a single shared signing key (`ENTERPRISE_SESSION_SIGNING_KEY`, ≥32 bytes, @@ -115,10 +152,10 @@ mode — bad signature, malformed token, expired — into one **Live and enforced.** `POST /query` and every `/dashboards` endpoint in `api` require a minimum role, resolved per-request via -`api/internal/authz.RequireRole`/`RequireRoleOrService` calling +`api/authz.RequireRole`/`RequireRoleOrService` calling `enterprise-auth`. Roles: Viewer < Editor < Admin < Owner, plus the separate `RoleService` lane above. `GET /dashboards` is Viewer+; -create/update/delete require Editor+ (`api/internal/dashboards/ +create/update/delete require Editor+ (`api/dashboards/ handler.go`). A nil `Authorizer` (no `ENTERPRISE_AUTH_URL` configured) is a deliberate no-op, matching Phase 0-3's no-auth behavior — this is correct default-open-for-single-tenant behavior, not an oversight, but @@ -135,12 +172,12 @@ dashboard in that tenant, not just their own/granted ones. **Application-layer tenant scoping (dashboards only).** Every `dashboards` store query filters `WHERE tenant_id = $identity.TenantID` -(`api/internal/dashboards/store.go`), and the handler resolves that +(`api/dashboards/store.go`), and the handler resolves that tenant ID from the RBAC-authenticated identity's context (`authz.IdentityFromContext`), **never** from a client-supplied request field. This closes a real gap found during this document's own review: `Dashboard.TenantID` is a JSON-tagged, client-settable field -(`api/internal/dashboards/types.go`), and the original handler/store +(`api/dashboards/types.go`), and the original handler/store implementation trusted it directly on create/update and applied no `tenant_id` filter at all on list/get/update/delete — meaning any authenticated user could read, modify, or delete any other tenant's @@ -149,7 +186,7 @@ dashboards simply by supplying (or guessing) their UUID, or spoof to. Fixed as part of this task, with regression tests proving cross-tenant access now returns 404 (not 403, which would itself leak that the ID exists under a different tenant) — -`api/internal/dashboards/handler_test.go`'s +`api/dashboards/handler_test.go`'s `TestCrossTenant*`/`TestCreateDashboardIgnoresClientSuppliedTenantID`/ `TestImportIgnoresExportedTenantID`. **This same class of bug should be assumed present anywhere else client-supplied identifiers cross a tenant @@ -200,7 +237,7 @@ altered after the fact" claim actually holds against a privileged insider. **Fail-open by design for routine queries.** `queryapi.Handler.logAudit` -(`api/internal/queryapi/handler.go`) logs a write failure and otherwise +(`api/queryapi/handler.go`) logs a write failure and otherwise ignores it — an audit-log outage does not take down the query path. This is a deliberate availability-over-completeness tradeoff: it means a brief audit outage produces an under-logged (not over-blocked) window. @@ -234,15 +271,21 @@ terms: credentials. That's an operational control (credential custody, infrastructure access review), out of scope for this system's own code. -- **`system.query_log` metadata leakage** (task 2's finding): once - per-tenant ClickHouse users exist, `system.query_log` and related - `system.*` tables can expose other tenants' query *text* (predicate - values, field names) even if row-level isolation between databases - works perfectly. The design calls for revoking `system.*` access from - every tenant user explicitly, not relying on ClickHouse's default - template — this can only be verified once per-tenant users actually - exist (they don't yet; see the top of this document), so it remains - an open verification item, not a closed one. +- **`system.query_log` metadata leakage — per-tenant users are now + real, but the check itself hasn't run yet.** Was an open verification + item because there were no per-tenant ClickHouse users to check + against; that blocker is gone (`enterprise/internal/tenantprovision` + exists), and `tenantprovision_test.go`'s + `TestProvisionedUserCannotReadSystemTables` asserts exactly what the + design calls for (`system.query_log`/`system.tables` inaccessible, + `SHOW DATABASES` not revealing other tenants) — but this environment + never had ClickHouse access to actually run it, so it remains + unconfirmed against the pinned version + (`clickhouse/clickhouse-server:24.8`) until someone with Docker access + runs it (`/docs/phase-4-runbook.md` §8). Also still contingent on the + deployment-shape caveat at the top of this document: even once + confirmed, this only holds when `enterprise-api` (not plain `api`) is + actually serving traffic. - **No deny-override grants** — `dashboard_permissions` is additive-only by design; a full allow/deny ACL system is unbuilt, future work. - **No data retention/deletion policy** for a deprovisioned tenant — @@ -278,12 +321,16 @@ terms: |---|---| | Role-based access control on `/query`, `/dashboards` | **Enforced** | | `alerting`↔`api` service-identity credential | **Enforced** | -| Tenant scoping on dashboards (control-plane data) | **Enforced** (fixed this task) | -| Tenant isolation on log data (`/query` → ClickHouse/Tantivy) | **Not implemented** | +| Tenant scoping on dashboards (control-plane data) | **Enforced** | +| 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 | | Human SSO login (OIDC/SAML) | **Not implemented** | | Per-resource dashboard grants (`own/granted`) | **Not implemented** | -| Query audit logging (routine queries) | **Enforced**, fail-open | +| 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 | -| `system.*` ClickHouse metadata isolation | **Unverified** — depends on unbuilt per-tenant users | +| Mid-provisioning-race handling (evaluator ticks against a not-yet-active tenant) | **Unverified** — see `api/queryapi/tenant_isolation_gap_test.go` | | Protection against a privileged DB administrator | **Explicit non-goal** | diff --git a/enterprise/README.md b/enterprise/README.md index adad98a..29f18e9 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -5,20 +5,26 @@ boundary. SSO (OIDC/SAML), tenant provisioning, and RBAC. Nothing in `/agent`, `/ingest`, `/storage`, `/api`, `/web` core, or `/cli` imports from this module — confirmed by `hack/check-tenant-boundary.sh`, run in CI. `enterprise/` supplies tenant-scoped implementations of core's -already-shipped `api/internal/querylang/executor.SQLRunner`/ +already-shipped `api/querylang/executor.SQLRunner`/ `SearchClient` interfaces rather than core growing tenant awareness — see `/docs/phase-4-isolation-design.md` for why. ## Status -Tasks 3-5 (module skeleton, SSO library wiring, audit logging, and auth -wiring in `/api`/`/web`/`/cli`) are built and tested. What's live -end-to-end: +What's built and wired end-to-end. Verification status varies by +piece -- `internal/audit` was confirmed against a real Postgres earlier +in this phase's work; everything else below has real integration tests +written the same way (skipped unless a live database's connection +details are supplied via env var, same pattern throughout this package) +but they have **not actually been run against a live database in this +environment** -- see `/docs/phase-4-runbook.md`'s verification-status +section for exactly what "not yet run" means here and why. Don't read +"has a test for this" as "this was confirmed to work." - `internal/session` issues/validates signed (HS256/JWT) tokens for both human sessions and `/alerting`'s `RoleService` credential. - `internal/authhandler` serves `POST /internal/authorize` (the endpoint - `api/internal/authz.HTTPAuthorizer` calls) and `GET /auth/features` + `api/authz.HTTPAuthorizer` calls) and `GET /auth/features` (the runtime-capability check `/web`'s settings page reads). - `api`'s `/query` and `/dashboards` endpoints enforce RBAC via `authz.RequireRole`/`RequireRoleOrService`, nil-safe (no-op) when @@ -29,8 +35,30 @@ end-to-end: - `sentryctl` presents `$SENTRYCTL_TOKEN` as a Bearer credential on every request when set. - `internal/rbacstore`: full CRUD over `users`/`tenants`/ - `tenant_memberships` (`metadata/migrations/0017-0023`), verified - against a live Postgres. + `tenant_memberships`/`data_sources` (`metadata/migrations/0017-0032`). +- `internal/tenantprovision`: real `CREATE DATABASE`/`CREATE USER`/ + `GRANT` against ClickHouse. Its tests assert a tenant A user cannot + read tenant B's database by fully-qualified name, and that + `system.query_log`/`system.tables`/`SHOW DATABASES` don't leak across + tenants either (task 2's finding was that the latter is + version-dependent) -- not yet run against a live ClickHouse in this + environment, see the note above. +- `internal/chrunner`: the tenant-scoped `SQLRunner` -- a per-tenant + connection registry that resolves which tenant's ClickHouse connection + to use from the authenticated identity in request context, never a + parameter. Same adversarial probe, now through the actual production + code path (`chrunner.Registry.RunSQL`, not just tenantprovision's raw + grants). +- `internal/audit.QueryAPILogger`: the real `api/queryapi.AuditLogger` + implementation -- wired into `enterprise-api`, no longer `nil`. +- `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 + this shape exists (`enterprise → api` is the allowed import direction; + `api` can never import `enterprise/`). `-provision-tenant=` is the + operator action that provisions ClickHouse and marks a tenant active, + same "offline action, not a network endpoint" shape as + `enterprise-auth -mint-service-token`. **Deliberately deferred, not half-built** -- named explicitly rather than silently left out: @@ -39,37 +67,42 @@ silently left out: `internal/saml` do the protocol mechanics; nothing calls them from an HTTP handler yet). `-mint-service-token` is the only way to get a token today, and it only mints `RoleService` credentials. -- `dashboard_permissions`/`data_sources` CRUD (schema exists, - `metadata/migrations/0024-0026`; no caller reads per-resource grants +- `dashboard_permissions` CRUD (schema exists, + `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/tenantprovision` (ClickHouse DB/user/grant + Tantivy index - provisioning) and the tenant-scoped `internal/chrunner`/ - `internal/searchclient` `SQLRunner`/`SearchClient` implementations -- - task 2's isolation model, not yet built against real per-tenant - connections. -- Wiring `internal/audit` into `api`'s `queryapi.AuditLogger` extension - point (built in core since task 4, still passed as `nil`). +- `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 + `/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. ## Package layout ``` cmd/enterprise-auth/ config loading, OIDC discovery at startup, health/authorize/features endpoints, -mint-service-token +cmd/enterprise-api/ multi-tenant-aware alternative to api/cmd/api -- see its own doc comment internal/tenant/ the ID type -- see its package doc comment before touching it internal/oidc/ coreos/go-oidc wiring: discovery, login redirect, code exchange + ID token verification internal/saml/ crewjam/saml wiring: SP setup, login redirect, response parsing/validation internal/session/ issues/validates signed session + RoleService tokens internal/authhandler/ POST /internal/authorize, GET /auth/features -internal/rbacstore/ users/tenants/tenant_memberships CRUD (pgx against sentry_metadata) -internal/audit/ append-only, hash-chained query audit log -- see its own package - doc comment and /docs/phase-4-isolation-design.md's audit section -internal/config/ env-var config, same convention as every other Go service here +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/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/tenantprovision`, `internal/chrunner`/ -`internal/searchclient` (tenant-scoped `SQLRunner`/`SearchClient` -implementations), the OIDC/SAML login/callback HTTP handlers, and -`dashboard_permissions`/`data_sources` CRUD -- see "Status" above. +Future additions: `internal/searchclient`, the OIDC/SAML login/callback +HTTP handlers, `dashboard_permissions` CRUD, and real deployment-topology +wiring for `enterprise-api` -- see "Status" above. ## Why OIDC and SAML aren't hand-rolled @@ -113,6 +146,23 @@ docker run --rm --network sentry_default -v $(pwd)/..:/src -w /src/enterprise \ golang:1.25-alpine go test ./internal/rbacstore/... -v ``` +`internal/tenantprovision` and `internal/chrunner` need a real +ClickHouse instead (they mount the repo root, not just `enterprise/`, +since `internal/chrunner` imports `api/authz`/`api/querylang/executor` +via `go.mod`'s `replace` directives to `../api`): + +```sh +docker run --rm --network sentry_default -v $(pwd)/..:/src -w /src/enterprise \ + -e TENANTPROVISION_TEST_CLICKHOUSE_ADDR=clickhouse:9000 \ + -e TENANTPROVISION_TEST_CLICKHOUSE_PASSWORD=sentry-dev-only \ + golang:1.25-alpine go test ./internal/tenantprovision/... -v + +docker run --rm --network sentry_default -v $(pwd)/..:/src -w /src/enterprise \ + -e CHRUNNER_TEST_CLICKHOUSE_ADDR=clickhouse:9000 \ + -e CHRUNNER_TEST_CLICKHOUSE_PASSWORD=sentry-dev-only \ + golang:1.25-alpine go test ./internal/chrunner/... -v +``` + ## Turning on auth enforcement for manual testing Off by default (see "Status" above -- there's no login flow to issue a @@ -129,7 +179,25 @@ TOKEN=$(docker compose run --rm enterprise-auth -mint-service-token=alerting) docker build -f Dockerfile -t sentry-enterprise-auth . # context is enterprise/, not the repo root ``` -## Environment variables +## Provisioning a tenant and running `enterprise-api` + +```sh +docker compose build enterprise-api # context is the repo root, not enterprise/ -- see cmd/enterprise-api/Dockerfile +docker compose run --rm enterprise-api -provision-tenant=acme -display-name="Acme Corp" +docker compose up -d enterprise-api +curl -s http://localhost:8083/healthz +``` + +`-provision-tenant` creates the tenant/data_source rows in rbacstore if +they don't exist, provisions ClickHouse, persists the credentials, and +marks the tenant active -- refuses to run twice for the same tenant +(re-provisioning would either rotate a live credential or silently fail +to, see `tenantprovision.ProvisionClickHouse`'s doc comment). `web` +still points at plain `api` by default (`VITE_API_BASE_URL`) -- +pointing it at `enterprise-api` instead is a manual `docker-compose.yml` +edit today, not a supported flag. + +## Environment variables (`enterprise-auth`) | Var | Default | |---|---| @@ -146,3 +214,22 @@ docker build -f Dockerfile -t sentry-enterprise-auth . # context is enterprise | `SAML_ACS_URL` | (empty) | | `SAML_IDP_METADATA_URL` | (empty — presence only feeds `GET /auth/features`; not yet fetched/parsed) | | `ENTERPRISE_SESSION_SIGNING_KEY` | **required**, min 32 bytes | + +## Environment variables (`enterprise-api`) + +| Var | Default | +|---|---| +| `HTTP_LISTEN_ADDR` | `:8083` | +| `CLICKHOUSE_ADDR` | `localhost:9000` | +| `CLICKHOUSE_ADMIN_USERNAME` | `default` | +| `CLICKHOUSE_ADMIN_PASSWORD` | (empty) | +| `SEARCH_GRPC_ADDR` | `localhost:50052` | +| `POSTGRES_ADDR` | `localhost:5432` | +| `POSTGRES_DATABASE` | `sentry_metadata` | +| `POSTGRES_USERNAME` | `sentry` | +| `POSTGRES_PASSWORD` | (empty) | +| `AUDIT_WRITER_USERNAME` | `audit_writer` | +| `AUDIT_WRITER_PASSWORD` | (empty) | +| `ENTERPRISE_AUTH_URL` | (empty — RBAC becomes a no-op, but `chrunner.Registry.RunSQL` still refuses every query with no resolved tenant identity, so leaving this unset does not mean "open access," it means "every query fails") | +| `CORS_ALLOWED_ORIGIN` | `*` | +| `QUERY_TIMEOUT_SECONDS` | `30` | diff --git a/enterprise/cmd/enterprise-api/Dockerfile b/enterprise/cmd/enterprise-api/Dockerfile new file mode 100644 index 0000000..c4f58dc --- /dev/null +++ b/enterprise/cmd/enterprise-api/Dockerfile @@ -0,0 +1,13 @@ +# Same shape as every other Go service's Dockerfile in this repo -- +# context must be the repo root (needs both enterprise/ and proto/, like +# api/Dockerfile does for api/ + proto/), not enterprise/ alone. +# docker build -f enterprise/cmd/enterprise-api/Dockerfile -t sentry-enterprise-api . +FROM golang:1.25-alpine AS builder +WORKDIR /src +COPY . . +WORKDIR /src/enterprise +RUN CGO_ENABLED=0 GOOS=linux go build -o /out/enterprise-api ./cmd/enterprise-api + +FROM gcr.io/distroless/static-debian12 +COPY --from=builder /out/enterprise-api /enterprise-api +ENTRYPOINT ["/enterprise-api"] diff --git a/enterprise/cmd/enterprise-api/main.go b/enterprise/cmd/enterprise-api/main.go new file mode 100644 index 0000000..97336f2 --- /dev/null +++ b/enterprise/cmd/enterprise-api/main.go @@ -0,0 +1,278 @@ +// Command enterprise-api is the multi-tenant-aware alternative to +// 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. +// +// 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 +// this) -- there is no way for api's own binary to construct an +// enterprise-supplied chrunner.Registry or audit.Store without that +// import. enterprise/ importing api/ is the allowed direction, so this +// binary lives here instead, wiring core's handler types together with +// enterprise's tenant-aware implementations. A single-tenant deployment +// 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). +package main + +import ( + "context" + "flag" + "fmt" + "log/slog" + "net/http" + "os" + "os/signal" + "strings" + "syscall" + "time" + + chdriver "github.com/ClickHouse/clickhouse-go/v2" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/sentry/sentry/api/authz" + "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/tenantprovision" +) + +func main() { + logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + + cfg, err := apiconfig.Load() + if err != nil { + logger.Error("loading config", "error", err) + os.Exit(1) + } + + if len(os.Args) > 1 && os.Args[1] == "-healthcheck" { + os.Exit(runHealthcheck(cfg.HTTPListenAddr)) + } + + provisionTenant := flag.String("provision-tenant", "", "provision ClickHouse for the named tenant id (creating it in rbacstore if needed) and exit") + provisionDisplayName := flag.String("display-name", "", "display name for -provision-tenant, if the tenant doesn't already exist in rbacstore") + flag.Parse() + + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + pgDSN := fmt.Sprintf("postgres://%s:%s@%s/%s", cfg.Postgres.Username, cfg.Postgres.Password, cfg.Postgres.Addr, cfg.Postgres.Database) + pgPool, err := pgxpool.New(ctx, pgDSN) + if err != nil { + logger.Error("opening postgres pool", "error", err) + os.Exit(1) + } + defer pgPool.Close() + if err := pgPool.Ping(ctx); err != nil { + logger.Error("pinging postgres", "error", err) + os.Exit(1) + } + rbac := rbacstore.NewStore(pgPool) + + if *provisionTenant != "" { + os.Exit(runProvisionTenant(ctx, logger, cfg, rbac, *provisionTenant, *provisionDisplayName)) + } + + adminConn, err := chdriver.Open(&chdriver.Options{ + Addr: []string{cfg.ClickHouseAddr}, + Auth: chdriver.Auth{Database: "default", Username: cfg.ClickHouseAdmin.Username, Password: cfg.ClickHouseAdmin.Password}, + }) + if err != nil { + logger.Error("opening clickhouse admin connection", "error", err) + os.Exit(1) + } + defer adminConn.Close() + + sources, err := rbac.ListProvisionedDataSources(ctx) + if err != nil { + logger.Error("listing provisioned data sources", "error", err) + os.Exit(1) + } + chrunnerSources := make([]chrunner.DataSource, 0, len(sources)) + for _, s := range sources { + if s.ClickHouseUsername == nil || s.ClickHousePassword == nil { + continue // ListProvisionedDataSources already filters these out; defensive only. + } + chrunnerSources = append(chrunnerSources, chrunner.DataSource{ + TenantID: s.TenantID, Database: s.ClickHouseDatabaseName, + Username: *s.ClickHouseUsername, Password: *s.ClickHousePassword, + }) + } + logger.Info("loaded tenant data sources", "count", len(chrunnerSources)) + + registry, err := chrunner.New(ctx, cfg.ClickHouseAddr, chrunnerSources) + if err != nil { + logger.Error("building tenant connection registry", "error", err) + os.Exit(1) + } + defer registry.Close() + + search, err := searchclient.Dial(cfg.SearchGRPCAddr) + if err != nil { + logger.Error("dialing search service", "error", err) + os.Exit(1) + } + defer search.Close() + + var authorizer authz.Authorizer + if cfg.EnterpriseAuthURL != "" { + authorizer = authz.NewHTTPAuthorizer(cfg.EnterpriseAuthURL) + } else { + logger.Warn("ENTERPRISE_AUTH_URL is not set -- RBAC enforcement is a no-op, but tenant query routing still requires a resolved identity, so every /query request will be refused (see chrunner.Registry.RunSQL)") + } + + auditWriterDSN := fmt.Sprintf("postgres://%s:%s@%s/%s", cfg.AuditWriter.Username, cfg.AuditWriter.Password, cfg.Postgres.Addr, cfg.Postgres.Database) + auditPool, err := pgxpool.New(ctx, auditWriterDSN) + if err != nil { + logger.Error("opening audit_writer postgres pool", "error", err) + os.Exit(1) + } + defer auditPool.Close() + if err := auditPool.Ping(ctx); err != nil { + logger.Error("pinging audit_writer postgres pool", "error", err) + os.Exit(1) + } + auditLogger := audit.NewQueryAPILogger(audit.NewStore(auditPool), audit.SourceAPI) + + queryHandler := queryapi.NewHandler(logger, registry, search, cfg.QueryTimeout, auditLogger, authorizer) + dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool), authorizer) + + mux := http.NewServeMux() + queryHandler.RegisterRoutes(mux) + dashboardsHandler.RegisterRoutes(mux) + mux.HandleFunc("GET /healthz", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + + srv := &http.Server{ + Addr: cfg.HTTPListenAddr, + Handler: httpserver.WithCORS(mux, cfg.CORSAllowedOrigin), + } + + errCh := make(chan error, 1) + go func() { + logger.Info("enterprise-api listening", "addr", cfg.HTTPListenAddr) + errCh <- srv.ListenAndServe() + }() + + select { + case <-ctx.Done(): + shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := srv.Shutdown(shutdownCtx); err != nil { + logger.Error("graceful shutdown failed", "error", err) + } + case err := <-errCh: + if err != nil && err != http.ErrServerClosed { + logger.Error("server exited with error", "error", err) + os.Exit(1) + } + } +} + +// runProvisionTenant is the operator action that actually closes +// /docs/phase-4-isolation-design.md's ordered provisioning gate: ensure +// the tenant row exists, ensure a data_sources row exists, provision +// ClickHouse (CREATE USER -> GRANT), persist the returned credentials, +// and only then mark the tenant active. Same "offline operator action, +// not a network-reachable endpoint" shape as enterprise-auth's +// -mint-service-token. +func runProvisionTenant(ctx context.Context, logger *slog.Logger, cfg apiconfig.Config, rbac *rbacstore.Store, tenantID, displayName string) int { + adminConn, err := chdriver.Open(&chdriver.Options{ + Addr: []string{cfg.ClickHouseAddr}, + Auth: chdriver.Auth{Database: "default", Username: cfg.ClickHouseAdmin.Username, Password: cfg.ClickHouseAdmin.Password}, + }) + if err != nil { + logger.Error("opening clickhouse admin connection", "error", err) + return 1 + } + defer adminConn.Close() + + tenant, err := rbac.GetTenant(ctx, tenantID) + if err != nil { + if err != rbacstore.ErrNotFound { + logger.Error("getting tenant", "error", err) + return 1 + } + name := displayName + if name == "" { + name = tenantID + } + tenant, err = rbac.CreateTenant(ctx, tenantID, name) + if err != nil { + logger.Error("creating tenant", "error", err) + return 1 + } + logger.Info("created tenant row", "tenant_id", tenantID) + } + if tenant.Status == "active" { + logger.Error("tenant is already active -- refusing to re-provision (would rotate a live credential)", "tenant_id", tenantID) + return 1 + } + + dataSource, err := rbac.GetDataSourceForTenant(ctx, tenantID) + if err != nil { + if err != rbacstore.ErrNotFound { + logger.Error("getting data source", "error", err) + return 1 + } + dataSource, err = rbac.CreateDataSource(ctx, tenantID, "default", tenantID, "/var/lib/sentry-search/tenants/"+tenantID) + if err != nil { + logger.Error("creating data source", "error", err) + return 1 + } + } + if dataSource.ClickHouseUsername != nil { + logger.Error("data source already has ClickHouse credentials -- refusing to re-provision", "tenant_id", tenantID) + return 1 + } + + creds, err := tenantprovision.New(adminConn).ProvisionClickHouse(ctx, tenantID) + if err != nil { + logger.Error("provisioning clickhouse", "error", err) + return 1 + } + if err := rbac.SetDataSourceClickHouseCredentials(ctx, dataSource.ID, creds.Username, creds.Password); err != nil { + logger.Error("persisting clickhouse credentials", "error", err) + return 1 + } + if err := rbac.SetTenantStatus(ctx, tenantID, "active"); err != nil { + logger.Error("activating tenant", "error", err) + return 1 + } + + logger.Info("tenant provisioned and active", "tenant_id", tenantID, "clickhouse_database", tenantID, "clickhouse_username", creds.Username) + return 0 +} + +func runHealthcheck(listenAddr string) int { + addr := listenAddr + if strings.HasPrefix(addr, ":") { + addr = "localhost" + addr + } + client := http.Client{Timeout: 3 * time.Second} + resp, err := client.Get("http://" + addr + "/healthz") + if err != nil { + return 1 + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return 1 + } + return 0 +} diff --git a/enterprise/cmd/enterprise-auth/main.go b/enterprise/cmd/enterprise-auth/main.go index 3dcf17d..873a603 100644 --- a/enterprise/cmd/enterprise-auth/main.go +++ b/enterprise/cmd/enterprise-auth/main.go @@ -3,7 +3,7 @@ // /docs/phase-4-isolation-design.md and /docs/phase-4-rbac-design.md. // // Phase 4 task 5 adds session issuance/validation (internal/session) and -// the POST /internal/authorize endpoint api/internal/authz.HTTPAuthorizer +// the POST /internal/authorize endpoint api/authz.HTTPAuthorizer // calls -- the piece that actually turns on RBAC enforcement in /api. // Still deliberately missing: the OIDC/SAML login/callback HTTP handlers // that would issue a *human* session after a real IdP round trip, and diff --git a/enterprise/go.mod b/enterprise/go.mod index 032e70c..29654f2 100644 --- a/enterprise/go.mod +++ b/enterprise/go.mod @@ -2,7 +2,24 @@ module github.com/sentry/sentry/enterprise go 1.25.0 +// enterprise/ importing api/ (core) is the allowed direction of the +// module boundary hack/check-tenant-boundary.sh enforces -- see +// enterprise/internal/chrunner's doc comment: it implements api's +// executor.SQLRunner interface, which structurally requires importing +// the package that defines it. +replace github.com/sentry/sentry/api => ../api + +// api's own go.mod replace directive for proto/ is module-local and +// doesn't propagate here -- enterprise/ needs its own, or `go build` +// tries to fetch github.com/sentry/sentry/proto from a real (nonexistent) +// remote, since api/searchclient (now transitively imported) depends on +// the generated search gRPC stubs. +replace github.com/sentry/sentry/proto => ../proto + +require github.com/sentry/sentry/api v0.0.0-00010101000000-000000000000 + require ( + github.com/ClickHouse/clickhouse-go/v2 v2.48.0 github.com/coreos/go-oidc/v3 v3.20.0 github.com/crewjam/saml v0.5.1 github.com/go-jose/go-jose/v4 v4.1.4 @@ -12,14 +29,32 @@ require ( ) require ( + github.com/ClickHouse/ch-go v0.74.0 // indirect + github.com/andybalholm/brotli v1.2.2 // indirect github.com/beevik/etree v1.5.0 // indirect + github.com/cespare/xxhash/v2 v2.3.0 // indirect + github.com/go-faster/city v1.0.1 // indirect + github.com/go-faster/errors v0.7.1 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/jonboulle/clockwork v0.2.2 // indirect + github.com/klauspost/compress v1.19.1 // indirect github.com/mattermost/xml-roundtrip-validator v0.1.0 // indirect + github.com/paulmach/orb v0.13.0 // indirect + github.com/pierrec/lz4/v4 v4.1.27 // indirect github.com/russellhaering/goxmldsig v1.4.0 // indirect - golang.org/x/crypto v0.33.0 // indirect - golang.org/x/sync v0.17.0 // indirect - golang.org/x/text v0.29.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 + golang.org/x/crypto v0.54.0 // indirect + golang.org/x/net v0.57.0 // indirect + golang.org/x/sync v0.22.0 // indirect + 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/go.sum b/enterprise/go.sum index cfca1f6..811e1e5 100644 --- a/enterprise/go.sum +++ b/enterprise/go.sum @@ -1,6 +1,14 @@ +github.com/ClickHouse/ch-go v0.74.0 h1:uYs2m4wIt0ZHSM1E72rg0maCfzhR2V3xWb/vZEgpeWE= +github.com/ClickHouse/ch-go v0.74.0/go.mod h1:sZ/r+8ttZMjyrP9PuFbgoVbth1ywIu2LIQNA2vgko6M= +github.com/ClickHouse/clickhouse-go/v2 v2.48.0 h1:auzd4VkapQYhQF8F2Gog7s3x78Bi1JZmByxGbrw3C+4= +github.com/ClickHouse/clickhouse-go/v2 v2.48.0/go.mod h1:lBjUCPRG6RpRQdMbkXq+JV8rY0/O5lw+Z7jShgReFjM= +github.com/andybalholm/brotli v1.2.2 h1:HzTuoo2ErYQqf5qvcJInB8uvqSVxRttzkFexPWtnceM= +github.com/andybalholm/brotli v1.2.2/go.mod h1:rzTDkvFWvIrjDXZHkuS16NPggd91W3kUSvPlQ1pLaKY= github.com/beevik/etree v1.1.0/go.mod h1:r8Aw8JqVegEf0w2fDnATrX9VpkMcyFeM0FhwO62wh+A= github.com/beevik/etree v1.5.0 h1:iaQZFSDS+3kYZiGoc9uKeOkUY3nYMXOKLl6KIJxiJWs= github.com/beevik/etree v1.5.0/go.mod h1:gPNJNaBGVZ9AwsidazFZyygnd+0pAU38N4D+WemwKNs= +github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= +github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/coreos/go-oidc/v3 v3.20.0 h1:EtE0WIBHk03N+DqGkY4+UONzzZHk7amKt6IyNd7OsZE= github.com/coreos/go-oidc/v3 v3.20.0/go.mod h1:DYCf24+ncYi+XkIH97GY1+dqoRlbaSI26KVTCI9SrY4= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= @@ -9,12 +17,22 @@ github.com/crewjam/saml v0.5.1/go.mod h1:r0fDkmFe5URDgPrmtH0IYokva6fac3AUdstiPhy github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/go-faster/city v1.0.1 h1:4WAxSZ3V2Ws4QRDrscLEDcibJY8uf41H6AhXDrNDcGw= +github.com/go-faster/city v1.0.1/go.mod h1:jKcUJId49qdW3L1qKHH/3wPeUstCVpVSXTM6vO3VcTw= +github.com/go-faster/errors v0.7.1 h1:MkJTnDoEdi9pDabt1dpWf7AA8/BaSYZqibYyhZ20AYg= +github.com/go-faster/errors v0.7.1/go.mod h1:5ySTjWFiphBs07IKuiL69nxdfd5+fzh1u7FPGZP2quo= github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= +github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= +github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= +github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/golang-jwt/jwt/v4 v4.5.2 h1:YtQM7lnr8iZ+j5q71MGKkNw9Mn7AjHM68uc9g5fXeUI= github.com/golang-jwt/jwt/v4 v4.5.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0= -github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= -github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= +github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= @@ -27,6 +45,8 @@ github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= github.com/jonboulle/clockwork v0.2.2 h1:UOGuzwb1PwsrDAObMuhUnj0p5ULPj8V/xJ7Kx9qUBdQ= github.com/jonboulle/clockwork v0.2.2/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= +github.com/klauspost/compress v1.19.1 h1:VsB4HPswih7mmZ8WleSFQ75c/Ui1M4trX5oAsJnhSlk= +github.com/klauspost/compress v1.19.1/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= @@ -35,29 +55,64 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/mattermost/xml-roundtrip-validator v0.1.0 h1:RXbVD2UAl7A7nOTR4u7E3ILa4IbtvKBHw64LDsmu9hU= github.com/mattermost/xml-roundtrip-validator v0.1.0/go.mod h1:qccnGMcpgwcNaBnxqpJpWWUiPNr5H3O8eDgGV9gT5To= +github.com/paulmach/orb v0.13.0 h1:r7n7mQGGF+cj/CbcivEj9J3HGK+XR+yXnvzRdq9saIw= +github.com/paulmach/orb v0.13.0/go.mod h1:6scRWINywA2Jf05dcjOfLfxrUIMECvTSG2MVbRLxu/k= +github.com/pierrec/lz4/v4 v4.1.27 h1:+PhzhWDrjRj89TH2sw43nE3+4+W8lSxIuQadEHZyjUk= +github.com/pierrec/lz4/v4 v4.1.27/go.mod h1:EoQMVJgeeEOMsCqCzqFm2O0cJvljX2nGZjcRIPL34O4= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= -github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= +github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= github.com/rogpeppe/go-internal v1.8.0/go.mod h1:WmiCO8CzOY8rg0OYDC4/i/2WRWAB6poM+XZ2dLUbcbE= github.com/russellhaering/goxmldsig v1.4.0 h1:8UcDh/xGyQiyrW+Fq5t8f+l2DLB1+zlhYzkPUJ7Qhys= github.com/russellhaering/goxmldsig v1.4.0/go.mod h1:gM4MDENBQf7M+V824SGfyIUVFWydB7n0KkEubVJl+Tw= +github.com/segmentio/asm v1.2.1 h1:DTNbBqs57ioxAD4PrArqftgypG4/qNpXoJx8TVXxPR0= +github.com/segmentio/asm v1.2.1/go.mod h1:BqMnlJP91P8d+4ibuonYZw9mfnzI9HfxselHZr5aAcs= +github.com/shopspring/decimal v1.4.0 h1:bxl37RwXBklmTi0C79JfXCEBD1cqqHt0bbgBAGFp81k= +github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= -golang.org/x/crypto v0.33.0 h1:IOBPskki6Lysi0lo9qQvbxiQ+FvsCC/YWOecCHAixus= -golang.org/x/crypto v0.33.0/go.mod h1:bVdXmD7IV/4GdElGPozy6U7lWdRXA4qyRVGJV57uQ5M= +github.com/xyproto/randomstring v1.0.5 h1:YtlWPoRdgMu3NZtP45drfy1GKoojuR7hmRcnhZqKjWU= +github.com/xyproto/randomstring v1.0.5/go.mod h1:rgmS5DeNXLivK7YprL0pY+lTuhNQW3iGxZ18UQApw/E= +go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= +go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= +go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= +go.opentelemetry.io/otel v1.44.0/go.mod h1:BMgjTHL9WPRlRjL2oZCBTL4whCGtXch2H4BhOPIAyYc= +go.opentelemetry.io/otel/metric v1.44.0 h1:1w0gILTcHdr3YI+ixLyjemwrVnsMURbTZFrSYCdDdmc= +go.opentelemetry.io/otel/metric v1.44.0/go.mod h1:8O7hanEPBNgEMmybD3s2VBKcgWOCsA6tzHBPODAiquo= +go.opentelemetry.io/otel/sdk v1.44.0 h1:nHYwb9lK+fJPU/dnT6s7W7Z8itMWyqrnVfbheVYrZ58= +go.opentelemetry.io/otel/sdk v1.44.0/go.mod h1:Osuydd3Se74nqjAKxid74N5eC+jfEqfTegHRnq58oK0= +go.opentelemetry.io/otel/sdk/metric v1.44.0 h1:3LlKgI+VjbVsjNRFZJZAJ30WjXC5VkNRks6si09iEfI= +go.opentelemetry.io/otel/sdk/metric v1.44.0/go.mod h1:5B5pMARnXxKhltooO4xUuCBorl65a4EpnTalObqOigA= +go.opentelemetry.io/otel/trace v1.44.0 h1:jxF5CsGYCe74MCRx2X4g7WsY/VBKRqqpNvXlX/6gtIk= +go.opentelemetry.io/otel/trace v1.44.0/go.mod h1:oLl1jrMQAVo6v3GAggN+1VH9VIz9iUSvW53sW1Q8PIE= +golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= +golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= +golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= -golang.org/x/sync v0.17.0 h1:l60nONMj9l5drqw6jlhIELNv9I0A4OFgRsG9k2oT9Ug= -golang.org/x/sync v0.17.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= -golang.org/x/text v0.29.0 h1:1neNs90w9YzJ9BocxfsQNHKuAT4pkghyXc4nhZ6sJvk= -golang.org/x/text v0.29.0/go.mod h1:7MhJOA9CD2qZyOKYazxdYMF85OwPdEr9jTtBpO7ydH4= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= +golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= +gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= +gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa h1:mZHHdPZl0dbGHCflZgAq/Q468DWVFcU2whhB2KAo8fk= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= +google.golang.org/grpc v1.83.0 h1:JeNZEKJFbQxArAMl+hiytHauacDNqJUllNfmIMmpqnQ= +google.golang.org/grpc v1.83.0/go.mod h1:kDyl6SKsiHKt0uylY5gtn5cEjkrIOhQOGDgIc4JGwzQ= +google.golang.org/protobuf v1.36.12 h1:pJOKDDOyeXErUroCihFAd5LQuwXBSpVnKGrj5o/fwxc= +google.golang.org/protobuf v1.36.12/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= diff --git a/enterprise/internal/apiconfig/apiconfig.go b/enterprise/internal/apiconfig/apiconfig.go new file mode 100644 index 0000000..4c03495 --- /dev/null +++ b/enterprise/internal/apiconfig/apiconfig.go @@ -0,0 +1,103 @@ +// Package apiconfig loads enterprise-api's configuration from +// environment variables -- same convention as every other Go service in +// this repo. Named apiconfig, not config, to avoid colliding with the +// already-existing enterprise/internal/config (enterprise-auth's own, +// differently-shaped config) within the same module. +package apiconfig + +import ( + "fmt" + "os" + "strconv" + "time" +) + +type Config struct { + HTTPListenAddr string + // ClickHouseAddr is the shared physical ClickHouse server's native + // address -- every tenant's connection (chrunner.Registry) and the + // admin connection (tenantprovision) both dial this same address, + // just with different credentials. Tenants sharing one physical + // server is today's model; per-tenant dedicated cluster nodes is + // named as later, non-schema-changing work in + // /docs/phase-4-isolation-design.md. + ClickHouseAddr string + // ClickHouseAdmin is the access_management-enabled credential + // tenantprovision uses for CREATE DATABASE/USER/GRANT -- the same + // credential api's plain (non-enterprise) binary uses as its one + // shared connection today (docker-compose.yml's CLICKHOUSE_PASSWORD). + // Never used to run a tenant's actual queries. + ClickHouseAdmin ClickHouseAdminConfig + Postgres PostgresConfig + AuditWriter AuditWriterConfig + SearchGRPCAddr string + QueryTimeout time.Duration + CORSAllowedOrigin string + // EnterpriseAuthURL, like api's own config, is optional -- see that + // package's doc comment on the nil-authorizer no-op default. In + // practice a real enterprise-api deployment always sets this (there + // is no reason to run this binary instead of plain api without RBAC + // enforcement on), but nothing here hard-requires it, for the same + // "never break a simpler deployment shape" reasoning used + // throughout this codebase. + EnterpriseAuthURL string +} + +type ClickHouseAdminConfig struct { + Username string + Password string +} + +type PostgresConfig struct { + Addr string + Database string + Username string + Password string +} + +// AuditWriterConfig is the separate, narrowly-granted credential +// enterprise/internal/audit.Store requires -- see that package's doc +// comment on why it must never share api's/dashboards' pool. +type AuditWriterConfig struct { + Username string + Password string +} + +func Load() (Config, error) { + cfg := Config{ + HTTPListenAddr: getenv("HTTP_LISTEN_ADDR", ":8083"), + ClickHouseAddr: getenv("CLICKHOUSE_ADDR", "localhost:9000"), + ClickHouseAdmin: ClickHouseAdminConfig{ + Username: getenv("CLICKHOUSE_ADMIN_USERNAME", "default"), + Password: getenv("CLICKHOUSE_ADMIN_PASSWORD", ""), + }, + Postgres: PostgresConfig{ + Addr: getenv("POSTGRES_ADDR", "localhost:5432"), + Database: getenv("POSTGRES_DATABASE", "sentry_metadata"), + Username: getenv("POSTGRES_USERNAME", "sentry"), + Password: getenv("POSTGRES_PASSWORD", ""), + }, + AuditWriter: AuditWriterConfig{ + Username: getenv("AUDIT_WRITER_USERNAME", "audit_writer"), + Password: getenv("AUDIT_WRITER_PASSWORD", ""), + }, + SearchGRPCAddr: getenv("SEARCH_GRPC_ADDR", "localhost:50052"), + CORSAllowedOrigin: getenv("CORS_ALLOWED_ORIGIN", "*"), + EnterpriseAuthURL: getenv("ENTERPRISE_AUTH_URL", ""), + } + + timeoutSec, err := strconv.Atoi(getenv("QUERY_TIMEOUT_SECONDS", "30")) + if err != nil { + return Config{}, fmt.Errorf("QUERY_TIMEOUT_SECONDS: %w", err) + } + cfg.QueryTimeout = time.Duration(timeoutSec) * time.Second + + return cfg, nil +} + +func getenv(key, fallback string) string { + if v := os.Getenv(key); v != "" { + return v + } + return fallback +} diff --git a/enterprise/internal/audit/integration_test.go b/enterprise/internal/audit/integration_test.go index e8f13af..1af6d2a 100644 --- a/enterprise/internal/audit/integration_test.go +++ b/enterprise/internal/audit/integration_test.go @@ -18,8 +18,12 @@ import ( "path/filepath" "sync" "testing" + "time" "github.com/jackc/pgx/v5/pgxpool" + + "github.com/sentry/sentry/api/authz" + "github.com/sentry/sentry/api/queryapi" ) func testPool(t *testing.T, user, password string) *pgxpool.Pool { @@ -92,6 +96,50 @@ func TestAppendAndVerifyChainRealPostgres(t *testing.T) { } } +// TestQueryAPILoggerWritesAttributedToContextIdentity proves the +// adapter queryapi.Handler actually calls in production (via +// enterprise-api's wiring) reads tenant/user from context, not from any +// field on QueryAuditEntry -- matching that type's own doc comment. +func TestQueryAPILoggerWritesAttributedToContextIdentity(t *testing.T) { + writerPool := testPool(t, "audit_writer", os.Getenv("AUDIT_TEST_POSTGRES_PASSWORD")) + adminPool := testPool(t, "sentry", os.Getenv("AUDIT_TEST_ADMIN_PASSWORD")) + cleanupAuditLog(t, adminPool) + defer cleanupAuditLog(t, adminPool) + + logger := NewQueryAPILogger(NewStore(writerPool), SourceAPI) + ctx := authz.WithIdentity(context.Background(), authz.Identity{TenantID: "acme", UserID: "11111111-1111-1111-1111-111111111111", Role: authz.RoleViewer}) + + err := logger.LogQuery(ctx, queryapi.QueryAuditEntry{ + Query: "stats count", Language: "spl", RowCount: 3, Duration: 42 * time.Millisecond, Success: true, + }) + if err != nil { + t.Fatalf("LogQuery: %v", err) + } + + var tenantID, userID, queryText string + row := adminPool.QueryRow(context.Background(), + `SELECT tenant_id, user_id, query_text FROM audit_log ORDER BY id DESC LIMIT 1`) + if err := row.Scan(&tenantID, &userID, &queryText); err != nil { + t.Fatalf("reading back the written row: %v", err) + } + if tenantID != "acme" || userID != "11111111-1111-1111-1111-111111111111" || queryText != "stats count" { + t.Fatalf("got tenant_id=%q user_id=%q query_text=%q, want acme/11111111-.../\"stats count\"", tenantID, userID, queryText) + } +} + +func TestQueryAPILoggerRefusesWithoutIdentity(t *testing.T) { + writerPool := testPool(t, "audit_writer", os.Getenv("AUDIT_TEST_POSTGRES_PASSWORD")) + adminPool := testPool(t, "sentry", os.Getenv("AUDIT_TEST_ADMIN_PASSWORD")) + cleanupAuditLog(t, adminPool) + defer cleanupAuditLog(t, adminPool) + + logger := NewQueryAPILogger(NewStore(writerPool), SourceAPI) + err := logger.LogQuery(context.Background(), queryapi.QueryAuditEntry{Query: "stats count", Success: true}) + if err == nil { + t.Fatal("expected LogQuery to refuse writing an entry with no tenant identity in context") + } +} + // TestVerifyChainDetectsTampering proves the chain actually catches an // in-place row modification -- not just that VerifyChain runs without // erroring on untampered data, which a bug returning OK unconditionally diff --git a/enterprise/internal/audit/queryapi_adapter.go b/enterprise/internal/audit/queryapi_adapter.go new file mode 100644 index 0000000..d3b1e95 --- /dev/null +++ b/enterprise/internal/audit/queryapi_adapter.go @@ -0,0 +1,76 @@ +// Adapts *Store to api/queryapi.AuditLogger -- the interface core +// defines and has carried as a nil-by-default field +// (api/queryapi.Handler.audit) since Phase 4 task 4, waiting on exactly +// this: a real implementation, wired in by enterprise/cmd/enterprise-api +// (the one binary allowed to import both packages -- see chrunner's doc +// comment on the enterprise->api import direction). +package audit + +import ( + "context" + "fmt" + + "github.com/sentry/sentry/api/authz" + "github.com/sentry/sentry/api/queryapi" +) + +// QueryAPILogger implements queryapi.AuditLogger by translating its +// tenant-agnostic QueryAuditEntry into this package's Entry, reading +// tenant/user identity from ctx -- exactly the shape +// queryapi.AuditLogger's doc comment describes: "an enterprise-side +// implementation reads identity from ctx rather than this interface +// growing tenant-awareness." +type QueryAPILogger struct { + store *Store + source Source +} + +// NewQueryAPILogger wraps store for use as a specific Source -- api's +// queryapi.Handler and /alerting's evaluations go through different +// enterprise-api-fronted paths today (SourceAPI is the only one +// actually wired to a real HTTP handler; SourceAlerting is named for +// when alerting's queries get audited the same way, not yet built). +func NewQueryAPILogger(store *Store, source Source) *QueryAPILogger { + return &QueryAPILogger{store: store, source: source} +} + +func (l *QueryAPILogger) LogQuery(ctx context.Context, entry queryapi.QueryAuditEntry) error { + identity, ok := authz.IdentityFromContext(ctx) + if !ok || identity.TenantID == "" { + // Fail open at the queryapi.Handler call site already covers + // "don't take down the query path" -- this specific error tells + // that fail-open path *why* the write didn't happen, distinct + // from a real audit-storage failure, since chrunner.RunSQL + // would already have refused the query itself in this case (see + // that package's RunSQL) -- this branch mostly protects against + // a future caller that skips chrunner's own check. + return fmt.Errorf("audit: no tenant identity in context, refusing to write an unattributable audit entry") + } + + status := StatusSuccess + var errMsg *string + if !entry.Success { + status = StatusError + errMsg = &entry.Error + } + var userID *string + if identity.UserID != "" { + userID = &identity.UserID + } + queryText := entry.Query + rowCount := entry.RowCount + durationMS := int(entry.Duration.Milliseconds()) + + _, err := l.store.Append(ctx, Entry{ + TenantID: identity.TenantID, + UserID: userID, + Source: l.source, + EventType: EventQuery, + QueryText: &queryText, + RowCount: &rowCount, + DurationMS: &durationMS, + Status: status, + ErrorMessage: errMsg, + }) + return err +} diff --git a/enterprise/internal/authhandler/authhandler.go b/enterprise/internal/authhandler/authhandler.go index b5e08a1..6498fc9 100644 --- a/enterprise/internal/authhandler/authhandler.go +++ b/enterprise/internal/authhandler/authhandler.go @@ -1,6 +1,6 @@ // Package authhandler implements enterprise-auth's POST /internal/authorize // endpoint -- the HTTP side of the "network boundary, not import boundary" -// pattern api/internal/authz.HTTPAuthorizer calls into (see that package's +// pattern api/authz.HTTPAuthorizer calls into (see that package's // doc comment). It resolves a caller's credentials (session cookie or // service-token Bearer header) to an identity, using session.Manager for // both -- a human session and /alerting's service token are both just @@ -18,7 +18,7 @@ import ( "github.com/sentry/sentry/enterprise/internal/session" ) -// SessionCookieName matches the name api/internal/authz.HTTPAuthorizer's +// SessionCookieName matches the name api/authz.HTTPAuthorizer's // tests and doc comments already assume ("sentry_session"). const SessionCookieName = "sentry_session" diff --git a/enterprise/internal/chrunner/chrunner.go b/enterprise/internal/chrunner/chrunner.go new file mode 100644 index 0000000..c00e995 --- /dev/null +++ b/enterprise/internal/chrunner/chrunner.go @@ -0,0 +1,121 @@ +// Package chrunner is the tenant-scoped implementation of api's +// querylang/executor.SQLRunner interface -- the piece +// /docs/security/threat-model.md's headline finding says was missing: +// until this package, api/cmd/api/main.go opened exactly one shared +// ClickHouse connection for every tenant, no matter how many +// tenant_memberships/Tenant CRs existed. This package requires +// importing api/querylang/executor and api/authz directly (see +// enterprise/go.mod's replace directive) -- implementing +// executor.SQLRunner structurally requires it (its RunSQL method +// returns *executor.Result, a type only that package defines), and +// that's the allowed import direction: enterprise -> api, never the +// reverse (hack/check-tenant-boundary.sh enforces that direction only). +// +// Design, per /docs/phase-4-isolation-design.md's ClickHouse section: +// Registry holds one fully separate *executor.ChRunner (and the +// driver.Conn under it) per tenant, built once at construction from an +// immutable map -- never a shared pool with session-level `USE`, which +// is a classic concurrency bug (a connection recycled between tenants +// mid-flight can interleave one tenant's session state into another's +// query). RunSQL resolves which tenant's runner to use from the +// request's authz.Identity (attached to ctx by +// api/authz.RequireRole/RequireRoleOrService), never from any +// caller-suppliable parameter -- there is no code path in this package +// that accepts a tenant ID as an argument to a query-executing method. +package chrunner + +import ( + "context" + "fmt" + + "github.com/ClickHouse/clickhouse-go/v2" + "github.com/sentry/sentry/api/authz" + "github.com/sentry/sentry/api/querylang/executor" +) + +// DataSource is the minimal shape Registry needs to open one tenant's +// connection -- deliberately not enterprise/internal/rbacstore.DataSource +// itself, so this package doesn't need to import rbacstore just to +// describe "an address and a credential." Callers (enterprise-api's +// main.go) adapt rbacstore rows into this. +type DataSource struct { + TenantID string + Database string + Username string + Password string +} + +// Registry implements executor.SQLRunner by routing each call to the +// caller's tenant-specific connection. Immutable after New returns -- +// see this file's doc comment on why that's load-bearing, not just a +// style choice. +type Registry struct { + runners map[string]*executor.ChRunner + closers []func() +} + +// New opens one real ClickHouse connection per DataSource (same native +// address for all of them -- tenants sharing a physical ClickHouse +// server today, per-tenant *pinning* to dedicated cluster nodes is +// named as later, non-schema-changing work in +// /docs/phase-4-isolation-design.md, not something this constructor +// does). Fails closed: if any one tenant's connection can't be opened +// or doesn't ping successfully, the whole Registry fails to construct +// rather than silently running with a partial tenant set -- a tenant +// missing from the map is a clear, loud "unknown tenant" error at query +// time (see RunSQL), not a connection nobody noticed never came up. +func New(ctx context.Context, addr string, sources []DataSource) (*Registry, error) { + reg := &Registry{runners: make(map[string]*executor.ChRunner, len(sources))} + for _, src := range sources { + conn, err := clickhouse.Open(&clickhouse.Options{ + Addr: []string{addr}, + Auth: clickhouse.Auth{ + Database: src.Database, + Username: src.Username, + Password: src.Password, + }, + }) + if err != nil { + reg.Close() + return nil, fmt.Errorf("chrunner: opening connection for tenant %q: %w", src.TenantID, err) + } + if err := conn.Ping(ctx); err != nil { + _ = conn.Close() + reg.Close() + return nil, fmt.Errorf("chrunner: pinging connection for tenant %q: %w", src.TenantID, err) + } + reg.runners[src.TenantID] = executor.NewChRunner(conn) + reg.closers = append(reg.closers, func() { _ = conn.Close() }) + } + return reg, nil +} + +// Close releases every underlying connection -- call once at process +// shutdown, same lifecycle as the single conn.Close() api/cmd/api/main.go +// defers today, just fanned out over N connections. +func (r *Registry) Close() { + for _, c := range r.closers { + c() + } +} + +// RunSQL implements executor.SQLRunner. Resolves the caller's tenant +// from ctx (never a parameter -- see this file's doc comment) and fails +// closed on every ambiguous case: no identity, an identity with no +// tenant (RoleService, or a misconfigured authorizer), or a tenant with +// no provisioned connection all return an error, never a fallback to +// some other tenant's connection or an arbitrarily-chosen default. +func (r *Registry) RunSQL(ctx context.Context, sql string) (*executor.Result, error) { + identity, ok := authz.IdentityFromContext(ctx) + if !ok { + return nil, fmt.Errorf("chrunner: no authenticated identity in context, refusing to run query") + } + if identity.TenantID == "" { + return nil, fmt.Errorf("chrunner: authenticated identity %q has no tenant, refusing to run query", identity.Role) + } + runner, ok := r.runners[identity.TenantID] + if !ok { + return nil, fmt.Errorf("chrunner: tenant %q has no provisioned ClickHouse connection", identity.TenantID) + } + return runner.RunSQL(ctx, sql) +} diff --git a/enterprise/internal/chrunner/chrunner_test.go b/enterprise/internal/chrunner/chrunner_test.go new file mode 100644 index 0000000..9eda0a4 --- /dev/null +++ b/enterprise/internal/chrunner/chrunner_test.go @@ -0,0 +1,185 @@ +// Integration tests against a real ClickHouse -- exercises the actual +// question this package exists to answer: does a query authenticated as +// tenant A ever see tenant B's data. Uses enterprise/internal/ +// tenantprovision to set up real per-tenant users first (this is the +// adversarial probe api/queryapi/tenant_isolation_gap_test.go's +// TestAdversarial_ClickHouseUserCannotReadOtherTenantDatabaseByFullyQualifiedName +// names as blocked -- this is where it stops being blocked, at the +// chrunner/query-execution layer specifically, complementing +// tenantprovision's own version of the same probe at the raw-SQL-user +// layer). +// +// Skipped unless CHRUNNER_TEST_CLICKHOUSE_ADDR is set; run via: +// +// docker run --rm --network sentry_default -v $(pwd)/../../..:/src -w /src/enterprise \ +// -e CHRUNNER_TEST_CLICKHOUSE_ADDR=clickhouse:9000 \ +// -e CHRUNNER_TEST_CLICKHOUSE_PASSWORD=sentry-dev-only \ +// golang:1.25-alpine go test ./internal/chrunner/... -v +package chrunner + +import ( + "context" + "fmt" + "os" + "testing" + + chdriver "github.com/ClickHouse/clickhouse-go/v2" + "github.com/google/uuid" + "github.com/sentry/sentry/api/authz" + + "github.com/sentry/sentry/enterprise/internal/tenantprovision" +) + +func testAddr(t *testing.T) string { + t.Helper() + addr := os.Getenv("CHRUNNER_TEST_CLICKHOUSE_ADDR") + if addr == "" { + t.Skip("CHRUNNER_TEST_CLICKHOUSE_ADDR not set -- skipping live-ClickHouse integration test") + } + return addr +} + +func provisionTestTenant(t *testing.T, addr string) (tenantID string, creds tenantprovision.Credentials) { + t.Helper() + admin, err := chdriver.Open(&chdriver.Options{ + Addr: []string{addr}, + Auth: chdriver.Auth{Database: "default", Username: "default", Password: os.Getenv("CHRUNNER_TEST_CLICKHOUSE_PASSWORD")}, + }) + if err != nil { + t.Fatalf("opening admin connection: %v", err) + } + t.Cleanup(func() { admin.Close() }) + + tenantID = "cr" + uuid.NewString()[:8] + creds, err = tenantprovision.New(admin).ProvisionClickHouse(context.Background(), tenantID) + if err != nil { + t.Fatalf("provisioning tenant %s: %v", tenantID, err) + } + return tenantID, creds +} + +func TestRegistryRoutesQueryToCorrectTenant(t *testing.T) { + addr := testAddr(t) + ctx := context.Background() + tenantA, credsA := provisionTestTenant(t, addr) + + // Seed a distinguishing row directly as the tenant (SELECT-only + // grant means chrunner's own connection can't INSERT -- use a + // throwaway admin connection to seed data, matching how a real + // deployment's ingest path would write, not how api's read-only + // query path does). + admin, err := chdriver.Open(&chdriver.Options{ + Addr: []string{addr}, + Auth: chdriver.Auth{Database: "default", Username: "default", Password: os.Getenv("CHRUNNER_TEST_CLICKHOUSE_PASSWORD")}, + }) + if err != nil { + t.Fatalf("opening admin connection: %v", err) + } + defer admin.Close() + if err := admin.Exec(ctx, fmt.Sprintf("CREATE TABLE `%s`.marker (id UInt8) ENGINE = Memory", tenantA)); err != nil { + t.Fatalf("creating marker table: %v", err) + } + if err := admin.Exec(ctx, fmt.Sprintf("INSERT INTO `%s`.marker VALUES (42)", tenantA)); err != nil { + t.Fatalf("seeding marker row: %v", err) + } + + reg, err := New(ctx, addr, []DataSource{ + {TenantID: tenantA, Database: tenantA, Username: credsA.Username, Password: credsA.Password}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + defer reg.Close() + + reqCtx := authz.WithIdentity(ctx, authz.Identity{TenantID: tenantA, Role: authz.RoleViewer}) + result, err := reg.RunSQL(reqCtx, "SELECT id FROM marker") + if err != nil { + t.Fatalf("RunSQL: %v", err) + } + if len(result.Rows) != 1 || result.Rows[0][0] != uint8(42) { + t.Fatalf("unexpected result: %+v", result.Rows) + } +} + +func TestRegistryRefusesQueryWithNoTenantContext(t *testing.T) { + addr := testAddr(t) + ctx := context.Background() + tenantA, credsA := provisionTestTenant(t, addr) + + reg, err := New(ctx, addr, []DataSource{ + {TenantID: tenantA, Database: tenantA, Username: credsA.Username, Password: credsA.Password}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + defer reg.Close() + + if _, err := reg.RunSQL(ctx, "SELECT 1"); err == nil { + t.Fatal("expected RunSQL to refuse a request with no authenticated identity in context") + } +} + +func TestRegistryRefusesUnknownTenant(t *testing.T) { + addr := testAddr(t) + ctx := context.Background() + tenantA, credsA := provisionTestTenant(t, addr) + + reg, err := New(ctx, addr, []DataSource{ + {TenantID: tenantA, Database: tenantA, Username: credsA.Username, Password: credsA.Password}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + defer reg.Close() + + reqCtx := authz.WithIdentity(ctx, authz.Identity{TenantID: "some-other-tenant-never-provisioned", Role: authz.RoleViewer}) + if _, err := reg.RunSQL(reqCtx, "SELECT 1"); err == nil { + t.Fatal("expected RunSQL to refuse a tenant with no provisioned connection, not silently fall back") + } +} + +// TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL is the full +// end-to-end adversarial probe: two tenants, two connections inside one +// Registry, and a raw-SQL attempt (which the query language's escape +// hatch would pass straight through unmodified) to read the other +// tenant's data by fully-qualified name. This is what proves the +// connection-layer isolation model actually holds through chrunner, not +// just through tenantprovision's own grants (already covered by +// tenantprovision_test.go) -- this test exercises the exact code path +// api/queryapi.Handler calls in production. +func TestRegistryTenantCannotReadOtherTenantEvenViaRawSQL(t *testing.T) { + addr := testAddr(t) + ctx := context.Background() + tenantA, credsA := provisionTestTenant(t, addr) + tenantB, credsB := provisionTestTenant(t, addr) + + admin, err := chdriver.Open(&chdriver.Options{ + Addr: []string{addr}, + Auth: chdriver.Auth{Database: "default", Username: "default", Password: os.Getenv("CHRUNNER_TEST_CLICKHOUSE_PASSWORD")}, + }) + if err != nil { + t.Fatalf("opening admin connection: %v", err) + } + defer admin.Close() + if err := admin.Exec(ctx, fmt.Sprintf("CREATE TABLE `%s`.secret (id UInt8) ENGINE = Memory", tenantB)); err != nil { + t.Fatalf("creating secret table: %v", err) + } + if err := admin.Exec(ctx, fmt.Sprintf("INSERT INTO `%s`.secret VALUES (99)", tenantB)); err != nil { + t.Fatalf("seeding secret row: %v", err) + } + + reg, err := New(ctx, addr, []DataSource{ + {TenantID: tenantA, Database: tenantA, Username: credsA.Username, Password: credsA.Password}, + {TenantID: tenantB, Database: tenantB, Username: credsB.Username, Password: credsB.Password}, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + defer reg.Close() + + reqCtx := authz.WithIdentity(ctx, authz.Identity{TenantID: tenantA, Role: authz.RoleViewer}) + _, err = reg.RunSQL(reqCtx, fmt.Sprintf("SELECT * FROM `%s`.secret", tenantB)) + if err == nil { + t.Fatal("tenant A's request was able to read tenant B's database by fully-qualified name -- isolation is broken") + } +} diff --git a/enterprise/internal/rbacstore/rbacstore.go b/enterprise/internal/rbacstore/rbacstore.go index 46a853f..c090e23 100644 --- a/enterprise/internal/rbacstore/rbacstore.go +++ b/enterprise/internal/rbacstore/rbacstore.go @@ -14,10 +14,11 @@ // that handler itself isn't built yet (see cmd/enterprise-auth/main.go's // doc comment), so today rbacstore's only production caller is // -mint-service-token's future tenant-aware successor and its own tests. -// dashboard_permissions and data_sources (also part of the schema) don't -// have CRUD here yet -- no caller needs them until dashboards' handler -// wiring reads per-resource grants, named as deferred in task 5's -// summary. +// dashboard_permissions doesn't have CRUD here yet -- no caller reads +// per-resource grants (see api/dashboards/handler.go's doc +// comment). data_sources CRUD was added once enterprise/internal/ +// chrunner needed a real place to read per-tenant ClickHouse credentials +// from at startup (see that package's doc comment). package rbacstore import ( @@ -44,17 +45,17 @@ type User struct { } type Tenant struct { - ID string - DisplayName string - Status string - OwnerUserID string // empty until a first Owner is assigned - CreatedAt time.Time - UpdatedAt time.Time + ID string + DisplayName string + Status string + OwnerUserID string // empty until a first Owner is assigned + CreatedAt time.Time + UpdatedAt time.Time } -// Role mirrors api/internal/authz.Role's string values, kept as a plain +// Role mirrors api/authz.Role's string values, kept as a plain // string here rather than importing authz -- rbacstore is enterprise -// code and api/internal/authz is core; enterprise may depend on +// code and api/authz is core; enterprise may depend on // nothing-shaped-like-an-import-from-core per the module boundary // (see /docs/phase-4-isolation-design.md), even though the reverse // (core importing enterprise) is the one hack/check-tenant-boundary.sh @@ -252,3 +253,114 @@ func (s *Store) ListMembershipsForUser(ctx context.Context, userID string) ([]Me } return out, rows.Err() } + +// DataSource is one tenant's data-plane location -- today, exactly one +// ClickHouse database + one Tantivy index per tenant (see +// /docs/phase-4-rbac-design.md's "data_sources" extension-point +// section). ClickHouseUsername/Password are nil until +// enterprise/internal/tenantprovision actually provisions the +// ClickHouse-side user/database and calls SetDataSourceClickHouseCredentials. +type DataSource struct { + ID string + TenantID string + Name string + ClickHouseDatabaseName string + TantivyIndexPath string + ClickHouseUsername *string + ClickHousePassword *string +} + +// CreateDataSource inserts the row tenantprovision will later attach +// credentials to (SetDataSourceClickHouseCredentials) -- split into two +// steps because the row (database name, index path) is decided before +// provisioning runs, but the ClickHouse-side username/password only +// exist after CREATE USER actually succeeds. +func (s *Store) CreateDataSource(ctx context.Context, tenantID, name, clickHouseDatabaseName, tantivyIndexPath string) (*DataSource, error) { + ds := DataSource{ + ID: uuid.NewString(), TenantID: tenantID, Name: name, + ClickHouseDatabaseName: clickHouseDatabaseName, TantivyIndexPath: tantivyIndexPath, + } + _, err := s.pool.Exec(ctx, ` + INSERT INTO data_sources (id, tenant_id, name, clickhouse_database_name, tantivy_index_path) + VALUES ($1, $2, $3, $4, $5)`, + ds.ID, ds.TenantID, ds.Name, ds.ClickHouseDatabaseName, ds.TantivyIndexPath) + if err != nil { + return nil, fmt.Errorf("rbacstore: creating data source: %w", err) + } + return &ds, nil +} + +// SetDataSourceClickHouseCredentials is the only way +// clickhouse_username/password change -- called once, right after +// enterprise/internal/tenantprovision.ProvisionClickHouse succeeds. +// Never called again for the same data source: rotating a live tenant's +// credential without first updating it on the ClickHouse side would +// just break every open connection, same reasoning as +// deploy/operator/internal/controller/tenant_controller.go's +// reconcileSecret. +func (s *Store) SetDataSourceClickHouseCredentials(ctx context.Context, id, username, password string) error { + tag, err := s.pool.Exec(ctx, + `UPDATE data_sources SET clickhouse_username = $2, clickhouse_password = $3 WHERE id = $1`, + id, username, password) + if err != nil { + return fmt.Errorf("rbacstore: setting data source credentials: %w", err) + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} + +func scanDataSource(row pgx.Row) (*DataSource, error) { + var ds DataSource + if err := row.Scan(&ds.ID, &ds.TenantID, &ds.Name, &ds.ClickHouseDatabaseName, &ds.TantivyIndexPath, + &ds.ClickHouseUsername, &ds.ClickHousePassword); err != nil { + return nil, err + } + return &ds, nil +} + +func (s *Store) GetDataSourceForTenant(ctx context.Context, tenantID string) (*DataSource, error) { + row := s.pool.QueryRow(ctx, ` + SELECT id, tenant_id, name, clickhouse_database_name, tantivy_index_path, clickhouse_username, clickhouse_password + FROM data_sources WHERE tenant_id = $1 ORDER BY created_at LIMIT 1`, tenantID) + ds, err := scanDataSource(row) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrNotFound + } + return nil, fmt.Errorf("rbacstore: getting data source: %w", err) + } + return ds, nil +} + +// ListProvisionedDataSources returns every data source for an active +// tenant that has already been provisioned (ClickHouse credentials set) +// -- exactly the set enterprise/internal/chrunner.NewRegistry needs at +// startup. A data source with no credentials yet (tenantprovision hasn't +// run for it) is deliberately excluded rather than returned with empty +// credentials -- chrunner has nothing safe to connect with for it, and +// silently including it would turn into a confusing empty-string +// connection attempt instead of a clear "not provisioned yet" absence. +func (s *Store) ListProvisionedDataSources(ctx context.Context) ([]DataSource, error) { + rows, err := s.pool.Query(ctx, ` + SELECT ds.id, ds.tenant_id, ds.name, ds.clickhouse_database_name, ds.tantivy_index_path, + ds.clickhouse_username, ds.clickhouse_password + FROM data_sources ds + JOIN tenants t ON t.id = ds.tenant_id + WHERE t.status = 'active' AND ds.clickhouse_username IS NOT NULL AND ds.clickhouse_password IS NOT NULL`) + if err != nil { + return nil, fmt.Errorf("rbacstore: listing provisioned data sources: %w", err) + } + defer rows.Close() + + var out []DataSource + for rows.Next() { + ds, err := scanDataSource(rows) + if err != nil { + return nil, fmt.Errorf("rbacstore: scanning data source: %w", err) + } + out = append(out, *ds) + } + return out, rows.Err() +} diff --git a/enterprise/internal/rbacstore/rbacstore_test.go b/enterprise/internal/rbacstore/rbacstore_test.go index 3d8de17..a4d38a9 100644 --- a/enterprise/internal/rbacstore/rbacstore_test.go +++ b/enterprise/internal/rbacstore/rbacstore_test.go @@ -225,3 +225,124 @@ func TestListMembershipsForUserAcrossTenants(t *testing.T) { t.Fatalf("got %d memberships, want 2: %+v", len(memberships), memberships) } } + +func TestCreateDataSourceThenSetCredentials(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) + } + + ds, err := s.CreateDataSource(ctx, tenantID, "default", tenantID, "/var/lib/sentry-search/tenants/"+tenantID) + if err != nil { + t.Fatalf("CreateDataSource: %v", err) + } + if ds.ClickHouseUsername != nil || ds.ClickHousePassword != nil { + t.Fatalf("new data source must have no credentials yet, got %+v", ds) + } + + got, err := s.GetDataSourceForTenant(ctx, tenantID) + if err != nil { + t.Fatalf("GetDataSourceForTenant: %v", err) + } + if got.ID != ds.ID || got.ClickHouseUsername != nil { + t.Fatalf("unexpected data source: %+v", got) + } + + if err := s.SetDataSourceClickHouseCredentials(ctx, ds.ID, "tenant_"+tenantID, "secret-password"); err != nil { + t.Fatalf("SetDataSourceClickHouseCredentials: %v", err) + } + got, err = s.GetDataSourceForTenant(ctx, tenantID) + if err != nil { + t.Fatalf("GetDataSourceForTenant after credentials set: %v", err) + } + if got.ClickHouseUsername == nil || *got.ClickHouseUsername != "tenant_"+tenantID { + t.Fatalf("ClickHouseUsername = %v, want tenant_%s", got.ClickHouseUsername, tenantID) + } + if got.ClickHousePassword == nil || *got.ClickHousePassword != "secret-password" { + t.Fatalf("ClickHousePassword = %v, want secret-password", got.ClickHousePassword) + } +} + +func TestSetDataSourceClickHouseCredentialsNotFound(t *testing.T) { + s := testStore(t) + if err := s.SetDataSourceClickHouseCredentials(context.Background(), "does-not-exist-"+uniqueSuffix(), "u", "p"); err != ErrNotFound { + t.Fatalf("SetDataSourceClickHouseCredentials error = %v, want ErrNotFound", err) + } +} + +func TestGetDataSourceForTenantNotFound(t *testing.T) { + s := testStore(t) + if _, err := s.GetDataSourceForTenant(context.Background(), "does-not-exist-"+uniqueSuffix()); err != ErrNotFound { + t.Fatalf("GetDataSourceForTenant error = %v, want ErrNotFound", err) + } +} + +// TestListProvisionedDataSourcesExcludesUnprovisionedAndInactive proves +// the two filters ListProvisionedDataSources documents: a data source +// with no ClickHouse credentials yet is excluded (nothing safe to +// connect with), and a data source belonging to a non-active tenant is +// excluded too (a suspended/provisioning tenant must not show up in +// chrunner's connection registry). +func TestListProvisionedDataSourcesExcludesUnprovisionedAndInactive(t *testing.T) { + s := testStore(t) + ctx := context.Background() + + activeProvisioned := "test-tenant-" + uniqueSuffix() + activeUnprovisioned := "test-tenant-" + uniqueSuffix() + suspended := "test-tenant-" + uniqueSuffix() + + for _, id := range []string{activeProvisioned, activeUnprovisioned, suspended} { + if _, err := s.CreateTenant(ctx, id, id); err != nil { + t.Fatalf("CreateTenant %s: %v", id, err) + } + } + if err := s.SetTenantStatus(ctx, activeProvisioned, "active"); err != nil { + t.Fatalf("SetTenantStatus activeProvisioned: %v", err) + } + if err := s.SetTenantStatus(ctx, activeUnprovisioned, "active"); err != nil { + t.Fatalf("SetTenantStatus activeUnprovisioned: %v", err) + } + // suspended stays in 'provisioning' (CreateTenant's default) -- not active. + + dsProvisioned, err := s.CreateDataSource(ctx, activeProvisioned, "default", activeProvisioned, "/idx") + if err != nil { + t.Fatalf("CreateDataSource activeProvisioned: %v", err) + } + if err := s.SetDataSourceClickHouseCredentials(ctx, dsProvisioned.ID, "u", "p"); err != nil { + t.Fatalf("SetDataSourceClickHouseCredentials: %v", err) + } + + if _, err := s.CreateDataSource(ctx, activeUnprovisioned, "default", activeUnprovisioned, "/idx"); err != nil { + t.Fatalf("CreateDataSource activeUnprovisioned: %v", err) + } + + dsSuspended, err := s.CreateDataSource(ctx, suspended, "default", suspended, "/idx") + if err != nil { + t.Fatalf("CreateDataSource suspended: %v", err) + } + if err := s.SetDataSourceClickHouseCredentials(ctx, dsSuspended.ID, "u", "p"); err != nil { + t.Fatalf("SetDataSourceClickHouseCredentials suspended: %v", err) + } + + list, err := s.ListProvisionedDataSources(ctx) + if err != nil { + t.Fatalf("ListProvisionedDataSources: %v", err) + } + foundOurs := false + for _, ds := range list { + if ds.TenantID == activeUnprovisioned { + t.Fatalf("unprovisioned data source leaked into the list: %+v", ds) + } + if ds.TenantID == suspended { + t.Fatalf("non-active tenant's data source leaked into the list: %+v", ds) + } + if ds.TenantID == activeProvisioned { + foundOurs = true + } + } + if !foundOurs { + t.Fatal("expected the active, provisioned data source to be in the list") + } +} diff --git a/enterprise/internal/session/session.go b/enterprise/internal/session/session.go index 6057e23..5e95215 100644 --- a/enterprise/internal/session/session.go +++ b/enterprise/internal/session/session.go @@ -20,7 +20,7 @@ import ( "github.com/go-jose/go-jose/v4/jwt" ) -// Claims mirrors api/internal/authz.Identity's fields (TenantID, UserID, +// Claims mirrors api/authz.Identity's fields (TenantID, UserID, // Role as a string) plus the standard registered JWT claims. Role is // deliberately a plain string, not enterprise's own type, since its only // consumer -- authz.Role -- is defined in core and this package must not diff --git a/enterprise/internal/tenantprovision/tenantprovision.go b/enterprise/internal/tenantprovision/tenantprovision.go new file mode 100644 index 0000000..e60dc05 --- /dev/null +++ b/enterprise/internal/tenantprovision/tenantprovision.go @@ -0,0 +1,143 @@ +// Package tenantprovision does the ClickHouse-side half of tenant +// provisioning /docs/phase-4-isolation-design.md describes: one +// dedicated database + one narrowly-granted user per tenant, ordered +// and idempotent (CREATE DATABASE -> CREATE USER -> GRANT). This is the +// piece that was missing before -- deploy/operator's Tenant controller +// only manages the K8s-side credential Secret; nothing called +// ClickHouse's DDL to make that Secret's credentials actually work +// until this package. +// +// What this package does NOT do: Tantivy index provisioning (still +// unbuilt -- see /docs/security/threat-model.md), and it does not +// itself decide when a tenant becomes 'active' in rbacstore.tenants -- +// the caller (enterprise-api's -provision-tenant flag) does that only +// after ProvisionClickHouse returns success, matching the ordered gate +// /docs/phase-4-isolation-design.md specifies: CREATE USER -> GRANT -> +// only then mark active. +package tenantprovision + +import ( + "context" + "crypto/rand" + "encoding/base64" + "fmt" + "regexp" + + "github.com/ClickHouse/clickhouse-go/v2/lib/driver" +) + +// tenantIdentifierPattern is deliberately strict: tenant IDs become +// literal ClickHouse database/user names, interpolated directly into +// DDL statements below (ClickHouse's driver has no parameterized-query +// support for identifiers, only values -- this is the real reason +// rbacstore.Tenant.ID and every K8s Tenant CRD name are constrained to +// look like a DNS-safe slug already; this regexp is the enforcement +// point specific to this package's SQL construction, not a general +// tenant-ID validator). +var tenantIdentifierPattern = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,62}$`) + +// Credentials is what ProvisionClickHouse hands back for the caller to +// persist (rbacstore.Store.SetDataSourceClickHouseCredentials) -- +// Password is returned exactly once; ClickHouse itself doesn't store it +// recoverably, so losing this return value means re-provisioning +// (dropping and recreating the user) is the only recovery path. +type Credentials struct { + Username string + Password string +} + +// Provisioner wraps an admin ClickHouse connection -- one with +// access_management enabled, the same credential docker-compose.yml's +// CLICKHOUSE_PASSWORD/the Helm chart's clickhouse Secret already is. +// Never the per-tenant connections enterprise/internal/chrunner opens. +type Provisioner struct { + admin driver.Conn +} + +func New(admin driver.Conn) *Provisioner { + return &Provisioner{admin: admin} +} + +// ProvisionClickHouse creates tenantID's database and a fresh user for +// it, granted SELECT on exactly that database and nothing else. +// Database creation is idempotent (CREATE DATABASE IF NOT EXISTS -- +// harmless to repeat). User creation deliberately is NOT idempotent +// (plain CREATE USER, no IF NOT EXISTS): ClickHouse has no way to read +// back an existing user's password, so silently succeeding on a second +// call would either mean returning stale/wrong credentials or silently +// rotating a live tenant's password out from under it -- the same +// "never rotate a live credential without coordinating the consumer +// side" reasoning as deploy/operator/internal/controller/ +// tenant_controller.go's reconcileSecret. A second call for an +// already-provisioned tenant fails loudly instead, which is the correct +// outcome: the caller (enterprise-api's -provision-tenant flag) must +// check rbacstore for existing credentials before ever calling this, +// not rely on this function to be safely re-callable. +// +// system.* access: intentionally not explicitly granted anywhere here, +// relying on ClickHouse RBAC's default-deny for a freshly created user +// once access_management is enabled on the admin connection (required +// for CREATE USER/GRANT to work at all). This is exactly the assumption +// /docs/phase-4-isolation-design.md's task 2 finding says must be +// verified live per ClickHouse version, not trusted from documentation +// -- see /docs/security/threat-model.md's "system.query_log metadata +// leakage" section; that verification has not happened yet. +func (p *Provisioner) ProvisionClickHouse(ctx context.Context, tenantID string) (Credentials, error) { + if !tenantIdentifierPattern.MatchString(tenantID) { + return Credentials{}, fmt.Errorf("tenantprovision: tenant id %q is not a safe ClickHouse identifier", tenantID) + } + + database := tenantID + username := "tenant_" + tenantID + + if err := p.admin.Exec(ctx, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS `%s`", database)); err != nil { + return Credentials{}, fmt.Errorf("tenantprovision: creating database: %w", err) + } + + password, err := generatePassword() + if err != nil { + return Credentials{}, err + } + + // No IF NOT EXISTS -- see this function's doc comment for why a + // second call must fail, not silently succeed. + if err := p.admin.Exec(ctx, fmt.Sprintf( + "CREATE USER `%s` IDENTIFIED WITH plaintext_password BY '%s'", + username, escapeSingleQuotes(password), + )); err != nil { + return Credentials{}, fmt.Errorf("tenantprovision: creating user (already provisioned? this call is not safe to retry): %w", err) + } + + if err := p.admin.Exec(ctx, fmt.Sprintf("GRANT SELECT ON `%s`.* TO `%s`", database, username)); err != nil { + return Credentials{}, fmt.Errorf("tenantprovision: granting select: %w", err) + } + + return Credentials{Username: username, Password: password}, nil +} + +func generatePassword() (string, error) { + buf := make([]byte, 24) + if _, err := rand.Read(buf); err != nil { + return "", fmt.Errorf("tenantprovision: generating password: %w", err) + } + return base64.RawURLEncoding.EncodeToString(buf), nil +} + +// escapeSingleQuotes guards against a generated password (base64 +// RawURLEncoding, so alphanumeric plus '-'/'_' only, never a literal +// quote) accidentally breaking out of the SQL string literal -- belt +// and suspenders given generatePassword's actual alphabet can't produce +// one, since this function's output gets interpolated directly into DDL +// (see tenantIdentifierPattern's doc comment on why: ClickHouse's driver +// has no parameterized identifiers/literals for DDL). +func escapeSingleQuotes(s string) string { + out := make([]byte, 0, len(s)) + for i := 0; i < len(s); i++ { + if s[i] == '\'' { + out = append(out, '\'', '\'') + continue + } + out = append(out, s[i]) + } + return string(out) +} diff --git a/enterprise/internal/tenantprovision/tenantprovision_test.go b/enterprise/internal/tenantprovision/tenantprovision_test.go new file mode 100644 index 0000000..6995f30 --- /dev/null +++ b/enterprise/internal/tenantprovision/tenantprovision_test.go @@ -0,0 +1,213 @@ +// Integration tests against a real ClickHouse -- this package's whole +// job is DDL side effects (CREATE DATABASE/USER, GRANT), which a mock +// driver.Conn can't meaningfully verify. Skipped unless +// TENANTPROVISION_TEST_CLICKHOUSE_ADDR is set; run via: +// +// docker run --rm --network sentry_default -v $(pwd)/../../..:/src -w /src/enterprise \ +// -e TENANTPROVISION_TEST_CLICKHOUSE_ADDR=clickhouse:9000 \ +// -e TENANTPROVISION_TEST_CLICKHOUSE_PASSWORD=sentry-dev-only \ +// golang:1.25-alpine go test ./internal/tenantprovision/... -v +package tenantprovision + +import ( + "context" + "fmt" + "os" + "testing" + + "github.com/ClickHouse/clickhouse-go/v2" + "github.com/ClickHouse/clickhouse-go/v2/lib/driver" + "github.com/google/uuid" +) + +func testAdminConn(t *testing.T) driver.Conn { + t.Helper() + addr := os.Getenv("TENANTPROVISION_TEST_CLICKHOUSE_ADDR") + if addr == "" { + t.Skip("TENANTPROVISION_TEST_CLICKHOUSE_ADDR not set -- skipping live-ClickHouse integration test") + } + conn, err := clickhouse.Open(&clickhouse.Options{ + Addr: []string{addr}, + Auth: clickhouse.Auth{ + Database: "default", + Username: "default", + Password: os.Getenv("TENANTPROVISION_TEST_CLICKHOUSE_PASSWORD"), + }, + }) + if err != nil { + t.Fatalf("opening admin connection: %v", err) + } + t.Cleanup(func() { conn.Close() }) + if err := conn.Ping(context.Background()); err != nil { + t.Fatalf("pinging clickhouse: %v", err) + } + return conn +} + +func testTenantID() string { + return "tp" + uuid.NewString()[:8] +} + +func TestProvisionClickHouseCreatesUsableTenantConnection(t *testing.T) { + admin := testAdminConn(t) + p := New(admin) + tenantID := testTenantID() + ctx := context.Background() + + creds, err := p.ProvisionClickHouse(ctx, tenantID) + if err != nil { + t.Fatalf("ProvisionClickHouse: %v", err) + } + if creds.Username != "tenant_"+tenantID || creds.Password == "" { + t.Fatalf("unexpected credentials: %+v", creds) + } + + // Prove the credential actually works: connect as the tenant user + // and run a real query against its own database. + tenantConn, err := clickhouse.Open(&clickhouse.Options{ + Addr: []string{os.Getenv("TENANTPROVISION_TEST_CLICKHOUSE_ADDR")}, + Auth: clickhouse.Auth{Database: tenantID, Username: creds.Username, Password: creds.Password}, + }) + if err != nil { + t.Fatalf("opening tenant connection: %v", err) + } + defer tenantConn.Close() + if err := tenantConn.Ping(ctx); err != nil { + t.Fatalf("pinging as the provisioned tenant user: %v", err) + } + if err := tenantConn.Exec(ctx, "SELECT 1"); err != nil { + t.Fatalf("running SELECT as the provisioned tenant user: %v", err) + } +} + +// TestProvisionedUserCannotReadOtherTenantDatabase is one of the four +// adversarial probes /docs/phase-4-isolation-design.md's verification +// plan names for Phase 4 task 8 (see api/queryapi/ +// tenant_isolation_gap_test.go, which stubs this exact scenario as +// blocked pending tenantprovision existing) -- now that tenantprovision +// exists, this is the first one that can actually run for real. +func TestProvisionedUserCannotReadOtherTenantDatabase(t *testing.T) { + admin := testAdminConn(t) + p := New(admin) + ctx := context.Background() + + tenantA := testTenantID() + tenantB := testTenantID() + credsA, err := p.ProvisionClickHouse(ctx, tenantA) + if err != nil { + t.Fatalf("provisioning tenant A: %v", err) + } + if _, err := p.ProvisionClickHouse(ctx, tenantB); err != nil { + t.Fatalf("provisioning tenant B: %v", err) + } + + // Seed a row in tenant B's database as admin. + if err := admin.Exec(ctx, fmt.Sprintf("CREATE TABLE `%s`.secret (id UInt8) ENGINE = Memory", tenantB)); err != nil { + t.Fatalf("creating table in tenant B's database: %v", err) + } + if err := admin.Exec(ctx, fmt.Sprintf("INSERT INTO `%s`.secret VALUES (1)", tenantB)); err != nil { + t.Fatalf("inserting into tenant B's database: %v", err) + } + + tenantAConn, err := clickhouse.Open(&clickhouse.Options{ + Addr: []string{os.Getenv("TENANTPROVISION_TEST_CLICKHOUSE_ADDR")}, + Auth: clickhouse.Auth{Database: tenantA, Username: credsA.Username, Password: credsA.Password}, + }) + if err != nil { + t.Fatalf("opening tenant A connection: %v", err) + } + defer tenantAConn.Close() + + // The core adversarial probe: tenant A's user attempting to read + // tenant B's database by fully-qualified name in raw SQL. + err = tenantAConn.Exec(ctx, fmt.Sprintf("SELECT * FROM `%s`.secret", tenantB)) + if err == nil { + t.Fatal("tenant A's user was able to read tenant B's database -- isolation is broken") + } +} + +// TestProvisionedUserCannotReadSystemTables is item 2 of +// /docs/phase-4-isolation-design.md's verification plan (see +// api/queryapi/tenant_isolation_gap_test.go for the other three items' +// status) -- task 2's finding was that system.* visibility for a +// non-admin ClickHouse user is version-dependent and must be checked +// live, not assumed from documentation. ProvisionClickHouse never +// explicitly grants system.* access to anything (see its doc comment); +// this test is what actually confirms that omission is sufficient on +// the ClickHouse version this repo pins (docker-compose.yml: +// clickhouse/clickhouse-server:24.8), rather than trusting the omission +// alone. +func TestProvisionedUserCannotReadSystemTables(t *testing.T) { + admin := testAdminConn(t) + p := New(admin) + tenantID := testTenantID() + creds, err := p.ProvisionClickHouse(context.Background(), tenantID) + if err != nil { + t.Fatalf("ProvisionClickHouse: %v", err) + } + + tenantConn, err := clickhouse.Open(&clickhouse.Options{ + Addr: []string{os.Getenv("TENANTPROVISION_TEST_CLICKHOUSE_ADDR")}, + Auth: clickhouse.Auth{Database: tenantID, Username: creds.Username, Password: creds.Password}, + }) + if err != nil { + t.Fatalf("opening tenant connection: %v", err) + } + defer tenantConn.Close() + + // system.query_log/system.tables: expect a hard access-denied error, + // not a filtered/empty result -- these tables contain other + // tenants' query text and schema, so "succeeds but happens to + // return nothing for this user" would still be a version-dependent + // assumption worth catching, not something this test treats as a pass. + for _, probe := range []string{ + "SELECT * FROM system.query_log LIMIT 1", + "SELECT * FROM system.tables LIMIT 1", + } { + if err := tenantConn.Exec(context.Background(), probe); err == nil { + t.Errorf("tenant user was able to run %q -- system.* access was not actually revoked on this ClickHouse version", probe) + } + } + + // SHOW DATABASES is checked differently: some ClickHouse versions + // filter this to only databases the user can see rather than + // erroring outright, which is an acceptable outcome for this + // specific statement (unlike query_log/tables above) as long as it + // doesn't reveal other tenants' database names. + rows, err := tenantConn.Query(context.Background(), "SHOW DATABASES") + if err != nil { + return // erroring outright is also an acceptable outcome here. + } + defer rows.Close() + for rows.Next() { + var db string + if err := rows.Scan(&db); err != nil { + t.Fatalf("scanning SHOW DATABASES row: %v", err) + } + if db != tenantID && db != "default" && db != "system" && db != "INFORMATION_SCHEMA" && db != "information_schema" { + t.Errorf("SHOW DATABASES revealed a database this tenant user shouldn't see: %q", db) + } + } +} + +func TestProvisionClickHouseRejectsUnsafeTenantID(t *testing.T) { + admin := testAdminConn(t) + p := New(admin) + if _, err := p.ProvisionClickHouse(context.Background(), "not safe; DROP TABLE x"); err == nil { + t.Fatal("expected ProvisionClickHouse to reject an unsafe tenant identifier") + } +} + +func TestProvisionClickHouseSecondCallForSameTenantFails(t *testing.T) { + admin := testAdminConn(t) + p := New(admin) + tenantID := testTenantID() + ctx := context.Background() + + if _, err := p.ProvisionClickHouse(ctx, tenantID); err != nil { + t.Fatalf("first ProvisionClickHouse: %v", err) + } + if _, err := p.ProvisionClickHouse(ctx, tenantID); err == nil { + t.Fatal("expected a second ProvisionClickHouse call for the same tenant to fail -- see the function's doc comment on why re-provisioning must not silently succeed") + } +} diff --git a/metadata/migrations/0032_add_data_sources_clickhouse_credentials.sql b/metadata/migrations/0032_add_data_sources_clickhouse_credentials.sql new file mode 100644 index 0000000..09bfc76 --- /dev/null +++ b/metadata/migrations/0032_add_data_sources_clickhouse_credentials.sql @@ -0,0 +1,12 @@ +-- Per-tenant ClickHouse credentials, generated by +-- enterprise/internal/tenantprovision and consumed by +-- enterprise/internal/chrunner's per-tenant connection registry at +-- enterprise-api startup. Stored in the same place every other +-- control-plane secret in this schema lives (audit_writer's password is +-- an env var, not a DB row, since it's one shared credential -- these +-- are per-tenant and need to be looked up by tenant, hence a table). +-- Same trust model as CLICKHOUSE_PASSWORD already being plaintext in +-- docker-compose.yml/the Helm chart's Secret -- Postgres itself is +-- already trusted infrastructure in this design, not a new exposure. +ALTER TABLE data_sources ADD COLUMN IF NOT EXISTS clickhouse_username TEXT; +ALTER TABLE data_sources ADD COLUMN IF NOT EXISTS clickhouse_password TEXT;