Re: Review of Row Level Security

Simon Riggs wrote:

> Each table has a single security clause. The clause doesn't enforce
> that it must contain something that depends on role, but that is the
> most easily understood usage of it. We do that to ensure that you can
> embed the intelligence into a function, say hasRowLevelAccess(), which
> doesn't have any provable relationship on role, and maybe wouldn't do
> anything in unit test, yet clearly in production it would do so.
>
> It would be easy enough to include read/write variable clauses by
> using a function similar to TG_OP for triggers, e.g. StatementType.
> That would make the security clause that applied only to reads into
> something like this (StatementType('INSERT, UPDATE') OR other-quals).

In the case where the logic in encapsulated into a function, what
are the logical differences from a BEFORE EACH ROW trigger? If
none, and this is strictly an optimization, what are the benchmarks
showing?

-Kevin

2012/12/21 Kevin Grittner <kgrittn(at)mail(dot)com>:
> Simon Riggs wrote:
>
>> Each table has a single security clause. The clause doesn't enforce
>> that it must contain something that depends on role, but that is the
>> most easily understood usage of it. We do that to ensure that you can
>> embed the intelligence into a function, say hasRowLevelAccess(), which
>> doesn't have any provable relationship on role, and maybe wouldn't do
>> anything in unit test, yet clearly in production it would do so.
>>
>> It would be easy enough to include read/write variable clauses by
>> using a function similar to TG_OP for triggers, e.g. StatementType.
>> That would make the security clause that applied only to reads into
>> something like this (StatementType('INSERT, UPDATE') OR other-quals).
>
> In the case where the logic in encapsulated into a function, what
> are the logical differences from a BEFORE EACH ROW trigger? If
> none, and this is strictly an optimization, what are the benchmarks
> showing?
>
It seems to me we need some more discussion about design and
implementation on row-security checks of writer-side, to reach our
consensus.
On the other hand, we are standing next to the consensus about
reader-side; a unique row-security policy (so, first version does not
support per-command policy) shall be checked on table scanning
on select, update or delete commands.
I'd like to suggest these arguable stuff being postponed to v9.4,
and we like to focus on the minimum / core functionality towards
the deadline of v9.3.
I think it is the most productive way to enhance our features.

I can understand Simon's opinion; security feature should not be
defective item. However, please don't forget sepgsql started from
just a small functional module to check DML permissions only.
How about folk's opinion?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

KaiGai, all,

* Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
> 2012/12/21 Kevin Grittner <kgrittn(at)mail(dot)com>:
> > Simon Riggs wrote:
> >
> >> Each table has a single security clause. The clause doesn't enforce
> >> that it must contain something that depends on role, but that is the
> >> most easily understood usage of it. We do that to ensure that you can
> >> embed the intelligence into a function, say hasRowLevelAccess(), which
> >> doesn't have any provable relationship on role, and maybe wouldn't do
> >> anything in unit test, yet clearly in production it would do so.
> >>
> >> It would be easy enough to include read/write variable clauses by
> >> using a function similar to TG_OP for triggers, e.g. StatementType.
> >> That would make the security clause that applied only to reads into
> >> something like this (StatementType('INSERT, UPDATE') OR other-quals).
> >
> > In the case where the logic in encapsulated into a function, what
> > are the logical differences from a BEFORE EACH ROW trigger? If
> > none, and this is strictly an optimization, what are the benchmarks
> > showing?

I'm trying to understand this piece also. It sounds like all we're
doing at the moment is adding different syntax to define a trigger
function and that's hardly what I, at least, was expecting. If a
function has to be written to have RLS, then I think we've failed. Much
of the point of this is to provide an easy solution which gets all the
details right for users who aren't comfortable or savvy enough to just
write functions/security definer views/etc themselves.

> It seems to me we need some more discussion about design and
> implementation on row-security checks of writer-side, to reach our
> consensus.

