Skip to content

Use raw role names verbatim for worker connections #364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Pino de Candia (pinodeca) merged 1 commit into from
Aug 29, 2026
+154 −82
Merged

Use raw role names verbatim for worker connections #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Pre-1.0 note: while `pg_durable` is in major version `0`, minor releases may inc

### Fixed

- **Worker connection role names:** catalog role names are now passed verbatim when opening workflow connections, preventing quote-wrapped names from being reinterpreted as a different role.
- **Restricted HTTP transport (#342):** restricted allow-list builds now require HTTPS so credentials and request bodies cannot be sent over plaintext HTTP; development-only `http-allow-all` builds continue to permit HTTP.

## [0.2.6] - 2026-08-23
Expand Down
85 changes: 3 additions & 82 deletions src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use chrono::{DateTime, Utc};
use cron::Schedule as CronSchedule;
use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
use serde::{Deserialize, Serialize};
use std::borrow::Cow;
use std::ffi::{CStr, CString};
use std::str::FromStr;
use std::sync::{Arc, OnceLock};
Expand Down Expand Up @@ -238,49 +237,6 @@ pub fn target_database() -> String {
get_database()
}

fn normalize_role_name_for_connection(user: &str) -> Result<Cow<'_, str>, String> {
if !user.starts_with('"') {
if user.ends_with('"') {
return Err(format!(
"Invalid role name '{}': unexpected trailing double quote in connection username",
user
));
}
return Ok(Cow::Borrowed(user));
}

if !user.ends_with('"') || user.len() < 2 {
return Err(format!(
"Invalid role name '{}': unterminated quoted identifier in connection username",
user
));
}

let inner = &user[1..user.len() - 1];
let mut normalized = String::with_capacity(inner.len());
let mut chars = inner.chars().peekable();

while let Some(ch) = chars.next() {
if ch != '"' {
normalized.push(ch);
continue;
}

if chars.peek() == Some(&'"') {
normalized.push('"');
chars.next();
continue;
}

return Err(format!(
"Invalid quoted role name '{}': expected doubled double quotes inside identifier",
user
));
}

Ok(Cow::Owned(normalized))
}

/// Create a single PostgreSQL connection authenticated as `user`.
pub async fn connect_as_user(
user: &str,
Expand Down Expand Up @@ -308,11 +264,10 @@ async fn connect_as_user_with_application_name(
/// Connection timeout for per-user SQL connections (seconds).
const CONNECT_TIMEOUT_SECS: u64 = 30;

let normalized_user = normalize_role_name_for_connection(user)?;
let default_db = target_database();
let db = database.unwrap_or(&default_db);
let mut options = PgConnectOptions::new()
.username(normalized_user.as_ref())
.username(user)
.database(db)
.port(get_port())
.application_name(application_name);
Expand All @@ -328,17 +283,13 @@ async fn connect_as_user_with_application_name(
.map_err(|_| {
format!(
"Connection to database '{}' as '{}' timed out after {}s",
db,
normalized_user.as_ref(),
CONNECT_TIMEOUT_SECS
db, user, CONNECT_TIMEOUT_SECS
)
})?
.map_err(|e| {
format!(
"Failed to connect to database '{}' as '{}'. Error: {}",
db,
normalized_user.as_ref(),
e
db, user, e
)
})?;

Expand Down Expand Up @@ -1981,36 +1932,6 @@ mod tests {
}
}

#[test]
fn normalize_role_name_keeps_raw_rolname() {
let normalized = normalize_role_name_for_connection("plain_role").unwrap();
assert_eq!(normalized.as_ref(), "plain_role");
}

#[test]
fn normalize_role_name_keeps_mixed_case_rolname() {
let normalized = normalize_role_name_for_connection("labUser").unwrap();
assert_eq!(normalized.as_ref(), "labUser");
}

#[test]
fn normalize_role_name_unquotes_regrole_text_output() {
let normalized = normalize_role_name_for_connection("\"Role Name\"").unwrap();
assert_eq!(normalized.as_ref(), "Role Name");
}

#[test]
fn normalize_role_name_unescapes_embedded_quotes() {
let normalized = normalize_role_name_for_connection("\"Role \"\"Name\"\"\"").unwrap();
assert_eq!(normalized.as_ref(), "Role \"Name\"");
}

#[test]
fn normalize_role_name_rejects_malformed_quoted_identifier() {
let err = normalize_role_name_for_connection("\"bad\"name\"").unwrap_err();
assert!(err.contains("Invalid quoted role name"));
}

#[test]
fn configured_host_takes_precedence_over_pghost() {
assert_eq!(
Expand Down
75 changes: 75 additions & 0 deletions tests/e2e/sql/17_superuser_guc.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
-- 3. Forgery caught by load_function_graph (instance-level rejection).
-- 4. Forgery caught by execute_sql (node-level rejection, post-cache tamper).
-- 5. Cross-iteration forgery neutralized by the frozen graph snapshot.
-- 6. A literal quoted catalog role cannot authenticate as postgres.
--
-- "GUC on + superuser succeeds" is implicitly covered by every other E2E test
-- (standard phase runs as postgres with enable_superuser_instances = on).
Expand Down Expand Up @@ -341,6 +342,80 @@ END $$;

DROP TABLE _su_guc_t5;

-- ============================================================
-- Test 6: Quoted catalog role names cannot authenticate as another role
--
-- PostgreSQL permits a role whose literal name is "postgres" (including
-- quote bytes). The worker must pass that catalog name unchanged to libpq.
-- Normalizing it as regrole display text would authenticate this workflow as
-- the real postgres superuser and write the protected sentinel.
-- ============================================================
DROP TABLE IF EXISTS su_guc_quoted_role_sentinel;
CREATE TABLE su_guc_quoted_role_sentinel (marker TEXT);

DO $setup_quoted_role$
BEGIN
IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = chr(34) || 'postgres' || chr(34)) THEN
EXECUTE 'DROP OWNED BY """postgres"""';
EXECUTE 'DROP ROLE """postgres"""';
END IF;
END $setup_quoted_role$;
CREATE ROLE """postgres""" NOLOGIN;

DROP TABLE IF EXISTS _su_guc_t6;
CREATE TABLE _su_guc_t6 (instance_id TEXT);
GRANT ALL ON _su_guc_t6 TO df_e2e_user, su_guc_forger;

SET SESSION AUTHORIZATION df_e2e_user;
INSERT INTO _su_guc_t6
SELECT df.start(
'INSERT INTO su_guc_quoted_role_sentinel VALUES (''unexpected superuser identity'')',
'su-guc-test6-quoted-role-identity'
);
RESET SESSION AUTHORIZATION;

SET SESSION AUTHORIZATION su_guc_forger;
DO $forge_quoted_role$
DECLARE
inst TEXT;
BEGIN
SELECT instance_id INTO inst FROM _su_guc_t6;
UPDATE df.instances
SET submitted_by = (
SELECT oid::regrole FROM pg_roles WHERE rolname = chr(34) || 'postgres' || chr(34)
)
WHERE id = inst;
UPDATE df.nodes
SET submitted_by = (
SELECT oid::regrole FROM pg_roles WHERE rolname = chr(34) || 'postgres' || chr(34)
)
WHERE instance_id = inst;
END $forge_quoted_role$;
RESET SESSION AUTHORIZATION;
Comment on lines +369 to +394

@pinodeca Pino de Candia (pinodeca) Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

P2 — Make submission and forgery atomic. df.start() commits before these updates, so the worker can load or execute the graph as the original df_e2e_user first. That role also cannot insert into su_guc_quoted_role_sentinel, producing exactly the asserted failed status and empty sentinel; the test can therefore pass without exercising the quoted-role connection path. Please wrap submission plus both identity updates in one explicit transaction so the worker cannot observe the instance until the forged identity is committed. It would also help to assert the failure detail identifies authentication of the literal quoted role rather than accepting any failure.


DO $assert_quoted_role$
DECLARE
inst_id TEXT;
status TEXT;
BEGIN
SELECT instance_id INTO inst_id FROM _su_guc_t6;
SELECT df.await_instance(inst_id, 30) INTO status;

IF lower(status) != 'failed' THEN
RAISE EXCEPTION 'TEST 6 FAILED: expected quoted-role workflow to fail, got: %', status;
END IF;

IF EXISTS (SELECT 1 FROM su_guc_quoted_role_sentinel) THEN
RAISE EXCEPTION 'TEST 6 FAILED: quoted catalog role authenticated as postgres';
END IF;

RAISE NOTICE 'TEST 6 PASSED: quoted catalog role did not authenticate as postgres';
END $assert_quoted_role$;

DROP TABLE _su_guc_t6;
DROP TABLE su_guc_quoted_role_sentinel;
DROP ROLE """postgres""";

-- ============================================================
-- Cleanup
-- ============================================================
Expand Down
75 changes: 75 additions & 0 deletions tests/e2e/sql/49_quoted_role_names.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,29 @@ BEGIN
END LOOP;
END $setup$;

DO $boundary_setup$
BEGIN
PERFORM pg_terminate_backend(pid)
FROM pg_stat_activity
WHERE usename IN ('quoteedge', '"quoteedge"')
AND pid <> pg_backend_pid();
BEGIN
EXECUTE format('DROP OWNED BY %I', '"quoteedge"');
EXCEPTION
WHEN undefined_object THEN NULL;
END;
EXECUTE format('DROP ROLE IF EXISTS %I', '"quoteedge"');
EXECUTE 'DROP ROLE IF EXISTS quoteedge';
EXECUTE 'CREATE ROLE quoteedge LOGIN';
EXECUTE format('CREATE ROLE %I LOGIN', '"quoteedge"');
PERFORM df.grant_usage('"quoteedge"');
EXECUTE format(
'GRANT TEMPORARY ON DATABASE %I TO %I',
current_database(),
'"quoteedge"'
);
END $boundary_setup$;

SET SESSION AUTHORIZATION "labUser";
CREATE TEMP TABLE _test_state_quoted_1 (instance_id TEXT);
INSERT INTO _test_state_quoted_1
Expand All @@ -49,6 +72,12 @@ INSERT INTO _test_state_quoted_3
SELECT df.start('SELECT 1 AS ok', 'quoted-role-embedded-quote');
RESET SESSION AUTHORIZATION;

SET SESSION AUTHORIZATION """quoteedge""";
CREATE TEMP TABLE _test_state_quoted_4 (instance_id TEXT);
INSERT INTO _test_state_quoted_4
SELECT df.start('SELECT current_user AS who', 'quoted-role-boundary');
RESET SESSION AUTHORIZATION;

DO $$
DECLARE
inst_id TEXT;
Expand Down Expand Up @@ -112,9 +141,40 @@ BEGIN
END IF;
END $$;

DO $$
DECLARE
inst_id TEXT;
final_status TEXT;
node_role TEXT;
ran_as TEXT;
BEGIN
SELECT instance_id INTO inst_id FROM _test_state_quoted_4;
SELECT df.await_instance(inst_id, 30) INTO final_status;
IF final_status != 'completed' THEN
RAISE EXCEPTION 'TEST FAILED (boundary "quoteedge"): expected completed, got %', final_status;
END IF;
SELECT r.rolname INTO node_role
FROM df.nodes n
JOIN pg_catalog.pg_roles r ON r.oid = n.submitted_by::oid
WHERE n.instance_id = inst_id
LIMIT 1;
IF node_role != '"quoteedge"' THEN
RAISE EXCEPTION 'TEST FAILED (boundary): expected submitted_by rolname "quoteedge" (quotes included), got %', node_role;
END IF;
SELECT (result->'rows'->0->>'who') INTO ran_as
FROM df.nodes
WHERE instance_id = inst_id
AND result IS NOT NULL
LIMIT 1;
IF ran_as != '"quoteedge"' THEN
RAISE EXCEPTION 'TEST FAILED (boundary): workflow executed as %, expected literal "quoteedge"', ran_as;
END IF;
END $$;

DROP TABLE _test_state_quoted_1;
DROP TABLE _test_state_quoted_2;
DROP TABLE _test_state_quoted_3;
DROP TABLE _test_state_quoted_4;

DO $cleanup$
DECLARE
Expand All @@ -135,4 +195,19 @@ BEGIN
END LOOP;
END $cleanup$;

DO $boundary_cleanup$
BEGIN
PERFORM pg_terminate_backend(pid)
FROM pg_stat_activity
WHERE usename IN ('quoteedge', '"quoteedge"')
AND pid <> pg_backend_pid();
BEGIN
EXECUTE format('DROP OWNED BY %I', '"quoteedge"');
EXCEPTION
WHEN undefined_object THEN NULL;
END;
EXECUTE format('DROP ROLE IF EXISTS %I', '"quoteedge"');
EXECUTE 'DROP ROLE IF EXISTS quoteedge';
END $boundary_cleanup$;

SELECT 'TEST PASSED' AS result;
Loading