citusdata / citusdata/citus

Preserve GRANTED BY semantics for distributed object privileges

Open
#8,759 0 comments 0 reactions 0 assignees Claimed by @ihalatci View on GitHub
pg19_features
Dominant language
C
Stars
12.8k
Forks
794
Avg merge
2d 14h
Merged PRs (30d)
31

Description

## Problem

Citus's manually reconstructed distributed-table `GRANT`/`REVOKE` commands omit `GrantStmt.grantor` (`GRANTED BY`). Coordinator and shard ACL grantors can therefore diverge, and a matching `REVOKE ... GRANTED BY` can remove the coordinator ACL while leaving shard ACLs intact.

This is a pre-existing distributed authorization correctness bug, not a PostgreSQL 19 parser gap. PostgreSQL 19 widens where `GRANTED BY` is accepted, but the distributed table deparser already loses the field.

## Verified reproduction

- `GRANT ... GRANTED BY gap_grantor` records `gap_grantor` on the coordinator but the current executing user on both shards.
- After correcting shard grantors manually, `REVOKE ... GRANTED BY gap_grantor` removes the coordinator ACL but leaves shard ACLs.
- Generic object and role deparsers preserve the grantor; the distributed-table path in `grant.c` is the affected special case.

## Acceptance criteria

- Distributed-table GRANT/REVOKE preserves explicit `GRANTED BY` on coordinator and all shards.
- Authorization and role-membership checks match PostgreSQL behavior.
- Quoted role names and keyword/current-user forms are handled safely.
- Multi-node tests assert identical ACL grantor and revoke results across coordinator/workers.
- Existing GRANT/REVOKE forms without `GRANTED BY` remain unchanged on supported PostgreSQL versions.
## Additional verified defect: shared-object REVOKE clause ordering

`AppendGrantSharedSuffix()` emits `RESTRICT`/`CASCADE` before `GRANTED BY`, but PostgreSQL grammar requires:

```sql
REVOKE ... FROM ... [GRANTED BY ...] [CASCADE|RESTRICT]
```

Generated worker SQL therefore fails at `GRANTED BY` for schema, database, sequence, function/procedure/routine/aggregate, FDW, and foreign-server privileges. GRANT propagation succeeds; REVOKE fails atomically and leaves coordinator/workers unchanged.

This malformed ordering exists on PG16–19. PG19 makes the table-path omission more severe by allowing an inherited eligible grantor and scoping REVOKE to that exact grantor.

## Minimum implementation plan

1. In `AppendGrantSharedSuffix()`, emit `GRANTED BY` before `RESTRICT`/`CASCADE`.
2. In the manually reconstructed table path:
- GRANT: emit grantor after `WITH GRANT OPTION`.
- REVOKE: emit grantor after grantees and before `CASCADE`/`RESTRICT`.
3. Reuse the existing role-spec serializer; no version gate or broad refactor is needed.
4. Add PG19 table coverage for quoted inherited grantors, grant options, physical-shard grantor equality, and complete REVOKE.
5. Add compact cross-version PG16–19 coverage for all shared-object callers and suffix combinations.

## Runtime evidence

- Distributed table GRANT records the requested grantor on the coordinator but the executing user on physical shards; matching REVOKE leaves shard ACLs behind.
- Shared-object REVOKE produces invalid worker SQL such as `... RESTRICT GRANTED BY ...`.
- Implicit/explicit RESTRICT, CASCADE, WITH GRANT OPTION, and GRANT OPTION FOR were reproduced.
- Role-membership GRANT/REVOKE already emits the correct order and is unaffected.
## Precise PG19 table reproduction

A table owned by the explicit grantor is not a discriminating test because PostgreSQL workers independently choose that same role as the best grantor. The verified divergent case requires the named inherited grantor to differ from the best grantor:

- Table owned by a third role.
- `role_a` and `role_b` both hold grant option.
- Login `grant_actor` inherits both.
- `GRANT ... GRANTED BY role_b` records `role_b` on the coordinator but unfixed shards independently choose `role_a`.
- Selective grants/revokes by role_a/role_b produce real coordinator/shard privilege divergence (`has_table_privilege` can be true on coordinator and false on shards, or the reverse).

The table fix should serialize the explicit grantor in the grammar-correct slot: after `WITH GRANT OPTION` for GRANT, and after grantees but before `CASCADE|RESTRICT` for REVOKE. It compiles cross-version but needs PG19 regression coverage because inherited alternative grantors become expressible there.

Implementation remains split into two independently reviewable changes:

1. Cross-version shared-object REVOKE suffix ordering targeting `main`.
2. PG19 table grantor serialization and regression targeting `pg19-support`.
## Accepted shared-object ordering implementation

Local commit `d866381b1` targets `main` and changes 3 files (+48/-1): one product-line ordering fix in `citus_grantutils.c` plus schema propagation regression coverage.

It emits `GRANTED BY` before `CASCADE|RESTRICT`, matching PostgreSQL GRANT/REVOKE grammar. Runtime negative control produced worker syntax errors before the fix and clean coordinator/worker ACL parity after it.

Independent release review found no significant issues across GRANT/REVOKE, grant options, behavior clauses, supported shared-object callers, role quoting, and PG16–19 compatibility.

The table grantor-serialization implementation remains a separate PG19-support change and is not part of this commit.
## Accepted table-grantor implementation

Local PG19-support commit `7bff7063a308cdc722a91a8e50092fcf1e7e3bc1` changes `commands/grant.c` plus PG19 regression coverage (3 files, +305/-4). It serializes explicit table grantors in grammar-correct GRANT/REVOKE slots.

The discriminating test uses a third-party owner and two inherited eligible grantors so the explicit grantor differs from PostgreSQL's independently selected worker grantor. Stock negative control flips four physical-shard assertions: grantor identity, selective-revoke privilege parity, quoted grantor/grant option, and revoke-grant-option identity. Coordinator rows remain unchanged, proving the worker propagation defect.

Independent release review found no significant issues. This remains separate from main PR #8766, which fixes shared-object REVOKE suffix ordering and intentionally only tracks this combined issue.

Implementation PRs:
- #8766 — shared-object ordering on `main` (Tracks #8759; must not close it alone)
- table-grantor PR — pending publication against `pg19-support`

Implementation PR: #8767 (table grantor propagation on pg19-support).

Contributor guide

Open the contributing guide

Research direction

The issue describes fixes in `citus_grantutils.c` for shared-object suffix ordering and `commands/grant.c` for distributed-table grantor serialization, with regression coverage for PostgreSQL 16–19. Read the relevant grant propagation code and existing regression tests; however, implementation PRs #8766 and #8767 already address the work, so this is not an available first issue.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, postgresql
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
1/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.