Re: [PATCH] remove redundant ownership checks

It is a cleanup patch apart from SELinux and security framework.

Now, EnableDisableRule() checks ownership of the relation which
owns the rewrite rule to be enabled/disabled.

But it has the following call path, and this check is already done
in the ATPrepCmd().

ATExecCmd()
-> ATExecEnableDisableRule()
-> EnableDisableRule()

This patch removes redundant permission checks.
No need to check same things twice.

Also see the related discussions:
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01593.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01839.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg01840.php

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

The patch was not attached...

(2009/12/16 15:15), KaiGai Kohei wrote:
> It is a cleanup patch apart from SELinux and security framework.
>
> Now, EnableDisableRule() checks ownership of the relation which
> owns the rewrite rule to be enabled/disabled.
>
> But it has the following call path, and this check is already done
> in the ATPrepCmd().
>
> ATExecCmd()
> -> ATExecEnableDisableRule()
> -> EnableDisableRule()
>
> This patch removes redundant permission checks.
> No need to check same things twice.
>
> Also see the related discussions:
> http://archives.postgresql.org/pgsql-hackers/2009-09/msg01593.php
> http://archives.postgresql.org/pgsql-hackers/2009-09/msg01839.php
> http://archives.postgresql.org/pgsql-hackers/2009-09/msg01840.php
>
> Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> The patch was not attached...

This patch either does too much, or not enough.

I would either leave the Assert() in-place as a double-check (I presume
that's why it was there in the first place, and if that Assert() fails
then our assumption about the permissions check being already done on
the object in question would be wrong, since the check is done against
the passed-in 'rel' and the assert is that 'rel' and 'ruletup->ev_class'
are the same; if they're not, then we might need to do perms checking on
ruletup->ev_class)

Or

Remove the now-unused variable eventRelationOid.

Overall, I agree with removing this check as it's already done by
ATSimplePermissions() and we don't double-check the permissions in the
other things called through ATExecCmd() (though there are cases where
specific commands have to do *additional* checks beyond what
ATSimplePermissions() does.. it might be worth looking into what those
are and thinking about if we should move/consolidate/etc them, or if it
makes sense to leave them where they are).

Thanks,

Stephen

(2009/12/18 6:38), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The patch was not attached...
>
> This patch either does too much, or not enough.
>
> I would either leave the Assert() in-place as a double-check (I presume
> that's why it was there in the first place, and if that Assert() fails
> then our assumption about the permissions check being already done on
> the object in question would be wrong, since the check is done against
> the passed-in 'rel' and the assert is that 'rel' and 'ruletup->ev_class'
> are the same; if they're not, then we might need to do perms checking on
> ruletup->ev_class)
>
> Or
>
> Remove the now-unused variable eventRelationOid.

My preference is the later option, because the pg_rewrite entry to be
checked is fetched using RULERELNAME syscache which takes OID of the
relation and name of the rule.
If this Assert() failed, it implies syscache mechanism has problem,
not only integrity of pg_rewrite system catalog.

The attached patch is an revised one.

Thanks,

