ASYNC Privileges proposal
Hey all,
I find the current LISTEN / NOTIFY rather limited in the context of
databases with multiple roles. As it stands it is not possible to restrict
the use of LISTEN or NOTIFY to specific roles, and therefore notifications
(and their payloads) cannot really be trusted as coming from any particular
source.
If the payloads of notifications could be trusted, then applications could
make better use of them, without fear of leaking any sensitive information
to anyone who shouldn't be able to see it.
I'd like to propose a new ASYNC database privilege that would control
whether a role can use LISTEN, NOTIFY and UNLISTEN statements and the
associated pg_notify function.
ie:
GRANT ASYNC ON DATABASE xxxx TO bob;
REVOKE ASYNC ON DATABASE xxxx FROM bob;
SECURITY DEFINER functions could then be used anywhere that a finer grained
access control was required.
I had a quick play to see what might be involved [attached], and would like
to hear people thoughts; good idea, bad idea, not like that! etc
Chris.
Attachments:
async_privileges_r0.patchapplication/octet-stream; name=async_privileges_r0.patchDownload
From e49dad001718d906676c18185639403014aeacad Mon Sep 17 00:00:00 2001
From: chrisfarms <chris@oxdi.eu>
Date: Mon, 20 May 2013 02:13:34 +0100
Subject: [PATCH] rough draft of adding ASYNC prilileges at the database level
---
src/backend/catalog/aclchk.c | 4 ++++
src/backend/commands/async.c | 25 +++++++++++++++++++++++++
src/backend/utils/adt/acl.c | 4 ++++
src/include/nodes/parsenodes.h | 3 ++-
src/include/utils/acl.h | 5 +++--
src/test/regress/expected/privileges.out | 8 ++++++++
src/test/regress/sql/privileges.sql | 9 +++++++++
7 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 976f2d2..5f4084f 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3215,6 +3215,8 @@ string_to_privilege(const char *privname)
return ACL_CREATE_TEMP;
if (strcmp(privname, "connect") == 0)
return ACL_CONNECT;
+ if (strcmp(privname, "async") == 0)
+ return ACL_ASYNC;
if (strcmp(privname, "rule") == 0)
return 0; /* ignore old RULE privileges */
ereport(ERROR,
@@ -3252,6 +3254,8 @@ privilege_to_string(AclMode privilege)
return "TEMP";
case ACL_CONNECT:
return "CONNECT";
+ case ACL_ASYNC:
+ return "ASYNC";
default:
elog(ERROR, "unrecognized privilege: %d", (int) privilege);
}
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 9845cf9..0253524 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -120,6 +120,7 @@
#include "access/xact.h"
#include "catalog/pg_database.h"
#include "commands/async.h"
+#include "commands/dbcommands.h"
#include "funcapi.h"
#include "libpq/libpq.h"
#include "libpq/pqformat.h"
@@ -133,6 +134,7 @@
#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/timestamp.h"
+#include "utils/acl.h"
/*
@@ -536,6 +538,8 @@ Async_Notify(const char *channel, const char *payload)
Notification *n;
MemoryContext oldcontext;
+ CheckAsyncPriv();
+
if (Trace_notify)
elog(DEBUG1, "Async_Notify(%s)", channel);
@@ -617,6 +621,21 @@ queue_listen(ListenActionKind action, const char *channel)
}
/*
+ * CheckAsyncPriv
+ *
+ * Ensures that user has prilieges to use LISTEN, NOTIFY
+ * Executed by Async_* methods
+ */
+void
+CheckAsyncPriv()
+{
+ AclResult aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(), ACL_ASYNC);
+ if( aclresult != ACLCHECK_OK)
+ aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_DATABASE,
+ get_database_name(MyDatabaseId));
+}
+
+/*
* Async_Listen
*
* This is executed by the SQL listen command.
@@ -627,6 +646,8 @@ Async_Listen(const char *channel)
if (Trace_notify)
elog(DEBUG1, "Async_Listen(%s,%d)", channel, MyProcPid);
+ CheckAsyncPriv();
+
queue_listen(LISTEN_LISTEN, channel);
}
@@ -641,6 +662,8 @@ Async_Unlisten(const char *channel)
if (Trace_notify)
elog(DEBUG1, "Async_Unlisten(%s,%d)", channel, MyProcPid);
+ CheckAsyncPriv();
+
/* If we couldn't possibly be listening, no need to queue anything */
if (pendingActions == NIL && !unlistenExitRegistered)
return;
@@ -659,6 +682,8 @@ Async_UnlistenAll(void)
if (Trace_notify)
elog(DEBUG1, "Async_UnlistenAll(%d)", MyProcPid);
+ CheckAsyncPriv();
+
/* If we couldn't possibly be listening, no need to queue anything */
if (pendingActions == NIL && !unlistenExitRegistered)
return;
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 7a0721e..355fa44 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -1625,6 +1625,8 @@ convert_priv_string(text *priv_type_text)
return ACL_CREATE_TEMP;
if (pg_strcasecmp(priv_type, "CONNECT") == 0)
return ACL_CONNECT;
+ if (pg_strcasecmp(priv_type, "ASYNC") == 0)
+ return ACL_ASYNC;
if (pg_strcasecmp(priv_type, "RULE") == 0)
return 0; /* ignore old RULE privileges */
@@ -3056,6 +3058,8 @@ convert_database_priv_string(text *priv_type_text)
{"TEMP WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE_TEMP)},
{"CONNECT", ACL_CONNECT},
{"CONNECT WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CONNECT)},
+ {"ASYNC", ACL_ASYNC},
+ {"ASYNC WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_ASYNC)},
{NULL, 0}
};
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 49c2a31..1b03fff 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -72,7 +72,8 @@ typedef uint32 AclMode; /* a bitmask of privilege bits */
#define ACL_CREATE (1<<9) /* for namespaces and databases */
#define ACL_CREATE_TEMP (1<<10) /* for databases */
#define ACL_CONNECT (1<<11) /* for databases */
-#define N_ACL_RIGHTS 12 /* 1 plus the last 1<<x */
+#define ACL_ASYNC (1<<12) /* for databases */
+#define N_ACL_RIGHTS 13 /* 1 plus the last 1<<x */
#define ACL_NO_RIGHTS 0
/* Currently, SELECT ... FOR [KEY] UPDATE/SHARE requires UPDATE privileges */
#define ACL_SELECT_FOR_UPDATE ACL_UPDATE
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 2116259..77b2cf1 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -137,9 +137,10 @@ typedef ArrayType Acl;
#define ACL_CREATE_CHR 'C'
#define ACL_CREATE_TEMP_CHR 'T'
#define ACL_CONNECT_CHR 'c'
+#define ACL_ASYNC_CHR 'n'
/* string holding all privilege code chars, in order by bitmask position */
-#define ACL_ALL_RIGHTS_STR "arwdDxtXUCTc"
+#define ACL_ALL_RIGHTS_STR "arwdDxtXUCTcn"
/*
* Bitmasks defining "all rights" for each supported object type
@@ -147,7 +148,7 @@ typedef ArrayType Acl;
#define ACL_ALL_RIGHTS_COLUMN (ACL_INSERT|ACL_SELECT|ACL_UPDATE|ACL_REFERENCES)
#define ACL_ALL_RIGHTS_RELATION (ACL_INSERT|ACL_SELECT|ACL_UPDATE|ACL_DELETE|ACL_TRUNCATE|ACL_REFERENCES|ACL_TRIGGER)
#define ACL_ALL_RIGHTS_SEQUENCE (ACL_USAGE|ACL_SELECT|ACL_UPDATE)
-#define ACL_ALL_RIGHTS_DATABASE (ACL_CREATE|ACL_CREATE_TEMP|ACL_CONNECT)
+#define ACL_ALL_RIGHTS_DATABASE (ACL_CREATE|ACL_CREATE_TEMP|ACL_CONNECT|ACL_ASYNC)
#define ACL_ALL_RIGHTS_FDW (ACL_USAGE)
#define ACL_ALL_RIGHTS_FOREIGN_SERVER (ACL_USAGE)
#define ACL_ALL_RIGHTS_FUNCTION (ACL_EXECUTE)
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 3da03fc..dc5ea33 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1400,6 +1400,14 @@ revoke select on dep_priv_test from regressuser4 cascade;
set session role regressuser1;
drop table dep_priv_test;
+-- Async privileges
+\c -
+grant async on database regression to regressuser1;
+set session role regressuser1;
+listen some_channel;
+set session role regressuser2;
+listen some_channel;
+ERROR: permission denied for database regression
-- clean up
\c
drop sequence x_seq;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index cb993ae..e462976 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -836,6 +836,15 @@ revoke select on dep_priv_test from regressuser4 cascade;
set session role regressuser1;
drop table dep_priv_test;
+-- Async privileges
+
+\c -
+
+grant async on database regression to regressuser1;
+set session role regressuser1;
+listen some_channel;
+set session role regressuser2;
+listen some_channel;
-- clean up
--
1.8.1.2
Chris Farmiloe <chrisfarms@gmail.com> writes:
I find the current LISTEN / NOTIFY rather limited in the context of
databases with multiple roles. As it stands it is not possible to restrict
the use of LISTEN or NOTIFY to specific roles, and therefore notifications
(and their payloads) cannot really be trusted as coming from any particular
source.
TBH, nobody has complained about this in the fifteen-plus years that
LISTEN has been around. I'm dubious about adding privilege-checking
overhead for everybody to satisfy a complaint from one person.
I'd like to propose a new ASYNC database privilege that would control
whether a role can use LISTEN, NOTIFY and UNLISTEN statements and the
associated pg_notify function.
... and if I did think that there were an issue here, I doubt I'd think
that a privilege as coarse-grained as that would fix it. Surely you'd
want per-channel privileges if you were feeling paranoid about this,
not to mention separate read and write privileges. But the demand for
that just isn't out there.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
In fairness NOTIFY has only had a payload since v9 (maybe 8.4?), and the
issue of trust is mainly tied to data leaking from the payload, so I
suspect I won't be last person to request this as people re-visit NOTIFY :)
...but I totally get your point of course.
My first thought was also that having control at the channel-level would be
ideal, but that would be a huge implementation change by the looks of
things, would certainly affect performance a great deal more and would not
really give much more benefit that could be attained with database-level
control + a "SECURITY DEFINER" function.
Chris
On 20 May 2013 03:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Chris Farmiloe <chrisfarms@gmail.com> writes:
I find the current LISTEN / NOTIFY rather limited in the context of
databases with multiple roles. As it stands it is not possible torestrict
the use of LISTEN or NOTIFY to specific roles, and therefore
notifications
(and their payloads) cannot really be trusted as coming from any
particular
source.
TBH, nobody has complained about this in the fifteen-plus years that
LISTEN has been around. I'm dubious about adding privilege-checking
overhead for everybody to satisfy a complaint from one person.I'd like to propose a new ASYNC database privilege that would control
whether a role can use LISTEN, NOTIFY and UNLISTEN statements and the
associated pg_notify function.... and if I did think that there were an issue here, I doubt I'd think
that a privilege as coarse-grained as that would fix it. Surely you'd
want per-channel privileges if you were feeling paranoid about this,
not to mention separate read and write privileges. But the demand for
that just isn't out there.regards, tom lane
On 05/20/2013 09:54 AM, Chris Farmiloe wrote:
Hey all,
I find the current LISTEN / NOTIFY rather limited in the context of
databases with multiple roles. As it stands it is not possible to restrict
the use of LISTEN or NOTIFY to specific roles, and therefore notifications
(and their payloads) cannot really be trusted as coming from any particular
source.
Just for context, here's the dba.stackexchange.com question where this
was previously raised:
http://dba.stackexchange.com/q/42496/7788
I agree that this only became relevant since NOTIFY payload support was
added, so we can't really just say "nobody's wanted this in 15 years". A
couple of years, maybe, but given the lagging client driver support for
NOTIFY payloads, maybe not even that.
That said, I personally don't see this as a significant problem since
the approach that worked prior to the introduction of notify payloads -
just using a staging table - continues to work just fine. I agree with
Tom that much finer grained control would be required if adding
privileges at all were to be particularly useful; such a coarse right as
Chris proposes seems unlikely to cover many use cases. We'd likely
control over who can NOTIFY and who can LISTEN to a given channel name.
I'm just not convinced it's worth the complexity it'll introduce or any
performance consequences. If it could be done by adding a few hooks that
a C extension could use, that might be worthwhile though.
The same argument might be applied to advisory locking, after all. I
can't really see adding fine-grained rights to _everything_, but hooks
might not be harmful.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers