ci: add pgspot check for extension SQL#199
Merged
Merged
Conversation
Add a dedicated `pgspot` CI job that lints the static SQL pg_durable actually ships -- the generated CREATE EXTENSION install SQL plus any non-frozen upgrade script -- for search_path / privilege-escalation issues (CVE-2018-1058 class) and other unsafe constructs. - scripts/run-pgspot.sh: generic engine pinning pgspot 0.9.2 in a venv, with per-file attribution and a documented (currently empty) --ignore allowlist. pgspot already exits non-zero on any error, warning, or unknown, so the strict zero-findings policy is enforced by its exit code. - scripts/pgspot-gate.sh: project entry point that scans the generated install SQL plus upgrade scripts not listed in the frozen baseline. - sql/pgspot-frozen.txt: baseline exempting already-released, immutable upgrade scripts. New upgrade scripts are gated automatically; the frozen first-version fixture is excluded by construction. - ci.yml: dedicated pgspot job (pg17) generating install SQL via cargo pgrx schema and running the gate, isolated for a clean red/green signal. This commit intentionally lands the gate without source fixes; the generated install SQL findings are addressed in follow-up commits.
Schema-qualify every function, type cast and operator in the generated `df` install DDL so pgspot reports no search_path / CVE-2018-1058 class findings: - now() -> pg_catalog.now() (column defaults) - quote_ident(current_user)::regrole -> pg_catalog.quote_ident(current_user)::pg_catalog.regrole (vars default and the three per-user RLS policies) - CHECK / RLS operators: `~` -> OPERATOR(pg_catalog.~); `IN (...)` -> OPERATOR(pg_catalog.=) ANY (ARRAY[...]); RLS `=` -> OPERATOR(pg_catalog.=) These changes are purely textual: the parser binds built-in objects to the same OIDs regardless of qualification, so the stored catalog representation (pg_get_constraintdef / pg_get_expr / policy defs) is byte-identical to the unqualified form. Verified against a live server that old and new DDL produce identical catalog state, so the upgrade test (Scenario A) is unaffected and no upgrade script is required. Allowlist PS010 for the pgrx-emitted `CREATE SCHEMA IF NOT EXISTS df` (not controllable from source; residual risk negligible). Schemas we control (CREATE SCHEMA duroxide) omit IF NOT EXISTS and remain gated.
- Qualify the two install-SQL DO blocks so unqualified refs can't resolve via the session search_path (CVE-2018-1058); verified clean in isolation - Scope PS010 to the df schema via a per-finding allowlist (not global --ignore) - Make scan_file fail-closed with a two-pass check (catches fatals/unknowns) - Add DO-block isolation scan to defeat pgspot's whole-file search_path leak - Treat an empty scan set as a hard error - Bump 0.2.2 -> 0.2.3 with a no-op upgrade script (qualification is catalog-invisible) to preserve release immutability - Docs: freeze-step/qualification guidance + Version-Specific Changes entry
The 0.2.2 -> 0.2.3 bump activated the B1/B2 upgrade scenarios (they only run for versions past the 0.2.2 provider-compat start), which exercise a single long-lived BGW across repeated DROP/CREATE EXTENSION while the non-extension- owned duroxide schema persists. That path hits a duroxide runtime/schema mismatch (orchestrator_queue / enqueue_orchestrator_work / fetch_work_item missing, cached-plan invalidation), leaving instances stuck pending — a pre-existing issue unrelated to the pgspot gate or the DDL qualification here. Keep this PR focused on the security gate + schema qualification (catalog- invisible, no bump required). The bump and the B1/B2 enablement will land in a dedicated PR where the worker/upgrade-harness issue can be addressed properly. Removes the no-op sql/pg_durable--0.2.2--0.2.3.sql and the v0.2.2->v0.2.3 version-specific doc entry; keeps the general pgspot qualification/freeze guidance.
Trim verbose comments and docstrings across the pgspot gate files (run-pgspot.sh, pgspot-gate.sh, extract-do-blocks.py, pgspot-frozen.txt, ci.yml, lib.rs auditability note) for readability. Comments only -- no code or logic changes.
Replace sql/pgspot-frozen.txt and its parsing with a small hardcoded EXCLUDE array in pgspot-gate.sh listing the pre-pgspot legacy upgrade scripts (authored before the install DDL was schema-qualified, so they don't all pass pgspot). Scripts written after qualification pass the gate and need no entry, so the list is fixed rather than growing per release. Update ci.yml comment and upgrade-testing.md accordingly.
The gate extracted each anonymous DO block and scanned it in isolation to work around a pgspot bug: pgspot's file-level search_path-secure flag leaks into DO blocks, suppressing unqualified-reference checks even though a DO block runs under the session search_path. The workaround added a pglast dependency and notable complexity. Drop the extraction (delete extract-do-blocks.py and the isolation loop, env var, and helper plumbing in run-pgspot.sh). Our DO blocks are already fully schema-qualified; the gate still qualifies everything else. Document the pgspot limitation so DO-block qualification is checked in review. A fix is being submitted upstream.
tjgreen42
changed the title
pinodeca
approved these changes
Jun 2, 2026
Contributor
There was a problem hiding this comment.
I have 2 concerns about ci: add pgspot check for extension SQL by tjgreen42 · Pull Request #199 · microsoft/pg_durable
- The new CI job introduces significant pipeline code duplication.
- Hardening lib.rs (via scoping) is awesome and great for new installations, but: should we have the 0.2.2->0.2.3 upgrade script modify existing functions and other objects (constraints) so that upgrades get the same benefits? The user-base will still be very small by the time this ships, but we'd probably do this if it were already a 1.x version, right?
- EDIT: Opus 4.7 tells me this won't add value because the hardening was NOT on bodies of the PL/pgSQL / SQL functions. The hardening in this PR only matters when the DDL runs, not DML.
What do you think about the points above AND if you think either should be addressed, would you prefer to do that in the same PR or merge this one and then do followup PRs?
Run the pgspot gate as steps in the existing pg17 test job instead of a separate job that rebuilt the whole pgrx environment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Thanks!
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a pgspot CI gate that lints the SQL pg_durable ships (generated install SQL + non-frozen upgrade scripts) for
search_path/privilege-escalation issues, and schema-qualifies the install DDL insrc/lib.rsso it passes.