> Overall, I agree with removing this check as it's already done by
> ATSimplePermissions() and we don't double-check the permissions in the
> other things called through ATExecCmd() (though there are cases where
> specific commands have to do *additional* checks beyond what
> ATSimplePermissions() does.. it might be worth looking into what those
> are and thinking about if we should move/consolidate/etc them, or if it
> makes sense to leave them where they are).
>
> Thanks,
>
> Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> [ patch to remove EnableDisableRule's permissions check ]

I don't particularly like this patch, mainly because I disagree with
randomly removing permissions checks without any sort of plan about
where they ought to go. There are two principal entry points in
rewriteDefine.c (the other one being DefineQueryRewrite), and currently
they both do permissions checks. There is also a third major function
RenameRewriteRule, currently ifdef'd out because it's not used, which
is commented as "Note that it lacks a permissions check", indicating
that somebody (possibly me, I don't recall for sure) thought it was
surprising that there wasn't such a check there. This is sensible if
you suppose that this file implements rule utility commands that are
more or less directly called by the user, which is in fact the case for
DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
EnableDisableRule. Since that's a globally exposed function, it's quite
possible that there's third-party code calling it and expecting it to do
the appropriate permissions check. (A quick look at Slony, in
particular, would be a good idea here.)

If we're going to start moving these checks around we need a very
well-defined notion of where permissions checks should be made, so that
everyone knows what to expect. I have not seen any plan for that.
Removing one check at a time because it appears to not be necessary
in the code paths you've looked at is not a plan.

regards, tom lane

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> If we're going to start moving these checks around we need a very
> well-defined notion of where permissions checks should be made, so that
> everyone knows what to expect.  I have not seen any plan for that.
> Removing one check at a time because it appears to not be necessary
> in the code paths you've looked at is not a plan.

I'm not completely familiar with the existing code structure here, but
it sort of seems like in general you might want to divide up the
processing of a statement into a parse analysis phase, a permissions
checking phase, and an execution phase. The parse analysis seems to
be mostly separated out into transformXyz() functions, but the
permissions checking is mixed in with the execution. Disentangling
that seems like a job and a half.

...Robert

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > [ patch to remove EnableDisableRule's permissions check ]
>
> I don't particularly like this patch, mainly because I disagree with
> randomly removing permissions checks without any sort of plan about
> where they ought to go.

The goal of this was to increase consistancy with the rest of the code,
in particular, ATPrepCmd checks ownership rights on the table, and
anything which wants to check permissions beyond that has to do it
independently. Do I like that? No, not really.

> There are two principal entry points in
> rewriteDefine.c (the other one being DefineQueryRewrite), and currently
> they both do permissions checks.

DefineQueryRewrite gets called out of tcop/utility.c (either under a
CREATE VIEW or a CREATE RULE). tcop/utility.c expects the functions it
calls to to handle permissions checking (except in the one case it
doesn't call a function- T_IndexStmt). Interestingly, DefineIndex,
while it handles a number of other permissions checks, doesn't do checks
to ensure the caller is the table owner- it expects callers to have done
that (which happens both in tcop/utility.c for CREATE INDEX and in
ATPrepCmd for ALTER TABLE ...).

> There is also a third major function
> RenameRewriteRule, currently ifdef'd out because it's not used, which
> is commented as "Note that it lacks a permissions check", indicating
> that somebody (possibly me, I don't recall for sure) thought it was
> surprising that there wasn't such a check there. This is sensible if
> you suppose that this file implements rule utility commands that are
> more or less directly called by the user, which is in fact the case for
> DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
> EnableDisableRule. Since that's a globally exposed function, it's quite
> possible that there's third-party code calling it and expecting it to do
> the appropriate permissions check. (A quick look at Slony, in
> particular, would be a good idea here.)

Personally I find it suprising that things called from ATExecCmd()
expect "some" permissions checks to have been done already.

> If we're going to start moving these checks around we need a very
> well-defined notion of where permissions checks should be made, so that
> everyone knows what to expect. I have not seen any plan for that.
> Removing one check at a time because it appears to not be necessary
> in the code paths you've looked at is not a plan.

What I suggested previously, though it may be naive, is to do the
permissions checks when you actually have enough information to do them.
At the moment we do a 'simple' check in ATPrepCmd (essentially,
ownership of the relation) and then any more complicated checks have to
be done by the function which actually understands what's going on
enough to know what else needs to be checked (eg:
ATAddForeignKeyConstraint).

As I see it, you've mainly got three steps:

Parsing the command (syntax, basic understanding)
Validation (check for object existance, get object info, etc)
Implementation (do the requested action)

I would put the permissions checking between "Validation" and
"Implementation", ideally at the 'top' of "Implementation". This, imv,
is pretty similar to how we handle DML commands today-
parsing/validation are done first but then the permissions checks aren't
done until we actually go to run the command (of course, this also deals
with prepared queries and the like). Right now what we have are a bunch
of checks scattered around during Validation, as we come across things
we think should be checked (gee, we know the table the user referenced,
let's check if he owns it now).

Of course, this patch isn't doing that because it was intended to make
the existing code consistant, not institute a new policy for how
permissions checking should be done. The big downside about trying to
define a new policy is that it's alot more difficult to get agreement
on, and there are likely to be exceptions brought out that might make
the policy appear to be ill-formed.

Do people think this is a sensible approach? Are there known exceptions
where this won't work or would cause problems? Is it possible to make
that kind of deliniation in general? Should we work up an example patch
which moves some of these checks in that direction?

Thanks for your comments.

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Disentangling that seems like a job and a half.

Indeed it will be, but I think it would be a good thing to actually have
a defined point at which permissions checking is to be done. Trying to
read the code and figure out what permissions you need to perform
certain actions, when some of those checks are done as 'prep work' far
up the tree, isn't fun. Makes validation of the checks we say we do in
the documentation more difficult too. Not to mention that if we want to
allow more granular permission granting for certain operations, it gets
even uglier..

Thanks,

Stephen

On Thu, Dec 17, 2009 at 10:14 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> Disentangling that seems like a job and a half.
>
> Indeed it will be, but I think it would be a good thing to actually have
> a defined point at which permissions checking is to be done.

Completely agree...

...Robert

(2009/12/18 9:19), Tom Lane wrote:
> KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> [ patch to remove EnableDisableRule's permissions check ]
>
> I don't particularly like this patch, mainly because I disagree with
> randomly removing permissions checks without any sort of plan about
> where they ought to go. There are two principal entry points in
> rewriteDefine.c (the other one being DefineQueryRewrite), and currently
> they both do permissions checks. There is also a third major function
> RenameRewriteRule, currently ifdef'd out because it's not used, which
> is commented as "Note that it lacks a permissions check", indicating
> that somebody (possibly me, I don't recall for sure) thought it was
> surprising that there wasn't such a check there. This is sensible if
> you suppose that this file implements rule utility commands that are
> more or less directly called by the user, which is in fact the case for
> DefineQueryRewrite, and it's not obvious why it wouldn't be the case for
> EnableDisableRule. Since that's a globally exposed function, it's quite
> possible that there's third-party code calling it and expecting it to do
> the appropriate permissions check. (A quick look at Slony, in
> particular, would be a good idea here.)

If we consider this permission check is a part of specification in
the EnableDisableRule(), it is a viewpoint/standpoint.

However, if we adopt this standpoint, we should skip ATSimplePermissions()
for ENABLE/DISABLE RULE options in ATPrepCmd(), because ATExecCmd() calls
EnableDisableRule() which applies permission checks according to the
specification.

We don't need to apply same checks twice. It will be enough with either
of them. But I don't think skipping ATSimplePermissions() is a right
direction, because the existing PG model obviously handles rewrite rules
as properties of relation, although it is not stored within pg_class.
So, it is quite natural to check permission to modify properties of
relations in ATPrepCmd().

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> [ patch to remove EnableDisableRule's permissions check ]
>
> I don't particularly like this patch, mainly because I disagree with
> randomly removing permissions checks without any sort of plan about
> where they ought to go.

So where ought they to go?

> If we're going to start moving these checks around we need a very
> well-defined notion of where permissions checks should be made, so that
> everyone knows what to expect.  I have not seen any plan for that.

This seems to me to get right the heart of the matter. When I
submitted my machine-readable explain patch, you critiqued it for
implementing half of an abstraction layer, and it seems to me that our
current permissions-checking logic has precisely the same issue. We
consistently write code that starts by checking permissions and then
moves right along to implementing the action. Those two things need
to be severed. I see two ways to do this. Given a function that (A)
does some prep work, (B) checks permissions, and (C) performs the
action, we could either:

1. Make the existing function do (A) and (B) and then call another
function to do (C), or
2. Make the existing function do (A), call another function to do (B),
and then do (C) itself.

I'm not sure which will work better, but I think making a decision
about which way to do it and how to name the functions would be a big
step towards having a coherent plan for this project.

A related issue is where parse analysis should be performed. We're
not completely consistent about this right now. Most of it seems to
be done by code in the "parser" directory, but there are several
exceptions, including DefineRule().

...Robert

(2009/12/21 12:53), Robert Haas wrote:
> On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> [ patch to remove EnableDisableRule's permissions check ]
>>
>> I don't particularly like this patch, mainly because I disagree with
>> randomly removing permissions checks without any sort of plan about
>> where they ought to go.
>
> So where ought they to go?
>
>> If we're going to start moving these checks around we need a very
>> well-defined notion of where permissions checks should be made, so that
>> everyone knows what to expect. I have not seen any plan for that.
>
> This seems to me to get right the heart of the matter. When I
> submitted my machine-readable explain patch, you critiqued it for
> implementing half of an abstraction layer, and it seems to me that our
> current permissions-checking logic has precisely the same issue. We
> consistently write code that starts by checking permissions and then
> moves right along to implementing the action. Those two things need
> to be severed. I see two ways to do this. Given a function that (A)
> does some prep work, (B) checks permissions, and (C) performs the
> action, we could either:
>
> 1. Make the existing function do (A) and (B) and then call another
> function to do (C), or
> 2. Make the existing function do (A), call another function to do (B),
> and then do (C) itself.
>
> I'm not sure which will work better, but I think making a decision
> about which way to do it and how to name the functions would be a big
> step towards having a coherent plan for this project.

We have mixed policy in the current implementation.

The point is what database object shall be handled in this function.

If we consider a rewrite rule as a database object, not a property of
the relation, it seems to me a correct manner to apply permission checks
in the EnableDisableRule(), because it handles a given rewrite rule.

If we consider a rewrite rule as a property of a relation, not an independent
database object, it seems to me we should apply permission checks in ATPrepCmd()
which handles a relation, rather than EnableDisableRule().

My patch stands on the later perspective, because pg_rewrite entries don't
have its own ownership and access privileges, and it is always owned by
a certain relation.

Thanks,

> A related issue is where parse analysis should be performed. We're
> not completely consistent about this right now. Most of it seems to
> be done by code in the "parser" directory, but there are several
> exceptions, including DefineRule().
>
> ...Robert

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2009/12/22 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2009/12/21 12:53), Robert Haas wrote:
>> On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>  wrote:
>>> KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>  writes:
>>>> [ patch to remove EnableDisableRule's permissions check ]
>>>
>>> I don't particularly like this patch, mainly because I disagree with
>>> randomly removing permissions checks without any sort of plan about
>>> where they ought to go.
>>
>> So where ought they to go?
>>
>>> If we're going to start moving these checks around we need a very
>>> well-defined notion of where permissions checks should be made, so that
>>> everyone knows what to expect.  I have not seen any plan for that.
>>
>> This seems to me to get right the heart of the matter.  When I
>> submitted my machine-readable explain patch, you critiqued it for
>> implementing half of an abstraction layer, and it seems to me that our
>> current permissions-checking logic has precisely the same issue.  We
>> consistently write code that starts by checking permissions and then
>> moves right along to implementing the action.  Those two things need
>> to be severed.  I see two ways to do this.  Given a function that (A)
>> does some prep work, (B) checks permissions, and (C) performs the
>> action, we could either:
>>
>> 1. Make the existing function do (A) and (B) and then call another
>> function to do (C), or
>> 2. Make the existing function do (A), call another function to do (B),
>> and then do (C) itself.
>>
>> I'm not sure which will work better, but I think making a decision
>> about which way to do it and how to name the functions would be a big
>> step towards having a coherent plan for this project.
>
> We have mixed policy in the current implementation.
>
> The point is what database object shall be handled in this function.
>
> If we consider a rewrite rule as a database object, not a property of
> the relation, it seems to me a correct manner to apply permission checks
> in the EnableDisableRule(), because it handles a given rewrite rule.
>
> If we consider a rewrite rule as a property of a relation, not an independent
> database object, it seems to me we should apply permission checks in ATPrepCmd()
> which handles a relation, rather than EnableDisableRule().
>
> My patch stands on the later perspective, because pg_rewrite entries don't
> have its own ownership and access privileges, and it is always owned by
> a certain relation.

That's somewhat separate from the point I was making, but it's a good
point all the same.

...Robert

On Sun, Dec 20, 2009 at 10:53 PM, I wrote:
> On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> [ patch to remove EnableDisableRule's permissions check ]
>>
>> I don't particularly like this patch, mainly because I disagree with
>> randomly removing permissions checks without any sort of plan about
>> where they ought to go.
>
> So where ought they to go?

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

Per Tom's suggestion, I did a quick grep of the Slony source code for
EnableDisableRule and got no hits. I think the appropriate level of
concern about the possibility that third-party code might be calling
this function is to (1) add a comment to the function noting that it
is the caller's responsibility to check permissions, and (2) if we're
*really* concerned that someone might miss that, release-note it. It
is very unclear that it's worth even that much effort, though, since
we have zero evidence that there is any third-party code using this
function directly. A quick Google search on EnableDisableRule hits
mostly this thread.

With respect to cleaning up permissions more generally, it seems to me
that Stephen Frost hit the nail on the upthread when he remarked that
the current coding, where we're expecting certain functions in
tablecmds.c to be called with PART of the permissions checking done,
is fairly counterintuitive. I think it would be interesting to see if
someone could propose a refactoring that eliminates this and puts all
the permissions checking in a single, appropriate location. But ISTM
that this patch would be a subset of any such more comprehensive
patch, so that doesn't seem like a reason to hold off on applying this
either.

So I am inclined to go ahead and apply this with the addition of the
comment discussed above.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I have looked this over a little bit and I guess I don't see why the
> lack of a grand plan for how to organize all of our permissions checks
> ought to keep us from removing this one on the grounds of redundancy.
> We have to attack this problem in small pieces if we're going to make
> any progress, and the pieces aren't going to get any smaller than
> this.

I would turn that argument around: given the lack of a grand plan,
why should we remove this particular check at all? Nobody has argued
that there would be a significant, or even measurable, performance gain.
When and if we do have a plan, we might find ourselves putting this
check back.

Even if you are right in your unsubstantiated hypothesis that this
change will be a subset of any future change that is made with some plan
in mind, I don't see that incremental revisions of the permissions check
placement are a good way to approach the problem. What I fear will
result from that is gaps in permissions checking, depending on what
combination of revisions of core and third-party code happen to get used
in a given installation.

I think we need a plan first, not random patches first.

regards, tom lane

On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I have looked this over a little bit and I guess I don't see why the
>> lack of a grand plan for how to organize all of our permissions checks
>> ought to keep us from removing this one on the grounds of redundancy.
>> We have to attack this problem in small pieces if we're going to make
>> any progress, and the pieces aren't going to get any smaller than
>> this.
>
> I would turn that argument around: given the lack of a grand plan,
> why should we remove this particular check at all? Nobody has argued
> that there would be a significant, or even measurable, performance gain.
> When and if we do have a plan, we might find ourselves putting this
> check back.

You're arguing against a straw man - there's clearly no argument here
from performance. We generally do not choose to litter the code with
redundant or irrelevant checks because it makes the code difficult to
maintain and understand. Sometimes it also hurts performance, but
that's not a necessary criterion for removal. Nor are we generally in
the habit of keeping redundant code around because a hypothetical
future refactoring might by chance end up putting exactly the same
code back.

> Even if you are right in your unsubstantiated hypothesis that this
> change will be a subset of any future change that is made with some plan
> in mind, I don't see that incremental revisions of the permissions check
> placement are a good way to approach the problem.  What I fear will
> result from that is gaps in permissions checking, depending on what
> combination of revisions of core and third-party code happen to get used
> in a given installation.
>
> I think we need a plan first, not random patches first.

I think the issue at hand is completely severable from the issue of a
more general plan. Again, when we find that we find that the code
does the same work twice, that's typically something we would want to
fix, independent of what else might or might not come later. That
having been said, I'm 100% in favor of talking about what a more
general plan should look like; in fact, I asked for your opinion here:

http://archives.postgresql.org/pgsql-hackers/2009-12/msg01824.php

...Robert

Robert Haas wrote:
> On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> I have looked this over a little bit and I guess I don't see why the
> >> lack of a grand plan for how to organize all of our permissions checks
> >> ought to keep us from removing this one on the grounds of redundancy.
> >> We have to attack this problem in small pieces if we're going to make
> >> any progress, and the pieces aren't going to get any smaller than
> >> this.
> >
> > I would turn that argument around: given the lack of a grand plan,
> > why should we remove this particular check at all? Nobody has argued
> > that there would be a significant, or even measurable, performance gain.
> > When and if we do have a plan, we might find ourselves putting this
> > check back.
>
> You're arguing against a straw man - there's clearly no argument here
> from performance. We generally do not choose to litter the code with
> redundant or irrelevant checks because it makes the code difficult to
> maintain and understand. Sometimes it also hurts performance, but
> that's not a necessary criterion for removal. Nor are we generally in
> the habit of keeping redundant code around because a hypothetical
> future refactoring might by chance end up putting exactly the same
> code back.

I agree. Why are arbitrary restrictions being placed on code
improvements? If code has no purpose, why not remove it, or at least
mark it as NOT_USED.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

(2010/01/12 10:27), Bruce Momjian wrote:
> Robert Haas wrote:
>> On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>>> I have looked this over a little bit and I guess I don't see why the
>>>> lack of a grand plan for how to organize all of our permissions checks
>>>> ought to keep us from removing this one on the grounds of redundancy.
>>>> We have to attack this problem in small pieces if we're going to make
>>>> any progress, and the pieces aren't going to get any smaller than
>>>> this.
>>>
>>> I would turn that argument around: given the lack of a grand plan,
>>> why should we remove this particular check at all? Nobody has argued
>>> that there would be a significant, or even measurable, performance gain.
>>> When and if we do have a plan, we might find ourselves putting this
>>> check back.
>>
>> You're arguing against a straw man - there's clearly no argument here
>> from performance. We generally do not choose to litter the code with
>> redundant or irrelevant checks because it makes the code difficult to
>> maintain and understand. Sometimes it also hurts performance, but
>> that's not a necessary criterion for removal. Nor are we generally in
>> the habit of keeping redundant code around because a hypothetical
>> future refactoring might by chance end up putting exactly the same
>> code back.
>
> I agree. Why are arbitrary restrictions being placed on code
> improvements? If code has no purpose, why not remove it, or at least
> mark it as NOT_USED.
>

The attached patch adds a source code comment which informs developers
that its own permission check had gone at the v8.5 release.

I also think we don't need to note it on the release-note. If we would
describe all the specification changes in external functions, is it
really valuable as a summary? It seems to me too details.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> >> I have looked this over a little bit and I guess I don't see why the
>> >> lack of a grand plan for how to organize all of our permissions checks
>> >> ought to keep us from removing this one on the grounds of redundancy.
>> >> We have to attack this problem in small pieces if we're going to make
>> >> any progress, and the pieces aren't going to get any smaller than
>> >> this.
>> >
>> > I would turn that argument around: given the lack of a grand plan,
>> > why should we remove this particular check at all?  Nobody has argued
>> > that there would be a significant, or even measurable, performance gain.
>> > When and if we do have a plan, we might find ourselves putting this
>> > check back.
>>
>> You're arguing against a straw man - there's clearly no argument here
>> from performance.  We generally do not choose to litter the code with
>> redundant or irrelevant checks because it makes the code difficult to
>> maintain and understand.  Sometimes it also hurts performance, but
>> that's not a necessary criterion for removal.  Nor are we generally in
>> the habit of keeping redundant code around because a hypothetical
>> future refactoring might by chance end up putting exactly the same
>> code back.
>
> I agree.  Why are arbitrary restrictions being placed on code
> improvements?  If code has no purpose, why not remove it, or at least
> mark it as NOT_USED.

So, where do we go from here? Any other opinions? I'm not sure how
much it's really worth fighting over a six line patch, but there's
something in me that rails against the idea of telling someone who
took the trouble to write a patch "no" when the only argument against
it is that we might change our mind at some point in the future. Of
course, I will accept the consensus of the community whatever it is,
but the only people who have expressed a clear opinion on this version
of the patch are Tom, Bruce, and myself, and 2-1 is not a consensus.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> I agree. Why are arbitrary restrictions being placed on code
>> improvements? If code has no purpose, why not remove it, or at least
>> mark it as NOT_USED.

> So, where do we go from here? Any other opinions? I'm not sure how
> much it's really worth fighting over a six line patch, but there's
> something in me that rails against the idea of telling someone who
> took the trouble to write a patch "no" when the only argument against
> it is that we might change our mind at some point in the future. Of
> course, I will accept the consensus of the community whatever it is,
> but the only people who have expressed a clear opinion on this version
> of the patch are Tom, Bruce, and myself, and 2-1 is not a consensus.

I still say that this patch is putting the cart before the horse.
What we need before we do any significant amount of rearrangement
of security checks is to have a plan for where they should go.
Making a change here and a change there without a plan isn't an
improvement, it just risks creating bugs.

If I thought this patch represented incremental movement in the
direction of a better security-check factorization, I'd be fine with it,
but that's not clear either. The argument for it is that these checks
are redundant with some other ones, but why should we remove these and
not the other ones instead? And there would still be still some checks
left in this file, so it doesn't seem to me that we'd have actually made
any progress towards clarity of where the checks should be.

Plan first, then code, please.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> So, where do we go from here? Any other opinions?

It seems that we often have people cleaning up redundant or
unreachable code to improve code clarity. I'm not in a position
right now to confirm that this code is redundant, but if that has
been firmly established, I haven't heard any good reason for not
fixing it.

-Kevin

On Wed, Jan 13, 2010 at 1:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> So, where do we go from here?  Any other opinions?

if it's not broken, then it doesn't need a fix...
if that code is causing a hole in security then i said remove it, if
not... what's the problem in let it be until we know exactly what the
plan is?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

* Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
> if it's not broken, then it doesn't need a fix...
> if that code is causing a hole in security then i said remove it, if
> not... what's the problem in let it be until we know exactly what the
> plan is?

The chances of getting concensus on an overarching big plan are slim
to none, which essentially means it's not going to get changed. I've
suggested an approach and had no feedback on it. What's probably
needed, to get attention anyway, is a patch which starts to move us in
the right direction. That's going to be more than a 6-line patch, but
doesn't have to be the whole thing either.

For my 2c, I think a patch which attacks 'AT_PrepCmd' and rips out all
of the owner checks from it and moves them to appropriate places (that
is to say, per my proposal, very shortly before the 'work' is actually
done, which is also often where the *other* permission checks are done,
for those pieces which require more than just a simple owner check)
would probably be the first step.

Of course, the code between AT_PrepCmd and where the permissions checks
are moved to would need to be reviewed and vetted to make sure there
isn't anything being done that shouldn't be done without permission,
but, honestly, I don't recall seeing much of that. We're actually
pretty good about having a "gather info" stage followed by a "implement
change" stage.

Thanks,

Stephen

On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>> I agree.  Why are arbitrary restrictions being placed on code
>>> improvements?  If code has no purpose, why not remove it, or at least
>>> mark it as NOT_USED.
>
>> So, where do we go from here?  Any other opinions?  I'm not sure how
>> much it's really worth fighting over a six line patch, but there's
>> something in me that rails against the idea of telling someone who
>> took the trouble to write a patch "no" when the only argument against
>> it is that we might change our mind at some point in the future.  Of
>> course, I will accept the consensus of the community whatever it is,
>> but the only people who have expressed a clear opinion on this version
>> of the patch are Tom, Bruce, and myself, and 2-1 is not a consensus.
>
> I still say that this patch is putting the cart before the horse.
> What we need before we do any significant amount of rearrangement
> of security checks is to have a plan for where they should go.
> Making a change here and a change there without a plan isn't an
> improvement, it just risks creating bugs.
>
> If I thought this patch represented incremental movement in the
> direction of a better security-check factorization, I'd be fine with it,
> but that's not clear either.  The argument for it is that these checks
> are redundant with some other ones, but why should we remove these and
> not the other ones instead?

That's a good question, and I have an answer. Most of the ALTER TABLE
operations use ATSimplePermissions() or
ATSimplePermissionsRelationOrIndex() to check permissions. The
exceptions are AT_SetDistinct (and I'm wondering if we shouldn't
remove that exception, see my recent message on attoptions) and
AT_ChangeOwner (see comments for why). ATSimplePermissions() checks
(1) that we are operating on the right sort of relkind, (2) that the
current user owns the object, and (3) that we are operating on a
system catalog only if allowSystemTableMods. When we are enabling or
disabling a rule (but not in other cases), we then recheck only the
second of those conditions. Removing the "other check" (the call to
ATSimplePermissions) would require copying the other two bits of logic
into EnableDisableRule - so we'd have duplicate code floating around,
and the EnableDisableRule operations would have bespoke code instead
of sharing the common mechanism used by the remaining ALTER TABLE
commands.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If I thought this patch represented incremental movement in the
>> direction of a better security-check factorization, I'd be fine with it,
>> but that's not clear either. The argument for it is that these checks
>> are redundant with some other ones, but why should we remove these and
>> not the other ones instead?

> That's a good question, and I have an answer [ namely that ALTER TABLE
> is the right place ].

But note Stephen Frost's concurrent reply suggesting that he wants to
move the checks *out* of ALTER TABLE. With his plan, these checks
are probably in the right place already.

I'm a little worried by Stephen's plan, mainly because I'm concerned
that it would lead to ALTER TABLE taking exclusive lock on a table long
before it gets around to checking permissions. Still, that's just
extending a window that exists now.

Anyway, this is the sort of thing that should be hashed out before
starting to write code. Adding or removing security checks is not
the hard part, the hard part is deciding where they should be.

regards, tom lane

On Wed, Jan 13, 2010 at 12:54, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm a little worried by Stephen's plan, mainly because I'm concerned
> that it would lead to ALTER TABLE taking exclusive lock on a table long
> before it gets around to checking permissions.  Still, that's just
> extending a window that exists now.

Im of the opinion if we are going to be meddling with the permission
checks in this area one of the goals should be close or at least
tighten up that window. So you cant lock a table you dont have
permission to (either via LOCK or ALTER TABLE). (Ignoring the issues
of concurrent permission changes of course...)

On Wed, Jan 13, 2010 at 13:46, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> Im of the opinion if we are going to be meddling with the permission
> checks [...]

Specifically:
http://archives.postgresql.org/pgsql-hackers/2009-05/msg00566.php

Sounds like most solutions would put us back to exactly what you were
trying to fix. :( Maybe its a moot point. But I figured while we are
talking about ALTER permissions....

* Alex Hunsaker (badalex(at)gmail(dot)com) wrote:
> On Wed, Jan 13, 2010 at 12:54, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I'm a little worried by Stephen's plan, mainly because I'm concerned
> > that it would lead to ALTER TABLE taking exclusive lock on a table long
> > before it gets around to checking permissions.  Still, that's just
> > extending a window that exists now.
>
> Im of the opinion if we are going to be meddling with the permission
> checks in this area one of the goals should be close or at least
> tighten up that window. So you cant lock a table you dont have
> permission to (either via LOCK or ALTER TABLE). (Ignoring the issues
> of concurrent permission changes of course...)

Trying to minimize that makes the permissions checking a royal mess by
making it have to happen all over the place, after every little bit of
information is gathered. I'm not a fan of that because of both concerns
about making sure it's correct and actually matches our documention, as
well as any possibility of making it a pluggable framework. At the
moment, we're doing permissions checks on the main table before we even
know if the other tables referenced in the command exist. I don't think
we're talking about a serious difference in time here either, to be
honest.

Not to mention that if you don't have access to the schema, you wouldn't
be able to take a lock on the table at all, so I'm really not sure how
big a deal this is..

Thanks,

Stephen

* Alex Hunsaker (badalex(at)gmail(dot)com) wrote:
> On Wed, Jan 13, 2010 at 13:46, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> > Im of the opinion if we are going to be meddling with the permission
> > checks [...]
>
> Specifically:
> http://archives.postgresql.org/pgsql-hackers/2009-05/msg00566.php
>
> Sounds like most solutions would put us back to exactly what you were
> trying to fix. :( Maybe its a moot point. But I figured while we are
> talking about ALTER permissions....

Maybe I missed it, but I don't see anything that said ALTER TABLE was
changed or fixed to address this concern. It might make the timing
increase some, and it'd be interesting in any case to see just what the
timing change looked like, but I don't know that it's really all that
important.. At least if the timing didn't change much we could claim
that this didn't affect this use-case, but I don't believe it shouldn't
be done if it does.

I don't see a way to actually *fix* the problem of not allowing someone
who doesn't have all the right permissions to not lock the table at
all. Taking a share lock and then trying to upgrade it isn't a good
idea either.

Thanks,

Stephen

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> Im of the opinion if we are going to be meddling with the permission
> checks in this area one of the goals should be close or at least
> tighten up that window. So you cant lock a table you dont have
> permission to (either via LOCK or ALTER TABLE). (Ignoring the issues
> of concurrent permission changes of course...)

Well, that's exactly the problem: it's not very sane to do permissions
checking on a table you have no lock whatsoever on, because the table
could be dropped, renamed, or have its permissions altered underneath
you. We could imagine taking a weak lock that forbids those operations
and then upgrading once we're sure we have the right to take a stronger
lock, but lock upgrade is a certain ticket to deadlocks.

So yeah, it'd be nice, but it's not apparent how to do it. The best
thing I can see how to do is keep the window between taking the lock
and verifying permissions narrow.

regards, tom lane

(2010/01/14 4:54), Tom Lane wrote:
> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> If I thought this patch represented incremental movement in the
>>> direction of a better security-check factorization, I'd be fine with it,
>>> but that's not clear either. �The argument for it is that these checks
>>> are redundant with some other ones, but why should we remove these and
>>> not the other ones instead?
>
>> That's a good question, and I have an answer [ namely that ALTER TABLE
>> is the right place ].
>
> But note Stephen Frost's concurrent reply suggesting that he wants to
> move the checks *out* of ALTER TABLE. With his plan, these checks
> are probably in the right place already.

Note that this patch tries to remove redundant checks in this code path.
If ATPrepCmd() would not be a right place to apply permission checks,
we should remove invocation of the ATSimplePermissions() for AT_EnableRule
and so on. (Of course, we need to copy two other sanity check in the
ATSimplePermissions() also)

However, in my opinion, ATPrepCmd() is more appropriate to apply permission
checks than EnableDisableRule(), because we deal with rewrite rule (that
does not have individual ownership and acls) as properties of a relation,
not an independent database object, although it is stored in its own
system catalog. It is quite natural to check privileges to alter properties
of a relaion in tablecmd.c, rather than rewriteDefine.c.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

(2010/01/14 4:27), Stephen Frost wrote:
> * Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
>> if it's not broken, then it doesn't need a fix...
>> if that code is causing a hole in security then i said remove it, if
>> not... what's the problem in let it be until we know exactly what the
>> plan is?
>
> The chances of getting concensus on an overarching big plan are slim
> to none, which essentially means it's not going to get changed. I've
> suggested an approach and had no feedback on it. What's probably
> needed, to get attention anyway, is a patch which starts to move us in
> the right direction. That's going to be more than a 6-line patch, but
> doesn't have to be the whole thing either.
>
> For my 2c, I think a patch which attacks 'AT_PrepCmd' and rips out all
> of the owner checks from it and moves them to appropriate places (that
> is to say, per my proposal, very shortly before the 'work' is actually
> done, which is also often where the *other* permission checks are done,
> for those pieces which require more than just a simple owner check)
> would probably be the first step.
>
> Of course, the code between AT_PrepCmd and where the permissions checks
> are moved to would need to be reviewed and vetted to make sure there
> isn't anything being done that shouldn't be done without permission,
> but, honestly, I don't recall seeing much of that. We're actually
> pretty good about having a "gather info" stage followed by a "implement
> change" stage.

Some of ALTER TABLE operations take multiple permission checks, not only
ownership of the relation to be altered.
For example, ALTER TABLE with SET TABLESPACE option also need ACL_CREATE
permission on the new tablespace, not only ownership of the relation.
It means we need to gather two information before whole of the permission
checks. (1) OID of the relation to be altered. (2) OID of the tablespace
to be set.

In my understanding, Stephen suggests that we should, ideally, rip out
permission logic from the code closely-combined with the steps of
implementation. Of course, it does not mean all the checks should be
moved just before simple_heap_update().

However, it is a separate topic for this patch.
This patch just tries to remove redundant checks, and integrate them
into a common place where most of ALTER TABLE option applies its
permission checks on.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> (2010/01/14 4:54), Tom Lane wrote:
>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> If I thought this patch represented incremental movement in the
>>>> direction of a better security-check factorization, I'd be fine
>>>> with it,
>>>> but that's not clear either. �The argument for it is that these checks
>>>> are redundant with some other ones, but why should we remove these and
>>>> not the other ones instead?
>>> That's a good question, and I have an answer [ namely that ALTER TABLE
>>> is the right place ].
>> But note Stephen Frost's concurrent reply suggesting that he wants to
>> move the checks *out* of ALTER TABLE. With his plan, these checks
>> are probably in the right place already.
>
> Note that this patch tries to remove redundant checks in this code path.
> If ATPrepCmd() would not be a right place to apply permission checks
> we should remove invocation of the ATSimplePermissions()...

I'm glad to see this discussion thread, because I think it's really
getting to the heart of the core issues that have made development in
this area (like SEPostgreSQL) more complicated than it needs to be. I
just talked with Stephen for a bit tonight to try and get my head
wrapped around what you're all trying to do, and from what I gather the
plan here that's taking shape looks like this:

1) Reach an agreement of the preferred way to handle permissions checks
in these situations where there is more than one such check going on,
and therefore a direction to consolidate all of them toward
2) Update the messy ALTER TABLE code to use that preferred method
3) Find another messy chunk of code and confirm the same style of
refactoring can be applied to it as well (Stephen suggested ALTER TYPE
as a candidate here)
4) Finish up converting any other ALTER commands that have split checks
in them, since ALTER seems to be operation that's most prone to this
type of issue
5) Confirm that the permissions checks in the major operations with
simpler permissions related code paths (CREATE etc.) are also free of
split checks
6) Add an extended alternate permissions checker to the now consolidated
code, such as considering security labels. If the previous steps were
done right, this should have a smaller logical and diff footprint than
previous such efforts because it will touch many less places.

Tom's objection to this patch is that without at least a general idea
what form (2) through (4) (or similar incremental steps) intend to take,
you don't want to touch just (2) lest you do something that only make
sense for it--but not the later work. The patch being discussed here is
a first step of the work needed for (2). However, it seems pretty clear
to me that there's not even close to an agreement about step (1) here
yet. Let me quote a few bits out of context to highlight:

KaiGai: "...it is quite natural to check permission to modify
properties of relations in ATPrepCmd"

Robert: "Most of the ALTER TABLE operations use ATSimplePermissions()
or ATSimplePermissionsRelationOrIndex() to check permissions...what I
have in mind is to modify ATPrepCmd...".

Stephen: "I think a patch which attacks ATPrepCmd and rips out all of
the owner checks from it and moves them to appropriate places...would
probably be the first step...At the moment we do a 'simple' check in
ATPrepCmd (essentially, ownership of the relation) and then any more
complicated checks have to be done by the function...this patch isn't
doing that because it was intended to make
the existing code consistant, not institute a new policy for how
permissions checking should be done."

(Apologies if a fragment or two of the above aren't in the archives, I
think I grabbed a bit from one of the off-list messages in my mailbox
while assembling).

I've looked at this for a little bit, and I sure can't tell who's right
here. What I am sure of though is that even a majority here isn't going
to fly. If we don't even have all three of you guys lined up in the
same direction on something this small, there's little hope of getting
the whole community sold on this already controversial issue. Tom said
back on 12/17 that "we need a very well-defined notion of where
permissions checks should be made", the thing I pulled out as (1)
above. The discussion around that topic has been going on here quite
regularly now for almost a month, and these little quoted bits highlight
that opinion is still quite split. Please keep hammering away at this
little piece; I think it's really important to set a good example here.

But the preference of the last CF is to not apply any patch which
doesn't have a very clear justification to be committed. Given that
whether this patch is applied or not to 8.5 really doesn't make any
functional difference, I don't see anywhere for this to go right now
except for "Returned with Feedback". It's extremely valuable to have
had this patch submitted. I don't believe an exact spot of contention
with the current code was ever highlighted so clearly before this
discussion, because previous patches were just too big. We do need to
get this whole thing off the list for a while now though, I think it's
gotten quite a fair slice of discussion already.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

(2010/01/14 15:04), Greg Smith wrote:
> KaiGai Kohei wrote:
>> (2010/01/14 4:54), Tom Lane wrote:
>>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>>> On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>> If I thought this patch represented incremental movement in the
>>>>> direction of a better security-check factorization, I'd be fine
>>>>> with it,
>>>>> but that's not clear either. �The argument for it is that these checks
>>>>> are redundant with some other ones, but why should we remove these and
>>>>> not the other ones instead?
>>>> That's a good question, and I have an answer [ namely that ALTER TABLE
>>>> is the right place ].
>>> But note Stephen Frost's concurrent reply suggesting that he wants to
>>> move the checks *out* of ALTER TABLE. With his plan, these checks
>>> are probably in the right place already.
>>
>> Note that this patch tries to remove redundant checks in this code path.
>> If ATPrepCmd() would not be a right place to apply permission checks
>> we should remove invocation of the ATSimplePermissions()...
>
> I'm glad to see this discussion thread, because I think it's really
> getting to the heart of the core issues that have made development in
> this area (like SEPostgreSQL) more complicated than it needs to be. I
> just talked with Stephen for a bit tonight to try and get my head
> wrapped around what you're all trying to do, and from what I gather the
> plan here that's taking shape looks like this:
>
> 1) Reach an agreement of the preferred way to handle permissions checks
> in these situations where there is more than one such check going on,
> and therefore a direction to consolidate all of them toward
> 2) Update the messy ALTER TABLE code to use that preferred method
> 3) Find another messy chunk of code and confirm the same style of
> refactoring can be applied to it as well (Stephen suggested ALTER TYPE
> as a candidate here)
> 4) Finish up converting any other ALTER commands that have split checks
> in them, since ALTER seems to be operation that's most prone to this
> type of issue
> 5) Confirm that the permissions checks in the major operations with
> simpler permissions related code paths (CREATE etc.) are also free of
> split checks
> 6) Add an extended alternate permissions checker to the now consolidated
> code, such as considering security labels. If the previous steps were
> done right, this should have a smaller logical and diff footprint than
> previous such efforts because it will touch many less places.
>
> Tom's objection to this patch is that without at least a general idea
> what form (2) through (4) (or similar incremental steps) intend to take,
> you don't want to touch just (2) lest you do something that only make
> sense for it--but not the later work. The patch being discussed here is
> a first step of the work needed for (2). However, it seems pretty clear
> to me that there's not even close to an agreement about step (1) here
> yet. Let me quote a few bits out of context to highlight:
>
> KaiGai: "...it is quite natural to check permission to modify properties
> of relations in ATPrepCmd"
>
> Robert: "Most of the ALTER TABLE operations use ATSimplePermissions() or
> ATSimplePermissionsRelationOrIndex() to check permissions...what I have
> in mind is to modify ATPrepCmd...".
>
> Stephen: "I think a patch which attacks ATPrepCmd and rips out all of
> the owner checks from it and moves them to appropriate places...would
> probably be the first step...At the moment we do a 'simple' check in
> ATPrepCmd (essentially, ownership of the relation) and then any more
> complicated checks have to be done by the function...this patch isn't
> doing that because it was intended to make
> the existing code consistant, not institute a new policy for how
> permissions checking should be done."
>
> (Apologies if a fragment or two of the above aren't in the archives, I
> think I grabbed a bit from one of the off-list messages in my mailbox
> while assembling).
>
> I've looked at this for a little bit, and I sure can't tell who's right
> here. What I am sure of though is that even a majority here isn't going
> to fly. If we don't even have all three of you guys lined up in the same
> direction on something this small, there's little hope of getting the
> whole community sold on this already controversial issue. Tom said back
> on 12/17 that "we need a very well-defined notion of where permissions
> checks should be made", the thing I pulled out as (1) above. The
> discussion around that topic has been going on here quite regularly now
> for almost a month, and these little quoted bits highlight that opinion
> is still quite split. Please keep hammering away at this little piece; I
> think it's really important to set a good example here.

Greg, thanks for this summary.

I'd like to introduce two points prior to the stage of (1).

First, we need to make clear what is the functionality of access control
features. It is making an access control decision either "allowed" or
"denied" on a certain combination of database role (subject), database
object and required action, based on the rules.

For example, when a database user X tries to alter a certain table T,
access control feature has to make its decision on the combination of:
- subject: user X
- object: table T
- action: ALTER TABLE

The default PG privilege has a rule that checks ownership of the relation
to be altered, and a few variations depending on the alter table option.

The point is that it is defined on the combination of S, O and A.

Even if some of ALTER TABLE option needs a few additional checks
(such as ACL_CREATE on the new tablespace with SET TABLESPACE option),
we should not forget the principal target is table T and the operation
to be controlled is ALTER TABLE.

In other word, what I want to make clear is that the permission checks
on ALTER TABLE tries to control an operation to the specified table object.

Second, we need to make clear what is the database object to be controlled.

Needless to say, a table is a database object. It has ownership and
access privileges. We can define a combination of S, O and A.

Rewrite rules are not individual database objects, at least, from the
perspective of access controls. It does not have its ownership and
access privileges. We consider it is a property of a certain relation,
although it is stored within pg_rewrite system catalog.
In fact, EnableDisableRule() checks ownership of the relation owning
the rewrite rule to be turned on/off.

Trigger is a similar case. It does not have its ownership and access
privileges. We always checks ownership of the relation owning the
trigger, when ALTER TABLE with ENABLE/DISABLE TRIGGER option and
ALTER TRIGGER. ACL_TRIGGER is also a privilege of relation.

So, the reason why my preference is to put this check in tablecmd.c,
rather than rewriteDefine.c, is that the target object to be altered
with this operation (ALTER TABLE) is a certain table.
The tablecmd.c is a right place to handle operations to table objects,
so I wrote "it is quite natural".

The S(subject) is obvious in the default PG privileges.
The points to be emphasized is to make clear A(action) and O(object)
at first.

Thanks,

> But the preference of the last CF is to not apply any patch which
> doesn't have a very clear justification to be committed. Given that
> whether this patch is applied or not to 8.5 really doesn't make any
> functional difference, I don't see anywhere for this to go right now
> except for "Returned with Feedback". It's extremely valuable to have had
> this patch submitted. I don't believe an exact spot of contention with
> the current code was ever highlighted so clearly before this discussion,
> because previous patches were just too big. We do need to get this whole
> thing off the list for a while now though, I think it's gotten quite a
> fair slice of discussion already.
>
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Some of ALTER TABLE operations take multiple permission checks, not only
> ownership of the relation to be altered.

Yes, exactly my point. Those places typically just presume that the
owner check has already been done.

> For example, ALTER TABLE with SET TABLESPACE option also need ACL_CREATE
> permission on the new tablespace, not only ownership of the relation.
> It means we need to gather two information before whole of the permission
> checks. (1) OID of the relation to be altered. (2) OID of the tablespace
> to be set.

Right. I would say that we should wait until we have all the necessary
information to do the permissions checking, and then do it all at that
point, similar to how we handle DML today.

> In my understanding, Stephen suggests that we should, ideally, rip out
> permission logic from the code closely-combined with the steps of
> implementation.

This might be a confusion due to language, but I think of the
"implementation" as being "the work". The check on permissions on the
tablespace that you're describing above is done right before "the work".
For this case, specifically, ATPrepSetTableSpace() takes the action on
line 6754: tab->newTableSpace = tablespaceId;
Prior to that, it checks the tablespace permissions (but not the table
permissions, since they've been checked already). I would suggest we
add a call to ATSimplePermissions() in ATPrepSetTableSpace at line 6745-
right after the /* Check its permissions */ comment (which would be
changed to "check permissions for ALTER TABLE x SET TABLESPACE y").

> Of course, it does not mean all the checks should be
> moved just before simple_heap_update().

No, I would have it earlier than simple_heap_update(), we don't need to
go building the structures and whatnot needed to call
simple_heap_update(). For this specific case though, I'm a bit torn by
the fact that the work associated with changing the tablespace can
actually happen in two distinct places- either through
ATExecSetTableSpace, or in ATRewriteTables directly.
ATExecSetTableSpace would actually be a good candidate rather than in
the 'prep' stage, if all tablespace changes were done there. The 'prep'
stage worries me a bit since I'm not sure if all permissions checking
is currently, or coulde be, done at that point, and I'd prefer that we
use the same approach for permissions checking throughout the code- for
example, it's either done in 'phase 3' (where we're going through the
subcommands) or all done in 'phase 1/2', where we're setting things up.

