Revoke tenant users' system.* access explicitly, don't assume default-deny

Verified live against clickhouse/clickhouse-server:24.8: a freshly
created tenant user was NOT default-denied from system.* the way the
original design assumed -- system.tables listed every tenant's
database/table names to any authenticated user regardless of grants.
Fixed with an explicit REVOKE SELECT ON system.* FROM <user> after the
existing GRANT SELECT, INSERT.

Verifying this live also surfaced a real ClickHouse behavioral split
the design didn't anticipate: system.query_log is genuinely access-
checked (the REVOKE makes it hard-deny, ACCESS_DENIED), but
system.tables is a filtered catalog view that ClickHouse 24.8 never
denies outright -- it just returns zero rows for a properly-revoked
user. Both outcomes close the actual leak. Corrected
TestProvisionedUserCannotReadSystemTables to assert what each table
actually does (hard error for query_log, verified-empty-and-no-foreign-
database-names for tables) instead of demanding a hard error from both.

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
 }
 

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.
 	}