Again, I agree with Kevin on this- there should be a wiki or similar
which actually outlines the high-level design, syntax, etc. That would
help us understand and be able to meaningfully comment about the
approach.

> On the other hand, we are standing next to the consensus about
> reader-side; a unique row-security policy (so, first version does not
> support per-command policy) shall be checked on table scanning
> on select, update or delete commands.

I don't feel that we've really reached a consensus about the
'reader-side' implemented in this patch- rather, we've agreed (at a
pretty high level) what the default impact of RLS for SELECT queries is.
While I'm glad that we were able to do that, I'm rather dismayed that it
took a great deal of discussion to get to that point.

Thanks,

Stephen

On 21 December 2012 22:01, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

>> On the other hand, we are standing next to the consensus about
>> reader-side; a unique row-security policy (so, first version does not
>> support per-command policy) shall be checked on table scanning
>> on select, update or delete commands.
>
> I don't feel that we've really reached a consensus about the
> 'reader-side' implemented in this patch- rather, we've agreed (at a
> pretty high level) what the default impact of RLS for SELECT queries is.
> While I'm glad that we were able to do that, I'm rather dismayed that it
> took a great deal of discussion to get to that point.

Would anybody like to discuss this on a conference call on say 28th
Dec, to see if we can agree a way forwards? I feel certain that we can
work through any difficulties and agree a minimal subset for change.
All comers welcome, just contact me offlist for details.

I've got literally nothing riding on this, other than my desire for
progress, so I don't see the need for going too many extra miles if
nobody else does.

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

Simon,

* Simon Riggs (simon(at)2ndQuadrant(dot)com) wrote:
> Would anybody like to discuss this on a conference call on say 28th
> Dec, to see if we can agree a way forwards? I feel certain that we can
> work through any difficulties and agree a minimal subset for change.
> All comers welcome, just contact me offlist for details.

Thank you for the offer, I agree that it's a good idea. I'd be happy
to (and would like to) participate.

> I've got literally nothing riding on this, other than my desire for
> progress, so I don't see the need for going too many extra miles if
> nobody else does.

I'd like to make progress also but let's make sure we're going in the
right direction. :)

Thanks again,

Stephen

2012/12/21 Stephen Frost <sfrost(at)snowman(dot)net>:
>> It seems to me we need some more discussion about design and
>> implementation on row-security checks of writer-side, to reach our
>> consensus.
>
> Again, I agree with Kevin on this- there should be a wiki or similar
> which actually outlines the high-level design, syntax, etc. That would
> help us understand and be able to meaningfully comment about the
> approach.
>
I also. RLS entry of wiki has not been updated for long time, I'll try to
update the entry for high-level design in a couple of days.

>> On the other hand, we are standing next to the consensus about
>> reader-side; a unique row-security policy (so, first version does not
>> support per-command policy) shall be checked on table scanning
>> on select, update or delete commands.
>
> I don't feel that we've really reached a consensus about the
> 'reader-side' implemented in this patch- rather, we've agreed (at a
> pretty high level) what the default impact of RLS for SELECT queries is.
> While I'm glad that we were able to do that, I'm rather dismayed that it
> took a great deal of discussion to get to that point.
>
> Thanks,
>
> Stephen
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2012/12/22 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> On 21 December 2012 22:01, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
>>> On the other hand, we are standing next to the consensus about
>>> reader-side; a unique row-security policy (so, first version does not
>>> support per-command policy) shall be checked on table scanning
>>> on select, update or delete commands.
>>
>> I don't feel that we've really reached a consensus about the
>> 'reader-side' implemented in this patch- rather, we've agreed (at a
>> pretty high level) what the default impact of RLS for SELECT queries is.
>> While I'm glad that we were able to do that, I'm rather dismayed that it
>> took a great deal of discussion to get to that point.
>
> Would anybody like to discuss this on a conference call on say 28th
> Dec, to see if we can agree a way forwards? I feel certain that we can
> work through any difficulties and agree a minimal subset for change.
> All comers welcome, just contact me offlist for details.
>
Of course, I'll join the conference. Please give me the detail.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 21 December 2012 16:51, Kevin Grittner <kgrittn(at)mail(dot)com> wrote:

