fix: add row level security over shared tables

[AndrewJackson2020]

Currently any user that has any ability to read the pg_net request or queue tables also has the ability to read every other users data on these tables. This is particularly problematic if users are using secrets (passwords, API keys, etc) in a multi tenant database environment.

@steve-chavez would appreciate any feedback on this PR in terms of the historical goals/design decisions of pg_net.

[steve-chavez]

@AndrewJackson2020 I wonder if this is the right design 🤔 :

1. If we switch to callbacks (Change table queue to in-memory queue and add callbacks #62) then the table can be controlled by the user. Side-stepping the need for RLS.
2. Another idea would be allowing the user to create the net tables on a custom schema. This would also avoid RLS.

1 would be a major redesign, 2 would keep it but seems more messy than yours.

Since we're going to do a breaking change soon, perhaps we should try 1 anyways? WDYT?

[AndrewJackson2020]

I wonder if this is the right design 🤔

maybe, maybe not. hoping we can figure this out. I will say that this design is consistent with pg_cron so there is prior art to draw from. The only thing analogous in postgres contrib is probably pg_stat_statements which uses a hybrid shared memory data structure/on disk flat file. the pg_stat_statements approach is a lot more complex than an RLS table.

If we switch to callbacks

This is an interesting idea. It's a lot more true to the interface of other asynchronous frameworks. I also feel like it is harder for users to reason about. It also feels like much higher touch than this PR. I don't think I have a good opinion on this right now.

Another idea would be allowing the user to create the net tables on a custom schema

This also feels a bit higher touch than the proposed solution. Interesting idea though. What would the interface look like? You would just provide the schema/tablename of the intended request/response tables to the function calls? The user would just need to make sure that the table is set up with the correct schema and update it accordingly if we ever decide to add/remove a column, change a data type, etc? I think it would allow more granular ability to grant access though. I can definitely see the row level security change breaking applications where one user executes a requests and lets another user pick up the response and do some processing on it or whatever.

[steve-chavez]

I will say that this design is consistent with pg_cron so there is prior art to draw from

TIL about that https://github.com/citusdata/pg_cron/blob/main/pg_cron.sql#L29. That does give more confidence in extensions working fine with RLS.

It also feels like much higher touch than this PR

Agree.

The user would just need to make sure that the table is set up with the correct schema and update it accordingly if we ever decide to add/remove a column, change a data type, etc?

Right, looks more complex and prone to failure.

I can definitely see the row level security change breaking applications where one user executes a requests and lets another user pick up the response and do some processing on it or whatever.

Seems acceptable to do on next major release.

[AndrewJackson2020]

I went ahead and refactored this in a way that I think will solve a lot of the security issues that people have with pg_net without breaking changes.

The idea now is that instead of a username column we have a role name column which RLS is based off of. By default this column will get 'public' inserted into it. If the user wants to restrict who sees their rows they can insert their own username or a role. RLS will allow all users to access 'public' rows but will restrict based on identity/group membership if something other than public is in there. Eventually we may want to do a breaking change to force people to select who they want to see their stuff but I think we can deploy this first without breaking change and decide on that later.

@utkarash2991 @steve-chavez lmk what you guys think.

[steve-chavez]

By default this column will get 'public' inserted into it. If the user wants to restrict who sees their rows they can insert their own username or a role.

Looks good to not cause a breaking change 👍

[utkarash2991]

@AndrewJackson2020 just adding here for completeness. We need to make required changes here too : https://github.com/supabase/postgres/blob/develop/ansible/files/admin_api_scripts/pg_upgrade_scripts/complete.sh#L157 to handle the migration scripts correctly

[utkarash2991]

Also correct me if I am wrong, dont we need any upgrade script ? sql/pg_net--X--Y.sql change ?

[AndrewJackson2020]

Those changes def need to be made and would not merge without that.

[utkarash2991]

nit: remove the comment . I think it got pasted from above.

[utkarash2991]

@AndrewJackson2020 change looks good to me . I have added some comments. Let me know what you think about it ? Approving the change to unblock you.

[AndrewJackson2020]

@AndrewJackson2020 change looks good to me . I have added some comments. Let me know what you think about it ? Approving the change to unblock you.

Thanks, agree with all of the above. If your sure that this is truly a non breaking change as I am then I'm good. Also was hoping to merge this one first to eliminate the possibility of spit brain in migrations as you pointed out above but it still has issues I need to fix.

#304