Re: RLS Design

Hi all

One of the remaining issues with row security is how to pass plan
invalidation information generated in the rewriter back into the planner.

With row security, it's necessary to set a field in PlannerGlobal,
tracking the user ID of the user the query was planned for if row
security was applied. It is also necessary to add a PlanInvalItem for
the user ID.

Currently the rewriter has no way to pass this information to the
planner. QueryRewrite returns just a Query*.

We use Query structs throughout the rewriter and planner; it doesn't
make sense to add a List* field for PlanInvalItem nodes and an Oid field
for the user ID to the Query node when it's only ever going to get used
for the top level Query node returned by the rewriter, and only for long
enough to copy the data into PlannerGlobal.

The alternative seems to be changing the return type of QueryRewrite,
introducing a new node type, say:

struct RewriteResult {
Query *productQuery;
Oid planUserId;
List* planInvalItems;
}

This seems cleaner, and more extensible, but it means changing a fair
bit of API, including:

pg_plan_query
planner
standard_planner
planner_hook_type
QueryRewrite

and probably the plan cache infrastructure too. So it'd be fairly
invasive, and I know that creates concerns about backpatching and
extensions.

I can't just polymorphically subclass Query as some kind of "TopQuery" -
no true polymorphism in C, would need a new NodeType for it, and then
need to teach everything that knows about T_Query about T_TopQuery too.
So that won't work.

So, I'm looking for advice before I embark on this change. I need _some_
way to pass invalidation information from the rewriter into the planner
when it's collected by row security code during rewriting.

Any advice/comments?

I'm inclined to bite the bullet and make the API change. It'll be a
pain, but I can see future uses for passing global info out of the
rewriter rather than shoving it into per-Query structures. I'd define a
RewriteResult and pass that down into all the rewriter internal
functions, then return the outer query wrapped in it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer escribió:

> One of the remaining issues with row security is how to pass plan
> invalidation information generated in the rewriter back into the planner.

I think I already asked this, but would it work to extract this info by
walking the rewritten list of queries instead; and in case it would,
would that be any easier than the API change you're proposing?

> We use Query structs throughout the rewriter and planner; it doesn't
> make sense to add a List* field for PlanInvalItem nodes and an Oid field
> for the user ID to the Query node when it's only ever going to get used
> for the top level Query node returned by the rewriter, and only for long
> enough to copy the data into PlannerGlobal.

So there is an assumption that you can't have a subquery that uses a
different role ID than the main query. That sounds fine, and anyway I
don't think we're prepared to deal with differing userids for
subqueries, so the proposal that it belongs only on the top-level node
is acceptable. And from there, it seems that not putting the info in
Query (which would be a waste everywhere else than the toplevel query
node) is sensible.

> The alternative seems to be changing the return type of QueryRewrite,
> introducing a new node type, say:
>
> struct RewriteResult {
> Query *productQuery;
> Oid planUserId;
> List* planInvalItems;
> }
>
> This seems cleaner, and more extensible, but it means changing a fair
> bit of API, including:
>
> pg_plan_query
> planner
> standard_planner
> planner_hook_type
> QueryRewrite

I think we should just bite the bullet and do the change (a new struct,
I assume, not a new node). It will cause an incompatibility to anyone
that has written planner hooks, but probably the number of such hooks is
not very large anyway.

I don't think we should base decisions on the amount of backpatching
pain we cause, for patches that involve new functionality such as this
one. We commit patches that will cause future merge conflicts all the
time.

> I'm inclined to bite the bullet and make the API change. It'll be a
> pain, but I can see future uses for passing global info out of the
> rewriter rather than shoving it into per-Query structures. I'd define a
> RewriteResult and pass that down into all the rewriter internal
> functions, then return the outer query wrapped in it.

Is there already something in Query that could be a toplevel struct
member only?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)hobby(dot)2ndquadrant(dot)com> writes:
> One of the remaining issues with row security is how to pass plan
> invalidation information generated in the rewriter back into the planner.

> With row security, it's necessary to set a field in PlannerGlobal,
> tracking the user ID of the user the query was planned for if row
> security was applied. It is also necessary to add a PlanInvalItem for
> the user ID.

TBH I'd just add a user OID field in struct Query and not hack up a bunch
of existing function APIs. It's not much worse than the existing
constraintDeps field.

The PlanInvalItem could perfectly well be generated by the planner,
no, if it has the user OID? But I'm not real sure why you need it.
I don't see the reason for an invalidation triggered by user ID.
What exactly about the *user*, and not something else, would trigger
plan invalidation?

What we do need is a notion that a plan cache entry might only be
valid for a specific calling user ID; but that's a matter for cache
entry lookup not for subsequent invalidation.

regards, tom lane

On 03/06/2014 02:58 AM, Tom Lane wrote:
> Craig Ringer <craig(at)hobby(dot)2ndquadrant(dot)com> writes:
>> One of the remaining issues with row security is how to pass plan
>> invalidation information generated in the rewriter back into the planner.
>
>> With row security, it's necessary to set a field in PlannerGlobal,
>> tracking the user ID of the user the query was planned for if row
>> security was applied. It is also necessary to add a PlanInvalItem for
>> the user ID.
>
> TBH I'd just add a user OID field in struct Query and not hack up a bunch
> of existing function APIs. It's not much worse than the existing
> constraintDeps field.

If you're happy with that, I certainly won't complain. It's much simpler
and less intrusive.

I should be able to post an update using this later today.

> The PlanInvalItem could perfectly well be generated by the planner,
> no, if it has the user OID? But I'm not real sure why you need it.
> I don't see the reason for an invalidation triggered by user ID.
> What exactly about the *user*, and not something else, would trigger
> plan invalidation?

It's only that the plan depends on the user ID. There's no point keeping
the plan around if the user no longer exists.

You're quite right that this can be done in the planner when a
dependency on the user ID is found, though. So there's no need to pass a
PlanInvalItem down, which is a lot nicer.

> What we do need is a notion that a plan cache entry might only be
> valid for a specific calling user ID; but that's a matter for cache
> entry lookup not for subsequent invalidation.

Yes, that would be good, but is IMO more of a separate optimization. I'm
currently using KaiGai's code to invalidate and re-plan when a user ID
change is detected.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> On 03/06/2014 02:58 AM, Tom Lane wrote:
>> The PlanInvalItem could perfectly well be generated by the planner,
>> no, if it has the user OID? But I'm not real sure why you need it.
>> I don't see the reason for an invalidation triggered by user ID.
>> What exactly about the *user*, and not something else, would trigger
>> plan invalidation?

> It's only that the plan depends on the user ID. There's no point keeping
> the plan around if the user no longer exists.

[ shrug... ] Leaving such a plan cached would be harmless, though.
Furthermore, the user ID we'd be talking about is either the owner
of the current session, or the owner of some view or security-definer
function that the plan is already dependent on, so it's fairly hard
to credit that the plan would survive long enough for the issue to
arise.

Even if there is a scenario where invalidating by user ID is actually
useful, I think adding infrastructure to cause invalidation in such a case
is optimizing for the wrong thing. You're adding cycles to every query to
benefit a case that is going to be quite infrequent in practice.

>> What we do need is a notion that a plan cache entry might only be
>> valid for a specific calling user ID; but that's a matter for cache
>> entry lookup not for subsequent invalidation.

> Yes, that would be good, but is IMO more of a separate optimization. I'm
> currently using KaiGai's code to invalidate and re-plan when a user ID
> change is detected.

I'm unlikely to accept a patch that does that; wouldn't it be catastrophic
for performance in the presence of security-definer functions? You can't
just trash the whole plan cache when a user ID switch occurs.

regards, tom lane

Craig, Tom, all,

I've been through the RLS code over the past couple of days which I
pulled from Craig's repo and have a bunch of minor updates. In general,
the patch seems pretty reasonable- except for the issues discussed
below. Quite a bit of this patch is tied up in plan invalidation and
tracking if the security quals depend on the current user, all of which
seems pretty grotty and the wrong way around to me.

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> > It's only that the plan depends on the user ID. There's no point keeping
> > the plan around if the user no longer exists.
>
> [ shrug... ] Leaving such a plan cached would be harmless, though.

Agreed.

> Furthermore, the user ID we'd be talking about is either the owner
> of the current session, or the owner of some view or security-definer
> function that the plan is already dependent on, so it's fairly hard
> to credit that the plan would survive long enough for the issue to
> arise.

I don't entirely follow which 'issue' is being referred to here, but we
need to consider that 'set role' changes should also cause a new plan.

> Even if there is a scenario where invalidating by user ID is actually
> useful, I think adding infrastructure to cause invalidation in such a case
> is optimizing for the wrong thing. You're adding cycles to every query to
> benefit a case that is going to be quite infrequent in practice.

Yeah, I have a hard time seeing that there's an issue w/ keeping the
cached plans around even if the session never goes back to being under
the user ID for which those older plans were built. Also, wouldn't a
'RESET ALL' clear any of them anyway?

> > Yes, that would be good, but is IMO more of a separate optimization. I'm
> > currently using KaiGai's code to invalidate and re-plan when a user ID
> > change is detected.
>
> I'm unlikely to accept a patch that does that; wouldn't it be catastrophic
> for performance in the presence of security-definer functions? You can't
> just trash the whole plan cache when a user ID switch occurs.

Yeah, this doesn't seem like the right approach. Adding the user ID to
the cache key definitely strikes me as the right way to fix this.

I've uploaded the latest patch, rebased against master, with my changes
to here: http://snowman.net/~sfrost/rls_ringerc_sf.patch.gz as I don't
believe it'd clear the mailing list (it's 29k).

I'll take a look at changing the cache key to include user ID and
ripping out the plan invalidation logic from the current patch tomorrow
but I seriously doubt I'll be able to get all of that done in the next
day or two. If anyone else is able to help out, it'd certainly be
appreciated; I really think that's the main hurdle to address at this
point with this patch- without the plan invalidation complexity, the
the patch is really just building out the catalog, the SQL-level
statements for managing it, and the bit of code required to add the
conditional to statements involving RLS-enabled tables.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I've uploaded the latest patch, rebased against master, with my changes
> to here: http://snowman.net/~sfrost/rls_ringerc_sf.patch.gz as I don't
> believe it'd clear the mailing list (it's 29k).

Please actually post it, for the archives' sake. 29k is far below the
list limit. (Which I don't know exactly what it is ... but certainly
in the hundreds of KB.)

> I'll take a look at changing the cache key to include user ID and
> ripping out the plan invalidation logic from the current patch tomorrow
> but I seriously doubt I'll be able to get all of that done in the next
> day or two.

TBH I think we are up against the deadline. April 15 was the agreed-to
drop dead date for pushing new features into 9.4.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I've uploaded the latest patch, rebased against master, with my changes
> > to here: http://snowman.net/~sfrost/rls_ringerc_sf.patch.gz as I don't
> > believe it'd clear the mailing list (it's 29k).
>
> Please actually post it, for the archives' sake. 29k is far below the
> list limit. (Which I don't know exactly what it is ... but certainly
> in the hundreds of KB.)

Huh, thought it was more like 25k. Well, here goes then...

> > I'll take a look at changing the cache key to include user ID and
> > ripping out the plan invalidation logic from the current patch tomorrow
> > but I seriously doubt I'll be able to get all of that done in the next
> > day or two.
>
> TBH I think we are up against the deadline. April 15 was the agreed-to
> drop dead date for pushing new features into 9.4.

Yeah. :/ May be for the best anyway, this should be able to go in early
in the 9.5 cycle and get more testing and refinement. Still stinks
though as I feel like this patch didn't get the attention it should have
due to a simple misunderstanding, but we do need to stop at some point
to get a release together.

Thanks,

