From 4d0ffce90939256bc8969294592e4a41fae9999a Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 13:40:50 +0000 Subject: [PATCH 1/3] Read secrets from Docker secrets, falling back to the environment Harvested from the security-improvements branch, which is marked WIP and pairs this with a Vault deployment. Only the Docker Secrets half is taken: it works with the compose file that exists today, needs no new services, and changes nothing for a deployment that mounts nothing. 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 on 2026-09-04. A Docker secret is a file mounted only into the containers that declare it. Two behaviours worth stating, both tested: - A mounted file beats the environment. A deployment that mounted a secret meant it; preferring a stale environment variable would make the secret look applied when it was not, and surface as an auth error somewhere else entirely. - A blank file counts as absent. init-docker-secrets on that branch creates placeholders containing a single space, and a blank password reaching Postgres fails somewhere far less obvious than here. The Vault half of the branch is deliberately not included. It reaches Postgres over a unix socket the current compose does not mount, loops forever on `hvac.exceptions.VaultDown` waiting for an unseal, switches chainlit's data layer and adds a git submodule -- and it logs the credentials Vault issues it, at warning level. That is a deployment migration to plan, not a merge. --- bin/chat-chainlit.py | 7 +++ env_template | 5 ++ src/util/secrets.py | 81 ++++++++++++++++++++++++++ tests/util/test_secrets.py | 113 +++++++++++++++++++++++++++++++++++++ 4 files changed, 206 insertions(+) create mode 100644 src/util/secrets.py create mode 100644 tests/util/test_secrets.py diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index e40bd85f..f8cf7dcc 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -22,8 +22,15 @@ 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 load_dotenv() +# Before anything reads os.environ. Docker secrets, where mounted, take +# precedence over .env; where not mounted, nothing changes. +_loaded_secrets = load_secrets_to_environ(SECRET_NAMES) +if _loaded_secrets: + logging.info(f"Loaded from Docker secrets: {', '.join(_loaded_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..7df2d7cf --- /dev/null +++ b/src/util/secrets.py @@ -0,0 +1,81 @@ +"""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 load_secrets_to_environ(names: Iterable[str]) -> list[str]: + """Copy mounted secrets into os.environ; return the names that came from a file. + + 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. + + Only file-backed values are written. A name already in the environment and + not mounted is left exactly as it is. + """ + loaded: list[str] = [] + 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 + loaded.append(name) + return loaded + + +# 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..57ea65d2 --- /dev/null +++ b/tests/util/test_secrets.py @@ -0,0 +1,113 @@ +"""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 + + +@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_reports_only_what_came_from_a_file( + mounted: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The return value is what gets logged, so it must not overstate.""" + (mounted / "TAVILY_API_KEY").write_text("tvly-secret") + (mounted / "CLOUDFLARE_SECRET_KEY").write_text(" ") # placeholder + monkeypatch.setenv("OPENAI_API_KEY", "sk-from-env") + + loaded = load_secrets_to_environ( + ["TAVILY_API_KEY", "CLOUDFLARE_SECRET_KEY", "OPENAI_API_KEY"] + ) + + assert loaded == ["TAVILY_API_KEY"] + import os + + assert os.environ["TAVILY_API_KEY"] == "tvly-secret" + assert os.environ["OPENAI_API_KEY"] == "sk-from-env", "left exactly as it was" + + +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") + assert 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}" From 0ca4661f3ad2e1cfff47776a9fd60fc354d93668 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 13:46:14 +0000 Subject: [PATCH 2/3] Split the loggable listing from the secret read CodeQL flagged the startup log as "Clear-text logging of sensitive information", and it was right to. The line logged only names, but they came back from load_secrets_to_environ -- a function that had read every secret body -- so nothing about the value said it was safe to log. Suppressing that would have been the same mistake the Vault code on security-improvements makes with logging.warning(response), which this PR's description criticises. mounted_secrets() answers "which of these were supplied" from st_size alone and never opens a file, so it is what the log is built from. load_secrets_to_environ() now returns nothing at all. --- bin/chat-chainlit.py | 10 ++++++---- src/util/secrets.py | 39 +++++++++++++++++++++++++++++------- tests/util/test_secrets.py | 41 +++++++++++++++++++++++++++++--------- 3 files changed, 70 insertions(+), 20 deletions(-) diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index f8cf7dcc..4c6d3157 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -22,14 +22,16 @@ 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 +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. -_loaded_secrets = load_secrets_to_environ(SECRET_NAMES) -if _loaded_secrets: - logging.info(f"Loaded from Docker secrets: {', '.join(_loaded_secrets)}") +_mounted = mounted_secrets(SECRET_NAMES) +load_secrets_to_environ(SECRET_NAMES) +if _mounted: + # Names only, and from a function that never reads the files. + logging.info(f"Supplied as Docker secrets: {', '.join(_mounted)}") config: Config | None = Config.from_yaml() diff --git a/src/util/secrets.py b/src/util/secrets.py index 7df2d7cf..7fa1d6ab 100644 --- a/src/util/secrets.py +++ b/src/util/secrets.py @@ -45,17 +45,44 @@ def get_secret(name: str, default: str | None = None) -> str | None: return contents or os.getenv(name, default) -def load_secrets_to_environ(names: Iterable[str]) -> list[str]: - """Copy mounted secrets into os.environ; return the names that came from a file. +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. - Only file-backed values are written. A name already in the environment and - not mounted is left exactly as it is. + 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. """ - loaded: list[str] = [] for name in names: secret_path = DOCKER_SECRETS / name try: @@ -64,8 +91,6 @@ def load_secrets_to_environ(names: Iterable[str]) -> list[str]: continue if contents: os.environ[name] = contents - loaded.append(name) - return loaded # The values worth mounting rather than passing as environment variables. Names diff --git a/tests/util/test_secrets.py b/tests/util/test_secrets.py index 57ea65d2..20dfbadf 100644 --- a/tests/util/test_secrets.py +++ b/tests/util/test_secrets.py @@ -5,7 +5,12 @@ import pytest import util.secrets as secrets -from util.secrets import SECRET_NAMES, get_secret, load_secrets_to_environ +from util.secrets import ( + SECRET_NAMES, + get_secret, + load_secrets_to_environ, + mounted_secrets, +) @pytest.fixture @@ -59,23 +64,42 @@ def test_a_missing_secret_falls_back_to_the_default( assert get_secret("NOT_SET_ANYWHERE") is None -def test_loading_reports_only_what_came_from_a_file( +def test_loading_writes_only_file_backed_values( mounted: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """The return value is what gets logged, so it must not overstate.""" + 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") - loaded = load_secrets_to_environ( + load_secrets_to_environ( ["TAVILY_API_KEY", "CLOUDFLARE_SECRET_KEY", "OPENAI_API_KEY"] ) - assert loaded == ["TAVILY_API_KEY"] - import os - 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( @@ -87,7 +111,6 @@ def test_loading_does_not_blank_an_environment_variable( value = "real-password" monkeypatch.setenv("POSTGRES_PASSWORD", value) load_secrets_to_environ(["POSTGRES_PASSWORD"]) - assert os.environ["POSTGRES_PASSWORD"] == value @@ -97,7 +120,7 @@ def test_missing_secrets_directory_is_not_an_error( """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") - assert load_secrets_to_environ(SECRET_NAMES) == [] + load_secrets_to_environ(SECRET_NAMES) assert get_secret("OPENAI_API_KEY") == "sk-from-env" From 7f272e94ab1e9b29ab871d1a6289be30cfae6858 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 13:48:24 +0000 Subject: [PATCH 3/3] Log how many secrets were mounted, not which CodeQL still flagged the startup log after splitting the stat-only listing from the read. Its clear-text-logging rule matches on the shape of the strings -- OPENAI_API_KEY, CLOUDFLARE_SECRET_KEY -- not on where they came from, so mounted_secrets() not opening a file does not settle it. The names are genuinely not secret values, but dismissing a security alert to keep a convenience is not a trade worth making, and `ls /run/secrets` answers the same question. The log now carries a count. --- bin/chat-chainlit.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index 4c6d3157..9c408065 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -30,8 +30,14 @@ _mounted = mounted_secrets(SECRET_NAMES) load_secrets_to_environ(SECRET_NAMES) if _mounted: - # Names only, and from a function that never reads the files. - logging.info(f"Supplied as Docker secrets: {', '.join(_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()