Re: RangeVarGetRelid() - Mailing list pgsql-hackers

On Mon, Dec 19, 2011 at 11:55 AM, Noah Misch <noah@leadboat.com> wrote:
> Here's the rule I used: the pre-lock checks must prove that the user could, by
> some command working as-designed, have taken a lock at least as strong as the
> one we're about to acquire.  How does that work out in practice?  Relation
> owners can always take AccessExclusiveLock by way of a DROP command, so
> ownership is always sufficient as a pre-lock test.
>
> The above namespace check is no exception; the object owner has authority to
> take the AccessExclusiveLock independent of his authority to ultimately
> complete the rename.  (Changes arising from the recent discussions about
> locking namespaces during DDL would complicate the situation in other ways.)

Yes: and I have it mind to work on that for the current release cycle,
time permitting.  It seems to me that there's no real value in
consolidating this callback if there's a change coming down the pipe
that would require us to split it right back out again.

Also, while I see the point that it wouldn't be a security issue to
defer this particular check until after the lock is taken, I still
don't think it's a good idea.  I basically don't see any point in
splitting up the security checks across multiple functions.  Our
permissions-checking logic is already rather diffuse; I think that
finding more reasons for some of it to be done in one function and
some of it to be done in some other function is going in the wrong
direction.  It makes it easier for future people editing the code to
rearrange things in ways that subtly break security without realizing
it, and it's also one more obstacle for things like sepgsql which
would like to get control whenever we do a security check. (Dimitri's
recent comments about command triggers suggest that MAC isn't the only
application of an option to override the default access control
policy.)  It's also not terribly consistent from a user perspective:
hmm, if I don't have permission to do operation X because of reason A,
then my command fails immediately; if I don't have permission because
of reason B, then it waits for a lock and then fails.  Some amount of
inconsistency there is probably inevitable, because, for example, we
won't know whether you have permission on an entire inheritance
hierarchy until we start walking it.  But I don't really see the point
in going out of our way to add more of it.

I think it's important to keep in mind that most of the code that is
going into those callbacks is just being moved.  We're not really
ending up with any more code overall, except to the extent that there
are a couple of lines of net increase for each callback because, well,
it has a comment, and maybe a declaration, and some curly braces at
the beginning and the end.  The ownership check is two lines of code;
it doesn't matter whether we duplicate that or not.  In many cases, I
think the callbacks are actually increasing readability, by pulling a
bunch of related checks into their own function instead of cluttering
up the main code path with them.  I don't want to end up with a lot of
needless code duplication, but I also don't feel any powerful need to
squeeze the number of callback functions down to an absolute minimum
just because I can.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company