From 3cabc3f1559d94ef572c8cbc22f4b2effecb5840 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Fri, 24 Jul 2026 10:31:40 +0200 Subject: [PATCH] feat(packagesettings): add `ExternalCommands` model for env filtering Add `ExternalCommands` Pydantic model with `keep_env` / `delete_env` pattern lists, a `filter_env()` method, and a `DEFAULT_KEEP_ENV` class variable for essential variables (HOME, PATH, LC_*, TERM, TZ, TMPDIR, etc.). `delete_env` matching is case-insensitive so credentials cannot slip through due to unexpected capitalisation. Non-POSIX env var keys are always stripped by `filter_env()`. Not yet wired into `external_commands.run()`. See: #1083 Co-Authored-By: Claude Signed-off-by: Christian Heimes --- docs/proposals/filter-env.md | 30 ++- src/fromager/packagesettings/__init__.py | 2 + src/fromager/packagesettings/_models.py | 162 +++++++++++++++- src/fromager/packagesettings/_settings.py | 20 +- tests/test_packagesettings.py | 221 ++++++++++++++++++++++ 5 files changed, 423 insertions(+), 12 deletions(-) diff --git a/docs/proposals/filter-env.md b/docs/proposals/filter-env.md index 34fe81aaf..e0158aa7e 100644 --- a/docs/proposals/filter-env.md +++ b/docs/proposals/filter-env.md @@ -68,20 +68,30 @@ validation error. ### Evaluation order -`keep_env` is evaluated before `delete_env`. All checks are case-insensitive -and short-circuit. If a variable matches the hard-coded always-keep set or -an entry in `keep_env`, then the variable is kept. +`keep_env` is evaluated before `delete_env`. If a variable matches the +hard-coded always-keep set or an entry in `keep_env`, then the variable +is kept. + +`delete_env` matching is **case-insensitive** for maximum convenience +and security -- a pattern `aws_*` removes `AWS_SECRET_ACCESS_KEY` +regardless of casing, so credentials cannot slip through due to +unexpected capitalisation. `keep_env` and the always-keep set are +case-sensitive, matching the exact variable names used in practice. For each variable in `os.environ`: -1. If it is in a hard-coded always-keep set -- **keep**, regardless of +1. If the key is not a valid POSIX name (`[A-Za-z_][A-Za-z0-9_]*`) -- + **delete**. This removes keys with dashes, dots, embedded spaces, + or bash-exported function definitions (`BASH_FUNC_*%%`) that no + build script should need. +2. If it is in a hard-coded always-keep set -- **keep**, regardless of configuration. The always-keep set contains variables required for - basic subprocess operation and proxy settings: `HOME`, `HOSTNAME`, - `LANG`, `LANGUAGE`, `LC_*`, `LOGNAME`, `NO_COLOR`, `PATH`, `SHELL`, - `USER`, `http_proxy`, `https_proxy`, `no_proxy`. -2. If any `keep_env` entry matches -- **keep**. -3. If any `delete_env` entry matches -- **delete**. -4. Otherwise -- **keep** (default passthrough). + basic subprocess operation: `HOME`, `HOSTNAME`, `LANG`, `LANGUAGE`, + `LC_*`, `LOGNAME`, `NO_COLOR`, `PATH`, `SHELL`, `TEMP`, `TERM`, + `TMP`, `TMPDIR`, `TZ`, `USER`. +3. If any `keep_env` entry matches (case-sensitive) -- **keep**. +4. If any `delete_env` entry matches (case-insensitive) -- **delete**. +5. Otherwise -- **keep** (default passthrough). `delete_env: ['*']` can be used to prevent passthrough. It filters all variables that neither match the always-keep set nor `keep_env` entries. diff --git a/src/fromager/packagesettings/__init__.py b/src/fromager/packagesettings/__init__.py index 6289d742b..abca2f21d 100644 --- a/src/fromager/packagesettings/__init__.py +++ b/src/fromager/packagesettings/__init__.py @@ -4,6 +4,7 @@ from ._models import ( BuildOptions, DownloadSource, + ExternalCommands, GitOptions, PackageSettings, ProjectOverride, @@ -57,6 +58,7 @@ "DownloadSource", "EnvKey", "EnvVars", + "ExternalCommands", "GitHubTagCloneResolver", "GitHubTagDownloadResolver", "GitLabTagCloneResolver", diff --git a/src/fromager/packagesettings/_models.py b/src/fromager/packagesettings/_models.py index 4e982e186..5ee6a3635 100644 --- a/src/fromager/packagesettings/_models.py +++ b/src/fromager/packagesettings/_models.py @@ -5,6 +5,7 @@ import logging import os import pathlib +import re import typing from collections.abc import Mapping @@ -12,7 +13,7 @@ import yaml from packaging.requirements import Requirement from packaging.utils import canonicalize_name -from pydantic import AnyUrl, Field +from pydantic import AnyUrl, Field, PrivateAttr, StringConstraints from pydantic_core import core_schema # from ._resolver import SourceResolver @@ -72,6 +73,165 @@ class SbomSettings(pydantic.BaseModel): """ +# Environment variable filter patterns for ExternalCommands. +# Pattern: starts with letter or underscore, rest is letters/digits/underscores, +# optionally ending with ``*`` (trailing wildcard). +# DeleteEnvPattern additionally allows bare ``*`` (catch-all). +KeepEnvPattern = typing.Annotated[ + str, + StringConstraints(pattern=r"^[a-zA-Z_][a-zA-Z0-9_]*\*?$"), +] + +DeleteEnvPattern = typing.Annotated[ + str, + StringConstraints(pattern=r"^(\*|[a-zA-Z_][a-zA-Z0-9_]*\*?)$"), +] + + +# POSIX.1-2024 sec. 8.1: environment variable names consist of uppercase +# letters, digits, and underscores and do not begin with a digit. We +# also accept lowercase letters for portability (common on Linux). +_POSIX_ENV_KEY_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") + + +def _compile_env_patterns( + patterns: tuple[str, ...], + *, + case_insensitive: bool = False, +) -> re.Pattern[str]: + """Compile env filter patterns into a single regex for ``fullmatch``. + + Exact patterns (e.g. ``HOME``) become ``HOME`` and prefix patterns + (e.g. ``LC_*``) become ``LC_.*``. The result is + ``HOME|LC_.*|...`` (used with ``fullmatch``). + + *patterns* must be non-empty. + """ + parts: list[str] = [] + for p in patterns: + if p.endswith("*"): + parts.append(re.escape(p[:-1]) + ".*") + else: + parts.append(re.escape(p)) + flags = re.IGNORECASE if case_insensitive else 0 + return re.compile("|".join(parts), flags) + + +class ExternalCommands(pydantic.BaseModel): + """Environment variable filtering for subprocesses. + + Variables whose keys are not valid POSIX names are always removed. + A hard-coded set of variables required for basic subprocess + operation (see ``DEFAULT_KEEP_ENV``) is always kept. User-supplied + ``keep_env`` patterns are evaluated before ``delete_env`` patterns. + + :: + + external_commands: + keep_env: + - "CARGO_*" + delete_env: + - "CI_TOKEN" + - "AWS_*" + + .. versionadded:: 0.92.0 + """ + + model_config = MODEL_CONFIG + + DEFAULT_KEEP_ENV: typing.ClassVar[tuple[str, ...]] = ( + "HOME", + "HOSTNAME", + "LANG", + "LANGUAGE", + "LC_*", + "LOGNAME", + "NO_COLOR", + "PATH", + "SHELL", + "TEMP", + "TERM", + "TMP", + "TMPDIR", + "TZ", + "USER", + ) + """Patterns always kept regardless of user configuration.""" + + keep_env: list[KeepEnvPattern] = Field(default_factory=list) + """Allowlist patterns (evaluated before ``delete_env``)""" + + delete_env: list[DeleteEnvPattern] = Field(default_factory=list) + """Blocklist patterns (evaluated after ``keep_env``)""" + + _keep_re: re.Pattern[str] | None = PrivateAttr(default=None) + _delete_re: re.Pattern[str] | None = PrivateAttr(default=None) + + @pydantic.model_validator(mode="after") + def validate_delete_env(self) -> typing.Self: + """Validate ``delete_env`` for conflicts and redundancy.""" + if not self.delete_env: + return self + if "*" in self.delete_env and len(self.delete_env) > 1: + raise ValueError( + "delete_env: bare '*' must be the only entry, " + "additional patterns are redundant" + ) + # Exact string overlap check. This catches obvious + # configuration mistakes (e.g. ``delete_env: [HOME]``) but does + # not detect all conflicts — for example ``delete_env: [LC_ALL]`` + # is not flagged even though ``LC_ALL`` matches the default keep + # pattern ``LC_*``. + keep = set(self.DEFAULT_KEEP_ENV) | set(self.keep_env) + overlap = keep & set(self.delete_env) + if overlap: + raise ValueError( + f"delete_env overlaps with keep_env / DEFAULT_KEEP_ENV: " + f"{sorted(overlap)}" + ) + return self + + def __bool__(self) -> bool: + """True when filtering is configured (``delete_env`` is non-empty).""" + return bool(self.delete_env) + + def model_post_init(self, __context: typing.Any) -> None: + """Pydantic post init hook to initialize internal data structures""" + if self.delete_env: + self._keep_re = _compile_env_patterns( + self.DEFAULT_KEEP_ENV + tuple(self.keep_env) + ) + if "*" not in self.delete_env: + self._delete_re = _compile_env_patterns( + tuple(self.delete_env), case_insensitive=True + ) + + def filter_env(self, env: Mapping[str, str]) -> Mapping[str, str]: + """Filter environment variables by keep/delete patterns. + + Variables whose keys are not valid POSIX names are always + removed first. Of the remaining variables, those matching + ``DEFAULT_KEEP_ENV`` or ``keep_env`` are always kept. Of the + rest, those matching ``delete_env`` are removed. Variables + matching neither list are kept. + """ + # Remove keys that are not valid POSIX names, e.g. keys with + # dashes, dots, spaces, or bash function exports (BASH_FUNC_*%%). + env = {k: v for k, v in env.items() if _POSIX_ENV_KEY_RE.fullmatch(k)} + # _keep_re is only set in model_post_init when delete_env is + # non-empty, so None means no filtering is configured. + if self._keep_re is None: + return env + if self._delete_re is None: + # delete_env is ["*"]: keep only what matches + return {k: v for k, v in env.items() if self._keep_re.fullmatch(k)} + return { + k: v + for k, v in env.items() + if self._keep_re.fullmatch(k) or not self._delete_re.fullmatch(k) + } + + class PurlConfig(pydantic.BaseModel): """Per-package purl configuration for SBOM generation. diff --git a/src/fromager/packagesettings/_settings.py b/src/fromager/packagesettings/_settings.py index 1d76c948a..888e6b225 100644 --- a/src/fromager/packagesettings/_settings.py +++ b/src/fromager/packagesettings/_settings.py @@ -13,7 +13,7 @@ from pydantic import Field from .. import overrides -from ._models import PackageSettings, SbomSettings +from ._models import ExternalCommands, PackageSettings, SbomSettings from ._pbi import PackageBuildInfo from ._typedefs import MODEL_CONFIG, GlobalChangelog, Package, Variant @@ -45,6 +45,16 @@ class SettingsFile(pydantic.BaseModel): are generated. """ + external_commands: ExternalCommands = Field(default_factory=ExternalCommands) + """Environment variable filtering for subprocesses + + Controls which environment variables are passed to child processes + using ``keep_env`` / ``delete_env`` patterns. Defaults to no + filtering. + + .. versionadded:: 0.92.0 + """ + @classmethod def from_string( cls, @@ -175,6 +185,14 @@ def sbom_settings(self) -> SbomSettings | None: """Get global SBOM settings, or None if SBOM generation is disabled.""" return self._settings.sbom + @property + def external_commands(self) -> ExternalCommands: + """Get external commands settings. + + .. versionadded:: 0.92.0 + """ + return self._settings.external_commands + def variant_changelog(self) -> list[str]: """Get global changelog for current variant""" return list(self._settings.changelog.get(self.variant, [])) diff --git a/tests/test_packagesettings.py b/tests/test_packagesettings.py index d16bba9ae..20e062edd 100644 --- a/tests/test_packagesettings.py +++ b/tests/test_packagesettings.py @@ -13,6 +13,7 @@ Annotations, BuildDirectory, EnvVars, + ExternalCommands, GitOptions, Package, PackageBuildInfo, @@ -23,6 +24,7 @@ Variant, substitute_template, ) +from fromager.packagesettings._models import DeleteEnvPattern, KeepEnvPattern from fromager.packagesettings._typedefs import PurlType, UpstreamPurl TEST_PKG = "test-pkg" @@ -931,3 +933,222 @@ def test_version_none_no_reference( result = pbi.get_extra_environ(template_env={}, version=None) assert result["FOO"] == "bar" assert "__version__" not in result + + +# --- ExternalCommands / env pattern tests --- + + +@pytest.mark.parametrize("valid", ["HOME", "CARGO_*", "X", "_FOO", "_1A*"]) +def test_type_keep_env_pattern_valid(valid: str) -> None: + """Verify `KeepEnvPattern` accepts well-formed patterns.""" + ta = pydantic.TypeAdapter(KeepEnvPattern) + assert ta.validate_python(valid) == valid + + +@pytest.mark.parametrize( + "invalid", ["*", "A*B", "", " ", " AWS_*", "AWS_* ", "1BAD", "foo-bar"] +) +def test_type_keep_env_pattern_invalid(invalid: str) -> None: + """Verify `KeepEnvPattern` rejects bare ``*``, mid-``*``, empty, whitespace, and bad chars.""" + ta = pydantic.TypeAdapter(KeepEnvPattern) + with pytest.raises(pydantic.ValidationError): + ta.validate_python(invalid) + + +@pytest.mark.parametrize("valid", ["CI_TOKEN", "AWS_*", "*", "_X"]) +def test_type_delete_env_pattern_valid(valid: str) -> None: + """Verify `DeleteEnvPattern` accepts valid patterns including bare ``*``.""" + ta = pydantic.TypeAdapter(DeleteEnvPattern) + assert ta.validate_python(valid) == valid + + +@pytest.mark.parametrize("invalid", ["A*B", "", " *", "1BAD", "foo-bar"]) +def test_type_delete_env_pattern_invalid(invalid: str) -> None: + """Verify `DeleteEnvPattern` rejects mid-``*``, empty, whitespace, and bad chars.""" + ta = pydantic.TypeAdapter(DeleteEnvPattern) + with pytest.raises(pydantic.ValidationError): + ta.validate_python(invalid) + + +def test_external_commands_valid() -> None: + """Valid configs, defaults, and frozen behavior.""" + # both lists + ec = ExternalCommands( + keep_env=["CARGO_*", "HOME"], + delete_env=["CI_TOKEN", "AWS_*"], + ) + assert ec.keep_env == ["CARGO_*", "HOME"] + assert ec.delete_env == ["CI_TOKEN", "AWS_*"] + + # catch-all delete + ec = ExternalCommands(delete_env=["*"]) + assert ec.keep_env == [] + assert ec.delete_env == ["*"] + + # defaults + ec = ExternalCommands() + assert ec.keep_env == [] + assert ec.delete_env == [] + + +@pytest.mark.parametrize( + "kwargs", + [ + pytest.param({"keep_env": ["*"]}, id="bare-star-keep"), + pytest.param({"keep_env": ["A*B"]}, id="mid-star"), + pytest.param({"delete_env": ["*", "AWS_*"]}, id="star-with-others"), + pytest.param({"delete_env": ["HOME"]}, id="delete-default-keep"), + pytest.param({"delete_env": ["LC_*"]}, id="delete-default-keep-prefix"), + pytest.param( + {"keep_env": ["CARGO_*"], "delete_env": ["CARGO_*"]}, + id="delete-overlaps-keep", + ), + pytest.param({"unknown_key": "value"}, id="extra-forbid"), + ], +) +def test_external_commands_invalid(kwargs: dict[str, typing.Any]) -> None: + """Invalid ``ExternalCommands`` configurations must raise.""" + with pytest.raises(pydantic.ValidationError): + ExternalCommands.model_validate(kwargs) + + +def test_settings_file_external_commands(tmp_path: pathlib.Path) -> None: + """Parse ``external_commands`` from YAML and access via ``Settings``.""" + # absent → falsy default + assert not SettingsFile.from_string("").external_commands + + # present + sf = SettingsFile.from_string( + """ +external_commands: + keep_env: + - "CARGO_*" + delete_env: + - "CI_TOKEN" +""" + ) + assert sf.external_commands + assert sf.external_commands.keep_env == ["CARGO_*"] + assert sf.external_commands.delete_env == ["CI_TOKEN"] + + # Settings property proxies SettingsFile + settings = Settings( + settings=sf, + package_settings=[], + variant="cpu", + patches_dir=tmp_path, + max_jobs=1, + ) + assert settings.external_commands is sf.external_commands + + settings_no_filter = Settings( + settings=SettingsFile(), + package_settings=[], + variant="cpu", + patches_dir=tmp_path, + max_jobs=1, + ) + assert not settings_no_filter.external_commands + + +# --- ExternalCommands.filter_env tests --- + +_ENV = {"HOME": "/h", "PATH": "/bin", "LC_ALL": "C", "SECRET": "s", "AWS_KEY": "k"} + + +def test_filter_env_no_delete() -> None: + """No delete_env configured keeps all POSIX-compliant keys.""" + ec = ExternalCommands() + assert ec.filter_env(_ENV) == _ENV + + +def test_filter_env_no_delete_strips_non_posix() -> None: + """Non-POSIX keys are removed even without delete_env.""" + ec = ExternalCommands() + env = {"HOME": "/h", "dot.key": "x", "BASH_FUNC_f%%": "y"} + assert ec.filter_env(env) == {"HOME": "/h"} + + +@pytest.mark.parametrize( + "keep,delete,env,expected", + [ + pytest.param( + [], + ["SECRET"], + _ENV, + {"HOME": "/h", "PATH": "/bin", "LC_ALL": "C", "AWS_KEY": "k"}, + id="exact-delete", + ), + pytest.param( + [], + ["AWS_*"], + _ENV, + {"HOME": "/h", "PATH": "/bin", "LC_ALL": "C", "SECRET": "s"}, + id="prefix-delete", + ), + pytest.param( + ["CI_SAFE"], + ["CI_*"], + {"CI_SAFE": "1", "CI_TOKEN": "x"}, + {"CI_SAFE": "1"}, + id="keep-protects-from-delete", + ), + pytest.param( + [], + ["*"], + _ENV, + {"HOME": "/h", "PATH": "/bin", "LC_ALL": "C"}, + id="delete-all", + ), + pytest.param( + ["AWS_*"], + ["*"], + _ENV, + {"HOME": "/h", "PATH": "/bin", "LC_ALL": "C", "AWS_KEY": "k"}, + id="delete-all-user-keep", + ), + pytest.param( + [], + ["CI_*"], + {"X": "1", "CI_T": "2"}, + {"X": "1"}, + id="unmatched-kept", + ), + pytest.param( + [], + ["secret"], + {"SECRET": "gone", "secret": "gone", "Other": "kept"}, + {"Other": "kept"}, + id="delete-case-insensitive", + ), + pytest.param( + ["my_var"], + ["*"], + {"my_var": "kept", "MY_VAR": "gone"}, + {"my_var": "kept"}, + id="keep-case-sensitive", + ), + pytest.param( + [], + ["SECRET"], + {"HOME": "/h", "SECRET": "s", "dot.key": "x", "BASH_FUNC_f%%": "y"}, + {"HOME": "/h"}, + id="non-posix-keys-removed", + ), + pytest.param( + ["KEEP_ME"], + ["*"], + {"KEEP_ME": "ok", "1BAD": "x", "k-v": "y"}, + {"KEEP_ME": "ok"}, + id="non-posix-keys-removed-delete-all", + ), + ], +) +def test_filter_env( + keep: list[str], + delete: list[str], + env: dict[str, str], + expected: dict[str, str], +) -> None: + ec = ExternalCommands(keep_env=keep, delete_env=delete) + assert ec.filter_env(env) == expected