Skip to content

fix(secrets_detection): detect quoted AWS secret keys - #138

Merged
msureshkumar88 merged 2 commits into
mainfrom
user/luca/fix-aws-secret-access-key-quotes
Jul 15, 2026
Merged

msureshkumar88 merged 2 commits into
mainfrom
user/luca/fix-aws-secret-access-key-quotes

Conversation

@lucarlig

Copy link
Copy Markdown
Collaborator

Summary

  • accept optional single or double quotes around AWS Secret Access Key values
  • add Rust scanner coverage for both quote styles
  • add tool post-invoke regression tests for Redact and Block behavior
  • bump the secrets_detection plugin version to 0.3.8

Root cause

The aws_secret_access_key regex required the 40-character value to begin immediately after whitespace following =. Common quoted assignments therefore produced zero findings, so neither redaction nor blocking was applied.

Impact

Tool responses containing quoted AWS Secret Access Key assignments are now detected and follow the configured Redact or Block enforcement action.

Validation

  • make ci in plugins/rust/python-package/secrets_detection
    • 80 Rust tests passed
    • 100 Python integration tests passed
    • rustfmt, Clippy, release wheel build, and benchmark compilation passed
  • repository pre-commit hooks passed, including secret detection

Tracking

Signed-off-by: lucarlig <luca.carlig@ibm.com>
@lucarlig
lucarlig marked this pull request as ready for review July 15, 2026 09:29
@lucarlig lucarlig self-assigned this Jul 15, 2026

@msureshkumar88 msureshkumar88 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Verified locally on this branch (rebuilt the Rust extension, ran the full suite): cargo test --lib 80/80 passed, pytest plugins/tests/secrets_detection/ 100/100 passed, cargo fmt --check clean. The fix itself is correct for the case it targets, no breaking change, no unrelated diff, DCO sign-off present.

Requesting changes: quoted-secret detection gap (JSON/YAML colon form)

The new regex only extends the key = "value" assignment form:

aws_secret_access_key = "FAKE..."   # now detected ✅

It does not cover colon-delimited quoted assignment, which is a very common shape for the payloads this plugin actually scans (JSON tool responses, YAML configs, Terraform state):

"aws_secret_access_key": "FAKE..."   # NOT detected
aws_secret_access_key: "FAKE..."     # NOT detected

Confirmed by testing the equivalent pattern directly — neither matches. Since this plugin's entire purpose is preventing secret leakage through tool responses, and JSON/YAML are extremely common response formats, this is a real residual false-negative gap (CWE-200-adjacent: sensitive data can pass through undetected in these formats).

Suggested fix: support [:=] as the separator, consistent with the existing generic_api_key_assignment pattern (src/patterns.rs) which already handles both. Add regression tests for the colon form alongside the existing quote-style tests.

Non-blocking follow-up (does not need to hold this PR)

aws_secret_access_key isn't registered in CAPTURE_PATTERNS (src/patterns.rs), so redaction replaces the entire match — including the aws_secret_access_key = prefix and quotes — rather than just the 40-char secret value, unlike base64_24 which is capture-aware. Pre-existing behavior, not introduced by this PR, but worth a follow-up ticket since this PR is already touching the same regex.

Signed-off-by: lucarlig <luca.carlig@ibm.com>
@lucarlig

Copy link
Copy Markdown
Collaborator Author

@msureshkumar88 Addressed the requested JSON/YAML gap in f0fd6d1.

  • Changed the AWS secret assignment separator from = to [:=].
  • Added a Rust matrix covering 9 accepted forms: equals/colon, unquoted/single-quoted/double-quoted, spaced/compact JSON, YAML, and mixed case.
  • Added 4 negative near-misses: missing separator, unsupported separator, unrelated AWS field, and a 39-character value.
  • Parameterized the framework regressions across 5 representative assignment forms through both Redact and Block (10 hook-level cases).
  • Documented the supported assignment forms.

Validation:

  • make ci: 81/81 Rust tests and 108/108 Python integration tests passed.
  • rustfmt, Clippy, release wheel build, benchmark compilation, and repository pre-commit/secret-detection hooks passed.

I left the non-blocking CAPTURE_PATTERNS behavior unchanged as suggested, since it is pre-existing and separate from the colon false-negative.

