Re: [HACKERS] Review of GetUserId() Usage
Jeevan, * Jeevan Chalke (jeevan.cha...@gmail.com) wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed I have reviewed the patch. Patch is excellent in shape and does what is expected and discussed. Also changes are straight forward too. Great, thanks! So looks good to go in. However I have one question: What is the motive for splitting the function return value from SIGNAL_BACKEND_NOPERMISSION into SIGNAL_BACKEND_NOSUPERUSER and SIGNAL_BACKEND_NOPERMISSION? Is that required for some other upcoming patches OR just for simplicity? That was done to provide a more useful error-message to the user. It's not strictly required, I'll grant, but I don't see a reason to avoid doing it either. Currently, we have combined error for both which is simply split into two. No issue as such, just curious as it does not go well with the subject. It seemed reasonable to me to improve the clarity of the error messages. You can mark this for ready for committer. Done. I've also claimed it as a committer and, barring objections, will go ahead and push it soonish. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed I have reviewed the patch. Patch is excellent in shape and does what is expected and discussed. Also changes are straight forward too. So looks good to go in. However I have one question: What is the motive for splitting the function return value from SIGNAL_BACKEND_NOPERMISSION into SIGNAL_BACKEND_NOSUPERUSER and SIGNAL_BACKEND_NOPERMISSION? Is that required for some other upcoming patches OR just for simplicity? Currently, we have combined error for both which is simply split into two. No issue as such, just curious as it does not go well with the subject. You can mark this for ready for committer. The new status of this patch is: Waiting on Author -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
On Fri, Dec 5, 2014 at 11:28 PM, Stephen Frost sfr...@snowman.net wrote: * Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@snowman.net) wrote: 3. It messes around with pg_signal_backend(). There are currently no cases in which pg_signal_backend() throws an error, which is good, because it lets you write queries against pg_stat_activity() that don't fail halfway through, even if you are missing permissions on some things. This patch introduces such a case, which is bad. Good point, I'll move that check up into the other functions, which will allow for a more descriptive error as well. Err, I'm missing something here, as pg_signal_backend() is a misc.c static internal function? How would you be calling it from a query against pg_stat_activity()? I'm fine making the change anyway, just curious.. Updated patch attached which move the ereport() out of pg_signal_backend() and into its callers, as the other permissions checks are done, and includes the documentation changes. The error messages are minimally changed to match the new behvaior. Moving to next CF, this patch did not get reviews. -- Michael
Re: [HACKERS] Review of GetUserId() Usage
* Stephen Frost (sfr...@snowman.net) wrote:
* Stephen Frost (sfr...@snowman.net) wrote:
3. It messes around with pg_signal_backend(). There are currently no
cases in which pg_signal_backend() throws an error, which is good,
because it lets you write queries against pg_stat_activity() that
don't fail halfway through, even if you are missing permissions on
some things. This patch introduces such a case, which is bad.
Good point, I'll move that check up into the other functions, which will
allow for a more descriptive error as well.
Err, I'm missing something here, as pg_signal_backend() is a misc.c
static internal function? How would you be calling it from a query
against pg_stat_activity()?
I'm fine making the change anyway, just curious..
Updated patch attached which move the ereport() out of
pg_signal_backend() and into its callers, as the other permissions
checks are done, and includes the documentation changes. The error
messages are minimally changed to match the new behvaior.
Thanks,
Stephen
From c60718e7a7ecb8d671627271533d6bd2b6878782 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Mon, 1 Dec 2014 11:13:09 -0500
Subject: [PATCH] GetUserId() changes to has_privs_of_role()
The pg_stat and pg_signal-related functions have been using GetUserId()
instead of has_privs_of_role() for checking if the current user should
be able to see details in pg_stat_activity or signal other processes,
requiring a user to do 'SET ROLE' for inheirited roles for a permissions
check, unlike other permissions checks.
This patch changes that behavior to, instead, act like most other
permission checks and use has_privs_of_role(), removing the 'SET ROLE'
need. Documentation and error messages updated accordingly.
Per discussion with Alvaro, Peter, Adam (though not using Adam's patch),
and Robert.
---
doc/src/sgml/func.sgml | 13 ++---
src/backend/utils/adt/misc.c| 39 +
src/backend/utils/adt/pgstatfuncs.c | 19 +-
3 files changed, 47 insertions(+), 24 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 62ec275..dbb61dc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -16121,9 +16121,9 @@ SELECT set_config('log_statement_stats', 'off', false);
literalfunctionpg_cancel_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
- entryCancel a backend's current query. You can execute this against
-another backend that has exactly the same role as the user calling the
-function. In all other cases, you must be a superuser.
+ entryCancel a backend's current query. This is also allowed if the
+calling role is a member of the role whose backend is being cancelled,
+however only superusers can cancel superuser backends.
/entry
/row
row
@@ -16145,10 +16145,9 @@ SELECT set_config('log_statement_stats', 'off', false);
literalfunctionpg_terminate_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
- entryTerminate a backend. You can execute this against
-another backend that has exactly the same role as the user
-calling the function. In all other cases, you must be a
-superuser.
+ entryTerminate a backend. This is also allowed if the calling role
+is a member of the role whose backend is being terminated, however only
+superusers can terminate superuser backends.
/entry
/row
/tbody
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 67539ec..2e74eb9 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -37,6 +37,7 @@
#include utils/lsyscache.h
#include utils/ruleutils.h
#include tcop/tcopprot.h
+#include utils/acl.h
#include utils/builtins.h
#include utils/timestamp.h
@@ -81,7 +82,9 @@ current_query(PG_FUNCTION_ARGS)
* role as the backend being signaled. For dangerous signals, an explicit
* check for superuser needs to be done prior to calling this function.
*
- * Returns 0 on success, 1 on general failure, and 2 on permission error.
+ * Returns 0 on success, 1 on general failure, 2 on normal permission error
+ * and 3 if the caller needs to be a superuser.
+ *
* In the event of a general failure (return code 1), a warning message will
* be emitted. For permission errors, doing that is the responsibility of
* the caller.
@@ -89,6 +92,7 @@ current_query(PG_FUNCTION_ARGS)
#define SIGNAL_BACKEND_SUCCESS 0
#define SIGNAL_BACKEND_ERROR 1
#define SIGNAL_BACKEND_NOPERMISSION 2
+#define SIGNAL_BACKEND_NOSUPERUSER 3
static int
pg_signal_backend(int pid, int sig)
{
@@ -113,7 +117,12 @@ pg_signal_backend(int pid, int sig)
return SIGNAL_BACKEND_ERROR;
}
- if (!(superuser() ||
Re: [HACKERS] Review of GetUserId() Usage
On 2014-12-05 09:28:13 -0500, Stephen Frost wrote:
static int
pg_signal_backend(int pid, int sig)
{
@@ -113,7 +117,12 @@ pg_signal_backend(int pid, int sig)
return SIGNAL_BACKEND_ERROR;
}
- if (!(superuser() || proc-roleId == GetUserId()))
+ /* Only allow superusers to signal superuser-owned backends. */
+ if (superuser_arg(proc-roleId) !superuser())
+ return SIGNAL_BACKEND_NOSUPERUSER;
+
+ /* Users can signal backends they have role membership in. */
+ if (!has_privs_of_role(GetUserId(), proc-roleId))
return SIGNAL_BACKEND_NOPERMISSION;
/*
@@ -141,35 +150,49 @@ pg_signal_backend(int pid, int sig)
}
Is the 'Only allow superusers to signal superuser-owned backends' check
actually safe that way? I personally try to never use a superuser role
as the login user, but grant my account a superuser role that doesn't
inherit. But IIRC PGPROC-roleId won't change, even if a user does SET
ROLE.
Greetings,
Andres Freund
--
Andres Freund 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
Re: [HACKERS] Review of GetUserId() Usage
* Andres Freund (and...@2ndquadrant.com) wrote: Is the 'Only allow superusers to signal superuser-owned backends' check actually safe that way? I personally try to never use a superuser role as the login user, but grant my account a superuser role that doesn't inherit. But IIRC PGPROC-roleId won't change, even if a user does SET ROLE. You're correct- but it's exactly the same as it is today. If you grant another user your role and then they 'SET ROLE' to you, they can cancel any of your queries or terminate your backends, regardless of if those roles have done some other 'SET ROLE'. This change only removes the need for those users to 'SET ROLE' to your user first. The backend isn't considered 'superuser-owned' unless it's the login role that's a superuser. It might be interesting to change that to mean 'when a SET ROLE to superuser has been done', but what about security definer functions or other transient escalation to superuser? Would those calls have to muck with PGPROC-roleId? If we want to go there, it should definitely be a different patch. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost sfr...@snowman.net wrote: * Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@snowman.net) wrote: Attached is a patch to address the pg_cancel/terminate_backend and the statistics info as discussed previously. It sounds like we're coming to And I forgot the attachment, of course. Apologies. Updated patch attached which also changes the error messages (which hadn't been updated in the prior versions and really should be). Barring objections, I plan to move forward with this one and get this relatively minor change wrapped up. As mentioned in the commit, while this might be an arguably back-patchable change, the lack of field complaints and the fact that it changes existing behavior mean it should go only against master, imv. This patch does a couple of different things: 1. It makes more of the crappy error message change that Andres and I already objected to on the other thread. Whether you disagree with those objections or not, don't make an end-run around them by putting more of the same stuff into patches on other threads. 2. It changes the functions in pgstatfuncs.c so that you can see the relevant information not only for your own role, but also for roles of which you are a member. That seems fine, but do we need a documentation change someplace? 3. It messes around with pg_signal_backend(). There are currently no cases in which pg_signal_backend() throws an error, which is good, because it lets you write queries against pg_stat_activity() that don't fail halfway through, even if you are missing permissions on some things. This patch introduces such a case, which is bad. I think it's unfathomable that you would consider anything in this patch a back-patchable bug fix. It's clearly a straight-up behavior change... or more properly three different changes, only one of which I agree with. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
Robert, * Robert Haas (robertmh...@gmail.com) wrote: On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost sfr...@snowman.net wrote: * Stephen Frost (sfr...@snowman.net) wrote: Updated patch attached which also changes the error messages (which hadn't been updated in the prior versions and really should be). Barring objections, I plan to move forward with this one and get this relatively minor change wrapped up. As mentioned in the commit, while this might be an arguably back-patchable change, the lack of field complaints and the fact that it changes existing behavior mean it should go only against master, imv. This patch does a couple of different things: 1. It makes more of the crappy error message change that Andres and I already objected to on the other thread. Whether you disagree with those objections or not, don't make an end-run around them by putting more of the same stuff into patches on other threads. The error message clearly needed to be updated either way or I wouldn't have touched it. I changed it to match what I feel is the prevelant and certainly more commonly seen messaging from PG when it comes to permissions errors, and drew attention to it by commenting on the fact that I changed it. Doing otherwise would have drawn similar criticism (is it did upthread, by Peter or Alvaro, I believe..) that I wasn't updating it to match the messaging which we should be using. 2. It changes the functions in pgstatfuncs.c so that you can see the relevant information not only for your own role, but also for roles of which you are a member. That seems fine, but do we need a documentation change someplace? Yes, I've added the documentation changes to my branch, just hadn't posted an update yet (travelling today). 3. It messes around with pg_signal_backend(). There are currently no cases in which pg_signal_backend() throws an error, which is good, because it lets you write queries against pg_stat_activity() that don't fail halfway through, even if you are missing permissions on some things. This patch introduces such a case, which is bad. Good point, I'll move that check up into the other functions, which will allow for a more descriptive error as well. I think it's unfathomable that you would consider anything in this patch a back-patchable bug fix. It's clearly a straight-up behavior change... or more properly three different changes, only one of which I agree with. I didn't think it was back-patchable and stated as much. I anticipated that argument and provided my thoughts on it. I *do* think it's wrong to be using GetUserId() in this case and it's only very slightly mollified by being documented that way. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
* Stephen Frost (sfr...@snowman.net) wrote: 3. It messes around with pg_signal_backend(). There are currently no cases in which pg_signal_backend() throws an error, which is good, because it lets you write queries against pg_stat_activity() that don't fail halfway through, even if you are missing permissions on some things. This patch introduces such a case, which is bad. Good point, I'll move that check up into the other functions, which will allow for a more descriptive error as well. Err, I'm missing something here, as pg_signal_backend() is a misc.c static internal function? How would you be calling it from a query against pg_stat_activity()? I'm fine making the change anyway, just curious.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
On Tue, Dec 2, 2014 at 2:50 PM, Stephen Frost sfr...@snowman.net wrote: 1. It makes more of the crappy error message change that Andres and I already objected to on the other thread. Whether you disagree with those objections or not, don't make an end-run around them by putting more of the same stuff into patches on other threads. The error message clearly needed to be updated either way or I wouldn't have touched it. I changed it to match what I feel is the prevelant and certainly more commonly seen messaging from PG when it comes to permissions errors, and drew attention to it by commenting on the fact that I changed it. Doing otherwise would have drawn similar criticism (is it did upthread, by Peter or Alvaro, I believe..) that I wasn't updating it to match the messaging which we should be using. OK, I guess that's a fair point. I think it's unfathomable that you would consider anything in this patch a back-patchable bug fix. It's clearly a straight-up behavior change... or more properly three different changes, only one of which I agree with. I didn't think it was back-patchable and stated as much. I anticipated that argument and provided my thoughts on it. I *do* think it's wrong to be using GetUserId() in this case and it's only very slightly mollified by being documented that way. It's not wrong. It's just different than what you happen to prefer. It's fine to want to change things, but not the way I would have done it is not the same as arguably a bug. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
All, * Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@snowman.net) wrote: Attached is a patch to address the pg_cancel/terminate_backend and the statistics info as discussed previously. It sounds like we're coming to And I forgot the attachment, of course. Apologies. Updated patch attached which also changes the error messages (which hadn't been updated in the prior versions and really should be). Barring objections, I plan to move forward with this one and get this relatively minor change wrapped up. As mentioned in the commit, while this might be an arguably back-patchable change, the lack of field complaints and the fact that it changes existing behavior mean it should go only against master, imv. Thanks, Stephen From bc1913464421921b7f846bcad332698ca6369e75 Mon Sep 17 00:00:00 2001 From: Stephen Frost sfr...@snowman.net Date: Sat, 29 Nov 2014 14:53:08 -0500 Subject: [PATCH] GetUserId() Cleanup for pg_stat and pg_signal The pg_stat and pg_signal-related functions have been using GetUserId() instead of has_privs_of_role() for checking if the current user should be able to see see details in pg_stat_activity or signal other processes. This is arguably a bug in existing branches, but as it's a user-visible change and we haven't gotten complaints about it, it's probably best to just do it in master. Per discussion with Alvaro, Peter and Adam (though not using Adam's patch). --- src/backend/utils/adt/misc.c| 31 +++ src/backend/utils/adt/pgstatfuncs.c | 19 ++- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c index 67539ec..f3dca1f 100644 --- a/src/backend/utils/adt/misc.c +++ b/src/backend/utils/adt/misc.c @@ -37,6 +37,7 @@ #include utils/lsyscache.h #include utils/ruleutils.h #include tcop/tcopprot.h +#include utils/acl.h #include utils/builtins.h #include utils/timestamp.h @@ -113,8 +114,16 @@ pg_signal_backend(int pid, int sig) return SIGNAL_BACKEND_ERROR; } - if (!(superuser() || proc-roleId == GetUserId())) - return SIGNAL_BACKEND_NOPERMISSION; + /* Only allow superusers to signal superuser-owned backends. */ + if (superuser_arg(proc-roleId) !superuser()) + ereport(ERROR, +(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + (errmsg(permission denied to send signal to other process), + errdetail(You must be a superuser to signal processes owned by superusers.; + + /* Users can signal backends they have role membership in. */ + if (!has_privs_of_role(GetUserId(), proc-roleId)) + return SIGNAL_BACKEND_NOPERMISSION; /* * Can the process we just validated above end, followed by the pid being @@ -141,8 +150,10 @@ pg_signal_backend(int pid, int sig) } /* - * Signal to cancel a backend process. This is allowed if you are superuser or - * have the same role as the process being canceled. + * Signal to cancel a backend process. This is allowed if you are a member of + * the role whose process is being canceled. + * + * Note that only superusers can signal superuser-owned processes. */ Datum pg_cancel_backend(PG_FUNCTION_ARGS) @@ -152,14 +163,17 @@ pg_cancel_backend(PG_FUNCTION_ARGS) if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg(must be superuser or have the same role to cancel queries running in other server processes; + (errmsg(permission denied to cancel query), + errdetail(You must be a member of the role whose query you are cancelling.; PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS); } /* - * Signal to terminate a backend process. This is allowed if you are superuser - * or have the same role as the process being terminated. + * Signal to terminate a backend process. This is allowed if you are a member + * of the role whose process is being terminated. + * + * Note that only superusers can signal superuser-owned processes. */ Datum pg_terminate_backend(PG_FUNCTION_ARGS) @@ -169,7 +183,8 @@ pg_terminate_backend(PG_FUNCTION_ARGS) if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg(must be superuser or have the same role to terminate other server processes; + (errmsg(permission denied to terminate server process), + errdetail(You must be a member of the role whose process you are terminating.; PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS); } diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index d621a68..3663e93 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -20,6 +20,7 @@ #include libpq/ip.h #include miscadmin.h #include pgstat.h +#include utils/acl.h #include utils/builtins.h #include utils/inet.h #include utils/timestamp.h @@ -674,8 +675,8 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) else nulls[15]
Re: [HACKERS] Review of GetUserId() Usage
* Stephen Frost (sfr...@snowman.net) wrote:
Attached is a patch to address the pg_cancel/terminate_backend and the
statistics info as discussed previously. It sounds like we're coming to
And I forgot the attachment, of course. Apologies.
Thanks,
Stephen
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
new file mode 100644
index 67539ec..6924fb7
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 37,42
--- 37,43
#include utils/lsyscache.h
#include utils/ruleutils.h
#include tcop/tcopprot.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/timestamp.h
*** pg_signal_backend(int pid, int sig)
*** 113,121
return SIGNAL_BACKEND_ERROR;
}
! if (!(superuser() || proc-roleId == GetUserId()))
return SIGNAL_BACKEND_NOPERMISSION;
/*
* Can the process we just validated above end, followed by the pid being
* recycled for a new process, before reaching here? Then we'd be trying
--- 114,127
return SIGNAL_BACKEND_ERROR;
}
! /* Only allow superusers to signal superuser-owned backends. */
! if (superuser_arg(proc-roleId) !superuser())
return SIGNAL_BACKEND_NOPERMISSION;
+ /* Users can signal their own backends (including through membership) */
+ if (!has_privs_of_role(GetUserId(), proc-roleId))
+ return SIGNAL_BACKEND_NOPERMISSION;
+
/*
* Can the process we just validated above end, followed by the pid being
* recycled for a new process, before reaching here? Then we'd be trying
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
new file mode 100644
index d621a68..3663e93
*** a/src/backend/utils/adt/pgstatfuncs.c
--- b/src/backend/utils/adt/pgstatfuncs.c
***
*** 20,25
--- 20,26
#include libpq/ip.h
#include miscadmin.h
#include pgstat.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/inet.h
#include utils/timestamp.h
*** pg_stat_get_activity(PG_FUNCTION_ARGS)
*** 674,681
else
nulls[15] = true;
! /* Values only available to same user or superuser */
! if (superuser() || beentry-st_userid == GetUserId())
{
SockAddr zero_clientaddr;
--- 675,682
else
nulls[15] = true;
! /* Values only available to role member */
! if (has_privs_of_role(GetUserId(), beentry-st_userid))
{
SockAddr zero_clientaddr;
*** pg_stat_get_backend_activity(PG_FUNCTION
*** 877,883
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = backend information not available;
! else if (!superuser() beentry-st_userid != GetUserId())
activity = insufficient privilege;
else if (*(beentry-st_activity) == '\0')
activity = command string not enabled;
--- 878,884
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = backend information not available;
! else if (!has_privs_of_role(GetUserId(), beentry-st_userid))
activity = insufficient privilege;
else if (*(beentry-st_activity) == '\0')
activity = command string not enabled;
*** pg_stat_get_backend_waiting(PG_FUNCTION_
*** 898,904
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_waiting;
--- 899,905
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_waiting;
*** pg_stat_get_backend_activity_start(PG_FU
*** 917,923
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_activity_start_timestamp;
--- 918,924
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_activity_start_timestamp;
*** pg_stat_get_backend_xact_start(PG_FUNCTI
*** 943,949
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_xact_start_timestamp;
--- 944,950
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_xact_start_timestamp;
*** pg_stat_get_backend_start(PG_FUNCTION_AR
*** 965,971
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_proc_start_timestamp;
Re: [HACKERS] Review of GetUserId() Usage
All, * Peter Eisentraut (pete...@gmx.net) wrote: It would be weird if it were inconsistent: some things require role membership, some things require SET ROLE. Try explaining that. Attached is a patch to address the pg_cancel/terminate_backend and the statistics info as discussed previously. It sounds like we're coming to consensus that this is the correct approach. Comments are always welcome, of course, but it's a pretty minor patch and I'd like to move forward with it soon against master. We'll rebase the role attributes patch with these changes and update that patch for review (with a few other changes) after. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
All,
I'll break them into three pieces- superuser() cleanup, GetUserId() -
has_privs_of_role(), and the additional-role-attributes patch will just
depend on the others.
Attached is a patch for the GetUserId() - has_privs_of_role() cleanup for
review.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
new file mode 100644
index 67539ec..42d9a1f
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 34,39
--- 34,40
#include storage/pmsignal.h
#include storage/proc.h
#include storage/procarray.h
+ #include utils/acl.h
#include utils/lsyscache.h
#include utils/ruleutils.h
#include tcop/tcopprot.h
*** pg_signal_backend(int pid, int sig)
*** 113,119
return SIGNAL_BACKEND_ERROR;
}
! if (!(superuser() || proc-roleId == GetUserId()))
return SIGNAL_BACKEND_NOPERMISSION;
/*
--- 114,120
return SIGNAL_BACKEND_ERROR;
}
! if (!has_privs_of_role(GetUserId(), proc-roleId))
return SIGNAL_BACKEND_NOPERMISSION;
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
new file mode 100644
index 44ccd37..ea2cd1e
*** a/src/backend/utils/adt/pgstatfuncs.c
--- b/src/backend/utils/adt/pgstatfuncs.c
***
*** 20,25
--- 20,26
#include libpq/ip.h
#include miscadmin.h
#include pgstat.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/inet.h
#include utils/timestamp.h
*** pg_stat_get_activity(PG_FUNCTION_ARGS)
*** 675,681
nulls[15] = true;
/* Values only available to same user or superuser */
! if (superuser() || beentry-st_userid == GetUserId())
{
SockAddr zero_clientaddr;
--- 676,682
nulls[15] = true;
/* Values only available to same user or superuser */
! if (has_privs_of_role(GetUserId(), beentry-st_userid))
{
SockAddr zero_clientaddr;
*** pg_stat_get_backend_activity(PG_FUNCTION
*** 877,883
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = backend information not available;
! else if (!superuser() beentry-st_userid != GetUserId())
activity = insufficient privilege;
else if (*(beentry-st_activity) == '\0')
activity = command string not enabled;
--- 878,884
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = backend information not available;
! else if (!has_privs_of_role(GetUserId(), beentry-st_userid))
activity = insufficient privilege;
else if (*(beentry-st_activity) == '\0')
activity = command string not enabled;
*** pg_stat_get_backend_waiting(PG_FUNCTION_
*** 898,904
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_waiting;
--- 899,905
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_waiting;
*** pg_stat_get_backend_activity_start(PG_FU
*** 917,923
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_activity_start_timestamp;
--- 918,924
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_activity_start_timestamp;
*** pg_stat_get_backend_xact_start(PG_FUNCTI
*** 943,949
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_xact_start_timestamp;
--- 944,950
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_xact_start_timestamp;
*** pg_stat_get_backend_start(PG_FUNCTION_AR
*** 965,971
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_proc_start_timestamp;
--- 966,972
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_proc_start_timestamp;
*** pg_stat_get_backend_client_addr(PG_FUNCT
*** 989,995
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid !=
Re: [HACKERS] Review of GetUserId() Usage
* Peter Eisentraut (pete...@gmx.net) wrote: On 9/24/14 4:58 PM, Stephen Frost wrote: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: I think the case for pgstat_get_backend_current_activity() and pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make and seems acceptable to me; but I would leave pg_signal_backend out of that discussion, because it has a potentially harmful side effect. By requiring SET ROLE you add an extra layer of protection against mistakes. (Hopefully, pg_signal_backend() is not a routine thing for well-run systems, which means human intervention, and therefore the room for error isn't insignificant.) While I certainly understand where you're coming from, I don't really buy into it. Yes, cancelling a query (the only thing normal users can do anyway- they can't terminate backends) could mean the loss of any in-progress work, but it's not like 'rm' and I don't see that it needs to require extra hoops for individuals to go through. It would be weird if it were inconsistent: some things require role membership, some things require SET ROLE. Try explaining that. Agreed. As a side-note, this change is included in the 'role attributes' patch. Might be worth splitting out if there is interest in back-patching this, but as it's a behavior change, my thinking was that it wouldn't make sense to back-patch. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
Robert, On Thursday, October 16, 2014, Robert Haas robertmh...@gmail.com wrote: On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost sfr...@snowman.net javascript:; wrote: As a side-note, this change is included in the 'role attributes' patch. It's really important that we keep separate changes in separate patches that are committed in separate commits. Otherwise, it gets really confusing. I can do that, but it overlaps with the MONITORING role attribute changes also.. Thanks, Stephen
Re: [HACKERS] Review of GetUserId() Usage
On Thu, Oct 16, 2014 at 11:28 AM, Stephen Frost sfr...@snowman.net wrote: On Thursday, October 16, 2014, Robert Haas robertmh...@gmail.com wrote: On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost sfr...@snowman.net wrote: As a side-note, this change is included in the 'role attributes' patch. It's really important that we keep separate changes in separate patches that are committed in separate commits. Otherwise, it gets really confusing. I can do that, but it overlaps with the MONITORING role attribute changes also.. I'm not sure what your point is. Whether keeping changes separate is easy or hard, and whether things overlap with multiple other things or just one, we need to make the effort to do it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
* Robert Haas (robertmh...@gmail.com) wrote: I'm not sure what your point is. Whether keeping changes separate is easy or hard, and whether things overlap with multiple other things or just one, we need to make the effort to do it. What I was getting at is that the role attributes patch would need to depend on these changes.. If the two are completely independent then one would fail to apply cleanly when/if the other is committed, that's all. I'll break them into three pieces- superuser() cleanup, GetUserId() - has_privs_of_role(), and the additional-role-attributes patch will just depend on the others. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
I'll break them into three pieces- superuser() cleanup, GetUserId() - has_privs_of_role(), and the additional-role-attributes patch will just depend on the others. The superuser() cleanup has already been submitted to the current commitfest. https://commitfest.postgresql.org/action/patch_view?id=1587 -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Review of GetUserId() Usage
On 9/24/14 4:58 PM, Stephen Frost wrote: Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: I think the case for pgstat_get_backend_current_activity() and pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make and seems acceptable to me; but I would leave pg_signal_backend out of that discussion, because it has a potentially harmful side effect. By requiring SET ROLE you add an extra layer of protection against mistakes. (Hopefully, pg_signal_backend() is not a routine thing for well-run systems, which means human intervention, and therefore the room for error isn't insignificant.) While I certainly understand where you're coming from, I don't really buy into it. Yes, cancelling a query (the only thing normal users can do anyway- they can't terminate backends) could mean the loss of any in-progress work, but it's not like 'rm' and I don't see that it needs to require extra hoops for individuals to go through. It would be weird if it were inconsistent: some things require role membership, some things require SET ROLE. Try explaining that. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of GetUserId() Usage
* Peter Eisentraut (pete...@gmx.net) wrote: On 9/24/14 4:58 PM, Stephen Frost wrote: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: I think the case for pgstat_get_backend_current_activity() and pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make and seems acceptable to me; but I would leave pg_signal_backend out of that discussion, because it has a potentially harmful side effect. By requiring SET ROLE you add an extra layer of protection against mistakes. (Hopefully, pg_signal_backend() is not a routine thing for well-run systems, which means human intervention, and therefore the room for error isn't insignificant.) While I certainly understand where you're coming from, I don't really buy into it. Yes, cancelling a query (the only thing normal users can do anyway- they can't terminate backends) could mean the loss of any in-progress work, but it's not like 'rm' and I don't see that it needs to require extra hoops for individuals to go through. It would be weird if it were inconsistent: some things require role membership, some things require SET ROLE. Try explaining that. I agree.. We already have that distinction, through inherit vs. noinherit. I don't think it makes sense to have it also for individual commands which we feel might not be as safe. You could still go delete all their data w/o a set role if you wanted... Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: I think the case for pgstat_get_backend_current_activity() and pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make and seems acceptable to me; but I would leave pg_signal_backend out of that discussion, because it has a potentially harmful side effect. By requiring SET ROLE you add an extra layer of protection against mistakes. (Hopefully, pg_signal_backend() is not a routine thing for well-run systems, which means human intervention, and therefore the room for error isn't insignificant.) While I certainly understand where you're coming from, I don't really buy into it. Yes, cancelling a query (the only thing normal users can do anyway- they can't terminate backends) could mean the loss of any in-progress work, but it's not like 'rm' and I don't see that it needs to require extra hoops for individuals to go through. Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] Review of GetUserId() Usage
Greetings, While looking through the GetUserId() usage in the backend, I couldn't help but notice a few rather questionable cases that, in my view, should be cleaned up. As a reminder, GetUserId() returns the OID of the user we are generally operating as (eg: whatever the 'role' is, as GetUserId() respects SET ROLE). It does NOT include roles which we currently have the privileges of (that would be has_privs_of_role()), nor roles which we could SET ROLE to (that's is_member_of_role, or check_is_... if you want to just error out in failure cases). On to the list- pgstat_get_backend_current_activity() Used to decide if the current activity string should be returned or not. In my view, this is a clear case which should be addressed through has_privs_of_role() instead of requiring the user to SET ROLE to each role they are an inheirited member of to query for what the other sessions are doing. pg_signal_backend() Used to decide if pg_terminate_backend and pg_cancel_backend are allowed. Another case which should be changed over to has_privs_of_role(), in my view. Requiring the user to SET ROLE for roles which they are an inheirited member of is confusing as it's generally not required. pg_stat_get_activity() Used to decide if the state information should be shared. My opinion is the same as above- use has_privs_of_role(). There are a number of other functions in pgstatfuncs.c with similar issues (eg: pg_stat_get_backend_activity(), pg_stat_get_backend_client_port(), and others). Changing these would make things easier for some of our users, I'm sure.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Review of GetUserId() Usage
Stephen Frost wrote: pgstat_get_backend_current_activity() Used to decide if the current activity string should be returned or not. In my view, this is a clear case which should be addressed through has_privs_of_role() instead of requiring the user to SET ROLE to each role they are an inheirited member of to query for what the other sessions are doing. pg_signal_backend() Used to decide if pg_terminate_backend and pg_cancel_backend are allowed. Another case which should be changed over to has_privs_of_role(), in my view. Requiring the user to SET ROLE for roles which they are an inheirited member of is confusing as it's generally not required. pg_stat_get_activity() Used to decide if the state information should be shared. My opinion is the same as above- use has_privs_of_role(). There are a number of other functions in pgstatfuncs.c with similar issues (eg: pg_stat_get_backend_activity(), pg_stat_get_backend_client_port(), and others). I think the case for pgstat_get_backend_current_activity() and pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make and seems acceptable to me; but I would leave pg_signal_backend out of that discussion, because it has a potentially harmful side effect. By requiring SET ROLE you add an extra layer of protection against mistakes. (Hopefully, pg_signal_backend() is not a routine thing for well-run systems, which means human intervention, and therefore the room for error isn't insignificant.) -- Álvaro Herrerahttp://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