Stephen

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/15/2014 10:06 AM, Stephen Frost wrote:
> I've uploaded the latest patch, rebased against master, with my
> changes to here: http://snowman.net/~sfrost/rls_ringerc_sf.patch.gz
> as I don't believe it'd clear the mailing list (it's 29k).

Does this exist in the form of an accessible git branch, too?

I was trying to maintain the patch as a series of distinct changes to
make it easier to see what each part is doing, and it'd be nice to
preserve that if possible. It also makes seeing what's changed a lot
easier.

- --
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTWbGNAAoJELBXNkqjr+S28W4H/R49CJfz4Y3TMbvwxhrwkjL2
WEv80qY4GDCzG5CGKROn3kT9H5xePvL9eadSjr+CPsilerHrPkHmXnU5w+K2LnKV
MCL/A2969b4ng1cUK9eHEFVx9BLLQmiVI6DbJ2OA2oWUs/Y7Zne5h6q0fNnnnTSq
XEU6r3tVkUp5ipbhHi+aJ+mfckirdcMR0U5X+2fgGpLZ3D+8j9azvuXvQjSOekVB
3+EVVI0UXhhvw4It4/1CjieHvScdxnsz9bOpKGiEeePUB3CGC0iPtBgIGtE0n2OK
cqKryuwZ3++LZih74M8z+Rn6yao5f4ElJrO3gz5q8axKzH/bHkEYElwEUhVfbSE=
=AKzL
-----END PGP SIGNATURE-----

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 04/15/2014 10:06 AM, Stephen Frost wrote:
> > I've uploaded the latest patch, rebased against master, with my
> > changes to here: http://snowman.net/~sfrost/rls_ringerc_sf.patch.gz
> > as I don't believe it'd clear the mailing list (it's 29k).
>
> Does this exist in the form of an accessible git branch, too?

Eh, no.

> I was trying to maintain the patch as a series of distinct changes to
> make it easier to see what each part is doing, and it'd be nice to
> preserve that if possible. It also makes seeing what's changed a lot
> easier.

Yeah, I almost just posted a patch against your tree. I'll look at
doing that tomorrow.

Thanks,

Stephen

Hi all,

This is my first post to the mailing list and I am looking forward to
working with everyone in the community.

With that said...

I'll take a look at changing the cache key to include user ID and
> ripping out the plan invalidation logic from the current patch tomorrow
> but I seriously doubt I'll be able to get all of that done in the next
> day or two. If anyone else is able to help out, it'd certainly be
> appreciated; I really think that's the main hurdle to address at this
> point with this patch- without the plan invalidation complexity, the
> the patch is really just building out the catalog, the SQL-level
> statements for managing it, and the bit of code required to add the
> conditional to statements involving RLS-enabled tables.
>

I have been collaborating with Stephen on addressing this particular item
with RLS.

As a basis, I have been working with Craig's 'rls-9.4-upd-sb-views' branch
rebased against master around 9.4beta1.

Through this effort, we have concluded that for RLS the case of
invalidating a plan is only necessary when switching between a superuser
and a non-superuser. Obviously, re-planning on every role change would be
too costly, but this approach should help minimize that cost. As well,
there were not any cases outside of this one that were immediately apparent
with respect to RLS that would require re-planning on a per userid basis.

I have tested this approach with the following patch.

https://github.com/abrightwell/postgres/commit/4c959e63f7a89b24ebbd46575a31a629d24efa75

Does this sound like a sane approach? Thoughts? Recommendations?

Thanks,
Adam

Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com> writes:
> Through this effort, we have concluded that for RLS the case of
> invalidating a plan is only necessary when switching between a superuser
> and a non-superuser. Obviously, re-planning on every role change would be
> too costly, but this approach should help minimize that cost. As well,
> there were not any cases outside of this one that were immediately apparent
> with respect to RLS that would require re-planning on a per userid basis.

Hm ... I'm not following why we'd need a special case for superusers and
not anyone else? Seems like any useful RLS scheme is going to require
more privilege levels than just superuser and not-superuser.

Could we put the "if superuser then ok" test into the RLS condition test
and thereby not need more than one plan at all?

regards, tom lane

Hey Tom,

> Hm ... I'm not following why we'd need a special case for superusers and
> not anyone else? Seems like any useful RLS scheme is going to require
> more privilege levels than just superuser and not-superuser.
>

As it stands right now, superuser is the only case where RLS policies
should not be applied/completely ignored. I suppose it is possible to
create RLS policies that are related to other privilege levels, but those
would still need to be applied despite user id, excepting superuser. I'll
defer to Stephen or Craig on the usefulness of this scheme.

Could we put the "if superuser then ok" test into the RLS condition test
> and thereby not need more than one plan at all?
>

As I understand it, the application of RLS policies occurs in the rewriter.
Therefore, when switching back and forth between superuser and
not-superuser the query must be rewritten, which would ultimately result in
the need for a new plan correct? If that is the case, then I am not sure
how one plan is possible. However, again, I'll have to defer to Stephen or
Craig on this one.

Thanks,
Adam

On 06/11/2014 02:19 AM, Tom Lane wrote:
> Hm ... I'm not following why we'd need a special case for superusers and
> not anyone else? Seems like any useful RLS scheme is going to require
> more privilege levels than just superuser and not-superuser.

What it really needs is to invalidate plans when switching between
RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an "RLS
exempt" right or mode sooner rather than later, so I'm against tying
this explicitly to superuser as such.

I wouldn't be surprised to see

SET ROW SECURITY ON|OFF

down the track, with a right controlling whether you can or not. Or at
least, a right that directly exempts a user from row security.

> Could we put the "if superuser then ok" test into the RLS condition test
> and thereby not need more than one plan at all?

Only if we put it in another level of security barrier subquery, because
otherwise the planner might execute the other quals (including possible
user defined functions) before the superuser test. Which was the whole
reason for the superuser test in the first place.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> On 06/11/2014 02:19 AM, Tom Lane wrote:
>> Could we put the "if superuser then ok" test into the RLS condition test
>> and thereby not need more than one plan at all?

> Only if we put it in another level of security barrier subquery, because
> otherwise the planner might execute the other quals (including possible
> user defined functions) before the superuser test. Which was the whole
> reason for the superuser test in the first place.

Is the point of that that the table owner might have put trojan-horse
functions into the RLS qual? If so, why are we only concerned about
defending the superuser and not other users? Seems like the right fix
would be to insist that functions in the RLS qual run as the table owner.
Granted, that might be painful to do. But it still seems like "we only
need to do this for superusers" is designing with blinkers on.

regards, tom lane

On 06/11/2014 07:24 AM, Tom Lane wrote:
> Is the point of that that the table owner might have put trojan-horse
> functions into the RLS qual? If so, why are we only concerned about
> defending the superuser and not other users? Seems like the right fix
> would be to insist that functions in the RLS qual run as the table owner.
> Granted, that might be painful to do. But it still seems like "we only
> need to do this for superusers" is designing with blinkers on.

I agree, and now that the urgency of trying to deliver this for 9.4 is
over it's worth seeing if we can just run as table owner.

Failing that, we could take the approach a certain other RDBMS does and
make the ability to define row security quals a GRANTable right
initially held only by the superuser.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> On 06/11/2014 07:24 AM, Tom Lane wrote:
>> Is the point of that that the table owner might have put trojan-horse
>> functions into the RLS qual? If so, why are we only concerned about
>> defending the superuser and not other users? Seems like the right fix
>> would be to insist that functions in the RLS qual run as the table owner.
>> Granted, that might be painful to do. But it still seems like "we only
>> need to do this for superusers" is designing with blinkers on.

> I agree, and now that the urgency of trying to deliver this for 9.4 is
> over it's worth seeing if we can just run as table owner.

> Failing that, we could take the approach a certain other RDBMS does and
> make the ability to define row security quals a GRANTable right
> initially held only by the superuser.

Hmm ... that might be a workable compromise. I think the main issue here
is whether we expect that RLS quals will be something that the planner
could optimize to any meaningful extent. If they're always (in effect)
wrapped in SECURITY DEFINER functions, I think that largely blocks any
optimizations; but maybe that wouldn't matter in practice.

regards, tom lane

On Tue, Jun 10, 2014 at 7:18 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 06/11/2014 02:19 AM, Tom Lane wrote:
>> Hm ... I'm not following why we'd need a special case for superusers and
>> not anyone else? Seems like any useful RLS scheme is going to require
>> more privilege levels than just superuser and not-superuser.
>
> What it really needs is to invalidate plans when switching between
> RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an "RLS
> exempt" right or mode sooner rather than later, so I'm against tying
> this explicitly to superuser as such.
>
> I wouldn't be surprised to see
>
> SET ROW SECURITY ON|OFF
>
> down the track, with a right controlling whether you can or not. Or at
> least, a right that directly exempts a user from row security.

I'm really concerned about the security implications of this patch. I
think we're setting ourselves up for a whole lot of hurt for somewhat
unclear gain.

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.

With this approach, there's a lot of stuff that we don't have to
reinvent. We've talked a lot about whether row-level security should
only be concerned with the rows it scans, or whether it should also
restrict the new rows that can be created. You can get either
behavior by choosing whether or not to use WITH CHECK OPTION. And
then there's this question of who should be RLS-exempt; that's
basically a question of to whom you grant privileges on the underlying
table. Note that this can be very fine-grained: for example, you can
allow someone to exempt themselves for selects but not for updates by
granting them SELECT privileges but not UPDATE privileges on the
underlying table. And potentially-exempt users can choose whether
they want a particular access to actually be exempt by targeting the
view when they don't want to be exempt and the table when they do.
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.

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.
Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough
equivalent of WITH CHECK OPTION, probably because we've talked a lot
about that specific issue, but it doesn't line up exactly to what WITH
CHECK OPTION actually does. There's no independently-grantable
RLS-exemption privilege - and even when we talk about that, it's
usually some kind of global bit that applies to all tables and all
operations equally - whereas with the above approach it can be
per-table and per-operation and doesn't require superuser intervention
to flip the bit. 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. 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.

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.
It's already the case that users need to be awfully careful about
modifying tables, because that might fire triggers that do bad things.
But at least you can SELECT from a table and it will either work, or
it will fail with a permission denied error. What it will not do is
unexpectedly run some code that you weren't expecting it to run. You
can't be so blithe about selecting from views, but reading a plain
table is always OK. Now, as soon as we introduce the concept that
selecting from a table might not really mean "read from the table" but
"read from the table after applying this owner-specified qual", we're
opening up a whole new set of attack surfaces. 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.

To recap:

1. Reinventing RLS-specific ways to do all of the things that can
already be done in the view-over-table model is a lot of work.
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.
3. On the whole, it seems likely that the RLS-specific world will
remain impoverished compared to the view-over-table model.
4. Making SELECT * FROM tab ambiguous seems likely to be a security minefield.

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

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 06/11/2014 02:19 AM, Tom Lane wrote:
> > Hm ... I'm not following why we'd need a special case for superusers and
> > not anyone else? Seems like any useful RLS scheme is going to require
> > more privilege levels than just superuser and not-superuser.
>
> What it really needs is to invalidate plans when switching between
> RLS-enabled and RLS-exempt users, yes. I'm sure we'll want an "RLS
> exempt" right or mode sooner rather than later, so I'm against tying
> this explicitly to superuser as such.

That certainly sounds reasonable to me, but the point is we're just
looking to see if the current role executing the plan should or should
not have RLS applied and, if it's changing, we need to re-plan. We
don't need to actually track an independent plan for each and every user
executing the plan, which means that the plan cache can be largely left
alone.

> I wouldn't be surprised to see
>
> SET ROW SECURITY ON|OFF
>
> down the track, with a right controlling whether you can or not. Or at
> least, a right that directly exempts a user from row security.

Agreed, but doing a re-planning in that case seems reasonable to me. I
find it pretty unlikely that there will be a lot of critical path cases
of the same plan flipping back and forth between a role for which RLS is
applied and a role where it shouldn't be.

> > Could we put the "if superuser then ok" test into the RLS condition test
> > and thereby not need more than one plan at all?
>
> Only if we put it in another level of security barrier subquery, because
> otherwise the planner might execute the other quals (including possible
> user defined functions) before the superuser test. Which was the whole
> reason for the superuser test in the first place.

Yeah, I'm not a big fan of this and it certainly seems a simpler
approach to just force a re-plan. We're talking about a query which
has been prepared and then is being executed by different roles, some
of which are RLS enabled and some which are RLS exempt. That just
strikes me as pretty unlikely to happen and if it does become an issue,
a user could work around it by having two different plans prepared and
making sure that they are called from the appropriate roles to avoid the
replanning.

Thanks,

Stephen

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 06/11/2014 07:24 AM, Tom Lane wrote:
> > Is the point of that that the table owner might have put trojan-horse
> > functions into the RLS qual? If so, why are we only concerned about
> > defending the superuser and not other users? Seems like the right fix
> > would be to insist that functions in the RLS qual run as the table owner.
> > Granted, that might be painful to do. But it still seems like "we only
> > need to do this for superusers" is designing with blinkers on.
>
> I agree, and now that the urgency of trying to deliver this for 9.4 is
> over it's worth seeing if we can just run as table owner.

We'll need to work out how to ensure that things like current_user()
still returns the calling user in that case, otherwise it won't make any
sense. In general, I agree that having the RLS quals run as the table
owner is a good approach and would love to hear suggestions about how we
can make that happen.

> Failing that, we could take the approach a certain other RDBMS does and
> make the ability to define row security quals a GRANTable right
> initially held only by the superuser.

I don't particularly like this idea- it's akin, to me anyway, to making
the ability to control other permissions on a table (SELECT, INSERT,
etc) something which a user would have to be granted- and it doesn't
really address the issue.

Thanks,

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> I'm really concerned about the security implications of this patch. I
> think we're setting ourselves up for a whole lot of hurt for somewhat
> unclear gain.

I'm certainly of a different opinion and, for the most part, I feel that
if there are security concerns then they need to be addressed- and
better by us than by asking users to use some other mechanism to
implement RLS.

> 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?
Yet I don't hear any calls for that to be ripped out now that you could
implement it through updatable security-barrier views. That commit was
the ground-work to allow us to finally get proper RLS and I'm very
disappointed to hear that the mechanical pieces around making RLS easy
for users to use (and getting that check-box taken care of in a wide
variety of fields that we are being exposed to now, see the PGConf.NYC
keynote speakers...) is receiving such push-back.

> With this approach, there's a lot of stuff that we don't have to
> reinvent. We've talked a lot about whether row-level security should
> only be concerned with the rows it scans, or whether it should also
> restrict the new rows that can be created. You can get either
> behavior by choosing whether or not to use WITH CHECK OPTION. And
> then there's this question of who should be RLS-exempt; that's
> basically a question of to whom you grant privileges on the underlying
> table. Note that this can be very fine-grained: for example, you can
> allow someone to exempt themselves for selects but not for updates by
> granting them SELECT privileges but not UPDATE privileges on the
> underlying table. And potentially-exempt users can choose whether
> they want a particular access to actually be exempt by targeting the
> view when they don't want to be exempt and the table when they do.

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.

> 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.

> 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.

> Your (Craig's) rls-9.4-upd-sb-views patch seems to have a rough
> equivalent of WITH CHECK OPTION, probably because we've talked a lot
> about that specific issue, but it doesn't line up exactly to what WITH
> CHECK OPTION actually does. There's no independently-grantable
> RLS-exemption privilege - and even when we talk about that, it's
> usually some kind of global bit that applies to all tables and all
> operations equally - whereas with the above approach it can be
> per-table and per-operation and doesn't require superuser intervention
> to flip the bit.

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.

> 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.

> 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.

> 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?

> It's already the case that users need to be awfully careful about
> modifying tables, because that might fire triggers that do bad things.
> But at least you can SELECT from a table and it will either work, or
> it will fail with a permission denied error. What it will not do is
> unexpectedly run some code that you weren't expecting it to run. You
> can't be so blithe about selecting from views, but reading a plain
> table is always OK. Now, as soon as we introduce the concept that
> selecting from a table might not really mean "read from the table" but
> "read from the table after applying this owner-specified qual", we're
> opening up a whole new set of attack surfaces.

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.

> 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.

> To recap:
>
> 1. Reinventing RLS-specific ways to do all of the things that can
> already be done in the view-over-table model is a lot of work.

I agree that there's a fair bit of work involved, but I do not see
reimplementing views as RLS as the goal.

> 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.

> 3. On the whole, it seems likely that the RLS-specific world will
> remain impoverished compared to the view-over-table model.

Agreed. As is the case with views vs. security definer functions.

> 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.

Tossing out any hope of having RLS in PG is tossing the baby out with
the bathwater though, imv.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> I'm really concerned about the security implications of this patch. I
>> think we're setting ourselves up for a whole lot of hurt for somewhat
>> unclear gain.

> I'm certainly of a different opinion and, for the most part, I feel that
> if there are security concerns then they need to be addressed- and
> better by us than by asking users to use some other mechanism to
> implement RLS.

TBH, I found Robert's argument pretty persuasive. The idea that
"SELECT * FROM table" might invoke arbitrary processing ought to scare
anyone who's concerned about security, because that's going to completely
break any assumptions about pg_dump being safe for instance, as well as
force top-to-bottom rethinking of many other security assumptions.

> ... That commit was
> the ground-work to allow us to finally get proper RLS and I'm very
> disappointed to hear that the mechanical pieces around making RLS easy
> for users to use (and getting that check-box taken care of in a wide
> variety of fields that we are being exposed to now, see the PGConf.NYC
> keynote speakers...) is receiving such push-back.

If this is being sold as merely "ease of use", then it is probably going
to get rejected. In order to get some extra ease of use for the minority
of users who need RLS, you are going to significantly complicate the lives
of all Postgres users. That's not a net win in any sane calculation of
ease of use.

Maybe the right thing to think about is how we can make it easier to set
up table + view combinations according to the pattern Robert described.
I wouldn't have a problem with some more-or-less-automated support for
doing that. (Consider SERIAL as a possible precedent here: it's basically
a table creation macro.)

> 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.

pg_dump is a sufficient counterexample to that statement.

regards, tom lane

On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost <sfrost(at)snowman(dot)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

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> I'm really concerned about the security implications of this patch. I
> >> think we're setting ourselves up for a whole lot of hurt for somewhat
> >> unclear gain.
>
> > I'm certainly of a different opinion and, for the most part, I feel that
> > if there are security concerns then they need to be addressed- and
> > better by us than by asking users to use some other mechanism to
> > implement RLS.
>
> TBH, I found Robert's argument pretty persuasive. The idea that
> "SELECT * FROM table" might invoke arbitrary processing ought to scare
> anyone who's concerned about security, because that's going to completely
> break any assumptions about pg_dump being safe for instance, as well as
> force top-to-bottom rethinking of many other security assumptions.

SELECT triggers for a wide variety of use-cases are pretty commonly
asked for here and are something I'd like to see us support also. There
are also quite a few ways in which a select can end up executing code.
Today it requires more than 'select * from table;', but not very
much.. I agree that it'd be good if we had a way to address that but I
continue to view that as an independent issue.

What I haven't heard any comments on, yet found interesting, was the
idea of having the RLS quals run as the owner of the table. Would that
address these concerns? I seem to recall wondering why we didn't do
that for views in the first place, though I doubt we could change it
now even if we wanted to (and I'm guessing the spec has something to say
about this, though I haven't gone and looked and don't remember
offhand). It's certainly rather curious that functions called under a
view are run as the calling user while permissions checks on relations
referred to by the view are as the view owner.

Hopefully that will make the rest of this discussion less relevant, but
I'll respond with my feelings anyway.

> > ... That commit was
> > the ground-work to allow us to finally get proper RLS and I'm very
> > disappointed to hear that the mechanical pieces around making RLS easy
> > for users to use (and getting that check-box taken care of in a wide
> > variety of fields that we are being exposed to now, see the PGConf.NYC
> > keynote speakers...) is receiving such push-back.
>
> If this is being sold as merely "ease of use", then it is probably going
> to get rejected. In order to get some extra ease of use for the minority
> of users who need RLS, you are going to significantly complicate the lives
> of all Postgres users. That's not a net win in any sane calculation of
> ease of use.

I don't view this as being at all accurate- how is this complicating the
lives of all Postgres users? If they are worried about running user
defined code then they *already* have a lot to worry about.

While the users of RLS might be less than 50% and therefore the
minority, I expect it will have quite a bit of up-take in certain
industries and I know that our lack of any RLS is currently preventing
use of Postgres in some rather important cases.

As for it being ease-of-use, again, there are ways in which column level
privileges could have been dealt with using views, rules, security
definer functions, etc, but that doesn't mean we don't want that
feature. I certainly view RLS (and have for quite some time..) as a much
needed capability, even if it can be done today using a bunch of user
written code that must be security audited.

> Maybe the right thing to think about is how we can make it easier to set
> up table + view combinations according to the pattern Robert described.

While this sounds interesting, I don't see adding columns or redefining
them as being in the perview of RLS. The current approach of
allowing a boolean expression to be defined is both extremely flexible
while also being simple when the requirement is simple. Having to
create, manage, update, etc, an independent object would add unnecessary
complexity.

Perhaps having it be a boolean expression is too much flexibility but
the alternatives that I can think of aren't terribly attractive to me
and the boolean expression approach is what folks coming from other
RDBMS's will be familiar with and understand how to build their
applications around. We may need to provide some additional pieces
around this (perhaps a trigger-like function type which also gets
information about the object being queried, etc) but the point is to
have a straight-forward and simply reasoned about way of limiting what
data is returned.

> I wouldn't have a problem with some more-or-less-automated support for
> doing that. (Consider SERIAL as a possible precedent here: it's basically
> a table creation macro.)

Perhaps there's a way to make that work, but personally it looks like a
whole bunch more work and I don't see the gain. How would adding RLS to
an existing table work? It's worse than the SERIAL case as at least
a default clause can be added later without impacting the application
code. Would the functions referenced through such a view run as the
user of the view?

> > 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.
>
> pg_dump is a sufficient counterexample to that statement.

No, it isn't. pg_dump's defined purpose is explicitly to pull out the
data contents underneath, or the definition of the object, which means
it happens to issue explicit select * from table's (or COPY commands)
for tables and pull the view definition for views.

There's no way to even ask it to dump out the contents of a view (rather
than the definition of it). I don't consider that a security design
which checks if the object *that we're asking to select the contents of*
is checked to see if it's a view or a table, in order to avoid calling
user-defined code.

I agree that pg_dump takes many precautions to avoid running user code
in a way which could be dangerous, both to avoid security issues and
because its goal is to reproduce the system exactly as it was, and
running user code would likely cause problems for that.

I still do not buy this argument that individuals or applications pay
much more attention to selecting from views than they do selecting from
tables, or generally go out of their way to try and avoid running user
defined code (indeed, much of the point is to be able to add such things
without having to change the application around..).

We care about these issues a great deal in pg_dump, rightfully, but
psql, pgAdmin3, Perl DBD/DBI, libpq-using application, etc, etc, have no
mechanism to say "give me just the data and only the data and don't run
any user-defined code". Adding that capability might be interesting if
we can figure out how exactly to define it but it's still an orthogonal
issue to RLS, imv.

Thanks,

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jun 11, 2014 at 12:23 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > 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.

We had security definer functions, even set-returning ones, along with
rules and triggers.

> 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.

We could try to avoid calling user-defined code for RLS, but it'd add a
whole lot of complexity and as far as I can see and your proposed
solution isn't avoiding the user-defined code anyway, so I'm not sure
why this solution should be required to meet that.

> 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.

In this case the user-defined code needs to return a boolean. We don't
currently do anything to prevent it from having side-effects, no, but
the same is true with views which incorporate functions. I agree that
it makes a difference when compared to column-level privileges, but my
point was that we have provided easier ways to do things which were
possible using more complicated methods before. Perhaps the risk with
RLS is higher but these issues look managable to me and the level of
doubt about our ability to provide this feature in a reasonable and
principled way that our users will understand surprises me.

> > 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.

We're using a design that's found in multiple other RDBMS's and used
extensively by certain industries which use those RDBMS's today. I'm
certainly open to improving what is found in other systems for PG but I
have a hard time seeing this approach as a bad design. Perhaps you're
referring to our implementation, in which case I might agree and things
like running the quals as the table owner is something which should be
considered (I don't know how the other RDBMS's operate in this regard
offhand- it'd be good to find out).

> 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.

How about "it's in high demand by our user base"? In particular, it's
being asked for by a *highly* technical section of our user base who
uses these capabilities today, with this design, in those other
databases.

> 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.

Improving the patch is exactly what I'd like to do, but throwing out the
notion that RLS can't be allowed to execute user-defined code is cutting
the legs out of the feature completely- particularly with our system
where users can create all manner of objects in the system with their
own code being run.

> >> 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).

The current approach allows a nearly unlimited level of flexibility,
should the user wish it, by being able to run user-defined code.
Perhaps that would be considered 'one policy', but it could certainly
take under consideration the calling user, the object being queried
(if a function is defined per table, or if we provide a way to get
that information in the function), etc. What it wouldn't require is
the same object to be queried through different object names, which is
what I was principally objecting to. What would it mean to have
mutliple RLS policies for a given object? There would have to be some
criteria to distinguish which one would be applied, yet that can be
handled with the existing design by the user already, if they wish to.

Were we to preclude users from being able to have user-defined functions
called, then there's quite a bit of additional complexity we'd need to
replicate. Per-user policies, per-role policies, a definition of which
one applies when, per-source-IP, per-connection-type (SSL vs. non-SSL),
per-security-label, etc..

> >> 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.

I don't agree with this characterization. This patch specifically
allows filtering the rows returned from the table, and it intentionally
does not allow changing the data.

> 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.

We are already looking at WITH CHECK OPTION-style support, but I
disagree that separate permissions or data changing will ever be a part
of RLS because then it's no longer RLS.

> I find the
> contrary argument - that people will only want X amount and no more -
> simply not plausible.

I'm not sure where you are seeing the requests for this feature from,
but where I have heard them it's been to match what exists in other
RDBMS's which do not have the capabilities that you're describing users
will want- yet RLS is heavily used in those organizations. For the use
cases that I've had in the past, RLS-as-defined would be the feature
that I want for most tables, with views for joins and data-changing
operations.

> 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 see the line quite clearly- RLS is about having a filtering mechanism
and that's it. If it isn't filtering the rows (meaning giving back a
'true' or 'false' result for each row) then it's beyond RLS.

> > 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.

This is not the feeling that the users which I have been working with
have, nor does it match my feelings about this. As mentioned in my
email to Tom just now, having another object to deal with adds
unnecessary complexity and will require application changes potentially
to implement over existing tables.

> >> 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.

I agree that we'll need to make sure we return the correct answer.
There is complexity there, but hopefully we've addressed much or all of
that with what we have in 9.4 and this is just adding a simpler and
often requested way to use that capability without the need to create
and manage another object in the system.

> >> 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.

I'm happy to have that discussion with the users who are asking for this
but in the conversations that I've had to date, updatable s.b. views are
not RLS to them and I have to agree- having to maintain twice as many
objects in the system which have to be named differently and have
permissions which can be distinct from each other (which is something
that could be a *problem* if it isn't intended), must both be updated
when adding or removing columns, etc, makes that solution quite
unappealing.

> > 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.

I'm quite sure you can select back the data from a view in every
graphical client that exists- and without any warning popping up that
you might be running code that someone else wrote. Yes, you can also
get the definition of the view in many cases and you can tell if what
you're selecting is a view or a table but that doesn't mean people are
actively being paranoid about that distinction or worrying about the
other cases where user-defined code might be run, even when selecting
from a table, in general.

> 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.

I'd love to hear how you decide which views are trusted and which are
not. Last I checked, most serious attacks still come from internal
individuals rather than external ones. Don't get me wrong- we
definitely have an issue here that it'd be great to find a solution to,
as has been discussed extensively, but I don't see RLS as making that
problem particularly worse, and really, excluding superusers and having
the option for other users to be excluded goes above what we've done to
date in other areas.

> 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.

I'm glad that you agree that restricting the RLS policy isn't the right
answer. I agree that we want to come up with a way to prevent users
from running code that isn't safe or isn't intended. I still don't see
RLS as making that particularly worse. The system is really nearly
unusable in any interactive way if you restrict yourself to operations
which can't possibly run any user-defined code today. There have been
discussions about ways to possibly improve that, and those ways would
need to address the RLS case in addition to the other already existing
cases but I don't see that as a signifigant increase in the amount of
work required to address that problem (which is already quite large..).

> > 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.

We have this problem with psql today, as has been discussed. The fact
that pg_dump doesn't happen to have this problem is great but it's no
true solution for the problem at hand.

> >> 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.

While RLS could be viewed as providing a subset of what updatable sb
views provide, I can see a clear line between the two and, for my part,
we should allow users to make their own decision about if they want the
complexity involved with maintaining another object in the system to
provide the filtering or if they want to implement the filtering and the
data manipulation, joins, etc, independently.

That's really another big point to be made here- there's value in
separating these concerns. Security is a big enough concern and a big
enough issue that being able to address it explicitly and with a simple
syntax is extremely valuable. RLS as we've been discussing it allows
that, while having to include it in more complicated view definitions
could make it much more difficult to reason about. I suppose one could
define a view for just the filtering and then another view for the data
manipulation and joining over top of the other views, but, again, that
adds another level of complexity that isn't needed- and you can't be
100% sure that the only thing the supposedly filtering view is doing is
*just* filtering unless you audit it regularly.

> >> 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.

Great.

Thanks,

Stephen

On Wed, Jun 11, 2014 at 8:59 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> 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.
>
> In this case the user-defined code needs to return a boolean. We don't
> currently do anything to prevent it from having side-effects, no, but
> the same is true with views which incorporate functions. I agree that
> it makes a difference when compared to column-level privileges, but my
> point was that we have provided easier ways to do things which were
> possible using more complicated methods before. Perhaps the risk with
> RLS is higher but these issues look managable to me and the level of
> doubt about our ability to provide this feature in a reasonable and
> principled way that our users will understand surprises me.

I'm glad the issues look manageable to you, but you haven't really
explained how to manage them. The way to dispel doubt is to come up
with specific technical proposals that address the technical issues
that have been raised. I accept that you are surprised that someone
might not think we are on the right course here, but it's entirely
appropriate for me to express my doubts about this or any other patch,
much as many people do in regards to many patches that are posted here
- generally for good and valid reasons.

For my part, I'm mildly surprised that anyone thinks it's a good idea
to have SELECT * FROM tab to mean different things depending on who is
typing it. To me, that seems very confusing; how does an unprivileged
user with no ability to assume some other role validate that the row
security policy they've configured works at all and exposes precisely
the intended set of rows? Even aside from security exposures, how
does a non-superuser who runs pg_dump know whether they've got a
complete backup or a filtered dump that's missing some rows? A
filtered dump might not even be restorable if foreign keys are
involved. I think those are serious issues that deserve serious
thought and consideration, not just a vague assurance that the issues
are probably manageable.

>> Because we don't have a good design.
>
> We're using a design that's found in multiple other RDBMS's and used
> extensively by certain industries which use those RDBMS's today. I'm
> certainly open to improving what is found in other systems for PG but I
> have a hard time seeing this approach as a bad design. Perhaps you're
> referring to our implementation, in which case I might agree and things
> like running the quals as the table owner is something which should be
> considered (I don't know how the other RDBMS's operate in this regard
> offhand- it'd be good to find out).

I'm not referring to the proposed implementation particularly; or at
least not that aspect of it. I don't think trying to run the view
quals as the defining user is likely to be very appealing, because I
think it's going to hurt performance, for example by preventing
function inlining and requiring lots of user-ID switches. But I'm not
gonna complain if someone wants to mull it over and make a proposal
for how to make it work. Rather, my concern is that all we've got is
what might be called the core of the feature; the actual guts of it.
There are a lot of ancillary details that seem to me to be not worked
out at all yet, or only half-baked.

>> 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.
>
> How about "it's in high demand by our user base"? In particular, it's
> being asked for by a *highly* technical section of our user base who
> uses these capabilities today, with this design, in those other
> databases.

Sure, that's a valid reason for considering any feature. But it's not
an excuse to overlook whatever design problems may exist.

>> 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).
>
> The current approach allows a nearly unlimited level of flexibility,
> should the user wish it, by being able to run user-defined code.
> Perhaps that would be considered 'one policy', but it could certainly
> take under consideration the calling user, the object being queried
> (if a function is defined per table, or if we provide a way to get
> that information in the function), etc.

In theory, that's true. But in practice, performance will suck unless
the security qual is easily optimizable. If your security qual is
WHERE somecomplexfunction() you're going to have to implement that by
sequential-scanning the table and evaluating the function for each
row.

For example, I once worked at a company where we had a table
containing information about our customers and potential customers.
Sales representatives were allowed to see their own accounts, and
partners were allowed to see accounts associated with that partner.
These things were independent. So for a sales rep, the security qual
was WHERE sales_rep_id = <something> and for a partner the security
qual was WHERE partner_id = <something>. Now, you could maybe write
this as a single qual, something like this:

WHERE sales_rep_id = (SELECT oid FROM pg_authid WHERE rolname =
current_user AND oid IN (SELECT id FROM person WHERE is_sales_rep)) OR
partner_id = (SELECT p.org_id FROM pg_authid a, person p WHERE
a.rolname = current_user and a.oid = p.id)

But that's probably not going to perform very well, because to match
an index on sales_rep_id, or an index on partner_id, that's going to
have to get simplified a whole lot, and that's probably not going to
happen. If we've only got one branch of the OR, I think we'll realize
we can evaluate the subquery as an InitPlan and then use an index, but
with two branches I think that will fail.

I don't want to overstate the importance of this particular case; but
I do think scenarios in which it's advantageous to have multiple
row-level security policies are plausible. Another, perhaps-simpler
example is that you might have a table containing unclassified data,
classified data, and secret data. You want to give access to the
unclassified data only to one category of users; access to the
unclassified data and the classified data to a second group of
more-trusted users; and access to all of the data to a third group of
very highly trusted users. If the table can only have one security
policy that applies to everyone who isn't exempt, how will you do
that? This sort of use case seems very plausible to me so I think we
need to give some real thought to what we will recommend to users who
want to do things like this. Can the proposed patch handle it? How?

>> > 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.
>
> I don't agree with this characterization. This patch specifically
> allows filtering the rows returned from the table, and it intentionally
> does not allow changing the data.

I don't know what to say to this. What I said is, quite literally,
what the patch does. It wraps the patch in an subquery RTE that is
precisely the same thing you would get if you defined a
security_barrier view with the security qual in the WHERE clause.
This is not a question of opinion; the patch either does that or it
doesn't, and I think it does.

>> 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.
>
> We are already looking at WITH CHECK OPTION-style support, but I
> disagree that separate permissions or data changing will ever be a part
> of RLS because then it's no longer RLS.

What do you mean by "data changing"? If you mean inserts, updates,
and deletes, I am very sure people are going to want to perform those
operations on RLS-enabled tables.

Do you find it implausible that someone will want to exempt a certain
role from RLS on only one table but not on other tables in the system?
Do you find it implausible that someone will want to allow a certain
table to bypass RLS when selecting rows, but not when updating or
deleting them? I find those scenarios very plausible.

>> 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.
>
> We have this problem with psql today, as has been discussed. The fact
> that pg_dump doesn't happen to have this problem is great but it's no
> true solution for the problem at hand.

It's true that users can break security by being incautious about the
queries they type into psql, and I'm all for having better tools to
manage that. But a feature that causes currently-safe uses of pg_dump
to become unsafe is, in my opinion, absolutely not OK.

I do agree with your argument that things like adding and removing
columns, or changing their data types, could be simpler with RLS than
in the view-over-table model - because in the view-over-table model,
we don't really know whether the user would like a new column to
cascade to the view, whereas in the RLS model, we can automatically do
the right thing.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Even aside from security exposures, how
> does a non-superuser who runs pg_dump know whether they've got a
> complete backup or a filtered dump that's missing some rows?

This seems to me to be a killer objection to the feature as
proposed, and points out a huge difference between column level
security and the proposed implementation of row level security.
(In fact it is a difference between just about any GRANTed
permission and row level security.)  If you try to SELECT * FROM
sometable and you don't have rights to all the columns, you get an
error.  A dump would always either work as expected or generate an
error.

test=# create user bob;
CREATE ROLE
test=# create user bill;
CREATE ROLE
test=# set role bob;
SET
test=> create table person (person_id int not null primary key,
name text not null, ssn text);
CREATE TABLE
test=> grant select (person_id, name) on table person to bill;
GRANT
test=> reset role;
RESET
test=# set role bill;
SET
test=> select person_id, name from person;
 person_id | name
-----------+------
(0 rows)

test=> select * from person;
ERROR:  permission denied for relation person

The proposed approach would leave the validity of any dump which
was not run as a superuser in doubt.  The last thing we need, in
terms of improving security, is another thing you can't do without
connecting as a superuser.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 6/11/14, 10:26 AM, Robert Haas wrote:
> Now, as soon as we introduce the concept that selecting from a table
> might not really mean "read from the table" but "read from the table
> after applying this owner-specified qual", we're opening up a whole
> new set of attack surfaces. Every pg_dump is an opportunity to hack
> somebody else's account, or at least audit their activity.

I'm in full agreement we should clearly communicate the issues around
pg_dump in particular, because they can't necessarily be eliminated
altogether without some major work that's going to take a while to
finish. And if the work-around is some sort of GUC for killing RLS
altogether, that's ugly but not unacceptable to me as a short-term fix.

One of the difficult design requests in my inbox right now asks how
pg_dump might be changed both to reduce its overlap with superuser
permissions and to allow auditing of its activity. Those requests
aren't going away; their incoming frequency is actually rising quite
fast right now. They're both things people expect from serious SQL
oriented commercial database products, and I'd like to see PostgreSQL
continue to displace those as we reach feature parity in those areas.

Any way you implement finer grained user permissions and auditing
features will be considered a new attack vector when you use those
features. The way the proposed RLS feature inserts an arbitrary
function for reads has a similar new attack vector when you use that
feature.

I'm kind of surprised to see this turn into a hot button all of the
sudden though, because my thought on all that so far has been a giant so
what? This is what PostgreSQL does.

You wanna write your own C code and then link the thing right into the
server, so that bugs can expose data and crash the whole server? Not
only can you shoot yourself in the foot that way, we supply a sample gun
and bullets in contrib. How about writing arbitrary code in any one of
a dozen server-side languages of wildly varying quality, then hooking
that code so it runs as a trigger function whenever you change a row?
PostgreSQL is *on it*; we love letting people write some random thing,
and then running that random thing against your data as a side-effect of
doing an operation. And if you like that...just wait until you learn
about this half-assed rules feature we have too!

And when the database breaks because the functions people inserted were
garbage, that's their fault, not a cause for a CVE. And when someone
blindly installs adminpack because it sounded like a pgAdmin
requirement, lets a monitoring system run as root so it can watch
pg_stat_activity, and then discovers that pair of reasonable decisions
suddenly means any fool with monitoring access can call
pg_file_unlink...that's their fault too. These are powerful tools with
serious implications, and they're expected to be used by equally serious
users.

We as a development community do need to put a major amount of work into
refactoring all of these security mechanisms. There should be less of
these embarrassing incidents where bad software design really forced the
insecure thing to happen, which I'd argue is the case for that
pg_stat_activity example. And luckily so far development resources are
appearing for organizations I know of working in that direction
recently, as fast as the requirements are rising. I think there's a
good outcome at the end of that road.

But let's not act like RLS is a scary bogeyman because it introduces a
new way to hack the server or get surprising side-effects. That's
expected and possibly unavoidable behavior in a feature like this, and
there are much worse instances of arbitrary function risk throughout the
core code already.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

Greg, all,

I will reply to the emails in detail when I get a chance but am out of town
at a funeral, so it'll likely be delayed. I did want to echo my agreement
for the most part with Greg and in particular...

On Thursday, June 12, 2014, Gregory Smith <gregsmithpgsql(at)gmail(dot)com> wrote:

> On 6/11/14, 10:26 AM, Robert Haas wrote:
>
>> Now, as soon as we introduce the concept that selecting from a table
>> might not really mean "read from the table" but "read from the table after
>> applying this owner-specified qual", we're opening up a whole new set of
>> attack surfaces. Every pg_dump is an opportunity to hack somebody else's
>> account, or at least audit their activity.
>>
>
> I'm in full agreement we should clearly communicate the issues around
> pg_dump in particular, because they can't necessarily be eliminated
> altogether without some major work that's going to take a while to finish.
> And if the work-around is some sort of GUC for killing RLS altogether,
> that's ugly but not unacceptable to me as a short-term fix.

A GUC which is enable / disable / error-instead may work quiet well, with
error-instead for pg_dump default if people really want it (there would
have to be a way to disable that though, imv).

Note that enable is default in general, disable would be for superuser only
(or on start-up) to disable everything, and error-instead anyone could use
but it would error instead of implementing RLS when querying an RLS-enabled
table.

This approach was suggested by an existing user testing out this RLS
approach, to be fair, but it looks pretty sane to me as a way to address
some of these concerns. Certainly open to other ideas and thoughts though.

Thanks,

Stephen

On 13 June 2014 01:13, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Greg, all,
>
> I will reply to the emails in detail when I get a chance but am out of town
> at a funeral, so it'll likely be delayed. I did want to echo my agreement
> for the most part with Greg and in particular...
>
> On Thursday, June 12, 2014, Gregory Smith <gregsmithpgsql(at)gmail(dot)com> wrote:
>>
>> On 6/11/14, 10:26 AM, Robert Haas wrote:
>>>
>>> Now, as soon as we introduce the concept that selecting from a table
>>> might not really mean "read from the table" but "read from the table after
>>> applying this owner-specified qual", we're opening up a whole new set of
>>> attack surfaces. Every pg_dump is an opportunity to hack somebody else's
>>> account, or at least audit their activity.
>>
>>
>> I'm in full agreement we should clearly communicate the issues around
>> pg_dump in particular, because they can't necessarily be eliminated
>> altogether without some major work that's going to take a while to finish.
>> And if the work-around is some sort of GUC for killing RLS altogether,
>> that's ugly but not unacceptable to me as a short-term fix.
>
>
> A GUC which is enable / disable / error-instead may work quiet well, with
> error-instead for pg_dump default if people really want it (there would have
> to be a way to disable that though, imv).
>
> Note that enable is default in general, disable would be for superuser only
> (or on start-up) to disable everything, and error-instead anyone could use
> but it would error instead of implementing RLS when querying an RLS-enabled
> table.
>
> This approach was suggested by an existing user testing out this RLS
> approach, to be fair, but it looks pretty sane to me as a way to address
> some of these concerns. Certainly open to other ideas and thoughts though.
>

Yeah, I was thinking something like this could work, but I would go
further. Suppose you had separate GRANTable privileges for direct
access to individual tables, bypassing RLS, e.g.

GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name

Combined with the GUC (direct_table_access, say) to request direct
access to all tables. Then with direct_table_access = true/required, a
SELECT from a table would error if the user hadn't been granted the
DIRECT SELECT privilege on all the tables referenced in the query.
Tools like pg_dump would require direct_table_access, but there might
be other levels of access that didn't error out.

I think if I were using RLS, I would definitely want/expect this level
of fine-grained control over permissions on a per-table basis, rather
than the superuser/non-superuser level of control, or having
RLS-exempt users.

Actually, given the fact that the majority of users won't be using
RLS, I would be tempted to invert the above logic and have the new
privilege be for LIMITED access (via RLS quals). So a user granted the
normal SELECT privilege would be able to bypass RLS, but a user only
granted LIMITED SELECT wouldn't.

Regards,
Dean

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com> writes:
> > Through this effort, we have concluded that for RLS the case of
> > invalidating a plan is only necessary when switching between a superuser
> > and a non-superuser. Obviously, re-planning on every role change would be
> > too costly, but this approach should help minimize that cost. As well,
> > there were not any cases outside of this one that were immediately apparent
> > with respect to RLS that would require re-planning on a per userid basis.
>
> Hm ... I'm not following why we'd need a special case for superusers and
> not anyone else? Seems like any useful RLS scheme is going to require
> more privilege levels than just superuser and not-superuser.

Just to clarify this- the proposal allows RLS to be implemented
essentially by any user-defined qual, where that qual can include the
current user, the IP the user is connecting from, or more-or-less
anything else, possibly even via a user-defined function or security
module. It is not superuser-or-not. This discussion is about how to
support users for which RLS should not be applied. I can see that being
useful at a more granular level than superuser-or-not, but even at that
level, RLS is still extremely useful.

> Could we put the "if superuser then ok" test into the RLS condition test
> and thereby not need more than one plan at all?

As discussed, that unfortunately doesn't quite work.

This discussion, in general, has been quite useful and I'll work on
adding documentation to the wiki pages which discusses the consideration
and suggestions for a GUC to disable-or-error when RLS is encountered,
along with a per-role capability to bypass RLS; that is in line with the
goal of avoiding adding superuser-specific capabilities.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> > I agree, and now that the urgency of trying to deliver this for 9.4 is
> > over it's worth seeing if we can just run as table owner.
>
> > Failing that, we could take the approach a certain other RDBMS does and
> > make the ability to define row security quals a GRANTable right
> > initially held only by the superuser.
>
> Hmm ... that might be a workable compromise. I think the main issue here
> is whether we expect that RLS quals will be something that the planner
> could optimize to any meaningful extent. If they're always (in effect)
> wrapped in SECURITY DEFINER functions, I think that largely blocks any
> optimizations; but maybe that wouldn't matter in practice.

From what I've heard from actual users with other RDBMS's who are coming
to PostgreSQL- the reality is that they're going to be using a security
module (eg: SELinux) whose responsibility it is to manage this whole
question of "can this user see this row", meaning there's zero chance of
optimization.

I'd certainly like to see the ability to optimize remain in cases where
the qual itself gives us a way to filter (eg: a table partitioned based
on some security level, where another table maps users to levels), but
that is, from a practical standpoint, not an immediate concern from real
users and I don't believe our approach paints us into a corner which
would prevent that. What that would require is better support for true
partitioning rather than constraint exclusions.

Thanks,

Stephen

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jun 11, 2014 at 8:59 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > In this case the user-defined code needs to return a boolean. We don't
> > currently do anything to prevent it from having side-effects, no, but
> > the same is true with views which incorporate functions. I agree that
> > it makes a difference when compared to column-level privileges, but my
> > point was that we have provided easier ways to do things which were
> > possible using more complicated methods before. Perhaps the risk with
> > RLS is higher but these issues look managable to me and the level of
> > doubt about our ability to provide this feature in a reasonable and
> > principled way that our users will understand surprises me.
>
> I'm glad the issues look manageable to you, but you haven't really
> explained how to manage them.

There's been a number of suggestions made and it'd be great to get more
feedback on them- running the quals as the table owner, having a GUC
which can be set to either run 'as normal' or either ignore RLS (if the
user has that right) or error out if RLS would happen, and undoubtably
there are other ideas along those same lines to address the pg_dump and
other concerns.

> For my part, I'm mildly surprised that anyone thinks it's a good idea
> to have SELECT * FROM tab to mean different things depending on who is
> typing it.

Realistically, in the RDBMS realm in which we're in and that we're
working to break into- this is absolutely a given and expected. It's
new to PostgreSQL, certainly, but it's not uncommon or surprising at all
in our industry.

> To me, that seems very confusing; how does an unprivileged
> user with no ability to assume some other role validate that the row
> security policy they've configured works at all and exposes precisely
> the intended set of rows?

While I see what you're getting at, I'm not convinced it's really all
that different from being set up without access to some schema or table
which the administrator setting up accounts didn't include for you.
Sure, in the case of a schema or table, you can get an error back
instead of just not seeing the data, but if you're looking for specific
data, chances are pretty good you'll realize the lack of data quickly
and ask the same question regarding access.

To wit, I've certainly had users ask exactly that question of- "do I
have access to all the data in this table?" even when using PG where
it's a bit tricky to limit such access. Clearly, the same risk applies
when using views and so the question is understandable. Perhaps these
were users with more experience in other RDBMS's where it's more common
to have RLS, but there are at least a couple cases which I can think of
where that wouldn't apply.

> Even aside from security exposures, how
> does a non-superuser who runs pg_dump know whether they've got a
> complete backup or a filtered dump that's missing some rows?

This would be addressed with the GUC that's been proposed. As would the
previous paragraph, though I wanted to apply to that independently.

> I'm not referring to the proposed implementation particularly; or at
> least not that aspect of it. I don't think trying to run the view
> quals as the defining user is likely to be very appealing, because I
> think it's going to hurt performance, for example by preventing
> function inlining and requiring lots of user-ID switches.

I understand that there are performance implications. As mentioned to
Tom, realistically, there's zero way to optimized at least some of these
use-cases because they require a completely external module (eg:
SELlinux) to be involved in the decision about who can view what
records. If we can optimize that, it'd be by a completely different
approach whereby we pull up the qual higher because we know the whole
query only involves leakproof functions or similar, allowing us to only
apply the filter to the final set of records prior to them being
returned to the user. The point being that such optimizations would
happen independently and regardless of the quals or user-defined
functions involved. At the end of the day, I can't think of a better
optimization for such a case (where we have to ask an external security
module if a row is acceptable to return to the user) than that. Is
there something specific you're thinking about that we'd be missing out
on?

> But I'm not
> gonna complain if someone wants to mull it over and make a proposal
> for how to make it work. Rather, my concern is that all we've got is
> what might be called the core of the feature; the actual guts of it.
> There are a lot of ancillary details that seem to me to be not worked
> out at all yet, or only half-baked.

Perhaps it's just my experience, but I've been focused on the main core
feature for quite some time and it feels like we're really close to
having it there. I agree that a few additional bits would be nice to
have but these strike me as relatively straight-forward to implement
overtop of this general construct. I do see value in documenting these
concerns and will see about making that happen, along with what the
general viewpoints and thoughts are about how to address the concern.

> > How about "it's in high demand by our user base"? In particular, it's
> > being asked for by a *highly* technical section of our user base who
> > uses these capabilities today, with this design, in those other
> > databases.
>
> Sure, that's a valid reason for considering any feature. But it's not
> an excuse to overlook whatever design problems may exist.

Agreed- improvements in the design, provided it continues to meet the
expectations of the user-base, are absolutely welcome.

> > The current approach allows a nearly unlimited level of flexibility,
> > should the user wish it, by being able to run user-defined code.
> > Perhaps that would be considered 'one policy', but it could certainly
> > take under consideration the calling user, the object being queried
> > (if a function is defined per table, or if we provide a way to get
> > that information in the function), etc.
>
> In theory, that's true. But in practice, performance will suck unless
> the security qual is easily optimizable. If your security qual is
> WHERE somecomplexfunction() you're going to have to implement that by
> sequential-scanning the table and evaluating the function for each
> row.

That's not actualy true today, is it? Given our leak-proof attribute,
if the qual is "WHERE somecomplexfunction() AND leakprooffunctionx()"
then we would be able to push down the leak-proof function and not
necessairly run a straight sequential scan, no? Even so, though, we've
had users who have tested exactly what this patch implements and they've
been happy with their real-world use-cases. I'm certainly all for
optimization and would love to see us make this better for everyone, but
I don't view that as a reason to delay this particular feature which is
really just bringing us up to parity with other RDMBS's.

> For example, I once worked at a company where we had a table
> containing information about our customers and potential customers.
> Sales representatives were allowed to see their own accounts, and
> partners were allowed to see accounts associated with that partner.
> These things were independent. So for a sales rep, the security qual
> was WHERE sales_rep_id = <something> and for a partner the security
> qual was WHERE partner_id = <something>. Now, you could maybe write
> this as a single qual, something like this:
>
> WHERE sales_rep_id = (SELECT oid FROM pg_authid WHERE rolname =
> current_user AND oid IN (SELECT id FROM person WHERE is_sales_rep)) OR
> partner_id = (SELECT p.org_id FROM pg_authid a, person p WHERE
> a.rolname = current_user and a.oid = p.id)

That looks like it'd work, or a pl/pgsql function which did the same.

> But that's probably not going to perform very well, because to match
> an index on sales_rep_id, or an index on partner_id, that's going to
> have to get simplified a whole lot, and that's probably not going to
> happen. If we've only got one branch of the OR, I think we'll realize
> we can evaluate the subquery as an InitPlan and then use an index, but
> with two branches I think that will fail.

You're right- we could perform better in such a case.

What solution did you come up with for this case, which performed well
and was also secure..?

> I don't want to overstate the importance of this particular case; but
> I do think scenarios in which it's advantageous to have multiple
> row-level security policies are plausible.

I'm not against this in general. The question, in my mind, is what
level of granularity we would provide this at. As I tried to outline
previously, there's a huge number of combinations which we could come up
with to support this under and I'm not 100% sure that it'd actualy end
up being better than the simplicity of a single qual where the user gets
to define any kind of relationship they want between the various
policies; even programatically if they want.

> Another, perhaps-simpler
> example is that you might have a table containing unclassified data,
> classified data, and secret data. You want to give access to the
> unclassified data only to one category of users; access to the
> unclassified data and the classified data to a second group of
> more-trusted users; and access to all of the data to a third group of
> very highly trusted users. If the table can only have one security
> policy that applies to everyone who isn't exempt, how will you do
> that? This sort of use case seems very plausible to me so I think we
> need to give some real thought to what we will recommend to users who
> want to do things like this. Can the proposed patch handle it? How?

There are multiple ways this could be implemented- the first, basic, way
would be through a table which maps users to security levels via an enum
where more privileged levels are higher in value and therefore a simple
greater-than could be applied after a join which would implement this
particular policy.

The reality (which I've had discussions with users about..) is actually
much more complicated where an extermal security module makes the
decision about if a given user/connection can have access to a specific
bit of labeled data. The reason is that things are not classified so
simply as "unclass", "class" and "secret" but rather into much more
granular pieces- user X might have access to A and B, but not C, while
user Y can access B and C. The absolute levels described above may
exist for less sensetive data but for data beyond that (which I'd hazard
to guess is most of it...), more granularity and control is needed.

> >> What this patch is doing is basically allowing a table to really be a
> >> view over itself.
> >
> > I don't agree with this characterization. This patch specifically
> > allows filtering the rows returned from the table, and it intentionally
> > does not allow changing the data.
>
> I don't know what to say to this. What I said is, quite literally,
> what the patch does. It wraps the patch in an subquery RTE that is
> precisely the same thing you would get if you defined a
> security_barrier view with the security qual in the WHERE clause.

Exactly- it does *not* allow changing the SELECT clause, or adding in a
GROUP BY, or a JOIN, or tossing in a windowing function, etc.

> This is not a question of opinion; the patch either does that or it
> doesn't, and I think it does.

Apologies for not being clearer but my point was that only the WHERE
clause can be modified by this patch, which is quite intentional. This
separates the concerns of "can I access this data" from "modify the data
to represent it in X way".

> > We are already looking at WITH CHECK OPTION-style support, but I
> > disagree that separate permissions or data changing will ever be a part
> > of RLS because then it's no longer RLS.
>
> What do you mean by "data changing"? If you mean inserts, updates,
> and deletes, I am very sure people are going to want to perform those
> operations on RLS-enabled tables.

Yes, they'll want to support those operations. However, they will not
expect RLS to allow them to redefine a columns as "x+10" instead of "x",
which a view does allow.

> Do you find it implausible that someone will want to exempt a certain
> role from RLS on only one table but not on other tables in the system?

No- excempting certain roles from RLS makes sense as a capability.

> Do you find it implausible that someone will want to allow a certain
> table to bypass RLS when selecting rows, but not when updating or
> deleting them? I find those scenarios very plausible.

This is also plausible and something which we were anticipating while
developing this patch. Simon, KaiGai and I specifically discussed
addressing SELECT vs UPDATE/DELETE earlier this year, as I recall.
Providing that level of flexibility is absolutely on the road map, but I
don't know that it all has to exist in 9.5; it may, which would be
great, but I don't view it as required.

> > We have this problem with psql today, as has been discussed. The fact
> > that pg_dump doesn't happen to have this problem is great but it's no
> > true solution for the problem at hand.
>
> It's true that users can break security by being incautious about the
> queries they type into psql, and I'm all for having better tools to
> manage that. But a feature that causes currently-safe uses of pg_dump
> to become unsafe is, in my opinion, absolutely not OK.

I don't particularly like it, and would require a way to override it,
but a GUC which pg_dump sets by default that says "give me everything or
error" would work to address this. I'm open to other thoughts, of
course, but it does seem like a relatively simple solution (which is a
good thing when it comes to security concerns, imv).

> I do agree with your argument that things like adding and removing
> columns, or changing their data types, could be simpler with RLS than
> in the view-over-table model - because in the view-over-table model,
> we don't really know whether the user would like a new column to
> cascade to the view, whereas in the RLS model, we can automatically do
> the right thing.

Agreed.

Thanks,

Stephen

Kevin,

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Even aside from security exposures, how
> > does a non-superuser who runs pg_dump know whether they've got a
> > complete backup or a filtered dump that's missing some rows?
>
> This seems to me to be a killer objection to the feature as
> proposed, and points out a huge difference between column level
> security and the proposed implementation of row level security.

I really hate this notion of "killer objection". It's been discussed
(perhaps not seen by all) at least one suggestion for how to address
this specific issue and there are other ways in which to address it
(having COPY have the same behavior as the GUC being discussed, instead
of having a GUC, though I feel like the GUC is a better approach..).

> (In fact it is a difference between just about any GRANTed
> permission and row level security.)  If you try to SELECT * FROM
> sometable and you don't have rights to all the columns, you get an
> error.  A dump would always either work as expected or generate an
> error.

Provided you know all of the tables and other objects which need to be
included in such a partial dump (as a full dump, today, must be run by a
superuser to be sure you're actually getting everything anyway...).

> The proposed approach would leave the validity of any dump which
> was not run as a superuser in doubt.  The last thing we need, in
> terms of improving security, is another thing you can't do without
> connecting as a superuser.

Any dump not run by a superuser is already in doubt, imv. That is a
problem we already have which really needs to be addressed, but I view
that as an independent issue.

I agree with avoiding adding another superuser-only capability; see the
other sub-thread about making this a per-user capability.

Thanks,

Stephen

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 13 June 2014 01:13, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > This approach was suggested by an existing user testing out this RLS
> > approach, to be fair, but it looks pretty sane to me as a way to address
> > some of these concerns. Certainly open to other ideas and thoughts though.
>
> Yeah, I was thinking something like this could work, but I would go
> further. Suppose you had separate GRANTable privileges for direct
> access to individual tables, bypassing RLS, e.g.
>
> GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name

This is certainly an interesting idea and I'm glad we're getting this
level of discussion early on in the 9.5 cycle as I'd really like to see
a good solution implemented for 9.5.

I've been going back-and-forth about this and what's really swaying me
right now is that it'd be nearly impossible to determine if a given RLS
qual actually allows full access to a table for a given user without
going through the entire table and testing the qual against each row.
With this GRANT ability, we'd be able to completely avoid calling the
RLS quals when the user is granted this right.

Not sure offhand how many bits we've got left at the per-table level
though; we added TRUNCATE rights not that long ago and this looks like
another good right to add, but there are only so many bits available..
At the same time, I do think this is something we could also add later,
perhaps after figuring out a good way to extend the set of bits
available for privileges on tables.

> Combined with the GUC (direct_table_access, say) to request direct
> access to all tables. Then with direct_table_access = true/required, a
> SELECT from a table would error if the user hadn't been granted the
> DIRECT SELECT privilege on all the tables referenced in the query.

I can see this working. One thing I'm curious about is if we would want
to support this inside of the SELECT statement (or perhaps COPY?)
directly, rather than making a user have to flip a GUC back and forth
while they're doing something. I can imagine, during testing, a session
looking like this:

select * from table;
@#(at)!$!
set direct_table_access = true;
select * from table;
select * from table where blah = x;
alter table set row level security blah = x;
select * from table;
select * from table;
select * from table;
@!#$!(at)#!
set direct_table_access = false;
select * from table;
...

Would 'select direct' or 'select * from DIRECT table' (or maybe 'ONLY'?)
be workable? There's certainly SQL standard concerns to be thought of
here which might precldue anything we do with SELECT, but we could
support something with COPY.

> Tools like pg_dump would require direct_table_access, but there might
> be other levels of access that didn't error out.

pg_dump would need an option to set direct_table_access or not. Having
it ask by default is acceptable to me, but I do think we need to be able
to tell it to *not* set that.

> I think if I were using RLS, I would definitely want/expect this level
> of fine-grained control over permissions on a per-table basis, rather
> than the superuser/non-superuser level of control, or having
> RLS-exempt users.

I agree that it'd be great to have- and we need to make sure we don't
paint ourselves into a corner with the initial versions. What I'm
worried about is that we're going to end up feature-creeping this to
death and ending up with nothing in 9.5. I'll try to get a wiki page
going to discuss these items (as mentioned up-thread) and we can look at
prioritizing them and looking at what dependencies exist on other parts
of the system and seeing what's required for the initial version.

> Actually, given the fact that the majority of users won't be using
> RLS, I would be tempted to invert the above logic and have the new
> privilege be for LIMITED access (via RLS quals). So a user granted the
> normal SELECT privilege would be able to bypass RLS, but a user only
> granted LIMITED SELECT wouldn't.

This I don't agree with- it goes against what is done on existing
systems afaik and part of the idea is that you can minimize changes to
the applications or users but still be able to curtail what they can
see. Making regular SELECTs start erroring if they haven't set some GUC
because RLS has been implemented on a given table would be quite
annoying, imv.

Now, that said, wouldn't the end user be able to control this for their
particular environment by setting the GUC accordingly in
postgresql.conf? I'd still argue that it should be defaulted to what I
view as the 'normal' case, where RLS is applied unless you asked for
your queries to error instead, but if a user wants to have it flipped
around the other way, they could update their postgresql.conf to make it
so.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:

>> The proposed approach would leave the validity of any dump which
>> was not run as a superuser in doubt.  The last thing we need, in
>> terms of improving security, is another thing you can't do
>> without connecting as a superuser.
>
> Any dump not run by a superuser is already in doubt, imv.  That
> is a problem we already have which really needs to be addressed,
> but I view that as an independent issue.

I'm not seeing that.  If the user can't dump, you get an error and
pg_dump returns something other than SUCCESS.

test=# create user bob;
CREATE ROLE
test=# create user tom;
CREATE ROLE
test=# set role bob;
SET
test=> create table person(person_id int primary key, name text not null, ssn text);
CREATE TABLE
test=> insert into person values (1, 'Stephen Frost', '123-45-6789');
INSERT 0 1
test=> insert into person values (2, 'Kevin Grittner');
INSERT 0 1
test=> grant select (person_id, name) on person to tom;
GRANT
test=> \q
kgrittn(at)Kevin-Desktop:~/pg/master$ pg_dump -U bob test >bob-backup.sql
kgrittn(at)Kevin-Desktop:~/pg/master$ pg_dump -U tom test >tom-backup.sql
pg_dump: [archiver (db)] query failed: ERROR:  permission denied for relation person
pg_dump: [archiver (db)] query was: LOCK TABLE public.person IN ACCESS SHARE MODE
kgrittn(at)Kevin-Desktop:~/pg/master$ echo $?
1

> I agree with avoiding adding another superuser-only capability;
> see the other sub-thread about making this a per-user capability.

It should be possible to design something which does not have this
risk.  What I was saying was that what was being described at that
point wasn't it, and IMV was not acceptable.  I think that there
should never by any doubt that a pg_dump run which completes
without error copied all requested tables in their entirety, not a
subset of the rows in the tables.

A GUC which only caused an error on the attempt to actually read
specific rows which the user does not have permission to see would
leak too much information.  A GUC which caused a SELECT or COPY
from a table to throw an error if the user was not entitled to see
all rows in the table could work.  Another thing which could work,
if it can be coded, would be a GUC which would throw an error if
the there were not quals on the query to prohibit seeing rows which
the security conditions would prohibit, whether or not any matching
rows actually existed.  The latter would match the behavior of
column level security -- you get an error when trying to select a
prohibited column even if there are no rows in the table.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Any dump not run by a superuser is already in doubt, imv.  That
> > is a problem we already have which really needs to be addressed,
> > but I view that as an independent issue.
>
> I'm not seeing that.  If the user can't dump, you get an error and
> pg_dump returns something other than SUCCESS.

We've outlined an approach with RLS which would do the same.

I'm still of the opinion that, today, we have a problem that only a
superuser-run dump has any chance of success (and even if you get it
working today it'll probably break tomorrow, and you had better be
paying attention). I'd like to fix that situation, but it's an
independent effort from this. We've had issues in the past with pg_dump
creating things that can't be restored and they're certainly bugs but
trying to make that work with a regular user as a whole system backup
strategy, today, is just asking for trouble.

> > I agree with avoiding adding another superuser-only capability;
> > see the other sub-thread about making this a per-user capability.
>
> It should be possible to design something which does not have this
> risk. 

The risk that pg_dump might create a dump which can't be restored?
Agreed, and I'd love to hear your thoughts on the proposal.

> What I was saying was that what was being described at that
> point wasn't it, and IMV was not acceptable.  I think that there
> should never by any doubt that a pg_dump run which completes
> without error copied all requested tables in their entirety, not a
> subset of the rows in the tables.

pg_dump needs to be able to have an option to go either way on this
case, as I can see value in running pg_dump in "RLS-enforcing" mode, but
it could default to "error-if-RLS".

> A GUC which only caused an error on the attempt to actually read
> specific rows which the user does not have permission to see would
> leak too much information.  A GUC which caused a SELECT or COPY
> from a table to throw an error if the user was not entitled to see
> all rows in the table could work.

Right- this would be the 'DIRECT SELECT' which would allow bypassing all
RLS and therefore mean that the user is allowed to see ALL rows of a
table. That's one of the reasons why I agree with Dean's approach,
because we really need to know at the outset if the calling user is
allowed to extract all rows from a table or not- we can't go looking
through the entire table testing each row before we start running the
query.

>   Another thing which could work,
> if it can be coded, would be a GUC which would throw an error if
> the there were not quals on the query to prohibit seeing rows which
> the security conditions would prohibit, whether or not any matching
> rows actually existed. 

If I'm following you correctly, this would be an optimization that
allows avoiding RLS in the case where some information about the user
causes the overall qual to always return 'true', correct? I'd certainly
like to see what happens in that case today and agree that it'd be great
to optimize for and perhaps even allow a user for which that is true to
not need the 'DIRECT SELECT' privilege, but in practice, I don't think
it'll be possible in most cases (certainly not in the case where an
external security module is deciding the access) and the optimization
may not be worth it.

> The latter would match the behavior of
> column level security -- you get an error when trying to select a
> prohibited column even if there are no rows in the table.

Agreed, but that would be a relaxation of the proposed approach and
therefore something which could be added later, if it's deemed
worthwhile.

Thanks,

Stephen

On Thu, Jun 12, 2014 at 6:33 PM, Gregory Smith <gregsmithpgsql(at)gmail(dot)com> wrote:
> I'm kind of surprised to see this turn into a hot button all of the sudden
> though, because my thought on all that so far has been a giant so what?
> This is what PostgreSQL does.
[...]
> But let's not act like RLS is a scary bogeyman because it introduces a new
> way to hack the server or get surprising side-effects. That's expected and
> possibly unavoidable behavior in a feature like this, and there are much
> worse instances of arbitrary function risk throughout the core code already.

I have some technical comments on later emails in this thread, but
first let me address this point. In the past, people have sometimes
complained that reviewers waited until very late in the cycle to
complain about issues which they found problematic. By the time the
issues were pointed out, insufficient time remained before feature
freeze to get those issues addressed, causing the patch to slip out of
the release and provoking developer frustration. It has therefore
been requested numerous times by numerous people that potential issues
be raised as early as possible.

The concerns that I have raised in this thread are not new; I have
raised them before. However, we are now at the beginning of a new
development cycle, and it seems fair to assume that the people who are
working on this patch are hoping very much that something will get
committed to 9.5. Since it seems to me that there are unaddressed
issues with the design of this patch, I felt that it was a good idea
to make sure that those concerns were on the table right from the
beginning of the process, rather than waiting until the patch was on
the verge of commit or, indeed, already committed. That is why, when
this thread was revived on June 10th, I decide that it was a good time
to again comment on the design points about which I was concerned.

After sending that one (1) email, I was promptly told that "I'm very
disappointed to hear that the mechanical pieces around making RLS easy
for users to use ... is receiving such push-back." The push-back, at
that point in time, consisted of one (1) email. Several more emails
have been sent that time, including the above-quoted text, seeming to
me to imply that the people who are concerned about this feature are
being unreasonable. I don't believe I am the only such person,
although I may be the main one right at the moment, and you may not be
entirely surprised to hear that I don't think I'm being unreasonable.

I will admit that my initial email may have contained just a touch of
hyperbole. But I won't admit to more than a touch, and frankly, I
think it was warranted. I perfectly well understand that people
really, really, really want this feature, and if I hadn't understood
that before, I certainly understand it now. However, I believe that
there has been a lack of focus in the development of the patch thus
far in a couple of key areas - first in terms of articulating how it
is different from and better than a writeable security barrier view,
and second on how to manage the security and operational aspects of
having a feature like this. I think that the discussion subsequent to
my June 10th email has let to some good discussion on both points,
which was my intent, but I still think much more time and thought
needs to be spent on those issues if we are to have a feature which is
up to our usual standards. I do apologize to anyone who interpreted
that initial as a pure rant, because it really wasn't intended that
way. Contrariwise, I hope that the people defending this patch will
admit that the issues I am raising are real and focus on whether and
how those concerns can be addressed.

Thanks,

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

On Thu, Jun 12, 2014 at 8:13 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> I'm in full agreement we should clearly communicate the issues around
>> pg_dump in particular, because they can't necessarily be eliminated
>> altogether without some major work that's going to take a while to finish.
>> And if the work-around is some sort of GUC for killing RLS altogether,
>> that's ugly but not unacceptable to me as a short-term fix.
>
> A GUC which is enable / disable / error-instead may work quiet well, with
> error-instead for pg_dump default if people really want it (there would have
> to be a way to disable that though, imv).
>
> Note that enable is default in general, disable would be for superuser only
> (or on start-up) to disable everything, and error-instead anyone could use
> but it would error instead of implementing RLS when querying an RLS-enabled
> table.
>
> This approach was suggested by an existing user testing out this RLS
> approach, to be fair, but it looks pretty sane to me as a way to address
> some of these concerns. Certainly open to other ideas and thoughts though.

In general, I agree that this is a good approach. I think it will be
difficult to have a GUC with three values, one of which is
superuser-only and the other two of which are not. I don't think
there's any precedent for something like that in the existing
framework, and I think it's likely we'll run into unpleasant corner
cases if we try to graft it in. Also, I think we need to separate
things: whether the system is willing to allow the user to access the
table without RLS, and whether the user is willing to accept RLS if
the system deems it necessary.

For the first one, two solutions have been proposed. The initial
proposal was to insist on RLS except for the superuser (and maybe the
table owner?). Having a separate grantable privilege, as Dean
suggests, may be better. I'll reply separately to that email also, as
I have a question about what he's proposing.

For the second one, I think the two most useful behaviors are "normal
mode" - i.e. allow access to the table, applying RLS predicates if
required and not applying them if I am exempt - and "error-instead"
mode - i.e. if my access to this table would be mediated by an RLS
predicate, then throw an error instead. There's a third mode which
might be useful as well, which is "even though I have the *right* to
bypass the RLS predicate on this table, please apply the predicate
anyway". This could be used by the table owner in testing, for
example. Here again, the level of granularity we want to provide is
an interesting question. Having a GUC (e.g. enable_row_level_security
= on, off, force) would be adequate for pg_dump, but allowing the
table name to be qualified in the query, as proposed downthread, would
be more granular, admittedly at some parser cost. I'm personally of
the view that we *at least* need the GUC, because that seems like the
best way to secure pg_dump, and perhaps other applications. We can
and should give pg_dump an--allow-row-level-security flag, I think,
but pg_dump's default behavior should be to configure the system in
such a way that the dump will fail rather than complete with a subset
of the data. I'm less sure whether we should have something that can
be used to qualify table names in particular queries. I think it
would be really useful, but I'm concerned that it will require
creating additional fully-reserved keywords, which are somewhat
painful for users.

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

Robert,

On Tuesday, June 17, 2014, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> After sending that one (1) email, I was promptly told that "I'm very
> disappointed to hear that the mechanical pieces around making RLS easy
> for users to use ... is receiving such push-back." The push-back, at
> that point in time, consisted of one (1) email. Several more emails
> have been sent that time, including the above-quoted text, seeming to
> me to imply that the people who are concerned about this feature are
> being unreasonable. I don't believe I am the only such person,
> although I may be the main one right at the moment, and you may not be
> entirely surprised to hear that I don't think I'm being unreasonable.

I'm on my phone at the moment but that looks like a quote from me. My email
and concern there was regarding the specific suggestion that we could check
off the "RLS" capability which users have been asking us to provide nearly
since I started with PG by saying that they could use Updatable SB views. I
did not intend it as a comment regarding the specific technical concerns
raised and have been responding to and trying to address those
independently and openly.

I've expressed elsewhere on this thread my gratitude that the technical
concerns are being brought up now, near the beginning of the cycle, so we
can address them. I've been working with others who are interested in RLS
on a wiki page to outline and understand the options and identify
dependencies and priorities. Hopefully the link will be posted shortly
(again, not at a computer right now) and we can get comments back. There
are some very specific questions which really need to be addressed and
which I've mentioned before (in particular the question of what user the
functions in a view definition should run as, both for "normal" views, for
SB views, and for when an RLS qual is included and run through that
framework, and if doing so would address some of the concerns which have
been raised regarding selects running code).

Thanks,

Stephen

On Fri, Jun 13, 2014 at 3:11 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Yeah, I was thinking something like this could work, but I would go
> further. Suppose you had separate GRANTable privileges for direct
> access to individual tables, bypassing RLS, e.g.
>
> GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name

So, is this one new privilege (DIRECT) or four separate new privileges
that are variants of the existing privileges (DIRECT SELECT, DIRECT
INSERT, DIRECT UPDATE, DIRECT DELETE)?

> Actually, given the fact that the majority of users won't be using
> RLS, I would be tempted to invert the above logic and have the new
> privilege be for LIMITED access (via RLS quals). So a user granted the
> normal SELECT privilege would be able to bypass RLS, but a user only
> granted LIMITED SELECT wouldn't.

Well, for the people who are not using RLS, there's no difference
anyway. I think it matters more what users of RLS will expect from a
command like GRANT SELECT ... and I'm guessing they'll prefer that RLS
always apply unless they very specifically grant the right for RLS to
not apply. I might be wrong, though.

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

On Mon, Jun 16, 2014 at 1:15 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> I'm not referring to the proposed implementation particularly; or at
>> least not that aspect of it. I don't think trying to run the view
>> quals as the defining user is likely to be very appealing, because I
>> think it's going to hurt performance, for example by preventing
>> function inlining and requiring lots of user-ID switches.
>
> I understand that there are performance implications. As mentioned to
> Tom, realistically, there's zero way to optimized at least some of these
> use-cases because they require a completely external module (eg:
> SELlinux) to be involved in the decision about who can view what
> records. If we can optimize that, it'd be by a completely different
> approach whereby we pull up the qual higher because we know the whole
> query only involves leakproof functions or similar, allowing us to only
> apply the filter to the final set of records prior to them being
> returned to the user. The point being that such optimizations would
> happen independently and regardless of the quals or user-defined
> functions involved. At the end of the day, I can't think of a better
> optimization for such a case (where we have to ask an external security
> module if a row is acceptable to return to the user) than that. Is
> there something specific you're thinking about that we'd be missing out
> on?

Yeah, if we have to ask an external security module a question for
each row, there's little hope of any real optimization. However, I
think there will be a significant number of cases where people will
want filtering clauses that can be realized by doing an index scan
instead of a sequential scan, and if we end up forcing a sequential
scan anyway, the feature will be useless to those people.

>> But I'm not
>> gonna complain if someone wants to mull it over and make a proposal
>> for how to make it work. Rather, my concern is that all we've got is
>> what might be called the core of the feature; the actual guts of it.
>> There are a lot of ancillary details that seem to me to be not worked
>> out at all yet, or only half-baked.
>
> Perhaps it's just my experience, but I've been focused on the main core
> feature for quite some time and it feels like we're really close to
> having it there. I agree that a few additional bits would be nice to
> have but these strike me as relatively straight-forward to implement
> overtop of this general construct. I do see value in documenting these
> concerns and will see about making that happen, along with what the
> general viewpoints and thoughts are about how to address the concern.

I feel like there's quite a bit of work left to do around these
issues. The technical bits may not be too hard, but deciding what we
want will take some thought and discussion.

>> > The current approach allows a nearly unlimited level of flexibility,
>> > should the user wish it, by being able to run user-defined code.
>> > Perhaps that would be considered 'one policy', but it could certainly
>> > take under consideration the calling user, the object being queried
>> > (if a function is defined per table, or if we provide a way to get
>> > that information in the function), etc.
>>
>> In theory, that's true. But in practice, performance will suck unless
>> the security qual is easily optimizable. If your security qual is
>> WHERE somecomplexfunction() you're going to have to implement that by
>> sequential-scanning the table and evaluating the function for each
>> row.
>
> That's not actualy true today, is it? Given our leak-proof attribute,
> if the qual is "WHERE somecomplexfunction() AND leakprooffunctionx()"
> then we would be able to push down the leak-proof function and not
> necessairly run a straight sequential scan, no? Even so, though, we've
> had users who have tested exactly what this patch implements and they've
> been happy with their real-world use-cases. I'm certainly all for
> optimization and would love to see us make this better for everyone, but
> I don't view that as a reason to delay this particular feature which is
> really just bringing us up to parity with other RDMBS's.

I'm a bit confused here, because your example seems to be totally
different from my example. In my example, somecomplexfunction() will
get pushed down because it's the security qual; that needs to be
inside the security_barrier view, or a malicious user can subvert the
system by getting some other qual evaluated first. In your example,
you seem to be imagining WHERE somecomplexfunction() AND
leakprooffunctionx() as queries sent by the untrusted user, in which
case, yet, the leak-proof one will get pushed down and the other one
will not.

>> But that's probably not going to perform very well, because to match
>> an index on sales_rep_id, or an index on partner_id, that's going to
>> have to get simplified a whole lot, and that's probably not going to
>> happen. If we've only got one branch of the OR, I think we'll realize
>> we can evaluate the subquery as an InitPlan and then use an index, but
>> with two branches I think that will fail.
>
> You're right- we could perform better in such a case.
>
> What solution did you come up with for this case, which performed well
> and was also secure..?

I put the logic in the client. :-(

>> I don't want to overstate the importance of this particular case; but
>> I do think scenarios in which it's advantageous to have multiple
>> row-level security policies are plausible.
>
> I'm not against this in general. The question, in my mind, is what
> level of granularity we would provide this at. As I tried to outline
> previously, there's a huge number of combinations which we could come up
> with to support this under and I'm not 100% sure that it'd actualy end
> up being better than the simplicity of a single qual where the user gets
> to define any kind of relationship they want between the various
> policies; even programatically if they want.

I agree. That's why I think we need some more design work in this
area. Perhaps it's OK to allow only one RLS-qual per table at most,
and tell people that if you want more than that, you need to use
security-barrier views as wrappers instead. But I'm not sure; that
feels like it's giving something up that might be important. And I
think that the kinds of syntax we're discussing won't support leaving
that out of the initial version and adding it later, so if we commit
to this syntax, we're stuck with that behavior. To avoid that, we'd
need something like this:

ALTER TABLE tab ADD POLICY polname WHERE quals;
GRANT SELECT (polname) ON TABLE tab TO role;

>> Another, perhaps-simpler
>> example is that you might have a table containing unclassified data,
>> classified data, and secret data. You want to give access to the
>> unclassified data only to one category of users; access to the
>> unclassified data and the classified data to a second group of
>> more-trusted users; and access to all of the data to a third group of
>> very highly trusted users. If the table can only have one security
>> policy that applies to everyone who isn't exempt, how will you do
>> that? This sort of use case seems very plausible to me so I think we
>> need to give some real thought to what we will recommend to users who
>> want to do things like this. Can the proposed patch handle it? How?
>
> There are multiple ways this could be implemented- the first, basic, way
> would be through a table which maps users to security levels via an enum
> where more privileged levels are higher in value and therefore a simple
> greater-than could be applied after a join which would implement this
> particular policy.

Interesting.

>> What do you mean by "data changing"? If you mean inserts, updates,
>> and deletes, I am very sure people are going to want to perform those
>> operations on RLS-enabled tables.
>
> Yes, they'll want to support those operations. However, they will not
> expect RLS to allow them to redefine a columns as "x+10" instead of "x",
> which a view does allow.

Hmm, I think some users do want to do things like this. There are
previous discussions of wanting to fuzz a set of coordinates, for
example, or blank out a certain list of columns.

>> Do you find it implausible that someone will want to allow a certain
>> table to bypass RLS when selecting rows, but not when updating or
>> deleting them? I find those scenarios very plausible.
>
> This is also plausible and something which we were anticipating while
> developing this patch. Simon, KaiGai and I specifically discussed
> addressing SELECT vs UPDATE/DELETE earlier this year, as I recall.
> Providing that level of flexibility is absolutely on the road map, but I
> don't know that it all has to exist in 9.5; it may, which would be
> great, but I don't view it as required.

I think we at least need to have a clear design for it before
committing anything. Otherwise we may find that we've committed to
syntax which backs us into a corner.

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

Robert,

However, I believe that
> there has been a lack of focus in the development of the patch thus
> far in a couple of key areas - first in terms of articulating how it
> is different from and better than a writeable security barrier view,
> and second on how to manage the security and operational aspects of
> having a feature like this. I think that the discussion subsequent to
> my June 10th email has let to some good discussion on both points,
> which was my intent, but I still think much more time and thought
> needs to be spent on those issues if we are to have a feature which is
> up to our usual standards. I do apologize to anyone who interpreted
> that initial as a pure rant, because it really wasn't intended that
> way. Contrariwise, I hope that the people defending this patch will
> admit that the issues I am raising are real and focus on whether and
> how those concerns can be addressed.

I absolutely appreciate all of the feedback that has been provided. It has
been educational. To your point above, I started putting together a wiki
page, as Stephen has spoken to, that is meant to capture these concerns and
considerations as well as to capture ideas around solutions.

https://wiki.postgresql.org/wiki/Row_Security_Considerations

This page is obviously not complete, but I think it is a good start.
Hopefully this document will help to continue the conversation and assist
in addressing all the concerns that have been brought to the table. As
well, I hope that this document serves to demonstrate our intent and that
we *are* taking these concerns seriously. I assure you that as one of the
individuals who is working towards the acceptance of this feature/patch, I
am very much concerned about meeting the expected standards of quality and
security.

Thanks,
Adam

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Jun 12, 2014 at 8:13 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > This approach was suggested by an existing user testing out this RLS
> > approach, to be fair, but it looks pretty sane to me as a way to address
> > some of these concerns. Certainly open to other ideas and thoughts though.
>
> In general, I agree that this is a good approach. I think it will be
> difficult to have a GUC with three values, one of which is
> superuser-only and the other two of which are not. I don't think
> there's any precedent for something like that in the existing
> framework, and I think it's likely we'll run into unpleasant corner
> cases if we try to graft it in. Also, I think we need to separate
> things: whether the system is willing to allow the user to access the
> table without RLS, and whether the user is willing to accept RLS if
> the system deems it necessary.

Good point- I agree that it's best to avoid having to support individual
superuser-only only options on a GUC. Also, addressing the issues
independently also makes sense to me.

> For the first one, two solutions have been proposed. The initial
> proposal was to insist on RLS except for the superuser (and maybe the
> table owner?). Having a separate grantable privilege, as Dean
> suggests, may be better. I'll reply separately to that email also, as
> I have a question about what he's proposing.

I like the idea of a grantable privilege as it allows the granularity
that some users may require (or be frustrated that we don't have it).

> For the second one, I think the two most useful behaviors are "normal
> mode" - i.e. allow access to the table, applying RLS predicates if
> required and not applying them if I am exempt - and "error-instead"
> mode - i.e. if my access to this table would be mediated by an RLS
> predicate, then throw an error instead.

Right, makes sense.

> There's a third mode which
> might be useful as well, which is "even though I have the *right* to
> bypass the RLS predicate on this table, please apply the predicate
> anyway". This could be used by the table owner in testing, for
> example.

Agreed, this sounds very useful too.

> Here again, the level of granularity we want to provide is
> an interesting question. Having a GUC (e.g. enable_row_level_security
> = on, off, force) would be adequate for pg_dump, but allowing the
> table name to be qualified in the query, as proposed downthread, would
> be more granular, admittedly at some parser cost. I'm personally of
> the view that we *at least* need the GUC, because that seems like the
> best way to secure pg_dump, and perhaps other applications. We can
> and should give pg_dump an--allow-row-level-security flag, I think,
> but pg_dump's default behavior should be to configure the system in
> such a way that the dump will fail rather than complete with a subset
> of the data.

This sounds good to me.

> I'm less sure whether we should have something that can
> be used to qualify table names in particular queries. I think it
> would be really useful, but I'm concerned that it will require
> creating additional fully-reserved keywords, which are somewhat
> painful for users.

I've been trying to think of the use-case for this. It certainly
*sounds* nice, but on reflection, the use-case for this seems to me to
be that you're trying to develop some application which will be
constrained by RLS totally and therefore want to flip back-and-forth
between "RLS on" and "RLS off" (for the tables involved). When would
you really need, in the same query, to have RLS enabled for table X but
disabled for table Y? I do like the idea of an *independent* option to
(just) COPY which says "give me all the data or error, independent of
the GUC for the same purpose". Would be curious to hear what others
think of that proposal.

Thanks,

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Fri, Jun 13, 2014 at 3:11 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> > Yeah, I was thinking something like this could work, but I would go
> > further. Suppose you had separate GRANTable privileges for direct
> > access to individual tables, bypassing RLS, e.g.
> >
> > GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name
>
> So, is this one new privilege (DIRECT) or four separate new privileges
> that are variants of the existing privileges (DIRECT SELECT, DIRECT
> INSERT, DIRECT UPDATE, DIRECT DELETE)?

I had taken it to be a single privilege, but you're right, it could be
done for each of those.. I really don't think we have the bits for more
than one case here though (if that) without a fair bit of additional
rework. I'm not against that rework (and called for it wayyy back when
I proposed the TRUNCATE privilege, as I recall) but that's a whole
different challenge and no small bit of work..

> > Actually, given the fact that the majority of users won't be using
> > RLS, I would be tempted to invert the above logic and have the new
> > privilege be for LIMITED access (via RLS quals). So a user granted the
> > normal SELECT privilege would be able to bypass RLS, but a user only
> > granted LIMITED SELECT wouldn't.
>
> Well, for the people who are not using RLS, there's no difference
> anyway. I think it matters more what users of RLS will expect from a
> command like GRANT SELECT ... and I'm guessing they'll prefer that RLS
> always apply unless they very specifically grant the right for RLS to
> not apply. I might be wrong, though.

The preference from the folks using RLS that I've talked to is
absolutely that it be applied by default for all 'normal' (eg:
non-pg_dump) sessions.

Thanks,

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Jun 16, 2014 at 1:15 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I understand that there are performance implications. As mentioned to
> > Tom, realistically, there's zero way to optimized at least some of these
> > use-cases because they require a completely external module (eg:
> > SELlinux) to be involved in the decision about who can view what
> > records. If we can optimize that, it'd be by a completely different
> > approach whereby we pull up the qual higher because we know the whole
> > query only involves leakproof functions or similar, allowing us to only
> > apply the filter to the final set of records prior to them being
> > returned to the user. The point being that such optimizations would
> > happen independently and regardless of the quals or user-defined
> > functions involved. At the end of the day, I can't think of a better
> > optimization for such a case (where we have to ask an external security
> > module if a row is acceptable to return to the user) than that. Is
> > there something specific you're thinking about that we'd be missing out
> > on?
>
> Yeah, if we have to ask an external security module a question for
> each row, there's little hope of any real optimization. However, I
> think there will be a significant number of cases where people will
> want filtering clauses that can be realized by doing an index scan
> instead of a sequential scan, and if we end up forcing a sequential
> scan anyway, the feature will be useless to those people.

I agree that we want to support that, if we can do so reasonably. What
I was trying to get at is simply this- don't we provide that already
with the leakproof attribute and functions? If we don't have enough
there to allow index scans then we should be looking to add more, I'm
thinking.

> > Perhaps it's just my experience, but I've been focused on the main core
> > feature for quite some time and it feels like we're really close to
> > having it there. I agree that a few additional bits would be nice to
> > have but these strike me as relatively straight-forward to implement
> > overtop of this general construct. I do see value in documenting these
> > concerns and will see about making that happen, along with what the
> > general viewpoints and thoughts are about how to address the concern.
>
> I feel like there's quite a bit of work left to do around these
> issues. The technical bits may not be too hard, but deciding what we
> want will take some thought and discussion.

I agree on this point, but I'm still hopeful that we'll be able to get a
good feature into 9.5. There are quite a few resources available for
the 'just programming' part, so the long pole in the tent here is
absolutely hashing out what we want and how it should function.

I'd be happy to host or participate in a conference call or similar if
that would be useful to move this along- or we can continue to
communicate via email. There's a bit of a lull in conferences to which
I'm going to right now, so in person is unlikely, unless folks want to
get together somewhere on the east coast (I'd be happy to travel to
Philly, Pittsburgh, NYC, etc, if it'd help..).

> > That's not actualy true today, is it? Given our leak-proof attribute,
> > if the qual is "WHERE somecomplexfunction() AND leakprooffunctionx()"
> > then we would be able to push down the leak-proof function and not
> > necessairly run a straight sequential scan, no? Even so, though, we've
> > had users who have tested exactly what this patch implements and they've
> > been happy with their real-world use-cases. I'm certainly all for
> > optimization and would love to see us make this better for everyone, but
> > I don't view that as a reason to delay this particular feature which is
> > really just bringing us up to parity with other RDMBS's.
>
> I'm a bit confused here, because your example seems to be totally
> different from my example. In my example, somecomplexfunction() will
> get pushed down because it's the security qual; that needs to be
> inside the security_barrier view, or a malicious user can subvert the
> system by getting some other qual evaluated first. In your example,
> you seem to be imagining WHERE somecomplexfunction() AND
> leakprooffunctionx() as queries sent by the untrusted user, in which
> case, yet, the leak-proof one will get pushed down and the other one
> will not.

Right- my point there was that the leakproof one might allow an index
scan to be run. This is all pretty hand-wavey, I admit, so I'll see if
I can get more details about how the currently-proposed patch is
performing for the users who are testing it and what kind of plans
they're seeing. If that falls through, I'll try and build up my own set
of realistic-looking (to myself and the users who are testing) example.

> > What solution did you come up with for this case, which performed well
> > and was also secure..?
>
> I put the logic in the client. :-(

Well, that's not helpful here. ;)

> >> I don't want to overstate the importance of this particular case; but
> >> I do think scenarios in which it's advantageous to have multiple
> >> row-level security policies are plausible.
> >
> > I'm not against this in general. The question, in my mind, is what
> > level of granularity we would provide this at. As I tried to outline
> > previously, there's a huge number of combinations which we could come up
> > with to support this under and I'm not 100% sure that it'd actualy end
> > up being better than the simplicity of a single qual where the user gets
> > to define any kind of relationship they want between the various
> > policies; even programatically if they want.
>
> I agree. That's why I think we need some more design work in this
> area. Perhaps it's OK to allow only one RLS-qual per table at most,
> and tell people that if you want more than that, you need to use
> security-barrier views as wrappers instead.

Note that my suggestion would be to simply put a pl/pgsql call (perhaps
a security definer one) into the RLS definition- not to say "use views".

> But I'm not sure; that
> feels like it's giving something up that might be important. And I
> think that the kinds of syntax we're discussing won't support leaving
> that out of the initial version and adding it later, so if we commit
> to this syntax, we're stuck with that behavior. To avoid that, we'd
> need something like this:
>
> ALTER TABLE tab ADD POLICY polname WHERE quals;
> GRANT SELECT (polname) ON TABLE tab TO role;

Right, if we were to support multiple policies on a given table then we
would have to support adding and removing them individually, as well as
specify when they are to be applied- and what if that "when" overlaps?
Do we apply both and only a row which passed them all gets sent to the
user? Essentially we'd be defining the RLS policies to be AND'd
together, right? Would we want to support both AND-based and OR-based,
and allow users to pick what set of conditionals they want applied to
their various overlapping RLS policies?

Sounds all rather painful and much better done programatically by the
user in a language which is suited to that task- eg: pl/pgsql, perl, C,
or something besides our ALTER syntax + catalog representation.

> >> What do you mean by "data changing"? If you mean inserts, updates,
> >> and deletes, I am very sure people are going to want to perform those
> >> operations on RLS-enabled tables.
> >
> > Yes, they'll want to support those operations. However, they will not
> > expect RLS to allow them to redefine a columns as "x+10" instead of "x",
> > which a view does allow.
>
> Hmm, I think some users do want to do things like this. There are
> previous discussions of wanting to fuzz a set of coordinates, for
> example, or blank out a certain list of columns.

Absolutely they'll want to be able to do this- but that's going to be a
case which I (and others, I think) feel comfortable going back and
saying "use views for that". I'm trying to draw that line in the ground
between what is RLS and what are views and keeping RLS to the WHERE
clause strikes me as a good line to draw (and one which matches up with
existing expectations in this space).

> > This is also plausible and something which we were anticipating while
> > developing this patch. Simon, KaiGai and I specifically discussed
> > addressing SELECT vs UPDATE/DELETE earlier this year, as I recall.
> > Providing that level of flexibility is absolutely on the road map, but I
> > don't know that it all has to exist in 9.5; it may, which would be
> > great, but I don't view it as required.
>
> I think we at least need to have a clear design for it before
> committing anything. Otherwise we may find that we've committed to
> syntax which backs us into a corner.

Fair enough. There was some support for this idea in the original patch
by Craig, but we can further develop this syntax (and what it may look
like for 9.5, if it ends up not covering all cases).

Thanks!

Stephen

On Tue, Jun 17, 2014 at 9:45 PM, Brightwell, Adam
<adam(dot)brightwell(at)crunchydatasolutions(dot)com> wrote:
> I absolutely appreciate all of the feedback that has been provided. It has
> been educational. To your point above, I started putting together a wiki
> page, as Stephen has spoken to, that is meant to capture these concerns and
> considerations as well as to capture ideas around solutions.
>
> https://wiki.postgresql.org/wiki/Row_Security_Considerations
>
> This page is obviously not complete, but I think it is a good start.
> Hopefully this document will help to continue the conversation and assist in
> addressing all the concerns that have been brought to the table. As well, I
> hope that this document serves to demonstrate our intent and that we *are*
> taking these concerns seriously. I assure you that as one of the
> individuals who is working towards the acceptance of this feature/patch, I
> am very much concerned about meeting the expected standards of quality and
> security.

Cool, thanks for weighing in. I think that page is a good start. An
item that I think should be added there is the potential overlap
between security_barrier views and row-level security. How can we
reuse code (and SQL syntax?) for existing features like WITH CHECK
OPTION instead of writing new code (and inventing new syntax) for very
similar concepts?

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

On Tue, Jun 17, 2014 at 10:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Fri, Jun 13, 2014 at 3:11 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> > Yeah, I was thinking something like this could work, but I would go
>> > further. Suppose you had separate GRANTable privileges for direct
>> > access to individual tables, bypassing RLS, e.g.
>> >
>> > GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name
>>
>> So, is this one new privilege (DIRECT) or four separate new privileges
>> that are variants of the existing privileges (DIRECT SELECT, DIRECT
>> INSERT, DIRECT UPDATE, DIRECT DELETE)?
>
> I had taken it to be a single privilege, but you're right, it could be
> done for each of those.. I really don't think we have the bits for more
> than one case here though (if that) without a fair bit of additional
> rework. I'm not against that rework (and called for it wayyy back when
> I proposed the TRUNCATE privilege, as I recall) but that's a whole
> different challenge and no small bit of work..

Technically, there are 4 bits left, and that's what we need for
separate privileges. We last consumed bits in 2008 (for TRUNCATE) and
2006 (for GRANT ON DATABASE), so even if we used all of the remaining
bits it might be another 5 years before anyone has to do that
refactoring. But even if the refactoring needs to be done now for
some reason, it's only June, and the last CommitFest doesn't start
until February 15th. I think we're being way too quick to jump to
talking about what can and can't be done in time for 9.5. Let's start
by figuring out how we'd really like it to work and then, if it's too
ambitious, we can scale it back.

My main concern about using only one bit is that someone might want to
allow a user to bypass RLS on SELECT while still enforcing it for
data-modifying operations. That seems like a plausible use case to
me.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Jun 17, 2014 at 10:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I had taken it to be a single privilege, but you're right, it could be
> > done for each of those.. I really don't think we have the bits for more
> > than one case here though (if that) without a fair bit of additional
> > rework. I'm not against that rework (and called for it wayyy back when
> > I proposed the TRUNCATE privilege, as I recall) but that's a whole
> > different challenge and no small bit of work..
>
> Technically, there are 4 bits left, and that's what we need for
> separate privileges.

I'd really hate to chew them all up..

> We last consumed bits in 2008 (for TRUNCATE) and
> 2006 (for GRANT ON DATABASE), so even if we used all of the remaining
> bits it might be another 5 years before anyone has to do that
> refactoring.

Perhaps, or we might come up with some new whiz-bang permission to add
next year. :/

> But even if the refactoring needs to be done now for
> some reason, it's only June, and the last CommitFest doesn't start
> until February 15th. I think we're being way too quick to jump to
> talking about what can and can't be done in time for 9.5. Let's start
> by figuring out how we'd really like it to work and then, if it's too
> ambitious, we can scale it back.

Alright- perhaps we can discuss what kind of refactoring would be needed
for such a change then, to get a better idea as to the scope of the
change and the level of effort required.

My thoughts on how to address this were to segregate the ACL bits by
object type. That is to say, the AclMode stored for databases might
only use bits 0-2 (create/connect/temporary), while tables would use
bits 0-7 (insert/select/update/delete/references/trigger). This would
allow us to more easily add more rights at the database and/or
tablespace level too.

> My main concern about using only one bit is that someone might want to
> allow a user to bypass RLS on SELECT while still enforcing it for
> data-modifying operations. That seems like a plausible use case to
> me.

I absolutely agree that it's a real use-case and one which we should
support, just trying to avoid biting off more than can be done between
now and February. Still, if we get things hammered out and more-or-less
agreement on the way forward, getting the code written may move quickly.

Thanks,

Stephen

On Tue, Jun 17, 2014 at 10:25 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Yeah, if we have to ask an external security module a question for
>> each row, there's little hope of any real optimization. However, I
>> think there will be a significant number of cases where people will
>> want filtering clauses that can be realized by doing an index scan
>> instead of a sequential scan, and if we end up forcing a sequential
>> scan anyway, the feature will be useless to those people.
>
> I agree that we want to support that, if we can do so reasonably. What
> I was trying to get at is simply this- don't we provide that already
> with the leakproof attribute and functions? If we don't have enough
> there to allow index scans then we should be looking to add more, I'm
> thinking.

So the reason why we got onto this particular topic was because of the
issue of multiple security policies for a single table. Of course,
multiple security policies can always be merged into a single
more-complex policy, but the resulting policy may be so complex that
the query-planner is no longer capable of doing a good job optimizing
it. I won't mention here exactly what a certain large commercial
database vendor has implemented here; suffice it to say, however, that
their design avoids this pitfall, and ours currently does not.

> I agree on this point, but I'm still hopeful that we'll be able to get a
> good feature into 9.5. There are quite a few resources available for
> the 'just programming' part, so the long pole in the tent here is
> absolutely hashing out what we want and how it should function.

Agreed.

> I'd be happy to host or participate in a conference call or similar if
> that would be useful to move this along- or we can continue to
> communicate via email. There's a bit of a lull in conferences to which
> I'm going to right now, so in person is unlikely, unless folks want to
> get together somewhere on the east coast (I'd be happy to travel to
> Philly, Pittsburgh, NYC, etc, if it'd help..).

For me, email is easiest; but there are other options, too.

>> > What solution did you come up with for this case, which performed well
>> > and was also secure..?
>>
>> I put the logic in the client. :-(
>
> Well, that's not helpful here. ;)

Sure. The reason I brought it up is to say - hey, look, I had this
come up in the real world. What would it take to be able to do
actually do it in the database server? And the answer is - something
that will handle multiple security policies cleanly.

>> But I'm not sure; that
>> feels like it's giving something up that might be important. And I
>> think that the kinds of syntax we're discussing won't support leaving
>> that out of the initial version and adding it later, so if we commit
>> to this syntax, we're stuck with that behavior. To avoid that, we'd
>> need something like this:
>>
>> ALTER TABLE tab ADD POLICY polname WHERE quals;
>> GRANT SELECT (polname) ON TABLE tab TO role;
>
> Right, if we were to support multiple policies on a given table then we
> would have to support adding and removing them individually, as well as
> specify when they are to be applied- and what if that "when" overlaps?
> Do we apply both and only a row which passed them all gets sent to the
> user? Essentially we'd be defining the RLS policies to be AND'd
> together, right? Would we want to support both AND-based and OR-based,
> and allow users to pick what set of conditionals they want applied to
> their various overlapping RLS policies?

AND is not a sensible policy; it would need to be OR. If you grant
someone access to two different subsets of the rows in a table, it
stands to reason that they will expect to have access to all of the
rows that are in at least one of those subsets. If you give someone
your car key and your house key, that means they can operate your car
or enter your house; it does not mean that they can operate your car
but only when it's inside your garage.

Alternatively, we could:

- Require the user to specify in some way which of the available
policies they want applied, and then apply only that one.
or
- Decide that such scenarios constitute misconfiguration. Throw an
error and make the table owner or other relevant local authority fix
it.

> Sounds all rather painful and much better done programatically by the
> user in a language which is suited to that task- eg: pl/pgsql, perl, C,
> or something besides our ALTER syntax + catalog representation.

I think exactly the opposite, for the query planning reasons
previously stated. I think the policies will quickly get so
complicated that they're no longer optimizable. Here's a simple
example:

- Policy 1 allows the user to access rows for which complexfunc() returns true.
- Policy 2 allows the user to access rows for which a = 1.

Most users have access only through policy 2, but some have access
through policy 1. Users who have access through policy 1 will always
get a sequential scan, but users who have access through policy 2 have
an excellent chance of getting an index scan if the selectivity of a =
1 is high. When you merge those two things into a single policy, no
matter how you do it, everyone gets sequential scans all the time.
That sucks.

>> Hmm, I think some users do want to do things like this. There are
>> previous discussions of wanting to fuzz a set of coordinates, for
>> example, or blank out a certain list of columns.
>
> Absolutely they'll want to be able to do this- but that's going to be a
> case which I (and others, I think) feel comfortable going back and
> saying "use views for that". I'm trying to draw that line in the ground
> between what is RLS and what are views and keeping RLS to the WHERE
> clause strikes me as a good line to draw (and one which matches up with
> existing expectations in this space).

Fair.

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

On Wed, Jun 18, 2014 at 10:40 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> On Tue, Jun 17, 2014 at 10:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > I had taken it to be a single privilege, but you're right, it could be
>> > done for each of those.. I really don't think we have the bits for more
>> > than one case here though (if that) without a fair bit of additional
>> > rework. I'm not against that rework (and called for it wayyy back when
>> > I proposed the TRUNCATE privilege, as I recall) but that's a whole
>> > different challenge and no small bit of work..
>>
>> Technically, there are 4 bits left, and that's what we need for
>> separate privileges.
>
> I'd really hate to chew them all up..

Usually it's the patch author who WANTS to chew up all the available
bit space and OTHER people who say no. :-)

>> We last consumed bits in 2008 (for TRUNCATE) and
>> 2006 (for GRANT ON DATABASE), so even if we used all of the remaining
>> bits it might be another 5 years before anyone has to do that
>> refactoring.
>
> Perhaps, or we might come up with some new whiz-bang permission to add
> next year. :/

Well, people proposed separate permissions for things like VACUUM and
ANALYZE around the time TRUNCATE was added, and those were rejected on
the grounds that they didn't add enough value to justify wasting bits
on them. I think we see whether there's a workable system that such
that marginal permissions (like TRUNCATE) that won't be checked often
don't have to consume bits.

>> But even if the refactoring needs to be done now for
>> some reason, it's only June, and the last CommitFest doesn't start
>> until February 15th. I think we're being way too quick to jump to
>> talking about what can and can't be done in time for 9.5. Let's start
>> by figuring out how we'd really like it to work and then, if it's too
>> ambitious, we can scale it back.
>
> Alright- perhaps we can discuss what kind of refactoring would be needed
> for such a change then, to get a better idea as to the scope of the
> change and the level of effort required.
>
> My thoughts on how to address this were to segregate the ACL bits by
> object type. That is to say, the AclMode stored for databases might
> only use bits 0-2 (create/connect/temporary), while tables would use
> bits 0-7 (insert/select/update/delete/references/trigger). This would
> allow us to more easily add more rights at the database and/or
> tablespace level too.

Yeah, that's another idea. But it really deserves its own thread.
I'm still not convinced we have to do this at all to meet this need,
but that should be argued back and forth on that other thread.

>> My main concern about using only one bit is that someone might want to
>> allow a user to bypass RLS on SELECT while still enforcing it for
>> data-modifying operations. That seems like a plausible use case to
>> me.
>
> I absolutely agree that it's a real use-case and one which we should
> support, just trying to avoid biting off more than can be done between
> now and February. Still, if we get things hammered out and more-or-less
> agreement on the way forward, getting the code written may move quickly.

Nifty.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Jun 17, 2014 at 10:25 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I agree that we want to support that, if we can do so reasonably. What
> > I was trying to get at is simply this- don't we provide that already
> > with the leakproof attribute and functions? If we don't have enough
> > there to allow index scans then we should be looking to add more, I'm
> > thinking.
>
> So the reason why we got onto this particular topic was because of the
> issue of multiple security policies for a single table. Of course,
> multiple security policies can always be merged into a single
> more-complex policy, but the resulting policy may be so complex that
> the query-planner is no longer capable of doing a good job optimizing
> it.

Yeah, I could see that happening with some use-cases.

> >> ALTER TABLE tab ADD POLICY polname WHERE quals;
> >> GRANT SELECT (polname) ON TABLE tab TO role;
> >
> > Right, if we were to support multiple policies on a given table then we
> > would have to support adding and removing them individually, as well as
> > specify when they are to be applied- and what if that "when" overlaps?
> > Do we apply both and only a row which passed them all gets sent to the
> > user? Essentially we'd be defining the RLS policies to be AND'd
> > together, right? Would we want to support both AND-based and OR-based,
> > and allow users to pick what set of conditionals they want applied to
> > their various overlapping RLS policies?
>
> AND is not a sensible policy; it would need to be OR. If you grant
> someone access to two different subsets of the rows in a table, it
> stands to reason that they will expect to have access to all of the
> rows that are in at least one of those subsets.

I think I can buy off on this. What that also means is that any
'short-circuiting' that we try to do here would be based on "stop once
we get back a 'true'". This could seriously change how we actually
implement RLS though as doing it all through query rewrites and making
this work with multiple security policies which are OR'd together and
yet keeping the optimization and qual push-down and index-based plans is
looking pretty daunting.

I'm also of the opinion that this isn't strictly necessary for the
initial RLS offering in PG- there's a clear way we could migrate
existing users to a multi-policy system from a single-policy system.
Sure, to get the performance and optimization benefits that we'd
presumably have in the multi-policy case they'd need to re-work their
RLS configuration, but for users who care, they'll likely be very happy
to do so to gain those benefits.

Perhaps the question here is- if we implement RLS one way for the single
case and then change the implementation all around for the multi case,
will we end up breaking the single case? Or destroying the performance
for it? I can't see either of those cases being allowed- if and when we
support multi, we must still support single and the whole point of multi
would be to allow more performant implementations and that solution will
require the single case to be at least as performant as what we're
proposing to do today, I believe.

Or are you thinking that we would never support calling user-defined
functions in any RLS scheme because we want to be able to do that
optimization? I don't see that being acceptable from a feature
standpoint.

> Alternatively, we could:
>
> - Require the user to specify in some way which of the available
> policies they want applied, and then apply only that one.

I'd want to at least see a way to apply an ordering to the policies
being applied, or have PG work out which one is "cheapest" and try that
one first.

> - Decide that such scenarios constitute misconfiguration. Throw an
> error and make the table owner or other relevant local authority fix
> it.

Having them all be OR'd together feels simpler and easier to work with
than trying to provide the user with all the knobs necessary to select
which subset of users they want the policy applied to when (user X from
IP range a.b.c.d/24 at time 1500). We could probably make it work with
exclusion constraints, range types, etc, and perhaps it'd be a reason to
bring btree_gist into core (which I'm all for) and make it work with
catalog tables, but... just 'yuck' all around, for my part.

> > Sounds all rather painful and much better done programatically by the
> > user in a language which is suited to that task- eg: pl/pgsql, perl, C,
> > or something besides our ALTER syntax + catalog representation.
>
> I think exactly the opposite, for the query planning reasons
> previously stated. I think the policies will quickly get so
> complicated that they're no longer optimizable. Here's a simple
> example:
>
> - Policy 1 allows the user to access rows for which complexfunc() returns true.
> - Policy 2 allows the user to access rows for which a = 1.
>
> Most users have access only through policy 2, but some have access
> through policy 1. Users who have access through policy 1 will always
> get a sequential scan,

This is the thing which I most object to- if the quals being provided at
any level are leakproof and would be able to reduce the returned set
sufficiently that an index scan is the best bet, we should be doing
that. I don't anticipate the RLS quals to be as selective as the
quals which the user is adding.

I agree that in cases where the user isn't using a leakproof function in
their quals and the policy is complex, a sequential scan would have to
be done over the table, but looking at the set of leakproof vs not
leakproof functions used by operators which return boolean, certainly
the most common of the index using cases are covered and we may be able
to add more leakproof functions, should we get user complaints that the
function they're using works fine with an index but isn't leakproof.

> but users who have access through policy 2 have
> an excellent chance of getting an index scan if the selectivity of a =
> 1 is high. When you merge those two things into a single policy, no
> matter how you do it, everyone gets sequential scans all the time.
> That sucks.

It just strikes me as unlikely that in such a simple policy the
selectivity of the RLS qual used will be high and this feels like a lot
of mechanism and complication to be adding for that use-case. If the
selectivity is actually high in terms of what the RLS qual will allow,
then it seems likely, to me at least, that it's going to need to depend
on another table or function, eg:

exists(select 1 from security_table
where (current_user(),a) = (sec_user,sec_label))

Still thinking about this approach in general. Having a good answer to
the question about granularity and how this multiple RLS-policy would
actually work would certainly help. Being able to pick a single policy
(rather than deal with overlapping policies that all have to be tested)
would definitely make this simpler, but I suppose we could build up "X
OR Y OR Z..." inside the query..

Thanks,

Stephen

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jun 18, 2014 at 10:40 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> Technically, there are 4 bits left, and that's what we need for
> >> separate privileges.
> >
> > I'd really hate to chew them all up..
>
> Usually it's the patch author who WANTS to chew up all the available
> bit space and OTHER people who say no. :-)

Ah, well, technically I'm not the patch author here, though I would like
to see it happen. :) Still, have to balance these features and
capabilities against the future unknown options we might want to add and
it certainly doesn't seem terribly nice to chew up all that remain
rather than addressing the need to support more.

Still, perhaps we can put together a patch for this and then review the
implementation and, if we like it and that functionality, we can make
the decision about if it should be on this patch to make more bits
available.

> > Perhaps, or we might come up with some new whiz-bang permission to add
> > next year. :/
>
> Well, people proposed separate permissions for things like VACUUM and
> ANALYZE around the time TRUNCATE was added, and those were rejected on
> the grounds that they didn't add enough value to justify wasting bits
> on them. I think we see whether there's a workable system that such
> that marginal permissions (like TRUNCATE) that won't be checked often
> don't have to consume bits.

That's an interesting approach but I'm not sure that we need to go a
system where we segregate "often-used" bits from "less-used" ones.

> > My thoughts on how to address this were to segregate the ACL bits by
> > object type. That is to say, the AclMode stored for databases might
> > only use bits 0-2 (create/connect/temporary), while tables would use
> > bits 0-7 (insert/select/update/delete/references/trigger). This would
> > allow us to more easily add more rights at the database and/or
> > tablespace level too.
>
> Yeah, that's another idea. But it really deserves its own thread.
> I'm still not convinced we have to do this at all to meet this need,
> but that should be argued back and forth on that other thread.

Fair enough.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> [ counting bits in ACLs ]

Wouldn't it be fairly painless to widen AclMode from 32 bits to 64,
and thereby double the number of available bits?

That code was all written before we required platforms to have an int64
primitive type, but of course now we expect that.

In any case, I concur with the position that this feature patch should
be separate from a patch to make additional bitspace available.

regards, tom lane

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> [ counting bits in ACLs ]
>
> Wouldn't it be fairly painless to widen AclMode from 32 bits to 64,
> and thereby double the number of available bits?

Thanks for commenting on this. I hadn't considered that but I don't see
any particular problem with it either..

> In any case, I concur with the position that this feature patch should
> be separate from a patch to make additional bitspace available.

Certainly. Thanks for your thoughts.

Stephen

On 17 June 2014 20:19, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 13, 2014 at 3:11 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> Yeah, I was thinking something like this could work, but I would go
>> further. Suppose you had separate GRANTable privileges for direct
>> access to individual tables, bypassing RLS, e.g.
>>
>> GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name
>
> So, is this one new privilege (DIRECT) or four separate new privileges
> that are variants of the existing privileges (DIRECT SELECT, DIRECT
> INSERT, DIRECT UPDATE, DIRECT DELETE)?
>

I was thinking it would be 4 new privileges, so that a user could for
example be granted DIRECT SELECT permission on a table, but not DIRECT
UPDATE.

On reflection though, I think I prefer the approach of allowing
multiple named security policies per table, because it gives the
planner more opportunity to optimize queries against specific RLS
quals, which won't work if the ACL logic is embedded in functions.
That seems like something that would have to be designed in now,
because it's difficult to see how you could add it later.

Managing policy names becomes an issue though, because if you have 2
tables each with 1 policy, but you give them different names, how can
the user querying the data specify that they want policy1 for table1
and policy2 for table2, possibly in the same query? I think that can
be made more manageable by making policies top-level objects that
exist independently of any particular tables. So you might do
something like:

\c - alice
CREATE POLICY policy1;
CREATE POLICY policy2;
ALTER TABLE t1 SET POLICY policy1 TO t1_quals;
ALTER TABLE t2 SET POLICY policy1 TO t2_quals;
...
GRANT SELECT ON TABLE t1, t2 TO bob USING policy1;
GRANT SELECT ON TABLE t1, t2 TO manager; -- Can use any policy, or
bypass all policies

Then a particular user would typically only have to set their policy
once per session, for accessing multiple tables:

\c - bob
SET rls_policy = policy1;
SELECT * FROM t1 JOIN t2; -- OK
SET rls_policy = policy2;
SELECT * FROM t1; -- ERROR: no permission to access t1 using policy2

or you'd be able to set a default policy for users, so that they
wouldn't need to explicitly choose one:

ALTER ROLE bob SET rls_policy = policy1;

Note that the syntax proposed elsewhere --- GRANT SELECT (polname) ON
TABLE tab TO role --- doesn't work because it conflicts with the
syntax for granting column privileges, so there needs to be a distinct
syntax for this, and I think it ought to ultimately allow things like

GRANT SELECT (col1, col2), UPDATE (col1) ON t1 TO bob USING policy1;

Regards,
Dean

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 17 June 2014 20:19, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Fri, Jun 13, 2014 at 3:11 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> >> Yeah, I was thinking something like this could work, but I would go
> >> further. Suppose you had separate GRANTable privileges for direct
> >> access to individual tables, bypassing RLS, e.g.
> >>
> >> GRANT DIRECT SELECT|INSERT|UPDATE|DELETE ON table_name TO role_name
> >
> > So, is this one new privilege (DIRECT) or four separate new privileges
> > that are variants of the existing privileges (DIRECT SELECT, DIRECT
> > INSERT, DIRECT UPDATE, DIRECT DELETE)?
>
> I was thinking it would be 4 new privileges, so that a user could for
> example be granted DIRECT SELECT permission on a table, but not DIRECT
> UPDATE.

Ok.

> On reflection though, I think I prefer the approach of allowing
> multiple named security policies per table, because it gives the
> planner more opportunity to optimize queries against specific RLS
> quals, which won't work if the ACL logic is embedded in functions.

Having more than one policy for the purpose of performance really
doesn't make a huge amount of sense to me. Perhaps someone could
explain the use-case with specific example applications where they would
benefit from this? Based on the discussion, they would have to be OR'd
together in the query as built with any result being marked as success.
One could build an SQL function which could be in-lined potentially
which does the same if their case is that simple.

Being able to define the policy based on some criteria may allow it to
be simpler (eg: policy 'a' applies for certain roles, while policy 'b'
applies for other roles), but I'm not enthusiastic about that approach
because there could be a huge number of permutations to allow.

How about another approach- what about having a function which is called
(as the table owner, I'm thinking..) that then returns the qual to be
included, instead of having to define a specific qual which is included
in the catalog? That function could take into consideration the user,
table, etc, and return a qual which includes constants to compare rows
against for planning purposes. This would have to be done early enough,
of course, which might be difficult. For my part, having that
capability would be neat, but nothing we're trying to do here would
preclude us from adding it later either.

> That seems like something that would have to be designed in now,
> because it's difficult to see how you could add it later.

I don't follow this at all. Going from supporting one qual to
supporting multiple seems like it'd be quite straight-forward to add
in later? Going the other way would be difficult.

> Managing policy names becomes an issue though, because if you have 2
> tables each with 1 policy, but you give them different names, how can
> the user querying the data specify that they want policy1 for table1
> and policy2 for table2, possibly in the same query?

From my experience, users don't pick the policy any more than they get
to pick which set of permissions get applied to them when querying
tables (modulo roles, of course, but that's a mechanism for changing
users, not for saying which set of permissions you get). All that you
describe could be done for regular permissions also, but we don't, and
I don't think we get complaints about that because we have roles-
which would work just the same for RLS (assuming the RLS policy defined
has a role component).

Having a function be able to be called to return a qual to be used would
be a way to have per-role RLS also, along with providing the flexibility
to have per-source-IP, per-connection-type, etc, RLS policies also.

Thanks,

Stephen

On Wed, Jun 18, 2014 at 2:18 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I'm also of the opinion that this isn't strictly necessary for the
> initial RLS offering in PG- there's a clear way we could migrate
> existing users to a multi-policy system from a single-policy system.
> Sure, to get the performance and optimization benefits that we'd
> presumably have in the multi-policy case they'd need to re-work their
> RLS configuration, but for users who care, they'll likely be very happy
> to do so to gain those benefits.

I think a lot depends on the syntax we choose. If we choose a syntax
that only makes sense in a single-policy framework, then I think
allowing upgrades to a multi-policy syntax is going to be really
difficult. On the other hand, if we choose a syntax that allows
multiple policies, I suspect we can support multiple policies from the
beginning without much extra effort.

>> - Require the user to specify in some way which of the available
>> policies they want applied, and then apply only that one.
>
> I'd want to at least see a way to apply an ordering to the policies
> being applied, or have PG work out which one is "cheapest" and try that
> one first.

Cost-based comparison of policies that return different results
doesn't seem sensible to me.

>> I think exactly the opposite, for the query planning reasons
>> previously stated. I think the policies will quickly get so
>> complicated that they're no longer optimizable. Here's a simple
>> example:
>>
>> - Policy 1 allows the user to access rows for which complexfunc() returns true.
>> - Policy 2 allows the user to access rows for which a = 1.
>>
>> Most users have access only through policy 2, but some have access
>> through policy 1. Users who have access through policy 1 will always
>> get a sequential scan,
>
> This is the thing which I most object to- if the quals being provided at
> any level are leakproof and would be able to reduce the returned set
> sufficiently that an index scan is the best bet, we should be doing
> that. I don't anticipate the RLS quals to be as selective as the
> quals which the user is adding.

I think it would be a VERY bad idea to design the system around the
assumption that the RLS quals will be much more or less selective than
the user-supplied quals. That's going to be different in different
environments.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jun 18, 2014 at 2:18 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I'm also of the opinion that this isn't strictly necessary for the
> > initial RLS offering in PG- there's a clear way we could migrate
> > existing users to a multi-policy system from a single-policy system.
> > Sure, to get the performance and optimization benefits that we'd
> > presumably have in the multi-policy case they'd need to re-work their
> > RLS configuration, but for users who care, they'll likely be very happy
> > to do so to gain those benefits.
>
> I think a lot depends on the syntax we choose. If we choose a syntax
> that only makes sense in a single-policy framework, then I think
> allowing upgrades to a multi-policy syntax is going to be really
> difficult. On the other hand, if we choose a syntax that allows
> multiple policies, I suspect we can support multiple policies from the
> beginning without much extra effort.

What are these policies going to depend on? Will they be allowed to
overlap? I don't see multi-policy support as being very easily added.

If there are specific ways to design the syntax which would make it
easier to support multiple policies in the future, I'm all for it. Have
any specific thoughts regarding that?

> >> - Require the user to specify in some way which of the available
> >> policies they want applied, and then apply only that one.
> >
> > I'd want to at least see a way to apply an ordering to the policies
> > being applied, or have PG work out which one is "cheapest" and try that
> > one first.
>
> Cost-based comparison of policies that return different results
> doesn't seem sensible to me.

I keep coming back to the thought that, really, having multiple
overlapping policies just adds unnecessary complication to the system
for not much gain in real functionality. Being able to specify a policy
per-role might be useful, but that's only one dimension and I can
imagine a lot of other dimensions that one might want to use to control
which policy is used.

> >> I think exactly the opposite, for the query planning reasons
> >> previously stated. I think the policies will quickly get so
> >> complicated that they're no longer optimizable. Here's a simple
> >> example:
> >>
> >> - Policy 1 allows the user to access rows for which complexfunc() returns true.
> >> - Policy 2 allows the user to access rows for which a = 1.
> >>
> >> Most users have access only through policy 2, but some have access
> >> through policy 1. Users who have access through policy 1 will always
> >> get a sequential scan,
> >
> > This is the thing which I most object to- if the quals being provided at
> > any level are leakproof and would be able to reduce the returned set
> > sufficiently that an index scan is the best bet, we should be doing
> > that. I don't anticipate the RLS quals to be as selective as the
> > quals which the user is adding.
>
> I think it would be a VERY bad idea to design the system around the
> assumption that the RLS quals will be much more or less selective than
> the user-supplied quals. That's going to be different in different
> environments.

Fine- but do you really see the query planner having a problem pushing
down whichever is the more selective qual, if the user-provided qual is
marked as leakproof?

I realize that you want multiple policies because you'd like a way for
the RLS qual to be made simpler for certain cases while also having more
complex quals for other cases. What I keep waiting to hear is exactly
how you want to specify which policy is used because that's where it
gets ugly and complicated. I still really don't like the idea of trying
to apply multiple policies inside of a single query execution.

Thanks,

Stephen

On Mon, Jun 23, 2014 at 2:29 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> What are these policies going to depend on? Will they be allowed to
> overlap? I don't see multi-policy support as being very easily added.

We discussed the point about overlap upthread, and I gave specific
examples. If there's something else you want me to provide here,
please be more clear about it.

> If there are specific ways to design the syntax which would make it
> easier to support multiple policies in the future, I'm all for it. Have
> any specific thoughts regarding that?

I did propose something already upthread, and then Dean said this:

# Note that the syntax proposed elsewhere --- GRANT SELECT (polname) ON
# TABLE tab TO role --- doesn't work because it conflicts with the
# syntax for granting column privileges, so there needs to be a distinct
# syntax for this, and I think it ought to ultimately allow things like
#
# GRANT SELECT (col1, col2), UPDATE (col1) ON t1 TO bob USING policy1;

He's got a good point there. I don't know whether the policy should
be given inline (e.g. GRANT ... WHERE stuff()) or out-of-line (GRANT
... USING policy1) but it seems like specifying it as some sort of
GRANT modifier might make sense. I'm sure there are other ways also,
of course.

>> >> - Require the user to specify in some way which of the available
>> >> policies they want applied, and then apply only that one.
>> >
>> > I'd want to at least see a way to apply an ordering to the policies
>> > being applied, or have PG work out which one is "cheapest" and try that
>> > one first.
>>
>> Cost-based comparison of policies that return different results
>> doesn't seem sensible to me.
>
> I keep coming back to the thought that, really, having multiple
> overlapping policies just adds unnecessary complication to the system
> for not much gain in real functionality. Being able to specify a policy
> per-role might be useful, but that's only one dimension and I can
> imagine a lot of other dimensions that one might want to use to control
> which policy is used.

Well, I don't agree, and I've given examples upthread showing the
kinds of scenarios that I'm concerned about, which are drawn from real
experiences I've had. It may be that I'm the only one who has had
such experiences, of course; or that there aren't enough people who
have to justify catering to such use cases. But I'm not sure there's
much point in trying to have a conversation about how such a thing
could be made to work if you're just going to revert back to "well, we
don't really need this anyway" each time I make or refute a technical
point.

>> I think it would be a VERY bad idea to design the system around the
>> assumption that the RLS quals will be much more or less selective than
>> the user-supplied quals. That's going to be different in different
>> environments.
>
> Fine- but do you really see the query planner having a problem pushing
> down whichever is the more selective qual, if the user-provided qual is
> marked as leakproof?

I'm not quite sure I understand the scenario you're describing here.
Can you provide a tangible example? I expect that most of the things
the RLS-limited user might write in the WHERE clause will NOT get
pushed down because most functions are not leakproof. However, the
issue I'm actually concerned about is whether the *security* qual is
simple enough to permit an index-scan. Anything with an OR clause in
it probably won't be, and any function call definitely won't be.

> I realize that you want multiple policies because you'd like a way for
> the RLS qual to be made simpler for certain cases while also having more
> complex quals for other cases. What I keep waiting to hear is exactly
> how you want to specify which policy is used because that's where it
> gets ugly and complicated. I still really don't like the idea of trying
> to apply multiple policies inside of a single query execution.

See above comments.

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

Robert Haas wrote:

> > Right, if we were to support multiple policies on a given table then we
> > would have to support adding and removing them individually, as well as
> > specify when they are to be applied- and what if that "when" overlaps?
> > Do we apply both and only a row which passed them all gets sent to the
> > user? Essentially we'd be defining the RLS policies to be AND'd
> > together, right? Would we want to support both AND-based and OR-based,
> > and allow users to pick what set of conditionals they want applied to
> > their various overlapping RLS policies?
>
> AND is not a sensible policy; it would need to be OR. If you grant
> someone access to two different subsets of the rows in a table, it
> stands to reason that they will expect to have access to all of the
> rows that are in at least one of those subsets.

I haven't been following this thread, but this bit caught my attention.
I'm not sure I agree that OR is always the right policy either.
There is a case for a policy that says "forbid these rows to these guys,
even if they have read permissions from elsewhere". If OR is the only
way to mix multiple policies there might not be a way to implement this.
So ISTM each policy must be able to indicate what to do -- sort of how
PAM config files allow you to specify "required", "optional" and so
forth for each module.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jun 24, 2014 at 10:30 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> > Right, if we were to support multiple policies on a given table then we
>> > would have to support adding and removing them individually, as well as
>> > specify when they are to be applied- and what if that "when" overlaps?
>> > Do we apply both and only a row which passed them all gets sent to the
>> > user? Essentially we'd be defining the RLS policies to be AND'd
>> > together, right? Would we want to support both AND-based and OR-based,
>> > and allow users to pick what set of conditionals they want applied to
>> > their various overlapping RLS policies?
>>
>> AND is not a sensible policy; it would need to be OR. If you grant
>> someone access to two different subsets of the rows in a table, it
>> stands to reason that they will expect to have access to all of the
>> rows that are in at least one of those subsets.
>
> I haven't been following this thread, but this bit caught my attention.
> I'm not sure I agree that OR is always the right policy either.
> There is a case for a policy that says "forbid these rows to these guys,
> even if they have read permissions from elsewhere". If OR is the only
> way to mix multiple policies there might not be a way to implement this.
> So ISTM each policy must be able to indicate what to do -- sort of how
> PAM config files allow you to specify "required", "optional" and so
> forth for each module.

Hmm. Well, that could be useful, but I'm not sure I'd view it as
something we absolutely have to have...

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

Robert,

I feel like we are getting to the point of simply talking past each
other and so I'll try anew, and I'll include my understanding of how the
different approaches would address the specific use-case you outlined
up-thread.

Single policy
-------------
The current implementation approach only allows a single policy to be
included. The concern raised with this approach is that it won't be
very performant due to the qual complexity, which you outlined
(reformatted a bit) as:

WHERE
sales_rep_id = (SELECT oid FROM pg_roles
WHERE rolname = current_user
AND
oid IN (SELECT id FROM person WHERE is_sales_rep))
OR
partner_id = (SELECT p.org_id
FROM pg_roles a, person p
WHERE a.rolname = current_user
AND a.oid = p.id)

Which I take to mean there is a 'person' table which looks like:

id, is_sales_rep, org_id

and a table which has the RLS qual which looks like:

pk_id, sales_rep_id, partner_id

Then, if the individual is_sales_rep and it's their account by
sales_rep_id, or if the individual's org_id matches the partner_id, they
can see the record.

Using this example with security barrier views and indexes on person.id,
data.pk_id, data.sales_rep_id, and data.partner_id, we'll get a bitmap
heap scan across the 'data' table by having the two OR's run as
InitPlan 1 and InitPlan 2.

Does that address the concern you had around multi-branch OR policies?
This works with more than two OR branches also, though of course we need
appropriate indexes to make use of a Bitmap Heap Scan.

Even with per-user policies, we would define a policy along these lines,
for the "sfrost" role:

WHERE
sales_rep_id = 16384
OR partner_id = 1

Which also ends up doing a Bitmap Heap Scan across the data table.

For the case where a sales rep isn't also a partner, you could simplify
this to:

WHERE
sales_rep_id = 16384

but I'm not sure that really buys you much? With the bitmap heap
scan, if one side of the OR ends up not returning anything then it
doesn't contribute to the blocks which have to be scanned. The index
might still need to be scanned, although I think you could avoid even
that with an EXISTS check to see if the user is a partner at all.
That's not to say that a bitmap scan is equivilant to an index scan, but
it's certainly likely to be far better than a sequential scan.

Now, if the query is "select * from data_view with pk_id = 1002;", then
we get an indexed lookup on the data table based on the PK. That's what
I was trying to point out previously regarding leakproof functions
(which comprise about half of the boolean functions we provide, if I
recall my previous analysis correctly). We also get indexed lookups
with "pk_id < 10" or similar as those are also leakproof.

Multiple, Overlapping policies
------------------------------
Per discussion, these would generally be OR'd together.

Building up the overall qual which has to include an OR branch for each
individual policy qual(s) looks like a complicated bit of work and one
which might be better left to the user (and, as just pointed out, the
user may actually want AND instead of OR in some cases..).

Managing the plan cache in a sensible way is certainly made more
complicated by this and might mean that it can't be used at all, which
has already been raised as a show-stopper issue.

In the example which you provided, while we could represent that the two
policies exist (sales representatives vs partners) and that they are to
be OR'd together in the catalog, but I don't immediately see how that
would change the qual which ends up being added to the query in this
case or really improving the overall query plan; at least, not without
eliminating one of the OR branches somehow- which I discuss below.

Multiple, Non-overlapping policies
----------------------------------
Preventing the overlap of policies ends up being very complicated if
many dimensions are allowed. For the simple case, perhaps only the
'current role' dimension is useful. I expect that going down that
route would very quickly lead to requests for other dimensions (client
IP, etc) which is why I'm not a big fan of it, but if that's the
concensus then let's work out the syntax and update the patch and move
on.

Another option might be to have a qual for each policy which
the user can define that indicates if that policy is to be applied or
not and then simply pick the first policy for which that qual which
returns 'true'. We would require an ordering to be defined in this
case, which I believe was an issue up-thread. If we allow all policies
matching the quals then we run into the complications mentioned under
"Overlapping policies" above.

If we decide that per-role policies need to be supported, I very
quickly see the need to have "groups" of roles to which a policy is to
be applied. This would differ from roles today as they would not be
allowed to overlap (otherwise we are into overlapping policies again, or
having to figure out which of the overlapping policies should be applied
for each query; another option would be to error at run-time, but that
seems pretty ugly). In this case we would still need to support "all"
as an option, which is what I would expect to have implemented for 9.5,
or at least in the early part of 9.5 (I really don't want to wait until
the last CF or even the CF before that to get anything in as I suspect
it will have grown by that point to be large enough to be an issue..),
adding the per-role(s) option could be for 9.6/10.0.

In your example, if sales representatives have distinct roles from
partners, then the specific policy could be chosen and used based on
which role is running the query, which might lead to, perhaps only
maginal, improved performance in those specific cases, as discussed
above.

General multi-policy concerns
-----------------------------
Choosing which policy or policies to apply for a given query gets very
complicated very quickly if we're to do so in an automated way. Dean
suggests that the user would pick which policy to use, to which I argued
that roles could be used to manage that instead (a user could 'set role'
to a role which has the access requested). That mechanism would also
work in the existing single-policy approach by having a policy which
depends on the calling role (eg: by looking up that role in a table
which defines what access that role should have). It would also work in
the above proposal for multiple non-overlapping policies where the
policy to use is based on the current role.

Overall, while I'm interested in defining where this is going in a way
which allows us implement an initial RLS capability while avoiding
future upgrade issues, I am perfectly happy to say that the 9.5 RLS
implementation may not be exactly syntax-compatible with 9.6 or 10.0.
What I wish to avoid is a case where what's in 9.5 includes RLS
definitions which can't be implemented in 9.6/10.0 and as would cause
upgrade problems. As long as what's in 9.5 can be represented and
supported in 9.6/10.0, we can implement the necessary logic to migrate
from one to the other in pg_dump. We do not guarantee syntax
compatibility between major versions and we often warn users of newer
features that there may be some changes in subsequent releases which
they'll need to address when they upgrade (and, of course, these are
noted in the release notes).

Hopefully this will help us move the discussion forward to a point where
we have a long-term design as well as a short-term goal which is
actionable for 9.5. The current work is around adding the GUCs
discussed previously to the RLS patch and modifying pg_dump to use them,
to address the concerns raised previously about pg_dump running user
code and possibly not having a complete copy of the data.

Thanks,

Stephen

On 24 June 2014 17:27, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Single policy vs Multiple, Overlapping policies vs Multiple, Non-overlapping policies
>

What I was describing upthread was multiple non-overlapping policies.

I disagree that this will be more complicated to use. It's a strict
superset of the single policy functionality, so if you want to do it
all using a single policy then you can. But I think that once the ACLs
reach a certain level of complexity, you probably will want to break
it up into multiple policies, and I think doing so will make things
simpler, not more complicated.

Taking a specific, simplistic example, suppose you had 2 groups of
users - some are normal users who should only be able to access their
own records. For these users, you might have a policy like

WHERE person_id = current_user

which would be highly selective, and probably use an index scan. Then
there might be another group of users who are managers with access to
the records of, say, everyone in their department. This might then be
a more complex qual along the lines of

WHERE person_id IN (SELECT ... FROM person_department
WHERE mgr_id = current_user AND ...)

which might end up being a hash or merge join, depending on any
user-supplied quals.

You _could_ combine those into a single policy, but I think it would
be much better to have 2 distinct policies, since they're 2 very
different queries, for different use cases. Normal users would only be
granted permission to use the normal_user_policy. Managers might be
granted permission to use either the normal_user_policy or the
manager_policy (but not both at the same time).

That's a very simplified example. In more realistic situations there
are likely to be many more classes of users, and trying to enforce all
the logic in a single WHERE clause is likely to get unmanageable, or
inefficient if it involves lots of logic hidden away in functions.
Allowing multiple, non-overlapping policies allows the problem to be
broken up into more manageable pieces, which also makes the planner's
job easier, since only a single, simpler policy is in effect in any
given query.

Regards,
Dean

Thinking about the examples upthread, a separate issue occurs to me
--- when defining a RLS qual, I think that there has to be a syntax to
specify an alias for the main table, so that correlated subqueries can
refer to it. I'm not sure if that's been mentioned in any of the
discussions so far, but it might be quite hard to define certain quals
without it.

Regards,
Dean

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> Thinking about the examples upthread, a separate issue occurs to me
> --- when defining a RLS qual, I think that there has to be a syntax to
> specify an alias for the main table, so that correlated subqueries can
> refer to it. I'm not sure if that's been mentioned in any of the
> discussions so far, but it might be quite hard to define certain quals
> without it.

Yeah, that thought had occured to me also. Have any suggestions about
how to approach that issue? The way triggers have OLD/NEW comes to mind
but I'm not sure how easily that'd work.

Thanks,

Stephen

Dean, all,

Changing the subject of this thread (though keeping it threaded) as
we've really moved on to a much broader discussion.

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 24 June 2014 17:27, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Single policy vs Multiple, Overlapping policies vs Multiple, Non-overlapping policies
>
> What I was describing upthread was multiple non-overlapping policies.

Ok.

> I disagree that this will be more complicated to use. It's a strict
> superset of the single policy functionality, so if you want to do it
> all using a single policy then you can. But I think that once the ACLs
> reach a certain level of complexity, you probably will want to break
> it up into multiple policies, and I think doing so will make things
> simpler, not more complicated.

If we keep it explicitly to per-role only, with only one policy ever
being applied, then perhaps it would be, but I'm not convinced..

> Taking a specific, simplistic example, suppose you had 2 groups of
> users - some are normal users who should only be able to access their
> own records. For these users, you might have a policy like
>
> WHERE person_id = current_user
>
> which would be highly selective, and probably use an index scan. Then
> there might be another group of users who are managers with access to
> the records of, say, everyone in their department. This might then be
> a more complex qual along the lines of
>
> WHERE person_id IN (SELECT ... FROM person_department
> WHERE mgr_id = current_user AND ...)
>
> which might end up being a hash or merge join, depending on any
> user-supplied quals.

Certainly my experience with such a setup is that it includes at least 4
levels (self, manager, director, officer). Now, officer you could
perhaps exclude as being simply RLS-exempt but with such a structure I
would think we'd just make that a special kind of policy (and not chew
up those last 4 bits). As for this example, it's quite naturally done
with a recursive query as it's a tree structure, but if you want to keep
the qual simple and fast, you'd materialize the results of such a query
and simply have:

WHERE EXISTS (SELECT 1 from org_chart
WHERE current_user = emp_id
AND person_id = org_chart.id)

> You _could_ combine those into a single policy, but I think it would
> be much better to have 2 distinct policies, since they're 2 very
> different queries, for different use cases. Normal users would only be
> granted permission to use the normal_user_policy. Managers might be
> granted permission to use either the normal_user_policy or the
> manager_policy (but not both at the same time).

I can't recall a system where managers have to request access to their
manager role. Having another way of changing the permissions which are
applied to a session (the existing one being 'set role') doesn't strike
me as a great idea either.

> That's a very simplified example. In more realistic situations there
> are likely to be many more classes of users, and trying to enforce all
> the logic in a single WHERE clause is likely to get unmanageable, or
> inefficient if it involves lots of logic hidden away in functions.

Functions and external security systems are exactly the real-world
use-case which users I've talked to are looking for. All of this
discussion is completely orthogonal to their requirements. I understand
that there are simpler use-cases than those and we may be able to
provide an approach which performs better for those.

> Allowing multiple, non-overlapping policies allows the problem to be
> broken up into more manageable pieces, which also makes the planner's
> job easier, since only a single, simpler policy is in effect in any
> given query.

Let's try to outline what this would look like then.

Taking your approach, we'd have:

CREATE POLICY p1;
CREATE POLICY p2;

ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
ALTER TABLE t1 SET POLICY p2 TO t1_p2_quals;

GRANT SELECT ON TABLE t1 TO role1 USING p1;
GRANT SELECT ON TABLE t1 TO role2 USING p2;

I'm guessing we would need to further support:

GRANT INSERT ON TABLE t1 TO role1 USING p2;

as we've already discussed being able to support per-action (SELECT,
INSERT, UPDATE, DELETE) policies. I'm not quite sure how to address
that though.

Further, as you mention, users would be able to do:

SET rls_policy = whatever;

and things would appear fine, until they tried to access a table to
which they didn't have that policy for, at which point they'd get an
error.

You mention:

GRANT SELECT (col1, col2), UPDATE (col1) ON t1 TO bob USING policy1;

but, to be clear, there would be no option for policies to be
column-specific, right? The policy would apply to the whole row and
just the SELECT/UPDATE privileges would be on the specific columns (as
exists today).

From this what I'm gathering is that we'd need catalog tables along
these lines:

rls_policy
oid, polname name, polowner oid, polnamespace oid, polacl aclitme[]
(oid, policy name, policy owner, policy namespace, ACL, eg: usage?)

rls_policy_table
ptblpolid oid, ptblrelid oid, ptblquals text(?), ptblacl aclitem[]?
(policy oid, table/relation oid, quals, ACL)

pg_class
relhasrls boolean ?

An extension to the existing ACLs which are for GRANT to include a
policy OID, eg:

typedef struct AclItem
{
Oid ai_grantee;
Oid ai_grantor;
AclMode ai_privs;
Oid rls_policy;
}

and further:

role1=r|p1/postgres
role2=r|p2/postgres

or even:

bob=|policy1/postgres

with no table-level privileges and only column-level privileges granted
to role3 for this table.

The plan cache would include what policy OID a given plan was run under
(with InvalidOid indicating an "everything-allowed" policy).

This doesn't address the concern raised about having different policies
depending on the action type (SELECT, INSERT, etc) though, as mentioned
above.. For that we may have to add "Oid rls_select_policy", etc, to
AclItem, which would be pretty painful. Other thoughts?

This certainly feels like quite a bit to try and bite off for 9.5 and,
as mentioned, this would be a strict superset of the current approach,
which could be implemented under this structure as:

CREATE POLICY t1_p1_policy;
ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
GRANT (user's rights) ON t1 TO user USING policy1;

Tha main downside here is that we'd have to create a policy for every
table in the system which had RLS applied, to avoid granting more than
should be. Perhaps the 9.4 approach could include the 'CREATE POLICY'
and 'ALTER TABLE' bits, but not the GRANT parts, meaning that we would,
for the 9.5 -> 9.6 upgrade, pg_dump:

GRANT (user's rights) ON t1 TO user USING policy1;

We would still need the GUCs for "rls_enable = on/off" and perhaps the
role-level "bypass_rls" attribute, but those wouldn't change with this.

Thoughts?

Thanks!

Stephen

On 06/24/2014 10:30 PM, Alvaro Herrera wrote:
> I haven't been following this thread, but this bit caught my attention.
> I'm not sure I agree that OR is always the right policy either.
> There is a case for a policy that says "forbid these rows to these guys,
> even if they have read permissions from elsewhere".

That's generally considered a "DENY" policy, a concept borrowed from ACLs.

You have access to a resource if:

- You have at least one policy that gives you access AND
- You have no policies that deny you access

> If OR is the only
> way to mix multiple policies there might not be a way to implement this.

I think that's a "later" myself, but we shouldn't design ourselves into
a corner where we can't support deny rules either.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig,

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 06/24/2014 10:30 PM, Alvaro Herrera wrote:
> > I haven't been following this thread, but this bit caught my attention.
> > I'm not sure I agree that OR is always the right policy either.
> > There is a case for a policy that says "forbid these rows to these guys,
> > even if they have read permissions from elsewhere".
>
> That's generally considered a "DENY" policy, a concept borrowed from ACLs.

Right.

> > If OR is the only
> > way to mix multiple policies there might not be a way to implement this.
>
> I think that's a "later" myself, but we shouldn't design ourselves into
> a corner where we can't support deny rules either.

Agreed, but I don't want to get so wrapped up in all of this that we end
up with a set of requirements so long that we'll never be able to
accomplish them all in a single release...

Thanks!

Stephen

On 25 June 2014 01:49, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Dean, all,
>
> Changing the subject of this thread (though keeping it threaded) as
> we've really moved on to a much broader discussion.
>
> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
>> On 24 June 2014 17:27, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > Single policy vs Multiple, Overlapping policies vs Multiple, Non-overlapping policies
>>
>> What I was describing upthread was multiple non-overlapping policies.
>
> Ok.
>
>> I disagree that this will be more complicated to use. It's a strict
>> superset of the single policy functionality, so if you want to do it
>> all using a single policy then you can. But I think that once the ACLs
>> reach a certain level of complexity, you probably will want to break
>> it up into multiple policies, and I think doing so will make things
>> simpler, not more complicated.
>
> If we keep it explicitly to per-role only, with only one policy ever
> being applied, then perhaps it would be, but I'm not convinced..
>
>> Taking a specific, simplistic example, suppose you had 2 groups of
>> users - some are normal users who should only be able to access their
>> own records. For these users, you might have a policy like
>>
>> WHERE person_id = current_user
>>
>> which would be highly selective, and probably use an index scan. Then
>> there might be another group of users who are managers with access to
>> the records of, say, everyone in their department. This might then be
>> a more complex qual along the lines of
>>
>> WHERE person_id IN (SELECT ... FROM person_department
>> WHERE mgr_id = current_user AND ...)
>>
>> which might end up being a hash or merge join, depending on any
>> user-supplied quals.
>
> Certainly my experience with such a setup is that it includes at least 4
> levels (self, manager, director, officer). Now, officer you could
> perhaps exclude as being simply RLS-exempt but with such a structure I
> would think we'd just make that a special kind of policy (and not chew
> up those last 4 bits). As for this example, it's quite naturally done
> with a recursive query as it's a tree structure, but if you want to keep
> the qual simple and fast, you'd materialize the results of such a query
> and simply have:
>
> WHERE EXISTS (SELECT 1 from org_chart
> WHERE current_user = emp_id
> AND person_id = org_chart.id)
>
>> You _could_ combine those into a single policy, but I think it would
>> be much better to have 2 distinct policies, since they're 2 very
>> different queries, for different use cases. Normal users would only be
>> granted permission to use the normal_user_policy. Managers might be
>> granted permission to use either the normal_user_policy or the
>> manager_policy (but not both at the same time).
>
> I can't recall a system where managers have to request access to their
> manager role. Having another way of changing the permissions which are
> applied to a session (the existing one being 'set role') doesn't strike
> me as a great idea either.
>

Actually I think it's quite common to build applications where more
privileged users might want to initially log in with normal
privileges, and then only escalate to a higher privilege level if
needed (much like only being root on a machine when absolutely
necessary). But as you say, that can be done through 'set role' so I
don't think being able to choose between policies is as important as
being able to define different policies for different roles.

>> That's a very simplified example. In more realistic situations there
>> are likely to be many more classes of users, and trying to enforce all
>> the logic in a single WHERE clause is likely to get unmanageable, or
>> inefficient if it involves lots of logic hidden away in functions.
>
> Functions and external security systems are exactly the real-world
> use-case which users I've talked to are looking for. All of this
> discussion is completely orthogonal to their requirements. I understand
> that there are simpler use-cases than those and we may be able to
> provide an approach which performs better for those.
>

OK.

>> Allowing multiple, non-overlapping policies allows the problem to be
>> broken up into more manageable pieces, which also makes the planner's
>> job easier, since only a single, simpler policy is in effect in any
>> given query.
>
> Let's try to outline what this would look like then.
>
> Taking your approach, we'd have:
>
> CREATE POLICY p1;
> CREATE POLICY p2;
>
> ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
> ALTER TABLE t1 SET POLICY p2 TO t1_p2_quals;
>
> GRANT SELECT ON TABLE t1 TO role1 USING p1;
> GRANT SELECT ON TABLE t1 TO role2 USING p2;
>
> I'm guessing we would need to further support:
>
> GRANT INSERT ON TABLE t1 TO role1 USING p2;
>
> as we've already discussed being able to support per-action (SELECT,
> INSERT, UPDATE, DELETE) policies. I'm not quite sure how to address
> that though.
>
> Further, as you mention, users would be able to do:
>
> SET rls_policy = whatever;
>
> and things would appear fine, until they tried to access a table to
> which they didn't have that policy for, at which point they'd get an
> error.
>
> You mention:
>
> GRANT SELECT (col1, col2), UPDATE (col1) ON t1 TO bob USING policy1;
>
> but, to be clear, there would be no option for policies to be
> column-specific, right? The policy would apply to the whole row and
> just the SELECT/UPDATE privileges would be on the specific columns (as
> exists today).
>

I think that would be OK for the first release. It could be extended
in a future release to support column-specific policy ACLs, as long as
we don't preclude that in the syntax we choose now. The syntax

GRANT <command> [,<command>] ON table TO role USING policy

works because columns can be added to it later.

> From this what I'm gathering is that we'd need catalog tables along
> these lines:
>
> rls_policy
> oid, polname name, polowner oid, polnamespace oid, polacl aclitme[]
> (oid, policy name, policy owner, policy namespace, ACL, eg: usage?)
>
> rls_policy_table
> ptblpolid oid, ptblrelid oid, ptblquals text(?), ptblacl aclitem[]?
> (policy oid, table/relation oid, quals, ACL)
>
> pg_class
> relhasrls boolean ?
>

Seems about right.

> An extension to the existing ACLs which are for GRANT to include a
> policy OID, eg:
>
> typedef struct AclItem
> {
> Oid ai_grantee;
> Oid ai_grantor;
> AclMode ai_privs;
> Oid rls_policy;
> }
>

Alternatively, use the ACLs on rls_policy_table - i.e., to SELECT from
a table using a particular policy, you would need to have the SELECT
bit assigned to you in the corresponding rls_policy_table entry's
ACLs. That seems like it would be a less invasive change, but I don't
know if there are other problems with that approach.

> and further:
>
> role1=r|p1/postgres
> role2=r|p2/postgres
>

Or just

table1:
role1=rw/grantor
table1 using policy1:
role2=rw/grantor

to avoid changing the privilege display pattern. That's also more in
keeping with the model of storing the per-policy ACLs in
rls_policy_table.

> or even:
>
> bob=|policy1/postgres
>
> with no table-level privileges and only column-level privileges granted
> to role3 for this table.
>

I don't get that last one. If there are no table-level privileges,
would it not just be empty?

> The plan cache would include what policy OID a given plan was run under
> (with InvalidOid indicating an "everything-allowed" policy).
>
> This doesn't address the concern raised about having different policies
> depending on the action type (SELECT, INSERT, etc) though, as mentioned
> above.. For that we may have to add "Oid rls_select_policy", etc, to
> AclItem, which would be pretty painful. Other thoughts?
>

Huh? Isn't it just another column in rls_policy_table to specify the
action type?

> This certainly feels like quite a bit to try and bite off for 9.5 and,
> as mentioned, this would be a strict superset of the current approach,
> which could be implemented under this structure as:
>
> CREATE POLICY t1_p1_policy;
> ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
> GRANT (user's rights) ON t1 TO user USING policy1;
>
> Tha main downside here is that we'd have to create a policy for every
> table in the system which had RLS applied, to avoid granting more than
> should be. Perhaps the 9.4 approach could include the 'CREATE POLICY'
> and 'ALTER TABLE' bits, but not the GRANT parts, meaning that we would,
> for the 9.5 -> 9.6 upgrade, pg_dump:
>
> GRANT (user's rights) ON t1 TO user USING policy1;
>
> We would still need the GUCs for "rls_enable = on/off" and perhaps the
> role-level "bypass_rls" attribute, but those wouldn't change with this.
>
> Thoughts?
>

Well I think you'd have to flesh out the alternatives to a similar
level of detail to assess the relative effort involved, but I think
it's encouraging to see this level of design this early in the 9.5
cycle.

Regards,
Dean

On Tue, Jun 24, 2014 at 12:27 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I feel like we are getting to the point of simply talking past each
> other and so I'll try anew, and I'll include my understanding of how the
> different approaches would address the specific use-case you outlined
> up-thread.

Thanks, you're right, and this is a good write-up.

> Single policy
> -------------
> The current implementation approach only allows a single policy to be
> included.
> [...snip...]
> For the case where a sales rep isn't also a partner, you could simplify
> this to:
>
> WHERE
> sales_rep_id = 16384
>
> but I'm not sure that really buys you much? With the bitmap heap
> scan, if one side of the OR ends up not returning anything then it
> doesn't contribute to the blocks which have to be scanned. The index
> might still need to be scanned, although I think you could avoid even
> that with an EXISTS check to see if the user is a partner at all.
> That's not to say that a bitmap scan is equivilant to an index scan, but
> it's certainly likely to be far better than a sequential scan.

True, but the wins could be much larger if one policy is WHERE
sales_rep_id = (SELECT oid FROM pg_roles WHERE rolname = current_user)
and the other policy is WHERE complexfn(). I'll also throw out a +1
for Dean's comments on this topic.

> Multiple, Non-overlapping policies
> ----------------------------------
> Preventing the overlap of policies ends up being very complicated if
> many dimensions are allowed. For the simple case, perhaps only the
> 'current role' dimension is useful. I expect that going down that
> route would very quickly lead to requests for other dimensions (client
> IP, etc) which is why I'm not a big fan of it, but if that's the
> concensus then let's work out the syntax and update the patch and move
> on.

I think role is good enough. That's the primary identifier for all
access-control related decisions, so it should be good enough here,
too.

I don't really understand your concerns about overlapping policies
being complex. If you've got a couple of WHERE clauses, combining
them with OR is not hard. Now, the query optimizer may have trouble
with it, but on the whole I expect to win more than we lose, by
entirely excluding some branches of an OR for users for whom entirely
policies can be excluded.

> Overall, while I'm interested in defining where this is going in a way
> which allows us implement an initial RLS capability while avoiding
> future upgrade issues, I am perfectly happy to say that the 9.5 RLS
> implementation may not be exactly syntax-compatible with 9.6 or 10.0.

Again, I think that's completely non-viable. Are you going to tell
people they can't pg_upgrade, and they can't dump-and-reload either,
without manual fiddling? There's no way that's going to be accepted.

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

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 25 June 2014 01:49, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I can't recall a system where managers have to request access to their
> > manager role. Having another way of changing the permissions which are
> > applied to a session (the existing one being 'set role') doesn't strike
> > me as a great idea either.
> >
>
> Actually I think it's quite common to build applications where more
> privileged users might want to initially log in with normal
> privileges, and then only escalate to a higher privilege level if
> needed (much like only being root on a machine when absolutely
> necessary). But as you say, that can be done through 'set role' so I
> don't think being able to choose between policies is as important as
> being able to define different policies for different roles.

For those kinds of applications (eg: sudo), yes. I was, perhaps,
looking at your example a bit too literally- I was thinking of HR
management type systems (timecard systems, etc).

> > You mention:
> >
> > GRANT SELECT (col1, col2), UPDATE (col1) ON t1 TO bob USING policy1;
> >
> > but, to be clear, there would be no option for policies to be
> > column-specific, right? The policy would apply to the whole row and
> > just the SELECT/UPDATE privileges would be on the specific columns (as
> > exists today).
> >
>
> I think that would be OK for the first release. It could be extended
> in a future release to support column-specific policy ACLs, as long as
> we don't preclude that in the syntax we choose now. The syntax
>
> GRANT <command> [,<command>] ON table TO role USING policy
>
> works because columns can be added to it later.

What would per-column RLS policies mean..? Would we have to work out
which columns are being updated vs. select'd on before being able to
choose the policy/quals to include? Seems like that's probably workable
but I've not thought about it very hard.

> > From this what I'm gathering is that we'd need catalog tables along
> > these lines:
> >
> > rls_policy
> > oid, polname name, polowner oid, polnamespace oid, polacl aclitme[]
> > (oid, policy name, policy owner, policy namespace, ACL, eg: usage?)
> >
> > rls_policy_table
> > ptblpolid oid, ptblrelid oid, ptblquals text(?), ptblacl aclitem[]?
> > (policy oid, table/relation oid, quals, ACL)
> >
> > pg_class
> > relhasrls boolean ?
>
> Seems about right.
>
> > An extension to the existing ACLs which are for GRANT to include a
> > policy OID, eg:
> >
> > typedef struct AclItem
> > {
> > Oid ai_grantee;
> > Oid ai_grantor;
> > AclMode ai_privs;
> > Oid rls_policy;
> > }
> >
>
> Alternatively, use the ACLs on rls_policy_table - i.e., to SELECT from
> a table using a particular policy, you would need to have the SELECT
> bit assigned to you in the corresponding rls_policy_table entry's
> ACLs. That seems like it would be a less invasive change, but I don't
> know if there are other problems with that approach.

Ah, that's a good thought. My original thinking for that column was
some kind of privilege structure around who is allowed to modify the
quals for a given policy+table, but using that as the definition of who
has what policies does make sense and means we can leave AclItem
more-or-less alone, which is very nice. The relhasrls boolean would
allow us to only query that catalog in cases where a policy exists,
hopefully minimizing the impact for users who are not using RLS.

> > and further:
> >
> > role1=r|p1/postgres
> > role2=r|p2/postgres
>
> Or just
>
> table1:
> role1=rw/grantor
> table1 using policy1:
> role2=rw/grantor
>
> to avoid changing the privilege display pattern. That's also more in
> keeping with the model of storing the per-policy ACLs in
> rls_policy_table.

I like that output, but do we expect any pushback from users who parse
out that field? Admittedly, they really shouldn't be doing that, but
I'm sure most actually do, and "table1 using policy1" isn't terribly
nice to parse.

> > or even:
> >
> > bob=|policy1/postgres
> >
> > with no table-level privileges and only column-level privileges granted
> > to role3 for this table.
>
> I don't get that last one. If there are no table-level privileges,
> would it not just be empty?

No, as there could be column-level privileges. Note that table-level
privileges get you access to all columns, and column level privileges
(as defined by SQL spec) give you access even if you don't have any
table-level privileges. As I was trying to exclude the notion of
column-level policies, I figured policies would always show up in the
"table" level ACL fields, but if there aren't any table-level rights,
what would that look like? With your proposal, it'd be:

table1 using policy1:
bob=/grantor

?

> > The plan cache would include what policy OID a given plan was run under
> > (with InvalidOid indicating an "everything-allowed" policy).
> >
> > This doesn't address the concern raised about having different policies
> > depending on the action type (SELECT, INSERT, etc) though, as mentioned
> > above.. For that we may have to add "Oid rls_select_policy", etc, to
> > AclItem, which would be pretty painful. Other thoughts?
> >
>
> Huh? Isn't it just another column in rls_policy_table to specify the
> action type?

I had been trying to fit it into the ACL structure somehow. What would
it look like to have multiple action types then? Here's one thought:

table1 using policy1 for INSERT:
bob=rw/grantor
table1 using policy1 for SELECT:
bob=r/grantor

Or how about:

table1|policy1/w:
bob=rw/grantor
table1|policy1/r:
bob=r/grantor

Another question is about showing what the actual quals are for a given
policy which is being applied to a table. Would we want that to show up
in \d, \d+, or only be available through querying the catalog..?

> > This certainly feels like quite a bit to try and bite off for 9.5 and,
> > as mentioned, this would be a strict superset of the current approach,
> > which could be implemented under this structure as:
> >
> > CREATE POLICY t1_p1_policy;
> > ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
> > GRANT (user's rights) ON t1 TO user USING policy1;
> >
> > Tha main downside here is that we'd have to create a policy for every
> > table in the system which had RLS applied, to avoid granting more than
> > should be. Perhaps the 9.4 approach could include the 'CREATE POLICY'
> > and 'ALTER TABLE' bits, but not the GRANT parts, meaning that we would,
> > for the 9.5 -> 9.6 upgrade, pg_dump:
> >
> > GRANT (user's rights) ON t1 TO user USING policy1;
> >
> > We would still need the GUCs for "rls_enable = on/off" and perhaps the
> > role-level "bypass_rls" attribute, but those wouldn't change with this.
>
> Well I think you'd have to flesh out the alternatives to a similar
> level of detail to assess the relative effort involved, but I think
> it's encouraging to see this level of design this early in the 9.5
> cycle.

I'm not sure which other alternatives you're thinking about here- could
you be more specific..? I can try to flesh them out but I had actually
been hoping that this would be a good compromise position among the
alternatives.

This provides the per-role policy granularity which has been mentioned a
few times, but doesn't allow the policies to overlap. Overlapping
policies could be added to this general design, I believe, though we'd
have to make a few catalog changes and invent some new syntax to define
how the policies are to be combined (ANDs vs ORs, etc). I had brought
up the idea of ordering/prioritizing policies, but I didn't particularly
like the suggestion when I made it and I don't recall anyone else
voicing interest in that approach.

For my part, I don't see the GUCs as really being "alternatives" so much
as pre-requisites. Even with all the granularity and comprehensive set
of features which we're talking about here, we're going to need a way
for pg_dump to simply say "do not apply RLS to me and ERROR out if
that's an issue". I agree that it's great to get these design
discussions happening now but I really do not want this to become a
behemoth patch by the last CF and ends up bounced because of that.

What I'd like to work through is the minimal set which would be accepted
and get that in, in a way that doesn't prevent further improvements, and
then see what can be done to get those improvements and refinements in
during the 9.5 cycle and what gets bounced to the next release. To that
end, I've been trying to gauge interest in this and get some feel for
who is interested in helping push this forward- your help was
instrumental in getting updatable security barrier views into 9.4, would
you have time to help with this also..?

Thanks!

Stephen

Robert, all,

Changing the thread topic to match the other one, and adding Dean in
explicitly since we're talking about the design discussed with him.

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> I think role is good enough. That's the primary identifier for all
> access-control related decisions, so it should be good enough here,
> too.

Alright. That works for me.

> I don't really understand your concerns about overlapping policies
> being complex. If you've got a couple of WHERE clauses, combining
> them with OR is not hard. Now, the query optimizer may have trouble
> with it, but on the whole I expect to win more than we lose, by
> entirely excluding some branches of an OR for users for whom entirely
> policies can be excluded.

On the thread with Dean we're proposing some specific catalog designs
and part of that included (fleshing it out a bit more) something like:

CREATE TABLE pg_relrlspolicy (-- relation RLS policy table
ptblrelid oid, -- Relation/table
ptblaction text, -- SELECT, INSERT, UPDATE, DELETE
ptblpolid oid, -- Policy
ptblquals text, -- Quals to add
ptblacl aclitem[], -- Rights to use this policy on the table

primary key (ptblrelid, ptblaction)
);

And note that I had expected aclitem to only include one entry per role.

To support overlapping policies, we could add 'ptblpolid' into the
primary key and then simply extract out all of the entries for the
relation and action that we're currently running and step through each
to find which of the policies apply to the current_role...?

If a role has policyA with 'INSERT' rights, but no rights to SELECT, but
they also have an entry for policyB with 'SELECT' rights, we would use
only policyB for a SELECT query? Does that approach mean we don't need
'ptblaction' after all? I'm thinking this approach would also toss out
the "pick your policy" concept that Dean had proposed up-thread.

How would these interact with the existing table-level rights? For
column-level rights, if you have access to SELECT the column then you
don't need any table-level rights (and the table-level rights mean you
can SELECT from any column), are we thinking the same would apply here,
such that having 'USING POLICY' rights means you can SELECT from the
table and the table-level rights end up being the 'DIRECT' rights which
had been discussed up-thread? Not sure that I like that approach,
though I understand some others might find it appealing.. As we're
integrating this with the GRANT command, perhaps it'd be alright.

> > Overall, while I'm interested in defining where this is going in a way
> > which allows us implement an initial RLS capability while avoiding
> > future upgrade issues, I am perfectly happy to say that the 9.5 RLS
> > implementation may not be exactly syntax-compatible with 9.6 or 10.0.
>
> Again, I think that's completely non-viable. Are you going to tell
> people they can't pg_upgrade, and they can't dump-and-reload either,
> without manual fiddling? There's no way that's going to be accepted.

I don't understand what you're getting at here. We dump the catalog
using the newer version of pg_dump for pg_upgrade and we could handle
any *syntax* change required during that process to ensure that the same
access is granted in the new cluster as existed in the old cluster.

We do the exact same thing every time we add a new reserved keyword-
anything which used that keyword before ends up getting double-quoted by
the new version of pg_dump and both pg_dump and pg_upgrade work just
fine. We routinly break some syntax compatibility between major
versions, address those changes in the newer version of pg_dump, and
move on.

I am not proposing that users won't be able to upgrade from 9.5 to 9.6
if they have RLS and agree that it'd be a non-starter.

Thanks,

Stephen

On Tue, Jun 24, 2014 at 8:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Let's try to outline what this would look like then.
>
> Taking your approach, we'd have:
>
> CREATE POLICY p1;
> CREATE POLICY p2;
>
> ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
> ALTER TABLE t1 SET POLICY p2 TO t1_p2_quals;

This seems like a very nice, flexible framework.

> GRANT SELECT ON TABLE t1 TO role1 USING p1;
> GRANT SELECT ON TABLE t1 TO role2 USING p2;

Instead of doing it this way, we could instead do:

ALTER ROLE role1 ADD POLICY p1;
ALTER ROLE role2 ADD POLICY p2;

We could possibly allow multiple policies to be set for the same user,
but given an error (or OR the quals together) if there are conflicting
policies for the same table. A user with no policies would see
everything to which they've been granted access.

To support different policies on different operations, you could have
something like:

ALTER TABLE t1 SET POLICY p1 ON INSERT TO t1_p1_quals;

Without the ON clause, it would establish the given policy for all operations.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Jun 24, 2014 at 8:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Let's try to outline what this would look like then.
> >
> > Taking your approach, we'd have:
> >
> > CREATE POLICY p1;
> > CREATE POLICY p2;
> >
> > ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
> > ALTER TABLE t1 SET POLICY p2 TO t1_p2_quals;
>
> This seems like a very nice, flexible framework.
>
> > GRANT SELECT ON TABLE t1 TO role1 USING p1;
> > GRANT SELECT ON TABLE t1 TO role2 USING p2;
>
> Instead of doing it this way, we could instead do:
>
> ALTER ROLE role1 ADD POLICY p1;
> ALTER ROLE role2 ADD POLICY p2;
>
> We could possibly allow multiple policies to be set for the same user,
> but given an error (or OR the quals together) if there are conflicting
> policies for the same table. A user with no policies would see
> everything to which they've been granted access.

Ok, I like that as it means that "normal" GRANTs, etc, remain the same
and are just constrained by RLS when there is an RLS policy on the
table, which I believe is really the right approach.

> To support different policies on different operations, you could have
> something like:
>
> ALTER TABLE t1 SET POLICY p1 ON INSERT TO t1_p1_quals;
>
> Without the ON clause, it would establish the given policy for all operations.

Right, this makes sense also and is similar to what we were angling
towards initially. I'll think further on this and propose a catalog
structure and try to delve into the semantics of query operations, etc.

One issue that occurs to me is trying to think through how to address
the plancache invalidation, such that we are not invalidating constantly
but also setting the correct quals for the current query. We had gone
down a road where we saved a plan for each role under which a query was
run but then ripped that out because the RLS policy would handle the
per-role issues (modulo whether RLS should be enabled or not). This
approach means that we'd have to bring back the notion of per-role plan
cacheing. I'm not against that, just making note of it.

Thanks,

Stephen

On 25 June 2014 16:44, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jun 24, 2014 at 8:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Let's try to outline what this would look like then.
>>
>> Taking your approach, we'd have:
>>
>> CREATE POLICY p1;
>> CREATE POLICY p2;
>>
>> ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
>> ALTER TABLE t1 SET POLICY p2 TO t1_p2_quals;
>
> This seems like a very nice, flexible framework.
>
>> GRANT SELECT ON TABLE t1 TO role1 USING p1;
>> GRANT SELECT ON TABLE t1 TO role2 USING p2;
>
> Instead of doing it this way, we could instead do:
>
> ALTER ROLE role1 ADD POLICY p1;
> ALTER ROLE role2 ADD POLICY p2;
>
> We could possibly allow multiple policies to be set for the same user,
> but given an error (or OR the quals together) if there are conflicting
> policies for the same table. A user with no policies would see
> everything to which they've been granted access.
>

I'm a bit uneasy about allowing overlapping policies like this,
because I think it is more likely to lead to unintended consequences
than solve real use cases. For example, suppose you define policies p1
and p2 and set them up on table t1, and you grant role1 permissions on
t1 and allow role1 the use of policy p1. Then you set up policy p2 on
another table t2, and decide you want to allow role1 access to t2
using this policy. The only way to do it is to add p2 to role1, but
doing so also then gives role1 access to t1 using p2, which might not
have been what you intended.

With the GRANT ... USING policy syntax, you have greater flexibility
to pick and choose which policies each user has permission to use with
each table. To me at least, that seems much less error prone, since
you are being much more explicit about exactly what privileges you are
granting. The ALTER ROLE ... ADD POLICY syntax is potentially adding a
whole bunch of extra privileges to the role, and you have to work
quite hard to see exactly what it's adding.

> To support different policies on different operations, you could have
> something like:
>
> ALTER TABLE t1 SET POLICY p1 ON INSERT TO t1_p1_quals;
>
> Without the ON clause, it would establish the given policy for all operations.
>

Yes, that makes sense. But as I was arguing above, I think the ACLs
should be attached to the specific RLS policy identified uniquely by
(table, policy, command). So, for example, if you did

ALTER TABLE t1 SET POLICY p1 ON SELECT TO t1_p1_sel_quals;
ALTER TABLE t1 SET POLICY p1 ON UPDATE TO t1_p1_upd_quals;

you could also do

GRANT SELECT ON TABLE t1 TO role1 USING p1;
GRANT UPDATE ON TABLE t1 TO role1 USING p1;

but it would be an error to do

GRANT DELETE ON TABLE t1 TO role1 USING p1;

because there is no p1 delete policy for t1;

Regards,
Dean

On Wed, Jun 25, 2014 at 4:48 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> Instead of doing it this way, we could instead do:
>>
>> ALTER ROLE role1 ADD POLICY p1;
>> ALTER ROLE role2 ADD POLICY p2;
>>
>> We could possibly allow multiple policies to be set for the same user,
>> but given an error (or OR the quals together) if there are conflicting
>> policies for the same table. A user with no policies would see
>> everything to which they've been granted access.
>>
> I'm a bit uneasy about allowing overlapping policies like this,
> because I think it is more likely to lead to unintended consequences
> than solve real use cases. For example, suppose you define policies p1
> and p2 and set them up on table t1, and you grant role1 permissions on
> t1 and allow role1 the use of policy p1. Then you set up policy p2 on
> another table t2, and decide you want to allow role1 access to t2
> using this policy. The only way to do it is to add p2 to role1, but
> doing so also then gives role1 access to t1 using p2, which might not
> have been what you intended.

I guess that's true but it just seems like a configuration error. I
have it in mind that most people will define policies for
non-overlapping sets of tables and then apply those policies as
appropriate to each user. Whether that's true or not, I don't see it
as being materially different from granting membership in a role - you
could easily give the user permission to do stuff they shouldn't be
able to do, but if you don't carefully examine the bundle of
privileges that come with that GRANT before executing on it, that's
your fault, not the system's.

>> To support different policies on different operations, you could have
>> something like:
>>
>> ALTER TABLE t1 SET POLICY p1 ON INSERT TO t1_p1_quals;
>>
>> Without the ON clause, it would establish the given policy for all operations.
>
> Yes, that makes sense. But as I was arguing above, I think the ACLs
> should be attached to the specific RLS policy identified uniquely by
> (table, policy, command). So, for example, if you did
>
> ALTER TABLE t1 SET POLICY p1 ON SELECT TO t1_p1_sel_quals;
> ALTER TABLE t1 SET POLICY p1 ON UPDATE TO t1_p1_upd_quals;
>
> you could also do
>
> GRANT SELECT ON TABLE t1 TO role1 USING p1;
> GRANT UPDATE ON TABLE t1 TO role1 USING p1;
>
> but it would be an error to do
>
> GRANT DELETE ON TABLE t1 TO role1 USING p1;

As I see it, the downside of this is that it gets a lot more complex.
We have to revise the ACL representation, which is already pretty darn
complicated, to keep track not only of the grantee, grantor, and
permissions, but also the policies qualifying those permissions. The
changes to GRANT will need to propagate into GRANT ON ALL TABLES IN
SCHEMA and AFTER DEFAULT PRIVILEGES. There is administrative
complexity as well, because if you want to policy-protect an
additional table, you've got to add the table to the policy and then
update all the grants as well. I think what will happen in practice
is that people will grant to PUBLIC all rights on the policy, and then
do all the access control through the GRANT statements.

An interesting question we haven't much considered is: who can set up
policies and add then to users? Maybe we should flip this around, and
instead of adding users to policies, we should exempt users from
policies.

CREATE POLICY p1;

And then, if they own p1 and t1, they can do:

ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
(or maybe we should associate it to the policy instead of the table:
ALTER POLICY p1 SET TABLE t1 TO t1_p1_quals)

And then the policy applies to everyone who doesn't have the grantable
EXEMPT privilege on the policy. The policy owner and superuser have
that privilege by default and it can be handed out to others like
this:

GRANT EXEMPT ON POLICY p1 TO snowden;

Then users who have row_level_security=on will bypass RLS if possible,
and otherwise it will be applied. Users who have
row_level_security=off will bypass RLS if possible, and otherwise
error. And users who have row_level_security=force will apply RLS
even if they are entitled to bypass it.

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

On 26 June 2014 18:04, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> ALTER TABLE t1 SET POLICY p1 ON SELECT TO t1_p1_sel_quals;
>> GRANT SELECT ON TABLE t1 TO role1 USING p1;
>
> As I see it, the downside of this is that it gets a lot more complex.
> We have to revise the ACL representation, which is already pretty darn
> complicated, to keep track not only of the grantee, grantor, and
> permissions, but also the policies qualifying those permissions. The
> changes to GRANT will need to propagate into GRANT ON ALL TABLES IN
> SCHEMA and AFTER DEFAULT PRIVILEGES.

No, it can be done without any changes to the permissions code by
storing the ACLs on the catalog entries where the RLS quals are held,
rather than modifying the ACL items on the table. I.e., instead of
thinking of "USING polname" as a modifier to the grant, think of it as
as an additional qualifier on the thing being granted.

That means the syntax I proposed earlier is wrong/misleading. Instead of

GRANT SELECT ON TABLE tbl TO role USING polname;

it should really be

GRANT SELECT USING polname ON TABLE tbl TO role;

> There is administrative
> complexity as well, because if you want to policy-protect an
> additional table, you've got to add the table to the policy and then
> update all the grants as well. I think what will happen in practice
> is that people will grant to PUBLIC all rights on the policy, and then
> do all the access control through the GRANT statements.
>

If you assume that most users will only have one policy through which
they can access any given table, then there is no more administrative
overhead than we have right now. Right now you have to grant each user
permissions on each table you define. The only difference is that now
you throw in a "USING polname". We could also simplify administration
by supporting

GRANT SELECT USING polname ON ALL TABLES IN SCHEMA sch TO role;

The important distinction is that this is only granting permissions on
tables that exist now, not on tables that might be created later.

> An interesting question we haven't much considered is: who can set up
> policies and add then to users? Maybe we should flip this around, and
> instead of adding users to policies, we should exempt users from
> policies.
>
> CREATE POLICY p1;
>
> And then, if they own p1 and t1, they can do:
>
> ALTER TABLE t1 SET POLICY p1 TO t1_p1_quals;
> (or maybe we should associate it to the policy instead of the table:
> ALTER POLICY p1 SET TABLE t1 TO t1_p1_quals)
>
> And then the policy applies to everyone who doesn't have the grantable
> EXEMPT privilege on the policy. The policy owner and superuser have
> that privilege by default and it can be handed out to others like
> this:
>
> GRANT EXEMPT ON POLICY p1 TO snowden;
>
> Then users who have row_level_security=on will bypass RLS if possible,
> and otherwise it will be applied. Users who have
> row_level_security=off will bypass RLS if possible, and otherwise
> error. And users who have row_level_security=force will apply RLS
> even if they are entitled to bypass it.
>