>> It would be easy enough to include read/write variable clauses by
>> using a function similar to TG_OP for triggers, e.g. StatementType.
>> That would make the security clause that applied only to reads into
>> something like this (StatementType('INSERT, UPDATE') OR other-quals).
>
> In the case where the logic in encapsulated into a function, what
> are the logical differences from a BEFORE EACH ROW trigger?

Logically it is broadly similar to a CHECK constraint, which is also
similar to a trigger in a few ways.

Implemented as a BEFORE EACH ROW trigger it would need to be a new
class of trigger that always executed after all other BEFORE EACH ROW
triggers had executed, in the same way that security barrier views act
last. The "act last" bit seems to be the most important thing here,
just as it was for security barriers and by analogy a string enough
reason to add specific syntax to support this case. (Syntax as yet
undecided...)

> If
> none, and this is strictly an optimization, what are the benchmarks
> showing?

AFAIK its well known that a check constraint is much faster than a
trigger. The objection to using triggers is partially for that reason
and partially because of the admin overhead, as I already said.

Adding a trigger could be done automatically, just as the current
patch does with the check constraint approach. So if anyone has
benchmarks that show triggers are actually faster, then we could add
that automatically instead, I guess.

Anyway, hope you can make call on 28th so we can discuss this and
agree a way forwards you're happy with.

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

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 21 December 2012 16:51, Kevin Grittner <kgrittn(at)mail(dot)com> wrote:
>> If none, and this is strictly an optimization, what are the benchmarks
>> showing?

> AFAIK its well known that a check constraint is much faster than a
> trigger.

I don't believe that that's "well known" at all, at least not for
apples-to-apples comparison cases. A C-coded BEFORE trigger doesn't
have very much overhead; I suspect it's probably comparable to
expression evaluation setup overhead. I think if you want to argue
for this on performance grounds, you need to actually prove there's
a significant performance advantage, not just assume there will be.

regards, tom lane

On 23 December 2012 19:16, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On 21 December 2012 16:51, Kevin Grittner <kgrittn(at)mail(dot)com> wrote:
>>> If none, and this is strictly an optimization, what are the benchmarks
>>> showing?
>
>> AFAIK its well known that a check constraint is much faster than a
>> trigger.
>
> I don't believe that that's "well known" at all, at least not for
> apples-to-apples comparison cases. A C-coded BEFORE trigger doesn't
> have very much overhead; I suspect it's probably comparable to
> expression evaluation setup overhead. I think if you want to argue
> for this on performance grounds, you need to actually prove there's
> a significant performance advantage, not just assume there will be.

If you want to see some tests, I'm sure those can be arranged, no
problem. But honestly, if its low enough, then which is the fastest
will likely be moot in comparison with the cost of a non-C coded
role-based security check. So I think our attention is best spent on
providing a few likely C-coded security checks, so we're able to
address the whole performance concern not just the constraint/trigger
debate.

That still leaves the points about ensuring the trigger/checks are
executed last and also that they are added automatically, rather than
requiring them to be added manually. As KaiGai points out, if they are
added automatically, it doesn't really matter which we pick.

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

On 23 December 2012 18:49, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Anyway, hope you can make call on 28th so we can discuss this and
> agree a way forwards you're happy with.

Stephen, KaiGai and myself met by phone on 28th to discuss.

1. The actual default is not that important to any of us. We could go
either way, or have no default at all.

2. What we do want is a declarative way of specifying row security,
with options to support all use cases discussed/requested on list. We
shouldn't
support just one of those use cases and force everybody else to use
triggers manually for the other cases.

