diff --git a/CLAUDE.md b/CLAUDE.md index fc65e541..3eafb673 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -740,6 +740,24 @@ Invariants for anyone touching this path: + the `__default_*`/`duckgres_*` prefixes, which activation re-creates). It MUST run before replay on every CreateSession in shared-warm mode, and a wipe failure MUST fail the session. +- **Client-ATTACHed catalogs get the same treatment, in the same place.** + `detachUserCatalogs` (`duckdbservice/user_catalogs.go`) runs right after the + secrets wipe on every shared-warm CreateSession and a failure MUST fail the + session (best-effort again at DestroySession). Attached databases are + instance-global exactly like secrets, and an attached catalog freezes its + connection string — credentials included — at ATTACH time, so it outlives + the wipe of the secret it was built from: without the detach, user B can + query user A's attached Postgres without ever holding A's credential. It is + also a correctness bug, not just isolation: clients attach with + `ATTACH IF NOT EXISTS ... AS db`, a no-op when the previous session left a + `db` behind, so the new session silently reads the PREVIOUS session's target + (2026-09-18: a tenant's sqlmesh run inherited an analyst's `db` endpoint and + its parallel postgres scans died with `SET TRANSACTION SNAPSHOT ... snapshot + does not exist`). Preserved: DuckDB-internal catalogs, the instance's primary + database (resolved by lowest oid — it is a file stem, not `memory`, when + DataDir is set), and the `isSystemCatalog` allowlist (`ducklake`, `delta`, + `__ducklake_metadata_*`). **A new worker-managed ATTACH must be added to that + allowlist** or the next session create will detach it. - **Execute-then-persist ordering.** Persist only statements DuckDB accepted; a store failure after a successful exec is an ERROR telling the user the secret will NOT survive the session. Replay failures at session create are @@ -762,10 +780,11 @@ Invariants for anyone touching this path: `usersecrets.RedactErrorForLog(query, errMsg)` guards those error sinks (`logQueryError`, `logQuery`); keep new error logging behind it too, and pass the original (un-redacted) query so it can classify. -- Touching the interception, wipe/replay, or payload shape → update - `server/conn_user_secrets_test.go`, `duckdbservice/user_secrets_test.go`, - and the `persistent_user_secret`(+`_isolation`) assertions in - `tests/mw-dev/e2e/harness.sh`. +- Touching the interception, wipe/replay, catalog detach, or payload shape → + update `server/conn_user_secrets_test.go`, + `duckdbservice/user_secrets_test.go`, `duckdbservice/user_catalogs_test.go`, + and the `persistent_user_secret`(+`_isolation`, which also carries the + attached-catalog leak check) assertions in `tests/mw-dev/e2e/harness.sh`. ## Admin Console (VPC-private web UI, `kubernetes` tag) diff --git a/duckdbservice/service.go b/duckdbservice/service.go index 3e695406..8cbeac90 100644 --- a/duckdbservice/service.go +++ b/duckdbservice/service.go @@ -1257,6 +1257,25 @@ func (p *SessionPool) CreateSession(username, memoryLimit string, threads int, s if len(wiped) > 0 { slog.Info("Wiped user secrets left by previous session.", "user", username, "count", len(wiped)) } + // Same isolation step for client-ATTACHed databases: they are + // instance-global too, and an attached catalog keeps the credentials + // it was built from even after the secret above is gone. Mandatory for + // the same reason as the wipe — see detachUserCatalogs. + // Own deadline: a slow detach must not eat the replay's budget below. + detachCtx, detachCancel := context.WithTimeout(context.Background(), userSecretOpTimeout) + detached, detachErr := detachUserCatalogs(detachCtx, conn) + detachCancel() + if detachErr != nil { + secretCancel() + _ = conn.Close() + p.mu.Lock() + p.reserved-- + p.mu.Unlock() + return nil, nil, fmt.Errorf("detach user catalogs before session start: %w", detachErr) + } + if len(detached) > 0 { + slog.Info("Detached user catalogs left by previous session.", "user", username, "count", len(detached), "catalogs", detached) + } secretWarnings = replayUserSecrets(secretCtx, conn, username, secretStatements) secretCancel() } @@ -1453,7 +1472,14 @@ func (p *SessionPool) DestroySession(token string) error { if _, err := wipeUserSecrets(wipeCtx, session.DB); err != nil { log.Warn("Failed to wipe user secrets on session destroy.", "user", session.Username, "error", err) } + // Likewise for client-ATTACHed databases, which carry their own copy + // of the credentials. Best-effort here; mandatory at CreateSession. wipeCancel() + detachCtx, detachCancel := context.WithTimeout(context.Background(), userSecretOpTimeout) + if _, err := detachUserCatalogs(detachCtx, session.DB); err != nil { + log.Warn("Failed to detach user catalogs on session destroy.", "user", session.Username, "error", err) + } + detachCancel() } // Best-effort restore of the cache-proxy S3 transport if this session diff --git a/duckdbservice/user_catalogs.go b/duckdbservice/user_catalogs.go new file mode 100644 index 00000000..8e4e273f --- /dev/null +++ b/duckdbservice/user_catalogs.go @@ -0,0 +1,109 @@ +package duckdbservice + +import ( + "context" + "fmt" + "strings" +) + +// systemCatalogNames are the attached databases the worker itself manages on a +// shared-warm instance: activation attaches (and re-attaches) them, so a +// session-boundary detach must leave them alone. +var systemCatalogNames = map[string]struct{}{ + "ducklake": {}, + "delta": {}, +} + +// systemCatalogPrefixes covers catalogs DuckDB extensions attach on the +// worker's behalf — DuckLake attaches its metadata store under this prefix. +var systemCatalogPrefixes = []string{"__ducklake_metadata_"} + +// isSystemCatalog reports whether an attached database is worker-managed. +// DuckDB catalog names are case-insensitive. +func isSystemCatalog(name string) bool { + name = strings.ToLower(name) + if _, ok := systemCatalogNames[name]; ok { + return true + } + for _, p := range systemCatalogPrefixes { + if strings.HasPrefix(name, p) { + return true + } + } + return false +} + +// detachUserCatalogs detaches every database a client ATTACHed on the shared +// per-org instance, preserving only what the worker manages itself: DuckDB's +// internal catalogs (system/temp), the instance's primary database, and the +// isSystemCatalog allowlist. +// +// Attached databases are instance-global, exactly like secrets, so an ATTACH +// survives its session on a hot-idle worker and is inherited by whichever +// session the worker serves next. Two distinct failures follow: +// +// - Cross-user isolation: an attached catalog freezes its connection string — +// credentials included — at ATTACH time, so it keeps working after +// wipeUserSecrets has dropped the secret it was built from. User B of the +// org could query user A's attached Postgres without ever holding A's +// credential. +// - Silent mis-routing: clients attach with `ATTACH IF NOT EXISTS ... AS db`, +// which is a no-op when a previous session left a `db` behind, so the new +// session reads whatever target the PREVIOUS session chose. On 2026-09-18 a +// tenant's sqlmesh run inherited an analyst's `db` (a different endpoint) +// and its parallel postgres scans failed with `SET TRANSACTION SNAPSHOT +// ... snapshot does not exist`. +// +// Like the secrets wipe this is only safe with one session per worker: with a +// concurrent session it would detach a catalog out from under a live query. +// +// Returns the names that were detached. +func detachUserCatalogs(ctx context.Context, h secretDBHandle) ([]string, error) { + // The primary database is the one the instance was opened on — the lowest + // non-internal oid, since everything else is attached afterwards. It is + // `memory` on k8s workers but a file stem when DataDir is set, so resolve it + // rather than hard-coding a name. + // + // The function is catalog-qualified on purpose. By this point in session + // create the default catalog is `ducklake`, and sessionmeta documents that + // an unqualified `duckdb_databases()` there starts a DuckLake transaction — + // which pays a full catalog reload (tens of seconds on a large tenant) when + // the schema version has moved. Naming `system` means the lookup never has + // to consult the default catalog. The session-init probe has normally paid + // for that reload already, so this is belt and braces, not a measured win. + rows, err := h.QueryContext(ctx, ` + SELECT database_name, + database_oid = (SELECT MIN(database_oid) FROM system.main.duckdb_databases() WHERE NOT internal) AS is_primary + FROM system.main.duckdb_databases() + WHERE NOT internal`) + if err != nil { + return nil, fmt.Errorf("list attached databases: %w", err) + } + var candidates []string + for rows.Next() { + var name string + var isPrimary bool + if err := rows.Scan(&name, &isPrimary); err != nil { + _ = rows.Close() + return nil, err + } + if isPrimary || isSystemCatalog(name) { + continue + } + candidates = append(candidates, name) + } + if err := rows.Err(); err != nil { + _ = rows.Close() + return nil, err + } + _ = rows.Close() + + var detached []string + for _, name := range candidates { + if _, err := h.ExecContext(ctx, "DETACH DATABASE IF EXISTS "+quoteSecretIdent(name)); err != nil { + return detached, fmt.Errorf("detach database %q: %w", name, err) + } + detached = append(detached, name) + } + return detached, nil +} diff --git a/duckdbservice/user_catalogs_test.go b/duckdbservice/user_catalogs_test.go new file mode 100644 index 00000000..e54c283a --- /dev/null +++ b/duckdbservice/user_catalogs_test.go @@ -0,0 +1,193 @@ +package duckdbservice + +import ( + "context" + "database/sql" + "path/filepath" + "sort" + "strings" + "testing" + "time" + + "github.com/posthog/duckgres/server" +) + +func attachedDatabases(t *testing.T, db *sql.DB) []string { + t.Helper() + rows, err := db.Query("SELECT database_name FROM duckdb_databases() WHERE NOT internal ORDER BY database_name") + if err != nil { + t.Fatalf("list databases: %v", err) + } + defer func() { _ = rows.Close() }() + var names []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + t.Fatalf("scan: %v", err) + } + names = append(names, n) + } + return names +} + +func mustExecDB(t *testing.T, db *sql.DB, q string) { + t.Helper() + if _, err := db.Exec(q); err != nil { + t.Fatalf("exec %q: %v", q, err) + } +} + +// Regression for the 2026-09-18 incident: a client `ATTACH ... AS db` survived +// its session on the hot-idle worker, so the next session's +// `ATTACH IF NOT EXISTS ... AS db` was a no-op and it read the PREVIOUS +// session's target. The detach must remove client catalogs and keep the +// worker-managed ones. +func TestDetachUserCatalogs(t *testing.T) { + db := openSecretTestDB(t) + dir := t.TempDir() + // Worker-managed catalogs, stood in for by plain DuckDB files: only the + // names matter to the allowlist. + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "lake.duckdb")+"' AS ducklake") + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "meta.duckdb")+"' AS __ducklake_metadata_ducklake") + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "delta.duckdb")+"' AS delta") + // Client catalogs, including one needing identifier quoting. + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "a.duckdb")+"' AS db") + mustExecDB(t, db, "ATTACH ':memory:' AS \"My \"\"odd\"\" Db\"") + + detached, err := detachUserCatalogs(context.Background(), db) + if err != nil { + t.Fatalf("detachUserCatalogs: %v", err) + } + sort.Strings(detached) + if got, want := strings.Join(detached, ","), `My "odd" Db,db`; got != want { + t.Errorf("detached = %q, want %q", got, want) + } + + got := strings.Join(attachedDatabases(t, db), ",") + if want := "__ducklake_metadata_ducklake,delta,ducklake,memory"; got != want { + t.Errorf("remaining databases = %q, want %q", got, want) + } + + // The point of the fix: the next session's IF NOT EXISTS attach must now + // take effect instead of silently inheriting the old target. + mustExecDB(t, db, "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "b.duckdb")+"' AS db") + var path string + if err := db.QueryRow("SELECT path FROM duckdb_databases() WHERE database_name = 'db'").Scan(&path); err != nil { + t.Fatalf("read db path: %v", err) + } + if !strings.HasSuffix(path, "b.duckdb") { + t.Errorf("db path = %q, want the new session's target (b.duckdb)", path) + } +} + +// The primary database is a file stem, not `memory`, when the instance is +// opened on a file. It must never be a detach candidate regardless of name. +func TestDetachUserCatalogsKeepsFileBackedPrimary(t *testing.T) { + dir := t.TempDir() + db, err := sql.Open("duckdb", filepath.Join(dir, "alice.duckdb")) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "other.duckdb")+"' AS db") + + detached, err := detachUserCatalogs(context.Background(), db) + if err != nil { + t.Fatalf("detachUserCatalogs: %v", err) + } + if len(detached) != 1 || detached[0] != "db" { + t.Errorf("detached = %v, want [db]", detached) + } + if got := strings.Join(attachedDatabases(t, db), ","); got != "alice" { + t.Errorf("remaining databases = %q, want %q", got, "alice") + } +} + +func TestDetachUserCatalogsNoop(t *testing.T) { + db := openSecretTestDB(t) + detached, err := detachUserCatalogs(context.Background(), db) + if err != nil { + t.Fatalf("detachUserCatalogs: %v", err) + } + if len(detached) != 0 { + t.Errorf("detached = %v, want none", detached) + } +} + +func TestIsSystemCatalog(t *testing.T) { + for name, want := range map[string]bool{ + "ducklake": true, + "DuckLake": true, + "delta": true, + "__ducklake_metadata_ducklake": true, + "db": false, + "ducklake2": false, + "memory": false, // protected as the primary, not by name + } { + if got := isSystemCatalog(name); got != want { + t.Errorf("isSystemCatalog(%q) = %v, want %v", name, got, want) + } + } +} + +// End-to-end at the pool level: two consecutive sessions on one shared-warm +// worker. Session A attaches `db`; session B — possibly a different user of the +// org — must not inherit it, and B's own IF NOT EXISTS attach must win. +func TestCreateSessionDetachesPreviousSessionCatalogs(t *testing.T) { + db := openSecretTestDB(t) + dir := t.TempDir() + pool := &SessionPool{ + sessions: make(map[string]*Session), + stopRefresh: make(map[string]func()), + duckLakeSem: make(chan struct{}, 1), + warmupDB: db, + warmupDone: make(chan struct{}), + cfg: server.Config{SessionInitTimeout: time.Second}, + maxSessions: 1, + sharedWarmMode: true, + activation: &activatedTenantRuntime{payload: ActivationPayload{OrgID: "analytics"}, db: db}, + } + close(pool.warmupDone) + + a, _, err := pool.CreateSession("alice", "", 0, nil) + if err != nil { + t.Fatalf("CreateSession(alice): %v", err) + } + if _, err := a.Conn.ExecContext(context.Background(), + "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "alice_target.duckdb")+"' AS db"); err != nil { + t.Fatalf("alice attach: %v", err) + } + if err := pool.DestroySession(a.ID); err != nil { + t.Fatalf("DestroySession(alice): %v", err) + } + // The destroy-time detach is best-effort, but with nothing in the way it + // should already have cleared the catalog off the hot-idle worker. + if got := strings.Join(attachedDatabases(t, db), ","); got != "memory" { + t.Errorf("databases after alice's session = %q, want only memory", got) + } + + // Simulate the destroy-time detach having been skipped (it is best-effort): + // the CreateSession detach is the one that must hold. + mustExecDB(t, db, "ATTACH '"+filepath.Join(dir, "alice_target.duckdb")+"' AS db") + + b, _, err := pool.CreateSession("bob", "", 0, nil) + if err != nil { + t.Fatalf("CreateSession(bob): %v", err) + } + defer func() { _ = pool.DestroySession(b.ID) }() + if got := strings.Join(attachedDatabases(t, db), ","); got != "memory" { + t.Fatalf("bob inherited alice's catalogs: %q", got) + } + if _, err := b.Conn.ExecContext(context.Background(), + "ATTACH IF NOT EXISTS '"+filepath.Join(dir, "bob_target.duckdb")+"' AS db"); err != nil { + t.Fatalf("bob attach: %v", err) + } + var path string + if err := b.Conn.QueryRowContext(context.Background(), + "SELECT path FROM duckdb_databases() WHERE database_name = 'db'").Scan(&path); err != nil { + t.Fatalf("read db path: %v", err) + } + if !strings.HasSuffix(path, "bob_target.duckdb") { + t.Errorf("bob's db path = %q, want bob_target.duckdb", path) + } +} diff --git a/tests/mw-dev/e2e/harness.sh b/tests/mw-dev/e2e/harness.sh index 5bfb09d8..176f8620 100755 --- a/tests/mw-dev/e2e/harness.sh +++ b/tests/mw-dev/e2e/harness.sh @@ -2548,11 +2548,43 @@ persistent_user_secret_isolation() { # org rootpw # TEMPORARY secret left behind on the instance-global worker DuckDB. n="$(pg "$org" "$u2pw" ducklake "SELECT count(*) FROM duckdb_secrets() WHERE name = '$tname'" "$u2")" [ "$n" = "0" ] || fail "user secret: user $u2 sees root's TEMPORARY secret — cross-user leak (count=$n)" + # Same leak path for client-ATTACHed catalogs, which are instance-global too + # and keep the credentials they were attached with. Regression for the + # 2026-09-18 incident: one session's `ATTACH ... AS db` survived on the + # hot-idle worker, the next session's `ATTACH IF NOT EXISTS ... AS db` was a + # no-op, and it silently queried the PREVIOUS session's target. Root leaves a + # catalog holding a marker table; $u2's IF NOT EXISTS attach under the same + # alias must come up EMPTY (a fresh catalog), not inherit root's. + # + # Each side's statements must ride ONE connection — every `pg` call is its + # own session, whose session-create detach would already have removed the + # catalog — so they are fed to psql on stdin (one Q message per statement, + # same session). ATTACH is not PostgreSQL syntax, so it parse-fails into the + # pin set and both sides land on the org's standard worker rather than being + # split across the exploratory tier. In-memory catalogs keep the check free of + # external dependencies; the name is run-unique. + lcat="e2e_leakcat_$$" + out="$(printf "ATTACH ':memory:' AS %s;\nCREATE TABLE %s.main.marker AS SELECT 42 AS x;\nSELECT count(*) FROM duckdb_tables() WHERE database_name = '%s';\n" "$lcat" "$lcat" "$lcat" | \ + PGPASSWORD="$pw" psql \ + "sslmode=require host=$org$SNI_SUFFIX hostaddr=$CP_IP port=5432 user=root dbname=ducklake" \ + -v ON_ERROR_STOP=1 -tA 2>&1)" || fail "user catalog: root attach session failed: $out" + [ "$(printf '%s' "$out" | tail -1)" = "1" ] || fail "user catalog: root's marker table not visible in its own session ($out)" + out="$(printf "ATTACH IF NOT EXISTS ':memory:' AS %s;\nSELECT count(*) FROM duckdb_tables() WHERE database_name = '%s';\n" "$lcat" "$lcat" | \ + PGPASSWORD="$u2pw" psql \ + "sslmode=require host=$org$SNI_SUFFIX hostaddr=$CP_IP port=5432 user=$u2 dbname=ducklake" \ + -v ON_ERROR_STOP=1 -tA 2>&1)" || fail "user catalog: $u2 attach session failed: $out" + [ "$(printf '%s' "$out" | tail -1)" = "0" ] || fail "user catalog: user $u2 inherited root's attached catalog — cross-session ATTACH leak ($out)" + # Gone from the next fresh session too, while the worker-managed catalogs + # must survive every session-create detach. + n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_databases() WHERE database_name = '$lcat'")" + [ "$n" = "0" ] || fail "user catalog: attached catalog survived into a fresh session (count=$n)" + n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_databases() WHERE database_name IN ('ducklake','memory')")" + [ "$n" = "2" ] || fail "user catalog: worker-managed catalogs missing after the detach (count=$n)" # Root's stored (persistent) copy must be unaffected by $u2's session-create wipe. n="$(pg "$org" "$pw" ducklake "SELECT count(*) FROM duckdb_secrets() WHERE name = '$sname'")" [ "$n" = "1" ] || fail "user secret: root's persistent secret lost after $u2's session (count=$n)" pg "$org" "$pw" ducklake "DROP PERSISTENT SECRET $sname" >/dev/null - log "user secret isolation OK on $org ($u2 blind to root's persistent AND temporary secrets; root's stored copy intact)" + log "user secret isolation OK on $org ($u2 blind to root's persistent AND temporary secrets AND attached catalogs; root's stored copy intact)" } # ---- resilience -----------------------------------------------------------