Skip to content

fix(proxy_server): remove redundant decryption of already-decrypted env variables#24586

Open
danielaskdd wants to merge 19 commits intoBerriAI:mainfrom
danielaskdd:fix/redundant-decrption
Open

fix(proxy_server): remove redundant decryption of already-decrypted env variables#24586
danielaskdd wants to merge 19 commits intoBerriAI:mainfrom
danielaskdd:fix/redundant-decrption

Conversation

@danielaskdd
Copy link
Copy Markdown
Contributor

@danielaskdd danielaskdd commented Mar 25, 2026

🐛 Observed symptom

Opening the Email Settings / callbacks config page triggers:

  • LiteLLM Proxy:ERROR: encrypt_decrypt_utils.py:74 - Error decrypting value for key: SMTP_HOST, Did your master_key/salt key change recently?
  • Error: The nonce must be exactly 24 bytes long

After the error, all SMTP fields in the admin UI appear empty (no backend error is surfaced — decrypt_value_helper with return_original_value=False silently returns None, and the UI renders an empty password input).

Sending a test email from the UI also fails with "No receiver email provided for SMTP email. None" because TEST_EMAIL_ADDRESS is stored encrypted in the DB and was never decrypted into os.environ before the health-check endpoint ran.

Root cause

1. Double-decryption in /get/config/callbacks

/get/config/callbacks was calling decrypt_value_helper on environment_variables values without return_original_value=True.

The behaviour differed by storage mode:

Mode Value in environment_variables when endpoint reads it Old code result Fixed code result
DB (store_model_in_db=True) Already-decrypted plaintext (by _update_config_from_db_update_config_fields) nacl decrypt fails → Noneempty UI plaintext returned as-is ✓
YAML Still-encrypted base64 ciphertext (stored as-is in config file) Decrypted correctly ✓ Decrypted correctly ✓

2. TEST_EMAIL_ADDRESS not resolved for the email health-check

/health/services?service=email read TEST_EMAIL_ADDRESS exclusively from os.environ. In DB mode the env-var is only populated as a side-effect of calling get_config(); if that path was never hit in the current server process the value was None"no recipient" error.

3. UI sends test email with stale / unsaved form values

The "Test Email Alerts" button called serviceHealthCheck directly without first persisting the current form values, so a freshly-typed TEST_EMAIL_ADDRESS was never sent to the backend.

Changes

Backend — litellm/proxy/proxy_server.py

  • Replace decrypt_value_helper(return_original_value=False) calls (and bare pass-throughs) for SMTP/email and Slack env-vars with decrypt_value_helper(..., return_original_value=True):
    • DB mode: values are already plaintext → nacl decrypt fails → return_original_value=True returns the original plaintext unchanged.
    • YAML mode: values are still encrypted → nacl decrypt succeeds → returns plaintext.

Backend — litellm/proxy/health_endpoints/_health_endpoints.py

  • In the service == "email" branch, call proxy_config.get_config() when store_model_in_db is enabled so TEST_EMAIL_ADDRESS is freshly decrypted from the DB before building the WebhookEvent.
  • Fall back to os.getenv("TEST_EMAIL_ADDRESS") for YAML / env-var deployments (store_model_in_db=False).

Frontend — ui/litellm-dashboard/src/components/email_settings.tsx

  • Add a silent option to handleSaveEmailSettings that suppresses notifications when called programmatically.
  • "Test Email Alerts" now silently persists the current form values before triggering the health-check, so the backend can read TEST_EMAIL_ADDRESS from the DB (DB mode). Errors during the silent save are swallowed so the test can still proceed using env-var / YAML config values as fallback (YAML mode / STORE_MODEL_IN_DB=False).

Tests — tests/proxy_unit_tests/test_proxy_server.py

  • Update the regression test to match the new call contract: mock decrypt_value_helper with a stub that honours return_original_value=True (identity function), assert it is called once per env-var, and verify final values are the expected plaintext.

… env variables

- remove duplicate decrypt_value_helper calls for slack and email env vars in get_config
- values from environment_variables are already decrypted, no need to decrypt again
- add test to verify decrypt_value_helper is not called for slack/email config values
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Ready Ready Preview, Comment Apr 10, 2026 6:10am

Request Review

@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq bot commented Mar 25, 2026

Merging this PR will not alter performance

✅ 16 untouched benchmarks


Comparing danielaskdd:fix/redundant-decrption (3cf35a6) with main (9e6d2d2)

Open in CodSpeed

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR fixes a triple issue with email alerting configuration in the LiteLLM proxy: (1) double-decryption of already-plaintext env variables in /get/config/callbacks when running in DB mode, (2) missing TEST_EMAIL_ADDRESS resolution in the email health check endpoint, and (3) the UI not persisting form values before triggering a health check.

The fix introduces return_original_value=True on decrypt_value_helper calls in the config callbacks endpoint, a new direct DB lookup for TEST_EMAIL_ADDRESS in _resolve_test_email_address, a new signing_key_utils.py helper to break a circular import cycle, and a silent pre-save step in the UI before the health check call. All three root causes are addressed cleanly and previous reviewer concerns (missing get_secret for external KMS, AssertionError side-effect) have been resolved.

Confidence Score: 5/5

This PR is safe to merge — all three root causes are correctly fixed, previous reviewer concerns have been addressed, and the changes are well-tested with no regressions introduced.

No P0 or P1 findings. The core fix (return_original_value=True), the circular-import refactor via signing_key_utils, the direct DB resolution of TEST_EMAIL_ADDRESS, and the UI silent-save are all logically sound. The test changes correctly model the new behavior rather than masking a regression. All remaining observations are P2 or below.

No files require special attention.

Important Files Changed

Filename Overview
litellm/proxy/common_utils/signing_key_utils.py New helper module that resolves the signing key from LITELLM_SALT_KEY → proxy_server.master_key → LITELLM_MASTER_KEY, breaking the circular import from encrypt_decrypt_utils back into proxy_server.
litellm/proxy/common_utils/encrypt_decrypt_utils.py Simplified _get_salt_key() to delegate to get_proxy_signing_key(); no behavior change for callers, circular import removed.
litellm/proxy/health_endpoints/_health_endpoints.py Adds _resolve_test_email_address() to fetch TEST_EMAIL_ADDRESS directly from DB when store_model_in_db is active; adds helper utilities and restores get_secret import for external KMS support.
litellm/proxy/proxy_server.py Both Slack and email env-var decryption loops in /get/config/callbacks now use return_original_value=True, fixing silent empty-field rendering in DB mode.
tests/proxy_unit_tests/test_proxy_server.py Updated regression test now uses an identity-function stub honoring return_original_value=True and asserts decrypt is called once per env-var, accurately reflecting the fixed behavior.
tests/test_litellm/proxy/common_utils/test_signing_key_utils.py New unit tests covering all three key resolution paths (LITELLM_SALT_KEY priority, proxy module master_key, LITELLM_MASTER_KEY env fallback) without real network calls.
tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py New unit tests cover _get_env_secret, get_secret_bool, _parse_config_row_param_value, and _build_model_param_to_info_mapping; mock-only tests with no real network calls.
ui/litellm-dashboard/src/components/email_settings.tsx Adds silent option to handleSaveEmailSettings and silently persists form values before triggering the email health check, so TEST_EMAIL_ADDRESS reaches the backend in DB mode.

Sequence Diagram

sequenceDiagram
    participant UI as Email Settings UI
    participant BE as /health/services?service=email
    participant PS as proxy_server /get/config/callbacks
    participant DB as litellm_config (DB)
    participant Decrypt as decrypt_value_helper

    Note over UI,Decrypt: Fix 1 - /get/config/callbacks no longer double-decrypts
    UI->>PS: GET /get/config/callbacks
    PS->>DB: read environment_variables row
    DB-->>PS: already-plaintext values (DB mode)
    PS->>Decrypt: decrypt(value, return_original_value=True)
    Decrypt-->>PS: plaintext (nacl fails gracefully, returns original)
    PS-->>UI: SMTP_HOST, SMTP_PASSWORD etc. (correct plaintext)

    Note over UI,Decrypt: Fix 2 - email health check resolves TEST_EMAIL_ADDRESS from DB
    UI->>UI: handleSaveEmailSettings(silent=true) persist form
    UI->>BE: GET /health/services?service=email
    BE->>DB: find_unique(param_name=general_settings)
    DB-->>BE: store_model_in_db flag
    BE->>DB: find_unique(param_name=environment_variables)
    DB-->>BE: encrypted/plaintext TEST_EMAIL_ADDRESS
    BE->>Decrypt: decrypt(value, return_original_value=True)
    Decrypt-->>BE: resolved email address
    BE-->>UI: success (email sent)
Loading

Reviews (17): Last reviewed commit: "Merge branch 'main' into fix/redundant-d..." | Re-trigger Greptile

…k config test

- remove side_effect assertion from decrypt_value_helper mock to allow
  the function to be called during /get/config/callbacks request
- call decrypt_value_helper with return_original_value=True for slack and email env vars
- supports DB mode (already plaintext) and YAML mode (still encrypted) gracefully
- update test to use fake_decrypt stub and assert correct decrypt call count
…in email health check

- fetch fresh decrypted config from DB when store_model_in_db is enabled
  so TEST_EMAIL_ADDRESS is correctly resolved in DB mode
- fall back to os.getenv for YAML / env-var deployments (no-op path)
- add silent mode to handleSaveEmailSettings to suppress notifications
  when called programmatically from the test email flow
- silently persist form values before triggering email health check so
  the backend can read TEST_EMAIL_ADDRESS from DB (DB mode)
- swallow errors in silent mode to allow test flow to proceed using
  env-var / YAML config values as fallback
@gitguardian
Copy link
Copy Markdown

gitguardian bot commented Apr 1, 2026

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
29203053 Triggered Generic Password 631942d .circleci/config.yml View secret
29375658 Triggered JSON Web Token 631942d tests/test_litellm/proxy/auth/test_handle_jwt.py View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

…ealth checks

- add _resolve_test_email_address() to fetch TEST_EMAIL_ADDRESS from DB when store_model_in_db is enabled
- add helper functions _parse_config_row_param_value() and _is_truthy_config_flag() for config parsing
- support encrypted values with fallback to environment variable
- refactor email health check to use new resolver instead of direct config access

✅ test(health): add comprehensive tests for db-backed email resolution

- test db email takes precedence when store_model_in_db is enabled
- test fallback to env var when db is disabled or config missing
- test json string parsing for environment_variables row
- add lazy import wrappers for get_secret, get_secret_bool, and decrypt_value_helper
- prevent circular dependencies during proxy initialization by deferring imports
- keep existing function signatures and behavior unchanged
…h endpoints

- inline secret retrieval functions to remove lazy imports from secret_managers.main
- implement local decryption logic with _get_proxy_signing_key and _decrypt_value helpers
- add _str_to_bool utility for boolean environment variable parsing
- remove dependency on encrypt_decrypt_utils module to break circular imports
…e secret resolution

- remove inline decrypt_value_helper and _decrypt_value functions from health endpoints
- import decrypt_value_helper from common_utils.encrypt_decrypt_utils module
- rename get_secret to _get_env_secret to avoid naming conflict with secret_managers.main.get_secret
- update _resolve_os_environ_variables to use centralized get_secret from secret_managers.main
- add comprehensive tests for _resolve_os_environ_variables with nested dicts and lists
…module

- move _get_salt_key logic to new signing_key_utils module
- use sys.modules lookup to avoid circular import with proxy_server
- add fallback to LITELLM_MASTER_KEY environment variable

✅ test(signing_key_utils): add unit tests for signing key resolution

- test priority: LITELLM_SALT_KEY > proxy_server.master_key > LITELLM_MASTER_KEY
- mock sys.modules to simulate loaded proxy_server module
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 88.09524% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...itellm/proxy/health_endpoints/_health_endpoints.py 85.50% 10 Missing ⚠️

📢 Thoughts on this report? Let us know!

…ctions

- add tests for _str_to_bool, _get_env_secret, get_secret_bool utility functions
- add tests for _parse_config_row_param_value json parsing logic
- add tests for _build_model_param_to_info_mapping model aggregation
- add tests for _aggregate_health_check_results health status aggregation
- add tests for _save_background_health_checks_to_db database operations
@danielaskdd
Copy link
Copy Markdown
Contributor Author

Codecov Report

❌ Patch coverage is 58.33333% with 35 lines in your changes missing coverage. Please review.

Additional test cases have been implemented to improve overall coverage. Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants