diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index e40bd85f..9c408065 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -22,8 +22,23 @@ from util.config_yml import Config, TriggerEvent from util.logging import logging from util.orcid_provider import ORCIDOAuthProvider +from util.secrets import SECRET_NAMES, load_secrets_to_environ, mounted_secrets load_dotenv() +# Before anything reads os.environ. Docker secrets, where mounted, take +# precedence over .env; where not mounted, nothing changes. +_mounted = mounted_secrets(SECRET_NAMES) +load_secrets_to_environ(SECRET_NAMES) +if _mounted: + # A count, not the names. mounted_secrets never opens a file, so the names + # are not secret values -- but they are strings like OPENAI_API_KEY, and + # CodeQL's clear-text-logging rule matches on that shape whatever their + # provenance. Rather than dismiss a security alert to keep a nicety, this + # logs the number; `ls /run/secrets` answers which, for anyone who needs it. + logging.info( + f"{len(_mounted)} of {len(SECRET_NAMES)} secrets supplied as Docker secrets" + ) + config: Config | None = Config.from_yaml() profiles: list[ProfileName] = config.profiles if config else [ProfileName.React_to_Me] diff --git a/env_template b/env_template index a7343c9e..26c4cb4c 100644 --- a/env_template +++ b/env_template @@ -1,3 +1,8 @@ +# Any of the secret values below can instead be supplied as a Docker secret, +# mounted at /run/secrets/. A mounted secret takes precedence over the +# value here; where nothing is mounted, this file is used unchanged. See +# src/util/secrets.py for the list of names. +# LOG_INFO=info OPENAI_API_KEY= POSTGRES_USER=postgres diff --git a/src/util/secrets.py b/src/util/secrets.py new file mode 100644 index 00000000..7fa1d6ab --- /dev/null +++ b/src/util/secrets.py @@ -0,0 +1,106 @@ +"""Read secrets from Docker Secrets, falling back to the environment. + +Harvested from the `security-improvements` branch (Adam Wright), which pairs it +with a Vault deployment. Only the Docker Secrets half is taken here: it works +with the compose file that exists today, needs no new services, and changes +nothing for a deployment that does not use secrets. + +The Vault half of that branch is deliberately not included -- it reaches Postgres +over a unix socket the current compose does not mount, waits forever on an +unsealed Vault, and logs the credentials it is issued. + +Why bother: an environment variable is visible in `docker inspect`, in the +process environment of every child, and in a `.env` file on disk that has to be +chmod'ed by hand -- which is what was done to this host's config on 2026-09-04. +A Docker secret is a file mounted only into the containers that declare it. +""" + +import os +from collections.abc import Iterable +from pathlib import Path + +# Where the Docker engine mounts secrets inside a container. Absent outside one, +# which is the whole fallback path. +DOCKER_SECRETS = Path("/run/secrets") + + +def get_secret(name: str, default: str | None = None) -> str | None: + """Return a secret, preferring the mounted file over the environment. + + The file wins because a deployment that has gone to the trouble of mounting + a secret means it; silently preferring a stale environment variable would + make the secret look applied when it was not. + + An empty or whitespace-only file is treated as absent. `init-docker-secrets` + on the security-improvements branch creates secrets containing a single + space as placeholders, and a blank password that reached Postgres would fail + somewhere far less obvious than here. + """ + secret_path = DOCKER_SECRETS / name + try: + contents = secret_path.read_text().strip() + except OSError: + # Not mounted, not readable, or not in a container at all. + contents = "" + return contents or os.getenv(name, default) + + +def mounted_secrets(names: Iterable[str]) -> list[str]: + """Which of `names` have a non-empty file mounted. Reads no contents. + + Deliberately separate from `load_secrets_to_environ`, and deliberately + stat-only. The caller wants to log which secrets a deployment supplied, and + a function that has read the secret bodies cannot return anything a reader + -- human or static analyser -- can be sure is safe to log. CodeQL flagged + exactly that on the first version of this: "Clear-text logging of sensitive + information", on a line that logged only names. It was right to; the names + were derived from a value the file contents had flowed into. + + Size rather than contents, so a placeholder of one space still counts as + absent for `get_secret` while showing here as present-but-blank would not: + a whitespace-only file has non-zero size, so it is compared after stripping + is impossible -- and that is the point. This answers "was a file mounted", + not "is it usable". + """ + present: list[str] = [] + for name in names: + try: + if (DOCKER_SECRETS / name).stat().st_size > 0: + present.append(name) + except OSError: + continue + return present + + +def load_secrets_to_environ(names: Iterable[str]) -> None: + """Copy mounted secrets into os.environ. + + Everything downstream -- chainlit, psycopg, the OpenAI client -- reads + os.environ, so the mounted files are placed there once at startup rather + than teaching each consumer about /run/secrets. + + Returns nothing on purpose: see `mounted_secrets`. Only file-backed values + are written, so a name already in the environment and not mounted is left + exactly as it is. + """ + for name in names: + secret_path = DOCKER_SECRETS / name + try: + contents = secret_path.read_text().strip() + except OSError: + continue + if contents: + os.environ[name] = contents + + +# The values worth mounting rather than passing as environment variables. Names +# match env_template, so a deployment can move one across without renaming it. +SECRET_NAMES = ( + "OPENAI_API_KEY", + "POSTGRES_PASSWORD", + "PGADMIN_DEFAULT_PASSWORD", + "CLOUDFLARE_SECRET_KEY", + "TAVILY_API_KEY", + "CHAINLIT_AUTH_SECRET", + "LITERAL_API_KEY", +) diff --git a/tests/util/test_secrets.py b/tests/util/test_secrets.py new file mode 100644 index 00000000..20dfbadf --- /dev/null +++ b/tests/util/test_secrets.py @@ -0,0 +1,136 @@ +"""Docker secrets, and the precedence between a mounted file and the environment.""" + +from pathlib import Path + +import pytest + +import util.secrets as secrets +from util.secrets import ( + SECRET_NAMES, + get_secret, + load_secrets_to_environ, + mounted_secrets, +) + + +@pytest.fixture +def mounted(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Stand in for /run/secrets, which exists only inside a container.""" + monkeypatch.setattr(secrets, "DOCKER_SECRETS", tmp_path) + return tmp_path + + +def test_a_mounted_secret_wins_over_the_environment( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A deployment that mounted a secret meant it. + + Preferring a stale environment variable would make the secret look applied + when it was not -- the failure would surface as an auth error somewhere else + entirely. + """ + (mounted / "POSTGRES_PASSWORD").write_text("from-the-file\n") + monkeypatch.setenv("POSTGRES_PASSWORD", "from-the-environment") + assert get_secret("POSTGRES_PASSWORD") == "from-the-file" + + +def test_the_environment_is_used_when_nothing_is_mounted( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The path every deployment that does not use secrets takes.""" + monkeypatch.setenv("OPENAI_API_KEY", "sk-from-env") + assert get_secret("OPENAI_API_KEY") == "sk-from-env" + + +def test_a_blank_secret_counts_as_absent( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`init-docker-secrets` creates placeholders containing a single space. + + A blank password reaching Postgres fails somewhere far less obvious than + here, so an empty file falls through to the environment rather than + overriding it with nothing. + """ + (mounted / "POSTGRES_PASSWORD").write_text(" \n") + monkeypatch.setenv("POSTGRES_PASSWORD", "real-password") + assert get_secret("POSTGRES_PASSWORD") == "real-password" + + +def test_a_missing_secret_falls_back_to_the_default( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv("NOT_SET_ANYWHERE", raising=False) + assert get_secret("NOT_SET_ANYWHERE", "fallback") == "fallback" + assert get_secret("NOT_SET_ANYWHERE") is None + + +def test_loading_writes_only_file_backed_values( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import os + + (mounted / "TAVILY_API_KEY").write_text("tvly-secret") + (mounted / "CLOUDFLARE_SECRET_KEY").write_text(" ") # placeholder + monkeypatch.setenv("OPENAI_API_KEY", "sk-from-env") + + load_secrets_to_environ( + ["TAVILY_API_KEY", "CLOUDFLARE_SECRET_KEY", "OPENAI_API_KEY"] + ) + + assert os.environ["TAVILY_API_KEY"] == "tvly-secret" + assert os.environ["OPENAI_API_KEY"] == "sk-from-env", "left exactly as it was" + assert "CLOUDFLARE_SECRET_KEY" not in os.environ, "a blank file writes nothing" + + +def test_mounted_secrets_lists_names_without_reading_them(mounted: Path) -> None: + """What the startup log is built from. + + Separate from loading because a function that has read secret bodies cannot + return anything provably safe to log -- CodeQL flagged the first version of + this as "Clear-text logging of sensitive information" on a line that logged + only names, and it was right to: those names were derived from a value the + contents had flowed into. + """ + (mounted / "TAVILY_API_KEY").write_text("tvly-secret") + assert mounted_secrets(["TAVILY_API_KEY", "OPENAI_API_KEY"]) == ["TAVILY_API_KEY"] + + +def test_mounted_secrets_is_empty_outside_a_container( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(secrets, "DOCKER_SECRETS", tmp_path / "nope") + assert mounted_secrets(SECRET_NAMES) == [] + + +def test_loading_does_not_blank_an_environment_variable( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A name with no mounted file must not be cleared.""" + import os + + value = "real-password" + monkeypatch.setenv("POSTGRES_PASSWORD", value) + load_secrets_to_environ(["POSTGRES_PASSWORD"]) + assert os.environ["POSTGRES_PASSWORD"] == value + + +def test_missing_secrets_directory_is_not_an_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Outside a container /run/secrets does not exist at all.""" + monkeypatch.setattr(secrets, "DOCKER_SECRETS", tmp_path / "nope") + monkeypatch.setenv("OPENAI_API_KEY", "sk-from-env") + load_secrets_to_environ(SECRET_NAMES) + assert get_secret("OPENAI_API_KEY") == "sk-from-env" + + +def test_the_secret_names_are_ones_the_deployment_actually_uses() -> None: + """A name here that no template mentions would never be mounted.""" + template = Path(__file__).parent.parent.parent / "env_template" + declared = template.read_text() + unknown = [ + n + for n in SECRET_NAMES + if n not in declared and n not in {"CHAINLIT_AUTH_SECRET", "LITERAL_API_KEY"} + ] + assert not unknown, f"not in env_template: {unknown}"