diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 9f51483702..aa03b577ae 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -22,13 +22,18 @@ from utils.cors_origin import normalize_web_app_origin from unstract.core.cache.redis_client import ( + apply_url_credentials, build_socketio_redis_url, ensure_tls_query_params, parse_db, + parse_port, + resolve_sentinel_master_check_hostname, resolve_ssl_cert_reqs, resolve_ssl_check_hostname, + resolve_ssl_enabled, set_url_db_path, url_db_path, + url_username_from_env, ) # Django 5.0+ caps URLValidator at 2048 chars. S3 pre-signed URLs signed with @@ -112,12 +117,20 @@ def get_required_setting(setting_key: str, default: str | None = None) -> str | REDIS_USER = os.environ.get("REDIS_USER", "default") REDIS_PASSWORD = os.environ.get("REDIS_PASSWORD", "") REDIS_HOST = os.environ.get("REDIS_HOST", "localhost") -REDIS_PORT = os.environ.get("REDIS_PORT", "6379") +# Through the shared parser, not a bare int() at the Sentinel call site below: +# a blank REDIS_PORT= — this repo's "leave the default" spelling — raised +# ValueError while Django settings were being imported, so the backend alone +# failed to start while every worker came up healthy on 6379. +REDIS_PORT = parse_port(os.environ.get("REDIS_PORT"), "REDIS_PORT", 6379) REDIS_DB = os.environ.get("REDIS_DB", "") # TLS to Redis (UN-4123). Off by default, so the in-cluster/local server is # untouched. `rediss://` is what actually selects TLS for both django-redis and # kombu; this flag only decides which scheme gets built. -REDIS_SSL = os.environ.get("REDIS_SSL", "false").strip().lower() == "true" +# Through the shared resolver, not a local `== "true"`: _TRUE_LITERALS accepts +# 1, yes and on, so the bare comparison read REDIS_SSL=1 as FALSE — the workers +# connected rediss:// while this cache built a redis:// LOCATION and skipped its +# CONNECTION_POOL_KWARGS, i.e. one endpoint with two TLS policies in one process. +REDIS_SSL = resolve_ssl_enabled() # Resolved by unstract.core, not re-read here: the raw value needs trimming, # lower-casing and validating, and a second copy of that logic is how this file # and create_redis_client came to hold two verification policies for one endpoint. @@ -552,18 +565,67 @@ def filter(self, record): REDIS_SENTINEL_MASTER_NAME = os.environ.get("REDIS_SENTINEL_MASTER_NAME", "mymaster") if REDIS_SENTINEL_MODE: + # This branch used to hold four divergences from create_redis_client, all + # pre-existing. They are closed here because "the cache and the client reach + # the same place" is not a property that can stop at a mode boundary — an + # operator on Sentinel gets the same guarantee or the guarantee is a + # half-truth. TestSentinelModeAgreesWithCore covers each one. + # + # The username comes from the shared resolver, not the module-level + # REDIS_USER: that one defaults to "default", so this cache sent a + # two-argument ACL AUTH where core sends the one-argument form, and it read + # only REDIS_USER so the REDIS_USERNAME spelling platform-service ships was + # ignored entirely. + _sentinel_username = url_username_from_env() + + # 26379 — the Sentinel port — matching core's + # _resolve_redis_env(default_port="26379"). The module-level REDIS_PORT + # defaults to 6379, which is the standalone port, so an unset REDIS_PORT + # pointed this cache at the wrong port while every other client found the + # sentinels. The chart always sets it, which is why this stayed hidden. + REDIS_PORT = parse_port(os.environ.get("REDIS_PORT"), "REDIS_PORT", 26379) + _sentinel_kwargs = {} if REDIS_PASSWORD: _sentinel_kwargs["password"] = REDIS_PASSWORD - if REDIS_USER: - _sentinel_kwargs["username"] = REDIS_USER + if _sentinel_username: + _sentinel_kwargs["username"] = _sentinel_username + + # TLS reached neither the discovery connections nor the master one, so a + # TLS-only Sentinel deployment got a plaintext cache while core encrypted + # both. Core builds them from one env dict for exactly this reason; the + # same settings go to both here. + _sentinel_pool_kwargs = {} + if REDIS_SSL: + _sentinel_tls = {"ssl": True, "ssl_cert_reqs": REDIS_SSL_CERT_REQS} + if REDIS_SSL_CA_CERTS: + _sentinel_tls["ssl_ca_certs"] = REDIS_SSL_CA_CERTS + + # The two planes answer by DIFFERENT rules, and copying one dict into + # both got that wrong. Discovery connects to REDIS_HOST, a name whose + # certificate can match, so it verifies like any other client. The + # MASTER connects to whatever SENTINEL get-master-addr-by-name returns — + # an IP that SentinelManagedConnection hands to SSLConnection as + # server_hostname, which no DNS SAN covers and which changes on + # failover. core turns checking off there unless the operator asked + # explicitly; this cache has to agree, or it fails verification against + # the same master that create_redis_client reaches. + _sentinel_kwargs.update(_sentinel_tls) + _sentinel_pool_kwargs.update(_sentinel_tls) + if REDIS_SSL_CERT_REQS != "none": + _sentinel_kwargs["ssl_check_hostname"] = REDIS_SSL_CHECK_HOSTNAME + _sentinel_pool_kwargs["ssl_check_hostname"] = ( + resolve_sentinel_master_check_hostname() + ) _redis_db = parse_db(REDIS_DB, "REDIS_") # SocketIO connection manager (Kombu Sentinel URL format) _cred_prefix = "" - if REDIS_USER and REDIS_PASSWORD: - _cred_prefix = f"{quote(REDIS_USER, safe='')}:{quote(REDIS_PASSWORD, safe='')}@" + if _sentinel_username and REDIS_PASSWORD: + _cred_prefix = ( + f"{quote(_sentinel_username, safe='')}:{quote(REDIS_PASSWORD, safe='')}@" + ) elif REDIS_PASSWORD: _cred_prefix = f":{quote(REDIS_PASSWORD, safe='')}@" SOCKET_IO_MANAGER_URL = ( @@ -572,7 +634,7 @@ def filter(self, record): SOCKET_IO_TRANSPORT_OPTIONS = {"master_name": REDIS_SENTINEL_MASTER_NAME} # django-redis expects username in the LOCATION URL for ACL auth - _user_prefix = f"{REDIS_USER}@" if REDIS_USER else "" + _user_prefix = f"{_sentinel_username}@" if _sentinel_username else "" CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", @@ -581,8 +643,12 @@ def filter(self, record): "CLIENT_CLASS": "django_redis.client.SentinelClient", "CONNECTION_POOL_CLASS": "redis.sentinel.SentinelConnectionPool", "CONNECTION_FACTORY": "django_redis.pool.SentinelConnectionFactory", - "SENTINELS": [(REDIS_HOST, int(REDIS_PORT))], + "SENTINELS": [(REDIS_HOST, REDIS_PORT)], "SENTINEL_KWARGS": _sentinel_kwargs, + # TLS for the MASTER connection. SENTINEL_KWARGS covers only the + # discovery ones; core carries both, so a deployment with TLS on + # got an encrypted discovery and a plaintext master. + "CONNECTION_POOL_KWARGS": _sentinel_pool_kwargs, "DB": _redis_db, "PASSWORD": REDIS_PASSWORD, "SERIALIZER": "django_redis.serializers.json.JSONSerializer", @@ -643,23 +709,72 @@ def filter(self, record): # So the db has to travel in the URL path, or this cache silently sits on db 0 # while every other service honours REDIS_DB: workers would RPUSH # log_history_queue to db N and the backend would LPOP an empty db 0. - # USERNAME and DB below are passed for readability and are DISCARDED by - # django-redis — they are not the mechanism for either. Auth stays password-only - # as the built-in `default` user (what a managed AUTH string is): a username - # would turn AUTH into its two-argument ACL form, and this cache never sends - # one. If a django-redis bump ever starts reading USERNAME, that becomes a real - # behaviour change rather than a silent one — which is what the assertions in - # tests/test_redis_settings_derivation.py pin. + # DB below is passed for readability and is DISCARDED by django-redis — it is + # not the mechanism. An ACL USERNAME is not passed through OPTIONS at all, + # because django-redis would discard that too: it travels in the LOCATION, + # the same route the URL branch uses. This used to say auth "stays + # password-only as the built-in `default` user ... this cache never sends + # one", which described the discrete path accurately and made it the one + # consumer that could not reach an ACL endpoint — create_redis_client in the + # same process sends the username, so where `default` is disabled the cache + # alone failed. That is the identical divergence the URL branch was fixed + # for, left standing on the path the module docstring calls primary. _cache_options = { "CLIENT_CLASS": "django_redis.client.DefaultClient", "SERIALIZER": "django_redis.serializers.json.JSONSerializer", } + _cache_username = url_username_from_env() if not _redis_url: - # Credentials and db travel IN the URL in URL mode; passing them again - # through OPTIONS risks one of them winning over the other. _cache_options["DB"] = _cache_db - _cache_options["USERNAME"] = REDIS_USER - _cache_options["PASSWORD"] = REDIS_PASSWORD + _cache_location = f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}" + if _cache_username: + # Only when a username must travel. Keeping the password in OPTIONS + # otherwise is deliberate: OPTIONS["PASSWORD"] matches Django's + # SafeExceptionReporterFilter and is masked in a settings dump, + # while LOCATION is not. Pay that exposure only where django-redis + # leaves no alternative. + _cache_location = apply_url_credentials( + _cache_location, REDIS_PASSWORD, _cache_username + ) + else: + _cache_options["PASSWORD"] = REDIS_PASSWORD + else: + # URL mode: the credentials go into the LOCATION, through the SAME helper + # the Socket.IO URL uses. Two reasons it is not OPTIONS["PASSWORD"]: + # + # - django-redis 5.4.0 discards OPTIONS["USERNAME"], so an ACL username + # could not reach this cache at all. It would authenticate as the + # built-in `default` user while create_redis_client in the same + # process authenticated as the configured one — and on an endpoint + # where `default` is disabled, the cache alone would fail. + # - a hand-rolled gate here diverged from the core one within a single + # PR: it keyed on "@" being absent from the netloc, which the core + # comment explicitly rejects, so a URL carrying only an ACL username + # left this cache anonymous. One helper, one rule, no second copy to + # keep in step. + # + # The helper returns the URL untouched when it already carries a + # password, so a credential-bearing REDIS_URL behaves exactly as before. + # The username comes from the SAME resolver the core client uses, not + # from a hand-written os.environ.get here. Two reasons, and the second + # was a live divergence: + # + # - the module-level REDIS_USER above defaults to "default", while + # create_redis_client reads the variable with no default. Passing + # the fallback would make this cache send the two-argument + # `AUTH default ` while the client in the same process sends the + # one-argument form. + # - the resolver accepts BOTH spellings, REDIS_USER and + # REDIS_USERNAME. platform-service ships the second one, so a single + # shared Redis-credential secret can easily inject it — and reading + # only REDIS_USER left this cache authenticating as `default` while + # the client beside it authenticated as the configured ACL user. + # + # Re-deriving the chain here is the same second copy this commit removed + # for the "@" heuristic; asking core for it is what keeps the three + # consumers in step by construction rather than by comment. + _redis_url = apply_url_credentials(_redis_url, REDIS_PASSWORD, _cache_username) + _cache_location = _redis_url # Gated on the EFFECTIVE scheme, not the flag. In URL mode TLS is carried by # the URL (and its query string), so a plaintext REDIS_URL left behind while # REDIS_SSL=true would otherwise hand ssl_cert_reqs to a plain @@ -697,11 +812,17 @@ def filter(self, record): # counters and the dashboard caches go with it, and during a rolling # deploy old pods read db 0 while new pods read db N. Drain # log_history_queue before cutting over. + # + # The message deliberately does NOT explain where the previous db came + # from. It fires on both paths — discrete, where the cache sat on db 0 + # regardless because django-redis ignores OPTIONS['DB'], and URL mode, + # where it came from the URL's own path — and the earlier wording gave + # the discrete explanation to both. The db NUMBERS are right in every + # case; only the stated reason was wrong on the URL path. logging.getLogger(__name__).warning( - "Django cache is moving from Redis db %s to db %s (REDIS_DB). In " - "discrete mode it previously sat on db 0 regardless, because " - "django-redis ignores OPTIONS['DB']. Anything already in db %s — " - "including log_history_queue — stays there.", + "Django cache is moving from Redis db %s to db %s (REDIS_DB). " + "Anything already in db %s — including log_history_queue — stays " + "there.", _previous_db, _effective_db, _previous_db, @@ -710,8 +831,7 @@ def filter(self, record): CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", - "LOCATION": _redis_url - or f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}", + "LOCATION": _cache_location, "OPTIONS": _cache_options, "KEY_FUNCTION": "utils.redis_cache.custom_key_function", } diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py index 67d3aa0d7c..7a474d707f 100644 --- a/backend/backend/tests/test_redis_settings_derivation.py +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -19,12 +19,81 @@ import logging import pathlib +import re +from urllib.parse import urlsplit import pytest _SETTINGS = pathlib.Path(__file__).resolve().parents[1] / "settings" / "base.py" +def _slice_bounds(source: str) -> dict[str, tuple[int, int]]: + """Character ranges of base.py that _derive executes. + + Factored out so the coverage guard below can assert on the SAME ranges the + harness runs, rather than a second description of them that could drift. + """ + defs_start = source.index('REDIS_USER = os.environ.get("REDIS_USER"') + defs_end = ( + source.index("\n", source.index('REDIS_URL = os.environ.get("REDIS_URL"')) + 1 + ) + imports_start = source.index("from unstract.core.cache.redis_client import (") + imports_end = source.index(")\n", imports_start) + 2 + urllib_start = source.index("from urllib.parse import ") + urllib_end = source.index("\n", urllib_start) + 1 + return { + "urllib": (urllib_start, urllib_end), + "imports": (imports_start, imports_end), + "defs": (defs_start, defs_end), + "block": ( + source.index("REDIS_SENTINEL_MODE = ("), + source.index("SESSION_ENGINE ="), + ), + } + + +def test_the_harness_covers_every_redis_line_in_the_settings_file(): + """The splice must not silently stop covering the code it claims to test. + + _derive executes two ranges out of base.py with a ~415-line gap between + them, and anything in that gap is invisible to every assertion in this + file. That is not theoretical: adding `REDIS_PASSWORD = ""` at base.py:251, + or appending a CACHES["default"]["LOCATION"] override after SESSION_ENGINE, + leaves all of these tests green while shipping a broken cache. + + The file already concedes the gap once — test_no_database_var_is_parsed_with + _a_bare_int greps the source text because FILE_ACTIVE_CACHE_REDIS_DB sits + outside both slices. A grep only covers the one pattern someone thought of; + this covers the boundary itself, and fails naming the line that escaped. + """ + source = _SETTINGS.read_text() + bounds = _slice_bounds(source) + covered = [] + for start, end in bounds.values(): + covered.append(range(start, end)) + + offenders = [] + offset = 0 + for line in source.splitlines(keepends=True): + if re.match(r"\s*(REDIS_|_redis|_cache|CACHES|SOCKET_IO)", line) and not any( + offset in span for span in covered + ): + offenders.append((source[:offset].count("\n") + 1, line.strip()[:70])) + offset += len(line) + + # Known and deliberate: these are asserted by source-text inspection + # instead, because they are consumed far from the derivation block. + allowed = {"FILE_ACTIVE_CACHE_REDIS_DB", "REDIS_DB_PORTAL"} + offenders = [o for o in offenders if not any(a in o[1] for a in allowed)] + + assert not offenders, ( + "these Redis lines in base.py are OUTSIDE the ranges _derive executes, " + "so no test in this file can observe them:\n" + + "\n".join(f" base.py:{n}: {text}" for n, text in offenders) + + "\nWiden the slice, or add the name to `allowed` with a reason." + ) + + def _derive(**env: str) -> dict: """Execute the standalone Redis block with the given env.""" source = _SETTINGS.read_text() @@ -55,10 +124,17 @@ def _derive(**env: str) -> dict: # without it that branch raises NameError instead of logging, which is part of # why it went untested. ns: dict = {"__name__": "backend.settings.base"} + # The urllib import is spliced from source for the same reason as the + # unstract.core one: a hand-written copy must be remembered every time the + # settings module starts using another name from it, and the symptom is a + # NameError in every case rather than one clear failure. + urllib_start = source.index("from urllib.parse import ") + urllib_end = source.index("\n", urllib_start) + 1 + prelude = ( - "import logging\n" - "import os\n" - "from urllib.parse import quote\n" + source[imports_start:imports_end] + "import logging\nimport os\n" + + source[urllib_start:urllib_end] + + source[imports_start:imports_end] ) + source[defs_start:defs_end] import os as _os @@ -98,20 +174,35 @@ def test_the_password_reaches_the_cache(self): derived = _derive(REDIS_HOST="h", REDIS_PASSWORD="s3cret") assert derived["CACHES"]["default"]["OPTIONS"]["PASSWORD"] == "s3cret" - def test_db_and_username_are_passed_through_options(self): - """Pins the stated invariant so a django-redis bump is visible. - - The comment beside this code says USERNAME is deliberately not honoured — - django-redis 5.4.0 discards it, so auth stays password-only as the - built-in `default` user. That holds by accident of the pinned version: - these assertions pin what the settings SEND, so if a bump starts reading - USERNAME the change is a deliberate one rather than a surprise. + def test_an_acl_username_travels_in_the_location_not_options(self): + """This used to assert OPTIONS["USERNAME"] == "alice" and call that the + stated invariant — django-redis discards the key, so auth "stays + password-only as the built-in `default` user". + + That made the discrete path the one consumer unable to reach an ACL + endpoint: create_redis_client in the same process sends the username, so + where `default` is disabled the cache alone failed. Passing a key the + library throws away is not an invariant worth pinning; reaching the same + server as everything else is. The LOCATION is the only route django-redis + leaves, which is exactly why the URL branch already used it. """ - options = _derive(REDIS_HOST="h", REDIS_DB="3", REDIS_USER="alice")["CACHES"][ - "default" - ]["OPTIONS"] - assert options["DB"] == 3 - assert options["USERNAME"] == "alice" + derived = _derive( + REDIS_HOST="h", REDIS_DB="3", REDIS_USER="alice", REDIS_PASSWORD="pw" + ) + cache = derived["CACHES"]["default"] + assert cache["OPTIONS"]["DB"] == 3 + assert "USERNAME" not in cache["OPTIONS"] + parts = urlsplit(cache["LOCATION"]) + assert parts.username == "alice" + assert parts.password == "pw" + assert parts.hostname == "h" + + def test_no_username_keeps_the_password_out_of_the_location(self): + """OPTIONS["PASSWORD"] is masked by Django's settings filter; LOCATION is + not. Pay that exposure only where django-redis leaves no alternative.""" + cache = _derive(REDIS_HOST="h", REDIS_PASSWORD="pw")["CACHES"]["default"] + assert cache["OPTIONS"]["PASSWORD"] == "pw" + assert urlsplit(cache["LOCATION"]).password is None def test_url_mode_does_not_duplicate_credentials_into_options(self): """They travel in the URL; passing both risks one winning over the other.""" @@ -130,7 +221,10 @@ def test_db_travels_in_the_location(self): an empty db 0. """ derived = _derive(REDIS_HOST="h", REDIS_DB="3") - assert derived["CACHES"]["default"]["LOCATION"].endswith("/3") + # Full string, not endswith("/3"): the failure this guards is + # same-endpoint-WRONG-db, and endswith is equally satisfied by + # "redis://WRONGHOST:6379/3" or a changed port. + assert derived["CACHES"]["default"]["LOCATION"] == "redis://h:6379/3" def test_ssl_switches_scheme_and_pool_kwargs(self): derived = _derive(REDIS_HOST="h", REDIS_SSL="true", REDIS_SSL_CA_CERTS="/ca.pem") @@ -438,3 +532,437 @@ def test_no_database_var_is_parsed_with_a_bare_int(self): "unstract.core.cache.redis_client.parse_db so the backend and the " "workers agree on a malformed value." ) + + +class TestUrlModeCacheCredentials: + """The cache credentials travel in the LOCATION, through the shared helper. + + Not OPTIONS["PASSWORD"]: django-redis 5.4.0 discards OPTIONS["USERNAME"], so + an ACL username could not reach this cache at all — it would authenticate as + the built-in `default` user while create_redis_client in the same process + used the configured one. + + A hand-rolled gate here also diverged from the core one within a single PR: + it keyed on "@" being absent from the netloc, which the core comment + explicitly rejects, leaving a username-only URL anonymous. One helper, one + rule. + """ + + def _location(self, **env: str) -> str: + return _derive(**env)["CACHES"]["default"]["LOCATION"] + + def test_the_password_fills_a_gap_the_url_leaves(self): + assert ( + urlsplit( + self._location(REDIS_URL="rediss://h:6380/0", REDIS_PASSWORD="s3cret") + ).password + == "s3cret" + ) + + def test_a_username_only_url_still_gets_the_password(self): + """The shape the `@` heuristic got wrong.""" + parts = urlsplit( + self._location(REDIS_URL="redis://alice@h:6379/0", REDIS_PASSWORD="s3cret") + ) + assert parts.hostname == "h" + assert parts.username == "alice" + assert parts.password == "s3cret" + + def test_an_acl_username_reaches_the_cache(self): + """django-redis discards OPTIONS["USERNAME"], so the LOCATION is the + only lever. Without it the cache authenticates as `default` while the + other consumers use the configured user. + """ + parts = urlsplit( + self._location( + REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="pw", REDIS_USER="alice" + ) + ) + assert parts.username == "alice" + assert parts.password == "pw" + + def test_a_url_carrying_credentials_is_left_alone(self): + assert ( + urlsplit( + self._location( + REDIS_URL="rediss://:in-url@h:6380/0", REDIS_PASSWORD="s3cret" + ) + ).password + == "in-url" + ) + + def test_no_password_stays_anonymous(self): + assert urlsplit(self._location(REDIS_URL="rediss://h:6380/0")).password is None + + def test_the_shipped_default_user_alone_adds_nothing(self): + """REDIS_USER defaults to "default" in this module but not in + create_redis_client; passing the fallback would make the two send + different AUTH forms for the same configuration. + + The password must be NON-empty. With REDIS_PASSWORD="" the helper + returns at its `if not password` guard before the username argument is + read at all, so the test named for this choice could not fail on it — + mutating the call to pass the module-level REDIS_USER left the suite + green while changing a one-argument AUTH into a two-argument one. + """ + assert ( + self._location(REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="pw") + == "redis://:pw@h:6379/0" + ) + + def test_the_cache_honours_the_second_username_spelling(self): + """platform-service ships REDIS_USERNAME, the chart ships REDIS_USER. + + Reading only REDIS_USER left this cache authenticating as the built-in + `default` while create_redis_client in the SAME process authenticated + as the configured ACL user — and where `default` is disabled, only the + cache fails. + """ + assert ( + self._location( + REDIS_URL="redis://h:6379/0", REDIS_PASSWORD="pw", REDIS_USERNAME="alice" + ) + == "redis://alice:pw@h:6379/0" + ) + + def test_discrete_mode_still_uses_options(self): + """URL mode moved to the LOCATION; the discrete path did not change.""" + options = _derive(REDIS_HOST="h", REDIS_PASSWORD="s3cret")["CACHES"]["default"][ + "OPTIONS" + ] + assert options["PASSWORD"] == "s3cret" + + +class TestTheSslFlagParsesTheSameEverywhere: + """REDIS_SSL decides TLS for the whole platform, so every consumer must + read the same literals. A local `== "true"` here read `REDIS_SSL=1` as + FALSE while unstract.core read it as True: the workers connected rediss:// + and this cache built a redis:// LOCATION and skipped its + CONNECTION_POOL_KWARGS entirely — against a TLS-only managed endpoint the + backend cache alone fails, and against one accepting both it authenticates + in clear while everything else is encrypted. + """ + + @pytest.mark.parametrize("literal", ["true", "1", "yes", "on", "TRUE", " on "]) + def test_every_true_literal_switches_the_scheme(self, literal): + derived = _derive(REDIS_HOST="h", REDIS_SSL=literal) + assert derived["CACHES"]["default"]["LOCATION"].startswith("rediss://") + assert derived["REDIS_SSL"] is True + + @pytest.mark.parametrize("literal", ["false", "0", "no", "off", "", " "]) + def test_every_false_and_blank_literal_leaves_it_plaintext(self, literal): + derived = _derive(REDIS_HOST="h", REDIS_SSL=literal) + assert derived["CACHES"]["default"]["LOCATION"].startswith("redis://") + assert derived["REDIS_SSL"] is False + + def test_the_pool_kwargs_follow_the_same_flag(self): + """The scheme and the verification settings must not disagree: the + `if REDIS_SSL` gate guards both, so a literal one reader accepts and + the other does not strips the cert settings as well as the scheme.""" + derived = _derive(REDIS_HOST="h", REDIS_SSL="1") + assert derived["CACHES"]["default"]["OPTIONS"]["CONNECTION_POOL_KWARGS"] + + +class TestTheBackendAgreesWithCoreOnEverySharedSetting: + """A parity sweep, not another one-off. + + Four review rounds on this PR produced the same finding four times in + different clothes: a setting was centralised in unstract.core and the + parallel read in this settings module was left behind — the "@" credential + heuristic, the REDIS_USERNAME spelling, blank-means-unset, and the SSL + true-literals. Each was fixed with a test for that one setting, and the next + round found the next one. + + This pins the PROPERTY instead: for a matrix of environments, what the + Django cache ends up talking to must match what create_redis_client in the + same process would. A future setting that is centralised on one side only + fails here without anyone having to predict which setting it will be. + """ + + # Each case is what an operator actually writes, not a minimal pair. + _CASES = [ + {"REDIS_HOST": "h"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USER": "alice"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USERNAME": "alice"}, + {"REDIS_HOST": "h", "REDIS_SSL": "true"}, + {"REDIS_HOST": "h", "REDIS_SSL": "1"}, + {"REDIS_HOST": "h", "REDIS_SSL": "yes", "REDIS_SSL_CERT_REQS": "none"}, + {"REDIS_URL": "redis://h:6379/0"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_PASSWORD": "pw", "REDIS_USER": "alice"}, + { + "REDIS_URL": "redis://h:6379/0", + "REDIS_PASSWORD": "pw", + "REDIS_USERNAME": "bob", + }, + {"REDIS_URL": "redis://alice@h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "redis://:inurl@h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "redis://:@h:6379/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "rediss://h:6380/0", "REDIS_PASSWORD": "pw"}, + {"REDIS_URL": "rediss://h:6380/0", "REDIS_PASSWORD": "p@ss/w:rd"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_DB": "3", "REDIS_PASSWORD": "pw"}, + # Username with NO password: neither side can honour it, so what the + # sweep pins is that they fail the SAME way rather than one going + # anonymous while the other crashes inside redis-py. + {"REDIS_HOST": "h", "REDIS_USER": "alice"}, + {"REDIS_URL": "redis://h:6379/0", "REDIS_USERNAME": "alice"}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_USER": ""}, + {"REDIS_HOST": "h", "REDIS_PASSWORD": "pw", "REDIS_SSL": ""}, + ] + + @staticmethod + def _from_core(env: dict) -> dict: + """What create_redis_client would connect with, for this environment.""" + import os + + from unstract.core.cache.redis_client import create_redis_client + + saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k} + for key in saved: + os.environ.pop(key, None) + os.environ.update(env) + try: + kwargs = create_redis_client().connection_pool.connection_kwargs + cls = create_redis_client().connection_pool.connection_class.__name__ + finally: + for key in list(os.environ): + if "REDIS" in key: + os.environ.pop(key, None) + os.environ.update({k: v for k, v in saved.items() if v is not None}) + return { + "host": kwargs.get("host"), + "port": kwargs.get("port"), + "db": int(kwargs.get("db") or 0), + "username": kwargs.get("username") or None, + "password": kwargs.get("password") or None, + "tls": cls == "SSLConnection", + } + + @staticmethod + def _from_cache(env: dict) -> dict: + """The same facts, resolved by django-redis ITSELF. + + Not by parsing the LOCATION string. django-redis's precedence — the + LOCATION's userinfo wins, OPTIONS["PASSWORD"] only backfills — is true + of 5.4.0, but hardcoding it here would be a second copy of library + behaviour inside the very test whose job is to survive changes it cannot + predict. A bump that reversed the precedence would leave this green + while the cache diverged, which is the exact failure mode this class + replaced. Asking ConnectionFactory means the rule is never restated. + """ + from django_redis.pool import ConnectionFactory + + derived = _derive(**env) + cache = derived["CACHES"]["default"] + options = { + k: v + for k, v in cache["OPTIONS"].items() + if k not in ("CLIENT_CLASS", "SERIALIZER") + } + # The factory memoises pools by LOCATION, so a stale entry from an + # earlier case would answer for this one. + ConnectionFactory._pools = {} + client = ConnectionFactory(dict(options)).connect(cache["LOCATION"]) + pool = client.connection_pool + kwargs = pool.connection_kwargs + return { + "host": kwargs.get("host"), + "port": int(kwargs.get("port") or derived["REDIS_PORT"]), + "db": int(kwargs.get("db") or 0), + "username": kwargs.get("username") or None, + "password": kwargs.get("password") or None, + "tls": pool.connection_class.__name__ == "SSLConnection", + } + + @pytest.mark.parametrize("env", _CASES, ids=lambda e: ",".join(sorted(e))) + def test_the_cache_and_the_client_reach_the_same_place(self, env): + assert self._from_cache(env) == self._from_core(env) + + +class TestSentinelModeAgreesWithCore: + """The parity property must not stop at a mode boundary. + + TestTheBackendAgreesWithCoreOnEverySharedSetting cannot cover Sentinel: its + _from_core calls create_redis_client, which in Sentinel mode performs real + discovery and blocks. So the comparison here is against the resolver rather + than a built client — _resolve_redis_env with the same default_port core's + Sentinel path passes — which is enough to pin the four facts that diverged. + + The branch was NOT untestable, which an earlier comment in base.py claimed: + _derive's slice spans both branches, so it just needed an env that selects + this one. "No test executes it" and "no test was written for it" are + different statements, and only the second was true. + """ + + @staticmethod + def _core(**env: str) -> dict: + import os + + from unstract.core.cache.redis_client import _resolve_redis_env + + saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k} + for key in saved: + os.environ.pop(key, None) + os.environ.update(env) + try: + return _resolve_redis_env("REDIS_", default_port="26379") + finally: + for key in list(os.environ): + if "REDIS" in key: + os.environ.pop(key, None) + os.environ.update({k: v for k, v in saved.items() if v is not None}) + + def test_the_sentinel_port_default_matches_core(self): + """REDIS_PORT unset pointed this cache at 6379 — the STANDALONE port — + while every other client found the sentinels on 26379.""" + env = {"REDIS_SENTINEL_MODE": "true", "REDIS_HOST": "sent"} + sentinels = _derive(**env)["CACHES"]["default"]["OPTIONS"]["SENTINELS"] + assert sentinels == [("sent", self._core(**env)["port"])] + assert sentinels == [("sent", 26379)] + + def test_an_explicit_port_still_wins(self): + env = {"REDIS_SENTINEL_MODE": "true", "REDIS_HOST": "sent", "REDIS_PORT": "27000"} + assert _derive(**env)["CACHES"]["default"]["OPTIONS"]["SENTINELS"] == [ + ("sent", 27000) + ] + + def test_no_username_configured_means_no_username_sent(self): + """REDIS_USER defaults to "default" at module scope, so this cache sent + a two-argument ACL AUTH where core sends the one-argument form.""" + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_PASSWORD": "pw", + } + cache = _derive(**env)["CACHES"]["default"] + assert self._core(**env)["username"] is None + assert "username" not in cache["OPTIONS"]["SENTINEL_KWARGS"] + assert urlsplit(cache["LOCATION"]).username is None + + @pytest.mark.parametrize("spelling", ["REDIS_USER", "REDIS_USERNAME"]) + def test_both_username_spellings_reach_the_sentinel_cache(self, spelling): + """platform-service ships REDIS_USERNAME; reading only REDIS_USER left + this cache authenticating as `default` while core used the ACL user.""" + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_PASSWORD": "pw", + spelling: "alice", + } + derived = _derive(**env) + cache = derived["CACHES"]["default"] + assert self._core(**env)["username"] == "alice" + assert cache["OPTIONS"]["SENTINEL_KWARGS"]["username"] == "alice" + assert urlsplit(cache["LOCATION"]).username == "alice" + assert urlsplit(derived["SOCKET_IO_MANAGER_URL"]).username == "alice" + + def test_tls_reaches_both_the_sentinels_and_the_master(self): + """SENTINEL_KWARGS covers only discovery. Carrying TLS there alone would + leave an encrypted discovery and a plaintext master; carrying it nowhere + — which is what this branch did — leaves a plaintext cache against a + TLS-only deployment while core encrypts both.""" + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_SSL": "true", + "REDIS_SSL_CERT_REQS": "none", + } + options = _derive(**env)["CACHES"]["default"]["OPTIONS"] + assert self._core(**env)["ssl"] is True + for where in ("SENTINEL_KWARGS", "CONNECTION_POOL_KWARGS"): + assert options[where]["ssl"] is True, where + assert options[where]["ssl_cert_reqs"] == "none", where + + def test_a_ca_reaches_both_too(self): + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_SSL": "true", + "REDIS_SSL_CA_CERTS": "/etc/ssl/ca.pem", + } + options = _derive(**env)["CACHES"]["default"]["OPTIONS"] + for where in ("SENTINEL_KWARGS", "CONNECTION_POOL_KWARGS"): + assert options[where]["ssl_ca_certs"] == "/etc/ssl/ca.pem", where + + def test_no_tls_configured_adds_no_tls_keys(self): + """Flag off must leave the Sentinel cache byte-identical to before.""" + options = _derive(REDIS_SENTINEL_MODE="true", REDIS_HOST="sent")["CACHES"][ + "default" + ]["OPTIONS"] + assert options["SENTINEL_KWARGS"] == {} + assert options["CONNECTION_POOL_KWARGS"] == {} + + +class TestSentinelTlsPlanesDifferCorrectly: + """Discovery and master verify by DIFFERENT rules, and one dict served both. + + Discovery connects to REDIS_HOST — a name whose certificate can match — so + it verifies like any other client. The master connects to whatever + `SENTINEL get-master-addr-by-name` returns, which SentinelManagedConnection + hands to SSLConnection as `server_hostname`: an IP no DNS SAN covers, and + one that changes on failover. core turns checking off there unless asked + explicitly, so this cache must too — otherwise it fails verification against + the very master create_redis_client connects to. + """ + + @staticmethod + def _options(**env: str) -> dict: + return _derive(REDIS_SENTINEL_MODE="true", REDIS_HOST="sent", **env)["CACHES"][ + "default" + ]["OPTIONS"] + + def test_the_master_does_not_verify_the_hostname_by_default(self): + options = self._options(REDIS_SSL="true") + assert options["SENTINEL_KWARGS"]["ssl_check_hostname"] is True + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is False + + def test_an_explicit_request_is_honoured_on_the_master(self): + """An operator who asks for it still gets it — core says the same.""" + options = self._options(REDIS_SSL="true", REDIS_SSL_CHECK_HOSTNAME="true") + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is True + + def test_an_explicit_false_is_honoured_on_both(self): + options = self._options(REDIS_SSL="true", REDIS_SSL_CHECK_HOSTNAME="false") + assert options["SENTINEL_KWARGS"]["ssl_check_hostname"] is False + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is False + + def test_cert_reqs_none_turns_checking_off_on_both_planes(self): + """redis-py 5.2.1 assigns check_hostname verbatim — it does NOT coerce + the pair — so CERT_NONE with checking on reaches Python's ssl module, + which rejects it. Verified against the pinned version.""" + options = self._options(REDIS_SSL="true", REDIS_SSL_CERT_REQS="none") + assert "ssl_check_hostname" not in options["SENTINEL_KWARGS"] + assert options["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] is False + + def test_the_master_matches_core_for_every_spelling(self): + """Pinned to the resolver core itself uses, not to a second copy.""" + import os + + from unstract.core.cache.redis_client import ( + resolve_sentinel_master_check_hostname, + ) + + for raw in (None, "true", "false", "1", "off"): + env = { + "REDIS_SENTINEL_MODE": "true", + "REDIS_HOST": "sent", + "REDIS_SSL": "true", + } + if raw is not None: + env["REDIS_SSL_CHECK_HOSTNAME"] = raw + derived = _derive(**env)["CACHES"]["default"]["OPTIONS"] + saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k} + for key in saved: + os.environ.pop(key, None) + os.environ.update(env) + try: + expected = resolve_sentinel_master_check_hostname() + finally: + for key in list(os.environ): + if "REDIS" in key: + os.environ.pop(key, None) + os.environ.update({k: v for k, v in saved.items() if v is not None}) + assert derived["CONNECTION_POOL_KWARGS"]["ssl_check_hostname"] == expected, ( + raw + ) diff --git a/backend/sample.env b/backend/sample.env index daea191349..ca5456167d 100644 --- a/backend/sample.env +++ b/backend/sample.env @@ -60,7 +60,13 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # 1. Discrete vars (what the Helm chart and these samples use). Set REDIS_SSL=true # alongside REDIS_HOST/REDIS_PORT. No URL-encoding to get wrong. # 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: -# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required +# REDIS_PASSWORD= +# REDIS_USER= # blank — see below +# (The password is better kept OUT of the URL. A URL is printed into log +# lines, error messages and deployment tooling; a password in one travels +# with it. Credentials in the URL still work and still win, so an existing +# rediss://:@host URL keeps behaving exactly as before.) # (6380 is an EXAMPLE, not a default — the TLS port is provider-specific: # Memorystore 6378, ElastiCache 6379, Azure Cache 6380. A wrong port # hangs the connection rather than erroring, so read it off the instance.) diff --git a/runner/sample.env b/runner/sample.env index efdf245a19..23a9bc5079 100644 --- a/runner/sample.env +++ b/runner/sample.env @@ -55,7 +55,13 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # 1. Discrete vars (what the Helm chart and these samples use). Set REDIS_SSL=true # alongside REDIS_HOST/REDIS_PORT. No URL-encoding to get wrong. # 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: -# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required +# REDIS_PASSWORD= +# REDIS_USER= # blank — see below +# (The password is better kept OUT of the URL. A URL is printed into log +# lines, error messages and deployment tooling; a password in one travels +# with it. Credentials in the URL still work and still win, so an existing +# rediss://:@host URL keeps behaving exactly as before.) # (6380 is an EXAMPLE, not a default — the TLS port is provider-specific: # Memorystore 6378, ElastiCache 6379, Azure Cache 6380. A wrong port # hangs the connection rather than erroring, so read it off the instance.) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index dbf30ba500..ca6c438379 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -136,15 +136,17 @@ def url_db_path(url: str) -> int | None: return None -def _parse_bool(raw: str, default: bool, name: str) -> bool: +def _parse_bool(raw: str | None, default: bool, name: str) -> bool: """Parse a boolean env var; blank means UNSET, unknown warns and defaults. Blank-means-unset is this repo's own convention — `FOO=` in a sample.env means "leave the default", and every other variable in UN-4123 treats it that way. `os.getenv` does not: it reports an empty string as SET, so a bare `os.getenv(...) == "true"` turns `FOO=` into False. + + Accepts None so callers can hand it env_chain's "nothing was set" directly. """ - value = raw.strip().lower() + value = (raw or "").strip().lower() if not value: return default if value in _TRUE_LITERALS: @@ -190,9 +192,7 @@ def resolve_ssl_cert_reqs(env_prefix: str = "REDIS_") -> str: endpoint, three verification policies. Case and stray whitespace had the same effect, since the "is verification off?" test is an equality check. """ - raw = os.getenv( - f"{env_prefix}SSL_CERT_REQS", os.getenv("REDIS_SSL_CERT_REQS", "") - ).strip() + raw = (env_chain(f"{env_prefix}SSL_CERT_REQS", "REDIS_SSL_CERT_REQS") or "").strip() value = raw.lower() if not value: return _DEFAULT_CERT_REQS @@ -208,6 +208,32 @@ def resolve_ssl_cert_reqs(env_prefix: str = "REDIS_") -> str: return value +def resolve_sentinel_master_check_hostname(env_prefix: str = "REDIS_") -> bool: + """ssl_check_hostname for a Sentinel-managed MASTER connection. + + The master plane answers by a DIFFERENT rule than the discovery plane, and + exporting it is what stops a consumer re-deriving it: a master connects to + whatever `SENTINEL get-master-addr-by-name` returns, and + SentinelManagedConnection assigns that address straight to ``self.host``, + which SSLConnection passes as ``server_hostname``. It is an IP, so + verification is checked against an address no DNS SAN covers — and pinning + an IP SAN is not a workable answer, because the address changes on failover. + + So it is OFF unless the operator asked for it explicitly, matching + _tls_kwargs(sentinel_master=True). Off as well when verification itself is + off: the pinned redis-py (5.2.1) does NOT coerce that pair — it assigns + check_hostname verbatim — so CERT_NONE with checking on reaches Python's ssl + module, which rejects it. + """ + raw, _ = env_chain_named( + f"{env_prefix}SSL_CHECK_HOSTNAME", "REDIS_SSL_CHECK_HOSTNAME" + ) + explicit = (raw or "").strip().lower() in _TRUE_LITERALS | _FALSE_LITERALS + if not explicit or resolve_ssl_cert_reqs(env_prefix) == "none": + return False + return resolve_ssl_check_hostname(env_prefix) + + def resolve_ssl_check_hostname(env_prefix: str = "REDIS_", default: bool = True) -> bool: """{prefix}SSL_CHECK_HOSTNAME, falling back to REDIS_SSL_CHECK_HOSTNAME, then on. @@ -216,13 +242,10 @@ def resolve_ssl_check_hostname(env_prefix: str = "REDIS_", default: bool = True) FALSE, silently downgrading the Django cache to an encrypted but unauthenticated connection. """ - return _parse_bool( - os.getenv( - f"{env_prefix}SSL_CHECK_HOSTNAME", os.getenv("REDIS_SSL_CHECK_HOSTNAME", "") - ), - default, - f"{env_prefix}SSL_CHECK_HOSTNAME", + raw, name = env_chain_named( + f"{env_prefix}SSL_CHECK_HOSTNAME", "REDIS_SSL_CHECK_HOSTNAME" ) + return _parse_bool(raw, default, name) def url_cert_reqs(url: str) -> str | None: @@ -322,7 +345,8 @@ def ensure_tls_query_params( """Add the TLS settings a `rediss://` URL is missing, leaving present ones alone. Anything that hands a URL to a library that reads TLS out of the query string - needs this: kombu's KombuManager takes a URL and NOTHING else, and + needs this: kombu's KombuManager reads TLS from the URL alone — its + connection_options reach kombu.Connection, but the TLS settings do not — and django-redis's LOCATION is a string too (it also reads OPTIONS["CONNECTION_POOL_KWARGS"], but the query string wins on conflict). @@ -380,6 +404,156 @@ def _compose_redis_url(env: dict[str, Any]) -> str: return f"{scheme}://{credentials}{env['host']}:{env['port']}" +def env_chain(*names: str) -> str | None: + """First NON-BLANK value among these env vars, else None. + + Blank means unset throughout this module — it is the "leave the default" + spelling the sample recipes and values files use, and it is why a nested + `os.getenv(prefixed, os.getenv(generic))` is wrong here: `os.getenv` reports + a set-but-empty variable as set, so the blank shadows the level below it. + """ + for name in names: + value = os.getenv(name, "") + # Emptiness is tested on the STRIPPED value; the RAW one is returned. + # Stripping the return value silently truncated a password or username + # that came from a file-mounted secret with a trailing newline, while + # backend/settings/base.py read the same variable unstripped — so one + # process authenticated two different ways against one endpoint. + if value.strip(): + return value + return None + + +def env_chain_named(*names: str) -> tuple[str | None, str]: + """env_chain, plus the name of the variable the value came from. + + So a warning about an unusable value can name the variable that is actually + SET. Reporting the prefixed name for a value that came from the generic one + sends an operator grepping for a key that does not exist in their config. + """ + for name in names: + value = os.getenv(name, "") + if value.strip(): + return value, name + return None, names[0] + + +def parse_port(raw: str | None, var_name: str, default_port: str | int) -> int: + """A port number; blank means unset, unparseable warns and falls back. + + parse_db already does this for the database and says why: an int() straight + out of os.getenv raises at client construction with a message naming neither + the variable nor the prefix, which in most consumers kills the process at + import and in the two that catch broadly degrades to no-cache. + """ + # Blank is unset, like everywhere else here. base.py hands this the raw env + # value rather than env_chain's None, so without this a bare `REDIS_PORT=` — + # the repo's own "leave the default" spelling — logged an ERROR at startup + # in the backend and nowhere else. + if raw is None or not raw.strip(): + return int(default_port) + try: + return int(raw) + except ValueError: + # error, not warning: the fallback is a PORT, and on a host serving both + # 6379 and 6380 a typo lands the client on the other server rather than + # failing — a wrong endpoint, reported as a wrong value. + logger.error("Invalid %s=%r; using %s", var_name, raw, default_port) + return int(default_port) + + +def resolve_ssl_enabled(env_prefix: str = "REDIS_") -> bool: + """{prefix}SSL, falling back to REDIS_SSL; blank means unset. + + Exported so the backend's settings module uses the SAME parse rather than + its own `== "true"`. _TRUE_LITERALS accepts 1, yes and on, so the bare + comparison read `REDIS_SSL=1` as FALSE — every create_redis_client consumer + connected rediss:// while the Django cache built a redis:// LOCATION and + skipped its CONNECTION_POOL_KWARGS entirely. One endpoint, one process, two + TLS policies, which is the failure this module was centralised to prevent. + """ + raw, name = env_chain_named(f"{env_prefix}SSL", "REDIS_SSL") + return _parse_bool(raw, False, name) + + +def url_username_from_env(env_prefix: str = "REDIS_") -> str | None: + """The ACL username for a URL, in BOTH spellings that ship. + + `{prefix}USER` is what the chart writes; `{prefix}USERNAME` is what + platform-service/sample.env writes and platform_service/env.py reads. One + shared Redis-credential secret naturally injects whichever name its author + picked, so a consumer that reads only one of them authenticates as a + different user than the client beside it in the same process — and on an + endpoint where the built-in `default` is disabled, only that one consumer + fails. + + Empty means absent, matching the rest of this module: `REDIS_USER=` is the + "leave it unset" spelling the sample recipes use, and `os.getenv(name, + fallback)` would return that blank instead of falling through to the second + spelling. + """ + return env_chain(f"{env_prefix}USER", f"{env_prefix}USERNAME") + + +def apply_url_credentials(url: str, password: str | None, username: str | None) -> str: + """Put credentials into a URL that carries none, leaving one that does alone. + + KombuManager reads TLS out of the query string only, so the URL is where TLS + has to go, and the credentials ride with it so ONE builder serves both the + kombu and the redis-py callers. That is a choice, not a constraint: + KombuManager forwards connection_options to kombu.Connection, which accepts + userid= and password=. + + A URL written without credentials otherwise produces an anonymous publisher + against an authenticated server, and Socket.IO events simply stop arriving. + + The password IS in the returned string. Smaller exposure than a values file, + but not none, and it now has TWO landing places in Django settings: + SOCKET_IO_MANAGER_URL, and CACHES["default"]["LOCATION"]. Neither name + matches SafeExceptionReporterFilter's `API|TOKEN|KEY|SECRET|PASS|SIGNATURE| + HTTP_COOKIE`, so neither is redacted on a technical-500 page, in + `manage.py diffsettings`, or in any settings dump — whereas + OPTIONS["PASSWORD"], where the cache password used to sit, IS masked. + The LOCATION is also the key of django-redis's process-global pool dict. + + This is a deliberate trade, not an oversight: django-redis 5.4.0 discards + OPTIONS["USERNAME"], so the LOCATION is the ONLY route by which an ACL + username can reach that cache. Stated here because the call site in + backend/settings/base.py is where someone will look for the risk and the + reasoning lives on this side of the boundary. One more reason DEBUG must + stay off. + """ + if not password: + return url + parts = urlsplit(url) + # Truthiness, not `is not None`: `redis://:@host` parses to password "", and + # both redis-py (`if url.password:`) and kombu (`unquote(password or "") or + # None`) read that as absent. + if parts.password: + return url + # REBUILD the netloc; do not prepend to it. parts.netloc INCLUDES any + # userinfo, so prepending on a username-only URL produced + # `alice:pw@alice@host` — and userinfo splits on the LAST "@", so the + # password parsed as "pw@alice". A wrong credential, not a missing one. + # + # A username already in the URL is reused VERBATIM rather than re-quoted: it + # is percent-encoded already, and quoting it again turned `al%40ice` into + # `al%2540ice`. + userinfo, _, host_port = parts.netloc.rpartition("@") + url_user = userinfo.partition(":")[0] + user = url_user or (quote(str(username), safe="") if username else "") + credentials = f"{user}:{quote(str(password), safe='')}@" + return urlunsplit( + ( + parts.scheme, + credentials + host_port, + parts.path, + parts.query, + parts.fragment, + ) + ) + + def build_socketio_redis_url(env_prefix: str = "REDIS_") -> str: """Redis URL for a Socket.IO/kombu client, TLS settings included (UN-4123). @@ -415,6 +589,9 @@ def build_socketio_redis_url(env_prefix: str = "REDIS_") -> str: ca_certs = env.get("ssl_ca_certs") url = env["url"] or _compose_redis_url(env) + # _compose_redis_url already embeds them for the discrete path; a configured + # URL may not carry any, and kombu can read them from nowhere else. + url = apply_url_credentials(url, env.get("url_password"), env.get("url_username")) if not url.startswith(_TLS_SCHEME): return url @@ -431,13 +608,45 @@ def _resolve_redis_env( env_prefix: str, default_port: str = "6379", db_override: int | None = None ) -> dict[str, Any]: """Read common Redis env vars into a dict.""" - host = os.getenv(f"{env_prefix}HOST", os.getenv("REDIS_HOST", "localhost")) - port = int(os.getenv(f"{env_prefix}PORT", os.getenv("REDIS_PORT", default_port))) - password = os.getenv(f"{env_prefix}PASSWORD", os.getenv("REDIS_PASSWORD")) - username = os.getenv( - f"{env_prefix}USER", - os.getenv(f"{env_prefix}USERNAME", os.getenv("REDIS_USER")), + # Stripped explicitly: env_chain returns the RAW value so a credential keeps + # any whitespace it was given, but a host must not. urlsplit() drops a + # trailing newline from a URL host while redis.Redis(host=...) keeps it, so + # a file-mounted REDIS_HOST ending in \n would fail DNS on the discrete path + # and succeed on the URL one. + host = (env_chain(f"{env_prefix}HOST", "REDIS_HOST") or "localhost").strip() + port_raw, port_name = env_chain_named(f"{env_prefix}PORT", "REDIS_PORT") + port = parse_port(port_raw, port_name, default_port) + # BLANK MEANS UNSET, like parse_db and _parse_bool in this same module — a + # nested os.getenv(prefixed, generic) takes the blank and never consults the + # generic one. workers/sample.env ships `CACHE_REDIS_PASSWORD=` uncommented, + # so an operator following this module's own recipe (REDIS_URL without + # credentials + REDIS_PASSWORD) got url_password="" for every prefixed + # client, apply_url_credentials no-opped, and the worker caches connected + # ANONYMOUSLY — the exact failure that helper exists to eliminate, + # reintroduced by an empty variable. + password = env_chain(f"{env_prefix}PASSWORD", "REDIS_PASSWORD") + username = env_chain( + f"{env_prefix}USER", f"{env_prefix}USERNAME", "REDIS_USER", "REDIS_USERNAME" ) + # An ACL username without a password is not a configuration Redis has. AUTH + # takes either one argument or two, so redis-py packs `AUTH alice None` and + # raises `DataError: Invalid input of type: 'NoneType'` on the FIRST command + # — a type error from inside the library, nowhere near the values file that + # caused it. Said plainly at configuration time instead, and the username is + # dropped so this client degrades to the same anonymous connection the + # Django cache already makes for this input (apply_url_credentials returns + # the URL untouched when there is no password), rather than the two + # disagreeing about a config neither can honour. + if username and not password: + logger.error( + "%sUSER=%r is set but no password is; Redis has no one-argument ACL " + "AUTH, so the username is being ignored. Set %sPASSWORD, or clear " + "the username.", + env_prefix, + username, + env_prefix, + ) + username = None prefixed_db = os.getenv(f"{env_prefix}DB", "").strip() generic_db = os.getenv("REDIS_DB", "").strip() db = ( @@ -449,10 +658,12 @@ def _resolve_redis_env( # turning TLS on platform-wide meant remembering CACHE_REDIS_SSL and # MANUAL_REVIEW_REDIS_SSL too — and a missed one fails as a plaintext client # talking to a TLS port, not as a config error. - ssl = ( - os.getenv(f"{env_prefix}SSL", os.getenv("REDIS_SSL", "false")).strip().lower() - == "true" - ) + # BLANK MEANS UNSET here too. It did not, and the asymmetry was dangerous + # rather than merely untidy: once password/user fell through a blank prefixed + # value but TLS did not, a blank CACHE_REDIS_SSL beside REDIS_SSL=true gave + # the worker cache a real AUTH over an UNENCRYPTED socket — the credential + # this same commit taught it to inherit, now on the wire in clear. + ssl = resolve_ssl_enabled(env_prefix) own_url = os.getenv(f"{env_prefix}URL", "").strip() generic_url = os.getenv("REDIS_URL", "").strip() result: dict[str, Any] = { @@ -499,6 +710,19 @@ def _resolve_redis_env( # docstring, sample.env or the chart state. For env_prefix="REDIS_" the two # levels are the same variable, so this reads identically there. explicit_db = prefixed_db if own_url else (prefixed_db or generic_db) + # CREDENTIALS AT THE URL'S OWN LEVEL, by the same rule as the database above. + # A prefix that brought its OWN url may point at a DIFFERENT endpoint, and + # an anonymous one at that; letting the generic REDIS_PASSWORD reach across + # would make that client send AUTH to a server with none, breaking a + # connection that worked before. An INHERITED url is the same endpoint as + # the generic one, so the full fallback chain applies. For env_prefix + # "REDIS_" the two levels are the same variable and this reads identically. + if own_url: + result["url_password"] = env_chain(f"{env_prefix}PASSWORD") + result["url_username"] = url_username_from_env(env_prefix) + else: + result["url_password"] = result["password"] + result["url_username"] = result["username"] if result["url"] and explicit_db and db_override is None: result["db_from_prefix_env"] = parse_db(explicit_db, env_prefix) # Read OUTSIDE the `if ssl` below: URL mode carries TLS in the scheme and never @@ -508,8 +732,8 @@ def _resolve_redis_env( # # Needed where the server's CA is not publicly trusted — notably Memorystore, # whose CA is Google-managed. ElastiCache and Azure chain to public CAs. - ca_certs = os.getenv( - f"{env_prefix}SSL_CA_CERTS", os.getenv("REDIS_SSL_CA_CERTS", "") + ca_certs = ( + env_chain(f"{env_prefix}SSL_CA_CERTS", "REDIS_SSL_CA_CERTS") or "" ).strip() if ca_certs: result["ssl_ca_certs"] = ca_certs @@ -536,8 +760,14 @@ def _resolve_redis_env( # check_hostname is True while verify_mode is CERT_NONE. Decided from the # EFFECTIVE value — the URL's query string outranks the env var, and testing # the env var alone is what produced that ValueError on every connection. - raw_check_hostname = os.getenv( - f"{env_prefix}SSL_CHECK_HOSTNAME", os.getenv("REDIS_SSL_CHECK_HOSTNAME", "") + # The SAME read as resolve_ssl_check_hostname above — this one decides + # whether the value was EXPLICIT, and leaving it on the nested os.getenv + # made a blank prefixed value fall through for the value but not for the + # explicitness. On a Sentinel + TLS deployment that meant a global + # REDIS_SSL_CHECK_HOSTNAME=true was honoured on the discovery connections + # and silently dropped on the master one. + raw_check_hostname, _ = env_chain_named( + f"{env_prefix}SSL_CHECK_HOSTNAME", "REDIS_SSL_CHECK_HOSTNAME" ) # Whether the operator ASKED for a value, as opposed to inheriting the # default. Sentinel's master plane needs to know the difference — see @@ -546,8 +776,8 @@ def _resolve_redis_env( # verification back on for Sentinel masters, which is the breakage this # distinction exists to avoid. result["ssl_check_hostname_explicit"] = ( - raw_check_hostname.strip().lower() in _TRUE_LITERALS | _FALSE_LITERALS - ) + raw_check_hostname or "" + ).strip().lower() in _TRUE_LITERALS | _FALSE_LITERALS if effective_cert_reqs(result["url"], result["ssl_cert_reqs"]) == "none": result["ssl_check_hostname"] = False result["ssl_check_hostname_explicit"] = False @@ -674,6 +904,8 @@ def _create_standalone_client( ssl_ca_certs=env.get("ssl_ca_certs"), ssl_cert_reqs=env.get("ssl_cert_reqs"), ssl_check_hostname=env.get("ssl_check_hostname"), + password=env.get("url_password"), + username=env.get("url_username"), ) logger.info( @@ -715,6 +947,8 @@ def _create_client_from_url( ssl_ca_certs: str | None, ssl_cert_reqs: str | None = None, ssl_check_hostname: bool | None = None, + password: str | None = None, + username: str | None = None, ) -> redis.Redis: """Build a client from a full Redis URL. @@ -760,6 +994,34 @@ def _create_client_from_url( kwargs["db"] = db_override parts = urlsplit(url) + # Credentials FILL A GAP the URL leaves; they never override one. + # ConnectionPool.from_url ends with kwargs.update(url_options), so a URL + # carrying credentials still wins. + # + # Without this, a URL written WITHOUT credentials plus a separately + # configured {prefix}PASSWORD connects ANONYMOUSLY — and setting the two + # apart is the configuration the on-prem recipe should be able to recommend, + # because it keeps the password out of a URL that gets printed into error + # messages, ArgoCD conditions and ExternalSecret templates. The endpoint + # answers NOAUTH on the first command, which reads as a broken server rather + # than a dropped password. + # Gated on the PASSWORD, and the username rides with it. A username alone is + # not a credential: values.yaml ships REDIS_USER: default, so filling it in + # on its own would make redis-py send AUTH to the in-cluster server, which + # has none — turning a working default deployment into a failing one. + # Keyed on the URL's PASSWORD, not on the presence of "@". A URL may carry an + # ACL username alone — redis://alice@host — and that @ is not evidence of a + # password. Truthiness rather than `is None`: `redis://:@host` parses to "" + # and redis-py's own parse_url sets the key only `if url.password`. + # + # No `not parts.username` guard below: from_url ends with + # kwargs.update(url_options), so a URL username wins regardless — that guard + # was unfalsifiable, which is where a later reader deletes something + # load-bearing by mistake. + if password and not parts.password: + kwargs["password"] = password + if username: + kwargs["username"] = username logger.info( "Redis URL mode enabled. Connecting to %s:%s (tls=%s)", parts.hostname, diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 63a0d191f4..7411369dfc 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -11,12 +11,18 @@ None of those raise at import, so they are asserted here instead. """ +import logging import pathlib import re +from urllib.parse import unquote, urlsplit import pytest import redis from unstract.core.cache.redis_client import ( + url_username_from_env, + parse_port, + env_chain_named, + env_chain, _build_connection_kwargs, _resolve_redis_env, build_socketio_redis_url, @@ -263,19 +269,39 @@ def test_prefixed_client_inherits_the_global_password(self, monkeypatch): "s3cr3t" ) - def test_empty_prefixed_password_shadows_the_fallback(self, monkeypatch): - """Documents a trap rather than endorsing it. + def test_empty_prefixed_password_falls_through_to_the_fallback(self, monkeypatch): + """This is the revisit the previous version of this test asked for. - ``os.getenv(key, fallback)`` returns "" when the key exists but is empty, - so an empty CACHE_REDIS_PASSWORD suppresses REDIS_PASSWORD and the client - connects UNAUTHENTICATED. The Helm chart must therefore never render an - empty credential; this test fails if that behaviour ever changes, so the - chart-side guarantee can be revisited. + It used to assert the opposite and said so: an empty + CACHE_REDIS_PASSWORD suppressed REDIS_PASSWORD and the client connected + UNAUTHENTICATED, which was tolerated because "the Helm chart must + therefore never render an empty credential". + + That guarantee does not hold. workers/sample.env ships + `CACHE_REDIS_PASSWORD=` uncommented, so an operator following this + module's own managed-Redis recipe — REDIS_URL without credentials plus + REDIS_PASSWORD — got no password on any prefixed client and a NOAUTH on + first command. Blank now means unset here, the same rule parse_db and + _parse_bool already document two functions away. """ monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") monkeypatch.setenv("CACHE_REDIS_PASSWORD", "") kwargs = _kwargs(create_redis_client(env_prefix="CACHE_REDIS_")) - assert kwargs.get("password") is None + assert kwargs["password"] == "s3cr3t" + + def test_a_prefixed_password_still_overrides(self, monkeypatch): + """Blank falling through must not turn into the prefix being ignored.""" + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "other") + kwargs = _kwargs(create_redis_client(env_prefix="CACHE_REDIS_")) + assert kwargs["password"] == "other" + + def test_a_blank_password_on_a_prefixed_url_reaches_the_url(self, monkeypatch): + """The recipe this PR documents, with the sample.env blank in place.""" + monkeypatch.setenv("REDIS_URL", "rediss://managed:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "") + assert urlsplit(build_socketio_redis_url("CACHE_REDIS_")).password == "pw" class TestSocketIoUrl: @@ -552,11 +578,25 @@ class TestContainerAllowlists: def _shared(cls) -> set[str]: """Every REDIS_* env var redis_client.py reads, minus the excused ones.""" source = cls._CLIENT.read_text() - names = set(re.findall(r'os\.getenv\(\s*"(REDIS_[A-Z_]+)"', source)) - names |= { - f"REDIS_{suffix}" - for suffix in re.findall(r'os\.getenv\(\s*f"\{env_prefix\}([A-Z_]+)"', source) - } + # Scans the ARGUMENTS of every env-reading call, not just os.getenv. + # Keying on os.getenv alone meant that rewriting a read to go through + # env_chain made the variable invisible here — the TLS set dropped out + # of this guard silently, which is the same drift this class exists to + # catch. Scoped to these call names rather than the whole file so a + # variable merely NAMED in a docstring is not demanded of every + # container. + calls = re.findall( + r"(?:os\.getenv|env_chain|env_chain_named)\(" + r"([^()]*(?:\([^()]*\)[^()]*)*)\)", + source, + ) + names: set[str] = set() + for args in calls: + names |= set(re.findall(r'"(REDIS_[A-Z_]+)"', args)) + names |= { + f"REDIS_{suffix}" + for suffix in re.findall(r'f"\{env_prefix\}([A-Z_]+)"', args) + } return names - cls._NOT_FORWARDED def test_the_derived_set_is_not_empty(self): @@ -909,3 +949,352 @@ def test_the_prefixs_own_db_still_wins(self, monkeypatch): monkeypatch.setenv("REDIS_DB", "3") monkeypatch.setenv("CACHE_REDIS_DB", "1") assert _kwargs(create_redis_client("CACHE_REDIS_"))["db"] == 1 + + +class TestUrlModeCredentials: + """A URL written without credentials must not connect anonymously. + + Keeping the password OUT of the URL is the safer configuration — a URL is + printed into error messages, ArgoCD conditions and ExternalSecret templates, + and a password in it travels to all three. That configuration is only usable + if the separately-supplied credential is actually applied. + """ + + def test_the_configured_password_fills_a_gap_the_url_leaves(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client())["password"] == "s3cret" + + def test_a_url_carrying_credentials_still_wins(self, monkeypatch): + """ConnectionPool.from_url ends with kwargs.update(url_options).""" + monkeypatch.setenv("REDIS_URL", "rediss://:in-url@h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client())["password"] == "in-url" + + def test_a_username_password_pair_in_the_url_wins_too(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://u:pw@h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + monkeypatch.setenv("REDIS_USER", "alice") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "pw" + assert kwargs["username"] == "u" + + def test_the_username_rides_with_the_password(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + monkeypatch.setenv("REDIS_USER", "alice") + assert _kwargs(create_redis_client())["username"] == "alice" + + def test_a_username_alone_is_not_a_credential(self, monkeypatch): + """values.yaml ships REDIS_USER: default, and the in-cluster server has + no auth. Filling in a username on its own would make redis-py send AUTH + to it — turning a working default deployment into a failing one. + """ + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_USER", "default") + monkeypatch.setenv("REDIS_PASSWORD", "") + kwargs = _kwargs(create_redis_client()) + assert kwargs.get("username") is None + assert kwargs.get("password") is None + + def test_no_credentials_anywhere_stays_anonymous(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + assert _kwargs(create_redis_client()).get("password") is None + + def test_the_socketio_url_gains_the_credentials_too(self, monkeypatch): + """Kombu takes a URL and nothing else, so a separately-supplied password + cannot reach it any other way. Without this the publisher is anonymous + against an authenticated server and Socket.IO events simply stop. + """ + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + # Assert on PARSED fields, not a substring: a substring check passes on + # `rediss://alice:s3cret@alice@h:6380/0`, where the password actually + # parses as "s3cret@alice". + parts = urlsplit(build_socketio_redis_url()) + assert parts.password == "s3cret" + assert parts.hostname == "h" + + def test_the_socketio_url_leaves_existing_credentials_alone(self, monkeypatch): + """Parsed fields, not substrings — the same rule as the test above. + + `"in-url" in url` also passes on `rediss://:in-url@in-url@h:6380/0`, + where the password parses as "in-url@in-url", and on + `rediss://h:6380/0?note=in-url`, where there is no password at all. + """ + monkeypatch.setenv("REDIS_URL", "rediss://:in-url@h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + parts = urlsplit(build_socketio_redis_url()) + assert parts.password == "in-url" + assert parts.hostname == "h" + + def test_an_empty_password_in_the_url_is_treated_as_absent(self, monkeypatch): + """`redis://:@host` parses to password "", which redis-py's own + parse_url reads as absent (`if url.password:`). Testing `is None` would + decline to fill a gap the library agrees is a gap — and the shape is + what Helm produces from `redis://:{{ .Values.password }}@host` when the + value is empty. + """ + monkeypatch.setenv("REDIS_URL", "redis://:@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client())["password"] == "s3cret" + + def test_a_username_only_url_still_gets_the_password(self, monkeypatch): + """`redis://alice@host` carries an @ but NO password. + + Treating the @ as evidence of credentials dropped the separately + supplied password and left the client unable to authenticate. + """ + monkeypatch.setenv("REDIS_URL", "redis://alice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "s3cret" + assert kwargs["username"] == "alice" + + +class TestUrlCredentialsAreResolvedAtTheUrlsLevel: + """Same rule as the database: a prefix that brought its OWN url owns its + own credentials. + + That url may point at a DIFFERENT endpoint, and an anonymous one; letting + the generic REDIS_PASSWORD reach across would make the client send AUTH to + a server that has none, breaking a connection that worked before. + """ + + def test_a_prefix_url_does_not_inherit_the_generic_password(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_URL", "redis://anon:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client("CACHE_REDIS_")).get("password") is None + + def test_the_prefixs_own_password_applies(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_URL", "redis://anon:6379/0") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "own") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client("CACHE_REDIS_"))["password"] == "own" + + def test_an_inherited_url_still_uses_the_generic_password(self, monkeypatch): + """Same endpoint as the generic one, so the fallback chain applies.""" + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert _kwargs(create_redis_client("CACHE_REDIS_"))["password"] == "s3cret" + + def test_the_socketio_url_follows_the_same_rule(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_URL", "rediss://anon:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + # A bare `"s3cret" not in url` also passes on "" and on any mangled + # output, so the endpoint is pinned too. + parts = urlsplit(build_socketio_redis_url("CACHE_REDIS_")) + assert parts.password is None + assert parts.hostname == "anon" + + +class TestSocketIoUrlCredentials: + """The Socket.IO builder must agree with create_redis_client on every shape. + + Its output is a STRING handed to kombu, so a malformed one authenticates + with a wrong secret rather than none — and `socketio.Server` is constructed + with logger=False, so the symptom is events silently stopping. + """ + + def test_a_username_only_url_keeps_its_host_and_gets_the_password(self, monkeypatch): + """`parts.netloc` INCLUDES userinfo; prepending to it produced + `alice:pw@alice@host`, and userinfo splits on the LAST @. + """ + monkeypatch.setenv("REDIS_URL", "redis://alice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + parts = urlsplit(build_socketio_redis_url()) + assert parts.hostname == "h" + assert parts.username == "alice" + assert parts.password == "s3cret" + + def test_an_encoded_username_is_not_double_encoded(self, monkeypatch): + """It is percent-encoded already; quoting it again gave `al%2540ice`.""" + monkeypatch.setenv("REDIS_URL", "redis://al%40ice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + # Asserted POSITIVELY: `"%2540" not in url` is equally satisfied by a + # URL that dropped the username, or replaced it with REDIS_USER. + assert urlsplit(build_socketio_redis_url()).username == "al%40ice" + + def test_a_password_with_url_metacharacters_is_encoded(self, monkeypatch): + """Unencoded, `p@ss/w:rd` reparses with host "ss" — a DIFFERENT server.""" + monkeypatch.setenv("REDIS_URL", "rediss://h:6380/0") + monkeypatch.setenv("REDIS_PASSWORD", "p@ss/w:rd") + url = build_socketio_redis_url() + assert urlsplit(url).hostname == "h" + # Round-trip, not a spelling check. The hostname guard catches the + # host half of a doubled userinfo but not the credential half: + # `rediss://x:p%40ss%2Fw%3Ard@p%40ss%2Fw%3Ard@h:6380/0` has hostname + # "h" and contains the substring, yet kombu would send + # "p@ss/w:rd@p@ss/w:rd". unquote() is what kombu itself applies. + assert unquote(urlsplit(url).password) == "p@ss/w:rd" + + def test_an_empty_password_in_the_url_is_treated_as_absent(self, monkeypatch): + """`redis://:@host` parses to "", which redis-py and kombu both read as + no password — so the separately supplied one must fill the gap. + """ + monkeypatch.setenv("REDIS_URL", "redis://:@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert urlsplit(build_socketio_redis_url()).password == "s3cret" + + def test_a_username_alone_stays_anonymous(self, monkeypatch): + """values.yaml ships REDIS_USER: default against an unauthenticated + in-cluster server; adding credentials here would break it. + """ + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_USER", "default") + monkeypatch.setenv("REDIS_PASSWORD", "") + assert build_socketio_redis_url() == "redis://h:6379/0" + + def test_the_prefix_username_spelling_is_honoured(self, monkeypatch): + """{prefix}USERNAME is the compatibility spelling of {prefix}USER.""" + monkeypatch.setenv("CACHE_REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "pw") + monkeypatch.setenv("CACHE_REDIS_USERNAME", "alice") + assert urlsplit(build_socketio_redis_url("CACHE_REDIS_")).username == "alice" + + +class TestTheConsumersAgreeOnTheUsername: + """The client, the Socket.IO URL and the Django cache must resolve the SAME + ACL user. Each was previously pinned only to itself, so a change that made + them disagree passed every test. + """ + + def test_a_url_username_outranks_the_configured_one(self, monkeypatch): + """Both set at once is the only environment that tells the two apart. + + values.yaml ships REDIS_USER: default, so pairing it with a URL that + names a different user is an ordinary config — and reversing the + precedence made the client authenticate as `alice` while kombu and the + Django cache authenticated as `default`. On an endpoint where `default` + is disabled, Socket.IO events stop and nothing else does. + """ + monkeypatch.setenv("REDIS_URL", "redis://alice@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + monkeypatch.setenv("REDIS_USER", "default") + assert urlsplit(build_socketio_redis_url()).username == "alice" + assert _kwargs(create_redis_client())["username"] == "alice" + + def test_a_password_only_url_takes_no_username_from_the_env(self, monkeypatch): + """The `not parts.password` guard is load-bearing for the USERNAME. + + For the password it is not: from_url ends with kwargs.update(url_options) + so the URL's password wins either way. Dropping the guard therefore + looks harmless and is not — it lets REDIS_USER ride along, turning a + one-argument `AUTH ` into a two-argument `AUTH alice ` while the + Socket.IO URL for the same config still sends the one-argument form. + """ + monkeypatch.setenv("REDIS_URL", "redis://:urlpw@h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "envpw") + monkeypatch.setenv("REDIS_USER", "alice") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "urlpw" + assert kwargs.get("username") is None + assert urlsplit(build_socketio_redis_url()).username in (None, "") + + def test_the_username_env_var_has_two_spellings(self, monkeypatch): + """REDIS_USER is what the chart writes; REDIS_USERNAME is what + platform-service/sample.env writes. One shared credential secret can + inject either, so a consumer that reads only one authenticates as a + different user than the client beside it. + """ + monkeypatch.delenv("REDIS_USER", raising=False) + monkeypatch.setenv("REDIS_USERNAME", "alice") + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + assert urlsplit(build_socketio_redis_url()).username == "alice" + assert _kwargs(create_redis_client())["username"] == "alice" + + def test_a_blank_user_falls_through_to_the_other_spelling(self, monkeypatch): + """`REDIS_USER=` is the "leave it unset" spelling the recipes use, and + `os.getenv(name, fallback)` would hand back the blank instead.""" + monkeypatch.setenv("REDIS_USER", "") + monkeypatch.setenv("REDIS_USERNAME", "alice") + monkeypatch.setenv("REDIS_URL", "redis://h:6379/0") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + assert urlsplit(build_socketio_redis_url()).username == "alice" + + +class TestBlankMeansUnsetForTlsToo: + """The convention has to cover TLS, not just credentials. + + Once a blank prefixed PASSWORD fell through to the generic one but a blank + prefixed SSL did not, `CACHE_REDIS_SSL=` beside `REDIS_SSL=true` gave the + worker cache a real AUTH over an UNENCRYPTED socket — the credential the + same change taught it to inherit, now in clear on the wire. + """ + + @pytest.mark.parametrize( + "blank,generic,key,expected", + [ + ("CACHE_REDIS_SSL", ("REDIS_SSL", "true"), "ssl", True), + ( + "CACHE_REDIS_SSL_CERT_REQS", + ("REDIS_SSL_CERT_REQS", "none"), + "ssl_cert_reqs", + "none", + ), + ( + "CACHE_REDIS_SSL_CHECK_HOSTNAME", + ("REDIS_SSL_CHECK_HOSTNAME", "false"), + "ssl_check_hostname", + False, + ), + ( + "CACHE_REDIS_SSL_CA_CERTS", + ("REDIS_SSL_CA_CERTS", "/etc/ca.pem"), + "ssl_ca_certs", + "/etc/ca.pem", + ), + ], + ) + def test_a_blank_prefixed_tls_var_does_not_shadow( + self, monkeypatch, blank, generic, key, expected + ): + monkeypatch.setenv(generic[0], generic[1]) + monkeypatch.setenv(blank, "") + assert _resolve_redis_env("CACHE_REDIS_")[key] == expected + + +class TestEnvChainHelpers: + """The helpers the consumers share. Each had a one-line failure mode.""" + + def test_env_chain_returns_the_raw_value_not_a_stripped_one(self, monkeypatch): + """A file-mounted secret ends in a newline. Stripping the RETURN value + truncated it here while backend/settings/base.py read the same variable + unstripped, so one process authenticated two different ways.""" + monkeypatch.setenv("REDIS_PASSWORD", "s3cret\n") + assert env_chain("REDIS_PASSWORD") == "s3cret\n" + + def test_env_chain_skips_whitespace_only_values(self, monkeypatch): + monkeypatch.setenv("CACHE_REDIS_PASSWORD", " ") + monkeypatch.setenv("REDIS_PASSWORD", "s3cret") + assert env_chain("CACHE_REDIS_PASSWORD", "REDIS_PASSWORD") == "s3cret" + + def test_env_chain_named_reports_the_variable_that_was_set(self, monkeypatch): + """So a warning cannot send an operator grepping for an unset key.""" + monkeypatch.delenv("CACHE_REDIS_PORT", raising=False) + monkeypatch.setenv("REDIS_PORT", "6380") + assert env_chain_named("CACHE_REDIS_PORT", "REDIS_PORT") == ("6380", "REDIS_PORT") + + def test_url_username_from_env_honours_the_same_blank_rule(self, monkeypatch): + monkeypatch.setenv("REDIS_USER", " ") + monkeypatch.setenv("REDIS_USERNAME", "alice") + assert url_username_from_env() == "alice" + + @pytest.mark.parametrize("raw", [None, "", " "]) + def test_parse_port_treats_blank_as_unset(self, raw, caplog): + """Quietly, too. base.py hands this the raw env value rather than + env_chain's None, so treating "" as unparseable logged an ERROR at + startup in the backend and nowhere else — for the repo's own + "leave the default" spelling.""" + with caplog.at_level(logging.ERROR): + assert parse_port(raw, "REDIS_PORT", 6379) == 6379 + assert caplog.text == "" + + def test_parse_port_falls_back_on_an_unparseable_value(self, caplog): + with caplog.at_level(logging.ERROR): + assert parse_port("638O", "REDIS_PORT", 6379) == 6379 + assert "REDIS_PORT" in caplog.text + + def test_parse_port_returns_an_int(self): + assert parse_port("6380", "REDIS_PORT", 6379) == 6380 diff --git a/workers/sample.env b/workers/sample.env index 375a62d2a8..b6c1078571 100644 --- a/workers/sample.env +++ b/workers/sample.env @@ -81,7 +81,13 @@ REDIS_SENTINEL_MASTER_NAME=mymaster # Managed / external Redis with TLS (UN-4123). Optional — unset keeps the plaintext # connection above. Either set REDIS_SSL=true beside REDIS_HOST/REDIS_PORT, or give a # full URL whose scheme carries TLS: -# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# REDIS_URL=rediss://:6380/0?ssl_cert_reqs=required +# REDIS_PASSWORD= +# REDIS_USER= # blank — see below +# (The password is better kept OUT of the URL. A URL is printed into log +# lines, error messages and deployment tooling; a password in one travels +# with it. Credentials in the URL still work and still win, so an existing +# rediss://:@host URL keeps behaving exactly as before.) # A URL wins over the discrete vars. Auth is password-only as the built-in `default` # user (what a managed AUTH string is), so leave the username empty for a managed # endpoint. REDIS_SSL_CA_CERTS is only needed when the server's CA is not publicly