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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)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

On 9/24/14 4:58 PM, Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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.

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 9/24/14 4:58 PM, Stephen Frost wrote:
> > * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 9/24/14 4:58 PM, Stephen Frost wrote:
> > * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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

On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost <sfrost(at)snowman(dot)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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

On Thursday, October 16, 2014, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost <sfrost(at)snowman(dot)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

On Thu, Oct 16, 2014 at 11:28 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> On Thursday, October 16, 2014, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost <sfrost(at)snowman(dot)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

* Robert Haas (robertmhaas(at)gmail(dot)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

>
> 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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

All,

* Peter Eisentraut (peter_e(at)gmx(dot)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

* Stephen Frost (sfrost(at)snowman(dot)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

All,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Stephen Frost (sfrost(at)snowman(dot)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

On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
>> * Stephen Frost (sfrost(at)snowman(dot)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

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Stephen Frost (sfrost(at)snowman(dot)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

* Stephen Frost (sfrost(at)snowman(dot)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

On Tue, Dec 2, 2014 at 2:50 PM, Stephen Frost <sfrost(at)snowman(dot)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

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Stephen Frost (sfrost(at)snowman(dot)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

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

* Andres Freund (andres(at)2ndquadrant(dot)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

On Fri, Dec 5, 2014 at 11:28 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> > * Stephen Frost (sfrost(at)snowman(dot)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

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

Jeevan,

* Jeevan Chalke (jeevan(dot)chalke(at)gmail(dot)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