From 7d9ac8d043009f8fcb3a9625829de770ac9d1d8d Mon Sep 17 00:00:00 2001 From: yucheng-berriai Date: Fri, 3 Jul 2026 17:56:45 -0700 Subject: [PATCH] fix(security): reject path traversal and control chars in key_alias-derived secret names (LIT-4201) key_alias becomes the secret name written to HashiCorp Vault and CyberArk Conjur when store_virtual_keys is enabled. Vault's get_url concatenated secret_name directly into the request URL and Conjur's _ensure_variable_exists interpolated it unescaped into a YAML policy body, so a malicious key_alias could path-traverse the Vault write or inject extra Conjur policy statements. The only existing guard was opt-in (enable_key_alias_format_validation, default off) and its charset still permitted ".." even when enabled. Add raise_if_unsafe_secret_name, an unconditional check rejecting ".." sequences and control characters (including the Unicode line breaks YAML treats the same as "\n": NEL, LINE SEPARATOR, PARAGRAPH SEPARATOR), applied at both vulnerable sinks and at the API boundary in _validate_key_alias_format, independent of the opt-in flag. Also close two adjacent gaps found in internal review: Vault's get_url now percent-encodes secret_name (preserving "/" and "@") so "#"/"?" can't turn into a URL fragment/query string instead of a literal path segment, and Conjur's policy YAML is now built with a real YAML serializer (yaml.safe_dump forced to double-quoted style) instead of raw string interpolation, so metacharacters that aren't on the traversal/control-char denylist (a bare ":" or "#") can no longer change the parsed policy structure. --- .../key_management_endpoints.py | 25 +++++- .../secret_managers/base_secret_manager.py | 21 +++++ .../cyberark_secret_manager.py | 10 ++- .../hashicorp_secret_manager.py | 9 +- tests/litellm_utils_tests/test_cyberark.py | 85 +++++++++++++++++++ tests/litellm_utils_tests/test_hashicorp.py | 55 ++++++++++++ .../test_key_management_endpoints.py | 44 +++++++++- .../test_base_secret_manager.py | 51 +++++++++++ 8 files changed, 291 insertions(+), 9 deletions(-) create mode 100644 tests/test_litellm/secret_managers/test_base_secret_manager.py diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index ff849240292..82061ba86ed 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -111,6 +111,7 @@ from litellm.repositories.verification_token_repository import ( VerificationTokenRepository, ) from litellm.router import Router +from litellm.secret_managers.base_secret_manager import raise_if_unsafe_secret_name from litellm.secret_managers.main import get_secret from litellm.types.proxy.management_endpoints.key_management_endpoints import ( BulkUpdateKeyRequest, @@ -6267,8 +6268,14 @@ def _validate_key_alias_format(key_alias: Optional[str]) -> None: """ Validate the format of the key_alias. - Gated behind ``litellm.enable_key_alias_format_validation`` (default **False**). - When disabled, no validation is performed so existing workflows are not broken. + key_alias can become the secret name written to an external secret manager + (HashiCorp Vault, CyberArk Conjur) when store_virtual_keys is enabled, so the + path-traversal / control-character check below always runs, regardless of + ``litellm.enable_key_alias_format_validation``. + + The remaining charset/length rules are gated behind + ``litellm.enable_key_alias_format_validation`` (default **False**). When disabled, + only the security check above is performed, so existing workflows are not broken. Rules (when enabled): - None is OK (no alias). @@ -6276,10 +6283,20 @@ def _validate_key_alias_format(key_alias: Optional[str]) -> None: - start/end with alphanumeric - only allow a-zA-Z0-9_-/.@ """ - if not litellm.enable_key_alias_format_validation: + if key_alias is None: return - if key_alias is None: + try: + raise_if_unsafe_secret_name(key_alias) + except ValueError as e: + raise ProxyException( + message=f"Invalid key_alias: {e}", + type=ProxyErrorTypes.bad_request_error, + param="key_alias", + code=400, + ) + + if not litellm.enable_key_alias_format_validation: return if not _KEY_ALIAS_PATTERN.match(key_alias): diff --git a/litellm/secret_managers/base_secret_manager.py b/litellm/secret_managers/base_secret_manager.py index d33d76093c9..3fc43b37b0b 100644 --- a/litellm/secret_managers/base_secret_manager.py +++ b/litellm/secret_managers/base_secret_manager.py @@ -1,3 +1,4 @@ +import re from abc import ABC, abstractmethod from typing import Any, Dict, Optional, Union @@ -5,6 +6,26 @@ import httpx from litellm import verbose_logger +# U+0085 (NEL), U+2028 (LINE SEPARATOR), and U+2029 (PARAGRAPH SEPARATOR) are line +# breaks under the YAML spec, same as backslash-n or backslash-r, so a denylist +# restricted to ASCII control characters is bypassable for YAML-based sinks. +_UNSAFE_SECRET_NAME_PATTERN = re.compile(r"\.\.|[\x00-\x1f\x7f-\x9f…

]") + + +def raise_if_unsafe_secret_name(secret_name: str) -> None: + """ + Reject secret names that could path-traverse a secret manager's API (e.g. HashiCorp + Vault, which builds its request URL by string concatenation) or inject control + characters into a policy document (e.g. CyberArk Conjur, which embeds the secret + name directly into a YAML policy body). + + Rejects any ".." substring rather than only a ".." path segment, since that is + the only traversal-safe answer for Vault's raw string-concatenated URL; a + version-like name such as "release-1.0..2" is rejected too. + """ + if _UNSAFE_SECRET_NAME_PATTERN.search(secret_name): + raise ValueError(f"Unsafe secret_name {secret_name!r}: must not contain '..' or control characters") + class BaseSecretManager(ABC): """ diff --git a/litellm/secret_managers/cyberark_secret_manager.py b/litellm/secret_managers/cyberark_secret_manager.py index faf6224757f..15fd65cc72a 100644 --- a/litellm/secret_managers/cyberark_secret_manager.py +++ b/litellm/secret_managers/cyberark_secret_manager.py @@ -4,6 +4,7 @@ from typing import Any, Dict, Optional, Union from urllib.parse import quote import httpx +import yaml import litellm from litellm._logging import verbose_logger @@ -15,7 +16,7 @@ from litellm.llms.custom_httpx.http_handler import ( ) from litellm.proxy._types import KeyManagementSystem -from .base_secret_manager import BaseSecretManager +from .base_secret_manager import BaseSecretManager, raise_if_unsafe_secret_name from .main import str_to_bool @@ -125,8 +126,13 @@ class CyberArkSecretManager(BaseSecretManager): """ # In production, we'd check if the variable exists first # For now, we'll attempt to create it and ignore if it already exists + raise_if_unsafe_secret_name(secret_name) policy_url = f"{self.conjur_addr}/policies/{self.conjur_account}/policy/root" - policy_yaml = f"- !variable {secret_name}\n" + # Force double-quoted scalar style so any YAML metacharacter in secret_name + # (newlines, other Unicode line breaks, colons, '#' comments) is escaped + # instead of being interpreted as new policy syntax. + quoted_name = yaml.safe_dump(secret_name, default_style='"').strip() + policy_yaml = f"- !variable {quoted_name}\n" try: client = _get_httpx_client(params={"ssl_verify": self.ssl_verify}) diff --git a/litellm/secret_managers/hashicorp_secret_manager.py b/litellm/secret_managers/hashicorp_secret_manager.py index bd1b1097347..b50017892dc 100644 --- a/litellm/secret_managers/hashicorp_secret_manager.py +++ b/litellm/secret_managers/hashicorp_secret_manager.py @@ -1,5 +1,6 @@ import os from typing import Any, Dict, Optional, Union +from urllib.parse import quote import httpx @@ -14,7 +15,7 @@ from litellm.llms.custom_httpx.http_handler import ( ) from litellm.proxy._types import KeyManagementSystem -from .base_secret_manager import BaseSecretManager +from .base_secret_manager import BaseSecretManager, raise_if_unsafe_secret_name class HashicorpSecretManager(BaseSecretManager): @@ -220,6 +221,7 @@ class HashicorpSecretManager(BaseSecretManager): - With custom mount: http://127.0.0.1:8200/v1/kv/data/mykey - With path prefix: http://127.0.0.1:8200/v1/secret/data/myapp/mykey """ + raise_if_unsafe_secret_name(secret_name) resolved_namespace = self._sanitize_path_component(namespace if namespace is not None else self.vault_namespace) resolved_mount = self._sanitize_path_component(mount_name if mount_name is not None else self.vault_mount_name) if resolved_mount is None: @@ -234,7 +236,10 @@ class HashicorpSecretManager(BaseSecretManager): _url += f"{resolved_mount}/data/" if resolved_path_prefix: _url += f"{resolved_path_prefix}/" - _url += secret_name + # Preserve "/" (hierarchical secret paths) and "@" (e.g. emails in aliases) + # unencoded; percent-encode everything else so a secret_name containing "#" + # or "?" can't turn into a URL fragment/query string instead of a path segment. + _url += quote(secret_name, safe="/@") return _url def _sanitize_plain_value(self, value: Optional[Union[str, int]]) -> Optional[str]: diff --git a/tests/litellm_utils_tests/test_cyberark.py b/tests/litellm_utils_tests/test_cyberark.py index 67575d3e781..73b27c5eb3f 100644 --- a/tests/litellm_utils_tests/test_cyberark.py +++ b/tests/litellm_utils_tests/test_cyberark.py @@ -5,6 +5,7 @@ Integration test for CyberArk Conjur Secret Manager. import os import sys import pytest +import yaml from dotenv import load_dotenv load_dotenv() @@ -42,6 +43,90 @@ def create_mock_response(status_code: int, text: str = ""): return mock_response +@pytest.mark.asyncio +async def test_cyberark_write_secret_rejects_yaml_injection(): + """ + Regression test: secret_name (derived from user-controlled key_alias) used to be + interpolated unescaped into the Conjur policy YAML body + (`f"- !variable {secret_name}\n"`), so a key_alias containing a newline plus + another policy directive would be sent as additional Conjur policy statements. + async_write_secret must reject it before any policy/value HTTP call is made. + """ + with patch("litellm.proxy.proxy_server.premium_user", True): + malicious_secret_name = "foo\n- !grant\n role: !!admin\n member: attacker" + + mock_sync_client = MagicMock() + mock_async_client = AsyncMock() + + with ( + patch( + "litellm.secret_managers.cyberark_secret_manager._get_httpx_client", + return_value=mock_sync_client, + ), + patch( + "litellm.secret_managers.cyberark_secret_manager.get_async_httpx_client", + return_value=mock_async_client, + ), + ): + cyberark_manager = CyberArkSecretManager() + + response = await cyberark_manager.async_write_secret( + secret_name=malicious_secret_name, + secret_value="sk-1234", + ) + + assert response["status"] == "error" + assert "Unsafe secret_name" in response["message"] + # The malicious policy YAML must never reach the wire. + mock_sync_client.client.post.assert_not_called() + mock_async_client.post.assert_not_called() + + +@pytest.mark.parametrize( + "secret_name", + [ + "foo: bar", # colon: would otherwise parse as a mapping, not a scalar + "foo # bar", # unquoted '#' starts a YAML comment mid-scalar + "plain-alias", + "team/user@example.com", + ], +) +def test_cyberark_ensure_variable_exists_escapes_yaml_metacharacters(secret_name): + """ + Regression test: secret_name is embedded in a hand-built single-line YAML + entry (`f"- !variable {secret_name}\\n"`). Characters that don't match + raise_if_unsafe_secret_name's traversal/control-character denylist (a bare + ':' or '#', no newline needed) can still change the parsed YAML structure + -- ':' turns the scalar into a mapping, '#' truncates it at a comment. + _ensure_variable_exists must escape the scalar (not just denylist-check it) + so the policy body always parses back to exactly one '!variable' scalar + node holding the untouched secret_name. + """ + with patch("litellm.proxy.proxy_server.premium_user", True): + captured = {} + + def _capture_post(url, headers=None, content=None): + captured["content"] = content + return create_mock_response(status_code=201, text="") + + mock_sync_client = MagicMock() + mock_sync_client.client.post.side_effect = _capture_post + + with patch( + "litellm.secret_managers.cyberark_secret_manager._get_httpx_client", + return_value=mock_sync_client, + ): + cyberark_manager = CyberArkSecretManager() + cyberark_manager._ensure_variable_exists(secret_name) + + policy_yaml = captured["content"] + parsed = yaml.compose(policy_yaml) + assert len(parsed.value) == 1 + node = parsed.value[0] + assert node.tag == "!variable" + assert node.value == secret_name + + @pytest.mark.asyncio async def test_cyberark_write_and_read_secret(): """ diff --git a/tests/litellm_utils_tests/test_hashicorp.py b/tests/litellm_utils_tests/test_hashicorp.py index 3bdf11ea565..3f068f289fd 100644 --- a/tests/litellm_utils_tests/test_hashicorp.py +++ b/tests/litellm_utils_tests/test_hashicorp.py @@ -409,6 +409,61 @@ def test_hashicorp_custom_mount_and_prefix(hashicorp_secret_manager): hashicorp_secret_manager.vault_namespace = original_namespace +@pytest.mark.parametrize( + "malicious_secret_name", + [ + "../../../other-app/creds", + "litellm/../../secret", + "foo\nbar", + "foo
bar", # YAML LINE SEPARATOR: same class of break as "\n" + "foo
bar", # YAML PARAGRAPH SEPARATOR + "foo\x85bar", # YAML NEL + ], +) +def test_hashicorp_get_url_rejects_path_traversal(monkeypatch, malicious_secret_name): + """ + Regression test: secret_name (derived from user-controlled key_alias) is + concatenated directly into the Vault request URL with no sanitization. get_url + must reject '..' sequences and control characters (including the Unicode line + breaks YAML treats the same as "\\n") instead of building a URL that could + escape the configured mount/path_prefix. + + Uses monkeypatch + a directly-constructed manager (not the shared + hashicorp_secret_manager fixture) so this runs in CI without real Vault + credentials configured; get_url performs no I/O. + """ + monkeypatch.setenv("HCP_VAULT_TOKEN", "test-token-for-get-url-only") + manager = HashicorpSecretManager() + + with pytest.raises(ValueError): + manager.get_url(malicious_secret_name) + + +def test_hashicorp_get_url_encodes_reserved_url_characters(monkeypatch): + """ + Regression test: secret_name used to be concatenated raw into the URL, so a + '#' or '?' would be interpreted as a URL fragment/query separator by the HTTP + client instead of a literal character in the Vault KV path. get_url must + percent-encode them while still preserving '/' (hierarchical paths) and '@' + (e.g. emails in aliases) unencoded, matching existing usage. + """ + monkeypatch.setenv("HCP_VAULT_TOKEN", "test-token-for-get-url-only") + manager = HashicorpSecretManager() + manager.vault_namespace = None + manager.vault_path_prefix = None + + url = manager.get_url("foo#bar") + assert "#" not in url + assert url.endswith("foo%23bar") + + url = manager.get_url("foo?evil=1") + assert "?" not in url.split("/data/", 1)[1] + assert url.endswith("foo%3Fevil%3D1") + + url = manager.get_url("team/user@example.com") + assert url.endswith("team/user@example.com") + + mock_old_vault_response = { "request_id": "80fafb6a-e96a-4c5b-29fa-ff505ac72201", "lease_id": "", diff --git a/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py index 7b97ae60443..f1784a1e6ba 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py @@ -8969,7 +8969,7 @@ class TestValidateKeyAliasFormat: litellm.enable_key_alias_format_validation = False def test_validation_skipped_when_flag_disabled(self): - """When enable_key_alias_format_validation is False (default), no validation occurs.""" + """When enable_key_alias_format_validation is False (default), no charset/length validation occurs.""" from litellm.proxy.management_endpoints.key_management_endpoints import ( _validate_key_alias_format, ) @@ -8980,6 +8980,48 @@ class TestValidateKeyAliasFormat: _validate_key_alias_format("!invalid!") _validate_key_alias_format("a" * 256) + @pytest.mark.parametrize( + "unsafe_alias", + [ + "../../../other-app/creds", + "litellm/../../secret", + "foo\n- !grant\n role: !!admin\n member: attacker", + "foo\rbar", + "foo\x00bar", + ], + ) + def test_validate_key_alias_format_rejects_traversal_and_control_chars_even_when_flag_disabled( + self, unsafe_alias + ): + """ + Regression test: key_alias becomes the secret name written to HashiCorp Vault + (path-concatenated into the request URL) and CyberArk Conjur (interpolated into a + YAML policy body) when store_virtual_keys is enabled. This check must reject + path traversal and control characters unconditionally, since + enable_key_alias_format_validation defaults to False and its charset alone + (when enabled) still permits ".." sequences. + """ + from litellm.proxy.management_endpoints.key_management_endpoints import ( + _validate_key_alias_format, + ) + + with pytest.raises(ProxyException) as exc: + _validate_key_alias_format(unsafe_alias) + assert str(exc.value.code) == "400" + assert "Invalid key_alias" in str(exc.value.message) + + def test_validate_key_alias_format_charset_alone_permits_dot_dot(self): + """ + Documents the pre-existing gap this PR closes: the opt-in charset regex allows + '.' and '/' individually, so it does not by itself catch '..' path traversal. + The unconditional security check (tested above) is what actually blocks it. + """ + from litellm.proxy.management_endpoints.key_management_endpoints import ( + _KEY_ALIAS_PATTERN, + ) + + assert _KEY_ALIAS_PATTERN.match("a/../../etc/secret") is not None + def test_validate_key_alias_format_valid(self): from litellm.proxy.management_endpoints.key_management_endpoints import ( _validate_key_alias_format, diff --git a/tests/test_litellm/secret_managers/test_base_secret_manager.py b/tests/test_litellm/secret_managers/test_base_secret_manager.py new file mode 100644 index 00000000000..4ca5f3a3571 --- /dev/null +++ b/tests/test_litellm/secret_managers/test_base_secret_manager.py @@ -0,0 +1,51 @@ +""" +Test raise_if_unsafe_secret_name, the shared guard applied before any secret_name +(derived from user-controlled key_alias) reaches a secret manager backend. +""" + +import os +import sys + +import pytest + +sys.path.insert(0, os.path.abspath("../..")) # Adds the parent directory to the system path + +from litellm.secret_managers.base_secret_manager import raise_if_unsafe_secret_name + + +@pytest.mark.parametrize( + "secret_name", + [ + "..", + "../../../other-app/creds", + "litellm/../../secret", + "foo\nbar", + "foo\rbar", + "foo\x00bar", + "foo\x7fbar", + "foo\x85bar", # YAML NEL + "foo
bar", # YAML LINE SEPARATOR + "foo
bar", # YAML PARAGRAPH SEPARATOR + ], +) +def test_raise_if_unsafe_secret_name_rejects_traversal_and_line_breaks(secret_name): + with pytest.raises(ValueError): + raise_if_unsafe_secret_name(secret_name) + + +@pytest.mark.parametrize( + "secret_name", + [ + "plain-alias", + "my-key-123", + "prod/my-service-key", + "team/user@example.com", + "foo: bar", + "foo # bar", + "foo?evil=1", + "foo#bar", + "a" * 500, + ], +) +def test_raise_if_unsafe_secret_name_allows_legitimate_aliases(secret_name): + raise_if_unsafe_secret_name(secret_name)