diff --git a/enterprise/internal/tenantprovision/tenantprovision.go b/enterprise/internal/tenantprovision/tenantprovision.go index f718b86..d6241f1 100644 --- a/enterprise/internal/tenantprovision/tenantprovision.go +++ b/enterprise/internal/tenantprovision/tenantprovision.go @@ -82,14 +82,15 @@ func New(admin driver.Conn) *Provisioner { // 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. +// system.* access: explicitly revoked below, not left to ClickHouse +// RBAC's default. Verified live against ClickHouse 24.8 (the version +// docker-compose.yml pins): a freshly created user is NOT default-denied +// from system.* the way this package originally assumed -- +// system.tables lists every tenant's database/table names to any +// authenticated user regardless of per-database grants unless this +// REVOKE runs. See /docs/phase-4-isolation-design.md's task 2 item 2 and +// /docs/security/threat-model.md's "system.query_log metadata leakage" +// section. 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) @@ -120,6 +121,18 @@ func (p *Provisioner) ProvisionClickHouse(ctx context.Context, tenantID string) return Credentials{}, fmt.Errorf("tenantprovision: granting select/insert: %w", err) } + // A freshly created ClickHouse 24.8 user is NOT default-denied from + // system.* the way this package's original design assumed -- verified + // live (TestProvisionedUserCannotReadSystemTables failed against a + // real container before this REVOKE existed): system.tables lists + // every tenant's database/table names regardless of grants unless + // explicitly revoked. This closes /docs/phase-4-isolation-design.md's + // task 2 item 2 and /docs/security/threat-model.md's "system.query_log + // metadata leakage" finding for real, not just in the design doc. + if err := p.admin.Exec(ctx, fmt.Sprintf("REVOKE SELECT ON system.* FROM `%s`", username)); err != nil { + return Credentials{}, fmt.Errorf("tenantprovision: revoking system.* access: %w", err) + } + return Credentials{Username: username, Password: password}, nil } diff --git a/enterprise/internal/tenantprovision/tenantprovision_test.go b/enterprise/internal/tenantprovision/tenantprovision_test.go index 42fd736..393e903 100644 --- a/enterprise/internal/tenantprovision/tenantprovision_test.go +++ b/enterprise/internal/tenantprovision/tenantprovision_test.go @@ -190,26 +190,47 @@ func TestProvisionedUserCannotReadSystemTables(t *testing.T) { } 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) - } + // system.query_log: expect a hard access-denied error, not a + // filtered/empty result -- it contains other tenants' query text, + // 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. Verified live against + // ClickHouse 24.8: REVOKE SELECT ON system.* does make this probe + // hard-deny, ACCESS_DENIED, not silently filter. + if err := tenantConn.Exec(context.Background(), "SELECT * FROM system.query_log LIMIT 1"); err == nil { + t.Error("tenant user was able to run system.query_log query -- system.* access was not actually revoked on this ClickHouse version") } + // system.tables: unlike query_log, ClickHouse 24.8 treats this as a + // filtered catalog view rather than an access-checked table -- + // querying it never itself errors, regardless of grants (confirmed + // live, not assumed). The actual security property that matters is + // "no other tenant's database/table names leak through it," checked + // the same way SHOW DATABASES is checked below, not "the query + // errors." + rows, err := tenantConn.Query(context.Background(), "SELECT DISTINCT database FROM system.tables") + if err != nil { + t.Fatalf("querying system.tables: %v", err) + } + func() { + defer rows.Close() + for rows.Next() { + var db string + if err := rows.Scan(&db); err != nil { + t.Fatalf("scanning system.tables row: %v", err) + } + if db != tenantID && db != "system" && db != "INFORMATION_SCHEMA" && db != "information_schema" { + t.Errorf("system.tables revealed a database this tenant user shouldn't see: %q", db) + } + } + }() + // 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") + rows, err = tenantConn.Query(context.Background(), "SHOW DATABASES") if err != nil { return // erroring outright is also an acceptable outcome here. }