Hello hackers,

Using placeholders for application variables allows the use of RLS for
application users as shown in this blog post
https://www.2ndquadrant.com/en/blog/application-users-vs-row-level-security/
.

  SET my.username = 'tomas'
   CREATE POLICY chat_policy ON chat
      USING (current_setting('my.username') IN (message_from, message_to))
      WITH CHECK (message_from = current_setting('my.username'))

This technique has enabled postgres sidecar services(PostgREST,
PostGraphQL, etc) to keep the application security at the database level,
which has worked great.

However, defining placeholders at the role level require superuser:

  alter role myrole set my.username to 'tomas';
  ERROR:  permission denied to set parameter "my.username"

Which is inconsistent and surprising behavior. I think it doesn't make
sense since you can already set them at the session or transaction
level(SET LOCAL my.username = 'tomas'). Enabling this would allow sidecar
services to store metadata scoped to its pertaining role.

I've attached a patch that removes this restriction. From my testing, this
doesn't affect permission checking when an extension defines its custom GUC
variables.

   DefineCustomStringVariable("my.custom", NULL, NULL,  &my_custom,  NULL,
      PGC_SUSET, ..);

Using PGC_SUSET or PGC_SIGHUP will fail accordingly. Also no tests fail
when doing "make installcheck".

---
Steve Chavez
Engineering at https://supabase.com/
From c3493373a8cddfff56d10bddce1dcdabc0722a34 Mon Sep 17 00:00:00 2001
From: Steve Chavez <st...@supabase.io>
Date: Sun, 5 Jun 2022 19:10:52 -0500
Subject: [PATCH] Allow placeholders in ALTER ROLE w/o superuser

Removes inconsistent superuser check for placeholders on
validate_option_array_item.
---
 src/backend/utils/misc/guc.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..7c83bc3004 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11658,19 +11658,12 @@ validate_option_array_item(const char *name, const char *value,
 	struct config_generic *gconf;

 	/*
-	 * There are three cases to consider:
+	 * There are two cases to consider:
 	 *
 	 * name is a known GUC variable.  Check the value normally, check
 	 * permissions normally (i.e., allow if variable is USERSET, or if it's
 	 * SUSET and user is superuser).
 	 *
-	 * name is not known, but exists or can be created as a placeholder (i.e.,
-	 * it has a valid custom name).  We allow this case if you're a superuser,
-	 * otherwise not.  Superusers are assumed to know what they're doing. We
-	 * can't allow it for other users, because when the placeholder is
-	 * resolved it might turn out to be a SUSET variable;
-	 * define_custom_variable assumes we checked that.
-	 *
 	 * name is not known and can't be created as a placeholder.  Throw error,
 	 * unless skipIfNoPermissions is true, in which case return false.
 	 */
@@ -11681,21 +11674,6 @@ validate_option_array_item(const char *name, const char *value,
 		return false;
 	}

-	if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)
-	{
-		/*
-		 * We cannot do any meaningful check on the value, so only permissions
-		 * are useful to check.
-		 */
-		if (superuser())
-			return true;
-		if (skipIfNoPermissions)
-			return false;
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("permission denied to set parameter \"%s\"", name)));
-	}
-
 	/* manual permissions check so we can avoid an error being thrown */
 	if (gconf->context == PGC_USERSET)
 		 /* ok */ ;
--
2.32.0

Reply via email to