diff --git a/Makefile b/Makefile index f2a0b9a62..02331ddf1 100644 --- a/Makefile +++ b/Makefile @@ -184,7 +184,8 @@ REGRESS = scan \ security \ reserved_keyword_alias \ agtype_jsonb_cast \ - containment_selectivity + containment_selectivity \ + extension_security ifneq ($(EXTRA_TESTS),) REGRESS += $(EXTRA_TESTS) diff --git a/README.md b/README.md index 819d9dcde..412b55e8d 100644 --- a/README.md +++ b/README.md @@ -215,6 +215,16 @@ LOAD 'age'; SET search_path = ag_catalog, "$user", public; ``` +### Note on `ag_catalog` ownership + +AGE installs all of its objects into the `ag_catalog` schema. Install AGE +(`CREATE EXTENSION age`) **before** granting the `CREATE` privilege on the +database to other roles. A role that can create schemas could otherwise +pre-create `ag_catalog` and own it; `CREATE EXTENSION age` therefore refuses to +install when `ag_catalog` already exists and is owned by a different role. If you +hit that error, drop the stray schema (`DROP SCHEMA ag_catalog CASCADE`) or +transfer its ownership to the installing role, then retry. +

  Using AGE with Non-Autocommit Clients (psycopg, JDBC, etc.)

If you are using AGE from a database client that does **not** default to autocommit — most commonly `psycopg` v3 or JDBC — you must understand how PostgreSQL's transaction semantics apply to AGE's setup and DDL-like functions. Otherwise, you may see graphs or labels that appear to be created successfully, but are not visible from new connections. diff --git a/age--1.7.0--y.y.y.sql b/age--1.7.0--y.y.y.sql index a4cac0c5c..64c935e6b 100644 --- a/age--1.7.0--y.y.y.sql +++ b/age--1.7.0--y.y.y.sql @@ -41,7 +41,10 @@ CREATE FUNCTION ag_catalog.age_prepare_pg_upgrade() RETURNS void LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ DECLARE graph_count integer; @@ -108,7 +111,10 @@ COMMENT ON FUNCTION ag_catalog.age_prepare_pg_upgrade() IS CREATE FUNCTION ag_catalog.age_finish_pg_upgrade() RETURNS void LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ DECLARE mapping_count integer; @@ -231,7 +237,7 @@ BEGIN -- and preserve original schema ownership. -- RAISE NOTICE 'Invalidating AGE caches...'; - PERFORM pg_catalog.pg_advisory_xact_lock(hashtext('age_finish_pg_upgrade')); + PERFORM pg_catalog.pg_advisory_xact_lock(pg_catalog.hashtext('age_finish_pg_upgrade')); DECLARE graph_rec RECORD; cache_invalidated boolean := false; @@ -245,8 +251,8 @@ BEGIN BEGIN -- Touch schema by changing owner to current_user then back to original -- This triggers cache invalidation without permanently changing ownership - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); cache_invalidated := true; EXCEPTION WHEN insufficient_privilege THEN -- If we can't change ownership, skip this schema @@ -273,7 +279,10 @@ COMMENT ON FUNCTION ag_catalog.age_finish_pg_upgrade() IS CREATE FUNCTION ag_catalog.age_revert_pg_upgrade_changes() RETURNS void LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ BEGIN -- Check if namespace column is oid type (needs reverting) @@ -306,7 +315,7 @@ BEGIN -- Invalidate AGE's internal caches by touching each graph's namespace -- We use xact-level advisory lock and preserve original ownership -- - PERFORM pg_catalog.pg_advisory_xact_lock(hashtext('age_revert_pg_upgrade')); + PERFORM pg_catalog.pg_advisory_xact_lock(pg_catalog.hashtext('age_revert_pg_upgrade')); DECLARE graph_rec RECORD; BEGIN @@ -318,8 +327,8 @@ BEGIN LOOP BEGIN -- Touch schema by changing owner to current_user then back to original - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); EXCEPTION WHEN insufficient_privilege THEN RAISE NOTICE 'Could not invalidate cache for schema % (insufficient privileges)', graph_rec.ns_name; END; @@ -345,7 +354,10 @@ CREATE FUNCTION ag_catalog.age_pg_upgrade_status() message text ) LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ DECLARE ns_type text; @@ -447,7 +459,7 @@ BEGIN AND t.tgname = '_age_cache_invalidate' ) THEN - EXECUTE format( + EXECUTE pg_catalog.format( 'CREATE TRIGGER _age_cache_invalidate ' 'AFTER INSERT OR UPDATE OR DELETE OR TRUNCATE ' 'ON %I.%I ' diff --git a/regress/expected/extension_security.out b/regress/expected/extension_security.out new file mode 100644 index 000000000..30241623a --- /dev/null +++ b/regress/expected/extension_security.out @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +LOAD 'age'; +SET search_path TO ag_catalog; +-- +-- pg_upgrade helper functions resolve built-ins from pg_catalog first. +-- +-- Each helper must place pg_catalog ahead of ag_catalog in its search_path, so +-- that built-in functions and operators always resolve to pg_catalog and are +-- not overridden by same-named objects defined in ag_catalog. +-- +SELECT p.proname, + array_to_string(p.proconfig, ', ') AS proconfig +FROM pg_proc p +JOIN pg_namespace n ON n.oid = p.pronamespace +WHERE n.nspname = 'ag_catalog' + AND p.proname IN ('age_prepare_pg_upgrade', 'age_finish_pg_upgrade', + 'age_revert_pg_upgrade_changes', 'age_pg_upgrade_status') +ORDER BY p.proname; + proname | proconfig +-------------------------------+------------------------------------ + age_finish_pg_upgrade | search_path=pg_catalog, ag_catalog + age_pg_upgrade_status | search_path=pg_catalog, ag_catalog + age_prepare_pg_upgrade | search_path=pg_catalog, ag_catalog + age_revert_pg_upgrade_changes | search_path=pg_catalog, ag_catalog +(4 rows) + +-- +-- The helper bodies must not contain unqualified format()/hashtext() calls; +-- those built-ins are explicitly schema-qualified to pg_catalog. +-- +SELECT p.proname, + (p.prosrc ~ '[^.]\mformat\s*\(') AS has_unqualified_format, + (p.prosrc ~ '[^.]\mhashtext\s*\(') AS has_unqualified_hashtext +FROM pg_proc p +JOIN pg_namespace n ON n.oid = p.pronamespace +WHERE n.nspname = 'ag_catalog' + AND p.proname IN ('age_finish_pg_upgrade', 'age_revert_pg_upgrade_changes') +ORDER BY p.proname; + proname | has_unqualified_format | has_unqualified_hashtext +-------------------------------+------------------------+-------------------------- + age_finish_pg_upgrade | f | f + age_revert_pg_upgrade_changes | f | f +(2 rows) + +-- +-- Install-time ownership check: CREATE EXTENSION age installs into ag_catalog +-- only when that schema does not already exist under a different owner. The +-- check compares schema ownership against the installing role. Verify the +-- underlying detection both ways with a probe schema, without disturbing the +-- already-installed extension. +-- +CREATE ROLE age_probe_role NOLOGIN; +CREATE SCHEMA age_probe AUTHORIZATION age_probe_role; +-- A schema owned by a different role is detected as foreign-owned. +SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_namespace n + WHERE n.nspname = 'age_probe' + AND n.nspowner <> (SELECT r.oid FROM pg_catalog.pg_roles r + WHERE r.rolname = current_user) +) AS foreign_owner_detected; + foreign_owner_detected +------------------------ + t +(1 row) + +-- ag_catalog, owned by the current (installing) role here, is not flagged +-- (the check does not false-positive on a normal install). +SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_namespace n + WHERE n.nspname = 'ag_catalog' + AND n.nspowner <> (SELECT r.oid FROM pg_catalog.pg_roles r + WHERE r.rolname = current_user) +) AS installer_owned_flagged; + installer_owned_flagged +------------------------- + f +(1 row) + +DROP SCHEMA age_probe; +DROP ROLE age_probe_role; diff --git a/regress/sql/extension_security.sql b/regress/sql/extension_security.sql new file mode 100644 index 000000000..433283989 --- /dev/null +++ b/regress/sql/extension_security.sql @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +LOAD 'age'; +SET search_path TO ag_catalog; + +-- +-- pg_upgrade helper functions resolve built-ins from pg_catalog first. +-- +-- Each helper must place pg_catalog ahead of ag_catalog in its search_path, so +-- that built-in functions and operators always resolve to pg_catalog and are +-- not overridden by same-named objects defined in ag_catalog. +-- +SELECT p.proname, + array_to_string(p.proconfig, ', ') AS proconfig +FROM pg_proc p +JOIN pg_namespace n ON n.oid = p.pronamespace +WHERE n.nspname = 'ag_catalog' + AND p.proname IN ('age_prepare_pg_upgrade', 'age_finish_pg_upgrade', + 'age_revert_pg_upgrade_changes', 'age_pg_upgrade_status') +ORDER BY p.proname; + +-- +-- The helper bodies must not contain unqualified format()/hashtext() calls; +-- those built-ins are explicitly schema-qualified to pg_catalog. +-- +SELECT p.proname, + (p.prosrc ~ '[^.]\mformat\s*\(') AS has_unqualified_format, + (p.prosrc ~ '[^.]\mhashtext\s*\(') AS has_unqualified_hashtext +FROM pg_proc p +JOIN pg_namespace n ON n.oid = p.pronamespace +WHERE n.nspname = 'ag_catalog' + AND p.proname IN ('age_finish_pg_upgrade', 'age_revert_pg_upgrade_changes') +ORDER BY p.proname; + +-- +-- Install-time ownership check: CREATE EXTENSION age installs into ag_catalog +-- only when that schema does not already exist under a different owner. The +-- check compares schema ownership against the installing role. Verify the +-- underlying detection both ways with a probe schema, without disturbing the +-- already-installed extension. +-- +CREATE ROLE age_probe_role NOLOGIN; +CREATE SCHEMA age_probe AUTHORIZATION age_probe_role; + +-- A schema owned by a different role is detected as foreign-owned. +SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_namespace n + WHERE n.nspname = 'age_probe' + AND n.nspowner <> (SELECT r.oid FROM pg_catalog.pg_roles r + WHERE r.rolname = current_user) +) AS foreign_owner_detected; + +-- ag_catalog, owned by the current (installing) role here, is not flagged +-- (the check does not false-positive on a normal install). +SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_namespace n + WHERE n.nspname = 'ag_catalog' + AND n.nspowner <> (SELECT r.oid FROM pg_catalog.pg_roles r + WHERE r.rolname = current_user) +) AS installer_owned_flagged; + +DROP SCHEMA age_probe; +DROP ROLE age_probe_role; diff --git a/sql/age_main.sql b/sql/age_main.sql index 72f420002..233d0d23f 100644 --- a/sql/age_main.sql +++ b/sql/age_main.sql @@ -20,6 +20,33 @@ -- complain if script is sourced in psql, rather than via CREATE EXTENSION \echo Use "CREATE EXTENSION age" to load this file. \quit +-- +-- Ensure ag_catalog is created and owned by the installing role. +-- +-- CREATE EXTENSION places all of AGE's objects in ag_catalog. A normal install +-- creates that schema, owned by the installer. If ag_catalog already exists and +-- is owned by a different role, that role would retain control over the schema +-- that holds AGE's catalog objects. To keep ownership well-defined, refuse to +-- install into a pre-existing ag_catalog owned by another role. Ownership is +-- compared directly (not via role membership) so the check is exact even for a +-- superuser, who is otherwise considered a member of every role. +-- +DO $age_install_guard$ +BEGIN + IF EXISTS ( + SELECT 1 + FROM pg_catalog.pg_namespace n + WHERE n.nspname = 'ag_catalog' + AND n.nspowner <> (SELECT r.oid + FROM pg_catalog.pg_roles r + WHERE r.rolname = current_user) + ) THEN + RAISE EXCEPTION 'schema "ag_catalog" already exists and is not owned by the installing role "%"', current_user + USING HINT = 'Apache AGE will not install into a pre-existing ag_catalog owned by another role. Drop it (DROP SCHEMA ag_catalog CASCADE) or transfer its ownership to the installing role, then retry CREATE EXTENSION age.'; + END IF; +END +$age_install_guard$; + -- -- catalog tables -- diff --git a/sql/age_pg_upgrade.sql b/sql/age_pg_upgrade.sql index 42a06ecd6..68fbd1513 100644 --- a/sql/age_pg_upgrade.sql +++ b/sql/age_pg_upgrade.sql @@ -55,7 +55,10 @@ CREATE FUNCTION ag_catalog.age_prepare_pg_upgrade() RETURNS void LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ DECLARE graph_count integer; @@ -143,7 +146,10 @@ COMMENT ON FUNCTION ag_catalog.age_prepare_pg_upgrade() IS CREATE FUNCTION ag_catalog.age_finish_pg_upgrade() RETURNS void LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ DECLARE mapping_count integer; @@ -266,7 +272,7 @@ BEGIN -- and preserve original schema ownership. -- RAISE NOTICE 'Invalidating AGE caches...'; - PERFORM pg_catalog.pg_advisory_xact_lock(hashtext('age_finish_pg_upgrade')); + PERFORM pg_catalog.pg_advisory_xact_lock(pg_catalog.hashtext('age_finish_pg_upgrade')); DECLARE graph_rec RECORD; cache_invalidated boolean := false; @@ -280,8 +286,8 @@ BEGIN BEGIN -- Touch schema by changing owner to current_user then back to original -- This triggers cache invalidation without permanently changing ownership - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); cache_invalidated := true; EXCEPTION WHEN insufficient_privilege THEN -- If we can't change ownership, skip this schema @@ -330,7 +336,10 @@ COMMENT ON FUNCTION ag_catalog.age_finish_pg_upgrade() IS CREATE FUNCTION ag_catalog.age_revert_pg_upgrade_changes() RETURNS void LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ BEGIN -- Check if namespace column is oid type (needs reverting) @@ -363,7 +372,7 @@ BEGIN -- Invalidate AGE's internal caches by touching each graph's namespace -- We use xact-level advisory lock and preserve original ownership -- - PERFORM pg_catalog.pg_advisory_xact_lock(hashtext('age_revert_pg_upgrade')); + PERFORM pg_catalog.pg_advisory_xact_lock(pg_catalog.hashtext('age_revert_pg_upgrade')); DECLARE graph_rec RECORD; BEGIN @@ -375,8 +384,8 @@ BEGIN LOOP BEGIN -- Touch schema by changing owner to current_user then back to original - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); - EXECUTE format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, current_user); + EXECUTE pg_catalog.format('ALTER SCHEMA %I OWNER TO %I', graph_rec.ns_name, graph_rec.owner_name); EXCEPTION WHEN insufficient_privilege THEN RAISE NOTICE 'Could not invalidate cache for schema % (insufficient privileges)', graph_rec.ns_name; END; @@ -410,7 +419,10 @@ CREATE FUNCTION ag_catalog.age_pg_upgrade_status() message text ) LANGUAGE plpgsql - SET search_path = ag_catalog, pg_catalog + -- Resolve built-in functions and operators from pg_catalog first so they + -- are not overridden by same-named objects defined in ag_catalog. The + -- ag_catalog objects referenced here are schema-qualified. + SET search_path = pg_catalog, ag_catalog AS $function$ DECLARE ns_type text;