-
Notifications
You must be signed in to change notification settings - Fork 74
feat(project): add 'project add credentials' (AgentCore Identity api-key + oauth) #2046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2fce262
10d9499
31decfe
a82b5cf
1cd5d14
19fd64d
9f6190f
224f6c4
0b39fb9
a37b46d
7e67b71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { afterEach, expect, test } from "bun:test"; | ||
| import { existsSync } from "node:fs"; | ||
| 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"; | ||
|
|
||
| const roots: string[] = []; | ||
| afterEach(async () => { | ||
| await Promise.all(roots.splice(0).map((r) => rm(r, { recursive: true, force: true }))); | ||
| }); | ||
|
|
||
| async function tempRoot(): Promise<string> { | ||
| const root = await mkdtemp(join(tmpdir(), "envlocal-")); | ||
| roots.push(root); | ||
| // Real projects always have the agentcore/ dir; the class does not create it. | ||
| await mkdir(dirname(new EnvLocalFile(root).path), { recursive: true }); | ||
| return root; | ||
| } | ||
|
|
||
| const ENTRY = { key: "SECRET", value: "v", comment: "c" }; | ||
|
|
||
| test("rollback deletes the file it created", async () => { | ||
| const root = await tempRoot(); | ||
| const file = new EnvLocalFile(root); | ||
| await file.insertIfNew([ENTRY]); | ||
| expect(existsSync(file.path)).toBe(true); | ||
|
|
||
| await file.rollback(); | ||
| expect(existsSync(file.path)).toBe(false); | ||
| }); | ||
|
|
||
| test("rollback restores the prior content of an existing file", async () => { | ||
| const root = await tempRoot(); | ||
| const file = new EnvLocalFile(root); | ||
| await Bun.write(file.path, "EXISTING=1\n"); | ||
|
|
||
| await file.insertIfNew([ENTRY]); | ||
| expect(await Bun.file(file.path).text()).toContain("SECRET='v'"); | ||
|
|
||
| await file.rollback(); | ||
| expect(await Bun.file(file.path).text()).toBe("EXISTING=1\n"); | ||
| }); | ||
|
|
||
| test("rollback is a no-op when insertIfNew wrote nothing", async () => { | ||
| const root = await tempRoot(); | ||
| const file = new EnvLocalFile(root); | ||
| await Bun.write(file.path, "SECRET=kept\n"); | ||
|
|
||
| await file.insertIfNew([ENTRY]); // key already present, so nothing is written | ||
| await file.rollback(); | ||
| expect(await Bun.file(file.path).text()).toBe("SECRET=kept\n"); | ||
| }); | ||
|
|
||
| test.each([ | ||
| ["left#right", "left#right"], | ||
| [" padded ", " padded "], | ||
| ['has"double', 'has"double'], | ||
| ["back\\slash", "back\\slash"], | ||
| ["dollar$sign", "dollar$sign"], | ||
| ])("a value with %p round-trips through parseEnv", async (value, expected) => { | ||
| const root = await tempRoot(); | ||
| const file = new EnvLocalFile(root); | ||
| await file.insertIfNew([{ key: "SECRET", value, comment: "c" }]); | ||
|
|
||
| const parsed = parseEnv(await Bun.file(file.path).text()) as Record<string, string>; | ||
| expect(parsed.SECRET).toBe(expected); | ||
| }); | ||
|
|
||
| test("rejects a value that contains a single quote", async () => { | ||
| const root = await tempRoot(); | ||
| const file = new EnvLocalFile(root); | ||
| await expect(file.insertIfNew([{ key: "SECRET", value: "a'b", comment: "c" }])).rejects.toThrow( | ||
| /single quote/, | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import { rm } from "node:fs/promises"; | ||
| import { join } from "node:path"; | ||
| import { atomicWrite, readTextFile } from "../../io"; | ||
| import { InputValidationError } from "../../errors"; | ||
| import type { EnvLocalEntry } from "../../handlers/project/types"; | ||
|
|
||
| /** The project-relative path of the local secrets file (read by `agentcore dev`). */ | ||
| export const ENV_LOCAL_RELATIVE_PATH = join("agentcore", ".env.local"); | ||
|
|
||
| const KEY_LINE = /^\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*=/; | ||
|
|
||
| /** | ||
| * The project's `.env.local` secrets file, edited transactionally. `insertIfNew` | ||
| * appends entries (never overwriting an existing key) and snapshots the prior | ||
| * state so `rollback` can undo the write if a later step in the same operation | ||
| * fails. Mirrors the class shape of {@link SourceResolver} so callers hold one | ||
| * object and reverse its effect, rather than tracking loose paths. | ||
| */ | ||
| export class EnvLocalFile { | ||
| // undefined: no write yet; null: file did not exist before the write; | ||
| // string: the file's content before the write. | ||
| private snapshot?: string | null; | ||
|
|
||
| constructor(private readonly rootPath: string) {} | ||
|
|
||
| /** The absolute path of the secrets file. */ | ||
| get path(): string { | ||
| return join(this.rootPath, ENV_LOCAL_RELATIVE_PATH); | ||
| } | ||
|
|
||
| /** | ||
| * Appends entries, creating the file when missing. Keys that already exist | ||
| * are left unchanged so user-managed values survive re-runs. Returns the keys | ||
| * written and those skipped. | ||
| */ | ||
| async insertIfNew(entries: EnvLocalEntry[]): Promise<{ written: string[]; skipped: string[] }> { | ||
| const existing = await this.readOrNull(); | ||
| const existingKeys = new Set( | ||
| (existing ?? "") | ||
| .split("\n") | ||
| .map((line) => KEY_LINE.exec(line)?.[1]) | ||
| .filter((key) => key !== undefined), | ||
| ); | ||
|
|
||
| const written: string[] = []; | ||
| const skipped: string[] = []; | ||
| let content = existing ?? ""; | ||
| for (const entry of entries) { | ||
| if (existingKeys.has(entry.key)) { | ||
| skipped.push(entry.key); | ||
| continue; | ||
| } | ||
| const separator = content === "" || content.endsWith("\n") ? "" : "\n"; | ||
| // Each entry is two lines: # <comment>\n<key>=<value> | ||
| content += `${separator}# ${entry.comment}\n${entry.key}=${formatValue(entry.value)}\n`; | ||
| written.push(entry.key); | ||
| } | ||
|
|
||
| if (written.length > 0) { | ||
| this.snapshot = existing; | ||
| await atomicWrite(this.path, content); | ||
| } | ||
| return { written, skipped }; | ||
| } | ||
|
|
||
| /** Restores the file to its pre-write state; a no-op when nothing was written. */ | ||
| async rollback(): Promise<void> { | ||
| if (this.snapshot === undefined) return; | ||
| if (this.snapshot === null) await rm(this.path, { force: true }); | ||
| else await atomicWrite(this.path, this.snapshot); | ||
| } | ||
|
|
||
| private async readOrNull(): Promise<string | null> { | ||
| try { | ||
| return await readTextFile(this.path); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Single-quotes a value so `node:util`'s `parseEnv` reads it back byte-for-byte. | ||
| * Single quotes are literal in that parser, so no character needs escaping, | ||
| * except a single quote itself, which the format cannot represent. | ||
| */ | ||
| function formatValue(value?: string): string { | ||
| if (!value) return ""; | ||
| if (value.includes("'")) { | ||
| throw new InputValidationError( | ||
| "a secret value that contains a single quote (') cannot be written to " + | ||
| ".env.local; supply it with a Secrets Manager reference instead", | ||
| ); | ||
| } | ||
| return `'${value}'`; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { | |
| type ReadWriteJson, | ||
| } from "../../io"; | ||
| import { defaultSource, type AssetSource } from "./source"; | ||
| import { ENV_LOCAL_RELATIVE_PATH, EnvLocalFile } from "./envLocal"; | ||
| import { createHarnessTreeFromSpec, createProjectTreeFromTemplate, TEMPLATES } from "./templates"; | ||
| import { ProjectSpecSchema, type ManagedBy } from "../../projectSchemas/project"; | ||
| import { enclosingProjectRoot } from "./fsUtils"; | ||
|
|
@@ -151,8 +152,11 @@ export class FsProjectManager implements ProjectManager { | |
| `a ${resourceType} with name '${resourceConfig.name}' already exists`, | ||
| ); | ||
|
|
||
| // Widened: arms push their own shapes; the whole-spec safeParse below validates. | ||
| const newResources: unknown[] = [...existingResources]; | ||
| const scaffoldedPaths: string[] = []; | ||
| // Non-file work that a failed spec write must also reverse. | ||
| let envFile: EnvLocalFile | undefined; | ||
|
|
||
| switch (resourceType) { | ||
| case "harness": { | ||
|
|
@@ -172,6 +176,22 @@ export class FsProjectManager implements ProjectManager { | |
| "runtime case not yet implemented in FsProjectManager.addResource", | ||
| ); | ||
| } | ||
| case "credential": { | ||
| // No file scaffolding; the secret placeholder is staged into .env.local | ||
| // and reversed with the spec write if that commit fails. | ||
| newResources.push(input.resourceConfig); | ||
| if (input.envEntries?.length) { | ||
| envFile = new EnvLocalFile(project.rootPath); | ||
| yield { message: `Updating secrets file at '${envFile.path}'` }; | ||
| const { skipped } = await envFile.insertIfNew(input.envEntries); | ||
| for (const key of skipped) { | ||
| yield { | ||
| message: `'${key}' already exists in ${ENV_LOCAL_RELATIVE_PATH}; left unchanged`, | ||
| }; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| case "config-bundle": | ||
| case "online-eval": | ||
| case "online-insight": | ||
|
|
@@ -186,37 +206,45 @@ export class FsProjectManager implements ProjectManager { | |
|
|
||
| yield { message: `Updating project spec file at '${agentCoreSpecPath}'` }; | ||
|
|
||
| // rollback scaffolding changes on failed config writes to prevent bad state. | ||
| const newSpec = { ...existingProjectSpec, [projectSpecKey]: newResources }; | ||
|
|
||
| // Validate and write inside the same boundary so a rejected spec rolls back | ||
| // staged side effects (.env.local, scaffolded files) rather than leaving them. | ||
| let newProjectSpec: z.infer<typeof ProjectSpecSchema>; | ||
| try { | ||
| const newSpec = { ...existingProjectSpec, [projectSpecKey]: newResources }; | ||
| const newSpecParseResult = ProjectSpecSchema.safeParse(newSpec); | ||
|
|
||
| if (!newSpecParseResult.success) | ||
| throw new InputValidationError(z.prettifyError(newSpecParseResult.error), { | ||
| cause: newSpecParseResult.error, | ||
| }); | ||
| const newProjectSpec = await this.json.write(agentCoreSpecPath, newSpecParseResult.data); | ||
|
|
||
| return { | ||
| ...project, | ||
| spec: newProjectSpec, | ||
| }; | ||
| newProjectSpec = await this.json.write(agentCoreSpecPath, newSpecParseResult.data); | ||
| } catch (err) { | ||
| this.logger.warn( | ||
| `failed to update ${agentCoreSpecPath}; attempting best-effort cleanup of scaffolded files`, | ||
| `could not commit the spec update to ${agentCoreSpecPath}; attempting best-effort cleanup of staged changes`, | ||
| ); | ||
| await Promise.all( | ||
| scaffoldedPaths.map((p) => | ||
| await Promise.all([ | ||
| ...scaffoldedPaths.map((p) => | ||
| rm(p, { recursive: true, force: true }).catch((e) => { | ||
| const error = AgentCoreCLIError.fromError(e); | ||
| this.logger | ||
| .child({ errorName: error.name, errorMessage: error.message }) | ||
| .warn(`failed to clean up ${p}`); | ||
| }), | ||
| ), | ||
| ); | ||
| envFile?.rollback().catch((e) => { | ||
| const error = AgentCoreCLIError.fromError(e); | ||
| this.logger | ||
| .child({ errorName: error.name, errorMessage: error.message }) | ||
| .warn(`failed to roll back ${ENV_LOCAL_RELATIVE_PATH}`); | ||
| }), | ||
| ]); | ||
| throw err; | ||
| } | ||
|
|
||
| return { | ||
| ...project, | ||
| spec: newProjectSpec, | ||
| }; | ||
| } | ||
|
|
||
| private getProjectSpecPath(project: Project): string { | ||
|
|
@@ -306,6 +334,8 @@ function toProjectSpecKey(resourceType: ProjectResource) { | |
| return "harnesses"; | ||
| case "runtime": | ||
| return "runtimes"; | ||
| case "credential": | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side Note: I really want to abstract this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving it for now. The switch is exhaustive over |
||
| return "credentials"; | ||
| case "config-bundle": | ||
| return "configBundles"; | ||
| case "online-eval": | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import z from "zod"; | ||
| import { createHandler, flag } from "../../../../../router"; | ||
| import { InputValidationError } from "../../../../../errors"; | ||
| import { SourceResolver } from "../../../../../io"; | ||
| import type { AddProjectResourceConfig } from "../../types"; | ||
| import type { EnvLocalEntry } from "../../../types"; | ||
| import { addCredentialToProject, credentialEnvVarName, parseExclusiveSecretRef } from "../shared"; | ||
|
|
||
| export const createAddApiKeyCredentialHandler = (config: AddProjectResourceConfig) => | ||
| createHandler({ | ||
| name: "api-key", | ||
| description: "add an API key credential provider to the current project", | ||
| flags: [ | ||
| flag("name", "the name of the credential provider", z.string().optional()), | ||
| flag( | ||
| "api-key", | ||
| "the API key (file://path or - for stdin; inline values are rejected)", | ||
| z.string().optional(), | ||
| { sensitive: true }, | ||
| ), | ||
| flag( | ||
| "api-key-secret-reference", | ||
| 'external secret reference JSON: {"secretId":"<arn>","jsonKey":"<key>"}', | ||
| z.string().optional(), | ||
| ), | ||
| ], | ||
| handle: async (ctx, flags) => { | ||
| if (!flags.name) | ||
| throw new InputValidationError("required option '--name <name>' not specified"); | ||
|
|
||
| const secretRef = parseExclusiveSecretRef( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we combine these flags because they do the same thing and only one of them can be used?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two flags carry different meanings, so I kept them apart. |
||
| "api-key-secret-reference", | ||
| flags["api-key-secret-reference"], | ||
| "api-key", | ||
| flags["api-key"], | ||
| ); | ||
|
|
||
| const resolver = new SourceResolver({ stdin: config.io.stdin }); | ||
| const apiKey = await resolver.resolveSecret("api-key", flags["api-key"]); | ||
|
|
||
| const envEntries: EnvLocalEntry[] = secretRef | ||
| ? [] | ||
| : [ | ||
| { | ||
| key: credentialEnvVarName(flags.name), | ||
| value: apiKey, | ||
| comment: `API key for credential provider '${flags.name}' (set before deploy)`, | ||
| }, | ||
| ]; | ||
|
|
||
| await addCredentialToProject(ctx, config, { | ||
| resourceConfig: { authorizerType: "ApiKeyCredentialProvider", name: flags.name, secretRef }, | ||
| envEntries, | ||
| }); | ||
| }, | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { Router } from "../../../../router"; | ||
| import type { AddProjectResourceConfig } from "../types"; | ||
| import { createAddApiKeyCredentialHandler } from "./api-key"; | ||
| import { createAddOauthCredentialHandler } from "./oauth"; | ||
|
|
||
| export function createAddCredentialsHandler(config: AddProjectResourceConfig): Router { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we continue to match the command tree with the file tree + 1:1 of file to handler pattern?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| const credentials = new Router( | ||
| "credentials", | ||
| "add AgentCore Identity credential providers to the current project", | ||
| ); | ||
| credentials.handler(createAddApiKeyCredentialHandler(config)); | ||
| credentials.handler(createAddOauthCredentialHandler(config)); | ||
| return credentials; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Leave a code comment show the format if you can.
// # <entry.comment>
<entry.key>=<entry.value>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.