Thread: [PATCH] Add reloption for views to enable RLS

Hi all!

As part of a customer project we are looking to implement an reloption 
for views which when set, runs the subquery as invoked by the user 
rather than the view owner, as is currently the case.
The rewrite rule's table references are then checked as if the user were 
referencing the table(s) directly.

This feature is similar to so-called 'SECURITY INVOKER' views in other DBMS.
Although such permission checking could be implemented using views which 
SELECT from a table function and further using triggers, that approach 
has obvious performance downsides.

Our initial thought on implementing this was to simply add another 
reloption for views, just like the already existing `security_barrier`. 
With this in place, we then can conditionally evaluate in 
RelationBuildRuleLock() if we need to call setRuleCheckAsUser() or not.
The new reloption has been named `security`, which is an enum currently 
only supporting a single value: `relation_permissions`.

The code for fetching the rules and triggers in RelationBuildDesc() had 
to be moved after the parsing of the reloptions, since with this change 
RelationBuildRuleLock()now depends upon having relation->rd_options 
available.

The current behavior of views without that new reloption set is unaltered.
This is implemented as such in patch 0001.

Regression tests are included for both the new reloption of CREATE VIEW 
and the row level security side of this too, contained in patch 0002.
All regression tests are passing without errors.

Finally, patch 0003 updates the documentation for this new reloption.

An simplified example on how this feature can be used could look like this:

   CREATE TABLE people (id int, name text, company text);
   ALTER TABLE people ENABLE ROW LEVEL SECURITY;
   INSERT INTO people VALUES (1, 'alice', 'foo'), (2, 'bob', 'bar');

   CREATE VIEW customers_no_security
       AS SELECT * FROM people;

   CREATE VIEW customers
       WITH (security=relation_permissions)
       AS SELECT * FROM people;

   -- We want carol to only see people from company 'foo'
   CREATE ROLE carol;
   CREATE POLICY company_foo_only
       ON people FOR ALL TO carol USING (company = 'foo');

   GRANT SELECT ON people TO carol;
   GRANT SELECT ON customers_no_security TO carol;
   GRANT SELECT ON customers TO carol;

Now using these tables as carol:

     postgres=# SET ROLE carol;
     SET

For the `people` table, the policy is applied as expected:

     postgres=> SELECT * FROM people;
      id | name  | company
     ----+-------+---------
       1 | alice | foo
     (1 row)

If we now use the view with the new relopt set, the policy is applied too:

     postgres=> SELECT * FROM customers;
      id | name  | company
     ----+-------+---------
       1 | alice | foo
     (1 row)

But without the `security=relation_permissions` relopt, carol gets to 
see data they should not be able to due to the policy not being applied, 
since the rules are checked against the view owner:

     postgres=> SELECT * FROM customers_no_security;
      id | name  | company
     ----+-------+---------
       1 | alice | foo
       2 | bob   | bar
     (2 rows)


Excluding regression tests and documentation, the changes boil down to this:
  src/backend/access/common/reloptions.c    | 20
  src/backend/nodes/copyfuncs.c             |  1
  src/backend/nodes/equalfuncs.c            |  1
  src/backend/nodes/outfuncs.c              |  1
  src/backend/nodes/readfuncs.c             |  1
  src/backend/optimizer/plan/subselect.c    |  1
  src/backend/optimizer/prep/prepjointree.c |  1
  src/backend/rewrite/rewriteHandler.c      |  1
  src/backend/utils/cache/relcache.c        | 62
  src/include/nodes/parsenodes.h            |  3
  src/include/utils/rel.h                   | 21
  11 files changed, 84 insertions(+), 29 deletions(-)

All patches are against current master.

Thanks,
Christoph Heiss

Hi Laurenz,

thanks for the review!
I've attached a v2 where I addressed the things you mentioned.

On 1/11/22 19:59, Laurenz Albe wrote:
> [..]
> 
> You made that an enum with only a single value.
> What other values could you imagine in the future?
> 
> I think that this should be a boolean reloption, for example "security_definer".
> If unset or set to "off", you would get the current behavior.

A boolean option would have been indeed the better choice, I agree.
I haven't though of any specific other values for this enum, it was 
rather a decision following a off-list discussion.

I've changed the option to be boolean and renamed it to 
"security_invoker". This puts it in line with how other systems (e.g. 
MySQL) name their equivalent feature, so I think this should be an 
appropriate choice.

> 
>> Finally, patch 0003 updates the documentation for this new reloption.
> 
> [..]
> 
> Please avoid long lines like that.  

Fixed.

> Also, I don't think that the documentation on
> RLS policies is the correct place for this.  It should be on a page dedicated to views
> or permissions.
> 
> The CREATE VIEW page already has a paragraph about this, starting with
> "Access to tables referenced in the view is determined by permissions of the view owner."
> This looks like the best place to me (and it would need to be adapted anyway).
It makes sense to put it there, thanks for the pointer! I wasn't really 
that sure where to put the documentation to start with, and this seems 
like a more appropriate place.

Please review further.

Thanks,
Christoph Heiss

Hi,

On Tue, Jan 18, 2022 at 04:16:53PM +0100, Christoph Heiss wrote:
> 
> I've attached a v2 where I addressed the things you mentioned.

This version unfortunately doesn't apply anymore:
http://cfbot.cputube.org/patch_36_3466.log
=== Applying patches on top of PostgreSQL commit ID e0e567a106726f6709601ee7cffe73eb6da8084e ===
=== applying patch ./0001-PATCH-v2-1-3-Add-new-boolean-reloption-security_invo.patch
=== applying patch ./0002-PATCH-v2-2-3-Add-regression-tests-for-new-security_i.patch
patching file src/test/regress/expected/create_view.out
Hunk #5 FAILED at 2019.
Hunk #6 succeeded at 2056 (offset 16 lines).
1 out of 6 hunks FAILED -- saving rejects to file src/test/regress/expected/create_view.out.rej

Could you send a rebased version?

Hi,

On 1/19/22 09:30, Julien Rouhaud wrote:
> Hi,
> 
> On Tue, Jan 18, 2022 at 04:16:53PM +0100, Christoph Heiss wrote:
>>
>> I've attached a v2 where I addressed the things you mentioned.
> 
> This version unfortunately doesn't apply anymore:
> http://cfbot.cputube.org/patch_36_3466.log
> === Applying patches on top of PostgreSQL commit ID e0e567a106726f6709601ee7cffe73eb6da8084e ===
> === applying patch ./0001-PATCH-v2-1-3-Add-new-boolean-reloption-security_invo.patch
> === applying patch ./0002-PATCH-v2-2-3-Add-regression-tests-for-new-security_i.patch
> patching file src/test/regress/expected/create_view.out
> Hunk #5 FAILED at 2019.
> Hunk #6 succeeded at 2056 (offset 16 lines).
> 1 out of 6 hunks FAILED -- saving rejects to file src/test/regress/expected/create_view.out.rej
> 
> Could you send a rebased version?

My bad - I attached a new version rebased on latest master.

Thanks,
Christoph Heiss

Hi Laurenz,

thank you again for the review!

On 1/20/22 15:20, Laurenz Albe wrote:
> [..]
> I gave the new patch a spin, and got a surprising result:
> 
>    [..]
> 
>    INSERT INTO v VALUES (1);
>    INSERT 0 1
> 
> Huh?  "duff" has no permission to insert into "tab"!
That really should not happen, thanks for finding that and helping me 
investigating on how to fix that!

This is now solved by checking the security_invoker property on the view 
in rewriteTargetView().

I've also added a testcase for this in v4 to catch that in future.

> 
> [..]
> 
> About the documentation:
> 
> --- a/doc/src/sgml/ref/create_view.sgml
> +++ b/doc/src/sgml/ref/create_view.sgml
> +       <varlistentry>
> +        <term><literal>security_invoker</literal> (<type>boolean</type>)</term>
> +        <listitem>
> +         <para>
> +          If this option is set, it will cause all access to the underlying
> +          tables to be checked as referenced by the invoking user, rather than
> +          the view owner.  This will only take effect when row level security is
> +          enabled on the underlying tables (using <link linkend="sql-altertable">
> +          <command>ALTER TABLE ... ENABLE ROW LEVEL SECURITY</command></link>).
> +         </para>
> 
> Why should this *only* take effect if (not "when") RLS is enabled?
> The above test shows that there is an effect even without RLS.
> 
> +         <para>This option can be changed on existing views using <link
> +          linkend="sql-alterview"><command>ALTER VIEW</command></link>. See
> +          <xref linkend="ddl-rowsecurity"/> for more details on row level security.
> +         </para>
> 
> I don't think that it is necessary to mention that this can be changed with
> ALTER VIEW - all storage parameters can be.  I guess you copied that from
> the "check_option" documentation, but I would say it need not be mentioned
> there either.
Exactly, I tried to fit it in with the existing parameters.
I moved the link to ALTER VIEW to the end of the paragraph, as it 
applies to all options anyways.

> 
> +   <para>
> +    If the <firstterm>security_invoker</firstterm> option is set on the view,
> +    access to tables is determined by permissions of the invoking user, rather
> +    than the view owner.  This can be used to provide stricter permission
> +    checking to the underlying tables than by default.
>      </para>
> 
> Since you are talking about use cases here, RLS might deserve a mention.
Expanded upon a little bit in v4.

> 
> --- a/src/backend/access/common/reloptions.c
> +++ b/src/backend/access/common/reloptions.c
> +   {
> +       {
> +           "security_invoker",
> +           "View subquery in invoked within the current security context.",
> +           RELOPT_KIND_VIEW,
> +           AccessExclusiveLock
> +       },
> +       false
> +   },
> 
> That doesn't seem to be proper English.
Yes, that happened when rewriting this for v1 -> v2.
Fixed.

Thanks,
Christoph Heiss

Christoph Heiss wrote:
> As part of a customer project we are looking to implement an reloption for views which when set, runs the subquery as
invokedby the user rather than the view owner, as is currently the case.
 
> The rewrite rule's table references are then checked as if the user were referencing the table(s) directly.
> 
> This feature is similar to so-called 'SECURITY INVOKER' views in other DBMS. 

This is a feature I have long been looking for. I tested the patch (v5) 
and found two cases that I feel need to be either fixed or documented 
explicitly.


Case 1 - Schema privileges:

create schema a;
create table a.t();

create schema b;
create view b.v with (security_invoker=true) as table a.t;

create role alice;
grant usage on schema b to alice; -- missing schema a
grant select on table a.t, b.v to alice;

set role alice;
table a.t; -- ERROR: permission denied for schema a (good)
table b.v; -- no error (good or bad?)

User alice does not have USAGE privileges on schema a, but only on table 
a.t. A SELECT directly on the table fails as expected, but a SELECT on 
the view succeeds. I assume the schema access is checked when the query 
is parsed - and at that stage, the user is still the view owner?
The docs mention explicitly that *all* objects are accessed with invoker 
privileges, which is not the case.

Personally I actually like this. It allows to keep a view-based api in a 
separate schema, while:
- preserving full RLS capabilities and
- forcing the user to go through the api, because a direct access to the 
data schema is not possible.

However, since this behavior was likely unintended until now, it raises 
the question whether there are any other privilege checks that are not 
taking the invoking user into account properly?


Case 2 - Chained views:

create schema a;
create table a.t();

create role bob;
grant create on database postgres to bob;
grant usage on schema a to bob;
set role bob;
create schema b;
create view b.v1 with (security_invoker=true) as table a.t;
create view b.v2 with (security_invoker=false) as table b.v1;

reset role;
create role alice;
grant usage on schema a, b to alice;
grant select on table a.t to bob;
grant select on table b.v2 to alice;

set role alice;
table b.v2; -- ERROR: permission denied for table t (bad)

When alice runs the SELECT on b.v2, the query on b.v1 is made with bob 
privileges as the view owner of b.v2. This is verified, because alice 
does not have privileges to access b.v1, but no such error is thrown.

b.v1 will then access a.t - and my first assumption was, that in this 
case a.t should be accessed by bob, still as the view owner of b.v2. 
Clearly, this is not the case as the permission denied error shows.

This is not actually a problem with this patch, I think, but just 
highlighting a quirk in the current implementation of views 
(security_invoker=false) in general: While the query will be run with 
the view owner, the CURRENT_USER is still the invoker, even "after" the 
view. In other words, the current implementation is *not* the same as 
"security definer". It's somewhere between "security definer" and 
"security invoker" - a strange mix really.

Afaik this mix is not documented explicitly so far. But the 
security_invoker reloption exposes it in a much less expected way, so I 
only see two options really:
a) make the current implementation of security_invoker=false a true 
"security definer", i.e. change the CURRENT_USER "after" the view for good.
b) document the "security infiner/devoker" default behavior as a feature.

I really like a), as this would make a clear cut between security 
definer and security invoker views - but this would be a big breaking 
change, which I don't think is acceptable.

Best,

Wolfgang

Laurenz Albe:
> So even though the view owner "duff" has no permissions
> on the schema "viewtest", we can still select from the table.
> Permissions on the schema containing the table are not
> checked, only permissions on the table itself.
> 
> [...]
> 
> If not, I don't know if it is the business of this patch to
> change the behavior.

Ah, good find. In that case, I suggest to change the docs slightly to 
say that the schema will not be checked.

In one place it's described as "it will cause all access to the 
underlying tables to be checked as ..." which is fine, I think. But in 
another place it's "access to tables, functions and *other objects* 
referenced in the view, ..." which is misleading.

> I agree that the name "security_invoker" is suggestive of SECURITY INVOKER
> in CREATE FUNCTION, but the behavior is different.
> Perhaps the solution is as simple as choosing a different name that does
> not prompt this association, for example "permissions_invoker".

Yes, given that there is not much that can be done about the 
functionality anymore, a different name would be better. This would also 
avoid the implicit "if security_invoker=false, the view behaves like 
SECURITY DEFINER" association, which is also clearly wrong. And this 
assumption is actually what made me think the chained views example was 
somehow off.

I am not convinced "permissions_invoker" is much better, though. The 
difference between SECURITY INVOKER and SECURITY DEFINER is invoker vs 
definer... where, I think, we need something else to describe what we 
currently have and what the patch provides.

Maybe we can look at it from the other perspective: Both ways of 
operating keep the CURRENT_USER the same, pretty much like what we 
understand "security invoker" should do. The difference, however, is the 
current default in which the permissions are checked with the view 
*owner*. Let's treat this difference as the thing that can be set: 
security_owner=true|false. Or run_as_owner=true|false.

xxx_owner=true would be the default and xxx_owner=false could be set 
explicitly to get the behavior we are looking for in this patch?


> I guess more documentation how this works would be a good idea.
> [...] but since it exposes this
> in new ways, it might as well clarify how all this works.

+1

Best

Wolfgang

Hi all,

again, many thanks for the reviews and testing!

On 2/4/22 17:09, Laurenz Albe wrote:
> I also see no reason to split a small patch like this into three parts.
I've split it into the three unrelated parts (code, docs, tests) to ease 
review, but I happily carry it as one patch too.

> In the attached, I dealt with the above and went over the comments.
> How do you like it?

That is really nice, I used it to base v6 on.

On 2/9/22 17:40, walther@technowledgy.de wrote:
> Ah, good find. In that case, I suggest to change the docs slightly to 
> say that the schema will not be checked.
> 
> In one place it's described as "it will cause all access to the 
> underlying tables to be checked as ..." which is fine, I think. But in 
> another place it's "access to tables, functions and *other objects* 
> referenced in the view, ..." which is misleading
I removed the reference to "other objects" for now in v6.

>> I agree that the name "security_invoker" is suggestive of SECURITY 
>> INVOKER
>> in CREATE FUNCTION, but the behavior is different.
>> Perhaps the solution is as simple as choosing a different name that does
>> not prompt this association, for example "permissions_invoker".
> 
> Yes, given that there is not much that can be done about the 
> functionality anymore, a different name would be better. This would also 
> avoid the implicit "if security_invoker=false, the view behaves like 
> SECURITY DEFINER" association, which is also clearly wrong. And this 
> assumption is actually what made me think the chained views example was 
> somehow off.
> 
> I am not convinced "permissions_invoker" is much better, though. The 
> difference between SECURITY INVOKER and SECURITY DEFINER is invoker vs 
> definer... where, I think, we need something else to describe what we 
> currently have and what the patch provides.
> 
> Maybe we can look at it from the other perspective: Both ways of 
> operating keep the CURRENT_USER the same, pretty much like what we 
> understand "security invoker" should do. The difference, however, is the 
> current default in which the permissions are checked with the view 
> *owner*. Let's treat this difference as the thing that can be set: 
> security_owner=true|false. Or run_as_owner=true|false.
> 
> xxx_owner=true would be the default and xxx_owner=false could be set 
> explicitly to get the behavior we are looking for in this patch?

I'm not sure if an option which is on by default would be best, IMHO. I 
would rather have an off-by-default option, so that you explicitly have 
to turn *on* that behavior rather than turning *off* the current.

[ Pretty much bike-shedding here, but if the agreement comes to one of 
"xxx_owner" I won't mind it either. ]

My best suggestions is maybe something like run_as_invoker=t|f, but that 
would probably raise the same "invoker vs definer" association.

I left it for now as-is.

>> I guess more documentation how this works would be a good idea.
>> [...] but since it exposes this
>> in new ways, it might as well clarify how all this works.

I tried to clarify this situation in the documentation in a concise 
matter, I'd appreciate further feedback on that.

Thanks,
Christoph Heiss

Laurenz Albe:
> So even though the view owner "duff" has no permissions
> on the schema "viewtest", we can still select from the table.
> Permissions on the schema containing the table are not
> checked, only permissions on the table itself.
> 
> I am not sure how to feel about this.  It is not what I would have
> expected, but changing it would be a compatibility break.
> Should this be considered a live bug in PostgreSQL?

I now found the docs to say:


USAGE:
For schemas, allows access to objects contained in the schema (assuming 
that the objects' own privilege requirements are also met). Essentially 
this allows the grantee to “look up” objects within the schema. Without 
this permission, it is still possible to see the object names, e.g., by 
querying system catalogs. Also, after revoking this permission, existing 
sessions might have statements that have previously performed this 
lookup, so this is not a completely secure way to prevent object access.


So, this seems to be perfectly fine.

Best

Wolfgang

Christoph Heiss:
>> xxx_owner=true would be the default and xxx_owner=false could be set 
>> explicitly to get the behavior we are looking for in this patch?
> 
> I'm not sure if an option which is on by default would be best, IMHO. I 
> would rather have an off-by-default option, so that you explicitly have 
> to turn *on* that behavior rather than turning *off* the current.

Just out of curiosity I asked myself whether there were any other 
boolean options that default to true in postgres - and there are plenty. 
./configure options, client connection settings, server config options, 
etc - but also some SQL statements:
- CREATE USER defaults to LOGIN
- CREATE ROLE defaults to INHERIT
- CREATE COLLATION defaults to DETERMINISTIC=true

There's even reloptions, that do, e.g. vacuum_truncate.


> My best suggestions is maybe something like run_as_invoker=t|f, but that 
> would probably raise the same "invoker vs definer" association.

It is slightly better, I agree. But, yes, that same association is 
raised easily. The more I think about it, the more it becomes clear that 
really the current default behavior of "running the query as the view 
owner" is the special thing here, not the behavior you are introducing.

If we were to start from scratch, it would be pretty obvious - to me - 
that run_as_owner=false would be the default, and the run_as_owner=true 
would need to be turned on explicitly. I'm thinking about "run_as_owner" 
as the better design and "defaults to true" as a backwards compatibility 
thing.

But yeah, it would be good to hear other opinions on that, too.

Best

Wolfgang

Hi,

On 2/15/22 09:37, walther@technowledgy.de wrote:
> Christoph Heiss:
>>> xxx_owner=true would be the default and xxx_owner=false could be set 
>>> explicitly to get the behavior we are looking for in this patch?
>>
>> I'm not sure if an option which is on by default would be best, IMHO. 
>> I would rather have an off-by-default option, so that you explicitly 
>> have to turn *on* that behavior rather than turning *off* the current.
> 
> Just out of curiosity I asked myself whether there were any other 
> boolean options that default to true in postgres - and there are plenty. 
> ./configure options, client connection settings, server config options, 
> etc - but also some SQL statements:
> - CREATE USER defaults to LOGIN
> - CREATE ROLE defaults to INHERIT
> - CREATE COLLATION defaults to DETERMINISTIC=true
> 
> There's even reloptions, that do, e.g. vacuum_truncate.

Knowing that I happily drop my objection about that. :^)

> [..] The more I think about it, the more it becomes clear that 
> really the current default behavior of "running the query as the view 
> owner" is the special thing here, not the behavior you are introducing.
> 
> If we were to start from scratch, it would be pretty obvious - to me - 
> that run_as_owner=false would be the default, and the run_as_owner=true 
> would need to be turned on explicitly. I'm thinking about "run_as_owner" 
> as the better design and "defaults to true" as a backwards compatibility 
> thing.

Right, if we treat that as a kind of "backwards-compatible" feature, 
having an reloption that is on by default makes sense.

I converted the option to run_as_owner=true|false in the attached v7.
It now definitely seems like the right way to move forward and getting 
more feedback.

Thanks,
Christoph Heiss

Laurenz Albe:
>> I converted the option to run_as_owner=true|false in the attached v7.
>> It now definitely seems like the right way to move forward and getting
>> more feedback.
> I think we are straying from the target.
> 
> "run_as_owner" seems wrong to me, because it is all about permission
> checking and*not*  about running.  As we have established, the query
> is always executed by the caller.
> 
> So my preferred bikeshed colors would be "permissions_owner" or
> "permissions_caller".

My main point was the "xxx_owner = true by default" thing. Whether xxx 
is "permissions" or "run_as" doesn't change that. permissions_caller, 
however, would be a step backwards.

I can see how permissions_owner is better than run_as_owner. The code 
uses checkAsUser, so check_as_owner would be an option, too. Although 
that could easily be associated with WITH CHECK OPTION. Thinking about 
that, the difference between LOCAL and CASCADED for CHECK OPTION pretty 
much sums up one of the confusing bits about the whole thing, too.

Maybe "local_permissions_owner = true | false"? That would make it 
crystal-clear, that this is only about the very first permissions check 
and not about any checks later in a chain of multiple views.

"local_permissions = owner | caller" could also work - as long as we're 
not using any of definer or invoker.

Best

Wolfgang

Laurenz Albe:
> I'd be happy with "check_as_owner", except it is unclear *what* is checked.

Yeah, that could be associated with WITH CHECK OPTION, too, as in "do 
the CHECK OPTION stuff as the owner".

> "check_permissions_as_owner" is ok with me, but a bit long.

check_permissions_as_owner is exactly what happens. The additional "as" 
shouldn't be a problem in length - but is much better to read. I 
wouldn't associate that with CHECK OPTION either. +1

Best

Wolfgang

On Fri, 18 Feb 2022 at 14:57, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> Here is a new version, with improved documentation and the option renamed
> to "check_permissions_owner".  I just prefer the shorter form.
>

Re-reading this thread, I think I preferred the name
"security_invoker". The main objection seemed to come from the
potential confusion with SECURITY INVOKER/DEFINER functions, but I
think that's really a different thing. As long as the documentation
for the default behaviour is clear (which I think it was), then it
should be easy to explain how a security invoker view behaves
differently. Also, there's value in using the same terminology as
other databases, because many users will already be familiar with the
feature from those databases.

Some other review comments:

1). This new comment:

