From 8ecd760ce49ee2fdd8741aefac490f565cd102a3 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Tue, 1 Sep 2026 01:26:39 +0000 Subject: [PATCH] Let every duckgres login authenticate to Trino A tenant's only Trino credential was its org root password: the projection joined duckgres_org_users on username = 'root' and wrote one password.db line per org, so anyone wanting Trino access had to be handed a shared production credential. Every login an org already has now authenticates to Trino too, as ., with the bcrypt hash copied through unchanged - it is the same hash pgwire verifies, so one password works on both engines and nothing is re-hashed, minted or reset. The bare principal survives alongside them for service-to-service use and for clients configured before this existed. Trino's password file is ONE flat namespace per cell, while duckgres keys a login on (org, username) and recovers the org from SNI - which a Trino login carries no equivalent of. Qualifying the username is what makes that flat namespace safe, and it brings three consequences that are load-bearing rather than cosmetic: * Usernames are projected through an allowlist. duckgres validates a username as little more than "not empty", while password.db is : per line and group.db is :,. A username holding ':', ',' or a newline would let whoever can create org users append arbitrary lines to those files, including a line for the admin principal. * rejectPrincipalCollisions now also holds back orgs deriving the same Trino username. Valid database_names make that unreachable, but grandfathered rows may hold a dot, and a duplicate password.db line lets one org's user authenticate against another org's entry. * The resource-group selector captures only up to the first '.'. The previous (?.*) matched the whole username, so every login would get a private leaf carrying the full per-tenant limits and an org with ten logins would quietly hold ten times its concurrency and memory budget. Project-scoped logins keep their scope rather than being excluded or silently widened. Such a login joins scope__team_ instead of the org group; that group owns the same catalog in group_catalogs - so the cross-tenant check is the unchanged check - and carries a new group_scopes document narrowing it to the team's schemas and individually granted relations. Scopes only ever subtract, which is the property that keeps this off the tenant-isolation path: a bug in the new rules can widen access only within one org's own catalog. The scope is read through OrgUserQueryAccess, the same derivation the pgwire session path uses, so the two engines cannot disagree about it, and a scoped row whose scope will not resolve is dropped rather than projected unscoped. Scoped logins get no write authority at all. duckgres has a read-only project login and a read/write one, and only the read-only half is expressible here today, so denying writes to both narrows project_user rather than widening project_reader. Two lags are worth stating: a disabled user leaves password.db only when the projected Secret is re-read (kubelet sync plus the group provider's file.refresh-period), so the kill switch takes effect on Trino in up to a couple of minutes rather than instantly as it does on pgwire; and service credentials (duckgres_service_grants) are deliberately not projected here - their TTL and revocation semantics deserve their own change. --- CLAUDE.md | 27 ++ controlplane/configstore/models.go | 63 ++++ controlplane/configstore/trino.go | 98 +++++- controlplane/provisioner/opa/builder.go | 34 +- controlplane/provisioner/opa/builder_test.go | 132 +++++++- controlplane/provisioner/opa/latency_test.go | 2 +- controlplane/provisioner/opa/policy.rego | 147 +++++++- controlplane/provisioner/opa/policy_test.go | 316 +++++++++++++++++- controlplane/provisioner/opa/types.go | 64 +++- controlplane/provisioner/trino_provisioner.go | 261 +++++++++++++-- .../provisioner/trino_provisioner_test.go | 302 ++++++++++++++++- tests/configstore/trino_postgres_test.go | 131 ++++++++ 12 files changed, 1509 insertions(+), 68 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5642264e..2b350962 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1577,6 +1577,33 @@ password/tenant/catalog changes never propagate. `opa.ManagedCatalogPattern`, and the regex literal inside `policy.rego`. `TestTrinoCatalogNameMatchesManagedNamePattern` + `TestPolicyRegoContainsManagedNamePattern` fail if any one moves alone. +- **Every duckgres login authenticates to Trino, under `.`.** + `ListTrinoEnabledOrgs` returns the org's logins in `Users`, and + `BuildTrinoAuthFiles` writes one `password.db` line per login with the + bcrypt hash **copied through unchanged** — it is the same hash pgwire + verifies, so one password works on both engines and nothing is re-hashed or + minted. The bare `` principal survives alongside them. Three + rules that are load-bearing rather than cosmetic: (1) usernames are + projected through an **allowlist** (`trinoUsernamePattern`) because + duckgres barely validates them and a `:`, `,` or newline would let whoever + can create org users append lines to `password.db` — including an admin + line; (2) `rejectPrincipalCollisions` now also holds back orgs that derive + the same Trino username, since the password file is ONE flat namespace per + cell and a duplicate line is a cross-tenant auth bug; (3) the resource-group + selector's `orgCaptureRegex` captures only up to the first `.`, or every + login gets a private leaf with the full per-tenant limits and an org with + ten logins holds ten times its budget. +- **A project-scoped login (`project_reader` / `project_user`) joins + `scope__team_`, NOT the org group.** The scope group owns the same + catalog in `group_catalogs` — so the cross-tenant check is the unchanged + check — and carries a `group_scopes` document that narrows it to that + team's schemas. Scopes only ever SUBTRACT; keep it that way if the rules are + restructured. The scope comes from `OrgUserQueryAccess`, the same derivation + the pgwire path uses, so Trino and DuckDB cannot disagree about it, and a + scoped row whose scope will not resolve is DROPPED rather than projected + unscoped. Scoped logins get **no write authority at all** — `project_user` + is read/write on pgwire and read-only here, a narrowing; making it writable + means gating writes per-schema, not per-catalog. - **The Rego policy is the tenant-isolation boundary.** The cell can assume every per-org duckling role, so nothing below OPA stops org A reading org B's catalog. Treat `provisioner/opa/policy.rego` as security review. diff --git a/controlplane/configstore/models.go b/controlplane/configstore/models.go index 5fdf1ed9..c71d2641 100644 --- a/controlplane/configstore/models.go +++ b/controlplane/configstore/models.go @@ -522,6 +522,69 @@ type TrinoEnabledOrg struct { CellID string RootPasswordHash string // bcrypt hash from OrgUser row where Username = "root" State ManagedWarehouseProvisioningState // current state at read time + // Users are the org's own duckgres logins, each of which authenticates + // to Trino under TrinoUserPrincipal(Username) with the very same bcrypt + // hash it uses at the pgwire handshake. Populated by a second query in + // ListTrinoEnabledOrgs, hence `gorm:"-"` -- it is not a column on the + // row the outer join scans into. + Users []TrinoOrgUser `gorm:"-"` +} + +// TrinoOrgUser is one of an org's duckgres logins, projected into the cell's +// password file so a person who already has a pgwire credential can use that +// same credential against Trino instead of sharing the org's root password. +type TrinoOrgUser struct { + Username string + // PasswordHash is duckgres_org_users.password, copied through unchanged. + // It is bcrypt at cost 10, which Trino's file authenticator accepts as + // is (its floor is cost 8), so ONE password works on both engines and no + // separate Trino credential is ever minted or stored. + PasswordHash string + // Scope, when non-nil, restricts the login to one project's schemas. + // Mirrors duckgres_org_users.access_mode's project_reader / project_user + // modes; nil means an unrestricted org-wide login. + // + // The value is whatever OrgUserQueryAccess reports for this user, so the + // scope Trino enforces and the scope pgwire enforces are the same object + // derived by the same code -- see ListTrinoEnabledOrgs. + Scope *OrgUserQueryAccess + // TeamID is the project a scoped login is bound to, and is what its Trino + // group is keyed on. Non-nil exactly when Scope is: the two are set + // together and a scoped row with no team is dropped rather than + // projected (the table's CHECK constraint already forbids that shape). + // + // Carried here rather than on OrgUserQueryAccess because that type is the + // pgwire session path's policy object and has no reason to grow a field + // only the Trino projection reads. + TeamID *int64 +} + +// TrinoPrincipalSeparator joins an org's principal to one of its usernames to +// form a cell-wide-unique Trino username. +// +// Trino's password file is ONE flat namespace per cell while duckgres keys a +// login on (org, username) and recovers the org from SNI -- which a Trino +// login carries no equivalent of. Two orgs may each have an `analyst`, and +// two identical password-file lines would let one org's user authenticate +// against the other's entry and land in the other's group. Qualifying the +// username is what makes the flat namespace safe. +// +// `.` is the separator because a valid database_name cannot contain one (see +// ValidateDatabaseName: a DNS label) and neither can a projectable username +// (see projectableTrinoUsername), so `.` splits unambiguously and +// the org prefix stays recoverable -- which the resource-group selector's +// named capture depends on. +const TrinoPrincipalSeparator = "." + +// TrinoUserPrincipal returns the Trino username for one of the org's duckgres +// logins: the org's own principal, the separator, then the duckgres username. +// Returns "" when either half is missing, which callers skip. +func (o TrinoEnabledOrg) TrinoUserPrincipal(username string) string { + principal := o.TrinoPrincipal() + if principal == "" || username == "" { + return "" + } + return principal + TrinoPrincipalSeparator + username } // TrinoPrincipal is the tenant's customer-facing identity in Trino: the diff --git a/controlplane/configstore/trino.go b/controlplane/configstore/trino.go index 94213506..42059576 100644 --- a/controlplane/configstore/trino.go +++ b/controlplane/configstore/trino.go @@ -3,6 +3,7 @@ package configstore import ( "errors" "fmt" + "log/slog" "time" "gorm.io/gorm" @@ -188,9 +189,15 @@ func (cs *ConfigStore) DisableTrino(orgID string) error { } // ListTrinoEnabledOrgs returns every org with ManagedWarehouseTrino.Enabled -// = true joined against its `root` OrgUser row. The provisioner needs the -// bcrypt hash to project the Trino password file, so this is a single join -// rather than two round-trips. +// = true joined against its `root` OrgUser row, each carrying the org's full +// set of projectable logins in Users. The provisioner needs the bcrypt hashes +// to project the Trino password file. +// +// The `root` join stays because database_name alone remains a principal in +// its own right (TrinoPrincipal) for service-to-service use and for every +// client configured before per-user logins existed. Users is the ADDITIONAL +// per-human projection; root therefore appears twice, as `` and as +// `.root`, and both authenticate against the same hash. // // Orgs that are Trino-enabled but have no `root` OrgUser are skipped — that // shape can't legitimately happen via the provisioning API (CreateOrgUser @@ -229,9 +236,94 @@ func (cs *ConfigStore) ListTrinoEnabledOrgs() ([]TrinoEnabledOrg, error) { if err != nil { return nil, fmt.Errorf("list trino-enabled orgs: %w", err) } + if len(out) == 0 { + return out, nil + } + if err := cs.attachTrinoOrgUsers(out); err != nil { + return nil, err + } return out, nil } +// trinoOrgUserRow is one (org, login) pair from the second listing query. +type trinoOrgUserRow struct { + OrgID string + Username string + Password string + AccessMode string + TeamID *int64 +} + +// attachTrinoOrgUsers loads every projectable duckgres login for the listed +// orgs and hangs it off the matching TrinoEnabledOrg. +// +// A second query rather than a widened join: the outer listing is one row per +// org and the provisioner's per-org steps (catalog, tenant password, state) +// all key on that shape, so fanning it out to one row per user would make +// every caller de-duplicate. One extra round trip per reconcile tick is not +// a cost worth that. +// +// Two rows are excluded in SQL, both fail-closed: +// +// - disabled = true. duckgres_org_users.disabled is the per-user kill +// switch. Trino learns about a flip only when the projected Secret is +// re-read (kubelet sync + the group provider's file.refresh-period), so +// a disable takes effect here in up to a couple of minutes rather than +// instantly as it does on pgwire. That lag is worth stating wherever the +// kill switch is surfaced to operators; it is not a reason to leave the +// row out of the exclusion. +// - a blank password. There is no hash to project and Trino would reject +// the line anyway. +// +// Project-scoped logins ARE included, and carry their scope. The scope is +// read through OrgUserQueryAccess -- the SAME derivation the pgwire session +// path uses -- so Trino and DuckDB can never disagree about which schemas a +// project login may read. A scoped row whose scope cannot be resolved is +// dropped rather than projected unscoped: an unresolvable scope must never +// silently widen into org-wide access. +func (cs *ConfigStore) attachTrinoOrgUsers(orgs []TrinoEnabledOrg) error { + ids := make([]string, 0, len(orgs)) + for _, o := range orgs { + ids = append(ids, o.OrgID) + } + var rows []trinoOrgUserRow + err := cs.db.Table("duckgres_org_users"). + Select("org_id, username, password, access_mode, team_id"). + Where("org_id IN ?", ids). + Where("disabled = ?", false). + Where("password <> ''"). + Order("org_id ASC, username ASC"). + Scan(&rows).Error + if err != nil { + return fmt.Errorf("list trino org users: %w", err) + } + + byOrg := make(map[string][]TrinoOrgUser, len(orgs)) + for _, r := range rows { + u := TrinoOrgUser{Username: r.Username, PasswordHash: r.Password} + if IsProjectScopedAccessMode(r.AccessMode) { + access, scoped := cs.OrgUserQueryAccess(r.OrgID, r.Username) + if !scoped || r.TeamID == nil { + // The row says scoped but the snapshot does not agree -- + // an unloaded or stale snapshot, or a user written since + // the last poll. Projecting it now would grant the whole + // org catalog to a login that must only see one project. + // Drop it; the next tick projects it once both agree. + slog.Warn("Trino: skipping project-scoped login whose scope is unresolved.", + "org", r.OrgID, "user", r.Username, "access_mode", r.AccessMode) + continue + } + u.Scope = &access + u.TeamID = r.TeamID + } + byOrg[r.OrgID] = append(byOrg[r.OrgID], u) + } + for i := range orgs { + orgs[i].Users = byOrg[orgs[i].OrgID] + } + return nil +} + // GetManagedWarehouseTrino reads the Trino row for an org. Returns // (nil, nil) when no row exists so callers can distinguish "never // configured" from a DB error. diff --git a/controlplane/provisioner/opa/builder.go b/controlplane/provisioner/opa/builder.go index 657611ae..863b4588 100644 --- a/controlplane/provisioner/opa/builder.go +++ b/controlplane/provisioner/opa/builder.go @@ -47,8 +47,14 @@ func NewBuilder() BundleBuilder { // activates a deny-everything policy (since no group owns any catalog). // That is the correct bootstrap behaviour: until the provisioner pushes // a populated GroupCatalogs, all customer queries are denied. -func (defaultBuilder) BuildBundle(gc GroupCatalogs) ([]byte, error) { - data, err := buildDataDocument(gc) +// +// gs carries the project scopes that narrow individual groups. A nil or empty +// GroupScopes means no group is scoped, which is the pre-scopes behaviour: a +// group reads the whole catalog it owns. Both documents are always emitted so +// the policy's `data.group_scopes[g]` lookup is undefined-on-missing-key +// rather than an error on a missing document. +func (defaultBuilder) BuildBundle(gc GroupCatalogs, gs GroupScopes) ([]byte, error) { + data, err := buildDataDocument(gc, gs) if err != nil { return nil, fmt.Errorf("build data document: %w", err) } @@ -56,7 +62,7 @@ func (defaultBuilder) BuildBundle(gc GroupCatalogs) ([]byte, error) { b := bundle.Bundle{ Manifest: bundle.Manifest{ Revision: bundleRevision, - Roots: &[]string{"trino", "group_catalogs"}, + Roots: &[]string{"trino", "group_catalogs", "group_scopes"}, }, Modules: []bundle.ModuleFile{ { @@ -85,26 +91,30 @@ func (defaultBuilder) BuildBundle(gc GroupCatalogs) ([]byte, error) { // stores under data.. We always emit `group_catalogs` even when gc is // nil so the policy's `data.group_catalogs[group][catalog]` lookup is // well-formed (undefined-on-missing-key, not error-on-missing-document). -func buildDataDocument(gc GroupCatalogs) (map[string]interface{}, error) { +func buildDataDocument(gc GroupCatalogs, gs GroupScopes) (map[string]interface{}, error) { // JSON round-trip ensures we emit canonical JSON-decoded types // (map[string]interface{} and bool) regardless of what the caller // passes in. OPA's bundle loader expects these types and treats // concrete map[string]map[string]bool as opaque if it ever leaks // through. Round-tripping is also a stable serialization for tests. + if gc == nil { + // Marshalling a nil map emits "null"; substitute an empty object so + // the policy sees `data.group_catalogs == {}` not `null`. + gc = GroupCatalogs{} + } + if gs == nil { + gs = GroupScopes{} + } raw, err := json.Marshal(struct { GroupCatalogs GroupCatalogs `json:"group_catalogs"` - }{GroupCatalogs: gc}) + GroupScopes GroupScopes `json:"group_scopes"` + }{GroupCatalogs: gc, GroupScopes: gs}) if err != nil { - return nil, fmt.Errorf("marshal group_catalogs: %w", err) - } - if gc == nil { - // Marshalling a nil map emits "null"; substitute an empty object - // so the policy sees `data.group_catalogs == {}` not `null`. - raw = []byte(`{"group_catalogs":{}}`) + return nil, fmt.Errorf("marshal bundle data: %w", err) } var data map[string]interface{} if err := json.Unmarshal(raw, &data); err != nil { - return nil, fmt.Errorf("unmarshal group_catalogs: %w", err) + return nil, fmt.Errorf("unmarshal bundle data: %w", err) } return data, nil } diff --git a/controlplane/provisioner/opa/builder_test.go b/controlplane/provisioner/opa/builder_test.go index e4d99aca..332bac16 100644 --- a/controlplane/provisioner/opa/builder_test.go +++ b/controlplane/provisioner/opa/builder_test.go @@ -4,12 +4,25 @@ import ( "bytes" "net/http" "net/http/httptest" + "slices" "strings" "testing" "github.com/open-policy-agent/opa/v1/bundle" ) +// readBundle parses built bundle bytes back through OPA's own reader, so the +// scope assertions below check what OPA will actually load rather than what +// the builder intended to write. +func readBundle(t *testing.T, raw []byte) bundle.Bundle { + t.Helper() + parsed, err := bundle.NewReader(bytes.NewReader(raw)).Read() + if err != nil { + t.Fatalf("bundle.Read: %v", err) + } + return parsed +} + // TestBuildBundleRoundTrip builds a bundle, parses it back through OPA's // bundle reader, and asserts that the round-trip preserves the policy // source and data document. @@ -20,7 +33,7 @@ func TestBuildBundleRoundTrip(t *testing.T) { AdminGroup: {"org_42": true, "org_43": true}, } - raw, err := NewBuilder().BuildBundle(gc) + raw, err := NewBuilder().BuildBundle(gc, nil) if err != nil { t.Fatalf("BuildBundle: %v", err) } @@ -68,7 +81,7 @@ func TestBuildBundleRoundTrip(t *testing.T) { // what an empty group_catalogs gives us. func TestBuildBundleEmptyInput(t *testing.T) { for _, gc := range []GroupCatalogs{nil, {}} { - raw, err := NewBuilder().BuildBundle(gc) + raw, err := NewBuilder().BuildBundle(gc, nil) if err != nil { t.Fatalf("BuildBundle(empty): %v", err) } @@ -100,7 +113,7 @@ func TestBuildBundleEmptyInput(t *testing.T) { func TestBundleStoreAndHandler200(t *testing.T) { gc := GroupCatalogs{"org_42": {"org_42": true}} - raw, err := NewBuilder().BuildBundle(gc) + raw, err := NewBuilder().BuildBundle(gc, nil) if err != nil { t.Fatalf("BuildBundle: %v", err) } @@ -140,7 +153,7 @@ func TestBundleStoreAndHandler200(t *testing.T) { func TestBundleHandler304OnIfNoneMatch(t *testing.T) { gc := GroupCatalogs{"org_42": {"org_42": true}} - raw, _ := NewBuilder().BuildBundle(gc) + raw, _ := NewBuilder().BuildBundle(gc, nil) b := NewBundle(raw) store := &BundleStore{} store.Set(b) @@ -167,7 +180,7 @@ func TestBundleHandler304OnIfNoneMatch(t *testing.T) { func TestBundleHandler200OnEtagMiss(t *testing.T) { gc := GroupCatalogs{"org_42": {"org_42": true}} - raw, _ := NewBuilder().BuildBundle(gc) + raw, _ := NewBuilder().BuildBundle(gc, nil) store := &BundleStore{} store.Set(NewBundle(raw)) @@ -203,7 +216,7 @@ func TestBundleHandler503BeforeFirstBundle(t *testing.T) { func TestBundleHandlerRejectsNonGET(t *testing.T) { store := &BundleStore{} - raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}) + raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}, nil) store.Set(NewBundle(raw)) srv := httptest.NewServer(NewHandler(store, allowAllForTest)) @@ -227,7 +240,7 @@ func TestBundleHandlerRejectsNonGET(t *testing.T) { func TestBundleHandlerBearerTokenAuth(t *testing.T) { store := &BundleStore{} - raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}) + raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}, nil) store.Set(NewBundle(raw)) srv := httptest.NewServer(NewHandler(store, BearerTokenAuth("hunter2"))) @@ -293,7 +306,7 @@ func TestNewHandlerRejectsNilArgs(t *testing.T) { // guard in ServeHTTP must still fail closed. func TestHandlerLiteralWithNilAuthFailsClosed(t *testing.T) { store := &BundleStore{} - raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}) + raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}, nil) store.Set(NewBundle(raw)) srv := httptest.NewServer(&Handler{Store: store}) // no Auth set @@ -322,7 +335,7 @@ func TestBundleStoreSetOverwrites(t *testing.T) { store := &BundleStore{} // First bundle. - raw1, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}) + raw1, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}, nil) b1 := NewBundle(raw1) store.Set(b1) got, ok := store.Current() @@ -334,7 +347,7 @@ func TestBundleStoreSetOverwrites(t *testing.T) { raw2, _ := NewBuilder().BuildBundle(GroupCatalogs{ "org_42": {"org_42": true}, "org_43": {"org_43": true}, - }) + }, nil) b2 := NewBundle(raw2) if b1.ETag == b2.ETag { t.Fatal("bundles with different content should not share an ETag (sha256 collision?)") @@ -376,7 +389,7 @@ func TestNewBundleIsolatesInputSlice(t *testing.T) { func TestBundleHasNoExportedMutableByteAccess(t *testing.T) { // Construct a bundle, store it, mutate everything we can reach. store := &BundleStore{} - raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}) + raw, _ := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}, nil) b := NewBundle(raw) store.Set(b) @@ -422,3 +435,100 @@ func TestNewBundleETagIsContentAddressed(t *testing.T) { t.Errorf("ETag should be a quoted string, got %q", b1.ETag) } } + +// --- project scopes --- + +// The bundle must carry group_scopes as its own document, and must declare it +// as a root: OPA refuses to activate a bundle that writes outside its +// declared roots, so a missing root is a bundle that silently never applies. +func TestBuildBundleCarriesGroupScopes(t *testing.T) { + gc := GroupCatalogs{"scope_acme_team_7": {"org_acme": true}} + gs := GroupScopes{"scope_acme_team_7": NewGroupScope( + []string{"posthog_7"}, []string{"posthog.events"})} + + raw, err := NewBuilder().BuildBundle(gc, gs) + if err != nil { + t.Fatalf("BuildBundle: %v", err) + } + b := readBundle(t, raw) + + roots := *b.Manifest.Roots + if !slices.Contains(roots, "group_scopes") { + t.Errorf("manifest roots = %v, must contain group_scopes", roots) + } + + scopes, ok := b.Data["group_scopes"].(map[string]interface{}) + if !ok { + t.Fatalf("group_scopes missing or wrong type: %#v", b.Data["group_scopes"]) + } + scope, ok := scopes["scope_acme_team_7"].(map[string]interface{}) + if !ok { + t.Fatalf("scope document missing: %#v", scopes) + } + for _, key := range []string{"schemas", "relations", "relation_schemas"} { + if _, ok := scope[key].(map[string]interface{}); !ok { + t.Errorf("scope.%s missing or wrong type: %#v", key, scope[key]) + } + } +} + +// An unscoped build must still emit an EMPTY group_scopes object rather than +// null or nothing: the policy's data.group_scopes[g] lookup has to be +// undefined-on-missing-key, and a null document makes it an evaluation error +// instead -- which fails every decision, not just the scoped ones. +func TestBuildBundleAlwaysEmitsGroupScopes(t *testing.T) { + raw, err := NewBuilder().BuildBundle(GroupCatalogs{"org_42": {"org_42": true}}, nil) + if err != nil { + t.Fatalf("BuildBundle: %v", err) + } + b := readBundle(t, raw) + scopes, ok := b.Data["group_scopes"].(map[string]interface{}) + if !ok { + t.Fatalf("group_scopes must be an object even when unscoped: %#v", b.Data["group_scopes"]) + } + if len(scopes) != 0 { + t.Errorf("group_scopes = %v, want empty", scopes) + } +} + +// RelationSchemas is derived, never supplied, so it cannot disagree with +// Relations. A schema the group can read one table in must appear there or +// the client can never navigate to that table. +func TestNewGroupScopeDerivesRelationSchemas(t *testing.T) { + scope := NewGroupScope( + []string{"posthog_7", ""}, + []string{"posthog.events", "posthog.persons", "legacy.hits"}, + ) + if !scope.Schemas["posthog_7"] { + t.Error("posthog_7 must be a whole-schema grant") + } + if scope.Schemas[""] { + t.Error("blank schema names must be dropped") + } + for _, r := range []string{"posthog.events", "posthog.persons", "legacy.hits"} { + if !scope.Relations[r] { + t.Errorf("relation %s missing", r) + } + } + for _, s := range []string{"posthog", "legacy"} { + if !scope.RelationSchemas[s] { + t.Errorf("relation schema %s missing", s) + } + } + if len(scope.RelationSchemas) != 2 { + t.Errorf("relation_schemas = %v, want exactly the two relation schemas", scope.RelationSchemas) + } +} + +// A relation that is not "." is dropped rather than stored: as +// a key no decision can ever match it would read like a working grant and +// silently be none. +func TestNewGroupScopeDropsMalformedRelations(t *testing.T) { + scope := NewGroupScope(nil, []string{"events", "a.b.c", ".events", "posthog.", "", "ok.tbl"}) + if len(scope.Relations) != 1 || !scope.Relations["ok.tbl"] { + t.Errorf("relations = %v, want only ok.tbl", scope.Relations) + } + if len(scope.RelationSchemas) != 1 || !scope.RelationSchemas["ok"] { + t.Errorf("relation_schemas = %v, want only ok", scope.RelationSchemas) + } +} diff --git a/controlplane/provisioner/opa/latency_test.go b/controlplane/provisioner/opa/latency_test.go index 9478c245..9232be21 100644 --- a/controlplane/provisioner/opa/latency_test.go +++ b/controlplane/provisioner/opa/latency_test.go @@ -46,7 +46,7 @@ func largeFixture(orgs int) GroupCatalogs { // decisions against it). func preparedLargeBundle(b interface{ Fatalf(string, ...interface{}) }, orgs int) rego.PreparedEvalQuery { ctx := context.Background() - data, err := buildDataDocument(largeFixture(orgs)) + data, err := buildDataDocument(largeFixture(orgs), nil) if err != nil { b.Fatalf("buildDataDocument: %v", err) } diff --git a/controlplane/provisioner/opa/policy.rego b/controlplane/provisioner/opa/policy.rego index 9ac1d58c..7268677a 100644 --- a/controlplane/provisioner/opa/policy.rego +++ b/controlplane/provisioner/opa/policy.rego @@ -270,6 +270,7 @@ writable_catalog(catalog) if { not is_admin tenant_owns_catalog(catalog) managed_catalog_name(catalog) + not holds_scoped_group } listable_catalog(catalog) if readable_catalog(catalog) @@ -284,6 +285,101 @@ listable_catalog(catalog) if { managed_catalog_name(catalog) } +# --------------------------------------------------------------------------- +# Project scopes: narrowing a group to part of the catalog it owns. +# +# A duckgres login can be bound to ONE project (team), in which case it reads +# only that project's schemas rather than the whole org catalog. The control +# plane projects such a login into a `scope__team_` group, grants +# that group the org's catalog in data.group_catalogs exactly like an unscoped +# group, and ADDITIONALLY publishes a scope document for it under +# data.group_scopes. +# +# The layering is deliberate and load-bearing: a scope only ever REMOVES +# access. Every schema and table decision below still requires a group that +# owns the catalog in data.group_catalogs, so the cross-tenant boundary is the +# same rule it has always been, and a bug anywhere in this section can widen +# access only WITHIN the org's own catalog -- never across tenants. That is +# the property to preserve if these rules are ever restructured. +# +# Shape (every set is an object with value true so lookups stay O(1)): +# +# data.group_scopes[g].schemas[] whole schema readable +# data.group_scopes[g].relations[".
"] one table readable +# data.group_scopes[g].relation_schemas[] a schema that appears in +# `relations`, precomputed +# so schema-level decisions +# stay O(1) instead of +# scanning `relations` +# +# A group with NO document under data.group_scopes is unscoped and sees the +# whole catalog -- which is what every org's `org_` group is, so the +# unscoped tenant path is unchanged. +# +# Scoped identities get NO write authority (see writable_catalog): duckgres +# has a read-only project login and a read/write one, and only the read-only +# half is expressible here today. Denying writes to both is a narrowing of the +# read/write login, never a widening of the read-only one. +# --------------------------------------------------------------------------- + +# The requester's own groups that own `catalog`. Both branches below draw +# their group from this set, so neither can authorize a catalog that no group +# of the requester's owns. +granting_groups(catalog) := {g | + some g in input.context.identity.groups + g != admin_group + g != observer_group + data.group_catalogs[g][catalog] == true +} + +# A group is scoped iff the bundle carries a scope document for it. +scoped_group(g) if data.group_scopes[g] + +# holds_scoped_group: the requester is in at least one project-scoped group. +# Used to deny write authority outright rather than per-object. +holds_scoped_group if { + some g in input.context.identity.groups + scoped_group(g) +} + +# readable_schema / readable_table are the scope-aware counterparts of +# readable_catalog. An unscoped granting group allows everything in its +# catalog; a scoped one allows only what its document names. + +readable_schema(catalog, _) if admin_bundle_catalog(catalog) + +readable_schema(catalog, _) if { + some g in granting_groups(catalog) + not scoped_group(g) +} + +readable_schema(catalog, schema) if { + some g in granting_groups(catalog) + data.group_scopes[g].schemas[schema] == true +} + +readable_schema(catalog, schema) if { + some g in granting_groups(catalog) + data.group_scopes[g].relation_schemas[schema] == true +} + +readable_table(catalog, _, _) if admin_bundle_catalog(catalog) + +readable_table(catalog, _, _) if { + some g in granting_groups(catalog) + not scoped_group(g) +} + +readable_table(catalog, schema, _) if { + some g in granting_groups(catalog) + data.group_scopes[g].schemas[schema] == true +} + +readable_table(catalog, schema, table) if { + some g in granting_groups(catalog) + data.group_scopes[g].relations[concat(".", [schema, table])] == true +} + # --------------------------------------------------------------------------- # Catalog-scope decisions. # --------------------------------------------------------------------------- @@ -319,12 +415,18 @@ allow if { allow if { input.action.operation == "FilterSchemas" - readable_catalog(input.action.resource.schema.catalogName) + readable_schema( + input.action.resource.schema.catalogName, + input.action.resource.schema.schemaName, + ) } allow if { input.action.operation == "ShowTables" - readable_catalog(input.action.resource.schema.catalogName) + readable_schema( + input.action.resource.schema.catalogName, + input.action.resource.schema.schemaName, + ) } # DuckLake schema DDL. Rename checks BOTH resource and targetResource even @@ -350,22 +452,38 @@ allow if { allow if { input.action.operation == "SelectFromColumns" - readable_catalog(input.action.resource.table.catalogName) + readable_table( + input.action.resource.table.catalogName, + input.action.resource.table.schemaName, + input.action.resource.table.tableName, + ) } allow if { input.action.operation == "FilterTables" - readable_catalog(input.action.resource.table.catalogName) + readable_table( + input.action.resource.table.catalogName, + input.action.resource.table.schemaName, + input.action.resource.table.tableName, + ) } allow if { input.action.operation == "ShowColumns" - readable_catalog(input.action.resource.table.catalogName) + readable_table( + input.action.resource.table.catalogName, + input.action.resource.table.schemaName, + input.action.resource.table.tableName, + ) } allow if { input.action.operation == "FilterColumns" - readable_catalog(input.action.resource.table.catalogName) + readable_table( + input.action.resource.table.catalogName, + input.action.resource.table.schemaName, + input.action.resource.table.tableName, + ) } # DuckLake table and view DDL/DML. MERGE and CTAS are composed by Trino from @@ -444,13 +562,20 @@ batch contains i if { batch contains i if { some i input.action.operation == "FilterSchemas" - readable_catalog(input.action.filterResources[i].schema.catalogName) + readable_schema( + input.action.filterResources[i].schema.catalogName, + input.action.filterResources[i].schema.schemaName, + ) } batch contains i if { some i input.action.operation == "FilterTables" - readable_catalog(input.action.filterResources[i].table.catalogName) + readable_table( + input.action.filterResources[i].table.catalogName, + input.action.filterResources[i].table.schemaName, + input.action.filterResources[i].table.tableName, + ) } # FilterColumns is the one operation whose indices point into the candidate's @@ -459,7 +584,11 @@ batch contains i if { batch contains i if { input.action.operation == "FilterColumns" count(input.action.filterResources) == 1 - readable_catalog(input.action.filterResources[0].table.catalogName) + readable_table( + input.action.filterResources[0].table.catalogName, + input.action.filterResources[0].table.schemaName, + input.action.filterResources[0].table.tableName, + ) some i, _ in input.action.filterResources[0].table.columns } diff --git a/controlplane/provisioner/opa/policy_test.go b/controlplane/provisioner/opa/policy_test.go index d0356fe2..586f3429 100644 --- a/controlplane/provisioner/opa/policy_test.go +++ b/controlplane/provisioner/opa/policy_test.go @@ -16,9 +16,17 @@ import ( // table-driven tests below so we pay the compile cost once per test // binary, not once per case. func preparedPolicy(t *testing.T, gc GroupCatalogs) rego.PreparedEvalQuery { + t.Helper() + return preparedScopedPolicy(t, gc, nil) +} + +// preparedScopedPolicy is preparedPolicy with project scopes in the bundle. +// Separate entry point so every pre-scopes test keeps calling preparedPolicy +// and keeps asserting the unscoped behaviour verbatim. +func preparedScopedPolicy(t *testing.T, gc GroupCatalogs, gs GroupScopes) rego.PreparedEvalQuery { t.Helper() ctx := context.Background() - data, err := buildDataDocument(gc) + data, err := buildDataDocument(gc, gs) if err != nil { t.Fatalf("buildDataDocument: %v", err) } @@ -883,7 +891,13 @@ func TestIsolationMatrix(t *testing.T) { // preparedBatch compiles the policy for the batched entrypoint. func preparedBatch(t *testing.T, gc GroupCatalogs) rego.PreparedEvalQuery { t.Helper() - data, err := buildDataDocument(gc) + return preparedScopedBatch(t, gc, nil) +} + +// preparedScopedBatch is preparedBatch with project scopes in the bundle. +func preparedScopedBatch(t *testing.T, gc GroupCatalogs, gs GroupScopes) rego.PreparedEvalQuery { + t.Helper() + data, err := buildDataDocument(gc, gs) if err != nil { t.Fatalf("buildDataDocument: %v", err) } @@ -1818,3 +1832,301 @@ func TestSystemNodesGrantIsObserverOnly(t *testing.T) { t.Error("claiming the observer group without the observer username must grant nothing") } } + +// -------------------------------------------------------------------------- +// Project scopes. +// +// A project-scoped login is in `scope__team_`, which owns the SAME +// catalog the org group owns and additionally carries a scope document. The +// property every test here defends is that the scope only ever SUBTRACTS: it +// cannot reach another tenant, and removing the scope document must restore +// exactly the unscoped behaviour. +// -------------------------------------------------------------------------- + +// scopedFixture is one org with an unscoped group and a project-scoped group, +// both owning org_acme. Team 7 holds schema posthog_7 whole, plus the single +// relation posthog.events out of the shared legacy schema. +func scopedFixture() (GroupCatalogs, GroupScopes) { + gc := GroupCatalogs{ + "org_acme": {"org_acme": true}, + "scope_acme_team_7": {"org_acme": true}, + "org_other": {"org_other": true}, + } + gs := GroupScopes{ + "scope_acme_team_7": NewGroupScope( + []string{"posthog_7", "posthog_7_data_imports"}, + []string{"posthog.events"}, + ), + } + return gc, gs +} + +func scopedIdentity() map[string]interface{} { + return map[string]interface{}{ + "identity": map[string]interface{}{ + "user": "acme.posthog_team_7", + "groups": []interface{}{"scope_acme_team_7", "tier_free"}, + }, + } +} + +func tableInput(op, catalog, schema, table string, ctx map[string]interface{}) map[string]interface{} { + return map[string]interface{}{ + "context": ctx, + "action": map[string]interface{}{ + "operation": op, + "resource": map[string]interface{}{ + "table": map[string]interface{}{ + "catalogName": catalog, + "schemaName": schema, + "tableName": table, + }, + }, + }, + } +} + +func schemaInput(op, catalog, schema string, ctx map[string]interface{}) map[string]interface{} { + return map[string]interface{}{ + "context": ctx, + "action": map[string]interface{}{ + "operation": op, + "resource": map[string]interface{}{ + "schema": map[string]interface{}{ + "catalogName": catalog, + "schemaName": schema, + }, + }, + }, + } +} + +// A scoped login reads its own project's schemas and nothing else in the very +// same catalog. This is the whole point of the feature: without it a project +// login projected into Trino would see every other project's data. +func TestScopedGroupReadsOnlyItsOwnSchemas(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + for _, tc := range []struct { + name string + schema string + table string + want bool + }{ + {"own schema", "posthog_7", "events", true}, + {"own imports schema", "posthog_7_data_imports", "stripe_charges", true}, + {"another project's schema", "posthog_9", "events", false}, + {"a shared schema it holds no grant in", "public", "anything", false}, + } { + t.Run(tc.name, func(t *testing.T) { + for _, op := range []string{"SelectFromColumns", "FilterTables", "ShowColumns"} { + got := evalAllow(t, q, tableInput(op, "org_acme", tc.schema, tc.table, scopedIdentity())) + if got != tc.want { + t.Errorf("%s on %s.%s = %v, want %v", op, tc.schema, tc.table, got, tc.want) + } + } + }) + } +} + +// An individually granted relation is readable, and its SIBLINGS in the same +// schema are not. duckgres grants a project the shared legacy `posthog` +// schema one table at a time precisely because the schema holds every other +// project's tables too, so a grant that leaked to the whole schema would be a +// cross-project read. +func TestScopedGroupRelationGrantDoesNotLeakItsSchema(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + if !evalAllow(t, q, tableInput("SelectFromColumns", "org_acme", "posthog", "events", scopedIdentity())) { + t.Error("granted relation posthog.events must be readable") + } + if evalAllow(t, q, tableInput("SelectFromColumns", "org_acme", "posthog", "persons", scopedIdentity())) { + t.Error("posthog.persons was NOT granted and must not be readable") + } +} + +// The schema holding a granted relation must be visible at schema level, or +// the client can never navigate to the table it is allowed to read. +func TestScopedGroupSeesTheSchemaOfAGrantedRelation(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + for _, op := range []string{"FilterSchemas", "ShowTables"} { + if !evalAllow(t, q, schemaInput(op, "org_acme", "posthog", scopedIdentity())) { + t.Errorf("%s on the schema of a granted relation must be allowed", op) + } + if !evalAllow(t, q, schemaInput(op, "org_acme", "posthog_7", scopedIdentity())) { + t.Errorf("%s on a wholly granted schema must be allowed", op) + } + if evalAllow(t, q, schemaInput(op, "org_acme", "posthog_9", scopedIdentity())) { + t.Errorf("%s on another project's schema must be denied", op) + } + } +} + +// The cross-tenant boundary is unchanged for a scoped login: its scope names +// schemas, and a schema name says nothing about which catalog it is in. A +// scope group that owns only org_acme must not reach org_other even for a +// schema name its own scope happens to list. +func TestScopedGroupStillCannotCrossTenants(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + for _, op := range []string{"SelectFromColumns", "FilterTables", "ShowColumns"} { + if evalAllow(t, q, tableInput(op, "org_other", "posthog_7", "events", scopedIdentity())) { + t.Errorf("%s reached another tenant's catalog", op) + } + } + if evalAllow(t, q, schemaInput("FilterSchemas", "org_other", "posthog_7", scopedIdentity())) { + t.Error("FilterSchemas reached another tenant's catalog") + } + catalogInput := map[string]interface{}{ + "context": scopedIdentity(), + "action": map[string]interface{}{ + "operation": "AccessCatalog", + "resource": map[string]interface{}{"catalog": map[string]interface{}{"name": "org_other"}}, + }, + } + if evalAllow(t, q, catalogInput) { + t.Error("AccessCatalog reached another tenant's catalog") + } +} + +// A scoped login still needs the catalog itself, or it cannot run any query +// at all against the schemas it IS allowed to read. +func TestScopedGroupCanAccessItsOwnCatalog(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + for _, op := range []string{"AccessCatalog", "FilterCatalogs", "ShowSchemas"} { + in := map[string]interface{}{ + "context": scopedIdentity(), + "action": map[string]interface{}{ + "operation": op, + "resource": map[string]interface{}{"catalog": map[string]interface{}{"name": "org_acme"}}, + }, + } + if !evalAllow(t, q, in) { + t.Errorf("%s on its own catalog must be allowed", op) + } + } +} + +// Scoped logins get no write authority anywhere, including inside the schemas +// they can read. duckgres has a read-only project login and a read/write one; +// only the read-only half is expressible here today, so both are read-only in +// Trino. That is a narrowing of the read/write login and never a widening of +// the read-only one -- if this test starts failing because writes were added +// for project_user, the scope must gate them per-schema, not per-catalog. +func TestScopedGroupHasNoWriteAuthority(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + for _, op := range []string{"CreateSchema", "DropSchema"} { + if evalAllow(t, q, schemaInput(op, "org_acme", "posthog_7", scopedIdentity())) { + t.Errorf("%s must be denied to a scoped login", op) + } + } + for _, op := range []string{"CreateTable", "DropTable", "InsertIntoTable", "DeleteFromTable", "UpdateTableColumns"} { + if evalAllow(t, q, tableInput(op, "org_acme", "posthog_7", "events", scopedIdentity())) { + t.Errorf("%s must be denied to a scoped login", op) + } + } +} + +// The unscoped org login is untouched by any of the above: it reads every +// schema in its catalog and keeps its write authority. This is the +// regression guard for every tenant that has no project logins at all. +func TestUnscopedGroupIsUnaffectedByScopesInTheBundle(t *testing.T) { + gc, gs := scopedFixture() + q := preparedScopedPolicy(t, gc, gs) + + unscoped := map[string]interface{}{ + "identity": map[string]interface{}{ + "user": "acme", + "groups": []interface{}{"org_acme", "tier_free"}, + }, + } + for _, schema := range []string{"posthog_7", "posthog_9", "public"} { + if !evalAllow(t, q, tableInput("SelectFromColumns", "org_acme", schema, "events", unscoped)) { + t.Errorf("unscoped login must read %s", schema) + } + if !evalAllow(t, q, schemaInput("FilterSchemas", "org_acme", schema, unscoped)) { + t.Errorf("unscoped login must see %s", schema) + } + } + if !evalAllow(t, q, schemaInput("CreateSchema", "org_acme", "whatever", unscoped)) { + t.Error("unscoped login must keep its write authority") + } + if evalAllow(t, q, tableInput("SelectFromColumns", "org_other", "posthog_7", "events", unscoped)) { + t.Error("unscoped login must not cross tenants") + } +} + +// A scope document with no readable namespace is the fail-closed shape +// configstore produces for a missing or disabled team. It must read NOTHING +// rather than degrading into unscoped access. +func TestEmptyScopeReadsNothing(t *testing.T) { + gc := GroupCatalogs{"scope_acme_team_7": {"org_acme": true}} + gs := GroupScopes{"scope_acme_team_7": NewGroupScope(nil, nil)} + q := preparedScopedPolicy(t, gc, gs) + + if evalAllow(t, q, tableInput("SelectFromColumns", "org_acme", "posthog_7", "events", scopedIdentity())) { + t.Error("an empty scope must read no table") + } + if evalAllow(t, q, schemaInput("FilterSchemas", "org_acme", "posthog_7", scopedIdentity())) { + t.Error("an empty scope must see no schema") + } +} + +// Batched filtering must answer identically to the non-batched path for +// scoped groups too, candidate by candidate -- the same invariant +// TestBatchedFilteringMatchesNonBatched pins for the unscoped path. The two +// entrypoints dispatch separately, so a scope rule added to one and not the +// other is exactly the drift this catches. +func TestBatchedFilteringMatchesNonBatchedForScopes(t *testing.T) { + gc, gs := scopedFixture() + single := preparedScopedPolicy(t, gc, gs) + batched := preparedScopedBatch(t, gc, gs) + ctx := context.Background() + + schemas := []string{"posthog_7", "posthog_9", "posthog", "public"} + resources := make([]interface{}, 0, len(schemas)) + for _, s := range schemas { + resources = append(resources, map[string]interface{}{ + "schema": map[string]interface{}{"catalogName": "org_acme", "schemaName": s}, + }) + } + rs, err := batched.Eval(ctx, rego.EvalInput(map[string]interface{}{ + "context": scopedIdentity(), + "action": map[string]interface{}{ + "operation": "FilterSchemas", + "filterResources": resources, + }, + })) + if err != nil { + t.Fatalf("Eval batch: %v", err) + } + allowed := map[int]bool{} + if len(rs) > 0 { + for _, v := range rs[0].Expressions[0].Value.([]interface{}) { + n, ok := v.(json.Number) + if !ok { + t.Fatalf("expected numeric index, got %T (%v)", v, v) + } + i, err := n.Int64() + if err != nil { + t.Fatalf("index %v: %v", v, err) + } + allowed[int(i)] = true + } + } + for i, s := range schemas { + want := evalAllow(t, single, schemaInput("FilterSchemas", "org_acme", s, scopedIdentity())) + if allowed[i] != want { + t.Errorf("schema %s: batch=%v single=%v", s, allowed[i], want) + } + } +} diff --git a/controlplane/provisioner/opa/types.go b/controlplane/provisioner/opa/types.go index 94ff7557..4257c747 100644 --- a/controlplane/provisioner/opa/types.go +++ b/controlplane/provisioner/opa/types.go @@ -28,6 +28,8 @@ // per-user and require a bundle-shape migration during the OIDC rollout. package opa +import "strings" + // GroupCatalogs maps a Trino group name (e.g. `org_` for customer // orgs, where `org` is the sanitized Org.Name; or the admin group for // the provisioner's smoke-test access) to the set of catalog names that @@ -44,11 +46,71 @@ package opa // bounded iteration, still O(1) in catalog count. type GroupCatalogs map[string]map[string]bool +// GroupScope narrows one group to part of the catalog it owns. A group with +// no GroupScope is unscoped and reads the whole catalog; a group WITH one +// reads only what these sets name. The policy consults a scope only after the +// group has already been found to own the catalog in GroupCatalogs, so a +// scope can subtract access but never add any -- in particular it can never +// reach another tenant's catalog. +// +// Each field is a set represented as map[string]bool with the value always +// true, for the same O(1)-lookup reason GroupCatalogs is (see above): the +// policy indexes into these objects rather than scanning them. +type GroupScope struct { + // Schemas are readable in full: every table in them is allowed. + Schemas map[string]bool `json:"schemas"` + // Relations are individually readable tables, keyed ".
", + // for schemas the group does NOT hold in full. duckgres grants these for + // a project's tables that live in the shared legacy `posthog` schema. + Relations map[string]bool `json:"relations"` + // RelationSchemas is the set of schema names appearing in Relations, + // precomputed so a schema-level decision (FilterSchemas, ShowTables) is + // an object lookup rather than a scan over Relations. Derived data -- + // build it with NewGroupScope rather than by hand, so it cannot drift + // from Relations and silently hide a schema the group can read a table + // in. + RelationSchemas map[string]bool `json:"relation_schemas"` +} + +// GroupScopes maps a Trino group name to the scope narrowing it. Only +// project-scoped groups appear; the absence of a key means "unscoped", which +// is what every org's own `org_` group is. +type GroupScopes map[string]GroupScope + +// NewGroupScope builds a GroupScope from the allowed-schema and +// allowed-relation lists duckgres derives for a project login, deriving +// RelationSchemas from relations so the two cannot disagree. +// +// A relation that is not ".
" is dropped rather than guessed +// at: it would otherwise land in the policy as a key no decision can ever +// match, which reads as a working grant and is not one. +func NewGroupScope(schemas, relations []string) GroupScope { + scope := GroupScope{ + Schemas: map[string]bool{}, + Relations: map[string]bool{}, + RelationSchemas: map[string]bool{}, + } + for _, s := range schemas { + if s != "" { + scope.Schemas[s] = true + } + } + for _, r := range relations { + schema, table, ok := strings.Cut(r, ".") + if !ok || schema == "" || table == "" || strings.Contains(table, ".") { + continue + } + scope.Relations[r] = true + scope.RelationSchemas[schema] = true + } + return scope +} + // BundleBuilder builds an OPA bundle (gzip'd tarball per OPA's bundle spec) // from a GroupCatalogs input. The returned bytes are suitable for serving // from a bundle endpoint or POSTing through OPA's bundle service API. type BundleBuilder interface { - BuildBundle(gc GroupCatalogs) ([]byte, error) + BuildBundle(gc GroupCatalogs, gs GroupScopes) ([]byte, error) } // AdminPrincipal is the Trino username the provisioner authenticates as diff --git a/controlplane/provisioner/trino_provisioner.go b/controlplane/provisioner/trino_provisioner.go index 39b8830e..629cc059 100644 --- a/controlplane/provisioner/trino_provisioner.go +++ b/controlplane/provisioner/trino_provisioner.go @@ -225,6 +225,42 @@ func TrinoGroupName(principal string) string { return "org_" + trinoSanitize(principal) } +// trinoUsernamePattern is the grammar a duckgres username must satisfy to be +// projected into the cell's auth files. +// +// This is an ALLOWLIST, and it is a security control rather than a tidiness +// one. duckgres validates a username as little more than "not empty" (see +// controlplane/validation.go), while password.db is `:` per line +// and group.db is `:,` per line. A username holding `:`, +// `,` or a newline would not merely render oddly -- it would let whoever can +// create org users append arbitrary lines to those files, including a line +// for the admin principal. Anything outside this grammar is therefore never +// written, and no amount of downstream escaping is relied on. +// +// `.` is excluded as well, so that `.` carries exactly the one +// separator TrinoPrincipalSeparator puts there and the org prefix stays +// recoverable by the resource-group selector (see orgCaptureRegex). +var trinoUsernamePattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_-]*$`) + +// projectableTrinoUsername reports whether a duckgres username is safe to +// render into password.db / group.db. +func projectableTrinoUsername(username string) bool { + return len(username) <= 255 && trinoUsernamePattern.MatchString(username) +} + +// TrinoScopeGroupName returns the group label for a project-scoped login: +// one group per (org, team), carrying that team's schema scope in the OPA +// bundle. +// +// The `scope_` prefix keeps these out of TrinoGroupName's `org_` space +// and TrinoTierGroupName's `tier_` space. That separation matters: a group in +// the `org_` space that the bundle happens not to scope reads the whole +// catalog, so a scope group whose name could collide with an org group would +// be a silent widening rather than a name clash. +func TrinoScopeGroupName(principal string, teamID int64) string { + return fmt.Sprintf("scope_%s_team_%d", trinoSanitize(principal), teamID) +} + // TrinoResourceGroupName returns the resource-group selector key for // an org. Sanitized like the catalog name so a `.` in orgName doesn't // get re-interpreted as a hierarchy separator in Trino's resource- @@ -678,8 +714,8 @@ func (p *TrinoProvisioner) Reconcile(ctx context.Context) error { return nil } -// rejectPrincipalCollisions splits orgs into those safe to project and -// those whose Trino catalog name is not unique to them. +// rejectPrincipalCollisions splits orgs into those safe to project and those +// whose Trino catalog name, or whose Trino username, is not unique to them. // // Every Trino-facing name is trinoSanitize(principal), and sanitization is // injective over principals that satisfy ValidateDatabaseName — that grammar @@ -699,6 +735,19 @@ func (p *TrinoProvisioner) Reconcile(ctx context.Context) error { // the other also believes it owns. func rejectPrincipalCollisions(orgs []configstore.TrinoEnabledOrg) (projectable []configstore.TrinoEnabledOrg, collisions map[string]error) { byCatalog := make(map[string][]string, len(orgs)) + byPrincipal := make(map[string]map[string]bool, len(orgs)) + claim := func(principal, orgID string) { + if byPrincipal[principal] == nil { + byPrincipal[principal] = map[string]bool{} + } + byPrincipal[principal][orgID] = true + } + // The cell's own principals are claimed first, so a tenant that derives + // either name is treated as contesting it and is held back. Neither is + // reachable from a valid database_name, but the policy's whole admin + // conjunction rests on the name being the provisioner's alone. + claim(opa.AdminPrincipal, "") + claim(opa.ObserverPrincipal, "") for _, o := range orgs { principal := o.TrinoPrincipal() if principal == "" { @@ -708,6 +757,13 @@ func rejectPrincipalCollisions(orgs []configstore.TrinoEnabledOrg) (projectable } name := TrinoCatalogName(principal) byCatalog[name] = append(byCatalog[name], o.OrgID) + claim(principal, o.OrgID) + for _, u := range o.Users { + if !projectableTrinoUsername(u.Username) { + continue + } + claim(o.TrinoUserPrincipal(u.Username), o.OrgID) + } } contested := make(map[string]string, 0) // orgID -> catalog name @@ -719,15 +775,51 @@ func rejectPrincipalCollisions(orgs []configstore.TrinoEnabledOrg) (projectable contested[id] = name } } - if len(contested) == 0 { + // A Trino username claimed by two orgs is a cross-tenant authentication + // bug, not a cosmetic clash: password.db is one flat namespace per cell, + // so the duplicate line lets one org's user authenticate against the + // other's entry and land in the other's group. Valid database_names make + // this unreachable — they are DNS labels, so `.` splits at its + // only dot — but grandfathered rows predate that rule and may hold a dot, + // which is exactly how `acme.analytics` the org and `acme` + `analytics` + // the login come to claim one name. + contestedPrincipal := map[string]string{} // orgID -> principal + for principal, owners := range byPrincipal { + if len(owners) < 2 { + continue + } + for id := range owners { + if id == "" { + continue // the cell's own principal, not an org + } + contestedPrincipal[id] = principal + } + } + if len(contested) == 0 && len(contestedPrincipal) == 0 { return orgs, nil } - collisions = make(map[string]error, len(contested)) - projectable = make([]configstore.TrinoEnabledOrg, 0, len(orgs)-len(contested)) + collisions = make(map[string]error, len(contested)+len(contestedPrincipal)) + projectable = make([]configstore.TrinoEnabledOrg, 0, len(orgs)) for _, o := range orgs { name, bad := contested[o.OrgID] if !bad { + if principal, dup := contestedPrincipal[o.OrgID]; dup { + others := make([]string, 0, len(byPrincipal[principal])) + for id := range byPrincipal[principal] { + if id != o.OrgID { + others = append(others, orgLabel(id)) + } + } + sort.Strings(others) + collisions[o.OrgID] = fmt.Errorf( + "Trino username %q is also claimed by %s; refusing to project either — "+ + "rename the org's database_name or the colliding login so the usernames differ", + principal, strings.Join(others, ", ")) + slog.Error("Trino reconcile: refusing to project orgs whose Trino usernames collide.", + "org", o.OrgID, "principal", principal, "colliding_with", others) + continue + } projectable = append(projectable, o) continue } @@ -748,6 +840,16 @@ func rejectPrincipalCollisions(orgs []configstore.TrinoEnabledOrg) (projectable return projectable, collisions } +// orgLabel names a principal's claimant in an operator-facing message. The +// empty org id is the cell itself (see the AdminPrincipal/ObserverPrincipal +// claims in rejectPrincipalCollisions), which has no org row to name. +func orgLabel(orgID string) string { + if orgID == "" { + return "this Trino cell's own operational principals" + } + return "org " + orgID +} + // claimCellOrgs filters the fleet-wide Trino-enabled listing down to the // orgs this cell is responsible for, claiming any that have no cell yet. // @@ -1742,22 +1844,28 @@ type TrinoClusterPrincipals struct { // Format conventions: // // password.db: : -// One line per org. The principal is the org's +// Two kinds of tenant line. The org's own principal is its // database_name (see TrinoEnabledOrg.TrinoPrincipal), so the -// tenant's Trino username is the same name it uses for its -// DuckDB warehouse rather than a bare org UUID. Hash is -// copied through unchanged — it's already bcrypt in the -// configstore, and it is the SAME hash the DuckDB warehouse -// authenticates with, so one password works for both. +// tenant is known by the same name it uses for its DuckDB +// warehouse rather than a bare org UUID. Each of the org's +// duckgres logins additionally gets `.` +// (TrinoUserPrincipal). Hashes are copied through unchanged — +// they are already bcrypt in the configstore, and they are the +// SAME hashes pgwire authenticates with, so one password works +// on both engines and nothing has to be re-hashed or reset. // group.db: : // NOTE: this is the opposite direction from password.db. -// For v1 (one user per org) the value is the single -// principal. Easy to get backwards, hence this comment. +// Easy to get backwards, hence this comment. // -// Orgs without a RootPasswordHash or a principal are skipped silently -// (the listing query already filters to (org, root-user) pairs with a -// non-blank database_name, so this is just defensive against future -// changes). +// An org's unscoped principals share its `org_` group, +// which the OPA bundle grants the whole catalog. A +// project-scoped login goes into a `scope__team_` +// group INSTEAD — never both, because the org group is +// unscoped and membership in it would defeat the scope. +// +// Orgs without a principal are skipped entirely. An org with a principal but +// no RootPasswordHash still projects its per-user logins: the bare org +// principal is one credential among several now, not the only way in. // // cluster carries the bcrypt hashes for the two non-tenant principals. // Each is prepended to both files when non-empty, regardless of orgs — @@ -1790,14 +1898,56 @@ func BuildTrinoAuthFiles(orgs []configstore.TrinoEnabledOrg, cluster TrinoCluste } for _, o := range orgs { principal := o.TrinoPrincipal() - if o.RootPasswordHash == "" || principal == "" { + if principal == "" { continue } - pwLines = append(pwLines, fmt.Sprintf("%s:%s", principal, o.RootPasswordHash)) - // group_name first, comma-separated users second. For v1 this - // is one user per group (the principal only). - grpLines = append(grpLines, fmt.Sprintf("%s:%s", TrinoGroupName(principal), principal)) - tierMembers[normalizeTier(o.Tier)] = append(tierMembers[normalizeTier(o.Tier)], principal) + // The org's own principal: database_name authenticating with the + // root hash. Kept for service-to-service use and for clients + // configured before per-user logins existed. + var orgGroupMembers []string + if o.RootPasswordHash != "" { + pwLines = append(pwLines, fmt.Sprintf("%s:%s", principal, o.RootPasswordHash)) + orgGroupMembers = append(orgGroupMembers, principal) + tierMembers[normalizeTier(o.Tier)] = append(tierMembers[normalizeTier(o.Tier)], principal) + } + // Per-user logins. Each one authenticates as . with the + // very same bcrypt hash it uses on pgwire. + scopeMembers := map[string][]string{} + for _, u := range o.Users { + if u.PasswordHash == "" || !projectableTrinoUsername(u.Username) { + // An unprojectable username costs that ONE login its Trino + // access and nothing else. Holding the whole org back would + // turn one odd name into an org-wide outage. + if u.PasswordHash != "" { + slog.Warn("Trino: skipping login whose username cannot be rendered into the auth files.", + "org", o.OrgID, "user", u.Username) + } + continue + } + userPrincipal := o.TrinoUserPrincipal(u.Username) + pwLines = append(pwLines, fmt.Sprintf("%s:%s", userPrincipal, u.PasswordHash)) + // A scoped login joins its scope group INSTEAD of the org group: + // the org group is unscoped in the bundle, so putting a project + // login in it would hand it the whole catalog. + if group, ok := scopeGroupFor(o, u); ok { + scopeMembers[group] = append(scopeMembers[group], userPrincipal) + } else { + orgGroupMembers = append(orgGroupMembers, userPrincipal) + } + tierMembers[normalizeTier(o.Tier)] = append(tierMembers[normalizeTier(o.Tier)], userPrincipal) + } + // group_name first, comma-separated users second. NOTE this is the + // opposite direction from password.db; easy to get backwards. + if len(orgGroupMembers) > 0 { + sort.Strings(orgGroupMembers) + grpLines = append(grpLines, fmt.Sprintf("%s:%s", + TrinoGroupName(principal), strings.Join(orgGroupMembers, ","))) + } + for _, group := range sortedKeys(scopeMembers) { + members := scopeMembers[group] + sort.Strings(members) + grpLines = append(grpLines, fmt.Sprintf("%s:%s", group, strings.Join(members, ","))) + } } // Tier claims. These carry a tenant's tier to the resource-group // selectors, which match on userGroup — that is what keeps @@ -1824,6 +1974,35 @@ func BuildTrinoAuthFiles(orgs []configstore.TrinoEnabledOrg, cluster TrinoCluste return strings.Join(pwLines, "\n"), strings.Join(grpLines, "\n") } +// scopeGroupFor returns the scope group a login belongs in, and whether it is +// scoped at all. A login is scoped iff the config store resolved a project +// policy for it AND that policy names a team, which is what the group is +// keyed on. +// +// A scoped login whose policy resolved to NO readable namespace still gets a +// group — an empty scope in the bundle, which reads nothing. That is the +// fail-closed shape configstore produces for a team that is missing or +// disabled, and it must survive the trip rather than degrading into "no scope +// group", which would put the login in the unscoped org group. +func scopeGroupFor(o configstore.TrinoEnabledOrg, u configstore.TrinoOrgUser) (string, bool) { + if u.Scope == nil || u.TeamID == nil { + return "", false + } + return TrinoScopeGroupName(o.TrinoPrincipal(), *u.TeamID), true +} + +// sortedKeys returns a map's keys in sorted order, so every projection this +// file writes is byte-stable across ticks (an unstable file would rewrite the +// Secret every reconcile and re-trigger every coordinator's file refresh). +func sortedKeys[V any](m map[string]V) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + // reconcileResourceGroups projects resource-groups.json into the // trino-resource-groups ConfigMap. // @@ -1911,9 +2090,21 @@ const ( // selector's named capture; orgCaptureRegex is the capture that fills it. // Together they let one templated node serve every tenant, which is what // keeps this file free of tenant names — see BuildTrinoResourceGroups. +// +// The capture stops at the first `.` because a tenant principal is either the +// org's bare database_name (`acme`) or one of its per-user logins +// (`acme.analyst`, see configstore.TrinoUserPrincipal), and BOTH must resolve +// to the SAME leaf resource group. A `(?.*)` capture -- what this was +// before per-user logins -- matches the whole username, so every user would +// get a private leaf carrying the full per-tenant limits, and an org with ten +// logins would quietly hold ten times its concurrency and memory budget. The +// selector is matched with Pattern.matcher(user).matches(), i.e. a full +// match, so the trailing group is required for qualified names to match at +// all; TestBuildTrinoResourceGroups_CapturesOrgFromQualifiedUsername pins +// both shapes. const ( orgTemplateVariable = "${org}" - orgCaptureRegex = "(?.*)" + orgCaptureRegex = `(?[^.]+)(?:\..*)?` ) // TrinoTierGroupName is the group.db claim that puts an org in a tier lane. @@ -2116,11 +2307,19 @@ func BuildTrinoResourceGroups() ([]byte, error) { // managed catalog so the provisioner's own SHOW CATALOGS idempotency // check (run as opa.AdminPrincipal) is allowed. // +// Project-scoped logins add a second kind of group, `scope__team_`, +// which owns exactly the same catalog its org group does and additionally +// carries a GroupScope. That layering is what keeps this change off the +// tenant-isolation path: the catalog grant is the same grant, and the scope +// can only subtract from it (see the "Project scopes" section of +// policy.rego). +// // ctx is currently unused (the builder is pure and the store Set is // in-memory), but kept on the signature for parity with the other // reconcile* steps and to permit instrumented builders later. func (p *TrinoProvisioner) reconcileOPABundle(_ context.Context, orgs []configstore.TrinoEnabledOrg) error { gc := make(opa.GroupCatalogs, len(orgs)+1) + gs := opa.GroupScopes{} adminCatalogs := make(map[string]bool, len(orgs)) for _, o := range orgs { principal := o.TrinoPrincipal() @@ -2130,6 +2329,18 @@ func (p *TrinoProvisioner) reconcileOPABundle(_ context.Context, orgs []configst catalog := TrinoCatalogName(principal) gc[TrinoGroupName(principal)] = map[string]bool{catalog: true} adminCatalogs[catalog] = true + // A project-scoped login sits in its own group, which owns the SAME + // catalog — the cross-tenant check is unchanged for it — and carries + // a scope document that narrows it to that project's schemas. Groups + // are per (org, team), so several logins on one team share one entry. + for _, u := range o.Users { + group, scoped := scopeGroupFor(o, u) + if !scoped || !projectableTrinoUsername(u.Username) || u.PasswordHash == "" { + continue + } + gc[group] = map[string]bool{catalog: true} + gs[group] = opa.NewGroupScope(u.Scope.AllowedSchemas, u.Scope.AllowedRelations) + } } if len(adminCatalogs) > 0 { // Admin owns every managed catalog so SHOW CATALOGS / catalog @@ -2138,7 +2349,7 @@ func (p *TrinoProvisioner) reconcileOPABundle(_ context.Context, orgs []configst // docstring). gc[opa.AdminGroup] = adminCatalogs } - bundle, err := p.bundleBuilder.BuildBundle(gc) + bundle, err := p.bundleBuilder.BuildBundle(gc, gs) if err != nil { return fmt.Errorf("build opa bundle: %w", err) } diff --git a/controlplane/provisioner/trino_provisioner_test.go b/controlplane/provisioner/trino_provisioner_test.go index 79be62dd..ed5ab5ea 100644 --- a/controlplane/provisioner/trino_provisioner_test.go +++ b/controlplane/provisioner/trino_provisioner_test.go @@ -747,13 +747,17 @@ type testProvisionerHarness struct { // capturingBundleBuilder is a pass-through opa.BundleBuilder that // remembers its last input. type capturingBundleBuilder struct { - inner opa.BundleBuilder - last opa.GroupCatalogs + inner opa.BundleBuilder + last opa.GroupCatalogs + lastCatalogs opa.GroupCatalogs + lastScopes opa.GroupScopes } -func (c *capturingBundleBuilder) BuildBundle(gc opa.GroupCatalogs) ([]byte, error) { +func (c *capturingBundleBuilder) BuildBundle(gc opa.GroupCatalogs, gs opa.GroupScopes) ([]byte, error) { c.last = gc - return c.inner.BuildBundle(gc) + c.lastCatalogs = gc + c.lastScopes = gs + return c.inner.BuildBundle(gc, gs) } const testCellID = "cell-test" @@ -1871,3 +1875,293 @@ func TestCatalogHTTPClientTagsItsSource(t *testing.T) { } } } + +// --- per-user logins --- + +func teamID(id int64) *int64 { return &id } + +// The point of the feature: an org's OWN duckgres logins each authenticate to +// Trino, under ., with the very same bcrypt hash +// they use on pgwire. The bare org principal survives alongside them, so +// anything configured before per-user logins existed keeps working. +func TestBuildTrinoAuthFiles_ProjectsEveryOrgUser(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "42", + DatabaseName: "acme", + RootPasswordHash: "$2a$10$roothash", + Users: []configstore.TrinoOrgUser{ + {Username: "root", PasswordHash: "$2a$10$roothash"}, + {Username: "analyst", PasswordHash: "$2a$10$analysthash"}, + }, + }} + pw, grp := BuildTrinoAuthFiles(orgs, TrinoClusterPrincipals{}) + + wantPW := "acme:$2a$10$roothash\n" + + "acme.root:$2a$10$roothash\n" + + "acme.analyst:$2a$10$analysthash\n" + if pw != wantPW { + t.Errorf("password.db =\n%q\nwant\n%q", pw, wantPW) + } + // All three principals share the org group, which the bundle grants the + // whole catalog, and all three carry the tier claim that routes them to + // the org's resource group. + wantGrp := "org_acme:acme,acme.analyst,acme.root\n" + + "tier_free:acme,acme.analyst,acme.root\n" + if grp != wantGrp { + t.Errorf("group.db =\n%q\nwant\n%q", grp, wantGrp) + } +} + +// A project-scoped login joins its scope group and NOT the org group. The org +// group is unscoped in the bundle, so membership in it would hand the login +// the whole catalog and defeat the scope entirely. +func TestBuildTrinoAuthFiles_ScopedUserJoinsOnlyItsScopeGroup(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "42", + DatabaseName: "acme", + RootPasswordHash: "$2a$10$roothash", + Users: []configstore.TrinoOrgUser{ + {Username: "analyst", PasswordHash: "$2a$10$analysthash"}, + { + Username: "posthog_team_7", + PasswordHash: "$2a$10$teamhash", + TeamID: teamID(7), + Scope: &configstore.OrgUserQueryAccess{ReadOnly: true, AllowedSchemas: []string{"posthog_7"}}, + }, + }, + }} + _, grp := BuildTrinoAuthFiles(orgs, TrinoClusterPrincipals{}) + + wantGrp := "org_acme:acme,acme.analyst\n" + + "scope_acme_team_7:acme.posthog_team_7\n" + + "tier_free:acme,acme.analyst,acme.posthog_team_7\n" + if grp != wantGrp { + t.Errorf("group.db =\n%q\nwant\n%q", grp, wantGrp) + } + if strings.Contains(grp, "org_acme:acme,acme.analyst,acme.posthog_team_7") { + t.Error("the scoped login must NOT be in the unscoped org group") + } +} + +// Several logins on one team share one scope group — the group is keyed on +// (org, team), not on the user. +func TestBuildTrinoAuthFiles_ScopeGroupIsSharedPerTeam(t *testing.T) { + scope := &configstore.OrgUserQueryAccess{ReadOnly: true, AllowedSchemas: []string{"posthog_7"}} + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "42", + DatabaseName: "acme", + Users: []configstore.TrinoOrgUser{ + {Username: "reader", PasswordHash: "$2a$10$a", TeamID: teamID(7), Scope: scope}, + {Username: "writer", PasswordHash: "$2a$10$b", TeamID: teamID(7), Scope: scope}, + }, + }} + _, grp := BuildTrinoAuthFiles(orgs, TrinoClusterPrincipals{}) + if want := "scope_acme_team_7:acme.reader,acme.writer\n" + + "tier_free:acme.reader,acme.writer\n"; grp != want { + t.Errorf("group.db =\n%q\nwant\n%q", grp, want) + } +} + +// duckgres validates a username as little more than "not empty", while +// password.db is `:` per line and group.db is +// `:,`. A username carrying `:`, `,` or a newline would +// let whoever can create org users append arbitrary lines to those files — +// including a line for the admin principal. The grammar is an allowlist, so +// such a row is never rendered at all. +func TestBuildTrinoAuthFiles_RefusesUsernamesThatCouldInjectLines(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "42", + DatabaseName: "acme", + RootPasswordHash: "$2a$10$roothash", + Users: []configstore.TrinoOrgUser{ + {Username: "ok", PasswordHash: "$2a$10$okhash"}, + {Username: "evil\n__admin_provisioner", PasswordHash: "$2a$10$attacker"}, + {Username: "has:colon", PasswordHash: "$2a$10$x"}, + {Username: "has,comma", PasswordHash: "$2a$10$x"}, + {Username: "has space", PasswordHash: "$2a$10$x"}, + {Username: "has.dot", PasswordHash: "$2a$10$x"}, + {Username: "", PasswordHash: "$2a$10$x"}, + }, + }} + pw, grp := BuildTrinoAuthFiles(orgs, TrinoClusterPrincipals{}) + + if want := "acme:$2a$10$roothash\nacme.ok:$2a$10$okhash\n"; pw != want { + t.Errorf("password.db =\n%q\nwant\n%q", pw, want) + } + if strings.Contains(pw, "$2a$10$attacker") { + t.Error("an injected admin line reached password.db") + } + for _, bad := range []string{"has:colon", "has,comma", "has space", "has.dot"} { + if strings.Contains(pw, bad) || strings.Contains(grp, bad) { + t.Errorf("username %q must not be projected", bad) + } + } + // Every rendered line must still be exactly one `user:hash` pair. + for _, line := range strings.Split(strings.TrimSuffix(pw, "\n"), "\n") { + if strings.Count(line, ":") != 1 { + t.Errorf("password.db line %q is not a single user:hash pair", line) + } + } +} + +// An org with per-user logins but no root hash still projects those logins: +// the bare org principal is one credential among several now, not the only +// way in. +func TestBuildTrinoAuthFiles_ProjectsUsersWithoutARootHash(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "42", + DatabaseName: "acme", + Users: []configstore.TrinoOrgUser{{Username: "analyst", PasswordHash: "$2a$10$a"}}, + }} + pw, grp := BuildTrinoAuthFiles(orgs, TrinoClusterPrincipals{}) + if want := "acme.analyst:$2a$10$a\n"; pw != want { + t.Errorf("password.db = %q, want %q", pw, want) + } + if want := "org_acme:acme.analyst\ntier_free:acme.analyst\n"; grp != want { + t.Errorf("group.db = %q, want %q", grp, want) + } +} + +// Two orgs deriving the same Trino username is a cross-tenant authentication +// bug: password.db is one flat namespace per cell, so the duplicate line lets +// one org's user authenticate against the other's entry. Valid database_names +// make it unreachable, but grandfathered rows may hold a dot — which is how +// org `acme.analyst` and org `acme` + login `analyst` come to claim one name. +func TestRejectPrincipalCollisions_HoldsBackOrgsSharingATrinoUsername(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{ + {OrgID: "1", DatabaseName: "acme.analyst", RootPasswordHash: "$2a$10$a"}, + { + OrgID: "2", + DatabaseName: "acme", + RootPasswordHash: "$2a$10$b", + Users: []configstore.TrinoOrgUser{{Username: "analyst", PasswordHash: "$2a$10$c"}}, + }, + } + projectable, collisions := rejectPrincipalCollisions(orgs) + if len(projectable) != 0 { + t.Errorf("both orgs must be held back, got %d projectable", len(projectable)) + } + for _, id := range []string{"1", "2"} { + if collisions[id] == nil { + t.Errorf("org %s must be reported as colliding", id) + } + } +} + +// A tenant that derives one of the cell's own principals must be held back: +// the OPA policy's admin authority rests on that username belonging to the +// provisioner alone. +func TestRejectPrincipalCollisions_HoldsBackATenantClaimingAnOperationalPrincipal(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "1", + DatabaseName: opa.AdminPrincipal, + RootPasswordHash: "$2a$10$a", + }} + projectable, collisions := rejectPrincipalCollisions(orgs) + if len(projectable) != 0 { + t.Errorf("the org must be held back, got %d projectable", len(projectable)) + } + if collisions["1"] == nil { + t.Error("claiming the admin principal must be reported as a collision") + } +} + +// Orgs with no user rows at all must project exactly as they did before +// per-user logins existed. This is the regression guard for every tenant on +// the cell today. +func TestBuildTrinoAuthFiles_UnchangedForOrgsWithoutUsers(t *testing.T) { + orgs := []configstore.TrinoEnabledOrg{ + {OrgID: "42", DatabaseName: "db42", RootPasswordHash: "$2a$10$hash42"}, + {OrgID: "43", DatabaseName: "db43", RootPasswordHash: "$2a$10$hash43"}, + } + pw, grp := BuildTrinoAuthFiles(orgs, TrinoClusterPrincipals{}) + if want := "db42:$2a$10$hash42\ndb43:$2a$10$hash43\n"; pw != want { + t.Errorf("password.db = %q, want %q", pw, want) + } + if want := "org_db42:db42\norg_db43:db43\ntier_free:db42,db43\n"; grp != want { + t.Errorf("group.db = %q, want %q", grp, want) + } +} + +// The resource-group selector must map BOTH the bare org principal and every +// qualified per-user login onto the SAME leaf group. The previous +// `(?.*)` capture matched the whole username, which would give each user +// a private leaf carrying the full per-tenant limits — an org with ten logins +// would quietly hold ten times its concurrency and memory budget. +// +// The constant is a Java regex (Trino compiles it with java.util.regex) and +// Go spells named groups `(?P<...>`, so the test translates that one token +// and nothing else. Trino matches with Pattern.matcher(user).matches(), i.e. +// a full match, which MustCompile + FindStringSubmatch on an anchored pattern +// reproduces. +func TestBuildTrinoResourceGroups_CapturesOrgFromQualifiedUsername(t *testing.T) { + goPattern := strings.ReplaceAll(orgCaptureRegex, "(?<", "(?P<") + re := regexp.MustCompile("^(?:" + goPattern + ")$") + idx := re.SubexpIndex("org") + if idx < 0 { + t.Fatalf("pattern %q has no `org` capture", orgCaptureRegex) + } + + for _, tc := range []struct{ user, want string }{ + {"acme", "acme"}, + {"acme.root", "acme"}, + {"acme.analyst", "acme"}, + {"acme.posthog_team_7", "acme"}, + {"acme-analytics.analyst", "acme-analytics"}, + } { + m := re.FindStringSubmatch(tc.user) + if m == nil { + t.Errorf("user %q does not match the selector at all — its queries would be rejected", tc.user) + continue + } + if m[idx] != tc.want { + t.Errorf("user %q captured org %q, want %q", tc.user, m[idx], tc.want) + } + } +} + +// The bundle must grant a scope group the SAME catalog its org group owns — +// the cross-tenant check is the same check for both — and additionally carry +// the scope that narrows it. +func TestReconcileOPABundle_ScopeGroupOwnsTheSameCatalog(t *testing.T) { + p := &TrinoProvisioner{} + captured := &capturingBundleBuilder{inner: opa.NewBuilder()} + p.bundleBuilder = captured + p.bundleStore = &opa.BundleStore{} + + orgs := []configstore.TrinoEnabledOrg{{ + OrgID: "42", + DatabaseName: "acme", + Users: []configstore.TrinoOrgUser{{ + Username: "posthog_team_7", + PasswordHash: "$2a$10$a", + TeamID: teamID(7), + Scope: &configstore.OrgUserQueryAccess{ + ReadOnly: true, + AllowedSchemas: []string{"posthog_7"}, + AllowedRelations: []string{"posthog.events"}, + }, + }}, + }} + if err := p.reconcileOPABundle(context.Background(), orgs); err != nil { + t.Fatalf("reconcileOPABundle: %v", err) + } + + gc, gs := captured.lastCatalogs, captured.lastScopes + if !gc["org_acme"]["org_acme"] { + t.Error("org group must own its catalog") + } + if !gc["scope_acme_team_7"]["org_acme"] { + t.Error("scope group must own the SAME catalog as its org group") + } + if _, ok := gs["org_acme"]; ok { + t.Error("the org group must stay unscoped") + } + scope, ok := gs["scope_acme_team_7"] + if !ok { + t.Fatal("scope group must carry a scope document") + } + if !scope.Schemas["posthog_7"] || !scope.Relations["posthog.events"] || !scope.RelationSchemas["posthog"] { + t.Errorf("scope = %#v, want the team's schemas and relations", scope) + } +} diff --git a/tests/configstore/trino_postgres_test.go b/tests/configstore/trino_postgres_test.go index 8c328d7a..6b66d1dc 100644 --- a/tests/configstore/trino_postgres_test.go +++ b/tests/configstore/trino_postgres_test.go @@ -3,6 +3,8 @@ package configstore_test import ( + "reflect" + "slices" "testing" "time" @@ -341,3 +343,132 @@ func TestEnableTrinoOnUnknownOrgViolatesForeignKeyPostgres(t *testing.T) { t.Fatal("expected a foreign-key violation enabling Trino for an org that does not exist") } } + +// Every one of an org's own logins must reach the Trino projection, not just +// `root` — that is the whole point of per-user Trino access. The listing also +// has to fail closed on a disabled user, which must never reach a password +// file. +func TestListTrinoEnabledOrgsProjectsEveryLogin(t *testing.T) { + store := newIsolatedConfigStore(t) + seedTrinoOrg(t, store, "acme") + if err := store.EnableTrino("acme", configstore.TrinoSettings{Tier: "free"}); err != nil { + t.Fatalf("EnableTrino: %v", err) + } + for _, u := range []struct{ name, hash string }{ + {"analyst", "$2a$10$analyst"}, + {"dashboards", "$2a$10$dashboards"}, + {"leaver", "$2a$10$leaver"}, + } { + if err := store.CreateOrgUser("acme", u.name, u.hash); err != nil { + t.Fatalf("CreateOrgUser(%s): %v", u.name, err) + } + } + if err := store.SetOrgUserDisabled("acme", "leaver", true); err != nil { + t.Fatalf("SetOrgUserDisabled: %v", err) + } + if err := store.ReloadSnapshot(); err != nil { + t.Fatalf("ReloadSnapshot: %v", err) + } + + got, err := store.ListTrinoEnabledOrgs() + if err != nil { + t.Fatalf("ListTrinoEnabledOrgs: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 org, got %d", len(got)) + } + byName := map[string]configstore.TrinoOrgUser{} + for _, u := range got[0].Users { + byName[u.Username] = u + } + // root appears here too, alongside the bare org principal, so the same + // credential works under either username. + for _, want := range []struct{ name, hash string }{ + {"root", "$2a$10$hash-acme"}, + {"analyst", "$2a$10$analyst"}, + {"dashboards", "$2a$10$dashboards"}, + } { + u, ok := byName[want.name] + if !ok { + t.Errorf("login %q missing from the projection", want.name) + continue + } + // The hash is copied through unchanged: it is the SAME bcrypt the + // pgwire handshake verifies, so one password works on both engines. + if u.PasswordHash != want.hash { + t.Errorf("login %q hash = %q, want %q", want.name, u.PasswordHash, want.hash) + } + if u.Scope != nil { + t.Errorf("login %q must be unscoped", want.name) + } + } + if _, ok := byName["leaver"]; ok { + t.Error("a disabled login must never reach the password file") + } + if len(byName) != 3 { + t.Errorf("projected logins = %v, want exactly root/analyst/dashboards", byName) + } +} + +// A project-scoped login must arrive carrying the SAME scope the pgwire +// session path enforces, so Trino and DuckDB cannot disagree about which +// schemas the login may read. +func TestListTrinoEnabledOrgsCarriesProjectScopes(t *testing.T) { + store := newIsolatedConfigStore(t) + seedTrinoOrg(t, store, "acme") + if err := store.EnableTrino("acme", configstore.TrinoSettings{Tier: "free"}); err != nil { + t.Fatalf("EnableTrino: %v", err) + } + if _, err := configstore.UpsertOrgTeamTx(store.DB(), "acme", configstore.OrgTeamUpsert{ + TeamID: 7, + SchemaName: "posthog_7", + }); err != nil { + t.Fatalf("UpsertOrgTeamTx: %v", err) + } + if err := store.CreateOrgUser("acme", "posthog_team_7", "$2a$10$team7"); err != nil { + t.Fatalf("CreateOrgUser: %v", err) + } + // No configstore mutator binds a login to a team (the admin API owns that + // surface), so bind it directly — the point under test is the listing, + // not the admin handler. + if err := store.DB().Exec( + `UPDATE duckgres_org_users SET access_mode = 'project_reader', team_id = 7 + WHERE org_id = 'acme' AND username = 'posthog_team_7'`).Error; err != nil { + t.Fatalf("bind project login: %v", err) + } + if err := store.ReloadSnapshot(); err != nil { + t.Fatalf("ReloadSnapshot: %v", err) + } + + got, err := store.ListTrinoEnabledOrgs() + if err != nil { + t.Fatalf("ListTrinoEnabledOrgs: %v", err) + } + var scoped *configstore.TrinoOrgUser + for i, u := range got[0].Users { + if u.Username == "posthog_team_7" { + scoped = &got[0].Users[i] + } + } + if scoped == nil { + t.Fatal("the project login is missing from the projection") + } + if scoped.Scope == nil { + t.Fatal("the project login must carry a scope, or it would read the whole catalog") + } + if scoped.TeamID == nil || *scoped.TeamID != 7 { + t.Fatalf("TeamID = %v, want 7 — the scope group is keyed on it", scoped.TeamID) + } + // Exactly what OrgUserQueryAccess reports for the same user, which is + // what pgwire enforces. + want, ok := store.OrgUserQueryAccess("acme", "posthog_team_7") + if !ok { + t.Fatal("OrgUserQueryAccess must report the login as scoped") + } + if !reflect.DeepEqual(*scoped.Scope, want) { + t.Errorf("scope = %+v, want %+v (the same policy pgwire enforces)", *scoped.Scope, want) + } + if !slices.Contains(scoped.Scope.AllowedSchemas, "posthog_7") { + t.Errorf("AllowedSchemas = %v, must contain the team's schema", scoped.Scope.AllowedSchemas) + } +}