Can grant access to emails (user, group, SA) (#8357)

* Can grant access to emails (user, group, SA)

* Support multiple secrets

* Lint and changelog

* PR feedback

Author: inlined

CHANGELOG.md

@@ -1 +1,2 @@
 - Fix bug in Auth emulator where accounts:lookup is case-sensitive for emails (#8344)
+- firebase apphosting:secrets:grantAccess can now grant access to emails and can grant multiple secrets at once (#8357)

src/apphosting/secrets/index.spec.ts

@@ -11,6 +11,7 @@ import * as utilsImport from "../../utils";
 import * as promptImport from "../../prompt";
 
 import { Secret } from "../yaml";
+import { FirebaseError } from "../../error";
 
 describe("secrets", () => {
   let gcsm: sinon.SinonStubbedInstance<typeof gcsmImport>;
@@ -259,6 +260,128 @@ describe("secrets", () => {
     });
   });
 
+  describe("grantEmailsSecretAccess", () => {
+    const secret = {
+      projectId: "projectId",
+      name: "secret",
+    };
+    const secret2 = {
+      projectId: "projectId",
+      name: "secret2",
+    };
+    const existingPolicy: iam.Policy = {
+      version: 1,
+      etag: "tag",
+      bindings: [
+        {
+          role: "roles/viewer",
+          members: ["serviceAccount:existingSA"],
+        },
+      ],
+    };
+
+    it("should brute force its way to success", async () => {
+      gcsm.getIamPolicy.resolves(existingPolicy);
+      gcsm.setIamPolicy
+        .onFirstCall()
+        .rejects(
+          new FirebaseError(
+            'Principal [email protected] is of type "group". The principal should appear as "group:[email protected]"',
+          ),
+        );
+      gcsm.setIamPolicy.onSecondCall().resolves();
+
+      await secrets.grantEmailsSecretAccess(
+        secret.projectId,
+        [secret.name],
+        ["[email protected]", "[email protected]"],
+      );
+
+      expect(gcsm.getIamPolicy).to.be.calledWithMatch(secret);
+      expect(gcsm.setIamPolicy.firstCall).to.be.calledWithMatch(secret, [
+        {
+          role: "roles/viewer",
+          members: ["serviceAccount:existingSA"],
+        },
+        {
+          role: "roles/secretmanager.secretAccessor",
+          members: ["user:[email protected]", "user:[email protected]"],
+        },
+      ]);
+      expect(gcsm.setIamPolicy.secondCall).to.be.calledWithMatch(secret, [
+        {
+          role: "roles/viewer",
+          members: ["serviceAccount:existingSA"],
+        },
+        {
+          role: "roles/secretmanager.secretAccessor",
+          members: ["user:[email protected]", "group:[email protected]"],
+        },
+      ]);
+    });
+
+    it("Should remember what it learns while brute forcing across multiple secrets", async () => {
+      gcsm.getIamPolicy.resolves(existingPolicy);
+      gcsm.setIamPolicy
+        .onFirstCall()
+        .rejects(
+          new FirebaseError(
+            'Principal [email protected] is of type "group". The principal should appear as "group:[email protected]"',
+          ),
+        );
+      gcsm.setIamPolicy.onSecondCall().resolves();
+      gcsm.setIamPolicy.onThirdCall().resolves();
+
+      await secrets.grantEmailsSecretAccess(
+        secret.projectId,
+        [secret.name, secret2.name],
+        ["[email protected]", "[email protected]"],
+      );
+
+      expect(gcsm.getIamPolicy).to.be.calledWithMatch(secret);
+      expect(gcsm.setIamPolicy).to.be.calledThrice;
+      expect(gcsm.setIamPolicy.firstCall).to.be.calledWithMatch(secret, [
+        {
+          role: "roles/viewer",
+          members: ["serviceAccount:existingSA"],
+        },
+        {
+          role: "roles/secretmanager.secretAccessor",
+          members: ["user:[email protected]", "user:[email protected]"],
+        },
+      ]);
+      expect(gcsm.setIamPolicy.secondCall).to.be.calledWithMatch(secret, [
+        {
+          role: "roles/viewer",
+          members: ["serviceAccount:existingSA"],
+        },
+        {
+          role: "roles/secretmanager.secretAccessor",
+          members: ["user:[email protected]", "group:[email protected]"],
+        },
+      ]);
+      expect(gcsm.setIamPolicy.thirdCall).to.be.calledWithMatch(secret2, [
+        {
+          role: "roles/viewer",
+          members: ["serviceAccount:existingSA"],
+        },
+        {
+          role: "roles/secretmanager.secretAccessor",
+          members: ["user:[email protected]", "group:[email protected]"],
+        },
+      ]);
+    });
+
+    it("Should fail if the error is not specifically a principal type error", async () => {
+      gcsm.getIamPolicy.resolves(existingPolicy);
+      gcsm.setIamPolicy.rejects(new FirebaseError("Some other error"));
+
+      await expect(
+        secrets.grantEmailsSecretAccess(secret.projectId, [secret.name], ["[email protected]"]),
+      ).to.eventually.be.rejectedWith(/Failed to set IAM bindings/);
+    });
+  });
+
   describe("fetchSecrets", () => {
     const projectId = "randomProject";
     it("correctly attempts to fetch secret and it's version", async () => {

src/apphosting/secrets/index.ts

@@ -98,20 +98,84 @@ export async function grantSecretAccess(
     );
   }
 
+  const updatedBindings = existingBindings.concat(newBindings);
   try {
-    // TODO: Merge with existing bindings with the same role
-    const updatedBindings = existingBindings.concat(newBindings);
     await gcsm.setIamPolicy({ projectId, name: secretName }, updatedBindings);
   } catch (err: unknown) {
     throw new FirebaseError(
-      `Failed to set IAM bindings ${JSON.stringify(newBindings)} on secret: ${secretName}. Ensure you have the permissions to do so and try again.`,
+      `Failed to set IAM bindings ${JSON.stringify(newBindings)} on secret: ${secretName}. Ensure you have the permissions to do so and try again. ` +
+        "For more information visit https://cloud.google.com/secret-manager/docs/manage-access-to-secrets#required-roles",
       { original: getError(err) },
     );
   }
 
   utils.logSuccess(`Successfully set IAM bindings on secret ${secretName}.\n`);
 }
 
+/**
+ * Grants the following users or groups access to the provided secret.
+ */
+export async function grantEmailsSecretAccess(
+  projectId: string,
+  secretNames: string[],
+  emails: string[],
+): Promise<void> {
+  // This feels like a hack, but it's actually sorta taking advantage of an escalation of privilege in Google IAM.
+  // The correct way to determine if an email address is a user or group is to use the Google Admin API
+  // (GET e.g. admin.googleapis.com/admin/directory/v1/users/<email> or GET admin.googleapis.com/admin/driectory/v1/groups/<email>)
+  // but that would require us to have admin permissions on GMail for example. Fortunately, IAM seems to give us well formed errors
+  // that dictate what type of role the email address should have been bound with. This seems... like a design mistake. If they knew
+  // already, why not just accept the value without leaking its type?
+  // Note: we keep typeGuesses outside of the loop so that we learn the type of principal an email is once across all secrets.
+  const typeGuesses = Object.fromEntries(emails.map((email) => [email, "user"]));
+  for (const secretName of secretNames) {
+    let existingBindings;
+    try {
+      existingBindings = (await gcsm.getIamPolicy({ projectId, name: secretName })).bindings || [];
+    } catch (err: unknown) {
+      throw new FirebaseError(
+        `Failed to get IAM bindings on secret: ${secretName}. Ensure you have the permissions to do so and try again. ` +
+          "For more information visit https://cloud.google.com/secret-manager/docs/manage-access-to-secrets#required-roles",
+        { original: getError(err) },
+      );
+    }
+
+    do {
+      try {
+        const newBindings: iam.Binding[] = [
+          {
+            role: "roles/secretmanager.secretAccessor",
+            members: Object.entries(typeGuesses).map(([email, type]) => `${type}:${email}`),
+          },
+        ];
+        const updatedBindings = existingBindings.concat(newBindings);
+        await gcsm.setIamPolicy({ projectId, name: secretName }, updatedBindings);
+        break;
+      } catch (err: any) {
+        if (!(err instanceof FirebaseError)) {
+          throw new FirebaseError(
+            `Unexpected error updating IAM bindings on secret: ${secretName}`,
+            {
+              original: getError(err),
+            },
+          );
+        }
+        const match = /Principal (.*) is of type "([^"]+)"/.exec(err.message);
+        if (!match) {
+          throw new FirebaseError(
+            `Failed to set IAM bindings on secret: ${secretName}. Ensure you have the permissions to do so and try again.`,
+            { original: getError(err) },
+          );
+        }
+        typeGuesses[match[1]] = match[2];
+        continue;
+      }
+    } while (true);
+
+    utils.logSuccess(`Successfully set IAM bindings on secret ${secretName}.\n`);
+  }
+}
+
 /**
  * Ensures a secret exists for use with app hosting, optionally locked to a region.
  * If a secret exists, we verify the user is not trying to change the region and verifies a secret

src/commands/apphosting-secrets-grantaccess.ts

@@ -9,10 +9,13 @@ import * as apphosting from "../gcp/apphosting";
 import * as secrets from "../apphosting/secrets";
 import { getBackendForAmbiguousLocation } from "../apphosting/backend";
 
-export const command = new Command("apphosting:secrets:grantaccess <secretName>")
-  .description("grant service accounts permissions to the provided secret")
+export const command = new Command("apphosting:secrets:grantaccess <secretNames>")
+  .description(
+    "grant service accounts, users, or groups permissions to the provided secret(s). Can pass one or more secrets, separated by a comma",
+  )
   .option("-l, --location <location>", "backend location", "-")
   .option("-b, --backend <backend>", "backend name")
+  .option("-e, --emails <emails>", "comma delimited list of user or group emails")
   .before(requireAuth)
   .before(secretManager.ensureApi)
   .before(apphosting.ensureApiEnabled)
@@ -24,19 +27,35 @@ export const command = new Command("apphosting:secrets:grantaccess <secretName>"
     "secretmanager.secrets.getIamPolicy",
     "secretmanager.secrets.setIamPolicy",
   ])
-  .action(async (secretName: string, options: Options) => {
+  .action(async (secretNames: string, options: Options) => {
     const projectId = needProjectId(options);
     const projectNumber = await needProjectNumber(options);
 
-    if (!options.backend) {
+    if (!options.backend && !options.emails) {
       throw new FirebaseError(
-        "Missing required flag --backend. See firebase apphosting:secrets:grantaccess --help for more info",
+        "Missing required flag --backend or --emails. See firebase apphosting:secrets:grantaccess --help for more info",
+      );
+    }
+    if (options.backend && options.emails) {
+      throw new FirebaseError(
+        "Cannot specify both --backend and --emails. See firebase apphosting:secrets:grantaccess --help for more info",
       );
     }
 
-    const exists = await secretManager.secretExists(projectId, secretName);
-    if (!exists) {
-      throw new FirebaseError(`Cannot find secret ${secretName}`);
+    const secretList = secretNames.split(",");
+    for (const secretName of secretList) {
+      const exists = await secretManager.secretExists(projectId, secretName);
+      if (!exists) {
+        throw new FirebaseError(`Cannot find secret ${secretName}`);
+      }
+    }
+
+    if (options.emails) {
+      return await secrets.grantEmailsSecretAccess(
+        projectId,
+        secretList,
+        String(options.emails).split(","),
+      );
     }
 
     const backendId = options.backend as string;
@@ -54,5 +73,9 @@ export const command = new Command("apphosting:secrets:grantaccess <secretName>"
 
     const accounts = secrets.toMulti(secrets.serviceAccountsForBackend(projectNumber, backend));
 
-    await secrets.grantSecretAccess(projectId, projectNumber, secretName, accounts);
+    await Promise.allSettled(
+      secretList.map((secretName) =>
+        secrets.grantSecretAccess(projectId, projectNumber, secretName, accounts),
+      ),
+    );
   });