Re: Preventing non-superusers from altering session authorization

2023-07-13 Thread Nathan Bossart 
On Wed, Jul 12, 2023 at 09:37:57PM -0700, Nathan Bossart wrote:
> On Mon, Jul 10, 2023 at 01:49:55PM -0700, Nathan Bossart wrote:
>> Great.  I'm going to wait a few more days in case anyone has additional
>> feedback, but otherwise I intend to commit this shortly.
> 
> I've committed 0001 for now.  I'm hoping to commit the other two patches
> within the next couple of days.

Committed.  I dwelled on whether to proceed with this change because it
doesn't completely solve the originally-stated problem; i.e., a role that
has changed its session authorization before losing superuser can still
take advantage of the privileges of the target role, which might include
reaquiring superuser.  However, I think SET ROLE is subject to basically
the same problem, and I'd argue that this change is strictly an
improvement, if for no other reason than it makes SET SESSION AUTHORIZATION
more consistent with SET ROLE.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Preventing non-superusers from altering session authorization

2023-07-12 Thread Nathan Bossart 
On Mon, Jul 10, 2023 at 01:49:55PM -0700, Nathan Bossart wrote:
> Great.  I'm going to wait a few more days in case anyone has additional
> feedback, but otherwise I intend to commit this shortly.

I've committed 0001 for now.  I'm hoping to commit the other two patches
within the next couple of days.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Preventing non-superusers from altering session authorization

2023-07-10 Thread Nathan Bossart 
On Mon, Jul 10, 2023 at 04:46:07PM -0400, Joseph Koshakow wrote:
> Thanks, I think the patch set looks good to go!

Great.  I'm going to wait a few more days in case anyone has additional
feedback, but otherwise I intend to commit this shortly.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Preventing non-superusers from altering session authorization

2023-07-10 Thread Joseph Koshakow 
On Mon, Jul 10, 2023 at 4:32 PM Nathan Bossart 
wrote:
> Okay.  Here's a new patch set in which I believe I've addressed all
> feedback.  I didn't keep the GetAuthenticatedUserIsSuperuser() helper
> function around, as I didn't see a strong need for it.

Thanks, I think the patch set looks good to go!

> And I haven't
> touched the "is_superuser" GUC, either.  I figured we can take up any
> changes for it in the other thread.

Yeah, I think that makes sense.

Thanks,
Joe Koshakow

Re: Preventing non-superusers from altering session authorization

2023-07-10 Thread Nathan Bossart 
On Sun, Jul 09, 2023 at 08:54:30PM -0400, Joseph Koshakow wrote:
> I just realized that you moved this comment from
> SetSessionAuthorization. I think we should leave the part about setting
> the GUC variable is_superuser on top of SetSessionAuthorization since
> that's where we actually set the GUC.

Okay.  Here's a new patch set in which I believe I've addressed all
feedback.  I didn't keep the GetAuthenticatedUserIsSuperuser() helper
function around, as I didn't see a strong need for it.  And I haven't
touched the "is_superuser" GUC, either.  I figured we can take up any
changes for it in the other thread.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 691c7a30d86385cca33a7ac43a5d63c4bc39dae1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Sun, 9 Jul 2023 11:28:56 -0700
Subject: [PATCH v6 1/3] Rename session_auth_is_superuser to
 current_role_is_superuser.

Suggested-by: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com
---
 src/backend/access/transam/parallel.c | 2 +-
 src/backend/utils/misc/guc_tables.c   | 9 ++---
 src/include/utils/guc.h   | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 2b8bc2f58d..12d97998cc 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -326,7 +326,7 @@ InitializeParallelDSM(ParallelContext *pcxt)
 	fps->database_id = MyDatabaseId;
 	fps->authenticated_user_id = GetAuthenticatedUserId();
 	fps->outer_user_id = GetCurrentRoleId();
-	fps->is_superuser = session_auth_is_superuser;
+	fps->is_superuser = current_role_is_superuser;
 	GetUserIdAndSecContext(&fps->current_user_id, &fps->sec_context);
 	GetTempNamespaceState(&fps->temp_namespace_id,
 		  &fps->temp_toast_namespace_id);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c14456060c..93dc2e7680 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -511,7 +511,7 @@ bool		check_function_bodies = true;
  * details.
  */
 bool		default_with_oids = false;
-bool		session_auth_is_superuser;
+bool		current_role_is_superuser;
 
 int			log_min_error_statement = ERROR;
 int			log_min_messages = WARNING;
@@ -1037,13 +1037,16 @@ struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		/* Not for general use --- used by SET SESSION AUTHORIZATION */
+		/*
+		 * Not for general use --- used by SET SESSION AUTHORIZATION and SET
+		 * ROLE
+		 */
 		{"is_superuser", PGC_INTERNAL, UNGROUPED,
 			gettext_noop("Shows whether the current user is a superuser."),
 			NULL,
 			GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
 		},
-		&session_auth_is_superuser,
+		¤t_role_is_superuser,
 		false,
 		NULL, NULL, NULL
 	},
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d5253c7ed2..223a19f80d 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -250,7 +250,7 @@ extern PGDLLIMPORT bool log_statement_stats;
 extern PGDLLIMPORT bool log_btree_build_stats;
 
 extern PGDLLIMPORT bool check_function_bodies;
-extern PGDLLIMPORT bool session_auth_is_superuser;
+extern PGDLLIMPORT bool current_role_is_superuser;
 
 extern PGDLLIMPORT bool log_duration;
 extern PGDLLIMPORT int log_parameter_max_length;
