From f1a0c3302ff50f32cc5e236fcb5756ea129d9bf9 Mon Sep 17 00:00:00 2001 From: Ronald Hensbergen Date: Fri, 30 Oct 2026 23:18:54 +0100 Subject: [PATCH 1/3] fix(security): stop flagging renderer references and waiver text as secrets (#979) `cds security team-dagster-postgres-superset --environment prod` reported 11 false positives whose values are not secrets: - CDS-SEC-001/033 on initDbEnv.*_DB_PASSWORD and CDS-SEC-003 on initDbEnv.*_DB_{USER,PASSWORD}: the values are renderer references (config.passwordFrom, config.dagsterDatabase.username, ...) that the declarative match engine sees unresolved in the planned config, while the renderer resolves them via _merge_init_db_env/_resolve_expr and the rendered Compose output contains only ${CDS_*} placeholders. - CDS-SEC-003 on security.waivers.runtimePolicy.reason: the documented entropy exemption pattern spec.security.waivers.*.reason never matched because _flatten_profile_by_module emits spec-level keys without the spec. prefix. - CDS-SEC-031 on the project-root .env when the CLI was invoked from outside the project (e.g. CDS_PROFILE_PATH set): the env path was normalized against cwd only, so an absolute root .env matched */*.env. - CDS-SEC-013 on backup.encryption.keyFrom/encryptionKeyFrom: the shared secrets.backup_encryption_key reference was treated as a reused value. Changes: - New opt-in rule flag ignoreRendererReferences (rule-schema.json, rule set 1.0.1) set on CDS-SEC-001/003/033: values that are entirely a renderer reference (bare config.x/secrets.x/bindings.x/service.x/ certificates.x or the ${...}-wrapped form, per renderer._resolve_expr) are skipped, same as ${secrets.*} was already. - _check_secret_reuse skips renderer references. - The waiver-reason entropy exemption also matches the unprefixed security.waivers.*.reason path. - CDS-SEC-031 now matches the env file path relative to the project root (resolve_project_root, moved from cli/main.py to cli/security_common.py so the scan shares one implementation), so a bare project-root .env never fires regardless of invocation directory. - Regression tests for all four cases plus an end-to-end run of the reference prod profile; docs expected-findings table and CHANGELOG updated. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe --- CHANGELOG.md | 16 ++ cli/main.py | 22 +-- cli/resources/rule-schema.json | 5 + cli/resources/rule-set.json | 13 +- cli/security.py | 92 ++++++++-- cli/security_common.py | 22 +++ docs/production-hardening-checklist.md | 14 +- tests/test_security.py | 233 ++++++++++++++++++++++++- 8 files changed, 374 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 62840ff2..cc44f93a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,22 @@ The format is based on Keep a Changelog. ### Fixed +- Fixed `cds security` false positives on the reference production profile + (#979): the secret-value rules CDS-SEC-001/003/033 no longer flag values + that are entirely a renderer reference (`config.passwordFrom`, + `${secrets.x}`, ...), which the declarative engine sees unresolved in the + planned config but the renderer resolves to `${CDS_*}` placeholders at + render time (new opt-in `ignoreRendererReferences` rule flag, rule set + 1.0.1); the CDS-SEC-013 secret-reuse check skips such references as well; + the waiver-reason entropy exemption now also matches the flattened + `security.waivers.*.reason` path (spec-level keys flatten without the + `spec.` prefix); and CDS-SEC-031 matches the env file path relative to the + project root (the same marker walk `resolve_env_file_path` uses, now shared + via `cli/security_common.py`), so a bare project-root `.env` never fires + when the CLI is invoked from another directory (e.g. with + `CDS_PROFILE_PATH` set). + + - Fixed the `helm` render target lacking the provide-side contract-host cross-check that the compose target got for issue #1010: the Kubernetes renderer only validated the consume side (E084), so a provided diff --git a/cli/main.py b/cli/main.py index 5ea4cdbb..b1bc1114 100644 --- a/cli/main.py +++ b/cli/main.py @@ -62,6 +62,7 @@ NIS2_ARTICLE_21_MEASURES, SEVERITY_ORDER, infer_profile_class, + resolve_project_root, ) from .state import format_state_output, group_services_by_health, parse_compose_ps_json from .up_runner import ( @@ -738,27 +739,6 @@ def resolve_profile_path(profile: str | None) -> str: ) -def resolve_project_root(profile_path: str) -> Path: - """ - Resolve a project root for output artifacts. - - The resolver walks up from the selected profile location and picks the - first directory containing `.cds` (CDS's own state directory, checked - first so it takes priority over an unrelated ancestor repository), - `pyproject.toml`, or `.git`. If no marker is found, it falls back to the - current working directory. - """ - start = Path(profile_path).resolve().parent - for directory in [start, *start.parents]: - if ( - (directory / ".cds").exists() - or (directory / "pyproject.toml").exists() - or (directory / ".git").exists() - ): - return directory - return Path.cwd().resolve() - - def resolve_env_file_path(profile_path: str) -> Path: """ Resolve the default .env location for a profile. diff --git a/cli/resources/rule-schema.json b/cli/resources/rule-schema.json index eee81ff3..da2369b5 100644 --- a/cli/resources/rule-schema.json +++ b/cli/resources/rule-schema.json @@ -253,6 +253,11 @@ "type": "boolean", "default": true }, + "ignoreRendererReferences": { + "type": "boolean", + "default": false, + "description": "For rules that match on secret-like *values*: skip values that are entirely a renderer reference expression (bare 'config.x'/'secrets.x'/'bindings.x'/'service.x'/'certificates.x' or the ${...}-wrapped form). The declarative engine scans the planned config where those references are still unresolved strings, but the renderer resolves them before anything is executed (secrets.*/certificates.* become ${CDS_*} Compose placeholders), so they are pointers, not literal secrets (#979)." + }, "codeEnforced": { "type": "boolean", "default": true, diff --git a/cli/resources/rule-set.json b/cli/resources/rule-set.json index c8520bed..d7a326b5 100644 --- a/cli/resources/rule-set.json +++ b/cli/resources/rule-set.json @@ -1,6 +1,6 @@ { "$schema": "rule-schema.json", - "version": "1.0.0", + "version": "1.0.1", "metadata": { "name": "cds-core-security-rules", "description": "Core security validation rules for cds validate --security", @@ -30,7 +30,8 @@ "Move the value to environment variables or a secret backend.", "Replace literal values with references." ], - "enabled": true + "enabled": true, + "ignoreRendererReferences": true }, { "id": "CDS-SEC-002", @@ -81,7 +82,8 @@ "Store private keys in a secret backend or mounted secret file.", "Do not keep cryptographic material directly in profile YAML." ], - "enabled": true + "enabled": true, + "ignoreRendererReferences": true }, { "id": "CDS-SEC-004", @@ -439,7 +441,7 @@ }, { "id": "CDS-SEC-031", - "$comment": "Only the project-root .env is covered by this repo's `/.env` .gitignore entry (leading slash pins it to the root). Any nested .env (profile-scoped, rendered/generated output, or otherwise) is NOT covered by that ignore rule and can be committed accidentally, so the pathPatterns below only flag paths with at least one directory component before the filename; a bare root '.env' is the expected, already-ignored location and must not fire here.", + "$comment": "Only the project-root .env is covered by this repo's `/.env` .gitignore entry (leading slash pins it to the root). Any nested .env (profile-scoped, rendered/generated output, or otherwise) is NOT covered by that ignore rule and can be committed accidentally, so the pathPatterns below only flag paths with at least one directory component before the filename; a bare root '.env' is the expected, already-ignored location and must not fire here. The engine matches the env file path relative to the project root (resolved from the profile path, same marker walk as resolve_env_file_path), not relative to the invocation cwd, so an absolute project-root .env (e.g. when the CLI runs from another directory with CDS_PROFILE_PATH set) is still seen as the bare '.env' and never matches (#979).", "title": "Env file path appears repository-managed or unmanaged", "severity": "medium", "category": "file-output-safety", @@ -512,7 +514,8 @@ "Use environment or secret-backend references for sensitive fields.", "Keep profiles declarative and secret-free." ], - "enabled": true + "enabled": true, + "ignoreRendererReferences": true }, { "id": "CDS-SEC-040", diff --git a/cli/security.py b/cli/security.py index bb1a3ed2..f5c453dd 100644 --- a/cli/security.py +++ b/cli/security.py @@ -30,7 +30,12 @@ resolved_secret_values, ) from .secrets import load_secrets_from_env -from .security_common import SECRET_KEY_RE, SEVERITY_ORDER, infer_profile_class +from .security_common import ( + SECRET_KEY_RE, + SEVERITY_ORDER, + infer_profile_class, + resolve_project_root, +) from .utils import _compose_service_name _PROFILE_SCOPES = { @@ -192,29 +197,42 @@ def _flatten_env_secrets(secrets: dict[str, str]) -> list[tuple[str, str, Any]]: return [("", f"secrets.{key}", value) for key, value in secrets.items()] -def _normalize_scan_path(path: Path) -> str: +def _normalize_scan_path(path: Path, project_root: Path | None = None) -> str: + """ + Render a scanned file path relative to the project root when possible, so + the project-root `.env` is always the bare ".env" no matter which directory + the CLI was invoked from (e.g. with CDS_PROFILE_PATH set); fall back to a + cwd-relative path, then to the absolute path. + """ resolved = path.resolve() - try: - return resolved.relative_to(Path.cwd().resolve()).as_posix() - except ValueError: - return resolved.as_posix() + for base in (project_root, Path.cwd().resolve()): + if base is None: + continue + try: + return resolved.relative_to(base).as_posix() + except ValueError: + continue + return resolved.as_posix() def _flatten_env_inputs( secrets: dict[str, str], env_file: str | None, + project_root: Path | None = None, ) -> list[tuple[str, str, Any]]: """ Emit both loaded .env secrets and the env file path itself when present. Some rules evaluate how a secret-bearing env file is located or managed rather than inspecting individual secret values. Represent the file path as - a synthetic flat item so those rules can use the same matcher. + a synthetic flat item so those rules can use the same matcher. The path is + normalized against `project_root` (not just cwd) so a bare project-root + `.env` stays bare regardless of the invocation directory. """ items = _flatten_env_secrets(secrets) env_path = Path(env_file) if env_file is not None else Path(".env") if env_path.exists(): - scan_path = _normalize_scan_path(env_path) + scan_path = _normalize_scan_path(env_path, project_root) items.append(("", scan_path, scan_path)) return items @@ -223,7 +241,10 @@ def _is_windows() -> bool: return os.name == "nt" -def _flatten_file_permissions(env_file: str | None) -> list[tuple[str, str, Any]]: +def _flatten_file_permissions( + env_file: str | None, + project_root: Path | None = None, +) -> list[tuple[str, str, Any]]: """ Emit the POSIX permission bits of the secret-bearing .env file as a synthetic item whose value is the octal mode string (e.g. "0644"). @@ -240,7 +261,7 @@ def _flatten_file_permissions(env_file: str | None) -> list[tuple[str, str, Any] mode = stat.S_IMODE(env_path.stat().st_mode) except OSError: return [] - return [("", f"{_normalize_scan_path(env_path)}.permissions", f"{mode:04o}")] + return [("", f"{_normalize_scan_path(env_path, project_root)}.permissions", f"{mode:04o}")] def _permission_mode(value: str) -> int | None: @@ -329,6 +350,28 @@ def _is_secret_reference(value: Any) -> bool: return isinstance(value, str) and bool(_SECRET_REF_RE.match(value)) +# The renderer's reference vocabulary (cli/renderer.py's _resolve_expr), in +# both its bare form (e.g. "config.passwordFrom", "secrets.cache_password", +# used in module config positions the renderer resolves, such as initDbEnv) +# and its ${...}-wrapped form (e.g. "${config.passwordFrom}"). A value that +# is entirely such an expression is a pointer the renderer resolves at render +# time (secrets.*/certificates.* become ${CDS_*} Compose placeholders), never +# a literal secret, so secret-value rules that opt in via the rule's +# `ignoreRendererReferences` flag skip it (#979). +_RENDERER_REF_RE = re.compile( + r"^\$\{(?:secrets|certificates|config|bindings|service)\.[^}]+\}$" # ${config.x} + r"|^(?:secrets|certificates|config|bindings|service)\.[A-Za-z0-9_.]+$" # config.x +) + +def _is_renderer_reference(value: Any) -> bool: + """ + Returns True if value is entirely a renderer reference expression + (bare "config.x"/"secrets.x"/... or "${config.x}"/"${secrets.x}"/...), + i.e. a value the renderer resolves at render time rather than a literal. + """ + return isinstance(value, str) and bool(_RENDERER_REF_RE.match(value)) + + # --------------------------------------------------------------------------- # Profile class inference # --------------------------------------------------------------------------- @@ -394,9 +437,13 @@ def _redact(value: Any) -> str | None: # secret heuristic. Unlike _NON_SECRET_PATH_SUFFIXES, these only suppress the # check for the exact documented field, so an unrelated key that happens to # end in "reason" (e.g. a genuine secret named "authFailureReason") is still -# scanned normally. +# scanned normally. Both the "spec."-prefixed and the bare form are listed: +# _flatten_profile_by_module emits spec-level keys without the "spec." prefix +# (e.g. "security.waivers.runtimePolicy.reason"), while callers that flatten +# the raw profile document use the prefixed form. _NON_SECRET_PATH_PATTERNS = ( "spec.security.waivers.*.reason", + "security.waivers.*.reason", ) def _eval_condition( path: str, @@ -498,6 +545,12 @@ def _check_secret_reuse( for module_id, path, value in flat_items: if not isinstance(value, str) or not value: continue + # Renderer references (e.g. "secrets.backup_encryption_key") are + # pointers resolved at render time, not secret values: sharing one + # reference across fields is the intended indirection, not reuse of + # the underlying value (#979). + if _is_renderer_reference(value): + continue if not SECRET_KEY_RE.search(path.split(".")[-1]): continue value_to_locations.setdefault(value, []).append((module_id, path)) @@ -535,12 +588,20 @@ def _rule_matches( findings: list[dict[str, Any]] = [] match = rule["match"] + # Rules flagging secret-like *values* can opt out of renderer references + # (bare "config.x" or "${secrets.x}" expressions): the declarative engine + # scans the planned config where those references are still unresolved + # strings, but the renderer resolves them before anything is executed, + # and rendered output only ever contains ${CDS_*} placeholders (#979). + skip_renderer_refs = bool(rule.get("ignoreRendererReferences", False)) for module_id, path, value in flat_items: # Skip unresolved ${secrets.*} references in profile config. # They are intentional indirections, not real values. if _is_secret_reference(value): continue + if skip_renderer_refs and _is_renderer_reference(value): + continue key = path.split(".")[-1] if path else "" @@ -1382,8 +1443,13 @@ def run_security_validation( secrets, secret_diags = load_secrets_from_env(env_file) flat_profile = _flatten_profile_by_module(profile, profile_dir=profile_path.parent) - flat_env = _flatten_env_inputs(secrets, env_file) - flat_permissions = _flatten_file_permissions(env_file) + # Normalize scanned file paths (e.g. the .env path CDS-SEC-031 matches on) + # against the same project root resolve_env_file_path uses, so a bare + # project-root .env never matches regardless of the invocation directory + # (e.g. when CDS_PROFILE_PATH points at a profile while cwd is elsewhere). + scan_project_root = resolve_project_root(str(profile_path)) + flat_env = _flatten_env_inputs(secrets, env_file, project_root=scan_project_root) + flat_permissions = _flatten_file_permissions(env_file, project_root=scan_project_root) # Planning and rendering the profile is only useful when some enabled # rule actually declares the "rendered-compose" scope -- e.g. a custom diff --git a/cli/security_common.py b/cli/security_common.py index aaa0a379..ea937c3e 100644 --- a/cli/security_common.py +++ b/cli/security_common.py @@ -1,10 +1,32 @@ from __future__ import annotations import re +from pathlib import Path from typing import Any SEVERITY_ORDER = {"high": 0, "medium": 1, "low": 2} + +def resolve_project_root(profile_path: str) -> Path: + """ + Resolve a project root for output artifacts. + + The resolver walks up from the selected profile location and picks the + first directory containing `.cds` (CDS's own state directory, checked + first so it takes priority over an unrelated ancestor repository), + `pyproject.toml`, or `.git`. If no marker is found, it falls back to the + current working directory. + """ + start = Path(profile_path).resolve().parent + for directory in [start, *start.parents]: + if ( + (directory / ".cds").exists() + or (directory / "pyproject.toml").exists() + or (directory / ".git").exists() + ): + return directory + return Path.cwd().resolve() + # Closed set of informational compliance control categories a rule-set.json # rule can be tagged with (rule-schema.json's $defs.complianceCategory), # each mapped to a short display label plus the rationale documented in diff --git a/docs/production-hardening-checklist.md b/docs/production-hardening-checklist.md index f568cb69..2b2fd060 100644 --- a/docs/production-hardening-checklist.md +++ b/docs/production-hardening-checklist.md @@ -178,19 +178,27 @@ Expected: `cds state` shows every service healthy; the proxy's TLS entry points answer (for example `curl -v https:///`) and the backend ports answer only through the proxy. `cds down` stops the stack. -## 4. Expected findings (snapshot 2026-10-26) +## 4. Expected findings (snapshot 2026-10-30) `cds security team-dagster-postgres-superset --environment prod` currently exits 1 with the following findings. The set is stable; a deviation in either direction is the operator's signal to stop and investigate. +Renderer references (for example `config.passwordFrom` in `initDbEnv.*`, +`${secrets.*}` aliases) and the free-text waiver reason are no longer +flagged: since #979 the secret-value rules CDS-SEC-001/003/033 skip values +that are entirely a renderer reference (new `ignoreRendererReferences` rule +flag), the CDS-SEC-013 reuse check skips such references, the waiver-reason +entropy exemption covers the flattened `security.waivers.*.reason` path, and +CDS-SEC-031 matches the env file path relative to the project root so a bare +project-root `.env` never fires regardless of the invocation directory. +Rendered output still contains only `${CDS_*}` placeholders. + | Finding | Count | Classification | Action / tracking | | --- | --- | --- | --- | -| `CDS-SEC-001`/`CDS-SEC-033` on `initDbEnv.*`; `CDS-SEC-003` on `initDbEnv.*` and on the waiver reason | 11 | False positive: the flagged values are renderer references (for example `config.passwordFrom`) or free-text waiver reasons, not secrets; rendered output contains only `${CDS_*}` placeholders | [#979](https://github.com/RonaldHensbergen/composable-data-stack/issues/979) | | `CDS-SEC-074` on `services.dagster-webserver.ports[0]` and `services.superset.ports[0]` | 2 | True positive: the admin backends still publish directly to the host, allowing a proxy bypass | Remove the backend host publishes (proxy becomes the sole entry point), or set `spec.security.waivers.plaintextEndpointExposure.reason` | | `CDS-SEC-081` on the admin-facing services | 3 | Known gap: no identity module is bound | [#370](https://github.com/RonaldHensbergen/composable-data-stack/issues/370) | | `CDS-SEC-080` on `spec.modules` | 1 | Known gap: the backup policy is declared at profile level (`spec.backup`), which the rule does not yet recognize as contract consumption | [backup-restore.md](backup-restore.md) follow-ups ([#210](https://github.com/RonaldHensbergen/composable-data-stack/issues/210)) | -| `CDS-SEC-013` on `backup.encryption.keyFrom` / `backup.encryptionKeyFrom` | 2 | Accepted: one backup encryption key is deliberately referenced by both the module capability and the profile policy | None (single purpose, single key) | | `W103` on `spec.security.waivers.runtimePolicy` | 1 | Informational: the documented non-root waiver for images that set `USER` in their Dockerfiles | Keep the waiver reason accurate | Additional `CDS-SEC-004` (placeholder/weak secret) or `CDS-SEC-012` (too diff --git a/tests/test_security.py b/tests/test_security.py index 55f7afa5..66443ff8 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -13,9 +13,11 @@ _check_admin_service_identity_binding, _check_backup_target_binding, _check_production_plaintext_exposure, + _check_secret_reuse, _eval_condition, _flatten_profile_by_module, _flatten_rendered_leak_surfaces, + _is_renderer_reference, _map_service_to_module, _module_provides_plaintext_http, _module_provides_tls_reverse_proxy, @@ -24,6 +26,7 @@ _plaintext_module_ids_fronted_by_tls_reverse_proxy, _port_is_non_local_host_exposure, _redact, + _rule_matches, _try_render_compose_for_scan, _validate_rule_set, run_security_validation, @@ -39,7 +42,7 @@ class BundledSecurityRulesTest(unittest.TestCase): def test_default_rule_set_is_available_as_package_data(self): rule_set = _validate_rule_set() - self.assertEqual(rule_set["version"], "1.0.0") + self.assertEqual(rule_set["version"], "1.0.1") self.assertGreater(len(rule_set["rules"]), 0) def test_invalid_rule_set_raises_validation_error_with_paths(self): @@ -464,6 +467,234 @@ def test_does_not_flag_the_conventional_project_root_env_file(self): hits = [f for f in findings if f["rule_id"] == "CDS-SEC-031"] self.assertEqual(hits, [], "a bare project-root .env must not be flagged") + def test_root_env_file_not_flagged_when_invoked_outside_project_root(self): + """#979: with cwd outside the project (e.g. CDS_PROFILE_PATH set), the + project-root .env arrives as an absolute path; matching it relative to + the project root keeps it the bare '.env', which must not fire.""" + with tempfile.TemporaryDirectory() as project_dir, tempfile.TemporaryDirectory() as other_dir: + project_root = Path(project_dir) + (project_root / ".git").mkdir() # project-root marker + profile_path = project_root / "profile.yaml" + profile_path.write_text( + "apiVersion: cds/v1alpha1\nkind: Profile\n" + "metadata:\n name: tmp\n environment: local\n" + "spec:\n modules: []\n", + encoding="utf-8", + ) + root_env = project_root / ".env" + root_env.write_text("SOME_VAR=value\n", encoding="utf-8") + + original_cwd = Path.cwd() + try: + os.chdir(other_dir) + findings, _diags = run_security_validation( + profile_path, + _RULE_SCHEMA_PATH, + _RULE_SET_PATH, + env_file=str(root_env), + ) + finally: + os.chdir(original_cwd) + + hits = [f for f in findings if f["rule_id"] == "CDS-SEC-031"] + self.assertEqual( + hits, [], + "a project-root .env must not be flagged even when the CLI runs from another directory", + ) + + def test_nested_env_file_flagged_relative_to_project_root(self): + """#979: a nested .env is still flagged, with the path shown relative + to the project root rather than to the invocation cwd.""" + with tempfile.TemporaryDirectory() as project_dir, tempfile.TemporaryDirectory() as other_dir: + project_root = Path(project_dir) + (project_root / ".git").mkdir() + profile_path = project_root / "profile.yaml" + profile_path.write_text( + "apiVersion: cds/v1alpha1\nkind: Profile\n" + "metadata:\n name: tmp\n environment: local\n" + "spec:\n modules: []\n", + encoding="utf-8", + ) + nested_env = project_root / "nested" / ".env" + nested_env.parent.mkdir() + nested_env.write_text("SOME_VAR=value\n", encoding="utf-8") + + original_cwd = Path.cwd() + try: + os.chdir(other_dir) + findings, _diags = run_security_validation( + profile_path, + _RULE_SCHEMA_PATH, + _RULE_SET_PATH, + env_file=str(nested_env), + ) + finally: + os.chdir(original_cwd) + + hits = [f for f in findings if f["rule_id"] == "CDS-SEC-031"] + self.assertEqual(len(hits), 1) + self.assertEqual(hits[0]["path"], "nested/.env") + + +class RendererReferenceDetectionTest(unittest.TestCase): + def test_bare_and_wrapped_reference_forms_are_references(self): + for value in ( + "config.passwordFrom", + "config.dagsterDatabase.username", + "secrets.cache_password", + "bindings.db.host", + "service.host", + "certificates.web.cert", + "${config.passwordFrom}", + "${secrets.cache_password}", + "${bindings.db.host}", + ): + self.assertTrue(_is_renderer_reference(value), value) + + def test_literals_placeholders_and_mixed_strings_are_not_references(self): + for value in ( + "${CDS_ANALYTICS_DB_NAME}", + "db://${bindings.db.host}:5432", + "Sup3r-Secret-Literal-2026!", + "example", + "", + 123, + None, + ): + self.assertFalse(_is_renderer_reference(value), value) + + +class RendererReferenceSecretRuleTest(unittest.TestCase): + """#979: secret-value rules must not flag renderer references, which the + declarative engine sees unresolved in the planned config but the renderer + resolves (to ${CDS_*} placeholders) before anything executes.""" + + def _rule(self, rule_id): + return next(r for r in _validate_rule_set()["rules"] if r["id"] == rule_id) + + def test_reference_flagged_rules_skip_renderer_references(self): + items = [ + ("postgres", "initDbEnv.ANALYTICS_DB_PASSWORD", "config.passwordFrom"), + ("postgres", "initDbEnv.DAGSTER_DB_PASSWORD", "config.dagsterDatabase.passwordFrom"), + ("postgres", "initDbEnv.DAGSTER_DB_USER", "config.dagsterDatabase.username"), + ("postgres", "initDbEnv.SUPERSET_DB_USER", "config.supersetDatabase.username"), + ("postgres", "passwordFrom", "secrets.analytics_db_password"), + ("postgres", "secretKeyFrom", "${secrets.superset_secret_key}"), + ] + for rule_id in ("CDS-SEC-001", "CDS-SEC-003", "CDS-SEC-033"): + rule = self._rule(rule_id) + self.assertTrue(rule.get("ignoreRendererReferences"), rule_id) + self.assertEqual( + _rule_matches(rule, items, "prod"), [], + f"{rule_id} must not fire on renderer references", + ) + + def test_secret_value_rules_still_flag_literal_secrets(self): + items = [ + ("postgres", "initDbEnv.ANALYTICS_DB_PASSWORD", "Sup3r-Secret-Literal-2026!x"), + ("postgres", "passwordFrom", "hunter2-literal"), + ] + for rule_id in ("CDS-SEC-001", "CDS-SEC-003", "CDS-SEC-033"): + findings = _rule_matches(self._rule(rule_id), items, "prod") + self.assertTrue(findings, f"{rule_id} must still fire on literal secret values") + + def test_rules_without_the_flag_do_not_skip_references(self): + # The skip is an explicit per-rule opt-in: a rule that does not set + # ignoreRendererReferences still sees the raw reference value. + rule = { + "id": "CDS-SEC-999", + "severity": "low", + "match": {"any": [{"valueRegex": r"^config\."}]}, + "message": "m", + "recommendation": [], + } + items = [("postgres", "initDbEnv.X", "config.passwordFrom")] + self.assertEqual(len(_rule_matches(rule, items, "prod")), 1) + + def test_secret_reuse_check_skips_renderer_references(self): + items = [ + ("", "backup.encryption.keyFrom", "secrets.backup_encryption_key"), + ("postgres", "backup.encryptionKeyFrom", "secrets.backup_encryption_key"), + ] + self.assertEqual(_check_secret_reuse(items), []) + + def test_secret_reuse_check_still_flags_reused_literal_values(self): + items = [ + ("", "secrets.CDS_A_PASSWORD", "Shared-Literal-Value-2026!"), + ("", "secrets.CDS_B_PASSWORD", "Shared-Literal-Value-2026!"), + ] + findings = _check_secret_reuse(items) + self.assertEqual(len(findings), 2) + self.assertTrue(all(f["rule_id"] == "CDS-SEC-013" for f in findings)) + + +class WaiverReasonFlattenedPathTest(unittest.TestCase): + def test_entropy_rule_ignores_unprefixed_waiver_reason_path(self): + # _flatten_profile_by_module emits spec-level keys without the + # "spec." prefix, so the waiver reason flattens to + # "security.waivers.runtimePolicy.reason" (#979). + matched = _eval_condition( + path="security.waivers.runtimePolicy.reason", + key="reason", + value="Dagster and Superset images set a non-root USER in their Dockerfiles.", + cond={"entropy": "high", "minLength": 24}, + profile_class="prod", + ) + self.assertFalse(matched) + + +class ReferenceProfileRegressionTest(unittest.TestCase): + """#979 end-to-end: the reference production profile's initDbEnv + references, waiver reason, and shared key references produce no + CDS-SEC-001/003/013/033 findings.""" + + _ENV = "\n".join( + f"{k}={v}" + for k, v in { + "CDS_POSTGRES_SUPERUSER_PASSWORD": "Str0ng!Superuser#2026x", + "CDS_ANALYTICS_DB_PASSWORD": "An4lytics!Pass#2026x", + "CDS_DAGSTER_DB_PASSWORD": "D4gster!Pass#2026x", + "CDS_SUPERSET_DB_PASSWORD": "Sup3rset!Pass#2026x", + "CDS_SUPERSET_SECRET_KEY": "Sup3rsetSecretKey!2026x", + "CDS_SUPERSET_ADMIN_PASSWORD": "Adm1n!Pass#2026x", + "CDS_CACHE_PASSWORD": "C4che!Pass#2026x", + "CDS_BACKUP_ENCRYPTION_KEY": "B4ckup!Key#2026x", + "CDS_ANALYTICS_DB_NAME": "analytics", + "CDS_ANALYTICS_DB_USER": "analytics_user", + "CDS_DAGSTER_DB_NAME": "dagster", + "CDS_DAGSTER_DB_USER": "dagster_user", + "CDS_SUPERSET_DB_NAME": "superset", + "CDS_SUPERSET_DB_USER": "superset_user", + }.items() + ) + "\n" + + def test_team_prod_profile_reference_false_positives_are_gone(self): + profile_path = ( + _REPO_ROOT / "profiles" / "team-dagster-postgres-superset" / "profile.yaml" + ) + with tempfile.TemporaryDirectory() as tmp_dir: + env_file = Path(tmp_dir) / ".env" + env_file.write_text(self._ENV, encoding="utf-8") + findings, _diags = run_security_validation( + profile_path, + _RULE_SCHEMA_PATH, + _RULE_SET_PATH, + env_file=str(env_file), + environment="prod", + ) + + false_positives = [ + f for f in findings + if f["rule_id"] in {"CDS-SEC-001", "CDS-SEC-003", "CDS-SEC-013", "CDS-SEC-033"} + and ( + "initDbEnv" in f["path"] + or "waivers" in f["path"] + or f["path"].endswith("keyFrom") + or f["path"].endswith("encryptionKeyFrom") + ) + ] + self.assertEqual(false_positives, []) + class DeferredNoneScopeRuleDocumentationTest(unittest.TestCase): def test_each_remaining_none_scope_rule_declares_why_it_is_deferred(self): From a691f9bf55066c2aa6a0b4bedc22ca7bf7e944a4 Mon Sep 17 00:00:00 2001 From: Ronald Hensbergen Date: Fri, 30 Oct 2026 23:52:07 +0100 Subject: [PATCH 2/3] fix(ci): align smoke-test expectations with #979, fix lint and SonarCloud findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - .github/workflows/ci.yml: the "Run end-to-end smoke test" step asserted `[FAIL] security` and exit 1 for `cds test local-dagster-postgres-superset` because CDS-SEC-001/003 used to fire on the initDbEnv renderer references. Since #979 those false positives are gone, so with CI's env-derived secret values the security stage passes (only the known MEDIUM CDS-SEC-080/081 gaps remain); the step now asserts `[PASS] security` and exit 0. Image verification is unchanged: the "Verify unverifiable images fail closed" step still expects `cds security --verify-images` to exit 1 (CDS-VER-*, cosign absent on the runner). - CHANGELOG.md: collapse a double blank line after the #979 entry (markdownlint MD012). - tests/test_security.py: factor the shared tmp-project/cwd-elsewhere setup of the two CDS-SEC-031 tests into one helper (SonarCloud new_duplicated_lines_density was 4.8 > 3.0 gate). - tests/test_cds_workflow.py: the init'd-project `cds test` still fails security, but now via CDS-SEC-012 on the generated placeholder .env values, not CDS-SEC-001/003 — docstring updated to match. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe --- .github/workflows/ci.yml | 21 ++++++++-------- CHANGELOG.md | 1 - tests/test_cds_workflow.py | 13 +++++----- tests/test_security.py | 49 +++++++++++++------------------------- 4 files changed, 33 insertions(+), 51 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2e15853e..d80889bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -222,25 +222,24 @@ jobs: - name: Run end-to-end smoke test run: | - # This profile intentionally has HIGH security findings against it - # in CI (env-derived DB passwords look like hardcoded secrets to - # CDS-SEC-001/003/041, and cosign is not installed on this runner, - # so CDS-VER-001 also fires) — the same reason the "Verify - # unverifiable images fail closed" step below expects `cds - # security` to exit 1. `cds test` bundles security into its single - # exit code, so assert only that validate/plan/render passed and - # that the sole failing stage is "security", rather than requiring - # the whole command to exit 0. + # With the env-derived secret values set above, the security stage + # passes: since #979 the renderer references in initDbEnv.* and + # the waiver reason no longer false-positive as hardcoded secrets, + # and only the known MEDIUM gaps (CDS-SEC-080/081) remain, which + # do not fail the stage. Image verification is separate: cosign is + # not installed on this runner, so the "Verify unverifiable images + # fail closed" step below still expects `cds security + # --verify-images` to exit 1 with CDS-VER-* findings. set +e test_output="$(cds test local-dagster-postgres-superset 2>&1)" test_status=$? set -e echo "$test_output" grep -Eq "^\[PASS\] validate$" <<<"$test_output" - grep -Eq "^\[FAIL\] security$" <<<"$test_output" + grep -Eq "^\[PASS\] security$" <<<"$test_output" grep -Eq "^\[PASS\] plan$" <<<"$test_output" grep -Eq "^\[PASS\] render$" <<<"$test_output" - test "$test_status" -eq 1 + test "$test_status" -eq 0 - name: Generate MVP plan run: cds plan local-dagster-postgres-superset --output /tmp/plan.json diff --git a/CHANGELOG.md b/CHANGELOG.md index cc44f93a..66defd9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,6 @@ The format is based on Keep a Changelog. when the CLI is invoked from another directory (e.g. with `CDS_PROFILE_PATH` set). - - Fixed the `helm` render target lacking the provide-side contract-host cross-check that the compose target got for issue #1010: the Kubernetes renderer only validated the consume side (E084), so a provided diff --git a/tests/test_cds_workflow.py b/tests/test_cds_workflow.py index 2c19f4fc..b117a332 100644 --- a/tests/test_cds_workflow.py +++ b/tests/test_cds_workflow.py @@ -91,12 +91,13 @@ def test_commands(self): def test_test_command(self): """`cds test` bundles validate/security/plan/render into one exit code. - This profile's committed secret values (from `cds init`) are the - same kind of low-entropy, non-external-reference values CI supplies - via env vars, so the security stage always reports HIGH findings - (CDS-SEC-001/003) here -- the same reason CI's "Run end-to-end smoke - test" step (see .github/workflows/ci.yml) asserts this specific - per-stage outcome instead of requiring `cds test` to exit 0. + The init'd project's committed `.env` holds generated placeholder + secret values, so the security stage always reports HIGH findings + (CDS-SEC-012 weak/predictable values on those literals) here and + `cds test` exits 1. CI's "Run end-to-end smoke test" step + (.github/workflows/ci.yml) instead supplies env vars and, since + #979 removed the renderer-reference false positives, expects every + stage to pass there. """ env_with_profile = {"CDS_PROFILE_PATH": self.profile_file} diff --git a/tests/test_security.py b/tests/test_security.py index 66443ff8..deef2163 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -467,10 +467,11 @@ def test_does_not_flag_the_conventional_project_root_env_file(self): hits = [f for f in findings if f["rule_id"] == "CDS-SEC-031"] self.assertEqual(hits, [], "a bare project-root .env must not be flagged") - def test_root_env_file_not_flagged_when_invoked_outside_project_root(self): - """#979: with cwd outside the project (e.g. CDS_PROFILE_PATH set), the - project-root .env arrives as an absolute path; matching it relative to - the project root keeps it the bare '.env', which must not fire.""" + def _scan_env_from_outside_project(self, env_rel: str) -> list[dict]: + """Build a throwaway project (`.git` marker + minimal profile + an + .env at `env_rel`), then run the scan from an unrelated cwd with the + env file passed as an absolute path — the CDS_PROFILE_PATH scenario + from #979. Returns the full findings list.""" with tempfile.TemporaryDirectory() as project_dir, tempfile.TemporaryDirectory() as other_dir: project_root = Path(project_dir) (project_root / ".git").mkdir() # project-root marker @@ -481,8 +482,9 @@ def test_root_env_file_not_flagged_when_invoked_outside_project_root(self): "spec:\n modules: []\n", encoding="utf-8", ) - root_env = project_root / ".env" - root_env.write_text("SOME_VAR=value\n", encoding="utf-8") + env_file = project_root / env_rel + env_file.parent.mkdir(parents=True, exist_ok=True) + env_file.write_text("SOME_VAR=value\n", encoding="utf-8") original_cwd = Path.cwd() try: @@ -491,11 +493,17 @@ def test_root_env_file_not_flagged_when_invoked_outside_project_root(self): profile_path, _RULE_SCHEMA_PATH, _RULE_SET_PATH, - env_file=str(root_env), + env_file=str(env_file), ) finally: os.chdir(original_cwd) + return findings + def test_root_env_file_not_flagged_when_invoked_outside_project_root(self): + """#979: with cwd outside the project (e.g. CDS_PROFILE_PATH set), the + project-root .env arrives as an absolute path; matching it relative to + the project root keeps it the bare '.env', which must not fire.""" + findings = self._scan_env_from_outside_project(".env") hits = [f for f in findings if f["rule_id"] == "CDS-SEC-031"] self.assertEqual( hits, [], @@ -505,32 +513,7 @@ def test_root_env_file_not_flagged_when_invoked_outside_project_root(self): def test_nested_env_file_flagged_relative_to_project_root(self): """#979: a nested .env is still flagged, with the path shown relative to the project root rather than to the invocation cwd.""" - with tempfile.TemporaryDirectory() as project_dir, tempfile.TemporaryDirectory() as other_dir: - project_root = Path(project_dir) - (project_root / ".git").mkdir() - profile_path = project_root / "profile.yaml" - profile_path.write_text( - "apiVersion: cds/v1alpha1\nkind: Profile\n" - "metadata:\n name: tmp\n environment: local\n" - "spec:\n modules: []\n", - encoding="utf-8", - ) - nested_env = project_root / "nested" / ".env" - nested_env.parent.mkdir() - nested_env.write_text("SOME_VAR=value\n", encoding="utf-8") - - original_cwd = Path.cwd() - try: - os.chdir(other_dir) - findings, _diags = run_security_validation( - profile_path, - _RULE_SCHEMA_PATH, - _RULE_SET_PATH, - env_file=str(nested_env), - ) - finally: - os.chdir(original_cwd) - + findings = self._scan_env_from_outside_project("nested/.env") hits = [f for f in findings if f["rule_id"] == "CDS-SEC-031"] self.assertEqual(len(hits), 1) self.assertEqual(hits[0]["path"], "nested/.env") From 8b04e655b52c7092eb052ea6965f347e7ef0b0b2 Mon Sep 17 00:00:00 2001 From: Ronald Hensbergen Date: Sat, 31 Oct 2026 13:48:32 +0100 Subject: [PATCH 3/3] fix(k8s): share E150 contract-host candidate logic between compose and k8s renderers (#979) SonarCloud flagged 4.8% duplication on new code in PR #1039 (gate: <=3%): a 28-line block building and validating provided-contract host candidates was identical in cli/renderer.py::_validate_provided_contract_hosts and cli/k8s_renderer.py::_check_provided_contract_services. Extract it into cli/renderer.py::_contract_host_candidates and call it from both. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe --- cli/k8s_renderer.py | 20 ++----------------- cli/renderer.py | 47 ++++++++++++++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/cli/k8s_renderer.py b/cli/k8s_renderer.py index 997eb43d..638ea654 100644 --- a/cli/k8s_renderer.py +++ b/cli/k8s_renderer.py @@ -38,9 +38,8 @@ from .confinement import plan_declares_confinement from .diagnostics import Diagnostic from .renderer import ( - _DNS_LABEL_RE, _build_context, - _endpoint_hostname, + _contract_host_candidates, _merge_init_db_env, _resolve_expr, _resolve_profile_dir, @@ -1920,22 +1919,7 @@ def _check_provided_contract_services( spec = provided.get("spec") if not isinstance(spec, dict): continue - candidates: list[tuple[str, str | None]] = [] - if kind == "http-service": - advertised_service = spec.get("serviceName") - if isinstance(advertised_service, str) and advertised_service: - candidates.append( - ("serviceName", _compose_service_name(module_id, advertised_service)) - ) - else: - candidates.append(("host", spec.get("host"))) - elif kind in ("sql-database", "cache-service"): - candidates.append(("host", spec.get("host"))) - elif kind == "object-storage": - candidates.append(("endpoint", _endpoint_hostname(spec.get("endpoint")))) - for field, host in candidates: - if not isinstance(host, str) or not _DNS_LABEL_RE.match(host): - continue + for field, host in _contract_host_candidates(module_id, kind, spec): if field == "serviceName": host = service_names.get(host, host) if host in rendered: diff --git a/cli/renderer.py b/cli/renderer.py index 8077c4f7..b09e2191 100644 --- a/cli/renderer.py +++ b/cli/renderer.py @@ -282,6 +282,36 @@ def _endpoint_hostname(value: Any) -> str | None: return value +def _contract_host_candidates( + module_id: str, kind: Any, spec: dict[str, Any] +) -> list[tuple[str, str]]: + """ + Collect the (field, host) pairs a provided contract advertises, filtered + to cluster-local names. Hosts outside the rendered namespace (dots, + ports, credentials) cannot be cross-checked against rendered services, + so they are dropped here. Shared by the compose and Kubernetes E150 + checks. + """ + raw: list[tuple[str, Any]] = [] + if kind == "http-service": + advertised_service = spec.get("serviceName") + if isinstance(advertised_service, str) and advertised_service: + raw.append( + ("serviceName", _compose_service_name(module_id, advertised_service)) + ) + else: + raw.append(("host", spec.get("host"))) + elif kind in ("sql-database", "cache-service"): + raw.append(("host", spec.get("host"))) + elif kind == "object-storage": + raw.append(("endpoint", _endpoint_hostname(spec.get("endpoint")))) + return [ + (field, host) + for field, host in raw + if isinstance(host, str) and _DNS_LABEL_RE.match(host) + ] + + def _validate_provided_contract_hosts( plan: dict[str, Any], module_service_names: dict[str, list[str]], @@ -316,22 +346,7 @@ def _validate_provided_contract_hosts( spec = provided.get("spec") if not isinstance(spec, dict): continue - candidates: list[tuple[str, str | None]] = [] - if kind == "http-service": - advertised_service = spec.get("serviceName") - if isinstance(advertised_service, str) and advertised_service: - candidates.append( - ("serviceName", _compose_service_name(module_id, advertised_service)) - ) - else: - candidates.append(("host", spec.get("host"))) - elif kind in ("sql-database", "cache-service"): - candidates.append(("host", spec.get("host"))) - elif kind == "object-storage": - candidates.append(("endpoint", _endpoint_hostname(spec.get("endpoint")))) - for field, host in candidates: - if not isinstance(host, str) or not _DNS_LABEL_RE.match(host): - continue + for field, host in _contract_host_candidates(module_id, kind, spec): if host in rendered: continue diagnostics.append(Diagnostic(