3. We want to have the possibility of multiple row security
expressions, defined for different privilege types (SELECT, UPDATE,
INSERT, DELETE). (Note that this means you'd be able to specify that
an update could read a row in one security mode by setting SELECT,
then update that row to a new security mode by setting a clause on
UPDATE - hence we refer to those as privileges not commands/events).
The expressions should be separate so they can be pushed easily into
query plans (exactly as in the current patch).

Stephen has updated the Wiki with some ideas on how that can be structured
https://wiki.postgresql.org/wiki/RLS

4. Supporting multiple expressions may not be possible for 9.3, but if
not, we want to agree now what the syntax is to make sure we have a
clear route for future development. If we can agree this quickly we
increase the chances of KaiGai successfully implementing that.

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

2012/12/31 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> On 23 December 2012 18:49, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
>> Anyway, hope you can make call on 28th so we can discuss this and
>> agree a way forwards you're happy with.
>
> Stephen, KaiGai and myself met by phone on 28th to discuss.
>
> 1. The actual default is not that important to any of us. We could go
> either way, or have no default at all.
>
> 2. What we do want is a declarative way of specifying row security,
> with options to support all use cases discussed/requested on list. We
> shouldn't
> support just one of those use cases and force everybody else to use
> triggers manually for the other cases.
>
> 3. We want to have the possibility of multiple row security
> expressions, defined for different privilege types (SELECT, UPDATE,
> INSERT, DELETE). (Note that this means you'd be able to specify that
> an update could read a row in one security mode by setting SELECT,
> then update that row to a new security mode by setting a clause on
> UPDATE - hence we refer to those as privileges not commands/events).
> The expressions should be separate so they can be pushed easily into
> query plans (exactly as in the current patch).
>
> Stephen has updated the Wiki with some ideas on how that can be structured
> https://wiki.postgresql.org/wiki/RLS
>
> 4. Supporting multiple expressions may not be possible for 9.3, but if
> not, we want to agree now what the syntax is to make sure we have a
> clear route for future development. If we can agree this quickly we
> increase the chances of KaiGai successfully implementing that.
>
The syntax being discussed were below:

ALTER TABLE <relname> SET ROW SECURITY FOR <privilege> TO (<expression>);
ALTER TABLE <relname> RESET ROW SECURITY FOR <privilege>;

<privilege> can be one of: ALL, SELECT, INSERT, UPDATE, DELETE

The point in development towards v9.3 is, we only support "ALL" but
we can add other command types in the future.
IMO, only "parser" should accept command types except for ALL but
raise an error something like "it is not supported yet" to protect from
syntax conflicts.

Right now, I plan to submit a revised patch with the syntax support
above, and without support for INSERT or NEW of UPDATE checks,
as minimum set of functionality for v9.3.

Please give me some suggestions, if you have different opinion
towards the overall direction, until 15th-Jan.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Jan 02, 2013 at 05:35:13PM +0100, Kohei KaiGai wrote:
> 2012/12/31 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> > On 23 December 2012 18:49, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >
> >> Anyway, hope you can make call on 28th so we can discuss this and
> >> agree a way forwards you're happy with.
> >
> > Stephen, KaiGai and myself met by phone on 28th to discuss.
> >
> > 1. The actual default is not that important to any of us. We could go
> > either way, or have no default at all.
> >
> > 2. What we do want is a declarative way of specifying row security,
> > with options to support all use cases discussed/requested on list. We
> > shouldn't
> > support just one of those use cases and force everybody else to use
> > triggers manually for the other cases.
> >
> > 3. We want to have the possibility of multiple row security
> > expressions, defined for different privilege types (SELECT, UPDATE,
> > INSERT, DELETE). (Note that this means you'd be able to specify that
> > an update could read a row in one security mode by setting SELECT,
> > then update that row to a new security mode by setting a clause on
> > UPDATE - hence we refer to those as privileges not commands/events).
> > The expressions should be separate so they can be pushed easily into
> > query plans (exactly as in the current patch).
> >
> > Stephen has updated the Wiki with some ideas on how that can be structured
> > https://wiki.postgresql.org/wiki/RLS
> >
> > 4. Supporting multiple expressions may not be possible for 9.3, but if
> > not, we want to agree now what the syntax is to make sure we have a
> > clear route for future development. If we can agree this quickly we
> > increase the chances of KaiGai successfully implementing that.
> >
> The syntax being discussed were below:
>
> ALTER TABLE <relname> SET ROW SECURITY FOR <privilege> TO (<expression>);
> ALTER TABLE <relname> RESET ROW SECURITY FOR <privilege>;
>
> <privilege> can be one of: ALL, SELECT, INSERT, UPDATE, DELETE
>
> The point in development towards v9.3 is, we only support "ALL" but
> we can add other command types in the future.
> IMO, only "parser" should accept command types except for ALL but
> raise an error something like "it is not supported yet" to protect from
> syntax conflicts.

