Date: Sat, 16 Nov 2024 10:34:59 -0800
From: Noah Misch <noah@leadboat.com>
To: Matthew Woodcraft <matthew@woodcraft.me.uk>,
	Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org,
	Etienne LAFARGE <etienne.lafarge@gmail.com>
Subject: Re: Today's Postgres Releases break login roles
Message-ID: <20241116183459.93.nmisch@google.com>
References: 
 <CADOZwSb0UsEr4_UTFXC5k7=fyyK8uKXekucd+-uuGjJsGBfxgw@mail.gmail.com>
 <87iksnsbhx.fsf@golux.woodcraft.me.uk>
 <2742790.1731771946@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Lz/uPAsoTFXKuyjF"
Content-Disposition: inline
In-Reply-To: <2742790.1731771946@sss.pgh.pa.us>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241116183459.93.nmisch%40google.com>
Precedence: bulk


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

On Sat, Nov 16, 2024 at 10:45:46AM -0500, Tom Lane wrote:
> Matthew Woodcraft <matthew@woodcraft.me.uk> writes:
> > I noticed one other change with the 14.14 point release, which I think
> > is likely to be related.
> > Previously a command like this would work to select a role immediately after
> > login:
> > psql "options='-crole=foo'"
> > With 14.14 as far as I can tell it has no effect.
> 
> Yeah, same issue.

Agreed.  I cut some corners by not directly testing that or ALTER DATABASE, so
here are some more tests.  The output is the same on master @ now, on master @
2024-11-10 (before the regression), and on v12.  It reproduces the trouble on
master @ 2024-11-12.  That's sufficient confirmation for the moment, so I'll
hold this until the releases wrap and make it master-only.

--Lz/uPAsoTFXKuyjF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="test-PGOPTIONS-role-v1.patch"

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Test "options=-crole=" and "ALTER DATABASE SET role".
    
    Commit 7b88529f4363994450bd4cd3c172006a8a77e222 fixed a regression
    spanning these features, but it didn't test them.  It did test code
    paths sufficient for their present implementations, so no back-patch.
    
    Reported by Matthew Woodcraft.  Reviewed by FIXME.
    
    Discussion: https://postgr.es/m/87iksnsbhx.fsf@golux.woodcraft.me.uk

diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefile
index d4ff227..a85c854 100644
--- a/src/test/modules/unsafe_tests/Makefile
+++ b/src/test/modules/unsafe_tests/Makefile
@@ -2,6 +2,8 @@
 
 REGRESS = rolenames setconfig alter_system_table guc_privs
 REGRESS_OPTS = \
+	--create-role=regress_authenticated_user_db_sr \
+	--create-role=regress_authenticated_user_db_ssa \
 	--create-role=regress_authenticated_user_sr \
 	--create-role=regress_authenticated_user_ssa
 
diff --git a/src/test/modules/unsafe_tests/expected/setconfig.out b/src/test/modules/unsafe_tests/expected/setconfig.out
index 6a021d9..5f42443 100644
--- a/src/test/modules/unsafe_tests/expected/setconfig.out
+++ b/src/test/modules/unsafe_tests/expected/setconfig.out
@@ -1,24 +1,92 @@
 -- This is borderline unsafe in that an additional login-capable user exists
 -- during the test run.  Under installcheck, a too-permissive pg_hba.conf
 -- might allow unwanted logins as regress_authenticated_user_ssa.
+-- Setup catalog state.
+ALTER USER regress_authenticated_user_db_ssa superuser;
 ALTER USER regress_authenticated_user_ssa superuser;
 CREATE ROLE regress_session_user;
 CREATE ROLE regress_current_user;
+GRANT regress_current_user TO regress_authenticated_user_db_sr;
 GRANT regress_current_user TO regress_authenticated_user_sr;
+GRANT regress_session_user TO regress_authenticated_user_db_ssa;
 GRANT regress_session_user TO regress_authenticated_user_ssa;
