Opened 6 months ago

Closed 3 months ago

Last modified 3 months ago

#5525 closed defect (fixed)

ERROR: permission denied for view pg_file_settings

Reported by: robe Owned by: strk
Priority: medium Milestone: PostGIS 3.3.5
Component: build Version: 3.3.x
Keywords: pgextwlist Cc: JelteF

Description

Unfortunately the change we made at [a514af68/git] makes it impossible for Amazon DbaaS customers to create postgis_topology and postgis_tiger_geocoder

Error is

postgres=> CREATE EXTENSION postgis_topology;
ERROR:  permission denied for view pg_file_settings
CONTEXT:  SQL statement "SELECT setting
		                           FROM pg_catalog.pg_file_settings
		WHERE name OPERATOR(pg_catalog.=) 'search_path' AND applied"
PL/pgSQL function addtosearchpath(character varying) line 22 at SQL statement

I'm suspect other DbaaS will be similarly impacted.

I'm going to test this using pgextwlist to see if it triggers the same issue. If it does, I'll put pgextwlist bot in our rotation.

Change History (26)

comment:1 by robe, 6 months ago

Another thing that was noted to me is postgis_tiger_geocoder is marked as super_user="no" so it's confusing it tries to use pg_file_settings

comment:2 by robe, 6 months ago

I'm moving toward the idea of keeping pg_file_settings for super user installs, but falling back on old way for non-super-user. I still need to rebuild twisttie and put twisttie back in service

comment:3 by robe, 5 months ago

Owner: changed from strk to robe

comment:4 by strk, 5 months ago

I wonder if we should stop changing search_path completely, isn't it somewhat invasive ?

comment:5 by robe, 5 months ago

It would be a breaking change to stop doing that in a 3.4.0, 3.3. We can make that change in 3.5.0 if we decide. Sadly the whole search_path thing is mighty confusing to people new to PostgreSQL so we'd have to give detailed instructions people on how to do it in release notes if we make the change.

I think it's more invasive to most people using topology and tiger_geocoder to stop doing it, but we'll see. I don't think I currently schema qualify all uses in tiger_geocoder (well definitely not PostGIS uses). And topology is not currently schema qualifying. At anyrate we won't be able to make the fix for people who are using < PG16

Last edited 5 months ago by robe (previous) (diff)

comment:6 by strk, 5 months ago

Keywords: pgextwlist added

comment:7 by strk, 4 months ago

Cc: github-tech@… added

Jelte as the author of the change, do you want to take care of this ?

comment:8 by strk, 4 months ago

Cc: JelteF added; github-tech@… removed

comment:9 by JelteF, 4 months ago

I sadly don't really have time in the near future to work on this. But it sounds like simply removing the pg_file_settings part would be a good fix.

comment:10 by strk, 4 months ago

Owner: changed from robe to strk
Status: newassigned
Version: 3.4.x3.3.x

comment:11 by Sandro Santilli <strk@…>, 4 months ago

In d20fc93/git:

Remove duplicated AddToSearchPath implementation

Use a common one, in new libpgcommon/sql directory.

References #5525

comment:12 by Sandro Santilli <strk@…>, 4 months ago

In 6f08541/git:

Remove duplicated AddToSearchPath implementation

Use a common one, in new libpgcommon/sql directory.

References #5525 in 3.4 branch

comment:13 by Sandro Santilli <strk@…>, 4 months ago

In 8131a1d0/git:

Remove duplicated AddToSearchPath implementation

Use a common one, in new libpgcommon/sql directory.

References #5525 in 3.3 branch

comment:14 by strk, 4 months ago

Now the de-duplication is in place, as far as into 3.3 branch - we can fix first and then see how to move forward.

The referenced breaking change moved from using pg_settings to use pg_file_settings, what was the rationale for that ?

Was the querying of pg_settings safe under pgextwlist ? Should we postpone the fix to after our CI can run under pgextwlist ? See #5566

comment:15 by strk, 4 months ago

Summary: Revert change of reliance on pg_file_settings for postgis_topology and postgis_tiger_geocoderERROR: permission denied for view pg_file_settings

comment:16 by strk, 4 months ago

The original problem description is in #5442 which reports 2 problems:

  1. Changing boot value based on current value rather than bool value
  2. Changing session value based on boot val rather than current val

Jeff is my understanding of the problem correct ?

Regina: would there be any problem to set per-function search_path rather than fully qualified all those pg_catalog calls ? That hurts my eyes :) My proposed solution for that is in https://git.osgeo.org/gitea/postgis/postgis/pulls/161

in reply to:  16 comment:17 by robe, 4 months ago

Regina: would there be any problem to set per-function search_path rather than fully qualified all those pg_catalog calls ? That hurts my eyes :) My proposed solution for that is in https://git.osgeo.org/gitea/postgis/postgis/pulls/161

I had tried the search_path function specific in the PostGIS 2.3/2.4 days, but it killed performance badly, so I had to take it out.

The discussion about it is here - https://www.postgresql.org/message-id/flat/CA%2BTgmoYo1eR3011SLSbm2K%2B4X4RtYGuDuhNSkuYMgH7fomMCRg%40mail.gmail.com#1cb2569a901d89212bb3a9225ee947eb

comment:18 by robe, 4 months ago

@strk If you just do it for the AddSEarchPath one as you did in https://git.osgeo.org/gitea/postgis/postgis/pulls/161 that's fine, since we don't care about losing a second or so of time for AddSearchPath. Just can't do it for all our functions.

comment:19 by strk, 4 months ago

Thank, I'm not planning to do it everywhere. Being specific about schema should be generally faster too (no need to go look in each schema of the search path).

comment:20 by strk, 4 months ago

Regina were you able to reproduce this problem ? CI is now running CREATE EXTENSION as an unprivileged user under pgextwlist and not showing any problem. I also cannot reproduce:

[strk@c19:/usr/local/src/postgis/postgis(main)] createdb test
[strk@c19:/usr/local/src/postgis/postgis(main)] psql -f regress/hooks/configure-pgextwlist.sql test
DO
[strk@c19:/usr/local/src/postgis/postgis(main)] psql test
psql (13.5 (Ubuntu 13.5-0ubuntu0.21.04.1))
Type "help" for help.

test=# set role postgis_sandboxed_user;
SET
test=> create extension postgis_topology cascade;
NOTICE:  installing required extension "postgis"
CREATE EXTENSION
test=> select rolsuper from pg_roles where rolname = current_user;
 rolsuper 
----------
 f
(1 row)

test=> 

comment:21 by robe, 4 months ago

Sorry haven't taken a look. Will do so this week.

comment:22 by robe, 3 months ago

Okay I looked at this more, pg_catalog.pg_db_role_setting is readable by nonsuper users so that's fine, but if that returns nothing, then we run into the next loop. So I'm going to put a superuser check around that to prevent anyone that it will error on from falling into that loop.

comment:23 by robe, 3 months ago

I was able to replicate the issue.

The postgis_tiger_geocoder is marked as nonsuperuser, so regular users can install it even without pgextwlist

Do as a super user

CREATE EXTENSION postgis;
CREATE EXTENSION fuzzystrmatch;

Then do as a non-super user

CREATE EXTENSION postgis_tiger_geocoder;

Yields error:

ERROR:  permission denied for view pg_file_settings
CONTEXT:  SQL statement "SELECT setting
		                           FROM pg_file_settings
		WHERE name = 'search_path' AND applied"
PL/pgSQL function tiger.postgis_extension_addtosearchpath(text) line 23 at SQL statement 

SQL state: 42501

comment:24 by Regina Obe <lr@…>, 3 months ago

In 645c0b9/git:

Don't use pg_file_settings unless person creating extension
is a super user
References #5525 for PostGIS 3.3.5

comment:25 by Regina Obe <lr@…>, 3 months ago

Resolution: fixed
Status: assignedclosed

In ef657b9/git:

Regression failure when installed by non-superuser
Closes #5525 for PostGIS 3.4.1

comment:26 by Regina Obe <lr@…>, 3 months ago

In d1abcc12/git:

Regression failure when installed by non-superuser
Closes #5525 for PostGIS 3.5.0

Note: See TracTickets for help on using tickets.