@lucarlig
lucarlig requested a review from msureshkumar88 July 15, 2026 10:29

@msureshkumar88 msureshkumar88 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real-gateway e2e verification (commit f0fd6d1)

The follow-up commit f0fd6d1 (support colon assignments) resolves the blocking gap from my earlier CHANGES_REQUESTED review. I re-verified end-to-end against a real running mcp-context-forge gateway (not just the plugin's own unit tests), since that's the actual consumer of this package.

Setup

  • Built the plugin as a real wheel from this PR's tip commit (f0fd6d1) via maturin build --release, installed into a local mcp-context-forge gateway's venv (uv pip install --force-reinstall --no-deps <wheel>), replacing the stale package that was there.
  • Started the real gateway (uvicorn mcpgateway.main:app --port 8000) plus a real downstream MCP tool server (output_schema_test_server, providing echo-echo-back) registered as a federated gateway.
  • plugins/config.yaml → SecretsDetection block temporarily set to mode: enforce (from permissive) to exercise real enforcement; restored to the original value afterward.

Test payload

The exact shape this PR's fix targets — a JSON/YAML colon-delimited, double-quoted AWS secret — sent through tools/call on echo-echo-back via the real /rpc JSON-RPC endpoint:

"aws_secret_access_key": "FAKESecretAccessKeyForTestingEXAMPLE0000"

Results (post-fix build, commit f0fd6d1)

  • Block mode (block_on_detection: true, redact: false): gateway returned -32602 Plugin Violation: Potential secrets detected in tool result (plugin_error_code: SECRETS_DETECTED) — call blocked, secret never reached the caller.
  • Redact mode (redact: true, block_on_detection: false): gateway returned the tool content with the secret replaced by ***REDACTED***; raw secret value absent from the response.

Regression control (parent commit 65a41b1, pre-colon-fix)

Rebuilt the plugin wheel from the parent commit (65a41b1, before this fix), reinstalled into the same gateway, same config, same request:

  • Response came back with the secret verbatim: "aws_secret_access_key": "FAKESecretAccessKeyForTestingEXAMPLE0000" — confirming the colon-delimited gap was real and reachable through the actual gateway request path, not just a unit-test artifact, and that this PR's fix is what closes it.

Gateway checkout and config were restored to their original state afterward (plugin reinstalled to this PR's build, plugins/config.yaml reverted, temp processes stopped).

Approving — the fix is verified correct in a real gateway end-to-end, in addition to the unit/integration tests already passing (81 Rust, 108 Python).

See separate comment for one non-blocking process item.

@msureshkumar88

Copy link
Copy Markdown
Collaborator

Summary of changes still required before merge

Not re-blocking the approval on this, but flagging per this repo's own AGENTS.md policy ("every change to a core plugin must include a plugin version bump"):

  • Cargo.toml version is still 0.3.8 across both commits in this PR (65a41b1 and f0fd6d1) — unchanged from before the fix.
  • cpex_secrets_detection/plugin-manifest.yaml version: field likewise not bumped.

Please bump both (and let Cargo.lock update on next build) before merging, per the repo's versioning convention.

Non-blocking, cosmetic, pre-existing (not introduced by this PR): redaction still consumes the assignment prefix/quotes rather than just the secret value (aws_secret_access_key isn't in CAPTURE_PATTERNS, unlike base64_24). Worth a follow-up ticket since this PR already touches the same regex, but not required for this merge.

@msureshkumar88

Copy link
Copy Markdown
Collaborator

Correction: version bump claim above was wrong

My mistake — I only diffed the version between this PR's two commits (65a41b1 → f0fd6d1, both 0.3.8) and missed that the bump was already applied at the start of the PR.

Checked against the actual merge-base with main (b9279ee): version there is 0.3.7, PR tip is 0.3.8 — bumped correctly in Cargo.toml and plugin-manifest.yaml in the first commit. No version-bump issue. Retracting that item; the approval stands unaffected (it wasn't the basis for the approval).

@msureshkumar88
msureshkumar88 merged commit 9aa8e81 into main Jul 15, 2026
23 checks passed
@msureshkumar88
msureshkumar88 deleted the user/luca/fix-aws-secret-access-key-quotes branch July 15, 2026 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants