docker secret manager - #23519
Conversation
Reads secrets from Docker secrets mounted at /run/secrets (configurable).
Supports Docker Swarm (encrypted at rest + in transit) and Docker Compose.
- DockerSecretManager class with sync + async read, read-only by design
- Path traversal protection via realpath() + prefix check (blocks ../, absolute paths, symlinks outside secrets dir)
- Case-folding: tries exact name then lowercase fallback (bridges UPPER_CASE env style and lower_case Docker secret naming)
- Trailing whitespace stripping (handles echo footgun)
- 27 unit + e2e tests
- Docs page + sidebar + overview entry
Config:
general_settings:
key_management_system: "docker"
key_management_settings:
secrets_path: "/run/secrets" # optional, this is the default
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds a new Key changes:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/secret_managers/docker_secret_manager.py | New read-only Docker secret manager with path traversal protection, Latin-1 fallback, and correct None-to-ValueError conversion for env-var fallback. Minor: httpx is imported at runtime solely for type hints that are never exercised. |
| litellm/secret_managers/secret_manager_handler.py | Docker branch follows the same pattern as CyberArk/Hashicorp: raises ValueError for None so get_secret() falls back to env vars. Minor: the ValueError message contains redundant "Falling back to environment variable." text that duplicates the log line emitted by get_secret(). |
| tests/test_litellm/secret_managers/test_docker_secret_manager.py | Comprehensive unit and E2E test suite with no real network calls. One gap: test_load_factory_no_settings resets secret_manager_client but not _key_management_system, leaving stale global state that can affect subsequent tests. |
| litellm/types/secret_managers/main.py | Adds DOCKER to KeyManagementSystem enum and secrets_path: Optional[str] = None to KeyManagementSettings. Field is correctly Optional so it does not pollute dumps for other managers. |
| litellm/proxy/proxy_server.py | Clean integration point mirroring the existing CyberArk pattern; delegates to the factory classmethod with the current key_management_settings. |
| docs/my-website/docs/secret_managers/docker_secret_manager.md | Documentation accurately describes the implemented behaviour including env-var fallback (now correct with the ValueError-based fallback in the handler), path traversal protection, and configuration options. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[get_secret called] --> B{Should use secret manager?}
B -- No --> C[Read from process environment]
B -- Yes --> D[get_secret_from_manager with docker key_manager]
D --> E{Correct client type?}
E -- No --> F[raise ValueError wrong client]
E -- Yes --> G[sync_read_secret called]
G --> H{Path separator in name?}
H -- Yes --> I[raise ValueError path traversal]
H -- No --> J{Resolved path inside secrets dir?}
J -- No --> K[raise ValueError path traversal]
J -- Yes --> L{Exact file exists?}
L -- No --> M{Lowercase file exists?}
M -- No --> N[return None]
M -- Yes --> O[Read file UTF-8 with Latin-1 fallback]
L -- Yes --> O
O --> P[return stripped secret value to caller]
N --> Q[handler raises ValueError not found]
Q --> R[get_secret catches exception and logs]
R --> S[Fallback to process environment lookup]
I --> R
K --> R
Comments Outside Diff (1)
-
tests/test_litellm/secret_managers/test_docker_secret_manager.py, line 737-740 (link)Incomplete cleanup leaves
_key_management_systemset after testDockerSecretManager.__init__sets bothlitellm.secret_manager_clientandlitellm._key_management_system, but this test only resetssecret_manager_client. Any test that runs after this one and inspectslitellm._key_management_systemwill see a staleKeyManagementSystem.DOCKERvalue, potentially causing spurious failures.Every other test that calls the constructor (including
test_init_sets_litellm_globalsandtest_load_factory_secrets_path_none_uses_default) correctly resets both globals. This cleanup block should be consistent:
Reviews (4): Last reviewed commit: "fix(docker-secret-manager): address PR r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
- Latin-1 fallback: _read_file now tries UTF-8 then falls back to Latin-1 so secrets with é, ñ, ü, ß (ISO-8859-1/Windows-1252) are decoded correctly instead of silently returning None - Path separator guard: reject secret names containing '/' or os.sep before realpath resolution to prevent unintended subdirectory reads - Consistent exception logging: wrap Docker branch in secret_manager_handler with try/except + print_verbose, matching the pattern used by CyberArk and HashiCorp branches Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Factory None crash: `getattr(settings, "secrets_path") or _DEFAULT` guard so operators who omit secrets_path from their config (the typical case) no longer hit TypeError from os.path.realpath(None) on startup - Misleading warning: reword "fall back to environment variables" to "lookups will return None" — env-var fallback is caller-controlled, not a DockerSecretManager responsibility - Root-safe test: add skipif(os.getuid() == 0) to test_unreadable_file_returns_none so chmod 000 tests don't silently pass/fail in Docker CI runners that run as root - New regression test: test_load_factory_secrets_path_none_uses_default explicitly covers the None factory path that was crashing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| - model_name: gpt-4o | ||
| litellm_params: | ||
| model: openai/gpt-4o | ||
| api_key: os.environ/OPENAI_API_KEY # reads $OPENAI_API_KEY from env | ||
| ``` | ||
|
|
||
| Without the `os.environ/` prefix, a value that isn't found as a Docker secret file is returned as-is (the literal string), so use literal values only when you intend to pass them directly. | ||
|
|
||
| ## Configuration options | ||
|
|
||
| | Setting | Required | Default | Description | | ||
| |---------|----------|---------|-------------| | ||
| | `key_management_system` | Yes | — | Must be `"docker"` | | ||
| | `key_management_settings.secrets_path` | No | `/run/secrets` | Directory where Docker secrets are mounted | |
There was a problem hiding this comment.
os.environ/ prefix does not fall back to the environment when a Docker secret is missing
This section tells operators that adding the os.environ/ prefix causes the system to read the corresponding environment variable when the Docker secret is not present. This is incorrect.
In litellm/secret_managers/main.py, the environment fallback inside get_secret() only triggers when get_secret_from_manager() raises an exception. When DockerSecretManager.sync_read_secret() returns None for a missing secret, get_secret propagates None directly — it does not read from the process environment as a fallback.
The test test_e2e_get_secret_returns_none_when_not_in_docker documents this explicitly: get_secret() only falls back to environment variables on exceptions, not on None returns.
An operator who follows this section and expects silent fallback behaviour will instead receive None, causing silent authentication failures.
This section should be removed or corrected to clarify that the os.environ/ prefix is the standard LiteLLM mechanism for naming a secret (no special fallback is added when the Docker manager is active).
| secret = client.sync_read_secret(secret_name=secret_name) | ||
| # None is valid — secret not found, caller falls back to env vars |
There was a problem hiding this comment.
Misleading comment about fallback behaviour
The comment states that the caller falls back to environment variables when None is returned, but this is not what happens. In litellm/secret_managers/main.py, get_secret() only performs an environment lookup when get_secret_from_manager() raises an exception — a None return propagates directly to the caller with no substitution. The test test_e2e_get_secret_returns_none_when_not_in_docker explicitly verifies this behaviour.
The comment should be corrected to accurately describe the None semantics, e.g. # None indicates the secret file was not found; the caller receives None.
- Raise ValueError in handler when Docker secret is missing so get_secret() falls back to os.environ (consistent with Hashicorp/CyberArk branches) - Remove misleading "caller falls back to env vars" comment on None return - Fix sync_read_secret docstring that incorrectly described the fallback mechanism - Fix docker_secret_manager.md: the os.environ/ section previously stated that a missing Docker secret triggers env-var fallback, which was wrong (fallback only triggered on exceptions). Now correctly documents that the handler raises for missing secrets, causing get_secret() to fall back. - Update test_e2e_get_secret_returns_none_when_not_in_docker → now asserts env-var fallback actually works when Docker secret is absent - Remove duplicate litellm._key_management_system = None in teardown Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| ) | ||
| return None | ||
|
|
||
| async def async_write_secret( |
There was a problem hiding this comment.
httpx imported only for type annotations but pulled in at import time
httpx is imported at module level solely to provide the httpx.Timeout type hint in async_read_secret and sync_read_secret. However, DockerSecretManager never performs any network I/O — timeout is accepted purely to satisfy the BaseSecretManager interface and is never used. The unconditional top-level import means every environment that imports this module must have httpx installed, even though the module never uses it at runtime.
Other pure-filesystem secret managers (e.g. the local-env helper) avoid this by using TYPE_CHECKING guards. Consider:
from __future__ import annotations
from typing import TYPE_CHECKING
if TYPE_CHECKING:
import httpxThis keeps the type hints intact for static analysis while removing the hard runtime dependency.
| raise ValueError( | ||
| f"No secret found in Docker secrets for {secret_name!r}. " | ||
| f"Falling back to environment variable." | ||
| ) |
There was a problem hiding this comment.
When this ValueError propagates to get_secret(), it is already logged with a "defaulting to env" message. The phrase "Falling back to environment variable." embedded in the exception body therefore produces a confusing duplicate — operators reading the log see two different phrasings of the same fact, and print_verbose at line 168 prepends yet another "An error occurred - " prefix.
The Hashicorp Vault and CyberArk branches immediately above use only "No secret found in … for {secret_name}" without any fallback commentary. Aligning the Docker branch with that style removes the redundancy and keeps the handler consistent.
1 participant
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes