Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2010/01/14 15:04), Greg Smith wrote:

KaiGai Kohei wrote:

(2010/01/14 4:54), Tom Lane wrote:

Robert Haasrobertmh...@gmail.com writes:

On Wed, Jan 13, 2010 at 1:34 PM, Tom Lanet...@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.


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 

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

* KaiGai Kohei (kai...@ak.jp.nec.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Thu, Jan 14, 2010 at 1:04 AM, Greg Smith g...@2ndquadrant.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2010/01/14 23:29), Stephen Frost wrote:
 * KaiGai Kohei (kai...@ak.jp.nec.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 

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian br...@momjian.us wrote:
 Robert Haas wrote:
 On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian br...@momjian.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Kevin Grittner]

Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Jaime Casanova]

On Wed, Jan 13, 2010 at 1:15 PM, Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

* Jaime Casanova (jcasa...@systemguards.com.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian br...@momjian.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane t...@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.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Alex Hunsaker]

On Wed, Jan 13, 2010 at 12:54, Tom Lane t...@sss.pgh.pa.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...)

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Alex Hunsaker]

On Wed, Jan 13, 2010 at 13:46, Alex Hunsaker bada...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

* Alex Hunsaker (bada...@gmail.com) wrote:
 On Wed, Jan 13, 2010 at 12:54, Tom Lane t...@sss.pgh.pa.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

* Alex Hunsaker (bada...@gmail.com) wrote:
 On Wed, Jan 13, 2010 at 13:46, Alex Hunsaker bada...@gmail.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Tom Lane]

Alex Hunsaker bada...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2010/01/14 4:54), Tom Lane wrote:

Robert Haasrobertmh...@gmail.com  writes:

On Wed, Jan 13, 2010 at 1:34 PM, Tom Lanet...@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() 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 kai...@ak.jp.nec.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2010/01/14 4:27), Stephen Frost wrote:
 * Jaime Casanova (jcasa...@systemguards.com.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 kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Greg Smith]

KaiGai Kohei wrote:

(2010/01/14 4:54), Tom Lane wrote:

Robert Haasrobertmh...@gmail.com  writes:

On Wed, Jan 13, 2010 at 1:34 PM, Tom Lanet...@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 

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Bruce Momjian]

Robert Haas wrote:
 On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Robert Haas robertmh...@gmail.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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2010/01/12 10:27), Bruce Momjian wrote:
 Robert Haas wrote:
 On Sun, Jan 10, 2010 at 4:54 PM, Tom Lanet...@sss.pgh.pa.us  wrote:
 Robert Haasrobertmh...@gmail.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 kai...@ak.jp.nec.com


pgsql-fix-enable_disable_rule.3.patch
Description: application/octect-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Sun, Dec 20, 2009 at 10:53 PM, I wrote:
 On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 KaiGai Kohei kai...@ak.jp.nec.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Tom Lane]

Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Sun, Jan 10, 2010 at 4:54 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

2009/12/22 KaiGai Kohei kai...@ak.jp.nec.com:
 (2009/12/21 12:53), Robert Haas wrote:
 On Thu, Dec 17, 2009 at 7:19 PM, Tom Lanet...@sss.pgh.pa.us  wrote:
 KaiGai Koheikai...@ak.jp.nec.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2009/12/21 12:53), Robert Haas wrote:
 On Thu, Dec 17, 2009 at 7:19 PM, Tom Lanet...@sss.pgh.pa.us  wrote:
 KaiGai Koheikai...@ak.jp.nec.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 kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 KaiGai Kohei kai...@ak.jp.nec.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

KaiGai,

* KaiGai Kohei (kai...@ak.jp.nec.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2009/12/18 6:38), Stephen Frost wrote:
 KaiGai,
 
 * KaiGai Kohei (kai...@ak.jp.nec.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 kai...@ak.jp.nec.com
*** base/src/backend/rewrite/rewriteDefine.c	(revision 2486)
--- base/src/backend/rewrite/rewriteDefine.c	(working copy)
***
*** 671,677 
  {
  	Relation	pg_rewrite_desc;
  	Oid			owningRel = RelationGetRelid(rel);
- 	Oid			eventRelationOid;
  	HeapTuple	ruletup;
  	bool		changed = false;
  
--- 671,676 
***
*** 690,704 
  		rulename, get_rel_name(owningRel;
  
  	/*
- 	 * Verify that the user has appropriate permissions.
- 	 */
- 	eventRelationOid = ((Form_pg_rewrite) GETSTRUCT(ruletup))-ev_class;
- 	Assert(eventRelationOid == owningRel);
- 	if (!pg_class_ownercheck(eventRelationOid, GetUserId()))
- 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- 	   get_rel_name(eventRelationOid));
- 
- 	/*
  	 * Change ev_enabled if it is different from the desired new state.
  	 */
  	if (DatumGetChar(((Form_pg_rewrite) GETSTRUCT(ruletup))-ev_enabled) !=
--- 689,694 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Tom Lane]

KaiGai Kohei kai...@ak.jp.nec.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane t...@sss.pgh.pa.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 KaiGai Kohei kai...@ak.jp.nec.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Stephen Frost]

* Robert Haas (robertmh...@gmail.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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] remove redundant ownership checks

[Robert Haas]

On Thu, Dec 17, 2009 at 10:14 PM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

(2009/12/18 9:19), Tom Lane wrote:
 KaiGai Koheikai...@ak.jp.nec.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 kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

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 kai...@ak.jp.nec.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] remove redundant ownership checks

[KaiGai Kohei]

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 kai...@ak.jp.nec.com
*** base/src/backend/rewrite/rewriteDefine.c	2009-11-09 18:44:10.0 +0900
--- pgsec/src/backend/rewrite/rewriteDefine.c	2009-12-19 06:11:58.0 +0900
*** EnableDisableRule(Relation rel, const ch
*** 690,704 
  		rulename, get_rel_name(owningRel;
  
  	/*
- 	 * Verify that the user has appropriate permissions.
- 	 */
- 	eventRelationOid = ((Form_pg_rewrite) GETSTRUCT(ruletup))-ev_class;
- 	Assert(eventRelationOid == owningRel);
- 	if (!pg_class_ownercheck(eventRelationOid, GetUserId()))
- 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- 	   get_rel_name(eventRelationOid));
- 
- 	/*
  	 * Change ev_enabled if it is different from the desired new state.
  	 */
  	if (DatumGetChar(((Form_pg_rewrite) GETSTRUCT(ruletup))-ev_enabled) !=
--- 690,695 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers