Re: [PATCH] remove redundant ownership checks

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