Re: Fix search_path for all maintenance commands

2024-01-17 Thread Jeff Davis
On Thu, 2024-01-18 at 09:24 +0530, Shubham Khanna wrote: > I tried to apply the patch but it is failing at the Head. It is > giving > the following error: I am withdrawing this patch from the CF until it's more clear that this is what we really want to do. Thank you for looking into it.

Re: Fix search_path for all maintenance commands

2024-01-17 Thread Shubham Khanna
On Thu, Jan 18, 2024 at 9:19 AM Jeff Davis wrote: > > On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote: > > Based on feedback, I plan to commit soon. > > Attached is a new version. > > Changes: > > * Also switch the search_path during CREATE MATERIALIZED VIEW, so that > it's consistent with

Re: Fix search_path for all maintenance commands

2023-11-07 Thread Jeff Davis
On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote: > Perhaps the search_path for running a maintenance command should be > the search_path set for the table owner (ALTER ROLE … SET search_path > …)? After some thought, I don't think that's the right approach. It adds another way search path

Re: Fix search_path for all maintenance commands

2023-11-06 Thread Isaac Morland
On Mon, 6 Nov 2023 at 15:54, Tom Lane wrote: > Isaac Morland writes: > > I still think the right default is that CREATE FUNCTION stores the > > search_path in effect when it runs with the function, and that is the > > search_path used to run the function (and don't "BEGIN ATOMIC" functions > >

Re: Fix search_path for all maintenance commands

2023-11-06 Thread Tom Lane
Isaac Morland writes: > I still think the right default is that CREATE FUNCTION stores the > search_path in effect when it runs with the function, and that is the > search_path used to run the function (and don't "BEGIN ATOMIC" functions > partially work this way already?). I don't see how that

Re: Fix search_path for all maintenance commands

2023-11-06 Thread Isaac Morland
On Thu, 2 Nov 2023 at 14:22, Jeff Davis wrote: > On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote: > > > Perhaps the search_path for running a maintenance command should be > > the search_path set for the table owner (ALTER ROLE … SET search_path > > …)? > > That's an interesting idea; I

Re: Fix search_path for all maintenance commands

2023-11-02 Thread Jeff Davis
On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote: > Perhaps the search_path for running a maintenance command should be > the search_path set for the table owner (ALTER ROLE … SET search_path > …)? That's an interesting idea; I hadn't considered that, or at least not very deeply. I feel

Re: Fix search_path for all maintenance commands

2023-10-31 Thread Isaac Morland
On Fri, 27 Oct 2023 at 19:04, Jeff Davis wrote: The approach of locking down search_path during maintenance commands > would solve the problem, but it means that we are enforcing search_path > in some contexts and not others. That's not great, but it's similar to > what we are doing when we

Re: Fix search_path for all maintenance commands

2023-10-31 Thread Nathan Bossart
On Fri, Oct 27, 2023 at 04:04:26PM -0700, Jeff Davis wrote: > Do we still want to do this? > > Right now, the MAINTAIN privilege is blocking on some way to prevent > malicious users from abusing the MAINTAIN privilege and search_path to > acquire the table owner's privileges. I vote +1 for

Re: Fix search_path for all maintenance commands

2023-10-27 Thread Jeff Davis
On Fri, 2023-07-21 at 15:32 -0700, Jeff Davis wrote: > Attached is a new version. Do we still want to do this? Right now, the MAINTAIN privilege is blocking on some way to prevent malicious users from abusing the MAINTAIN privilege and search_path to acquire the table owner's privileges. The

Re: Fix search_path for all maintenance commands

2023-07-22 Thread Jeff Davis
On Sat, 2023-07-22 at 07:04 -0700, Noah Misch wrote: > Commit a117ceb added that, and it added some test cases that behaved > differently without that. Thank you. The reasoning there seems to apply to search_path restriction as well, so I will leave it as-is. I'll wait a few more days for

Re: Fix search_path for all maintenance commands

2023-07-22 Thread Noah Misch
On Fri, Jul 21, 2023 at 03:32:43PM -0700, Jeff Davis wrote: > Why do we switch to the table owner and use > SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in > index_build (etc.) anyway? Commit a117ceb added that, and it added some test cases that behaved differently without

Re: Fix search_path for all maintenance commands

2023-07-21 Thread Jeff Davis
On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote: > Based on feedback, I plan to commit soon. Attached is a new version. Changes: * Also switch the search_path during CREATE MATERIALIZED VIEW, so that it's consistent with REFRESH. As a part of this change, I slightly reordered things in

Re: Fix search_path for all maintenance commands

2023-07-17 Thread Jeff Davis
On Mon, 2023-07-17 at 10:58 -0700, Nathan Bossart wrote: > On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote: > > The 2018 security fixes instigated many function repairs that > > $SUBJECT would > > otherwise instigate.  That wasn't too painful.  The net new pain of > > $SUBJECT > > will

Re: Fix search_path for all maintenance commands

2023-07-17 Thread Nathan Bossart
On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote: > The 2018 security fixes instigated many function repairs that $SUBJECT would > otherwise instigate. That wasn't too painful. The net new pain of $SUBJECT > will be less, since the 2018 security fixes prepared the path. Hence, I >

Re: Fix search_path for all maintenance commands

2023-07-15 Thread Noah Misch
On Thu, Jul 13, 2023 at 02:07:27PM -0700, David G. Johnston wrote: > On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh wrote: > > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston > > wrote: > > > I'm against simply breaking the past without any recourse as what we > > did for pg_dump/pg_restore

Re: Fix search_path for all maintenance commands

2023-07-13 Thread Jeff Davis
On Thu, 2023-07-13 at 13:37 -0700, David G. Johnston wrote: > If this is limited to MAINT, which I'm in support of, there is no > need for an "escape hatch".  A prerequisite for leveraging the new > feature is that you fix the code so it conforms to the new way of > doing things. The current

Re: Fix search_path for all maintenance commands

2023-07-13 Thread David G. Johnston
On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh wrote: > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston > wrote: > > > > I'm against simply breaking the past without any recourse as what we > did for pg_dump/pg_restore still bothers me. > > I'm sure this is tangential, but can you please

Re: Fix search_path for all maintenance commands

2023-07-13 Thread Gurjeet Singh
On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston wrote: > > I'm against simply breaking the past without any recourse as what we did for > pg_dump/pg_restore still bothers me. I'm sure this is tangential, but can you please provide some context/links to the change you're referring to here.

Re: Fix search_path for all maintenance commands

2023-07-13 Thread David G. Johnston
On Thu, Jul 13, 2023 at 12:54 PM Gurjeet Singh wrote: > > The approach seems good to me. My concern is with this change's > potential to cause an extended database outage. Hence sending it out > as part of v16, without any escape hatch for the DBA, is my objection. > > If this is limited to

Re: Fix search_path for all maintenance commands

2023-07-13 Thread Gurjeet Singh
On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis wrote: > > The current approach seemed to get support from Noah, Nathan, and Greg > Stark. > > Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but I didn't see Tom's or Robert's concerns raised in this thread. I see now that for some

Re: Fix search_path for all maintenance commands

2023-07-13 Thread Jeff Davis
On Thu, 2023-07-06 at 18:39 -0700, Jeff Davis wrote: > * The fix might not go far enough or might be in the wrong place. I'm > open to suggestion here, too. Maybe we can make it part of the > general > function call mechanism, and can be overridden by explicitly setting > the function search

Re: Fix search_path for all maintenance commands

2023-07-07 Thread Jeff Davis
Hi, On Thu, 2023-07-06 at 23:22 -0400, Isaac Morland wrote: > Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" > to any function used in a functional index that doesn't have a > search_path setting of its own? I don't think we want to go down the road of trying to solve this

Re: Fix search_path for all maintenance commands

2023-07-06 Thread Isaac Morland
On Thu, 6 Jul 2023 at 21:39, Jeff Davis wrote: I apologize in advance if anything I’ve written below is either too obvious or too crazy or misinformed to belong here. I hope I have something to say that is on point, but feel unsure what makes sense to say. * It might break for users who have a

Re: Fix search_path for all maintenance commands

2023-07-06 Thread Jeff Davis
On Fri, 2023-05-26 at 16:21 -0700, Jeff Davis wrote: > Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, > REINDEX, and VACUUM) currently run as the table owner, and as a > SECURITY_RESTRICTED_OPERATION. > > I propose that we also fix the search_path to "pg_catalog, pg_temp" >

Re: Fix search_path for all maintenance commands

2023-06-09 Thread Jeff Davis
On Fri, 2023-06-09 at 16:29 -0700, Gurjeet Singh wrote: > I'm not sure if mine is a valid concern, and it has been a long time > since I looked at the search_path's and switching Role's implications > (CVE-2009-4136) so pardon my ignorance. > > It feels a bit late in release cycle to introduce

Re: Fix search_path for all maintenance commands

2023-06-09 Thread Gurjeet Singh
On Fri, Jun 9, 2023 at 2:00 PM Jeff Davis wrote: > > On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote: > > On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote: > > > I guess that's pretty narrow and a reasonable thing to desupport. > > > Users could just mark those functions with

Re: Fix search_path for all maintenance commands

2023-06-09 Thread Jeff Davis
On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote: > On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote: > > I guess that's pretty narrow and a reasonable thing to desupport. > > Users could just mark those functions with search_path or schema > > qualify the object references in

Re: Fix search_path for all maintenance commands

2023-06-08 Thread Nathan Bossart
On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote: > I guess that's pretty narrow and a reasonable thing to desupport. > Users could just mark those functions with search_path or schema > qualify the object references in them. Perhaps we should also be > picking up cases like that sooner

Re: Fix search_path for all maintenance commands

2023-06-08 Thread Greg Stark
On Fri, 26 May 2023 at 19:22, Jeff Davis wrote: > > Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, > REINDEX, and VACUUM) currently run as the table owner, and as a > SECURITY_RESTRICTED_OPERATION. > > I propose that we also fix the search_path to "pg_catalog, pg_temp" > when

Fix search_path for all maintenance commands

2023-05-26 Thread Jeff Davis
Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, and VACUUM) currently run as the table owner, and as a SECURITY_RESTRICTED_OPERATION. I propose that we also fix the search_path to "pg_catalog, pg_temp" when running maintenance commands, for two reasons: 1. Make the