Remove the RLS policy and supporting role. (#23622)

* Remove the RLS policy on repo and sg_service role.

We encountered performance issues for our use cases when we deployed
RLS to production. We made the decision to back that approach out and
solve the security concerns in application-level code instead.

Role management via migrations is tricky business. They are server-wide
objects, and given our database templating approach for parallelism it
leads to the role having dependent objects in multiple databases that
outlive any one test run.

This leads to a situation where the migrations complete successfully,
but the role remains as an artifact because those other test databases
technically depend on it.

To avoid this, and since we're no longer using this strategy, the prior
migrations that referred to the policy + role have been commented away
with notes pointing to the migrations introduced here, which remove
everything in a safe and idempotent way.

Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>

CHANGELOG.md

@@ -45,6 +45,7 @@ All notable changes to Sourcegraph are documented in this file.
 
 - The old batch repository syncer was removed and can no longer be activated by setting `ENABLE_STREAMING_REPOS_SYNCER=false`. [#22949](https://github.com/sourcegraph/sourcegraph/pull/22949)
 - Email notifications for saved searches are now deprecated in favor of Code Monitoring. Email notifications can no longer be enabled for saved searches. Saved searches that already have notifications enabled will continue to work, but there is now a button users can click to migrate to code monitors. Notifications for saved searches will be removed entirely in the future. [#23275](https://github.com/sourcegraph/sourcegraph/pull/23275)
+- The `sg_service` Postgres role and `sg_repo_access_policy` policy on the `repo` table have been removed due to performance concerns. [#23622](https://github.com/sourcegraph/sourcegraph/pull/23622)
 
 ## 3.30.3
 

doc/admin/repo/row_level_security.md

@@ -1,5 +1,7 @@
 # Row-level security
 
+> NOTE: this document is deprecated, but preserved for historical and informational purposes.
+
 Starting with version 9.5, Postgres provides a [row-level security](https://www.postgresql.org/docs/13/ddl-rowsecurity.html) mechanism (abbreviated as "RLS") that can restrict table access in a granular, per-user fashion. Sourcegraph uses this mechanism to provide data isolation and protection guarantees beyond those supplied by application-level techniques. This document serves as a brief overview of the concept, its application at Sourcegraph and administrative implications.
 
 ## Basics of RLS

internal/database/schema.md

@@ -1475,17 +1475,6 @@ Referenced by:
     TABLE "lsif_retention_configuration" CONSTRAINT "lsif_retention_configuration_repository_id_fkey" FOREIGN KEY (repository_id) REFERENCES repo(id) ON DELETE CASCADE
     TABLE "search_context_repos" CONSTRAINT "search_context_repos_repo_id_fk" FOREIGN KEY (repo_id) REFERENCES repo(id) ON DELETE CASCADE
     TABLE "user_public_repos" CONSTRAINT "user_public_repos_repo_id_fkey" FOREIGN KEY (repo_id) REFERENCES repo(id) ON DELETE CASCADE
-Policies:
-    POLICY "sg_repo_access_policy"
-      TO sg_service
-      USING ((((NOT (current_setting('rls.use_permissions_user_mapping'::text))::boolean) AND ((private IS FALSE) OR (EXISTS ( SELECT
-   FROM (external_services es
-     JOIN external_service_repos esr ON (((esr.external_service_id = es.id) AND (esr.repo_id = repo.id) AND (es.unrestricted = true) AND (es.deleted_at IS NULL))))
- LIMIT 1)))) OR (EXISTS ( SELECT 1
-   FROM external_service_repos
-  WHERE ((external_service_repos.repo_id = repo.id) AND (external_service_repos.user_id = (current_setting('rls.user_id'::text))::integer)))) OR ( SELECT (user_permissions.object_ids_ints @> intset(repo.id))
-   FROM user_permissions
-  WHERE ((user_permissions.user_id = (current_setting('rls.user_id'::text))::integer) AND (user_permissions.permission = current_setting('rls.permission'::text)) AND (user_permissions.object_type = 'repos'::text)))))
 Triggers:
     trig_delete_repo_ref_on_external_service_repos AFTER UPDATE OF deleted_at ON repo FOR EACH ROW EXECUTE FUNCTION delete_repo_ref_on_external_service_repos()
 

migrations/frontend/1528395840_create_sg_service_role.down.sql

@@ -1,16 +1,11 @@
 BEGIN;
 
-DO $$
-BEGIN
-    REVOKE ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public FROM sg_service;
-    REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA public FROM sg_service;
-    REVOKE USAGE ON SCHEMA public FROM sg_service;
-    DROP ROLE IF EXISTS sg_service;
-EXCEPTION WHEN dependent_objects_still_exist THEN
-    -- Roles are cluster-wide, which makes them visible to both real and test
-    -- code. Since tests run in the same cluster as local development code, it
-    -- may not be possible to roll the change back.
-END;
-$$;
+-- We encountered performance issues for our use cases when we deployed
+-- RLS to production. We made the decision to back that approach out and
+-- solve the security concerns in application-level code instead.
+--
+-- ref migrations/frontend/1528395860_remove_repo_table_policy.up.sql
+-- ref migrations/frontend/1528395861_remove_sg_service_grants.up.sql
+-- ref migrations/frontend/1528395862_remove_sg_service_role.up.sql
 
 COMMIT;

migrations/frontend/1528395840_create_sg_service_role.up.sql

@@ -1,23 +1,11 @@
 BEGIN;
 
--- The "sg_service" role is one that the frontend and other services
--- will assume on startup/init. It lowers the privilege of those services
--- such that we can apply security policies to the role and let Postgres
--- manage things that previously would need to be done in app-level code.
-DO $$
-BEGIN
-    CREATE ROLE sg_service INHERIT;
-    GRANT USAGE ON SCHEMA public TO sg_service;
-    GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA public TO sg_service;
-    GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public TO sg_service;
-EXCEPTION 
-	WHEN duplicate_object THEN
-    -- Roles are cluster-wide, which makes them visible to both real and test
-    -- code. The test runners may effectively execute this code multiple times,
-    -- so if the role happens to exist, we just ignore it.
-	WHEN unique_violation THEN
-    -- Same as above.
-END;
-$$;
+-- We encountered performance issues for our use cases when we deployed
+-- RLS to production. We made the decision to back that approach out and
+-- solve the security concerns in application-level code instead.
+--
+-- ref migrations/frontend/1528395860_remove_repo_table_policy.up.sql
+-- ref migrations/frontend/1528395861_remove_sg_service_grants.up.sql
+-- ref migrations/frontend/1528395862_remove_sg_service_role.up.sql
 
 COMMIT;

migrations/frontend/1528395841_add_repo_table_policy.down.sql

@@ -1,8 +1,11 @@
 BEGIN;
 
--- With row-level security disabled, all rows in the repo table
--- will be accessible/visible to all roles.
-DROP POLICY IF EXISTS sg_repo_access_policy ON repo;
-ALTER TABLE repo DISABLE ROW LEVEL SECURITY;
+-- We encountered performance issues for our use cases when we deployed
+-- RLS to production. We made the decision to back that approach out and
+-- solve the security concerns in application-level code instead.
+--
+-- ref migrations/frontend/1528395860_remove_repo_table_policy.up.sql
+-- ref migrations/frontend/1528395861_remove_sg_service_grants.up.sql
+-- ref migrations/frontend/1528395862_remove_sg_service_role.up.sql
 
 COMMIT;

migrations/frontend/1528395841_add_repo_table_policy.up.sql

@@ -1,57 +1,11 @@
 BEGIN;
--- When row-level security is enabled, the table immediately "fails closed"
--- for all roles other than the table owner. The "sg_repo_access_policy" then
--- dictates the filtering mechanism used to decide what rows can be seen or
--- updated by the specified role(s).
+
+-- We encountered performance issues for our use cases when we deployed
+-- RLS to production. We made the decision to back that approach out and
+-- solve the security concerns in application-level code instead.
 --
--- The USING clause requires three LOCAL variables to be set:
---     1. rls.use_permissions_user_mapping: the switch to turn on permissions uesr mapping
---     2. rls.user_id: the effective ID for the user making the request
---     3. rls.permission: the permission that the user needs (e.g. "read")
-ALTER TABLE
-  repo ENABLE ROW LEVEL SECURITY;
-CREATE POLICY sg_repo_access_policy ON repo FOR ALL TO sg_service USING (
-  (
-    NOT current_setting(
-      'rls.use_permissions_user_mapping'
-    )::BOOLEAN -- Disregard unrestricted state when permissions user mapping is enabled
-    AND (
-      repo.private IS false -- Happy path of non-private repositories
-      OR EXISTS (
-        -- Each external service defines if repositories are unrestricted
-        SELECT
-        FROM
-          external_services AS es
-          JOIN external_service_repos AS esr ON (
-            esr.external_service_id = es.id
-            AND esr.repo_id = repo.id
-            AND es.unrestricted = TRUE
-            AND es.deleted_at IS NULL
-          )
-        LIMIT
-          1
-      )
-    )
-  ) OR EXISTS (
-    -- We assume that all repos added by the authenticated user should be shown
-    SELECT
-      1
-    FROM
-      external_service_repos
-    WHERE
-      repo_id = repo.id
-      AND user_id = current_setting('rls.user_id')::INTEGER
-  )
-  OR (
-    -- Restricted repositories require checking permissions
-    SELECT
-      object_ids_ints @> INTSET(repo.id)
-    FROM
-      user_permissions
-    WHERE
-      user_id = current_setting('rls.user_id')::INTEGER
-      AND permission = current_setting('rls.permission')::TEXT
-      AND object_type = 'repos'
-  )
-);
+-- ref migrations/frontend/1528395860_remove_repo_table_policy.up.sql
+-- ref migrations/frontend/1528395861_remove_sg_service_grants.up.sql
+-- ref migrations/frontend/1528395862_remove_sg_service_role.up.sql
+
 COMMIT;

migrations/frontend/1528395860_remove_repo_table_policy.down.sql

@@ -0,0 +1,7 @@
+BEGIN;
+
+-- We do not recreate the policy, as we've shifted our strategy away from row-
+-- level security to application-level code. Prior migrations that created the
+-- policy have also been removed.
+
+COMMIT;

migrations/frontend/1528395860_remove_repo_table_policy.up.sql

@@ -0,0 +1,8 @@
+BEGIN;
+
+-- This removes the row-level security policy (if present), and disables RLS on
+-- the repo table. Both operations are idempotent.
+DROP POLICY IF EXISTS sg_repo_access_policy ON repo;
+ALTER TABLE repo DISABLE ROW LEVEL SECURITY;
+
+COMMIT;

migrations/frontend/1528395861_remove_sg_service_grants.down.sql

@@ -0,0 +1,7 @@
+BEGIN;
+
+-- We do not recreate the grants, as we've shifted our strategy away from row-
+-- level security to application-level code. Prior migrations that created the
+-- grants have also been removed.
+
+COMMIT;

migrations/frontend/1528395861_remove_sg_service_grants.up.sql

@@ -0,0 +1,17 @@
+
+BEGIN;
+
+DO $$
+BEGIN
+    REVOKE ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public FROM sg_service;
+    REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA public FROM sg_service;
+    REVOKE USAGE ON SCHEMA public FROM sg_service;
+EXCEPTION WHEN undefined_object THEN
+    -- Roles are visible across databases within a server, and we use templated
+    -- databases for test parallelism, so it's possible in some cases for the
+    -- tests to hit a case where the role can't be dropped because one of the
+    -- test databases still has objects that depend on it.
+END;
+$$;
+
+COMMIT;

migrations/frontend/1528395862_remove_sg_service_role.down.sql

@@ -0,0 +1,7 @@
+BEGIN;
+
+-- We do not recreate the role, as we've shifted our strategy away from row-
+-- level security to application-level code. Prior migrations that created the
+-- role have also been removed.
+
+COMMIT;

migrations/frontend/1528395862_remove_sg_service_role.up.sql

@@ -0,0 +1,14 @@
+BEGIN;
+
+DO $$
+BEGIN
+    DROP ROLE IF EXISTS sg_service;
+EXCEPTION WHEN dependent_objects_still_exist THEN
+    -- Roles are visible across databases within a server, and we use templated
+    -- databases for test parallelism, so it's possible in some cases for the
+    -- tests to hit a case where the role can't be dropped because one of the
+    -- test databases still has objects that depend on it.
+END;
+$$;
+
+COMMIT;