Repository navigation
feat(proxy)!: refuse to start with an unset, empty, or publicly known master key #42019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
fe48053
feat(proxy)!: refuse to start with an unset, empty, or publicly known…
ryan-crabbe-berri 0415382
fix(proxy): word the config step so it also fits a config that alread…
ryan-crabbe-berri 186ba50
fix(proxy): point users who must rotate at the rotation guide before …
ryan-crabbe-berri 0049a51
fix(proxy): import assert_never from typing_extensions for Python 3.1…
ryan-crabbe-berri 3c9c860
test(proxy): set the unsafe-proxy override at the remaining test boot…
ryan-crabbe-berri fdd614d
fix(proxy): tell users with an already exported master key to replace…
ryan-crabbe-berri 38d776b
feat(proxy): re-encrypt stored secrets at boot from LITELLM_MIGRATE_F…
ryan-crabbe-berri a6c51ba
fix(proxy): never treat plaintext that base64-decodes to nothing as a…
ryan-crabbe-berri 99f99cf
fix(proxy): stop the boot when the requested master key migration fai…
ryan-crabbe-berri ecf1751
refactor(proxy): rename the local development override to dangerously…
ryan-crabbe-berri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,227 @@ | ||
| """Decides at boot whether the proxy may start with the master key it resolved.""" | ||
|
|
||
| import atexit | ||
| import sys | ||
| from collections.abc import Callable, Mapping | ||
| from dataclasses import dataclass | ||
| from enum import Enum | ||
| from types import MappingProxyType | ||
| from typing import Final | ||
|
|
||
| from typing_extensions import assert_never | ||
|
|
||
| from litellm._logging import verbose_proxy_logger | ||
|
|
||
| UNSAFE_PROXY_OVERRIDE_SETTING: Final = "dangerously_allow_unsafe_proxy" | ||
| UNSAFE_PROXY_OVERRIDE_ENV_VAR: Final = "LITELLM_DANGEROUSLY_ALLOW_UNSAFE_PROXY" | ||
| MASTER_KEY_SETTING: Final = "master_key" | ||
| MASTER_KEY_ENV_VAR: Final = "LITELLM_MASTER_KEY" | ||
| SALT_KEY_ENV_VAR: Final = "LITELLM_SALT_KEY" | ||
| PUBLICLY_KNOWN_MASTER_KEYS: Final = frozenset({"sk-1234"}) | ||
| ROTATION_DOCS_URL: Final = "https://docs.litellm.ai/docs/proxy/master_key_rotations#proxy-refuses-to-start" | ||
| _NEW_MASTER_KEY: Final = "sk-$(openssl rand -hex 32)" | ||
| GENERATE_MASTER_KEY_COMMAND: Final = f'echo "{MASTER_KEY_ENV_VAR}={_NEW_MASTER_KEY}" | tee -a .env' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe use |
||
| PRINT_NEW_MASTER_KEY_COMMAND: Final = f'echo "{_NEW_MASTER_KEY}"' | ||
|
|
||
|
|
||
| class UnsafeMasterKeyReason(Enum): | ||
| NOT_SET = "not_set" | ||
| EMPTY = "empty" | ||
| PUBLICLY_KNOWN = "publicly_known" | ||
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class ConfigFileSource: | ||
| config_file_path: str | None | ||
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class EnvironmentSource: | ||
| pass | ||
|
|
||
|
|
||
| MasterKeySource = ConfigFileSource | EnvironmentSource | ||
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class SafeMasterKey: | ||
| pass | ||
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class UnsafeMasterKeyAllowed: | ||
| reason: UnsafeMasterKeyReason | ||
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class UnsafeMasterKeyRefused: | ||
| reason: UnsafeMasterKeyReason | ||
| source: MasterKeySource | ||
| stored_credentials_need_rotation: bool | ||
|
|
||
|
|
||
| MasterKeyBootVerdict = SafeMasterKey | UnsafeMasterKeyAllowed | UnsafeMasterKeyRefused | ||
|
|
||
|
|
||
| class UnsafeMasterKeyError(Exception): | ||
| pass | ||
|
|
||
|
|
||
| def master_key_boot_verdict( | ||
| *, | ||
| master_key: str | None, | ||
| environment_master_key: str | None, | ||
| general_settings: Mapping[str, object], | ||
| config_file_path: str | None, | ||
| override_env_is_on: bool, | ||
| salt_key_is_set: bool, | ||
| database_is_configured: bool, | ||
| ) -> MasterKeyBootVerdict: | ||
| reason: Final = _unsafe_reason(master_key) | ||
| if reason is None: | ||
| return SafeMasterKey() | ||
| if override_env_is_on or general_settings.get(UNSAFE_PROXY_OVERRIDE_SETTING) is True: | ||
| return UnsafeMasterKeyAllowed(reason=reason) | ||
| config_file_only_relays_the_environment: Final = master_key is not None and master_key == environment_master_key | ||
| return UnsafeMasterKeyRefused( | ||
| reason=reason, | ||
| source=( | ||
| ConfigFileSource(config_file_path=config_file_path) | ||
| if MASTER_KEY_SETTING in general_settings and not config_file_only_relays_the_environment | ||
| else EnvironmentSource() | ||
| ), | ||
| stored_credentials_need_rotation=( | ||
| reason is UnsafeMasterKeyReason.PUBLICLY_KNOWN and not salt_key_is_set and database_is_configured | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def enforce_master_key_boot_verdict(verdict: MasterKeyBootVerdict, announce: Callable[[str], object]) -> None: | ||
| match verdict: | ||
| case SafeMasterKey(): | ||
| return | ||
| case UnsafeMasterKeyAllowed(reason=reason): | ||
| verbose_proxy_logger.warning( | ||
| "%s is on, so the proxy is starting with %s. Never run this outside local development.", | ||
| UNSAFE_PROXY_OVERRIDE_SETTING, | ||
| _UNSAFE_STATE[reason], | ||
| ) | ||
| case UnsafeMasterKeyRefused(reason=reason): | ||
| announce(f"\n{render_refusal(verdict)}\n\n") | ||
| raise UnsafeMasterKeyError( | ||
| f"LiteLLM proxy refused to start: {_REFUSAL_HEADLINE[reason]} The fix is printed once the server exits." | ||
| ) | ||
| case _: | ||
| assert_never(verdict) | ||
|
|
||
|
|
||
| def announce_on_stderr_at_exit(message: str) -> None: | ||
| """Keeps the fix as the last thing on screen and away from the log handlers. | ||
|
|
||
| A failed lifespan prints a traceback hundreds of lines long (a frame pair per included router) that buries | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| anything written before it, and the log redactor strips the key-shaped command from anything sent to a logger. | ||
| """ | ||
| atexit.register(_flush_stdout_then_write_stderr, message) | ||
|
|
||
|
|
||
| def _flush_stdout_then_write_stderr(message: str) -> None: | ||
| sys.stdout.flush() | ||
| sys.stderr.write(message) | ||
|
|
||
|
|
||
| def render_refusal(refusal: UnsafeMasterKeyRefused) -> str: | ||
| return "\n\n".join( | ||
| ( | ||
| f"LiteLLM proxy refused to start: {_REFUSAL_HEADLINE[refusal.reason]}\n{_source_line(refusal)}", | ||
| _ROTATE_INSTEAD_OF_REPLACING if refusal.stored_credentials_need_rotation else _fix_steps(refusal.source), | ||
| _OVERRIDE_HINT, | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| _UNSAFE_STATE: Final = MappingProxyType( | ||
| { | ||
| UnsafeMasterKeyReason.NOT_SET: "no master key, which accepts every request without authentication", | ||
| UnsafeMasterKeyReason.EMPTY: "an empty master key", | ||
| UnsafeMasterKeyReason.PUBLICLY_KNOWN: "a publicly known master key", | ||
| } | ||
| ) | ||
|
|
||
| _REFUSAL_HEADLINE: Final = MappingProxyType( | ||
| { | ||
| UnsafeMasterKeyReason.NOT_SET: ( | ||
| "no master key is set, so every request would be accepted without authentication." | ||
| ), | ||
| UnsafeMasterKeyReason.EMPTY: "the master key is empty.", | ||
| UnsafeMasterKeyReason.PUBLICLY_KNOWN: "the master key is a publicly known default.", | ||
| } | ||
| ) | ||
|
|
||
| _SAVE_KEY_STEP: Final = ( | ||
| "Generate a key and save it to .env:\n" | ||
| f" {GENERATE_MASTER_KEY_COMMAND}\n" | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
|
||
| " Not using a .env file (docker run, Kubernetes, pip install)? Pass the same value as the\n" | ||
| f" {MASTER_KEY_ENV_VAR} environment variable instead." | ||
| ) | ||
|
|
||
| _ROTATE_INSTEAD_OF_REPLACING: Final = ( | ||
| f"Credentials stored in your database are encrypted with this master key because {SALT_KEY_ENV_VAR} is not\n" | ||
| "set, so replacing the key makes them undecryptable. Rotate it by following this guide, which re-encrypts them:\n" | ||
| f" {ROTATION_DOCS_URL}\n" | ||
| "Generate the new key for it with (save it only once the guide says to):\n" | ||
| f" {PRINT_NEW_MASTER_KEY_COMMAND}" | ||
| ) | ||
|
|
||
| _OVERRIDE_HINT: Final = ( | ||
| f"Local development only: set {UNSAFE_PROXY_OVERRIDE_ENV_VAR}=true, or\n" | ||
| f"general_settings.{UNSAFE_PROXY_OVERRIDE_SETTING}: true, to start anyway." | ||
| ) | ||
|
|
||
|
|
||
| def _unsafe_reason(master_key: str | None) -> UnsafeMasterKeyReason | None: | ||
| if master_key is None: | ||
| return UnsafeMasterKeyReason.NOT_SET | ||
| stripped: Final = master_key.strip() | ||
| if not stripped: | ||
| return UnsafeMasterKeyReason.EMPTY | ||
| if stripped in PUBLICLY_KNOWN_MASTER_KEYS: | ||
| return UnsafeMasterKeyReason.PUBLICLY_KNOWN | ||
| return None | ||
|
|
||
|
|
||
| def _config_label(source: ConfigFileSource) -> str: | ||
| return source.config_file_path or "your config" | ||
|
|
||
|
|
||
| def _source_line(refusal: UnsafeMasterKeyRefused) -> str: | ||
|
|
||
| match refusal.source: | ||
| case ConfigFileSource() as source: | ||
| if refusal.reason is UnsafeMasterKeyReason.NOT_SET: | ||
| return ( | ||
| f"general_settings.{MASTER_KEY_SETTING} in {_config_label(source)} is blank, or points at an " | ||
| "environment variable that is not set." | ||
| ) | ||
| return f"It comes from general_settings.{MASTER_KEY_SETTING} in {_config_label(source)}." | ||
| case EnvironmentSource(): | ||
| if refusal.reason is UnsafeMasterKeyReason.NOT_SET: | ||
| return ( | ||
| f"Neither general_settings.{MASTER_KEY_SETTING} nor the {MASTER_KEY_ENV_VAR} " | ||
| "environment variable is set." | ||
| ) | ||
| return f"It comes from the {MASTER_KEY_ENV_VAR} environment variable." | ||
| case _: | ||
| assert_never(refusal.source) | ||
|
|
||
|
|
||
| def _fix_steps(source: MasterKeySource) -> str: | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| match source: | ||
| case ConfigFileSource(): | ||
| return ( | ||
| f"1. Make sure {_config_label(source)} reads the key from the environment:\n" | ||
| f" general_settings:\n {MASTER_KEY_SETTING}: os.environ/{MASTER_KEY_ENV_VAR}\n" | ||
| f"2. {_SAVE_KEY_STEP}" | ||
| ) | ||
| case EnvironmentSource(): | ||
| return f"1. {_SAVE_KEY_STEP}" | ||
| case _: | ||
| assert_never(source) | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
LITELLM_DANGEROUSLY_PERMIT_WEAK_MASTER_KEY=true<- I feel like this is more descriptiveThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed in ecf1751 to LITELLM_DANGEROUSLY_PERMIT_WEAK_OR_UNSET_MASTER_KEY (YAML dangerously_permit_weak_or_unset_master_key), since the override also covers an unset or empty key, not only a weak one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
late to the party and bit of a nit, but we explicitly are not checking key strength, so calling it "weak" is a misnomer. I think
LITELLM_DANGEROUSLY_PERMIT_DEFAULT_OR_UNSET_MASTER_KEYwould have been better, but I imagine changing this after merge is a lot more trouble than it's worth