Great!

Would COPY be covered separately? How about TRUNCATE?

Also, is there any way to apply this to the catalog, or would that be
too large a restructuring, given how catalog access actually works?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 2 January 2013 17:19, David Fetter <david(at)fetter(dot)org> wrote:

> Would COPY be covered separately? How about TRUNCATE?

COPY == INSERT

TRUNCATE isn't covered at all since it doesn't look at rows. It has a
separate privilege that can be granted to those that need it.

> Also, is there any way to apply this to the catalog, or would that be
> too large a restructuring, given how catalog access actually works?

Doubt it.

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

On Wed, Jan 02, 2013 at 05:31:42PM +0000, Simon Riggs wrote:
> On 2 January 2013 17:19, David Fetter <david(at)fetter(dot)org> wrote:
>
> > Would COPY be covered separately? How about TRUNCATE?
>
> COPY == INSERT

Makes sense. The reason I mentioned it is that COPY is supposed to be
a very fast bulk loading process, which implies a couple of things:

1. In the RLS (really should be RLAC, but let's not go there now)
case, COPY makes it pretty simple to probe hugely many things at once
for existence unless there's some kind of COPY pre-processor that
throws away non-matching rows. Fortunately there's work being done to
that end.

2. COPY, being intended to be very, very fast, should probably get
some kind of notation, at least in the docs, about how it will slow
down in the RLS case.

> TRUNCATE isn't covered at all since it doesn't look at rows. It has a
> separate privilege that can be granted to those that need it.

OK

> > Also, is there any way to apply this to the catalog, or would that
> > be too large a restructuring, given how catalog access actually
> > works?
>
> Doubt it.

Somewhat related issue: Is there a worked example of setting up
PostgreSQL to a "default deny" access policy as far as is possible
today? This touches a lot of things, among them reading the catalog.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

KaiGai,

* Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
> IMO, only "parser" should accept command types except for ALL but
> raise an error something like "it is not supported yet" to protect from
> syntax conflicts.

Right, I agree with this.

> Right now, I plan to submit a revised patch with the syntax support
> above, and without support for INSERT or NEW of UPDATE checks,
> as minimum set of functionality for v9.3.

Sounds good.

Thanks,

Stephen

The attached patch is row-security v9.

According to the upthread discussion, I adjusted the syntax as follows:

ALTER TABLE <table> SET ROW SECURITY FOR <cmd> TO (<expression>);
ALTER TABLE <table> RESET ROW SECURITY FOR <cmd>;

It seems to me "FOR <cmd>" might be omissible as synonym of "FOR ALL".
User needs to input this clause everytime, so they may feel it troublesome.

As previous version doing, references to a table with row-security policy is
replaced by a simple sub-query that scans the target table with configured
row-security policy.

postgres=> ALTER TABLE t1 SET ROW SECURITY FOR ALL TO (a % 2 = 0);
ALTER TABLE
postgres=> ALTER TABLE t2 SET ROW SECURITY FOR ALL TO (a % 2 = 1);
ALTER TABLE
postgres=> EXPLAIN SELECT * FROM t1 WHERE f_leak(b);
QUERY PLAN
---------------------------------------------------------------------------
Result (cost=0.00..50.82 rows=413 width=36)
-> Append (cost=0.00..50.82 rows=413 width=36)
-> Subquery Scan on t1 (cost=0.00..0.01 rows=1 width=36)
Filter: f_leak(t1.b)
-> Seq Scan on t1 t1_1 (cost=0.00..0.00 rows=1 width=36)
Filter: ((a % 2) = 0)
-> Subquery Scan on t2 (cost=0.00..28.51 rows=2 width=36)
Filter: f_leak(t2.b)
-> Seq Scan on t2 t2_1 (cost=0.00..28.45 rows=6 width=36)
Filter: ((a % 2) = 1)
-> Seq Scan on t3 (cost=0.00..22.30 rows=410 width=36)
Filter: f_leak(b)
(12 rows)

In case of UPDATE or DELETE, row-security also prevent to modify
rows that does not satisfy the configured security policy.

postgres=> EXPLAIN UPDATE t1 SET b=b || '_update' WHERE b like '%abc%';
QUERY PLAN
---------------------------------------------------------------------
Update on t1 (cost=0.00..54.04 rows=51 width=42)
-> Subquery Scan on t1_1 (cost=0.00..0.02 rows=1 width=42)
Filter: (t1_1.b ~~ '%abc%'::text)
-> Seq Scan on t1 t1_2 (cost=0.00..0.00 rows=1 width=42)
Filter: ((a % 2) = 0)
-> Subquery Scan on t2 (cost=0.00..28.53 rows=1 width=42)
Filter: (t2.b ~~ '%abc%'::text)
-> Seq Scan on t2 t2_1 (cost=0.00..28.45 rows=6 width=42)
Filter: ((a % 2) = 1)
-> Seq Scan on t3 (cost=0.00..25.50 rows=49 width=42)
Filter: (b ~~ '%abc%'::text)
(11 rows)

One significant change to the planner is, planner had to accept cases
that result relation is not identical with source relation being replaced
to row-security subquery. E.g, constructed plan for UPDATE may scans
tuples from a sub-query with rtindex=5 then update the relation with
rtindex=1. Some existing code assumes result relation is also source
relation, so it was my headache during the development.
Even though the current implementation is working for all the test cases
in regression test as I expected, I'm not 100% certain whether this
implementation is the best way. So, it's welcome if we can have better
and stable implementation than my proposition.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 01/16/2013 06:28 AM, Kohei KaiGai wrote:
> The attached patch is row-security v9.
Has anyone had a chance to check this out? It's seen a lot of work over
a long time and looks really valuable if it's solid enough.

Kohei KaiGai, is there any chance you can go through Simon's review and
offer an itemized response showing what changes you've made related to
each review point? It might help encourage movement on this patch if
it's easier to see what's changed since the last discussion and review.

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

The attached patch is rebased one towards the latest master branch, with
some documentation / source code updates. Except for these cosmetic
changes, nothing has been changed.

What this patch tries to do is simple; that replaces reference to tables with
pre-configured row-security policy by a sub-query that simply scans this
table with row-security policy and security-barrier flag.
I expect nobody has strong arguments towards this basic design.

However, some detailed implementations has not been reviewed so much
deeply. For example, when system-column or whole-row of the table with
row-security policy are referenced, rowsecurity.c adds the row-security
subquery target-entries to reference underlying system-columns or
whole-row reference, then it also fixed up Var->varattno that originally
referenced these columns because subquery has no system columns
and subquery's record type is not compatible with underlying table.
If table has inherited children, it also needs to have additional fixup
at prep/preptlist.c or /prep/prepunion.c.

It is much helpful if someone give me comments around these
arguable portions from the standpoint with fresh eyes.

Thanks,

2013/1/15 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> The attached patch is row-security v9.
>
> According to the upthread discussion, I adjusted the syntax as follows:
>
> ALTER TABLE <table> SET ROW SECURITY FOR <cmd> TO (<expression>);
> ALTER TABLE <table> RESET ROW SECURITY FOR <cmd>;
>
> It seems to me "FOR <cmd>" might be omissible as synonym of "FOR ALL".
> User needs to input this clause everytime, so they may feel it troublesome.
>
> As previous version doing, references to a table with row-security policy is
> replaced by a simple sub-query that scans the target table with configured
> row-security policy.
>
> postgres=> ALTER TABLE t1 SET ROW SECURITY FOR ALL TO (a % 2 = 0);
> ALTER TABLE
> postgres=> ALTER TABLE t2 SET ROW SECURITY FOR ALL TO (a % 2 = 1);
> ALTER TABLE
> postgres=> EXPLAIN SELECT * FROM t1 WHERE f_leak(b);
> QUERY PLAN
> ---------------------------------------------------------------------------
> Result (cost=0.00..50.82 rows=413 width=36)
> -> Append (cost=0.00..50.82 rows=413 width=36)
> -> Subquery Scan on t1 (cost=0.00..0.01 rows=1 width=36)
> Filter: f_leak(t1.b)
> -> Seq Scan on t1 t1_1 (cost=0.00..0.00 rows=1 width=36)
> Filter: ((a % 2) = 0)
> -> Subquery Scan on t2 (cost=0.00..28.51 rows=2 width=36)
> Filter: f_leak(t2.b)
> -> Seq Scan on t2 t2_1 (cost=0.00..28.45 rows=6 width=36)
> Filter: ((a % 2) = 1)
> -> Seq Scan on t3 (cost=0.00..22.30 rows=410 width=36)
> Filter: f_leak(b)
> (12 rows)
>
> In case of UPDATE or DELETE, row-security also prevent to modify
> rows that does not satisfy the configured security policy.
>
> postgres=> EXPLAIN UPDATE t1 SET b=b || '_update' WHERE b like '%abc%';
> QUERY PLAN
> ---------------------------------------------------------------------
> Update on t1 (cost=0.00..54.04 rows=51 width=42)
> -> Subquery Scan on t1_1 (cost=0.00..0.02 rows=1 width=42)
> Filter: (t1_1.b ~~ '%abc%'::text)
> -> Seq Scan on t1 t1_2 (cost=0.00..0.00 rows=1 width=42)
> Filter: ((a % 2) = 0)
> -> Subquery Scan on t2 (cost=0.00..28.53 rows=1 width=42)
> Filter: (t2.b ~~ '%abc%'::text)
> -> Seq Scan on t2 t2_1 (cost=0.00..28.45 rows=6 width=42)
> Filter: ((a % 2) = 1)
> -> Seq Scan on t3 (cost=0.00..25.50 rows=49 width=42)
> Filter: (b ~~ '%abc%'::text)
> (11 rows)
>
> One significant change to the planner is, planner had to accept cases
> that result relation is not identical with source relation being replaced
> to row-security subquery. E.g, constructed plan for UPDATE may scans
> tuples from a sub-query with rtindex=5 then update the relation with
> rtindex=1. Some existing code assumes result relation is also source
> relation, so it was my headache during the development.
> Even though the current implementation is working for all the test cases
> in regression test as I expected, I'm not 100% certain whether this
> implementation is the best way. So, it's welcome if we can have better
> and stable implementation than my proposition.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 19 March 2013 15:08, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> It is much helpful if someone give me comments around these
> arguable portions from the standpoint with fresh eyes.

My feeling at this point is that I don't think I should commit this
and related patches without more work.

I certainly have time and willingness to do that in the next release
cycle, but I feel like we need substantially longer to confirm that
this is as rock solid as it needs to be.

With everybody's permission, I'd like to move this to the next CF,
with apologies to KaiGai.

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

2013/3/25 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> On 19 March 2013 15:08, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>
>> It is much helpful if someone give me comments around these
>> arguable portions from the standpoint with fresh eyes.
>
> My feeling at this point is that I don't think I should commit this
> and related patches without more work.
>
> I certainly have time and willingness to do that in the next release
> cycle, but I feel like we need substantially longer to confirm that
> this is as rock solid as it needs to be.
>
> With everybody's permission, I'd like to move this to the next CF,
> with apologies to KaiGai.
>
I have to admit it will become time to move v9.4 development cycle
soon, and row-level security patch still needs some more works to
merge into the core.
At least, it does not stand on 40km point at marathon.

One thing we need to consider is, the current version of RLS patch
has cut-down functionality about writer side on INSERT / UPDATE
commands. So, we anyway needed to work on this feature on v9.4
development cycle.
So, I can agree to move this patch to the v9.4 development cycle.
Also, I'd like to have discussion for this feature in earlier half of
v9.4 to keep time for the remaining features, such as check on
writer-side, integration with selinux, and so on.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

I moved Row Level Security patch to the 2013-Next commitfest.

2013/3/27 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2013/3/25 Simon Riggs <simon(at)2ndquadrant(dot)com>:
>> On 19 March 2013 15:08, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>
>>> It is much helpful if someone give me comments around these
>>> arguable portions from the standpoint with fresh eyes.
>>
>> My feeling at this point is that I don't think I should commit this
>> and related patches without more work.
>>
>> I certainly have time and willingness to do that in the next release
>> cycle, but I feel like we need substantially longer to confirm that
>> this is as rock solid as it needs to be.
>>
>> With everybody's permission, I'd like to move this to the next CF,
>> with apologies to KaiGai.
>>
> I have to admit it will become time to move v9.4 development cycle
> soon, and row-level security patch still needs some more works to
> merge into the core.
> At least, it does not stand on 40km point at marathon.
>
> One thing we need to consider is, the current version of RLS patch
> has cut-down functionality about writer side on INSERT / UPDATE
> commands. So, we anyway needed to work on this feature on v9.4
> development cycle.
> So, I can agree to move this patch to the v9.4 development cycle.
> Also, I'd like to have discussion for this feature in earlier half of
> v9.4 to keep time for the remaining features, such as check on
> writer-side, integration with selinux, and so on.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 27 March 2013 10:57, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2013/3/25 Simon Riggs <simon(at)2ndquadrant(dot)com>:
>> On 19 March 2013 15:08, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>
>>> It is much helpful if someone give me comments around these
>>> arguable portions from the standpoint with fresh eyes.
>>
>> My feeling at this point is that I don't think I should commit this
>> and related patches without more work.
>>
>> I certainly have time and willingness to do that in the next release
>> cycle, but I feel like we need substantially longer to confirm that
>> this is as rock solid as it needs to be.
>>
>> With everybody's permission, I'd like to move this to the next CF,
>> with apologies to KaiGai.
>>
> I have to admit it will become time to move v9.4 development cycle
> soon, and row-level security patch still needs some more works to
> merge into the core.
> At least, it does not stand on 40km point at marathon.
>
> One thing we need to consider is, the current version of RLS patch
> has cut-down functionality about writer side on INSERT / UPDATE
> commands. So, we anyway needed to work on this feature on v9.4
> development cycle.
> So, I can agree to move this patch to the v9.4 development cycle.
> Also, I'd like to have discussion for this feature in earlier half of
> v9.4 to keep time for the remaining features, such as check on
> writer-side, integration with selinux, and so on.

Thanks for your hard work, and understanding.

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