public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: David Steele <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Possible regression setting GUCs on \connect
Date: Thu, 27 Apr 2023 15:22:04 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
David Steele <[email protected]> writes:
> On 4/27/23 21:16, Tom Lane wrote:
>> I tried to replicate this per that recipe, but it works for me:
> I included the errors in the expect log so I could link to them. So test
> success means the error is happening.
Ah, got it.
I added some debug output to the test, and what I see is that
at the "\connect - user1" commands that work ok, what we have
in pg_db_role_setting is along the lines of
+select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
+ setdatabase | setrole | setconfig | setuser
+-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
+ 0 | user1 | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_relation=on} | {f,f,f,f,f}
...
while where it's not working:
+select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
+ setdatabase | setrole | setconfig | setuser
+-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
+ 0 | user1 | {pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_statement=off} | {t,f,f,f}
...
So it is failing because setuser = 't' for that setting; which makes the
failure itself unsurprising, but it seems like the flag should not
have been set that way. Digging further, it looks like the flag array
is not correctly updated during ALTER USER RESET:
select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
NOTICE: AUDIT: SESSION,1,1,READ,SELECT,TABLE,pg_catalog.pg_db_role_setting,"select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;",<not logged>
setdatabase | setrole | setconfig | setuser
-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
0 | user1 | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_relation=on} | {f,f,f,f,f}
... that's fine ...
ALTER ROLE user1 RESET pgaudit.log_relation;
select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
setdatabase | setrole | setconfig | setuser
-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
0 | user1 | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor} | {t,f,f,f}
... wrong ...
ALTER ROLE user1 RESET pgaudit.log;
select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
setdatabase | setrole | setconfig | setuser
-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
0 | user1 | {pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor} | {t,f,f}
... and wronger.
I had not paid any attention to 096dd80f3 when it went in, but now
that I have looked at it I'm quite distressed, independently of this
probably-minor bug. It seems to me that this feature is not well
designed and completely ignores the precedents set by commits
a0ffa885e and 13d838815. The right way to do this was not to add some
poorly-explained option to ALTER ROLE, but to record the role OID that
issued the ALTER ROLE, and then to check when loading the ALTER ROLE
setting whether that role (still) has the right to change the
specified setting. As implemented, this can't possibly track changes
in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
introducing outright security holes like the one fixed by 13d838815.
I think we ought to revert 096dd80f3 and try again in v17.
regards, tom lane
view thread (22+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Possible regression setting GUCs on \connect
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox