Re: [PATCH] remove redundant ownership checks

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