Date: Sat, 28 May 2022 20:30:33 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-bugs@lists.postgresql.org,
	=?iso-8859-1?Q?F=E4rber=2C_Franz-Josef_=28StMUK=29?=
 <Franz-Josef.Faerber@stmuk.bayern.de>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Extension pg_trgm, permissions and pg_dump order
Message-ID: <20220529033033.GA1973706@nathanxps13>
References: <f8a4105f076544c180a87ef0c4822352@stmuk.bayern.de>
 <1961551.1653498155@sss.pgh.pa.us>
 <20220526055047.GA3153526@rfd.leadboat.com>
 <20220528045122.GA3228881@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="X1bOJ3K7DJ5YkBrT"
Content-Disposition: inline
In-Reply-To: <20220528045122.GA3228881@rfd.leadboat.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220529033033.GA1973706%40nathanxps13>
Precedence: bulk


--X1bOJ3K7DJ5YkBrT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, May 27, 2022 at 09:51:22PM -0700, Noah Misch wrote:
> On Wed, May 25, 2022 at 10:50:47PM -0700, Noah Misch wrote:
>> BEGIN;
>> CREATE ROLE limitedrole;
>> CREATE SCHEMA ext_trgm;
>> CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
>> CREATE TABLE x(y text);
>> ALTER TABLE x OWNER TO limitedrole;
>> CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
>> ROLLBACK;
> 
>> Not too simple, no.  The parts of DefineIndex() that execute user code
>> (e.g. DefineIndex->ComputeIndexAttrs->CheckMutability) are interspersed with
>> the parts that do permissions checks, like the one yielding $SUBJECT at
>> DefineIndex->ComputeIndexAttrs->ResolveOpClass->LookupExplicitNamespace.  My
>> first candidate is to undo the userid switch before the ResolveOpClass() call
>> and restore it after.  My second candidate is to pass down the userid we want
>> used for this sort of permissions check.  Depending on the full list of call
>> stacks reaching permissions checks, this could get hairy.
> 
> While I'd value the opportunity to work on this, there only a 50% chance I
> could get it done by 2022-08-01.  I've set aside four hours on 2022-06-12 to
> see how far I get.  For a 95% chance, the date would be 2023-02-01.  (I've
> already canceled a 2022-07 vacation for the thing taking my time instead;
> there's nothing clearly left to cut.)  If anyone would like it done faster
> than that, I welcome that person taking over.

I'd like to help.  I started looking at the problem and hacked together a
proof-of-concept based on your first candidate that seems to fix the
reported issue.  However, from the upthread discussion, it is not clear
whether there is agreement on the approach.  IIUC there are still many
other code paths that would require a similar treatment, so perhaps
identifying all of those would be a good next step.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--X1bOJ3K7DJ5YkBrT
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="fix_pg_dump_proof_of_concept.patch"

diff --git a/contrib/pg_trgm/expected/pg_trgm.out b/contrib/pg_trgm/expected/pg_trgm.out
index 20141ce7f3..060fd8e23d 100644
--- a/contrib/pg_trgm/expected/pg_trgm.out
+++ b/contrib/pg_trgm/expected/pg_trgm.out
@@ -5402,3 +5402,18 @@ SELECT DISTINCT city, similarity(city, 'Warsaw'), show_limit()
  Warsaw |          1 |        0.5
 (1 row)
 
+DROP EXTENSION pg_trgm CASCADE;
+NOTICE:  drop cascades to 4 other objects
+DETAIL:  drop cascades to index test2_idx_gist
+drop cascades to index restaurants_city_idx
+drop cascades to index trgm_idx
+drop cascades to index t_trgm_idx
+BEGIN;
+CREATE ROLE limitedrole;
+CREATE SCHEMA ext_trgm;
+CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
+CREATE TABLE x(y text);
+ALTER TABLE x OWNER TO limitedrole;
+CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
+ROLLBACK;
+CREATE EXTENSION pg_trgm;
diff --git a/contrib/pg_trgm/sql/pg_trgm.sql b/contrib/pg_trgm/sql/pg_trgm.sql
index 6a9da24d5a..00917f8085 100644
--- a/contrib/pg_trgm/sql/pg_trgm.sql
+++ b/contrib/pg_trgm/sql/pg_trgm.sql
@@ -236,3 +236,14 @@ SELECT DISTINCT city, similarity(city, 'Warsaw'), show_limit()
 SELECT set_limit(0.5);
 SELECT DISTINCT city, similarity(city, 'Warsaw'), show_limit()
   FROM restaurants WHERE city % 'Warsaw';
+
+DROP EXTENSION pg_trgm CASCADE;
+BEGIN;
+CREATE ROLE limitedrole;
+CREATE SCHEMA ext_trgm;
+CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
+CREATE TABLE x(y text);
+ALTER TABLE x OWNER TO limitedrole;
+CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
+ROLLBACK;
+CREATE EXTENSION pg_trgm;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index eac13ac0b7..d4c718b696 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -80,7 +80,9 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
 							  Oid relId,
 							  const char *accessMethodName, Oid accessMethodId,
 							  bool amcanorder,
-							  bool isconstraint);
+							  bool isconstraint,
+							  Oid root_save_userid,
+							  int root_save_sec_context);
 static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 List *colnames, List *exclusionOpNames,
 							 bool primary, bool isconstraint);
@@ -236,7 +238,7 @@ CheckIndexCompatible(Oid oldId,
 					  coloptions, attributeList,
 					  exclusionOpNames, relationId,
 					  accessMethodName, accessMethodId,
-					  amcanorder, isconstraint);
+					  amcanorder, isconstraint, 0, 0);
 
 
 	/* Get the soon-obsolete pg_index tuple. */
@@ -890,7 +892,8 @@ DefineIndex(Oid relationId,
 					  coloptions, allIndexParams,
 					  stmt->excludeOpNames, relationId,
 					  accessMethodName, accessMethodId,
-					  amcanorder, stmt->isconstraint);
+					  amcanorder, stmt->isconstraint, root_save_userid,
+					  root_save_sec_context);
 
 	/*
 	 * Extra checks when creating a PRIMARY KEY index.
@@ -1743,12 +1746,16 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 				  const char *accessMethodName,
 				  Oid accessMethodId,
 				  bool amcanorder,
-				  bool isconstraint)
+				  bool isconstraint,
+				  Oid root_save_userid,
+				  int root_save_sec_context)
 {
 	ListCell   *nextExclOp;
 	ListCell   *lc;
 	int			attn;
 	int			nkeycols = indexInfo->ii_NumIndexKeyAttrs;
+	Oid			saved_userid;
+	int			saved_sec_ctx;
 
 	/* Allocate space for exclusion operator info, if needed */
 	if (exclusionOpNames)
@@ -1922,6 +1929,12 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 
 		collationOidP[attn] = attcollation;
 
+		if (OidIsValid(root_save_userid))
+		{
+			GetUserIdAndSecContext(&saved_userid, &saved_sec_ctx);
+			SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+		}
+
 		/*
 		 * Identify the opclass to use.
 		 */
@@ -1930,6 +1943,9 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 										 accessMethodName,
 										 accessMethodId);
 
+		if (OidIsValid(root_save_userid))
+			SetUserIdAndSecContext(saved_userid, saved_sec_ctx);
+
 		/*
 		 * Identify the exclusion operator, if any.
 		 */

--X1bOJ3K7DJ5YkBrT--