diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 1a923bc9fc..7466774426 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -104,6 +104,19 @@ def get_required_setting(setting_key: str, default: str | None = None) -> str | REDIS_HOST = os.environ.get("REDIS_HOST", "localhost") REDIS_PORT = os.environ.get("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" +REDIS_SSL_CERT_REQS = os.environ.get("REDIS_SSL_CERT_REQS", "required") +REDIS_SSL_CA_CERTS = os.environ.get("REDIS_SSL_CA_CERTS", "").strip() +# A full URL overrides the discrete vars above, exactly as it does in +# unstract.core's create_redis_client — otherwise the workers would follow the URL +# while this process stayed on REDIS_HOST, and the two would silently sit on +# DIFFERENT servers. That split is invisible until something written by one side +# is read by the other: an API deployment returns `result: null` because the +# execution's cached result was written to the URL's Redis and looked up here. +REDIS_URL = os.environ.get("REDIS_URL", "").strip() SESSION_EXPIRATION_TIME_IN_SECOND = os.environ.get( "SESSION_EXPIRATION_TIME_IN_SECOND", 3600 ) @@ -565,20 +578,65 @@ def filter(self, record): _cred_prefix = f"{quote(REDIS_USER, safe='')}:{quote(REDIS_PASSWORD, safe='')}@" elif REDIS_PASSWORD: _cred_prefix = f":{quote(REDIS_PASSWORD, safe='')}@" - SOCKET_IO_MANAGER_URL = f"redis://{_cred_prefix}{REDIS_HOST}:{REDIS_PORT}" + _scheme = "rediss" if REDIS_SSL else "redis" + _cache_db = int(REDIS_DB) if REDIS_DB else 0 + + # Both django-redis (via redis-py) and kombu read TLS settings out of the URL's + # query string, so one string configures both — verified against the pinned + # versions. The CA is appended rather than required in the URL, since it is a + # local path rather than part of the endpoint's identity. + _redis_url = REDIS_URL + if ( + _redis_url + and REDIS_SSL_CA_CERTS + and _redis_url.startswith("rediss://") + and "ssl_ca_certs=" not in _redis_url + ): + _sep = "&" if "?" in _redis_url else "?" + _redis_url = ( + f"{_redis_url}{_sep}ssl_ca_certs={quote(REDIS_SSL_CA_CERTS, safe='/')}" + ) + + # kombu reads TLS off the scheme, but defaults ssl_cert_reqs to CERT_NONE — + # encrypted while accepting ANY certificate, which is not what "TLS" is meant + # to buy. The query parameter is the only way to say otherwise here, since + # KombuManager takes a URL rather than connection kwargs. + _socketio_tls_query = f"?ssl_cert_reqs={REDIS_SSL_CERT_REQS}" if REDIS_SSL else "" + SOCKET_IO_MANAGER_URL = _redis_url or ( + f"{_scheme}://{_cred_prefix}{REDIS_HOST}:{REDIS_PORT}{_socketio_tls_query}" + ) SOCKET_IO_TRANSPORT_OPTIONS = {} + # django-redis 5.4.0 reads only PASSWORD (plus timeouts) out of OPTIONS — its + # ConnectionFactory.make_connection_params ignores USERNAME and DB entirely. + # 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 is deliberately NOT restored: auth is password-only as the built-in + # `default` user (what managed AUTH strings are), and sending a username turns + # AUTH into its two-argument ACL form. + _cache_options = { + "CLIENT_CLASS": "django_redis.client.DefaultClient", + "SERIALIZER": "django_redis.serializers.json.JSONSerializer", + } + 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 + if REDIS_SSL: + _pool_kwargs = {"ssl_cert_reqs": REDIS_SSL_CERT_REQS} + if REDIS_SSL_CA_CERTS: + _pool_kwargs["ssl_ca_certs"] = REDIS_SSL_CA_CERTS + _cache_options["CONNECTION_POOL_KWARGS"] = _pool_kwargs + CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", - "LOCATION": f"redis://{REDIS_HOST}:{REDIS_PORT}", - "OPTIONS": { - "CLIENT_CLASS": "django_redis.client.DefaultClient", - "SERIALIZER": "django_redis.serializers.json.JSONSerializer", - "DB": int(REDIS_DB) if REDIS_DB else 0, - "USERNAME": REDIS_USER, - "PASSWORD": REDIS_PASSWORD, - }, + "LOCATION": _redis_url + or f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}", + "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 new file mode 100644 index 0000000000..7392f1af6d --- /dev/null +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -0,0 +1,135 @@ +"""How the Redis settings block derives its URLs (UN-4123). + +The block lives inside ``settings/base.py`` and runs at import, so it cannot be +called directly; these tests execute that exact source range against a controlled +environment instead of re-implementing it, which would test a copy rather than +the thing that ships. + +The case that matters is REDIS_URL. `create_redis_client` honoured it from the +start, while this module still built its own URL from REDIS_HOST/REDIS_PORT — so +the workers moved to the configured endpoint and the backend silently stayed +behind. Nothing errors: the API deployment simply returns ``result: null``, +because the execution's cached result was written to one Redis and looked up in +the other. Found on a live run, not by reading the code. +""" + +from __future__ import annotations + +import pathlib + +import pytest + +_SETTINGS = pathlib.Path(__file__).resolve().parents[1] / "settings" / "base.py" + + +def _derive(**env: str) -> dict: + """Execute the standalone Redis block with the given env.""" + source = _SETTINGS.read_text() + start = source.index("REDIS_SENTINEL_MODE = (") + end = source.index("SESSION_ENGINE =") + + ns: dict = {} + prelude = ( + "import os\n" + "from urllib.parse import quote\n" + "REDIS_USER = os.environ.get('REDIS_USER', 'default')\n" + "REDIS_PASSWORD = os.environ.get('REDIS_PASSWORD', '')\n" + "REDIS_HOST = os.environ.get('REDIS_HOST', 'localhost')\n" + "REDIS_PORT = os.environ.get('REDIS_PORT', '6379')\n" + "REDIS_DB = os.environ.get('REDIS_DB', '')\n" + "REDIS_SSL = os.environ.get('REDIS_SSL', 'false').strip().lower() == 'true'\n" + "REDIS_SSL_CERT_REQS = os.environ.get('REDIS_SSL_CERT_REQS', 'required')\n" + "REDIS_SSL_CA_CERTS = os.environ.get('REDIS_SSL_CA_CERTS', '').strip()\n" + "REDIS_URL = os.environ.get('REDIS_URL', '').strip()\n" + ) + import os as _os + + 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: + exec(prelude + source[start:end], ns) # noqa: S102 - the code under test + 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 ns + + +@pytest.fixture +def plain() -> dict: + return _derive(REDIS_HOST="unstract-redis", REDIS_PORT="6379") + + +class TestDiscreteVars: + def test_plaintext_is_unchanged(self, plain): + assert plain["CACHES"]["default"]["LOCATION"] == "redis://unstract-redis:6379/0" + assert plain["SOCKET_IO_MANAGER_URL"] == "redis://unstract-redis:6379" + assert "CONNECTION_POOL_KWARGS" not in plain["CACHES"]["default"]["OPTIONS"] + + def test_db_travels_in_the_location(self): + """django-redis 5.4.0 ignores OPTIONS['DB'], so the path is the only route. + + Without it this cache sits on db 0 while every other service honours + REDIS_DB — workers RPUSH log_history_queue to db N and the backend LPOPs + an empty db 0. + """ + derived = _derive(REDIS_HOST="h", REDIS_DB="3") + assert derived["CACHES"]["default"]["LOCATION"].endswith("/3") + + def test_ssl_switches_scheme_and_pool_kwargs(self): + derived = _derive(REDIS_HOST="h", REDIS_SSL="true", REDIS_SSL_CA_CERTS="/ca.pem") + cache = derived["CACHES"]["default"] + assert cache["LOCATION"].startswith("rediss://") + assert cache["OPTIONS"]["CONNECTION_POOL_KWARGS"] == { + "ssl_cert_reqs": "required", + "ssl_ca_certs": "/ca.pem", + } + + def test_socketio_pins_certificate_verification(self): + """kombu defaults rediss:// to CERT_NONE — encrypted, but unauthenticated.""" + derived = _derive(REDIS_HOST="h", REDIS_SSL="true") + assert "ssl_cert_reqs=required" in derived["SOCKET_IO_MANAGER_URL"] + + +class TestUrlMode: + def test_url_drives_both_cache_and_socketio(self): + """The regression: these two used to ignore REDIS_URL entirely.""" + url = "rediss://:pw@managed.example:6380/0?ssl_cert_reqs=required" + derived = _derive(REDIS_HOST="in-cluster", REDIS_URL=url) + assert derived["CACHES"]["default"]["LOCATION"].startswith(url) + assert derived["SOCKET_IO_MANAGER_URL"].startswith(url) + assert "in-cluster" not in derived["CACHES"]["default"]["LOCATION"] + + def test_credentials_are_not_passed_twice(self): + """In URL mode the URL is the single source for db and credentials.""" + derived = _derive(REDIS_URL="rediss://:pw@h:6380/2", REDIS_PASSWORD="other") + options = derived["CACHES"]["default"]["OPTIONS"] + assert "PASSWORD" not in options + assert "DB" not in options + + def test_ca_is_appended_for_tls_urls(self): + derived = _derive( + REDIS_URL="rediss://h:6380/0?ssl_cert_reqs=required", + REDIS_SSL_CA_CERTS="/etc/ssl/redis-ca.pem", + ) + assert "ssl_ca_certs=/etc/ssl/redis-ca.pem" in ( + derived["CACHES"]["default"]["LOCATION"] + ) + + def test_ca_is_not_appended_to_a_plaintext_url(self): + derived = _derive( + REDIS_URL="redis://h:6379/0", REDIS_SSL_CA_CERTS="/etc/ssl/redis-ca.pem" + ) + assert "ssl_ca_certs" not in derived["CACHES"]["default"]["LOCATION"] + + def test_an_explicit_ca_in_the_url_wins(self): + derived = _derive( + REDIS_URL="rediss://h:6380/0?ssl_ca_certs=/in/url.pem", + REDIS_SSL_CA_CERTS="/env/ca.pem", + ) + location = derived["CACHES"]["default"]["LOCATION"] + assert location.count("ssl_ca_certs") == 1 + assert "/in/url.pem" in location diff --git a/backend/sample.env b/backend/sample.env index 700422d1a4..f63235062e 100644 --- a/backend/sample.env +++ b/backend/sample.env @@ -53,6 +53,33 @@ REDIS_RETRY_BACKOFF_FACTOR=0.5 REDIS_SENTINEL_MODE=False REDIS_SENTINEL_MASTER_NAME=mymaster +# Managed / external Redis with TLS (UN-4123). All optional — unset means the +# plaintext connection this file otherwise describes, unchanged. +# +# Two ways to configure it, pick one: +# 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 +# A URL wins over the discrete vars above. Percent-encode @ / + in the password. +# +# Auth is password-only, as Redis's built-in `default` user — that is what a managed +# AUTH string is. Leave REDIS_USER empty for a managed endpoint; named ACL users are +# not supported platform-wide. +# +# REDIS_SSL_CA_CERTS is only needed when the server's CA is not in the system trust +# store (Memorystore). ElastiCache and Azure Cache chain to public CAs. +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= + +# Proactive health check for pooled connections, in seconds (0 disables). +# Managed Redis fails over during maintenance and reaps idle connections — Azure +# Cache closes them at 10 minutes — so a parked connection can be dead before its +# next command. Defaults to 30. +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Connector OAuth SOCIAL_AUTH_EXTRA_DATA_EXPIRATION_TIME_IN_SECOND=3600 GOOGLE_OAUTH2_KEY= diff --git a/docker/docker-compose-redis-tls.yaml b/docker/docker-compose-redis-tls.yaml new file mode 100644 index 0000000000..1c2604508d --- /dev/null +++ b/docker/docker-compose-redis-tls.yaml @@ -0,0 +1,69 @@ +# A stand-in for a managed Redis, for local development only (UN-4123). +# +# Managed offerings differ from the plain dev Redis in exactly two ways that our +# code has to cope with: they require AUTH, and they speak TLS. This container +# does both, so the TLS path can be exercised end to end — execution, log +# streaming, tool sidecars — without cloud access or a VPN. +# +# It runs ALONGSIDE the normal `unstract-redis`, on a different port, so nothing +# switches until you point services at it. That is deliberate: the point of the +# exercise is to flip between the two and confirm BOTH work. +# +# ./redis-tls/generate-certs.sh +# docker compose -f docker-compose-redis-tls.yaml up -d +# +# See redis-tls/README.md for the env to set and what to verify. +services: + redis-managed: + image: "redis:7.2.3" + container_name: unstract-redis-managed + restart: unless-stopped + # `--port 0` disables the plaintext listener entirely, which is what Azure + # Cache does by default. A service that fails to pick up the TLS settings + # therefore cannot quietly succeed over plaintext and hide the bug. + # + # `--tls-auth-clients no` mirrors the managed services: the client verifies + # the server, not the other way round. No mTLS, no client certs to ship. + command: > + redis-server + --port 0 + --tls-port 6380 + --tls-cert-file /certs/server.crt + --tls-key-file /certs/server.key + --tls-ca-cert-file /certs/ca.crt + --tls-auth-clients no + --requirepass ${REDIS_DEV_PASSWORD:-devpassword} + --loglevel notice + ports: + # Same port inside and out, so one hostname:port works from containers + # (unstract-redis-managed:6380) and from a backend on the host + # (localhost:6380). + - "6380:6380" + volumes: + - ./redis-tls/certs:/certs:ro + - redis_managed_data:/data + healthcheck: + test: + [ + "CMD", + "redis-cli", + "--tls", + "-p", "6380", + "--cacert", "/certs/ca.crt", + "-a", "${REDIS_DEV_PASSWORD:-devpassword}", + "--no-auth-warning", + "ping", + ] + interval: 10s + timeout: 5s + retries: 5 + labels: + - traefik.enable=false + +volumes: + redis_managed_data: + +networks: + default: + name: unstract-network + external: true diff --git a/docker/redis-tls/.gitignore b/docker/redis-tls/.gitignore new file mode 100644 index 0000000000..e63f64c5cf --- /dev/null +++ b/docker/redis-tls/.gitignore @@ -0,0 +1,2 @@ +# Dev-only self-signed material, regenerated by generate-certs.sh. +certs/ diff --git a/docker/redis-tls/README.md b/docker/redis-tls/README.md new file mode 100644 index 0000000000..3f01f5940a --- /dev/null +++ b/docker/redis-tls/README.md @@ -0,0 +1,121 @@ +# Testing against a managed-like Redis locally (UN-4123) + +A managed Redis (Memorystore, ElastiCache, Azure Cache) differs from the dev Redis +in exactly two ways our code has to handle: it **requires AUTH** and it **speaks +TLS**. `docker-compose-redis-tls.yaml` runs a Redis that does both, so the TLS path +can be exercised without cloud access or a VPN. + +It runs **alongside** the normal `unstract-redis` on port 6380, so nothing switches +until you point services at it — the point of the exercise is to flip between the +two and confirm both work. + +Its plaintext listener is disabled (`--port 0`), the same as Azure Cache's default. +A service that fails to pick up the TLS settings therefore *cannot* quietly succeed +over plaintext and hide the bug. + +## Start it + +```bash +cd docker +./redis-tls/generate-certs.sh +docker compose -f docker-compose-redis-tls.yaml up -d +``` + +Certificates are self-signed and dev-only. The SAN list covers both names the same +server answers to — `unstract-redis-managed` from inside the network, +`localhost` from a backend on the host — because a certificate valid for only one +fails verification from the other side, which looks like a code bug and is not one. + +Confirm it is up, and that plaintext is refused: + +```bash +docker exec unstract-redis-managed redis-cli --tls -p 6380 \ + --cacert /certs/ca.crt -a devpassword --no-auth-warning ping # PONG +docker exec unstract-redis-managed redis-cli -p 6380 \ + -a devpassword --no-auth-warning ping # I/O error +``` + +## Switch the platform onto it + +Two values differ by where the process runs: containers reach it as +`unstract-redis-managed:6380`, a backend on the host as `localhost:6380`. + +**`backend/.env`** (runs on the host): + +```bash +REDIS_URL=rediss://:devpassword@localhost:6380/0?ssl_cert_reqs=required +REDIS_SSL_CA_CERTS=/abs/path/to/unstract/docker/redis-tls/certs/ca.crt +``` + +**`workers/.env`, `platform-service/.env`** (containers): + +```bash +REDIS_URL=rediss://:devpassword@unstract-redis-managed:6380/0?ssl_cert_reqs=required +REDIS_SSL_CA_CERTS=/certs/ca.crt +CACHE_REDIS_URL=rediss://:devpassword@unstract-redis-managed:6380/1?ssl_cert_reqs=required +``` + +Those containers need the CA mounted. Add to your `docker/compose.override.yaml`: + +```yaml +services: + worker-pg-orchestrator-general: # repeat for each worker + platform-service + volumes: + - ./redis-tls/certs:/certs:ro +``` + +**`runner/.env`** — one deliberate difference: + +```bash +REDIS_URL=rediss://:devpassword@unstract-redis-managed:6380/0 +REDIS_SSL_CERT_REQS=none +``` + +The runner forwards these to each **tool sidecar** it spawns, and sidecars get no +CA mount — the runner builds their environment as an allowlist and mounts only the +shared log dir. `none` keeps the connection encrypted while skipping verification, +which still exercises the forwarding fix and the TLS handshake. Against a real +managed endpoint this does not arise: ElastiCache and Azure chain to public CAs, and +for Memorystore you would mount its CA into the sidecar image. + +Then restart: `docker compose restart` for the containers, and your usual manual +restart for the backend. + +## What to check + +1. **An execution completes.** Include a workflow that uses a **container-based + tool** (classifier or text_extractor) — a Prompt Studio structure tool runs + in-process in the executor and never spawns a sidecar, so it leaves the whole + sidecar path untested. +2. **Logs stream in the UI and land in `execution_log`.** That is the Redis-list + transport end to end, the part with no unit-test coverage. +3. **The keys are in the new server, and the old one is idle:** + + ```bash + docker exec unstract-redis-managed redis-cli --tls -p 6380 \ + --cacert /certs/ca.crt -a devpassword --no-auth-warning dbsize + docker exec unstract-redis redis-cli dbsize # should not be growing + ``` + +4. **Nothing fell back to plaintext.** The TLS-only listener makes this + self-enforcing: a component that missed the settings fails loudly instead. + +## Switch back + +Remove the `REDIS_URL` / `REDIS_SSL_*` lines, restart, and run the same execution +again. Both modes are supported and both must pass — that is the acceptance bar, +not just "TLS works". + +## Against a real managed Redis + +Same env, different endpoint. Memorystore is VPC-private, so from a laptop it needs +a tunnel: + +```bash +gcloud compute ssh -- -L 6380::6379 +REDIS_URL=rediss://:@localhost:6380/0?ssl_cert_reqs=required +``` + +Drop the `rediss` to `redis` when the instance has TLS disabled — an AUTH string +without in-transit encryption is a supported configuration and worth testing too, +since it is what a VPC-internal deployment may well run. diff --git a/docker/redis-tls/generate-certs.sh b/docker/redis-tls/generate-certs.sh new file mode 100755 index 0000000000..f43e260e36 --- /dev/null +++ b/docker/redis-tls/generate-certs.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +# Self-signed CA + server certificate for the local TLS Redis (UN-4123). +# +# Only for local development: it stands in for a managed Redis endpoint so the +# TLS path can be exercised without cloud access or a VPN. Never use these +# anywhere else — the private keys are written unencrypted, right here. +# +# The SAN list covers both names the same server answers to: containers reach it +# as `unstract-redis-managed`, while a backend running on the host reaches the +# published port as `localhost`. A certificate valid for only one of them fails +# verification from the other side, which looks like a code bug and is not one. +set -euo pipefail + +CERT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/certs" +mkdir -p "$CERT_DIR" +cd "$CERT_DIR" + +if [[ -f server.crt && "${FORCE:-}" != "1" ]]; then + echo "Certificates already exist in $CERT_DIR (FORCE=1 to regenerate)." + exit 0 +fi + +openssl genrsa -out ca.key 4096 2>/dev/null +openssl req -x509 -new -nodes -key ca.key -sha256 -days 825 -out ca.crt \ + -subj "/CN=Unstract Local Redis Dev CA" 2>/dev/null + +openssl genrsa -out server.key 2048 2>/dev/null +openssl req -new -key server.key -out server.csr \ + -subj "/CN=unstract-redis-managed" 2>/dev/null + +cat > server.ext <<'EXT' +subjectAltName = DNS:unstract-redis-managed, DNS:localhost, IP:127.0.0.1 +extendedKeyUsage = serverAuth +EXT + +openssl x509 -req -in server.csr -CA ca.crt -CAkey ca.key -CAcreateserial \ + -out server.crt -days 825 -sha256 -extfile server.ext 2>/dev/null + +# Redis runs as uid 999 in the official image and must read the key. +chmod 644 ca.crt server.crt server.key +rm -f server.csr server.ext ca.srl + +echo "Wrote CA + server certificate to $CERT_DIR" +echo " CA (for REDIS_SSL_CA_CERTS): $CERT_DIR/ca.crt" diff --git a/platform-service/sample.env b/platform-service/sample.env index 72ea02d195..b6ad62985c 100644 --- a/platform-service/sample.env +++ b/platform-service/sample.env @@ -14,6 +14,20 @@ REDIS_PASSWORD= REDIS_SENTINEL_MODE=False 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 +# 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 +# trusted (Memorystore). +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Backend DB PG_BE_HOST=unstract-db PG_BE_PORT=5432 diff --git a/runner/sample.env b/runner/sample.env index afb58aae15..a970b16d45 100644 --- a/runner/sample.env +++ b/runner/sample.env @@ -48,6 +48,33 @@ REDIS_PASSWORD= REDIS_SENTINEL_MODE=False REDIS_SENTINEL_MASTER_NAME=mymaster +# Managed / external Redis with TLS (UN-4123). All optional — unset means the +# plaintext connection this file otherwise describes, unchanged. +# +# Two ways to configure it, pick one: +# 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 +# A URL wins over the discrete vars above. Percent-encode @ / + in the password. +# +# Auth is password-only, as Redis's built-in `default` user — that is what a managed +# AUTH string is. Leave REDIS_USER empty for a managed endpoint; named ACL users are +# not supported platform-wide. +# +# REDIS_SSL_CA_CERTS is only needed when the server's CA is not in the system trust +# store (Memorystore). ElastiCache and Azure Cache chain to public CAs. +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= + +# Proactive health check for pooled connections, in seconds (0 disables). +# Managed Redis fails over during maintenance and reaps idle connections — Azure +# Cache closes them at 10 minutes — so a parked connection can be dead before its +# next command. Defaults to 30. +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Flask related envs # Can be 'production' or 'development' FLASK_ENV=production diff --git a/runner/src/unstract/runner/constants.py b/runner/src/unstract/runner/constants.py index e4f7449fb7..166f3bdcda 100644 --- a/runner/src/unstract/runner/constants.py +++ b/runner/src/unstract/runner/constants.py @@ -41,6 +41,15 @@ class Env: REDIS_PASSWORD = "REDIS_PASSWORD" REDIS_SENTINEL_MODE = "REDIS_SENTINEL_MODE" REDIS_SENTINEL_MASTER_NAME = "REDIS_SENTINEL_MASTER_NAME" + # UN-4123. Same allowlist trap as LOG_TRANSPORT below: the sidecar publishes tool + # logs through LogPublisher and builds its own Redis client, so without these it + # would keep connecting in PLAINTEXT to db 0 while every other process moved to + # TLS — a connection failure at best, and tool logs silently absent at worst. + REDIS_DB = "REDIS_DB" + REDIS_SSL = "REDIS_SSL" + REDIS_SSL_CERT_REQS = "REDIS_SSL_CERT_REQS" + REDIS_SSL_CA_CERTS = "REDIS_SSL_CA_CERTS" + REDIS_URL = "REDIS_URL" CELERY_BROKER_BASE_URL = "CELERY_BROKER_BASE_URL" CELERY_BROKER_USER = "CELERY_BROKER_USER" CELERY_BROKER_PASS = "CELERY_BROKER_PASS" diff --git a/runner/src/unstract/runner/runner.py b/runner/src/unstract/runner/runner.py index 2414ac7548..bc538021ac 100644 --- a/runner/src/unstract/runner/runner.py +++ b/runner/src/unstract/runner/runner.py @@ -257,6 +257,27 @@ def _get_sidecar_container_config( ), "CONTAINER_NAME": container_name, } + # UN-4123: TLS and db settings for the sidecar's OWN Redis client. It + # publishes tool logs through LogPublisher and tracks tool status in Redis, + # and this dict is a hand-picked allowlist rather than inherited env — the + # same trap that made LOG_TRANSPORT above necessary. Without these the + # sidecar would keep connecting in plaintext to db 0 after everything else + # moved to TLS. + # + # Set only when present, so an unset var leaves the sidecar's environment + # exactly as it is today rather than introducing an empty-string value — + # which os.getenv() in the sidecar would treat as configured. + for _redis_env in ( + Env.REDIS_DB, + Env.REDIS_SSL, + Env.REDIS_SSL_CERT_REQS, + Env.REDIS_SSL_CA_CERTS, + Env.REDIS_URL, + ): + _redis_value = os.getenv(_redis_env) + if _redis_value: + sidecar_env[_redis_env] = _redis_value + sidecar_config = self.client.get_container_run_config( command=[], file_execution_id=file_execution_id, diff --git a/runner/tests/test_sidecar_log_transport.py b/runner/tests/test_sidecar_log_transport.py index 01c46280b1..72e668e0cd 100644 --- a/runner/tests/test_sidecar_log_transport.py +++ b/runner/tests/test_sidecar_log_transport.py @@ -82,3 +82,68 @@ def test_celery_broker_still_forwarded_for_the_flag_off_path(self, sidecar_env): # Flag-off must stay intact: the sidecar still publishes over AMQP. envs = sidecar_env(CELERY_BROKER_BASE_URL="amqp://x") assert envs["CELERY_BROKER_BASE_URL"] == "amqp://x" + + +class TestSidecarRedisTls: + """TLS settings must reach the sidecar too (UN-4123). + + Same allowlist trap as ``LOG_TRANSPORT`` above, one layer deeper: the sidecar + builds its OWN Redis client, so when the platform moves to a TLS endpoint a + sidecar that never learned about it keeps dialling plaintext. The failure is a + connection error at best; at worst it reaches a different db and tool logs go + missing with everything else looking healthy. + """ + + def test_tls_settings_are_forwarded(self, sidecar_env): + envs = sidecar_env( + REDIS_SSL="true", + REDIS_SSL_CERT_REQS="required", + REDIS_SSL_CA_CERTS="/etc/ssl/redis-ca.pem", + ) + assert envs[Env.REDIS_SSL] == "true" + assert envs[Env.REDIS_SSL_CERT_REQS] == "required" + assert envs[Env.REDIS_SSL_CA_CERTS] == "/etc/ssl/redis-ca.pem" + + def test_db_is_forwarded(self, sidecar_env): + """Without this the sidecar sits on db 0 while everyone else honours REDIS_DB. + + Nothing errors: it simply publishes into a keyspace no consumer drains. + """ + envs = sidecar_env(REDIS_DB="3") + assert envs[Env.REDIS_DB] == "3" + + def test_url_is_forwarded(self, sidecar_env): + envs = sidecar_env(REDIS_URL="rediss://cache.example:6380/2") + assert envs[Env.REDIS_URL] == "rediss://cache.example:6380/2" + + def test_unset_values_are_omitted_entirely(self, sidecar_env, monkeypatch): + """An empty string is NOT the same as absent. + + ``os.getenv(key, fallback)`` returns "" for a key that exists but is empty, + which suppresses the fallback — so forwarding blanks would turn "inherit the + default" into "explicitly configured as nothing". + """ + for key in ( + Env.REDIS_SSL, + Env.REDIS_SSL_CERT_REQS, + Env.REDIS_SSL_CA_CERTS, + Env.REDIS_DB, + Env.REDIS_URL, + ): + monkeypatch.delenv(key, raising=False) + envs = sidecar_env() + for key in ( + Env.REDIS_SSL, + Env.REDIS_SSL_CERT_REQS, + Env.REDIS_SSL_CA_CERTS, + Env.REDIS_DB, + Env.REDIS_URL, + ): + assert key not in envs + + def test_plaintext_deployment_is_unchanged(self, sidecar_env, monkeypatch): + """The whole point: no TLS configured means the sidecar env is as it was.""" + monkeypatch.delenv(Env.REDIS_SSL, raising=False) + envs = sidecar_env(REDIS_HOST="r", REDIS_PORT="6379") + assert envs["REDIS_HOST"] == "r" + assert Env.REDIS_SSL not in envs diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index c392d6a4b2..6bfe3be6bd 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -5,6 +5,12 @@ - Sentinel mode: Sentinel.master_for() when REDIS_SENTINEL_MODE=True Mode is detected from {prefix}SENTINEL_MODE env var (LLMW pattern). + +A full URL in {prefix}URL (falling back to REDIS_URL) overrides the discrete +host/port/credential vars, and `rediss://` turns on TLS by itself — the scheme is +the switch, so there is no separate "use TLS" flag to forget. Discrete vars remain +the default and primary path: they need no URL-encoding of passwords, and they are +what the Helm chart and every sample.env configure. In Sentinel mode, REDIS_HOST/REDIS_PORT point to the K8s Sentinel service endpoint. Master name defaults to "mymaster" (configurable via REDIS_SENTINEL_MASTER_NAME env var). REDIS_PASSWORD is reused for Sentinel auth. @@ -17,6 +23,7 @@ import random import time from typing import Any +from urllib.parse import urlsplit, urlunsplit import redis from redis.sentinel import Sentinel @@ -32,6 +39,45 @@ _SENTINEL_JITTER_MAX = 1.2 _DEFAULT_SENTINEL_MASTER_NAME = os.getenv("REDIS_SENTINEL_MASTER_NAME", "mymaster") +# redis-py sends a PING before reusing a connection idle for longer than this, so a +# connection killed while parked (managed-Redis failover, an idle-connection reaper — +# Azure Cache closes at 10 minutes) is discovered and replaced by the health check +# rather than by the next real command failing. 30s is redis-py's own documented +# recommendation. 0 disables it, which is what every client except the two worker +# caches used before UN-4123. +_DEFAULT_HEALTH_CHECK_INTERVAL = 30 + + +def _resolve_health_check_interval(env_prefix: str, explicit: int) -> int: + """Explicit argument wins; otherwise env, otherwise the default above.""" + if explicit: + return explicit + raw = os.getenv( + f"{env_prefix}HEALTH_CHECK_INTERVAL", + os.getenv("REDIS_HEALTH_CHECK_INTERVAL", str(_DEFAULT_HEALTH_CHECK_INTERVAL)), + ) + try: + return max(int(raw), 0) + except ValueError: + logger.warning( + "Invalid %sHEALTH_CHECK_INTERVAL=%r; using %s", + env_prefix, + raw, + _DEFAULT_HEALTH_CHECK_INTERVAL, + ) + return _DEFAULT_HEALTH_CHECK_INTERVAL + + +def _strip_url_db_path(url: str) -> str: + """Drop the / path from a Redis URL. + + redis-py resolves the db from the URL path and IGNORES a `db=` kwarg, so a + caller that asks for a specific db (sdk1 metrics uses db=1) would silently get + the URL's db instead. Stripping the path lets the explicit argument apply. + """ + parts = urlsplit(url) + return urlunsplit((parts.scheme, parts.netloc, "", parts.query, parts.fragment)) + def _is_sentinel_mode(env_prefix: str) -> bool: return os.getenv(f"{env_prefix}SENTINEL_MODE", "False").strip().lower() == "true" @@ -59,8 +105,11 @@ def create_redis_client( socket_connect_timeout: Connection timeout in seconds. socket_timeout: Socket timeout in seconds. max_connections: Optional max connections for ConnectionPool. - health_check_interval: Proactive health check interval in seconds (0=disabled). - db: Optional DB index override. + health_check_interval: Proactive health check interval in seconds. 0 means + "unset" and falls back to {env_prefix}HEALTH_CHECK_INTERVAL, then + REDIS_HEALTH_CHECK_INTERVAL, then 30s; set that env to 0 to disable. + db: Optional DB index override. Applied even when a URL carries its own + db path, which redis-py would otherwise silently prefer. Returns: Configured redis.Redis client (standalone or Sentinel-backed). @@ -68,6 +117,9 @@ def create_redis_client( Raises: RedisSentinelConnectionError: After exhausting retries in Sentinel mode. """ + health_check_interval = _resolve_health_check_interval( + env_prefix, health_check_interval + ) if _is_sentinel_mode(env_prefix): return _create_sentinel_client( env_prefix=env_prefix, @@ -106,7 +158,14 @@ def _resolve_redis_env( if db_override is not None else int(os.getenv(f"{env_prefix}DB", os.getenv("REDIS_DB", "0"))) ) - ssl = os.getenv(f"{env_prefix}SSL", "false").strip().lower() == "true" + # Falls back to REDIS_SSL: before UN-4123 each prefix needed its own *_SSL, so + # 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" + ) result: dict[str, Any] = { "host": host, "port": port, @@ -114,7 +173,32 @@ def _resolve_redis_env( "username": username, "db": db, "ssl": ssl, + # A full URL, when given, is authoritative for host/port/credentials/db and + # carries TLS in its scheme (rediss://). Everything above stays the default + # path, so an unset URL changes nothing. + "url": os.getenv(f"{env_prefix}URL", os.getenv("REDIS_URL", "")).strip(), } + # A prefix that INHERITS the generic REDIS_URL must still honour its own + # {prefix}DB. The Helm chart sets CACHE_REDIS_DB=1 while configuring one + # REDIS_URL for the platform; without this the worker cache would silently + # follow the URL's db instead, landing on db 0 beside everything else. An + # explicit {prefix}URL is left alone — it names its own db deliberately. + own_url = os.getenv(f"{env_prefix}URL", "").strip() + own_db = os.getenv(f"{env_prefix}DB", "").strip() + if result["url"] and not own_url and own_db and db_override is None: + result["db_from_prefix_env"] = int(own_db) + # Read OUTSIDE the `if ssl` below: URL mode carries TLS in the scheme and never + # sets {prefix}SSL, so gating the CA on that flag left `rediss://` verifying + # against the system trust store alone — which fails for exactly the servers + # that need a CA. Consumers decide whether it applies. + # + # 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", "") + ).strip() + if ca_certs: + result["ssl_ca_certs"] = ca_certs if ssl: result["ssl_cert_reqs"] = os.getenv(f"{env_prefix}SSL_CERT_REQS", "required") return result @@ -162,6 +246,8 @@ def _build_connection_kwargs( if env.get("ssl"): kwargs["ssl"] = True kwargs["ssl_cert_reqs"] = env.get("ssl_cert_reqs", "required") + if env.get("ssl_ca_certs"): + kwargs["ssl_ca_certs"] = env["ssl_ca_certs"] return kwargs @@ -176,6 +262,20 @@ def _create_standalone_client( ) -> redis.Redis: env = _resolve_redis_env(env_prefix, default_port="6379", db_override=db_override) + if env["url"]: + return _create_client_from_url( + url=env["url"], + decode_responses=decode_responses, + socket_connect_timeout=socket_connect_timeout, + socket_timeout=socket_timeout, + max_connections=max_connections, + health_check_interval=health_check_interval, + db_override=( + db_override if db_override is not None else env.get("db_from_prefix_env") + ), + ssl_ca_certs=env.get("ssl_ca_certs"), + ) + logger.info( "Redis standalone mode enabled. Connecting to %s:%s", env["host"], env["port"] ) @@ -191,12 +291,62 @@ def _create_standalone_client( kwargs["port"] = env["port"] if max_connections is not None: - pool = redis.ConnectionPool(max_connections=max_connections, **kwargs) + pool_kwargs = dict(kwargs) + # ConnectionPool hands its kwargs to the connection class, and the plain + # Connection has no `ssl` parameter — passing it raises TypeError. TLS on a + # POOLED client (platform-service sets max_connections) therefore has to be + # selected by connection class, not by a flag. + if pool_kwargs.pop("ssl", False): + pool_kwargs["connection_class"] = redis.SSLConnection + pool = redis.ConnectionPool(max_connections=max_connections, **pool_kwargs) return redis.Redis(connection_pool=pool) return redis.Redis(**kwargs) +def _create_client_from_url( + url: str, + decode_responses: bool, + socket_connect_timeout: int, + socket_timeout: int, + max_connections: int | None, + health_check_interval: int, + db_override: int | None, + ssl_ca_certs: str | None, +) -> redis.Redis: + """Build a client from a full Redis URL. + + `rediss://` selects TLS on its own — redis-py picks SSLConnection from the + scheme — so TLS needs no separate switch, and `redis://` behaves exactly as the + discrete host/port path does. TLS verification is tuned in the URL itself, e.g. + `?ssl_cert_reqs=required`. + """ + kwargs: dict[str, Any] = { + "decode_responses": decode_responses, + "socket_connect_timeout": socket_connect_timeout, + "socket_timeout": socket_timeout, + } + if health_check_interval: + kwargs["health_check_interval"] = health_check_interval + if max_connections is not None: + kwargs["max_connections"] = max_connections + if ssl_ca_certs and url.startswith("rediss://"): + kwargs["ssl_ca_certs"] = ssl_ca_certs + if db_override is not None: + # The URL path wins over a db kwarg in redis-py, so it has to go. + url = _strip_url_db_path(url) + kwargs["db"] = db_override + + parts = urlsplit(url) + logger.info( + "Redis URL mode enabled. Connecting to %s:%s (tls=%s)", + parts.hostname, + parts.port, + parts.scheme == "rediss", + ) + return redis.Redis.from_url(url, **kwargs) + + def _create_sentinel_client( env_prefix: str, decode_responses: bool, diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py new file mode 100644 index 0000000000..07f8c3c8f3 --- /dev/null +++ b/unstract/core/tests/test_redis_client_config.py @@ -0,0 +1,258 @@ +"""Connection configuration for the shared Redis client factory (UN-4123). + +These tests build clients and inspect the resulting connection kwargs. Nothing +connects to a server: every property under test is decided at construction, which +is exactly where the bugs being locked down lived. + +The recurring theme is that a MISCONFIGURED Redis client fails quietly. A missing +TLS flag is a plaintext socket against a TLS port; an ignored db override is reads +and writes against the wrong keyspace; an empty-string env var counts as "set" to +``os.getenv`` and shadows the fallback that would have supplied the real value. +None of those raise at import, so they are asserted here instead. +""" + +import pytest +import redis + +from unstract.core.cache.redis_client import create_redis_client + +_REDIS_ENV_MARKERS = ("REDIS_", "CACHE_REDIS_", "MANUAL_REVIEW_REDIS_") + + +@pytest.fixture(autouse=True) +def _clean_redis_env(monkeypatch): + """Drop inherited REDIS_* so a developer's shell cannot change the outcome.""" + import os + + for key in list(os.environ): + if any(marker in key for marker in _REDIS_ENV_MARKERS): + monkeypatch.delenv(key, raising=False) + + +def _kwargs(client: redis.Redis) -> dict: + return client.connection_pool.connection_kwargs + + +def _connection_class(client: redis.Redis) -> str: + return client.connection_pool.connection_class.__name__ + + +class TestDefaults: + def test_no_env_is_plaintext_localhost_db0(self): + """The in-cluster/local path: unchanged by everything else in this suite.""" + client = create_redis_client() + assert _connection_class(client) == "Connection" + assert _kwargs(client)["host"] == "localhost" + assert _kwargs(client)["port"] == 6379 + assert _kwargs(client)["db"] == 0 + assert "ssl_cert_reqs" not in _kwargs(client) + + def test_health_check_is_on_by_default(self, monkeypatch): + """Guards against a connection killed while idle. + + A managed Redis fails over during maintenance and reaps idle connections + (Azure Cache at 10 minutes). Without a health check the pooled connection + is only discovered dead when a real command fails on it. + """ + assert _kwargs(create_redis_client())["health_check_interval"] == 30 + + monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "0") + # redis-py always populates this key; 0 is its "disabled" value. + assert _kwargs(create_redis_client())["health_check_interval"] == 0 + + def test_explicit_argument_beats_env(self, monkeypatch): + monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "45") + assert _kwargs(create_redis_client(health_check_interval=7))[ + "health_check_interval" + ] == 7 + + def test_unparseable_health_check_falls_back(self, monkeypatch): + """A typo must not take the platform's Redis down.""" + monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "thirty") + assert _kwargs(create_redis_client())["health_check_interval"] == 30 + + +class TestDiscreteTLS: + def test_ssl_selects_a_tls_connection(self, monkeypatch): + monkeypatch.setenv("REDIS_HOST", "cache.internal") + monkeypatch.setenv("REDIS_SSL", "true") + client = create_redis_client() + assert _connection_class(client) == "SSLConnection" + assert _kwargs(client)["ssl_cert_reqs"] == "required" + + def test_pooled_tls_client_is_usable(self, monkeypatch): + """Regression: TLS + max_connections used to fail on the FIRST COMMAND. + + ``ConnectionPool`` hands its kwargs to the connection class, and the plain + ``Connection`` has no ``ssl`` parameter. Construction succeeded, the pool + kept the non-TLS class, and the first command raised + ``TypeError: AbstractConnection.__init__() got an unexpected keyword + argument 'ssl'`` — so platform-service (max_connections=10) would have + started healthy and broken on first use. + """ + monkeypatch.setenv("REDIS_SSL", "true") + client = create_redis_client(max_connections=10) + pool = client.connection_pool + assert _connection_class(client) == "SSLConnection" + assert "ssl" not in _kwargs(client) + # Instantiating the connection is the step that used to raise; redis-py + # opens the socket lazily, so this stays offline. + assert pool.connection_class(**pool.connection_kwargs) is not None + + def test_prefixed_client_inherits_the_global_ssl_flag(self, monkeypatch): + """CACHE_REDIS_* and MANUAL_REVIEW_* must not need their own SSL flag. + + Before this fell back, enabling TLS platform-wide meant remembering every + prefix, and a forgotten one is a plaintext client against a TLS port. + """ + monkeypatch.setenv("REDIS_SSL", "true") + for prefix in ("CACHE_REDIS_", "MANUAL_REVIEW_REDIS_"): + assert _connection_class(create_redis_client(env_prefix=prefix)) == ( + "SSLConnection" + ) + + def test_prefix_can_still_override_the_global_flag(self, monkeypatch): + monkeypatch.setenv("REDIS_SSL", "true") + monkeypatch.setenv("CACHE_REDIS_SSL", "false") + assert _connection_class(create_redis_client(env_prefix="CACHE_REDIS_")) == ( + "Connection" + ) + + def test_ca_certs_reach_the_connection(self, monkeypatch): + """Needed for Memorystore, whose CA is not in the system trust store.""" + monkeypatch.setenv("REDIS_SSL", "true") + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert _kwargs(create_redis_client())["ssl_ca_certs"] == "/etc/ssl/redis-ca.pem" + + def test_ca_certs_ignored_without_tls(self, monkeypatch): + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert "ssl_ca_certs" not in _kwargs(create_redis_client()) + + +class TestUrlMode: + def test_rediss_scheme_turns_on_tls_without_a_flag(self, monkeypatch): + monkeypatch.setenv( + "REDIS_URL", "rediss://cache.example:6380/2?ssl_cert_reqs=required" + ) + client = create_redis_client() + assert _connection_class(client) == "SSLConnection" + assert _kwargs(client)["ssl_cert_reqs"] == "required" + assert _kwargs(client)["db"] == 2 + + def test_plain_scheme_stays_plaintext(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://cache.example:6379") + assert _connection_class(create_redis_client()) == "Connection" + + def test_password_is_url_decoded(self, monkeypatch): + """Generated passwords contain @ / +, so they arrive percent-encoded.""" + monkeypatch.setenv("REDIS_URL", "rediss://:p%40ss%2Fword@cache.example:6380") + assert _kwargs(create_redis_client())["password"] == "p@ss/word" + + def test_explicit_db_argument_beats_the_url_path(self, monkeypatch): + """redis-py lets the URL path win over a ``db`` kwarg — silently. + + sdk1's metrics client asks for db=1 explicitly. Without stripping the path + it would land in the URL's db instead, writing metrics into another + service's keyspace with nothing to indicate it. + """ + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/5") + assert _kwargs(create_redis_client(db=1))["db"] == 1 + + def test_url_db_applies_when_no_override_is_given(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/5") + assert _kwargs(create_redis_client())["db"] == 5 + + def test_url_takes_precedence_over_discrete_vars(self, monkeypatch): + monkeypatch.setenv("REDIS_HOST", "in-cluster") + monkeypatch.setenv("REDIS_URL", "redis://managed.example:6379") + assert _kwargs(create_redis_client())["host"] == "managed.example" + + def test_prefixed_url_is_used_for_that_prefix_only(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://shared.example:6379") + monkeypatch.setenv("CACHE_REDIS_URL", "redis://cache.example:6379") + assert _kwargs(create_redis_client())["host"] == "shared.example" + assert ( + _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["host"] + == "cache.example" + ) + + def test_ca_certs_apply_in_url_mode(self, monkeypatch): + """Found by a live run against a TLS Redis, not by reading the code. + + URL mode carries TLS in the scheme and never sets REDIS_SSL, so while the + CA was read only inside that flag's branch, `rediss://` verified against + the system trust store alone — and failed for precisely the servers a CA + is needed for (Memorystore's CA is not publicly trusted). + """ + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert _kwargs(create_redis_client())["ssl_ca_certs"] == "/etc/ssl/redis-ca.pem" + + def test_ca_certs_ignored_for_a_plaintext_url(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://cache.example:6379/0") + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert "ssl_ca_certs" not in _kwargs(create_redis_client()) + + def test_prefix_db_wins_over_an_inherited_url(self, monkeypatch): + """The Helm chart's shape: one REDIS_URL, plus CACHE_REDIS_DB=1. + + The cache prefix inherits the generic URL for its endpoint, but its own + db must still apply — otherwise the worker cache silently moves to the + URL's db and lands beside everything else on db 0. + """ + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("CACHE_REDIS_DB", "1") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["db"] == 1 + assert _kwargs(create_redis_client())["db"] == 0 + + def test_an_explicit_prefix_url_keeps_its_own_db(self, monkeypatch): + """A URL written FOR this prefix names its db deliberately.""" + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("CACHE_REDIS_URL", "rediss://cache.example:6380/3") + monkeypatch.setenv("CACHE_REDIS_DB", "1") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["db"] == 3 + + def test_explicit_argument_still_wins(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("CACHE_REDIS_DB", "1") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_", db=7))["db"] == 7 + + def test_pool_size_survives_url_mode(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380") + client = create_redis_client(max_connections=10) + assert client.connection_pool.max_connections == 10 + assert _connection_class(client) == "SSLConnection" + + +class TestAuth: + def test_password_only_auth_sends_no_username(self, monkeypatch): + """Managed AUTH strings authenticate the built-in ``default`` user. + + Named ACL users are not supported platform-wide — django-redis discards + the username — so an empty REDIS_USER must stay empty rather than + defaulting to something that turns AUTH into its two-argument form. + """ + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "s3cr3t" + assert kwargs.get("username") is None + + def test_prefixed_client_inherits_the_global_password(self, monkeypatch): + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["password"] == ( + "s3cr3t" + ) + + def test_empty_prefixed_password_shadows_the_fallback(self, monkeypatch): + """Documents a trap rather than endorsing it. + + ``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. + """ + 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 diff --git a/unstract/workflow-execution/src/unstract/workflow_execution/constants.py b/unstract/workflow-execution/src/unstract/workflow_execution/constants.py index 3786706cd2..120604fd6b 100644 --- a/unstract/workflow-execution/src/unstract/workflow_execution/constants.py +++ b/unstract/workflow-execution/src/unstract/workflow_execution/constants.py @@ -28,6 +28,13 @@ class ToolRuntimeVariable: REDIS_PASSWORD = "REDIS_PASSWORD" REDIS_SENTINEL_MODE = "REDIS_SENTINEL_MODE" REDIS_SENTINEL_MASTER_NAME = "REDIS_SENTINEL_MASTER_NAME" + # UN-4123 — TLS settings for tool containers, which build their own Redis + # client (sdk1 metrics). Same allowlist trap as the sidecar's. + REDIS_DB = "REDIS_DB" + REDIS_SSL = "REDIS_SSL" + REDIS_SSL_CERT_REQS = "REDIS_SSL_CERT_REQS" + REDIS_SSL_CA_CERTS = "REDIS_SSL_CA_CERTS" + REDIS_URL = "REDIS_URL" class WorkflowFileType: diff --git a/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py b/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py index 040c9485cb..14c2a22ca8 100644 --- a/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py +++ b/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py @@ -240,6 +240,20 @@ def get_tool_environment_variables(self) -> dict[str, Any]: ToolRV.REDIS_SENTINEL_MODE: self.redis_sentinel_mode or "False", ToolRV.REDIS_SENTINEL_MASTER_NAME: self.redis_sentinel_master_name or "mymaster", + # UN-4123: only the keys actually set, so an unset var leaves the tool's + # environment unchanged instead of gaining an empty string that + # os.getenv() would read as configured. + **{ + name: os.environ[name] + for name in ( + ToolRV.REDIS_DB, + ToolRV.REDIS_SSL, + ToolRV.REDIS_SSL_CERT_REQS, + ToolRV.REDIS_SSL_CA_CERTS, + ToolRV.REDIS_URL, + ) + if os.environ.get(name) + }, } # For async LLM Whisperer extraction if self.llmw_poll_interval: diff --git a/workers/sample.env b/workers/sample.env index abb09255f3..85131d8c49 100644 --- a/workers/sample.env +++ b/workers/sample.env @@ -78,6 +78,20 @@ REDIS_DB=0 REDIS_SENTINEL_MODE=False 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 +# 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 +# trusted (Memorystore). +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Cache-Specific Redis Configuration CACHE_REDIS_ENABLED=true CACHE_REDIS_HOST=unstract-redis @@ -85,8 +99,11 @@ CACHE_REDIS_PORT=6379 CACHE_REDIS_DB=0 CACHE_REDIS_PASSWORD= CACHE_REDIS_USERNAME= +# Falls back to REDIS_SSL when unset (UN-4123), so TLS does not have to be repeated +# per prefix — a forgotten one is a plaintext client against a TLS port. CACHE_REDIS_SSL=false CACHE_REDIS_SSL_CERT_REQS=required +# CACHE_REDIS_URL= # Cache Redis Sentinel mode for worker execution cache backend # When True: point CACHE_REDIS_HOST to Sentinel K8s service DNS, CACHE_REDIS_PORT to 26379