Re: [PATCH] remove redundant ownership checks - Mailing list pgsql-hackers
From | Greg Smith |
---|---|
Subject | Re: [PATCH] remove redundant ownership checks |
Date | |
Msg-id | 4B4EB3F7.3090900@2ndquadrant.com Whole thread Raw |
In response to | Re: [PATCH] remove redundant ownership checks (KaiGai Kohei <kaigai@ak.jp.nec.com>) |
Responses |
Re: [PATCH] remove redundant ownership checks
Re: [PATCH] remove redundant ownership checks |
List | pgsql-hackers |
KaiGai Kohei wrote: > (2010/01/14 4:54), Tom Lane wrote: >> Robert Haas<robertmhaas@gmail.com> writes: >>> On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane<tgl@sss.pgh.pa.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@2ndQuadrant.com www.2ndQuadrant.com
pgsql-hackers by date: