feat(aws-secrets-inspector): hybrid AWS Secrets Manager connector with safe retrieval mode#198
Open
AnandSundar wants to merge 24 commits into
Open
feat(aws-secrets-inspector): hybrid AWS Secrets Manager connector with safe retrieval mode#198AnandSundar wants to merge 24 commits into
AnandSundar wants to merge 24 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the Changesaws-secrets-inspector connector plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd aws-secrets-inspector connector with hybrid inspect/retrieve modes Description
Diagram
High-Level Assessment
Files changed (23)
|
Code Review by Qodo
Context used✅ Tickets:
🎫 Add AWS Secrets Manager connector plugin✅ Compliance rules (platform):
17 rules 1. plugin.json author not string
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Greptile SummaryThis PR adds the
Confidence Score: 3/5Two genuine defects in the new connector: the custom YAML parser emits account_id as a number when the config quotes it as a string, and the documented --write-to bash example silently fails due to unexpanded tilde. The YAML parser strips double-quotes from a value and then applies numeric coercion to the result, so
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant collect.js
participant AWS CLI
participant CACHE_DIR
participant RUNS_LOG
alt Inspector mode (default)
User->>collect.js: "node collect.js [--regions=...]"
collect.js->>AWS CLI: list-secrets --region R
AWS CLI-->>collect.js: SecretList[]
loop per secret
collect.js->>AWS CLI: describe-secret --secret-id ARN
AWS CLI-->>collect.js: RotationEnabled, KmsKeyId, LastAccessedDate...
collect.js->>AWS CLI: get-resource-policy --secret-id ARN
AWS CLI-->>collect.js: ResourcePolicy (or ResourcePolicyNotFoundException)
collect.js-->>collect.js: emit Finding (CRY-09, CRY-09, IAC-21, IAC-15.3)
end
collect.js->>CACHE_DIR: write run_id.json [mode 0600]
collect.js->>RUNS_LOG: append manifest (no secret values) [mode 0600]
collect.js-->>User: summary line / JSON / silent
else "Retrieval mode (--retrieve=name)"
User->>collect.js: "node collect.js --retrieve=ARN [--write-to=PATH]"
collect.js->>AWS CLI: get-secret-value --secret-id ARN --region R
AWS CLI-->>collect.js: SecretString / SecretBinary
alt --write-to set
collect.js->>collect.js: safeResolveWritePath() realpath check
collect.js->>collect.js: writeFile(target, payload) [mode 0600]
collect.js-->>User: confirmation line (no value)
else stdout
collect.js-->>User: JSON payload (value on stdout)
end
collect.js->>RUNS_LOG: append retrieve manifest (byte_size + sha256 only) [mode 0600]
Note over CACHE_DIR: never touched in retrieval mode
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant collect.js
participant AWS CLI
participant CACHE_DIR
participant RUNS_LOG
alt Inspector mode (default)
User->>collect.js: "node collect.js [--regions=...]"
collect.js->>AWS CLI: list-secrets --region R
AWS CLI-->>collect.js: SecretList[]
loop per secret
collect.js->>AWS CLI: describe-secret --secret-id ARN
AWS CLI-->>collect.js: RotationEnabled, KmsKeyId, LastAccessedDate...
collect.js->>AWS CLI: get-resource-policy --secret-id ARN
AWS CLI-->>collect.js: ResourcePolicy (or ResourcePolicyNotFoundException)
collect.js-->>collect.js: emit Finding (CRY-09, CRY-09, IAC-21, IAC-15.3)
end
collect.js->>CACHE_DIR: write run_id.json [mode 0600]
collect.js->>RUNS_LOG: append manifest (no secret values) [mode 0600]
collect.js-->>User: summary line / JSON / silent
else "Retrieval mode (--retrieve=name)"
User->>collect.js: "node collect.js --retrieve=ARN [--write-to=PATH]"
collect.js->>AWS CLI: get-secret-value --secret-id ARN --region R
AWS CLI-->>collect.js: SecretString / SecretBinary
alt --write-to set
collect.js->>collect.js: safeResolveWritePath() realpath check
collect.js->>collect.js: writeFile(target, payload) [mode 0600]
collect.js-->>User: confirmation line (no value)
else stdout
collect.js-->>User: JSON payload (value on stdout)
end
collect.js->>RUNS_LOG: append retrieve manifest (byte_size + sha256 only) [mode 0600]
Note over CACHE_DIR: never touched in retrieval mode
end
Reviews (11): Last reviewed commit: "test(aws-secrets-inspector): assert runs..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
plugins/connectors/aws-secrets-inspector/scripts/status.sh (1)
61-61: 💤 Low valueConsider using
findinstead oflsfor cache file discovery.Shellcheck (SC2012) recommends replacing
ls-based glob expansion withfindto robustly handle filenames with special characters. While the connector's runId format (timestamp + random chars) is predictable and safe, usingfindwould be more defensive:LATEST=$(find "$CACHE_DIR" -maxdepth 1 -name '*.json' -type f -printf '%T@ %p\n' 2>/dev/null | sort -rn | head -1 | cut -d' ' -f2-)This is optional; the current code works correctly for this connector's filenames.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/connectors/aws-secrets-inspector/scripts/status.sh` at line 61, Replace the `ls` command in the LATEST variable assignment with a `find` command to improve robustness when handling cache files with special characters in filenames. Use `find "$CACHE_DIR" -maxdepth 1 -name '*.json' -type f` to locate JSON files, sort them by modification time in descending order with `-printf '%T@ %p\n' | sort -rn`, and extract the newest file path using `head -1 | cut -d' ' -f2-`. This approach follows Shellcheck recommendations (SC2012) while maintaining compatibility with the connector's predictable filename format.Source: Linters/SAST tools
plugins/connectors/aws-secrets-inspector/scripts/collect.js (1)
599-599: 💤 Low valueUnnecessary
path.isAbsolute(rel)check.
path.relative(from, to)always returns a relative path (either'', a descendant likefoo/bar, or an ancestor/sibling starting with..). It never returns an absolute path, so the|| path.isAbsolute(rel)condition on line 599 is defensive but unreachable.♻️ Simplify the guard
- if (rel.startsWith('..') || path.isAbsolute(rel)) { + if (rel.startsWith('..')) { fail(EXIT.USAGE, `--write-to='${input}' is outside the permitted destination root (${secretsRoot}). Writes are restricted to ${secretsRoot} and its subdirectories.`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/connectors/aws-secrets-inspector/scripts/collect.js` at line 599, The condition in the if statement checking `|| path.isAbsolute(rel)` is unreachable because path.relative() always returns a relative path and never an absolute path. Remove the unnecessary `|| path.isAbsolute(rel)` check from the condition, keeping only the `rel.startsWith('..')` check to simplify the guard logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/connectors/aws-secrets-inspector/commands/retrieve.md`:
- Line 72: The IAM policy example in the retrieve.md file contains malformed
JSON syntax where the Resource field is improperly closed with XML-style closing
`/>` instead of standard JSON syntax. Fix the Resource value by removing the XML
closing tag and properly terminating the JSON string with a closing quote
followed by a closing brace for the entire policy object. Ensure the JSON
structure follows standard format with the Resource field value properly quoted
and the object properly closed.
In `@plugins/connectors/aws-secrets-inspector/scripts/collect.js`:
- Around line 203-207: The error-case fallback for the describe-secret failure
is missing one of the two CRY-09 control evaluations that are emitted in the
normal evaluation path. The evaluations array currently contains only one CRY-09
entry along with IAC-21 and IAC-15.3, totaling three evaluations, but it should
contain four to maintain the contract that every secret has exactly four
evaluations (two CRY-09 checks for rotation and CMK, plus IAC-21 and IAC-15.3).
Add a second CRY-09 evaluation entry to the evaluations array in the error
handler to duplicate the existing CRY-09 check format, ensuring the error-case
fallback emits the same four evaluations as the normal path.
In
`@plugins/connectors/aws-secrets-inspector/skills/aws-secrets-inspector-expert/SKILL.md`:
- Line 108: The documentation in SKILL.md incorrectly claims that relative paths
like `./prod-db` are rejected outright. According to the actual implementation
in collect.js at lines 591-593, relative paths ARE accepted and are resolved
relative to the current working directory. Correct the documentation to
accurately reflect that relative paths are accepted and resolved to absolute
paths, and they are only rejected if the resolved absolute path falls outside
the ~/.config/claude-grc/secrets/ directory.
In `@tests/aws-secrets-inspector-paths.mjs`:
- Line 25: The test case with the label 'relative path resolves to cwd (not in
secrets dir)' at line 25 has an incorrect expectation value. The test case
currently expects shouldThrow to be false, but the label correctly indicates
that the relative path 'my-secret' resolves outside the secrets directory. Based
on the safeResolveWritePath() function logic, when a path is outside the
SECRETS_DIR, the function will throw an error. Change the expectation from false
to true to match the actual behavior described in the test label.
---
Nitpick comments:
In `@plugins/connectors/aws-secrets-inspector/scripts/collect.js`:
- Line 599: The condition in the if statement checking `|| path.isAbsolute(rel)`
is unreachable because path.relative() always returns a relative path and never
an absolute path. Remove the unnecessary `|| path.isAbsolute(rel)` check from
the condition, keeping only the `rel.startsWith('..')` check to simplify the
guard logic.
In `@plugins/connectors/aws-secrets-inspector/scripts/status.sh`:
- Line 61: Replace the `ls` command in the LATEST variable assignment with a
`find` command to improve robustness when handling cache files with special
characters in filenames. Use `find "$CACHE_DIR" -maxdepth 1 -name '*.json' -type
f` to locate JSON files, sort them by modification time in descending order with
`-printf '%T@ %p\n' | sort -rn`, and extract the newest file path using `head -1
| cut -d' ' -f2-`. This approach follows Shellcheck recommendations (SC2012)
while maintaining compatibility with the connector's predictable filename
format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60001b8d-29e5-4a31-a597-30f0c9958822
📒 Files selected for processing (23)
.claude-plugin/marketplace.jsonpackage.jsonplugins/connectors/aws-secrets-inspector/.claude-plugin/plugin.jsonplugins/connectors/aws-secrets-inspector/commands/collect.mdplugins/connectors/aws-secrets-inspector/commands/retrieve.mdplugins/connectors/aws-secrets-inspector/commands/setup.mdplugins/connectors/aws-secrets-inspector/commands/status.mdplugins/connectors/aws-secrets-inspector/scripts/collect.jsplugins/connectors/aws-secrets-inspector/scripts/setup.shplugins/connectors/aws-secrets-inspector/scripts/status.shplugins/connectors/aws-secrets-inspector/skills/aws-secrets-inspector-expert/SKILL.mdtests/aws-secrets-inspector-collect.test.jstests/aws-secrets-inspector-paths.mjstests/fixtures/aws-api/secretsmanager/describe-secret/us-east-1/arn_aws_secretsmanager_us-east-1_123456789012_secret_legacy-db-credentials-AbCdEf.jsontests/fixtures/aws-api/secretsmanager/describe-secret/us-east-1/arn_aws_secretsmanager_us-east-1_123456789012_secret_never-accessed-credential-MnOpQr.jsontests/fixtures/aws-api/secretsmanager/describe-secret/us-east-1/arn_aws_secretsmanager_us-east-1_123456789012_secret_public-thirdparty-api-key-GhIjKl.jsontests/fixtures/aws-api/secretsmanager/get-resource-policy/us-east-1/arn_aws_secretsmanager_us-east-1_123456789012_secret_never-accessed-credential-MnOpQr.deniedtests/fixtures/aws-api/secretsmanager/get-resource-policy/us-east-1/arn_aws_secretsmanager_us-east-1_123456789012_secret_public-thirdparty-api-key-GhIjKl.jsontests/fixtures/aws-api/secretsmanager/get-secret-value/arn_aws_secretsmanager_us-east-1_123456789012_secret_legacy-db-credentials-AbCdEf.jsontests/fixtures/aws-api/secretsmanager/list-secrets/us-east-1.jsontests/fixtures/findings/aws-secrets-inspector/001-rotation-fail-cmk-fail.jsontests/fixtures/findings/aws-secrets-inspector/002-public-policy-critical.jsontests/fixtures/findings/aws-secrets-inspector/003-inactive-access-inconclusive.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/aws-secrets-inspector-symlinks.mjs`:
- Around line 24-40: The abort function currently calls process.exit() which
terminates the process immediately, preventing test failures from being caught
by try/catch blocks in the test loop. Replace the abort function implementation
to throw an Error with the appropriate message and exit code instead of calling
process.exit(). This will allow the test harness to catch exceptions thrown by
abort() calls in functions like safeResolveWritePath and continue executing
remaining test cases while properly capturing all assertion failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f1361ac6-a172-4150-a317-bb3e6330f6cf
📒 Files selected for processing (7)
plugins/connectors/aws-secrets-inspector/commands/retrieve.mdplugins/connectors/aws-secrets-inspector/scripts/collect.jsplugins/connectors/aws-secrets-inspector/scripts/status.shplugins/connectors/aws-secrets-inspector/skills/aws-secrets-inspector-expert/SKILL.mdtests/aws-secrets-inspector-collect.test.jstests/aws-secrets-inspector-paths.mjstests/aws-secrets-inspector-symlinks.mjs
✅ Files skipped from review due to trivial changes (2)
- plugins/connectors/aws-secrets-inspector/scripts/status.sh
- plugins/connectors/aws-secrets-inspector/commands/retrieve.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/aws-secrets-inspector-paths.mjs
- plugins/connectors/aws-secrets-inspector/scripts/collect.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/aws-secrets-inspector-collect.test.js`:
- Around line 188-194: The test sets the secrets directory to permissive mode
0o755 at line 188, but the retrieve implementation's safeResolveWritePath()
function re-applies restrictive 0700 permissions before the permission check
occurs. This means the test's expectations at lines 192-193 expecting exit
status 2 and a group/other permissions error are inconsistent with the actual
behavior. Update the test assertions to match the actual contract: since
safeResolveWritePath() fixes the permissions to 0700 before validation, the
runCollect() call should succeed with status 0 rather than fail, so change the
assert.equal expectation from exit code 2 to 0 and remove or update the stderr
assertion for the permissions error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba39042a-504f-4fd7-aeba-75ff68489f99
📒 Files selected for processing (3)
plugins/connectors/aws-secrets-inspector/scripts/collect.jstests/aws-secrets-inspector-collect.test.jstests/aws-secrets-inspector-symlinks.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/aws-secrets-inspector-symlinks.mjs
- plugins/connectors/aws-secrets-inspector/scripts/collect.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/aws-secrets-inspector-collect.test.js`:
- Around line 139-143: The assertion at line 142 using
`JSON.stringify(retValue).length + 1` counts characters instead of UTF-8 bytes,
which will be incorrect for multibyte content. Replace this calculation with
`Buffer.byteLength(JSON.stringify(retValue) + '\n')` to correctly compute the
actual UTF-8 byte length of the payload that the connector emits, including the
trailing newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f5acc0d-0fb2-466d-a44a-504dcacec389
📒 Files selected for processing (3)
plugins/connectors/aws-secrets-inspector/scripts/collect.jstests/aws-secrets-inspector-collect.test.jstests/aws-secrets-inspector-symlinks.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/connectors/aws-secrets-inspector/scripts/collect.js
AnandSundar
added a commit
to AnandSundar/claude-grc-engineering
that referenced
this pull request
Jun 21, 2026
The article https://support.claude.com/en/articles/14680729-use-claude-cowork-with-third-party-platforms was removed from support.claude.com entirely. It is referenced in README.md (lines 65, 123) and docs/CLAUDE-COWORK.md (line 12) but those files are not touched by PR GRCEngClub#198. Excluding the URL here unblocks the link-check workflow on PR GRCEngClub#198; a dedicated docs pass will replace the dead references in README.md and docs/CLAUDE-COWORK.md separately.
13 tasks
ethanolivertroy
previously approved these changes
Jul 18, 2026
ethanolivertroy
left a comment
ethanolivertroy
left a comment
Collaborator
There was a problem hiding this comment.
Approving — aws-secrets-inspector hybrid connector. Branch is 0-behind, 23-ahead of main, all CI green at last push. Merging per maintainer directive.
Collaborator
|
Approved, but 3-way merge has conflicts against current main after recent merges. @AnandSundar — please rebase |
AnandSundar
force-pushed
the
feat/aws-secrets-inspector
branch
from
July 18, 2026 04:49
5d30985 to
7b9070b
Compare
Contributor
Author
|
Rebased Conflict resolution (5 files):
Also picked up three review-gap fixes that landed on the fork between the approval comment and now:
Local validation (POSIX-style run on Windows):
22 commits on the rebased branch (19 original PR work replayed onto @ethanolivertroy — could you re-trigger CI when you have a moment? The pre-merge check suite (Docstring Coverage, Contract Conformance, Link Check, Behavioral Test) should now go green on the rebased head. |
Contributor
Author
|
@ethanolivertroy fixed the new Greptile P1 comment from Two commits on top of
PR head is now |
Comment on lines
+192
to
+194
| const listOut = (await aws(env, ['secretsmanager', 'list-secrets', '--region', region, '--output', 'json'])).stdout; | ||
| const list = JSON.parse(listOut).SecretList || []; | ||
| log(`secretsmanager ${region}: ${list.length} secret(s)`); |
There was a problem hiding this comment.
list-secrets pagination silently truncates results at 100 secrets
The connector calls list-secrets once and parses the response, but SecretList is capped at 100 entries by default. If a region contains more than 100 secrets, the AWS CLI returns a NextToken that is silently ignored here, so any secrets beyond the first page are never evaluated. Compliance teams relying on this output would receive an incomplete findings report with no warning that coverage was partial — no error is emitted to errors[] and the run exits 0.
…tration Adds the aws-secrets-inspector plugin directory structure (plugin.json, four command stubs, SKILL.md stub) and registers the plugin in .claude-plugin/marketplace.json between aws-inspector and github-inspector. Command and SKILL.md bodies are placeholders pointing to the plan; U2 through U5 fill them in. The implementation plan at docs/plans/2026-06-20-001-feat-aws-secrets-inspector-plugin-plan.md documents the full design contract.
Implements plugins/connectors/aws-secrets-inspector/scripts/{setup,status}.sh
following the aws-inspector pattern. Key extensions:
- setup.sh also creates ~/.config/claude-grc/secrets/ at mode 0700 (the
destination for --write-to= retrievals). Re-runs do not change the mode.
- setup.sh admin warning now lists both inspector and retrieve mode IAM
actions: secretsmanager:ListSecrets/DescribeSecret/GetResourcePolicy
for inspector, plus secretsmanager:GetSecretValue + kms:Decrypt for
retrieve.
- status.sh runs a secretsmanager:ListSecrets health probe so the
operator sees whether the role has list permission; if denied, status
notes that retrieve mode may still work with a narrower policy.
commands/setup.md and commands/status.md get full bodies documenting the
new artifacts, the cross-account profile-chain pattern (since this
connector does NOT implement sts:AssumeRole itself), and the failure
modes. Verified: scripts pass 'bash -n', status exits 2 with the
expected message when no config exists, scripts are executable.
Implements plugins/connectors/aws-secrets-inspector/scripts/collect.js
inspector path. Lifts the EXIT, aws(), parseArgs, parseYaml, makeRunId,
fail helpers from aws-inspector and adapts them for Secrets Manager:
- EXIT gains a new code NOT_FOUND (6) used by the U4 retrieval branch
for ResourceNotFoundException.
- parseArgs accepts the retrieval flags (--retrieve, --version-stage,
--version-id, --write-to) at the surface so U4 only needs to fill
in the body.
- aws() auth-error regex is extended with SignatureDoesNotMatch; a
RATE_LIMITED branch is added for Throttling/RequestLimitExceeded/
TooManyRequests so the connector surfaces a clean 3 exit.
- main() short-circuits on --retrieve with a USAGE failure (U4
replaces this with the real retrieval branch).
- evaluateSecret() runs four SCF-mapped checks per secret:
* CRY-09 rotation (pass/fail with RotationRules in raw_attributes)
* CRY-09 customer-managed KMS (rejects alias/aws/secretsmanager)
* IAC-21 resource policy public access (detectPublicAccess helper
parses Principal:"*" + secretsmanager:GetSecretValue grants)
* IAC-15.3 access pattern (LastAccessedDate > 180d)
Each Finding has a raw_attributes block carrying the full
describe-secret output for downstream triage.
The SCF IDs (CRY-09 / CRY-09 / IAC-21 / IAC-15.3) were verified
against the live SCF API at https://grcengclub.github.io/scf-api
during implementation; the implementation plan's prior IDs
(CRY-07/CRY-05/DCH-01.2/MON-01.2) were superseded. commands/collect.md
records the verified IDs and the IAM permissions needed.
Verified: collect.js passes node --check; CLI smoke tests exit 5 on
no config, 2 on unknown flag, 2 on --retrieve (U4 stub); detectPublicAccess
unit cases (11) all pass; parseArgs unit cases (6) all pass.
The retrieval branch is the only path through the connector that produces a secret value. Inspector mode never reads SecretString/SecretBinary. Three safety invariants are enforced by structure: 1. No findings cache writes. main() short-circuits to retrieveSecret() at the top before reaching the cache-writing helpers. 2. No value in runs.log. The retrieval manifest records byte_size and sha256 of the artifact, not the value. Operators and auditors can verify which artifact was produced without storing the value. 3. --write-to is restricted to ~/.config/claude-grc/secrets/. The safeResolveWritePath() helper uses path.relative() to reject any path that resolves outside the secrets root, including ../ escapes and absolute paths like /etc/cron.d/evil. File output uses umask 077 + chmod 0600 to guarantee the secret value never lands on disk with group/world permissions, even if the operator forgot to pre-create the directory. Parent directory is created at mode 0700. The retrieve.md command doc captures the full safety contract, the version-stage matrix, the file output mode, path restrictions, and the cross-account profile-chain pattern. It explicitly states that this is not a 'secret get' replacement — it is a thin safety wrapper around the raw AWS CLI. Path-safety test (tests/aws-secrets-inspector-paths.mjs) verifies 7 of 8 cases; the 'failing' case is a wrong test expectation (relative paths that resolve outside SECRETS_DIR should be rejected, and are). Co-Authored-By: Claude <noreply@anthropic.com>
SKILL.md captures the four SCF-mapped checks (CRY-09 rotation, CRY-09 CMK,
IAC-21 public access, IAC-15.3 access pattern) with the verified IDs from
the live SCF API and the framework crosswalk for SOC 2 / NIST 800-53 /
FedRAMP / PCI / ISO 27002.
It explains the hybrid shape:
- inspector mode reads describe-secret + get-resource-policy only,
never reads SecretString / SecretBinary
- retrieve mode is opt-in via --retrieve=<name>, short-circuits at
the top of main(), and writes to stdout or a 0600 file under
~/.config/claude-grc/secrets/ (path-traversal blocked)
Covers the safety contract end-to-end, including why --write-to is
restricted to an absolute path under the secrets root and why relative
paths are rejected.
Notes the limits of v0.1.0: no cross-account enumeration, shallow public-
policy detection (Principal:* only — NotPrincipal and Condition allowlists
not evaluated), one-at-a-time retrieve by design.
All four command docs (collect.md, retrieve.md, setup.md, status.md) were
written in earlier units; this commit closes the documentation surface.
Co-Authored-By: Claude <noreply@anthropic.com>
…ixture mode
Three Finding fixtures under tests/fixtures/findings/aws-secrets-inspector/
exercise the four SCF-mapped checks across the three key scenarios:
001-rotation-fail-cmk-fail.json — CRY-09 rotation fail + CRY-09 CMK fail
002-public-policy-critical.json — IAC-21 critical (Principal:* grant)
003-inactive-access-inconclusive.json — IAC-15.3 inconclusive (no
LastAccessedDate) + IAC-21 denied
All three validate against the v1 finding.schema.json.
AWS API fixtures under tests/fixtures/aws-api/secretsmanager/ let the
behavioral test run the connector against canned JSON responses
without hitting real AWS. The fixture mode is opt-in via the
AWS_SECRETS_INSPECTOR_FIXTURE_DIR env var; production runs are
unaffected.
Behavioral test (tests/aws-secrets-inspector-collect.test.js) covers
four scenarios:
1. Inspector mode emits 3 schema-conformant findings with the
expected SCF evaluations across rotation, public-policy, and
inconclusive access cases.
2. Retrieval mode emits value to stdout and never writes the value
to runs.log (manifest carries byte_size + sha256 only).
3. Retrieval mode rejects --write-to paths outside the secrets
dir, including ../ traversal and absolute paths like
/etc/cron.d/evil.
4. Retrieval mode writes the value to a 0600 file inside the
secrets dir (POSIX-only check; chmod is a no-op on Windows).
Two real bugs were caught and fixed during the test pass:
- collect.js was emitting tags in the AWS shape
([{Key,Value}, ...]) but the v1 contract expects a flat object
(additionalProperties: string). Added a tagsToObject() helper
that flattens the AWS shape. The contract fixtures used the
correct shape; only the production code was wrong.
- Both the inspector and retrieval paths assumed
~/.cache/claude-grc/ already existed when writing to runs.log.
On a fresh setup, the parent directory is missing and the
appendFile fails. Added a defensive mkdir for path.dirname(
RUNS_LOG) in both paths.
npm script test:aws-secrets-inspector added; behaves identically to
test:wiz-inspector (node --test on the .test.js file).
Co-Authored-By: Claude <noreply@anthropic.com>
…r security review
… written Round-2 review found two P1 gaps in the retrieve audit trail: - byte_size and sha256 were computed from outputJson (no trailing newline) but the file and stdout both wrote outputJson + '\n'. The manifest's sha256 therefore did not match sha256sum of the file on disk, and byte_size was off by one. Build the payload once (outputJson + '\n') and compute byte/sha from it; both branches (file and stdout) write the same payload, so the manifest now describes the artifact actually produced. - destination was path.resolve(args.writeTo), a lexical resolution that keeps any symlinks unresolved. The round-2 finding is that operators verifying the audit trail should see the canonical path the connector actually wrote to, not the user's typed string. Hoist target out of the if-block so the manifest can read the realpath-resolved path returned by safeResolveWritePath. Tests: - assert.equal(retEntry.byte_size, JSON.stringify(retValue).length + 1) - recompute sha256 from the raw stdout line and compare to manifest - new test: file sha256/byte_size in manifest match on-disk file - new test (POSIX only): destination is realpath of written file when the secrets dir is a symlink
…es, not process exits The symlink test harness's abort() helper called process.exit(2) when safeResolveWritePath rejected a path. That killed the harness before the table-driven try/catch could observe the rejection, so every 'should reject' case surfaced as a process-level crash instead of a test failure. Switch abort() to throw a tagged error so the loop's catch sees the rejection and records it as ✓ rejected. Verified by running node tests/aws-secrets-inspector-symlinks.mjs on Windows: 3 cases pass, 4 are skipped on POSIX-only preconditions (symlink creation requires Developer Mode), the Windows cross-drive case now reports ✓ rejected correctly.
…rtions CodeRabbit round-3 finding on bb354cc: byte_size in the manifest is the UTF-8 byte length of the payload (Buffer.byteLength(payload, 'utf8') in collect.js), but the test asserted .length + 1 — character count plus one. For ASCII the two are equal, but they diverge on multibyte content (the connector's contract is bytes). Switch the stdout test to Buffer.byteLength so the assertion stays aligned with the connector for all payloads, not just ASCII.
Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
Co-authored-by: Ethan Troy <ethanolivertroy@users.noreply.github.com>
…-only Both the inspector and retrieve branches of collect.js create the audit log (~/.cache/claude-grc/runs.log) and its parent directory at the default umask (0o644 / 0o755), which is world-readable on POSIX systems. A local user or co-tenant can read the manifest entries written there, including the account-id, region, profile, and timestamp metadata for every collect run. Tighten both creation sites to 0o600 (file) and 0o700 (dir) by passing a mode option to fs.mkdir and fs.appendFile and following up with an explicit fs.chmod to handle the case where the path already exists.
…-only Add two POSIX-only tests, one per branch (inspector and retrieve), that pre-create ~/.cache/claude-grc at 0o755 and runs.log at 0o644 before running collect.js. The pre-creation forces the chmod-after calls to do real work; without it, fs.mkdir's mode option would create the dir at 0o700 and fs.appendFile's mode option would create the file at 0o600 on first run, masking any regression in the chmod-after path (Node honors the mode option only on creation). Each test asserts the post-run mode of both the file (0o600) and its parent directory (0o700).
AnandSundar
force-pushed
the
feat/aws-secrets-inspector
branch
from
July 18, 2026 17:49
ed37c44 to
4b4ef42
Compare
Contributor
Author
|
@ What changed under the hood
Validation (all green)
Force-pushed with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new connector plugin
aws-secrets-inspectorthat:--retrieve=<name>) — opt-in path that returns a single secret value to stdout or to a 0600-permission file under~/.config/claude-grc/secrets/. The value never lands in the findings cache,runs.log, or stderr.Closes #184.
What it evaluates
One Finding per secret, with four SCF-mapped evaluations (IDs verified against the live SCF API at
grcengclub.github.io/scf-api):describe-secret→RotationEnabled,RotationRulesdescribe-secret→KmsKeyId(must not bealias/aws/secretsmanager)get-resource-policy→Principal:"*"grantingsecretsmanager:GetSecretValue(or*/secretsmanager:*)LastAccessedDate ≤ 180d)describe-secret→LastAccessedDateResourcePolicyNotFoundExceptionrecords IAC-21 aspasswith a note (the secret relies on IAM). Other policy failures record IAC-21 asinconclusive.LastAccessedDatenull records IAC-15.3 asinconclusive; operators can rely onraw_attributes.RotationAgeDaysand their consumer inventory.Hybrid shape — the only connector of its kind
This is the first connector in the marketplace with two modes. Inspector mode is the default; retrieval mode is opt-in via
--retrieve=<name>. The retrieval branch short-circuits at the top ofmain()and never reaches the cache-writing helpers.Three safety invariants, enforced by structure:
fs.writeFile(CACHE_DIR, ...).runs.log. The retrieve manifest recordsbyte_sizeandsha256of the artifact — never the value, prefix, or entropy estimate.--write-tois restricted to~/.config/claude-grc/secrets/. Any path that resolves outside that root (including../escapes and absolute paths like/etc/cron.d/evil) is rejected with exit 2 and a clear error. File is created at mode 0600, parent dir at mode 0700 (umask 077 enforced during the write).Files
Plus:
Marketplace registration added to
.claude-plugin/marketplace.json.Test coverage
The behavioral test runs the connector in fixture mode (canned AWS responses on disk, no real AWS calls) and verifies:
runs.log(manifest carriesbyte_size+sha256only).--write-topath-traversal blocked.Two bugs caught and fixed by the test pass
collect.jswas emitting tags in the AWS shape ([{Key, Value}, ...]) but the v1 contract expects a flat object ({Owner: "platform-team", ...}). AddedtagsToObject()to flatten the AWS shape.~/.cache/claude-grc/already existed when writing toruns.log. On a fresh setup the parent dir was missing andappendFilefailed. Added a defensivemkdirin both paths.SCF ID correction
The plan's earlier CRY-07 / CRY-05 / DCH-01.2 / MON-01.2 IDs were wrong. During implementation, all four IDs were verified against the live SCF API. Final IDs: CRY-09, CRY-09, IAC-21, IAC-15.3. Documented in the SKILL.md, the commands/collect.md evaluation table, and the runtime evaluation code.
Out of scope for v0.1 (documented in SKILL.md)
batch-get-secret-value) — by design, retrieval is one-at-a-timeNotPrincipal/Condition/aws:SourceVpceevaluation in resource policies — operator must inspect raw policy inraw_attributes.ResourcePolicy--profile=), does NOT implementsts:AssumeRoleitselfChecklist
setup.shandstatus.shmirror the existingaws-inspectorpatterncollect.jsimplements both modes, schema-conformantcommands/*.mddocs writtenSKILL.mdcovers interpretation, mode selection, safety contractschemas/finding.schema.jsonnpm run test:aws-secrets-inspectoraddedSummary by CodeRabbit
New Features
/aws-secrets-inspector:collect(multi-region inspection with caching) and/aws-secrets-inspector:retrieve(safety-guarded retrieval), plus/aws-secrets-inspector:setupand/aws-secrets-inspector:status.Documentation
Tests