diff --git a/CHANGELOG.md b/CHANGELOG.md index 5973ccd95..54aabcf70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **An acquisition source no longer receives this host's ambient database credentials.** A contract + names the host its postgres or mysql source lives on, and libpq and mysql_scanner read + credentials from the process environment for whatever host they are handed — so a contract naming + a host of its own choosing could be authenticated with this machine's `PGPASSWORD`, `PGPASSFILE`, + `PGSERVICE` or `MYSQL_PWD`: a credential the contract was never given, belonging to a different + server. `fluid init --discover` has suppressed these since its JDBC introspection landed; the + acquisition runners had not, so the same contract fields reached the same client libraries with + the environment intact. + The duckdb runner's `ATTACH` now runs inside the same suppression window, which is shared from + `credentials/host_db_env.py` rather than copied (`build_runners` may not import `cli`). 14 + variables are covered, and `cli/discover/_jdbc_introspect` keeps its original private names as + aliases, so nothing that referenced them changes. + **This route is independent of a contract's own `connection.secretRef`** and no allowlist over + `secretRef` would close it, because the credential never passes through `secretRef` at all. The + declared DSN still carries its own credentials, so a legitimate source is unaffected and no + contract change is needed. + + ### Fixed - **An incremental cursor written on DuckDB 1.5 and read on 2.0 silently dropped or diff --git a/fluid_build/build_runners/duckdb/runner.py b/fluid_build/build_runners/duckdb/runner.py index 4d7c63e21..cf20293a4 100644 --- a/fluid_build/build_runners/duckdb/runner.py +++ b/fluid_build/build_runners/duckdb/runner.py @@ -58,6 +58,7 @@ ) from fluid_build.api.schema import SchemaFingerprint from fluid_build.api.source import AcquisitionMode +from fluid_build.credentials.host_db_env import host_credential_env_suppressed from fluid_build.providers._duckdb_compat import first_row from fluid_build.providers._duckdb_sandbox import ( DuckDBAllowlist, @@ -883,7 +884,12 @@ def _attach_external_databases(self, con: Any, ctx: RunContext) -> None: raise _SourceSetupError(str(exc)) from exc except duckdb.Error as exc: # the extension did not load raise _SourceAttachError(str(exc)) from exc - _attach_with_password_masked(con, attach_sql, conn.get("password")) + # The contract chose this host, and libpq / mysql_scanner would + # read this machine's credentials for it (see host_db_env). The + # declared DSN still carries its own, so a real source is + # unaffected. + with host_credential_env_suppressed(): + _attach_with_password_masked(con, attach_sql, conn.get("password")) elif kind == "sqlite": conn = dict(ctx.source.connection.raw) # SQLite uses a path, not a libpq-style DSN. The path comes diff --git a/fluid_build/cli/discover/_jdbc_introspect.py b/fluid_build/cli/discover/_jdbc_introspect.py index ff0fadfae..39d04be79 100644 --- a/fluid_build/cli/discover/_jdbc_introspect.py +++ b/fluid_build/cli/discover/_jdbc_introspect.py @@ -78,6 +78,11 @@ from typing import Any, Dict, Iterator, List, Optional, Set, Tuple from urllib.parse import urlparse +from fluid_build.credentials.host_db_env import ( + HOST_CREDENTIAL_ENV, + NO_CLIENT_CERT, + host_credential_env_suppressed, +) from fluid_build.providers._sql_safety import ( build_libpq_dsn, build_mysql_dsn, @@ -115,39 +120,16 @@ # MySQL client's connection defaults (MySQL 8.4 Reference Manual, "Environment # Variables"), cleared so none of them stands in for a part the DSN leaves out. # MYSQL_GROUP_SUFFIX only names extra option-file groups. DuckDB 1.5.6's -# mysql_scanner was seen to send no password from an option file (~/.my.cnf, -# ~/.mylogin.cnf, ~/my.cnf, /etc/my.cnf, /etc/mysql/my.cnf, $MYSQL_HOME/my.cnf), -# with or without that suffix, so clearing it is a precaution: there is no -# option-file fallback for it to close. -_HOST_CREDENTIAL_ENV = ( - "PGPASSWORD", - "PGPASSFILE", - "PGSERVICE", - "PGSERVICEFILE", - "PGSYSCONFDIR", - "PGSSLCERT", - "PGSSLKEY", - "PGOAUTHTOKEN", - "PGGSSDELEGATION", - "MYSQL_PWD", - "MYSQL_HOST", - "MYSQL_TCP_PORT", - "MYSQL_UNIX_PORT", - "MYSQL_GROUP_SUFFIX", -) - -# libpq skips a client certificate whose file does not exist (ENOENT or -# ENOTDIR). Pointing a pinned connection's certificate and key below os.devnull -# means it never presents this host's ~/.postgresql/postgresql.crt to a server -# that asks for one. -_NO_CLIENT_CERT = os.path.join(os.devnull, "fluid-no-client-certificate") - -# The suppression below mutates the process environment. forge_run's in-process -# worker thread can run alongside forge_from_source, so windows are serialised -# (and re-entrant within one thread) rather than interleaved: two overlapping -# windows could otherwise restore the host credentials while the second attach -# is still inside its own. -_HOST_CREDENTIAL_ENV_LOCK = threading.RLock() +# The one implementation now lives in ``credentials/host_db_env`` so the +# acquisition runners can use it too -- ``build_runners`` may not import +# ``cli``. These aliases keep this module's original private names, which its +# own tests and the patch points around them refer to. +# Only the names something actually refers to: _HOST_CREDENTIAL_ENV from this +# module's own tests, _NO_CLIENT_CERT from build_attach_dsn below. The lock +# needs no alias -- nothing outside host_db_env touches it, and it is reachable +# there as HOST_CREDENTIAL_ENV_LOCK if that ever changes. +_HOST_CREDENTIAL_ENV = HOST_CREDENTIAL_ENV +_NO_CLIENT_CERT = NO_CLIENT_CERT # The mysql_scanner quoting lives beside the libpq one in _sql_safety, which @@ -341,35 +323,7 @@ def _uri_scheme(uri: str) -> str: ) -@contextlib.contextmanager -def _host_credential_env_suppressed() -> Iterator[None]: - """Hide this host's ambient DB credential env vars for a pinned ATTACH. - - The saved values are put back when the window closes, also when the attach - raises. Windows opened through this function in different threads are - serialised by :data:`_HOST_CREDENTIAL_ENV_LOCK`, so they never interleave - their save and restore; within one thread a nested window is safe when it - closes before the window around it. - - That is all the lock covers. Code that does not go through this function - is not serialised with it, so there is no guarantee against: - - * another thread reading these variables, which sees them unset while a - window is open; - * ``os.environ.copy()`` in another thread, which can race the pop and the - restore (and raise ``KeyError``); - * an ``engine._run_sync_stage(env_snapshot=True)`` whose snapshot and - restore overlap a window, which can delete the variables for good or - bring them back inside the window; - * same-thread windows that do not close in LIFO order, where the first to - close restores the variables inside the other. - """ - with _HOST_CREDENTIAL_ENV_LOCK: - saved = {k: os.environ.pop(k) for k in _HOST_CREDENTIAL_ENV if k in os.environ} - try: - yield - finally: - os.environ.update(saved) +_host_credential_env_suppressed = host_credential_env_suppressed @dataclass diff --git a/fluid_build/credentials/host_db_env.py b/fluid_build/credentials/host_db_env.py new file mode 100644 index 000000000..500d1b426 --- /dev/null +++ b/fluid_build/credentials/host_db_env.py @@ -0,0 +1,107 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""This host's ambient database credentials, hidden for the length of one attach. + +A contract names the host its source lives on. libpq and mysql_scanner then read +credentials from the PROCESS environment for whatever host they are handed, so a +contract naming a host of its own choosing could be authenticated with this +machine's ``PGPASSWORD`` -- a credential the contract was never given, belonging +to a different server. No allowlist over the contract's own ``secretRef`` closes +that, because the credential never passes through ``secretRef`` at all. + +``fluid init --discover`` has suppressed these since its JDBC introspection +landed. The acquisition runners had not, so the same contract fields reached the +same client libraries with the environment intact. + +This is where the one implementation lives so both use it. It sits under +``credentials`` rather than beside the discover code because ``build_runners`` +may not import ``cli`` (the import-linter contract "build_runners must not +depend on cli"), and because hiding a credential is a credentials concern rather +than a discover detail. ``cli/discover/_jdbc_introspect`` re-exports these under +their original private spellings, so the tests and patch points that grew up +there keep working unchanged. +""" + +from __future__ import annotations + +import contextlib +import os +import threading +from typing import Iterator + +# mysql_scanner was seen to send no password from an option file (~/.my.cnf, +# ~/.mylogin.cnf, ~/my.cnf, /etc/my.cnf, /etc/mysql/my.cnf, $MYSQL_HOME/my.cnf), +# with or without that suffix, so clearing it is a precaution: there is no +# option-file fallback for it to close. +HOST_CREDENTIAL_ENV = ( + "PGPASSWORD", + "PGPASSFILE", + "PGSERVICE", + "PGSERVICEFILE", + "PGSYSCONFDIR", + "PGSSLCERT", + "PGSSLKEY", + "PGOAUTHTOKEN", + "PGGSSDELEGATION", + "MYSQL_PWD", + "MYSQL_HOST", + "MYSQL_TCP_PORT", + "MYSQL_UNIX_PORT", + "MYSQL_GROUP_SUFFIX", +) + +# libpq skips a client certificate whose file does not exist (ENOENT or +# ENOTDIR). Pointing a pinned connection's certificate and key below os.devnull +# means it never presents this host's ~/.postgresql/postgresql.crt to a server +# that asks for one. +NO_CLIENT_CERT = os.path.join(os.devnull, "fluid-no-client-certificate") + +# The suppression below mutates the process environment. forge_run's in-process +# worker thread can run alongside forge_from_source, so windows are serialised +# (and re-entrant within one thread) rather than interleaved: two overlapping +# windows could otherwise restore the host credentials while the second attach +# is still inside its own. +HOST_CREDENTIAL_ENV_LOCK = threading.RLock() + + +@contextlib.contextmanager +def host_credential_env_suppressed() -> Iterator[None]: + """Hide this host's ambient DB credential env vars for a pinned ATTACH. + + The saved values are put back when the window closes, also when the attach + raises. Windows opened through this function in different threads are + serialised by :data:`HOST_CREDENTIAL_ENV_LOCK`, so they never interleave + their save and restore; within one thread a nested window is safe when it + closes before the window around it. + + That is all the lock covers. Code that does not go through this function + is not serialised with it, so there is no guarantee against: + + * another thread reading these variables, which sees them unset while a + window is open; + * ``os.environ.copy()`` in another thread, which can race the pop and the + restore (and raise ``KeyError``); + * an ``engine._run_sync_stage(env_snapshot=True)`` whose snapshot and + restore overlap a window, which can delete the variables for good or + bring them back inside the window; + * same-thread windows that do not close in LIFO order, where the first to + close restores the variables inside the other. + """ + with HOST_CREDENTIAL_ENV_LOCK: + saved = {k: os.environ.pop(k) for k in HOST_CREDENTIAL_ENV if k in os.environ} + try: + yield + finally: + os.environ.update(saved) diff --git a/tests/build_runners/test_duckdb_host_credential_env.py b/tests/build_runners/test_duckdb_host_credential_env.py new file mode 100644 index 000000000..3fc21ff09 --- /dev/null +++ b/tests/build_runners/test_duckdb_host_credential_env.py @@ -0,0 +1,179 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""A contract's source host never gets this machine's ambient DB credentials. + +A contract names the host its postgres or mysql source lives on. libpq and +mysql_scanner then read credentials from the PROCESS environment for whatever +host they are handed, so a contract naming a host of its own choosing could be +authenticated with this machine's ``PGPASSWORD`` -- a credential it was never +given, belonging to a different server. + +This is a SECOND route, independent of the contract's own ``connection.secretRef``: +no allowlist over ``secretRef`` closes it, because the credential never passes +through ``secretRef`` at all. ``fluid init --discover`` has suppressed these +since its JDBC introspection landed; the acquisition runners had not. +""" + +from __future__ import annotations + +import os +from typing import Any, Dict, List + +import pytest + +from fluid_build.credentials.host_db_env import ( + HOST_CREDENTIAL_ENV, + host_credential_env_suppressed, +) + +#: A value no real credential store would hand out, so a leak is unmistakable. +SENTINEL = "SENTINEL-HOST-PASSWORD-DO-NOT-SEND" # pragma: allowlist secret + +#: What the CONTRACT declared, as opposed to what the host environment holds. +#: Named once so neither the fixture nor the assertion spells a literal that +#: reads as a credential to the secret scanner. +CONTRACT_PASSWORD = "from-the-contract" # pragma: allowlist secret + + +@pytest.mark.parametrize("name", ["PGPASSWORD", "PGPASSFILE", "PGSERVICE", "MYSQL_PWD"]) +def test_the_variables_libpq_and_mysql_read_are_covered(name: str) -> None: + """The set is the contract: a variable missing from it is a live hole.""" + assert name in HOST_CREDENTIAL_ENV, f"{name} is not suppressed" + + +def test_the_window_hides_and_restores(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("PGPASSWORD", SENTINEL) + monkeypatch.setenv("MYSQL_PWD", SENTINEL) + with host_credential_env_suppressed(): + assert "PGPASSWORD" not in os.environ + assert "MYSQL_PWD" not in os.environ + assert os.environ["PGPASSWORD"] == SENTINEL + assert os.environ["MYSQL_PWD"] == SENTINEL + + +def test_the_window_restores_when_the_attach_raises(monkeypatch: pytest.MonkeyPatch) -> None: + """A failed attach must not leave the operator's environment stripped.""" + monkeypatch.setenv("PGPASSWORD", SENTINEL) + # A flag rather than `else:` or a nested `pytest.raises`. Both of those read + # to CodeQL as unreachable code, because its analysis does not model a + # context manager's __exit__ returning true -- which is precisely the + # failure this checks for: a window that SWALLOWED the exception would + # otherwise leave the test passing with nothing asserted. + propagated = False + try: + with host_credential_env_suppressed(): + raise RuntimeError("attach failed") + except RuntimeError as exc: + propagated = True + assert "attach failed" in str(exc) + assert propagated, "the window swallowed the exception instead of re-raising" + assert os.environ["PGPASSWORD"] == SENTINEL + + +def test_an_unset_variable_is_not_invented(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("PGPASSWORD", raising=False) + with host_credential_env_suppressed(): + pass + assert "PGPASSWORD" not in os.environ + + +# ── the runner's attach, which is what this change is for ──────────────── + + +def test_the_duckdb_runner_attaches_inside_the_window( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The regression test. Reverting the `with` in the runner fails this. + + Observes the environment from inside the attach itself, which is the only + place that proves the window actually wraps the call that reaches libpq. + """ + from fluid_build.build_runners.duckdb import runner as duckdb_runner + + monkeypatch.setenv("PGPASSWORD", SENTINEL) + seen: Dict[str, Any] = {} + + def spy(_con: Any, statement: str, password: Any) -> None: + seen["pgpassword_visible_during_attach"] = os.environ.get("PGPASSWORD") + seen["statement"] = statement + seen["password"] = password + + monkeypatch.setattr(duckdb_runner, "_attach_with_password_masked", spy) + + class _Conn: + raw = { + "host": "db.example.com", + "port": 5432, + "user": "alice", + "database": "sales", + "password": CONTRACT_PASSWORD, + } + + class _Source: + kind = "postgres" + connection = _Conn() + + class _Ctx: + build_id = "ingest" + source = _Source() + + class _Con: + def execute(self, _sql: str) -> None: + return None + + runner = duckdb_runner.DuckdbRunner() + runner._attach_external_databases(_Con(), _Ctx()) + + assert ( + seen["pgpassword_visible_during_attach"] is None + ), "the runner attached with this host's PGPASSWORD still in the environment" + # Restored afterwards, and the contract's own credential was untouched. + assert os.environ["PGPASSWORD"] == SENTINEL + assert seen["password"] == CONTRACT_PASSWORD + + +def test_a_sqlite_source_needs_no_window(monkeypatch: pytest.MonkeyPatch) -> None: + """Only the libpq/mysql_scanner paths read these; sqlite takes a path. + + Pins that the change did not quietly widen to the sqlite branch, whose + confinement is a different control. + """ + from fluid_build.build_runners.duckdb import runner as duckdb_runner + + calls: List[str] = [] + + class _Con: + def execute(self, sql: str) -> None: + calls.append(sql) + + class _Conn: + raw: Dict[str, Any] = {"uri": "/nonexistent/does-not-exist.sqlite"} + + class _Source: + kind = "sqlite" + connection = _Conn() + + class _Ctx: + build_id = "ingest" + source = _Source() + contract: Dict[str, Any] = {} + workdir = "." + + runner = duckdb_runner.DuckdbRunner() + # The path is confined before any attach, so an outside path is refused + # here; what matters is that it does not reach the credential window. + with pytest.raises(Exception): + runner._attach_external_databases(_Con(), _Ctx()) + assert not any("ATTACH" in c for c in calls), calls