+DO $$BEGIN EXECUTE format(
+	'ALTER DATABASE %I SET session_authorization = regress_session_user',
+	current_catalog); END$$;
 ALTER ROLE regress_authenticated_user_ssa
 	SET session_authorization = regress_session_user;
 ALTER ROLE regress_authenticated_user_sr SET ROLE = regress_current_user;
-\c - regress_authenticated_user_sr
+-- Test ALTER DATABASE consequences
+-- The longstanding historical behavior is that session_authorization in
+-- setconfig has no effect.  Hence, session_user remains
+-- regress_authenticated_user_ssa.  See comment in InitializeSessionUserId().
+\c - regress_authenticated_user_db_ssa
+SELECT current_user, session_user;
+           current_user            |           session_user            
+-----------------------------------+-----------------------------------
+ regress_authenticated_user_db_ssa | regress_authenticated_user_db_ssa
+(1 row)
+
+-- We document "The DEFAULT and RESET forms reset the session and current user
+-- identifiers to be the originally authenticated user name."  If we let
+-- session_authorization in setconfig have an effect, we'll need to decide
+-- whether to make RESET differ from DEFAULT.
+RESET SESSION AUTHORIZATION;
+SELECT current_user, session_user;
+           current_user            |           session_user            
+-----------------------------------+-----------------------------------
+ regress_authenticated_user_db_ssa | regress_authenticated_user_db_ssa
+(1 row)
+
+DO $$BEGIN
+	EXECUTE format(
+		'ALTER DATABASE %I RESET session_authorization', current_catalog);
+	EXECUTE format(
+		'ALTER DATABASE %I SET role = regress_current_user', current_catalog);
+END$$;
+\c - regress_authenticated_user_db_sr
+SELECT current_user, session_user;
+     current_user     |           session_user           
+----------------------+----------------------------------
+ regress_current_user | regress_authenticated_user_db_sr
+(1 row)
+
+-- Back to superuser, to reverse ALTER DATABASE
+\c - regress_authenticated_user_db_ssa
+SELECT current_user, session_user;
+     current_user     |           session_user            
+----------------------+-----------------------------------
+ regress_current_user | regress_authenticated_user_db_ssa
+(1 row)
+
+SET ROLE NONE;
+DO $$BEGIN EXECUTE format(
+	'ALTER DATABASE %I RESET role', current_catalog); END$$;
+-- Test connection string options
+\c -reuse-previous=on "user=regress_authenticated_user_db_sr options=-crole=regress_current_user"
+SELECT current_user, session_user;
+     current_user     |           session_user           
+----------------------+----------------------------------
+ regress_current_user | regress_authenticated_user_db_sr
+(1 row)
+
+-- As above, session_authorization has no effect.
+\c -reuse-previous=on "user=regress_authenticated_user_db_ssa options=-csession_authorization=regress_session_user"
+SELECT current_user, session_user;
+           current_user            |           session_user            
+-----------------------------------+-----------------------------------
+ regress_authenticated_user_db_ssa | regress_authenticated_user_db_ssa
+(1 row)
+
+-- Test ALTER ROLE consequences
+\c  -reuse-previous=on "user=regress_authenticated_user_sr options="
 SELECT current_user, session_user;
      current_user     |         session_user          
 ----------------------+-------------------------------
  regress_current_user | regress_authenticated_user_sr
 (1 row)
 
--- The longstanding historical behavior is that session_authorization in
--- setconfig has no effect.  Hence, session_user remains
--- regress_authenticated_user_ssa.  See comment in InitializeSessionUserId().
+-- As above, session_authorization has no effect.
 \c - regress_authenticated_user_ssa
 SELECT current_user, session_user;
           current_user          |          session_user          
diff --git a/src/test/modules/unsafe_tests/meson.build b/src/test/modules/unsafe_tests/meson.build
index 3e174c7..cf1480a 100644
--- a/src/test/modules/unsafe_tests/meson.build
+++ b/src/test/modules/unsafe_tests/meson.build
@@ -11,7 +11,9 @@ tests += {
       'alter_system_table',
       'guc_privs',
     ],
-    'regress_args': ['--create-role=regress_authenticated_user_sr',
+    'regress_args': ['--create-role=regress_authenticated_user_db_sr',
+                     '--create-role=regress_authenticated_user_db_ssa',
+                     '--create-role=regress_authenticated_user_sr',
                      '--create-role=regress_authenticated_user_ssa'],
     'runningcheck': false,
   },
diff --git a/src/test/modules/unsafe_tests/sql/setconfig.sql b/src/test/modules/unsafe_tests/sql/setconfig.sql
index 8817a7c..81296d1 100644
--- a/src/test/modules/unsafe_tests/sql/setconfig.sql
+++ b/src/test/modules/unsafe_tests/sql/setconfig.sql
@@ -2,21 +2,70 @@
 -- during the test run.  Under installcheck, a too-permissive pg_hba.conf
 -- might allow unwanted logins as regress_authenticated_user_ssa.
 
+-- Setup catalog state.
+ALTER USER regress_authenticated_user_db_ssa superuser;
 ALTER USER regress_authenticated_user_ssa superuser;
 CREATE ROLE regress_session_user;
 CREATE ROLE regress_current_user;
+GRANT regress_current_user TO regress_authenticated_user_db_sr;
 GRANT regress_current_user TO regress_authenticated_user_sr;
+GRANT regress_session_user TO regress_authenticated_user_db_ssa;
 GRANT regress_session_user TO regress_authenticated_user_ssa;
+DO $$BEGIN EXECUTE format(
+	'ALTER DATABASE %I SET session_authorization = regress_session_user',
+	current_catalog); END$$;
 ALTER ROLE regress_authenticated_user_ssa
 	SET session_authorization = regress_session_user;
 ALTER ROLE regress_authenticated_user_sr SET ROLE = regress_current_user;
 
-\c - regress_authenticated_user_sr
-SELECT current_user, session_user;
+
+-- Test ALTER DATABASE consequences
 
 -- The longstanding historical behavior is that session_authorization in
 -- setconfig has no effect.  Hence, session_user remains
 -- regress_authenticated_user_ssa.  See comment in InitializeSessionUserId().
+\c - regress_authenticated_user_db_ssa
+SELECT current_user, session_user;
+-- We document "The DEFAULT and RESET forms reset the session and current user
+-- identifiers to be the originally authenticated user name."  If we let
+-- session_authorization in setconfig have an effect, we'll need to decide
+-- whether to make RESET differ from DEFAULT.
+RESET SESSION AUTHORIZATION;
+SELECT current_user, session_user;
+DO $$BEGIN
+	EXECUTE format(
+		'ALTER DATABASE %I RESET session_authorization', current_catalog);
+	EXECUTE format(
+		'ALTER DATABASE %I SET role = regress_current_user', current_catalog);
+END$$;
+
+\c - regress_authenticated_user_db_sr
+SELECT current_user, session_user;
+
+-- Back to superuser, to reverse ALTER DATABASE
+\c - regress_authenticated_user_db_ssa
+SELECT current_user, session_user;
+SET ROLE NONE;
+DO $$BEGIN EXECUTE format(
+	'ALTER DATABASE %I RESET role', current_catalog); END$$;
+
+
+-- Test connection string options
+
+\c -reuse-previous=on "user=regress_authenticated_user_db_sr options=-crole=regress_current_user"
+SELECT current_user, session_user;
+
+-- As above, session_authorization has no effect.
+\c -reuse-previous=on "user=regress_authenticated_user_db_ssa options=-csession_authorization=regress_session_user"
+SELECT current_user, session_user;
+
+
+-- Test ALTER ROLE consequences
+
+\c  -reuse-previous=on "user=regress_authenticated_user_sr options="
+SELECT current_user, session_user;
+
+-- As above, session_authorization has no effect.
 \c - regress_authenticated_user_ssa
 SELECT current_user, session_user;
 RESET SESSION AUTHORIZATION;

--Lz/uPAsoTFXKuyjF--