Re: 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

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

Torello Querci <tquerci(at)gmail(dot)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

On Mon, Feb 14, 2011 at 8:58 AM, Anssi Kääriäinen
<anssi(dot)kaariainen(at)thl(dot)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

Kevin Grittner wrote:
> Torello Querci <tquerci(at)gmail(dot)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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Mar 11, 2011 at 8:54 AM, Bruce Momjian <bruce(at)momjian(dot)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

On Sat, May 28, 2011 at 01:44:20PM -0400, Josh Kupershmidt wrote:
> 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.

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.

On the other hand, this *would* be substantial new authority for database
owners. Seems like a reasonable authority to grant, though.

> 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 think you want BackendPidGetProc().

Thanks,
nm

On Sun, May 29, 2011 at 5:04 AM, Noah Misch <noah(at)leadboat(dot)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

On Sun, May 29, 2011 at 10:56:02AM -0400, Josh Kupershmidt wrote:
> On Sun, May 29, 2011 at 5:04 AM, Noah Misch <noah(at)leadboat(dot)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.

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."

> 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.

On Wed, Jun 1, 2011 at 5:55 PM, Noah Misch <noah(at)leadboat(dot)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

On Wed, Jun 01, 2011 at 10:26:34PM -0400, Josh Kupershmidt wrote:
> On Wed, Jun 1, 2011 at 5:55 PM, Noah Misch <noah(at)leadboat(dot)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?

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.

Thanks,
nm

2011/6/2 Noah Misch <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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

On Fri, Jul 01, 2011 at 07:31:30PM +0200, Torello Querci wrote:
> 2011/6/2 Noah Misch <noah(at)leadboat(dot)com>:

> > 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?

Checking whether a session is authenticated to a superuser is not necessary or
sufficient to close the hazard I described above. My inclination is to just say
that the hazard is acceptable, and we should not worry about it.

No database owner should be allowed to kill processes like the bgwriter or the
stats collector. Since they do not connect to databases or operate as an
authenticated user, none of the proposed tests would open up ways to kill them.

> 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?

Yes. Modulo concerns I described above, database owners should be allowed to
cancel or terminate any backend connected to their databases, and any user
should be able to cancel or terminate backends authenticated to themselves.