-- 
2.25.1

>From a08490dd85f0d41830aa06982a5c5ea588ba79d1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Sat, 8 Jul 2023 21:35:03 -0700
Subject: [PATCH v6 2/3] Move session auth privilege check to
 check_session_authorization().

Author: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com
---
 src/backend/commands/variable.c   | 21 +
 src/backend/utils/init/miscinit.c | 30 --
 src/include/miscadmin.h   |  1 +
 3 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..b8a75012a5 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -846,6 +846,27 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 
 	ReleaseSysCache(roleTup);
 
+	/*
+	 * Only superusers may SET SESSION AUTHORIZATION to roles other than
+	 * itself.  Note that in case of multiple SETs in a single session, the
+	 * original authenticated user's superuserness is what matters.
+	 */
+	if (roleid != GetAuthenticatedUserId() &&
+		!GetAuthenticatedUserIsSuperuser())
+	{
+		if (source == PGC_S_TEST)
+		{
+			ereport(NOTICE,
+	(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+	 errmsg("permission will be denied to set session authorization \"%s\"",
+			*newval)));
+			return true;
+		}
+		GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+		GUC_check_errmsg("permission denied to s

Re: Preventing non-superusers from altering session authorization

2023-07-09 Thread Joseph Koshakow 
On Sun, Jul 9, 2023 at 1:03 PM Joseph Koshakow  wrote:

>> * Only a superuser may set auth ID to something other than himself

> Is "auth ID" the right term here? Maybe something like "Only a
> superuser may set their session authorization/ID to something other
> than their authenticated ID."

>>   But we set the GUC variable
>> * is_superuser to indicate whether the *current* session userid is a
>> * superuser.

> Just a small correction here, I believe the is_superuser GUC is meant
> to indicate whether the current user id is a superuser, not the current
> session user id. We only update is_superuser in SetSessionAuthorization
> because we are also updating the current user id in SetSessionUserId.

I just realized that you moved this comment from
SetSessionAuthorization. I think we should leave the part about setting
the GUC variable is_superuser on top of SetSessionAuthorization since
that's where we actually set the GUC.

Thanks,
Joe Koshakow

Re: Preventing non-superusers from altering session authorization

2023-07-09 Thread Joseph Koshakow 
On Sun, Jul 9, 2023 at 12:47 AM Nathan Bossart 
wrote:

> I think we should split this into two patches: one to move the permission
> check to check_session_authorization() and another for the behavior
change.
> I've attached an attempt at the first one (that borrows heavily from your
> latest patch).  AFAICT the only reason that the permission check lives in
> SetSessionAuthorization() is because AuthenticatedUserIsSuperuser is
static
> to miscinit.c and doesn't have an accessor function.  I added one, but it
> would probably just be removed by the following patch.  WDYT?

I think that's a good idea. We could even keep around the accessor
function as a good place to bundle the calls to
Assert(OidIsValid(AuthenticatedUserId))
and
superuser_arg(AuthenticatedUserId)

> * Only a superuser may set auth ID to something other than himself

Is "auth ID" the right term here? Maybe something like "Only a
superuser may set their session authorization/ID to something other
than their authenticated ID."

>   But we set the GUC variable
> * is_superuser to indicate whether the *current* session userid is a
> * superuser.

Just a small correction here, I believe the is_superuser GUC is meant
to indicate whether the current user id is a superuser, not the current
session user id. We only update is_superuser in SetSessionAuthorization
because we are also updating the current user id in SetSessionUserId.
For example,

test=# CREATE ROLE r1 SUPERUSER;
CREATE ROLE
test=# CREATE ROLE r2;
CREATE ROLE
test=# SET SESSION AUTHORIZATION r1;
SET
test=# SET ROLE r2;
SET
test=> SELECT session_user, current_user;
 session_user | current_user
--+--
 r1   | r2
(1 row)

test=> SHOW is_superuser;
 is_superuser
--
 off
(1 row)

Which has also made me realize that the comment on is_superuser in
guc_tables.c is incorrect:

> /* Not for general use --- used by SET SESSION AUTHORIZATION */

Additionally the C variable name for is_superuser is fairly misleading:

> session_auth_is_superuser

The documentation for this GUC in show.sgml is correct:

> True if the current role has superuser privileges.

As an aside, I'm starting to think we should consider removing this
GUC. It sometimes reports an incorrect value [0], and potentially is
not used internally for anything.

I've rebased my changes over your patch and attached them both.

[0]
https://www.postgresql.org/message-id/CAAvxfHcxH-hLndty6CRThGXL1hLsgCn%2BE3QuG_4Qi7GxrHmgKg%40mail.gmail.com
From 2e1689b5fe384d675043beb9df8eff49a0ff436e Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Sun, 9 Jul 2023 12:58:41 -0400
Subject: [PATCH 2/2] Prevent non-superusers from altering session auth

Previously, if a user connected with as a role that had the superuser
attribute, then they could always execute a SET SESSION AUTHORIZATION
statement for the duration of their session. Even if the role was
altered to set superuser to false, the user was still allowed to
execute SET SESSION AUTHORIZATION. This allowed them to set their
session role to some other superuser and effectively regain the
superuser privileges. They could even reset their own superuser
attribute to true.

This commit alters the privilege checks for SET SESSION AUTHORIZATION
such that a user can only execute it if the role they connected with is
currently a superuser. This prevents users from regaining superuser
privileges after it has been revoked.
---
 doc/src/sgml/ref/set_session_auth.sgml |  2 +-
 src/backend/utils/init/miscinit.c  | 19 +--
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml
index f8fcafc194..94adab2468 100644
--- a/doc/src/sgml/ref/set_session_auth.sgml
+++ b/doc/src/sgml/ref/set_session_auth.sgml
@@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION
 
   
The session user identifier can be changed only if the initial session
-   user (the authenticated user) had the
+   user (the authenticated user) has the
superuser privilege.  Otherwise, the command is accepted only if it
specifies the authenticated user name.
   
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index f5548a0f47..1aa393f9fd 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,7 +467,7 @@ ChangeToDataDir(void)
  * AuthenticatedUserId is determined at connection start and never changes.
  *
  * SessionUserId is initially the same as AuthenticatedUserId, but can be
- * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser).
+ * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserId is a superuser).
  * This is the ID reported by the SESSION_USER SQL function.
  *
  * OuterUserId is the current user ID in effect at the "outer level" (outside
@@ -492,8 +492,7 @@ static Oid	OuterUserId = InvalidOid;
 static Oid	Cu

Re: Preventing non-superusers from altering session authorization

2023-07-08 Thread Nathan Bossart 
On Sat, Jul 08, 2023 at 07:08:35PM -0400, Joseph Koshakow wrote:
> On Sat, Jul 8, 2023 at 6:09 PM Nathan Bossart 
> wrote:
> 
>>> I think the issue here is that if a session loses the ability to set
>>> their session authorization in the middle of a transaction, then
>>> rolling back the transaction may fail and cause the server to panic.
>>> That's probably what the deleted comment mean when it said:
>>>
 * It's OK because the check does not require catalog access and can't
 * fail during an end-of-transaction GUC reversion
>>
>> Yeah.  IIUC the ERROR longjmps to a block that calls AbortTransaction(),
>> which ERRORs again when resetting the session authorization, which causes
>> us to call AbortTransaction() again, etc., etc.

src/backend/utils/misc/README has the following relevant text:

Note that there is no provision for a failure result code.  assign_hooks
should never fail except under the most dire circumstances, since a 
failure
may for example result in GUC settings not being rolled back properly 
during
transaction abort.  In general, try to do anything that could 
conceivably
fail in a check_hook instead, and pass along the results in an "extra"
struct, so that the assign hook has little to do beyond copying the 
data to
someplace.  This applies particularly to catalog lookups: any required
lookups must be done in the check_hook, since the assign_hook may be
executed during transaction rollback when lookups will be unsafe.

> Everything seems to work fine if the privilege check is moved to
> check_session_authorization. Which is maybe what the comment meant
> instead of assign_session_authorization.

Ah, that does make more sense.

I think we should split this into two patches: one to move the permission
check to check_session_authorization() and another for the behavior change.
I've attached an attempt at the first one (that borrows heavily from your
latest patch).  AFAICT the only reason that the permission check lives in
SetSessionAuthorization() is because AuthenticatedUserIsSuperuser is static
to miscinit.c and doesn't have an accessor function.  I added one, but it
would probably just be removed by the following patch.  WDYT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 4972f29cef6dfbb948e843517ce5ff413628c2f1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Sat, 8 Jul 2023 21:35:03 -0700
Subject: [PATCH v4 1/1] move session auth permission check

