Re: [HACKERS] pg_terminate_backend and pg_cancel_backend by not administrator user
2011/6/2 Noah Misch n...@leadboat.com: On Wed, Jun 01, 2011 at 10:26:34PM -0400, Josh Kupershmidt wrote: On Wed, Jun 1, 2011 at 5:55 PM, Noah Misch n...@leadboat.com wrote: On Sun, May 29, 2011 at 10:56:02AM -0400, Josh Kupershmidt wrote: Looking around, I see there were real problems[1] with sending SIGTERM to individual backends back in 2005 or so, and pg_terminate_backend() was only deemed safe enough to put in for 8.4 [2]. So expanding pg_terminate_backend() privileges does make me a tad nervous. The documentation for the CREATE USER flag would boil down to omit this flag only if you're worried about undiscovered PostgreSQL bugs in this area. ?I'd echo Tom's sentiment from the first thread, In any case I think we have to solve it, not create new mechanisms to try to ignore it. I do agree with Tom's sentiment from that thread. But, if we are confident that pg_terminate_backend() is safe enough to relax permissions on, then I take it you agree we should plan to extend this power to all users? Yes; that's what I was trying to say. Having thought about this some more, I do now see a risk. Currently, a SECURITY DEFINER function (actually any function, but that's where it matters) can trap query_canceled. By doing so, the author can ensure that only superusers and crashes may halt the function during a section protected in this way. One might use it to guard a series of updates made over dblink. pg_terminate_backend() breaks this protection. I've never designed something this way; it only suffices when you merely sort-of-care about transactional integrity. Perhaps it's an acceptable loss for this feature? And if so, is this patch a good first step on that path? Understand that the pg_terminate_backend() is able to kill process that need not to be killed. I suppose that looking inside the internal postgreql table in order to not allow a normal db owner to kill a superuser connection can avoid this problem? Yes. Reading through those old threads made me realize this patch would give database owners the ability to kill off autovacuum workers. Seems like we'd want to restrict that power to superusers. Would we? ?Any old user can already stifle VACUUM by holding a transaction open. This is true, though it's possible we might at some point want a backend process which really shouldn't be killable by non-superusers (if vacuum/autovacuum isn't one already.) Actually, I could easily imagine a superuser running an important query on a database getting peeved if a non-superuser were allowed to cancel/terminate his queries. That's really a different level of user isolation than we have. If your important query runs on a database owned by someone else, calls functions owned by someone else, or reads tables owned by someone else, you're substantially at the mercy of those object owners. That situation probably is unsatisfactory to some folks. Adding the possibility that a database owner could cancel your query seems like an extension of that codependency more than a new exposure. If I am the database owner I need to be able to manage my DB. Ok for superuser connection (and internal administrative process like autovacuum) I am the developer, not the DBA, so sometimes, when I wrong something, I need to kill my session if I wrong something Can we suppose, in a more generic case, that an user can kill connection only from the same user even if this is not the database owner? Best Regards, Torello -- 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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Wed, Jun 1, 2011 at 5:55 PM, Noah Misch n...@leadboat.com wrote: On Sun, May 29, 2011 at 10:56:02AM -0400, Josh Kupershmidt wrote: Looking around, I see there were real problems[1] with sending SIGTERM to individual backends back in 2005 or so, and pg_terminate_backend() was only deemed safe enough to put in for 8.4 [2]. So expanding pg_terminate_backend() privileges does make me a tad nervous. The documentation for the CREATE USER flag would boil down to omit this flag only if you're worried about undiscovered PostgreSQL bugs in this area. I'd echo Tom's sentiment from the first thread, In any case I think we have to solve it, not create new mechanisms to try to ignore it. I do agree with Tom's sentiment from that thread. But, if we are confident that pg_terminate_backend() is safe enough to relax permissions on, then I take it you agree we should plan to extend this power to all users? And if so, is this patch a good first step on that path? Reading through those old threads made me realize this patch would give database owners the ability to kill off autovacuum workers. Seems like we'd want to restrict that power to superusers. Would we? Any old user can already stifle VACUUM by holding a transaction open. This is true, though it's possible we might at some point want a backend process which really shouldn't be killable by non-superusers (if vacuum/autovacuum isn't one already.) Actually, I could easily imagine a superuser running an important query on a database getting peeved if a non-superuser were allowed to cancel/terminate his queries. Josh -- 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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Sun, May 29, 2011 at 5:04 AM, Noah Misch n...@leadboat.com wrote:
What risks arise from unconditionally allowing these calls for the same user's
backends? `pg_cancel_backend' ought to be safe enough; the user always has
access to the standard cancellation protocol, making the SQL interface a mere
convenience (albeit a compelling one). `pg_terminate_backend' does open up
access to a new behavior, but no concrete risks come to mind.
Looking around, I see there were real problems[1] with sending SIGTERM
to individual backends back in 2005 or so, and pg_terminate_backend()
was only deemed safe enough to put in for 8.4 [2]. So expanding
pg_terminate_backend() privileges does make me a tad nervous.
Reading through those old threads made me realize this patch would
give database owners the ability to kill off autovacuum workers. Seems
like we'd want to restrict that power to superusers.
On the other hand, this *would* be substantial new authority for database
owners. Seems like a reasonable authority to grant, though.
And I also realized that this patch's approach might force us to
maintain a permissions wart if we ever want to implement fine-grained
control for this stuff, e.g. a per-role setting enabling self-kills.
It would be a bit lame to have to document Use this CREATE/ALTER ROLE
flag. Or be the database owner. Or be a superuser.
Now, a few technical comments about the patch:
1.) This bit looks dangerous:
+ backend = pgstat_fetch_stat_beentry(i);
+ if (backend-st_procpid == pid) {
Since pgstat_fetch_stat_beentry() might return NULL.
I think you want BackendPidGetProc().
Ah, thanks for the pointer.
Josh
--
[1]
http://postgresql.1045698.n5.nabble.com/pg-terminate-backend-idea-td1930120.html
[2]
http://postgresql.1045698.n5.nabble.com/Re-COMMITTERS-pgsql-Add-pg-terminate-backend-to-allow-terminating-only-a-single-td1983763i20.html
--
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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Fri, Mar 11, 2011 at 8:54 AM, Bruce Momjian br...@momjian.us wrote:
I have added it to the next commit fest.
Hi Torello,
I have volunteered (more accurately, Greg Smith volunteered me :-)
to be a reviewer for this patch. I know you're a bit new here, so I
thought I'd outline where this patch stands and what's expected if
you'd like to move it along.
We organize patch reviews via commitfests lasting a month or so.
Some more information about this process:
http://wiki.postgresql.org/wiki/CommitFest
Each commitfest is a period wherein you can expect to receive some
feedback on your patch and advice on things which might need to be
improved (in this case, it's my job to provide you this feedback).
Your patch is in the upcoming commitfest, scheduled to run from June
15 to July 14.
So if you're interested in being responsible for this patch, or some
variant of it, eventually making its way into PostgreSQL 9.2, you
should be willing to update your patch based on feedback, request
advice, etc. during this period. If you're not interested in getting
sucked into this process that's OK -- just please advise us if that's
the case, and maybe someone else will be willing to take charge of the
patch.
Anssi and I posted some initial feedback on the patch's goals earlier.
I would like to ultimately see users have the capability to
pg_cancel_backend() their own queries. But I could at least conceive
of others not wanting this behavior enabled by default. So perhaps
this patch's approach of granting extra privs to the database owner
could work as a first attempt. And maybe a later version could
introduce a GUC allowing the DBA to control whether users can
cancel/terminate their backends, or we could instead have an option
flag to CREATE/ALTER ROLE, allowing per-user configuration.
It would be helpful to hear from others whether this patch's goals
would work as a first pass at this problem, so that Torello doesn't
waste time on a doomed approach. Also, it might be helpful to add an
entry on the Todo list for 'allow non-superusers to use
pg_cancel_backend()', in case this patch gets sunk.
Now, a few technical comments about the patch:
1.) This bit looks dangerous:
+backend = pgstat_fetch_stat_beentry(i);
+if (backend-st_procpid == pid) {
Since pgstat_fetch_stat_beentry() might return NULL.
I'm a bit suspicious about whether looping through
pgstat_fetch_stat_beentry() is the best way to determine the database
owner for a given backend PID, but I haven't dug in enough yet to
suggest a better alternative.
2.) The way the code inside pg_signal_backend() is structured, doing:
select pg_cancel_backend(12345);
as non-superuser, where '12345' is a fictitious PID, can now give you
the incorrect error message:
ERROR: must be superuser or target database owner to signal other
server processes
3.) No documentation adjustments, and the comments need some cleaup.
Torello: I'll be happy to handle comments/documentation for you as a
native English speaker, so you don't have to worry about this part.
That's it for now. Torello, I look forward to hearing back from you,
and hope that you have some time to work on this patch further.
Josh
--
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] pg_terminate_backend and pg_cancel_backend by not administrator user
Kevin Grittner wrote: Torello Querci tque...@gmail.com wrote: I attach a path for this It's too late in the release cycle to consider this for version 9.1. Please add it to the open CommitFest for consideration for 9.2: https://commitfest.postgresql.org/action/commitfest_view/open I have added it to the next commit fest. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] pg_terminate_backend and pg_cancel_backend by not administrator user
On Mon, Feb 14, 2011 at 8:58 AM, Anssi Kääriäinen anssi.kaariai...@thl.fi wrote: On 02/14/2011 02:10 PM, Torello Querci wrote: I suppose that give the right to the owner db user to terminate or cancel other session connected to the database which it is owner is a good thing. I not see any security problem because this user can cancel or terminate only the session related with the own database, but if you think that this is a problem, a configuration parameter can be used. For what it's worth, a big +1 from me. We have pretty much the same use case. It would be good if you could also terminate your own connections. The superuser-only restriction for pg_cancel_backend() has been a pet peeve of mine as well. I actually posted a patch a while back to let users pg_cancel_backend() their own queries, see: http://archives.postgresql.org/pgsql-admin/2010-02/msg00052.php IMO it'd be better to do away with this patch's check of: /* If the user not is the superuser, need to be the db owner. */ and instead just check if the target session's user matches that of the cancel requester. Additionally, this patch keeps all the permission checking inside pg_signal_backend(). That's fine if we're sure that we want pg_cancel_backend() and pg_terminate_backend() to undergo the same permissions check, but perhaps it's a bad idea to relax the permissions check on pg_terminate_backend() ? Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_terminate_backend and pg_cancel_backend by not administrator user
Hi,
this is the first time that I post here, so if I wrong please don't kill me ...
I see that pg_terminate_backend and pg_cancel_backend can be execute
only by admin users.
This approach seems to be too restrictive in a lots of real situation.
In dept, I have a situation where it is created one database machine
for all the postgresql database.
This database machine is managed by IT staff that have created two
user for each application.
One user is the owner db user that create, drop, grant on this db,
while the other user is the application db.
In this situation I (the developer) not able to disconnect any client
and stop any high weight queries.
Unfortunately the application run on application server that is
manager, again, by IT staff and I not have the right to stop it.
I suppose that give the right to the owner db user to terminate or
cancel other session connected to the database which it is owner is a
good thing.
I not see any security problem because this user can cancel or
terminate only the session related with the own database,
but if you think that this is a problem, a configuration parameter can be used.
Of course I can create a function with admin right that do the same
thing but the IT staff need to install, configure, and give the right
grant.
So, I suppose, that this can to be only a workaround, not the solution.
Sorry for my English.
I attach a path for this
Best Regards, Torello
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 5bda4af..5327447 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -33,6 +33,7 @@
#include storage/procarray.h
#include utils/builtins.h
#include tcop/tcopprot.h
+#include pgstat.h
#define atooid(x) ((Oid) strtoul((x), NULL, 10))
@@ -75,9 +76,33 @@ static bool
pg_signal_backend(int pid, int sig)
{
if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg(must be superuser to signal other server processes;
+ {
+bool haveRight = false;
+PgBackendStatus *backend;
+
+ /* If the user not is the superuser, need to be the db owner. */
+ if (pg_database_ownercheck(MyDatabaseId, GetUserId())) {
+
+/* Check for the specify backend in the stat info table */
+int nBackend = pgstat_fetch_stat_numbackends();
+int i;
+for (i = 1; i=nBackend; ++i) {
+backend = pgstat_fetch_stat_beentry(i);
+if (backend-st_procpid == pid) {
+if (backend-st_databaseid == MyDatabaseId)
+haveRight = true;
+break;
+}
+}
+}
+
+if (!haveRight)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+(errmsg(must be superuser or database destination owner to signal other server processes;
+ }
+
+
if (!IsBackendPid(pid))
{
--
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] pg_terminate_backend and pg_cancel_backend by not administrator user
On 02/14/2011 02:10 PM, Torello Querci wrote: I suppose that give the right to the owner db user to terminate or cancel other session connected to the database which it is owner is a good thing. I not see any security problem because this user can cancel or terminate only the session related with the own database, but if you think that this is a problem, a configuration parameter can be used. For what it's worth, a big +1 from me. We have pretty much the same use case. It would be good if you could also terminate your own connections. - Anssi -- 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] pg_terminate_backend and pg_cancel_backend by not administrator user
Torello Querci tque...@gmail.com wrote: I attach a path for this It's too late in the release cycle to consider this for version 9.1. Please add it to the open CommitFest for consideration for 9.2: https://commitfest.postgresql.org/action/commitfest_view/open -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
