Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Make the api_owner role first class #400

Open
steve-chavez opened this issue May 10, 2021 · 14 comments
Open

Make the api_owner role first class #400

steve-chavez opened this issue May 10, 2021 · 14 comments
Labels
explanations background and context about how the project works.

Comments

@steve-chavez
Copy link
Member

To make views work as expected for RLS, we recommend creating an api_owner role on:
https://postgrest.org/en/stable/schema_structure.html#views

But this role is thought as a workaround, that will be avoided once views support something similar to SECURITY INVOKER.

If views were to support that, then the INVOKER role would need privileges on the source tables of the views, i.e. the anon role would not only need privileges on the api view, but also on the private table. That would be incorrect, as the api roles should only need privileges on the exposed schemas.

So I think the api_owner role is here to stay. It should be as integral as our auth roles.

@steve-chavez steve-chavez added the explanations background and context about how the project works. label May 10, 2021
@steve-chavez
Copy link
Member Author

Also, as it is on the docs now:

CREATE ROLE api_views_owner NOINHERIT;
ALTER VIEW sample_view OWNER TO api_views_owner;

The api_owner privileges are lacking, it would also need grants on the underlying tables(and this is not mentioned).

Perhaps we could even make this role the target of RLS policies that are meant to work for views.

@wolfgangwalther
Copy link
Member

I agree with it in general.

If views were to support that, then the INVOKER role would need privileges on the source tables of the views, i.e. the anon role would not only need privileges on the api view, but also on the private table. That would be incorrect, as the api roles should only need privileges on the exposed schemas.

But not with this. Assume SECURITY INVOKER on views was a thing, then of course I'd need privileges on the private tables for my auth roles. But that's not wrong or incorrect. That's just a logical consequence, because the access control layer is moved to the table level in this case.

In theory, there are 2 "pure" ways to do access control, imho - both of which are currently not possible with postgres:

  • Only at the table level - this needs SECURITY INVOKER
  • Only at the view level - this would need the ability to add RLS directly on a view

You can do the view-level-access-control with security_barrier views right now - but those are not nearly as powerful as RLS is.

So looking at those, the SECURITY DEFINER thing we have right now, is really a strange mix. This mixes view-level relation privileges with table-level row privileges.

@wolfgangwalther
Copy link
Member

Perhaps we could even make this role the target of RLS policies that are meant to work for views.

Yes, I think that makes a lot of sense - especially in the shared-role case, where those policies will depend on some of our GUCs. Whenever that's the case, the policies should really only affect access through PostgREST - they won't work through psql anyway. Restricting those to the view owner seems to be exactly the right thing to do.

@steve-chavez
Copy link
Member Author

steve-chavez commented May 13, 2021

Assume SECURITY INVOKER on views was a thing, then of course I'd need privileges on the private tables for my auth roles. But that's not wrong or incorrect.

  • Only at the view level - this would need the ability to add RLS directly on a view

Yes, agree. The whole thing about RLS on views, I think the solution shouldn't be to enable SECURITY INVOKER on them. but to add the conditions(implicit WHEREs) to their rules. Not sure if it's possible in pg, but that is what most users would expect, as that wouldn't change the privilege model.

In theory, there are 2 "pure" ways to do access control

That reminds me that there's a 3rd way - only at the function level. There was a related discussion here(supabase repo), and it makes sense as a more strict model. More simple in terms of privileges, but probably more work if you have many tables you want to expose.

Perhaps that stricter model could be mentioned on schema isolation. Some users might want an only RPC API(reddit discussion), and seems we're not mentioning our RPC capabilities enough(also the landing page should mention it somewhere).

Edit: new request for this explanation on stackoverflow.

@steve-chavez
Copy link
Member Author

Really liked the analogy of views being security definer by default on PostgREST/postgrest#2087 (reply in thread). We should add that when doing the api_owner docs.

@wolfgangwalther
Copy link
Member

Really liked the analogy of views being security definer by default on PostgREST/postgrest#2087 (reply in thread). We should add that when doing the api_owner docs.

We really need to be careful to word this correctly. I noticed some quirks:

  • An auto-updateable view will pretty much behave as expected: The view owner's privileges will be used "downstream", but CURRENT_USER will still be set to the api user.
  • Views with INSTEAD OF triggers, will run those functions depending on the SECURITY INVOKER / SECURITY DEFINER setting of that function. However, if I remember correctly, in this case it's not possible to replicate the same behaviour as in SELECT. INVOKER will keep CURRENT_USER, but obviously access the tables with that user's privileges too. DEFINER however will change both.

So views are not really DEFINER after all. It's a strange mix. Really not useful at all....

I ended up needing to give a strange mix of permissions to the base tables to both my view owner and my api users...

@steve-chavez
Copy link
Member Author

Hm, lots of gotchas 😕

So updatable views are cool, queries and mutations will work, only one db object.

But non-updatable views require the extra work(INSTEAD OF triggers plus the privilege gotchas) for mutations to work, so more than one db object, one function/trigger per operation IINM.

Perhaps we should recommend exposing functions instead of non-updatable views? Their behavior is more consistent and the db objects required for enabling full CRUD should be the same.

@wolfgangwalther
Copy link
Member

Perhaps we should recommend exposing functions instead of non-updatable views? Their behavior is more consistent and the db objects required for enabling full CRUD should be the same.

Ah, no. I don't think that should be our recommendation. I think we should find a way to make working with views easier.

I just tested a bit more.. and we can use DO INSTEAD rules (which are what auto-updatable views use under the hood). Those will work the same as auto-updatable views.

Here's an example using all three methods - and highlighting the problems with those:

create role alice;
create role bob;
create role charly;
grant create on database postgres to alice, bob;


set role alice;
create schema a;
grant usage on schema a to bob;

create table a.t (c text);
alter table a.t enable row level security;
grant all privileges on table a.t to bob;

create policy current_user_only on a.t
for all to public
using (c = current_user)
with check (c = current_user);



set role bob;
create schema b;
grant usage on schema b to charly;

-- auto-updateable view
create view b.v_auto as select * from a.t;
grant all privileges on table b.v_auto to charly;

-- view with instead of insert trigger (security invoker)
create view b.v_trigger as select * from a.t;
grant all privileges on table b.v_trigger to charly;

create function b.insert_t () returns trigger
language plpgsql as $$
begin
  insert into a.t values (new.*);
  return new;
end
$$;
create trigger ins instead of insert on b.v_trigger
for each row execute function b.insert_t();

-- view with instead of insert trigger (security definer)
create view b.v_trigger_def as select * from a.t;
grant all privileges on table b.v_trigger_def to charly;

create function b.insert_t_def () returns trigger
security definer
language plpgsql as $$
begin
  insert into a.t values (new.*);
  return new;
end
$$;
create trigger ins instead of insert on b.v_trigger_def
for each row execute function b.insert_t_def();

-- view with rules
create or replace view b.v_rule as select * from a.t;
grant all privileges on table b.v_rule to charly;

create rule ins as on insert to b.v_rule
do instead insert into a.t values (new.*);



set role charly;
-- ok
insert into b.v_auto values ('charly');

-- permission denied for schema a
insert into b.v_trigger values ('charly');

-- violates row-level security
insert into b.v_trigger_def values ('charly'); 

-- works... but is not what we want. charly should not insert bob.
insert into b.v_trigger_def values ('bob');

-- ok
insert into b.v_rule values ('charly');

Rules are also one db object less than trigger+trigger function ;)


Of course, working with INSTEAD OF triggers is still possible. The trigger function needs to be SECURITY INVOKER (the default) and we'll need to grant privileges for the api user to both the view schema and the data schema.


Maybe we should try to make another push for PostgreSQL to allow SECURITY INVOKER views. Those would at least make the different cases consistent and move all privilege handling to the data schema.

@wolfgangwalther
Copy link
Member

Maybe we should try to make another push for PostgreSQL to allow SECURITY INVOKER views. Those would at least make the different cases consistent and move all privilege handling to the data schema.

In fact, this has happened in December already:

https://www.postgresql.org/message-id/flat/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at

I'll try to build this patch for myself and try to test and review it to push it further. Hopefully this "nightmare" of how to use views with RLS properly is over soon.

@wolfgangwalther
Copy link
Member

I just tested a bit more.. and we can use DO INSTEAD rules (which are what auto-updatable views use under the hood). Those will work the same as auto-updatable views.

One thing I noticed is that we can't use INSERT ... ON CONFLICT with views all the time:

  • with instead of triggers it didn't work for me (would have to set up a case replicating this, it's been a while)
  • with rules I get: INSERT with ON CONFLICT clause cannot be used with table that has INSERT or UPDATE rules
  • I remember it only worked with auto-updateable views

@steve-chavez
Copy link
Member Author

INSERT with ON CONFLICT clause cannot be used with table that has INSERT or UPDATE rules

I remember a similar limitation for rules:

@wolfgangwalther
Copy link
Member

https://www.postgresql.org/message-id/flat/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at

I'll try to build this patch for myself and try to test and review it to push it further. Hopefully this "nightmare" of how to use views with RLS properly is over soon.

I gave the patch a test and it works.. kind of. I gave it a review, but I am not sure whether the underlying problem of the current implementation of views might stop that patch in the end... we'll see.

@wolfgangwalther
Copy link
Member

The patch has been committed in postgres/postgres@7faa5fc and will be available in PG15.

I backported the patch to PG14 and built a custom docker image (alpine based) with it:

Using security invoker views now... :)

@steve-chavez
Copy link
Member Author

Using security invoker views now... :)

🚀 Awesome! Not too much code required on PostgreSQL to make that work as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
explanations background and context about how the project works.
Development

No branches or pull requests

2 participants