From dd1aeaa1757abc00c8e7d877ca0f7082243d5f74 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 00:54:50 +0100 Subject: [PATCH 1/7] fix(duckdb): a remote discover could not read the host's CA bundle on 2.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fluid init --discover s3://…` and `--discover https://…` failed on duckdb 2.0.0 before a byte was read: PermissionException: Cannot read a CA certificate bundle: file system access is restricted by configuration. Add one of /etc/ssl/certs/ca-certificates.crt, … to allowed_paths, point ca_cert_file at an allowed file, or disable server certificate verification TLS needs the host's trust store, the sandbox granted no path to it, and nothing granted one when a remote prefix was declared — so the whole remote-discovery surface was broken on the engine the project resolves to by default, while the same call worked on 1.5.x. The cause is upstream and deliberate: duckdb/duckdb-httpfs#474 moved the bundle read inside DuckDB's own file system (`CURLOPT_CAINFO_BLOB`, an in-memory store for httplib), where curl and OpenSSL used to open it themselves and bypassed `allowed_paths` entirely. `_confine_connection` now appends one CA bundle to `allowed_paths` — the first of the five DuckDB probes that exists — and only when `allow.remote_prefixes` is non-empty. One file, never a directory, so `allowed_directories` is untouched and what a remote prefix grants locally is unchanged (#757). The grant is applied at SET time and not folded into `allow`, so refusal hints and the read-back still speak only of what the call site declared. The candidate list mirrors DuckDB's own, in DuckDB's order, rather than borrowing `ssl.get_default_verify_paths()` or `certifi` the way sethmlarson/truststore picks a store for OpenSSL: the grant has to name the file DuckDB will open, and DuckDB consults neither OpenSSL's defaults nor `SSL_CERT_FILE`. certifi would also add a dependency the base install does not ship. bergaaberg/dovre-packs#62 grants the system bundle into `allowed_paths` the same way. Measured, not assumed: * Certificate verification is untouched and stays on. An expired and a self-signed certificate are refused on 1.5.6 and 2.0.0 alike, with and without `enable_server_cert_verification` — that setting governs the httplib client while s3 and https go through curl, whose own flag defaults to on (duckdb/duckdb-httpfs#378 unified the two). Nothing here disables verification or pins `ca_cert_file`. * Plain `http://` never needed the grant: duckdb/duckdb-httpfs#499 made a failed bundle load non-fatal there, and 2.0.0 carries it. * Live, through the CLI: `--discover s3://noaa-ghcn-pds/csv/by_year/2024.csv` emits 8 columns and `--discover …/customer.csv` emits 18, on 1.5.1, 1.5.6 and 2.0.0; `fluid validate` accepts the result. The 9 new tests are offline. Each mechanism was reverted and the failures recorded: returning `None` fails 3, returning the bundle's directory fails 4, dropping the remote-prefix guard fails 1. On 2.0.0 the refusal arrives before any connection is attempted, so one test parses DuckDB's own candidate list out of its refusal and fails if upstream reorders it. The file is registered in the `duckdb-floor` CI lane, which `test_ci_duckdb_floor` requires of any test that branches on the engine version — it caught the omission. Trello: https://trello.com/c/C2fT4zj1 (P0) --- .github/workflows/ci.yml | 1 + CHANGELOG.md | 29 ++ fluid_build/providers/_duckdb_sandbox.py | 72 ++++- .../test_duckdb_sandbox_ca_bundle.py | 294 ++++++++++++++++++ 4 files changed, 393 insertions(+), 3 deletions(-) create mode 100644 tests/providers/test_duckdb_sandbox_ca_bundle.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e6971825..2dea548f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -683,6 +683,7 @@ jobs: tests/providers/test_duckdb_compat.py \ tests/providers/test_duckdb_sandbox.py \ tests/providers/test_duckdb_sandbox_workspace_root.py \ + tests/providers/test_duckdb_sandbox_ca_bundle.py \ tests/build_runners/test_duckdb_cursor_canonical.py \ tests/build_runners/test_schema_evolution_type_family.py \ tests/cli/discover/test_discover_type_canonicalization.py \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 91fab6e1..cde3baea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the Command Center gate still lets a credential reach any public `https` endpoint unlisted. Mirroring them would have reproduced the hole. +### Fixed + +- **`fluid init --discover` against a remote URL failed on DuckDB 2.0.0**, before a byte was read: + `PermissionException: Cannot read a CA certificate bundle: file system access is restricted by + configuration`. TLS needs the host's trust store, the sandbox granted no path to it, and nothing + granted one when a remote prefix was declared — so the whole remote-discovery surface was broken + on the engine the project resolves to by default, while the same call worked on 1.5.x. The cause + is upstream and deliberate: duckdb/duckdb-httpfs#474 moved the bundle read inside DuckDB's own + file system, where curl and OpenSSL used to open it themselves and bypassed `allowed_paths` + entirely. + + The sandbox now grants the first CA bundle that exists out of the five DuckDB probes, as **one + file** in `allowed_paths`, and only when the allowlist carries a remote prefix. A local read + gains nothing, `allowed_directories` is untouched — so what a remote prefix grants locally is + unchanged — and the grant is not folded into the allowlist, so refusal messages still name only + what the contract declared. On a host with none of the five (a minimal container, Windows) + nothing is granted and DuckDB's own refusal, which names the paths it looked for, is left to + speak. + + Certificate verification is untouched and stays on: an expired and a self-signed certificate are + refused on 1.5.6 and 2.0.0 alike, with and without `enable_server_cert_verification` — that + setting governs the httplib client, while `s3://` and `https://` go through curl, whose own flag + defaults to on (duckdb/duckdb-httpfs#378 unified the two). Nothing here disables verification or + pins `ca_cert_file`. + + Verified by running the CLI: `fluid init --discover s3://noaa-ghcn-pds/csv/by_year/2024.csv` + emits 8 columns and `--discover https://…/customer.csv` emits 18, on 1.5.1, 1.5.6 and 2.0.0, + and `fluid validate` accepts the result. + ### Upgrade notes - **A contract whose orchestrator is on a public host now fails until the operator allows it.** diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index 62a10ab9..269caab0 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -34,8 +34,9 @@ false``, so a query cannot pull an extension in afterwards; 4. ``temp_directory`` (a directory this helper creates and owns, see below), ``home_directory`` (pinned to ``$HOME``, see below), then - ``allowed_directories`` and ``allowed_paths``: only what the call site - declares it reads or writes; + ``allowed_directories`` and ``allowed_paths``: what the call site declares + it reads or writes, plus -- only when it declared a remote prefix -- the one + host CA bundle file DuckDB's TLS needs (:func:`_ca_bundle_for_remote`); 5. ``enable_external_access = false``; 6. ``lock_configuration = true``, so the SQL cannot ``SET`` any of it back, then a read-back that the grants are the ones the call site asked for. @@ -53,6 +54,25 @@ refused on 1.5.0; see ``tests/providers/test_duckdb_sandbox.py``). :data:`MIN_DUCKDB_VERSION` is therefore 1.5.0, and a connection refuses to open on anything older. +* A remote read needs the host's CA bundle, and from DuckDB 2.0 that read goes + through the sandbox. duckdb/duckdb-httpfs#474 moved it inside DuckDB's file + system (``CURLOPT_CAINFO_BLOB``, an in-memory store for httplib) where curl + and OpenSSL used to open it themselves, bypassing ``allowed_paths`` + entirely. So on 1.5.x a remote read works with no CA grant, and on 2.0.0 the + same read fails before a byte moves with ``Cannot read a CA certificate + bundle``. :func:`_ca_bundle_for_remote` grants it: one file, only when the + allowlist carries a remote prefix. Measured, not assumed: + ``fluid init --discover s3://noaa-ghcn-pds/...`` emitted 8 columns on 1.5.6 + and exited 1 on 2.0.0, measured at ``5f2ec50b`` (and on clean ``7cf5471a`` + when the card was filed); the grant makes 2.0.0 emit the same 8. + Two things this does NOT do, because neither is needed: it does not set + ``ca_cert_file`` (the probe mirrors DuckDB's own order, so DuckDB opens the + file that was granted), and it touches no verification setting -- an expired + and a self-signed certificate are already refused on both engines, with and + without ``enable_server_cert_verification``, which is the httplib flag while + s3 and https go through curl (duckdb/duckdb-httpfs#378 unified the two). + Plain ``http://`` never needed the grant: duckdb/duckdb-httpfs#499 made a + failed bundle load non-fatal there, and 2.0.0 carries it. * DuckDB folds its ``temp_directory`` into ``allowed_directories``, and its default is relative: ``.tmp`` in the working directory, or ``.tmp`` beside a file database. Both sit where the contract's repository can place a @@ -1008,6 +1028,47 @@ def _refuse_escaping_remote_prefixes(allow: DuckDBAllowlist) -> None: ) +#: The CA bundles DuckDB's httpfs probes, in the order its own refusal names +#: them ("Add one of ... to allowed_paths"). Mirrored from DuckDB rather than +#: borrowed from ``ssl.get_default_verify_paths()`` or ``certifi``, the way +#: sethmlarson/truststore picks a trust store for OpenSSL: the grant has to +#: name the file DuckDB will open, and DuckDB probes these five itself, +#: consulting neither OpenSSL's defaults nor ``SSL_CERT_FILE``. Granting +#: certifi's bundle would also add a dependency the base install does not ship. +#: ``tests/providers/test_duckdb_sandbox_ca_bundle.py`` parses the list back out +#: of DuckDB's refusal and fails if upstream's order or contents drift. +_CA_BUNDLE_CANDIDATES = ( + "/etc/ssl/certs/ca-certificates.crt", + "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", + "/etc/pki/tls/certs/ca-bundle.crt", + "/etc/ssl/ca-bundle.pem", + "/etc/ssl/cert.pem", +) + + +def _ca_bundle_for_remote(allow: DuckDBAllowlist) -> Optional[str]: + """The one CA bundle to grant for ``allow``'s remote prefixes, if any. + + ``None`` when the call site declared no remote prefix -- a purely local + read must not gain a grant it has no use for -- and ``None`` when the host + has none of DuckDB's candidates, which is a minimal container or Windows. + Granting nothing there is what this already did: DuckDB then raises its own + refusal naming the paths it looked for, which is more accurate than + anything this could say. + + A directory is never returned. ``allowed_paths`` grants one file, so this + cannot widen what :func:`_refuse_ungranted_dirs` reads back from + ``allowed_directories`` -- including the remote prefixes that are + load-bearing on 1.5.x (see :func:`_escaping_remote_prefix`). + """ + if not allow.remote_prefixes: + return None + for candidate in _CA_BUNDLE_CANDIDATES: + if os.path.isfile(candidate): + return candidate + return None + + def _refuse_ungranted_dirs(con: Any, allow: DuckDBAllowlist, spill: str) -> None: """Refuse the connection if DuckDB granted a directory ``allow`` does not name. @@ -1191,7 +1252,12 @@ def _confine_connection( _refuse_escaping_remote_prefixes(allow) directories = [_with_sep(d) for d in allow.dirs] + list(allow.remote_prefixes) con.execute(f"SET allowed_directories = {_sql_list(directories)}") - con.execute(f"SET allowed_paths = {_sql_list(allow.paths)}") + # The CA bundle is appended here and NOT folded into ``allow``: the + # allowlist stays what the call site declared, so a refusal hint and the + # read-back below speak only of the caller's own grants. + bundle = _ca_bundle_for_remote(allow) + paths = list(allow.paths) + ([bundle] if bundle is not None else []) + con.execute(f"SET allowed_paths = {_sql_list(paths)}") con.execute("SET enable_external_access = false") con.execute("SET lock_configuration = true") _refuse_ungranted_dirs(con, allow, spill) diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py new file mode 100644 index 00000000..7725023c --- /dev/null +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -0,0 +1,294 @@ +# 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 declared remote prefix grants the one CA bundle its TLS needs, and nothing else. + +On DuckDB 2.0.0 every remote read failed before a byte moved:: + + PermissionException: Cannot read a CA certificate bundle: file system access + is restricted by configuration. Add one of /etc/ssl/certs/ca-certificates.crt, + ... to allowed_paths, point ca_cert_file at an allowed file, or disable + server certificate verification + +duckdb/duckdb-httpfs#474 moved the bundle read inside DuckDB's file system +(``CURLOPT_CAINFO_BLOB``, an in-memory store for httplib) where curl and OpenSSL +used to open it themselves, bypassing ``allowed_paths`` entirely -- so the same +read works with no grant on 1.5.x and is refused on 2.0.0. +``fluid init --discover s3://noaa-ghcn-pds/csv/by_year/2024.csv`` emitted a +contract with 8 columns on 1.5.6 and exited 1 on 2.0.0, measured on clean main +(Trello card C2fT4zj1, P0). + +What the grant must not become is the reason this file exists. It is one FILE in +``allowed_paths``, so it cannot widen ``allowed_directories`` -- where the remote +prefixes that are load-bearing on 1.5.x live (#757) -- and it is added only when +the call site declared a remote prefix, so a local read gains nothing. The +allowlist object is left alone: the grant is appended at ``SET`` time, so a +refusal hint still speaks only of what the caller asked for. + +Everything here is offline. The real network proof is the CLI run in the PR +description; what CI can check without a network is the read-back, and on 2.0.0 +the refusal itself, which arrives before any connection is attempted. + +Each mechanism was reverted on duckdb 2.0.0 and the failures recorded, not +predicted. ``_ca_bundle_for_remote`` returning ``None`` fails +``test_a_declared_remote_prefix_grants_one_ca_bundle``, +``test_duckdb_can_read_the_bundle_it_was_granted`` and +``test_the_granted_bundle_is_the_first_candidate_that_exists``. Returning +``dirname(bundle)`` fails those three and +``test_the_grant_is_one_file_and_not_a_directory`` -- which it did NOT, until +that test was made to check the granted entry itself: a directory handed to +``allowed_paths`` never reaches ``allowed_directories``, so reading the latter +back could not see it. Dropping the ``allow.remote_prefixes`` guard fails +``test_a_local_read_grants_no_ca_bundle`` alone. +""" + +from __future__ import annotations + +import os +import re +import sys +from pathlib import Path +from typing import Any, List + +import pytest + +duckdb = pytest.importorskip("duckdb") + +from fluid_build.providers._duckdb_sandbox import ( # noqa: E402 + DuckDBAllowlist, + duckdb_version, + sandbox_refusal_hint, + secure_duckdb_connect, +) + +# The probe is the sibling suite's, not a copy: a second one that drifted would +# quietly weaken whichever file still held the old spelling. +from tests.providers.test_duckdb_sandbox_spill import _setting # noqa: E402 + +sandbox = sys.modules[DuckDBAllowlist.__module__] + +#: A public bucket, used for its TEXT only. Nothing here opens a connection to +#: it: declaring a prefix is what puts the sandbox in its remote shape. +REMOTE = "s3://noaa-ghcn-pds/csv/by_year/2024.csv" + +#: An https URL on a port nothing listens on. On 2.0.0 the CA refusal lands +#: before the connection is attempted, which is what makes the drift guard +#: below work with no network. +UNREACHABLE = "https://127.0.0.1:1/nothing.csv" + + +def _granted_paths(con: Any) -> List[str]: + """``allowed_paths`` in the form DuckDB reports it back. + + DuckDB resolves each entry, so a grant of ``/etc/ssl/cert.pem`` is read + back as ``/private/etc/ssl/cert.pem`` on macOS, where ``/etc`` is a link. + The case fold is the production helper rather than a second spelling of it: + whether the comparison is case-sensitive is the sandbox's own judgement. + """ + return [sandbox._fold(os.path.realpath(p)) for p in _setting(con, "allowed_paths")] + + +def _host_bundle() -> str: + """The candidate this host actually has, or skip.""" + for candidate in sandbox._CA_BUNDLE_CANDIDATES: + if os.path.isfile(candidate): + return candidate + pytest.skip("no CA bundle among DuckDB's candidates on this host") + + +# ── the defect ─────────────────────────────────────────────────────────── + + +def test_a_declared_remote_prefix_grants_one_ca_bundle() -> None: + """The grant that makes a remote read possible on 2.0.0. + + Exactly one entry, and it is a bundle DuckDB named: before the fix + ``allowed_paths`` was empty here and the read raised PermissionException. + """ + bundle = _host_bundle() + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + try: + assert _granted_paths(con) == [sandbox._fold(os.path.realpath(bundle))] + finally: + con.close() + + +def test_duckdb_can_read_the_bundle_it_was_granted() -> None: + """The grant is the file DuckDB opens, not merely a path it tolerates. + + Reading the bundle as text through the sandbox proves the entry is + effective -- the probe mirrors DuckDB's own order, so the file granted is + the one its TLS picks up. The read of an UNGRANTED candidate-shaped path is + the control: the grant is one file, not its directory. + """ + bundle = _host_bundle() + neighbour = os.path.join(os.path.dirname(bundle), "fluid-not-granted.pem") + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + try: + rows = con.execute(f"SELECT count(*) FROM read_text('{bundle}')").fetchall() + assert rows[0][0] == 1 + with pytest.raises(duckdb.Error): + con.execute(f"SELECT count(*) FROM read_text('{neighbour}')").fetchall() + finally: + con.close() + + +# ── the grant stays narrow ─────────────────────────────────────────────── + + +def test_a_local_read_grants_no_ca_bundle(tmp_path: Path) -> None: + """No remote prefix declared, so no CA path appears. + + A local read has no TLS to do, and a grant it cannot use is a grant that + should not be there. + """ + local = tmp_path / "data.csv" + local.write_text("a,b\n1,2\n", encoding="utf-8") + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(str(local))) + try: + assert _granted_paths(con) == [sandbox._fold(os.path.realpath(str(local)))] + finally: + con.close() + + +def test_the_grant_is_one_file_and_not_a_directory() -> None: + """``allowed_directories`` is untouched, so #757's prefix grant is unaffected. + + A CA *directory* would be a local grant bolted onto a remote declaration, + which is the shape that made ``s3://etc/`` dangerous in the first place. + """ + _host_bundle() + allow = DuckDBAllowlist.none().with_locations(REMOTE) + + con = secure_duckdb_connect(":memory:", allow=allow) + try: + # Each granted path is a FILE. Asserted on the entry itself and not + # only on ``allowed_directories``: a directory handed to + # ``allowed_paths`` stays in ``allowed_paths``, so the read-back below + # cannot see it. Measured -- granting ``dirname(bundle)`` left the + # directory assertion green. + for entry in _setting(con, "allowed_paths"): + assert os.path.isfile(str(entry)), entry + directories = [str(d) for d in _setting(con, "allowed_directories")] + # Matched on the bucket, not on ``s3://``: 1.5.x reports the prefix as + # ``/s3:/noaa-ghcn-pds/csv/by_year/`` and 2.0.0 verbatim, which is + # the difference #757 turns on. + assert any("noaa-ghcn-pds" in entry for entry in directories), directories + assert not any("ssl" in entry or "ca-trust" in entry for entry in directories), directories + finally: + con.close() + + +def test_the_allowlist_the_caller_passed_is_not_widened() -> None: + """The grant is appended at ``SET`` time, not folded into ``allow``. + + So a refusal hint lists what the contract declared and not a trust store + the operator never mentioned. + """ + _host_bundle() + allow = DuckDBAllowlist.none().with_locations(REMOTE) + + con = secure_duckdb_connect(":memory:", allow=allow) + try: + assert allow.paths == () + assert "ssl" not in sandbox_refusal_hint(allow) + finally: + con.close() + + +# ── which candidate, and in whose order ────────────────────────────────── + + +def test_the_granted_bundle_is_the_first_candidate_that_exists( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Probed in DuckDB's order, so the grant names the file DuckDB opens. + + Two candidates present: the earlier one wins. A host that has both + ``/etc/ssl/certs/ca-certificates.crt`` and ``/etc/ssl/cert.pem`` is + ordinary on Debian, and granting the later one would leave the read + refused on the file DuckDB actually reaches for. + """ + present = { + sandbox._CA_BUNDLE_CANDIDATES[0], + sandbox._CA_BUNDLE_CANDIDATES[-1], + } + monkeypatch.setattr(sandbox.os.path, "isfile", lambda p: p in present) + + allow = DuckDBAllowlist.none().with_locations(REMOTE) + assert sandbox._ca_bundle_for_remote(allow) == sandbox._CA_BUNDLE_CANDIDATES[0] + + +def test_a_host_with_no_candidate_grants_nothing(monkeypatch: pytest.MonkeyPatch) -> None: + """A minimal container or Windows: DuckDB's own refusal is the better message. + + Nothing is invented, and the connection still opens -- the failure, if the + read is attempted, is DuckDB's, naming the paths it looked for. + """ + monkeypatch.setattr(sandbox.os.path, "isfile", lambda _p: False) + + allow = DuckDBAllowlist.none().with_locations(REMOTE) + assert sandbox._ca_bundle_for_remote(allow) is None + + con = secure_duckdb_connect(":memory:", allow=allow) + try: + assert _granted_paths(con) == [] + finally: + con.close() + + +def test_every_candidate_is_an_absolute_file_path() -> None: + """A relative entry would be refused by DuckDB and a directory would widen the grant.""" + assert sandbox._CA_BUNDLE_CANDIDATES + for candidate in sandbox._CA_BUNDLE_CANDIDATES: + assert os.path.isabs(candidate), candidate + assert not candidate.endswith(("/", os.sep)), candidate + assert not os.path.isdir(candidate), candidate + + +def test_the_candidate_list_matches_the_one_duckdb_names(monkeypatch: pytest.MonkeyPatch) -> None: + """The drift guard: upstream's list parsed back out of its own refusal. + + DuckDB names its candidates in the refusal, in probe order, and on 2.0.0 + that refusal arrives before any connection is attempted -- so this needs no + network. If duckdb-httpfs adds, drops or reorders a path, this fails with + both lists in the message instead of the fix silently granting a file the + engine no longer reads. + + Skipped where the refusal does not exist: on 1.5.x curl opens the bundle + itself, outside the sandbox, so the read fails on the connection instead. + """ + if duckdb_version(duckdb) < (2, 0, 0): + pytest.skip("the CA bundle is read outside the sandbox before 2.0") + monkeypatch.setattr(sandbox, "_ca_bundle_for_remote", lambda _allow: None) + + con = secure_duckdb_connect( + ":memory:", + allow=DuckDBAllowlist.none().with_locations(UNREACHABLE), + extensions=["httpfs"], + ) + try: + with pytest.raises(duckdb.Error) as excinfo: + con.execute(f"SELECT * FROM read_csv_auto('{UNREACHABLE}') LIMIT 0") + finally: + con.close() + + message = str(excinfo.value) + if "CA certificate bundle" not in message: + pytest.skip(f"no CA refusal from this build: {message.splitlines()[0]}") + named = re.findall(r"/[\w./-]+\.(?:crt|pem)", message) + assert named == list(sandbox._CA_BUNDLE_CANDIDATES), (named, sandbox._CA_BUNDLE_CANDIDATES) From 9e817d0aba8c11b89ebc44d230c13c98797f5d48 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 01:15:04 +0100 Subject: [PATCH 2/7] fix(duckdb): the refusal hint owns up to the CA grant, and the probe test stops patching os MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the defect scan of the previous commit, both introduced by it. The refusal hint told the user that contract SQL "may only read and write the locations the contract declares and its own directory". With a remote prefix declared that is now false: the trust store is readable too, which the commit's own `test_duckdb_can_read_the_bundle_it_was_granted` proves. The hint now names the bundle when one is granted, and says nothing about it otherwise, where it would be noise on every local refusal. The allowlist object is still left untouched — it is what every other caller reads as the declared-inputs list. `test_the_granted_bundle_is_the_first_candidate_that_exists` and `test_a_host_with_no_candidate_grants_nothing` patched `sandbox.os.path.isfile`, and `sandbox.os` IS the stdlib module, so that replaced `posixpath.isfile` for pytest and everything else running while a real connection was opened. They now inject `_CA_BUNDLE_CANDIDATES` with real files under tmp_path, which exercises the production probe instead of stubbing it out, and pins that an absent candidate ahead of an existing one is skipped. A new test pins the same for a directory and a dangling symlink at a candidate path — `os.path.isfile` answers False for both, and the fix depends on that. Also corrected the module docstring's http clause: plain `http://` did not NEED the grant (duckdb/duckdb-httpfs#499 made a failed load non-fatal there) but it gets one anyway, because the declared prefix is what the grant keys on and an http base URL that redirects to https still verifies. 11 tests on 2.0.0, 10 + 1 skip on 1.5.6 and 1.5.1; the five sandbox suites pass together (174) but for the two pre-existing workspace-root failures. --- fluid_build/providers/_duckdb_sandbox.py | 17 +++- .../test_duckdb_sandbox_ca_bundle.py | 77 +++++++++++++++---- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index 269caab0..5fbc3c91 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -71,8 +71,11 @@ and a self-signed certificate are already refused on both engines, with and without ``enable_server_cert_verification``, which is the httplib flag while s3 and https go through curl (duckdb/duckdb-httpfs#378 unified the two). - Plain ``http://`` never needed the grant: duckdb/duckdb-httpfs#499 made a - failed bundle load non-fatal there, and 2.0.0 carries it. + Plain ``http://`` did not need the grant -- duckdb/duckdb-httpfs#499 made a + failed bundle load non-fatal there, and 2.0.0 carries it -- but it gets one + anyway, because the prefix is what is declared and an http base URL that + redirects to https still verifies (which is #499's own reason for loading the + bundle on that path). * DuckDB folds its ``temp_directory`` into ``allowed_directories``, and its default is relative: ``.tmp`` in the working directory, or ``.tmp`` beside a file database. Both sit where the contract's repository can place a @@ -1325,10 +1328,16 @@ def sandbox_refusal_hint(allow: DuckDBAllowlist, refusal: Optional[BaseException ) granted = [*allow.dirs, *allow.paths, *allow.remote_prefixes] where = ", ".join(granted) if granted else "nothing (in-memory only)" + # Named because it is readable, not because anyone declared it: a remote read + # also grants the host's trust store (:func:`_ca_bundle_for_remote`), so + # "only the locations the contract declares" would be false here. Left out + # when there is no remote prefix, where it would be noise on every refusal. + bundle = _ca_bundle_for_remote(allow) + also = f", and the TLS trust store a remote read needs ({bundle})" if bundle else "" return ( "DuckDB refused it: contract SQL may only read and write the locations the " - f"contract declares and its own directory ({where}). Declare the file as an " - "input under the contract's directory or workspace, or move it there " + f"contract declares and its own directory ({where}){also}. Declare the file as " + "an input under the contract's directory or workspace, or move it there " f"(the operator can allow another directory with {OPERATOR_DIRS_ENV})." ) diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py index 7725023c..27f04024 100644 --- a/tests/providers/test_duckdb_sandbox_ca_bundle.py +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -196,50 +196,99 @@ def test_the_grant_is_one_file_and_not_a_directory() -> None: def test_the_allowlist_the_caller_passed_is_not_widened() -> None: """The grant is appended at ``SET`` time, not folded into ``allow``. - So a refusal hint lists what the contract declared and not a trust store - the operator never mentioned. + The allowlist object is what every other caller reads, so widening it would + make a declared-inputs list grow a path nobody declared. + + The refusal hint is the one place that must still own up to it: it tells the + user what contract SQL may read, and with a remote prefix declared that + includes the trust store. Saying "only the locations the contract declares" + there was false -- ``test_duckdb_can_read_the_bundle_it_was_granted`` proves + SQL can read the file. Found by the defect scan on this commit. """ - _host_bundle() + bundle = _host_bundle() allow = DuckDBAllowlist.none().with_locations(REMOTE) con = secure_duckdb_connect(":memory:", allow=allow) try: assert allow.paths == () - assert "ssl" not in sandbox_refusal_hint(allow) + assert bundle in sandbox_refusal_hint(allow) finally: con.close() +def test_a_local_refusal_hint_says_nothing_about_a_trust_store() -> None: + """No remote prefix, no disclosure: it would be noise on every local refusal.""" + allow = DuckDBAllowlist.none().with_dirs(os.getcwd()) + + assert "trust store" not in sandbox_refusal_hint(allow) + + # ── which candidate, and in whose order ────────────────────────────────── def test_the_granted_bundle_is_the_first_candidate_that_exists( - monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """Probed in DuckDB's order, so the grant names the file DuckDB opens. Two candidates present: the earlier one wins. A host that has both ``/etc/ssl/certs/ca-certificates.crt`` and ``/etc/ssl/cert.pem`` is ordinary on Debian, and granting the later one would leave the read - refused on the file DuckDB actually reaches for. + refused on the file DuckDB actually reaches for. An absent candidate ahead + of both is skipped rather than returned. + + The candidate list is injected rather than ``os.path.isfile`` patched: + ``sandbox.os`` IS the stdlib module, so patching its ``isfile`` replaces + ``posixpath.isfile`` for pytest and everything else running at the time. + With real files on disk this exercises the production probe itself. + """ + earlier = tmp_path / "earlier-ca-certificates.crt" + earlier.write_text("-----BEGIN CERTIFICATE-----\n", encoding="utf-8") + later = tmp_path / "later-cert.pem" + later.write_text("-----BEGIN CERTIFICATE-----\n", encoding="utf-8") + monkeypatch.setattr( + sandbox, + "_CA_BUNDLE_CANDIDATES", + (str(tmp_path / "absent.crt"), str(earlier), str(later)), + ) + + allow = DuckDBAllowlist.none().with_locations(REMOTE) + assert sandbox._ca_bundle_for_remote(allow) == str(earlier) + + +def test_a_candidate_that_is_not_a_file_is_skipped( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A directory and a dangling symlink at a candidate path are passed over. + + DuckDB opens a file. Returning a directory would hand ``allowed_paths`` an + entry that grants nothing and leave the read refused, and a dangling link + the same. ``os.path.isfile`` follows links and answers False for both -- + pinned here because the fix depends on it. """ - present = { - sandbox._CA_BUNDLE_CANDIDATES[0], - sandbox._CA_BUNDLE_CANDIDATES[-1], - } - monkeypatch.setattr(sandbox.os.path, "isfile", lambda p: p in present) + directory = tmp_path / "dir-shaped.crt" + directory.mkdir() + dangling = tmp_path / "dangling.pem" + dangling.symlink_to(tmp_path / "gone.pem") + real = tmp_path / "real-cert.pem" + real.write_text("-----BEGIN CERTIFICATE-----\n", encoding="utf-8") + monkeypatch.setattr( + sandbox, "_CA_BUNDLE_CANDIDATES", (str(directory), str(dangling), str(real)) + ) allow = DuckDBAllowlist.none().with_locations(REMOTE) - assert sandbox._ca_bundle_for_remote(allow) == sandbox._CA_BUNDLE_CANDIDATES[0] + assert sandbox._ca_bundle_for_remote(allow) == str(real) -def test_a_host_with_no_candidate_grants_nothing(monkeypatch: pytest.MonkeyPatch) -> None: +def test_a_host_with_no_candidate_grants_nothing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: """A minimal container or Windows: DuckDB's own refusal is the better message. Nothing is invented, and the connection still opens -- the failure, if the read is attempted, is DuckDB's, naming the paths it looked for. """ - monkeypatch.setattr(sandbox.os.path, "isfile", lambda _p: False) + monkeypatch.setattr(sandbox, "_CA_BUNDLE_CANDIDATES", (str(tmp_path / "absent.crt"),)) allow = DuckDBAllowlist.none().with_locations(REMOTE) assert sandbox._ca_bundle_for_remote(allow) is None From 42320dbb4184983a94bbb0cdda8e1920fe027a6b Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 01:38:54 +0100 Subject: [PATCH 3/7] security(duckdb): grant a private copy of the CA bundle, never the host trust store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit granted the host's CA bundle to `allowed_paths` directly. **A path in `allowed_paths` is granted for writing**, which the world-class inspection of that commit found and I then reproduced on 1.5.6 and 2.0.0: COPY (SELECT 'ATTACKER CA' AS c) TO '' (FORMAT CSV, USE_TMP_FILE false) replaced a 333 KB trust store with 14 bytes, while an ungranted sibling in the same directory stayed refused — so the grant is file-scoped, and that one file was the trust store. Only file ownership stood in the way: `/etc/ssl/cert.pem` is root-owned, so an unprivileged `fluid` is stopped by the OS. But `cli/scaffold_ci.py` generates a `python:3.12-slim` job, which runs as root, and that job runs `fluid apply --mode amend-and-build`; `providers/local/local.py` turns a contract's `s3://` spec into a remote prefix and then runs that contract's own `properties.sql` on the connection. A contract could have installed its own CA into the trust store of the container it was built in — worse than the read failure the previous commit set out to fix, and the same class as #757/#758. `_private_ca_bundle` now copies the chosen store into a `tempfile.mkdtemp` directory (0700, outside the contract's tree), chmods the copy 0400, grants the COPY, and pins `ca_cert_file` to it so DuckDB opens that file instead of walking its own candidates. The copy is removed by the same `weakref.finalize` teardown the spill directory uses. Two barriers: the store is not in `allowed_paths` at all, and an unprivileged write to the copy is refused by its mode. Root defeats the second, which is why the first matters — and why the copy is per-connection and thrown away, so the damage stops at one connection's own TLS trust, over remote locations the contract chose anyway. The pin is load-bearing, not decoration: with it removed, a live `s3://` read on 2.0.0 fails with the original `Cannot read a CA certificate bundle`, because the store is no longer granted. Also from the same review, all verified before acting: * `sandbox_refusal_hint` now says a private read-only copy of the trust store is readable, without printing the host path — SQL is refused that file, so naming it would point the reader at the wrong one. * duckdb-httpfs#378 is OPEN, not merged. The docstring and CHANGELOG said it "unified" the two verification settings. What is true is measured: 1.5.6 exposes `enable_curl_server_cert_verification` (default true), 2.0.0 no longer does, and an expired and a self-signed certificate are refused on both regardless of `enable_server_cert_verification`. * `certifi` IS installed in the base environment, via requests and httpx, so "would add a dependency" was wrong. The reason to mirror DuckDB's own candidates stands and is now stated correctly: certifi's bundle lives in site-packages, where overwriting it would reach every other tool in the environment. * The drift-guard test installs httpfs, so it now skips where that is not possible, as its sibling in `test_duckdb_sandbox.py` does, and the file's "everything here is offline" claim is corrected. 14 tests on 2.0.0, 13 + 1 skip on 1.5.6 and 1.5.1. Seven mutations, each reverted and the failures recorded in the test docstring rather than predicted — two of them exposed tests that passed for the wrong reason, including the write test itself, which is refused on a developer machine by file ownership whatever the allowlist says and now asserts the store's absence from `allowed_paths`. Live: `--discover s3://…` emits 8 columns and `--discover …/customer.csv` 18, on 1.5.1, 1.5.6 and 2.0.0, and `fluid validate` accepts the result. A CLI discover leaves no directory behind in $TMPDIR. --- CHANGELOG.md | 31 ++- fluid_build/providers/_duckdb_sandbox.py | 120 ++++++++-- .../test_duckdb_sandbox_ca_bundle.py | 220 +++++++++++++++--- 3 files changed, 301 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cde3baea..ad65ddcc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,19 +64,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 file system, where curl and OpenSSL used to open it themselves and bypassed `allowed_paths` entirely. - The sandbox now grants the first CA bundle that exists out of the five DuckDB probes, as **one - file** in `allowed_paths`, and only when the allowlist carries a remote prefix. A local read - gains nothing, `allowed_directories` is untouched — so what a remote prefix grants locally is - unchanged — and the grant is not folded into the allowlist, so refusal messages still name only - what the contract declared. On a host with none of the five (a minimal container, Windows) - nothing is granted and DuckDB's own refusal, which names the paths it looked for, is left to - speak. + The sandbox now takes the first CA bundle that exists out of the five DuckDB probes and grants a + **private, read-only copy** of it — one file in `allowed_paths`, mode 0400 in a 0700 directory + the connection owns, with `ca_cert_file` pinned to it and the copy removed on teardown — and only + when the allowlist carries a remote prefix. A local read gains nothing, `allowed_directories` is + untouched (so what a remote prefix grants locally is unchanged), and the allowlist object is not + widened. On a host with none of the five (a minimal container, Windows) nothing is granted and + DuckDB's own refusal, which names the paths it looked for, is left to speak. + + **Why a copy and not the store itself:** a path in `allowed_paths` is granted for *writing*. The + first version of this fix granted the host store directly, and contract SQL could then run + `COPY (SELECT 'ATTACKER CA') TO ''` and replace it — measured on 1.5.6 and 2.0.0, a + 333 KB trust store overwritten with 14 bytes, with an ungranted sibling in the same directory + still refused. Only file ownership stood in the way: `/etc/ssl/cert.pem` is root-owned, so an + unprivileged `fluid` cannot write it, but `fluid generate ci` emits a `python:3.12-slim` job that + runs as root, and a contract built there could have installed its own CA into that container's + trust store. Granting a copy removes the capability: the store is not in `allowed_paths` at all, + and the copy's mode refuses an unprivileged write as well. Certificate verification is untouched and stays on: an expired and a self-signed certificate are refused on 1.5.6 and 2.0.0 alike, with and without `enable_server_cert_verification` — that - setting governs the httplib client, while `s3://` and `https://` go through curl, whose own flag - defaults to on (duckdb/duckdb-httpfs#378 unified the two). Nothing here disables verification or - pins `ca_cert_file`. + setting governs the httplib client, while `s3://` and `https://` go through curl. 1.5.6 exposes + `enable_curl_server_cert_verification` (default true) and 2.0.0 no longer does; + duckdb/duckdb-httpfs#378 proposes unifying the two and is still open, so the behaviour above is + what was measured rather than what upstream documents. Nothing here disables verification. Verified by running the CLI: `fluid init --discover s3://noaa-ghcn-pds/csv/by_year/2024.csv` emits 8 columns and `--discover https://…/customer.csv` emits 18, on 1.5.1, 1.5.6 and 2.0.0, diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index 5fbc3c91..1b1ee86f 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -60,22 +60,28 @@ and OpenSSL used to open it themselves, bypassing ``allowed_paths`` entirely. So on 1.5.x a remote read works with no CA grant, and on 2.0.0 the same read fails before a byte moves with ``Cannot read a CA certificate - bundle``. :func:`_ca_bundle_for_remote` grants it: one file, only when the - allowlist carries a remote prefix. Measured, not assumed: + bundle``. Measured, not assumed: ``fluid init --discover s3://noaa-ghcn-pds/...`` emitted 8 columns on 1.5.6 and exited 1 on 2.0.0, measured at ``5f2ec50b`` (and on clean ``7cf5471a`` - when the card was filed); the grant makes 2.0.0 emit the same 8. - Two things this does NOT do, because neither is needed: it does not set - ``ca_cert_file`` (the probe mirrors DuckDB's own order, so DuckDB opens the - file that was granted), and it touches no verification setting -- an expired - and a self-signed certificate are already refused on both engines, with and - without ``enable_server_cert_verification``, which is the httplib flag while - s3 and https go through curl (duckdb/duckdb-httpfs#378 unified the two). + when the card was filed). + What is granted is NOT the host store. A path in ``allowed_paths`` can be + WRITTEN, so :func:`_ca_bundle_for_remote` only picks which store to copy and + :func:`_private_ca_bundle` grants a 0400 copy the connection owns, with + ``ca_cert_file`` pinned to it so DuckDB opens that copy and not its own probe + result. Granting the store itself was the first version of this and it let + contract SQL replace the host trust store wherever the process can write it. + No verification setting is touched: an expired and a self-signed certificate + are refused on 1.5.6 and 2.0.0 alike, with and without + ``enable_server_cert_verification``, which governs the httplib client while + s3 and https go through curl. 1.5.6 exposes + ``enable_curl_server_cert_verification`` (default true) and 2.0.0 no longer + does; duckdb/duckdb-httpfs#378 proposes unifying them and is still open, so + the behaviour above is what was measured rather than what upstream documents. Plain ``http://`` did not need the grant -- duckdb/duckdb-httpfs#499 made a failed bundle load non-fatal there, and 2.0.0 carries it -- but it gets one - anyway, because the prefix is what is declared and an http base URL that - redirects to https still verifies (which is #499's own reason for loading the - bundle on that path). + anyway, because the declared prefix is what the grant keys on and an http base + URL that redirects to https still verifies (which is #499's own reason for + loading the bundle on that path). * DuckDB folds its ``temp_directory`` into ``allowed_directories``, and its default is relative: ``.tmp`` in the working directory, or ``.tmp`` beside a file database. Both sit where the contract's repository can place a @@ -919,6 +925,10 @@ def _open_lock(target: str) -> Any: #: Named so an operator who finds one left behind knows what wrote it. SPILL_DIR_PREFIX = "fluid-duckdb-spill-" +#: Prefix of the directory holding one connection's private CA-bundle copy +#: (:func:`_private_ca_bundle`). Named for the same reason. +CA_DIR_PREFIX = "fluid-duckdb-ca-" + def _refuse_caller_spill_dir(config: Optional[Mapping[str, Any]]) -> None: """Refuse a ``config`` that names ``temp_directory``: the sandbox owns it. @@ -1033,11 +1043,13 @@ def _refuse_escaping_remote_prefixes(allow: DuckDBAllowlist) -> None: #: The CA bundles DuckDB's httpfs probes, in the order its own refusal names #: them ("Add one of ... to allowed_paths"). Mirrored from DuckDB rather than -#: borrowed from ``ssl.get_default_verify_paths()`` or ``certifi``, the way -#: sethmlarson/truststore picks a trust store for OpenSSL: the grant has to -#: name the file DuckDB will open, and DuckDB probes these five itself, -#: consulting neither OpenSSL's defaults nor ``SSL_CERT_FILE``. Granting -#: certifi's bundle would also add a dependency the base install does not ship. +#: borrowed from ``ssl.get_default_verify_paths()`` or ``certifi`` (which IS +#: installed, via requests and httpx), the way sethmlarson/truststore picks a +#: trust store for OpenSSL: what is copied has to be a store DuckDB itself +#: would have trusted, and DuckDB probes these five, consulting neither +#: OpenSSL's defaults nor ``SSL_CERT_FILE``. certifi's bundle sits inside +#: site-packages, where overwriting it would reach every other tool in the +#: environment -- the opposite of what :func:`_private_ca_bundle` is for. #: ``tests/providers/test_duckdb_sandbox_ca_bundle.py`` parses the list back out #: of DuckDB's refusal and fails if upstream's order or contents drift. _CA_BUNDLE_CANDIDATES = ( @@ -1050,7 +1062,7 @@ def _refuse_escaping_remote_prefixes(allow: DuckDBAllowlist) -> None: def _ca_bundle_for_remote(allow: DuckDBAllowlist) -> Optional[str]: - """The one CA bundle to grant for ``allow``'s remote prefixes, if any. + """The host CA bundle a remote read needs, if there is one to copy. ``None`` when the call site declared no remote prefix -- a purely local read must not gain a grant it has no use for -- and ``None`` when the host @@ -1059,6 +1071,10 @@ def _ca_bundle_for_remote(allow: DuckDBAllowlist) -> Optional[str]: refusal naming the paths it looked for, which is more accurate than anything this could say. + What reaches ``allowed_paths`` is never this path. It is a copy made by + :func:`_private_ca_bundle`, because a granted path is WRITABLE -- see that + function. + A directory is never returned. ``allowed_paths`` grants one file, so this cannot widen what :func:`_refuse_ungranted_dirs` reads back from ``allowed_directories`` -- including the remote prefixes that are @@ -1072,6 +1088,40 @@ def _ca_bundle_for_remote(allow: DuckDBAllowlist) -> Optional[str]: return None +def _private_ca_bundle(bundle: str) -> str: + """A mode-0400 copy of ``bundle`` in a 0700 directory this connection owns. + + **A path in ``allowed_paths`` is granted for WRITING, not only reading.** + Measured on 1.5.6 and 2.0.0: with the host store granted directly, + ``COPY (SELECT 'ATTACKER CA') TO '' (FORMAT CSV, USE_TMP_FILE + false)`` replaced a 333 KB trust store with 14 bytes, while an ungranted + sibling in the same directory stayed refused. The OS is the only thing that + stops it: ``/etc/ssl/cert.pem`` is root-owned, so an unprivileged ``fluid`` + cannot write it -- but ``cli/scaffold_ci.py`` generates a ``python:3.12-slim`` + job, which runs as root, and that job runs ``fluid apply``. Contract SQL + could have installed its own CA into the trust store of the container it was + built in. Found by the world-class inspection of the first version of this + fix, which granted the store itself. + + So the store is never granted. The connection reads a copy that belongs to + it: 0400 so an unprivileged write is refused by the OS as well as by the + allowlist, under :func:`tempfile.mkdtemp`'s 0700 directory outside the + contract's tree, removed when the connection is gone. Root can still + overwrite the copy, and that is the whole point of the copy -- the damage + stops at one connection's own TLS trust, whose remote locations the contract + chose anyway, and no state outside the process is touched. + + The cost is one ~330 KB file copy per connection that declared a remote + prefix, which is why :func:`_ca_bundle_for_remote` returns ``None`` for + every other connection. + """ + directory = tempfile.mkdtemp(prefix=CA_DIR_PREFIX) + copy = os.path.join(directory, "ca-bundle.pem") + shutil.copyfile(bundle, copy) + os.chmod(copy, 0o400) + return copy + + def _refuse_ungranted_dirs(con: Any, allow: DuckDBAllowlist, spill: str) -> None: """Refuse the connection if DuckDB granted a directory ``allow`` does not name. @@ -1256,10 +1306,24 @@ def _confine_connection( directories = [_with_sep(d) for d in allow.dirs] + list(allow.remote_prefixes) con.execute(f"SET allowed_directories = {_sql_list(directories)}") # The CA bundle is appended here and NOT folded into ``allow``: the - # allowlist stays what the call site declared, so a refusal hint and the - # read-back below speak only of the caller's own grants. - bundle = _ca_bundle_for_remote(allow) - paths = list(allow.paths) + ([bundle] if bundle is not None else []) + # allowlist stays what the call site declared, so the read-back below and + # every caller that reads ``allow`` as a declared-inputs list are unchanged. + # What is granted is a private copy, never the host store: a granted path + # can be written (:func:`_private_ca_bundle`). + paths = list(allow.paths) + host_bundle = _ca_bundle_for_remote(allow) + if host_bundle is not None: + private = _private_ca_bundle(host_bundle) + spill_cleanup.append( + weakref.finalize(con, shutil.rmtree, os.path.dirname(private), ignore_errors=True) + ) + paths.append(private) + try: + con.execute(f"SET ca_cert_file = {quote_ansi_string_literal(private)}") + except duckdb.Error: + # httpfs is not loaded on this connection, so the setting does not + # exist and no TLS will happen either. Nothing to point at. + pass con.execute(f"SET allowed_paths = {_sql_list(paths)}") con.execute("SET enable_external_access = false") con.execute("SET lock_configuration = true") @@ -1332,8 +1396,15 @@ def sandbox_refusal_hint(allow: DuckDBAllowlist, refusal: Optional[BaseException # also grants the host's trust store (:func:`_ca_bundle_for_remote`), so # "only the locations the contract declares" would be false here. Left out # when there is no remote prefix, where it would be noise on every refusal. - bundle = _ca_bundle_for_remote(allow) - also = f", and the TLS trust store a remote read needs ({bundle})" if bundle else "" + # Named without its path: what SQL can read is the private copy + # :func:`_private_ca_bundle` made for this connection, not the host store + # this returns, and printing the store would point the reader at a file the + # SQL was in fact refused. + also = ( + ", and a private read-only copy of the host's TLS trust store, which a remote " "read needs" + if _ca_bundle_for_remote(allow) + else "" + ) return ( "DuckDB refused it: contract SQL may only read and write the locations the " f"contract declares and its own directory ({where}){also}. Declare the file as " @@ -1344,6 +1415,7 @@ def sandbox_refusal_hint(allow: DuckDBAllowlist, refusal: Optional[BaseException __all__ = [ "BROAD_ROOT_REASONS", + "CA_DIR_PREFIX", "MIN_DUCKDB_VERSION", "OPERATOR_DIRS_ENV", "SPILL_DIR_PREFIX", diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py index 27f04024..a8feb855 100644 --- a/tests/providers/test_duckdb_sandbox_ca_bundle.py +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -32,29 +32,58 @@ What the grant must not become is the reason this file exists. It is one FILE in ``allowed_paths``, so it cannot widen ``allowed_directories`` -- where the remote prefixes that are load-bearing on 1.5.x live (#757) -- and it is added only when -the call site declared a remote prefix, so a local read gains nothing. The -allowlist object is left alone: the grant is appended at ``SET`` time, so a -refusal hint still speaks only of what the caller asked for. - -Everything here is offline. The real network proof is the CLI run in the PR +the call site declared a remote prefix, so a local read gains nothing. + +And it is never the host's trust store. **A path in ``allowed_paths`` is granted +for WRITING**, which the world-class inspection of the first version of this fix +proved by replacing a 333 KB store with 14 bytes of its own PEM through ``COPY +… TO``. What is granted is a mode-0400 copy the connection owns, with +``ca_cert_file`` pinned to it, thrown away on teardown. The allowlist OBJECT is +still left alone -- the grant is appended at ``SET`` time, so every caller that +reads ``allow`` as the declared-inputs list is unaffected -- while the refusal +hint does disclose that a copy of the trust store is readable, because telling +the user otherwise would be false. + +Every test here but one is offline, and that one +(``test_the_candidate_list_matches_the_one_duckdb_names``) needs httpfs, so it +skips when the extension cannot be installed, as its sibling in +``test_duckdb_sandbox.py`` does. The real network proof is the CLI run in the PR description; what CI can check without a network is the read-back, and on 2.0.0 the refusal itself, which arrives before any connection is attempted. Each mechanism was reverted on duckdb 2.0.0 and the failures recorded, not -predicted. ``_ca_bundle_for_remote`` returning ``None`` fails -``test_a_declared_remote_prefix_grants_one_ca_bundle``, -``test_duckdb_can_read_the_bundle_it_was_granted`` and -``test_the_granted_bundle_is_the_first_candidate_that_exists``. Returning -``dirname(bundle)`` fails those three and -``test_the_grant_is_one_file_and_not_a_directory`` -- which it did NOT, until -that test was made to check the granted entry itself: a directory handed to +predicted: + +=================================== ==================================== +reverted tests that fail +=================================== ==================================== +the probe returns ``None`` 8 +the probe returns the directory 9 +the remote-prefix guard is dropped 2 (both local-read tests) +the copy is replaced by the store 4 +``chmod 0400`` is dropped 2 +the ``ca_cert_file`` pin is dropped 1 -- AND a live ``s3://`` read on + 2.0.0 fails with the original + ``Cannot read a CA certificate + bundle``, which is what makes the pin + load-bearing rather than belt-and-braces +the cleanup finalize is dropped 1 +=================================== ==================================== + +Two of those were worth the trouble of checking. Returning the bundle's +directory did NOT fail ``test_the_grant_is_one_file_and_not_a_directory`` until +that test was made to check the granted entry itself -- a directory handed to ``allowed_paths`` never reaches ``allowed_directories``, so reading the latter -back could not see it. Dropping the ``allow.remote_prefixes`` guard fails -``test_a_local_read_grants_no_ca_bundle`` alone. +back could not see it. And granting the host store did NOT fail the write test +until it asserted the store's absence from ``allowed_paths``: the ``COPY`` is +refused on a developer machine either way, because ``/etc/ssl/cert.pem`` is +root-owned. In the root container the threat model is about, it would not have +been refused at all. """ from __future__ import annotations +import gc import os import re import sys @@ -110,42 +139,155 @@ def _host_bundle() -> str: # ── the defect ─────────────────────────────────────────────────────────── -def test_a_declared_remote_prefix_grants_one_ca_bundle() -> None: +def test_a_declared_remote_prefix_grants_one_private_copy_of_the_bundle() -> None: """The grant that makes a remote read possible on 2.0.0. - Exactly one entry, and it is a bundle DuckDB named: before the fix - ``allowed_paths`` was empty here and the read raised PermissionException. + Exactly one entry; it carries the host store's bytes, and it is NOT the host + store. Before the fix ``allowed_paths`` was empty here and the read raised + PermissionException. """ bundle = _host_bundle() con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) try: - assert _granted_paths(con) == [sandbox._fold(os.path.realpath(bundle))] + granted = _granted_paths(con) + assert len(granted) == 1, granted + copy = granted[0] + assert sandbox.CA_DIR_PREFIX in copy, copy + assert copy != sandbox._fold(os.path.realpath(bundle)) + with open(copy, "rb") as made, open(bundle, "rb") as host: + assert made.read() == host.read() + assert oct(os.stat(copy).st_mode & 0o777) == "0o400" + assert oct(os.stat(os.path.dirname(copy)).st_mode & 0o777) == "0o700" + finally: + con.close() + + +def test_duckdb_is_pinned_at_the_copy() -> None: + """``ca_cert_file`` names the copy, so DuckDB cannot fall back to its probe. + + Without the pin DuckDB would walk its own five candidates and open the host + store, which is no longer granted -- the read would fail with the very + refusal this fix exists to remove. + + ``ca_cert_file`` is an httpfs setting and does not exist on a connection + that did not load the extension, which is why the production code tolerates + the ``SET`` failing and this test loads httpfs (and skips where it cannot be + installed). + """ + _host_bundle() + + try: + con = secure_duckdb_connect( + ":memory:", + allow=DuckDBAllowlist.none().with_locations(REMOTE), + extensions=["httpfs"], + ) + except duckdb.Error: + pytest.skip("httpfs is not installable here (offline)") + try: + # Both sides through the same fold: _granted_paths() case-folds the way + # the sandbox does, and the raw setting does not. + pinned = sandbox._fold(os.path.realpath(_setting(con, "ca_cert_file"))) + assert pinned == _granted_paths(con)[0] finally: con.close() -def test_duckdb_can_read_the_bundle_it_was_granted() -> None: - """The grant is the file DuckDB opens, not merely a path it tolerates. +def test_duckdb_can_read_the_copy_it_was_granted_and_not_the_host_store() -> None: + """The grant is the file DuckDB opens, and the host store stays unreadable. - Reading the bundle as text through the sandbox proves the entry is - effective -- the probe mirrors DuckDB's own order, so the file granted is - the one its TLS picks up. The read of an UNGRANTED candidate-shaped path is - the control: the grant is one file, not its directory. + ``ca_cert_file`` is pinned to the copy, so the file granted is the file its + TLS picks up -- proven by the live reads elsewhere, and here by reading the + copy as text through the sandbox. The two controls are what the first + version of this fix got wrong: the host store itself is refused, and so is + an ungranted sibling of the copy, so the grant is one file and not its + directory. """ bundle = _host_bundle() - neighbour = os.path.join(os.path.dirname(bundle), "fluid-not-granted.pem") con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) try: - rows = con.execute(f"SELECT count(*) FROM read_text('{bundle}')").fetchall() - assert rows[0][0] == 1 + copy = _granted_paths(con)[0] + assert con.execute(f"SELECT count(*) FROM read_text('{copy}')").fetchall()[0][0] == 1 + neighbour = os.path.join(os.path.dirname(copy), "fluid-not-granted.pem") with pytest.raises(duckdb.Error): con.execute(f"SELECT count(*) FROM read_text('{neighbour}')").fetchall() + with pytest.raises(duckdb.Error): + con.execute(f"SELECT count(*) FROM read_text('{bundle}')").fetchall() + finally: + con.close() + + +def test_the_granted_copy_cannot_be_written_and_the_host_store_is_not_granted() -> None: + """The reason the grant is a copy at all. + + **A path in ``allowed_paths`` is granted for writing.** Measured on 1.5.6 + and 2.0.0 against the first version of this fix, which granted the host + store directly: ``COPY (SELECT 'ATTACKER CA') TO '' (FORMAT CSV, + USE_TMP_FILE false)`` replaced a 333 KB trust store with 14 bytes, while an + ungranted sibling stayed refused. Only file ownership stopped it, and + ``cli/scaffold_ci.py`` generates a root ``python:3.12-slim`` job that runs + ``fluid apply``. + + Two barriers now, and this asserts both: the host store is not in + ``allowed_paths`` at all, and the copy that is there is mode 0400, so an + unprivileged write is refused by the OS as well. Running as root defeats + the second -- which is why the first one matters, and why the copy is + per-connection and thrown away. + """ + bundle = _host_bundle() + with open(bundle, "rb") as host: + before = host.read() + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + try: + copy = _granted_paths(con)[0] + # The barrier that does not depend on who we are. Asserted because the + # COPY below is refused on this host either way: /etc/ssl/cert.pem is + # root-owned, so granting the store outright (the first version of this + # fix) still failed to write it as an unprivileged user -- the test + # would have passed for the wrong reason, and in the root CI container + # that this threat model is about, it would not have passed at all. + assert sandbox._fold(os.path.realpath(bundle)) not in _granted_paths(con) + with pytest.raises(duckdb.Error): + con.execute( + f"COPY (SELECT 'ATTACKER CA' AS c) TO '{bundle}' " + "(FORMAT CSV, USE_TMP_FILE false)" + ) + if os.geteuid() != 0: + with pytest.raises(duckdb.Error): + con.execute( + f"COPY (SELECT 'ATTACKER CA' AS c) TO '{copy}' " + "(FORMAT CSV, USE_TMP_FILE false)" + ) + # The store is byte-identical afterwards. Compared to itself rather than + # to an expected header: macOS's bundle opens with an OpenBSD comment, + # so a guess at its first line fails on a file that was never touched. + with open(bundle, "rb") as host: + assert host.read() == before finally: con.close() +def test_the_copy_is_removed_once_the_connection_is_gone() -> None: + """One ~330 KB file per remote connection, not a leak in $TMPDIR. + + The same ``weakref.finalize`` teardown the spill directory uses. + """ + _host_bundle() + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + directory = os.path.dirname(_granted_paths(con)[0]) + assert os.path.isdir(directory) + + con.close() + del con + gc.collect() + + assert not os.path.exists(directory), directory + + # ── the grant stays narrow ─────────────────────────────────────────────── @@ -201,9 +343,10 @@ def test_the_allowlist_the_caller_passed_is_not_widened() -> None: The refusal hint is the one place that must still own up to it: it tells the user what contract SQL may read, and with a remote prefix declared that - includes the trust store. Saying "only the locations the contract declares" - there was false -- ``test_duckdb_can_read_the_bundle_it_was_granted`` proves - SQL can read the file. Found by the defect scan on this commit. + includes a copy of the trust store. Saying "only the locations the contract + declares" there was false, which the defect scan caught. The host store's + PATH is deliberately not printed -- SQL is refused that file, so naming it + would point the reader at the wrong one. """ bundle = _host_bundle() allow = DuckDBAllowlist.none().with_locations(REMOTE) @@ -211,7 +354,9 @@ def test_the_allowlist_the_caller_passed_is_not_widened() -> None: con = secure_duckdb_connect(":memory:", allow=allow) try: assert allow.paths == () - assert bundle in sandbox_refusal_hint(allow) + hint = sandbox_refusal_hint(allow) + assert "trust store" in hint + assert bundle not in hint finally: con.close() @@ -325,11 +470,14 @@ def test_the_candidate_list_matches_the_one_duckdb_names(monkeypatch: pytest.Mon pytest.skip("the CA bundle is read outside the sandbox before 2.0") monkeypatch.setattr(sandbox, "_ca_bundle_for_remote", lambda _allow: None) - con = secure_duckdb_connect( - ":memory:", - allow=DuckDBAllowlist.none().with_locations(UNREACHABLE), - extensions=["httpfs"], - ) + try: + con = secure_duckdb_connect( + ":memory:", + allow=DuckDBAllowlist.none().with_locations(UNREACHABLE), + extensions=["httpfs"], + ) + except duckdb.Error: + pytest.skip("httpfs is not installable here (offline)") try: with pytest.raises(duckdb.Error) as excinfo: con.execute(f"SELECT * FROM read_csv_auto('{UNREACHABLE}') LIMIT 0") From 4ce1cc65edcae833a5ce6e63669fdadcabbcecc3 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 02:24:04 +0100 Subject: [PATCH 4/7] test(duckdb): close the four gaps the five-lens review left standing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adversarial review of dd1aeaa1 raised 24 findings, 12 verified by skeptics. Its HIGH — the CA grant is a write grant — is the same defect the world-class inspection found and 42320dbb fixed, reached independently by a second route. Four items survived against the current code: * `os.path.isabs` on a POSIX candidate list. From Python 3.13 `ntpath.isabs` answers False for a path with one leading slash, so the test failed on Windows under 3.13+ about the platform rather than the list. Measured: `ntpath.isabs('/etc/ssl/cert.pem')` is True on 3.12, False on 3.14. Now `posixpath.isabs`. No CI lane runs this file on Windows today — the Test matrix is ubuntu-only and the windows DuckDB lane runs a different file list — so this was a contributor-facing break, not a red build. * Nothing pinned the candidate list where it can be checked everywhere. The drift guard reads the list back out of DuckDB's own refusal, but it needs 2.0.0 and httpfs, and the `duckdb-floor` lane — the lane this file was added to — installs MIN_DUCKDB_VERSION 1.5.0, where it skips. The review measured that narrowing the production tuple to one absent path left 1.5.6 fully green. `test_the_candidate_list_is_pinned_literally` has no host or engine dependency, so an edit to the list has to be made twice, deliberately. * Nothing pinned the MIXED allowlist the changed line builds. Every other test declares either a remote prefix or a local file, never both, so a regression in `allow.paths + [copy]` was invisible from this file. `test_a_declared_file_and_the_ca_copy_both_reach_allowed_paths` covers it and fails when the caller's paths are dropped. It asserts MEMBERSHIP: DuckDB's read-back order is its own — three files declared `zzz, aaa, mmm` came back `mmm, aaa, zzz`, neither sorted nor as declared. * The refusal was quoted with a prefix DuckDB never prints. `PermissionException` is the exception type; the message starts `Permission Error:`. Spliced, the recorded symptom matched nothing in a log or a grep. Corrected here and in the CHANGELOG. Two of the review's MEDIUMs were already fixed in 42320dbb (the open #378 citation, the certifi claim) and its remaining operational findings are recorded as known limitations rather than silently left: there is no operator override for a CA bundle outside DuckDB's five paths, which leaves a TLS-inspecting-proxy host with no exit, and `azure://` is not measured. `gs://`, `http://`, `https://` and `s3://` all read through the fix on 2.0.0 with exactly one granted path each (2, 18, 18 and 8 columns), so the CHANGELOG's coverage claim is measured rather than assumed. 16 tests on 2.0.0, 15 + 1 skip on 1.5.6 and 1.5.1. --- CHANGELOG.md | 12 ++- .../test_duckdb_sandbox_ca_bundle.py | 83 +++++++++++++++++-- 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad65ddcc..aa75feff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **`fluid init --discover` against a remote URL failed on DuckDB 2.0.0**, before a byte was read: - `PermissionException: Cannot read a CA certificate bundle: file system access is restricted by + `Permission Error: Cannot read a CA certificate bundle: file system access is restricted by configuration`. TLS needs the host's trust store, the sandbox granted no path to it, and nothing granted one when a remote prefix was declared — so the whole remote-discovery surface was broken on the engine the project resolves to by default, while the same call worked on 1.5.x. The cause @@ -109,6 +109,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `{{ env.NAME }}` interpolation inside a contract bypasses `secretRef` entirely. Neither is changed by this release. +- **A CA bundle outside DuckDB's five candidate paths cannot be used.** There is no operator + override (no `FLUID_DUCKDB_CA_BUNDLE`, and DuckDB consults neither `SSL_CERT_FILE` nor OpenSSL's + defaults — measured), so a host whose trust store lives elsewhere, including one behind a + TLS-inspecting proxy whose own CA is installed somewhere else, still gets DuckDB's refusal on a + remote read. That is no worse than before this release, where every such read failed on 2.0.0 + regardless, and the refusal still names the paths it looked for — but it is not closed. +- **`azure://` / `az://` is not measured.** The fix grants the copy for every declared remote + prefix, but the Azure filesystem extension does its own TLS and may not read the bundle through + this path. `s3://`, `gs://`, `http://` and `https://` were verified live. + ## [0.24.0] — 2026-10-27 ### Fixed diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py index a8feb855..507964c6 100644 --- a/tests/providers/test_duckdb_sandbox_ca_bundle.py +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -14,9 +14,11 @@ """A declared remote prefix grants the one CA bundle its TLS needs, and nothing else. -On DuckDB 2.0.0 every remote read failed before a byte moved:: +On DuckDB 2.0.0 every remote read failed before a byte moved. The message, as +DuckDB prints it and as the CLI shows it on its ``why`` line -- the exception +type is ``duckdb.PermissionException``, which is not part of the text:: - PermissionException: Cannot read a CA certificate bundle: file system access + Permission Error: Cannot read a CA certificate bundle: file system access is restricted by configuration. Add one of /etc/ssl/certs/ca-certificates.crt, ... to allowed_paths, point ca_cert_file at an allowed file, or disable server certificate verification @@ -85,6 +87,7 @@ import gc import os +import posixpath import re import sys from pathlib import Path @@ -116,6 +119,9 @@ #: below work with no network. UNREACHABLE = "https://127.0.0.1:1/nothing.csv" +#: The same bucket as a bare prefix, for the allowlist built by hand. +REMOTE_PREFIX = "s3://noaa-ghcn-pds/csv/by_year/" + def _granted_paths(con: Any) -> List[str]: """``allowed_paths`` in the form DuckDB reports it back. @@ -307,6 +313,33 @@ def test_a_local_read_grants_no_ca_bundle(tmp_path: Path) -> None: con.close() +def test_a_declared_file_and_the_ca_copy_both_reach_allowed_paths(tmp_path: Path) -> None: + """The shape the changed line assembles: ``allow.paths`` plus the copy. + + Every other test here declares one or the other, so a regression in the + concatenation -- dropping the caller's own declared files and granting only + the bundle -- would be invisible. + + Membership, not order: DuckDB's read-back order is its own. Measured with + three files declared ``zzz, aaa, mmm``, it reported ``mmm, aaa, zzz`` -- + neither sorted nor as declared -- so an assertion on position would be + pinning an implementation detail that happens to hold today. + """ + _host_bundle() + declared = tmp_path / "declared.csv" + declared.write_text("a,b\n1,2\n", encoding="utf-8") + allow = DuckDBAllowlist.none().with_paths(str(declared)).with_remote(REMOTE_PREFIX) + + con = secure_duckdb_connect(":memory:", allow=allow) + try: + granted = _granted_paths(con) + assert len(granted) == 2, granted + assert sandbox._fold(os.path.realpath(str(declared))) in granted, granted + assert any(sandbox.CA_DIR_PREFIX in entry for entry in granted), granted + finally: + con.close() + + def test_the_grant_is_one_file_and_not_a_directory() -> None: """``allowed_directories`` is untouched, so #757's prefix grant is unaffected. @@ -445,15 +478,53 @@ def test_a_host_with_no_candidate_grants_nothing( con.close() -def test_every_candidate_is_an_absolute_file_path() -> None: - """A relative entry would be refused by DuckDB and a directory would widen the grant.""" +def test_every_candidate_is_an_absolute_posix_file_path() -> None: + """A relative entry would be refused by DuckDB and a directory would widen the grant. + + Judged with ``posixpath``, not ``os.path``: these are POSIX paths, and from + Python 3.13 ``ntpath.isabs`` answers False for a path with a single leading + slash (it is drive-relative on Windows). Measured: ``ntpath.isabs( + '/etc/ssl/cert.pem')`` is True on 3.12 and False on 3.14, so ``os.path`` + would fail this test for a contributor on Windows under 3.13 or later -- + about their platform, not about the list. + + The directory check only bites for a candidate that exists; what keeps a + non-existent one honest is :func:`_ca_bundle_for_remote`'s own + ``os.path.isfile``, pinned by ``test_a_candidate_that_is_not_a_file_is_skipped``. + """ assert sandbox._CA_BUNDLE_CANDIDATES for candidate in sandbox._CA_BUNDLE_CANDIDATES: - assert os.path.isabs(candidate), candidate - assert not candidate.endswith(("/", os.sep)), candidate + assert posixpath.isabs(candidate), candidate + assert not candidate.endswith("/"), candidate assert not os.path.isdir(candidate), candidate +#: What :data:`sandbox._CA_BUNDLE_CANDIDATES` must hold, spelled out here so an +#: edit to it has to be made twice, deliberately. +EXPECTED_CANDIDATES = ( + "/etc/ssl/certs/ca-certificates.crt", + "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", + "/etc/pki/tls/certs/ca-bundle.crt", + "/etc/ssl/ca-bundle.pem", + "/etc/ssl/cert.pem", +) + + +def test_the_candidate_list_is_pinned_literally() -> None: + """The one check on the list that runs on every host and every engine. + + ``test_the_candidate_list_matches_the_one_duckdb_names`` is the authority on + what DuckDB actually probes, but it needs 2.0.0 and httpfs, so it skips on + the ``duckdb-floor`` lane -- which installs MIN_DUCKDB_VERSION, 1.5.0, the + very lane this file was added to. The probe tests skip too where the host + has none of the five. Measured on the first version of this file: narrowing + the production tuple to one absent path left duckdb 1.5.6 fully green, so + total drift in OUR copy of the list was invisible everywhere but one lane. + This assertion has no host or engine dependency at all. + """ + assert sandbox._CA_BUNDLE_CANDIDATES == EXPECTED_CANDIDATES + + def test_the_candidate_list_matches_the_one_duckdb_names(monkeypatch: pytest.MonkeyPatch) -> None: """The drift guard: upstream's list parsed back out of its own refusal. From 4d6e9449cf9c9c47f71aa11a22445636d11219ea Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 03:04:26 +0100 Subject: [PATCH 5/7] security(duckdb): the CA grant is dropped when it cannot be used or cannot be trusted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round two of the adversarial review, aimed at the redesign itself. 28 findings, 12 verified; four mechanisms and a pile of prose. Everything below I reproduced before acting on it. **The 0400 is not a write bar.** `tempfile.mkdtemp` honours `$TMPDIR`, and a workspace-local `$TMPDIR` is an ordinary CI, direnv or Nix arrangement. Where it points inside a directory the connection grants, the copy's directory is writable by contract SQL, and `COPY TO` with its DEFAULT options writes a temporary file and renames it over the target — measured on 1.5.6 and 2.0.0, 333483 bytes of trust anchors replaced by 14. The mode only ever refused a plain open. `_ca_dir_within_a_grant` now detects that case and the grant is dropped with a `duckdb_ca_bundle_grant_skipped` warning, because granting a trust store the SQL can replace is worse than granting none. **The pin now gates the grant.** `ca_cert_file` lives in httpfs, so on a connection that loaded a different extension the `SET` fails and DuckDB never opens the copy — `cli/discover/filesystem.py` loads `azure` for an `azure://` URL, which is exactly that path. The old code granted the copy anyway: a writable file in `allowed_paths` plus a 330 KB copy for a benefit that cannot exist. No pin, no grant, and the copy is removed at once. **The pin is `SET GLOBAL`.** `ca_cert_file` is a LOCAL-scope setting, so a session-scoped pin left `con.cursor()` with `''` while `allowed_paths` (GLOBAL) still showed it the copy: on 2.0.0 every remote read through a cursor failed with the original `Cannot read a CA certificate bundle`, and the locked configuration meant no caller could fix it. Measured: GLOBAL, the cursor inherits it, both read 8 columns, and SQL still cannot change it after the lock. **A failed copy cleans up after itself.** `mkdtemp` runs before the caller registers the `weakref.finalize`, so an ENOSPC, an I/O error, or the candidate disappearing between the `isfile` probe and the copy leaked the directory for the lifetime of `$TMPDIR`. Prose the review caught, all of it load-bearing for the next reader: * the module's numbered pipeline (step 4) still described version 1 — the thing the redesign exists to stop doing — and omitted `ca_cert_file`; * the comment above `sandbox_refusal_hint`'s branch still said a remote read "grants the host's trust store", three lines above the new comment saying the opposite; * `_private_ca_bundle` claimed the 0400 made a write "refused by the OS as well as by the allowlist". The allowlist does not refuse it — that is the premise of the function — and the same docstring claimed "no state outside the process is touched" about a write that lands in `$TMPDIR`; * the CHANGELOG said the copy "removes the capability" in the one threat model (a root container) where the mode buys nothing. It now says what is removed: the reach. The host store is not in `allowed_paths`, so nothing outside this process's temporary directory can be written; * "removed on teardown" overstated the finalizer: `con.close()` alone does not remove the copy, measured; it goes when the connection object does, at interpreter exit, or not at all on `os._exit` — the same limits the spill directory documents; * "every test here but one is offline" was false once the grant tests needed httpfs. 20 tests on 2.0.0, 19 + 1 skip on 1.5.6 and 1.5.1. The recorded mutation table is re-measured against all 20 and now has eleven rows; one row's count DROPPED (dropping the remote-prefix guard fails one test, not two, because the pin gate independently blocks a local connection) and the table says so, since a count that drifts silently teaches a maintainer to distrust it. Live: `--discover` emits 8 and 18 columns on 1.5.1, 1.5.6 and 2.0.0; a clean suite run leaves no directory in $TMPDIR. --- CHANGELOG.md | 19 +- fluid_build/providers/_duckdb_sandbox.py | 119 ++++++++-- .../test_duckdb_sandbox_ca_bundle.py | 219 ++++++++++++++---- 3 files changed, 283 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa75feff..7e92c992 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,9 +65,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 entirely. The sandbox now takes the first CA bundle that exists out of the five DuckDB probes and grants a - **private, read-only copy** of it — one file in `allowed_paths`, mode 0400 in a 0700 directory - the connection owns, with `ca_cert_file` pinned to it and the copy removed on teardown — and only - when the allowlist carries a remote prefix. A local read gains nothing, `allowed_directories` is + **private copy** of it — one file in `allowed_paths`, mode 0400 in a 0700 directory the connection + owns, with `ca_cert_file` pinned to it (`SET GLOBAL`, so a `con.cursor()` inherits the pin) and + the copy removed when the connection object is finalised — and only when the allowlist carries a + remote prefix AND that pin lands. A local read gains nothing, `allowed_directories` is untouched (so what a remote prefix grants locally is unchanged), and the allowlist object is not widened. On a host with none of the five (a minimal container, Windows) nothing is granted and DuckDB's own refusal, which names the paths it looked for, is left to speak. @@ -79,8 +80,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 still refused. Only file ownership stood in the way: `/etc/ssl/cert.pem` is root-owned, so an unprivileged `fluid` cannot write it, but `fluid generate ci` emits a `python:3.12-slim` job that runs as root, and a contract built there could have installed its own CA into that container's - trust store. Granting a copy removes the capability: the store is not in `allowed_paths` at all, - and the copy's mode refuses an unprivileged write as well. + trust store. Granting a copy removes the reach, which is the part that mattered: the host store is + not in `allowed_paths` at all, so nothing outside this process's own temporary directory can be + written. It does **not** make the copy itself immutable — the mode is a speed bump. `COPY TO` with + its default options writes a temporary file and renames it over the target, so 0400 stops nothing + once the containing directory is writable by the SQL, and root ignores the mode regardless. The + grant is therefore dropped outright when the copy would land inside a granted directory (`$TMPDIR` + pointed into the workspace — an ordinary CI, direnv or Nix arrangement), and dropped when the + `ca_cert_file` pin cannot land, which is every connection that loaded no httpfs. What an attacker + who replaces the copy gets is the trust anchors of one connection, over remote locations the + contract named anyway, on a file that is gone when that connection is. Certificate verification is untouched and stays on: an expired and a self-signed certificate are refused on 1.5.6 and 2.0.0 alike, with and without `enable_server_cert_verification` — that diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index 1b1ee86f..d9dec4be 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -35,8 +35,10 @@ 4. ``temp_directory`` (a directory this helper creates and owns, see below), ``home_directory`` (pinned to ``$HOME``, see below), then ``allowed_directories`` and ``allowed_paths``: what the call site declares - it reads or writes, plus -- only when it declared a remote prefix -- the one - host CA bundle file DuckDB's TLS needs (:func:`_ca_bundle_for_remote`); + it reads or writes, plus -- only when it declared a remote prefix, and only + when the pin below lands -- a private copy of the host CA bundle DuckDB's TLS + needs (:func:`_private_ca_bundle`), with ``ca_cert_file`` pinned to that copy + (``SET GLOBAL``, so a ``con.cursor()`` inherits it); 5. ``enable_external_access = false``; 6. ``lock_configuration = true``, so the SQL cannot ``SET`` any of it back, then a read-back that the grants are the ones the call site asked for. @@ -70,6 +72,14 @@ ``ca_cert_file`` pinned to it so DuckDB opens that copy and not its own probe result. Granting the store itself was the first version of this and it let contract SQL replace the host trust store wherever the process can write it. + The copy's mode is a speed bump, not the barrier: measured, ``COPY TO`` with + its default options writes a temporary file and renames it over the target, so + 0400 stops nothing once the containing DIRECTORY is writable by the SQL -- + which is why the grant is dropped outright when the copy would land inside a + granted directory (``$TMPDIR`` pointed into the workspace, an ordinary CI or + direnv setup). The barrier that holds is that the host store is never granted; + the worst an attacker-chosen copy buys is the trust anchors of one connection + whose remote locations the contract named anyway. No verification setting is touched: an expired and a self-signed certificate are refused on 1.5.6 and 2.0.0 alike, with and without ``enable_server_cert_verification``, which governs the httplib client while @@ -182,6 +192,7 @@ from __future__ import annotations import contextlib +import logging import os import re import shutil @@ -210,6 +221,8 @@ from ._duckdb_compat import storage_compat_config from ._sql_safety import quote_ansi_string_literal, validate_ident +_log = logging.getLogger(__name__) + if TYPE_CHECKING: # pragma: no cover - typing only import duckdb @@ -1104,24 +1117,62 @@ def _private_ca_bundle(bundle: str) -> str: fix, which granted the store itself. So the store is never granted. The connection reads a copy that belongs to - it: 0400 so an unprivileged write is refused by the OS as well as by the - allowlist, under :func:`tempfile.mkdtemp`'s 0700 directory outside the - contract's tree, removed when the connection is gone. Root can still - overwrite the copy, and that is the whole point of the copy -- the damage - stops at one connection's own TLS trust, whose remote locations the contract - chose anyway, and no state outside the process is touched. + it, under :func:`tempfile.mkdtemp`'s 0700 directory, removed when the + connection object is finalised. + + What the 0400 is worth, stated exactly: it refuses an unprivileged write + through a plain open, and nothing more. The allowlist does NOT refuse a + write to this path -- it is in ``allowed_paths``, which is the premise of + this whole function -- and measured, ``COPY TO`` with its default options + writes a temporary file and renames it over the target, so the mode is + bypassed as soon as the containing directory is writable by the SQL. The + caller therefore drops the grant entirely when the directory would sit + inside a granted one (:func:`_ca_dir_within_a_grant`), and root bypasses the + mode in any case. + + The guarantee that does hold is the one that matters: the host store is not + in ``allowed_paths``, so nothing outside this process's own temporary + directory can be reached. An attacker who does replace the copy chooses the + trust anchors of one connection, over remote locations the contract named + anyway, and the file is gone when that connection is. The cost is one ~330 KB file copy per connection that declared a remote prefix, which is why :func:`_ca_bundle_for_remote` returns ``None`` for every other connection. """ directory = tempfile.mkdtemp(prefix=CA_DIR_PREFIX) - copy = os.path.join(directory, "ca-bundle.pem") - shutil.copyfile(bundle, copy) - os.chmod(copy, 0o400) + try: + copy = os.path.join(directory, "ca-bundle.pem") + shutil.copyfile(bundle, copy) + os.chmod(copy, 0o400) + except BaseException: + # The caller registers the remover only once this returns, so a failure + # here -- ENOSPC, an I/O error, or the candidate disappearing between + # the isfile probe and the copy -- would leak the directory for good. + shutil.rmtree(directory, ignore_errors=True) + raise return copy +def _ca_dir_within_a_grant(directory: str, allow: DuckDBAllowlist) -> Optional[str]: + """The granted directory ``directory`` sits inside, or ``None``. + + ``tempfile.mkdtemp`` honours ``$TMPDIR``, and a workspace-local ``$TMPDIR`` + is an ordinary CI, direnv or Nix arrangement. Where it points inside a + directory this connection grants, the copy is writable by the SQL whatever + its mode: measured, ``COPY TO`` with default options renamed a new file over + the 0400 copy on 1.5.6 and 2.0.0, turning 333 KB of trust anchors into 14 + bytes. Granting a trust store the SQL can replace is worse than granting + none, so the caller grants none. + """ + real = _fold(os.path.realpath(directory)) + for granted in allow.dirs: + root = _fold(os.path.realpath(granted)) + if _is_within(real, root): + return root + return None + + def _refuse_ungranted_dirs(con: Any, allow: DuckDBAllowlist, spill: str) -> None: """Refuse the connection if DuckDB granted a directory ``allow`` does not name. @@ -1314,16 +1365,38 @@ def _confine_connection( host_bundle = _ca_bundle_for_remote(allow) if host_bundle is not None: private = _private_ca_bundle(host_bundle) - spill_cleanup.append( - weakref.finalize(con, shutil.rmtree, os.path.dirname(private), ignore_errors=True) - ) - paths.append(private) - try: - con.execute(f"SET ca_cert_file = {quote_ansi_string_literal(private)}") - except duckdb.Error: - # httpfs is not loaded on this connection, so the setting does not - # exist and no TLS will happen either. Nothing to point at. - pass + directory = os.path.dirname(private) + exposed = _ca_dir_within_a_grant(directory, allow) + if exposed is not None: + # Writable by the SQL whatever the mode, so worse than no grant. + shutil.rmtree(directory, ignore_errors=True) + _log.warning( + "duckdb_ca_bundle_grant_skipped: the temporary directory would sit inside " + "the granted directory %s, where contract SQL could replace the trust " + "store this connection reads. A remote read may fail on DuckDB 2.0 with " + "'Cannot read a CA certificate bundle'; point TMPDIR outside the granted " + "tree to restore it.", + exposed, + ) + else: + try: + # GLOBAL, not the session default: ca_cert_file is a LOCAL-scope + # setting, so a session-scoped pin leaves con.cursor() with '' + # and its remote reads failing on 2.0.0 (measured). The lock + # still refuses a change afterwards, GLOBAL or not. + con.execute(f"SET GLOBAL ca_cert_file = {quote_ansi_string_literal(private)}") + except duckdb.Error: + # httpfs is not loaded, so the setting is not in the catalog and + # DuckDB will never open this copy. Granting it would leave a + # writable file in allowed_paths for a benefit that cannot + # exist -- the azure extension takes exactly this path, and its + # own TLS does not read the bundle from here. + shutil.rmtree(directory, ignore_errors=True) + else: + spill_cleanup.append( + weakref.finalize(con, shutil.rmtree, directory, ignore_errors=True) + ) + paths.append(private) con.execute(f"SET allowed_paths = {_sql_list(paths)}") con.execute("SET enable_external_access = false") con.execute("SET lock_configuration = true") @@ -1399,7 +1472,9 @@ def sandbox_refusal_hint(allow: DuckDBAllowlist, refusal: Optional[BaseException # Named without its path: what SQL can read is the private copy # :func:`_private_ca_bundle` made for this connection, not the host store # this returns, and printing the store would point the reader at a file the - # SQL was in fact refused. + # SQL was in fact refused. It can overstate by one line -- the grant is + # dropped when the pin cannot land or the copy would be exposed -- which is + # the safe direction for a sentence about what SQL may read. also = ( ", and a private read-only copy of the host's TLS trust store, which a remote " "read needs" if _ca_bundle_for_remote(allow) diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py index 507964c6..748ef2af 100644 --- a/tests/providers/test_duckdb_sandbox_ca_bundle.py +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -46,41 +46,53 @@ hint does disclose that a copy of the trust store is readable, because telling the user otherwise would be false. -Every test here but one is offline, and that one -(``test_the_candidate_list_matches_the_one_duckdb_names``) needs httpfs, so it -skips when the extension cannot be installed, as its sibling in -``test_duckdb_sandbox.py`` does. The real network proof is the CLI run in the PR -description; what CI can check without a network is the read-back, and on 2.0.0 +Nothing here reads a remote location, but the tests that assert a GRANT do load +httpfs -- the grant is gated on the ``ca_cert_file`` pin, and that setting lives +in the extension -- so they go through :func:`_granting_connection`, which skips +when httpfs cannot be installed, as ``test_duckdb_sandbox.py`` does. On a cold +machine that is a ~19 MB download, once. The tests that assert NO grant need no +extension at all. The real network proof is the CLI run in the PR description; +what CI can check without reading anything remote is the read-back, and on 2.0.0 the refusal itself, which arrives before any connection is attempted. Each mechanism was reverted on duckdb 2.0.0 and the failures recorded, not -predicted: - -=================================== ==================================== -reverted tests that fail -=================================== ==================================== -the probe returns ``None`` 8 -the probe returns the directory 9 -the remote-prefix guard is dropped 2 (both local-read tests) -the copy is replaced by the store 4 -``chmod 0400`` is dropped 2 -the ``ca_cert_file`` pin is dropped 1 -- AND a live ``s3://`` read on - 2.0.0 fails with the original - ``Cannot read a CA certificate - bundle``, which is what makes the pin - load-bearing rather than belt-and-braces -the cleanup finalize is dropped 1 -=================================== ==================================== - -Two of those were worth the trouble of checking. Returning the bundle's -directory did NOT fail ``test_the_grant_is_one_file_and_not_a_directory`` until -that test was made to check the granted entry itself -- a directory handed to +predicted. Re-measured against this file's 20 tests whenever a test was added, +because a table that drifts teaches a maintainer to distrust the count: + +===================================== ============== +reverted tests that fail +===================================== ============== +the probe returns ``None`` 11 +the probe returns the directory 13 +the remote-prefix guard is dropped 1 +the copy is replaced by the store 7 +``chmod 0400`` is dropped 2 +the ``ca_cert_file`` pin is dropped 3 +the pin is session-scoped, not GLOBAL 1 +the exposed-directory check is dropped 1 +the grant is kept when the pin fails 1 +the cleanup finalize is dropped 1 +a failed copy is not cleaned up 1 +===================================== ============== + +Dropping the remote-prefix guard fails only ONE test now, where it used to fail +two: the grant is gated on the pin as well, so a local connection (which loads +no httpfs) gets nothing even with the guard gone, and +``test_a_local_refusal_hint_says_nothing_about_a_trust_store`` is what catches +it. Two mechanisms covering one mistake is the point; the count moving is not a +regression. + +Three rows were worth the trouble of checking. Returning the bundle's directory +did NOT fail ``test_the_grant_is_one_file_and_not_a_directory`` until that test +was made to check the granted entry itself -- a directory handed to ``allowed_paths`` never reaches ``allowed_directories``, so reading the latter -back could not see it. And granting the host store did NOT fail the write test -until it asserted the store's absence from ``allowed_paths``: the ``COPY`` is -refused on a developer machine either way, because ``/etc/ssl/cert.pem`` is -root-owned. In the root container the threat model is about, it would not have -been refused at all. +back could not see it. Granting the host store did NOT fail the write test until +it asserted the store's absence from ``allowed_paths``: the ``COPY`` is refused +on a developer machine either way, because ``/etc/ssl/cert.pem`` is root-owned, +and in the root container the threat model is about it would not have been +refused at all. And the pin being session-scoped instead of GLOBAL fails exactly +one test, ``test_a_cursor_inherits_the_pin``, which exists because nothing else +in the file would have noticed. """ from __future__ import annotations @@ -90,6 +102,7 @@ import posixpath import re import sys +import tempfile from pathlib import Path from typing import Any, List @@ -134,6 +147,27 @@ def _granted_paths(con: Any) -> List[str]: return [sandbox._fold(os.path.realpath(p)) for p in _setting(con, "allowed_paths")] +def _granting_connection(allow: DuckDBAllowlist) -> Any: + """A sandboxed connection that actually carries the CA grant. + + The grant is gated on the ``ca_cert_file`` pin landing, and that setting + only exists once httpfs is loaded -- so a connection without the extension + gets no copy at all, deliberately (there is no TLS for it to serve). Every + test that asserts a grant therefore loads httpfs, and skips where the + extension cannot be installed, as ``test_duckdb_sandbox.py`` does. + """ + try: + return secure_duckdb_connect(":memory:", allow=allow, extensions=["httpfs"]) + except duckdb.Error: + pytest.skip("httpfs is not installable here (offline)") + + +def _ca_directories() -> List[str]: + """The CA copy directories currently in $TMPDIR.""" + root = tempfile.gettempdir() + return [d for d in os.listdir(root) if d.startswith(sandbox.CA_DIR_PREFIX)] + + def _host_bundle() -> str: """The candidate this host actually has, or skip.""" for candidate in sandbox._CA_BUNDLE_CANDIDATES: @@ -154,7 +188,7 @@ def test_a_declared_remote_prefix_grants_one_private_copy_of_the_bundle() -> Non """ bundle = _host_bundle() - con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) try: granted = _granted_paths(con) assert len(granted) == 1, granted @@ -183,14 +217,7 @@ def test_duckdb_is_pinned_at_the_copy() -> None: """ _host_bundle() - try: - con = secure_duckdb_connect( - ":memory:", - allow=DuckDBAllowlist.none().with_locations(REMOTE), - extensions=["httpfs"], - ) - except duckdb.Error: - pytest.skip("httpfs is not installable here (offline)") + con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) try: # Both sides through the same fold: _granted_paths() case-folds the way # the sandbox does, and the raw setting does not. @@ -212,7 +239,7 @@ def test_duckdb_can_read_the_copy_it_was_granted_and_not_the_host_store() -> Non """ bundle = _host_bundle() - con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) try: copy = _granted_paths(con)[0] assert con.execute(f"SELECT count(*) FROM read_text('{copy}')").fetchall()[0][0] == 1 @@ -246,7 +273,7 @@ def test_the_granted_copy_cannot_be_written_and_the_host_store_is_not_granted() with open(bundle, "rb") as host: before = host.read() - con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) try: copy = _granted_paths(con)[0] # The barrier that does not depend on who we are. Asserted because the @@ -279,11 +306,17 @@ def test_the_granted_copy_cannot_be_written_and_the_host_store_is_not_granted() def test_the_copy_is_removed_once_the_connection_is_gone() -> None: """One ~330 KB file per remote connection, not a leak in $TMPDIR. - The same ``weakref.finalize`` teardown the spill directory uses. + The same ``weakref.finalize`` teardown the spill directory uses, with the + same limits, measured: ``con.close()`` ALONE does not remove it, because the + finalizer is keyed on the connection object, so a caller holding a closed + connection (a cached handle, or a traceback pinning the frame) keeps the + copy until that reference goes. An unclosed connection is cleaned at normal + interpreter exit; ``os._exit`` or a kill leaves it for ``$TMPDIR``'s reaper, + exactly as the spill directory documents. """ _host_bundle() - con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) directory = os.path.dirname(_granted_paths(con)[0]) assert os.path.isdir(directory) @@ -330,7 +363,7 @@ def test_a_declared_file_and_the_ca_copy_both_reach_allowed_paths(tmp_path: Path declared.write_text("a,b\n1,2\n", encoding="utf-8") allow = DuckDBAllowlist.none().with_paths(str(declared)).with_remote(REMOTE_PREFIX) - con = secure_duckdb_connect(":memory:", allow=allow) + con = _granting_connection(allow) try: granted = _granted_paths(con) assert len(granted) == 2, granted @@ -349,7 +382,7 @@ def test_the_grant_is_one_file_and_not_a_directory() -> None: _host_bundle() allow = DuckDBAllowlist.none().with_locations(REMOTE) - con = secure_duckdb_connect(":memory:", allow=allow) + con = _granting_connection(allow) try: # Each granted path is a FILE. Asserted on the entry itself and not # only on ``allowed_directories``: a directory handed to @@ -384,7 +417,7 @@ def test_the_allowlist_the_caller_passed_is_not_widened() -> None: bundle = _host_bundle() allow = DuckDBAllowlist.none().with_locations(REMOTE) - con = secure_duckdb_connect(":memory:", allow=allow) + con = _granting_connection(allow) try: assert allow.paths == () hint = sandbox_refusal_hint(allow) @@ -458,6 +491,98 @@ def test_a_candidate_that_is_not_a_file_is_skipped( assert sandbox._ca_bundle_for_remote(allow) == str(real) +def test_a_connection_without_httpfs_gets_no_grant_and_leaves_nothing(tmp_path: Path) -> None: + """No pin, no grant -- and no copy left on disk either. + + ``ca_cert_file`` is an httpfs setting, so on a connection that did not load + the extension the pin cannot land and DuckDB will never open the copy. + Granting it anyway would put a writable file in ``allowed_paths`` for a + benefit that cannot exist. The azure extension takes exactly this path + (``cli/discover/filesystem.py`` loads ``azure``, not ``httpfs``, for an + ``azure://`` URL), which is how the review found it. + """ + before = len(_ca_directories()) + + con = secure_duckdb_connect(":memory:", allow=DuckDBAllowlist.none().with_locations(REMOTE)) + try: + assert _granted_paths(con) == [] + assert len(_ca_directories()) == before + finally: + con.close() + + +def test_a_tmpdir_inside_a_granted_directory_gets_no_grant( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """A trust store the SQL could replace is worse than no trust store. + + ``tempfile.mkdtemp`` honours ``$TMPDIR``, and a workspace-local ``$TMPDIR`` + is an ordinary CI, direnv or Nix arrangement. Measured there: ``COPY TO`` + with its DEFAULT options writes a temporary file and renames it over the + target, so the copy's 0400 stops nothing once its directory is writable by + the SQL -- 333 KB of trust anchors became 14 bytes on 1.5.6 and 2.0.0. The + grant is dropped instead, loudly, and nothing is left behind. + """ + _host_bundle() + granted_dir = tmp_path / "workspace" + (granted_dir / ".tmp").mkdir(parents=True) + monkeypatch.setenv("TMPDIR", str(granted_dir / ".tmp") + os.sep) + monkeypatch.setattr("tempfile.tempdir", None) + allow = DuckDBAllowlist.none().with_dirs(str(granted_dir)).with_remote(REMOTE_PREFIX) + + with caplog.at_level("WARNING"): + con = secure_duckdb_connect(":memory:", allow=allow, extensions=["httpfs"]) + try: + assert _granted_paths(con) == [] + assert not list((granted_dir / ".tmp").glob(sandbox.CA_DIR_PREFIX + "*")) + assert "duckdb_ca_bundle_grant_skipped" in caplog.text + finally: + con.close() + + +def test_a_failed_copy_leaves_no_directory_behind(monkeypatch: pytest.MonkeyPatch) -> None: + """The remover is registered by the caller, so the copy has to clean up itself. + + ``mkdtemp`` runs first and the ``weakref.finalize`` only once + :func:`_private_ca_bundle` has returned, so a failure in between -- ENOSPC, + an I/O error, or the candidate disappearing between the ``isfile`` probe and + the copy -- used to leak the directory for the lifetime of ``$TMPDIR``. + """ + bundle = _host_bundle() + before = len(_ca_directories()) + + def _boom(*_a: Any, **_k: Any) -> Any: + raise OSError(28, "No space left on device") + + monkeypatch.setattr(sandbox.shutil, "copyfile", _boom) + + with pytest.raises(OSError): + sandbox._private_ca_bundle(bundle) + + assert len(_ca_directories()) == before + + +def test_a_cursor_inherits_the_pin(tmp_path: Path) -> None: + """``ca_cert_file`` is LOCAL scope, so the pin has to be GLOBAL. + + Measured with a session-scoped ``SET``: ``con.cursor()`` reported + ``ca_cert_file = ''`` and its remote read failed on 2.0.0 with the original + ``Cannot read a CA certificate bundle`` -- ``allowed_paths`` is GLOBAL so + the cursor saw the copy, but nothing pointed DuckDB at it, and the + configuration is locked so no caller could set it either. + """ + _host_bundle() + + con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + try: + copy = _granted_paths(con)[0] + cursor = con.cursor() + pinned = sandbox._fold(os.path.realpath(_setting(cursor, "ca_cert_file"))) + assert pinned == copy + finally: + con.close() + + def test_a_host_with_no_candidate_grants_nothing( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From fdfbe5753274c0cbfcfbb522eee4ef6a165de63d Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 04:18:08 +0100 Subject: [PATCH 6/7] test(duckdb): the CA tests stop depending on a shared $TMPDIR, and three claims are corrected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round three of the review. 23 findings, 2 confirmed — both mine, both in the tests, and no production defect. Fixed, with the measured prose errors it also turned up. **Both $TMPDIR-counting tests were flaky under the project's own CI command.** `_ca_directories()` listed the SHARED system temp directory, and ci.yml runs the unit suite with `pytest -n auto --dist loadscope`: any other worker opening a sandboxed connection with a remote prefix creates and tears one of these down, so the count moved in both directions. The review measured a failure about one run in six under concurrency. A `_private_tmpdir` fixture now points `$TMPDIR` at the test's own directory, so only the copies the test provokes exist there. Verified: six consecutive `-n 4 --dist loadscope` runs, 20 passed each, against six background processes churning CA directories in the shared `$TMPDIR`. **The exposed-TMPDIR test was RED, not skipped, where httpfs cannot be installed.** It called `secure_duckdb_connect(..., extensions=["httpfs"])` directly instead of the guarded helper — on an offline runner the INSTALL raises and the test fails, and this file is in the `duckdb-floor` lane, so a download blip would have failed that leg. The helper is now `_with_httpfs` (it is for any test whose subject is the grant, not only the ones expecting one) and every httpfs test goes through it. Prose, each measured before changing: * "the host store is not in `allowed_paths`, so nothing outside this process's own temporary directory can be reached" — false as written, in the docstring and again in the CHANGELOG. The same `SET allowed_paths` grants everything the call site declared; the sentence is about the CA grant alone and now says so. * The comment justifying `SET GLOBAL` said `ca_cert_file` is "a LOCAL-scope setting". `duckdb_settings()` reports it GLOBAL on 1.5.1, 1.5.6 and 2.0.0 until a plain `SET` puts a local value there — the behaviour is real and the GLOBAL keyword is necessary, but what makes it necessary is SET's own scope resolution. A maintainer checking the scope column would have found the opposite of what the comment claimed. * `sandbox_refusal_hint` still told the user "a private read-only copy", the claim this branch retracts everywhere else — the CHANGELOG and the module docstring both now say the mode is a speed bump, not a write bar. It was the one place the superseded wording survived, and the one place a user reads. A new assertion fails if it comes back. The pin-failure branch was the only silent one; it now logs at DEBUG, not WARNING, because for the azure and local paths dropping the grant is the correct outcome and a warning would cry wolf on every run — but the day a DuckDB release renames or refuses that setting, remote reads go back to failing with 'Cannot read a CA certificate bundle' and nothing else would say why. 20 tests on 2.0.0, 19 + 1 skip on 1.5.6 and 1.5.1; providers + output_ports 2552 passed with only the two pre-existing workspace-root failures. --- CHANGELOG.md | 5 +- fluid_build/providers/_duckdb_sandbox.py | 2 +- .../test_duckdb_sandbox_ca_bundle.py | 75 ++++++++++++++----- 3 files changed, 61 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e92c992..b11121cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,8 +81,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 unprivileged `fluid` cannot write it, but `fluid generate ci` emits a `python:3.12-slim` job that runs as root, and a contract built there could have installed its own CA into that container's trust store. Granting a copy removes the reach, which is the part that mattered: the host store is - not in `allowed_paths` at all, so nothing outside this process's own temporary directory can be - written. It does **not** make the copy itself immutable — the mode is a speed bump. `COPY TO` with + not in `allowed_paths` at all, so the CA grant writes nothing outside this process's own temporary + directory (the connection still reaches whatever the contract declared — that is the allowlist's + job, unchanged here). It does **not** make the copy itself immutable — the mode is a speed bump. `COPY TO` with its default options writes a temporary file and renames it over the target, so 0400 stops nothing once the containing directory is writable by the SQL, and root ignores the mode regardless. The grant is therefore dropped outright when the copy would land inside a granted directory (`$TMPDIR` diff --git a/fluid_build/providers/_duckdb_sandbox.py b/fluid_build/providers/_duckdb_sandbox.py index d9dec4be..2e2804b5 100644 --- a/fluid_build/providers/_duckdb_sandbox.py +++ b/fluid_build/providers/_duckdb_sandbox.py @@ -1476,7 +1476,7 @@ def sandbox_refusal_hint(allow: DuckDBAllowlist, refusal: Optional[BaseException # dropped when the pin cannot land or the copy would be exposed -- which is # the safe direction for a sentence about what SQL may read. also = ( - ", and a private read-only copy of the host's TLS trust store, which a remote " "read needs" + ", and a private copy of the host's TLS trust store, which a remote read needs" if _ca_bundle_for_remote(allow) else "" ) diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py index 748ef2af..ec33fa7b 100644 --- a/tests/providers/test_duckdb_sandbox_ca_bundle.py +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -147,14 +147,19 @@ def _granted_paths(con: Any) -> List[str]: return [sandbox._fold(os.path.realpath(p)) for p in _setting(con, "allowed_paths")] -def _granting_connection(allow: DuckDBAllowlist) -> Any: - """A sandboxed connection that actually carries the CA grant. +def _with_httpfs(allow: DuckDBAllowlist) -> Any: + """A sandboxed connection with httpfs loaded, or a skip. The grant is gated on the ``ca_cert_file`` pin landing, and that setting only exists once httpfs is loaded -- so a connection without the extension gets no copy at all, deliberately (there is no TLS for it to serve). Every - test that asserts a grant therefore loads httpfs, and skips where the - extension cannot be installed, as ``test_duckdb_sandbox.py`` does. + test whose SUBJECT is the grant therefore loads httpfs, whether it expects + one or not, and skips where the extension cannot be installed, as + ``test_duckdb_sandbox.py`` does. Going through here rather than calling + ``secure_duckdb_connect`` with ``extensions=["httpfs"]`` is what makes the + difference between a skip and a red build on an offline runner -- and this + file is in the ``duckdb-floor`` CI lane, where a download blip would + otherwise fail it. """ try: return secure_duckdb_connect(":memory:", allow=allow, extensions=["httpfs"]) @@ -163,11 +168,34 @@ def _granting_connection(allow: DuckDBAllowlist) -> Any: def _ca_directories() -> List[str]: - """The CA copy directories currently in $TMPDIR.""" + """The CA copy directories currently in $TMPDIR. + + Only meaningful under :func:`_private_tmpdir`. Counting the SHARED system + temp directory is what the first version of these tests did, and CI runs + the unit suite with ``pytest -n auto``: every other worker opening a + sandboxed connection with a remote prefix creates and tears down one of + these, so the count moved in both directions and the assertions failed + about one run in six under concurrency (measured by the review). + """ root = tempfile.gettempdir() return [d for d in os.listdir(root) if d.startswith(sandbox.CA_DIR_PREFIX)] +@pytest.fixture +def _private_tmpdir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Point ``$TMPDIR`` at this test's own directory. + + ``tempfile.mkdtemp`` honours ``$TMPDIR``, so the copies this test provokes + are the only ones :func:`_ca_directories` can see. Deterministic under + xdist, where the shared temp directory is not. + """ + private = tmp_path / "tmp" + private.mkdir() + monkeypatch.setenv("TMPDIR", str(private) + os.sep) + monkeypatch.setattr("tempfile.tempdir", None) + return private + + def _host_bundle() -> str: """The candidate this host actually has, or skip.""" for candidate in sandbox._CA_BUNDLE_CANDIDATES: @@ -188,7 +216,7 @@ def test_a_declared_remote_prefix_grants_one_private_copy_of_the_bundle() -> Non """ bundle = _host_bundle() - con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + con = _with_httpfs(DuckDBAllowlist.none().with_locations(REMOTE)) try: granted = _granted_paths(con) assert len(granted) == 1, granted @@ -217,7 +245,7 @@ def test_duckdb_is_pinned_at_the_copy() -> None: """ _host_bundle() - con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + con = _with_httpfs(DuckDBAllowlist.none().with_locations(REMOTE)) try: # Both sides through the same fold: _granted_paths() case-folds the way # the sandbox does, and the raw setting does not. @@ -239,7 +267,7 @@ def test_duckdb_can_read_the_copy_it_was_granted_and_not_the_host_store() -> Non """ bundle = _host_bundle() - con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + con = _with_httpfs(DuckDBAllowlist.none().with_locations(REMOTE)) try: copy = _granted_paths(con)[0] assert con.execute(f"SELECT count(*) FROM read_text('{copy}')").fetchall()[0][0] == 1 @@ -273,7 +301,7 @@ def test_the_granted_copy_cannot_be_written_and_the_host_store_is_not_granted() with open(bundle, "rb") as host: before = host.read() - con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + con = _with_httpfs(DuckDBAllowlist.none().with_locations(REMOTE)) try: copy = _granted_paths(con)[0] # The barrier that does not depend on who we are. Asserted because the @@ -316,7 +344,7 @@ def test_the_copy_is_removed_once_the_connection_is_gone() -> None: """ _host_bundle() - con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + con = _with_httpfs(DuckDBAllowlist.none().with_locations(REMOTE)) directory = os.path.dirname(_granted_paths(con)[0]) assert os.path.isdir(directory) @@ -363,7 +391,7 @@ def test_a_declared_file_and_the_ca_copy_both_reach_allowed_paths(tmp_path: Path declared.write_text("a,b\n1,2\n", encoding="utf-8") allow = DuckDBAllowlist.none().with_paths(str(declared)).with_remote(REMOTE_PREFIX) - con = _granting_connection(allow) + con = _with_httpfs(allow) try: granted = _granted_paths(con) assert len(granted) == 2, granted @@ -382,7 +410,7 @@ def test_the_grant_is_one_file_and_not_a_directory() -> None: _host_bundle() allow = DuckDBAllowlist.none().with_locations(REMOTE) - con = _granting_connection(allow) + con = _with_httpfs(allow) try: # Each granted path is a FILE. Asserted on the entry itself and not # only on ``allowed_directories``: a directory handed to @@ -417,12 +445,15 @@ def test_the_allowlist_the_caller_passed_is_not_widened() -> None: bundle = _host_bundle() allow = DuckDBAllowlist.none().with_locations(REMOTE) - con = _granting_connection(allow) + con = _with_httpfs(allow) try: assert allow.paths == () hint = sandbox_refusal_hint(allow) assert "trust store" in hint assert bundle not in hint + # Not "read-only": the copy's mode is a speed bump, not a write bar + # (COPY TO renames over it), and this sentence is what the user reads. + assert "read-only" not in hint finally: con.close() @@ -491,7 +522,9 @@ def test_a_candidate_that_is_not_a_file_is_skipped( assert sandbox._ca_bundle_for_remote(allow) == str(real) -def test_a_connection_without_httpfs_gets_no_grant_and_leaves_nothing(tmp_path: Path) -> None: +def test_a_connection_without_httpfs_gets_no_grant_and_leaves_nothing( + _private_tmpdir: Path, +) -> None: """No pin, no grant -- and no copy left on disk either. ``ca_cert_file`` is an httpfs setting, so on a connection that did not load @@ -531,7 +564,7 @@ def test_a_tmpdir_inside_a_granted_directory_gets_no_grant( allow = DuckDBAllowlist.none().with_dirs(str(granted_dir)).with_remote(REMOTE_PREFIX) with caplog.at_level("WARNING"): - con = secure_duckdb_connect(":memory:", allow=allow, extensions=["httpfs"]) + con = _with_httpfs(allow) try: assert _granted_paths(con) == [] assert not list((granted_dir / ".tmp").glob(sandbox.CA_DIR_PREFIX + "*")) @@ -540,7 +573,9 @@ def test_a_tmpdir_inside_a_granted_directory_gets_no_grant( con.close() -def test_a_failed_copy_leaves_no_directory_behind(monkeypatch: pytest.MonkeyPatch) -> None: +def test_a_failed_copy_leaves_no_directory_behind( + _private_tmpdir: Path, monkeypatch: pytest.MonkeyPatch +) -> None: """The remover is registered by the caller, so the copy has to clean up itself. ``mkdtemp`` runs first and the ``weakref.finalize`` only once @@ -563,7 +598,7 @@ def _boom(*_a: Any, **_k: Any) -> Any: def test_a_cursor_inherits_the_pin(tmp_path: Path) -> None: - """``ca_cert_file`` is LOCAL scope, so the pin has to be GLOBAL. + """A plain SET lands in LOCAL scope, so the pin has to be GLOBAL. Measured with a session-scoped ``SET``: ``con.cursor()`` reported ``ca_cert_file = ''`` and its remote read failed on 2.0.0 with the original @@ -573,8 +608,12 @@ def test_a_cursor_inherits_the_pin(tmp_path: Path) -> None: """ _host_bundle() - con = _granting_connection(DuckDBAllowlist.none().with_locations(REMOTE)) + con = _with_httpfs(DuckDBAllowlist.none().with_locations(REMOTE)) try: + # NB the setting REPORTS scope GLOBAL in duckdb_settings() until a plain + # SET puts a local value there, measured on 1.5.1, 1.5.6 and 2.0.0 -- it + # is SET's own scope resolution that makes the GLOBAL keyword necessary, + # not a declared LOCAL scope. copy = _granted_paths(con)[0] cursor = con.cursor() pinned = sandbox._fold(os.path.realpath(_setting(cursor, "ca_cert_file"))) From cea70535ac5e597fd0fe64f442df5214bb4a004c Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 06:10:30 +0100 Subject: [PATCH 7/7] test(duckdb): the two skip helpers raise explicitly CodeQL flagged both on #759: a helper that returns a value and then ends on `pytest.skip(...)` reads as mixing explicit returns with an implicit `return None`, because the analysis cannot see that `pytest.skip` raises. `raise pytest.skip.Exception(...)` is the same class by the public alias, so the behaviour is unchanged and the exceptional path is explicit. Proved rather than assumed: with the candidate list forced empty and with the connect call made to raise, both helpers report SKIPPED with their own messages, and the `raise AssertionError` placed after each call never runs. 20 tests on 2.0.0, 19 + 1 skip on 1.5.6 and 1.5.1. --- tests/providers/test_duckdb_sandbox_ca_bundle.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/providers/test_duckdb_sandbox_ca_bundle.py b/tests/providers/test_duckdb_sandbox_ca_bundle.py index ec33fa7b..7fa9bafc 100644 --- a/tests/providers/test_duckdb_sandbox_ca_bundle.py +++ b/tests/providers/test_duckdb_sandbox_ca_bundle.py @@ -164,7 +164,10 @@ def _with_httpfs(allow: DuckDBAllowlist) -> Any: try: return secure_duckdb_connect(":memory:", allow=allow, extensions=["httpfs"]) except duckdb.Error: - pytest.skip("httpfs is not installable here (offline)") + # ``raise pytest.skip.Exception`` rather than ``pytest.skip(...)``, which + # raises the same class: a helper that otherwise returns a value and then + # falls off the end reads to CodeQL as an implicit ``return None``. + raise pytest.skip.Exception("httpfs is not installable here (offline)") from None def _ca_directories() -> List[str]: @@ -201,7 +204,8 @@ def _host_bundle() -> str: for candidate in sandbox._CA_BUNDLE_CANDIDATES: if os.path.isfile(candidate): return candidate - pytest.skip("no CA bundle among DuckDB's candidates on this host") + # Raised, not ``pytest.skip(...)``, for the reason given in _with_httpfs. + raise pytest.skip.Exception("no CA bundle among DuckDB's candidates on this host") # ── the defect ───────────────────────────────────────────────────────────