> However, it is a separate topic for this patch.
> This patch just tries to remove redundant checks, and integrate them
> into a common place where most of ALTER TABLE option applies its
> permission checks on.

I don't believe we can make the case that it's a distinct topic based on
the feedback received.

Thanks,

Stephen

On Thu, Jan 14, 2010 at 1:04 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> But the preference of the last CF is to not apply any patch which doesn't
> have a very clear justification to be committed.  Given that whether this
> patch is applied or not to 8.5 really doesn't make any functional
> difference, I don't see anywhere for this to go right now except for
> "Returned with Feedback".  It's extremely valuable to have had this patch
> submitted.  I don't believe an exact spot of contention with the current
> code was ever highlighted so clearly before this discussion, because
> previous patches were just too big.  We do need to get this whole thing off
> the list for a while now though, I think it's gotten quite a fair slice of
> discussion already.

While I understand your desire to get this patch closed out and move
on to other things, I don't agree that we've given any meaningful
feedback as yet. We really haven't made any progress getting an
agreement on where or how the permissions checks should be done. I
would be perfectly happy to throw this patch under the bus in exchange
for some meaningful feedback on what a more comprehensive approach
would look like, but so far none has been forthcoming - and not
because it hasn't been requested.

http://archives.postgresql.org/pgsql-hackers/2009-12/msg01824.php

