From e790d70fc9078f7db62739914eb7deaadff8e6ad Mon Sep 17 00:00:00 2001 From: notgitika Date: Mon, 24 Aug 2026 15:58:24 -0400 Subject: [PATCH 1/3] fix(project): check the template before letting the Toolkit deploy it Two review findings from #2058, both cases of deploy trusting something it had not checked. A synthesized template with no resources makes the CDK Toolkit *delete* an existing stack of that name and return as though it deployed. #2058 caught that after the fact, by which point the stack was already gone. Check the resource count before handing the assembly to the Toolkit instead. Stack selection matched on the target-name tag alone, never on the account and region the artifact was synthesized for. Those derive from the same target today and so cannot disagree, but nothing enforced it, and the Toolkit deploys where the artifact's environment points rather than where the tag says. Both fields were also being stripped on read, since the manifest schema declared neither. stackArtifactIdForTarget becomes stackArtifactForTarget, returning the template path alongside the id so the resource check needs no second read of the manifest. --- src/core/project/backends/cdk.test.ts | 72 ++++++++-- src/core/project/backends/cdk.ts | 18 +-- .../project/backends/cdk/assembly.test.ts | 135 +++++++++++++++--- src/core/project/backends/cdk/assembly.ts | 120 +++++++++++++++- src/core/project/backends/cdk/toolkit.ts | 3 +- 5 files changed, 311 insertions(+), 37 deletions(-) diff --git a/src/core/project/backends/cdk.test.ts b/src/core/project/backends/cdk.test.ts index 221d4b181..8049f320e 100644 --- a/src/core/project/backends/cdk.test.ts +++ b/src/core/project/backends/cdk.test.ts @@ -49,9 +49,37 @@ async function project(withDependencies = true): Promise { }; } -async function writeAssembly(project: Project, targetNames: string[]): Promise { +type AssemblyOptions = { + /** Overrides the environment every stack artifact is synthesized for. */ + environment?: string; + /** Overrides the resources every stack's template declares. */ + resources?: Record; +}; + +async function writeAssembly( + project: Project, + targetNames: string[], + options: AssemblyOptions = {}, +): Promise { const directory = assemblyDirectory(project); await mkdir(directory, { recursive: true }); + const stacks = targetNames.map((target, index) => ({ + target, + id: `AgentCore-example-${target}-${index}`, + templateFile: `AgentCore-example-${target}-${index}.template.json`, + })); + + await Promise.all( + stacks.map((stack) => + writeFile( + join(directory, stack.templateFile), + JSON.stringify({ + Resources: options.resources ?? { Runtime: { Type: "AWS::BedrockAgentCore::Runtime" } }, + }), + ), + ), + ); + await writeFile( join(directory, "manifest.json"), JSON.stringify({ @@ -59,14 +87,16 @@ async function writeAssembly(project: Project, targetNames: string[]): Promise [ - [ - `AgentCore-example-${target}-${index}`, - { - type: "aws:cloudformation:stack", - properties: { tags: { "agentcore:target-name": target } }, + stacks.map((stack) => [ + stack.id, + { + type: "aws:cloudformation:stack", + environment: options.environment ?? `aws://${TARGET.account}/${TARGET.region}`, + properties: { + templateFile: stack.templateFile, + tags: { "agentcore:target-name": stack.target }, }, - ], + }, ]), ), }, @@ -239,6 +269,32 @@ describe("CdkBackend.deploy", () => { expect(subject.templateLoads()).toBe(0); }); + test("refuses a resource-less stack before the Toolkit can delete it", async () => { + const input = await project(); + await writeAssembly(input, [TARGET.name], { resources: {} }); + const subject = harness(); + + await expect(collectDeploy(subject.backend.deploy(input, { target: TARGET }))).rejects.toThrow( + /declares no resources/, + ); + // Nothing reached the Toolkit, so no stack was deleted and none bootstrapped. + expect(subject.runs).toEqual([]); + expect(subject.bootstrapRegions).toEqual([]); + }); + + test("refuses a stack synthesized for a different region than the target", async () => { + const input = await project(); + await writeAssembly(input, [TARGET.name], { + environment: `aws://${TARGET.account}/us-west-2`, + }); + const subject = harness(); + + await expect(collectDeploy(subject.backend.deploy(input, { target: TARGET }))).rejects.toThrow( + /built for region us-west-2 \(target expects us-east-1\)/, + ); + expect(subject.runs).toEqual([]); + }); + test.each([ ["absent", { kind: "absent" } as const], ["outdated", { kind: "outdated", version: 29 } as const], diff --git a/src/core/project/backends/cdk.ts b/src/core/project/backends/cdk.ts index 334d3d373..d92a49b8c 100644 --- a/src/core/project/backends/cdk.ts +++ b/src/core/project/backends/cdk.ts @@ -11,7 +11,7 @@ import { } from "../../../io"; import type { Logger } from "../../../logging"; import type { DeployBackendInput, ProjectBackend } from "./types"; -import { stackArtifactIdForTarget } from "./cdk/assembly"; +import { assertStackHasResources, stackArtifactForTarget } from "./cdk/assembly"; import { probeBootstrap, resolveAwsAccount, @@ -102,11 +102,13 @@ export class CdkBackend implements ProjectBackend { yield* this.build(project); const assemblyDirectory = this.assemblyDirectory(project); - const stackArtifactId = await stackArtifactIdForTarget( - this.json, - assemblyDirectory, - target.name, - ); + const artifact = await stackArtifactForTarget(this.json, assemblyDirectory, target.name, { + account: target.account, + region: target.region, + }); + // Checked here rather than after the fact: the Toolkit deletes an existing + // stack whose new template has no resources, and returns as if it deployed. + await assertStackHasResources(this.json, assemblyDirectory, artifact); const options = { assemblyDirectory, credentials, region: target.region }; const bootstrap = await this.bootstrap(target.region, credentials); @@ -137,8 +139,8 @@ export class CdkBackend implements ProjectBackend { } } - yield { message: `Deploying ${stackArtifactId}` }; - const outputs = await this.cdk({ kind: "deploy", stackArtifactId }, options); + yield { message: `Deploying ${artifact.id}` }; + const outputs = await this.cdk({ kind: "deploy", stackArtifactId: artifact.id }, options); return { outputs }; } diff --git a/src/core/project/backends/cdk/assembly.test.ts b/src/core/project/backends/cdk/assembly.test.ts index 20180acee..8e2cb7775 100644 --- a/src/core/project/backends/cdk/assembly.test.ts +++ b/src/core/project/backends/cdk/assembly.test.ts @@ -4,11 +4,14 @@ import { join } from "node:path"; import { tmpdir } from "node:os"; import { FsReadWriteJson } from "../../../../io"; import { createSilentLogger } from "../../../../testing"; -import { stackArtifactIdForTarget } from "./assembly"; +import { assertStackHasResources, stackArtifactForTarget } from "./assembly"; const temporaryDirectories: string[] = []; const json = new FsReadWriteJson({ logger: createSilentLogger() }); +const EXPECTED = { account: "111122223333", region: "us-east-1" } as const; +const TEMPLATE_FILE = "stack.template.json"; + afterEach(async () => { await Promise.all( temporaryDirectories @@ -24,31 +27,42 @@ async function assembly(artifacts: Record): Promise { return directory; } -describe("stackArtifactIdForTarget", () => { +/** A stack artifact for `target`, bound to EXPECTED unless `overrides` says otherwise. */ +function stackArtifact(target: string, overrides: Record = {}) { + return { + type: "aws:cloudformation:stack", + environment: `aws://${EXPECTED.account}/${EXPECTED.region}`, + properties: { + tags: { "agentcore:target-name": target }, + templateFile: TEMPLATE_FILE, + }, + ...overrides, + }; +} + +async function writeTemplate(directory: string, resources: Record): Promise { + await writeFile(join(directory, TEMPLATE_FILE), JSON.stringify({ Resources: resources })); +} + +describe("stackArtifactForTarget", () => { test("selects by the target tag instead of deriving a stack name", async () => { - const directory = await assembly({ - "nested/stack-id": { - type: "aws:cloudformation:stack", - properties: { - tags: { "agentcore:target-name": "prod" }, - }, - }, - }); + const directory = await assembly({ "nested/stack-id": stackArtifact("prod") }); - expect(await stackArtifactIdForTarget(json, directory, "prod")).toBe("nested/stack-id"); + expect(await stackArtifactForTarget(json, directory, "prod", EXPECTED)).toEqual({ + id: "nested/stack-id", + templateFile: TEMPLATE_FILE, + }); }); test("ignores non-stack artifacts", async () => { const directory = await assembly({ Tree: { type: "cdk:tree", - properties: { - tags: { "agentcore:target-name": "prod" }, - }, + properties: { tags: { "agentcore:target-name": "prod" } }, }, }); - await expect(stackArtifactIdForTarget(json, directory, "prod")).rejects.toThrow( + await expect(stackArtifactForTarget(json, directory, "prod", EXPECTED)).rejects.toThrow( /defines 0 stack/, ); }); @@ -58,8 +72,97 @@ describe("stackArtifactIdForTarget", () => { temporaryDirectories.push(directory); await mkdir(directory, { recursive: true }); - await expect(stackArtifactIdForTarget(json, directory, "prod")).rejects.toThrow( + await expect(stackArtifactForTarget(json, directory, "prod", EXPECTED)).rejects.toThrow( /No synthesized cloud assembly was found/, ); }); + + test("rejects a stack tagged for the target but built for another account", async () => { + const directory = await assembly({ + Stack: stackArtifact("prod", { environment: `aws://999988887777/${EXPECTED.region}` }), + }); + + await expect(stackArtifactForTarget(json, directory, "prod", EXPECTED)).rejects.toThrow( + /built for account 999988887777 \(target expects 111122223333\)/, + ); + }); + + test("rejects a stack tagged for the target but built for another region", async () => { + const directory = await assembly({ + Stack: stackArtifact("prod", { environment: `aws://${EXPECTED.account}/eu-west-1` }), + }); + + await expect(stackArtifactForTarget(json, directory, "prod", EXPECTED)).rejects.toThrow( + /built for region eu-west-1 \(target expects us-east-1\)/, + ); + }); + + test("names both halves when neither account nor region matches", async () => { + const directory = await assembly({ + Stack: stackArtifact("prod", { environment: "aws://999988887777/eu-west-1" }), + }); + + await expect(stackArtifactForTarget(json, directory, "prod", EXPECTED)).rejects.toThrow( + /account 999988887777 \(target expects 111122223333\) and region eu-west-1/, + ); + }); + + test.each([ + ["no environment at all", undefined], + ["CDK's unknown-* placeholders", "aws://unknown-account/unknown-region"], + ])("accepts an environment-agnostic stack with %s", async (_label, environment) => { + const directory = await assembly({ + // An absent key and an explicit undefined both mean "agnostic" once + // serialized, which is what the manifest on disk actually looks like. + Stack: stackArtifact("prod", { environment }), + }); + + expect((await stackArtifactForTarget(json, directory, "prod", EXPECTED)).id).toBe("Stack"); + }); + + test("rejects an environment it cannot parse rather than assuming a match", async () => { + const directory = await assembly({ + Stack: stackArtifact("prod", { environment: "us-east-1" }), + }); + + await expect(stackArtifactForTarget(json, directory, "prod", EXPECTED)).rejects.toThrow( + /unrecognized environment 'us-east-1'/, + ); + }); +}); + +describe("assertStackHasResources", () => { + test("accepts a template that declares resources", async () => { + const directory = await assembly({ Stack: stackArtifact("prod") }); + await writeTemplate(directory, { Runtime: { Type: "AWS::BedrockAgentCore::Runtime" } }); + + expect( + await assertStackHasResources(json, directory, { id: "Stack", templateFile: TEMPLATE_FILE }), + ).toBeUndefined(); + }); + + test("rejects a resource-less template, which the Toolkit reads as a deletion", async () => { + const directory = await assembly({ Stack: stackArtifact("prod") }); + await writeTemplate(directory, {}); + + await expect( + assertStackHasResources(json, directory, { id: "Stack", templateFile: TEMPLATE_FILE }), + ).rejects.toThrow(/declares no resources, so deploying it would delete the existing stack/); + }); + + test("rejects a template the assembly names but does not contain", async () => { + const directory = await assembly({ Stack: stackArtifact("prod") }); + + await expect( + assertStackHasResources(json, directory, { id: "Stack", templateFile: TEMPLATE_FILE }), + ).rejects.toThrow(/synthesized template for stack 'Stack' is missing/); + }); + + test("rejects a stack artifact that names no template at all", async () => { + const directory = await assembly({ Stack: stackArtifact("prod") }); + + await expect( + assertStackHasResources(json, directory, { id: "Stack", templateFile: undefined }), + ).rejects.toThrow(/names no template file/); + }); }); diff --git a/src/core/project/backends/cdk/assembly.ts b/src/core/project/backends/cdk/assembly.ts index 4077da573..288a36d10 100644 --- a/src/core/project/backends/cdk/assembly.ts +++ b/src/core/project/backends/cdk/assembly.ts @@ -6,6 +6,9 @@ import type { ReadWriteJson } from "../../../../io"; const TARGET_TAG = "agentcore:target-name"; const STACK_ARTIFACT = "aws:cloudformation:stack"; +/** What CDK writes in an artifact's `environment` when the stack is env-agnostic. */ +const UNKNOWN_ACCOUNT = "unknown-account"; +const UNKNOWN_REGION = "unknown-region"; const AssemblyManifestSchema = z.object({ artifacts: z @@ -13,9 +16,11 @@ const AssemblyManifestSchema = z.object({ z.string(), z.object({ type: z.string(), + environment: z.string().optional(), properties: z .object({ tags: z.record(z.string(), z.string()).optional(), + templateFile: z.string().optional(), }) .optional(), }), @@ -23,12 +28,36 @@ const AssemblyManifestSchema = z.object({ .default({}), }); -/** Finds the one synthesized stack artifact tagged for the selected deployment target. */ -export async function stackArtifactIdForTarget( +const StackTemplateSchema = z + .object({ + Resources: z.record(z.string(), z.unknown()).default({}), + }) + .passthrough(); + +/** The synthesized stack a deploy selected, and where its template lives. */ +export interface StackArtifact { + /** Artifact id the CDK Toolkit selects the stack by. */ + id: string; + /** Assembly-relative path of the synthesized template. */ + templateFile: string | undefined; +} + +/** The account and region a deploy expects its stack to be bound to. */ +export interface StackEnvironment { + account: string; + region: string; +} + +/** + * Finds the one synthesized stack artifact tagged for the selected deployment + * target, and checks it is bound to the environment that target names. + */ +export async function stackArtifactForTarget( json: ReadWriteJson, assemblyDirectory: string, target: string, -): Promise { + expected: StackEnvironment, +): Promise { const manifestPath = join(assemblyDirectory, "manifest.json"); if (!existsSync(manifestPath)) { throw new ProjectStateError(`No synthesized cloud assembly was found at ${manifestPath}.`); @@ -54,5 +83,88 @@ export async function stackArtifactIdForTarget( `'${target}'. Exactly one stack must be tagged ${TARGET_TAG}='${target}'.`, ); } - return matches[0]![0]; + + const [id, artifact] = matches[0]!; + assertEnvironmentMatches(id, artifact.environment, expected); + return { id, templateFile: artifact.properties?.templateFile }; +} + +/** + * Refuses to deploy a synthesized template that declares no resources. + * + * The CDK Toolkit reads such a template as an instruction to *delete* an + * existing stack of that name, and reports the run as a normal success. Without + * this check `project deploy` is the only way to destroy a deployed stack, and + * there is no `project destroy` for a user to have asked for it with. + */ +export async function assertStackHasResources( + json: ReadWriteJson, + assemblyDirectory: string, + artifact: StackArtifact, +): Promise { + // The cloud assembly schema requires templateFile on a stack artifact, so its + // absence is a malformed assembly rather than a stack to deploy unchecked. + if (artifact.templateFile === undefined) { + throw new ProjectStateError( + `Stack artifact '${artifact.id}' names no template file in the cloud assembly manifest.`, + ); + } + + const templatePath = join(assemblyDirectory, artifact.templateFile); + if (!existsSync(templatePath)) { + throw new ProjectStateError( + `The synthesized template for stack '${artifact.id}' is missing from the cloud ` + + `assembly at ${templatePath}.`, + ); + } + + const template = await json.read(templatePath, StackTemplateSchema); + if (Object.keys(template.Resources).length === 0) { + throw new ProjectStateError( + `The synthesized stack '${artifact.id}' declares no resources, so deploying it would ` + + `delete the existing stack rather than update it. Check that the project spec still ` + + `declares the runtimes, gateways and memories it should before deploying.`, + ); + } +} + +// Both the target tag and the stack's environment derive from the same target in +// the synthesized app, so today they cannot disagree. Checking anyway keeps a +// correct tag from carrying a stack into the wrong account or region: the Toolkit +// deploys where the artifact's environment points, not where the tag says. +function assertEnvironmentMatches( + id: string, + environment: string | undefined, + expected: StackEnvironment, +): void { + // An artifact with no environment is environment-agnostic: it deploys into + // whatever the credentials resolve to, which the account preflight checked. + if (environment === undefined) return; + + const parsed = /^aws:\/\/([^/]+)\/(.+)$/.exec(environment); + if (!parsed) { + throw new ProjectStateError( + `Stack artifact '${id}' declares an unrecognized environment '${environment}'. ` + + `Expected the form aws:///.`, + ); + } + + const account = parsed[1]!; + const region = parsed[2]!; + // The unknown-* placeholders are the env-agnostic case spelled out. + const mismatches = [ + account !== UNKNOWN_ACCOUNT && account !== expected.account + ? `account ${account} (target expects ${expected.account})` + : undefined, + region !== UNKNOWN_REGION && region !== expected.region + ? `region ${region} (target expects ${expected.region})` + : undefined, + ].filter((mismatch) => mismatch !== undefined); + + if (mismatches.length > 0) { + throw new ProjectStateError( + `The synthesized stack '${id}' is built for ${mismatches.join(" and ")}. ` + + `Re-synthesize the project so its stack matches the deployment target.`, + ); + } } diff --git a/src/core/project/backends/cdk/toolkit.ts b/src/core/project/backends/cdk/toolkit.ts index 243427c14..0d259c20c 100644 --- a/src/core/project/backends/cdk/toolkit.ts +++ b/src/core/project/backends/cdk/toolkit.ts @@ -192,7 +192,8 @@ export async function performCdkOperation( // than one stack, so a missing result is not "no match": the Toolkit skips a // stack whose template has no resources, and *deletes* it if it already // exists. Both return normally, so reporting empty outputs here would call a - // deletion a successful deploy. + // deletion a successful deploy. assertStackHasResources rejects the known way + // into that state before we get here; this stays as the backstop for any other. if (result.stacks.length !== 1) { throw new AgentCoreCLIError( `The CDK Toolkit deployed no stack for '${operation.stackArtifactId}'. ` + From dfb7780ddcf3205e0f2a45d52a5024f17b43472a Mon Sep 17 00:00:00 2001 From: notgitika Date: Mon, 24 Aug 2026 16:37:25 -0400 Subject: [PATCH 2/3] docs(project): note the dropped-credentials gap deploy still has Credentials declared in agentcore.json are not provisioned by `project deploy` on this branch: the synthesized app reads their provider ARNs out of .cli/deployed-state.json and tolerates the file's absence, and nothing writes it. Points at #2093 rather than working around it here. Credential providers are supported by CloudFormation, so the fix belongs in the synthesized stack, not in an imperative pre-synth step in the CLI. --- src/core/project/backends/cdk.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/project/backends/cdk.ts b/src/core/project/backends/cdk.ts index d92a49b8c..2636d2101 100644 --- a/src/core/project/backends/cdk.ts +++ b/src/core/project/backends/cdk.ts @@ -100,6 +100,10 @@ export class CdkBackend implements ProjectBackend { ); } + // TODO(#2093): credentials declared in agentcore.json are silently dropped. + // The synthesized app reads their provider ARNs out of .cli/deployed-state.json + // and tolerates the file's absence, and nothing here writes it. The fix is to + // let CloudFormation own the providers rather than creating them from the CLI. yield* this.build(project); const assemblyDirectory = this.assemblyDirectory(project); const artifact = await stackArtifactForTarget(this.json, assemblyDirectory, target.name, { From 378208a1be35283edc1f95b7a550a9179e5c223e Mon Sep 17 00:00:00 2001 From: notgitika Date: Mon, 24 Aug 2026 17:47:55 -0400 Subject: [PATCH 3/3] feat(project): deploy credential providers through CloudFormation `agentcore project deploy` created credential providers imperatively over the Identity API before synth, so the CLI owned resources CloudFormation did not know about and a rolled-back stack left them orphaned. The stack now declares them. Real secret material still never reaches the template: a credential carrying an external Secrets Manager reference deploys pointing at that secret, and one whose secret lives in `.env.local` is created with the L3's placeholder, which the CLI replaces over the Identity API once the deploy succeeds. - `assertCredentialsDeployable` runs before synth. A missing `.env.local` variable fails there, naming the variable and the file, rather than after a provider is already live holding a placeholder no later run would replace. - `createCredentialSynchronizer` runs after the deploy, because the providers do not exist until CloudFormation makes them. It updates every provider whose secret is inline on every deploy: the placeholder cannot be told apart from a real key by reading the provider back, since `ApiKey` is a write-only CloudFormation property and `GetApiKeyCredentialProvider` returns only the secret's ARN. - A `PaymentCredentialProvider` is refused up front. CloudFormation has no payment credential provider resource, and a payment provider needs vendor configuration `agentcore.json` has no fields for. - Payment connectors now carry the credential's *name*; the stack resolves it to the ARN of the provider it created. That drops the last reader of `deployed-state.json` from the vended CDK app. Both passes share one `resolveSecret`, so the preflight cannot disagree with the sync about which credentials need a secret pushed. --- src/assets/cdk/bin/cdk.ts | 43 +-- src/assets/cdk/lib/cdk-stack.ts | 122 +++++++- src/core/project/backends/cdk.test.ts | 86 +++++ src/core/project/backends/cdk.ts | 30 +- .../project/backends/cdk/credentials.test.ts | 294 ++++++++++++++++++ src/core/project/backends/cdk/credentials.ts | 292 +++++++++++++++++ src/core/project/envLocal.test.ts | 26 +- src/core/project/envLocal.ts | 27 ++ .../project/add/credentials/oauth/index.ts | 9 +- .../project/add/credentials/shared.ts | 8 +- 10 files changed, 882 insertions(+), 55 deletions(-) create mode 100644 src/core/project/backends/cdk/credentials.test.ts create mode 100644 src/core/project/backends/cdk/credentials.ts diff --git a/src/assets/cdk/bin/cdk.ts b/src/assets/cdk/bin/cdk.ts index 83e54bb4a..b41cf3e27 100644 --- a/src/assets/cdk/bin/cdk.ts +++ b/src/assets/cdk/bin/cdk.ts @@ -124,14 +124,6 @@ async function main() { const connectorParametersByFile = resolveConnectorParametersByFile(specAny, projectRoot); const harnessConfigs = resolveHarnessConfigs(specAny, projectRoot); - // Read deployed state for credential ARNs (populated by pre-deploy identity setup) - let deployedState: Record | undefined; - try { - deployedState = JSON.parse(fs.readFileSync(path.join(configRoot, '.cli', 'deployed-state.json'), 'utf8')); - } catch { - // Deployed state may not exist on first deploy - } - const app = new App(); for (const target of stackTargets) { @@ -140,20 +132,6 @@ async function main() { const env = target ? toEnvironment(target) : undefined; const stackName = target ? toStackName(spec.name, target.name) : `AgentCore-${sanitize(spec.name)}`; - // Extract credentials from deployed state for this target - const targetState = (deployedState as Record)?.targets as - | Record> - | undefined; - const targetResources = target - ? (targetState?.[target.name]?.resources as Record | undefined) - : undefined; - const credentials = targetResources?.credentials as - | Record - | undefined; - - // Payment credential provider ARNs live in the same credentials map as identity credentials - const paymentCredentials = credentials; - const paymentSpec = specAny.payments?.length ? specAny.payments.map( (p: { @@ -173,19 +151,13 @@ async function main() { autoPayment: p.autoPayment, paymentToolAllowlist: p.paymentToolAllowlist, networkPreferences: p.networkPreferences, - connectors: p.connectors.map(c => { - const credentialProviderArn = paymentCredentials?.[c.credentialName]?.credentialProviderArn; - if (!credentialProviderArn) { - // Fail fast with an actionable message rather than passing an empty - // ARN that fails opaquely server-side at CreatePaymentConnector. - throw new Error( - `Payment connector "${c.name}" on manager "${p.name}" references credential ` + - `"${c.credentialName}", but no deployed credential provider was found for it. ` + - `Run \`agentcore deploy\` so the credential provider is created first.` - ); - } - return { name: c.name, provider: c.provider, credentialProviderArn }; - }), + // The stack creates the credential providers, so a connector carries + // the credential's name and the stack resolves it to that provider's ARN. + connectors: p.connectors.map(c => ({ + name: c.name, + provider: c.provider, + credentialName: c.credentialName, + })), }) ) : undefined; @@ -193,7 +165,6 @@ async function main() { new AgentCoreStack(app, stackName, { spec, mcpSpec, - credentials, connectorParametersByFile, harnesses: harnessConfigs.length > 0 ? harnessConfigs : undefined, paymentSpec, diff --git a/src/assets/cdk/lib/cdk-stack.ts b/src/assets/cdk/lib/cdk-stack.ts index 3dac0669d..d2c978869 100644 --- a/src/assets/cdk/lib/cdk-stack.ts +++ b/src/assets/cdk/lib/cdk-stack.ts @@ -1,6 +1,8 @@ import { + AgentCoreApiKeyCredentialProvider, AgentCoreApplication, AgentCoreMcp, + AgentCoreOauth2CredentialProvider, AgentCorePaymentManager, AgentCorePaymentConnector, type AgentCoreProjectSpec, @@ -22,7 +24,39 @@ export type HarnessConfig = HarnessDeploymentConfig; export interface PaymentConnectorSpec { name: string; provider: 'CoinbaseCDP' | 'StripePrivy'; - credentialProviderArn: string; + /** + * Name of the credential this connector authorizes with. Resolved to the ARN of + * the provider this stack creates, so the connector depends on it rather than + * on a provider that had to exist before synth. + */ + credentialName: string; +} + +/** A reference to a secret the customer already keeps in AWS Secrets Manager. */ +interface CredentialSecretRef { + secretId: string; + jsonKey: string; +} + +/** + * The credential fields this stack reads off the project spec. The published + * @aws/agentcore-cdk spec type can lag the CLI's own schema, so these are read + * from a local shape the same way bin/cdk.ts reads not-yet-published fields. + */ +interface CredentialDeclaration { + authorizerType: + | 'ApiKeyCredentialProvider' + | 'OAuthCredentialProvider' + | 'PaymentCredentialProvider'; + name: string; + /** API key credentials: external secret for the key itself. */ + secretRef?: CredentialSecretRef; + /** OAuth credentials: external secret for the client secret. */ + clientSecretRef?: CredentialSecretRef; + vendor?: string; + clientId?: string; + discoveryUrl?: string; + providerConfig?: Record; } export interface PaymentSpec { @@ -45,10 +79,6 @@ export interface AgentCoreStackProps extends StackProps { * The MCP specification containing gateways and servers. */ mcpSpec?: AgentCoreMcpSpec; - /** - * Credential provider ARNs from deployed state, keyed by credential name. - */ - credentials?: Record; /** * Harness role configurations. */ @@ -97,7 +127,12 @@ export class AgentCoreStack extends Stack { constructor(scope: Construct, id: string, props: AgentCoreStackProps) { super(scope, id, props); - const { spec, mcpSpec, credentials, harnesses, connectorParametersByFile, paymentSpec } = props; + const { spec, mcpSpec, harnesses, connectorParametersByFile, paymentSpec } = props; + + // Create the credential providers the spec declares, before anything that + // consumes them. CloudFormation owns their lifecycle; the ARNs below are + // synth-time tokens, so no provider has to exist before this stack deploys. + const credentials = this.createCredentialProviders(spec); // Create AgentCoreApplication with all agents and harness roles // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -108,7 +143,7 @@ export class AgentCoreStack extends Stack { if (connectorParametersByFile && Object.keys(connectorParametersByFile).length > 0) { appProps.connectorParametersByFile = connectorParametersByFile; } - if (credentials) { + if (Object.keys(credentials).length > 0) { appProps.credentials = credentials; } this.application = new AgentCoreApplication(this, 'Application', appProps as any); @@ -203,12 +238,25 @@ export class AgentCoreStack extends Stack { // Create connectors for this manager for (const connector of payment.connectors) { const connId = toCdkId(connector.name); + const credential = credentials[connector.credentialName]; + if (!credential) { + // The spec cross-validates that this name is a declared credential, + // so reaching here means the credential exists but no provider was + // created for it — today only a PaymentCredentialProvider, which + // CloudFormation cannot create. + throw new Error( + `Payment connector "${connector.name}" on manager "${payment.name}" references ` + + `credential "${connector.credentialName}", which this stack cannot create a ` + + `credential provider for. CloudFormation has no payment credential provider ` + + `resource; remove the connector to deploy the rest of the project.` + ); + } const conn = new AgentCorePaymentConnector(this, `Payment${mgrId}${connId}`, { projectName: spec.name, paymentManager: manager, connectorName: connector.name, connectorType: connector.provider, - credentialProviderArn: connector.credentialProviderArn, + credentialProviderArn: credential.credentialProviderArn, }); // Wire first connector's ID as env var (eligible agents only) @@ -246,4 +294,62 @@ export class AgentCoreStack extends Stack { value: this.stackName, }); } + + /** + * Creates a credential provider for every credential the spec declares, and + * returns their ARNs keyed by credential name for the constructs that consume + * them. + * + * Real secret material never reaches the template. A credential carrying an + * external Secrets Manager reference deploys pointing at that secret; one whose + * secret lives in `.env.local` is created with the L3's placeholder, which + * `agentcore project deploy` replaces over the Identity API once the stack is + * up. Payment credentials get no provider — CloudFormation has no resource for + * them — so they are absent from the map and any connector naming one fails. + */ + private createCredentialProviders( + spec: AgentCoreProjectSpec + ): Record { + const declared = (spec.credentials ?? []) as CredentialDeclaration[]; + const created: Record = {}; + + for (const credential of declared) { + const id = `Credential${toCdkId(credential.name)}`; + let credentialProviderArn: string; + + switch (credential.authorizerType) { + case 'ApiKeyCredentialProvider': + credentialProviderArn = new AgentCoreApiKeyCredentialProvider(this, id, { + projectName: spec.name, + name: credential.name, + ...(credential.secretRef && { secretRef: credential.secretRef }), + projectTags: spec.tags, + }).credentialProviderArn; + break; + case 'OAuthCredentialProvider': + credentialProviderArn = new AgentCoreOauth2CredentialProvider(this, id, { + projectName: spec.name, + name: credential.name, + vendor: credential.vendor ?? 'CustomOauth2', + ...(credential.clientId !== undefined && { clientId: credential.clientId }), + ...(credential.discoveryUrl !== undefined && { discoveryUrl: credential.discoveryUrl }), + ...(credential.providerConfig && { providerConfig: credential.providerConfig }), + // The spec names the OAuth external reference clientSecretRef; the + // construct takes one secretRef whichever credential kind it is on. + ...(credential.clientSecretRef && { secretRef: credential.clientSecretRef }), + projectTags: spec.tags, + }).credentialProviderArn; + break; + case 'PaymentCredentialProvider': + // Deliberately left out of the map rather than thrown on here: a + // payment credential with no connector referencing it is inert, and + // `agentcore project deploy` rejects the project before synth anyway. + continue; + } + + created[credential.name] = { credentialProviderArn }; + } + + return created; + } } diff --git a/src/core/project/backends/cdk.test.ts b/src/core/project/backends/cdk.test.ts index 8049f320e..83e727661 100644 --- a/src/core/project/backends/cdk.test.ts +++ b/src/core/project/backends/cdk.test.ts @@ -111,11 +111,19 @@ type HarnessOptions = { template?: boolean; failOperation?: CdkOperation["kind"]; bootstrapError?: Error; + /** Rejection from the credential preflight that runs before anything else. */ + preflightError?: Error; + /** Events the credential secret sync emits once the stack is deployed. */ + syncEvents?: ProjectEvent[]; }; function harness(options: HarnessOptions = {}) { const commands: { command: string[]; cwd: string }[] = []; const runs: { operation: CdkOperation; options: CdkRunOptions }[] = []; + const preflights: Project[] = []; + const syncs: { project: Project; region: string; credentials: CdkCredentialProvider }[] = []; + /** Every side effect in deploy order, so ordering is asserted rather than assumed. */ + const sequence: string[] = []; const credentialRegions: string[] = []; const accountCredentials: CdkCredentialProvider[] = []; const bootstrapCredentials: CdkCredentialProvider[] = []; @@ -131,9 +139,20 @@ function harness(options: HarnessOptions = {}) { const backend = new CdkBackend({ logger: createSilentLogger(), runner: async (command, { cwd }) => { + sequence.push("synth"); commands.push({ command, cwd }); }, checkTool: async () => {}, + assertCredentials: async (input) => { + sequence.push("preflight"); + preflights.push(input); + if (options.preflightError) throw options.preflightError; + }, + syncCredentials: async function* (input, { region, credentials: provider }) { + sequence.push("sync"); + syncs.push({ project: input, region, credentials: provider }); + yield* options.syncEvents ?? []; + }, resolveCredentials: async (region) => { credentialRegions.push(region); return credentials; @@ -150,6 +169,7 @@ function harness(options: HarnessOptions = {}) { return options.bootstrap ?? { kind: "current", version: 30 }; }, cdk: async (operation, runOptions) => { + sequence.push(operation.kind); runs.push({ operation, options: runOptions }); if (operation.kind === options.failOperation) { throw new Error(`${operation.kind} failed`); @@ -177,7 +197,10 @@ function harness(options: HarnessOptions = {}) { commands, credentialRegions, credentials, + preflights, runs, + sequence, + syncs, templateLoads: () => templateLoads, templateCleanups: () => templateCleanups, }; @@ -269,6 +292,69 @@ describe("CdkBackend.deploy", () => { expect(subject.templateLoads()).toBe(0); }); + test("preflights credentials first and syncs their secrets last", async () => { + const input = await project(); + await writeAssembly(input, [TARGET.name]); + const subject = harness({ bootstrap: { kind: "absent" } }); + + await collectDeploy(subject.backend.deploy(input, { target: TARGET })); + + // The providers do not exist until CloudFormation makes them, so the secret + // sync has to follow the deploy; a credential with no secret to sync has to + // fail before the synth that would create its provider. + expect(subject.sequence).toEqual(["preflight", "synth", "bootstrap", "deploy", "sync"]); + expect(subject.preflights).toEqual([input]); + expect(subject.syncs).toEqual([ + { project: input, region: TARGET.region, credentials: subject.credentials }, + ]); + }); + + test("surfaces the secret sync's progress after the deploy message", async () => { + const input = await project(); + await writeAssembly(input, [TARGET.name]); + const subject = harness({ + syncEvents: [{ message: "Setting the secret for credential provider 'openai-key'" }], + }); + + const deployed = await collectDeploy(subject.backend.deploy(input, { target: TARGET })); + + expect(deployed.events).toEqual([ + { message: `Verifying AWS account ${TARGET.account}` }, + { message: "Synthesizing CloudFormation templates" }, + { message: "Deploying AgentCore-example-default-0" }, + { message: "Setting the secret for credential provider 'openai-key'" }, + ]); + }); + + test("refuses a credential it cannot deploy before synthesizing or bootstrapping", async () => { + const input = await project(); + await writeAssembly(input, [TARGET.name]); + const subject = harness({ + preflightError: new Error("Credential 'openai-key' has no secret"), + }); + + await expect(collectDeploy(subject.backend.deploy(input, { target: TARGET }))).rejects.toThrow( + "Credential 'openai-key' has no secret", + ); + // Nothing was synthesized, bootstrapped or deployed, so no provider exists + // holding a placeholder no later run would replace. + expect(subject.commands).toEqual([]); + expect(subject.bootstrapRegions).toEqual([]); + expect(subject.runs).toEqual([]); + expect(subject.syncs).toEqual([]); + }); + + test("does not sync secrets when the deploy itself fails", async () => { + const input = await project(); + await writeAssembly(input, [TARGET.name]); + const subject = harness({ failOperation: "deploy" }); + + await expect(collectDeploy(subject.backend.deploy(input, { target: TARGET }))).rejects.toThrow( + "deploy failed", + ); + expect(subject.syncs).toEqual([]); + }); + test("refuses a resource-less stack before the Toolkit can delete it", async () => { const input = await project(); await writeAssembly(input, [TARGET.name], { resources: {} }); diff --git a/src/core/project/backends/cdk.ts b/src/core/project/backends/cdk.ts index 2636d2101..1cb2d0842 100644 --- a/src/core/project/backends/cdk.ts +++ b/src/core/project/backends/cdk.ts @@ -12,6 +12,12 @@ import { import type { Logger } from "../../../logging"; import type { DeployBackendInput, ProjectBackend } from "./types"; import { assertStackHasResources, stackArtifactForTarget } from "./cdk/assembly"; +import { + assertCredentialsDeployable, + createCredentialSynchronizer, + type CredentialPreflight, + type CredentialSynchronizer, +} from "./cdk/credentials"; import { probeBootstrap, resolveAwsAccount, @@ -37,6 +43,8 @@ export type CdkBackendConfig = { bootstrap?: BootstrapProbe; resolveAccount?: AccountResolver; loadBootstrapTemplate?: BootstrapTemplateLoader; + assertCredentials?: CredentialPreflight; + syncCredentials?: CredentialSynchronizer; }; /** Builds and deploys projects through the scaffolded CDK app. */ @@ -50,6 +58,8 @@ export class CdkBackend implements ProjectBackend { private readonly bootstrap: BootstrapProbe; private readonly resolveAccount: AccountResolver; private readonly loadBootstrapTemplate: BootstrapTemplateLoader; + private readonly assertCredentials: CredentialPreflight; + private readonly syncCredentials: CredentialSynchronizer; constructor(config: CdkBackendConfig) { this.logger = config.logger; @@ -62,6 +72,8 @@ export class CdkBackend implements ProjectBackend { this.bootstrap = config.bootstrap ?? probeBootstrap; this.resolveAccount = config.resolveAccount ?? resolveAwsAccount; this.loadBootstrapTemplate = config.loadBootstrapTemplate ?? loadBootstrapTemplate; + this.assertCredentials = config.assertCredentials ?? assertCredentialsDeployable; + this.syncCredentials = config.syncCredentials ?? createCredentialSynchronizer(); } public async *build(project: Project): AsyncGenerator { @@ -100,10 +112,12 @@ export class CdkBackend implements ProjectBackend { ); } - // TODO(#2093): credentials declared in agentcore.json are silently dropped. - // The synthesized app reads their provider ARNs out of .cli/deployed-state.json - // and tolerates the file's absence, and nothing here writes it. The fix is to - // let CloudFormation own the providers rather than creating them from the CLI. + // Before anything is synthesized or bootstrapped: the stack creates each + // credential provider with a placeholder secret that only the post-deploy + // sync below can replace, so a credential with no secret to sync has to fail + // here rather than after its provider is already live. + await this.assertCredentials(project); + yield* this.build(project); const assemblyDirectory = this.assemblyDirectory(project); const artifact = await stackArtifactForTarget(this.json, assemblyDirectory, target.name, { @@ -145,6 +159,14 @@ export class CdkBackend implements ProjectBackend { yield { message: `Deploying ${artifact.id}` }; const outputs = await this.cdk({ kind: "deploy", stackArtifactId: artifact.id }, options); + + // The credential providers exist only now that CloudFormation has made them, + // each holding the placeholder secret its template carried. Replacing those + // is what keeps real secret material out of the template. A failure here + // leaves a correctly deployed stack whose providers still hold placeholders; + // the next deploy retries the sync. + yield* this.syncCredentials(project, { region: target.region, credentials }); + return { outputs }; } diff --git a/src/core/project/backends/cdk/credentials.test.ts b/src/core/project/backends/cdk/credentials.test.ts new file mode 100644 index 000000000..5a326d82a --- /dev/null +++ b/src/core/project/backends/cdk/credentials.test.ts @@ -0,0 +1,294 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import type { Oauth2ProviderConfigInput } from "@aws-sdk/client-bedrock-agentcore-control"; +import type { Project, ProjectEvent } from "../../../../handlers/project/types"; +import { ProjectSpecSchema } from "../../../../projectSchemas/project"; +import { ENV_LOCAL_RELATIVE_PATH } from "../../envLocal"; +import { + assertCredentialsDeployable, + createCredentialSynchronizer, + type IdentitySecretClientFactory, +} from "./credentials"; +import type { CdkCredentialProvider } from "./toolkit"; + +const REGION = "us-east-1"; +const SECRET_REF = { secretId: "arn:aws:secretsmanager:us-east-1:1:secret:s", jsonKey: "key" }; +const DISCOVERY_URL = "https://idp.example.com/.well-known/openid-configuration"; + +const credentials: CdkCredentialProvider = async () => ({ + accessKeyId: "access-key", + secretAccessKey: "secret-key", +}); + +const tempDirectories: string[] = []; + +afterEach(async () => { + await Promise.all( + tempDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true })), + ); +}); + +/** A project declaring `declared`, with `env` written to its `.env.local`. */ +async function project(declared: unknown[], env: Record = {}): Promise { + const rootPath = await mkdtemp(join(tmpdir(), "agentcore-credentials-")); + tempDirectories.push(rootPath); + const entries = Object.entries(env); + if (entries.length > 0) { + await mkdir(join(rootPath, "agentcore"), { recursive: true }); + await writeFile( + join(rootPath, ENV_LOCAL_RELATIVE_PATH), + entries.map(([key, value]) => `${key}=${value}`).join("\n"), + ); + } + return { + name: "example", + rootPath, + spec: ProjectSpecSchema.parse({ name: "example", version: 1, credentials: declared }), + }; +} + +function apiKey(name: string, overrides: Record = {}) { + return { authorizerType: "ApiKeyCredentialProvider", name, ...overrides }; +} + +function oauth(name: string, overrides: Record = {}) { + return { + authorizerType: "OAuthCredentialProvider", + name, + discoveryUrl: DISCOVERY_URL, + ...overrides, + }; +} + +function recorder() { + const apiKeys: { name: string; apiKey: string }[] = []; + const oauth2: { name: string; vendor: string; config: Oauth2ProviderConfigInput }[] = []; + const clients: { region: string; credentials: CdkCredentialProvider }[] = []; + + const factory: IdentitySecretClientFactory = async (region, provider) => { + clients.push({ region, credentials: provider }); + return { + async setApiKey(name, key) { + apiKeys.push({ name, apiKey: key }); + }, + async setOauth2ClientSecret(name, vendor, config) { + oauth2.push({ name, vendor, config }); + }, + }; + }; + + return { apiKeys, clients, factory, oauth2 }; +} + +async function sync(input: Project, factory: IdentitySecretClientFactory): Promise { + const events: ProjectEvent[] = []; + for await (const event of createCredentialSynchronizer(factory)(input, { + region: REGION, + credentials, + })) { + events.push(event); + } + return events; +} + +describe("assertCredentialsDeployable", () => { + test("accepts a project that declares no credentials", async () => { + expect(await assertCredentialsDeployable(await project([]))).toBeUndefined(); + }); + + test("rejects a payment credential, naming the missing CloudFormation resource", async () => { + const input = await project([ + { authorizerType: "PaymentCredentialProvider", name: "pay-1", provider: "CoinbaseCDP" }, + ]); + + await expect(assertCredentialsDeployable(input)).rejects.toThrow( + /CloudFormation has no payment credential provider resource/, + ); + }); + + test.each([ + ["ready", { AGENTCORE_CREDENTIAL_OPENAI_KEY: "sk-live" }], + ["itself missing a secret", {}], + ])( + "blames the payment credential when the project's other credential is %s", + async (_label, env) => { + const input = await project( + [ + apiKey("openai-key"), + { authorizerType: "PaymentCredentialProvider", name: "pay-1", provider: "CoinbaseCDP" }, + ], + env, + ); + + // The unsupported credential is the reason this project cannot deploy, so + // it is what the error names either way — not whichever one comes first. + await expect(assertCredentialsDeployable(input)).rejects.toThrow(/PaymentCredentialProvider/); + }, + ); + + test("names the variable and the file when an API key has no secret", async () => { + const input = await project([apiKey("openai-key")]); + + await expect(assertCredentialsDeployable(input)).rejects.toThrow( + new RegExp( + `Set AGENTCORE_CREDENTIAL_OPENAI_KEY in ${join(input.rootPath, ENV_LOCAL_RELATIVE_PATH)}`, + ), + ); + }); + + test("names the client-secret variable when an OAuth credential has no secret", async () => { + const input = await project([oauth("my-idp")]); + + await expect(assertCredentialsDeployable(input)).rejects.toThrow( + /Set AGENTCORE_CREDENTIAL_MY_IDP_CLIENT_SECRET in .*clientSecretRef/s, + ); + }); + + test("accepts credentials whose secrets are in .env.local", async () => { + const input = await project([apiKey("openai-key"), oauth("my-idp")], { + AGENTCORE_CREDENTIAL_OPENAI_KEY: "sk-live", + AGENTCORE_CREDENTIAL_MY_IDP_CLIENT_SECRET: "shhh", + }); + + expect(await assertCredentialsDeployable(input)).toBeUndefined(); + }); + + test.each([ + ["an API key", apiKey("openai-key", { secretRef: SECRET_REF })], + ["an OAuth credential", oauth("my-idp", { clientSecretRef: SECRET_REF })], + ])( + "accepts %s pointing at Secrets Manager with no .env.local at all", + async (_label, declared) => { + expect(await assertCredentialsDeployable(await project([declared]))).toBeUndefined(); + }, + ); +}); + +describe("createCredentialSynchronizer", () => { + test("builds no client for a project that declares no credentials", async () => { + const client = recorder(); + + expect(await sync(await project([]), client.factory)).toEqual([]); + expect(client.clients).toEqual([]); + }); + + test("sets an API key read from .env.local", async () => { + const client = recorder(); + const input = await project([apiKey("openai-key")], { + AGENTCORE_CREDENTIAL_OPENAI_KEY: "sk-live", + }); + + expect(await sync(input, client.factory)).toEqual([ + { message: "Setting the secret for credential provider 'openai-key'" }, + ]); + expect(client.apiKeys).toEqual([{ name: "openai-key", apiKey: "sk-live" }]); + expect(client.clients).toEqual([{ region: REGION, credentials }]); + }); + + test.each([ + ["an API key", apiKey("openai-key", { secretRef: SECRET_REF })], + ["an OAuth credential", oauth("my-idp", { clientSecretRef: SECRET_REF })], + ])( + "leaves %s pointing at Secrets Manager alone, building no client", + async (_label, declared) => { + const client = recorder(); + + expect(await sync(await project([declared]), client.factory)).toEqual([]); + expect(client.clients).toEqual([]); + expect(client.apiKeys).toEqual([]); + expect(client.oauth2).toEqual([]); + }, + ); + + test("rebuilds a guided OAuth config around the secret, without forwarding scopes", async () => { + const client = recorder(); + const input = await project( + [oauth("my-idp", { clientId: "client-1", scopes: ["read", "write"] })], + { AGENTCORE_CREDENTIAL_MY_IDP_CLIENT_SECRET: "shhh" }, + ); + + await sync(input, client.factory); + + expect(client.oauth2).toEqual([ + { + name: "my-idp", + vendor: "CustomOauth2", + config: { + customOauth2ProviderConfig: { + oauthDiscovery: { discoveryUrl: DISCOVERY_URL }, + clientId: "client-1", + clientSecret: "shhh", + }, + }, + }, + ]); + }); + + test("injects the secret into a spec-supplied vendor config", async () => { + const client = recorder(); + const input = await project( + [ + { + authorizerType: "OAuthCredentialProvider", + name: "github", + vendor: "GithubOauth2", + providerConfig: { githubOauth2ProviderConfig: { clientId: "gh-client" } }, + }, + ], + { AGENTCORE_CREDENTIAL_GITHUB_CLIENT_SECRET: "gh-secret" }, + ); + + await sync(input, client.factory); + + expect(client.oauth2).toEqual([ + { + name: "github", + vendor: "GithubOauth2", + config: { + githubOauth2ProviderConfig: { clientId: "gh-client", clientSecret: "gh-secret" }, + }, + }, + ]); + }); + + test("rejects a providerConfig that is not exactly one vendor config", async () => { + const client = recorder(); + const input = await project( + [ + { + authorizerType: "OAuthCredentialProvider", + name: "github", + vendor: "GithubOauth2", + providerConfig: { githubOauth2ProviderConfig: {}, googleOauth2ProviderConfig: {} }, + }, + ], + { AGENTCORE_CREDENTIAL_GITHUB_CLIENT_SECRET: "gh-secret" }, + ); + + await expect(sync(input, client.factory)).rejects.toThrow( + /providerConfig with 2 entries; it must hold exactly one vendor config/, + ); + }); + + test("syncs every credential that needs it, in the order the spec declares them", async () => { + const client = recorder(); + const input = await project( + [apiKey("first"), apiKey("with-ref", { secretRef: SECRET_REF }), oauth("last")], + { + AGENTCORE_CREDENTIAL_FIRST: "key-1", + AGENTCORE_CREDENTIAL_LAST_CLIENT_SECRET: "secret-2", + }, + ); + + expect(await sync(input, client.factory)).toEqual([ + { message: "Setting the secret for credential provider 'first'" }, + { message: "Setting the secret for credential provider 'last'" }, + ]); + expect(client.apiKeys.map(({ name }) => name)).toEqual(["first"]); + expect(client.oauth2.map(({ name }) => name)).toEqual(["last"]); + // One client for the whole run, not one per credential. + expect(client.clients).toHaveLength(1); + }); +}); diff --git a/src/core/project/backends/cdk/credentials.ts b/src/core/project/backends/cdk/credentials.ts new file mode 100644 index 000000000..3f1ba6111 --- /dev/null +++ b/src/core/project/backends/cdk/credentials.ts @@ -0,0 +1,292 @@ +import { join } from "node:path"; +import type { Oauth2ProviderConfigInput } from "@aws-sdk/client-bedrock-agentcore-control"; +import { ProjectStateError } from "../../../../errors/errors"; +import type { Project, ProjectEvent } from "../../../../handlers/project/types"; +import type { Credential, OAuthCredential } from "../../../../projectSchemas/credential"; +import { + CLIENT_SECRET_SUFFIX, + credentialEnvVarName, + ENV_LOCAL_RELATIVE_PATH, + EnvLocalFile, +} from "../../envLocal"; +import type { CdkCredentialProvider } from "./toolkit"; + +/** + * The two Identity calls the deploy-time secret sync needs. Narrowed to the + * update half of the API so tests can substitute a recorder without standing up + * an SDK client, following the seam style of this directory's collaborators. + */ +export type IdentitySecretClient = { + setApiKey(name: string, apiKey: string): Promise; + setOauth2ClientSecret( + name: string, + vendor: string, + config: Oauth2ProviderConfigInput, + ): Promise; +}; + +export type IdentitySecretClientFactory = ( + region: string, + credentials: CdkCredentialProvider, +) => Promise; + +export type CredentialPreflight = (project: Project) => Promise; + +export type CredentialSyncInput = { + region: string; + /** Credential provider shared with the rest of the deployment. */ + credentials: CdkCredentialProvider; +}; + +export type CredentialSynchronizer = ( + project: Project, + input: CredentialSyncInput, +) => AsyncGenerator; + +/** + * Refuses a project whose declared credentials cannot be deployed, before any + * template is synthesized. + * + * CloudFormation owns the credential providers, but it cannot own an API key or + * client secret that only exists in `.env.local` — the stack is created with a + * placeholder and {@link createCredentialSynchronizer} replaces it once the + * deploy succeeds. That sync has no secret to push if the variable `add` told + * the user to set is missing, and by then the provider is already live holding + * the placeholder. Checking here instead means a missing secret costs nothing: + * no synth, no bootstrap, no stack. + */ +export const assertCredentialsDeployable: CredentialPreflight = async (project) => { + const declared = project.spec.credentials; + if (declared.length === 0) return; + + // Rejected first so a project carrying one fails the same way whether or not + // its other credentials have their secrets. + const payment = declared.find( + (credential) => credential.authorizerType === "PaymentCredentialProvider", + ); + if (payment) throw paymentUnsupported(payment.name); + + const env = await new EnvLocalFile(project.rootPath).read(); + for (const credential of declared) { + const secret = resolveSecret(credential, env); + if (secret.kind === "missing") { + throw missingSecret(credential.name, secret.envKey, secret.refField, project.rootPath); + } + } +}; + +/** + * Replaces the placeholder secret CloudFormation created each credential + * provider with, for the credentials whose secret lives in `.env.local`. + * + * Runs after the deploy rather than before it because the providers do not exist + * until CloudFormation makes them. Credentials pointing at a Secrets Manager + * secret of the customer's own are skipped: those deploy as an `EXTERNAL` source + * that already resolves to the real value. + * + * Every applicable provider is updated on every deploy. The placeholder cannot + * be told apart from a real key by reading the provider back — `ApiKey` is a + * write-only CloudFormation property and `GetApiKeyCredentialProvider` returns + * only the secret's ARN — so there is nothing to compare against, and skipping + * the update would leave a provider stuck on the placeholder. + */ +export function createCredentialSynchronizer( + createClient: IdentitySecretClientFactory = createIdentitySecretClient, +): CredentialSynchronizer { + return async function* syncCredentialSecrets(project, { region, credentials }) { + const declared = project.spec.credentials; + if (declared.length === 0) return; + + const env = await new EnvLocalFile(project.rootPath).read(); + const pending: { credential: Credential; secret: string }[] = []; + for (const credential of declared) { + const secret = resolveSecret(credential, env); + // A missing secret is unreachable: the preflight refused the deploy. + if (secret.kind === "inline") pending.push({ credential, secret: secret.value }); + } + if (pending.length === 0) return; + + const client = await createClient(region, credentials); + for (const { credential, secret } of pending) { + yield { message: `Setting the secret for credential provider '${credential.name}'` }; + await setSecret(client, credential, secret); + } + }; +} + +/** + * Builds an Identity client against the deployment target's own credentials. The + * SDK is imported lazily so projects without credentials never pay for loading + * it, matching how the CloudFormation and STS clients are built. + */ +export const createIdentitySecretClient: IdentitySecretClientFactory = async ( + region, + credentials, +) => { + const { + BedrockAgentCoreControlClient, + UpdateApiKeyCredentialProviderCommand, + UpdateOauth2CredentialProviderCommand, + } = await import("@aws-sdk/client-bedrock-agentcore-control"); + const client = new BedrockAgentCoreControlClient({ credentials, region }); + + return { + async setApiKey(name, apiKey) { + await client.send(new UpdateApiKeyCredentialProviderCommand({ name, apiKey })); + }, + async setOauth2ClientSecret(name, vendor, config) { + await client.send( + new UpdateOauth2CredentialProviderCommand({ + name, + // The spec's vendor is free-form so a new service vendor works without + // a CLI release; the service rejects values it does not know. + credentialProviderVendor: vendor as never, + oauth2ProviderConfigInput: config, + }), + ); + }, + }; +}; + +async function setSecret( + client: IdentitySecretClient, + credential: Credential, + secret: string, +): Promise { + switch (credential.authorizerType) { + case "ApiKeyCredentialProvider": + return client.setApiKey(credential.name, secret); + case "OAuthCredentialProvider": + // UpdateOauth2CredentialProvider replaces the whole provider config, so + // the secret-free config from the spec is rebuilt with the secret in it. + return client.setOauth2ClientSecret( + credential.name, + credential.vendor, + oauth2ConfigWithSecret(credential, secret), + ); + case "PaymentCredentialProvider": + // Unreachable: the preflight refuses a project declaring one. + throw paymentUnsupported(credential.name); + } +} + +/** Where a credential's secret material comes from, resolved once for both passes. */ +type ResolvedSecret = + /** A Secrets Manager secret the customer owns: CloudFormation resolves it. */ + | { kind: "external" } + /** Read from `.env.local`, so the deploy-time sync has to push it. */ + | { kind: "inline"; value: string } + | { kind: "missing"; envKey: string; refField: "secretRef" | "clientSecretRef" }; + +/** + * The preflight and the sync must agree on which credentials need a secret + * pushed and where it comes from, so both read it from here. + */ +function resolveSecret(credential: Credential, env: Record): ResolvedSecret { + switch (credential.authorizerType) { + case "ApiKeyCredentialProvider": { + if (credential.secretRef) return { kind: "external" }; + return fromEnv(env, credentialEnvVarName(credential.name), "secretRef"); + } + case "OAuthCredentialProvider": { + if (credential.clientSecretRef) return { kind: "external" }; + const envKey = credentialEnvVarName(credential.name, CLIENT_SECRET_SUFFIX); + return fromEnv(env, envKey, "clientSecretRef"); + } + case "PaymentCredentialProvider": + // Unreachable: the preflight rejects payment credentials before this runs. + throw paymentUnsupported(credential.name); + } +} + +function fromEnv( + env: Record, + envKey: string, + refField: "secretRef" | "clientSecretRef", +): ResolvedSecret { + const value = env[envKey]; + return value ? { kind: "inline", value } : { kind: "missing", envKey, refField }; +} + +function oauth2ConfigWithSecret( + credential: OAuthCredential, + clientSecret: string, +): Oauth2ProviderConfigInput { + return credential.providerConfig + ? vendorConfigWithSecret(credential.name, credential.providerConfig, clientSecret) + : guidedCustomConfig(credential, clientSecret); +} + +/** + * Injects the secret into a complete, spec-supplied vendor config. The spec + * keeps provider configs secret-free, so the one vendor key it carries is the + * only place the secret can go. + */ +function vendorConfigWithSecret( + name: string, + providerConfig: Record, + clientSecret: string, +): Oauth2ProviderConfigInput { + const entries = Object.entries(providerConfig); + const [configKey, vendorConfig] = entries[0] ?? []; + if ( + entries.length !== 1 || + !configKey || + typeof vendorConfig !== "object" || + vendorConfig === null || + Array.isArray(vendorConfig) + ) { + throw new ProjectStateError( + `Credential '${name}' has a providerConfig with ${entries.length} entries; it must hold ` + + `exactly one vendor config object (for example { "customOauth2ProviderConfig": { ... } }).`, + ); + } + return { [configKey]: { ...vendorConfig, clientSecret } } as unknown as Oauth2ProviderConfigInput; +} + +function guidedCustomConfig( + credential: OAuthCredential, + clientSecret: string, +): Oauth2ProviderConfigInput { + // The spec's schema requires discoveryUrl for a guided credential; this guards + // a spec written before that rule rather than a reachable state. + if (!credential.discoveryUrl) { + throw new ProjectStateError( + `Credential '${credential.name}' needs either a discoveryUrl or a providerConfig ` + + `to set its OAuth2 client secret.`, + ); + } + // `scopes` is deliberately not forwarded: the provider config has no scopes + // field, and the spec's scopes are consumed where the credential is used. + return { + customOauth2ProviderConfig: { + oauthDiscovery: { discoveryUrl: credential.discoveryUrl }, + ...(credential.clientId !== undefined && { clientId: credential.clientId }), + clientSecret, + }, + }; +} + +function missingSecret( + name: string, + envKey: string, + refField: "secretRef" | "clientSecretRef", + rootPath: string, +): ProjectStateError { + return new ProjectStateError( + `Credential '${name}' has no secret for 'agentcore project deploy' to set on its ` + + `credential provider. Set ${envKey} in ${join(rootPath, ENV_LOCAL_RELATIVE_PATH)}, or give ` + + `the credential a '${refField}' in agentcore.json pointing at a secret you keep in AWS ` + + `Secrets Manager.`, + ); +} + +function paymentUnsupported(name: string): ProjectStateError { + return new ProjectStateError( + `Credential '${name}' is a PaymentCredentialProvider, which 'agentcore project deploy' ` + + `cannot create: CloudFormation has no payment credential provider resource, and a payment ` + + `provider needs vendor configuration (API key, wallet and authorization secrets) that ` + + `agentcore.json has no fields for. Remove the credential and any payment connector ` + + `referencing it to deploy the rest of the project.`, + ); +} diff --git a/src/core/project/envLocal.test.ts b/src/core/project/envLocal.test.ts index 44834806d..902b35c6b 100644 --- a/src/core/project/envLocal.test.ts +++ b/src/core/project/envLocal.test.ts @@ -4,7 +4,7 @@ import { mkdir, mkdtemp, rm } from "node:fs/promises"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { parseEnv } from "node:util"; -import { EnvLocalFile } from "./envLocal"; +import { CLIENT_SECRET_SUFFIX, credentialEnvVarName, EnvLocalFile } from "./envLocal"; const roots: string[] = []; afterEach(async () => { @@ -68,6 +68,30 @@ test.each([ expect(parsed.SECRET).toBe(expected); }); +test("read returns an empty record when the file does not exist", async () => { + const root = await tempRoot(); + expect(await new EnvLocalFile(root).read()).toEqual({}); +}); + +test("read parses back exactly what insertIfNew wrote", async () => { + const root = await tempRoot(); + const file = new EnvLocalFile(root); + await file.insertIfNew([ + { key: "SECRET", value: " spaced # value ", comment: "c" }, + { key: "EMPTY", comment: "c" }, + ]); + + expect(await file.read()).toEqual({ SECRET: " spaced # value ", EMPTY: "" }); +}); + +test.each([ + ["openai-key", "AGENTCORE_CREDENTIAL_OPENAI_KEY"], + ["mixed_Case-name", "AGENTCORE_CREDENTIAL_MIXED_CASE_NAME"], +])("credentialEnvVarName maps %p to %p", (name, expected) => { + expect(credentialEnvVarName(name)).toBe(expected); + expect(credentialEnvVarName(name, CLIENT_SECRET_SUFFIX)).toBe(`${expected}_CLIENT_SECRET`); +}); + test("rejects a value that contains a single quote", async () => { const root = await tempRoot(); const file = new EnvLocalFile(root); diff --git a/src/core/project/envLocal.ts b/src/core/project/envLocal.ts index 5774dde36..8a5ea6ee4 100644 --- a/src/core/project/envLocal.ts +++ b/src/core/project/envLocal.ts @@ -1,5 +1,6 @@ import { rm } from "node:fs/promises"; import { join } from "node:path"; +import { parseEnv } from "node:util"; import { atomicWrite, readTextFile } from "../../io"; import { InputValidationError } from "../../errors"; import type { EnvLocalEntry } from "../../handlers/project/types"; @@ -9,6 +10,19 @@ export const ENV_LOCAL_RELATIVE_PATH = join("agentcore", ".env.local"); const KEY_LINE = /^\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*=/; +/** Suffix distinguishing an OAuth credential's client secret from an API key. */ +export const CLIENT_SECRET_SUFFIX = "_CLIENT_SECRET"; + +/** + * Derives the variable name a credential's secret is stored under. This is the + * only contract between `project add credentials` (which writes the entry) and + * `project deploy` (which reads it back to create the provider), so both sides + * derive the name here rather than formatting it themselves. + */ +export function credentialEnvVarName(credentialName: string, suffix = ""): string { + return `AGENTCORE_CREDENTIAL_${credentialName.replace(/-/g, "_").toUpperCase()}${suffix}`; +} + /** * The project's `.env.local` secrets file, edited transactionally. `insertIfNew` * appends entries (never overwriting an existing key) and snapshots the prior @@ -63,6 +77,19 @@ export class EnvLocalFile { return { written, skipped }; } + /** + * Parses the file into its variables, returning an empty record when the file + * does not exist. Values are read back with the same parser `agentcore dev` + * uses, so quoting written by {@link insertIfNew} round-trips. + */ + async read(): Promise> { + const content = await this.readOrNull(); + if (content === null) return {}; + // parseEnv types values as string | undefined for repeated keys; the last + // assignment wins and only string values are ever produced. + return parseEnv(content) as Record; + } + /** Restores the file to its pre-write state; a no-op when nothing was written. */ async rollback(): Promise { if (this.snapshot === undefined) return; diff --git a/src/handlers/project/add/credentials/oauth/index.ts b/src/handlers/project/add/credentials/oauth/index.ts index 96c71fe58..818f10699 100644 --- a/src/handlers/project/add/credentials/oauth/index.ts +++ b/src/handlers/project/add/credentials/oauth/index.ts @@ -9,7 +9,12 @@ import { } from "../../../../identity/oauth2-credential-provider/config"; import type { AddProjectResourceConfig } from "../../types"; import type { EnvLocalEntry } from "../../../types"; -import { addCredentialToProject, credentialEnvVarName, parseExclusiveSecretRef } from "../shared"; +import { + addCredentialToProject, + CLIENT_SECRET_SUFFIX, + credentialEnvVarName, + parseExclusiveSecretRef, +} from "../shared"; export const createAddOauthCredentialHandler = (config: AddProjectResourceConfig) => createHandler({ @@ -95,7 +100,7 @@ export const createAddOauthCredentialHandler = (config: AddProjectResourceConfig ? [] : [ { - key: credentialEnvVarName(flags.name, "_CLIENT_SECRET"), + key: credentialEnvVarName(flags.name, CLIENT_SECRET_SUFFIX), value: clientSecret, comment: `OAuth client secret for credential provider '${flags.name}' (set before deploy)`, }, diff --git a/src/handlers/project/add/credentials/shared.ts b/src/handlers/project/add/credentials/shared.ts index a9a1445c9..b4f05c554 100644 --- a/src/handlers/project/add/credentials/shared.ts +++ b/src/handlers/project/add/credentials/shared.ts @@ -1,13 +1,13 @@ import { ProjectKey, type Context } from "../../../../router"; import { InputValidationError } from "../../../../errors"; +import { CLIENT_SECRET_SUFFIX, credentialEnvVarName } from "../../../../core/project/envLocal"; import { parseSecretReference } from "../../../identity/parser"; import type { AddProjectResourceConfig } from "../types"; import type { AddResourceInput } from "../../types"; -/** Derives the .env.local variable name a credential's secret is stored under. */ -export function credentialEnvVarName(credentialName: string, suffix = ""): string { - return `AGENTCORE_CREDENTIAL_${credentialName.replace(/-/g, "_").toUpperCase()}${suffix}`; -} +// Re-exported so the add handlers and `project deploy` derive secret variable +// names from one definition: deploy reads back exactly what add writes. +export { CLIENT_SECRET_SUFFIX, credentialEnvVarName }; /** Parses a secret-reference flag, rejecting a directly supplied secret alongside it. */ export function parseExclusiveSecretRef(