diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 3fc27d7255..609ba953c9 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -790,7 +790,25 @@ InitializeSessionUserId(const char *rolename, Oid roleid) { SetAuthenticatedUserId(roleid, is_superuser); - /* Set SessionUserId and related variables via the GUC mechanisms */ + /* + * Set SessionUserId and related variables, including "role", via the + * GUC mechanisms. + * + * Note: ideally we would use PGC_S_DYNAMIC_DEFAULT here, so that + * session_authorization could subsequently be changed from + * pg_db_role_setting entries. Instead, session_authorization in + * pg_db_role_setting has no effect. Changing that would require + * solving two problems: + * + * 1. If pg_db_role_setting has values for both session_authorization + * and role, we could not be sure which order those would be applied + * in, and it would matter. + * + * 2. Sites may have years-old session_authorization entries. There's + * not been any particular reason to remove them. Ending the dormancy + * of those entries could seriously change application behavior, so + * only a major release should do that. + */ SetConfigOption("session_authorization", rname, PGC_BACKEND, PGC_S_OVERRIDE); } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 79599a2c10..b733e692de 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8287,6 +8287,12 @@ set_config_option_ext(const char *name, const char *value, * expect that if "role" isn't supposed to be default, it * has been or will be set by a separate reload action. * + * Also, for the call from InitializeSessionUserId with + * source == PGC_S_OVERRIDE, use PGC_S_DYNAMIC_DEFAULT for + * "role"'s source, so that it's still possible to set + * "role" from pg_db_role_setting entries. (See notes in + * InitializeSessionUserId before changing this.) + * * A fine point: for RESET session_authorization, we do * "RESET role" not "SET ROLE NONE" (by passing down NULL * rather than "none" for the value). This would have the @@ -8299,7 +8305,9 @@ set_config_option_ext(const char *name, const char *value, (void) set_config_option_ext("role", value ? "none" : NULL, orig_context, - orig_source, + (orig_source == PGC_S_OVERRIDE) + ? PGC_S_DYNAMIC_DEFAULT + : orig_source, orig_srole, action, true, diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefile index 90d1979187..d4ff227ef0 100644 --- a/src/test/modules/unsafe_tests/Makefile +++ b/src/test/modules/unsafe_tests/Makefile @@ -1,6 +1,9 @@ # src/test/modules/unsafe_tests/Makefile -REGRESS = rolenames alter_system_table guc_privs +REGRESS = rolenames setconfig alter_system_table guc_privs +REGRESS_OPTS = \ + --create-role=regress_authenticated_user_sr \ + --create-role=regress_authenticated_user_ssa # the whole point of these tests is to not run installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/unsafe_tests/expected/setconfig.out b/src/test/modules/unsafe_tests/expected/setconfig.out new file mode 100644 index 0000000000..6a021d9ad0 --- /dev/null +++ b/src/test/modules/unsafe_tests/expected/setconfig.out @@ -0,0 +1,31 @@ +-- 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. +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_sr; +GRANT regress_session_user TO regress_authenticated_user_ssa; +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; + 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(). +\c - regress_authenticated_user_ssa +SELECT current_user, session_user; + current_user | session_user +--------------------------------+-------------------------------- + regress_authenticated_user_ssa | regress_authenticated_user_ssa +(1 row) + +RESET SESSION AUTHORIZATION; +DROP USER regress_session_user; +DROP USER regress_current_user; diff --git a/src/test/modules/unsafe_tests/sql/setconfig.sql b/src/test/modules/unsafe_tests/sql/setconfig.sql new file mode 100644 index 0000000000..8817a7c763 --- /dev/null +++ b/src/test/modules/unsafe_tests/sql/setconfig.sql @@ -0,0 +1,24 @@ +-- 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. + +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_sr; +GRANT regress_session_user TO regress_authenticated_user_ssa; +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; + +-- 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_ssa +SELECT current_user, session_user; +RESET SESSION AUTHORIZATION; +DROP USER regress_session_user; +DROP USER regress_current_user;