---
 src/backend/commands/variable.c   | 23 +++
 src/backend/utils/init/miscinit.c | 29 ++---
 src/include/miscadmin.h   |  1 +
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..f8e308f1d0 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -846,6 +846,29 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 
 	ReleaseSysCache(roleTup);
 
+	/*
+	 * Only a superuser may set auth ID to something other than himself.  Note
+	 * that in case of multiple SETs in a single session, the original
+	 * userid's superuserness is what matters.  But we set the GUC variable
+	 * is_superuser to indicate whether the *current* session userid is a
+	 * superuser.
+	 */
+	if (roleid != GetAuthenticatedUserId() &&
+		!GetAuthenticatedUserIsSuperuser())
+	{
+		if (source == PGC_S_TEST)
+		{
+			ereport(NOTICE,
+	(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+	 errmsg("permission will be denied to set session authorization \"%s\"",
+			*newval)));
+			return true;
+		}
+		GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+		GUC_check_errmsg("permission denied to set session authorization");
+		return false;
+	}
+
 	/* Set up "extra" struct for assign_session_authorization to use */
 	myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra));
 	if (!myextra)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..f5548a0f47 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -582,6 +582,16 @@ GetAuthenticatedUserId(void)
 	return AuthenticatedUserId;
 }
 
+/*
+ * Return whether the authenticated user was superuser at connection start.
+ */
+bool
+GetAuthenticatedUserIsSuperuser(void)
+{
+	Assert(OidIsValid(AuthenticatedUserId));
+	return AuthenticatedUserIsSuperuser;
+}
+
 
 /*
  * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
@@ -888,29 +898,10 @@ system_user(PG_FUNCTION_ARGS)
 
 /*
  * Change session auth ID while running
- *
- * Only a superuser may set auth ID to something other than himself.  Note
- * that in case of multiple SETs in a single session, the original userid's
- * superuserness is what matters.  But we set the GUC variable is_superuser
- * to indicate whether the *current* session userid is a superuser.

Re: Preventing non-superusers from altering session authorization

2023-07-08 Thread Joseph Koshakow 
On Sat, Jul 8, 2023 at 6:09 PM Nathan Bossart 
wrote:

>> I think the issue here is that if a session loses the ability to set
>> their session authorization in the middle of a transaction, then
>> rolling back the transaction may fail and cause the server to panic.
>> That's probably what the deleted comment mean when it said:
>>
>>> * It's OK because the check does not require catalog access and can't
>>> * fail during an end-of-transaction GUC reversion
>
> Yeah.  IIUC the ERROR longjmps to a block that calls AbortTransaction(),
> which ERRORs again when resetting the session authorization, which causes
> us to call AbortTransaction() again, etc., etc.

Everything seems to work fine if the privilege check is moved to
check_session_authorization. Which is maybe what the comment meant
instead of assign_session_authorization.

I've attached a patch with this change.

Thanks,
Joe Koshakow
From cb0198524d96068079e301a6785301440f3be3aa Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Thu, 15 Jun 2023 14:53:11 -0400
Subject: [PATCH] Prevent non-superusers from altering session auth

Previously, if a user connected with as a role that had the superuser
attribute, then they could always execute a SET SESSION AUTHORIZATION
statement for the duration of their session. Even if the role was
altered to set superuser to false, the user was still allowed to
execute SET SESSION AUTHORIZATION. This allowed them to set their
session role to some other superuser and effectively regain the
superuser privileges. They could even reset their own superuser
attribute to true.

This commit alters the privilege checks for SET SESSION AUTHORIZATION
such that a user can only execute it if the role they connected with is
currently a superuser. This prevents users from regaining superuser
privileges after it has been revoked.
---
 doc/src/sgml/ref/set_session_auth.sgml |  2 +-
 src/backend/commands/variable.c| 13 +++-
 src/backend/utils/init/miscinit.c  | 28 ++
 3 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml
index f8fcafc194..94adab2468 100644
--- a/doc/src/sgml/ref/set_session_auth.sgml
+++ b/doc/src/sgml/ref/set_session_auth.sgml
@@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION
 
   
The session user identifier can be changed only if the initial session
-   user (the authenticated user) had the
+   user (the authenticated user) has the
superuser privilege.  Otherwise, the command is accepted only if it
specifies the authenticated user name.
   
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..e2f47eceb7 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -803,7 +803,8 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 {
 	HeapTuple	roleTup;
 	Form_pg_authid roleform;
-	Oid			roleid;
+	Oid			roleid,
+authenticated_user_id;
 	bool		is_superuser;
 	role_auth_extra *myextra;
 
@@ -846,6 +847,16 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 
 	ReleaseSysCache(roleTup);
 
+	authenticated_user_id = GetAuthenticatedUserId();
+	/* Must have authenticated already, else can't make permission check */
+	Assert(OidIsValid(authenticated_user_id));
+
+	if (roleid != authenticated_user_id &&
+		!superuser_arg(authenticated_user_id))
+		ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to set session authorization")));
+
 	/* Set up "extra" struct for assign_session_authorization to use */
 	myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra));
 	if (!myextra)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..04e019df20 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,7 +467,7 @@ ChangeToDataDir(void)
  * AuthenticatedUserId is determined at connection start and never changes.
  *
  * SessionUserId is initially the same as AuthenticatedUserId, but can be
- * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser).
+ * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserId is a superuser).
  * This is the ID reported by the SESSION_USER SQL function.
  *
  * OuterUserId is the current user ID in effect at the "outer level" (outside
@@ -492,8 +492,6 @@ static Oid	OuterUserId = InvalidOid;
 static Oid	CurrentUserId = InvalidOid;
 static const char *SystemUser = NULL;
 
-/* We also have to remember the superuser state of some of these levels */
-static bool AuthenticatedUserIsSuperuser = false;
 static bool SessionUserIsSuperuser = false;
 
 static int	SecurityRestrictionContext = 0;
@@ -731,6 +729,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	HeapTuple	roleTup;
 	Form_pg_authid rform;
 	char	   *rname;
+	bool	   is_superuser;
 
 	/*
 	 * Don't do scans if we'r

Re: Preventing non-superusers from altering session authorization

2023-07-08 Thread Nathan Bossart 
On Sat, Jul 08, 2023 at 04:44:06PM -0400, Joseph Koshakow wrote:
> 2023-07-08 16:33:27.787 EDT [157141] PANIC:  ERRORDATA_STACK_SIZE exceeded
> 2023-07-08 16:33:27.882 EDT [156878] LOG:  server process (PID 157141) was
> terminated by signal 6: Aborted
> 2023-07-08 16:33:27.882 EDT [156878] DETAIL:  Failed process was running:
> CREATE TABLE t ();
> 
> I think the issue here is that if a session loses the ability to set
> their session authorization in the middle of a transaction, then
> rolling back the transaction may fail and cause the server to panic.
> That's probably what the deleted comment mean when it said:
> 
>> * It's OK because the check does not require catalog access and can't
>> * fail during an end-of-transaction GUC reversion

Yeah.  IIUC the ERROR longjmps to a block that calls AbortTransaction(),
which ERRORs again when resetting the session authorization, which causes
us to call AbortTransaction() again, etc., etc.

> Interestingly, if the r1 session manually types `ROLLBACK` instead of
> executing a command that fails, then everything is fine and there's no
> panic. I'm not familiar enough with transaction handling to know why
> there would be a difference there.

I haven't had a chance to dig into this one yet, but that is indeed
interesting.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Preventing non-superusers from altering session authorization

2023-07-08 Thread Joseph Koshakow 
I've discovered an issue with this approach. Let's say you have some
session open that is connected as a superuser and you run the following
commands:

  - CREATE ROLE r1 LOGIN SUPERUSER;
  - CREATE ROLE r2;
  - CREATE ROLE r3;

Then you open another session connected with user r1 and run the
following commands:

  - SET SESSION AUTHROIZATION r2;
  - BEGIN;
  - SET SESSION AUTHORIZATION r3;

Then in your original session run:

  - ALTER ROLE r1 NOSUPERUSER;

Finally in the r1 session run:

  - CREATE TABLE t ();

Postgres will then panic with the following logs:

2023-07-08 16:33:27.787 EDT [157141] ERROR:  permission denied for schema
public at character 14
2023-07-08 16:33:27.787 EDT [157141] STATEMENT:  CREATE TABLE t ();
2023-07-08 16:33:27.787 EDT [157141] ERROR:  permission denied to set
session authorization
2023-07-08 16:33:27.787 EDT [157141] WARNING:  AbortTransaction while in
ABORT state
2023-07-08 16:33:27.787 EDT [157141] ERROR:  permission denied to set
session authorization
2023-07-08 16:33:27.787 EDT [157141] WARNING:  AbortTransaction while in
ABORT state
2023-07-08 16:33:27.787 EDT [157141] ERROR:  permission denied to set
session authorization
2023-07-08 16:33:27.787 EDT [157141] WARNING:  AbortTransaction while in
ABORT state
2023-07-08 16:33:27.787 EDT [157141] ERROR:  permission denied to set
session authorization
2023-07-08 16:33:27.787 EDT [157141] PANIC:  ERRORDATA_STACK_SIZE exceeded
2023-07-08 16:33:27.882 EDT [156878] LOG:  server process (PID 157141) was
terminated by signal 6: Aborted
2023-07-08 16:33:27.882 EDT [156878] DETAIL:  Failed process was running:
CREATE TABLE t ();

I think the issue here is that if a session loses the ability to set
their session authorization in the middle of a transaction, then
rolling back the transaction may fail and cause the server to panic.
That's probably what the deleted comment mean when it said:

> * It's OK because the check does not require catalog access and can't
> * fail during an end-of-transaction GUC reversion

Interestingly, if the r1 session manually types `ROLLBACK` instead of
executing a command that fails, then everything is fine and there's no
panic. I'm not familiar enough with transaction handling to know why
there would be a difference there.

Thanks,
Joe Koshakow

Re: Preventing non-superusers from altering session authorization

2023-07-08 Thread Joseph Koshakow 
Nathan Bossart  wrote:

> I see that RESET SESSION AUTHORIZATION
> with a concurrently dropped role will FATAL with your patch but succeed
> without it, which could be part of the reason.

I didn't even realize it, but the change to superuser_arg() in v2 fixed
this issue. The catalog lookup is only done if
userid != AuthenticatedUserId. So RESET SESSION AUTHORIZATION with a
concurrently dropped role will no longer FATAL.

Thanks,
Joe

On Sat, Jul 1, 2023 at 11:33 AM Joseph Koshakow  wrote:

> >> That might be a good change? If the original authenticated role ID no
> >> longer exists then we may want to return an error when trying to set
> >> your session authorization to that role.
> >
> > I was curious why we don't block DROP ROLE if there are active sessions
> for
> > the role or terminate any such sessions as part of the command, and I
> found
> > this discussion from 2016:
> >
> >https://postgr.es/m/flat/56E87CD8.60007%40ohmu.fi
>
> Ah, that makes sense that we don't prevent DROP ROLE on active roles.
> Though, we do error when you try and set your role or session
> authorization to a dropped role. So erroring on RESET SESSION
> AUTHORIZATION when the original role is dropped makes it consistent
> with SET SESSION AUTHORIZATION TO . On the other
> hand it makes it inconsistent with RESET ROLE, which does not error on
> a dropped role.
>
> - Joe Koshakow
>
> On Fri, Jun 23, 2023 at 1:54 PM Nathan Bossart 
> wrote:
>
>> On Thu, Jun 22, 2023 at 06:39:45PM -0400, Joseph Koshakow wrote:
>> > On Wed, Jun 21, 2023 at 11:48 PM Nathan Bossart <
>> nathandboss...@gmail.com>
>> > wrote:
>> >> I see that RESET SESSION AUTHORIZATION
>> >> with a concurrently dropped role will FATAL with your patch but succeed
>> >> without it, which could be part of the reason.
>> >
>> > That might be a good change? If the original authenticated role ID no
>> > longer exists then we may want to return an error when trying to set
>> > your session authorization to that role.
>>
>> I was curious why we don't block DROP ROLE if there are active sessions
>> for
>> the role or terminate any such sessions as part of the command, and I
>> found
>> this discussion from 2016:
>>
>> https://postgr.es/m/flat/56E87CD8.60007%40ohmu.fi
>>
>> --
>> Nathan Bossart
>> Amazon Web Services: https://aws.amazon.com
>>
>

Re: Preventing non-superusers from altering session authorization

2023-07-01 Thread Joseph Koshakow 
>> That might be a good change? If the original authenticated role ID no
>> longer exists then we may want to return an error when trying to set
>> your session authorization to that role.
>
> I was curious why we don't block DROP ROLE if there are active sessions
for
> the role or terminate any such sessions as part of the command, and I
found
> this discussion from 2016:
>
>https://postgr.es/m/flat/56E87CD8.60007%40ohmu.fi

Ah, that makes sense that we don't prevent DROP ROLE on active roles.
Though, we do error when you try and set your role or session
authorization to a dropped role. So erroring on RESET SESSION
AUTHORIZATION when the original role is dropped makes it consistent
with SET SESSION AUTHORIZATION TO . On the other
hand it makes it inconsistent with RESET ROLE, which does not error on
a dropped role.

- Joe Koshakow

On Fri, Jun 23, 2023 at 1:54 PM Nathan Bossart 
wrote:

> On Thu, Jun 22, 2023 at 06:39:45PM -0400, Joseph Koshakow wrote:
> > On Wed, Jun 21, 2023 at 11:48 PM Nathan Bossart <
> nathandboss...@gmail.com>
> > wrote:
> >> I see that RESET SESSION AUTHORIZATION
> >> with a concurrently dropped role will FATAL with your patch but succeed
> >> without it, which could be part of the reason.
> >
> > That might be a good change? If the original authenticated role ID no
> > longer exists then we may want to return an error when trying to set
> > your session authorization to that role.
>
> I was curious why we don't block DROP ROLE if there are active sessions for
> the role or terminate any such sessions as part of the command, and I found
> this discussion from 2016:
>
> https://postgr.es/m/flat/56E87CD8.60007%40ohmu.fi
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>

Re: Preventing non-superusers from altering session authorization

2023-06-23 Thread Nathan Bossart 
On Thu, Jun 22, 2023 at 06:39:45PM -0400, Joseph Koshakow wrote:
> On Wed, Jun 21, 2023 at 11:48 PM Nathan Bossart 
> wrote:
>> I see that RESET SESSION AUTHORIZATION
>> with a concurrently dropped role will FATAL with your patch but succeed
>> without it, which could be part of the reason.
> 
> That might be a good change? If the original authenticated role ID no
> longer exists then we may want to return an error when trying to set
> your session authorization to that role.

I was curious why we don't block DROP ROLE if there are active sessions for
the role or terminate any such sessions as part of the command, and I found
this discussion from 2016:

https://postgr.es/m/flat/56E87CD8.60007%40ohmu.fi

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Preventing non-superusers from altering session authorization

2023-06-22 Thread Michał Kłeczek 
Hi,

I’ve just stumbled upon this patch and thread and thought I could share an idea 
of adding an optional temporary secret to SET SESSION AUTHORIZATION so that it 
is only possible to RESET SESSION AUTHORIZATION by providing the same secret 
,like:

SET SESSION AUTHORIZATION [role] GUARDED BY ‘[secret]’;

...

RESET SESSION AUTHORIZATION WITH ‘[secret]’;


The use case is: I have a set of Liquibase scripts I would like to execute as a 
different role each and make sure they cannot escape the sandbox.

As I am not a Postgres hacker I wonder how difficult to implement it might be…

Thanks,
Michal

> On 23 Jun 2023, at 00:39, Joseph Koshakow  wrote:
> 
> 
> 
> On Wed, Jun 21, 2023 at 11:48 PM Nathan Bossart  > wrote:
> >
> >On Wed, Jun 21, 2023 at 04:28:43PM -0400, Joseph Koshakow wrote:
> >> + roleTup = SearchSysCache1(AUTHOID, 
> > ObjectIdGetDatum(AuthenticatedUserId));
> >> + if (!HeapTupleIsValid(roleTup))
> >> + ereport(FATAL,
> >> + 
> > (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
> >> + errmsg("role with OID %u 
> > does not exist", AuthenticatedUserId)));
> >> + rform = (Form_pg_authid) GETSTRUCT(roleTup);
> >
> >I think "superuser_arg(AuthenticatedUserId)" would work here.
> 
> Yep, that worked. I've attached a patch with this change.
> 
> > I see that RESET SESSION AUTHORIZATION
> > with a concurrently dropped role will FATAL with your patch but succeed
> > without it, which could be part of the reason.
> 
> That might be a good change? If the original authenticated role ID no
> longer exists then we may want to return an error when trying to set
> your session authorization to that role.
> 
> Thanks,
> Joe Koshakow
>

Re: Preventing non-superusers from altering session authorization

2023-06-22 Thread Joseph Koshakow 
On Wed, Jun 21, 2023 at 11:48 PM Nathan Bossart 
wrote:
>
>On Wed, Jun 21, 2023 at 04:28:43PM -0400, Joseph Koshakow wrote:
>> + roleTup = SearchSysCache1(AUTHOID,
ObjectIdGetDatum(AuthenticatedUserId));
>> + if (!HeapTupleIsValid(roleTup))
>> + ereport(FATAL,
>> +
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
>> + errmsg("role with OID
%u does not exist", AuthenticatedUserId)));
>> + rform = (Form_pg_authid) GETSTRUCT(roleTup);
>
>I think "superuser_arg(AuthenticatedUserId)" would work here.

Yep, that worked. I've attached a patch with this change.

> I see that RESET SESSION AUTHORIZATION
> with a concurrently dropped role will FATAL with your patch but succeed
> without it, which could be part of the reason.

That might be a good change? If the original authenticated role ID no
longer exists then we may want to return an error when trying to set
your session authorization to that role.

Thanks,
Joe Koshakow
From 2b2817e3ea4f1541a781216afb7415435ca362a0 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Thu, 15 Jun 2023 14:53:11 -0400
Subject: [PATCH] Prevent non-superusers from altering session auth

Previously, if a user connected with as a role that had the superuser
attribute, then they could always execute a SET SESSION AUTHORIZATION
statement for the duration of their session. Even if the role was
altered to set superuser to false, the user was still allowed to
execute SET SESSION AUTHORIZATION. This allowed them to set their
session role to some other superuser and effectively regain the
superuser privileges. They could even reset their own superuser
attribute to true.

This commit alters the privilege checks for SET SESSION AUTHORIZATION
such that a user can only execute it if the role they connected with is
currently a superuser. This prevents users from regaining superuser
privileges after it has been revoked.
---
 doc/src/sgml/ref/set_session_auth.sgml |  2 +-
 src/backend/utils/init/miscinit.c  | 21 +++--
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml
index f8fcafc194..94adab2468 100644
--- a/doc/src/sgml/ref/set_session_auth.sgml
+++ b/doc/src/sgml/ref/set_session_auth.sgml
@@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION
 
   
The session user identifier can be changed only if the initial session
-   user (the authenticated user) had the
+   user (the authenticated user) has the
superuser privilege.  Otherwise, the command is accepted only if it
specifies the authenticated user name.
   
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..4cef655703 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,7 +467,7 @@ ChangeToDataDir(void)
  * AuthenticatedUserId is determined at connection start and never changes.
  *
  * SessionUserId is initially the same as AuthenticatedUserId, but can be
- * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser).
+ * changed by SET SESSION AUTHORIZATION.
  * This is the ID reported by the SESSION_USER SQL function.
  *
  * OuterUserId is the current user ID in effect at the "outer level" (outside
@@ -492,8 +492,6 @@ static Oid	OuterUserId = InvalidOid;
 static Oid	CurrentUserId = InvalidOid;
 static const char *SystemUser = NULL;
 
-/* We also have to remember the superuser state of some of these levels */
-static bool AuthenticatedUserIsSuperuser = false;
 static bool SessionUserIsSuperuser = false;
 
 static int	SecurityRestrictionContext = 0;
@@ -731,6 +729,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	HeapTuple	roleTup;
 	Form_pg_authid rform;
 	char	   *rname;
+	bool	   is_superuser;
 
 	/*
 	 * Don't do scans if we're bootstrapping, none of the system catalogs
@@ -770,10 +769,10 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	rname = NameStr(rform->rolname);
 
 	AuthenticatedUserId = roleid;
-	AuthenticatedUserIsSuperuser = rform->rolsuper;
+	is_superuser = rform->rolsuper;
 
 	/* This sets OuterUserId/CurrentUserId too */
-	SetSessionUserId(roleid, AuthenticatedUserIsSuperuser);
+	SetSessionUserId(roleid, is_superuser);
 
 	/* Also mark our PGPROC entry with the authenticated user id */
 	/* (We assume this is an atomic store so no lock is needed) */
@@ -806,7 +805,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 		 * just document that the connection limit is approximate.
 		 */
 		if (rform->rolconnlimit >= 0 &&
-			!AuthenticatedUserIsSuperuser &&
+			!is_superuser &&
 			CountUserBackends(roleid) > rform->rolconnlimit)
 			ereport(FATAL,
 	(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
@@ -818,7 +817,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	SetConfigOption("session_authorization", rname,
 	PGC_BACKEND, PGC_S_OVERRIDE);
 	SetConfigOpt

Re: Preventing non-superusers from altering session authorization

2023-06-21 Thread Nathan Bossart 
On Wed, Jun 21, 2023 at 04:28:43PM -0400, Joseph Koshakow wrote:
> + roleTup = SearchSysCache1(AUTHOID, 
> ObjectIdGetDatum(AuthenticatedUserId));
> + if (!HeapTupleIsValid(roleTup))
> + ereport(FATAL,
> + 
> (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
> + errmsg("role with OID %u does 
> not exist", AuthenticatedUserId)));
> + rform = (Form_pg_authid) GETSTRUCT(roleTup);

I think "superuser_arg(AuthenticatedUserId)" would work here.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Preventing non-superusers from altering session authorization

2023-06-21 Thread Nathan Bossart 
On Wed, Jun 21, 2023 at 04:28:43PM -0400, Joseph Koshakow wrote:
> Currently, a user is allowed to execute SET SESSION AUTHORIZATION [1]
> if the role they connected to PostgreSQL with was a superuser at the
> time of connection. Even if the role is later altered to no longer be a
> superuser, the session can still execute SET SESSION AUTHORIZATION, as
> long as the session isn't disconnected. As a consequence, if that role
> is altered to no longer be a superuser, then the user can use SET
> SESSION AUTHORIZATION to switch to another role that is a superuser and
> regain superuser privileges. They can even re-grant themselves the
> superuser attribute.

I suspect most users aren't changing the superuser attribute on roles very
often, so it's unlikely to be a problem.  But it might still be worth
revisiting.

> It is possible that the user had already run SET SESSION AUTHORIZATION
> to set their session to a superuser before their connecting role lost
> the superuser attribute. In this case there's not much we can do.

Right.

> Also, from looking at the code and documentation, it looks like SET
> SESSION AUTHORIZATION works this way intentionally. However, I'm unable
> to figure out why we'd want it to work this way.

I found a brief mention in the archives about this implementation decision
[0], but I don't think it explains the reasoning.

> I've attached a patch that would fix this issue by checking the catalog
> to see if the connecting role is currently a superuser every time SET
> SESSION AUTHORIZATION is run. However, according to the comment I
> deleted there's something invalid about reading the catalog from that
> function, though I wasn't able to understand it fully.

This comment was added in e5d6b91.  I see that RESET SESSION AUTHORIZATION
with a concurrently dropped role will FATAL with your patch but succeed
without it, which could be part of the reason.

> One downside is that if a user switches their session authorization to
> some role, then loses the superuser attribute on their connecting role,
> they may be stuck in a that role with no way to reset their session
> authorization without disconnecting and reconnecting.

It looks like SetSessionAuthorization() skips the privilege checks if the
target role is the authenticated role, so I don't think they'll get stuck.

[0] 
https://postgr.es/m/Pine.LNX.4.30.0104182119290.762-10%40peter.localdomain

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Preventing non-superusers from altering session authorization

2023-06-21 Thread Joseph Koshakow 
Hi all,

I briefly mentioned this issue in another mailing thread [0].

Currently, a user is allowed to execute SET SESSION AUTHORIZATION [1]
if the role they connected to PostgreSQL with was a superuser at the
time of connection. Even if the role is later altered to no longer be a
superuser, the session can still execute SET SESSION AUTHORIZATION, as
long as the session isn't disconnected. As a consequence, if that role
is altered to no longer be a superuser, then the user can use SET
SESSION AUTHORIZATION to switch to another role that is a superuser and
regain superuser privileges. They can even re-grant themselves the
superuser attribute.

It is possible that the user had already run SET SESSION AUTHORIZATION
to set their session to a superuser before their connecting role lost
the superuser attribute. In this case there's not much we can do.

Also, from looking at the code and documentation, it looks like SET
SESSION AUTHORIZATION works this way intentionally. However, I'm unable
to figure out why we'd want it to work this way.

I've attached a patch that would fix this issue by checking the catalog
to see if the connecting role is currently a superuser every time SET
SESSION AUTHORIZATION is run. However, according to the comment I
deleted there's something invalid about reading the catalog from that
function, though I wasn't able to understand it fully.

One downside is that if a user switches their session authorization to
some role, then loses the superuser attribute on their connecting role,
they may be stuck in a that role with no way to reset their session
authorization without disconnecting and reconnecting.

Thanks,
Joe Koshakow

[0]
https://www.postgresql.org/message-id/CAAvxfHco7iGw4NarymhfLWN6PjzYRrbYFt2BnSFeSD5sFzqEJQ%40mail.gmail.com
[1] https://www.postgresql.org/docs/15/sql-set-session-authorization.html
From b5f7d42ea325b2be46b7c93e5404792046f1befc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Thu, 15 Jun 2023 14:53:11 -0400
Subject: [PATCH] Prevent non-superusers from altering session auth

Previously, if a user connected with as a role that had the superuser
attribute, then they could always execute a SET SESSION AUTHORIZATION
statement for the duration of their session. Even if the role was
altered to set superuser to false, the user was still allowed to
execute SET SESSION AUTHORIZATION. This allowed them to set their
session role to some other superuser and effectively regain the
superuser privileges. They could even reset their own superuser
attribute to true.

This commit alters the privilege checks for SET SESSION AUTHORIZATION
such that a user can only execute it if the role they connected with is
currently a superuser. This prevents users from regaining superuser
privileges after it has been revoked.
---
 doc/src/sgml/ref/set_session_auth.sgml |  2 +-
 src/backend/utils/init/miscinit.c  | 33 +++---
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml
index f8fcafc194..94adab2468 100644
--- a/doc/src/sgml/ref/set_session_auth.sgml
+++ b/doc/src/sgml/ref/set_session_auth.sgml
@@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION
 
   
The session user identifier can be changed only if the initial session
-   user (the authenticated user) had the
+   user (the authenticated user) has the
superuser privilege.  Otherwise, the command is accepted only if it
specifies the authenticated user name.
   
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..459af11691 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,7 +467,7 @@ ChangeToDataDir(void)
  * AuthenticatedUserId is determined at connection start and never changes.
  *
  * SessionUserId is initially the same as AuthenticatedUserId, but can be
- * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser).
+ * changed by SET SESSION AUTHORIZATION.
  * This is the ID reported by the SESSION_USER SQL function.
  *
  * OuterUserId is the current user ID in effect at the "outer level" (outside
@@ -492,8 +492,6 @@ static Oid	OuterUserId = InvalidOid;
 static Oid	CurrentUserId = InvalidOid;
 static const char *SystemUser = NULL;
 
-/* We also have to remember the superuser state of some of these levels */
-static bool AuthenticatedUserIsSuperuser = false;
 static bool SessionUserIsSuperuser = false;
 
 static int	SecurityRestrictionContext = 0;
@@ -731,6 +729,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	HeapTuple	roleTup;
 	Form_pg_authid rform;
 	char	   *rname;
+	bool	   is_superuser;
 
 	/*
 	 * Don't do scans if we're bootstrapping, none of the system catalogs
@@ -770,10 +769,10 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 	rname = NameStr(rform->rolname);
 
 	AuthenticatedUserId = roleid;
-	AuthenticatedUserIsSuperuser = rform->rolsuper;
+	is_superuser = rform->