feat(secrets): encrypt sensitive .env.local values at rest#1621
feat(secrets): encrypt sensitive .env.local values at rest#1621aidandaly24 wants to merge 15 commits into
Conversation
…yption Adds @napi-rs/keyring@^1.3.0 as an optionalDependency so the OS keychain path in key-provider.ts is backed by a declared package. The native module installed successfully on this machine, so the now-redundant @ts-expect-error suppression comment is removed (it was guarding against "module not found"; with the dep declared and resolved, TypeScript no longer needs it). npm ci continues to succeed even when the native build fails on a given platform because it is optional. Updates docs/payments.md with the at-rest encryption subsection and a revised credential rotation section that recommends the re-add path.
… its native binary
github-actions
Bot
added
the
agentcore-harness-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
Package TarballHow to installgh release download pr-1621-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.20.2.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-cli-automation
left a comment
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Nice scoped, transparent design — single chokepoint, self-migrating, opt-out keychain. A few issues to address before merge, mostly around the keyfile-fallback path and integ-test isolation. Inline comments below.
| mkdirSync(resolveConfigDir(), { recursive: true }); | ||
| const key = randomBytes(KEY_BYTES); | ||
| writeFileSync(path, key, { mode: 0o600 }); | ||
| return key; |
There was a problem hiding this comment.
Silent data-loss on a wrong-sized keyfile. If secrets.key exists but isn't 32 bytes (e.g., truncated by a bad backup/sync, partially-written from a crash, or hand-edited), keyfileKey() silently falls through to the writeFileSync below and overwrites it with a fresh random key. Every encrypted secret in every project on this machine becomes permanently undecryptable, with no warning — the next readEnvFile will surface as SecretDecryptionError: Re-add the credential, but the user has no signal that the key (not the ciphertext) is what got clobbered.
Options:
- Throw
SecretEncryptionErrorwhen the keyfile exists with the wrong size (treat as corruption — fail loudly, let the user investigate / move it aside). - Only write the keyfile when it does not exist; if it exists with the wrong size, refuse and require explicit deletion.
Either way, the create-new-key branch should be reached only when the file genuinely doesn't exist.
| } | ||
| mkdirSync(resolveConfigDir(), { recursive: true }); | ||
| const key = randomBytes(KEY_BYTES); | ||
| writeFileSync(path, key, { mode: 0o600 }); |
There was a problem hiding this comment.
Race on first-time keyfile creation. Two CLI processes calling resolveEncryptionKey() concurrently (e.g., a script doing agentcore add … & in parallel, or two terminals) can both see existsSync(path) === false, both randomBytes(32), and both writeFileSync — the second one wins and silently invalidates whatever the first one just encrypted.
Use writeFileSync(path, key, { mode: 0o600, flag: 'wx' }) and, on EEXIST, re-read the file (and validate its length). That makes first-time creation last-writer-safe.
| // no stored password yet | ||
| } | ||
| const key = randomBytes(KEY_BYTES); | ||
| entry.setPassword(key.toString('base64')); |
There was a problem hiding this comment.
Same race exists in the keychain path: between entry.getPassword() returning null and entry.setPassword(...), a concurrent process can win and store a different key. After the setPassword, you should re-read with getPassword() and use whichever value is now persisted, rather than trusting the local key variable. (Less likely than the keyfile race, but the failure mode is identical: encrypted-with-one-key, decrypted-with-another.)
| @@ -47,6 +48,10 @@ describe('multi-agent credential behavior', () => { | |||
| } | |||
There was a problem hiding this comment.
Integ tests pollute the developer's real keychain / ~/.agentcore. runCLI inherits process.env, and these tests don't set AGENTCORE_DISABLE_KEYCHAIN=1 or AGENTCORE_CONFIG_DIR. On a dev/CI machine with a keychain (macOS, Linux with libsecret), the test creates/uses the aws-agentcore / env-local-secret-key entry in the user's keychain; without a keychain, it writes to ~/.agentcore/secrets.key. That's a real side effect from npm run test:integ and (more concerning) means once the developer's keychain has a key from a previous run, these tests will validate using that key rather than a fresh one — cross-run state leakage.
Same issue in integ-tests/add-agent-auth.test.ts (the test added on line 5/120-129 of that file).
Fix: set AGENTCORE_DISABLE_KEYCHAIN=1 and AGENTCORE_CONFIG_DIR=<tmpdir> in the spawned env (via runCLI(..., { env: { ... } }) and createTestProject/beforeAll).
| `${ANSI.yellow}Warning: passing secrets as CLI flags exposes them to shell history and the ` + | ||
| `process table. Prefer interactive mode (run \`agentcore add payment-connector\` with no ` + | ||
| `secret flags) for masked entry.${ANSI.reset}\n` | ||
| ); |
There was a problem hiding this comment.
The CLI-flag leak warning only fires for add payment-connector. The same hazard applies to add agent --api-key … and add agent --client-secret … (both also persist secrets to .env.local and both currently accept secret values via positional flags in non-interactive mode). Either:
- Extract this warning into a shared helper (e.g.,
warnOnLiteralSecretFlag(flagNames: string[])incli/primitives/) and call it fromAgentPrimitive/CredentialPrimitive/auth-utilswherever a secret flag is consumed; or - Document why the warning is payment-only (e.g., "agent api-keys are typed differently" — but I don't see a reason).
Given the PR scope says "Covers the whole CLI uniformly … not just payments," option 1 fits the stated goal.
github-actions
Bot
removed
the
agentcore-harness-reviewing
Coverage Report
|
…and error text
Three bug-bash findings on the secret-at-rest feature:
- Keychain/keyfile key mismatch: a value sealed by one key source (e.g. OS
keychain) failed to decrypt when the active source differed (e.g. after
AGENTCORE_DISABLE_KEYCHAIN was set), aborting deploy with an opaque error.
readEnvFile now resolves ALL candidate keys (keychain + keyfile) and
decryptSecret tries each, so a value decrypts regardless of which source is
active. resolveCandidateKeys() is read-only (never creates a key).
- The connector secret-leak warning was emitted AFTER format validation, which
exits on a malformed secret — so a malformed-but-already-leaked secret was
never warned. Move the warning before validation (the secret hits shell
history the instant it is typed, well-formed or not).
- The decrypt-failure message ended in a period that collided with the validate
command's own `${message}.` wrapper ("credential.. Fix"). Drop the trailing
period so the wrapped sentence reads cleanly.
…ncrypted An API-key credential stores its secret in the bare AGENTCORE_CREDENTIAL_<NAME> env var. If <NAME> normalizes to a reserved reference suffix (e.g. `my-client-id` → AGENTCORE_CREDENTIAL_MY_CLIENT_ID), isSensitiveKey can't distinguish it from a non-secret reference and the value was written PLAINTEXT at rest — defeating the encryption feature. The string ambiguity is unresolvable at write time, so fail closed at creation: a shared validateCredentialNameEncryptable() rejects such names. Wired into both the CLI add path (ValidationError) and the TUI name step (customValidation → red ✗, blocks Enter until fixed). OAuth credentials append _CLIENT_SECRET so their names are unaffected.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
…ag warning Addresses review feedback on the at-rest secret encryption work: - Keyfile fallback now fails loud on a wrong-sized (corrupt) key instead of silently overwriting it, and creates the file with the wx flag so two concurrent processes cannot clobber each other's freshly minted key (the EEXIST loser re-reads the winner's key). - Keychain path re-reads the entry after setPassword so concurrent writers converge on the same persisted key rather than diverging. - Extracted the 'secret passed as a literal CLI flag' warning into a shared warnOnLiteralSecretFlag helper and wired it into both the payment-connector and credential add paths, so the nudge toward masked interactive entry is consistent and suppressed under --json. - Isolated the keychain/keyfile in the agent-auth and multi-agent-credential tests via AGENTCORE_DISABLE_KEYCHAIN + a temp AGENTCORE_CONFIG_DIR so the suite never touches the developer's real OS keychain. Adds unit tests for the corrupt-keyfile-fails-loud and keyfile-reuse paths and for the shared warning helper.
…ore deploy The payment deploy e2e already exercises the encrypt-on-disk to deploy-decrypt round trip implicitly — if decryption broke, the deploy would fail. But it never asserted the on-disk format, so a regression that silently reverted to plaintext storage would still deploy green. Add an assertion that reads agentcore/.env.local directly and verifies (a) at least one enc:v1: envelope is present and (b) the raw CDP secret values never appear in cleartext. This pins the security invariant the encryption feature exists to guarantee, on the real deploy path, with zero added AWS cost (no extra deploy — it reads the file the prior step already wrote).
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
added
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
aidandaly24
had a problem deploying
to
e2e-testing
GitHub Actions
Failure
3 participants
Description
Encrypts sensitive credential values in
agentcore/.env.localat rest, closing a security finding that provider secrets (payment provider keys, OAuth client secrets, model-provider API keys) were persisted in plaintext on disk — so copying, syncing, or backing up a project directory exposed live credentials.How it works
src/lib/secrets/module: a sensitive-key registry (isSensitiveKey), an AES-256-GCM cipher (enc:v1:envelope), and a key-provider chain..env.localreader/writer already uses (readEnvFile/writeEnvFileinsrc/lib/utils/env.ts): sensitive values are encrypted on write and decrypted on read, so deploy/dev/validate/fetch-access are unaffected and need no changes.@napi-rs/keyring), falling back to a0600~/.agentcore/secrets.keyon machines/CI without a keychain. A copiedagentcore/folder therefore contains only ciphertext.enc:v1:...; reference IDs (_API_KEY_ID,_APP_ID,_CLIENT_ID,_AUTHORIZATION_ID) stay readable..env.localfiles are read as-is and re-encrypted on the next write. No migration command, no breakage.Decryption failure throws a
SecretDecryptionError— ciphertext is never silently surfaced as if it were the real secret, and never sent to an AWS API.Related Issue
Closes the SIM-T AppSec finding for plaintext-at-rest payment/credential secrets (tracked internally; not a GitHub issue).
Closes #
Documentation PR
N/A —
docs/payments.mdis updated in this PR (Secret encryption at rest + rotation guidance).Type of Change
Testing
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsNotes:
typecheck(0 errors),lint(0 errors),build(succeeds —@napi-rs/keyringis marked esbuild-external so its native binary isn't bundled),test:unit(5434 passed),test:integ(291 passed). Nosrc/assets/changes.0600; sensitive-key registry; env.ts encrypt-on-write, decrypt-on-read, legacy self-migration, no-double-encrypt, removeEnvVars-keeps-encrypted.enc:v1:on disk AND round-trips back viareadEnvFile(OAuth client secret; model-provider keys).add payment-connectorwritesenc:v1:for the secret (reference ID stays plaintext) and prints the leak warning;agentcore validatereads the secret back and passes — confirming the cross-process encrypt→decrypt round-trip deploy depends on.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.