+   <para>
+    Be aware that <literal>USAGE</literal> privileges on schemas containing
+    the underlying base relations are <emphasis>not</emphasis> checked.
+   </para>

is not entirely accurate. It's more accurate to say that a user
creating or replacing a view must have CREATE privileges on the schema
containing the view and USAGE privileges on any schemas referred to in
the view query, whereas a user using the view only needs USAGE
privileges on the schema containing the view.

(Note that, for the view creator, USAGE is required on any schema
referred to in the query -- e.g., schemas containing functions as well
as base relations.)

2). The patch is adding a new field to RangeTblEntry which seems to be
unnecessary -- it's set, and copied around, but never read, so it
should just be removed.

3). Looking at this change:

-        setRuleCheckAsUser((Node *) rule->actions, relation->rd_rel->relowner);
-        setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
+        if (!(relation->rd_rel->relkind == RELKIND_VIEW
+              && !RelationSubqueryCheckPermsOwner(relation)))
+        {
+            setRuleCheckAsUser((Node *) rule->actions,
relation->rd_rel->relowner);
+            setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
+        }

I think it should call setRuleCheckAsUser() in all cases. It might be
true that the rule fetched has checkAsUser set to InvalidOid
throughout its action and quals, but it seems unwise to rely on that
-- better to code defensively and explicitly set it in all cases.

4). In the same code block, I think the new behaviour should be
applied to SELECT rules only. The view may have other non-SELECT rules
(just as a table may have non-SELECT rules), created using CREATE
RULE, but their actions are independent of the view definition.
Currently their permissions are checked as the view/table owner, and
if anyone wanted to change that, it should be an option on the rule,
not the view (just as triggers can be made security definer or
invoker, depending on how the trigger function is defined).

(Note: I'm not suggesting that anyone actually spend any time adding
such an option to rules. Given all the pitfalls associated with rules,
I think their use should be discouraged, and no development effort
should be expended enhancing them.)

5). In the same function, the block of code that fetches rules and
triggers has been moved. I think it would be worth adding a comment to
explain why it's now important to extract the reloptions *before*
fetching the relation's rules and triggers.

6). The second set of tests added to rowsecurity.sql seem to have
nothing to do with RLS, and probably belong in updatable_views.sql,
and I think it would be worth adding a few more tests for things like
views on top of views.

Regards,
Dean

Thanks for reviewing!

On 2/25/22 19:22, Dean Rasheed wrote:
> Re-reading this thread, I think I preferred the name
> "security_invoker". The main objection seemed to come from the
> potential confusion with SECURITY INVOKER/DEFINER functions, but I
> think that's really a different thing. As long as the documentation
> for the default behaviour is clear (which I think it was), then it
> should be easy to explain how a security invoker view behaves
> differently. Also, there's value in using the same terminology as
> other databases, because many users will already be familiar with the
> feature from those databases.

That is also the main reason I preferred naming it "security_invoker" - 
it is consistent with other databases and eases transition from such 
systems.

I kept "check_permissions_owner" for now. Constantly changing it around 
with each iteration doesn't really bring any value IMHO, I'd rather have 
a final consensus on how to name the option and *then* change it for good.

> 
> Some other review comments:
> 
> 1). This new comment:
> 
> +   <para>
> +    Be aware that <literal>USAGE</literal> privileges on schemas containing
> +    the underlying base relations are <emphasis>not</emphasis> checked.
> +   </para>
> 
> is not entirely accurate. It's more accurate to say that a user
> creating or replacing a view must have CREATE privileges on the schema
> containing the view and USAGE privileges on any schemas referred to in
> the view query, whereas a user using the view only needs USAGE
> privileges on the schema containing the view.
> 
> (Note that, for the view creator, USAGE is required on any schema
> referred to in the query -- e.g., schemas containing functions as well
> as base relations.)

Improved in the attached v9.

> 
> 2). The patch is adding a new field to RangeTblEntry which seems to be
> unnecessary -- it's set, and copied around, but never read, so it
> should just be removed.

I removed that field in v9 since it is indeed completely unused. I 
initially added it to be consistent with the "security_barrier" 
implementation and than somewhat forgot about it.

> 
> 3). Looking at this change:
> 
> [..]
> 
> I think it should call setRuleCheckAsUser() in all cases. It might be
> true that the rule fetched has checkAsUser set to InvalidOid
> throughout its action and quals, but it seems unwise to rely on that
> -- better to code defensively and explicitly set it in all cases.

It probably doesn't really matter, but I agree that coding defensively 
is always a good thing.
Changed that in v9 to call setRuleCheckAsUser() either with ->relowner 
or InvalidOid.

> 
> 4). In the same code block, I think the new behaviour should be
> applied to SELECT rules only. The view may have other non-SELECT rules
> (just as a table may have non-SELECT rules), created using CREATE
> RULE, but their actions are independent of the view definition.
> Currently their permissions are checked as the view/table owner, and
> if anyone wanted to change that, it should be an option on the rule,
> not the view (just as triggers can be made security definer or
> invoker, depending on how the trigger function is defined).
> 

Good catch, I added a additional check for rule->event and a test for 
that in v9.
[ I also had to add a missing DROP statement to some previous test, just 
a heads up. ]

It makes sense to mimic the behavior of triggers and further, 
user-created rules otherwise might behave differently for tables and 
views, depending on the view definition.
[ But I'm not _that_ familiar with CREATE RULE, FWIW. ]

> 
> 5). In the same function, the block of code that fetches rules and
> triggers has been moved. I think it would be worth adding a comment to
> explain why it's now important to extract the reloptions *before*
> fetching the relation's rules and triggers.

Added a small comment explaining that in v9.

> 
> 6). The second set of tests added to rowsecurity.sql seem to have
> nothing to do with RLS, and probably belong in updatable_views.sql,
> and I think it would be worth adding a few more tests for things like
> views on top of views.

Seems reasonable to move them into updatable_views.sql, done that for 
v9. Further I added two (simple) tests for chained views as you 
mentioned, hope they reflect what you had in mind.

Thanks,
Christoph

On Tue, 1 Mar 2022 at 16:40, Christoph Heiss
<christoph.heiss@cybertec.at> wrote:
>
> That is also the main reason I preferred naming it "security_invoker" -
> it is consistent with other databases and eases transition from such
> systems.
>
> I kept "check_permissions_owner" for now. Constantly changing it around
> with each iteration doesn't really bring any value IMHO, I'd rather have
> a final consensus on how to name the option and *then* change it for good.
>

Yes indeed, it's annoying to keep changing the name between patch
versions, so let's try to get a consensus now.

For my part, I find myself more and more convinced that
"security_invoker" is the right name, because it matches the
terminology used for functions, and in other database systems. I think
the parallels between security invoker functions and security invoker
views are quite strong.

There are a couple of additional considerations that lend weight to
that choice of name, though not uniquely to it:

1). There is a slight advantage to having an option that defaults to
false/off, like the existing "security_barrier" option -- it allows a
shorthand to turn the option on, because the system automatically
turns "WITH (security_barrier)" into "WITH (security_barrier=true)".

2). Grammatically, a name like this works better, because it serves
both as the name of the boolean option, and as an adjective that can
be used to describe and name the feature -- as in "security barrier
views are cool" -- making it easier to talk about the feature.

"check_permissions_owner=false" doesn't work as well in either regard,
and just feels much more clumsy.

When we come to write the release notes for this feature, saying that
this version of PG now supports security invoker views is going to
mean a lot more to people who already use that feature in other
databases.

What are other people's opinions?

Regards,
Dean

Dean Rasheed:
>> That is also the main reason I preferred naming it "security_invoker" -
>> it is consistent with other databases and eases transition from such
>> systems.
> [...]
>
> For my part, I find myself more and more convinced that
> "security_invoker" is the right name, because it matches the
> terminology used for functions, and in other database systems. I think
> the parallels between security invoker functions and security invoker
> views are quite strong.
>
> [...]
>
> When we come to write the release notes for this feature, saying that
> this version of PG now supports security invoker views is going to
> mean a lot more to people who already use that feature in other
> databases.
>
> What are other people's opinions?

All those points in favor of security_invoker are very good indeed. The 
main objection was not the term invoker, though, but the implicit 
association it creates as in "security_invoker=false behaves like 
security definer". But this is clearly wrong, the "security definer" 
semantics as used for functions or in other databases just don't apply 
as the default in PG.

I think renaming the reloption was a shortcut to avoid that association, 
while the best way to deal with that would be explicit documentation. 
Meanwhile, the patch has added a mention about CURRENT_USER, so that's a 
first step. Maybe an explicit mention that security_invoker=false, is 
NOT the same as "security definer" and explaining why would already be 
enough?

Best

Wolfgang

On Wed, 2022-03-02 at 10:10 +0000, Dean Rasheed wrote:
> > I kept "check_permissions_owner" for now. Constantly changing it around
> > with each iteration doesn't really bring any value IMHO, I'd rather have
> > a final consensus on how to name the option and *then* change it for good.
> 
> Yes indeed, it's annoying to keep changing the name between patch
> versions, so let's try to get a consensus now.
> 
> For my part, I find myself more and more convinced that
> "security_invoker" is the right name [...]
> 
> What are other people's opinions?

I am fine with "security_invoker".  If there are other databases that use the
same term for the same thing, that is a strong argument.

I also agree that having "off" for the default setting is nicer.

My main worry is that other people misunderstand it in the same way that
Walter did, namely that this behaves just like security invoker functions.
But if the behavior is well documented, I think that is ok.

Yours,
Laurenz Albe

On 3/2/22 11:10, Dean Rasheed wrote:
> For my part, I find myself more and more convinced that
> "security_invoker" is the right name, because it matches the
> terminology used for functions, and in other database systems. I think
> the parallels between security invoker functions and security invoker
> views are quite strong.
> 
> [..]
> 
> What are other people's opinions?
> 

Since there don't seem to be any more objections to "security_invoker" I 
attached v10 renaming it again.

I've tried to better clarify the whole invoker vs. definer thing in the 
CREATE VIEW documentation by explicitly mentioning that 
"security_invoker=false" is _not_ the same as "security definer", based 
on the earlier discussions.

This should hopefully avoid any implicit associations.

Thanks,
Christoph

On Tue, 2022-03-08 at 18:17 +0100, Christoph Heiss wrote:
> Since there don't seem to be any more objections to "security_invoker" I 
> attached v10 renaming it again.
> 
> I've tried to better clarify the whole invoker vs. definer thing in the 
> CREATE VIEW documentation by explicitly mentioning that 
> "security_invoker=false" is _not_ the same as "security definer", based 
> on the earlier discussions.
> 
> This should hopefully avoid any implicit associations.

I have only some minor comments:

> --- a/doc/src/sgml/ref/create_view.sgml
> +++ b/doc/src/sgml/ref/create_view.sgml
> @@ -387,10 +430,17 @@ CREATE VIEW vista AS SELECT text 'Hello World' AS hello;
>     <para>
>      Note that the user performing the insert, update or delete on the view
>      must have the corresponding insert, update or delete privilege on the
> -    view.  In addition the view's owner must have the relevant privileges on
> -    the underlying base relations, but the user performing the update does
> -    not need any permissions on the underlying base relations (see
> -    <xref linkend="rules-privileges"/>).
> +    view.
> +   </para>
> +
> +   <para>
> +    Additionally, by default the view's owner must have the relevant privileges
> +    on the underlying base relations, but the user performing the update does
> +    not need any permissions on the underlying base relations. (see
> +    <xref linkend="rules-privileges"/>)  If the view has the
> +    <literal>security_invoker</literal> property is set to
> +    <literal>true</literal>, the invoking user will need to have the relevant
> +    privileges rather than the view owner.
>     </para>
>    </refsect2>
>   </refsect1>

This paragraph contains a couple of grammatical errors.
How about

  <para>
   Note that the user performing the insert, update or delete on the view
   must have the corresponding insert, update or delete privilege on the
   view.  Unless <literal>security_invoker</literal> is set to
   <literal>true</literal>, the view's owner must additionally have the
   relevant privileges on the underlying base relations, but the user
   performing the update does not need any permissions on the underlying
   base relations (see <xref linkend="rules-privileges"/>).
   If <literal>security_invoker</literal> is set to <literal>true</literal>,
   it is the invoking user rather than the view owner that must have the
   relevant privileges on the underlying base relations.
  </para>

Also, this:

> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -838,8 +846,18 @@ RelationBuildRuleLock(Relation relation)
>          * the rule tree during load is relatively cheap (compared to
>          * constructing it in the first place), so we do it here.
>          */
> -       setRuleCheckAsUser((Node *) rule->actions, relation->rd_rel->relowner);
> -       setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
> +       if (rule->event == CMD_SELECT
> +           && relation->rd_rel->relkind == RELKIND_VIEW
> +           && RelationHasSecurityInvoker(relation))
> +       {
> +           setRuleCheckAsUser((Node *) rule->actions, InvalidOid);
> +           setRuleCheckAsUser(rule->qual, InvalidOid);
> +       }
> +       else
> +       {
> +           setRuleCheckAsUser((Node *) rule->actions, relation->rd_rel->relowner);
> +           setRuleCheckAsUser(rule->qual, relation->rd_rel->relowner);
> +       }

could be written like this (introducing a new variable):

  if (rule->event == CMD_SELECT
      && relation->rd_rel->relkind == RELKIND_VIEW
      && RelationHasSecurityInvoker(relation))
      user_for_check = InvalidOid;
  else
      user_for_check = relation->rd_rel->relowner;

  setRuleCheckAsUser((Node *) rule->actions, user_for_check);
  setRuleCheckAsUser(rule->qual, user_for_check);

This might be easier to read.


Yours,
Laurenz Albe

On 3/9/22 16:06, Laurenz Albe wrote:
> This paragraph contains a couple of grammatical errors.
> How about
> 
>    <para>
>     Note that the user performing the insert, update or delete on the view
>     must have the corresponding insert, update or delete privilege on the
>     view.  Unless <literal>security_invoker</literal> is set to
>     <literal>true</literal>, the view's owner must additionally have the
>     relevant privileges on the underlying base relations, but the user
>     performing the update does not need any permissions on the underlying
>     base relations (see <xref linkend="rules-privileges"/>).
>     If <literal>security_invoker</literal> is set to <literal>true</literal>,
>     it is the invoking user rather than the view owner that must have the
>     relevant privileges on the underlying base relations.
>    </para>

Replaced the two paragraphs with your suggestion, it is indeed easier to 
read.

