Re: SE-PostgreSQL/Lite Review

On Thu, Dec 10, 2009 at 10:39 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Let's start by taking the patch I reviewed and splitting up
> security/access_control.c along object lines.  Of course, the individual
> security/<object>_ac.c files would only include the .h's that are
> necessary.  This would be a set of much smaller and much more
> managable files which only know about what they should know about- the
> object type they're responsible for.

Good idea.

> Clearly, code comments also need to be reviewed and issues with them
> addressed.  I'm also not a fan of the "skip permissions check"
> arguments, which are there specifically to address cascade deletions,
> which is a requirment of our PG security model.  I'd love to see a
> better solution and am open to suggestions or thoughts about what one
> would be.  I know one of KaiGai's concerns in this area was performance,
> butas we often do for PG, perhaps we should consider that second and
> first consider what it would look like if we ignored performance
> concerns.  This would change "skip permissions check" to "what object is
> being deleted, and am I a sub-object of that?".  I don't feel like I've
> explained that well enough, perhaps someone else could come up with
> another solution, or maybe figure out a better way to describe what I'm
> trying to get with this.

I don't know the right solution to this either but I'm glad you're
thinking about it.

> Regarding the special-purpose shims- I feel there should be a way for us
> to handle them better.  This might be a good use-case for column-level
> privileges in pg_catalog.  That's pure speculation at the moment tho, I
> havn't re-looked at those shims lately to see if that even makes sense.
> I don't like them either though, for what it's worth.

I am not sure if I fully understand either the problem or your
proposed solution, but it seems to me that permissions checking needs
to be forcibly crammed into a fixed number of buckets. In other
words, for, say, databases, there should be a finite list of objects
that can be performed on databases: create, drop, connect, etc. Any
permissions question that comes up has to be asked in terms of one of
those operations. So if somebody writes code, in core or contrib,
that displays the size of the database, it has to key off the same
permissions as one of those pre-existing categories. It doesn't get
to invent rules specific to that case from scratch.

(Humerous interlude: I worked on a project written in C++ many years
ago where there were these security context objects, and all
user-visible interfaces had to check with the security context before
allowing any privileged operation, using the may_i() method. By
convention, variables of the security context type were always named
"mother", thus mother->may_i(whatever)...)

[...snip...]

One comment I have in general about this process is that I think it
would enormously reduce the level of pain associated with making these
kinds of changes if we could get patches that were not full of minor
issues that need to be cleaned up (like comments not properly
adjusted, spelling/grammar errors, formatting inconsistent with the
rest of the code base, poor English). I can't speak for anyone else,
but it seems to me that asking a committer to fix all of that stuff on
top of substantive review of the patch is asking a lot. I realize
that this is difficult for non-native speakers of English and a lot of
work even for those who are fluent in the language, but it is part of
our project culture and style to do these kinds of things right and I
certainly don't want to be the one who lowers our standards in this
area. For a small patch, it's not so bad to have to fix these things
(though it's certainly nicer not to need to) but for a patch of five
or ten thousand lines, it's a LOT of work.

...Robert