From d664e21c5727be46c29cf6a14e81a3c246d8cd85 Mon Sep 17 00:00:00 2001 From: Yujong Lee Date: Thu, 5 Nov 2026 12:47:57 +0900 Subject: [PATCH] fix(redis-resolve): refuse ssl_include_verify_flags and ssl_exclude_verify_flags Both ask for X.509 checks such as CRL revocation that rustls cannot perform, and redis-py 5.3.1's async client already refuses them with a TypeError, so a set value now fails with Error::Unenforceable like the OCSP settings instead of connecting without the check. An empty list asks for nothing. A Reject row now says which values ask for its check: true for the OCSP flags, any present value for the verify flag lists. tests/catalog.rs fails if the locked redis-py's async, cluster or connection signature accepts a Reject row, so a redis-py bump forces the decision again --- litellm-rust/crates/redis-resolve/AGENTS.md | 6 +- litellm-rust/crates/redis-resolve/src/keys.rs | 34 +++++++---- litellm-rust/crates/redis-resolve/src/lib.rs | 2 +- .../crates/redis-resolve/src/options.rs | 24 +++++--- .../crates/redis-resolve/tests/catalog.rs | 56 ++++++++++++++++--- .../crates/redis-resolve/tests/interpret.rs | 5 ++ tests/unit/test_redis.py | 12 ++++ 7 files changed, 107 insertions(+), 32 deletions(-) diff --git a/litellm-rust/crates/redis-resolve/AGENTS.md b/litellm-rust/crates/redis-resolve/AGENTS.md index 91d0d22d1ad..5574bf2829c 100644 --- a/litellm-rust/crates/redis-resolve/AGENTS.md +++ b/litellm-rust/crates/redis-resolve/AGENTS.md @@ -12,9 +12,11 @@ - Precedence matches Python. The target is cluster, then URL, then Sentinel (`sentinel_nodes` plus `service_name`), then host. Auth is a `credential_provider` handle, then GCP, then Azure, then AWS, and the first one wins with a warning - Numbers are read with `litellm_python_compat::number`, so whitespace and digit underscores parse like Python's `int()` and `float()`. A value they cannot parse (`port: abc`, `socket_timeout: slow`) is dropped with a warning and the default is used, like `_coerce_redis_kwargs_types`. A number that parses but is out of range (negative seconds, a port above 65535) fails with `Error::Invalid` - Flags (`ssl`, `ssl_check_hostname`, `aws_iam_auth`, `aws_iam_serverless`) are read one way on every surface. `true`, `1` and `yes` are true and `false`, `0`, `no`, `off`, `f` and `n` are false, the text redis-py's `_str_to_bool` and the proxy's pydantic models agree on. Any other text is `Error::Invalid`, so a TLS setting is never guessed -- The resolver fails on a setting it cannot honor only when that setting is a security check. A `Warn` row is dropped and returned as an `Ignored` naming its source (`ssl_ciphers` or `REDIS_SSL_CIPHERS`) and the consequence on its row, because a Python deployment that worked must keep working when its stores move to Rust. Only a value that is malformed (`Error::Invalid`) or a target that is missing fails. Promoting a `Warn` row to `Read` is then a pure improvement with no behavior cliff +- The resolver fails on a setting it cannot honor only when that setting is a security check. A `Warn` row is dropped and returned as an `Ignored` naming its source (`ssl_ciphers` or `REDIS_SSL_CIPHERS`) and the consequence on its row, because a Python deployment that worked must keep working when its stores move to Rust. Only a value that is malformed (`Error::Invalid`), a target that is missing or a `Reject` row that asks for its check fails. Promoting a `Warn` row to `Read` is then a pure improvement with no behavior cliff, while moving one to `Reject` can fail a config that connects today - A `Warn` row with a `met` check is dropped silently when its value asks for nothing more than Rust does, so `ssl_min_version` of TLS 1.2 or lower never warns - - A `Reject` row fails closed. `ssl_validate_ocsp` and `ssl_validate_ocsp_stapled` set to true, from the kwargs, `REDIS_*` or a url query, are `Error::Unenforceable`, because rustls has no client-side OCSP and connecting without the check would silently drop it. redis-py 5.3.1's async client refuses both with a `TypeError`, so this matches the proxy's path. Set to false they ask for nothing and are dropped without a warning + - A `Reject` row is a security check rustls cannot perform that Python's locked async client already refuses with a `TypeError`, so failing closed breaks no proxy that runs today. `tests/catalog.rs` fails if the locked redis-py's async, cluster or connection signature accepts a `Reject` row, so a redis-py bump forces the decision again. A row's `Asked` says which values ask for the check, from the kwargs, `REDIS_*` or a url query, and those are `Error::Unenforceable` + - `ssl_validate_ocsp` and `ssl_validate_ocsp_stapled` ask when true, because rustls has no client-side OCSP. False asks for nothing and is dropped without a warning + - `ssl_include_verify_flags` and `ssl_exclude_verify_flags` ask when set, because rustls takes no X.509 verify flags such as CRL checking. An empty list asks for nothing - `ssl_ca_data` and `ssl_ca_path` are read into `TlsOptions` and trusted next to `ssl_ca_certs`. `ssl_password` is read into `TlsOptions::key_password`, which `redis-client` uses to decrypt a PKCS#8 key like the sync client's `load_cert_chain(password=...)`. redis-py 5.3.1's async client refuses it, so this is a superset of the path the proxy uses, like `ssl_ca_path` - `unix_socket_path` resolves to `Target::Unix`, which like redis-py's `UnixDomainSocketConnection` replaces host, port and TLS. Python still requires `host` next to it, so Rust does too - A url's `?db=`, `?username=` and `?password=` are read like `parse_url` reads them, so `?db=` wins over the path and the userinfo wins over `?username=` and `?password=`. Any other query parameter, including one with no row, is ignored naming `url ?`, because redis-py turns each one into a client kwarg. Which ones a url carries is the `FromUrl` column diff --git a/litellm-rust/crates/redis-resolve/src/keys.rs b/litellm-rust/crates/redis-resolve/src/keys.rs index bb711f08cae..d8240d0210e 100644 --- a/litellm-rust/crates/redis-resolve/src/keys.rs +++ b/litellm-rust/crates/redis-resolve/src/keys.rs @@ -12,8 +12,18 @@ pub enum Handling { consequence: &'static str, met: Option bool>, }, - /// A security check the Rust client cannot perform, so setting it to true fails instead of connecting without it. - Reject { check: &'static str }, + /// A security check the Rust client cannot perform and Python's async client already refuses, so asking for it + /// fails instead of connecting without it. + Reject { check: &'static str, asked: Asked }, +} + +/// Which values of a `Reject` row ask for its check. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Asked { + /// A flag read as true. False asks for nothing. + WhenTrue, + /// Any present value, like a list of flags. An empty one asks for nothing. + WhenSet, } /// What the value is, which decides where an unread setting lands in a `Resolution`. @@ -127,8 +137,8 @@ const fn warn_unless( ) } -const fn reject(kwarg: &'static str, check: &'static str) -> Setting { - Setting::new(kwarg, Handling::Reject { check }) +const fn reject(kwarg: &'static str, check: &'static str, asked: Asked) -> Setting { + Setting::new(kwarg, Handling::Reject { check, asked }) } const NO_EQUIVALENT: &str = "the Rust client has no equivalent"; @@ -139,7 +149,7 @@ const MAINTENANCE: &str = "the Rust client does not follow server maintenance no const RUSTLS: &str = "rustls negotiates TLS 1.2+ with its own ECDHE AEAD suites"; const REVOCATION: &str = "OCSP revocation checks"; const OCSP: &str = "it only configures ssl_validate_ocsp, which the Rust client refuses when set"; -const VERIFY_FLAGS: &str = "rustls verifies certificates with its own fixed rules, so no extra X.509 flag such as CRL checking applies"; +const VERIFY_FLAGS: &str = "X.509 verify flags such as CRL checking"; /// Every Redis kwarg redis-py or `litellm/_redis.py` accepts, one row each. `redis-py-kwargs.json` records which /// redis-py versions and `REDIS_*` names accept them, and `tests/catalog.rs` keeps the two in step. @@ -243,15 +253,15 @@ pub const SETTINGS: &[Setting] = &[ read("ssl_certfile").env().in_url(FromUrl::Rediss), read("ssl_check_hostname").env().in_url(FromUrl::Rediss), warn("ssl_ciphers", RUSTLS).env(), - warn("ssl_exclude_verify_flags", VERIFY_FLAGS), - warn("ssl_include_verify_flags", VERIFY_FLAGS), + reject("ssl_exclude_verify_flags", VERIFY_FLAGS, Asked::WhenSet), + reject("ssl_include_verify_flags", VERIFY_FLAGS, Asked::WhenSet), read("ssl_keyfile").env().in_url(FromUrl::Rediss), warn_unless("ssl_min_version", RUSTLS, is_met_by_rustls).env(), warn("ssl_ocsp_context", OCSP).env().object(), warn("ssl_ocsp_expected_cert", OCSP).env(), read("ssl_password").env().in_url(FromUrl::Rediss), - reject("ssl_validate_ocsp", REVOCATION).env(), - reject("ssl_validate_ocsp_stapled", REVOCATION).env(), + reject("ssl_validate_ocsp", REVOCATION, Asked::WhenTrue).env(), + reject("ssl_validate_ocsp_stapled", REVOCATION, Asked::WhenTrue).env(), read("startup_nodes").or_secrets(&["REDIS_CLUSTER_NODES"]), read("unix_socket_path").env(), read("url").env(), @@ -278,10 +288,10 @@ pub fn setting(kwarg: &str) -> Option<&'static Setting> { SETTINGS.iter().find(|setting| setting.kwarg == kwarg) } -/// The check a `Reject` row asks for when it is set to true. -pub(crate) fn refused_check(kwarg: &str) -> Option<&'static str> { +/// The check a `Reject` row stands for and which values ask for it. +pub(crate) fn refusal(kwarg: &str) -> Option<(&'static str, Asked)> { match setting(kwarg)?.handling { - Handling::Reject { check } => Some(check), + Handling::Reject { check, asked } => Some((check, asked)), Handling::Read | Handling::Warn { .. } => None, } } diff --git a/litellm-rust/crates/redis-resolve/src/lib.rs b/litellm-rust/crates/redis-resolve/src/lib.rs index 83db0a805ce..e58d5584cb1 100644 --- a/litellm-rust/crates/redis-resolve/src/lib.rs +++ b/litellm-rust/crates/redis-resolve/src/lib.rs @@ -11,7 +11,7 @@ mod options; mod tls; pub use error::{Error, Expected}; -pub use keys::{FromUrl, Handling, SETTINGS, Setting, Shape, setting}; +pub use keys::{Asked, FromUrl, Handling, SETTINGS, Setting, Shape, setting}; pub use options::is_present; use options::Options; diff --git a/litellm-rust/crates/redis-resolve/src/options.rs b/litellm-rust/crates/redis-resolve/src/options.rs index 41fee1d8ded..9c8b8c7e79b 100644 --- a/litellm-rust/crates/redis-resolve/src/options.rs +++ b/litellm-rust/crates/redis-resolve/src/options.rs @@ -7,7 +7,7 @@ use serde_json::Value; use crate::{ Entry, Error, Expected, Kwargs, - keys::{self, Shape}, + keys::{self, Asked, Shape}, }; /// Every setting the resolver reads. A setting is `None` when it is unset, null or empty. @@ -168,19 +168,25 @@ impl Options { } } -/// A `Reject` row that is set to true asks for a check Rust cannot perform. One set to false asks for nothing. +/// A `Reject` row whose value asks for its check fails, because Rust cannot perform it. pub(super) fn refuse_unenforceable<'a>( entries: impl IntoIterator, ) -> Result<(), Error> { entries .into_iter() - .filter_map(|(key, entry)| Some((keys::refused_check(key)?, entry))) - .try_for_each(|(check, entry)| match flag(Some(entry))? { - true => Err(Error::Unenforceable { - setting: entry.source.clone(), - check, - }), - false => Ok(()), + .filter_map(|(key, entry)| Some((keys::refusal(key)?, entry))) + .try_for_each(|((check, asked), entry)| { + let asks = match asked { + Asked::WhenTrue => flag(Some(entry))?, + Asked::WhenSet => is_present(&entry.value), + }; + match asks { + true => Err(Error::Unenforceable { + setting: entry.source.clone(), + check, + }), + false => Ok(()), + } }) } diff --git a/litellm-rust/crates/redis-resolve/tests/catalog.rs b/litellm-rust/crates/redis-resolve/tests/catalog.rs index 517fc3f7bf1..f9e91c74471 100644 --- a/litellm-rust/crates/redis-resolve/tests/catalog.rs +++ b/litellm-rust/crates/redis-resolve/tests/catalog.rs @@ -1,18 +1,37 @@ use std::collections::{BTreeMap, BTreeSet}; -use litellm_redis_resolve::{Error, Handling, SETTINGS, Shape, interpret, kwargs}; +use litellm_redis_resolve::{Asked, Error, Handling, SETTINGS, Setting, Shape, interpret, kwargs}; use rstest::rstest; use serde::Deserialize; use serde_json::json; #[derive(Deserialize)] struct Catalog { + meta: Meta, args: BTreeMap, } +#[derive(Deserialize)] +struct Meta { + redis_py_locked: String, +} + #[derive(Deserialize)] struct Arg { environment: bool, + redis_py: BTreeMap>, +} + +// the signatures of the clients the proxy builds, where an unknown kwarg raises TypeError +const PROXY_SIGNATURES: &[&str] = &["async", "cluster", "connection"]; + +fn rejected() -> impl Iterator { + SETTINGS + .iter() + .filter_map(|setting| match setting.handling { + Handling::Reject { asked, .. } => Some((setting, asked)), + Handling::Read | Handling::Warn { .. } => None, + }) } fn catalog() -> Catalog { @@ -78,17 +97,38 @@ fn every_warned_setting_is_reported_and_still_reaches_the_host() { } #[rstest] -fn every_rejected_setting_fails_when_asked_for() { - let rejected = SETTINGS - .iter() - .filter(|setting| matches!(setting.handling, Handling::Reject { .. })); - - for setting in rejected { - let error = interpret(&host_with(setting.kwarg, json!(true)), None).unwrap_err(); +fn every_rejected_setting_fails_when_asked_for_and_not_otherwise() { + for (setting, asked) in rejected() { + let (asking, not_asking) = match asked { + Asked::WhenTrue => (json!(true), json!(false)), + Asked::WhenSet => (json!(["VERIFY_CRL_CHECK_LEAF"]), json!([])), + }; + let error = interpret(&host_with(setting.kwarg, asking), None).unwrap_err(); assert!( matches!(&error, Error::Unenforceable { setting: source, .. } if source == setting.kwarg), "{error:?}" ); + assert!(interpret(&host_with(setting.kwarg, not_asking), None).is_ok()); } } + +#[rstest] +fn python_already_refuses_every_rejected_setting() { + let catalog = catalog(); + + let accepted: Vec<&str> = rejected() + .map(|(setting, _)| setting.kwarg) + .filter(|kwarg| { + catalog.args[*kwarg] + .redis_py + .get(&catalog.meta.redis_py_locked) + .is_some_and(|signatures| { + signatures + .iter() + .any(|signature| PROXY_SIGNATURES.contains(&signature.as_str())) + }) + }) + .collect(); + assert!(accepted.is_empty(), "{accepted:?}"); +} diff --git a/litellm-rust/crates/redis-resolve/tests/interpret.rs b/litellm-rust/crates/redis-resolve/tests/interpret.rs index a2f033644e7..0be83cf794a 100644 --- a/litellm-rust/crates/redis-resolve/tests/interpret.rs +++ b/litellm-rust/crates/redis-resolve/tests/interpret.rs @@ -277,6 +277,11 @@ fn expected_view(fields: serde_json::Value) -> serde_json::Value { &[], Err("ssl_validate_ocsp asks for OCSP revocation checks, which rustls cannot perform, so Redis is not connected without them"), )] +#[case::ssl_include_verify_flags_is_unsupported( + json!({"host": "redis", "ssl": true, "ssl_include_verify_flags": ["VERIFY_CRL_CHECK_LEAF"]}), + &[], + Err("ssl_include_verify_flags asks for X.509 verify flags such as CRL checking, which rustls cannot perform, so Redis is not connected without them"), +)] #[case::ssl_password_is_unsupported( json!({"host": "redis", "ssl": true, "ssl_certfile": "/client.pem", "ssl_password": "key-secret"}), &[], diff --git a/tests/unit/test_redis.py b/tests/unit/test_redis.py index f6c5ebafb92..1578b6fe963 100644 --- a/tests/unit/test_redis.py +++ b/tests/unit/test_redis.py @@ -2170,6 +2170,18 @@ def _expected_view(fields: dict[str, object]) -> dict[str, object]: }, id="ssl_validate_ocsp_is_unsupported", ), + pytest.param( + {"host": "redis", "ssl": True, "ssl_include_verify_flags": ["VERIFY_CRL_CHECK_LEAF"]}, + {}, + {"host": "redis", "ssl": True, "ssl_include_verify_flags": ["VERIFY_CRL_CHECK_LEAF"]}, + { + "rejected": { + "message": "Redis.__init__() got an unexpected keyword argument 'ssl_include_verify_flags'", + "type": "TypeError", + } + }, + id="ssl_include_verify_flags_is_unsupported", + ), pytest.param( { "db": 2,