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

On Fri, 2022-02-04 at 22:28 +0100, walther(at)technowledgy(dot)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