Skip site navigation (1) Skip section navigation (2)

Re: [PATCH] remove redundant ownership checks

From: Greg Smith <greg(at)2ndquadrant(dot)com>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] remove redundant ownership checks
Date: 2010-01-14 06:04:39
Message-ID: 4B4EB3F7.3090900@2ndquadrant.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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


In response to

Responses

pgsql-hackers by date

Next:From: KaiGai KoheiDate: 2010-01-14 07:04:17
Subject: Re: [BUG?] strange behavior in ALTER TABLE ... RENAME TO on inherited columns
Previous:From: Tom LaneDate: 2010-01-14 05:52:10
Subject: Re: Pretty printed trigger in psql

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group