[PATCH] Add reloption for views to enable RLS

On Fri, 2021-12-17 at 18:31 +0100, 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 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.

This has been requested before, see for example
https://stackoverflow.com/q/33858030/6464308

Row Level Security is only one use case; there may be other situations
when it is useful to check permissions on the underlying objects with
the current user rather than with the view owner.

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

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.

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

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 64d9030652..760ea2f794 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2292,6 +2292,10 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
    are not subject to row security.
   </para>

+  <para>
+   For views, the policies are applied as being referenced through the view owner by default, rather than the user referencing the view. To apply row security policies as defined for the invoking
user, the <firstterm>security</firstterm> option can be set on views (see <link linkend="sql-createview">CREATE VIEW</link>) to get the same behavior.
+  </para>
+
   <para>
    Row security policies can be specific to commands, or to roles, or to
    both.  A policy can be specified to apply to <literal>ALL</literal>

Please avoid long lines like that. 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).

Yours,
Laurenz Albe

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

On Tue, 2022-01-18 at 16:16 +0100, Christoph Heiss wrote:

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.

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.

I gave the new patch a spin, and got a surprising result:

CREATE TABLE tab (id integer);

CREATE ROLE duff LOGIN;

CREATE ROLE jock LOGIN;

GRANT INSERT, UPDATE, DELETE ON tab TO jock;

GRANT SELECT ON tab TO duff;

CREATE VIEW v WITH (security_invoker = TRUE) AS SELECT * FROM tab;

ALTER VIEW v OWNER TO jock;

GRANT SELECT, INSERT, UPDATE, DELETE ON v TO duff;

SET SESSION AUTHORIZATION duff;

SELECT * FROM v;
id
════
(0 rows)

That's ok, "duff" has permissions to read "tab".

INSERT INTO v VALUES (1);
INSERT 0 1

Huh? "duff" has no permission to insert into "tab"!

RESET SESSION AUTHORIZATION;

ALTER VIEW v SET (security_invoker = FALSE);

SET SESSION AUTHORIZATION duff;

SELECT * FROM v;
ERROR: permission denied for table tab

As expected.

INSERT INTO v VALUES (1);
INSERT 0 1

As expected.

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.

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

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

Yours,
Laurenz Albe

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

On Wed, 2022-02-02 at 18:23 +0100, Christoph Heiss wrote:

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.

I tested it, and the patch works fine now.

Some little comments:

--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -3242,9 +3243,13 @@ rewriteTargetView(Query *parsetree, Relation view)
0);

/*
-    * Mark the new target RTE for the permissions checks that we want to
-    * enforce against the view owner, as distinct from the query caller.  At
-    * the relation level, require the same INSERT/UPDATE/DELETE permissions
+    * If the view has security_invoker set, mark the new target RTE for the
+    * permissions checks that we want to enforce against the query caller, as
+    * distince from the view owner.

Typo: distince

diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 509e930fc7..fea893569f 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -261,15 +261,26 @@ CREATE VIEW mysecview3 WITH (security_barrier=false)
        AS SELECT * FROM tbl1 WHERE a < 0;
 CREATE VIEW mysecview4 WITH (security_barrier)
        AS SELECT * FROM tbl1 WHERE a <> 0;
-CREATE VIEW mysecview5 WITH (security_barrier=100) -- Error
+CREATE VIEW mysecview5 WITH (security_invoker=true)
+       AS SELECT * FROM tbl1 WHERE a = 100;
+CREATE VIEW mysecview6 WITH (security_invoker=false)
        AS SELECT * FROM tbl1 WHERE a > 100;
+CREATE VIEW mysecview7 WITH (security_invoker)
+       AS SELECT * FROM tbl1 WHERE a < 100;
+CREATE VIEW mysecview8 WITH (security_barrier=100) -- Error
+       AS SELECT * FROM tbl1 WHERE a <> 100;
 ERROR:  invalid value for boolean option "security_barrier": 100
-CREATE VIEW mysecview6 WITH (invalid_option)       -- Error
+CREATE VIEW mysecview9 WITH (security_invoker=100) -- Error
+       AS SELECT * FROM tbl1 WHERE a = 100;
+ERROR:  invalid value for boolean option "security_invoker": 100
+CREATE VIEW mysecview10 WITH (invalid_option)      -- Error

I see no reasons to remove two of the existing tests.

+++ b/src/test/regress/expected/rowsecurity.out
@@ -8,9 +8,11 @@ DROP USER IF EXISTS regress_rls_alice;
 DROP USER IF EXISTS regress_rls_bob;
 DROP USER IF EXISTS regress_rls_carol;
 DROP USER IF EXISTS regress_rls_dave;
+DROP USER IF EXISTS regress_rls_grace;

But the name has to start with "e"!

I also see no reason to split a small patch like this into three parts.

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

Yours,
Laurenz Albe

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

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

On Fri, 2022-02-04 at 22:28 +0100, walther@technowledgy.de wrote:

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.

Thanks for testing and weighing in!

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?

This behavior is not new:

CREATE SCHEMA viewtest;

CREATE ROLE duff LOGIN;
CREATE ROLE jock LOGIN;

CREATE TABLE viewtest.tab (id integer);
GRANT SELECT ON viewtest.tab TO duff;

CREATE VIEW v AS SELECT * FROM viewtest.tab;
ALTER VIEW v OWNER TO duff;
GRANT SELECT ON v TO jock;

SET ROLE jock;

SELECT * FROM v;
id
════
(0 rows)

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?

If not, I don't know if it is the business of this patch to
change the behavior.

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.

Right. Even though permissions on "v1" are checked for user "bob",
permissions on the table are checked for the current user, which remains
"alice".

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

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.

I agree that changing the current behavior is not acceptable.

I guess more documentation how this works would be a good idea.
Not sure if this is the job of this patch, but since it exposes this
in new ways, it might as well clarify how all this works.

Yours,
Laurenz Albe

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

On Tue, 2022-02-15 at 13:02 +0100, Christoph Heiss wrote:

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

About the documentation:

--- a/doc/src/sgml/ref/alter_view.sgml
+++ b/doc/src/sgml/ref/alter_view.sgml
@@ -156,11 +156,21 @@ ALTER VIEW [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RESET
         <listitem>
          <para>
           Changes the security-barrier property of the view.  The value must
-          be Boolean value, such as <literal>true</literal>
+          be a Boolean value, such as <literal>true</literal>
           or <literal>false</literal>.
          </para>
         </listitem>
        </varlistentry>
+       <varlistentry>
+        <term><literal>run_as_owner</literal> (<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Changes the user as which the subquery is run. Default is
+          <literal>true</literal>.  The value must be a Boolean value, such as
+          <literal>true</literal> or <literal>false</literal>.
+         </para>
+        </listitem>
+       </varlistentry>

Correct would be

If set to <literal>true</literal> (which is the default value), permissions
on the underlying relations are checked as view owner, otherwise as the user
executing the query.

(I used "relation" to express that it doesn't hold for functions.)

--- a/doc/src/sgml/ref/create_view.sgml
+++ b/doc/src/sgml/ref/create_view.sgml
@@ -265,13 +278,39 @@ CREATE VIEW vista AS SELECT text 'Hello World' AS hello;
    </para>

    <para>
-    Access to tables referenced in the view is determined by permissions of
-    the view owner.  In some cases, this can be used to provide secure but
-    restricted access to the underlying tables.  However, not all views are
-    secure against tampering; see <xref linkend="rules-privileges"/> for
-    details.  Functions called in the view are treated the same as if they had
-    been called directly from the query using the view.  Therefore the user of
-    a view must have permissions to call all functions used by the view.
+    By default, access to tables and functions referenced in the view is
+    determined by permissions of the view owner.

No, access to the functions is checked for the caller.

+ [...] Therefore the user of a view must have permissions

Comma after "therefore".

+    to call all functions used by the view.  This also means that functions
+    are executed as the invoking user, not the view owner.
+   </para>
+
+   <para>
+    However, when using chained views, the <literal>CURRENT_USER</literal> user
+    will always stay the invoking user,

"However" would introduce something that is different from what came before,
which this doesn't seem to be.

Perhaps "In particular" or "moreover".

+                                        regardless of whether the query is run
+    as the view owner (the default) or the invoking user (when
+    <literal>run_as_owner</literal> is set to <literal>false</literal>)
+    and the depth of the current invocation.
+   </para>

The query is *always* run as the invoking user. Better:

regardless of whether relation permissions are checked as the view owner or ...

+   <para>
+    Be aware that <literal>USAGE</literal> privileges on schemas are not checked
+    when referencing the underlying base relations, even if they are part of a
+    different schema.
    </para>

"referencing" is a bit unclear.
Perhaps "when checking permissions on the underlying base relations".

Otherwise, this looks good!

Yours,
Laurenz Albe

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

On Tue, 2022-02-15 at 16:07 +0100, walther@technowledgy.de wrote:

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.

I don't think that "local" will make this clearer.
I'd be happy with "check_as_owner", except it is unclear *what* is checked.
"check_permissions_as_owner" is ok with me, but a bit long.

How about "check_permissions_owner"?

Yours,
Laurenz

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 Tue, 2022-02-15 at 16:32 +0100, walther@technowledgy.de wrote:

"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

Here is a new version, with improved documentation and the option renamed
to "check_permissions_owner". I just prefer the shorter form.

Yours,
Laurenz Albe