feat(aws-secrets-inspector): hybrid AWS Secrets Manager connector with safe retrieval mode#198
Open
AnandSundar wants to merge 20 commits into
Open
feat(aws-secrets-inspector): hybrid AWS Secrets Manager connector with safe retrieval mode#198AnandSundar wants to merge 20 commits into
AnandSundar wants to merge 20 commits into
Conversation
…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>
|
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}
Comment on lines
+2
to
+6
| "name": "aws-secrets-inspector", | ||
| "version": "0.1.0", | ||
| "description": "GRC connector for AWS Secrets Manager: evaluates rotation, customer-managed KMS key, resource policy (public access), and access patterns. Hybrid shape — emits v1 finding-contract documents in inspector mode and supports opt-in secret retrieval to stdout or 0600 files in retrieve mode.", | ||
| "author": { "name": "GRC Engineering Club Contributors" }, | ||
| "license": "MIT", |
There was a problem hiding this comment.
1. plugin.json author not string 📘 Rule violation § Compliance
The new plugin manifest sets author to an object instead of a non-empty string. This violates the minimal manifest requirements and may break tooling that expects author to be a string.
Agent Prompt
## Issue description
`plugins/connectors/aws-secrets-inspector/.claude-plugin/plugin.json` does not satisfy the manifest requirement that `author` is a non-empty string.
## Issue Context
Compliance requires top-level keys `name`, `version`, and `author` to exist and each must be a non-empty string.
## Fix Focus Areas
- plugins/connectors/aws-secrets-inspector/.claude-plugin/plugin.json[2-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+27
| #!/usr/bin/env node | ||
|
|
||
| /** | ||
| * aws-secrets-inspector:collect | ||
| * | ||
| * Runs AWS CLI read-only queries against AWS Secrets Manager and emits | ||
| * findings conforming to schemas/finding.schema.json v1. | ||
| * | ||
| * Hybrid shape: this script also implements `--retrieve=<name>` mode, | ||
| * which returns a single secret's value to stdout (or to a 0600-permission | ||
| * file under ~/.config/claude-grc/secrets/ when --write-to is set). The | ||
| * retrieval branch lives at the top of main() and short-circuits before | ||
| * any cache writes; the inspector branch is the default path. | ||
| * | ||
| * Usage: | ||
| * # Inspector mode (default) | ||
| * node collect.js [--regions=us-east-1,us-west-2] | ||
| * [--profile=<name>] [--output=summary|silent|json] | ||
| * [--refresh] [--quiet] | ||
| * | ||
| * # Retrieval mode (opt-in) | ||
| * node collect.js --retrieve=<secret-name> | ||
| * [--version-stage=AWSCURRENT|AWSPREVIOUS|<label>] | ||
| * [--version-id=<uuid>] | ||
| * [--write-to=<path>] | ||
| * [--profile=<name>] | ||
| * |
There was a problem hiding this comment.
2. Node script in connector plugin 📘 Rule violation ⌂ Architecture
A new Node.js entry script was added under plugins/connectors/aws-secrets-inspector/, which is not one of the allowed persona plugin names. This violates the restriction limiting Node.js scripts to specific persona plugins.
Agent Prompt
## Issue description
A Node.js entry script (`.js`) was added in a non-allowed (non-persona) plugin directory.
## Issue Context
Compliance restricts Node.js scripts to allowed persona plugins only: `grc-engineer`, `grc-auditor`, `grc-internal`, `grc-tprm`.
## Fix Focus Areas
- plugins/connectors/aws-secrets-inspector/scripts/collect.js[1-27]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Greptile SummaryAdds the
Confidence Score: 4/5Safe to merge with one code fix: the The retrieval region-resolution loop documents
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([node collect.js args]) --> B{--retrieve set?}
B -- Yes --> R1[retrieveSecret]
B -- No --> I1[Read config YAML]
subgraph Retrieval Mode
R1 --> R2[Read config YAML]
R2 --> R3{--regions.length > 1?}
R3 -- Yes --> R4[fail EXIT.USAGE]
R3 -- No --> R5{--region set?}
R5 -- Yes --> R6[use --region]
R5 -- No --> R7{config.defaults.regions 0?}
R7 -- Yes --> R8[use config.defaults.regions 0]
R7 -- No --> R9[config/env/default fallback]
R8 --> R10[aws get-secret-value --region]
R6 --> R10
R9 --> R10
R10 --> R11{--write-to set?}
R11 -- Yes --> R12[safeResolveWritePath]
R12 --> R13[mkdir 0700 + chmod]
R13 --> R14[writeFile 0600]
R14 --> R15[stdout: confirmation only]
R11 -- No --> R16[stdout: JSON payload]
R15 & R16 --> R17[runs.log: byte_size + sha256 only]
end
subgraph Inspector Mode
I1 --> I2[For each region]
I2 --> I3[list-secrets]
I3 --> I4[For each secret ARN]
I4 --> I5[describe-secret]
I5 --> I6[get-resource-policy]
I6 --> I7[Evaluate 4 SCF checks]
I7 --> I8[Emit Finding]
I8 --> I4
I4 --> I9[Write cache JSON]
I9 --> I10[Append runs.log manifest]
end
style R4 fill:#ff9999
style R12 fill:#99ccff
style R14 fill:#99ccff
%%{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"}}}%%
flowchart TD
A([node collect.js args]) --> B{--retrieve set?}
B -- Yes --> R1[retrieveSecret]
B -- No --> I1[Read config YAML]
subgraph Retrieval Mode
R1 --> R2[Read config YAML]
R2 --> R3{--regions.length > 1?}
R3 -- Yes --> R4[fail EXIT.USAGE]
R3 -- No --> R5{--region set?}
R5 -- Yes --> R6[use --region]
R5 -- No --> R7{config.defaults.regions 0?}
R7 -- Yes --> R8[use config.defaults.regions 0]
R7 -- No --> R9[config/env/default fallback]
R8 --> R10[aws get-secret-value --region]
R6 --> R10
R9 --> R10
R10 --> R11{--write-to set?}
R11 -- Yes --> R12[safeResolveWritePath]
R12 --> R13[mkdir 0700 + chmod]
R13 --> R14[writeFile 0600]
R14 --> R15[stdout: confirmation only]
R11 -- No --> R16[stdout: JSON payload]
R15 & R16 --> R17[runs.log: byte_size + sha256 only]
end
subgraph Inspector Mode
I1 --> I2[For each region]
I2 --> I3[list-secrets]
I3 --> I4[For each secret ARN]
I4 --> I5[describe-secret]
I5 --> I6[get-resource-policy]
I6 --> I7[Evaluate 4 SCF checks]
I7 --> I8[Emit Finding]
I8 --> I4
I4 --> I9[Write cache JSON]
I9 --> I10[Append runs.log manifest]
end
style R4 fill:#ff9999
style R12 fill:#99ccff
style R14 fill:#99ccff
Reviews (6): Last reviewed commit: "fix(lychee): exclude deleted support.cla..." | 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
…r security review
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
added 2 commits
June 20, 2026 19:00
… 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.
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
…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.
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.
Comment on lines
+598
to
+618
| if (args.regions.length > 1) { | ||
| fail(EXIT.USAGE, `--retrieve does not accept --regions with multiple entries; pass a single --region=<id> instead (got: ${args.regions.join(',')}).`); | ||
| } | ||
| let region; | ||
| let regionSource; | ||
| if (args.region) { | ||
| region = args.region; regionSource = '--region'; | ||
| } else if (config.defaults?.regions?.[0]) { | ||
| region = config.defaults.regions[0]; regionSource = 'config.defaults.regions[0]'; | ||
| } else if (config.default_region) { | ||
| region = config.default_region; regionSource = 'config.default_region'; | ||
| } else if (process.env.AWS_DEFAULT_REGION) { | ||
| region = process.env.AWS_DEFAULT_REGION; regionSource = 'AWS_DEFAULT_REGION'; | ||
| } else if (process.env.AWS_REGION) { | ||
| region = process.env.AWS_REGION; regionSource = 'AWS_REGION'; | ||
| } else { | ||
| region = 'us-east-1'; regionSource = 'default'; | ||
| } | ||
| if (regionSource !== '--region') { | ||
| log(`retrieve: region=${region} inferred from ${regionSource}; pass --region=<id> to make this explicit.`); | ||
| } |
There was a problem hiding this comment.
--regions single-entry silently dropped in retrieval mode
commands/retrieve.md documents the fallback chain as: --region → first of --regions → config.defaults.regions[0] → config.default_region → env vars → us-east-1. The code, however, skips args.regions[0] entirely — after the multi-region guard (lines 598–600 reject only length > 1), the resolution immediately falls through to config.defaults.regions[0]. A user who passes --regions=eu-west-1 expecting a single-entry fallback (as the docs promise) silently uses the config default region instead. The warning on line 617 tells them a region was inferred from config, but never mentions that their --regions value was discarded — especially invisible under --quiet. The fix is to add else if (args.regions[0]) between the args.region and config.defaults?.regions?.[0] branches.
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
1 participant
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