Thread: Review of Row Level Security
Patch looks good and also like it will/can be ready for 9.3. I'm happy to put time into this as committer and/or reviewer and take further responsibility for it, unless others wish to. LIKES * It's pretty simple to understand and use * Check qual is stored in pre-parsed form. That is fast, and also secure, since changing search_path of the user doesn't change the security check at all. Nice. * Performance overhead is low, integration with indexing is clear and effective and it works with partitioning * It works, apart from design holes listed below, easily plugged IMHO DISLIKEs * Who gets to see stats on the underlying table? Are the stats protected by security? How does ANALYZE work? * INSERT ignores the SECURITY clause, on the ground that this has no meaning. So its possible to INSERT data you can't see. For example, you can insert medical records for *another* patient, or insert new top secret information. This also causes a security hole... since inserted rows can violate defined constraints, letting you know that other keys you can't see exist. Why don't we treat the SECURITY clause as a CHECK constraint? That makes intuitive sense to me. * UPDATE allows you to bypass the SECURITY clause, to produce new rows that you can't see. (Not good). But you can't get them back again, cos you can't see them. * TRUNCATE works, and allows you to remove all rows of a table, even ones you can't see to run a DELETE on. Er... None of those things are cool, at all. Oracle defaults to putting VPD on all event types: INSERT, UPDATE, DELETE, SELECT. ISTM we should be doing the same, not just say "we can add an INSERT trigger if you want". Adding a trigger just begs the question as to why we are bothering in the first place, since this functionality could already be added by INSERT, UPDATE or DELETE triggers, if they are a full replacement for this feature. The only answer is "ease of use" We can easily add syntax like this [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]] with the default being "ALL" * The design has nothing at all to do with SECURITY LABELs. Why did we create them, I wonder? I understood we would have row-level label security. Doesn't that require us to have a data type, such as reglabel or similar enum? Seems strange. Oracle has two features: Oracle Label Security and Row Level Security - OTHER * The docs should explain a little better how to optimize using RLS. Specifically, the fact that indexable operators are marked leakproof and thus can be optimized ahead of the rlsquals. The docs say "rls quals are guaranteed to be applied first" which isn't true in all cases. * Why is pg_rowlevelsec in a separate catalog table? * Internally, I think we should call this "rowsecurity" rather than "rowlevelsec" - the "level" word is just noise, whereas the "security" word benefits from being spelt out in full. * psql \d support needed * Docs need work, but thats OK. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Thanks for your reviewing in spite of large number of lines. My comments are below. 2012/12/4 Simon Riggs <simon@2ndquadrant.com>: > Patch looks good and also like it will/can be ready for 9.3. I'm happy > to put time into this as committer and/or reviewer and take further > responsibility for it, unless others wish to. > > LIKES > > * It's pretty simple to understand and use > > * Check qual is stored in pre-parsed form. That is fast, and also > secure, since changing search_path of the user doesn't change the > security check at all. Nice. > > * Performance overhead is low, integration with indexing is clear and > effective and it works with partitioning > > * It works, apart from design holes listed below, easily plugged IMHO > > > DISLIKEs > > * Who gets to see stats on the underlying table? Are the stats > protected by security? How does ANALYZE work? > I think, ANALYZE should perform on the raw tables without row-security policy. Even though statistics are "gray-zone", it is not a complete set of the raw table contents, so all we can do is just implying the original from processed statistical values. The situation is similar to iteration of probe using PK/FK violation. In general, it is called covert channel, and out of the scope in regular access control mechanism (including MAC). So, I don't think we have special protection on pg_stat even if row- security is configured. > * INSERT ignores the SECURITY clause, on the ground that this has no > meaning. So its possible to INSERT data you can't see. For example, > you can insert medical records for *another* patient, or insert new > top secret information. This also causes a security hole... since > inserted rows can violate defined constraints, letting you know that > other keys you can't see exist. Why don't we treat the SECURITY clause > as a CHECK constraint? That makes intuitive sense to me. > > * UPDATE allows you to bypass the SECURITY clause, to produce new rows > that you can't see. (Not good). But you can't get them back again, cos > you can't see them. > The above two comments seems me that you are suggesting to apply checks on both of scanning rows stage (UPDATE case) and modifying rows stage (INSERT and UPDATE), to prevent touchable rows getting gone anywhere. In the previous discussion, it was suggested we can implement this feature using before-row trigger. However, I love the idea to support same row-security policy integrated with CHECK constraint; that kills individual user's operation to define triggers. One problem is a case when row-security policy contains SubLink node. I expect it takes a special case handling, however, also guess not hard to implement so much. Let me investigate the code around here. > * TRUNCATE works, and allows you to remove all rows of a table, even > ones you can't see to run a DELETE on. Er... > It was my oversight. My preference is to rewrite TRUNCATE command with DELETE statement in case when row-security policy is active on the target table. In this case, a NOTICE message may be helpful for users not to assume the table is always empty after the command. > None of those things are cool, at all. > > Oracle defaults to putting VPD on all event types: INSERT, UPDATE, > DELETE, SELECT. ISTM we should be doing the same, not just say "we can > add an INSERT trigger if you want". > > Adding a trigger just begs the question as to why we are bothering in > the first place, since this functionality could already be added by > INSERT, UPDATE or DELETE triggers, if they are a full replacement for > this feature. The only answer is "ease of use" > > We can easily add syntax like this > > [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]] > > with the default being "ALL" > I think it is flaw of Oracle. :-) In case when user can define leakable function, it enables to leak contents of invisible rows at the timing when executor fetch the rows, prior to modification stage, even if we allows to configure individual row-security policies for SELECT and DELETE or UPDATE commands. My preference is one policy on a particular table for all the commands. > * The design has nothing at all to do with SECURITY LABELs. Why did we > create them, I wonder? I understood we would have row-level label > security. Doesn't that require us to have a data type, such as > reglabel or similar enum? Seems strange. Oracle has two features: > Oracle Label Security and Row Level Security - > I think it should be implemented on the next step. It additionally takes two independent features (1) functionality to inject a column to store security label at table definition. (2) functionality to assign a default security label when a new row is inserted. As Oracle constructs OLS on the top of VPD feature, the base row- security feature shall be upstreamed first. > OTHER > > * The docs should explain a little better how to optimize using RLS. > Specifically, the fact that indexable operators are marked leakproof > and thus can be optimized ahead of the rlsquals. The docs say "rls > quals are guaranteed to be applied first" which isn't true in all > cases. > Indeed. It should be updated as: although mechanism guarantees to evaluate this condition earlier than any other user givencondition without LEAKPROOF flag (that means qualifier can have side-effects, thus it possibly leaks rows should beinvisible.) > * Why is pg_rowlevelsec in a separate catalog table? > To define dependency towards functions, operators or relations being referenced with SubLinks. If we store row-security policy within pg_class catalog, here is no way to distinguish a dependency records due to row- security policy or others, thus it makes problem when user wants to replace the row-security policy. Do you have a good idea? If this problem can be solved, I can prefer an approach to store the policy within pg_class. > * Internally, I think we should call this "rowsecurity" rather than > "rowlevelsec" - the "level" word is just noise, whereas the "security" > word benefits from being spelt out in full. > OK, I'll update them. > * psql \d support needed > Are you suggesting to print out full qualifiers of row-security? Or, a mark to indicate whether row-security is configured, or not? > * Docs need work, but thats OK. > I'd like to want some help with native English speakers. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> * TRUNCATE works, and allows you to remove all rows of a table, even >> ones you can't see to run a DELETE on. Er... >> > It was my oversight. My preference is to rewrite TRUNCATE command > with DELETE statement in case when row-security policy is active on > the target table. > In this case, a NOTICE message may be helpful for users not to assume > the table is always empty after the command. I think the default must be to throw an ERROR, since part of the contract with TRUNCATE is that it is fast and removes storage. >> * Docs need work, but thats OK. >> > I'd like to want some help with native English speakers. I'll help with that. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> Oracle defaults to putting VPD on all event types: INSERT, UPDATE, >> DELETE, SELECT. ISTM we should be doing the same, not just say "we can >> add an INSERT trigger if you want". >> >> Adding a trigger just begs the question as to why we are bothering in >> the first place, since this functionality could already be added by >> INSERT, UPDATE or DELETE triggers, if they are a full replacement for >> this feature. The only answer is "ease of use" >> >> We can easily add syntax like this >> >> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]] >> >> with the default being "ALL" >> > I think it is flaw of Oracle. :-) Agreed > In case when user can define leakable function, it enables to leak contents > of invisible rows at the timing when executor fetch the rows, prior to > modification > stage, even if we allows to configure individual row-security policies > for SELECT > and DELETE or UPDATE commands. > My preference is one policy on a particular table for all the commands. Yes, only one security policy allowed. Question is, should we offer the option to enforce it on a subset of command types. That isn't anything I can see a need for myself. >> * psql \d support needed >> > Are you suggesting to print out full qualifiers of row-security? > Or, a mark to indicate whether row-security is configured, or not? One of those options, yes -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
2012/12/7 Simon Riggs <simon@2ndquadrant.com>: > On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > >>> * TRUNCATE works, and allows you to remove all rows of a table, even >>> ones you can't see to run a DELETE on. Er... >>> >> It was my oversight. My preference is to rewrite TRUNCATE command >> with DELETE statement in case when row-security policy is active on >> the target table. >> In this case, a NOTICE message may be helpful for users not to assume >> the table is always empty after the command. > > I think the default must be to throw an ERROR, since part of the > contract with TRUNCATE is that it is fast and removes storage. > OK. Does the default imply you are suggesting configurable behavior using GUC or something? I think both of the behaviors are reasonable from security point of view, as long as user cannot remove unprivileged rows. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
2012/12/7 Simon Riggs <simon@2ndquadrant.com>: > On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > >>> Oracle defaults to putting VPD on all event types: INSERT, UPDATE, >>> DELETE, SELECT. ISTM we should be doing the same, not just say "we can >>> add an INSERT trigger if you want". >>> >>> Adding a trigger just begs the question as to why we are bothering in >>> the first place, since this functionality could already be added by >>> INSERT, UPDATE or DELETE triggers, if they are a full replacement for >>> this feature. The only answer is "ease of use" >>> >>> We can easily add syntax like this >>> >>> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]] >>> >>> with the default being "ALL" >>> >> I think it is flaw of Oracle. :-) > > Agreed > >> In case when user can define leakable function, it enables to leak contents >> of invisible rows at the timing when executor fetch the rows, prior to >> modification >> stage, even if we allows to configure individual row-security policies >> for SELECT >> and DELETE or UPDATE commands. >> My preference is one policy on a particular table for all the commands. > > Yes, only one security policy allowed. > > Question is, should we offer the option to enforce it on a subset of > command types. > > That isn't anything I can see a need for myself. > It is not hard to support a feature not to apply security policy on particular command types, from implementation perspective. So, my preference is to support only the behavior corresponding to above "ALL" option, then support per commands basis when we got strong demands. How about your thought? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 9 December 2012 06:21, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2012/12/7 Simon Riggs <simon@2ndquadrant.com>: >> On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> >>>> Oracle defaults to putting VPD on all event types: INSERT, UPDATE, >>>> DELETE, SELECT. ISTM we should be doing the same, not just say "we can >>>> add an INSERT trigger if you want". >>>> >>>> Adding a trigger just begs the question as to why we are bothering in >>>> the first place, since this functionality could already be added by >>>> INSERT, UPDATE or DELETE triggers, if they are a full replacement for >>>> this feature. The only answer is "ease of use" >>>> >>>> We can easily add syntax like this >>>> >>>> [ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]] >>>> >>>> with the default being "ALL" >>>> >>> I think it is flaw of Oracle. :-) >> >> Agreed >> >>> In case when user can define leakable function, it enables to leak contents >>> of invisible rows at the timing when executor fetch the rows, prior to >>> modification >>> stage, even if we allows to configure individual row-security policies >>> for SELECT >>> and DELETE or UPDATE commands. >>> My preference is one policy on a particular table for all the commands. >> >> Yes, only one security policy allowed. >> >> Question is, should we offer the option to enforce it on a subset of >> command types. >> >> That isn't anything I can see a need for myself. >> > It is not hard to support a feature not to apply security policy on > particular command types, from implementation perspective. > So, my preference is to support only the behavior corresponding > to above "ALL" option, then support per commands basis when > we got strong demands. > How about your thought? Very much agree that ALL should be the default, and only option for first commit of this feature. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 9 December 2012 06:08, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2012/12/7 Simon Riggs <simon@2ndquadrant.com>: >> On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> >>>> * TRUNCATE works, and allows you to remove all rows of a table, even >>>> ones you can't see to run a DELETE on. Er... >>>> >>> It was my oversight. My preference is to rewrite TRUNCATE command >>> with DELETE statement in case when row-security policy is active on >>> the target table. >>> In this case, a NOTICE message may be helpful for users not to assume >>> the table is always empty after the command. >> >> I think the default must be to throw an ERROR, since part of the >> contract with TRUNCATE is that it is fast and removes storage. >> > OK. Does the default imply you are suggesting configurable > behavior using GUC or something? > I think both of the behaviors are reasonable from security point > of view, as long as user cannot remove unprivileged rows. Hmm, its difficult one that. I guess this raises the question as to whether users know they are accessing a table with RLS enabled. If they don't and we want to keep it that way, then changing TRUNCATE into DELETE makes sense. To issue TRUNCATE you need the correct privilege, which is separate from DELETE. If they have TRUNCATE privilege they should be allowed to remove all rows, bypassing the row level security. If that behavious isn't wanted, then the table owner can create an INSTEAD OF TRUNCATE trigger that turns the action into a DELETE, which is then subject to RLS rules. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
2012/12/9 Simon Riggs <simon@2ndquadrant.com>: > On 9 December 2012 06:08, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> 2012/12/7 Simon Riggs <simon@2ndquadrant.com>: >>> On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> >>>>> * TRUNCATE works, and allows you to remove all rows of a table, even >>>>> ones you can't see to run a DELETE on. Er... >>>>> >>>> It was my oversight. My preference is to rewrite TRUNCATE command >>>> with DELETE statement in case when row-security policy is active on >>>> the target table. >>>> In this case, a NOTICE message may be helpful for users not to assume >>>> the table is always empty after the command. >>> >>> I think the default must be to throw an ERROR, since part of the >>> contract with TRUNCATE is that it is fast and removes storage. >>> >> OK. Does the default imply you are suggesting configurable >> behavior using GUC or something? >> I think both of the behaviors are reasonable from security point >> of view, as long as user cannot remove unprivileged rows. > > Hmm, its difficult one that. I guess this raises the question as to > whether users know they are accessing a table with RLS enabled. If > they don't and we want to keep it that way, then changing TRUNCATE > into DELETE makes sense. > > To issue TRUNCATE you need the correct privilege, which is separate from DELETE. > > If they have TRUNCATE privilege they should be allowed to remove all > rows, bypassing the row level security. > > If that behavious isn't wanted, then the table owner can create an > INSTEAD OF TRUNCATE trigger that turns the action into a DELETE, which > is then subject to RLS rules. > It seems to me make sense, also. Even though selinux does not define separated permissions for TRUNCATE, the later option will work well for me in case of row-level label based security is configured in the future version. So, I don't implement something special around TRUNCATE, except for paying mention at the documentation. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
This attached patch is revised version of row-security.
As I don't call it row-level-security, the feature is renamed to row-security,
thus, its syntax, catalog name, source files, ... are also renamed:
The new syntax is below:
ALTER TABLE xxx SET ROW SECURITY (<expression>);
ALTER TABLE xxx RESET ROW SECURITY;
Most significant change is the configured row-security policy is also
checked just before insertion or update of newer tuple, as follows.
postgres=> CREATE TABLE t1 (a int, b text);
CREATE TABLE
postgres=> CREATE TABLE t2 (x int, y text);
CREATE TABLE
postgres=> INSERT INTO t1 VALUES (1,'aaa'),(2,'bbb'),(3,'ccc');
INSERT 0 3
postgres=> INSERT INTO t2 VALUES (2,'xxx'),(3,'yyy'),(4,'zzz');
INSERT 0 3
postgres=> ALTER TABLE t1 SET ROW SECURITY (a in (SELECT x FROM t2));
ALTER TABLE
postgres=> SELECT * FROM t1;
a | b
---+-----
2 | bbb
3 | ccc
(2 rows)
postgres=> INSERT INTO t1 VALUES (4,'ddd');
INSERT 0 1
postgres=> INSERT INTO t1 VALUES (5,'eee');
ERROR: new row for relation "t1" violates row-secirity
DETAIL: Failing row contains (5, eee).
postgres=> UPDATE t1 SET a = a + 1;
ERROR: new row for relation "t1" violates row-secirity
DETAIL: Failing row contains (5, ddd).
postgres=> UPDATE t1 SET a = a + 1 WHERE b = 'ccc';
UPDATE 1
The configured row-security policy requires t1.a has to exist in t2.x,
thus, possible value is either 2, 3 or 4.
The first INSERT is OK, but second one violates.
Also, the first UPDATE gets violated when it tried to update the row
of (4,'ddd').
Also, \d+ command was extended to show the configured row-security policy.
postgres=> \d+
List of relations
Schema | Name | Type | Owner | Size | Description | Row-security
--------+------+-------+-------+-------+-------------+------------------------------
public | t1 | table | alice | 16 kB | | (a IN (SELECT
t2.x FROM t2))
public | t2 | table | alice | 16 kB | |
(2 rows)
According to the upthread discussion, I didn't touch the code around
TRUNCATE command due the nature of separated permission.
While I had code revising, I got some ideas to the issues Simon pointed out.
* row-security policy per command type.
If we can set arbitrary row-security policy towards each command type,
it may allow to leak contents of rows to be invisible, using DELETE ...
RETURNING * for example.
Origin of the problem was that row-security of UPDATE or DELETE
can be laxer than SELECT, thus, these commands can leak them.
So, if row-security policy of writer-side implies the one of reader-side,
it never become a problem.
For example, if reader's row-security policy is (uname = current_user)
and writer's one is (permission = 'w'), it is not difficult to combine them
using AND, as (uname = current_user AND permission = 'w').
A problem is, it may take redundant calculation if user gives very
complex expression on both of reader and writer commands but these
are different at a very tiny point.
* reference to statistics catalogs.
Once ANALYZE collect samples from the table with row-security policy,
the statistical data lost where this value come from. Thus, it is not
possible to apply checks individual elements of them.
I think, only possible way is to hide these values on view definition,
and a SQL function to indicate whether row-security policy is
configured, or not. If we have, it can be used on pg_stat definition
to hide raw statistical values.
Thanks,
2012/12/9 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2012/12/9 Simon Riggs <simon@2ndquadrant.com>:
>> On 9 December 2012 06:08, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> 2012/12/7 Simon Riggs <simon@2ndquadrant.com>:
>>>> On 5 December 2012 11:16, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>>>
>>>>>> * TRUNCATE works, and allows you to remove all rows of a table, even
>>>>>> ones you can't see to run a DELETE on. Er...
>>>>>>
>>>>> It was my oversight. My preference is to rewrite TRUNCATE command
>>>>> with DELETE statement in case when row-security policy is active on
>>>>> the target table.
>>>>> In this case, a NOTICE message may be helpful for users not to assume
>>>>> the table is always empty after the command.
>>>>
>>>> I think the default must be to throw an ERROR, since part of the
>>>> contract with TRUNCATE is that it is fast and removes storage.
>>>>
>>> OK. Does the default imply you are suggesting configurable
>>> behavior using GUC or something?
>>> I think both of the behaviors are reasonable from security point
>>> of view, as long as user cannot remove unprivileged rows.
>>
>> Hmm, its difficult one that. I guess this raises the question as to
>> whether users know they are accessing a table with RLS enabled. If
>> they don't and we want to keep it that way, then changing TRUNCATE
>> into DELETE makes sense.
>>
>> To issue TRUNCATE you need the correct privilege, which is separate from DELETE.
>>
>> If they have TRUNCATE privilege they should be allowed to remove all
>> rows, bypassing the row level security.
>>
>> If that behavious isn't wanted, then the table owner can create an
>> INSTEAD OF TRUNCATE trigger that turns the action into a DELETE, which
>> is then subject to RLS rules.
>>
> It seems to me make sense, also.
> Even though selinux does not define separated permissions for TRUNCATE,
> the later option will work well for me in case of row-level label based security
> is configured in the future version.
> So, I don't implement something special around TRUNCATE, except for
> paying mention at the documentation.
>
> Thanks,
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > postgres=> INSERT INTO t1 VALUES (4,'ddd'); > INSERT 0 1 > postgres=> INSERT INTO t1 VALUES (5,'eee'); > ERROR: new row for relation "t1" violates row-secirity > DETAIL: Failing row contains (5, eee). I've argued against this before - and maybe I should drop my objection, because a number of people seem to be on the other side. But I still think there will be some people who don't want this behavior. Right now, for example, you can give someone INSERT but not SELECT permission on a table, and they will then be able to put rows into the table that they cannot read back. Similarly, in the RLS case, it is not necessarily undesirable for a user to be able to insert a row that they can't read back; or for them to be able to update a row from a value that they can see to one that they cannot. Some people will want to prohibit that, while others will not. Previously, I suggested that we handle this by enforcing row-level security only on data read from the table - the OLD row, so to speak - and not on data written to the table - the NEW row, so to speak - because the latter case can be handled well enough by triggers. (The OLD case cannot, because not seeing the row is different from erroring out when you do see it.) There are other alternatives, like allowing the user to specify which behavior they want. But I think that simply decreeing that the policy will apply not only to rows read but also rows written in all cases will be less flexible than we will ultimately want to be. YMMV, of course. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 19 December 2012 17:25, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> postgres=> INSERT INTO t1 VALUES (4,'ddd'); >> INSERT 0 1 >> postgres=> INSERT INTO t1 VALUES (5,'eee'); >> ERROR: new row for relation "t1" violates row-secirity >> DETAIL: Failing row contains (5, eee). > > I've argued against this before - and maybe I should drop my > objection, because a number of people seem to be on the other side. > But I still think there will be some people who don't want this > behavior. Right now, for example, you can give someone INSERT but not > SELECT permission on a table, and they will then be able to put rows > into the table that they cannot read back. Similarly, in the RLS > case, it is not necessarily undesirable for a user to be able to > insert a row that they can't read back; or for them to be able to > update a row from a value that they can see to one that they cannot. > Some people will want to prohibit that, while others will not. I can see a use case for not having security apply for users who have *only* INSERT privilege. This would allow people to run bulk loads of data into a table with row security. We should add that. That is not the common case, so with proper documentation that should be a useful feature without relaxing default security. Never applying security for INSERT and then forcing them to add BEFORE triggers if they want full security is neither secure nor performant. > Previously, I suggested that we handle this by enforcing row-level > security only on data read from the table - the OLD row, so to speak - > and not on data written to the table - the NEW row, so to speak - > because the latter case can be handled well enough by triggers. (The > OLD case cannot, because not seeing the row is different from erroring > out when you do see it.) There are other alternatives, like allowing > the user to specify which behavior they want. But I think that simply > decreeing that the policy will apply not only to rows read but also > rows written in all cases will be less flexible than we will > ultimately want to be. As discussed, we should add a security feature that is secure by default. Adding options to make it less secure can follow initial commit. We might even make it in this release if the review of the main feature goes well. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> postgres=> INSERT INTO t1 VALUES (4,'ddd');
>> INSERT 0 1
>> postgres=> INSERT INTO t1 VALUES (5,'eee');
>> ERROR: new row for relation "t1" violates row-secirity
>> DETAIL: Failing row contains (5, eee).
> I've argued against this before - and maybe I should drop my
> objection, because a number of people seem to be on the other side.
> But I still think there will be some people who don't want this
> behavior. Right now, for example, you can give someone INSERT but not
> SELECT permission on a table, and they will then be able to put rows
> into the table that they cannot read back.
There is also precedent for your opinion in the spec-mandated behavior
of updatable views: it is perfectly possible to INSERT a row that you
can't read back via the view, or UPDATE it to a state you can't see
via the view. The RLS patch's current behavior corresponds to a view
created WITH CHECK OPTION --- which we don't support yet. Whether
we add that feature soon or not, what seems important for the current
debate is that the SQL spec authors chose not to make it the default
behavior. This seems to weigh heavily against making it the default,
much less only, behavior for RLS cases.
I'd also suggest that "throw an error" is not the only response that
people are likely to want for attempts to insert/update non-compliant
rows, so hard-wiring that choice is insufficiently flexible even if you
grant that local policy is to not allow such updates. (As an example,
they might prefer to log the attempt and substitute some other value.)
> Previously, I suggested that we handle this by enforcing row-level
> security only on data read from the table - the OLD row, so to speak -
> and not on data written to the table - the NEW row, so to speak -
> because the latter case can be handled well enough by triggers.
+1. I'm less than excited about RLS in the first place, so the less
complexity we have to put into the core system for it the better IMO.
regards, tom lane
On 19 December 2012 18:40, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> postgres=> INSERT INTO t1 VALUES (4,'ddd'); >>> INSERT 0 1 >>> postgres=> INSERT INTO t1 VALUES (5,'eee'); >>> ERROR: new row for relation "t1" violates row-secirity >>> DETAIL: Failing row contains (5, eee). > >> I've argued against this before - and maybe I should drop my >> objection, because a number of people seem to be on the other side. >> But I still think there will be some people who don't want this >> behavior. Right now, for example, you can give someone INSERT but not >> SELECT permission on a table, and they will then be able to put rows >> into the table that they cannot read back. > > There is also precedent for your opinion in the spec-mandated behavior > of updatable views: it is perfectly possible to INSERT a row that you > can't read back via the view, or UPDATE it to a state you can't see > via the view. The RLS patch's current behavior corresponds to a view > created WITH CHECK OPTION --- which we don't support yet. Whether > we add that feature soon or not, what seems important for the current > debate is that the SQL spec authors chose not to make it the default > behavior. This seems to weigh heavily against making it the default, > much less only, behavior for RLS cases. This is security, not spec compliance. By default, we need full security. Nobody has argued that it should be the only behaviour, only that it is the most typically requested behaviour and the most secure, therefore the one we should do first. > I'd also suggest that "throw an error" is not the only response that > people are likely to want for attempts to insert/update non-compliant > rows, so hard-wiring that choice is insufficiently flexible even if you > grant that local policy is to not allow such updates. (As an example, > they might prefer to log the attempt and substitute some other value.) > >> Previously, I suggested that we handle this by enforcing row-level >> security only on data read from the table - the OLD row, so to speak - >> and not on data written to the table - the NEW row, so to speak - >> because the latter case can be handled well enough by triggers. > > +1. I'm less than excited about RLS in the first place, so the less > complexity we have to put into the core system for it the better IMO. Agree with the need for less complexity, but that decision increases complexity for the typical user and does very little to the complexity of the patch. Treating a security rule as a check constraint is natural and obvious, so there are no core system problems here. If we don't enforce rules on INSERT the user has to specifically add a trigger, which makes things noticeably slower. There is more maintenance work for the average user, less performance and more mistakes to make. The way to do this is by adding an option to allow users to specify INSERT should be exempt from the security rule, which Kaigai and I agreed on list some weeks back should come after the initial patch, to no other comment. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs wrote: > This is security, not spec compliance. By default, we need full > security. But you are arguing that users should not be able to make something secure if they, and everyone with the same permissions, could not later access it. I thought about situations where I've seen a need for something like this, and probably the best fit that I've seen is the ability of a judge to order that something is sealed. There are various levels where that can happen, but I'll focus on just one which Wisconsin Courts have implemented at the application level, but which would be nice to be able to support at the database level. Let's say we're talking about Milwaukee County, where hundreds of people work for the courts and related organizations with some rights to view the court data. Let's say a battered wife has moved to a new address she wants to keep secret for safety. She files a case with the court for a temporary restraining order, prohibiting the husband from coming near her. The court needs her address for the official record, but the judge will order the address "sealed" so that only people with a certain authority can see it. The authority is very limited, for obvious reasons. It is quite likely that the person initially entering the address and flagging it as sealed will not have authority to see the address if they go back and look up the case. Neither will the dozens of other people making the same kind of entries in the county. Obviously, if the person doing the initial entry is a friend of the husband, the data is compromised; but not allowing entry of the data in a state sealed by people without authority to look it up exposes the data to every other person with entry authority, with fairly obvious risks. The more secure behavior is to allow entry of data which will not be visible by the person doing the entry. -Kevin
On 19 December 2012 19:46, Kevin Grittner <kgrittn@mail.com> wrote: > But you are arguing that users should not be able to make something > secure if they, and everyone with the same permissions, could not > later access it. Not exactly, no. I've argued that row security should apply to ALL commands by default. Which is exactly the same default as Oracle, as well as being the obvious common sense understanding of "row security", which does not cause a POLA violation. I have no objection to an option to allow row security to not apply to inserts, if people want that. I do object to the idea that row security for inserts/updates should only happen via triggers, which is an ugly and non-performant route, as well as complicating security. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2012-12-19 14:46:18 -0500, Kevin Grittner wrote: > Simon Riggs wrote: > > > This is security, not spec compliance. By default, we need full > > security. > > But you are arguing that users should not be able to make something > secure if they, and everyone with the same permissions, could not > later access it. I thought about situations where I've seen a need > for something like this, and probably the best fit that I've seen > is the ability of a judge to order that something is sealed. There > are various levels where that can happen, but I'll focus on just > one which Wisconsin Courts have implemented at the application > level, but which would be nice to be able to support at the > database level. > > Let's say we're talking about Milwaukee County, where hundreds of > people work for the courts and related organizations with some > rights to view the court data. Let's say a battered wife has moved > to a new address she wants to keep secret for safety. She files a > case with the court for a temporary restraining order, prohibiting > the husband from coming near her. The court needs her address for > the official record, but the judge will order the address "sealed" > so that only people with a certain authority can see it. The > authority is very limited, for obvious reasons. > > It is quite likely that the person initially entering the address > and flagging it as sealed will not have authority to see the > address if they go back and look up the case. Neither will the > dozens of other people making the same kind of entries in the > county. Obviously, if the person doing the initial entry is a > friend of the husband, the data is compromised; but not allowing > entry of the data in a state sealed by people without authority to > look it up exposes the data to every other person with entry > authority, with fairly obvious risks. > > The more secure behavior is to allow entry of data which will not > be visible by the person doing the entry. I don't think it is that simple. Allowing inserts without regard for row level restrictions makes it far easier to probe for data. E.g. by inserting rows and checking for unique violations. Greetings, Andres Freund --Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs wrote: > I've argued that row security should apply to ALL commands by default. > Which is exactly the same default as Oracle, as well as being the > obvious common sense understanding of "row security", which does not > cause a POLA violation. > > I have no objection to an option to allow row security to not apply to > inserts, if people want that. > > I do object to the idea that row security for inserts/updates should > only happen via triggers, which is an ugly and non-performant route, > as well as complicating security. In the software where I've seen this managed at an application level, an address (for example) can be sealed by an update to the "sealed" column or inserted with the sealed column set to true. I'm not sure why you would want to allow one and not the other. Now, I can envision a situation where it could make sense to use the same predicate for limiting what a role could read and what a role could write, but I'm not buying that that is more secure. In fact, I see cases where you cannot achieve decent security without the ability for both INSERT and UPDATE to write rows which security excludes on reads. Functionally, I don't see anything which can't be accomplished with just the read security and triggers. I do agree that it would be nice if there were an easy way to specify that the same predicate applies to both reads and writes. I hope we can leave the syntax for this feature open to such specification, even if the initial implementation only supports limiting reads. -Kevin
Andres Freund wrote: > I don't think it is that simple. Allowing inserts without regard for row > level restrictions makes it far easier to probe for data. E.g. by > inserting rows and checking for unique violations. Unless you want to go to a military-style security level system where people at each security level have a separate version of the same data, primary keys (and I think other unique constraints) can leak. It seems clear enough that sensitive data should not be used for such constraints. That doesn't even require completely meaningless numeric keys. Court cases in Wisconsin have been numbered within county by year, case type, a sequential portion, and an optional apha suffix since before things were computerized -- you may know that there is a 2010 case in Dane County for mental commitment number 45, but that doesn't leak any sensitive data. In the sealed address example, if that were moved to the database layer, someone might be able to test whether addess number 5 existed on a case by seeing whether an insert succeeded; but if it did, there would be a record of that insert with their user ID that they could not retract in any way -- they would know very little, and be exposed as having done something inappropriate. -Kevin
On 19 December 2012 20:23, Kevin Grittner <kgrittn@mail.com> wrote: > I hope we can leave the syntax for this feature open to such > specification, even if the initial implementation only supports > limiting reads. Well, I hope the opposite: that we can support simple full security by default, while leaving syntax open. The basic model for this is complete separation of data between customers/people. They can't see my data, I can't see theirs. Simple privacy. Obvious. Sure, more complex applications exist, but forcing the simple/common usage to adopt triggers because of that is not a sensible way forwards. Simple basic functionality, with an option for more advanced cases is what we need. Setting a status flag so that the current user no longer sees the row is a good example of more complex workflows in secure applications, I agree, but its not the common case by any means. When we have these discussions about priority, it seems people think this means "don't do it ever". It doesn't, it means do the most important things first and then do other stuff later. I always wish to do both, but circumstances teach me that hard cutoffs and deadlines mean we can't always have everything if debates overrun and decisions aren't forthcoming. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
> > The more secure behavior is to allow entry of data which will not be > > visible by the person doing the entry. > > I don't think it is that simple. Allowing inserts without regard for row level > restrictions makes it far easier to probe for data. E.g. by inserting rows and > checking for unique violations. > So the PK column(s) are not as secure as, say, the address-related column. Vice-versa I may know that someone lives at a given address (because my attempt to place someone else there failed) but I would have no way of knowing who that other person is. My recourse would be to escalate the data-entry request to someone with higher security permissions who could read and write to the appropriate tables and resolve the conflict. In both cases the direct write-only situation necessitates that some level of exposure occurs. The work-around if that is unacceptable would be to accept all data but any entries that cannot be directly inserted into the table would remain in a staging area that someone with higher security would have to monitor and clear as needed. The same intervention is required but in the first situation you can at least avoid coding the special logic and instead trade security for ease-of-use. As a default level of security we could throw a generic "secure DLL rejected for ROW(...)" and not tell the user anything about the cause. If that person knows all unique indexes and constraints defined on the table they could use trial-and-error to discover information about stored records but even then if they get an error on two different columns they still have no way of knowing if those "errors" belong to the same record. Beyond that level you provide the user with some information as to the cause so that they have a reasonable chance to catch typos and other mistakes instead of escalating an benign issue. Lastly is the custom solution whereby the developers accept ALL data entered as being correct but saved to a staging table. A review process by someone with higher security clearances would then process and clear out that table as necessary. If the user is write-only then regardless of whether the entry succeeded or failed they are considered to be "done" with their task at that point and no meaningful results from the system can be supplied to them. None of these options disallows the presence of non-security related check constraints to be checked, enforced, and communicated to the user. I've probably lost sight of the bigger picture as my response to mostly informed by these last couple of messages. David J. > Greetings, > > Andres Freund >
On 19 December 2012 20:37, Kevin Grittner <kgrittn@mail.com> wrote: > Andres Freund wrote: > >> I don't think it is that simple. Allowing inserts without regard for row >> level restrictions makes it far easier to probe for data. E.g. by >> inserting rows and checking for unique violations. > > Unless you want to go to a military-style security level system > where people at each security level have a separate version of the > same data, primary keys (and I think other unique constraints) can > leak. It seems clear enough that sensitive data should not be used > for such constraints. But there is the more obvious case where you shouldn't be able to insert medical history for a patient you have no responsibility for. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs wrote: > Kevin Grittner <kgrittn@mail.com> wrote: > >> I hope we can leave the syntax for this feature open to such >> specification, even if the initial implementation only supports >> limiting reads. > > Well, I hope the opposite: that we can support simple full security by > default, while leaving syntax open. > > The basic model for this is complete separation of data between > customers/people. They can't see my data, I can't see theirs. Simple > privacy. Obvious. And something we already can handle several different ways. Inheritance, schemas, etc. Allowing data to be fully secured from prying eyes on entry, regardless of whether the role allowing the entry has rights to see the data does not yet have any built-in support. > Sure, more complex applications exist, but forcing the simple/common > usage to adopt triggers because of that is not a sensible way > forwards. Simple basic functionality, with an option for more advanced > cases is what we need. Setting a status flag so that the current user > no longer sees the row is a good example of more complex workflows in > secure applications, I agree, but its not the common case by any > means. > > When we have these discussions about priority, it seems people think > this means "don't do it ever". It doesn't, it means do the most > important things first and then do other stuff later. I always wish to > do both, but circumstances teach me that hard cutoffs and deadlines > mean we can't always have everything if debates overrun and decisions > aren't forthcoming. Well, it seems we have different views of what is intuitively obvious here. What you suggest does not look to me to be more secure (making "full security" a misnomer), simpler, nor more important. Perhaps we can avoid divisive discussions on which is more important if we can manage both in the initial implementation. I guess the usual rule if we can't manage it is that a tie goes to the author, which is neither you nor I. -Kevin
On 2012-12-19 18:25, Robert Haas wrote: > On Tue, Dec 18, 2012 at 3:39 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> postgres=> INSERT INTO t1 VALUES (4,'ddd'); >> INSERT 0 1 >> postgres=> INSERT INTO t1 VALUES (5,'eee'); >> ERROR: new row for relation "t1" violates row-secirity >> DETAIL: Failing row contains (5, eee). > I've argued against this before - and maybe I should drop my > objection, because a number of people seem to be on the other side. > But I still think there will be some people who don't want this > behavior. Right now, for example, you can give someone INSERT but not > SELECT permission on a table, and they will then be able to put rows > into the table that they cannot read back. Similarly, in the RLS > case, it is not necessarily undesirable for a user to be able to > insert a row that they can't read back; or for them to be able to > update a row from a value that they can see to one that they cannot. > Some people will want to prohibit that, while others will not. Maybe it is an idea to provide different RLS expressions for read and write. I remember reading a scenario (it might be well known in security land) where it is possible to write to authorization levels >= users level, and read levels <= the users level. In this setup Kevin's address example is possible, a user could write to e.g. the highest level, but then not read it anymore if his own level was lower than the highest. This setup also shows that to implement it, one would need a different expression for read and write (or the rls expression should know the query's commandtype). regards, Yeb
On Wed, Dec 19, 2012 at 12:54 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > I can see a use case for not having security apply for users who have > *only* INSERT privilege. This would allow people to run bulk loads of > data into a table with row security. We should add that. That is not > the common case, so with proper documentation that should be a useful > feature without relaxing default security. > > Never applying security for INSERT and then forcing them to add BEFORE > triggers if they want full security is neither secure nor performant. I think INSERT vs. not-INSERT is not the relevant distinction, because the question also arises for UPDATE. In the UPDATE case, the question is whether the RLS qual should be checked only against the OLD tuple (to make sure that we can see the tuple to modify it) or also against the NEW tuple (to make sure that we're not modifying it to a form that we can no longer see). In other words, the question is not "do we support all of the commands?" but rather "do we check not only the tuple read but also the tuple written?". For INSERT, we only write a tuple, without reading. For SELECT and DELETE, we only read a tuple, without writing a new one. UPDATE does both a read and a write. >> Previously, I suggested that we handle this by enforcing row-level >> security only on data read from the table - the OLD row, so to speak - >> and not on data written to the table - the NEW row, so to speak - >> because the latter case can be handled well enough by triggers. (The >> OLD case cannot, because not seeing the row is different from erroring >> out when you do see it.) There are other alternatives, like allowing >> the user to specify which behavior they want. But I think that simply >> decreeing that the policy will apply not only to rows read but also >> rows written in all cases will be less flexible than we will >> ultimately want to be. > > As discussed, we should add a security feature that is secure by > default. Adding options to make it less secure can follow initial > commit. We might even make it in this release if the review of the > main feature goes well. Saying that something is or is not secure is not meaningful without defining what you want to be secure against. There's nothing "insecure" about checking only the tuples read; it's just a different (and useful) threat model. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Dec 19, 2012 at 1:58 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > If we don't enforce rules on INSERT the user has to specifically add a > trigger, which makes things noticeably slower. There is more > maintenance work for the average user, less performance and more > mistakes to make. Well, again, only if that's the behavior they want. Also, it's also worth noting that, even if we assume that it is in fact the behavior that users will want, the contention that it is faster than a trigger is thus far unsubstantiated by any actual benchmarks. It may indeed be faster ... but I don't know without testing whether it's slightly faster or a whole lot faster. That might be a good thing to find out, because if it is a whole lot faster, that would certainly strengthen the case for including a mode that works that way, whether or not we also provide other options. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 20 December 2012 00:24, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Dec 19, 2012 at 12:54 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> I can see a use case for not having security apply for users who have >> *only* INSERT privilege. This would allow people to run bulk loads of >> data into a table with row security. We should add that. That is not >> the common case, so with proper documentation that should be a useful >> feature without relaxing default security. >> >> Never applying security for INSERT and then forcing them to add BEFORE >> triggers if they want full security is neither secure nor performant. > > I think INSERT vs. not-INSERT is not the relevant distinction, because > the question also arises for UPDATE. Not sure I understand you. You suggested it was a valid use case for a user to have only INSERT privilege and wish to bypass security checks. I agreed and suggested it could be special-cased. > In the UPDATE case, the question > is whether the RLS qual should be checked only against the OLD tuple > (to make sure that we can see the tuple to modify it) or also against > the NEW tuple (to make sure that we're not modifying it to a form that > we can no longer see). In other words, the question is not "do we > support all of the commands?" but rather "do we check not only the > tuple read but also the tuple written?". For INSERT, we only write a > tuple, without reading. For SELECT and DELETE, we only read a tuple, > without writing a new one. UPDATE does both a read and a write. I'm not sure what this comment adds to the discussion. What you say is understood. >>> Previously, I suggested that we handle this by enforcing row-level >>> security only on data read from the table - the OLD row, so to speak - >>> and not on data written to the table - the NEW row, so to speak - >>> because the latter case can be handled well enough by triggers. (The >>> OLD case cannot, because not seeing the row is different from erroring >>> out when you do see it.) There are other alternatives, like allowing >>> the user to specify which behavior they want. But I think that simply >>> decreeing that the policy will apply not only to rows read but also >>> rows written in all cases will be less flexible than we will >>> ultimately want to be. >> >> As discussed, we should add a security feature that is secure by >> default. Adding options to make it less secure can follow initial >> commit. We might even make it in this release if the review of the >> main feature goes well. > > Saying that something is or is not secure is not meaningful without > defining what you want to be secure against. There's nothing > "insecure" about checking only the tuples read; it's just a different > (and useful) threat model. There are three main points * "Applies to all commands" should not be implemented via triggers. Complex, slow, unacceptable thing to force upon users. Doing that begs the question of why we would have the feature at all, since we already have triggers and barrier views. * the default for row security should be "applies to all commands". Anything else may be useful in some cases, but is surprising to users and requires careful thought to determine if it is appropriate. * How to handle asymmetric row security policies? KaiGai has already begun discussing problems caused by a security policy that differs between reads/writes, on his latest patch post. That needs further analysis to check that it actually makes sense to allow it, since it is more complex. It would be better to fully analyse that situation and post solutions, rather than simply argue its OK. Kevin has made good arguments to show there could be value in such a setup; nobody has talked about banning it, but we do need analysis, suggested syntax/mechanisms and extensive documentation to explain it etc. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Kevin, all,
* Kevin Grittner (kgrittn@mail.com) wrote:
> The more secure behavior is to allow entry of data which will not
> be visible by the person doing the entry.
wrt this- I'm inclined to agree with Kevin. It's certainly common in
certain environments that you can write to a higher level than you can
read from. Granting those writers access to read the data later would
be... difficult.
What we're really arguing about here, afaict, is what the default should
be. In line with Kevin's comments and Tom's reading of the spec (along
with my own experience in these environments), I'd argue for the default
to allow writing rows you're not allowed to read.
It would certainly be ideal if we could support both options, on a
per-relation basis, when we release the overall feature. It doesn't
"feel" like it'd be a lot of work to do that, but I've not been able to
follow this discussion up til now. Thankfully, I'm hopeful that I'm
going to have more time now to keep up with PG. :)
Thanks,
Stephen
On Thu, Dec 20, 2012 at 4:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > Not sure I understand you. You suggested it was a valid use case for a > user to have only INSERT privilege and wish to bypass security checks. > I agreed and suggested it could be special-cased. That's not really what I intended to suggest. I view checking an inserted tuple and checking the new version of an updated tuple as of a piece. I would think we would check against the RLS quals in either both of those situations or neither, not one without the other. > * "Applies to all commands" should not be implemented via triggers. > Complex, slow, unacceptable thing to force upon users. Doing that begs > the question of why we would have the feature at all, since we already > have triggers and barrier views. I agree that it is questionable whether we need this feature given that we already have security barrier views. I don't agree that performing security checks via triggers is unacceptably slow or complex. Rather, I would say it is flexible and can meet a variety of needs, unlike this feature, which imposes much tighter constraints on what you can and cannot check and in which situations. > * the default for row security should be "applies to all commands". > Anything else may be useful in some cases, but is surprising to users > and requires careful thought to determine if it is appropriate. I (and several other people, it seems) do not agree. > * How to handle asymmetric row security policies? KaiGai has already > begun discussing problems caused by a security policy that differs > between reads/writes, on his latest patch post. That needs further > analysis to check that it actually makes sense to allow it, since it > is more complex. It would be better to fully analyse that situation > and post solutions, rather than simply argue its OK. Kevin has made > good arguments to show there could be value in such a setup; nobody > has talked about banning it, but we do need analysis, suggested > syntax/mechanisms and extensive documentation to explain it etc. Frankly, in view of your comments above, I am starting to rethink whether we want this at all. I mean, if you've got security barrier views, you can check the data being read. If you've got triggers, you can check the data being written. So what's left? There's something notationally appealing about being able to apply a security policy to a table rather than creating a separate view and telling people to use the view in lieu of the table, but how much is that notational convenience worth? It has some value from the standpoint of compatibility with other database products ... but probably not a whole lot, since all the syntax we're inventing here is PostgreSQL-specific anyway. Your proposal to check both tuples read and tuples written might add some value ... but unless there's an as-yet-undemonstrated performance benefit, it is again mostly a notational benefit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2012/12/20 Stephen Frost <sfrost@snowman.net>: > Kevin, all, > > * Kevin Grittner (kgrittn@mail.com) wrote: >> The more secure behavior is to allow entry of data which will not >> be visible by the person doing the entry. > > wrt this- I'm inclined to agree with Kevin. It's certainly common in > certain environments that you can write to a higher level than you can > read from. Granting those writers access to read the data later would > be... difficult. > > What we're really arguing about here, afaict, is what the default should > be. In line with Kevin's comments and Tom's reading of the spec (along > with my own experience in these environments), I'd argue for the default > to allow writing rows you're not allowed to read. > If system ensures writer's permission is always equivalent or more restrictive than reader's permission, it also eliminates the problem around asymmetric row-security policy between commands. The problematic scenario was that updatable but invisible rows are exposed; using update with returning clause for example. In case when updatable rows are always visible, here is no matter even if user shows it. Probably, we can implement it as ((row-security of select) AND (row-security of update)) that ensures "always restrictive row-security policy". Thanks, > It would certainly be ideal if we could support both options, on a > per-relation basis, when we release the overall feature. It doesn't > "feel" like it'd be a lot of work to do that, but I've not been able to > follow this discussion up til now. Thankfully, I'm hopeful that I'm > going to have more time now to keep up with PG. :) > > Thanks, > > Stephen -- KaiGai Kohei <kaigai@kaigai.gr.jp>
* Robert Haas (robertmhaas@gmail.com) wrote:
> > * "Applies to all commands" should not be implemented via triggers.
> > Complex, slow, unacceptable thing to force upon users. Doing that begs
> > the question of why we would have the feature at all, since we already
> > have triggers and barrier views.
I would rather neither requires writing custom triggers but rather both
are supported through this feature.
> I agree that it is questionable whether we need this feature given
> that we already have security barrier views.
This I don't agree with- the plan has long been to have PG-specific RLS
first and then to support SELinux capabilities on top of it. We didn't
want to have SELinux-specific functionality that couldn't be achieved
without SELinux being involved, and I continue to agree with that.
There are many situations, environments, and individuals that would
view having to implement RLS through views and triggers as being
far-and-away too painful and error-prone to rely on.
Thanks,
Stephen
2012/12/20 Robert Haas <robertmhaas@gmail.com>: > On Thu, Dec 20, 2012 at 4:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> Not sure I understand you. You suggested it was a valid use case for a >> user to have only INSERT privilege and wish to bypass security checks. >> I agreed and suggested it could be special-cased. > > That's not really what I intended to suggest. I view checking an > inserted tuple and checking the new version of an updated tuple as of > a piece. I would think we would check against the RLS quals in either > both of those situations or neither, not one without the other. > >> * "Applies to all commands" should not be implemented via triggers. >> Complex, slow, unacceptable thing to force upon users. Doing that begs >> the question of why we would have the feature at all, since we already >> have triggers and barrier views. > > I agree that it is questionable whether we need this feature given > that we already have security barrier views. I don't agree that > performing security checks via triggers is unacceptably slow or > complex. Rather, I would say it is flexible and can meet a variety of > needs, unlike this feature, which imposes much tighter constraints on > what you can and cannot check and in which situations. > I'd like to ask Simon which point is more significant; performance penalty or complex operations by users. If later, FK constraint is a good example that automatically defines triggers that applies its checks on inserted tuple and newer version of updated tuple. Even though we need to consider how to handle dynamically added row-security policy by extension (e.g sepgsql), I don't think we need to enforce users to define triggers for each tables with row-security as long as system support it. >> * the default for row security should be "applies to all commands". >> Anything else may be useful in some cases, but is surprising to users >> and requires careful thought to determine if it is appropriate. > > I (and several other people, it seems) do not agree. > >> * How to handle asymmetric row security policies? KaiGai has already >> begun discussing problems caused by a security policy that differs >> between reads/writes, on his latest patch post. That needs further >> analysis to check that it actually makes sense to allow it, since it >> is more complex. It would be better to fully analyse that situation >> and post solutions, rather than simply argue its OK. Kevin has made >> good arguments to show there could be value in such a setup; nobody >> has talked about banning it, but we do need analysis, suggested >> syntax/mechanisms and extensive documentation to explain it etc. > > Frankly, in view of your comments above, I am starting to rethink > whether we want this at all. I mean, if you've got security barrier > views, you can check the data being read. If you've got triggers, you > can check the data being written. So what's left? > In some cases, it is not a reasonable choice to re-define kind of database objects or its name from what existing application assumes. It is a reason why we need adaptive security features on regular tables without or minimum application changes.... Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai wrote: > If system ensures writer's permission is always equivalent or > more restrictive than reader's permission, it also eliminates the > problem around asymmetric row-security policy between commands. I'm not sure we're understanding each other; so far people who favor asymmetric read and write predicates for row level security have been arguing that people should be able to write tuples that they can't read back. Of course you need to be able to read a tuple to update it, but the argument is that you should be able to configure security so that a role can (for example) update a row to set a "sealed" flag, and then no longer have rights to read that row (including for purposes of update). You can "give away" data which is yours, but you can't then "take it back" if it's not. > The problematic scenario was that updatable but invisible rows > are exposed; I have not seen anyone argue that such behavior should be allowed. > Probably, we can implement it as ((row-security of select) AND > (row-security of update)) that ensures "always restrictive > row-security policy". I hadn't been paying a lot of attention to this patch until I saw the question about whether a user with a particular role could write a row which would then not be visible to that role. I just took a look at the patch. I don't think I like ALTER TABLE as a syntax for row level security. How about using existing GRANT syntax but allowing a WHERE clause? That seems more natural to me, and it would make it easy to apply the same conditions to multiple types of operations when desired, but use different expressions when desired. Without having spent a lot of time pondering it, I think that if row level SELECT permissions exist, they would need to be met on the OLD tuple to allow DELETE or UPDATE, and UPDATE row level permissions would be applied to the NEW tuple. So, Simon's use-case could be met with: GRANT SELECT, INSERT, UPDATE, DELETE ON tabx TO org12user WHERE org = 12; ... and similar GRANTs. A user who should be able to access rows for a particular value of org would be granted the appropriate permission. These could be combined by granting a role to another role. To go back to a Wisconsin Courts example, staff in a county might be granted rights to access data for that county, but district roles could be set up and granted to court officials, who need to be able to access data for all counties in their judicial district, because judges fill in for each other across county lines, but only within their own district. My use-case could be met with: GRANT SELECT, INSERT, UPDATE, DELETE ON addr TO general_staff WHERE NOT sealed; GRANT SELECT, INSERT, UPDATE, DELETE ON addr TO sealed_addr_authority WHERE SEALED; GRANT general_staff TO sealed_addr_authority; Note that I think that if one has multiple roles with row level permissions on a table, access should be allowed if any of those roles allows it. I think that the above should be logically equivalent to (although perhaps slightly less efficient at run-time): GRANT SELECT, INSERT, UPDATE, DELETE ON addr TO general_staff WHERE NOT sealed; GRANT SELECT, INSERT, UPDATE, DELETE ON addr TO sealed_addr_authority; And just to round it out, that these could be applied to users with: GRANT general_staff TO bob; GRANT sealed_addr_authority TO supervisor; GRANT supervisor TO jean; I realize this is a major syntactic departure from the current patch, but it just seems a lot more natural and flexible. -Kevin
On 20 December 2012 21:50, Kevin Grittner <kgrittn@mail.com> wrote: > How about using existing GRANT syntax but allowing a > WHERE clause? It's a nice feature, but a completely different thing to what is being discussed here. Row security adds the ability to enforce a single coherent policy at table level. It might be nice to have multiple, potentially overlapping policies, but that would require significantly different design and coding to what we have here. For me, enforcing a single policy at table level helps to make it secure by being coherent and understandable. So perhaps in later releases we might do the feature you suggest. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
2012/12/20 Kevin Grittner <kgrittn@mail.com>: > Kohei KaiGai wrote: > >> If system ensures writer's permission is always equivalent or >> more restrictive than reader's permission, it also eliminates the >> problem around asymmetric row-security policy between commands. > > I'm not sure we're understanding each other; so far people who > favor asymmetric read and write predicates for row level security > have been arguing that people should be able to write tuples that > they can't read back. Of course you need to be able to read a tuple > to update it, but the argument is that you should be able to > configure security so that a role can (for example) update a row to > set a "sealed" flag, and then no longer have rights to read that > row (including for purposes of update). You can "give away" data > which is yours, but you can't then "take it back" if it's not. > Hmm, for this example, the right behavior is to check rows with ((row-security of select) AND (row-security of update)) on scan- stage, but only (row-security of update) is checked on just-before row updates. In case of (row-security of select) is "sealed = false", it is not visible thus not target row of the update, but user can turn on the flag to make it gone. Anyway, the row-security to be applied on "scanning-stage" of source of update/delete needs to be equivalent or more restrictive than rules on select. However, it might not be needed to ensure rules being restrictive on "writer-stage". >> Probably, we can implement it as ((row-security of select) AND >> (row-security of update)) that ensures "always restrictive >> row-security policy". > > I hadn't been paying a lot of attention to this patch until I saw > the question about whether a user with a particular role could > write a row which would then not be visible to that role. I just > took a look at the patch. > > I don't think I like ALTER TABLE as a syntax for row level > security. How about using existing GRANT syntax but allowing a > WHERE clause? That seems more natural to me, and it would make it > easy to apply the same conditions to multiple types of operations > when desired, but use different expressions when desired. > It seems to me this syntax gives an impression that row-security feature is tightly combined with role-mechanism, even though it does not need. For example, we can set row-security policy of primary key is even/odd number, independent from working role. > Without > having spent a lot of time pondering it, I think that if row level > SELECT permissions exist, they would need to be met on the OLD > tuple to allow DELETE or UPDATE, and UPDATE row level permissions > would be applied to the NEW tuple. > Yes, I agree. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 20 December 2012 19:42, Stephen Frost <sfrost@snowman.net> wrote: > Kevin, all, > > * Kevin Grittner (kgrittn@mail.com) wrote: >> The more secure behavior is to allow entry of data which will not >> be visible by the person doing the entry. > > wrt this- I'm inclined to agree with Kevin. It's certainly common in > certain environments that you can write to a higher level than you can > read from. Granting those writers access to read the data later would > be... difficult. > > What we're really arguing about here, afaict, is what the default should > be. In line with Kevin's comments and Tom's reading of the spec (along > with my own experience in these environments), I'd argue for the default > to allow writing rows you're not allowed to read. > > It would certainly be ideal if we could support both options, on a > per-relation basis, when we release the overall feature. It doesn't > "feel" like it'd be a lot of work to do that, but I've not been able to > follow this discussion up til now. Thankfully, I'm hopeful that I'm > going to have more time now to keep up with PG. :) It is certainly possible to deliver a row security feature that covers all the cases discussed in 9.3. I want that. 1. The case of "row security applies to all commands" is simple to implement and document, since it presents no anomalies. 2. As KaiGai has explained, there are significant anomalies in the behaviour of "row security applies only to reads". Those anomalies need to be analysed carefully. They also need to be explained to the user in documentation. It's unreasonable for people to demand a feature yet provide no guidance to the person trying (hard) to provide that feature in a sensible way. If people genuinely believe case (2) is worth pursuing, additional work and input is needed so that KaiGai can make changes in time for the 9.3 deadline. Please read what KaiGai has said and respond. Since there are so many people reading this thread and wanting (2), that seems reasonable to expect. What I have proposed is that I work on the review for case (1) and then if we solve (2) that can go in also. I don't think its reasonable to reject the whole feature because of unresolved difficulties around one use case, which is what will happen if this is seen as merely a debate about defaults. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 21 December 2012 08:56, Simon Riggs <simon@2ndquadrant.com> wrote: > It's unreasonable for people to demand a feature yet provide no > guidance to the person trying (hard) to provide that feature in a > sensible way. If people genuinely believe case (2) is worth pursuing, > additional work and input is needed so that KaiGai can make changes in > time for the 9.3 deadline. Please read what KaiGai has said and > respond. Since there are so many people reading this thread and > wanting (2), that seems reasonable to expect. > > What I have proposed is that I work on the review for case (1) and > then if we solve (2) that can go in also. I don't think its reasonable > to reject the whole feature because of unresolved difficulties around > one use case, which is what will happen if this is seen as merely a > debate about defaults. > One comment on the code itself -- I think it needs some locking of rows from the subquery to ensure correct concurrency behaviour when there are multiple transactions doing updates at the same time. Regards, Dean
On 21 December 2012 09:29, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > On 21 December 2012 08:56, Simon Riggs <simon@2ndquadrant.com> wrote: >> It's unreasonable for people to demand a feature yet provide no >> guidance to the person trying (hard) to provide that feature in a >> sensible way. If people genuinely believe case (2) is worth pursuing, >> additional work and input is needed so that KaiGai can make changes in >> time for the 9.3 deadline. Please read what KaiGai has said and >> respond. Since there are so many people reading this thread and >> wanting (2), that seems reasonable to expect. >> >> What I have proposed is that I work on the review for case (1) and >> then if we solve (2) that can go in also. I don't think its reasonable >> to reject the whole feature because of unresolved difficulties around >> one use case, which is what will happen if this is seen as merely a >> debate about defaults. >> > > One comment on the code itself -- I think it needs some locking of > rows from the subquery to ensure correct concurrency behaviour when > there are multiple transactions doing updates at the same time. > Another comment -- the use of the RangeTblEntry structure in the new code is a bit odd. It seems to be creating an RTE that is both an RTE_RELATION and an RTE_SUBQUERY at the same time. I think it would be cleaner to just add a new RTE for the subquery (cf. the trigger-updatable view code in ApplyRetrieveRule). Regards, Dean
On Thu, Dec 20, 2012 at 4:50 PM, Kevin Grittner <kgrittn@mail.com> wrote: > I don't think I like ALTER TABLE as a syntax for row level > security. How about using existing GRANT syntax but allowing a > WHERE clause? That seems more natural to me, and it would make it > easy to apply the same conditions to multiple types of operations > when desired, but use different expressions when desired. Without > having spent a lot of time pondering it, I think that if row level > SELECT permissions exist, they would need to be met on the OLD > tuple to allow DELETE or UPDATE, and UPDATE row level permissions > would be applied to the NEW tuple. This gets thorny if a role inherits from multiple roles each having a different RLS predicate. You can OR them together, but performance will likely suck. I initially thought of this as well, but I think it's just too ugly to live. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Dec 21, 2012 at 3:56 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > It's unreasonable for people to demand a feature yet provide no > guidance to the person trying (hard) to provide that feature in a > sensible way. You've got it backwards. All the issues that are being discussed now on this thread have been discussed previously on prior threads about row-level security. For the most part, nobody other than KaiGai and I participated in those, and we had a consensus hammered out that was reflected in the patch that started this thread. The person insisting on an eleventh-hour design change is you. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 21 December 2012 14:19, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Dec 21, 2012 at 3:56 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> It's unreasonable for people to demand a feature yet provide no >> guidance to the person trying (hard) to provide that feature in a >> sensible way. > > You've got it backwards. All the issues that are being discussed now > on this thread have been discussed previously on prior threads about > row-level security. For the most part, nobody other than KaiGai and I > participated in those, and we had a consensus hammered out that was > reflected in the patch that started this thread. The person insisting > on an eleventh-hour design change is you. Forwards or backwards, the problems of that previous design still exist and need some attention before we can commit. If you can spend some time investigating the problems and documenting how it works in that mode, it would be a great help for this patch. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Kohei KaiGai wrote: >> I don't think I like ALTER TABLE as a syntax for row level >> security. How about using existing GRANT syntax but allowing a >> WHERE clause? That seems more natural to me, and it would make >> it easy to apply the same conditions to multiple types of >> operations when desired, but use different expressions when >> desired. > > It seems to me this syntax gives an impression that row-security > feature is tightly combined with role-mechanism, even though it > does not need. For example, we can set row-security policy of > primary key is even/odd number, independent from working role. Is there some high-level discussion of the concept of row level security that operates independently of roles? I'm having a little trouble getting my head around the idea, there is no README in the patch, and the Wiki page on RLS hasn't been updated for two and a half years and seems to be mainly discussing the possibility of adding non-leaky views (which we now have). If it doesn't control which roles have access to which rows, what does it do, conceptually? (A URL to a page explaining this would be fine.) -Kevin
On 21 December 2012 14:48, Kevin Grittner <kgrittn@mail.com> wrote:
> Kohei KaiGai wrote:
>
>>> I don't think I like ALTER TABLE as a syntax for row level
>>> security. How about using existing GRANT syntax but allowing a
>>> WHERE clause? That seems more natural to me, and it would make
>>> it easy to apply the same conditions to multiple types of
>>> operations when desired, but use different expressions when
>>> desired.
>>
>> It seems to me this syntax gives an impression that row-security
>> feature is tightly combined with role-mechanism, even though it
>> does not need. For example, we can set row-security policy of
>> primary key is even/odd number, independent from working role.
>
> Is there some high-level discussion of the concept of row level
> security that operates independently of roles? I'm having a little
> trouble getting my head around the idea, there is no README in the
> patch, and the Wiki page on RLS hasn't been updated for two and a
> half years and seems to be mainly discussing the possibility of
> adding non-leaky views (which we now have). If it doesn't control
> which roles have access to which rows, what does it do,
> conceptually? (A URL to a page explaining this would be fine.)
There's some docs there, but that needs improving.
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).
If you push for GRANT ... WHERE then you may as well just say you want
the patch cancelled in this release. There's no way to analyze, design
and code something like that in 3 weeks. As I've said, I single
table-level policy is much easier to manage anyway.
-- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
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@mail.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@kaigai.gr.jp>
KaiGai, all,
* Kohei KaiGai (kaigai@kaigai.gr.jp) wrote:
> 2012/12/21 Kevin Grittner <kgrittn@mail.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@snowman.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@2ndQuadrant.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@snowman.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@kaigai.gr.jp>
2012/12/22 Simon Riggs <simon@2ndquadrant.com>: > On 21 December 2012 22:01, Stephen Frost <sfrost@snowman.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@kaigai.gr.jp>
Kohei KaiGai wrote: > 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. Thanks, I think that is essential for a productive discussion of the issue. For me, it would help tremendously if you could provide a very short statement of the over-arching goal of the current development effort. As an example, I could summarize the SSI development as: "Ensure that the result of executing any set of successfully committed serializable transactions is the same as having run those transactions one at a time, without introducing any new blocking." Proceeding from a general goal statement like that, to general principles of how it will be achieved before getting down to implementation details helps me put the details in proper context. I apologize again for coming in so late with strong opinions, but I thought I knew what "row level security" meant, and it was just a question of how to do it, but I can't reconcile what I thought the feature was about with the patch I'm seeing; perhaps it's just a lack of the hight level context that's making it difficult. -Kevin
On 22 December 2012 20:13, Kevin Grittner <kgrittn@mail.com> wrote: > I apologize again for coming in so late with strong opinions, but I > thought I knew what "row level security" meant, and it was just a > question of how to do it, but I can't reconcile what I thought the > feature was about with the patch I'm seeing; perhaps it's just a > lack of the hight level context that's making it difficult. Agreed, I think we're all feeling that. I'll do my best to accommodate all viewpoints. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
2012/12/22 Kevin Grittner <kgrittn@mail.com>: > Kohei KaiGai wrote: > >> 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. > > Thanks, I think that is essential for a productive discussion of > the issue. > I tried to update http://wiki.postgresql.org/wiki/RLS I backed to the definition of feature for information security; that requires to ensure confidentiality, integrity and availability (C.I.A) of information asset managed by system. Access control contributes the first two elements. So, I'm inclined RLS feature "eventually" support reader-side and writer-side, to prevent unprivileged rows are read or written. If I could introduce the most conceptual stuff in one statement, it shall be: "Overall, RLS prevents users to read and write rows that does not satisfies the row-security policy being configured on the table by the table owner. Reader-side ensures confidentiality of data, writer-side ensures integrity of data." Also note that, I believe this criteria never deny to have multiple (asymmetric) row-security policy for each command type, as long as we care about problematic scenario properly. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 21 December 2012 16:51, Kevin Grittner <kgrittn@mail.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@2ndQuadrant.com> writes:
> On 21 December 2012 16:51, Kevin Grittner <kgrittn@mail.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@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> On 21 December 2012 16:51, Kevin Grittner <kgrittn@mail.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@2ndquadrant.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@2ndquadrant.com>: > On 23 December 2012 18:49, Simon Riggs <simon@2ndquadrant.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@kaigai.gr.jp>
On Wed, Jan 02, 2013 at 05:35:13PM +0100, Kohei KaiGai wrote: > 2012/12/31 Simon Riggs <simon@2ndquadrant.com>: > > On 23 December 2012 18:49, Simon Riggs <simon@2ndquadrant.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.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@fetter.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@kaigai.gr.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@kaigai.gr.jp>
Attachment
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@kaigai.gr.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@kaigai.gr.jp> -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On 19 March 2013 15:08, Kohei KaiGai <kaigai@kaigai.gr.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@2ndquadrant.com>: > On 19 March 2013 15:08, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>
I moved Row Level Security patch to the 2013-Next commitfest. 2013/3/27 Kohei KaiGai <kaigai@kaigai.gr.jp>: > 2013/3/25 Simon Riggs <simon@2ndquadrant.com>: >> On 19 March 2013 15:08, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp> -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 27 March 2013 10:57, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2013/3/25 Simon Riggs <simon@2ndquadrant.com>: >> On 19 March 2013 15:08, Kohei KaiGai <kaigai@kaigai.gr.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