> 
> Also, this:
> 
> [..]
> 
> could be written like this (introducing a new variable):
> 
>    if (rule->event == CMD_SELECT
>        && relation->rd_rel->relkind == RELKIND_VIEW
>        && RelationHasSecurityInvoker(relation))
>        user_for_check = InvalidOid;
>    else
>        user_for_check = relation->rd_rel->relowner;
> 
>    setRuleCheckAsUser((Node *) rule->actions, user_for_check);
>    setRuleCheckAsUser(rule->qual, user_for_check);
> 
> This might be easier to read.

Makes sense, I've changed that. This also seems to be more in line with 
all the other code.
While at it I also split the comment alongside it to match, hopefully 
that makes sense.

Thanks,
Christoph Heiss

On Mon, 2022-03-14 at 13:40 +0100, Christoph Heiss wrote:
> On 3/9/22 16:06, Laurenz Albe wrote:
> > This paragraph contains a couple of grammatical errors.
>
> Replaced the two paragraphs with your suggestion, it is indeed easier to 
> read.
> 
> > Also, this:
> > could be written like this (introducing a new variable):
> > 
> >    if (rule->event == CMD_SELECT
> >        && relation->rd_rel->relkind == RELKIND_VIEW
> >        && RelationHasSecurityInvoker(relation))
> >        user_for_check = InvalidOid;
> >    else
> >        user_for_check = relation->rd_rel->relowner;
> > 
> >    setRuleCheckAsUser((Node *) rule->actions, user_for_check);
> >    setRuleCheckAsUser(rule->qual, user_for_check);
> > 
> > This might be easier to read.
> 
> Makes sense, I've changed that. This also seems to be more in line with 
> all the other code.
> While at it I also split the comment alongside it to match, hopefully 
> that makes sense.

The patch is fine from my point of view.

It passes "make check-world".

I'll mark it as "ready for committer".

Yours,
Laurenz Albe

On Mon, 14 Mar 2022 at 16:16, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> The patch is fine from my point of view.
>
> It passes "make check-world".
>
> I'll mark it as "ready for committer".
>

Cool, thanks. I think this will make a useful addition to PG15.

I have been hacking on it a bit, and attached is an updated version.
Aside from some general copy editing, the most notable changes are:

In the updatable_views tests, I have moved the new tests to
immediately after the existing permission checking tests, which seems
like a more logical place to put them, and modified them to use the
same style as those existing tests. IMO, this test style makes the
task of writing tests simpler, since the expected output is a little
more obvious.

Similarly in the rowsecurity tests, I have moved the new tests to
immediately after the existing tests for RLS policies on tables
accessed via views, and added a few new tests in the same style,
including verifying permission checks on relations in subqueries in
RLS policies, when the table is accessed via a view.

I wasn't happy with the overall level of test coverage for this new
feature, so I have expanded on them quite a bit. This includes tests
for a bug in rewriteTargetView() -- it wasn't consistently handling
the case of an update involving an ordinary view on top of a security
invoker view.

I have added explicit documentation for the fact that a security
invoker view always does permission checks as the current user, even
if it is accessed from a non-security invoker view, since that was the
cause of some discussion on this thread.

I've also added some more detailed documentation describing how all
this affects RLS, since that's likely to be a common use case.

I've done a fairly extensive doc search, and I *think* I've identified
all the other places that needed updating.

One additional thing that had been missed was that the LOCK command
can be used to lock views, which includes locking all underlying base
relations, after checking permissions as the view owner. The
logical/consistent thing to do for security invoker views is to do the
permission checks as the invoking user, so I've done that.

Barring any other comments or objections, I'll push this in a couple
of days or so, after a bit more proof-reading.

Regards,
Dean

On Sat, 2022-03-19 at 01:10 +0000, Dean Rasheed wrote:
> I have been hacking on it a bit, and attached is an updated version.
> Aside from some general copy editing, the most notable changes are:
> [...]

Thanks for your diligent work on this, and the patch looks good to me.
It is good that you found the oversight in LOCK - I wasn't even
aware that views could be locked.

Yours,
Laurenz Albe

On Mon, 2022-03-21 at 18:09 +0800, Japin Li wrote:
> After apply the patch, I found pg_checksums.c also has the similar code.
> 
> In progress_report(), I'm not sure we can do this replace for this code.
> 
>     snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
>              total_size / (1024 * 1024));
>     snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
>              current_size / (1024 * 1024));
> 
>     fprintf(stderr, _("%*s/%s MB (%d%%) computed"),
>             (int) strlen(current_size_str), current_size_str, total_size_str,
>             percent);

I think you replied to the wrong thread...

Yours,
Laurenz Albe

On Mon, 21 Mar 2022 at 20:40, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> On Mon, 2022-03-21 at 18:09 +0800, Japin Li wrote:
>> After apply the patch, I found pg_checksums.c also has the similar code.
>>
>> In progress_report(), I'm not sure we can do this replace for this code.
>>
>>     snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
>>              total_size / (1024 * 1024));
>>     snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
>>              current_size / (1024 * 1024));
>>
>>     fprintf(stderr, _("%*s/%s MB (%d%%) computed"),
>>             (int) strlen(current_size_str), current_size_str, total_size_str,
>>             percent);
>
> I think you replied to the wrong thread...
>


I'm sorry!  There is a problem with my email client and I didn't notice the
subject of the reply email.

Again, sorry for the noise!

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, 21 Mar 2022 at 09:47, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> Thanks for your diligent work on this, and the patch looks good to me.

Thanks for looking again. Pushed.

Regards,
Dean