It is my view that no patch which makes substantial changes to the
security checks in the code is likely to get committed without Tom's
approval. If Tom is not willing to provide input on a comprehensive
plan, and is also not willing to accept even the least-consequential
change without a fully-fleshed-out comprehensive plan, then I think we
are at an impasse. Note that I am NOT saying it is Tom's
RESPONSIBILITY to provide input on a comprehensive plan. I am only
expressing my opinion that no progress can be made otherwise.

...Robert

(2010/01/14 23:29), Stephen Frost wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Some of ALTER TABLE operations take multiple permission checks, not only
>> ownership of the relation to be altered.
>
> Yes, exactly my point. Those places typically just presume that the
> owner check has already been done.
>
>> For example, ALTER TABLE with SET TABLESPACE option also need ACL_CREATE
>> permission on the new tablespace, not only ownership of the relation.
>> It means we need to gather two information before whole of the permission
>> checks. (1) OID of the relation to be altered. (2) OID of the tablespace
>> to be set.
>
> Right. I would say that we should wait until we have all the necessary
> information to do the permissions checking, and then do it all at that
> point, similar to how we handle DML today.
>
>> In my understanding, Stephen suggests that we should, ideally, rip out
>> permission logic from the code closely-combined with the steps of
>> implementation.
>
> This might be a confusion due to language, but I think of the
> "implementation" as being "the work". The check on permissions on the
> tablespace that you're describing above is done right before "the work".
> For this case, specifically, ATPrepSetTableSpace() takes the action on
> line 6754: tab->newTableSpace = tablespaceId;
> Prior to that, it checks the tablespace permissions (but not the table
> permissions, since they've been checked already). I would suggest we
> add a call to ATSimplePermissions() in ATPrepSetTableSpace at line 6745-
> right after the /* Check its permissions */ comment (which would be
> changed to "check permissions for ALTER TABLE x SET TABLESPACE y").
>
>> Of course, it does not mean all the checks should be
>> moved just before simple_heap_update().
>
> No, I would have it earlier than simple_heap_update(), we don't need to
> go building the structures and whatnot needed to call
> simple_heap_update().

Sorry for this confusion. I used the "just before simple_heap_update()"
as a metaphor to mean "much deep stage in execution phase".
It does not mean we should put security checks after the catalog updates.

> For this specific case though, I'm a bit torn by
> the fact that the work associated with changing the tablespace can
> actually happen in two distinct places- either through
> ATExecSetTableSpace, or in ATRewriteTables directly.
> ATExecSetTableSpace would actually be a good candidate rather than in
> the 'prep' stage, if all tablespace changes were done there. The 'prep'
> stage worries me a bit since I'm not sure if all permissions checking
> is currently, or coulde be, done at that point, and I'd prefer that we
> use the same approach for permissions checking throughout the code- for
> example, it's either done in 'phase 3' (where we're going through the
> subcommands) or all done in 'phase 1/2', where we're setting things up.

It seems to me it is highly suggestive idea, and we should not ignore it.

Currently, ATPrepCmd() applies permission checks and set up recursion
for inherited tables, if necessary.

The following commands have its own variations:

* AT_AddColumn, AT_AddColumnToView, AT_AddOids
It eventually calls ATPrepAddColumn(), because ColumnDef->inhcount of
the child relation should be 1, not 0.

* AT_SetStatistics
ATPrepSetStatistics() does same job with ATSimplePermissionsRelationOrIndex()
except for it allows to alter system relation.

* AT_AddIndex
It check table's permission here, then, it eventually checks permission
to create a new index later.
The point is that whether the index is an individual object class, or
a property of the table. In fact, it has its own ownership, but it is
a copy from the relation to be indexed. Its namespace is also a copy.
In other word, it is equivalent to check properties of the relation.
IMO, we should move all the permission checks in DefineIndex() to the
caller side. In ALTER TABLE case, ATExecAddIndex() is a candidate.
(It is also reason why DefineIndex() takes 'check_right' argument.)

* AT_AlterColumnType
It calls ATPrepAlterColumnType() to check it is available, or not.

* AT_ChangeOwner
It does all the task in ATExecChangeOwner(), and it check permission
only when ownership is actually changed.

* AT_DropOids
It recursively calls ATPrepCmd() with pseudo AT_DropColumn with "oid".

* AT_SetTableSpace
ATPrepSetTableSpace() checks permission on tablespace, in addition to
the ownership of the relation checked in ATSimplePermissionsRelationOrIndex().

And, note that some of AT_* command already checks table's permission due to
the recursion of inheritance tree.

Example)
static void
ATExecDropColumn(List **wqueue, Relation rel, const char *colName, ...)
{
:
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
ATSimplePermissions(rel, false);
:

In addition, we need pay mention ATSimplePermissions() is a multi-functional
function.
(1) Ensure that it is a relation (or possibly a view)
(2) Ensure this user is the owner
(3) Ensure that it is not a system table

If we move the permission checks at the head of ATExecXXX() routines,
these checks should be broken out. At least, (1) and (3) are independent
from security model.

In the result of this modification, we will come on a clear plan to apply
permission checks on ALTER TABLE, namely, we should apply permission checks
at the head of ATExecXXXX phase (basically; exception is OWNER TO option).

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>