fix(proxy): make /config/update env-var encryption idempotent
A single decrypt-then-encrypt chokepoint (_encrypt_env_variables_for_db) now backs both update_config and save_config. Re-submitting a value the Admin UI read back from /get/config/callbacks as ciphertext no longer stacks a second encryption layer, which previously decrypted to garbage and silently broke the callback. The chokepoint decrypts with the pure _decrypt_db_variables (no os.environ mutation on the write path) and encrypts exactly once; update_config merges only the sent keys so untouched env vars keep their stored ciphertext byte-for-byte.
This commit is contained in:
parent
2f041a5224
commit
0d8c9137fb
@ -3415,20 +3415,18 @@ class ProxyConfig:
|
||||
# Make a copy to avoid mutating the original config
|
||||
config_to_save = new_config.copy()
|
||||
|
||||
# SECURITY: Always encrypt environment_variables before DB write
|
||||
# SECURITY: Always encrypt environment_variables before DB write.
|
||||
# _encrypt_env_variables_for_db is idempotent — a caller that
|
||||
# already encrypted the values (or re-submitted ciphertext read
|
||||
# back from the DB) will not get a stacked second layer.
|
||||
if (
|
||||
"environment_variables" in config_to_save
|
||||
and config_to_save["environment_variables"]
|
||||
):
|
||||
# decrypt the environment_variables - in case a caller function has already encrypted the environment_variables
|
||||
decrypted_env_vars = self._decrypt_and_set_db_env_variables(
|
||||
environment_variables=config_to_save["environment_variables"],
|
||||
return_original_value=True,
|
||||
)
|
||||
|
||||
# encrypt the environment_variables,
|
||||
config_to_save["environment_variables"] = self._encrypt_env_variables(
|
||||
environment_variables=decrypted_env_vars
|
||||
config_to_save["environment_variables"] = (
|
||||
self._encrypt_env_variables_for_db(
|
||||
environment_variables=config_to_save["environment_variables"]
|
||||
)
|
||||
)
|
||||
|
||||
config_to_save.pop("model_list", None)
|
||||
@ -5076,6 +5074,29 @@ class ProxyConfig:
|
||||
decrypted_variables[k] = decrypted_value
|
||||
return decrypted_variables
|
||||
|
||||
def _encrypt_env_variables_for_db(
|
||||
self, environment_variables: dict, new_encryption_key: Optional[str] = None
|
||||
) -> dict:
|
||||
"""
|
||||
Idempotently encrypt environment variables for a DB write.
|
||||
|
||||
Config writers may pass either plaintext (first write) or values that
|
||||
are already ciphertext — e.g. the Admin UI reads config back via
|
||||
/get/config/callbacks (which returns the stored, still-encrypted
|
||||
value) and re-POSTs it on the next save. Decrypt first so an
|
||||
already-encrypted value is not stacked with a second encryption
|
||||
layer, then encrypt exactly once.
|
||||
|
||||
Decryption here deliberately uses _decrypt_db_variables (not
|
||||
_decrypt_and_set_db_env_variables): this is a write path, and
|
||||
loading values into os.environ is the read path's responsibility.
|
||||
"""
|
||||
decrypted_env_vars = self._decrypt_db_variables(environment_variables)
|
||||
return self._encrypt_env_variables(
|
||||
environment_variables=decrypted_env_vars,
|
||||
new_encryption_key=new_encryption_key,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _parse_router_settings_value(value: Any) -> Optional[dict]:
|
||||
"""
|
||||
@ -13788,11 +13809,18 @@ async def update_config( # noqa: PLR0915
|
||||
existing[k] = v
|
||||
await _upsert_section("general_settings", existing)
|
||||
|
||||
# environment_variables: encrypt request values, then merge into existing.
|
||||
# environment_variables: idempotently encrypt the request values
|
||||
# (plaintext on first write, OR ciphertext the UI read back via
|
||||
# /get/config/callbacks and re-submitted on save), then merge into
|
||||
# existing. Only the sent keys are re-written; untouched keys keep
|
||||
# their stored ciphertext byte-for-byte.
|
||||
if config_info.environment_variables is not None:
|
||||
existing = await _read_section("environment_variables")
|
||||
for k, v in config_info.environment_variables.items():
|
||||
existing[k] = encrypt_value_helper(value=v)
|
||||
existing.update(
|
||||
proxy_config._encrypt_env_variables_for_db(
|
||||
environment_variables=config_info.environment_variables
|
||||
)
|
||||
)
|
||||
await _upsert_section("environment_variables", existing)
|
||||
|
||||
# litellm_settings: merge existing + request, request wins (matching
|
||||
|
||||
@ -3850,6 +3850,62 @@ def test_update_config_fields_uppercases_env_vars(monkeypatch):
|
||||
assert os.environ.get("DD_SITE") == "us5.datadoghq.com"
|
||||
|
||||
|
||||
def test_encrypt_env_variables_for_db_is_idempotent(monkeypatch):
|
||||
"""
|
||||
Regression: /config/update and save_config must not stack a second
|
||||
encryption layer when a caller re-submits a value that is already
|
||||
ciphertext (the Admin UI reads config back from /get/config/callbacks —
|
||||
which returns the stored, still-encrypted value — and re-POSTs it on the
|
||||
next save). _encrypt_env_variables_for_db must yield a value that decrypts
|
||||
to the original plaintext in exactly ONE layer, no matter how many times
|
||||
its own output is fed back in. It must also not mutate os.environ (write
|
||||
path — loading into the process env is the read path's job).
|
||||
"""
|
||||
from litellm.proxy.common_utils.encrypt_decrypt_utils import (
|
||||
decrypt_value_helper,
|
||||
)
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
monkeypatch.setenv("LITELLM_SALT_KEY", "sk-test-salt-key")
|
||||
monkeypatch.delenv("LANGFUSE_PUBLIC_KEY", raising=False)
|
||||
|
||||
proxy_config = ProxyConfig()
|
||||
plaintext = "pk-langfuse-secret-value"
|
||||
|
||||
# First write: plaintext in -> single-encrypted out.
|
||||
enc1 = proxy_config._encrypt_env_variables_for_db(
|
||||
{"LANGFUSE_PUBLIC_KEY": plaintext}
|
||||
)
|
||||
assert enc1["LANGFUSE_PUBLIC_KEY"] != plaintext
|
||||
assert (
|
||||
decrypt_value_helper(
|
||||
value=enc1["LANGFUSE_PUBLIC_KEY"], key="LANGFUSE_PUBLIC_KEY"
|
||||
)
|
||||
== plaintext
|
||||
)
|
||||
|
||||
# UI round-trip: feed the ciphertext back in. Must NOT double-encrypt.
|
||||
enc2 = proxy_config._encrypt_env_variables_for_db(enc1)
|
||||
assert (
|
||||
decrypt_value_helper(
|
||||
value=enc2["LANGFUSE_PUBLIC_KEY"], key="LANGFUSE_PUBLIC_KEY"
|
||||
)
|
||||
== plaintext
|
||||
)
|
||||
|
||||
# And again — still exactly one layer, never stacked.
|
||||
enc3 = proxy_config._encrypt_env_variables_for_db(enc2)
|
||||
assert (
|
||||
decrypt_value_helper(
|
||||
value=enc3["LANGFUSE_PUBLIC_KEY"], key="LANGFUSE_PUBLIC_KEY"
|
||||
)
|
||||
== plaintext
|
||||
)
|
||||
|
||||
# Write path must not leak the value into the process environment.
|
||||
assert os.environ.get("LANGFUSE_PUBLIC_KEY") is None
|
||||
|
||||
|
||||
def test_get_prompt_spec_for_db_prompt_with_versions():
|
||||
"""
|
||||
Test that _get_prompt_spec_for_db_prompt correctly converts database prompts
|
||||
|
||||
Loading…
Reference in New Issue
Block a user