diff --git a/.changelog/5407.fixed b/.changelog/5407.fixed new file mode 100644 index 00000000000..e6c5f221467 --- /dev/null +++ b/.changelog/5407.fixed @@ -0,0 +1 @@ +`opentelemetry-configuration`: perform environment variable substitution on scalar values after parsing the configuration file, so `${VAR}` references inside comments and mapping keys are no longer substituted and undefined references in comments no longer abort loading diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 8b5488233d8..82327ce2db7 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -8,9 +8,24 @@ def substitute_env_vars(text: str) -> str: - """Substitute environment variables in configuration text. + """Substitute environment variables within a configuration value. + + A configuration value is a single value from the parsed configuration file + (the value of one key or one list item), never a key, a comment, or a whole + mapping/list. Substitution is applied per configuration value after the + file has been parsed, so comments and mapping keys are never touched. + + For example, given the YAML:: + + service_name: ${SERVICE_NAME} + endpoint: http://${HOST}:${PORT} + + the configuration values are ``${SERVICE_NAME}`` and + ``http://${HOST}:${PORT}``; this function is called once with each, and + never with the keys ``service_name`` or ``endpoint``. Supports the following syntax: + - ${VAR}: Substitute with environment variable VAR, or an empty value if VAR is not set. - ${VAR:-default}: Substitute with VAR if set, otherwise use default value. @@ -23,19 +38,19 @@ def substitute_env_vars(text: str) -> str: files be shared across languages. Args: - text: Configuration text with potential ${VAR} placeholders. + text: A configuration value with potential ${VAR} placeholders. Returns: - Text with environment variables substituted. + The configuration value with environment variables substituted. Examples: >>> os.environ["SERVICE_NAME"] = "my-service" - >>> substitute_env_vars("name: ${SERVICE_NAME}") - 'name: my-service' - >>> substitute_env_vars("name: ${MISSING:-default}") - 'name: default' - >>> substitute_env_vars("price: $$100") - 'price: $100' + >>> substitute_env_vars("${SERVICE_NAME}") + 'my-service' + >>> substitute_env_vars("${MISSING:-default}") + 'default' + >>> substitute_env_vars("$$100") + '$100' """ # Pattern matches $$ (escape sequence) or ${VAR_NAME} / ${VAR_NAME:-default_value} # Handling both in a single pass ensures $$ followed by ${VAR} works correctly @@ -57,23 +72,11 @@ def replace_var(match) -> str: # provided, otherwise with an empty value, per the spec. return default_value or "" - # Per spec: "It MUST NOT be possible to inject YAML structures by - # environment variables." Newlines are the primary injection vector — - # a value like "legit\nmalicious_key: val" would create extra YAML - # keys if substituted verbatim. Wrap such values in a YAML - # double-quoted scalar so the newline is treated as literal text. - # Simple values (no newlines) are returned as-is so that YAML type - # coercion still applies per spec ("Node types MUST be interpreted - # after environment variable substitution takes place"). - if "\n" in value or "\r" in value: - escaped = ( - value.replace("\\", "\\\\") - .replace('"', '\\"') - .replace("\n", "\\n") - .replace("\r", "\\r") - .replace("\t", "\\t") - ) - return f'"{escaped}"' + # Substitution runs on an already-parsed configuration value, so the + # result cannot inject new YAML structure regardless of its contents + # (a newline in the value stays a literal character within this one + # value). Type interpretation for standalone references is handled by + # the loader, which re-resolves the node tag after substitution. return value return re.sub(pattern, replace_var, text) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index fd1d96ffda0..f3a29aa7986 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -7,6 +7,7 @@ import json import logging import os +import re from pathlib import Path from typing import Any @@ -60,14 +61,167 @@ def _get_schema() -> dict: _logger = logging.getLogger(__name__) +_YAML_STR_TAG = "tag:yaml.org,2002:str" + +# A configuration value whose entire content is a single ``${VAR}`` / +# ``${VAR:-default}`` reference. Only such standalone references (when +# unquoted) have their type re-interpreted after substitution; embedded or +# multiple references resolve to a string per the configuration spec. A +# leading ``$$`` escape does not match, so ``$${VAR}`` is treated as an +# embedded (string) value. +_STANDALONE_ENV_REF = re.compile(r"\A\$\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\}\Z") + + +def _substitute_env_in_yaml_node( + node: yaml.Node, + loader: yaml.SafeLoader, + visited: set[int] | None = None, +): + """Apply env-var substitution to string configuration values in a node tree. + + Substitution runs after parsing on configuration values only, so comments + and mapping keys are never candidates (per the configuration spec). For an + unquoted standalone ``${VAR}`` reference the node's type tag is re-resolved + from the substituted value so YAML type coercion still applies (e.g. + ``${LIMIT}`` -> int); quoted or embedded references stay strings. + + Args: + node: Root of the node tree to walk. Substitution happens in place. + loader: The loader that composed ``node``; used to re-resolve tags. + visited: Ids of nodes already processed in this traversal. Callers pass + nothing; the walker creates and threads one set through the whole + tree. + """ + # Every anchor/alias pair shares one composed node, so the same node is + # reached once per reference to it. Each node must be substituted exactly + # once for two reasons: + # + # 1. Substituting twice re-reads the output of the first pass. An + # anchored ``$${TOKEN}`` becomes the escaped literal ``${TOKEN}`` on + # the first visit, which the second visit then resolves as a real + # variable -- silently defeating the ``$$`` escape. + # 2. A cyclic alias (``a: &anchor {self: *anchor}``) makes the node tree + # a graph with a loop, so an unguarded walk recurses until it raises + # ``RecursionError``. + # + # Tracking ``id(node)`` is safe here because ``root_node`` keeps every node + # in the tree alive for the whole traversal, so no id can be reused. + if visited is None: + visited = set() + if id(node) in visited: + return + visited.add(id(node)) + # Worked example: a node value that makes all three checks below true. + # + # YAML (with environment LIMIT=100): + # attribute_limits: + # attribute_count_limit: ${LIMIT} + # + # The parser hands this function the value node of attribute_count_limit, + # which has value="${LIMIT}", tag=str, style=None (plain/unquoted). Then: + # + # 1. isinstance(node, yaml.ScalarNode) -> True: it is a leaf value. + # 2. node.tag == _YAML_STR_TAG -> True: "${LIMIT}" is not a number or + # bool, so the parser tagged it str. We substitute in place, giving + # node.value = "100". + # 3. node.style is None and _STANDALONE_ENV_REF.match(raw) -> True: + # it was unquoted and the whole value is a single reference. We + # re-resolve the tag: resolve("100") -> int, and construct_document() + # later builds the integer 100 (not the string "100"). + # + # Counter-cases that reach check 3 but stop there: "${LIMIT}" quoted fails + # the style test; x${LIMIT} fails the regex. Both keep the str tag. + if isinstance(node, yaml.ScalarNode): + # A ``ScalarNode`` is a leaf -- one configuration value. Only a + # ``str``-tagged one can hold a ``${VAR}`` reference (nodes the parser + # already typed as int, float, bool or null cannot), so those are the + # only substitution candidates. + if node.tag == _YAML_STR_TAG: + # ``node.value`` is the raw text of this one value (never a key or + # comment). Replace any ${VAR} / ${VAR:-default} / $$ in it. + raw = node.value + node.value = substitute_env_vars(raw) + # The spec says node types are interpreted *after* substitution, + # but only for a value that is exactly one reference and written + # unquoted (``style is None``). Example: ``count: ${N}`` with + # ``N=42`` must yield the int 42, not the string "42". In that case + # re-run YAML's implicit type resolution on the substituted text + # and retag the node. A quoted (``"${N}"``) or embedded + # (``foo-${N}``) value keeps its ``str`` tag and stays a string. + if node.style is None and _STANDALONE_ENV_REF.match(raw): + # ``resolve`` returns the implicit tag (int/float/bool/null/ + # str) YAML would assign to ``node.value``; the ``(True, False)`` + # implicit-flag pair marks it as a plain scalar so that + # type-guessing applies, exactly as during normal parsing. + node.tag = loader.resolve(yaml.ScalarNode, node.value, (True, False)) + elif isinstance(node, yaml.SequenceNode): + # A sequence (list): each item is itself a value node -- recurse. + for item in node.value: + _substitute_env_in_yaml_node(item, loader, visited) + elif isinstance(node, yaml.MappingNode): + # A mapping (dict): recurse into values only; keys are not + # substitution candidates per the spec. A merge key (``<<: *base``) + # appears here as an ordinary pair whose value node is the anchored + # mapping itself, so ``visited`` also covers the merged-in values. + for _key_node, value_node in node.value: + _substitute_env_in_yaml_node(value_node, loader, visited) + + +def _substitute_env_in_json_value(value: Any) -> Any: + """Recursively apply env-var substitution to string values in JSON data. + + JSON has explicit types and no comments, so substitution applies only to + string values (the result stays a string) and mapping keys are left as-is. + """ + if isinstance(value, str): + return substitute_env_vars(value) + if isinstance(value, list): + return [_substitute_env_in_json_value(item) for item in value] + if isinstance(value, dict): + return {key: _substitute_env_in_json_value(val) for key, val in value.items()} + return value + + +def _parse_config_content(content: str, suffix: str, file_path: str | os.PathLike[str]) -> Any: + """Parse configuration text and substitute environment variables. + + Parsing happens first, so ``${VAR}`` references in comments and mapping + keys are never substitution candidates; substitution then runs on + configuration values, with YAML node types re-resolved for standalone + references. + + Raises: + ConfigurationError: If the content cannot be parsed or substitution of + a required environment variable fails. + """ + try: + if suffix == ".json": + return _substitute_env_in_json_value(json.loads(content)) + yaml_loader = yaml.SafeLoader(content) + try: + root_node = yaml_loader.get_single_node() + if root_node is None: + return None + _substitute_env_in_yaml_node(root_node, yaml_loader) + return yaml_loader.construct_document(root_node) + finally: + yaml_loader.dispose() + except yaml.YAMLError as exc: + _logger.exception("Failed to parse YAML from %s", file_path) + raise ConfigurationError(f"Failed to parse YAML: {exc}") from exc + except json.JSONDecodeError as exc: + _logger.exception("Failed to parse JSON from %s", file_path) + raise ConfigurationError(f"Failed to parse JSON: {exc}") from exc + def load_config_file( file_path: str | os.PathLike[str], ) -> OpenTelemetryConfiguration: """Load and parse an OpenTelemetry configuration file. - Supports YAML and JSON formats. Performs environment variable substitution - before parsing. + Supports YAML and JSON formats. Environment variable substitution is + performed after parsing, on configuration values only, so ``${VAR}`` + references in comments or mapping keys are left untouched. Args: file_path: Path to the configuration file (.yaml, .yml, or .json). @@ -99,28 +253,15 @@ def load_config_file( _logger.exception("Failed to read configuration file: %s", file_path) raise ConfigurationError(f"Failed to read configuration file: {file_path}") from exc - # Perform environment variable substitution - try: - content = substitute_env_vars(content) - except Exception as exc: - raise ConfigurationError(f"Environment variable substitution failed: {exc}") from exc - - # Parse based on file extension + # Parse the file, then substitute env vars in configuration values only. + # Parsing first means comments and mapping keys are never substitution + # candidates, and node types are still resolved after substitution. suffix = path.suffix.lower() - try: - if suffix in (".yaml", ".yml"): - data = yaml.safe_load(content) - elif suffix == ".json": - data = json.loads(content) - else: - _logger.error("Unsupported file format: %s", suffix) - raise ConfigurationError(f"Unsupported file format: {suffix}. Use .yaml, .yml, or .json") - except yaml.YAMLError as exc: - _logger.exception("Failed to parse YAML from %s", file_path) - raise ConfigurationError(f"Failed to parse YAML: {exc}") from exc - except json.JSONDecodeError as exc: - _logger.exception("Failed to parse JSON from %s", file_path) - raise ConfigurationError(f"Failed to parse JSON: {exc}") from exc + if suffix not in (".yaml", ".yml", ".json"): + _logger.error("Unsupported file format: %s", suffix) + raise ConfigurationError(f"Unsupported file format: {suffix}. Use .yaml, .yml, or .json") + + data = _parse_config_content(content, suffix, file_path) if data is None: _logger.error("Configuration file is empty: %s", file_path) diff --git a/opentelemetry-configuration/tests/file/test_env_substitution.py b/opentelemetry-configuration/tests/file/test_env_substitution.py index ff52b9d704c..2714d085435 100644 --- a/opentelemetry-configuration/tests/file/test_env_substitution.py +++ b/opentelemetry-configuration/tests/file/test_env_substitution.py @@ -109,35 +109,22 @@ def test_only_dollar_signs(self): result = substitute_env_vars("$$$$") self.assertEqual(result, "$$") - def test_newline_in_value_prevents_yaml_injection(self): - """Values containing newlines must not inject YAML structure. + def test_newline_in_value_returned_verbatim(self): + """A newline in a value is returned as-is. - Per spec: "It MUST NOT be possible to inject YAML structures by - environment variables." A value like "legit\\nmalicious_key: val" - must be emitted as a quoted scalar, not raw YAML. + Substitution runs per configuration value after parsing, so it does + not escape or quote newlines; YAML injection is prevented structurally + (see the loader tests), not by rewriting the value here. """ - with patch.dict( - os.environ, - {"SERVICE_NAME": "legit-service\nmalicious_key: injected_value"}, - ): - result = substitute_env_vars("file_format: '1.0'\nservice_name: ${SERVICE_NAME}") - parsed = yaml.safe_load(result) - self.assertNotIn("malicious_key", parsed) - self.assertIn("legit-service", parsed["service_name"]) - - def test_newline_in_value_preserved_as_literal(self): - """Newline within a value is preserved as a literal newline character.""" with patch.dict(os.environ, {"MULTI": "line1\nline2"}): - result = substitute_env_vars("key: ${MULTI}") - parsed = yaml.safe_load(result) - self.assertEqual(parsed["key"], "line1\nline2") + result = substitute_env_vars("${MULTI}") + self.assertEqual(result, "line1\nline2") - def test_carriage_return_in_value_is_escaped(self): - """Carriage return in value is escaped, not injected.""" + def test_carriage_return_in_value_returned_verbatim(self): + """A carriage return in a value is returned as-is, not escaped.""" with patch.dict(os.environ, {"VAL": "text\r\nmore"}): - result = substitute_env_vars("key: ${VAL}") - parsed = yaml.safe_load(result) - self.assertIsInstance(parsed["key"], str) + result = substitute_env_vars("${VAL}") + self.assertEqual(result, "text\r\nmore") def test_type_coercion_preserved_for_simple_values(self): """Simple values without newlines still undergo YAML type coercion per spec.""" diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index 9b13b371b65..4dc4629de66 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -7,6 +7,8 @@ from pathlib import Path from unittest.mock import patch +import yaml + from opentelemetry.configuration._tracer_provider import ( create_tracer_provider, ) @@ -17,6 +19,8 @@ from opentelemetry.configuration.file._loader import ( _SUPPORTED_SCHEMA_MAJOR, _SUPPORTED_SCHEMA_MINOR, + _substitute_env_in_json_value, + _substitute_env_in_yaml_node, ) from opentelemetry.configuration.models import ( BatchSpanProcessor as BatchSpanProcessorConfig, @@ -261,9 +265,9 @@ class TestConfigLoaderEndToEnd(unittest.TestCase): trace_id_ratio_based: {ratio: 0.5} """ - def _load(self, yaml: str | None = None) -> OpenTelemetryConfiguration: + def _load(self, yaml_content: str | None = None) -> OpenTelemetryConfiguration: with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as fh: - fh.write(self._YAML if yaml is None else yaml) + fh.write(self._YAML if yaml_content is None else yaml_content) path = fh.name try: return load_config_file(path) @@ -383,3 +387,169 @@ def test_malformed_version_is_rejected(self): with self.assertRaises(ConfigurationError) as ctx: self._load("not-a-version") self.assertIn("file_format", str(ctx.exception)) + + +def _substitute_yaml(text: str): + """Parse ``text``, run the node walker over it, and build the document. + + This is what ``_parse_config_content`` does for YAML, without the schema + validation, so walker behavior can be asserted on any document shape. + """ + loader = yaml.SafeLoader(text) + try: + node = loader.get_single_node() + _substitute_env_in_yaml_node(node, loader) + return loader.construct_document(node) + finally: + loader.dispose() + + +class TestEnvVarSubstitutionScope(unittest.TestCase): + """Substitution applies only to configuration values, per the config spec. + + These exercise the YAML node walker directly so the spec's type and + scope rules can be asserted without the JSON schema constraining shape. + """ + + def test_unquoted_standalone_reference_is_type_coerced(self): + with patch.dict(os.environ, {"N": "42", "FLAG": "true"}): + result = _substitute_yaml("count: ${N}\nflag: ${FLAG}") + self.assertEqual(result["count"], 42) + self.assertIsInstance(result["count"], int) + self.assertIs(result["flag"], True) + + def test_quoted_reference_stays_string(self): + with patch.dict(os.environ, {"N": "42"}): + result = _substitute_yaml('count: "${N}"') + self.assertEqual(result["count"], "42") + + def test_embedded_reference_resolves_to_string(self): + with patch.dict(os.environ, {"N": "42"}): + result = _substitute_yaml("name: svc-${N}") + self.assertEqual(result["name"], "svc-42") + + def test_mapping_key_is_not_substituted(self): + # A ${VAR} in a key position is left verbatim and triggers no lookup, + # so an undefined variable there does not raise. + with patch.dict(os.environ, {}, clear=True): + result = _substitute_yaml("${UNDEFINED_KEY}: value") + self.assertEqual(result, {"${UNDEFINED_KEY}": "value"}) + + def test_escape_sequence_is_not_a_reference(self): + with patch.dict(os.environ, {}, clear=True): + result = _substitute_yaml("literal: $${NOT_A_VAR}") + self.assertEqual(result["literal"], "${NOT_A_VAR}") + + def test_value_newline_cannot_inject_mapping_keys(self): + with patch.dict(os.environ, {"VAL": "legit\nmalicious_key: injected"}): + result = _substitute_yaml("service_name: ${VAL}") + self.assertEqual(list(result), ["service_name"]) + self.assertEqual(result["service_name"], "legit\nmalicious_key: injected") + + +class TestEnvVarSubstitutionWithAliases(unittest.TestCase): + """An anchored value is substituted once, however many aliases reach it. + + An alias resolves to the very same composed node as its anchor, so the + walker reaches one node once per reference. Substituting it more than once + would re-read the first pass's output and defeat the ``$$`` escape, and a + cyclic alias would recurse without end. + """ + + def test_aliased_reference_resolves_for_every_alias(self): + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml("a: &anchor ${TOKEN}\nb: *anchor\nc: *anchor") + self.assertEqual(result, {"a": "resolved", "b": "resolved", "c": "resolved"}) + + def test_aliased_escape_stays_literal(self): + # Without a visited set the first visit turns $${TOKEN} into the + # literal ${TOKEN} and the second resolves it, leaking TOKEN's value. + with patch.dict(os.environ, {"TOKEN": "leaked"}): + result = _substitute_yaml("a: &anchor $${TOKEN}\nb: *anchor\nc: *anchor") + self.assertEqual(result, {"a": "${TOKEN}", "b": "${TOKEN}", "c": "${TOKEN}"}) + + def test_aliased_reference_is_type_coerced_once(self): + with patch.dict(os.environ, {"N": "42"}): + result = _substitute_yaml("a: &anchor ${N}\nb: *anchor") + self.assertEqual(result["a"], 42) + self.assertIsInstance(result["a"], int) + self.assertEqual(result["b"], 42) + + def test_merge_key_escape_stays_literal(self): + # A merge key's value node is the anchored mapping itself, so the + # merged-in values are reached twice as well. + text = "base: &base\n x: $${TOKEN}\nderived:\n <<: *base\n y: 1\n" + with patch.dict(os.environ, {"TOKEN": "leaked"}): + result = _substitute_yaml(text) + self.assertEqual(result, {"base": {"x": "${TOKEN}"}, "derived": {"x": "${TOKEN}", "y": 1}}) + + def test_merge_key_resolves_reference(self): + text = "base: &base\n x: ${TOKEN}\nderived:\n <<: *base\n y: 1\n" + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml(text) + self.assertEqual(result, {"base": {"x": "resolved"}, "derived": {"x": "resolved", "y": 1}}) + + def test_cyclic_alias_terminates(self): + # A mapping that aliases itself makes the node tree a graph with a + # loop; the walk must end instead of raising RecursionError. + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml("a: &anchor\n self: *anchor\n value: ${TOKEN}") + self.assertIs(result["a"]["self"], result["a"]) + self.assertEqual(result["a"]["value"], "resolved") + + def test_cyclic_alias_in_sequence_terminates(self): + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml("a: &anchor\n - *anchor\n - ${TOKEN}") + self.assertIs(result["a"][0], result["a"]) + self.assertEqual(result["a"][1], "resolved") + + +class TestJsonEnvVarSubstitution(unittest.TestCase): + """JSON substitution touches only string values, not keys or non-strings.""" + + def test_string_values_substituted_keys_untouched(self): + with patch.dict(os.environ, {"V": "resolved"}): + result = _substitute_env_in_json_value({"${KEY}": "${V}", "nested": ["${V}", 1, True, None]}) + self.assertEqual( + result, + {"${KEY}": "resolved", "nested": ["resolved", 1, True, None]}, + ) + + +class TestEnvVarSubstitutionEndToEnd(unittest.TestCase): + """End-to-end loader behavior for issue #5406.""" + + @staticmethod + def _load_yaml(text: str) -> OpenTelemetryConfiguration: + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as fh: + fh.write(text) + path = fh.name + try: + return load_config_file(path) + finally: + os.unlink(path) + + def test_undefined_variable_in_comment_does_not_crash(self): + # The reported bug: a ${VAR} inside a comment must be ignored, so an + # undefined variable there no longer aborts loading. + text = "file_format: '1.0'\n# documented default uses ${UNDEFINED_VAR} - not substituted\ndisabled: false\n" + with patch.dict(os.environ, {}, clear=True): + config = self._load_yaml(text) + self.assertEqual(config.file_format, "1.0") + self.assertIs(config.disabled, False) + + def test_standalone_reference_coerces_type_for_schema(self): + # An integer field populated from ${VAR} must be an int so it passes + # JSON-schema validation. + text = "file_format: '1.0'\nattribute_limits:\n attribute_count_limit: ${LIMIT}\n" + with patch.dict(os.environ, {"LIMIT": "100"}): + config = self._load_yaml(text) + self.assertEqual(config.attribute_limits.attribute_count_limit, 100) + + def test_quoted_reference_for_int_field_fails_schema(self): + # Quoting forces a string, which is invalid for an integer field. + text = "file_format: '1.0'\nattribute_limits:\n attribute_count_limit: \"${LIMIT}\"\n" + with patch.dict(os.environ, {"LIMIT": "100"}): + with self.assertRaises(ConfigurationError) as ctx: + self._load_yaml(text) + self.assertIn("schema", str(ctx.exception).lower())