firebase · tammam-g · Aug 29, 2024 · Aug 21, 2024 · Aug 21, 2024 · Aug 21, 2024
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -0,0 +1,2 @@
+- Improve Firebase Data Connect postgres security by granting fine grained SQL privileges to the users the need it. (#7578)
+- Remove `dataconnect:sql:migrate` command hard dependency on 'roles/cloudsql.admin'. (#7578)
diff --git a/src/api.ts b/src/api.ts
@@ -151,6 +151,11 @@ export const githubClientSecret = () =>
 
 export const dataconnectOrigin = () =>
   utils.envOverride("FIREBASE_DATACONNECT_URL", "https://firebasedataconnect.googleapis.com");
+export const dataconnectP4SADomain = () =>
+  utils.envOverride(
+    "FIREBASE_DATACONNECT_P4SA_DOMAIN",
+    "gcp-sa-firebasedataconnect.iam.gserviceaccount.com",
+  );
 export const dataConnectLocalConnString = () =>
   utils.envOverride("FIREBASE_DATACONNECT_POSTGRESQL_STRING", "");
 export const cloudSQLAdminOrigin = () =>

diff --git a/src/commands/dataconnect-sql-migrate.ts b/src/commands/dataconnect-sql-migrate.ts
@@ -16,7 +16,6 @@ export const command = new Command("dataconnect:sql:migrate [serviceId]")
     "firebasedataconnect.schemas.list",
     "firebasedataconnect.schemas.update",
     "cloudsql.instances.connect",
-    "cloudsql.users.create",
   ])
   .before(requireAuth)
   .withForce("Execute any required database changes without prompting")

diff --git a/src/dataconnect/schemaMigration.ts b/src/dataconnect/schemaMigration.ts
@@ -3,13 +3,22 @@ import { format } from "sql-formatter";
 
 import { IncompatibleSqlSchemaError, Diff, SCHEMA_ID } from "./types";
 import { getSchema, upsertSchema, deleteConnector } from "./client";
-import { execute, firebaseowner, setupIAMUser } from "../gcp/cloudsql/connect";
+import {
+  setupIAMUsers,
+  getIAMUser,
+  executeSqlCmdsAsIamUser,
+  executeSqlCmdsAsSuperUser,
+} from "../gcp/cloudsql/connect";
+import {
+  firebaseowner,
+  iamUserIsCSQLAdmin,
+  checkSQLRoleIsGranted,
+} from "../gcp/cloudsql/permissions";
 import { promptOnce, confirm } from "../prompt";
 import { logger } from "../logger";
 import { Schema } from "./types";
 import { Options } from "../options";
 import { FirebaseError } from "../error";
-import { needProjectId } from "../projectUtils";
 import { logLabeledBullet, logLabeledWarning, logLabeledSuccess } from "../utils";
 import * as experiments from "../experiments";
 import * as errors from "./errors";
@@ -186,8 +195,6 @@ async function handleIncompatibleSchemaError(args: {
       "This schema migration includes potentially destructive changes. If you'd like to execute it anyway, rerun this command with --force",
     );
   }
-  const projectId = needProjectId(options);
-  const iamUser = await setupIAMUser(instanceId, databaseId, options);
 
   const commandsToExecute = incompatibleSchemaError.diffs
     .filter((d) => {
@@ -202,20 +209,65 @@ async function handleIncompatibleSchemaError(args: {
     })
     .map((d) => d.sql);
   if (commandsToExecute.length) {
-    await execute(
-      [
-        `SET ROLE "${firebaseowner(databaseId)}"`,
-        ...commandsToExecute,
-        `GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA "public" TO PUBLIC`,
-      ],
-      {
-        projectId,
+    const commandsToExecuteBySuperUser = commandsToExecute.filter(
+      (sql) => sql.startsWith("CREATE EXTENSION") || sql.startsWith("CREATE SCHEMA"),
+    );
+    const commandsToExecuteByOwner = commandsToExecute.filter(
+      (sql) => !commandsToExecuteBySuperUser.includes(sql),
+    );
+
+    const userIsCSQLAdmin = await iamUserIsCSQLAdmin(options);
+
+    if (!userIsCSQLAdmin && commandsToExecuteBySuperUser.length) {
+      throw new FirebaseError(`Some SQL commands required for this migration require Admin permissions.\n 
+        Please ask a user with 'roles/cloudsql.admin' to apply the following commands.\n
+        ${commandsToExecuteBySuperUser.join("\n")}`);
+    }
+
+    // TODO (tammam-g): at some point we would want to only run this after notifying the admin but
+    // until we confirm stability it's ok to run it on every migration by admin user.
+    if (userIsCSQLAdmin) {
+      await setupIAMUsers(instanceId, databaseId, options);
+    }
+
+    // Test if iam user has access to the roles required for this migration
+    if (
+      !(await checkSQLRoleIsGranted(
+        options,
         instanceId,
         databaseId,
-        username: iamUser,
-      },
-    );
-    return incompatibleSchemaError.diffs;
+        firebaseowner(databaseId),
+        (await getIAMUser(options)).user,
+      ))
+    ) {
+      throw new FirebaseError(
+        `Command aborted. Only users granted firebaseowner SQL role can run migrations.`,
+      );
+    }
+
+    if (commandsToExecuteBySuperUser.length) {
+      logger.info(
+        `The diffs require CloudSQL superuser permissions, attempting to apply changes as superuser.`,
+      );
+      await executeSqlCmdsAsSuperUser(
+        options,
+        instanceId,
+        databaseId,
+        commandsToExecuteBySuperUser,
+        /** silent=*/ false,
+      );
+    }
+
+    if (commandsToExecuteByOwner.length) {
+      await executeSqlCmdsAsIamUser(
+        options,
+        instanceId,
+        databaseId,
+        [`SET ROLE "${firebaseowner(databaseId)}"`, ...commandsToExecuteByOwner],
+        /** silent=*/ false,
+      );
+      return incompatibleSchemaError.diffs;
+    }
   }
   return [];
 }

diff --git a/src/gcp/cloudsql/connect.ts b/src/gcp/cloudsql/connect.ts
@@ -2,14 +2,16 @@ import * as pg from "pg";
 import { Connector, IpAddressTypes, AuthTypes } from "@google-cloud/cloud-sql-connector";
 
 import { requireAuth } from "../../requireAuth";
-import { needProjectId } from "../../projectUtils";
+import { needProjectId, needProjectNumber } from "../../projectUtils";
+import { dataconnectP4SADomain } from "../../api";
 import * as cloudSqlAdminClient from "./cloudsqladmin";
 import { UserType } from "./types";
 import * as utils from "../../utils";
 import { logger } from "../../logger";
 import { FirebaseError } from "../../error";
 import { Options } from "../../options";
 import { FBToolsAuthClient } from "./fbToolsAuthClient";
+import { setupSQLPermissions, firebaseowner, firebasewriter } from "./permissions";
 
 export async function execute(
   sqlStatements: string[],
@@ -105,82 +107,119 @@ export async function execute(
   connector.close();
 }
 
-// setupIAMUser sets up the current user identity to connect to CloudSQL.
-// Steps:
-// 2. Create an IAM user for the current identity
-// 3. Connect to the DB as the temporary user and run the necessary grants
-// 4. Deletes the temporary user
-export async function setupIAMUser(
+export async function executeSqlCmdsAsIamUser(
+  options: Options,
   instanceId: string,
   databaseId: string,
+  cmds: string[],
+  silent = false,
+): Promise<void> {
+  const projectId = needProjectId(options);
+  const { user: iamUser } = await getIAMUser(options);
+
+  return await execute(cmds, {
+    projectId,
+    instanceId,
+    databaseId,
+    username: iamUser,
+    silent: silent,
+  });
+}
+
+// Note this will change the password of the builtin firebasesuperuser user on every invocation.
+// The role is set to 'cloudsqlsuperuser' (not the builtin user) unless SET ROLE is explicitly
+// set in the commands.
+export async function executeSqlCmdsAsSuperUser(
   options: Options,
-): Promise<string> {
-  // TODO: Is there a good way to short circuit this by checking if the IAM user exists and has the appropriate role first?
+  instanceId: string,
+  databaseId: string,
+  cmds: string[],
+  silent = false,
+) {
   const projectId = needProjectId(options);
-  // 0. Get the current identity
-  const account = await requireAuth(options);
-  if (!account) {
-    throw new FirebaseError(
-      "No account to set up! Run `firebase login` or set Application Default Credentials",
-    );
-  }
   // 1. Create a temporary builtin user
-  const setupUser = "firebasesuperuser";
+  const superuser = "firebasesuperuser";
   const temporaryPassword = utils.generateId(20);
   await cloudSqlAdminClient.createUser(
     projectId,
     instanceId,
     "BUILT_IN",
-    setupUser,
+    superuser,
     temporaryPassword,
   );
 
-  // 2. Create an IAM user for the current identity
-  const { user, mode } = toDatabaseUser(account);
-  await cloudSqlAdminClient.createUser(projectId, instanceId, mode, user);
-
-  // 3. Connect to the DB as the temporary user and run the necessary grants
-  // TODO: I think we're missing something here, sometimes backend can't see the tables.
-  const grants = [
-    `do
-      $$
-      begin
-        if not exists (select FROM pg_catalog.pg_roles
-          WHERE  rolname = '${firebaseowner(databaseId)}') then
-          CREATE ROLE "${firebaseowner(databaseId)}" WITH ADMIN "${setupUser}";
-        end if;
-      end
-      $$
-      ;`,
-    `GRANT ALL PRIVILEGES ON DATABASE "${databaseId}" TO "${firebaseowner(databaseId)}"`,
-    `GRANT cloudsqlsuperuser TO "${firebaseowner(databaseId)}"`,
-    `GRANT "${firebaseowner(databaseId)}" TO "${setupUser}"`,
-    `GRANT "${firebaseowner(databaseId)}" TO "${user}"`,
-    `ALTER SCHEMA public OWNER TO "${firebaseowner(databaseId)}"`,
-    `GRANT USAGE ON SCHEMA "public" TO PUBLIC`,
-    `GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA "public" TO PUBLIC`,
-    `GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA "public" TO PUBLIC`,
-  ];
-  await execute(grants, {
+  return await execute([`SET ROLE = cloudsqlsuperuser`, ...cmds], {
     projectId,
     instanceId,
     databaseId,
-    username: setupUser,
+    username: superuser,
     password: temporaryPassword,
-    silent: true,
+    silent: silent,
   });
-  return user;
 }
 
-export function firebaseowner(databaseId: string) {
-  return `firebaseowner_${databaseId}_public`;
+export function getDataConnectP4SA(projectNumber: string): string {
+  return `service-${projectNumber}@${dataconnectP4SADomain()}`;
+}
+
+export async function getIAMUser(options: Options): Promise<{ user: string; mode: UserType }> {
+  const account = await requireAuth(options);
+  if (!account) {
+    throw new FirebaseError(
+      "No account to set up! Run `firebase login` or set Application Default Credentials",
+    );
+  }
+
+  return toDatabaseUser(account);
+}
+
+// setupIAMUsers sets up the current user identity to connect to CloudSQL.
+// Steps:
+// 1. Create an IAM user for the current identity
+// 2. Create an IAM user for FDC P4SA
+// 3. Run setupSQLPermissions to setup the SQL database roles and permissions.
+// 4. Connect to the DB as the temporary user and run the necessary grants
+export async function setupIAMUsers(
+  instanceId: string,
+  databaseId: string,
+  options: Options,
+): Promise<string> {
+  // TODO: Is there a good way to short circuit this by checking if the IAM user exists and has the appropriate role first?
+  const projectId = needProjectId(options);
+
+  // 0. Get the current identity
+  const { user, mode } = await getIAMUser(options);
+
+  // 1. Create an IAM user for the current identity.
+  await cloudSqlAdminClient.createUser(projectId, instanceId, mode, user);
+
+  // 2. Create dataconnenct P4SA user in case it's not created.
+  const projectNumber = await needProjectNumber(options);
+  const { user: fdcP4SAUser, mode: fdcP4SAmode } = toDatabaseUser(
+    getDataConnectP4SA(projectNumber),
+  );
+  await cloudSqlAdminClient.createUser(projectId, instanceId, fdcP4SAmode, fdcP4SAUser);
+
+  // 3. Setup FDC required SQL roles and permissions.
+  await setupSQLPermissions(instanceId, databaseId, options, true);
+
+  // 4. Apply necessary grants.
+  const grants = [
+    // Grant firebaseowner role to the current IAM user.
+    `GRANT "${firebaseowner(databaseId)}" TO "${user}"`,
+    // Grant firebaswriter to the FDC P4SA user
+    `GRANT "${firebasewriter(databaseId)}" TO "${fdcP4SAUser}"`,
+  ];
+
+  await executeSqlCmdsAsSuperUser(options, instanceId, databaseId, grants, /** silent=*/ true);
+  return user;
 }
 
 // Converts a account name to the equivalent SQL user.
 // - Postgres: https://cloud.google.com/sql/docs/postgres/iam-logins#log-in-with-automatic
 //   - For user: it's full email address.
 //   - For service account: it's email address without the .gserviceaccount.com domain suffix.
-function toDatabaseUser(account: string): { user: string; mode: UserType } {
+export function toDatabaseUser(account: string): { user: string; mode: UserType } {
   let mode: UserType = "CLOUD_IAM_USER";
   let user = account;
   if (account.endsWith(".gserviceaccount.com")) {
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		- Improve Firebase Data Connect postgres security by granting fine grained SQL privileges to the users the need it. (#7578)
		- Remove `dataconnect:sql:migrate` command hard dependency on 'roles/cloudsql.admin'. (#7578)