feat(secrets): encrypt local project credentials at rest by aidandaly24 · Pull Request #2565 · aws/agentcore-cli

aidandaly24 · 2026-10-11T20:55:06Z

Description

Ports the local secret-at-rest behavior from main (#1621) into the refactor architecture.

Preserve the AES-256-GCM enc:v1: envelope, sensitive-name rules, OS keychain identity, and secrets.key fallback so existing encrypted projects remain readable.
Keep the cipher and classification pure; put keychain/keyfile access in LocalSecrets under src/io.
Encrypt new secrets and migrate legacy plaintext in EnvLocalFile, preserving comments, existing values, blank placeholders, atomic writes, owner-only permissions, and rollback.
Share decrypted environment-file reads between local development and deployment credential provisioning. Missing keys or tampered values fail before a credential reaches Identity.
Keep the native keyring package optional and external for the Node bundle; embed the target-specific native addon in standalone binaries.
Update existing command and wizard tests to assert decrypted values and encrypted on-disk storage.

The follow-up test review removed duplicate envelope coverage and the simulated concurrent-writer test, which did not exercise concurrent IO. Redundant cipher input variations were removed while retaining v1 compatibility, empty-envelope decoding, fresh IVs, tamper rejection, candidate keys, and bad-key handling. Existing migration, rollback, and command-flow tests remain.

Review fixes:

Pin keyring 2.1.1, whose getters distinguish absent entries from read failures. Do not replace keys after a read error.
Prefer an existing keyfile for new encryption, and avoid keychain access when the file key decrypts the needed values.
Select required credential fields and apply plaintext environment overrides before decryption. Ignore runner-owned development variables before reading their values.
Share secret/reference suffix definitions with the project credential schema.
Embed the native binding for each standalone target and test that the compiled probe still loads after the original addon is hidden.

No popup notice was added. Quote-safe encryption before serialization remains intact.

Related Issue

None. No issue was opened, as requested.

Documentation PR

The README addition was removed as requested. No replacement documentation was added, and README.md is unchanged from the PR base.

Type of Change

Testing

Rebased onto refactor at 609ae000; preserved upstream's removal of the model/API-key flags from headless create. Encryption assertions remain on the supported add commands and create wizard.
Full local bun test on rebased head 64edb029: 4007 passed, 0 failed.
Three minimal regressions failed on the old PR head and pass with the fixes: keychain overwrite after a read error, file-key precedence, and environment overrides blocked by unrelated/overridden ciphertext.
Typecheck, lint, formatting, secret scanning, and the Node bundle passed locally.
All six supported standalone targets compiled successfully.
Node and Linux standalone --help smoke tests passed.
The native-addon regression passes with the original addon file temporarily renamed, so the compiled probe must load its embedded copy.
The isolated CI checkout passed 90 focused tests plus typecheck and lint before the local validation.
Actual macOS permission dialogs and cross-version OS-keychain contents were not tested locally; the host is Linux. Existing keychain identity and envelope format are preserved.
No AWS request shapes or infrastructure changed. Credential provisioning tests verify plaintext reaches Identity from encrypted storage. No live AWS tests were rerun for this test-only conflict resolution.
Pre-rebase Linux, Windows, and macOS verification and AWS CodeBuild e2e passed on 56e65fdb. The first macOS attempt timed out in the unrelated Harness endpoint-delete cancellation test; its unchanged-head rerun passed. Fresh CI for the rebased head is pending.
I ran bun test
I ran the relevant end-to-end tests with bun run test:e2e, or explained why they are not applicable
I ran bun run typecheck
I ran bun run lint:check
I ran bun run format:check
I ran bun run build
If I modified src/assets/, I updated affected snapshots with bun test <test-file> --update-snapshots and committed them

No files in src/assets/ were modified.

Checklist

I have read the CONTRIBUTING document
I have added any necessary tests that prove my fix is effective or my feature works
I have updated the documentation accordingly
I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
My changes generate no new warnings
Any dependent changes have been merged and published

README changes were intentionally removed. No companion SDK/CDK changes are required.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

agentcore-devx-automation

AgentCore Harness Review

Verdict: Looks good

Clean, well-tested port of the local-secrets-at-rest feature. Design choices look right:

AES-256-GCM with an authenticated enc:v1: envelope, random IVs, and fail-closed decryption (src/secrets/cipher.ts).
Keychain-first with a documented, owner-only secrets.key fallback; the optional @napi-rs/keyring is correctly marked external for both the Node bundle and the standalone binary builds, with a graceful try/catch around require() so standalone and keychain-less environments silently fall back.
writeFileSync(..., { flag: "wx" }) with EEXIST re-read is the right concurrency pattern for shared fallback keyfile creation, and independent writers reuse a fallback key covers it.
EnvLocalFile.encryptContent reparses with parseEnv for value semantics and uses the regex only to pick replacement spans, so comments, multiline values, and unrelated content are preserved. Already-encrypted (enc:v1: prefix), empty, and identifier-suffixed values are skipped, so encrypt is idempotent on migrated files.
Rollback semantics are preserved because encryptContent snapshots the original existing even when the only change is legacy migration — provisioning that fails after a migrating insertIfNew call still unwinds correctly.
Credential provisioning reads via EnvLocalFile.read() → readEnvFile, so a missing/corrupt key fails before any Identity call (verified by refuses an unreadable encrypted secret before calling Identity).
Tests exercise real tmpdirs plus injected KeychainEntry fakes rather than mocking fs — in line with the project's testing guidance.
testing/setup.ts pins AGENTCORE_DISABLE_KEYCHAIN=1 and a tmpdir AGENTCORE_CONFIG_DIR, so the suite can't touch the real OS keychain.

Nothing blocking from me — ship it.

codecov-commenter · 2026-10-11T21:00:01Z

Codecov Report

❌ Patch coverage is 98.10606% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.50%. Comparing base (609ae00) to head (64edb02).

Files with missing lines	Patch %	Lines
scripts/keyring.ts	90.00%	3 Missing ⚠️
src/io/envFile.ts	96.77%	1 Missing ⚠️
src/io/keyring.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           refactor    #2565    +/-   ##
==========================================
  Coverage     97.50%   97.50%            
==========================================
  Files           635      641     +6     
  Lines         47250    47479   +229     
==========================================
+ Hits          46071    46296   +225     
- Misses         1179     1183     +4

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

agentcore-devx-automation · 2026-10-11T21:00:24Z

Claude Security Review: no high-confidence findings. (run)

agentcore-devx-automation · 2026-10-12T14:34:02Z

Claude Security Review: no high-confidence findings. (run)

tejaskash

I checked each of these comments by running the PR head (65bf2a2) locally, including a darwin-arm64 binary build and the npm bundle on the same project.

tejaskash · 2026-10-12T16:45:44Z

+      let password: string | null = null;
+      try {
+        password = entry.getPassword();
+      } catch {


This can overwrite the existing keychain key. Any getPassword() error is swallowed here, password stays null, and the code then calls setPassword with a new random key. In @napi-rs/keyring 1.3.0 a missing entry returns null and does not throw, so this catch only ever sees real failures such as a locked keychain or a denied access prompt.

I reproduced this with an injected keychain whose getPassword throws once. The key was replaced, and every earlier enc:v1 value then failed with "Could not decrypt a stored secret". The old key can't be recovered.

Suggested fix: let the error propagate, or fall back to the keyfile without writing. Only call setPassword when getPassword() returned null.

Fixed in 4716f91. A thrown read now exits the keychain path without calling setPassword. I also pinned keyring 2.1.1: 1.3.0 itself collapsed native read errors into null, whereas 2.1.1 distinguishes absence from failure. The regression confirms no writes occur after a denied read and previously encrypted values remain readable when access returns.

tejaskash · 2026-10-12T16:45:44Z

+      try {
+        password = entry.getPassword();
+      } catch {
+        // Some native stores report an absent entry by throwing.


This comment is not accurate for @napi-rs/keyring 1.3.0. I checked against the real library: new Entry("<missing service>", "x").getPassword() returns null. Separately, the PR adds six // comments in non-test code (four in this file, one in cipher.ts, one in envLocal.ts). Please remove the ones that restate the code.

Removed the inaccurate missing-entry comment and the comments that restated the implementation. Kept the parser/span-replacement rationale in envLocal because it explains why value parsing and preserving surrounding file text use different mechanisms.

tejaskash · 2026-10-12T16:45:44Z

+
+  private encryptionKey(): Buffer {
+    if (this.key) return this.key;
+    const keychainKey = this.keychainKey(true);


npm and binary users end up with two different keys. This prefers the keychain even when secrets.key already exists. Together with the build.ts comment (the binary never loads the keychain), values written by the npm CLI can't be read by the binary.

End to end on one project: the binary added fromBin (encrypted with the keyfile), then the npm bundle added fromNpm (encrypted with the keychain). Using only the keyfile, which is what the binary sees, FROMBIN decrypts and FROMNPM fails. Using the keychain and the keyfile, which is what npm sees, both decrypt.

Suggested fix: if a keyfile already exists, use it for encryption instead of creating a keychain key.

Fixed in 4716f91. An existing valid keyfile now takes precedence for encryption. Decryption tries that file key before touching the keychain, while still supporting older keychain-encrypted values. The regression supplies an available keychain and confirms it is never accessed when the file key handles both operations.

tejaskash · 2026-10-12T16:45:44Z

    define: DEFINE,
    root: REPO_ROOT,
    naming: { asset: ASSET_NAMING },
+    external: ["@napi-rs/keyring"],


The keychain never loads in the standalone binary. With the module marked external, createRequire(import.meta.url) inside the compiled binary can't resolve @napi-rs/keyring, so defaultKeychain() silently returns null.

I built agentcore-darwin-arm64, ran add credentials api-key from /tmp, and it created ~/.agentcore/secrets.key even though this Mac already has an aws-agentcore keychain entry. Binary users always get the plaintext keyfile and are never told.

Fixed in 56e65fd. The npm loader stays optional/external, but standalone builds replace the loader with a direct require of the target-specific .node addon so Bun embeds it. All six targets compile locally. The runtime regression hides the original addon before executing the compiled probe, proving it does not load from the build checkout. Fresh macOS/Windows CI will exercise the same probe on those platforms.

tejaskash · 2026-10-12T16:45:44Z

+  } catch (error) {
+    throw new InputValidationError(`Invalid local environment file at ${path}`, { cause: error });
+  }
+  return (config.secrets ?? new LocalSecrets()).decrypt(env);


One value that can't be decrypted fails the whole read. This includes keys the caller never uses. A .env.local holding an undecryptable SOME_API_KEY makes EnvLocalFile.read() throw, and agentcore dev goes through this same function.

Suggested fix: decrypt lazily, or only the keys the caller asks for, so an unrelated value doesn't block the command.

Added a caller-provided selection predicate before decryption. Deployment selects only required credential fields, so unrelated ciphertext no longer blocks it. Dev excludes runner-owned PORT/FASTMCP_PORT/LOCAL_DEV values before reading them. Dev still forwards arbitrary other file variables to the agent; those remain fail-closed rather than silently dropping a value the agent may need.

tejaskash · 2026-10-12T16:45:44Z

-    const content = await this.readOrNull();
-    if (content === null) return {};
-    return parseEnv(content);
+    return readEnvFile(this.path, { secrets: this.secrets });


This blocks the process environment override in deploy. createCredentialProvisioner (credentials.ts:183) spreads credentialEnvironment(processEnv) over this read so a deploy can get its secrets from the environment. This read now throws first.

I ran the provisioner with AGENTCORE_CREDENTIAL_OPENAI=from-env and one undecryptable value in .env.local. It threw "Could not decrypt a stored secret" before any Identity call.

Fixed in 0fb1fb3. Plaintext process-environment overrides are applied before decryption: overridden file values are excluded entirely. Updated the existing provisioning test to use both an undecryptable stored value for the overridden credential and unrelated bad ciphertext; the supplied environment value reaches Identity successfully.

tejaskash · 2026-10-12T16:45:44Z

      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`;
+      const encrypted = await this.secrets.encrypt({ [entry.key]: entry.value ?? "" });


This per-entry encrypt is redundant. encryptContent(content) runs right after the loop and already encrypts any plaintext sensitive value in the file. Dropping this call and writing entry.value directly gives the same file content.

I kept encryption before formatting here: formatValue rejects a single quote in plaintext, so simply replacing the encrypted argument with entry.value throws before encryptContent can run. The existing quote-bearing-secret regression covers that case. We could batch the pre-serialization encryption separately, but dropping this call alone would lose that behavior. I removed the surrounding narrating comment.

tejaskash · 2026-10-12T16:45:44Z

+  "_API_KEY",
+];
+
+const REFERENCE_SUFFIXES = ["_API_KEY_ID", "_APP_ID", "_CLIENT_ID", "_AUTHORIZATION_ID"];


These suffix lists repeat the secret and identifier split in src/projectSchemas/credential.ts (CREDENTIAL_FIELD_SUFFIXES and credentialEnvironmentVariableNames). Every field written today is classified correctly. If a new field is added there and not here, it falls back to the AGENTCORE_CREDENTIAL_ prefix rule, and an identifier could end up encrypted. Consider deriving these lists from credential.ts.

Addressed in ef4e6ce. credential.ts now owns the secret and reference suffix arrays; both credential-name validation and sensitive-value classification derive from those definitions. The generic _API_KEY suffix remains supported outside the credential namespace. Added _NAME to the existing readable-reference test table.

agentcore-devx-automation · 2026-10-12T18:32:38Z

Claude Security Review: no high-confidence findings. (run)

agentcore-devx-automation · 2026-10-13T13:55:29Z

Claude Security Review: no high-confidence findings. (run)

… compile (#2587) The prerelease workflow compiles all six binary targets but installed only the host platform's @napi-rs/keyring binding, so compile fails on the first non-host target since #2565. Match the release workflow's install step.

github-actions Bot added the size/l PR size: L label Oct 11, 2026

agentcore-devx-automation Bot added agentcore-harness-reviewing AgentCore Harness review in progress claude-security-reviewing Claude Code /security-review in progress labels Oct 11, 2026

agentcore-devx-automation Bot reviewed Oct 11, 2026

View reviewed changes

agentcore-devx-automation Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Oct 11, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Oct 11, 2026

github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Oct 12, 2026

agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Oct 12, 2026

github-actions Bot added size/l PR size: L and removed size/l PR size: L labels Oct 12, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Oct 12, 2026

tejaskash reviewed Oct 12, 2026

View reviewed changes

github-actions Bot added size/xl PR size: XL and removed size/l PR size: L labels Oct 12, 2026

agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Oct 12, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Oct 12, 2026

github-actions Bot added size/xl PR size: XL and removed size/xl PR size: XL labels Oct 12, 2026

AlexanderRichey previously approved these changes Oct 12, 2026

View reviewed changes

aidandaly24 added 4 commits October 13, 2026 13:44

feat(secrets): port authenticated secret envelopes

6d46c04

feat(secrets): classify sensitive environment values

3d5facb

feat(secrets): add machine-local encryption key storage

238657a

feat(project): encrypt local credentials and share decrypted reads

783abac

aidandaly24 and others added 9 commits October 13, 2026 13:45

build(secrets): make native keyring an optional external dependency

3b53b8c

docs(secrets): explain local encryption and key recovery

8cc483f

docs(secrets): remove README addition

47a9ecb

test(secrets): trim redundant coverage

f6bbe03

fix(secrets): preserve encryption keys on keychain read failures

4fea5ff

fix(project): scope local secret reads before decryption

70ba9bc

fix(project): apply credential environment overrides before decryption

d697bf4

refactor(secrets): share credential suffix classification

cde862a

build(secrets): embed native keyring bindings in standalone binaries

64edb02

aidandaly24 dismissed AlexanderRichey’s stale review via 64edb02 October 13, 2026 13:48

aidandaly24 force-pushed the feat/refactor-secret-encryption branch from 56e65fd to 64edb02 Compare October 13, 2026 13:48

github-actions Bot added size/xl PR size: XL and removed size/xl PR size: XL labels Oct 13, 2026

agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Oct 13, 2026

agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Oct 13, 2026

notgitika approved these changes Oct 13, 2026

View reviewed changes

aidandaly24 merged commit 8f38d29 into aws:refactor Oct 13, 2026
22 checks passed

tejaskash mentioned this pull request Oct 13, 2026

fix(ci): install every keyring platform binding before the prerelease compile #2587

Merged

Conversation

aidandaly24 commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Documentation PR

Type of Change

Testing

Checklist

Uh oh!

agentcore-devx-automation Bot left a comment

Choose a reason for hiding this comment

AgentCore Harness Review

Uh oh!

codecov-commenter commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agentcore-devx-automation Bot commented Oct 11, 2026

Uh oh!

agentcore-devx-automation Bot commented Oct 12, 2026

Uh oh!

tejaskash left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentcore-devx-automation Bot commented Oct 12, 2026

Uh oh!

agentcore-devx-automation Bot commented Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aidandaly24 commented Oct 11, 2026 •

edited

Loading

codecov-commenter commented Oct 11, 2026 •

edited

Loading