feat(identity): add identity scaffolding and CRUDL operations for api key - #1811
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #1811 +/- ##
============================================
+ Coverage 93.94% 94.11% +0.16%
============================================
Files 122 130 +8
Lines 6462 6648 +186
============================================
+ Hits 6071 6257 +186
Misses 391 391 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "apiKeySecretArn": { | |||
| "secretArn": "arn:aws:secretsmanager:us-west-2:314146320088:secret:bedrock-agentcore-identity!default/apikey/agentcore-cli-identity-fixture-2-7fd24230-KEL3mK" | |||
There was a problem hiding this comment.
Could these be regenerated using the E2E account. 685197708687
aidandaly24
left a comment
There was a problem hiding this comment.
A few comments, but it looks good to me and matches what I did for Runtime well. One small thing also is we probably want to update the README.md.
| description: "create an API key credential provider", | ||
| flags: [ | ||
| flag("name", "the name of the API key credential provider", z.string().optional()), | ||
| flag("api-key", "the API key value", z.string().optional()), |
There was a problem hiding this comment.
Adding --api-key puts the plaintext credential into the generic flags object. The root withLogging middleware currently logs all flags at DEBUG, and production file logging is always configured at DEBUG, so create/update will persist the key under ~/.agentcore/logs.
We should probably mark this flag as sensitive and update withLogging to redact sensitive flag values before binding { flags, args }. We could cover this generically in withLogging.test.ts, verifying that sensitive values are redacted while ordinary flags remain visible.
The same thing should effect update as well.
There was a problem hiding this comment.
Good call. I'll make the change to redact sensitive values in a separate PR to keep separation of concerns. I'll rebase the change into this PR after its merged and update the api-key flag.
| description: "create an API key credential provider", | ||
| flags: [ | ||
| flag("name", "the name of the API key credential provider", z.string().optional()), | ||
| flag("api-key", "the API key value", z.string().optional()), |
There was a problem hiding this comment.
In the doc I had wrote, the Identity contract supports managed keys through source-aware --api-key values (inline, file://path, or -) and external secrets through a structured-reference flag containing the secret ID and JSON key. Could we support both mutually exclusive input forms here?
I sent you the doc line over slack. This could also be a follow up because I think this PR is well structured where it is now. Also tagging.
There was a problem hiding this comment.
Thanks for the context! I'll do this in a follow-up PR
There was a problem hiding this comment.
Yeah, we'll have to accept the key via stdin so that it doesn't get into the user's shell history.
There was a problem hiding this comment.
Currently working on the Runtime invoke, and I have to do something similar with the keys for the bearer token. I am currently using this function which I have within the Runtime handlers request.ts, but it should eventually be a util we use:
async function readSource(
source: string,
input?: NodeJS.ReadStream,
signal?: AbortSignal,
): Promise<Uint8Array> {
signal?.throwIfAborted();
if (source.startsWith("file://")) {
try {
return await readFile(source.slice("file://".length), { signal });
} catch (error) {
if ((error as Error)?.name === "AbortError") throw error;
throw new UsageError("Unable to read request source file");
}
}
if (source !== "-") return new TextEncoder().encode(source);
if (!input) throw new UsageError("stdin is not available for this request source");
const stream = signal ? addAbortSignal(signal, input) : input;
return buffer(stream);
}and it's used like so:
export async function resolveRuntimeInvokeSources(
sources: { payload: string; bearerToken?: string },
stdin?: NodeJS.ReadStream,
signal?: AbortSignal,
): Promise<{ payload: Uint8Array; bearerToken?: string }> {
if (sources.payload === "-" && sources.bearerToken === "-") {
throw new UsageError("Payload and bearer token cannot both read from stdin");
}
const payload = await readSource(sources.payload, stdin, signal);
if (sources.bearerToken === undefined) return { payload };
const token = await readSource(sources.bearerToken, stdin, signal);
return { payload, bearerToken: new TextDecoder().decode(token) };
}| const FIXTURES = join(import.meta.dir, "__fixtures__"); | ||
|
|
||
| // Record with AWS_PROFILE=YOUR_PROFILE RECORD=1 bun test src/handlers/identity/identity.test.tsx | ||
| // The test account must have two providers pre-created for pagination tests. |
There was a problem hiding this comment.
I believe RECORD=1 creates both fixed provider names in the tests below, so pre-creating them would cause those create calls to fail with ConflictException. I think we should instead state that neither provider should exist before recording, or clean up stale providers before the create tests.
There was a problem hiding this comment.
Good call, never updated comment after I changed test to create both providers
AlexanderRichey
left a comment
There was a problem hiding this comment.
Looks good! Let's merge this in but follow up with the fix to move the api key input to stdin right away.
| description: "create an API key credential provider", | ||
| flags: [ | ||
| flag("name", "the name of the API key credential provider", z.string().optional()), | ||
| flag("api-key", "the API key value", z.string().optional()), |
There was a problem hiding this comment.
Yeah, we'll have to accept the key via stdin so that it doesn't get into the user's shell history.
| import { createUpdateApiKeyCredentialProviderHandler } from "./update"; | ||
|
|
||
| export function createApiKeyCredentialProviderHandler(core: Core, io: AppIO): Router { | ||
| return new Router("api-key-credential-provider", "manage API key credential providers") |
There was a problem hiding this comment.
I wonder if we could shorten this to api-key. api-key-credential-provider is so looooong.
Description
Adds identity scaffolding, as well as command-line CRUDL for Identity API key credential providers:
identity api-key-credential-provider createidentity api-key-credential-provider getidentity api-key-credential-provider listidentity api-key-credential-provider updateidentity api-key-credential-provider deleteFollows the structure established by harness and runtime for commands. Create/update accept
--nameand--api-key, get/delete accept justname, list accepts--max-resultsand--next-token. Similar to runtime, identity will appear in the TUI command menu at the root, but prints help pending TUI routing.Additional change to move generic default help handler created for runtime to
src/handlers/help.tsxto avoid duplication. This PR updates runtime call sites to point to new help handler.Testing
Tested via fixture tests against live AWS resources, as well as manual dogfooding via running the build artifact in CLI.
Related Issue
N/A
Type of Change
Testing
Verified from current HEAD:
Additional verification:
The committed identity fixtures were recorded with
RECORD=1against a live account in us-west-2. The RECORD run creates two providers (agentcore-cli-identity-fixture, agentcore-cli-identity-fixture-2), exercises pagination, then deletes both. The fixture set contains no credential material (API key values are not returned by the service).Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.