Re: [HACKERS] pg_terminate_backend and pg_cancel_backend by not administrator user

2011-07-01 Thread Torello Querci
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

2011-06-01 Thread Josh Kupershmidt
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

2011-05-29 Thread Josh Kupershmidt
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

2011-05-28 Thread Josh Kupershmidt
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

2011-03-11 Thread Bruce Momjian
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

2011-02-27 Thread Josh Kupershmidt
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


Re: [HACKERS] pg_terminate_backend and pg_cancel_backend by not administrator user

2011-02-14 Thread Anssi Kääriäinen

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

2011-02-14 Thread Kevin Grittner
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