diff --git a/CHANGELOG.md b/CHANGELOG.md index dd90ef62c27..7132efcea70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,4 @@ +- Fixed `functions:secrets:set` and `functions:secrets:prune` not destroying unused versions of secrets created with v13.6.1 or later. (#11066) - [fixed] Improve error message when billing is not enabled - Fixed a crash in `setEnqueuer` when deploying Cloud Tasks functions whose IAM policy has no bindings (#11184). - Updated dependencies to address security vulnerabilities, including `protobufjs`, `tar`, `@grpc/grpc-js`, `express`, `undici`, `hono`, `tmp`, and `form-data`. diff --git a/src/commands/functions-secrets-prune.ts b/src/commands/functions-secrets-prune.ts index a360deafd8f..c2c1bdf9732 100644 --- a/src/commands/functions-secrets-prune.ts +++ b/src/commands/functions-secrets-prune.ts @@ -7,11 +7,10 @@ import { Command } from "../command"; import { Options } from "../options"; import { needProjectId, needProjectNumber } from "../projectUtils"; import { requirePermissions } from "../requirePermissions"; -import { isFirebaseManaged } from "../deploymentTool"; import { logBullet, logSuccess } from "../utils"; import { confirm } from "../prompt"; -import { destroySecretVersion } from "../gcp/secretManager"; import { requireAuth } from "../requireAuth"; +import { FirebaseError } from "../error"; export const command = new Command("functions:secrets:prune") .withForce("destroy unused secrets without prompt") @@ -30,10 +29,11 @@ export const command = new Command("functions:secrets:prune") logBullet("Loading secrets..."); - const haveBackend = await backend.existingBackend({ projectId } as args.Context); - const haveEndpoints = backend - .allEndpoints(haveBackend) - .filter((e) => isFirebaseManaged(e.labels || [])); + const context = { projectId } as args.Context; + const haveBackend = await backend.existingBackend(context); + backend.assertAllRegionsReachable(context); + // Every function counts as a consumer, whatever tool deployed it. + const haveEndpoints = backend.allEndpoints(haveBackend); const pruned = await secrets.pruneSecrets({ projectNumber, projectId }, haveEndpoints); @@ -48,14 +48,17 @@ export const command = new Command("functions:secrets:prune") pruned.map((sv) => `${sv.secret}@${sv.version}`).join("\n\t"), ); - const confirmed = - options.destroy || - (await confirm({ - message: `Do you want to destroy unused secret versions?`, - default: true, - force: options.force, - nonInteractive: options.nonInteractive, - })); + if (options.nonInteractive && !options.force) { + throw new FirebaseError( + "Refusing to destroy secret versions in non-interactive mode. Pass --force to destroy them without confirmation.", + ); + } + const confirmed = await confirm({ + message: `Do you want to destroy unused secret versions?`, + default: true, + force: options.force, + nonInteractive: options.nonInteractive, + }); if (!confirmed) { logBullet( "Run the following commands to destroy each unused secret version:\n\t" + @@ -65,6 +68,18 @@ export const command = new Command("functions:secrets:prune") ); return; } - await Promise.all(pruned.map((sv) => destroySecretVersion(projectId, sv.secret, sv.version))); + const { destroyed, erred } = await secrets.destroySecretVersions(pruned); + if (destroyed.length) { + logBullet( + "Destroyed secret versions:\n\t" + + destroyed.map((sv) => `${sv.secret}@${sv.version}`).join("\n\t"), + ); + } + if (erred.length) { + throw new FirebaseError( + `Failed to destroy ${erred.length} secret versions:\n\t` + + erred.map((e) => e.message).join("\n\t"), + ); + } logSuccess("Destroyed all unused secrets!"); }); diff --git a/src/commands/functions-secrets-set.ts b/src/commands/functions-secrets-set.ts index cd586d2724c..645a5b17d04 100644 --- a/src/commands/functions-secrets-set.ts +++ b/src/commands/functions-secrets-set.ts @@ -12,7 +12,6 @@ import { logBullet, logSuccess, logWarning, readSecretValue } from "../utils"; import { needProjectId, needProjectNumber } from "../projectUtils"; import { addVersion, - destroySecretVersion, toSecretVersionResourceName, isFunctionsManaged, ensureApi, @@ -31,8 +30,11 @@ export const command = new Command("functions:secrets:set ") .before(requirePermissions, [ "secretmanager.secrets.create", "secretmanager.secrets.get", + "secretmanager.secrets.list", "secretmanager.secrets.update", "secretmanager.versions.add", + "secretmanager.versions.destroy", + "secretmanager.versions.list", ]) .option( "--data-file ", @@ -91,7 +93,8 @@ export const command = new Command("functions:secrets:set ") return; } - let haveBackend = await backend.existingBackend({ projectId } as args.Context); + const context = { projectId } as args.Context; + let haveBackend = await backend.existingBackend(context); const endpointsToUpdate = backend .allEndpoints(haveBackend) .filter((e) => secrets.inUse({ projectId, projectNumber }, secret, e)); @@ -105,10 +108,19 @@ export const command = new Command("functions:secrets:set ") endpointsToUpdate.map((e) => `${e.id}(${e.region})`).join("\n\t"), ); + // Only the versions these functions are moving off are candidates for destruction. + // Anything else unused is left to functions:secrets:prune, which lists what it will remove. + const staleVersions = new Set( + secrets + .of(endpointsToUpdate) + .filter((sev) => sev.secret === secret.name && sev.version && sev.version !== "latest") + .map((sev) => sev.version), + ); + const redeploy = options.nonInteractive ? false : await confirm({ - message: `Do you want to re-deploy the functions and destroy the stale version of secret ${secret.name}?`, + message: `Do you want to re-deploy the functions and destroy the stale versions of secret ${secret.name}?`, default: true, force: options.force, }); @@ -133,7 +145,8 @@ export const command = new Command("functions:secrets:set ") await Promise.all(updateOps); // Double check that old secrets versions are unused. - haveBackend = await backend.existingBackend({ projectId } as args.Context, true); + haveBackend = await backend.existingBackend(context, true); + backend.assertAllRegionsReachable(context); const staleEndpoints = backend.allEndpoints( backend.matchingBackend(haveBackend, (e) => { const pInfo = { projectId, projectNumber }; @@ -151,16 +164,22 @@ export const command = new Command("functions:secrets:set ") ); } - // Remove stale secret versions; const secretsToPrune = ( await secrets.pruneSecrets({ projectId, projectNumber }, backend.allEndpoints(haveBackend)) - ).filter((sv) => sv.key === key); + ).filter((sv) => sv.key === key && staleVersions.has(sv.version)); + if (secretsToPrune.length === 0) { + return; + } logBullet( `Removing secret versions: ${secretsToPrune .map((sv) => sv.key + "[" + sv.version + "]") .join(", ")}`, ); - await Promise.all( - secretsToPrune.map((sv) => destroySecretVersion(projectId, sv.secret, sv.version)), - ); + const { erred } = await secrets.destroySecretVersions(secretsToPrune); + if (erred.length) { + throw new FirebaseError( + `Failed to destroy ${erred.length} secret versions:\n\t` + + erred.map((e) => e.message).join("\n\t"), + ); + } }); diff --git a/src/deploy/functions/backend.spec.ts b/src/deploy/functions/backend.spec.ts index 92b2d4101db..da692601b6b 100644 --- a/src/deploy/functions/backend.spec.ts +++ b/src/deploy/functions/backend.spec.ts @@ -538,6 +538,30 @@ describe("Backend", () => { expect(logLabeledWarning).to.have.been.called; }); }); + + describe("assertAllRegionsReachable", () => { + it("does nothing when every region was reachable", async () => { + listAllFunctions.resolves({ functions: [], unreachable: [] }); + listAllFunctionsV2.resolves({ functions: [], unreachable: [] }); + const context = newContext(); + await backend.existingBackend(context); + + expect(() => backend.assertAllRegionsReachable(context)).to.not.throw(); + }); + + it("throws naming the unreachable regions", async () => { + listAllFunctions.resolves({ functions: [], unreachable: ["us-central1"] }); + listAllFunctionsV2.resolves({ functions: [], unreachable: ["europe-west1"] }); + const context = newContext(); + await backend.existingBackend(context); + + expect(() => backend.assertAllRegionsReachable(context)) + .to.throw(FirebaseError) + .with.property("message") + .that.includes("us-central1") + .and.includes("europe-west1"); + }); + }); }); describe("compareFunctions", () => { diff --git a/src/deploy/functions/backend.ts b/src/deploy/functions/backend.ts index 570bb765ce0..886f6fd55c4 100644 --- a/src/deploy/functions/backend.ts +++ b/src/deploy/functions/backend.ts @@ -689,6 +689,27 @@ async function loadCloudRunServices( } } +/** + * Throws if any region was unreachable when the existing backend was loaded. For callers that + * act on the absence of a function, such as destroying secret versions nothing appears to use, + * where a missing region must not read as "not in use". + * @param context A context object from the Command library, after existingBackend has run. + */ +export function assertAllRegionsReachable(context: Context): void { + const unreachable = [ + ...(context.unreachableRegions?.gcfV1 || []), + ...(context.unreachableRegions?.gcfV2 || []), + ...(context.unreachableRegions?.run || []), + ]; + if (unreachable.length) { + throw new FirebaseError( + "The following Cloud Functions regions are currently unreachable:\n\t" + + unreachable.join("\n\t") + + "\nFunctions in those regions could not be checked. Please try again in a few minutes.", + ); + } +} + /** * A helper function that guards against unavailable regions affecting a backend deployment. * If the desired backend uses a region that is unavailable, a FirebaseError is thrown. diff --git a/src/functions/secrets.spec.ts b/src/functions/secrets.spec.ts index ac086b6a774..48f88e2b598 100644 --- a/src/functions/secrets.spec.ts +++ b/src/functions/secrets.spec.ts @@ -227,10 +227,11 @@ describe("functions/secret", () => { let listSecretVersionsStub: sinon.SinonStub; let getSecretVersionStub: sinon.SinonStub; + // Secrets created before v13.6.1 carry firebase-managed=true. const secret1: secretManager.Secret = { projectId: "project", name: "MY_SECRET1", - labels: {}, + labels: { [secretManager.FIREBASE_MANAGED]: "true" }, replication: {}, }; const secretVersion11: secretManager.SecretVersion = { @@ -247,9 +248,35 @@ describe("functions/secret", () => { const secret2: secretManager.Secret = { projectId: "project", name: "MY_SECRET2", + labels: { [secretManager.FIREBASE_MANAGED]: "functions" }, + replication: {}, + }; + + const unmanagedSecret: secretManager.Secret = { + projectId: "project", + name: "UNMANAGED", labels: {}, replication: {}, }; + + const apphostingSecret: secretManager.Secret = { + projectId: "project", + name: "APPHOSTING_SECRET", + labels: { [secretManager.FIREBASE_MANAGED]: "apphosting" }, + replication: {}, + }; + + const unmanagedVersion: secretManager.SecretVersion = { + secret: unmanagedSecret, + versionId: "1", + createTime: "2024-03-28T19:43:26", + }; + + const apphostingVersion: secretManager.SecretVersion = { + secret: apphostingSecret, + versionId: "1", + createTime: "2024-03-28T19:43:26", + }; const secretVersion21: secretManager.SecretVersion = { secret: secret2, versionId: "1", @@ -289,6 +316,43 @@ describe("functions/secret", () => { ).to.eventually.deep.equal([]); }); + it("finds secrets managed under either label value and ignores the rest", async () => { + listSecretsStub.resolves([secret1, secret2, unmanagedSecret, apphostingSecret]); + // Keyed by name rather than call order so an unmanaged secret slipping through + // shows up as an extra pruned version instead of an exhausted stub. + listSecretVersionsStub.withArgs("project", secret1.name).resolves([secretVersion11]); + listSecretVersionsStub.withArgs("project", secret2.name).resolves([secretVersion21]); + listSecretVersionsStub.withArgs("project", unmanagedSecret.name).resolves([unmanagedVersion]); + listSecretVersionsStub + .withArgs("project", apphostingSecret.name) + .resolves([apphostingVersion]); + + const pruned = await secrets.pruneSecrets( + { projectId: "project", projectNumber: "12345" }, + [], + ); + + expect(pruned).to.have.deep.members([secretVersion11, secretVersion21].map(toSecretEnvVar)); + expect(pruned).to.have.length(2); + // Deep-equal the full argument list: calledWith would still pass if a stale + // label filter were being sent as a second argument. + expect(listSecretsStub.firstCall.args).to.deep.equal(["project"]); + expect(listSecretVersionsStub).to.have.callCount(2); + }); + + it("only considers enabled versions", async () => { + listSecretsStub.resolves([secret2]); + listSecretVersionsStub.resolves([secretVersion21]); + + await secrets.pruneSecrets({ projectId: "project", projectNumber: "12345" }, []); + + expect(listSecretVersionsStub.firstCall.args).to.deep.equal([ + "project", + secret2.name, + "state: ENABLED", + ]); + }); + it("returns all secrets given no endpoints", async () => { listSecretsStub.resolves([secret1, secret2]); listSecretVersionsStub.onFirstCall().resolves([secretVersion11, secretVersion12]); @@ -436,6 +500,50 @@ describe("functions/secret", () => { }); }); + describe("destroySecretVersions", () => { + let destroySecretVersionStub: sinon.SinonStub; + + const version1: secrets.SecretForPruning = { + projectId: "project", + key: "MY_SECRET", + secret: "MY_SECRET", + version: "1", + }; + const version2: secrets.SecretForPruning = { ...version1, version: "2" }; + + beforeEach(() => { + destroySecretVersionStub = sinon + .stub(secretManager, "destroySecretVersion") + .rejects("Unexpected call"); + }); + + afterEach(() => { + destroySecretVersionStub.restore(); + }); + + it("destroys every version and reports them", async () => { + destroySecretVersionStub.resolves(); + + await expect(secrets.destroySecretVersions([version1, version2])).to.eventually.deep.equal({ + destroyed: [version1, version2], + erred: [], + }); + expect(destroySecretVersionStub).to.have.been.calledWithExactly("project", "MY_SECRET", "1"); + expect(destroySecretVersionStub).to.have.been.calledWithExactly("project", "MY_SECRET", "2"); + }); + + it("keeps destroying after a failure and reports both outcomes", async () => { + destroySecretVersionStub.withArgs("project", "MY_SECRET", "1").rejects({ message: "boom" }); + destroySecretVersionStub.withArgs("project", "MY_SECRET", "2").resolves(); + + await expect(secrets.destroySecretVersions([version1, version2])).to.eventually.deep.equal({ + destroyed: [version2], + erred: [{ message: "boom" }], + }); + expect(destroySecretVersionStub).to.have.callCount(2); + }); + }); + describe("pruneAndDestroySecrets", () => { let pruneSecretsStub: sinon.SinonStub; let destroySecretVersionStub: sinon.SinonStub; diff --git a/src/functions/secrets.ts b/src/functions/secrets.ts index aa95be09a5c..cb1c212fc1b 100644 --- a/src/functions/secrets.ts +++ b/src/functions/secrets.ts @@ -237,10 +237,11 @@ export async function pruneSecrets( const pruneKey = (name: string, version: string) => `${name}@${version}`; const prunedSecrets: Set = new Set(); - // Collect all Firebase managed secret versions - const haveSecrets = await listSecrets(projectId, `labels.${FIREBASE_MANAGED}=true`); + // A server-side label filter matches one value; isFunctionsManaged accepts both managed values. + const haveSecrets = (await listSecrets(projectId)).filter(isFunctionsManaged); for (const secret of haveSecrets) { - const versions = await listSecretVersions(projectId, secret.name, `NOT state: DESTROYED`); + // Disabled versions are a user's recoverable safety net; only functions:secrets:destroy removes them. + const versions = await listSecretVersions(projectId, secret.name, `state: ENABLED`); for (const version of versions) { prunedSecrets.add(pruneKey(secret.name, version.versionId)); } @@ -287,11 +288,35 @@ export async function pruneSecrets( .map(([secret, version]) => ({ projectId, version, secret, key: secret })); } -type PruneResult = { +export type PruneResult = { destroyed: backend.SecretEnvVar[]; erred: { message: string }[]; }; +/** + * Destroys the given secret versions, continuing past individual failures so callers + * can report exactly which versions were destroyed and which were not. + */ +export async function destroySecretVersions(versions: SecretForPruning[]): Promise { + const destroyed: PruneResult["destroyed"] = []; + const erred: PruneResult["erred"] = []; + const destroyResults = await utils.allSettled( + versions.map(async (sev) => { + await destroySecretVersion(sev.projectId, sev.secret, sev.version); + return sev; + }), + ); + + for (const result of destroyResults) { + if (result.status === "fulfilled") { + destroyed.push(result.value); + } else { + erred.push(result.reason as { message: string }); + } + } + return { destroyed, erred }; +} + /** * Prune and destroy all unused secret versions. Only Firebase managed secrets will be scanned. */ @@ -311,25 +336,9 @@ export async function pruneAndDestroySecrets( return { destroyed: [], erred: [] }; } - const destroyed: PruneResult["destroyed"] = []; - const erred: PruneResult["erred"] = []; const msg = unusedSecrets.map((s) => `${s.secret}@${s.version}`); logger.debug(`Found unused secret versions: ${msg}. Destroying them...`); - const destroyResults = await utils.allSettled( - unusedSecrets.map(async (sev) => { - await destroySecretVersion(sev.projectId, sev.secret, sev.version); - return sev; - }), - ); - - for (const result of destroyResults) { - if (result.status === "fulfilled") { - destroyed.push(result.value); - } else { - erred.push(result.reason as { message: string }); - } - } - return { destroyed, erred }; + return destroySecretVersions(unusedSecrets); } /**