API change advice: Passing plan invalidation info from the rewriter into the planner?

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

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

Craig Ringer <craig@hobby.2ndquadrant.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

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

On 03/06/2014 02:58 AM, Tom Lane wrote:

Craig Ringer <craig@hobby.2ndquadrant.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

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

Craig Ringer <craig@2ndquadrant.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

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

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@sss.pgh.pa.us) wrote:

Craig Ringer <craig@2ndquadrant.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@snowman.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

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.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-----

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

* Craig Ringer (craig@2ndquadrant.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.brightwell@crunchydatasolutions.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

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

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

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

Craig Ringer <craig@2ndquadrant.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

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

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

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

Craig Ringer <craig@2ndquadrant.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

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

On Tue, Jun 10, 2014 at 7:18 PM, Craig Ringer <craig@2ndquadrant.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

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

* Craig Ringer (craig@2ndquadrant.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@2ndquadrant.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