fix(security): make CDS-SEC-070 evaluate rendered Compose command/logging fields by RonaldHensbergen · Pull Request #353 · RonaldHensbergen/composable-data-stack

RonaldHensbergen · 2026-08-03T21:01:20Z

Summary

CDS-SEC-070 ("Secret appears in command args or log configuration") had scope: ["none"], so it was never dispatched by run_security_validation() despite "enabled": true. Its valueRegex was also malformed ("(?i)(--******", an invalid/unbalanced regex) — since the rule never ran, nothing ever compiled or exercised it. This is exactly the rule that should have caught the Vault dev-root-token-as-command-arg issue fixed in #290.

Fixed in this PR

CDS-SEC-070: added a "rendered-compose" scope to cli/security.py. run_security_validation() now does a best-effort plan + render of the profile and flattens each service's command/entrypoint/logging fields, so rules can inspect where a module's implementation template actually places a secret-bearing value — this is only visible once ${config.*} template expressions resolve into their final Compose ${CDS_*} placeholders, which the profile/env scanners alone can't see. Plan/render failures are handled silently (return no findings for this scope) since the separate plan/render stages in cds test already report them with full diagnostics.
Re-scoped CDS-SEC-070 from ["none"] to ["rendered-compose"], fixed its pathPatterns to match list-shaped command/entrypoint/logging fields (services.*.command* etc., since flattened list items produce paths like services.vault.command[2]), and fixed the broken valueRegex to actually catch secret-like command-line flags (e.g. -dev-root-token-id=${CDS_VAULT_TOKEN}).
Added "rendered-compose" to the rule-schema.json scope enum.
Added regression fixtures under tests/fixtures/security/rendered-command-secret/ mirroring the real PR Pass Vault dev root token via env var instead of command-line arg #290 bug/fix in miniature: one fixture module passes a secret via command: (vulnerable), the other passes the same secret via environment: instead (safe). Tests prove CDS-SEC-070 now fires for the vulnerable shape and stays silent for the safe one.
Updated the #297 deferred-rule documentation test (DeferredNoneScopeRuleDocumentationTest) since CDS-SEC-070 is no longer scope: ["none"].

Deferred (unchanged)

CDS-SEC-006, 030, 032, 050-054, and 071 remain scope: ["none"] as already documented by #303 — they need rendered-artifact/CLI-output scanning or file-permission checks that are a larger, separate effort. CDS-SEC-050/051/052/054 are already enforced separately by cli/image_verification.py, as their $comments note.

Testing

python -m unittest tests.test_security -v: 17/17 pass, including 3 new tests for CDS-SEC-070.
python -m unittest discover -s tests -p "test_*.py": 414/414 pass (1 skipped, Docker-only).
make lint: clean.
Manually verified cds validate profiles/local-dagster-postgres-superset-vault still passes, and the real (already-fixed) Vault profile does not produce a false-positive CDS-SEC-070 finding.

Addresses #297

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

SemTiOne

Solid work. I have some suggestions for you to consider (see inline comments).

- Add a third CDS-SEC-070 match branch to catch bare positional ${CDS_*}/${config.*}/${secrets.*} placeholders, not just "--flag=" style command arguments. - Extend pathPatterns/coverage to include healthcheck.* alongside command/entrypoint/logging, since healthcheck probes leak secrets the same way. - Map rendered-compose findings back to the owning CDS module id (via a new _map_service_to_module helper reusing the renderer's service-naming rule) instead of the raw Compose service name, which only coincidentally matched module ids in the fixture. - Replace the silent `except Exception: return None` in _try_render_compose_for_scan with a warning diagnostic (W096) so unexpected internal errors are surfaced instead of vanishing as zero findings. - Let callers pass an already-computed plan/rendered Compose YAML into run_security_validation(), and wire cli/main.py's `cds test` to do so, so the profile isn't planned and rendered twice per run. - Expand the regression fixture to exercise all three leak surfaces (flag-style, bare positional, healthcheck) and assert exact path/value/module attribution instead of loose substring checks. - Add coverage for the plan/render reuse behavior and for the new warning-diagnostic path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

RonaldHensbergen · 2026-08-05T05:26:23Z

Thanks for the thorough review, @SemTiOne! Addressed all 7 comments in 6a76339:

Bare positional secret args: added a third match.any branch (\${(CDS_|config\.|secrets\.)) so placeholders without a --flag= prefix are caught.
Dead logging coverage: extended pathPatterns to cover logging* (and healthcheck*) in the flag-style branch too, so it's no longer dead weight.
Module attribution: added _map_service_to_module(), which reuses renderer._compose_service_name to derive the real module id from the plan instead of assuming the rendered Compose service name IS the module id.
Silent exception swallowing: _try_render_compose_for_scan now emits a W096 warning diagnostic on unexpected errors instead of a bare except Exception: return None.
Duplicate plan/render: run_security_validation() now accepts optional plan/rendered_compose_yaml kwargs, and cds test (cli/main.py) builds them once and reuses them across the security/plan/render stages.
healthcheck.command leaks: added healthcheck to the leak-prone keys and to the fixture.
Stronger assertions: the regression test now asserts exact paths (command[2], command[3], healthcheck.test[1]), exact placeholder values, and the module id — for all three leak surfaces — plus a dedicated test proving the module-id mapping isn't a name coincidence (differing service/module names), and tests for the plan/render reuse and warning-diagnostic behavior.

Full suite (417 tests) and make lint pass. Ready for another look.

SemTiOne

Nice work! I re-checked and found 4 tweaks that need to be addressed (see inline comments).

- CDS-SEC-070: remove the two match.any branches that could never fire against rendered output (keyRegex only tests the final path segment, which is a list index or ordinary leaf key on rendered command/ entrypoint/healthcheck/logging paths; and ${secrets.*}/${config.*} resolve or fail before this rule ever runs), leaving only the single working "${CDS_" branch. - cds test: pass skip_self_plan_render=True to run_security_validation when its own plan/render stages already failed, so _try_render_compose_for_scan doesn't retry (and re-fail) the same build_plan()/render_compose() calls a second time. - _try_render_compose_for_scan now emits a W096 warning diagnostic when planning or rendering fails (not just on unexpected exceptions), so `cds security` (which has no separate plan/render stage) can no longer silently report "No security findings" for a profile that never actually got scanned by rendered-compose-scoped rules. - run_security_validation now only plans+renders the profile when some enabled rule in the active rule set actually declares the "rendered-compose" scope, avoiding unconditional plan+render overhead on every security scan when no such rule applies. - Add regression tests for each of the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

RonaldHensbergen · 2026-08-05T11:01:46Z

Thanks for the second pass, @SemTiOne — all 4 addressed in 071d4e6:

Dead match branches: confirmed both the `keyRegex` branch and the `config.`/`secrets.` valueRegex alternative could never fire against rendered output for exactly the reasons you called out. Trimmed CDS-SEC-070 down to the single working `${CDS_` branch, with a `$comment` explaining why the others were removed.
Redundant re-plan/render in cds test: added a `skip_self_plan_render` flag to `run_security_validation`/`_try_render_compose_for_scan`. `cds test` now sets it whenever its own plan/render stages already failed, so the security stage doesn't retry the same doomed `build_plan`/`render_compose` calls.
Silent gap in cds security: `_try_render_compose_for_scan` now emits a W096 warning diagnostic when plan/render fails too (previously only on unexpected exceptions), so cds security — which has no separate plan/render stage — surfaces that rendered-compose-scoped rules were skipped instead of just printing "No security findings."
Unconditional plan+render overhead: `run_security_validation` now checks whether any enabled rule in the active rule set actually declares the `rendered-compose` scope before planning/rendering at all.

Added regression tests for all four. Full suite (420 tests) and `make lint` pass.

SemTiOne

Nice, progress here. Some issues needs to be resolved and this good to go.

SemTiOne · 2026-08-06T07:18:23Z

Also @RonaldHensbergen, docs/threat-model.md still lists CDS-SEC-070 as dead/deferred (#297) around lines 170-172, 197-198, 257. Want to update that while this lands?

- Add a third CDS-SEC-070 match branch to catch bare positional ${CDS_*}/${config.*}/${secrets.*} placeholders, not just "--flag=" style command arguments. - Extend pathPatterns/coverage to include healthcheck.* alongside command/entrypoint/logging, since healthcheck probes leak secrets the same way. - Map rendered-compose findings back to the owning CDS module id (via a new _map_service_to_module helper reusing the renderer's service-naming rule) instead of the raw Compose service name, which only coincidentally matched module ids in the fixture. - Replace the silent `except Exception: return None` in _try_render_compose_for_scan with a warning diagnostic (W096) so unexpected internal errors are surfaced instead of vanishing as zero findings. - Let callers pass an already-computed plan/rendered Compose YAML into run_security_validation(), and wire cli/main.py's `cds test` to do so, so the profile isn't planned and rendered twice per run. - Expand the regression fixture to exercise all three leak surfaces (flag-style, bare positional, healthcheck) and assert exact path/value/module attribution instead of loose substring checks. - Add coverage for the plan/render reuse behavior and for the new warning-diagnostic path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- CDS-SEC-070: remove the two match.any branches that could never fire against rendered output (keyRegex only tests the final path segment, which is a list index or ordinary leaf key on rendered command/ entrypoint/healthcheck/logging paths; and ${secrets.*}/${config.*} resolve or fail before this rule ever runs), leaving only the single working "${CDS_" branch. - cds test: pass skip_self_plan_render=True to run_security_validation when its own plan/render stages already failed, so _try_render_compose_for_scan doesn't retry (and re-fail) the same build_plan()/render_compose() calls a second time. - _try_render_compose_for_scan now emits a W096 warning diagnostic when planning or rendering fails (not just on unexpected exceptions), so `cds security` (which has no separate plan/render stage) can no longer silently report "No security findings" for a profile that never actually got scanned by rendered-compose-scoped rules. - run_security_validation now only plans+renders the profile when some enabled rule in the active rule set actually declares the "rendered-compose" scope, avoiding unconditional plan+render overhead on every security scan when no such rule applies. - Add regression tests for each of the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

RonaldHensbergen · 2026-08-06T13:16:30Z

Addressed all 6 comments from this review round in 1fc156c:

Title overpromise (rule-set.json): extended CDS-SEC-070's $comment to document that it only catches unresolved ${CDS_*} placeholders, not literal hardcoded secrets, and why (entropy/keyword scanning of rendered values was previously dead code and intentionally dropped).
cds security exit code: it now exits non-zero when rendered-compose-scoped rules (e.g. CDS-SEC-070) were skipped due to a plan/render failure, instead of reporting "No security findings." as if the scan had actually completed.
CI-reproducible no-false-positive claim: added test_no_false_positive_for_cds_sec_070_against_the_real_rendered_compose to RealVaultProfileEndToEndTest — it builds a full temp .env from the profile's own spec.secrets.values declarations, actually plans+renders the real vault profile, and asserts CDS-SEC-070 produces zero findings.
W096 message: now includes the first underlying plan/render error code, so the warning is self-contained for cds security callers (which have no separate plan/render stage to consult for detail).
API complexity: replaced the plan/rendered_compose_yaml/skip_self_plan_render three-parameter matrix with a single PrecomputedRender(plan=, rendered_compose_yaml=, failed=) dataclass, used consistently by run_security_validation, _try_render_compose_for_scan, and both call sites (cli/main.py, tests).
Duplicate env resolution: cds test no longer calls resolve_env_file_path() twice — it now reuses the already-computed env_file.

Full suite (421 tests) and ruff check . pass.

…ging fields CDS-SEC-070 ("Secret appears in command args or log configuration") had scope: ["none"], so it was never dispatched despite enabled: true. Its valueRegex was also malformed ("(?i)(--******"), an invalid/unbalanced regex that nothing ever compiled or exercised -- exactly the kind of silent dead-rule regression #297 warned about, and the rule that should have caught the Vault dev-root-token-as-command-arg issue fixed in #290. - Add a "rendered-compose" scope to cli/security.py: run_security_validation now does a best-effort plan+render of the profile and flattens each service's command/entrypoint/logging fields, so rules can inspect where a module's implementation template actually places a secret-bearing value (only visible after ${config.*} template expressions resolve into their final Compose "${CDS_*}" placeholders). Plan/render failures are handled silently here since the separate plan/render stages already report them. - Re-scope CDS-SEC-070 to "rendered-compose", fix its pathPatterns to match list-shaped command/entrypoint/logging fields, and fix the broken valueRegex to actually catch secret-like command-line flags. - Add "rendered-compose" to the rule-schema.json scope enum. - Add regression fixtures mirroring the real PR #290 bug/fix (a fixture module passing a secret via "command:", and a safe variant passing it via "environment:"), and tests proving CDS-SEC-070 now fires for the vulnerable shape and stays silent for the safe one. - Update the #297 deferred-rule documentation test: CDS-SEC-070 is no longer scope: ["none"]. CDS-SEC-006, 030, 032, 050-054, and 071 remain scope: ["none"] as documented by #303 -- they need rendered-artifact/CLI-output scanning or file-permission checks that are out of scope here, and 050-052/054 are already enforced separately by cli/image_verification.py. Addresses #297

- Add a third CDS-SEC-070 match branch to catch bare positional ${CDS_*}/${config.*}/${secrets.*} placeholders, not just "--flag=" style command arguments. - Extend pathPatterns/coverage to include healthcheck.* alongside command/entrypoint/logging, since healthcheck probes leak secrets the same way. - Map rendered-compose findings back to the owning CDS module id (via a new _map_service_to_module helper reusing the renderer's service-naming rule) instead of the raw Compose service name, which only coincidentally matched module ids in the fixture. - Replace the silent `except Exception: return None` in _try_render_compose_for_scan with a warning diagnostic (W096) so unexpected internal errors are surfaced instead of vanishing as zero findings. - Let callers pass an already-computed plan/rendered Compose YAML into run_security_validation(), and wire cli/main.py's `cds test` to do so, so the profile isn't planned and rendered twice per run. - Expand the regression fixture to exercise all three leak surfaces (flag-style, bare positional, healthcheck) and assert exact path/value/module attribution instead of loose substring checks. - Add coverage for the plan/render reuse behavior and for the new warning-diagnostic path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- CDS-SEC-070: remove the two match.any branches that could never fire against rendered output (keyRegex only tests the final path segment, which is a list index or ordinary leaf key on rendered command/ entrypoint/healthcheck/logging paths; and ${secrets.*}/${config.*} resolve or fail before this rule ever runs), leaving only the single working "${CDS_" branch. - cds test: pass skip_self_plan_render=True to run_security_validation when its own plan/render stages already failed, so _try_render_compose_for_scan doesn't retry (and re-fail) the same build_plan()/render_compose() calls a second time. - _try_render_compose_for_scan now emits a W096 warning diagnostic when planning or rendering fails (not just on unexpected exceptions), so `cds security` (which has no separate plan/render stage) can no longer silently report "No security findings" for a profile that never actually got scanned by rendered-compose-scoped rules. - run_security_validation now only plans+renders the profile when some enabled rule in the active rule set actually declares the "rendered-compose" scope, avoiding unconditional plan+render overhead on every security scan when no such rule applies. - Add regression tests for each of the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- rule-set.json: document CDS-SEC-070's scope limitation directly in its $comment (literal hardcoded secrets aren't caught, only unresolved ${CDS_*} placeholders) - main.py: drop the duplicate resolve_env_file_path() call in `cds test` by reusing the already-computed env_file - security.py: replace the plan/rendered_compose_yaml/ skip_self_plan_render parameter matrix with a single PrecomputedRender(plan=, rendered_compose_yaml=, failed=) object - security.py: W096 warnings now include the first underlying plan/render error code so the message is self-contained for `cds security` callers with no separate plan/render stage to consult - main.py: `cds security` now exits non-zero when rendered-compose scoped rules were skipped due to a plan/render failure, instead of reporting "No security findings." as if the scan completed - test_environment_classification.py: add an end-to-end regression test that actually plans/renders the real vault profile (building a full temp .env from its own secrets declarations) and asserts zero CDS-SEC-070 findings, making the "no false positive" claim CI-reproducible rather than only manually verified Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

CDS-SEC-070 is no longer a dead/deferred rule with scope: ["none"]; it's now evaluated against rendered Compose command/entrypoint/ healthcheck/logging fields ("rendered-compose" scope, PR #353, closing #297). Update T3's control/residual-risk description and the risk-prioritization summary to reflect the fix instead of the now-resolved dead-scope bug, and split the former "T3 / T4" combined entry since only T4 (CDS-SEC-013 disabled) remains an open gap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SemTiOne

RonaldHensbergen requested a review from SemTiOne as a code owner August 3, 2026 21:01

SemTiOne requested changes Aug 4, 2026

View reviewed changes

Comment thread cli/resources/rule-set.json Outdated

Comment thread cli/resources/rule-set.json

Comment thread cli/security.py Outdated

Comment thread cli/security.py Outdated

Comment thread cli/security.py Outdated

Comment thread cli/security.py Outdated

Comment thread tests/test_security.py

SemTiOne self-requested a review August 5, 2026 07:25

SemTiOne requested changes Aug 5, 2026

View reviewed changes

Comment thread cli/resources/rule-set.json Outdated

Comment thread cli/main.py Outdated

Comment thread cli/security.py

Comment thread cli/security.py Outdated

RonaldHensbergen requested a review from SemTiOne August 5, 2026 11:17

SemTiOne requested changes Aug 6, 2026

View reviewed changes

Comment thread cli/resources/rule-set.json

Comment thread cli/main.py

Comment thread tests/test_security.py

Comment thread cli/security.py

Comment thread cli/security.py

Comment thread cli/main.py Outdated

RonaldHensbergen and others added 5 commits August 6, 2026 15:18

RonaldHensbergen force-pushed the fix/cds-sec-070-rendered-compose-scan branch from 1fc156c to af12f26 Compare August 6, 2026 13:21

RonaldHensbergen requested a review from SemTiOne August 6, 2026 13:26

SemTiOne approved these changes Aug 7, 2026

View reviewed changes

RonaldHensbergen merged commit d36d00c into main Aug 7, 2026
11 checks passed

Conversation

RonaldHensbergen commented Aug 3, 2026

Summary

Fixed in this PR

Deferred (unchanged)

Testing

Uh oh!

SemTiOne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RonaldHensbergen commented Aug 5, 2026

Uh oh!

SemTiOne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RonaldHensbergen commented Aug 5, 2026

Uh oh!

SemTiOne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SemTiOne commented Aug 6, 2026

Uh oh!

RonaldHensbergen commented Aug 6, 2026

Uh oh!

SemTiOne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants