Skip to content

Enforce object ownership for more citus-internal UDFs - #8587

Merged
onurctirtir merged 8 commits into
mainfrom
fix/delete-placement-metadata
May 20, 2026
Merged

Enforce object ownership for more citus-internal UDFs#8587
onurctirtir merged 8 commits into
mainfrom
fix/delete-placement-metadata

Conversation

@onurctirtir

@onurctirtir onurctirtir commented May 15, 2026

Copy link
Copy Markdown
Member

DESCRIPTION: Enforces object-ownership for more citus-internal UDFs for security.

@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.75%. Comparing base (c2bd6ae) to head (d770c2c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8587      +/-   ##
==========================================
- Coverage   88.75%   88.75%   -0.01%     
==========================================
  Files         288      288              
  Lines       64263    64289      +26     
  Branches     8086     8089       +3     
==========================================
+ Hits        57036    57059      +23     
+ Misses       4892     4891       -1     
- Partials     2335     2339       +4     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@onurctirtir
onurctirtir force-pushed the fix/delete-placement-metadata branch from e39bb77 to 96a4f00 Compare May 15, 2026 14:14
@onurctirtir onurctirtir changed the title fix Enforce object ownership for more citus-internal UDFs May 15, 2026
@onurctirtir
onurctirtir marked this pull request as ready for review May 15, 2026 14:32
{
EnsureCitusInitiatedOperation();

EnsureTableOwner(relationId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that EnsureTableOwner() is called before ShouldSkipMetadataChecks() checks in pre-existing code (e.g., citus_internal_delete_partition_metadata at L3540). Here, are the ownership checks bypassable via the EnableManualMetadataChangesForUser GUC ? If so, is that intentional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we have examples for both approaches in the code-base.
citus_internal_update_placement_metadata and citus_internal_delete_shard_metadata have their EnsureShardOwner() checks behind !ShouldSkipMetadataChecks() too. However, all other UDFs perform such checks regardless of ShouldSkipMetadataChecks().

So, yes, we should probably avoid having such checks by-passable, let me update the PR.

@colm-mchugh colm-mchugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks solid - it fixes a real security gap in 4 UDFs - let's add tests to show the fix in action ? It should be possible to test that:

  • Non-owner users are blocked from calling the fixed UDFs
  • Owner users (via Citus-initiated operations) still succeed

@onurctirtir

Copy link
Copy Markdown
Member Author

PR looks solid - it fixes a real security gap in 4 UDFs - let's add tests to show the fix in action ? It should be possible to test that:

  • Non-owner users are blocked from calling the fixed UDFs
  • Owner users (via Citus-initiated operations) still succeed

We're anyways testing 2) implicitly in many code-paths, but pushed a commit to add tests for 1).

@onurctirtir
onurctirtir merged commit 10c3a8b into main May 20, 2026
159 checks passed
@onurctirtir
onurctirtir deleted the fix/delete-placement-metadata branch May 20, 2026 16:54
SpencerGarnets added a commit to ai-blaise/citus that referenced this pull request May 26, 2026
Extends the upstream-rebase smoke from source-level fingerprints to a real
end-to-end Citus exercise. Phase 2 boots a coordinator + worker on a docker
network from the latest pre-cherry-pick stable upstream image
(citusdata/citus:14.0.0-pg17, v14.0.0 tagged 2026-02-10 — pre-dates all 5
cherry-picks per `git log v14.0.0..upstream/main`); Phase 3 rebuilds
bundle1-final-light from the in-tree cherry-picked Citus source and reruns
the same SQL patterns against the rebuilt image. Each fix MUST behave
correctly on the rebuilt image or the smoke fails.

Verified on experiment-playground-plus-2 (gcloud blaise-478114, Debian 12 +
Docker 20.10, 48 cores) 2026-05-26T17:42:33Z:

  upstream@citus:14.0.0-pg17 (no cherry-picks):
    citusdata#8498 COLLATE+typecast  bug_observed
    citusdata#8497 NOT IS DISTINCT   bug_observed (TLP 224+0 != 288)
    citusdata#8587 ownership         behaves_correctly

  fork@bundle1-final-light (with cherry-picks):
    citusdata#8498 COLLATE+typecast  behaves_correctly
    citusdata#8497 NOT IS DISTINCT   behaves_correctly (TLP 224+64 == 288)
    citusdata#8587 ownership         behaves_correctly

The fork bundle1-final-light boots with the full ai-blaise customization
stack — citus + timescaledb + ai_blaise_citus + vector + pg_cron + age +
pgaudit + pgauditlogtofile + pg_failover_slots + pgnodemx + 14 SQL-only
extensions — proving the 5 cherry-picks compose end-to-end with our
extension overlay + initdb path.

Reproducer details mirror the upstream regression tests:
- citusdata#8498: GROUP BY (...::VARCHAR COLLATE "C") on distributed inet table
- citusdata#8497: TLP NOT (x IS DISTINCT FROM y) on INHERITS hierarchy with LEFT JOIN
- citusdata#8587: SET ROLE r_other; citus_internal_update_relation_colocation

Evidence TSV grew from 11 to 16 fields adding upstream_*/fork_* columns for
each bug so the bug -> fix delta is machine-readable. Phase 1 source-level
verification still runs unconditionally for the cheap path.

Operational details handled in the smoke:
- PGSODIUM_KEY env (bundle1 preloads pgsodium fail-closed)
- shared_preload set from images/citus-pg-overlay/shared-preload-libraries.conf
  minus pgsodium (which is SQL-loaded with the dev key)
- Wait for actual SELECT 1 connectivity (bundle1 restarts postgres mid-initdb)
- Drop pgsodium event trigger before DDL (GUC dependency)
- Split CREATE ROLE from distributed-table DDL (Citus parallel-op constraint)

PRODUCTION_READINESS_AUDIT.md gains a paragraph recording the VM-verified
evidence inline with the 2026-05-26 promotion paragraphs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ihalatci added a commit that referenced this pull request Jun 30, 2026
…to release-12.1) (#8626)

Backport of #8587 to `release-12.1`.

Cherry-picked (`-x`) the security fix that enforces object ownership for
more citus-internal UDFs. Author (Onur Tirtir) preserved.

### Adaptations for 12.1
- Resolved a cherry-pick conflict and adapted the SQL regression test
for schema renames present on this branch.
- Mapped the 13.2 C API `EnsureCitusInitiatedOperation()` to the 12.1
equivalent `EnsureCoordinatorInitiatedOperation()`.

### Verification
- Native build on PG 16.14 (WSL): GREEN.
- Affected regressions: **9/9 PASS**, no `regression.diffs`.

Original PR: #8587

Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ihalatci added a commit that referenced this pull request Jul 1, 2026
## Set 14.2.0 as upcoming version

Bumps the in-development version on the `release-14` line from released
**14.1** to upcoming **14.2.0**, and points the N-1 CI slot at
**14.1.0** (previously 14.0.1). This is the direct 14.x analogue of the
13.4.0 bootstrap commit `4cd5cdde5` (*"Set 13.4.0 as upcoming version;
run N-1 tests against 13.3.0"*) and follows the same canonical 12-file
pattern.

### What changed (mirrors the 13.4 template exactly)
- `configure.ac` / `configure` — project version `14.1.0` → `14.2.0`
- `citus.control` / `citus_columnar.control` — `default_version =
'14.2-1'`
- `config.py` — `MASTER_VERSION = "14.2"`
- 4 new comment-only migration stubs: `citus--14.1-1--14.2-1` (+
downgrade) and `citus_columnar--14.1-1--14.2-1` (+ downgrade)
- `multi_extension.sql` / `.out` — 14.2-1 snapshot + downgrade blocks;
`SHOW citus.version` → `14.2.0`; DETAIL lines bumped to `14.2`
- `.github/workflows/build_and_test.yml` — N-1 jobs now run against
`/opt/citus-versions/v14.1.0` / `14.1-1`, stable image suffix
`-vf4adc47`

The workflow half is byte-identical to the earlier CI-validated N-1
experiment. Its only divergence from the 13.4 template is the 5 PG-minor
param bumps (16.14 / 17.10 / 18.4), which the 14.x `-vf4adc47` stable
images were published at.

### N-1 CI expectation (pre-existing, orthogonal)
Running N-1 against 14.1.0 is expected to reproduce the **latent citus
#8587 gap** — the same 2 deterministic `check-multi-1` reds seen on 13.x
(PR #8632): **Lib N-1 / PG18** and **Coordinator N-1 / PG18** fail on
`create_ref_dist_from_citus_local` (`delete_placement_metadata(1)`);
**Worker N-1** and **SQL N-1** pass. This was confirmed across two runs
during the validation experiment (closed PR #8633) and is pre-existing /
orthogonal to this version bump.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Ibrahim Halatci <ihalatci@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants