Re: API change advice: Passing plan invalidation info from the rewriter into the planner? - Mailing list pgsql-hackers

On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> In my view, commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5 basically
>> *is* row-level security: instead of applying a row-level security
>> policy to a table, just create a security-barrier view over the table
>> and grant access to the view.  Forget that the table ever existed.
>> Done.
>
> This argument could have been made for column-level privileges also, no?

Not really.  First of all, we didn't have security_barrier views at
that time, let alone security barrier views that are auto-updateable.
That's a really important piece of technology which makes filtering
access via views feasible in ways that really were not feasible in the
past.  Secondly, column-level permissions - like every other
currently-existing type of permissions - are declarative.  They are an
additional opportunity for the system to say "no" to something it
otherwise would have allowed, but no user-defined code is executed.
Row-level security is not a chance for the system to deny access; it's
a chance for user-defined code to take control and perform arbitrary
operations.  So the scope of what we're contemplating for row-level
security is really far, far more invasive than what we did for
column-level privileges.

> I agree that views, or even security-definer functions, offer a great
> deal of flexibility, and that may be necessary in some use-cases, but I
> fail to see why that means we should avoid providing the mechanics to
> achieve simple and usable RLS akin to what other major RDBMS's have.

Because we don't have a good design.

I'm not categorically opposed to adding more RLS features to
PostgreSQL and never have been; in fact, I was deeply involved in the
original design of security barrier views and committed the original
patch to add that functionality to PostgreSQL, without which none of
what we're talking about here would be possible.  But the
currently-proposed design is very unappealing to me, for the reasons
that I've explained.  The right answer to "this feature doesn't
provide anything that we don't already have and will introduce major
new security exposures that haven't been adequate thought" is
debatable, but "well other people have this so we should too" is
definitely not it.  Craig's patch really hasn't grappled with any of
these thorny definition and security issues; it's just about making
the basic functionality work.  That's fine for a POC, but it's not
enough for a feature that the project would be committing to maintain
for the indefinite future.

>> That's mighty useful for debugging, at least IMHO.  And, if you want
>> to have several row-level security policies for different classes of
>> users, just create more than one view and grant different privileges
>> on each.
>
> I'm really not impressed with the idea that RLS should be done with
> multiple different views of the same underlying table.

Are you equally unimpressed with the idea that RLS as proposed can't
support more than one security policy right now *at all*?  Because it
seems to me that either you think multiple RLS policies on a single
table is important (in which case the current patch is inadequate) or
you think it's not important (in which case we need not argue about
whether doing it with multiple views over the same underlying table is
awkward).

>> By contrast, it seems to me that every design so far proposed for
>> something that is actually called row-level security - as opposed to
>> commit 842faa714c0454d67e523f5a0b6df6500e9bc1a5, which *really is*
>> row-level security, is extremely limited.  Look back at all the things
>> listed in the previous paragraph; can you do those things easily with
>> the designs that have been proposed?  As far as I can see, not really.
>
> I don't feel that RLS will, or even *should*, have the same level of
> flexibility that you can achieve with views and/or security definer
> functions.  I expect that, over time, we will add more capabilities to
> it, but it's never going to be able to redefine the contents of a column
> as a view can, nor will it be able to add columns to a table as views
> can.  I don't see those as reasons against having support for RLS.

What this patch is doing is basically allowing a table to really be a
view over itself.  If we choose to support that, I think it is
absolutely inevitable that people are going to want all the same
options that they would have if they really made a separate view -
separate permissions, WITH CHECK OPTION, all of it.  I find the
contrary argument - that people will only want X amount and no more -
simply not plausible.  If it's valuable to have some of those
capabilities in an RLS framework, somebody's going to want all of
them.  There's no bright line to divide the things that are valuable
in that context from those that aren't.

> I'm glad to hear your thoughts on the level of granularity which might
> be nice to have with RLS.  What would be great is to spend a bit more
> time reviewing what other systems provide in this area and considering
> what makes sense for us.  This will also be a feature and an area which
> we will be improving for a long time to come, but we do need this
> capability and we have to start somewhere.

I think this definitely important.  I also think that we should be
careful to study the deficiencies in those other systems and to
clearly call out what value the capabilities we're thinking of adding
to PostgreSQL 9.5 have over the status quo in PostgreSQL 9.4.  I'm not
so much arguing that we shouldn't have row-level security as that, in
every way that's really meaningful, we already do.

>> There's no way for users who are RLS exempt to turn
>> off their exemption for testing purposes, let alone on a per-table
>> basis.
>
> I don't follow this argument entirely- users can't turn off the existing
> permissions system for testing either, unless an authorized user with
> the correct permissions makes the change to allow it- or the user bumps
> themselves up to superuser, or to a role which has broader permissions,
> both of which would also be possible to do with RLS.

Sure, but in the existing system, the query either returns the same
results for everybody, or it fails outright with an error.  It's
certainly possible to screw up the existing permissions, but this new
thing that's being proposed is much more complicated, because it's not
just whether it works that's at issue, but what results you actually
get.

>> There's no way to have multiple RLS policies on a single
>> table.  All of those are things that we get "for free" in the
>> view-over-table model, and implementing formal RLS basically requires
>> us to either invent a new RLS-specific way of doing each of those
>> things, or suffer along with a subset of the functionality.  Yuck.
>
> What would probably be good is to review the use-cases which the current
> patch already addresses- and we've had good responses from actual users
> who are already playing with the patch and are hearing that it is
> addressing their requirements.

Yes.  And in particular, I think we should have a much clearer
statement than we currently do about the use cases in which it falls
short.

>> But what's really awful about this whole design is that it breaks the
>> invariant that reading from a table doesn't run anybody else's code.
>
> You're suggesting that we use views instead, which clearly could run
> someone else's code.  Perhaps the user will notice that they're
> selecting from a view instead of a table, but I've never seen a security
> design around making sure that what is being select'd from is a table
> vs. a view.  Have you seen applications which implement such a check
> prior to running a query?

Yes.  pg_dump, to name one really important one.  I wouldn't be
surprised if graphical clients did something similar - display the
table data for a table, or the view definition for a view.  But I
admit to not having checked that.  More than that, if I were a DBA,
I'd certainly be darn careful about selecting from untrusted views,
but I expect to be able to read a table, or run pg_dump, without
getting my account hacked.

> With this, I agree, there is risk associated with the implementation
> we're looking at for RLS.  We could narrow the case by reducing the
> capabilities of RLS in PG by only allowing certain functions to be used
> in the definition of a RLS policy (eg: btree operators of known data
> types, or something similar to our "leak-proof" attribute), but I don't
> see that it really buys us much.  There are a *lot* of ways in which an
> individual who has the ability to create objects inside the database can
> cause problems, but that comes with the flexibility we provide users
> with.  That will always be a balance but, I believe, we wouldn't have
> the same level of success or have such an awesome system without that
> flexibility.

I don't think restricting what can go into an RLS policy is the right
answer; that to me misses the point.  What needs to be restricted is
the possibility that a user will inadvertently run code they didn't
mean to run.

>> Every pg_dump is an
>> opportunity to hack somebody else's account, or at least audit their
>> activity.  Protecting the superuser against everybody else is nice,
>> but I think it's just as important to protect non-superusers against
>> each other, and I think that's going to be hard -- because in the RLS
>> world, SELECT * FROM tab is now *fundamentally* ambiguous.  Maybe it's
>> reading from the table, and maybe it's really clandestinely reading
>> from a view over the table, and the user has no way of being really
>> clear about which behavior they want.  From a security point of view,
>> that seems very bad.
>
> I don't see this as being an insurmountable issue.  I agree that having
> a way for pg_dump to run safely is important and the superuser check
> does address that, given that we don't have a "read-only (and
> everything)" capability today.  Once we do (and I surely hope that will
> come sooner rather than later), such a role should also have the 'no
> RLS' bit, as it wouldn't make any sense for such a role anyway.  The
> lack of that is not a strike against RLS though.

It addresses running pg_dump *as the superuser*, but not as a database
owner or just a regular users.  If unprivileged user A runs pg_dump -t
some_table_owned_by_user_b, and falls victim to a Trojan horse, that
is going to get reported as a security defect in PostgreSQL.  Telling
the person who reports that issue that it's design behavior is not
going to make them happy, or result in good press coverage for
PostgreSQL.

>> 2. There's a danger that the functionality available in the two models
>> will diverge, so that certain things can only be done in one world or
>> the other.
>
> They will always be distinct, intentionally so.

I think that's an absolutely terrible idea.  We do not want to be in
the business of having two parallel systems with slightly different
capabilities and syntax that are providing the same fundamental
functionality.  And they are: the proposal for RLS is to make it work
just like a security_barrier view, sharing a common implementation.

>> 4. Making SELECT * FROM tab ambiguous seems likely to be a security minefield.
>
> While I agree that we need to consider this, I don't think it will be a
> "minefield", but rather something we need to document and educate our
> users about.  If you'd like a "disable-all-RLS" GUC, I'm all for it.

I would definitely like that.  I have proposed it in the past.

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