fix(functions): defer secret access granting to release phase to prevent SA 404 race condition

CHANGELOG.md

@@ -1,3 +1,4 @@
+- [Fixed] Defer secret access permission granting to release phase to prevent service account 404 race conditions.
 - Fixed parsing and path resolution bugs in `ext:export` options, and reverted `--extension-instance` option back to `--instance`.
 - [Fixed] Increases default polling timeout for App Hosting operations and rollouts to 60 minutes.
 - Fixed an issue where App Hosting deploys failed when the deploying account lacked permission to create or grant roles to the default compute service account, even when that service account already existed. (#10806)

src/deploy/functions/ensure.spec.ts

@@ -31,7 +31,7 @@
   afterEach(() => {
     expect(nock.isDone()).to.be.true;
     sandbox.restore();
     timeStub = null;
     logStub = null;
   });
 
@@ -157,12 +157,6 @@
     secret: "MY_SECRET_0",
     version: "2",
   };
-  const secret1: backend.SecretEnvVar = {
-    projectId: "project",
-    key: "ANOTHER_SECRET",
-    secret: "ANOTHER_SECRET",
-    version: "1",
-  };
   const e: backend.Endpoint = {
     ...ENDPOINT,
     project: projectId,
@@ -184,84 +178,34 @@
     secretManagerMock.restore();
   });
 
-  it("ensures access to default service account", async () => {
-    const b = backend.of({
-      ...e,
-      secretEnvironmentVariables: [secret0],
-    });
+  it("grants secret access to specified service accounts", async () => {
     secretManagerMock
       .expects("ensureServiceAgentRole")
       .once()
       .withExactArgs(
-        { name: secret0.secret, projectId: projectId },
-        [DEFAULT_SA],
+        { name: secret0.secret, projectId },
+        ["foo@bar.com"],
         "roles/secretmanager.secretAccessor",
       );
-    await ensure.secretAccess(projectId, b, backend.empty());
-  });
-
-  it("ensures access to all secrets", async () => {
-    const b = backend.of({
-      ...e,
-      secretEnvironmentVariables: [secret0, secret1],
+    await ensure.grantSecretAccess({
+      projectId,
+      secret: secret0.secret,
+      serviceAccounts: ["foo@bar.com"],
     });
-    secretManagerMock.expects("ensureServiceAgentRole").twice();
-    await ensure.secretAccess(projectId, b, backend.empty());
   });
 
-  it("combines service account to make one call per secret", async () => {
-    const b = backend.of(
-      {
-        ...e,
-        secretEnvironmentVariables: [secret0],
-      },
-      {
-        ...e,
-        id: "another-id",
-        serviceAccount: "foo@bar.com",
-        secretEnvironmentVariables: [secret0],
-      },
-    );
-    secretManagerMock
-      .expects("ensureServiceAgentRole")
-      .once()
-      .withExactArgs(
-        { name: secret0.secret, projectId: projectId },
-        [DEFAULT_SA, "foo@bar.com"],
-        "roles/secretmanager.secretAccessor",
-      );
-    await ensure.secretAccess(projectId, b, backend.empty());
-  });
-
-  it("skips calling IAM if secret is already bound to a service account", async () => {
+  it("calculates secretsAccessDelta correctly", async () => {
     const b = backend.of({
       ...e,
       secretEnvironmentVariables: [secret0],
     });
-    secretManagerMock.expects("ensureServiceAgentRole").never();
-    await ensure.secretAccess(projectId, b, b);
-  });
-
-  it("does not include service account already bounud to a secret", async () => {
-    const haveEndpoint = {
-      ...e,
-      secretEnvironmentVariables: [secret0],
-    };
-    const haveBackend = backend.of(haveEndpoint);
-    const wantBackend = backend.of(haveEndpoint, {
-      ...e,
-      id: "another-id",
-      serviceAccount: "foo@bar.com",
-      secretEnvironmentVariables: [secret0],
+    const delta = await ensure.secretsAccessDelta({
+      projectId,
+      wantBackend: b,
+      haveBackend: backend.empty(),
+    });
+    expect(delta).to.deep.equal({
+      [secret0.secret]: [DEFAULT_SA],
     });
-    secretManagerMock
-      .expects("ensureServiceAgentRole")
-      .once()
-      .withExactArgs(
-        { name: secret0.secret, projectId: projectId },
-        ["foo@bar.com"],
-        "roles/secretmanager.secretAccessor",
-      );
-    await ensure.secretAccess(projectId, wantBackend, haveBackend);
   });
 });

src/deploy/functions/ensure.ts

@@ -67,17 +67,17 @@
 
 /**
  * Checks for various warnings and API enablements needed based on the runtime
  * of the deployed functions.
  *
  * @param projectId Project ID upon which to check enablement.
  */
 export async function cloudBuildEnabled(projectId: string): Promise<void> {
   try {
     await ensure(projectId, cloudbuildOrigin(), "functions");
   } catch (e: any) {
     if (isBillingError(e)) {
       throw nodeBillingError(projectId);
     } else if (isPermissionError(e)) {
       throw nodePermissionError(projectId);
     }
 
@@ -91,10 +91,13 @@
 async function secretsToServiceAccounts(b: backend.Backend): Promise<Record<string, Set<string>>> {
   const secretsToSa: Record<string, Set<string>> = {};
   for (const e of backend.allEndpoints(b)) {
+    if (!e.secretEnvironmentVariables || e.secretEnvironmentVariables.length === 0) {
+      continue;
+    }
     // BUG BUG BUG? Test whether we've resolved e.serviceAccount to be project-relative
     // by this point.
     const sa = e.serviceAccount || ((await module.exports.defaultServiceAccount(e)) as string);
-    for (const s of e.secretEnvironmentVariables || []) {
+    for (const s of e.secretEnvironmentVariables) {
       const serviceAccounts = secretsToSa[s.secret] || new Set();
       serviceAccounts.add(sa);
       secretsToSa[s.secret] = serviceAccounts;
@@ -104,51 +107,18 @@
 }
 
 /**
- * Ensures that runtime service account has access to the secrets.
- *
- * To avoid making more than one simultaneous call to setIamPolicy calls per secret, the function batches all
- * service account that requires access to it.
+ * Returns a mapping of secret names to service account emails that require access to them.
  */
-export async function secretAccess(
-  projectId: string,
-  wantBackend: backend.Backend,
-  haveBackend: backend.Backend,
-  dryRun?: boolean,
-) {
-  const ensureAccess = async (secret: string, serviceAccounts: string[]) => {
-    logLabeledBullet(
-      "functions",
-      `ensuring ${clc.bold(serviceAccounts.join(", "))} access to secret ${clc.bold(secret)}.`,
-    );
-    if (dryRun) {
-      const check = await checkServiceAgentRole(
-        { name: secret, projectId },
-        serviceAccounts,
-        "roles/secretmanager.secretAccessor",
-      );
-      if (check.length) {
-        logLabeledBullet(
-          "functions",
-          `On your next deploy, ${clc.bold(serviceAccounts.join(", "))} will be granted access to secret ${clc.bold(secret)}.`,
-        );
-      }
-    } else {
-      await ensureServiceAgentRole(
-        { name: secret, projectId },
-        serviceAccounts,
-        "roles/secretmanager.secretAccessor",
-      );
-    }
-    logLabeledSuccess(
-      "functions",
-      `ensured ${clc.bold(serviceAccounts.join(", "))} access to ${clc.bold(secret)}.`,
-    );
-  };
-
+export async function secretsAccessDelta(args: {
+  projectId: string;
+  wantBackend: backend.Backend;
+  haveBackend: backend.Backend;
+}): Promise<Record<string, string[]>> {
+  const { wantBackend, haveBackend } = args;
   const wantSecrets = await secretsToServiceAccounts(wantBackend);
   const haveSecrets = await secretsToServiceAccounts(haveBackend);
 
-  // Remove secret/service account pairs that already exists to avoid unnecessary IAM calls.
+  // Remove secret/service account pairs that already exist to avoid unnecessary IAM calls.
   for (const [secret, serviceAccounts] of Object.entries(haveSecrets)) {
     for (const serviceAccount of serviceAccounts) {
       wantSecrets[secret]?.delete(serviceAccount);
@@ -158,9 +128,61 @@
     }
   }
 
-  const ensure = [];
+  const delta: Record<string, string[]> = {};
   for (const [secret, serviceAccounts] of Object.entries(wantSecrets)) {
-    ensure.push(ensureAccess(secret, Array.from(serviceAccounts)));
+    if (serviceAccounts.size > 0) {
+      delta[secret] = Array.from(serviceAccounts);
+    }
+  }
+  return delta;
+}
+
+/**
+ * Checks secret access in dry run mode and logs messages for permissions to be granted.
+ */
+export async function checkSecretAccess(
+  projectId: string,
+  secretAccessDelta: Record<string, string[]>,
+): Promise<void> {
+  for (const [secret, serviceAccounts] of Object.entries(secretAccessDelta)) {
+    logLabeledBullet(
+      "functions",
+      `ensuring ${clc.bold(serviceAccounts.join(", "))} access to secret ${clc.bold(secret)}.`,
+    );
+    const check = await checkServiceAgentRole(
+      { name: secret, projectId },
+      serviceAccounts,
+      "roles/secretmanager.secretAccessor",
+    );
+    if (check.length) {
+      logLabeledBullet(
+        "functions",
+        `On your next deploy, ${clc.bold(serviceAccounts.join(", "))} will be granted access to secret ${clc.bold(secret)}.`,
+      );
+    }
   }
-  await Promise.all(ensure);
+}
+
+/**
+ * Grants secret access for a single secret to specified service accounts.
+ */
+export async function grantSecretAccess(args: {
+  projectId: string;
+  secret: string;
+  serviceAccounts: string[];
+}): Promise<void> {
+  const { projectId, secret, serviceAccounts } = args;
+  logLabeledBullet(
+    "functions",
+    `ensuring ${clc.bold(serviceAccounts.join(", "))} access to secret ${clc.bold(secret)}.`,
+  );
+  await ensureServiceAgentRole(
+    { name: secret, projectId },
+    serviceAccounts,
+    "roles/secretmanager.secretAccessor",
+  );
+  logLabeledSuccess(
+    "functions",
+    `ensured ${clc.bold(serviceAccounts.join(", "))} access to ${clc.bold(secret)}.`,
+  );
 }

src/deploy/functions/prepare.ts

@@ -171,7 +171,7 @@
   }
 
   const existingSalt = haveRolesEtag ? haveRolesEtag.split("-")[0] : undefined;
   const newEtag = iam.computeRolesEtag(requiredRoles!, existingSalt);
 
   for (const endpoint of backend.allEndpoints(want)) {
     endpoint.serviceAccount = managedSA;
@@ -210,10 +210,10 @@
   if (existingManagedSA) {
     try {
       haveRoles = await resourcemanager.getServiceAccountRoles(projectId, managedSA);
     } catch (err: any) {
       throw new FirebaseError(
         `The declarative security roles for codebase ${codebase} have changed, but you do not have access to see what has changed. Please ask an IAM administrator to perform the next deploy.`,
         { original: err },
       );
     }
   }
@@ -546,7 +546,15 @@
     haveBackend,
     options.dryRun,
   );
-  await ensure.secretAccess(projectId, matchingBackend, haveBackend, options.dryRun);
+  // Actual granting of secret access permissions has been moved to the fabricator in release because declarative security may mean that the desired service account hasn't been created
+  if (options.dryRun) {
+    const secretAccessDelta = await ensure.secretsAccessDelta({
+      projectId,
+      wantBackend: matchingBackend,
+      haveBackend,
+    });
+    await ensure.checkSecretAccess(projectId, secretAccessDelta);
+  }
   /**
    * ===Phase 8 Generates the hashes for each of the functions now that secret versions have been resolved.
    * This must be called after `await validate.secretsAreValid`.

src/deploy/functions/release/fabricator.ts

@@ -7,6 +7,7 @@ import {
   isTransientError,
   parseErrorCode,
 } from "./executor";
+import * as ensure from "../ensure";
 import { FirebaseError } from "../../../error";
 
 import { SourceTokenScraper } from "./sourceTokenScraper";
@@ -242,6 +243,23 @@ export class Fabricator {
       await this.grantNewRoles(codebasePlan, codebase);
     }
 
+    const secretAccessPromises = Object.values(plan).flatMap((codebasePlan) =>
+      Object.entries(codebasePlan.secretAccessPlan || {}).map(([secret, serviceAccounts]) =>
+        this.executor.run(
+          () =>
+            ensure.grantSecretAccess({
+              projectId: this.projectId,
+              secret,
+              serviceAccounts,
+            }),
+          {
+            retryPredicates: [isTransientError, isServiceAccount404],
+          },
+        ),
+      ),
+    );
+    await Promise.all(secretAccessPromises);
+
     // Accumulate all regional changesets across all codebases
     const allChangesets: planner.Changeset[] = [];
     for (const codebasePlan of Object.values(plan)) {

src/deploy/functions/release/planner.ts

@@ -7,6 +7,7 @@ import { isFirebaseManaged } from "../../../deploymentTool";
 import { FirebaseError } from "../../../error";
 import * as utils from "../../../utils";
 import * as backend from "../backend";
+import * as ensure from "../ensure";
 import * as v2events from "../../../functions/events/v2";
 
 export interface EndpointUpdate {
@@ -25,6 +26,7 @@ export interface Changeset {
 export interface BaseCodebasePlan {
   regionalChangesets: Record<string, Changeset>;
   plannedBackend: backend.Backend;
+  secretAccessPlan?: Record<string, string[]>;
 }
 
 export interface ActiveSecurityPlan {
@@ -225,6 +227,12 @@ export async function createDeploymentPlan(args: PlanArgs): Promise<CodebasePlan
         "old default of 1. You can change this with the 'concurrency' option.",
     );
   }
+  const secretAccessPlan = await ensure.secretsAccessDelta({
+    projectId: args.projectId,
+    wantBackend,
+    haveBackend,
+  });
+
   if (requiredRoles && hasWantEndpoints) {
     if (!managedSA) {
       throw new FirebaseError("managedServiceAccount is required when requiredRoles is defined.", {
@@ -234,6 +242,7 @@ export async function createDeploymentPlan(args: PlanArgs): Promise<CodebasePlan
     return {
       regionalChangesets,
       plannedBackend: wantBackend,
+      secretAccessPlan,
       rolesToAdd,
       rolesToRemove,
       serviceAccountToCreate,
@@ -243,6 +252,7 @@ export async function createDeploymentPlan(args: PlanArgs): Promise<CodebasePlan
     return {
       regionalChangesets,
       plannedBackend: wantBackend,
+      secretAccessPlan,
       serviceAccountToDelete,
     };
   }