diff --git a/charts/model-engine/Chart.yaml b/charts/model-engine/Chart.yaml index 26a32221..ed2f7a2a 100644 --- a/charts/model-engine/Chart.yaml +++ b/charts/model-engine/Chart.yaml @@ -15,7 +15,7 @@ type: application # This is the chart version. This version number should be incremented each time you make changes # to the chart and its templates, including the app version. # Versions are expected to follow Semantic Versioning (https://semver.org/) -version: 0.2.9 +version: 0.2.10 # This is the version number of the application being deployed. This version number should be # incremented each time you make changes to the application. Versions are not expected to diff --git a/charts/model-engine/templates/_helpers.tpl b/charts/model-engine/templates/_helpers.tpl index 18935711..7d03dd65 100644 --- a/charts/model-engine/templates/_helpers.tpl +++ b/charts/model-engine/templates/_helpers.tpl @@ -290,6 +290,46 @@ env: {{- end }} {{- end }} +{{- /* Resolves the Redis scheme for both the app and the KEDA scaler. An unset + value mirrors the app's own fallback -- a configured credential implies + TLS, which is what ElastiCache in-transit encryption needs -- so the two + cannot disagree about the transport. */}} +{{- define "modelEngine.redisEnableTLSIsSet" -}} +{{- if not (or (kindIs "invalid" .Values.redis.enableTLS) (eq (toString .Values.redis.enableTLS) "")) -}}true{{- end -}} +{{- end }} + +{{- define "modelEngine.redisEnableTLS" -}} +{{- if include "modelEngine.redisEnableTLSIsSet" . -}} +{{- .Values.redis.enableTLS }} +{{- else if or .Values.redis.authSecretName .Values.redis.auth -}} +true +{{- else -}} +false +{{- end -}} +{{- end }} + +{{- /* Every workload that opens a Redis connection must include this, or it + reaches an authenticated broker with no credential and fails NOAUTH. */}} +{{- define "modelEngine.redisAuthEnv" }} + {{- if .Values.redis.authSecretName }} + - name: REDIS_AUTH_TOKEN + valueFrom: + secretKeyRef: + name: {{ .Values.redis.authSecretName }} + key: {{ .Values.redis.authSecretKey | default "auth_token" }} + {{- else if .Values.redis.auth }} + - name: REDIS_AUTH_TOKEN + value: {{ .Values.redis.auth }} + {{- end }} + {{- /* Withheld when the chart cannot determine it: a token reaching the pod + through extraEnvVars is invisible here, and an explicit false would + override the app's own inference and force plaintext. */}} + {{- if or (include "modelEngine.redisEnableTLSIsSet" .) .Values.redis.authSecretName .Values.redis.auth }} + - name: REDIS_ENABLE_TLS + value: {{ include "modelEngine.redisEnableTLS" . | quote }} + {{- end }} +{{- end }} + {{- define "modelEngine.serviceEnvBase" }} env: - name: DD_TRACE_ENABLED @@ -379,16 +419,7 @@ env: - name: CELERY_RESULT_BACKEND value: {{ .Values.celeryResultBackend | quote }} {{- end }} - {{- if .Values.redis.authSecretName }} - - name: REDIS_AUTH_TOKEN - valueFrom: - secretKeyRef: - name: {{ .Values.redis.authSecretName }} - key: {{ .Values.redis.authSecretKey | default "auth_token" }} - {{- else if .Values.redis.auth }} - - name: REDIS_AUTH_TOKEN - value: {{ .Values.redis.auth }} - {{- end }} + {{- include "modelEngine.redisAuthEnv" . }} {{- if .Values.azure}} - name: AZURE_IDENTITY_NAME value: {{ .Values.azure.identity_name }} diff --git a/charts/model-engine/templates/celery_autoscaler_stateful_set.yaml b/charts/model-engine/templates/celery_autoscaler_stateful_set.yaml index 3c7a0e95..540b3bed 100644 --- a/charts/model-engine/templates/celery_autoscaler_stateful_set.yaml +++ b/charts/model-engine/templates/celery_autoscaler_stateful_set.yaml @@ -66,6 +66,7 @@ spec: value: {{ $broker_name }} - name: CELERY_ELASTICACHE_ENABLED value: {{ (eq $message_broker "elasticache") | squote }} + {{- include "modelEngine.redisAuthEnv" . | indent 6 }} - name: POD_NAME valueFrom: fieldRef: diff --git a/charts/model-engine/templates/service_template_config_map.yaml b/charts/model-engine/templates/service_template_config_map.yaml index 57432a06..a49fd5bd 100644 --- a/charts/model-engine/templates/service_template_config_map.yaml +++ b/charts/model-engine/templates/service_template_config_map.yaml @@ -513,7 +513,7 @@ data: listName: "launch-endpoint-autoscaling:${ENDPOINT_ID}" listLength: "100" # something absurdly high so we don't scale past 1 pod activationListLength: "0" - enableTLS: "{{ .Values.redis.enableTLS }}" + enableTLS: "{{ include "modelEngine.redisEnableTLS" . }}" unsafeSsl: "{{ .Values.redis.unsafeSsl }}" databaseIndex: "${REDIS_DB_INDEX}" {{- if .Values.redis.enableAuth }} diff --git a/charts/model-engine/values.yaml b/charts/model-engine/values.yaml index 664a9766..318c7432 100644 --- a/charts/model-engine/values.yaml +++ b/charts/model-engine/values.yaml @@ -80,7 +80,9 @@ redis: auth: authSecretName: "" authSecretKey: "" - enableTLS: false + # Unset infers TLS from the presence of a Redis credential, which is what + # ElastiCache in-transit encryption needs. Set explicitly to pin the scheme. + enableTLS: enableAuth: false kedaSecretName: "" unsafeSsl: false diff --git a/model-engine/model_engine_server/common/config.py b/model-engine/model_engine_server/common/config.py index d85573f1..05e421bb 100644 --- a/model-engine/model_engine_server/common/config.py +++ b/model-engine/model_engine_server/common/config.py @@ -6,6 +6,7 @@ from dataclasses import dataclass from pathlib import Path from typing import Optional, Sequence +from urllib.parse import quote import yaml from azure.identity import DefaultAzureCredential @@ -47,6 +48,24 @@ def get_model_cache_directory_name(model_name: str): return name +def _apply_redis_auth_token(url: str) -> str: + """Add REDIS_AUTH_TOKEN to a Redis URL that carries no credential of its own. + + Keeps the password out of Helm values: the chart names the host and db + index, the app supplies the credential from its secret-backed env var. A + URL that already has userinfo is left alone so an explicit override wins. + """ + auth_token = os.getenv("REDIS_AUTH_TOKEN") + if not auth_token: + return url + scheme, sep, remainder = url.partition("://") + if not sep or "@" in remainder.split("/")[0]: + return url + # redis-py unquotes the userinfo, so percent-encoding here is what lets a + # password containing @, / or # survive URL parsing. + return f"{scheme}://:{quote(auth_token, safe='')}@{remainder}" + + @dataclass class HostedModelInferenceServiceConfig: gateway_namespace: str @@ -104,7 +123,7 @@ def from_yaml(cls, yaml_path): def cache_redis_url(self) -> str: # On-prem Redis support (explicit URL, no cloud provider dependency) if self.cache_redis_onprem_url: - return self.cache_redis_onprem_url + return _apply_redis_auth_token(self.cache_redis_onprem_url) cloud_provider = infra_config().cloud_provider @@ -112,10 +131,10 @@ def cache_redis_url(self) -> str: if cloud_provider == "onprem": if self.cache_redis_aws_url: logger.info("On-prem deployment using cache_redis_aws_url") - return self.cache_redis_aws_url + return _apply_redis_auth_token(self.cache_redis_aws_url) redis_host = os.getenv("REDIS_HOST", "redis") redis_port = getattr(infra_config(), "redis_port", 6379) - return f"redis://{redis_host}:{redis_port}/0" + return _apply_redis_auth_token(f"redis://{redis_host}:{redis_port}/0") if cloud_provider == "gcp": assert self.cache_redis_gcp_url, "cache_redis_gcp_url required for GCP" @@ -148,11 +167,13 @@ def cache_redis_url_expiration_timestamp(self) -> Optional[int]: @property def cache_redis_host_port(self) -> str: - # redis://redis.url:6379/ + # redis://:password@redis.url:6379/ # -> redis.url:6379 - if "rediss://" in self.cache_redis_url: - return self.cache_redis_url.split("rediss://")[1].split("@")[-1].split("/")[0] - return self.cache_redis_url.split("redis://")[1].split("/")[0] + # Credentials must be stripped for every scheme: this value is rendered + # into the KEDA scaler's `address` metadata, so a password left in here + # both breaks the address and leaks into the ScaledObject. + authority = self.cache_redis_url.split("://", 1)[-1] + return authority.split("@")[-1].split("/")[0] @property def cache_redis_db_index(self) -> int: diff --git a/model-engine/model_engine_server/core/celery/__init__.py b/model-engine/model_engine_server/core/celery/__init__.py index 3368bc69..0617d001 100644 --- a/model-engine/model_engine_server/core/celery/__init__.py +++ b/model-engine/model_engine_server/core/celery/__init__.py @@ -3,6 +3,7 @@ from .app import ( DEFAULT_TASK_VISIBILITY_SECONDS, TaskVisibility, + build_redis_url, celery_app, get_all_db_indexes, get_redis_host_port, @@ -10,6 +11,7 @@ ) __all__: Sequence[str] = ( + "build_redis_url", "celery_app", "get_all_db_indexes", "get_redis_host_port", diff --git a/model-engine/model_engine_server/core/celery/app.py b/model-engine/model_engine_server/core/celery/app.py index 6ec4711a..7ef233b5 100644 --- a/model-engine/model_engine_server/core/celery/app.py +++ b/model-engine/model_engine_server/core/celery/app.py @@ -2,6 +2,7 @@ import os from enum import IntEnum, unique from typing import Any, Dict, Iterable, List, Optional, Tuple, Union +from urllib.parse import quote import celery import redis.asyncio as aioredis @@ -209,31 +210,53 @@ def get_redis_endpoint(db_index: int = 0) -> str: return f"{scheme}:{auth_token}@{host}:{port}/{db_index}{query_params}" return f"{scheme}{host}:{port}/{db_index}{query_params}" host, port = get_redis_host_port() + return build_redis_url(host, port, db_index) + + +def redis_tls_enabled() -> bool: + """Whether to speak TLS to Redis. + + Transport security and authentication are independent: on-prem Redis is + reached over plaintext while still requiring a password. REDIS_ENABLE_TLS + decides the scheme; when it is unset the presence of a credential implies + TLS, which is what ElastiCache with in-transit encryption needs. + """ + explicit = os.getenv("REDIS_ENABLE_TLS") + if explicit: + return explicit.lower() == "true" + return bool(os.getenv("REDIS_AUTH_TOKEN")) + + +def build_redis_url(host: str, port: Union[str, int], db_index: int = 0) -> str: auth_token = os.getenv("REDIS_AUTH_TOKEN") - if auth_token: - return f"rediss://:{auth_token}@{host}:{port}/{db_index}?ssl_cert_reqs=none" - return f"redis://{host}:{port}/{db_index}" + # kombu and redis-py both unquote the userinfo, so percent-encoding here is + # what lets a password containing @, / or # survive URL parsing. + credential = f":{quote(auth_token, safe='')}@" if auth_token else "" + if redis_tls_enabled(): + return f"rediss://{credential}{host}:{port}/{db_index}?ssl_cert_reqs=none" + return f"redis://{credential}{host}:{port}/{db_index}" def get_redis_instance(db_index: int = 0) -> Union[Redis, StrictRedis]: host, port = get_redis_host_port() auth_token = os.getenv("REDIS_AUTH_TOKEN") - - if auth_token: - return StrictRedis( - host=host, - port=port, - db=db_index, - password=auth_token, - ssl=True, - ssl_cert_reqs="none", - ) - return Redis(host=host, port=port, db=db_index) + use_tls = redis_tls_enabled() + + if not auth_token and not use_tls: + return Redis(host=host, port=port, db=db_index) + ssl_kwargs: Dict[str, Any] = {"ssl": True, "ssl_cert_reqs": "none"} if use_tls else {} + return StrictRedis( + host=host, + port=port, + db=db_index, + password=auth_token, + **ssl_kwargs, + ) def get_async_redis_instance(db_index: int = 0) -> aioredis.Redis: host, port = get_redis_host_port() - return build_aioredis_client(f"redis://{host}:{port}/{db_index}") + return build_aioredis_client(build_redis_url(host, port, db_index)) def celery_app( diff --git a/model-engine/model_engine_server/core/celery/celery_autoscaler.py b/model-engine/model_engine_server/core/celery/celery_autoscaler.py index 9d9e1b92..3799b407 100644 --- a/model-engine/model_engine_server/core/celery/celery_autoscaler.py +++ b/model-engine/model_engine_server/core/celery/celery_autoscaler.py @@ -25,6 +25,7 @@ from model_engine_server.core.aws.roles import session from model_engine_server.core.celery import ( TaskVisibility, + build_redis_url, celery_app, get_all_db_indexes, get_redis_host_port, @@ -362,7 +363,7 @@ async def _init_client(self): get_redis_host_port() ) # Switches the redis instance based on CELERY_ELASTICACHE_ENABLED's value self.redis = { - db_index: build_aioredis_client(f"redis://{host}:{port}/{db_index}") + db_index: build_aioredis_client(build_redis_url(host, port, db_index)) for db_index in get_all_db_indexes() } self.initialized = True diff --git a/model-engine/tests/unit/common/test_redis_auth.py b/model-engine/tests/unit/common/test_redis_auth.py new file mode 100644 index 00000000..7c0d2a7f --- /dev/null +++ b/model-engine/tests/unit/common/test_redis_auth.py @@ -0,0 +1,152 @@ +import pytest +from model_engine_server.common.config import ( + HostedModelInferenceServiceConfig, + _apply_redis_auth_token, +) +from model_engine_server.core.celery.app import ( + build_redis_url, + get_redis_instance, + redis_tls_enabled, +) +from redis.connection import SSLConnection + +TOKEN_WITH_RESERVED_CHARS = "p@ss/w#rd" +ENCODED_RESERVED_CHARS = "p%40ss%2Fw%23rd" + + +@pytest.fixture +def redis_env(monkeypatch): + def _set(auth_token=None, enable_tls=None): + for name, value in ( + ("REDIS_AUTH_TOKEN", auth_token), + ("REDIS_ENABLE_TLS", enable_tls), + ): + if value is None: + monkeypatch.delenv(name, raising=False) + else: + monkeypatch.setenv(name, value) + + return _set + + +def test_tls_inferred_from_credential_when_unset(redis_env): + redis_env(auth_token="tok") + assert redis_tls_enabled() is True + redis_env() + assert redis_tls_enabled() is False + + +@pytest.mark.parametrize("value,expected", [("true", True), ("True", True), ("false", False)]) +def test_explicit_tls_setting_overrides_credential(redis_env, value, expected): + redis_env(auth_token="tok", enable_tls=value) + assert redis_tls_enabled() is expected + + +def test_credential_without_tls_yields_plaintext_url(redis_env): + redis_env(auth_token=TOKEN_WITH_RESERVED_CHARS, enable_tls="false") + assert build_redis_url("redis", 6379, 2) == f"redis://:{ENCODED_RESERVED_CHARS}@redis:6379/2" + + +def test_tls_without_credential_yields_rediss_url(redis_env): + redis_env(enable_tls="true") + assert build_redis_url("host", 6379, 0) == "rediss://host:6379/0?ssl_cert_reqs=none" + + +def test_credential_defaults_to_tls_url(redis_env): + redis_env(auth_token="tok") + assert build_redis_url("host", 6379, 1) == "rediss://:tok@host:6379/1?ssl_cert_reqs=none" + + +def test_no_credential_no_tls_yields_plain_url(redis_env): + redis_env() + assert build_redis_url("host", 6379, 0) == "redis://host:6379/0" + + +def test_auth_token_injected_into_credential_free_url(redis_env): + redis_env(auth_token=TOKEN_WITH_RESERVED_CHARS) + assert ( + _apply_redis_auth_token("redis://redis:6379/0") + == f"redis://:{ENCODED_RESERVED_CHARS}@redis:6379/0" + ) + + +def test_existing_credential_is_preserved(redis_env): + redis_env(auth_token="tok") + assert _apply_redis_auth_token("redis://:mine@h:6379/0") == "redis://:mine@h:6379/0" + + +def test_url_unchanged_without_auth_token(redis_env): + redis_env() + assert _apply_redis_auth_token("redis://redis:6379/0") == "redis://redis:6379/0" + + +def _host_port(url: str) -> str: + """Evaluate the cache_redis_host_port property against a stubbed URL.""" + + class _Stub: + cache_redis_url = url + + return HostedModelInferenceServiceConfig.cache_redis_host_port.fget(_Stub()) + + +@pytest.mark.parametrize( + "url,expected", + [ + ("redis://redis.url:6379/0", "redis.url:6379"), + ("rediss://redis.url:6379/0", "redis.url:6379"), + ("redis://:p%40ss@redis.url:6379/2", "redis.url:6379"), + ("rediss://:tok@redis.url:6379/0", "redis.url:6379"), + ("rediss://user:tok@cache.redis.azure.com", "cache.redis.azure.com"), + ("redis://redis:6379", "redis:6379"), + ], +) +def test_host_port_strips_credentials_for_every_scheme(url, expected): + assert _host_port(url) == expected + + +def test_host_port_never_leaks_password_into_scaler_address(redis_env): + redis_env(auth_token=TOKEN_WITH_RESERVED_CHARS) + url = _apply_redis_auth_token("redis://redis:6379/0") + host_port = _host_port(url) + assert "@" not in host_port + assert ENCODED_RESERVED_CHARS not in host_port + assert host_port == "redis:6379" + + +@pytest.fixture +def fixed_redis_host(monkeypatch): + monkeypatch.setattr( + "model_engine_server.core.celery.app.get_redis_host_port", lambda: ("h", 6379) + ) + + +def _uses_tls(client) -> bool: + return client.connection_pool.connection_class is SSLConnection + + +def test_instance_credential_without_tls(redis_env, fixed_redis_host): + redis_env(auth_token="tok", enable_tls="false") + client = get_redis_instance(1) + assert client.connection_pool.connection_kwargs["password"] == "tok" + assert not _uses_tls(client) + + +def test_instance_tls_without_credential(redis_env, fixed_redis_host): + redis_env(enable_tls="true") + client = get_redis_instance() + assert client.connection_pool.connection_kwargs.get("password") is None + assert _uses_tls(client) + + +def test_instance_credential_implies_tls_when_unset(redis_env, fixed_redis_host): + redis_env(auth_token="tok") + client = get_redis_instance() + assert client.connection_pool.connection_kwargs["password"] == "tok" + assert _uses_tls(client) + + +def test_instance_plain_when_neither_set(redis_env, fixed_redis_host): + redis_env() + client = get_redis_instance() + assert client.connection_pool.connection_kwargs.get("password") is None + assert not _uses_tls(client)